[RFC/PATCH v2 0/3] add gadget quirk to adapt f_fs for DWC3

[RFC/PATCH v2 0/3] add gadget quirk to adapt f_fs for DWC3

[David Cohen]

Hi,

These patches are a proposal to add gadget quirks in an immediate objective to
adapt f_fs when using DWC3 controller. But the quirk solution is generic and
can be used by other controllers to adapt gadget functions to their
non-standard restrictions.

This change is necessary to make Android's adbd service to work on Intel
Merrifield with f_fs instead of out-of-tree android gadget.

---
David Cohen (3):
  usb: gadget: add quirks field to struct usb_gadget
  usb: ffs: check quirk to pad epout buf size when not aligned to
    maxpacketsize
  usb: dwc3: add quirk USB_GADGET_QUIRK_EP_OUT_ALIGNED_SIZE to gadget
    driver

 drivers/usb/dwc3/gadget.c  |  1 +
 drivers/usb/gadget/f_fs.c  | 17 +++++++++++++++++
 include/linux/usb/gadget.h |  5 +++++
 3 files changed, 23 insertions(+)

-- 
1.8.4.rc3

[RFC/PATCH v2 1/3] usb: gadget: add quirks field to struct usb_gadget

[David Cohen]

Due to USB controllers may have different restrictions, usb gadget layer
needs to provide a generic way to inform gadget functions to complain
with non-standard requirements.

This patch adds 'quirks' field to struct usb_gadget and the first quirk
called USB_GADGET_QUIRK_EP_OUT_ALIGNED_SIZE necessary to inform when
controller's epout requires buffer size to be aligned to MaxPacketSize.

Signed-off-by: David Cohen <david.a.cohen@linux.intel.com>
---
 include/linux/usb/gadget.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
index 942ef5e..7014ad9 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -540,6 +540,11 @@ struct usb_gadget {
 	struct device			dev;
 	unsigned			out_epnum;
 	unsigned			in_epnum;
+
+	u32				quirks;
+/* epout requires buffer size to be aligned to MaxPacketSize */
+#define USB_GADGET_QUIRK_EP_OUT_ALIGNED_SIZE		(1 << 0)
+
 };
 #define work_to_gadget(w)	(container_of((w), struct usb_gadget, work))
 
-- 
1.8.4.rc3

Re: [RFC/PATCH v2 1/3] usb: gadget: add quirks field to struct usb_gadget

[Alan Stern]

On Tue, 29 Oct 2013, David Cohen wrote:

> Due to USB controllers may have different restrictions, usb gadget layer
> needs to provide a generic way to inform gadget functions to complain
> with non-standard requirements.
> 
> This patch adds 'quirks' field to struct usb_gadget and the first quirk
> called USB_GADGET_QUIRK_EP_OUT_ALIGNED_SIZE necessary to inform when
> controller's epout requires buffer size to be aligned to MaxPacketSize.
> 
> Signed-off-by: David Cohen <david.a.cohen@linux.intel.com>
> ---
>  include/linux/usb/gadget.h | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
> index 942ef5e..7014ad9 100644
> --- a/include/linux/usb/gadget.h
> +++ b/include/linux/usb/gadget.h
> @@ -540,6 +540,11 @@ struct usb_gadget {
>  	struct device			dev;
>  	unsigned			out_epnum;
>  	unsigned			in_epnum;
> +
> +	u32				quirks;
> +/* epout requires buffer size to be aligned to MaxPacketSize */
> +#define USB_GADGET_QUIRK_EP_OUT_ALIGNED_SIZE		(1 << 0)

If you decide to go through with this, it might be better to define a 
series of single-bit flags instead of a single "quirks" field.  For 
example:

	unsigned			quirk_ep_out_aligned_size:1;

Yes, other people (including me!) have done it your way in the past,
but now this seems to make more sense.

Alan Stern

Re: [RFC/PATCH v2 1/3] usb: gadget: add quirks field to struct usb_gadget

[David Cohen]

>> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
>> index 942ef5e..7014ad9 100644
>> --- a/include/linux/usb/gadget.h
>> +++ b/include/linux/usb/gadget.h
>> @@ -540,6 +540,11 @@ struct usb_gadget {
>>   	struct device			dev;
>>   	unsigned			out_epnum;
>>   	unsigned			in_epnum;
>> +
>> +	u32				quirks;
>> +/* epout requires buffer size to be aligned to MaxPacketSize */
>> +#define USB_GADGET_QUIRK_EP_OUT_ALIGNED_SIZE		(1 << 0)
>
> If you decide to go through with this, it might be better to define a
> series of single-bit flags instead of a single "quirks" field.  For
> example:
>
> 	unsigned			quirk_ep_out_aligned_size:1;
>
> Yes, other people (including me!) have done it your way in the past,
> but now this seems to make more sense.

It would be less error-prone. I'll change it.

Thanks,

David

[RFC/PATCH v2 2/3] usb: ffs: check quirk to pad epout buf size when not aligned to maxpacketsize

[David Cohen]

Use USB_GADGET_QUIRK_EP_OUT_ALIGNED_SIZE quirk to check if buffer size
requires to be aligned to maxpacketsize of an out endpoint.
ffs_epfile_io() needs to pad epout buffer to match above condition if
quirk is found.

Signed-off-by: David Cohen <david.a.cohen@linux.intel.com>
---
 drivers/usb/gadget/f_fs.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/usb/gadget/f_fs.c b/drivers/usb/gadget/f_fs.c
index 75e4b78..2988eb7 100644
--- a/drivers/usb/gadget/f_fs.c
+++ b/drivers/usb/gadget/f_fs.c
@@ -755,10 +755,12 @@ static ssize_t ffs_epfile_io(struct file *file,
 			     char __user *buf, size_t len, int read)
 {
 	struct ffs_epfile *epfile = file->private_data;
+	struct usb_gadget *gadget = epfile->ffs->gadget;
 	struct ffs_ep *ep;
 	char *data = NULL;
 	ssize_t ret;
 	int halt;
+	size_t orig_len = len;
 
 	goto first_try;
 	do {
@@ -794,6 +796,21 @@ first_try:
 			goto error;
 		}
 
+		/*
+		 * Controller requires buffer size to be aligned to
+		 * maxpacketsize of an out endpoint.
+		 */
+		if (gadget->quirks & USB_GADGET_QUIRK_EP_OUT_ALIGNED_SIZE &&
+		    read && !IS_ALIGNED(len, ep->ep->desc->wMaxPacketSize)) {
+			size_t old_len = len;
+			len = roundup(orig_len,
+				      (size_t)ep->ep->desc->wMaxPacketSize);
+			if (unlikely(data) && len > old_len) {
+				kfree(data);
+				data = NULL;
+			}
+		}
+
 		/* Allocate & copy */
 		if (!halt && !data) {
 			data = kzalloc(len, GFP_KERNEL);
-- 
1.8.4.rc3

[RFC/PATCH v2 3/3] usb: dwc3: add quirk USB_GADGET_QUIRK_EP_OUT_ALIGNED_SIZE to gadget driver

[David Cohen]

DWC3 requires epout to have buffer size aligned to MaxPacketSize value.
This patch adds necessary quirk for it.

Signed-off-by: David Cohen <david.a.cohen@linux.intel.com>
---
 drivers/usb/dwc3/gadget.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 5452c0f..0dcc89f 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2598,6 +2598,7 @@ int dwc3_gadget_init(struct dwc3 *dwc)
 	dwc->gadget.speed		= USB_SPEED_UNKNOWN;
 	dwc->gadget.sg_supported	= true;
 	dwc->gadget.name		= "dwc3-gadget";
+	dwc->gadget.quirks		= USB_GADGET_QUIRK_EP_OUT_ALIGNED_SIZE;
 
 	/*
 	 * REVISIT: Here we should clear all pending IRQs to be
-- 
1.8.4.rc3

RE: [RFC/PATCH v2 0/3] add gadget quirk to adapt f_fs for DWC3

[Paul Zimmerman]

> From: David Cohen
> Sent: Tuesday, October 29, 2013 2:53 PM
> 
> These patches are a proposal to add gadget quirks in an immediate objective to
> adapt f_fs when using DWC3 controller. But the quirk solution is generic and
> can be used by other controllers to adapt gadget functions to their
> non-standard restrictions.
> 
> This change is necessary to make Android's adbd service to work on Intel
> Merrifield with f_fs instead of out-of-tree android gadget.
> 
> ---
> David Cohen (3):
>   usb: gadget: add quirks field to struct usb_gadget
>   usb: ffs: check quirk to pad epout buf size when not aligned to
>     maxpacketsize
>   usb: dwc3: add quirk USB_GADGET_QUIRK_EP_OUT_ALIGNED_SIZE to gadget
>     driver
> 
>  drivers/usb/dwc3/gadget.c  |  1 +
>  drivers/usb/gadget/f_fs.c  | 17 +++++++++++++++++
>  include/linux/usb/gadget.h |  5 +++++
>  3 files changed, 23 insertions(+)

Wouldn't it be simpler and safer to just do this unconditionally? Sure,
you need it for DWC3 because the controller refuses to do an OUT transfer
at all if the transfer size is less than maxpacketsize. But it's possible
that other controllers allow the transfer, and it works in most cases,
but if an error occurs and the host sends too much data, they could
overrun the buffer and crash your device.

For example, the DWC2 databook says "For OUT transfers, the Transfer
Size field in the endpoint's Transfer Size register must be a multiple
of the maximum packet size of the endpoint". But I don't think the
controller enforces that, it is up to the programmer to do the right
thing. So that controller probably needs this quirk also. There could be
more like that which we don't know about.

So unless the buffer allocation code is in a real fast path, I would
suggest to just do the aligned buffer allocation always.

-- 
Paul

RE: [RFC/PATCH v2 0/3] add gadget quirk to adapt f_fs for DWC3

[David Laight]

> Wouldn't it be simpler and safer to just do this unconditionally? Sure,
> you need it for DWC3 because the controller refuses to do an OUT transfer
> at all if the transfer size is less than maxpacketsize. But it's possible
> that other controllers allow the transfer, and it works in most cases,
> but if an error occurs and the host sends too much data, they could
> overrun the buffer and crash your device.
> 
> For example, the DWC2 databook says "For OUT transfers, the Transfer
> Size field in the endpoint's Transfer Size register must be a multiple
> of the maximum packet size of the endpoint". But I don't think the
> controller enforces that, it is up to the programmer to do the right
> thing. So that controller probably needs this quirk also. There could be
> more like that which we don't know about.
> 
> So unless the buffer allocation code is in a real fast path, I would
> suggest to just do the aligned buffer allocation always.

You wouldn't normally want to pad OUT transfers that way - if only
because of the additional USB bandwidth use.

Also, if the controller can't do (I assume bulk) OUT (and IN?)
transfers for less than maxpacketsize it seriously restricts
the type of devices that can be attached - none of the USB
ethernet devices would work.

	David

RE: [RFC/PATCH v2 0/3] add gadget quirk to adapt f_fs for DWC3

[Alan Stern]

On Wed, 30 Oct 2013, David Laight wrote:

> > Wouldn't it be simpler and safer to just do this unconditionally? Sure,
> > you need it for DWC3 because the controller refuses to do an OUT transfer
> > at all if the transfer size is less than maxpacketsize. But it's possible
> > that other controllers allow the transfer, and it works in most cases,
> > but if an error occurs and the host sends too much data, they could
> > overrun the buffer and crash your device.
> > 
> > For example, the DWC2 databook says "For OUT transfers, the Transfer
> > Size field in the endpoint's Transfer Size register must be a multiple
> > of the maximum packet size of the endpoint". But I don't think the
> > controller enforces that, it is up to the programmer to do the right
> > thing. So that controller probably needs this quirk also. There could be
> > more like that which we don't know about.
> > 
> > So unless the buffer allocation code is in a real fast path, I would
> > suggest to just do the aligned buffer allocation always.
> 
> You wouldn't normally want to pad OUT transfers that way - if only
> because of the additional USB bandwidth use.

What additional bandwidth use?  Allocating more memory doesn't mean any
additional data will be transmitted over the USB bus.

> Also, if the controller can't do (I assume bulk) OUT (and IN?)
> transfers for less than maxpacketsize it seriously restricts
> the type of devices that can be attached - none of the USB
> ethernet devices would work.

The controllers _are_ capable of doing shorter transfers.  You are
missing the point: These are device controllers, not host controllers,
so they don't have any choice about the length of an OUT transfer.

Alan Stern

Re: [RFC/PATCH v2 0/3] add gadget quirk to adapt f_fs for DWC3

[David Cohen]

On 10/29/2013 03:47 PM, Paul Zimmerman wrote:
>> From: David Cohen
>> Sent: Tuesday, October 29, 2013 2:53 PM
>>
>> These patches are a proposal to add gadget quirks in an immediate objective to
>> adapt f_fs when using DWC3 controller. But the quirk solution is generic and
>> can be used by other controllers to adapt gadget functions to their
>> non-standard restrictions.
>>
>> This change is necessary to make Android's adbd service to work on Intel
>> Merrifield with f_fs instead of out-of-tree android gadget.
>>
>> ---
>> David Cohen (3):
>>    usb: gadget: add quirks field to struct usb_gadget
>>    usb: ffs: check quirk to pad epout buf size when not aligned to
>>      maxpacketsize
>>    usb: dwc3: add quirk USB_GADGET_QUIRK_EP_OUT_ALIGNED_SIZE to gadget
>>      driver
>>
>>   drivers/usb/dwc3/gadget.c  |  1 +
>>   drivers/usb/gadget/f_fs.c  | 17 +++++++++++++++++
>>   include/linux/usb/gadget.h |  5 +++++
>>   3 files changed, 23 insertions(+)
>
> Wouldn't it be simpler and safer to just do this unconditionally? Sure,
> you need it for DWC3 because the controller refuses to do an OUT transfer
> at all if the transfer size is less than maxpacketsize. But it's possible
> that other controllers allow the transfer, and it works in most cases,
> but if an error occurs and the host sends too much data, they could
> overrun the buffer and crash your device.
>
> For example, the DWC2 databook says "For OUT transfers, the Transfer
> Size field in the endpoint's Transfer Size register must be a multiple
> of the maximum packet size of the endpoint". But I don't think the
> controller enforces that, it is up to the programmer to do the right
> thing. So that controller probably needs this quirk also. There could be
> more like that which we don't know about.

Unfortunately DWC2 is a bad example... the driver couldn't even get out
of staging. If the author was reckless to ignore this restriction (s)he
should fix.
But I don't have enough data to tell it's better to waste everybody's
memory in this case in favor of DWC3. I'd still stick with the
exception.

>
> So unless the buffer allocation code is in a real fast path, I would
> suggest to just do the aligned buffer allocation always.

This code would affect embedded devices which value too much memory
consumption (and performance on handling it!). IMO we'd need to be more
careful prior to take such decision.

Br, David Cohen

Re: [RFC/PATCH v2 0/3] add gadget quirk to adapt f_fs for DWC3

[Felipe Balbi]

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

Hi,

On Wed, Oct 30, 2013 at 09:36:20AM -0700, David Cohen wrote:
> On 10/29/2013 03:47 PM, Paul Zimmerman wrote:
> >>From: David Cohen
> >>Sent: Tuesday, October 29, 2013 2:53 PM
> >>
> >>These patches are a proposal to add gadget quirks in an immediate objective to
> >>adapt f_fs when using DWC3 controller. But the quirk solution is generic and
> >>can be used by other controllers to adapt gadget functions to their
> >>non-standard restrictions.
> >>
> >>This change is necessary to make Android's adbd service to work on Intel
> >>Merrifield with f_fs instead of out-of-tree android gadget.
> >>
> >>---
> >>David Cohen (3):
> >>   usb: gadget: add quirks field to struct usb_gadget
> >>   usb: ffs: check quirk to pad epout buf size when not aligned to
> >>     maxpacketsize
> >>   usb: dwc3: add quirk USB_GADGET_QUIRK_EP_OUT_ALIGNED_SIZE to gadget
> >>     driver
> >>
> >>  drivers/usb/dwc3/gadget.c  |  1 +
> >>  drivers/usb/gadget/f_fs.c  | 17 +++++++++++++++++
> >>  include/linux/usb/gadget.h |  5 +++++
> >>  3 files changed, 23 insertions(+)
> >
> >Wouldn't it be simpler and safer to just do this unconditionally? Sure,
> >you need it for DWC3 because the controller refuses to do an OUT transfer
> >at all if the transfer size is less than maxpacketsize. But it's possible
> >that other controllers allow the transfer, and it works in most cases,
> >but if an error occurs and the host sends too much data, they could
> >overrun the buffer and crash your device.
> >
> >For example, the DWC2 databook says "For OUT transfers, the Transfer
> >Size field in the endpoint's Transfer Size register must be a multiple
> >of the maximum packet size of the endpoint". But I don't think the
> >controller enforces that, it is up to the programmer to do the right
> >thing. So that controller probably needs this quirk also. There could be
> >more like that which we don't know about.
> 
> Unfortunately DWC2 is a bad example... the driver couldn't even get out
> of staging. If the author was reckless to ignore this restriction (s)he
> should fix.
> But I don't have enough data to tell it's better to waste everybody's
> memory in this case in favor of DWC3. I'd still stick with the
> exception.

on top of that we don't what quirks other controllers might have. There
could very well be a controller which quirk goes the other around: you
_must_ set bulk out length to the correct size (e.g. 31 bytes for mass
storage's CBW) otherwise transfer won't complete.

I could see that happening on controllers with broken short packet
handling.

> >So unless the buffer allocation code is in a real fast path, I would
> >suggest to just do the aligned buffer allocation always.
> 
> This code would affect embedded devices which value too much memory
> consumption (and performance on handling it!). IMO we'd need to be more
> careful prior to take such decision.

agreed. Quirk flag is the way to go here.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]