* [PATCH v2] pwm: Create device class for pwm channels
@ 2016-07-18 22:14 David Hsu
2016-09-05 14:41 ` Thierry Reding
2016-09-05 15:20 ` Thierry Reding
0 siblings, 2 replies; 5+ messages in thread
From: David Hsu @ 2016-07-18 22:14 UTC (permalink / raw)
To: thierry.reding; +Cc: gregkh, sspatil, linux-pwm, linux-kernel, davidhsu
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(-)
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");
}
}
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2] pwm: Create device class for pwm channels
2016-07-18 22:14 [PATCH v2] pwm: Create device class for pwm channels David Hsu
@ 2016-09-05 14:41 ` Thierry Reding
2016-09-05 15:15 ` Thierry Reding
2016-09-05 15:20 ` Thierry Reding
1 sibling, 1 reply; 5+ messages in thread
From: Thierry Reding @ 2016-09-05 14:41 UTC (permalink / raw)
To: David Hsu; +Cc: gregkh, sspatil, linux-pwm, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 5782 bytes --]
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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] pwm: Create device class for pwm channels
2016-09-05 14:41 ` Thierry Reding
@ 2016-09-05 15:15 ` Thierry Reding
2016-09-05 15:42 ` Thierry Reding
0 siblings, 1 reply; 5+ messages in thread
From: Thierry Reding @ 2016-09-05 15:15 UTC (permalink / raw)
To: David Hsu; +Cc: gregkh, sspatil, linux-pwm, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 6374 bytes --]
On Mon, Sep 05, 2016 at 04:41:39PM +0200, Thierry Reding wrote:
> On Mon, Jul 18, 2016 at 03:14:50PM -0700, David Hsu wrote:
[...]
> > 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;
>
> 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?
*sigh*, nevermind. I realized too late that I hadn't fully applied your
patch and the change to use device_create_with_groups() actually gets
rid of this.
Unfortunately....
> > 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.
... that's still there. However the output is now somewhat cleaner and
the ->uevent() callback doesn't seem to be called recursively but rather
repeatedly (until I unexport the PWM again).
Let me investigate a little more where this is coming from. For
reference, I have the below on top of your patch.
Thierry
--- >8 ---
diff --git a/drivers/pwm/sysfs.c b/drivers/pwm/sysfs.c
index 2ab5ed61d496..45e56a04f091 100644
--- a/drivers/pwm/sysfs.c
+++ b/drivers/pwm/sysfs.c
@@ -241,8 +241,17 @@ static struct attribute *pwm_attrs[] = {
};
ATTRIBUTE_GROUPS(pwm);
+static int pwm_uevent(struct device *dev, struct kobj_uevent_env *env)
+{
+ int err = 0;
+ dev_info(dev, "> %s(dev=%p, env=%p)\n", __func__, dev, env);
+ dev_info(dev, "< %s() = %d\n", __func__, err);
+ return err;
+}
+
static const struct device_type pwm_channel_type = {
- .name = "pwm_channel",
+ .name = "pwm_channel",
+ .uevent = pwm_uevent,
};
static void pwm_export_release(struct device *child)
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2] pwm: Create device class for pwm channels
2016-07-18 22:14 [PATCH v2] pwm: Create device class for pwm channels David Hsu
2016-09-05 14:41 ` Thierry Reding
@ 2016-09-05 15:20 ` Thierry Reding
1 sibling, 0 replies; 5+ messages in thread
From: Thierry Reding @ 2016-09-05 15:20 UTC (permalink / raw)
To: David Hsu; +Cc: gregkh, sspatil, linux-pwm, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 1879 bytes --]
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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] pwm: Create device class for pwm channels
2016-09-05 15:15 ` Thierry Reding
@ 2016-09-05 15:42 ` Thierry Reding
0 siblings, 0 replies; 5+ messages in thread
From: Thierry Reding @ 2016-09-05 15:42 UTC (permalink / raw)
To: David Hsu; +Cc: gregkh, sspatil, linux-pwm, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 10230 bytes --]
On Mon, Sep 05, 2016 at 05:15:02PM +0200, Thierry Reding wrote:
> On Mon, Sep 05, 2016 at 04:41:39PM +0200, Thierry Reding wrote:
> > On Mon, Jul 18, 2016 at 03:14:50PM -0700, David Hsu wrote:
> [...]
> > > 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;
> >
> > 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?
>
> *sigh*, nevermind. I realized too late that I hadn't fully applied your
> patch and the change to use device_create_with_groups() actually gets
> rid of this.
>
> Unfortunately....
>
> > > 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.
>
> ... that's still there. However the output is now somewhat cleaner and
> the ->uevent() callback doesn't seem to be called recursively but rather
> repeatedly (until I unexport the PWM again).
>
> Let me investigate a little more where this is coming from. For
> reference, I have the below on top of your patch.
Calling dump_stack() in pwm_uevent() indicates that someone in userspace
keeps reading the file:
[ 65.685811] pwm pwmchip0:0: > pwm_uevent(dev=c2ac1400, env=eeb2e000)
[ 65.692388] CPU: 0 PID: 282 Comm: sh Not tainted 4.8.0-rc3-next-20160825-00027-gcd8380ab5fe3-dirty #90
[ 65.701706] Hardware name: NVIDIA Tegra SoC (Flattened Device Tree)
[ 65.708076] [<c020f26c>] (unwind_backtrace) from [<c020a704>] (show_stack+0x10/0x14)
[ 65.715877] [<c020a704>] (show_stack) from [<c04721d8>] (dump_stack+0x88/0x9c)
[ 65.723142] [<c04721d8>] (dump_stack) from [<c04b1cac>] (pwm_uevent+0x30/0x54)
[ 65.730449] [<c04b1cac>] (pwm_uevent) from [<c0535970>] (dev_uevent+0xe4/0x1cc)
[ 65.737810] [<c0535970>] (dev_uevent) from [<c0475538>] (kobject_uevent_env+0x200/0x508)
[ 65.745938] [<c0475538>] (kobject_uevent_env) from [<c0535380>] (device_add+0x364/0x568)
[ 65.754056] [<c0535380>] (device_add) from [<c04b1ddc>] (export_store+0x10c/0x174)
[ 65.761666] [<c04b1ddc>] (export_store) from [<c035fc98>] (kernfs_fop_write+0xc0/0x1cc)
[ 65.769712] [<c035fc98>] (kernfs_fop_write) from [<c030167c>] (__vfs_write+0x1c/0x114)
[ 65.777662] [<c030167c>] (__vfs_write) from [<c0302464>] (vfs_write+0xa4/0x168)
[ 65.785002] [<c0302464>] (vfs_write) from [<c03031b0>] (SyS_write+0x3c/0x90)
[ 65.792091] [<c03031b0>] (SyS_write) from [<c0207440>] (ret_fast_syscall+0x0/0x34)
[ 65.799866] pwm pwmchip0:0: < pwm_uevent() = 0
[ 65.803675] pwm pwmchip0:0: > pwm_uevent(dev=c2ac1400, env=eeaba000)
[ 65.803706] CPU: 2 PID: 191 Comm: systemd-journal Not tainted 4.8.0-rc3-next-20160825-00027-gcd8380ab5fe3-dirty #90
[ 65.803718] Hardware name: NVIDIA Tegra SoC (Flattened Device Tree)
[ 65.803791] [<c020f26c>] (unwind_backtrace) from [<c020a704>] (show_stack+0x10/0x14)
[ 65.803838] [<c020a704>] (show_stack) from [<c04721d8>] (dump_stack+0x88/0x9c)
[ 65.803880] [<c04721d8>] (dump_stack) from [<c04b1cac>] (pwm_uevent+0x30/0x54)
[ 65.803917] [<c04b1cac>] (pwm_uevent) from [<c0535970>] (dev_uevent+0xe4/0x1cc)
[ 65.803949] [<c0535970>] (dev_uevent) from [<c0533c1c>] (uevent_show+0xb0/0x11c)
[ 65.803977] [<c0533c1c>] (uevent_show) from [<c05341c4>] (dev_attr_show+0x1c/0x48)
[ 65.804018] [<c05341c4>] (dev_attr_show) from [<c036067c>] (sysfs_kf_seq_show+0x88/0xf8)
[ 65.804059] [<c036067c>] (sysfs_kf_seq_show) from [<c03243a4>] (seq_read+0x1ec/0x49c)
[ 65.804093] [<c03243a4>] (seq_read) from [<c0301570>] (__vfs_read+0x1c/0x10c)
[ 65.804125] [<c0301570>] (__vfs_read) from [<c0302334>] (vfs_read+0x8c/0x118)
[ 65.804157] [<c0302334>] (vfs_read) from [<c0303120>] (SyS_read+0x3c/0x90)
[ 65.804194] [<c0303120>] (SyS_read) from [<c0207440>] (ret_fast_syscall+0x0/0x34)
[ 65.804220] pwm pwmchip0:0: < pwm_uevent() = 0
[ 65.815938] pwm pwmchip0:0: > pwm_uevent(dev=c2ac1400, env=eeaba000)
[ 65.815969] CPU: 2 PID: 191 Comm: systemd-journal Not tainted 4.8.0-rc3-next-20160825-00027-gcd8380ab5fe3-dirty #90
[ 65.815981] Hardware name: NVIDIA Tegra SoC (Flattened Device Tree)
[ 65.816056] [<c020f26c>] (unwind_backtrace) from [<c020a704>] (show_stack+0x10/0x14)
[ 65.816103] [<c020a704>] (show_stack) from [<c04721d8>] (dump_stack+0x88/0x9c)
[ 65.816143] [<c04721d8>] (dump_stack) from [<c04b1cac>] (pwm_uevent+0x30/0x54)
[ 65.816182] [<c04b1cac>] (pwm_uevent) from [<c0535970>] (dev_uevent+0xe4/0x1cc)
[ 65.816211] [<c0535970>] (dev_uevent) from [<c0533c1c>] (uevent_show+0xb0/0x11c)
[ 65.816238] [<c0533c1c>] (uevent_show) from [<c05341c4>] (dev_attr_show+0x1c/0x48)
[ 65.816280] [<c05341c4>] (dev_attr_show) from [<c036067c>] (sysfs_kf_seq_show+0x88/0xf8)
[ 65.816321] [<c036067c>] (sysfs_kf_seq_show) from [<c03243a4>] (seq_read+0x1ec/0x49c)
[ 65.816354] [<c03243a4>] (seq_read) from [<c0301570>] (__vfs_read+0x1c/0x10c)
[ 65.816386] [<c0301570>] (__vfs_read) from [<c0302334>] (vfs_read+0x8c/0x118)
[ 65.816418] [<c0302334>] (vfs_read) from [<c0303120>] (SyS_read+0x3c/0x90)
[ 65.816455] [<c0303120>] (SyS_read) from [<c0207440>] (ret_fast_syscall+0x0/0x34)
[ 65.816479] pwm pwmchip0:0: < pwm_uevent() = 0
Even adding content to the env object doesn't change this. Am I using
this incorrectly?
Thierry
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-09-05 15:42 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-07-18 22:14 [PATCH v2] pwm: Create device class for pwm channels David Hsu
2016-09-05 14:41 ` Thierry Reding
2016-09-05 15:15 ` Thierry Reding
2016-09-05 15:42 ` Thierry Reding
2016-09-05 15:20 ` Thierry Reding
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).