[PATCH] lan78xx: Don't reset the interface on open

[PATCH] lan78xx: Don't reset the interface on open

[Phil Elwell]

Commit 92571a1aae40 ("lan78xx: Connect phy early") moves the PHY
initialisation into lan78xx_probe, but lan78xx_open subsequently calls
lan78xx_reset. As well as forcing a second round of link negotiation,
this reset frequently prevents the phy interrupt from being generated
(even though the link is up), rendering the interface unusable.

Fix this issue by removing the lan78xx_reset call from lan78xx_open.

Fixes: 92571a1aae40 ("lan78xx: Connect phy early")
Signed-off-by: Phil Elwell <phil@raspberrypi.org>
---
 drivers/net/usb/lan78xx.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
index aff105f..108f04a 100644
--- a/drivers/net/usb/lan78xx.c
+++ b/drivers/net/usb/lan78xx.c
@@ -2514,10 +2514,6 @@ static int lan78xx_open(struct net_device *net)
 	if (ret < 0)
 		goto out;
 
-	ret = lan78xx_reset(dev);
-	if (ret < 0)
-		goto done;
-
 	phy_start(net->phydev);
 
 	netif_dbg(dev, ifup, dev->net, "phy initialised successfully");
-- 
2.7.4

RE: [PATCH] lan78xx: Don't reset the interface on open

[Nisar.Sayed]

Thanks Phil, for identifying the issues.

> -	ret = lan78xx_reset(dev);
> -	if (ret < 0)
> -		goto done;
> -
>  	phy_start(net->phydev);
> 
>  	netif_dbg(dev, ifup, dev->net, "phy initialised successfully");
> --

You may need to start the interrupts before "phy_start" instead of suppressing call to "lan78xx_reset".

+             if (dev->domain_data.phyirq > 0)
+                             phy_start_interrupts(dev->net->phydev);

- Nisar

Re: [PATCH] lan78xx: Don't reset the interface on open

[Phil Elwell]

Hi Nisar,

On 10/04/2018 15:16, Nisar.Sayed@microchip.com wrote:
> Thanks Phil, for identifying the issues.
> 
>> -	ret = lan78xx_reset(dev);
>> -	if (ret < 0)
>> -		goto done;
>> -
>>  	phy_start(net->phydev);
>>
>>  	netif_dbg(dev, ifup, dev->net, "phy initialised successfully");
>> --
> 
> You may need to start the interrupts before "phy_start" instead of suppressing call to "lan78xx_reset".
> 
> +             if (dev->domain_data.phyirq > 0)
> +                             phy_start_interrupts(dev->net->phydev);

Shouldn't phy_connect_direct, called from lan78xx_phy_init, already have enabled interrupts for us?

This patch addresses two problems - time wasted by renegotiating the link after the reset and the
missed interrupt - and I'd like both to be fixed. Unless you can come up with a good reason for
performing the reset from the open handler I think it should be removed.

Phil

RE: [PATCH] lan78xx: Don't reset the interface on open

[Nisar.Sayed]

Hi Phil,

> Hi Nisar,
> 
> On 10/04/2018 15:16, Nisar.Sayed@microchip.com wrote:
> > Thanks Phil, for identifying the issues.
> >
> >> -	ret = lan78xx_reset(dev);
> >> -	if (ret < 0)
> >> -		goto done;
> >> -
> >>  	phy_start(net->phydev);
> >>
> >>  	netif_dbg(dev, ifup, dev->net, "phy initialised successfully");
> >> --
> >
> > You may need to start the interrupts before "phy_start" instead of
> suppressing call to "lan78xx_reset".
> >
> > +             if (dev->domain_data.phyirq > 0)
> > +                             phy_start_interrupts(dev->net->phydev);
> 
> Shouldn't phy_connect_direct, called from lan78xx_phy_init, already have
> enabled interrupts for us?
> 
> This patch addresses two problems - time wasted by renegotiating the link
> after the reset and the missed interrupt - and I'd like both to be fixed. Unless
> you can come up with a good reason for performing the reset from the open
> handler I think it should be removed.
> 
> Phil

Thanks, we have verified suspected test cases and these are passed, the changes are good to go.

- Nisar

Re: [PATCH] lan78xx: Don't reset the interface on open

[David Miller]

From: Phil Elwell <phil@raspberrypi.org>
Date: Tue, 10 Apr 2018 13:18:25 +0100

> Commit 92571a1aae40 ("lan78xx: Connect phy early") moves the PHY
> initialisation into lan78xx_probe, but lan78xx_open subsequently calls
> lan78xx_reset. As well as forcing a second round of link negotiation,
> this reset frequently prevents the phy interrupt from being generated
> (even though the link is up), rendering the interface unusable.
> 
> Fix this issue by removing the lan78xx_reset call from lan78xx_open.
> 
> Fixes: 92571a1aae40 ("lan78xx: Connect phy early")
> Signed-off-by: Phil Elwell <phil@raspberrypi.org>

Applied and queued up for -stable, thanks.