| Message ID | 1502317752-8792-2-git-send-email-joro@8bytes.org |
|---|---|
| State | Deferred |
| Headers | show |
On Thu, Aug 10, 2017 at 12:29:11AM +0200, 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(+) Acked-by: Thierry Reding <treding@nvidia.com>
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 -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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); -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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@suse.de> 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@nvidia.com> Fixes: 0b480e447006 ('iommu/tegra: Add support for struct iommu_device') Signed-off-by: Joerg Roedel <jroedel@suse.de> --- 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);
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
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/
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); }