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

[Felipe Balbi <balbi@kernel.org>]

Hi,

Jack Pham <jackp@codeaurora.org> writes:
>> 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...

yeah, It should be okay to use round down for fs, hs and ls as
well. Thinking about it, this bMaxPower should be treated as a "at most
this much"; but never more than maximum.

-- 
balbi