| Message ID | 20240906031345.1052241-3-ruanjinjie@huawei.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | spi: geni-qcom: Fix incorrect free_irq() sequence | expand |
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 >
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 >> >
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 > >> > >
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 >>>> >>> > > >
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 > >>>> > >>> > > > > > >
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 >>>>>> >>>>> >>> >>> >>> > > >
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 > >>>>>> > >>>>> > >>> > >>> > >>> > > > > > >
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
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
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 --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); }
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(+)