clk: davinci: fix regmap use PTR_ERR after initial

Commit Message

Jian Dong March 23, 2021, 7:22 a.m. UTC

From: Jian Dong <[e-mail]>

fixes coccicheck ERROR:

drivers/clk/davinci/da8xx-cfgchip.c:768:18-25: ERROR:
PTR_ERR applied after initialization to constant on line 746

Signed-off-by: Jian Dong <[e-mail]>
---
 drivers/clk/davinci/da8xx-cfgchip.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

David Lechner March 23, 2021, 4:03 p.m. UTC | #1

On 3/23/21 2:22 AM, Jian Dong wrote:
> From: Jian Dong <[e-mail]>
> 
> fixes coccicheck ERROR:
> 
> drivers/clk/davinci/da8xx-cfgchip.c:768:18-25: ERROR:
> PTR_ERR applied after initialization to constant on line 746
> 
> Signed-off-by: Jian Dong <[e-mail]>
> ---
>   drivers/clk/davinci/da8xx-cfgchip.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/davinci/da8xx-cfgchip.c b/drivers/clk/davinci/da8xx-cfgchip.c
> index 77d1827..f57ba1b 100644
> --- a/drivers/clk/davinci/da8xx-cfgchip.c
> +++ b/drivers/clk/davinci/da8xx-cfgchip.c
> @@ -743,7 +743,7 @@ static int da8xx_cfgchip_probe(struct platform_device *pdev)
>   	struct da8xx_cfgchip_clk_platform_data *pdata = dev->platform_data;
>   	const struct of_device_id *of_id;
>   	da8xx_cfgchip_init clk_init = NULL;
> -	struct regmap *regmap = NULL;
> +	struct regmap *regmap;
>   
>   	of_id = of_match_device(da8xx_cfgchip_of_match, dev);
>   	if (of_id) {
> 

I think it is better to leave this as-is. Otherwise some compilers
may create an error/warning about using an uninitialized value
even if it technically is not possible.

In fact, the coccicheck error seems wrong. regmap will have always
been assigned a value by the time IS_ERR_OR_NULL(regmap) is called.

Here is the full function for context:

static int da8xx_cfgchip_probe(struct platform_device *pdev)
{
	struct device *dev = &pdev->dev;
	struct da8xx_cfgchip_clk_platform_data *pdata = dev->platform_data;
	const struct of_device_id *of_id;
	da8xx_cfgchip_init clk_init = NULL;
	struct regmap *regmap = NULL;

	of_id = of_match_device(da8xx_cfgchip_of_match, dev);
	if (of_id) {
		struct device_node *parent;

		clk_init = of_id->data;
		parent = of_get_parent(dev->of_node);
		regmap = syscon_node_to_regmap(parent);
		of_node_put(parent);
	} else if (pdev->id_entry && pdata) {
		clk_init = (void *)pdev->id_entry->driver_data;
		regmap = pdata->cfgchip;
	}

	if (!clk_init) {
		dev_err(dev, "unable to find driver data\n");
		return -EINVAL;
	}

	if (IS_ERR_OR_NULL(regmap)) {
		dev_err(dev, "no regmap for CFGCHIP syscon\n");
		return regmap ? PTR_ERR(regmap) : -ENOENT;
	}

	return clk_init(dev, regmap);
}

Jian Dong March 23, 2021, 4:25 p.m. UTC | #2

On Tue, 23 Mar 2021 11:03:38 -0500
David Lechner <[e-mail]> wrote:

> On 3/23/21 2:22 AM, Jian Dong wrote:
> > From: Jian Dong <[e-mail]>
> > 
> > fixes coccicheck ERROR:
> > 
> > drivers/clk/davinci/da8xx-cfgchip.c:768:18-25: ERROR:
> > PTR_ERR applied after initialization to constant on line 746
> > 
> > Signed-off-by: Jian Dong <[e-mail]>
> > ---
> >   drivers/clk/davinci/da8xx-cfgchip.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/clk/davinci/da8xx-cfgchip.c
> > b/drivers/clk/davinci/da8xx-cfgchip.c index 77d1827..f57ba1b 100644
> > --- a/drivers/clk/davinci/da8xx-cfgchip.c
> > +++ b/drivers/clk/davinci/da8xx-cfgchip.c
> > @@ -743,7 +743,7 @@ static int da8xx_cfgchip_probe(struct
> > platform_device *pdev) struct da8xx_cfgchip_clk_platform_data
> > *pdata = dev->platform_data; const struct of_device_id *of_id;
> >   	da8xx_cfgchip_init clk_init = NULL;
> > -	struct regmap *regmap = NULL;
> > +	struct regmap *regmap;
> >   
> >   	of_id = of_match_device(da8xx_cfgchip_of_match, dev);
> >   	if (of_id) {
> >   
> 
> I think it is better to leave this as-is. Otherwise some compilers
> may create an error/warning about using an uninitialized value
> even if it technically is not possible.
> 
> In fact, the coccicheck error seems wrong. regmap will have always
> been assigned a value by the time IS_ERR_OR_NULL(regmap) is called.
> 
> Here is the full function for context:
> 
> static int da8xx_cfgchip_probe(struct platform_device *pdev)
> {
> 	struct device *dev = &pdev->dev;
> 	struct da8xx_cfgchip_clk_platform_data *pdata =
> dev->platform_data; const struct of_device_id *of_id;
> 	da8xx_cfgchip_init clk_init = NULL;
> 	struct regmap *regmap = NULL;
> 
> 	of_id = of_match_device(da8xx_cfgchip_of_match, dev);
> 	if (of_id) {
> 		struct device_node *parent;
> 
> 		clk_init = of_id->data;
> 		parent = of_get_parent(dev->of_node);
> 		regmap = syscon_node_to_regmap(parent);
> 		of_node_put(parent);
> 	} else if (pdev->id_entry && pdata) {
> 		clk_init = (void *)pdev->id_entry->driver_data;
> 		regmap = pdata->cfgchip;
> 	}
> 
> 	if (!clk_init) {
> 		dev_err(dev, "unable to find driver data\n");
> 		return -EINVAL;
> 	}
> 
> 	if (IS_ERR_OR_NULL(regmap)) {
> 		dev_err(dev, "no regmap for CFGCHIP syscon\n");
> 		return regmap ? PTR_ERR(regmap) : -ENOENT;
> 	}
> 
> 	return clk_init(dev, regmap);
> }

Hi, thanks for reply.

I see other use of PTR_ERR in this file such as
of_da8xx_usb_phy_clk_init() and of_da8xx_cfgchip_init_mux_clock(), all
PTR_ERR(value) with uninitialized, So I think maybe it is a way to fix
the coccicheck error.

In fact I'm not sure about this, initialized should be a good habit.

Stephen Boyd April 8, 2021, 7:26 a.m. UTC | #3

Quoting David Lechner (2021-03-23 09:03:38)
> On 3/23/21 2:22 AM, Jian Dong wrote:
> > From: Jian Dong <[e-mail]>
> > 
> > fixes coccicheck ERROR:
> > 
> > drivers/clk/davinci/da8xx-cfgchip.c:768:18-25: ERROR:
> > PTR_ERR applied after initialization to constant on line 746
> > 
> > Signed-off-by: Jian Dong <[e-mail]>
> > ---
> >   drivers/clk/davinci/da8xx-cfgchip.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/clk/davinci/da8xx-cfgchip.c b/drivers/clk/davinci/da8xx-cfgchip.c
> > index 77d1827..f57ba1b 100644
> > --- a/drivers/clk/davinci/da8xx-cfgchip.c
> > +++ b/drivers/clk/davinci/da8xx-cfgchip.c
> > @@ -743,7 +743,7 @@ static int da8xx_cfgchip_probe(struct platform_device *pdev)
> >       struct da8xx_cfgchip_clk_platform_data *pdata = dev->platform_data;
> >       const struct of_device_id *of_id;
> >       da8xx_cfgchip_init clk_init = NULL;
> > -     struct regmap *regmap = NULL;
> > +     struct regmap *regmap;
> >   
> >       of_id = of_match_device(da8xx_cfgchip_of_match, dev);
> >       if (of_id) {
> > 
> 
> I think it is better to leave this as-is. Otherwise some compilers
> may create an error/warning about using an uninitialized value
> even if it technically is not possible.
> 
> In fact, the coccicheck error seems wrong. regmap will have always
> been assigned a value by the time IS_ERR_OR_NULL(regmap) is called.
> 
> Here is the full function for context:
> 
> static int da8xx_cfgchip_probe(struct platform_device *pdev)
> {
>         struct device *dev = &pdev->dev;
>         struct da8xx_cfgchip_clk_platform_data *pdata = dev->platform_data;
>         const struct of_device_id *of_id;
>         da8xx_cfgchip_init clk_init = NULL;
>         struct regmap *regmap = NULL;
> 
>         of_id = of_match_device(da8xx_cfgchip_of_match, dev);
>         if (of_id) {
>                 struct device_node *parent;
> 
>                 clk_init = of_id->data;
>                 parent = of_get_parent(dev->of_node);
>                 regmap = syscon_node_to_regmap(parent);
>                 of_node_put(parent);
>         } else if (pdev->id_entry && pdata) {
>                 clk_init = (void *)pdev->id_entry->driver_data;
>                 regmap = pdata->cfgchip;
>         }
> 
>         if (!clk_init) {
>                 dev_err(dev, "unable to find driver data\n");
>                 return -EINVAL;
>         }

Would the static checkers get tripped up if this if condition was turned
into an else on the other two arms? Otherwise, why not initialize regmap
to PTR_ERR(-ENOENT) and silence the checkers? Then the usage of
IS_ERR_OR_NULL could be replaced with IS_ERR() and everyone wins?

> 
>         if (IS_ERR_OR_NULL(regmap)) {
>                 dev_err(dev, "no regmap for CFGCHIP syscon\n");
>                 return regmap ? PTR_ERR(regmap) : -ENOENT;
>         }
> 
>         return clk_init(dev, regmap);
> }

Patch

diff --git a/drivers/clk/davinci/da8xx-cfgchip.c b/drivers/clk/davinci/da8xx-cfgchip.c
index 77d1827..f57ba1b 100644
--- a/drivers/clk/davinci/da8xx-cfgchip.c
+++ b/drivers/clk/davinci/da8xx-cfgchip.c
@@ -743,7 +743,7 @@  static int da8xx_cfgchip_probe(struct platform_device *pdev)
 	struct da8xx_cfgchip_clk_platform_data *pdata = dev->platform_data;
 	const struct of_device_id *of_id;
 	da8xx_cfgchip_init clk_init = NULL;
-	struct regmap *regmap = NULL;
+	struct regmap *regmap;
 
 	of_id = of_match_device(da8xx_cfgchip_of_match, dev);
 	if (of_id) {