Messages in this thread | | | Date | Tue, 3 Jan 2023 10:45:24 +0800 | Subject | Re: [PATCH v3 18/20] iommu: Call set_platform_dma if default domain is unavailable | From | Baolu Lu <> |
| |
Hi Jason,
On 11/28/22 10:57 PM, Jason Gunthorpe wrote: > On Mon, Nov 28, 2022 at 02:46:46PM +0800, Lu Baolu wrote: >> If the IOMMU driver has no default domain support, call set_platform_dma >> explicitly to return the kernel DMA control back to the platform DMA ops. >> >> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com> >> --- >> drivers/iommu/iommu.c | 28 ++++++++-------------------- >> 1 file changed, 8 insertions(+), 20 deletions(-) >> >> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c >> index 7c99d8eb3182..e4966f088184 100644 >> --- a/drivers/iommu/iommu.c >> +++ b/drivers/iommu/iommu.c >> @@ -2040,16 +2040,6 @@ int iommu_deferred_attach(struct device *dev, struct iommu_domain *domain) >> return 0; >> } >> >> -static void __iommu_detach_device(struct iommu_domain *domain, >> - struct device *dev) >> -{ >> - if (iommu_is_attach_deferred(dev)) >> - return; > > This removal might want to be its own patch with an explanation. > > It looks like at the current moment __iommu_detach_device() is only > called via call chains that are after the device driver is attached - > eg via explicit attach APIs called by the device driver. > > So it should just unconditionally work. It is actually looks like a > bug that we were blocking detach on these paths since the attach was > unconditional and the caller is going to free the (probably) UNAMANGED > domain once this returns. > > The only place we should be testing for deferred attach is during the > initial point the dma device is linked to the group, and then again > during the dma api calls to check if the device > > This maybe the patch that is needed to explain this: > > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c > index d69ebba81bebd8..06f1fe6563bb30 100644 > --- a/drivers/iommu/iommu.c > +++ b/drivers/iommu/iommu.c > @@ -993,8 +993,8 @@ int iommu_group_add_device(struct iommu_group *group, struct device *dev) > > mutex_lock(&group->mutex); > list_add_tail(&device->list, &group->devices); > - if (group->domain && !iommu_is_attach_deferred(dev)) > - ret = __iommu_attach_device(group->domain, dev); > + if (group->domain) > + ret = iommu_group_do_dma_first_attach(dev, group->domain); > mutex_unlock(&group->mutex); > if (ret) > goto err_put_group; > @@ -1760,21 +1760,24 @@ static void probe_alloc_default_domain(struct bus_type *bus, > > } > > -static int iommu_group_do_dma_attach(struct device *dev, void *data) > +static int iommu_group_do_dma_first_attach(struct device *dev, void *data) > { > struct iommu_domain *domain = data; > - int ret = 0; > > - if (!iommu_is_attach_deferred(dev)) > - ret = __iommu_attach_device(domain, dev); > + lockdep_assert_held(&dev->iommu_group->mutex); > > - return ret; > + if (iommu_is_attach_deferred(dev)) { > + dev->iommu->attach_deferred = 1; > + return 0; > + } > + > + return __iommu_attach_device(domain, dev); > } > > -static int __iommu_group_dma_attach(struct iommu_group *group) > +static int __iommu_group_dma_first_attach(struct iommu_group *group) > { > return __iommu_group_for_each_dev(group, group->default_domain, > - iommu_group_do_dma_attach); > + iommu_group_do_dma_first_attach); > } > > static int iommu_group_do_probe_finalize(struct device *dev, void *data) > @@ -1839,7 +1842,7 @@ int bus_iommu_probe(struct bus_type *bus) > > iommu_group_create_direct_mappings(group); > > - ret = __iommu_group_dma_attach(group); > + ret = __iommu_group_dma_first_attach(group); > > mutex_unlock(&group->mutex); > > @@ -1971,9 +1974,11 @@ static int __iommu_attach_device(struct iommu_domain *domain, > return -ENODEV; > > ret = domain->ops->attach_dev(domain, dev); > - if (!ret) > - trace_attach_device_to_domain(dev); > - return ret; > + if (ret) > + return ret; > + dev->iommu->attach_deferred = 0; > + trace_attach_device_to_domain(dev); > + return 0; > } > > /** > @@ -2018,7 +2023,7 @@ EXPORT_SYMBOL_GPL(iommu_attach_device); > > int iommu_deferred_attach(struct device *dev, struct iommu_domain *domain) > { > - if (iommu_is_attach_deferred(dev)) > + if (dev->iommu && dev->iommu->attach_deferred) > return __iommu_attach_device(domain, dev); > > return 0; > @@ -2027,9 +2032,6 @@ int iommu_deferred_attach(struct device *dev, struct iommu_domain *domain) > static void __iommu_detach_device(struct iommu_domain *domain, > struct device *dev) > { > - if (iommu_is_attach_deferred(dev)) > - return; > - > domain->ops->detach_dev(domain, dev); > trace_detach_device_from_domain(dev); > } > diff --git a/include/linux/iommu.h b/include/linux/iommu.h > index 1690c334e51631..ebac04a13fff68 100644 > --- a/include/linux/iommu.h > +++ b/include/linux/iommu.h > @@ -413,6 +413,7 @@ struct dev_iommu { > struct iommu_device *iommu_dev; > void *priv; > u32 max_pasids; > + u8 attach_deferred; > }; > > int iommu_device_register(struct iommu_device *iommu,
Thanks for the patch! It seems that we also need to call iommu_group_do_dma_first_attach() in the iommu_probe_device() path?
@@ -401,7 +425,7 @@ int iommu_probe_device(struct device *dev) * attach the default domain. */ if (group->default_domain && !group->owner) { - ret = __iommu_attach_device(group->default_domain, dev); + ret = iommu_group_do_dma_first_attach(dev, group->default_domain); if (ret) { mutex_unlock(&group->mutex); iommu_group_put(group);
By the way, I'd like to put above code in a separated patch of the next version, can I add your signed-off-by?
-- Best regards, baolu
| |