Skip to contentSkip to conversation

USB: lpc32xx: fix error handling

PATCH In review Johan Hovold 3 participants 13 messages Dec 18, 2025 – Dec 19, 2025

Subsystems (4)
Stable branch USB Arm/LPC32XX SoC support USB ohci driver
lore
Share & tools
lore

Johan Hovold

author
#0 lore raw

v2 cover USB: lpc32xx: fix error handling

A recent change fixing a device reference leak introduced a clock imbalance by reusing an error path so that the clock may be disabled before having been enabled.

The very same change could also lead to a use-after-free in case the driver is used with non-OF probing.

This series fixes the resulting mess as well as the related device leak in the NXP OHCI driver.

Included are also two related cleanups.

Johan

Changes in v2:
- defer an ohci error label rename to the clean up patch

Johan Hovold (5):
usb: gadget: lpc32xx_udc: fix clock imbalance in error path
usb: phy: isp1301: fix non-OF device reference imbalance
usb: ohci-nxp: fix device leak on probe failure
usb: gadget: lpc32xx_udc: clean up probe error labels
usb: ohci-nxp: clean up probe error labels

 drivers/usb/gadget/udc/lpc32xx_udc.c | 41 ++++++++++++++--------------
 drivers/usb/host/ohci-nxp.c          | 18 ++++++------
 drivers/usb/phy/phy-isp1301.c        |  7 ++++-
 3 files changed, 36 insertions(+), 30 deletions(-)
Signature · 1 line
2.51.2
author
#1 lore raw

PATCH v2 1/5 usb: gadget: lpc32xx_udc: fix clock imbalance in error path

A recent change fixing a device reference leak introduced a clock imbalance by reusing an error path so that the clock may be disabled before having been enabled.

Note that the clock framework allows for passing in NULL clocks so there is no risk for a NULL pointer dereference.

Also drop the bogus I2C client NULL check added by the offending commit as the pointer has already been verified to be non-NULL.

Fixes c84117912bdd ("USB: lpc32xx_udc: Fix error handling in probe")
Cc [email protected]
Cc Ma Ke <[email protected]>
Signed-off-by Johan Hovold <[email protected]>
---
 drivers/usb/gadget/udc/lpc32xx_udc.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)
drivers/usb/gadget/udc/lpc32xx_udc.c +10 -10
index 73c0f28a8585..a962d4294fbe 100644
@@ -3020,7 +3020,7 @@ static int lpc32xx_udc_probe(struct platform_device *pdev)
 	pdev->dev.dma_mask = &lpc32xx_usbd_dmamask;
 	retval = dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32));
 	if (retval)
-		goto i2c_fail;
+		goto err_put_client;
 
 	udc->board = &lpc32xx_usbddata;
 

@@ -3040,7 +3040,7 @@ static int lpc32xx_udc_probe(struct platform_device *pdev)
 		udc->udp_irq[i] = platform_get_irq(pdev, i);
 		if (udc->udp_irq[i] < 0) {
 			retval = udc->udp_irq[i];
-			goto i2c_fail;
+			goto err_put_client;
 		}
 	}
 

@@ -3048,7 +3048,7 @@ static int lpc32xx_udc_probe(struct platform_device *pdev)
 	if (IS_ERR(udc->udp_baseaddr)) {
 		dev_err(udc->dev, "IO map failure\n");
 		retval = PTR_ERR(udc->udp_baseaddr);
-		goto i2c_fail;
+		goto err_put_client;
 	}
 
 	/* Get USB device clock */

@@ -3056,14 +3056,14 @@ static int lpc32xx_udc_probe(struct platform_device *pdev)
 	if (IS_ERR(udc->usb_slv_clk)) {
 		dev_err(udc->dev, "failed to acquire USB device clock\n");
 		retval = PTR_ERR(udc->usb_slv_clk);
-		goto i2c_fail;
+		goto err_put_client;
 	}
 
 	/* Enable USB device clock */
 	retval = clk_prepare_enable(udc->usb_slv_clk);
 	if (retval < 0) {
 		dev_err(udc->dev, "failed to start USB device clock\n");
-		goto i2c_fail;
+		goto err_put_client;
 	}
 
 	/* Setup deferred workqueue data */

@@ -3165,9 +3165,10 @@ static int lpc32xx_udc_probe(struct platform_device *pdev)
 	dma_free_coherent(&pdev->dev, UDCA_BUFF_SIZE,
 			  udc->udca_v_base, udc->udca_p_base);
 i2c_fail:
-	if (udc->isp1301_i2c_client)
-		put_device(&udc->isp1301_i2c_client->dev);
 	clk_disable_unprepare(udc->usb_slv_clk);
+err_put_client:
+	put_device(&udc->isp1301_i2c_client->dev);
+
 	dev_err(udc->dev, "%s probe failed, %d\n", driver_name, retval);
 
 	return retval;

@@ -3195,10 +3196,9 @@ static void lpc32xx_udc_remove(struct platform_device *pdev)
 	dma_free_coherent(&pdev->dev, UDCA_BUFF_SIZE,
 			  udc->udca_v_base, udc->udca_p_base);
 
-	if (udc->isp1301_i2c_client)
-		put_device(&udc->isp1301_i2c_client->dev);
-
 	clk_disable_unprepare(udc->usb_slv_clk);
+
+	put_device(&udc->isp1301_i2c_client->dev);
 }
 
 #ifdef CONFIG_PM
Signature · 1 line
2.51.2
maintainer Reviewed-by on v2 1/5
#2 lore raw

Re: [PATCH v2 1/5] usb: gadget: lpc32xx_udc: fix clock imbalance in error path

Hi Johan,

thank you for looking at the issue.

Quoted 11 lines · Johan Hovold

A recent change fixing a device reference leak introduced a clock imbalance by reusing an error path so that the clock may be disabled before having been enabled.

Note that the clock framework allows for passing in NULL clocks so there is no risk for a NULL pointer dereference.

Also drop the bogus I2C client NULL check added by the offending commit as the pointer has already been verified to be non-NULL.

Fixes c84117912bdd ("USB: lpc32xx_udc: Fix error handling in probe")

This commit didn't get a proper review, and it is broken in so many ways, that I'd say it has to be reverted.

Quoted 71 lines
Cc [email protected]
Cc Ma Ke <[email protected]>
Signed-off-by Johan Hovold <[email protected]>
---
  drivers/usb/gadget/udc/lpc32xx_udc.c | 20 ++++++++++----------
  1 file changed, 10 insertions(+), 10 deletions(-)
drivers/usb/gadget/udc/lpc32xx_udc.c +8 -7
index 73c0f28a8585..a962d4294fbe 100644
@@ -3020,7 +3020,7 @@ static int lpc32xx_udc_probe(struct platform_device *pdev)
  	pdev->dev.dma_mask = &lpc32xx_usbd_dmamask;
  	retval = dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32));
  	if (retval)
-		goto i2c_fail;
+		goto err_put_client;
 
  	udc->board = &lpc32xx_usbddata;
 

@@ -3040,7 +3040,7 @@ static int lpc32xx_udc_probe(struct platform_device *pdev)
  		udc->udp_irq[i] = platform_get_irq(pdev, i);
  		if (udc->udp_irq[i] < 0) {
  			retval = udc->udp_irq[i];
-			goto i2c_fail;
+			goto err_put_client;
  		}
  	}
 

@@ -3048,7 +3048,7 @@ static int lpc32xx_udc_probe(struct platform_device *pdev)
  	if (IS_ERR(udc->udp_baseaddr)) {
  		dev_err(udc->dev, "IO map failure\n");
  		retval = PTR_ERR(udc->udp_baseaddr);
-		goto i2c_fail;
+		goto err_put_client;
  	}
 
  	/* Get USB device clock */

@@ -3056,14 +3056,14 @@ static int lpc32xx_udc_probe(struct platform_device *pdev)
  	if (IS_ERR(udc->usb_slv_clk)) {
  		dev_err(udc->dev, "failed to acquire USB device clock\n");
  		retval = PTR_ERR(udc->usb_slv_clk);
-		goto i2c_fail;
+		goto err_put_client;
  	}
 
  	/* Enable USB device clock */
  	retval = clk_prepare_enable(udc->usb_slv_clk);
  	if (retval < 0) {
  		dev_err(udc->dev, "failed to start USB device clock\n");
-		goto i2c_fail;
+		goto err_put_client;
  	}
 
  	/* Setup deferred workqueue data */

@@ -3165,9 +3165,10 @@ static int lpc32xx_udc_probe(struct platform_device *pdev)
  	dma_free_coherent(&pdev->dev, UDCA_BUFF_SIZE,
  			  udc->udca_v_base, udc->udca_p_base);
  i2c_fail:
-	if (udc->isp1301_i2c_client)
-		put_device(&udc->isp1301_i2c_client->dev);
  	clk_disable_unprepare(udc->usb_slv_clk);
+err_put_client:
+	put_device(&udc->isp1301_i2c_client->dev);
+
  	dev_err(udc->dev, "%s probe failed, %d\n", driver_name, retval);
 
  	return retval;

@@ -3195,10 +3196,9 @@ static void lpc32xx_udc_remove(struct platform_device *pdev)
  	dma_free_coherent(&pdev->dev, UDCA_BUFF_SIZE,
  			  udc->udca_v_base, udc->udca_p_base);

drivers/usb/gadget/udc/lpc32xx_udc.c static void lpc32xx_udc_remove(struct platform_device *pdev) · line 3204

Quoted 10 lines
drivers/usb/gadget/udc/lpc32xx_udc.c +2 -3
 
-	if (udc->isp1301_i2c_client)
-		put_device(&udc->isp1301_i2c_client->dev);
-
  	clk_disable_unprepare(udc->usb_slv_clk);
+
+	put_device(&udc->isp1301_i2c_client->dev);
  }
 
  #ifdef CONFIG_PM
Reviewed-by Vladimir Zapolskiy <[email protected]>
Signature · 2 lines
Best wishes,
Vladimir
author
#3 lore raw

PATCH v2 2/5 usb: phy: isp1301: fix non-OF device reference imbalance

In reply to Johan Hovold

A recent change fixing a device reference leak in a UDC driver introduced a potential use-after-free in the non-OF case as the isp1301_get_client() helper only increases the reference count for the returned I2C device in the OF case.

Increment the reference count also for non-OF so that the caller can decrement it unconditionally.

Note that this is inherently racy just as using the returned I2C device is since nothing is preventing the PHY driver from being unbound while in use.

Fixes c84117912bdd ("USB: lpc32xx_udc: Fix error handling in probe")
Cc [email protected]
Cc Ma Ke <[email protected]>
Signed-off-by Johan Hovold <[email protected]>
---
 drivers/usb/phy/phy-isp1301.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)
drivers/usb/phy/phy-isp1301.c +6 -1
index f9b5c411aee4..2940f0c84e1b 100644
@@ -149,7 +149,12 @@ struct i2c_client *isp1301_get_client(struct device_node *node)
 		return client;
 
 	/* non-DT: only one ISP1301 chip supported */
-	return isp1301_i2c_client;
+	if (isp1301_i2c_client) {
+		get_device(&isp1301_i2c_client->dev);
+		return isp1301_i2c_client;
+	}
+
+	return NULL;
 }
 EXPORT_SYMBOL_GPL(isp1301_get_client);
Signature · 1 line
2.51.2
maintainer Reviewed-by on v2 2/5
#4 lore raw

Re: [PATCH v2 2/5] usb: phy: isp1301: fix non-OF device reference imbalance

Quoted 4 lines · Johan Hovold

A recent change fixing a device reference leak in a UDC driver introduced a potential use-after-free in the non-OF case as the isp1301_get_client() helper only increases the reference count for the returned I2C device in the OF case.

Fortunatly there is no non-OF users of this driver, it's been discussed recently.

Quoted 24 lines

Increment the reference count also for non-OF so that the caller can decrement it unconditionally.

Note that this is inherently racy just as using the returned I2C device is since nothing is preventing the PHY driver from being unbound while in use.

Fixes c84117912bdd ("USB: lpc32xx_udc: Fix error handling in probe")
Cc [email protected]
Cc Ma Ke <[email protected]>
Signed-off-by Johan Hovold <[email protected]>
---
  drivers/usb/phy/phy-isp1301.c | 7 ++++++-
  1 file changed, 6 insertions(+), 1 deletion(-)
drivers/usb/phy/phy-isp1301.c +0 -0
index f9b5c411aee4..2940f0c84e1b 100644
@@ -149,7 +149,12 @@ struct i2c_client *isp1301_get_client(struct device_node *node)
  		return client;
 
  	/* non-DT: only one ISP1301 chip supported */

drivers/usb/phy/phy-isp1301.c struct i2c_client *isp1301_get_client(struct device_node *node) · line 160

Quoted 10 lines
drivers/usb/phy/phy-isp1301.c +6 -1
-	return isp1301_i2c_client;
+	if (isp1301_i2c_client) {
+		get_device(&isp1301_i2c_client->dev);
+		return isp1301_i2c_client;
+	}
+
+	return NULL;
  }
  EXPORT_SYMBOL_GPL(isp1301_get_client);

Okay, let's go the way of fixing the broken commit instead of its reversal.

Reviewed-by Vladimir Zapolskiy <[email protected]>
Signature · 2 lines
Best wishes,
Vladimir
author
#5 lore raw

Re: [PATCH v2 2/5] usb: phy: isp1301: fix non-OF device reference imbalance

Quoted 8 lines · Vladimir Zapolskiy
Quoted 4 lines · Johan Hovold

A recent change fixing a device reference leak in a UDC driver introduced a potential use-after-free in the non-OF case as the isp1301_get_client() helper only increases the reference count for the returned I2C device in the OF case.

Fortunatly there is no non-OF users of this driver, it's been discussed recently.

Yeah, I saw the discussion, but figured it was best to just fix up the existing code before you guys get on with ripping out the legacy support.

Quoted 29 lines
Quoted 16 lines

Increment the reference count also for non-OF so that the caller can decrement it unconditionally.

Note that this is inherently racy just as using the returned I2C device is since nothing is preventing the PHY driver from being unbound while in use.

Fixes c84117912bdd ("USB: lpc32xx_udc: Fix error handling in probe")
Cc [email protected]
Cc Ma Ke <[email protected]>
Signed-off-by Johan Hovold <[email protected]>
drivers/usb/phy/phy-isp1301.c +0 -0
@@ -149,7 +149,12 @@ struct i2c_client *isp1301_get_client(struct device_node *node)
  		return client;
 
  	/* non-DT: only one ISP1301 chip supported */

drivers/usb/phy/phy-isp1301.c struct i2c_client *isp1301_get_client(struct device_node *node) · line 160

Quoted 10 lines
drivers/usb/phy/phy-isp1301.c +6 -1
-	return isp1301_i2c_client;
+	if (isp1301_i2c_client) {
+		get_device(&isp1301_i2c_client->dev);
+		return isp1301_i2c_client;
+	}
+
+	return NULL;
  }
  EXPORT_SYMBOL_GPL(isp1301_get_client);

Okay, let's go the way of fixing the broken commit instead of its reversal.

Quoted 1 line
Reviewed-by Vladimir Zapolskiy <[email protected]>

Thanks for reviewing.

Johan

author
#6 lore raw

PATCH v2 3/5 usb: ohci-nxp: fix device leak on probe failure

In reply to Johan Hovold

Make sure to drop the reference taken when looking up the PHY I2C device during probe on probe failure (e.g. probe deferral) and on driver unbind.

Fixes 73108aa90cbf ("USB: ohci-nxp: Use isp1301 driver")
Cc [email protected] # 3.5
Reported-by Ma Ke <[email protected]>
Link https://lore.kernel.org/lkml/[email protected]/ open here
Signed-off-by Johan Hovold <[email protected]>
---
 drivers/usb/host/ohci-nxp.c | 2 ++
 1 file changed, 2 insertions(+)
drivers/usb/host/ohci-nxp.c +2 -0
index 24d5a1dc5056..509ca7d8d513 100644
@@ -223,6 +223,7 @@ static int ohci_hcd_nxp_probe(struct platform_device *pdev)
 fail_resource:
 	usb_put_hcd(hcd);
 fail_disable:
+	put_device(&isp1301_i2c_client->dev);
 	isp1301_i2c_client = NULL;
 	return ret;
 }

@@ -234,6 +235,7 @@ static void ohci_hcd_nxp_remove(struct platform_device *pdev)
 	usb_remove_hcd(hcd);
 	ohci_nxp_stop_hc();
 	usb_put_hcd(hcd);
+	put_device(&isp1301_i2c_client->dev);
 	isp1301_i2c_client = NULL;
 }
Signature · 1 line
2.51.2
maintainer Acked-by on v2 3/5
#7 lore raw

Re: [PATCH v2 3/5] usb: ohci-nxp: fix device leak on probe failure

Quoted 10 lines · Johan Hovold

Make sure to drop the reference taken when looking up the PHY I2C device during probe on probe failure (e.g. probe deferral) and on driver unbind.

Fixes 73108aa90cbf ("USB: ohci-nxp: Use isp1301 driver")
Cc [email protected] # 3.5
Reported-by Ma Ke <[email protected]>
Link https://lore.kernel.org/lkml/[email protected]/ open here
Signed-off-by Johan Hovold <[email protected]>

---

For this patch and the 5/5 patch:

Acked-by Alan Stern <[email protected]>

Alan Stern

Quoted 26 lines
 drivers/usb/host/ohci-nxp.c | 2 ++
 1 file changed, 2 insertions(+)
drivers/usb/host/ohci-nxp.c +2 -0
index 24d5a1dc5056..509ca7d8d513 100644
@@ -223,6 +223,7 @@ static int ohci_hcd_nxp_probe(struct platform_device *pdev)
 fail_resource:
 	usb_put_hcd(hcd);
 fail_disable:
+	put_device(&isp1301_i2c_client->dev);
 	isp1301_i2c_client = NULL;
 	return ret;
 }

@@ -234,6 +235,7 @@ static void ohci_hcd_nxp_remove(struct platform_device *pdev)
 	usb_remove_hcd(hcd);
 	ohci_nxp_stop_hc();
 	usb_put_hcd(hcd);
+	put_device(&isp1301_i2c_client->dev);
 	isp1301_i2c_client = NULL;
 }
Signature · 1 line
2.51.2
maintainer Reviewed-by on v2 3/5
#8 lore raw

Re: [PATCH v2 3/5] usb: ohci-nxp: fix device leak on probe failure

In reply to Johan Hovold
Quoted 26 lines · Johan Hovold

Make sure to drop the reference taken when looking up the PHY I2C device during probe on probe failure (e.g. probe deferral) and on driver unbind.

Fixes 73108aa90cbf ("USB: ohci-nxp: Use isp1301 driver")
Cc [email protected] # 3.5
Reported-by Ma Ke <[email protected]>
Link https://lore.kernel.org/lkml/[email protected]/ open here
Signed-off-by Johan Hovold <[email protected]>
---
  drivers/usb/host/ohci-nxp.c | 2 ++
  1 file changed, 2 insertions(+)
drivers/usb/host/ohci-nxp.c +1 -0
index 24d5a1dc5056..509ca7d8d513 100644
@@ -223,6 +223,7 @@ static int ohci_hcd_nxp_probe(struct platform_device *pdev)
  fail_resource:
  	usb_put_hcd(hcd);
  fail_disable:
+	put_device(&isp1301_i2c_client->dev);
  	isp1301_i2c_client = NULL;
  	return ret;
  }

drivers/usb/host/ohci-nxp.c static void ohci_hcd_nxp_remove(struct platform_device *pdev) · line 241

Quoted 7 lines
drivers/usb/host/ohci-nxp.c +1 -0
@@ -234,6 +235,7 @@ static void ohci_hcd_nxp_remove(struct platform_device *pdev)
  	usb_remove_hcd(hcd);
  	ohci_nxp_stop_hc();
  	usb_put_hcd(hcd);
+	put_device(&isp1301_i2c_client->dev);
  	isp1301_i2c_client = NULL;
  }
Reviewed-by Vladimir Zapolskiy <[email protected]>
Signature · 2 lines
Best wishes,
Vladimir
author
#9 lore raw

PATCH v2 4/5 usb: gadget: lpc32xx_udc: clean up probe error labels

In reply to Johan Hovold

Error labels should be named after what they do rather than after from where they are jumped to.

Rename the probe error labels for consistency and to improve readability.

Signed-off-by Johan Hovold <[email protected]>
---
 drivers/usb/gadget/udc/lpc32xx_udc.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)
drivers/usb/gadget/udc/lpc32xx_udc.c +10 -11
index a962d4294fbe..83c7e243dcf9 100644
@@ -3084,7 +3084,7 @@ static int lpc32xx_udc_probe(struct platform_device *pdev)
 	if (!udc->udca_v_base) {
 		dev_err(udc->dev, "error getting UDCA region\n");
 		retval = -ENOMEM;
-		goto i2c_fail;
+		goto err_disable_clk;
 	}
 	udc->udca_p_base = dma_handle;
 	dev_dbg(udc->dev, "DMA buffer(0x%x bytes), P:0x%08x, V:0x%p\n",

@@ -3097,7 +3097,7 @@ static int lpc32xx_udc_probe(struct platform_device *pdev)
 	if (!udc->dd_cache) {
 		dev_err(udc->dev, "error getting DD DMA region\n");
 		retval = -ENOMEM;
-		goto dma_alloc_fail;
+		goto err_free_dma;
 	}
 
 	/* Clear USB peripheral and initialize gadget endpoints */

@@ -3111,14 +3111,14 @@ static int lpc32xx_udc_probe(struct platform_device *pdev)
 	if (retval < 0) {
 		dev_err(udc->dev, "LP request irq %d failed\n",
 			udc->udp_irq[IRQ_USB_LP]);
-		goto irq_req_fail;
+		goto err_destroy_pool;
 	}
 	retval = devm_request_irq(dev, udc->udp_irq[IRQ_USB_HP],
 				  lpc32xx_usb_hp_irq, 0, "udc_hp", udc);
 	if (retval < 0) {
 		dev_err(udc->dev, "HP request irq %d failed\n",
 			udc->udp_irq[IRQ_USB_HP]);
-		goto irq_req_fail;
+		goto err_destroy_pool;
 	}
 
 	retval = devm_request_irq(dev, udc->udp_irq[IRQ_USB_DEVDMA],

@@ -3126,7 +3126,7 @@ static int lpc32xx_udc_probe(struct platform_device *pdev)
 	if (retval < 0) {
 		dev_err(udc->dev, "DEV request irq %d failed\n",
 			udc->udp_irq[IRQ_USB_DEVDMA]);
-		goto irq_req_fail;
+		goto err_destroy_pool;
 	}
 
 	/* The transceiver interrupt is used for VBUS detection and will

@@ -3137,7 +3137,7 @@ static int lpc32xx_udc_probe(struct platform_device *pdev)
 	if (retval < 0) {
 		dev_err(udc->dev, "VBUS request irq %d failed\n",
 			udc->udp_irq[IRQ_USB_ATX]);
-		goto irq_req_fail;
+		goto err_destroy_pool;
 	}
 
 	/* Initialize wait queue */

@@ -3146,7 +3146,7 @@ static int lpc32xx_udc_probe(struct platform_device *pdev)
 
 	retval = usb_add_gadget_udc(dev, &udc->gadget);
 	if (retval < 0)
-		goto add_gadget_fail;
+		goto err_destroy_pool;
 
 	dev_set_drvdata(dev, udc);
 	device_init_wakeup(dev, 1);

@@ -3158,13 +3158,12 @@ static int lpc32xx_udc_probe(struct platform_device *pdev)
 	dev_info(udc->dev, "%s version %s\n", driver_name, DRIVER_VERSION);
 	return 0;
 
-add_gadget_fail:
-irq_req_fail:
+err_destroy_pool:
 	dma_pool_destroy(udc->dd_cache);
-dma_alloc_fail:
+err_free_dma:
 	dma_free_coherent(&pdev->dev, UDCA_BUFF_SIZE,
 			  udc->udca_v_base, udc->udca_p_base);
-i2c_fail:
+err_disable_clk:
 	clk_disable_unprepare(udc->usb_slv_clk);
 err_put_client:
 	put_device(&udc->isp1301_i2c_client->dev);
Signature · 1 line
2.51.2
maintainer Reviewed-by on v2 4/5
#10 lore raw

Re: [PATCH v2 4/5] usb: gadget: lpc32xx_udc: clean up probe error labels

Quoted 84 lines · Johan Hovold

Error labels should be named after what they do rather than after from where they are jumped to.

Rename the probe error labels for consistency and to improve readability.

Signed-off-by Johan Hovold <[email protected]>
---
  drivers/usb/gadget/udc/lpc32xx_udc.c | 21 ++++++++++-----------
  1 file changed, 10 insertions(+), 11 deletions(-)
drivers/usb/gadget/udc/lpc32xx_udc.c +8 -9
index a962d4294fbe..83c7e243dcf9 100644
@@ -3084,7 +3084,7 @@ static int lpc32xx_udc_probe(struct platform_device *pdev)
  	if (!udc->udca_v_base) {
  		dev_err(udc->dev, "error getting UDCA region\n");
  		retval = -ENOMEM;
-		goto i2c_fail;
+		goto err_disable_clk;
  	}
  	udc->udca_p_base = dma_handle;
  	dev_dbg(udc->dev, "DMA buffer(0x%x bytes), P:0x%08x, V:0x%p\n",

@@ -3097,7 +3097,7 @@ static int lpc32xx_udc_probe(struct platform_device *pdev)
  	if (!udc->dd_cache) {
  		dev_err(udc->dev, "error getting DD DMA region\n");
  		retval = -ENOMEM;
-		goto dma_alloc_fail;
+		goto err_free_dma;
  	}
 
  	/* Clear USB peripheral and initialize gadget endpoints */

@@ -3111,14 +3111,14 @@ static int lpc32xx_udc_probe(struct platform_device *pdev)
  	if (retval < 0) {
  		dev_err(udc->dev, "LP request irq %d failed\n",
  			udc->udp_irq[IRQ_USB_LP]);
-		goto irq_req_fail;
+		goto err_destroy_pool;
  	}
  	retval = devm_request_irq(dev, udc->udp_irq[IRQ_USB_HP],
  				  lpc32xx_usb_hp_irq, 0, "udc_hp", udc);
  	if (retval < 0) {
  		dev_err(udc->dev, "HP request irq %d failed\n",
  			udc->udp_irq[IRQ_USB_HP]);
-		goto irq_req_fail;
+		goto err_destroy_pool;
  	}
 
  	retval = devm_request_irq(dev, udc->udp_irq[IRQ_USB_DEVDMA],

@@ -3126,7 +3126,7 @@ static int lpc32xx_udc_probe(struct platform_device *pdev)
  	if (retval < 0) {
  		dev_err(udc->dev, "DEV request irq %d failed\n",
  			udc->udp_irq[IRQ_USB_DEVDMA]);
-		goto irq_req_fail;
+		goto err_destroy_pool;
  	}
 
  	/* The transceiver interrupt is used for VBUS detection and will

@@ -3137,7 +3137,7 @@ static int lpc32xx_udc_probe(struct platform_device *pdev)
  	if (retval < 0) {
  		dev_err(udc->dev, "VBUS request irq %d failed\n",
  			udc->udp_irq[IRQ_USB_ATX]);
-		goto irq_req_fail;
+		goto err_destroy_pool;
  	}
 
  	/* Initialize wait queue */

@@ -3146,7 +3146,7 @@ static int lpc32xx_udc_probe(struct platform_device *pdev)
 
  	retval = usb_add_gadget_udc(dev, &udc->gadget);
  	if (retval < 0)
-		goto add_gadget_fail;
+		goto err_destroy_pool;
 
  	dev_set_drvdata(dev, udc);
  	device_init_wakeup(dev, 1);

@@ -3158,13 +3158,12 @@ static int lpc32xx_udc_probe(struct platform_device *pdev)
  	dev_info(udc->dev, "%s version %s\n", driver_name, DRIVER_VERSION);
  	return 0;
 
-add_gadget_fail:
-irq_req_fail:
+err_destroy_pool:

drivers/usb/gadget/udc/lpc32xx_udc.c static int lpc32xx_udc_probe(struct platform_device *pdev) · line 3169

Quoted 10 lines
drivers/usb/gadget/udc/lpc32xx_udc.c +2 -2
  	dma_pool_destroy(udc->dd_cache);
-dma_alloc_fail:
+err_free_dma:
  	dma_free_coherent(&pdev->dev, UDCA_BUFF_SIZE,
  			  udc->udca_v_base, udc->udca_p_base);
-i2c_fail:
+err_disable_clk:
  	clk_disable_unprepare(udc->usb_slv_clk);
  err_put_client:
  	put_device(&udc->isp1301_i2c_client->dev);
Reviewed-by Vladimir Zapolskiy <[email protected]>
Signature · 2 lines
Best wishes,
Vladimir
author
#11 lore raw

PATCH v2 5/5 usb: ohci-nxp: clean up probe error labels

In reply to Johan Hovold

Error labels should be named after what they do rather than after from where they are jumped to.

Rename the probe error labels for consistency and to improve readability.

Signed-off-by Johan Hovold <[email protected]>
---
 drivers/usb/host/ohci-nxp.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)
drivers/usb/host/ohci-nxp.c +8 -8
index 509ca7d8d513..7663f2aa35e9 100644
@@ -169,13 +169,13 @@ static int ohci_hcd_nxp_probe(struct platform_device *pdev)
 
 	ret = dma_coerce_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
 	if (ret)
-		goto fail_disable;
+		goto err_put_client;
 
 	dev_dbg(&pdev->dev, "%s: " DRIVER_DESC " (nxp)\n", hcd_name);
 	if (usb_disabled()) {
 		dev_err(&pdev->dev, "USB is disabled\n");
 		ret = -ENODEV;
-		goto fail_disable;
+		goto err_put_client;
 	}
 
 	/* Enable USB host clock */

@@ -183,7 +183,7 @@ static int ohci_hcd_nxp_probe(struct platform_device *pdev)
 	if (IS_ERR(usb_host_clk)) {
 		dev_err(&pdev->dev, "failed to acquire and start USB OHCI clock\n");
 		ret = PTR_ERR(usb_host_clk);
-		goto fail_disable;
+		goto err_put_client;
 	}
 
 	isp1301_configure();

@@ -192,13 +192,13 @@ static int ohci_hcd_nxp_probe(struct platform_device *pdev)
 	if (!hcd) {
 		dev_err(&pdev->dev, "Failed to allocate HC buffer\n");
 		ret = -ENOMEM;
-		goto fail_disable;
+		goto err_put_client;
 	}
 
 	hcd->regs = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
 	if (IS_ERR(hcd->regs)) {
 		ret = PTR_ERR(hcd->regs);
-		goto fail_resource;
+		goto err_put_hcd;
 	}
 	hcd->rsrc_start = res->start;
 	hcd->rsrc_len = resource_size(res);

@@ -206,7 +206,7 @@ static int ohci_hcd_nxp_probe(struct platform_device *pdev)
 	irq = platform_get_irq(pdev, 0);
 	if (irq < 0) {
 		ret = -ENXIO;
-		goto fail_resource;
+		goto err_put_hcd;
 	}
 
 	ohci_nxp_start_hc();

@@ -220,9 +220,9 @@ static int ohci_hcd_nxp_probe(struct platform_device *pdev)
 	}
 
 	ohci_nxp_stop_hc();
-fail_resource:
+err_put_hcd:
 	usb_put_hcd(hcd);
-fail_disable:
+err_put_client:
 	put_device(&isp1301_i2c_client->dev);
 	isp1301_i2c_client = NULL;
 	return ret;
Signature · 1 line
2.51.2
maintainer Reviewed-by on v2 5/5
#12 lore raw

Re: [PATCH v2 5/5] usb: ohci-nxp: clean up probe error labels

Quoted 67 lines · Johan Hovold

Error labels should be named after what they do rather than after from where they are jumped to.

Rename the probe error labels for consistency and to improve readability.

Signed-off-by Johan Hovold <[email protected]>
---
  drivers/usb/host/ohci-nxp.c | 16 ++++++++--------
  1 file changed, 8 insertions(+), 8 deletions(-)
drivers/usb/host/ohci-nxp.c +6 -6
index 509ca7d8d513..7663f2aa35e9 100644
@@ -169,13 +169,13 @@ static int ohci_hcd_nxp_probe(struct platform_device *pdev)
 
  	ret = dma_coerce_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
  	if (ret)
-		goto fail_disable;
+		goto err_put_client;
 
  	dev_dbg(&pdev->dev, "%s: " DRIVER_DESC " (nxp)\n", hcd_name);
  	if (usb_disabled()) {
  		dev_err(&pdev->dev, "USB is disabled\n");
  		ret = -ENODEV;
-		goto fail_disable;
+		goto err_put_client;
  	}
 
  	/* Enable USB host clock */

@@ -183,7 +183,7 @@ static int ohci_hcd_nxp_probe(struct platform_device *pdev)
  	if (IS_ERR(usb_host_clk)) {
  		dev_err(&pdev->dev, "failed to acquire and start USB OHCI clock\n");
  		ret = PTR_ERR(usb_host_clk);
-		goto fail_disable;
+		goto err_put_client;
  	}
 
  	isp1301_configure();

@@ -192,13 +192,13 @@ static int ohci_hcd_nxp_probe(struct platform_device *pdev)
  	if (!hcd) {
  		dev_err(&pdev->dev, "Failed to allocate HC buffer\n");
  		ret = -ENOMEM;
-		goto fail_disable;
+		goto err_put_client;
  	}
 
  	hcd->regs = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
  	if (IS_ERR(hcd->regs)) {
  		ret = PTR_ERR(hcd->regs);
-		goto fail_resource;
+		goto err_put_hcd;
  	}
  	hcd->rsrc_start = res->start;
  	hcd->rsrc_len = resource_size(res);

@@ -206,7 +206,7 @@ static int ohci_hcd_nxp_probe(struct platform_device *pdev)
  	irq = platform_get_irq(pdev, 0);
  	if (irq < 0) {
  		ret = -ENXIO;
-		goto fail_resource;
+		goto err_put_hcd;
  	}
 
  	ohci_nxp_start_hc();

@@ -220,9 +220,9 @@ static int ohci_hcd_nxp_probe(struct platform_device *pdev)
  	}

drivers/usb/host/ohci-nxp.c static int ohci_hcd_nxp_probe(struct platform_device *pdev) · line 228

Quoted 10 lines
drivers/usb/host/ohci-nxp.c +2 -2
 
  	ohci_nxp_stop_hc();
-fail_resource:
+err_put_hcd:
  	usb_put_hcd(hcd);
-fail_disable:
+err_put_client:
  	put_device(&isp1301_i2c_client->dev);
  	isp1301_i2c_client = NULL;
  	return ret;
Reviewed-by Vladimir Zapolskiy <[email protected]>

Thank you for the fixes!

Signature · 2 lines
Best wishes,
Vladimir