Re: [PATCH v7 04/24] iommu: Add a page fault handler

[Jean-Philippe Brucker <jean-philippe@linaro.org>]

Hi Xiang,

Thank you for reviewing this. I forgot to send a reply, sorry for the
delay.

On Fri, May 29, 2020 at 05:18:27PM +0800, Xiang Zheng wrote:
> Hi,
> 
> On 2020/5/20 1:54, Jean-Philippe Brucker wrote:
> > Some systems allow devices to handle I/O Page Faults in the core mm. For
> > example systems implementing the PCIe PRI extension or Arm SMMU stall
> > model. Infrastructure for reporting these recoverable page faults was
> > added to the IOMMU core by commit 0c830e6b3282 ("iommu: Introduce device
> > fault report API"). Add a page fault handler for host SVA.
> > 
> > IOMMU driver can now instantiate several fault workqueues and link them
> > to IOPF-capable devices. Drivers can choose between a single global
> > workqueue, one per IOMMU device, one per low-level fault queue, one per
> > domain, etc.
> > 
> > When it receives a fault event, supposedly in an IRQ handler, the IOMMU
> > driver reports the fault using iommu_report_device_fault(), which calls
> > the registered handler. The page fault handler then calls the mm fault
> > handler, and reports either success or failure with iommu_page_response().
> > When the handler succeeded, the IOMMU retries the access.
> > 
> > The iopf_param pointer could be embedded into iommu_fault_param. But
> > putting iopf_param into the iommu_param structure allows us not to care
> > about ordering between calls to iopf_queue_add_device() and
> > iommu_register_device_fault_handler().
> > 
> > Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
[...]
> > +/**
> > + * iopf_queue_free - Free IOPF queue
> > + * @queue: queue to free
> > + *
> > + * Counterpart to iopf_queue_alloc(). The driver must not be queuing faults or
> > + * adding/removing devices on this queue anymore.
> > + */
> > +void iopf_queue_free(struct iopf_queue *queue)
> > +{
> > +	struct iopf_device_param *iopf_param, *next;
> > +
> > +	if (!queue)
> > +		return;
> > +
> > +	list_for_each_entry_safe(iopf_param, next, &queue->devices, queue_list)
> > +		iopf_queue_remove_device(queue, iopf_param->dev);
> > +
> > +	destroy_workqueue(queue->wq);
> 
> Do we need to free iopf_group(s) here in case the work queue of the group(s) are not
> scheduled yet? If that occurs, we might leak memory here.

Partial groups are freed by iopf_queue_remove_device(), and all other
groups are freed when destroy_workqueue() executes the remaining work.

Thanks,
Jean