Received-SPF: pass (google.com: domain of linux-kernel+bounces-94371-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Message-ID: <f28633b9-cad8-4e1f-807c-830ba9368eb5@gmail.com>
Date: Wed, 6 Mar 2024 19:03:06 +0100
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] firmware: qcom_scm: disable clocks if
 qcom_scm_bw_enable() fails
Content-Language: hu
To: Konrad Dybcio <konrad.dybcio@linaro.org>,
 Bjorn Andersson <andersson@kernel.org>, Sibi Sankar
 <quic_sibis@quicinc.com>, linux-arm-msm@vger.kernel.org,
 linux-kernel@vger.kernel.org, stable@vger.kernel.org
References: <20240304-qcom-scm-disable-clk-v1-1-b36e51577ca1@gmail.com>
 <d655a4db-89a8-4b03-86b1-55258d37aa19@linaro.org>
 <20240305200306921-0800.eberman@hu-eberman-lv.qualcomm.com>
 <2fdb87f5-3702-44d9-9ebe-974c4a53a77d@linaro.org>
From: Gabor Juhos <j4g8y7@gmail.com>
In-Reply-To: <2fdb87f5-3702-44d9-9ebe-974c4a53a77d@linaro.org>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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;
}