On Wed, May 14, 2025 at 05:04:03AM +0000, Michael Kelley wrote: > From: Shradha Gupta <shradhagupta@xxxxxxxxxxxxxxxxxxx> 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
