| Message ID | 20190223023639.27038-1-linus.walleij@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | net: phy: realtek: Dummy IRQ calls for RTL8366RB | expand |
On 23.02.2019 03:36, Linus Walleij wrote: > This fixes a regression introduced by > commit 0d2e778e38e0ddffab4bb2b0e9ed2ad5165c4bf7 > "net: phy: replace PHY_HAS_INTERRUPT with a check for > config_intr and ack_interrupt". > > This assumes that a PHY cannot trigger interrupt unless > it has .config_intr() or .ack_interrupt() implemented. > A later patch makes the code assume both need to be > implemented for interrupts to be present. > > But this PHY (which is inside a DSA) will happily > fire interrupts without either callback. > > Implement a dummy callback for .config_intr() and > .ack_interrupt() as a workaround. > > Tested on the RTL8366RB on D-Link DIR-685. > > Fixes: 0d2e778e38e0 ("net: phy: replace PHY_HAS_INTERRUPT with a check for config_intr and ack_interrupt") > Cc: Heiner Kallweit <hkallweit1@gmail.com> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > --- > drivers/net/phy/realtek.c | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c > index c6010fb1aa0f..31372be44570 100644 > --- a/drivers/net/phy/realtek.c > +++ b/drivers/net/phy/realtek.c > @@ -211,6 +211,17 @@ static int rtl8366rb_config_init(struct phy_device *phydev) > return ret; > } > > +static int rtl8366rb_ack_interrupt(struct phy_device *phydev) > +{ > + return 0; > +} > + > +static int rtl8366rb_config_intr(struct phy_device *phydev) > +{ > + /* Dummy function to make sure we get interrupts */ > + return 0; > +} > + Maybe we should place these dummy calls in the core. We have this for other cases already, see e.g. genphy_no_soft_reset. Then these dummies can be re-used by other drivers that may have the need to do so. Names could be: genphy_no_ack_interrupt genphy_no_config_intr > static struct phy_driver realtek_drvs[] = { > { > PHY_ID_MATCH_EXACT(0x00008201), > @@ -282,6 +293,8 @@ static struct phy_driver realtek_drvs[] = { > .name = "RTL8366RB Gigabit Ethernet", > .features = PHY_GBIT_FEATURES, > .config_init = &rtl8366rb_config_init, > + .ack_interrupt = &rtl8366rb_ack_interrupt, > + .config_intr = &rtl8366rb_config_intr, > .suspend = genphy_suspend, > .resume = genphy_resume, > }, >
On Sat, Feb 23, 2019 at 03:36:39AM +0100, Linus Walleij wrote: > This fixes a regression introduced by > commit 0d2e778e38e0ddffab4bb2b0e9ed2ad5165c4bf7 > "net: phy: replace PHY_HAS_INTERRUPT with a check for > config_intr and ack_interrupt". > > This assumes that a PHY cannot trigger interrupt unless > it has .config_intr() or .ack_interrupt() implemented. > A later patch makes the code assume both need to be > implemented for interrupts to be present. > > But this PHY (which is inside a DSA) will happily > fire interrupts without either callback. Hi Linus Can you disable these interrupts? If you have dummy implementations, what is clearing the interrupt? Andrew
On Sat, Feb 23, 2019 at 4:17 PM Andrew Lunn <andrew@lunn.ch> wrote: > On Sat, Feb 23, 2019 at 03:36:39AM +0100, Linus Walleij wrote: > > This fixes a regression introduced by > > commit 0d2e778e38e0ddffab4bb2b0e9ed2ad5165c4bf7 > > "net: phy: replace PHY_HAS_INTERRUPT with a check for > > config_intr and ack_interrupt". > > > > This assumes that a PHY cannot trigger interrupt unless > > it has .config_intr() or .ack_interrupt() implemented. > > A later patch makes the code assume both need to be > > implemented for interrupts to be present. > > > > But this PHY (which is inside a DSA) will happily > > fire interrupts without either callback. > > Hi Linus > > Can you disable these interrupts? > > If you have dummy implementations, what is clearing the interrupt? They are handled by the irqchip mask/unmask inside the RTL8366RB, see: drivers/net/dsa/rtl8366rb.c So as soon as the phy core request the threaded IRQ the irqchip will deal with this business on its own. How exactly the RTL8366RB IRQ machine looks inside I doubt even Realtek knows themselves, but from my experiements, they seem all edge triggered, and the irq will be raised every time an edge occurse (such as inserting or removing the cable). The "ACK" happens in hardware when we read the status register in the nested interrupt handler in rtl8366rb_irq() so no further registers need to be accessed. Yours, Linus Walleij
> They are handled by the irqchip mask/unmask inside > the RTL8366RB, see: > drivers/net/dsa/rtl8366rb.c > > So as soon as the phy core request the threaded IRQ > the irqchip will deal with this business on its own. > > How exactly the RTL8366RB IRQ machine looks inside > I doubt even Realtek knows themselves, but from > my experiements, they seem all edge triggered, > and the irq will be raised every time an edge occurse > (such as inserting or removing the cable). The "ACK" > happens in hardware when we read the status register > in the nested interrupt handler in rtl8366rb_irq() so no > further registers need to be accessed. Hi Linus Thanks for the explanation. So dummy functions are fine in this case. However, in general, i don't think dummy functions will work for a PHY driver, and may lead to interrupt storms. So it might be better to have them in the driver, not the core, with comments about why they are safe. Andrew
On Sun, Feb 24, 2019 at 1:07 AM Andrew Lunn <andrew@lunn.ch> wrote: > > They are handled by the irqchip mask/unmask inside > > the RTL8366RB, see: > > drivers/net/dsa/rtl8366rb.c > > > > So as soon as the phy core request the threaded IRQ > > the irqchip will deal with this business on its own. > > > > How exactly the RTL8366RB IRQ machine looks inside > > I doubt even Realtek knows themselves, but from > > my experiements, they seem all edge triggered, > > and the irq will be raised every time an edge occurse > > (such as inserting or removing the cable). The "ACK" > > happens in hardware when we read the status register > > in the nested interrupt handler in rtl8366rb_irq() so no > > further registers need to be accessed. > > Hi Linus > > Thanks for the explanation. So dummy functions are fine in this case. > > However, in general, i don't think dummy functions will work for a PHY > driver, and may lead to interrupt storms. So it might be better to > have them in the driver, not the core, with comments about why they > are safe. OK shall I just send a v3 moving them back to the driver so we avoid confusion on which version should be applied? Yours, Linus Walleij
diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c index c6010fb1aa0f..31372be44570 100644 --- a/drivers/net/phy/realtek.c +++ b/drivers/net/phy/realtek.c @@ -211,6 +211,17 @@ static int rtl8366rb_config_init(struct phy_device *phydev) return ret; } +static int rtl8366rb_ack_interrupt(struct phy_device *phydev) +{ + return 0; +} + +static int rtl8366rb_config_intr(struct phy_device *phydev) +{ + /* Dummy function to make sure we get interrupts */ + return 0; +} + static struct phy_driver realtek_drvs[] = { { PHY_ID_MATCH_EXACT(0x00008201), @@ -282,6 +293,8 @@ static struct phy_driver realtek_drvs[] = { .name = "RTL8366RB Gigabit Ethernet", .features = PHY_GBIT_FEATURES, .config_init = &rtl8366rb_config_init, + .ack_interrupt = &rtl8366rb_ack_interrupt, + .config_intr = &rtl8366rb_config_intr, .suspend = genphy_suspend, .resume = genphy_resume, },
This fixes a regression introduced by commit 0d2e778e38e0ddffab4bb2b0e9ed2ad5165c4bf7 "net: phy: replace PHY_HAS_INTERRUPT with a check for config_intr and ack_interrupt". This assumes that a PHY cannot trigger interrupt unless it has .config_intr() or .ack_interrupt() implemented. A later patch makes the code assume both need to be implemented for interrupts to be present. But this PHY (which is inside a DSA) will happily fire interrupts without either callback. Implement a dummy callback for .config_intr() and .ack_interrupt() as a workaround. Tested on the RTL8366RB on D-Link DIR-685. Fixes: 0d2e778e38e0 ("net: phy: replace PHY_HAS_INTERRUPT with a check for config_intr and ack_interrupt") Cc: Heiner Kallweit <hkallweit1@gmail.com> Signed-off-by: Linus Walleij <linus.walleij@linaro.org> --- drivers/net/phy/realtek.c | 13 +++++++++++++ 1 file changed, 13 insertions(+) -- 2.20.1