[PATCH 0/3] ath: advance ath.ko with one more helper

[PATCH 0/3] ath: advance ath.ko with one more helper

[Luis R. Rodriguez]

This adds a common structure where we can start stuffing shared items
and introduces a helper for both ath5k and ath9k's use.

Luis R. Rodriguez (3):
  ath: add common ath_rxbuf_alloc() and make ath9k use it
  ath5k: use common ath.ko ath_rxbuf_alloc()
  ath5k: use bit shift operators for cache line size

 drivers/net/wireless/ath/Kconfig       |    4 +--
 drivers/net/wireless/ath/ath.h         |   30 ++++++++++++++++++++++++
 drivers/net/wireless/ath/ath5k/base.c  |   23 ++++++------------
 drivers/net/wireless/ath/ath5k/base.h  |    3 +-
 drivers/net/wireless/ath/ath9k/ath9k.h |    4 ++-
 drivers/net/wireless/ath/ath9k/main.c  |    2 +-
 drivers/net/wireless/ath/ath9k/recv.c  |   40 +++----------------------------
 drivers/net/wireless/ath/main.c        |   36 ++++++++++++++++++++++++++++
 8 files changed, 85 insertions(+), 57 deletions(-)
 create mode 100644 drivers/net/wireless/ath/ath.h

[PATCH 1/3] ath: add common ath_rxbuf_alloc() and make ath9k use it

[Luis R. Rodriguez]

Turns out ath5k and ath9k can share the same helper to
allocates RX skbs. We allocate skbs aligned to the cache line
size. This requirement seems to have come from AR5210; when
this was not done it seems sometimes we'd get bogus data. I'm
also told it may have been a performance enhancement
consideration. In the end I can't be sure we can remove this
on new hardware so just keep this and start sharing it through
ath.ko.

Make ath9k start using this, ath5k is next.

Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>
---
 drivers/net/wireless/ath/Kconfig       |    4 +--
 drivers/net/wireless/ath/ath.h         |   30 ++++++++++++++++++++++++
 drivers/net/wireless/ath/ath9k/ath9k.h |    4 ++-
 drivers/net/wireless/ath/ath9k/main.c  |    2 +-
 drivers/net/wireless/ath/ath9k/recv.c  |   40 +++----------------------------
 drivers/net/wireless/ath/main.c        |   36 ++++++++++++++++++++++++++++
 6 files changed, 75 insertions(+), 41 deletions(-)
 create mode 100644 drivers/net/wireless/ath/ath.h

diff --git a/drivers/net/wireless/ath/Kconfig b/drivers/net/wireless/ath/Kconfig
index 253b95a..11ded15 100644
--- a/drivers/net/wireless/ath/Kconfig
+++ b/drivers/net/wireless/ath/Kconfig
@@ -5,9 +5,7 @@ menuconfig ATH_COMMON
 	---help---
 	  This will enable the support for the Atheros wireless drivers.
 	  ath5k, ath9k and ar9170 drivers share some common code, this option
-	  enables the common ath.ko module which currently shares just common
-	  regulatory EEPROM helpers but will likely be extended later to share
-	  more between modules.
+	  enables the common ath.ko module which shares common helpers.
 
 	  For more information and documentation on this module you can visit:
 
diff --git a/drivers/net/wireless/ath/ath.h b/drivers/net/wireless/ath/ath.h
new file mode 100644
index 0000000..e284cd3
--- /dev/null
+++ b/drivers/net/wireless/ath/ath.h
@@ -0,0 +1,30 @@
+/*
+ * Copyright (c) 2008-2009 Atheros Communications Inc.
+ *
+ * Permission to use, copy, modify, and/or distribute this software for any
+ * purpose with or without fee is hereby granted, provided that the above
+ * copyright notice and this permission notice appear in all copies.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
+ * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
+ * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
+ * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
+ * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
+ * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
+ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
+ */
+
+#ifndef ATH_H
+#define ATH_H
+
+#include <linux/skbuff.h>
+
+struct ath_common {
+	u16 cachelsz;
+};
+
+struct sk_buff *ath_rxbuf_alloc(struct ath_common *common,
+				u32 len,
+				gfp_t gfp_mask);
+
+#endif /* ATH_H */
diff --git a/drivers/net/wireless/ath/ath9k/ath9k.h b/drivers/net/wireless/ath/ath9k/ath9k.h
index 7a5a157..2fd663c 100644
--- a/drivers/net/wireless/ath/ath9k/ath9k.h
+++ b/drivers/net/wireless/ath/ath9k/ath9k.h
@@ -25,6 +25,7 @@
 #include "hw.h"
 #include "rc.h"
 #include "debug.h"
+#include "../ath.h"
 
 struct ath_node;
 
@@ -532,6 +533,8 @@ struct ath_softc {
 	struct ieee80211_hw *hw;
 	struct device *dev;
 
+	struct ath_common common;
+
 	spinlock_t wiphy_lock; /* spinlock to protect ath_wiphy data */
 	struct ath_wiphy *pri_wiphy;
 	struct ath_wiphy **sec_wiphy; /* secondary wiphys (virtual radios); may
@@ -564,7 +567,6 @@ struct ath_softc {
 	u32 sc_flags; /* SC_OP_* */
 	u16 curtxpow;
 	u16 curaid;
-	u16 cachelsz;
 	u8 nbcnvifs;
 	u16 nvifs;
 	u8 tx_chainmask;
diff --git a/drivers/net/wireless/ath/ath9k/main.c b/drivers/net/wireless/ath/ath9k/main.c
index efe2e85..b29d0ff 100644
--- a/drivers/net/wireless/ath/ath9k/main.c
+++ b/drivers/net/wireless/ath/ath9k/main.c
@@ -1327,7 +1327,7 @@ static int ath_init_softc(u16 devid, struct ath_softc *sc)
 	 */
 	ath_read_cachesize(sc, &csz);
 	/* XXX assert csz is non-zero */
-	sc->cachelsz = csz << 2;	/* convert to bytes */
+	sc->common.cachelsz = csz << 2;	/* convert to bytes */
 
 	ah = kzalloc(sizeof(struct ath_hw), GFP_KERNEL);
 	if (!ah) {
diff --git a/drivers/net/wireless/ath/ath9k/recv.c b/drivers/net/wireless/ath/ath9k/recv.c
index 1a08c69..61dbdd2 100644
--- a/drivers/net/wireless/ath/ath9k/recv.c
+++ b/drivers/net/wireless/ath/ath9k/recv.c
@@ -100,38 +100,6 @@ static u64 ath_extend_tsf(struct ath_softc *sc, u32 rstamp)
 	return (tsf & ~0x7fff) | rstamp;
 }
 
-static struct sk_buff *ath_rxbuf_alloc(struct ath_softc *sc, u32 len, gfp_t gfp_mask)
-{
-	struct sk_buff *skb;
-	u32 off;
-
-	/*
-	 * Cache-line-align.  This is important (for the
-	 * 5210 at least) as not doing so causes bogus data
-	 * in rx'd frames.
-	 */
-
-	/* Note: the kernel can allocate a value greater than
-	 * what we ask it to give us. We really only need 4 KB as that
-	 * is this hardware supports and in fact we need at least 3849
-	 * as that is the MAX AMSDU size this hardware supports.
-	 * Unfortunately this means we may get 8 KB here from the
-	 * kernel... and that is actually what is observed on some
-	 * systems :( */
-	skb = __dev_alloc_skb(len + sc->cachelsz - 1, gfp_mask);
-	if (skb != NULL) {
-		off = ((unsigned long) skb->data) % sc->cachelsz;
-		if (off != 0)
-			skb_reserve(skb, sc->cachelsz - off);
-	} else {
-		DPRINTF(sc, ATH_DBG_FATAL,
-			"skbuff alloc of size %u failed\n", len);
-		return NULL;
-	}
-
-	return skb;
-}
-
 /*
  * For Decrypt or Demic errors, we only mark packet status here and always push
  * up the frame up to let mac80211 handle the actual error case, be it no
@@ -336,10 +304,10 @@ int ath_rx_init(struct ath_softc *sc, int nbufs)
 	spin_lock_init(&sc->rx.rxbuflock);
 
 	sc->rx.bufsize = roundup(IEEE80211_MAX_MPDU_LEN,
-				 min(sc->cachelsz, (u16)64));
+				 min(sc->common.cachelsz, (u16)64));
 
 	DPRINTF(sc, ATH_DBG_CONFIG, "cachelsz %u rxbufsize %u\n",
-		sc->cachelsz, sc->rx.bufsize);
+		sc->common.cachelsz, sc->rx.bufsize);
 
 	/* Initialize rx descriptors */
 
@@ -352,7 +320,7 @@ int ath_rx_init(struct ath_softc *sc, int nbufs)
 	}
 
 	list_for_each_entry(bf, &sc->rx.rxbuf, list) {
-		skb = ath_rxbuf_alloc(sc, sc->rx.bufsize, GFP_KERNEL);
+		skb = ath_rxbuf_alloc(&sc->common, sc->rx.bufsize, GFP_KERNEL);
 		if (skb == NULL) {
 			error = -ENOMEM;
 			goto err;
@@ -777,7 +745,7 @@ int ath_rx_tasklet(struct ath_softc *sc, int flush)
 
 		/* Ensure we always have an skb to requeue once we are done
 		 * processing the current buffer's skb */
-		requeue_skb = ath_rxbuf_alloc(sc, sc->rx.bufsize, GFP_ATOMIC);
+		requeue_skb = ath_rxbuf_alloc(&sc->common, sc->rx.bufsize, GFP_ATOMIC);
 
 		/* If there is no memory we ignore the current RX'd frame,
 		 * tell hardware it can give us a new frame using the old
diff --git a/drivers/net/wireless/ath/main.c b/drivers/net/wireless/ath/main.c
index 9949b11..487193f 100644
--- a/drivers/net/wireless/ath/main.c
+++ b/drivers/net/wireless/ath/main.c
@@ -17,6 +17,42 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 
+#include "ath.h"
+
 MODULE_AUTHOR("Atheros Communications");
 MODULE_DESCRIPTION("Shared library for Atheros wireless LAN cards.");
 MODULE_LICENSE("Dual BSD/GPL");
+
+struct sk_buff *ath_rxbuf_alloc(struct ath_common *common,
+				u32 len,
+				gfp_t gfp_mask)
+{
+	struct sk_buff *skb;
+	u32 off;
+
+	/*
+	 * Cache-line-align.  This is important (for the
+	 * 5210 at least) as not doing so causes bogus data
+	 * in rx'd frames.
+	 */
+
+	/* Note: the kernel can allocate a value greater than
+	 * what we ask it to give us. We really only need 4 KB as that
+	 * is this hardware supports and in fact we need at least 3849
+	 * as that is the MAX AMSDU size this hardware supports.
+	 * Unfortunately this means we may get 8 KB here from the
+	 * kernel... and that is actually what is observed on some
+	 * systems :( */
+	skb = __dev_alloc_skb(len + common->cachelsz - 1, gfp_mask);
+	if (skb != NULL) {
+		off = ((unsigned long) skb->data) % common->cachelsz;
+		if (off != 0)
+			skb_reserve(skb, common->cachelsz - off);
+	} else {
+		printk(KERN_ERR "skbuff alloc of size %u failed\n", len);
+		return NULL;
+	}
+
+	return skb;
+}
+EXPORT_SYMBOL(ath_rxbuf_alloc);
-- 
1.6.3.3

[PATCH 2/3] ath5k: use common ath.ko ath_rxbuf_alloc()

[Luis R. Rodriguez]

Now that its shared we can remove ath5k's own implementation.

Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>
---
 drivers/net/wireless/ath/ath5k/base.c |   21 +++++++--------------
 drivers/net/wireless/ath/ath5k/base.h |    3 ++-
 2 files changed, 9 insertions(+), 15 deletions(-)

diff --git a/drivers/net/wireless/ath/ath5k/base.c b/drivers/net/wireless/ath/ath5k/base.c
index acbcfc2..63c2b57 100644
--- a/drivers/net/wireless/ath/ath5k/base.c
+++ b/drivers/net/wireless/ath/ath5k/base.c
@@ -544,7 +544,7 @@ ath5k_pci_probe(struct pci_dev *pdev,
 	__set_bit(ATH_STAT_INVALID, sc->status);
 
 	sc->iobase = mem; /* So we can unmap it on detach */
-	sc->cachelsz = csz * sizeof(u32); /* convert to bytes */
+	sc->common.cachelsz = csz * sizeof(u32); /* convert to bytes */
 	sc->opmode = NL80211_IFTYPE_STATION;
 	sc->bintval = 1000;
 	mutex_init(&sc->lock);
@@ -1151,27 +1151,20 @@ static
 struct sk_buff *ath5k_rx_skb_alloc(struct ath5k_softc *sc, dma_addr_t *skb_addr)
 {
 	struct sk_buff *skb;
-	unsigned int off;
 
 	/*
 	 * Allocate buffer with headroom_needed space for the
 	 * fake physical layer header at the start.
 	 */
-	skb = dev_alloc_skb(sc->rxbufsize + sc->cachelsz - 1);
+	skb = ath_rxbuf_alloc(&sc->common,
+			      sc->rxbufsize + sc->common.cachelsz - 1,
+			      GFP_ATOMIC);
 
 	if (!skb) {
 		ATH5K_ERR(sc, "can't alloc skbuff of size %u\n",
-				sc->rxbufsize + sc->cachelsz - 1);
+				sc->rxbufsize + sc->common.cachelsz - 1);
 		return NULL;
 	}
-	/*
-	 * Cache-line-align.  This is important (for the
-	 * 5210 at least) as not doing so causes bogus data
-	 * in rx'd frames.
-	 */
-	off = ((unsigned long)skb->data) % sc->cachelsz;
-	if (off != 0)
-		skb_reserve(skb, sc->cachelsz - off);
 
 	*skb_addr = pci_map_single(sc->pdev,
 		skb->data, sc->rxbufsize, PCI_DMA_FROMDEVICE);
@@ -1613,10 +1606,10 @@ ath5k_rx_start(struct ath5k_softc *sc)
 	struct ath5k_buf *bf;
 	int ret;
 
-	sc->rxbufsize = roundup(IEEE80211_MAX_LEN, sc->cachelsz);
+	sc->rxbufsize = roundup(IEEE80211_MAX_LEN, sc->common.cachelsz);
 
 	ATH5K_DBG(sc, ATH5K_DEBUG_RESET, "cachelsz %u rxbufsize %u\n",
-		sc->cachelsz, sc->rxbufsize);
+		sc->common.cachelsz, sc->rxbufsize);
 
 	spin_lock_bh(&sc->rxbuflock);
 	sc->rxlink = NULL;
diff --git a/drivers/net/wireless/ath/ath5k/base.h b/drivers/net/wireless/ath/ath5k/base.h
index 667bd9d..25a72a8 100644
--- a/drivers/net/wireless/ath/ath5k/base.h
+++ b/drivers/net/wireless/ath/ath5k/base.h
@@ -50,6 +50,7 @@
 
 #include "ath5k.h"
 #include "debug.h"
+#include "../ath.h"
 
 #define	ATH_RXBUF	40		/* number of RX buffers */
 #define	ATH_TXBUF	200		/* number of TX buffers */
@@ -112,6 +113,7 @@ struct ath5k_rfkill {
  * associated with an instance of a device */
 struct ath5k_softc {
 	struct pci_dev		*pdev;		/* for dma mapping */
+	struct ath_common	common;
 	void __iomem		*iobase;	/* address of the device */
 	struct mutex		lock;		/* dev-level lock */
 	struct ieee80211_tx_queue_stats tx_stats[AR5K_NUM_TX_QUEUES];
@@ -134,7 +136,6 @@ struct ath5k_softc {
 	struct ath5k_desc	*desc;		/* TX/RX descriptors */
 	dma_addr_t		desc_daddr;	/* DMA (physical) address */
 	size_t			desc_len;	/* size of TX/RX descriptors */
-	u16			cachelsz;	/* cache line size */
 
 	DECLARE_BITMAP(status, 5);
 #define ATH_STAT_INVALID	0		/* disable hardware accesses */
-- 
1.6.3.3

[PATCH 3/3] ath5k: use bit shift operators for cache line size

[Luis R. Rodriguez]

This matches ath9k, providing consistency when reading both drivers.

Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>
---
 drivers/net/wireless/ath/ath5k/base.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/ath/ath5k/base.c b/drivers/net/wireless/ath/ath5k/base.c
index 63c2b57..2b3cf39 100644
--- a/drivers/net/wireless/ath/ath5k/base.c
+++ b/drivers/net/wireless/ath/ath5k/base.c
@@ -471,7 +471,7 @@ ath5k_pci_probe(struct pci_dev *pdev,
 		 * DMA to work so force a reasonable value here if it
 		 * comes up zero.
 		 */
-		csz = L1_CACHE_BYTES / sizeof(u32);
+		csz = L1_CACHE_BYTES >> 2;
 		pci_write_config_byte(pdev, PCI_CACHE_LINE_SIZE, csz);
 	}
 	/*
@@ -544,7 +544,7 @@ ath5k_pci_probe(struct pci_dev *pdev,
 	__set_bit(ATH_STAT_INVALID, sc->status);
 
 	sc->iobase = mem; /* So we can unmap it on detach */
-	sc->common.cachelsz = csz * sizeof(u32); /* convert to bytes */
+	sc->common.cachelsz = csz << 2; /* convert to bytes */
 	sc->opmode = NL80211_IFTYPE_STATION;
 	sc->bintval = 1000;
 	mutex_init(&sc->lock);
-- 
1.6.3.3

Re: [PATCH 3/3] ath5k: use bit shift operators for cache line size

[Bob Copeland]

On Wed, Aug 12, 2009 at 12:57 PM, Luis R.
Rodriguez<lrodriguez@atheros.com> wrote:
> This matches ath9k, providing consistency when reading both drivers.
>
> Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>
> ---
>  drivers/net/wireless/ath/ath5k/base.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath5k/base.c b/drivers/net/wireless/ath/ath5k/base.c
> index 63c2b57..2b3cf39 100644
> --- a/drivers/net/wireless/ath/ath5k/base.c
> +++ b/drivers/net/wireless/ath/ath5k/base.c
> @@ -471,7 +471,7 @@ ath5k_pci_probe(struct pci_dev *pdev,
>                 * DMA to work so force a reasonable value here if it
>                 * comes up zero.
>                 */
> -               csz = L1_CACHE_BYTES / sizeof(u32);
> +               csz = L1_CACHE_BYTES >> 2;
>                pci_write_config_byte(pdev, PCI_CACHE_LINE_SIZE, csz);

I'm not sure it's better, although the whole thing seems bogus to
me.  Is there really a modern machine where PCI cache line size should
only be four bytes?

-- 
Bob Copeland %% www.bobcopeland.com

Re: [PATCH 0/3] ath: advance ath.ko with one more helper

[Bob Copeland]

On Wed, Aug 12, 2009 at 12:56 PM, Luis R.
Rodriguez<lrodriguez@atheros.com> wrote:
> This adds a common structure where we can start stuffing shared items
> and introduces a helper for both ath5k and ath9k's use.
>
> Luis R. Rodriguez (3):
>  ath: add common ath_rxbuf_alloc() and make ath9k use it
>  ath5k: use common ath.ko ath_rxbuf_alloc()
>  ath5k: use bit shift operators for cache line size

Series looks OK to me but I think we can add a 4/4 that would:

- include ath/reg.h [don't remember if that's the name right now]
  in ath.h
- move reg structs into ath_common (although, this could be a
  bad call for ar9170, haven't really checked).

Then we only have to deal with one header and one composite struct
(for now) as the interface between the modules.

-- 
Bob Copeland %% www.bobcopeland.com

Re: [ath5k-devel] [PATCH 0/3] ath: advance ath.ko with one more helper

[Luis R. Rodriguez]

On Wed, Aug 12, 2009 at 10:21 AM, Bob Copeland<me@bobcopeland.com> wrote:
> On Wed, Aug 12, 2009 at 12:56 PM, Luis R.
> Rodriguez<lrodriguez@atheros.com> wrote:
>> This adds a common structure where we can start stuffing shared items
>> and introduces a helper for both ath5k and ath9k's use.
>>
>> Luis R. Rodriguez (3):
>>  ath: add common ath_rxbuf_alloc() and make ath9k use it
>>  ath5k: use common ath.ko ath_rxbuf_alloc()
>>  ath5k: use bit shift operators for cache line size
>
> Series looks OK to me but I think we can add a 4/4 that would:
>
> - include ath/reg.h [don't remember if that's the name right now]
>  in ath.h
> - move reg structs into ath_common (although, this could be a
>  bad call for ar9170, haven't really checked).
>
> Then we only have to deal with one header and one composite struct
> (for now) as the interface between the modules.

Sure, I was thinking of doing this after this. Is that acceptable?

  Luis

Re: [PATCH 3/3] ath5k: use bit shift operators for cache line size

[Luis R. Rodriguez]

On Wed, Aug 12, 2009 at 10:13 AM, Bob Copeland<me@bobcopeland.com> wrote:
> On Wed, Aug 12, 2009 at 12:57 PM, Luis R.
> Rodriguez<lrodriguez@atheros.com> wrote:
>> This matches ath9k, providing consistency when reading both drivers.
>>
>> Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>
>> ---
>>  drivers/net/wireless/ath/ath5k/base.c |    4 ++--
>>  1 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/wireless/ath/ath5k/base.c b/drivers/net/wireless/ath/ath5k/base.c
>> index 63c2b57..2b3cf39 100644
>> --- a/drivers/net/wireless/ath/ath5k/base.c
>> +++ b/drivers/net/wireless/ath/ath5k/base.c
>> @@ -471,7 +471,7 @@ ath5k_pci_probe(struct pci_dev *pdev,
>>                 * DMA to work so force a reasonable value here if it
>>                 * comes up zero.
>>                 */
>> -               csz = L1_CACHE_BYTES / sizeof(u32);
>> +               csz = L1_CACHE_BYTES >> 2;
>>                pci_write_config_byte(pdev, PCI_CACHE_LINE_SIZE, csz);
>
> I'm not sure it's better,

I did this for consistency between drivers but yes the advantage with
a shift is it should be cheaper than a multiplication. Although I am
not sure if simple multiplications get optimized by either the
compiler or an architecture to shifts.

> although the whole thing seems bogus to
> me.  Is there really a modern machine where PCI cache line size should
> only be four bytes?

Beats me, I was just matching the code for ath9k. The whole cache
alignment practice seems to be debatable to me and and hoping Sam
Leffer might recall the exact reasonings behind it.

Whether we remove this though would be a change which should go
through a separate patch I think.

  Luis

Re: [PATCH 3/3] ath5k: use bit shift operators for cache line size

[Bob Copeland]

On Wed, Aug 12, 2009 at 1:32 PM, Luis R.
Rodriguez<lrodriguez@atheros.com> wrote:
>>> diff --git a/drivers/net/wireless/ath/ath5k/base.c b/drivers/net/wireless/ath/ath5k/base.c
>>> index 63c2b57..2b3cf39 100644
>>> --- a/drivers/net/wireless/ath/ath5k/base.c
>>> +++ b/drivers/net/wireless/ath/ath5k/base.c
>>> @@ -471,7 +471,7 @@ ath5k_pci_probe(struct pci_dev *pdev,
>>>                 * DMA to work so force a reasonable value here if it
>>>                 * comes up zero.
>>>                 */
>>> -               csz = L1_CACHE_BYTES / sizeof(u32);
>>> +               csz = L1_CACHE_BYTES >> 2;
>>>                pci_write_config_byte(pdev, PCI_CACHE_LINE_SIZE, csz);
>>
>> I'm not sure it's better,
>
> I did this for consistency between drivers but yes the advantage with
> a shift is it should be cheaper than a multiplication. Although I am
> not sure if simple multiplications get optimized by either the
> compiler or an architecture to shifts.

It shouldn't matter in the above case -- division by two constants.
In the multiplication by power of 2 constant case, it should also
get optimized by the compiler.  '>> 2' looks like magic though, maybe
a comment to say why?

>> although the whole thing seems bogus to
>> me.  Is there really a modern machine where PCI cache line size should
>> only be four bytes?

To correct above, I misread what it was doing.. it's getting cpu cache
size and dividing by 4 to get the number of words, if cache line size
was zeroed initially.  Ok, I'll go back to sleep now.

Whether needed or not, there's a lot of confusing comments and
voodoo around the stuff (something about 2.4 kernels...) that would
be nice to clear up.

> Whether we remove this though would be a change which should go
> through a separate patch I think.

Yeah, that's reasonable.

-- 
Bob Copeland %% www.bobcopeland.com

Re: [ath5k-devel] [PATCH 0/3] ath: advance ath.ko with one more helper

[Nick Kossifidis]

2009/8/12 Luis R. Rodriguez <lrodriguez@atheros.com>:
> On Wed, Aug 12, 2009 at 10:21 AM, Bob Copeland<me@bobcopeland.com> wrote:
>> On Wed, Aug 12, 2009 at 12:56 PM, Luis R.
>> Rodriguez<lrodriguez@atheros.com> wrote:
>>> This adds a common structure where we can start stuffing shared items
>>> and introduces a helper for both ath5k and ath9k's use.
>>>
>>> Luis R. Rodriguez (3):
>>>  ath: add common ath_rxbuf_alloc() and make ath9k use it
>>>  ath5k: use common ath.ko ath_rxbuf_alloc()
>>>  ath5k: use bit shift operators for cache line size
>>
>> Series looks OK to me but I think we can add a 4/4 that would:
>>
>> - include ath/reg.h [don't remember if that's the name right now]
>>  in ath.h
>> - move reg structs into ath_common (although, this could be a
>>  bad call for ar9170, haven't really checked).
>>
>> Then we only have to deal with one header and one composite struct
>> (for now) as the interface between the modules.
>
> Sure, I was thinking of doing this after this. Is that acceptable?
>
>  Luis

You mean have a common reg.h for both ath5k and ath9k ? Works for me
but we have to deal with some new register ranges and some registers
have moved on ath9k + reg.h on ath9k has no descriptions, comments, it doesn't
include macros for accessing queue registers or tables, mixes eeprom offsets
with register addresses and other macros.

My plan was to start merging ath9k hw code on ath5k (not the driver part,
pcu.c, qcu.c, phy.c, eeprom.c etc + registers/eeprom offsets/descriptor formats)
and then move all that on ath and have both ath5k/ath9k use it. I
believe ath5k hw
code is much cleaner than ath9k and it's a better place to start, i've
seen most hw
code on ath9k and i'm ready to move on if it's O.K. with you.

-- 
GPG ID: 0xD21DB2DB
As you read this post global entropy rises. Have Fun ;-)
Nick

Re: [ath5k-devel] [PATCH 0/3] ath: advance ath.ko with one more helper

[Bob Copeland]

On Thu, Aug 13, 2009 at 05:13:50AM +0300, Nick Kossifidis wrote:
> You mean have a common reg.h for both ath5k and ath9k ?

I meant the existing regulatory, not register, headers -- but if
there is stuff to be shared in reg[ister].h we can do that too.

> code is much cleaner than ath9k and it's a better place to start, i've
> seen most hw
> code on ath9k and i'm ready to move on if it's O.K. with you.

I guess we should coordinate a bit.  I don't have much in the way
of unsent patches but perhaps we should pick a specific time to
start moving stuff around so we don't all have a pile of rejects.
Maybe after 2.6.32 merge window?

(This particular patch series, though, is fine enough now I think.)

-- 
Bob Copeland %% www.bobcopeland.com

Re: [ath5k-devel] [PATCH 0/3] ath: advance ath.ko with one more helper

[Luis R. Rodriguez]

On Wed, Aug 12, 2009 at 7:13 PM, Nick Kossifidis<mickflemm@gmail.com> wrote:
> 2009/8/12 Luis R. Rodriguez <lrodriguez@atheros.com>:
>> On Wed, Aug 12, 2009 at 10:21 AM, Bob Copeland<me@bobcopeland.com> wrote:
>>> On Wed, Aug 12, 2009 at 12:56 PM, Luis R.
>>> Rodriguez<lrodriguez@atheros.com> wrote:
>>>> This adds a common structure where we can start stuffing shared items
>>>> and introduces a helper for both ath5k and ath9k's use.
>>>>
>>>> Luis R. Rodriguez (3):
>>>>  ath: add common ath_rxbuf_alloc() and make ath9k use it
>>>>  ath5k: use common ath.ko ath_rxbuf_alloc()
>>>>  ath5k: use bit shift operators for cache line size
>>>
>>> Series looks OK to me but I think we can add a 4/4 that would:
>>>
>>> - include ath/reg.h [don't remember if that's the name right now]
>>>  in ath.h
>>> - move reg structs into ath_common (although, this could be a
>>>  bad call for ar9170, haven't really checked).
>>>
>>> Then we only have to deal with one header and one composite struct
>>> (for now) as the interface between the modules.
>>
>> Sure, I was thinking of doing this after this. Is that acceptable?
>>
>>  Luis
>
> You mean have a common reg.h for both ath5k and ath9k ? Works for me
> but we have to deal with some new register ranges and some registers
> have moved on ath9k + reg.h on ath9k has no descriptions, comments, it doesn't
> include macros for accessing queue registers or tables, mixes eeprom offsets
> with register addresses and other macros.

Sorry no I meant ath/reg.h as in regulatory, as that is already party of ath.ko.

> My plan was to start merging ath9k hw code on ath5k (not the driver part,
> pcu.c, qcu.c, phy.c, eeprom.c etc + registers/eeprom offsets/descriptor formats)
> and then move all that on ath and have both ath5k/ath9k use it. I
> believe ath5k hw
> code is much cleaner than ath9k and it's a better place to start, i've
> seen most hw
> code on ath9k and i'm ready to move on if it's O.K. with you.

When we update ath9k for new hw support it is easiest if that code
matches what we have internally at Atheros. Granted we diverge from
that every now and then but I believe those changes can be brought
back in that we do and yet keep the internal code working. I believe
the changes so far bring clarity and readability. Unfortunately we
haven't yet gotten any the changes we've made on ath9k hw access stuff
or regulatory merged back in so we keep diverging more and more from
our internal codebase.

Because of this for now I would not welcome bringing ath9k code to
ath5k in any way whatsoever. What I think is reasonable though is to
start merging into ath.ko common code which doesn't change much or
would mean diverging a lot from ath9k's current style for the hw
related stuff. Don't get me wrong, changes are welcomed but the less
intrusive the better. "start merging ath9k hw code on ath5k" sounds
very intrusive by all means.

Lets do this slowly and take it on, on a patch by patch basis.

  Luis

Re: [ath5k-devel] [PATCH 0/3] ath: advance ath.ko with one more helper

[Nick Kossifidis]

2009/8/13 Luis R. Rodriguez <lrodriguez@atheros.com>:
> On Wed, Aug 12, 2009 at 7:13 PM, Nick Kossifidis<mickflemm@gmail.com> wrote:
>> 2009/8/12 Luis R. Rodriguez <lrodriguez@atheros.com>:
>>> On Wed, Aug 12, 2009 at 10:21 AM, Bob Copeland<me@bobcopeland.com> wrote:
>>>> On Wed, Aug 12, 2009 at 12:56 PM, Luis R.
>>>> Rodriguez<lrodriguez@atheros.com> wrote:
>>>>> This adds a common structure where we can start stuffing shared items
>>>>> and introduces a helper for both ath5k and ath9k's use.
>>>>>
>>>>> Luis R. Rodriguez (3):
>>>>>  ath: add common ath_rxbuf_alloc() and make ath9k use it
>>>>>  ath5k: use common ath.ko ath_rxbuf_alloc()
>>>>>  ath5k: use bit shift operators for cache line size
>>>>
>>>> Series looks OK to me but I think we can add a 4/4 that would:
>>>>
>>>> - include ath/reg.h [don't remember if that's the name right now]
>>>>  in ath.h
>>>> - move reg structs into ath_common (although, this could be a
>>>>  bad call for ar9170, haven't really checked).
>>>>
>>>> Then we only have to deal with one header and one composite struct
>>>> (for now) as the interface between the modules.
>>>
>>> Sure, I was thinking of doing this after this. Is that acceptable?
>>>
>>>  Luis
>>
>> You mean have a common reg.h for both ath5k and ath9k ? Works for me
>> but we have to deal with some new register ranges and some registers
>> have moved on ath9k + reg.h on ath9k has no descriptions, comments, it doesn't
>> include macros for accessing queue registers or tables, mixes eeprom offsets
>> with register addresses and other macros.
>
> Sorry no I meant ath/reg.h as in regulatory, as that is already party of ath.ko.
>
>> My plan was to start merging ath9k hw code on ath5k (not the driver part,
>> pcu.c, qcu.c, phy.c, eeprom.c etc + registers/eeprom offsets/descriptor formats)
>> and then move all that on ath and have both ath5k/ath9k use it. I
>> believe ath5k hw
>> code is much cleaner than ath9k and it's a better place to start, i've
>> seen most hw
>> code on ath9k and i'm ready to move on if it's O.K. with you.
>
> When we update ath9k for new hw support it is easiest if that code
> matches what we have internally at Atheros. Granted we diverge from
> that every now and then but I believe those changes can be brought
> back in that we do and yet keep the internal code working. I believe
> the changes so far bring clarity and readability. Unfortunately we
> haven't yet gotten any the changes we've made on ath9k hw access stuff
> or regulatory merged back in so we keep diverging more and more from
> our internal codebase.
>
> Because of this for now I would not welcome bringing ath9k code to
> ath5k in any way whatsoever. What I think is reasonable though is to
> start merging into ath.ko common code which doesn't change much or
> would mean diverging a lot from ath9k's current style for the hw
> related stuff. Don't get me wrong, changes are welcomed but the less
> intrusive the better. "start merging ath9k hw code on ath5k" sounds
> very intrusive by all means.
>
> Lets do this slowly and take it on, on a patch by patch basis.
>
>  Luis
>

ACK ;-)


-- 
GPG ID: 0xD21DB2DB
As you read this post global entropy rises. Have Fun ;-)
Nick