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 <[email protected]>
---
drivers/net/usb/ax88179_178a.c | 26 +++++++++++++++-----------
1 file changed, 15 insertions(+), 11 deletions(-)
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;
}
--
2.43.0
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
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 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
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
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.
Fixes: e2ca90c276e1f ("ax88179_178a: ASIX AX88179_178A USB 3.0/2.0 to gigabit ethernet adapter driver")
Signed-off-by: Jose Ignacio Tornos Martinez <[email protected]>
---
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).
drivers/net/usb/ax88179_178a.c | 16 ++++++++++++++--
1 file changed, 14 insertions(+), 2 deletions(-)
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;
}
--
2.43.0
On Fri, Dec 01, 2023 at 12:51:43PM +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.
>
> Fixes: e2ca90c276e1f ("ax88179_178a: ASIX AX88179_178A USB 3.0/2.0 to gigabit ethernet adapter driver")
> Signed-off-by: Jose Ignacio Tornos Martinez <[email protected]>
> ---
> 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).
>
> drivers/net/usb/ax88179_178a.c | 16 ++++++++++++++--
> 1 file changed, 14 insertions(+), 2 deletions(-)
Hi,
This is the friendly patch-bot of Greg Kroah-Hartman. You have sent him
a patch that has triggered this response. He used to manually respond
to these common problems, but in order to save his sanity (he kept
writing the same thing over and over, yet to different people), I was
created. Hopefully you will not take offence and will fix the problem
in your patch and resubmit it so that it can be accepted into the Linux
kernel tree.
You are receiving this message because of the following common error(s)
as indicated below:
- You have marked a patch with a "Fixes:" tag for a commit that is in an
older released kernel, yet you do not have a cc: stable line in the
signed-off-by area at all, which means that the patch will not be
applied to any older kernel releases. To properly fix this, please
follow the documented rules in the
Documentation/process/stable-kernel-rules.rst file for how to resolve
this.
If you wish to discuss this problem further, or you have questions about
how to resolve this issue, please feel free to respond to this email and
Greg will reply once he has dug out from the pending patches received
from other developers.
thanks,
greg k-h's patch email bot
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: [email protected]
Fixes: e2ca90c276e1f ("ax88179_178a: ASIX AX88179_178A USB 3.0/2.0 to gigabit ethernet adapter driver")
Signed-off-by: Jose Ignacio Tornos Martinez <[email protected]>
---
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(-)
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;
}
--
2.43.0
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: [email protected]
> Fixes: e2ca90c276e1f ("ax88179_178a: ASIX AX88179_178A USB 3.0/2.0 to gigabit ethernet adapter driver")
> Signed-off-by: Jose Ignacio Tornos Martinez <[email protected]>
> @@ -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
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 disconnecting status is enabled.
cc: [email protected]
Fixes: e2ca90c276e1f ("ax88179_178a: ASIX AX88179_178A USB 3.0/2.0 to gigabit ethernet adapter driver")
Signed-off-by: Jose Ignacio Tornos Martinez <[email protected]>
---
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.
V3 -> V4
- Follow the suggestions from Oliver Neukum to use only one flag when
disconnecting and include barriers to avoid memory ordering issues.
drivers/net/usb/ax88179_178a.c | 38 +++++++++++++++++++++++++++-------
1 file changed, 31 insertions(+), 7 deletions(-)
diff --git a/drivers/net/usb/ax88179_178a.c b/drivers/net/usb/ax88179_178a.c
index 4ea0e155bb0d..1c671f2a43ee 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 disconnecting;
};
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,9 +221,12 @@ 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))
- netdev_warn(dev->net, "Failed to read reg index 0x%04x: %d\n",
- index, ret);
+ if (unlikely(ret < 0)) {
+ smp_rmb();
+ if (!(ret == -ENODEV && ax179_data->disconnecting))
+ netdev_warn(dev->net, "Failed to read reg index 0x%04x: %d\n",
+ index, ret);
+ }
return ret;
}
@@ -231,6 +236,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,9 +248,12 @@ 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))
- netdev_warn(dev->net, "Failed to write reg index 0x%04x: %d\n",
- index, ret);
+ if (unlikely(ret < 0)) {
+ smp_rmb();
+ if (!(ret == -ENODEV && ax179_data->disconnecting))
+ netdev_warn(dev->net, "Failed to write reg index 0x%04x: %d\n",
+ index, ret);
+ }
return ret;
}
@@ -492,6 +501,21 @@ static int ax88179_resume(struct usb_interface *intf)
return usbnet_resume(intf);
}
+static void ax88179_disconnect(struct usb_interface *intf)
+{
+ struct usbnet *dev = usb_get_intfdata(intf);
+ struct ax88179_data *ax179_data;
+
+ if (!dev)
+ return;
+
+ ax179_data = dev->driver_priv;
+ ax179_data->disconnecting = 1;
+ smp_wmb();
+
+ usbnet_disconnect(intf);
+}
+
static void
ax88179_get_wol(struct net_device *net, struct ethtool_wolinfo *wolinfo)
{
@@ -1906,7 +1930,7 @@ static struct usb_driver ax88179_178a_driver = {
.suspend = ax88179_suspend,
.resume = ax88179_resume,
.reset_resume = ax88179_resume,
- .disconnect = usbnet_disconnect,
+ .disconnect = ax88179_disconnect,
.supports_autosuspend = 1,
.disable_hub_initiated_lpm = 1,
};
--
2.43.0
On Tue, Dec 05, 2023 at 02:51:54PM +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 disconnecting status is enabled.
>
> cc: [email protected]
> Fixes: e2ca90c276e1f ("ax88179_178a: ASIX AX88179_178A USB 3.0/2.0 to gigabit ethernet adapter driver")
> Signed-off-by: Jose Ignacio Tornos Martinez <[email protected]>
> ---
> 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.
> V3 -> V4
> - Follow the suggestions from Oliver Neukum to use only one flag when
> disconnecting and include barriers to avoid memory ordering issues.
The __ax88179_read_cmd() and __ax88179_write_cmd() routines are
asynchronous with respect to ax88179_disconnect(), right? Or at least,
they are if they run as a result of the user closing the network
interface. Otherwise there wouldn't be any memory ordering issues.
But the memory barriers you added are not the proper solution. What you
need here is _synchronization_, not _ordering_. As it is, the memory
barriers you have added don't do anything; they shouldn't be in the
patch.
If you would like a more in-depth explanation, let me know.
Alan Stern
> drivers/net/usb/ax88179_178a.c | 38 +++++++++++++++++++++++++++-------
> 1 file changed, 31 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/net/usb/ax88179_178a.c b/drivers/net/usb/ax88179_178a.c
> index 4ea0e155bb0d..1c671f2a43ee 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 disconnecting;
> };
>
> 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,9 +221,12 @@ 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))
> - netdev_warn(dev->net, "Failed to read reg index 0x%04x: %d\n",
> - index, ret);
> + if (unlikely(ret < 0)) {
> + smp_rmb();
> + if (!(ret == -ENODEV && ax179_data->disconnecting))
> + netdev_warn(dev->net, "Failed to read reg index 0x%04x: %d\n",
> + index, ret);
> + }
>
> return ret;
> }
> @@ -231,6 +236,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,9 +248,12 @@ 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))
> - netdev_warn(dev->net, "Failed to write reg index 0x%04x: %d\n",
> - index, ret);
> + if (unlikely(ret < 0)) {
> + smp_rmb();
> + if (!(ret == -ENODEV && ax179_data->disconnecting))
> + netdev_warn(dev->net, "Failed to write reg index 0x%04x: %d\n",
> + index, ret);
> + }
>
> return ret;
> }
> @@ -492,6 +501,21 @@ static int ax88179_resume(struct usb_interface *intf)
> return usbnet_resume(intf);
> }
>
> +static void ax88179_disconnect(struct usb_interface *intf)
> +{
> + struct usbnet *dev = usb_get_intfdata(intf);
> + struct ax88179_data *ax179_data;
> +
> + if (!dev)
> + return;
> +
> + ax179_data = dev->driver_priv;
> + ax179_data->disconnecting = 1;
> + smp_wmb();
> +
> + usbnet_disconnect(intf);
> +}
> +
> static void
> ax88179_get_wol(struct net_device *net, struct ethtool_wolinfo *wolinfo)
> {
> @@ -1906,7 +1930,7 @@ static struct usb_driver ax88179_178a_driver = {
> .suspend = ax88179_suspend,
> .resume = ax88179_resume,
> .reset_resume = ax88179_resume,
> - .disconnect = usbnet_disconnect,
> + .disconnect = ax88179_disconnect,
> .supports_autosuspend = 1,
> .disable_hub_initiated_lpm = 1,
> };
> --
> 2.43.0
>
Hello Alan,
> The __ax88179_read_cmd() and __ax88179_write_cmd() routines are
> asynchronous with respect to ax88179_disconnect(), right? Or at least,
> they are if they run as a result of the user closing the network
> interface. Otherwise there wouldn't be any memory ordering issues.
Yes, I think so, they could be asynchronous regarding ax88179_disconnect.
> But the memory barriers you added are not the proper solution. What you
> need here is _synchronization_, not _ordering_. As it is, the memory
> barriers you have added don't do anything; they shouldn't be in the
> patch.
Ok, thank you for the helpful clarification, let me check it better,
I understood it in a wrong way.
> If you would like a more in-depth explanation, let me know.
Thank you for your help, I will try first, I really appreciate this.
Best regards
José Ignacio
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 disconnecting status is enabled.
cc: [email protected]
Fixes: e2ca90c276e1f ("ax88179_178a: ASIX AX88179_178A USB 3.0/2.0 to gigabit ethernet adapter driver")
Signed-off-by: Jose Ignacio Tornos Martinez <[email protected]>
---
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.
V3 -> V4
- Follow the suggestions from Oliver Neukum to use only one flag when
disconnecting and include barriers to avoid memory ordering issues.
V4 -> V5
- Fix my misundestanding and follow the suggestion from Alan Stern to
syncronize and not order the flag.
drivers/net/usb/ax88179_178a.c | 47 +++++++++++++++++++++++++++++-----
1 file changed, 40 insertions(+), 7 deletions(-)
diff --git a/drivers/net/usb/ax88179_178a.c b/drivers/net/usb/ax88179_178a.c
index 4ea0e155bb0d..e07614799f75 100644
--- a/drivers/net/usb/ax88179_178a.c
+++ b/drivers/net/usb/ax88179_178a.c
@@ -173,6 +173,8 @@ struct ax88179_data {
u8 in_pm;
u32 wol_supported;
u32 wolopts;
+ u8 disconnecting;
+ struct mutex lock;
};
struct ax88179_int_data {
@@ -208,6 +210,8 @@ 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;
+ u8 disconnecting;
BUG_ON(!dev);
@@ -219,9 +223,14 @@ 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))
- netdev_warn(dev->net, "Failed to read reg index 0x%04x: %d\n",
- index, ret);
+ if (unlikely(ret < 0)) {
+ mutex_lock(&ax179_data->lock);
+ disconnecting = ax179_data->disconnecting;
+ mutex_unlock(&ax179_data->lock);
+ if (!(ret == -ENODEV && disconnecting))
+ netdev_warn(dev->net, "Failed to read reg index 0x%04x: %d\n",
+ index, ret);
+ }
return ret;
}
@@ -231,6 +240,8 @@ 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;
+ u8 disconnecting;
BUG_ON(!dev);
@@ -242,9 +253,14 @@ 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))
- netdev_warn(dev->net, "Failed to write reg index 0x%04x: %d\n",
- index, ret);
+ if (unlikely(ret < 0)) {
+ mutex_lock(&ax179_data->lock);
+ disconnecting = ax179_data->disconnecting;
+ mutex_unlock(&ax179_data->lock);
+ if (!(ret == -ENODEV && disconnecting))
+ netdev_warn(dev->net, "Failed to write reg index 0x%04x: %d\n",
+ index, ret);
+ }
return ret;
}
@@ -492,6 +508,22 @@ static int ax88179_resume(struct usb_interface *intf)
return usbnet_resume(intf);
}
+static void ax88179_disconnect(struct usb_interface *intf)
+{
+ struct usbnet *dev = usb_get_intfdata(intf);
+ struct ax88179_data *ax179_data;
+
+ if (!dev)
+ return;
+
+ ax179_data = dev->driver_priv;
+ mutex_lock(&ax179_data->lock);
+ ax179_data->disconnecting = 1;
+ mutex_unlock(&ax179_data->lock);
+
+ usbnet_disconnect(intf);
+}
+
static void
ax88179_get_wol(struct net_device *net, struct ethtool_wolinfo *wolinfo)
{
@@ -1274,6 +1306,7 @@ static int ax88179_bind(struct usbnet *dev, struct usb_interface *intf)
ax179_data = kzalloc(sizeof(*ax179_data), GFP_KERNEL);
if (!ax179_data)
return -ENOMEM;
+ mutex_init(&ax179_data->lock);
dev->driver_priv = ax179_data;
@@ -1906,7 +1939,7 @@ static struct usb_driver ax88179_178a_driver = {
.suspend = ax88179_suspend,
.resume = ax88179_resume,
.reset_resume = ax88179_resume,
- .disconnect = usbnet_disconnect,
+ .disconnect = ax88179_disconnect,
.supports_autosuspend = 1,
.disable_hub_initiated_lpm = 1,
};
--
2.43.0
On Thu, Dec 07, 2023 at 12:42:09PM +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 disconnecting status is enabled.
>
> cc: [email protected]
> Fixes: e2ca90c276e1f ("ax88179_178a: ASIX AX88179_178A USB 3.0/2.0 to gigabit ethernet adapter driver")
> Signed-off-by: Jose Ignacio Tornos Martinez <[email protected]>
> ---
> 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.
> V3 -> V4
> - Follow the suggestions from Oliver Neukum to use only one flag when
> disconnecting and include barriers to avoid memory ordering issues.
> V4 -> V5
> - Fix my misundestanding and follow the suggestion from Alan Stern to
> syncronize and not order the flag.
I did not suggest that you should synchronize anything. What I said was
that the problem you faced was one of synchronization, not of ordering.
In fact, you should not try to synchronize -- ultimately it is
impossible to do. The only way to get true synchronization is to
prevent the user from disconnecting the device and bringing the
interface down at the same time, and obviously the kernel cannot do
this.
> drivers/net/usb/ax88179_178a.c | 47 +++++++++++++++++++++++++++++-----
> 1 file changed, 40 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/net/usb/ax88179_178a.c b/drivers/net/usb/ax88179_178a.c
> index 4ea0e155bb0d..e07614799f75 100644
> --- a/drivers/net/usb/ax88179_178a.c
> +++ b/drivers/net/usb/ax88179_178a.c
> @@ -173,6 +173,8 @@ struct ax88179_data {
> u8 in_pm;
> u32 wol_supported;
> u32 wolopts;
> + u8 disconnecting;
> + struct mutex lock;
> };
>
> struct ax88179_int_data {
> @@ -208,6 +210,8 @@ 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;
> + u8 disconnecting;
>
> BUG_ON(!dev);
>
> @@ -219,9 +223,14 @@ 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))
> - netdev_warn(dev->net, "Failed to read reg index 0x%04x: %d\n",
> - index, ret);
> + if (unlikely(ret < 0)) {
> + mutex_lock(&ax179_data->lock);
> + disconnecting = ax179_data->disconnecting;
> + mutex_unlock(&ax179_data->lock);
Clearly you don't understand how mutexes work. Using a mutex like this
accomplishes nothing.
> + if (!(ret == -ENODEV && disconnecting))
> + netdev_warn(dev->net, "Failed to read reg index 0x%04x: %d\n",
> + index, ret);
> + }
>
> return ret;
> }
> @@ -231,6 +240,8 @@ 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;
> + u8 disconnecting;
>
> BUG_ON(!dev);
>
> @@ -242,9 +253,14 @@ 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))
> - netdev_warn(dev->net, "Failed to write reg index 0x%04x: %d\n",
> - index, ret);
> + if (unlikely(ret < 0)) {
> + mutex_lock(&ax179_data->lock);
> + disconnecting = ax179_data->disconnecting;
> + mutex_unlock(&ax179_data->lock);
Same here.
> + if (!(ret == -ENODEV && disconnecting))
> + netdev_warn(dev->net, "Failed to write reg index 0x%04x: %d\n",
> + index, ret);
> + }
>
> return ret;
> }
> @@ -492,6 +508,22 @@ static int ax88179_resume(struct usb_interface *intf)
> return usbnet_resume(intf);
> }
>
> +static void ax88179_disconnect(struct usb_interface *intf)
> +{
> + struct usbnet *dev = usb_get_intfdata(intf);
> + struct ax88179_data *ax179_data;
> +
> + if (!dev)
> + return;
> +
> + ax179_data = dev->driver_priv;
> + mutex_lock(&ax179_data->lock);
> + ax179_data->disconnecting = 1;
> + mutex_unlock(&ax179_data->lock);
And again, this does nothing.
Imagine a situation where the user brings the interface down on CPU 1,
while at the same time CPU 2 handles a disconnection. Then the
following timeline of events can occur:
CPU 1 CPU 2
--------------------------- -----------------------------
The stop operation calls
__ax88179_read_cmd()
The device is unplugged
The routine tries to communicate
with the device and gets a -ENODEV
error because the device is gone
The USB core calls ax88179_disconnect()
The routine locks the mutex,
reads ax179_data->disconnecting = 0,
and unlocks the mutex
ax88179_disconnect() locks the mutex,
sets ax179_data->disconnecting = 1,
and unlocks the mutex
The routine prints an error message
As you can see, using the mutex does not prevent the problem from
occurring. You should not add the mutex at all; it serves no purpose.
Alan Stern
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 disconnecting status is enabled.
cc: [email protected]
Fixes: e2ca90c276e1f ("ax88179_178a: ASIX AX88179_178A USB 3.0/2.0 to gigabit ethernet adapter driver")
Signed-off-by: Jose Ignacio Tornos Martinez <[email protected]>
---
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.
V3 -> V4
- Follow the suggestions from Oliver Neukum to use only one flag when
disconnecting and include barriers to avoid memory ordering issues.
V4 -> V5
- Fix my misundestanding and follow the suggestion from Alan Stern to
syncronize and not order the flag.
V5 -> V6
- Remove the unnecessary mutex. Thank you Alan for your teaching and
patience!
drivers/net/usb/ax88179_178a.c | 23 ++++++++++++++++++++---
1 file changed, 20 insertions(+), 3 deletions(-)
diff --git a/drivers/net/usb/ax88179_178a.c b/drivers/net/usb/ax88179_178a.c
index 4ea0e155bb0d..5a1bf42ce156 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 disconnecting;
};
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->disconnecting)))
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->disconnecting)))
netdev_warn(dev->net, "Failed to write reg index 0x%04x: %d\n",
index, ret);
@@ -492,6 +495,20 @@ static int ax88179_resume(struct usb_interface *intf)
return usbnet_resume(intf);
}
+static void ax88179_disconnect(struct usb_interface *intf)
+{
+ struct usbnet *dev = usb_get_intfdata(intf);
+ struct ax88179_data *ax179_data;
+
+ if (!dev)
+ return;
+
+ ax179_data = dev->driver_priv;
+ ax179_data->disconnecting = 1;
+
+ usbnet_disconnect(intf);
+}
+
static void
ax88179_get_wol(struct net_device *net, struct ethtool_wolinfo *wolinfo)
{
@@ -1906,7 +1923,7 @@ static struct usb_driver ax88179_178a_driver = {
.suspend = ax88179_suspend,
.resume = ax88179_resume,
.reset_resume = ax88179_resume,
- .disconnect = usbnet_disconnect,
+ .disconnect = ax88179_disconnect,
.supports_autosuspend = 1,
.disable_hub_initiated_lpm = 1,
};
--
2.43.0
On Thu, Dec 07, 2023 at 06:50:07PM +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 disconnecting status is enabled.
>
> cc: [email protected]
> Fixes: e2ca90c276e1f ("ax88179_178a: ASIX AX88179_178A USB 3.0/2.0 to gigabit ethernet adapter driver")
> Signed-off-by: Jose Ignacio Tornos Martinez <[email protected]>
> ---
> 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.
> V3 -> V4
> - Follow the suggestions from Oliver Neukum to use only one flag when
> disconnecting and include barriers to avoid memory ordering issues.
> V4 -> V5
> - Fix my misundestanding and follow the suggestion from Alan Stern to
> syncronize and not order the flag.
> V5 -> V6
> - Remove the unnecessary mutex. Thank you Alan for your teaching and
> patience!
Acked-by: Alan Stern <[email protected]>
On Thu, 7 Dec 2023 15:23:23 -0500 Alan Stern wrote:
> Acked-by: Alan Stern <[email protected]>
FWIW I'm expecting Greg to pick this up for usb.
On Thu, Dec 07, 2023 at 12:32:56PM -0800, Jakub Kicinski wrote:
> On Thu, 7 Dec 2023 15:23:23 -0500 Alan Stern wrote:
> > Acked-by: Alan Stern <[email protected]>
>
> FWIW I'm expecting Greg to pick this up for usb.
Sure, will do!