Re: [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@xxxxxxxxxxxx>
> > > > ---
> > > >    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