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
v3->v4:
* PATCH-2: Fixed typo in subject
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: Expand mutex protection range
drivers/iommu/tegra-smmu.c | 53 ++++++++++++++++++--------------------
1 file changed, 25 insertions(+), 28 deletions(-)
--
2.17.1
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 <[email protected]>
---
Changelog
v2->v4:
* 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
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 <[email protected]>
---
Changelog
v3->v4:
* Fixed typo "Expend" => "Expand"
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
29.09.2020 09:13, Nicolin Chen пишет:
> 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 <[email protected]>
> ---
It's always better not to mix success and error code paths in order to
keep code readable, but not a big deal in the case of this particular
patch since the changed code is quite simple. Please try to avoid doing
this in the future patches.
Also, please note that in general it's better to use locked/unlocked
versions for the functions like it's already done for
tegra_smmu_map/unmap, this will remove the need to maintain lockings in
the code. The code touched by this patch doesn't have complicated code
paths, so it's good enough to me.
Reviewed-by: Dmitry Osipenko <[email protected]>
29.09.2020 09:13, Nicolin Chen пишет:
> 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 <[email protected]>
> ---
Reviewed-by: Dmitry Osipenko <[email protected]>
On Tue, Sep 29, 2020 at 08:42:52PM +0300, Dmitry Osipenko wrote:
> 29.09.2020 09:13, Nicolin Chen пишет:
> > 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 <[email protected]>
> > ---
>
> It's always better not to mix success and error code paths in order to
> keep code readable, but not a big deal in the case of this particular
> patch since the changed code is quite simple. Please try to avoid doing
> this in the future patches.
>
> Also, please note that in general it's better to use locked/unlocked
> versions for the functions like it's already done for
> tegra_smmu_map/unmap, this will remove the need to maintain lockings in
> the code. The code touched by this patch doesn't have complicated code
> paths, so it's good enough to me.
>
> Reviewed-by: Dmitry Osipenko <[email protected]>
Okay. Thanks for the review!
29.09.2020 20:41, Dmitry Osipenko пишет:
> 29.09.2020 09:13, Nicolin Chen пишет:
>> 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 <[email protected]>
>> ---
>
> Reviewed-by: Dmitry Osipenko <[email protected]>
>
Tested-by: Dmitry Osipenko <[email protected]>
29.09.2020 20:42, Dmitry Osipenko пишет:
> 29.09.2020 09:13, Nicolin Chen пишет:
>> 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 <[email protected]>
>> ---
>
> It's always better not to mix success and error code paths in order to
> keep code readable, but not a big deal in the case of this particular
> patch since the changed code is quite simple. Please try to avoid doing
> this in the future patches.
>
> Also, please note that in general it's better to use locked/unlocked
> versions for the functions like it's already done for
> tegra_smmu_map/unmap, this will remove the need to maintain lockings in
> the code. The code touched by this patch doesn't have complicated code
> paths, so it's good enough to me.
>
> Reviewed-by: Dmitry Osipenko <[email protected]>
>
Tested-by: Dmitry Osipenko <[email protected]>
On Mon, Sep 28, 2020 at 11:13:25PM -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.
>
> Signed-off-by: Nicolin Chen <[email protected]>
> ---
>
> Changelog
> v3->v4:
> * Fixed typo "Expend" => "Expand"
> 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(-)
Acked-by: Thierry Reding <[email protected]>
On Mon, Sep 28, 2020 at 11:13:24PM -0700, Nicolin Chen wrote:
> 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 <[email protected]>
> ---
>
> Changelog
> v2->v4:
> * 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(-)
Acked-by: Thierry Reding <[email protected]>
On Mon, Sep 28, 2020 at 11:13:23PM -0700, Nicolin Chen wrote:
> 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
These two changes have Acked-by and Reviewed-by for a month already.
Who can apply them?
Thanks
Nic
> Changelog
> v3->v4:
> * PATCH-2: Fixed typo in subject
> 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: Expand mutex protection range
>
> drivers/iommu/tegra-smmu.c | 53 ++++++++++++++++++--------------------
> 1 file changed, 25 insertions(+), 28 deletions(-)
>
> --
> 2.17.1
>