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

From: Baolu Lu <baolu.lu@linux.intel.com>
To: Jason Gunthorpe <jgg@nvidia.com>
Cc: baolu.lu@linux.intel.com,
	Baolin Wang <baolin.wang@linux.alibaba.com>,
	David Woodhouse <dwmw2@infradead.org>,
	Heiko Stuebner <heiko@sntech.de>,
	iommu@lists.linux.dev, Jernej Skrabec <jernej.skrabec@gmail.com>,
	Joerg Roedel <joro@8bytes.org>,
	linux-arm-kernel@lists.infradead.org,
	linux-rockchip@lists.infradead.org, linux-sunxi@lists.linux.dev,
	Orson Zhai <orsonzhai@gmail.com>,
	Robin Murphy <robin.murphy@arm.com>,
	Samuel Holland <samuel@sholland.org>,
	Chen-Yu Tsai <wens@csie.org>, Will Deacon <will@kernel.org>,
	Chunyan Zhang <zhang.lyra@gmail.com>,
	Alex Williamson <alex.williamson@redhat.com>
Subject: Re: [PATCH 03/10] iommu: Add generic_single_device_group()
Date: Thu, 20 Jul 2023 22:01:54 +0800	[thread overview]
Message-ID: <c98c37bb-57c5-faec-1d17-730f3ceb2b21@linux.intel.com> (raw)
In-Reply-To: <ZLkiu7IyimCckiFs@nvidia.com>

On 2023/7/20 20:04, Jason Gunthorpe wrote:
> On Thu, Jul 20, 2023 at 03:39:27PM +0800, Baolu Lu wrote:
>> 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.
> Yes, that is correct.
> 
> The refcount from alloc belongs to iommu->singleton_group and the
> pair'd put is here:
> 
> @@ -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);
>   }
> 
> The refcount from iommu_group_ref_get() belongs to the caller and the
> caller must have a paired put.

Oh, yes! The extra reference counter is paired with above put.

Thanks for the explanation.

Then, another small comment:

iommu->singleton_group will be freed with above put, right? Do you need
to set iommu->singleton_group to NULL? Given that iommu_device is not
freed here.

Best regards,
baolu

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel