[PATCH] iommu: qcom: Revert "iommu/arm: Cleanup resources in case of probe error path"

[PATCH] iommu: qcom: Revert "iommu/arm: Cleanup resources in case of probe error path"

[Marek Szyprowski]

QCOM IOMMU driver calls bus_set_iommu() for every IOMMU device controller,
what fails for the second and latter IOMMU devices. This is intended and
must be not fatal to the driver registration process. Also the cleanup
path should take care of the runtime PM state, what is missing in the
current patch. Revert relevant changes to the QCOM IOMMU driver until
a proper fix is prepared.

This partially reverts commit 249c9dc6aa0db74a0f7908efd04acf774e19b155.

Fixes: 249c9dc6aa0d ("iommu/arm: Cleanup resources in case of probe error path")
Suggested-by: Will Deacon <will@kernel.org>
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/iommu/arm/arm-smmu/qcom_iommu.c | 13 ++-----------
 1 file changed, 2 insertions(+), 11 deletions(-)

diff --git a/drivers/iommu/arm/arm-smmu/qcom_iommu.c b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
index 25ed444ff94d..021cf8f65ffc 100644
--- a/drivers/iommu/arm/arm-smmu/qcom_iommu.c
+++ b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
@@ -849,12 +849,10 @@ static int qcom_iommu_device_probe(struct platform_device *pdev)
 	ret = iommu_device_register(&qcom_iommu->iommu, &qcom_iommu_ops, dev);
 	if (ret) {
 		dev_err(dev, "Failed to register iommu\n");
-		goto err_sysfs_remove;
+		return ret;
 	}
 
-	ret = bus_set_iommu(&platform_bus_type, &qcom_iommu_ops);
-	if (ret)
-		goto err_unregister_device;
+	bus_set_iommu(&platform_bus_type, &qcom_iommu_ops);
 
 	if (qcom_iommu->local_base) {
 		pm_runtime_get_sync(dev);
@@ -863,13 +861,6 @@ static int qcom_iommu_device_probe(struct platform_device *pdev)
 	}
 
 	return 0;
-
-err_unregister_device:
-	iommu_device_unregister(&qcom_iommu->iommu);
-
-err_sysfs_remove:
-	iommu_device_sysfs_remove(&qcom_iommu->iommu);
-	return ret;
 }
 
 static int qcom_iommu_device_remove(struct platform_device *pdev)
-- 
2.17.1

Re: [PATCH] iommu: qcom: Revert "iommu/arm: Cleanup resources in case of probe error path"

[Amey Narkhede]

On 21/07/05 08:56AM, Marek Szyprowski wrote:
> QCOM IOMMU driver calls bus_set_iommu() for every IOMMU device controller,
> what fails for the second and latter IOMMU devices. This is intended and
> must be not fatal to the driver registration process. Also the cleanup
> path should take care of the runtime PM state, what is missing in the
> current patch. Revert relevant changes to the QCOM IOMMU driver until
> a proper fix is prepared.
>
Apologies for the broken patch I don't have any arm machine to test the
patches. Is this bug unique to qcom iommu?
[...]

Thanks,
Amey

Re: [PATCH] iommu: qcom: Revert "iommu/arm: Cleanup resources in case of probe error path"

[Marek Szyprowski]

On 05.07.2021 16:30, Amey Narkhede wrote:
> On 21/07/05 08:56AM, Marek Szyprowski wrote:
>> QCOM IOMMU driver calls bus_set_iommu() for every IOMMU device controller,
>> what fails for the second and latter IOMMU devices. This is intended and
>> must be not fatal to the driver registration process. Also the cleanup
>> path should take care of the runtime PM state, what is missing in the
>> current patch. Revert relevant changes to the QCOM IOMMU driver until
>> a proper fix is prepared.
>>
> Apologies for the broken patch I don't have any arm machine to test the
> patches. Is this bug unique to qcom iommu?

Frankly, I have no idea. Just grep for bus_set_iommu() and check if the 
caller might be executed more than once.

Best regards

-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH] iommu: qcom: Revert "iommu/arm: Cleanup resources in case of probe error path"

[Will Deacon]

On Mon, Jul 05, 2021 at 08:56:57AM +0200, Marek Szyprowski wrote:
> QCOM IOMMU driver calls bus_set_iommu() for every IOMMU device controller,
> what fails for the second and latter IOMMU devices. This is intended and
> must be not fatal to the driver registration process. Also the cleanup
> path should take care of the runtime PM state, what is missing in the
> current patch. Revert relevant changes to the QCOM IOMMU driver until
> a proper fix is prepared.
> 
> This partially reverts commit 249c9dc6aa0db74a0f7908efd04acf774e19b155.
> 
> Fixes: 249c9dc6aa0d ("iommu/arm: Cleanup resources in case of probe error path")
> Suggested-by: Will Deacon <will@kernel.org>
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  drivers/iommu/arm/arm-smmu/qcom_iommu.c | 13 ++-----------
>  1 file changed, 2 insertions(+), 11 deletions(-)

Thanks, Marek:

Acked-by: Will Deacon <will@kernel.org>

Joerg -- please can you pick this up as a fix?

Cheers,

Will

> diff --git a/drivers/iommu/arm/arm-smmu/qcom_iommu.c b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
> index 25ed444ff94d..021cf8f65ffc 100644
> --- a/drivers/iommu/arm/arm-smmu/qcom_iommu.c
> +++ b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
> @@ -849,12 +849,10 @@ static int qcom_iommu_device_probe(struct platform_device *pdev)
>  	ret = iommu_device_register(&qcom_iommu->iommu, &qcom_iommu_ops, dev);
>  	if (ret) {
>  		dev_err(dev, "Failed to register iommu\n");
> -		goto err_sysfs_remove;
> +		return ret;
>  	}
>  
> -	ret = bus_set_iommu(&platform_bus_type, &qcom_iommu_ops);
> -	if (ret)
> -		goto err_unregister_device;
> +	bus_set_iommu(&platform_bus_type, &qcom_iommu_ops);
>  
>  	if (qcom_iommu->local_base) {
>  		pm_runtime_get_sync(dev);
> @@ -863,13 +861,6 @@ static int qcom_iommu_device_probe(struct platform_device *pdev)
>  	}
>  
>  	return 0;
> -
> -err_unregister_device:
> -	iommu_device_unregister(&qcom_iommu->iommu);
> -
> -err_sysfs_remove:
> -	iommu_device_sysfs_remove(&qcom_iommu->iommu);
> -	return ret;
>  }
>  
>  static int qcom_iommu_device_remove(struct platform_device *pdev)
> -- 
> 2.17.1
> 

Re: [PATCH] iommu: qcom: Revert "iommu/arm: Cleanup resources in case of probe error path"

[Joerg Roedel]

On Mon, Jul 05, 2021 at 08:56:57AM +0200, Marek Szyprowski wrote:
> QCOM IOMMU driver calls bus_set_iommu() for every IOMMU device controller,
> what fails for the second and latter IOMMU devices. This is intended and
> must be not fatal to the driver registration process. Also the cleanup
> path should take care of the runtime PM state, what is missing in the
> current patch. Revert relevant changes to the QCOM IOMMU driver until
> a proper fix is prepared.
> 
> This partially reverts commit 249c9dc6aa0db74a0f7908efd04acf774e19b155.
> 
> Fixes: 249c9dc6aa0d ("iommu/arm: Cleanup resources in case of probe error path")
> Suggested-by: Will Deacon <will@kernel.org>
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  drivers/iommu/arm/arm-smmu/qcom_iommu.c | 13 ++-----------
>  1 file changed, 2 insertions(+), 11 deletions(-)

Applied to iommu/fixes, thanks.