[PATCH 1/2] iommu/mtk: Avoid redundant TLB syncs locally

[PATCH 1/2] iommu/mtk: Avoid redundant TLB syncs locally

[Robin Murphy]

Under certain circumstances, the io-pgtable code may end up issuing two
TLB sync operations without any intervening invalidations. This goes
badly for the M4U hardware, since it means the second sync ends up
polling for a non-existent operation to finish, and as a result times
out and warns. The io_pgtable_tlb_* helpers implement a high-level
optimisation to avoid issuing the second sync at all in such cases, but
in order to work correctly that requires all pagetable operations to be
serialised under a lock, thus is no longer applicable to all io-pgtable
users.

Since we're the only user actually relying on this flag for correctness,
let's reimplement it locally to avoid the headache of trying to make the
high-level version concurrency-safe for other users.

CC: Yong Wu <yong.wu-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
CC: Matthias Brugger <matthias.bgg-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
---
 drivers/iommu/mtk_iommu.c | 6 ++++++
 drivers/iommu/mtk_iommu.h | 1 +
 2 files changed, 7 insertions(+)

diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
index 5d14cd15198d..91c6d367ab35 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -129,6 +129,7 @@ static void mtk_iommu_tlb_add_flush_nosync(unsigned long iova, size_t size,
 	writel_relaxed(iova, data->base + REG_MMU_INVLD_START_A);
 	writel_relaxed(iova + size - 1, data->base + REG_MMU_INVLD_END_A);
 	writel_relaxed(F_MMU_INV_RANGE, data->base + REG_MMU_INVALIDATE);
+	data->tlb_flush_active = true;
 }
 
 static void mtk_iommu_tlb_sync(void *cookie)
@@ -137,6 +138,10 @@ static void mtk_iommu_tlb_sync(void *cookie)
 	int ret;
 	u32 tmp;
 
+	/* Avoid timing out if there's nothing to wait for */
+	if (!data->tlb_flush_active)
+		return;
+
 	ret = readl_poll_timeout_atomic(data->base + REG_MMU_CPE_DONE, tmp,
 					tmp != 0, 10, 100000);
 	if (ret) {
@@ -146,6 +151,7 @@ static void mtk_iommu_tlb_sync(void *cookie)
 	}
 	/* Clear the CPE status */
 	writel_relaxed(0, data->base + REG_MMU_CPE_DONE);
+	data->tlb_flush_active = false;
 }
 
 static const struct iommu_gather_ops mtk_iommu_gather_ops = {
diff --git a/drivers/iommu/mtk_iommu.h b/drivers/iommu/mtk_iommu.h
index 2a28eadeea0e..c06cc91b5d9a 100644
--- a/drivers/iommu/mtk_iommu.h
+++ b/drivers/iommu/mtk_iommu.h
@@ -47,6 +47,7 @@ struct mtk_iommu_data {
 	struct iommu_group		*m4u_group;
 	struct mtk_smi_iommu		smi_imu;      /* SMI larb iommu info */
 	bool                            enable_4GB;
+	bool				tlb_flush_active;
 
 	struct iommu_device		iommu;
 };
-- 
2.12.2.dirty

[PATCH 2/2] Revert "iommu/io-pgtable: Avoid redundant TLB syncs"

[Robin Murphy]

The tlb_sync_pending flag was necessary for correctness in the Mediatek
M4U driver, but since it offered a small theoretical optimisation for
all io-pgtable users it was implemented as a high-level thing. However,
now that some users may not be using a synchronising lock, there are
several ways this flag can go wrong for them, and at worst it could
result in incorrect behaviour.

Since we've addressed the correctness issue within the Mediatek driver
itself, and fixing the optimisation aspect to be concurrency-safe would
be quite a headache (and impose extra overhead on every operation for
the sake of slightly helping one case which will virtually never happen
in typical usage), let's just retire it.

This reverts commit 88492a4700360a086e55d8874ad786105a5e8b0f.

Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
---
 drivers/iommu/io-pgtable.h | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/drivers/iommu/io-pgtable.h b/drivers/iommu/io-pgtable.h
index 524263a7ae6f..a3e667077b14 100644
--- a/drivers/iommu/io-pgtable.h
+++ b/drivers/iommu/io-pgtable.h
@@ -158,14 +158,12 @@ void free_io_pgtable_ops(struct io_pgtable_ops *ops);
  * @fmt:    The page table format.
  * @cookie: An opaque token provided by the IOMMU driver and passed back to
  *          any callback routines.
- * @tlb_sync_pending: Private flag for optimising out redundant syncs.
  * @cfg:    A copy of the page table configuration.
  * @ops:    The page table operations in use for this set of page tables.
  */
 struct io_pgtable {
 	enum io_pgtable_fmt	fmt;
 	void			*cookie;
-	bool			tlb_sync_pending;
 	struct io_pgtable_cfg	cfg;
 	struct io_pgtable_ops	ops;
 };
@@ -175,22 +173,17 @@ struct io_pgtable {
 static inline void io_pgtable_tlb_flush_all(struct io_pgtable *iop)
 {
 	iop->cfg.tlb->tlb_flush_all(iop->cookie);
-	iop->tlb_sync_pending = true;
 }
 
 static inline void io_pgtable_tlb_add_flush(struct io_pgtable *iop,
 		unsigned long iova, size_t size, size_t granule, bool leaf)
 {
 	iop->cfg.tlb->tlb_add_flush(iova, size, granule, leaf, iop->cookie);
-	iop->tlb_sync_pending = true;
 }
 
 static inline void io_pgtable_tlb_sync(struct io_pgtable *iop)
 {
-	if (iop->tlb_sync_pending) {
-		iop->cfg.tlb->tlb_sync(iop->cookie);
-		iop->tlb_sync_pending = false;
-	}
+	iop->cfg.tlb->tlb_sync(iop->cookie);
 }
 
 /**
-- 
2.12.2.dirty

Re: [PATCH 1/2] iommu/mtk: Avoid redundant TLB syncs locally

[Yong Wu]

On Thu, 2017-07-06 at 17:55 +0100, Robin Murphy wrote:
> Under certain circumstances, the io-pgtable code may end up issuing two
> TLB sync operations without any intervening invalidations. This goes
> badly for the M4U hardware, since it means the second sync ends up
> polling for a non-existent operation to finish, and as a result times
> out and warns. The io_pgtable_tlb_* helpers implement a high-level
> optimisation to avoid issuing the second sync at all in such cases, but
> in order to work correctly that requires all pagetable operations to be
> serialised under a lock, thus is no longer applicable to all io-pgtable
> users.
> 
> Since we're the only user actually relying on this flag for correctness,
> let's reimplement it locally to avoid the headache of trying to make the
> high-level version concurrency-safe for other users.
> 
> CC: Yong Wu <yong.wu-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> CC: Matthias Brugger <matthias.bgg-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>

Thanks Robin.

Tested-by: Yong Wu <yong.wu-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>

> ---
>  drivers/iommu/mtk_iommu.c | 6 ++++++
>  drivers/iommu/mtk_iommu.h | 1 +
>  2 files changed, 7 insertions(+)
> 
> diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
> index 5d14cd15198d..91c6d367ab35 100644
> --- a/drivers/iommu/mtk_iommu.c
> +++ b/drivers/iommu/mtk_iommu.c
> @@ -129,6 +129,7 @@ static void mtk_iommu_tlb_add_flush_nosync(unsigned long iova, size_t size,
>  	writel_relaxed(iova, data->base + REG_MMU_INVLD_START_A);
>  	writel_relaxed(iova + size - 1, data->base + REG_MMU_INVLD_END_A);
>  	writel_relaxed(F_MMU_INV_RANGE, data->base + REG_MMU_INVALIDATE);
> +	data->tlb_flush_active = true;
>  }
>  
>  static void mtk_iommu_tlb_sync(void *cookie)
> @@ -137,6 +138,10 @@ static void mtk_iommu_tlb_sync(void *cookie)
>  	int ret;
>  	u32 tmp;
>  
> +	/* Avoid timing out if there's nothing to wait for */
> +	if (!data->tlb_flush_active)
> +		return;
> +
>  	ret = readl_poll_timeout_atomic(data->base + REG_MMU_CPE_DONE, tmp,
>  					tmp != 0, 10, 100000);
>  	if (ret) {
> @@ -146,6 +151,7 @@ static void mtk_iommu_tlb_sync(void *cookie)
>  	}
>  	/* Clear the CPE status */
>  	writel_relaxed(0, data->base + REG_MMU_CPE_DONE);
> +	data->tlb_flush_active = false;
>  }
>  
>  static const struct iommu_gather_ops mtk_iommu_gather_ops = {
> diff --git a/drivers/iommu/mtk_iommu.h b/drivers/iommu/mtk_iommu.h
> index 2a28eadeea0e..c06cc91b5d9a 100644
> --- a/drivers/iommu/mtk_iommu.h
> +++ b/drivers/iommu/mtk_iommu.h
> @@ -47,6 +47,7 @@ struct mtk_iommu_data {
>  	struct iommu_group		*m4u_group;
>  	struct mtk_smi_iommu		smi_imu;      /* SMI larb iommu info */
>  	bool                            enable_4GB;
> +	bool				tlb_flush_active;
>  
>  	struct iommu_device		iommu;
>  };