Re: [PATCH v4 05/16] drivers/iommu: Take a ref to the IOMMU driver prior to ->add_device()

[Greg Kroah-Hartman]

On Thu, Jan 09, 2020 at 02:16:03PM +0000, Will Deacon wrote:
> Hi Greg,
> 
> On Thu, Dec 19, 2019 at 03:44:37PM +0100, Greg Kroah-Hartman wrote:
> > On Thu, Dec 19, 2019 at 12:03:41PM +0000, Will Deacon wrote:
> > > diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> > > index f2223cbb5fd5..e9f94d3f7a04 100644
> > > --- a/include/linux/iommu.h
> > > +++ b/include/linux/iommu.h
> > > @@ -246,9 +246,10 @@ struct iommu_iotlb_gather {
> > >   * @sva_get_pasid: Get PASID associated to a SVA handle
> > >   * @page_response: handle page request response
> > >   * @cache_invalidate: invalidate translation caches
> > > - * @pgsize_bitmap: bitmap of all possible supported page sizes
> > >   * @sva_bind_gpasid: bind guest pasid and mm
> > >   * @sva_unbind_gpasid: unbind guest pasid and mm
> > > + * @pgsize_bitmap: bitmap of all possible supported page sizes
> > > + * @owner: Driver module providing these ops
> > >   */
> > >  struct iommu_ops {
> > >  	bool (*capable)(enum iommu_cap);
> > > @@ -318,6 +319,7 @@ struct iommu_ops {
> > >  	int (*sva_unbind_gpasid)(struct device *dev, int pasid);
> > >  
> > >  	unsigned long pgsize_bitmap;
> > > +	struct module *owner;
> > 
> > Everyone is always going to forget to set this field.  I don't think you
> > even set it for all of the different iommu_ops possible in this series,
> > right?
> 
> I only initialised the field for those drivers which can actually be built
> as a module, but I take your point about this being error-prone.
> 
> > The "trick" we did to keep people from having to remember this is to do
> > what we did for the bus registering functions.
> > 
> > Look at pci_register_driver in pci.h:
> > #define pci_register_driver(driver)             \
> >         __pci_register_driver(driver, THIS_MODULE, KBUILD_MODNAME)
> > 
> > Then we set the .owner field in the "real" __pci_register_driver() call.
> > 
> > Same thing for USB and lots, if not all, other driver register
> > functions.
> > 
> > You can do the same thing here, and I would recommend it.
> 
> Yes, that makes sense, cheers. Diff below. I'll send it to Joerg along
> with some other SMMU patches that have come in since the holiday.
> 
> Will
> 
> --->8
> 
> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> index 03dc97842875..e82997a705a8 100644
> --- a/drivers/iommu/arm-smmu-v3.c
> +++ b/drivers/iommu/arm-smmu-v3.c
> @@ -2733,7 +2733,6 @@ static struct iommu_ops arm_smmu_ops = {
>  	.get_resv_regions	= arm_smmu_get_resv_regions,
>  	.put_resv_regions	= arm_smmu_put_resv_regions,
>  	.pgsize_bitmap		= -1UL, /* Restricted during device attach */
> -	.owner			= THIS_MODULE,
>  };
>  
>  /* Probing and initialisation functions */
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index 5ef1f2e100d7..93d332423f6f 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -1623,7 +1623,6 @@ static struct iommu_ops arm_smmu_ops = {
>  	.get_resv_regions	= arm_smmu_get_resv_regions,
>  	.put_resv_regions	= arm_smmu_put_resv_regions,
>  	.pgsize_bitmap		= -1UL, /* Restricted during device attach */
> -	.owner			= THIS_MODULE,
>  };
>  
>  static void arm_smmu_device_reset(struct arm_smmu_device *smmu)
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index e9f94d3f7a04..90007c92ad2d 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -388,12 +388,19 @@ void iommu_device_sysfs_remove(struct iommu_device *iommu);
>  int  iommu_device_link(struct iommu_device   *iommu, struct device *link);
>  void iommu_device_unlink(struct iommu_device *iommu, struct device *link);
>  
> -static inline void iommu_device_set_ops(struct iommu_device *iommu,
> -					const struct iommu_ops *ops)
> +static inline void __iommu_device_set_ops(struct iommu_device *iommu,
> +					  const struct iommu_ops *ops)
>  {
>  	iommu->ops = ops;
>  }
>  
> +#define iommu_device_set_ops(iommu, ops)				\
> +do {									\
> +	struct iommu_ops *__ops = (struct iommu_ops *)(ops);		\
> +	__ops->owner = THIS_MODULE;					\
> +	__iommu_device_set_ops(iommu, __ops);				\
> +} while (0)
> +
>  static inline void iommu_device_set_fwnode(struct iommu_device *iommu,
>  					   struct fwnode_handle *fwnode)
>  {

Looks good:

Reviewed-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>