* [v3.6 1/3] Revert "iommu/tegra: smmu: Fix unsleepable memory allocation" @ 2012-07-02 11:26 Hiroshi DOYU [not found] ` <1341228398-6878-1-git-send-email-hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> 0 siblings, 1 reply; 11+ messages in thread From: Hiroshi DOYU @ 2012-07-02 11:26 UTC (permalink / raw) To: iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA Cc: linux-tegra-u79uwXL29TY76Z2rM5mHXA, 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
[parent not found: <1341228398-6878-1-git-send-email-hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>]
* [v3.6 2/3] iommu/tegra: smmu: Remove unnecessary sanity check at alloc_pdir() [not found] ` <1341228398-6878-1-git-send-email-hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> @ 2012-07-02 11:26 ` Hiroshi DOYU 2012-07-02 11:26 ` [v3.6 3/3] iommu/tegra: smmu: Fix unsleepable memory allocation " Hiroshi DOYU 1 sibling, 0 replies; 11+ messages in thread From: Hiroshi DOYU @ 2012-07-02 11:26 UTC (permalink / raw) To: iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA Cc: linux-tegra-u79uwXL29TY76Z2rM5mHXA 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [v3.6 3/3] iommu/tegra: smmu: Fix unsleepable memory allocation at alloc_pdir() [not found] ` <1341228398-6878-1-git-send-email-hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> 2012-07-02 11:26 ` [v3.6 2/3] iommu/tegra: smmu: Remove unnecessary sanity check at alloc_pdir() Hiroshi DOYU @ 2012-07-02 11:26 ` Hiroshi DOYU [not found] ` <1341228398-6878-3-git-send-email-hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> 1 sibling, 1 reply; 11+ messages in thread From: Hiroshi DOYU @ 2012-07-02 11:26 UTC (permalink / raw) To: iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA Cc: linux-tegra-u79uwXL29TY76Z2rM5mHXA, Chris Wright 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
[parent not found: <1341228398-6878-3-git-send-email-hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>]
* Re: [v3.6 3/3] iommu/tegra: smmu: Fix unsleepable memory allocation at alloc_pdir() [not found] ` <1341228398-6878-3-git-send-email-hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> @ 2012-07-17 10:09 ` Joerg Roedel 0 siblings, 0 replies; 11+ messages in thread From: Joerg Roedel @ 2012-07-17 10:09 UTC (permalink / raw) To: Hiroshi DOYU Cc: iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA, linux-tegra-u79uwXL29TY76Z2rM5mHXA, Chris Wright 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [v3.6 3/3] iommu/tegra: smmu: Fix unsleepable memory allocation at alloc_pdir() @ 2012-07-17 10:09 ` Joerg Roedel 0 siblings, 0 replies; 11+ messages in thread From: Joerg Roedel @ 2012-07-17 10:09 UTC (permalink / raw) To: Hiroshi DOYU Cc: iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA, linux-tegra-u79uwXL29TY76Z2rM5mHXA, Chris Wright 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
[parent not found: <20120717100901.GH4213-5C7GfCeVMHo@public.gmane.org>]
* Re: [v3.6 3/3] iommu/tegra: smmu: Fix unsleepable memory allocation at alloc_pdir() 2012-07-17 10:09 ` Joerg Roedel @ 2012-07-17 12:25 ` Hiroshi Doyu -1 siblings, 0 replies; 11+ messages in thread From: Hiroshi Doyu @ 2012-07-17 12:25 UTC (permalink / raw) To: joerg.roedel-5C7GfCeVMHo, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r Cc: iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA, linux-tegra-u79uwXL29TY76Z2rM5mHXA, chrisw-69jw2NvuJkxg9hUCZPvPmw, ohad-Ix1uc/W3ht7QT0dZR+AlfA, linux-omap-u79uwXL29TY76Z2rM5mHXA 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [v3.6 3/3] iommu/tegra: smmu: Fix unsleepable memory allocation at alloc_pdir() @ 2012-07-17 12:25 ` Hiroshi Doyu 0 siblings, 0 replies; 11+ messages in thread From: Hiroshi Doyu @ 2012-07-17 12:25 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
[parent not found: <20120717.152524.175499431618552821.hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>]
* Re: [v3.6 3/3] iommu/tegra: smmu: Fix unsleepable memory allocation at alloc_pdir() 2012-07-17 12:25 ` Hiroshi Doyu @ 2012-07-17 13:23 ` joerg.roedel at amd.com -1 siblings, 0 replies; 11+ messages in thread From: joerg.roedel-5C7GfCeVMHo @ 2012-07-17 13:23 UTC (permalink / raw) To: Hiroshi Doyu Cc: ohad-Ix1uc/W3ht7QT0dZR+AlfA, chrisw-69jw2NvuJkxg9hUCZPvPmw, iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA, linux-tegra-u79uwXL29TY76Z2rM5mHXA, linux-omap-u79uwXL29TY76Z2rM5mHXA, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [v3.6 3/3] iommu/tegra: smmu: Fix unsleepable memory allocation at alloc_pdir() @ 2012-07-17 13:23 ` joerg.roedel at amd.com 0 siblings, 0 replies; 11+ messages in thread From: joerg.roedel at amd.com @ 2012-07-17 13:23 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [v3.6 3/3] iommu/tegra: smmu: Fix unsleepable memory allocation at alloc_pdir() 2012-07-17 13:23 ` joerg.roedel at amd.com @ 2012-07-18 8:50 ` Hiroshi Doyu -1 siblings, 0 replies; 11+ messages in thread From: Hiroshi Doyu @ 2012-07-18 8:50 UTC (permalink / raw) To: joerg.roedel Cc: linux-arm-kernel, iommu, linux-tegra, chrisw, ohad, linux-omap "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. ^ permalink raw reply [flat|nested] 11+ messages in thread
* [v3.6 3/3] iommu/tegra: smmu: Fix unsleepable memory allocation at alloc_pdir() @ 2012-07-18 8:50 ` Hiroshi Doyu 0 siblings, 0 replies; 11+ messages in thread From: Hiroshi Doyu @ 2012-07-18 8:50 UTC (permalink / raw) To: linux-arm-kernel "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. ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2012-07-18 8:50 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2012-07-02 11:26 [v3.6 1/3] Revert "iommu/tegra: smmu: Fix unsleepable memory allocation" Hiroshi DOYU [not found] ` <1341228398-6878-1-git-send-email-hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> 2012-07-02 11:26 ` [v3.6 2/3] iommu/tegra: smmu: Remove unnecessary sanity check at alloc_pdir() Hiroshi DOYU 2012-07-02 11:26 ` [v3.6 3/3] iommu/tegra: smmu: Fix unsleepable memory allocation " Hiroshi DOYU [not found] ` <1341228398-6878-3-git-send-email-hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> 2012-07-17 10:09 ` Joerg Roedel 2012-07-17 10:09 ` Joerg Roedel [not found] ` <20120717100901.GH4213-5C7GfCeVMHo@public.gmane.org> 2012-07-17 12:25 ` Hiroshi Doyu 2012-07-17 12:25 ` Hiroshi Doyu [not found] ` <20120717.152524.175499431618552821.hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> 2012-07-17 13:23 ` joerg.roedel-5C7GfCeVMHo 2012-07-17 13:23 ` joerg.roedel at amd.com 2012-07-18 8:50 ` Hiroshi Doyu 2012-07-18 8:50 ` Hiroshi Doyu
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.