1. Patchew
  2. linux
  3. View series

[PATCH v2] usb: gadget: Set self-powered based on MaxPower and bmAttributes

Prashanth K posted 1 patch 10 months ago 
drivers/usb/gadget/composite.c | 16 +++++++++++-----
1 file changed, 11 insertions(+), 5 deletions(-)
[PATCH v2] usb: gadget: Set self-powered based on MaxPower and bmAttributes
Posted by Prashanth K 10 months ago 
Currently the USB gadget will be set as bus-powered based solely
on whether its bMaxPower is greater than 100mA, but this may miss
devices that may legitimately draw less than 100mA but still want
to report as bus-powered. Similarly during suspend & resume, USB
gadget is incorrectly marked as bus/self powered without checking
the bmAttributes field. Fix these by configuring the USB gadget
as self or bus powered based on bmAttributes, and explicitly set
it as bus-powered if it draws more than 100mA.

Cc: stable@vger.kernel.org
Fixes: 5e5caf4fa8d3 ("usb: gadget: composite: Inform controller driver of self-powered")
Signed-off-by: Prashanth K <prashanth.k@oss.qualcomm.com>
---
Changes in v2:
- Didn't change anything from RFC.
- Link to RFC: https://lore.kernel.org/all/20250204105908.2255686-1-prashanth.k@oss.qualcomm.com/

 drivers/usb/gadget/composite.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
index bdda8c74602d..1fb28bbf6c45 100644
--- a/drivers/usb/gadget/composite.c
+++ b/drivers/usb/gadget/composite.c
@@ -1050,10 +1050,11 @@ static int set_config(struct usb_composite_dev *cdev,
 	else
 		usb_gadget_set_remote_wakeup(gadget, 0);
 done:
-	if (power <= USB_SELF_POWER_VBUS_MAX_DRAW)
-		usb_gadget_set_selfpowered(gadget);
-	else
+	if (power > USB_SELF_POWER_VBUS_MAX_DRAW ||
+	    !(c->bmAttributes & USB_CONFIG_ATT_SELFPOWER))
 		usb_gadget_clear_selfpowered(gadget);
+	else
+		usb_gadget_set_selfpowered(gadget);
 
 	usb_gadget_vbus_draw(gadget, power);
 	if (result >= 0 && cdev->delayed_status)
@@ -2615,7 +2616,9 @@ void composite_suspend(struct usb_gadget *gadget)
 
 	cdev->suspended = 1;
 
-	usb_gadget_set_selfpowered(gadget);
+	if (cdev->config->bmAttributes & USB_CONFIG_ATT_SELFPOWER)
+		usb_gadget_set_selfpowered(gadget);
+
 	usb_gadget_vbus_draw(gadget, 2);
 }
 
@@ -2649,8 +2652,11 @@ void composite_resume(struct usb_gadget *gadget)
 		else
 			maxpower = min(maxpower, 900U);
 
-		if (maxpower > USB_SELF_POWER_VBUS_MAX_DRAW)
+		if (maxpower > USB_SELF_POWER_VBUS_MAX_DRAW ||
+		    !(cdev->config->bmAttributes & USB_CONFIG_ATT_SELFPOWER))
 			usb_gadget_clear_selfpowered(gadget);
+		else
+			usb_gadget_set_selfpowered(gadget);
 
 		usb_gadget_vbus_draw(gadget, maxpower);
 	} else {
-- 
2.25.1
Re: [PATCH v2] usb: gadget: Set self-powered based on MaxPower and bmAttributes
Posted by Kees Bakker 10 months ago 
Op 17-02-2025 om 13:03 schreef Prashanth K:
> Currently the USB gadget will be set as bus-powered based solely
> on whether its bMaxPower is greater than 100mA, but this may miss
> devices that may legitimately draw less than 100mA but still want
> to report as bus-powered. Similarly during suspend & resume, USB
> gadget is incorrectly marked as bus/self powered without checking
> the bmAttributes field. Fix these by configuring the USB gadget
> as self or bus powered based on bmAttributes, and explicitly set
> it as bus-powered if it draws more than 100mA.
>
> Cc: stable@vger.kernel.org
> Fixes: 5e5caf4fa8d3 ("usb: gadget: composite: Inform controller driver of self-powered")
> Signed-off-by: Prashanth K <prashanth.k@oss.qualcomm.com>
> ---
> Changes in v2:
> - Didn't change anything from RFC.
> - Link to RFC: https://lore.kernel.org/all/20250204105908.2255686-1-prashanth.k@oss.qualcomm.com/
>
>   drivers/usb/gadget/composite.c | 16 +++++++++++-----
>   1 file changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
> index bdda8c74602d..1fb28bbf6c45 100644
> --- a/drivers/usb/gadget/composite.c
> +++ b/drivers/usb/gadget/composite.c
> @@ -1050,10 +1050,11 @@ static int set_config(struct usb_composite_dev *cdev,
>   	else
>   		usb_gadget_set_remote_wakeup(gadget, 0);
>   done:
> -	if (power <= USB_SELF_POWER_VBUS_MAX_DRAW)
> -		usb_gadget_set_selfpowered(gadget);
> -	else
> +	if (power > USB_SELF_POWER_VBUS_MAX_DRAW ||
> +	    !(c->bmAttributes & USB_CONFIG_ATT_SELFPOWER))
Please check this change again. From line 983-884 there is a `goto done`.
in case `c` is NULL. So, there will be a potential NULL pointer dereference
with your change.
>   		usb_gadget_clear_selfpowered(gadget);
> +	else
> +		usb_gadget_set_selfpowered(gadget);
>   
>   	usb_gadget_vbus_draw(gadget, power);
>   	if (result >= 0 && cdev->delayed_status)
> [...]
Re: [PATCH v2] usb: gadget: Set self-powered based on MaxPower and bmAttributes
Posted by Prashanth K 9 months, 4 weeks ago 

On 22-02-25 12:56 am, Kees Bakker wrote:
> Op 17-02-2025 om 13:03 schreef Prashanth K:
>> Currently the USB gadget will be set as bus-powered based solely
>> on whether its bMaxPower is greater than 100mA, but this may miss
>> devices that may legitimately draw less than 100mA but still want
>> to report as bus-powered. Similarly during suspend & resume, USB
>> gadget is incorrectly marked as bus/self powered without checking
>> the bmAttributes field. Fix these by configuring the USB gadget
>> as self or bus powered based on bmAttributes, and explicitly set
>> it as bus-powered if it draws more than 100mA.
>>
>> Cc: stable@vger.kernel.org
>> Fixes: 5e5caf4fa8d3 ("usb: gadget: composite: Inform controller driver
>> of self-powered")
>> Signed-off-by: Prashanth K <prashanth.k@oss.qualcomm.com>
>> ---
>> Changes in v2:
>> - Didn't change anything from RFC.
>> - Link to RFC: https://lore.kernel.org/all/20250204105908.2255686-1-
>> prashanth.k@oss.qualcomm.com/
>>
>>   drivers/usb/gadget/composite.c | 16 +++++++++++-----
>>   1 file changed, 11 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/
>> composite.c
>> index bdda8c74602d..1fb28bbf6c45 100644
>> --- a/drivers/usb/gadget/composite.c
>> +++ b/drivers/usb/gadget/composite.c
>> @@ -1050,10 +1050,11 @@ static int set_config(struct usb_composite_dev
>> *cdev,
>>       else
>>           usb_gadget_set_remote_wakeup(gadget, 0);
>>   done:
>> -    if (power <= USB_SELF_POWER_VBUS_MAX_DRAW)
>> -        usb_gadget_set_selfpowered(gadget);
>> -    else
>> +    if (power > USB_SELF_POWER_VBUS_MAX_DRAW ||
>> +        !(c->bmAttributes & USB_CONFIG_ATT_SELFPOWER))
> Please check this change again. From line 983-884 there is a `goto done`.
> in case `c` is NULL. So, there will be a potential NULL pointer dereference
> with your change.

Yea good catch, sorry for missing the corner case. Ill send another patch.

Regards,
Prashanth K
Re: [PATCH v2] usb: gadget: Set self-powered based on MaxPower and bmAttributes
Posted by William McVicker 10 months ago 
Hi Prashanth,

On 02/17/2025, Prashanth K wrote:
> Currently the USB gadget will be set as bus-powered based solely
> on whether its bMaxPower is greater than 100mA, but this may miss
> devices that may legitimately draw less than 100mA but still want
> to report as bus-powered. Similarly during suspend & resume, USB
> gadget is incorrectly marked as bus/self powered without checking
> the bmAttributes field. Fix these by configuring the USB gadget
> as self or bus powered based on bmAttributes, and explicitly set
> it as bus-powered if it draws more than 100mA.
> 
> Cc: stable@vger.kernel.org
> Fixes: 5e5caf4fa8d3 ("usb: gadget: composite: Inform controller driver of self-powered")
> Signed-off-by: Prashanth K <prashanth.k@oss.qualcomm.com>
> ---
> Changes in v2:
> - Didn't change anything from RFC.
> - Link to RFC: https://lore.kernel.org/all/20250204105908.2255686-1-prashanth.k@oss.qualcomm.com/
> 
>  drivers/usb/gadget/composite.c | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
> index bdda8c74602d..1fb28bbf6c45 100644
> --- a/drivers/usb/gadget/composite.c
> +++ b/drivers/usb/gadget/composite.c
> @@ -1050,10 +1050,11 @@ static int set_config(struct usb_composite_dev *cdev,
>  	else
>  		usb_gadget_set_remote_wakeup(gadget, 0);
>  done:
> -	if (power <= USB_SELF_POWER_VBUS_MAX_DRAW)
> -		usb_gadget_set_selfpowered(gadget);
> -	else
> +	if (power > USB_SELF_POWER_VBUS_MAX_DRAW ||
> +	    !(c->bmAttributes & USB_CONFIG_ATT_SELFPOWER))
>  		usb_gadget_clear_selfpowered(gadget);
> +	else
> +		usb_gadget_set_selfpowered(gadget);
>  
>  	usb_gadget_vbus_draw(gadget, power);
>  	if (result >= 0 && cdev->delayed_status)
> @@ -2615,7 +2616,9 @@ void composite_suspend(struct usb_gadget *gadget)
>  
>  	cdev->suspended = 1;
>  
> -	usb_gadget_set_selfpowered(gadget);
> +	if (cdev->config->bmAttributes & USB_CONFIG_ATT_SELFPOWER)
> +		usb_gadget_set_selfpowered(gadget);

I'm hitting a null pointer derefence here on my Pixel 6 device on suspend.  I
haven't dug deep into it how we get here, but in my case `cdev->config` is
NULL. This happens immediate after booting my device. I verified that just
adding a NULL check fixes the issue and dwc3 gadget can successfully suspend.

Here is the crash stack:

  Unable to handle kernel NULL pointer dereference at virtual address 000000000000002a
  <snip>
  Modules linked in: tcpci_maxim(E) at24(E) phy_exynos_ufs(E)
    phy_exynos5_usbdrd(E) dwc3_exynos(E) ufs_exynos(E) i2c_exynos5(E)
    s3c2410_wdt(E) arm_dsu_pmu(E) simplefb(E)
  CPU: 0 UID: 0 PID: 885 Comm: irq/118-dwc3 Tainted: G            E
    6.14.0-rc3-next-20250220-4k-g50a0c754714a-dirty #1
    02ae1fc192b79fc15e3493a7f5cb2e58e2817b0a
  Tainted: [E]=UNSIGNED_MODULE
  Hardware name: Raven (DT)
  pstate: a04000c5 (NzCv daIF +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
  pc : composite_suspend+0x8c/0xe8
  lr : configfs_composite_suspend+0x70/0x80
  <snip>
  Call trace:
   composite_suspend+0x8c/0xe8 (P)
   configfs_composite_suspend+0x70/0x80
   dwc3_suspend_gadget+0x48/0x64
   dwc3_thread_interrupt+0x568/0xbe8
   irq_thread_fn+0x30/0xb0
   irq_thread+0x174/0x284
   kthread+0x130/0x21c
   ret_from_fork+0x10/0x20

And that decoded at base commit 50a0c754714a (from linux-next):

  composite_suspend (drivers/usb/gadget/composite.c:2619) (P)
  configfs_composite_suspend (drivers/usb/gadget/configfs.c:1939)
  dwc3_suspend_gadget (include/linux/spinlock.h:351 drivers/usb/dwc3/gadget.c:3962 drivers/usb/dwc3/gadget.c:3957)
  dwc3_thread_interrupt (drivers/usb/dwc3/gadget.c:4466 drivers/usb/dwc3/gadget.c:4494 drivers/usb/dwc3/gadget.c:4514 drivers/usb/dwc3/gadget.c:4535 drivers/usb/dwc3/gadget.c:4577)
  irq_thread_fn (kernel/irq/manage.c:1191)
  irq_thread (kernel/irq/manage.c:1318)
  kthread (kernel/kthread.c:464)
  ret_from_fork (arch/arm64/kernel/entry.S:863)

Thanks,
Will

> +
>  	usb_gadget_vbus_draw(gadget, 2);
>  }
>  
> @@ -2649,8 +2652,11 @@ void composite_resume(struct usb_gadget *gadget)
>  		else
>  			maxpower = min(maxpower, 900U);
>  
> -		if (maxpower > USB_SELF_POWER_VBUS_MAX_DRAW)
> +		if (maxpower > USB_SELF_POWER_VBUS_MAX_DRAW ||
> +		    !(cdev->config->bmAttributes & USB_CONFIG_ATT_SELFPOWER))
>  			usb_gadget_clear_selfpowered(gadget);
> +		else
> +			usb_gadget_set_selfpowered(gadget);
>  
>  		usb_gadget_vbus_draw(gadget, maxpower);
>  	} else {
> -- 
> 2.25.1
>
Re: [PATCH v2] usb: gadget: Set self-powered based on MaxPower and bmAttributes
Posted by Greg Kroah-Hartman 10 months ago 
On Thu, Feb 20, 2025 at 10:09:38AM -0800, William McVicker wrote:
> Hi Prashanth,
> 
> On 02/17/2025, Prashanth K wrote:
> > Currently the USB gadget will be set as bus-powered based solely
> > on whether its bMaxPower is greater than 100mA, but this may miss
> > devices that may legitimately draw less than 100mA but still want
> > to report as bus-powered. Similarly during suspend & resume, USB
> > gadget is incorrectly marked as bus/self powered without checking
> > the bmAttributes field. Fix these by configuring the USB gadget
> > as self or bus powered based on bmAttributes, and explicitly set
> > it as bus-powered if it draws more than 100mA.
> > 
> > Cc: stable@vger.kernel.org
> > Fixes: 5e5caf4fa8d3 ("usb: gadget: composite: Inform controller driver of self-powered")
> > Signed-off-by: Prashanth K <prashanth.k@oss.qualcomm.com>
> > ---
> > Changes in v2:
> > - Didn't change anything from RFC.
> > - Link to RFC: https://lore.kernel.org/all/20250204105908.2255686-1-prashanth.k@oss.qualcomm.com/
> > 
> >  drivers/usb/gadget/composite.c | 16 +++++++++++-----
> >  1 file changed, 11 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
> > index bdda8c74602d..1fb28bbf6c45 100644
> > --- a/drivers/usb/gadget/composite.c
> > +++ b/drivers/usb/gadget/composite.c
> > @@ -1050,10 +1050,11 @@ static int set_config(struct usb_composite_dev *cdev,
> >  	else
> >  		usb_gadget_set_remote_wakeup(gadget, 0);
> >  done:
> > -	if (power <= USB_SELF_POWER_VBUS_MAX_DRAW)
> > -		usb_gadget_set_selfpowered(gadget);
> > -	else
> > +	if (power > USB_SELF_POWER_VBUS_MAX_DRAW ||
> > +	    !(c->bmAttributes & USB_CONFIG_ATT_SELFPOWER))
> >  		usb_gadget_clear_selfpowered(gadget);
> > +	else
> > +		usb_gadget_set_selfpowered(gadget);
> >  
> >  	usb_gadget_vbus_draw(gadget, power);
> >  	if (result >= 0 && cdev->delayed_status)
> > @@ -2615,7 +2616,9 @@ void composite_suspend(struct usb_gadget *gadget)
> >  
> >  	cdev->suspended = 1;
> >  
> > -	usb_gadget_set_selfpowered(gadget);
> > +	if (cdev->config->bmAttributes & USB_CONFIG_ATT_SELFPOWER)
> > +		usb_gadget_set_selfpowered(gadget);
> 
> I'm hitting a null pointer derefence here on my Pixel 6 device on suspend.  I
> haven't dug deep into it how we get here, but in my case `cdev->config` is
> NULL. This happens immediate after booting my device. I verified that just
> adding a NULL check fixes the issue and dwc3 gadget can successfully suspend.

This was just fixed in my tree today with this commit:
	https://lore.kernel.org/r/20250220120314.3614330-1-m.szyprowski@samsung.com

Hope this helps,

greg k-h
Re: [PATCH v2] usb: gadget: Set self-powered based on MaxPower and bmAttributes
Posted by William McVicker 10 months ago 
On 02/20/2025, Greg Kroah-Hartman wrote:
> On Thu, Feb 20, 2025 at 10:09:38AM -0800, William McVicker wrote:
> > Hi Prashanth,
> > 
> > On 02/17/2025, Prashanth K wrote:
> > > Currently the USB gadget will be set as bus-powered based solely
> > > on whether its bMaxPower is greater than 100mA, but this may miss
> > > devices that may legitimately draw less than 100mA but still want
> > > to report as bus-powered. Similarly during suspend & resume, USB
> > > gadget is incorrectly marked as bus/self powered without checking
> > > the bmAttributes field. Fix these by configuring the USB gadget
> > > as self or bus powered based on bmAttributes, and explicitly set
> > > it as bus-powered if it draws more than 100mA.
> > > 
> > > Cc: stable@vger.kernel.org
> > > Fixes: 5e5caf4fa8d3 ("usb: gadget: composite: Inform controller driver of self-powered")
> > > Signed-off-by: Prashanth K <prashanth.k@oss.qualcomm.com>
> > > ---
> > > Changes in v2:
> > > - Didn't change anything from RFC.
> > > - Link to RFC: https://lore.kernel.org/all/20250204105908.2255686-1-prashanth.k@oss.qualcomm.com/
> > > 
> > >  drivers/usb/gadget/composite.c | 16 +++++++++++-----
> > >  1 file changed, 11 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
> > > index bdda8c74602d..1fb28bbf6c45 100644
> > > --- a/drivers/usb/gadget/composite.c
> > > +++ b/drivers/usb/gadget/composite.c
> > > @@ -1050,10 +1050,11 @@ static int set_config(struct usb_composite_dev *cdev,
> > >  	else
> > >  		usb_gadget_set_remote_wakeup(gadget, 0);
> > >  done:
> > > -	if (power <= USB_SELF_POWER_VBUS_MAX_DRAW)
> > > -		usb_gadget_set_selfpowered(gadget);
> > > -	else
> > > +	if (power > USB_SELF_POWER_VBUS_MAX_DRAW ||
> > > +	    !(c->bmAttributes & USB_CONFIG_ATT_SELFPOWER))
> > >  		usb_gadget_clear_selfpowered(gadget);
> > > +	else
> > > +		usb_gadget_set_selfpowered(gadget);
> > >  
> > >  	usb_gadget_vbus_draw(gadget, power);
> > >  	if (result >= 0 && cdev->delayed_status)
> > > @@ -2615,7 +2616,9 @@ void composite_suspend(struct usb_gadget *gadget)
> > >  
> > >  	cdev->suspended = 1;
> > >  
> > > -	usb_gadget_set_selfpowered(gadget);
> > > +	if (cdev->config->bmAttributes & USB_CONFIG_ATT_SELFPOWER)
> > > +		usb_gadget_set_selfpowered(gadget);
> > 
> > I'm hitting a null pointer derefence here on my Pixel 6 device on suspend.  I
> > haven't dug deep into it how we get here, but in my case `cdev->config` is
> > NULL. This happens immediate after booting my device. I verified that just
> > adding a NULL check fixes the issue and dwc3 gadget can successfully suspend.
> 
> This was just fixed in my tree today with this commit:
> 	https://lore.kernel.org/r/20250220120314.3614330-1-m.szyprowski@samsung.com
> 
> Hope this helps,
> 
> greg k-h

Yup, works for me. Thanks!

--Will

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.