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

[Jinjie Ruan <ruanjinjie@xxxxxxxxxx>]

On 2024/9/6 12:03, Dmitry Baryshkov wrote:
> 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.

Thank you! I'll fix it with devm_pm_runtime_enable() in v3 in the first
patch according to your suggestion.

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