On Sun, Jul 06, 2014 at 11:47:50PM +0500, Abbas Raza wrote: > On 07/06/2014 11:01 PM, Greg KH wrote: > >On Sun, Jul 06, 2014 at 07:52:41PM +0200, Michael Grzeschik wrote: > >>Hi, > >> > >>On Sun, Jul 06, 2014 at 09:26:02PM +0500, Abbas Raza wrote: > >>>From: Abbas Raza <abbas_r...@mentor.com> > >>> > >>>There are 2 methods for ZLP (zero-length packet) generation: > >>>1) In software > >>>2) Automatic generation by device controller > >>> > >>>1) is implemented in UDC driver and it attaches ZLP to IN packet if > >>> descriptor->size < wLength > >>>2) can be enabled/disabled by setting ZLT bit in the QH > >>> > >>>When gadget ffs is connected to ubuntu host, the host sends > >>>get descriptor request and wLength in setup packet is 255 while the > >>>size of descriptor which will be sent by gadget in IN packet is > >>>64 byte. So the composite driver sets req->zero = 1. > >>>In UDC driver following code will be executed then > >>> > >>> if (hwreq->req.zero && hwreq->req.length > >>> && (hwreq->req.length % hwep->ep.maxpacket == 0)) > >>> add_td_to_list(hwep, hwreq, 0); > >>> > >>>Case-A: > >>>So in case of ubuntu host, UDC driver will attach a ZLP to the IN packet. > >>>ubuntu host will request 255 byte in IN request, gadget will send 64 byte > >>>with ZLP and host will come to know that there is no more data. > >>>But hold on, by default ZLT=0 for endpoint 0 so hardware also tries to > >>>automatically generate the ZLP which blocks enumeration for ~6 seconds due > >>>to endpoint 0 STALL, NAKs are sent to host for any requests (OUT/PING) > >>> > >>>Case-B: > >>>In case when gadget ffs is connected to Apple device, Apple device sends > >>>setup packet with wLength=64. So descriptor->size = 64 and wLength=64 > >>>therefore req->zero = 0 and UDC driver will not attach any ZLP to the > >>>IN packet. Apple device requests 64 bytes, gets 64 bytes and doesn't > >>>further request for IN data. But ZLT=0 by default for endpoint 0 so > >>>hardware tries to automatically generate the ZLP which blocks enumeration > >>>for ~6 seconds due to endpoint 0 STALL, NAKs are sent to host for any > >>>requests (OUT/PING) > >>> > >>>According to USB2.0 specs: > >>> > >>> 8.5.3.2 Variable-length Data Stage > >>> A control pipe may have a variable-length data phase in which the > >>> host requests more data than is contained in the specified data > >>> structure. When all of the data structure is returned to the host, > >>> the function should indicate that the Data stage is ended by > >>> returning a packet that is shorter than the MaxPacketSize for the > >>> pipe. If the data structure is an exact multiple of wMaxPacketSize > >>> for the pipe, the function will return a zero-length packet to > >>> indicate > >>> the end of the Data stage. > >>> > >>>In Case-A mentioned above: > >>>If we disable software ZLP generation & ZLT=0 for endpoint 0 OR if software > >>>ZLP generation is not disabled but we set ZLT=1 for endpoint 0 then > >>>enumeration doesn't block for 6 seconds. > >>> > >>>In Case-B mentioned above: > >>>If we disable software ZLP generation & ZLT=0 for endpoint then enumeration > >>>still blocks due to ZLP automatically generated by hardware and host not > >>>needing > >>>it. But if we keep software ZLP generation enabled but we set ZLT=1 for > >>>endpoint 0 then enumeration doesn't block for 6 seconds. > >>> > >>>So the proper solution for this issue seems to disable automatic ZLP > >>>generation > >>>by hardware (i.e by setting ZLT=1 for endpoint 0) and let software (UDC > >>>driver) > >>>handle the ZLP generation based on req->zero field. > >>> > >>>Signed-off-by: Abbas Raza <abbas_r...@mentor.com> > >>>--- > >>> drivers/usb/chipidea/udc.c | 4 ++-- > >>> 1 file changed, 2 insertions(+), 2 deletions(-) > >>> > >>>diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c > >>>index 7a12d13..2857dff 100644 > >>>--- a/drivers/usb/chipidea/udc.c > >>>+++ b/drivers/usb/chipidea/udc.c > >>>@@ -1169,8 +1169,8 @@ static int ep_enable(struct usb_ep *ep, > >>> if (hwep->type == USB_ENDPOINT_XFER_CONTROL) > >>> cap |= QH_IOS; > >>>- if (hwep->num) > >>>- cap |= QH_ZLT; > >>>+ > >>>+ cap |= QH_ZLT; > >>> cap |= (hwep->ep.maxpacket << __ffs(QH_MAX_PKT)) & QH_MAX_PKT; > >>> /* > >>> * For ISO-TX, we set mult at QH as the largest value, and use > >>>-- > >>>1.8.3.2 > >>How about an module parameter for this? > >If you want me to reject the patch, sure, do that :) > > > >Hint, never add module parameters for a driver, that is not acceptable. > > > >greg k-h > > Hi Michael, > > Could you please clarify why do you want to make it configurable? > Can the proposed change cause problems in some scenario?
The problem you are talking about is not the candidate for an module parameter. In fact I was thinking/refering to the hardcoded ZLT bit. Were setting it is not correct for every case. The information who is supposed to generate the zero package should come from the upper layers, or at least from the kernel paramter. The latter is probably not the nicest way. Regards, Michael -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
