> I think your patch will not always do the right thing.
I was going by subsection 4.3.1.3.6 of the OHCI spec, which lists
three generic categories of transfer errors and then says which
of them will halt the endpoint. (Most of them ... it retries first
for transmission errors like DeviceNotResponding.)
> 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.
The text in 4.3.1.3.6.2 saying underrun (only) "is not always an
error" also says that it's affected by the "bufferRounding" (R) bit.
Basically, if that bit is set, then short packets (as set up by the
HCD) won't be reported as errors. You set that for IN endpoints
that could really halt (bulk, interrupt), so I don't see how one of
those non-error cases could happen.
I don't see any restrictions there for queues with multiple TDs.
Even if I send, say, 4097 bytes (one max size URB, and one
short one), and someone queues another transfer after that, it
should behave. Am I missing some information?
> 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.
The OHCI spec does say it halts the "endpoint". Though of
course there are cases where the device and the HC won't
see the same facts and so might conclude differently. The
OHCI spec is effectively saying it just flags unrecoverable
errors and expects other software to figure it out; though not
many USB modules check for halts yet.
I tried this patch on a bunch of different devices, no trouble.
As I said, I think the storage driver is most likely to run into
problems if this causes any, since most other software ignores
"halt" conditions like those showing up during unplugs. Right
now those error paths aren't really getting used with OHCI.
- Dave
>
>
> 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]
