Re: [PATCH v2 2/2] spi: spi-geni-qcom: Fix missing undo runtime PM changes at driver exit time

[Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>]

On Fri, 6 Sept 2024 at 07:02, Jinjie Ruan <ruanjinjie@xxxxxxxxxx> wrote:
>
>
>
> On 2024/9/6 11:52, Dmitry Baryshkov wrote:
> > On Fri, 6 Sept 2024 at 06:51, Jinjie Ruan <ruanjinjie@xxxxxxxxxx> wrote:
> >>
> >>
> >>
> >> On 2024/9/6 11:43, Dmitry Baryshkov wrote:
> >>> On Fri, 6 Sept 2024 at 06:41, Jinjie Ruan <ruanjinjie@xxxxxxxxxx> wrote:
> >>>>
> >>>>
> >>>>
> >>>> On 2024/9/6 11:36, Dmitry Baryshkov wrote:
> >>>>> On Fri, 6 Sept 2024 at 06:31, Jinjie Ruan <ruanjinjie@xxxxxxxxxx> wrote:
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>> On 2024/9/6 11:15, Dmitry Baryshkov wrote:
> >>>>>>> On Fri, Sep 06, 2024 at 11:13:45AM GMT, Jinjie Ruan wrote:
> >>>>>>>> It's important to undo pm_runtime_use_autosuspend() with
> >>>>>>>> pm_runtime_dont_use_autosuspend() at driver exit time unless driver
> >>>>>>>> initially enabled pm_runtime with devm_pm_runtime_enable()
> >>>>>>>> (which handles it for you).
> >>>>>>>>
> >>>>>>>> Hence, call pm_runtime_dont_use_autosuspend() at driver exit time
> >>>>>>>> to fix it.
> >>>>>>>>
> >>>>>>>> Fixes: cfdab2cd85ec ("spi: spi-geni-qcom: Set an autosuspend delay of 250 ms")
> >>>>>>>> ---
> >>>>>>>> v2:
> >>>>>>>> - Fix it directly instead of use devm_pm_runtime_enable().
> >>>>>>>
> >>>>>>> Why?
> >>>>>>
> >>>>>> The devm* sequence will have some problem, which will not consistent
> >>>>>> with the former.
> >>>>>>
> >>>>>> Link:
> >>>>>> https://lore.kernel.org/all/CAD=FV=VyDk-e2KNiuiBcACFAdrQmihOH6X6BSpGB+T1MsgsiKw@xxxxxxxxxxxxxx/
> >>>>>
> >>>>> That comment was for devm_request_irq(), not devm_pm_runtime_enable().
> >>>>
> >>>>
> >>>> In the very least, ** parch #2 needs to come before this one and that
> >>>> would help, but it won't fix everything **. Specifically in order to
> >>>> keep the order proper you'll need to use devm_add_action_or_reset() to
> >>>> "devm-ize" the freeing of the DMA channels.
> >>>
> >>> This is patch #2. so I don't understand your comment. Moreover you
> >>> don't have to use devm for each and every possible item. However I
> >>> think it makes sense for pm_runtime in this case.
> >>
> >> You are right, only use devm_pm_runtime_enable() here, there is no
> >> change for the resource release sequence, but I have a cleanup patch
> >> ready to replace all these with devm*, which depends on the 2 fix patch.
> >
> > You can use the devm_pm_runtime_enable() here and land the rest of the
> > cleanups afterwards.
>
> But Doug suggest that the bug fix patch should not contain "-next", but
> the cleanup patch is "-next", which let me split them 🤣

Using devm_pm_runtime_enable() is a bugfix too, if done properly.

>
> >
> >>
> >>>
> >>>>
> >>>>
> >>>>>
> >>>>>>
> >>>>>>>
> >>>>>>>> ---
> >>>>>>>>  drivers/spi/spi-geni-qcom.c | 2 ++
> >>>>>>>>  1 file changed, 2 insertions(+)
> >>>>>>>>
> >>>>>>>> diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
> >>>>>>>> index fc2819effe2d..38857edbc785 100644
> >>>>>>>> --- a/drivers/spi/spi-geni-qcom.c
> >>>>>>>> +++ b/drivers/spi/spi-geni-qcom.c
> >>>>>>>> @@ -1158,6 +1158,7 @@ static int spi_geni_probe(struct platform_device *pdev)
> >>>>>>>>  spi_geni_release_dma:
> >>>>>>>>      spi_geni_release_dma_chan(mas);
> >>>>>>>>  spi_geni_probe_runtime_disable:
> >>>>>>>> +    pm_runtime_dont_use_autosuspend(dev);
> >>>>>>>>      pm_runtime_disable(dev);
> >>>>>>>>      return ret;
> >>>>>>>>  }
> >>>>>>>> @@ -1174,6 +1175,7 @@ static void spi_geni_remove(struct platform_device *pdev)
> >>>>>>>>
> >>>>>>>>      spi_geni_release_dma_chan(mas);
> >>>>>>>>
> >>>>>>>> +    pm_runtime_dont_use_autosuspend(&pdev->dev);
> >>>>>>>>      pm_runtime_disable(&pdev->dev);
> >>>>>>>>  }
> >>>>>>>>
> >>>>>>>> --
> >>>>>>>> 2.34.1
> >>>>>>>>
> >>>>>>>
> >>>>>
> >>>>>
> >>>>>
> >>>
> >>>
> >>>
> >
> >
> >



-- 
With best wishes
Dmitry