[v3.6 1/3] Revert "iommu/tegra: smmu: Fix unsleepable memory allocation"

[v3.6 1/3] Revert "iommu/tegra: smmu: Fix unsleepable memory allocation"

[Hiroshi DOYU]

This reverts commit 64b3aeed242ae55611e0f6286159957873cf968d.

For the succeeding commits.

Signed-off-by: Hiroshi DOYU <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 drivers/iommu/tegra-smmu.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index 3f3d09d..ecd6790 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -550,13 +550,13 @@ static int alloc_pdir(struct smmu_as *as)
 		return 0;
 
 	as->pte_count = devm_kzalloc(smmu->dev,
-		     sizeof(as->pte_count[0]) * SMMU_PDIR_COUNT, GFP_ATOMIC);
+		     sizeof(as->pte_count[0]) * SMMU_PDIR_COUNT, GFP_KERNEL);
 	if (!as->pte_count) {
 		dev_err(smmu->dev,
 			"failed to allocate smmu_device PTE cunters\n");
 		return -ENOMEM;
 	}
-	as->pdir_page = alloc_page(GFP_ATOMIC | __GFP_DMA);
+	as->pdir_page = alloc_page(GFP_KERNEL | __GFP_DMA);
 	if (!as->pdir_page) {
 		dev_err(smmu->dev,
 			"failed to allocate smmu_device page directory\n");
-- 
1.7.5.4

[v3.6 2/3] iommu/tegra: smmu: Remove unnecessary sanity check at alloc_pdir()

[Hiroshi DOYU]

alloc_pdir() is called with smmu->as[?].pdir_page == NULL. No need to
check pdir_page again inside alloc_pdir().

Signed-off-by: Hiroshi DOYU <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
Acked-by: Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
---
 drivers/iommu/tegra-smmu.c |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index ecd6790..532c8a4 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -546,9 +546,6 @@ static int alloc_pdir(struct smmu_as *as)
 	u32 val;
 	struct smmu_device *smmu = as->smmu;
 
-	if (as->pdir_page)
-		return 0;
-
 	as->pte_count = devm_kzalloc(smmu->dev,
 		     sizeof(as->pte_count[0]) * SMMU_PDIR_COUNT, GFP_KERNEL);
 	if (!as->pte_count) {
-- 
1.7.5.4

[v3.6 3/3] iommu/tegra: smmu: Fix unsleepable memory allocation at alloc_pdir()

[Hiroshi DOYU]

alloc_pdir() is called from smmu_iommu_domain_init() with spin_lock
held. memory allocations in alloc_pdir() had to be atomic. Instead of
converting into atomic allocation, this patch once releases a lock,
does the allocation, holds the lock again and then sees if it's raced
or not in order to avoid introducing mutex and preallocation.

Signed-off-by: Hiroshi DOYU <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
Reported-by: Chris Wright <chrisw-69jw2NvuJkxg9hUCZPvPmw@public.gmane.org>
Cc: Chris Wright <chrisw-69jw2NvuJkxg9hUCZPvPmw@public.gmane.org>
Acked-by: Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
---
 drivers/iommu/tegra-smmu.c |   77 +++++++++++++++++++++++++------------------
 1 files changed, 45 insertions(+), 32 deletions(-)

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index 532c8a4..dbba94c 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -539,28 +539,39 @@ static inline void put_signature(struct smmu_as *as,
 /*
  * Caller must lock/unlock as
  */
-static int alloc_pdir(struct smmu_as *as)
+static int alloc_pdir(struct smmu_as *as, unsigned long *flags)
 {
 	unsigned long *pdir;
-	int pdn;
+	int pdn, err = 0;
 	u32 val;
 	struct smmu_device *smmu = as->smmu;
+	struct page *page;
+	unsigned int *cnt;
 
-	as->pte_count = devm_kzalloc(smmu->dev,
-		     sizeof(as->pte_count[0]) * SMMU_PDIR_COUNT, GFP_KERNEL);
-	if (!as->pte_count) {
-		dev_err(smmu->dev,
-			"failed to allocate smmu_device PTE cunters\n");
-		return -ENOMEM;
+	/*
+	 * do the allocation outside the as lock
+	 */
+	spin_unlock_irqrestore(&as->lock, *flags);
+	cnt = devm_kzalloc(smmu->dev,
+			   sizeof(cnt[0]) * SMMU_PDIR_COUNT, GFP_KERNEL);
+	page = alloc_page(GFP_KERNEL | __GFP_DMA);
+	spin_lock_irqsave(&as->lock, *flags);
+
+	if (as->pdir_page) {
+		/* We raced, free the redundant */
+		err = -EAGAIN;
+		goto err_out;
 	}
-	as->pdir_page = alloc_page(GFP_KERNEL | __GFP_DMA);
-	if (!as->pdir_page) {
-		dev_err(smmu->dev,
-			"failed to allocate smmu_device page directory\n");
-		devm_kfree(smmu->dev, as->pte_count);
-		as->pte_count = NULL;
-		return -ENOMEM;
+
+	if (!page || !cnt) {
+		dev_err(smmu->dev, "failed to allocate at %s\n", __func__);
+		err = -ENOMEM;
+		goto err_out;
 	}
+
+	as->pdir_page = page;
+	as->pte_count = cnt;
+
 	SetPageReserved(as->pdir_page);
 	pdir = page_address(as->pdir_page);
 
@@ -577,6 +588,12 @@ static int alloc_pdir(struct smmu_as *as)
 	FLUSH_SMMU_REGS(as->smmu);
 
 	return 0;
+
+err_out:
+	devm_kfree(smmu->dev, cnt);
+	if (page)
+		__free_page(page);
+	return err;
 }
 
 static void __smmu_iommu_unmap(struct smmu_as *as, dma_addr_t iova)
@@ -768,29 +785,29 @@ out:
 
 static int smmu_iommu_domain_init(struct iommu_domain *domain)
 {
-	int i;
+	int i, err = -ENODEV;
 	unsigned long flags;
 	struct smmu_as *as;
 	struct smmu_device *smmu = smmu_handle;
 
 	/* Look for a free AS with lock held */
 	for  (i = 0; i < smmu->num_as; i++) {
-		struct smmu_as *tmp = &smmu->as[i];
-
-		spin_lock_irqsave(&tmp->lock, flags);
-		if (!tmp->pdir_page) {
-			as = tmp;
-			goto found;
+		as = &smmu->as[i];
+		spin_lock_irqsave(&as->lock, flags);
+		if (!as->pdir_page) {
+			err = alloc_pdir(as, &flags);
+			if (!err)
+				goto found;
 		}
-		spin_unlock_irqrestore(&tmp->lock, flags);
+		spin_unlock_irqrestore(&as->lock, flags);
+		if (err != -EAGAIN)
+			break;
 	}
-	dev_err(smmu->dev, "no free AS\n");
-	return -ENODEV;
+	if (i == smmu->num_as)
+		dev_err(smmu->dev,  "no free AS\n");
+	return err;
 
 found:
-	if (alloc_pdir(as) < 0)
-		goto err_alloc_pdir;
-
 	spin_lock(&smmu->lock);
 
 	/* Update PDIR register */
@@ -806,10 +823,6 @@ found:
 
 	dev_dbg(smmu->dev, "smmu_as@%p\n", as);
 	return 0;
-
-err_alloc_pdir:
-	spin_unlock_irqrestore(&as->lock, flags);
-	return -ENODEV;
 }
 
 static void smmu_iommu_domain_destroy(struct iommu_domain *domain)
-- 
1.7.5.4

Re: [v3.6 3/3] iommu/tegra: smmu: Fix unsleepable memory allocation at alloc_pdir()

[Joerg Roedel]

On Mon, Jul 02, 2012 at 02:26:38PM +0300, Hiroshi DOYU wrote:

> Signed-off-by: Hiroshi DOYU <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> Reported-by: Chris Wright <chrisw-69jw2NvuJkxg9hUCZPvPmw@public.gmane.org>
> Cc: Chris Wright <chrisw-69jw2NvuJkxg9hUCZPvPmw@public.gmane.org>
> Acked-by: Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>

Applied patch 2 and 3 but not patch 1. The resulting conflicts are
solved while merging the next branch. Also I am not happy with the way
the as->lock is taken and released multiple times in patch 3. So I added
another commit on-top. Please have a look at it as I can only
compile-test that change:

From f9a4f063a88297e361fd6676986cf3e39b22de72 Mon Sep 17 00:00:00 2001
From: Joerg Roedel <joerg.roedel-5C7GfCeVMHo@public.gmane.org>
Date: Tue, 17 Jul 2012 11:47:14 +0200
Subject: [PATCH] iommu/tegra: Don't call alloc_pdir with as->lock

Instead of taking as->lock before calling alloc_pdir() and
releasing it in that function to allocate memory, just take
the lock only in the alloc_pdir function and run the loop
without any lock held. This simplifies the complicated
lock->unlock->alloc->lock->unlock sequence into
alloc->lock->unlock.

Signed-off-by: Joerg Roedel <joerg.roedel-5C7GfCeVMHo@public.gmane.org>
---
 drivers/iommu/tegra-smmu.c |   29 ++++++++++++++++-------------
 1 file changed, 16 insertions(+), 13 deletions(-)

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index 68a15a0..541d210 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -553,11 +553,11 @@ static inline void put_signature(struct smmu_as *as,
 #endif
 
 /*
- * Caller must lock/unlock as
+ * Caller must not hold as->lock
  */
-static int alloc_pdir(struct smmu_as *as, unsigned long *flags)
+static int alloc_pdir(struct smmu_as *as)
 {
-	unsigned long *pdir;
+	unsigned long *pdir, flags;
 	int pdn, err = 0;
 	u32 val;
 	struct smmu_device *smmu = as->smmu;
@@ -565,13 +565,14 @@ static int alloc_pdir(struct smmu_as *as, unsigned long *flags)
 	unsigned int *cnt;
 
 	/*
-	 * do the allocation outside the as->lock
+	 * do the allocation, then grab as->lock
 	 */
-	spin_unlock_irqrestore(&as->lock, *flags);
 	cnt = devm_kzalloc(smmu->dev,
-			   sizeof(cnt[0]) * SMMU_PDIR_COUNT, GFP_KERNEL);
+			   sizeof(cnt[0]) * SMMU_PDIR_COUNT,
+			   GFP_KERNEL);
 	page = alloc_page(GFP_KERNEL | __GFP_DMA);
-	spin_lock_irqsave(&as->lock, *flags);
+
+	spin_lock_irqsave(&as->lock, flags);
 
 	if (as->pdir_page) {
 		/* We raced, free the redundant */
@@ -603,9 +604,13 @@ static int alloc_pdir(struct smmu_as *as, unsigned long *flags)
 	smmu_write(smmu, val, SMMU_TLB_FLUSH);
 	FLUSH_SMMU_REGS(as->smmu);
 
+	spin_unlock_irqrestore(&as->lock, flags);
+
 	return 0;
 
 err_out:
+	spin_unlock_irqrestore(&as->lock, flags);
+
 	devm_kfree(smmu->dev, cnt);
 	if (page)
 		__free_page(page);
@@ -809,13 +814,11 @@ static int smmu_iommu_domain_init(struct iommu_domain *domain)
 	/* Look for a free AS with lock held */
 	for  (i = 0; i < smmu->num_as; i++) {
 		as = &smmu->as[i];
-		spin_lock_irqsave(&as->lock, flags);
 		if (!as->pdir_page) {
-			err = alloc_pdir(as, &flags);
+			err = alloc_pdir(as);
 			if (!err)
 				goto found;
 		}
-		spin_unlock_irqrestore(&as->lock, flags);
 		if (err != -EAGAIN)
 			break;
 	}
@@ -824,7 +827,7 @@ static int smmu_iommu_domain_init(struct iommu_domain *domain)
 	return err;
 
 found:
-	spin_lock(&smmu->lock);
+	spin_lock_irqsave(&smmu->lock, flags);
 
 	/* Update PDIR register */
 	smmu_write(smmu, SMMU_PTB_ASID_CUR(as->asid), SMMU_PTB_ASID);
@@ -832,12 +835,12 @@ found:
 		   SMMU_MK_PDIR(as->pdir_page, as->pdir_attr), SMMU_PTB_DATA);
 	FLUSH_SMMU_REGS(smmu);
 
-	spin_unlock(&smmu->lock);
+	spin_unlock_irqrestore(&smmu->lock, flags);
 
-	spin_unlock_irqrestore(&as->lock, flags);
 	domain->priv = as;
 
 	dev_dbg(smmu->dev, "smmu_as@%p\n", as);
+
 	return 0;
 }
 
-- 
1.7.9.5


-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

Re: [v3.6 3/3] iommu/tegra: smmu: Fix unsleepable memory allocation at alloc_pdir()

[Joerg Roedel]

On Mon, Jul 02, 2012 at 02:26:38PM +0300, Hiroshi DOYU wrote:

> Signed-off-by: Hiroshi DOYU <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> Reported-by: Chris Wright <chrisw-69jw2NvuJkxg9hUCZPvPmw@public.gmane.org>
> Cc: Chris Wright <chrisw-69jw2NvuJkxg9hUCZPvPmw@public.gmane.org>
> Acked-by: Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>

Applied patch 2 and 3 but not patch 1. The resulting conflicts are
solved while merging the next branch. Also I am not happy with the way
the as->lock is taken and released multiple times in patch 3. So I added
another commit on-top. Please have a look at it as I can only
compile-test that change:

>From f9a4f063a88297e361fd6676986cf3e39b22de72 Mon Sep 17 00:00:00 2001
From: Joerg Roedel <joerg.roedel-5C7GfCeVMHo@public.gmane.org>
Date: Tue, 17 Jul 2012 11:47:14 +0200
Subject: [PATCH] iommu/tegra: Don't call alloc_pdir with as->lock

Instead of taking as->lock before calling alloc_pdir() and
releasing it in that function to allocate memory, just take
the lock only in the alloc_pdir function and run the loop
without any lock held. This simplifies the complicated
lock->unlock->alloc->lock->unlock sequence into
alloc->lock->unlock.

Signed-off-by: Joerg Roedel <joerg.roedel-5C7GfCeVMHo@public.gmane.org>
---
 drivers/iommu/tegra-smmu.c |   29 ++++++++++++++++-------------
 1 file changed, 16 insertions(+), 13 deletions(-)

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index 68a15a0..541d210 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -553,11 +553,11 @@ static inline void put_signature(struct smmu_as *as,
 #endif
 
 /*
- * Caller must lock/unlock as
+ * Caller must not hold as->lock
  */
-static int alloc_pdir(struct smmu_as *as, unsigned long *flags)
+static int alloc_pdir(struct smmu_as *as)
 {
-	unsigned long *pdir;
+	unsigned long *pdir, flags;
 	int pdn, err = 0;
 	u32 val;
 	struct smmu_device *smmu = as->smmu;
@@ -565,13 +565,14 @@ static int alloc_pdir(struct smmu_as *as, unsigned long *flags)
 	unsigned int *cnt;
 
 	/*
-	 * do the allocation outside the as->lock
+	 * do the allocation, then grab as->lock
 	 */
-	spin_unlock_irqrestore(&as->lock, *flags);
 	cnt = devm_kzalloc(smmu->dev,
-			   sizeof(cnt[0]) * SMMU_PDIR_COUNT, GFP_KERNEL);
+			   sizeof(cnt[0]) * SMMU_PDIR_COUNT,
+			   GFP_KERNEL);
 	page = alloc_page(GFP_KERNEL | __GFP_DMA);
-	spin_lock_irqsave(&as->lock, *flags);
+
+	spin_lock_irqsave(&as->lock, flags);
 
 	if (as->pdir_page) {
 		/* We raced, free the redundant */
@@ -603,9 +604,13 @@ static int alloc_pdir(struct smmu_as *as, unsigned long *flags)
 	smmu_write(smmu, val, SMMU_TLB_FLUSH);
 	FLUSH_SMMU_REGS(as->smmu);
 
+	spin_unlock_irqrestore(&as->lock, flags);
+
 	return 0;
 
 err_out:
+	spin_unlock_irqrestore(&as->lock, flags);
+
 	devm_kfree(smmu->dev, cnt);
 	if (page)
 		__free_page(page);
@@ -809,13 +814,11 @@ static int smmu_iommu_domain_init(struct iommu_domain *domain)
 	/* Look for a free AS with lock held */
 	for  (i = 0; i < smmu->num_as; i++) {
 		as = &smmu->as[i];
-		spin_lock_irqsave(&as->lock, flags);
 		if (!as->pdir_page) {
-			err = alloc_pdir(as, &flags);
+			err = alloc_pdir(as);
 			if (!err)
 				goto found;
 		}
-		spin_unlock_irqrestore(&as->lock, flags);
 		if (err != -EAGAIN)
 			break;
 	}
@@ -824,7 +827,7 @@ static int smmu_iommu_domain_init(struct iommu_domain *domain)
 	return err;
 
 found:
-	spin_lock(&smmu->lock);
+	spin_lock_irqsave(&smmu->lock, flags);
 
 	/* Update PDIR register */
 	smmu_write(smmu, SMMU_PTB_ASID_CUR(as->asid), SMMU_PTB_ASID);
@@ -832,12 +835,12 @@ found:
 		   SMMU_MK_PDIR(as->pdir_page, as->pdir_attr), SMMU_PTB_DATA);
 	FLUSH_SMMU_REGS(smmu);
 
-	spin_unlock(&smmu->lock);
+	spin_unlock_irqrestore(&smmu->lock, flags);
 
-	spin_unlock_irqrestore(&as->lock, flags);
 	domain->priv = as;
 
 	dev_dbg(smmu->dev, "smmu_as@%p\n", as);
+
 	return 0;
 }
 
-- 
1.7.9.5


-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

Re: [v3.6 3/3] iommu/tegra: smmu: Fix unsleepable memory allocation at alloc_pdir()

[Hiroshi Doyu]

Hi Joerg,

Joerg Roedel <joerg.roedel-5C7GfCeVMHo@public.gmane.org> wrote @ Tue, 17 Jul 2012 12:09:01 +0200:

> On Mon, Jul 02, 2012 at 02:26:38PM +0300, Hiroshi DOYU wrote:
> 
> > Signed-off-by: Hiroshi DOYU <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > Reported-by: Chris Wright <chrisw-69jw2NvuJkxg9hUCZPvPmw@public.gmane.org>
> > Cc: Chris Wright <chrisw-69jw2NvuJkxg9hUCZPvPmw@public.gmane.org>
> > Acked-by: Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
> 
> Applied patch 2 and 3 but not patch 1. The resulting conflicts are
> solved while merging the next branch. Also I am not happy with the way
> the as->lock is taken and released multiple times in patch 3. So I added
> another commit on-top. Please have a look at it as I can only
> compile-test that change:
> 
> From f9a4f063a88297e361fd6676986cf3e39b22de72 Mon Sep 17 00:00:00 2001
> From: Joerg Roedel <joerg.roedel-5C7GfCeVMHo@public.gmane.org>
> Date: Tue, 17 Jul 2012 11:47:14 +0200
> Subject: [PATCH] iommu/tegra: Don't call alloc_pdir with as->lock
> 
> Instead of taking as->lock before calling alloc_pdir() and
> releasing it in that function to allocate memory, just take
> the lock only in the alloc_pdir function and run the loop
> without any lock held. This simplifies the complicated
> lock->unlock->alloc->lock->unlock sequence into
> alloc->lock->unlock.
> 
> Signed-off-by: Joerg Roedel <joerg.roedel-5C7GfCeVMHo@public.gmane.org>
> ---
>  drivers/iommu/tegra-smmu.c |   29 ++++++++++++++++-------------
>  1 file changed, 16 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
> index 68a15a0..541d210 100644
> --- a/drivers/iommu/tegra-smmu.c
> +++ b/drivers/iommu/tegra-smmu.c
> @@ -553,11 +553,11 @@ static inline void put_signature(struct smmu_as *as,
>  #endif
>  
>  /*
> - * Caller must lock/unlock as
> + * Caller must not hold as->lock
>   */
> -static int alloc_pdir(struct smmu_as *as, unsigned long *flags)
> +static int alloc_pdir(struct smmu_as *as)
>  {
> -	unsigned long *pdir;
> +	unsigned long *pdir, flags;
>  	int pdn, err = 0;
>  	u32 val;
>  	struct smmu_device *smmu = as->smmu;
> @@ -565,13 +565,14 @@ static int alloc_pdir(struct smmu_as *as, unsigned long *flags)
>  	unsigned int *cnt;
>  
>  	/*
> -	 * do the allocation outside the as->lock
> +	 * do the allocation, then grab as->lock
>  	 */
> -	spin_unlock_irqrestore(&as->lock, *flags);
>  	cnt = devm_kzalloc(smmu->dev,
> -			   sizeof(cnt[0]) * SMMU_PDIR_COUNT, GFP_KERNEL);
> +			   sizeof(cnt[0]) * SMMU_PDIR_COUNT,
> +			   GFP_KERNEL);
>  	page = alloc_page(GFP_KERNEL | __GFP_DMA);
> -	spin_lock_irqsave(&as->lock, *flags);
> +
> +	spin_lock_irqsave(&as->lock, flags);
>  
>  	if (as->pdir_page) {
>  		/* We raced, free the redundant */
> @@ -603,9 +604,13 @@ static int alloc_pdir(struct smmu_as *as, unsigned long *flags)
>  	smmu_write(smmu, val, SMMU_TLB_FLUSH);
>  	FLUSH_SMMU_REGS(as->smmu);
>  
> +	spin_unlock_irqrestore(&as->lock, flags);
> +
>  	return 0;
>  
>  err_out:
> +	spin_unlock_irqrestore(&as->lock, flags);
> +
>  	devm_kfree(smmu->dev, cnt);
>  	if (page)
>  		__free_page(page);
> @@ -809,13 +814,11 @@ static int smmu_iommu_domain_init(struct iommu_domain *domain)
>  	/* Look for a free AS with lock held */
>  	for  (i = 0; i < smmu->num_as; i++) {
>  		as = &smmu->as[i];
> -		spin_lock_irqsave(&as->lock, flags);
>  		if (!as->pdir_page) {
> -			err = alloc_pdir(as, &flags);
> +			err = alloc_pdir(as);
>  			if (!err)
>  				goto found;

The above spin_lock is always necessary. "as->lock" should be held to
protect "as->pdir_page". Only when "as->pdir_page" is NULL,
"as->pdir_page" would be allocated in "alloc_pdir()". Without this
lock, the following race could happen:


Without as->lock:
A:			B:
i == 3
pdir_page == NULL
			i == 3
	     		pdir_page == NULL
pdir_page = a;
			pdir_page = b;	!!!!!! OVERWRITTEN !!!!!!



With as->lock:
A:			B:
i == 3
lock(as->lock)
pdir_page == NULL
			i == 3
			Waiting lock released....
	     		Waiting lock released....
pdir_page = a;		
unlock(as->lock)	
			lock(as->lock)
			pdir_page != NULL && continue
			unlock(as->lock)

			i == 4
			.....


This "lock, unlock, alloc, lock, check race" method was originally
introduced by Russell King a few years ago(*1). And the same mechanism
has been used in omap iommu for years(*2) at least as below:

drivers/iommu/omap-iommu.c:
.....
505          * do the allocation outside the page table lock
506          */
507         spin_unlock(&obj->page_table_lock);
508         iopte = kmem_cache_zalloc(iopte_cachep, GFP_KERNEL);
509         spin_lock(&obj->page_table_lock);
510 
511         if (!*iopgd) {
512                 if (!iopte)
513                         return ERR_PTR(-ENOMEM);
514 
515                 *iopgd = virt_to_phys(iopte) | IOPGD_TABLE;
516                 flush_iopgd_range(iopgd, iopgd);
517 
518                 dev_vdbg(obj->dev, "%s: a new pte:%p\n", __func__, iopte);
519         } else {
520                 /* We raced, free the reduniovant table */
521                 iopte_free(iopte);
522         }


Still we can do preallocation for pdir_page before this lock held, but
if we do that, we have to change the function name, "alloc_pdir()" to
something else because it doesn't allocate actually, and some other
allocations also have to be done in advance too. At this moment, I'd
rather keep the current structure with Russell's method.

*1:
http://www.mail-archive.com/linux-omap-u79uwXL29TY76Z2rM5mHXA@public.gmane.org/msg04007.html
*2:
http://lxr.free-electrons.com/source/drivers/iommu/omap-iommu.c#L496

[v3.6 3/3] iommu/tegra: smmu: Fix unsleepable memory allocation at alloc_pdir()

[Hiroshi Doyu]

Hi Joerg,

Joerg Roedel <joerg.roedel@amd.com> wrote @ Tue, 17 Jul 2012 12:09:01 +0200:

> On Mon, Jul 02, 2012 at 02:26:38PM +0300, Hiroshi DOYU wrote:
> 
> > Signed-off-by: Hiroshi DOYU <hdoyu@nvidia.com>
> > Reported-by: Chris Wright <chrisw@sous-sol.org>
> > Cc: Chris Wright <chrisw@sous-sol.org>
> > Acked-by: Stephen Warren <swarren@wwwdotorg.org>
> 
> Applied patch 2 and 3 but not patch 1. The resulting conflicts are
> solved while merging the next branch. Also I am not happy with the way
> the as->lock is taken and released multiple times in patch 3. So I added
> another commit on-top. Please have a look at it as I can only
> compile-test that change:
> 
> From f9a4f063a88297e361fd6676986cf3e39b22de72 Mon Sep 17 00:00:00 2001
> From: Joerg Roedel <joerg.roedel@amd.com>
> Date: Tue, 17 Jul 2012 11:47:14 +0200
> Subject: [PATCH] iommu/tegra: Don't call alloc_pdir with as->lock
> 
> Instead of taking as->lock before calling alloc_pdir() and
> releasing it in that function to allocate memory, just take
> the lock only in the alloc_pdir function and run the loop
> without any lock held. This simplifies the complicated
> lock->unlock->alloc->lock->unlock sequence into
> alloc->lock->unlock.
> 
> Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
> ---
>  drivers/iommu/tegra-smmu.c |   29 ++++++++++++++++-------------
>  1 file changed, 16 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
> index 68a15a0..541d210 100644
> --- a/drivers/iommu/tegra-smmu.c
> +++ b/drivers/iommu/tegra-smmu.c
> @@ -553,11 +553,11 @@ static inline void put_signature(struct smmu_as *as,
>  #endif
>  
>  /*
> - * Caller must lock/unlock as
> + * Caller must not hold as->lock
>   */
> -static int alloc_pdir(struct smmu_as *as, unsigned long *flags)
> +static int alloc_pdir(struct smmu_as *as)
>  {
> -	unsigned long *pdir;
> +	unsigned long *pdir, flags;
>  	int pdn, err = 0;
>  	u32 val;
>  	struct smmu_device *smmu = as->smmu;
> @@ -565,13 +565,14 @@ static int alloc_pdir(struct smmu_as *as, unsigned long *flags)
>  	unsigned int *cnt;
>  
>  	/*
> -	 * do the allocation outside the as->lock
> +	 * do the allocation, then grab as->lock
>  	 */
> -	spin_unlock_irqrestore(&as->lock, *flags);
>  	cnt = devm_kzalloc(smmu->dev,
> -			   sizeof(cnt[0]) * SMMU_PDIR_COUNT, GFP_KERNEL);
> +			   sizeof(cnt[0]) * SMMU_PDIR_COUNT,
> +			   GFP_KERNEL);
>  	page = alloc_page(GFP_KERNEL | __GFP_DMA);
> -	spin_lock_irqsave(&as->lock, *flags);
> +
> +	spin_lock_irqsave(&as->lock, flags);
>  
>  	if (as->pdir_page) {
>  		/* We raced, free the redundant */
> @@ -603,9 +604,13 @@ static int alloc_pdir(struct smmu_as *as, unsigned long *flags)
>  	smmu_write(smmu, val, SMMU_TLB_FLUSH);
>  	FLUSH_SMMU_REGS(as->smmu);
>  
> +	spin_unlock_irqrestore(&as->lock, flags);
> +
>  	return 0;
>  
>  err_out:
> +	spin_unlock_irqrestore(&as->lock, flags);
> +
>  	devm_kfree(smmu->dev, cnt);
>  	if (page)
>  		__free_page(page);
> @@ -809,13 +814,11 @@ static int smmu_iommu_domain_init(struct iommu_domain *domain)
>  	/* Look for a free AS with lock held */
>  	for  (i = 0; i < smmu->num_as; i++) {
>  		as = &smmu->as[i];
> -		spin_lock_irqsave(&as->lock, flags);
>  		if (!as->pdir_page) {
> -			err = alloc_pdir(as, &flags);
> +			err = alloc_pdir(as);
>  			if (!err)
>  				goto found;

The above spin_lock is always necessary. "as->lock" should be held to
protect "as->pdir_page". Only when "as->pdir_page" is NULL,
"as->pdir_page" would be allocated in "alloc_pdir()". Without this
lock, the following race could happen:


Without as->lock:
A:			B:
i == 3
pdir_page == NULL
			i == 3
	     		pdir_page == NULL
pdir_page = a;
			pdir_page = b;	!!!!!! OVERWRITTEN !!!!!!



With as->lock:
A:			B:
i == 3
lock(as->lock)
pdir_page == NULL
			i == 3
			Waiting lock released....
	     		Waiting lock released....
pdir_page = a;		
unlock(as->lock)	
			lock(as->lock)
			pdir_page != NULL && continue
			unlock(as->lock)

			i == 4
			.....


This "lock, unlock, alloc, lock, check race" method was originally
introduced by Russell King a few years ago(*1). And the same mechanism
has been used in omap iommu for years(*2) at least as below:

drivers/iommu/omap-iommu.c:
.....
505          * do the allocation outside the page table lock
506          */
507         spin_unlock(&obj->page_table_lock);
508         iopte = kmem_cache_zalloc(iopte_cachep, GFP_KERNEL);
509         spin_lock(&obj->page_table_lock);
510 
511         if (!*iopgd) {
512                 if (!iopte)
513                         return ERR_PTR(-ENOMEM);
514 
515                 *iopgd = virt_to_phys(iopte) | IOPGD_TABLE;
516                 flush_iopgd_range(iopgd, iopgd);
517 
518                 dev_vdbg(obj->dev, "%s: a new pte:%p\n", __func__, iopte);
519         } else {
520                 /* We raced, free the reduniovant table */
521                 iopte_free(iopte);
522         }


Still we can do preallocation for pdir_page before this lock held, but
if we do that, we have to change the function name, "alloc_pdir()" to
something else because it doesn't allocate actually, and some other
allocations also have to be done in advance too. At this moment, I'd
rather keep the current structure with Russell's method.

*1:
http://www.mail-archive.com/linux-omap at vger.kernel.org/msg04007.html
*2:
http://lxr.free-electrons.com/source/drivers/iommu/omap-iommu.c#L496

Re: [v3.6 3/3] iommu/tegra: smmu: Fix unsleepable memory allocation at alloc_pdir()

[joerg.roedel-5C7GfCeVMHo]

On Tue, Jul 17, 2012 at 02:25:24PM +0200, Hiroshi Doyu wrote:
> The above spin_lock is always necessary. "as->lock" should be held to
> protect "as->pdir_page". Only when "as->pdir_page" is NULL,
> "as->pdir_page" would be allocated in "alloc_pdir()". Without this
> lock, the following race could happen:
> 
> 
> Without as->lock:
> A:			B:
> i == 3
> pdir_page == NULL
> 			i == 3
> 	     		pdir_page == NULL
> pdir_page = a;
> 			pdir_page = b;	!!!!!! OVERWRITTEN !!!!!!
>

Unless I am missing something, this is not the correct situation with my
patch. It would look more like this:


 A:					B:
 i == 3
 pdir_page == NULL
 					i == 3
 	     				pdir_page == NULL

 take as->lock

 /* race check */
 pdir_page == NULL -> proceed		/* spinning on as->lock */

 pdir_page = a;

 release as->lock

					 take as->lock

					 /* race check */
					 pdir_page != NULL -> return

This should be fine, no? Do I miss something?


	Joerg

[v3.6 3/3] iommu/tegra: smmu: Fix unsleepable memory allocation at alloc_pdir()

[joerg.roedel at amd.com]

On Tue, Jul 17, 2012 at 02:25:24PM +0200, Hiroshi Doyu wrote:
> The above spin_lock is always necessary. "as->lock" should be held to
> protect "as->pdir_page". Only when "as->pdir_page" is NULL,
> "as->pdir_page" would be allocated in "alloc_pdir()". Without this
> lock, the following race could happen:
> 
> 
> Without as->lock:
> A:			B:
> i == 3
> pdir_page == NULL
> 			i == 3
> 	     		pdir_page == NULL
> pdir_page = a;
> 			pdir_page = b;	!!!!!! OVERWRITTEN !!!!!!
>

Unless I am missing something, this is not the correct situation with my
patch. It would look more like this:


 A:					B:
 i == 3
 pdir_page == NULL
 					i == 3
 	     				pdir_page == NULL

 take as->lock

 /* race check */
 pdir_page == NULL -> proceed		/* spinning on as->lock */

 pdir_page = a;

 release as->lock

					 take as->lock

					 /* race check */
					 pdir_page != NULL -> return

This should be fine, no? Do I miss something?


	Joerg

Re: [v3.6 3/3] iommu/tegra: smmu: Fix unsleepable memory allocation at alloc_pdir()

[Hiroshi Doyu]

"joerg.roedel@amd.com" <joerg.roedel@amd.com> wrote @ Tue, 17 Jul 2012 15:23:00 +0200:

> On Tue, Jul 17, 2012 at 02:25:24PM +0200, Hiroshi Doyu wrote:
> > The above spin_lock is always necessary. "as->lock" should be held to
> > protect "as->pdir_page". Only when "as->pdir_page" is NULL,
> > "as->pdir_page" would be allocated in "alloc_pdir()". Without this
> > lock, the following race could happen:
> > 
> > 
> > Without as->lock:
> > A:			B:
> > i == 3
> > pdir_page == NULL
> > 			i == 3
> > 	     		pdir_page == NULL
> > pdir_page = a;
> > 			pdir_page = b;	!!!!!! OVERWRITTEN !!!!!!
> >
> 
> Unless I am missing something, this is not the correct situation with my
> patch. It would look more like this:
> 
> 
>  A:					B:
>  i == 3
>  pdir_page == NULL
>  					i == 3
>  	     				pdir_page == NULL
> 
>  take as->lock
> 
>  /* race check */
>  pdir_page == NULL -> proceed		/* spinning on as->lock */
> 
>  pdir_page = a;
> 
>  release as->lock
> 
> 					 take as->lock
> 
> 					 /* race check */
> 					 pdir_page != NULL -> return
> 
> This should be fine, no? Do I miss something?

You are right. I didn't get the point of your patch. In the case that
you can return -EAGAIN, the complicated "lock,unlock,lock,check race"
is not necessary as you did.

Verified the patch w/ Tegra3 based board. Please put this into next
queue. Thanks.

[v3.6 3/3] iommu/tegra: smmu: Fix unsleepable memory allocation at alloc_pdir()

[Hiroshi Doyu]

"joerg.roedel at amd.com" <joerg.roedel@amd.com> wrote @ Tue, 17 Jul 2012 15:23:00 +0200:

> On Tue, Jul 17, 2012 at 02:25:24PM +0200, Hiroshi Doyu wrote:
> > The above spin_lock is always necessary. "as->lock" should be held to
> > protect "as->pdir_page". Only when "as->pdir_page" is NULL,
> > "as->pdir_page" would be allocated in "alloc_pdir()". Without this
> > lock, the following race could happen:
> > 
> > 
> > Without as->lock:
> > A:			B:
> > i == 3
> > pdir_page == NULL
> > 			i == 3
> > 	     		pdir_page == NULL
> > pdir_page = a;
> > 			pdir_page = b;	!!!!!! OVERWRITTEN !!!!!!
> >
> 
> Unless I am missing something, this is not the correct situation with my
> patch. It would look more like this:
> 
> 
>  A:					B:
>  i == 3
>  pdir_page == NULL
>  					i == 3
>  	     				pdir_page == NULL
> 
>  take as->lock
> 
>  /* race check */
>  pdir_page == NULL -> proceed		/* spinning on as->lock */
> 
>  pdir_page = a;
> 
>  release as->lock
> 
> 					 take as->lock
> 
> 					 /* race check */
> 					 pdir_page != NULL -> return
> 
> This should be fine, no? Do I miss something?

You are right. I didn't get the point of your patch. In the case that
you can return -EAGAIN, the complicated "lock,unlock,lock,check race"
is not necessary as you did.

Verified the patch w/ Tegra3 based board. Please put this into next
queue. Thanks.