| Message ID | 36e245489361de2d13db22a510fa5c79e7126278.1740667667.git.robin.murphy@arm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | iommu: Unexport iommu_fwspec_free() | expand |
On Thu, Feb 27, 2025 at 02:47:47PM +0000, Robin Murphy wrote: > The drivers doing their own fwspec parsing have no need to call > iommu_fwspec_free() since fwspecs were moved into dev_iommu, as > returning an error from .probe_device will tear down the whole lot > anyway. Move it into the private interface now that it only serves > for of_iommu to clean up in an error case. > > I have no idea what mtk_v1 was doing in effectively guaranteeing > a NULL fwspec would be dereferenced if no "iommus" DT property was > found, so add a check for that to at least make the code look sane. > Signed-off-by: Robin Murphy <robin.murphy@arm.com> > --- > drivers/iommu/arm/arm-smmu/arm-smmu.c | 1 - > drivers/iommu/iommu-priv.h | 2 ++ > drivers/iommu/iommu.c | 1 - > drivers/iommu/mtk_iommu_v1.c | 14 ++++---------- > drivers/iommu/tegra-smmu.c | 1 - > include/linux/iommu.h | 5 ----- > 6 files changed, 6 insertions(+), 18 deletions(-) Reviewed-by: Jason Gunthorpe <jgg@nvidia.com> Jason
On Thu, Feb 27, 2025 at 02:47:47PM +0000, Robin Murphy wrote: > The drivers doing their own fwspec parsing have no need to call > iommu_fwspec_free() since fwspecs were moved into dev_iommu, as > returning an error from .probe_device will tear down the whole lot > anyway. Move it into the private interface now that it only serves > for of_iommu to clean up in an error case. > > I have no idea what mtk_v1 was doing in effectively guaranteeing > a NULL fwspec would be dereferenced if no "iommus" DT property was > found, so add a check for that to at least make the code look sane. > > Signed-off-by: Robin Murphy <robin.murphy@arm.com> > --- > drivers/iommu/arm/arm-smmu/arm-smmu.c | 1 - > drivers/iommu/iommu-priv.h | 2 ++ > drivers/iommu/iommu.c | 1 - > drivers/iommu/mtk_iommu_v1.c | 14 ++++---------- > drivers/iommu/tegra-smmu.c | 1 - > include/linux/iommu.h | 5 ----- > 6 files changed, 6 insertions(+), 18 deletions(-) Applied, thanks.
This patch triggered two compile issues on ARM32, which I fixed up in my tree: On Thu, Feb 27, 2025 at 02:47:47PM +0000, Robin Murphy wrote: > static struct iommu_device *mtk_iommu_v1_probe_device(struct device *dev) > { > - struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev); > + struct iommu_fwspec *fwspec = NULL Missing semicolon here. > struct of_phandle_args iommu_spec; > struct mtk_iommu_v1_data *data; > int err, idx = 0, larbid, larbidx; > struct device_link *link; > struct device *larbdev; > > - /* > - * In the deferred case, free the existed fwspec. > - * Always initialize the fwspec internally. > - */ > - if (fwspec) { > - iommu_fwspec_free(dev); > - fwspec = dev_iommu_fwspec_get(dev); > - } > - > while (!of_parse_phandle_with_args(dev->of_node, "iommus", > "#iommu-cells", > idx, &iommu_spec)) { > @@ -476,6 +467,9 @@ static struct iommu_device *mtk_iommu_v1_probe_device(struct device *dev) > idx++; > } > > + if (!fwspec) > + return -ENODEV; > + Wrong return type here. Please be more cautious next time and make sure to compile-test all changed files of a patch. Thanks, Joerg
On 11/03/2025 1:15 pm, Joerg Roedel wrote: > This patch triggered two compile issues on ARM32, which I fixed up in my > tree: > > On Thu, Feb 27, 2025 at 02:47:47PM +0000, Robin Murphy wrote: >> static struct iommu_device *mtk_iommu_v1_probe_device(struct device *dev) >> { >> - struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev); >> + struct iommu_fwspec *fwspec = NULL > > Missing semicolon here. > >> struct of_phandle_args iommu_spec; >> struct mtk_iommu_v1_data *data; >> int err, idx = 0, larbid, larbidx; >> struct device_link *link; >> struct device *larbdev; >> >> - /* >> - * In the deferred case, free the existed fwspec. >> - * Always initialize the fwspec internally. >> - */ >> - if (fwspec) { >> - iommu_fwspec_free(dev); >> - fwspec = dev_iommu_fwspec_get(dev); >> - } >> - >> while (!of_parse_phandle_with_args(dev->of_node, "iommus", >> "#iommu-cells", >> idx, &iommu_spec)) { >> @@ -476,6 +467,9 @@ static struct iommu_device *mtk_iommu_v1_probe_device(struct device *dev) >> idx++; >> } >> >> + if (!fwspec) >> + return -ENODEV; >> + > > Wrong return type here. > > Please be more cautious next time and make sure to compile-test all > changed files of a patch. Oops, sorry, indeed that was a failure of my process - seems I need to be more diligent about COMPILE_TEST. Patch sent :) Thanks a lot for the fixups! Robin.
diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c index de205a34ffc6..ea9f8e484e35 100644 --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c @@ -1486,7 +1486,6 @@ static struct iommu_device *arm_smmu_probe_device(struct device *dev) out_cfg_free: kfree(cfg); out_free: - iommu_fwspec_free(dev); return ERR_PTR(ret); } diff --git a/drivers/iommu/iommu-priv.h b/drivers/iommu/iommu-priv.h index de5b54eaa8bf..17e245cf17bb 100644 --- a/drivers/iommu/iommu-priv.h +++ b/drivers/iommu/iommu-priv.h @@ -24,6 +24,8 @@ static inline const struct iommu_ops *iommu_fwspec_ops(struct iommu_fwspec *fwsp return iommu_ops_from_fwnode(fwspec ? fwspec->iommu_fwnode : NULL); } +void iommu_fwspec_free(struct device *dev); + int iommu_group_replace_domain(struct iommu_group *group, struct iommu_domain *new_domain); diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index 60aed01e54f2..0fce7a1677e6 100644 --- a/drivers/iommu/iommu.c +++ b/drivers/iommu/iommu.c @@ -2849,7 +2849,6 @@ void iommu_fwspec_free(struct device *dev) dev_iommu_fwspec_set(dev, NULL); } } -EXPORT_SYMBOL_GPL(iommu_fwspec_free); int iommu_fwspec_add_ids(struct device *dev, const u32 *ids, int num_ids) { diff --git a/drivers/iommu/mtk_iommu_v1.c b/drivers/iommu/mtk_iommu_v1.c index a565b9e40f4a..09676bd7cb9c 100644 --- a/drivers/iommu/mtk_iommu_v1.c +++ b/drivers/iommu/mtk_iommu_v1.c @@ -446,22 +446,13 @@ static int mtk_iommu_v1_create_mapping(struct device *dev, static struct iommu_device *mtk_iommu_v1_probe_device(struct device *dev) { - struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev); + struct iommu_fwspec *fwspec = NULL struct of_phandle_args iommu_spec; struct mtk_iommu_v1_data *data; int err, idx = 0, larbid, larbidx; struct device_link *link; struct device *larbdev; - /* - * In the deferred case, free the existed fwspec. - * Always initialize the fwspec internally. - */ - if (fwspec) { - iommu_fwspec_free(dev); - fwspec = dev_iommu_fwspec_get(dev); - } - while (!of_parse_phandle_with_args(dev->of_node, "iommus", "#iommu-cells", idx, &iommu_spec)) { @@ -476,6 +467,9 @@ static struct iommu_device *mtk_iommu_v1_probe_device(struct device *dev) idx++; } + if (!fwspec) + return -ENODEV; + data = dev_iommu_priv_get(dev); /* Link the consumer device with the smi-larb device(supplier) */ diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c index 7f633bb5efef..69d353e1df84 100644 --- a/drivers/iommu/tegra-smmu.c +++ b/drivers/iommu/tegra-smmu.c @@ -846,7 +846,6 @@ static int tegra_smmu_configure(struct tegra_smmu *smmu, struct device *dev, err = ops->of_xlate(dev, args); if (err < 0) { dev_err(dev, "failed to parse SW group ID: %d\n", err); - iommu_fwspec_free(dev); return err; } diff --git a/include/linux/iommu.h b/include/linux/iommu.h index 38c65e92ecd0..446ca5a5af35 100644 --- a/include/linux/iommu.h +++ b/include/linux/iommu.h @@ -1079,7 +1079,6 @@ struct iommu_mm_data { }; int iommu_fwspec_init(struct device *dev, struct fwnode_handle *iommu_fwnode); -void iommu_fwspec_free(struct device *dev); int iommu_fwspec_add_ids(struct device *dev, const u32 *ids, int num_ids); static inline struct iommu_fwspec *dev_iommu_fwspec_get(struct device *dev) @@ -1390,10 +1389,6 @@ static inline int iommu_fwspec_init(struct device *dev, return -ENODEV; } -static inline void iommu_fwspec_free(struct device *dev) -{ -} - static inline int iommu_fwspec_add_ids(struct device *dev, u32 *ids, int num_ids) {
The drivers doing their own fwspec parsing have no need to call iommu_fwspec_free() since fwspecs were moved into dev_iommu, as returning an error from .probe_device will tear down the whole lot anyway. Move it into the private interface now that it only serves for of_iommu to clean up in an error case. I have no idea what mtk_v1 was doing in effectively guaranteeing a NULL fwspec would be dereferenced if no "iommus" DT property was found, so add a check for that to at least make the code look sane. Signed-off-by: Robin Murphy <robin.murphy@arm.com> --- drivers/iommu/arm/arm-smmu/arm-smmu.c | 1 - drivers/iommu/iommu-priv.h | 2 ++ drivers/iommu/iommu.c | 1 - drivers/iommu/mtk_iommu_v1.c | 14 ++++---------- drivers/iommu/tegra-smmu.c | 1 - include/linux/iommu.h | 5 ----- 6 files changed, 6 insertions(+), 18 deletions(-)