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] usb: dwc3: avoid probe deferral when USB power supply is not available
Date: Sat, 2 May 2026 00:55:57 +0000 [thread overview]
Message-ID: <afVDFDK_cTO7rH2d@vbox> (raw)
In-Reply-To: <20260407232410.4101455-1-elson.serrao@oss.qualcomm.com>
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?
> + 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?
> + 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? Beside a couple of comments above, it looks fine to me.
Thanks,
Thinh
prev parent reply other threads:[~2026-05-02 2:43 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-04-07 23:24 [PATCH] usb: dwc3: avoid probe deferral when USB power supply is not available Elson Serrao
2026-04-29 17:52 ` Elson Serrao
2026-04-30 7:43 ` Thinh Nguyen
2026-05-02 0:55 ` 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=afVDFDK_cTO7rH2d@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