Return-path: Received: from mail-ew0-f219.google.com ([209.85.219.219]:59255 "EHLO mail-ew0-f219.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755210AbZKWSdB (ORCPT ); Mon, 23 Nov 2009 13:33:01 -0500 Received: by ewy19 with SMTP id 19so2138486ewy.21 for ; Mon, 23 Nov 2009 10:33:07 -0800 (PST) From: Ivo van Doorn To: Gertjan van Wingerde Subject: Re: [PATCH 5/6] rt2x00: Properly request tx headroom for alignment operations. Date: Mon, 23 Nov 2009 19:33:04 +0100 Cc: users@rt2x00.serialmonkey.com, linux-wireless@vger.kernel.org References: <1258960565-26736-1-git-send-email-gwingerde@gmail.com> <1258960565-26736-5-git-send-email-gwingerde@gmail.com> <1258960565-26736-6-git-send-email-gwingerde@gmail.com> In-Reply-To: <1258960565-26736-6-git-send-email-gwingerde@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-15" Message-Id: <200911231933.04875.IvDoorn@gmail.com> Sender: linux-wireless-owner@vger.kernel.org List-ID: On Monday 23 November 2009, Gertjan van Wingerde wrote: > Current rt2x00 drivers may result in a "ieee80211_tx_status: headroom too > small" error message when a frame needs to be properly aligned before > transmitting it. > This is because the space needed to ensure proper alignment isn't > requested from mac80211. > Fix this by adding sufficient amount of alignment space to the amount > of headroom requested for TX frames. > > Reported-by: David Ellingsworth > Signed-off-by: Gertjan van Wingerde > Tested-by: David Ellingsworth > --- > drivers/net/wireless/rt2x00/rt2400pci.c | 30 +++++++++++++++------------- > drivers/net/wireless/rt2x00/rt2500pci.c | 30 +++++++++++++++------------- > drivers/net/wireless/rt2x00/rt2500usb.c | 29 ++++++++++++++------------- > drivers/net/wireless/rt2x00/rt2800lib.c | 7 +---- > drivers/net/wireless/rt2x00/rt2800pci.c | 31 +++++++++++++++-------------- > drivers/net/wireless/rt2x00/rt2800usb.c | 25 ++++++++++++----------- > drivers/net/wireless/rt2x00/rt2x00.h | 7 ++++++ > drivers/net/wireless/rt2x00/rt2x00queue.c | 6 ++-- > drivers/net/wireless/rt2x00/rt61pci.c | 28 ++++++++++++++------------ > drivers/net/wireless/rt2x00/rt73usb.c | 27 +++++++++++++------------ > 10 files changed, 117 insertions(+), 103 deletions(-) > > diff --git a/drivers/net/wireless/rt2x00/rt2400pci.c b/drivers/net/wireless/rt2x00/rt2400pci.c > index 6e68bc7..f534d70 100644 > --- a/drivers/net/wireless/rt2x00/rt2400pci.c > +++ b/drivers/net/wireless/rt2x00/rt2400pci.c > @@ -1432,7 +1432,8 @@ static int rt2400pci_probe_hw_mode(struct rt2x00_dev *rt2x00dev) > IEEE80211_HW_SIGNAL_DBM | > IEEE80211_HW_SUPPORTS_PS | > IEEE80211_HW_PS_NULLFUNC_STACK; > - rt2x00dev->hw->extra_tx_headroom = 0; > + rt2x00dev->hw->extra_tx_headroom = rt2x00dev->ops->extra_tx_headroom + > + RT2X00_ALIGN_SIZE; Can't rt2x00lib initialize rt2x00dev->hw->extra_tx_headroom? The drivers aren't performing the aligning, so it is logical that rt2x00lib adds the required bytes rather than the drivers. The driver already sets the value in ops->extra_tx_headroom, so it can rely on rt2x00lib to do the rest. > diff --git a/drivers/net/wireless/rt2x00/rt2800lib.c b/drivers/net/wireless/rt2x00/rt2800lib.c > index b809c49..de358b5 100644 > --- a/drivers/net/wireless/rt2x00/rt2800lib.c > +++ b/drivers/net/wireless/rt2x00/rt2800lib.c > @@ -2030,11 +2030,8 @@ int rt2800_probe_hw_mode(struct rt2x00_dev *rt2x00dev) > IEEE80211_HW_SUPPORTS_PS | > IEEE80211_HW_PS_NULLFUNC_STACK; > > - if (rt2x00_intf_is_usb(rt2x00dev)) > - rt2x00dev->hw->extra_tx_headroom = > - TXINFO_DESC_SIZE + TXWI_DESC_SIZE; > - else if (rt2x00_intf_is_pci(rt2x00dev)) > - rt2x00dev->hw->extra_tx_headroom = TXWI_DESC_SIZE; > + rt2x00dev->hw->extra_tx_headroom = rt2x00dev->ops->extra_tx_headroom + > + RT2X00_L2PAD_SIZE; In this case we don't use RT2X00_ALIGN_SIZE, but the driver does set a flag to tell rt2x00lib that L2padding should be used rather then aligning. So this case would still hold when it is moved to rt2x00lib. > /* > + * Constants for extra TX headroom for alignment purposes. > + */ > +#define RT2X00_ALIGN_SIZE 4 > +#define RT2X00_L2PAD_SIZE 8 #define RT2X00_L2PAD_SIZE RT2X00_ALIGN_SIZE + 4 Would probably better indicate why you are using 8. Ivo