Re: [PATCH v5 10/17] iommu: Introduce iommu_group_mutex_assert()

[Vasant Hegde <vasant.hegde@amd.com>]

On 1/22/2024 11:30 PM, Jason Gunthorpe wrote:
> On Mon, Jan 22, 2024 at 11:54:39AM +0530, Vasant Hegde wrote:
>> Jason,
>>
>>
>> On 1/20/2024 12:39 AM, Jason Gunthorpe wrote:
>>> On Tue, Jan 16, 2024 at 04:53:28PM +0000, Vasant Hegde wrote:
>>>
>>>> +/**
>>>> + * iommu_group_mutex_assert - Check device group mutex lock
>>>> + * @dev: the device that has group param set
>>>> + *
>>>> + * This function is called by an iommu driver to check whether it holds
>>>> + * group mutex lock for the given device or not.
>>>> + *
>>>> + * Note that this function must be called after device group param is set.
>>>> + */
>>>> +void iommu_group_mutex_assert(struct device *dev)
>>>> +{
>>>> +	struct iommu_group *group = dev->iommu_group;
>>>> +
>>>> +	lockdep_assert_held(&group->mutex);
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(iommu_group_mutex_assert);
>>>> +
>>>>  static struct device *iommu_group_first_dev(struct iommu_group *group)
>>>>  {
>>>>  	lockdep_assert_held(&group->mutex);
>>>> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
>>>> index 7f6342bc71c3..c983b6a1ebce 100644
>>>> --- a/include/linux/iommu.h
>>>> +++ b/include/linux/iommu.h
>>>> @@ -751,6 +751,7 @@ extern int iommu_group_set_name(struct iommu_group *group, const char *name);
>>>>  extern int iommu_group_add_device(struct iommu_group *group,
>>>>  				  struct device *dev);
>>>>  extern void iommu_group_remove_device(struct device *dev);
>>>> +extern void iommu_group_mutex_assert(struct device *dev);
>>>
>>> This shouldn't be unconditional. Like this outside the other ifdefs:
>>>
>>> #if IS_ENABLED(CONFIG_LOCKDEP) && IS_ENABLED(CONFIG_IOMMU)
>>
>> This is already covered by `CONFIG_IOMMU_API` check.
>> Also lockdep_assert_held() is already covered by LOCKDEP config check in
>> lockdep.h file.
>>
>> So I think another explicit check in redundant.
>>
>> Is there  any other reason to have explicit check?
> 
> We don't want the out of line function call overhead even when lockdep
> is not turned on.
> 
> The point is that without lockdep the inline stub should be the
> only thing present and the compiler simply does nothing at all.
> 
> What you have here will generate an function call and a stack frame
> push to do nothing.
> 

Ok. Will fix it.

-Vasant