Re: [RFC/PATCH v2 0/3] add gadget quirk to adapt f_fs for DWC3

[Felipe Balbi]

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

Attachment: signature.asc
Description: Digital signature