| Message ID | 20170506082359.6736-1-christophe.jaillet@wanadoo.fr |
|---|---|
| State | New |
| Headers | show |
Hi Christophe, On Sat, May 06, 2017 at 10:23:59AM +0200, Christophe JAILLET wrote: > If 'devm_kzalloc' fails, a NULL pointer will be dereferenced. > Return -ENOMEM instead, as done for the other memory allocation just a > few lines below. This looks fine. > BTW, change the 'devm_kzalloc' into a 'devm_kcalloc'. Any reason for the devm_kcalloc change? It looks like the next for loop does set all of the group_name values. -Stafford > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> > --- > drivers/pinctrl/freescale/pinctrl-imx.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/pinctrl/freescale/pinctrl-imx.c b/drivers/pinctrl/freescale/pinctrl-imx.c > index 74bd90dfd7b1..90a946c028ff 100644 > --- a/drivers/pinctrl/freescale/pinctrl-imx.c > +++ b/drivers/pinctrl/freescale/pinctrl-imx.c > @@ -581,9 +581,10 @@ static int imx_pinctrl_parse_functions(struct device_node *np, > dev_err(info->dev, "no groups defined in %s\n", np->full_name); > return -EINVAL; > } > - func->group_names = devm_kzalloc(info->dev, > - func->num_group_names * > + func->group_names = devm_kcalloc(info->dev, func->num_group_names, > sizeof(char *), GFP_KERNEL); > + if (!func->group_names) > + return -ENOMEM; > > for_each_child_of_node(np, child) { > func->group_names[i] = child->name; > -- > 2.11.0 > -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Le 06/05/2017 à 21:40, Stafford Horne a écrit : > Hi Christophe, > > On Sat, May 06, 2017 at 10:23:59AM +0200, Christophe JAILLET wrote: >> If 'devm_kzalloc' fails, a NULL pointer will be dereferenced. >> Return -ENOMEM instead, as done for the other memory allocation just a >> few lines below. > This looks fine. > >> BTW, change the 'devm_kzalloc' into a 'devm_kcalloc'. > Any reason for the devm_kcalloc change? It looks like the next for loop > does set all of the group_name values. > > -Stafford The semantic of kcalloc is better than kzalloc here because we allocate an array. It also checks if "func->num_group_names * sizeof(char *)" is not too big. I agree that zerroing memory could probably be avoided here. I can send a V2 without the (not that useful) change for kcalloc, or with a 'devm_kmalloc_array' if you prefer. CJ > >> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> >> --- >> drivers/pinctrl/freescale/pinctrl-imx.c | 5 +++-- >> 1 file changed, 3 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/pinctrl/freescale/pinctrl-imx.c b/drivers/pinctrl/freescale/pinctrl-imx.c >> index 74bd90dfd7b1..90a946c028ff 100644 >> --- a/drivers/pinctrl/freescale/pinctrl-imx.c >> +++ b/drivers/pinctrl/freescale/pinctrl-imx.c >> @@ -581,9 +581,10 @@ static int imx_pinctrl_parse_functions(struct device_node *np, >> dev_err(info->dev, "no groups defined in %s\n", np->full_name); >> return -EINVAL; >> } >> - func->group_names = devm_kzalloc(info->dev, >> - func->num_group_names * >> + func->group_names = devm_kcalloc(info->dev, func->num_group_names, >> sizeof(char *), GFP_KERNEL); >> + if (!func->group_names) >> + return -ENOMEM; >> >> for_each_child_of_node(np, child) { >> func->group_names[i] = child->name; >> -- >> 2.11.0 >> > -- > To unsubscribe from this list: send the line "unsubscribe linux-gpio" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, May 07, 2017 at 04:40:38AM +0900, Stafford Horne wrote: > Hi Christophe, > > On Sat, May 06, 2017 at 10:23:59AM +0200, Christophe JAILLET wrote: > > If 'devm_kzalloc' fails, a NULL pointer will be dereferenced. > > Return -ENOMEM instead, as done for the other memory allocation just a > > few lines below. > > This looks fine. > > > BTW, change the 'devm_kzalloc' into a 'devm_kcalloc'. > > Any reason for the devm_kcalloc change? It looks like the next for loop > does set all of the group_name values. > The advantage of kcalloc() over kzalloc() is the integer overflow checking. There is kmalloc_array() if we don't need to zero the memory. regards, dan carpenter -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, May 09, 2017 at 10:27:20AM +0300, Dan Carpenter wrote: > On Sun, May 07, 2017 at 04:40:38AM +0900, Stafford Horne wrote: > > Hi Christophe, > > > > On Sat, May 06, 2017 at 10:23:59AM +0200, Christophe JAILLET wrote: > > > If 'devm_kzalloc' fails, a NULL pointer will be dereferenced. > > > Return -ENOMEM instead, as done for the other memory allocation just a > > > few lines below. > > > > This looks fine. > > > > > BTW, change the 'devm_kzalloc' into a 'devm_kcalloc'. > > > > Any reason for the devm_kcalloc change? It looks like the next for loop > > does set all of the group_name values. > > > > The advantage of kcalloc() over kzalloc() is the integer overflow > checking. There is kmalloc_array() if we don't need to zero the memory. Right, usually its good to have a reason why in the commit log. i.e. BTW, change the 'devm_kzalloc' into a 'devm_kcalloc' for overflow checking. Or switch to devm_kmalloc_array() and say: for overflow checking and no need for zeroing. -Stafford -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, May 6, 2017 at 10:23 AM, Christophe JAILLET <christophe.jaillet@wanadoo.fr> wrote: > If 'devm_kzalloc' fails, a NULL pointer will be dereferenced. > Return -ENOMEM instead, as done for the other memory allocation just a > few lines below. > BTW, change the 'devm_kzalloc' into a 'devm_kcalloc'. > > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> Patch applied. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/pinctrl/freescale/pinctrl-imx.c b/drivers/pinctrl/freescale/pinctrl-imx.c index 74bd90dfd7b1..90a946c028ff 100644 --- a/drivers/pinctrl/freescale/pinctrl-imx.c +++ b/drivers/pinctrl/freescale/pinctrl-imx.c @@ -581,9 +581,10 @@ static int imx_pinctrl_parse_functions(struct device_node *np, dev_err(info->dev, "no groups defined in %s\n", np->full_name); return -EINVAL; } - func->group_names = devm_kzalloc(info->dev, - func->num_group_names * + func->group_names = devm_kcalloc(info->dev, func->num_group_names, sizeof(char *), GFP_KERNEL); + if (!func->group_names) + return -ENOMEM; for_each_child_of_node(np, child) { func->group_names[i] = child->name;
If 'devm_kzalloc' fails, a NULL pointer will be dereferenced. Return -ENOMEM instead, as done for the other memory allocation just a few lines below. BTW, change the 'devm_kzalloc' into a 'devm_kcalloc'. Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> --- drivers/pinctrl/freescale/pinctrl-imx.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)