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

From: Bob Copeland <me@bobcopeland.com>
To: Peter Oh <poh@qca.qualcomm.com>
Cc: linux-wireless@vger.kernel.org, ath10k@lists.infradead.org
Subject: Re: [PATCH] ath10k: Replace ioread with wmb for data sync
Date: Tue, 27 Jan 2015 16:33:49 -0500	[thread overview]
Message-ID: <20150127213349.GA24933@localhost> (raw)
In-Reply-To: <1422311118-11320-1-git-send-email-poh@qca.qualcomm.com>

On Mon, Jan 26, 2015 at 02:25:18PM -0800, Peter Oh wrote:
> Using ioread() to perform data sync is excessive.
> Use compact API, wmb(), that intended to be used for the case.
> It reduces total 14 CPU clocks per interrupt.

Hi,

>  	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);
> +	/* 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.

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

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