Re: [PATCH v6 3/3] memory: mtk-smi: mt8188: Use devm_pm_runtime_enable

[Krzysztof Kozlowski <krzk@kernel.org>]

On 09/04/2025 17:50, AngeloGioacchino Del Regno wrote:
> 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@mediatek.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?! :-)

That's unbind path.

> 
> 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.

so this should be in commit msg... I doubt that author did any
investigation but instead just blindly converted to devm.

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

You change the order of cleanup and this is known to introduce errors.
Real bugs during probe error paths or removal. Some are tricky to
trigger, but some are obvious and really happening. The easiest to
trigger issues is for devices sharing interrupts (there is even CONFIG
for that). That's why generic recommendation is don't use devm with
shared interrupts. Even more generic recommendation is don't mix devm
with non-devm, but just choose one.

Best regards,
Krzysztof