Messages in this thread | ![/](/images/icornerl.gif) | | Date | Wed, 13 Dec 2023 10:19:32 +0800 | Subject | Re: [PATCH v8 12/12] iommu: Use refcount for fault data access | From | Baolu Lu <> |
| |
On 12/12/23 11:18 PM, Jason Gunthorpe wrote: > On Tue, Dec 12, 2023 at 01:07:17PM +0800, Baolu Lu wrote: > >> 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; > > We should never get here if this is the case, prevent the device from > being added in the first place
Yeah, could move it to iopf_queue_add_device(). WARN and return failure there if the driver is not ready for page request handling.
> >> /* 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); > > I would have expected the group to free'd here? But regardless this > looks like a good direction
Both work for me. We can decide it according to the needs of code later.
> > Jason
Best regards, baolu
| ![\](/images/icornerr.gif) |