linux-kernel - Re: [PATCH v3 3/8] drm/imagination: Use pwrseq for TH1520 GPU power management

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

Message-ID: <c7774790-07c3-469d-a994-9e84108ad21d@samsung.com>
Date: Thu, 5 Jun 2025 09:47:47 +0200
From: Michal Wilczynski <m.wilczynski@...sung.com>
To: Krzysztof Kozlowski <krzk@...nel.org>
Cc: Drew Fustini <drew@...7.com>, Guo Ren <guoren@...nel.org>, Fu Wei
	<wefu@...hat.com>, Rob Herring <robh@...nel.org>, Krzysztof Kozlowski
	<krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, Bartosz
	Golaszewski <brgl@...ev.pl>, Philipp Zabel <p.zabel@...gutronix.de>, Frank
	Binns <frank.binns@...tec.com>, Matt Coster <matt.coster@...tec.com>,
	Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>, Maxime Ripard
	<mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>, David Airlie
	<airlied@...il.com>, Simona Vetter <simona@...ll.ch>, Paul Walmsley
	<paul.walmsley@...ive.com>, Palmer Dabbelt <palmer@...belt.com>, Albert Ou
	<aou@...s.berkeley.edu>, Alexandre Ghiti <alex@...ti.fr>, Ulf Hansson
	<ulf.hansson@...aro.org>, Marek Szyprowski <m.szyprowski@...sung.com>,
	linux-riscv@...ts.infradead.org, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org,
	dri-devel@...ts.freedesktop.org
Subject: Re: [PATCH v3 3/8] drm/imagination: Use pwrseq for TH1520 GPU power
 management



On 6/4/25 14:07, Krzysztof Kozlowski wrote:
> On 04/06/2025 13:53, Michal Wilczynski wrote:
>>>>
>>>> The GPU node will depend on the AON node, which will be the sole
>>>> provider for the 'gpu-power' sequencer (based on the discussion in patch
>>>> 1).
>>>>
>>>> Therefore, if the AON/pwrseq driver has already completed its probe, and
>>>> devm_pwrseq_get() in the GPU driver subsequently returns -EPROBE_DEFER
>>>> (because pwrseq_get found 'no match' on the bus for 'gpu-power'), the
>>>> interpretation is that the AON driver did not register this optional
>>>> sequencer. Since AON is the only anticipated source, it implies the
>>>> sequencer won't become available later from its designated provider.
>>>
>>> I don't understand why you made this assumption. AON could be a module
>>> and this driver built-in. AON will likely probe later.
>>
>> You're absolutely right that AON could be a module and would generally
>> probe later in that scenario. However, the GPU device also has a
>> 'power-domains = <&aon TH1520_GPU_PD>' dependency. If the AON driver (as
>> the PM domain provider) were a late probing module, the GPU driver's
>> probe would hit -EPROBE_DEFER when its power domain is requested
>> which happens before attempting to get other resources like a power
>> sequencer.
> 
> Huh, so basically you imply certain hardware design and certain DTS
> description in your driver code. Well, that's clearly fragile design to
> me, because you should not rely how hardware properties are presented in
> DTS. Will work here on th1520 with this DTS, won't work with something else.
> 
> Especially that this looks like generic Imagination GPU code, common to
> multiple devices, not TH1520 only specific.
> 
>>
>> So, if the GPU driver's code does reach the devm_pwrseq_get(dev,
>> "gpu-power") call, it strongly implies the AON driver has already
>> successfully probed.
>>
>> This leads to the core challenge with the optional 'gpu-power'
>> sequencer: Even if the AON driver has already probed, if it then chooses
>> not to register the "gpu-power" sequence (because it's an optional
>> feature), pwrseq_get() will still find "no device matched" on the
>> pwrseq_bus and return EPROBE_DEFER.
>>
>> If the GPU driver defers here, as it normally should for -EPROBE_DEFER,
>> it could wait indefinitely for an optional sequence that its
>> already probed AON provider will not supply.
>>
>> Anyway I think you're right, that this is probably confusing and we
>> shouldn't rely on this behavior.
>>
>> To solve this, and to allow the GPU driver to correctly handle
>> -EPROBE_DEFER when a sequencer is genuinely expected, I propose using a
>> boolean property on the GPU's DT node, e.g.
>> img,gpu-expects-power-sequencer. If the GPU node provides this property
>> it means the pwrseq 'gpu-power' is required.
> 
> No, that would be driver design in DTS.
> 
> I think the main problem is the pwrseq API: you should get via phandle,
> not name of the pwrseq controller. That's how all producer-consumer
> relationships are done in OF platforms.

Bart,
Given Krzysztof's valid concerns about the current name based
lookup in pwrseq_get() and the benefits of phandle based resource
linking in OF platforms: Would you be open to a proposal for extending
the pwrseq API to allow consumers to obtain a sequencer (or a specific
target sequence) via a phandle defined in their Device Tree node? For
instance, a consumer device could specify power-sequencer =
<&aon> and a new API variant could resolve this.

> 
> It's also fragile to rely on names in case of systems with multiple
> similar devices. This does not affect your platform and this hardware in
> general, but shows issues with interface: imagine multiple gpus and
> multiple pwr sequence devices. Which one should be obtained?
> gpu-power-1? But if GPUs are the same class of devices (e.g. 2x TG1520
> GPU) this is just imprecise.
> 
> 
> Best regards,
> Krzysztof
> 

Best regards,
-- 
Michal Wilczynski <m.wilczynski@...sung.com>