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

[Jason Gunthorpe <jgg@ziepe.ca>]

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.

Jason