Re: Regression with PM / wakeup: Show wakeup sources stats in sysfs"
On Sat, May 30, 2020 at 11:52 AM Florian Fainelli wrote:
>
>
>
> On 5/29/2020 4:14 PM, Tri Vo wrote:
> > On Fri, May 29, 2020 at 3:37 PM Florian Fainelli
> > wrote:
> >>
> >> On 5/29/20 3:28 PM, Tri Vo wrote:
> >>> On Fri, May 29, 2020 at 9:51 AM Rafael J. Wysocki
> >>> wrote:
> >>>>
> >>>> On 5/28/2020 10:46 PM, Florian Fainelli wrote:
> >>>>> Hi,
> >>>>>
> >>>>> Commit c8377adfa78103be5380200eb9dab764d7ca890e ("PM / wakeup: Show
> >>>>> wakeup sources stats in sysfs") is causing some of our tests to fail
> >>>>> because /sys/class/net/*/device/power/wakeup_count is now 0, despite
> >>>>> /sys/kernel/debug/wakeup_sources clearly indicating that the Ethernet
> >>>>> device was responsible for system wake-up.
> >>>>>
> >>>>> What's more in looking at /sys/class/wakekup/wakeup2/event_count, we
> >>>>> have the number of Wake-on-LAN wakeups recorded properly, but
> >>>>> wakeup_count is desperately 0, why is that?
> >>>>
> >>>> I need to look at that commit in detail to find out what is going on.
> >>>
> >>> It would be helpful to see the contents of
> >>> /sys/kernel/debug/wakeup_sources, /sys/class/net/*/device/power/*, and
> >>> /sys/class/wakekup/* corresponding to the device in question. The
> >>> values in these files are queried from the same struct wakeup_source.
> >>> So it's odd if wakeup_count diverges.
> >>
> >> Most certainly, below is the information you want, the two cat
> >> /s/k/d/wakeup_sources were done before Wake-on-LAN and after waking-up
> >> from LAN. /sys/class/wakeup/wakeup2 maps to the Ethernet device.
> >>
> >> The Ethernet device calls pm_wakeup_event() against the struct device
> >> that is embedded in the platform_device that it was probed with. I will
> >> try to debug this myself over the weekend, time permitting.
> >>
> >>
> >> # ethtool -s eth0 wol g
> >> # cat /sys/kernel/debug/wakeup_sources
> >> nameactive_countevent_count wakeup_count
> >> expire_countactive_sincetotal_time max_time
> >> last_changep
> >> revent_suspend_time
> >> 47d58.ethernet 0 0 0
> >> 0 0 0 0 0 0
> >> alarmtimer 0 0 0 0
> >> 0 0 0 0 0
> >> 47c408400.waketimer 2 2 0
> >> 0 0 0 0 6144
> >> 1 0
> >> # pml -w20
> >> [ 3449.937142] brcm-waketimer 47c408400.waketimer: Using sysfs
> >> attributes, consider using 'rtcwake'
> >> Pass 1 out of 1, mode=none, tp_al[ 3449.952654] PM: suspend entry (shallow)
> >> l=1, cycle_tp=, sleep=, [ 3449.959004] Filesystems sync: 0.000 seconds
> >> wakeup_time=20
> >> [ 3449.965984] Freezing user space processes ... (elapsed 0.001 seconds)
> >> done.
> >> [ 3449.974087] OOM killer disabled.
> >> [ 3449.977316] Freezing remaining freezable tasks ... (elapsed 0.006
> >> seconds) done.
> >> [ 3449.991114] printk: Suspending console(s) (use no_console_suspend to
> >> debug)
> >> AMS: System is entering S2...
> >> [ 3450.022381] bcmgenet 47d58.ethernet eth0: Link is Down
> >> [ 3450.048340] Disabling non-boot CPUs ...
> >> [ 3450.049344] CPU1: shutdown
> >> [ 3450.050393] psci: CPU1 killed (polled 1 ms)
> >> [ 3450.051332] Enabling non-boot CPUs ...
> >> [ 3450.051712] Detected PIPT I-cache on CPU1
> >> [ 3450.051812] CPU1: Booted secondary processor 0x01 [0x410fd083]
> >> [ 3450.052435] CPU1 is up
> >> [ 3450.683588] bcmgenet 47d58.ethernet eth0: Link is Up - 1Gbps/Full
> >> - flow control rx/tx
> >> [ 3450.729677] OOM killer enabled.
> >> [ 3450.732908] Restarting tasks ... done.
> >> [ 3450.738539] PM: suspend exit
> >> --
> >> [ 3450.744239] brcm-waketimer 47c408400.waketimer: Using sysfs
> >> attributes, consider using 'rtcwake'
> >> # cat /sys/kernel/debug/wakeup_sources
> >> nameactive_countevent_count wakeup_count
> >> expire_countactive_sincetotal_time max_time
Re: Regression with PM / wakeup: Show wakeup sources stats in sysfs"
On Fri, May 29, 2020 at 3:37 PM Florian Fainelli wrote:
>
> On 5/29/20 3:28 PM, Tri Vo wrote:
> > On Fri, May 29, 2020 at 9:51 AM Rafael J. Wysocki
> > wrote:
> >>
> >> On 5/28/2020 10:46 PM, Florian Fainelli wrote:
> >>> Hi,
> >>>
> >>> Commit c8377adfa78103be5380200eb9dab764d7ca890e ("PM / wakeup: Show
> >>> wakeup sources stats in sysfs") is causing some of our tests to fail
> >>> because /sys/class/net/*/device/power/wakeup_count is now 0, despite
> >>> /sys/kernel/debug/wakeup_sources clearly indicating that the Ethernet
> >>> device was responsible for system wake-up.
> >>>
> >>> What's more in looking at /sys/class/wakekup/wakeup2/event_count, we
> >>> have the number of Wake-on-LAN wakeups recorded properly, but
> >>> wakeup_count is desperately 0, why is that?
> >>
> >> I need to look at that commit in detail to find out what is going on.
> >
> > It would be helpful to see the contents of
> > /sys/kernel/debug/wakeup_sources, /sys/class/net/*/device/power/*, and
> > /sys/class/wakekup/* corresponding to the device in question. The
> > values in these files are queried from the same struct wakeup_source.
> > So it's odd if wakeup_count diverges.
>
> Most certainly, below is the information you want, the two cat
> /s/k/d/wakeup_sources were done before Wake-on-LAN and after waking-up
> from LAN. /sys/class/wakeup/wakeup2 maps to the Ethernet device.
>
> The Ethernet device calls pm_wakeup_event() against the struct device
> that is embedded in the platform_device that it was probed with. I will
> try to debug this myself over the weekend, time permitting.
>
>
> # ethtool -s eth0 wol g
> # cat /sys/kernel/debug/wakeup_sources
> nameactive_countevent_count wakeup_count
> expire_countactive_sincetotal_time max_timelast_changep
> revent_suspend_time
> 47d58.ethernet 0 0 0
> 0 0 0 0 0 0
> alarmtimer 0 0 0 0
> 0 0 0 0 0
> 47c408400.waketimer 2 2 0
> 0 0 0 0 6144
> 1 0
> # pml -w20
> [ 3449.937142] brcm-waketimer 47c408400.waketimer: Using sysfs
> attributes, consider using 'rtcwake'
> Pass 1 out of 1, mode=none, tp_al[ 3449.952654] PM: suspend entry (shallow)
> l=1, cycle_tp=, sleep=, [ 3449.959004] Filesystems sync: 0.000 seconds
> wakeup_time=20
> [ 3449.965984] Freezing user space processes ... (elapsed 0.001 seconds)
> done.
> [ 3449.974087] OOM killer disabled.
> [ 3449.977316] Freezing remaining freezable tasks ... (elapsed 0.006
> seconds) done.
> [ 3449.991114] printk: Suspending console(s) (use no_console_suspend to
> debug)
> AMS: System is entering S2...
> [ 3450.022381] bcmgenet 47d58.ethernet eth0: Link is Down
> [ 3450.048340] Disabling non-boot CPUs ...
> [ 3450.049344] CPU1: shutdown
> [ 3450.050393] psci: CPU1 killed (polled 1 ms)
> [ 3450.051332] Enabling non-boot CPUs ...
> [ 3450.051712] Detected PIPT I-cache on CPU1
> [ 3450.051812] CPU1: Booted secondary processor 0x01 [0x410fd083]
> [ 3450.052435] CPU1 is up
> [ 3450.683588] bcmgenet 47d58.ethernet eth0: Link is Up - 1Gbps/Full
> - flow control rx/tx
> [ 3450.729677] OOM killer enabled.
> [ 3450.732908] Restarting tasks ... done.
> [ 3450.738539] PM: suspend exit
> --
> [ 3450.744239] brcm-waketimer 47c408400.waketimer: Using sysfs
> attributes, consider using 'rtcwake'
> # cat /sys/kernel/debug/wakeup_sources
> nameactive_countevent_count wakeup_count
> expire_countactive_sincetotal_time max_timelast_changep
> revent_suspend_time
> 47d58.ethernet 1 1 0
> 0 0 0 0 3450
> 054 0
> alarmtimer 0 0 0 0
> 0 0 0 0 0
> 47c408400.waketimer 2 2 0
> 0 0 0 0 6144
> 1 0
> # cat /sys/class/net/*/device/power/*
> cat: read error: Input/output error
> auto
> 0
> unsupported
> 0
> enabled
> 0
> 0
> 1
> 0
> 0
> 3450054
> 0
> 0
UUIC, 47d58.ethernet is the device of interest here. It's
wakeup_count was 0 before wake up, and we expect it to be
Re: Regression with PM / wakeup: Show wakeup sources stats in sysfs"
On Fri, May 29, 2020 at 9:51 AM Rafael J. Wysocki
wrote:
>
> On 5/28/2020 10:46 PM, Florian Fainelli wrote:
> > Hi,
> >
> > Commit c8377adfa78103be5380200eb9dab764d7ca890e ("PM / wakeup: Show
> > wakeup sources stats in sysfs") is causing some of our tests to fail
> > because /sys/class/net/*/device/power/wakeup_count is now 0, despite
> > /sys/kernel/debug/wakeup_sources clearly indicating that the Ethernet
> > device was responsible for system wake-up.
> >
> > What's more in looking at /sys/class/wakekup/wakeup2/event_count, we
> > have the number of Wake-on-LAN wakeups recorded properly, but
> > wakeup_count is desperately 0, why is that?
>
> I need to look at that commit in detail to find out what is going on.
It would be helpful to see the contents of
/sys/kernel/debug/wakeup_sources, /sys/class/net/*/device/power/*, and
/sys/class/wakekup/* corresponding to the device in question. The
values in these files are queried from the same struct wakeup_source.
So it's odd if wakeup_count diverges.
Re: [PATCH v3 2/2] PM / wakeup: Unexport pm_wakeup_sysfs_{add,remove}()
On Mon, Aug 19, 2019 at 11:21 AM Stephen Boyd wrote:
>
> Sorry, subject should be
>
> PM / wakeup: Unexport wakeup_source_sysfs_{add,remove}()
>
> Quoting Stephen Boyd (2019-08-19 10:54:57)
> > These functions are just used by the PM core, and that isn't modular so
> > these functions don't need to be exported. Drop the exports.
> >
> > Fixes: 986845e747af ("PM / wakeup: Show wakeup sources stats in sysfs")
> > Cc: Tri Vo
> Messed up the address here too. Should be
>
> Cc: Tri Vo
>
> > Signed-off-by: Stephen Boyd
Reviewed-by: Tri Vo
Re: [PATCH v3 1/2] PM / wakeup: Register wakeup class kobj after device is added
On Mon, Aug 19, 2019 at 10:55 AM Stephen Boyd wrote:
>
> The device_set_wakeup_enable() function can be called on a device that
> hasn't been registered with device_add() yet. This allows the device to
> be in a state where wakeup is enabled for it but the device isn't
> published to userspace in sysfs yet.
>
> After commit 986845e747af ("PM / wakeup: Show wakeup sources stats in
> sysfs"), calling device_set_wakeup_enable() will fail for a device that
> hasn't been registered with the driver core via device_add(). This is
> because we try to create sysfs entries for the device and associate a
> wakeup class kobject with it before the device has been registered.
>
> Let's follow a similar approach that device_set_wakeup_capable() takes
> here and register the wakeup class either from
> device_set_wakeup_enable() when the device is already registered, or
> from dpm_sysfs_add() when the device is being registered with the driver
> core via device_add().
>
> Fixes: 986845e747af ("PM / wakeup: Show wakeup sources stats in sysfs")
> Reported-by: Qian Cai
> Cc: Qian Cai
> Cc: Tri Vo
> Signed-off-by: Stephen Boyd
Reviewed-by: Tri Vo
Re: [PATCH v2] PM / wakeup: Register wakeup class kobj after device is added
On Mon, Aug 19, 2019 at 8:06 AM Stephen Boyd wrote:
>
> The device_set_wakeup_enable() function can be called on a device that
> hasn't been registered with device_add() yet. This allows the device to
> be in a state where wakeup is enabled for it but the device isn't
> published to userspace in sysfs yet.
>
> After commit 986845e747af ("PM / wakeup: Show wakeup sources stats in
> sysfs"), calling device_set_wakeup_enable() will fail for a device that
> hasn't been registered with the driver core via device_add(). This is
> because we try to create sysfs entries for the device and associate a
> wakeup class kobject with it before the device has been registered.
> Let's follow a similar approach that device_set_wakeup_capable() takes
> here and register the wakeup class either from
> device_set_wakeup_enable() when the device is already registered, or
> from dpm_sysfs_add() when the device is being registered with the driver
> core via device_add().
>
> Fixes: 986845e747af ("PM / wakeup: Show wakeup sources stats in sysfs")
> Reported-by: Qian Cai
> Cc: Qian Cai
> Cc: Tri Vo
> Signed-off-by: Stephen Boyd
> ---
>
> Changes from v1:
> * Export wakeup_source_sysfs_add/remove stubs
> * New function to check if we should add the device from
>dpm_sysfs_add()
>
> drivers/base/power/power.h | 9 +
> drivers/base/power/sysfs.c | 10 +-
> drivers/base/power/wakeup.c | 10 ++
> include/linux/pm_wakeup.h | 10 ++
> 4 files changed, 34 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/base/power/power.h b/drivers/base/power/power.h
> index 57b1d1d88c8e..22a1533ec56b 100644
> --- a/drivers/base/power/power.h
> +++ b/drivers/base/power/power.h
> @@ -156,5 +156,14 @@ static inline void device_pm_init(struct device *dev)
> extern int wakeup_source_sysfs_add(struct device *parent,
>struct wakeup_source *ws);
> extern void wakeup_source_sysfs_remove(struct wakeup_source *ws);
> +#else /* !CONFIG_PM_SLEEP */
> +
> +static inline int wakeup_source_sysfs_add(struct device *parent,
> + struct wakeup_source *ws)
> +{
> + return 0;
> +}
> +
> +static inline void wakeup_source_sysfs_remove(struct wakeup_source *ws) {}
>
> #endif /* CONFIG_PM_SLEEP */
> diff --git a/drivers/base/power/sysfs.c b/drivers/base/power/sysfs.c
> index 1b9c281cbe41..1468d03ae9fb 100644
> --- a/drivers/base/power/sysfs.c
> +++ b/drivers/base/power/sysfs.c
> @@ -5,6 +5,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include "power.h"
> @@ -661,14 +662,21 @@ int dpm_sysfs_add(struct device *dev)
> if (rc)
> goto err_runtime;
> }
> + if (!device_has_wakeup_dev(dev)) {
This evaluates to true if dev->power.wakeup is NULL, which will result
in a null pointer dereference later in wakeup_source_sysfs_add().
I think the condition you want to check for is the one you pointed out
in previous patch.
if (dev->power.wakeup && !dev->power.wakeup->dev)
> + rc = wakeup_source_sysfs_add(dev, dev->power.wakeup);
> + if (rc)
> + goto err_wakeup;
> + }
> if (dev->power.set_latency_tolerance) {
> rc = sysfs_merge_group(>kobj,
>_qos_latency_tolerance_attr_group);
> if (rc)
> - goto err_wakeup;
> + goto err_wakeup_source;
> }
> return 0;
>
> + err_wakeup_source:
> + wakeup_source_sysfs_remove(dev->power.wakeup);
> err_wakeup:
> sysfs_unmerge_group(>kobj, _wakeup_attr_group);
> err_runtime:
> diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
> index f7925820b5ca..5817b51d2b15 100644
> --- a/drivers/base/power/wakeup.c
> +++ b/drivers/base/power/wakeup.c
> @@ -220,10 +220,12 @@ struct wakeup_source *wakeup_source_register(struct
> device *dev,
>
> ws = wakeup_source_create(name);
> if (ws) {
> - ret = wakeup_source_sysfs_add(dev, ws);
> - if (ret) {
> - wakeup_source_free(ws);
> - return NULL;
> + if (!dev || device_is_registered(dev)) {
> + ret = wakeup_source_sysfs_add(dev, ws);
> + if (ret) {
> + wakeup_source_free(ws);
> + return NULL;
> + }
> }
> wakeup_source_add
Re: [PATCH] PM / wakeup: Register wakeup class kobj after device is added
On Fri, Aug 16, 2019 at 2:46 PM Stephen Boyd wrote:
>
> Quoting Tri Vo (2019-08-16 14:27:35)
> > On Fri, Aug 16, 2019 at 7:56 AM Stephen Boyd wrote:
> > > diff --git a/drivers/base/power/sysfs.c b/drivers/base/power/sysfs.c
> > > index 1b9c281cbe41..27ee00f50bd7 100644
> > > --- a/drivers/base/power/sysfs.c
> > > +++ b/drivers/base/power/sysfs.c
> > > @@ -5,6 +5,7 @@
> > > #include
> > > #include
> > > #include
> > > +#include
> > > #include
> > > #include
> > > #include "power.h"
> > > @@ -661,14 +662,21 @@ int dpm_sysfs_add(struct device *dev)
> > > if (rc)
> > > goto err_runtime;
> > > }
> > > + if (dev->power.wakeup) {
> >
> > This conditional checks for the situation when wakeup source
> > registration have been previously attempted, but failed at
> > wakeup_source_sysfs_add(). My concern is that it's not easy to
> > understand what this check does without knowing exactly what
> > device_wakeup_enable() does to dev->power.wakeup before we reach this
> > point.
>
> Oh, actually this is wrong. It should be a check for
> dev->power.wakeup->dev being non-NULL. That's the variable that's set by
> wakeup_source_sysfs_add() upon success. So I should make it:
>
> if (dev->power.wakeup && !dev->power.wakeup->dev)
Oh ok, this makes more sense now :)
>
> And there's the problem that CONFIG_PM_SLEEP could be unset. Let me fix
> it up with a new inline function like device_has_wakeup_dev().
>
> >
> > > + rc = wakeup_source_sysfs_add(dev, dev->power.wakeup);
> > > + if (rc)
> > > + goto err_wakeup;
> > > + }
> > > if (dev->power.set_latency_tolerance) {
> > > rc = sysfs_merge_group(>kobj,
> > >
> > > _qos_latency_tolerance_attr_group);
> > > if (rc)
> > > - goto err_wakeup;
> > > + goto err_wakeup_source;
> > > }
> > > return 0;
> > >
> > > + err_wakeup_source:
> > > + wakeup_source_sysfs_remove(dev->power.wakeup);
> > > err_wakeup:
> > > sysfs_unmerge_group(>kobj, _wakeup_attr_group);
> > > err_runtime:
> > > diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
> > > index f7925820b5ca..5817b51d2b15 100644
> > > --- a/drivers/base/power/wakeup.c
> > > +++ b/drivers/base/power/wakeup.c
> > > @@ -220,10 +220,12 @@ struct wakeup_source *wakeup_source_register(struct
> > > device *dev,
> > >
> > > ws = wakeup_source_create(name);
> > > if (ws) {
> > > - ret = wakeup_source_sysfs_add(dev, ws);
> > > - if (ret) {
> > > - wakeup_source_free(ws);
> > > - return NULL;
> > > + if (!dev || device_is_registered(dev)) {
> >
> > Is there a possible race condition here? If dev->power.wakeup check in
> > dpm_sysfs_add() is done at the same time as device_is_registered(dev)
> > check here, then wakeup_source_sysfs_add() won't ever be called?
>
> The same race exists for device_set_wakeup_capable() so I didn't bother
> to try to avoid it. I suppose wakeup_source_sysfs_add() could run
> completely, allocate the device and set the name, etc., but not call
> device_add() and then we can set ws->dev and call device_add() under a
> mutex so that we keep a very small window where the wakeup class is
> published to sysfs. Or just throw a big mutex around the whole wakeup
> class creation path so that there isn't a chance of a race. But really,
> is anyone going to call device_set_wakeup_*() on a device that is also
> being added to the system? Seems unlikely.
True. I don't have a strong opinion.
>
> >
> > > + ret = wakeup_source_sysfs_add(dev, ws);
> > > + if (ret) {
> > > + wakeup_source_free(ws);
Re: [PATCH] PM / wakeup: Register wakeup class kobj after device is added
On Fri, Aug 16, 2019 at 7:56 AM Stephen Boyd wrote:
>
> The device_set_wakeup_enable() function can be called on a device that
> hasn't been registered with device_add() yet. This allows the device to
> be in a state where wakeup is enabled for it but the device isn't
> published to userspace in sysfs yet.
>
> After commit 986845e747af ("PM / wakeup: Show wakeup sources stats in
> sysfs"), calling device_set_wakeup_enable() will fail for a device that
> hasn't been registered with the driver core via device_add(). This is
> because we try to create sysfs entries for the device and associate a
> wakeup class kobject with it before the device has been registered.
> Let's follow a similar approach that device_set_wakeup_capable() takes
> here and register the wakeup class either from
> device_set_wakeup_enable() when the device is already registered, or
> from dpm_sysfs_add() when the device is being registered with the driver
> core via device_add().
>
> Fixes: 986845e747af ("PM / wakeup: Show wakeup sources stats in sysfs")
> Reported-by: Qian Cai
> Cc: Qian Cai
> Cc: Tri Vo
> Signed-off-by: Stephen Boyd
> ---
> drivers/base/power/sysfs.c | 10 +-
> drivers/base/power/wakeup.c | 10 ++
> 2 files changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/base/power/sysfs.c b/drivers/base/power/sysfs.c
> index 1b9c281cbe41..27ee00f50bd7 100644
> --- a/drivers/base/power/sysfs.c
> +++ b/drivers/base/power/sysfs.c
> @@ -5,6 +5,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include "power.h"
> @@ -661,14 +662,21 @@ int dpm_sysfs_add(struct device *dev)
> if (rc)
> goto err_runtime;
> }
> + if (dev->power.wakeup) {
This conditional checks for the situation when wakeup source
registration have been previously attempted, but failed at
wakeup_source_sysfs_add(). My concern is that it's not easy to
understand what this check does without knowing exactly what
device_wakeup_enable() does to dev->power.wakeup before we reach this
point.
> + rc = wakeup_source_sysfs_add(dev, dev->power.wakeup);
> + if (rc)
> + goto err_wakeup;
> + }
> if (dev->power.set_latency_tolerance) {
> rc = sysfs_merge_group(>kobj,
>_qos_latency_tolerance_attr_group);
> if (rc)
> - goto err_wakeup;
> + goto err_wakeup_source;
> }
> return 0;
>
> + err_wakeup_source:
> + wakeup_source_sysfs_remove(dev->power.wakeup);
> err_wakeup:
> sysfs_unmerge_group(>kobj, _wakeup_attr_group);
> err_runtime:
> diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
> index f7925820b5ca..5817b51d2b15 100644
> --- a/drivers/base/power/wakeup.c
> +++ b/drivers/base/power/wakeup.c
> @@ -220,10 +220,12 @@ struct wakeup_source *wakeup_source_register(struct
> device *dev,
>
> ws = wakeup_source_create(name);
> if (ws) {
> - ret = wakeup_source_sysfs_add(dev, ws);
> - if (ret) {
> - wakeup_source_free(ws);
> - return NULL;
> + if (!dev || device_is_registered(dev)) {
Is there a possible race condition here? If dev->power.wakeup check in
dpm_sysfs_add() is done at the same time as device_is_registered(dev)
check here, then wakeup_source_sysfs_add() won't ever be called?
> + ret = wakeup_source_sysfs_add(dev, ws);
> + if (ret) {
> + wakeup_source_free(ws);
> + return NULL;
> + }
> }
> wakeup_source_add(ws);
> }
> --
> Sent by a computer through tubes
>
Re: "PM / wakeup: Show wakeup sources stats in sysfs" causes boot warnings
On Wed, Aug 14, 2019 at 1:40 AM Tony Lindgren wrote: > > * Stephen Boyd [691231 23:00]: > > I also notice that device_set_wakeup_capable() has a check to see if the > > device is registered yet and it skips creating sysfs entries for the > > device if it isn't created in sysfs yet. Why? Just so it can be called > > before the device is created? I guess the same logic is handled by > > dpm_sysfs_add() if the device is registered after calling > > device_set_wakeup_*(). > > Hmm just guessing.. It's maybe because drivers can enable and disable > the wakeup capability at any point for example like driver/net drivers > do based on WOL etc? > > > There's two approaches I see: > > > > 1) Do a similar check for device_set_wakeup_enable() and skip > > adding the wakeup class until dpm_sysfs_add(). > > > > 2) Find each case where this happens and only call wakeup APIs > > on the device after the device is added. > > > > I guess it's better to let devices have wakeup modified on them before > > they're registered with the device core? > > I think we should at least initially handle case #1 above as multiple > places otherwise seem to break. Then maybe we could add a warning to > help fix all the #2 cases if needed? Makes sense. For case#1, we could also just register the wakeup source without specifying the parent device if the latter hasn't been registered yet. Userspace won't be able to associate a wakeup source to the parent device. But I think it's a reasonable fix, assuming we want to fix devices not being added before calling wakeup APIs #2.
Re: "PM / wakeup: Show wakeup sources stats in sysfs" causes boot warnings
On Tue, Aug 13, 2019 at 3:35 PM Stephen Boyd wrote: > > Quoting Qian Cai (2019-08-13 14:32:56) > > The linux-next commit "PM / wakeup: Show wakeup sources stats in sysfs" [1] > > introduced some baddies during boot on several x86 servers. Reverted the > > commit > > fixed the issue. > > > > [1] https://lore.kernel.org/lkml/20190807014846.143949-4-tr...@android.com/ > > > > [ 39.195053][T1] serio: i8042 KBD port at 0x60,0x64 irq 1 > > [ 39.197347][T1] kobject_add_internal failed for wakeup (error: -2 > > parent: > > serio0) > > [ 39.199845][T1] INFO: trying to register non-static key. > > [ 39.201582][T1] the code is fine but needs lockdep annotation. > > [ 39.203477][T1] turning off the locking correctness validator. > > [ 39.205399][T1] CPU: 12 PID: 1 Comm: swapper/0 Not tainted 5.3.0-rc4- > > next-20190813 #3 > > [ 39.207938][T1] Hardware name: HP ProLiant XL420 Gen9/ProLiant XL420 > > Gen9, BIOS U19 12/27/2015 > > [ 39.210606][T1] Call Trace: > > [ 39.210606][T1] dump_stack+0x62/0x9a > > [ 39.210606][T1] register_lock_class+0x95a/0x960 > > [ 39.210606][T1] ? __platform_driver_probe+0xcd/0x230 > > [ 39.210606][T1] ? __platform_create_bundle+0xc0/0xe0 > > [ 39.210606][T1] ? i8042_init+0x4ec/0x578 > > [ 39.210606][T1] ? do_one_initcall+0xfe/0x45a > > [ 39.219571][T1] ? kernel_init_freeable+0x614/0x6a7 > > [ 39.219571][T1] ? kernel_init+0x11/0x138 > > [ 39.219571][T1] ? ret_from_fork+0x35/0x40 > > [ 39.219571][T1] ? is_dynamic_key+0xf0/0xf0 > > [ 39.219571][T1] ? rwlock_bug.part.0+0x60/0x60 > > [ 39.219571][T1] ? __debug_check_no_obj_freed+0x8e/0x250 > > [ 39.219571][T1] __lock_acquire.isra.13+0x5f/0x830 > > [ 39.229491][T1] ? __debug_check_no_obj_freed+0x152/0x250 > > [ 39.229491][T1] lock_acquire+0x107/0x220 > > [ 39.229491][T1] ? __pm_relax.part.2+0x21/0xa0 > > [ 39.229491][T1] _raw_spin_lock_irqsave+0x35/0x50 > > [ 39.229491][T1] ? __pm_relax.part.2+0x21/0xa0 > > [ 39.229491][T1] __pm_relax.part.2+0x21/0xa0 > > [ 39.239588][T1] wakeup_source_destroy.part.3+0x18/0x190 > > [ 39.239588][T1] wakeup_source_register+0x43/0x50 > > We shouldn't call wakeup_source_destroy() from the error path in > wakeup_source_register() because that calls __pm_relax() and that takes > a lock that isn't initialized until wakeup_source_add() is called. Can > you try this patch? Right, that makes sense. Thanks for sending a fix, Stephen! What's the preferred procedure for merging this fix? Should we apply this commit on top of pm tree? Or should I resend a new version of the offending patch? Sorry, I'm still new to this.
[PATCH v8 1/3] PM / wakeup: Drop wakeup_source_init(), wakeup_source_prepare()
wakeup_source_init() has no users. Remove it.
As a result, wakeup_source_prepare() is only called from
wakeup_source_create(). Merge wakeup_source_prepare() into
wakeup_source_create() and remove it.
Change wakeup_source_create() behavior so that assigning NULL to wakeup
source's name throws an error.
Signed-off-by: Tri Vo
---
drivers/base/power/wakeup.c | 33 +
include/linux/pm_wakeup.h | 11 ---
2 files changed, 13 insertions(+), 31 deletions(-)
diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
index ee31d4f8d856..3938892c8903 100644
--- a/drivers/base/power/wakeup.c
+++ b/drivers/base/power/wakeup.c
@@ -72,23 +72,6 @@ static struct wakeup_source deleted_ws = {
.lock = __SPIN_LOCK_UNLOCKED(deleted_ws.lock),
};
-/**
- * wakeup_source_prepare - Prepare a new wakeup source for initialization.
- * @ws: Wakeup source to prepare.
- * @name: Pointer to the name of the new wakeup source.
- *
- * Callers must ensure that the @name string won't be freed when @ws is still
in
- * use.
- */
-void wakeup_source_prepare(struct wakeup_source *ws, const char *name)
-{
- if (ws) {
- memset(ws, 0, sizeof(*ws));
- ws->name = name;
- }
-}
-EXPORT_SYMBOL_GPL(wakeup_source_prepare);
-
/**
* wakeup_source_create - Create a struct wakeup_source object.
* @name: Name of the new wakeup source.
@@ -96,13 +79,23 @@ EXPORT_SYMBOL_GPL(wakeup_source_prepare);
struct wakeup_source *wakeup_source_create(const char *name)
{
struct wakeup_source *ws;
+ const char *ws_name;
- ws = kmalloc(sizeof(*ws), GFP_KERNEL);
+ ws = kzalloc(sizeof(*ws), GFP_KERNEL);
if (!ws)
- return NULL;
+ goto err_ws;
+
+ ws_name = kstrdup_const(name, GFP_KERNEL);
+ if (!ws_name)
+ goto err_name;
+ ws->name = ws_name;
- wakeup_source_prepare(ws, name ? kstrdup_const(name, GFP_KERNEL) :
NULL);
return ws;
+
+err_name:
+ kfree(ws);
+err_ws:
+ return NULL;
}
EXPORT_SYMBOL_GPL(wakeup_source_create);
diff --git a/include/linux/pm_wakeup.h b/include/linux/pm_wakeup.h
index 91027602d137..c0cad2d8f800 100644
--- a/include/linux/pm_wakeup.h
+++ b/include/linux/pm_wakeup.h
@@ -81,7 +81,6 @@ static inline void device_set_wakeup_path(struct device *dev)
}
/* drivers/base/power/wakeup.c */
-extern void wakeup_source_prepare(struct wakeup_source *ws, const char *name);
extern struct wakeup_source *wakeup_source_create(const char *name);
extern void wakeup_source_destroy(struct wakeup_source *ws);
extern void wakeup_source_add(struct wakeup_source *ws);
@@ -112,9 +111,6 @@ static inline bool device_can_wakeup(struct device *dev)
return dev->power.can_wakeup;
}
-static inline void wakeup_source_prepare(struct wakeup_source *ws,
-const char *name) {}
-
static inline struct wakeup_source *wakeup_source_create(const char *name)
{
return NULL;
@@ -181,13 +177,6 @@ static inline void pm_wakeup_dev_event(struct device *dev,
unsigned int msec,
#endif /* !CONFIG_PM_SLEEP */
-static inline void wakeup_source_init(struct wakeup_source *ws,
- const char *name)
-{
- wakeup_source_prepare(ws, name);
- wakeup_source_add(ws);
-}
-
static inline void __pm_wakeup_event(struct wakeup_source *ws, unsigned int
msec)
{
return pm_wakeup_ws_event(ws, msec, false);
--
2.22.0.770.g0f2c4a37fd-goog
[PATCH v8 3/3] PM / wakeup: Show wakeup sources stats in sysfs
Add an ID and a device pointer to 'struct wakeup_source'. Use them to to expose wakeup sources statistics in sysfs under /sys/class/wakeup/wakeup/*. Co-developed-by: Greg Kroah-Hartman Signed-off-by: Greg Kroah-Hartman Co-developed-by: Stephen Boyd Signed-off-by: Stephen Boyd Signed-off-by: Tri Vo Tested-by: Tri Vo Tested-by: Kalesh Singh --- Documentation/ABI/testing/sysfs-class-wakeup | 76 +++ drivers/acpi/device_pm.c | 3 +- drivers/base/power/Makefile | 2 +- drivers/base/power/power.h | 9 + drivers/base/power/wakeup.c | 28 ++- drivers/base/power/wakeup_stats.c| 203 +++ fs/eventpoll.c | 4 +- include/linux/pm_wakeup.h| 10 +- kernel/power/autosleep.c | 2 +- kernel/power/wakelock.c | 2 +- kernel/time/alarmtimer.c | 2 +- 11 files changed, 328 insertions(+), 13 deletions(-) create mode 100644 Documentation/ABI/testing/sysfs-class-wakeup create mode 100644 drivers/base/power/wakeup_stats.c diff --git a/Documentation/ABI/testing/sysfs-class-wakeup b/Documentation/ABI/testing/sysfs-class-wakeup new file mode 100644 index ..754aab8b6dcd --- /dev/null +++ b/Documentation/ABI/testing/sysfs-class-wakeup @@ -0,0 +1,76 @@ +What: /sys/class/wakeup/ +Date: June 2019 +Contact: Tri Vo +Description: + The /sys/class/wakeup/ directory contains pointers to all + wakeup sources in the kernel at that moment in time. + +What: /sys/class/wakeup/.../name +Date: June 2019 +Contact: Tri Vo +Description: + This file contains the name of the wakeup source. + +What: /sys/class/wakeup/.../active_count +Date: June 2019 +Contact: Tri Vo +Description: + This file contains the number of times the wakeup source was + activated. + +What: /sys/class/wakeup/.../event_count +Date: June 2019 +Contact: Tri Vo +Description: + This file contains the number of signaled wakeup events + associated with the wakeup source. + +What: /sys/class/wakeup/.../wakeup_count +Date: June 2019 +Contact: Tri Vo +Description: + This file contains the number of times the wakeup source might + abort suspend. + +What: /sys/class/wakeup/.../expire_count +Date: June 2019 +Contact: Tri Vo +Description: + This file contains the number of times the wakeup source's + timeout has expired. + +What: /sys/class/wakeup/.../active_time_ms +Date: June 2019 +Contact: Tri Vo +Description: + This file contains the amount of time the wakeup source has + been continuously active, in milliseconds. If the wakeup + source is not active, this file contains '0'. + +What: /sys/class/wakeup/.../total_time_ms +Date: June 2019 +Contact: Tri Vo +Description: + This file contains the total amount of time this wakeup source + has been active, in milliseconds. + +What: /sys/class/wakeup/.../max_time_ms +Date: June 2019 +Contact: Tri Vo +Description: + This file contains the maximum amount of time this wakeup + source has been continuously active, in milliseconds. + +What: /sys/class/wakeup/.../last_change_ms +Date: June 2019 +Contact: Tri Vo +Description: + This file contains the monotonic clock time when the wakeup + source was touched last time, in milliseconds. + +What: /sys/class/wakeup/.../prevent_suspend_time_ms +Date: June 2019 +Contact: Tri Vo +Description: + The file contains the total amount of time this wakeup source + has been preventing autosleep, in milliseconds. diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c index 28cffaaf9d82..91634e2dba8a 100644 --- a/drivers/acpi/device_pm.c +++ b/drivers/acpi/device_pm.c @@ -495,7 +495,8 @@ acpi_status acpi_add_pm_notifier(struct acpi_device *adev, struct device *dev, goto out; mutex_lock(_pm_notifier_lock); - adev->wakeup.ws = wakeup_source_register(dev_name(>dev)); + adev->wakeup.ws = wakeup_source_register(>dev, +dev_name(>dev)); adev->wakeup.context.dev = dev; adev->wakeup.context.func = func; adev->wakeup.flags.notifier_present = true; diff --git a/drivers/base/power/Makefile b/drivers/base/power/Makefile index e1bb691cf8f1..ec5bb190b9d0 100644 --- a/drivers/base/power/Makefile +++ b/drivers/base/power/Makefile @@ -1,6 +1,6 @@ # SPDX-License-Identifier:
[PATCH v8 2/3] PM / wakeup: Use wakeup_source_register() in wakelock.c
kernel/power/wakelock.c duplicates wakeup source creation and
registration code from drivers/base/power/wakeup.c.
Change struct wakelock's wakeup source to a pointer and use
wakeup_source_register() function to create and register said wakeup
source. Use wakeup_source_unregister() on cleanup path.
Signed-off-by: Tri Vo
Reviewed-by: Stephen Boyd
---
kernel/power/wakelock.c | 32 +++-
1 file changed, 19 insertions(+), 13 deletions(-)
diff --git a/kernel/power/wakelock.c b/kernel/power/wakelock.c
index 4210152e56f0..d1eb7fd98b64 100644
--- a/kernel/power/wakelock.c
+++ b/kernel/power/wakelock.c
@@ -27,7 +27,7 @@ static DEFINE_MUTEX(wakelocks_lock);
struct wakelock {
char*name;
struct rb_node node;
- struct wakeup_sourcews;
+ struct wakeup_source*ws;
#ifdef CONFIG_PM_WAKELOCKS_GC
struct list_headlru;
#endif
@@ -46,7 +46,7 @@ ssize_t pm_show_wakelocks(char *buf, bool show_active)
for (node = rb_first(_tree); node; node = rb_next(node)) {
wl = rb_entry(node, struct wakelock, node);
- if (wl->ws.active == show_active)
+ if (wl->ws->active == show_active)
str += scnprintf(str, end - str, "%s ", wl->name);
}
if (str > buf)
@@ -112,16 +112,16 @@ static void __wakelocks_gc(struct work_struct *work)
u64 idle_time_ns;
bool active;
- spin_lock_irq(>ws.lock);
- idle_time_ns = ktime_to_ns(ktime_sub(now, wl->ws.last_time));
- active = wl->ws.active;
- spin_unlock_irq(>ws.lock);
+ spin_lock_irq(>ws->lock);
+ idle_time_ns = ktime_to_ns(ktime_sub(now, wl->ws->last_time));
+ active = wl->ws->active;
+ spin_unlock_irq(>ws->lock);
if (idle_time_ns < ((u64)WL_GC_TIME_SEC * NSEC_PER_SEC))
break;
if (!active) {
- wakeup_source_remove(>ws);
+ wakeup_source_unregister(wl->ws);
rb_erase(>node, _tree);
list_del(>lru);
kfree(wl->name);
@@ -187,9 +187,15 @@ static struct wakelock *wakelock_lookup_add(const char
*name, size_t len,
kfree(wl);
return ERR_PTR(-ENOMEM);
}
- wl->ws.name = wl->name;
- wl->ws.last_time = ktime_get();
- wakeup_source_add(>ws);
+
+ wl->ws = wakeup_source_register(wl->name);
+ if (!wl->ws) {
+ kfree(wl->name);
+ kfree(wl);
+ return ERR_PTR(-ENOMEM);
+ }
+ wl->ws->last_time = ktime_get();
+
rb_link_node(>node, parent, node);
rb_insert_color(>node, _tree);
wakelocks_lru_add(wl);
@@ -233,9 +239,9 @@ int pm_wake_lock(const char *buf)
u64 timeout_ms = timeout_ns + NSEC_PER_MSEC - 1;
do_div(timeout_ms, NSEC_PER_MSEC);
- __pm_wakeup_event(>ws, timeout_ms);
+ __pm_wakeup_event(wl->ws, timeout_ms);
} else {
- __pm_stay_awake(>ws);
+ __pm_stay_awake(wl->ws);
}
wakelocks_lru_most_recent(wl);
@@ -271,7 +277,7 @@ int pm_wake_unlock(const char *buf)
ret = PTR_ERR(wl);
goto out;
}
- __pm_relax(>ws);
+ __pm_relax(wl->ws);
wakelocks_lru_most_recent(wl);
wakelocks_gc();
--
2.22.0.770.g0f2c4a37fd-goog
[PATCH v8 0/3] PM / wakeup: Show wakeup sources stats in sysfs
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, create a 'struct device' to expose wakeup sources statistics in sysfs under /sys/class/wakeup/wakeup/*. Patch 1 and 2 do some cleanup to simplify our changes to how wakeup sources are created. Patch 3 implements wakeup sources stats in sysfs. Tri Vo (3): PM / wakeup: Drop wakeup_source_init(), wakeup_source_prepare() PM / wakeup: Use wakeup_source_register() in wakelock.c PM / wakeup: Show wakeup sources stats in sysfs Documentation/ABI/testing/sysfs-class-wakeup | 76 +++ drivers/acpi/device_pm.c | 3 +- drivers/base/power/Makefile | 2 +- drivers/base/power/power.h | 9 + drivers/base/power/wakeup.c | 59 +++--- drivers/base/power/wakeup_stats.c| 203 +++ fs/eventpoll.c | 4 +- include/linux/pm_wakeup.h| 21 +- kernel/power/autosleep.c | 2 +- kernel/power/wakelock.c | 32 +-- kernel/time/alarmtimer.c | 2 +- 11 files changed, 358 insertions(+), 55 deletions(-) create mode 100644 Documentation/ABI/testing/sysfs-class-wakeup create mode 100644 drivers/base/power/wakeup_stats.c v2: - Updated Documentation/ABI/, as per Greg. - Removed locks in attribute functions, as per Greg. - Lifetimes of struct wakelock and struck wakeup_source are now different due to the latter embedding a refcounted kobject. Changed it so that struct wakelock only has a pointer to struct wakeup_source, instead of embedding it. - Added CONFIG_PM_SLEEP_STATS that enables/disables wakeup source statistics in sysfs. v3: Changes by Greg: - Reworked code to use 'struct device' instead of raw kobjects. - Updated documentation file. - Only link wakeup_stats.o when CONFIG_PM_SLEEP_STATS is enabled. Changes by Tri: - Reverted changes to kernel/power/wakelock.c. 'struct device' hides kobject operations. So no need to handle lifetimes in wakelock.c v4: - Added 'Co-developed-by:' and 'Tested-by:' fields to commit message. - Moved new documentation to a separate file Documentation/ABI/testing/sysfs-class-wakeup, as per Greg. - Fixed copyright header in drivers/base/power/wakeup_stats.c, as per Greg. v5: - Removed CONFIG_PM_SLEEP_STATS - Used PTR_ERR_OR_ZERO instead of if(IS_ERR(...)) + PTR_ERR, reported by kbuild test robot - Stephen reported that a call to device_init_wakeup() and writing 'enabled' to that device's power/wakeup file results in multiple wakeup source being allocated for that device. Changed device_wakeup_enable() to check if device wakeup was previously enabled. Changes by Stephen: - Changed stats location from /sys/class/wakeup//* to /sys/class/wakeup/wakeup/*, where ID is an IDA-allocated integer. This avoids name collisions in /sys/class/wakeup/ directory. - Added a "name" attribute to wakeup sources, and updated documentation. - Device registering the wakeup source is now the parent of the wakeup source. Updated wakeup_source_register()'s signature and its callers accordingly. v6: - Changed stats location to /sys/class/wakeup/ws/* - Replaced ida_simple_get()/ida_simple_remove() with ida_alloc()/ida_free() as the former is deprecated. - Reverted changes to device_init_wakeup(). Rafael is preparing a patch to deal with extra wakeup source allocation in a separate patch. v7: - Removed wakeup_source_init(), wakeup_source_prepare(). - Removed duplicate wakeup source creation code from kernel/power/wakelock. - Moved ID allocation to wakeup source object creation time. - Changed stats location back to /sys/class/wakeup/wakeup/* - Remove wakeup source device's "power" attributes. v8: - Updated commit message on patch 1 to indicate change of behavior of wakeup_source_create(), as per Stephen. - Included headers for used symbols, as per Stephen. - Added a function to create wakeup source devices to use device_set_pm_not_required() to skip power management for such devices, as per Stephen. -- 2.22.0.770.g0f2c4a37fd-goog
Re: [PATCH v7 3/3] PM / wakeup: Show wakeup sources stats in sysfs
On Mon, Aug 5, 2019 at 4:29 PM Stephen Boyd wrote:
>
> Quoting Tri Vo (2019-08-05 10:58:48)
> > diff --git a/drivers/base/power/wakeup_stats.c
> > b/drivers/base/power/wakeup_stats.c
> > new file mode 100644
> > index ..3a4f55028e27
> > --- /dev/null
> > +++ b/drivers/base/power/wakeup_stats.c
> > @@ -0,0 +1,161 @@
> [...]
> > +/**
> > + * wakeup_source_sysfs_add - Add wakeup_source attributes to sysfs.
> > + * @parent: Device given wakeup source is associated with (or NULL if
> > virtual).
> > + * @ws: Wakeup source to be added in sysfs.
> > + */
> > +int wakeup_source_sysfs_add(struct device *parent, struct wakeup_source
> > *ws)
> > +{
> > + struct device *dev;
> > +
> > + dev = device_create_with_groups(wakeup_class, parent, MKDEV(0, 0),
> > ws,
> > + wakeup_source_groups, "wakeup%d",
> > + ws->id);
> > + if (IS_ERR(dev))
> > + return PTR_ERR(dev);
> > + ws->dev = dev;
> > + pm_runtime_no_callbacks(ws->dev);
>
> Does this only avoid adding runtime PM attributes?
>
> I thought we would call device_set_pm_not_required() on the device here.
> Probably requiring a bit of copy/paste from device_create_with_groups()
> so that it can be set before the device is registered. Or another
> version of device_create_with_groups() that does everything besides call
> device_add().
Comments on pm_runtime_no_callbacks() say,
"Set the power.no_callbacks flag, which tells the PM core that this
device is power-managed through its parent and has no runtime PM
callbacks of its own. The runtime sysfs attributes will be removed."
Sound like it's appropriate to apply this function to the wakeup source.
Re: [PATCH v7 1/3] PM / wakeup: Drop wakeup_source_init(), wakeup_source_prepare()
On Mon, Aug 5, 2019 at 1:54 PM Stephen Boyd wrote:
>
> Quoting Tri Vo (2019-08-05 10:58:46)
> > diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
> > index ee31d4f8d856..3938892c8903 100644
> > --- a/drivers/base/power/wakeup.c
> > +++ b/drivers/base/power/wakeup.c
> > @@ -72,23 +72,6 @@ static struct wakeup_source deleted_ws = {
> > .lock = __SPIN_LOCK_UNLOCKED(deleted_ws.lock),
> > };
> >
> > -/**
> > - * wakeup_source_prepare - Prepare a new wakeup source for initialization.
> > - * @ws: Wakeup source to prepare.
> > - * @name: Pointer to the name of the new wakeup source.
> > - *
> > - * Callers must ensure that the @name string won't be freed when @ws is
> > still in
> > - * use.
> > - */
> > -void wakeup_source_prepare(struct wakeup_source *ws, const char *name)
> > -{
> > - if (ws) {
> > - memset(ws, 0, sizeof(*ws));
> > - ws->name = name;
> > - }
> > -}
> > -EXPORT_SYMBOL_GPL(wakeup_source_prepare);
> > -
> > /**
> > * wakeup_source_create - Create a struct wakeup_source object.
> > * @name: Name of the new wakeup source.
> > @@ -96,13 +79,23 @@ EXPORT_SYMBOL_GPL(wakeup_source_prepare);
> > struct wakeup_source *wakeup_source_create(const char *name)
> > {
> > struct wakeup_source *ws;
> > + const char *ws_name;
> >
> > - ws = kmalloc(sizeof(*ws), GFP_KERNEL);
> > + ws = kzalloc(sizeof(*ws), GFP_KERNEL);
> > if (!ws)
> > - return NULL;
> > + goto err_ws;
> > +
> > + ws_name = kstrdup_const(name, GFP_KERNEL);
> > + if (!ws_name)
>
> Does this intentionally change this function to return an error if
> 'name' is NULL? Before, wakeup_source_prepare() would just assign
> ws->name to NULL, but now it errors out. I don't see how it's good or
> useful to allow NULL for the wakeup source name, but it is what it is.
Yes, the change to not allow ws->name to be NULL is intentional.
Re: [PATCH v2] PM/sleep: Expose suspend stats in sysfs
On Thu, Aug 1, 2019 at 9:34 AM Kalesh Singh wrote: > > On Wed, Jul 31, 2019 at 11:19 PM Greg KH wrote: > > > > On Wed, Jul 31, 2019 at 02:29:33PM -0700, Kalesh Singh wrote: > > > Userspace can get suspend stats from the suspend stats debugfs node. > > > Since debugfs doesn't have stable ABI, expose suspend stats in > > > sysfs under /sys/power/suspend_stats. > > > > > > Signed-off-by: Kalesh Singh > > > --- > > > Changes in v2: > > > - Added separate show functions for last_failed_* stats, as per Greg > > > - Updated ABI Documentation > > > > This is nice, I didn't even know some of these were in the debugfs > > entries, so this should be more helpful to people. > > > > Reviewed-by: Greg Kroah-Hartman Reviewed-by: Tri Vo
[PATCH v7 1/3] PM / wakeup: Drop wakeup_source_init(), wakeup_source_prepare()
wakeup_source_init() has no users. Remove it.
As a result, wakeup_source_prepare() is only called from
wakeup_source_create(). Merge wakeup_source_prepare() into
wakeup_source_create() and remove it.
Signed-off-by: Tri Vo
---
drivers/base/power/wakeup.c | 33 +
include/linux/pm_wakeup.h | 11 ---
2 files changed, 13 insertions(+), 31 deletions(-)
diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
index ee31d4f8d856..3938892c8903 100644
--- a/drivers/base/power/wakeup.c
+++ b/drivers/base/power/wakeup.c
@@ -72,23 +72,6 @@ static struct wakeup_source deleted_ws = {
.lock = __SPIN_LOCK_UNLOCKED(deleted_ws.lock),
};
-/**
- * wakeup_source_prepare - Prepare a new wakeup source for initialization.
- * @ws: Wakeup source to prepare.
- * @name: Pointer to the name of the new wakeup source.
- *
- * Callers must ensure that the @name string won't be freed when @ws is still
in
- * use.
- */
-void wakeup_source_prepare(struct wakeup_source *ws, const char *name)
-{
- if (ws) {
- memset(ws, 0, sizeof(*ws));
- ws->name = name;
- }
-}
-EXPORT_SYMBOL_GPL(wakeup_source_prepare);
-
/**
* wakeup_source_create - Create a struct wakeup_source object.
* @name: Name of the new wakeup source.
@@ -96,13 +79,23 @@ EXPORT_SYMBOL_GPL(wakeup_source_prepare);
struct wakeup_source *wakeup_source_create(const char *name)
{
struct wakeup_source *ws;
+ const char *ws_name;
- ws = kmalloc(sizeof(*ws), GFP_KERNEL);
+ ws = kzalloc(sizeof(*ws), GFP_KERNEL);
if (!ws)
- return NULL;
+ goto err_ws;
+
+ ws_name = kstrdup_const(name, GFP_KERNEL);
+ if (!ws_name)
+ goto err_name;
+ ws->name = ws_name;
- wakeup_source_prepare(ws, name ? kstrdup_const(name, GFP_KERNEL) :
NULL);
return ws;
+
+err_name:
+ kfree(ws);
+err_ws:
+ return NULL;
}
EXPORT_SYMBOL_GPL(wakeup_source_create);
diff --git a/include/linux/pm_wakeup.h b/include/linux/pm_wakeup.h
index 91027602d137..c0cad2d8f800 100644
--- a/include/linux/pm_wakeup.h
+++ b/include/linux/pm_wakeup.h
@@ -81,7 +81,6 @@ static inline void device_set_wakeup_path(struct device *dev)
}
/* drivers/base/power/wakeup.c */
-extern void wakeup_source_prepare(struct wakeup_source *ws, const char *name);
extern struct wakeup_source *wakeup_source_create(const char *name);
extern void wakeup_source_destroy(struct wakeup_source *ws);
extern void wakeup_source_add(struct wakeup_source *ws);
@@ -112,9 +111,6 @@ static inline bool device_can_wakeup(struct device *dev)
return dev->power.can_wakeup;
}
-static inline void wakeup_source_prepare(struct wakeup_source *ws,
-const char *name) {}
-
static inline struct wakeup_source *wakeup_source_create(const char *name)
{
return NULL;
@@ -181,13 +177,6 @@ static inline void pm_wakeup_dev_event(struct device *dev,
unsigned int msec,
#endif /* !CONFIG_PM_SLEEP */
-static inline void wakeup_source_init(struct wakeup_source *ws,
- const char *name)
-{
- wakeup_source_prepare(ws, name);
- wakeup_source_add(ws);
-}
-
static inline void __pm_wakeup_event(struct wakeup_source *ws, unsigned int
msec)
{
return pm_wakeup_ws_event(ws, msec, false);
--
2.22.0.770.g0f2c4a37fd-goog
[PATCH v7 2/3] PM / wakeup: Use wakeup_source_register() in wakelock.c
kernel/power/wakelock.c duplicates wakeup source creation and
registration code from drivers/base/power/wakeup.c.
Change struct wakelock's wakeup source to a pointer and use
wakeup_source_register() function to create and register said wakeup
source. Use wakeup_source_unregister() on cleanup path.
Signed-off-by: Tri Vo
---
kernel/power/wakelock.c | 32 +++-
1 file changed, 19 insertions(+), 13 deletions(-)
diff --git a/kernel/power/wakelock.c b/kernel/power/wakelock.c
index 4210152e56f0..d1eb7fd98b64 100644
--- a/kernel/power/wakelock.c
+++ b/kernel/power/wakelock.c
@@ -27,7 +27,7 @@ static DEFINE_MUTEX(wakelocks_lock);
struct wakelock {
char*name;
struct rb_node node;
- struct wakeup_sourcews;
+ struct wakeup_source*ws;
#ifdef CONFIG_PM_WAKELOCKS_GC
struct list_headlru;
#endif
@@ -46,7 +46,7 @@ ssize_t pm_show_wakelocks(char *buf, bool show_active)
for (node = rb_first(_tree); node; node = rb_next(node)) {
wl = rb_entry(node, struct wakelock, node);
- if (wl->ws.active == show_active)
+ if (wl->ws->active == show_active)
str += scnprintf(str, end - str, "%s ", wl->name);
}
if (str > buf)
@@ -112,16 +112,16 @@ static void __wakelocks_gc(struct work_struct *work)
u64 idle_time_ns;
bool active;
- spin_lock_irq(>ws.lock);
- idle_time_ns = ktime_to_ns(ktime_sub(now, wl->ws.last_time));
- active = wl->ws.active;
- spin_unlock_irq(>ws.lock);
+ spin_lock_irq(>ws->lock);
+ idle_time_ns = ktime_to_ns(ktime_sub(now, wl->ws->last_time));
+ active = wl->ws->active;
+ spin_unlock_irq(>ws->lock);
if (idle_time_ns < ((u64)WL_GC_TIME_SEC * NSEC_PER_SEC))
break;
if (!active) {
- wakeup_source_remove(>ws);
+ wakeup_source_unregister(wl->ws);
rb_erase(>node, _tree);
list_del(>lru);
kfree(wl->name);
@@ -187,9 +187,15 @@ static struct wakelock *wakelock_lookup_add(const char
*name, size_t len,
kfree(wl);
return ERR_PTR(-ENOMEM);
}
- wl->ws.name = wl->name;
- wl->ws.last_time = ktime_get();
- wakeup_source_add(>ws);
+
+ wl->ws = wakeup_source_register(wl->name);
+ if (!wl->ws) {
+ kfree(wl->name);
+ kfree(wl);
+ return ERR_PTR(-ENOMEM);
+ }
+ wl->ws->last_time = ktime_get();
+
rb_link_node(>node, parent, node);
rb_insert_color(>node, _tree);
wakelocks_lru_add(wl);
@@ -233,9 +239,9 @@ int pm_wake_lock(const char *buf)
u64 timeout_ms = timeout_ns + NSEC_PER_MSEC - 1;
do_div(timeout_ms, NSEC_PER_MSEC);
- __pm_wakeup_event(>ws, timeout_ms);
+ __pm_wakeup_event(wl->ws, timeout_ms);
} else {
- __pm_stay_awake(>ws);
+ __pm_stay_awake(wl->ws);
}
wakelocks_lru_most_recent(wl);
@@ -271,7 +277,7 @@ int pm_wake_unlock(const char *buf)
ret = PTR_ERR(wl);
goto out;
}
- __pm_relax(>ws);
+ __pm_relax(wl->ws);
wakelocks_lru_most_recent(wl);
wakelocks_gc();
--
2.22.0.770.g0f2c4a37fd-goog
[PATCH v7 3/3] PM / wakeup: Show wakeup sources stats in sysfs
Add an ID and a device pointer to 'struct wakeup_source'. Use them to to expose wakeup sources statistics in sysfs under /sys/class/wakeup/wakeup/*. Co-developed-by: Greg Kroah-Hartman Signed-off-by: Greg Kroah-Hartman Co-developed-by: Stephen Boyd Signed-off-by: Stephen Boyd Signed-off-by: Tri Vo Tested-by: Tri Vo Tested-by: Kalesh Singh --- Documentation/ABI/testing/sysfs-class-wakeup | 76 + drivers/acpi/device_pm.c | 3 +- drivers/base/power/Makefile | 2 +- drivers/base/power/power.h | 9 ++ drivers/base/power/wakeup.c | 28 +++- drivers/base/power/wakeup_stats.c| 161 +++ fs/eventpoll.c | 4 +- include/linux/pm_wakeup.h| 10 +- kernel/power/autosleep.c | 2 +- kernel/power/wakelock.c | 2 +- kernel/time/alarmtimer.c | 2 +- 11 files changed, 286 insertions(+), 13 deletions(-) create mode 100644 Documentation/ABI/testing/sysfs-class-wakeup create mode 100644 drivers/base/power/wakeup_stats.c diff --git a/Documentation/ABI/testing/sysfs-class-wakeup b/Documentation/ABI/testing/sysfs-class-wakeup new file mode 100644 index ..754aab8b6dcd --- /dev/null +++ b/Documentation/ABI/testing/sysfs-class-wakeup @@ -0,0 +1,76 @@ +What: /sys/class/wakeup/ +Date: June 2019 +Contact: Tri Vo +Description: + The /sys/class/wakeup/ directory contains pointers to all + wakeup sources in the kernel at that moment in time. + +What: /sys/class/wakeup/.../name +Date: June 2019 +Contact: Tri Vo +Description: + This file contains the name of the wakeup source. + +What: /sys/class/wakeup/.../active_count +Date: June 2019 +Contact: Tri Vo +Description: + This file contains the number of times the wakeup source was + activated. + +What: /sys/class/wakeup/.../event_count +Date: June 2019 +Contact: Tri Vo +Description: + This file contains the number of signaled wakeup events + associated with the wakeup source. + +What: /sys/class/wakeup/.../wakeup_count +Date: June 2019 +Contact: Tri Vo +Description: + This file contains the number of times the wakeup source might + abort suspend. + +What: /sys/class/wakeup/.../expire_count +Date: June 2019 +Contact: Tri Vo +Description: + This file contains the number of times the wakeup source's + timeout has expired. + +What: /sys/class/wakeup/.../active_time_ms +Date: June 2019 +Contact: Tri Vo +Description: + This file contains the amount of time the wakeup source has + been continuously active, in milliseconds. If the wakeup + source is not active, this file contains '0'. + +What: /sys/class/wakeup/.../total_time_ms +Date: June 2019 +Contact: Tri Vo +Description: + This file contains the total amount of time this wakeup source + has been active, in milliseconds. + +What: /sys/class/wakeup/.../max_time_ms +Date: June 2019 +Contact: Tri Vo +Description: + This file contains the maximum amount of time this wakeup + source has been continuously active, in milliseconds. + +What: /sys/class/wakeup/.../last_change_ms +Date: June 2019 +Contact: Tri Vo +Description: + This file contains the monotonic clock time when the wakeup + source was touched last time, in milliseconds. + +What: /sys/class/wakeup/.../prevent_suspend_time_ms +Date: June 2019 +Contact: Tri Vo +Description: + The file contains the total amount of time this wakeup source + has been preventing autosleep, in milliseconds. diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c index 28cffaaf9d82..91634e2dba8a 100644 --- a/drivers/acpi/device_pm.c +++ b/drivers/acpi/device_pm.c @@ -495,7 +495,8 @@ acpi_status acpi_add_pm_notifier(struct acpi_device *adev, struct device *dev, goto out; mutex_lock(_pm_notifier_lock); - adev->wakeup.ws = wakeup_source_register(dev_name(>dev)); + adev->wakeup.ws = wakeup_source_register(>dev, +dev_name(>dev)); adev->wakeup.context.dev = dev; adev->wakeup.context.func = func; adev->wakeup.flags.notifier_present = true; diff --git a/drivers/base/power/Makefile b/drivers/base/power/Makefile index e1bb691cf8f1..ec5bb190b9d0 100644 --- a/drivers/base/power/Makefile +++ b/drivers/base/power/Makefile @@ -1,6 +1,6 @@ # SPDX-License-
[PATCH v7 0/3] PM / wakeup: show wakeup sources stats in sysfs
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, create a 'struct device' to expose wakeup sources statistics in sysfs under /sys/class/wakeup/wakeup/*. Patch 1 and 2 do some cleanup to simplify our changes to how wakeup sources are created. Patch 3 implements wakeup sources stats in sysfs. Tri Vo (3): PM / wakeup: Drop wakeup_source_init(), wakeup_source_prepare() PM / wakeup: Use wakeup_source_register() in wakelock.c PM / wakeup: Show wakeup sources stats in sysfs Documentation/ABI/testing/sysfs-class-wakeup | 76 + drivers/acpi/device_pm.c | 3 +- drivers/base/power/Makefile | 2 +- drivers/base/power/power.h | 9 ++ drivers/base/power/wakeup.c | 59 --- drivers/base/power/wakeup_stats.c| 161 +++ fs/eventpoll.c | 4 +- include/linux/pm_wakeup.h| 21 +-- kernel/power/autosleep.c | 2 +- kernel/power/wakelock.c | 32 ++-- kernel/time/alarmtimer.c | 2 +- 11 files changed, 316 insertions(+), 55 deletions(-) create mode 100644 Documentation/ABI/testing/sysfs-class-wakeup create mode 100644 drivers/base/power/wakeup_stats.c v2: - Updated Documentation/ABI/, as per Greg. - Removed locks in attribute functions, as per Greg. - Lifetimes of struct wakelock and struck wakeup_source are now different due to the latter embedding a refcounted kobject. Changed it so that struct wakelock only has a pointer to struct wakeup_source, instead of embedding it. - Added CONFIG_PM_SLEEP_STATS that enables/disables wakeup source statistics in sysfs. v3: Changes by Greg: - Reworked code to use 'struct device' instead of raw kobjects. - Updated documentation file. - Only link wakeup_stats.o when CONFIG_PM_SLEEP_STATS is enabled. Changes by Tri: - Reverted changes to kernel/power/wakelock.c. 'struct device' hides kobject operations. So no need to handle lifetimes in wakelock.c v4: - Added 'Co-developed-by:' and 'Tested-by:' fields to commit message. - Moved new documentation to a separate file Documentation/ABI/testing/sysfs-class-wakeup, as per Greg. - Fixed copyright header in drivers/base/power/wakeup_stats.c, as per Greg. v5: - Removed CONFIG_PM_SLEEP_STATS - Used PTR_ERR_OR_ZERO instead of if(IS_ERR(...)) + PTR_ERR, reported by kbuild test robot - Stephen reported that a call to device_init_wakeup() and writing 'enabled' to that device's power/wakeup file results in multiple wakeup source being allocated for that device. Changed device_wakeup_enable() to check if device wakeup was previously enabled. Changes by Stephen: - Changed stats location from /sys/class/wakeup//* to /sys/class/wakeup/wakeup/*, where ID is an IDA-allocated integer. This avoids name collisions in /sys/class/wakeup/ directory. - Added a "name" attribute to wakeup sources, and updated documentation. - Device registering the wakeup source is now the parent of the wakeup source. Updated wakeup_source_register()'s signature and its callers accordingly. v6: - Changed stats location to /sys/class/wakeup/ws/* - Replaced ida_simple_get()/ida_simple_remove() with ida_alloc()/ida_free() as the former is deprecated. - Reverted changes to device_init_wakeup(). Rafael is preparing a patch to deal with extra wakeup source allocation in a separate patch. v7: - Removed wakeup_source_init(), wakeup_source_prepare(). - Removed duplicate wakeup source creation code from kernel/power/wakelock. - Moved ID allocation to wakeup source object creation time. - Changed stats location back to /sys/class/wakeup/wakeup/* - Remove wakeup source device's "power" attributes. -- 2.22.0.770.g0f2c4a37fd-goog
Re: [PATCH v6] PM / wakeup: show wakeup sources stats in sysfs
On Thu, Aug 1, 2019 at 3:10 PM Rafael J. Wysocki wrote: > > On Thu, Aug 1, 2019 at 11:45 PM Tri Vo wrote: > > > > On Thu, Aug 1, 2019 at 1:23 PM Stephen Boyd wrote: > > > > > > Quoting Tri Vo (2019-08-01 12:50:25) > > > > On Wed, Jul 31, 2019 at 4:45 PM Stephen Boyd > > > > wrote: > > > > > > > > > > Quoting Rafael J. Wysocki (2019-07-31 16:10:38) > > > > > > On Thu, Aug 1, 2019 at 12:59 AM Tri Vo 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). Yes, allocating the ID when creating the wakeup source object makes sense. However, kernel/power/wakelock.c allocates its wakeup sources manually. I imagine we don't want these IDs to be created in more than one place. Making wakelock.c only use wakeup_source_*() family of functions when dealing with wakeup sources might be a worthwhile change though. Then we won't have to worry about ID allocation in wakelock.c. WDYT? Also, it sounds like we all agree with "/sys/class/wakeup/wsN/" path and "/sys/class/wakeup/wsN/name" attribute for each wakeup source, right?
Re: [PATCH v6] PM / wakeup: show wakeup sources stats in sysfs
On Thu, Aug 1, 2019 at 3:11 PM Stephen Boyd wrote: > > Quoting Tri Vo (2019-08-01 14:44:52) > > On Thu, Aug 1, 2019 at 1:23 PM Stephen Boyd wrote: > > > > > > > > > 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. If the device name is going to be something generic like 'wakeupN', > then we need to make sure that the wakeup source name is unique. If we could easily make sure that wakeup source names are unique, then we wouldn't need to generate "wakeupN" ids :) > Otherwise, I'm not able to see how userspace will differentiate between > two of the same named wakelocks. Before this patch the wakeup source > name looks to have been used for debugging, but now it's being used > programmatically to let userspace act upon it somehow. Maybe it's for > debug still, but I could see how userspace may want to hunt down the > wakelock that's created in userspace and penalize or kill the task > that's waking up the device. Two wakelocks can't have the same name. So they are still distinguishable from userspace. However, there is still no way to figure out from userspace which process created which wake lock. That's a weakness of /sys/power/wake_lock API, independent of this patch. > > I see that wakelock_lookup_add() already checks the list of wakelock > wakeup sources, but I don't see how I can't create an "alarmtimer" > wakelock again, but this time for userspace, by writing into > /sys/power/wake_lock. Behind the scenes, writing "alarmtimer" to /sys/power/wake_lock creates a wakeup source named "alarmtimer", which in turn creates a directory /sys/class/wakeup/alarmtimer (in you patch), which is likely already created by alarmtimer. This leads to an error. The error is resolved if wakeup source's sysfs entry is /sys/class/wakeup/wakeupN instead. > > What happens with namespaces here BTW? Can a wakelock be made in one > namespace and that is the same name as another wakelock in a different > namespace? Right now it doesn't look possible because of the global name > matching, but it probably makes sense to support this? Maybe we just > shouldn't make anything in sysfs for wake sources that can be any random > name created from the wakelock path right now. I don't see how it can be > traced back to the process that created it in any reasonable way. It should be OK if we don't use the arbitrary wakelock name in the path, but instead use the generated id "wakeupN".
Re: [PATCH v6] PM / wakeup: show wakeup sources stats in sysfs
On Thu, Aug 1, 2019 at 1:23 PM Stephen Boyd wrote: > > Quoting Tri Vo (2019-08-01 12:50:25) > > On Wed, Jul 31, 2019 at 4:45 PM Stephen Boyd wrote: > > > > > > Quoting Rafael J. Wysocki (2019-07-31 16:10:38) > > > > On Thu, Aug 1, 2019 at 12:59 AM Tri Vo 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.
Re: [PATCH v6] PM / wakeup: show wakeup sources stats in sysfs
On Wed, Jul 31, 2019 at 4:45 PM Stephen Boyd wrote: > > Quoting Rafael J. Wysocki (2019-07-31 16:10:38) > > On Thu, Aug 1, 2019 at 12:59 AM Tri Vo wrote: > > > > > > On Wed, Jul 31, 2019 at 3:42 PM Rafael J. Wysocki > > > wrote: > > > > > > > > That's not my point (see below). > > > > > > > > > > > > + if (id < 0) > > > > > > > > + return id; > > > > > > > > + ws->id = id; > > > > > > > > + > > > > > > > > + dev = device_create_with_groups(wakeup_class, parent, > > > > > > > > MKDEV(0, 0), ws, > > > > > > > > + wakeup_source_groups, > > > > > > > > "ws%d", > > > > > > > > > > > > > > I thought the name was going to still be 'wakeupN'? > > > > > > > > > > > > So can't we prefix the wakeup source name with something like > > > > > > "wakeup:" or similar here? > > > > > > > > > > "ws%d" here is the name in the sysfs path rather than the name of the > > > > > wakeup source. Wakeup source name is not altered in this patch. > > > > > > > > > > > > > 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. IMO it's cleaner to consistently have /sys/class/wakeup/wakeupN/name for every wakeup source.
Re: [PATCH v6] PM / wakeup: show wakeup sources stats in sysfs
On Wed, Jul 31, 2019 at 4:10 PM Rafael J. Wysocki wrote:
>
> On Thu, Aug 1, 2019 at 12:59 AM Tri Vo wrote:
> >
> > On Wed, Jul 31, 2019 at 3:42 PM Rafael J. Wysocki
> > wrote:
> > >
> > > On Thursday, August 1, 2019 12:31:16 AM CEST Tri Vo wrote:
> > > > On Wed, Jul 31, 2019 at 3:17 PM Rafael J. Wysocki
> > > > wrote:
> > > > >
> > > > > On Wednesday, July 31, 2019 11:59:32 PM CEST Stephen Boyd wrote:
> > > > > > Quoting Tri Vo (2019-07-31 14:55:14)
> > > > > > > +/**
> > > > > > > + * wakeup_source_sysfs_add - Add wakeup_source attributes to
> > > > > > > sysfs.
> > > > > > > + * @parent: Device given wakeup source is associated with (or
> > > > > > > NULL if virtual).
> > > > > > > + * @ws: Wakeup source to be added in sysfs.
> > > > > > > + */
> > > > > > > +int wakeup_source_sysfs_add(struct device *parent, struct
> > > > > > > wakeup_source *ws)
> > > > > > > +{
> > > > > > > + struct device *dev;
> > > > > > > + int id;
> > > > > > > +
> > > > > > > + id = ida_alloc(_ida, GFP_KERNEL);
> > > > >
> > > > > So can anyone remind me why the IDA thing is needed here at all?
> > > >
> > > > IDA is used to generate the directory name ("ws%d", ID) that is unique
> > > > among wakeup_sources. That is what ends up in
> > > > /sys/class/wakeup/ws/* path.
> > >
> > > That's not my point (see below).
> > >
> > > > > > > + if (id < 0)
> > > > > > > + return id;
> > > > > > > + ws->id = id;
> > > > > > > +
> > > > > > > + dev = device_create_with_groups(wakeup_class, parent,
> > > > > > > MKDEV(0, 0), ws,
> > > > > > > + wakeup_source_groups,
> > > > > > > "ws%d",
> > > > > >
> > > > > > I thought the name was going to still be 'wakeupN'?
> > > > >
> > > > > So can't we prefix the wakeup source name with something like
> > > > > "wakeup:" or similar here?
> > > >
> > > > "ws%d" here is the name in the sysfs path rather than the name of the
> > > > wakeup source. Wakeup source name is not altered in this patch.
> > > >
> > >
> > > 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?
Yes, in Android we do rely on the name to aggregate statistics across
a fleet of devices. That wouldn't be possible with just the id, as
those are generated at dynamically runtime.
>
> That's kind of fair enough, but having two different identification
> schemes for wakeup sources will end up confusing.
It's not without precedent though. rtc, input, and other devices have
a similar scheme.
Re: [PATCH v6] PM / wakeup: show wakeup sources stats in sysfs
On Wed, Jul 31, 2019 at 3:42 PM Rafael J. Wysocki wrote:
>
> On Thursday, August 1, 2019 12:31:16 AM CEST Tri Vo wrote:
> > On Wed, Jul 31, 2019 at 3:17 PM Rafael J. Wysocki
> > wrote:
> > >
> > > On Wednesday, July 31, 2019 11:59:32 PM CEST Stephen Boyd wrote:
> > > > Quoting Tri Vo (2019-07-31 14:55:14)
> > > > > +/**
> > > > > + * wakeup_source_sysfs_add - Add wakeup_source attributes to sysfs.
> > > > > + * @parent: Device given wakeup source is associated with (or NULL
> > > > > if virtual).
> > > > > + * @ws: Wakeup source to be added in sysfs.
> > > > > + */
> > > > > +int wakeup_source_sysfs_add(struct device *parent, struct
> > > > > wakeup_source *ws)
> > > > > +{
> > > > > + struct device *dev;
> > > > > + int id;
> > > > > +
> > > > > + id = ida_alloc(_ida, GFP_KERNEL);
> > >
> > > So can anyone remind me why the IDA thing is needed here at all?
> >
> > IDA is used to generate the directory name ("ws%d", ID) that is unique
> > among wakeup_sources. That is what ends up in
> > /sys/class/wakeup/ws/* path.
>
> That's not my point (see below).
>
> > > > > + if (id < 0)
> > > > > + return id;
> > > > > + ws->id = id;
> > > > > +
> > > > > + dev = device_create_with_groups(wakeup_class, parent,
> > > > > MKDEV(0, 0), ws,
> > > > > + wakeup_source_groups, "ws%d",
> > > >
> > > > I thought the name was going to still be 'wakeupN'?
> > >
> > > So can't we prefix the wakeup source name with something like "wakeup:"
> > > or similar here?
> >
> > "ws%d" here is the name in the sysfs path rather than the name of the
> > wakeup source. Wakeup source name is not altered in this patch.
> >
>
> 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.
Re: [PATCH v6] PM / wakeup: show wakeup sources stats in sysfs
On Wed, Jul 31, 2019 at 3:17 PM Rafael J. Wysocki wrote:
>
> On Wednesday, July 31, 2019 11:59:32 PM CEST Stephen Boyd wrote:
> > Quoting Tri Vo (2019-07-31 14:55:14)
> > > +/**
> > > + * wakeup_source_sysfs_add - Add wakeup_source attributes to sysfs.
> > > + * @parent: Device given wakeup source is associated with (or NULL if
> > > virtual).
> > > + * @ws: Wakeup source to be added in sysfs.
> > > + */
> > > +int wakeup_source_sysfs_add(struct device *parent, struct wakeup_source
> > > *ws)
> > > +{
> > > + struct device *dev;
> > > + int id;
> > > +
> > > + id = ida_alloc(_ida, GFP_KERNEL);
>
> So can anyone remind me why the IDA thing is needed here at all?
IDA is used to generate the directory name ("ws%d", ID) that is unique
among wakeup_sources. That is what ends up in
/sys/class/wakeup/ws/* path.
>
> > > + if (id < 0)
> > > + return id;
> > > + ws->id = id;
> > > +
> > > + dev = device_create_with_groups(wakeup_class, parent, MKDEV(0,
> > > 0), ws,
> > > + wakeup_source_groups, "ws%d",
> >
> > I thought the name was going to still be 'wakeupN'?
>
> So can't we prefix the wakeup source name with something like "wakeup:" or
> similar here?
"ws%d" here is the name in the sysfs path rather than the name of the
wakeup source. Wakeup source name is not altered in this patch.
Re: [PATCH v6] PM / wakeup: show wakeup sources stats in sysfs
On Wed, Jul 31, 2019 at 2:59 PM Stephen Boyd wrote:
>
> Quoting Tri Vo (2019-07-31 14:55:14)
> > +/**
> > + * wakeup_source_sysfs_add - Add wakeup_source attributes to sysfs.
> > + * @parent: Device given wakeup source is associated with (or NULL if
> > virtual).
> > + * @ws: Wakeup source to be added in sysfs.
> > + */
> > +int wakeup_source_sysfs_add(struct device *parent, struct wakeup_source
> > *ws)
> > +{
> > + struct device *dev;
> > + int id;
> > +
> > + id = ida_alloc(_ida, GFP_KERNEL);
> > + if (id < 0)
> > + return id;
> > + ws->id = id;
> > +
> > + dev = device_create_with_groups(wakeup_class, parent, MKDEV(0, 0),
> > ws,
> > + wakeup_source_groups, "ws%d",
>
> I thought the name was going to still be 'wakeupN'?
I don't really have an opinion on this. Rafael seems to prefer "ws",
and he's the maintainer :)
>
> > + ws->id);
> > + if (IS_ERR(dev)) {
> > + ida_free(_ida, ws->id);
> > + return PTR_ERR(dev);
> > + }
> > +
> > + ws->dev = dev;
> > + return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(wakeup_source_sysfs_add);
> > +
> > +/**
> > + * wakeup_source_sysfs_remove - Remove wakeup_source attributes from sysfs.
> > + * @ws: Wakeup source to be removed from sysfs.
> > + */
> > +void wakeup_source_sysfs_remove(struct wakeup_source *ws)
> > +{
> > + device_unregister(ws->dev);
> > + ida_simple_remove(_ida, ws->id);
>
> Should be ida_free()?
oops
>
> > +}
> > +EXPORT_SYMBOL_GPL(wakeup_source_sysfs_remove);
> > +
> > +static int __init wakeup_sources_sysfs_init(void)
> > +{
> > + wakeup_class = class_create(THIS_MODULE, "wakeup");
> > +
> > + return PTR_ERR_OR_ZERO(wakeup_class);
> > +}
> > +
> > +postcore_initcall(wakeup_sources_sysfs_init);
>
> Style nitpick: Stick the initcall to the function it calls by dropping
> the extra newline between them.
will do
[PATCH v6] PM / wakeup: show wakeup sources stats in sysfs
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, create a 'struct device' to expose wakeup sources statistics in sysfs under /sys/class/wakeup/ws/*. Co-developed-by: Greg Kroah-Hartman Signed-off-by: Greg Kroah-Hartman Co-developed-by: Stephen Boyd Signed-off-by: Stephen Boyd Signed-off-by: Tri Vo Tested-by: Tri Vo Tested-by: Kalesh Singh --- Documentation/ABI/testing/sysfs-class-wakeup | 76 + drivers/acpi/device_pm.c | 3 +- drivers/base/power/Makefile | 2 +- drivers/base/power/wakeup.c | 18 +- drivers/base/power/wakeup_stats.c| 171 +++ fs/eventpoll.c | 4 +- include/linux/pm_wakeup.h| 15 +- kernel/power/autosleep.c | 2 +- kernel/power/wakelock.c | 10 ++ kernel/time/alarmtimer.c | 2 +- 10 files changed, 291 insertions(+), 12 deletions(-) create mode 100644 Documentation/ABI/testing/sysfs-class-wakeup create mode 100644 drivers/base/power/wakeup_stats.c v2: - Updated Documentation/ABI/, as per Greg. - Removed locks in attribute functions, as per Greg. - Lifetimes of struct wakelock and struck wakeup_source are now different due to the latter embedding a refcounted kobject. Changed it so that struct wakelock only has a pointer to struct wakeup_source, instead of embedding it. - Added CONFIG_PM_SLEEP_STATS that enables/disables wakeup source statistics in sysfs. v3: Changes by Greg: - Reworked code to use 'struct device' instead of raw kobjects. - Updated documentation file. - Only link wakeup_stats.o when CONFIG_PM_SLEEP_STATS is enabled. Changes by Tri: - Reverted changes to kernel/power/wakelock.c. 'struct device' hides kobject operations. So no need to handle lifetimes in wakelock.c v4: - Added 'Co-developed-by:' and 'Tested-by:' fields to commit message. - Moved new documentation to a separate file Documentation/ABI/testing/sysfs-class-wakeup, as per Greg. - Fixed copyright header in drivers/base/power/wakeup_stats.c, as per Greg. v5: - Removed CONFIG_PM_SLEEP_STATS - Used PTR_ERR_OR_ZERO instead of if(IS_ERR(...)) + PTR_ERR, reported by kbuild test robot - Stephen reported that a call to device_init_wakeup() and writing 'enabled' to that device's power/wakeup file results in multiple wakeup source being allocated for that device. Changed device_wakeup_enable() to check if device wakeup was previously enabled. Changes by Stephen: - Changed stats location from /sys/class/wakeup//* to /sys/class/wakeup/wakeup/*, where ID is an IDA-allocated integer. This avoids name collisions in /sys/class/wakeup/ directory. - Added a "name" attribute to wakeup sources, and updated documentation. - Device registering the wakeup source is now the parent of the wakeup source. Updated wakeup_source_register()'s signature and its callers accordingly. v6: - Changed stats location to /sys/class/wakeup/ws/* - Replaced ida_simple_get()/ida_simple_remove() with ida_alloc()/ida_free() as the former is deprecated. - Reverted changes to device_init_wakeup(). Rafael is preparing a patch to deal with extra wakeup source allocation in a separate patch. diff --git a/Documentation/ABI/testing/sysfs-class-wakeup b/Documentation/ABI/testing/sysfs-class-wakeup new file mode 100644 index ..754aab8b6dcd --- /dev/null +++ b/Documentation/ABI/testing/sysfs-class-wakeup @@ -0,0 +1,76 @@ +What: /sys/class/wakeup/ +Date: June 2019 +Contact: Tri Vo +Description: + The /sys/class/wakeup/ directory contains pointers to all + wakeup sources in the kernel at that moment in time. + +What: /sys/class/wakeup/.../name +Date: June 2019 +Contact: Tri Vo +Description: + This file contains the name of the wakeup source. + +What: /sys/class/wakeup/.../active_count +Date: June 2019 +Contact: Tri Vo +Description: + This file contains the number of times the wakeup source was + activated. + +What: /sys/class/wakeup/.../event_count +Date: June 2019 +Contact: Tri Vo +Description: + This file contains the number of signaled wakeup events + associated with the wakeup source. + +What: /sys/class/wakeup/.../wakeup_count +Date: June 2019 +Contact: Tri Vo +Description: + This file contains the number of times the wakeup source might + abort suspend. + +What: /sys/class/wakeup/.../expire_count +Date: June 2019 +Contact:
Re: [PATCH v5] PM / wakeup: show wakeup sources stats in sysfs
On Wed, Jul 31, 2019 at 2:19 PM Rafael J. Wysocki wrote:
>
> On Wed, Jul 31, 2019 at 7:14 PM Stephen Boyd wrote:
> >
> > Quoting Rafael J. Wysocki (2019-07-31 04:58:36)
> > > On Wednesday, July 31, 2019 10:34:11 AM CEST Rafael J. Wysocki wrote:
> > > > On Wed, Jul 31, 2019 at 1:41 AM Stephen Boyd
> > > > wrote:
> > > > >
> > > >
> > > > > 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.
> > > >
> > > > Or suffix, like ".
> > > >
> > > > But if prefixes are used by an existing convention, I would prefer
> > > > "ws-" as it is concise enough and should not be confusing.
> >
> > Another possibility is 'eventN', so it reads as /sys/class/wakeup/event0
> >
> > > >
> > > > > 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.
> > > >
> > > > Since we agree here, let's make this change first. I can cut a patch
> > > > for that in a reasonable time frame I think if no one else beats me to
> > > > that.
> > >
> > > So maybe something like the patch below (untested).
> > >
> > > Index: linux-pm/drivers/base/power/wakeup.c
> > > ===
> > > --- linux-pm.orig/drivers/base/power/wakeup.c
> > > +++ linux-pm/drivers/base/power/wakeup.c
> > > @@ -265,15 +244,29 @@ int device_wakeup_enable(struct device *
> > > if (pm_suspend_target_state != PM_SUSPEND_ON)
> > > dev_dbg(dev, "Suspicious %s() during system
> > > transition!\n", __func__);
> > >
> > > + spin_lock_irq(>power.lock);
> > > +
> > > + if (dev->power.wakeup) {
> > > + spin_unlock_irq(>power.lock);
> > > + return -EEXIST;
> > > + }
> > > + dev->power.wakeup = ERR_PTR(-EBUSY);
> > > +
> > > + spin_unlock_irq(>power.lock);
> > > +
> > > ws = wakeup_source_register(dev_name(dev));
> > > if (!ws)
> > > return -ENOMEM;
> > >
> >
> > Let's say that device_wakeup_enable() is called twice at around the same
> > time. First thread gets to wakeup_source_register() and it fails, we
> > return -ENOMEM.
>
> The return is premature. dev->power.wakeup should be reset back to
> NULL if the wakeup source creation fails.
>
> > dev->power.wakeup is assigned to ERR_PTR(-EBUSY). Second
> > thread is at the spin_lock_irq() above, it grabs the lock and sees
> > dev->power.wakeup is ERR_PTR(-EBUSY) so it bails out with return
> > -EEXIST. I'd think we would want to try to create the wakeup source
> > instead.
> >
> > CPU0 CPU1
> >
> > spin_lock_irq(>power.lock)
> > ...
> > dev->power.wakeup = ERR_PTR(-EBUSY)
> > spin_unlock_irq(>power.lock)
> > ws = wakeup_source_register(...)
> > if (!ws)
> > return -ENOMEM; spin_lock_irq(>power.lock)
> > if (dev->power.wakeup)
> > return -EEXIST; // Bad
> >
> >
> > Similar problems probably exist with wakeup destruction racing with
> > creation. I think it might have to be a create and then publish pointer
> > style of code to keep the spinlock section small?
>
> There is a problem when there are two concurrent callers of
> device_wakeup_enable() running in parallel with a caller of
> device_wakeup_disable(), but that can be prevented by an extra check
> in the latter.
>
> Apart from that I missed a few if (dev->power.wakeup) checks to convert.
>
> I'll update the patch and resend it.
Ok thanks, I'll ignore the device_wakeup_enable() issue in this patch,
since you're addressing it in a separate patch.
IIUC checking and assigning to dev->power.wakeup must be in the same
critical section for correctness, implying that allocation of the
wakeup source must also be in that critical section (since we check
dev->power.wakeup to see whether we need a wakeup source).
Wakeup source virtual device registration can fail (it allocates
memory), in which case dev->power.wakeup need to be cleaned up.
Meaning that wakeup source virtual device registration need to be in
that same critical section.
So I'm not sure it is at all
Re: [PATCH v5] PM / wakeup: show wakeup sources stats in sysfs
On Tue, Jul 30, 2019 at 4:06 PM Rafael J. Wysocki wrote: > > On Wed, Jul 31, 2019 at 12:26 AM Stephen Boyd wrote: > > > > Quoting Rafael J. Wysocki (2019-07-30 15:17:55) > > > On Tuesday, July 30, 2019 8:48:09 PM CEST Stephen Boyd wrote: > > > > Quoting Tri Vo (2019-07-30 11:39:34) > > > > > On Mon, Jul 29, 2019 at 10:46 PM Rafael J. Wysocki > > > > > wrote: > > > > > > > > > > > > On Tue, Jul 30, 2019 at 4:45 AM Tri Vo wrote: > > > > > > > - Device registering the wakeup source is now the parent of the > > > > > > > wakeup source. > > > > > > > Updated wakeup_source_register()'s signature and its callers > > > > > > > accordingly. > > > > > > > > > > > > And I really don't like these changes. Especially having "wakeup" > > > > > > twice in the path. > > > > > > > > > > I can trim it down to /sys/class/wakeup//. Does that sound good? > > > > > > > > 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. > > 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. This addresses the issue with device_wakeup_enable(), but IIUC we still have the general problem of multiple devices having the same name, which leads to name collisions when identifying wakeup sources with the name only.
Re: [PATCH v5] PM / wakeup: show wakeup sources stats in sysfs
On Mon, Jul 29, 2019 at 11:47 PM Greg KH wrote:
>
> On Mon, Jul 29, 2019 at 07:43:09PM -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, create a
> > 'struct device' to expose wakeup sources statistics in sysfs under
> > /sys/class/wakeup/wakeup/*.
>
> I agree with Rafael here, no need for the extra "wakeup" in the device
> name as you are in the "wakeup" namespace already.
>
> If you have an IDA-allocated name, there's no need for the extra
> 'wakeup' at all.
>
> > +int wakeup_source_sysfs_add(struct device *parent, struct wakeup_source
> > *ws)
> > +{
> > + struct device *dev;
> > + int id;
> > +
> > + id = ida_simple_get(_ida, 0, 0, GFP_KERNEL);
> > + if (id < 0)
> > + return id;
>
> No lock needed for this ida? Are you sure?
>
> > + ws->id = id;
> > +
> > + dev = device_create_with_groups(wakeup_class, parent, MKDEV(0, 0), ws,
> > + wakeup_source_groups, "wakeup%d",
> > + ws->id);
> > + if (IS_ERR(dev)) {
> > + ida_simple_remove(_ida, ws->id);
> > + return PTR_ERR(dev);
> > + }
> > +
> > + ws->dev = dev;
> > + return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(wakeup_source_sysfs_add);
> > +
> > +/**
> > + * wakeup_source_sysfs_remove - Remove wakeup_source attributes from sysfs.
> > + * @ws: Wakeup source to be removed from sysfs.
> > + */
> > +void wakeup_source_sysfs_remove(struct wakeup_source *ws)
> > +{
> > + device_unregister(ws->dev);
> > + ida_simple_remove(_ida, ws->id);
>
> Again, no lock, is that ok? I think ida's can work without a lock, but
> not always, sorry, I don't remember the rules anymore given the recent
> changes in that code.
Documentation says, "The IDA handles its own locking. It is safe to
call any of the IDA functions without synchronisation in your code."
https://www.kernel.org/doc/html/latest/core-api/idr.html#ida-usage
Re: [PATCH v5] PM / wakeup: show wakeup sources stats in sysfs
On Mon, Jul 29, 2019 at 10:46 PM Rafael J. Wysocki wrote: > > On Tue, Jul 30, 2019 at 4:45 AM 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, create a > > 'struct device' to expose wakeup sources statistics in sysfs under > > /sys/class/wakeup/wakeup/*. > > > > Co-developed-by: Greg Kroah-Hartman > > Signed-off-by: Greg Kroah-Hartman > > Co-developed-by: Stephen Boyd > > Signed-off-by: Stephen Boyd > > Signed-off-by: Tri Vo > > Tested-by: Tri Vo > > Tested-by: Kalesh Singh > > Reported-by: kbuild test robot > > --- > > Documentation/ABI/testing/sysfs-class-wakeup | 76 + > > drivers/acpi/device_pm.c | 3 +- > > drivers/base/power/Makefile | 2 +- > > drivers/base/power/wakeup.c | 21 ++- > > drivers/base/power/wakeup_stats.c| 171 +++ > > fs/eventpoll.c | 4 +- > > include/linux/pm_wakeup.h| 15 +- > > kernel/power/autosleep.c | 2 +- > > kernel/power/wakelock.c | 10 ++ > > kernel/time/alarmtimer.c | 2 +- > > 10 files changed, 294 insertions(+), 12 deletions(-) > > create mode 100644 Documentation/ABI/testing/sysfs-class-wakeup > > create mode 100644 drivers/base/power/wakeup_stats.c > > > > v2: > > - Updated Documentation/ABI/, as per Greg. > > - Removed locks in attribute functions, as per Greg. > > - Lifetimes of struct wakelock and struck wakeup_source are now different > > due to > > the latter embedding a refcounted kobject. Changed it so that struct > > wakelock > > only has a pointer to struct wakeup_source, instead of embedding it. > > - Added CONFIG_PM_SLEEP_STATS that enables/disables wakeup source > > statistics in > > sysfs. > > > > v3: > > Changes by Greg: > > - Reworked code to use 'struct device' instead of raw kobjects. > > - Updated documentation file. > > - Only link wakeup_stats.o when CONFIG_PM_SLEEP_STATS is enabled. > > Changes by Tri: > > - Reverted changes to kernel/power/wakelock.c. 'struct device' hides kobject > > operations. So no need to handle lifetimes in wakelock.c > > > > v4: > > - Added 'Co-developed-by:' and 'Tested-by:' fields to commit message. > > - Moved new documentation to a separate file > > Documentation/ABI/testing/sysfs-class-wakeup, as per Greg. > > - Fixed copyright header in drivers/base/power/wakeup_stats.c, as per Greg. > > > > v5: > > - Removed CONFIG_PM_SLEEP_STATS > > - Used PTR_ERR_OR_ZERO instead of if(IS_ERR(...)) + PTR_ERR, reported by > > kbuild test robot > > - Stephen reported that a call to device_init_wakeup() and writing > > 'enabled' to > > that device's power/wakeup file results in multiple wakeup source being > > allocated for that device. Changed device_wakeup_enable() to check if > > device > > wakeup was previously enabled. > > Changes by Stephen: > > - Changed stats location from /sys/class/wakeup//* to > > /sys/class/wakeup/wakeup/*, where ID is an IDA-allocated integer. This > > avoids name collisions in /sys/class/wakeup/ directory. > > - Added a "name" attribute to wakeup sources, and updated documentation. > > - Device registering the wakeup source is now the parent of the wakeup > > source. > > Updated wakeup_source_register()'s signature and its callers accordingly. > > And I really don't like these changes. Especially having "wakeup" > twice in the path. I can trim it down to /sys/class/wakeup//. Does that sound good? About the other change, I think making the registering device the parent of the wakeup source is a worthwhile change, since that way one can associate a wakeup source sysfs entry with the device that created it. > > Couldn't you find a simpler way to avoid the name collisions? I could also simply log an error in case of a name collision instead of failing hard. That way I can keep the old path with the wakeup source name in it. Other than that, I can't think of a way to resolve the directory name collisions without making that directory name unique, i.e. generating IDs is probably the simplest way. I'm still learning about the kernel, and I might be wrong though. What do you think?
[PATCH v5] PM / wakeup: show wakeup sources stats in sysfs
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, create a 'struct device' to expose wakeup sources statistics in sysfs under /sys/class/wakeup/wakeup/*. Co-developed-by: Greg Kroah-Hartman Signed-off-by: Greg Kroah-Hartman Co-developed-by: Stephen Boyd Signed-off-by: Stephen Boyd Signed-off-by: Tri Vo Tested-by: Tri Vo Tested-by: Kalesh Singh Reported-by: kbuild test robot --- Documentation/ABI/testing/sysfs-class-wakeup | 76 + drivers/acpi/device_pm.c | 3 +- drivers/base/power/Makefile | 2 +- drivers/base/power/wakeup.c | 21 ++- drivers/base/power/wakeup_stats.c| 171 +++ fs/eventpoll.c | 4 +- include/linux/pm_wakeup.h| 15 +- kernel/power/autosleep.c | 2 +- kernel/power/wakelock.c | 10 ++ kernel/time/alarmtimer.c | 2 +- 10 files changed, 294 insertions(+), 12 deletions(-) create mode 100644 Documentation/ABI/testing/sysfs-class-wakeup create mode 100644 drivers/base/power/wakeup_stats.c v2: - Updated Documentation/ABI/, as per Greg. - Removed locks in attribute functions, as per Greg. - Lifetimes of struct wakelock and struck wakeup_source are now different due to the latter embedding a refcounted kobject. Changed it so that struct wakelock only has a pointer to struct wakeup_source, instead of embedding it. - Added CONFIG_PM_SLEEP_STATS that enables/disables wakeup source statistics in sysfs. v3: Changes by Greg: - Reworked code to use 'struct device' instead of raw kobjects. - Updated documentation file. - Only link wakeup_stats.o when CONFIG_PM_SLEEP_STATS is enabled. Changes by Tri: - Reverted changes to kernel/power/wakelock.c. 'struct device' hides kobject operations. So no need to handle lifetimes in wakelock.c v4: - Added 'Co-developed-by:' and 'Tested-by:' fields to commit message. - Moved new documentation to a separate file Documentation/ABI/testing/sysfs-class-wakeup, as per Greg. - Fixed copyright header in drivers/base/power/wakeup_stats.c, as per Greg. v5: - Removed CONFIG_PM_SLEEP_STATS - Used PTR_ERR_OR_ZERO instead of if(IS_ERR(...)) + PTR_ERR, reported by kbuild test robot - Stephen reported that a call to device_init_wakeup() and writing 'enabled' to that device's power/wakeup file results in multiple wakeup source being allocated for that device. Changed device_wakeup_enable() to check if device wakeup was previously enabled. Changes by Stephen: - Changed stats location from /sys/class/wakeup//* to /sys/class/wakeup/wakeup/*, where ID is an IDA-allocated integer. This avoids name collisions in /sys/class/wakeup/ directory. - Added a "name" attribute to wakeup sources, and updated documentation. - Device registering the wakeup source is now the parent of the wakeup source. Updated wakeup_source_register()'s signature and its callers accordingly. diff --git a/Documentation/ABI/testing/sysfs-class-wakeup b/Documentation/ABI/testing/sysfs-class-wakeup new file mode 100644 index ..754aab8b6dcd --- /dev/null +++ b/Documentation/ABI/testing/sysfs-class-wakeup @@ -0,0 +1,76 @@ +What: /sys/class/wakeup/ +Date: June 2019 +Contact: Tri Vo +Description: + The /sys/class/wakeup/ directory contains pointers to all + wakeup sources in the kernel at that moment in time. + +What: /sys/class/wakeup/.../name +Date: June 2019 +Contact: Tri Vo +Description: + This file contains the name of the wakeup source. + +What: /sys/class/wakeup/.../active_count +Date: June 2019 +Contact: Tri Vo +Description: + This file contains the number of times the wakeup source was + activated. + +What: /sys/class/wakeup/.../event_count +Date: June 2019 +Contact: Tri Vo +Description: + This file contains the number of signaled wakeup events + associated with the wakeup source. + +What: /sys/class/wakeup/.../wakeup_count +Date: June 2019 +Contact: Tri Vo +Description: + This file contains the number of times the wakeup source might + abort suspend. + +What: /sys/class/wakeup/.../expire_count +Date: June 2019 +Contact: Tri Vo +Description: + This file contains the number of times the wakeup source's + timeout has expired. + +What: /sys/class/wakeup/.../active_time_ms +Date: June 2019 +Contact: Tri Vo +D
Re: [PATCH] PM / wakeup: Avoid dev_name collisions in wakeup class
On Sat, Jul 27, 2019 at 6:10 AM Rafael J. Wysocki wrote: > > On Sat, Jul 27, 2019 at 3:11 AM Stephen Boyd wrote: > > > > If a device is wakeup capable and the driver calls device_wakeup_init() > > on it during probe and then userspace writes 'enabled' to that device's > > power/wakeup file in sysfs we'll try to create the same named wakeup > > device in sysfs. The kernel will complain about duplicate file names. Thanks for reporting the issue, Stephen! > > > > sysfs: cannot create duplicate filename '/devices/virtual/wakeup/1-1.1' > > kobject_add_internal failed for 1-1.1 with -EEXIST, don't try to register > > things with the same name in the same directory. > > > > It may be advantageous to not write 'enabled' to the wakeup file (see > > wakeup_store()) from userspace for these devices because we allocate > > devices and register them and then throw them all away later on if the > > device driver has already initialized the wakeup attribute. The > > implementation currently tries to avoid taking locks here so it seems > > best to optimize that path in a separate patch. > > > > Let's rename the wakeup class devices as 'wakeupN' with an IDA that's > > simple enough to just return some sort of number. In addition, let's > > make the device registering the wakeup the parent and include a 'name' > > attribute in case userspace wants to figure out the type of wakeup it is > > (in the case of virtual wakeups) or the device associated with the > > wakeup. This makes it easier for userspace to go from /sys/class/wakeup > > to a place in the device hierarchy where the wakeup is generated from > > like an input device. > > > > Cc: Tri Vo > > Cc: Kalesh Singh > > Cc: Greg Kroah-Hartman > > Cc: Ravi Chandra Sadineni > > Signed-off-by: Stephen Boyd > > I'd rather change the commit that introduced this issue which is only > in linux-next for now. Raphael, could you roll back my patch? I'll work with Stephen to fix it.
Re: [PATCH v4] PM / wakeup: show wakeup sources stats in sysfs
On Tue, Jul 16, 2019 at 11:13 AM Greg Kroah-Hartman wrote: > > On Mon, Jul 15, 2019 at 11:48:27PM +0200, Rafael J. Wysocki wrote: > > On Mon, Jul 15, 2019 at 11:44 PM 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, create a > > > 'struct device' to expose wakeup sources statistics in sysfs under > > > /sys/class/wakeup//. > > > > > > Introduce CONFIG_PM_SLEEP_STATS that enables/disables showing wakeup > > > source statistics in sysfs. > > > > I'm not sure if this is really needed, but I'll let Greg decide. > > You are right. Having zillions of config options is a pain, who is > going to turn this off? > > But we can always remove the option before 5.4-rc1, so I'll take this > as-is for now :) > > > Apart from this > > > > Reviewed-by: Rafael J. Wysocki > > thanks for the review! I'll wait for 5.3-rc1 to come out before adding > this to my tree. Greg, Rafael, thanks for taking the time to review this patch!
Re: [PATCH v3] PM / wakeup: show wakeup sources stats in sysfs
On Mon, Jul 15, 2019 at 1:37 PM Greg KH wrote: > > On Mon, Jul 15, 2019 at 01:11:16PM -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, create a > > 'struct device' to expose wakeup sources statistics in sysfs under > > /sys/class/wakeup//. > > > > Introduce CONFIG_PM_SLEEP_STATS that enables/disables showing wakeup > > source statistics in sysfs. > > > > Suggested-by: Greg Kroah-Hartman > > Signed-off-by: Tri Vo > > Tested-by: Tri Vo > > Co-Developed-by: Greg Kroah-Hartman > perhaps given that I rewrote the whole file last time? :) Thanks again for the help! > > > > --- > > Documentation/ABI/testing/sysfs-power | 73 - > > drivers/base/power/Makefile | 1 + > > drivers/base/power/wakeup.c | 12 ++- > > drivers/base/power/wakeup_stats.c | 148 ++ > > include/linux/pm_wakeup.h | 19 > > kernel/power/Kconfig | 10 ++ > > kernel/power/wakelock.c | 10 ++ > > 7 files changed, 270 insertions(+), 3 deletions(-) > > create mode 100644 drivers/base/power/wakeup_stats.c > > What changed from v2? :) Oops, my bad. > And I am guessing that you actually tested this all out, and it works > for you? Yes, I played around with wakelocks to make sure that wakeup source stats are added/updated/removed as expected. > Have you changed Android userspace to use the new api with no > problems? Kalesh helped me test this patch (added him in Tested-by: field in latest patch version). We haven't tested beyond booting and manual inspection on android devices. Android userspace changes should be fairly trivial though.
[PATCH v4] PM / wakeup: show wakeup sources stats in sysfs
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, create a 'struct device' to expose wakeup sources statistics in sysfs under /sys/class/wakeup//. Introduce CONFIG_PM_SLEEP_STATS that enables/disables showing wakeup source statistics in sysfs. Co-developed-by: Greg Kroah-Hartman Signed-off-by: Tri Vo Tested-by: Tri Vo Tested-by: Kalesh Singh --- Documentation/ABI/testing/sysfs-class-wakeup | 70 + drivers/base/power/Makefile | 1 + drivers/base/power/wakeup.c | 12 +- drivers/base/power/wakeup_stats.c| 149 +++ include/linux/pm_wakeup.h| 19 +++ kernel/power/Kconfig | 10 ++ kernel/power/wakelock.c | 10 ++ 7 files changed, 269 insertions(+), 2 deletions(-) create mode 100644 Documentation/ABI/testing/sysfs-class-wakeup create mode 100644 drivers/base/power/wakeup_stats.c v2: - Updated Documentation/ABI/, as per Greg. - Removed locks in attribute functions, as per Greg. - Lifetimes of struct wakelock and struck wakeup_source are now different due to the latter embedding a refcounted kobject. Changed it so that struct wakelock only has a pointer to struct wakeup_source, instead of embedding it. - Added CONFIG_PM_SLEEP_STATS that enables/disables wakeup source statistics in sysfs. v3: Changes by Greg: - Reworked code to use 'struct device' instead of raw kobjects. - Updated documentation file. - Only link wakeup_stats.o when CONFIG_PM_SLEEP_STATS is enabled. Changes by Tri: - Reverted changes to kernel/power/wakelock.c. 'struct device' hides kobject operations. So no need to handle lifetimes in wakelock.c v4: - Added 'Co-developed-by:' and 'Tested-by:' fields to commit message. - Moved new documentation to a separate file Documentation/ABI/testing/sysfs-class-wakeup, as per Greg. - Fixed copyright header in drivers/base/power/wakeup_stats.c, as per Greg. diff --git a/Documentation/ABI/testing/sysfs-class-wakeup b/Documentation/ABI/testing/sysfs-class-wakeup new file mode 100644 index ..30fb23eb9112 --- /dev/null +++ b/Documentation/ABI/testing/sysfs-class-wakeup @@ -0,0 +1,70 @@ +What: /sys/class/wakeup/ +Date: June 2019 +Contact: Tri Vo +Description: + The /sys/class/wakeup/ directory contains pointers to all + wakeup sources in the kernel at that moment in time. + +What: /sys/class/wakeup/.../active_count +Date: June 2019 +Contact: Tri Vo +Description: + This file contains the number of times the wakeup source was + activated. + +What: /sys/class/wakeup/.../event_count +Date: June 2019 +Contact: Tri Vo +Description: + This file contains the number of signaled wakeup events + associated with the wakeup source. + +What: /sys/class/wakeup/.../wakeup_count +Date: June 2019 +Contact: Tri Vo +Description: + This file contains the number of times the wakeup source might + abort suspend. + +What: /sys/class/wakeup/.../expire_count +Date: June 2019 +Contact: Tri Vo +Description: + This file contains the number of times the wakeup source's + timeout has expired. + +What: /sys/class/wakeup/.../active_time_ms +Date: June 2019 +Contact: Tri Vo +Description: + This file contains the amount of time the wakeup source has + been continuously active, in milliseconds. If the wakeup + source is not active, this file contains '0'. + +What: /sys/class/wakeup/.../total_time_ms +Date: June 2019 +Contact: Tri Vo +Description: + This file contains the total amount of time this wakeup source + has been active, in milliseconds. + +What: /sys/class/wakeup/.../max_time_ms +Date: June 2019 +Contact: Tri Vo +Description: + This file contains the maximum amount of time this wakeup + source has been continuously active, in milliseconds. + +What: /sys/class/wakeup/.../last_change_ms +Date: June 2019 +Contact: Tri Vo +Description: + This file contains the monotonic clock time when the wakeup + source was touched last time, in milliseconds. + +What: /sys/class/wakeup/.../prevent_suspend_time_ms +Date: June 2019 +Contact: Tri Vo +Description: + The file contains the total amount of time this wakeup source + has been preventing autosleep
[PATCH v3] PM / wakeup: show wakeup sources stats in sysfs
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, create a 'struct device' to expose wakeup sources statistics in sysfs under /sys/class/wakeup//. Introduce CONFIG_PM_SLEEP_STATS that enables/disables showing wakeup source statistics in sysfs. Suggested-by: Greg Kroah-Hartman Signed-off-by: Tri Vo Tested-by: Tri Vo --- Documentation/ABI/testing/sysfs-power | 73 - drivers/base/power/Makefile | 1 + drivers/base/power/wakeup.c | 12 ++- drivers/base/power/wakeup_stats.c | 148 ++ include/linux/pm_wakeup.h | 19 kernel/power/Kconfig | 10 ++ kernel/power/wakelock.c | 10 ++ 7 files changed, 270 insertions(+), 3 deletions(-) create mode 100644 drivers/base/power/wakeup_stats.c diff --git a/Documentation/ABI/testing/sysfs-power b/Documentation/ABI/testing/sysfs-power index 18b7dc929234..ef92628e6fc3 100644 --- a/Documentation/ABI/testing/sysfs-power +++ b/Documentation/ABI/testing/sysfs-power @@ -300,4 +300,75 @@ Description: attempt. Using this sysfs file will override any values that were - set using the kernel command line for disk offset. \ No newline at end of file + set using the kernel command line for disk offset. + +What: /sys/class/wakeup/ +Date: June 2019 +Contact: Tri Vo +Description: + The /sys/class/wakeup/ directory contains pointers to all + wakeup sources in the kernel at that moment in time. + +What: /sys/class/wakeup/.../active_count +Date: June 2019 +Contact: Tri Vo +Description: + This file contains the number of times the wakeup source was + activated. + +What: /sys/class/wakeup/.../event_count +Date: June 2019 +Contact: Tri Vo +Description: + This file contains the number of signaled wakeup events + associated with the wakeup source. + +What: /sys/class/wakeup/.../wakeup_count +Date: June 2019 +Contact: Tri Vo +Description: + This file contains the number of times the wakeup source might + abort suspend. + +What: /sys/class/wakeup/.../expire_count +Date: June 2019 +Contact: Tri Vo +Description: + This file contains the number of times the wakeup source's + timeout has expired. + +What: /sys/class/wakeup/.../active_time_ms +Date: June 2019 +Contact: Tri Vo +Description: + This file contains the amount of time the wakeup source has + been continuously active, in milliseconds. If the wakeup + source is not active, this file contains '0'. + +What: /sys/class/wakeup/.../total_time_ms +Date: June 2019 +Contact: Tri Vo +Description: + This file contains the total amount of time this wakeup source + has been active, in milliseconds. + +What: /sys/class/wakeup/.../max_time_ms +Date: June 2019 +Contact: Tri Vo +Description: + This file contains the maximum amount of time this wakeup + source has been continuously active, in milliseconds. + +What: /sys/class/wakeup/.../last_change_ms +Date: June 2019 +Contact: Tri Vo +Description: + This file contains the monotonic clock time when the wakeup + source was touched last time, in milliseconds. + +What: /sys/class/wakeup/.../prevent_suspend_time_ms +Date: June 2019 +Contact: Tri Vo +Description: + The file contains the total amount of time this wakeup source + has been preventing autosleep, in milliseconds. diff --git a/drivers/base/power/Makefile b/drivers/base/power/Makefile index e1bb691cf8f1..1963f53d9982 100644 --- a/drivers/base/power/Makefile +++ b/drivers/base/power/Makefile @@ -1,6 +1,7 @@ # SPDX-License-Identifier: GPL-2.0 obj-$(CONFIG_PM) += sysfs.o generic_ops.o common.o qos.o runtime.o wakeirq.o obj-$(CONFIG_PM_SLEEP) += main.o wakeup.o +obj-$(CONFIG_PM_SLEEP_STATS) += wakeup_stats.o obj-$(CONFIG_PM_TRACE_RTC) += trace.o obj-$(CONFIG_PM_GENERIC_DOMAINS) += domain.o domain_governor.o obj-$(CONFIG_HAVE_CLK) += clock_ops.o diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c index 5b2b6a05a4f3..fd48e78c06b9 100644 --- a/drivers/base/power/wakeup.c +++ b/drivers/base/power/wakeup.c @@ -205,11 +205,18 @@ EXPORT_SYMBOL_GPL(wakeup_source_remove); struct wakeup_source *wakeup_source_register
Re: [PATCH v2] PM / wakeup: show wakeup sources stats in sysfs
On Thu, Jul 4, 2019 at 7:31 PM Rafael J. Wysocki 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// > > > > > > 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 > > > > 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!
Re: [PATCH] PM / wakeup: show wakeup sources stats in sysfs
On Wed, Jun 26, 2019 at 5:04 PM Greg KH wrote: > > On Wed, Jun 26, 2019 at 03:48:58PM -0700, Tri Vo wrote: > > On Tue, Jun 25, 2019 at 6:33 PM Tri Vo wrote: > > > > > > On Tue, Jun 25, 2019 at 6:12 PM Greg KH > > > wrote: > > > > > > > > On Tue, Jun 25, 2019 at 05:54:49PM -0700, Tri Vo wrote: > > > > > 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(). > > > > > > > > Ick, are you sure you need a new kobject here? Why wouldn't a named > > > > attribute group work instead? That should keep this patch much smaller > > > > and simpler. > > > > > > Yeah, named attribute groups might be a much cleaner way to do this. > > > Let me investigate. > > > > Say, we read /sys/power/wakeup_sources/foo/active_count. > > What is "foo" here? You didn't include Documentation of the sysfs > files so it was pretty impossible to say what exactly your heirachy was > going to be in order to determine this :) Sorry about that. I sent out a new version of the patch with updated documentation.
[PATCH v2] PM / wakeup: show wakeup sources stats in sysfs
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// 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 --- Documentation/ABI/testing/sysfs-power | 80 ++- drivers/base/power/Makefile | 2 +- drivers/base/power/power.h| 7 + drivers/base/power/wakeup.c | 16 ++- drivers/base/power/wakeup_stats.c | 194 ++ include/linux/pm_wakeup.h | 15 ++ kernel/power/Kconfig | 10 ++ kernel/power/wakelock.c | 42 -- 8 files changed, 347 insertions(+), 19 deletions(-) create mode 100644 drivers/base/power/wakeup_stats.c v2: - Updated Documentation/ABI/, as per Greg. - Removed locks in attribute functions, as per Greg. - Lifetimes of struct wakelock and struck wakeup_source are now different due to the latter embedding a refcounted kobject. Changed it so that struct wakelock only has a pointer to struct wakeup_source, instead of embedding it. - Added CONFIG_PM_SLEEP_STATS that enables/disables wakeup source statistics in sysfs. diff --git a/Documentation/ABI/testing/sysfs-power b/Documentation/ABI/testing/sysfs-power index 18b7dc929234..ba35f1eef73b 100644 --- a/Documentation/ABI/testing/sysfs-power +++ b/Documentation/ABI/testing/sysfs-power @@ -300,4 +300,82 @@ Description: attempt. Using this sysfs file will override any values that were - set using the kernel command line for disk offset. \ No newline at end of file + set using the kernel command line for disk offset. + +What: /sys/power/wakeup_sources/.../ +Date: June 2019 +Contact: Tri Vo +Description: + The /sys/power/wakeup_sources/.../ directory contains attributes + that expose statistics about a given wakeup source to user + space. Attributes in this directory are read-only. + +What: /sys/power/wakeup_sources/.../active_count +Date: June 2019 +Contact: Tri Vo +Description: + The /sys/power/wakeup_sources/.../active_count file contains the + number of times the wakeup source was activated. + +What: /sys/power/wakeup_sources/.../event_count +Date: June 2019 +Contact: Tri Vo +Description: + The /sys/power/wakeup_sources/.../event_count file contains the + number of signaled wakeup events associated with the wakeup + source. + +What: /sys/power/wakeup_sources/.../wakeup_count +Date: June 2019 +Contact: Tri Vo +Description: + The /sys/power/wakeup_sources/.../wakeup_count file contains the + number of times the wakeup source might abort suspend. + +What: /sys/power/wakeup_sources/.../expire_count +Date: June 2019 +Contact: Tri Vo +Description: + The /sys/power/wakeup_sources/.../expire_count file contains the + number of times the wakeup source's timeout has expired. + +What: /sys/power/wakeup_sources/.../active_time_ms +Date: June 2019 +Contact: Tri Vo +Description: + The /sys/power/wakeup_sources/.../active_time_ms file contains + the amount of time the wakeup source has been continuously + active, in milliseconds. If the wakeup source is not active, + this file contains '0'. + +What: /sys/power/wakeup_sources/.../total_time_ms +Date: June 2019 +Contact: Tri Vo +Description: + The /sys/power/wakeup_sources/.../total_time_ms file contains + the total amount of time this wakeup source has been active, in + milliseconds. + +What: /sys/power/wakeup_sources/.../max_time_ms +Date: June 2019 +Contact: Tri Vo +Description: + The /sys/power/wakeup_sources/.../max_time_ms file contains the + maximum amount of time this wakeup source has been continuously + active, in milliseconds. + +What: /sys/power/wakeup_sources/.../last_change_ms +Date: June 2019 +Contact: Tri Vo +Description
Re: [PATCH] PM / wakeup: show wakeup sources stats in sysfs
On Tue, Jun 25, 2019 at 6:33 PM Tri Vo wrote: > > On Tue, Jun 25, 2019 at 6:12 PM Greg KH wrote: > > > > On Tue, Jun 25, 2019 at 05:54:49PM -0700, Tri Vo wrote: > > > 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(). > > > > Ick, are you sure you need a new kobject here? Why wouldn't a named > > attribute group work instead? That should keep this patch much smaller > > and simpler. > > Yeah, named attribute groups might be a much cleaner way to do this. > Let me investigate. Say, we read /sys/power/wakeup_sources/foo/active_count. This attribute's show function needs to find wakeup_source struct of "foo". I'm not sure how to do that without embedding a kobject inside of wakeup_source.
Re: [PATCH] PM / wakeup: show wakeup sources stats in sysfs
On Tue, Jun 25, 2019 at 6:48 PM Greg KH wrote:
>
> On Tue, Jun 25, 2019 at 06:33:08PM -0700, Tri Vo wrote:
> > On Tue, Jun 25, 2019 at 6:12 PM Greg KH wrote:
> > > > +static ssize_t wakeup_source_count_show(struct wakeup_source *ws,
> > > > + struct wakeup_source_attribute
> > > > *attr,
> > > > + char *buf)
> > > > +{
> > > > + unsigned long flags;
> > > > + unsigned long var;
> > > > +
> > > > + spin_lock_irqsave(>lock, flags);
> > > > + if (strcmp(attr->attr.name, "active_count") == 0)
> > > > + var = ws->active_count;
> > > > + else if (strcmp(attr->attr.name, "event_count") == 0)
> > > > + var = ws->event_count;
> > > > + else if (strcmp(attr->attr.name, "wakeup_count") == 0)
> > > > + var = ws->wakeup_count;
> > > > + else
> > > > + var = ws->expire_count;
> > > > + spin_unlock_irqrestore(>lock, flags);
> > > > +
> > > > + return sprintf(buf, "%lu\n", var);
> > > > +}
> > >
> > > Why is this lock always needed to be grabbed? You are just reading a
> > > value, who cares if it changes inbetween reading it and returning the
> > > buffer string as it can change at that point in time anyway?
> >
> > Right, we don't care if the value changes in between us reading and
> > printing it. However, IIUC not grabbing this lock results in a data
> > race, which is undefined behavior.
>
> A data race where? Writing to the value? How can that happen? All you
> are doing is incrementing this variable elsewhere, what is the worst
> that can happen?
Ok, I'll remove the locks.
Re: [PATCH] PM / wakeup: show wakeup sources stats in sysfs
On Tue, Jun 25, 2019 at 6:12 PM Greg KH wrote:
>
> On Tue, Jun 25, 2019 at 05:54:49PM -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//
> >
> > Add following attributes to each wakeup source. These attributes match
> > the columns of /sys/kernel/debug/wakeup_sources.
> >
> > active_count
> > event_count
> > wakeup_count
> > expire_count
> > active_time (named "active_since" in debugfs)
> > total_time
> > max_time
> > last_change
> > prevent_suspend_time
>
> Can you also add a Documentation/ABI/ update for your new sysfs files so
> that we can properly review this?
>
> > 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().
Will do.
>
> Ick, are you sure you need a new kobject here? Why wouldn't a named
> attribute group work instead? That should keep this patch much smaller
> and simpler.
Yeah, named attribute groups might be a much cleaner way to do this.
Let me investigate.
>
> > +static ssize_t wakeup_source_count_show(struct wakeup_source *ws,
> > + struct wakeup_source_attribute *attr,
> > + char *buf)
> > +{
> > + unsigned long flags;
> > + unsigned long var;
> > +
> > + spin_lock_irqsave(>lock, flags);
> > + if (strcmp(attr->attr.name, "active_count") == 0)
> > + var = ws->active_count;
> > + else if (strcmp(attr->attr.name, "event_count") == 0)
> > + var = ws->event_count;
> > + else if (strcmp(attr->attr.name, "wakeup_count") == 0)
> > + var = ws->wakeup_count;
> > + else
> > + var = ws->expire_count;
> > + spin_unlock_irqrestore(>lock, flags);
> > +
> > + return sprintf(buf, "%lu\n", var);
> > +}
>
> Why is this lock always needed to be grabbed? You are just reading a
> value, who cares if it changes inbetween reading it and returning the
> buffer string as it can change at that point in time anyway?
Right, we don't care if the value changes in between us reading and
printing it. However, IIUC not grabbing this lock results in a data
race, which is undefined behavior.
Re: [PATCH] PM / wakeup: show wakeup sources stats in sysfs
On Tue, Jun 25, 2019 at 5:55 PM 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//
>
> Add following attributes to each wakeup source. These attributes match
> the columns of /sys/kernel/debug/wakeup_sources.
>
> active_count
> event_count
> wakeup_count
> expire_count
> active_time (named "active_since" in debugfs)
> total_time
> max_time
> last_change
> prevent_suspend_time
>
> 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().
>
> Signed-off-by: Tri Vo
> ---
> drivers/base/power/Makefile | 2 +-
> drivers/base/power/wakeup.c | 13 +-
> drivers/base/power/wakeup_sysfs.c | 204 ++
> include/linux/pm_wakeup.h | 9 ++
> kernel/power/wakelock.c | 13 +-
> 5 files changed, 236 insertions(+), 5 deletions(-)
> create mode 100644 drivers/base/power/wakeup_sysfs.c
>
> diff --git a/drivers/base/power/Makefile b/drivers/base/power/Makefile
> index e1bb691cf8f1..e6ea9eb46275 100644
> --- a/drivers/base/power/Makefile
> +++ b/drivers/base/power/Makefile
> @@ -1,6 +1,6 @@
> # SPDX-License-Identifier: GPL-2.0
> obj-$(CONFIG_PM) += sysfs.o generic_ops.o common.o qos.o runtime.o
> wakeirq.o
> -obj-$(CONFIG_PM_SLEEP) += main.o wakeup.o
> +obj-$(CONFIG_PM_SLEEP) += main.o wakeup.o wakeup_sysfs.o
> obj-$(CONFIG_PM_TRACE_RTC) += trace.o
> obj-$(CONFIG_PM_GENERIC_DOMAINS) += domain.o domain_governor.o
> obj-$(CONFIG_HAVE_CLK) += clock_ops.o
> diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
> index 5b2b6a05a4f3..6fcbb4d2045f 100644
> --- a/drivers/base/power/wakeup.c
> +++ b/drivers/base/power/wakeup.c
> @@ -96,12 +96,22 @@ EXPORT_SYMBOL_GPL(wakeup_source_prepare);
> struct wakeup_source *wakeup_source_create(const char *name)
> {
> struct wakeup_source *ws;
> + int ret;
>
> ws = kmalloc(sizeof(*ws), GFP_KERNEL);
> if (!ws)
> return NULL;
>
> wakeup_source_prepare(ws, name ? kstrdup_const(name, GFP_KERNEL) :
> NULL);
> +
> + ws->kobj.kset = wakeup_source_kset;
> + ret = kobject_init_and_add(>kobj, _source_ktype, NULL,
> "%s",
> + ws->name);
> + if (ret) {
> + kobject_put(>kobj);
> + return NULL;
> + }
> +
> return ws;
> }
> EXPORT_SYMBOL_GPL(wakeup_source_create);
> @@ -147,8 +157,7 @@ void wakeup_source_destroy(struct wakeup_source *ws)
>
> __pm_relax(ws);
> wakeup_source_record(ws);
> - kfree_const(ws->name);
> - kfree(ws);
> + kobject_put(>kobj);
> }
> EXPORT_SYMBOL_GPL(wakeup_source_destroy);
>
> diff --git a/drivers/base/power/wakeup_sysfs.c
> b/drivers/base/power/wakeup_sysfs.c
> new file mode 100644
> index ..d872afc645d9
> --- /dev/null
> +++ b/drivers/base/power/wakeup_sysfs.c
> @@ -0,0 +1,204 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include
> +
> +#include "power.h"
> +
> +struct wakeup_source_attribute {
> + struct attribute attr;
> + ssize_t (*show)(struct wakeup_source *ws,
> + struct wakeup_source_attribute *attr, char *buf);
> + ssize_t (*store)(struct wakeup_source *ws,
> +struct wakeup_source_attribute *attr, const char
> *buf,
> +size_t count);
> +};
> +
> +#define to_wakeup_source_obj(x) container_of(x, struct wakeup_source, kobj)
> +#define to_wakeup_source_attr(x) \
> + container_of(x, struct wakeup_source_attribute, attr)
> +
> +static ssize_t wakeup_source_attr_show(struct kobject *kobj,
> + struct attribute *attr,
> + char *buf)
> +{
> + struct wakeup_source_attribute *attribute;
> + struct wakeup_source *ws;
> +
> + ws = to_wakeup_source_obj(kobj);
> + attribute = to_wakeup_source_attr(attr);
> +
> + if (!attribute->show)
> + return -EIO;
> +
> + ret
[PATCH] PM / wakeup: show wakeup sources stats in sysfs
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//
Add following attributes to each wakeup source. These attributes match
the columns of /sys/kernel/debug/wakeup_sources.
active_count
event_count
wakeup_count
expire_count
active_time (named "active_since" in debugfs)
total_time
max_time
last_change
prevent_suspend_time
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().
Signed-off-by: Tri Vo
---
drivers/base/power/Makefile | 2 +-
drivers/base/power/wakeup.c | 13 +-
drivers/base/power/wakeup_sysfs.c | 204 ++
include/linux/pm_wakeup.h | 9 ++
kernel/power/wakelock.c | 13 +-
5 files changed, 236 insertions(+), 5 deletions(-)
create mode 100644 drivers/base/power/wakeup_sysfs.c
diff --git a/drivers/base/power/Makefile b/drivers/base/power/Makefile
index e1bb691cf8f1..e6ea9eb46275 100644
--- a/drivers/base/power/Makefile
+++ b/drivers/base/power/Makefile
@@ -1,6 +1,6 @@
# SPDX-License-Identifier: GPL-2.0
obj-$(CONFIG_PM) += sysfs.o generic_ops.o common.o qos.o runtime.o
wakeirq.o
-obj-$(CONFIG_PM_SLEEP) += main.o wakeup.o
+obj-$(CONFIG_PM_SLEEP) += main.o wakeup.o wakeup_sysfs.o
obj-$(CONFIG_PM_TRACE_RTC) += trace.o
obj-$(CONFIG_PM_GENERIC_DOMAINS) += domain.o domain_governor.o
obj-$(CONFIG_HAVE_CLK) += clock_ops.o
diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
index 5b2b6a05a4f3..6fcbb4d2045f 100644
--- a/drivers/base/power/wakeup.c
+++ b/drivers/base/power/wakeup.c
@@ -96,12 +96,22 @@ EXPORT_SYMBOL_GPL(wakeup_source_prepare);
struct wakeup_source *wakeup_source_create(const char *name)
{
struct wakeup_source *ws;
+ int ret;
ws = kmalloc(sizeof(*ws), GFP_KERNEL);
if (!ws)
return NULL;
wakeup_source_prepare(ws, name ? kstrdup_const(name, GFP_KERNEL) :
NULL);
+
+ ws->kobj.kset = wakeup_source_kset;
+ ret = kobject_init_and_add(>kobj, _source_ktype, NULL, "%s",
+ ws->name);
+ if (ret) {
+ kobject_put(>kobj);
+ return NULL;
+ }
+
return ws;
}
EXPORT_SYMBOL_GPL(wakeup_source_create);
@@ -147,8 +157,7 @@ void wakeup_source_destroy(struct wakeup_source *ws)
__pm_relax(ws);
wakeup_source_record(ws);
- kfree_const(ws->name);
- kfree(ws);
+ kobject_put(>kobj);
}
EXPORT_SYMBOL_GPL(wakeup_source_destroy);
diff --git a/drivers/base/power/wakeup_sysfs.c
b/drivers/base/power/wakeup_sysfs.c
new file mode 100644
index ..d872afc645d9
--- /dev/null
+++ b/drivers/base/power/wakeup_sysfs.c
@@ -0,0 +1,204 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include
+
+#include "power.h"
+
+struct wakeup_source_attribute {
+ struct attribute attr;
+ ssize_t (*show)(struct wakeup_source *ws,
+ struct wakeup_source_attribute *attr, char *buf);
+ ssize_t (*store)(struct wakeup_source *ws,
+struct wakeup_source_attribute *attr, const char *buf,
+size_t count);
+};
+
+#define to_wakeup_source_obj(x) container_of(x, struct wakeup_source, kobj)
+#define to_wakeup_source_attr(x) \
+ container_of(x, struct wakeup_source_attribute, attr)
+
+static ssize_t wakeup_source_attr_show(struct kobject *kobj,
+ struct attribute *attr,
+ char *buf)
+{
+ struct wakeup_source_attribute *attribute;
+ struct wakeup_source *ws;
+
+ ws = to_wakeup_source_obj(kobj);
+ attribute = to_wakeup_source_attr(attr);
+
+ if (!attribute->show)
+ return -EIO;
+
+ return attribute->show(ws, attribute, buf);
+}
+
+static const struct sysfs_ops wakeup_source_sysfs_ops = {
+ .show = wakeup_source_attr_show,
+};
+
+static void wakeup_source_release(struct kobject *kobj)
+{
+ struct wakeup_source *ws;
+
+ ws = to_wakeup_source_obj(kobj);
+ kfree_const(ws->name);
+ kfree(ws);
+}
+
+static ssize_t wakeup_source_count_show(struct wakeup_source *ws,
+ struct wakeup_source_attribute *attr,
+ char *buf)
+{
+ unsigned long flags;
+ unsigned long var;
+
+ spin_lock_irqsave(>lock, flags);
+ if (strcmp(attr->attr.name, "active_count") == 0)
+
Re: Alternatives to /sys/kernel/debug/wakeup_sources
On Mon, Jun 24, 2019 at 2:55 PM Rafael J. Wysocki wrote: > > On Mon, Jun 24, 2019 at 2:27 PM Joel Fernandes wrote: > > > > On Mon, Jun 24, 2019 at 3:37 AM Greg Kroah-Hartman > > wrote: > > > > > > On Sun, Jun 23, 2019 at 06:48:43PM -0700, Tri Vo wrote: > > > > On Wed, Jun 19, 2019 at 1:35 AM Rafael J. Wysocki > > > > wrote: > > > > > > > > > > On Wed, Jun 19, 2019 at 1:52 AM Joel Fernandes > > > > > wrote: > > > > > > > > > > > > On Tue, Jun 18, 2019 at 7:15 PM Tri Vo wrote: > > > > > > [snip] > > > > > > > > > > > > > > > > > > > > > > Android userspace reading wakeup_sources is not ideal > > > > > > > > > > > because: > > > > > > > > > > > - Debugfs API is not stable, i.e. Android tools built on > > > > > > > > > > > top of it are > > > > > > > > > > > not guaranteed to be backward/forward compatible. > > > > > > > > > > > - This file requires debugfs to be mounted, which itself > > > > > > > > > > > is > > > > > > > > > > > undesirable for security reasons. > > > > > > > > > > > > > > > > > > > > > > To address these problems, we want to contribute a way to > > > > > > > > > > > expose these > > > > > > > > > > > statistics that doesn't depend on debugfs. > > > > > > > > > > > > > > > > > > > > > > Some initial thoughts/questions: Should we expose the > > > > > > > > > > > stats in sysfs? > > > > > > > > > > > Or maybe implement eBPF-based solution? What do you think? > > > > > > > > > > > > > > > > > > We are going through Android's out-of-tree kernel > > > > > > > > > dependencies along with > > > > > > > > > userspace APIs that are not necessarily considered "stable > > > > > > > > > and forever > > > > > > > > > supported" upstream. The debugfs dependencies showed up on > > > > > > > > > our radar as a > > > > > > > > > result and so we are wondering if we should worry about > > > > > > > > > changes in debugfs > > > > > > > > > interface and hence the question(s) below. > > > > > > > > > > > > > > > > > > So, can we rely on /d/wakeup_sources to be considered a > > > > > > > > > userspace API and > > > > > > > > > hence maintained stable as we do for other /proc and /sys > > > > > > > > > entries? > > > > > > > > > > > > > > > > > > If yes, then we will go ahead and add tests for this in LTP or > > > > > > > > > somewhere else suitable. > > > > > > > > > > > > > > > > No, debugfs is not ABI. > > > > > > > > > > > > > > > > > If no, then we would love to hear suggestions for any changes > > > > > > > > > that need to be > > > > > > > > > made or we simply just move the debugfs entry into somewhere > > > > > > > > > like > > > > > > > > > /sys/power/ ? > > > > > > > > > > > > > > > > No, moving that entire file from debugfs into sysfs is not an > > > > > > > > option either. > > > > > > > > > > > > > > > > The statistics for the wakeup sources associated with devices > > > > > > > > are already there > > > > > > > > under /sys/devices/.../power/ , but I guess you want all wakeup > > > > > > > > sources? > > > > > > > > > > > > > > > > That would require adding a kobject to struct wakeup_source and > > > > > > > > exposing > > > > > > > > all of the statistics as separate attributes under it. In > > > > > > > > which case it would be > > > > > > >
Re: Alternatives to /sys/kernel/debug/wakeup_sources
On Wed, Jun 19, 2019 at 1:35 AM Rafael J. Wysocki wrote: > > On Wed, Jun 19, 2019 at 1:52 AM Joel Fernandes wrote: > > > > On Tue, Jun 18, 2019 at 7:15 PM Tri Vo wrote: > > [snip] > > > > > > > > > > > > > > Android userspace reading wakeup_sources is not ideal because: > > > > > > > - Debugfs API is not stable, i.e. Android tools built on top of > > > > > > > it are > > > > > > > not guaranteed to be backward/forward compatible. > > > > > > > - This file requires debugfs to be mounted, which itself is > > > > > > > undesirable for security reasons. > > > > > > > > > > > > > > To address these problems, we want to contribute a way to expose > > > > > > > these > > > > > > > statistics that doesn't depend on debugfs. > > > > > > > > > > > > > > Some initial thoughts/questions: Should we expose the stats in > > > > > > > sysfs? > > > > > > > Or maybe implement eBPF-based solution? What do you think? > > > > > > > > > > We are going through Android's out-of-tree kernel dependencies along > > > > > with > > > > > userspace APIs that are not necessarily considered "stable and forever > > > > > supported" upstream. The debugfs dependencies showed up on our radar > > > > > as a > > > > > result and so we are wondering if we should worry about changes in > > > > > debugfs > > > > > interface and hence the question(s) below. > > > > > > > > > > So, can we rely on /d/wakeup_sources to be considered a userspace API > > > > > and > > > > > hence maintained stable as we do for other /proc and /sys entries? > > > > > > > > > > If yes, then we will go ahead and add tests for this in LTP or > > > > > somewhere else suitable. > > > > > > > > No, debugfs is not ABI. > > > > > > > > > If no, then we would love to hear suggestions for any changes that > > > > > need to be > > > > > made or we simply just move the debugfs entry into somewhere like > > > > > /sys/power/ ? > > > > > > > > No, moving that entire file from debugfs into sysfs is not an option > > > > either. > > > > > > > > The statistics for the wakeup sources associated with devices are > > > > already there > > > > under /sys/devices/.../power/ , but I guess you want all wakeup sources? > > > > > > > > That would require adding a kobject to struct wakeup_source and exposing > > > > all of the statistics as separate attributes under it. In which case > > > > it would be > > > > good to replace the existing wakeup statistics under > > > > /sys/devices/.../power/ > > > > with symbolic links to the attributes under the wakeup_source kobject. > > > > > > Thanks for your input, Rafael! Your suggestion makes sense. I'll work > > > on a patch for this. > > > > Does that entail making each wake up source, a new sysfs node under a > > particular device, and then adding stats under that new node? > > Not under a device, because there are wakeup source objects without > associated devices. > > It is conceivable to have a "wakeup_sources" directory under > /sys/power/ and sysfs nodes for all wakeup sources in there. > > Then, instead of exposing wakeup statistics directly under > /sys/devices/.../power/, there can be symbolic links from there to the > new wakeup source nodes under "wakeup_sources" (so as to avoid > exposing the same data in two different places in sysfs, which may be > confusing). This may be a dumb question. Is it appropriate to make symbolic links in sysfs from one attribute to another attribute? For example, /sys/devices/.../power/wakeup_count -> /sys/power/wakeup_sources/.../wakeup_count. I only see kobject to kobject symlinks around. And I don't think we can make /sys/devices/.../power/ directory a symlink to where our new wakeup stats will be, since the former contains attributes other than wakeup ones. Thanks!
Re: Alternatives to /sys/kernel/debug/wakeup_sources
On Wed, Jun 19, 2019 at 11:01 AM Joel Fernandes wrote: > > On Wed, Jun 19, 2019 at 1:07 PM Greg Kroah-Hartman > wrote: > > > > On Wed, Jun 19, 2019 at 12:53:12PM -0400, Joel Fernandes wrote: > > > > It is conceivable to have a "wakeup_sources" directory under > > > > /sys/power/ and sysfs nodes for all wakeup sources in there. > > > > > > One of the "issues" with this is, now if you have say 100 wake up > > > sources, with 10 entries each, then we're talking about a 1000 sysfs > > > files. Each one has to be opened, and read individually. This adds > > > overhead and it is more convenient to read from a single file. The > > > problem is this single file is not ABI. So the question I guess is, > > > how do we solve this in both an ABI friendly way while keeping the > > > overhead low. > > > > How much overhead? Have you measured it, reading from virtual files is > > fast :) > > I measured, and it is definitely not free. If you create and read a > 1000 files and just return a string back, it can take up to 11-13 > milliseconds (did not lock CPU frequencies, was just looking for > average ball park). This is assuming that the counter reading is just > doing that, and nothing else is being done to return the sysfs data > which is probably not always true in practice. > > Our display pipeline deadline is around 16ms at 60Hz. Conceivably, any > CPU scheduling competion reading sysfs can hurt the deadline. There's > also the question of power - we definitely have spent time in the past > optimizing other virtual files such as /proc/pid/smaps for this reason > where it spent lots of CPU time. > > > And how often does this happen? Does it _need_ to happen? > > These are good questions and we should find out. I am not saying that > sysfs will not work, I am just saying we need to consider all the > things. I will let Tri look into this since he is working on it, I > don't know off the top. There are really two use cases for wakeup_sources in Android: (1) As Sandeep pointed out, it's used to plot history of wakeup sources. This use case might actually be performance sensitive. I'll check with our internal Android teams and loop back here. (2) wakeup_sources information is included in bugreports. Reading/parsing wakeup_sources in this case only happens when generating a bugreport, so not particularly sensitive to overhead.
Re: Alternatives to /sys/kernel/debug/wakeup_sources
On Tue, Jun 18, 2019 at 2:23 PM Rafael J. Wysocki wrote: > > On Tuesday, June 18, 2019 10:17:16 PM CEST Sandeep Patil wrote: > > > > Hi Rafael, Viresh etc. > > > > On Tue, Jun 11, 2019 at 10:31:16AM -0700, Tri Vo wrote: > > > On Tue, Jun 4, 2019 at 5:23 PM Tri Vo wrote: > > > > > > > > Hello Rafael, > > > > > > > > Currently, Android reads wakeup sources statistics from > > > > /sys/kernel/debug/wakeup_sources in production environment. This > > > > information is used, for example, to report which wake lock prevents > > > > the device from suspending. > > > > Android's usage of the 'wakeup_sources' from debugfs can is linked at[1]. > > Basically, android's battery stats implementation to plot history for > > suspend > > blocking wakeup sources over device's boot cycle. This is used both for > > power > > specific bug reporting but also is one of the stats that will be used > > towards > > attributing the battery consumption to specific processes over the period of > > time. > > > > Android depended on the out-of-tree /proc/wakelocks before and now relies on > > wakeup_sources debugfs entry heavily for the aforementioned use cases. > > > > > > > > > > Android userspace reading wakeup_sources is not ideal because: > > > > - Debugfs API is not stable, i.e. Android tools built on top of it are > > > > not guaranteed to be backward/forward compatible. > > > > - This file requires debugfs to be mounted, which itself is > > > > undesirable for security reasons. > > > > > > > > To address these problems, we want to contribute a way to expose these > > > > statistics that doesn't depend on debugfs. > > > > > > > > Some initial thoughts/questions: Should we expose the stats in sysfs? > > > > Or maybe implement eBPF-based solution? What do you think? > > > > We are going through Android's out-of-tree kernel dependencies along with > > userspace APIs that are not necessarily considered "stable and forever > > supported" upstream. The debugfs dependencies showed up on our radar as a > > result and so we are wondering if we should worry about changes in debugfs > > interface and hence the question(s) below. > > > > So, can we rely on /d/wakeup_sources to be considered a userspace API and > > hence maintained stable as we do for other /proc and /sys entries? > > > > If yes, then we will go ahead and add tests for this in LTP or > > somewhere else suitable. > > No, debugfs is not ABI. > > > If no, then we would love to hear suggestions for any changes that need to > > be > > made or we simply just move the debugfs entry into somewhere like > > /sys/power/ ? > > No, moving that entire file from debugfs into sysfs is not an option either. > > The statistics for the wakeup sources associated with devices are already > there > under /sys/devices/.../power/ , but I guess you want all wakeup sources? > > That would require adding a kobject to struct wakeup_source and exposing > all of the statistics as separate attributes under it. In which case it > would be > good to replace the existing wakeup statistics under /sys/devices/.../power/ > with symbolic links to the attributes under the wakeup_source kobject. Thanks for your input, Rafael! Your suggestion makes sense. I'll work on a patch for this. > > > As a side effect, if the entry moves out of debugfs, Android can run without > > mounting debugfs in production that I assume is a good thing. > > And really Android developers might have thought about this a bit earlier. I'm still learning about kernel development. And Android has made missteps before. So I figured it's a good idea to ask first :) Thanks!
[PATCH] ARM: disable FUNCTION_TRACER when building with Clang
Clang needs "-meabi gnu" flag to emit calls to "__gnu_mcount_nc". Otherwise, it inserts calls to undefined "mcount". kernel/softirq.o: In function `_local_bh_enable': ... undefined reference to `mcount' "-meabi gnu" resolves link failures. However, Clang does not implement calls to "__gnu_mcount_nc" correctly. It does not save the link register on the stack, which corrupts the stack. The resulting kernel does not boot. Disable FUNCTION_TRACER support when building with Clang. Link: https://github.com/ClangBuiltLinux/linux/issues/35 Suggested-by: Stefan Agner Signed-off-by: Tri Vo --- arch/arm/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig index 8869742a85df..7a1804392795 100644 --- a/arch/arm/Kconfig +++ b/arch/arm/Kconfig @@ -75,7 +75,7 @@ config ARM select HAVE_EXIT_THREAD select HAVE_FTRACE_MCOUNT_RECORD if !XIP_KERNEL select HAVE_FUNCTION_GRAPH_TRACER if !THUMB2_KERNEL && !CC_IS_CLANG - select HAVE_FUNCTION_TRACER if !XIP_KERNEL + select HAVE_FUNCTION_TRACER if !XIP_KERNEL && !CC_IS_CLANG select HAVE_GCC_PLUGINS select HAVE_HW_BREAKPOINT if PERF_EVENTS && (CPU_V6 || CPU_V6K || CPU_V7) select HAVE_IDE if PCI || ISA || PCMCIA -- 2.21.0.1020.gf2820cf01a-goog
Re: [PATCH v5] gcov: Clang support
On Wed, Apr 17, 2019 at 4:21 PM Andrew Morton wrote:
>
> On Wed, 17 Apr 2019 15:53:28 -0700 Tri Vo wrote:
>
> > LLVM uses profiling data that's deliberately similar to GCC, but has a very
> > different way of exporting that data. LLVM calls llvm_gcov_init() once per
> > module, and provides a couple of callbacks that we can use to ask for more
> > data.
> >
> > We care about the "writeout" callback, which in turn calls back into
> > compiler-rt/this module to dump all the gathered coverage data to disk:
> >
> >llvm_gcda_start_file()
> > llvm_gcda_emit_function()
> > llvm_gcda_emit_arcs()
> > llvm_gcda_emit_function()
> > llvm_gcda_emit_arcs()
> > [... repeats for each function ...]
> >llvm_gcda_summary_info()
> >llvm_gcda_end_file()
> >
> > This design is much more stateless and unstructured than gcc's, and is
> > intended to run at process exit. This forces us to keep some local state
> > about which module we're dealing with at the moment. On the other hand, it
> > also means we don't depend as much on how LLVM represents profiling data
> > internally.
> >
> > See LLVM's lib/Transforms/Instrumentation/GCOVProfiling.cpp for more
> > details on how this works, particularly GCOVProfiler::emitProfileArcs(),
> > GCOVProfiler::insertCounterWriteout(), and GCOVProfiler::insertFlush().
>
> checkpatch speaketh truth.
>
> Also, I'll change those Co-authored-by's to the documented Co-developed-by.
>
>
> From: Andrew Morton
> Subject: gcov-clang-support-checkpatch-fixes
>
> WARNING: Non-standard signature: Co-authored-by:
> #31:
> Co-authored-by: Nick Desaulniers
>
> WARNING: Non-standard signature: Co-authored-by:
> #32:
> Co-authored-by: Tri Vo
>
> WARNING: Possible unnecessary 'out of memory' message
> #158: FILE: kernel/gcov/clang.c:90:
> + if (!info) {
> + pr_warn_ratelimited("failed to allocate gcov info\n");
>
> WARNING: Possible unnecessary 'out of memory' message
> #193: FILE: kernel/gcov/clang.c:125:
> + if (!info) {
> + pr_warn_ratelimited("failed to allocate gcov function info
> for %s\n",
>
> WARNING: line over 80 characters
> #546: FILE: kernel/gcov/clang.c:478:
> + pos += store_gcov_u32(buffer, pos,
> fi_ptr->cfg_checksum);
>
> total: 0 errors, 5 warnings, 663 lines checked
>
> NOTE: For some of the reported defects, checkpatch may be able to
> mechanically convert to the typical style using --fix or --fix-inplace.
>
> ./patches/gcov-clang-support.patch has style problems, please review.
>
> NOTE: If any of the errors are false positives, please report
> them to the maintainer, see CHECKPATCH in MAINTAINERS.
>
> Please run checkpatch prior to sending patches
Thanks! I'll keep that in mind.
[PATCH v5] gcov: Clang support
From: Greg Hackmann
LLVM uses profiling data that's deliberately similar to GCC, but has a very
different way of exporting that data. LLVM calls llvm_gcov_init() once per
module, and provides a couple of callbacks that we can use to ask for more
data.
We care about the "writeout" callback, which in turn calls back into
compiler-rt/this module to dump all the gathered coverage data to disk:
llvm_gcda_start_file()
llvm_gcda_emit_function()
llvm_gcda_emit_arcs()
llvm_gcda_emit_function()
llvm_gcda_emit_arcs()
[... repeats for each function ...]
llvm_gcda_summary_info()
llvm_gcda_end_file()
This design is much more stateless and unstructured than gcc's, and is
intended to run at process exit. This forces us to keep some local state
about which module we're dealing with at the moment. On the other hand, it
also means we don't depend as much on how LLVM represents profiling data
internally.
See LLVM's lib/Transforms/Instrumentation/GCOVProfiling.cpp for more
details on how this works, particularly GCOVProfiler::emitProfileArcs(),
GCOVProfiler::insertCounterWriteout(), and GCOVProfiler::insertFlush().
Co-authored-by: Nick Desaulniers
Co-authored-by: Tri Vo
Signed-off-by: Greg Hackmann
Signed-off-by: Nick Desaulniers
Signed-off-by: Tri Vo
Tested-by: Trilok Soni
Tested-by: Prasad Sodagudi
Tested-by: Tri Vo
Tested-by: Daniel Mentz
Tested-by: Petri Gynther
Reviewed-by: Peter Oberparleiter
---
v5:
- Same as v4. Build error when !CONFIG_MODULES was fixed by another commit
41e72eeff32c ("module: add stubs for within_module functions")
kernel/gcov/Kconfig | 3 +-
kernel/gcov/Makefile | 1 +
kernel/gcov/base.c| 2 +-
kernel/gcov/clang.c | 586 ++
kernel/gcov/gcc_3_4.c | 12 +
kernel/gcov/gcc_4_7.c | 12 +
kernel/gcov/gcov.h| 2 +
7 files changed, 616 insertions(+), 2 deletions(-)
create mode 100644 kernel/gcov/clang.c
diff --git a/kernel/gcov/Kconfig b/kernel/gcov/Kconfig
index 1e3823fa799b..f71c1adcff31 100644
--- a/kernel/gcov/Kconfig
+++ b/kernel/gcov/Kconfig
@@ -53,6 +53,7 @@ config GCOV_PROFILE_ALL
choice
prompt "Specify GCOV format"
depends on GCOV_KERNEL
+ depends on CC_IS_GCC
---help---
The gcov format is usually determined by the GCC version, and the
default is chosen according to your GCC version. However, there are
@@ -62,7 +63,7 @@ choice
config GCOV_FORMAT_3_4
bool "GCC 3.4 format"
- depends on CC_IS_GCC && GCC_VERSION < 40700
+ depends on GCC_VERSION < 40700
---help---
Select this option to use the format defined by GCC 3.4.
diff --git a/kernel/gcov/Makefile b/kernel/gcov/Makefile
index 45431ed679d1..d66a74b0f100 100644
--- a/kernel/gcov/Makefile
+++ b/kernel/gcov/Makefile
@@ -4,3 +4,4 @@ ccflags-y := -DSRCTREE='"$(srctree)"' -DOBJTREE='"$(objtree)"'
obj-y := base.o fs.o
obj-$(CONFIG_GCOV_FORMAT_3_4) += gcc_base.o gcc_3_4.o
obj-$(CONFIG_GCOV_FORMAT_4_7) += gcc_base.o gcc_4_7.o
+obj-$(CONFIG_CC_IS_CLANG) += clang.o
diff --git a/kernel/gcov/base.c b/kernel/gcov/base.c
index 799d42072727..0ffe9f194080 100644
--- a/kernel/gcov/base.c
+++ b/kernel/gcov/base.c
@@ -64,7 +64,7 @@ static int gcov_module_notifier(struct notifier_block *nb,
unsigned long event,
/* Remove entries located in module from linked list. */
while ((info = gcov_info_next(info))) {
- if (within_module((unsigned long)info, mod)) {
+ if (gcov_info_within_module(info, mod)) {
gcov_info_unlink(prev, info);
if (gcov_events_enabled)
gcov_event(GCOV_REMOVE, info);
diff --git a/kernel/gcov/clang.c b/kernel/gcov/clang.c
new file mode 100644
index ..125c50397ba2
--- /dev/null
+++ b/kernel/gcov/clang.c
@@ -0,0 +1,586 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2019 Google, Inc.
+ * modified from kernel/gcov/gcc_4_7.c
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ *
+ * LLVM uses profiling data that's deliberately similar to GCC, but has a
+ * very different way of exporting that data. LLVM calls llvm_gcov_init() once
+ * per module, and provides a couple of callbacks that we can use to ask for
+ * more data.
+ *
+ * We care about the "writeout" callback, which in turn calls back into
+ * compiler-rt/this module to dump all the gathered coverage data to disk:
+ *
+ *ll
Re: [PATCH v2] module: add stubs for within_module functions
On Tue, Apr 16, 2019 at 10:55 AM Tri Vo wrote:
>
> On Tue, Apr 16, 2019 at 8:21 AM Jessica Yu wrote:
> >
> > +++ Tri Vo [15/04/19 11:18 -0700]:
> > >Provide stubs for within_module_core(), within_module_init(), and
> > >within_module() to prevent build errors when !CONFIG_MODULES.
> > >
> > >v2:
> > >- Generalized commit message, as per Jessica.
> > >- Stubs for within_module_core() and within_module_init(), as per Nick.
> > >
> > >Suggested-by: Matthew Wilcox
> > >Reported-by: Randy Dunlap
> > >Reported-by: kbuild test robot
> > >Link: https://marc.info/?l=linux-mm=155384681109231=2
> > >Signed-off-by: Tri Vo
> >
> > Applied, thanks!
>
> Thank you!
Andrew,
this patch fixes 8c3d220cb6b5 ("gcov: clang support"). Could you
re-apply the gcov patch? Sorry, if it's a dumb question. I'm not
familiar with how cross-tree patches are handled in Linux.
Re: [PATCH v2] module: add stubs for within_module functions
On Tue, Apr 16, 2019 at 8:21 AM Jessica Yu wrote: > > +++ Tri Vo [15/04/19 11:18 -0700]: > >Provide stubs for within_module_core(), within_module_init(), and > >within_module() to prevent build errors when !CONFIG_MODULES. > > > >v2: > >- Generalized commit message, as per Jessica. > >- Stubs for within_module_core() and within_module_init(), as per Nick. > > > >Suggested-by: Matthew Wilcox > >Reported-by: Randy Dunlap > >Reported-by: kbuild test robot > >Link: https://marc.info/?l=linux-mm=155384681109231=2 > >Signed-off-by: Tri Vo > > Applied, thanks! Thank you!
[PATCH v2] module: add stubs for within_module functions
Provide stubs for within_module_core(), within_module_init(), and
within_module() to prevent build errors when !CONFIG_MODULES.
v2:
- Generalized commit message, as per Jessica.
- Stubs for within_module_core() and within_module_init(), as per Nick.
Suggested-by: Matthew Wilcox
Reported-by: Randy Dunlap
Reported-by: kbuild test robot
Link: https://marc.info/?l=linux-mm=155384681109231=2
Signed-off-by: Tri Vo
---
include/linux/module.h | 17 +
1 file changed, 17 insertions(+)
diff --git a/include/linux/module.h b/include/linux/module.h
index 5bf5dcd91009..35d83765bfbd 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -709,6 +709,23 @@ static inline bool is_module_text_address(unsigned long
addr)
return false;
}
+static inline bool within_module_core(unsigned long addr,
+ const struct module *mod)
+{
+ return false;
+}
+
+static inline bool within_module_init(unsigned long addr,
+ const struct module *mod)
+{
+ return false;
+}
+
+static inline bool within_module(unsigned long addr, const struct module *mod)
+{
+ return false;
+}
+
/* Get/put a kernel symbol (calls should be symmetric) */
#define symbol_get(x) ({ extern typeof(x) x __attribute__((weak)); &(x); })
#define symbol_put(x) do { } while (0)
--
2.21.0.392.gf8f6787159e-goog
[PATCH] module: add stub for within_module
Provide a stub for within_module() when CONFIG_MODULES is not set. This
is needed to build CONFIG_GCOV_KERNEL.
Fixes: 8c3d220cb6b5 ("gcov: clang support")
Suggested-by: Matthew Wilcox
Reported-by: Randy Dunlap
Reported-by: kbuild test robot
Link: https://marc.info/?l=linux-mm=155384681109231=2
Signed-off-by: Tri Vo
---
include/linux/module.h | 5 +
1 file changed, 5 insertions(+)
diff --git a/include/linux/module.h b/include/linux/module.h
index 5bf5dcd91009..47190ebb70bf 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -709,6 +709,11 @@ static inline bool is_module_text_address(unsigned long
addr)
return false;
}
+static inline bool within_module(unsigned long addr, const struct module *mod)
+{
+ return false;
+}
+
/* Get/put a kernel symbol (calls should be symmetric) */
#define symbol_get(x) ({ extern typeof(x) x __attribute__((weak)); &(x); })
#define symbol_put(x) do { } while (0)
--
2.21.0.392.gf8f6787159e-goog
Re: [PATCH v2] gcov: fix when CONFIG_MODULES is not set
On Fri, Mar 29, 2019 at 1:53 PM Randy Dunlap wrote:
>
> On 3/29/19 11:18 AM, Nick Desaulniers wrote:
> > Fixes commit 8c3d220cb6b5 ("gcov: clang support")
>
> There is a certain format for Fixes: and that's not quite it. :(
>
> > Cc: Greg Hackmann
> > Cc: Tri Vo
> > Cc: Peter Oberparleiter
> > Cc: linux...@kvack.org
> > Cc: kbuild-...@01.org
> > Reported-by: Randy Dunlap
> > Reported-by: kbuild test robot
> > Link: https://marc.info/?l=linux-mm=155384681109231=2
> > Signed-off-by: Nick Desaulniers
>
> Acked-by: Randy Dunlap # build-tested
>
> Thanks.
>
> > ---
> > kernel/gcov/gcc_3_4.c | 4
> > kernel/gcov/gcc_4_7.c | 4
> > 2 files changed, 8 insertions(+)
Thanks for taking a look at this Nick! I believe same fix should be
applied to kernel/gcov/clang.c. I'll send out an updated version later
today.
[PATCH v4 3/3] gcov: docs: add a note on GCC vs Clang differences
Document some things of note to gcov users:
1. GCC gcov and Clang llvm-cov tools are not compatible.
2. The use of GCC vs Clang is transparent at build-time.
Also adjust the documentation to account for the removal of config
symbol CONFIG_GCOV_FORMAT_AUTODETECT by commit 6a61b70b43c9
("gcov: remove CONFIG_GCOV_FORMAT_AUTODETECT").
Signed-off-by: Tri Vo
Reviewed-by: Peter Oberparleiter
---
Documentation/dev-tools/gcov.rst | 18 ++
1 file changed, 14 insertions(+), 4 deletions(-)
diff --git a/Documentation/dev-tools/gcov.rst b/Documentation/dev-tools/gcov.rst
index 69a7d90c320a..46aae52a41d0 100644
--- a/Documentation/dev-tools/gcov.rst
+++ b/Documentation/dev-tools/gcov.rst
@@ -34,10 +34,6 @@ Configure the kernel with::
CONFIG_DEBUG_FS=y
CONFIG_GCOV_KERNEL=y
-select the gcc's gcov format, default is autodetect based on gcc version::
-
-CONFIG_GCOV_FORMAT_AUTODETECT=y
-
and to get coverage data for the entire kernel::
CONFIG_GCOV_PROFILE_ALL=y
@@ -169,6 +165,20 @@ b) gcov is run on the BUILD machine
[user@build] gcov -o /tmp/coverage/tmp/out/init main.c
+Note on compilers
+-
+
+GCC and LLVM gcov tools are not necessarily compatible. Use gcov_ to work with
+GCC-generated .gcno and .gcda files, and use llvm-cov_ for Clang.
+
+.. _gcov: http://gcc.gnu.org/onlinedocs/gcc/Gcov.html
+.. _llvm-cov: https://llvm.org/docs/CommandGuide/llvm-cov.html
+
+Build differences between GCC and Clang gcov are handled by Kconfig. It
+automatically selects the appropriate gcov format depending on the detected
+toolchain.
+
+
Troubleshooting
---
--
2.21.0.225.g810b269d1ac-goog
[PATCH v4 0/3] gcov: add Clang support
This patch series adds Clang support for gcov. Patch 1 refactors existing code in preparation for Clang support. Patch 2 implements necessary LLVM runtime hooks and gcov kernel interfaces. Patch 3 updates documentation. Greg Hackmann (2): gcov: Clang: move common GCC code into gcc_base.c gcov: Clang support Tri Vo (1): gcov: docs: add a note on GCC vs Clang differences Documentation/dev-tools/gcov.rst | 18 +- kernel/gcov/Kconfig | 3 +- kernel/gcov/Makefile | 5 +- kernel/gcov/base.c | 86 + kernel/gcov/clang.c | 586 +++ kernel/gcov/gcc_3_4.c| 12 + kernel/gcov/gcc_4_7.c| 12 + kernel/gcov/gcc_base.c | 86 + kernel/gcov/gcov.h | 5 + 9 files changed, 723 insertions(+), 90 deletions(-) create mode 100644 kernel/gcov/clang.c create mode 100644 kernel/gcov/gcc_base.c v2: - Reorganized config dependencies, as per Masahiro. v3: - Squashed patches 2-4 of v2, as per Nick, Masahiro, and Peter. Addressed comments by Peter: - Moved __gcov_exit() to gcc_base.c - Added missing header to gcc_base.c - Removed unnecessary boundary checks in gcov_info_add(). - Changed counters' allocation to use vmalloc(). - Added check for failed allocation of filename. - Changed list_for_each_entry_safe to list_for_each_entry when traversing without modifying. - Updated Documentation/dev-tools/gcov.rst v4: Made following changes to pass kernel module test cases suggested by Peter: - Generic code in base.c unlinks gcov_info data sets while iterating over the gcov_info list. Changed Clang's gcov_info_unlink() to preserve next and prev links of the unlinked gcov_info instance. - Attributing a given gcov_info to a module is done differently in Clang vs GCC. Generic code in base.c needs to do this. So added gcov_info_within_module() interface to gcov.h to abstract away the difference. - Clang provides checksums for each function. These checksums are used to determine whether the source files of the compiled kernel module have been modified. Changed gcov_info_is_compatible() to account for function-level checksums. Addressed comments by Peter: - Removed unnecessary #define, if-clause, empty line. - Amended commit message to note documentation was adjusted to account for the removal of config symbol CONFIG_GCOV_FORMAT_AUTODETECT. -- 2.21.0.225.g810b269d1ac-goog
[PATCH v4 1/3] gcov: Clang: move common GCC code into gcc_base.c
From: Greg Hackmann
base.c contains a few callbacks specific to GCC's gcov implementation.
Move these into their own module in preparation for Clang support.
Signed-off-by: Greg Hackmann
Signed-off-by: Nick Desaulniers
Signed-off-by: Tri Vo
Tested-by: Trilok Soni
Tested-by: Prasad Sodagudi
Tested-by: Tri Vo
Reviewed-by: Peter Oberparleiter
---
kernel/gcov/Makefile | 4 +-
kernel/gcov/base.c | 84 +
kernel/gcov/gcc_base.c | 86 ++
kernel/gcov/gcov.h | 3 ++
4 files changed, 93 insertions(+), 84 deletions(-)
create mode 100644 kernel/gcov/gcc_base.c
diff --git a/kernel/gcov/Makefile b/kernel/gcov/Makefile
index ff06d64df397..45431ed679d1 100644
--- a/kernel/gcov/Makefile
+++ b/kernel/gcov/Makefile
@@ -2,5 +2,5 @@
ccflags-y := -DSRCTREE='"$(srctree)"' -DOBJTREE='"$(objtree)"'
obj-y := base.o fs.o
-obj-$(CONFIG_GCOV_FORMAT_3_4) += gcc_3_4.o
-obj-$(CONFIG_GCOV_FORMAT_4_7) += gcc_4_7.o
+obj-$(CONFIG_GCOV_FORMAT_3_4) += gcc_base.o gcc_3_4.o
+obj-$(CONFIG_GCOV_FORMAT_4_7) += gcc_base.o gcc_4_7.o
diff --git a/kernel/gcov/base.c b/kernel/gcov/base.c
index 9c7c8d5c18f2..799d42072727 100644
--- a/kernel/gcov/base.c
+++ b/kernel/gcov/base.c
@@ -22,88 +22,8 @@
#include
#include "gcov.h"
-static int gcov_events_enabled;
-static DEFINE_MUTEX(gcov_lock);
-
-/*
- * __gcov_init is called by gcc-generated constructor code for each object
- * file compiled with -fprofile-arcs.
- */
-void __gcov_init(struct gcov_info *info)
-{
- static unsigned int gcov_version;
-
- mutex_lock(_lock);
- if (gcov_version == 0) {
- gcov_version = gcov_info_version(info);
- /*
-* Printing gcc's version magic may prove useful for debugging
-* incompatibility reports.
-*/
- pr_info("version magic: 0x%x\n", gcov_version);
- }
- /*
-* Add new profiling data structure to list and inform event
-* listener.
-*/
- gcov_info_link(info);
- if (gcov_events_enabled)
- gcov_event(GCOV_ADD, info);
- mutex_unlock(_lock);
-}
-EXPORT_SYMBOL(__gcov_init);
-
-/*
- * These functions may be referenced by gcc-generated profiling code but serve
- * no function for kernel profiling.
- */
-void __gcov_flush(void)
-{
- /* Unused. */
-}
-EXPORT_SYMBOL(__gcov_flush);
-
-void __gcov_merge_add(gcov_type *counters, unsigned int n_counters)
-{
- /* Unused. */
-}
-EXPORT_SYMBOL(__gcov_merge_add);
-
-void __gcov_merge_single(gcov_type *counters, unsigned int n_counters)
-{
- /* Unused. */
-}
-EXPORT_SYMBOL(__gcov_merge_single);
-
-void __gcov_merge_delta(gcov_type *counters, unsigned int n_counters)
-{
- /* Unused. */
-}
-EXPORT_SYMBOL(__gcov_merge_delta);
-
-void __gcov_merge_ior(gcov_type *counters, unsigned int n_counters)
-{
- /* Unused. */
-}
-EXPORT_SYMBOL(__gcov_merge_ior);
-
-void __gcov_merge_time_profile(gcov_type *counters, unsigned int n_counters)
-{
- /* Unused. */
-}
-EXPORT_SYMBOL(__gcov_merge_time_profile);
-
-void __gcov_merge_icall_topn(gcov_type *counters, unsigned int n_counters)
-{
- /* Unused. */
-}
-EXPORT_SYMBOL(__gcov_merge_icall_topn);
-
-void __gcov_exit(void)
-{
- /* Unused. */
-}
-EXPORT_SYMBOL(__gcov_exit);
+int gcov_events_enabled;
+DEFINE_MUTEX(gcov_lock);
/**
* gcov_enable_events - enable event reporting through gcov_event()
diff --git a/kernel/gcov/gcc_base.c b/kernel/gcov/gcc_base.c
new file mode 100644
index ..3cf736b9f880
--- /dev/null
+++ b/kernel/gcov/gcc_base.c
@@ -0,0 +1,86 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include
+#include
+#include
+#include "gcov.h"
+
+/*
+ * __gcov_init is called by gcc-generated constructor code for each object
+ * file compiled with -fprofile-arcs.
+ */
+void __gcov_init(struct gcov_info *info)
+{
+ static unsigned int gcov_version;
+
+ mutex_lock(_lock);
+ if (gcov_version == 0) {
+ gcov_version = gcov_info_version(info);
+ /*
+* Printing gcc's version magic may prove useful for debugging
+* incompatibility reports.
+*/
+ pr_info("version magic: 0x%x\n", gcov_version);
+ }
+ /*
+* Add new profiling data structure to list and inform event
+* listener.
+*/
+ gcov_info_link(info);
+ if (gcov_events_enabled)
+ gcov_event(GCOV_ADD, info);
+ mutex_unlock(_lock);
+}
+EXPORT_SYMBOL(__gcov_init);
+
+/*
+ * These functions may be referenced by gcc-generated profiling code but serve
+ * no function for kernel profiling.
+ */
+void __gcov_flush(void)
+{
+ /* Unused. */
+}
+EXPORT_SYMBOL(__gcov_flush);
+
+void __gcov_merge_add(gcov_type *counters, unsigned int n_counters)
+{
+ /* Unused. */
+}
+EXPORT_SYMBOL(__gcov
[PATCH v4 2/3] gcov: Clang support
From: Greg Hackmann
LLVM uses profiling data that's deliberately similar to GCC, but has a very
different way of exporting that data. LLVM calls llvm_gcov_init() once per
module, and provides a couple of callbacks that we can use to ask for more
data.
We care about the "writeout" callback, which in turn calls back into
compiler-rt/this module to dump all the gathered coverage data to disk:
llvm_gcda_start_file()
llvm_gcda_emit_function()
llvm_gcda_emit_arcs()
llvm_gcda_emit_function()
llvm_gcda_emit_arcs()
[... repeats for each function ...]
llvm_gcda_summary_info()
llvm_gcda_end_file()
This design is much more stateless and unstructured than gcc's, and is
intended to run at process exit. This forces us to keep some local state
about which module we're dealing with at the moment. On the other hand, it
also means we don't depend as much on how LLVM represents profiling data
internally.
See LLVM's lib/Transforms/Instrumentation/GCOVProfiling.cpp for more
details on how this works, particularly GCOVProfiler::emitProfileArcs(),
GCOVProfiler::insertCounterWriteout(), and GCOVProfiler::insertFlush().
Co-authored-by: Nick Desaulniers
Co-authored-by: Tri Vo
Signed-off-by: Greg Hackmann
Signed-off-by: Nick Desaulniers
Signed-off-by: Tri Vo
Tested-by: Trilok Soni
Tested-by: Prasad Sodagudi
Tested-by: Tri Vo
Tested-by: Daniel Mentz
Tested-by: Petri Gynther
Reviewed-by: Peter Oberparleiter
---
kernel/gcov/Kconfig | 3 +-
kernel/gcov/Makefile | 1 +
kernel/gcov/base.c| 2 +-
kernel/gcov/clang.c | 586 ++
kernel/gcov/gcc_3_4.c | 12 +
kernel/gcov/gcc_4_7.c | 12 +
kernel/gcov/gcov.h| 2 +
7 files changed, 616 insertions(+), 2 deletions(-)
create mode 100644 kernel/gcov/clang.c
diff --git a/kernel/gcov/Kconfig b/kernel/gcov/Kconfig
index 1e3823fa799b..f71c1adcff31 100644
--- a/kernel/gcov/Kconfig
+++ b/kernel/gcov/Kconfig
@@ -53,6 +53,7 @@ config GCOV_PROFILE_ALL
choice
prompt "Specify GCOV format"
depends on GCOV_KERNEL
+ depends on CC_IS_GCC
---help---
The gcov format is usually determined by the GCC version, and the
default is chosen according to your GCC version. However, there are
@@ -62,7 +63,7 @@ choice
config GCOV_FORMAT_3_4
bool "GCC 3.4 format"
- depends on CC_IS_GCC && GCC_VERSION < 40700
+ depends on GCC_VERSION < 40700
---help---
Select this option to use the format defined by GCC 3.4.
diff --git a/kernel/gcov/Makefile b/kernel/gcov/Makefile
index 45431ed679d1..d66a74b0f100 100644
--- a/kernel/gcov/Makefile
+++ b/kernel/gcov/Makefile
@@ -4,3 +4,4 @@ ccflags-y := -DSRCTREE='"$(srctree)"' -DOBJTREE='"$(objtree)"'
obj-y := base.o fs.o
obj-$(CONFIG_GCOV_FORMAT_3_4) += gcc_base.o gcc_3_4.o
obj-$(CONFIG_GCOV_FORMAT_4_7) += gcc_base.o gcc_4_7.o
+obj-$(CONFIG_CC_IS_CLANG) += clang.o
diff --git a/kernel/gcov/base.c b/kernel/gcov/base.c
index 799d42072727..0ffe9f194080 100644
--- a/kernel/gcov/base.c
+++ b/kernel/gcov/base.c
@@ -64,7 +64,7 @@ static int gcov_module_notifier(struct notifier_block *nb,
unsigned long event,
/* Remove entries located in module from linked list. */
while ((info = gcov_info_next(info))) {
- if (within_module((unsigned long)info, mod)) {
+ if (gcov_info_within_module(info, mod)) {
gcov_info_unlink(prev, info);
if (gcov_events_enabled)
gcov_event(GCOV_REMOVE, info);
diff --git a/kernel/gcov/clang.c b/kernel/gcov/clang.c
new file mode 100644
index ..125c50397ba2
--- /dev/null
+++ b/kernel/gcov/clang.c
@@ -0,0 +1,586 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2019 Google, Inc.
+ * modified from kernel/gcov/gcc_4_7.c
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ *
+ * LLVM uses profiling data that's deliberately similar to GCC, but has a
+ * very different way of exporting that data. LLVM calls llvm_gcov_init() once
+ * per module, and provides a couple of callbacks that we can use to ask for
+ * more data.
+ *
+ * We care about the "writeout" callback, which in turn calls back into
+ * compiler-rt/this module to dump all the gathered coverage data to disk:
+ *
+ *llvm_gcda_start_file()
+ * llvm_gcda_emit_function()
+ * llvm_gcda_emit_arcs()
+ * llvm_gcda_emit_function()
+ * llvm_gcda_emit_arcs()
Re: [PATCH v3 0/3] gcov: add Clang support
On Tue, Mar 5, 2019 at 6:29 AM Peter Oberparleiter wrote: > > On 23.01.2019 00:37, Tri Vo wrote: > > This patch series adds Clang supoprt for gcov. > > > > Patch 1 refactors existing code in preparation for Clang support. > > Patch 2 implements necessary LLVM runtime hooks and gcov kernel interfaces. > > Patch 3 updates documentation. > > Thanks for the updates! I've provided suggestions for some minor > improvements in my other review e-mails. > > With those changes applied, the patch set is in my opinion ready for > inclusion into the mainline kernel. My suggestion would be to re-post > the resulting version while putting Andrew Morton on CC as gcov changes > are typically integrated via his tree. > > Also I've successfully re-tested this patch set version on s390 using > GCC 7.3.0 to ensure that the existing GCC support is still working. > Unfortunately I wasn't able to test the Clang version due to some > compile problems on s390 (unrelated to this patch set). > > If you haven't done so, I would like to suggest to run the following > tests with the Clang gcov-kernel version that should catch problems in > some corner cases: > > 1. Unload a module, then use llvm-cov on the associated coverage file. > >Expectation: correct llvm-cov output including coverage of module >exit code. > > 2. Unload a module, modify its source, re-compile it and load it again, >then use llvm-cov on the associated coverage file. > >Expectation: kernel message "discarding saved data", correct llvm-cov >output with no coverage of module exit code. > > 3. Unload a module, then reset all coverage data by writing to >/sys/kernel/debug/gcov/reset. > >Expectation: all coverage files associated with the module are >removed from debugfs. Thanks for the suggested test cases! The current patchset doesn't seem to handle module loading/unloading correctly. I'll fix that in a follow-up.
[PATCH v3 0/3] gcov: add Clang support
This patch series adds Clang supoprt for gcov. Patch 1 refactors existing code in preparation for Clang support. Patch 2 implements necessary LLVM runtime hooks and gcov kernel interfaces. Patch 3 updates documentation. Greg Hackmann (2): gcov: Clang: move common GCC code into gcc_base.c gcov: Clang support Tri Vo (1): gcov: docs: add a note on GCC vs Clang differences Documentation/dev-tools/gcov.rst | 18 +- kernel/gcov/Kconfig | 3 +- kernel/gcov/Makefile | 5 +- kernel/gcov/base.c | 84 + kernel/gcov/clang.c | 555 +++ kernel/gcov/gcc_base.c | 86 + kernel/gcov/gcov.h | 3 + 7 files changed, 665 insertions(+), 89 deletions(-) create mode 100644 kernel/gcov/clang.c create mode 100644 kernel/gcov/gcc_base.c v2: - Reorganized config dependencies, as per Masahiro. v3: - Squashed patches 2-4 of v2, as per Nick, Masahiro, and Peter. Addressed comments by Peter: - Moved __gcov_exit() to gcc_base.c - Added missing header to gcc_base.c - Removed unnecessary boundary checks in gcov_info_add(). - Changed counters' allocation to use vmalloc(). - Added check for failed allocation of filename. - Changed list_for_each_entry_safe to list_for_each_entry when traversing without modifying. - Updated Documentation/dev-tools/gcov.rst -- 2.20.1.321.g9e740568ce-goog
[PATCH v3 3/3] gcov: docs: add a note on GCC vs Clang differences
Document some things of note to gcov users: 1. GCC gcov and Clang llvm-cov tools are not compatible. 2. The use of GCC vs Clang is transparent at build-time. Signed-off-by: Tri Vo --- Documentation/dev-tools/gcov.rst | 18 ++ 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/Documentation/dev-tools/gcov.rst b/Documentation/dev-tools/gcov.rst index 69a7d90c320a..46aae52a41d0 100644 --- a/Documentation/dev-tools/gcov.rst +++ b/Documentation/dev-tools/gcov.rst @@ -34,10 +34,6 @@ Configure the kernel with:: CONFIG_DEBUG_FS=y CONFIG_GCOV_KERNEL=y -select the gcc's gcov format, default is autodetect based on gcc version:: - -CONFIG_GCOV_FORMAT_AUTODETECT=y - and to get coverage data for the entire kernel:: CONFIG_GCOV_PROFILE_ALL=y @@ -169,6 +165,20 @@ b) gcov is run on the BUILD machine [user@build] gcov -o /tmp/coverage/tmp/out/init main.c +Note on compilers +- + +GCC and LLVM gcov tools are not necessarily compatible. Use gcov_ to work with +GCC-generated .gcno and .gcda files, and use llvm-cov_ for Clang. + +.. _gcov: http://gcc.gnu.org/onlinedocs/gcc/Gcov.html +.. _llvm-cov: https://llvm.org/docs/CommandGuide/llvm-cov.html + +Build differences between GCC and Clang gcov are handled by Kconfig. It +automatically selects the appropriate gcov format depending on the detected +toolchain. + + Troubleshooting --- -- 2.20.1.321.g9e740568ce-goog
[PATCH v3 1/3] gcov: Clang: move common GCC code into gcc_base.c
From: Greg Hackmann
base.c contains a few callbacks specific to GCC's gcov implementation.
Move these into their own module in preparation for Clang support.
Signed-off-by: Greg Hackmann
Signed-off-by: Nick Desaulniers
Signed-off-by: Tri Vo
Tested-by: Trilok Soni
Tested-by: Prasad Sodagudi
Tested-by: Tri Vo
---
kernel/gcov/Makefile | 4 +-
kernel/gcov/base.c | 84 +
kernel/gcov/gcc_base.c | 86 ++
kernel/gcov/gcov.h | 3 ++
4 files changed, 93 insertions(+), 84 deletions(-)
create mode 100644 kernel/gcov/gcc_base.c
diff --git a/kernel/gcov/Makefile b/kernel/gcov/Makefile
index ff06d64df397..45431ed679d1 100644
--- a/kernel/gcov/Makefile
+++ b/kernel/gcov/Makefile
@@ -2,5 +2,5 @@
ccflags-y := -DSRCTREE='"$(srctree)"' -DOBJTREE='"$(objtree)"'
obj-y := base.o fs.o
-obj-$(CONFIG_GCOV_FORMAT_3_4) += gcc_3_4.o
-obj-$(CONFIG_GCOV_FORMAT_4_7) += gcc_4_7.o
+obj-$(CONFIG_GCOV_FORMAT_3_4) += gcc_base.o gcc_3_4.o
+obj-$(CONFIG_GCOV_FORMAT_4_7) += gcc_base.o gcc_4_7.o
diff --git a/kernel/gcov/base.c b/kernel/gcov/base.c
index 9c7c8d5c18f2..799d42072727 100644
--- a/kernel/gcov/base.c
+++ b/kernel/gcov/base.c
@@ -22,88 +22,8 @@
#include
#include "gcov.h"
-static int gcov_events_enabled;
-static DEFINE_MUTEX(gcov_lock);
-
-/*
- * __gcov_init is called by gcc-generated constructor code for each object
- * file compiled with -fprofile-arcs.
- */
-void __gcov_init(struct gcov_info *info)
-{
- static unsigned int gcov_version;
-
- mutex_lock(_lock);
- if (gcov_version == 0) {
- gcov_version = gcov_info_version(info);
- /*
-* Printing gcc's version magic may prove useful for debugging
-* incompatibility reports.
-*/
- pr_info("version magic: 0x%x\n", gcov_version);
- }
- /*
-* Add new profiling data structure to list and inform event
-* listener.
-*/
- gcov_info_link(info);
- if (gcov_events_enabled)
- gcov_event(GCOV_ADD, info);
- mutex_unlock(_lock);
-}
-EXPORT_SYMBOL(__gcov_init);
-
-/*
- * These functions may be referenced by gcc-generated profiling code but serve
- * no function for kernel profiling.
- */
-void __gcov_flush(void)
-{
- /* Unused. */
-}
-EXPORT_SYMBOL(__gcov_flush);
-
-void __gcov_merge_add(gcov_type *counters, unsigned int n_counters)
-{
- /* Unused. */
-}
-EXPORT_SYMBOL(__gcov_merge_add);
-
-void __gcov_merge_single(gcov_type *counters, unsigned int n_counters)
-{
- /* Unused. */
-}
-EXPORT_SYMBOL(__gcov_merge_single);
-
-void __gcov_merge_delta(gcov_type *counters, unsigned int n_counters)
-{
- /* Unused. */
-}
-EXPORT_SYMBOL(__gcov_merge_delta);
-
-void __gcov_merge_ior(gcov_type *counters, unsigned int n_counters)
-{
- /* Unused. */
-}
-EXPORT_SYMBOL(__gcov_merge_ior);
-
-void __gcov_merge_time_profile(gcov_type *counters, unsigned int n_counters)
-{
- /* Unused. */
-}
-EXPORT_SYMBOL(__gcov_merge_time_profile);
-
-void __gcov_merge_icall_topn(gcov_type *counters, unsigned int n_counters)
-{
- /* Unused. */
-}
-EXPORT_SYMBOL(__gcov_merge_icall_topn);
-
-void __gcov_exit(void)
-{
- /* Unused. */
-}
-EXPORT_SYMBOL(__gcov_exit);
+int gcov_events_enabled;
+DEFINE_MUTEX(gcov_lock);
/**
* gcov_enable_events - enable event reporting through gcov_event()
diff --git a/kernel/gcov/gcc_base.c b/kernel/gcov/gcc_base.c
new file mode 100644
index ..3cf736b9f880
--- /dev/null
+++ b/kernel/gcov/gcc_base.c
@@ -0,0 +1,86 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include
+#include
+#include
+#include "gcov.h"
+
+/*
+ * __gcov_init is called by gcc-generated constructor code for each object
+ * file compiled with -fprofile-arcs.
+ */
+void __gcov_init(struct gcov_info *info)
+{
+ static unsigned int gcov_version;
+
+ mutex_lock(_lock);
+ if (gcov_version == 0) {
+ gcov_version = gcov_info_version(info);
+ /*
+* Printing gcc's version magic may prove useful for debugging
+* incompatibility reports.
+*/
+ pr_info("version magic: 0x%x\n", gcov_version);
+ }
+ /*
+* Add new profiling data structure to list and inform event
+* listener.
+*/
+ gcov_info_link(info);
+ if (gcov_events_enabled)
+ gcov_event(GCOV_ADD, info);
+ mutex_unlock(_lock);
+}
+EXPORT_SYMBOL(__gcov_init);
+
+/*
+ * These functions may be referenced by gcc-generated profiling code but serve
+ * no function for kernel profiling.
+ */
+void __gcov_flush(void)
+{
+ /* Unused. */
+}
+EXPORT_SYMBOL(__gcov_flush);
+
+void __gcov_merge_add(gcov_type *counters, unsigned int n_counters)
+{
+ /* Unused. */
+}
+EXPORT_SYMBOL(__gcov_merge_add);
+
+void __gc
[PATCH v3 2/3] gcov: Clang support
From: Greg Hackmann LLVM uses profiling data that's deliberately similar to GCC, but has a very different way of exporting that data. LLVM calls llvm_gcov_init() once per module, and provides a couple of callbacks that we can use to ask for more data. We care about the "writeout" callback, which in turn calls back into compiler-rt/this module to dump all the gathered coverage data to disk: llvm_gcda_start_file() llvm_gcda_emit_function() llvm_gcda_emit_arcs() llvm_gcda_emit_function() llvm_gcda_emit_arcs() [... repeats for each function ...] llvm_gcda_summary_info() llvm_gcda_end_file() This design is much more stateless and unstructured than gcc's, and is intended to run at process exit. This forces us to keep some local state about which module we're dealing with at the moment. On the other hand, it also means we don't depend as much on how LLVM represents profiling data internally. See LLVM's lib/Transforms/Instrumentation/GCOVProfiling.cpp for more details on how this works, particularly GCOVProfiler::emitProfileArcs(), GCOVProfiler::insertCounterWriteout(), and GCOVProfiler::insertFlush(). Co-authored-by: Nick Desaulniers Co-authored-by: Tri Vo Signed-off-by: Greg Hackmann Signed-off-by: Nick Desaulniers Signed-off-by: Tri Vo Tested-by: Trilok Soni Tested-by: Prasad Sodagudi Tested-by: Tri Vo Tested-by: Daniel Mentz Tested-by: Petri Gynther --- kernel/gcov/Kconfig | 3 +- kernel/gcov/Makefile | 1 + kernel/gcov/clang.c | 555 +++ 3 files changed, 558 insertions(+), 1 deletion(-) create mode 100644 kernel/gcov/clang.c diff --git a/kernel/gcov/Kconfig b/kernel/gcov/Kconfig index 1e3823fa799b..f71c1adcff31 100644 --- a/kernel/gcov/Kconfig +++ b/kernel/gcov/Kconfig @@ -53,6 +53,7 @@ config GCOV_PROFILE_ALL choice prompt "Specify GCOV format" depends on GCOV_KERNEL + depends on CC_IS_GCC ---help--- The gcov format is usually determined by the GCC version, and the default is chosen according to your GCC version. However, there are @@ -62,7 +63,7 @@ choice config GCOV_FORMAT_3_4 bool "GCC 3.4 format" - depends on CC_IS_GCC && GCC_VERSION < 40700 + depends on GCC_VERSION < 40700 ---help--- Select this option to use the format defined by GCC 3.4. diff --git a/kernel/gcov/Makefile b/kernel/gcov/Makefile index 45431ed679d1..d66a74b0f100 100644 --- a/kernel/gcov/Makefile +++ b/kernel/gcov/Makefile @@ -4,3 +4,4 @@ ccflags-y := -DSRCTREE='"$(srctree)"' -DOBJTREE='"$(objtree)"' obj-y := base.o fs.o obj-$(CONFIG_GCOV_FORMAT_3_4) += gcc_base.o gcc_3_4.o obj-$(CONFIG_GCOV_FORMAT_4_7) += gcc_base.o gcc_4_7.o +obj-$(CONFIG_CC_IS_CLANG) += clang.o diff --git a/kernel/gcov/clang.c b/kernel/gcov/clang.c new file mode 100644 index ..f9acdee3d578 --- /dev/null +++ b/kernel/gcov/clang.c @@ -0,0 +1,555 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright (C) 2019 Google, Inc. + * modified from kernel/gcov/gcc_4_7.c + * + * This software is licensed under the terms of the GNU General Public + * License version 2, as published by the Free Software Foundation, and + * may be copied, distributed, and modified under those terms. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * + * LLVM uses profiling data that's deliberately similar to GCC, but has a + * very different way of exporting that data. LLVM calls llvm_gcov_init() once + * per module, and provides a couple of callbacks that we can use to ask for + * more data. + * + * We care about the "writeout" callback, which in turn calls back into + * compiler-rt/this module to dump all the gathered coverage data to disk: + * + *llvm_gcda_start_file() + * llvm_gcda_emit_function() + * llvm_gcda_emit_arcs() + * llvm_gcda_emit_function() + * llvm_gcda_emit_arcs() + * [... repeats for each function ...] + *llvm_gcda_summary_info() + *llvm_gcda_end_file() + * + * This design is much more stateless and unstructured than gcc's, and is + * intended to run at process exit. This forces us to keep some local state + * about which module we're dealing with at the moment. On the other hand, it + * also means we don't depend as much on how LLVM represents profiling data + * internally. + * + * See LLVM's lib/Transforms/Instrumentation/GCOVProfiling.cpp for more + * details on how this works, particularly GCOVProfiler::emitProfileArcs(), + * GCOVProfiler::insertCounterWriteout(), and + * GCOVProfiler::insertFlush(). + */ + +#define pr_fmt(fmt)"gcov: " fmt + +#include +#include +#include +#include +#include +#include +#include +#include "gcov.
Re: [PATCH v2 0/4] gcov: add Clang support
On Thu, Jan 17, 2019 at 1:30 PM Nick Desaulniers wrote: > > On Wed, Jan 16, 2019 at 7:17 AM Peter Oberparleiter > wrote: > > > > On 15.01.2019 19:36, Tri Vo wrote: > > > From: Tri Vo > > > > > > This patch series adds Clang supoprt for gcov. > > > > > > Patch 1 refactors existing code in preparation for Clang support. Patches > > > 2-3 implement necessary LLVM runtime hooks and gcov kernel interfaces. > > > Patch 4 simplifies config selection. > > > > > > Greg Hackmann (2): > > > gcov: clang: move common gcc code into gcc_base.c > > > gcov: clang support > > > > > > Nick Desaulniers (1): > > > gcov: clang: link/unlink profiling data set. > > > > > > Tri Vo (1): > > > gcov: clang: pick GCC vs Clang format depending on compiler > > > > Overall this patch set looks good to me. It introduces a clean way for > > GCC and Clang specific implementation details to coexist. > > > > A quick test compile using GCC on s390 with the patch set applied was > > successful, so it seems like the existing support is unaffected. > > > > There are some detail suggestions on patch improvements that I'll post > > in separate e-mails. > > Peter, > Thank you so much for the valuable code review and feedback. We'll prep a v3. > > Tri, > I think you should change the authorship for the patches to yourself, > and add a "Co-authored-by: " tag to the commit messages. Can probably > squash this series into 2 patches; the one that separates out the > shared code; and the new llvm additions. WDYT? Squashing into 2 patches sounds good. I'll leave Greg's authorship in there though.
[PATCH v2 3/4] gcov: clang: link/unlink profiling data set.
From: Nick Desaulniers
gcov.h defines an interface to access gcov_info data.
Add Clang implementation of gcov_info_link/gcov_info_unlink interfaces.
Signed-off-by: Nick Desaulniers
Signed-off-by: Tri Vo
Tested-by: Trilok Soni
Tested-by: Prasad Sodagudi
Tested-by: Tri Vo
---
kernel/gcov/clang.c | 33 -
1 file changed, 28 insertions(+), 5 deletions(-)
diff --git a/kernel/gcov/clang.c b/kernel/gcov/clang.c
index b00795d28137..ea42deb852f7 100644
--- a/kernel/gcov/clang.c
+++ b/kernel/gcov/clang.c
@@ -77,6 +77,7 @@ struct gcov_fn_info {
u32 num_counters;
u64 *counters;
+ const char *function_name;
};
static struct gcov_info *current_info;
@@ -124,7 +125,7 @@ void llvm_gcda_emit_function(u32 ident, const char
*function_name,
if (!info) {
pr_warn_ratelimited("failed to allocate gcov function info for
%s\n",
- function_name);
+ function_name ?: "UNKNOWN");
return;
}
@@ -133,6 +134,8 @@ void llvm_gcda_emit_function(u32 ident, const char
*function_name,
info->checksum = func_checksum;
info->use_extra_checksum = use_extra_checksum;
info->cfg_checksum = cfg_checksum;
+ if (function_name)
+ info->function_name = kstrdup(function_name, GFP_KERNEL);
list_add_tail(>head, _info->functions);
}
@@ -193,6 +196,26 @@ struct gcov_info *gcov_info_next(struct gcov_info *info)
return list_next_entry(info, head);
}
+/**
+ * gcov_info_link - link/add profiling data set to the list
+ * @info: profiling data set
+ */
+void gcov_info_link(struct gcov_info *info)
+{
+ list_add_tail(>head, _gcov_list);
+}
+
+/**
+ * gcov_info_unlink - unlink/remove profiling data set from the list
+ * @prev: previous profiling data set
+ * @info: profiling data set
+ */
+void gcov_info_unlink(struct gcov_info *prev, struct gcov_info *info)
+{
+ if (prev)
+ list_del(>head);
+}
+
/* Symbolic links to be created for each profiling data file. */
const struct gcov_link gcov_link[] = {
{ OBJ_TREE, "gcno" }, /* Link to .gcno file in $(objtree). */
@@ -256,8 +279,8 @@ static struct gcov_fn_info *gcov_fn_info_dup(struct
gcov_fn_info *fn)
return NULL;
INIT_LIST_HEAD(_dup->head);
- fn_dup->name = kstrdup(fn->name, GFP_KERNEL);
- if (!fn_dup->name)
+ fn_dup->function_name = kstrdup(fn->function_name, GFP_KERNEL);
+ if (!fn_dup->function_name)
goto err_name;
fn_dup->counters = kmemdup(fn->counters,
@@ -269,7 +292,7 @@ static struct gcov_fn_info *gcov_fn_info_dup(struct
gcov_fn_info *fn)
return fn_dup;
err_counters:
- kfree(fn_dup->name);
+ kfree(fn_dup->function_name);
err_name:
kfree(fn_dup);
return NULL;
@@ -317,7 +340,7 @@ void gcov_info_free(struct gcov_info *info)
struct gcov_fn_info *fn, *tmp;
list_for_each_entry_safe(fn, tmp, >functions, head) {
- kfree(fn->name);
+ kfree(fn->function_name);
kfree(fn->counters);
list_del(>head);
kfree(fn);
--
2.20.1.97.g81188d93c3-goog
[PATCH v2 2/4] gcov: clang support
From: Greg Hackmann
LLVM uses profiling data that's deliberately similar to GCC, but has a very
different way of exporting that data. LLVM calls llvm_gcov_init() once per
module, and provides a couple of callbacks that we can use to ask for more
data.
We care about the "writeout" callback, which in turn calls back into
compiler-rt/this module to dump all the gathered coverage data to disk:
llvm_gcda_start_file()
llvm_gcda_emit_function()
llvm_gcda_emit_arcs()
llvm_gcda_emit_function()
llvm_gcda_emit_arcs()
[... repeats for each function ...]
llvm_gcda_summary_info()
llvm_gcda_end_file()
This design is much more stateless and unstructured than gcc's, and is
intended to run at process exit. This forces us to keep some local state
about which module we're dealing with at the moment. On the other hand, it
also means we don't depend as much on how LLVM represents profiling data
internally.
See LLVM's lib/Transforms/Instrumentation/GCOVProfiling.cpp for more
details on how this works, particularly GCOVProfiler::emitProfileArcs(),
GCOVProfiler::insertCounterWriteout(), and GCOVProfiler::insertFlush().
Signed-off-by: Greg Hackmann
Signed-off-by: Nick Desaulniers
Signed-off-by: Tri Vo
Tested-by: Trilok Soni
Tested-by: Prasad Sodagudi
Tested-by: Tri Vo
---
kernel/gcov/Kconfig | 5 +
kernel/gcov/Makefile | 1 +
kernel/gcov/clang.c | 531 +++
3 files changed, 537 insertions(+)
create mode 100644 kernel/gcov/clang.c
diff --git a/kernel/gcov/Kconfig b/kernel/gcov/Kconfig
index 1e3823fa799b..eb428e570923 100644
--- a/kernel/gcov/Kconfig
+++ b/kernel/gcov/Kconfig
@@ -71,6 +71,11 @@ config GCOV_FORMAT_4_7
---help---
Select this option to use the format defined by GCC 4.7.
+config GCOV_FORMAT_CLANG
+ bool "Clang format"
+ ---help---
+ Select this option to use the format defined by Clang.
+
endchoice
endmenu
diff --git a/kernel/gcov/Makefile b/kernel/gcov/Makefile
index 45431ed679d1..83da4765c18d 100644
--- a/kernel/gcov/Makefile
+++ b/kernel/gcov/Makefile
@@ -4,3 +4,4 @@ ccflags-y := -DSRCTREE='"$(srctree)"' -DOBJTREE='"$(objtree)"'
obj-y := base.o fs.o
obj-$(CONFIG_GCOV_FORMAT_3_4) += gcc_base.o gcc_3_4.o
obj-$(CONFIG_GCOV_FORMAT_4_7) += gcc_base.o gcc_4_7.o
+obj-$(CONFIG_GCOV_FORMAT_CLANG) += clang.o
diff --git a/kernel/gcov/clang.c b/kernel/gcov/clang.c
new file mode 100644
index ..b00795d28137
--- /dev/null
+++ b/kernel/gcov/clang.c
@@ -0,0 +1,531 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2017 Google, Inc.
+ * modified from kernel/gcov/gcc_4_7.c
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ *
+ * LLVM uses profiling data that's deliberately similar to GCC, but has a
+ * very different way of exporting that data. LLVM calls llvm_gcov_init() once
+ * per module, and provides a couple of callbacks that we can use to ask for
+ * more data.
+ *
+ * We care about the "writeout" callback, which in turn calls back into
+ * compiler-rt/this module to dump all the gathered coverage data to disk:
+ *
+ *llvm_gcda_start_file()
+ * llvm_gcda_emit_function()
+ * llvm_gcda_emit_arcs()
+ * llvm_gcda_emit_function()
+ * llvm_gcda_emit_arcs()
+ * [... repeats for each function ...]
+ *llvm_gcda_summary_info()
+ *llvm_gcda_end_file()
+ *
+ * This design is much more stateless and unstructured than gcc's, and is
+ * intended to run at process exit. This forces us to keep some local state
+ * about which module we're dealing with at the moment. On the other hand, it
+ * also means we don't depend as much on how LLVM represents profiling data
+ * internally.
+ *
+ * See LLVM's lib/Transforms/Instrumentation/GCOVProfiling.cpp for more
+ * details on how this works, particularly GCOVProfiler::emitProfileArcs(),
+ * GCOVProfiler::insertCounterWriteout(), and
+ * GCOVProfiler::insertFlush().
+ */
+
+#define pr_fmt(fmt)"gcov: " fmt
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include "gcov.h"
+
+#define GCOV_TAG_FUNCTION_LENGTH 3
+
+typedef void (*llvm_gcov_callback)(void);
+
+struct gcov_info {
+ struct list_head head;
+
+ const char *filename;
+ unsigned int version;
+ u32 checksum;
+
+ struct list_head functions;
+};
+
+struct gcov_fn_info {
+ struct list_head head;
+
+ u32 ident;
+ u32 checksum;
+ u8 use_extra_checksum;
+ u32 cfg_chec
[PATCH v2 4/4] gcov: clang: pick GCC vs Clang format depending on compiler
From: Tri Vo Clang gcov format is only supported by Clang compiler, and Clang compiler only supports Clang format. We set gcov format to depend on which compiler (GCC or Clang) is used. Automatic format detection behavior is preserved because: If GCC is used, one of the GCC gcov formats is selected. If Clang is used, Clang gcov format is selected. Signed-off-by: Tri Vo --- kernel/gcov/Kconfig | 8 ++-- kernel/gcov/Makefile | 2 +- 2 files changed, 3 insertions(+), 7 deletions(-) diff --git a/kernel/gcov/Kconfig b/kernel/gcov/Kconfig index eb428e570923..f71c1adcff31 100644 --- a/kernel/gcov/Kconfig +++ b/kernel/gcov/Kconfig @@ -53,6 +53,7 @@ config GCOV_PROFILE_ALL choice prompt "Specify GCOV format" depends on GCOV_KERNEL + depends on CC_IS_GCC ---help--- The gcov format is usually determined by the GCC version, and the default is chosen according to your GCC version. However, there are @@ -62,7 +63,7 @@ choice config GCOV_FORMAT_3_4 bool "GCC 3.4 format" - depends on CC_IS_GCC && GCC_VERSION < 40700 + depends on GCC_VERSION < 40700 ---help--- Select this option to use the format defined by GCC 3.4. @@ -71,11 +72,6 @@ config GCOV_FORMAT_4_7 ---help--- Select this option to use the format defined by GCC 4.7. -config GCOV_FORMAT_CLANG - bool "Clang format" - ---help--- - Select this option to use the format defined by Clang. - endchoice endmenu diff --git a/kernel/gcov/Makefile b/kernel/gcov/Makefile index 83da4765c18d..d66a74b0f100 100644 --- a/kernel/gcov/Makefile +++ b/kernel/gcov/Makefile @@ -4,4 +4,4 @@ ccflags-y := -DSRCTREE='"$(srctree)"' -DOBJTREE='"$(objtree)"' obj-y := base.o fs.o obj-$(CONFIG_GCOV_FORMAT_3_4) += gcc_base.o gcc_3_4.o obj-$(CONFIG_GCOV_FORMAT_4_7) += gcc_base.o gcc_4_7.o -obj-$(CONFIG_GCOV_FORMAT_CLANG) += clang.o +obj-$(CONFIG_CC_IS_CLANG) += clang.o -- 2.20.1.97.g81188d93c3-goog
[PATCH v2 1/4] gcov: clang: move common gcc code into gcc_base.c
From: Greg Hackmann
base.c contains a few callbacks specific to GCC's gcov implementation.
Move these into their own module in preparation for clang support.
Signed-off-by: Greg Hackmann
Signed-off-by: Nick Desaulniers
Signed-off-by: Tri Vo
Tested-by: Trilok Soni
Tested-by: Prasad Sodagudi
Tested-by: Tri Vo
---
kernel/gcov/Makefile | 4 +--
kernel/gcov/base.c | 78 ++---
kernel/gcov/gcc_base.c | 79 ++
kernel/gcov/gcov.h | 3 ++
4 files changed, 86 insertions(+), 78 deletions(-)
create mode 100644 kernel/gcov/gcc_base.c
diff --git a/kernel/gcov/Makefile b/kernel/gcov/Makefile
index ff06d64df397..45431ed679d1 100644
--- a/kernel/gcov/Makefile
+++ b/kernel/gcov/Makefile
@@ -2,5 +2,5 @@
ccflags-y := -DSRCTREE='"$(srctree)"' -DOBJTREE='"$(objtree)"'
obj-y := base.o fs.o
-obj-$(CONFIG_GCOV_FORMAT_3_4) += gcc_3_4.o
-obj-$(CONFIG_GCOV_FORMAT_4_7) += gcc_4_7.o
+obj-$(CONFIG_GCOV_FORMAT_3_4) += gcc_base.o gcc_3_4.o
+obj-$(CONFIG_GCOV_FORMAT_4_7) += gcc_base.o gcc_4_7.o
diff --git a/kernel/gcov/base.c b/kernel/gcov/base.c
index 9c7c8d5c18f2..5262bb784597 100644
--- a/kernel/gcov/base.c
+++ b/kernel/gcov/base.c
@@ -22,82 +22,8 @@
#include
#include "gcov.h"
-static int gcov_events_enabled;
-static DEFINE_MUTEX(gcov_lock);
-
-/*
- * __gcov_init is called by gcc-generated constructor code for each object
- * file compiled with -fprofile-arcs.
- */
-void __gcov_init(struct gcov_info *info)
-{
- static unsigned int gcov_version;
-
- mutex_lock(_lock);
- if (gcov_version == 0) {
- gcov_version = gcov_info_version(info);
- /*
-* Printing gcc's version magic may prove useful for debugging
-* incompatibility reports.
-*/
- pr_info("version magic: 0x%x\n", gcov_version);
- }
- /*
-* Add new profiling data structure to list and inform event
-* listener.
-*/
- gcov_info_link(info);
- if (gcov_events_enabled)
- gcov_event(GCOV_ADD, info);
- mutex_unlock(_lock);
-}
-EXPORT_SYMBOL(__gcov_init);
-
-/*
- * These functions may be referenced by gcc-generated profiling code but serve
- * no function for kernel profiling.
- */
-void __gcov_flush(void)
-{
- /* Unused. */
-}
-EXPORT_SYMBOL(__gcov_flush);
-
-void __gcov_merge_add(gcov_type *counters, unsigned int n_counters)
-{
- /* Unused. */
-}
-EXPORT_SYMBOL(__gcov_merge_add);
-
-void __gcov_merge_single(gcov_type *counters, unsigned int n_counters)
-{
- /* Unused. */
-}
-EXPORT_SYMBOL(__gcov_merge_single);
-
-void __gcov_merge_delta(gcov_type *counters, unsigned int n_counters)
-{
- /* Unused. */
-}
-EXPORT_SYMBOL(__gcov_merge_delta);
-
-void __gcov_merge_ior(gcov_type *counters, unsigned int n_counters)
-{
- /* Unused. */
-}
-EXPORT_SYMBOL(__gcov_merge_ior);
-
-void __gcov_merge_time_profile(gcov_type *counters, unsigned int n_counters)
-{
- /* Unused. */
-}
-EXPORT_SYMBOL(__gcov_merge_time_profile);
-
-void __gcov_merge_icall_topn(gcov_type *counters, unsigned int n_counters)
-{
- /* Unused. */
-}
-EXPORT_SYMBOL(__gcov_merge_icall_topn);
+int gcov_events_enabled;
+DEFINE_MUTEX(gcov_lock);
void __gcov_exit(void)
{
diff --git a/kernel/gcov/gcc_base.c b/kernel/gcov/gcc_base.c
new file mode 100644
index ..823565bcf9bf
--- /dev/null
+++ b/kernel/gcov/gcc_base.c
@@ -0,0 +1,79 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include
+#include
+#include "gcov.h"
+
+/*
+ * __gcov_init is called by gcc-generated constructor code for each object
+ * file compiled with -fprofile-arcs.
+ */
+void __gcov_init(struct gcov_info *info)
+{
+ static unsigned int gcov_version;
+
+ mutex_lock(_lock);
+ if (gcov_version == 0) {
+ gcov_version = gcov_info_version(info);
+ /*
+* Printing gcc's version magic may prove useful for debugging
+* incompatibility reports.
+*/
+ pr_info("version magic: 0x%x\n", gcov_version);
+ }
+ /*
+* Add new profiling data structure to list and inform event
+* listener.
+*/
+ gcov_info_link(info);
+ if (gcov_events_enabled)
+ gcov_event(GCOV_ADD, info);
+ mutex_unlock(_lock);
+}
+EXPORT_SYMBOL(__gcov_init);
+
+/*
+ * These functions may be referenced by gcc-generated profiling code but serve
+ * no function for kernel profiling.
+ */
+void __gcov_flush(void)
+{
+ /* Unused. */
+}
+EXPORT_SYMBOL(__gcov_flush);
+
+void __gcov_merge_add(gcov_type *counters, unsigned int n_counters)
+{
+ /* Unused. */
+}
+EXPORT_SYMBOL(__gcov_merge_add);
+
+void __gcov_merge_single(gcov_type *counters, unsigned int n_counters)
+{
+ /* Unused. */
+}
+EXPORT_SYMBOL(__gcov_merge_single);
+
+void __g
[PATCH v2 0/4] gcov: add Clang support
From: Tri Vo This patch series adds Clang supoprt for gcov. Patch 1 refactors existing code in preparation for Clang support. Patches 2-3 implement necessary LLVM runtime hooks and gcov kernel interfaces. Patch 4 simplifies config selection. Greg Hackmann (2): gcov: clang: move common gcc code into gcc_base.c gcov: clang support Nick Desaulniers (1): gcov: clang: link/unlink profiling data set. Tri Vo (1): gcov: clang: pick GCC vs Clang format depending on compiler kernel/gcov/Kconfig| 3 +- kernel/gcov/Makefile | 5 +- kernel/gcov/base.c | 78 +- kernel/gcov/clang.c| 554 + kernel/gcov/gcc_base.c | 79 ++ kernel/gcov/gcov.h | 3 + 6 files changed, 643 insertions(+), 79 deletions(-) create mode 100644 kernel/gcov/clang.c create mode 100644 kernel/gcov/gcc_base.c v2: - Reorganized config dependencies, as per Masahiro. -- 2.20.1.97.g81188d93c3-goog
Re: [PATCH 4/4] gcov: clang: pick GCC vs Clang format depending on compiler
On Mon, Jan 14, 2019 at 5:25 PM Masahiro Yamada wrote: > > On Tue, Jan 15, 2019 at 6:07 AM Tri Vo wrote: > > > > Clang gcov format is only supported by Clang compiler, and Clang > > compiler only supports Clang format. > > > If so, what is the point of putting GCOV_FORMAT_CLANG into the > choice menu? > > > You can choose the format only when you are using GCC. > > I think the following is more sensible: > > > if GCOV_KERNEL > > config GCOV_PROFILE_ALL > > > > choice > prompt "Specify GCOV format for GCC" > depends on CC_IS_GCC > ... > > config GCOV_FORMAT_3_4 > bool "GCC 3.4 format" > depends on GCC_VERSION < 40700 > ... > > config GCOV_FORMAT_4_7 > bool "GCC 4.7 format" > ... > > endchoice > > > config GCOV_FORMAT_CLANG > def_bool CC_IS_CLANG > > endif > > > > > > Or, you can delete GCOV_FORMAT_CLANG > if you write the Makefile like follows: > > > > obj-y := base.o fs.o > obj-$(CONFIG_GCOV_FORMAT_3_4) += gcc_base.o gcc_3_4.o > obj-$(CONFIG_GCOV_FORMAT_4_7) += gcc_base.o gcc_4_7.o > obj-$(CONFIG_CC_IS_CLANG) += clang.o Thanks for the suggestion! It is more sensible than the current approach. I'll send an update.
Re: [PATCH 0/4] gcov: add Clang support
On Mon, Jan 14, 2019 at 1:33 PM Nick Desaulniers wrote: > > On Mon, Jan 14, 2019 at 1:04 PM Tri Vo wrote: > > > > This patch series adds Clang supoprt for gcov. > > > > Patch 1 refactors existing code in preparation for Clang support. Patches > > 2-3 implement necessary LLVM runtime hooks and gcov kernel interfaces. > > Patch 4 simplifies config selection. > > > > Greg Hackmann (2): > > gcov: clang: move common gcc code into gcc_base.c > > gcov: clang support > > > > Nick Desaulniers (1): > > gcov: clang: link/unlink profiling data set. > > > > Tri Vo (1): > > gcov: clang: pick GCC vs Clang format depending on compiler > > Thanks for sending Tri! Doesn't the output format require llvm-cov, or no? Yes, .gcda files generated by a Clang-built kernel could only be parsed by llvm-cov, but not gcc gcov (at least in my local tests). llvm-cov documentation also indicates that GCC and LLVM gcov tools are not necessarily compatible. "[llvm-cov] is compatible with the gcov tool from version 4.2 of GCC and may also be compatible with some later versions of gcov." https://llvm.org/docs/CommandGuide/llvm-cov.html
[PATCH 3/4] gcov: clang: link/unlink profiling data set.
From: Nick Desaulniers
gcov.h defines an interface to access gcov_info data.
Add Clang implementation of gcov_info_link/gcov_info_unlink interfaces.
Signed-off-by: Nick Desaulniers
Signed-off-by: Tri Vo
Tested-by: Trilok Soni
Tested-by: Prasad Sodagudi
Tested-by: Tri Vo
---
kernel/gcov/clang.c | 33 -
1 file changed, 28 insertions(+), 5 deletions(-)
diff --git a/kernel/gcov/clang.c b/kernel/gcov/clang.c
index b00795d28137..ea42deb852f7 100644
--- a/kernel/gcov/clang.c
+++ b/kernel/gcov/clang.c
@@ -77,6 +77,7 @@ struct gcov_fn_info {
u32 num_counters;
u64 *counters;
+ const char *function_name;
};
static struct gcov_info *current_info;
@@ -124,7 +125,7 @@ void llvm_gcda_emit_function(u32 ident, const char
*function_name,
if (!info) {
pr_warn_ratelimited("failed to allocate gcov function info for
%s\n",
- function_name);
+ function_name ?: "UNKNOWN");
return;
}
@@ -133,6 +134,8 @@ void llvm_gcda_emit_function(u32 ident, const char
*function_name,
info->checksum = func_checksum;
info->use_extra_checksum = use_extra_checksum;
info->cfg_checksum = cfg_checksum;
+ if (function_name)
+ info->function_name = kstrdup(function_name, GFP_KERNEL);
list_add_tail(>head, _info->functions);
}
@@ -193,6 +196,26 @@ struct gcov_info *gcov_info_next(struct gcov_info *info)
return list_next_entry(info, head);
}
+/**
+ * gcov_info_link - link/add profiling data set to the list
+ * @info: profiling data set
+ */
+void gcov_info_link(struct gcov_info *info)
+{
+ list_add_tail(>head, _gcov_list);
+}
+
+/**
+ * gcov_info_unlink - unlink/remove profiling data set from the list
+ * @prev: previous profiling data set
+ * @info: profiling data set
+ */
+void gcov_info_unlink(struct gcov_info *prev, struct gcov_info *info)
+{
+ if (prev)
+ list_del(>head);
+}
+
/* Symbolic links to be created for each profiling data file. */
const struct gcov_link gcov_link[] = {
{ OBJ_TREE, "gcno" }, /* Link to .gcno file in $(objtree). */
@@ -256,8 +279,8 @@ static struct gcov_fn_info *gcov_fn_info_dup(struct
gcov_fn_info *fn)
return NULL;
INIT_LIST_HEAD(_dup->head);
- fn_dup->name = kstrdup(fn->name, GFP_KERNEL);
- if (!fn_dup->name)
+ fn_dup->function_name = kstrdup(fn->function_name, GFP_KERNEL);
+ if (!fn_dup->function_name)
goto err_name;
fn_dup->counters = kmemdup(fn->counters,
@@ -269,7 +292,7 @@ static struct gcov_fn_info *gcov_fn_info_dup(struct
gcov_fn_info *fn)
return fn_dup;
err_counters:
- kfree(fn_dup->name);
+ kfree(fn_dup->function_name);
err_name:
kfree(fn_dup);
return NULL;
@@ -317,7 +340,7 @@ void gcov_info_free(struct gcov_info *info)
struct gcov_fn_info *fn, *tmp;
list_for_each_entry_safe(fn, tmp, >functions, head) {
- kfree(fn->name);
+ kfree(fn->function_name);
kfree(fn->counters);
list_del(>head);
kfree(fn);
--
2.20.1.97.g81188d93c3-goog
[PATCH 2/4] gcov: clang support
From: Greg Hackmann
LLVM uses profiling data that's deliberately similar to GCC, but has a very
different way of exporting that data. LLVM calls llvm_gcov_init() once per
module, and provides a couple of callbacks that we can use to ask for more
data.
We care about the "writeout" callback, which in turn calls back into
compiler-rt/this module to dump all the gathered coverage data to disk:
llvm_gcda_start_file()
llvm_gcda_emit_function()
llvm_gcda_emit_arcs()
llvm_gcda_emit_function()
llvm_gcda_emit_arcs()
[... repeats for each function ...]
llvm_gcda_summary_info()
llvm_gcda_end_file()
This design is much more stateless and unstructured than gcc's, and is
intended to run at process exit. This forces us to keep some local state
about which module we're dealing with at the moment. On the other hand, it
also means we don't depend as much on how LLVM represents profiling data
internally.
See LLVM's lib/Transforms/Instrumentation/GCOVProfiling.cpp for more
details on how this works, particularly GCOVProfiler::emitProfileArcs(),
GCOVProfiler::insertCounterWriteout(), and GCOVProfiler::insertFlush().
Signed-off-by: Greg Hackmann
Signed-off-by: Nick Desaulniers
Signed-off-by: Tri Vo
Tested-by: Trilok Soni
Tested-by: Prasad Sodagudi
Tested-by: Tri Vo
---
kernel/gcov/Kconfig | 5 +
kernel/gcov/Makefile | 1 +
kernel/gcov/clang.c | 531 +++
3 files changed, 537 insertions(+)
create mode 100644 kernel/gcov/clang.c
diff --git a/kernel/gcov/Kconfig b/kernel/gcov/Kconfig
index 1e3823fa799b..eb428e570923 100644
--- a/kernel/gcov/Kconfig
+++ b/kernel/gcov/Kconfig
@@ -71,6 +71,11 @@ config GCOV_FORMAT_4_7
---help---
Select this option to use the format defined by GCC 4.7.
+config GCOV_FORMAT_CLANG
+ bool "Clang format"
+ ---help---
+ Select this option to use the format defined by Clang.
+
endchoice
endmenu
diff --git a/kernel/gcov/Makefile b/kernel/gcov/Makefile
index 45431ed679d1..83da4765c18d 100644
--- a/kernel/gcov/Makefile
+++ b/kernel/gcov/Makefile
@@ -4,3 +4,4 @@ ccflags-y := -DSRCTREE='"$(srctree)"' -DOBJTREE='"$(objtree)"'
obj-y := base.o fs.o
obj-$(CONFIG_GCOV_FORMAT_3_4) += gcc_base.o gcc_3_4.o
obj-$(CONFIG_GCOV_FORMAT_4_7) += gcc_base.o gcc_4_7.o
+obj-$(CONFIG_GCOV_FORMAT_CLANG) += clang.o
diff --git a/kernel/gcov/clang.c b/kernel/gcov/clang.c
new file mode 100644
index ..b00795d28137
--- /dev/null
+++ b/kernel/gcov/clang.c
@@ -0,0 +1,531 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2017 Google, Inc.
+ * modified from kernel/gcov/gcc_4_7.c
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ *
+ * LLVM uses profiling data that's deliberately similar to GCC, but has a
+ * very different way of exporting that data. LLVM calls llvm_gcov_init() once
+ * per module, and provides a couple of callbacks that we can use to ask for
+ * more data.
+ *
+ * We care about the "writeout" callback, which in turn calls back into
+ * compiler-rt/this module to dump all the gathered coverage data to disk:
+ *
+ *llvm_gcda_start_file()
+ * llvm_gcda_emit_function()
+ * llvm_gcda_emit_arcs()
+ * llvm_gcda_emit_function()
+ * llvm_gcda_emit_arcs()
+ * [... repeats for each function ...]
+ *llvm_gcda_summary_info()
+ *llvm_gcda_end_file()
+ *
+ * This design is much more stateless and unstructured than gcc's, and is
+ * intended to run at process exit. This forces us to keep some local state
+ * about which module we're dealing with at the moment. On the other hand, it
+ * also means we don't depend as much on how LLVM represents profiling data
+ * internally.
+ *
+ * See LLVM's lib/Transforms/Instrumentation/GCOVProfiling.cpp for more
+ * details on how this works, particularly GCOVProfiler::emitProfileArcs(),
+ * GCOVProfiler::insertCounterWriteout(), and
+ * GCOVProfiler::insertFlush().
+ */
+
+#define pr_fmt(fmt)"gcov: " fmt
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include "gcov.h"
+
+#define GCOV_TAG_FUNCTION_LENGTH 3
+
+typedef void (*llvm_gcov_callback)(void);
+
+struct gcov_info {
+ struct list_head head;
+
+ const char *filename;
+ unsigned int version;
+ u32 checksum;
+
+ struct list_head functions;
+};
+
+struct gcov_fn_info {
+ struct list_head head;
+
+ u32 ident;
+ u32 checksum;
+ u8 use_extra_checksum;
+ u32 cfg_chec
[PATCH 4/4] gcov: clang: pick GCC vs Clang format depending on compiler
Clang gcov format is only supported by Clang compiler, and Clang compiler only supports Clang format. We set gcov format to depend on which compiler (GCC or Clang) is used. Automatic format detection behavior is preserved because: If GCC is used, one of the GCC gcov formats is selected. If Clang is used, Clang gcov format is selected. Signed-off-by: Tri Vo --- kernel/gcov/Kconfig | 4 1 file changed, 4 insertions(+) diff --git a/kernel/gcov/Kconfig b/kernel/gcov/Kconfig index eb428e570923..37ec551d4039 100644 --- a/kernel/gcov/Kconfig +++ b/kernel/gcov/Kconfig @@ -60,6 +60,8 @@ choice In such a case, change this option to adjust the format used in the kernel accordingly. + Select Clang gcov format if building with Clang compiler. + config GCOV_FORMAT_3_4 bool "GCC 3.4 format" depends on CC_IS_GCC && GCC_VERSION < 40700 @@ -68,11 +70,13 @@ config GCOV_FORMAT_3_4 config GCOV_FORMAT_4_7 bool "GCC 4.7 format" + depends on CC_IS_GCC ---help--- Select this option to use the format defined by GCC 4.7. config GCOV_FORMAT_CLANG bool "Clang format" + depends on CC_IS_CLANG ---help--- Select this option to use the format defined by Clang. -- 2.20.1.97.g81188d93c3-goog
[PATCH 1/4] gcov: clang: move common gcc code into gcc_base.c
From: Greg Hackmann
base.c contains a few callbacks specific to GCC's gcov implementation.
Move these into their own module in preparation for clang support.
Signed-off-by: Greg Hackmann
Signed-off-by: Nick Desaulniers
Signed-off-by: Tri Vo
Tested-by: Trilok Soni
Tested-by: Prasad Sodagudi
Tested-by: Tri Vo
---
kernel/gcov/Makefile | 4 +--
kernel/gcov/base.c | 78 ++---
kernel/gcov/gcc_base.c | 79 ++
kernel/gcov/gcov.h | 3 ++
4 files changed, 86 insertions(+), 78 deletions(-)
create mode 100644 kernel/gcov/gcc_base.c
diff --git a/kernel/gcov/Makefile b/kernel/gcov/Makefile
index ff06d64df397..45431ed679d1 100644
--- a/kernel/gcov/Makefile
+++ b/kernel/gcov/Makefile
@@ -2,5 +2,5 @@
ccflags-y := -DSRCTREE='"$(srctree)"' -DOBJTREE='"$(objtree)"'
obj-y := base.o fs.o
-obj-$(CONFIG_GCOV_FORMAT_3_4) += gcc_3_4.o
-obj-$(CONFIG_GCOV_FORMAT_4_7) += gcc_4_7.o
+obj-$(CONFIG_GCOV_FORMAT_3_4) += gcc_base.o gcc_3_4.o
+obj-$(CONFIG_GCOV_FORMAT_4_7) += gcc_base.o gcc_4_7.o
diff --git a/kernel/gcov/base.c b/kernel/gcov/base.c
index 9c7c8d5c18f2..5262bb784597 100644
--- a/kernel/gcov/base.c
+++ b/kernel/gcov/base.c
@@ -22,82 +22,8 @@
#include
#include "gcov.h"
-static int gcov_events_enabled;
-static DEFINE_MUTEX(gcov_lock);
-
-/*
- * __gcov_init is called by gcc-generated constructor code for each object
- * file compiled with -fprofile-arcs.
- */
-void __gcov_init(struct gcov_info *info)
-{
- static unsigned int gcov_version;
-
- mutex_lock(_lock);
- if (gcov_version == 0) {
- gcov_version = gcov_info_version(info);
- /*
-* Printing gcc's version magic may prove useful for debugging
-* incompatibility reports.
-*/
- pr_info("version magic: 0x%x\n", gcov_version);
- }
- /*
-* Add new profiling data structure to list and inform event
-* listener.
-*/
- gcov_info_link(info);
- if (gcov_events_enabled)
- gcov_event(GCOV_ADD, info);
- mutex_unlock(_lock);
-}
-EXPORT_SYMBOL(__gcov_init);
-
-/*
- * These functions may be referenced by gcc-generated profiling code but serve
- * no function for kernel profiling.
- */
-void __gcov_flush(void)
-{
- /* Unused. */
-}
-EXPORT_SYMBOL(__gcov_flush);
-
-void __gcov_merge_add(gcov_type *counters, unsigned int n_counters)
-{
- /* Unused. */
-}
-EXPORT_SYMBOL(__gcov_merge_add);
-
-void __gcov_merge_single(gcov_type *counters, unsigned int n_counters)
-{
- /* Unused. */
-}
-EXPORT_SYMBOL(__gcov_merge_single);
-
-void __gcov_merge_delta(gcov_type *counters, unsigned int n_counters)
-{
- /* Unused. */
-}
-EXPORT_SYMBOL(__gcov_merge_delta);
-
-void __gcov_merge_ior(gcov_type *counters, unsigned int n_counters)
-{
- /* Unused. */
-}
-EXPORT_SYMBOL(__gcov_merge_ior);
-
-void __gcov_merge_time_profile(gcov_type *counters, unsigned int n_counters)
-{
- /* Unused. */
-}
-EXPORT_SYMBOL(__gcov_merge_time_profile);
-
-void __gcov_merge_icall_topn(gcov_type *counters, unsigned int n_counters)
-{
- /* Unused. */
-}
-EXPORT_SYMBOL(__gcov_merge_icall_topn);
+int gcov_events_enabled;
+DEFINE_MUTEX(gcov_lock);
void __gcov_exit(void)
{
diff --git a/kernel/gcov/gcc_base.c b/kernel/gcov/gcc_base.c
new file mode 100644
index ..823565bcf9bf
--- /dev/null
+++ b/kernel/gcov/gcc_base.c
@@ -0,0 +1,79 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include
+#include
+#include "gcov.h"
+
+/*
+ * __gcov_init is called by gcc-generated constructor code for each object
+ * file compiled with -fprofile-arcs.
+ */
+void __gcov_init(struct gcov_info *info)
+{
+ static unsigned int gcov_version;
+
+ mutex_lock(_lock);
+ if (gcov_version == 0) {
+ gcov_version = gcov_info_version(info);
+ /*
+* Printing gcc's version magic may prove useful for debugging
+* incompatibility reports.
+*/
+ pr_info("version magic: 0x%x\n", gcov_version);
+ }
+ /*
+* Add new profiling data structure to list and inform event
+* listener.
+*/
+ gcov_info_link(info);
+ if (gcov_events_enabled)
+ gcov_event(GCOV_ADD, info);
+ mutex_unlock(_lock);
+}
+EXPORT_SYMBOL(__gcov_init);
+
+/*
+ * These functions may be referenced by gcc-generated profiling code but serve
+ * no function for kernel profiling.
+ */
+void __gcov_flush(void)
+{
+ /* Unused. */
+}
+EXPORT_SYMBOL(__gcov_flush);
+
+void __gcov_merge_add(gcov_type *counters, unsigned int n_counters)
+{
+ /* Unused. */
+}
+EXPORT_SYMBOL(__gcov_merge_add);
+
+void __gcov_merge_single(gcov_type *counters, unsigned int n_counters)
+{
+ /* Unused. */
+}
+EXPORT_SYMBOL(__gcov_merge_single);
+
+void __g
[PATCH 0/4] gcov: add Clang support
This patch series adds Clang supoprt for gcov. Patch 1 refactors existing code in preparation for Clang support. Patches 2-3 implement necessary LLVM runtime hooks and gcov kernel interfaces. Patch 4 simplifies config selection. Greg Hackmann (2): gcov: clang: move common gcc code into gcc_base.c gcov: clang support Nick Desaulniers (1): gcov: clang: link/unlink profiling data set. Tri Vo (1): gcov: clang: pick GCC vs Clang format depending on compiler kernel/gcov/Kconfig| 9 + kernel/gcov/Makefile | 5 +- kernel/gcov/base.c | 78 +- kernel/gcov/clang.c| 554 + kernel/gcov/gcc_base.c | 79 ++ kernel/gcov/gcov.h | 3 + 6 files changed, 650 insertions(+), 78 deletions(-) create mode 100644 kernel/gcov/clang.c create mode 100644 kernel/gcov/gcc_base.c -- 2.20.1.97.g81188d93c3-goog
[PATCH v3] x86_64: Add "-m elf_i386" when linking i386 object files.
From: George Rimar Linux kernel uses OUTPUT_FORMAT in it's linker scripts. Most of the time -m option is passed to the linker with correct architecture, but sometimes (at least for x86_64) the -m option contradicts OUTPUT_FORMAT directive. Specifically, arch/x86/boot and arch/x86/realmode/rm modules have i386 object files, but are linked with -m elf_x86_64 linker flag when building for x86_64. "man ld" doesn't explicitly state any tie-breakers between -m and OUTPUT_FORMAT. BFD and Gold linkers override -m value with OUTPUT_FORMAT. But LLVM lld has a different behavior. When supplied with contradicting -m and OUTPUT_FORMAT values it fails with the following error message: ld.lld: error: arch/x86/realmode/rm/header.o is incompatible with elf_x86_64 Suggested fix: just add correct -m after incorrect one (it overrides it), so the linker invocation looks like this: ld -m elf_x86_64 -z max-page-size=0x20 -m elf_i386 --emit-relocs -T realmode.lds header.o trampoline_64.o stack.o reboot.o -o realmode.elf This is not a functional change for GNU ld, because (although not explicitly documented) it already overrides -m EMULATION with OUTPUT_FORMAT. Tested by building x86_64 kernel with GNU gcc/ld toolchain and booting it in QEMU. Suggested-by: Dmitry Golovin Signed-off-by: George Rimar Signed-off-by: Tri Vo Tested-by: Tri Vo Tested-by: Nick Desaulniers --- v2: updated commit message to clarify that ld documentation is ambiguous w.r.t -m vs OUTPUT_FORMAT behavior. v3: fixed/added SOB and "Tested-by" fields. arch/x86/boot/Makefile| 2 +- arch/x86/realmode/rm/Makefile | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/arch/x86/boot/Makefile b/arch/x86/boot/Makefile index 9b5adae9cc40..e2839b5c246c 100644 --- a/arch/x86/boot/Makefile +++ b/arch/x86/boot/Makefile @@ -100,7 +100,7 @@ $(obj)/zoffset.h: $(obj)/compressed/vmlinux FORCE AFLAGS_header.o += -I$(objtree)/$(obj) $(obj)/header.o: $(obj)/zoffset.h -LDFLAGS_setup.elf := -T +LDFLAGS_setup.elf := -m elf_i386 -T $(obj)/setup.elf: $(src)/setup.ld $(SETUP_OBJS) FORCE $(call if_changed,ld) diff --git a/arch/x86/realmode/rm/Makefile b/arch/x86/realmode/rm/Makefile index 4463fa72db94..96cb20de08af 100644 --- a/arch/x86/realmode/rm/Makefile +++ b/arch/x86/realmode/rm/Makefile @@ -47,7 +47,7 @@ $(obj)/pasyms.h: $(REALMODE_OBJS) FORCE targets += realmode.lds $(obj)/realmode.lds: $(obj)/pasyms.h -LDFLAGS_realmode.elf := --emit-relocs -T +LDFLAGS_realmode.elf := -m elf_i386 --emit-relocs -T CPPFLAGS_realmode.lds += -P -C -I$(objtree)/$(obj) targets += realmode.elf -- 2.20.1.97.g81188d93c3-goog
[PATCH v2] x86_64: Add "-m elf_i386" when linking i386 object files.
From: George Rimar Linux kernel uses OUTPUT_FORMAT in it's linker scripts. Most of the time -m option is passed to the linker with correct architecture, but sometimes (at least for x86_64) the -m option contradicts OUTPUT_FORMAT directive. Specifically, arch/x86/boot and arch/x86/realmode/rm modules have i386 object files, but are linked with -m elf_x86_64 linker flag when building for x86_64. "man ld" doesn't explicitly state any tie-breakers between -m and OUTPUT_FORMAT. BFD and Gold linkers override -m value with OUTPUT_FORMAT. But LLVM lld has a different behavior. When supplied with contradicting -m and OUTPUT_FORMAT values it fails with the following error message: ld.lld: error: arch/x86/realmode/rm/header.o is incompatible with elf_x86_64 Suggested fix: just add correct -m after incorrect one (it overrides it), so the linker invocation looks like this: ld -m elf_x86_64 -z max-page-size=0x20 -m elf_i386 --emit-relocs -T realmode.lds header.o trampoline_64.o stack.o reboot.o -o realmode.elf This is not a functional change for GNU ld, because (although not explicitly documented) it already overrides -m EMULATION with OUTPUT_FORMAT. Tested by building x86_64 kernel with GNU gcc/ld toolchain and booting it in QEMU. Suggested-by: Dmitry Golovin Signed-off-by: Georgii Rymar Signed-off-by: Tri Vo Tested-by: Tri Vo --- arch/x86/boot/Makefile| 2 +- arch/x86/realmode/rm/Makefile | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/arch/x86/boot/Makefile b/arch/x86/boot/Makefile index 9b5adae9cc40..e2839b5c246c 100644 --- a/arch/x86/boot/Makefile +++ b/arch/x86/boot/Makefile @@ -100,7 +100,7 @@ $(obj)/zoffset.h: $(obj)/compressed/vmlinux FORCE AFLAGS_header.o += -I$(objtree)/$(obj) $(obj)/header.o: $(obj)/zoffset.h -LDFLAGS_setup.elf := -T +LDFLAGS_setup.elf := -m elf_i386 -T $(obj)/setup.elf: $(src)/setup.ld $(SETUP_OBJS) FORCE $(call if_changed,ld) diff --git a/arch/x86/realmode/rm/Makefile b/arch/x86/realmode/rm/Makefile index 4463fa72db94..96cb20de08af 100644 --- a/arch/x86/realmode/rm/Makefile +++ b/arch/x86/realmode/rm/Makefile @@ -47,7 +47,7 @@ $(obj)/pasyms.h: $(REALMODE_OBJS) FORCE targets += realmode.lds $(obj)/realmode.lds: $(obj)/pasyms.h -LDFLAGS_realmode.elf := --emit-relocs -T +LDFLAGS_realmode.elf := -m elf_i386 --emit-relocs -T CPPFLAGS_realmode.lds += -P -C -I$(objtree)/$(obj) targets += realmode.elf -- 2.20.1.97.g81188d93c3-goog
Re: [RFC PATCH] x86_64: Add "-m elf_i386" when linking i386 object files.
Re-sending as plain text. Sorry. On Mon, Jan 7, 2019 at 3:11 PM Tri Vo wrote: > > Hello, hope everyone's had great holidays. > Could someone review at this patch? > > On Tue, Dec 18, 2018 at 3:39 AM George Rimar wrote: >> >> Added Rui, an LLD code owner. >> >> This patch contains the approach we discussed earlier during LLD development >> when >> faced this issue first time (and as a result of the discussion, >> the same fix was suggested: >> https://bugzilla.kernel.org/show_bug.cgi?id=194091#c0), >> so I think it is fine. >> >> Best regards, >> George | Developer | Access Softek, Inc >> >> >> От: Tri Vo >> Отправлено: 12 декабря 2018 г. 22:48 >> Кому: Nick Desaulniers >> Копия: b...@alien8.de; x...@kernel.org; t...@linutronix.de; >> mi...@redhat.com; George Rimar; Dmitry Golovin; Bill Wendling; >> h...@zytor.com; linux-kernel@vger.kernel.org >> Тема: Re: [RFC PATCH] x86_64: Add "-m elf_i386" when linking i386 object >> files. >> >> CAUTION: This email originated from outside of the organization. Do not >> click links or open attachments unless you recognize the sender and know the >> content is safe. >> >> Adding appropriate people, lists. >> On Tue, Dec 11, 2018 at 1:07 PM Nick Desaulniers >> wrote: >> > >> > On Mon, Dec 10, 2018 at 2:50 PM Nick Desaulniers >> > wrote: >> > > >> > > On Mon, Dec 10, 2018 at 2:26 PM Tri Vo wrote: >> > > > >> > > > From: George Rimar >> > > > >> > > > Linux kernel uses OUTPUT_FORMAT in it's linker scripts. Most of the >> > > > time >> > > > -m option is passed to the linker with correct architecture, but >> > > > sometimes (at least for x86_64) the -m option contradicts OUTPUT_FORMAT >> > > > directive. Specifically, arch/x86/boot and arch/x86/realmode/rm modules >> > > > have i386 object files, but are linked with -m elf_x86_64 linker flag >> > > > when building for x86_64. >> > > > >> > > > BFD and Gold linkers are OK with this, but lld fails: >> > > > ld.lld: error: arch/x86/realmode/rm/header.o is incompatible with >> > > > elf_x86_64 >> > > > >> > > > Suggested fix: just add correct -m after incorrect one (it overrides >> > > > it), so the linker invocation looks like this: ld -m elf_x86_64 -z >> > > > max-page-size=0x20 -m elf_i386 --emit-relocs -T realmode.lds >> > > > header.o trampoline_64.o stack.o reboot.o -o realmode.elf (it will also >> > > > work with GNU ld, because it supports OUTPUT_FORMAT and just ignores -m >> > > > options if this directive is in the linker script). >> > > > >> > > > Tested by building x86_64 kernel with GNU gcc/ld toolchain and booting >> > > > it in QEMU. >> > > > >> > > > Suggested-by: Dmitry Golovin >> > > > Signed-off-by: Tri Vo >> > > > Tested-by: Tri Vo >> > > >> > > This fixes the following linkage error I observe when linking an x86 >> > > kernel with LLD: >> > > >> > > ``` >> > > LD arch/x86/realmode/rm/realmode.elf >> > > ld.lld: error: arch/x86/realmode/rm/header.o is incompatible with >> > > elf_x86_64 >> > > ld.lld: error: arch/x86/realmode/rm/trampoline_64.o is incompatible >> > > with elf_x86_64 >> > > ld.lld: error: arch/x86/realmode/rm/stack.o is incompatible with >> > > elf_x86_64 >> > > ld.lld: error: arch/x86/realmode/rm/reboot.o is incompatible with >> > > elf_x86_64 >> > > ld.lld: error: arch/x86/realmode/rm/wakeup_asm.o is incompatible with >> > > elf_x86_64 >> > > ld.lld: error: arch/x86/realmode/rm/wakemain.o is incompatible with >> > > elf_x86_64 >> > > ld.lld: error: arch/x86/realmode/rm/video-mode.o is incompatible with >> > > elf_x86_64 >> > > ld.lld: error: arch/x86/realmode/rm/copy.o is incompatible with >> > > elf_x86_64 >> > > ld.lld: error: arch/x86/realmode/rm/bioscall.o is incompatible with >> > > elf_x86_64 >> > > ld.lld: error: arch/x86/realmode/rm/regs.o is incompatible with >> > > elf_x86_64 >> > > ld.lld: error: arch/x86/realmode/rm/video-vga.o is incompatible with >> > > elf_x86_64 >> > > ld.lld: error: arch/
Re: [RFC PATCH] x86_64: Add "-m elf_i386" when linking i386 object files.
Adding appropriate people, lists. On Tue, Dec 11, 2018 at 1:07 PM Nick Desaulniers wrote: > > On Mon, Dec 10, 2018 at 2:50 PM Nick Desaulniers > wrote: > > > > On Mon, Dec 10, 2018 at 2:26 PM Tri Vo wrote: > > > > > > From: George Rimar > > > > > > Linux kernel uses OUTPUT_FORMAT in it's linker scripts. Most of the time > > > -m option is passed to the linker with correct architecture, but > > > sometimes (at least for x86_64) the -m option contradicts OUTPUT_FORMAT > > > directive. Specifically, arch/x86/boot and arch/x86/realmode/rm modules > > > have i386 object files, but are linked with -m elf_x86_64 linker flag > > > when building for x86_64. > > > > > > BFD and Gold linkers are OK with this, but lld fails: > > > ld.lld: error: arch/x86/realmode/rm/header.o is incompatible with > > > elf_x86_64 > > > > > > Suggested fix: just add correct -m after incorrect one (it overrides > > > it), so the linker invocation looks like this: ld -m elf_x86_64 -z > > > max-page-size=0x20 -m elf_i386 --emit-relocs -T realmode.lds > > > header.o trampoline_64.o stack.o reboot.o -o realmode.elf (it will also > > > work with GNU ld, because it supports OUTPUT_FORMAT and just ignores -m > > > options if this directive is in the linker script). > > > > > > Tested by building x86_64 kernel with GNU gcc/ld toolchain and booting > > > it in QEMU. > > > > > > Suggested-by: Dmitry Golovin > > > Signed-off-by: Tri Vo > > > Tested-by: Tri Vo > > > > This fixes the following linkage error I observe when linking an x86 > > kernel with LLD: > > > > ``` > > LD arch/x86/realmode/rm/realmode.elf > > ld.lld: error: arch/x86/realmode/rm/header.o is incompatible with elf_x86_64 > > ld.lld: error: arch/x86/realmode/rm/trampoline_64.o is incompatible > > with elf_x86_64 > > ld.lld: error: arch/x86/realmode/rm/stack.o is incompatible with elf_x86_64 > > ld.lld: error: arch/x86/realmode/rm/reboot.o is incompatible with elf_x86_64 > > ld.lld: error: arch/x86/realmode/rm/wakeup_asm.o is incompatible with > > elf_x86_64 > > ld.lld: error: arch/x86/realmode/rm/wakemain.o is incompatible with > > elf_x86_64 > > ld.lld: error: arch/x86/realmode/rm/video-mode.o is incompatible with > > elf_x86_64 > > ld.lld: error: arch/x86/realmode/rm/copy.o is incompatible with elf_x86_64 > > ld.lld: error: arch/x86/realmode/rm/bioscall.o is incompatible with > > elf_x86_64 > > ld.lld: error: arch/x86/realmode/rm/regs.o is incompatible with elf_x86_64 > > ld.lld: error: arch/x86/realmode/rm/video-vga.o is incompatible with > > elf_x86_64 > > ld.lld: error: arch/x86/realmode/rm/video-vesa.o is incompatible with > > elf_x86_64 > > ld.lld: error: arch/x86/realmode/rm/video-bios.o is incompatible with > > elf_x86_64 > > arch/x86/realmode/rm/Makefile:55: recipe for target > > 'arch/x86/realmode/rm/realmode.elf' failed > > ``` > > Tested-by: Nick Desaulniers > > > > Looks like we still have a few other (unrelated) issues to track down > > with LLD, but this gets us one step closer. > > > > Thanks for sending this, Tri! > > Just some additional thoughts on this: > > The kernel is adding -m elf_x86_64 to the linker command line > parameters, but then relying on binutils tie-breaking-behavior by > specifying a different architecture via linker script. So it's > ambiguous which arch the kernel is trying to link for, and the kernel > gets lucky/relies on the way ld.bfd happens to resolve this ambiguity > without warning. An alternative fix may be to just filter out/remove > the -m elf_x86_64 for this target's linker flags. > > > > > > --- > > > arch/x86/boot/Makefile| 2 +- > > > arch/x86/realmode/rm/Makefile | 2 +- > > > 2 files changed, 2 insertions(+), 2 deletions(-) > > > > > > diff --git a/arch/x86/boot/Makefile b/arch/x86/boot/Makefile > > > index 9b5adae9cc40..e2839b5c246c 100644 > > > --- a/arch/x86/boot/Makefile > > > +++ b/arch/x86/boot/Makefile > > > @@ -100,7 +100,7 @@ $(obj)/zoffset.h: $(obj)/compressed/vmlinux FORCE > > > AFLAGS_header.o += -I$(objtree)/$(obj) > > > $(obj)/header.o: $(obj)/zoffset.h > > > > > > -LDFLAGS_setup.elf := -T > > > +LDFLAGS_setup.elf := -m elf_i386 -T > > > $(obj)/setup.elf: $(src)/setup.ld $(SETUP_OBJS) FORCE > > > $(call if_changed,ld) > > > > > > diff --git a/arch/x86/realmode/rm/Makefile b/arch/x86/realmode/rm/Makefile > > > index 4463fa72db94..96cb20de08af 100644 > > > --- a/arch/x86/realmode/rm/Makefile > > > +++ b/arch/x86/realmode/rm/Makefile > > > @@ -47,7 +47,7 @@ $(obj)/pasyms.h: $(REALMODE_OBJS) FORCE > > > targets += realmode.lds > > > $(obj)/realmode.lds: $(obj)/pasyms.h > > > > > > -LDFLAGS_realmode.elf := --emit-relocs -T > > > +LDFLAGS_realmode.elf := -m elf_i386 --emit-relocs -T > > > CPPFLAGS_realmode.lds += -P -C -I$(objtree)/$(obj) > > > > > > targets += realmode.elf > > > -- > > > 2.20.0.rc2.403.gdbc3b29805-goog > > > > > > > > > -- > > Thanks, > > ~Nick Desaulniers > > > > -- > Thanks, > ~Nick Desaulniers
Re: [PATCH] x86_64: Mark per-cpu symbols as absolute.
Adding appropriate people, lists. On Tue, Dec 11, 2018 at 11:39 AM Nick Desaulniers wrote: > > On Mon, Dec 10, 2018 at 4:57 PM Tri Vo wrote: > > > > From: Rafael Avila de Espindola > > > > The kernel has many variables that it wants to have a copy for each > > cpu. It is similar to how each thread wants a copy of a thread local > > variable. > > > > To access such variable, the code has to find the offset of that > > variable in the per cpu block and add it to the address of the current > > brock for that cpu. > > > > Finding the offset is a problem. There are no relocations (at least on > > X86_64) that compute the offset of a symbol in a section. To solve this > > problem, the kernel linker script puts the .data..percpu section at > > address 0. By doing this, the fake address is actually the offset. > > > > This means that symbols defined in that section are effectively > > absolute. There is no way for the linker to guess that. So LLD complains > > when linking the kernel: > > ``` > > ld.lld: error: ./arch/x86/kernel/vmlinux.lds:153: at least one side of the > > expression must be absolute > > ld.lld: error: ./arch/x86/kernel/vmlinux.lds:154: at least one side of the > > expression must be absolute > > Makefile:1040: recipe for target 'vmlinux' failed > > ``` > > This fixes the above error for me when linking with LLD with the > previous patch you sent applied (I'm having trouble finding a public > mailing list link though; 'x86_64: Add "-m elf_i386" when linking i386 > object files.'). Still looks like there's maybe 2 more unrelated > issues to track down with LLD, but this gets us closer. > > To make sure this doesn't regress anything for ld.bfd, I triple > checked a more standard build without LLD. > > ld.bfd (clang) + this patch: > -rwxr-xr-x 1 ndesaulniers primarygroup 56931688 Dec 11 11:32 vmlinux > > ld.bfd (clang) w/o this patch: > -rwxr-xr-x 1 ndesaulniers primarygroup 56931688 Dec 11 11:33 vmlinux > > ld.bfd (gcc) w/o this patch: > -rwxr-xr-x 1 ndesaulniers primarygroup 54047232 Dec 11 11:34 vmlinux > > ld.bfd (gcc) w/ this patch: > -rwxr-xr-x 1 ndesaulniers primarygroup 54047232 Dec 11 11:35 vmlinux > > so looks like no change in binary size w/ vs w/o this patch. > > Further, I did a boot test of ld.bfd (gcc) w/ this patch in QEMU. No > issue there. Thanks for sending. > > Tested-by: Nick Desaulniers > > > > > Tell the linker that per-cpu symbols are absolute. > > > > Reported-by: Dmitry Golovin > > Tested-by: Dmitry Golovin > > Signed-off-by: Tri Vo > > --- > > arch/x86/kernel/vmlinux.lds.S | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/arch/x86/kernel/vmlinux.lds.S b/arch/x86/kernel/vmlinux.lds.S > > index 0d618ee634ac..ee3b5c7d662e 100644 > > --- a/arch/x86/kernel/vmlinux.lds.S > > +++ b/arch/x86/kernel/vmlinux.lds.S > > @@ -401,7 +401,7 @@ SECTIONS > > * Per-cpu symbols which need to be offset from __per_cpu_load > > * for the boot processor. > > */ > > -#define INIT_PER_CPU(x) init_per_cpu__##x = x + __per_cpu_load > > +#define INIT_PER_CPU(x) init_per_cpu__##x = ABSOLUTE(x) + __per_cpu_load > > INIT_PER_CPU(gdt_page); > > INIT_PER_CPU(irq_stack_union); > > > > -- > > 2.20.0.rc2.403.gdbc3b29805-goog > > > > > -- > Thanks, > ~Nick Desaulniers
