| Message ID | CAHp75VcSRxLZJxmqXWpAGypvZa6W2cEAiTF2muJWqdaOYu_TpA@mail.gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
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.
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).
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?
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?
* 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
On Friday, February 16, 2018 3:52:22 PM CET Andy Shevchenko wrote: > 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? Well, I guess so. I'm not the maintainer of that code, however. :-)
* Andy Shevchenko <andy.shevchenko@gmail.com> [180214 10:24]: > 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. Finally got around trying this one. Yeah it's nice and it seems that this is how we could also allow add support for showing the time when the wakeirq triggered and was actually seen by the irq subsystem. I think that's what people have been after for a while :) It seems we should do both /sys/kernel/irq/*/wakeup and also have a name for the dedicated wakeirqs. I'll send out my kasprintf updated patch shortly. Regards, Tony
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