2017-08-09 22:29:19

by Joerg Roedel

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

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


2017-08-09 22:29:21

by Joerg Roedel

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

From: Joerg Roedel <[email protected]>

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 <[email protected]>
---
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

2017-08-09 22:29:49

by Joerg Roedel

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

From: Joerg Roedel <[email protected]>

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 <[email protected]>
---
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

2017-08-16 22:22:11

by Dmitry Osipenko

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

Hello Joerg,

On 10.08.2017 01:29, Joerg Roedel wrote:
> From: Joerg Roedel <[email protected]>
>
> 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 <[email protected]>
> ---
> drivers/iommu/tegra-gart.c | 26 ++++++++++++++++++++++++++
> 1 file changed, 26 insertions(+)
>

Reviewed-by: Dmitry Osipenko <[email protected]>
Tested-by: Dmitry Osipenko <[email protected]>

> 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

2017-08-17 13:52:35

by Thierry Reding

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

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 <[email protected]>
> >
> > 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 <[email protected]>
> > ---
> > drivers/iommu/tegra-gart.c | 26 ++++++++++++++++++++++++++
> > 1 file changed, 26 insertions(+)
> >
>
> Reviewed-by: Dmitry Osipenko <[email protected]>
> Tested-by: Dmitry Osipenko <[email protected]>
>
> > 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


Attachments:
(No filename) (1.67 kB)
signature.asc (833.00 B)
Download all attachments

2017-08-17 13:58:09

by Thierry Reding

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

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 <[email protected]>


Attachments:
(No filename) (809.00 B)
signature.asc (833.00 B)
Download all attachments

2017-08-17 17:18:01

by Dmitry Osipenko

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

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 <[email protected]>
>>>
>>> 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 <[email protected]>
>>> ---
>>> drivers/iommu/tegra-gart.c | 26 ++++++++++++++++++++++++++
>>> 1 file changed, 26 insertions(+)
>>>
>>
>> Reviewed-by: Dmitry Osipenko <[email protected]>
>> Tested-by: Dmitry Osipenko <[email protected]>
>>
>>> 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

2017-08-18 12:47:12

by Thierry Reding

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

On Thu, Aug 10, 2017 at 12:29:11AM +0200, Joerg Roedel wrote:
> From: Joerg Roedel <[email protected]>
>
> 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 <[email protected]>
> ---
> drivers/iommu/tegra-smmu.c | 25 +++++++++++++++++++++++++
> 1 file changed, 25 insertions(+)

Acked-by: Thierry Reding <[email protected]>


Attachments:
(No filename) (468.00 B)
signature.asc (833.00 B)
Download all attachments

2017-08-18 12:47:23

by Thierry Reding

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

On Thu, Aug 10, 2017 at 12:29:12AM +0200, Joerg Roedel wrote:
> From: Joerg Roedel <[email protected]>
>
> 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 <[email protected]>
> ---
> drivers/iommu/tegra-gart.c | 26 ++++++++++++++++++++++++++
> 1 file changed, 26 insertions(+)

Acked-by: Thierry Reding <[email protected]>


Attachments:
(No filename) (469.00 B)
signature.asc (833.00 B)
Download all attachments

2017-08-30 11:06:58

by Jon Hunter

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


On 09/08/17 23:29, Joerg Roedel wrote:
> From: Joerg Roedel <[email protected]>
>
> 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 <[email protected]>
> ---
> 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

2017-08-30 12:09:09

by Joerg Roedel

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

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);


2017-08-30 14:24:19

by Jon Hunter

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

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

2017-08-30 15:30:38

by Joerg Roedel

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

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 <[email protected]>
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 <[email protected]>
Fixes: 0b480e447006 ('iommu/tegra: Add support for struct iommu_device')
Signed-off-by: Joerg Roedel <[email protected]>
---
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

2017-08-30 16:57:18

by Jon Hunter

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


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

2017-08-31 10:24:28

by Jon Hunter

[permalink] [raw]
Subject: 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