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

[PATCH v5 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 v4 to v5:
 - Added 2 patches from Michal Nazarewicz to address comments from Alan Stern
   on f_fs driver.

---
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: set gadget's quirk ep_out_align_size

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

 drivers/usb/dwc3/gadget.c  |   6 +++
 drivers/usb/gadget/f_fs.c  | 115 +++++++++++++++++++++++----------------------
 include/linux/usb/gadget.h |  35 ++++++++++----
 3 files changed, 91 insertions(+), 65 deletions(-)

-- 
1.8.4.rc3

[PATCH v5 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>
---
 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.rc3

[PATCH v5 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>
---

Michal,

I added prefix 'usb: f_fs: ' to patch's subject. I did not add 'usb: gadget:
f_fs' due to it would be too long.


 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.rc3

[PATCH v5 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, 41 insertions(+), 53 deletions(-)

diff --git a/drivers/usb/gadget/f_fs.c b/drivers/usb/gadget/f_fs.c
index 75e4b7846a8d..f1563c47e0c2 100644
--- a/drivers/usb/gadget/f_fs.c
+++ b/drivers/usb/gadget/f_fs.c
@@ -756,74 +756,61 @@ static ssize_t ffs_epfile_io(struct file *file,
 {
 	struct ffs_epfile *epfile = file->private_data;
 	struct ffs_ep *ep;
-	char *data = NULL;
 	ssize_t ret;
+	char *data;
 	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;
+		if (wait_event_interruptible(epfile->wait, (ep = epfile->ep))) {
+			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;
+	spin_lock_irq(&epfile->ffs->eps_lock);
 
-		/*
-		 * While we were acquiring mutex endpoint got disabled
-		 * or changed?
-		 */
-	} while (unlikely(epfile->ep != ep));
+	/* While we were acquiring mutex endpoint got disabled or changed. */
+	if (epfile->ep != ep) {
+		ret = -ESHUTDOWN;
+		spin_unlock_irq(&epfile->ffs->eps_lock);
+		goto error_unlock;
+	}
 
 	/* Halt */
 	if (unlikely(halt)) {
@@ -858,6 +845,7 @@ first_try:
 		}
 	}
 
+error_unlock:
 	mutex_unlock(&epfile->mutex);
 error:
 	kfree(data);
-- 
1.8.4.rc3

[PATCH v5 4/5] usb: f_fs: 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 | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/gadget/f_fs.c b/drivers/usb/gadget/f_fs.c
index f1563c47e0c2..c49288778fe0 100644
--- a/drivers/usb/gadget/f_fs.c
+++ b/drivers/usb/gadget/f_fs.c
@@ -755,8 +755,9 @@ 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;
-	ssize_t ret;
+	ssize_t ret, data_len;
 	char *data;
 	int halt;
 
@@ -789,7 +790,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.
+		 */
+		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;
 
@@ -826,7 +834,7 @@ static ssize_t ffs_epfile_io(struct file *file,
 		req->context  = &done;
 		req->complete = ffs_epfile_io_complete;
 		req->buf      = data;
-		req->length   = len;
+		req->length   = data_len;
 
 		ret = usb_ep_queue(ep->ep, req, GFP_ATOMIC);
 
@@ -838,9 +846,16 @@ static ssize_t ffs_epfile_io(struct file *file,
 			ret = -EINTR;
 			usb_ep_dequeue(ep->ep, req);
 		} else {
+			/*
+			 * XXX We may end up silently droping data here.
+			 * Since data_len (i.e. req->length) may be bigger
+			 * than len (after being rounded up to maxpacketsize),
+			 * we may end up with more data then user space has
+			 * space for.
+			 */
 			ret = ep->status;
 			if (read && ret > 0 &&
-			    unlikely(copy_to_user(buf, data, ret)))
+			    unlikely(copy_to_user(buf, data, min(ret, len))))
 				ret = -EFAULT;
 		}
 	}
-- 
1.8.4.rc3

[PATCH v5 5/5] usb: dwc3: set gadget's quirk ep_out_align_size

[David Cohen]

DWC3 requires epout to have buffer size aligned to MaxPacketSize value.
This patch sets 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 5452c0fce360..b85ec110d6a0 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 = true;
+
+	/*
 	 * 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 v5 3/5] usb: gadget: f_fs: remove loop from I/O function

[David Cohen]

On 11/11/2013 12:16 PM, David Cohen wrote:
> 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, 41 insertions(+), 53 deletions(-)
>
> diff --git a/drivers/usb/gadget/f_fs.c b/drivers/usb/gadget/f_fs.c
> index 75e4b7846a8d..f1563c47e0c2 100644
> --- a/drivers/usb/gadget/f_fs.c
> +++ b/drivers/usb/gadget/f_fs.c
> @@ -756,74 +756,61 @@ static ssize_t ffs_epfile_io(struct file *file,
>   {
>   	struct ffs_epfile *epfile = file->private_data;
>   	struct ffs_ep *ep;
> -	char *data = NULL;
>   	ssize_t ret;
> +	char *data;
>   	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;
> +		if (wait_event_interruptible(epfile->wait, (ep = epfile->ep))) {
> +			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;

One curiosity here. This patch prints the following warning:

In file included from (...)/drivers/usb/gadget/g_ffs.c:55:0:
(...)/drivers/usb/gadget/f_fs.c: In function 'ffs_epfile_io.isra.18':
(...)/drivers/usb/gadget/f_fs.c:837:15: warning: 'data_len' may be used 
uninitialized in this function [-Wmaybe-uninitialized]

But if we do this dummy change on it:

@@ -782,7 +782,10 @@ static ssize_t ffs_epfile_io(struct file *file,
         }

         /* Do we halt? */
-       halt = !read == !epfile->in;
+       if (!read == !epfile->in)
+               halt = 1;
+       else
+               halt = 0;
         if (halt && epfile->isoc) {
                 ret = -EINVAL;
                 goto error;

The warning goes away. This false-positive warning comes out of this
gcc version:
$ i686-linux-android-gcc --version
i686-linux-android-gcc (GCC) 4.7

Br, David Cohen

> +	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;
> +	spin_lock_irq(&epfile->ffs->eps_lock);
>
> -		/*
> -		 * While we were acquiring mutex endpoint got disabled
> -		 * or changed?
> -		 */
> -	} while (unlikely(epfile->ep != ep));
> +	/* While we were acquiring mutex endpoint got disabled or changed. */
> +	if (epfile->ep != ep) {
> +		ret = -ESHUTDOWN;
> +		spin_unlock_irq(&epfile->ffs->eps_lock);
> +		goto error_unlock;
> +	}
>
>   	/* Halt */
>   	if (unlikely(halt)) {
> @@ -858,6 +845,7 @@ first_try:
>   		}
>   	}
>
> +error_unlock:
>   	mutex_unlock(&epfile->mutex);
>   error:
>   	kfree(data);
>

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

[Michal Nazarewicz]

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

On Mon, Nov 11 2013, David Cohen wrote:
> 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.rc3
>

-- 
Best regards,                                         _     _
.o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz    (o o)
ooo +--<mpn@google.com>--<xmpp:mina86@jabber.org>--ooO--(_)--Ooo--

[-- Attachment #2.1: Type: text/plain, Size: 0 bytes --]



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

Re: [PATCH v5 2/5] usb: gadget: add quirk_ep_out_aligned_size field to struct usb_gadget

[Michal Nazarewicz]

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

On Mon, Nov 11 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. 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>

> ---
>
> Michal,
>
> I added prefix 'usb: f_fs: ' to patch's subject. I did not add 'usb: gadget:
> f_fs' due to it would be too long.
>
>
>  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);
> +}
> +

Come to think of it, perhaps even better helper would be:

static inline size_t usb_ep_align_maybe(
	struct usb_gadget *gadget, struct usb_ep *ep, size_t len) {
	return gadget->quir_ep_out_aligned_size ?
		round_up(len, (size_t)ep->desc->wMaxPacketSize) : len;
}

>  
>  /*-------------------------------------------------------------------------*/
>  
> @@ -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.rc3
>

-- 
Best regards,                                         _     _
.o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz    (o o)
ooo +--<mpn@google.com>--<xmpp:mina86@jabber.org>--ooO--(_)--Ooo--

[-- Attachment #2.1: Type: text/plain, Size: 0 bytes --]



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

[PATCHv5.1 3/5] usb: gadget: f_fs: remove loop from I/O function

[Michal Nazarewicz]

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(-)

Fixed checkpatch.pl issues pointed out by David.

diff --git a/drivers/usb/gadget/f_fs.c b/drivers/usb/gadget/f_fs.c
index 0658908..6cb0b22 100644
--- a/drivers/usb/gadget/f_fs.c
+++ b/drivers/usb/gadget/f_fs.c
@@ -758,73 +758,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.1

[PATCHv5.1 4/5] check quirk to pad epout buf size when not aligned to maxpacketsize

[Michal Nazarewicz]

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>
---
 drivers/usb/gadget/f_fs.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

On Tue, Nov 12 2013, David Cohen wrote:
> One curiosity here. This patch prints the following warning:
>
> In file included from (...)/drivers/usb/gadget/g_ffs.c:55:0:
> (...)/drivers/usb/gadget/f_fs.c: In function 'ffs_epfile_io.isra.18':
> (...)/drivers/usb/gadget/f_fs.c:837:15: warning: 'data_len' may be used 
> uninitialized in this function [-Wmaybe-uninitialized]

With the below code, this should no longer be a problem.

Also fixes issues pointed out by Alan.

diff --git a/drivers/usb/gadget/f_fs.c b/drivers/usb/gadget/f_fs.c
index 6cb0b22..9aa3200 100644
--- a/drivers/usb/gadget/f_fs.c
+++ b/drivers/usb/gadget/f_fs.c
@@ -753,6 +753,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;
@@ -788,7 +789,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.1

Re: [PATCH v5 2/5] usb: gadget: add quirk_ep_out_aligned_size field to struct usb_gadget

[David Cohen]

On 11/11/2013 03:55 PM, Michal Nazarewicz wrote:
> On Mon, Nov 11 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. 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>
>
>> ---
>>
>> Michal,
>>
>> I added prefix 'usb: f_fs: ' to patch's subject. I did not add 'usb: gadget:
>> f_fs' due to it would be too long.
>>
>>
>>   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);
>> +}
>> +
>
> Come to think of it, perhaps even better helper would be:
>
> static inline size_t usb_ep_align_maybe(
> 	struct usb_gadget *gadget, struct usb_ep *ep, size_t len) {
> 	return gadget->quir_ep_out_aligned_size ?
> 		round_up(len, (size_t)ep->desc->wMaxPacketSize) : len;
> }

The CPU time to check unsigned:1 and possibly jump is about the same as
round_up() itself. For readability matters, we can round_up() directly.

Br, David

Re: [PATCHv5.1 4/5] check quirk to pad epout buf size when not aligned to maxpacketsize

[David Cohen]

Hi Michal,

On 11/11/2013 03:58 PM, Michal Nazarewicz 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: Michal Nazarewicz <mina86@mina86.com>
> ---
>   drivers/usb/gadget/f_fs.c | 10 +++++++++-
>   1 file changed, 9 insertions(+), 1 deletion(-)
>
> On Tue, Nov 12 2013, David Cohen wrote:
>> One curiosity here. This patch prints the following warning:
>>
>> In file included from (...)/drivers/usb/gadget/g_ffs.c:55:0:
>> (...)/drivers/usb/gadget/f_fs.c: In function 'ffs_epfile_io.isra.18':
>> (...)/drivers/usb/gadget/f_fs.c:837:15: warning: 'data_len' may be used
>> uninitialized in this function [-Wmaybe-uninitialized]
>
> With the below code, this should no longer be a problem.
>
> Also fixes issues pointed out by Alan.

You need to update req->length otherwise it's going to crash DWC3.
I'd rather to keep your previous version.

Br, David

Re: [PATCH v5 2/5] usb: gadget: add quirk_ep_out_aligned_size field to struct usb_gadget

[Michal Nazarewicz]

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

On Tue, Nov 12 2013, David Cohen wrote:
> On 11/11/2013 03:55 PM, Michal Nazarewicz wrote:
>> Come to think of it, perhaps even better helper would be:
>>
>> static inline size_t usb_ep_align_maybe(
>> 	struct usb_gadget *gadget, struct usb_ep *ep, size_t len) {
>> 	return gadget->quir_ep_out_aligned_size ?
>> 		round_up(len, (size_t)ep->desc->wMaxPacketSize) : len;
>> }
>
> The CPU time to check unsigned:1 and possibly jump is about the same as
> round_up() itself. For readability matters, we can round_up() directly.

I was proposing to have this function and than not have functions check
for the flag.  I.e. instead of

	if (gadget->quirk_ep_out_aligned_size)
		len = usb_ep_align_maxpacketsize(ep, len);

the code would just be:

	len = usb_ep_align_maybe(gadget, ep, len);

-- 
Best regards,                                         _     _
.o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz    (o o)
ooo +--<mpn@google.com>--<xmpp:mina86@jabber.org>--ooO--(_)--Ooo--

[-- Attachment #2.1: Type: text/plain, Size: 0 bytes --]



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

Re: [PATCHv5.1 4/5] check quirk to pad epout buf size when not aligned to maxpacketsize

[Michal Nazarewicz]

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

On Tue, Nov 12 2013, David Cohen wrote:
> You need to update req->length otherwise it's going to crash DWC3.
> I'd rather to keep your previous version.

That's unfortunate.  Do you want me to resend it or will you just send
a v6 of your whole series?

-- 
Best regards,                                         _     _
.o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz    (o o)
ooo +--<mpn@google.com>--<xmpp:mina86@jabber.org>--ooO--(_)--Ooo--

[-- Attachment #2.1: Type: text/plain, Size: 0 bytes --]



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

Re: [PATCHv5.1 4/5] check quirk to pad epout buf size when not aligned to maxpacketsize

[David Cohen]

On 11/12/2013 04:59 AM, Michal Nazarewicz wrote:
> On Tue, Nov 12 2013, David Cohen wrote:
>> You need to update req->length otherwise it's going to crash DWC3.
>> I'd rather to keep your previous version.
> 
> That's unfortunate.  Do you want me to resend it or will you just send
> a v6 of your whole series?

Alan disagreed and proposed a good approach to fix req->length inside
DWC3. We can keep this patch of yours but I'll need to change mine to
DWC3. I'll send a v6 with update on my patch.

Br, David

Re: [PATCH v5 2/5] usb: gadget: add quirk_ep_out_aligned_size field to struct usb_gadget

[Felipe Balbi]

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

On Tue, Nov 12, 2013 at 01:54:28PM +0100, Michal Nazarewicz wrote:
> On Tue, Nov 12 2013, David Cohen wrote:
> > On 11/11/2013 03:55 PM, Michal Nazarewicz wrote:
> >> Come to think of it, perhaps even better helper would be:
> >>
> >> static inline size_t usb_ep_align_maybe(
> >> 	struct usb_gadget *gadget, struct usb_ep *ep, size_t len) {
> >> 	return gadget->quir_ep_out_aligned_size ?
> >> 		round_up(len, (size_t)ep->desc->wMaxPacketSize) : len;
> >> }
> >
> > The CPU time to check unsigned:1 and possibly jump is about the same as
> > round_up() itself. For readability matters, we can round_up() directly.
> 
> I was proposing to have this function and than not have functions check
> for the flag.  I.e. instead of
> 
> 	if (gadget->quirk_ep_out_aligned_size)
> 		len = usb_ep_align_maxpacketsize(ep, len);
> 
> the code would just be:
> 
> 	len = usb_ep_align_maybe(gadget, ep, len);

that looks very good to me, do we have a version with that already ?

-- 
balbi

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

Re: [PATCH v5 2/5] usb: gadget: add quirk_ep_out_aligned_size field to struct usb_gadget

[David Cohen]

On 12/06/2013 12:13 PM, Felipe Balbi wrote:
> On Tue, Nov 12, 2013 at 01:54:28PM +0100, Michal Nazarewicz wrote:
>> On Tue, Nov 12 2013, David Cohen wrote:
>>> On 11/11/2013 03:55 PM, Michal Nazarewicz wrote:
>>>> Come to think of it, perhaps even better helper would be:
>>>>
>>>> static inline size_t usb_ep_align_maybe(
>>>> 	struct usb_gadget *gadget, struct usb_ep *ep, size_t len) {
>>>> 	return gadget->quir_ep_out_aligned_size ?
>>>> 		round_up(len, (size_t)ep->desc->wMaxPacketSize) : len;
>>>> }
>>>
>>> The CPU time to check unsigned:1 and possibly jump is about the same as
>>> round_up() itself. For readability matters, we can round_up() directly.
>>
>> I was proposing to have this function and than not have functions check
>> for the flag.  I.e. instead of
>>
>> 	if (gadget->quirk_ep_out_aligned_size)
>> 		len = usb_ep_align_maxpacketsize(ep, len);
>>
>> the code would just be:
>>
>> 	len = usb_ep_align_maybe(gadget, ep, len);
> 
> that looks very good to me, do we have a version with that already ?

Nope. But as soon as soon as you finish the whole review, I can resend
new version with all changes at once.

Br, David

Re: [PATCHv5.1 3/5] usb: gadget: f_fs: remove loop from I/O function

[Felipe Balbi]

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

On Tue, Nov 12, 2013 at 12:57:22AM +0100, Michal Nazarewicz wrote:
> 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>

pretty obvious patch, looks good.

-- 
balbi

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

Re: [PATCH v5 5/5] usb: dwc3: set gadget's quirk ep_out_align_size

[Felipe Balbi]

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

On Mon, Nov 11, 2013 at 12:16:51PM -0800, David Cohen wrote:
> DWC3 requires epout to have buffer size aligned to MaxPacketSize value.
> This patch sets necessary quirk for it.
> 
> Signed-off-by: David Cohen <david.a.cohen@linux.intel.com>

looks good

-- 
balbi

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

Re: [PATCHv5.1 4/5] check quirk to pad epout buf size when not aligned to maxpacketsize

[Felipe Balbi]

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

On Tue, Nov 12, 2013 at 10:26:17AM -0800, David Cohen wrote:
> On 11/12/2013 04:59 AM, Michal Nazarewicz wrote:
> > On Tue, Nov 12 2013, David Cohen wrote:
> >> You need to update req->length otherwise it's going to crash DWC3.
> >> I'd rather to keep your previous version.
> > 
> > That's unfortunate.  Do you want me to resend it or will you just send
> > a v6 of your whole series?
> 
> Alan disagreed and proposed a good approach to fix req->length inside
> DWC3. We can keep this patch of yours but I'll need to change mine to
> DWC3. I'll send a v6 with update on my patch.

I rather update req->length on the functions. They own the buffer anyway

-- 
balbi

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