Message-ID: <20240810005634.6ig2e3hdsgx3wkan@synopsys.com>
Date: Sat, 10 Aug 2024 00:56:46 +0000
From: Thinh Nguyen <Thinh.Nguyen@...opsys.com>
To: Kyle Tso <kyletso@...gle.com>
CC: Thinh Nguyen <Thinh.Nguyen@...opsys.com>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "raychi@...gle.com" <raychi@...gle.com>,
        "badhri@...gle.com" <badhri@...gle.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "royluo@...gle.com" <royluo@...gle.com>,
        "bvanassche@....org" <bvanassche@....org>,
        "stable@...r.kernel.org" <stable@...r.kernel.org>
Subject: Re: [PATCH v3] usb: dwc3: Runtime get and put usb power_supply handle

On Thu, Aug 08, 2024, Kyle Tso wrote:
> On Thu, Aug 8, 2024 at 9:38 AM Thinh Nguyen <Thinh.Nguyen@...opsys.com> wrote:
> >
> > On Wed, Aug 07, 2024, Kyle Tso wrote:
> > > On Wed, Aug 7, 2024 at 7:29 AM Thinh Nguyen <Thinh.Nguyen@...opsys.com> wrote:
> > > >
> > > > On Sun, Aug 04, 2024, Kyle Tso wrote:
> > > > > It is possible that the usb power_supply is registered after the probe
> > > >
> > > > Should we defer the dwc3 probe until the power_supply is registered
> > > > then?
> > > >
> > >
> > > We can do that, but getting the power_supply reference just before
> > > using the power_supply APIs is safer because we don't risk waiting for
> > > the registration of the usb power_supply. If vbus_draw is being called
> >
> > I'm a bit confused, wouldn't we need the power_supply to be registered
> > before you can get the reference. Can you clarify the risk here?
> >
> 
> I know it's weird, but usb power_supply module is not guaranteed to be
> loaded while dwc3 is being probed. What if, for example, it requires
> userspace to manually load the usb power_supply module. If we defer
> the probe just to wait for the usb power_supply, it might be waiting
> for a long time.

You still have not clarified what the risk is. How does "waiting for a
long time" impact the device functionality?

I'm not sure if there's a case where there's power_supply specified in
the devicetree but the dwc3 should not care whether it's available.
I need clarification if there's actually an actual device like this.

The current logic in dwc3 is that if power_supply is not available, then
return an error and it will not be available for the rest of the dwc3.
If it is available, keep it around until dwc3 is removed. Should we
enforce hard dependency on this? I need to know the above.

> 
> > > but the usb power_supply is still not ready, just let it fail without
> > > doing anything (only print the error logs). The usb gadget function
> > > still works. And once the usb power_supply is ready, the vbus_draw
> > > will be fine in following usb state changes.
> > >
> > > Moreover, all drivers using power_supply_get_by_name in the source
> > > tree adopt this way. IMO it should be okay.
> > >
> > > > > of dwc3. In this case, trying to get the usb power_supply during the
> > > > > probe will fail and there is no chance to try again. Also the usb
> > > > > power_supply might be unregistered at anytime so that the handle of it
> > > >
> > > > This is problematic... If the power_supply is unregistered, the device
> > > > is no longer usable.
> > > >
> > > > > in dwc3 would become invalid. To fix this, get the handle right before
> > > > > calling to power_supply functions and put it afterward.
> > > >
> > > > Shouldn't the life-cycle of the dwc3 match with the power_supply? How
> > > > can we maintain function without the proper power_supply?
> > > >
> > > > BR,
> > > > Thinh
> > > >
> > >
> > > usb power_supply is controlled by "another" driver which can be
> > > unloaded without notifying other drivers using it (such as dwc3).
> > > Unless there is a notification mechanism for the (un)registration of
> > > the power_supply class, getting/putting the reference right
> > > before/after calling the power_supply api is the best we can do for
> > > now.
> > >
> >
> > The power_supply driver should not be able to unload while the dwc3
> > holds the power_supply handle due to dependency between the two. Why
> > would we want to release the handle while dwc3 still needs it.
> >
> 
> It is possible. Calling power_supply_unregister only results in
> WARN_ON if the use_cnt is not equal to 1.
> 
> /**
>  * power_supply_unregister() - Remove this power supply from system
>  * @psy: Pointer to power supply to unregister
>  *
>  * Remove this power supply from the system. The resources of power supply
>  * will be freed here or on last power_supply_put() call.
>  */
> void power_supply_unregister(struct power_supply *psy)
> {
>     WARN_ON(atomic_dec_return(&psy->use_cnt));
> ...
> 

If you force remove the module while it's being in used, then yes that
would be a problem. But that's a decision by the user, and the user will
know about it. If things break, it's on the user. What you're doing now
is basically remove this protection and tell the user that this flow is
normal/ok when it may not.

> 
> > This creates an unpredictable behavior where sometime vbus can be drawn
> > and sometime it can't. Your specific gadget function may work for its
> > specific purpose, some other may not as its vbus_draw may be essential
> > for its application.
> >
> > BR,
> > Thinh
> 
> I agree that it might be unpredictable. But If we rely on the
> power_supply class to control the vbus_draw, it is the risk that we

If you rely on having power_supply, then enforce the dependency
properly. If you don't need it to function, then why do we need all this
change?

> need to take. I believe most of the time the usb power_supply would be
> there until some specific timing such as shutdown/reboot. This patch

What if the device is disconnected/re-enumerated or go into
suspend/resume? Now we don't have the power_supply to properly draw vbus
for the application.

> is only to handle the small chances that the usb power_supply is
> unregistered for some weird reason. It is better to give up the
> vbus_draw than dwc3 accessing an invalid reference.
> 

If we lose access to power_supply, we should not silently let it be.

BR,
Thinh

Powered by blists - more mailing lists