From patchwork Mon Mar 22 21:10:48 2010 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: ben@bigfootnetworks.com X-Patchwork-Id: 48307 X-Patchwork-Delegate: davem@davemloft.net Return-Path: X-Original-To: patchwork-incoming@ozlabs.org Delivered-To: patchwork-incoming@ozlabs.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id 9E91CB7CF6 for ; Tue, 23 Mar 2010 08:11:18 +1100 (EST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756120Ab0CVVK7 (ORCPT ); Mon, 22 Mar 2010 17:10:59 -0400 Received: from out02.themessagecenter.com ([208.113.96.229]:2240 "EHLO out02.themessagecenter.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756010Ab0CVVK6 (ORCPT ); Mon, 22 Mar 2010 17:10:58 -0400 Received: from [192.168.66.93] by out02.themessagecenter.com with ESMTP (Tumbleweed Email Firewall SMTP Relay); Mon, 22 Mar 2010 13:59:35 -0700 X-Server-Uuid: 385D78BC-A8BE-4780-80CD-A503E34C32DD Received: from EX-BE-017-SFO.shared.themessagecenter.com ( [192.168.66.103]) by EX-CH-002-SFO.shared.themessagecenter.com ( [192.168.66.84]) with mapi; Mon, 22 Mar 2010 14:10:46 -0700 From: "Ben Menchaca (ben@bigfootnetworks.com)" To: "avorontsov@ru.mvista.com" , "David Miller" cc: "netdev@vger.kernel.org" , "Sandeep.Kumar@freescale.com" Date: Mon, 22 Mar 2010 14:10:48 -0700 Subject: RE: Gianfar: RX Recycle skb->len error Thread-Topic: Gianfar: RX Recycle skb->len error Thread-Index: AcrJ5JRd5N8WqCHXSpm81DwRKgvEEgABYgeQ Message-ID: References: <20100321.214642.67901344.davem@davemloft.net> <20100322172446.GA32758@oksana.dev.rtsoft.ru> In-Reply-To: <20100322172446.GA32758@oksana.dev.rtsoft.ru> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: acceptlanguage: en-US MIME-Version: 1.0 X-WSS-ID: 67B905BD28C11589199-01-01 Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org > Yes, skb_unreserve() (or skb_reset_reserved() for naming consistency?) > would be great. Just a couple of notes: It's yucky, but skb_reserve(skb, -alignamount) works, if you know the alignamount (which I discuss a bit below). If that's not intended, then should len be unsigned int? skb_put, skb_push, and skb_pull all seem to be unsigned int. It seems in both these cases for gianfar, the amount of the alignment is not immediately available to the code the recognizes that the skb_reset_reserved() is required. A bit larger rework appears to be needed. I took a shot at using a new heuristic for the rx_ring to prevent having to skb_reset_reserved() at all. The idea is to guarantee that the elements in ring are reserve()-ed, and that if we encounter an error condition that yields the two-skb case, free the new skb, since it has not yet been reserve()-ed. Looking forward to hearing from FS on the "right" way, though. Ben Menchaca Bigfoot Networks diff --git a/drivers/net/gianfar.c b/drivers/net/gianfar.c index b671555..82f8486 100644 --- a/drivers/net/gianfar.c +++ b/drivers/net/gianfar.c @@ -109,6 +109,7 @@ static int gfar_start_xmit(struct sk_buff *skb, struct net_device *dev); static void gfar_reset_task(struct work_struct *work); static void gfar_timeout(struct net_device *dev); static int gfar_close(struct net_device *dev); +static void gfar_skb_reserve_aligned(struct sk_buff *skb); struct sk_buff *gfar_new_skb(struct net_device *dev); static void gfar_new_rxbdp(struct gfar_priv_rx_q *rx_queue, struct rxbd8 *bdp, struct sk_buff *skb); @@ -214,6 +215,7 @@ static int gfar_init_bds(struct net_device *ndev) ndev->name); goto err_rxalloc_fail; } + gfar_skb_reserve_aligned(ndev); rx_queue->rx_skbuff[j] = skb; gfar_new_rxbdp(rx_queue, rxbdp, skb); @@ -2372,6 +2374,20 @@ static void gfar_new_rxbdp(struct gfar_priv_rx_q *rx_queue, struct rxbd8 *bdp, } +static void gfar_skb_reserve_aligned(struct sk_buff *skb) +{ + unsigned int alignamount; + + alignamount = RXBUF_ALIGNMENT - + (((unsigned long) skb->data) & (RXBUF_ALIGNMENT - 1)); + + /* We need the data buffer to be aligned properly. We will reserve + * as many bytes as needed to align the data properly + */ + skb_reserve(skb, alignamount); +} + + struct sk_buff * gfar_new_skb(struct net_device *dev) { unsigned int alignamount; @@ -2383,17 +2399,6 @@ struct sk_buff * gfar_new_skb(struct net_device *dev) skb = netdev_alloc_skb(dev, priv->rx_buffer_size + RXBUF_ALIGNMENT); - if (!skb) - return NULL; - - alignamount = RXBUF_ALIGNMENT - - (((unsigned long) skb->data) & (RXBUF_ALIGNMENT - 1)); - - /* We need the data buffer to be aligned properly. We will reserve - * as many bytes as needed to align the data properly - */ - skb_reserve(skb, alignamount); - return skb; } @@ -2532,21 +2537,17 @@ int gfar_clean_rx_ring(struct gfar_priv_rx_q *rx_queue, int rx_work_limit) if (unlikely(!newskb)) newskb = skb; else if (skb) { - /* - * We need to reset ->data to what it - * was before gfar_new_skb() re-aligned - * it to an RXBUF_ALIGNMENT boundary - * before we put the skb back on the - * recycle list. - */ - skb->data = skb->head + NET_SKB_PAD; - __skb_queue_head(&priv->rx_recycle, skb); + __skb_queue_head(&priv->rx_recycle, newskb); + newskb = skb; + } else { + gfar_skb_reserve_aligned(newskb); } } else { /* Increment the number of packets */ rx_queue->stats.rx_packets++; howmany++; + gfar_skb_reserve_aligned(newskb); if (likely(skb)) { pkt_len = bdp->length - ETH_FCS_LEN; /* Remove the FCS from the packet length */