linux-kernel - Re: [PATCH] interconnect: qcom: Fix DT backwards compatibility for QoS

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] [day] [month] [year] [list]

Message-ID: <c15fc1de-c930-44b6-a9f9-d17e4da002fe@linaro.org>
Date: Sat, 6 Jul 2024 14:31:42 +0200
From: Konrad Dybcio <konrad.dybcio@...aro.org>
To: Bjorn Andersson <andersson@...nel.org>,
 Odelu Kukatla <quic_okukatla@...cinc.com>
Cc: Georgi Djakov <djakov@...nel.org>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
 Conor Dooley <conor+dt@...nel.org>, Kees Cook <keescook@...omium.org>,
 cros-qcom-dts-watchers@...omium.org,
 "Gustavo A . R . Silva" <gustavoars@...nel.org>,
 linux-arm-msm@...r.kernel.org, linux-pm@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-hardening@...r.kernel.org, quic_rlaggysh@...cinc.com,
 quic_mdtipton@...cinc.com
Subject: Re: [PATCH] interconnect: qcom: Fix DT backwards compatibility for
 QoS

On 4.07.2024 7:44 PM, Bjorn Andersson wrote:
> On Thu, Jul 04, 2024 at 06:25:15PM GMT, Odelu Kukatla wrote:
>> Add qos_clks_required flag to skip QoS configuration if clocks property
>> is not populated in devicetree for providers which require clocks to be
>> enabled for accessing registers. This is to keep the QoS configuration
>> backwards compatible with devices that have older DTB.
>>
> 
> Please read "Describe your changes" [1], and make your commit message
> start with the problem description - establish to the reader why this
> change is needed, then follow that with a technical description of the
> solution (likely in a separate paragraph).
> 
> [1] https://docs.kernel.org/process/submitting-patches.html#describe-your-changes
> 
>> Reported-by: Bjorn Andersson <andersson@...nel.org>
>> Closes: https://lore.kernel.org/all/ciji6nlxn752ina4tmh6kwvek52nxpnguomqek6plwvwgvoqef@yrtexkpmn5br/
>> Signed-off-by: Odelu Kukatla <quic_okukatla@...cinc.com>
>> ---
>>  drivers/interconnect/qcom/icc-rpmh.c | 2 +-
>>  drivers/interconnect/qcom/icc-rpmh.h | 1 +
>>  drivers/interconnect/qcom/sc7280.c   | 2 ++
>>  3 files changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/interconnect/qcom/icc-rpmh.c b/drivers/interconnect/qcom/icc-rpmh.c
>> index 93047defd5e2..f49a8e0cb03c 100644
>> --- a/drivers/interconnect/qcom/icc-rpmh.c
>> +++ b/drivers/interconnect/qcom/icc-rpmh.c
>> @@ -311,7 +311,7 @@ int qcom_icc_rpmh_probe(struct platform_device *pdev)
>>  		}
>>  
>>  		qp->num_clks = devm_clk_bulk_get_all(qp->dev, &qp->clks);
>> -		if (qp->num_clks < 0) {
>> +		if (qp->num_clks < 0 || (!qp->num_clks && desc->qos_clks_required)) {
> 
> For this new case, I think the dev_info() below makes total sense. I.e.
> this looks good to me.
> 
> 
> However, the num_clks < 0 case would represent finding a devicetree node
> with clocks specified, but failing to get these clocks. I believe that
> this would include EPROBE_DEFER.
> 
> I don't think it's correct to print a informational message and continue
> without QoS. I think we should fail here.

Since setting QoS settings is optional, I'd say we should simply skip trying
to do so. Unless setting them on some buses (i.e. ones without failing clocks)
and not on the rest would cause issues. But then, these settings should be
bus-local, so perhaps it would still be fine?

Konrad