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

[Vasant Hegde <vasant.hegde@amd.com>]

Joerg,


On 9/15/2022 12:58 PM, Baolu Lu wrote:
> 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.

Any suggestion? Is there someway to handle error code?


-Vasant