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

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

[Dmitry Torokhov]

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

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 2/4] ptp: use kcalloc/kmallco_array when allocating arrays

[Dmitry Torokhov]

kcalloc/kmalloc_array are 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 | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/ptp/ptp_sysfs.c b/drivers/ptp/ptp_sysfs.c
index 53d43954a974..77843790c381 100644
--- a/drivers/ptp/ptp_sysfs.c
+++ b/drivers/ptp/ptp_sysfs.c
@@ -269,13 +269,13 @@ 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 = kmalloc_array(1 + n_pins, sizeof(*ptp->pin_attr),
+				      GFP_KERNEL);
 	if (!ptp->pin_attr)
 		goto no_pin_attr;
 
@@ -289,6 +289,9 @@ static int ptp_populate_pins(struct ptp_clock *ptp)
 		ptp->pin_attr[i] = &da->attr;
 	}
 
+	/* NULL terminator */
+	ptp->pin_attr[n_pins] = NULL;
+
 	ptp->pin_attr_group.name = "pins";
 	ptp->pin_attr_group.attrs = ptp->pin_attr;
 
-- 
2.11.0.483.g087da7b7c-goog

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

[Richard Cochran]

On Mon, Feb 13, 2017 at 07:51:06PM -0800, Dmitry Torokhov wrote:
> @@ -269,13 +269,13 @@ 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 = kmalloc_array(1 + n_pins, sizeof(*ptp->pin_attr),
> +				      GFP_KERNEL);

I prefer kcalloc here as well, even if it isn't strictly necessary
according to the current usage of pin_attr.  That way, any future
changes to the pin handling code won't have to worry about
uninitialized memory.  After all, this is hardly a performance path.

>  	if (!ptp->pin_attr)
>  		goto no_pin_attr;
>  
> @@ -289,6 +289,9 @@ static int ptp_populate_pins(struct ptp_clock *ptp)
>  		ptp->pin_attr[i] = &da->attr;
>  	}
>  
> +	/* NULL terminator */
> +	ptp->pin_attr[n_pins] = NULL;

And drop this then, please.

Thanks,
Richard

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

[Dmitry Torokhov]

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 77843790c381..a6292744c7ed 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);
@@ -310,46 +328,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

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

[Richard Cochran]

On Mon, Feb 13, 2017 at 07:51:07PM -0800, Dmitry Torokhov wrote:
> 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.

At first glance, this patch and the next look like nice improvements.
I don't futz around with sysfs code very often, and so may I ask how
or whether you tested it?

Thanks,
Richard

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

[Dmitry Torokhov]

Hi Richard,

On Tue, Feb 14, 2017 at 09:42:26AM +0100, Richard Cochran wrote:
> On Mon, Feb 13, 2017 at 07:51:07PM -0800, Dmitry Torokhov wrote:
> > 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.
> 
> At first glance, this patch and the next look like nice improvements.
> I don't futz around with sysfs code very often, and so may I ask how
> or whether you tested it?

I used the hack below.

Thanks.

-- 
Dmitry

PTP Test

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/net/ethernet/intel/e1000e/ptp.c |   13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/e1000e/ptp.c b/drivers/net/ethernet/intel/e1000e/ptp.c
index 34cc3be0df8e..3b4c0d3e4093 100644
--- a/drivers/net/ethernet/intel/e1000e/ptp.c
+++ b/drivers/net/ethernet/intel/e1000e/ptp.c
@@ -260,18 +260,27 @@ static void e1000e_systim_overflow_work(struct work_struct *work)
 			      E1000_SYSTIM_OVERFLOW_PERIOD);
 }
 
+static struct ptp_pin_desc e1000e_ptp_pin_config[1] = {
+	{
+		.name = "TestPin1",
+		.index = 0,
+		.func = PTP_PF_NONE,
+	}
+};
+
 static const struct ptp_clock_info e1000e_ptp_clock_info = {
 	.owner		= THIS_MODULE,
 	.n_alarm	= 0,
 	.n_ext_ts	= 0,
-	.n_per_out	= 0,
-	.n_pins		= 0,
+	.n_per_out	= 1,
+	.n_pins		= 1,
 	.pps		= 0,
 	.adjfreq	= e1000e_phc_adjfreq,
 	.adjtime	= e1000e_phc_adjtime,
 	.gettime64	= e1000e_phc_gettime,
 	.settime64	= e1000e_phc_settime,
 	.enable		= e1000e_phc_enable,
+	.pin_config	= e1000e_ptp_pin_config,
 };
 
 /**

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

[Richard Cochran]

On Tue, Feb 14, 2017 at 10:07:47AM -0800, Dmitry Torokhov wrote:
> > At first glance, this patch and the next look like nice improvements.
> > I don't futz around with sysfs code very often, and so may I ask how
> > or whether you tested it?
> 
> I used the hack below.

OK, thats fine.  I'll be sure and test this here with my i210 which
has real pins.

Thanks,
Richard

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

[Dmitry Torokhov]

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 a6292744c7ed..f8813ca8801c 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)
@@ -313,28 +302,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

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

[Richard Cochran]

Dmitry,

Be sure to put davem onto CC next time, as he merges any PTP work.

On Mon, Feb 13, 2017 at 07:51:05PM -0800, Dmitry Torokhov wrote:
> 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>