[v2] pinctrl:sunplus: Add check for kmalloc

Fix Smatch static checker warning:
potential null dereference 'configs'. (kmalloc returns null)

Changes in v2:
1. Add free allocated memory before returned -ENOMEM.
2. Add call of_node_put() before returned -ENOMEM.

Fixes: aa74c44be19c ("pinctrl: Add driver for Sunplus SP7021")
Signed-off-by: Wells Lu <wellslutw@gmail.com>
---
 drivers/pinctrl/sunplus/sppctl.c | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

Sun, May 28, 2023 at 08:34:37PM +0800, Wells Lu kirjoitti:
> Fix Smatch static checker warning:
> potential null dereference 'configs'. (kmalloc returns null)

> Changes in v2:
> 1. Add free allocated memory before returned -ENOMEM.
> 2. Add call of_node_put() before returned -ENOMEM.

The placeholder for this is after the cutter '---' line.
I think it may be fixed during application by Linus W.

Otherwise LGTM,
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> Fixes: aa74c44be19c ("pinctrl: Add driver for Sunplus SP7021")
> Signed-off-by: Wells Lu <wellslutw@gmail.com>
> ---
>  drivers/pinctrl/sunplus/sppctl.c | 24 ++++++++++++++++++------
>  1 file changed, 18 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/pinctrl/sunplus/sppctl.c b/drivers/pinctrl/sunplus/sppctl.c
> index 6bbbab3..e91ce5b 100644
> --- a/drivers/pinctrl/sunplus/sppctl.c
> +++ b/drivers/pinctrl/sunplus/sppctl.c
> @@ -834,11 +834,6 @@ static int sppctl_dt_node_to_map(struct pinctrl_dev *pctldev, struct device_node
>  	int i, size = 0;
>  
>  	list = of_get_property(np_config, "sunplus,pins", &size);
> -
> -	if (nmG <= 0)
> -		nmG = 0;
> -
> -	parent = of_get_parent(np_config);
>  	*num_maps = size / sizeof(*list);
>  
>  	/*
> @@ -866,10 +861,14 @@ static int sppctl_dt_node_to_map(struct pinctrl_dev *pctldev, struct device_node
>  		}
>  	}
>  
> +	if (nmG <= 0)
> +		nmG = 0;
> +
>  	*map = kcalloc(*num_maps + nmG, sizeof(**map), GFP_KERNEL);
> -	if (*map == NULL)
> +	if (!(*map))
>  		return -ENOMEM;
>  
> +	parent = of_get_parent(np_config);
>  	for (i = 0; i < (*num_maps); i++) {
>  		dt_pin = be32_to_cpu(list[i]);
>  		pin_num = FIELD_GET(GENMASK(31, 24), dt_pin);
> @@ -883,6 +882,8 @@ static int sppctl_dt_node_to_map(struct pinctrl_dev *pctldev, struct device_node
>  			(*map)[i].data.configs.num_configs = 1;
>  			(*map)[i].data.configs.group_or_pin = pin_get_name(pctldev, pin_num);
>  			configs = kmalloc(sizeof(*configs), GFP_KERNEL);
> +			if (!configs)
> +				goto sppctl_map_err;
>  			*configs = FIELD_GET(GENMASK(7, 0), dt_pin);
>  			(*map)[i].data.configs.configs = configs;
>  
> @@ -896,6 +897,8 @@ static int sppctl_dt_node_to_map(struct pinctrl_dev *pctldev, struct device_node
>  			(*map)[i].data.configs.num_configs = 1;
>  			(*map)[i].data.configs.group_or_pin = pin_get_name(pctldev, pin_num);
>  			configs = kmalloc(sizeof(*configs), GFP_KERNEL);
> +			if (!configs)
> +				goto sppctl_map_err;
>  			*configs = SPPCTL_IOP_CONFIGS;
>  			(*map)[i].data.configs.configs = configs;
>  
> @@ -965,6 +968,15 @@ static int sppctl_dt_node_to_map(struct pinctrl_dev *pctldev, struct device_node
>  	of_node_put(parent);
>  	dev_dbg(pctldev->dev, "%d pins mapped\n", *num_maps);
>  	return 0;
> +
> +sppctl_map_err:
> +	for (i = 0; i < (*num_maps); i++)
> +		if (((*map)[i].type == PIN_MAP_TYPE_CONFIGS_PIN) &&
> +		    (*map)[i].data.configs.configs)
> +			kfree((*map)[i].data.configs.configs);
> +	kfree(*map);
> +	of_node_put(parent);
> +	return -ENOMEM;
>  }
>  
>  static const struct pinctrl_ops sppctl_pctl_ops = {
> -- 
> 2.7.4
>

On Sun, May 28, 2023 at 2:35 PM Wells Lu <wellslutw@gmail.com> wrote:

> Fix Smatch static checker warning:
> potential null dereference 'configs'. (kmalloc returns null)
>
> Changes in v2:
> 1. Add free allocated memory before returned -ENOMEM.
> 2. Add call of_node_put() before returned -ENOMEM.
>
> Fixes: aa74c44be19c ("pinctrl: Add driver for Sunplus SP7021")
> Signed-off-by: Wells Lu <wellslutw@gmail.com>

Patch applied as non-urgent fix, dropped the changelog as pointed
out by Andy.

Yours,
Linus Walleij

On Tue, Jun 6, 2023 at 4:26 PM Markus Elfring <Markus.Elfring@web.de> wrote:
>
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Tue, 6 Jun 2023 15:00:18 +0200

You need to utilize what MAINTAINERS file has.

> It can be known that the function “kfree” performs a null pointer check
> for its input parameter.
> It is therefore not needed to repeat such a check before its call.
>
> Thus remove a redundant pointer check.

Seems reasonable to me.
FWIW,
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/pinctrl/sunplus/sppctl.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/pinctrl/sunplus/sppctl.c b/drivers/pinctrl/sunplus/sppctl.c
> index e91ce5b5d559..150996949ede 100644
> --- a/drivers/pinctrl/sunplus/sppctl.c
> +++ b/drivers/pinctrl/sunplus/sppctl.c
> @@ -971,8 +971,7 @@ static int sppctl_dt_node_to_map(struct pinctrl_dev *pctldev, struct device_node
>
>  sppctl_map_err:
>         for (i = 0; i < (*num_maps); i++)
> -               if (((*map)[i].type == PIN_MAP_TYPE_CONFIGS_PIN) &&
> -                   (*map)[i].data.configs.configs)
> +               if ((*map)[i].type == PIN_MAP_TYPE_CONFIGS_PIN)
>                         kfree((*map)[i].data.configs.configs);
>         kfree(*map);
>         of_node_put(parent);
> --
> 2.40.1
>

On Tue, Jun 6, 2023 at 3:26 PM Markus Elfring <Markus.Elfring@web.de> wrote:

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Tue, 6 Jun 2023 15:00:18 +0200
>
> It can be known that the function “kfree” performs a null pointer check
> for its input parameter.
> It is therefore not needed to repeat such a check before its call.
>
> Thus remove a redundant pointer check.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>

Lu Hongfei sent a patch changing the code around this location
so I rebased it manually and applied, check the result, thanks!

Yours,
Linus Walleij