linux-kernel - Re: [PATCH] soc: mediatek: cmdq: Don't log an error when gce-client-reg is not found

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]

Date: Wed, 28 Feb 2024 15:48:32 +0100
From: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
To: Nícolas F. R. A. Prado <nfraprado@...labora.com>
Cc: Matthias Brugger <matthias.bgg@...il.com>, kernel@...labora.com,
 linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
 linux-mediatek@...ts.infradead.org
Subject: Re: [PATCH] soc: mediatek: cmdq: Don't log an error when
 gce-client-reg is not found

Il 28/02/24 15:44, Nícolas F. R. A. Prado ha scritto:
> On Wed, Feb 28, 2024 at 10:41:15AM +0100, AngeloGioacchino Del Regno wrote:
>> Il 26/02/24 22:31, Nícolas F. R. A. Prado ha scritto:
>>> Most of the callers to this function do not require CMDQ support, it is
>>> optional, so the missing property shouldn't cause an error message.
>>> Furthermore, the callers that do require CMDQ support already log at the
>>> error level when an error is returned.
>>>
>>> Change the log message in this helper to be printed at the debug level
>>> instead.
>>
>> CMDQ is optional, yes. At least, for some devices it is.
>>
>> Full story, though, wants that if you use the CPU for register manipulation
>> instead of programming the GCE (even with threading, fantastic!) you will
>> trigger various performance issues.
>>
>> In the end, you *don't want* to use the CPU if GCE is available!
>>
>> The reasons why the CMDQ/GCE is optional are:
>>   - You can check register-by-register r/w for debugging scenarios by using
>>     the CPU to manipulate them instead of having something magically doing
>>     that for you at a certain (pre-set, yes, but still!) point;
>>   - Not all SoCs have the same amount of GCE threads and channels, some may
>>     support writing to IP block X through the GCE, some may not, but both
>>     may support writing for IP block Y through this mailbox;
>>   - MediaTek chose to support both ways, enabling means to debug stuff upstream,
>>     the other choice would've been to never support CPU register R/W on some IPs
>>
>>     ... and btw - about the last part: Kudos, MediaTek.
> 
> Thank you for all the insight! :)
> 
>>
>> Now, I also get why you're raising this, but we have to find an agreement here
>> on a different way to proceed that satisfies all of us.
>>
>> First of all..
>>
>> Which device on which SoC is missing the GCE client register DT property?
>> Do said SoC really not have a GCE client register for that device?
> 
> This is what I see:
> 
> On mt8192-asurada-spherion:
> mediatek-mutex 14001000.mutex: error -2 can't parse gce-client-reg property (0)
> 
> On mt8195-cherry-tomato:
> mtk-mmsys 14000000.syscon: error -2 can't parse gce-client-reg property (0)
> mtk-mmsys 14f00000.syscon: error -2 can't parse gce-client-reg property (0)
> mediatek-mutex 1c016000.mutex: error -2 can't parse gce-client-reg property (0)
> mtk-mmsys 1c01a000.syscon: error -2 can't parse gce-client-reg property (0)
> mediatek-mutex 1c101000.mutex: error -2 can't parse gce-client-reg property (0)
> 
> So yes, there are a few missing. I will send patches adding them so we can get
> the best performance possible upstream.
> 
>>
>> Is any upstream supported SoC really lacking a GCE register for the upstream
>> supported IPs?
>>
>> I'm not sure.... :-)
>>
>> P.S.: I guess that the alternative (that I somewhat dislike, and you can probably
>>        understand why after reading the reasons above) would be to turn that into a
>>        dev_info() instead...
>>
>> P.P.S.: Having no GCE usually means that there's a performance issue! In that case,
>>          it's ... a mistake, so, an error, kind-of.... :-)
> 
> Given the performance impact, I agree that debug, and even info level is not a
> good option. At the same time, the hardware still works correctly without the
> GCE (as we have been running it so far without even realizing!), so I think
> calling it an error is too much, and a warning would be the most suitable level,
> as we just want to warn userspace: "Hey user, be advised that you're missing
> GCE, so you'll get worse performance! It will still work though". What do you
> think?
> 

Agreed.

Cheers,
Angelo