Re: [PATCH v2] iommu: Handle iommu faults for a bad iopf setup

[Pranjal Shrivastava <praan@google.com>]

On Thu, Aug 15, 2024 at 03:03:07PM +0100, Robin Murphy wrote:
> On 15/08/2024 1:46 pm, Pranjal Shrivastava wrote:
> [...]> @@ -155,22 +208,37 @@ static struct iopf_group
> *iopf_group_alloc(struct iommu_fault_param *iopf_param,
> >    * hardware has been set to block the page faults) and the pending page faults
> >    * have been flushed.
> >    */
> > -void iommu_report_device_fault(struct device *dev, struct iopf_fault *evt)
> > +int iommu_report_device_fault(struct device *dev, struct iopf_fault *evt)
> >   {
> > +	struct iommu_attach_handle *attach_handle;
> >   	struct iommu_fault *fault = &evt->fault;
> >   	struct iommu_fault_param *iopf_param;
> >   	struct iopf_group abort_group = {};
> >   	struct iopf_group *group;
> > +	int ret = 0;
> > +	attach_handle = find_fault_handler(dev, evt);
> > +	if (!attach_handle) {
> > +		ret = -EINVAL;
> > +		goto err_bad_iopf;
> > +	}
> > +
> > +	/*
> > +	 * Something has gone wrong if a fault capable domain is attached but no
> > +	 * iopf_param is setup
> > +	 */
> >   	iopf_param = iopf_get_dev_fault_param(dev);
> > -	if (WARN_ON(!iopf_param))
> > -		return;
> > +	if (WARN_ON(!iopf_param)) {
> > +		ret = -EINVAL;
> > +		goto err_bad_iopf;
> 
> Nit: the err_bad_iopf path is only ever returning -EINVAL, so you could
> simply hard-code that there...
> 
> > +	}
> >   	if (!(fault->prm.flags & IOMMU_FAULT_PAGE_REQUEST_LAST_PAGE)) {
> > -		report_partial_fault(iopf_param, fault);
> > +		ret = report_partial_fault(iopf_param, fault);
> 
> ...and declare "ret" here in the one scope it's actually needed.

Sure, will refactor this in the next version. Let me also swap the
signed-off-bys and prefix with rc as suggested by Jason in v3.

> 
> Thanks,
> Robin.
> 
> >   		iopf_put_dev_fault_param(iopf_param);
> >   		/* A request that is not the last does not need to be ack'd */
> > -		return;
> > +
> > +		return ret;
> >   	}
> >   	/*

Thanks,
Pranjal