Messages in this thread | | | Date | Fri, 27 Jan 2023 08:32:45 -0800 | From | Jacob Pan <> | Subject | Re: [PATCH v2 5/8] iommu: Switch __iommu_domain_alloc() to device ops |
| |
Hi Robin,
On Fri, 27 Jan 2023 11:42:27 +0000, Robin Murphy <robin.murphy@arm.com> wrote:
> On 2023-01-26 23:22, Jacob Pan wrote: > > Hi Robin, > > > > On Thu, 26 Jan 2023 18:26:20 +0000, Robin Murphy <robin.murphy@arm.com> > > wrote: > > > >> > >> +static int __iommu_domain_alloc_dev(struct device *dev, void *data) > >> +{ > >> + struct device **alloc_dev = data; > >> + > >> + if (!dev_iommu_ops_valid(dev)) > >> + return 0; > >> + > >> + WARN_ONCE(*alloc_dev && dev_iommu_ops(dev) != > >> dev_iommu_ops(*alloc_dev), > >> + "Multiple IOMMU drivers present, which the public > >> IOMMU API can't fully support yet. You may still need to disable one > >> or more to get the expected result here, sorry!\n"); + > >> + *alloc_dev = dev; > >> + return 0; > >> +} > >> + > >> struct iommu_domain *iommu_domain_alloc(struct bus_type *bus) > >> { > >> - return __iommu_domain_alloc(bus, IOMMU_DOMAIN_UNMANAGED); > >> + struct device *dev = NULL; > >> + > >> + /* We always check the whole bus, so the return value isn't > >> useful */ > >> + bus_for_each_dev(bus, NULL, &dev, __iommu_domain_alloc_dev); > >> + if (!dev) > >> + return NULL; > > Since __iommu_domain_alloc_dev() will always return 0, > > bus_for_each_dev() will never breakout until the whole dev list is > > iterated over. If so, would dev only record the last one? i.e. prior > > results get overwritten. Maybe a misunderstood the logic. > > Yes, as the comment points out, the intent is to walk the whole bus to > check it for consistency. Beyond that, we just need *a* device with > IOMMU ops; it doesn't matter at all which one it is. It happens to be > the last one off the list because that's what fell out of writing the > fewest lines of code. > > (You could argue that there's no need to repeat the full walk if the > WARN_ONCE has already fired, but I'd rather keep the behaviour simple > and consistent - this is only meant to be a short-term solution, and > it's not a performance-critical path) That make sense now, thank you for the explanation.
Jacob
| |