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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20250424165743.GJ3042781@horms.kernel.org>
Date: Thu, 24 Apr 2025 17:57:43 +0100
From: Simon Horman <horms@...nel.org>
To: Shradha Gupta <shradhagupta@...ux.microsoft.com>
Cc: linux-hyperv@...r.kernel.org, linux-pci@...r.kernel.org,
	linux-kernel@...r.kernel.org, Nipun Gupta <nipun.gupta@....com>,
	Yury Norov <yury.norov@...il.com>, Jason Gunthorpe <jgg@...pe.ca>,
	Jonathan Cameron <Jonathan.Cameron@...ei.com>,
	Anna-Maria Behnsen <anna-maria@...utronix.de>,
	Shivamurthy Shastri <shivamurthy.shastri@...utronix.de>,
	Kevin Tian <kevin.tian@...el.com>, Long Li <longli@...rosoft.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	Bjorn Helgaas <bhelgaas@...gle.com>, Rob Herring <robh@...nel.org>,
	Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>,
	Krzysztof Wilczyński <kw@...ux.com>,
	Lorenzo Pieralisi <lpieralisi@...nel.org>,
	Dexuan Cui <decui@...rosoft.com>, Wei Liu <wei.liu@...nel.org>,
	Haiyang Zhang <haiyangz@...rosoft.com>,
	"K. Y. Srinivasan" <kys@...rosoft.com>,
	Andrew Lunn <andrew+netdev@...n.ch>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Konstantin Taranov <kotaranov@...rosoft.com>,
	Leon Romanovsky <leon@...nel.org>,
	Maxim Levitsky <mlevitsk@...hat.com>,
	Erni Sri Satya Vennela <ernis@...ux.microsoft.com>,
	Peter Zijlstra <peterz@...radead.org>, netdev@...r.kernel.org,
	linux-rdma@...r.kernel.org, Paul Rosswurm <paulros@...rosoft.com>,
	Shradha Gupta <shradhagupta@...rosoft.com>
Subject: Re: [PATCH 2/2] net: mana: Allow MANA driver to allocate PCI vector
 dynamically

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@...ux.microsoft.com>
> Reviewed-by: Haiyang Zhang <haiyangz@...rosoft.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;
> +	}

...