Re: [PATCH net 2/2] net: usb: smsc95xx: propagate MDIO write errors

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The patch makes smsc95xx_mdio_write() return an error, but
  only one of its two callers was updated.

--

net: usb: smsc95xx: propagate MDIO write errors

This commit changes smsc95xx_mdio_write() from void to int so it returns
register access errors, while still treating -ENODEV as success during
disconnect. smsc95xx_mdiobus_write() now passes that error back to the
MDIO bus instead of always returning 0.

> diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
> index f85ee5f8a5394..52035c160f4b0 100644
> --- a/drivers/net/usb/smsc95xx.c
> +++ b/drivers/net/usb/smsc95xx.c
> @@ -215,8 +215,8 @@ static int smsc95xx_mdio_read(struct usbnet *dev, int phy_id, int idx)
>  	return ret;
>  }
>  
> -static void smsc95xx_mdio_write(struct usbnet *dev, int phy_id, int idx,
> -				int regval)
> +static int smsc95xx_mdio_write(struct usbnet *dev, int phy_id, int idx,
> +			       int regval)

[Severity: Medium]
smsc95xx_mdio_write() has a second caller, smsc95xx_enter_suspend1(),
and this patch doesn't change it. Both writes there still throw away the
return value:

drivers/net/usb/smsc95xx.c:smsc95xx_enter_suspend1() {
    ...
	if (pdata->features & FEATURE_PHY_NLP_CROSSOVER)
		smsc95xx_mdio_write(dev, phy_id, PHY_EDPD_CONFIG,
				    PHY_EDPD_CONFIG_DEFAULT);

	/* enable energy detect power-down mode */
	ret = smsc95xx_mdio_read(dev, phy_id, PHY_MODE_CTRL_STS);
	if (ret < 0)
		return ret;

	ret |= MODE_CTRL_STS_EDPWRDOWN_;

	smsc95xx_mdio_write(dev, phy_id, PHY_MODE_CTRL_STS, ret);
    ...
}

Every other access in that function returns on error, including the
smsc95xx_mdio_read() between the two writes.

What happens if one of these writes fails? That could be an -EIO from
smsc95xx_phy_wait_not_busy(), or a USB error other than -ENODEV from
smsc95xx_write_reg().

In that case the function still writes PM_CTRL with PM_CTL_SUS_MODE_1
and PM_CTL_WUPS_ED_ | PM_CTL_ED_EN_. It then sets SUSPEND_SUSPEND1 in
pdata->suspend_flags and returns 0. The controller waits for an
energy-detect wake, but the PHY may not be in energy-detect power-down.

smsc95xx_suspend() gets 0 back from smsc95xx_enter_suspend1(), both from
the autosuspend path and from the WAKE_PHY path with the link down. So
the usbnet_resume() recovery at its done: label never runs.

Should smsc95xx_enter_suspend1() check these return values too, since
the subject says MDIO write errors are propagated?

Also, unlike smsc95xx_read_reg(), smsc95xx_write_reg() and
smsc95xx_phy_wait_not_busy() in this file, the new int-returning helper
isn't marked __must_check. Would adding it make the compiler flag
dropped results like these?

>  {
>  	u32 val, addr;
>  	int ret;
> @@ -255,6 +255,12 @@ static void smsc95xx_mdio_write(struct usbnet *dev, int phy_id, int idx,
>  
>  done:
>  	mutex_unlock(&dev->phy_mutex);
> +
> +	/* Ignore -ENODEV error during disconnect() */
> +	if (ret == -ENODEV)
> +		return 0;
> +
> +	return ret;
>  }

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261114050954.275495-1-artem122novak%40gmail.com