On Tue, 19 Jul 2011, Arvid Brodin wrote: > Alan Stern wrote: > > On Tue, 19 Jul 2011, Arvid Brodin wrote: > > > >> I'm not comfortable with the usage of usb_hub_clear_tt_buffer(). The > >> function takes an urb as an argument (the urb that was involved in the > >> unlink or failed transaction) and may fail. This means that one have to > >> have a way of storing the offending urb and deferring calling > >> usb_hcd_giveback_urb() on it if the usb_hub_clear_tt_buffer() call fails. > > > > No, not really. If usb_hub_clear_tt_buffer() fails, you're stuck -- we > > don't have any way to recover. Hence there's no reason to store the > > offending URB or defer giving it back. > > > > Fortunately, the only failure mode is if there's not enough free memory > > to allocate a reasonably small data structure. > > Well, since the only failure mode is lack of memory, isn't there a good > chance that trying again a bit later would succeed? (I.e. the only reason > we're stuck is because we gave back the urb containing the info we need?) We can't try later because the routine runs in interrupt context. It's not allowed to block and wait. > Now I realise this is mostly academic since we would have a world of > trouble anyway on a system that low on memory, but given Murphy, sooner or > later something is going hit exactly that failure mode, probably at the > worst possible time... It's a graceful sort of failure. Every TT has multiple buffers; failure to clear a buffer would mean that you'd have to rely on the other buffers. Of course, after enough errors like this there wouldn't be any buffers left. Then you'd encounter the sort of problems you saw with usbtest. > And even though this might have a low probability of occuring, it's bad > since a reboot would then be needed to get USB on-line again. You wouldn't need a reboot. Reinitializing the hub containing the TT would be good enough. > I have no great idea how to solve this though. > > I'm not sure if > > -int usb_hub_clear_tt_buffer(struct urb *urb) > +int usb_hub_clear_tt_buffer(struct usb_device *udev, > + struct usb_host_endpoint *ep, int pipe) > > would help? The one exisiting call to this function could be changed > > -usb_hub_clear_tt_buffer(urb); > +usb_hub_clear_tt_buffer(urb->dev, urb->ep, urb->pipe); > > and I could store these parameters in my queue head struct to enable > retrying, but will these values always be valid, even after the urb > has been given back? Also, this solution feels awkward. > > Perhaps an error message will have to do... Yes; I don't think it's worth worrying about. At some point you just have to admit that things have gotten too bad and the situation isn't recoverable. Also, usb_hub_clear_tt_buffer() already logs an error message when the kmalloc fails. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
