| Message ID | 20240804084612.2561230-1-kyletso@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] usb: dwc3: Runtime get and put usb power_supply handle | expand |
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? > 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 > > dwc3_gadet_vbus_draw might be in interrupt context. Create a kthread > worker beforehand and use it to process the "might-sleep" > power_supply_put ASAP after the property set. > > Fixes: 6f0764b5adea ("usb: dwc3: add a power supply for current control") > Cc: stable@vger.kernel.org > Signed-off-by: Kyle Tso <kyletso@google.com> > --- > v2 -> v3: > - Only move power_supply_put to a work. Still call _get_by_name and > _set_property in dwc3_gadget_vbus_draw. > - Create a kthread_worker to handle the work > > v1 -> v2: > - move power_supply_put out of interrupt context > > drivers/usb/dwc3/core.c | 29 ++++++++++++---------------- > drivers/usb/dwc3/core.h | 6 ++++-- > drivers/usb/dwc3/gadget.c | 40 +++++++++++++++++++++++++++++++++++---- > 3 files changed, 52 insertions(+), 23 deletions(-) > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > index 734de2a8bd21..82c8376330d7 100644 > --- a/drivers/usb/dwc3/core.c > +++ b/drivers/usb/dwc3/core.c > @@ -1631,8 +1631,6 @@ static void dwc3_get_properties(struct dwc3 *dwc) > u8 tx_thr_num_pkt_prd = 0; > u8 tx_max_burst_prd = 0; > u8 tx_fifo_resize_max_num; > - const char *usb_psy_name; > - int ret; > > /* default to highest possible threshold */ > lpm_nyet_threshold = 0xf; > @@ -1667,12 +1665,7 @@ static void dwc3_get_properties(struct dwc3 *dwc) > > dwc->sys_wakeup = device_may_wakeup(dwc->sysdev); > > - ret = device_property_read_string(dev, "usb-psy-name", &usb_psy_name); > - if (ret >= 0) { > - dwc->usb_psy = power_supply_get_by_name(usb_psy_name); > - if (!dwc->usb_psy) > - dev_err(dev, "couldn't get usb power supply\n"); > - } > + device_property_read_string(dev, "usb-psy-name", &dwc->usb_psy_name); > > dwc->has_lpm_erratum = device_property_read_bool(dev, > "snps,has-lpm-erratum"); > @@ -2132,19 +2125,24 @@ static int dwc3_probe(struct platform_device *pdev) > > dwc3_get_software_properties(dwc); > > + dwc->worker = kthread_create_worker(0, "dwc3-worker"); > + if (IS_ERR(dwc->worker)) > + return PTR_ERR(dwc->worker); > + sched_set_fifo(dwc->worker->task); > + > dwc->reset = devm_reset_control_array_get_optional_shared(dev); > if (IS_ERR(dwc->reset)) { > ret = PTR_ERR(dwc->reset); > - goto err_put_psy; > + goto err_destroy_worker; > } > > ret = dwc3_get_clocks(dwc); > if (ret) > - goto err_put_psy; > + goto err_destroy_worker; > > ret = reset_control_deassert(dwc->reset); > if (ret) > - goto err_put_psy; > + goto err_destroy_worker; > > ret = dwc3_clk_enable(dwc); > if (ret) > @@ -2245,9 +2243,8 @@ static int dwc3_probe(struct platform_device *pdev) > dwc3_clk_disable(dwc); > err_assert_reset: > reset_control_assert(dwc->reset); > -err_put_psy: > - if (dwc->usb_psy) > - power_supply_put(dwc->usb_psy); > +err_destroy_worker: > + kthread_destroy_worker(dwc->worker); > > return ret; > } > @@ -2258,6 +2255,7 @@ static void dwc3_remove(struct platform_device *pdev) > > pm_runtime_get_sync(&pdev->dev); > > + kthread_destroy_worker(dwc->worker); > dwc3_core_exit_mode(dwc); > dwc3_debugfs_exit(dwc); > > @@ -2276,9 +2274,6 @@ static void dwc3_remove(struct platform_device *pdev) > pm_runtime_set_suspended(&pdev->dev); > > dwc3_free_event_buffers(dwc); > - > - if (dwc->usb_psy) > - power_supply_put(dwc->usb_psy); > } > > #ifdef CONFIG_PM > diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h > index 1e561fd8b86e..3fc58204db6e 100644 > --- a/drivers/usb/dwc3/core.h > +++ b/drivers/usb/dwc3/core.h > @@ -993,6 +993,7 @@ struct dwc3_scratchpad_array { > /** > * struct dwc3 - representation of our controller > * @drd_work: workqueue used for role swapping > + * @worker: dedicated kthread worker > * @ep0_trb: trb which is used for the ctrl_req > * @bounce: address of bounce buffer > * @setup_buf: used while precessing STD USB requests > @@ -1045,7 +1046,7 @@ struct dwc3_scratchpad_array { > * @role_sw: usb_role_switch handle > * @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 interface > * @usb2_phy: pointer to USB2 PHY > * @usb3_phy: pointer to USB3 PHY > * @usb2_generic_phy: pointer to array of USB2 PHYs > @@ -1163,6 +1164,7 @@ struct dwc3_scratchpad_array { > */ > struct dwc3 { > struct work_struct drd_work; > + struct kthread_worker *worker; > struct dwc3_trb *ep0_trb; > void *bounce; > u8 *setup_buf; > @@ -1223,7 +1225,7 @@ struct dwc3 { > struct usb_role_switch *role_sw; > enum usb_dr_mode role_switch_default_mode; > > - struct power_supply *usb_psy; > + const char *usb_psy_name; > > u32 fladj; > u32 ref_clk_per; > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > index 89fc690fdf34..1ff583281eff 100644 > --- a/drivers/usb/dwc3/gadget.c > +++ b/drivers/usb/dwc3/gadget.c > @@ -30,6 +30,11 @@ > #define DWC3_ALIGN_FRAME(d, n) (((d)->frame_number + ((d)->interval * (n))) \ > & ~((d)->interval - 1)) > > +struct dwc3_psy_put { > + struct kthread_work work; > + struct power_supply *psy; > +}; > + > /** > * dwc3_gadget_set_test_mode - enables usb2 test modes > * @dwc: pointer to our context structure > @@ -3047,22 +3052,49 @@ static void dwc3_gadget_set_ssp_rate(struct usb_gadget *g, > spin_unlock_irqrestore(&dwc->lock, flags); > } > > +static void dwc3_gadget_psy_put(struct kthread_work *work) > +{ > + struct dwc3_psy_put *psy_put = container_of(work, struct dwc3_psy_put, work); > + > + power_supply_put(psy_put->psy); > + kfree(psy_put); > +} > + > static int dwc3_gadget_vbus_draw(struct usb_gadget *g, unsigned int mA) > { > - struct dwc3 *dwc = gadget_to_dwc(g); > + struct dwc3 *dwc = gadget_to_dwc(g); > + struct power_supply *usb_psy; > union power_supply_propval val = {0}; > + struct dwc3_psy_put *psy_put; > int ret; > > if (dwc->usb2_phy) > return usb_phy_set_power(dwc->usb2_phy, mA); > > - if (!dwc->usb_psy) > + if (!dwc->usb_psy_name) > return -EOPNOTSUPP; > > + usb_psy = power_supply_get_by_name(dwc->usb_psy_name); > + if (!usb_psy) { > + dev_err(dwc->dev, "couldn't get usb power supply\n"); > + return -ENODEV; > + } > + > val.intval = 1000 * mA; > - ret = power_supply_set_property(dwc->usb_psy, POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT, &val); > + ret = power_supply_set_property(usb_psy, POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT, &val); > + if (ret < 0) { > + dev_err(dwc->dev, "failed to set power supply property\n"); > + return ret; > + } > > - return ret; > + psy_put = kzalloc(sizeof(*psy_put), GFP_ATOMIC); > + if (!psy_put) > + return -ENOMEM; > + kthread_init_work(&psy_put->work, dwc3_gadget_psy_put); > + psy_put->psy = usb_psy; > + kthread_queue_work(dwc->worker, &psy_put->work); > + > + return 0; > } > > /** > -- > 2.46.0.rc2.264.g509ed76dc8-goog >
On Wed, Aug 7, 2024 at 7:29 AM Thinh Nguyen <Thinh.Nguyen@synopsys.com> 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 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. Kyle > > > > dwc3_gadet_vbus_draw might be in interrupt context. Create a kthread > > worker beforehand and use it to process the "might-sleep" > > power_supply_put ASAP after the property set. > > > > Fixes: 6f0764b5adea ("usb: dwc3: add a power supply for current control") > > Cc: stable@vger.kernel.org > > Signed-off-by: Kyle Tso <kyletso@google.com> > > --- > > v2 -> v3: > > - Only move power_supply_put to a work. Still call _get_by_name and > > _set_property in dwc3_gadget_vbus_draw. > > - Create a kthread_worker to handle the work > > > > v1 -> v2: > > - move power_supply_put out of interrupt context > > > > drivers/usb/dwc3/core.c | 29 ++++++++++++---------------- > > drivers/usb/dwc3/core.h | 6 ++++-- > > drivers/usb/dwc3/gadget.c | 40 +++++++++++++++++++++++++++++++++++---- > > 3 files changed, 52 insertions(+), 23 deletions(-) > > > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > > index 734de2a8bd21..82c8376330d7 100644 > > --- a/drivers/usb/dwc3/core.c > > +++ b/drivers/usb/dwc3/core.c > > @@ -1631,8 +1631,6 @@ static void dwc3_get_properties(struct dwc3 *dwc) > > u8 tx_thr_num_pkt_prd = 0; > > u8 tx_max_burst_prd = 0; > > u8 tx_fifo_resize_max_num; > > - const char *usb_psy_name; > > - int ret; > > > > /* default to highest possible threshold */ > > lpm_nyet_threshold = 0xf; > > @@ -1667,12 +1665,7 @@ static void dwc3_get_properties(struct dwc3 *dwc) > > > > dwc->sys_wakeup = device_may_wakeup(dwc->sysdev); > > > > - ret = device_property_read_string(dev, "usb-psy-name", &usb_psy_name); > > - if (ret >= 0) { > > - dwc->usb_psy = power_supply_get_by_name(usb_psy_name); > > - if (!dwc->usb_psy) > > - dev_err(dev, "couldn't get usb power supply\n"); > > - } > > + device_property_read_string(dev, "usb-psy-name", &dwc->usb_psy_name); > > > > dwc->has_lpm_erratum = device_property_read_bool(dev, > > "snps,has-lpm-erratum"); > > @@ -2132,19 +2125,24 @@ static int dwc3_probe(struct platform_device *pdev) > > > > dwc3_get_software_properties(dwc); > > > > + dwc->worker = kthread_create_worker(0, "dwc3-worker"); > > + if (IS_ERR(dwc->worker)) > > + return PTR_ERR(dwc->worker); > > + sched_set_fifo(dwc->worker->task); > > + > > dwc->reset = devm_reset_control_array_get_optional_shared(dev); > > if (IS_ERR(dwc->reset)) { > > ret = PTR_ERR(dwc->reset); > > - goto err_put_psy; > > + goto err_destroy_worker; > > } > > > > ret = dwc3_get_clocks(dwc); > > if (ret) > > - goto err_put_psy; > > + goto err_destroy_worker; > > > > ret = reset_control_deassert(dwc->reset); > > if (ret) > > - goto err_put_psy; > > + goto err_destroy_worker; > > > > ret = dwc3_clk_enable(dwc); > > if (ret) > > @@ -2245,9 +2243,8 @@ static int dwc3_probe(struct platform_device *pdev) > > dwc3_clk_disable(dwc); > > err_assert_reset: > > reset_control_assert(dwc->reset); > > -err_put_psy: > > - if (dwc->usb_psy) > > - power_supply_put(dwc->usb_psy); > > +err_destroy_worker: > > + kthread_destroy_worker(dwc->worker); > > > > return ret; > > } > > @@ -2258,6 +2255,7 @@ static void dwc3_remove(struct platform_device *pdev) > > > > pm_runtime_get_sync(&pdev->dev); > > > > + kthread_destroy_worker(dwc->worker); > > dwc3_core_exit_mode(dwc); > > dwc3_debugfs_exit(dwc); > > > > @@ -2276,9 +2274,6 @@ static void dwc3_remove(struct platform_device *pdev) > > pm_runtime_set_suspended(&pdev->dev); > > > > dwc3_free_event_buffers(dwc); > > - > > - if (dwc->usb_psy) > > - power_supply_put(dwc->usb_psy); > > } > > > > #ifdef CONFIG_PM > > diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h > > index 1e561fd8b86e..3fc58204db6e 100644 > > --- a/drivers/usb/dwc3/core.h > > +++ b/drivers/usb/dwc3/core.h > > @@ -993,6 +993,7 @@ struct dwc3_scratchpad_array { > > /** > > * struct dwc3 - representation of our controller > > * @drd_work: workqueue used for role swapping > > + * @worker: dedicated kthread worker > > * @ep0_trb: trb which is used for the ctrl_req > > * @bounce: address of bounce buffer > > * @setup_buf: used while precessing STD USB requests > > @@ -1045,7 +1046,7 @@ struct dwc3_scratchpad_array { > > * @role_sw: usb_role_switch handle > > * @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 interface > > * @usb2_phy: pointer to USB2 PHY > > * @usb3_phy: pointer to USB3 PHY > > * @usb2_generic_phy: pointer to array of USB2 PHYs > > @@ -1163,6 +1164,7 @@ struct dwc3_scratchpad_array { > > */ > > struct dwc3 { > > struct work_struct drd_work; > > + struct kthread_worker *worker; > > struct dwc3_trb *ep0_trb; > > void *bounce; > > u8 *setup_buf; > > @@ -1223,7 +1225,7 @@ struct dwc3 { > > struct usb_role_switch *role_sw; > > enum usb_dr_mode role_switch_default_mode; > > > > - struct power_supply *usb_psy; > > + const char *usb_psy_name; > > > > u32 fladj; > > u32 ref_clk_per; > > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > > index 89fc690fdf34..1ff583281eff 100644 > > --- a/drivers/usb/dwc3/gadget.c > > +++ b/drivers/usb/dwc3/gadget.c > > @@ -30,6 +30,11 @@ > > #define DWC3_ALIGN_FRAME(d, n) (((d)->frame_number + ((d)->interval * (n))) \ > > & ~((d)->interval - 1)) > > > > +struct dwc3_psy_put { > > + struct kthread_work work; > > + struct power_supply *psy; > > +}; > > + > > /** > > * dwc3_gadget_set_test_mode - enables usb2 test modes > > * @dwc: pointer to our context structure > > @@ -3047,22 +3052,49 @@ static void dwc3_gadget_set_ssp_rate(struct usb_gadget *g, > > spin_unlock_irqrestore(&dwc->lock, flags); > > } > > > > +static void dwc3_gadget_psy_put(struct kthread_work *work) > > +{ > > + struct dwc3_psy_put *psy_put = container_of(work, struct dwc3_psy_put, work); > > + > > + power_supply_put(psy_put->psy); > > + kfree(psy_put); > > +} > > + > > static int dwc3_gadget_vbus_draw(struct usb_gadget *g, unsigned int mA) > > { > > - struct dwc3 *dwc = gadget_to_dwc(g); > > + struct dwc3 *dwc = gadget_to_dwc(g); > > + struct power_supply *usb_psy; > > union power_supply_propval val = {0}; > > + struct dwc3_psy_put *psy_put; > > int ret; > > > > if (dwc->usb2_phy) > > return usb_phy_set_power(dwc->usb2_phy, mA); > > > > - if (!dwc->usb_psy) > > + if (!dwc->usb_psy_name) > > return -EOPNOTSUPP; > > > > + usb_psy = power_supply_get_by_name(dwc->usb_psy_name); > > + if (!usb_psy) { > > + dev_err(dwc->dev, "couldn't get usb power supply\n"); > > + return -ENODEV; > > + } > > + > > val.intval = 1000 * mA; > > - ret = power_supply_set_property(dwc->usb_psy, POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT, &val); > > + ret = power_supply_set_property(usb_psy, POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT, &val); > > + if (ret < 0) { > > + dev_err(dwc->dev, "failed to set power supply property\n"); > > + return ret; > > + } > > > > - return ret; > > + psy_put = kzalloc(sizeof(*psy_put), GFP_ATOMIC); > > + if (!psy_put) > > + return -ENOMEM; > > + kthread_init_work(&psy_put->work, dwc3_gadget_psy_put); > > + psy_put->psy = usb_psy; > > + kthread_queue_work(dwc->worker, &psy_put->work); > > + > > + return 0; > > } > > > > /** > > -- > > 2.46.0.rc2.264.g509ed76dc8-goog > >
On Wed, Aug 07, 2024, Kyle Tso wrote: > On Wed, Aug 7, 2024 at 7:29 AM Thinh Nguyen <Thinh.Nguyen@synopsys.com> 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? > 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. 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
On Thu, Aug 8, 2024 at 9:38 AM Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote: > > On Wed, Aug 07, 2024, Kyle Tso wrote: > > On Wed, Aug 7, 2024 at 7:29 AM Thinh Nguyen <Thinh.Nguyen@synopsys.com> 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
On Thu, Aug 08, 2024, Kyle Tso wrote: > On Thu, Aug 8, 2024 at 9:38 AM Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote: > > > > On Wed, Aug 07, 2024, Kyle Tso wrote: > > > On Wed, Aug 7, 2024 at 7:29 AM Thinh Nguyen <Thinh.Nguyen@synopsys.com> 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. You still have not clarified what the risk is. How does "waiting for a long time" impact the device functionality? I'm not sure if there's a case where there's power_supply specified in the devicetree but the dwc3 should not care whether it's available. I need clarification if there's actually an actual device like this. The current logic in dwc3 is that if power_supply is not available, then return an error and it will not be available for the rest of the dwc3. If it is available, keep it around until dwc3 is removed. Should we enforce hard dependency on this? I need to know the above. > > > > 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)); > ... > If you force remove the module while it's being in used, then yes that would be a problem. But that's a decision by the user, and the user will know about it. If things break, it's on the user. What you're doing now is basically remove this protection and tell the user that this flow is normal/ok when it may not. > > > 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 If you rely on having power_supply, then enforce the dependency properly. If you don't need it to function, then why do we need all this change? > 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 What if the device is disconnected/re-enumerated or go into suspend/resume? Now we don't have the power_supply to properly draw vbus for the application. > 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. > If we lose access to power_supply, we should not silently let it be. BR, Thinh
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c index 734de2a8bd21..82c8376330d7 100644 --- a/drivers/usb/dwc3/core.c +++ b/drivers/usb/dwc3/core.c @@ -1631,8 +1631,6 @@ static void dwc3_get_properties(struct dwc3 *dwc) u8 tx_thr_num_pkt_prd = 0; u8 tx_max_burst_prd = 0; u8 tx_fifo_resize_max_num; - const char *usb_psy_name; - int ret; /* default to highest possible threshold */ lpm_nyet_threshold = 0xf; @@ -1667,12 +1665,7 @@ static void dwc3_get_properties(struct dwc3 *dwc) dwc->sys_wakeup = device_may_wakeup(dwc->sysdev); - ret = device_property_read_string(dev, "usb-psy-name", &usb_psy_name); - if (ret >= 0) { - dwc->usb_psy = power_supply_get_by_name(usb_psy_name); - if (!dwc->usb_psy) - dev_err(dev, "couldn't get usb power supply\n"); - } + device_property_read_string(dev, "usb-psy-name", &dwc->usb_psy_name); dwc->has_lpm_erratum = device_property_read_bool(dev, "snps,has-lpm-erratum"); @@ -2132,19 +2125,24 @@ static int dwc3_probe(struct platform_device *pdev) dwc3_get_software_properties(dwc); + dwc->worker = kthread_create_worker(0, "dwc3-worker"); + if (IS_ERR(dwc->worker)) + return PTR_ERR(dwc->worker); + sched_set_fifo(dwc->worker->task); + dwc->reset = devm_reset_control_array_get_optional_shared(dev); if (IS_ERR(dwc->reset)) { ret = PTR_ERR(dwc->reset); - goto err_put_psy; + goto err_destroy_worker; } ret = dwc3_get_clocks(dwc); if (ret) - goto err_put_psy; + goto err_destroy_worker; ret = reset_control_deassert(dwc->reset); if (ret) - goto err_put_psy; + goto err_destroy_worker; ret = dwc3_clk_enable(dwc); if (ret) @@ -2245,9 +2243,8 @@ static int dwc3_probe(struct platform_device *pdev) dwc3_clk_disable(dwc); err_assert_reset: reset_control_assert(dwc->reset); -err_put_psy: - if (dwc->usb_psy) - power_supply_put(dwc->usb_psy); +err_destroy_worker: + kthread_destroy_worker(dwc->worker); return ret; } @@ -2258,6 +2255,7 @@ static void dwc3_remove(struct platform_device *pdev) pm_runtime_get_sync(&pdev->dev); + kthread_destroy_worker(dwc->worker); dwc3_core_exit_mode(dwc); dwc3_debugfs_exit(dwc); @@ -2276,9 +2274,6 @@ static void dwc3_remove(struct platform_device *pdev) pm_runtime_set_suspended(&pdev->dev); dwc3_free_event_buffers(dwc); - - if (dwc->usb_psy) - power_supply_put(dwc->usb_psy); } #ifdef CONFIG_PM diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h index 1e561fd8b86e..3fc58204db6e 100644 --- a/drivers/usb/dwc3/core.h +++ b/drivers/usb/dwc3/core.h @@ -993,6 +993,7 @@ struct dwc3_scratchpad_array { /** * struct dwc3 - representation of our controller * @drd_work: workqueue used for role swapping + * @worker: dedicated kthread worker * @ep0_trb: trb which is used for the ctrl_req * @bounce: address of bounce buffer * @setup_buf: used while precessing STD USB requests @@ -1045,7 +1046,7 @@ struct dwc3_scratchpad_array { * @role_sw: usb_role_switch handle * @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 interface * @usb2_phy: pointer to USB2 PHY * @usb3_phy: pointer to USB3 PHY * @usb2_generic_phy: pointer to array of USB2 PHYs @@ -1163,6 +1164,7 @@ struct dwc3_scratchpad_array { */ struct dwc3 { struct work_struct drd_work; + struct kthread_worker *worker; struct dwc3_trb *ep0_trb; void *bounce; u8 *setup_buf; @@ -1223,7 +1225,7 @@ struct dwc3 { struct usb_role_switch *role_sw; enum usb_dr_mode role_switch_default_mode; - struct power_supply *usb_psy; + const char *usb_psy_name; u32 fladj; u32 ref_clk_per; diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index 89fc690fdf34..1ff583281eff 100644 --- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -30,6 +30,11 @@ #define DWC3_ALIGN_FRAME(d, n) (((d)->frame_number + ((d)->interval * (n))) \ & ~((d)->interval - 1)) +struct dwc3_psy_put { + struct kthread_work work; + struct power_supply *psy; +}; + /** * dwc3_gadget_set_test_mode - enables usb2 test modes * @dwc: pointer to our context structure @@ -3047,22 +3052,49 @@ static void dwc3_gadget_set_ssp_rate(struct usb_gadget *g, spin_unlock_irqrestore(&dwc->lock, flags); } +static void dwc3_gadget_psy_put(struct kthread_work *work) +{ + struct dwc3_psy_put *psy_put = container_of(work, struct dwc3_psy_put, work); + + power_supply_put(psy_put->psy); + kfree(psy_put); +} + static int dwc3_gadget_vbus_draw(struct usb_gadget *g, unsigned int mA) { - struct dwc3 *dwc = gadget_to_dwc(g); + struct dwc3 *dwc = gadget_to_dwc(g); + struct power_supply *usb_psy; union power_supply_propval val = {0}; + struct dwc3_psy_put *psy_put; int ret; if (dwc->usb2_phy) return usb_phy_set_power(dwc->usb2_phy, mA); - if (!dwc->usb_psy) + if (!dwc->usb_psy_name) return -EOPNOTSUPP; + usb_psy = power_supply_get_by_name(dwc->usb_psy_name); + if (!usb_psy) { + dev_err(dwc->dev, "couldn't get usb power supply\n"); + return -ENODEV; + } + val.intval = 1000 * mA; - ret = power_supply_set_property(dwc->usb_psy, POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT, &val); + ret = power_supply_set_property(usb_psy, POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT, &val); + if (ret < 0) { + dev_err(dwc->dev, "failed to set power supply property\n"); + return ret; + } - return ret; + psy_put = kzalloc(sizeof(*psy_put), GFP_ATOMIC); + if (!psy_put) + return -ENOMEM; + kthread_init_work(&psy_put->work, dwc3_gadget_psy_put); + psy_put->psy = usb_psy; + kthread_queue_work(dwc->worker, &psy_put->work); + + return 0; } /**
It is possible that the usb power_supply is registered after the probe 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 in dwc3 would become invalid. To fix this, get the handle right before calling to power_supply functions and put it afterward. dwc3_gadet_vbus_draw might be in interrupt context. Create a kthread worker beforehand and use it to process the "might-sleep" power_supply_put ASAP after the property set. Fixes: 6f0764b5adea ("usb: dwc3: add a power supply for current control") Cc: stable@vger.kernel.org Signed-off-by: Kyle Tso <kyletso@google.com> --- v2 -> v3: - Only move power_supply_put to a work. Still call _get_by_name and _set_property in dwc3_gadget_vbus_draw. - Create a kthread_worker to handle the work v1 -> v2: - move power_supply_put out of interrupt context drivers/usb/dwc3/core.c | 29 ++++++++++++---------------- drivers/usb/dwc3/core.h | 6 ++++-- drivers/usb/dwc3/gadget.c | 40 +++++++++++++++++++++++++++++++++++---- 3 files changed, 52 insertions(+), 23 deletions(-)