* [PATCH v3 0/2] iommu/tegra-smmu: Two followup changes @ 2020-09-29 4:52 ` Nicolin Chen 0 siblings, 0 replies; 10+ messages in thread From: Nicolin Chen @ 2020-09-29 4:52 UTC (permalink / raw) To: thierry.reding, joro Cc: vdumpa, jonathanh, digetx, linux-tegra, iommu, linux-kernel Two followup patches for tegra-smmu: PATCH-1 is a clean-up patch for the recently applied SWGROUP change. PATCH-2 fixes a potential race condition Changelog v2->v3: * PATCH-2: renamed "err_unlock" to "unlock" v1->v2: * Separated first two changs of V1 so they may get applied first, since the other three changes need some extra time to finalize. Nicolin Chen (2): iommu/tegra-smmu: Unwrap tegra_smmu_group_get iommu/tegra-smmu: Expend mutex protection range drivers/iommu/tegra-smmu.c | 53 ++++++++++++++++++-------------------- 1 file changed, 25 insertions(+), 28 deletions(-) -- 2.17.1 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v3 0/2] iommu/tegra-smmu: Two followup changes @ 2020-09-29 4:52 ` Nicolin Chen 0 siblings, 0 replies; 10+ messages in thread From: Nicolin Chen @ 2020-09-29 4:52 UTC (permalink / raw) To: thierry.reding, joro; +Cc: linux-kernel, iommu, jonathanh, linux-tegra, digetx Two followup patches for tegra-smmu: PATCH-1 is a clean-up patch for the recently applied SWGROUP change. PATCH-2 fixes a potential race condition Changelog v2->v3: * PATCH-2: renamed "err_unlock" to "unlock" v1->v2: * Separated first two changs of V1 so they may get applied first, since the other three changes need some extra time to finalize. Nicolin Chen (2): iommu/tegra-smmu: Unwrap tegra_smmu_group_get iommu/tegra-smmu: Expend mutex protection range drivers/iommu/tegra-smmu.c | 53 ++++++++++++++++++-------------------- 1 file changed, 25 insertions(+), 28 deletions(-) -- 2.17.1 _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v3 1/2] iommu/tegra-smmu: Unwrap tegra_smmu_group_get 2020-09-29 4:52 ` Nicolin Chen @ 2020-09-29 4:52 ` Nicolin Chen -1 siblings, 0 replies; 10+ messages in thread From: Nicolin Chen @ 2020-09-29 4:52 UTC (permalink / raw) To: thierry.reding, joro Cc: vdumpa, jonathanh, digetx, linux-tegra, iommu, linux-kernel The tegra_smmu_group_get was added to group devices in different SWGROUPs and it'd return a NULL group pointer upon a mismatch at tegra_smmu_find_group(), so for most of clients/devices, it very likely would mismatch and need a fallback generic_device_group(). But now tegra_smmu_group_get handles devices in same SWGROUP too, which means that it would allocate a group for every new SWGROUP or would directly return an existing one upon matching a SWGROUP, i.e. any device will go through this function. So possibility of having a NULL group pointer in device_group() is upon failure of either devm_kzalloc() or iommu_group_alloc(). In either case, calling generic_device_group() no longer makes a sense. Especially for devm_kzalloc() failing case, it'd cause a problem if it fails at devm_kzalloc() yet succeeds at a fallback generic_device_group(), because it does not create a group->list for other devices to match. This patch simply unwraps the function to clean it up. Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com> --- Changelog v2->v3: * N/A v1->v2: * Changed type of swgroup to "unsigned int", following Thierry's commnets. drivers/iommu/tegra-smmu.c | 19 ++++--------------- 1 file changed, 4 insertions(+), 15 deletions(-) diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c index 0becdbfea306..ec4c9dafff95 100644 --- a/drivers/iommu/tegra-smmu.c +++ b/drivers/iommu/tegra-smmu.c @@ -903,10 +903,12 @@ static void tegra_smmu_group_release(void *iommu_data) mutex_unlock(&smmu->lock); } -static struct iommu_group *tegra_smmu_group_get(struct tegra_smmu *smmu, - unsigned int swgroup) +static struct iommu_group *tegra_smmu_device_group(struct device *dev) { + struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev); + struct tegra_smmu *smmu = dev_iommu_priv_get(dev); const struct tegra_smmu_group_soc *soc; + unsigned int swgroup = fwspec->ids[0]; struct tegra_smmu_group *group; struct iommu_group *grp; @@ -950,19 +952,6 @@ static struct iommu_group *tegra_smmu_group_get(struct tegra_smmu *smmu, return group->group; } -static struct iommu_group *tegra_smmu_device_group(struct device *dev) -{ - struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev); - struct tegra_smmu *smmu = dev_iommu_priv_get(dev); - struct iommu_group *group; - - group = tegra_smmu_group_get(smmu, fwspec->ids[0]); - if (!group) - group = generic_device_group(dev); - - return group; -} - static int tegra_smmu_of_xlate(struct device *dev, struct of_phandle_args *args) { -- 2.17.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v3 1/2] iommu/tegra-smmu: Unwrap tegra_smmu_group_get @ 2020-09-29 4:52 ` Nicolin Chen 0 siblings, 0 replies; 10+ messages in thread From: Nicolin Chen @ 2020-09-29 4:52 UTC (permalink / raw) To: thierry.reding, joro; +Cc: linux-kernel, iommu, jonathanh, linux-tegra, digetx The tegra_smmu_group_get was added to group devices in different SWGROUPs and it'd return a NULL group pointer upon a mismatch at tegra_smmu_find_group(), so for most of clients/devices, it very likely would mismatch and need a fallback generic_device_group(). But now tegra_smmu_group_get handles devices in same SWGROUP too, which means that it would allocate a group for every new SWGROUP or would directly return an existing one upon matching a SWGROUP, i.e. any device will go through this function. So possibility of having a NULL group pointer in device_group() is upon failure of either devm_kzalloc() or iommu_group_alloc(). In either case, calling generic_device_group() no longer makes a sense. Especially for devm_kzalloc() failing case, it'd cause a problem if it fails at devm_kzalloc() yet succeeds at a fallback generic_device_group(), because it does not create a group->list for other devices to match. This patch simply unwraps the function to clean it up. Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com> --- Changelog v2->v3: * N/A v1->v2: * Changed type of swgroup to "unsigned int", following Thierry's commnets. drivers/iommu/tegra-smmu.c | 19 ++++--------------- 1 file changed, 4 insertions(+), 15 deletions(-) diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c index 0becdbfea306..ec4c9dafff95 100644 --- a/drivers/iommu/tegra-smmu.c +++ b/drivers/iommu/tegra-smmu.c @@ -903,10 +903,12 @@ static void tegra_smmu_group_release(void *iommu_data) mutex_unlock(&smmu->lock); } -static struct iommu_group *tegra_smmu_group_get(struct tegra_smmu *smmu, - unsigned int swgroup) +static struct iommu_group *tegra_smmu_device_group(struct device *dev) { + struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev); + struct tegra_smmu *smmu = dev_iommu_priv_get(dev); const struct tegra_smmu_group_soc *soc; + unsigned int swgroup = fwspec->ids[0]; struct tegra_smmu_group *group; struct iommu_group *grp; @@ -950,19 +952,6 @@ static struct iommu_group *tegra_smmu_group_get(struct tegra_smmu *smmu, return group->group; } -static struct iommu_group *tegra_smmu_device_group(struct device *dev) -{ - struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev); - struct tegra_smmu *smmu = dev_iommu_priv_get(dev); - struct iommu_group *group; - - group = tegra_smmu_group_get(smmu, fwspec->ids[0]); - if (!group) - group = generic_device_group(dev); - - return group; -} - static int tegra_smmu_of_xlate(struct device *dev, struct of_phandle_args *args) { -- 2.17.1 _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v3 2/2] iommu/tegra-smmu: Expend mutex protection range 2020-09-29 4:52 ` Nicolin Chen @ 2020-09-29 4:52 ` Nicolin Chen -1 siblings, 0 replies; 10+ messages in thread From: Nicolin Chen @ 2020-09-29 4:52 UTC (permalink / raw) To: thierry.reding, joro Cc: vdumpa, jonathanh, digetx, linux-tegra, iommu, linux-kernel This is used to protect potential race condition at use_count. since probes of client drivers, calling attach_dev(), may run concurrently. Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com> --- Changelog v2->v3: * Renamed label "err_unlock" to "unlock" v1->v2: * N/A drivers/iommu/tegra-smmu.c | 34 +++++++++++++++++++++------------- 1 file changed, 21 insertions(+), 13 deletions(-) diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c index ec4c9dafff95..6a3ecc334481 100644 --- a/drivers/iommu/tegra-smmu.c +++ b/drivers/iommu/tegra-smmu.c @@ -256,26 +256,19 @@ static int tegra_smmu_alloc_asid(struct tegra_smmu *smmu, unsigned int *idp) { unsigned long id; - mutex_lock(&smmu->lock); - id = find_first_zero_bit(smmu->asids, smmu->soc->num_asids); - if (id >= smmu->soc->num_asids) { - mutex_unlock(&smmu->lock); + if (id >= smmu->soc->num_asids) return -ENOSPC; - } set_bit(id, smmu->asids); *idp = id; - mutex_unlock(&smmu->lock); return 0; } static void tegra_smmu_free_asid(struct tegra_smmu *smmu, unsigned int id) { - mutex_lock(&smmu->lock); clear_bit(id, smmu->asids); - mutex_unlock(&smmu->lock); } static bool tegra_smmu_capable(enum iommu_cap cap) @@ -420,17 +413,21 @@ static int tegra_smmu_as_prepare(struct tegra_smmu *smmu, struct tegra_smmu_as *as) { u32 value; - int err; + int err = 0; + + mutex_lock(&smmu->lock); if (as->use_count > 0) { as->use_count++; - return 0; + goto unlock; } as->pd_dma = dma_map_page(smmu->dev, as->pd, 0, SMMU_SIZE_PD, DMA_TO_DEVICE); - if (dma_mapping_error(smmu->dev, as->pd_dma)) - return -ENOMEM; + if (dma_mapping_error(smmu->dev, as->pd_dma)) { + err = -ENOMEM; + goto unlock; + } /* We can't handle 64-bit DMA addresses */ if (!smmu_dma_addr_valid(smmu, as->pd_dma)) { @@ -453,24 +450,35 @@ static int tegra_smmu_as_prepare(struct tegra_smmu *smmu, as->smmu = smmu; as->use_count++; + mutex_unlock(&smmu->lock); + return 0; err_unmap: dma_unmap_page(smmu->dev, as->pd_dma, SMMU_SIZE_PD, DMA_TO_DEVICE); +unlock: + mutex_unlock(&smmu->lock); + return err; } static void tegra_smmu_as_unprepare(struct tegra_smmu *smmu, struct tegra_smmu_as *as) { - if (--as->use_count > 0) + mutex_lock(&smmu->lock); + + if (--as->use_count > 0) { + mutex_unlock(&smmu->lock); return; + } tegra_smmu_free_asid(smmu, as->id); dma_unmap_page(smmu->dev, as->pd_dma, SMMU_SIZE_PD, DMA_TO_DEVICE); as->smmu = NULL; + + mutex_unlock(&smmu->lock); } static int tegra_smmu_attach_dev(struct iommu_domain *domain, -- 2.17.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v3 2/2] iommu/tegra-smmu: Expend mutex protection range @ 2020-09-29 4:52 ` Nicolin Chen 0 siblings, 0 replies; 10+ messages in thread From: Nicolin Chen @ 2020-09-29 4:52 UTC (permalink / raw) To: thierry.reding, joro; +Cc: linux-kernel, iommu, jonathanh, linux-tegra, digetx This is used to protect potential race condition at use_count. since probes of client drivers, calling attach_dev(), may run concurrently. Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com> --- Changelog v2->v3: * Renamed label "err_unlock" to "unlock" v1->v2: * N/A drivers/iommu/tegra-smmu.c | 34 +++++++++++++++++++++------------- 1 file changed, 21 insertions(+), 13 deletions(-) diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c index ec4c9dafff95..6a3ecc334481 100644 --- a/drivers/iommu/tegra-smmu.c +++ b/drivers/iommu/tegra-smmu.c @@ -256,26 +256,19 @@ static int tegra_smmu_alloc_asid(struct tegra_smmu *smmu, unsigned int *idp) { unsigned long id; - mutex_lock(&smmu->lock); - id = find_first_zero_bit(smmu->asids, smmu->soc->num_asids); - if (id >= smmu->soc->num_asids) { - mutex_unlock(&smmu->lock); + if (id >= smmu->soc->num_asids) return -ENOSPC; - } set_bit(id, smmu->asids); *idp = id; - mutex_unlock(&smmu->lock); return 0; } static void tegra_smmu_free_asid(struct tegra_smmu *smmu, unsigned int id) { - mutex_lock(&smmu->lock); clear_bit(id, smmu->asids); - mutex_unlock(&smmu->lock); } static bool tegra_smmu_capable(enum iommu_cap cap) @@ -420,17 +413,21 @@ static int tegra_smmu_as_prepare(struct tegra_smmu *smmu, struct tegra_smmu_as *as) { u32 value; - int err; + int err = 0; + + mutex_lock(&smmu->lock); if (as->use_count > 0) { as->use_count++; - return 0; + goto unlock; } as->pd_dma = dma_map_page(smmu->dev, as->pd, 0, SMMU_SIZE_PD, DMA_TO_DEVICE); - if (dma_mapping_error(smmu->dev, as->pd_dma)) - return -ENOMEM; + if (dma_mapping_error(smmu->dev, as->pd_dma)) { + err = -ENOMEM; + goto unlock; + } /* We can't handle 64-bit DMA addresses */ if (!smmu_dma_addr_valid(smmu, as->pd_dma)) { @@ -453,24 +450,35 @@ static int tegra_smmu_as_prepare(struct tegra_smmu *smmu, as->smmu = smmu; as->use_count++; + mutex_unlock(&smmu->lock); + return 0; err_unmap: dma_unmap_page(smmu->dev, as->pd_dma, SMMU_SIZE_PD, DMA_TO_DEVICE); +unlock: + mutex_unlock(&smmu->lock); + return err; } static void tegra_smmu_as_unprepare(struct tegra_smmu *smmu, struct tegra_smmu_as *as) { - if (--as->use_count > 0) + mutex_lock(&smmu->lock); + + if (--as->use_count > 0) { + mutex_unlock(&smmu->lock); return; + } tegra_smmu_free_asid(smmu, as->id); dma_unmap_page(smmu->dev, as->pd_dma, SMMU_SIZE_PD, DMA_TO_DEVICE); as->smmu = NULL; + + mutex_unlock(&smmu->lock); } static int tegra_smmu_attach_dev(struct iommu_domain *domain, -- 2.17.1 _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v3 2/2] iommu/tegra-smmu: Expend mutex protection range 2020-09-29 4:52 ` Nicolin Chen @ 2020-09-29 6:03 ` Christoph Hellwig -1 siblings, 0 replies; 10+ messages in thread From: Christoph Hellwig @ 2020-09-29 6:03 UTC (permalink / raw) To: Nicolin Chen Cc: thierry.reding, joro, linux-kernel, iommu, jonathanh, linux-tegra, digetx On Mon, Sep 28, 2020 at 09:52:47PM -0700, Nicolin Chen wrote: > This is used to protect potential race condition at use_count. > since probes of client drivers, calling attach_dev(), may run > concurrently. Shouldn't this read "expand" instead of "expend"? ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3 2/2] iommu/tegra-smmu: Expend mutex protection range @ 2020-09-29 6:03 ` Christoph Hellwig 0 siblings, 0 replies; 10+ messages in thread From: Christoph Hellwig @ 2020-09-29 6:03 UTC (permalink / raw) To: Nicolin Chen Cc: linux-kernel, jonathanh, iommu, thierry.reding, linux-tegra, digetx On Mon, Sep 28, 2020 at 09:52:47PM -0700, Nicolin Chen wrote: > This is used to protect potential race condition at use_count. > since probes of client drivers, calling attach_dev(), may run > concurrently. Shouldn't this read "expand" instead of "expend"? _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3 2/2] iommu/tegra-smmu: Expend mutex protection range 2020-09-29 6:03 ` Christoph Hellwig @ 2020-09-29 6:02 ` Nicolin Chen -1 siblings, 0 replies; 10+ messages in thread From: Nicolin Chen @ 2020-09-29 6:02 UTC (permalink / raw) To: Christoph Hellwig Cc: thierry.reding, joro, linux-kernel, iommu, jonathanh, linux-tegra, digetx On Tue, Sep 29, 2020 at 07:03:36AM +0100, Christoph Hellwig wrote: > On Mon, Sep 28, 2020 at 09:52:47PM -0700, Nicolin Chen wrote: > > This is used to protect potential race condition at use_count. > > since probes of client drivers, calling attach_dev(), may run > > concurrently. > > Shouldn't this read "expand" instead of "expend"? Oops...my poor English :) Fixing.... Thanks! ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3 2/2] iommu/tegra-smmu: Expend mutex protection range @ 2020-09-29 6:02 ` Nicolin Chen 0 siblings, 0 replies; 10+ messages in thread From: Nicolin Chen @ 2020-09-29 6:02 UTC (permalink / raw) To: Christoph Hellwig Cc: linux-kernel, jonathanh, iommu, thierry.reding, linux-tegra, digetx On Tue, Sep 29, 2020 at 07:03:36AM +0100, Christoph Hellwig wrote: > On Mon, Sep 28, 2020 at 09:52:47PM -0700, Nicolin Chen wrote: > > This is used to protect potential race condition at use_count. > > since probes of client drivers, calling attach_dev(), may run > > concurrently. > > Shouldn't this read "expand" instead of "expend"? Oops...my poor English :) Fixing.... Thanks! _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-09-29 6:08 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-09-29 4:52 [PATCH v3 0/2] iommu/tegra-smmu: Two followup changes Nicolin Chen 2020-09-29 4:52 ` Nicolin Chen 2020-09-29 4:52 ` [PATCH v3 1/2] iommu/tegra-smmu: Unwrap tegra_smmu_group_get Nicolin Chen 2020-09-29 4:52 ` Nicolin Chen 2020-09-29 4:52 ` [PATCH v3 2/2] iommu/tegra-smmu: Expend mutex protection range Nicolin Chen 2020-09-29 4:52 ` Nicolin Chen 2020-09-29 6:03 ` Christoph Hellwig 2020-09-29 6:03 ` Christoph Hellwig 2020-09-29 6:02 ` Nicolin Chen 2020-09-29 6:02 ` Nicolin Chen
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.