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

[Peter Oh <poh@qca.qualcomm.com>]

On 01/27/2015 08:30 PM, Bob Copeland wrote:
> 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?
yes, they are different and wmb guarantees both.
>    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.
the mechanism that  read is using to make sure write work done is the 
same mechanism that wmb is using.
> Anyway, it's your driver, just pointing out that the "IMPORTANT"
> read might still be important to someone.
I really appreciate your opinion.
>
Regards,
Peter

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k