Messages in this thread | | | From | Shameerali Kolothum Thodi <> | Subject | RE: [PATCH v2] iommu: Check dev->iommu in iommu_dev_xxx functions | Date | Fri, 12 Feb 2021 16:44:46 +0000 |
| |
> -----Original Message----- > From: Robin Murphy [mailto:robin.murphy@arm.com] > Sent: 12 February 2021 16:39 > To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>; > linux-kernel@vger.kernel.org; iommu@lists.linux-foundation.org > Cc: joro@8bytes.org; jean-philippe@linaro.org; will@kernel.org; Zengtao (B) > <prime.zeng@hisilicon.com>; linuxarm@openeuler.org > Subject: Re: [PATCH v2] iommu: Check dev->iommu in iommu_dev_xxx functions > > On 2021-02-12 14:54, Shameerali Kolothum Thodi wrote: > > Hi Robin/Joerg, > > > >> -----Original Message----- > >> From: Shameer Kolothum [mailto:shameerali.kolothum.thodi@huawei.com] > >> Sent: 01 February 2021 12:41 > >> To: linux-kernel@vger.kernel.org; iommu@lists.linux-foundation.org > >> Cc: joro@8bytes.org; robin.murphy@arm.com; jean-philippe@linaro.org; > >> will@kernel.org; Zengtao (B) <prime.zeng@hisilicon.com>; > >> linuxarm@openeuler.org > >> Subject: [Linuxarm] [PATCH v2] iommu: Check dev->iommu in > iommu_dev_xxx > >> functions > >> > >> The device iommu probe/attach might have failed leaving dev->iommu > >> to NULL and device drivers may still invoke these functions resulting > >> in a crash in iommu vendor driver code. Hence make sure we check that. > >> > >> Also added iommu_ops to the "struct dev_iommu" and set it if the dev > >> is successfully associated with an iommu. > >> > >> Fixes: a3a195929d40 ("iommu: Add APIs for multiple domains per device") > >> Signed-off-by: Shameer Kolothum > <shameerali.kolothum.thodi@huawei.com> > >> --- > >> v1 --> v2: > >> -Added iommu_ops to struct dev_iommu based on the discussion with > Robin. > >> -Rebased against iommu-tree core branch. > > > > A gentle ping on this... > > Is there a convincing justification for maintaining yet another copy of > the ops pointer rather than simply dereferencing iommu_dev->ops at point > of use? >
TBH, nothing I can think of now. That was mainly the way I interpreted your suggestion from the v1. Now it looks like you didn’t mean it :). I am Ok to rework it to dereference it from iommu_dev. Please let me know.
Thanks, Shameer
> Robin. > > > Thanks, > > Shameer > > > >> --- > >> drivers/iommu/iommu.c | 19 +++++++------------ > >> include/linux/iommu.h | 2 ++ > >> 2 files changed, 9 insertions(+), 12 deletions(-) > >> > >> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c > >> index fd76e2f579fe..6023d0b7c542 100644 > >> --- a/drivers/iommu/iommu.c > >> +++ b/drivers/iommu/iommu.c > >> @@ -217,6 +217,7 @@ static int __iommu_probe_device(struct device > *dev, > >> struct list_head *group_list > >> } > >> > >> dev->iommu->iommu_dev = iommu_dev; > >> + dev->iommu->ops = iommu_dev->ops; > >> > >> group = iommu_group_get_for_dev(dev); > >> if (IS_ERR(group)) { > >> @@ -2865,10 +2866,8 @@ > EXPORT_SYMBOL_GPL(iommu_fwspec_add_ids); > >> */ > >> int iommu_dev_enable_feature(struct device *dev, enum > >> iommu_dev_features feat) > >> { > >> - const struct iommu_ops *ops = dev->bus->iommu_ops; > >> - > >> - if (ops && ops->dev_enable_feat) > >> - return ops->dev_enable_feat(dev, feat); > >> + if (dev->iommu && dev->iommu->ops->dev_enable_feat) > >> + return dev->iommu->ops->dev_enable_feat(dev, feat); > >> > >> return -ENODEV; > >> } > >> @@ -2881,10 +2880,8 @@ > >> EXPORT_SYMBOL_GPL(iommu_dev_enable_feature); > >> */ > >> int iommu_dev_disable_feature(struct device *dev, enum > >> iommu_dev_features feat) > >> { > >> - const struct iommu_ops *ops = dev->bus->iommu_ops; > >> - > >> - if (ops && ops->dev_disable_feat) > >> - return ops->dev_disable_feat(dev, feat); > >> + if (dev->iommu && dev->iommu->ops->dev_disable_feat) > >> + return dev->iommu->ops->dev_disable_feat(dev, feat); > >> > >> return -EBUSY; > >> } > >> @@ -2892,10 +2889,8 @@ > >> EXPORT_SYMBOL_GPL(iommu_dev_disable_feature); > >> > >> bool iommu_dev_feature_enabled(struct device *dev, enum > >> iommu_dev_features feat) > >> { > >> - const struct iommu_ops *ops = dev->bus->iommu_ops; > >> - > >> - if (ops && ops->dev_feat_enabled) > >> - return ops->dev_feat_enabled(dev, feat); > >> + if (dev->iommu && dev->iommu->ops->dev_feat_enabled) > >> + return dev->iommu->ops->dev_feat_enabled(dev, feat); > >> > >> return false; > >> } > >> diff --git a/include/linux/iommu.h b/include/linux/iommu.h > >> index 524ffc2ff64f..ff0c76bdfb67 100644 > >> --- a/include/linux/iommu.h > >> +++ b/include/linux/iommu.h > >> @@ -354,6 +354,7 @@ struct iommu_fault_param { > >> * @fault_param: IOMMU detected device fault reporting data > >> * @fwspec: IOMMU fwspec data > >> * @iommu_dev: IOMMU device this device is linked to > >> + * @ops: iommu-ops for talking to the iommu_dev > >> * @priv: IOMMU Driver private data > >> * > >> * TODO: migrate other per device data pointers under > iommu_dev_data, > >> e.g. > >> @@ -364,6 +365,7 @@ struct dev_iommu { > >> struct iommu_fault_param *fault_param; > >> struct iommu_fwspec *fwspec; > >> struct iommu_device *iommu_dev; > >> + const struct iommu_ops *ops; > >> void *priv; > >> }; > >> > >> -- > >> 2.17.1 > >> _______________________________________________ > >> Linuxarm mailing list -- linuxarm@openeuler.org > >> To unsubscribe send an email to linuxarm-leave@openeuler.org
| |