Re: [PATCH 2/2] usb: gadget: composite: Support more than 500mA MaxPower

[Jack Pham <jackp@codeaurora.org>]

Hi Felipe,

On Tue, Oct 29, 2019 at 01:03:27PM +0200, Felipe Balbi wrote:
> 
> Hi,
> 
> jackp@codeaurora.org writes:
> > On 2019-10-23 00:49, Felipe Balbi wrote:
> >> Hi,
> >> 
> >> Jack Pham <jackp@codeaurora.org> writes:
> >>> USB 3.x SuperSpeed peripherals can draw up to 900mA of VBUS power
> >>> when in configured state. However, if a configuration wanting to
> >>> take advantage of this is added with MaxPower greater than 500
> >>> (currently possible if using a ConfigFS gadget) the composite
> >>> driver fails to accommodate this for a couple reasons:
> >>> 
> >>>  - usb_gadget_vbus_draw() when called from set_config() and
> >>>    composite_resume() will be passed the MaxPower value without
> >>>    regard for the current connection speed, resulting in a
> >>>    violation for USB 2.0 since the max is 500mA.
> >>> 
> >>>  - the bMaxPower of the configuration descriptor would be
> >>>    incorrectly encoded, again if the connection speed is only
> >>>    at USB 2.0 or below, likely wrapping around UINT8_MAX since
> >>>    the 2mA multiplier corresponds to a maximum of 610mA.
> >>> 
> >>> Fix these by adding checks against the current gadget->speed
> >>> when the c->MaxPower value is used and appropriately limit
> >>> based on whether it is currently at a low-/full-/high- or super-
> >>> speed connection.
> >>> 
> >>> Incidentally, 900 is not divisble by 8, so even for super-speed
> >>> the bMaxPower neds to be capped at 896mA, otherwise due to the
> >>                 ^^^^
> >>                 needs
> >> 
> >> Why do you need to cap it? DIV_ROUND_UP() should still work.
> >
> > The round up causes 900 on the input side to be greater than 900 when 
> > doing the
> > reverse, i.e. multiplication by 8.
> >
> > Alternatively we could just do a normal integer division here 
> > (effectively
> > round down).
> 
> (...)
> 
> >> DIV_ROUND_UP(896, 8) = 112
> >> DIV_ROUND_UP(900, 8) = 113
> >> 
> >> Why value do you want here?
>    ^^^
>    I mean which, sorry
> 
> > Right, but now on the host it will do the reverse calculation, i.e.
> > 113*8 == 904mA.  For a root port this would be greater than it's
> > maximum power budget of 900mA and would result in not selecting the
> > config.
> 
> That's a very good explanation of the problem, thank you. It seems like
> a round down would be safer here in all cases.

Ok, so do you mean something like:

	if (speed < USB_SPEED_SUPER)
-		return DIV_ROUND_UP(val, 2);
+		return DIV_ROUND_UP(min(val, 500U), 2);
	else
-		return DIV_ROUND_UP(val, 8);
+		/*
+		 * USB 3.x supports up to 900mA, but since 900 isn't
+		 * divisible by 8, we need to round down.
+		 */
+		return min(val, 900U) / 8;

Or by "all cases" did you also mean high/full/low speeds too where the
divisor is 2mA (in the first part of the if/else)? Otherwise it looks a
little inconsistent using two modes of division here. Technically the
calculation would then be changed for any odd values less than 500mA but
we're only talking about a difference of 2mA here...

Jack
-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project