This patch series consists of two fixes and changes to goto labels
to use meaningful names.
While working on goto label changes, I noticed put_sync calls without
successful get_sync in error legs. The first two patches are the fixes
and the rest of the patches are goto label changes.
Shuah Khan (7):
usb: dwc3: core: fix dwc3_probe() to not do put_sync when get_sync
fails
usb: dwc3: dwc3-omap: fix dwc3_omap_probe() do put_sync when get_sync
works
usb: dwc3: core: change goto labels to meaningful names
usb: dwc3: exynos: change goto labels in this file to meaningful names
usb: dwc3: omap: change goto labels in this file to meaningful names
usb: dwc3: gadget: change goto labels in this file to meaningful names
usb: dwc3: host: change goto labels in this file to meaningful names
drivers/usb/dwc3/core.c | 64 +++++++++++++++++++++---------------------
drivers/usb/dwc3/dwc3-exynos.c | 16 +++++------
drivers/usb/dwc3/dwc3-omap.c | 11 ++++----
drivers/usb/dwc3/gadget.c | 56 ++++++++++++++++++------------------
drivers/usb/dwc3/host.c | 10 +++----
5 files changed, 79 insertions(+), 78 deletions(-)
--
2.7.4
dwc3_probe() does pm_runtime_put_sync() in its err1 handling when
pm_runtime_get_sync() fails. Move the pm_runtime_put_sync() under
err2 instead as it is used in error paths after pm_runtime_get_sync()
succeeds.
Signed-off-by: Shuah Khan <[email protected]>
---
drivers/usb/dwc3/core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 56f1367..0fc7bef 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -1162,9 +1162,9 @@ static int dwc3_probe(struct platform_device *pdev)
err2:
pm_runtime_allow(&pdev->dev);
+ pm_runtime_put_sync(&pdev->dev);
err1:
- pm_runtime_put_sync(&pdev->dev);
pm_runtime_disable(&pdev->dev);
err0:
--
2.7.4
Change goto labels in this file to meaningful names.
Signed-off-by: Shuah Khan <[email protected]>
---
drivers/usb/dwc3/dwc3-omap.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/dwc3/dwc3-omap.c b/drivers/usb/dwc3/dwc3-omap.c
index 55b12a9..ca0075a 100644
--- a/drivers/usb/dwc3/dwc3-omap.c
+++ b/drivers/usb/dwc3/dwc3-omap.c
@@ -503,7 +503,7 @@ static int dwc3_omap_probe(struct platform_device *pdev)
ret = pm_runtime_get_sync(dev);
if (ret < 0) {
dev_err(dev, "get_sync failed with err %d\n", ret);
- goto err1;
+ goto runtime_disable;
}
dwc3_omap_map_offset(omap);
@@ -537,7 +537,7 @@ static int dwc3_omap_probe(struct platform_device *pdev)
put_sync:
pm_runtime_put_sync(dev);
-err1:
+runtime_disable:
pm_runtime_disable(dev);
return ret;
--
2.7.4
dwc3_omap_probe() does pm_runtime_put_sync() in its err1 handling when
pm_runtime_get_sync() fails. Fix it to do put_sync only when get_sync
succeeds.
Signed-off-by: Shuah Khan <[email protected]>
---
drivers/usb/dwc3/dwc3-omap.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/dwc3/dwc3-omap.c b/drivers/usb/dwc3/dwc3-omap.c
index 2092e46..55b12a9 100644
--- a/drivers/usb/dwc3/dwc3-omap.c
+++ b/drivers/usb/dwc3/dwc3-omap.c
@@ -518,25 +518,26 @@ static int dwc3_omap_probe(struct platform_device *pdev)
if (ret) {
dev_err(dev, "failed to request IRQ #%d --> %d\n",
omap->irq, ret);
- goto err1;
+ goto put_sync;
}
ret = dwc3_omap_extcon_register(omap);
if (ret < 0)
- goto err1;
+ goto put_sync;
ret = of_platform_populate(node, NULL, NULL, dev);
if (ret) {
dev_err(&pdev->dev, "failed to create dwc3 core\n");
- goto err1;
+ goto put_sync;
}
dwc3_omap_enable_irqs(omap);
enable_irq(omap->irq);
return 0;
-err1:
+put_sync:
pm_runtime_put_sync(dev);
+err1:
pm_runtime_disable(dev);
return ret;
--
2.7.4
Change goto labels in this file to meaningful names.
Signed-off-by: Shuah Khan <[email protected]>
---
drivers/usb/dwc3/gadget.c | 56 +++++++++++++++++++++++------------------------
1 file changed, 28 insertions(+), 28 deletions(-)
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 8d44a2f..9795226 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -1345,19 +1345,19 @@ static int dwc3_gadget_ep_dequeue(struct usb_ep *ep,
if (r == req) {
/* wait until it is processed */
dwc3_stop_active_transfer(dwc, dep->number, true);
- goto out1;
+ goto gadget_giveback;
}
dev_err(dwc->dev, "request %p was not queued to %s\n",
request, ep->name);
ret = -EINVAL;
- goto out0;
+ goto return_ret;
}
-out1:
+gadget_giveback:
/* giveback the request */
dwc3_gadget_giveback(dep, req, -ECONNRESET);
-out0:
+return_ret:
spin_unlock_irqrestore(&dwc->lock, flags);
return ret;
@@ -1811,14 +1811,14 @@ static int __dwc3_gadget_start(struct dwc3 *dwc)
ret = __dwc3_gadget_ep_enable(dep, false, false);
if (ret) {
dev_err(dwc->dev, "failed to enable %s\n", dep->name);
- goto err0;
+ goto return_ret;
}
dep = dwc->eps[1];
ret = __dwc3_gadget_ep_enable(dep, false, false);
if (ret) {
dev_err(dwc->dev, "failed to enable %s\n", dep->name);
- goto err1;
+ goto ep_disable;
}
/* begin to receive SETUP packets */
@@ -1829,10 +1829,10 @@ static int __dwc3_gadget_start(struct dwc3 *dwc)
return 0;
-err1:
+ep_disable:
__dwc3_gadget_ep_disable(dwc->eps[0]);
-err0:
+return_ret:
return ret;
}
@@ -1850,7 +1850,7 @@ static int dwc3_gadget_start(struct usb_gadget *g,
if (ret) {
dev_err(dwc->dev, "failed to request irq #%d --> %d\n",
irq, ret);
- goto err0;
+ goto return_ret;
}
spin_lock_irqsave(&dwc->lock, flags);
@@ -1859,7 +1859,7 @@ static int dwc3_gadget_start(struct usb_gadget *g,
dwc->gadget.name,
dwc->gadget_driver->driver.name);
ret = -EBUSY;
- goto err1;
+ goto irq_free;
}
dwc->gadget_driver = driver;
@@ -1871,11 +1871,11 @@ static int dwc3_gadget_start(struct usb_gadget *g,
return 0;
-err1:
+irq_free:
spin_unlock_irqrestore(&dwc->lock, flags);
free_irq(irq, dwc);
-err0:
+return_ret:
return ret;
}
@@ -3063,7 +3063,7 @@ int dwc3_gadget_init(struct dwc3 *dwc)
irq = dwc3_gadget_get_irq(dwc);
if (irq < 0) {
ret = irq;
- goto err0;
+ goto return_ret;
}
dwc->irq_gadget = irq;
@@ -3073,7 +3073,7 @@ int dwc3_gadget_init(struct dwc3 *dwc)
if (!dwc->ctrl_req) {
dev_err(dwc->dev, "failed to allocate ctrl request\n");
ret = -ENOMEM;
- goto err0;
+ goto return_ret;
}
dwc->ep0_trb = dma_alloc_coherent(dwc->sysdev,
@@ -3082,13 +3082,13 @@ int dwc3_gadget_init(struct dwc3 *dwc)
if (!dwc->ep0_trb) {
dev_err(dwc->dev, "failed to allocate ep0 trb\n");
ret = -ENOMEM;
- goto err1;
+ goto free_ctrl_req;
}
dwc->setup_buf = kzalloc(DWC3_EP0_BOUNCE_SIZE, GFP_KERNEL);
if (!dwc->setup_buf) {
ret = -ENOMEM;
- goto err2;
+ goto free_ep0_trb;
}
dwc->ep0_bounce = dma_alloc_coherent(dwc->sysdev,
@@ -3097,20 +3097,20 @@ int dwc3_gadget_init(struct dwc3 *dwc)
if (!dwc->ep0_bounce) {
dev_err(dwc->dev, "failed to allocate ep0 bounce buffer\n");
ret = -ENOMEM;
- goto err3;
+ goto free_setup_buf;
}
dwc->zlp_buf = kzalloc(DWC3_ZLP_BUF_SIZE, GFP_KERNEL);
if (!dwc->zlp_buf) {
ret = -ENOMEM;
- goto err4;
+ goto free_ep0_bounce;
}
dwc->bounce = dma_alloc_coherent(dwc->sysdev, DWC3_BOUNCE_SIZE,
&dwc->bounce_addr, GFP_KERNEL);
if (!dwc->bounce) {
ret = -ENOMEM;
- goto err5;
+ goto free_zlp_buf;
}
init_completion(&dwc->ep0_in_setup);
@@ -3150,39 +3150,39 @@ int dwc3_gadget_init(struct dwc3 *dwc)
ret = dwc3_gadget_init_endpoints(dwc, dwc->num_eps);
if (ret)
- goto err6;
+ goto free_bounce;
ret = usb_add_gadget_udc(dwc->dev, &dwc->gadget);
if (ret) {
dev_err(dwc->dev, "failed to register udc\n");
- goto err6;
+ goto free_bounce;
}
return 0;
-err6:
+free_bounce:
dma_free_coherent(dwc->sysdev, DWC3_BOUNCE_SIZE, dwc->bounce,
dwc->bounce_addr);
-err5:
+free_zlp_buf:
kfree(dwc->zlp_buf);
-err4:
+free_ep0_bounce:
dwc3_gadget_free_endpoints(dwc);
dma_free_coherent(dwc->sysdev, DWC3_EP0_BOUNCE_SIZE,
dwc->ep0_bounce, dwc->ep0_bounce_addr);
-err3:
+free_setup_buf:
kfree(dwc->setup_buf);
-err2:
+free_ep0_trb:
dma_free_coherent(dwc->sysdev, sizeof(*dwc->ep0_trb) * 2,
dwc->ep0_trb, dwc->ep0_trb_addr);
-err1:
+free_ctrl_req:
dma_free_coherent(dwc->sysdev, sizeof(*dwc->ctrl_req),
dwc->ctrl_req, dwc->ctrl_req_addr);
-err0:
+return_ret:
return ret;
}
--
2.7.4
Change goto labels in this file to meaningful names
Signed-off-by: Shuah Khan <[email protected]>
---
drivers/usb/dwc3/dwc3-exynos.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/usb/dwc3/dwc3-exynos.c b/drivers/usb/dwc3/dwc3-exynos.c
index 98f74ff..11f31f4 100644
--- a/drivers/usb/dwc3/dwc3-exynos.c
+++ b/drivers/usb/dwc3/dwc3-exynos.c
@@ -58,12 +58,12 @@ static int dwc3_exynos_register_phys(struct dwc3_exynos *exynos)
ret = platform_device_add_data(exynos->usb2_phy, &pdata, sizeof(pdata));
if (ret)
- goto err1;
+ goto usb2_phy_device_put;
pdev = platform_device_alloc("usb_phy_generic", PLATFORM_DEVID_AUTO);
if (!pdev) {
ret = -ENOMEM;
- goto err1;
+ goto usb2_phy_device_put;
}
exynos->usb3_phy = pdev;
@@ -71,25 +71,25 @@ static int dwc3_exynos_register_phys(struct dwc3_exynos *exynos)
ret = platform_device_add_data(exynos->usb3_phy, &pdata, sizeof(pdata));
if (ret)
- goto err2;
+ goto usb3_phy_device_put;
ret = platform_device_add(exynos->usb2_phy);
if (ret)
- goto err2;
+ goto usb3_phy_device_put;
ret = platform_device_add(exynos->usb3_phy);
if (ret)
- goto err3;
+ goto usb2_phy_device_del;
return 0;
-err3:
+usb2_phy_device_del:
platform_device_del(exynos->usb2_phy);
-err2:
+usb3_phy_device_put:
platform_device_put(exynos->usb3_phy);
-err1:
+usb2_phy_device_put:
platform_device_put(exynos->usb2_phy);
return ret;
--
2.7.4
Change goto labels in this file to meaningful names.
Signed-off-by: Shuah Khan <[email protected]>
---
drivers/usb/dwc3/host.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c
index 76f0b0d..eb264a9 100644
--- a/drivers/usb/dwc3/host.c
+++ b/drivers/usb/dwc3/host.c
@@ -93,7 +93,7 @@ int dwc3_host_init(struct dwc3 *dwc)
DWC3_XHCI_RESOURCES_NUM);
if (ret) {
dev_err(dwc->dev, "couldn't add resources to xHCI device\n");
- goto err1;
+ goto put_device;
}
memset(props, 0, sizeof(struct property_entry) * ARRAY_SIZE(props));
@@ -117,7 +117,7 @@ int dwc3_host_init(struct dwc3 *dwc)
ret = platform_device_add_properties(xhci, props);
if (ret) {
dev_err(dwc->dev, "failed to add properties to xHCI\n");
- goto err1;
+ goto put_device;
}
}
@@ -129,16 +129,16 @@ int dwc3_host_init(struct dwc3 *dwc)
ret = platform_device_add(xhci);
if (ret) {
dev_err(dwc->dev, "failed to register xHCI device\n");
- goto err2;
+ goto remove_lookup;
}
return 0;
-err2:
+remove_lookup:
phy_remove_lookup(dwc->usb2_generic_phy, "usb2-phy",
dev_name(dwc->dev));
phy_remove_lookup(dwc->usb3_generic_phy, "usb3-phy",
dev_name(dwc->dev));
-err1:
+put_device:
platform_device_put(xhci);
return ret;
}
--
2.7.4
Change goto labels to meaningful names.
Signed-off-by: Shuah Khan <[email protected]>
---
drivers/usb/dwc3/core.c | 62 ++++++++++++++++++++++++-------------------------
1 file changed, 31 insertions(+), 31 deletions(-)
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 0fc7bef..f82786a 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -347,7 +347,7 @@ static int dwc3_setup_scratch_buffers(struct dwc3 *dwc)
if (dma_mapping_error(dwc->sysdev, scratch_addr)) {
dev_err(dwc->sysdev, "failed to map scratch buffer\n");
ret = -EFAULT;
- goto err0;
+ goto return_err;
}
dwc->scratch_addr = scratch_addr;
@@ -357,22 +357,22 @@ static int dwc3_setup_scratch_buffers(struct dwc3 *dwc)
ret = dwc3_send_gadget_generic_command(dwc,
DWC3_DGCMD_SET_SCRATCHPAD_ADDR_LO, param);
if (ret < 0)
- goto err1;
+ goto unmap_scratchbuf;
param = upper_32_bits(scratch_addr);
ret = dwc3_send_gadget_generic_command(dwc,
DWC3_DGCMD_SET_SCRATCHPAD_ADDR_HI, param);
if (ret < 0)
- goto err1;
+ goto unmap_scratchbuf;
return 0;
-err1:
+unmap_scratchbuf:
dma_unmap_single(dwc->sysdev, dwc->scratch_addr, dwc->nr_scratch *
DWC3_SCRATCHBUF_SIZE, DMA_BIDIRECTIONAL);
-err0:
+return_err:
return ret;
}
@@ -669,7 +669,7 @@ static int dwc3_core_init(struct dwc3 *dwc)
if (!dwc3_core_is_valid(dwc)) {
dev_err(dwc->dev, "this is not a DesignWare USB3 DRD Core\n");
ret = -ENODEV;
- goto err0;
+ goto return_err;
}
/*
@@ -687,18 +687,18 @@ static int dwc3_core_init(struct dwc3 *dwc)
ret = dwc3_core_soft_reset(dwc);
if (ret)
- goto err0;
+ goto return_err;
ret = dwc3_phy_setup(dwc);
if (ret)
- goto err0;
+ goto return_err;
dwc3_core_setup_global_control(dwc);
dwc3_core_num_eps(dwc);
ret = dwc3_setup_scratch_buffers(dwc);
if (ret)
- goto err1;
+ goto phy_shutdown_exit;
/* Adjust Frame Length */
dwc3_frame_length_adjustment(dwc);
@@ -707,16 +707,16 @@ static int dwc3_core_init(struct dwc3 *dwc)
usb_phy_set_suspend(dwc->usb3_phy, 0);
ret = phy_power_on(dwc->usb2_generic_phy);
if (ret < 0)
- goto err2;
+ goto phy_suspend;
ret = phy_power_on(dwc->usb3_generic_phy);
if (ret < 0)
- goto err3;
+ goto usb2_phy_poweroff;
ret = dwc3_event_buffers_setup(dwc);
if (ret) {
dev_err(dwc->dev, "failed to setup event buffers\n");
- goto err4;
+ goto usb3_phy_poweroff;
}
switch (dwc->dr_mode) {
@@ -757,23 +757,23 @@ static int dwc3_core_init(struct dwc3 *dwc)
return 0;
-err4:
+usb3_phy_poweroff:
phy_power_off(dwc->usb3_generic_phy);
-err3:
+usb2_phy_poweroff:
phy_power_off(dwc->usb2_generic_phy);
-err2:
+phy_suspend:
usb_phy_set_suspend(dwc->usb2_phy, 1);
usb_phy_set_suspend(dwc->usb3_phy, 1);
-err1:
+phy_shutdown_exit:
usb_phy_shutdown(dwc->usb2_phy);
usb_phy_shutdown(dwc->usb3_phy);
phy_exit(dwc->usb2_generic_phy);
phy_exit(dwc->usb3_generic_phy);
-err0:
+return_err:
return ret;
}
@@ -1091,7 +1091,7 @@ static int dwc3_probe(struct platform_device *pdev)
regs = devm_ioremap_resource(dev, res);
if (IS_ERR(regs)) {
ret = PTR_ERR(regs);
- goto err0;
+ goto restore_res_start;
}
dwc->regs = regs;
@@ -1104,7 +1104,7 @@ static int dwc3_probe(struct platform_device *pdev)
ret = dwc3_core_get_phy(dwc);
if (ret)
- goto err0;
+ goto restore_res_start;
spin_lock_init(&dwc->lock);
@@ -1114,7 +1114,7 @@ static int dwc3_probe(struct platform_device *pdev)
pm_runtime_enable(dev);
ret = pm_runtime_get_sync(dev);
if (ret < 0)
- goto err1;
+ goto runtime_disable;
pm_runtime_forbid(dev);
@@ -1122,52 +1122,52 @@ 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 runtime_allow_put_sync;
}
ret = dwc3_get_dr_mode(dwc);
if (ret)
- goto err3;
+ goto ulpi_exit;
ret = dwc3_alloc_scratch_buffers(dwc);
if (ret)
- goto err3;
+ goto ulpi_exit;
ret = dwc3_core_init(dwc);
if (ret) {
dev_err(dev, "failed to initialize core\n");
- goto err4;
+ goto free_scratchbufs;
}
dwc3_check_params(dwc);
ret = dwc3_core_init_mode(dwc);
if (ret)
- goto err5;
+ goto event_buffers_cleanup;
dwc3_debugfs_init(dwc);
pm_runtime_put(dev);
return 0;
-err5:
+event_buffers_cleanup:
dwc3_event_buffers_cleanup(dwc);
-err4:
+free_scratchbufs:
dwc3_free_scratch_buffers(dwc);
-err3:
+ulpi_exit:
dwc3_free_event_buffers(dwc);
dwc3_ulpi_exit(dwc);
-err2:
+runtime_allow_put_sync:
pm_runtime_allow(&pdev->dev);
pm_runtime_put_sync(&pdev->dev);
-err1:
+runtime_disable:
pm_runtime_disable(&pdev->dev);
-err0:
+restore_res_start:
/*
* restore res->start back to its original value so that, in case the
* probe is deferred, we don't end up getting error in request the
--
2.7.4
Hi,
Shuah Khan <[email protected]> writes:
> This patch series consists of two fixes and changes to goto labels
we *NEVER* put cleanups and fixes together.
> to use meaningful names.
>
> While working on goto label changes, I noticed put_sync calls without
> successful get_sync in error legs. The first two patches are the fixes
> and the rest of the patches are goto label changes.
so, you mean to say that you're adding regressions because you *THOUGHT*
there was a bug there, right? Read the docs ;-)
--
balbi
Hi,
Shuah Khan <[email protected]> writes:
> dwc3_probe() does pm_runtime_put_sync() in its err1 handling when
> pm_runtime_get_sync() fails. Move the pm_runtime_put_sync() under
> err2 instead as it is used in error paths after pm_runtime_get_sync()
> succeeds.
there's nothing wrong with current code. Read the docs. Even if
pm_runtime_get*() fails, you still need to decrement the usage
counter. pm_runtime_put*() is one way of achieving so.
--
balbi
Hi,
Shuah Khan <[email protected]> writes:
> dwc3_omap_probe() does pm_runtime_put_sync() in its err1 handling when
> pm_runtime_get_sync() fails. Fix it to do put_sync only when get_sync
> succeeds.
>
> Signed-off-by: Shuah Khan <[email protected]>
sorry, not taking any of these pointless patches. There's nothing wrong
with the goto labels, there's nothing wrong with the error path in any
of these drivers.
--
balbi
On 03/25/2017 01:51 AM, Felipe Balbi wrote:
>
> Hi,
>
> Shuah Khan <[email protected]> writes:
>> This patch series consists of two fixes and changes to goto labels
>
> we *NEVER* put cleanups and fixes together.
>
>> to use meaningful names.
>>
>> While working on goto label changes, I noticed put_sync calls without
>> successful get_sync in error legs. The first two patches are the fixes
>> and the rest of the patches are goto label changes.
>
> so, you mean to say that you're adding regressions because you *THOUGHT*
> there was a bug there, right? Read the docs ;-)
>
Sorry for the noise. Thanks for pointing this out.
-- Shuah