Hi David,
I think your patch will not always do the right thing.
The halt-bit in the ED-head-pointer indicates that the HC halts the ED
due to an error (including a short packet = DATAUNDERRUN).
If the transfer_buffer of a transfer is larger then 4KB (more then 1 TD)
then a DATAUNDERRUN needs not be an error and therefore a halt
of the endpoint would be wrong.
So an error or halt of the HC ED does not automatically
mean that the endpoint itself
is also halted (=stalled).
Even though, to halt the endpoint on any real error
would be a good practise.
Roman
David Brownell wrote:
>
> > Apparently, when using usb-ohci with an interrupt endpoint and you
> > disconnect the device physically, the interrupt callback is called one
> > more time before the _disconnect() function is called.
>
> The experiments I just tried (keyboard) showed _lots_ of completion
> callbacks, all with 110/ETIMEDOUT. As seems right: each time
> the controller tried to poll the device, it didn't respond. And whatever
> got those callbacks didn't unlink the URB, so the next poll failed too.
>
> After a while, the (root) hub noticed it the port was gone, and then
> called disconnect().
>
> > Using a UHCI driver, the 'state' field of the callback is set to -ENOENT.
>
> Callback -- or the code that unlinks the URB? I found a case where there
> was a clear driver difference, but it was the unlinking case.
>
> > However, with usb-ohci, this is set to -110 (I think that's -ETIMEDOUT).
> > I think this should be -ENOENT. At the very least the UHCI and OHCI
> > drivers should show the same behavior.
>
> Please try the enclosed patch. It does three things to improve consistency
> of the (three) drivers:
>
> * Makes unlinking active URBs return ENOENT. I hope this will do what
> you mentioned above, but I can't test it.
>
> * When it notices endpoints the controller marked as halted (and which are
> haltable -- e.g. no control endpoints) it flags them as halted. It didn't
> do
> this correctly before, it missed most halts including
> "DeviceNotResponding"
> which caused the ETIMEDOUT.
>
> * Submitting URBs against halted endpoints get EPIPEs (like the UHCIs).
>
> I think mass storage is the main driver that'll check for halted endpoints,
> so
> if that second change turns up any problems, you'll be most likely to
> notice.
>
> - Dave
>
> p.s. I hope outlook doesn't mangle this patch; Netscape died a horrible
> death, even new installations coredump on startup, and I'm digging
> out from under the mess that left me. Sigh. What I see below has no
> tabs and probably has carriage returns too.
>
> p.p.s. I generated this against pre4 or so, since I can't boot current
> kernels,
> but I don't remember these parts of the OHCI driver changing much.
>
> --- linux/drivers/usb/usb-ohci.c-orig Fri Apr 14 14:32:46 2000
> +++ linux/drivers/usb/usb-ohci.c Fri Apr 14 21:00:16 2000
> @@ -414,9 +414,10 @@
>
> if (urb->hcpriv) return -EINVAL; /* urb already in use */
>
> -// if(usb_endpoint_halted (urb->dev, usb_pipeendpoint (pipe), usb_pipeout
> (pipe)))
> -// return -EPIPE;
> -
> + if (usb_endpoint_halted (urb->dev,
> + usb_pipeendpoint (pipe), usb_pipeout (pipe)))
> + return -EPIPE;
> +
> usb_inc_dev_use (urb->dev);
> ohci = (ohci_t *) urb->dev->bus->hcpriv;
>
> @@ -554,6 +555,7 @@
> remove_wait_queue (&op_wakeup, &wait);
> else
> err("unlink URB timeout!");
> + urb->status = USB_ST_URB_KILLED;
> } else
> urb_rm_priv (urb);
> usb_dec_dev_use (urb->dev);
> @@ -1097,8 +1099,21 @@
> if (TD_CC_GET (le32_to_cpup (&td_list->hwINFO))) {
> urb_priv = (urb_priv_t *) td_list->urb->hcpriv;
> dbg(" USB-error/status: %x : %p",
> - TD_CC_GET (le32_to_cpup (&td_list->hwINFO)), td_list);
> + TD_CC_GET (le32_to_cpup (&td_list->hwINFO)),
> + td_list);
> +
> + // did the controller halt the endpoint?
> if (td_list->ed->hwHeadP & cpu_to_le32 (0x1)) {
> + urb_t *urb = td_list->urb;
> + int type = usb_pipetype (urb->pipe);
> +
> + // iso and control can't really halt
> + if (type != PIPE_ISOCHRONOUS
> + && type != PIPE_CONTROL)
> + usb_endpoint_halt (urb->dev,
> + usb_pipeendpoint (urb->pipe),
> + usb_pipeout (urb->pipe));
> +
> if (urb_priv && ((td_list->index + 1) < urb_priv->length)) {
> td_list->ed->hwHeadP =
> (urb_priv->td[urb_priv->length - 1]->hwNextTD & cpu_to_le32
> (0xfffffff0)) |
> @@ -1243,7 +1258,6 @@
> }
> /* error code of transfer */
> cc = TD_CC_GET (tdINFO);
> - if( cc == TD_CC_STALL) usb_endpoint_halt(urb->dev,
> usb_pipeendpoint(urb->pipe), usb_pipeout(urb->pipe));
>
> if (!(urb->transfer_flags & USB_DISABLE_SPD) && (cc ==
> TD_DATAUNDERRUN))
> cc = TD_CC_NOERROR;
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [EMAIL PROTECTED]
> For additional commands, e-mail: [EMAIL PROTECTED]
---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
