[RESEND.] mt76: mt76u_vendor_request: Do not print error messages when -EPROTO

Commit Message

WangYuli Dec. 18, 2024, 9:08 a.m. UTC

When initializing the network card, unplugging the device will
trigger an -EPROTO error, resulting in a flood of error messages
being printed frantically.

Co-developed-by: Xu Rao <[e-mail]>
Signed-off-by: Xu Rao <[e-mail]>
Signed-off-by: WangYuli <[e-mail]>
---
 drivers/net/wireless/mediatek/mt76/usb.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Alexander Lobakin Dec. 18, 2024, 4:10 p.m. UTC | #1

From: Wangyuli <[e-mail]>
Date: Wed, 18 Dec 2024 17:08:33 +0800

> [RESEND. PATCH] mt76: mt76u_vendor_request: Do not print error messages when -EPROTO

Is it a fix or an improvement?
You need to specify the target tree, either 'PATCH net' (fixes) or
'PATCH net-next' (improvements).

The '.' after 'RESEND' is not needed.

> When initializing the network card, unplugging the device will
> trigger an -EPROTO error, resulting in a flood of error messages
> being printed frantically.
> 

If it's a fix, you need to have a 'Fixes:' tag here.

> Co-developed-by: Xu Rao <[e-mail]>
> Signed-off-by: Xu Rao <[e-mail]>
> Signed-off-by: WangYuli <[e-mail]>
> ---
>  drivers/net/wireless/mediatek/mt76/usb.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/wireless/mediatek/mt76/usb.c b/drivers/net/wireless/mediatek/mt76/usb.c
> index 58ff06823389..f9e67b8c3b3c 100644
> --- a/drivers/net/wireless/mediatek/mt76/usb.c
> +++ b/drivers/net/wireless/mediatek/mt76/usb.c
> @@ -33,9 +33,9 @@ int __mt76u_vendor_request(struct mt76_dev *dev, u8 req, u8 req_type,
>  
>  		ret = usb_control_msg(udev, pipe, req, req_type, val,
>  				      offset, buf, len, MT_VEND_REQ_TOUT_MS);
> -		if (ret == -ENODEV)
> +		if (ret == -ENODEV || ret == -EPROTO)
>  			set_bit(MT76_REMOVED, &dev->phy.state);
> -		if (ret >= 0 || ret == -ENODEV)
> +		if (ret >= 0 || ret == -ENODEV || ret == -EPROTO)
>  			return ret;
>  		usleep_range(5000, 10000);

How do other drivers handle this?
Can -EPROTO happen in other cases, not only unplugging, which this patch
would break?

Thanks,
Olek

Kalle Valo Dec. 18, 2024, 4:50 p.m. UTC | #2

Alexander Lobakin <[e-mail]> writes:

> From: Wangyuli <[e-mail]>
> Date: Wed, 18 Dec 2024 17:08:33 +0800
>
>> [RESEND. PATCH] mt76: mt76u_vendor_request: Do not print error messages when -EPROTO
>
> Is it a fix or an improvement?
> You need to specify the target tree, either 'PATCH net' (fixes) or
> 'PATCH net-next' (improvements).

Actually this is a wireless driver so the options are either wireless
tree or Felix' tree (for -next). And please add 'wifi:' to subject.

Alexander Lobakin Dec. 19, 2024, 3:23 p.m. UTC | #3

From: Wangyuli <[e-mail]>
Date: Thu, 19 Dec 2024 15:11:24 +0800

> On 2024/12/19 00:10, Alexander Lobakin wrote:
> 
>> Is it a fix or an improvement?
>> You need to specify the target tree, either 'PATCH net' (fixes) or
>> 'PATCH net-next' (improvements).
>  It is a fix not an improvement.

So you need to add the correct tree and/or subject prefix and specify
"Fixes:" tag with the commit this change fixes.

>> How do other drivers handle this?
>> Can -EPROTO happen in other cases, not only unplugging, which this patch
>> would break?
>>
> When initializing the network card, unplugging the device will trigger
> an -EPROTO error.
> 
> The exception is printed as follows：
> 
>         
>         mt76x2u 2-2.4:1.0: vendor request req:47 off:9018 failed:-71
>         mt76x2u 2-2.4:1.0: vendor request req:47 off:9018 failed:-71
>         ...
>         
> 
> It will continue to print more than 2000 times for about 5 minutes,
> causing the usb device to be unable to be disconnected.  During this
> period, the usb port cannot recognize the new device because the old
> device has not disconnected.
> 
> There may be other operating methods that cause -EPROTO, but -EPROTO is
> a low-level hardware error. It is unwise to repeat vendor requests
> expecting to read correct data. It is a better choice to treat -EPROTO
> and -ENODEV the same way.
> 
> Similar to commit （mt76: usb: process URBs with status EPROTO
> properly）do no schedule rx_worker for urb marked with status set -
> EPROTO. I also reproduced this situation when plugging and unplugging
> the device, and this patch is effective.

I'm not a wireless expert, from my PoV sounds good. Just describe
everything in details in the commit message, so that it will be clear
for everyone.

> 
> Just do not vendor request again for urb marked with status set -EPROTO .
> 
> 
> Thanks,
> 
> -- 
> WangYuli

Thanks,
Olek

Michał Pecio Dec. 19, 2024, 11:27 p.m. UTC | #4

> >  		ret = usb_control_msg(udev, pipe, req, req_type, val,
> >  				      offset, buf, len, MT_VEND_REQ_TOUT_MS);
> > -		if (ret == -ENODEV)
> > +		if (ret == -ENODEV || ret == -EPROTO)
> >  			set_bit(MT76_REMOVED, &dev->phy.state);
> > -		if (ret >= 0 || ret == -ENODEV)
> > +		if (ret >= 0 || ret == -ENODEV || ret == -EPROTO)
> >  			return ret;
> >  		usleep_range(5000, 10000);
> 
> How do other drivers handle this?
> Can -EPROTO happen in other cases, not only unplugging, which this
> patch would break?

Yes, -EPROTO may be a transient error, although they are relatively
rare as some retries are done before the URB fails. This patch will
only break things if things work in the first place, i.e. the driver
has sensible retry policies, the hardware doesn't get confused, etc.

Note that -EPROTO is not guaranteed in this case, see
Documentation/driver-api/usb/error-codes.rst

I know that xHCI gives -EPROTO and it looks like EHCI does too (IDK
if this is reliable), but I just checked that OHCI gives -ETIME.


I don't have this hardware, but I played with some other wired/WiFi
dongles and observed similar problems of varying severity.

It looks to me like USB core doesn't actually return -ENODEV on
disconnected devices, or at least doesn't react to disconnection
until .probe() returns, I am not yet sure which of those.

And drivers don't seem to expect -EPROTO or -ETIME to be fatal.

And maybe they should, if they wouldn't be able to recover from it
anyway, and I know that there are drivers which can't. But I think
this USB subsystem behavior is suboptimal too.

Regards,
Michal

Patch

diff --git a/drivers/net/wireless/mediatek/mt76/usb.c b/drivers/net/wireless/mediatek/mt76/usb.c
index 58ff06823389..f9e67b8c3b3c 100644
--- a/drivers/net/wireless/mediatek/mt76/usb.c
+++ b/drivers/net/wireless/mediatek/mt76/usb.c
@@ -33,9 +33,9 @@  int __mt76u_vendor_request(struct mt76_dev *dev, u8 req, u8 req_type,
 
 		ret = usb_control_msg(udev, pipe, req, req_type, val,
 				      offset, buf, len, MT_VEND_REQ_TOUT_MS);
-		if (ret == -ENODEV)
+		if (ret == -ENODEV || ret == -EPROTO)
 			set_bit(MT76_REMOVED, &dev->phy.state);
-		if (ret >= 0 || ret == -ENODEV)
+		if (ret >= 0 || ret == -ENODEV || ret == -EPROTO)
 			return ret;
 		usleep_range(5000, 10000);
 	}