[PATCH v6 0/5] add gadget quirk to adapt f_fs for DWC3

[PATCH v6 0/5] 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.

This new patch set was tested and validated in my environment:
 - Intel Merrifield was able to use DWC3/f_fs with adbd service (it wasn't
   before).

Changes from v45 to v6:
 - Updated patches from Michal Nazarewicz to address comments from Alan Stern
   and mine.
 - Modified patch 5/5 to apply request->length's pad internally to DWC3.

---
David Cohen (3):
  usb: gadget: move bitflags to the end of usb_gadget struct
  usb: gadget: add quirk_ep_out_aligned_size field to struct usb_gadget
  usb: dwc3: implement gadget's quirk ep_out_align_size

Michal Nazarewicz (2):
  usb: gadget: f_fs: remove loop from I/O function
  check quirk to pad epout buf size when not aligned to maxpacketsize

 drivers/usb/dwc3/core.h    |   6 +++
 drivers/usb/dwc3/gadget.c  |  23 ++++++++++
 drivers/usb/gadget/f_fs.c  | 102 +++++++++++++++++++++------------------------
 include/linux/usb/gadget.h |  35 ++++++++++++----
 4 files changed, 103 insertions(+), 63 deletions(-)

-- 
1.8.4.2

[PATCH v6 1/5] usb: gadget: move bitflags to the end of usb_gadget struct

[David Cohen]

This patch moves all bitflags to the end of usb_gadget struct in order
to improve readability.

Signed-off-by: David Cohen <david.a.cohen@linux.intel.com>
Acked-by: Michal Nazarewicz <mina86@mina86.com>
---
 include/linux/usb/gadget.h | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
index 942ef5e053bf..23b3bfd0a842 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -485,6 +485,11 @@ struct usb_gadget_ops {
  * @max_speed: Maximal speed the UDC can handle.  UDC must support this
  *      and all slower speeds.
  * @state: the state we are now (attached, suspended, configured, etc)
+ * @name: Identifies the controller hardware type.  Used in diagnostics
+ *	and sometimes configuration.
+ * @dev: Driver model state for this abstract device.
+ * @out_epnum: last used out ep number
+ * @in_epnum: last used in ep number
  * @sg_supported: true if we can handle scatter-gather
  * @is_otg: True if the USB device port uses a Mini-AB jack, so that the
  *	gadget driver must provide a USB OTG descriptor.
@@ -497,11 +502,6 @@ struct usb_gadget_ops {
  *	only supports HNP on a different root port.
  * @b_hnp_enable: OTG device feature flag, indicating that the A-Host
  *	enabled HNP support.
- * @name: Identifies the controller hardware type.  Used in diagnostics
- *	and sometimes configuration.
- * @dev: Driver model state for this abstract device.
- * @out_epnum: last used out ep number
- * @in_epnum: last used in ep number
  *
  * Gadgets have a mostly-portable "gadget driver" implementing device
  * functions, handling all usb configurations and interfaces.  Gadget
@@ -530,16 +530,17 @@ struct usb_gadget {
 	enum usb_device_speed		speed;
 	enum usb_device_speed		max_speed;
 	enum usb_device_state		state;
+	const char			*name;
+	struct device			dev;
+	unsigned			out_epnum;
+	unsigned			in_epnum;
+
 	unsigned			sg_supported:1;
 	unsigned			is_otg:1;
 	unsigned			is_a_peripheral:1;
 	unsigned			b_hnp_enable:1;
 	unsigned			a_hnp_support:1;
 	unsigned			a_alt_hnp_support:1;
-	const char			*name;
-	struct device			dev;
-	unsigned			out_epnum;
-	unsigned			in_epnum;
 };
 #define work_to_gadget(w)	(container_of((w), struct usb_gadget, work))
 
-- 
1.8.4.2

[PATCH v6 2/5] 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. A helper is also provided to align buffer size when
necessary.

Signed-off-by: David Cohen <david.a.cohen@linux.intel.com>
Acked-by: Alan Stern <stern@rowland.harvard.edu>
Acked-by: Michal Nazarewicz <mina86@mina86.com>
---
 include/linux/usb/gadget.h | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
index 23b3bfd0a842..41e8834af57d 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -441,6 +441,19 @@ static inline void usb_ep_fifo_flush(struct usb_ep *ep)
 		ep->ops->fifo_flush(ep);
 }
 
+/**
+ * usb_ep_align_maxpacketsize - returns @len aligned to ep's maxpacketsize
+ * @ep: the endpoint whose maxpacketsize is used to align @len
+ * @len: buffer size's length to align to @ep's maxpacketsize
+ *
+ * This helper is used in case it's required for any reason to align buffer's
+ * size to an ep's maxpacketsize.
+ */
+static inline size_t usb_ep_align_maxpacketsize(struct usb_ep *ep, size_t len)
+{
+	return round_up(len, (size_t)ep->desc->wMaxPacketSize);
+}
+
 
 /*-------------------------------------------------------------------------*/
 
@@ -502,6 +515,8 @@ struct usb_gadget_ops {
  *	only supports HNP on a different root port.
  * @b_hnp_enable: OTG device feature flag, indicating that the A-Host
  *	enabled HNP support.
+ * @quirk_ep_out_aligned_size: epout requires buffer size to be aligned to
+ *	MaxPacketSize.
  *
  * Gadgets have a mostly-portable "gadget driver" implementing device
  * functions, handling all usb configurations and interfaces.  Gadget
@@ -541,6 +556,7 @@ struct usb_gadget {
 	unsigned			b_hnp_enable:1;
 	unsigned			a_hnp_support:1;
 	unsigned			a_alt_hnp_support:1;
+	unsigned			quirk_ep_out_aligned_size:1;
 };
 #define work_to_gadget(w)	(container_of((w), struct usb_gadget, work))
 
-- 
1.8.4.2

[PATCH v6 3/5] usb: gadget: f_fs: remove loop from I/O function

[David Cohen]

From: Michal Nazarewicz <mina86@mina86.com>

When endpoint changes (due to it being disabled or alt setting changed),
mimic the action as if the change happened after the request has been
queued, instead of retrying with the new endpoint.

Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
Cc: David Cohen <david.a.cohen@linux.intel.com>
---
 drivers/usb/gadget/f_fs.c | 94 ++++++++++++++++++++---------------------------
 1 file changed, 40 insertions(+), 54 deletions(-)

diff --git a/drivers/usb/gadget/f_fs.c b/drivers/usb/gadget/f_fs.c
index 75e4b7846a8d..efa1152a4c15 100644
--- a/drivers/usb/gadget/f_fs.c
+++ b/drivers/usb/gadget/f_fs.c
@@ -760,73 +760,59 @@ static ssize_t ffs_epfile_io(struct file *file,
 	ssize_t ret;
 	int halt;
 
-	goto first_try;
-	do {
-		spin_unlock_irq(&epfile->ffs->eps_lock);
-		mutex_unlock(&epfile->mutex);
+	/* Are we still active? */
+	if (WARN_ON(epfile->ffs->state != FFS_ACTIVE)) {
+		ret = -ENODEV;
+		goto error;
+	}
 
-first_try:
-		/* Are we still active? */
-		if (WARN_ON(epfile->ffs->state != FFS_ACTIVE)) {
-			ret = -ENODEV;
+	/* Wait for endpoint to be enabled */
+	ep = epfile->ep;
+	if (!ep) {
+		if (file->f_flags & O_NONBLOCK) {
+			ret = -EAGAIN;
 			goto error;
 		}
 
-		/* Wait for endpoint to be enabled */
-		ep = epfile->ep;
-		if (!ep) {
-			if (file->f_flags & O_NONBLOCK) {
-				ret = -EAGAIN;
-				goto error;
-			}
-
-			if (wait_event_interruptible(epfile->wait,
-						     (ep = epfile->ep))) {
-				ret = -EINTR;
-				goto error;
-			}
-		}
-
-		/* Do we halt? */
-		halt = !read == !epfile->in;
-		if (halt && epfile->isoc) {
-			ret = -EINVAL;
+		ret = wait_event_interruptible(epfile->wait, (ep = epfile->ep));
+		if (ret) {
+			ret = -EINTR;
 			goto error;
 		}
+	}
 
-		/* Allocate & copy */
-		if (!halt && !data) {
-			data = kzalloc(len, GFP_KERNEL);
-			if (unlikely(!data))
-				return -ENOMEM;
+	/* Do we halt? */
+	halt = !read == !epfile->in;
+	if (halt && epfile->isoc) {
+		ret = -EINVAL;
+		goto error;
+	}
 
-			if (!read &&
-			    unlikely(__copy_from_user(data, buf, len))) {
-				ret = -EFAULT;
-				goto error;
-			}
-		}
+	/* Allocate & copy */
+	if (!halt) {
+		data = kmalloc(len, GFP_KERNEL);
+		if (unlikely(!data))
+			return -ENOMEM;
 
-		/* We will be using request */
-		ret = ffs_mutex_lock(&epfile->mutex,
-				     file->f_flags & O_NONBLOCK);
-		if (unlikely(ret))
+		if (!read && unlikely(copy_from_user(data, buf, len))) {
+			ret = -EFAULT;
 			goto error;
+		}
+	}
 
-		/*
-		 * We're called from user space, we can use _irq rather then
-		 * _irqsave
-		 */
-		spin_lock_irq(&epfile->ffs->eps_lock);
+	/* We will be using request */
+	ret = ffs_mutex_lock(&epfile->mutex, file->f_flags & O_NONBLOCK);
+	if (unlikely(ret))
+		goto error;
 
-		/*
-		 * While we were acquiring mutex endpoint got disabled
-		 * or changed?
-		 */
-	} while (unlikely(epfile->ep != ep));
+	spin_lock_irq(&epfile->ffs->eps_lock);
 
-	/* Halt */
-	if (unlikely(halt)) {
+	if (epfile->ep != ep) {
+		/* In the meantime, endpoint got disabled or changed. */
+		ret = -ESHUTDOWN;
+		spin_unlock_irq(&epfile->ffs->eps_lock);
+	} else if (halt) {
+		/* Halt */
 		if (likely(epfile->ep == ep) && !WARN_ON(!ep->ep))
 			usb_ep_set_halt(ep->ep);
 		spin_unlock_irq(&epfile->ffs->eps_lock);
-- 
1.8.4.2

[PATCH v6 4/5] check quirk to pad epout buf size when not aligned to maxpacketsize

[David Cohen]

From: Michal Nazarewicz <mina86@mina86.com>

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: Michal Nazarewicz <mina86@mina86.com>
Acked-by: David Cohen <david.a.cohen@linux.intel.com>
---
 drivers/usb/gadget/f_fs.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/f_fs.c b/drivers/usb/gadget/f_fs.c
index efa1152a4c15..688cfa005b4d 100644
--- a/drivers/usb/gadget/f_fs.c
+++ b/drivers/usb/gadget/f_fs.c
@@ -755,6 +755,7 @@ 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;
@@ -790,7 +791,14 @@ static ssize_t ffs_epfile_io(struct file *file,
 
 	/* Allocate & copy */
 	if (!halt) {
-		data = kmalloc(len, GFP_KERNEL);
+		/*
+		 * Controller requires buffer size to be aligned to
+		 * maxpacketsize of an out endpoint.
+		 */
+		size_t data_len = read && gadget->quirk_ep_out_aligned_size ?
+			usb_ep_align_maxpacketsize(ep->ep, len) : len;
+
+		data = kmalloc(data_len, GFP_KERNEL);
 		if (unlikely(!data))
 			return -ENOMEM;
 
-- 
1.8.4.2

[PATCH v6 5/5] usb: dwc3: implement gadget's quirk ep_out_align_size

[David Cohen]

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

Signed-off-by: David Cohen <david.a.cohen@linux.intel.com>
---
 drivers/usb/dwc3/core.h   |  6 ++++++
 drivers/usb/dwc3/gadget.c | 23 +++++++++++++++++++++++
 2 files changed, 29 insertions(+)

diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index f8af8d44af85..ff42d7ddc546 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -571,6 +571,12 @@ struct dwc3_request {
 	struct dwc3_ep		*dep;
 	u32			start_slot;
 
+	/*
+	 * If gadget/epout, we need to pad buffer size to align with
+	 * maxpacketsize.
+	 */
+	size_t			pad;
+
 	u8			epnum;
 	struct dwc3_trb		*trb;
 	dma_addr_t		trb_dma;
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 5452c0fce360..7c2d36f6ad4b 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -1130,6 +1130,14 @@ static int dwc3_gadget_ep_queue(struct usb_ep *ep, struct usb_request *request,
 	dev_vdbg(dwc->dev, "queing request %p to %s length %d\n",
 			request, ep->name, request->length);
 
+	/* If ep out, roundup request->length to epout maxpacketsize */
+	if (!(dep->number & 1)) {
+		unsigned int aligned = roundup(request->length,
+					       ep->desc->wMaxPacketSize);
+		req->pad = aligned - request->length;
+		request->length = aligned;
+	}
+
 	spin_lock_irqsave(&dwc->lock, flags);
 	ret = __dwc3_gadget_ep_queue(dep, req);
 	spin_unlock_irqrestore(&dwc->lock, flags);
@@ -1173,6 +1181,15 @@ static int dwc3_gadget_ep_dequeue(struct usb_ep *ep,
 	}
 
 out1:
+	if (!(dep->number & 1)) {
+		/*
+		 * Sanitize request->length after pad was applied before
+		 * queue.
+		 */
+		request->length -= req->pad;
+		req->pad = 0;
+	}
+
 	/* giveback the request */
 	dwc3_gadget_giveback(dep, req, -ECONNRESET);
 
@@ -2600,6 +2617,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 = true;
+
+	/*
 	 * REVISIT: Here we should clear all pending IRQs to be
 	 * sure we're starting from a well known location.
 	 */
-- 
1.8.4.2

Re: [PATCH v6 0/5] add gadget quirk to adapt f_fs for DWC3

[David Cohen]

On 11/12/2013 01:04 PM, David Cohen wrote:
> 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.
>
> This new patch set was tested and validated in my environment:
>   - Intel Merrifield was able to use DWC3/f_fs with adbd service (it wasn't
>     before).
>
> Changes from v45 to v6:

I hope not have to get to v45 :) but I meant v5.

Re: [PATCH v6 5/5] usb: dwc3: implement gadget's quirk ep_out_align_size

[Felipe Balbi]

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

Hi,

On Tue, Nov 12, 2013 at 01:04:46PM -0800, David Cohen wrote:
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 5452c0fce360..7c2d36f6ad4b 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -1130,6 +1130,14 @@ static int dwc3_gadget_ep_queue(struct usb_ep *ep, struct usb_request *request,
>  	dev_vdbg(dwc->dev, "queing request %p to %s length %d\n",
>  			request, ep->name, request->length);
>  
> +	/* If ep out, roundup request->length to epout maxpacketsize */
> +	if (!(dep->number & 1)) {

we have a direction field in the dep structure, please use that.

> +		unsigned int aligned = roundup(request->length,
> +					       ep->desc->wMaxPacketSize);
> +		req->pad = aligned - request->length;
> +		request->length = aligned;

this is quite dangerous. You really don't know the size that gadget
driver allocated. What if we're using SLOB and gadget driver allocated
exactly 31 bytes (think MSC's CBW) ? Then you change request->length to
512-bytes (or 1024 if USB SS), and host happens to be buggy (or
exploited somehow) and sends more than 31-bytes ? You told dwc3 you
could receive more than 31-bytes even though you don't know what follows
your 31-byte buffer.

This is why I have been saying that gadget driver *must* be the one
hadnling this issue based on the quirk flag.

-- 
balbi

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

Re: [PATCH v6 5/5] usb: dwc3: implement gadget's quirk ep_out_align_size

[David Cohen]

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

On 11/25/2013 01:06 PM, Felipe Balbi wrote:
> Hi,
> 
> On Tue, Nov 12, 2013 at 01:04:46PM -0800, David Cohen wrote:
>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> index 5452c0fce360..7c2d36f6ad4b 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
>> @@ -1130,6 +1130,14 @@ static int dwc3_gadget_ep_queue(struct usb_ep *ep, struct usb_request *request,
>>  	dev_vdbg(dwc->dev, "queing request %p to %s length %d\n",
>>  			request, ep->name, request->length);
>>  
>> +	/* If ep out, roundup request->length to epout maxpacketsize */
>> +	if (!(dep->number & 1)) {
> 
> we have a direction field in the dep structure, please use that.
> 
>> +		unsigned int aligned = roundup(request->length,
>> +					       ep->desc->wMaxPacketSize);
>> +		req->pad = aligned - request->length;
>> +		request->length = aligned;
> 
> this is quite dangerous. You really don't know the size that gadget
> driver allocated. What if we're using SLOB and gadget driver allocated
> exactly 31 bytes (think MSC's CBW) ? Then you change request->length to
> 512-bytes (or 1024 if USB SS), and host happens to be buggy (or
> exploited somehow) and sends more than 31-bytes ? You told dwc3 you
> could receive more than 31-bytes even though you don't know what follows
> your 31-byte buffer.
> 
> This is why I have been saying that gadget driver *must* be the one
> hadnling this issue based on the quirk flag.

Thanks. I've seen different point of views in this thread. Since you're
the maintainer, I'll resend the patch following your directions.

Br, David



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 897 bytes --]

Re: [PATCH v6 5/5] usb: dwc3: implement gadget's quirk ep_out_align_size

[David Cohen]

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

Hi Felipe,

>> This is why I have been saying that gadget driver *must* be the one
>> hadnling this issue based on the quirk flag.
> 
> Thanks. I've seen different point of views in this thread. Since you're
> the maintainer, I'll resend the patch following your directions.

Probably you want the v5 I sent (disregarding the v5.1 4/5 amend):
http://marc.info/?l=linux-usb&m=138420082526804&w=2

Br, David Cohen


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 897 bytes --]