[PATCH v2 0/2] spi: geni-qcom: Fix incorrect free_irq() sequence

[PATCH v2 0/2] spi: geni-qcom: Fix incorrect free_irq() sequence

[Jinjie Ruan]

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

-- 
2.34.1

[PATCH v2 1/2] spi: geni-qcom: Fix incorrect free_irq() sequence

[Jinjie Ruan]

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.

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

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

[Jinjie Ruan]

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(+)

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

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

[Dmitry Baryshkov]

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
> 

-- 
With best wishes
Dmitry

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

[Jinjie Ruan]

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

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

[Dmitry Baryshkov]

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



-- 
With best wishes
Dmitry

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

[Jinjie Ruan]

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

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

[Dmitry Baryshkov]

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



-- 
With best wishes
Dmitry

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

[Jinjie Ruan]

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

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

[Dmitry Baryshkov]

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



-- 
With best wishes
Dmitry

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

[Jinjie Ruan]

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

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

[Dmitry Baryshkov]

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



-- 
With best wishes
Dmitry

Re: [PATCH v2 1/2] spi: geni-qcom: Fix incorrect free_irq() sequence

[Mukesh Kumar Savaliya]

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

Re: [PATCH v2 1/2] spi: geni-qcom: Fix incorrect free_irq() sequence

[Jinjie Ruan]

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

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

[Jinjie Ruan]

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

Re: [PATCH v2 1/2] spi: geni-qcom: Fix incorrect free_irq() sequence

[Mukesh Kumar Savaliya]

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