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/msu.c') 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 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/msu.c') 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 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/msu.c') 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 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/msu.c') 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/msu.c') 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