[PATCH] firmware: qcom_scm: disable clocks if qcom_scm_bw_enable() fails

[PATCH] firmware: qcom_scm: disable clocks if qcom_scm_bw_enable() fails

[Gabor Juhos]

There are several functions which are calling qcom_scm_bw_enable()
then returns immediately if the call fails and leaves the clocks
enabled.

Change the code of these functions to disable clocks when the
qcom_scm_bw_enable() call fails. This also fixes a possible dma
buffer leak in the qcom_scm_pas_init_image() function.

Compile tested only due to lack of hardware with interconnect
support.

Cc: stable@vger.kernel.org
Fixes: 65b7ebda5028 ("firmware: qcom_scm: Add bw voting support to the SCM interface")
Signed-off-by: Gabor Juhos <j4g8y7@gmail.com>
---
Based on v6.8-rc7.

Note: Removing the two empty lines from qcom_scm_pas_init_image()
and fomr qcom_scm_pas_shutdown() functions is intentional to make
those consistent with the other two functions.
---
 drivers/firmware/qcom/qcom_scm.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
index 520de9b5633ab..e8460626fb0c4 100644
--- a/drivers/firmware/qcom/qcom_scm.c
+++ b/drivers/firmware/qcom/qcom_scm.c
@@ -569,13 +569,14 @@ int qcom_scm_pas_init_image(u32 peripheral, const void *metadata, size_t size,
 
 	ret = qcom_scm_bw_enable();
 	if (ret)
-		return ret;
+		goto disable_clk;
 
 	desc.args[1] = mdata_phys;
 
 	ret = qcom_scm_call(__scm->dev, &desc, &res);
-
 	qcom_scm_bw_disable();
+
+disable_clk:
 	qcom_scm_clk_disable();
 
 out:
@@ -637,10 +638,12 @@ int qcom_scm_pas_mem_setup(u32 peripheral, phys_addr_t addr, phys_addr_t size)
 
 	ret = qcom_scm_bw_enable();
 	if (ret)
-		return ret;
+		goto disable_clk;
 
 	ret = qcom_scm_call(__scm->dev, &desc, &res);
 	qcom_scm_bw_disable();
+
+disable_clk:
 	qcom_scm_clk_disable();
 
 	return ret ? : res.result[0];
@@ -672,10 +675,12 @@ int qcom_scm_pas_auth_and_reset(u32 peripheral)
 
 	ret = qcom_scm_bw_enable();
 	if (ret)
-		return ret;
+		goto disable_clk;
 
 	ret = qcom_scm_call(__scm->dev, &desc, &res);
 	qcom_scm_bw_disable();
+
+disable_clk:
 	qcom_scm_clk_disable();
 
 	return ret ? : res.result[0];
@@ -706,11 +711,12 @@ int qcom_scm_pas_shutdown(u32 peripheral)
 
 	ret = qcom_scm_bw_enable();
 	if (ret)
-		return ret;
+		goto disable_clk;
 
 	ret = qcom_scm_call(__scm->dev, &desc, &res);
-
 	qcom_scm_bw_disable();
+
+disable_clk:
 	qcom_scm_clk_disable();
 
 	return ret ? : res.result[0];

---
base-commit: 90d35da658da8cff0d4ecbb5113f5fac9d00eb72
change-id: 20240304-qcom-scm-disable-clk-08e7ad853fa1

Best regards,
-- 
Gabor Juhos <j4g8y7@gmail.com>

Re: [PATCH] firmware: qcom_scm: disable clocks if qcom_scm_bw_enable() fails

[Mukesh Ojha]

On 3/4/2024 6:44 PM, Gabor Juhos wrote:
> There are several functions which are calling qcom_scm_bw_enable()
> then returns immediately if the call fails and leaves the clocks
> enabled.
> 
> Change the code of these functions to disable clocks when the
> qcom_scm_bw_enable() call fails. This also fixes a possible dma
> buffer leak in the qcom_scm_pas_init_image() function.
> 
> Compile tested only due to lack of hardware with interconnect
> support.
> 
> Cc: stable@vger.kernel.org
> Fixes: 65b7ebda5028 ("firmware: qcom_scm: Add bw voting support to the SCM interface")
> Signed-off-by: Gabor Juhos <j4g8y7@gmail.com>
> ---
> Based on v6.8-rc7.
> 
> Note: Removing the two empty lines from qcom_scm_pas_init_image()
> and fomr qcom_scm_pas_shutdown() functions is intentional to make
> those consistent with the other two functions.

LGTM..

Reviewed-by: Mukesh Ojha <quic_mojha@quicinc.com>

-Mukesh

Re: [PATCH] firmware: qcom_scm: disable clocks if qcom_scm_bw_enable() fails

[Konrad Dybcio]

On 3/4/24 14:14, Gabor Juhos wrote:
> There are several functions which are calling qcom_scm_bw_enable()
> then returns immediately if the call fails and leaves the clocks
> enabled.
> 
> Change the code of these functions to disable clocks when the
> qcom_scm_bw_enable() call fails. This also fixes a possible dma
> buffer leak in the qcom_scm_pas_init_image() function.
> 
> Compile tested only due to lack of hardware with interconnect
> support.
> 
> Cc: stable@vger.kernel.org
> Fixes: 65b7ebda5028 ("firmware: qcom_scm: Add bw voting support to the SCM interface")
> Signed-off-by: Gabor Juhos <j4g8y7@gmail.com>
> ---

Taking a closer look, is there any argument against simply
putting the clk/bw en/dis calls in qcom_scm_call()?

Konrad

Re: Re: [PATCH] firmware: qcom_scm: disable clocks if qcom_scm_bw_enable() fails

[Elliot Berman]

On Tue, Mar 05, 2024 at 10:15:19PM +0100, Konrad Dybcio wrote:
> 
> 
> On 3/4/24 14:14, Gabor Juhos wrote:
> > There are several functions which are calling qcom_scm_bw_enable()
> > then returns immediately if the call fails and leaves the clocks
> > enabled.
> > 
> > Change the code of these functions to disable clocks when the
> > qcom_scm_bw_enable() call fails. This also fixes a possible dma
> > buffer leak in the qcom_scm_pas_init_image() function.
> > 
> > Compile tested only due to lack of hardware with interconnect
> > support.
> > 
> > Cc: stable@vger.kernel.org
> > Fixes: 65b7ebda5028 ("firmware: qcom_scm: Add bw voting support to the SCM interface")
> > Signed-off-by: Gabor Juhos <j4g8y7@gmail.com>
> > ---
> 
> Taking a closer look, is there any argument against simply
> putting the clk/bw en/dis calls in qcom_scm_call()?

We shouldn't do this because the clk/bw en/dis calls are only needed in
few SCM calls.

Thanks,
Elliot

Re: [PATCH] firmware: qcom_scm: disable clocks if qcom_scm_bw_enable() fails

[Konrad Dybcio]

On 3/6/24 05:10, Elliot Berman wrote:
> On Tue, Mar 05, 2024 at 10:15:19PM +0100, Konrad Dybcio wrote:
>>
>>
>> On 3/4/24 14:14, Gabor Juhos wrote:
>>> There are several functions which are calling qcom_scm_bw_enable()
>>> then returns immediately if the call fails and leaves the clocks
>>> enabled.
>>>
>>> Change the code of these functions to disable clocks when the
>>> qcom_scm_bw_enable() call fails. This also fixes a possible dma
>>> buffer leak in the qcom_scm_pas_init_image() function.
>>>
>>> Compile tested only due to lack of hardware with interconnect
>>> support.
>>>
>>> Cc: stable@vger.kernel.org
>>> Fixes: 65b7ebda5028 ("firmware: qcom_scm: Add bw voting support to the SCM interface")
>>> Signed-off-by: Gabor Juhos <j4g8y7@gmail.com>
>>> ---
>>
>> Taking a closer look, is there any argument against simply
>> putting the clk/bw en/dis calls in qcom_scm_call()?
> 
> We shouldn't do this because the clk/bw en/dis calls are only needed in
> few SCM calls.

Then the argument list could be expanded with `bool require_resources`,
or so still saving us a lot of boilerplate

Konrad

Re: Re: [PATCH] firmware: qcom_scm: disable clocks if qcom_scm_bw_enable() fails

[Elliot Berman]

On Wed, Mar 06, 2024 at 05:02:37PM +0100, Konrad Dybcio wrote:
> On 3/6/24 05:10, Elliot Berman wrote:
> > On Tue, Mar 05, 2024 at 10:15:19PM +0100, Konrad Dybcio wrote:
> > > On 3/4/24 14:14, Gabor Juhos wrote:
> > > > There are several functions which are calling qcom_scm_bw_enable()
> > > > then returns immediately if the call fails and leaves the clocks
> > > > enabled.
> > > > 
> > > > Change the code of these functions to disable clocks when the
> > > > qcom_scm_bw_enable() call fails. This also fixes a possible dma
> > > > buffer leak in the qcom_scm_pas_init_image() function.
> > > > 
> > > > Compile tested only due to lack of hardware with interconnect
> > > > support.
> > > > 
> > > > Cc: stable@vger.kernel.org
> > > > Fixes: 65b7ebda5028 ("firmware: qcom_scm: Add bw voting support to the SCM interface")
> > > > Signed-off-by: Gabor Juhos <j4g8y7@gmail.com>
> > > > ---
> > > 
> > > Taking a closer look, is there any argument against simply
> > > putting the clk/bw en/dis calls in qcom_scm_call()?
> > 
> > We shouldn't do this because the clk/bw en/dis calls are only needed in
> > few SCM calls.
> 
> Then the argument list could be expanded with `bool require_resources`,
> or so still saving us a lot of boilerplate

If we want to go that route, I'd vote to add it to qcom_scm_desc in a
new field.

Re: [PATCH] firmware: qcom_scm: disable clocks if qcom_scm_bw_enable() fails

[Gabor Juhos]

2024. 03. 06. 17:02 keltezéssel, Konrad Dybcio írta:
> 
> 
> On 3/6/24 05:10, Elliot Berman wrote:
>> On Tue, Mar 05, 2024 at 10:15:19PM +0100, Konrad Dybcio wrote:
>>>
>>>
>>> On 3/4/24 14:14, Gabor Juhos wrote:
>>>> There are several functions which are calling qcom_scm_bw_enable()
>>>> then returns immediately if the call fails and leaves the clocks
>>>> enabled.
>>>>
>>>> Change the code of these functions to disable clocks when the
>>>> qcom_scm_bw_enable() call fails. This also fixes a possible dma
>>>> buffer leak in the qcom_scm_pas_init_image() function.
>>>>
>>>> Compile tested only due to lack of hardware with interconnect
>>>> support.
>>>>
>>>> Cc: stable@vger.kernel.org
>>>> Fixes: 65b7ebda5028 ("firmware: qcom_scm: Add bw voting support to the SCM
>>>> interface")
>>>> Signed-off-by: Gabor Juhos <j4g8y7@gmail.com>
>>>> ---
>>>
>>> Taking a closer look, is there any argument against simply
>>> putting the clk/bw en/dis calls in qcom_scm_call()?
>>
>> We shouldn't do this because the clk/bw en/dis calls are only needed in
>> few SCM calls.
> 
> Then the argument list could be expanded with `bool require_resources`,
> or so still saving us a lot of boilerplate

That would mean that we have to modify each callers of qcom_scm_call() to pass a
new parameter. Additionally, there are cases, when the bw enable part is not
needed so we should add separate parameters, one for clk and one for bw or we
should use a bitmask.

Would not it be simpler to use a helper function like the following instead?

static int qcom_scm_call_clk_bw(struct device *dev,
				const struct qcom_scm_desc *desc,
				struct qcom_scm_res *res)
{
	int ret;

	ret = qcom_scm_clk_enable();
	if (ret)
		return ret;

	ret = qcom_scm_bw_enable();
	if (ret)
		goto disable_clk;

	ret = qcom_scm_call(dev, desc, res);
	qcom_scm_bw_disable();

disable_clk:
	qcom_scm_clk_disable();

	return ret;
}

Re: [PATCH] firmware: qcom_scm: disable clocks if qcom_scm_bw_enable() fails

[Bjorn Andersson]

On Wed, Mar 06, 2024 at 05:02:37PM +0100, Konrad Dybcio wrote:
> 
> 
> On 3/6/24 05:10, Elliot Berman wrote:
> > On Tue, Mar 05, 2024 at 10:15:19PM +0100, Konrad Dybcio wrote:
> > > 
> > > 
> > > On 3/4/24 14:14, Gabor Juhos wrote:
> > > > There are several functions which are calling qcom_scm_bw_enable()
> > > > then returns immediately if the call fails and leaves the clocks
> > > > enabled.
> > > > 
> > > > Change the code of these functions to disable clocks when the
> > > > qcom_scm_bw_enable() call fails. This also fixes a possible dma
> > > > buffer leak in the qcom_scm_pas_init_image() function.
> > > > 
> > > > Compile tested only due to lack of hardware with interconnect
> > > > support.
> > > > 
> > > > Cc: stable@vger.kernel.org
> > > > Fixes: 65b7ebda5028 ("firmware: qcom_scm: Add bw voting support to the SCM interface")
> > > > Signed-off-by: Gabor Juhos <j4g8y7@gmail.com>
> > > > ---
> > > 
> > > Taking a closer look, is there any argument against simply
> > > putting the clk/bw en/dis calls in qcom_scm_call()?
> > 
> > We shouldn't do this because the clk/bw en/dis calls are only needed in
> > few SCM calls.
> 
> Then the argument list could be expanded with `bool require_resources`,
> or so still saving us a lot of boilerplate
> 

I don't think there's reason for making this more general, because I
think this is a problem specific to PAS - much related to Bartosz
special handling of shmbridge for these calls.

It would be very nice if someone could help document why this is.

Regards,
Bjorn

Re: [PATCH] firmware: qcom_scm: disable clocks if qcom_scm_bw_enable() fails

[Bjorn Andersson]

On Mon, 04 Mar 2024 14:14:53 +0100, Gabor Juhos wrote:
> There are several functions which are calling qcom_scm_bw_enable()
> then returns immediately if the call fails and leaves the clocks
> enabled.
> 
> Change the code of these functions to disable clocks when the
> qcom_scm_bw_enable() call fails. This also fixes a possible dma
> buffer leak in the qcom_scm_pas_init_image() function.
> 
> [...]

Applied, thanks!

[1/1] firmware: qcom_scm: disable clocks if qcom_scm_bw_enable() fails
      commit: 0c50b7fcf2773b4853e83fc15aba1a196ba95966

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