[RFC/PATCH 02/45] usb: gadget: composite: correctly initialize ep->maxpacket

[RFC/PATCH 02/45] usb: gadget: composite: correctly initialize ep->maxpacket

[Felipe Balbi]

usb_endpoint_maxp() returns wMaxPacketSize in its
raw form. Without taking into consideration that it
also contains other bits reserved for isochronous
endpoints.

This patch fixes one occasion where this is a
problem by making sure that we initialize
ep->maxpacket only with lower 10 bits of the value
returned by usb_endpoint_maxp(). Note that seperate
patches will be necessary to audit all call sites of
usb_endpoint_maxp() and make sure that
usb_endpoint_maxp() only returns lower 10 bits of
wMaxPacketSize.

Cc: <stable@vger.kernel.org>
Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
---
 drivers/usb/gadget/composite.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
index 32176f779861..f6a7583ab6d1 100644
--- a/drivers/usb/gadget/composite.c
+++ b/drivers/usb/gadget/composite.c
@@ -197,7 +197,7 @@ int config_ep_by_speed(struct usb_gadget *g,
 
 ep_found:
 	/* commit results */
-	_ep->maxpacket = usb_endpoint_maxp(chosen_desc);
+	_ep->maxpacket = usb_endpoint_maxp(chosen_desc) & 0x7ff;
 	_ep->desc = chosen_desc;
 	_ep->comp_desc = NULL;
 	_ep->maxburst = 0;
-- 
2.10.0.440.g21f862b

[RFC/PATCH 03/45] usb: gadget: composite: always set ep->mult to a sensible value

[Felipe Balbi]

ep->mult is supposed to be set to Isochronous and
Interrupt Endapoint's multiplier value. This value
is computed from different places depending on the
link speed.

If we're dealing with HighSpeed, then it's part of
bits [12:11] of wMaxPacketSize. This case wasn't
taken into consideration before.

While at that, also make sure the ep->mult defaults
to one so drivers can use it unconditionally and
assume they'll never multiply ep->maxpacket to zero.

Cc: <stable@vger.kernel.org>
Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
---
 drivers/usb/gadget/composite.c          | 9 +++++++--
 drivers/usb/gadget/function/uvc_video.c | 2 +-
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
index f6a7583ab6d1..0426d3c1fff9 100644
--- a/drivers/usb/gadget/composite.c
+++ b/drivers/usb/gadget/composite.c
@@ -201,7 +201,12 @@ int config_ep_by_speed(struct usb_gadget *g,
 	_ep->desc = chosen_desc;
 	_ep->comp_desc = NULL;
 	_ep->maxburst = 0;
-	_ep->mult = 0;
+	_ep->mult = 1;
+
+	if (g->speed == USB_SPEED_HIGH && (usb_endpoint_xfer_isoc(_ep->desc) ||
+				usb_endpoint_xfer_int(_ep->desc)))
+		_ep->mult = usb_endpoint_maxp_mult(_ep->desc);
+
 	if (!want_comp_desc)
 		return 0;
 
@@ -218,7 +223,7 @@ int config_ep_by_speed(struct usb_gadget *g,
 		switch (usb_endpoint_type(_ep->desc)) {
 		case USB_ENDPOINT_XFER_ISOC:
 			/* mult: bits 1:0 of bmAttributes */
-			_ep->mult = comp_desc->bmAttributes & 0x3;
+			_ep->mult = (comp_desc->bmAttributes & 0x3) + 1;
 		case USB_ENDPOINT_XFER_BULK:
 		case USB_ENDPOINT_XFER_INT:
 			_ep->maxburst = comp_desc->bMaxBurst + 1;
diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
index 3d0d5d94a62f..0f01c04d7cbd 100644
--- a/drivers/usb/gadget/function/uvc_video.c
+++ b/drivers/usb/gadget/function/uvc_video.c
@@ -243,7 +243,7 @@ uvc_video_alloc_requests(struct uvc_video *video)
 
 	req_size = video->ep->maxpacket
 		 * max_t(unsigned int, video->ep->maxburst, 1)
-		 * (video->ep->mult + 1);
+		 * (video->ep->mult);
 
 	for (i = 0; i < UVC_NUM_REQUESTS; ++i) {
 		video->req_buffer[i] = kmalloc(req_size, GFP_KERNEL);
-- 
2.10.0.440.g21f862b

[RFC/PATCH 04/45] usb: dwc3: gadget: set PCM1 field of isochronous-first TRBs

[Felipe Balbi]

In case of High-Speed, High-Bandwidth endpoints, we
need to tell DWC3 that we have more than one packet
per interval. We do that by setting PCM1 field of
Isochronous-First TRB.

Cc: <stable@vger.kernel.org>
Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
---
 drivers/usb/dwc3/gadget.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 07cc8929f271..89145a8a92a8 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -771,6 +771,9 @@ static void dwc3_prepare_one_trb(struct dwc3_ep *dep,
 		unsigned length, unsigned chain, unsigned node)
 {
 	struct dwc3_trb		*trb;
+	struct dwc3		*dwc = dep->dwc;
+	struct usb_gadget	*gadget = &dwc->gadget;
+	enum usb_device_speed	speed = gadget->speed;
 
 	dwc3_trace(trace_dwc3_gadget, "%s: req %p dma %08llx length %d%s",
 			dep->name, req, (unsigned long long) dma,
@@ -797,10 +800,16 @@ static void dwc3_prepare_one_trb(struct dwc3_ep *dep,
 		break;
 
 	case USB_ENDPOINT_XFER_ISOC:
-		if (!node)
+		if (!node) {
 			trb->ctrl = DWC3_TRBCTL_ISOCHRONOUS_FIRST;
-		else
+
+			if (speed == USB_SPEED_HIGH) {
+				struct usb_ep *ep = &dep->endpoint;
+				trb->size |= DWC3_TRB_SIZE_PCM1(ep->mult - 1);
+			}
+		} else {
 			trb->ctrl = DWC3_TRBCTL_ISOCHRONOUS;
+		}
 
 		/* always enable Interrupt on Missed ISOC */
 		trb->ctrl |= DWC3_TRB_CTRL_ISP_IMI;
-- 
2.10.0.440.g21f862b

[RFC/PATCH 13/45] media: usbtv: core: make use of new usb_endpoint_maxp_mult()

[Felipe Balbi]

We have introduced a helper to calculate multiplier
value from wMaxPacketSize. Start using it.

Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
Cc: <linux-media@vger.kernel.org>
Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
---
 drivers/media/usb/usbtv/usbtv-core.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/media/usb/usbtv/usbtv-core.c b/drivers/media/usb/usbtv/usbtv-core.c
index dc76fd41e00f..ceb953be0770 100644
--- a/drivers/media/usb/usbtv/usbtv-core.c
+++ b/drivers/media/usb/usbtv/usbtv-core.c
@@ -71,6 +71,7 @@ static int usbtv_probe(struct usb_interface *intf,
 	int size;
 	struct device *dev = &intf->dev;
 	struct usbtv *usbtv;
+	struct usb_host_endpoint *ep;
 
 	/* Checks that the device is what we think it is. */
 	if (intf->num_altsetting != 2)
@@ -78,10 +79,12 @@ static int usbtv_probe(struct usb_interface *intf,
 	if (intf->altsetting[1].desc.bNumEndpoints != 4)
 		return -ENODEV;
 
+	ep = &intf->altsetting[1].endpoint[0];
+
 	/* Packet size is split into 11 bits of base size and count of
 	 * extra multiplies of it.*/
-	size = usb_endpoint_maxp(&intf->altsetting[1].endpoint[0].desc);
-	size = (size & 0x07ff) * (((size & 0x1800) >> 11) + 1);
+	size = usb_endpoint_maxp(&ep->desc);
+	size = (size & 0x07ff) * usb_endpoint_maxp_mult(&ep->desc);
 
 	/* Device structure */
 	usbtv = kzalloc(sizeof(struct usbtv), GFP_KERNEL);
-- 
2.10.0.440.g21f862b

[RFC/PATCH 14/45] media: usb: uvc: make use of new usb_endpoint_maxp_mult()

[Felipe Balbi]

We have introduced a helper to calculate multiplier
value from wMaxPacketSize. Start using it.

Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
Cc: <linux-media@vger.kernel.org>
Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
---
 drivers/media/usb/uvc/uvc_video.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
index b5589d5f5da4..11e0e5f4e1c2 100644
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c
@@ -1467,6 +1467,7 @@ static unsigned int uvc_endpoint_max_bpi(struct usb_device *dev,
 					 struct usb_host_endpoint *ep)
 {
 	u16 psize;
+	u16 mult;
 
 	switch (dev->speed) {
 	case USB_SPEED_SUPER:
@@ -1474,7 +1475,8 @@ static unsigned int uvc_endpoint_max_bpi(struct usb_device *dev,
 		return le16_to_cpu(ep->ss_ep_comp.wBytesPerInterval);
 	case USB_SPEED_HIGH:
 		psize = usb_endpoint_maxp(&ep->desc);
-		return (psize & 0x07ff) * (1 + ((psize >> 11) & 3));
+		mult = usb_endpoint_maxp_mult(&ep->desc);
+		return (psize & 0x07ff) * mult;
 	case USB_SPEED_WIRELESS:
 		psize = usb_endpoint_maxp(&ep->desc);
 		return psize;
-- 
2.10.0.440.g21f862b

[RFC/PATCH 20/45] usb: gadget: udc: fsl: make use of new usb_endpoint_maxp_mult()

[Felipe Balbi]

We have introduced a helper to calculate multiplier
value from wMaxPacketSize. Start using it.

Cc: Li Yang <leoli@freescale.com>
Cc: <linuxppc-dev@lists.ozlabs.org>
Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
---
 drivers/usb/gadget/udc/fsl_udc_core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/udc/fsl_udc_core.c b/drivers/usb/gadget/udc/fsl_udc_core.c
index aab5221d6c2e..4459644b9b55 100644
--- a/drivers/usb/gadget/udc/fsl_udc_core.c
+++ b/drivers/usb/gadget/udc/fsl_udc_core.c
@@ -585,7 +585,7 @@ static int fsl_ep_enable(struct usb_ep *_ep,
 		break;
 	case USB_ENDPOINT_XFER_ISOC:
 		/* Calculate transactions needed for high bandwidth iso */
-		mult = (unsigned char)(1 + ((max >> 11) & 0x03));
+		mult = usb_endpoint_maxp_mult(desc);
 		max = max & 0x7ff;	/* bit 0~10 */
 		/* 3 transactions at most */
 		if (mult > 3)
-- 
2.10.0.440.g21f862b

[RFC/PATCH 30/45] media: usb: uvc: remove unnecessary & operation

[Felipe Balbi]

Now that usb_endpoint_maxp() only returns the lowest
11 bits from wMaxPacketSize, we can remove the &
operation from this driver.

Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
Cc: <linux-media@vger.kernel.org>
Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
---
 drivers/media/usb/uvc/uvc_video.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
index 11e0e5f4e1c2..f3c1c852e401 100644
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c
@@ -1553,7 +1553,7 @@ static int uvc_init_video_bulk(struct uvc_streaming *stream,
 	u16 psize;
 	u32 size;
 
-	psize = usb_endpoint_maxp(&ep->desc) & 0x7ff;
+	psize = usb_endpoint_maxp(&ep->desc);
 	size = stream->ctrl.dwMaxPayloadTransferSize;
 	stream->bulk.max_payload_size = size;
 
-- 
2.10.0.440.g21f862b

[RFC/PATCH 43/45] usb: gadget: udc: fsl: remove unnecessary & operation

[Felipe Balbi]

Now that usb_endpoint_maxp() only returns the lowest
11 bits from wMaxPacketSize, we can remove the &
operation from this driver.

Cc: Li Yang <leoli@freescale.com>
Cc: <linuxppc-dev@lists.ozlabs.org>
Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
---
 drivers/usb/gadget/udc/fsl_udc_core.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/usb/gadget/udc/fsl_udc_core.c b/drivers/usb/gadget/udc/fsl_udc_core.c
index 4459644b9b55..71094e479a96 100644
--- a/drivers/usb/gadget/udc/fsl_udc_core.c
+++ b/drivers/usb/gadget/udc/fsl_udc_core.c
@@ -586,7 +586,6 @@ static int fsl_ep_enable(struct usb_ep *_ep,
 	case USB_ENDPOINT_XFER_ISOC:
 		/* Calculate transactions needed for high bandwidth iso */
 		mult = usb_endpoint_maxp_mult(desc);
-		max = max & 0x7ff;	/* bit 0~10 */
 		/* 3 transactions at most */
 		if (mult > 3)
 			goto en_done;
-- 
2.10.0.440.g21f862b

Re: [RFC/PATCH 14/45] media: usb: uvc: make use of new usb_endpoint_maxp_mult()

[Laurent Pinchart]

Hi Felipe,

Thanks for the patch.

On Wednesday 28 Sep 2016 16:05:23 Felipe Balbi wrote:
> We have introduced a helper to calculate multiplier
> value from wMaxPacketSize. Start using it.
> 
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
> Cc: <linux-media@vger.kernel.org>
> Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
> ---
>  drivers/media/usb/uvc/uvc_video.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_video.c
> b/drivers/media/usb/uvc/uvc_video.c index b5589d5f5da4..11e0e5f4e1c2 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -1467,6 +1467,7 @@ static unsigned int uvc_endpoint_max_bpi(struct
> usb_device *dev, struct usb_host_endpoint *ep)
>  {
>  	u16 psize;
> +	u16 mult;
> 
>  	switch (dev->speed) {
>  	case USB_SPEED_SUPER:
> @@ -1474,7 +1475,8 @@ static unsigned int uvc_endpoint_max_bpi(struct
> usb_device *dev, return le16_to_cpu(ep->ss_ep_comp.wBytesPerInterval);
>  	case USB_SPEED_HIGH:
>  		psize = usb_endpoint_maxp(&ep->desc);
> -		return (psize & 0x07ff) * (1 + ((psize >> 11) & 3));
> +		mult = usb_endpoint_maxp_mult(&ep->desc);
> +		return (psize & 0x07ff) * mult;

I believe you can remove the & 0x07ff here after patch 28/45.

This being said, wouldn't it be useful to introduce a helper function that 
performs the full computation instead of requiring usage of two helpers that 
both call __le16_to_cpu() on the same field ? Or possibly turn the whole 
uvc_endpoint_max_bpi() function into a core helper ? I haven't checked whether 
it can be useful to other drivers though.

>  	case USB_SPEED_WIRELESS:
>  		psize = usb_endpoint_maxp(&ep->desc);
>  		return psize;

-- 
Regards,

Laurent Pinchart

Re: [RFC/PATCH 30/45] media: usb: uvc: remove unnecessary & operation

[Laurent Pinchart]

Hi Felipe,

Thank you for the patch.

On Wednesday 28 Sep 2016 16:05:39 Felipe Balbi wrote:
> Now that usb_endpoint_maxp() only returns the lowest
> 11 bits from wMaxPacketSize, we can remove the &
> operation from this driver.
> 
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
> Cc: <linux-media@vger.kernel.org>
> Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/media/usb/uvc/uvc_video.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_video.c
> b/drivers/media/usb/uvc/uvc_video.c index 11e0e5f4e1c2..f3c1c852e401 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -1553,7 +1553,7 @@ static int uvc_init_video_bulk(struct uvc_streaming
> *stream, u16 psize;
>  	u32 size;
> 
> -	psize = usb_endpoint_maxp(&ep->desc) & 0x7ff;
> +	psize = usb_endpoint_maxp(&ep->desc);
>  	size = stream->ctrl.dwMaxPayloadTransferSize;
>  	stream->bulk.max_payload_size = size;

-- 
Regards,

Laurent Pinchart

Re: [RFC/PATCH 14/45] media: usb: uvc: make use of new usb_endpoint_maxp_mult()

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2272 bytes --]


Hi,

Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes:
> Hi Felipe,
>
> Thanks for the patch.
>
> On Wednesday 28 Sep 2016 16:05:23 Felipe Balbi wrote:
>> We have introduced a helper to calculate multiplier
>> value from wMaxPacketSize. Start using it.
>> 
>> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
>> Cc: <linux-media@vger.kernel.org>
>> Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
>> ---
>>  drivers/media/usb/uvc/uvc_video.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/media/usb/uvc/uvc_video.c
>> b/drivers/media/usb/uvc/uvc_video.c index b5589d5f5da4..11e0e5f4e1c2 100644
>> --- a/drivers/media/usb/uvc/uvc_video.c
>> +++ b/drivers/media/usb/uvc/uvc_video.c
>> @@ -1467,6 +1467,7 @@ static unsigned int uvc_endpoint_max_bpi(struct
>> usb_device *dev, struct usb_host_endpoint *ep)
>>  {
>>  	u16 psize;
>> +	u16 mult;
>> 
>>  	switch (dev->speed) {
>>  	case USB_SPEED_SUPER:
>> @@ -1474,7 +1475,8 @@ static unsigned int uvc_endpoint_max_bpi(struct
>> usb_device *dev, return le16_to_cpu(ep->ss_ep_comp.wBytesPerInterval);
>>  	case USB_SPEED_HIGH:
>>  		psize = usb_endpoint_maxp(&ep->desc);
>> -		return (psize & 0x07ff) * (1 + ((psize >> 11) & 3));
>> +		mult = usb_endpoint_maxp_mult(&ep->desc);
>> +		return (psize & 0x07ff) * mult;
>
> I believe you can remove the & 0x07ff here after patch 28/45.
>
> This being said, wouldn't it be useful to introduce a helper function that 
> performs the full computation instead of requiring usage of two helpers that 
> both call __le16_to_cpu() on the same field ? Or possibly turn the whole 
> uvc_endpoint_max_bpi() function into a core helper ? I haven't checked whether 
> it can be useful to other drivers though.

it's probably not useful for many other drivers. The multiplier is only
valid for High-speed Isochronous and Interrupt endpoints. If it's not
High speed, we don't care. If it's not Isoc or Int, we don't care.

If we have a single helper, we are likely gonna be doing extra
computation for no reason. Moreover, this is only called once during
probe(), there's really nothing to optimize here.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 800 bytes --]