Re: [PATCH 03/10] iommu: Add generic_single_device_group()

[Baolu Lu <baolu.lu@linux.intel.com>]

On 2023/7/19 3:05, Jason Gunthorpe wrote:
> This implements the common pattern seen in drivers of a single
> iommu_group for the entire iommu driver. Implement this in core code
> so the drivers that want this can select it from their ops.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>   drivers/iommu/iommu.c | 25 +++++++++++++++++++++++++
>   include/linux/iommu.h |  3 +++
>   2 files changed, 28 insertions(+)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 9e41ad4e3219b6..1e0c5d9a0370fb 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -289,6 +289,9 @@ void iommu_device_unregister(struct iommu_device *iommu)
>   	spin_lock(&iommu_device_lock);
>   	list_del(&iommu->list);
>   	spin_unlock(&iommu_device_lock);
> +
> +	/* Pairs with the alloc in generic_single_device_group() */
> +	iommu_group_put(iommu->singleton_group);
>   }
>   EXPORT_SYMBOL_GPL(iommu_device_unregister);
>   
> @@ -1595,6 +1598,28 @@ struct iommu_group *generic_device_group(struct device *dev)
>   }
>   EXPORT_SYMBOL_GPL(generic_device_group);
>   
> +/*
> + * Generic device_group call-back function. It just allocates one
> + * iommu-group per iommu driver.
> + */
> +struct iommu_group *generic_single_device_group(struct device *dev)
> +{
> +	struct iommu_device *iommu = dev->iommu->iommu_dev;
> +
> +	lockdep_assert_held(&dev_iommu_group_lock);
> +
> +	if (!iommu->singleton_group) {
> +		struct iommu_group *group;
> +
> +		group = iommu_group_alloc();
> +		if (IS_ERR(group))
> +			return group;
> +		iommu->singleton_group = group;
> +	}
> +	return iommu_group_ref_get(iommu->singleton_group);
> +}
> +EXPORT_SYMBOL_GPL(generic_single_device_group);

When allocating the singleton group for the first time, the group's
refcount is taken twice. This can cause memory leaks even after calling
iommu_device_unregister(). Perhaps it can be adjusted as follows?

struct iommu_group *generic_single_device_group(struct device *dev)
{
         struct iommu_device *iommu = dev->iommu->iommu_dev;
         struct iommu_group *group;

         lockdep_assert_held(&dev_iommu_group_lock);

         if (iommu->singleton_group)
                 return iommu_group_ref_get(iommu->singleton_group);

         group = iommu_group_alloc();
         if (!IS_ERR(group))
                 iommu->singleton_group = group;

         return group;
}

> +
>   /*
>    * Use standard PCI bus topology, isolation features, and DMA alias quirks
>    * to find or create an IOMMU group for a device.
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index b1dcb1b9b17040..f1e18e81fca78b 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -361,6 +361,7 @@ struct iommu_domain_ops {
>    * @list: Used by the iommu-core to keep a list of registered iommus
>    * @ops: iommu-ops for talking to this iommu
>    * @dev: struct device for sysfs handling
> + * @singleton_group: Used internally for drivers that have only one group
>    * @max_pasids: number of supported PASIDs
>    */
>   struct iommu_device {
> @@ -368,6 +369,7 @@ struct iommu_device {
>   	const struct iommu_ops *ops;
>   	struct fwnode_handle *fwnode;
>   	struct device *dev;
> +	struct iommu_group *singleton_group;
>   	u32 max_pasids;
>   };
>   
> @@ -640,6 +642,7 @@ extern struct iommu_group *pci_device_group(struct device *dev);
>   extern struct iommu_group *generic_device_group(struct device *dev);
>   /* FSL-MC device grouping function */
>   struct iommu_group *fsl_mc_device_group(struct device *dev);
> +extern struct iommu_group *generic_single_device_group(struct device *dev);

"extern" is not necessary.

>   
>   /**
>    * struct iommu_fwspec - per-device IOMMU instance data

Best regards,
baolu