pci_set_mwi() and ath5k

pci_set_mwi() and ath5k

[Luis R. Rodriguez]

Curious if anyone recalls the issues seen with enabling MWI on ath5k.
I could have sworn there was some discussion on this but for the life
of me I cannot find it.

  Luis

Re: pci_set_mwi() and ath5k

[Nick Kossifidis]

2009/11/4 Luis R. Rodriguez <mcgrof@gmail.com>:
> Curious if anyone recalls the issues seen with enabling MWI on ath5k.
> I could have sworn there was some discussion on this but for the life
> of me I cannot find it.
>
>  Luis

Maybe this one ?
http://osdir.com/ml/linux.drivers.ath5k.devel/2008-07/msg00088.html



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

Re: pci_set_mwi() and ath5k

[Luis R. Rodriguez]

On Wed, Nov 4, 2009 at 12:12 PM, Nick Kossifidis <mickflemm@gmail.com> wrote:
> 2009/11/4 Luis R. Rodriguez <mcgrof@gmail.com>:
>> Curious if anyone recalls the issues seen with enabling MWI on ath5k.
>> I could have sworn there was some discussion on this but for the life
>> of me I cannot find it.
>>
>>  Luis
>
> Maybe this one ?
> http://osdir.com/ml/linux.drivers.ath5k.devel/2008-07/msg00088.html

That was it, thanks! For the record then Kyle pointed to this bug:

http://marc.info/?t=121430463700001&r=1&w=2

as a reference for possible issues.

  Luis

Re: pci_set_mwi() and ath5k

[Luis R. Rodriguez]

On Wed, Nov 4, 2009 at 1:30 PM, Luis R. Rodriguez <mcgrof@gmail.com> wrote:
> On Wed, Nov 4, 2009 at 12:12 PM, Nick Kossifidis <mickflemm@gmail.com> wrote:
>> 2009/11/4 Luis R. Rodriguez <mcgrof@gmail.com>:
>>> Curious if anyone recalls the issues seen with enabling MWI on ath5k.
>>> I could have sworn there was some discussion on this but for the life
>>> of me I cannot find it.
>>>
>>>  Luis
>>
>> Maybe this one ?
>> http://osdir.com/ml/linux.drivers.ath5k.devel/2008-07/msg00088.html
>
> That was it, thanks! For the record then Kyle pointed to this bug:
>
> http://marc.info/?t=121430463700001&r=1&w=2
>
> as a reference for possible issues.

Actually the threads above are for MSI, not MWI, which would be set
with pci_set_mwi() not pci_disable_msi().

MWI is for enabling memory write invalidate.

I guess we never had the MWI discussion then for ath5k,

Either way I'm reluctant to enable it and was actually considering
simplifying the the PCI cache line size thing on ath/ath5k/ath9k. Will
send an RFC.

  Luis

Re: pci_set_mwi() and ath5k

[Luis R. Rodriguez]

On Wed, Nov 4, 2009 at 1:36 PM, Luis R. Rodriguez <mcgrof@gmail.com> wrote:
> On Wed, Nov 4, 2009 at 1:30 PM, Luis R. Rodriguez <mcgrof@gmail.com> wrote:
>> On Wed, Nov 4, 2009 at 12:12 PM, Nick Kossifidis <mickflemm@gmail.com> wrote:
>>> 2009/11/4 Luis R. Rodriguez <mcgrof@gmail.com>:
>>>> Curious if anyone recalls the issues seen with enabling MWI on ath5k.
>>>> I could have sworn there was some discussion on this but for the life
>>>> of me I cannot find it.
>>>>
>>>>  Luis
>>>
>>> Maybe this one ?
>>> http://osdir.com/ml/linux.drivers.ath5k.devel/2008-07/msg00088.html
>>
>> That was it, thanks! For the record then Kyle pointed to this bug:
>>
>> http://marc.info/?t=121430463700001&r=1&w=2
>>
>> as a reference for possible issues.
>
> Actually the threads above are for MSI, not MWI, which would be set
> with pci_set_mwi() not pci_disable_msi().
>
> MWI is for enabling memory write invalidate.
>
> I guess we never had the MWI discussion then for ath5k,
>
> Either way I'm reluctant to enable it and was actually considering
> simplifying the the PCI cache line size thing on ath/ath5k/ath9k. Will
> send an RFC.

Even better: I just confirmation from our systems team that our legacy
devices and 11n PCI devices don't support MWR so I'll remove all that
cruft crap.

  Luis

Re: pci_set_mwi() and ath5k

[Luis R. Rodriguez]

On Wed, Nov 4, 2009 at 1:52 PM, Luis R. Rodriguez <mcgrof@gmail.com> wrote:
> On Wed, Nov 4, 2009 at 1:36 PM, Luis R. Rodriguez <mcgrof@gmail.com> wrote:
>> On Wed, Nov 4, 2009 at 1:30 PM, Luis R. Rodriguez <mcgrof@gmail.com> wrote:
>>> On Wed, Nov 4, 2009 at 12:12 PM, Nick Kossifidis <mickflemm@gmail.com> wrote:
>>>> 2009/11/4 Luis R. Rodriguez <mcgrof@gmail.com>:
>>>>> Curious if anyone recalls the issues seen with enabling MWI on ath5k.
>>>>> I could have sworn there was some discussion on this but for the life
>>>>> of me I cannot find it.
>>>>>
>>>>>  Luis
>>>>
>>>> Maybe this one ?
>>>> http://osdir.com/ml/linux.drivers.ath5k.devel/2008-07/msg00088.html
>>>
>>> That was it, thanks! For the record then Kyle pointed to this bug:
>>>
>>> http://marc.info/?t=121430463700001&r=1&w=2
>>>
>>> as a reference for possible issues.
>>
>> Actually the threads above are for MSI, not MWI, which would be set
>> with pci_set_mwi() not pci_disable_msi().
>>
>> MWI is for enabling memory write invalidate.
>>
>> I guess we never had the MWI discussion then for ath5k,
>>
>> Either way I'm reluctant to enable it and was actually considering
>> simplifying the the PCI cache line size thing on ath/ath5k/ath9k. Will
>> send an RFC.
>
> Even better: I just confirmation from our systems team that our legacy
> devices and 11n PCI devices don't support MWR so I'll remove all that
> cruft crap.

I meant MWI of course.

  Luis

Re: pci_set_mwi() and ath5k

[Matthew Wilcox]

On Wed, Nov 04, 2009 at 01:52:30PM -0800, Luis R. Rodriguez wrote:
> > Even better: I just confirmation from our systems team that our legacy
> > devices and 11n PCI devices don't support MWR so I'll remove all that
> > cruft crap.
> 
> I meant MWI of course.

Yes, but they don't necessarily just use cacheline size for MWI ... some
devices use cacheline size for setting up data structures.  Might be
worth just checking explicitly that they don't use the cacheline size
register for anything.

-- 
Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: pci_set_mwi() and ath5k

[Luis R. Rodriguez]

On Wed, Nov 4, 2009 at 2:00 PM, Matthew Wilcox <matthew@wil.cx> wrote:
> On Wed, Nov 04, 2009 at 01:52:30PM -0800, Luis R. Rodriguez wrote:
>> > Even better: I just confirmation from our systems team that our legacy
>> > devices and 11n PCI devices don't support MWR so I'll remove all that
>> > cruft crap.
>>
>> I meant MWI of course.
>
> Yes, but they don't necessarily just use cacheline size for MWI ... some
> devices use cacheline size for setting up data structures.  Might be
> worth just checking explicitly that they don't use the cacheline size
> register for anything.

Oh right -- so the typical Atheros hack for this is to check the cache
line size, and if its 0 set it to L1_CACHE_BYTES. Then eventually read
from PCI_CACHE_LINE_SIZE pci config to align the skb data. So what I
was doing now is removing all this cruft and replacing it with a
generic allocator for atheros drivers that aligns simply to the
L1_CACHE_BYTES. Sound kosher?

  Luis

Re: pci_set_mwi() and ath5k

[Luis R. Rodriguez]

On Wed, Nov 04, 2009 at 02:04:11PM -0800, Luis R. Rodriguez wrote:
> On Wed, Nov 4, 2009 at 2:00 PM, Matthew Wilcox <matthew@wil.cx> wrote:
> > On Wed, Nov 04, 2009 at 01:52:30PM -0800, Luis R. Rodriguez wrote:
> >> > Even better: I just confirmation from our systems team that our legacy
> >> > devices and 11n PCI devices don't support MWR so I'll remove all that
> >> > cruft crap.
> >>
> >> I meant MWI of course.
> >
> > Yes, but they don't necessarily just use cacheline size for MWI ... some
> > devices use cacheline size for setting up data structures.  Might be
> > worth just checking explicitly that they don't use the cacheline size
> > register for anything.
> 
> Oh right -- so the typical Atheros hack for this is to check the cache
> line size, and if its 0 set it to L1_CACHE_BYTES. Then eventually read
> from PCI_CACHE_LINE_SIZE pci config to align the skb data. So what I
> was doing now is removing all this cruft and replacing it with a
> generic allocator for atheros drivers that aligns simply to the
> L1_CACHE_BYTES. Sound kosher?

Something like this:

 drivers/net/wireless/ath/ath.h         |    6 +-----
 drivers/net/wireless/ath/ath5k/base.c  |   27 +++------------------------
 drivers/net/wireless/ath/ath9k/ahb.c   |    8 --------
 drivers/net/wireless/ath/ath9k/ath9k.h |    5 -----
 drivers/net/wireless/ath/ath9k/main.c  |    9 ---------
 drivers/net/wireless/ath/ath9k/pci.c   |   20 --------------------
 drivers/net/wireless/ath/ath9k/recv.c  |   11 +++++------
 drivers/net/wireless/ath/main.c        |   27 +++++----------------------
 8 files changed, 14 insertions(+), 99 deletions(-)

diff --git a/drivers/net/wireless/ath/ath.h b/drivers/net/wireless/ath/ath.h
index 4af1362..438f46f 100644
--- a/drivers/net/wireless/ath/ath.h
+++ b/drivers/net/wireless/ath/ath.h
@@ -63,7 +63,6 @@ struct ath_ops {
 struct ath_common;
 
 struct ath_bus_ops {
-	void		(*read_cachesize)(struct ath_common *common, int *csz);
 	void		(*cleanup)(struct ath_common *common);
 	bool		(*eeprom_read)(struct ath_common *common, u32 off, u16 *data);
 	void		(*bt_coex_prep)(struct ath_common *common);
@@ -78,7 +77,6 @@ struct ath_common {
 
 	struct ath_ani ani;
 
-	u16 cachelsz;
 	u16 curaid;
 	u8 macaddr[ETH_ALEN];
 	u8 curbssid[ETH_ALEN];
@@ -94,9 +92,7 @@ struct ath_common {
 	const struct ath_bus_ops *bus_ops;
 };
 
-struct sk_buff *ath_rxbuf_alloc(struct ath_common *common,
-				u32 len,
-				gfp_t gfp_mask);
+struct sk_buff *ath_rxbuf_alloc(u32 len, gfp_t gfp_mask);
 
 void ath_hw_setbssidmask(struct ath_common *common);
 
diff --git a/drivers/net/wireless/ath/ath5k/base.c b/drivers/net/wireless/ath/ath5k/base.c
index c7fc13c..6943660 100644
--- a/drivers/net/wireless/ath/ath5k/base.c
+++ b/drivers/net/wireless/ath/ath5k/base.c
@@ -466,7 +466,6 @@ ath5k_pci_probe(struct pci_dev *pdev,
 	struct ath_common *common;
 	struct ieee80211_hw *hw;
 	int ret;
-	u8 csz;
 
 	ret = pci_enable_device(pdev);
 	if (ret) {
@@ -482,22 +481,6 @@ ath5k_pci_probe(struct pci_dev *pdev,
 	}
 
 	/*
-	 * Cache line size is used to size and align various
-	 * structures used to communicate with the hardware.
-	 */
-	pci_read_config_byte(pdev, PCI_CACHE_LINE_SIZE, &csz);
-	if (csz == 0) {
-		/*
-		 * Linux 2.4.18 (at least) writes the cache line size
-		 * register as a 16-bit wide register which is wrong.
-		 * We must have this setup properly for rx buffer
-		 * DMA to work so force a reasonable value here if it
-		 * comes up zero.
-		 */
-		csz = L1_CACHE_BYTES >> 2;
-		pci_write_config_byte(pdev, PCI_CACHE_LINE_SIZE, csz);
-	}
-	/*
 	 * The default setting of latency timer yields poor results,
 	 * set it to the value used by other systems.  It may be worth
 	 * tweaking this setting more.
@@ -598,7 +581,6 @@ ath5k_pci_probe(struct pci_dev *pdev,
 	common->ops = &ath5k_common_ops;
 	common->ah = sc->ah;
 	common->hw = hw;
-	common->cachelsz = csz << 2; /* convert to bytes */
 
 	/* Initialize device */
 	ret = ath5k_hw_attach(sc);
@@ -1184,9 +1166,7 @@ struct sk_buff *ath5k_rx_skb_alloc(struct ath5k_softc *sc, dma_addr_t *skb_addr)
 	 * Allocate buffer with headroom_needed space for the
 	 * fake physical layer header at the start.
 	 */
-	skb = ath_rxbuf_alloc(common,
-			      common->rx_bufsize,
-			      GFP_ATOMIC);
+	skb = ath_rxbuf_alloc(common->rx_bufsize, GFP_ATOMIC);
 
 	if (!skb) {
 		ATH5K_ERR(sc, "can't alloc skbuff of size %u\n",
@@ -1636,10 +1616,9 @@ ath5k_rx_start(struct ath5k_softc *sc)
 	struct ath5k_buf *bf;
 	int ret;
 
-	common->rx_bufsize = roundup(IEEE80211_MAX_LEN, common->cachelsz);
+	common->rx_bufsize = IEEE80211_MAX_LEN;
 
-	ATH5K_DBG(sc, ATH5K_DEBUG_RESET, "cachelsz %u rx_bufsize %u\n",
-		  common->cachelsz, common->rx_bufsize);
+	ATH5K_DBG(sc, ATH5K_DEBUG_RESET, "rx_bufsize %u\n", common->rx_bufsize);
 
 	spin_lock_bh(&sc->rxbuflock);
 	sc->rxlink = NULL;
diff --git a/drivers/net/wireless/ath/ath9k/ahb.c b/drivers/net/wireless/ath/ath9k/ahb.c
index 329e6bc..9949524 100644
--- a/drivers/net/wireless/ath/ath9k/ahb.c
+++ b/drivers/net/wireless/ath/ath9k/ahb.c
@@ -21,12 +21,6 @@
 #include <linux/ath9k_platform.h>
 #include "ath9k.h"
 
-/* return bus cachesize in 4B word units */
-static void ath_ahb_read_cachesize(struct ath_common *common, int *csz)
-{
-	*csz = L1_CACHE_BYTES >> 2;
-}
-
 static void ath_ahb_cleanup(struct ath_common *common)
 {
 	struct ath_softc *sc = (struct ath_softc *)common->priv;
@@ -53,9 +47,7 @@ static bool ath_ahb_eeprom_read(struct ath_common *common, u32 off, u16 *data)
 }
 
 static struct ath_bus_ops ath_ahb_bus_ops  = {
-	.read_cachesize = ath_ahb_read_cachesize,
 	.cleanup = ath_ahb_cleanup,
-
 	.eeprom_read = ath_ahb_eeprom_read,
 };
 
diff --git a/drivers/net/wireless/ath/ath9k/ath9k.h b/drivers/net/wireless/ath/ath9k/ath9k.h
index 377b0ea..66d4a70 100644
--- a/drivers/net/wireless/ath/ath9k/ath9k.h
+++ b/drivers/net/wireless/ath/ath9k/ath9k.h
@@ -618,11 +618,6 @@ int ath_get_hal_qnum(u16 queue, struct ath_softc *sc);
 int ath_get_mac80211_qnum(u32 queue, struct ath_softc *sc);
 int ath_cabq_update(struct ath_softc *);
 
-static inline void ath_read_cachesize(struct ath_common *common, int *csz)
-{
-	common->bus_ops->read_cachesize(common, csz);
-}
-
 static inline void ath_bus_cleanup(struct ath_common *common)
 {
 	common->bus_ops->cleanup(common);
diff --git a/drivers/net/wireless/ath/ath9k/main.c b/drivers/net/wireless/ath/ath9k/main.c
index 01ac897..af9c785 100644
--- a/drivers/net/wireless/ath/ath9k/main.c
+++ b/drivers/net/wireless/ath/ath9k/main.c
@@ -1624,7 +1624,6 @@ static int ath_init_softc(u16 devid, struct ath_softc *sc, u16 subsysid,
 	struct ath_hw *ah = NULL;
 	struct ath_common *common;
 	int r = 0, i;
-	int csz = 0;
 	int qnum;
 
 	/* XXX: hardware will not be ready until ath_open() being called */
@@ -1656,14 +1655,6 @@ static int ath_init_softc(u16 devid, struct ath_softc *sc, u16 subsysid,
 	common->priv = sc;
 	common->debug_mask = ath9k_debug;
 
-	/*
-	 * Cache line size is used to size and align various
-	 * structures used to communicate with the hardware.
-	 */
-	ath_read_cachesize(common, &csz);
-	/* XXX assert csz is non-zero */
-	common->cachelsz = csz << 2;	/* convert to bytes */
-
 	r = ath9k_hw_init(ah);
 	if (r) {
 		ath_print(common, ATH_DBG_FATAL,
diff --git a/drivers/net/wireless/ath/ath9k/pci.c b/drivers/net/wireless/ath/ath9k/pci.c
index 5321f73..b0f72e4 100644
--- a/drivers/net/wireless/ath/ath9k/pci.c
+++ b/drivers/net/wireless/ath/ath9k/pci.c
@@ -30,25 +30,6 @@ static struct pci_device_id ath_pci_id_table[] __devinitdata = {
 	{ 0 }
 };
 
-/* return bus cachesize in 4B word units */
-static void ath_pci_read_cachesize(struct ath_common *common, int *csz)
-{
-	struct ath_softc *sc = (struct ath_softc *) common->priv;
-	u8 u8tmp;
-
-	pci_read_config_byte(to_pci_dev(sc->dev), PCI_CACHE_LINE_SIZE, &u8tmp);
-	*csz = (int)u8tmp;
-
-	/*
-	 * This check was put in to avoid "unplesant" consequences if
-	 * the bootrom has not fully initialized all PCI devices.
-	 * Sometimes the cache line size register is not set
-	 */
-
-	if (*csz == 0)
-		*csz = DEFAULT_CACHELINE >> 2;   /* Use the default size */
-}
-
 static void ath_pci_cleanup(struct ath_common *common)
 {
 	struct ath_softc *sc = (struct ath_softc *) common->priv;
@@ -97,7 +78,6 @@ static void ath_pci_bt_coex_prep(struct ath_common *common)
 }
 
 const static struct ath_bus_ops ath_pci_bus_ops = {
-	.read_cachesize = ath_pci_read_cachesize,
 	.cleanup = ath_pci_cleanup,
 	.eeprom_read = ath_pci_eeprom_read,
 	.bt_coex_prep = ath_pci_bt_coex_prep,
diff --git a/drivers/net/wireless/ath/ath9k/recv.c b/drivers/net/wireless/ath/ath9k/recv.c
index 36d23ef..341ff0a 100644
--- a/drivers/net/wireless/ath/ath9k/recv.c
+++ b/drivers/net/wireless/ath/ath9k/recv.c
@@ -346,11 +346,10 @@ int ath_rx_init(struct ath_softc *sc, int nbufs)
 	sc->sc_flags &= ~SC_OP_RXFLUSH;
 	spin_lock_init(&sc->rx.rxbuflock);
 
-	common->rx_bufsize = roundup(IEEE80211_MAX_MPDU_LEN,
-				     min(common->cachelsz, (u16)64));
+	common->rx_bufsize = IEEE80211_MAX_MPDU_LEN;
 
-	ath_print(common, ATH_DBG_CONFIG, "cachelsz %u rxbufsize %u\n",
-		  common->cachelsz, common->rx_bufsize);
+	ath_print(common, ATH_DBG_CONFIG, "rx_bufsize %u\n",
+		  common->rx_bufsize);
 
 	/* Initialize rx descriptors */
 
@@ -363,7 +362,7 @@ int ath_rx_init(struct ath_softc *sc, int nbufs)
 	}
 
 	list_for_each_entry(bf, &sc->rx.rxbuf, list) {
-		skb = ath_rxbuf_alloc(common, common->rx_bufsize, GFP_KERNEL);
+		skb = ath_rxbuf_alloc(common->rx_bufsize, GFP_KERNEL);
 		if (skb == NULL) {
 			error = -ENOMEM;
 			goto err;
@@ -810,7 +809,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(common, common->rx_bufsize, GFP_ATOMIC);
+		requeue_skb = ath_rxbuf_alloc(common->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 487193f..0a1c762 100644
--- a/drivers/net/wireless/ath/main.c
+++ b/drivers/net/wireless/ath/main.c
@@ -23,35 +23,18 @@ 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 *ath_rxbuf_alloc(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);
+	skb = __dev_alloc_skb(len + L1_CACHE_BYTES - 1, gfp_mask);
 	if (skb != NULL) {
-		off = ((unsigned long) skb->data) % common->cachelsz;
+		off = ((unsigned long) skb->data) % L1_CACHE_BYTES;
 		if (off != 0)
-			skb_reserve(skb, common->cachelsz - off);
-	} else {
-		printk(KERN_ERR "skbuff alloc of size %u failed\n", len);
+			skb_reserve(skb, L1_CACHE_BYTES - off);
+	} else
 		return NULL;
-	}
 
 	return skb;
 }

Re: pci_set_mwi() and ath5k

[Luis R. Rodriguez]

On Wed, Nov 4, 2009 at 2:14 PM, Luis R. Rodriguez
<mcgrof@bombadil.infradead.org> wrote:
> On Wed, Nov 04, 2009 at 02:04:11PM -0800, Luis R. Rodriguez wrote:
>> On Wed, Nov 4, 2009 at 2:00 PM, Matthew Wilcox <matthew@wil.cx> wrote:
>> > On Wed, Nov 04, 2009 at 01:52:30PM -0800, Luis R. Rodriguez wrote:
>> >> > Even better: I just confirmation from our systems team that our legacy
>> >> > devices and 11n PCI devices don't support MWR so I'll remove all that
>> >> > cruft crap.
>> >>
>> >> I meant MWI of course.
>> >
>> > Yes, but they don't necessarily just use cacheline size for MWI ... some
>> > devices use cacheline size for setting up data structures.  Might be
>> > worth just checking explicitly that they don't use the cacheline size
>> > register for anything.
>>
>> Oh right -- so the typical Atheros hack for this is to check the cache
>> line size, and if its 0 set it to L1_CACHE_BYTES. Then eventually read
>> from PCI_CACHE_LINE_SIZE pci config to align the skb data. So what I
>> was doing now is removing all this cruft and replacing it with a
>> generic allocator for atheros drivers that aligns simply to the
>> L1_CACHE_BYTES. Sound kosher?
>
> Something like this:

I also checked with our systems team and it seems we do not rely on
the  PCI_CACHE_LINE_SIZE pci config to internally align data in the
hardware itself. It that what you meant?

  Luis

Re: pci_set_mwi() and ath5k

[Matthew Wilcox]

On Wed, Nov 04, 2009 at 05:14:26PM -0500, Luis R. Rodriguez wrote:
> On Wed, Nov 04, 2009 at 02:04:11PM -0800, Luis R. Rodriguez wrote:
> > On Wed, Nov 4, 2009 at 2:00 PM, Matthew Wilcox <matthew@wil.cx> wrote:
> > > On Wed, Nov 04, 2009 at 01:52:30PM -0800, Luis R. Rodriguez wrote:
> > >> > Even better: I just confirmation from our systems team that our legacy
> > >> > devices and 11n PCI devices don't support MWR so I'll remove all that
> > >> > cruft crap.
> > >>
> > >> I meant MWI of course.
> > >
> > > Yes, but they don't necessarily just use cacheline size for MWI ... some
> > > devices use cacheline size for setting up data structures. ?Might be
> > > worth just checking explicitly that they don't use the cacheline size
> > > register for anything.
> > 
> > Oh right -- so the typical Atheros hack for this is to check the cache
> > line size, and if its 0 set it to L1_CACHE_BYTES. Then eventually read
> > from PCI_CACHE_LINE_SIZE pci config to align the skb data. So what I
> > was doing now is removing all this cruft and replacing it with a
> > generic allocator for atheros drivers that aligns simply to the
> > L1_CACHE_BYTES. Sound kosher?
> 
> Something like this:

Doesn't look kosher to me.  You're not programming the CLS register
now at all, which means you're relying on something else having set
it up for you.  If you could EXPORT_SYMBOL(pci_set_cacheline_size)
and include a call to it somewhere, that would be good.  You can rely
on pci_cache_line_size not changing after the system has booted.

>  	/*
> -	 * Cache line size is used to size and align various
> -	 * structures used to communicate with the hardware.
> -	 */
> -	pci_read_config_byte(pdev, PCI_CACHE_LINE_SIZE, &csz);
> -	if (csz == 0) {
> -		/*
> -		 * Linux 2.4.18 (at least) writes the cache line size
> -		 * register as a 16-bit wide register which is wrong.
> -		 * We must have this setup properly for rx buffer
> -		 * DMA to work so force a reasonable value here if it
> -		 * comes up zero.
> -		 */
> -		csz = L1_CACHE_BYTES >> 2;
> -		pci_write_config_byte(pdev, PCI_CACHE_LINE_SIZE, csz);
> -	}

These comments are what give me pause.

-- 
Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: pci_set_mwi() and ath5k

[Luis R. Rodriguez]

On Wed, Nov 4, 2009 at 2:29 PM, Matthew Wilcox <matthew@wil.cx> wrote:
> On Wed, Nov 04, 2009 at 05:14:26PM -0500, Luis R. Rodriguez wrote:
>> On Wed, Nov 04, 2009 at 02:04:11PM -0800, Luis R. Rodriguez wrote:
>> > On Wed, Nov 4, 2009 at 2:00 PM, Matthew Wilcox <matthew@wil.cx> wrote:
>> > > On Wed, Nov 04, 2009 at 01:52:30PM -0800, Luis R. Rodriguez wrote:
>> > >> > Even better: I just confirmation from our systems team that our legacy
>> > >> > devices and 11n PCI devices don't support MWR so I'll remove all that
>> > >> > cruft crap.
>> > >>
>> > >> I meant MWI of course.
>> > >
>> > > Yes, but they don't necessarily just use cacheline size for MWI ... some
>> > > devices use cacheline size for setting up data structures. ?Might be
>> > > worth just checking explicitly that they don't use the cacheline size
>> > > register for anything.
>> >
>> > Oh right -- so the typical Atheros hack for this is to check the cache
>> > line size, and if its 0 set it to L1_CACHE_BYTES. Then eventually read
>> > from PCI_CACHE_LINE_SIZE pci config to align the skb data. So what I
>> > was doing now is removing all this cruft and replacing it with a
>> > generic allocator for atheros drivers that aligns simply to the
>> > L1_CACHE_BYTES. Sound kosher?
>>
>> Something like this:
>
> Doesn't look kosher to me.  You're not programming the CLS register
> now at all, which means you're relying on something else having set
> it up for you.  If you could EXPORT_SYMBOL(pci_set_cacheline_size)
> and include a call to it somewhere, that would be good.  You can rely
> on pci_cache_line_size not changing after the system has booted.

I see what you mean but the concern would be *if the hw does use it
for some internal stuff* right?

>>       /*
>> -      * Cache line size is used to size and align various
>> -      * structures used to communicate with the hardware.
>> -      */
>> -     pci_read_config_byte(pdev, PCI_CACHE_LINE_SIZE, &csz);
>> -     if (csz == 0) {
>> -             /*
>> -              * Linux 2.4.18 (at least) writes the cache line size
>> -              * register as a 16-bit wide register which is wrong.
>> -              * We must have this setup properly for rx buffer
>> -              * DMA to work so force a reasonable value here if it
>> -              * comes up zero.
>> -              */
>> -             csz = L1_CACHE_BYTES >> 2;
>> -             pci_write_config_byte(pdev, PCI_CACHE_LINE_SIZE, csz);
>> -     }
>
> These comments are what give me pause.

I see -- OK well I believe the above is in reference to driver code
which uses the written to value on the PCI_CACHE_LINE_SIZE to later
align the skb->data for our RX buffers.

At least our systems team does not believe we rely on the
PCI_CACHE_LINE_SIZE internally on the hardware for anything.

But if there is concern over the doubt of old hardware probably
relying it for some obscure things no one remembers then yeah its best
to leave it huh.

  Luis

Re: pci_set_mwi() and ath5k

[Nick Kossifidis]

2009/11/5 Luis R. Rodriguez <mcgrof@bombadil.infradead.org>:
> On Wed, Nov 04, 2009 at 02:04:11PM -0800, Luis R. Rodriguez wrote:
>> On Wed, Nov 4, 2009 at 2:00 PM, Matthew Wilcox <matthew@wil.cx> wrote:
>> > On Wed, Nov 04, 2009 at 01:52:30PM -0800, Luis R. Rodriguez wrote:
>> >> > Even better: I just confirmation from our systems team that our legacy
>> >> > devices and 11n PCI devices don't support MWR so I'll remove all that
>> >> > cruft crap.
>> >>
>> >> I meant MWI of course.
>> >
>> > Yes, but they don't necessarily just use cacheline size for MWI ... some
>> > devices use cacheline size for setting up data structures.  Might be
>> > worth just checking explicitly that they don't use the cacheline size
>> > register for anything.
>>
>> Oh right -- so the typical Atheros hack for this is to check the cache
>> line size, and if its 0 set it to L1_CACHE_BYTES. Then eventually read
>> from PCI_CACHE_LINE_SIZE pci config to align the skb data. So what I
>> was doing now is removing all this cruft and replacing it with a
>> generic allocator for atheros drivers that aligns simply to the
>> L1_CACHE_BYTES. Sound kosher?
>
> Something like this:
>

According to comments inside MadWiFi AR5210 needs cache line align
else we get corruptions. I don't know if this is correct for all
platforms or later cards but since we (plan to) support AR5210 i guess
we should leave it there. We need to test this a lot on various
archs/cards before applying it.

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

Re: pci_set_mwi() and ath5k

[Luis R. Rodriguez]

On Wed, Nov 4, 2009 at 2:31 PM, Nick Kossifidis <mickflemm@gmail.com> wrote:
> 2009/11/5 Luis R. Rodriguez <mcgrof@bombadil.infradead.org>:
>> On Wed, Nov 04, 2009 at 02:04:11PM -0800, Luis R. Rodriguez wrote:
>>> On Wed, Nov 4, 2009 at 2:00 PM, Matthew Wilcox <matthew@wil.cx> wrote:
>>> > On Wed, Nov 04, 2009 at 01:52:30PM -0800, Luis R. Rodriguez wrote:
>>> >> > Even better: I just confirmation from our systems team that our legacy
>>> >> > devices and 11n PCI devices don't support MWR so I'll remove all that
>>> >> > cruft crap.
>>> >>
>>> >> I meant MWI of course.
>>> >
>>> > Yes, but they don't necessarily just use cacheline size for MWI ... some
>>> > devices use cacheline size for setting up data structures.  Might be
>>> > worth just checking explicitly that they don't use the cacheline size
>>> > register for anything.
>>>
>>> Oh right -- so the typical Atheros hack for this is to check the cache
>>> line size, and if its 0 set it to L1_CACHE_BYTES. Then eventually read
>>> from PCI_CACHE_LINE_SIZE pci config to align the skb data. So what I
>>> was doing now is removing all this cruft and replacing it with a
>>> generic allocator for atheros drivers that aligns simply to the
>>> L1_CACHE_BYTES. Sound kosher?
>>
>> Something like this:
>>
>
> According to comments inside MadWiFi AR5210 needs cache line align
> else we get corruptions.

For what though?

> I don't know if this is correct for all
> platforms or later cards but since we (plan to) support AR5210 i guess
> we should leave it there. We need to test this a lot on various
> archs/cards before applying it.

There are two cases where we can use the PCI_CACHE_LINE_SIZE:

1) Hardware has been designed to use it on some block to align some data somehow
2) Software uses it to align skb->data for performance/hw purposes

I believe the second case can be handled by using L1_CACHE_BYTES
instead and I'd at least like to change our common skb allocator to
use that.

The first case is where it seems there may be some skepticism as to
whether or not hw really did not rely on it and I agree its safer to
keep the programming of the  PCI_CACHE_LINE_SIZE in case it has a
bogus value.

Does this seem reasonable?

  Luis

Re: pci_set_mwi() and ath5k

[Luis R. Rodriguez]

On Wed, Nov 4, 2009 at 2:45 PM, Luis R. Rodriguez <mcgrof@gmail.com> wrote:
> On Wed, Nov 4, 2009 at 2:31 PM, Nick Kossifidis <mickflemm@gmail.com> wrote:
>> 2009/11/5 Luis R. Rodriguez <mcgrof@bombadil.infradead.org>:
>>> On Wed, Nov 04, 2009 at 02:04:11PM -0800, Luis R. Rodriguez wrote:
>>>> On Wed, Nov 4, 2009 at 2:00 PM, Matthew Wilcox <matthew@wil.cx> wrote:
>>>> > On Wed, Nov 04, 2009 at 01:52:30PM -0800, Luis R. Rodriguez wrote:
>>>> >> > Even better: I just confirmation from our systems team that our legacy
>>>> >> > devices and 11n PCI devices don't support MWR so I'll remove all that
>>>> >> > cruft crap.
>>>> >>
>>>> >> I meant MWI of course.
>>>> >
>>>> > Yes, but they don't necessarily just use cacheline size for MWI ... some
>>>> > devices use cacheline size for setting up data structures.  Might be
>>>> > worth just checking explicitly that they don't use the cacheline size
>>>> > register for anything.
>>>>
>>>> Oh right -- so the typical Atheros hack for this is to check the cache
>>>> line size, and if its 0 set it to L1_CACHE_BYTES. Then eventually read
>>>> from PCI_CACHE_LINE_SIZE pci config to align the skb data. So what I
>>>> was doing now is removing all this cruft and replacing it with a
>>>> generic allocator for atheros drivers that aligns simply to the
>>>> L1_CACHE_BYTES. Sound kosher?
>>>
>>> Something like this:
>>>
>>
>> According to comments inside MadWiFi AR5210 needs cache line align
>> else we get corruptions.
>
> For what though?
>
>> I don't know if this is correct for all
>> platforms or later cards but since we (plan to) support AR5210 i guess
>> we should leave it there. We need to test this a lot on various
>> archs/cards before applying it.
>
> There are two cases where we can use the PCI_CACHE_LINE_SIZE:
>
> 1) Hardware has been designed to use it on some block to align some data somehow
> 2) Software uses it to align skb->data for performance/hw purposes
>
> I believe the second case can be handled by using L1_CACHE_BYTES
> instead and I'd at least like to change our common skb allocator to
> use that.
>
> The first case is where it seems there may be some skepticism as to
> whether or not hw really did not rely on it and I agree its safer to
> keep the programming of the  PCI_CACHE_LINE_SIZE in case it has a
> bogus value.
>
> Does this seem reasonable?

I guess we can keep the second case allocator which relies on
PCI_CACHE_LINE_SIZE, but if so then USB will just define its own csz
statically always to L1_CACHE_BYTES which is fine too -- I was just
hoping we could remove the PCI_CACHE_LINE_SIZE programming completely
too and use a simple L1_CACHE_BYTES aligned allocator.

  Luis

Re: pci_set_mwi() and ath5k

[Luis R. Rodriguez]

On Wed, Nov 4, 2009 at 2:47 PM, Luis R. Rodriguez <mcgrof@gmail.com> wrote:
> On Wed, Nov 4, 2009 at 2:45 PM, Luis R. Rodriguez <mcgrof@gmail.com> wrote:
>> On Wed, Nov 4, 2009 at 2:31 PM, Nick Kossifidis <mickflemm@gmail.com> wrote:
>>> 2009/11/5 Luis R. Rodriguez <mcgrof@bombadil.infradead.org>:
>>>> On Wed, Nov 04, 2009 at 02:04:11PM -0800, Luis R. Rodriguez wrote:
>>>>> On Wed, Nov 4, 2009 at 2:00 PM, Matthew Wilcox <matthew@wil.cx> wrote:
>>>>> > On Wed, Nov 04, 2009 at 01:52:30PM -0800, Luis R. Rodriguez wrote:
>>>>> >> > Even better: I just confirmation from our systems team that our legacy
>>>>> >> > devices and 11n PCI devices don't support MWR so I'll remove all that
>>>>> >> > cruft crap.
>>>>> >>
>>>>> >> I meant MWI of course.
>>>>> >
>>>>> > Yes, but they don't necessarily just use cacheline size for MWI ... some
>>>>> > devices use cacheline size for setting up data structures.  Might be
>>>>> > worth just checking explicitly that they don't use the cacheline size
>>>>> > register for anything.
>>>>>
>>>>> Oh right -- so the typical Atheros hack for this is to check the cache
>>>>> line size, and if its 0 set it to L1_CACHE_BYTES. Then eventually read
>>>>> from PCI_CACHE_LINE_SIZE pci config to align the skb data. So what I
>>>>> was doing now is removing all this cruft and replacing it with a
>>>>> generic allocator for atheros drivers that aligns simply to the
>>>>> L1_CACHE_BYTES. Sound kosher?
>>>>
>>>> Something like this:
>>>>
>>>
>>> According to comments inside MadWiFi AR5210 needs cache line align
>>> else we get corruptions.
>>
>> For what though?
>>
>>> I don't know if this is correct for all
>>> platforms or later cards but since we (plan to) support AR5210 i guess
>>> we should leave it there. We need to test this a lot on various
>>> archs/cards before applying it.
>>
>> There are two cases where we can use the PCI_CACHE_LINE_SIZE:
>>
>> 1) Hardware has been designed to use it on some block to align some data somehow
>> 2) Software uses it to align skb->data for performance/hw purposes
>>
>> I believe the second case can be handled by using L1_CACHE_BYTES
>> instead and I'd at least like to change our common skb allocator to
>> use that.
>>
>> The first case is where it seems there may be some skepticism as to
>> whether or not hw really did not rely on it and I agree its safer to
>> keep the programming of the  PCI_CACHE_LINE_SIZE in case it has a
>> bogus value.
>>
>> Does this seem reasonable?
>
> I guess we can keep the second case allocator which relies on
> PCI_CACHE_LINE_SIZE, but if so then USB will just define its own csz
> statically always to L1_CACHE_BYTES which is fine too -- I was just
> hoping we could remove the PCI_CACHE_LINE_SIZE programming completely
> too and use a simple L1_CACHE_BYTES aligned allocator.

OK I'll leave all this crap and just annotate why its there. Right now
its just voodoo.

  Luis

Re: pci_set_mwi() and ath5k

[Nick Kossifidis]

2009/11/5 Luis R. Rodriguez <mcgrof@gmail.com>:
>
> There are two cases where we can use the PCI_CACHE_LINE_SIZE:
>
> 1) Hardware has been designed to use it on some block to align some data somehow
> 2) Software uses it to align skb->data for performance/hw purposes
>
> I believe the second case can be handled by using L1_CACHE_BYTES
> instead and I'd at least like to change our common skb allocator to
> use that.
>
> The first case is where it seems there may be some skepticism as to
> whether or not hw really did not rely on it and I agree its safer to
> keep the programming of the  PCI_CACHE_LINE_SIZE in case it has a
> bogus value.
>
> Does this seem reasonable?
>
>  Luis
>

Yup, i believe 2 is the case, from what i've read the problem is that
we get corrupted data so i guess we can just use the L1_CACHE_BYTES to
align skb->data and not write anything to PCI_CACHE_LINE_SIZE if you
think that will work. Just be sure to test on some ath5k cards (don't
have time for anything right now ;-( ). I believe having cleaner code
is better than supporting an ancient chip anyway, if this is really
necessary for AR5210 -writing PCI_CACHE_LINE_SIZE- we can deal with it
later.

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

Re: pci_set_mwi() and ath5k

[Bob Copeland]

On Wed, Nov 4, 2009 at 5:14 PM, Luis R. Rodriguez
<mcgrof@bombadil.infradead.org> wrote:
> -               off = ((unsigned long) skb->data) % common->cachelsz;
> +               off = ((unsigned long) skb->data) % L1_CACHE_BYTES;
>                if (off != 0)

Side note, at least this needs to use PTR_ALIGN :)

I have a patch somewhere to do that but if you're in there anyway...

-- 
Bob Copeland %% www.bobcopeland.com