diff mbox series

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

Message ID 20240906031345.1052241-3-ruanjinjie@huawei.com (mailing list archive)
State New, archived
Headers show
Series spi: geni-qcom: Fix incorrect free_irq() sequence | expand

Commit Message

Jinjie Ruan Sept. 6, 2024, 3:13 a.m. UTC
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().
---
 drivers/spi/spi-geni-qcom.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Dmitry Baryshkov Sept. 6, 2024, 3:15 a.m. UTC | #1
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?

> ---
>  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
>
Jinjie Ruan Sept. 6, 2024, 3:31 a.m. UTC | #2
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@mail.gmail.com/

> 
>> ---
>>  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
>>
>
Dmitry Baryshkov Sept. 6, 2024, 3:36 a.m. UTC | #3
On Fri, 6 Sept 2024 at 06:31, Jinjie Ruan <ruanjinjie@huawei.com> 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@mail.gmail.com/

That comment was for devm_request_irq(), not devm_pm_runtime_enable().

>
> >
> >> ---
> >>  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
> >>
> >
Jinjie Ruan Sept. 6, 2024, 3:40 a.m. UTC | #4
On 2024/9/6 11:36, Dmitry Baryshkov wrote:
> On Fri, 6 Sept 2024 at 06:31, Jinjie Ruan <ruanjinjie@huawei.com> 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@mail.gmail.com/
> 
> 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.


> 
>>
>>>
>>>> ---
>>>>  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
>>>>
>>>
> 
> 
>
Dmitry Baryshkov Sept. 6, 2024, 3:43 a.m. UTC | #5
On Fri, 6 Sept 2024 at 06:41, Jinjie Ruan <ruanjinjie@huawei.com> wrote:
>
>
>
> On 2024/9/6 11:36, Dmitry Baryshkov wrote:
> > On Fri, 6 Sept 2024 at 06:31, Jinjie Ruan <ruanjinjie@huawei.com> 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@mail.gmail.com/
> >
> > 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.

>
>
> >
> >>
> >>>
> >>>> ---
> >>>>  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
> >>>>
> >>>
> >
> >
> >
Jinjie Ruan Sept. 6, 2024, 3:50 a.m. UTC | #6
On 2024/9/6 11:43, Dmitry Baryshkov wrote:
> On Fri, 6 Sept 2024 at 06:41, Jinjie Ruan <ruanjinjie@huawei.com> wrote:
>>
>>
>>
>> On 2024/9/6 11:36, Dmitry Baryshkov wrote:
>>> On Fri, 6 Sept 2024 at 06:31, Jinjie Ruan <ruanjinjie@huawei.com> 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@mail.gmail.com/
>>>
>>> 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.

> 
>>
>>
>>>
>>>>
>>>>>
>>>>>> ---
>>>>>>  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
>>>>>>
>>>>>
>>>
>>>
>>>
> 
> 
>
Dmitry Baryshkov Sept. 6, 2024, 3:52 a.m. UTC | #7
On Fri, 6 Sept 2024 at 06:51, Jinjie Ruan <ruanjinjie@huawei.com> wrote:
>
>
>
> On 2024/9/6 11:43, Dmitry Baryshkov wrote:
> > On Fri, 6 Sept 2024 at 06:41, Jinjie Ruan <ruanjinjie@huawei.com> wrote:
> >>
> >>
> >>
> >> On 2024/9/6 11:36, Dmitry Baryshkov wrote:
> >>> On Fri, 6 Sept 2024 at 06:31, Jinjie Ruan <ruanjinjie@huawei.com> 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@mail.gmail.com/
> >>>
> >>> 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.

>
> >
> >>
> >>
> >>>
> >>>>
> >>>>>
> >>>>>> ---
> >>>>>>  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
> >>>>>>
> >>>>>
> >>>
> >>>
> >>>
> >
> >
> >
Jinjie Ruan Sept. 6, 2024, 4:01 a.m. UTC | #8
On 2024/9/6 11:52, Dmitry Baryshkov wrote:
> On Fri, 6 Sept 2024 at 06:51, Jinjie Ruan <ruanjinjie@huawei.com> wrote:
>>
>>
>>
>> On 2024/9/6 11:43, Dmitry Baryshkov wrote:
>>> On Fri, 6 Sept 2024 at 06:41, Jinjie Ruan <ruanjinjie@huawei.com> wrote:
>>>>
>>>>
>>>>
>>>> On 2024/9/6 11:36, Dmitry Baryshkov wrote:
>>>>> On Fri, 6 Sept 2024 at 06:31, Jinjie Ruan <ruanjinjie@huawei.com> 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@mail.gmail.com/
>>>>>
>>>>> 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 
Dmitry Baryshkov Sept. 6, 2024, 4:03 a.m. UTC | #9
On Fri, 6 Sept 2024 at 07:02, Jinjie Ruan <ruanjinjie@huawei.com> wrote:
>
>
>
> On 2024/9/6 11:52, Dmitry Baryshkov wrote:
> > On Fri, 6 Sept 2024 at 06:51, Jinjie Ruan <ruanjinjie@huawei.com> wrote:
> >>
> >>
> >>
> >> On 2024/9/6 11:43, Dmitry Baryshkov wrote:
> >>> On Fri, 6 Sept 2024 at 06:41, Jinjie Ruan <ruanjinjie@huawei.com> wrote:
> >>>>
> >>>>
> >>>>
> >>>> On 2024/9/6 11:36, Dmitry Baryshkov wrote:
> >>>>> On Fri, 6 Sept 2024 at 06:31, Jinjie Ruan <ruanjinjie@huawei.com> 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@mail.gmail.com/
> >>>>>
> >>>>> 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 
Jinjie Ruan Sept. 6, 2024, 6:50 a.m. UTC | #10
On 2024/9/6 12:03, Dmitry Baryshkov wrote:
> On Fri, 6 Sept 2024 at 07:02, Jinjie Ruan <ruanjinjie@huawei.com> wrote:
>>
>>
>>
>> On 2024/9/6 11:52, Dmitry Baryshkov wrote:
>>> On Fri, 6 Sept 2024 at 06:51, Jinjie Ruan <ruanjinjie@huawei.com> wrote:
>>>>
>>>>
>>>>
>>>> On 2024/9/6 11:43, Dmitry Baryshkov wrote:
>>>>> On Fri, 6 Sept 2024 at 06:41, Jinjie Ruan <ruanjinjie@huawei.com> wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 2024/9/6 11:36, Dmitry Baryshkov wrote:
>>>>>>> On Fri, 6 Sept 2024 at 06:31, Jinjie Ruan <ruanjinjie@huawei.com> 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@mail.gmail.com/
>>>>>>>
>>>>>>> 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 
diff mbox series

Patch

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);
 }