Messages in this thread | | | Date | Tue, 12 Dec 2023 13:07:17 +0800 | Subject | Re: [PATCH v8 12/12] iommu: Use refcount for fault data access | From | Baolu Lu <> |
| |
On 12/11/23 11:24 PM, Jason Gunthorpe wrote: > On Thu, Dec 07, 2023 at 02:43:08PM +0800, Lu Baolu wrote: >> @@ -217,12 +250,9 @@ int iommu_page_response(struct device *dev, >> if (!ops->page_response) >> return -ENODEV; >> >> - mutex_lock(¶m->lock); >> - fault_param = param->fault_param; >> - if (!fault_param) { >> - mutex_unlock(¶m->lock); >> + fault_param = iopf_get_dev_fault_param(dev); >> + if (!fault_param) >> return -EINVAL; >> - } > The refcounting should work by passing around the fault_param object, > not re-obtaining it from the dev from a work. > > The work should be locked to the iommu_fault_param that was active > when the work was launched. > > When we get to iommu_page_response it does this: > > /* Only send response if there is a fault report pending */ > mutex_lock(&fault_param->lock); > if (list_empty(&fault_param->faults)) { > dev_warn_ratelimited(dev, "no pending PRQ, drop response\n"); > goto done_unlock; > } > > Which determines that the iommu_fault_param is stale and pending > free..
Yes, agreed. The iopf_fault_param should be passed in together with the iopf_group. The reference count should be released in the iopf_free_group(). These two helps could look like below:
int iommu_page_response(struct iopf_group *group, struct iommu_page_response *msg) { bool needs_pasid; int ret = -EINVAL; struct iopf_fault *evt; struct iommu_fault_page_request *prm; struct device *dev = group->fault_param->dev; const struct iommu_ops *ops = dev_iommu_ops(dev); bool has_pasid = msg->flags & IOMMU_PAGE_RESP_PASID_VALID; struct iommu_fault_param *fault_param = group->fault_param;
if (!ops->page_response) return -ENODEV;
/* Only send response if there is a fault report pending */ mutex_lock(&fault_param->lock); if (list_empty(&fault_param->faults)) { dev_warn_ratelimited(dev, "no pending PRQ, drop response\n"); goto done_unlock; } /* * Check if we have a matching page request pending to respond, * otherwise return -EINVAL */ list_for_each_entry(evt, &fault_param->faults, list) { prm = &evt->fault.prm; if (prm->grpid != msg->grpid) continue;
/* * If the PASID is required, the corresponding request is * matched using the group ID, the PASID valid bit and the PASID * value. Otherwise only the group ID matches request and * response. */ needs_pasid = prm->flags & IOMMU_FAULT_PAGE_RESPONSE_NEEDS_PASID; if (needs_pasid && (!has_pasid || msg->pasid != prm->pasid)) continue;
if (!needs_pasid && has_pasid) { /* No big deal, just clear it. */ msg->flags &= ~IOMMU_PAGE_RESP_PASID_VALID; msg->pasid = 0; }
ret = ops->page_response(dev, evt, msg); list_del(&evt->list); kfree(evt); break; }
done_unlock: mutex_unlock(&fault_param->lock);
return ret; }
...
void iopf_free_group(struct iopf_group *group) { struct iopf_fault *iopf, *next;
list_for_each_entry_safe(iopf, next, &group->faults, list) { if (!(iopf->fault.prm.flags & IOMMU_FAULT_PAGE_REQUEST_LAST_PAGE)) kfree(iopf); }
/* Pair with iommu_report_device_fault(). */ iopf_put_dev_fault_param(group->fault_param); kfree(group); }
Best regards, baolu
| |