| Message ID | 2f468a46-e966-761c-8466-51601d8f11a3@gmail.com |
|---|---|
| State | Accepted |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net] net: phy: avoid clearing PHY interrupts twice in irq handler | expand |
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.] -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@walle.cc> > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.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);
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. > -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@walle.cc> >> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.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);
From: Heiner Kallweit <hkallweit1@gmail.com> Date: Sun, 1 Mar 2020 21:36:09 +0100 > 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. > > 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@walle.cc> > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> Applied and queued up for -stable, thanks.
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@walle.cc> >>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.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); >
Am 2020-03-04 13:13, schrieb Heiner Kallweit: > 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. Ok, thanks for the information. I have enough other things to do ;) -michael
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);
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. 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@walle.cc> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- drivers/net/phy/phy.c | 3 ++- include/linux/phy.h | 1 + 2 files changed, 3 insertions(+), 1 deletion(-)