Message ID | CA+v9cxad+THL8u7tr_ubD7H7o+6x+qTOXOgowmPDnWUxa8aJtQ@mail.gmail.com |
---|---|
State | Rejected, archived |
Delegated to: | David Miller |
Headers | show |
From: Huajun Li <huajun.li.lee@gmail.com> Date: Wed, 13 Jun 2012 20:50:31 +0800 > intr_complete() submits URB even the interrupt endpoint stalls. > This patch will try to activate the endpoint once the exception > occurs, and then re-submit the URB if the endpoint works again. > > Signed-off-by: Huajun Li <huajun.li.lee@gmail.com> Review from USB experts would be appreciated. Thanks. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Am Montag, 18. Juni 2012, 01:30:17 schrieb David Miller: > From: Huajun Li <huajun.li.lee@gmail.com> > Date: Wed, 13 Jun 2012 20:50:31 +0800 > > > intr_complete() submits URB even the interrupt endpoint stalls. > > This patch will try to activate the endpoint once the exception > > occurs, and then re-submit the URB if the endpoint works again. > > > > Signed-off-by: Huajun Li <huajun.li.lee@gmail.com> > > Review from USB experts would be appreciated. The code implements a minimum error handler correctly. Did you observe a stall in actual hardware or is this a just in case patch? Regards Oliver -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jun 18, 2012 at 3:23 PM, Oliver Neukum <oneukum@suse.de> wrote: > Am Montag, 18. Juni 2012, 01:30:17 schrieb David Miller: >> From: Huajun Li <huajun.li.lee@gmail.com> >> Date: Wed, 13 Jun 2012 20:50:31 +0800 >> >> > intr_complete() submits URB even the interrupt endpoint stalls. >> > This patch will try to activate the endpoint once the exception >> > occurs, and then re-submit the URB if the endpoint works again. >> > >> > Signed-off-by: Huajun Li <huajun.li.lee@gmail.com> >> >> Review from USB experts would be appreciated. > > The code implements a minimum error handler correctly. > Did you observe a stall in actual hardware or is this a just > in case patch? > This one is just a patch, thanks for your comments. > Regards > Oliver -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Am Montag, 18. Juni 2012, 18:25:15 schrieb Huajun Li: > On Mon, Jun 18, 2012 at 3:23 PM, Oliver Neukum <oneukum@suse.de> wrote: > > Am Montag, 18. Juni 2012, 01:30:17 schrieb David Miller: > >> From: Huajun Li <huajun.li.lee@gmail.com> > >> Date: Wed, 13 Jun 2012 20:50:31 +0800 > >> > >> > intr_complete() submits URB even the interrupt endpoint stalls. > >> > This patch will try to activate the endpoint once the exception > >> > occurs, and then re-submit the URB if the endpoint works again. > >> > > >> > Signed-off-by: Huajun Li <huajun.li.lee@gmail.com> > >> > >> Review from USB experts would be appreciated. > > > > The code implements a minimum error handler correctly. > > Did you observe a stall in actual hardware or is this a just > > in case patch? > > > > This one is just a patch, thanks for your comments. Then I am inclined to say that this is not fully thought through If the endpoint stalls, the device has detected an error. We have no idea how to generically handle this error. We might come into a vicious circle. Could you add a sanity limit for the amount of halts we clear? Regards Oliver -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jun 19, 2012 at 12:51 AM, Oliver Neukum <oneukum@suse.de> wrote: > > Then I am inclined to say that this is not fully thought through > If the endpoint stalls, the device has detected an error. > We have no idea how to generically handle this error. > We might come into a vicious circle. Why does it come into a vicious circle? If the interrupt endpoint is HALTed, the clear halt event is scheduled to call usb_clear_halt(int pipe), and if it returns successfully, the pipe shouldn't be halted any more, so the later usb_submit_urb() should be OK on the interrupt endpoint. The similar handling policy is taken for RX/TX endpoints in case they are halted. > Could you add a sanity limit for the > amount of halts we clear? Looks it is not needed, at least clearing HALT on Rx/Tx endpoints works well without the limit. Thanks,
Am Montag, 18. Juni 2012, 18:25:15 schrieb Huajun Li: > On Mon, Jun 18, 2012 at 3:23 PM, Oliver Neukum <oneukum@suse.de> wrote: > > Am Montag, 18. Juni 2012, 01:30:17 schrieb David Miller: > >> From: Huajun Li <huajun.li.lee@gmail.com> > >> Date: Wed, 13 Jun 2012 20:50:31 +0800 > >> > >> > intr_complete() submits URB even the interrupt endpoint stalls. > >> > This patch will try to activate the endpoint once the exception > >> > occurs, and then re-submit the URB if the endpoint works again. > >> > > >> > Signed-off-by: Huajun Li <huajun.li.lee@gmail.com> > >> > >> Review from USB experts would be appreciated. > > > > The code implements a minimum error handler correctly. > > Did you observe a stall in actual hardware or is this a just > > in case patch? > > > > This one is just a patch, thanks for your comments. Very well, on second thought, this patch makes sense. Could you resend and I'll ack? Regards Oliver -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jun 20, 2012 at 4:00 PM, Oliver Neukum <oneukum@suse.de> wrote: > > Very well, on second thought, this patch makes sense. > Could you resend and I'll ack? BTW, maybe it is better to add below usbnet_defer_kevent(dev, EVENT_STS_HALT); for -EPIPE returned from usb_urb_submit if it will be resent. thanks,
Am Mittwoch, 20. Juni 2012, 10:07:55 schrieb Ming Lei: > On Wed, Jun 20, 2012 at 4:00 PM, Oliver Neukum <oneukum@suse.de> wrote: > > > > Very well, on second thought, this patch makes sense. > > Could you resend and I'll ack? > > BTW, maybe it is better to add below > > usbnet_defer_kevent(dev, EVENT_STS_HALT); > > for -EPIPE returned from usb_urb_submit if it will be resent. Why? If it failed once it'll probably also fail the next time. In that case we'd need to do something more intrusive like resetting the device, but that cannot be done well in the generic usbnet part. Regards Oliver -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jun 20, 2012 at 4:58 PM, Oliver Neukum <oneukum@suse.de> wrote: > Am Mittwoch, 20. Juni 2012, 10:07:55 schrieb Ming Lei: >> BTW, maybe it is better to add below >> >> usbnet_defer_kevent(dev, EVENT_STS_HALT); >> >> for -EPIPE returned from usb_urb_submit if it will be resent. > > Why? If it failed once it'll probably also fail the next time. -EPIPE just means the endpoint is halted, either from usb_urb_submit or urb->status, so the HALT should be cleared in the situation. > In that case we'd need to do something more intrusive > like resetting the device, but that cannot be done well > in the generic usbnet part. IMO, resetting is not needed for -EPIPE, but may be needed for -EPROTO failure. Thanks,
Am Mittwoch, 20. Juni 2012, 12:15:25 schrieb Ming Lei: > On Wed, Jun 20, 2012 at 4:58 PM, Oliver Neukum <oneukum@suse.de> wrote: > > Am Mittwoch, 20. Juni 2012, 10:07:55 schrieb Ming Lei: > >> BTW, maybe it is better to add below > >> > >> usbnet_defer_kevent(dev, EVENT_STS_HALT); > >> > >> for -EPIPE returned from usb_urb_submit if it will be resent. > > > > Why? If it failed once it'll probably also fail the next time. > > -EPIPE just means the endpoint is halted, either from usb_urb_submit > or urb->status, so the HALT should be cleared in the situation. It probably was halted and cleared. However that you cleared a halt doesn't mean that the reason for stalling went away. So you must cope with an endpoint being halted again right after it was cleared. > > In that case we'd need to do something more intrusive > > like resetting the device, but that cannot be done well > > in the generic usbnet part. > > IMO, resetting is not needed for -EPIPE, but may be needed for > -EPROTO failure. We don't need it for a single failure, but what else would we do if we keep getting -EPIPE? Regards Oliver -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jun 20, 2012 at 6:21 PM, Oliver Neukum <oneukum@suse.de> wrote: > It probably was halted and cleared. However that you cleared > a halt doesn't mean that the reason for stalling went away. > So you must cope with an endpoint being halted again right after > it was cleared. I only suggested we should handle -EPIPE for usb_submit_urb on interrupt endpoint, maybe it is the 1st handling, at least it is per USB spec. Also from implementation of usb gadget device, generally ClearFeature(HALT) is to clear the some halt related flag of endpoint hardware. Looks the reasons of interrupt endpoint stalling is invisible for usbnet driver, so it is not easy to handle the situation you described(halted and cleared repeatedly). > >> > In that case we'd need to do something more intrusive >> > like resetting the device, but that cannot be done well >> > in the generic usbnet part. >> >> IMO, resetting is not needed for -EPIPE, but may be needed for >> -EPROTO failure. > > We don't need it for a single failure, but what else would we do > if we keep getting -EPIPE? Suppose the case will happen, what is the appropriate actions usbnet should take on the failure? I am not sure RESET can deal with it. Also is it a actual failure case or only a theory case? Thanks,
diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c index ac2e493..314aaea 100644 --- a/drivers/net/usb/usbnet.c +++ b/drivers/net/usb/usbnet.c @@ -198,6 +198,10 @@ static void intr_complete (struct urb *urb) "intr shutdown, code %d\n", status); return; + case -EPIPE: + usbnet_defer_kevent(dev, EVENT_STS_HALT); + return; + /* NOTE: not throttling like RX/TX, since this endpoint * already polls infrequently */ @@ -964,6 +968,33 @@ fail_halt: } } + if (test_bit(EVENT_STS_HALT, &dev->flags)) { + unsigned pipe; + struct usb_endpoint_descriptor *desc; + + desc = &dev->status->desc; + pipe = usb_rcvintpipe(dev->udev, + desc->bEndpointAddress & USB_ENDPOINT_NUMBER_MASK); + status = usb_autopm_get_interface(dev->intf); + if (status < 0) + goto fail_sts; + status = usb_clear_halt(dev->udev, pipe); + if (status < 0) { + usb_autopm_put_interface(dev->intf); +fail_sts: + netdev_err(dev->net, + "can't clear intr halt, status %d\n", status); + } else { + clear_bit(EVENT_STS_HALT, &dev->flags); + status = usb_submit_urb(dev->interrupt, GFP_KERNEL); + if (status != 0) + netif_err(dev, timer, dev->net, + "intr resubmit --> %d\n", status); + + usb_autopm_put_interface(dev->intf); + } + } + /* tasklet could resubmit itself forever if memory is tight */ if (test_bit (EVENT_RX_MEMORY, &dev->flags)) { struct urb *urb = NULL; diff --git a/include/linux/usb/usbnet.h b/include/linux/usb/usbnet.h index f87cf62..81b4473 100644 --- a/include/linux/usb/usbnet.h +++ b/include/linux/usb/usbnet.h @@ -62,12 +62,13 @@ struct usbnet { unsigned long flags; # define EVENT_TX_HALT 0 # define EVENT_RX_HALT 1 -# define EVENT_RX_MEMORY 2 -# define EVENT_STS_SPLIT 3 -# define EVENT_LINK_RESET 4 -# define EVENT_RX_PAUSED 5 -# define EVENT_DEV_ASLEEP 6 -# define EVENT_DEV_OPEN 7 +# define EVENT_STS_HALT 2 +# define EVENT_RX_MEMORY 3 +# define EVENT_STS_SPLIT 4 +# define EVENT_LINK_RESET 5 +# define EVENT_RX_PAUSED 6 +# define EVENT_DEV_ASLEEP 7 +# define EVENT_DEV_OPEN 8 }; static inline struct usb_driver *driver_of(struct usb_interface *intf)
intr_complete() submits URB even the interrupt endpoint stalls. This patch will try to activate the endpoint once the exception occurs, and then re-submit the URB if the endpoint works again. Signed-off-by: Huajun Li <huajun.li.lee@gmail.com> --- drivers/net/usb/usbnet.c | 31 +++++++++++++++++++++++++++++++ include/linux/usb/usbnet.h | 13 +++++++------ 2 files changed, 38 insertions(+), 6 deletions(-)