Hi Felipe, On Tue, Oct 29, 2019 at 01:03:27PM +0200, Felipe Balbi wrote: > > Hi, > > jackp@xxxxxxxxxxxxxx writes: > > On 2019-10-23 00:49, Felipe Balbi wrote: > >> Hi, > >> > >> Jack Pham <jackp@xxxxxxxxxxxxxx> 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
