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

[AngeloGioacchino Del Regno]

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 at 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?! :-)

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