* Re: [PATCH] PM / wakeirq: Add wakeup name to dedicated wake irqs
2018-02-09 16:14 [PATCH] PM / wakeirq: Add wakeup name to dedicated wake irqs Tony Lindgren
@ 2018-02-14 10:52 ` Rafael J. Wysocki
2018-02-14 11:23 ` Andy Shevchenko
2018-02-16 9:42 ` Rafael J. Wysocki
2 siblings, 0 replies; 10+ messages in thread
From: Rafael J. Wysocki @ 2018-02-14 10:52 UTC (permalink / raw)
To: Tony Lindgren
Cc: Rafael J . Wysocki, Alan Stern, linux-pm, linux-kernel,
Brian Norris, Grygorii Strashko, Ulf Hansson
On Friday, February 9, 2018 5:14:56 PM CET Tony Lindgren wrote:
> This makes it easy to grep :wakeup /proc/interrupts.
>
> Cc: Brian Norris <briannorris@chromium.org>
> Cc: Grygorii Strashko <grygorii.strashko@ti.com>
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> Suggested-by: Jeffy Chen <jeffy.chen@rock-chips.com>
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
> drivers/base/power/power.h | 1 +
> drivers/base/power/wakeirq.c | 18 +++++++++++++++---
> 2 files changed, 16 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/base/power/power.h b/drivers/base/power/power.h
> --- a/drivers/base/power/power.h
> +++ b/drivers/base/power/power.h
> @@ -31,6 +31,7 @@ struct wake_irq {
> struct device *dev;
> unsigned int status;
> int irq;
> + char *name;
> };
>
> extern void dev_pm_arm_wake_irq(struct wake_irq *wirq);
> diff --git a/drivers/base/power/wakeirq.c b/drivers/base/power/wakeirq.c
> --- a/drivers/base/power/wakeirq.c
> +++ b/drivers/base/power/wakeirq.c
> @@ -112,6 +112,7 @@ void dev_pm_clear_wake_irq(struct device *dev)
> free_irq(wirq->irq, wirq);
> wirq->status &= ~WAKE_IRQ_DEDICATED_MASK;
> }
> + kfree(wirq->name);
> kfree(wirq);
> }
> EXPORT_SYMBOL_GPL(dev_pm_clear_wake_irq);
> @@ -175,7 +176,8 @@ static irqreturn_t handle_threaded_wake_irq(int irq, void *_wirq)
> int dev_pm_set_dedicated_wake_irq(struct device *dev, int irq)
> {
> struct wake_irq *wirq;
> - int err;
> + int err, namelen;
> + const char *postfix = ":wakeup";
>
> if (irq < 0)
> return -EINVAL;
> @@ -184,6 +186,14 @@ int dev_pm_set_dedicated_wake_irq(struct device *dev, int irq)
> if (!wirq)
> return -ENOMEM;
>
> + namelen = strlen(dev_name(dev)) + strlen(postfix) + 1;
> + wirq->name = kzalloc(namelen, GFP_KERNEL);
> + if (!wirq->name) {
> + err = -ENOMEM;
> + goto err_free;
> + }
> +
> + snprintf(wirq->name, namelen, "%s%s", dev_name(dev), postfix);
> wirq->dev = dev;
> wirq->irq = irq;
> irq_set_status_flags(irq, IRQ_NOAUTOEN);
> @@ -196,9 +206,9 @@ int dev_pm_set_dedicated_wake_irq(struct device *dev, int irq)
> * so we use a threaded irq.
> */
> err = request_threaded_irq(irq, NULL, handle_threaded_wake_irq,
> - IRQF_ONESHOT, dev_name(dev), wirq);
> + IRQF_ONESHOT, wirq->name, wirq);
> if (err)
> - goto err_free;
> + goto err_free_name;
>
> err = dev_pm_attach_wake_irq(dev, irq, wirq);
> if (err)
> @@ -210,6 +220,8 @@ int dev_pm_set_dedicated_wake_irq(struct device *dev, int irq)
>
> err_free_irq:
> free_irq(irq, wirq);
> +err_free_name:
> + kfree(wirq->name);
> err_free:
> kfree(wirq);
>
>
I'll queue this up for 4.17 if there are no objections.
Thanks,
Rafael
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] PM / wakeirq: Add wakeup name to dedicated wake irqs
2018-02-09 16:14 [PATCH] PM / wakeirq: Add wakeup name to dedicated wake irqs Tony Lindgren
2018-02-14 10:52 ` Rafael J. Wysocki
@ 2018-02-14 11:23 ` Andy Shevchenko
2018-02-16 9:14 ` Rafael J. Wysocki
2018-02-16 9:42 ` Rafael J. Wysocki
2 siblings, 1 reply; 10+ messages in thread
From: Andy Shevchenko @ 2018-02-14 11:23 UTC (permalink / raw)
To: Tony Lindgren
Cc: Rafael J . Wysocki, Alan Stern, Linux PM,
Linux Kernel Mailing List, Brian Norris, Grygorii Strashko,
Ulf Hansson
[-- Attachment #1: Type: text/plain, Size: 414 bytes --]
On Fri, Feb 9, 2018 at 6:14 PM, Tony Lindgren <tony@atomide.com> wrote:
> This makes it easy to grep :wakeup /proc/interrupts.
I used to have another patch (not published) to provide this
information via /sys/kernel/irq.
OK, here we are:
> + namelen = strlen(dev_name(dev)) + strlen(postfix) + 1;
> + wirq->name = kzalloc(namelen, GFP_KERNEL);
kasprintf()
--
With Best Regards,
Andy Shevchenko
[-- Attachment #2: 0001-genirq-Add-wakeup-sysfs-node-to-show-IRQ-wakeup-stat.patch --]
[-- Type: text/x-patch, Size: 2328 bytes --]
From ef35d3fd789a32d85212b2bcb82f754ae244cd58 Mon Sep 17 00:00:00 2001
From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Date: Fri, 12 May 2017 20:19:32 +0300
Subject: [PATCH] genirq: Add wakeup sysfs node to show IRQ wakeup state
Surprisingly there is no simple way to see if the IRQ line in question
is wakeup source or not.
Note that wakeup might be an OOB (out-of-band) source like GPIO line
which makes things slightly more complicated.
Add a sysfs node to cover this case.
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
Documentation/ABI/testing/sysfs-kernel-irq | 7 +++++++
kernel/irq/irqdesc.c | 17 +++++++++++++++++
2 files changed, 24 insertions(+)
diff --git a/Documentation/ABI/testing/sysfs-kernel-irq b/Documentation/ABI/testing/sysfs-kernel-irq
index eb074b100986..10ad998f9411 100644
--- a/Documentation/ABI/testing/sysfs-kernel-irq
+++ b/Documentation/ABI/testing/sysfs-kernel-irq
@@ -51,3 +51,10 @@ Date: September 2016
KernelVersion: 4.9
Contact: Craig Gallek <kraig@google.com>
Description: The type of the interrupt. Either the string 'level' or 'edge'.
+
+What: /sys/kernel/irq/<irq>/wakeup
+Date: Jan 2018
+KernelVersion: 4.16
+Contact: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
+Description: The wakeup state of the interrupt. Either the string
+ 'enabled' or 'disabled'.
diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c
index 49b54e9979cc..d9ded088d336 100644
--- a/kernel/irq/irqdesc.c
+++ b/kernel/irq/irqdesc.c
@@ -210,6 +210,22 @@ static ssize_t type_show(struct kobject *kobj,
}
IRQ_ATTR_RO(type);
+static ssize_t wakeup_show(struct kobject *kobj,
+ struct kobj_attribute *attr, char *buf)
+{
+ struct irq_desc *desc = container_of(kobj, struct irq_desc, kobj);
+ ssize_t ret = 0;
+
+ raw_spin_lock_irq(&desc->lock);
+ ret = sprintf(buf, "%s\n",
+ irqd_is_wakeup_set(&desc->irq_data) ? "enabled" : "disabled");
+ raw_spin_unlock_irq(&desc->lock);
+
+ return ret;
+
+}
+IRQ_ATTR_RO(wakeup);
+
static ssize_t name_show(struct kobject *kobj,
struct kobj_attribute *attr, char *buf)
{
@@ -253,6 +269,7 @@ static struct attribute *irq_attrs[] = {
&chip_name_attr.attr,
&hwirq_attr.attr,
&type_attr.attr,
+ &wakeup_attr.attr,
&name_attr.attr,
&actions_attr.attr,
NULL
--
2.15.1
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH] PM / wakeirq: Add wakeup name to dedicated wake irqs
2018-02-14 11:23 ` Andy Shevchenko
@ 2018-02-16 9:14 ` Rafael J. Wysocki
2018-02-16 10:39 ` Andy Shevchenko
0 siblings, 1 reply; 10+ messages in thread
From: Rafael J. Wysocki @ 2018-02-16 9:14 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Tony Lindgren, Rafael J . Wysocki, Alan Stern, Linux PM,
Linux Kernel Mailing List, Brian Norris, Grygorii Strashko,
Ulf Hansson
On Wed, Feb 14, 2018 at 12:23 PM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Fri, Feb 9, 2018 at 6:14 PM, Tony Lindgren <tony@atomide.com> wrote:
>> This makes it easy to grep :wakeup /proc/interrupts.
>
> I used to have another patch (not published) to provide this
> information via /sys/kernel/irq.
>
> OK, here we are:
>
>
>> + namelen = strlen(dev_name(dev)) + strlen(postfix) + 1;
>> + wirq->name = kzalloc(namelen, GFP_KERNEL);
>
> kasprintf()
It's a bit hard to comment patches sent as attachments, but I'll try anyway. :-)
IMO it is somewhat excessive to put the entire sprintf() under a raw
spinlock and it's not even necessary.
The value can change any time after you've dropped the lock and in
particular before the function returns, so why bother with locking?
desc will not go away from under you at that point anyway.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] PM / wakeirq: Add wakeup name to dedicated wake irqs
2018-02-16 9:14 ` Rafael J. Wysocki
@ 2018-02-16 10:39 ` Andy Shevchenko
2018-02-16 10:54 ` Rafael J. Wysocki
2018-02-16 16:54 ` Tony Lindgren
0 siblings, 2 replies; 10+ messages in thread
From: Andy Shevchenko @ 2018-02-16 10:39 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Tony Lindgren, Rafael J . Wysocki, Alan Stern, Linux PM,
Linux Kernel Mailing List, Brian Norris, Grygorii Strashko,
Ulf Hansson
On Fri, Feb 16, 2018 at 11:14 AM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Wed, Feb 14, 2018 at 12:23 PM, Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
>> On Fri, Feb 9, 2018 at 6:14 PM, Tony Lindgren <tony@atomide.com> wrote:
>>> This makes it easy to grep :wakeup /proc/interrupts.
>>
>> I used to have another patch (not published) to provide this
>> information via /sys/kernel/irq.
>>
>> OK, here we are:
> It's a bit hard to comment patches sent as attachments, but I'll try anyway. :-)
Sorry, I was thinking that Tony may test it first to see if it does
what he wants.
Of course I would send normally in case it's an expected approach.
> IMO it is somewhat excessive to put the entire sprintf() under a raw
> spinlock and it's not even necessary.
It's a copy'n'paste of from the rest of functions there.
> The value can change any time after you've dropped the lock and in
> particular before the function returns, so why bother with locking?
> desc will not go away from under you at that point anyway.
IIRC descriptor's content might be changed, or descriptor itself might
be gone (potential crash).
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] PM / wakeirq: Add wakeup name to dedicated wake irqs
2018-02-16 10:39 ` Andy Shevchenko
@ 2018-02-16 10:54 ` Rafael J. Wysocki
2018-02-16 14:52 ` Andy Shevchenko
2018-02-16 16:54 ` Tony Lindgren
1 sibling, 1 reply; 10+ messages in thread
From: Rafael J. Wysocki @ 2018-02-16 10:54 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Rafael J. Wysocki, Tony Lindgren, Rafael J . Wysocki, Alan Stern,
Linux PM, Linux Kernel Mailing List, Brian Norris,
Grygorii Strashko, Ulf Hansson
On Fri, Feb 16, 2018 at 11:39 AM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Fri, Feb 16, 2018 at 11:14 AM, Rafael J. Wysocki <rafael@kernel.org> wrote:
>> On Wed, Feb 14, 2018 at 12:23 PM, Andy Shevchenko
>> <andy.shevchenko@gmail.com> wrote:
>>> On Fri, Feb 9, 2018 at 6:14 PM, Tony Lindgren <tony@atomide.com> wrote:
>>>> This makes it easy to grep :wakeup /proc/interrupts.
>>>
>>> I used to have another patch (not published) to provide this
>>> information via /sys/kernel/irq.
>>>
>>> OK, here we are:
>
>> It's a bit hard to comment patches sent as attachments, but I'll try anyway. :-)
>
> Sorry, I was thinking that Tony may test it first to see if it does
> what he wants.
> Of course I would send normally in case it's an expected approach.
>
>> IMO it is somewhat excessive to put the entire sprintf() under a raw
>> spinlock and it's not even necessary.
>
> It's a copy'n'paste of from the rest of functions there.
Fair enough. :-)
>> The value can change any time after you've dropped the lock and in
>> particular before the function returns, so why bother with locking?
>> desc will not go away from under you at that point anyway.
>
> IIRC descriptor's content might be changed, or descriptor itself might
> be gone (potential crash).
No, desc cannot go away at this point AFAICS due to the kernfs
refcounting. And the lock is *inside* of the desc object anyway, so
it doesn't help really against that.
The contents may change, but so what?
Effectively, you read an int and reading an int is atomic. It may
change after that, but the lock doesn't prevent it from changing. It
only prevents the change from being applied to it before you drop the
lock, but why do you care?
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] PM / wakeirq: Add wakeup name to dedicated wake irqs
2018-02-16 10:54 ` Rafael J. Wysocki
@ 2018-02-16 14:52 ` Andy Shevchenko
0 siblings, 0 replies; 10+ messages in thread
From: Andy Shevchenko @ 2018-02-16 14:52 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Tony Lindgren, Rafael J . Wysocki, Alan Stern, Linux PM,
Linux Kernel Mailing List, Brian Norris, Grygorii Strashko,
Ulf Hansson
On Fri, Feb 16, 2018 at 12:54 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Fri, Feb 16, 2018 at 11:39 AM, Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
>> On Fri, Feb 16, 2018 at 11:14 AM, Rafael J. Wysocki <rafael@kernel.org> wrote:
>>> On Wed, Feb 14, 2018 at 12:23 PM, Andy Shevchenko
>>> <andy.shevchenko@gmail.com> wrote:
>>> IMO it is somewhat excessive to put the entire sprintf() under a raw
>>> spinlock and it's not even necessary.
>>
>> It's a copy'n'paste of from the rest of functions there.
>
> Fair enough. :-)
>>> The value can change any time after you've dropped the lock and in
>>> particular before the function returns, so why bother with locking?
>>> desc will not go away from under you at that point anyway.
>>
>> IIRC descriptor's content might be changed, or descriptor itself might
>> be gone (potential crash).
>
> No, desc cannot go away at this point AFAICS due to the kernfs
> refcounting. And the lock is *inside* of the desc object anyway, so
> it doesn't help really against that.
Oh, indeed.
> The contents may change, but so what?
>
> Effectively, you read an int and reading an int is atomic. It may
> change after that, but the lock doesn't prevent it from changing. It
> only prevents the change from being applied to it before you drop the
> lock, but why do you care?
So, with explanations above, perhaps we can produce the patch to
remove those locks from the rest?
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] PM / wakeirq: Add wakeup name to dedicated wake irqs
2018-02-16 10:39 ` Andy Shevchenko
2018-02-16 10:54 ` Rafael J. Wysocki
@ 2018-02-16 16:54 ` Tony Lindgren
1 sibling, 0 replies; 10+ messages in thread
From: Tony Lindgren @ 2018-02-16 16:54 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Rafael J. Wysocki, Rafael J . Wysocki, Alan Stern, Linux PM,
Linux Kernel Mailing List, Brian Norris, Grygorii Strashko,
Ulf Hansson
* Andy Shevchenko <andy.shevchenko@gmail.com> [180216 10:40]:
> Sorry, I was thinking that Tony may test it first to see if it does
> what he wants.
Well using dev_name() for the wakeirq name might still conflict
with the device IO interrupt. But yeah I'll give it a try.
Regards,
Tony
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] PM / wakeirq: Add wakeup name to dedicated wake irqs
2018-02-09 16:14 [PATCH] PM / wakeirq: Add wakeup name to dedicated wake irqs Tony Lindgren
2018-02-14 10:52 ` Rafael J. Wysocki
2018-02-14 11:23 ` Andy Shevchenko
@ 2018-02-16 9:42 ` Rafael J. Wysocki
2018-02-16 16:42 ` Tony Lindgren
2 siblings, 1 reply; 10+ messages in thread
From: Rafael J. Wysocki @ 2018-02-16 9:42 UTC (permalink / raw)
To: Tony Lindgren
Cc: Rafael J . Wysocki, Alan Stern, Linux PM,
Linux Kernel Mailing List, Brian Norris, Grygorii Strashko,
Ulf Hansson
On Fri, Feb 9, 2018 at 5:14 PM, Tony Lindgren <tony@atomide.com> wrote:
> This makes it easy to grep :wakeup /proc/interrupts.
>
> Cc: Brian Norris <briannorris@chromium.org>
> Cc: Grygorii Strashko <grygorii.strashko@ti.com>
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> Suggested-by: Jeffy Chen <jeffy.chen@rock-chips.com>
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
> drivers/base/power/power.h | 1 +
> drivers/base/power/wakeirq.c | 18 +++++++++++++++---
> 2 files changed, 16 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/base/power/power.h b/drivers/base/power/power.h
> --- a/drivers/base/power/power.h
> +++ b/drivers/base/power/power.h
> @@ -31,6 +31,7 @@ struct wake_irq {
> struct device *dev;
> unsigned int status;
> int irq;
> + char *name;
> };
>
> extern void dev_pm_arm_wake_irq(struct wake_irq *wirq);
> diff --git a/drivers/base/power/wakeirq.c b/drivers/base/power/wakeirq.c
> --- a/drivers/base/power/wakeirq.c
> +++ b/drivers/base/power/wakeirq.c
> @@ -112,6 +112,7 @@ void dev_pm_clear_wake_irq(struct device *dev)
> free_irq(wirq->irq, wirq);
> wirq->status &= ~WAKE_IRQ_DEDICATED_MASK;
> }
> + kfree(wirq->name);
> kfree(wirq);
> }
> EXPORT_SYMBOL_GPL(dev_pm_clear_wake_irq);
> @@ -175,7 +176,8 @@ static irqreturn_t handle_threaded_wake_irq(int irq, void *_wirq)
> int dev_pm_set_dedicated_wake_irq(struct device *dev, int irq)
> {
> struct wake_irq *wirq;
> - int err;
> + int err, namelen;
> + const char *postfix = ":wakeup";
>
> if (irq < 0)
> return -EINVAL;
> @@ -184,6 +186,14 @@ int dev_pm_set_dedicated_wake_irq(struct device *dev, int irq)
> if (!wirq)
> return -ENOMEM;
>
> + namelen = strlen(dev_name(dev)) + strlen(postfix) + 1;
> + wirq->name = kzalloc(namelen, GFP_KERNEL);
> + if (!wirq->name) {
> + err = -ENOMEM;
> + goto err_free;
> + }
> +
> + snprintf(wirq->name, namelen, "%s%s", dev_name(dev), postfix);
As Andy said you can do
wirq->name = kasprintf(GFP_KERNEL, "%s:wakeup", dev_name(dev));
if (!wirq->name) {
err = -ENOMEM;
goto err_free;
}
here and it will allocate the buffer for you.
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] PM / wakeirq: Add wakeup name to dedicated wake irqs
2018-02-16 9:42 ` Rafael J. Wysocki
@ 2018-02-16 16:42 ` Tony Lindgren
0 siblings, 0 replies; 10+ messages in thread
From: Tony Lindgren @ 2018-02-16 16:42 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Rafael J . Wysocki, Alan Stern, Linux PM,
Linux Kernel Mailing List, Brian Norris, Grygorii Strashko,
Ulf Hansson
* Rafael J. Wysocki <rafael@kernel.org> [180216 09:42]:
> On Fri, Feb 9, 2018 at 5:14 PM, Tony Lindgren <tony@atomide.com> wrote:
> > This makes it easy to grep :wakeup /proc/interrupts.
> > diff --git a/drivers/base/power/wakeirq.c b/drivers/base/power/wakeirq.c
> > --- a/drivers/base/power/wakeirq.c
> > +++ b/drivers/base/power/wakeirq.c
> > @@ -112,6 +112,7 @@ void dev_pm_clear_wake_irq(struct device *dev)
> > free_irq(wirq->irq, wirq);
> > wirq->status &= ~WAKE_IRQ_DEDICATED_MASK;
> > }
> > + kfree(wirq->name);
> > kfree(wirq);
> > }
> > EXPORT_SYMBOL_GPL(dev_pm_clear_wake_irq);
> > @@ -175,7 +176,8 @@ static irqreturn_t handle_threaded_wake_irq(int irq, void *_wirq)
> > int dev_pm_set_dedicated_wake_irq(struct device *dev, int irq)
> > {
> > struct wake_irq *wirq;
> > - int err;
> > + int err, namelen;
> > + const char *postfix = ":wakeup";
> >
> > if (irq < 0)
> > return -EINVAL;
> > @@ -184,6 +186,14 @@ int dev_pm_set_dedicated_wake_irq(struct device *dev, int irq)
> > if (!wirq)
> > return -ENOMEM;
> >
> > + namelen = strlen(dev_name(dev)) + strlen(postfix) + 1;
> > + wirq->name = kzalloc(namelen, GFP_KERNEL);
> > + if (!wirq->name) {
> > + err = -ENOMEM;
> > + goto err_free;
> > + }
> > +
> > + snprintf(wirq->name, namelen, "%s%s", dev_name(dev), postfix);
>
> As Andy said you can do
>
> wirq->name = kasprintf(GFP_KERNEL, "%s:wakeup", dev_name(dev));
> if (!wirq->name) {
> err = -ENOMEM;
> goto err_free;
> }
>
> here and it will allocate the buffer for you.
Yeah great, will update and repost next week. I'll also
give Andy's patch a try, sounds like we may want to do
both though.
Regards,
Tony
^ permalink raw reply [flat|nested] 10+ messages in thread