All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/2] usb: gadget: composite: Fix bMaxPower for SuperSpeedPlus
@ 2019-10-23  6:57 Jack Pham
  2019-10-23  6:57 ` [PATCH 2/2] usb: gadget: composite: Support more than 500mA MaxPower Jack Pham
  2019-10-26  1:04 ` [PATCH v2 " Jack Pham
  0 siblings, 2 replies; 9+ messages in thread
From: Jack Pham @ 2019-10-23  6:57 UTC (permalink / raw)
  To: Felipe Balbi, Greg Kroah-Hartman; +Cc: linux-usb, Jack Pham, stable

SuperSpeedPlus peripherals must report their bMaxPower of the
configuration descriptor in units of 8mA as per the USB 3.2
specification. The current switch statement in encode_bMaxPower()
only checks for USB_SPEED_SUPER but not USB_SPEED_SUPER_PLUS so
the latter falls back to USB 2.0 encoding which uses 2mA units.
Replace the switch with a simple if/else.

Fixes: eae5820b852f ("usb: gadget: composite: Write SuperSpeedPlus config descriptors")
Cc: stable@vger.kernel.org
Signed-off-by: Jack Pham <jackp@codeaurora.org>
---
 drivers/usb/gadget/composite.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
index d516e8d6cd7f..e1db94d1fe2e 100644
--- a/drivers/usb/gadget/composite.c
+++ b/drivers/usb/gadget/composite.c
@@ -437,12 +437,10 @@ static u8 encode_bMaxPower(enum usb_device_speed speed,
 		val = CONFIG_USB_GADGET_VBUS_DRAW;
 	if (!val)
 		return 0;
-	switch (speed) {
-	case USB_SPEED_SUPER:
-		return DIV_ROUND_UP(val, 8);
-	default:
+	if (speed < USB_SPEED_SUPER)
 		return DIV_ROUND_UP(val, 2);
-	}
+	else
+		return DIV_ROUND_UP(val, 8);
 }
 
 static int config_buf(struct usb_configuration *config,
-- 
2.21.0


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 2/2] usb: gadget: composite: Support more than 500mA MaxPower
  2019-10-23  6:57 [PATCH 1/2] usb: gadget: composite: Fix bMaxPower for SuperSpeedPlus Jack Pham
@ 2019-10-23  6:57 ` Jack Pham
  2019-10-23  7:02   ` Jack Pham
  2019-10-23  7:49   ` Felipe Balbi
  2019-10-26  1:04 ` [PATCH v2 " Jack Pham
  1 sibling, 2 replies; 9+ messages in thread
From: Jack Pham @ 2019-10-23  6:57 UTC (permalink / raw)
  To: Felipe Balbi, Greg Kroah-Hartman; +Cc: linux-usb, Jack Pham

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
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);
 }
 
 static int config_buf(struct usb_configuration *config,
@@ -852,6 +853,10 @@ static int set_config(struct usb_composite_dev *cdev,
 
 	/* when we return, be sure our power usage is valid */
 	power = c->MaxPower ? c->MaxPower : CONFIG_USB_GADGET_VBUS_DRAW;
+	if (gadget->speed < USB_SPEED_SUPER)
+		power = min(power, 500U);
+	else
+		power = min(power, 900U);
 done:
 	usb_gadget_vbus_draw(gadget, power);
 	if (result >= 0 && cdev->delayed_status)
@@ -2289,6 +2294,10 @@ void composite_resume(struct usb_gadget *gadget)
 		}
 
 		maxpower = cdev->config->MaxPower;
+		if (gadget->speed < USB_SPEED_SUPER)
+			maxpower = min_t(u16, maxpower, 500U);
+		else
+			maxpower = min_t(u16, maxpower, 900U);
 
 		usb_gadget_vbus_draw(gadget, maxpower ?
 			maxpower : CONFIG_USB_GADGET_VBUS_DRAW);
-- 
2.21.0


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] usb: gadget: composite: Support more than 500mA MaxPower
  2019-10-23  6:57 ` [PATCH 2/2] usb: gadget: composite: Support more than 500mA MaxPower Jack Pham
@ 2019-10-23  7:02   ` Jack Pham
  2019-10-23  7:49   ` Felipe Balbi
  1 sibling, 0 replies; 9+ messages in thread
From: Jack Pham @ 2019-10-23  7:02 UTC (permalink / raw)
  To: Felipe Balbi, Greg Kroah-Hartman; +Cc: linux-usb

On Tue, Oct 22, 2019 at 11:57:53PM -0700, Jack Pham wrote:
> 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.
                                                    ^^^^^

Argh, my bad math/typo. Should be 510mA (UINT8_MAX = 255 * 2mA).

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] usb: gadget: composite: Support more than 500mA MaxPower
  2019-10-23  6:57 ` [PATCH 2/2] usb: gadget: composite: Support more than 500mA MaxPower Jack Pham
  2019-10-23  7:02   ` Jack Pham
@ 2019-10-23  7:49   ` Felipe Balbi
  2019-10-23  8:31     ` jackp
  1 sibling, 1 reply; 9+ messages in thread
From: Felipe Balbi @ 2019-10-23  7:49 UTC (permalink / raw)
  To: Jack Pham, Greg Kroah-Hartman; +Cc: linux-usb, Jack Pham

[-- Attachment #1: Type: text/plain, Size: 2612 bytes --]


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.

> 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?

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] usb: gadget: composite: Support more than 500mA MaxPower
  2019-10-23  7:49   ` Felipe Balbi
@ 2019-10-23  8:31     ` jackp
  2019-10-29 11:03       ` Felipe Balbi
  0 siblings, 1 reply; 9+ messages in thread
From: jackp @ 2019-10-23  8:31 UTC (permalink / raw)
  To: Felipe Balbi; +Cc: Greg Kroah-Hartman, linux-usb

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.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v2 2/2] usb: gadget: composite: Support more than 500mA MaxPower
  2019-10-23  6:57 [PATCH 1/2] usb: gadget: composite: Fix bMaxPower for SuperSpeedPlus Jack Pham
  2019-10-23  6:57 ` [PATCH 2/2] usb: gadget: composite: Support more than 500mA MaxPower Jack Pham
@ 2019-10-26  1:04 ` Jack Pham
  1 sibling, 0 replies; 9+ messages in thread
From: Jack Pham @ 2019-10-26  1:04 UTC (permalink / raw)
  To: Felipe Balbi, Greg Kroah-Hartman; +Cc: linux-usb, Jack Pham

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 510mA.

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 divisible by 8, so even for Superspeed
the bMaxPower needs to be capped at 896mA, otherwise due to the
round-up division a MaxPower of 900mA will result in an encoded
value of 0x71. When a host stack (including Linux and Windows)
enumerates this on a single port root hub, it reads this value
back and decodes (multiplies by 8) to get 904mA which is strictly
greater than 900mA that is typically budgeted for that port,
causing it to 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>
---
v2: Fix typos in commit text and reworded the blurb about rounding

 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);
 }
 
 static int config_buf(struct usb_configuration *config,
@@ -852,6 +853,10 @@ static int set_config(struct usb_composite_dev *cdev,
 
 	/* when we return, be sure our power usage is valid */
 	power = c->MaxPower ? c->MaxPower : CONFIG_USB_GADGET_VBUS_DRAW;
+	if (gadget->speed < USB_SPEED_SUPER)
+		power = min(power, 500U);
+	else
+		power = min(power, 900U);
 done:
 	usb_gadget_vbus_draw(gadget, power);
 	if (result >= 0 && cdev->delayed_status)
@@ -2289,6 +2294,10 @@ void composite_resume(struct usb_gadget *gadget)
 		}
 
 		maxpower = cdev->config->MaxPower;
+		if (gadget->speed < USB_SPEED_SUPER)
+			maxpower = min_t(u16, maxpower, 500U);
+		else
+			maxpower = min_t(u16, maxpower, 900U);
 
 		usb_gadget_vbus_draw(gadget, maxpower ?
 			maxpower : CONFIG_USB_GADGET_VBUS_DRAW);
-- 
2.21.0


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] usb: gadget: composite: Support more than 500mA MaxPower
  2019-10-23  8:31     ` jackp
@ 2019-10-29 11:03       ` Felipe Balbi
  2019-10-30  2:11         ` Jack Pham
  0 siblings, 1 reply; 9+ messages in thread
From: Felipe Balbi @ 2019-10-29 11:03 UTC (permalink / raw)
  To: jackp; +Cc: Greg Kroah-Hartman, linux-usb

[-- Attachment #1: Type: text/plain, Size: 2225 bytes --]


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.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] usb: gadget: composite: Support more than 500mA MaxPower
  2019-10-29 11:03       ` Felipe Balbi
@ 2019-10-30  2:11         ` Jack Pham
  2019-10-30 11:39           ` Felipe Balbi
  0 siblings, 1 reply; 9+ messages in thread
From: Jack Pham @ 2019-10-30  2:11 UTC (permalink / raw)
  To: Felipe Balbi; +Cc: Greg Kroah-Hartman, linux-usb

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] usb: gadget: composite: Support more than 500mA MaxPower
  2019-10-30  2:11         ` Jack Pham
@ 2019-10-30 11:39           ` Felipe Balbi
  0 siblings, 0 replies; 9+ messages in thread
From: Felipe Balbi @ 2019-10-30 11:39 UTC (permalink / raw)
  To: Jack Pham; +Cc: Greg Kroah-Hartman, linux-usb


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

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2019-10-30 11:39 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-23  6:57 [PATCH 1/2] usb: gadget: composite: Fix bMaxPower for SuperSpeedPlus Jack Pham
2019-10-23  6:57 ` [PATCH 2/2] usb: gadget: composite: Support more than 500mA MaxPower Jack Pham
2019-10-23  7:02   ` Jack Pham
2019-10-23  7:49   ` Felipe Balbi
2019-10-23  8:31     ` jackp
2019-10-29 11:03       ` Felipe Balbi
2019-10-30  2:11         ` Jack Pham
2019-10-30 11:39           ` Felipe Balbi
2019-10-26  1:04 ` [PATCH v2 " Jack Pham

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.