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

[PATCH v3 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 quirk_ep_out_aligned_size 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  |  6 ++++++
 drivers/usb/gadget/f_fs.c  | 17 +++++++++++++++++
 include/linux/usb/gadget.h |  3 +++
 3 files changed, 26 insertions(+)

-- 
1.8.4.rc3

[PATCH v3 1/3] usb: gadget: add quirk_ep_out_aligned_size 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 'quirk_ep_out_aligned_size' field to struct usb_gadget
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 | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
index 942ef5e..9405d0f 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -540,6 +540,9 @@ struct usb_gadget {
 	struct device			dev;
 	unsigned			out_epnum;
 	unsigned			in_epnum;
+
+	/* epout requires buffer size to be aligned to MaxPacketSize */
+	unsigned			quirk_ep_out_aligned_size:1;
 };
 #define work_to_gadget(w)	(container_of((w), struct usb_gadget, work))
 
-- 
1.8.4.rc3

Re: [PATCH v3 1/3] usb: gadget: add quirk_ep_out_aligned_size field to struct usb_gadget

[Alan Stern]

On Wed, 30 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 'quirk_ep_out_aligned_size' field to struct usb_gadget
> 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 | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
> index 942ef5e..9405d0f 100644
> --- a/include/linux/usb/gadget.h
> +++ b/include/linux/usb/gadget.h
> @@ -540,6 +540,9 @@ struct usb_gadget {
>  	struct device			dev;
>  	unsigned			out_epnum;
>  	unsigned			in_epnum;
> +
> +	/* epout requires buffer size to be aligned to MaxPacketSize */
> +	unsigned			quirk_ep_out_aligned_size:1;
>  };
>  #define work_to_gadget(w)	(container_of((w), struct usb_gadget, work))

Instead of adding this new bitflag to the end of the structure, where 
it will cause the structure to grow by at least 4 bytes, why not add it 
in the middle, along with all the other bitflags, where it won't cause 
any change in the structure's size?

If you prefer, you can move the name, dev, out_epnum, and in_epnum 
fields higher up, so that the bitflags come last.

Alan Stern

Re: [PATCH v3 1/3] usb: gadget: add quirk_ep_out_aligned_size field to struct usb_gadget

[Felipe Balbi]

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

On Wed, Oct 30, 2013 at 01:31:50PM -0400, Alan Stern wrote:
> On Wed, 30 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 'quirk_ep_out_aligned_size' field to struct usb_gadget
> > 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 | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
> > index 942ef5e..9405d0f 100644
> > --- a/include/linux/usb/gadget.h
> > +++ b/include/linux/usb/gadget.h
> > @@ -540,6 +540,9 @@ struct usb_gadget {
> >  	struct device			dev;
> >  	unsigned			out_epnum;
> >  	unsigned			in_epnum;
> > +
> > +	/* epout requires buffer size to be aligned to MaxPacketSize */
> > +	unsigned			quirk_ep_out_aligned_size:1;
> >  };
> >  #define work_to_gadget(w)	(container_of((w), struct usb_gadget, work))
> 
> Instead of adding this new bitflag to the end of the structure, where 
> it will cause the structure to grow by at least 4 bytes, why not add it 
> in the middle, along with all the other bitflags, where it won't cause 
> any change in the structure's size?
> 
> If you prefer, you can move the name, dev, out_epnum, and in_epnum 
> fields higher up, so that the bitflags come last.

I'd prefer moving the fields and having all bit-fields, last. Thanks

-- 
balbi

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

Re: [PATCH v3 1/3] usb: gadget: add quirk_ep_out_aligned_size field to struct usb_gadget

[Felipe Balbi]

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

Hi,

On Wed, Oct 30, 2013 at 10:06:16AM -0700, 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 'quirk_ep_out_aligned_size' field to struct usb_gadget
> 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 | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
> index 942ef5e..9405d0f 100644
> --- a/include/linux/usb/gadget.h
> +++ b/include/linux/usb/gadget.h
> @@ -540,6 +540,9 @@ struct usb_gadget {
>  	struct device			dev;
>  	unsigned			out_epnum;
>  	unsigned			in_epnum;
> +
> +	/* epout requires buffer size to be aligned to MaxPacketSize */

please document this on the kernel-doc comment above.

-- 
balbi

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

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

[David Cohen]

Check gadget.quirk_ep_out_aligned_size to decide 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..b49dd55 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->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

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

[Alan Stern]

On Wed, 30 Oct 2013, David Cohen wrote:

> Check gadget.quirk_ep_out_aligned_size to decide 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..b49dd55 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->quirk_ep_out_aligned_size && read &&
> +		    !IS_ALIGNED(len, ep->ep->desc->wMaxPacketSize)) {

IS_ALIGNED works only when the second argument is a power of 2.  

Interrupt endpoints are not required to have maxpacket sizes that are 
powers of 2.  Does this code ever get used for an interrupt endpoint?

> +			size_t old_len = len;

Why add old_len here when you added orig_len above?

> +			len = roundup(orig_len,
> +				      (size_t)ep->ep->desc->wMaxPacketSize);
> +			if (unlikely(data) && len > old_len) {

If the original value wasn't aligned, how can len fail to be > old_len?

> +				kfree(data);
> +				data = NULL;
> +			}
> +		}
> +
>  		/* Allocate & copy */
>  		if (!halt && !data) {
>  			data = kzalloc(len, GFP_KERNEL);

Alan Stern

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

[David Cohen]

Hi Alan,

Appreciate your comments. Please, see my reply.

On 10/30/2013 10:35 AM, Alan Stern wrote:
> On Wed, 30 Oct 2013, David Cohen wrote:
>
>> Check gadget.quirk_ep_out_aligned_size to decide 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..b49dd55 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->quirk_ep_out_aligned_size && read &&
>> +		    !IS_ALIGNED(len, ep->ep->desc->wMaxPacketSize)) {
>
> IS_ALIGNED works only when the second argument is a power of 2.
>
> Interrupt endpoints are not required to have maxpacket sizes that are
> powers of 2.  Does this code ever get used for an interrupt endpoint?

That's a good point. It won't use interrupt ep on the case I am working
on, but f_fs allows it. I'll cover both cases in next patchset.

>
>> +			size_t old_len = len;
>
> Why add old_len here when you added orig_len above?

See below.

>
>> +			len = roundup(orig_len,
>> +				      (size_t)ep->ep->desc->wMaxPacketSize);
>> +			if (unlikely(data) && len > old_len) {
>
> If the original value wasn't aligned, how can len fail to be > old_len?

This code is inside a loop. There is an unlikely case explained in the
end to loop again if ep got disabled or changed while acquiring mutex.
If that happens, I want to avoid to re-allocate buffer if previous size
was already enough. Since 'len' gets updated when aligning buf size, we
need to keep track of 'orig_len' to do only minimum necessary pad on
new alignment. 'old_len' is used to save previous value to decide for
new allocation or not. But maybe I should change it to something more
accurate like 'buf_size' or 'alloc_size'.

Br, David Cohen

[PATCH v3 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 | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 5452c0f..528c7d7 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2600,6 +2600,12 @@ int dwc3_gadget_init(struct dwc3 *dwc)
 	dwc->gadget.name		= "dwc3-gadget";
 
 	/*
+	 * Per databook, DWC3 needs buffer size to be aligned to MaxPacketSize
+	 * on ep out.
+	 */
+	dwc->gadget.quirk_ep_out_aligned_size = 1;
+
+	/*
 	 * REVISIT: Here we should clear all pending IRQs to be
 	 * sure we're starting from a well known location.
 	 */
-- 
1.8.4.rc3

Re: [PATCH v3 3/3] usb: dwc3: add quirk USB_GADGET_QUIRK_EP_OUT_ALIGNED_SIZE to gadget driver

[Felipe Balbi]

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

On Wed, Oct 30, 2013 at 10:06:18AM -0700, David Cohen wrote:
> 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 | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 5452c0f..528c7d7 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -2600,6 +2600,12 @@ int dwc3_gadget_init(struct dwc3 *dwc)
>  	dwc->gadget.name		= "dwc3-gadget";
>  
>  	/*
> +	 * Per databook, DWC3 needs buffer size to be aligned to MaxPacketSize
> +	 * on ep out.
> +	 */
> +	dwc->gadget.quirk_ep_out_aligned_size = 1;

just to make it look cooler and neater, could you set to 'true' instead
:-)

-- 
balbi

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

Re: [PATCH v3 3/3] usb: dwc3: add quirk USB_GADGET_QUIRK_EP_OUT_ALIGNED_SIZE to gadget driver

[David Cohen]

On 10/30/2013 10:35 AM, Felipe Balbi wrote:
> On Wed, Oct 30, 2013 at 10:06:18AM -0700, David Cohen wrote:
>> 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 | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> index 5452c0f..528c7d7 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
>> @@ -2600,6 +2600,12 @@ int dwc3_gadget_init(struct dwc3 *dwc)
>>  	dwc->gadget.name		= "dwc3-gadget";
>>  
>>  	/*
>> +	 * Per databook, DWC3 needs buffer size to be aligned to MaxPacketSize
>> +	 * on ep out.
>> +	 */
>> +	dwc->gadget.quirk_ep_out_aligned_size = 1;
> 
> just to make it look cooler and neater, could you set to 'true' instead
> :-)

'bool' is an alien in C :)
But I can change to true in next patch set.

Br, David Cohen

Re: [PATCH v3 3/3] usb: dwc3: add quirk USB_GADGET_QUIRK_EP_OUT_ALIGNED_SIZE to gadget driver

[Felipe Balbi]

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

On Thu, Oct 31, 2013 at 12:58:51PM -0700, David Cohen wrote:
> On 10/30/2013 10:35 AM, Felipe Balbi wrote:
> > On Wed, Oct 30, 2013 at 10:06:18AM -0700, David Cohen wrote:
> >> 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 | 6 ++++++
> >>  1 file changed, 6 insertions(+)
> >>
> >> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> >> index 5452c0f..528c7d7 100644
> >> --- a/drivers/usb/dwc3/gadget.c
> >> +++ b/drivers/usb/dwc3/gadget.c
> >> @@ -2600,6 +2600,12 @@ int dwc3_gadget_init(struct dwc3 *dwc)
> >>  	dwc->gadget.name		= "dwc3-gadget";
> >>  
> >>  	/*
> >> +	 * Per databook, DWC3 needs buffer size to be aligned to MaxPacketSize
> >> +	 * on ep out.
> >> +	 */
> >> +	dwc->gadget.quirk_ep_out_aligned_size = 1;
> > 
> > just to make it look cooler and neater, could you set to 'true' instead
> > :-)
> 
> 'bool' is an alien in C :)
> But I can change to true in next patch set.

don't change the type, just assign true instead of 1. true is defined as
!0, so it'll be a 1 anywa. Still the rest of the driver uses true/false
for one-bit fields.

-- 
balbi

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