Re: [PATCH v5 5/8] iommufd/vdevice: Remove struct device reference from struct vdevice

[Xu Yilun]

On Tue, Jul 15, 2025 at 11:56:31AM -0700, Nicolin Chen wrote:
> On Tue, Jul 15, 2025 at 02:32:42PM +0800, Xu Yilun wrote:
> > diff --git a/drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c b/drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c
> > index eb90af5093d8..8a515987b948 100644
> > --- a/drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c
> > +++ b/drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c
> > @@ -1218,7 +1218,7 @@ static void tegra241_vintf_destroy_vsid(struct iommufd_vdevice *vdev)
> >  
> >  static int tegra241_vintf_init_vsid(struct iommufd_vdevice *vdev)
> >  {
> > -	struct arm_smmu_master *master = dev_iommu_priv_get(vdev->dev);
> > +	struct arm_smmu_master *master = dev_iommu_priv_get(vdev->idev->dev);
> 
> Hmm, this breaks :(
> 
> drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c: In function 'tegra241_vintf_init_vsid':
> drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c:1230:71: error: invalid use of undefined type 'struct iommufd_device'
>  1230 |         struct arm_smmu_master *master = dev_iommu_priv_get(vdev->idev->dev);
> 
> Unfortunately the iommufd_device structure is defined in the
> private header that's not shared with any IOMMU driver.
> 
> So, we need in the driver.c a new helper that converts a vdev
> pointer to dev. Something like:
> 
> diff --git a/drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c b/drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c
> index ff6bbd2137146..fd6b083535271 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c
> @@ -1227,7 +1227,8 @@ static void tegra241_vintf_destroy_vsid(struct iommufd_vdevice *vdev)
>  
>  static int tegra241_vintf_init_vsid(struct iommufd_vdevice *vdev)
>  {
> -	struct arm_smmu_master *master = dev_iommu_priv_get(vdev->idev->dev);
> +	struct device *dev = iommufd_vdevice_to_device(vdev);
> +	struct arm_smmu_master *master = dev_iommu_priv_get(dev);
>  	struct tegra241_vintf *vintf = viommu_to_vintf(vdev->viommu);
>  	struct tegra241_vintf_sid *vsid = vdev_to_vsid(vdev);
>  	struct arm_smmu_stream *stream = &master->streams[0];
> diff --git a/drivers/iommu/iommufd/driver.c b/drivers/iommu/iommufd/driver.c
> index df25db6d2eafc..6f1010da221c9 100644
> --- a/drivers/iommu/iommufd/driver.c
> +++ b/drivers/iommu/iommufd/driver.c
> @@ -83,6 +83,12 @@ void _iommufd_destroy_mmap(struct iommufd_ctx *ictx,
>  }
>  EXPORT_SYMBOL_NS_GPL(_iommufd_destroy_mmap, "IOMMUFD");
>  
> +struct device *iommufd_vdevice_to_device(struct iommufd_vdevice *vdev)
> +{
> +	return vdev->idev->dev;
> +}
> +EXPORT_SYMBOL_NS_GPL(iommufd_vdevice_to_device, "IOMMUFD");
> +
>  /* Caller should xa_lock(&viommu->vdevs) to protect the return value */
>  struct device *iommufd_viommu_find_dev(struct iommufd_viommu *viommu,
>  				       unsigned long vdev_id)
> @@ -92,7 +98,7 @@ struct device *iommufd_viommu_find_dev(struct iommufd_viommu *viommu,
>  	lockdep_assert_held(&viommu->vdevs.xa_lock);
>  
>  	vdev = xa_load(&viommu->vdevs, vdev_id);
> -	return vdev ? vdev->idev->dev : NULL;
> +	return vdev ? iommufd_vdevice_to_device(vdev) : NULL;
>  }
>  EXPORT_SYMBOL_NS_GPL(iommufd_viommu_find_dev, "IOMMUFD");
>  
> @@ -109,7 +115,7 @@ int iommufd_viommu_get_vdev_id(struct iommufd_viommu *viommu,
>  
>  	xa_lock(&viommu->vdevs);
>  	xa_for_each(&viommu->vdevs, index, vdev) {
> -		if (vdev->idev->dev == dev) {
> +		if (iommufd_vdevice_to_device(vdev) == dev) {
>  			*vdev_id = vdev->virt_id;
>  			rc = 0;
>  			break;
> diff --git a/include/linux/iommufd.h b/include/linux/iommufd.h
> index 61410a78cbce7..ee88e90021870 100644
> --- a/include/linux/iommufd.h
> +++ b/include/linux/iommufd.h
> @@ -266,6 +266,7 @@ int _iommufd_alloc_mmap(struct iommufd_ctx *ictx, struct iommufd_object *owner,
>  			unsigned long *offset);
>  void _iommufd_destroy_mmap(struct iommufd_ctx *ictx,
>  			   struct iommufd_object *owner, unsigned long offset);
> +struct device *iommufd_vdevice_to_device(struct iommufd_vdevice *vdev);
>  struct device *iommufd_viommu_find_dev(struct iommufd_viommu *viommu,
>  				       unsigned long vdev_id);
>  int iommufd_viommu_get_vdev_id(struct iommufd_viommu *viommu,
> @@ -300,6 +301,12 @@ static inline void _iommufd_destroy_mmap(struct iommufd_ctx *ictx,
>  {
>  }
>  
> +static inline struct device *
> +iommufd_vdevice_to_device(struct iommufd_vdevice *vdev)
> +{
> +	return ERR_PTR(-ENODEV);

I prefer return NULL, which is consistent with iommufd_viommu_find_dev().

Others good to me, and thanks for your fixing.

> +}
> +
>  static inline struct device *
>  iommufd_viommu_find_dev(struct iommufd_viommu *viommu, unsigned long vdev_id)
>  {
>