On 2023/11/19 23:13, Jason Gunthorpe wrote:
> On Sun, Nov 19, 2023 at 06:19:43PM +0900, Hector Martin wrote:
>>>> +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);
>
> Yes!
>
>>> 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.
>
> It doesn't.. The code there is:
>
> if (!fwspec && dev->iommu)
> fwspec = dev->iommu->fwspec;
> if (fwspec)
> ops = fwspec->ops;
> else
> ops = dev->bus->iommu_ops;
> if (!ops) {
> ret = -ENODEV;
> goto out_unlock;
> }
>
> Which is sensitive only to !NULL fwspec, and if EPROBE_DEFER is
> returned fwspec will be freed and dev->iommu->fwspec will be NULL
> here.
>
> In the NULL case it does a 'bus probe' with a NULL fwspec and all the
> fwspec drivers return immediately from their probe functions.
>
> Did I miss something?
apple_dart is not a fwspec driver and doesn't do that :-)
>
>> 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 should be fixed by the first hunk to check every iommu and fail?
>
> BTW, do you have a systems with same device attached to multiple
> iommus?
Yes, Apple ARM64 machines all have multiple ganged IOMMUs for certain
devices (USB and ISP). We also attach all display IOMMUs to the global
virtual display-subsystem device to handle framebuffer mappings, instead
of trying to dynamically map them to a bunch of individual display
controllers (which is a lot more painful). That last one is what
reliably reproduces this problem, display breaks without both previous
patches ever since we started supporting more than one display output.
The first one is not enough.
> I've noticed another bug here, many drivers don't actually support
> differing iommu instances and nothing seems to check it..
apple-dart does (as long as all the IOMMUs are using that driver).
>
> Thanks,
> Jason
>
>
- Hector
