| Message ID | 20230414055306.8805-1-qianfanguijin@163.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v1] drivers: usb: wwan: treat any error as a fatal error | expand |
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
在 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
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
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,