From e787bc463cc1fe3f51b0cd7bf540236318f69cf1 Mon Sep 17 00:00:00 2001 From: Alexander Shishkin Date: Thu, 31 Mar 2016 16:30:15 +0300 Subject: [PATCH 01/19] MAINTAINERS: Add a git tree for the stm class So that people know where their patches go. Signed-off-by: Alexander Shishkin Reviewed-by: Laurent Fert --- MAINTAINERS | 1 + 1 file changed, 1 insertion(+) diff --git a/MAINTAINERS b/MAINTAINERS index 03e00c7c88eb..4b511b25d179 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -9742,6 +9742,7 @@ F: drivers/mmc/host/dw_mmc* SYSTEM TRACE MODULE CLASS M: Alexander Shishkin S: Maintained +T: git git://git.kernel.org/pub/scm/linux/kernel/git/ash/stm.git F: Documentation/trace/stm.txt F: drivers/hwtracing/stm/ F: include/linux/stm.h From f57af6df6af23c086cfe023896822200eee48dd1 Mon Sep 17 00:00:00 2001 From: Chunyan Zhang Date: Mon, 28 Mar 2016 15:55:42 +0800 Subject: [PATCH 02/19] stm class: Fix integer boundary checks for master range Master IDs are of unsigned int type, yet in the configfs policy code we're validating user's input against INT_MAX. This is both pointless and misleading as the real limits are imposed by the stm device's [sw_start..sw_end] (which are also limited by the spec to be no larger than 2^16-1). Clean this up by getting rid of the redundant comparisons. Signed-off-by: Chunyan Zhang Signed-off-by: Alexander Shishkin Reviewed-by: Laurent Fert --- drivers/hwtracing/stm/policy.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/hwtracing/stm/policy.c b/drivers/hwtracing/stm/policy.c index 1db189657b2b..e8b50b1ac619 100644 --- a/drivers/hwtracing/stm/policy.c +++ b/drivers/hwtracing/stm/policy.c @@ -107,8 +107,7 @@ stp_policy_node_masters_store(struct config_item *item, const char *page, goto unlock; /* must be within [sw_start..sw_end], which is an inclusive range */ - if (first > INT_MAX || last > INT_MAX || first > last || - first < stm->data->sw_start || + if (first > last || first < stm->data->sw_start || last > stm->data->sw_end) { ret = -ERANGE; goto unlock; From 118b4515aa6916ee7751f29c8b2a3af95abc9783 Mon Sep 17 00:00:00 2001 From: Alexander Shishkin Date: Fri, 4 Mar 2016 16:22:33 +0200 Subject: [PATCH 03/19] stm class: dummy_stm: Make nr_dummies parameter read-only Changing nr_dummies after the module has been loaded doesn't actually change anything, so just make it read-only. Reported-by: Alan Cox Signed-off-by: Alexander Shishkin Reviewed-by: Laurent Fert --- drivers/hwtracing/stm/dummy_stm.c | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/drivers/hwtracing/stm/dummy_stm.c b/drivers/hwtracing/stm/dummy_stm.c index 310adf57e7a1..a86612d989f9 100644 --- a/drivers/hwtracing/stm/dummy_stm.c +++ b/drivers/hwtracing/stm/dummy_stm.c @@ -46,9 +46,7 @@ static struct stm_data dummy_stm[DUMMY_STM_MAX]; static int nr_dummies = 4; -module_param(nr_dummies, int, 0600); - -static unsigned int dummy_stm_nr; +module_param(nr_dummies, int, 0400); static unsigned int fail_mode; @@ -65,12 +63,12 @@ static int dummy_stm_link(struct stm_data *data, unsigned int master, static int dummy_stm_init(void) { - int i, ret = -ENOMEM, __nr_dummies = ACCESS_ONCE(nr_dummies); + int i, ret = -ENOMEM; - if (__nr_dummies < 0 || __nr_dummies > DUMMY_STM_MAX) + if (nr_dummies < 0 || nr_dummies > DUMMY_STM_MAX) return -EINVAL; - for (i = 0; i < __nr_dummies; i++) { + for (i = 0; i < nr_dummies; i++) { dummy_stm[i].name = kasprintf(GFP_KERNEL, "dummy_stm.%d", i); if (!dummy_stm[i].name) goto fail_unregister; @@ -86,8 +84,6 @@ static int dummy_stm_init(void) goto fail_free; } - dummy_stm_nr = __nr_dummies; - return 0; fail_unregister: @@ -105,7 +101,7 @@ static void dummy_stm_exit(void) { int i; - for (i = 0; i < dummy_stm_nr; i++) { + for (i = 0; i < nr_dummies; i++) { stm_unregister_device(&dummy_stm[i]); kfree(dummy_stm[i].name); } From c8be4899449c0b27bc5daf71742cd601b2b3b4e3 Mon Sep 17 00:00:00 2001 From: Alexander Shishkin Date: Fri, 4 Mar 2016 16:22:33 +0200 Subject: [PATCH 04/19] stm class: stm_heartbeat: Make nr_devs parameter read-only Changing nr_devs after the module has been loaded doesn't actually change anything, so just make it read-only. Reported-by: Alan Cox Signed-off-by: Alexander Shishkin Reviewed-by: Laurent Fert --- drivers/hwtracing/stm/heartbeat.c | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/drivers/hwtracing/stm/heartbeat.c b/drivers/hwtracing/stm/heartbeat.c index 0133571b506f..3da7b673aab2 100644 --- a/drivers/hwtracing/stm/heartbeat.c +++ b/drivers/hwtracing/stm/heartbeat.c @@ -26,7 +26,7 @@ static int nr_devs = 4; static int interval_ms = 10; -module_param(nr_devs, int, 0600); +module_param(nr_devs, int, 0400); module_param(interval_ms, int, 0600); static struct stm_heartbeat { @@ -35,8 +35,6 @@ static struct stm_heartbeat { unsigned int active; } stm_heartbeat[STM_HEARTBEAT_MAX]; -static unsigned int nr_instances; - static const char str[] = "heartbeat stm source driver is here to serve you"; static enum hrtimer_restart stm_heartbeat_hrtimer_handler(struct hrtimer *hr) @@ -74,12 +72,12 @@ static void stm_heartbeat_unlink(struct stm_source_data *data) static int stm_heartbeat_init(void) { - int i, ret = -ENOMEM, __nr_instances = ACCESS_ONCE(nr_devs); + int i, ret = -ENOMEM; - if (__nr_instances < 0 || __nr_instances > STM_HEARTBEAT_MAX) + if (nr_devs < 0 || nr_devs > STM_HEARTBEAT_MAX) return -EINVAL; - for (i = 0; i < __nr_instances; i++) { + for (i = 0; i < nr_devs; i++) { stm_heartbeat[i].data.name = kasprintf(GFP_KERNEL, "heartbeat.%d", i); if (!stm_heartbeat[i].data.name) @@ -98,8 +96,6 @@ static int stm_heartbeat_init(void) goto fail_free; } - nr_instances = __nr_instances; - return 0; fail_unregister: @@ -116,7 +112,7 @@ static void stm_heartbeat_exit(void) { int i; - for (i = 0; i < nr_instances; i++) { + for (i = 0; i < nr_devs; i++) { stm_source_unregister_device(&stm_heartbeat[i].data); kfree(stm_heartbeat[i].data.name); } From 8fa11d1c1322f3de40a0e3f3f3e57436a204fcc4 Mon Sep 17 00:00:00 2001 From: Alexander Shishkin Date: Fri, 4 Mar 2016 16:30:24 +0200 Subject: [PATCH 05/19] stm class: Remove a pointless line No point in explicitly setting something to zero right after we explicitly checked that it is zero. Fix this. Reported-by: Alan Cox Signed-off-by: Alexander Shishkin Reviewed-by: Laurent Fert --- drivers/hwtracing/stm/core.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/drivers/hwtracing/stm/core.c b/drivers/hwtracing/stm/core.c index de80d45d8df9..18f176eac06d 100644 --- a/drivers/hwtracing/stm/core.c +++ b/drivers/hwtracing/stm/core.c @@ -546,8 +546,6 @@ static int stm_char_policy_set_ioctl(struct stm_file *stmf, void __user *arg) if (ret) goto err_free; - ret = 0; - if (stm->data->link) ret = stm->data->link(stm->data, stmf->output.master, stmf->output.channel); From cbe4a61d1ddc4790d950ca8c33ef79ee68ef5e2b Mon Sep 17 00:00:00 2001 From: Alexander Shishkin Date: Fri, 4 Mar 2016 16:36:10 +0200 Subject: [PATCH 06/19] stm class: Do not leak the chrdev in error path Currently, the error path of stm_register_device() forgets to unregister the chrdev. Fix this. Reported-by: Alan Cox Signed-off-by: Alexander Shishkin Reviewed-by: Laurent Fert --- drivers/hwtracing/stm/core.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/hwtracing/stm/core.c b/drivers/hwtracing/stm/core.c index 18f176eac06d..e55ed6714223 100644 --- a/drivers/hwtracing/stm/core.c +++ b/drivers/hwtracing/stm/core.c @@ -688,6 +688,8 @@ int stm_register_device(struct device *parent, struct stm_data *stm_data, return 0; err_device: + unregister_chrdev(stm->major, stm_data->name); + /* matches device_initialize() above */ put_device(&stm->dev); err_free: From 389b6699a2aa0b457aa69986e9ddf39f3b4030fd Mon Sep 17 00:00:00 2001 From: Alexander Shishkin Date: Fri, 4 Mar 2016 16:48:14 +0200 Subject: [PATCH 07/19] stm class: Fix stm device initialization order Currently, stm_register_device() makes the device visible and then proceeds to initializing spinlocks and other properties, which leaves a window when the device can already be opened but is not yet fully operational. Fix this by reversing the initialization order. Reported-by: Alan Cox Signed-off-by: Alexander Shishkin Reviewed-by: Laurent Fert --- drivers/hwtracing/stm/core.c | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/drivers/hwtracing/stm/core.c b/drivers/hwtracing/stm/core.c index e55ed6714223..2591442e2c5b 100644 --- a/drivers/hwtracing/stm/core.c +++ b/drivers/hwtracing/stm/core.c @@ -666,6 +666,18 @@ int stm_register_device(struct device *parent, struct stm_data *stm_data, stm->dev.parent = parent; stm->dev.release = stm_device_release; + mutex_init(&stm->link_mutex); + spin_lock_init(&stm->link_lock); + INIT_LIST_HEAD(&stm->link_list); + + /* initialize the object before it is accessible via sysfs */ + spin_lock_init(&stm->mc_lock); + mutex_init(&stm->policy_mutex); + stm->sw_nmasters = nmasters; + stm->owner = owner; + stm->data = stm_data; + stm_data->stm = stm; + err = kobject_set_name(&stm->dev.kobj, "%s", stm_data->name); if (err) goto err_device; @@ -674,17 +686,6 @@ int stm_register_device(struct device *parent, struct stm_data *stm_data, if (err) goto err_device; - mutex_init(&stm->link_mutex); - spin_lock_init(&stm->link_lock); - INIT_LIST_HEAD(&stm->link_list); - - spin_lock_init(&stm->mc_lock); - mutex_init(&stm->policy_mutex); - stm->sw_nmasters = nmasters; - stm->owner = owner; - stm->data = stm_data; - stm_data->stm = stm; - return 0; err_device: From fb0801904bbbc7b109d4009520c7fa34bcfb7450 Mon Sep 17 00:00:00 2001 From: Alexander Shishkin Date: Fri, 4 Mar 2016 16:55:12 +0200 Subject: [PATCH 08/19] stm class: Remove unnecessary pointer increment Readability: a postfix increment is used on a pointer which is not used anywhere afterwards, which may send the reader looking through the function one extra time. Drop the unnecessary increment. Reported-by: Alan Cox Signed-off-by: Alexander Shishkin Reviewed-by: Laurent Fert --- drivers/hwtracing/stm/policy.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/hwtracing/stm/policy.c b/drivers/hwtracing/stm/policy.c index e8b50b1ac619..6c0ae2996326 100644 --- a/drivers/hwtracing/stm/policy.c +++ b/drivers/hwtracing/stm/policy.c @@ -341,7 +341,7 @@ stp_policies_make(struct config_group *group, const char *name) return ERR_PTR(-EINVAL); } - *p++ = '\0'; + *p = '\0'; stm = stm_find_device(devname); kfree(devname); From 8f1127ea09a7bfe7e1b5fab5b7f52bd56f2d1bf5 Mon Sep 17 00:00:00 2001 From: Alexander Shishkin Date: Fri, 4 Mar 2016 16:10:20 +0200 Subject: [PATCH 09/19] intel_th: pti: Do remove sysfs group on device removal Right now, the PTI output driver forgets to clean up its sysfs group when it gets removed. Fix this. Reported-by: Alan Cox Signed-off-by: Alexander Shishkin Reviewed-by: Laurent Fert --- drivers/hwtracing/intel_th/pti.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/hwtracing/intel_th/pti.c b/drivers/hwtracing/intel_th/pti.c index 57cbfdcc7ef0..2692cad4c3c5 100644 --- a/drivers/hwtracing/intel_th/pti.c +++ b/drivers/hwtracing/intel_th/pti.c @@ -230,6 +230,7 @@ static int intel_th_pti_probe(struct intel_th_device *thdev) static void intel_th_pti_remove(struct intel_th_device *thdev) { + sysfs_remove_group(&thdev->dev.kobj, &pti_output_group); } static struct intel_th_driver intel_th_pti_driver = { From 6575cbd67172bbcbfbb50ddd854d2b90c9f4e358 Mon Sep 17 00:00:00 2001 From: Alexander Shishkin Date: Fri, 4 Mar 2016 16:16:31 +0200 Subject: [PATCH 10/19] intel_th: msu: Handle kstrndup() failure Currently, the nr_pages attribute store does not check if kstrndup() succeeded. Fix this. Reported-by: Alan Cox Signed-off-by: Alexander Shishkin Reviewed-by: Laurent Fert --- drivers/hwtracing/intel_th/msu.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/drivers/hwtracing/intel_th/msu.c b/drivers/hwtracing/intel_th/msu.c index d9d6022c5aca..747ccf84bd93 100644 --- a/drivers/hwtracing/intel_th/msu.c +++ b/drivers/hwtracing/intel_th/msu.c @@ -1393,6 +1393,11 @@ nr_pages_store(struct device *dev, struct device_attribute *attr, do { end = memchr(p, ',', len); s = kstrndup(p, end ? end - p : len, GFP_KERNEL); + if (!s) { + ret = -ENOMEM; + goto free_win; + } + ret = kstrtoul(s, 10, &val); kfree(s); From b5edbf1ea3ad044b185be7015cffabba9c442660 Mon Sep 17 00:00:00 2001 From: Alexander Shishkin Date: Fri, 4 Mar 2016 19:42:48 +0200 Subject: [PATCH 11/19] intel_th: Allow subdevice drivers to bring in own attribute groups Some subdevices (MSU, PTI) need to register their own driver-specific attribute groups. Provide a way for those to pass their attribute groups to the core driver in their driver structure so that the core can take care of creating and removing them. Signed-off-by: Alexander Shishkin Reviewed-by: Laurent Fert --- drivers/hwtracing/intel_th/core.c | 12 ++++++++++++ drivers/hwtracing/intel_th/intel_th.h | 3 +++ 2 files changed, 15 insertions(+) diff --git a/drivers/hwtracing/intel_th/core.c b/drivers/hwtracing/intel_th/core.c index 4272f2ce5f6e..db0691929a60 100644 --- a/drivers/hwtracing/intel_th/core.c +++ b/drivers/hwtracing/intel_th/core.c @@ -71,6 +71,15 @@ static int intel_th_probe(struct device *dev) if (ret) return ret; + if (thdrv->attr_group) { + ret = sysfs_create_group(&thdev->dev.kobj, thdrv->attr_group); + if (ret) { + thdrv->remove(thdev); + + return ret; + } + } + if (thdev->type == INTEL_TH_OUTPUT && !intel_th_output_assigned(thdev)) ret = hubdrv->assign(hub, thdev); @@ -91,6 +100,9 @@ static int intel_th_remove(struct device *dev) return err; } + if (thdrv->attr_group) + sysfs_remove_group(&thdev->dev.kobj, thdrv->attr_group); + thdrv->remove(thdev); if (intel_th_output_assigned(thdev)) { diff --git a/drivers/hwtracing/intel_th/intel_th.h b/drivers/hwtracing/intel_th/intel_th.h index eedd09332db6..15ebd48a29f2 100644 --- a/drivers/hwtracing/intel_th/intel_th.h +++ b/drivers/hwtracing/intel_th/intel_th.h @@ -115,6 +115,7 @@ intel_th_output_assigned(struct intel_th_device *thdev) * @enable: enable tracing for a given output device * @disable: disable tracing for a given output device * @fops: file operations for device nodes + * @attr_group: attributes provided by the driver * * Callbacks @probe and @remove are required for all device types. * Switch device driver needs to fill in @assign, @enable and @disable @@ -139,6 +140,8 @@ struct intel_th_driver { void (*deactivate)(struct intel_th_device *thdev); /* file_operations for those who want a device node */ const struct file_operations *fops; + /* optional attributes */ + struct attribute_group *attr_group; /* source ops */ int (*set_output)(struct intel_th_device *thdev, From 9d482aedd0e2389b483ea8ea727ec201b65e2f27 Mon Sep 17 00:00:00 2001 From: Alexander Shishkin Date: Fri, 4 Mar 2016 19:55:10 +0200 Subject: [PATCH 12/19] intel_th: msu: Create sysfs attributes using core driver's facility The core intel_th driver allows subdevices to bring in their sysfs attributes. Use this instead of taking care of them in probe and remove. Signed-off-by: Alexander Shishkin Reviewed-by: Laurent Fert --- drivers/hwtracing/intel_th/msu.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/drivers/hwtracing/intel_th/msu.c b/drivers/hwtracing/intel_th/msu.c index 747ccf84bd93..25af2146866c 100644 --- a/drivers/hwtracing/intel_th/msu.c +++ b/drivers/hwtracing/intel_th/msu.c @@ -1478,10 +1478,6 @@ static int intel_th_msc_probe(struct intel_th_device *thdev) if (err) return err; - err = sysfs_create_group(&dev->kobj, &msc_output_group); - if (err) - return err; - dev_set_drvdata(dev, msc); return 0; @@ -1489,7 +1485,6 @@ static int intel_th_msc_probe(struct intel_th_device *thdev) static void intel_th_msc_remove(struct intel_th_device *thdev) { - sysfs_remove_group(&thdev->dev.kobj, &msc_output_group); } static struct intel_th_driver intel_th_msc_driver = { @@ -1498,6 +1493,7 @@ static struct intel_th_driver intel_th_msc_driver = { .activate = intel_th_msc_activate, .deactivate = intel_th_msc_deactivate, .fops = &intel_th_msc_fops, + .attr_group = &msc_output_group, .driver = { .name = "msc", .owner = THIS_MODULE, From e8644e4c2aa5c52c357f63af9cc17ef5dce38396 Mon Sep 17 00:00:00 2001 From: Alexander Shishkin Date: Fri, 4 Mar 2016 19:55:10 +0200 Subject: [PATCH 13/19] intel_th: pti: Create sysfs attributes using core driver's facility The core intel_th driver allows subdevices to bring in their sysfs attributes. Use this instead of taking care of them in probe and remove. Signed-off-by: Alexander Shishkin Reviewed-by: Laurent Fert --- drivers/hwtracing/intel_th/pti.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/drivers/hwtracing/intel_th/pti.c b/drivers/hwtracing/intel_th/pti.c index 2692cad4c3c5..35738b5bfccd 100644 --- a/drivers/hwtracing/intel_th/pti.c +++ b/drivers/hwtracing/intel_th/pti.c @@ -200,7 +200,6 @@ static int intel_th_pti_probe(struct intel_th_device *thdev) struct resource *res; struct pti_device *pti; void __iomem *base; - int ret; res = intel_th_device_get_resource(thdev, IORESOURCE_MEM, 0); if (!res) @@ -219,10 +218,6 @@ static int intel_th_pti_probe(struct intel_th_device *thdev) read_hw_config(pti); - ret = sysfs_create_group(&dev->kobj, &pti_output_group); - if (ret) - return ret; - dev_set_drvdata(dev, pti); return 0; @@ -230,7 +225,6 @@ static int intel_th_pti_probe(struct intel_th_device *thdev) static void intel_th_pti_remove(struct intel_th_device *thdev) { - sysfs_remove_group(&thdev->dev.kobj, &pti_output_group); } static struct intel_th_driver intel_th_pti_driver = { @@ -238,6 +232,7 @@ static struct intel_th_driver intel_th_pti_driver = { .remove = intel_th_pti_remove, .activate = intel_th_pti_activate, .deactivate = intel_th_pti_deactivate, + .attr_group = &pti_output_group, .driver = { .name = "pti", .owner = THIS_MODULE, From f18a9531f6da9aba2920a3a5f166dba5a20592a0 Mon Sep 17 00:00:00 2001 From: Alexander Shishkin Date: Mon, 7 Mar 2016 17:04:45 +0200 Subject: [PATCH 14/19] intel_th: Fix activating a subdevice without a driver If output subdevice driver is not loaded, activating it will try to call its ->activate method and crash. Fix this by explicitly checking for the driver. Signed-off-by: Alexander Shishkin Reviewed-by: Laurent Fert --- drivers/hwtracing/intel_th/core.c | 12 ++++++++++-- drivers/hwtracing/intel_th/intel_th.h | 3 +++ 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/drivers/hwtracing/intel_th/core.c b/drivers/hwtracing/intel_th/core.c index db0691929a60..20339470c2c6 100644 --- a/drivers/hwtracing/intel_th/core.c +++ b/drivers/hwtracing/intel_th/core.c @@ -183,7 +183,11 @@ static DEVICE_ATTR_RO(port); static int intel_th_output_activate(struct intel_th_device *thdev) { - struct intel_th_driver *thdrv = to_intel_th_driver(thdev->dev.driver); + struct intel_th_driver *thdrv = + to_intel_th_driver_or_null(thdev->dev.driver); + + if (!thdrv) + return -ENODEV; if (thdrv->activate) return thdrv->activate(thdev); @@ -195,7 +199,11 @@ static int intel_th_output_activate(struct intel_th_device *thdev) static void intel_th_output_deactivate(struct intel_th_device *thdev) { - struct intel_th_driver *thdrv = to_intel_th_driver(thdev->dev.driver); + struct intel_th_driver *thdrv = + to_intel_th_driver_or_null(thdev->dev.driver); + + if (!thdrv) + return; if (thdrv->deactivate) thdrv->deactivate(thdev); diff --git a/drivers/hwtracing/intel_th/intel_th.h b/drivers/hwtracing/intel_th/intel_th.h index 15ebd48a29f2..0df22e30673d 100644 --- a/drivers/hwtracing/intel_th/intel_th.h +++ b/drivers/hwtracing/intel_th/intel_th.h @@ -151,6 +151,9 @@ struct intel_th_driver { #define to_intel_th_driver(_d) \ container_of((_d), struct intel_th_driver, driver) +#define to_intel_th_driver_or_null(_d) \ + ((_d) ? to_intel_th_driver(_d) : NULL) + static inline struct intel_th_device * to_intel_th_hub(struct intel_th_device *thdev) { From a45ff6ed742cdfdb3cdebee83d19ab1c00d91fcc Mon Sep 17 00:00:00 2001 From: Alexander Shishkin Date: Thu, 10 Mar 2016 18:21:14 +0200 Subject: [PATCH 15/19] intel_th: msu: Serialize enabling/disabling In order to guarantee that readers don't race with trace enabling, both should happen under the same mutex. Having two mutexes seems like an overkill, considering that because of the above, they'll have to be acquired together, around trace enabling and char device opening. This patch makes both buffer accesses and readers serialize on msc::buf_mutex and makes sure that 'enabled' flag accesses are also serialized on it. Signed-off-by: Alexander Shishkin Reviewed-by: Laurent Fert --- drivers/hwtracing/intel_th/msu.c | 92 +++++++++++++++++--------------- 1 file changed, 49 insertions(+), 43 deletions(-) diff --git a/drivers/hwtracing/intel_th/msu.c b/drivers/hwtracing/intel_th/msu.c index 25af2146866c..ee153067e136 100644 --- a/drivers/hwtracing/intel_th/msu.c +++ b/drivers/hwtracing/intel_th/msu.c @@ -122,7 +122,6 @@ struct msc { atomic_t mmap_count; struct mutex buf_mutex; - struct mutex iter_mutex; struct list_head iter_list; /* config */ @@ -257,23 +256,37 @@ static struct msc_iter *msc_iter_install(struct msc *msc) iter = kzalloc(sizeof(*iter), GFP_KERNEL); if (!iter) - return NULL; + return ERR_PTR(-ENOMEM); + + mutex_lock(&msc->buf_mutex); + + /* + * Reading and tracing are mutually exclusive; if msc is + * enabled, open() will fail; otherwise existing readers + * will prevent enabling the msc and the rest of fops don't + * need to worry about it. + */ + if (msc->enabled) { + kfree(iter); + iter = ERR_PTR(-EBUSY); + goto unlock; + } msc_iter_init(iter); iter->msc = msc; - mutex_lock(&msc->iter_mutex); list_add_tail(&iter->entry, &msc->iter_list); - mutex_unlock(&msc->iter_mutex); +unlock: + mutex_unlock(&msc->buf_mutex); return iter; } static void msc_iter_remove(struct msc_iter *iter, struct msc *msc) { - mutex_lock(&msc->iter_mutex); + mutex_lock(&msc->buf_mutex); list_del(&iter->entry); - mutex_unlock(&msc->iter_mutex); + mutex_unlock(&msc->buf_mutex); kfree(iter); } @@ -454,7 +467,6 @@ static void msc_buffer_clear_hw_header(struct msc *msc) { struct msc_window *win; - mutex_lock(&msc->buf_mutex); list_for_each_entry(win, &msc->win_list, entry) { unsigned int blk; size_t hw_sz = sizeof(struct msc_block_desc) - @@ -466,7 +478,6 @@ static void msc_buffer_clear_hw_header(struct msc *msc) memset(&bdesc->hw_tag, 0, hw_sz); } } - mutex_unlock(&msc->buf_mutex); } /** @@ -474,12 +485,15 @@ static void msc_buffer_clear_hw_header(struct msc *msc) * @msc: the MSC device to configure * * Program storage mode, wrapping, burst length and trace buffer address - * into a given MSC. If msc::enabled is set, enable the trace, too. + * into a given MSC. Then, enable tracing and set msc::enabled. + * The latter is serialized on msc::buf_mutex, so make sure to hold it. */ static int msc_configure(struct msc *msc) { u32 reg; + lockdep_assert_held(&msc->buf_mutex); + if (msc->mode > MSC_MODE_MULTI) return -ENOTSUPP; @@ -497,21 +511,19 @@ static int msc_configure(struct msc *msc) reg = ioread32(msc->reg_base + REG_MSU_MSC0CTL); reg &= ~(MSC_MODE | MSC_WRAPEN | MSC_EN | MSC_RD_HDR_OVRD); + reg |= MSC_EN; reg |= msc->mode << __ffs(MSC_MODE); reg |= msc->burst_len << __ffs(MSC_LEN); - /*if (msc->mode == MSC_MODE_MULTI) - reg |= MSC_RD_HDR_OVRD; */ + if (msc->wrap) reg |= MSC_WRAPEN; - if (msc->enabled) - reg |= MSC_EN; iowrite32(reg, msc->reg_base + REG_MSU_MSC0CTL); - if (msc->enabled) { - msc->thdev->output.multiblock = msc->mode == MSC_MODE_MULTI; - intel_th_trace_enable(msc->thdev); - } + msc->thdev->output.multiblock = msc->mode == MSC_MODE_MULTI; + intel_th_trace_enable(msc->thdev); + msc->enabled = 1; + return 0; } @@ -521,15 +533,14 @@ static int msc_configure(struct msc *msc) * @msc: MSC device to disable * * If @msc is enabled, disable tracing on the switch and then disable MSC - * storage. + * storage. Caller must hold msc::buf_mutex. */ static void msc_disable(struct msc *msc) { unsigned long count; u32 reg; - if (!msc->enabled) - return; + lockdep_assert_held(&msc->buf_mutex); intel_th_trace_disable(msc->thdev); @@ -569,33 +580,35 @@ static void msc_disable(struct msc *msc) static int intel_th_msc_activate(struct intel_th_device *thdev) { struct msc *msc = dev_get_drvdata(&thdev->dev); - int ret = 0; + int ret = -EBUSY; if (!atomic_inc_unless_negative(&msc->user_count)) return -ENODEV; - mutex_lock(&msc->iter_mutex); - if (!list_empty(&msc->iter_list)) - ret = -EBUSY; - mutex_unlock(&msc->iter_mutex); + mutex_lock(&msc->buf_mutex); - if (ret) { + /* if there are readers, refuse */ + if (list_empty(&msc->iter_list)) + ret = msc_configure(msc); + + mutex_unlock(&msc->buf_mutex); + + if (ret) atomic_dec(&msc->user_count); - return ret; - } - msc->enabled = 1; - - return msc_configure(msc); + return ret; } static void intel_th_msc_deactivate(struct intel_th_device *thdev) { struct msc *msc = dev_get_drvdata(&thdev->dev); - msc_disable(msc); - - atomic_dec(&msc->user_count); + mutex_lock(&msc->buf_mutex); + if (msc->enabled) { + msc_disable(msc); + atomic_dec(&msc->user_count); + } + mutex_unlock(&msc->buf_mutex); } /** @@ -1035,8 +1048,8 @@ static int intel_th_msc_open(struct inode *inode, struct file *file) return -EPERM; iter = msc_iter_install(msc); - if (!iter) - return -ENOMEM; + if (IS_ERR(iter)) + return PTR_ERR(iter); file->private_data = iter; @@ -1101,11 +1114,6 @@ static ssize_t intel_th_msc_read(struct file *file, char __user *buf, if (!atomic_inc_unless_negative(&msc->user_count)) return 0; - if (msc->enabled) { - ret = -EBUSY; - goto put_count; - } - if (msc->mode == MSC_MODE_SINGLE && !msc->single_wrap) size = msc->single_sz; else @@ -1254,8 +1262,6 @@ static int intel_th_msc_init(struct msc *msc) msc->mode = MSC_MODE_MULTI; mutex_init(&msc->buf_mutex); INIT_LIST_HEAD(&msc->win_list); - - mutex_init(&msc->iter_mutex); INIT_LIST_HEAD(&msc->iter_list); msc->burst_len = From e2ea295baf87d78f2ed86ce595b30c691b18b210 Mon Sep 17 00:00:00 2001 From: Alexander Shishkin Date: Fri, 8 Apr 2016 17:35:04 +0300 Subject: [PATCH 16/19] intel_th: Hold output driver module reference while capture is active Right now it's possible to unload the output subdevice's driver while the capture to this output is active. Prevent this by holding the output driver's module reference. Signed-off-by: Alexander Shishkin Reviewed-by: Laurent Fert --- drivers/hwtracing/intel_th/core.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/drivers/hwtracing/intel_th/core.c b/drivers/hwtracing/intel_th/core.c index 20339470c2c6..1be543e8e42f 100644 --- a/drivers/hwtracing/intel_th/core.c +++ b/drivers/hwtracing/intel_th/core.c @@ -189,6 +189,9 @@ static int intel_th_output_activate(struct intel_th_device *thdev) if (!thdrv) return -ENODEV; + if (!try_module_get(thdrv->driver.owner)) + return -ENODEV; + if (thdrv->activate) return thdrv->activate(thdev); @@ -209,6 +212,8 @@ static void intel_th_output_deactivate(struct intel_th_device *thdev) thdrv->deactivate(thdev); else intel_th_trace_disable(thdev); + + module_put(thdrv->driver.owner); } static ssize_t active_show(struct device *dev, struct device_attribute *attr, From 8e9a2beb5f991916e530184957c4137fab14604c Mon Sep 17 00:00:00 2001 From: Alexander Shishkin Date: Fri, 8 Apr 2016 17:36:04 +0300 Subject: [PATCH 17/19] intel_th: msu: Set fops::owner to prevent module from unloading Right now it's possible to unload the msu driver while its character device is open. Prevent it by setting fops::owner, which will result in the module reference being held while the device node is open. Signed-off-by: Alexander Shishkin Reviewed-by: Laurent Fert --- drivers/hwtracing/intel_th/msu.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/hwtracing/intel_th/msu.c b/drivers/hwtracing/intel_th/msu.c index ee153067e136..bcc3b4713377 100644 --- a/drivers/hwtracing/intel_th/msu.c +++ b/drivers/hwtracing/intel_th/msu.c @@ -1253,6 +1253,7 @@ static const struct file_operations intel_th_msc_fops = { .read = intel_th_msc_read, .mmap = intel_th_msc_mmap, .llseek = no_llseek, + .owner = THIS_MODULE, }; static int intel_th_msc_init(struct msc *msc) From f152dfee19c0cd146b16179a60ffb2cacf995736 Mon Sep 17 00:00:00 2001 From: Alexander Shishkin Date: Fri, 8 Apr 2016 17:37:40 +0300 Subject: [PATCH 18/19] intel_th: msu: Release resources on removal Do release the resources when msu subdevice gets removed: stop the capture if it is active (which is still possible even though the module in pinned) and free the capture buffers. Signed-off-by: Alexander Shishkin Reviewed-by: Laurent Fert --- drivers/hwtracing/intel_th/msu.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/drivers/hwtracing/intel_th/msu.c b/drivers/hwtracing/intel_th/msu.c index bcc3b4713377..0974090abc7d 100644 --- a/drivers/hwtracing/intel_th/msu.c +++ b/drivers/hwtracing/intel_th/msu.c @@ -1492,6 +1492,18 @@ static int intel_th_msc_probe(struct intel_th_device *thdev) static void intel_th_msc_remove(struct intel_th_device *thdev) { + struct msc *msc = dev_get_drvdata(&thdev->dev); + int ret; + + intel_th_msc_deactivate(thdev); + + /* + * Buffers should not be used at this point except if the + * output character device is still open and the parent + * device gets detached from its bus, which is a FIXME. + */ + ret = msc_buffer_free_unless_used(msc); + WARN_ON_ONCE(ret); } static struct intel_th_driver intel_th_msc_driver = { From aaa3ca82286d53c5409d2c22204426c9ca419d8c Mon Sep 17 00:00:00 2001 From: Alexander Shishkin Date: Fri, 8 Apr 2016 18:26:52 +0300 Subject: [PATCH 19/19] intel_th: pci: Add Broxton-M SOC support This adds Intel(R) Trace Hub PCI ID for Broxton-M SOC. Signed-off-by: Alexander Shishkin Reviewed-by: Laurent Fert --- drivers/hwtracing/intel_th/pci.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/drivers/hwtracing/intel_th/pci.c b/drivers/hwtracing/intel_th/pci.c index bca7a2ac00d6..5e25c7eb31d3 100644 --- a/drivers/hwtracing/intel_th/pci.c +++ b/drivers/hwtracing/intel_th/pci.c @@ -75,6 +75,11 @@ static const struct pci_device_id intel_th_pci_id_table[] = { PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x0a80), .driver_data = (kernel_ulong_t)0, }, + { + /* Broxton B-step */ + PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x1a8e), + .driver_data = (kernel_ulong_t)0, + }, { 0 }, };