Re: [PATCH v3 02/13] iommu/amd: Introduce get_amd_iommu_from_dev()

[Jason Gunthorpe <jgg@ziepe.ca>]

On Fri, Oct 13, 2023 at 03:16:41PM +0000, Vasant Hegde wrote:
> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> 
> And replace rlookup_amd_iommu() with the new helper function where
> applicable to avoid unnecessary loop to look up struct amd_iommu from
> struct device.
> 
> Suggested-by: Jason Gunthorpe <jgg@ziepe.ca>
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
> ---
>  drivers/iommu/amd/amd_iommu.h | 14 ++++++++++++++
>  drivers/iommu/amd/iommu.c     | 20 ++++++++++----------
>  include/linux/iommu.h         | 13 +++++++++++++
>  3 files changed, 37 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/iommu/amd/amd_iommu.h b/drivers/iommu/amd/amd_iommu.h
> index 38b3f4562f3b..b2071ebc73b5 100644
> --- a/drivers/iommu/amd/amd_iommu.h
> +++ b/drivers/iommu/amd/amd_iommu.h
> @@ -150,6 +150,20 @@ static inline void *alloc_pgtable_page(int nid, gfp_t gfp)
>  	return page ? page_address(page) : NULL;
>  }
>  
> +/*
> + * This must be called after device probe completes. During probe
> + * use rlookup_amd_iommu() get the iommu.
> + */
> +static inline struct amd_iommu *get_amd_iommu_from_dev(struct device *dev)
> +{
> +	struct iommu_device *iommu = iommu_get_iommu_dev(dev);
> +
> +	if (!iommu)
> +		return NULL;

This shouldn't be done. See the comment for iommu_get_iommu_dev(). If
you are calling this outside an op context it is broken and the if
won't save it.

Ideally you'd put these calls only at the top of functions
implementing ops and then pass either the amd_iommu or iommu_dev_data
pointers down the call chain.

> +	return container_of(iommu, struct amd_iommu, iommu);
> +}
> +
>  bool translation_pre_enabled(struct amd_iommu *iommu);
>  bool amd_iommu_is_attach_deferred(struct device *dev);
>  int __init add_special_device(u8 type, u8 id, u32 *devid, bool cmd_line);
> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
> index 4f1b356adb8f..eedfa341085c 100644
> --- a/drivers/iommu/amd/iommu.c
> +++ b/drivers/iommu/amd/iommu.c
> @@ -1408,7 +1408,7 @@ static int device_flush_iotlb_range(struct iommu_dev_data *dev_data,
>  	bool gn = is_pasid_valid(pasid);
>
>  	qdep     = dev_data->ats_qdep;
> -	iommu    = rlookup_amd_iommu(dev_data->dev);
> +	iommu    = get_amd_iommu_from_dev(dev_data->dev);
>  	if (!iommu)
>  		return -EINVAL;

Eg here we have an iommu_dev_data which must mean the device is probed
and iommu is valid.

> +/**
> + * iommu_get_iommu_dev - Get iommu_device for a device
> + * @dev: an end-point device
> + *
> + * Note that this function must be called from the iommu_ops
> + * to retrieve the iommu_device for a device, which the core code
> + * guarentees it will not invoke the op without an attached iommu.
                                                  ^^^^^^^^^^^^^^^^^^

Means the function never returns NULL.

Jason