diff mbox series

[01/21] pinctrl: ti: iodelay: Use scope based of_node_put() cleanups

Message ID 20240501-pinctrl-cleanup-v1-1-797ceca46e5c@nxp.com
State New
Headers show
Series pinctrl: Use scope based of_node_put() cleanups | expand

Commit Message

Peng Fan (OSS) May 1, 2024, 12:55 p.m. UTC 
From: Peng Fan <peng.fan@nxp.com>

Use scope based of_node_put() cleanup to simplify code.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/pinctrl/ti/pinctrl-ti-iodelay.c | 37 +++++++++++++--------------------
 1 file changed, 14 insertions(+), 23 deletions(-)

Comments

Dan Carpenter May 1, 2024, 1:32 p.m. UTC | #1 
On Wed, May 01, 2024 at 08:55:59PM +0800, Peng Fan (OSS) wrote:
> @@ -879,16 +874,12 @@ static int ti_iodelay_probe(struct platform_device *pdev)
>  	ret = pinctrl_register_and_init(&iod->desc, dev, iod, &iod->pctl);
>  	if (ret) {
>  		dev_err(dev, "Failed to register pinctrl\n");
> -		goto exit_out;
> +		return ret;
>  	}
>  
>  	platform_set_drvdata(pdev, iod);
>  
>  	return pinctrl_enable(iod->pctl);
> -
> -exit_out:
> -	of_node_put(np);
> -	return ret;
>  }

This will call of_node_put() on the success path so it's a behavior
change.  The original code is buggy, it's supposed to call of_node_put()
on the success path here or in ti_iodelay_remove().

If it's supposed to call of_node_put() here, then fine, this is bugfix
but if it's supposed to call it in ti_iodelay_remove() then we need to
save the pointer somewhere using no_free_ptr().  Probably saving ->np
is the safest choice?

The original code is already a little bit buggy because it doesn't
check for pinctrl_enable() errors and cleanup.


diff --git a/drivers/pinctrl/ti/pinctrl-ti-iodelay.c b/drivers/pinctrl/ti/pinctrl-ti-iodelay.c
index 040f2c46a868..f40a1476e4ff 100644
--- a/drivers/pinctrl/ti/pinctrl-ti-iodelay.c
+++ b/drivers/pinctrl/ti/pinctrl-ti-iodelay.c
@@ -156,6 +156,7 @@ struct ti_iodelay_device {
 
 	const struct ti_iodelay_reg_data *reg_data;
 	struct ti_iodelay_reg_values reg_init_conf_values;
+	struct device_node *np;
 };
 
 /**
@@ -884,7 +885,12 @@ static int ti_iodelay_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, iod);
 
-	return pinctrl_enable(iod->pctl);
+	ret = pinctrl_enable(iod->pctl);
+	if (ret)
+		goto exit_out;
+
+	iod->np = no_free_ptr(np);
+	return 0;
 
 exit_out:
 	of_node_put(np);
@@ -903,6 +909,7 @@ static void ti_iodelay_remove(struct platform_device *pdev)
 		pinctrl_unregister(iod->pctl);
 
 	ti_iodelay_pinconf_deinit_dev(iod);
+	of_node_put(iod->np);
 
 	/* Expect other allocations to be freed by devm */
 }
Peng Fan May 2, 2024, 12:28 a.m. UTC | #2 
> Subject: Re: [PATCH 01/21] pinctrl: ti: iodelay: Use scope based of_node_put()
> cleanups
> 
> On Wed, May 01, 2024 at 08:55:59PM +0800, Peng Fan (OSS) wrote:
> > @@ -879,16 +874,12 @@ static int ti_iodelay_probe(struct
> platform_device *pdev)
> >  	ret = pinctrl_register_and_init(&iod->desc, dev, iod, &iod->pctl);
> >  	if (ret) {
> >  		dev_err(dev, "Failed to register pinctrl\n");
> > -		goto exit_out;
> > +		return ret;
> >  	}
> >
> >  	platform_set_drvdata(pdev, iod);
> >
> >  	return pinctrl_enable(iod->pctl);
> > -
> > -exit_out:
> > -	of_node_put(np);
> > -	return ret;
> >  }
> 
> This will call of_node_put() on the success path so it's a behavior change.  The
> original code is buggy, it's supposed to call of_node_put() on the success path
> here or in ti_iodelay_remove().
> 
> If it's supposed to call of_node_put() here, then fine, this is bugfix but if it's
> supposed to call it in ti_iodelay_remove() then we need to save the pointer
> somewhere using no_free_ptr().  Probably saving ->np is the safest choice?
> 
> The original code is already a little bit buggy because it doesn't check for
> pinctrl_enable() errors and cleanup.

It was introduced by 
commit 6118714275f0a313ecc296a87ed1af32d9691bed (tag: pinctrl-v4.11-4)
Author: Tony Lindgren <tony@atomide.com>
Date:   Thu Mar 30 09:16:39 2017 -0700

    pinctrl: core: Fix pinctrl_register_and_init() with pinctrl_enable()

of_node_put is expected in probe, not in remove.

Thanks,
Peng.

> 
> 
> diff --git a/drivers/pinctrl/ti/pinctrl-ti-iodelay.c b/drivers/pinctrl/ti/pinctrl-ti-
> iodelay.c
> index 040f2c46a868..f40a1476e4ff 100644
> --- a/drivers/pinctrl/ti/pinctrl-ti-iodelay.c
> +++ b/drivers/pinctrl/ti/pinctrl-ti-iodelay.c
> @@ -156,6 +156,7 @@ struct ti_iodelay_device {
> 
>  	const struct ti_iodelay_reg_data *reg_data;
>  	struct ti_iodelay_reg_values reg_init_conf_values;
> +	struct device_node *np;
>  };
> 
>  /**
> @@ -884,7 +885,12 @@ static int ti_iodelay_probe(struct platform_device
> *pdev)
> 
>  	platform_set_drvdata(pdev, iod);
> 
> -	return pinctrl_enable(iod->pctl);
> +	ret = pinctrl_enable(iod->pctl);
> +	if (ret)
> +		goto exit_out;
> +
> +	iod->np = no_free_ptr(np);
> +	return 0;
> 
>  exit_out:
>  	of_node_put(np);
> @@ -903,6 +909,7 @@ static void ti_iodelay_remove(struct platform_device
> *pdev)
>  		pinctrl_unregister(iod->pctl);
> 
>  	ti_iodelay_pinconf_deinit_dev(iod);
> +	of_node_put(iod->np);
> 
>  	/* Expect other allocations to be freed by devm */  }
> 
> 
>
Dan Carpenter May 2, 2024, 7:05 a.m. UTC | #3 
On Thu, May 02, 2024 at 12:28:42AM +0000, Peng Fan wrote:
> > Subject: Re: [PATCH 01/21] pinctrl: ti: iodelay: Use scope based of_node_put()
> > cleanups
> > 
> > On Wed, May 01, 2024 at 08:55:59PM +0800, Peng Fan (OSS) wrote:
> > > @@ -879,16 +874,12 @@ static int ti_iodelay_probe(struct
> > platform_device *pdev)
> > >  	ret = pinctrl_register_and_init(&iod->desc, dev, iod, &iod->pctl);
> > >  	if (ret) {
> > >  		dev_err(dev, "Failed to register pinctrl\n");
> > > -		goto exit_out;
> > > +		return ret;
> > >  	}
> > >
> > >  	platform_set_drvdata(pdev, iod);
> > >
> > >  	return pinctrl_enable(iod->pctl);
> > > -
> > > -exit_out:
> > > -	of_node_put(np);
> > > -	return ret;
> > >  }
> > 
> > This will call of_node_put() on the success path so it's a behavior change.  The
> > original code is buggy, it's supposed to call of_node_put() on the success path
> > here or in ti_iodelay_remove().
> > 
> > If it's supposed to call of_node_put() here, then fine, this is bugfix but if it's
> > supposed to call it in ti_iodelay_remove() then we need to save the pointer
> > somewhere using no_free_ptr().  Probably saving ->np is the safest choice?
> > 
> > The original code is already a little bit buggy because it doesn't check for
> > pinctrl_enable() errors and cleanup.
> 
> It was introduced by 
> commit 6118714275f0a313ecc296a87ed1af32d9691bed (tag: pinctrl-v4.11-4)
> Author: Tony Lindgren <tony@atomide.com>
> Date:   Thu Mar 30 09:16:39 2017 -0700
> 
>     pinctrl: core: Fix pinctrl_register_and_init() with pinctrl_enable()
> 
> of_node_put is expected in probe, not in remove.
> 

Ah, right.  You'll add that for the Fixes tag obviously...

regards,
dan carpenter
diff mbox series

Patch

diff --git a/drivers/pinctrl/ti/pinctrl-ti-iodelay.c b/drivers/pinctrl/ti/pinctrl-ti-iodelay.c
index 040f2c46a868..1032bc9c36aa 100644
--- a/drivers/pinctrl/ti/pinctrl-ti-iodelay.c
+++ b/drivers/pinctrl/ti/pinctrl-ti-iodelay.c
@@ -822,53 +822,48 @@  MODULE_DEVICE_TABLE(of, ti_iodelay_of_match);
 static int ti_iodelay_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
-	struct device_node *np = of_node_get(dev->of_node);
+	struct device_node *np __free(device_node) = of_node_get(dev->of_node);
 	struct resource *res;
 	struct ti_iodelay_device *iod;
-	int ret = 0;
+	int ret;
 
 	if (!np) {
-		ret = -EINVAL;
 		dev_err(dev, "No OF node\n");
-		goto exit_out;
+		return -EINVAL;
 	}
 
 	iod = devm_kzalloc(dev, sizeof(*iod), GFP_KERNEL);
-	if (!iod) {
-		ret = -ENOMEM;
-		goto exit_out;
-	}
+	if (!iod)
+		return -ENOMEM;
+
 	iod->dev = dev;
 	iod->reg_data = device_get_match_data(dev);
 	if (!iod->reg_data) {
-		ret = -EINVAL;
 		dev_err(dev, "No DATA match\n");
-		goto exit_out;
+		return -EINVAL;
 	}
 
 	/* So far We can assume there is only 1 bank of registers */
 	iod->reg_base = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
-	if (IS_ERR(iod->reg_base)) {
-		ret = PTR_ERR(iod->reg_base);
-		goto exit_out;
-	}
+	if (IS_ERR(iod->reg_base))
+		return PTR_ERR(iod->reg_base);
+
 	iod->phys_base = res->start;
 
 	iod->regmap = devm_regmap_init_mmio(dev, iod->reg_base,
 					    iod->reg_data->regmap_config);
 	if (IS_ERR(iod->regmap)) {
 		dev_err(dev, "Regmap MMIO init failed.\n");
-		ret = PTR_ERR(iod->regmap);
-		goto exit_out;
+		return PTR_ERR(iod->regmap);
 	}
 
 	ret = ti_iodelay_pinconf_init_dev(iod);
 	if (ret)
-		goto exit_out;
+		return ret;
 
 	ret = ti_iodelay_alloc_pins(dev, iod, res->start);
 	if (ret)
-		goto exit_out;
+		return ret;
 
 	iod->desc.pctlops = &ti_iodelay_pinctrl_ops;
 	/* no pinmux ops - we are pinconf */
@@ -879,16 +874,12 @@  static int ti_iodelay_probe(struct platform_device *pdev)
 	ret = pinctrl_register_and_init(&iod->desc, dev, iod, &iod->pctl);
 	if (ret) {
 		dev_err(dev, "Failed to register pinctrl\n");
-		goto exit_out;
+		return ret;
 	}
 
 	platform_set_drvdata(pdev, iod);
 
 	return pinctrl_enable(iod->pctl);
-
-exit_out:
-	of_node_put(np);
-	return ret;
 }
 
 /**