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
next prev parent reply other threads:[~2026-05-02 2:43 UTC|newest]
Thread overview: 7+ 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]
2026-05-06 22:59 ` Elson Serrao
2026-05-07 23:02 ` Thinh Nguyen
2026-05-08 0:28 ` Elson Serrao
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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.