| Message ID | f8a4a6897deb0c8cb2e576580790303550f15fcd.1629314734.git.christophe.jaillet@wanadoo.fr (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 6f15a2a09cecb7a2faba4a75bbd101f6f962294b |
| Headers | show |
| Series | [1/2] usb: bdc: Fix an error handling path in 'bdc_probe()' when no suitable DMA config is available | expand |
On 8/18/2021 9:32 PM, Christophe JAILLET wrote: > If an error occurs after a successful 'clk_prepare_enable()' call, it must > be undone by a corresponding 'clk_disable_unprepare()' call. > This call is already present in the remove function. > > Add this call in the error handling path and reorder the code so that the > 'clk_prepare_enable()' call happens later in the function. > The goal is to have as much managed resources functions as possible > before the 'clk_prepare_enable()' call in order to keep the error handling > path simple. > > While at it, remove the now unneeded 'clk' variable. > > Fixes: c87dca047849 ("usb: bdc: Add clock enable for new chips with a separate BDC clock") > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> Acked-by: Florian Fainelli <f.fainelli@gmail.com> > --- > Review with care. > I don't like shuffling code like that because of possible side effect. > Moving the code related to this clk looks fine to me, but who knows... There are no register accesses until bdc_phy_init() gets called, so this looks fine to me. Al knows this code better than I do though, so it would be better to wait for his Acked-by tag.
On Fri, Aug 20, 2021 at 4:03 PM Florian Fainelli <f.fainelli@gmail.com> wrote: > > > > On 8/18/2021 9:32 PM, Christophe JAILLET wrote: > > If an error occurs after a successful 'clk_prepare_enable()' call, it must > > be undone by a corresponding 'clk_disable_unprepare()' call. > > This call is already present in the remove function. > > > > Add this call in the error handling path and reorder the code so that the > > 'clk_prepare_enable()' call happens later in the function. > > The goal is to have as much managed resources functions as possible > > before the 'clk_prepare_enable()' call in order to keep the error handling > > path simple. > > > > While at it, remove the now unneeded 'clk' variable. > > > > Fixes: c87dca047849 ("usb: bdc: Add clock enable for new chips with a separate BDC clock") > > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> > > Acked-by: Florian Fainelli <f.fainelli@gmail.com> > > > --- > > Review with care. > > I don't like shuffling code like that because of possible side effect. > > Moving the code related to this clk looks fine to me, but who knows... > > There are no register accesses until bdc_phy_init() gets called, so this > looks fine to me. Al knows this code better than I do though, so it > would be better to wait for his Acked-by tag. This is safe. Acked-by: Al Cooper <alcooperx@gmail.com> > -- > Florian
diff --git a/drivers/usb/gadget/udc/bdc/bdc_core.c b/drivers/usb/gadget/udc/bdc/bdc_core.c index 251db57e51fa..fa1a3908ec3b 100644 --- a/drivers/usb/gadget/udc/bdc/bdc_core.c +++ b/drivers/usb/gadget/udc/bdc/bdc_core.c @@ -488,27 +488,14 @@ static int bdc_probe(struct platform_device *pdev) int irq; u32 temp; struct device *dev = &pdev->dev; - struct clk *clk; int phy_num; dev_dbg(dev, "%s()\n", __func__); - clk = devm_clk_get_optional(dev, "sw_usbd"); - if (IS_ERR(clk)) - return PTR_ERR(clk); - - ret = clk_prepare_enable(clk); - if (ret) { - dev_err(dev, "could not enable clock\n"); - return ret; - } - bdc = devm_kzalloc(dev, sizeof(*bdc), GFP_KERNEL); if (!bdc) return -ENOMEM; - bdc->clk = clk; - bdc->regs = devm_platform_ioremap_resource(pdev, 0); if (IS_ERR(bdc->regs)) return PTR_ERR(bdc->regs); @@ -545,10 +532,20 @@ static int bdc_probe(struct platform_device *pdev) } } + bdc->clk = devm_clk_get_optional(dev, "sw_usbd"); + if (IS_ERR(bdc->clk)) + return PTR_ERR(bdc->clk); + + ret = clk_prepare_enable(bdc->clk); + if (ret) { + dev_err(dev, "could not enable clock\n"); + return ret; + } + ret = bdc_phy_init(bdc); if (ret) { dev_err(bdc->dev, "BDC phy init failure:%d\n", ret); - return ret; + goto disable_clk; } temp = bdc_readl(bdc->regs, BDC_BDCCAP1); @@ -581,6 +578,8 @@ static int bdc_probe(struct platform_device *pdev) bdc_hw_exit(bdc); phycleanup: bdc_phy_exit(bdc); +disable_clk: + clk_disable_unprepare(bdc->clk); return ret; }
If an error occurs after a successful 'clk_prepare_enable()' call, it must be undone by a corresponding 'clk_disable_unprepare()' call. This call is already present in the remove function. Add this call in the error handling path and reorder the code so that the 'clk_prepare_enable()' call happens later in the function. The goal is to have as much managed resources functions as possible before the 'clk_prepare_enable()' call in order to keep the error handling path simple. While at it, remove the now unneeded 'clk' variable. Fixes: c87dca047849 ("usb: bdc: Add clock enable for new chips with a separate BDC clock") Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> --- Review with care. I don't like shuffling code like that because of possible side effect. Moving the code related to this clk looks fine to me, but who knows... --- drivers/usb/gadget/udc/bdc/bdc_core.c | 27 +++++++++++++-------------- 1 file changed, 13 insertions(+), 14 deletions(-)