On Thu, Aug 8, 2024 at 9:38 AM Thinh Nguyen <Thinh.Nguyen@xxxxxxxxxxxx> wrote:
>
> On Wed, Aug 07, 2024, Kyle Tso wrote:
> > On Wed, Aug 7, 2024 at 7:29 AM Thinh Nguyen <Thinh.Nguyen@xxxxxxxxxxxx> 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.
> > 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));
...
> 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
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
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.
Kyle
