net: phy: realtek: Dummy IRQ calls for RTL8366RB

Message ID 20190223023639.27038-1-linus.walleij@linaro.org
State New
Headers show
Series
  • net: phy: realtek: Dummy IRQ calls for RTL8366RB
Related show

Commit Message

Linus Walleij Feb. 23, 2019, 2:36 a.m.
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

Comments

Heiner Kallweit Feb. 23, 2019, 9 a.m. | #1
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,

>  	},

>
Andrew Lunn Feb. 23, 2019, 3:17 p.m. | #2
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
Linus Walleij Feb. 23, 2019, 11:57 p.m. | #3
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
Andrew Lunn Feb. 24, 2019, 12:07 a.m. | #4
> 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
Linus Walleij Feb. 24, 2019, 12:14 a.m. | #5
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

Patch

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,
 	},