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