[v2] pwm: Create device class for pwm channels

Submitted by David Hsu on July 18, 2016, 10:14 p.m.

Details

Message ID 1468880090-46076-1-git-send-email-davidhsu@google.com
State New
Headers show

Commit Message

David Hsu July 18, 2016, 10:14 p.m.
Pwm channels don't send uevents when exported, this change adds the
channels to a pwm class and set their device type to pwm_channel so
uevents are sent.

To do this properly, the device names need to change to uniquely
identify a channel.  This change is from pwmN to pwmchipM:N

Signed-off-by: David Hsu <davidhsu@google.com>
---
v2: Use parent name instead of chip->base for channel naming.

 Documentation/pwm.txt |  6 ++++--
 drivers/pwm/sysfs.c   | 18 +++++++++++++-----
 2 files changed, 17 insertions(+), 7 deletions(-)

Comments

Thierry Reding Sept. 5, 2016, 2:41 p.m.
On Mon, Jul 18, 2016 at 03:14:50PM -0700, David Hsu wrote:
> Pwm channels don't send uevents when exported, this change adds the
> channels to a pwm class and set their device type to pwm_channel so
> uevents are sent.
> 
> To do this properly, the device names need to change to uniquely
> identify a channel.  This change is from pwmN to pwmchipM:N
> 
> Signed-off-by: David Hsu <davidhsu@google.com>
> ---
> v2: Use parent name instead of chip->base for channel naming.
> 
>  Documentation/pwm.txt |  6 ++++--
>  drivers/pwm/sysfs.c   | 18 +++++++++++++-----
>  2 files changed, 17 insertions(+), 7 deletions(-)

Sorry for taking so long to look at this. I had applied this to my tree
and was doing some experiments when I noticed some oddities.

> diff --git a/drivers/pwm/sysfs.c b/drivers/pwm/sysfs.c
> index 01695d4..cb2b376 100644
> --- a/drivers/pwm/sysfs.c
> +++ b/drivers/pwm/sysfs.c
> @@ -23,6 +23,8 @@
>  #include <linux/kdev_t.h>
>  #include <linux/pwm.h>
>  
> +static struct class pwm_class;
> +
>  struct pwm_export {
>  	struct device child;
>  	struct pwm_device *pwm;
> @@ -222,6 +224,10 @@ static struct attribute *pwm_attrs[] = {
>  };
>  ATTRIBUTE_GROUPS(pwm);
>  
> +static const struct device_type pwm_channel_type = {
> +	.name		= "pwm_channel",
> +};

In order to do some tracing, I ended up implementing the ->uevent()
callback of struct device_type. What I noticed when exporting a PWM is
that that ->uevent() gets called recursively and the operation never
finishes. I have no idea why that happens, though.

> +
>  static void pwm_export_release(struct device *child)
>  {
>  	struct pwm_export *export = child_to_pwm_export(child);
> @@ -248,9 +254,11 @@ static int pwm_export_child(struct device *parent, struct pwm_device *pwm)
>  
>  	export->child.release = pwm_export_release;
>  	export->child.parent = parent;
> +	export->child.type = &pwm_channel_type;
>  	export->child.devt = MKDEV(0, 0);
> +	export->child.class = &pwm_class;

This particular change isn't going to work, unfortunately. Children of a
PWM chip, i.e. PWM devices (or channels) are not the same as chips. The
above, however, will cause the attributes associated with a PWM chip to
be associated with each PWM device as well. For example it will cause a
PWM device to expose an "export" attribute in userspace. Writing to that
file will give you a nice oops, along these lines:

	[   89.631855] Unable to handle kernel NULL pointer dereference at virtual address 00000014
	[   89.640146] pgd = c2b16700
	[   89.642879] [00000014] *pgd=82b74003, *pmd=f4dd8003
	[   89.647830] Internal error: Oops: 207 [#1] PREEMPT SMP ARM
	[   89.653330] Modules linked in: nouveau tegra_drm ttm drm_kms_helper cfbfillrect syscopyarea cfbimgblt sysfillrect sysimgblt fb_sys_fops cfbcopyarea drm
	[   89.667194] CPU: 3 PID: 284 Comm: sh Not tainted 4.8.0-rc3-next-20160825-00026-g7065511b7003-dirty #82
	[   89.676507] Hardware name: NVIDIA Tegra SoC (Flattened Device Tree)
	[   89.682787] task: c2b39500 task.stack: c3034000
	[   89.687327] PC is at export_store+0x34/0x170
	[   89.691598] LR is at _kstrtoull+0x2c/0x70
	[   89.695607] pc : [<c04b1cb0>]    lr : [<c048a0e4>]    psr: 60000013
	[   89.695607] sp : c3035ea0  ip : c04b1c7c  fp : bec8eb0c
	[   89.707068] r10: ee1e9b8c  r9 : c3035f80  r8 : 00000002
	[   89.712287] r7 : c2b70800  r6 : 00000000  r5 : ee1e9b80  r4 : 00000000
	[   89.718806] r3 : 00000000  r2 : 00000000  r1 : 00000000  r0 : 00000000
	[   89.725327] Flags: nZCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment user
	[   89.732453] Control: 30c5387d  Table: 82b16700  DAC: 55555555
	[   89.738193] Process sh (pid: 284, stack limit = 0xc3034210)
	[   89.743758] Stack: (0xc3035ea0 to 0xc3036000)
	[   89.748116] 5ea0: ee1e9b8c 00000000 0014e408 00000002 ee1e9b80 00000000 00000000 ee19f180
	[   89.756286] 5ec0: c3035f80 c035fc98 00000000 00000000 c3070180 c035fbd8 0014e408 c3035f80
	[   89.764456] 5ee0: 00000000 00000002 00000000 c030167c 032cf000 c109bf44 c109bf44 c02f88b0
	[   89.772625] 5f00: ee8270c0 c02fac24 00000001 c3035f10 ef05c9e0 c0300fd8 c0327c08 c2b7b000
	[   89.780794] 5f20: 0000000a c2b7e000 c3184ec0 0000000a 00000400 c2b7e040 00000000 c3070180
	[   89.788964] 5f40: 00000002 0014e408 c3035f80 00000000 00000002 c0302464 00000001 0000000a
	[   89.797132] 5f60: c2d076c0 c3070180 c3070180 00000000 00000000 0014e408 00000002 c03031b0
	[   89.805302] 5f80: 00000000 00000000 00000014 00000002 0014e408 b6e74d50 00000004 c02075e4
	[   89.813470] 5fa0: c3034000 c0207440 00000002 0014e408 00000001 0014e408 00000002 00000000
	[   89.821638] 5fc0: 00000002 0014e408 b6e74d50 00000004 00000002 00000004 b6f0e000 bec8eb0c
	[   89.829808] 5fe0: 00000000 bec8ea64 b6d9ffa4 b6df970c 60000010 00000001 0b0a0908 0f0e0d0c
	[   89.837996] [<c04b1cb0>] (export_store) from [<c035fc98>] (kernfs_fop_write+0xc0/0x1cc)
	[   89.846005] [<c035fc98>] (kernfs_fop_write) from [<c030167c>] (__vfs_write+0x1c/0x114)
	[   89.853940] [<c030167c>] (__vfs_write) from [<c0302464>] (vfs_write+0xa4/0x168)
	[   89.861252] [<c0302464>] (vfs_write) from [<c03031b0>] (SyS_write+0x3c/0x90)
	[   89.868304] [<c03031b0>] (SyS_write) from [<c0207440>] (ret_fast_syscall+0x0/0x34)
	[   89.875883] Code: ebff6164 e2506000 ba000039 e59d1004 (e5943014) 
	[   89.882225] ---[ end trace 716eda7e65a4136a ]---

Given that we need a class associated with a device in order for it to
generate uevents (why is that so, by the way?), I think we'd need to add
a separate class implementation for PWM devices. That has the downside
of adding yet another subdirectory to /sys/class, but I can't really
think of another way of achieving what you need here.

Greg, any ideas?

Thierry
Thierry Reding Sept. 5, 2016, 3:20 p.m.
On Mon, Jul 18, 2016 at 03:14:50PM -0700, David Hsu wrote:
> Pwm channels don't send uevents when exported, this change adds the
> channels to a pwm class and set their device type to pwm_channel so
> uevents are sent.
> 
> To do this properly, the device names need to change to uniquely
> identify a channel.  This change is from pwmN to pwmchipM:N
> 
> Signed-off-by: David Hsu <davidhsu@google.com>
> ---
> v2: Use parent name instead of chip->base for channel naming.
> 
>  Documentation/pwm.txt |  6 ++++--
>  drivers/pwm/sysfs.c   | 18 +++++++++++++-----
>  2 files changed, 17 insertions(+), 7 deletions(-)

Forgot to mention one other thing that had come to my mind...

> diff --git a/drivers/pwm/sysfs.c b/drivers/pwm/sysfs.c
[...]
> @@ -248,9 +254,11 @@ static int pwm_export_child(struct device *parent, struct pwm_device *pwm)
>  
>  	export->child.release = pwm_export_release;
>  	export->child.parent = parent;
> +	export->child.type = &pwm_channel_type;
>  	export->child.devt = MKDEV(0, 0);
> +	export->child.class = &pwm_class;
>  	export->child.groups = pwm_groups;
> -	dev_set_name(&export->child, "pwm%u", pwm->hwpwm);
> +	dev_set_name(&export->child, "%s:%u", dev_name(parent), pwm->hwpwm);

Using the colon as separator could possibly prevent the use of these
files in paths lists. Consider the case where somebody wanted to give a
list of PWM sysfs references:

	PWM_DEVICES=/sys/class/pwm/pwmchip0:0:/sys/class/pwm/pwmchip0:1

which might get parsed as four entries:

	- /sys/class/pwm/pwmchip0
	- 0
	- /sys/class/pwm/pwmchip0
	- 1

none of which point to a valid PWM. There are a couple of other
separators that might be possible, such as '-' or '.'.

Then again, looking at existing usage in sysfs it seems like the colon
is a fairly popular separator, so maybe I'm just being too paranoid.

Thierry

Patch hide | download patch | download mbox

diff --git a/Documentation/pwm.txt b/Documentation/pwm.txt
index 789b27c..8453435 100644
--- a/Documentation/pwm.txt
+++ b/Documentation/pwm.txt
@@ -80,9 +80,11 @@  unexport - Unexports a PWM channel from sysfs (write-only).
 
 The PWM channels are numbered using a per-chip index from 0 to npwm-1.
 
-When a PWM channel is exported a pwmX directory will be created in the
+When a PWM channel is exported a pwmchipN:X directory will be created in the
 pwmchipN directory it is associated with, where X is the number of the
-channel that was exported. The following properties will then be available:
+channel that was exported. It will also be exposed at /sys/class/pwm/ and
+can be identified by the pwm_channel device type.
+The following properties will then be available:
 
 period - The total period of the PWM signal (read/write).
 	Value is in nanoseconds and is the sum of the active and inactive
diff --git a/drivers/pwm/sysfs.c b/drivers/pwm/sysfs.c
index 01695d4..cb2b376 100644
--- a/drivers/pwm/sysfs.c
+++ b/drivers/pwm/sysfs.c
@@ -23,6 +23,8 @@ 
 #include <linux/kdev_t.h>
 #include <linux/pwm.h>
 
+static struct class pwm_class;
+
 struct pwm_export {
 	struct device child;
 	struct pwm_device *pwm;
@@ -222,6 +224,10 @@  static struct attribute *pwm_attrs[] = {
 };
 ATTRIBUTE_GROUPS(pwm);
 
+static const struct device_type pwm_channel_type = {
+	.name		= "pwm_channel",
+};
+
 static void pwm_export_release(struct device *child)
 {
 	struct pwm_export *export = child_to_pwm_export(child);
@@ -248,9 +254,11 @@  static int pwm_export_child(struct device *parent, struct pwm_device *pwm)
 
 	export->child.release = pwm_export_release;
 	export->child.parent = parent;
+	export->child.type = &pwm_channel_type;
 	export->child.devt = MKDEV(0, 0);
+	export->child.class = &pwm_class;
 	export->child.groups = pwm_groups;
-	dev_set_name(&export->child, "pwm%u", pwm->hwpwm);
+	dev_set_name(&export->child, "%s:%u", dev_name(parent), pwm->hwpwm);
 
 	ret = device_register(&export->child);
 	if (ret) {
@@ -355,7 +363,6 @@  ATTRIBUTE_GROUPS(pwm_chip);
 static struct class pwm_class = {
 	.name = "pwm",
 	.owner = THIS_MODULE,
-	.dev_groups = pwm_chip_groups,
 };
 
 static int pwmchip_sysfs_match(struct device *parent, const void *data)
@@ -368,14 +375,15 @@  void pwmchip_sysfs_export(struct pwm_chip *chip)
 	struct device *parent;
 
 	/*
-	 * If device_create() fails the pwm_chip is still usable by
+	 * If device_create_with_groups() fails the pwm_chip is still usable by
 	 * the kernel its just not exported.
 	 */
-	parent = device_create(&pwm_class, chip->dev, MKDEV(0, 0), chip,
+	parent = device_create_with_groups(&pwm_class, chip->dev, MKDEV(0, 0),
+			       chip, pwm_chip_groups,
 			       "pwmchip%d", chip->base);
 	if (IS_ERR(parent)) {
 		dev_warn(chip->dev,
-			 "device_create failed for pwm_chip sysfs export\n");
+			 "device_create_with_groups failed for pwm_chip sysfs export\n");
 	}
 }