Re: [PATCH v3 4/4] net: mana: Allocate MSI-X vectors dynamically

[Yury Norov <yury.norov@gmail.com>]

On Wed, May 14, 2025 at 05:04:03AM +0000, Michael Kelley wrote:
> From: Shradha Gupta <shradhagupta@linux.microsoft.com> Sent: Friday, May 9, 2025 3:14 AM
> > 
> > Currently, the MANA driver allocates MSI-X vectors statically based on
> > MANA_MAX_NUM_QUEUES and num_online_cpus() values and in some cases ends
> > up allocating more vectors than it needs. This is because, by this time
> > we do not have a HW channel and do not know how many IRQs should be
> > allocated.
> > 
> > To avoid this, we allocate 1 MSI-X vector during the creation of HWC and
> > after getting the value supported by hardware, dynamically add the
> > remaining MSI-X vectors.
> 
> After this patch is applied, there are two functions for setting up IRQs:
> 1. mana_gd_setup_dyn_irqs()
> 2. mana_gd_setup_irqs()
> 
> #1 is about 78 lines of code and comments, while #2 is about 103 lines of
> code and comments. But the two functions have a lot of commonality,
> and that amount of commonality raises a red flag for me.
> 
> Have you looked at parameterizing things so a single function can serve
> both purposes? I haven't worked through all the details, but at first
> glance it looks very feasible, and without introducing unreasonable
> messiness. Saving 70 to 80 lines of fairly duplicative code is worth a bit
> of effort.
> 
> I have some other comments on the code. But if those two functions can
> be combined, I'd rather re-review the result before adding comments that
> may become irrelevant due to the restructuring.
> 
> Michael

On previous iteration I already mentioned that this patch is too big,
doesn't do exactly one thing and should be split to become a reviewable 
change. Shradha split-out the irq_setup() piece, and your review proves
that splitting helps to reviewability.

The rest of the change is still a mess. I agree that the functions you
mention likely duplicate each other. But overall, this patch bomb is
above my ability to review. Fortunately, I don't have to.

Thanks,
Yury