[RFC 0/7] network device irq naming

[RFC 0/7] network device irq naming

[Stephen Hemminger]

This is a proposal for a wrapper routine to cause
network devices to have standard convention for IRQ naming.

Includes a subset of devices to show how it would be used
and some of the existing problems.

[RFC 1/7] netdev: add standardized irq naming function

[Stephen Hemminger]

[-- Attachment #1: netdev-irq-name.patch --]
[-- Type: text/plain, Size: 1561 bytes --]

To force driver developers to use a standard convention for naming
network device IRQ's, provide a standardized method for creating
the name.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>


--- a/include/linux/netdevice.h	2011-06-21 08:58:32.207953328 -0700
+++ b/include/linux/netdevice.h	2011-06-21 09:12:12.155952869 -0700
@@ -2631,6 +2631,46 @@ static inline const char *netdev_name(co
 	return dev->name;
 }
 
+/* function bits for netdev_irqname */
+#define NETIF_IRQ_TX	1
+#define NETIF_IRQ_RX	2
+#define NETIF_IRQ_TXRX	3
+#define NETIF_IRQ_OTHER 0	/* none of the above */
+
+/**
+ *	netdev_irqname - generate name for irq
+ *	@buf: space to store result
+ *	@buflen: sizeof buf
+ *	@dev: network device
+ *	@queue: assoctiated network queue
+ *	@function: function of irq
+ *
+ * Format a IRQ name according to standard convention to be passed
+ * to request_irq().
+ */
+static inline const char *netdev_irqname(char *buf, size_t buflen,
+					 const struct net_device *dev,
+					 unsigned queue,
+					 unsigned function)
+{
+	switch (function) {
+	case NETIF_IRQ_TX:
+		snprintf(buf, buflen, "%s-tx-%u", dev->name, queue);
+		break;
+	case NETIF_IRQ_RX:
+		snprintf(buf, buflen, "%s-rx-%u", dev->name, queue);
+		break;
+	case NETIF_IRQ_TXRX:
+		snprintf(buf, buflen, "%s-%u", dev->name, queue);
+		break;
+	default:
+		snprintf(buf, buflen, "%s", dev->name);
+	}
+
+	return buf;
+}
+
+
 extern int netdev_printk(const char *level, const struct net_device *dev,
 			 const char *format, ...)
 	__attribute__ ((format (printf, 3, 4)));

[RFC 2/7] igb: use netdev_irqname

[Stephen Hemminger]

[-- Attachment #1: igb-use-irq-name.patch --]
[-- Type: text/plain, Size: 1563 bytes --]

This is an example of usage of netdev_irqname to create standard
IRQ names.  There is a change of behavior, the driver will now skip
unused IRQ vectors (similar to ixgbe).

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>


--- a/drivers/net/igb/igb_main.c	2011-06-21 09:12:49.567952849 -0700
+++ b/drivers/net/igb/igb_main.c	2011-06-21 09:29:48.211952277 -0700
@@ -920,16 +920,20 @@ static int igb_request_msix(struct igb_a
 		q_vector->itr_register = hw->hw_addr + E1000_EITR(vector);
 
 		if (q_vector->rx_ring && q_vector->tx_ring)
-			sprintf(q_vector->name, "%s-TxRx-%u", netdev->name,
-			        q_vector->rx_ring->queue_index);
+			netdev_irqname(q_vector->name, sizeof(q_vector->name),
+				       netdev,  NETIF_IRQ_TXRX,
+				       q_vector->rx_ring->queue_index);
 		else if (q_vector->tx_ring)
-			sprintf(q_vector->name, "%s-tx-%u", netdev->name,
-			        q_vector->tx_ring->queue_index);
+			netdev_irqname(q_vector->name, sizeof(q_vector->name),
+				       netdev,  NETIF_IRQ_TX,
+				       q_vector->tx_ring->queue_index);
 		else if (q_vector->rx_ring)
-			sprintf(q_vector->name, "%s-rx-%u", netdev->name,
-			        q_vector->rx_ring->queue_index);
-		else
-			sprintf(q_vector->name, "%s-unused", netdev->name);
+			netdev_irqname(q_vector->name, sizeof(q_vector->name),
+				       netdev,  NETIF_IRQ_RX,
+				       q_vector->rx_ring->queue_index);
+		else 	/* skip this unused q_vector */
+			continue;
+
 
 		err = request_irq(adapter->msix_entries[vector].vector,
 		                  igb_msix_ring, 0, q_vector->name,

[RFC 3/7] ixgbe: use netdev_irqname

[Stephen Hemminger]

[-- Attachment #1: ixgbe-use-irq-name.patch --]
[-- Type: text/plain, Size: 1563 bytes --]

New standard function for generating irq names.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>


--- a/drivers/net/ixgbe/ixgbe_main.c	2011-06-21 09:30:35.327952251 -0700
+++ b/drivers/net/ixgbe/ixgbe_main.c	2011-06-21 09:32:33.483952184 -0700
@@ -2352,20 +2352,19 @@ static int ixgbe_request_msix_irqs(struc
 		struct ixgbe_q_vector *q_vector = adapter->q_vector[vector];
 		handler = SET_HANDLER(q_vector);
 
-		if (handler == &ixgbe_msix_clean_rx) {
-			snprintf(q_vector->name, sizeof(q_vector->name) - 1,
-			         "%s-%s-%d", netdev->name, "rx", ri++);
-		} else if (handler == &ixgbe_msix_clean_tx) {
-			snprintf(q_vector->name, sizeof(q_vector->name) - 1,
-			         "%s-%s-%d", netdev->name, "tx", ti++);
-		} else if (handler == &ixgbe_msix_clean_many) {
-			snprintf(q_vector->name, sizeof(q_vector->name) - 1,
-			         "%s-%s-%d", netdev->name, "TxRx", ri++);
+		if (handler == &ixgbe_msix_clean_rx)
+			netdev_irqname(q_vector->name, sizeof(q_vector->name),
+				       netdev, NETIF_IRQ_RX, ri++);
+		else if (handler == &ixgbe_msix_clean_tx)
+			netdev_irqname(q_vector->name, sizeof(q_vector->name),
+				       netdev, NETIF_IRQ_TX, ti++);
+		else if (handler == &ixgbe_msix_clean_many) {
+			netdev_irqname(q_vector->name, sizeof(q_vector->name),
+				       netdev, NETIF_IRQ_TXRX, ri++);
 			ti++;
-		} else {
-			/* skip this unused q_vector */
+		} else /* skip this unused q_vector */
 			continue;
-		}
+
 		err = request_irq(adapter->msix_entries[vector].vector,
 				  handler, 0, q_vector->name,
 				  q_vector);

[RFC 4/7] benet: use irq naming standard

[Stephen Hemminger]

[-- Attachment #1: benet-use-irqname.patch --]
[-- Type: text/plain, Size: 1687 bytes --]

Use the standard for network device IRQ nameing for multiqueue devices.
It appears, this device has one transmit interrup, but multiple receive
interrupts. They will now be named:
  ethX-tx-0 ethX-rx-0 ethX-rx-1 ...

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>


--- a/drivers/net/benet/be_main.c	2011-06-21 09:34:01.407952135 -0700
+++ b/drivers/net/benet/be_main.c	2011-06-21 09:41:03.643951899 -0700
@@ -2196,13 +2196,16 @@ static inline int be_msix_vec_get(struct
 }
 
 static int be_request_irq(struct be_adapter *adapter,
-		struct be_eq_obj *eq_obj,
-		void *handler, char *desc, void *context)
+			  struct be_eq_obj *eq_obj,
+			  unsigned int usage, unsigned int queue,
+			  irq_handler_t handler, void *context)
 {
 	struct net_device *netdev = adapter->netdev;
 	int vec;
 
-	sprintf(eq_obj->desc, "%s-%s", netdev->name, desc);
+	netdev_irqname(eq_obj->desc, sizeof(eq_obj->desc),
+		       netdev, usage, queue);
+
 	vec = be_msix_vec_get(adapter, eq_obj);
 	return request_irq(vec, handler, 0, eq_obj->desc, context);
 }
@@ -2218,17 +2221,17 @@ static int be_msix_register(struct be_ad
 {
 	struct be_rx_obj *rxo;
 	int status, i;
-	char qname[10];
 
-	status = be_request_irq(adapter, &adapter->tx_eq, be_msix_tx_mcc, "tx",
-				adapter);
+	status = be_request_irq(adapter, &adapter->tx_eq,
+				NETIF_IRQ_TX, 0,
+				be_msix_tx_mcc, adapter);
 	if (status)
 		goto err;
 
 	for_all_rx_queues(adapter, rxo, i) {
-		sprintf(qname, "rxq%d", i);
-		status = be_request_irq(adapter, &rxo->rx_eq, be_msix_rx,
-				qname, rxo);
+		status = be_request_irq(adapter, &rxo->rx_eq,
+					NETIF_IRQ_RX, i,
+					be_msix_rx, rxo);
 		if (status)
 			goto err_msix;
 	}

[RFC 5/7] bnx2: use netdev_irqname

[Stephen Hemminger]

[-- Attachment #1: bnx2-use-irqname.patch --]
[-- Type: text/plain, Size: 1006 bytes --]

Also increase size of irq name to account for longer device names.
Original code was broken for full size names.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>


--- a/drivers/net/bnx2.c	2011-06-21 09:52:00.527951530 -0700
+++ b/drivers/net/bnx2.c	2011-06-21 09:52:58.807951497 -0700
@@ -6173,7 +6173,8 @@ bnx2_enable_msix(struct bnx2 *bp, int ms
 	bp->flags |= BNX2_FLAG_USING_MSIX | BNX2_FLAG_ONE_SHOT_MSI;
 	for (i = 0; i < total_vecs; i++) {
 		bp->irq_tbl[i].vector = msix_ent[i].vector;
-		snprintf(bp->irq_tbl[i].name, len, "%s-%d", dev->name, i);
+		netdev_irqname(bp->irq_tbl[i].name, len,
+			       dev, NETIF_IRQ_TXRX, i);
 		bp->irq_tbl[i].handler = bnx2_msi_1shot;
 	}
 }
--- a/drivers/net/bnx2.h	2011-06-21 09:53:18.331951487 -0700
+++ b/drivers/net/bnx2.h	2011-06-21 09:53:51.723951469 -0700
@@ -6657,7 +6657,7 @@ struct bnx2_irq {
 	irq_handler_t	handler;
 	unsigned int	vector;
 	u8		requested;
-	char		name[IFNAMSIZ + 2];
+	char		name[32];
 };
 
 struct bnx2_tx_ring_info {

[RFC 6/7] niu: use netdev_irqname

[Stephen Hemminger]

[-- Attachment #1: niu-use-irqname.patch --]
[-- Type: text/plain, Size: 841 bytes --]

This device also has some other IRQ's unrelated to rings
which are named with another convention.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>


--- a/drivers/net/niu.c	2011-06-21 09:57:00.739951363 -0700
+++ b/drivers/net/niu.c	2011-06-21 09:59:22.683951282 -0700
@@ -6048,11 +6048,12 @@ static void niu_set_irq_name(struct niu
 
 	for (i = 0; i < np->num_ldg - j; i++) {
 		if (i < np->num_rx_rings)
-			sprintf(np->irq_name[i+j], "%s-rx-%d",
-				np->dev->name, i);
+			netdev_irqname(np->irq_name[i+j], IFNAMSIZ+6,
+				       np->dev, NETIF_IRQ_RX, i);
 		else if (i < np->num_tx_rings + np->num_rx_rings)
-			sprintf(np->irq_name[i+j], "%s-tx-%d", np->dev->name,
-				i - np->num_rx_rings);
+			netdev_irqname(np->irq_name[i+j], IFNAMSIZ+6,
+				       np->dev, NETIF_IRQ_TX,
+				       i - np->num_rx_rings);
 	}
 }
 

[RFC 7/7] netxen: use netdev_irqname

[Stephen Hemminger]

[-- Attachment #1: netxen-use-irqname.patch --]
[-- Type: text/plain, Size: 763 bytes --]

Use the new netdev_irqname to cause network device interrupts to
be named according to the standard convention of "ethX-N".

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>


--- a/drivers/net/netxen/netxen_nic_main.c	2011-06-21 10:00:10.223951255 -0700
+++ b/drivers/net/netxen/netxen_nic_main.c	2011-06-21 10:02:46.695951169 -0700
@@ -955,7 +955,8 @@ netxen_nic_request_irq(struct netxen_ada
 
 	for (ring = 0; ring < adapter->max_sds_rings; ring++) {
 		sds_ring = &recv_ctx->sds_rings[ring];
-		sprintf(sds_ring->name, "%s[%d]", netdev->name, ring);
+		netdev_irqname(sds_ring->name, sizeof(sds_ring->name),
+			       netdev, NETIF_IRQ_TXRX, ring);
 		err = request_irq(sds_ring->irq, handler,
 				  flags, sds_ring->name, sds_ring);
 		if (err)

Re: [RFC 1/7] netdev: add standardized irq naming function

[Michał Mirosław]

2011/6/21 Stephen Hemminger <shemminger@vyatta.com>:
> To force driver developers to use a standard convention for naming
> network device IRQ's, provide a standardized method for creating
> the name.

Can this be modified to track netdev renames?

Best Regards,
Michał Mirosław

Re: [RFC 1/7] netdev: add standardized irq naming function

[Ben Hutchings]

On Tue, 2011-06-21 at 19:30 +0200, Michał Mirosław wrote:
> 2011/6/21 Stephen Hemminger <shemminger@vyatta.com>:
> > To force driver developers to use a standard convention for naming
> > network device IRQ's, provide a standardized method for creating
> > the name.
> 
> Can this be modified to track netdev renames?

We should handle renames somehow.

sfc currently uses a netdev notifier to update the names of its IRQs
(and MTDs) but it might make more sense to add something to
net_device_ops rather than having every driver receive notification for
every device.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [RFC 1/7] netdev: add standardized irq naming function

[Stephen Hemminger]

On Tue, 21 Jun 2011 18:48:55 +0100
Ben Hutchings <bhutchings@solarflare.com> wrote:

> On Tue, 2011-06-21 at 19:30 +0200, Michał Mirosław wrote:
> > 2011/6/21 Stephen Hemminger <shemminger@vyatta.com>:
> > > To force driver developers to use a standard convention for naming
> > > network device IRQ's, provide a standardized method for creating
> > > the name.
> > 
> > Can this be modified to track netdev renames?
> 
> We should handle renames somehow.
> 
> sfc currently uses a netdev notifier to update the names of its IRQs
> (and MTDs) but it might make more sense to add something to
> net_device_ops rather than having every driver receive notification for
> every device.
> 
> Ben.
> 

Since renames are only allowed when netdevice is down.
Renames do not need to be tracked if device registers irq in open
handler.

Re: [RFC 1/7] netdev: add standardized irq naming function

[Ben Hutchings]

On Tue, 2011-06-21 at 10:05 -0700, Stephen Hemminger wrote:

> To force driver developers to use a standard convention for naming
> network device IRQ's, provide a standardized method for creating
> the name.
> 
> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
> 
> 
> --- a/include/linux/netdevice.h	2011-06-21 08:58:32.207953328 -0700
> +++ b/include/linux/netdevice.h	2011-06-21 09:12:12.155952869 -0700
> @@ -2631,6 +2631,46 @@ static inline const char *netdev_name(co
>  	return dev->name;
>  }
>  
> +/* function bits for netdev_irqname */
> +#define NETIF_IRQ_TX	1
> +#define NETIF_IRQ_RX	2
> +#define NETIF_IRQ_TXRX	3
> +#define NETIF_IRQ_OTHER 0	/* none of the above */

There can be multiple 'other' IRQs in which case they will need their
own suffixes.

Also: 'netdev' and 'NETIF'?  We are terribly inconsistent about this but
we could at least make this single change self-consistent. :-)

> +/**
> + *	netdev_irqname - generate name for irq
> + *	@buf: space to store result
> + *	@buflen: sizeof buf
> + *	@dev: network device
> + *	@queue: assoctiated network queue

Queue index.

> + *	@function: function of irq
> + *
> + * Format a IRQ name according to standard convention to be passed
> + * to request_irq().
> + */
> +static inline const char *netdev_irqname(char *buf, size_t buflen,
> +					 const struct net_device *dev,
> +					 unsigned queue,
> +					 unsigned function)

Might be worth making this a little more generic as storage devices are
also going multiqueue in a similar way.

> +{
> +	switch (function) {
> +	case NETIF_IRQ_TX:
> +		snprintf(buf, buflen, "%s-tx-%u", dev->name, queue);
> +		break;
> +	case NETIF_IRQ_RX:
> +		snprintf(buf, buflen, "%s-rx-%u", dev->name, queue);
> +		break;
> +	case NETIF_IRQ_TXRX:
> +		snprintf(buf, buflen, "%s-%u", dev->name, queue);
> +		break;
> +	default:
> +		snprintf(buf, buflen, "%s", dev->name);
> +	}
> +
> +	return buf;
> +}

So perhaps something like:

/* kernel/irq/manage.c */

const char *irq_name(char *buf, size_t buflen, const char *base_name,
		     int index, const char *type)
{
	if (type) {
		if (index >= 0)
			snprintf(buf, buflen, "%s-%s-%d", base_name, type, index);
		else
			snprintf(buf, buflen, "%s-%s", base_name, type);
	} else {
		if (index >= 0)
			snprintf(buf, buflen, "%s-%d", base_name, index);
		else
			strlcpy(buf, base_name, buflen);
	}
	return buf;
}

/* include/linux/interrupt.h */

#define IRQ_NAME_NO_INDEX (-1)

/* include/linux/netdevice.h */

/* IRQ type name for netdev_irqname */
#define NETDEV_IRQ_TYPE_TX   "tx"
#define NETDEV_IRQ_TYPE_RX   "rx"
#define NETDEV_IRQ_TYPE_TXRX NULL

/**
 *     netdev_irqname - generate name for irq
 *     @buf: space to store result
 *     @buflen: sizeof buf
 *     @dev: network device
 *     @index: network queue index
 *     @type: type of IRQ
 *
 * Format a IRQ name according to standard convention to be passed
 * to request_irq().
 */
static inline const char *netdev_irqname(char *buf, size_t buflen,
                                        const struct net_device *dev,
                                        int index, const char *type)
{
	return irq_name(buf, buflen, dev->name, index, type);
}

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [RFC 5/7] bnx2: use netdev_irqname

[Ben Hutchings]

On Tue, 2011-06-21 at 10:05 -0700, Stephen Hemminger wrote:

> Also increase size of irq name to account for longer device names.
> Original code was broken for full size names.
[...]

We should define a macro for the maximum netdev IRQ name length.  I
think this would be IFNAMSIZ + 1 + 2 + 1 + 5 (2 characters for queue
type; up to 5 digits for 16-bit queue index).

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [RFC 5/7] bnx2: use netdev_irqname

[Stephen Hemminger]

On Tue, 21 Jun 2011 19:11:51 +0100
Ben Hutchings <bhutchings@solarflare.com> wrote:

> On Tue, 2011-06-21 at 10:05 -0700, Stephen Hemminger wrote:
> 
> > Also increase size of irq name to account for longer device names.
> > Original code was broken for full size names.
> [...]
> 
> We should define a macro for the maximum netdev IRQ name length.  I
> think this would be IFNAMSIZ + 1 + 2 + 1 + 5 (2 characters for queue
> type; up to 5 digits for 16-bit queue index).
> 
> Ben.
> 

Probably should put it in the queue structures?

Re: [RFC 5/7] bnx2: use netdev_irqname

[Ben Hutchings]

On Tue, 2011-06-21 at 11:17 -0700, Stephen Hemminger wrote:
> On Tue, 21 Jun 2011 19:11:51 +0100
> Ben Hutchings <bhutchings@solarflare.com> wrote:
> 
> > On Tue, 2011-06-21 at 10:05 -0700, Stephen Hemminger wrote:
> > 
> > > Also increase size of irq name to account for longer device names.
> > > Original code was broken for full size names.
> > [...]
> > 
> > We should define a macro for the maximum netdev IRQ name length.  I
> > think this would be IFNAMSIZ + 1 + 2 + 1 + 5 (2 characters for queue
> > type; up to 5 digits for 16-bit queue index).
> > 
> > Ben.
> > 
> 
> Probably should put it in the queue structures?

Not really, given an IRQ can be related to multiple queues.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.