Hi, Jack Pham <jackp@xxxxxxxxxxxxxx> writes: >> 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... 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
