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: <a6a29e58-8613-47f0-9e5c-d125da7ddb49@samsung.com>
Date: Wed, 4 Jun 2025 13:53:49 +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 08:36, Krzysztof Kozlowski wrote:
> On 03/06/2025 21:43, Michal Wilczynski wrote:
>>>> +	 * and resets. Otherwise, we fall back to managing them ourselves.
>>>> +	 */
>>>> +	pvr_dev->pwrseq = devm_pwrseq_get(dev, "gpu-power");
>>>> +	if (IS_ERR(pvr_dev->pwrseq)) {
>>>> +		int pwrseq_err = PTR_ERR(pvr_dev->pwrseq);
>>>> +
>>>> +		/*
>>>> +		 * If the error is -EPROBE_DEFER, it's because the
>>>> +		 * optional sequencer provider is not present
>>>> +		 * and it's safe to fall back on manual power-up.
>>>
>>> It is safe but why it is desirable? The rule is rather to defer the
>>> probe, assuming this is probe path.
>>
>> Yeah this is probe path.
>>
>> 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.

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.

I didn't want to use this approach at first, as it seemed to me like the
pwrseq API had a hands-off approach for the DT with its matching logic,
but it seems unavoidable at this point.

> 
> 
> 
> Best regards,
> Krzysztof
> 

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