diff mbox series

[1/2] usb: ohci-omap: Create private state container

Message ID 20200703175701.570446-1-linus.walleij@linaro.org
State New
Headers show
Series [1/2] usb: ohci-omap: Create private state container | expand

Commit Message

Linus Walleij July 3, 2020, 5:57 p.m. UTC 
The OMAP1 was using static locals to hold the clock handles
which is uncommon and does not scale. Create a private data
struct and use that to hold the clocks.

Cc: Janusz Krzysztofik <jmkrzyszt@gmail.com>
Cc: Tony Lindgren <tony@atomide.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

---
 drivers/usb/host/ohci-omap.c | 73 +++++++++++++++++++++---------------
 1 file changed, 42 insertions(+), 31 deletions(-)

-- 
2.25.4

Comments

Alan Stern July 3, 2020, 7:47 p.m. UTC | #1 
On Fri, Jul 03, 2020 at 07:57:00PM +0200, Linus Walleij wrote:
> The OMAP1 was using static locals to hold the clock handles

> which is uncommon and does not scale. Create a private data

> struct and use that to hold the clocks.

> 

> Cc: Janusz Krzysztofik <jmkrzyszt@gmail.com>

> Cc: Tony Lindgren <tony@atomide.com>

> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

> ---


Okay in principle, but the patch needs to be revised.

> @@ -196,6 +201,7 @@ static int ohci_omap_reset(struct usb_hcd *hcd)

>  {

>  	struct ohci_hcd		*ohci = hcd_to_ohci(hcd);

>  	struct omap_usb_config	*config = dev_get_platdata(hcd->self.controller);

> +	struct ohci_omap_priv *priv = hcd_to_ohci_omap_priv(hcd);


Use a tab to align with the other declarations, please.

> @@ -341,6 +333,21 @@ static int ohci_hcd_omap_probe(struct platform_device *pdev)

>  	}

>  	hcd->rsrc_start = pdev->resource[0].start;

>  	hcd->rsrc_len = pdev->resource[0].end - pdev->resource[0].start + 1;

> +	priv = hcd_to_ohci_omap_priv(hcd);

> +

> +	priv->usb_host_ck = clk_get(&pdev->dev, "usb_hhc_ck");

> +	if (IS_ERR(priv->usb_host_ck))

> +		return PTR_ERR(priv->usb_host_ck);

> +

> +	if (!cpu_is_omap15xx())

> +		priv->usb_dc_ck = clk_get(&pdev->dev, "usb_dc_ck");

> +	else

> +		priv->usb_dc_ck = clk_get(&pdev->dev, "lb_ck");

> +

> +	if (IS_ERR(priv->usb_dc_ck)) {

> +		clk_put(priv->usb_host_ck);

> +		return PTR_ERR(priv->usb_dc_ck);


If these two clk_get() calls fail, you mustn't just return.  You need to 
perform the usb_put_hcd() at the bottom of the function.

> +	}

>  

>  	if (!request_mem_region(hcd->rsrc_start, hcd->rsrc_len, hcd_name)) {

>  		dev_dbg(&pdev->dev, "request_mem_region failed\n");

> @@ -373,8 +380,8 @@ static int ohci_hcd_omap_probe(struct platform_device *pdev)

>  err1:

>  	usb_put_hcd(hcd);

>  err0:

> -	clk_put(usb_dc_ck);

> -	clk_put(usb_host_ck);

> +	clk_put(priv->usb_dc_ck);

> +	clk_put(priv->usb_host_ck);


When usb_put_hcd() is called, priv bacomes a stale pointer.  These two 
lines need to come earlier.  Also, you may as well create a label for the 
second one, so that you can jump there if the second clk_get() fails.

In fact, it wouldn't hurt to rename all these error labels to something
more meaningful than err0, err1, etc.  That could be done in a separate,
preliminary patch.

> @@ -406,8 +414,8 @@ static int ohci_hcd_omap_remove(struct platform_device *pdev)

>  	iounmap(hcd->regs);

>  	release_mem_region(hcd->rsrc_start, hcd->rsrc_len);

>  	usb_put_hcd(hcd);

> -	clk_put(usb_dc_ck);

> -	clk_put(usb_host_ck);

> +	clk_put(priv->usb_dc_ck);

> +	clk_put(priv->usb_host_ck);


Same comment about stale pointers here as before.

Alan Stern
diff mbox series

Patch

diff --git a/drivers/usb/host/ohci-omap.c b/drivers/usb/host/ohci-omap.c
index d8d35d456456..add2eac53a98 100644
--- a/drivers/usb/host/ohci-omap.c
+++ b/drivers/usb/host/ohci-omap.c
@@ -69,22 +69,27 @@  static inline int tps65010_set_gpio_out_value(unsigned gpio, unsigned value)
 
 #endif
 
-static struct clk *usb_host_ck;
-static struct clk *usb_dc_ck;
+struct ohci_omap_priv {
+	struct clk *usb_host_ck;
+	struct clk *usb_dc_ck;
+};
 
 static const char hcd_name[] = "ohci-omap";
 static struct hc_driver __read_mostly ohci_omap_hc_driver;
 
-static void omap_ohci_clock_power(int on)
+#define hcd_to_ohci_omap_priv(h) \
+	((struct ohci_omap_priv *)hcd_to_ohci(h)->priv)
+
+static void omap_ohci_clock_power(struct ohci_omap_priv *priv, int on)
 {
 	if (on) {
-		clk_enable(usb_dc_ck);
-		clk_enable(usb_host_ck);
+		clk_enable(priv->usb_dc_ck);
+		clk_enable(priv->usb_host_ck);
 		/* guesstimate for T5 == 1x 32K clock + APLL lock time */
 		udelay(100);
 	} else {
-		clk_disable(usb_host_ck);
-		clk_disable(usb_dc_ck);
+		clk_disable(priv->usb_host_ck);
+		clk_disable(priv->usb_dc_ck);
 	}
 }
 
@@ -196,6 +201,7 @@  static int ohci_omap_reset(struct usb_hcd *hcd)
 {
 	struct ohci_hcd		*ohci = hcd_to_ohci(hcd);
 	struct omap_usb_config	*config = dev_get_platdata(hcd->self.controller);
+	struct ohci_omap_priv *priv = hcd_to_ohci_omap_priv(hcd);
 	int			need_transceiver = (config->otg != 0);
 	int			ret;
 
@@ -235,7 +241,7 @@  static int ohci_omap_reset(struct usb_hcd *hcd)
 	}
 #endif
 
-	omap_ohci_clock_power(1);
+	omap_ohci_clock_power(priv, 1);
 
 	if (cpu_is_omap15xx()) {
 		omap_1510_local_bus_power(1);
@@ -305,6 +311,7 @@  static int ohci_hcd_omap_probe(struct platform_device *pdev)
 {
 	int retval, irq;
 	struct usb_hcd *hcd = 0;
+	struct ohci_omap_priv *priv;
 
 	if (pdev->num_resources != 2) {
 		dev_err(&pdev->dev, "invalid num_resources: %i\n",
@@ -318,21 +325,6 @@  static int ohci_hcd_omap_probe(struct platform_device *pdev)
 		return -ENODEV;
 	}
 
-	usb_host_ck = clk_get(&pdev->dev, "usb_hhc_ck");
-	if (IS_ERR(usb_host_ck))
-		return PTR_ERR(usb_host_ck);
-
-	if (!cpu_is_omap15xx())
-		usb_dc_ck = clk_get(&pdev->dev, "usb_dc_ck");
-	else
-		usb_dc_ck = clk_get(&pdev->dev, "lb_ck");
-
-	if (IS_ERR(usb_dc_ck)) {
-		clk_put(usb_host_ck);
-		return PTR_ERR(usb_dc_ck);
-	}
-
-
 	hcd = usb_create_hcd(&ohci_omap_hc_driver, &pdev->dev,
 			dev_name(&pdev->dev));
 	if (!hcd) {
@@ -341,6 +333,21 @@  static int ohci_hcd_omap_probe(struct platform_device *pdev)
 	}
 	hcd->rsrc_start = pdev->resource[0].start;
 	hcd->rsrc_len = pdev->resource[0].end - pdev->resource[0].start + 1;
+	priv = hcd_to_ohci_omap_priv(hcd);
+
+	priv->usb_host_ck = clk_get(&pdev->dev, "usb_hhc_ck");
+	if (IS_ERR(priv->usb_host_ck))
+		return PTR_ERR(priv->usb_host_ck);
+
+	if (!cpu_is_omap15xx())
+		priv->usb_dc_ck = clk_get(&pdev->dev, "usb_dc_ck");
+	else
+		priv->usb_dc_ck = clk_get(&pdev->dev, "lb_ck");
+
+	if (IS_ERR(priv->usb_dc_ck)) {
+		clk_put(priv->usb_host_ck);
+		return PTR_ERR(priv->usb_dc_ck);
+	}
 
 	if (!request_mem_region(hcd->rsrc_start, hcd->rsrc_len, hcd_name)) {
 		dev_dbg(&pdev->dev, "request_mem_region failed\n");
@@ -373,8 +380,8 @@  static int ohci_hcd_omap_probe(struct platform_device *pdev)
 err1:
 	usb_put_hcd(hcd);
 err0:
-	clk_put(usb_dc_ck);
-	clk_put(usb_host_ck);
+	clk_put(priv->usb_dc_ck);
+	clk_put(priv->usb_host_ck);
 	return retval;
 }
 
@@ -393,10 +400,11 @@  static int ohci_hcd_omap_probe(struct platform_device *pdev)
 static int ohci_hcd_omap_remove(struct platform_device *pdev)
 {
 	struct usb_hcd	*hcd = platform_get_drvdata(pdev);
+	struct ohci_omap_priv *priv = hcd_to_ohci_omap_priv(hcd);
 
 	dev_dbg(hcd->self.controller, "stopping USB Controller\n");
 	usb_remove_hcd(hcd);
-	omap_ohci_clock_power(0);
+	omap_ohci_clock_power(priv, 0);
 	if (!IS_ERR_OR_NULL(hcd->usb_phy)) {
 		(void) otg_set_host(hcd->usb_phy->otg, 0);
 		usb_put_phy(hcd->usb_phy);
@@ -406,8 +414,8 @@  static int ohci_hcd_omap_remove(struct platform_device *pdev)
 	iounmap(hcd->regs);
 	release_mem_region(hcd->rsrc_start, hcd->rsrc_len);
 	usb_put_hcd(hcd);
-	clk_put(usb_dc_ck);
-	clk_put(usb_host_ck);
+	clk_put(priv->usb_dc_ck);
+	clk_put(priv->usb_host_ck);
 	return 0;
 }
 
@@ -419,6 +427,7 @@  static int ohci_omap_suspend(struct platform_device *pdev, pm_message_t message)
 {
 	struct usb_hcd *hcd = platform_get_drvdata(pdev);
 	struct ohci_hcd *ohci = hcd_to_ohci(hcd);
+	struct ohci_omap_priv *priv = hcd_to_ohci_omap_priv(hcd);
 	bool do_wakeup = device_may_wakeup(&pdev->dev);
 	int ret;
 
@@ -430,7 +439,7 @@  static int ohci_omap_suspend(struct platform_device *pdev, pm_message_t message)
 	if (ret)
 		return ret;
 
-	omap_ohci_clock_power(0);
+	omap_ohci_clock_power(priv, 0);
 	return ret;
 }
 
@@ -438,12 +447,13 @@  static int ohci_omap_resume(struct platform_device *dev)
 {
 	struct usb_hcd	*hcd = platform_get_drvdata(dev);
 	struct ohci_hcd	*ohci = hcd_to_ohci(hcd);
+	struct ohci_omap_priv *priv = hcd_to_ohci_omap_priv(hcd);
 
 	if (time_before(jiffies, ohci->next_statechange))
 		msleep(5);
 	ohci->next_statechange = jiffies;
 
-	omap_ohci_clock_power(1);
+	omap_ohci_clock_power(priv, 1);
 	ohci_resume(hcd, false);
 	return 0;
 }
@@ -470,7 +480,8 @@  static struct platform_driver ohci_hcd_omap_driver = {
 
 static const struct ohci_driver_overrides omap_overrides __initconst = {
 	.product_desc	= "OMAP OHCI",
-	.reset		= ohci_omap_reset
+	.reset		= ohci_omap_reset,
+	.extra_priv_size = sizeof(struct ohci_omap_priv),
 };
 
 static int __init ohci_omap_init(void)