On Sunday 18 March 2007 8:16 pm, Li Yang-r58472 wrote:
> > -----Original Message-----
> > From: David Brownell [mailto:[EMAIL PROTECTED]
> > Sent: Saturday, March 17, 2007 6:38 AM
> > To: [email protected]
> > Cc: Li Yang-r58472; [EMAIL PROTECTED]
> > Subject: Re: [linux-usb-devel] [PATCH] Fix gadget serial response on
> > USB_CDC_REQ_SET_LINE_CODING
> >
> > On Wednesday 14 March 2007 5:18 am, Li Yang wrote:
> > > Apparently this gadget doesn't support setting usb_cdc_line_coding.
> >
> > It does support it ... but that support is buggy. It should be
> issuing
> > a read request before doing the memcpy(); instead, it wrongly assumes
> > that something else already issued the read.
>
> Memcpy() the line_coding from host to the private port_line_coding
> structure doesn't mean the port is really changed. Port_line_coding
> structure isn't used anywhere in the driver accept it fools around the
> next USB_CDC_REQ_GET_LINE_CODING request from host.
So if it's not used ... in what sense is the port *NOT* changed?
Remember, it's not hooked up to a serial line right now. And other
than asking for 8N1 (etc) "coding", that would be the only way to
observe changes.
That is, if the memcpy were done *correctly* there would be no way
to demonstrate what you're claiming. Because every testable behavior
would show the opposite.
> > Plus, it doesn't correctly test for some error cases: port is null,
> > there's not enough incoming data.
> >
> >
> > > But the code pretends that it supports, and queues a zero length
> > > request for the DATA phase. That will cause overflow for udc in
> > > processing the DATA transaction. The patch make it return
> > > -EOPNOTSUPP and stall cleanly.
> >
> > So, a workaround for the bug. Did you notice how the other SET
> > request has the same bug?
> >
> > If you're not going to really fix this, please update the patch
> > to address the same bug in the other path; and its description
> > to say that you're replacing buggy code with an explicit failure.
> >
> > Best of course would be to fix the underlying bugs, since you
> > clearly have a test setup and the fixes should be trivial, more or
> > less
> >
> > if (wLength == right length) {
> > update req->complete
> > ret = right length
> > }
> >
> > and providing new completion handlers which copy the data.
>
> I knew it was not a complete fix.
It's not a "fix" at all. It swapped one bug for a more polite one.
> But I didn't have the time and
> knowledge to add support to this function.
See above. All those ep0out transfers have the same bug: in
the initial setup() call they must issue a read request to ep0,
and doing the memcpy when that completes. Instead, they are
wrongly assuming the read request was already issued.
> Moreover, this is an
> optional function for CDC class, so it is all right to leave it
> unsupported.
Which is why I asked you to either do a real fix, or at least change
both instances of this bug and provide a correct description of
what's being changed.
- Dave
-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
[email protected]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
