On Thu, Apr 24, 2025 at 05:57:43PM +0100, Simon Horman wrote: > On Wed, Apr 16, 2025 at 08:36:21AM -0700, Shradha Gupta wrote: > > Currently, the MANA driver allocates pci vector 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 IRQ vector during the creation of HWC and > > after getting the value supported by hardware, dynamically add the > > remaining vectors. > > > > Signed-off-by: Shradha Gupta <shradhagu...@linux.microsoft.com> > > Reviewed-by: Haiyang Zhang <haiya...@microsoft.com> > > Hi Shradha, > > Some minor nits from my side. > > ... > > > diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c > > b/drivers/net/ethernet/microsoft/mana/gdma_main.c > > ... > > > @@ -465,9 +475,10 @@ static int mana_gd_register_irq(struct gdma_queue > > *queue, > > struct gdma_irq_context *gic; > > struct gdma_context *gc; > > unsigned int msi_index; > > - unsigned long flags; > > + struct list_head *pos; > > + unsigned long flags, flag_irq; > > struct device *dev; > > - int err = 0; > > + int err = 0, count; > > As this is Networking code, please preserve the arrangement of local > variables in reverse xmas tree order - longest line to shortest. > > Edward Cree's tool can be useful in this area: > https://github.com/ecree-solarflare/xmastree > > > > > gc = gd->gdma_context; > > dev = gc->dev; > > @@ -482,7 +493,22 @@ static int mana_gd_register_irq(struct gdma_queue > > *queue, > > } > > > > queue->eq.msix_index = msi_index; > > - gic = &gc->irq_contexts[msi_index]; > > + > > + /* get the msi_index value from the list*/ > > + count = 0; > > + spin_lock_irqsave(&gc->irq_ctxs_lock, flag_irq); > > + list_for_each(pos, &gc->irq_contexts) { > > + if (count == msi_index) { > > + gic = list_entry(pos, struct gdma_irq_context, > > gic_list); > > Please consider line wrapping to 80 columns or less, as is still preferred > in Networking code. > > Likewise elsewhere in this patch. > > checkpatch.pl --max-line-length=80 > can be helpful here. > > > + break; > > + } > > + > > + count++; > > + } > > + spin_unlock_irqrestore(&gc->irq_ctxs_lock, flag_irq); > > + > > + if (!gic) > > + return -1; > > > > spin_lock_irqsave(&gic->lock, flags); > > list_add_rcu(&queue->entry, &gic->eq_list); > > @@ -497,8 +523,10 @@ static void mana_gd_deregiser_irq(struct gdma_queue > > *queue) > > struct gdma_irq_context *gic; > > struct gdma_context *gc; > > unsigned int msix_index; > > - unsigned long flags; > > + struct list_head *pos; > > + unsigned long flags, flag_irq; > > struct gdma_queue *eq; > > + int count; > > Reverse xmas tree here too. > > > > > gc = gd->gdma_context; > > > > @@ -507,7 +535,22 @@ static void mana_gd_deregiser_irq(struct gdma_queue > > *queue) > > if (WARN_ON(msix_index >= gc->num_msix_usable)) > > return; > > > > - gic = &gc->irq_contexts[msix_index]; > > + /* get the msi_index value from the list*/ > > + count = 0; > > + spin_lock_irqsave(&gc->irq_ctxs_lock, flag_irq); > > + list_for_each(pos, &gc->irq_contexts) { > > + if (count == msix_index) { > > + gic = list_entry(pos, struct gdma_irq_context, > > gic_list); > > + break; > > + } > > + > > + count++; > > + } > > + spin_unlock_irqrestore(&gc->irq_ctxs_lock, flag_irq); > > + > > Does gic need to be initialised to NULL before the list_for_each loop > to ensure that it is always initialised here? > > Flagged by Clang 20.1.2 KCFLAGS=-Wsometimes-uninitialized builds, and Smatch > > > + if (!gic) > > + return; > > + > > spin_lock_irqsave(&gic->lock, flags); > > list_for_each_entry_rcu(eq, &gic->eq_list, entry) { > > if (queue == eq) { > > ... > > > @@ -1317,29 +1372,92 @@ static int irq_setup(unsigned int *irqs, unsigned > > int len, int node) > > return 0; > > } > > > > -static int mana_gd_setup_irqs(struct pci_dev *pdev) > > +static int mana_gd_setup_dyn_irqs(struct pci_dev *pdev, int nvec) > > { > > struct gdma_context *gc = pci_get_drvdata(pdev); > > - unsigned int max_queues_per_port; > > struct gdma_irq_context *gic; > > - unsigned int max_irqs, cpu; > > - int start_irq_index = 1; > > - int nvec, *irqs, irq; > > + int *irqs, irq, skip_first_cpu = 0; > > + unsigned long flags; > > int err, i = 0, j; > > Reverse xmas tree here too. > > > > > cpus_read_lock(); > > - max_queues_per_port = num_online_cpus(); > > - if (max_queues_per_port > MANA_MAX_NUM_QUEUES) > > - max_queues_per_port = MANA_MAX_NUM_QUEUES; > > + spin_lock_irqsave(&gc->irq_ctxs_lock, flags); > > + irqs = kmalloc_array(nvec, sizeof(int), GFP_KERNEL); > > + if (!irqs) { > > + err = -ENOMEM; > > + goto free_irq_vector; > > + } > > ...
Hi Simon, Thank you so much for all the comments, I will have them incorporated in the next version. :) Regards, Shradha.
