[RFC/PATCH 0/2] patchset to workaround f_fs for DWC3

[RFC/PATCH 0/2] patchset to workaround f_fs for DWC3

[David Cohen]

Hi,

These patches are a proposal to workaround f_fs when using DWC3 controller.
Since DWC3 requires epout buffer size to be aligned to maxpacketsize, f_fs
needs to pad buffer size to match the above case.

This change is necessary to make Android's adbd service to work with f_fs
instead of out-of-tree android gadget. If this same situation is happening in
other still untested places, a more generic solution may be required.

---
David Cohen (2):
  usb: gadget: re-introduce gadget_is_dwc3()
  usb: ffs/dwc3: pad epout buffer size when not aligned to maxpacketsize

 drivers/usb/gadget/f_fs.c         | 19 +++++++++++++++++++
 drivers/usb/gadget/gadget_chips.h |  1 +
 2 files changed, 20 insertions(+)

-- 
1.8.4.rc3

[RFC/PATCH 1/2] usb: gadget: re-introduce gadget_is_dwc3()

[David Cohen]

gadget_is_dwc3() is necessary to check whether we are running on
Desineware USB3 DRD controller.

This macro was previously removed by commit
ed9cbda63d45638b69ce62412e3a3c7b00644835 due to it wasn't needed
anymore. We're adding it again as things have changed.

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

diff --git a/drivers/usb/gadget/gadget_chips.h b/drivers/usb/gadget/gadget_chips.h
index bcd04bc..3186a5e 100644
--- a/drivers/usb/gadget/gadget_chips.h
+++ b/drivers/usb/gadget/gadget_chips.h
@@ -28,6 +28,7 @@
  * do that for you.
  */
 #define gadget_is_at91(g)		(!strcmp("at91_udc", (g)->name))
+#define gadget_is_dwc3(g)		(!strcmp("dwc3-gadget", (g)->name))
 #define gadget_is_goku(g)		(!strcmp("goku_udc", (g)->name))
 #define gadget_is_musbhdrc(g)		(!strcmp("musb-hdrc", (g)->name))
 #define gadget_is_net2280(g)		(!strcmp("net2280", (g)->name))
-- 
1.8.4.rc3

Re: [RFC/PATCH 1/2] usb: gadget: re-introduce gadget_is_dwc3()

[Felipe Balbi]

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

Hi,

On Mon, Oct 28, 2013 at 06:12:59PM -0700, David Cohen wrote:
> gadget_is_dwc3() is necessary to check whether we are running on
> Desineware USB3 DRD controller.
> 
> This macro was previously removed by commit
> ed9cbda63d45638b69ce62412e3a3c7b00644835 due to it wasn't needed
> anymore. We're adding it again as things have changed.
> 
> Signed-off-by: David Cohen <david.a.cohen@linux.intel.com>
> ---
>  drivers/usb/gadget/gadget_chips.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/usb/gadget/gadget_chips.h b/drivers/usb/gadget/gadget_chips.h
> index bcd04bc..3186a5e 100644
> --- a/drivers/usb/gadget/gadget_chips.h
> +++ b/drivers/usb/gadget/gadget_chips.h
> @@ -28,6 +28,7 @@
>   * do that for you.
>   */
>  #define gadget_is_at91(g)		(!strcmp("at91_udc", (g)->name))
> +#define gadget_is_dwc3(g)		(!strcmp("dwc3-gadget", (g)->name))

NAK, we want to get rid of all of them, in fact. gadget drivers
shouldn't have to know which controller they're running on, they need to
know about quirks and features the controller supports.

-- 
balbi

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

[RFC/PATCH 2/2] usb: ffs/dwc3: pad epout buffer size when not aligned to maxpacketsize

[David Cohen]

DWC3 requires buffer size to be aligned to maxpacketsize of an out
endpoint. ffs_epfile_io() needs to pad epout buffer to match above
condition if DWC3 controller is used.

This patch solves an specific situation but a more generic solution
should be found.

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

diff --git a/drivers/usb/gadget/f_fs.c b/drivers/usb/gadget/f_fs.c
index 75e4b78..33880e6 100644
--- a/drivers/usb/gadget/f_fs.c
+++ b/drivers/usb/gadget/f_fs.c
@@ -27,6 +27,7 @@
 #include <linux/usb/composite.h>
 #include <linux/usb/functionfs.h>
 
+#include "gadget_chips.h"
 
 #define FUNCTIONFS_MAGIC	0xa647361 /* Chosen by a honest dice roll ;) */
 
@@ -755,10 +756,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 +797,22 @@ first_try:
 			goto error;
 		}
 
+		/*
+		 * DWC3 requires buffer size to be aligned to maxpacketsize
+		 * of an out endpoint.
+		 * FIXME: a more generic solution might be necessary.
+		 */
+		if (gadget_is_dwc3(gadget) && 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: [RFC/PATCH 2/2] usb: ffs/dwc3: pad epout buffer size when not aligned to maxpacketsize

[Felipe Balbi]

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

Hi,

On Mon, Oct 28, 2013 at 06:13:00PM -0700, David Cohen wrote:
> DWC3 requires buffer size to be aligned to maxpacketsize of an out
> endpoint. ffs_epfile_io() needs to pad epout buffer to match above
> condition if DWC3 controller is used.
> 
> This patch solves an specific situation but a more generic solution
> should be found.
> 
> Signed-off-by: David Cohen <david.a.cohen@linux.intel.com>
> ---
>  drivers/usb/gadget/f_fs.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/drivers/usb/gadget/f_fs.c b/drivers/usb/gadget/f_fs.c
> index 75e4b78..33880e6 100644
> --- a/drivers/usb/gadget/f_fs.c
> +++ b/drivers/usb/gadget/f_fs.c
> @@ -27,6 +27,7 @@
>  #include <linux/usb/composite.h>
>  #include <linux/usb/functionfs.h>
>  
> +#include "gadget_chips.h"
>  
>  #define FUNCTIONFS_MAGIC	0xa647361 /* Chosen by a honest dice roll ;) */
>  
> @@ -755,10 +756,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 +797,22 @@ first_try:
>  			goto error;
>  		}
>  
> +		/*
> +		 * DWC3 requires buffer size to be aligned to maxpacketsize
> +		 * of an out endpoint.
> +		 * FIXME: a more generic solution might be necessary.
> +		 */

see, gadget drivers shouldn't have to know about DWC3 at all. They need
to know that current UDC has a quirk where EP OUT transactions need to
be aligned to wMaxPacketSize, so what I was expecting to see here was:

		if (test_bit(USB_GADGET_QUIRK_EP_OUT_ALIGNED_SIZE, &gadget->qirks) &&
			!IS_ALIGNED(len, ep->ep->desc->wMaxPacketSize))
			len = align_length(orig_len, wMaxPacketSize);


-- 
balbi

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

Re: [RFC/PATCH 2/2] usb: ffs/dwc3: pad epout buffer size when not aligned to maxpacketsize

[David Cohen]

>> +		/*
>> +		 * DWC3 requires buffer size to be aligned to maxpacketsize
>> +		 * of an out endpoint.
>> +		 * FIXME: a more generic solution might be necessary.
>> +		 */
> 
> see, gadget drivers shouldn't have to know about DWC3 at all. They need
> to know that current UDC has a quirk where EP OUT transactions need to
> be aligned to wMaxPacketSize, so what I was expecting to see here was:
> 
> 		if (test_bit(USB_GADGET_QUIRK_EP_OUT_ALIGNED_SIZE, &gadget->qirks) &&
> 			!IS_ALIGNED(len, ep->ep->desc->wMaxPacketSize))
> 			len = align_length(orig_len, wMaxPacketSize);

That makes sense. I'll send a new version.

Thanks,

David Cohen