>-----Original Message-----
>From: Yury Norov <[email protected]>
>Sent: Tuesday, December 12, 2023 10:04 PM
>To: Souradeep Chakrabarti <[email protected]>
>Cc: KY Srinivasan <[email protected]>; Haiyang Zhang
><[email protected]>; [email protected]; Dexuan Cui
><[email protected]>; [email protected]; [email protected];
>[email protected]; [email protected]; Long Li <[email protected]>;
>[email protected]; [email protected]; [email protected];
>[email protected]; [email protected]; [email protected];
>[email protected]; [email protected]; linux-
>[email protected]; Souradeep Chakrabarti <[email protected]>;
>Paul Rosswurm <[email protected]>
>Subject: [EXTERNAL] Re: [PATCH V5 net-next] net: mana: Assigning IRQ affinity
>on
>HT cores
>
>[Some people who received this message don't often get email from
>[email protected]. Learn why this is important at
>https://aka.ms/LearnAboutSenderIdentification ]
>
>> > > > > + rcu_read_lock();
>> > > > > + for_each_numa_hop_mask(next, next_node) {
>> > > > > + cpumask_andnot(curr, next, prev);
>> > > > > + for (w = cpumask_weight(curr), cnt = 0; cnt < w; ) {
>> > > > > + cpumask_copy(cpus, curr);
>> > > > > + for_each_cpu(cpu, cpus) {
>> > > > > + irq_set_affinity_and_hint(irqs[i],
>topology_sibling_cpumask(cpu));
>> > > > > + if (++i == nvec)
>> > > > > + goto done;
>> > > >
>> > > > Think what if you're passed with irq_setup(NULL, 0, 0).
>> > > > That's why I suggested to place this check at the beginning.
>> > > >
>> > > irq_setup() is a helper function for mana_gd_setup_irqs(), which
>> > > already takes care of no NULL pointer for irqs, and 0 number of
>> > > interrupts can
>not be passed.
>> > >
>> > > nvec = pci_alloc_irq_vectors(pdev, 2, max_irqs, PCI_IRQ_MSIX); if
>> > > (nvec < 0)
>> > > return nvec;
>> >
>> > I know that. But still it's a bug. The common convention is that if
>> > a 0-length array is passed to a function, it should not dereference
>> > the pointer.
>> >
>> I will add one if check in the begining of irq_setup() to verify the
>> pointer and the nvec number.
>
>Yes you can, but what for? This is an error anyways, and you don't care about
>early
>return. So instead of adding and bearing extra logic, I'd just swap 2 lines of
>existing
>code.
Problem with the code you had proposed is shown below:
> ./a.out
i is 1
i is 2
i is 3
i is 4
i is 5
i is 6
i is 7
i is 8
i is 9
i is 10
in done
lisatest ~
> cat test3.c
#include<stdio.h>
main() {
int i = 0, cur, nvec = 10;
for (cur = 0; cur < 20; cur++) {
if (i++ == nvec)
goto done;
printf(" i is %d\n", i);
}
done:
printf("in done\n");
}
So now it is because post increment operator in i++,
For that reason in the posposed code we will hit irqs[nvec], which may cause
crash, as size of
irqs is nvec.
Now if we preincrement, then we will loop correctly, but nvec == 0 check will
not happen.
Like here with preincrement in above code we are not hitting (i == nvec) .
> ./a.out
i is 1
i is 2
i is 3
i is 4
i is 5
i is 6
i is 7
i is 8
i is 9
in done
So with preincrement if we want the check for nvec == 0, we will need the check
with extra if condition
before the loop.
