| Message ID | 20240906031345.1052241-1-ruanjinjie@huawei.com |
|---|---|
| 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 🤣 > >> >>> >>>> >>>> >>>>> >>>>>> >>>>>>> >>>>>>>> --- >>>>>>>> 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 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 🤣 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 > >>>>>>>> > >>>>>>> > >>>>> > >>>>> > >>>>> > >>> > >>> > >>> > > > > > >
Hi Jinjie, On 9/6/2024 8:43 AM, Jinjie Ruan wrote: > In spi_geni_remove(), the IRQ will still remain and it's interrupt handler > may use the dma channel after release dma channel and before free irq, > which is not secure, fix it. > What's the possibility of having irq if spi_geni_release_dma_chan(mas) is completed ? As such controller is already unregistered so transfer request can't come. > Fixes: b59c122484ec ("spi: spi-geni-qcom: Add support for GPI dma") > Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com> > --- > drivers/spi/spi-geni-qcom.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c > index 37ef8c40b276..fc2819effe2d 100644 > --- a/drivers/spi/spi-geni-qcom.c > +++ b/drivers/spi/spi-geni-qcom.c > @@ -1170,9 +1170,10 @@ static void spi_geni_remove(struct platform_device *pdev) > /* Unregister _before_ disabling pm_runtime() so we stop transfers */ > spi_unregister_controller(spi); > > + free_irq(mas->irq, spi); > + > spi_geni_release_dma_chan(mas); > > - free_irq(mas->irq, spi); > pm_runtime_disable(&pdev->dev); > } >
On 2024/9/6 12:27, Mukesh Kumar Savaliya wrote: > Hi Jinjie, > > On 9/6/2024 8:43 AM, Jinjie Ruan wrote: >> In spi_geni_remove(), the IRQ will still remain and it's interrupt >> handler >> may use the dma channel after release dma channel and before free irq, >> which is not secure, fix it. >> > What's the possibility of having irq if spi_geni_release_dma_chan(mas) > is completed ? As such controller is already unregistered so transfer > request can't come. The irq is not freed, the IRQ can come and then it may enter the irq handler with the registered one. >> Fixes: b59c122484ec ("spi: spi-geni-qcom: Add support for GPI dma") >> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com> >> --- >> Â drivers/spi/spi-geni-qcom.c | 3 ++- >> Â 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c >> index 37ef8c40b276..fc2819effe2d 100644 >> --- a/drivers/spi/spi-geni-qcom.c >> +++ b/drivers/spi/spi-geni-qcom.c >> @@ -1170,9 +1170,10 @@ static void spi_geni_remove(struct >> platform_device *pdev) >> Â Â Â Â Â /* Unregister _before_ disabling pm_runtime() so we stop >> transfers */ >> Â Â Â Â Â spi_unregister_controller(spi); >> Â +Â Â Â free_irq(mas->irq, spi); >> + >> Â Â Â Â Â spi_geni_release_dma_chan(mas); >> Â -Â Â Â free_irq(mas->irq, spi); >> Â Â Â Â Â pm_runtime_disable(&pdev->dev); >> Â } >> Â >
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 🤣 > > 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 >>>>>>>>>> >>>>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>> >>>>> >>>>> >>> >>> >>> > > >
Thanks ! On 9/6/2024 12:17 PM, Jinjie Ruan wrote: > > > On 2024/9/6 12:27, Mukesh Kumar Savaliya wrote: >> Hi Jinjie, >> >> On 9/6/2024 8:43 AM, Jinjie Ruan wrote: >>> In spi_geni_remove(), the IRQ will still remain and it's interrupt >>> handler >>> may use the dma channel after release dma channel and before free irq, >>> which is not secure, fix it. >>> >> What's the possibility of having irq if spi_geni_release_dma_chan(mas) >> is completed ? As such controller is already unregistered so transfer >> request can't come. > > The irq is not freed, the IRQ can come and then it may enter the irq > handler with the registered one. > My question is about knowing source of interrupt at earlier place. By just moving it above to spi_geni_release_dma_chan(), is there anything changing ? >>> Fixes: b59c122484ec ("spi: spi-geni-qcom: Add support for GPI dma") >>> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com> >>> --- >>> Â drivers/spi/spi-geni-qcom.c | 3 ++- >>> Â 1 file changed, 2 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c >>> index 37ef8c40b276..fc2819effe2d 100644 >>> --- a/drivers/spi/spi-geni-qcom.c >>> +++ b/drivers/spi/spi-geni-qcom.c >>> @@ -1170,9 +1170,10 @@ static void spi_geni_remove(struct >>> platform_device *pdev) >>> Â Â Â Â Â /* Unregister _before_ disabling pm_runtime() so we stop >>> transfers */ >>> Â Â Â Â Â spi_unregister_controller(spi); >>> Â +Â Â Â free_irq(mas->irq, spi); >>> + >>> Â Â Â Â Â spi_geni_release_dma_chan(mas); >>> Â -Â Â Â free_irq(mas->irq, spi); >>> Â Â Â Â Â pm_runtime_disable(&pdev->dev); >>> Â } >>> >>
Fix two bugs for geni-qcom. Changes in v2: - Split out the device managed cleanup patch. - PATCH -next -> PATCH - Also fix the incorrect free_irq() sequence. Jinjie Ruan (2): spi: geni-qcom: Fix incorrect free_irq() sequence spi: spi-geni-qcom: Fix missing undo runtime PM changes at driver exit time drivers/spi/spi-geni-qcom.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)