Re: [PATCH net] net: phy: smsc: Disable Energy Detect Power-Down in interrupt mode

[Florian Fainelli <f.fainelli@gmail.com>]

On 6/20/22 04:04, Lukas Wunner wrote:
> Simon reports that if two LAN9514 USB adapters are directly connected
> without an intermediate switch, the link fails to come up and link LEDs
> remain dark.  The issue was introduced by commit 1ce8b37241ed ("usbnet:
> smsc95xx: Forward PHY interrupts to PHY driver to avoid polling").
> 
> The PHY suffers from a known erratum wherein link detection becomes
> unreliable if Energy Detect Power-Down is used.  In poll mode, the
> driver works around the erratum by briefly disabling EDPD for 640 msec
> to detect a neighbor, then re-enabling it to save power.
> 
> In interrupt mode, no interrupt is signaled if EDPD is used by both link
> partners, so it must not be enabled at all.
> 
> We'll recoup the power savings by enabling SUSPEND1 mode on affected
> LAN95xx chips in a forthcoming commit.
> 
> Fixes: 1ce8b37241ed ("usbnet: smsc95xx: Forward PHY interrupts to PHY driver to avoid polling")
> Reported-by: Simon Han <z.han@kunbus.com>
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> ---
>   drivers/net/phy/smsc.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/phy/smsc.c b/drivers/net/phy/smsc.c
> index 1b54684b68a0..96d3c40932d8 100644
> --- a/drivers/net/phy/smsc.c
> +++ b/drivers/net/phy/smsc.c
> @@ -110,7 +110,7 @@ static int smsc_phy_config_init(struct phy_device *phydev)
>   	struct smsc_phy_priv *priv = phydev->priv;
>   	int rc;
>   
> -	if (!priv->energy_enable)
> +	if (!priv->energy_enable || phydev->irq != PHY_POLL)

phy_interrupt_is_valid() may be more appropriate, since you are assuming 
that you either have PHY_POLL or valid "external" PHY interrupt but 
there is also the special case of PHY_MAC_INTERRUPT that is not dealt with.

>   		return 0;
>   
>   	rc = phy_read(phydev, MII_LAN83C185_CTRL_STATUS);
> @@ -210,6 +210,8 @@ static int lan95xx_config_aneg_ext(struct phy_device *phydev)
>    * response on link pulses to detect presence of plugged Ethernet cable.
>    * The Energy Detect Power-Down mode is enabled again in the end of procedure to
>    * save approximately 220 mW of power if cable is unplugged.
> + * The workaround is only applicable to poll mode. Energy Detect Power-Down may
> + * not be used in interrupt mode lest link change detection becomes unreliable.
>    */
>   static int lan87xx_read_status(struct phy_device *phydev)
>   {
> @@ -217,7 +219,7 @@ static int lan87xx_read_status(struct phy_device *phydev)
>   
>   	int err = genphy_read_status(phydev);
>   
> -	if (!phydev->link && priv->energy_enable) {
> +	if (!phydev->link && priv->energy_enable && phydev->irq == PHY_POLL) {

phy_polling_mode()?

>   		/* Disable EDPD to wake up PHY */
>   		int rc = phy_read(phydev, MII_LAN83C185_CTRL_STATUS);
>   		if (rc < 0)


-- 
Florian