On 2024/4/17 10:30, Baolu Lu wrote:
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@xxxxxxxxx>
---
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.
aha, I didn't make it clear. It should be no present pasid entry if no
dev_pasid. Anyhow, I noticed intel_pasid_tear_down_entry() checks present
first before going ahead. So keep going is ok to me now.
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.
TBH. I don't know if it is necessary to make every internal helpers
self-contained. It looks to be more readable to avoid unnecessary
function calls in the caller side. :)
--
Regards,
Yi Liu
