[PATCH v1] drivers: usb: wwan: treat any error as a fatal error

[PATCH v1] drivers: usb: wwan: treat any error as a fatal error

[qianfanguijin]

From: qianfan Zhao <qianfanguijin@163.com>

Kernel print such flood message when the modem dead (the device is not
disconnected but it doesn't response anything):

option1 ttyUSB1: usb_wwan_indat_callback: nonzero status: -71 on endpoint 05.
option1 ttyUSB1: usb_wwan_indat_callback: nonzero status: -71 on endpoint 05.
...

So treat any error that doesn't recognized as a fatal error and do not
resubmit again.

Signed-off-by: qianfan Zhao <qianfanguijin@163.com>
---
 drivers/usb/serial/usb_wwan.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/usb/serial/usb_wwan.c b/drivers/usb/serial/usb_wwan.c
index cb01283d4d15..daa3e2beff0f 100644
--- a/drivers/usb/serial/usb_wwan.c
+++ b/drivers/usb/serial/usb_wwan.c
@@ -227,8 +227,7 @@ static void usb_wwan_indat_callback(struct urb *urb)
 			__func__, status, endpoint);
 
 		/* don't resubmit on fatal errors */
-		if (status == -ESHUTDOWN || status == -ENOENT)
-			return;
+		return;
 	} else {
 		if (urb->actual_length) {
 			tty_insert_flip_string(&port->port, data,
-- 
2.25.1

Re: [PATCH v1] drivers: usb: wwan: treat any error as a fatal error

[Johan Hovold]

On Fri, Apr 14, 2023 at 01:53:06PM +0800, qianfanguijin@163.com wrote:
> From: qianfan Zhao <qianfanguijin@163.com>
> 
> Kernel print such flood message when the modem dead (the device is not
> disconnected but it doesn't response anything):
> 
> option1 ttyUSB1: usb_wwan_indat_callback: nonzero status: -71 on endpoint 05.
> option1 ttyUSB1: usb_wwan_indat_callback: nonzero status: -71 on endpoint 05.
> ...
> 
> So treat any error that doesn't recognized as a fatal error and do not
> resubmit again.

This could potentially break setups that are currently able to recover
from intermittent errors. 

Try adding the missing known fatal ones as you suggested in your other
thread first.

There could still be an issue with -EPROTO (-71) error that would
require some kind of back-off or limit, but that would need to be
implemented in a more central place rather than in each and every usb
driver (as has been discussed in the past).

> Signed-off-by: qianfan Zhao <qianfanguijin@163.com>
> ---
>  drivers/usb/serial/usb_wwan.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/serial/usb_wwan.c b/drivers/usb/serial/usb_wwan.c
> index cb01283d4d15..daa3e2beff0f 100644
> --- a/drivers/usb/serial/usb_wwan.c
> +++ b/drivers/usb/serial/usb_wwan.c
> @@ -227,8 +227,7 @@ static void usb_wwan_indat_callback(struct urb *urb)
>  			__func__, status, endpoint);
>  
>  		/* don't resubmit on fatal errors */
> -		if (status == -ESHUTDOWN || status == -ENOENT)
> -			return;
> +		return;
>  	} else {
>  		if (urb->actual_length) {
>  			tty_insert_flip_string(&port->port, data,

Johan

Re: [PATCH v1] drivers: usb: wwan: treat any error as a fatal error

[Oliver Neukum]

On 14.04.23 09:01, Johan Hovold wrote:
> On Fri, Apr 14, 2023 at 01:53:06PM +0800, qianfanguijin@163.com wrote:
>> From: qianfan Zhao <qianfanguijin@163.com>
>>
>> Kernel print such flood message when the modem dead (the device is not
>> disconnected but it doesn't response anything):
>>
>> option1 ttyUSB1: usb_wwan_indat_callback: nonzero status: -71 on endpoint 05.
>> option1 ttyUSB1: usb_wwan_indat_callback: nonzero status: -71 on endpoint 05.
>> ...
>>
>> So treat any error that doesn't recognized as a fatal error and do not
>> resubmit again.
> 
> This could potentially break setups that are currently able to recover
> from intermittent errors.

Yes. The basic issue is that a physically disconnected device
produces the same errors as an intermittent failure for a short
time before the disconnection is detected.

Hence the correct way to handle this would be like usbhid does
with hid_io_error(), that is a delay before resubmitting
and eventually a device reset.

> Try adding the missing known fatal ones as you suggested in your other
> thread first.
> 
> There could still be an issue with -EPROTO (-71) error that would
> require some kind of back-off or limit, but that would need to be
> implemented in a more central place rather than in each and every usb
> driver (as has been discussed in the past).

Exactly. How would that look like conceptually?
A centralized work with a pool of URBs to be retried after a delay
and eventually a device reset?

Handling unbinding a driver would be tough, though.

	Regards
		Oliver

Re: [PATCH v1] drivers: usb: wwan: treat any error as a fatal error

[qianfan]

在 2023/4/17 17:50, Oliver Neukum 写道:
>
>
> On 14.04.23 09:01, Johan Hovold wrote:
>> On Fri, Apr 14, 2023 at 01:53:06PM +0800, qianfanguijin@163.com wrote:
>>> From: qianfan Zhao <qianfanguijin@163.com>
>>>
>>> Kernel print such flood message when the modem dead (the device is not
>>> disconnected but it doesn't response anything):
>>>
>>> option1 ttyUSB1: usb_wwan_indat_callback: nonzero status: -71 on 
>>> endpoint 05.
>>> option1 ttyUSB1: usb_wwan_indat_callback: nonzero status: -71 on 
>>> endpoint 05.
>>> ...
>>>
>>> So treat any error that doesn't recognized as a fatal error and do not
>>> resubmit again.
>>
>> This could potentially break setups that are currently able to recover
>> from intermittent errors.
>
> Yes. The basic issue is that a physically disconnected device
> produces the same errors as an intermittent failure for a short
> time before the disconnection is detected.
>
> Hence the correct way to handle this would be like usbhid does
> with hid_io_error(), that is a delay before resubmitting
> and eventually a device reset.
Thanks, and `acm_read_bulk_callback` is also a good example, create a
delayed work and resubmit later.
>
>> Try adding the missing known fatal ones as you suggested in your other
>> thread first.
>>
>> There could still be an issue with -EPROTO (-71) error that would
>> require some kind of back-off or limit, but that would need to be
>> implemented in a more central place rather than in each and every usb
>> driver (as has been discussed in the past).
>
> Exactly. How would that look like conceptually?
> A centralized work with a pool of URBs to be retried after a delay
> and eventually a device reset?
>
> Handling unbinding a driver would be tough, though.
>
>     Regards
>         Oliver

Re: [PATCH v1] drivers: usb: wwan: treat any error as a fatal error

[Johan Hovold]

Hi Oliver,

and sorry about the late follow-up on this. Was travelling last week.

On Mon, Apr 17, 2023 at 11:50:34AM +0200, Oliver Neukum wrote:
> 
> 
> On 14.04.23 09:01, Johan Hovold wrote:
> > On Fri, Apr 14, 2023 at 01:53:06PM +0800, qianfanguijin@163.com wrote:
> >> From: qianfan Zhao <qianfanguijin@163.com>
> >>
> >> Kernel print such flood message when the modem dead (the device is not
> >> disconnected but it doesn't response anything):
> >>
> >> option1 ttyUSB1: usb_wwan_indat_callback: nonzero status: -71 on endpoint 05.
> >> option1 ttyUSB1: usb_wwan_indat_callback: nonzero status: -71 on endpoint 05.
> >> ...
> >>
> >> So treat any error that doesn't recognized as a fatal error and do not
> >> resubmit again.
> > 
> > This could potentially break setups that are currently able to recover
> > from intermittent errors.
> 
> Yes. The basic issue is that a physically disconnected device
> produces the same errors as an intermittent failure for a short
> time before the disconnection is detected.
> 
> Hence the correct way to handle this would be like usbhid does
> with hid_io_error(), that is a delay before resubmitting
> and eventually a device reset.
> 
> > Try adding the missing known fatal ones as you suggested in your other
> > thread first.
> > 
> > There could still be an issue with -EPROTO (-71) error that would
> > require some kind of back-off or limit, but that would need to be
> > implemented in a more central place rather than in each and every usb
> > driver (as has been discussed in the past).
> 
> Exactly. How would that look like conceptually?
> A centralized work with a pool of URBs to be retried after a delay
> and eventually a device reset?

I haven't tried to solve this yet, so I don't have a solution, but
ideally this would work seamlessly for drivers either by handling it in
core or possibly in the affected host-controller drivers if it's just
some of them.

If that's not doable, we should at least try to provide a generic
implementation which we'd then need to hook up each and every driver to
use.
 
> Handling unbinding a driver would be tough, though.

Why would that be a problem? We should be able to differentiate a
stopped URB from other errors, right?

Johan