From: Thierry Reding <thierry.reding@gmail.com>
To: David Hsu <davidhsu@google.com>
Cc: gregkh@linuxfoundation.org, sspatil@google.com,
linux-pwm@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] pwm: Create device class for pwm channels
Date: Mon, 5 Sep 2016 17:42:09 +0200 [thread overview]
Message-ID: <20160905154209.GU31424@ulmo.ba.sec> (raw)
In-Reply-To: <20160905151502.GS31424@ulmo.ba.sec>
[-- 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 --]
next prev parent reply other threads:[~2016-09-05 15:42 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
2016-09-05 15:20 ` Thierry Reding
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20160905154209.GU31424@ulmo.ba.sec \
--to=thierry.reding@gmail.com \
--cc=davidhsu@google.com \
--cc=gregkh@linuxfoundation.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pwm@vger.kernel.org \
--cc=sspatil@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.