diff mbox

[v2] ath10k: Replace ioread with mb to drain write buffer

Message ID 7a87e5df81499e4d26a4f8bedf76ed3250a6f7bb.1422663244.git.poh@qca.qualcomm.com (mailing list archive)
State Not Applicable
Headers show

Commit Message

Peter Oh Jan. 31, 2015, 12:14 a.m. UTC
Using ioread() to perform draining write buffer is excessive.
Use compact API, mb(), that intended to be used for the case.
It reduces total 14 CPU clocks per interrupt.

Signed-off-by: Peter Oh <poh@qca.qualcomm.com>
---
 drivers/net/wireless/ath/ath10k/pci.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

Comments

Jakub Kici?ski Feb. 6, 2015, 8:58 a.m. UTC | #1
On Fri, 30 Jan 2015 16:14:30 -0800, Peter Oh wrote:
> Using ioread() to perform draining write buffer is excessive.
> Use compact API, mb(), that intended to be used for the case.
> It reduces total 14 CPU clocks per interrupt.
>
> Signed-off-by: Peter Oh <poh@qca.qualcomm.com>

I have no idea what the code does but this change looks suspicious. 
Usually the point of ioread() is to flush the interconnect buffers 
while mb() ensures ordering only from the CPU perspective.

Could you provide the reason *why* flushing buffers is unnecessary
in the commit message?
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jakub Kici?ski Feb. 6, 2015, 2:53 p.m. UTC | #2
On Fri, 6 Feb 2015 09:58:46 +0100, Jakub Kici?ski wrote:
> On Fri, 30 Jan 2015 16:14:30 -0800, Peter Oh wrote:
> > Using ioread() to perform draining write buffer is excessive.
> > Use compact API, mb(), that intended to be used for the case.
> > It reduces total 14 CPU clocks per interrupt.
> >
> > Signed-off-by: Peter Oh <poh@qca.qualcomm.com>
> 
> I have no idea what the code does but this change looks suspicious. 
> Usually the point of ioread() is to flush the interconnect buffers 
> while mb() ensures ordering only from the CPU perspective.
> 
> Could you provide the reason *why* flushing buffers is unnecessary
> in the commit message?

I just noticed the discussion on the first version of the patch.
Sorry about the noise.
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index e6972b0..f1e6980 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -353,10 +353,8 @@  static void ath10k_pci_disable_and_clear_legacy_irq(struct ath10k *ar)
 	ath10k_pci_write32(ar, SOC_CORE_BASE_ADDRESS + PCIE_INTR_CLR_ADDRESS,
 			   PCIE_INTR_FIRMWARE_MASK | PCIE_INTR_CE_MASK_ALL);
 
-	/* IMPORTANT: this extra read transaction is required to
-	 * flush the posted write buffer. */
-	(void)ath10k_pci_read32(ar, SOC_CORE_BASE_ADDRESS +
-				PCIE_INTR_ENABLE_ADDRESS);
+	/* drain write buffer */
+	mb();
 }
 
 static void ath10k_pci_enable_legacy_irq(struct ath10k *ar)
@@ -365,10 +363,8 @@  static void ath10k_pci_enable_legacy_irq(struct ath10k *ar)
 			   PCIE_INTR_ENABLE_ADDRESS,
 			   PCIE_INTR_FIRMWARE_MASK | PCIE_INTR_CE_MASK_ALL);
 
-	/* IMPORTANT: this extra read transaction is required to
-	 * flush the posted write buffer. */
-	(void)ath10k_pci_read32(ar, SOC_CORE_BASE_ADDRESS +
-				PCIE_INTR_ENABLE_ADDRESS);
+	/* drain write buffer */
+	mb();
 }
 
 static inline const char *ath10k_pci_get_irq_method(struct ath10k *ar)