| Message ID | 20250408033206.12176-4-friday.yang@mediatek.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add SMI reset and clamp for MediaTek MT8188 SoC | expand |
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
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 :-) Instead, the big deal is that Friday forgot to retain Reviewed-by and Acked-by tags that were released to him on those patches....... :-) Cheers, Angelo
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? I see that runtime PM will be disabled much later and what certainty you have that device won't get resumed that time? Best regards, Krzysztof
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?! :-) 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
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
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); err_link_remove: device_link_remove(dev, larb->smi_common_dev); return ret; @@ -733,7 +734,6 @@ static void mtk_smi_larb_remove(struct platform_device *pdev) struct mtk_smi_larb *larb = platform_get_drvdata(pdev); device_link_remove(&pdev->dev, larb->smi_common_dev); - pm_runtime_disable(&pdev->dev); component_del(&pdev->dev, &mtk_smi_larb_component_ops); } @@ -954,7 +954,10 @@ static int mtk_smi_common_probe(struct platform_device *pdev) return ret; } - pm_runtime_enable(dev); + ret = devm_pm_runtime_enable(dev); + if (ret) + return ret; + platform_set_drvdata(pdev, common); return 0; } @@ -965,7 +968,6 @@ static void mtk_smi_common_remove(struct platform_device *pdev) if (common->plat->type == MTK_SMI_GEN2_SUB_COMM) device_link_remove(&pdev->dev, common->smi_common_dev); - pm_runtime_disable(&pdev->dev); } static int __maybe_unused mtk_smi_common_resume(struct device *dev)
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(-) -- 2.46.0