Re: [RFC PATCH] iommu: Handle domain allocation failure gracefully

[Baolu Lu <baolu.lu@linux.intel.com>]

On 2022/9/15 14:13, Vasant Hegde wrote:
> On 9/15/2022 8:44 AM, Baolu Lu wrote:
>> On 9/14/22 10:44 PM, Vasant Hegde wrote:
>>> Current code does not validate domain allocation failures. This will
>>> result in random failure at later stage. Hence handle domain allocation
>>> failures.
>>>
>>> Signed-off-by: Vasant Hegde<vasant.hegde@amd.com>
>>> ---
>>> Hi Joerg,
>>>     With current code whenever amd driver failes to allocate domain we hit
>>>     failure at late stage and it became difficult to debug. Hence I
>>>     thought of adding this fix.
>>>
>>>     I see that in commit 6e1aa2049154 you have intentionally didn't check
>>>     the return value. Looking into drivers/iommu/* it looks like all
>>>     drivers has domain_alloc() callback handler. But some drivers (like msm
>>>     - msm_iommu_domain_alloc()) allocates default domain for certain type
>>>     only. Hence I'm not sure whether its ok to handle domain allocation
>>>     failures or not. Hence I have marked this patch as RFC.
>>>
>>>     Let me know whether this patch is fine -OR- is there any other way we can
>>>     handle domain allocation failures?
>>>
>>> -Vasant
>>>
>>>    drivers/iommu/iommu.c | 24 +++++++++++++-----------
>>>    1 file changed, 13 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>>> index 780fb7071577..e8a1e7fbffb9 100644
>>> --- a/drivers/iommu/iommu.c
>>> +++ b/drivers/iommu/iommu.c
>>> @@ -289,18 +289,21 @@ int iommu_probe_device(struct device *dev)
>>>          /*
>>>         * Try to allocate a default domain - needs support from the
>>> -     * IOMMU driver. There are still some drivers which don't
>>> -     * support default domains, so the return value is not yet
>>> -     * checked.
>>> +     * IOMMU driver.
>>>         */
>>>        mutex_lock(&group->mutex);
>>> -    iommu_alloc_default_domain(group, dev);
>>> +    ret = iommu_alloc_default_domain(group, dev);
>>> +    if (ret) {
>>> +        mutex_unlock(&group->mutex);
>>> +        iommu_group_put(group);
>>> +        goto err_release;
>>> +    }
>> Some IOMMU drivers (s390, gart, tegra...) don't support default domains.
>> iommu_alloc_default_domain() returns a ENOMEM and the probe process
>> continues.
> Thanks for the clarification.
> 
>> Above changes this behavior. The probe process will always fails with
>> those IOMMU drivers.
> Right. I am wondering whether we have any other way to check the return value?

The __iommu_probe_device() treats ENODEV as a special return value which
actually means "the device has no IOMMU hardware, go ahead with other
devices." You can check with Joerg whether you can play the same game
here.

Best regards,
baolu