David Brownell wrote:
>
> > 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 just set the (R) bit on the last TD of multi TD transfers.
> 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?
>
But if we request 4097 bytes
(2 TDs, 1st without (R) bit, 2nd has (R) bit)
and we just get 4095 (1TD without (R)) then we get an
DATAUNDERRUN error that is no real error (well, depends
on the USB_DISABLE_SPD flag).
And this DATAUNDERRUN error tells us that we have to remove
the second TD because the requested transfer is done and we
do not need the 2nd TD anymore.
In this case your patch would indicate a halt of the endpoint,
and that is 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.
>
> 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.
>
Most if not all drivers now use relative short buffers (<=4096 Byte),
so you just don't see the trouble.
But this could change.
Roman
> 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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
