[PATCH 1/2] iommu/tegra: smmu: Simplify allocation at once

[PATCH 1/2] iommu/tegra: smmu: Simplify allocation at once

[Hiroshi DOYU]

To simplify the code, alloc necessary data at once.

Signed-off-by: Hiroshi DOYU <hdoyu@nvidia.com>
---
This patch requires:

    [PATCH 1/2] iommu/tegra: smmu: Add device tree support for SMMU
        http://marc.info/?l=linux-tegra&m=133663641107327&w=2

Also the above patch requires:

    [PATCH 1/1] dt: Add general DMA window parser
        http://marc.info/?l=linux-tegra&m=133671302703840&w=2
---
 drivers/iommu/tegra-smmu.c |   29 +++++++++--------------------
 1 files changed, 9 insertions(+), 20 deletions(-)

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index dbaba77..e4acd44 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -241,8 +241,6 @@ struct smmu_device {
 	spinlock_t	lock;
 	char		*name;
 	struct device	*dev;
-	int		num_as;
-	struct smmu_as	*as;		/* Run-time allocated array */
 	struct page *avp_vector_page;	/* dummy page shared by all AS's */
 
 	/*
@@ -254,6 +252,9 @@ struct smmu_device {
 	unsigned long asid_security;
 
 	struct device_node *ahb;
+
+	int		num_as;
+	struct smmu_as	as[0];		/* Run-time allocated array */
 };
 
 static struct smmu_device *smmu_handle; /* unique for a system */
@@ -902,15 +903,18 @@ static int tegra_smmu_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	int i, asids, err = 0;
 	dma_addr_t base;
-	size_t size;
-	const void *prop;
+	size_t bytes, size;
 
 	if (smmu_handle)
 		return -EIO;
 
 	BUILD_BUG_ON(PAGE_SHIFT != SMMU_PAGE_SHIFT);
 
-	smmu = devm_kzalloc(dev, sizeof(*smmu), GFP_KERNEL);
+	if (of_property_read_u32(dev->of_node, "nvidia,#asids", &asids))
+		return -ENODEV;
+
+	bytes = sizeof(*smmu) + asids * sizeof(*smmu->as);
+	smmu = devm_kzalloc(dev, bytes, GFP_KERNEL);
 	if (!smmu) {
 		dev_err(dev, "failed to allocate smmu_device\n");
 		return -ENOMEM;
@@ -939,13 +943,6 @@ static int tegra_smmu_probe(struct platform_device *pdev)
 	if (!size)
 		return -EINVAL;
 
-	prop = of_get_property(dev->of_node, "nvidia,#asids", NULL);
-	if (!prop)
-		return -ENODEV;
-	asids = be32_to_cpup(prop);
-	if (!asids)
-		return -ENODEV;
-
 	smmu->ahb = of_parse_phandle(dev->of_node, "nvidia,ahb", 0);
 	if (!smmu->ahb)
 		return -ENODEV;
@@ -960,14 +957,6 @@ static int tegra_smmu_probe(struct platform_device *pdev)
 	smmu->translation_enable_2 = ~0;
 	smmu->asid_security = 0;
 
-	smmu->as = devm_kzalloc(dev,
-			sizeof(smmu->as[0]) * smmu->num_as, GFP_KERNEL);
-	if (!smmu->as) {
-		dev_err(dev, "failed to allocate smmu_as\n");
-		err = -ENOMEM;
-		goto fail;
-	}
-
 	for (i = 0; i < smmu->num_as; i++) {
 		struct smmu_as *as = &smmu->as[i];
 
-- 
1.7.5.4

[PATCH 2/2] iommu/tegra: smmu: Remove unnecessary cleanups with devm_*()

[Hiroshi DOYU]

Remove unnecessary cleanup procedures with devm_*() functions.

Signed-off-by: Hiroshi DOYU <hdoyu@nvidia.com>
---
 drivers/iommu/tegra-smmu.c |   37 ++++++-------------------------------
 1 files changed, 6 insertions(+), 31 deletions(-)

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index e4acd44..9d0cc33 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -972,51 +972,26 @@ static int tegra_smmu_probe(struct platform_device *pdev)
 	spin_lock_init(&smmu->lock);
 	err = smmu_setup_regs(smmu);
 	if (err)
-		goto fail;
+		return err;
 	platform_set_drvdata(pdev, smmu);
 
 	smmu->avp_vector_page = alloc_page(GFP_KERNEL);
 	if (!smmu->avp_vector_page)
-		goto fail;
+		return -ENOMEM;
 
 	smmu_handle = smmu;
 	return 0;
-
-fail:
-	if (smmu->avp_vector_page)
-		__free_page(smmu->avp_vector_page);
-	if (smmu && smmu->as) {
-		for (i = 0; i < smmu->num_as; i++) {
-			if (smmu->as[i].pdir_page) {
-				ClearPageReserved(smmu->as[i].pdir_page);
-				__free_page(smmu->as[i].pdir_page);
-			}
-		}
-		devm_kfree(dev, smmu->as);
-	}
-	devm_kfree(dev, smmu);
-	return err;
 }
 
 static int tegra_smmu_remove(struct platform_device *pdev)
 {
 	struct smmu_device *smmu = platform_get_drvdata(pdev);
-	struct device *dev = smmu->dev;
+	int i;
 
 	smmu_write(smmu, SMMU_CONFIG_DISABLE, SMMU_CONFIG);
-	platform_set_drvdata(pdev, NULL);
-	if (smmu->as) {
-		int i;
-
-		for (i = 0; i < smmu->num_as; i++)
-			free_pdir(&smmu->as[i]);
-		devm_kfree(dev, smmu->as);
-	}
-	if (smmu->avp_vector_page)
-		__free_page(smmu->avp_vector_page);
-	if (smmu->regs)
-		devm_iounmap(dev, smmu->regs);
-	devm_kfree(dev, smmu);
+	for (i = 0; i < smmu->num_as; i++)
+		free_pdir(&smmu->as[i]);
+	__free_page(smmu->avp_vector_page);
 	smmu_handle = NULL;
 	return 0;
 }
-- 
1.7.5.4

Re: [PATCH 1/2] iommu/tegra: smmu: Simplify allocation at once

[Stephen Warren]

On 05/14/2012 01:16 PM, Hiroshi DOYU wrote:
> To simplify the code, alloc necessary data at once.
> 
> Signed-off-by: Hiroshi DOYU <hdoyu@nvidia.com>
> ---
> This patch requires:
> 
>     [PATCH 1/2] iommu/tegra: smmu: Add device tree support for SMMU
>         http://marc.info/?l=linux-tegra&m=133663641107327&w=2
> 
> Also the above patch requires:
> 
>     [PATCH 1/1] dt: Add general DMA window parser
>         http://marc.info/?l=linux-tegra&m=133671302703840&w=2

I know I've been harping on about dependencies, but you typically only
need to mention them if the dependencies are not already checked into
the branch you expect this patch to be checked into.

> -	smmu = devm_kzalloc(dev, sizeof(*smmu), GFP_KERNEL);
> +	if (of_property_read_u32(dev->of_node, "nvidia,#asids", &asids))
> +		return -ENODEV;

I believe you need to change the asids variable from int to u32 to avoid
a warning here.

Aside from that, the series,
Acked-by: Stephen Warren <swarren@wwwdotorg.org>

Re: [PATCH 1/2] iommu/tegra: smmu: Simplify allocation at once

[Hiroshi Doyu]

Stephen Warren <swarren@wwwdotorg.org> wrote @ Tue, 15 May 2012 01:34:15 +0200:

> On 05/14/2012 01:16 PM, Hiroshi DOYU wrote:
> > To simplify the code, alloc necessary data at once.
> > 
> > Signed-off-by: Hiroshi DOYU <hdoyu@nvidia.com>
> > ---
> > This patch requires:
> > 
> >     [PATCH 1/2] iommu/tegra: smmu: Add device tree support for SMMU
> >         http://marc.info/?l=linux-tegra&m=133663641107327&w=2
> > 
> > Also the above patch requires:
> > 
> >     [PATCH 1/1] dt: Add general DMA window parser
> >         http://marc.info/?l=linux-tegra&m=133671302703840&w=2
> 
> I know I've been harping on about dependencies, but you typically only
> need to mention them if the dependencies are not already checked into
> the branch you expect this patch to be checked into.

There was a bit time gap. The former one is merged now. The latter
dt/DMA window patch isn't checked in.

[v2 1/2] iommu/tegra: smmu: Simplify allocation at once

[Hiroshi DOYU]

To simplify the code, alloc necessary data at once.

Signed-off-by: Hiroshi DOYU <hdoyu@nvidia.com>
Acked-by: Stephen Warren <swarren@wwwdotorg.org>
---
This patch requires:

    [PATCH 1/1] dt: Add general DMA window parser
        http://marc.info/?l=linux-tegra&m=133671302703840&w=2
---
 drivers/iommu/tegra-smmu.c |   32 +++++++++++---------------------
 1 files changed, 11 insertions(+), 21 deletions(-)

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index dbaba77..5eb32b3 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -241,8 +241,6 @@ struct smmu_device {
 	spinlock_t	lock;
 	char		*name;
 	struct device	*dev;
-	int		num_as;
-	struct smmu_as	*as;		/* Run-time allocated array */
 	struct page *avp_vector_page;	/* dummy page shared by all AS's */
 
 	/*
@@ -254,6 +252,9 @@ struct smmu_device {
 	unsigned long asid_security;
 
 	struct device_node *ahb;
+
+	int		num_as;
+	struct smmu_as	as[0];		/* Run-time allocated array */
 };
 
 static struct smmu_device *smmu_handle; /* unique for a system */
@@ -900,17 +901,21 @@ static int tegra_smmu_probe(struct platform_device *pdev)
 {
 	struct smmu_device *smmu;
 	struct device *dev = &pdev->dev;
-	int i, asids, err = 0;
+	int i, err = 0;
+	u32 asids;
 	dma_addr_t base;
-	size_t size;
-	const void *prop;
+	size_t bytes, size;
 
 	if (smmu_handle)
 		return -EIO;
 
 	BUILD_BUG_ON(PAGE_SHIFT != SMMU_PAGE_SHIFT);
 
-	smmu = devm_kzalloc(dev, sizeof(*smmu), GFP_KERNEL);
+	if (of_property_read_u32(dev->of_node, "nvidia,#asids", &asids))
+		return -ENODEV;
+
+	bytes = sizeof(*smmu) + asids * sizeof(*smmu->as);
+	smmu = devm_kzalloc(dev, bytes, GFP_KERNEL);
 	if (!smmu) {
 		dev_err(dev, "failed to allocate smmu_device\n");
 		return -ENOMEM;
@@ -939,13 +944,6 @@ static int tegra_smmu_probe(struct platform_device *pdev)
 	if (!size)
 		return -EINVAL;
 
-	prop = of_get_property(dev->of_node, "nvidia,#asids", NULL);
-	if (!prop)
-		return -ENODEV;
-	asids = be32_to_cpup(prop);
-	if (!asids)
-		return -ENODEV;
-
 	smmu->ahb = of_parse_phandle(dev->of_node, "nvidia,ahb", 0);
 	if (!smmu->ahb)
 		return -ENODEV;
@@ -960,14 +958,6 @@ static int tegra_smmu_probe(struct platform_device *pdev)
 	smmu->translation_enable_2 = ~0;
 	smmu->asid_security = 0;
 
-	smmu->as = devm_kzalloc(dev,
-			sizeof(smmu->as[0]) * smmu->num_as, GFP_KERNEL);
-	if (!smmu->as) {
-		dev_err(dev, "failed to allocate smmu_as\n");
-		err = -ENOMEM;
-		goto fail;
-	}
-
 	for (i = 0; i < smmu->num_as; i++) {
 		struct smmu_as *as = &smmu->as[i];
 
-- 
1.7.5.4

[v2 2/2] iommu/tegra: smmu: Remove unnecessary cleanups with devm_*()

[Hiroshi DOYU]

Remove unnecessary cleanup procedures with devm_*() functions.

Signed-off-by: Hiroshi DOYU <hdoyu@nvidia.com>
Acked-by: Stephen Warren <swarren@wwwdotorg.org>
---
 drivers/iommu/tegra-smmu.c |   37 ++++++-------------------------------
 1 files changed, 6 insertions(+), 31 deletions(-)

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index 5eb32b3..9c2a99e 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -973,51 +973,26 @@ static int tegra_smmu_probe(struct platform_device *pdev)
 	spin_lock_init(&smmu->lock);
 	err = smmu_setup_regs(smmu);
 	if (err)
-		goto fail;
+		return err;
 	platform_set_drvdata(pdev, smmu);
 
 	smmu->avp_vector_page = alloc_page(GFP_KERNEL);
 	if (!smmu->avp_vector_page)
-		goto fail;
+		return -ENOMEM;
 
 	smmu_handle = smmu;
 	return 0;
-
-fail:
-	if (smmu->avp_vector_page)
-		__free_page(smmu->avp_vector_page);
-	if (smmu && smmu->as) {
-		for (i = 0; i < smmu->num_as; i++) {
-			if (smmu->as[i].pdir_page) {
-				ClearPageReserved(smmu->as[i].pdir_page);
-				__free_page(smmu->as[i].pdir_page);
-			}
-		}
-		devm_kfree(dev, smmu->as);
-	}
-	devm_kfree(dev, smmu);
-	return err;
 }
 
 static int tegra_smmu_remove(struct platform_device *pdev)
 {
 	struct smmu_device *smmu = platform_get_drvdata(pdev);
-	struct device *dev = smmu->dev;
+	int i;
 
 	smmu_write(smmu, SMMU_CONFIG_DISABLE, SMMU_CONFIG);
-	platform_set_drvdata(pdev, NULL);
-	if (smmu->as) {
-		int i;
-
-		for (i = 0; i < smmu->num_as; i++)
-			free_pdir(&smmu->as[i]);
-		devm_kfree(dev, smmu->as);
-	}
-	if (smmu->avp_vector_page)
-		__free_page(smmu->avp_vector_page);
-	if (smmu->regs)
-		devm_iounmap(dev, smmu->regs);
-	devm_kfree(dev, smmu);
+	for (i = 0; i < smmu->num_as; i++)
+		free_pdir(&smmu->as[i]);
+	__free_page(smmu->avp_vector_page);
 	smmu_handle = NULL;
 	return 0;
 }
-- 
1.7.5.4

Re: [PATCH 1/2] iommu/tegra: smmu: Simplify allocation at once

[Hiroshi Doyu]

Stephen Warren <swarren@wwwdotorg.org> wrote @ Tue, 15 May 2012 01:34:15 +0200:

> On 05/14/2012 01:16 PM, Hiroshi DOYU wrote:
> > To simplify the code, alloc necessary data at once.
> > 
> > Signed-off-by: Hiroshi DOYU <hdoyu@nvidia.com>
> > ---
> > This patch requires:
> > 
> >     [PATCH 1/2] iommu/tegra: smmu: Add device tree support for SMMU
> >         http://marc.info/?l=linux-tegra&m=133663641107327&w=2
> > 
> > Also the above patch requires:
> > 
> >     [PATCH 1/1] dt: Add general DMA window parser
> >         http://marc.info/?l=linux-tegra&m=133671302703840&w=2
> 
> I know I've been harping on about dependencies, but you typically only
> need to mention them if the dependencies are not already checked into
> the branch you expect this patch to be checked into.
> 
> > -	smmu = devm_kzalloc(dev, sizeof(*smmu), GFP_KERNEL);
> > +	if (of_property_read_u32(dev->of_node, "nvidia,#asids", &asids))
> > +		return -ENODEV;
> 
> I believe you need to change the asids variable from int to u32 to avoid
> a warning here.

There's no warning but it's allowed because of "-Wno-pointer-sign". It
seems that this flag has been used for long in kernel, and there are
many places. Is this conversion allowed because it's expeced that the
author should know the range of value, or any other reason?

*1:
-Wno-pointer-sign
    Don't warn for pointer argument passing or assignment with
    different signedness. Only useful in the negative form since this
    warning is enabled by default. This option is only supported for C
    and Objective-C.

Re: [PATCH 1/2] iommu/tegra: smmu: Simplify allocation at once

[Stephen Warren]

On 05/15/2012 02:26 AM, Hiroshi Doyu wrote:
> Stephen Warren <swarren@wwwdotorg.org> wrote @ Tue, 15 May 2012 01:34:15 +0200:
> 
>> On 05/14/2012 01:16 PM, Hiroshi DOYU wrote:
>>> To simplify the code, alloc necessary data at once.
>>>
>>> Signed-off-by: Hiroshi DOYU <hdoyu@nvidia.com>
>>> ---
>>> This patch requires:
>>>
>>>     [PATCH 1/2] iommu/tegra: smmu: Add device tree support for SMMU
>>>         http://marc.info/?l=linux-tegra&m=133663641107327&w=2
>>>
>>> Also the above patch requires:
>>>
>>>     [PATCH 1/1] dt: Add general DMA window parser
>>>         http://marc.info/?l=linux-tegra&m=133671302703840&w=2
>>
>> I know I've been harping on about dependencies, but you typically only
>> need to mention them if the dependencies are not already checked into
>> the branch you expect this patch to be checked into.
>>
>>> -	smmu = devm_kzalloc(dev, sizeof(*smmu), GFP_KERNEL);
>>> +	if (of_property_read_u32(dev->of_node, "nvidia,#asids", &asids))
>>> +		return -ENODEV;
>>
>> I believe you need to change the asids variable from int to u32 to avoid
>> a warning here.
> 
> There's no warning but it's allowed because of "-Wno-pointer-sign". It

That's odd. I'm sure I have seen this warning recently when calling this
API, but you're right, I'm not able to trigger that warning right now,
so this is fine.

Re: [PATCH 1/2] iommu/tegra: smmu: Simplify allocation at once

[Hiroshi Doyu]

Stephen Warren <swarren@wwwdotorg.org> wrote @ Tue, 15 May 2012 18:09:13 +0200:

> On 05/15/2012 02:26 AM, Hiroshi Doyu wrote:
> > Stephen Warren <swarren@wwwdotorg.org> wrote @ Tue, 15 May 2012 01:34:15 +0200:
> > 
> >> On 05/14/2012 01:16 PM, Hiroshi DOYU wrote:
> >>> To simplify the code, alloc necessary data at once.
> >>>
> >>> Signed-off-by: Hiroshi DOYU <hdoyu@nvidia.com>
> >>> ---
> >>> This patch requires:
> >>>
> >>>     [PATCH 1/2] iommu/tegra: smmu: Add device tree support for SMMU
> >>>         http://marc.info/?l=linux-tegra&m=133663641107327&w=2
> >>>
> >>> Also the above patch requires:
> >>>
> >>>     [PATCH 1/1] dt: Add general DMA window parser
> >>>         http://marc.info/?l=linux-tegra&m=133671302703840&w=2
> >>
> >> I know I've been harping on about dependencies, but you typically only
> >> need to mention them if the dependencies are not already checked into
> >> the branch you expect this patch to be checked into.
> >>
> >>> -	smmu = devm_kzalloc(dev, sizeof(*smmu), GFP_KERNEL);
> >>> +	if (of_property_read_u32(dev->of_node, "nvidia,#asids", &asids))
> >>> +		return -ENODEV;
> >>
> >> I believe you need to change the asids variable from int to u32 to avoid
> >> a warning here.
> > 
> > There's no warning but it's allowed because of "-Wno-pointer-sign". It
> 
> That's odd. I'm sure I have seen this warning recently when calling this
> API, but you're right, I'm not able to trigger that warning right now,
> so this is fine.

Then, either [PATCH v1] or [PATCH v2] would be ok, but v2 may be a bit
better because it's exacltly compatible with that function prototype,
of_property_read_u32(...u32 *).