[PATCH] smsc95xx: Ignore -ENODEV errors when device is unplugged

[PATCH] smsc95xx: Ignore -ENODEV errors when device is unplugged

[Fabio Estevam]

From: Fabio Estevam <festevam@denx.de>

According to Documentation/driver-api/usb/URB.rst when a device
is unplugged usb_submit_urb() returns -ENODEV.

This error code propagates all the way up to usbnet_read_cmd() and
usbnet_write_cmd() calls inside the smsc95xx.c driver during
Ethernet cable unplug, unbind or reboot.

This causes the following errors to be shown on reboot, for example:

ci_hdrc ci_hdrc.1: remove, state 1
usb usb2: USB disconnect, device number 1
usb 2-1: USB disconnect, device number 2
usb 2-1.1: USB disconnect, device number 3
smsc95xx 2-1.1:1.0 eth1: unregister 'smsc95xx' usb-ci_hdrc.1-1.1, smsc95xx USB 2.0 Ethernet
smsc95xx 2-1.1:1.0 eth1: Failed to read reg index 0x00000114: -19
smsc95xx 2-1.1:1.0 eth1: Error reading MII_ACCESS
smsc95xx 2-1.1:1.0 eth1: __smsc95xx_mdio_read: MII is busy
smsc95xx 2-1.1:1.0 eth1: Failed to read reg index 0x00000114: -19
smsc95xx 2-1.1:1.0 eth1: Error reading MII_ACCESS
smsc95xx 2-1.1:1.0 eth1: __smsc95xx_mdio_read: MII is busy
smsc95xx 2-1.1:1.0 eth1: hardware isn't capable of remote wakeup
usb 2-1.4: USB disconnect, device number 4
ci_hdrc ci_hdrc.1: USB bus 2 deregistered
ci_hdrc ci_hdrc.0: remove, state 4
usb usb1: USB disconnect, device number 1
ci_hdrc ci_hdrc.0: USB bus 1 deregistered
imx2-wdt 30280000.watchdog: Device shutdown: Expect reboot!
reboot: Restarting system

Ignore the -ENODEV errors inside __smsc95xx_mdio_read() and
__smsc95xx_phy_wait_not_busy() and do not print error messages
when -ENODEV is returned.

Signed-off-by: Fabio Estevam <festevam@denx.de>
---
Hi,

Tested on 5.10.102 and 5.17-rc6.

On 5.10.102 this avoids the following kernel warning on reboot:

[   23.077179] ci_hdrc ci_hdrc.1: remove, state 1
[   23.081674] usb usb2: USB disconnect, device number 1
[   23.086740] usb 2-1: USB disconnect, device number 2
[   23.088393] smsc95xx 2-1.1:1.0 eth1: Failed to read reg index 0x00000114: -19
[   23.091718] usb 2-1.1: USB disconnect, device number 3
[   23.094090] smsc95xx 2-1.1:1.0 eth1: unregister 'smsc95xx'
usb-ci_hdrc.1-1.1, smsc95xx USB 2.0 Ethernet
[   23.098869] smsc95xx 2-1.1:1.0 eth1: Error reading MII_ACCESS
[   23.098877] smsc95xx 2-1.1:1.0 eth1: __smsc95xx_mdio_read: MII is busy
[   23.125763] ------------[ cut here ]------------
[   23.125860] smsc95xx 2-1.1:1.0 eth1: Failed to read reg index 0x00000114: -19
[   23.130393] WARNING: CPU: 3 PID: 119 at drivers/net/phy/phy.c:958
phy_error+0x14/0x60
[   23.130397] Modules linked in:
[   23.137550] smsc95xx 2-1.1:1.0 eth1: Error reading MII_ACCESS
[   23.145367]  iwlmvm mac80211 libarc4
[   23.148439] smsc95xx 2-1.1:1.0 eth1: __smsc95xx_mdio_read: MII is busy
[   23.154175]  af_alg caam_jr caamhash_desc caamalg_desc
crypto_engine rng_core authenc libdes btusb hci_uart btrtl btintel
btqca crct10dif_ce btbcm fsl_imx8_ddr_perf bluetooth ecdh_generic ecc
spi_imx spi_bitbang clk_bd718x7 at24 caam error rtc_snvs snvs_pwrkey
imx8mm_thermal imx_cpufreq_dt pwm_bl overlay iwlwifi cfg80211 rfkill
ipv6
[   23.193841] CPU: 3 PID: 119 Comm: kworker/u8:2 Not tainted 5.10.102-stable-standard #1
[   23.201764] Hardware name: CompuLab i.MX8MM IoT Gateway (DT)
[   23.207433] Workqueue: events_power_efficient phy_state_machine
[   23.213362] pstate: 60000005 (nZCv daif -PAN -UAO -TCO BTYPE=--)
[   23.219375] pc : phy_error+0x14/0x60
[   23.222954] lr : phy_state_machine+0x88/0x218
[   23.227315] sp : ffff800011743d20
[   23.230631] x29: ffff800011743d20 x28: ffff800011207000
[   23.235952] x27: ffff000000008070 x26: ffff000000008020
[   23.241273] x25: 0000000000000000 x24: 00000000ffffffed
[   23.246595] x23: ffff0000002e44e8 x22: ffff000000161c80
[   23.251915] x21: ffff0000002e4000 x20: 0000000000000005
[   23.257238] x19: ffff0000002e4000 x18: 0000000000000010
[   23.262558] x17: 0000000000000000 x16: 0000000000000000
[   23.267878] x15: 0000000e9816052e x14: 0000000000000000
[   23.273198] x13: 000000000000002f x12: 0000000000000198
[   23.278518] x11: 000000000000c6f5 x10: 000000000000c6f5
[   23.283838] x9 : 0000000000000000 x8 : ffff00007fbbc0c0
[   23.289161] x7 : ffff00007fbbb600 x6 : 0000000000000000
[   23.294481] x5 : 0000000000000008 x4 : 0000000000000000
[   23.299801] x3 : ffff0000002e44e8 x2 : 0000000000000000
[   23.305121] x1 : ffff000000161c80 x0 : ffff0000002e4000
[   23.310442] Call trace:
[   23.312894]  phy_error+0x14/0x60
[   23.316125]  phy_state_machine+0x88/0x218
[   23.320144]  process_one_work+0x1bc/0x338
[   23.324158]  worker_thread+0x50/0x420
[   23.327826]  kthread+0x140/0x160
[   23.331061]  ret_from_fork+0x10/0x34
[   23.334639] ---[ end trace 2cf86ece81b89776 ]---
[   23.339391] smsc95xx 2-1.1:1.0 eth1: Failed to read reg index 0x00000114: -19
[   23.346550] smsc95xx 2-1.1:1.0 eth1: Error reading MII_ACCESS
[   23.352312] smsc95xx 2-1.1:1.0 eth1: __smsc95xx_mdio_read: MII is busy
[   23.358863] smsc95xx 2-1.1:1.0 eth1: hardware isn't capable of remote wakeup
[   23.384599] usb 2-1.4: USB disconnect, device number 4
[   23.394062] ci_hdrc ci_hdrc.1: USB bus 2 deregistered
[   23.401921] ci_hdrc ci_hdrc.0: remove, state 4
[   23.406444] usb usb1: USB disconnect, device number 1
[   23.412082] ci_hdrc ci_hdrc.0: USB bus 1 deregistered
[   23.438063] imx2-wdt 30280000.watchdog: Device shutdown: Expect reboot!
[   23.444895] reboot: Restarting system

 drivers/net/usb/smsc95xx.c | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
index b17bff6a1015..61a5748da133 100644
--- a/drivers/net/usb/smsc95xx.c
+++ b/drivers/net/usb/smsc95xx.c
@@ -84,7 +84,7 @@ static int __must_check __smsc95xx_read_reg(struct usbnet *dev, u32 index,
 	ret = fn(dev, USB_VENDOR_REQUEST_READ_REGISTER, USB_DIR_IN
 		 | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
 		 0, index, &buf, 4);
-	if (unlikely(ret < 0)) {
+	if (ret < 0 && ret != -ENODEV) {
 		netdev_warn(dev->net, "Failed to read reg index 0x%08x: %d\n",
 			    index, ret);
 		return ret;
@@ -116,7 +116,7 @@ static int __must_check __smsc95xx_write_reg(struct usbnet *dev, u32 index,
 	ret = fn(dev, USB_VENDOR_REQUEST_WRITE_REGISTER, USB_DIR_OUT
 		 | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
 		 0, index, &buf, 4);
-	if (unlikely(ret < 0))
+	if (ret < 0 && ret != -ENODEV)
 		netdev_warn(dev->net, "Failed to write reg index 0x%08x: %d\n",
 			    index, ret);
 
@@ -159,6 +159,9 @@ static int __must_check __smsc95xx_phy_wait_not_busy(struct usbnet *dev,
 	do {
 		ret = __smsc95xx_read_reg(dev, MII_ADDR, &val, in_pm);
 		if (ret < 0) {
+			/* Ignore -ENODEV error during disconnect() */
+			if (ret == -ENODEV)
+				return 0;
 			netdev_warn(dev->net, "Error reading MII_ACCESS\n");
 			return ret;
 		}
@@ -194,7 +197,8 @@ static int __smsc95xx_mdio_read(struct usbnet *dev, int phy_id, int idx,
 	addr = mii_address_cmd(phy_id, idx, MII_READ_ | MII_BUSY_);
 	ret = __smsc95xx_write_reg(dev, MII_ADDR, addr, in_pm);
 	if (ret < 0) {
-		netdev_warn(dev->net, "Error writing MII_ADDR\n");
+		if (ret != -ENODEV)
+			netdev_warn(dev->net, "Error writing MII_ADDR\n");
 		goto done;
 	}
 
@@ -206,7 +210,8 @@ static int __smsc95xx_mdio_read(struct usbnet *dev, int phy_id, int idx,
 
 	ret = __smsc95xx_read_reg(dev, MII_DATA, &val, in_pm);
 	if (ret < 0) {
-		netdev_warn(dev->net, "Error reading MII_DATA\n");
+		if (ret != -ENODEV)
+			netdev_warn(dev->net, "Error reading MII_DATA\n");
 		goto done;
 	}
 
@@ -214,6 +219,10 @@ static int __smsc95xx_mdio_read(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;
 }
 
@@ -235,7 +244,8 @@ static void __smsc95xx_mdio_write(struct usbnet *dev, int phy_id,
 	val = regval;
 	ret = __smsc95xx_write_reg(dev, MII_DATA, val, in_pm);
 	if (ret < 0) {
-		netdev_warn(dev->net, "Error writing MII_DATA\n");
+		if (ret != -ENODEV)
+			netdev_warn(dev->net, "Error writing MII_DATA\n");
 		goto done;
 	}
 
@@ -243,7 +253,8 @@ static void __smsc95xx_mdio_write(struct usbnet *dev, int phy_id,
 	addr = mii_address_cmd(phy_id, idx, MII_WRITE_ | MII_BUSY_);
 	ret = __smsc95xx_write_reg(dev, MII_ADDR, addr, in_pm);
 	if (ret < 0) {
-		netdev_warn(dev->net, "Error writing MII_ADDR\n");
+		if (ret != -ENODEV)
+			netdev_warn(dev->net, "Error writing MII_ADDR\n");
 		goto done;
 	}
 
-- 
2.25.1

Re: [PATCH] smsc95xx: Ignore -ENODEV errors when device is unplugged

[Andrew Lunn]

On Sat, Mar 05, 2022 at 03:45:03PM -0300, Fabio Estevam wrote:
> From: Fabio Estevam <festevam@denx.de>
> 
> According to Documentation/driver-api/usb/URB.rst when a device
> is unplugged usb_submit_urb() returns -ENODEV.
> 
> This error code propagates all the way up to usbnet_read_cmd() and
> usbnet_write_cmd() calls inside the smsc95xx.c driver during
> Ethernet cable unplug, unbind or reboot.
> 
> This causes the following errors to be shown on reboot, for example:
> 
> ci_hdrc ci_hdrc.1: remove, state 1
> usb usb2: USB disconnect, device number 1
> usb 2-1: USB disconnect, device number 2
> usb 2-1.1: USB disconnect, device number 3
> smsc95xx 2-1.1:1.0 eth1: unregister 'smsc95xx' usb-ci_hdrc.1-1.1, smsc95xx USB 2.0 Ethernet
> smsc95xx 2-1.1:1.0 eth1: Failed to read reg index 0x00000114: -19
> smsc95xx 2-1.1:1.0 eth1: Error reading MII_ACCESS
> smsc95xx 2-1.1:1.0 eth1: __smsc95xx_mdio_read: MII is busy
> smsc95xx 2-1.1:1.0 eth1: Failed to read reg index 0x00000114: -19
> smsc95xx 2-1.1:1.0 eth1: Error reading MII_ACCESS
> smsc95xx 2-1.1:1.0 eth1: __smsc95xx_mdio_read: MII is busy
> smsc95xx 2-1.1:1.0 eth1: hardware isn't capable of remote wakeup
> usb 2-1.4: USB disconnect, device number 4
> ci_hdrc ci_hdrc.1: USB bus 2 deregistered
> ci_hdrc ci_hdrc.0: remove, state 4
> usb usb1: USB disconnect, device number 1
> ci_hdrc ci_hdrc.0: USB bus 1 deregistered
> imx2-wdt 30280000.watchdog: Device shutdown: Expect reboot!
> reboot: Restarting system
> 
> Ignore the -ENODEV errors inside __smsc95xx_mdio_read() and
> __smsc95xx_phy_wait_not_busy() and do not print error messages
> when -ENODEV is returned.
> 
> Signed-off-by: Fabio Estevam <festevam@denx.de>
> ---
> Hi,
> 
> Tested on 5.10.102 and 5.17-rc6.

Please indicate what tree this patch is actually for. It should be
against net, since you want it backporting. Please see the netdev FAQ.
Please also include a Fixes: tag.

> @@ -84,7 +84,7 @@ static int __must_check __smsc95xx_read_reg(struct usbnet *dev, u32 index,
>  	ret = fn(dev, USB_VENDOR_REQUEST_READ_REGISTER, USB_DIR_IN
>  		 | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
>  		 0, index, &buf, 4);
> -	if (unlikely(ret < 0)) {
> +	if (ret < 0 && ret != -ENODEV) {
>  		netdev_warn(dev->net, "Failed to read reg index 0x%08x: %d\n",
>  			    index, ret);
>  		return ret;

I suspect this will result in kasan warnings. The contents of buf is
probably undefined because of the error, yet you continue to set *data
to it. You probably need to explicitly handle the ENODEV case setting
*data to some value.

      Andrew

Re: [PATCH] smsc95xx: Ignore -ENODEV errors when device is unplugged

[Fabio Estevam]

Hi Andrew,

On Sat, Mar 5, 2022 at 4:24 PM Andrew Lunn <andrew@lunn.ch> wrote:

> Please indicate what tree this patch is actually for. It should be
> against net, since you want it backporting. Please see the netdev FAQ.
> Please also include a Fixes: tag.

Thanks for your review. Fixed in v2.

> I suspect this will result in kasan warnings. The contents of buf is
> probably undefined because of the error, yet you continue to set *data
> to it. You probably need to explicitly handle the ENODEV case setting
> *data to some value.

Good catch. Fixed in v2.

Thanks