Re: [PATCH v2 09/10] iommu/amd: Reorder attach device code

[Joerg Roedel <joro@8bytes.org>]

On Tue, Sep 10, 2024 at 06:58:11AM +0000, Vasant Hegde wrote:
> Previous patch converted dev_data lock to mutex. Move PCI device capability
> enablement (ATS, PRI, PASID) and IOPF enablement code inside the lock. Also
> in attach_device(), update 'dev_data->domain' at the end so that error
> handling becomes simple.

This patch description lacks the 'Why?' part.

> 
> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
> ---
>  drivers/iommu/amd/iommu.c | 65 +++++++++++++++++----------------------
>  1 file changed, 29 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
> index 0f83e1b3fb0c..e7fc59e30387 100644
> --- a/drivers/iommu/amd/iommu.c
> +++ b/drivers/iommu/amd/iommu.c
> @@ -2086,6 +2086,7 @@ static int attach_device(struct device *dev,
>  {
>  	struct iommu_dev_data *dev_data = dev_iommu_priv_get(dev);
>  	struct amd_iommu *iommu = get_amd_iommu_from_dev_data(dev_data);
> +	struct pci_dev *pdev;
>  	int ret = 0;
>  
>  	mutex_lock(&dev_data->mutex);
> @@ -2095,10 +2096,6 @@ static int attach_device(struct device *dev,
>  		goto out;
>  	}
>  
> -	/* Update data structures */
> -	dev_data->domain = domain;
> -	list_add(&dev_data->list, &domain->dev_list);
> -
>  	/* Do reference counting */
>  	ret = pdom_attach_iommu(iommu, domain);
>  	if (ret)
> @@ -2113,6 +2110,28 @@ static int attach_device(struct device *dev,
>  		}
>  	}
>  
> +	pdev = dev_is_pci(dev_data->dev) ? to_pci_dev(dev_data->dev) : NULL;
> +	if (pdev && pdom_is_sva_capable(domain)) {
> +		pdev_enable_caps(pdev);
> +
> +		/*
> +		 * Device can continue to function even if IOPF
> +		 * enablement failed. Hence in error path just
> +		 * disable device PRI support.
> +		 */
> +		if (amd_iommu_iopf_add_device(iommu, dev_data))
> +			pdev_disable_cap_pri(pdev);
> +	} else if (pdev) {
> +		pdev_enable_cap_ats(pdev);
> +	}
> +
> +	/* Update data structures */
> +	dev_data->domain = domain;
> +	list_add(&dev_data->list, &domain->dev_list);
> +
> +	/* Update device table */
> +	dev_update_dte(dev_data, true);
> +
>  out:
>  	mutex_unlock(&dev_data->mutex);
>  
> @@ -2127,7 +2146,6 @@ static void detach_device(struct device *dev)
>  	struct iommu_dev_data *dev_data = dev_iommu_priv_get(dev);
>  	struct amd_iommu *iommu = get_amd_iommu_from_dev_data(dev_data);
>  	struct protection_domain *domain = dev_data->domain;
> -	bool ppr = dev_data->ppr;
>  
>  	mutex_lock(&dev_data->mutex);
>  
> @@ -2140,13 +2158,15 @@ static void detach_device(struct device *dev)
>  	if (WARN_ON(!dev_data->domain))
>  		goto out;
>  
> -	if (ppr) {
> +	/* Remove IOPF handler */
> +	if (dev_data->ppr) {
>  		iopf_queue_flush_dev(dev);
> -
> -		/* Updated here so that it gets reflected in DTE */
> -		dev_data->ppr = false;
> +		amd_iommu_iopf_remove_device(iommu, dev_data);
>  	}
>  
> +	if (dev_is_pci(dev))
> +		pdev_disable_caps(to_pci_dev(dev));
> +
>  	/* Clear DTE and flush the entry */
>  	dev_update_dte(dev_data, false);
>  
> @@ -2166,14 +2186,6 @@ static void detach_device(struct device *dev)
>  
>  out:
>  	mutex_unlock(&dev_data->mutex);
> -
> -	/* Remove IOPF handler */
> -	if (ppr)
> -		amd_iommu_iopf_remove_device(iommu, dev_data);
> -
> -	if (dev_is_pci(dev))
> -		pdev_disable_caps(to_pci_dev(dev));
> -
>  }
>  
>  static struct iommu_device *amd_iommu_probe_device(struct device *dev)
> @@ -2450,7 +2462,6 @@ static int amd_iommu_attach_device(struct iommu_domain *dom,
>  	struct iommu_dev_data *dev_data = dev_iommu_priv_get(dev);
>  	struct protection_domain *domain = to_pdomain(dom);
>  	struct amd_iommu *iommu = get_amd_iommu_from_dev(dev);
> -	struct pci_dev *pdev;
>  	int ret;
>  
>  	/*
> @@ -2483,24 +2494,6 @@ static int amd_iommu_attach_device(struct iommu_domain *dom,
>  	}
>  #endif
>  
> -	pdev = dev_is_pci(dev_data->dev) ? to_pci_dev(dev_data->dev) : NULL;
> -	if (pdev && pdom_is_sva_capable(domain)) {
> -		pdev_enable_caps(pdev);
> -
> -		/*
> -		 * Device can continue to function even if IOPF
> -		 * enablement failed. Hence in error path just
> -		 * disable device PRI support.
> -		 */
> -		if (amd_iommu_iopf_add_device(iommu, dev_data))
> -			pdev_disable_cap_pri(pdev);
> -	} else if (pdev) {
> -		pdev_enable_cap_ats(pdev);
> -	}
> -
> -	/* Update device table */
> -	dev_update_dte(dev_data, true);
> -
>  	return ret;
>  }
>  
> -- 
> 2.31.1
>