Linux USB
 help / color / mirror / Atom feed
From: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
To: Elson Serrao <elson.serrao@oss.qualcomm.com>
Cc: Thinh Nguyen <Thinh.Nguyen@synopsys.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	"linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"jack.pham@oss.qualcomm.com" <jack.pham@oss.qualcomm.com>,
	"wesley.cheng@oss.qualcomm.com" <wesley.cheng@oss.qualcomm.com>
Subject: Re: [PATCH v2] usb: dwc3: avoid probe deferral when USB power supply is not available
Date: Wed, 27 May 2026 23:23:32 +0000	[thread overview]
Message-ID: <ahdzhu-wahIyD7eR@vbox> (raw)
In-Reply-To: <20260526183016.3501307-1-elson.serrao@oss.qualcomm.com>

On Tue, May 26, 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.
> 
> Signed-off-by: Elson Serrao <elson.serrao@oss.qualcomm.com>
> ---
> Changes in v2:
>  - Removed notifier unregistration from the vbus_draw work function to
>    avoid a race with remove callback.
>  - Added an early psy registration check in the notifier callback.
>  - Moved power supply registration check after notifier registration
>    in dwc3_get_usb_power_supply() to address the race identified in v1.
>  - Link to v1: https://urldefense.com/v3/__https://lore.kernel.org/all/20260407232410.4101455-1-elson.serrao@oss.qualcomm.com/__;!!A4F2R9G_pg!dSMtLrX-mLTOSz0TpKE_P1WkMg9odkCd4am7BgM94g3Gcf8x3bbgjSpdry7eAcFusLByXqMWEpfdzqArp0Bir8OKctBmzEPi$ 
> ---
>  drivers/usb/dwc3/core.c   | 99 +++++++++++++++++++++++++++++++++------
>  drivers/usb/dwc3/core.h   |  4 ++
>  drivers/usb/dwc3/gadget.c | 10 +++-
>  3 files changed, 97 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 65213896de99..c035b5fbfb2f 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -2192,22 +2192,89 @@ static void dwc3_vbus_draw_work(struct work_struct *work)
>  			ret, dwc->current_limit);
>  }
>  
> -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 power_supply *usb_psy;
> -	const char *usb_psy_name;
> +	struct dwc3 *dwc = container_of(nb, struct dwc3, psy_nb);
> +	struct power_supply *psy = data;
> +	unsigned long flags;
> +
> +	if (dwc->usb_psy)
> +		return NOTIFY_DONE;
> +
> +	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)

Create a macro for these kinds of checks for readability:

#define DWC3_CURRENT_UNSPECIFIED 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 *psy;
> +	unsigned long flags;
>  	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->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;
> +	}
> +
> +	psy = power_supply_get_by_name(dwc->usb_psy_name);
> +	if (!psy)
> +		return;
> +
> +	/* Unregister the notifier now that we have the power supply */
> +	power_supply_unreg_notifier(&dwc->psy_nb);
> +	dwc->psy_nb.notifier_call = NULL;
> +
> +	spin_lock_irqsave(&dwc->lock, flags);
> +	/*
> +	 * It is possible that the notifier callback ran before we reached here
> +	 * and successfully fetched the power supply. In that case we need to
> +	 * release the above reference.
> +	 */
> +	if (dwc->usb_psy) {
> +		spin_unlock_irqrestore(&dwc->lock, flags);
> +		power_supply_put(psy);
> +		return;
> +	}
> +
> +	dwc->usb_psy = psy;
> +	spin_unlock_irqrestore(&dwc->lock, flags);
>  }
>  
>  int dwc3_core_probe(const struct dwc3_probe_data *data)
> @@ -2255,9 +2322,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);
> @@ -2309,7 +2376,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);
> @@ -2377,6 +2443,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);
>  
> @@ -2433,6 +2501,9 @@ 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);
> +
>  	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 e0dee9d28740..4854cfdbc64a 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -1059,6 +1059,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
> @@ -1251,6 +1253,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;
>  
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 3d4ca68e584c..303598048e9a 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);

Can we use the check if dwc->psy_nb.notifier_block is set to determine
if we expect to have a power_supply? Then we can print the message above
when it's really not ready, and only return -EOPNOTSUPP if we really
don't have a power_supply.

>  		return -EOPNOTSUPP;
> +	}
>  
> -	dwc->current_limit = mA;
>  	schedule_work(&dwc->vbus_draw_work);
> +	spin_unlock_irqrestore(&dwc->lock, flags);
>  
>  	return 0;
>  }
> -- 
> 2.34.1
> 

The rest looks good to me.

Thanks,
Thinh

      reply	other threads:[~2026-05-27 23:23 UTC|newest]

Thread overview: 2+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-05-26 18:30 [PATCH v2] usb: dwc3: avoid probe deferral when USB power supply is not available Elson Serrao
2026-05-27 23:23 ` Thinh Nguyen [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=ahdzhu-wahIyD7eR@vbox \
    --to=thinh.nguyen@synopsys.com \
    --cc=elson.serrao@oss.qualcomm.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=jack.pham@oss.qualcomm.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-usb@vger.kernel.org \
    --cc=wesley.cheng@oss.qualcomm.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox