[PATCH v4 0/4] add gadget quirk to adapt f_fs for DWC3

[PATCH v4 0/4] 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.

Changes from v3 to v4:
 - replace u32 quirk flags by single unsigned:1 flag for ep out aligned size
   quirk on usb_gadget
 - add static inline helper to align ep out buf size

---
David Cohen (4):
  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: 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  | 22 ++++++++++++++++++++++
 include/linux/usb/gadget.h | 38 +++++++++++++++++++++++++++++---------
 3 files changed, 57 insertions(+), 9 deletions(-)

-- 
1.8.4.rc3

[PATCH v4 1/4] 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 v4 2/4] 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>
---
 include/linux/usb/gadget.h | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
index 23b3bfd0a842..260d972489bd 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -442,6 +442,22 @@ static inline void usb_ep_fifo_flush(struct usb_ep *ep)
 }
 
 
+static inline size_t usb_ep_align_maxpacketsize(struct usb_ep *ep, size_t len)
+{
+	int aligned;
+
+	if (ep->desc->bmAttributes & USB_ENDPOINT_XFER_INT)
+		/*
+		 * Interrupt eps don't need max packet size to be power of 2,
+		 * so can't use cheap IS_ALIGNED() macro.
+		 */
+		aligned = !(len % ep->desc->wMaxPacketSize);
+	else
+		aligned = IS_ALIGNED(len, ep->desc->wMaxPacketSize);
+
+	return aligned ? len : round_up(len, (size_t)ep->desc->wMaxPacketSize);
+}
+
 /*-------------------------------------------------------------------------*/
 
 struct usb_dcd_config_params {
@@ -502,6 +518,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 +559,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 v4 3/4] 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 | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/usb/gadget/f_fs.c b/drivers/usb/gadget/f_fs.c
index 75e4b7846a8d..e7c3c3119552 100644
--- a/drivers/usb/gadget/f_fs.c
+++ b/drivers/usb/gadget/f_fs.c
@@ -755,10 +755,13 @@ 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;
+	size_t data_len = 0;
 	ssize_t ret;
 	int halt;
+	size_t orig_len = len;
 
 	goto first_try;
 	do {
@@ -794,11 +797,30 @@ 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) {
+			/*
+			 * We pass 'orig_len' to usp_ep_align_maxpacketsize()
+			 * due to we're in a loop and 'len' may have been
+			 * changed.
+			 */
+			len = usb_ep_align_maxpacketsize(ep->ep, orig_len);
+			if (data && len > data_len) {
+				kfree(data);
+				data = NULL;
+				data_len = 0;
+			}
+		}
+
 		/* Allocate & copy */
 		if (!halt && !data) {
 			data = kzalloc(len, GFP_KERNEL);
 			if (unlikely(!data))
 				return -ENOMEM;
+			data_len = len;
 
 			if (!read &&
 			    unlikely(__copy_from_user(data, buf, len))) {
-- 
1.8.4.rc3

[PATCH v4 4/4] 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 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

[PATCH v4.1 4/4] 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>
---

Changes from v4 to v4.1:
 - just updated patch's subject. No actual code changed.

 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 v4 2/4] usb: gadget: add quirk_ep_out_aligned_size field to struct usb_gadget

[Alan Stern]

On Mon, 4 Nov 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>
> ---
>  include/linux/usb/gadget.h | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
> index 23b3bfd0a842..260d972489bd 100644
> --- a/include/linux/usb/gadget.h
> +++ b/include/linux/usb/gadget.h
> @@ -442,6 +442,22 @@ static inline void usb_ep_fifo_flush(struct usb_ep *ep)
>  }
>  
>  
> +static inline size_t usb_ep_align_maxpacketsize(struct usb_ep *ep, size_t len)
> +{
> +	int aligned;
> +
> +	if (ep->desc->bmAttributes & USB_ENDPOINT_XFER_INT)
> +		/*
> +		 * Interrupt eps don't need max packet size to be power of 2,
> +		 * so can't use cheap IS_ALIGNED() macro.
> +		 */
> +		aligned = !(len % ep->desc->wMaxPacketSize);
> +	else
> +		aligned = IS_ALIGNED(len, ep->desc->wMaxPacketSize);

This isn't on a hot path, and I suspect that the extra "if" will 
require nearly as much time as you save by not doing the division.  You 
might as well always use the % operation.

Alan Stern

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

[Alan Stern]

On Mon, 4 Nov 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 | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/drivers/usb/gadget/f_fs.c b/drivers/usb/gadget/f_fs.c
> index 75e4b7846a8d..e7c3c3119552 100644
> --- a/drivers/usb/gadget/f_fs.c
> +++ b/drivers/usb/gadget/f_fs.c
> @@ -755,10 +755,13 @@ 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;
> +	size_t data_len = 0;
>  	ssize_t ret;
>  	int halt;
> +	size_t orig_len = len;
>  
>  	goto first_try;
>  	do {
> @@ -794,11 +797,30 @@ 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) {
> +			/*
> +			 * We pass 'orig_len' to usp_ep_align_maxpacketsize()
> +			 * due to we're in a loop and 'len' may have been
> +			 * changed.
> +			 */
> +			len = usb_ep_align_maxpacketsize(ep->ep, orig_len);
> +			if (data && len > data_len) {
> +				kfree(data);
> +				data = NULL;
> +				data_len = 0;
> +			}
> +		}

Since the value of orig_len never changes, there's no point calling 
usb_ep_align_maxpacketsize() inside the loop.  You should call it only 
once, before the loop starts.  Once you do that, you won't need 
orig_len at all.

Alan Stern

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

[David Cohen]

On 11/05/2013 06:52 AM, Alan Stern wrote:
> On Mon, 4 Nov 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 | 22 ++++++++++++++++++++++
>>  1 file changed, 22 insertions(+)
>>
>> diff --git a/drivers/usb/gadget/f_fs.c b/drivers/usb/gadget/f_fs.c
>> index 75e4b7846a8d..e7c3c3119552 100644
>> --- a/drivers/usb/gadget/f_fs.c
>> +++ b/drivers/usb/gadget/f_fs.c
>> @@ -755,10 +755,13 @@ 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;
>> +	size_t data_len = 0;
>>  	ssize_t ret;
>>  	int halt;
>> +	size_t orig_len = len;
>>  
>>  	goto first_try;
>>  	do {
>> @@ -794,11 +797,30 @@ 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) {
>> +			/*
>> +			 * We pass 'orig_len' to usp_ep_align_maxpacketsize()
>> +			 * due to we're in a loop and 'len' may have been
>> +			 * changed.
>> +			 */
>> +			len = usb_ep_align_maxpacketsize(ep->ep, orig_len);
>> +			if (data && len > data_len) {
>> +				kfree(data);
>> +				data = NULL;
>> +				data_len = 0;
>> +			}
>> +		}
> 
> Since the value of orig_len never changes, there's no point calling 
> usb_ep_align_maxpacketsize() inside the loop.  You should call it only 
> once, before the loop starts.  Once you do that, you won't need 
> orig_len at all.

orig_len doesn't change but ep->ep does. If USB specs say max packet
size won't change even if ep does, than we can call it from outside the
loop.

Br, David

Re: [PATCH v4 2/4] usb: gadget: add quirk_ep_out_aligned_size field to struct usb_gadget

[David Cohen]

On 11/05/2013 06:50 AM, Alan Stern wrote:
> On Mon, 4 Nov 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>
>> ---
>>  include/linux/usb/gadget.h | 19 +++++++++++++++++++
>>  1 file changed, 19 insertions(+)
>>
>> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
>> index 23b3bfd0a842..260d972489bd 100644
>> --- a/include/linux/usb/gadget.h
>> +++ b/include/linux/usb/gadget.h
>> @@ -442,6 +442,22 @@ static inline void usb_ep_fifo_flush(struct usb_ep *ep)
>>  }
>>  
>>  
>> +static inline size_t usb_ep_align_maxpacketsize(struct usb_ep *ep, size_t len)
>> +{
>> +	int aligned;
>> +
>> +	if (ep->desc->bmAttributes & USB_ENDPOINT_XFER_INT)
>> +		/*
>> +		 * Interrupt eps don't need max packet size to be power of 2,
>> +		 * so can't use cheap IS_ALIGNED() macro.
>> +		 */
>> +		aligned = !(len % ep->desc->wMaxPacketSize);
>> +	else
>> +		aligned = IS_ALIGNED(len, ep->desc->wMaxPacketSize);
> 
> This isn't on a hot path, and I suspect that the extra "if" will 
> require nearly as much time as you save by not doing the division.  You 
> might as well always use the % operation.

Perhaps if I use unlikely() on 'if' condition instead?
Anyway I'll double check the costs of if + IS_ALIGNED vs modulo.

Br, David

Re: [PATCH v4 2/4] usb: gadget: add quirk_ep_out_aligned_size field to struct usb_gadget

[David Cohen]

On 11/05/2013 06:50 AM, Alan Stern wrote:
> On Mon, 4 Nov 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>
>> ---
>>  include/linux/usb/gadget.h | 19 +++++++++++++++++++
>>  1 file changed, 19 insertions(+)
>>
>> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
>> index 23b3bfd0a842..260d972489bd 100644
>> --- a/include/linux/usb/gadget.h
>> +++ b/include/linux/usb/gadget.h
>> @@ -442,6 +442,22 @@ static inline void usb_ep_fifo_flush(struct usb_ep *ep)
>>  }
>>  
>>  
>> +static inline size_t usb_ep_align_maxpacketsize(struct usb_ep *ep, size_t len)
>> +{
>> +	int aligned;
>> +
>> +	if (ep->desc->bmAttributes & USB_ENDPOINT_XFER_INT)
>> +		/*
>> +		 * Interrupt eps don't need max packet size to be power of 2,
>> +		 * so can't use cheap IS_ALIGNED() macro.
>> +		 */
>> +		aligned = !(len % ep->desc->wMaxPacketSize);
>> +	else
>> +		aligned = IS_ALIGNED(len, ep->desc->wMaxPacketSize);
> 
> This isn't on a hot path, and I suspect that the extra "if" will 
> require nearly as much time as you save by not doing the division.  You 
> might as well always use the % operation.

Perhaps if I use unlikely() on 'if' condition instead?
Anyway I'll double check the costs of if + IS_ALIGNED vs modulo.

Br, David

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

[Cohen, David A]

On 11/05/2013 06:52 AM, Alan Stern wrote:
> On Mon, 4 Nov 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 | 22 ++++++++++++++++++++++
>>  1 file changed, 22 insertions(+)
>>
>> diff --git a/drivers/usb/gadget/f_fs.c b/drivers/usb/gadget/f_fs.c
>> index 75e4b7846a8d..e7c3c3119552 100644
>> --- a/drivers/usb/gadget/f_fs.c
>> +++ b/drivers/usb/gadget/f_fs.c
>> @@ -755,10 +755,13 @@ 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;
>> +	size_t data_len = 0;
>>  	ssize_t ret;
>>  	int halt;
>> +	size_t orig_len = len;
>>  
>>  	goto first_try;
>>  	do {
>> @@ -794,11 +797,30 @@ 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) {
>> +			/*
>> +			 * We pass 'orig_len' to usp_ep_align_maxpacketsize()
>> +			 * due to we're in a loop and 'len' may have been
>> +			 * changed.
>> +			 */
>> +			len = usb_ep_align_maxpacketsize(ep->ep, orig_len);
>> +			if (data && len > data_len) {
>> +				kfree(data);
>> +				data = NULL;
>> +				data_len = 0;
>> +			}
>> +		}
> 
> Since the value of orig_len never changes, there's no point calling 
> usb_ep_align_maxpacketsize() inside the loop.  You should call it only 
> once, before the loop starts.  Once you do that, you won't need 
> orig_len at all.

orig_len doesn't change but ep->ep does. If USB specs say max packet
size won't change even if ep does, than we can call it from outside the
loop.

Br, David

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

[Alan Stern]

On Tue, 5 Nov 2013, David Cohen wrote:

> >> +		/*
> >> +		 * Controller requires buffer size to be aligned to
> >> +		 * maxpacketsize of an out endpoint.
> >> +		 */
> >> +		if (gadget->quirk_ep_out_aligned_size && read) {
> >> +			/*
> >> +			 * We pass 'orig_len' to usp_ep_align_maxpacketsize()
> >> +			 * due to we're in a loop and 'len' may have been
> >> +			 * changed.
> >> +			 */
> >> +			len = usb_ep_align_maxpacketsize(ep->ep, orig_len);
> >> +			if (data && len > data_len) {
> >> +				kfree(data);
> >> +				data = NULL;
> >> +				data_len = 0;
> >> +			}
> >> +		}
> > 
> > Since the value of orig_len never changes, there's no point calling 
> > usb_ep_align_maxpacketsize() inside the loop.  You should call it only 
> > once, before the loop starts.  Once you do that, you won't need 
> > orig_len at all.
> 
> orig_len doesn't change but ep->ep does. If USB specs say max packet
> size won't change even if ep does, than we can call it from outside the
> loop.

I'm not too familiar with this driver.  It looks like the only way 
ep->ep can change is if the endpoint gets enabled while you're sitting 
inside the wait_event_interruptible() call.

In fact, the whole structure of that loop looks peculiar.  Why not
acquire the mutex first and then do everything else?

Does it even make sense for ep to change?  Would this change be visible
to the host?  What if the host changes the alternate setting while this
loop is running -- does it make sense for the userspace program to
start a read or write under one altsetting but then have the read/write
take place under a different altsetting?

Alan Stern

Re: [PATCH v4 2/4] usb: gadget: add quirk_ep_out_aligned_size field to struct usb_gadget

[Alan Stern]

On Tue, 5 Nov 2013, David Cohen wrote:

> >> +static inline size_t usb_ep_align_maxpacketsize(struct usb_ep *ep, size_t len)
> >> +{
> >> +	int aligned;
> >> +
> >> +	if (ep->desc->bmAttributes & USB_ENDPOINT_XFER_INT)
> >> +		/*
> >> +		 * Interrupt eps don't need max packet size to be power of 2,
> >> +		 * so can't use cheap IS_ALIGNED() macro.
> >> +		 */
> >> +		aligned = !(len % ep->desc->wMaxPacketSize);
> >> +	else
> >> +		aligned = IS_ALIGNED(len, ep->desc->wMaxPacketSize);
> > 
> > This isn't on a hot path, and I suspect that the extra "if" will 
> > require nearly as much time as you save by not doing the division.  You 
> > might as well always use the % operation.
> 
> Perhaps if I use unlikely() on 'if' condition instead?
> Anyway I'll double check the costs of if + IS_ALIGNED vs modulo.

You're missing the point.  You and I (not to mention anybody who ever
reads this code in the future) have already wasted more time talking
about it and trying to understand it than you will ever save by using
IS_ALIGNED.

The difference to the computer is minimal at best.  Make things easier 
for the programmers.

Alan Stern

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

[David Cohen]

Hi Alan,

On 11/05/2013 07:38 AM, Alan Stern wrote:
> On Tue, 5 Nov 2013, David Cohen wrote:
> 
>>>> +		/*
>>>> +		 * Controller requires buffer size to be aligned to
>>>> +		 * maxpacketsize of an out endpoint.
>>>> +		 */
>>>> +		if (gadget->quirk_ep_out_aligned_size && read) {
>>>> +			/*
>>>> +			 * We pass 'orig_len' to usp_ep_align_maxpacketsize()
>>>> +			 * due to we're in a loop and 'len' may have been
>>>> +			 * changed.
>>>> +			 */
>>>> +			len = usb_ep_align_maxpacketsize(ep->ep, orig_len);
>>>> +			if (data && len > data_len) {
>>>> +				kfree(data);
>>>> +				data = NULL;
>>>> +				data_len = 0;
>>>> +			}
>>>> +		}
>>>
>>> Since the value of orig_len never changes, there's no point calling 
>>> usb_ep_align_maxpacketsize() inside the loop.  You should call it only 
>>> once, before the loop starts.  Once you do that, you won't need 
>>> orig_len at all.
>>
>> orig_len doesn't change but ep->ep does. If USB specs say max packet
>> size won't change even if ep does, than we can call it from outside the
>> loop.
> 
> I'm not too familiar with this driver.  It looks like the only way 
> ep->ep can change is if the endpoint gets enabled while you're sitting 
> inside the wait_event_interruptible() call.
> 
> In fact, the whole structure of that loop looks peculiar.  Why not
> acquire the mutex first and then do everything else?

I'm not 100% familiar with this driver too. I'd keep this change to
another patch.

> 
> Does it even make sense for ep to change?  Would this change be visible
> to the host?  What if the host changes the alternate setting while this
> loop is running -- does it make sense for the userspace program to
> start a read or write under one altsetting but then have the read/write
> take place under a different altsetting?

It doesn't make sense to do so, but gadget driver allows it. If we just
ignore, it would be a security or instability issue possible to xploit
(for DWC3 and any other controller which may depend on this quirk).

Br, David Cohen

Re: [PATCH v4 2/4] usb: gadget: add quirk_ep_out_aligned_size field to struct usb_gadget

[David Cohen]

On 11/05/2013 07:41 AM, Alan Stern wrote:
> On Tue, 5 Nov 2013, David Cohen wrote:
> 
>>>> +static inline size_t usb_ep_align_maxpacketsize(struct usb_ep *ep, size_t len)
>>>> +{
>>>> +	int aligned;
>>>> +
>>>> +	if (ep->desc->bmAttributes & USB_ENDPOINT_XFER_INT)
>>>> +		/*
>>>> +		 * Interrupt eps don't need max packet size to be power of 2,
>>>> +		 * so can't use cheap IS_ALIGNED() macro.
>>>> +		 */
>>>> +		aligned = !(len % ep->desc->wMaxPacketSize);
>>>> +	else
>>>> +		aligned = IS_ALIGNED(len, ep->desc->wMaxPacketSize);
>>>
>>> This isn't on a hot path, and I suspect that the extra "if" will 
>>> require nearly as much time as you save by not doing the division.  You 
>>> might as well always use the % operation.
>>
>> Perhaps if I use unlikely() on 'if' condition instead?
>> Anyway I'll double check the costs of if + IS_ALIGNED vs modulo.
> 
> You're missing the point.  You and I (not to mention anybody who ever
> reads this code in the future) have already wasted more time talking
> about it and trying to understand it than you will ever save by using
> IS_ALIGNED.
> 
> The difference to the computer is minimal at best.  Make things easier 
> for the programmers.

I don't see it as complex :)
But I'm fine with your proposal. I can send new patch dropping
IS_ALIGNED() case.

Br, David Cohen

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

[Alan Stern]

On Tue, 5 Nov 2013, David Cohen wrote:

> Hi Alan,
> 
> On 11/05/2013 07:38 AM, Alan Stern wrote:
> > On Tue, 5 Nov 2013, David Cohen wrote:
> > 
> >>>> +		/*
> >>>> +		 * Controller requires buffer size to be aligned to
> >>>> +		 * maxpacketsize of an out endpoint.
> >>>> +		 */
> >>>> +		if (gadget->quirk_ep_out_aligned_size && read) {
> >>>> +			/*
> >>>> +			 * We pass 'orig_len' to usp_ep_align_maxpacketsize()
> >>>> +			 * due to we're in a loop and 'len' may have been
> >>>> +			 * changed.
> >>>> +			 */
> >>>> +			len = usb_ep_align_maxpacketsize(ep->ep, orig_len);
> >>>> +			if (data && len > data_len) {
> >>>> +				kfree(data);
> >>>> +				data = NULL;
> >>>> +				data_len = 0;
> >>>> +			}
> >>>> +		}
> >>>
> >>> Since the value of orig_len never changes, there's no point calling 
> >>> usb_ep_align_maxpacketsize() inside the loop.  You should call it only 
> >>> once, before the loop starts.  Once you do that, you won't need 
> >>> orig_len at all.
> >>
> >> orig_len doesn't change but ep->ep does. If USB specs say max packet
> >> size won't change even if ep does, than we can call it from outside the
> >> loop.
> > 
> > I'm not too familiar with this driver.  It looks like the only way 
> > ep->ep can change is if the endpoint gets enabled while you're sitting 
> > inside the wait_event_interruptible() call.
> > 
> > In fact, the whole structure of that loop looks peculiar.  Why not
> > acquire the mutex first and then do everything else?
> 
> I'm not 100% familiar with this driver too. I'd keep this change to
> another patch.
> 
> > 
> > Does it even make sense for ep to change?  Would this change be visible
> > to the host?  What if the host changes the alternate setting while this
> > loop is running -- does it make sense for the userspace program to
> > start a read or write under one altsetting but then have the read/write
> > take place under a different altsetting?
> 
> It doesn't make sense to do so, but gadget driver allows it. If we just
> ignore, it would be a security or instability issue possible to xploit
> (for DWC3 and any other controller which may depend on this quirk).

Maybe Michal can enlighten us.

Alan Stern

Re: [PATCH v4 2/4] usb: gadget: add quirk_ep_out_aligned_size field to struct usb_gadget

[David Cohen]

On 11/05/2013 10:13 AM, David Cohen wrote:
> On 11/05/2013 07:41 AM, Alan Stern wrote:
>> On Tue, 5 Nov 2013, David Cohen wrote:
>>
>>>>> +static inline size_t usb_ep_align_maxpacketsize(struct usb_ep *ep, size_t len)
>>>>> +{
>>>>> +	int aligned;
>>>>> +
>>>>> +	if (ep->desc->bmAttributes & USB_ENDPOINT_XFER_INT)
>>>>> +		/*
>>>>> +		 * Interrupt eps don't need max packet size to be power of 2,
>>>>> +		 * so can't use cheap IS_ALIGNED() macro.
>>>>> +		 */
>>>>> +		aligned = !(len % ep->desc->wMaxPacketSize);
>>>>> +	else
>>>>> +		aligned = IS_ALIGNED(len, ep->desc->wMaxPacketSize);
>>>>
>>>> This isn't on a hot path, and I suspect that the extra "if" will
>>>> require nearly as much time as you save by not doing the division.  You
>>>> might as well always use the % operation.
>>>
>>> Perhaps if I use unlikely() on 'if' condition instead?
>>> Anyway I'll double check the costs of if + IS_ALIGNED vs modulo.
>>
>> You're missing the point.  You and I (not to mention anybody who ever
>> reads this code in the future) have already wasted more time talking
>> about it and trying to understand it than you will ever save by using
>> IS_ALIGNED.
>>
>> The difference to the computer is minimal at best.  Make things easier
>> for the programmers.
>
> I don't see it as complex :)
> But I'm fine with your proposal. I can send new patch dropping
> IS_ALIGNED() case.

At a second though, it's even better to drop both 'if' cases.
I can just return round_up(...) directly.

Br, David

[PATCH v4.1 2/4] 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>
---

Changes from v4 to v4.1:
 - Simplified usb_ep_align_maxpacketsize() macro as per Alen Stern's request

 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

Re: [PATCH v4.1 2/4] usb: gadget: add quirk_ep_out_aligned_size field to struct usb_gadget

[Alan Stern]

On Tue, 5 Nov 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>
> ---
> 
> Changes from v4 to v4.1:
>  - Simplified usb_ep_align_maxpacketsize() macro as per Alen Stern's request
> 
>  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))

This looks good now.

Acked-by: Alan Stern <stern@rowland.harvard.edu>

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

[Michal Nazarewicz]

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

On Tue, Nov 05 2013, Alan Stern wrote:
> Maybe Michal can enlighten us.

Sorry for late response, this thread fell under my radar for some
reason.

So here's how it works:

epfile represents an end point file on the fuctionfs file system,
i.e. what user space is seeing.  It's numbering is independent of which
USB configuration is chosen.

A FunctionFS user space daemon may read/write to those files regardless
of whether the function is enabled.  If it is not, the operation will
block until host enables the function.

epfile->ep represents an actual USB end point, and it's number does not
have to correspond to the epfile file name, and may be different in
different configuration.  FunctionFS hides all that from the user space
daemon.

epfile->ep is set when host changes configuration (i.e. function's
set_alt or disable callbacks).  IIRC this implies that epfile->ep cannot
be protected by a mutex, and therefore is protected by a spinlock.

Since it is protected by a spinlock, the ffs_epfile_io function cannot
lock it and then proceed to allocating memory and copying data from user
space.  That's why there is the need for the loop since there is no way
to guarantee that while the memory was allocated and data was read from
user space (if it is a write), the function has not been disabled and
re-enabled.

However, I'm not sure whether maxpacketsize can change.  It is part of
endpoint's descriptor and even though the endpoint number can change
while ffs_epfile_io is running, all the other descriptor fields should
stay the same.

-- 
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 v4 3/4] usb: ffs: check quirk to pad epout buf size when not aligned to maxpacketsize

[Alan Stern]

On Wed, 6 Nov 2013, Michal Nazarewicz wrote:

> On Tue, Nov 05 2013, Alan Stern wrote:
> > Maybe Michal can enlighten us.
> 
> Sorry for late response, this thread fell under my radar for some
> reason.
> 
> So here's how it works:
> 
> epfile represents an end point file on the fuctionfs file system,
> i.e. what user space is seeing.  It's numbering is independent of which
> USB configuration is chosen.
> 
> A FunctionFS user space daemon may read/write to those files regardless
> of whether the function is enabled.  If it is not, the operation will
> block until host enables the function.

What happens if the userspace daemon writes to epfile but the host 
changes the config or altsetting before all the data can be sent?  Does 
the remaining data get flushed?

Similarly, what happens if the config or altsetting changes before a 
read of epfile completes?  Does the read get terminated?

> epfile->ep represents an actual USB end point, and it's number does not
> have to correspond to the epfile file name, and may be different in
> different configuration.  FunctionFS hides all that from the user space
> daemon.
> 
> epfile->ep is set when host changes configuration (i.e. function's
> set_alt or disable callbacks).  IIRC this implies that epfile->ep cannot
> be protected by a mutex, and therefore is protected by a spinlock.
> 
> Since it is protected by a spinlock, the ffs_epfile_io function cannot
> lock it and then proceed to allocating memory and copying data from user
> space.  That's why there is the need for the loop since there is no way
> to guarantee that while the memory was allocated and data was read from
> user space (if it is a write), the function has not been disabled and
> re-enabled.

I'm still a little unclear on this.  Disabling the function ought to
have much the same effect as changing the config or altsetting: Writes
to endpoint files should be flushed and reads should be terminated.  
Otherwise you would end up sending stale data to the host or reading 
data that the daemon isn't prepared for.

Given that, there doesn't seem to be any need for a loop.  Copy the 
data; if the function was disabled in the meantime then throw away the 
data and return an appropriate error code.

I don't see any reason why you should ever have to copy the same data 
from userspace multiple times.

> However, I'm not sure whether maxpacketsize can change.  It is part of
> endpoint's descriptor and even though the endpoint number can change
> while ffs_epfile_io is running, all the other descriptor fields should
> stay the same.

If the config or altsetting can change then the maxpacket size can 
change too.  However, the reasoning above indicates that this should be 
moot.

Alan Stern

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

[Michal Nazarewicz]

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

On Thu, Nov 07 2013, Alan Stern wrote:
> What happens if the userspace daemon writes to epfile but the host 
> changes the config or altsetting before all the data can be sent?  Does 
> the remaining data get flushed?

Each read and write is mapped to a single request, so the usual.

> I'm still a little unclear on this.  Disabling the function ought to
> have much the same effect as changing the config or altsetting: Writes
> to endpoint files should be flushed and reads should be terminated.  
> Otherwise you would end up sending stale data to the host or reading 
> data that the daemon isn't prepared for.

You may have a point here.  I'll try to prepare a patch over the weekend.

> Given that, there doesn't seem to be any need for a loop.  Copy the 
> data; if the function was disabled in the meantime then throw away the 
> data and return an appropriate error code.

Right.

> I don't see any reason why you should ever have to copy the same data 
> from userspace multiple times.

That actually never happens.  The data is copied at most once.

-- 
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 v4 3/4] usb: ffs: check quirk to pad epout buf size when not aligned to maxpacketsize

[David Cohen]

Hi Michal,

On 11/08/2013 04:23 AM, Michal Nazarewicz wrote:
> On Thu, Nov 07 2013, Alan Stern wrote:
>> What happens if the userspace daemon writes to epfile but the host
>> changes the config or altsetting before all the data can be sent?  Does
>> the remaining data get flushed?
>
> Each read and write is mapped to a single request, so the usual.
>
>> I'm still a little unclear on this.  Disabling the function ought to
>> have much the same effect as changing the config or altsetting: Writes
>> to endpoint files should be flushed and reads should be terminated.
>> Otherwise you would end up sending stale data to the host or reading
>> data that the daemon isn't prepared for.
>
> You may have a point here.  I'll try to prepare a patch over the weekend.

It looks like our patches are going to have dependence.
If you send yours this weekend, I'll wait to send new version of this
patch of mine on top of yours.

Br, David Cohen

[PATCH 1/2] usb: gadget: f_fs: remove loop from I/O function

[Michal Nazarewicz]

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>
---
 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 44cf775..f875f26 100644
--- a/drivers/usb/gadget/f_fs.c
+++ b/drivers/usb/gadget/f_fs.c
@@ -754,74 +754,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)) {
@@ -856,6 +843,7 @@ first_try:
 		}
 	}

+error_unlock:
 	mutex_unlock(&epfile->mutex);
 error:
 	kfree(data);
--
1.8.3.2

[PATCH 2/2] check quirk to pad epout buf size when not aligned to maxpacketsize

[Michal Nazarewicz]

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>
---
 I'm wondering whether the len should be aligned down rather then up.
 This would have it's own problems, but maybe better then
 a possibility of silently dropping data.

 drivers/usb/gadget/f_fs.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/gadget/f_fs.c b/drivers/usb/gadget/f_fs.c
index f875f26..ea0b8ba 100644
--- a/drivers/usb/gadget/f_fs.c
+++ b/drivers/usb/gadget/f_fs.c
@@ -753,8 +753,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;
 
@@ -787,6 +788,13 @@ static ssize_t ffs_epfile_io(struct file *file,
 
 	/* Allocate & copy */
 	if (!halt) {
+		/*
+		 * 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(len, GFP_KERNEL);
 		if (unlikely(!data))
 			return -ENOMEM;
@@ -824,7 +832,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);
 
@@ -836,9 +844,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.3.2

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

[David Cohen]

Hi Michal,

On 11/10/2013 08:50 AM, Michal Nazarewicz wrote:
> 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>
> ---
>  I'm wondering whether the len should be aligned down rather then up.
>  This would have it's own problems, but maybe better then
>  a possibility of silently dropping data.
> 
>  drivers/usb/gadget/f_fs.c | 21 ++++++++++++++++++---
>  1 file changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/gadget/f_fs.c b/drivers/usb/gadget/f_fs.c
> index f875f26..ea0b8ba 100644
> --- a/drivers/usb/gadget/f_fs.c
> +++ b/drivers/usb/gadget/f_fs.c
> @@ -753,8 +753,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;
>  
> @@ -787,6 +788,13 @@ static ssize_t ffs_epfile_io(struct file *file,
>  
>  	/* Allocate & copy */
>  	if (!halt) {
> +		/*
> +		 * 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(len, GFP_KERNEL);

Shouldn't this kmalloc() allocate 'data_len' bytes, instead of 'len'?

Br, David

[PATCHv2 2/2] 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 | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

> On 11/10/2013 08:50 AM, Michal Nazarewicz wrote:
>> @@ -787,6 +788,13 @@ static ssize_t ffs_epfile_io(struct file *file,
>>  
>>  	/* Allocate & copy */
>>  	if (!halt) {
>> +		/*
>> +		 * 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(len, GFP_KERNEL);

On Mon, Nov 11 2013, David Cohen <david.a.cohen@linux.intel.com> wrote:
> Shouldn't this kmalloc() allocate 'data_len' bytes, instead of 'len'?

Yes, of coures.

diff --git a/drivers/usb/gadget/f_fs.c b/drivers/usb/gadget/f_fs.c
index e496b72..fd769a8 100644
--- a/drivers/usb/gadget/f_fs.c
+++ b/drivers/usb/gadget/f_fs.c
@@ -753,8 +753,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;
 
@@ -787,7 +788,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;
 
@@ -824,7 +832,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);
 
@@ -836,9 +844,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.1

Re: [PATCHv2 2/2] check quirk to pad epout buf size when not aligned to maxpacketsize

[David Cohen]

On 11/11/2013 03:21 AM, 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 | 23 +++++++++++++++++++----
>   1 file changed, 19 insertions(+), 4 deletions(-)
>
>> On 11/10/2013 08:50 AM, Michal Nazarewicz wrote:
>>> @@ -787,6 +788,13 @@ static ssize_t ffs_epfile_io(struct file *file,
>>>
>>>   	/* Allocate & copy */
>>>   	if (!halt) {
>>> +		/*
>>> +		 * 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(len, GFP_KERNEL);
>
> On Mon, Nov 11 2013, David Cohen <david.a.cohen@linux.intel.com> wrote:
>> Shouldn't this kmalloc() allocate 'data_len' bytes, instead of 'len'?
>
> Yes, of coures.

Thanks. It looks fine. I'll test these patches now.

But the whole series became messy with too many amends. If you don't 
mind, I'll send a v5 of my patch set including my v4.1 patches + your 2
ones following the correct sequence.

Br, David Cohen

Re: [PATCH 1/2] usb: gadget: f_fs: remove loop from I/O function

[David Cohen]

Hi Michal,

On 11/10/2013 08:50 AM, Michal Nazarewicz 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>
> ---
>   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 44cf775..f875f26 100644
> --- a/drivers/usb/gadget/f_fs.c
> +++ b/drivers/usb/gadget/f_fs.c
> @@ -754,74 +754,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))) {

FYI this line fails checkpatch:
ERROR: do not use assignment in if condition
#70: FILE: drivers/usb/gadget/f_fs.c:777:
+		if (wait_event_interruptible(epfile->wait, (ep = epfile->ep))) {

total: 1 errors, 0 warnings, 121 lines checked

Since you're just moving that line here, you may want (or not) to clean
this up in a new patch for 3.13.

Br, David Cohen

Re: [PATCHv2 2/2] check quirk to pad epout buf size when not aligned to maxpacketsize

[Alan Stern]

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

I think this is still wrong.

> @@ -787,7 +788,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;
>  
> @@ -824,7 +832,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;

IIUC, req->length should still be set to len, not to data_len.

>  
>  		ret = usb_ep_queue(ep->ep, req, GFP_ATOMIC);
>  
> @@ -836,9 +844,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.
> +			 */

Then this will never come up.  If the host sends a packet that's too 
long, you'll get a -EOVERFLOW error.

>  			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;
>  		}
>  	}

The reason for the quirk is that the controller may write all the 
incoming data to the buffer, even if this is more data than the driver 
requested.

Alan Stern

Re: [PATCHv2 2/2] check quirk to pad epout buf size when not aligned to maxpacketsize

[Michal Nazarewicz]

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

On Mon, Nov 11 2013, Alan Stern wrote:
> On Mon, 11 Nov 2013, 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>
>
> I think this is still wrong.
>
>> @@ -824,7 +832,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;
>
> IIUC, req->length should still be set to len, not to data_len.
>
>>  
>>  		ret = usb_ep_queue(ep->ep, req, GFP_ATOMIC);
>>  
>> @@ -836,9 +844,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.
>> +			 */
>
> Then this will never come up.  If the host sends a packet that's too 
> long, you'll get a -EOVERFLOW error.
>
>>  			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;
>>  		}
>>  	}
>
> The reason for the quirk is that the controller may write all the 
> incoming data to the buffer, even if this is more data than the driver 
> requested.

If that's the case, then it indeed solves the problem of silently
throwing away data.  I guess it makes more sense then my understanding
of the quirk.

-- 
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: [PATCHv2 2/2] check quirk to pad epout buf size when not aligned to maxpacketsize

[Michal Nazarewicz]

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

On Mon, Nov 11 2013, David Cohen wrote:
> But the whole series became messy with too many amends. If you don't 
> mind, I'll send a v5 of my patch set including my v4.1 patches + your 2
> ones following the correct sequence.

Please do, but as Alan pointed out my second patch needs some fixes,
namely the last two hunks are not needed.  If you want I can resend the
patch, or you could just drop them by yourself.

-- 
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 1/2] usb: gadget: f_fs: remove loop from I/O function

[Michal Nazarewicz]

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

> On 11/10/2013 08:50 AM, Michal Nazarewicz wrote:
>> +		if (wait_event_interruptible(epfile->wait, (ep = epfile->ep))) {

On Mon, Nov 11 2013, David Cohen wrote:
> FYI this line fails checkpatch:
> ERROR: do not use assignment in if condition
> #70: FILE: drivers/usb/gadget/f_fs.c:777:
> +		if (wait_event_interruptible(epfile->wait, (ep = epfile->ep))) {
>
> total: 1 errors, 0 warnings, 121 lines checked

This is intentional.

-- 
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: [PATCHv2 2/2] check quirk to pad epout buf size when not aligned to maxpacketsize

[David Cohen]

Hi Alan, Michal,

On 11/11/2013 01:09 PM, Michal Nazarewicz wrote:
> On Mon, Nov 11 2013, Alan Stern wrote:
>> On Mon, 11 Nov 2013, 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>
>>
>> I think this is still wrong.
>>
>>> @@ -824,7 +832,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;
>>
>> IIUC, req->length should still be set to len, not to data_len.

I misunderstood the first time I read it:
In order to avoid DWC3 to stall, we need to update req->length (this is
the most important fix). kmalloc() is updated too to prevent USB
controller to overflow buffer boundaries.

>>
>>>
>>>   		ret = usb_ep_queue(ep->ep, req, GFP_ATOMIC);
>>>
>>> @@ -836,9 +844,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.
>>> +			 */
>>
>> Then this will never come up.  If the host sends a packet that's too
>> long, you'll get a -EOVERFLOW error.
>>
>>>   			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;
>>>   		}
>>>   	}
>>
>> The reason for the quirk is that the controller may write all the
>> incoming data to the buffer, even if this is more data than the driver
>> requested.
>
> If that's the case, then it indeed solves the problem of silently
> throwing away data.  I guess it makes more sense then my understanding
> of the quirk.

The main reason of this quirk it to prevent DWC3 to stall or to
overflow buffer after usb_ep_queue() above. Since req->length has to be
updated, I disagree with Alan in this case and give my ack to this
Michal's approach.

Br, David Cohen

Re: [PATCH 1/2] usb: gadget: f_fs: remove loop from I/O function

[David Cohen]

Hi Michal,

On 11/10/2013 08:50 AM, Michal Nazarewicz 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>
> ---
>   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 44cf775..f875f26 100644
> --- a/drivers/usb/gadget/f_fs.c
> +++ b/drivers/usb/gadget/f_fs.c
> @@ -754,74 +754,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;

You can't non-initialize data, otherwise we'll end up with this correct
warning:
drivers/usb/gadget/f_fs.c:866:7: warning: 'data' may be used 
uninitialized in this function [-Wmaybe-uninitialized]

If you agree, you can send v5.1 to my 5th patch set (or let me handle
it).

Br, David

Re: [PATCHv2 2/2] check quirk to pad epout buf size when not aligned to maxpacketsize

[David Cohen]

On 11/11/2013 03:21 AM, 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 | 23 +++++++++++++++++++----
>   1 file changed, 19 insertions(+), 4 deletions(-)
>
>> On 11/10/2013 08:50 AM, Michal Nazarewicz wrote:
>>> @@ -787,6 +788,13 @@ static ssize_t ffs_epfile_io(struct file *file,
>>>
>>>   	/* Allocate & copy */
>>>   	if (!halt) {
>>> +		/*
>>> +		 * 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(len, GFP_KERNEL);
>
> On Mon, Nov 11 2013, David Cohen <david.a.cohen@linux.intel.com> wrote:
>> Shouldn't this kmalloc() allocate 'data_len' bytes, instead of 'len'?
>
> Yes, of coures.
>
> diff --git a/drivers/usb/gadget/f_fs.c b/drivers/usb/gadget/f_fs.c
> index e496b72..fd769a8 100644
> --- a/drivers/usb/gadget/f_fs.c
> +++ b/drivers/usb/gadget/f_fs.c

[snip]

>   			ret = ep->status;
>   			if (read && ret > 0 &&
> -			    unlikely(copy_to_user(buf, data, ret)))
> +			    unlikely(copy_to_user(buf, data, min(ret, len))))

You need to replace min(ret, len) by min_t(size_t, ret, len) to avoid
this warning:
drivers/usb/gadget/f_fs.c:858:124: warning: comparison of distinct 
pointer types lacks a cast [enabled by default]

Once again, you (or me) need to reply to my v5 patch set.

Br, David

Re: [PATCHv2 2/2] check quirk to pad epout buf size when not aligned to maxpacketsize

[Alan Stern]

On Mon, 11 Nov 2013, David Cohen wrote:

> Hi Alan, Michal,
> 
> On 11/11/2013 01:09 PM, Michal Nazarewicz wrote:
> > On Mon, Nov 11 2013, Alan Stern wrote:
> >> On Mon, 11 Nov 2013, 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>
> >>
> >> I think this is still wrong.
> >>
> >>> @@ -824,7 +832,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;
> >>
> >> IIUC, req->length should still be set to len, not to data_len.
> 
> I misunderstood the first time I read it:
> In order to avoid DWC3 to stall, we need to update req->length (this is
> the most important fix). kmalloc() is updated too to prevent USB
> controller to overflow buffer boundaries.

Here I disagree.

If the DWC3 hardware stalls, it is up to the DWC3 UDC driver to fix it.  
Gadget drivers should not have to worry.  Most especially, gadget 
drivers should not lie about a request length.

If the UDC driver decides to round up req->length before sending it to
the hardware, that's okay.  But req->length should be set to len, not
data_len.  And if the hardware receives more than len bytes of data,
the UDC driver should set req->status to -EOVERFLOW.

Alan Stern

Re: [PATCHv2 2/2] check quirk to pad epout buf size when not aligned to maxpacketsize

[David Cohen]

>>>> IIUC, req->length should still be set to len, not to data_len.
>>
>> I misunderstood the first time I read it:
>> In order to avoid DWC3 to stall, we need to update req->length (this is
>> the most important fix). kmalloc() is updated too to prevent USB
>> controller to overflow buffer boundaries.
> 
> Here I disagree.
> 
> If the DWC3 hardware stalls, it is up to the DWC3 UDC driver to fix it.  
> Gadget drivers should not have to worry.  Most especially, gadget 
> drivers should not lie about a request length.
> 
> If the UDC driver decides to round up req->length before sending it to
> the hardware, that's okay.  But req->length should be set to len, not
> data_len.  And if the hardware receives more than len bytes of data,
> the UDC driver should set req->status to -EOVERFLOW.

Got your point. As long as buffer allocation has enough size, DWC3 is
able to fix itself. I'm fine with this approach too. But in this case I
will need to redo my DWC3 patch and my validation.

Br, David

> 
> Alan Stern
> 

RE: [PATCHv2 2/2] check quirk to pad epout buf size when not aligned to maxpacketsize

[Paul Zimmerman]

> From: linux-usb-owner@vger.kernel.org [mailto:linux-usb-owner@vger.kernel.org] On Behalf Of Alan Stern
> Sent: Tuesday, November 12, 2013 7:51 AM
> 
> On Mon, 11 Nov 2013, David Cohen wrote:
> 
> > Hi Alan, Michal,
> >
> > On 11/11/2013 01:09 PM, Michal Nazarewicz wrote:
> > > On Mon, Nov 11 2013, Alan Stern wrote:
> > >> On Mon, 11 Nov 2013, 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>
> > >>
> > >> I think this is still wrong.
> > >>
> > >>> @@ -824,7 +832,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;
> > >>
> > >> IIUC, req->length should still be set to len, not to data_len.
> >
> > I misunderstood the first time I read it:
> > In order to avoid DWC3 to stall, we need to update req->length (this is
> > the most important fix). kmalloc() is updated too to prevent USB
> > controller to overflow buffer boundaries.
> 
> Here I disagree.
> 
> If the DWC3 hardware stalls, it is up to the DWC3 UDC driver to fix it.
> Gadget drivers should not have to worry.  Most especially, gadget
> drivers should not lie about a request length.

The whole point of this quirk is to lie to the DWC3 driver. The quirk is
only enabled for the DWC3 driver.

> If the UDC driver decides to round up req->length before sending it to
> the hardware, that's okay.

Not really. If the DWC3 driver unconditionally rounds up req->length,
then in the case where the buffer has not been expanded to a multiple of
maxpacket, by this quirk or otherwise, there is the potential to write
beyond the end of the allocation.

If we do what you suggest, then all the gadgets will have to be audited
to make sure an OUT buffer with a non-aligned length is never passed to
the DWC3 driver.

I still think that's the best solution anyway. Just make that the rule,
and then there is no need for this quirk at all. And there is no need to
round up req->length in the DWC3 driver either.

> But req->length should be set to len, not data_len.

According to gadget.h:

	@buf: Buffer used for data
	@length: Length of that data

So why shouldn't length be the length of the allocated data buffer?
Remember, this is for the OUT direction only, so the host has control
over how much data is actually sent. You could even argue that an OUT
data buffer should always be a multiple of the max packet size, given
how USB works.

> And if the hardware receives more than len bytes of data,
> the UDC driver should set req->status to -EOVERFLOW.

That would be nice, but I don't see how to accomplish that given the
above.

-- 
Paul

Re: [PATCHv2 2/2] check quirk to pad epout buf size when not aligned to maxpacketsize

[David Cohen]

Hi Paul,

On 11/12/2013 03:09 PM, Paul Zimmerman wrote:
>> From: linux-usb-owner@vger.kernel.org [mailto:linux-usb-owner@vger.kernel.org] On Behalf Of Alan Stern
>> Sent: Tuesday, November 12, 2013 7:51 AM
>>
>> On Mon, 11 Nov 2013, David Cohen wrote:
>>
>>> Hi Alan, Michal,
>>>
>>> On 11/11/2013 01:09 PM, Michal Nazarewicz wrote:
>>>> On Mon, Nov 11 2013, Alan Stern wrote:
>>>>> On Mon, 11 Nov 2013, 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>
>>>>>
>>>>> I think this is still wrong.
>>>>>
>>>>>> @@ -824,7 +832,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;
>>>>>
>>>>> IIUC, req->length should still be set to len, not to data_len.
>>>
>>> I misunderstood the first time I read it:
>>> In order to avoid DWC3 to stall, we need to update req->length (this is
>>> the most important fix). kmalloc() is updated too to prevent USB
>>> controller to overflow buffer boundaries.
>>
>> Here I disagree.
>>
>> If the DWC3 hardware stalls, it is up to the DWC3 UDC driver to fix it.
>> Gadget drivers should not have to worry.  Most especially, gadget
>> drivers should not lie about a request length.
>
> The whole point of this quirk is to lie to the DWC3 driver. The quirk is
> only enabled for the DWC3 driver.
>
>> If the UDC driver decides to round up req->length before sending it to
>> the hardware, that's okay.
>
> Not really. If the DWC3 driver unconditionally rounds up req->length,
> then in the case where the buffer has not been expanded to a multiple of
> maxpacket, by this quirk or otherwise, there is the potential to write
> beyond the end of the allocation.
>
> If we do what you suggest, then all the gadgets will have to be audited
> to make sure an OUT buffer with a non-aligned length is never passed to
> the DWC3 driver.

I was really convinced about not updating rep->length was a better idea
until I read this argument of yours: with this approach buffer overflow
can silently happen.

>
> I still think that's the best solution anyway. Just make that the rule,
> and then there is no need for this quirk at all. And there is no need to
> round up req->length in the DWC3 driver either.

I'd have nothing against that, but I'm not sure how it would affect
other environments, given embedded environments are pretty sensitive to
memory consumption (and performance).

>
>> But req->length should be set to len, not data_len.
>
> According to gadget.h:
>
> 	@buf: Buffer used for data
> 	@length: Length of that data
>
> So why shouldn't length be the length of the allocated data buffer?
> Remember, this is for the OUT direction only, so the host has control
> over how much data is actually sent. You could even argue that an OUT
> data buffer should always be a multiple of the max packet size, given
> how USB works.

How about something like this, to let USB controllers to know about
whole allocated memory and avoid hidden overflows. Does that make sense
to you?

diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
index 25a5007f92e3..973b57b709ab 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -31,13 +31,16 @@ struct usb_ep;
   * struct usb_request - describes one i/o request
   * @buf: Buffer used for data.  Always provide this; some controllers
   *     only use PIO, or don't use DMA for some endpoints.
+ * @length: Length of that data
+ * @buf_pad: Some USB controllers may need to pad buffer size due to 
alignment
+ *     constraints. This keeps track of how much memory was allocated 
to @buf
+ *     in addition to @length.
   * @dma: DMA address corresponding to 'buf'.  If you don't set this
   *     field, and the usb controller needs one, it is responsible
   *     for mapping and unmapping the buffer.
   * @sg: a scatterlist for SG-capable controllers.
   * @num_sgs: number of SG entries
   * @num_mapped_sgs: number of SG entries mapped to DMA (internal)
- * @length: Length of that data
   * @stream_id: The stream id, when USB3.0 bulk streams are being used
   * @no_interrupt: If true, hints that no completion irq is needed.
   *     Helpful sometimes with deep request queues that are handled
@@ -91,6 +94,7 @@ struct usb_ep;
  struct usb_request {
         void                    *buf;
         unsigned                length;
+       unsigned                buf_pad;
         dma_addr_t              dma;

         struct scatterlist      *sg;


Br, David Cohen

RE: [PATCHv2 2/2] check quirk to pad epout buf size when not aligned to maxpacketsize

[Paul Zimmerman]

> From: David Cohen [mailto:david.a.cohen@linux.intel.com]
> Sent: Tuesday, November 12, 2013 3:44 PM
> 
> On 11/12/2013 03:09 PM, Paul Zimmerman wrote:
> 
> >> From: linux-usb-owner@vger.kernel.org [mailto:linux-usb-owner@vger.kernel.org] On Behalf Of Alan Stern
> >> Sent: Tuesday, November 12, 2013 7:51 AM
> >>
> >> On Mon, 11 Nov 2013, David Cohen wrote:
> >>
> >>> In order to avoid DWC3 to stall, we need to update req->length (this is
> >>> the most important fix). kmalloc() is updated too to prevent USB
> >>> controller to overflow buffer boundaries.
> >>
> >> Here I disagree.
> >>
> >> If the DWC3 hardware stalls, it is up to the DWC3 UDC driver to fix it.
> >> Gadget drivers should not have to worry.  Most especially, gadget
> >> drivers should not lie about a request length.
> >
> > The whole point of this quirk is to lie to the DWC3 driver. The quirk is
> > only enabled for the DWC3 driver.
> >
> >> If the UDC driver decides to round up req->length before sending it to
> >> the hardware, that's okay.
> >
> > Not really. If the DWC3 driver unconditionally rounds up req->length,
> > then in the case where the buffer has not been expanded to a multiple of
> > maxpacket, by this quirk or otherwise, there is the potential to write
> > beyond the end of the allocation.
> >
> > If we do what you suggest, then all the gadgets will have to be audited
> > to make sure an OUT buffer with a non-aligned length is never passed to
> > the DWC3 driver.
> 
> I was really convinced about not updating rep->length was a better idea
> until I read this argument of yours: with this approach buffer overflow
> can silently happen.
> 
> >
> > I still think that's the best solution anyway. Just make that the rule,
> > and then there is no need for this quirk at all. And there is no need to
> > round up req->length in the DWC3 driver either.
> 
> I'd have nothing against that, but I'm not sure how it would affect
> other environments, given embedded environments are pretty sensitive to
> memory consumption (and performance).

There would be no performance impact. The memory impact would be less than
1024 bytes per OUT ep (worst case), and most gadget devices only have a
couple of OUT eps. I don't think it's a problem.

> >
> >> But req->length should be set to len, not data_len.
> >
> > According to gadget.h:
> >
> > 	@buf: Buffer used for data
> > 	@length: Length of that data
> >
> > So why shouldn't length be the length of the allocated data buffer?
> > Remember, this is for the OUT direction only, so the host has control
> > over how much data is actually sent. You could even argue that an OUT
> > data buffer should always be a multiple of the max packet size, given
> > how USB works.
> 
> How about something like this, to let USB controllers to know about
> whole allocated memory and avoid hidden overflows. Does that make sense
> to you?
> 
> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
> index 25a5007f92e3..973b57b709ab 100644
> --- a/include/linux/usb/gadget.h
> +++ b/include/linux/usb/gadget.h
> @@ -31,13 +31,16 @@ struct usb_ep;
>    * struct usb_request - describes one i/o request
>    * @buf: Buffer used for data.  Always provide this; some controllers
>    *     only use PIO, or don't use DMA for some endpoints.
> + * @length: Length of that data
> + * @buf_pad: Some USB controllers may need to pad buffer size due to
> alignment
> + *     constraints. This keeps track of how much memory was allocated
> to @buf
> + *     in addition to @length.
>    * @dma: DMA address corresponding to 'buf'.  If you don't set this
>    *     field, and the usb controller needs one, it is responsible
>    *     for mapping and unmapping the buffer.
>    * @sg: a scatterlist for SG-capable controllers.
>    * @num_sgs: number of SG entries
>    * @num_mapped_sgs: number of SG entries mapped to DMA (internal)
> - * @length: Length of that data
>    * @stream_id: The stream id, when USB3.0 bulk streams are being used
>    * @no_interrupt: If true, hints that no completion irq is needed.
>    *     Helpful sometimes with deep request queues that are handled
> @@ -91,6 +94,7 @@ struct usb_ep;
>   struct usb_request {
>          void                    *buf;
>          unsigned                length;
> +       unsigned                buf_pad;
>          dma_addr_t              dma;
> 
>          struct scatterlist      *sg;

Yes, I think that would work. But you only need 11 bits or so for
buf_pad, so make it 'unsigned buf_pad:11" and move it down with the
rest of the bitfields.

And you could also report -EOVERFLOW in the DWC3 driver, as Alan
suggested, if the received data is more than 'length'.

-- 
Paul

RE: [PATCHv2 2/2] check quirk to pad epout buf size when not aligned to maxpacketsize

[Alan Stern]

On Tue, 12 Nov 2013, Paul Zimmerman wrote:

> > > >>> @@ -824,7 +832,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;
> > > >>
> > > >> IIUC, req->length should still be set to len, not to data_len.
> > >
> > > I misunderstood the first time I read it:
> > > In order to avoid DWC3 to stall, we need to update req->length (this is
> > > the most important fix). kmalloc() is updated too to prevent USB
> > > controller to overflow buffer boundaries.
> > 
> > Here I disagree.
> > 
> > If the DWC3 hardware stalls, it is up to the DWC3 UDC driver to fix it.
> > Gadget drivers should not have to worry.  Most especially, gadget
> > drivers should not lie about a request length.
> 
> The whole point of this quirk is to lie to the DWC3 driver.

No.  The whole point of this quirk is to allow the DWC3 hardware to run 
without stalling.

It's dumb to lie to drivers.  They are there to help you; how can they
help you if you don't tell them what you want them to do?

>  The quirk is
> only enabled for the DWC3 driver.

So what?  Maybe other drivers will need it in the future.

> > If the UDC driver decides to round up req->length before sending it to
> > the hardware, that's okay.
> 
> Not really. If the DWC3 driver unconditionally rounds up req->length,
> then in the case where the buffer has not been expanded to a multiple of
> maxpacket, by this quirk or otherwise, there is the potential to write
> beyond the end of the allocation.

Indeed.  It is the gadget driver's responsibility to make sure that 
doesn't happen.

> If we do what you suggest, then all the gadgets will have to be audited
> to make sure an OUT buffer with a non-aligned length is never passed to
> the DWC3 driver.

Certainly.  And since gadget drivers have no idea (or, _should_ have no
idea) which UDC driver they are using, this means all gadgets will have
to make sure that they _never_ use OUT buffers with non-aligned
lengths.

> I still think that's the best solution anyway. Just make that the rule,
> and then there is no need for this quirk at all. And there is no need to
> round up req->length in the DWC3 driver either.

I agree that there really is no need for the quirk flag.  As for what
the DWC3 driver does internally, I couldn't care less.  But gadget
drivers should not round up req->length.

> > But req->length should be set to len, not data_len.
> 
> According to gadget.h:
> 
> 	@buf: Buffer used for data
> 	@length: Length of that data
> 
> So why shouldn't length be the length of the allocated data buffer?

Read what you just quoted: @length is the length of the _data_.  Not
the length of the _buffer_.

If the gadget driver wants to receive 100 bytes of data and the buffer
is 128 bytes long, it should set req->length to 100, not 128.  By doing
so, it tells the UDC driver that receiving more than 100 bytes is an
error condition.

> Remember, this is for the OUT direction only, so the host has control
> over how much data is actually sent. You could even argue that an OUT
> data buffer should always be a multiple of the max packet size, given
> how USB works.

Yes, you could.  I have no objection to changing the gadget API so that 
this becomes a firm requirement.

> > And if the hardware receives more than len bytes of data,
> > the UDC driver should set req->status to -EOVERFLOW.
> 
> That would be nice, but I don't see how to accomplish that given the
> above.

Why not?  The UDC driver knows what was originally in req->length and 
it knows how much data the hardware actually received.  All it has to 
do is:

	if (actual_length > req->length)
		req->status = -EOVERFLOW;

Alan Stern

Re: [PATCHv2 2/2] check quirk to pad epout buf size when not aligned to maxpacketsize

[David Cohen]

On 11/13/2013 07:52 AM, Alan Stern wrote:
> On Tue, 12 Nov 2013, Paul Zimmerman wrote:
>
>>>>>>> @@ -824,7 +832,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;
>>>>>>
>>>>>> IIUC, req->length should still be set to len, not to data_len.
>>>>
>>>> I misunderstood the first time I read it:
>>>> In order to avoid DWC3 to stall, we need to update req->length (this is
>>>> the most important fix). kmalloc() is updated too to prevent USB
>>>> controller to overflow buffer boundaries.
>>>
>>> Here I disagree.
>>>
>>> If the DWC3 hardware stalls, it is up to the DWC3 UDC driver to fix it.
>>> Gadget drivers should not have to worry.  Most especially, gadget
>>> drivers should not lie about a request length.
>>
>> The whole point of this quirk is to lie to the DWC3 driver.
>
> No.  The whole point of this quirk is to allow the DWC3 hardware to run
> without stalling.
>
> It's dumb to lie to drivers.  They are there to help you; how can they
> help you if you don't tell them what you want them to do?

IMO if we're not changing req->length we may need to add pad
information to struct usb_request to avoid hidden buffer overflows.
Even though we say it's UDC's responsibility of configure correctly
itself the buffer comes from gadget driver. The driver needs to know
the memory used is safe.

>
>>   The quirk is
>> only enabled for the DWC3 driver.
>
> So what?  Maybe other drivers will need it in the future.
>
>>> If the UDC driver decides to round up req->length before sending it to
>>> the hardware, that's okay.
>>
>> Not really. If the DWC3 driver unconditionally rounds up req->length,
>> then in the case where the buffer has not been expanded to a multiple of
>> maxpacket, by this quirk or otherwise, there is the potential to write
>> beyond the end of the allocation.
>
> Indeed.  It is the gadget driver's responsibility to make sure that
> doesn't happen.
>
>> If we do what you suggest, then all the gadgets will have to be audited
>> to make sure an OUT buffer with a non-aligned length is never passed to
>> the DWC3 driver.
>
> Certainly.  And since gadget drivers have no idea (or, _should_ have no
> idea) which UDC driver they are using, this means all gadgets will have
> to make sure that they _never_ use OUT buffers with non-aligned
> lengths.

But a possibility for a sanity check would be safer. UDCs should be
able to catch insufficient buffer size. Since buffer comes from gadget
driver, we need a way to inform both: request's length and buffer's
length.

>
>> I still think that's the best solution anyway. Just make that the rule,
>> and then there is no need for this quirk at all. And there is no need to
>> round up req->length in the DWC3 driver either.
>
> I agree that there really is no need for the quirk flag.  As for what
> the DWC3 driver does internally, I couldn't care less.  But gadget
> drivers should not round up req->length.
>
>>> But req->length should be set to len, not data_len.
>>
>> According to gadget.h:
>>
>> 	@buf: Buffer used for data
>> 	@length: Length of that data
>>
>> So why shouldn't length be the length of the allocated data buffer?
>
> Read what you just quoted: @length is the length of the _data_.  Not
> the length of the _buffer_.
>
> If the gadget driver wants to receive 100 bytes of data and the buffer
> is 128 bytes long, it should set req->length to 100, not 128.  By doing
> so, it tells the UDC driver that receiving more than 100 bytes is an
> error condition.
>
>> Remember, this is for the OUT direction only, so the host has control
>> over how much data is actually sent. You could even argue that an OUT
>> data buffer should always be a multiple of the max packet size, given
>> how USB works.
>
> Yes, you could.  I have no objection to changing the gadget API so that
> this becomes a firm requirement.

That's another way to go. No quirk would be necessary and DWC3 could
safely take care of itself. As long as no one goes against it.

>
>>> And if the hardware receives more than len bytes of data,
>>> the UDC driver should set req->status to -EOVERFLOW.
>>
>> That would be nice, but I don't see how to accomplish that given the
>> above.
>
> Why not?  The UDC driver knows what was originally in req->length and
> it knows how much data the hardware actually received.  All it has to
> do is:
>
> 	if (actual_length > req->length)
> 		req->status = -EOVERFLOW;

Agreed.

Br, David

Re: [PATCHv2 2/2] check quirk to pad epout buf size when not aligned to maxpacketsize

[David Cohen]

Hi Felipe,

On 11/13/2013 01:51 PM, David Cohen wrote:
> On 11/13/2013 07:52 AM, Alan Stern wrote:
>> On Tue, 12 Nov 2013, Paul Zimmerman wrote:
>>
>>>>>>>> @@ -824,7 +832,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;
>>>>>>>
>>>>>>> IIUC, req->length should still be set to len, not to data_len.
>>>>>
>>>>> I misunderstood the first time I read it:
>>>>> In order to avoid DWC3 to stall, we need to update req->length
>>>>> (this is
>>>>> the most important fix). kmalloc() is updated too to prevent USB
>>>>> controller to overflow buffer boundaries.
>>>>
>>>> Here I disagree.
>>>>
>>>> If the DWC3 hardware stalls, it is up to the DWC3 UDC driver to fix it.
>>>> Gadget drivers should not have to worry.  Most especially, gadget
>>>> drivers should not lie about a request length.
>>>
>>> The whole point of this quirk is to lie to the DWC3 driver.
>>
>> No.  The whole point of this quirk is to allow the DWC3 hardware to run
>> without stalling.
>>
>> It's dumb to lie to drivers.  They are there to help you; how can they
>> help you if you don't tell them what you want them to do?
> 
> IMO if we're not changing req->length we may need to add pad
> information to struct usb_request to avoid hidden buffer overflows.
> Even though we say it's UDC's responsibility of configure correctly
> itself the buffer comes from gadget driver. The driver needs to know
> the memory used is safe.
> 
>>
>>>   The quirk is
>>> only enabled for the DWC3 driver.
>>
>> So what?  Maybe other drivers will need it in the future.
>>
>>>> If the UDC driver decides to round up req->length before sending it to
>>>> the hardware, that's okay.
>>>
>>> Not really. If the DWC3 driver unconditionally rounds up req->length,
>>> then in the case where the buffer has not been expanded to a multiple of
>>> maxpacket, by this quirk or otherwise, there is the potential to write
>>> beyond the end of the allocation.
>>
>> Indeed.  It is the gadget driver's responsibility to make sure that
>> doesn't happen.
>>
>>> If we do what you suggest, then all the gadgets will have to be audited
>>> to make sure an OUT buffer with a non-aligned length is never passed to
>>> the DWC3 driver.
>>
>> Certainly.  And since gadget drivers have no idea (or, _should_ have no
>> idea) which UDC driver they are using, this means all gadgets will have
>> to make sure that they _never_ use OUT buffers with non-aligned
>> lengths.
> 
> But a possibility for a sanity check would be safer. UDCs should be
> able to catch insufficient buffer size. Since buffer comes from gadget
> driver, we need a way to inform both: request's length and buffer's
> length.
> 
>>
>>> I still think that's the best solution anyway. Just make that the rule,
>>> and then there is no need for this quirk at all. And there is no need to
>>> round up req->length in the DWC3 driver either.
>>
>> I agree that there really is no need for the quirk flag.  As for what
>> the DWC3 driver does internally, I couldn't care less.  But gadget
>> drivers should not round up req->length.
>>
>>>> But req->length should be set to len, not data_len.
>>>
>>> According to gadget.h:
>>>
>>>     @buf: Buffer used for data
>>>     @length: Length of that data
>>>
>>> So why shouldn't length be the length of the allocated data buffer?
>>
>> Read what you just quoted: @length is the length of the _data_.  Not
>> the length of the _buffer_.
>>
>> If the gadget driver wants to receive 100 bytes of data and the buffer
>> is 128 bytes long, it should set req->length to 100, not 128.  By doing
>> so, it tells the UDC driver that receiving more than 100 bytes is an
>> error condition.
>>
>>> Remember, this is for the OUT direction only, so the host has control
>>> over how much data is actually sent. You could even argue that an OUT
>>> data buffer should always be a multiple of the max packet size, given
>>> how USB works.
>>
>> Yes, you could.  I have no objection to changing the gadget API so that
>> this becomes a firm requirement.
> 
> That's another way to go. No quirk would be necessary and DWC3 could
> safely take care of itself. As long as no one goes against it.
> 
>>
>>>> And if the hardware receives more than len bytes of data,
>>>> the UDC driver should set req->status to -EOVERFLOW.
>>>
>>> That would be nice, but I don't see how to accomplish that given the
>>> above.
>>
>> Why not?  The UDC driver knows what was originally in req->length and
>> it knows how much data the hardware actually received.  All it has to
>> do is:
>>
>>     if (actual_length > req->length)
>>         req->status = -EOVERFLOW;
> 
> Agreed.

After the whole discussion, I'm afraid we need you (as usb gadget
maintainer) to tell how you prefer to solve this situation. I'd like to
send new patch set version without having open points to decide.

Br, David