| Message ID | 20231201132647.178979-1-jtornosm@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v3] net: usb: ax88179_178a: avoid failed operations when device is disconnected | expand |
On Fri, Dec 01, 2023 at 02:26:47PM +0100, Jose Ignacio Tornos Martinez wrote: > When the device is disconnected we get the following messages showing > failed operations: > Nov 28 20:22:11 localhost kernel: usb 2-3: USB disconnect, device number 2 > Nov 28 20:22:11 localhost kernel: ax88179_178a 2-3:1.0 enp2s0u3: unregister 'ax88179_178a' usb-0000:02:00.0-3, ASIX AX88179 USB 3.0 Gigabit Ethernet > Nov 28 20:22:11 localhost kernel: ax88179_178a 2-3:1.0 enp2s0u3: Failed to read reg index 0x0002: -19 > Nov 28 20:22:11 localhost kernel: ax88179_178a 2-3:1.0 enp2s0u3: Failed to write reg index 0x0002: -19 > Nov 28 20:22:11 localhost kernel: ax88179_178a 2-3:1.0 enp2s0u3 (unregistered): Failed to write reg index 0x0002: -19 > Nov 28 20:22:11 localhost kernel: ax88179_178a 2-3:1.0 enp2s0u3 (unregistered): Failed to write reg index 0x0001: -19 > Nov 28 20:22:11 localhost kernel: ax88179_178a 2-3:1.0 enp2s0u3 (unregistered): Failed to write reg index 0x0002: -19 > > The reason is that although the device is detached, normal stop and > unbind operations are commanded from the driver. These operations are > not necessary in this situation, so avoid these logs when the device is > detached if the result of the operation is -ENODEV and if the new flag > informing about the stopping or unbind operation is enabled. > > cc: stable@vger.kernel.org > Fixes: e2ca90c276e1f ("ax88179_178a: ASIX AX88179_178A USB 3.0/2.0 to gigabit ethernet adapter driver") > Signed-off-by: Jose Ignacio Tornos Martinez <jtornosm@redhat.com> > @@ -242,7 +245,7 @@ static int __ax88179_write_cmd(struct usbnet *dev, u8 cmd, u16 value, u16 index, > ret = fn(dev, cmd, USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_DEVICE, > value, index, data, size); > > - if (unlikely(ret < 0)) > + if (unlikely(ret < 0 && !(ret == -ENODEV && ax179_data->stopping_unbinding))) Would it be good enough just to check for ret != -ENODEV and not do the stopping_unbinding check at all? Alan Stern
Hi Alan, > Would it be good enough just to check for ret != -ENODEV and not do the > stopping_unbinding check at all? I thought about that but if possible, I would like to ignore the failed operation messages only under a controlled and expected situation. I think that if there is a problem with the device it will be easier to analyze it later with all the possible information. But this is my opinion ... Thank you Best regards José Ignacio
On 01.12.23 14:26, Jose Ignacio Tornos Martinez wrote: Hi, this is much better. > @@ -1661,14 +1668,19 @@ static int ax88179_reset(struct usbnet *dev) > > static int ax88179_stop(struct usbnet *dev) > { > + struct ax88179_data *ax179_data = dev->driver_priv; > u16 tmp16; > > + ax179_data->stopping_unbinding = 1; This is problematic. ndo_stop() is not limited to disconnection. It is also used whenever an interface transitions from up to down. > + > ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_MEDIUM_STATUS_MODE, > 2, 2, &tmp16); > tmp16 &= ~AX_MEDIUM_RECEIVE_EN; > ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_MEDIUM_STATUS_MODE, > 2, 2, &tmp16); > > + ax179_data->stopping_unbinding = 0; > + > return 0; > } > On a general note, you are going for a belt and suspenders approach. It seems to me that you have two options. 1. Do as Alan suggested and ignore ENODEV. You'd be acknowledging that these devices are hotpluggable and therefore -ENODEV is not an error 2. Use only a flag. But if you do that, you are setting it in the wrong place. It should be set in usbnet_disconnect() O and, well, this is a very mior issue, but you've introduced a memory ordering issue. You ought to use smp_wmb() after setting the flag and smp_rmb() before reading it. Regards Oliver
Hi Oliver, > this is much better. >> ... > This is problematic. ndo_stop() is not limited to disconnection. > It is also used whenever an interface transitions from up to down. >> ... > > On a general note, you are going for a belt and suspenders approach. > It seems to me that you have two options. > 1. Do as Alan suggested and ignore ENODEV. You'd be acknowledging that > these devices are hotpluggable and therefore -ENODEV is not an error > 2. Use only a flag. But if you do that, you are setting it in the wrong > place. It should be set in usbnet_disconnect() Thank you for you help, I will do what you say. I would like to try 2 first so that it only affects the unbind operation. If I find any problem, I will try 1. > O and, well, this is a very mior issue, but you've introduced a memory > ordering issue. You ought to use smp_wmb() after setting the flag and > smp_rmb() before reading it. Thank you again, I'll keep that in mind Best regards José Ignacio
diff --git a/drivers/net/usb/ax88179_178a.c b/drivers/net/usb/ax88179_178a.c index 4ea0e155bb0d..105bae360128 100644 --- a/drivers/net/usb/ax88179_178a.c +++ b/drivers/net/usb/ax88179_178a.c @@ -173,6 +173,7 @@ struct ax88179_data { u8 in_pm; u32 wol_supported; u32 wolopts; + u8 stopping_unbinding; }; struct ax88179_int_data { @@ -208,6 +209,7 @@ static int __ax88179_read_cmd(struct usbnet *dev, u8 cmd, u16 value, u16 index, { int ret; int (*fn)(struct usbnet *, u8, u8, u16, u16, void *, u16); + struct ax88179_data *ax179_data = dev->driver_priv; BUG_ON(!dev); @@ -219,7 +221,7 @@ static int __ax88179_read_cmd(struct usbnet *dev, u8 cmd, u16 value, u16 index, ret = fn(dev, cmd, USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_DEVICE, value, index, data, size); - if (unlikely(ret < 0)) + if (unlikely(ret < 0 && !(ret == -ENODEV && ax179_data->stopping_unbinding))) netdev_warn(dev->net, "Failed to read reg index 0x%04x: %d\n", index, ret); @@ -231,6 +233,7 @@ static int __ax88179_write_cmd(struct usbnet *dev, u8 cmd, u16 value, u16 index, { int ret; int (*fn)(struct usbnet *, u8, u8, u16, u16, const void *, u16); + struct ax88179_data *ax179_data = dev->driver_priv; BUG_ON(!dev); @@ -242,7 +245,7 @@ static int __ax88179_write_cmd(struct usbnet *dev, u8 cmd, u16 value, u16 index, ret = fn(dev, cmd, USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_DEVICE, value, index, data, size); - if (unlikely(ret < 0)) + if (unlikely(ret < 0 && !(ret == -ENODEV && ax179_data->stopping_unbinding))) netdev_warn(dev->net, "Failed to write reg index 0x%04x: %d\n", index, ret); @@ -1308,6 +1311,8 @@ static void ax88179_unbind(struct usbnet *dev, struct usb_interface *intf) struct ax88179_data *ax179_data = dev->driver_priv; u16 tmp16; + ax179_data->stopping_unbinding = 1; + /* Configure RX control register => stop operation */ tmp16 = AX_RX_CTL_STOP; ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_RX_CTL, 2, 2, &tmp16); @@ -1319,6 +1324,8 @@ static void ax88179_unbind(struct usbnet *dev, struct usb_interface *intf) tmp16 = 0; ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_PHYPWR_RSTCTL, 2, 2, &tmp16); + ax179_data->stopping_unbinding = 0; + kfree(ax179_data); } @@ -1661,14 +1668,19 @@ static int ax88179_reset(struct usbnet *dev) static int ax88179_stop(struct usbnet *dev) { + struct ax88179_data *ax179_data = dev->driver_priv; u16 tmp16; + ax179_data->stopping_unbinding = 1; + ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_MEDIUM_STATUS_MODE, 2, 2, &tmp16); tmp16 &= ~AX_MEDIUM_RECEIVE_EN; ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_MEDIUM_STATUS_MODE, 2, 2, &tmp16); + ax179_data->stopping_unbinding = 0; + return 0; }
When the device is disconnected we get the following messages showing failed operations: Nov 28 20:22:11 localhost kernel: usb 2-3: USB disconnect, device number 2 Nov 28 20:22:11 localhost kernel: ax88179_178a 2-3:1.0 enp2s0u3: unregister 'ax88179_178a' usb-0000:02:00.0-3, ASIX AX88179 USB 3.0 Gigabit Ethernet Nov 28 20:22:11 localhost kernel: ax88179_178a 2-3:1.0 enp2s0u3: Failed to read reg index 0x0002: -19 Nov 28 20:22:11 localhost kernel: ax88179_178a 2-3:1.0 enp2s0u3: Failed to write reg index 0x0002: -19 Nov 28 20:22:11 localhost kernel: ax88179_178a 2-3:1.0 enp2s0u3 (unregistered): Failed to write reg index 0x0002: -19 Nov 28 20:22:11 localhost kernel: ax88179_178a 2-3:1.0 enp2s0u3 (unregistered): Failed to write reg index 0x0001: -19 Nov 28 20:22:11 localhost kernel: ax88179_178a 2-3:1.0 enp2s0u3 (unregistered): Failed to write reg index 0x0002: -19 The reason is that although the device is detached, normal stop and unbind operations are commanded from the driver. These operations are not necessary in this situation, so avoid these logs when the device is detached if the result of the operation is -ENODEV and if the new flag informing about the stopping or unbind operation is enabled. cc: stable@vger.kernel.org Fixes: e2ca90c276e1f ("ax88179_178a: ASIX AX88179_178A USB 3.0/2.0 to gigabit ethernet adapter driver") Signed-off-by: Jose Ignacio Tornos Martinez <jtornosm@redhat.com> --- V1 -> V2: - Follow the suggestions from Alan Stern and Oliver Neukum to check the result of the operations (-ENODEV) and not the internal state of the USB layer (USB_STATE_NOTATTACHED). V2 -> V3 - Add cc: stable line in the signed-off-by area. drivers/net/usb/ax88179_178a.c | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-)