Re: [PATCH v2 09/10] iommu: Use dev_iommu_ops() helper

[Jason Gunthorpe <jgg@nvidia.com>]

On Tue, Feb 08, 2022 at 09:25:58AM +0800, Lu Baolu wrote:
> Convert all the feasible instances of dev->bus->iommu_ops to
> dev_iommu_ops() in order to making the operation of obtaining
> iommu_ops from a device consistent.

Why are there two patches doing this conversion? Roll this into the
prior patch?

>  void iommu_get_resv_regions(struct device *dev, struct list_head *list)
>  {
> -	const struct iommu_ops *ops = dev->bus->iommu_ops;
> +	const struct iommu_ops *ops = dev_iommu_ops(dev);
>  
>  	if (ops && ops->get_resv_regions)
>  		ops->get_resv_regions(dev, list);

And agree with Christoph, don't keep confusing ops null tests -
dev_iommu_ops() never returns null and any function using it must rely
on the caller to handle this, somehow.

However, I wonder how safe this is. Certainly some APIs like this one
it is fine, but I would be happier if the 'first' APIs like
bind/attach/alloc/etc fail silently upwards. In many cases these APIs
are called based on things like DT configuration, or VFIO or
something, and the caller is expecting the iommu layer to do all
necessary validation.

> @@ -2788,7 +2789,7 @@ iommu_sva_bind_device(struct device *dev, struct mm_struct *mm, void *drvdata)
>  {
>  	struct iommu_group *group;
>  	struct iommu_sva *handle = ERR_PTR(-EINVAL);
> -	const struct iommu_ops *ops = dev->bus->iommu_ops;
> +	const struct iommu_ops *ops = dev_iommu_ops(dev);
>  
>  	if (!ops || !ops->sva_bind)
>  		return ERR_PTR(-ENODEV);
> @@ -2831,7 +2832,7 @@ void iommu_sva_unbind_device(struct iommu_sva *handle)
>  {
>  	struct iommu_group *group;
>  	struct device *dev = handle->dev;
> -	const struct iommu_ops *ops = dev->bus->iommu_ops;
> +	const struct iommu_ops *ops = dev_iommu_ops(dev);
>  
>  	if (!ops || !ops->sva_unbind)
>  		return;
> @@ -2850,7 +2851,7 @@ EXPORT_SYMBOL_GPL(iommu_sva_unbind_device);
>  
>  u32 iommu_sva_get_pasid(struct iommu_sva *handle)
>  {
> -	const struct iommu_ops *ops = handle->dev->bus->iommu_ops;
> +	const struct iommu_ops *ops = dev_iommu_ops(handle->dev);
>  
>  	if (!ops || !ops->sva_get_pasid)
>  		return IOMMU_PASID_INVALID;

We all agreed that this sva object will turn into a domain and thus
all of this will eventually move to domain ops?

Jason