Hi,
These patches are a proposal to add gadget quirks in an immediate objective to
adapt f_fs when using DWC3 controller. But the quirk solution is generic and
can be used by other controllers to adapt gadget functions to their
non-standard restrictions.
This change is necessary to make Android's adbd service to work on Intel
Merrifield with f_fs instead of out-of-tree android gadget.
---
David Cohen (3):
usb: gadget: add quirks 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 | 1 +
drivers/usb/gadget/f_fs.c | 17 +++++++++++++++++
include/linux/usb/gadget.h | 5 +++++
3 files changed, 23 insertions(+)
--
1.8.4.rc3
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 'quirks' field to struct usb_gadget and the first quirk
called USB_GADGET_QUIRK_EP_OUT_ALIGNED_SIZE necessary to inform when
controller's epout requires buffer size to be aligned to MaxPacketSize.
Signed-off-by: David Cohen <[email protected]>
---
include/linux/usb/gadget.h | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
index 942ef5e..7014ad9 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -540,6 +540,11 @@ struct usb_gadget {
struct device dev;
unsigned out_epnum;
unsigned in_epnum;
+
+ u32 quirks;
+/* epout requires buffer size to be aligned to MaxPacketSize */
+#define USB_GADGET_QUIRK_EP_OUT_ALIGNED_SIZE (1 << 0)
+
};
#define work_to_gadget(w) (container_of((w), struct usb_gadget, work))
--
1.8.4.rc3
Use USB_GADGET_QUIRK_EP_OUT_ALIGNED_SIZE quirk to check 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 <[email protected]>
---
drivers/usb/gadget/f_fs.c | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
diff --git a/drivers/usb/gadget/f_fs.c b/drivers/usb/gadget/f_fs.c
index 75e4b78..2988eb7 100644
--- a/drivers/usb/gadget/f_fs.c
+++ b/drivers/usb/gadget/f_fs.c
@@ -755,10 +755,12 @@ static ssize_t ffs_epfile_io(struct file *file,
char __user *buf, size_t len, int read)
{
struct ffs_epfile *epfile = file->private_data;
+ struct usb_gadget *gadget = epfile->ffs->gadget;
struct ffs_ep *ep;
char *data = NULL;
ssize_t ret;
int halt;
+ size_t orig_len = len;
goto first_try;
do {
@@ -794,6 +796,21 @@ first_try:
goto error;
}
+ /*
+ * Controller requires buffer size to be aligned to
+ * maxpacketsize of an out endpoint.
+ */
+ if (gadget->quirks & USB_GADGET_QUIRK_EP_OUT_ALIGNED_SIZE &&
+ read && !IS_ALIGNED(len, ep->ep->desc->wMaxPacketSize)) {
+ size_t old_len = len;
+ len = roundup(orig_len,
+ (size_t)ep->ep->desc->wMaxPacketSize);
+ if (unlikely(data) && len > old_len) {
+ kfree(data);
+ data = NULL;
+ }
+ }
+
/* Allocate & copy */
if (!halt && !data) {
data = kzalloc(len, GFP_KERNEL);
--
1.8.4.rc3
DWC3 requires epout to have buffer size aligned to MaxPacketSize value.
This patch adds necessary quirk for it.
Signed-off-by: David Cohen <[email protected]>
---
drivers/usb/dwc3/gadget.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 5452c0f..0dcc89f 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2598,6 +2598,7 @@ int dwc3_gadget_init(struct dwc3 *dwc)
dwc->gadget.speed = USB_SPEED_UNKNOWN;
dwc->gadget.sg_supported = true;
dwc->gadget.name = "dwc3-gadget";
+ dwc->gadget.quirks = USB_GADGET_QUIRK_EP_OUT_ALIGNED_SIZE;
/*
* REVISIT: Here we should clear all pending IRQs to be
--
1.8.4.rc3
> From: David Cohen
> Sent: Tuesday, October 29, 2013 2:53 PM
>
> These patches are a proposal to add gadget quirks in an immediate objective to
> adapt f_fs when using DWC3 controller. But the quirk solution is generic and
> can be used by other controllers to adapt gadget functions to their
> non-standard restrictions.
>
> This change is necessary to make Android's adbd service to work on Intel
> Merrifield with f_fs instead of out-of-tree android gadget.
>
> ---
> David Cohen (3):
> usb: gadget: add quirks 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 | 1 +
> drivers/usb/gadget/f_fs.c | 17 +++++++++++++++++
> include/linux/usb/gadget.h | 5 +++++
> 3 files changed, 23 insertions(+)
Wouldn't it be simpler and safer to just do this unconditionally? Sure,
you need it for DWC3 because the controller refuses to do an OUT transfer
at all if the transfer size is less than maxpacketsize. But it's possible
that other controllers allow the transfer, and it works in most cases,
but if an error occurs and the host sends too much data, they could
overrun the buffer and crash your device.
For example, the DWC2 databook says "For OUT transfers, the Transfer
Size field in the endpoint's Transfer Size register must be a multiple
of the maximum packet size of the endpoint". But I don't think the
controller enforces that, it is up to the programmer to do the right
thing. So that controller probably needs this quirk also. There could be
more like that which we don't know about.
So unless the buffer allocation code is in a real fast path, I would
suggest to just do the aligned buffer allocation always.
--
Paul
> Wouldn't it be simpler and safer to just do this unconditionally? Sure,
> you need it for DWC3 because the controller refuses to do an OUT transfer
> at all if the transfer size is less than maxpacketsize. But it's possible
> that other controllers allow the transfer, and it works in most cases,
> but if an error occurs and the host sends too much data, they could
> overrun the buffer and crash your device.
>
> For example, the DWC2 databook says "For OUT transfers, the Transfer
> Size field in the endpoint's Transfer Size register must be a multiple
> of the maximum packet size of the endpoint". But I don't think the
> controller enforces that, it is up to the programmer to do the right
> thing. So that controller probably needs this quirk also. There could be
> more like that which we don't know about.
>
> So unless the buffer allocation code is in a real fast path, I would
> suggest to just do the aligned buffer allocation always.
You wouldn't normally want to pad OUT transfers that way - if only
because of the additional USB bandwidth use.
Also, if the controller can't do (I assume bulk) OUT (and IN?)
transfers for less than maxpacketsize it seriously restricts
the type of devices that can be attached - none of the USB
ethernet devices would work.
David
On Tue, 29 Oct 2013, David Cohen wrote:
> Due to USB controllers may have different restrictions, usb gadget layer
> needs to provide a generic way to inform gadget functions to complain
> with non-standard requirements.
>
> This patch adds 'quirks' field to struct usb_gadget and the first quirk
> called USB_GADGET_QUIRK_EP_OUT_ALIGNED_SIZE necessary to inform when
> controller's epout requires buffer size to be aligned to MaxPacketSize.
>
> Signed-off-by: David Cohen <[email protected]>
> ---
> include/linux/usb/gadget.h | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
> index 942ef5e..7014ad9 100644
> --- a/include/linux/usb/gadget.h
> +++ b/include/linux/usb/gadget.h
> @@ -540,6 +540,11 @@ struct usb_gadget {
> struct device dev;
> unsigned out_epnum;
> unsigned in_epnum;
> +
> + u32 quirks;
> +/* epout requires buffer size to be aligned to MaxPacketSize */
> +#define USB_GADGET_QUIRK_EP_OUT_ALIGNED_SIZE (1 << 0)
If you decide to go through with this, it might be better to define a
series of single-bit flags instead of a single "quirks" field. For
example:
unsigned quirk_ep_out_aligned_size:1;
Yes, other people (including me!) have done it your way in the past,
but now this seems to make more sense.
Alan Stern
On Wed, 30 Oct 2013, David Laight wrote:
> > Wouldn't it be simpler and safer to just do this unconditionally? Sure,
> > you need it for DWC3 because the controller refuses to do an OUT transfer
> > at all if the transfer size is less than maxpacketsize. But it's possible
> > that other controllers allow the transfer, and it works in most cases,
> > but if an error occurs and the host sends too much data, they could
> > overrun the buffer and crash your device.
> >
> > For example, the DWC2 databook says "For OUT transfers, the Transfer
> > Size field in the endpoint's Transfer Size register must be a multiple
> > of the maximum packet size of the endpoint". But I don't think the
> > controller enforces that, it is up to the programmer to do the right
> > thing. So that controller probably needs this quirk also. There could be
> > more like that which we don't know about.
> >
> > So unless the buffer allocation code is in a real fast path, I would
> > suggest to just do the aligned buffer allocation always.
>
> You wouldn't normally want to pad OUT transfers that way - if only
> because of the additional USB bandwidth use.
What additional bandwidth use? Allocating more memory doesn't mean any
additional data will be transmitted over the USB bus.
> Also, if the controller can't do (I assume bulk) OUT (and IN?)
> transfers for less than maxpacketsize it seriously restricts
> the type of devices that can be attached - none of the USB
> ethernet devices would work.
The controllers _are_ capable of doing shorter transfers. You are
missing the point: These are device controllers, not host controllers,
so they don't have any choice about the length of an OUT transfer.
Alan Stern
>> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
>> index 942ef5e..7014ad9 100644
>> --- a/include/linux/usb/gadget.h
>> +++ b/include/linux/usb/gadget.h
>> @@ -540,6 +540,11 @@ struct usb_gadget {
>> struct device dev;
>> unsigned out_epnum;
>> unsigned in_epnum;
>> +
>> + u32 quirks;
>> +/* epout requires buffer size to be aligned to MaxPacketSize */
>> +#define USB_GADGET_QUIRK_EP_OUT_ALIGNED_SIZE (1 << 0)
>
> If you decide to go through with this, it might be better to define a
> series of single-bit flags instead of a single "quirks" field. For
> example:
>
> unsigned quirk_ep_out_aligned_size:1;
>
> Yes, other people (including me!) have done it your way in the past,
> but now this seems to make more sense.
It would be less error-prone. I'll change it.
Thanks,
David
On 10/29/2013 03:47 PM, Paul Zimmerman wrote:
>> From: David Cohen
>> Sent: Tuesday, October 29, 2013 2:53 PM
>>
>> These patches are a proposal to add gadget quirks in an immediate objective to
>> adapt f_fs when using DWC3 controller. But the quirk solution is generic and
>> can be used by other controllers to adapt gadget functions to their
>> non-standard restrictions.
>>
>> This change is necessary to make Android's adbd service to work on Intel
>> Merrifield with f_fs instead of out-of-tree android gadget.
>>
>> ---
>> David Cohen (3):
>> usb: gadget: add quirks 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 | 1 +
>> drivers/usb/gadget/f_fs.c | 17 +++++++++++++++++
>> include/linux/usb/gadget.h | 5 +++++
>> 3 files changed, 23 insertions(+)
>
> Wouldn't it be simpler and safer to just do this unconditionally? Sure,
> you need it for DWC3 because the controller refuses to do an OUT transfer
> at all if the transfer size is less than maxpacketsize. But it's possible
> that other controllers allow the transfer, and it works in most cases,
> but if an error occurs and the host sends too much data, they could
> overrun the buffer and crash your device.
>
> For example, the DWC2 databook says "For OUT transfers, the Transfer
> Size field in the endpoint's Transfer Size register must be a multiple
> of the maximum packet size of the endpoint". But I don't think the
> controller enforces that, it is up to the programmer to do the right
> thing. So that controller probably needs this quirk also. There could be
> more like that which we don't know about.
Unfortunately DWC2 is a bad example... the driver couldn't even get out
of staging. If the author was reckless to ignore this restriction (s)he
should fix.
But I don't have enough data to tell it's better to waste everybody's
memory in this case in favor of DWC3. I'd still stick with the
exception.
>
> So unless the buffer allocation code is in a real fast path, I would
> suggest to just do the aligned buffer allocation always.
This code would affect embedded devices which value too much memory
consumption (and performance on handling it!). IMO we'd need to be more
careful prior to take such decision.
Br, David Cohen
Hi,
On Wed, Oct 30, 2013 at 09:36:20AM -0700, David Cohen wrote:
> On 10/29/2013 03:47 PM, Paul Zimmerman wrote:
> >>From: David Cohen
> >>Sent: Tuesday, October 29, 2013 2:53 PM
> >>
> >>These patches are a proposal to add gadget quirks in an immediate objective to
> >>adapt f_fs when using DWC3 controller. But the quirk solution is generic and
> >>can be used by other controllers to adapt gadget functions to their
> >>non-standard restrictions.
> >>
> >>This change is necessary to make Android's adbd service to work on Intel
> >>Merrifield with f_fs instead of out-of-tree android gadget.
> >>
> >>---
> >>David Cohen (3):
> >> usb: gadget: add quirks 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 | 1 +
> >> drivers/usb/gadget/f_fs.c | 17 +++++++++++++++++
> >> include/linux/usb/gadget.h | 5 +++++
> >> 3 files changed, 23 insertions(+)
> >
> >Wouldn't it be simpler and safer to just do this unconditionally? Sure,
> >you need it for DWC3 because the controller refuses to do an OUT transfer
> >at all if the transfer size is less than maxpacketsize. But it's possible
> >that other controllers allow the transfer, and it works in most cases,
> >but if an error occurs and the host sends too much data, they could
> >overrun the buffer and crash your device.
> >
> >For example, the DWC2 databook says "For OUT transfers, the Transfer
> >Size field in the endpoint's Transfer Size register must be a multiple
> >of the maximum packet size of the endpoint". But I don't think the
> >controller enforces that, it is up to the programmer to do the right
> >thing. So that controller probably needs this quirk also. There could be
> >more like that which we don't know about.
>
> Unfortunately DWC2 is a bad example... the driver couldn't even get out
> of staging. If the author was reckless to ignore this restriction (s)he
> should fix.
> But I don't have enough data to tell it's better to waste everybody's
> memory in this case in favor of DWC3. I'd still stick with the
> exception.
on top of that we don't what quirks other controllers might have. There
could very well be a controller which quirk goes the other around: you
_must_ set bulk out length to the correct size (e.g. 31 bytes for mass
storage's CBW) otherwise transfer won't complete.
I could see that happening on controllers with broken short packet
handling.
> >So unless the buffer allocation code is in a real fast path, I would
> >suggest to just do the aligned buffer allocation always.
>
> This code would affect embedded devices which value too much memory
> consumption (and performance on handling it!). IMO we'd need to be more
> careful prior to take such decision.
agreed. Quirk flag is the way to go here.
--
balbi