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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Thu, 1 Aug 2019 19:21:44 +0200
From:   "Rafael J. Wysocki" <rafael@...nel.org>
To:     Stephen Boyd <swboyd@...omium.org>
Cc:     "Rafael J. Wysocki" <rafael@...nel.org>,
        Tri Vo <trong@...roid.com>,
        "Rafael J. Wysocki" <rjw@...ysocki.net>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Viresh Kumar <viresh.kumar@...aro.org>,
        Hridya Valsaraju <hridya@...gle.com>,
        Sandeep Patil <sspatil@...gle.com>,
        Kalesh Singh <kaleshsingh@...gle.com>,
        Ravi Chandra Sadineni <ravisadineni@...omium.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Linux PM <linux-pm@...r.kernel.org>,
        "Cc: Android Kernel" <kernel-team@...roid.com>
Subject: Re: [PATCH v6] PM / wakeup: show wakeup sources stats in sysfs

On Thu, Aug 1, 2019 at 5:31 PM Stephen Boyd <swboyd@...omium.org> wrote:
>
> Quoting Rafael J. Wysocki (2019-08-01 01:09:22)
> > On Thu, Aug 1, 2019 at 2:45 AM Stephen Boyd <swboyd@...omium.org> wrote:
> > >
> > > Quoting Stephen Boyd (2019-07-31 16:45:31)
> > > >
> > > > This approach also nicely detects duplicate wakeup source names in the
> > > > case that the string passed in to wakeup_source_register() is already
> > > > used on the virtual bus.
> > >
> > > This was clearly untested! Here's a better one. This is what I see on my
> > > device with this patch squashed in:
> > >
> > > localhost ~ # cat /sys/kernel/debug/wakeup_sources
> > > name            active_count    event_count     wakeup_count    expire_count    active_since    total_time      max_time        last_change  prevent_suspend_time
> > > 1-1.2.4.1       0               0               0               0               0               0               0               0   0
> > > 1-1.1           0               0               0               0               0               0               0               0   0
> > > gpio-keys       0               0               0               0               0               0               0               0   0
> > > spi10.0         0               0               0               0               0               0               0               0   0
> > > a88000.spi:ec@0:keyboard-controller     0               0               0               0               0               0           0
> > >                 0               0
> > > alarmtimer      0               0               0               0               0               0               0               0   0
> > > cros-ec-rtc.1.auto      0               0               0               0               0               0               0           0
> > >                 0
> > > a8f8800.usb     0               0               0               0               0               0               0               0   0
> > > a6f8800.usb     0               0               0               0               0               0               0               0   0
> > > localhost ~ # ls -l /sys/class/wakeup/
> > > total 0
> > > lrwxrwxrwx. 1 root root 0 Jul 31 17:43 alarmtimer -> ../../devices/platform/soc/ac0000.geniqup/a88000.spi/spi_master/spi10/spi10.0/cros-ec-dev.0.auto/cros-ec-rtc.1.auto/rtc/rtc0/alarmtimer
> >
> > So why is this not "(...)rtc0/wakeup/alarmtimer" ?
> >
> > This particular bit looks kind of inconsistent.
>
> I believe this is the code you're looking for in drivers/base/core.c
>
>                 /*
>                  * If we have no parent, we live in "virtual".
>                  * Class-devices with a non class-device as parent, live
>                  * in a "glue" directory to prevent namespace collisions.
>                  */
>                 if (parent == NULL)
>                         parent_kobj = virtual_device_parent(dev);
>                 else if (parent->class && !dev->class->ns_type)
>                         return &parent->kobj;
>                 else
>                         parent_kobj = &parent->kobj;
>

OK, so it looks like there really is a little benefit from making the
device associated with the wakeup source be the parent of its virtual
dev.

> >
> > I guess without your patch you'd see "(...)rtc0/wakeup/wakeup0" instead, right?
>
> No, it would be rtc0/wakeup0. That's because rtc is a class, and rtc0 is
> part of that class, so we don't try to make a glue directory named after
> the class to avoid collisions (see class_dir_create_and_add()
> implementation).

That's not really consistent.

> BTW, paths in /sys/devices aren't supposed to matter too much. In this
> case, I'd expect to see userspace looking at the /sys/class/wakeup path
> to follow the symlink to figure out what device triggered a wakeup. It
> can look at the 'device' symlink inside the directory for the wakeup
> device to figure out which one it is.

But if you go from the device, it would be good to be able to figure
out which wakeup sources are associated with it and in the alarmtimer
example you don't even see that it is a wakeup source without
following the link.

So the "wakeupN" virtual dev names for all wakeup source objects are
less confusing IMO.

It would be good to avoid the glue dir creation in all cases somehow too.

> Final thought, might want to suppress the power directory from being
> created for the wakeup class. It looks odd to have
> /sys/class/wakeup/wakeup0/power when the presumably does nothing.

I agree and there is a flag for that IIRC.