Re: [PATCH v6] PM / wakeup: show wakeup sources stats in sysfs

["Rafael J. Wysocki" <rafael@kernel.org>]

On Thu, Aug 1, 2019 at 11:45 PM Tri Vo <trong@android.com> wrote:
>
> On Thu, Aug 1, 2019 at 1:23 PM Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > Quoting Tri Vo (2019-08-01 12:50:25)
> > > On Wed, Jul 31, 2019 at 4:45 PM Stephen Boyd <swboyd@chromium.org> wrote:
> > > >
> > > > Quoting Rafael J. Wysocki (2019-07-31 16:10:38)
> > > > > On Thu, Aug 1, 2019 at 12:59 AM Tri Vo <trong@android.com> wrote:
> > > > > >
> > > > > > >
> > > > > > > So why wouldn't something like this suffice:
> > > > > > >
> > > > > > > dev = device_create_with_groups(wakeup_class, parent, MKDEV(0, 0), ws,
> > > > > > >                                 wakeup_source_groups, "wakeup:%s", ws->name);
> > > > > > >
> > > > > > > ?
> > > > > >
> > > > > > ws->name is inherited from the device name. IIUC device names are not
> > > > > > guaranteed to be unique. So if different devices with the same name
> > > > > > register wakeup sources, there is an error.
> > > > >
> > > > > OK
> > > > >
> > > > > So I guess the names are retained for backwards compatibility with
> > > > > existing user space that may be using them?
> > > > >
> > > > > That's kind of fair enough, but having two different identification
> > > > > schemes for wakeup sources will end up confusing.
> > > >
> > > > I understand your concern about the IDA now. Thanks for clarifying.
> > > >
> > > > How about we name the devices 'wakeupN' with the IDA when they're
> > > > registered with a non-NULL device pointer and then name them whatever
> > > > the name argument is when the device pointer is NULL. If we have this,
> > > > we should be able to drop the name attribute in sysfs and figure out the
> > > > name either by looking at the device name in /sys/class/wakeup/ if it
> > > > isn't 'wakeupN', or follow the symlink to the device in /sys/devices/
> > > > and look at the parent device name there.
> > >
> > > This makes it difficult for userspace to query the name a wakeup
> > > source, as it now has to first figure out if a wakeup source is
> > > associated with a device or not. The criteria for that is also
> > > awkward, userspase has to check if directory path contains "wakeupN",
> > > then it's a virtual wakeup source.
> >
> > I think you mean if it doesn't match wakeupN then it's a virtual wakeup
> > source?
>
> Yes
> >
> > >
> > > IMO it's cleaner to consistently have /sys/class/wakeup/wakeupN/name
> > > for every wakeup source.
> >
> > I don't find it awkward or difficult. Just know what the name of the
> > /sys/class/wakeup/ path is and then extract the name from there if it
> > doesn't match wakeupN, otherwise read the 'device' symlink and run it
> > through basename.
>
> The concern was that having both "id" and "name" around might be
> confusing. I don't think that making the presence of "name"
> conditional helps here. And we have to maintain additional logic in
> both kernel and userspace to support this.
>
> Also, say, userspace grabs a wakelock named "wakeup0". In the current
> patch, this results in a name collision and an error. Even assuming
> that userspace doesn't have ill intent, it still needs to be aware of
> "wakeupN" naming pattern to avoid this error condition.
>
> All wakeup sources in the /sys/class/wakeup/ are in the same namespace
> regardless of where they originate from, i.e. we have to either (1)
> inspect the name of a wakeup source and make sure it's unique before
> using it as a directory name OR (2) generate the directory name on
> behalf of whomever is registering a wakeup source, which I think is a
> much simpler solution.

OK, whatever.

Let's use the IDA as originally proposed and retain the names for
backwards compatibility only.

Maybe just allocate the ID at the wakeup source object creation time
already (ISTR that you did that before attempting to create a virtual
device for the wakeup source).