[PATCH 0/2] bus: qcom-ssc-block-bus: Fix some error handling paths

[PATCH 0/2] bus: qcom-ssc-block-bus: Fix some error handling paths

[Christophe JAILLET]

Patch 1 should be straight forward. It removes some ioumap() from
resources allocated with devm_ioremap_resource().

Patch 2 is more speculative. It releases some resources in the error
handling path of the probe, as done in the .remove() function.


Finally, the .remove() function also has some 
	pm_runtime_disable(&pdev->dev);
	pm_clk_destroy(&pdev->dev);
calls.

Usually pm_runtime_disable() is paired with pm_runtime_enable() and I
would expect some pm_clk_add(() calls related to pm_clk_destroy().
Neither exists.

So I wonder if these 2 calls are correct in the .remove() function.
Thought?

Christophe JAILLET (2):
  bus: qcom-ssc-block-bus: Remove some duplicated iounmap() calls
  bus: qcom-ssc-block-bus: Fix the error handling path of
    qcom_ssc_block_bus_probe()

 drivers/bus/qcom-ssc-block-bus.c | 34 ++++++++++++++++++--------------
 1 file changed, 19 insertions(+), 15 deletions(-)

-- 
2.48.1

[PATCH 1/2] bus: qcom-ssc-block-bus: Remove some duplicated iounmap() calls

[Christophe JAILLET]

reg_mpm_sscaon_config[01] are allocated with devm_ioremap_resource(). So,
they will be unmapped automatically by the manage resource framework.

Remove the incorrect explicit iounmap() calls from the remove function.

Fixes: 97d485edc1d9 ("bus: add driver for initializing the SSC bus on (some) qcom SoCs")
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
 drivers/bus/qcom-ssc-block-bus.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/bus/qcom-ssc-block-bus.c b/drivers/bus/qcom-ssc-block-bus.c
index 85d781a32df4..c95a985e3498 100644
--- a/drivers/bus/qcom-ssc-block-bus.c
+++ b/drivers/bus/qcom-ssc-block-bus.c
@@ -356,9 +356,6 @@ static void qcom_ssc_block_bus_remove(struct platform_device *pdev)
 
 	qcom_ssc_block_bus_deinit(&pdev->dev);
 
-	iounmap(data->reg_mpm_sscaon_config0);
-	iounmap(data->reg_mpm_sscaon_config1);
-
 	qcom_ssc_block_bus_pds_disable(data->pds, data->num_pds);
 	qcom_ssc_block_bus_pds_detach(&pdev->dev, data->pds, data->num_pds);
 	pm_runtime_disable(&pdev->dev);
-- 
2.48.1

Re: [PATCH 1/2] bus: qcom-ssc-block-bus: Remove some duplicated iounmap() calls

[Dmitry Baryshkov]

On Sun, Mar 02, 2025 at 05:21:34PM +0100, Christophe JAILLET wrote:
> reg_mpm_sscaon_config[01] are allocated with devm_ioremap_resource(). So,
> they will be unmapped automatically by the manage resource framework.
> 
> Remove the incorrect explicit iounmap() calls from the remove function.
> 
> Fixes: 97d485edc1d9 ("bus: add driver for initializing the SSC bus on (some) qcom SoCs")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
>  drivers/bus/qcom-ssc-block-bus.c | 3 ---
>  1 file changed, 3 deletions(-)
> 

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

-- 
With best wishes
Dmitry

[PATCH 2/2] bus: qcom-ssc-block-bus: Fix the error handling path of qcom_ssc_block_bus_probe()

[Christophe JAILLET]

If qcom_ssc_block_bus_pds_enable() fails, the previous call to
qcom_ssc_block_bus_pds_attach() must be undone, as already done in the
remove function.

In order to do that, move the code related to the power domains management
to the end of the function, in order to avoid many changes in all the error
handling path that would need to go through the new error handling path.

Fixes: 97d485edc1d9 ("bus: add driver for initializing the SSC bus on (some) qcom SoCs")
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
This patch is compile tested only.

It is also speculative. Power management interaction can be sometimes
tricky and I'm not 100% sure that moving this code in fine.

Review with care.
---
 drivers/bus/qcom-ssc-block-bus.c | 31 +++++++++++++++++++------------
 1 file changed, 19 insertions(+), 12 deletions(-)

diff --git a/drivers/bus/qcom-ssc-block-bus.c b/drivers/bus/qcom-ssc-block-bus.c
index c95a985e3498..7f5fd4e0940d 100644
--- a/drivers/bus/qcom-ssc-block-bus.c
+++ b/drivers/bus/qcom-ssc-block-bus.c
@@ -264,18 +264,6 @@ static int qcom_ssc_block_bus_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, data);
 
-	data->pd_names = qcom_ssc_block_pd_names;
-	data->num_pds = ARRAY_SIZE(qcom_ssc_block_pd_names);
-
-	/* power domains */
-	ret = qcom_ssc_block_bus_pds_attach(&pdev->dev, data->pds, data->pd_names, data->num_pds);
-	if (ret < 0)
-		return dev_err_probe(&pdev->dev, ret, "error when attaching power domains\n");
-
-	ret = qcom_ssc_block_bus_pds_enable(data->pds, data->num_pds);
-	if (ret < 0)
-		return dev_err_probe(&pdev->dev, ret, "error when enabling power domains\n");
-
 	/* low level overrides for when the HW logic doesn't "just work" */
 	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "mpm_sscaon_config0");
 	data->reg_mpm_sscaon_config0 = devm_ioremap_resource(&pdev->dev, res);
@@ -343,11 +331,30 @@ static int qcom_ssc_block_bus_probe(struct platform_device *pdev)
 
 	data->ssc_axi_halt = halt_args.args[0];
 
+	/* power domains */
+	data->pd_names = qcom_ssc_block_pd_names;
+	data->num_pds = ARRAY_SIZE(qcom_ssc_block_pd_names);
+
+	ret = qcom_ssc_block_bus_pds_attach(&pdev->dev, data->pds, data->pd_names, data->num_pds);
+	if (ret < 0)
+		return dev_err_probe(&pdev->dev, ret, "error when attaching power domains\n");
+
+	ret = qcom_ssc_block_bus_pds_enable(data->pds, data->num_pds);
+	if (ret < 0) {
+		dev_err_probe(&pdev->dev, ret, "error when enabling power domains\n");
+		goto err_detach_pds_bus;
+	}
+
 	qcom_ssc_block_bus_init(&pdev->dev);
 
 	of_platform_populate(np, NULL, NULL, &pdev->dev);
 
 	return 0;
+
+err_detach_pds_bus:
+	qcom_ssc_block_bus_pds_detach(&pdev->dev, data->pds, data->num_pds);
+
+	return ret;
 }
 
 static void qcom_ssc_block_bus_remove(struct platform_device *pdev)
-- 
2.48.1

Re: [PATCH 2/2] bus: qcom-ssc-block-bus: Fix the error handling path of qcom_ssc_block_bus_probe()

[Michael Srba]

iirc it took me quite a long time to figure out the correct sequence for the bus to come up, so I'd be careful with that indeed. Sadly I can't easily test this on the original device right now, when I have time I want to upstream support for sdm845 which I could test more easily, but the sdm845 case is simpler so idk if testing on that would be sufficient.

On 02. 03. 25 17:21, Christophe JAILLET wrote:
> If qcom_ssc_block_bus_pds_enable() fails, the previous call to
> qcom_ssc_block_bus_pds_attach() must be undone, as already done in the
> remove function.
>
> In order to do that, move the code related to the power domains management
> to the end of the function, in order to avoid many changes in all the error
> handling path that would need to go through the new error handling path.
>
> Fixes: 97d485edc1d9 ("bus: add driver for initializing the SSC bus on (some) qcom SoCs")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
> This patch is compile tested only.
>
> It is also speculative. Power management interaction can be sometimes
> tricky and I'm not 100% sure that moving this code in fine.
>
> Review with care.
> ---
>   drivers/bus/qcom-ssc-block-bus.c | 31 +++++++++++++++++++------------
>   1 file changed, 19 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/bus/qcom-ssc-block-bus.c b/drivers/bus/qcom-ssc-block-bus.c
> index c95a985e3498..7f5fd4e0940d 100644
> --- a/drivers/bus/qcom-ssc-block-bus.c
> +++ b/drivers/bus/qcom-ssc-block-bus.c
> @@ -264,18 +264,6 @@ static int qcom_ssc_block_bus_probe(struct platform_device *pdev)
>   
>   	platform_set_drvdata(pdev, data);
>   
> -	data->pd_names = qcom_ssc_block_pd_names;
> -	data->num_pds = ARRAY_SIZE(qcom_ssc_block_pd_names);
> -
> -	/* power domains */
> -	ret = qcom_ssc_block_bus_pds_attach(&pdev->dev, data->pds, data->pd_names, data->num_pds);
> -	if (ret < 0)
> -		return dev_err_probe(&pdev->dev, ret, "error when attaching power domains\n");
> -
> -	ret = qcom_ssc_block_bus_pds_enable(data->pds, data->num_pds);
> -	if (ret < 0)
> -		return dev_err_probe(&pdev->dev, ret, "error when enabling power domains\n");
> -
>   	/* low level overrides for when the HW logic doesn't "just work" */
>   	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "mpm_sscaon_config0");
>   	data->reg_mpm_sscaon_config0 = devm_ioremap_resource(&pdev->dev, res);
> @@ -343,11 +331,30 @@ static int qcom_ssc_block_bus_probe(struct platform_device *pdev)
>   
>   	data->ssc_axi_halt = halt_args.args[0];
>   
> +	/* power domains */
> +	data->pd_names = qcom_ssc_block_pd_names;
> +	data->num_pds = ARRAY_SIZE(qcom_ssc_block_pd_names);
> +
> +	ret = qcom_ssc_block_bus_pds_attach(&pdev->dev, data->pds, data->pd_names, data->num_pds);
> +	if (ret < 0)
> +		return dev_err_probe(&pdev->dev, ret, "error when attaching power domains\n");
> +
> +	ret = qcom_ssc_block_bus_pds_enable(data->pds, data->num_pds);
> +	if (ret < 0) {
> +		dev_err_probe(&pdev->dev, ret, "error when enabling power domains\n");
> +		goto err_detach_pds_bus;
> +	}
> +
>   	qcom_ssc_block_bus_init(&pdev->dev);
>   
>   	of_platform_populate(np, NULL, NULL, &pdev->dev);
>   
>   	return 0;
> +
> +err_detach_pds_bus:
> +	qcom_ssc_block_bus_pds_detach(&pdev->dev, data->pds, data->num_pds);
> +
> +	return ret;
>   }
>   
>   static void qcom_ssc_block_bus_remove(struct platform_device *pdev)

Re: [PATCH 0/2] bus: qcom-ssc-block-bus: Fix some error handling paths

[Bjorn Andersson]

On Sun, 02 Mar 2025 17:21:33 +0100, Christophe JAILLET wrote:
> Patch 1 should be straight forward. It removes some ioumap() from
> resources allocated with devm_ioremap_resource().
> 
> Patch 2 is more speculative. It releases some resources in the error
> handling path of the probe, as done in the .remove() function.
> 
> 
> [...]

Applied, thanks!

[1/2] bus: qcom-ssc-block-bus: Remove some duplicated iounmap() calls
      commit: a9ac4ba7dcace2b3b91e7b87bf0ba97c47edd94f
[2/2] bus: qcom-ssc-block-bus: Fix the error handling path of qcom_ssc_block_bus_probe()
      commit: f41658cd081ad7697796b3dacd9a717a57919268

Best regards,
-- 
Bjorn Andersson <andersson@kernel.org>