* Re: gianfar: reallocate skb when headroom is not enough for fcb @ 2009-03-26 0:21 David Miller 2009-03-26 17:08 ` [PATCH] gianfar: fix headroom expansion code Stephen Hemminger 0 siblings, 1 reply; 9+ messages in thread From: David Miller @ 2009-03-26 0:21 UTC (permalink / raw) To: leoli; +Cc: netdev Applied, thanks! ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] gianfar: fix headroom expansion code 2009-03-26 0:21 gianfar: reallocate skb when headroom is not enough for fcb David Miller @ 2009-03-26 17:08 ` Stephen Hemminger 2009-03-27 4:26 ` Li Yang 2009-03-27 7:38 ` [PATCH] gianfar: fix headroom expansion code David Miller 0 siblings, 2 replies; 9+ messages in thread From: Stephen Hemminger @ 2009-03-26 17:08 UTC (permalink / raw) To: David Miller; +Cc: leoli, netdev The code that was added to increase headroom was wrong. It doesn't handle the case where gfar_add_fcb() changes the skb. Better to do check at start of transmit (outside of lock), where error handling is better anyway. Signed-off-by: Stephen Hemminger <shemminger@vyatta.com> --- a/drivers/net/gianfar.c 2009-03-26 09:14:39.273669929 -0700 +++ b/drivers/net/gianfar.c 2009-03-26 09:22:46.477545004 -0700 @@ -1239,19 +1239,9 @@ static int gfar_enet_open(struct net_dev return err; } -static inline struct txfcb *gfar_add_fcb(struct sk_buff **skbp) +static inline struct txfcb *gfar_add_fcb(struct sk_buff *skb) { - struct txfcb *fcb; - struct sk_buff *skb = *skbp; - - if (unlikely(skb_headroom(skb) < GMAC_FCB_LEN)) { - struct sk_buff *old_skb = skb; - skb = skb_realloc_headroom(old_skb, GMAC_FCB_LEN); - if (!skb) - return NULL; - dev_kfree_skb_any(old_skb); - } - fcb = (struct txfcb *)skb_push(skb, GMAC_FCB_LEN); + struct txfcb *fcb = (struct txfcb *)skb_push(skb, GMAC_FCB_LEN); cacheable_memzero(fcb, GMAC_FCB_LEN); return fcb; @@ -1320,6 +1310,20 @@ static int gfar_start_xmit(struct sk_buf base = priv->tx_bd_base; + /* make space for additional header */ + if (skb_headroom(skb) < GMAC_FCB_LEN) { + struct sk_buff *skb_new; + + skb_new = skb_realloc_headroom(skb, GMAC_FCB_LEN); + if (!skb_new) { + dev->stats.tx_errors++; + kfree(skb); + return NETDEV_TX_OK; + } + kfree_skb(skb); + skb = skb_new; + } + /* total number of fragments in the SKB */ nr_frags = skb_shinfo(skb)->nr_frags; @@ -1372,20 +1376,18 @@ static int gfar_start_xmit(struct sk_buf /* Set up checksumming */ if (CHECKSUM_PARTIAL == skb->ip_summed) { - fcb = gfar_add_fcb(&skb); - if (likely(fcb != NULL)) { - lstatus |= BD_LFLAG(TXBD_TOE); - gfar_tx_checksum(skb, fcb); - } + fcb = gfar_add_fcb(skb); + lstatus |= BD_LFLAG(TXBD_TOE); + gfar_tx_checksum(skb, fcb); } if (priv->vlgrp && vlan_tx_tag_present(skb)) { - if (unlikely(NULL == fcb)) - fcb = gfar_add_fcb(&skb); - if (likely(fcb != NULL)) { + if (unlikely(NULL == fcb)) { + fcb = gfar_add_fcb(skb); lstatus |= BD_LFLAG(TXBD_TOE); - gfar_tx_vlan(skb, fcb); } + + gfar_tx_vlan(skb, fcb); } /* setup the TxBD length and buffer pointer for the first BD */ @@ -1433,7 +1435,7 @@ static int gfar_start_xmit(struct sk_buf /* Unlock priv */ spin_unlock_irqrestore(&priv->txlock, flags); - return 0; + return NETDEV_TX_OK; } /* Stops the kernel queue, and halts the controller */ ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] gianfar: fix headroom expansion code 2009-03-26 17:08 ` [PATCH] gianfar: fix headroom expansion code Stephen Hemminger @ 2009-03-27 4:26 ` Li Yang 2009-03-27 7:39 ` David Miller 2009-03-27 8:01 ` [PATCH] gianfar: only check headroom when FCB is needed Li Yang 2009-03-27 7:38 ` [PATCH] gianfar: fix headroom expansion code David Miller 1 sibling, 2 replies; 9+ messages in thread From: Li Yang @ 2009-03-27 4:26 UTC (permalink / raw) To: Stephen Hemminger; +Cc: David Miller, netdev On Fri, Mar 27, 2009 at 1:08 AM, Stephen Hemminger <shemminger@vyatta.com> wrote: > The code that was added to increase headroom was wrong. > It doesn't handle the case where gfar_add_fcb() changes the skb. Oops. I messed up the pointer to pointer manipulating. > Better to do check at start of transmit (outside of lock), where > error handling is better anyway. > > Signed-off-by: Stephen Hemminger <shemminger@vyatta.com> > > > --- a/drivers/net/gianfar.c 2009-03-26 09:14:39.273669929 -0700 > +++ b/drivers/net/gianfar.c 2009-03-26 09:22:46.477545004 -0700 > @@ -1239,19 +1239,9 @@ static int gfar_enet_open(struct net_dev > return err; > } > > -static inline struct txfcb *gfar_add_fcb(struct sk_buff **skbp) > +static inline struct txfcb *gfar_add_fcb(struct sk_buff *skb) > { > - struct txfcb *fcb; > - struct sk_buff *skb = *skbp; > - > - if (unlikely(skb_headroom(skb) < GMAC_FCB_LEN)) { > - struct sk_buff *old_skb = skb; > - skb = skb_realloc_headroom(old_skb, GMAC_FCB_LEN); > - if (!skb) > - return NULL; > - dev_kfree_skb_any(old_skb); > - } > - fcb = (struct txfcb *)skb_push(skb, GMAC_FCB_LEN); > + struct txfcb *fcb = (struct txfcb *)skb_push(skb, GMAC_FCB_LEN); > cacheable_memzero(fcb, GMAC_FCB_LEN); > > return fcb; > @@ -1320,6 +1310,20 @@ static int gfar_start_xmit(struct sk_buf > > base = priv->tx_bd_base; > > + /* make space for additional header */ > + if (skb_headroom(skb) < GMAC_FCB_LEN) { > + struct sk_buff *skb_new; > + > + skb_new = skb_realloc_headroom(skb, GMAC_FCB_LEN); > + if (!skb_new) { > + dev->stats.tx_errors++; > + kfree(skb); > + return NETDEV_TX_OK; > + } > + kfree_skb(skb); > + skb = skb_new; > + } > + We have legacy devices without the offloading feature. And we can even turn off the IP checksum offloading at runtime. Your code will cause unnecessary realloc for these cases. I can propose a new patch to fix the pointer problem and add more error handling. > /* total number of fragments in the SKB */ > nr_frags = skb_shinfo(skb)->nr_frags; > > @@ -1372,20 +1376,18 @@ static int gfar_start_xmit(struct sk_buf > > /* Set up checksumming */ > if (CHECKSUM_PARTIAL == skb->ip_summed) { > - fcb = gfar_add_fcb(&skb); > - if (likely(fcb != NULL)) { > - lstatus |= BD_LFLAG(TXBD_TOE); > - gfar_tx_checksum(skb, fcb); > - } > + fcb = gfar_add_fcb(skb); > + lstatus |= BD_LFLAG(TXBD_TOE); > + gfar_tx_checksum(skb, fcb); > } > > if (priv->vlgrp && vlan_tx_tag_present(skb)) { > - if (unlikely(NULL == fcb)) > - fcb = gfar_add_fcb(&skb); > - if (likely(fcb != NULL)) { > + if (unlikely(NULL == fcb)) { > + fcb = gfar_add_fcb(skb); > lstatus |= BD_LFLAG(TXBD_TOE); > - gfar_tx_vlan(skb, fcb); > } > + > + gfar_tx_vlan(skb, fcb); > } > > /* setup the TxBD length and buffer pointer for the first BD */ > @@ -1433,7 +1435,7 @@ static int gfar_start_xmit(struct sk_buf > /* Unlock priv */ > spin_unlock_irqrestore(&priv->txlock, flags); > > - return 0; > + return NETDEV_TX_OK; > } > > /* Stops the kernel queue, and halts the controller */ ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] gianfar: fix headroom expansion code 2009-03-27 4:26 ` Li Yang @ 2009-03-27 7:39 ` David Miller 2009-03-27 8:06 ` Li Yang 2009-03-27 8:01 ` [PATCH] gianfar: only check headroom when FCB is needed Li Yang 1 sibling, 1 reply; 9+ messages in thread From: David Miller @ 2009-03-27 7:39 UTC (permalink / raw) To: leoli; +Cc: shemminger, netdev From: Li Yang <leoli@freescale.com> Date: Fri, 27 Mar 2009 12:26:33 +0800 > We have legacy devices without the offloading feature. And we can > even turn off the IP checksum offloading at runtime. Your code will > cause unnecessary realloc for these cases. > > I can propose a new patch to fix the pointer problem and add more > error handling. I'm applying Stephen's patch for now, please send any improvements relative to that. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] gianfar: fix headroom expansion code 2009-03-27 7:39 ` David Miller @ 2009-03-27 8:06 ` Li Yang 2009-03-27 8:11 ` David Miller 0 siblings, 1 reply; 9+ messages in thread From: Li Yang @ 2009-03-27 8:06 UTC (permalink / raw) To: David Miller; +Cc: shemminger, netdev On Fri, Mar 27, 2009 at 3:39 PM, David Miller <davem@davemloft.net> wrote: > From: Li Yang <leoli@freescale.com> > Date: Fri, 27 Mar 2009 12:26:33 +0800 > >> We have legacy devices without the offloading feature. And we can >> even turn off the IP checksum offloading at runtime. Your code will >> cause unnecessary realloc for these cases. >> >> I can propose a new patch to fix the pointer problem and add more >> error handling. > > I'm applying Stephen's patch for now, please send any improvements > relative to that. Ok. However is it ok to use kfree() instead of kfree_skb() in Stephen's patch? + skb_new = skb_realloc_headroom(skb, GMAC_FCB_LEN); + if (!skb_new) { + dev->stats.tx_errors++; + kfree(skb); + return NETDEV_TX_OK; + } - Leo ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] gianfar: fix headroom expansion code 2009-03-27 8:06 ` Li Yang @ 2009-03-27 8:11 ` David Miller 0 siblings, 0 replies; 9+ messages in thread From: David Miller @ 2009-03-27 8:11 UTC (permalink / raw) To: leoli; +Cc: shemminger, netdev From: Li Yang <leoli@freescale.com> Date: Fri, 27 Mar 2009 16:06:27 +0800 > On Fri, Mar 27, 2009 at 3:39 PM, David Miller <davem@davemloft.net> wrote: > > From: Li Yang <leoli@freescale.com> > > Date: Fri, 27 Mar 2009 12:26:33 +0800 > > > >> We have legacy devices without the offloading feature. And we can > >> even turn off the IP checksum offloading at runtime. Your code will > >> cause unnecessary realloc for these cases. > >> > >> I can propose a new patch to fix the pointer problem and add more > >> error handling. > > > > I'm applying Stephen's patch for now, please send any improvements > > relative to that. > > Ok. > > However is it ok to use kfree() instead of kfree_skb() in Stephen's patch? > > + skb_new = skb_realloc_headroom(skb, GMAC_FCB_LEN); > + if (!skb_new) { > + dev->stats.tx_errors++; > + kfree(skb); > + return NETDEV_TX_OK; > + } I'll fix this, thanks for noticing. ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] gianfar: only check headroom when FCB is needed 2009-03-27 4:26 ` Li Yang 2009-03-27 7:39 ` David Miller @ 2009-03-27 8:01 ` Li Yang 2009-03-27 22:54 ` David Miller 1 sibling, 1 reply; 9+ messages in thread From: Li Yang @ 2009-03-27 8:01 UTC (permalink / raw) To: davem; +Cc: shemminger, netdev, Li Yang Signed-off-by: Li Yang <leoli@freescale.com> --- drivers/net/gianfar.c | 6 ++++-- 1 files changed, 4 insertions(+), 2 deletions(-) diff --git a/drivers/net/gianfar.c b/drivers/net/gianfar.c index 14f9b5e..6e28088 100644 --- a/drivers/net/gianfar.c +++ b/drivers/net/gianfar.c @@ -1277,8 +1277,10 @@ static int gfar_start_xmit(struct sk_buff *skb, struct net_device *dev) base = priv->tx_bd_base; - /* make space for additional header */ - if (skb_headroom(skb) < GMAC_FCB_LEN) { + /* make space for additional header when fcb is needed */ + if (((skb->ip_summed == CHECKSUM_PARTIAL) || + (priv->vlgrp && vlan_tx_tag_present(skb))) && + (skb_headroom(skb) < GMAC_FCB_LEN)) { struct sk_buff *skb_new; skb_new = skb_realloc_headroom(skb, GMAC_FCB_LEN); -- 1.5.4 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] gianfar: only check headroom when FCB is needed 2009-03-27 8:01 ` [PATCH] gianfar: only check headroom when FCB is needed Li Yang @ 2009-03-27 22:54 ` David Miller 0 siblings, 0 replies; 9+ messages in thread From: David Miller @ 2009-03-27 22:54 UTC (permalink / raw) To: leoli; +Cc: shemminger, netdev From: Li Yang <leoli@freescale.com> Date: Fri, 27 Mar 2009 16:01:30 +0800 > Signed-off-by: Li Yang <leoli@freescale.com> Applied, thanks. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] gianfar: fix headroom expansion code 2009-03-26 17:08 ` [PATCH] gianfar: fix headroom expansion code Stephen Hemminger 2009-03-27 4:26 ` Li Yang @ 2009-03-27 7:38 ` David Miller 1 sibling, 0 replies; 9+ messages in thread From: David Miller @ 2009-03-27 7:38 UTC (permalink / raw) To: shemminger; +Cc: leoli, netdev From: Stephen Hemminger <shemminger@vyatta.com> Date: Thu, 26 Mar 2009 10:08:56 -0700 > The code that was added to increase headroom was wrong. > It doesn't handle the case where gfar_add_fcb() changes the skb. > Better to do check at start of transmit (outside of lock), where > error handling is better anyway. > > Signed-off-by: Stephen Hemminger <shemminger@vyatta.com> Applied, thanks Stephen. ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2009-03-27 22:54 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2009-03-26 0:21 gianfar: reallocate skb when headroom is not enough for fcb David Miller 2009-03-26 17:08 ` [PATCH] gianfar: fix headroom expansion code Stephen Hemminger 2009-03-27 4:26 ` Li Yang 2009-03-27 7:39 ` David Miller 2009-03-27 8:06 ` Li Yang 2009-03-27 8:11 ` David Miller 2009-03-27 8:01 ` [PATCH] gianfar: only check headroom when FCB is needed Li Yang 2009-03-27 22:54 ` David Miller 2009-03-27 7:38 ` [PATCH] gianfar: fix headroom expansion code David Miller
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.