Hi, Sergei Shtylyov wrote: > Hello. > > On 22-11-2010 13:02, Holger brunck wrote: > >>>> From: Stefan Bigler<stefan.bigler@xxxxxxxxxxx> > >>>> Some usb serial host drivers expect a short packet before they forward >>>> the data to the application. This is caused by them trying to read more >>>> than one packet at a time. So when the gadget sends an exact multiple >>>> of the maximum packet size, it should append a zero-length packet. >>>> The gadget driver for fsl_qe was not handling this case correctly. >>>> This patch adds sending of a null packet if it is the last packet of >>>> a frame and the request length is USB_EP0_MAX_SIZE (64 Byte). > >>>> See also commit 2e25134122c25ebb0679b4bbd536fb46c669f9d7 > >>> Linus has asked to also specify the commit summary in parens. > >> Ok I will do this if the patch is with regards to its content ok. > > It's does not seem OK currently... > >>>> Signed-off-by: Stefan Bigler<stefan.bigler@xxxxxxxxxxx> >>>> Signed-off-by: Holger Brunck<holger.brunck@xxxxxxxxxxx> >>>> Cc: Li Yang<leoli@xxxxxxxxxxxxx> >>>> Cc: David Brownell<dbrownell@xxxxxxxxxxxxxxxxxxxxx> > >>>> --- >>>> drivers/usb/gadget/fsl_qe_udc.c | 3 +++ >>>> 1 files changed, 3 insertions(+), 0 deletions(-) > >>>> diff --git a/drivers/usb/gadget/fsl_qe_udc.c >>>> b/drivers/usb/gadget/fsl_qe_udc.c >>>> index 7881f12..279454a 100644 >>>> --- a/drivers/usb/gadget/fsl_qe_udc.c >>>> +++ b/drivers/usb/gadget/fsl_qe_udc.c >>>> @@ -1229,6 +1229,9 @@ static int frame_create_tx(struct qe_ep *ep, >>>> struct qe_frame *frame) >>>> else >>>> reval = sendnulldata(ep, frame, 0); > >>> No, really you shouldn't send null data in every case when >>> req->req.length - ep->sent<= 0, only if req->req.length == 0 or >>> req->req.zero == 1... > >> Hm, yes this piece of code looks a bit odd to me too. And I did not >> understand >> what it should solve. But I let it untouched, because the bug my patch >> should >> fix is in my eyes a different topic. > > No, this is actually the same topic. > Ok. >>>> + if (req->req.zero&& (req->req.length == USB_EP0_MAX_SIZE)) > >>> Not (req->req.length % USB_EP0_MAX_SIZE == 0)? Also, you should >>> probably check that ep->sent == req->req.length here... > >> No not (req->req.length % USB_EP0_MAX_SIZE == 0), because the nulldata >> which is >> missing should only be send if it is the last packet of a frame and >> not every > > But your code will only send nulldata is the transfer is *exactly* 64 > bytes. > req->req.length shouldn't change. > this is not what I see when I test the patch. My patch sends nulldata if the transfer size is n*64 Bytes and if it's the last packet from the transfer. I checked it with transfers of 64, 192 and 79 Bytes and my code is called once if the last 64 Byte packet is sent and this is what this patch is trying to solve. Here is the trace output in this function with some printk: sh-3.2# ./main ./main: send 64 Bytes req->req.length = 64 req->req.zero = 1 ep->sent = 0 add sendnulldata <--- last packet my patch send nulldata ./main: send 192 Bytes req->req.length = 64 req->req.zero = 0 ep->sent = 0 req->req.length = 64 req->req.zero = 0 ep->sent = 0 req->req.length = 64 req->req.zero = 1 ep->sent = 0 add sendnulldata <--- last packet my patch send nulldata ./main: send 79 Bytes req->req.length = 64 req->req.zero = 0 ep->sent = 0 req->req.length = 15 req->req.zero = 1 ep->sent = 0 <--- last packet but no nulldata send, because packet is not on 64 Byte border >> time a 64byte packet is send. > > You've misunderstood what I proposed -- there's no way it can send > nulldata for each 64 packet. That's why we should check that (ep->sent > == req->req.length). > Hm in the trace outpu above ep->sent is never equal to req->req.length, so the patch would not work. Best regards Holger -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
