Message ID | 20081020191149.1dbf8e3b@extreme |
---|---|
State | Deferred, archived |
Delegated to: | Jeff Garzik |
Headers | show |
Stephen Hemminger wrote: > 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 <shemminger@vyatta.com> > > --- a/drivers/net/e1000e/e1000.h 2008-10-20 18:13:53.000000000 -0700 > +++ b/drivers/net/e1000e/e1000.h 2008-10-20 18:18:30.000000000 -0700 > @@ -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; > --- a/drivers/net/e1000e/netdev.c 2008-10-20 18:19:22.000000000 -0700 > +++ b/drivers/net/e1000e/netdev.c 2008-10-20 18:21:00.000000000 -0700 > @@ -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); 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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Jeff Garzik wrote: > >Stephen Hemminger wrote: >> 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 <shemminger@vyatta.com> [snip] > >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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 22 Oct 2008 12:01:16 -0600 "Williams, Mitch A" <mitch.a.williams@intel.com> wrote: > Jeff Garzik wrote: > > > > >Stephen Hemminger wrote: > >> 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 <shemminger@vyatta.com> > [snip] > > > >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 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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
--- a/drivers/net/e1000e/e1000.h 2008-10-20 18:13:53.000000000 -0700 +++ b/drivers/net/e1000e/e1000.h 2008-10-20 18:18:30.000000000 -0700 @@ -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; --- a/drivers/net/e1000e/netdev.c 2008-10-20 18:19:22.000000000 -0700 +++ b/drivers/net/e1000e/netdev.c 2008-10-20 18:21:00.000000000 -0700 @@ -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);
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 <shemminger@vyatta.com> -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html