Re: [PATCH v7 11/12] iommu: Consolidate per-device fault data management

[Baolu Lu]

On 12/2/23 3:46 AM, Jason Gunthorpe wrote:
> On Wed, Nov 15, 2023 at 11:02:25AM +0800, Lu Baolu wrote:
>
> > diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> > index d19031c1b0e6..c17d5979d70d 100644
> > --- a/include/linux/iommu.h
> > +++ b/include/linux/iommu.h
> > @@ -597,6 +597,8 @@ struct iommu_device {
> >   /**
> >    * struct iommu_fault_param - per-device IOMMU fault data
> >    * @lock: protect pending faults list
> > + * @users: user counter to manage the lifetime of the data, this field
> > + *         is protected by dev->iommu->lock.
> >    * @dev: the device that owns this param
> >    * @queue: IOPF queue
> >    * @queue_list: index into queue->devices
> > @@ -606,6 +608,7 @@ struct iommu_device {
> >    */
> >   struct iommu_fault_param {
> >   	struct mutex lock;
> > +	int users;
>
> Use refcount_t for the debugging features

Yes.

> >   	struct device *dev;
> >   	struct iopf_queue *queue;
>
> But why do we need this to be refcounted? iopf_queue_remove_device()
> is always called before we get to release? This struct isn't very big
> so I'd just leave it allocated and free it during release?

iopf_queue_remove_device() should always be called before device
release.

The reference counter is implemented to synchronize access to the fault
parameter among different paths. For example, iopf_queue_remove_device()
removes the parameter, while iommu_report_device_fault() and
iommu_page_response() have needs to reference it. These three paths
could possibly happen in different threads.

> > @@ -72,23 +115,14 @@ static int iommu_handle_iopf(struct iommu_fault *fault, struct device *dev)
> >   	struct iopf_group *group;
> >   	struct iopf_fault *iopf, *next;
> >   	struct iommu_domain *domain = NULL;
> > -	struct iommu_fault_param *iopf_param;
> > -	struct dev_iommu *param = dev->iommu;
> > +	struct iommu_fault_param *iopf_param = dev->iommu->fault_param;
> >   
> > -	lockdep_assert_held(&param->lock);
> > +	lockdep_assert_held(&iopf_param->lock);
>
> This patch seems like it is doing a few things, can the locking
> changes be kept in their own patch?

Yes. Let me try to.

Best regards,
baolu