Re: [PATCH 2/2] net: mana: Allow MANA driver to allocate PCI vector dynamically

Shradha Gupta
2025-04-25T07:29:20Z

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 <shradhagupta@linux.microsoft.com>
> > Reviewed-by: Haiyang Zhang <haiyangz@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.