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

[David Miller]

From: Phil Elwell 
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 

Applied and queued up for -stable, thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Nisar.Sayed]

Hi Phil,

> Hi Nisar,
> 
> On 10/04/2018 15:16, nisar.sa...@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

[Phil Elwell]

Hi Nisar,

On 10/04/2018 15:16, nisar.sa...@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
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[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 
---
 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

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html