Skip to contentSkip to conversation

e1000e: get rid of wasted space

PATCH In review Stephen Hemminger 4 participants 5 messages Oct 21, 2008 – Oct 22, 2008

Subsystems (2)
Networking Network drivers
lore
Share & tools
lore

Stephen Hemminger

author
#0 lore raw

PATCH 1/1 e1000e: get rid of wasted space

The name in the ring is only used once during setup so it shouldn't be in the data structure.

Signed-off-by Stephen Hemminger <[email protected]>
drivers/net/e1000e/e1000.h +0 -1
@@ -153,7 +153,6 @@ struct e1000_ring {
 	/* array of buffer information structs */
 	struct e1000_buffer *buffer_info;
 
-	char name[IFNAMSIZ + 5];
 	u32 ims_val;
 	u32 itr_val;
 	u16 itr_register;
drivers/net/e1000e/netdev.c +5 -12
@@ -1473,27 +1473,20 @@ static int e1000_request_msix(struct e10
 {
 	struct net_device *netdev = adapter->netdev;
 	int err = 0, vector = 0;
+	char irqname[IFNAMSIZ+5];
 
-	if (strlen(netdev->name) < (IFNAMSIZ - 5))
-		sprintf(adapter->rx_ring->name, "%s-rx0", netdev->name);
-	else
-		memcpy(adapter->rx_ring->name, netdev->name, IFNAMSIZ);
+	snprintf(irqname, sizeof(irqname), "%s-rx0", netdev->name);
 	err = request_irq(adapter->msix_entries[vector].vector,
-			  &e1000_intr_msix_rx, 0, adapter->rx_ring->name,
-			  netdev);
+			  &e1000_intr_msix_rx, 0, irqname, netdev);
 	if (err)
 		goto out;
 	adapter->rx_ring->itr_register = E1000_EITR_82574(vector);
 	adapter->rx_ring->itr_val = adapter->itr;
 	vector++;
 
-	if (strlen(netdev->name) < (IFNAMSIZ - 5))
-		sprintf(adapter->tx_ring->name, "%s-tx0", netdev->name);
-	else
-		memcpy(adapter->tx_ring->name, netdev->name, IFNAMSIZ);
+	snprintf(irqname, sizeof(irqname), "%s-tx0", netdev->name);
 	err = request_irq(adapter->msix_entries[vector].vector,
-			  &e1000_intr_msix_tx, 0, adapter->tx_ring->name,
-			  netdev);
+			  &e1000_intr_msix_tx, 0, irqname, netdev);
 	if (err)
 		goto out;
 	adapter->tx_ring->itr_register = E1000_EITR_82574(vector);
#1 lore raw
Quoted 2 lines · Stephen Hemminger

The name in the ring is only used once during setup so it shouldn't be in the data structure.

[...]

No. The name pointer passed to request_irq() has to remain valid until the IRQ is freed. Try looking in /proc/interrupts.

Ben.

Signature · 3 lines
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
#2 lore raw
In reply to Stephen Hemminger
Quoted 40 lines · Stephen Hemminger

The name in the ring is only used once during setup so it shouldn't be in the data structure.

Signed-off-by Stephen Hemminger <[email protected]>
drivers/net/e1000e/e1000.h +0 -1
@@ -153,7 +153,6 @@ struct e1000_ring {
 	/* array of buffer information structs */
 	struct e1000_buffer *buffer_info;
 
-	char name[IFNAMSIZ + 5];
 	u32 ims_val;
 	u32 itr_val;
 	u16 itr_register;
drivers/net/e1000e/netdev.c +3 -8
@@ -1473,27 +1473,20 @@ static int e1000_request_msix(struct e10
 {
 	struct net_device *netdev = adapter->netdev;
 	int err = 0, vector = 0;
+	char irqname[IFNAMSIZ+5];
 
-	if (strlen(netdev->name) < (IFNAMSIZ - 5))
-		sprintf(adapter->rx_ring->name, "%s-rx0", netdev->name);
-	else
-		memcpy(adapter->rx_ring->name, netdev->name, IFNAMSIZ);
+	snprintf(irqname, sizeof(irqname), "%s-rx0", netdev->name);
 	err = request_irq(adapter->msix_entries[vector].vector,
-			  &e1000_intr_msix_rx, 0, adapter->rx_ring->name,
-			  netdev);
+			  &e1000_intr_msix_rx, 0, irqname, netdev);
 	if (err)
 		goto out;
 	adapter->rx_ring->itr_register = E1000_EITR_82574(vector);
 	adapter->rx_ring->itr_val = adapter->itr;
 	vector++;
 
-	if (strlen(netdev->name) < (IFNAMSIZ - 5))
-		sprintf(adapter->tx_ring->name, "%s-tx0", netdev->name);

drivers/net/e1000e/netdev.c static int e1000_request_msix(struct e10 · line 1492

Quoted 10 lines
drivers/net/e1000e/netdev.c +2 -4
-	else
-		memcpy(adapter->tx_ring->name, netdev->name, IFNAMSIZ);
+	snprintf(irqname, sizeof(irqname), "%s-tx0", netdev->name);
 	err = request_irq(adapter->msix_entries[vector].vector,
-			  &e1000_intr_msix_tx, 0, adapter->tx_ring->name,
-			  netdev);
+			  &e1000_intr_msix_tx, 0, irqname, netdev);
 	if (err)
 		goto out;
 	adapter->tx_ring->itr_register = E1000_EITR_82574(vector);

ACK

for what it's worth... I am following DaveM's lead, and waiting until net-next opens to start taking patches for 2.6.29. He wants us to focus on bug fixing for now.

So, please resend once the merge window opens... thanks.

Jeff

#3 lore raw
Quoted 6 lines · Jeff Garzik
Quoted 4 lines · Stephen Hemminger

The name in the ring is only used once during setup so it shouldn't be in the data structure.

Signed-off-by Stephen Hemminger <[email protected]>

[snip]

Quoted 10 lines

ACK

for what it's worth... I am following DaveM's lead, and waiting until net-next opens to start taking patches for 2.6.29. He wants us to focus
on bug fixing for now.

So, please resend once the merge window opens... thanks.

NAK. Seriously. Don't resend this. NAK.

That string DOES get used later, like when you cat /proc/interrupts. So you can't allocate it on the stack. It's got to be persistent, just like we have it. Heck, look at the declaration for request_irq(). The devname param is declared as const char *.

So NAK. Same for igb and ixgbe. Don't do this.

-Mitch

-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge Build the coolest Linux based applications with Moblin SDK & win great prizes Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/

author
#4 lore raw
Quoted 22 lines · Mitch A
Quoted 6 lines · Jeff Garzik
Quoted 4 lines · Stephen Hemminger

The name in the ring is only used once during setup so it shouldn't be in the data structure.

Signed-off-by Stephen Hemminger <[email protected]>

[snip]

Quoted 10 lines

ACK

for what it's worth... I am following DaveM's lead, and waiting until net-next opens to start taking patches for 2.6.29. He wants us to focus
on bug fixing for now.

So, please resend once the merge window opens... thanks.

NAK. Seriously. Don't resend this. NAK.

Quoted 8 lines

That string DOES get used later, like when you cat /proc/interrupts. So you can't allocate it on the stack. It's got to be persistent, just like we have it. Heck, look at the declaration for request_irq(). The devname param is declared as const char *.

So NAK. Same for igb and ixgbe. Don't do this.

-Mitch

The comments were right, this needs to stay, although it might be better to use a bigger field to allow full width of IFNAME+5