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

[jackp@codeaurora.org]

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).

>> round-up division a MaxPower of 900 will result in an encoded
>> value of 904mA and many host stacks (including Linux and Windows)
>> of a root port will reject the configuration.
>> 
>> N.B. USB 3.2 Gen N x 2 allows for up to 1500mA but there doesn't
>> seem to be any any peripheral controller supported by Linux that
>> does two lane operation, so for now keeping the clamp at 900
>> should be fine.
>> 
>> Signed-off-by: Jack Pham <jackp@codeaurora.org>
>> ---
>>  drivers/usb/gadget/composite.c | 13 +++++++++++--
>>  1 file changed, 11 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/usb/gadget/composite.c 
>> b/drivers/usb/gadget/composite.c
>> index e1db94d1fe2e..92ce3018f482 100644
>> --- a/drivers/usb/gadget/composite.c
>> +++ b/drivers/usb/gadget/composite.c
>> @@ -438,9 +438,10 @@ static u8 encode_bMaxPower(enum usb_device_speed 
>> speed,
>>  	if (!val)
>>  		return 0;
>>  	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 900mA, but that isn't divisible by 8... */
>> +		return DIV_ROUND_UP(min(val, 896U), 8);
> 
> DIV_ROUND_UP(896, 8) = 112
> DIV_ROUND_UP(900, 8) = 113
> 
> Why value do you want here?

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.