linux-kernel - Re: [PATCH 2/3] soc: qcom: icc-bwmon: Handle global registers correctly

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: <3fce171e-3503-3ed9-91a6-7718fa1c12e6@linaro.org>
Date:   Mon, 6 Mar 2023 13:36:48 +0100
From:   Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To:     Konrad Dybcio <konrad.dybcio@...aro.org>,
        Andy Gross <agross@...nel.org>,
        Bjorn Andersson <andersson@...nel.org>,
        Georgi Djakov <djakov@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Thara Gopinath <thara.gopinath@...il.com>
Cc:     linux-arm-msm@...r.kernel.org, linux-pm@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/3] soc: qcom: icc-bwmon: Handle global registers
 correctly

On 06/03/2023 13:17, Konrad Dybcio wrote:
>> That's still nothing to fix - msm8998 was added here only for
>> compatibility reasons for bindings. It wasn't ever tested on MSM8998 and
>> also probably never written in a way that it should work for MSM8998.
> Don't you think making something like
> 
> compatible = "qcom,msm8998-bwmon"
> 
> not actually compatible with MSM8998 is a bit wrong?

I would argue that's the binding problem, but I get your point. Driver
just incorrectly stated that it could work with msm8998.

> 
>  It
>> could work, but that was not the intention. The driver is supposed to
>> work on sdm845 and according to your description - there is nothing
>> wrong with that.
>>
>>>

(...)

>>>
>>>>
>>>>> +	if (ret)
>>>>> +		return ret;
>>>>> +
>>>>> +	if (bwmon->data->quirks & BWMON_HAS_GLOBAL_IRQ) {
>>>>> +		/* Map the global base, if separate */
>>>>> +		base = devm_platform_ioremap_resource(pdev, 1);
>>>>
>>>> Wouldn't this now print errors for sdm845, thus introduce dmesg regression?
>>> I explicitly stated this in the cover letter and asked for opinions.
>>>
>>
>> Sorry, long time ago I stopped reading cover letters, maybe except it's
>> top few sentences. Just too many of them and too much of text usually
>> useless. Commits should describe everything as they go to the Git and
>> they should justify their own existence.
>>
>> Anyway, above dmesg error regression is a no. The devices turn out not
>> to be compatible with each other so just adjust the bindings and match
>> each driver by proper compatible string.
> So what am I supposed to do here? Add "qcom,msm8998-actual-msm8998-bwmon"?

No, make msm8998 binding working for msm8998 and add entry for sdm845.


> Otherwise, changing msm8998 data would break 845-8550, unless I added all
> of them to a separate match table.

Exactly.

>  Or I can add some boilerplate C code
> that would not throw a warning, or perhaps try and introduce
> devm_platform_ioremap_resource_optional..
> 
> I guess the last option sounds the best.

Best regards,
Krzysztof