linux-kernel - Re: [PATCH v2 3/3] gpio: brcmstb: allow parent

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <b9732251-936b-4935-a586-25993881ce4e@broadcom.com>
Date: Thu, 29 Jan 2026 12:02:32 -0800
From: Florian Fainelli <florian.fainelli@...adcom.com>
To: Andy Shevchenko <andy.shevchenko@...il.com>
Cc: linux-kernel@...r.kernel.org, Doug Berger <opendmb@...il.com>,
 Broadcom internal kernel review list
 <bcm-kernel-feedback-list@...adcom.com>, Linus Walleij <linusw@...nel.org>,
 Bartosz Golaszewski <brgl@...nel.org>, Christophe Leroy
 <chleroy@...nel.org>, "open list:GPIO SUBSYSTEM"
 <linux-gpio@...r.kernel.org>,
 "moderated list:BROADCOM BCM7XXX ARM ARCHITECTURE"
 <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH v2 3/3] gpio: brcmstb: allow parent_irq to wake

On 1/29/26 06:23, Andy Shevchenko wrote:
> On Tue, Jan 27, 2026 at 11:47 PM Florian Fainelli
> <florian.fainelli@...adcom.com> wrote:
> 
>> The classic parent_wake_irq can only occur after the system has
>> been placed into a hardware managed power management state. This
>> prevents its use for waking from software managed suspend states
>> like s2idle.
>>
>> By allowing the parent_irq to be enabled for wake enabled GPIO
>> during suspend, these GPIO can now be used to wake from these
>> states. The 'suspended' boolean is introduced to support wake
>> event accounting.
> 
> ...
> 
>>          if (of_property_read_bool(np, "wakeup-source")) {
>> +               /*
>> +                * Set wakeup capability so we can process boot-time
>> +                * "wakeups" (e.g., from S5 cold boot)
> 
> While at it, add a period at the end.
> 
>> +                */
>> +               device_set_wakeup_capable(dev, true);
>> +               device_wakeup_enable(dev);
> 
>>          }
> 
> ...
> 
>> +       /* disable interrupts */
> 
> Still the comment is useless.
> 
>> +       if (priv->parent_irq > 0)
>> +               disable_irq(priv->parent_irq);
> 
> And looking more at this, I don't see why we even need the check. Does
> the code WARNs or so when there is no parent_irq available?
> 
> *Yes, I saw this is the original code, perhaps can be addressed in a follow up.
> 
> ...
> 
>> +       /* disable interrupts while we save the masks */
> 
>> +       if (priv->parent_irq > 0)
> 
> Ditto.
> 
>> +               disable_irq(priv->parent_irq);
> 
> ...
> 
>> +       /* disable interrupts while we restore the masks */
>> +       if (priv->parent_wake_irq)
> 
> Ditto.
> 
>> +               disable_irq(priv->parent_irq);
> 
> ...
> 
>> +       /* re-enable interrupts */
>> +       if (priv->parent_irq > 0)
> 
> Same here.
> 
>>                  enable_irq(priv->parent_irq);
> 
> ...
> 
> All we are diving into is the 2 questions:
> - is 0 on the particular platform an IRQ number and there is no sparse
> tree enabled?
> - is maple tree implementation clever enough to not crash (or have
> side effects) when we ask for a non-existing index?
> 
> Anyway, this can be done later on.

OK, I will remove the superfluous comments, add punctuation where 
necessary and respin (removing patch #1 since it was applied already).

Thank you!
-- 
Florian