Date: Wed, 29 Dec 2004 08:12:19 +1100 From: John Birrell <jb@cimlogic.com.au> To: ticso@cicely.de Cc: Julian Elischer <julian@elischer.org> Subject: Re: USB problems Message-ID: <20041228211219.GE61668@freebsd3.cimlogic.com.au> In-Reply-To: <20041228202832.GN81585@cicely12.cicely.de> References: <20041228010938.GA39686@freebsd3.cimlogic.com.au> <41D0C63F.3000702@elischer.org> <20041228025836.GA53223@freebsd3.cimlogic.com.au> <20041228152212.GM81585@cicely12.cicely.de> <20041228195436.GA61668@freebsd3.cimlogic.com.au> <20041228202832.GN81585@cicely12.cicely.de>
next in thread | previous in thread | raw e-mail | index | archive | help
On Tue, Dec 28, 2004 at 09:28:33PM +0100, Bernd Walter wrote:
> Well, getting a stall on control endpoint is an error, but getting an
> error on string descriptors shouldn't stop further processing.
Windows obviously begs to differ. This reference design comes from the
Singapore lab of Philips which I believe also contains the engineers
who contributed to the USB specification.
The firmware code looks like it was written by an inexperienced hardware
engineer. It is only provided as an example by Philips. They intend you
to write your own when you build your own board. I will do that when I
get the ability to burn 8051 code. In the meantime, I want FreeBSD's
USB implementation to function as well as Windows. That means coming
up with an implementation that tolerates the control endpoint being
stalled.
> > The string descriptors are returned normally by the device, except for the
> > fact that it sets the stall bit on the control endpoint.
>
> Here is the device broken, unless FreeBSD's request is not correct.
Windows can talk to it though, so at least it's possible to get around
aspects of the device which, I agree, are probably broken.
The thing that nags me, though, is that this is a 'reference design' and
that means that any company choosing to build an MPEG2 encoder based
on Philips' SAA7114, SAA6752 and ISP1581 is going to start with this
firmware code and build a product that probably has the same behaviour.
Philips only offer support to companies who are planning to by large
numbers of chips. That means I'm on my own here. I can't communicate
with their engineers in Singapore. I've tried - they won't respond.
> The device shouldn't stall on requesting partial descriptors.
> IIRC FreeBSD changed the way how descriptors are fetched normaly in
> that it doesn't trust the size fields anymore and asks for more data
> than required.
It also shouldn't stall getting the configuration. Or getting status.
But it is coded to do that.
> > The FreeBSD code *never* takes into account that the control endpoint
> > stall will be cleared with the next setup packet.
>
> Maybe the host controller needs any kind of unstall mimic, but not the
> device.
> The question is more what happens with further requests after that
> error.
Further requests start with a setup packet, so the stall is cleared when
that is received. The problem is that FreeBSD gives up on talking to the
device because it treats the stall condition as an error. It isn't
even possible for FreeBSD to get past the device initialisation because
the first 8 bytes of the device descriptor are returned with the stall
bit set. At that point, FreeBSD barfs.
> > I think that a control endpoint transfer should not return USBD_STALLED
> > to the caller unless the length received is less than that requested
> > and ~SHORT_TRANSER_OK. A stall on the control endpoint should only be an
> > error if the controller has a problem with the protocol (i.e. it is
> > a protocol stall, not a device stall);
>
> I had something like this localy running (cut & paste'd):
> Index: usbdi.c
> ===================================================================
> RCS file: /home/ncvs/src/sys/dev/usb/usbdi.c,v
> retrieving revision 1.76
> diff -u -r1.76 usbdi.c
> --- usbdi.c 26 May 2002 22:00:06 -0000 1.76
> +++ usbdi.c 15 Jun 2003 21:32:34 -0000
> @@ -931,8 +931,17 @@
> usbd_do_request_flags(usbd_device_handle dev, usb_device_request_t *req,
> void *data, u_int16_t flags, int *actlen, u_int32_t timo)
> {
> - return (usbd_do_request_flags_pipe(dev, dev->default_pipe, req,
> - data, flags, actlen, timo));
> + int actlen2;
> + usbd_status ret;
> +
> + ret = usbd_do_request_flags_pipe(dev, dev->default_pipe, req,
> + data, flags, &actlen2, timo);
> + if (actlen != NULL)
> + *actlen = actlen2;
> + if (ret == USBD_STALLED && actlen2 == UGETW(req->wLength)) {
> + ret = USBD_NORMAL_COMPLETION;
> + }
> + return (ret);
> }
>
> usbd_status
>
> But it's wrong.
> You just don't know if the received data is correct when receiving
> a stall - especially for requests without data.
> This is something to hand up to the driver which might have a clue
> about a specific device brokenness.
A problem with FreeBSD's code is that some of the places where the driver
*could* interpret the stall condition differently and actually handled
in common routines that are hard-coded to treat the stall as an error.
Take the configuration descriptor as an example. The FreeBSD code tries
to get the first 9 bytes to discover the wTotalLength, then it goes and
gets the full descriptor (including interfaces and endpoints). In the
case of this rogue device, the get of the first 9 bytes results in a
stall. FreeBSD's code then fails the entire configuration descriptor
get and it isn't possible for a driver to configure the device.
--
John Birrell
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20041228211219.GE61668>