Re: [PATCH 1/5] IA64-IRQ: Use kmalloc_array() in sn_irq_lh_init()

[Joe Perches <joe@perches.com>]

On Fri, 2016-08-26 at 15:57 -0400, Julia Lawall wrote:
> On Fri, 26 Aug 2016, SF Markus Elfring wrote:
> > From: Markus Elfring <elfring@users.sourceforge.net>
> > Date: Fri, 26 Aug 2016 18:32:53 +0200
> >
> > * A multiplication for the size determination of a memory allocation
> >   indicated that an array data structure should be processed.
> >   Thus use the corresponding function "kmalloc_array".
> >
> >   This issue was detected by using the Coccinelle software.
> >
> > * Replace the specification of data structures by pointer dereferences
> >   to make the corresponding size determination a bit safer according to
> >   the Linux coding style convention.
> >
> > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> > ---
> >  arch/ia64/sn/kernel/irq.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/ia64/sn/kernel/irq.c b/arch/ia64/sn/kernel/irq.c
[]
> > @@ -474,12 +474,12 @@ void __init sn_irq_lh_init(void)
> >  {
> >       int i;
> >
> > -     sn_irq_lh = kmalloc(sizeof(struct list_head *) * NR_IRQS, GFP_KERNEL);
> > +     sn_irq_lh = kmalloc_array(NR_IRQS, sizeof(*sn_irq_lh), GFP_KERNEL);
> >       if (!sn_irq_lh)
> >               panic("SN PCI INIT: Failed to allocate memory for PCI init\n");
> >
> >       for (i = 0; i < NR_IRQS; i++) {
> > -             sn_irq_lh[i] = kmalloc(sizeof(struct list_head), GFP_KERNEL);
> > +             sn_irq_lh[i] = kmalloc(*sn_irq_lh[i], GFP_KERNEL);
> 
> Did a sizeof get lost here?

Yes, thanks Julia.

This is why adding the generating spatch code is always good.

And Markus, please always compile test your code using the
appropriate cross-compilers available here:
https://www.kernel.org/pub/tools/crosstool/

And btw: using sizeof(*pp[i]) or sizeof(**pp) is not always
clearer or better than using sizeof(type)

If you _really wanted to clear up this code and make it more
robust/better, it'd probably be nicer to convert the
struct list_head **sn_irq_lh to a single struct list_head *
and do a single
	struct list_head *sn_irq_la = malloc_array(nr_irqs, sizeof(struct list_head);
instead of an malloc_array of the *sn_irq_lh and the
multiple individual struct list_head malloc entries
and change the INIT_LIST_HEAD and indexing code.

That would be less data space overall given the alignment
waste of the individual allocs.

It also appears that sn_irq_lh is extern in
arch/ia64/include/asm/sn/intr.h but could be made static
to arch/ia64/sn/kernel/irq.c.

But really, the conversion isn't worthwhile as NR_IRQS is
limited to much less than the maximum allocation / sizeof(ptr).

--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html