From 8f1127ea09a7bfe7e1b5fab5b7f52bd56f2d1bf5 Mon Sep 17 00:00:00 2001 From: Alexander Shishkin Date: Fri, 4 Mar 2016 16:10:20 +0200 Subject: 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(+) (limited to 'drivers/hwtracing/intel_th') 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 = { -- cgit v1.2.3 From 6575cbd67172bbcbfbb50ddd854d2b90c9f4e358 Mon Sep 17 00:00:00 2001 From: Alexander Shishkin Date: Fri, 4 Mar 2016 16:16:31 +0200 Subject: 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(+) (limited to 'drivers/hwtracing/intel_th') 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); -- cgit v1.2.3 From b5edbf1ea3ad044b185be7015cffabba9c442660 Mon Sep 17 00:00:00 2001 From: Alexander Shishkin Date: Fri, 4 Mar 2016 19:42:48 +0200 Subject: 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(+) (limited to 'drivers/hwtracing/intel_th') 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, -- cgit v1.2.3 From 9d482aedd0e2389b483ea8ea727ec201b65e2f27 Mon Sep 17 00:00:00 2001 From: Alexander Shishkin Date: Fri, 4 Mar 2016 19:55:10 +0200 Subject: 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(-) (limited to 'drivers/hwtracing/intel_th') 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, -- cgit v1.2.3 From e8644e4c2aa5c52c357f63af9cc17ef5dce38396 Mon Sep 17 00:00:00 2001 From: Alexander Shishkin Date: Fri, 4 Mar 2016 19:55:10 +0200 Subject: 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(-) (limited to 'drivers/hwtracing/intel_th') 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, -- cgit v1.2.3 From f18a9531f6da9aba2920a3a5f166dba5a20592a0 Mon Sep 17 00:00:00 2001 From: Alexander Shishkin Date: Mon, 7 Mar 2016 17:04:45 +0200 Subject: 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(-) (limited to 'drivers/hwtracing/intel_th') 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) { -- cgit v1.2.3 From a45ff6ed742cdfdb3cdebee83d19ab1c00d91fcc Mon Sep 17 00:00:00 2001 From: Alexander Shishkin Date: Thu, 10 Mar 2016 18:21:14 +0200 Subject: 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(-) (limited to 'drivers/hwtracing/intel_th') 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) { - atomic_dec(&msc->user_count); - return ret; - } + /* if there are readers, refuse */ + if (list_empty(&msc->iter_list)) + ret = msc_configure(msc); - msc->enabled = 1; + mutex_unlock(&msc->buf_mutex); + + if (ret) + atomic_dec(&msc->user_count); - 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 = -- cgit v1.2.3 From e2ea295baf87d78f2ed86ce595b30c691b18b210 Mon Sep 17 00:00:00 2001 From: Alexander Shishkin Date: Fri, 8 Apr 2016 17:35:04 +0300 Subject: 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(+) (limited to 'drivers/hwtracing/intel_th') 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, -- cgit v1.2.3 From 8e9a2beb5f991916e530184957c4137fab14604c Mon Sep 17 00:00:00 2001 From: Alexander Shishkin Date: Fri, 8 Apr 2016 17:36:04 +0300 Subject: 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(+) (limited to 'drivers/hwtracing/intel_th') 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) -- cgit v1.2.3 From f152dfee19c0cd146b16179a60ffb2cacf995736 Mon Sep 17 00:00:00 2001 From: Alexander Shishkin Date: Fri, 8 Apr 2016 17:37:40 +0300 Subject: 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(+) (limited to 'drivers/hwtracing/intel_th') 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 = { -- cgit v1.2.3 From aaa3ca82286d53c5409d2c22204426c9ca419d8c Mon Sep 17 00:00:00 2001 From: Alexander Shishkin Date: Fri, 8 Apr 2016 18:26:52 +0300 Subject: 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(+) (limited to 'drivers/hwtracing/intel_th') 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 }, }; -- cgit v1.2.3