[PATCH v2 0/4] PTP attribute handling cleanup

STALE3495d

9 messages, 3 authors, 2017-02-17 · open the first message on its own page

[PATCH v2 0/4] PTP attribute handling cleanup

From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Date: 2017-02-14 18:23:46

PTP core was creating some attributes, such as "period" and "fifo", and the
entire "pins" attribute group, after creating class deevice, which creates
a race for userspace: uevent may arrive before all attributes are created.

This series of patches switches PTP to use is_visible() to control
visibility of attributes in a group, and device_create_with_groups() to
ensure that attributes are created before we notify userspace of a new
device.

v2:
- added Richard's acked-by to patch #1
- removed use of kmalloc_array in favor of kcalloc in patch #2 at
  Richard's request
- added a cover letter

v1:
- initial patch set

Dmitry Torokhov (4):
  ptp: do not explicitly set drvdata in ptp_clock_register()
  ptp: use kcalloc when allocating arrays
  ptp: use is_visible method to hide unused attributes
  ptp: create "pins" together with the rest of attributes

 drivers/ptp/ptp_clock.c   |  22 +++---
 drivers/ptp/ptp_private.h |   7 +-
 drivers/ptp/ptp_sysfs.c   | 167 ++++++++++++++++++----------------------------
 3 files changed, 80 insertions(+), 116 deletions(-)

Thanks.

-- 
Dmitry

[PATCH v2 1/4] ptp: do not explicitly set drvdata in ptp_clock_register()

From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Date: 2017-02-14 18:23:50

We do not need explicitly call dev_set_drvdata(), as it is done for us by
device_create().

Acked-by: Richard Cochran <richardcochran@gmail.com>
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/ptp/ptp_clock.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c
index 9c13381b6966..b4e5e8022c29 100644
--- a/drivers/ptp/ptp_clock.c
+++ b/drivers/ptp/ptp_clock.c

@@ -227,8 +227,6 @@ struct ptp_clock *ptp_clock_register(struct ptp_clock_info *info,
 	if (IS_ERR(ptp->dev))
 		goto no_device;
 
-	dev_set_drvdata(ptp->dev, ptp);
-
 	err = ptp_populate_sysfs(ptp);
 	if (err)
 		goto no_sysfs;

-- 
2.11.0.483.g087da7b7c-goog

[PATCH v2 4/4] ptp: create "pins" together with the rest of attributes

From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Date: 2017-02-14 18:24:15

Let's switch to using device_create_with_groups(), which will allow us to
create "pins" attribute group together with the rest of ptp device
attributes, and before userspace gets notified about ptp device creation.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/ptp/ptp_clock.c   | 20 +++++++++++---------
 drivers/ptp/ptp_private.h |  7 ++++---
 drivers/ptp/ptp_sysfs.c   | 39 +++++++++------------------------------
 3 files changed, 24 insertions(+), 42 deletions(-)

diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c
index b4e5e8022c29..e8142803a1a7 100644
--- a/drivers/ptp/ptp_clock.c
+++ b/drivers/ptp/ptp_clock.c

@@ -221,16 +221,17 @@ struct ptp_clock *ptp_clock_register(struct ptp_clock_info *info,
 	mutex_init(&ptp->pincfg_mux);
 	init_waitqueue_head(&ptp->tsev_wq);
 
+	err = ptp_populate_pin_groups(ptp);
+	if (err)
+		goto no_pin_groups;
+
 	/* Create a new device in our class. */
-	ptp->dev = device_create(ptp_class, parent, ptp->devid, ptp,
-				 "ptp%d", ptp->index);
+	ptp->dev = device_create_with_groups(ptp_class, parent, ptp->devid,
+					     ptp, ptp->pin_attr_groups,
+					     "ptp%d", ptp->index);
 	if (IS_ERR(ptp->dev))
 		goto no_device;
 
-	err = ptp_populate_sysfs(ptp);
-	if (err)
-		goto no_sysfs;
-
 	/* Register a new PPS source. */
 	if (info->pps) {
 		struct pps_source_info pps;

@@ -258,10 +259,10 @@ struct ptp_clock *ptp_clock_register(struct ptp_clock_info *info,
 	if (ptp->pps_source)
 		pps_unregister_source(ptp->pps_source);
 no_pps:
-	ptp_cleanup_sysfs(ptp);
-no_sysfs:
 	device_destroy(ptp_class, ptp->devid);
 no_device:
+	ptp_cleanup_pin_groups(ptp);
+no_pin_groups:
 	mutex_destroy(&ptp->tsevq_mux);
 	mutex_destroy(&ptp->pincfg_mux);
 	ida_simple_remove(&ptp_clocks_map, index);

@@ -280,8 +281,9 @@ int ptp_clock_unregister(struct ptp_clock *ptp)
 	/* Release the clock's resources. */
 	if (ptp->pps_source)
 		pps_unregister_source(ptp->pps_source);
-	ptp_cleanup_sysfs(ptp);
+
 	device_destroy(ptp_class, ptp->devid);
+	ptp_cleanup_pin_groups(ptp);
 
 	posix_clock_unregister(&ptp->clock);
 	return 0;

diff --git a/drivers/ptp/ptp_private.h b/drivers/ptp/ptp_private.h
index 9c5d41421b65..d95888974d0c 100644
--- a/drivers/ptp/ptp_private.h
+++ b/drivers/ptp/ptp_private.h

@@ -54,6 +54,8 @@ struct ptp_clock {
 	struct device_attribute *pin_dev_attr;
 	struct attribute **pin_attr;
 	struct attribute_group pin_attr_group;
+	/* 1st entry is a pointer to the real group, 2nd is NULL terminator */
+	const struct attribute_group *pin_attr_groups[2];
 };
 
 /*

@@ -94,8 +96,7 @@ uint ptp_poll(struct posix_clock *pc,
 
 extern const struct attribute_group *ptp_groups[];
 
-int ptp_cleanup_sysfs(struct ptp_clock *ptp);
-
-int ptp_populate_sysfs(struct ptp_clock *ptp);
+int ptp_populate_pin_groups(struct ptp_clock *ptp);
+void ptp_cleanup_pin_groups(struct ptp_clock *ptp);
 
 #endif

diff --git a/drivers/ptp/ptp_sysfs.c b/drivers/ptp/ptp_sysfs.c
index 426e42c51df4..48401dfcd999 100644
--- a/drivers/ptp/ptp_sysfs.c
+++ b/drivers/ptp/ptp_sysfs.c

@@ -268,25 +268,14 @@ static ssize_t ptp_pin_store(struct device *dev, struct device_attribute *attr,
 	return count;
 }
 
-int ptp_cleanup_sysfs(struct ptp_clock *ptp)
+int ptp_populate_pin_groups(struct ptp_clock *ptp)
 {
-	struct device *dev = ptp->dev;
-	struct ptp_clock_info *info = ptp->info;
-
-	if (info->n_pins) {
-		sysfs_remove_group(&dev->kobj, &ptp->pin_attr_group);
-		kfree(ptp->pin_attr);
-		kfree(ptp->pin_dev_attr);
-	}
-	return 0;
-}
-
-static int ptp_populate_pins(struct ptp_clock *ptp)
-{
-	struct device *dev = ptp->dev;
 	struct ptp_clock_info *info = ptp->info;
 	int err = -ENOMEM, i, n_pins = info->n_pins;
 
+	if (!n_pins)
+		return 0;
+
 	ptp->pin_dev_attr = kcalloc(n_pins, sizeof(*ptp->pin_dev_attr),
 				    GFP_KERNEL);
 	if (!ptp->pin_dev_attr)

@@ -309,28 +298,18 @@ static int ptp_populate_pins(struct ptp_clock *ptp)
 	ptp->pin_attr_group.name = "pins";
 	ptp->pin_attr_group.attrs = ptp->pin_attr;
 
-	err = sysfs_create_group(&dev->kobj, &ptp->pin_attr_group);
-	if (err)
-		goto no_group;
+	ptp->pin_attr_groups[0] = &ptp->pin_attr_group;
+
 	return 0;
 
-no_group:
-	kfree(ptp->pin_attr);
 no_pin_attr:
 	kfree(ptp->pin_dev_attr);
 no_dev_attr:
 	return err;
 }
 
-int ptp_populate_sysfs(struct ptp_clock *ptp)
+void ptp_cleanup_pin_groups(struct ptp_clock *ptp)
 {
-	struct ptp_clock_info *info = ptp->info;
-	int err;
-
-	if (info->n_pins) {
-		err = ptp_populate_pins(ptp);
-		if (err)
-			return err;
-	}
-	return 0;
+	kfree(ptp->pin_attr);
+	kfree(ptp->pin_dev_attr);
 }

-- 
2.11.0.483.g087da7b7c-goog

[PATCH v2 3/4] ptp: use is_visible method to hide unused attributes

From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Date: 2017-02-14 18:24:26

Instead of creating selected attributes after the device is created (and
after userspace potentially seen uevent), lets use attribute group
is_visible() method to control which attributes are shown. This will allow
us to create all attributes (except "pins" group, which will be taken care
of later) before userspace gets notified about new ptp class device.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/ptp/ptp_sysfs.c | 125 +++++++++++++++++++++---------------------------
 1 file changed, 55 insertions(+), 70 deletions(-)

diff --git a/drivers/ptp/ptp_sysfs.c b/drivers/ptp/ptp_sysfs.c
index 27cd46ab5e32..426e42c51df4 100644
--- a/drivers/ptp/ptp_sysfs.c
+++ b/drivers/ptp/ptp_sysfs.c

@@ -46,27 +46,6 @@ PTP_SHOW_INT(n_periodic_outputs, n_per_out);
 PTP_SHOW_INT(n_programmable_pins, n_pins);
 PTP_SHOW_INT(pps_available, pps);
 
-static struct attribute *ptp_attrs[] = {
-	&dev_attr_clock_name.attr,
-	&dev_attr_max_adjustment.attr,
-	&dev_attr_n_alarms.attr,
-	&dev_attr_n_external_timestamps.attr,
-	&dev_attr_n_periodic_outputs.attr,
-	&dev_attr_n_programmable_pins.attr,
-	&dev_attr_pps_available.attr,
-	NULL,
-};
-
-static const struct attribute_group ptp_group = {
-	.attrs = ptp_attrs,
-};
-
-const struct attribute_group *ptp_groups[] = {
-	&ptp_group,
-	NULL,
-};
-
-
 static ssize_t extts_enable_store(struct device *dev,
 				  struct device_attribute *attr,
 				  const char *buf, size_t count)

@@ -91,6 +70,7 @@ static ssize_t extts_enable_store(struct device *dev,
 out:
 	return err;
 }
+static DEVICE_ATTR(extts_enable, 0220, NULL, extts_enable_store);
 
 static ssize_t extts_fifo_show(struct device *dev,
 			       struct device_attribute *attr, char *page)

@@ -124,6 +104,7 @@ static ssize_t extts_fifo_show(struct device *dev,
 	mutex_unlock(&ptp->tsevq_mux);
 	return cnt;
 }
+static DEVICE_ATTR(fifo, 0444, extts_fifo_show, NULL);
 
 static ssize_t period_store(struct device *dev,
 			    struct device_attribute *attr,

@@ -151,6 +132,7 @@ static ssize_t period_store(struct device *dev,
 out:
 	return err;
 }
+static DEVICE_ATTR(period, 0220, NULL, period_store);
 
 static ssize_t pps_enable_store(struct device *dev,
 				struct device_attribute *attr,

@@ -177,6 +159,57 @@ static ssize_t pps_enable_store(struct device *dev,
 out:
 	return err;
 }
+static DEVICE_ATTR(pps_enable, 0220, NULL, pps_enable_store);
+
+static struct attribute *ptp_attrs[] = {
+	&dev_attr_clock_name.attr,
+
+	&dev_attr_max_adjustment.attr,
+	&dev_attr_n_alarms.attr,
+	&dev_attr_n_external_timestamps.attr,
+	&dev_attr_n_periodic_outputs.attr,
+	&dev_attr_n_programmable_pins.attr,
+	&dev_attr_pps_available.attr,
+
+	&dev_attr_extts_enable.attr,
+	&dev_attr_fifo.attr,
+	&dev_attr_period.attr,
+	&dev_attr_pps_enable.attr,
+	NULL
+};
+
+static umode_t ptp_is_attribute_visible(struct kobject *kobj,
+					struct attribute *attr, int n)
+{
+	struct device *dev = kobj_to_dev(kobj);
+	struct ptp_clock *ptp = dev_get_drvdata(dev);
+	struct ptp_clock_info *info = ptp->info;
+	umode_t mode = attr->mode;
+
+	if (attr == &dev_attr_extts_enable.attr ||
+	    attr == &dev_attr_fifo.attr) {
+		if (!info->n_ext_ts)
+			mode = 0;
+	} else if (attr == &dev_attr_period.attr) {
+		if (!info->n_per_out)
+			mode = 0;
+	} else if (attr == &dev_attr_pps_enable.attr) {
+		if (!info->pps)
+			mode = 0;
+	}
+
+	return mode;
+}
+
+static const struct attribute_group ptp_group = {
+	.is_visible	= ptp_is_attribute_visible,
+	.attrs		= ptp_attrs,
+};
+
+const struct attribute_group *ptp_groups[] = {
+	&ptp_group,
+	NULL
+};
 
 static int ptp_pin_name2index(struct ptp_clock *ptp, const char *name)
 {

@@ -235,26 +268,11 @@ static ssize_t ptp_pin_store(struct device *dev, struct device_attribute *attr,
 	return count;
 }
 
-static DEVICE_ATTR(extts_enable, 0220, NULL, extts_enable_store);
-static DEVICE_ATTR(fifo,         0444, extts_fifo_show, NULL);
-static DEVICE_ATTR(period,       0220, NULL, period_store);
-static DEVICE_ATTR(pps_enable,   0220, NULL, pps_enable_store);
-
 int ptp_cleanup_sysfs(struct ptp_clock *ptp)
 {
 	struct device *dev = ptp->dev;
 	struct ptp_clock_info *info = ptp->info;
 
-	if (info->n_ext_ts) {
-		device_remove_file(dev, &dev_attr_extts_enable);
-		device_remove_file(dev, &dev_attr_fifo);
-	}
-	if (info->n_per_out)
-		device_remove_file(dev, &dev_attr_period);
-
-	if (info->pps)
-		device_remove_file(dev, &dev_attr_pps_enable);
-
 	if (info->n_pins) {
 		sysfs_remove_group(&dev->kobj, &ptp->pin_attr_group);
 		kfree(ptp->pin_attr);

@@ -306,46 +324,13 @@ static int ptp_populate_pins(struct ptp_clock *ptp)
 
 int ptp_populate_sysfs(struct ptp_clock *ptp)
 {
-	struct device *dev = ptp->dev;
 	struct ptp_clock_info *info = ptp->info;
 	int err;
 
-	if (info->n_ext_ts) {
-		err = device_create_file(dev, &dev_attr_extts_enable);
-		if (err)
-			goto out1;
-		err = device_create_file(dev, &dev_attr_fifo);
-		if (err)
-			goto out2;
-	}
-	if (info->n_per_out) {
-		err = device_create_file(dev, &dev_attr_period);
-		if (err)
-			goto out3;
-	}
-	if (info->pps) {
-		err = device_create_file(dev, &dev_attr_pps_enable);
-		if (err)
-			goto out4;
-	}
 	if (info->n_pins) {
 		err = ptp_populate_pins(ptp);
 		if (err)
-			goto out5;
+			return err;
 	}
 	return 0;
-out5:
-	if (info->pps)
-		device_remove_file(dev, &dev_attr_pps_enable);
-out4:
-	if (info->n_per_out)
-		device_remove_file(dev, &dev_attr_period);
-out3:
-	if (info->n_ext_ts)
-		device_remove_file(dev, &dev_attr_fifo);
-out2:
-	if (info->n_ext_ts)
-		device_remove_file(dev, &dev_attr_extts_enable);
-out1:
-	return err;
 }

-- 
2.11.0.483.g087da7b7c-goog

[PATCH v2 2/4] ptp: use kcalloc when allocating arrays

From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Date: 2017-02-14 18:24:38

kcalloc is more semantically correct when allocating arrays of objects, and
overflow-safe.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/ptp/ptp_sysfs.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/ptp/ptp_sysfs.c b/drivers/ptp/ptp_sysfs.c
index 53d43954a974..27cd46ab5e32 100644
--- a/drivers/ptp/ptp_sysfs.c
+++ b/drivers/ptp/ptp_sysfs.c

@@ -269,13 +269,12 @@ static int ptp_populate_pins(struct ptp_clock *ptp)
 	struct ptp_clock_info *info = ptp->info;
 	int err = -ENOMEM, i, n_pins = info->n_pins;
 
-	ptp->pin_dev_attr = kzalloc(n_pins * sizeof(*ptp->pin_dev_attr),
+	ptp->pin_dev_attr = kcalloc(n_pins, sizeof(*ptp->pin_dev_attr),
 				    GFP_KERNEL);
 	if (!ptp->pin_dev_attr)
 		goto no_dev_attr;
 
-	ptp->pin_attr = kzalloc((1 + n_pins) * sizeof(struct attribute *),
-				GFP_KERNEL);
+	ptp->pin_attr = kcalloc(1 + n_pins, sizeof(*ptp->pin_attr), GFP_KERNEL);
 	if (!ptp->pin_attr)
 		goto no_pin_attr;

-- 
2.11.0.483.g087da7b7c-goog

Re: [PATCH v2 0/4] PTP attribute handling cleanup

From: David Miller <davem@davemloft.net>
Date: 2017-02-15 17:22:18

From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Date: Tue, 14 Feb 2017 10:23:30 -0800

PTP core was creating some attributes, such as "period" and "fifo", and the
entire "pins" attribute group, after creating class deevice, which creates
a race for userspace: uevent may arrive before all attributes are created.

This series of patches switches PTP to use is_visible() to control
visibility of attributes in a group, and device_create_with_groups() to
ensure that attributes are created before we notify userspace of a new
device.

Richard, please review.

Re: [PATCH v2 0/4] PTP attribute handling cleanup

From: Richard Cochran <richardcochran@gmail.com>
Date: 2017-02-16 10:30:49

On Wed, Feb 15, 2017 at 12:22:15PM -0500, David Miller wrote:

Richard, please review.

Ok, I am testing this today or tomorrow.
I'll report back as soon as I can.

Thanks,
Richard

Re: [PATCH v2 0/4] PTP attribute handling cleanup

From: Richard Cochran <richardcochran@gmail.com>
Date: 2017-02-16 20:19:37

On Tue, Feb 14, 2017 at 10:23:30AM -0800, Dmitry Torokhov wrote:

 drivers/ptp/ptp_clock.c   |  22 +++---
 drivers/ptp/ptp_private.h |   7 +-
 drivers/ptp/ptp_sysfs.c   | 167 ++++++++++++++++++----------------------------
 3 files changed, 80 insertions(+), 116 deletions(-)

Nice reduction in sysfs code!

I gave this some light testing on the coldfire/phyter (which has 12
pins), and nothing exploded.

For the series:

Acked-by: Richard Cochran <richardcochran@gmail.com>

Re: [PATCH v2 0/4] PTP attribute handling cleanup

From: David Miller <davem@davemloft.net>
Date: 2017-02-17 16:03:40

From: Richard Cochran <richardcochran@gmail.com>
Date: Thu, 16 Feb 2017 21:19:29 +0100

On Tue, Feb 14, 2017 at 10:23:30AM -0800, Dmitry Torokhov wrote:

quoted

 drivers/ptp/ptp_clock.c   |  22 +++---
 drivers/ptp/ptp_private.h |   7 +-
 drivers/ptp/ptp_sysfs.c   | 167 ++++++++++++++++++----------------------------
 3 files changed, 80 insertions(+), 116 deletions(-)

Nice reduction in sysfs code!

I gave this some light testing on the coldfire/phyter (which has 12
pins), and nothing exploded.

For the series:

Acked-by: Richard Cochran <richardcochran@gmail.com>

Series applied to net-next, thanks.

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help