John, Sujith pointed out it would be best to have the ioread/iowrite
serialized routines as inline, here is the new batch based on that.
I had tried a macro but that is just too ugly..
I figured its best to revert instead of patching on top. Sorry for any
trouble.
It requires revert of the last serialization series:
3149fde1bf083d32d5190cdc7f2bf8d0035966b1
ath9k: remove dummy PCI "retry timeout" fix
b214ce676b438434a32d8e3dbddb28639ffb56a1
ath9k: add cpu notifier to enhance device configuration
c006bbb7932b3f6594fcbf56fec0552fe51d4386
ath9k: AR9280 PCI devices must serialize IO as well
cd70622da755f37534e72d289fca6b98347aff65
ath9k: implement IO serialization
Luis R. Rodriguez (4):
ath9k: implement IO serialization
ath9k: AR9280 PCI devices must serialize IO as well
ath9k: add cpu notifier to enhance device configuration
ath9k: remove dummy PCI "retry timeout" fix
drivers/net/wireless/ath9k/ath9k.h | 41 ++++++++++++++++++++++++++++++++++++
drivers/net/wireless/ath9k/hw.c | 41 +++++++++++++++++++++++++++++++++++-
drivers/net/wireless/ath9k/hw.h | 9 ++++++-
drivers/net/wireless/ath9k/main.c | 30 ++++++++++++++++++++++++++
drivers/net/wireless/ath9k/pci.c | 22 +++---------------
5 files changed, 122 insertions(+), 21 deletions(-)
2009/3/11 Johannes Berg <[email protected]>:
> On Wed, 2009-03-11 at 09:07 -0700, Luis R. Rodriguez wrote:
>
>> > This is utterly insane. Just make it _always_ do the spinlock, and get
>> > rid of the hotplug notifier and all that crap.
>>
>> That is certainly an option as well but we would obviously be incurring
>> the lock on all reads/writes regardless of what bus you use which is
>> not necessary. We'll review this and determine whether its worth it.
>
> Hmm. I really don't understand this at all. Most operations won't be
> single writes and reads, and if you need multiple like a write+read for
> indirect register accesses then I'm sure you need to serialise them
> against each other in some other way too, no?
The issue is not serializing against separate routines calling some
reads or writes, this serialization prevents the PCI hardware FIFO
queue from getting more than two requests as otherwise the hardware
poops out. But as you can see this is only an issue with PCI devices,
not PCI express devices.
Luis
Cc: [email protected]
Signed-off-by: Luis R. Rodriguez <[email protected]>
Signed-off-by: John W. Linville <[email protected]>
---
drivers/net/wireless/ath9k/hw.c | 9 ++++++++-
1 files changed, 8 insertions(+), 1 deletions(-)
diff --git a/drivers/net/wireless/ath9k/hw.c b/drivers/net/wireless/ath9k/hw.c
index 50a44a0..51d3a4e 100644
--- a/drivers/net/wireless/ath9k/hw.c
+++ b/drivers/net/wireless/ath9k/hw.c
@@ -628,8 +628,15 @@ static struct ath_hw *ath9k_hw_do_attach(u16 devid, struct ath_softc *sc,
goto bad;
}
+ /*
+ * All PCI devices should be put here.
+ * XXX: remove ah->is_pciexpress and use pdev->is_pcie, then
+ * we can just check for !pdev->is_pcie here, but
+ * consideration must be taken for handling AHB as well.
+ */
if (ah->config.serialize_regmode == SER_REG_MODE_AUTO) {
- if (ah->hw_version.macVersion == AR_SREV_VERSION_5416_PCI) {
+ if (ah->hw_version.macVersion == AR_SREV_VERSION_5416_PCI ||
+ (AR_SREV_9280(ah) && !ah->is_pciexpress)) {
ah->config.serialize_regmode =
SER_REG_MODE_ON;
} else {
--
1.6.0.6
Upon CPU Hotplug the number of CPUs can change and we can tweak
our driver configuration as such. Keep in mind CPU hotplug also
occurs during suspend/resume.
Since we have a notifier now we can rely on num_present_cpus()
rather than num_possible_cpus() to enable/disable serialization.
Signed-off-by: Luis R. Rodriguez <[email protected]>
Signed-off-by: John W. Linville <[email protected]>
---
drivers/net/wireless/ath9k/ath9k.h | 7 +++++
drivers/net/wireless/ath9k/hw.c | 49 ++++++++++++++++++++++-------------
drivers/net/wireless/ath9k/hw.h | 3 ++
drivers/net/wireless/ath9k/main.c | 29 +++++++++++++++++++++
drivers/net/wireless/ath9k/pci.c | 4 +++
5 files changed, 74 insertions(+), 18 deletions(-)
diff --git a/drivers/net/wireless/ath9k/ath9k.h b/drivers/net/wireless/ath9k/ath9k.h
index 050304e..bcd4d3f 100644
--- a/drivers/net/wireless/ath9k/ath9k.h
+++ b/drivers/net/wireless/ath9k/ath9k.h
@@ -22,6 +22,7 @@
#include <net/mac80211.h>
#include <linux/leds.h>
#include <linux/rfkill.h>
+#include <linux/cpu.h>
#include "hw.h"
#include "rc.h"
@@ -554,6 +555,9 @@ struct ath_bus_ops {
struct ath_wiphy;
+int ath9k_cpu_callback(struct notifier_block *nfb,
+ unsigned long action, void *hcpu);
+
struct ath_softc {
struct ieee80211_hw *hw;
struct device *dev;
@@ -573,6 +577,9 @@ struct ath_softc {
unsigned long wiphy_scheduler_int;
int wiphy_scheduler_index;
+ /* This should or _needs_ to be __cpuinit (?) */
+ struct notifier_block cpu_notifer;
+
struct tasklet_struct intr_tq;
struct tasklet_struct bcon_tasklet;
struct ath_hw *sc_ah;
diff --git a/drivers/net/wireless/ath9k/hw.c b/drivers/net/wireless/ath9k/hw.c
index 51d3a4e..669cc46 100644
--- a/drivers/net/wireless/ath9k/hw.c
+++ b/drivers/net/wireless/ath9k/hw.c
@@ -16,6 +16,7 @@
#include <linux/io.h>
#include <asm/unaligned.h>
+#include <linux/cpu.h>
#include "ath9k.h"
#include "initvals.h"
@@ -353,6 +354,35 @@ static const char *ath9k_hw_devname(u16 devid)
return NULL;
}
+/*
+ * We can slap on to here things we need to tune depending
+ * on the number of CPUs. This will happen on CPU hotplug
+ * which is also used for suspend/resume.
+ */
+void ath9k_hw_config_for_cpus(struct ath_hw *ah)
+{
+ /*
+ * We need this for PCI devices only (Cardbus, PCI, miniPCI)
+ * _and_ if on non-uniprocessor systems (Multiprocessor/HT).
+ * This means we use it for all AR5416 devices, and the few
+ * minor PCI AR9280 devices out there.
+ *
+ * Serialization is required because these devices do not handle
+ * well the case of two concurrent reads/writes due to the latency
+ * involved. During one read/write another read/write can be issued
+ * on another CPU while the previous read/write may still be working
+ * on our hardware, if we hit this case the hardware poops in a loop.
+ * We prevent this by serializing reads and writes.
+ *
+ * This issue is not present on PCI-Express devices or pre-AR5416
+ * devices (legacy, 802.11abg).
+ */
+ if (num_present_cpus() > 1)
+ ah->config.serialize_regmode = SER_REG_MODE_AUTO;
+ else
+ ah->config.serialize_regmode = SER_REG_MODE_OFF;
+}
+
static void ath9k_hw_set_defaults(struct ath_hw *ah)
{
int i;
@@ -392,24 +422,7 @@ static void ath9k_hw_set_defaults(struct ath_hw *ah)
ah->config.intr_mitigation = 1;
- /*
- * We need this for PCI devices only (Cardbus, PCI, miniPCI)
- * _and_ if on non-uniprocessor systems (Multiprocessor/HT).
- * This means we use it for all AR5416 devices, and the few
- * minor PCI AR9280 devices out there.
- *
- * Serialization is required because these devices do not handle
- * well the case of two concurrent reads/writes due to the latency
- * involved. During one read/write another read/write can be issued
- * on another CPU while the previous read/write may still be working
- * on our hardware, if we hit this case the hardware poops in a loop.
- * We prevent this by serializing reads and writes.
- *
- * This issue is not present on PCI-Express devices or pre-AR5416
- * devices (legacy, 802.11abg).
- */
- if (num_possible_cpus() > 1)
- ah->config.serialize_regmode = SER_REG_MODE_AUTO;
+ ath9k_hw_config_for_cpus(ah);
}
static struct ath_hw *ath9k_hw_newstate(u16 devid, struct ath_softc *sc,
diff --git a/drivers/net/wireless/ath9k/hw.h b/drivers/net/wireless/ath9k/hw.h
index 43de66d..5262a21 100644
--- a/drivers/net/wireless/ath9k/hw.h
+++ b/drivers/net/wireless/ath9k/hw.h
@@ -568,6 +568,9 @@ struct ath_hw {
struct ar5416IniArray iniModesTxGain;
};
+/* Lets us tweak the device per CPU changes */
+void ath9k_hw_config_for_cpus(struct ath_hw *ah);
+
/* Attach, Detach, Reset */
const char *ath9k_hw_probe(u16 vendorid, u16 devid);
void ath9k_hw_detach(struct ath_hw *ah);
diff --git a/drivers/net/wireless/ath9k/main.c b/drivers/net/wireless/ath9k/main.c
index 8db75f6..a8cdc51 100644
--- a/drivers/net/wireless/ath9k/main.c
+++ b/drivers/net/wireless/ath9k/main.c
@@ -2788,6 +2788,35 @@ struct ieee80211_ops ath9k_ops = {
.sw_scan_complete = ath9k_sw_scan_complete,
};
+int ath9k_cpu_callback(struct notifier_block *nfb,
+ unsigned long action, void *hcpu)
+{
+ struct ath_softc *sc = container_of(nfb, struct ath_softc, cpu_notifer);
+ int old_serial_mode;
+
+ old_serial_mode = sc->sc_ah->config.serialize_regmode;
+
+ switch (action) {
+ case CPU_ONLINE:
+ case CPU_ONLINE_FROZEN:
+ ath9k_hw_config_for_cpus(sc->sc_ah);
+ break;
+ case CPU_DEAD:
+ case CPU_DEAD_FROZEN:
+ ath9k_hw_config_for_cpus(sc->sc_ah);
+ break;
+ }
+
+ if (unlikely(old_serial_mode != sc->sc_ah->config.serialize_regmode)) {
+ DPRINTF(sc, ATH_DBG_RESET,
+ "serialize_regmode has changed due to CPU "
+ "count to %d\n",
+ sc->sc_ah->config.serialize_regmode);
+ }
+
+ return NOTIFY_OK;
+}
+
static struct {
u32 version;
const char * name;
diff --git a/drivers/net/wireless/ath9k/pci.c b/drivers/net/wireless/ath9k/pci.c
index 9a58baa..c1ba207 100644
--- a/drivers/net/wireless/ath9k/pci.c
+++ b/drivers/net/wireless/ath9k/pci.c
@@ -181,6 +181,9 @@ static int ath_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
goto bad3;
}
+ sc->cpu_notifer.notifier_call = ath9k_cpu_callback;
+ register_hotcpu_notifier(&sc->cpu_notifer);
+
/* setup interrupt service routine */
if (request_irq(pdev->irq, ath_isr, IRQF_SHARED, "ath", sc)) {
@@ -223,6 +226,7 @@ static void ath_pci_remove(struct pci_dev *pdev)
struct ath_wiphy *aphy = hw->priv;
struct ath_softc *sc = aphy->sc;
+ unregister_hotcpu_notifier(&sc->cpu_notifer);
ath_cleanup(sc);
}
--
1.6.0.6
On Wed, Mar 11, 2009 at 02:10:11AM -0700, Johannes Berg wrote:
> On Tue, 2009-03-10 at 22:52 -0400, Luis R. Rodriguez wrote:
>
> > + if (ah->config.serialize_regmode == SER_REG_MODE_ON) {
> > + unsigned long flags;
> > + spin_lock_irqsave(&ah->ah_sc->sc_serial_rw, flags);
> > + iowrite32(val, ah->ah_sc->mem + reg_offset);
> > + spin_unlock_irqrestore(&ah->ah_sc->sc_serial_rw, flags);
> > + } else
> > + iowrite32(val, ah->ah_sc->mem + reg_offset);
>
> This is utterly insane. Just make it _always_ do the spinlock, and get
> rid of the hotplug notifier and all that crap.
That is certainly an option as well but we would obviously be incurring
the lock on all reads/writes regardless of what bus you use which is
not necessary. We'll review this and determine whether its worth it.
Luis
All 802.11n PCI devices (Cardbus, PCI, mini-PCI) require
serialization of IO when on non-uniprocessor systems. PCI
express devices not not require this.
This should fix our only last standing open ath9k kernel.org
bugzilla bug report:
http://bugzilla.kernel.org/show_bug.cgi?id=12110
A port is probably required to older kernels and I can work on
that.
Tested-by: Adel Gadllah <[email protected]>
Signed-off-by: Luis R. Rodriguez <[email protected]>
Signed-off-by: John W. Linville <[email protected]>
---
drivers/net/wireless/ath9k/ath9k.h | 34 ++++++++++++++++++++++++++++++++++
drivers/net/wireless/ath9k/hw.c | 19 +++++++++++++++++++
drivers/net/wireless/ath9k/hw.h | 6 ++++--
drivers/net/wireless/ath9k/main.c | 1 +
4 files changed, 58 insertions(+), 2 deletions(-)
diff --git a/drivers/net/wireless/ath9k/ath9k.h b/drivers/net/wireless/ath9k/ath9k.h
index f0b105a..050304e 100644
--- a/drivers/net/wireless/ath9k/ath9k.h
+++ b/drivers/net/wireless/ath9k/ath9k.h
@@ -579,6 +579,7 @@ struct ath_softc {
void __iomem *mem;
int irq;
spinlock_t sc_resetlock;
+ spinlock_t sc_serial_rw;
struct mutex mutex;
u8 curbssid[ETH_ALEN];
@@ -724,4 +725,37 @@ void ath9k_wiphy_pause_all_forced(struct ath_softc *sc,
bool ath9k_wiphy_scanning(struct ath_softc *sc);
void ath9k_wiphy_work(struct work_struct *work);
+/*
+ * Read and write, they both share the same lock. We do this to serialize
+ * reads and writes on Atheros 802.11n PCI devices only. This is required
+ * as the FIFO on these devices can only accept sanely 2 requests. After
+ * that the device goes bananas. Serializing the reads/writes prevents this
+ * from happening.
+ */
+
+static inline void ath9k_iowrite32(struct ath_hw *ah, u32 reg_offset, u32 val)
+{
+ if (ah->config.serialize_regmode == SER_REG_MODE_ON) {
+ unsigned long flags;
+ spin_lock_irqsave(&ah->ah_sc->sc_serial_rw, flags);
+ iowrite32(val, ah->ah_sc->mem + reg_offset);
+ spin_unlock_irqrestore(&ah->ah_sc->sc_serial_rw, flags);
+ } else
+ iowrite32(val, ah->ah_sc->mem + reg_offset);
+}
+
+static inline unsigned int ath9k_ioread32(struct ath_hw *ah, u32 reg_offset)
+{
+ u32 val;
+ if (ah->config.serialize_regmode == SER_REG_MODE_ON) {
+ unsigned long flags;
+ spin_lock_irqsave(&ah->ah_sc->sc_serial_rw, flags);
+ val = ioread32(ah->ah_sc->mem + reg_offset);
+ spin_unlock_irqrestore(&ah->ah_sc->sc_serial_rw, flags);
+ } else
+ val = ioread32(ah->ah_sc->mem + reg_offset);
+ return val;
+}
+
+
#endif /* ATH9K_H */
diff --git a/drivers/net/wireless/ath9k/hw.c b/drivers/net/wireless/ath9k/hw.c
index ea550cc..50a44a0 100644
--- a/drivers/net/wireless/ath9k/hw.c
+++ b/drivers/net/wireless/ath9k/hw.c
@@ -391,6 +391,25 @@ static void ath9k_hw_set_defaults(struct ath_hw *ah)
}
ah->config.intr_mitigation = 1;
+
+ /*
+ * We need this for PCI devices only (Cardbus, PCI, miniPCI)
+ * _and_ if on non-uniprocessor systems (Multiprocessor/HT).
+ * This means we use it for all AR5416 devices, and the few
+ * minor PCI AR9280 devices out there.
+ *
+ * Serialization is required because these devices do not handle
+ * well the case of two concurrent reads/writes due to the latency
+ * involved. During one read/write another read/write can be issued
+ * on another CPU while the previous read/write may still be working
+ * on our hardware, if we hit this case the hardware poops in a loop.
+ * We prevent this by serializing reads and writes.
+ *
+ * This issue is not present on PCI-Express devices or pre-AR5416
+ * devices (legacy, 802.11abg).
+ */
+ if (num_possible_cpus() > 1)
+ ah->config.serialize_regmode = SER_REG_MODE_AUTO;
}
static struct ath_hw *ath9k_hw_newstate(u16 devid, struct ath_softc *sc,
diff --git a/drivers/net/wireless/ath9k/hw.h b/drivers/net/wireless/ath9k/hw.h
index 89936a0..43de66d 100644
--- a/drivers/net/wireless/ath9k/hw.h
+++ b/drivers/net/wireless/ath9k/hw.h
@@ -20,6 +20,7 @@
#include <linux/if_ether.h>
#include <linux/delay.h>
#include <linux/io.h>
+#include <linux/spinlock.h>
#include "mac.h"
#include "ani.h"
@@ -28,6 +29,7 @@
#include "regd.h"
#include "reg.h"
#include "phy.h"
+#include "ath9k.h"
#define ATHEROS_VENDOR_ID 0x168c
#define AR5416_DEVID_PCI 0x0023
@@ -42,8 +44,8 @@
#define AR5416_MAGIC 0x19641014
/* Register read/write primitives */
-#define REG_WRITE(_ah, _reg, _val) iowrite32(_val, _ah->ah_sc->mem + _reg)
-#define REG_READ(_ah, _reg) ioread32(_ah->ah_sc->mem + _reg)
+#define REG_WRITE(_ah, _reg, _val) ath9k_iowrite32((_ah), (_reg), (_val))
+#define REG_READ(_ah, _reg) ath9k_ioread32((_ah), (_reg))
#define SM(_v, _f) (((_v) << _f##_S) & _f)
#define MS(_v, _f) (((_v) & _f) >> _f##_S)
diff --git a/drivers/net/wireless/ath9k/main.c b/drivers/net/wireless/ath9k/main.c
index a2f5af6..8db75f6 100644
--- a/drivers/net/wireless/ath9k/main.c
+++ b/drivers/net/wireless/ath9k/main.c
@@ -1370,6 +1370,7 @@ static int ath_init(u16 devid, struct ath_softc *sc)
spin_lock_init(&sc->wiphy_lock);
spin_lock_init(&sc->sc_resetlock);
+ spin_lock_init(&sc->sc_serial_rw);
mutex_init(&sc->mutex);
tasklet_init(&sc->intr_tq, ath9k_tasklet, (unsigned long)sc);
tasklet_init(&sc->bcon_tasklet, ath_beacon_tasklet,
--
1.6.0.6
On Wed, 2009-03-11 at 09:07 -0700, Luis R. Rodriguez wrote:
> > This is utterly insane. Just make it _always_ do the spinlock, and get
> > rid of the hotplug notifier and all that crap.
>
> That is certainly an option as well but we would obviously be incurring
> the lock on all reads/writes regardless of what bus you use which is
> not necessary. We'll review this and determine whether its worth it.
Hmm. I really don't understand this at all. Most operations won't be
single writes and reads, and if you need multiple like a write+read for
indirect register accesses then I'm sure you need to serialise them
against each other in some other way too, no?
johannes
Remove the PCI retry timeout code as that was just taken from ipw2100
due to historical reasons but in reality its a no-op, additionally its
simply incorrect as each PCI devices has its own custom PCI configuration
space on PCI config space >= 0x40. Not to mention we were trying to write
0 to a place that already has 0 on it.
Cc: Matthew Garrett <[email protected]>
Cc: Ben Cahill <[email protected]>
Cc: Inaky Perez-Gonzalez <[email protected]>
Tested-by: Adel Gadllah <[email protected]>
Signed-off-by: Luis R. Rodriguez <[email protected]>
Signed-off-by: John W. Linville <[email protected]>
---
drivers/net/wireless/ath9k/pci.c | 18 ------------------
1 files changed, 0 insertions(+), 18 deletions(-)
diff --git a/drivers/net/wireless/ath9k/pci.c b/drivers/net/wireless/ath9k/pci.c
index c1ba207..a1a9b90 100644
--- a/drivers/net/wireless/ath9k/pci.c
+++ b/drivers/net/wireless/ath9k/pci.c
@@ -87,7 +87,6 @@ static int ath_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
struct ath_softc *sc;
struct ieee80211_hw *hw;
u8 csz;
- u32 val;
int ret = 0;
struct ath_hw *ah;
@@ -134,14 +133,6 @@ static int ath_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
pci_set_master(pdev);
- /*
- * Disable the RETRY_TIMEOUT register (0x41) to keep
- * PCI Tx retries from interfering with C3 CPU state.
- */
- pci_read_config_dword(pdev, 0x40, &val);
- if ((val & 0x0000ff00) != 0)
- pci_write_config_dword(pdev, 0x40, val & 0xffff00ff);
-
ret = pci_request_region(pdev, 0, "ath9k");
if (ret) {
dev_err(&pdev->dev, "PCI memory region reserve error\n");
@@ -257,21 +248,12 @@ static int ath_pci_resume(struct pci_dev *pdev)
struct ieee80211_hw *hw = pci_get_drvdata(pdev);
struct ath_wiphy *aphy = hw->priv;
struct ath_softc *sc = aphy->sc;
- u32 val;
int err;
err = pci_enable_device(pdev);
if (err)
return err;
pci_restore_state(pdev);
- /*
- * Suspend/Resume resets the PCI configuration space, so we have to
- * re-disable the RETRY_TIMEOUT register (0x41) to keep
- * PCI Tx retries from interfering with C3 CPU state
- */
- pci_read_config_dword(pdev, 0x40, &val);
- if ((val & 0x0000ff00) != 0)
- pci_write_config_dword(pdev, 0x40, val & 0xffff00ff);
/* Enable LED */
ath9k_hw_cfg_output(sc->sc_ah, ATH_LED_PIN,
--
1.6.0.6
On Tue, 2009-03-10 at 22:52 -0400, Luis R. Rodriguez wrote:
> + if (ah->config.serialize_regmode == SER_REG_MODE_ON) {
> + unsigned long flags;
> + spin_lock_irqsave(&ah->ah_sc->sc_serial_rw, flags);
> + iowrite32(val, ah->ah_sc->mem + reg_offset);
> + spin_unlock_irqrestore(&ah->ah_sc->sc_serial_rw, flags);
> + } else
> + iowrite32(val, ah->ah_sc->mem + reg_offset);
This is utterly insane. Just make it _always_ do the spinlock, and get
rid of the hotplug notifier and all that crap.
johannes
On Wed, Mar 11, 2009 at 10:30 AM, Johannes Berg
<[email protected]> wrote:
> On Wed, 2009-03-11 at 10:20 -0700, Luis R. Rodriguez wrote:
>
>> > Hmm. I really don't understand this at all. Most operations won't be
>> > single writes and reads, and if you need multiple like a write+read for
>> > indirect register accesses then I'm sure you need to serialise them
>> > against each other in some other way too, no?
>>
>> The issue is not serializing against separate routines calling some
>> reads or writes, this serialization prevents the PCI hardware FIFO
>> queue from getting more than two requests as otherwise the hardware
>> poops out. But as you can see this is only an issue with PCI devices,
>> not PCI express devices.
>
> Well yes, but if you ever do multi-read/write operations then you need
> to synchronise at a higher level anyway -- hence I don't quite
> understand why this is necessary at all.
Agreed as the PCI FIFO issue should be visible on even UP boxes too. I
was trying to find out out exactly why in an SMP system this would
occur and it seems the only thing that would cause this is two
separate threads reading/writing. I did review this but I was unable
to find a specific synchronization issue. Technically the most obvious
case is where we handle the beacon tasklet on one CPU and then the
RX/TX tasklet (this is done in just one tasklet) in another but we
don't handle the beaconing tasklet in STA mode and this issue is seen
immediately upon association as a STA. I've been unable to locate that
source of possible synchronization issue. If you get an idea let me
know.
Your point about the simplicity of the code by always doing the
locking makes sense and its difficult to decide whether or not we want
that without doing elaborate tests and analysis. Only reason to
consider this carefully is that we will in fact be changing the
behavior of each read/write.
Luis
On Wed, Mar 11, 2009 at 02:38:24PM -0700, Christian Lamparter wrote:
> On Wednesday 11 March 2009 22:08:43 Luis R. Rodriguez wrote:
> > On Wed, Mar 11, 2009 at 10:30 AM, Johannes Berg
> > <[email protected]> wrote:
> > > On Wed, 2009-03-11 at 10:20 -0700, Luis R. Rodriguez wrote:
> > >
> > >> > Hmm. I really don't understand this at all. Most operations won't be
> > >> > single writes and reads, and if you need multiple like a write+read for
> > >> > indirect register accesses then I'm sure you need to serialise them
> > >> > against each other in some other way too, no?
> > >>
> > >> The issue is not serializing against separate routines calling some
> > >> reads or writes, this serialization prevents the PCI hardware FIFO
> > >> queue from getting more than two requests as otherwise the hardware
> > >> poops out. But as you can see this is only an issue with PCI devices,
> > >> not PCI express devices.
> > >
> > > Well yes, but if you ever do multi-read/write operations then you need
> > > to synchronise at a higher level anyway -- hence I don't quite
> > > understand why this is necessary at all.
> >
> > Agreed as the PCI FIFO issue should be visible on even UP boxes too. I
> > was trying to find out out exactly why in an SMP system this would
> > occur and it seems the only thing that would cause this is two
> > separate threads reading/writing. I did review this but I was unable
> > to find a specific synchronization issue. Technically the most obvious
> > case is where we handle the beacon tasklet on one CPU and then the
> > RX/TX tasklet (this is done in just one tasklet) in another but we
> > don't handle the beaconing tasklet in STA mode and this issue is seen
> > immediately upon association as a STA. I've been unable to locate that
> > source of possible synchronization issue. If you get an idea let me
> > know.
>
> maybe it's down to the simple fact that most mainboard nowadays
> queue up pci writes?
The issue is not the PCI host controller queuing these up, the issue
is actually having the PCI host controller send too many requests.
For example when the device is still working on one PCI write and its
own device queue is already full to the capicity it supports (say 2)
and it tells the PCI bus to retry if the device is taking a little
longer than expected to do some specific operation and the queue is
still full upon the next request it will poo. An alternative I was
looking for was to increase a udelay() somewhere guessing that perhaps
that would cure this as well. This could potentiall work as well but
I haven't found that spot yet or know for sure if any does exist for
sure.
> what happends when you always read the
> register after it was updated?
Not sure what you mean. Keep in mind some registers do not keep the
data we write to it.
Luis
On Wednesday 11 March 2009 22:08:43 Luis R. Rodriguez wrote:
> On Wed, Mar 11, 2009 at 10:30 AM, Johannes Berg
> <[email protected]> wrote:
> > On Wed, 2009-03-11 at 10:20 -0700, Luis R. Rodriguez wrote:
> >
> >> > Hmm. I really don't understand this at all. Most operations won't be
> >> > single writes and reads, and if you need multiple like a write+read for
> >> > indirect register accesses then I'm sure you need to serialise them
> >> > against each other in some other way too, no?
> >>
> >> The issue is not serializing against separate routines calling some
> >> reads or writes, this serialization prevents the PCI hardware FIFO
> >> queue from getting more than two requests as otherwise the hardware
> >> poops out. But as you can see this is only an issue with PCI devices,
> >> not PCI express devices.
> >
> > Well yes, but if you ever do multi-read/write operations then you need
> > to synchronise at a higher level anyway -- hence I don't quite
> > understand why this is necessary at all.
>
> Agreed as the PCI FIFO issue should be visible on even UP boxes too. I
> was trying to find out out exactly why in an SMP system this would
> occur and it seems the only thing that would cause this is two
> separate threads reading/writing. I did review this but I was unable
> to find a specific synchronization issue. Technically the most obvious
> case is where we handle the beacon tasklet on one CPU and then the
> RX/TX tasklet (this is done in just one tasklet) in another but we
> don't handle the beaconing tasklet in STA mode and this issue is seen
> immediately upon association as a STA. I've been unable to locate that
> source of possible synchronization issue. If you get an idea let me
> know.
maybe it's down to the simple fact that most mainboard nowadays
queue up pci writes? what happends when you always read the
register after it was updated?
> Your point about the simplicity of the code by always doing the
> locking makes sense and its difficult to decide whether or not we want
> that without doing elaborate tests and analysis. Only reason to
> consider this carefully is that we will in fact be changing the
> behavior of each read/write.
>
On Wed, 2009-03-11 at 10:20 -0700, Luis R. Rodriguez wrote:
> > Hmm. I really don't understand this at all. Most operations won't be
> > single writes and reads, and if you need multiple like a write+read for
> > indirect register accesses then I'm sure you need to serialise them
> > against each other in some other way too, no?
>
> The issue is not serializing against separate routines calling some
> reads or writes, this serialization prevents the PCI hardware FIFO
> queue from getting more than two requests as otherwise the hardware
> poops out. But as you can see this is only an issue with PCI devices,
> not PCI express devices.
Well yes, but if you ever do multi-read/write operations then you need
to synchronise at a higher level anyway -- hence I don't quite
understand why this is necessary at all.
johannes