linux-kernel - Re: [PATCH v6 3/3] memory: mtk-smi: mt8188: Use devm_pm_runtime

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: <399f89fb-092e-4fb3-8a0b-987dea129554@collabora.com>
Date: Wed, 9 Apr 2025 17:50:55 +0200
From: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
To: Krzysztof Kozlowski <krzk@...nel.org>,
 Friday Yang <friday.yang@...iatek.com>
Cc: Yong Wu <yong.wu@...iatek.com>, Rob Herring <robh@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>, Matthias Brugger
 <matthias.bgg@...il.com>, Philipp Zabel <p.zabel@...gutronix.de>,
 linux-mediatek@...ts.infradead.org, linux-kernel@...r.kernel.org,
 devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
 Project_Global_Chrome_Upstream_Group@...iatek.com
Subject: Re: [PATCH v6 3/3] memory: mtk-smi: mt8188: Use
 devm_pm_runtime_enable

Il 09/04/25 11:56, Krzysztof Kozlowski ha scritto:
> On 09/04/2025 10:26, AngeloGioacchino Del Regno wrote:
>> Il 08/04/25 08:29, Krzysztof Kozlowski ha scritto:
>>> On Tue, Apr 08, 2025 at 11:31:56AM GMT, Friday Yang wrote:
>>>> Replace pm_runtime_enable with the devres-enabled version which
>>>> can trigger pm_runtime_disable.
>>>>
>>>> Signed-off-by: Friday Yang <friday.yang@...iatek.com>
>>>> ---
>>>>    drivers/memory/mtk-smi.c | 16 +++++++++-------
>>>>    1 file changed, 9 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/drivers/memory/mtk-smi.c b/drivers/memory/mtk-smi.c
>>>> index f25d46d2ef33..daef6d350419 100644
>>>> --- a/drivers/memory/mtk-smi.c
>>>> +++ b/drivers/memory/mtk-smi.c
>>>> @@ -713,16 +713,17 @@ static int mtk_smi_larb_probe(struct platform_device *pdev)
>>>>    	if (ret)
>>>>    		goto err_link_remove;
>>>>
>>>> -	pm_runtime_enable(dev);
>>>> +	ret = devm_pm_runtime_enable(dev);
>>>> +	if (ret)
>>>> +		goto err_link_remove;
>>>> +
>>>>    	platform_set_drvdata(pdev, larb);
>>>>    	ret = component_add(dev, &mtk_smi_larb_component_ops);
>>>>    	if (ret)
>>>> -		goto err_pm_disable;
>>>> +		goto err_link_remove;
>>>>
>>>>    	return 0;
>>>>
>>>> -err_pm_disable:
>>>> -	pm_runtime_disable(dev);
>>>
>>> You now broke/changed the order of cleanup without any explanation.
>>>
>>> Best regards,
>>> Krzysztof
>>>
>>
>> I agree some comment in the commit description saying that the cleanup reordering
>> doesn't matter in this specific case would've been nice to have, but anyway IMO
>> it's not a big deal - he didn't break anything, anyway :-)
> 
> Cleanup orderings are tricky, so are you sure nothing got here called in
> incorrect moment?

Yes.

 >> I see that runtime PM will be disabled much later and
> what certainty you have that device won't get resumed that time?
> 
How can a device that failed to probe be resumed?! Who's going to resume it?! :-)

Also, in the remove phase, all users get removed first, there's no ISR (implies
that there's no isr that will resume this device inadvertently, and other than
no isr - there's no kthread/queue/this/that that could do this), and no nothing.

Moreover, SMI-LARB cannot be removed unless all of the components are unbound;
SMI-Common (be it a common or a sub-common) cannot be removed if SMI-LARB is still
using it.

No I don't see anything that can resume it before devm does its job.

If you do see something though, I'm curious to understand what I'm missing here :-)

Cheers!
Angelo