Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1374275 > unrolled thread

[QUEUED v20160408 00/18] stm class/intel_th: Patches in my queue

Back to article view | Back to linux.kernel

Contents

  [QUEUED v20160408 00/18] stm class/intel_th: Patches in my queue Alexander Shishkin <alexander.shishkin@linux.intel.com> - 2016-04-08 17:00 +0200
    [QUEUED v20160408 09/18] intel_th: pti: Do remove sysfs group on device removal Alexander Shishkin <alexander.shishkin@linux.intel.com> - 2016-04-08 17:00 +0200
    [QUEUED v20160408 15/18] intel_th: msu: Serialize enabling/disabling Alexander Shishkin <alexander.shishkin@linux.intel.com> - 2016-04-08 17:00 +0200
    [QUEUED v20160408 10/18] intel_th: msu: Handle kstrndup() failure Alexander Shishkin <alexander.shishkin@linux.intel.com> - 2016-04-08 17:10 +0200
    [QUEUED v20160408 07/18] stm class: Fix stm device initialization order Alexander Shishkin <alexander.shishkin@linux.intel.com> - 2016-04-08 17:10 +0200
    [QUEUED v20160408 08/18] stm class: Remove unnecessary pointer increment Alexander Shishkin <alexander.shishkin@linux.intel.com> - 2016-04-08 17:10 +0200
    [QUEUED v20160408 04/18] stm class: stm_heartbeat: Make nr_devs parameter read-only Alexander Shishkin <alexander.shishkin@linux.intel.com> - 2016-04-08 17:10 +0200
    [QUEUED v20160408 03/18] stm class: dummy_stm: Make nr_dummies parameter read-only Alexander Shishkin <alexander.shishkin@linux.intel.com> - 2016-04-08 17:10 +0200

#1374275 — [QUEUED v20160408 00/18] stm class/intel_th: Patches in my queue

Hi everybody,

These are the patches that I have in my queue for the next merge
window at the moment. They can also be found in a git tree [1] or via
cgit [2], branch stm-dev. Let me know what you think.

[1] git://git.kernel.org/pub/scm/linux/kernel/git/ash/stm.git
[2] https://git.kernel.org/cgit/linux/kernel/git/ash/stm.git/log/?h=stm-dev

Alexander Shishkin (17):
  MAINTAINERS: Add a git tree for the stm class
  stm class: dummy_stm: Make nr_dummies parameter read-only
  stm class: stm_heartbeat: Make nr_devs parameter read-only
  stm class: Remove a pointless line
  stm class: Do not leak the chrdev in error path
  stm class: Fix stm device initialization order
  stm class: Remove unnecessary pointer increment
  intel_th: pti: Do remove sysfs group on device removal
  intel_th: msu: Handle kstrndup() failure
  intel_th: Allow subdevice drivers to bring in own attribute groups
  intel_th: msu: Create sysfs attributes using core driver's facility
  intel_th: pti: Create sysfs attributes using core driver's facility
  intel_th: Fix activating a subdevice without a driver
  intel_th: msu: Serialize enabling/disabling
  intel_th: Hold output driver module reference while capture is active
  intel_th: msu: Set fops::owner to prevent module from unloading
  intel_th: msu: Release resources on removal

Chunyan Zhang (1):
  stm class: Fix integer boundary checks for master range

 MAINTAINERS                           |   1 +
 drivers/hwtracing/intel_th/core.c     |  29 ++++++++-
 drivers/hwtracing/intel_th/intel_th.h |   6 ++
 drivers/hwtracing/intel_th/msu.c      | 116 ++++++++++++++++++++--------------
 drivers/hwtracing/intel_th/pti.c      |   6 +-
 drivers/hwtracing/stm/core.c          |  21 +++---
 drivers/hwtracing/stm/dummy_stm.c     |  14 ++--
 drivers/hwtracing/stm/heartbeat.c     |  14 ++--
 drivers/hwtracing/stm/policy.c        |   5 +-
 9 files changed, 126 insertions(+), 86 deletions(-)

-- 
2.8.0.rc3

[toc] | [next] | [standalone]

#1374277 — [QUEUED v20160408 09/18] 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 <alan.cox@intel.com>
Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
---
 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 57cbfdcc7e..2692cad4c3 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 = {
-- 
2.8.0.rc3

[toc] | [prev] | [next] | [standalone]

#1374278 — [QUEUED v20160408 15/18] 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 <alexander.shishkin@linux.intel.com>
---
 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 25af214686..ee153067e1 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 =
-- 
2.8.0.rc3

[toc] | [prev] | [next] | [standalone]

#1374280 — [QUEUED v20160408 10/18] intel_th: msu: Handle kstrndup() failure

Currently, the nr_pages attribute store does not check if kstrndup()
succeeded. Fix this.

Reported-by: Alan Cox <alan.cox@intel.com>
Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
---
 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 d9d6022c5a..747ccf84bd 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);
 
-- 
2.8.0.rc3

[toc] | [prev] | [next] | [standalone]

#1374285 — [QUEUED v20160408 07/18] 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 <alan.cox@intel.com>
Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
---
 drivers/hwtracing/stm/core.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/hwtracing/stm/core.c b/drivers/hwtracing/stm/core.c
index e55ed67142..2591442e2c 100644
--- a/drivers/hwtracing/stm/core.c
+++ b/drivers/hwtracing/stm/core.c
@@ -666,18 +666,11 @@ int stm_register_device(struct device *parent, struct stm_data *stm_data,
 	stm->dev.parent = parent;
 	stm->dev.release = stm_device_release;
 
-	err = kobject_set_name(&stm->dev.kobj, "%s", stm_data->name);
-	if (err)
-		goto err_device;
-
-	err = device_add(&stm->dev);
-	if (err)
-		goto err_device;
-
 	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;
@@ -685,6 +678,14 @@ int stm_register_device(struct device *parent, struct stm_data *stm_data,
 	stm->data = stm_data;
 	stm_data->stm = stm;
 
+	err = kobject_set_name(&stm->dev.kobj, "%s", stm_data->name);
+	if (err)
+		goto err_device;
+
+	err = device_add(&stm->dev);
+	if (err)
+		goto err_device;
+
 	return 0;
 
 err_device:
-- 
2.8.0.rc3

[toc] | [prev] | [next] | [standalone]

#1374288 — [QUEUED v20160408 08/18] 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 <alan.cox@intel.com>
Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
---
 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 e8b50b1ac6..6c0ae29963 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);
-- 
2.8.0.rc3

[toc] | [prev] | [next] | [standalone]

#1374289 — [QUEUED v20160408 04/18] 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 <alan.cox@intel.com>
Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
---
 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 0133571b50..3da7b673aa 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);
 	}
-- 
2.8.0.rc3

[toc] | [prev] | [next] | [standalone]

#1374291 — [QUEUED v20160408 03/18] 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 <alan.cox@intel.com>
Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
---
 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 310adf57e7..a86612d989 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);
 	}
-- 
2.8.0.rc3

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web