On 2023/11/19 17:10, Hector Martin wrote: > On 2023/11/15 23:05, Jason Gunthorpe wrote: >> Allow fwspec to exist independently from the dev->iommu by providing >> functions to allow allocating and freeing the raw struct iommu_fwspec. >> >> Reflow the existing paths to call the new alloc/dealloc functions. >> >> Reviewed-by: Jerry Snitselaar <jsnitsel@xxxxxxxxxx> >> Signed-off-by: Jason Gunthorpe <jgg@xxxxxxxxxx> >> --- >> drivers/iommu/iommu.c | 82 ++++++++++++++++++++++++++++++++----------- >> include/linux/iommu.h | 11 +++++- >> 2 files changed, 72 insertions(+), 21 deletions(-) >> >> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c >> index 18a82a20934d53..86bbb9e75c7e03 100644 >> --- a/drivers/iommu/iommu.c >> +++ b/drivers/iommu/iommu.c >> @@ -361,10 +361,8 @@ static void dev_iommu_free(struct device *dev) >> struct dev_iommu *param = dev->iommu; >> >> dev->iommu = NULL; >> - if (param->fwspec) { >> - fwnode_handle_put(param->fwspec->iommu_fwnode); >> - kfree(param->fwspec); >> - } >> + if (param->fwspec) >> + iommu_fwspec_dealloc(param->fwspec); >> kfree(param); >> } >> >> @@ -2920,10 +2918,61 @@ const struct iommu_ops *iommu_ops_from_fwnode(struct fwnode_handle *fwnode) >> return ops; >> } >> >> +static int iommu_fwspec_assign_iommu(struct iommu_fwspec *fwspec, >> + struct device *dev, >> + struct fwnode_handle *iommu_fwnode) >> +{ >> + const struct iommu_ops *ops; >> + >> + if (fwspec->iommu_fwnode) { >> + /* >> + * fwspec->iommu_fwnode is the first iommu's fwnode. In the rare >> + * case of multiple iommus for one device they must point to the >> + * same driver, checked via same ops. >> + */ >> + ops = iommu_ops_from_fwnode(iommu_fwnode); > > This carries over a related bug from the original code: If a device has > two IOMMUs and the first one probes but the second one defers, ops will > be NULL here and the check will fail with EINVAL. > > Adding a check for that case here fixes it: > > if (!ops) > return driver_deferred_probe_check_state(dev); > > With that, for the whole series: > > Tested-by: Hector Martin <marcan@xxxxxxxxx> > > I can't specifically test for the probe races the series intends to fix > though, since that bug we only hit extremely rarely. I'm just testing > that nothing breaks. Actually no, this fix is not sufficient. If the first IOMMU is ready then the xlate path allocates dev->iommu, which then __iommu_probe_device takes as a sign that all IOMMUs are ready and does the device init. Then when the xlate comes along again after suceeding with the second IOMMU, __iommu_probe_device sees the device is already in a group and never initializes the second IOMMU, leaving the device with only one IOMMU. This patch fixes it, but honestly, at this point I have no idea how to "properly" fix this. There is *way* too much subtlety in this whole codepath. diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index 2477dec29740..2e4baf0572e7 100644 --- a/drivers/iommu/iommu.c +++ b/drivers/iommu/iommu.c @@ -2935,6 +2935,12 @@ int iommu_fwspec_of_xlate(struct iommu_fwspec *fwspec, struct device *dev, int ret; ret = iommu_fwspec_assign_iommu(fwspec, dev, iommu_fwnode); + if (ret == -EPROBE_DEFER) { + mutex_lock(&iommu_probe_device_lock); + if (dev->iommu) + dev_iommu_free(dev); + mutex_unlock(&iommu_probe_device_lock); + } if (ret) return ret; > >> + if (fwspec->ops != ops) >> + return -EINVAL; >> + return 0; >> + } >> + >> + if (!fwspec->ops) { >> + ops = iommu_ops_from_fwnode(iommu_fwnode); >> + if (!ops) >> + return driver_deferred_probe_check_state(dev); >> + fwspec->ops = ops; >> + } >> + >> + of_node_get(to_of_node(iommu_fwnode)); >> + fwspec->iommu_fwnode = iommu_fwnode; >> + return 0; >> +} >> + >> +struct iommu_fwspec *iommu_fwspec_alloc(void) >> +{ >> + struct iommu_fwspec *fwspec; >> + >> + fwspec = kzalloc(sizeof(*fwspec), GFP_KERNEL); >> + if (!fwspec) >> + return ERR_PTR(-ENOMEM); >> + return fwspec; >> +} >> + >> +void iommu_fwspec_dealloc(struct iommu_fwspec *fwspec) >> +{ >> + if (!fwspec) >> + return; >> + >> + if (fwspec->iommu_fwnode) >> + fwnode_handle_put(fwspec->iommu_fwnode); >> + kfree(fwspec); >> +} >> + >> int iommu_fwspec_init(struct device *dev, struct fwnode_handle *iommu_fwnode, >> const struct iommu_ops *ops) >> { >> struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev); >> + int ret; >> >> if (fwspec) >> return ops == fwspec->ops ? 0 : -EINVAL; >> @@ -2931,29 +2980,22 @@ int iommu_fwspec_init(struct device *dev, struct fwnode_handle *iommu_fwnode, >> if (!dev_iommu_get(dev)) >> return -ENOMEM; >> >> - fwspec = kzalloc(sizeof(*fwspec), GFP_KERNEL); >> - if (!fwspec) >> - return -ENOMEM; >> + fwspec = iommu_fwspec_alloc(); >> + if (IS_ERR(fwspec)) >> + return PTR_ERR(fwspec); >> >> - of_node_get(to_of_node(iommu_fwnode)); >> - fwspec->iommu_fwnode = iommu_fwnode; >> fwspec->ops = ops; >> + ret = iommu_fwspec_assign_iommu(fwspec, dev, iommu_fwnode); >> + if (ret) { >> + iommu_fwspec_dealloc(fwspec); >> + return ret; >> + } >> + >> dev_iommu_fwspec_set(dev, fwspec); >> return 0; >> } >> EXPORT_SYMBOL_GPL(iommu_fwspec_init); >> >> -void iommu_fwspec_free(struct device *dev) >> -{ >> - struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev); >> - >> - if (fwspec) { >> - fwnode_handle_put(fwspec->iommu_fwnode); >> - kfree(fwspec); >> - dev_iommu_fwspec_set(dev, NULL); >> - } >> -} >> -EXPORT_SYMBOL_GPL(iommu_fwspec_free); >> >> int iommu_fwspec_add_ids(struct device *dev, u32 *ids, int num_ids) >> { >> diff --git a/include/linux/iommu.h b/include/linux/iommu.h >> index e98a4ca8f536b7..c7c68cb59aa4dc 100644 >> --- a/include/linux/iommu.h >> +++ b/include/linux/iommu.h >> @@ -813,9 +813,18 @@ struct iommu_sva { >> struct iommu_domain *domain; >> }; >> >> +struct iommu_fwspec *iommu_fwspec_alloc(void); >> +void iommu_fwspec_dealloc(struct iommu_fwspec *fwspec); >> + >> int iommu_fwspec_init(struct device *dev, struct fwnode_handle *iommu_fwnode, >> const struct iommu_ops *ops); >> -void iommu_fwspec_free(struct device *dev); >> +static inline void iommu_fwspec_free(struct device *dev) >> +{ >> + if (!dev->iommu) >> + return; >> + iommu_fwspec_dealloc(dev->iommu->fwspec); >> + dev->iommu->fwspec = NULL; >> +} >> int iommu_fwspec_add_ids(struct device *dev, u32 *ids, int num_ids); >> const struct iommu_ops *iommu_ops_from_fwnode(struct fwnode_handle *fwnode); >> > > - Hector > > - Hector