Re: [PATCH] ath10k: Replace ioread with wmb for data sync

[Bob Copeland <me@bobcopeland.com>]

On Tue, Jan 27, 2015 at 03:53:00PM -0800, Peter Oh wrote:
> >>-	/* 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);
> >>+	/* invoke data sync barrier */
> >>+	wmb();
> >>  }
> >I am no expert in arcane PCI matters, but that looks suspicious to me.  I seem
> >to recall wmb() only enforced ordering, and maybe not even memory-IO ordering
> >on all platforms.  If you want to disable an irq, it really seems like you
> >would want to flush posted writes so you know the hardware has seen it.
> enforced ordering is happened by flush write buffer and wmb is
> commonly used to flush write buffer.
> so that wmb guarantees ordering by flush write buffer. That's why
> it's called a memory barrier.

Ok, sure, but I/O ordering two different writes, and ensuring device
has seen a posted write, are related but different things, no?  So if
you are disabling an interrupt and want to be really sure interrupts
are off when function returns, I think you still want the read.

Anyway, it's your driver, just pointing out that the "IMPORTANT"
read might still be important to someone.

-- 
Bob Copeland %% http://bobcopeland.com/