linux-kernel - Re: [PATCH 2/2] bus: qcom-ssc-block-bus: Fix the error handling path of qcom_ssc_block_bus

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <164b21fb-2ba9-42a4-9964-5e4f051df37a@seznam.cz>
Date: Sun, 2 Mar 2025 18:46:37 +0100
From: Michael Srba <Michael.Srba@...nam.cz>
To: Christophe JAILLET <christophe.jaillet@...adoo.fr>, andersson@...nel.org,
 konradybcio@...nel.org, jeffrey.l.hugo@...il.com
Cc: linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org,
 kernel-janitors@...r.kernel.org
Subject: Re: [PATCH 2/2] bus: qcom-ssc-block-bus: Fix the error handling path
 of qcom_ssc_block_bus_probe()

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@...adoo.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)