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 <[email protected]>
---
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
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
Hi Nisar,
On 10/04/2018 15:16, [email protected] 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
Hi Phil,
> Hi Nisar,
>
> On 10/04/2018 15:16, [email protected] 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
From: Phil Elwell <[email protected]>
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 <[email protected]>
Applied and queued up for -stable, thanks.