[PATCH] pinctrl: rza1: report if devm_kasptrinf() fails

[PATCH] pinctrl: rza1: report if devm_kasptrinf() fails

[Nicholas Mc Guire]

devm_kasprintf() may return NULL on failure of internal allocation 
thus the assignments are not safe if not checked. On error
rza1_pinctrl_register() respectively rza1_parse_gpiochip() return
negative values so -ENOMEM in the (unlikely) failure case of 
devm_kasprintf() should be fine here. 

Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
Fixes: 5a49b644b307 ("pinctrl: Renesas RZ/A1 pin and gpio controller")
---

Problem was located with an experimental coccinelle script

The dev_err() for this unlikely case is added so that a failures of
rza1_pinctrl_register() respectively rza1_parse_gpiochip() can be
understood as currently the negative return value is simply passed up
the call stack but it would be hard to identify the cause.

Patch was compile tested with: shmobile_defconfig (implies PINCTRL_RZA1=y)

Patch is against 4.20-rc3 (localversion-next is next-20181123)

 drivers/pinctrl/pinctrl-rza1.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/pinctrl/pinctrl-rza1.c b/drivers/pinctrl/pinctrl-rza1.c
index 14eb576..2533df4 100644
--- a/drivers/pinctrl/pinctrl-rza1.c
+++ b/drivers/pinctrl/pinctrl-rza1.c
@@ -1225,6 +1225,11 @@ static int rza1_parse_gpiochip(struct rza1_pinctrl *rza1_pctl,
 	chip->base	= -1;
 	chip->label	= devm_kasprintf(rza1_pctl->dev, GFP_KERNEL, "%pOFn",
 					 np);
+	if (!chip->label) {
+		dev_err(rza1_pctl->dev, "Failed to allocate label\n");
+		return -ENOMEM;
+	}
+
 	chip->ngpio	= of_args.args[2];
 	chip->of_node	= np;
 	chip->parent	= rza1_pctl->dev;
@@ -1326,6 +1331,11 @@ static int rza1_pinctrl_register(struct rza1_pinctrl *rza1_pctl)
 		pins[i].number = i;
 		pins[i].name = devm_kasprintf(rza1_pctl->dev, GFP_KERNEL,
 					      "P%u-%u", port, pin);
+		if (!pins[i].name) {
+			dev_err(rza1_pctl->dev,
+				"RZ/A1 pin controller allocation failed\n");
+			return -ENOMEM;
+		}
 
 		if (i % RZA1_PINS_PER_PORT == 0) {
 			/*
-- 
2.1.4

Re: [PATCH] pinctrl: rza1: report if devm_kasptrinf() fails

[Linus Walleij]

On Sat, Nov 24, 2018 at 5:34 PM Nicholas Mc Guire <hofrat@osadl.org> wrote:

> devm_kasprintf() may return NULL on failure of internal allocation
> thus the assignments are not safe if not checked. On error
> rza1_pinctrl_register() respectively rza1_parse_gpiochip() return
> negative values so -ENOMEM in the (unlikely) failure case of
> devm_kasprintf() should be fine here.
>
> Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
> Fixes: 5a49b644b307 ("pinctrl: Renesas RZ/A1 pin and gpio controller")

This patch needs to be handled by Geert Uytterhoeven,
who maintain the Renesas pin controllers.

If he didn't get it you might have to resend the patch to him.

Yours,
Linus Walleij

Re: [PATCH] pinctrl: rza1: report if devm_kasptrinf() fails

[jacopo mondi]

[-- Attachment #1: Type: text/plain, Size: 2351 bytes --]

Hi Nicholas, Linus,
   I'm sorry I might have missed this

On Sat, Nov 24, 2018 at 05:30:33PM +0100, Nicholas Mc Guire wrote:
> devm_kasprintf() may return NULL on failure of internal allocation
> thus the assignments are not safe if not checked. On error
> rza1_pinctrl_register() respectively rza1_parse_gpiochip() return
> negative values so -ENOMEM in the (unlikely) failure case of
> devm_kasprintf() should be fine here.

devm_kasprintf() returns -ENOMEM in case of failure (which I agree it's
unlikely, but still...), so I guess it's fine returning -ENOMEM as well
here.

Acked-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
   j

>
> Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
> Fixes: 5a49b644b307 ("pinctrl: Renesas RZ/A1 pin and gpio controller")
> ---
>
> Problem was located with an experimental coccinelle script
>
> The dev_err() for this unlikely case is added so that a failures of
> rza1_pinctrl_register() respectively rza1_parse_gpiochip() can be
> understood as currently the negative return value is simply passed up
> the call stack but it would be hard to identify the cause.
>
> Patch was compile tested with: shmobile_defconfig (implies PINCTRL_RZA1=y)
>
> Patch is against 4.20-rc3 (localversion-next is next-20181123)
>
>  drivers/pinctrl/pinctrl-rza1.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/drivers/pinctrl/pinctrl-rza1.c b/drivers/pinctrl/pinctrl-rza1.c
> index 14eb576..2533df4 100644
> --- a/drivers/pinctrl/pinctrl-rza1.c
> +++ b/drivers/pinctrl/pinctrl-rza1.c
> @@ -1225,6 +1225,11 @@ static int rza1_parse_gpiochip(struct rza1_pinctrl *rza1_pctl,
>  	chip->base	= -1;
>  	chip->label	= devm_kasprintf(rza1_pctl->dev, GFP_KERNEL, "%pOFn",
>  					 np);
> +	if (!chip->label) {
> +		dev_err(rza1_pctl->dev, "Failed to allocate label\n");
> +		return -ENOMEM;
> +	}
> +
>  	chip->ngpio	= of_args.args[2];
>  	chip->of_node	= np;
>  	chip->parent	= rza1_pctl->dev;
> @@ -1326,6 +1331,11 @@ static int rza1_pinctrl_register(struct rza1_pinctrl *rza1_pctl)
>  		pins[i].number = i;
>  		pins[i].name = devm_kasprintf(rza1_pctl->dev, GFP_KERNEL,
>  					      "P%u-%u", port, pin);
> +		if (!pins[i].name) {
> +			dev_err(rza1_pctl->dev,
> +				"RZ/A1 pin controller allocation failed\n");
> +			return -ENOMEM;
> +		}
>
>  		if (i % RZA1_PINS_PER_PORT == 0) {
>  			/*
> --
> 2.1.4
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] pinctrl: rza1: report if devm_kasptrinf() fails

[Geert Uytterhoeven]

Hi Nicholas,

On Sat, Nov 24, 2018 at 5:35 PM Nicholas Mc Guire <hofrat@osadl.org> wrote:
> devm_kasprintf() may return NULL on failure of internal allocation
> thus the assignments are not safe if not checked. On error
> rza1_pinctrl_register() respectively rza1_parse_gpiochip() return
> negative values so -ENOMEM in the (unlikely) failure case of
> devm_kasprintf() should be fine here.
>
> Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
> Fixes: 5a49b644b307 ("pinctrl: Renesas RZ/A1 pin and gpio controller")

Thanks for your patch!

> The dev_err() for this unlikely case is added so that a failures of
> rza1_pinctrl_register() respectively rza1_parse_gpiochip() can be
> understood as currently the negative return value is simply passed up
> the call stack but it would be hard to identify the cause.

Adding the dev_err() is not needed, as the memory management core
print a message on memory allocation failures anyway.
Hence please remove it.

With the above fixed:
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds