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

[William McVicker <willmcvicker@google.com>]

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