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]

Message-ID: <5d4305df.1c69fb81.c4013.1950@mx.google.com>
Date:   Thu, 01 Aug 2019 08:31:42 -0700
From:   Stephen Boyd <swboyd@...omium.org>
To:     "Rafael J. Wysocki" <rafael@...nel.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

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;


> 
> 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).

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.

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.