Re: [PATCH v10 06/18] iommufd/device: Add helper to detect the first attach of a group

[Yi Liu <yi.l.liu@intel.com>]

On 2025/3/21 01:51, Nicolin Chen wrote:
> On Thu, Mar 20, 2025 at 10:37:01AM -0700, Nicolin Chen wrote:
>> On Thu, Mar 20, 2025 at 06:47:32AM -0700, Yi Liu wrote:
>>> The existing code detects the first attach by checking the
>>> igroup->device_list. However, the igroup->hwpt can also be used to detect
>>> the first attach. In future modifications, it is better to check the
>>> igroup->hwpt instead of the device_list. To improve readbility and also
>>> prepare for further modifications on this part, this adds a helper for it.
>>>
>>> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
>>> ---
>>> v9 -> v10: It is patch 07 of v9, it's reworked hence renamed as well.
>>> ---
>>>   drivers/iommu/iommufd/device.c | 11 +++++++++--
>>>   1 file changed, 9 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
>>> index ac54d734b819..9db36346328f 100644
>>> --- a/drivers/iommu/iommufd/device.c
>>> +++ b/drivers/iommu/iommufd/device.c
>>> @@ -444,6 +444,13 @@ static int iommufd_group_setup_msi(struct iommufd_group *igroup,
>>>   	return 0;
>>>   }
>>>   
>>> +static inline bool
>>> +igroup_first_attach(struct iommufd_group *igroup, ioasid_t pasid)
>>> +{
>>> +	lockdep_assert_held(&igroup->lock);
>>> +	return !igroup->hwpt;
>>> +}
>>> +
>>>   static int
>>>   iommufd_device_attach_reserved_iova(struct iommufd_device *idev,
>>>   				    struct iommufd_hwpt_paging *hwpt_paging)
>>> @@ -459,7 +466,7 @@ iommufd_device_attach_reserved_iova(struct iommufd_device *idev,
>>>   	if (rc)
>>>   		return rc;
>>>   
>>> -	if (list_empty(&igroup->device_list)) {
>>> +	if (igroup_first_attach(igroup, IOMMU_NO_PASID)) {
>>>   		rc = iommufd_group_setup_msi(igroup, hwpt_paging);
>>>   		if (rc) {
>>>   			iopt_remove_reserved_iova(&hwpt_paging->ioas->iopt,
>>> @@ -623,7 +630,7 @@ int iommufd_hw_pagetable_attach(struct iommufd_hw_pagetable *hwpt,
>>>   	 * reserved regions are only updated during individual device
>>>   	 * attachment.
>>>   	 */
>>> -	if (list_empty(&igroup->device_list)) {
>>> +	if (igroup_first_attach(igroup, pasid)) {
>>>   		rc = iommufd_hwpt_attach_device(hwpt, idev, pasid);
>>>   		if (rc)
>>>   			goto err_unresv;
>>
>> We have the same list_empty in the iommufd_hw_pagetable_detach()
>> and iommufd_group_release() too?
>>
>> And I feel "igroup_is_not_attached" could be clearer, as it fits
>> the detach/release context too.
> 
> Oh, I just found that the following patch changes those paths.
> 
> Yet, at the end of the series this igroup_first_attach() is quite
> similar to iommufd_device_is_attached(). So, maybe we could align
> with that the naming here: iommufd_group_is_attached?

maybe just iommufd_group_first_attach(), I would expect using it in
the attach path so far. I can add a kdoc for this.

> With that,
> Reviewed-by: Nicolin Chen <nicolinc@nvidia.com>

-- 
Regards,
Yi Liu