Messages in this thread | | | From | Stephen Boyd <> | Subject | Re: [PATCH v5] PM / wakeup: show wakeup sources stats in sysfs | Date | Tue, 30 Jul 2019 16:41:38 -0700 |
| |
Quoting Rafael J. Wysocki (2019-07-30 16:05:55) > On Wed, Jul 31, 2019 at 12:26 AM Stephen Boyd <swboyd@chromium.org> wrote: > > > > Quoting Rafael J. Wysocki (2019-07-30 15:17:55) > > > On Tuesday, July 30, 2019 8:48:09 PM CEST Stephen Boyd wrote: > > > > > > > > Using the same prefix for the class and the device name is quite common. > > > > For example, see the input, regulator, tty, tpm, remoteproc, hwmon, > > > > extcon classes. I'd prefer it was left as /sys/class/wakeup/wakeupN. The > > > > class name could be changed to wakeup_source perhaps (i.e. > > > > /sys/class/wakeup_source/wakeupN)? > > > > > > Alternatively /sys/class/wakeup/wsN > > > > > > > Or /sys/class/wakeup/eventN? It's your bikeshed to paint. > > So actually the underlying problem here is that device_wakeup_enable() > tries to register a wakeup source and then attach it to the device to > avoid calling possibly sleeping functions under a spinlock.
Agreed, that is one problem.
> > However, it should be possible to call wakeup_source_create(name) > first, then attach the wakeup source to the device (after checking for > presence), and then invoke wakeup_source_add() (after dropping the > lock). If the wakeup source virtual device registration is done in > wakeup_source_add(), that should avoid the problem altogether without > having to introduce extra complexity.
While reordering the code to do what you describe will fix this specific duplicate name problem, it won't fix the general problem with reusing device names from one bus on a different bus/class. We can run into the same problem when two buses name their devices the same name and then we attempt to attach a wakeup source to those two devices. Or we can have a problem where a virtual wakeup is made with the same name, and again we'll try to make a duplicate named device. Using something like 'event' or 'wakeup' or 'ws' as the prefix avoids this problem and keeps things clean.
We should probably avoid letting the same virtual wakeup source be made with the same name anyway, because userspace will be confused about what virtual wakeup it is otherwise. I concede that using the name of the wakeup source catches this problem without adding extra code.
Either way, I'd like to see what you outline implemented so that we don't need to do more work than is necessary when userspace writes to the file. I just don't want to see us need to change the name of the wakeup device later on and then add a 'name' attribute to the class so that we can avoid name collisions due to various buses controlling the string we use to create the name of the wakeup device.
| |