diff mbox

[v2,1/1] iommu/tegra: smmu: Fix unsleepable memory allocation at alloc_pdir()

Message ID 1340880707-6934-1-git-send-email-hdoyu@nvidia.com
State Not Applicable, archived
Headers show

Commit Message

Hiroshi Doyu June 28, 2012, 10:51 a.m. UTC 
alloc_pdir() is called from smmu_iommu_domain_init() with spin_lock
held. memory allocations in alloc_pdir() had to be
atomic/unsleepable. Instead of converting into atomic allocation, this
patch once releases a lock, do the allocation, hold the lock again and
then see if it's raced or not in order to avoid introducing mutex.

Signed-off-by: Hiroshi DOYU <hdoyu@nvidia.com>
Reported-by: Chris Wright <chrisw@sous-sol.org>
Cc: Chris Wright <chrisw@sous-sol.org>
Cc: Stephen Warren <swarren@wwwdotorg.org>
---
 drivers/iommu/tegra-smmu.c |   49 +++++++++++++++++++++++++++++--------------
 1 files changed, 33 insertions(+), 16 deletions(-)

Comments

Stephen Warren June 28, 2012, 4:57 p.m. UTC | #1 
On 06/28/2012 04:51 AM, Hiroshi DOYU wrote:
> alloc_pdir() is called from smmu_iommu_domain_init() with spin_lock
> held. memory allocations in alloc_pdir() had to be
> atomic/unsleepable. Instead of converting into atomic allocation, this
> patch once releases a lock, do the allocation, hold the lock again and
> then see if it's raced or not in order to avoid introducing mutex.

> ---

You'd typically want to include a brief description of what changed from
v1->v2 here, as a hint to reviewers re: what to concentrate on.

> +static int alloc_pdir(struct smmu_as *as, unsigned long *flags)

> +	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 = -ENODEV;
> +		goto err_out;
>  	}

Is that really an error; it just means that something else allocated the
same pdir already. Since the top of the function does:

 	if (as->pdir_page)
 		return 0;

I'd expect to s/err = -ENODEV/err = 0/ inside that if condition that I
quoted above, but still of cause "goto err_out" to free the now unneeded
allocations.

Aside from that, I think this looks reasonable.
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index ecd6790..ec656ec 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -539,31 +539,42 @@  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;
 
 	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) {
-		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 = -ENODEV;
+		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);
 
@@ -580,6 +591,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)
@@ -791,7 +808,7 @@  static int smmu_iommu_domain_init(struct iommu_domain *domain)
 	return -ENODEV;
 
 found:
-	if (alloc_pdir(as) < 0)
+	if (alloc_pdir(as, &flags) < 0)
 		goto err_alloc_pdir;
 
 	spin_lock(&smmu->lock);