| Message ID | 20240412081516.31168-11-yi.l.liu@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iommufd support pasid attach/replace | expand |
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. 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. In the end, perhaps we just need to remove the WARN_ON() from the code. > > domain_detach_iommu(dmar_domain, iommu); > intel_iommu_debugfs_remove_dev_pasid(dev_pasid); Best regards, baolu
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. > > 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(). > In the end, perhaps we just need to remove the WARN_ON() from the code. > >> domain_detach_iommu(dmar_domain, iommu); >> intel_iommu_debugfs_remove_dev_pasid(dev_pasid); > > Best regards, > baolu
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
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@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. 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. :)
> From: Liu, Yi L <yi.l.liu@intel.com> > Sent: Friday, April 12, 2024 4:15 PM > > 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. > > Signed-off-by: Yi Liu <yi.l.liu@intel.com> with your discussions let's remove it from this series. If there is anything to further cleanup it can be done alone.
On 2024/4/17 17:03, Tian, Kevin wrote: >> From: Liu, Yi L <yi.l.liu@intel.com> >> Sent: Friday, April 12, 2024 4:15 PM >> >> 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. >> >> Signed-off-by: Yi Liu <yi.l.liu@intel.com> > > with your discussions let's remove it from this series. > > If there is anything to further cleanup it can be done alone. ok. Would drop it from this series.
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; domain_detach_iommu(dmar_domain, iommu); intel_iommu_debugfs_remove_dev_pasid(dev_pasid);
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. Signed-off-by: Yi Liu <yi.l.liu@intel.com> --- drivers/iommu/intel/iommu.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)