Re: [PATCH v2 10/12] iommu/vt-d: Return if no dev_pasid is found in domain

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

On 4/16/24 5:21 PM, Yi Liu wrote:
> 
> n 2024/4/15 14:04, Baolu Lu wrote:
>> On 4/12/24 4:15 PM, Yi Liu wrote:
>>> If no dev_pasid is found, it should be a problem of caller. So a WARN_ON
>>> is fine, but no need to go further as nothing to be cleanup and also it
>>> may hit unknown issue.
>>
>> If "... it should be a problem of caller ...", then the check and WARN()
>> should be added in the caller instead of individual drivers.
>>
>>>
>>> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
>>> ---
>>>   drivers/iommu/intel/iommu.c | 3 ++-
>>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
>>> index df49aed3df5e..fff7dea012a7 100644
>>> --- a/drivers/iommu/intel/iommu.c
>>> +++ b/drivers/iommu/intel/iommu.c
>>> @@ -4614,8 +4614,9 @@ static void intel_iommu_remove_dev_pasid(struct 
>>> device *dev, ioasid_t pasid,
>>>               break;
>>>           }
>>>       }
>>> -    WARN_ON_ONCE(!dev_pasid);
>>>       spin_unlock_irqrestore(&dmar_domain->lock, flags);
>>> +    if (WARN_ON_ONCE(!dev_pasid))
>>> +        return;
>>
>> The iommu core calls remove_dev_pasid op to tear down the translation on
>> a pasid and park it in a BLOCKED state. Since this is a must-be-
>> successful callback, it makes no sense to return before tearing down the
>> pasid table entry.
> 
> but if no dev_pasid is found, does it mean there is no pasid table entry
> to be destroyed? That's why I think it deserves a warn, but no need to
> continue.

The pasid table is allocated in the iommu probe path, hence the entry is
*always* there. Teardown a pasid translation just means zeroing out all
fields of the entry.

> 
>>
>>  From the Intel iommu driver's perspective, the pasid devices have
>> already been tracked in the core, hence the dev_pasid is a duplicate and
>> will be removed later, so don't use it for other purposes.
> 
> 
> good to know it. But for the current code, if we continue, it would hit
> call trace in the end in the intel_iommu_debugfs_remove_dev_pasid().

The debugfs interface should be designed to be self-contained. That
means, even if one passes a NULL pointer to
intel_iommu_debugfs_remove_dev_pasid(), it should handle it gracefully.

Best regards,
baolu