netdev - Re: [PATCH v2 10/15] net: rnpgbe: Add netdev irq in open

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: <2C80B81E5B7F54CD+20250723061303.GE169181@nic-Precision-5820-Tower>
Date: Wed, 23 Jul 2025 14:13:03 +0800
From: Yibo Dong <dong100@...se.com>
To: Simon Horman <horms@...nel.org>
Cc: andrew+netdev@...n.ch, davem@...emloft.net, edumazet@...gle.com,
	kuba@...nel.org, pabeni@...hat.com, corbet@....net,
	gur.stavi@...wei.com, maddy@...ux.ibm.com, mpe@...erman.id.au,
	danishanwar@...com, lee@...ger.us, gongfan1@...wei.com,
	lorenzo@...nel.org, geert+renesas@...der.be,
	Parthiban.Veerasooran@...rochip.com, lukas.bulwahn@...hat.com,
	alexanderduyck@...com, richardcochran@...il.com,
	netdev@...r.kernel.org, linux-doc@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 10/15] net: rnpgbe: Add netdev irq in open

On Tue, Jul 22, 2025 at 03:03:26PM +0100, Simon Horman wrote:
> On Mon, Jul 21, 2025 at 07:32:33PM +0800, Dong Yibo wrote:
> > Initialize irq for tx/rx in open func.
> > 
> > Signed-off-by: Dong Yibo <dong100@...se.com>
> 
> ...
> 
> > diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c
> 
> ...
> 
> > +/**
> > + * rnpgbe_set_mac_hw_ops_n500 - Setup mac address to hw
> > + * @hw: pointer to hw structure
> > + * @mac: pointer to mac addr
> > + *
> > + * Setup a mac address to hw.
> > + **/
> > +static void rnpgbe_set_mac_hw_ops_n500(struct mucse_hw *hw, u8 *mac)
> > +{
> > +	struct mucse_eth_info *eth = &hw->eth;
> > +	struct mucse_mac_info *mac_info = &hw->mac;
> 
> Reverse xmas tree here please.
> 

Got it, I'll fix it.

> > +
> > +	/* use idx 0 */
> > +	eth->ops.set_rar(eth, 0, mac);
> > +	mac_info->ops.set_mac(mac_info, mac, 0);
> > +}
> 
> ...
> 
> > diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_lib.c b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_lib.c
> 
> ...
> 
> > +/**
> > + * rnpgbe_msix_clean_rings - msix irq handler for ring irq
> > + * @irq: irq num
> > + * @data: private data
> > + *
> > + * rnpgbe_msix_clean_rings handle irq from ring, start napi
> 
> Please also document the return value of this function.
> Likewise for rnpgbe_request_msix_irqs(), rnpgbe_intr() and
> rnpgbe_request_irq().
> 

Got it, I'll fix it.

> > + **/
> > +static irqreturn_t rnpgbe_msix_clean_rings(int irq, void *data)
> > +{
> > +	return IRQ_HANDLED;
> > +}
> 
> ...
> 
> > +/**
> > + * rnpgbe_request_msix_irqs - Initialize MSI-X interrupts
> > + * @mucse: pointer to private structure
> > + *
> > + * rnpgbe_request_msix_irqs allocates MSI-X vectors and requests
> > + * interrupts from the kernel.
> > + **/
> > +static int rnpgbe_request_msix_irqs(struct mucse *mucse)
> > +{
> > +	struct net_device *netdev = mucse->netdev;
> > +	int q_off = mucse->q_vector_off;
> > +	struct msix_entry *entry;
> > +	int i = 0;
> > +	int err;
> > +
> > +	for (i = 0; i < mucse->num_q_vectors; i++) {
> > +		struct mucse_q_vector *q_vector = mucse->q_vector[i];
> > +
> > +		entry = &mucse->msix_entries[i + q_off];
> > +		if (q_vector->tx.ring && q_vector->rx.ring) {
> > +			snprintf(q_vector->name, sizeof(q_vector->name) - 1,
> > +				 "%s-%s-%d", netdev->name, "TxRx", i);
> 
> Probably the full range of i is not used, in particular I assume
> it is never negative, and thus i and mucse->num_q_vectors
> could be unsigned int rather than int.
> 
> But as it stands q_vector->name is once character too short to
> fit the maximum possible string formatted by snprintf().
> 
> I was able to address the warning flagged by GCC 15.0.0 about this by
> increasing the size of q_vector->name by one byte.
> 
>   .../rnpgbe_lib.c: In function 'rnpgbe_request_irq':
>   .../rnpgbe_lib.c:1015:43: warning: 'snprintf' output may be truncated before the last format character [-Wformat-truncation=]
>    1015 |                                  "%s-%s-%d", netdev->name, "TxRx", i);
>         |                                           ^
>   In function 'rnpgbe_request_msix_irqs',
>       inlined from 'rnpgbe_request_irq' at drivers/net/ethernet/mucse/rnpgbe/rnpgbe_lib.c:1069:9:
>   .../rnpgbe_lib.c:1014:25: note: 'snprintf' output between 8 and 33 bytes into a destination of size 32
>    1014 |                         snprintf(q_vector->name, sizeof(q_vector->name) - 1,
>         |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>    1015 |                                  "%s-%s-%d", netdev->name, "TxRx", i);
>         |                                  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> 

Yes, q_vector->name is too short, I'll fix it.

> > +		} else {
> > +			/* skip this unused q_vector */
> > +			continue;
> > +		}
> > +		err = request_irq(entry->vector, &rnpgbe_msix_clean_rings, 0,
> > +				  q_vector->name, q_vector);
> > +		if (err)
> > +			goto free_queue_irqs;
> > +		/* register for affinity change notifications */
> > +		q_vector->affinity_notify.notify = rnpgbe_irq_affinity_notify;
> > +		q_vector->affinity_notify.release = rnpgbe_irq_affinity_release;
> > +		irq_set_affinity_notifier(entry->vector,
> > +					  &q_vector->affinity_notify);
> > +		irq_set_affinity_hint(entry->vector, &q_vector->affinity_mask);
> > +	}
> > +
> > +	return 0;
> > +
> > +free_queue_irqs:
> > +	while (i) {
> > +		i--;
> > +		entry = &mucse->msix_entries[i + q_off];
> > +		irq_set_affinity_hint(entry->vector, NULL);
> > +		free_irq(entry->vector, mucse->q_vector[i]);
> > +		irq_set_affinity_notifier(entry->vector, NULL);
> > +		irq_set_affinity_hint(entry->vector, NULL);
> > +	}
> > +	return err;
> > +}
> 
> ...
> 

Thanks for your feedback.