Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20190627000412.GA527@kroah.com> <20190627225335.72107-1-trong@android.com>
 <20190628151040.GA14074@kroah.com> <3451189.hAX5mKGt1u@kreacher>
In-Reply-To: <3451189.hAX5mKGt1u@kreacher>
From:   Tri Vo <trong@android.com>
Date:   Mon, 8 Jul 2019 12:33:55 +0900
Message-ID: <CANA+-vAxU5jp6PQ26NU+UMc6iyw6KkBS9nbd6wj0qqkO-1WhYg@mail.gmail.com>
Subject: Re: [PATCH v2] PM / wakeup: show wakeup sources stats in sysfs
To:     "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc:     Greg KH <gregkh@linuxfoundation.org>,
        Viresh Kumar <viresh.kumar@linaro.org>,
        "Rafael J. Wysocki" <rafael@kernel.org>,
        Hridya Valsaraju <hridya@google.com>,
        Sandeep Patil <sspatil@google.com>, kaleshsingh@google.com,
        LKML <linux-kernel@vger.kernel.org>,
        Linux PM <linux-pm@vger.kernel.org>,
        "Cc: Android Kernel" <kernel-team@android.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, Jul 4, 2019 at 7:31 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> On Friday, June 28, 2019 5:10:40 PM CEST Greg KH wrote:
> > On Thu, Jun 27, 2019 at 03:53:35PM -0700, Tri Vo wrote:
> > > Userspace can use wakeup_sources debugfs node to plot history of suspend
> > > blocking wakeup sources over device's boot cycle. This information can
> > > then be used (1) for power-specific bug reporting and (2) towards
> > > attributing battery consumption to specific processes over a period of
> > > time.
> > >
> > > However, debugfs doesn't have stable ABI. For this reason, expose wakeup
> > > sources statistics in sysfs under /sys/power/wakeup_sources/<name>/
> > >
> > > Embedding a struct kobject into struct wakeup_source changes lifetime
> > > requirements on the latter. To that end, change deallocation of struct
> > > wakeup_source using kfree to kobject_put().
> > >
> > > Change struct wakelock's wakeup_source member to a pointer to decouple
> > > lifetimes of struct wakelock and struct wakeup_source for above reason.
> > >
> > > Introduce CONFIG_PM_SLEEP_STATS that enables/disables showing wakeup
> > > source statistics in sysfs.
> > >
> > > Signed-off-by: Tri Vo <trong@android.com>
> >
> > Ok, this looks much better, but I don't like the use of a "raw" kobject
> > here.  It is much simpler, and less code, to use 'struct device'
> > instead.
> >
> > As proof, I reworked the patch to do just that, and it saves over 50
> > lines of .c code, which is always nice :)
>
> Thanks for taking the time to do that!

Thanks a lot, Greg!
>
> > Attached below is the reworked code, along with the updated
> > documentation file.  It creates devices in a virtual class, and you can
> > easily iterate over them all by looking in /sys/class/wakeup/.
>
> That actually is nice - no need to add anything under /sys/power/.
>
> > Note, I'm note quite sure you need all of the changes you made in
> > kernel/power/wakelock.c when you make the structure contain a pointer to
> > the wakeup source and not the structure itself, but I just went with it
> > and got it all to build properly.
>
> I'm not really sure about it either.
>
> > Also note, I've not actually tested this at all, only built it, so I
> > _strongly_ suggest that you test this to make sure it really works :)
> >
> > What do you think?
>
> I agree with the direction. :-)

I'll test things out and send v3 of the patch. Thanks!