[PATCH 0/2] iommu/tegra*: Add support for struct iommu_device

[PATCH 0/2] iommu/tegra*: Add support for struct iommu_device

[Joerg Roedel]

Hi,

here are two patches to add support for 'struct iommu_device'
to the tegra iommu-drivers. This will make the iommu-core
code aware of the hardware iommus that a driver manages.

It will also add the iommus to sysfs and link them to the
devices managed by them.

The patches apply on-top of Robin's iommu-group patches.

Please review.

Regards,

	Joerg

Joerg Roedel (2):
  iommu/tegra: Add support for struct iommu_device
  iommu/tegra-gart: Add support for struct iommu_device

 drivers/iommu/tegra-gart.c | 26 ++++++++++++++++++++++++++
 drivers/iommu/tegra-smmu.c | 25 +++++++++++++++++++++++++
 2 files changed, 51 insertions(+)

-- 
2.7.4

[PATCH 1/2] iommu/tegra: Add support for struct iommu_device

[Joerg Roedel]

From: Joerg Roedel <jroedel@suse.de>

Add a struct iommu_device to each tegra-smmu and register it
with the iommu-core. Also link devices added to the driver
to their respective hardware iommus.

Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
 drivers/iommu/tegra-smmu.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index faa9c1e..2802e12 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -36,6 +36,8 @@ struct tegra_smmu {
 	struct list_head list;
 
 	struct dentry *debugfs;
+
+	struct iommu_device iommu;	/* IOMMU Core code handle */
 };
 
 struct tegra_smmu_as {
@@ -720,6 +722,9 @@ static int tegra_smmu_add_device(struct device *dev)
 			 * first match.
 			 */
 			dev->archdata.iommu = smmu;
+
+			iommu_device_link(&smmu->iommu, dev);
+
 			break;
 		}
 
@@ -737,6 +742,11 @@ static int tegra_smmu_add_device(struct device *dev)
 
 static void tegra_smmu_remove_device(struct device *dev)
 {
+	struct tegra_smmu *smmu = dev->archdata.iommu;
+
+	if (smmu)
+		iommu_device_unlink(&smmu->iommu, dev);
+
 	dev->archdata.iommu = NULL;
 	iommu_group_remove_device(dev);
 }
@@ -943,6 +953,18 @@ struct tegra_smmu *tegra_smmu_probe(struct device *dev,
 	if (err < 0)
 		return ERR_PTR(err);
 
+	err = iommu_device_sysfs_add(&smmu->iommu, dev, NULL, dev_name(dev));
+	if (err)
+		return ERR_PTR(err);
+
+	iommu_device_set_ops(&smmu->iommu, &tegra_smmu_ops);
+
+	err = iommu_device_register(&smmu->iommu);
+	if (err) {
+		iommu_device_sysfs_remove(&smmu->iommu);
+		return ERR_PTR(err);
+	}
+
 	if (IS_ENABLED(CONFIG_DEBUG_FS))
 		tegra_smmu_debugfs_init(smmu);
 
@@ -951,6 +973,9 @@ struct tegra_smmu *tegra_smmu_probe(struct device *dev,
 
 void tegra_smmu_remove(struct tegra_smmu *smmu)
 {
+	iommu_device_unregister(&smmu->iommu);
+	iommu_device_sysfs_remove(&smmu->iommu);
+
 	if (IS_ENABLED(CONFIG_DEBUG_FS))
 		tegra_smmu_debugfs_exit(smmu);
 }
-- 
2.7.4

Re: [PATCH 1/2] iommu/tegra: Add support for struct iommu_device

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 552 bytes --]

On Thu, Aug 10, 2017 at 12:29:11AM +0200, Joerg Roedel wrote:
> From: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>
> 
> Add a struct iommu_device to each tegra-smmu and register it
> with the iommu-core. Also link devices added to the driver
> to their respective hardware iommus.
> 
> Signed-off-by: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>
> ---
>  drivers/iommu/tegra-smmu.c | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)

Acked-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/2] iommu/tegra: Add support for struct iommu_device

[Jon Hunter]

On 09/08/17 23:29, Joerg Roedel wrote:
> From: Joerg Roedel <jroedel@suse.de>
> 
> Add a struct iommu_device to each tegra-smmu and register it
> with the iommu-core. Also link devices added to the driver
> to their respective hardware iommus.
> 
> Signed-off-by: Joerg Roedel <jroedel@suse.de>
> ---
>  drivers/iommu/tegra-smmu.c | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
> index faa9c1e..2802e12 100644
> --- a/drivers/iommu/tegra-smmu.c
> +++ b/drivers/iommu/tegra-smmu.c
> @@ -36,6 +36,8 @@ struct tegra_smmu {
>  	struct list_head list;
>  
>  	struct dentry *debugfs;
> +
> +	struct iommu_device iommu;	/* IOMMU Core code handle */
>  };
>  
>  struct tegra_smmu_as {
> @@ -720,6 +722,9 @@ static int tegra_smmu_add_device(struct device *dev)
>  			 * first match.
>  			 */
>  			dev->archdata.iommu = smmu;
> +
> +			iommu_device_link(&smmu->iommu, dev);
> +
>  			break;
>  		}
>  
> @@ -737,6 +742,11 @@ static int tegra_smmu_add_device(struct device *dev)
>  
>  static void tegra_smmu_remove_device(struct device *dev)
>  {
> +	struct tegra_smmu *smmu = dev->archdata.iommu;
> +
> +	if (smmu)
> +		iommu_device_unlink(&smmu->iommu, dev);
> +
>  	dev->archdata.iommu = NULL;
>  	iommu_group_remove_device(dev);
>  }
> @@ -943,6 +953,18 @@ struct tegra_smmu *tegra_smmu_probe(struct device *dev,
>  	if (err < 0)
>  		return ERR_PTR(err);
>  
> +	err = iommu_device_sysfs_add(&smmu->iommu, dev, NULL, dev_name(dev));
> +	if (err)
> +		return ERR_PTR(err);
> +
> +	iommu_device_set_ops(&smmu->iommu, &tegra_smmu_ops);
> +
> +	err = iommu_device_register(&smmu->iommu);
> +	if (err) {
> +		iommu_device_sysfs_remove(&smmu->iommu);
> +		return ERR_PTR(err);
> +	}
> +
>  	if (IS_ENABLED(CONFIG_DEBUG_FS))
>  		tegra_smmu_debugfs_init(smmu);
>  
> @@ -951,6 +973,9 @@ struct tegra_smmu *tegra_smmu_probe(struct device *dev,
>  
>  void tegra_smmu_remove(struct tegra_smmu *smmu)
>  {
> +	iommu_device_unregister(&smmu->iommu);
> +	iommu_device_sysfs_remove(&smmu->iommu);
> +
>  	if (IS_ENABLED(CONFIG_DEBUG_FS))
>  		tegra_smmu_debugfs_exit(smmu);
>  }
> 

With next-20170829 I am seeing several Tegra boards crashing [0][1] on
boot in tegra_smmu_probe() and the bisect is point to this commit. Looks
like there maybe a sequence problem between calls to bus_set_iommu() and
iommu_device_add_sysfs() which results in a NULL pointer dereference.

You can see the full crash log here [1].

Cheers
Jon

[0] https://nvtb.github.io//linux-next/
[1]
https://nvtb.github.io//linux-next/test_next-20170829/20170829024534/boot/tegra124-jetson-tk1/tegra124-jetson-tk1/tegra_defconfig_log.txt

Re: [PATCH 1/2] iommu/tegra: Add support for struct iommu_device

[Joerg Roedel]

Hi Jon,

On Wed, Aug 30, 2017 at 12:04:38PM +0100, Jon Hunter wrote:
> With next-20170829 I am seeing several Tegra boards crashing [0][1] on
> boot in tegra_smmu_probe() and the bisect is point to this commit. Looks
> like there maybe a sequence problem between calls to bus_set_iommu() and
> iommu_device_add_sysfs() which results in a NULL pointer dereference.

Thanks for the report. Can you please test whether the patch below
fixes the problem?

Thanks,

	Joerg

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index 2802e12e6a54..48ffbe95a663 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -949,10 +949,6 @@ struct tegra_smmu *tegra_smmu_probe(struct device *dev,
 
 	tegra_smmu_ahb_enable();
 
-	err = bus_set_iommu(&platform_bus_type, &tegra_smmu_ops);
-	if (err < 0)
-		return ERR_PTR(err);
-
 	err = iommu_device_sysfs_add(&smmu->iommu, dev, NULL, dev_name(dev));
 	if (err)
 		return ERR_PTR(err);
@@ -965,6 +961,10 @@ struct tegra_smmu *tegra_smmu_probe(struct device *dev,
 		return ERR_PTR(err);
 	}
 
+	err = bus_set_iommu(&platform_bus_type, &tegra_smmu_ops);
+	if (err < 0)
+		return ERR_PTR(err);
+
 	if (IS_ENABLED(CONFIG_DEBUG_FS))
 		tegra_smmu_debugfs_init(smmu);
 

Re: [PATCH 1/2] iommu/tegra: Add support for struct iommu_device

[Jon Hunter]

Hi Joerg,

On 30/08/17 13:09, Joerg Roedel wrote:
> Hi Jon,
> 
> On Wed, Aug 30, 2017 at 12:04:38PM +0100, Jon Hunter wrote:
>> With next-20170829 I am seeing several Tegra boards crashing [0][1] on
>> boot in tegra_smmu_probe() and the bisect is point to this commit. Looks
>> like there maybe a sequence problem between calls to bus_set_iommu() and
>> iommu_device_add_sysfs() which results in a NULL pointer dereference.
> 
> Thanks for the report. Can you please test whether the patch below
> fixes the problem?
> 
> Thanks,
> 
> 	Joerg
> 
> diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
> index 2802e12e6a54..48ffbe95a663 100644
> --- a/drivers/iommu/tegra-smmu.c
> +++ b/drivers/iommu/tegra-smmu.c
> @@ -949,10 +949,6 @@ struct tegra_smmu *tegra_smmu_probe(struct device *dev,
>  
>  	tegra_smmu_ahb_enable();
>  
> -	err = bus_set_iommu(&platform_bus_type, &tegra_smmu_ops);
> -	if (err < 0)
> -		return ERR_PTR(err);
> -
>  	err = iommu_device_sysfs_add(&smmu->iommu, dev, NULL, dev_name(dev));
>  	if (err)
>  		return ERR_PTR(err);
> @@ -965,6 +961,10 @@ struct tegra_smmu *tegra_smmu_probe(struct device *dev,
>  		return ERR_PTR(err);
>  	}
>  
> +	err = bus_set_iommu(&platform_bus_type, &tegra_smmu_ops);
> +	if (err < 0)
> +		return ERR_PTR(err);
> +

Yes I can confirm that this fixes the crash. I assume that you will fix
the error path for bus_set_iommu() above as I believe now it needs to
call iommu_device_sysfs_remove().

Cheers!
Jon

-- 
nvpublic

Re: [PATCH 1/2] iommu/tegra: Add support for struct iommu_device

[Joerg Roedel]

Hi Jon,

On Wed, Aug 30, 2017 at 03:22:05PM +0100, Jon Hunter wrote:
> Yes I can confirm that this fixes the crash. I assume that you will fix
> the error path for bus_set_iommu() above as I believe now it needs to
> call iommu_device_sysfs_remove().

Thanks for testing the patch. I updated the error-path and put the
commit below into the iommu-tree:

>From 96302d89a03524e04d46ec82c6730881bb755923 Mon Sep 17 00:00:00 2001
From: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>
Date: Wed, 30 Aug 2017 15:06:43 +0200
Subject: [PATCH] arm/tegra: Call bus_set_iommu() after iommu_device_register()

The bus_set_iommu() function will call the add_device()
call-back which needs the iommu to be registered.

Reported-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
Fixes: 0b480e447006 ('iommu/tegra: Add support for struct iommu_device')
Signed-off-by: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>
---
 drivers/iommu/tegra-smmu.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index 2802e12e6a54..3b6449e2cbf1 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -949,10 +949,6 @@ struct tegra_smmu *tegra_smmu_probe(struct device *dev,
 
 	tegra_smmu_ahb_enable();
 
-	err = bus_set_iommu(&platform_bus_type, &tegra_smmu_ops);
-	if (err < 0)
-		return ERR_PTR(err);
-
 	err = iommu_device_sysfs_add(&smmu->iommu, dev, NULL, dev_name(dev));
 	if (err)
 		return ERR_PTR(err);
@@ -965,6 +961,13 @@ struct tegra_smmu *tegra_smmu_probe(struct device *dev,
 		return ERR_PTR(err);
 	}
 
+	err = bus_set_iommu(&platform_bus_type, &tegra_smmu_ops);
+	if (err < 0) {
+		iommu_device_unregister(&smmu->iommu);
+		iommu_device_sysfs_remove(&smmu->iommu);
+		return ERR_PTR(err);
+	}
+
 	if (IS_ENABLED(CONFIG_DEBUG_FS))
 		tegra_smmu_debugfs_init(smmu);
 
-- 
2.13.5

Re: [PATCH 1/2] iommu/tegra: Add support for struct iommu_device

[Jon Hunter]

On 30/08/17 16:30, Joerg Roedel wrote:
> Hi Jon,
> 
> On Wed, Aug 30, 2017 at 03:22:05PM +0100, Jon Hunter wrote:
>> Yes I can confirm that this fixes the crash. I assume that you will fix
>> the error path for bus_set_iommu() above as I believe now it needs to
>> call iommu_device_sysfs_remove().
> 
> Thanks for testing the patch. I updated the error-path and put the
> commit below into the iommu-tree:
Great! Thanks, Jon

-- 
nvpublic

Re: [PATCH 1/2] iommu/tegra: Add support for struct iommu_device

[Jon Hunter]

Hi Joerg,

On 30/08/17 16:30, Joerg Roedel wrote:
> Hi Jon,
> 
> On Wed, Aug 30, 2017 at 03:22:05PM +0100, Jon Hunter wrote:
>> Yes I can confirm that this fixes the crash. I assume that you will fix
>> the error path for bus_set_iommu() above as I believe now it needs to
>> call iommu_device_sysfs_remove().
> 
> Thanks for testing the patch. I updated the error-path and put the
> commit below into the iommu-tree:
Today's -next is still failing and I don't see this fix in your public
tree yet [0]. Can you push this out?

Cheers
Jon

[0] https://git.kernel.org/pub/scm/linux/kernel/git/joro/iommu.git/

-- 
nvpublic

[PATCH 2/2] iommu/tegra-gart: Add support for struct iommu_device

[Joerg Roedel]

From: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>

Add a struct iommu_device to each tegra-gart and register it
with the iommu-core. Also link devices added to the driver
to their respective hardware iommus.

Signed-off-by: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>
---
 drivers/iommu/tegra-gart.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/drivers/iommu/tegra-gart.c b/drivers/iommu/tegra-gart.c
index 29bafc6..b62f790 100644
--- a/drivers/iommu/tegra-gart.c
+++ b/drivers/iommu/tegra-gart.c
@@ -61,6 +61,8 @@ struct gart_device {
 	struct list_head	client;
 	spinlock_t		client_lock;	/* for client list */
 	struct device		*dev;
+
+	struct iommu_device	iommu;		/* IOMMU Core handle */
 };
 
 struct gart_domain {
@@ -342,12 +344,16 @@ static int gart_iommu_add_device(struct device *dev)
 		return PTR_ERR(group);
 
 	iommu_group_put(group);
+
+	iommu_device_link(&gart_handle->iommu, dev);
+
 	return 0;
 }
 
 static void gart_iommu_remove_device(struct device *dev)
 {
 	iommu_group_remove_device(dev);
+	iommu_device_unlink(&gart_handle->iommu, dev);
 }
 
 static const struct iommu_ops gart_iommu_ops = {
@@ -397,6 +403,7 @@ static int tegra_gart_probe(struct platform_device *pdev)
 	struct resource *res, *res_remap;
 	void __iomem *gart_regs;
 	struct device *dev = &pdev->dev;
+	int ret;
 
 	if (gart_handle)
 		return -EIO;
@@ -423,6 +430,22 @@ static int tegra_gart_probe(struct platform_device *pdev)
 		return -ENXIO;
 	}
 
+	ret = iommu_device_sysfs_add(&gart->iommu, &pdev->dev, NULL,
+				     dev_name(&pdev->dev));
+	if (ret) {
+		dev_err(dev, "Failed to register IOMMU in sysfs\n");
+		return ret;
+	}
+
+	iommu_device_set_ops(&gart->iommu, &gart_iommu_ops);
+
+	ret = iommu_device_register(&gart->iommu);
+	if (ret) {
+		dev_err(dev, "Failed to register IOMMU\n");
+		iommu_device_sysfs_remove(&gart->iommu);
+		return ret;
+	}
+
 	gart->dev = &pdev->dev;
 	spin_lock_init(&gart->pte_lock);
 	spin_lock_init(&gart->client_lock);
@@ -449,6 +472,9 @@ static int tegra_gart_remove(struct platform_device *pdev)
 {
 	struct gart_device *gart = platform_get_drvdata(pdev);
 
+	iommu_device_unregister(&gart->iommu);
+	iommu_device_sysfs_remove(&gart->iommu);
+
 	writel(0, gart->regs + GART_CONFIG);
 	if (gart->savedata)
 		vfree(gart->savedata);
-- 
2.7.4

Re: [PATCH 2/2] iommu/tegra-gart: Add support for struct iommu_device

[Dmitry Osipenko]

Hello Joerg,

On 10.08.2017 01:29, Joerg Roedel wrote:
> From: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>
> 
> Add a struct iommu_device to each tegra-gart and register it
> with the iommu-core. Also link devices added to the driver
> to their respective hardware iommus.
> 
> Signed-off-by: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>
> ---
>  drivers/iommu/tegra-gart.c | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 

Reviewed-by: Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Tested-by: Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

> diff --git a/drivers/iommu/tegra-gart.c b/drivers/iommu/tegra-gart.c
> index 29bafc6..b62f790 100644
> --- a/drivers/iommu/tegra-gart.c
> +++ b/drivers/iommu/tegra-gart.c

[snip]

> @@ -449,6 +472,9 @@ static int tegra_gart_remove(struct platform_device *pdev)
>  {

BTW, GART's driver can't be build as a module, so this function is pretty much a
dead code. Probably worth considering its removal.

>  	struct gart_device *gart = platform_get_drvdata(pdev);
>  
> +	iommu_device_unregister(&gart->iommu);
> +	iommu_device_sysfs_remove(&gart->iommu);
> +
>  	writel(0, gart->regs + GART_CONFIG);
>  	if (gart->savedata)
>  		vfree(gart->savedata);
> 


-- 
Dmitry

Re: [PATCH 2/2] iommu/tegra-gart: Add support for struct iommu_device

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 1857 bytes --]

On Thu, Aug 17, 2017 at 01:21:52AM +0300, Dmitry Osipenko wrote:
> Hello Joerg,
> 
> On 10.08.2017 01:29, Joerg Roedel wrote:
> > From: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>
> > 
> > Add a struct iommu_device to each tegra-gart and register it
> > with the iommu-core. Also link devices added to the driver
> > to their respective hardware iommus.
> > 
> > Signed-off-by: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>
> > ---
> >  drivers/iommu/tegra-gart.c | 26 ++++++++++++++++++++++++++
> >  1 file changed, 26 insertions(+)
> > 
> 
> Reviewed-by: Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Tested-by: Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> 
> > diff --git a/drivers/iommu/tegra-gart.c b/drivers/iommu/tegra-gart.c
> > index 29bafc6..b62f790 100644
> > --- a/drivers/iommu/tegra-gart.c
> > +++ b/drivers/iommu/tegra-gart.c
> 
> [snip]
> 
> > @@ -449,6 +472,9 @@ static int tegra_gart_remove(struct platform_device *pdev)
> >  {
> 
> BTW, GART's driver can't be build as a module, so this function is pretty much a
> dead code. Probably worth considering its removal.

Technically you can unbind the driver via sysfs, in which case this
function would still be called. That said, all sorts of things will
probably start to crash when you do that. I'd like to think that we
will eventually be able to deal with this sanely (there's some work
in progress to establish stronger links between devices, so that we
can sanely deal with this kind of dependency), so I think it's okay
to keep this around in case we ever get there.

I don't have any objections to making the driver unloadable if that
is what everyone else prefers, though. In that case, however, there
are more steps involved than just removing the ->remove() callback.

Thierry

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 2/2] iommu/tegra-gart: Add support for struct iommu_device

[Dmitry Osipenko]

On 17.08.2017 16:52, Thierry Reding wrote:
> On Thu, Aug 17, 2017 at 01:21:52AM +0300, Dmitry Osipenko wrote:
>> Hello Joerg,
>>
>> On 10.08.2017 01:29, Joerg Roedel wrote:
>>> From: Joerg Roedel <jroedel@suse.de>
>>>
>>> Add a struct iommu_device to each tegra-gart and register it
>>> with the iommu-core. Also link devices added to the driver
>>> to their respective hardware iommus.
>>>
>>> Signed-off-by: Joerg Roedel <jroedel@suse.de>
>>> ---
>>>  drivers/iommu/tegra-gart.c | 26 ++++++++++++++++++++++++++
>>>  1 file changed, 26 insertions(+)
>>>
>>
>> Reviewed-by: Dmitry Osipenko <digetx@gmail.com>
>> Tested-by: Dmitry Osipenko <digetx@gmail.com>
>>
>>> diff --git a/drivers/iommu/tegra-gart.c b/drivers/iommu/tegra-gart.c
>>> index 29bafc6..b62f790 100644
>>> --- a/drivers/iommu/tegra-gart.c
>>> +++ b/drivers/iommu/tegra-gart.c
>>
>> [snip]
>>
>>> @@ -449,6 +472,9 @@ static int tegra_gart_remove(struct platform_device *pdev)
>>>  {
>>
>> BTW, GART's driver can't be build as a module, so this function is pretty much a
>> dead code. Probably worth considering its removal.
> 
> Technically you can unbind the driver via sysfs, in which case this
> function would still be called. That said, all sorts of things will
> probably start to crash when you do that. I'd like to think that we
> will eventually be able to deal with this sanely (there's some work
> in progress to establish stronger links between devices, so that we
> can sanely deal with this kind of dependency), so I think it's okay
> to keep this around in case we ever get there.
> 

Good point! I tried to unbind and with this patch kernel crashes immediately:

[  193.968506] kernel BUG at mm/slab.c:2816!
[  193.968912] Internal error: Oops - BUG: 0 [#1] PREEMPT SMP ARM
[  193.969466] Modules linked in:
[  193.969822] CPU: 1 PID: 1177 Comm: bash Not tainted
4.13.0-rc5-next-20170816-00067-gd320d19b76f8 #593
[  193.970653] Hardware name: NVIDIA Tegra SoC (Flattened Device Tree)
[  193.971313] task: ee31e380 task.stack: ee394000
[  193.971771] PC is at ___cache_free+0x374/0x47c
[  193.972261] LR is at 0x1
[  193.972539] pc : [<c02776ec>]    lr : [<00000001>]    psr: 200b0093
[  193.973112] sp : ee395dd0  ip : 00000009  fp : 00000000
[  193.973603] r10: 00000480  r9 : 2e844000  r8 : c1814d8c
[  193.974097] r7 : 005e4c40  r6 : c0f91fc0  r5 : ef262480  r4 : ef0003c0
[  193.974694] r3 : ef262400  r2 : ef262000  r1 : 00000400  r0 : 00000004

[snip]

[  193.991288] [<c02776ec>] (___cache_free) from [<c0278230>] (kfree+0xbc/0x260)
[  193.991978] [<c0278230>] (kfree) from [<c058897c>] (device_release+0x2c/0x90)
[  193.992732] [<c058897c>] (device_release) from [<c0ab6f14>]
(kobject_put+0x8c/0xe8)
[  193.993474] [<c0ab6f14>] (kobject_put) from [<c0527868>]
(tegra_gart_remove+0x1c/0x58)
[  193.994320] [<c0527868>] (tegra_gart_remove) from [<c058f770>]
(platform_drv_remove+0x24/0x3c)
[  193.995121] [<c058f770>] (platform_drv_remove) from [<c058db30>]
(device_release_driver_internal+0x15c/0x204)
[  193.996033] [<c058db30>] (device_release_driver_internal) from [<c058c2fc>]
(unbind_store+0x7c/0xfc)
[  193.996890] [<c058c2fc>] (unbind_store) from [<c02f8078>]
(kernfs_fop_write+0x104/0x208)
[  193.997661] [<c02f8078>] (kernfs_fop_write) from [<c027f6ec>]
(__vfs_write+0x1c/0x128)
[  193.998445] [<c027f6ec>] (__vfs_write) from [<c02810b8>] (vfs_write+0xa4/0x168)
[  194.017668] [<c02810b8>] (vfs_write) from [<c0281f0c>] (SyS_write+0x3c/0x90)
[  194.038493] [<c0281f0c>] (SyS_write) from [<c0107ce0>]
(ret_fast_syscall+0x0/0x1c)
[  194.057182] Code: e3a03000 ebfff54e eaffffe2 e7f001f2 (e7f001f2)

Without this patch, it crashes too after unbinding but not immediately. Either
way unbinding isn't useful for this device.

> I don't have any objections to making the driver unloadable if that
> is what everyone else prefers, though. In that case, however, there
> are more steps involved than just removing the ->remove() callback.
> 
Indeed!

-- 
Dmitry

Re: [PATCH 2/2] iommu/tegra-gart: Add support for struct iommu_device

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 482 bytes --]

On Thu, Aug 10, 2017 at 12:29:12AM +0200, Joerg Roedel wrote:
> From: Joerg Roedel <jroedel@suse.de>
> 
> Add a struct iommu_device to each tegra-gart and register it
> with the iommu-core. Also link devices added to the driver
> to their respective hardware iommus.
> 
> Signed-off-by: Joerg Roedel <jroedel@suse.de>
> ---
>  drivers/iommu/tegra-gart.c | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)

Acked-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 0/2] iommu/tegra*: Add support for struct iommu_device

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 867 bytes --]

On Thu, Aug 10, 2017 at 12:29:10AM +0200, Joerg Roedel wrote:
> Hi,
> 
> here are two patches to add support for 'struct iommu_device'
> to the tegra iommu-drivers. This will make the iommu-core
> code aware of the hardware iommus that a driver manages.
> 
> It will also add the iommus to sysfs and link them to the
> devices managed by them.
> 
> The patches apply on-top of Robin's iommu-group patches.
> 
> Please review.
> 
> Regards,
> 
> 	Joerg
> 
> Joerg Roedel (2):
>   iommu/tegra: Add support for struct iommu_device
>   iommu/tegra-gart: Add support for struct iommu_device
> 
>  drivers/iommu/tegra-gart.c | 26 ++++++++++++++++++++++++++
>  drivers/iommu/tegra-smmu.c | 25 +++++++++++++++++++++++++
>  2 files changed, 51 insertions(+)

The series:

Acked-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/2] iommu/tegra: Add support for struct iommu_device

[Jörg Rödel]

[-- Attachment #1.1: Type: text/plain, Size: 1283 bytes --]


    
Yes, I was running some tests against the new tree which didn't finish yesterday. Today I am traveling, but will be back im the evening and then I push the tree out.
Regards, Joerg


Sent from a Galaxy Class Phone

-------- Ursprüngliche Nachricht --------
Von: Jon Hunter <jonathanh@nvidia.com> 
Datum: 31.08.17  12:23  (GMT+01:00) 
An: Joerg Roedel <joro@8bytes.org> 
Cc: Thierry Reding <thierry.reding@gmail.com>, Robin Murphy <robin.murphy@arm.com>, iommu@lists.linux-foundation.org, linux-tegra@vger.kernel.org, linux-kernel@vger.kernel.org, Joerg Roedel <jroedel@suse.de> 
Betreff: Re: [PATCH 1/2] iommu/tegra: Add support for struct iommu_device 

Hi Joerg,

On 30/08/17 16:30, Joerg Roedel wrote:
> Hi Jon,
> 
> On Wed, Aug 30, 2017 at 03:22:05PM +0100, Jon Hunter wrote:
>> Yes I can confirm that this fixes the crash. I assume that you will fix
>> the error path for bus_set_iommu() above as I believe now it needs to
>> call iommu_device_sysfs_remove().
> 
> Thanks for testing the patch. I updated the error-path and put the
> commit below into the iommu-tree:
Today's -next is still failing and I don't see this fix in your public
tree yet [0]. Can you push this out?

Cheers
Jon

[0] https://git.kernel.org/pub/scm/linux/kernel/git/joro/iommu.git/

-- 
nvpublic

[-- Attachment #1.2: Type: text/html, Size: 1705 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]