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

[SF Markus Elfring <elfring@users.sourceforge.net>]

>>> @@ -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.

Unfortunately, another copy mistake happened during a bit of
source code editing.


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

I find that this broken update suggestion can point a few details out
for further considerations.

I dared to combine some software aspects once more in this use case.
Such a combination (join point) shows interesting challenges,
doesn't it?


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

Thanks for your link.


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

Do you express a target conflict between your expectations
and the evolving Linux coding style documentation here?

Would any software developers insist to see the corresponding
data type directly instead of "evaluating" a pointer expression?


> 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 *
…
> That would be less data space overall given the alignment
> waste of the individual allocs.

Does this suggestion mean that I should drop my proposal
around the software components "IRQ" and "TLB" for the system
architecture "IA64" in such a questionable patch series?

Regards,
Markus