Re: [PATCH] Fix gadget serial response on USB_CDC_REQ_SET_LINE_CODING

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