| Message ID | 20230404072524.19014-1-johan+linaro@kernel.org |
|---|---|
| Headers | show |
| Series | USB: dwc3: error handling fixes and cleanups | expand |
On Tue, Apr 04, 2023, Johan Hovold wrote: > Use descriptive names consistently for the probe error labels. > > Signed-off-by: Johan Hovold <johan+linaro@kernel.org> > --- > drivers/usb/dwc3/core.c | 45 ++++++++++++++++++++--------------------- > 1 file changed, 22 insertions(+), 23 deletions(-) > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > index d837ab511686..de84e057d28b 100644 > --- a/drivers/usb/dwc3/core.c > +++ b/drivers/usb/dwc3/core.c > @@ -1705,7 +1705,7 @@ static int dwc3_probe(struct platform_device *pdev) > dwc->reset = devm_reset_control_array_get_optional_shared(dev); > if (IS_ERR(dwc->reset)) { > ret = PTR_ERR(dwc->reset); > - goto put_usb_psy; > + goto err_put_psy; > } > > if (dev->of_node) { > @@ -1719,7 +1719,7 @@ static int dwc3_probe(struct platform_device *pdev) > if (IS_ERR(dwc->bus_clk)) { > ret = dev_err_probe(dev, PTR_ERR(dwc->bus_clk), > "could not get bus clock\n"); > - goto put_usb_psy; > + goto err_put_psy; > } > > if (dwc->bus_clk == NULL) { > @@ -1727,7 +1727,7 @@ static int dwc3_probe(struct platform_device *pdev) > if (IS_ERR(dwc->bus_clk)) { > ret = dev_err_probe(dev, PTR_ERR(dwc->bus_clk), > "could not get bus clock\n"); > - goto put_usb_psy; > + goto err_put_psy; > } > } > > @@ -1735,7 +1735,7 @@ static int dwc3_probe(struct platform_device *pdev) > if (IS_ERR(dwc->ref_clk)) { > ret = dev_err_probe(dev, PTR_ERR(dwc->ref_clk), > "could not get ref clock\n"); > - goto put_usb_psy; > + goto err_put_psy; > } > > if (dwc->ref_clk == NULL) { > @@ -1743,7 +1743,7 @@ static int dwc3_probe(struct platform_device *pdev) > if (IS_ERR(dwc->ref_clk)) { > ret = dev_err_probe(dev, PTR_ERR(dwc->ref_clk), > "could not get ref clock\n"); > - goto put_usb_psy; > + goto err_put_psy; > } > } > > @@ -1751,7 +1751,7 @@ static int dwc3_probe(struct platform_device *pdev) > if (IS_ERR(dwc->susp_clk)) { > ret = dev_err_probe(dev, PTR_ERR(dwc->susp_clk), > "could not get suspend clock\n"); > - goto put_usb_psy; > + goto err_put_psy; > } > > if (dwc->susp_clk == NULL) { > @@ -1759,23 +1759,23 @@ static int dwc3_probe(struct platform_device *pdev) > if (IS_ERR(dwc->susp_clk)) { > ret = dev_err_probe(dev, PTR_ERR(dwc->susp_clk), > "could not get suspend clock\n"); > - goto put_usb_psy; > + goto err_put_psy; > } > } > } > > ret = reset_control_deassert(dwc->reset); > if (ret) > - goto put_usb_psy; > + goto err_put_psy; > > ret = dwc3_clk_enable(dwc); > if (ret) > - goto assert_reset; > + goto err_assert_reset; > > if (!dwc3_core_is_valid(dwc)) { > dev_err(dwc->dev, "this is not a DesignWare USB3 DRD Core\n"); > ret = -ENODEV; > - goto disable_clks; > + goto err_disable_clks; > } > > platform_set_drvdata(pdev, dwc); > @@ -1785,7 +1785,7 @@ static int dwc3_probe(struct platform_device *pdev) > DWC3_GHWPARAMS0_AWIDTH(dwc->hwparams.hwparams0) == 64) { > ret = dma_set_mask_and_coherent(dwc->sysdev, DMA_BIT_MASK(64)); > if (ret) > - goto disable_clks; > + goto err_disable_clks; > } > > spin_lock_init(&dwc->lock); > @@ -1803,23 +1803,23 @@ static int dwc3_probe(struct platform_device *pdev) > if (ret) { > dev_err(dwc->dev, "failed to allocate event buffers\n"); > ret = -ENOMEM; > - goto err2; > + goto err_allow_rpm; > } > > dwc->edev = dwc3_get_extcon(dwc); > if (IS_ERR(dwc->edev)) { > ret = dev_err_probe(dwc->dev, PTR_ERR(dwc->edev), "failed to get extcon\n"); > - goto err3; > + goto err_free_event_buffers; > } > > ret = dwc3_get_dr_mode(dwc); > if (ret) > - goto err3; > + goto err_free_event_buffers; > > ret = dwc3_core_init(dwc); > if (ret) { > dev_err_probe(dev, ret, "failed to initialize core\n"); > - goto err3; > + goto err_free_event_buffers; > } > > dwc3_check_params(dwc); > @@ -1827,13 +1827,13 @@ static int dwc3_probe(struct platform_device *pdev) > > ret = dwc3_core_init_mode(dwc); > if (ret) > - goto err5; > + goto err_exit_debugfs; > > pm_runtime_put(dev); > > return 0; > > -err5: > +err_exit_debugfs: > dwc3_debugfs_exit(dwc); > dwc3_event_buffers_cleanup(dwc); > > @@ -1848,20 +1848,19 @@ static int dwc3_probe(struct platform_device *pdev) > phy_exit(dwc->usb3_generic_phy); > > dwc3_ulpi_exit(dwc); > -err3: > +err_free_event_buffers: > dwc3_free_event_buffers(dwc); > - > -err2: > +err_allow_rpm: > pm_runtime_allow(dev); > pm_runtime_disable(dev); > pm_runtime_dont_use_autosuspend(dev); > pm_runtime_set_suspended(dev); > pm_runtime_put_noidle(dev); > -disable_clks: > +err_disable_clks: > dwc3_clk_disable(dwc); > -assert_reset: > +err_assert_reset: > reset_control_assert(dwc->reset); > -put_usb_psy: > +err_put_psy: > if (dwc->usb_psy) > power_supply_put(dwc->usb_psy); > > -- > 2.39.2 > Acked-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com> Thanks, Thinh
On Tue, Apr 04, 2023, Johan Hovold wrote: > While there likely are no platforms out there that mix generic and > legacy PHYs the driver should still be able to handle that, if only for > consistency reasons. > > Add the missing calls to shutdown any legacy PHYs if generic PHY > initialisation fails. > > Note that we continue to happily ignore potential errors from the legacy > PHY callbacks... > > Signed-off-by: Johan Hovold <johan+linaro@kernel.org> > --- > drivers/usb/dwc3/core.c | 18 +++++++++--------- > 1 file changed, 9 insertions(+), 9 deletions(-) > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > index de84e057d28b..15405f1f7aef 100644 > --- a/drivers/usb/dwc3/core.c > +++ b/drivers/usb/dwc3/core.c > @@ -1031,15 +1031,14 @@ static int dwc3_core_init(struct dwc3 *dwc) > > usb_phy_init(dwc->usb2_phy); > usb_phy_init(dwc->usb3_phy); > + > ret = phy_init(dwc->usb2_generic_phy); > if (ret < 0) > - goto err0a; > + goto err_shutdown_usb3_phy; > > ret = phy_init(dwc->usb3_generic_phy); > - if (ret < 0) { > - phy_exit(dwc->usb2_generic_phy); > - goto err0a; > - } > + if (ret < 0) > + goto err_exit_usb2_phy; > > ret = dwc3_core_soft_reset(dwc); > if (ret) > @@ -1215,11 +1214,12 @@ static int dwc3_core_init(struct dwc3 *dwc) > usb_phy_set_suspend(dwc->usb3_phy, 1); > > err1: > - usb_phy_shutdown(dwc->usb2_phy); > - usb_phy_shutdown(dwc->usb3_phy); > - phy_exit(dwc->usb2_generic_phy); > phy_exit(dwc->usb3_generic_phy); > - > +err_exit_usb2_phy: > + phy_exit(dwc->usb2_generic_phy); > +err_shutdown_usb3_phy: > + usb_phy_shutdown(dwc->usb3_phy); > + usb_phy_shutdown(dwc->usb2_phy); > err0a: > dwc3_ulpi_exit(dwc); > > -- > 2.39.2 > Acked-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com> Thanks, Thinh
On Tue, Apr 04, 2023, Johan Hovold wrote: > When reviewing the dwc3 runtime PM implementation I noticed that the > probe error handling and unbind code was broken. The first two patches > addresses the corresponding imbalances. > > The probe error handling has suffered from some bit rot over years and > an attempt to clean it up lead to the realisation that the code dealing > with the "hibernation" feature was both broken and had never been used. > Rather than try to fix up something which has never been used since it > was first merged ten years ago, let's get rid of this dead code until > there is a mainline user (and a complete implementation). > > The rest of the series clean up probe and core initialisation by using > descriptive error labels and adding a few helper functions to improve > readability which will hopefully help prevent similar bugs from being > introduced in the future. > > Johan > > > Johan Hovold (11): > USB: dwc3: fix runtime pm imbalance on probe errors > USB: dwc3: fix runtime pm imbalance on unbind > USB: dwc3: disable autosuspend on unbind > USB: dwc3: gadget: drop dead hibernation code > USB: dwc3: drop dead hibernation code > USB: dwc3: clean up probe error labels > USB: dwc3: clean up phy init error handling > USB: dwc3: clean up core init error handling > USB: dwc3: refactor phy handling > USB: dwc3: refactor clock lookups > USB: dwc3: clean up probe declarations > > drivers/usb/dwc3/core.c | 426 ++++++++++++++++---------------------- > drivers/usb/dwc3/core.h | 8 - > drivers/usb/dwc3/gadget.c | 46 +--- > 3 files changed, 182 insertions(+), 298 deletions(-) > > -- > 2.39.2 > Thanks for the cleanup work. I've reviewed some patches, but still need to spend some more time reviewing the runtime changes. Thanks, Thinh
On Tue, Apr 04, 2023, Johan Hovold wrote: > Make sure to balance the runtime PM usage count on driver unbind by > adding back the pm_runtime_allow() call that had been erroneously > removed. > > Fixes: 266d0493900a ("usb: dwc3: core: don't trigger runtime pm when remove driver") > Cc: stable@vger.kernel.org # 5.9 > Cc: Li Jun <jun.li@nxp.com> > Signed-off-by: Johan Hovold <johan+linaro@kernel.org> > --- > drivers/usb/dwc3/core.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > index 5058bd8d56ca..9f8c988c25cb 100644 > --- a/drivers/usb/dwc3/core.c > +++ b/drivers/usb/dwc3/core.c > @@ -1979,6 +1979,7 @@ static int dwc3_remove(struct platform_device *pdev) > dwc3_core_exit(dwc); > dwc3_ulpi_exit(dwc); > > + pm_runtime_allow(&pdev->dev); > pm_runtime_disable(&pdev->dev); > pm_runtime_put_noidle(&pdev->dev); > pm_runtime_set_suspended(&pdev->dev); > -- > 2.39.2 > Acked-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com> Thanks, Thinh