The Linux Kernel Mailing List
 help / color / mirror / Atom feed
* Re: [PATCH] usb: dwc3: avoid probe deferral when USB power supply is not available
       [not found] ` <afVDFDK_cTO7rH2d@vbox>
@ 2026-05-06 22:59   ` Elson Serrao
  2026-05-07 23:02     ` Thinh Nguyen
  0 siblings, 1 reply; 3+ messages in thread
From: Elson Serrao @ 2026-05-06 22:59 UTC (permalink / raw)
  To: Thinh Nguyen
  Cc: Greg Kroah-Hartman, linux-usb@vger.kernel.org,
	linux-kernel@vger.kernel.org, jack.pham@oss.qualcomm.com,
	wesley.cheng@oss.qualcomm.com



On 5/1/2026 5:55 PM, Thinh Nguyen wrote:
> On Tue, Apr 07, 2026, Elson Serrao wrote:
>> The dwc3 driver currently defers probe if the USB power supply is not yet
>> registered. On some platforms, even though charging and power supply
>> functionality is available during normal operation, there may exist
>> minimal booting modes (such as recovery or diagnostic environments) where
>> the relevant USB power supply device is not registered. In such cases,
>> probe deferral prevents USB gadget operation entirely.
>>
>> USB data functionality for basic operation does not inherently depend on
>> the power supply framework, which is only required for enforcing VBUS
>> current control. The configured VBUS current limit is typically enforced
>> through the charger or PMIC power path. When charging functionality is
>> unavailable, applying a current limit has no practical effect, reducing
>> the benefit of strict probe-time enforcement in these environments.
>>
>> Instead of deferring probe, register a power supply notifier when the
>> USB power supply is not yet available. Cache the requested VBUS current
>> limit and apply it once the matching power supply becomes available, as
>> notified through the registered callback.
>>
> 
> This gets a bit cumbersome now that we need to consider some corner
> cases around the notifier callback.
> 
>> Signed-off-by: Elson Serrao <elson.serrao@oss.qualcomm.com>
>> ---
>>  drivers/usb/dwc3/core.c   | 82 ++++++++++++++++++++++++++++++++-------
>>  drivers/usb/dwc3/core.h   |  4 ++
>>  drivers/usb/dwc3/gadget.c | 10 ++++-
>>  3 files changed, 80 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>> index 161a4d58b2ce..20df0b287623 100644
>> --- a/drivers/usb/dwc3/core.c
>> +++ b/drivers/usb/dwc3/core.c
>> @@ -2167,24 +2167,72 @@ static void dwc3_vbus_draw_work(struct work_struct *work)
>>  	if (ret < 0)
>>  		dev_dbg(dwc->dev, "Error (%d) setting vbus draw (%d mA)\n",
>>  			ret, dwc->current_limit);
>> +
>> +	/* Unregister the psy notifier now that we have the power_supply reference */
>> +	if (dwc->psy_nb.notifier_call) {
> 
> Is it possible for dwc3_core_remove() to happen here? If so, should we
> do something about it?
> 

Hi Thinh

Thanks for the review.

Yes dwc3_core_remove() could race with this path.

To simplify things, I’m planning to unregister the notifier only
from dwc3_core_remove(), so we don’t need to handle this case here
and the notifier lifetime remains tied to the device lifecycle.

Let me know if you’d prefer a different approach.

>> +		power_supply_unreg_notifier(&dwc->psy_nb);
>> +		dwc->psy_nb.notifier_call = NULL;
>> +	}
>>  }
>>  
>> -static struct power_supply *dwc3_get_usb_power_supply(struct dwc3 *dwc)
>> +static int dwc3_psy_notifier(struct notifier_block *nb,
>> +			     unsigned long event, void *data)
>> +{
>> +	struct dwc3 *dwc = container_of(nb, struct dwc3, psy_nb);
>> +	struct power_supply *psy = data;
>> +	unsigned long flags;
>> +
>> +	if (strcmp(psy->desc->name, dwc->usb_psy_name) != 0)
>> +		return NOTIFY_DONE;
>> +
>> +	/* Explicitly get the reference for this psy */
>> +	psy = power_supply_get_by_name(dwc->usb_psy_name);
>> +	if (!psy)
>> +		return NOTIFY_DONE;
>> +
>> +	spin_lock_irqsave(&dwc->lock, flags);
>> +	/*
>> +	 * The USB power_supply may already be set. This can happen if notifier
>> +	 * callbacks for the USB power_supply race, or if a previous notifier
>> +	 * callback has already successfully fetched and associated the instance.
>> +	 * In such cases, release the newly acquired reference and ignore
>> +	 * subsequent notifications until the notifier is unregistered.
>> +	 */
>> +	if (dwc->usb_psy) {
>> +		spin_unlock_irqrestore(&dwc->lock, flags);
>> +		power_supply_put(psy);
>> +		return NOTIFY_DONE;
>> +	}
>> +
>> +	dwc->usb_psy = psy;
>> +	if (dwc->current_limit != UINT_MAX)
>> +		schedule_work(&dwc->vbus_draw_work);
>> +	spin_unlock_irqrestore(&dwc->lock, flags);
>> +
>> +	return NOTIFY_OK;
>> +}
>> +
>> +static void dwc3_get_usb_power_supply(struct dwc3 *dwc)
>>  {
>> -	struct power_supply *usb_psy;
>> -	const char *usb_psy_name;
>>  	int ret;
>>  
>> -	ret = device_property_read_string(dwc->dev, "usb-psy-name", &usb_psy_name);
>> +	ret = device_property_read_string(dwc->dev, "usb-psy-name", &dwc->usb_psy_name);
>>  	if (ret < 0)
>> -		return NULL;
>> -
>> -	usb_psy = power_supply_get_by_name(usb_psy_name);
>> -	if (!usb_psy)
>> -		return ERR_PTR(-EPROBE_DEFER);
>> +		return;
>>  
>>  	INIT_WORK(&dwc->vbus_draw_work, dwc3_vbus_draw_work);
>> -	return usb_psy;
>> +
>> +	dwc->usb_psy = power_supply_get_by_name(dwc->usb_psy_name);
>> +	if (!dwc->usb_psy) {
> 
> Is it possible for the power supply to register here?
> 

The power_supply framework introduces a ~10 ms delay [1]
before invoking notifiers after registration. So for the race described
above to occur, our probe would need to stall for more than that
duration between the initial power_supply_get_by_name() call
and notifier registration, which seems highly unlikely under normal
conditions.

That said, there is still a theoretical window where the power
supply could register in that gap. If you think it's worth being
defensive here, we could re-check power_supply_get_by_name() after
registering the notifier and handle the case accordingly.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/power/supply/power_supply_core.c?h=v7.1-rc2#n40

>> +		dwc->current_limit = UINT_MAX;
>> +		dwc->psy_nb.notifier_call = dwc3_psy_notifier;
>> +		ret = power_supply_reg_notifier(&dwc->psy_nb);
>> +		if (ret) {
>> +			dev_err(dwc->dev, "Failed to register power supply notifier: %d\n", ret);
>> +			dwc->psy_nb.notifier_call = NULL;
>> +			return;
>> +		}
>> +	}
>>  }
>>  
>>  int dwc3_core_probe(const struct dwc3_probe_data *data)
>> @@ -2232,9 +2280,9 @@ int dwc3_core_probe(const struct dwc3_probe_data *data)
>>  
>>  	dwc3_get_software_properties(dwc, &data->properties);
>>  
>> -	dwc->usb_psy = dwc3_get_usb_power_supply(dwc);
>> -	if (IS_ERR(dwc->usb_psy))
>> -		return dev_err_probe(dev, PTR_ERR(dwc->usb_psy), "couldn't get usb power supply\n");
>> +	spin_lock_init(&dwc->lock);
>> +
>> +	dwc3_get_usb_power_supply(dwc);
>>  
>>  	if (!data->ignore_clocks_and_resets) {
>>  		dwc->reset = devm_reset_control_array_get_optional_shared(dev);
>> @@ -2286,7 +2334,6 @@ int dwc3_core_probe(const struct dwc3_probe_data *data)
>>  		dwc->num_usb3_ports = 1;
>>  	}
>>  
>> -	spin_lock_init(&dwc->lock);
>>  	mutex_init(&dwc->mutex);
>>  
>>  	pm_runtime_get_noresume(dev);
>> @@ -2354,6 +2401,8 @@ int dwc3_core_probe(const struct dwc3_probe_data *data)
>>  err_assert_reset:
>>  	reset_control_assert(dwc->reset);
>>  err_put_psy:
>> +	if (dwc->psy_nb.notifier_call)
>> +		power_supply_unreg_notifier(&dwc->psy_nb);
>>  	if (dwc->usb_psy)
>>  		power_supply_put(dwc->usb_psy);
>>  
>> @@ -2410,6 +2459,11 @@ void dwc3_core_remove(struct dwc3 *dwc)
>>  
>>  	dwc3_free_event_buffers(dwc);
>>  
>> +	if (dwc->psy_nb.notifier_call) {
>> +		power_supply_unreg_notifier(&dwc->psy_nb);
>> +		dwc->psy_nb.notifier_call = NULL;
>> +	}
>> +
>>  	if (dwc->usb_psy) {
>>  		cancel_work_sync(&dwc->vbus_draw_work);
>>  		power_supply_put(dwc->usb_psy);
>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
>> index a35b3db1f9f3..68171629c7bf 100644
>> --- a/drivers/usb/dwc3/core.h
>> +++ b/drivers/usb/dwc3/core.h
>> @@ -1058,6 +1058,8 @@ struct dwc3_glue_ops {
>>   * @role_switch_default_mode: default operation mode of controller while
>>   *			usb role is USB_ROLE_NONE.
>>   * @usb_psy: pointer to power supply interface.
>> + * @usb_psy_name: name of the USB power supply
>> + * @psy_nb: power supply notifier block
>>   * @vbus_draw_work: Work to set the vbus drawing limit
>>   * @current_limit: How much current to draw from vbus, in milliAmperes.
>>   * @usb2_phy: pointer to USB2 PHY
>> @@ -1246,6 +1248,8 @@ struct dwc3 {
>>  	enum usb_dr_mode	role_switch_default_mode;
>>  
>>  	struct power_supply	*usb_psy;
>> +	const char		*usb_psy_name;
>> +	struct notifier_block	psy_nb;
>>  	struct work_struct	vbus_draw_work;
>>  	unsigned int		current_limit;
> 
> Would be nice to group these in a struct, but it's fine as is for now.
> 
>>  
>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> index 0a688904ce8c..4717c251596d 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
>> @@ -3124,15 +3124,21 @@ static void dwc3_gadget_set_ssp_rate(struct usb_gadget *g,
>>  static int dwc3_gadget_vbus_draw(struct usb_gadget *g, unsigned int mA)
>>  {
>>  	struct dwc3		*dwc = gadget_to_dwc(g);
>> +	unsigned long		flags;
>>  
>>  	if (dwc->usb2_phy)
>>  		return usb_phy_set_power(dwc->usb2_phy, mA);
>>  
>> -	if (!dwc->usb_psy)
>> +	spin_lock_irqsave(&dwc->lock, flags);
>> +	dwc->current_limit = mA;
>> +	if (!dwc->usb_psy) {
>> +		spin_unlock_irqrestore(&dwc->lock, flags);
>> +		dev_dbg(dwc->dev, "Stored VBUS draw: %u mA (power supply not ready)\n", mA);
>>  		return -EOPNOTSUPP;
>> +	}
>>  
>> -	dwc->current_limit = mA;
>>  	schedule_work(&dwc->vbus_draw_work);
>> +	spin_unlock_irqrestore(&dwc->lock, flags);
>>  
>>  	return 0;
>>  }
>> -- 
>> 2.34.1
>>
> 
> So, now if vbus_draw() is called without power supply registered, the
> composite framework will just report 0mA because we don't have the power
> supply yet? 
> 

Yes, if vbus_draw() is called before the power supply is registered,
gadget->mA will not be updated since we return an error, so it may
appear as 0mA in traces (along with the error code) [1].

However, the composite framework has already determined the
appropriate current limit before calling vbus_draw(), and does not
rely on this gadget op for that decision. 

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/usb/gadget/udc/core.c?h=v7.1-rc2#n645

Thanks
Elson

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

* Re: [PATCH] usb: dwc3: avoid probe deferral when USB power supply is not available
  2026-05-06 22:59   ` [PATCH] usb: dwc3: avoid probe deferral when USB power supply is not available Elson Serrao
@ 2026-05-07 23:02     ` Thinh Nguyen
  2026-05-08  0:28       ` Elson Serrao
  0 siblings, 1 reply; 3+ messages in thread
From: Thinh Nguyen @ 2026-05-07 23:02 UTC (permalink / raw)
  To: Elson Serrao
  Cc: Thinh Nguyen, Greg Kroah-Hartman, linux-usb@vger.kernel.org,
	linux-kernel@vger.kernel.org, jack.pham@oss.qualcomm.com,
	wesley.cheng@oss.qualcomm.com

On Wed, May 06, 2026, Elson Serrao wrote:
> 
> 
> On 5/1/2026 5:55 PM, Thinh Nguyen wrote:
> > On Tue, Apr 07, 2026, Elson Serrao wrote:
> >> The dwc3 driver currently defers probe if the USB power supply is not yet
> >> registered. On some platforms, even though charging and power supply
> >> functionality is available during normal operation, there may exist
> >> minimal booting modes (such as recovery or diagnostic environments) where
> >> the relevant USB power supply device is not registered. In such cases,
> >> probe deferral prevents USB gadget operation entirely.
> >>
> >> USB data functionality for basic operation does not inherently depend on
> >> the power supply framework, which is only required for enforcing VBUS
> >> current control. The configured VBUS current limit is typically enforced
> >> through the charger or PMIC power path. When charging functionality is
> >> unavailable, applying a current limit has no practical effect, reducing
> >> the benefit of strict probe-time enforcement in these environments.
> >>
> >> Instead of deferring probe, register a power supply notifier when the
> >> USB power supply is not yet available. Cache the requested VBUS current
> >> limit and apply it once the matching power supply becomes available, as
> >> notified through the registered callback.
> >>
> > 
> > This gets a bit cumbersome now that we need to consider some corner
> > cases around the notifier callback.
> > 
> >> Signed-off-by: Elson Serrao <elson.serrao@oss.qualcomm.com>
> >> ---
> >>  drivers/usb/dwc3/core.c   | 82 ++++++++++++++++++++++++++++++++-------
> >>  drivers/usb/dwc3/core.h   |  4 ++
> >>  drivers/usb/dwc3/gadget.c | 10 ++++-
> >>  3 files changed, 80 insertions(+), 16 deletions(-)
> >>
> >> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> >> index 161a4d58b2ce..20df0b287623 100644
> >> --- a/drivers/usb/dwc3/core.c
> >> +++ b/drivers/usb/dwc3/core.c
> >> @@ -2167,24 +2167,72 @@ static void dwc3_vbus_draw_work(struct work_struct *work)
> >>  	if (ret < 0)
> >>  		dev_dbg(dwc->dev, "Error (%d) setting vbus draw (%d mA)\n",
> >>  			ret, dwc->current_limit);
> >> +
> >> +	/* Unregister the psy notifier now that we have the power_supply reference */
> >> +	if (dwc->psy_nb.notifier_call) {
> > 
> > Is it possible for dwc3_core_remove() to happen here? If so, should we
> > do something about it?
> > 
> 
> Hi Thinh
> 
> Thanks for the review.
> 
> Yes dwc3_core_remove() could race with this path.
> 
> To simplify things, I’m planning to unregister the notifier only
> from dwc3_core_remove(), so we don’t need to handle this case here
> and the notifier lifetime remains tied to the device lifecycle.
> 
> Let me know if you’d prefer a different approach.
> 

That's fine to me. Just make sure to return early if the power supply is
registered.

<snip>

> >> +
> >> +	dwc->usb_psy = power_supply_get_by_name(dwc->usb_psy_name);
> >> +	if (!dwc->usb_psy) {
> > 
> > Is it possible for the power supply to register here?
> > 
> 
> The power_supply framework introduces a ~10 ms delay [1]
> before invoking notifiers after registration. So for the race described
> above to occur, our probe would need to stall for more than that
> duration between the initial power_supply_get_by_name() call
> and notifier registration, which seems highly unlikely under normal
> conditions.
> 
> That said, there is still a theoretical window where the power
> supply could register in that gap. If you think it's worth being
> defensive here, we could re-check power_supply_get_by_name() after
> registering the notifier and handle the case accordingly.
> 
> [1] https://urldefense.com/v3/__https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/power/supply/power_supply_core.c?h=v7.1-rc2*n40__;Iw!!A4F2R9G_pg!dUiPW6mibrvsk4uGO4MnGVg3R1zR3EmxxIROrw4N-ytHZq7N9q-V6irNAWrBrolUR2HABsAGSQoMPzGnEGQsvWdhzWzcVHOU$ 
> 

For my own sanity, can we have that extra check? Otherwise, every time I
scan through this I would need to recall why it wasn't needed.

Thanks,
Thinh

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

* Re: [PATCH] usb: dwc3: avoid probe deferral when USB power supply is not available
  2026-05-07 23:02     ` Thinh Nguyen
@ 2026-05-08  0:28       ` Elson Serrao
  0 siblings, 0 replies; 3+ messages in thread
From: Elson Serrao @ 2026-05-08  0:28 UTC (permalink / raw)
  To: Thinh Nguyen
  Cc: Greg Kroah-Hartman, linux-usb@vger.kernel.org,
	linux-kernel@vger.kernel.org, jack.pham@oss.qualcomm.com,
	wesley.cheng@oss.qualcomm.com



On 5/7/2026 4:02 PM, Thinh Nguyen wrote:
> On Wed, May 06, 2026, Elson Serrao wrote:
>>
>>
>> On 5/1/2026 5:55 PM, Thinh Nguyen wrote:
>>> On Tue, Apr 07, 2026, Elson Serrao wrote:
>>>> The dwc3 driver currently defers probe if the USB power supply is not yet
>>>> registered. On some platforms, even though charging and power supply
>>>> functionality is available during normal operation, there may exist
>>>> minimal booting modes (such as recovery or diagnostic environments) where
>>>> the relevant USB power supply device is not registered. In such cases,
>>>> probe deferral prevents USB gadget operation entirely.
>>>>
>>>> USB data functionality for basic operation does not inherently depend on
>>>> the power supply framework, which is only required for enforcing VBUS
>>>> current control. The configured VBUS current limit is typically enforced
>>>> through the charger or PMIC power path. When charging functionality is
>>>> unavailable, applying a current limit has no practical effect, reducing
>>>> the benefit of strict probe-time enforcement in these environments.
>>>>
>>>> Instead of deferring probe, register a power supply notifier when the
>>>> USB power supply is not yet available. Cache the requested VBUS current
>>>> limit and apply it once the matching power supply becomes available, as
>>>> notified through the registered callback.
>>>>
>>>
>>> This gets a bit cumbersome now that we need to consider some corner
>>> cases around the notifier callback.
>>>
>>>> Signed-off-by: Elson Serrao <elson.serrao@oss.qualcomm.com>
>>>> ---
>>>>  drivers/usb/dwc3/core.c   | 82 ++++++++++++++++++++++++++++++++-------
>>>>  drivers/usb/dwc3/core.h   |  4 ++
>>>>  drivers/usb/dwc3/gadget.c | 10 ++++-
>>>>  3 files changed, 80 insertions(+), 16 deletions(-)
>>>>
>>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>>> index 161a4d58b2ce..20df0b287623 100644
>>>> --- a/drivers/usb/dwc3/core.c
>>>> +++ b/drivers/usb/dwc3/core.c
>>>> @@ -2167,24 +2167,72 @@ static void dwc3_vbus_draw_work(struct work_struct *work)
>>>>  	if (ret < 0)
>>>>  		dev_dbg(dwc->dev, "Error (%d) setting vbus draw (%d mA)\n",
>>>>  			ret, dwc->current_limit);
>>>> +
>>>> +	/* Unregister the psy notifier now that we have the power_supply reference */
>>>> +	if (dwc->psy_nb.notifier_call) {
>>>
>>> Is it possible for dwc3_core_remove() to happen here? If so, should we
>>> do something about it?
>>>
>>
>> Hi Thinh
>>
>> Thanks for the review.
>>
>> Yes dwc3_core_remove() could race with this path.
>>
>> To simplify things, I’m planning to unregister the notifier only
>> from dwc3_core_remove(), so we don’t need to handle this case here
>> and the notifier lifetime remains tied to the device lifecycle.
>>
>> Let me know if you’d prefer a different approach.
>>
> 
> That's fine to me. Just make sure to return early if the power supply is
> registered.
> 

Ack. I will fix this in v2

> <snip>
> 
>>>> +
>>>> +	dwc->usb_psy = power_supply_get_by_name(dwc->usb_psy_name);
>>>> +	if (!dwc->usb_psy) {
>>>
>>> Is it possible for the power supply to register here?
>>>
>>
>> The power_supply framework introduces a ~10 ms delay [1]
>> before invoking notifiers after registration. So for the race described
>> above to occur, our probe would need to stall for more than that
>> duration between the initial power_supply_get_by_name() call
>> and notifier registration, which seems highly unlikely under normal
>> conditions.
>>
>> That said, there is still a theoretical window where the power
>> supply could register in that gap. If you think it's worth being
>> defensive here, we could re-check power_supply_get_by_name() after
>> registering the notifier and handle the case accordingly.
>>
>> [1] https://urldefense.com/v3/__https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/power/supply/power_supply_core.c?h=v7.1-rc2*n40__;Iw!!A4F2R9G_pg!dUiPW6mibrvsk4uGO4MnGVg3R1zR3EmxxIROrw4N-ytHZq7N9q-V6irNAWrBrolUR2HABsAGSQoMPzGnEGQsvWdhzWzcVHOU$ 
>>
> 
> For my own sanity, can we have that extra check? Otherwise, every time I
> scan through this I would need to recall why it wasn't needed.
> 

Ack. I will fix this in v2

Thanks
Elson

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

end of thread, other threads:[~2026-05-08  0:28 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
     [not found] <20260407232410.4101455-1-elson.serrao@oss.qualcomm.com>
     [not found] ` <afVDFDK_cTO7rH2d@vbox>
2026-05-06 22:59   ` [PATCH] usb: dwc3: avoid probe deferral when USB power supply is not available Elson Serrao
2026-05-07 23:02     ` Thinh Nguyen
2026-05-08  0:28       ` Elson Serrao

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox