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