netdev - Re: [PATCH net] net: phy: avoid clearing PHY interrupts twice in irq handler

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 for Android: free password hash cracker in your pocket

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

Message-ID: <c2928823-da08-0321-6917-1481aab79e09@gmail.com>
Date:   Wed, 4 Mar 2020 13:13:42 +0100
From:   Heiner Kallweit <hkallweit1@...il.com>
To:     Michael Walle <michael@...le.cc>
Cc:     Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>,
        Russell King - ARM Linux <linux@...linux.org.uk>,
        David Miller <davem@...emloft.net>, netdev@...r.kernel.org
Subject: Re: [PATCH net] net: phy: avoid clearing PHY interrupts twice in irq
 handler

On 02.03.2020 00:20, Heiner Kallweit wrote:
> On 01.03.2020 23:52, Michael Walle wrote:
>> Am 2020-03-01 21:36, schrieb Heiner Kallweit:
>>> On all PHY drivers that implement did_interrupt() reading the interrupt
>>> status bits clears them. This means we may loose an interrupt that
>>> is triggered between calling did_interrupt() and phy_clear_interrupt().
>>> As part of the fix make it a requirement that did_interrupt() clears
>>> the interrupt.
>>
>> Looks good. But how would you use did_interrupt() and handle_interrupt()
>> together? I guess you can't. At least not if handle_interrupt() has
>> to read the pending bits again. So you'd have to handle custom
>> interrupts in did_interrupt(). Any idea how to solve that?
>>
>> [I know, this is only about fixing the lost interrupts.]
>>
> Right, this one is meant for stable to fix the issue with the potentially
> lost interrupts. Based on it I will submit a patch for net-next that
> tackles the issue that did_interrupt() has to read (and therefore clear)
> irq status bits and therefore makes them unusable for handle_interrupt().
> The basic idea is that did_interrupt() is called only if handle_interrupt()
> isn't implemented. handle_interrupt() has to include the did_interrupt
> functionality. It can read the irq status once and store it in a variable
> for later use.
> 
In case you wait for this patch to base further own work on it:
I'm waiting for next merge of net into net-next, because my patch will
apply cleanly only after that. This merge should happen in the next days.

>> -michael
>>
>>>
>>> The Fixes tag refers to the first commit where the patch applies
>>> cleanly.
>>>
>>> Fixes: 49644e68f472 ("net: phy: add callback for custom interrupt
>>> handler to struct phy_driver")
>>> Reported-by: Michael Walle <michael@...le.cc>
>>> Signed-off-by: Heiner Kallweit <hkallweit1@...il.com>
>>> ---
>>>  drivers/net/phy/phy.c | 3 ++-
>>>  include/linux/phy.h   | 1 +
>>>  2 files changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
>>> index d76e038cf..16e3fb79e 100644
>>> --- a/drivers/net/phy/phy.c
>>> +++ b/drivers/net/phy/phy.c
>>> @@ -727,7 +727,8 @@ static irqreturn_t phy_interrupt(int irq, void *phy_dat)
>>>          phy_trigger_machine(phydev);
>>>      }
>>>
>>> -    if (phy_clear_interrupt(phydev))
>>> +    /* did_interrupt() may have cleared the interrupt already */
>>> +    if (!phydev->drv->did_interrupt && phy_clear_interrupt(phydev))
>>>          goto phy_err;
>>>      return IRQ_HANDLED;
>>>
>>> diff --git a/include/linux/phy.h b/include/linux/phy.h
>>> index 80f8b2158..8b299476b 100644
>>> --- a/include/linux/phy.h
>>> +++ b/include/linux/phy.h
>>> @@ -557,6 +557,7 @@ struct phy_driver {
>>>      /*
>>>       * Checks if the PHY generated an interrupt.
>>>       * For multi-PHY devices with shared PHY interrupt pin
>>> +     * Set interrupt bits have to be cleared.
>>>       */
>>>      int (*did_interrupt)(struct phy_device *phydev);
>