Re: [PATCH] Fix gadget serial response on USB_CDC_REQ_SET_LINE_CODING

> -----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.

> 
> 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.  But I didn't have the time and
knowledge to add support to this function.  Moreover, this is an
optional function for CDC class, so it is all right to leave it
unsupported.

> 
> - Dave
> 
> 
> > Signed-off-by: Li Yang <[email protected]>
> > ---
> >
> > diff --git a/drivers/usb/gadget/serial.c
b/drivers/usb/gadget/serial.c
> > index e552668..0911bc4 100644
> > --- a/drivers/usb/gadget/serial.c
> > +++ b/drivers/usb/gadget/serial.c
> > @@ -1692,14 +1692,7 @@ static int gs_setup_class(struct usb_gadget
*gadget,
> >
> >  	switch (ctrl->bRequest) {
> >  	case USB_CDC_REQ_SET_LINE_CODING:
> > -		ret = min(wLength,
> > -			(u16)sizeof(struct usb_cdc_line_coding));
> > -		if (port) {
> > -			spin_lock(&port->port_lock);
> > -			memcpy(&port->port_line_coding, req->buf, ret);
> > -			spin_unlock(&port->port_lock);
> > -		}
> > -		ret = 0;
> > +		/* FIXME: We don't support set line coding */
> >  		break;
> >
> >  	case USB_CDC_REQ_GET_LINE_CODING:
> >
> >
> >
------------------------------------------------------------------------
-
> > 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=DEVDE
V
> > _______________________________________________
> > [email protected]
> > To unsubscribe, use the last form field at:
> > https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
> >

-------------------------------------------------------------------------
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