diff mbox series

[1/5] iommu/tegra-smmu: Unwrap tegra_smmu_group_get

Message ID 20200926080719.6822-2-nicoleotsuka@gmail.com
State New
Headers show
Series iommu/tegra-smmu: Adding PCI support and mappings debugfs node | expand

Commit Message

Nicolin Chen Sept. 26, 2020, 8:07 a.m. UTC
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>
---
 drivers/iommu/tegra-smmu.c | 19 ++++---------------
 1 file changed, 4 insertions(+), 15 deletions(-)

Comments

Thierry Reding Sept. 28, 2020, 7:13 a.m. UTC | #1
On Sat, Sep 26, 2020 at 01:07:15AM -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 <nicoleotsuka@gmail.com>
> ---
>  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..6335285dc373 100644
> --- a/drivers/iommu/tegra-smmu.c
> +++ b/drivers/iommu/tegra-smmu.c
> @@ -903,11 +903,13 @@ 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;
>  	struct tegra_smmu_group *group;
> +	int swgroup = fwspec->ids[0];

This should be unsigned int to match the type of swgroup elsewhere.
Also, it might not be worth having an extra local variable for this
since it's only used once.

Thierry
Nicolin Chen Sept. 28, 2020, 7:33 p.m. UTC | #2
Hi Thierry,

Thanks for the review.

On Mon, Sep 28, 2020 at 09:13:56AM +0200, Thierry Reding wrote:
> > -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;
> >  	struct tegra_smmu_group *group;
> > +	int swgroup = fwspec->ids[0];
> 
> This should be unsigned int to match the type of swgroup elsewhere.
> Also, it might not be worth having an extra local variable for this
> since it's only used once.

Will change to unsigned int. There are actually a few places using
it in this function, previously tegra_smmu_group_get().
diff mbox series

Patch

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index 0becdbfea306..6335285dc373 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -903,11 +903,13 @@  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;
 	struct tegra_smmu_group *group;
+	int swgroup = fwspec->ids[0];
 	struct iommu_group *grp;
 
 	/* Find group_soc associating with swgroup */
@@ -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)
 {