| Message ID | 20231129151618.455618-1-jtornosm@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: usb: ax88179_178a: avoid failed operations when device is disconnected | expand |
On 29.11.23 16:16, Jose Ignacio Tornos Martinez wrote: Hi, > The reason is that although the device is detached, normal stop and > unbind operations are commanded. Avoid these unnecessary operations > when the device is detached (state is USB_STATE_NOTATTACHED) so as > not to get the error messages. I am sorry, but this is a layering violation. You are looking at an internal state of the USB layer to surpress logging -ENODEV. If you think these messages should go away, filter for ENODEV where they are generated. Regards Oliver
On Wed, Nov 29, 2023 at 04:33:58PM +0100, Oliver Neukum wrote: > On 29.11.23 16:16, Jose Ignacio Tornos Martinez wrote: > > Hi, > > > The reason is that although the device is detached, normal stop and > > unbind operations are commanded. Avoid these unnecessary operations > > when the device is detached (state is USB_STATE_NOTATTACHED) so as > > not to get the error messages. > > I am sorry, but this is a layering violation. You are looking > at an internal state of the USB layer to surpress logging > -ENODEV. If you think these messages should go away, filter > for ENODEV where they are generated. Indeed. In addition, you should be more careful about the distinction between "unbound" and "disconnected". It's possible for the driver to be unbound from the device even while the device is still plugged in. In this situation, submitting URBs will fail with an error even though the device state isn't USB_STATE_NOTATTACHED. Alan Stern
Hi Oliver, > I am sorry, but this is a layering violation. You are looking > at an internal state of the USB layer to surpress logging > -ENODEV. If you think these messages should go away, filter > for ENODEV where they are generated. Thank you for your comments and suggestion. My intention was also to avoid unnecessary and failed operations if disconnection was detected. Ok, let me research more and do better. Hi Alan, > In addition, you should be more careful about the distinction between > "unbound" and "disconnected". It's possible for the driver to be > unbound from the device even while the device is still plugged in. In > this situation, submitting URBs will fail with an error even though the > device state isn't USB_STATE_NOTATTACHED. Thank you for you comments. I also tested "unbound" and stop and unbind operations were correctly executed. I just wanted to avoid the issues during disconnection, if other operations are commanded later I think it is better to warn. Ok, I will try to do better. Best regards Jose Ignacio
On Thu, Nov 30, 2023 at 09:41:36AM +0100, Jose Ignacio Tornos Martinez wrote: > Hi Oliver, > > > I am sorry, but this is a layering violation. You are looking > > at an internal state of the USB layer to surpress logging > > -ENODEV. If you think these messages should go away, filter > > for ENODEV where they are generated. > Thank you for your comments and suggestion. > My intention was also to avoid unnecessary and failed operations > if disconnection was detected. What are these unnecessary operations? > Ok, let me research more and do better. > > Hi Alan, > > > In addition, you should be more careful about the distinction between > > "unbound" and "disconnected". It's possible for the driver to be > > unbound from the device even while the device is still plugged in. In > > this situation, submitting URBs will fail with an error even though the > > device state isn't USB_STATE_NOTATTACHED. > Thank you for you comments. > I also tested "unbound" and stop and unbind operations were correctly > executed. I just wanted to avoid the issues during disconnection, if other > operations are commanded later I think it is better to warn. In general, drivers should treat "unbind" the same as "disconnect" (in both cases, the ->disconnect() routine is called). If a driver tries to send commands to the device while the disconnect routine is running, it should expect that they might fail and not generate an error message if they do. (Also, note that the USB core will allow a driver to send commands to the device during unbind only if the .soft_unbind flag is set in the usb_driver structure.) And in any case, a driver should _never_ try to communicate with the device after the disconnect routine has returned. Alan Stern
Hi Alan, Thank you again for you comments. > What are these unnecessary operations? Sorry if I was not clear, I was referring to the reading and writing operations that are commanded within stop and unbind driver functions. This operations are necessary if we unbind to get the device stopped in a known state but if the device is detached, they are failing and imho they are not necessary. That is the reason why I was trying to detect when the device is really disconnected, to allow sending commands only if the device was still connected. > In general, drivers should treat "unbind" the same as "disconnect" (in > both cases, the ->disconnect() routine is called). If a driver tries to > send commands to the device while the disconnect routine is running, it > should expect that they might fail and not generate an error message if > they do. > > (Also, note that the USB core will allow a driver to send commands to > the device during unbind only if the .soft_unbind flag is set in the > usb_driver structure.) > > And in any case, a driver should _never_ try to communicate with the > device after the disconnect routine has returned. Ok, understood, very helpful clarification. In any case, I was referring to the internal operations during stop and unbind. And if any failed operations are commanded before and after disconnection (if any), try to detect with the warning to be sure if there is any problem. I have checked that other drivers are using USB_STATE_NOTATTACHED to check and confirm the device disconnection. And I am trying to analyze other drivers to check if this can be done in another way. If I use -ENODEV as Oliver suggested, I think I wouldn't know if the device is disconnected previous to any operation. But maybe this is the way. Best regards José Ignacio
On Thu, Nov 30, 2023 at 06:25:21PM +0100, Jose Ignacio Tornos Martinez wrote: > Hi Alan, > > Thank you again for you comments. > > > What are these unnecessary operations? > Sorry if I was not clear, I was referring to the reading and writing > operations that are commanded within stop and unbind driver functions. > This operations are necessary if we unbind to get the device stopped in > a known state but if the device is detached, they are failing and imho > they are not necessary. That is the reason why I was trying to detect > when the device is really disconnected, to allow sending commands only > if the device was still connected. > > > In general, drivers should treat "unbind" the same as "disconnect" (in > > both cases, the ->disconnect() routine is called). If a driver tries to > > send commands to the device while the disconnect routine is running, it > > should expect that they might fail and not generate an error message if > > they do. > > > > (Also, note that the USB core will allow a driver to send commands to > > the device during unbind only if the .soft_unbind flag is set in the > > usb_driver structure.) > > > > And in any case, a driver should _never_ try to communicate with the > > device after the disconnect routine has returned. > Ok, understood, very helpful clarification. > In any case, I was referring to the internal operations during stop and > unbind. And if any failed operations are commanded before and after > disconnection (if any), try to detect with the warning to be sure if there > is any problem. > > I have checked that other drivers are using USB_STATE_NOTATTACHED to check > and confirm the device disconnection. And I am trying to analyze other > drivers to check if this can be done in another way. > If I use -ENODEV as Oliver suggested, I think I wouldn't know if the device > is disconnected previous to any operation. But maybe this is the way. I see. Your approach isn't all that bad. Another possibility would be to have the unbind routine set a flag in the private data structure, and then make the ax88179_write_cmd() and ax88179_read_cmd() routines avoid printing error messages if the flag is set. Or don't bother with the flag, and simply make the routines skip printing an error message if a transfer fails with error code -ENODEV. Alan Stern
Hi Alan, Thank you for your help. > Another possibility would be to have the unbind routine set a flag in > the private data structure, and then make the ax88179_write_cmd() and > ax88179_read_cmd() routines avoid printing error messages if the flag is > set. Or don't bother with the flag, and simply make the routines skip > printing an error message if a transfer fails with error code -ENODEV. Yes, I had thought about those possibilities and I think they are the only ones from the driver. As you are commenting as well, I will try a second version with that. Best regards José Ignacio
diff --git a/drivers/net/usb/ax88179_178a.c b/drivers/net/usb/ax88179_178a.c index 4ea0e155bb0d..e78d555dd95e 100644 --- a/drivers/net/usb/ax88179_178a.c +++ b/drivers/net/usb/ax88179_178a.c @@ -1308,16 +1308,18 @@ static void ax88179_unbind(struct usbnet *dev, struct usb_interface *intf) struct ax88179_data *ax179_data = dev->driver_priv; u16 tmp16; - /* Configure RX control register => stop operation */ - tmp16 = AX_RX_CTL_STOP; - ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_RX_CTL, 2, 2, &tmp16); + if (dev->udev->state != USB_STATE_NOTATTACHED) { + /* Configure RX control register => stop operation */ + tmp16 = AX_RX_CTL_STOP; + ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_RX_CTL, 2, 2, &tmp16); - tmp16 = 0; - ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_CLK_SELECT, 1, 1, &tmp16); + tmp16 = 0; + ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_CLK_SELECT, 1, 1, &tmp16); - /* Power down ethernet PHY */ - tmp16 = 0; - ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_PHYPWR_RSTCTL, 2, 2, &tmp16); + /* Power down ethernet PHY */ + tmp16 = 0; + ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_PHYPWR_RSTCTL, 2, 2, &tmp16); + } kfree(ax179_data); } @@ -1663,11 +1665,13 @@ static int ax88179_stop(struct usbnet *dev) { u16 tmp16; - ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_MEDIUM_STATUS_MODE, + if (dev->udev->state != USB_STATE_NOTATTACHED) { + 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, + tmp16 &= ~AX_MEDIUM_RECEIVE_EN; + ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_MEDIUM_STATUS_MODE, 2, 2, &tmp16); + } 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. Avoid these unnecessary operations when the device is detached (state is USB_STATE_NOTATTACHED) so as not to get the error messages. 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> --- drivers/net/usb/ax88179_178a.c | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-)