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
> /* 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
Jason
