[PATCH v3 0/3] spi: geni-qcom: Undo runtime PM changes at driver exit time

[PATCH v3 0/3] spi: geni-qcom: Undo runtime PM changes at driver exit time

[Jinjie Ruan]

Fix two bugs for geni-qcom and use dev managed function
to simplify code.

Changes in v3:
- Adjust the runtime PM patch to be the first.
- Use devm_pm_runtime_enable() to undo runtime PM changes.
- Land the rest of the cleanups afterwards.
- Update the commit message.

Changes in v2:
- Split out the device managed cleanup patch.
- PATCH -next -> PATCH
- Also fix the incorrect free_irq() sequence.

Jinjie Ruan (3):
  spi: geni-qcom: Undo runtime PM changes at driver exit time
  spi: geni-qcom: Fix incorrect free_irq() sequence
  spi: geni-qcom: Use devm functions to simplify code

 drivers/spi/spi-geni-qcom.c | 50 ++++++++++++++-----------------------
 1 file changed, 19 insertions(+), 31 deletions(-)

-- 
2.34.1

[PATCH v3 1/3] spi: geni-qcom: 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, switch to devm_pm_runtime_enable() to fix it, so the
pm_runtime_disable() in probe error path and remove function
can be removed.

Fixes: cfdab2cd85ec ("spi: spi-geni-qcom: Set an autosuspend delay of 250 ms")
Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
Suggested-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
v3:
- Fix it with devm_pm_runtime_enable() as Dmitry suggested.
- Adjust to be the first patch.
- Add suggested-by.
v2:
- Fix it directly instead of use devm_pm_runtime_enable().
---
 drivers/spi/spi-geni-qcom.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
index 37ef8c40b276..fef522fece1b 100644
--- a/drivers/spi/spi-geni-qcom.c
+++ b/drivers/spi/spi-geni-qcom.c
@@ -1110,25 +1110,27 @@ static int spi_geni_probe(struct platform_device *pdev)
 	spin_lock_init(&mas->lock);
 	pm_runtime_use_autosuspend(&pdev->dev);
 	pm_runtime_set_autosuspend_delay(&pdev->dev, 250);
-	pm_runtime_enable(dev);
+	ret = devm_pm_runtime_enable(dev);
+	if (ret)
+		return ret;
 
 	if (device_property_read_bool(&pdev->dev, "spi-slave"))
 		spi->target = true;
 
 	ret = geni_icc_get(&mas->se, NULL);
 	if (ret)
-		goto spi_geni_probe_runtime_disable;
+		return ret;
 	/* Set the bus quota to a reasonable value for register access */
 	mas->se.icc_paths[GENI_TO_CORE].avg_bw = Bps_to_icc(CORE_2X_50_MHZ);
 	mas->se.icc_paths[CPU_TO_GENI].avg_bw = GENI_DEFAULT_BW;
 
 	ret = geni_icc_set_bw(&mas->se);
 	if (ret)
-		goto spi_geni_probe_runtime_disable;
+		return ret;
 
 	ret = spi_geni_init(mas);
 	if (ret)
-		goto spi_geni_probe_runtime_disable;
+		return ret;
 
 	/*
 	 * check the mode supported and set_cs for fifo mode only
@@ -1157,8 +1159,6 @@ static int spi_geni_probe(struct platform_device *pdev)
 	free_irq(mas->irq, spi);
 spi_geni_release_dma:
 	spi_geni_release_dma_chan(mas);
-spi_geni_probe_runtime_disable:
-	pm_runtime_disable(dev);
 	return ret;
 }
 
@@ -1173,7 +1173,6 @@ static void spi_geni_remove(struct platform_device *pdev)
 	spi_geni_release_dma_chan(mas);
 
 	free_irq(mas->irq, spi);
-	pm_runtime_disable(&pdev->dev);
 }
 
 static int __maybe_unused spi_geni_runtime_suspend(struct device *dev)
-- 
2.34.1

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

[Jinjie Ruan]

In spi_geni_remove(), the free_irq() sequence is different from that
on the probe error path. And 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>
---
v3:
- Rebased on the devm_pm_runtime_enable() patch.
- Update the commit message.
---
 drivers/spi/spi-geni-qcom.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
index fef522fece1b..6f4057330444 100644
--- a/drivers/spi/spi-geni-qcom.c
+++ b/drivers/spi/spi-geni-qcom.c
@@ -1170,9 +1170,9 @@ static void spi_geni_remove(struct platform_device *pdev)
 	/* Unregister _before_ disabling pm_runtime() so we stop transfers */
 	spi_unregister_controller(spi);
 
-	spi_geni_release_dma_chan(mas);
-
 	free_irq(mas->irq, spi);
+
+	spi_geni_release_dma_chan(mas);
 }
 
 static int __maybe_unused spi_geni_runtime_suspend(struct device *dev)
-- 
2.34.1

[PATCH v3 3/3] spi: geni-qcom: Use devm functions to simplify code

[Jinjie Ruan]

Use devm_pm_runtime_enable(), devm_request_irq() and
devm_spi_register_controller() to simplify code.

And also register a callback spi_geni_release_dma_chan() with
devm_add_action_or_reset(), to release dma channel in both error
and device detach path, which can make sure the release sequence is
consistent with the original one.

1. Unregister spi controller.
2. Free the IRQ.
3. Free DMA chans
4. Disable runtime PM.

So the remove function can also be removed.

Suggested-by: Doug Anderson <dianders@chromium.org>
Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
---
v3:
- Land the rest of the cleanups afterwards.
---
 drivers/spi/spi-geni-qcom.c | 37 +++++++++++++------------------------
 1 file changed, 13 insertions(+), 24 deletions(-)

diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
index 6f4057330444..8b0039d14605 100644
--- a/drivers/spi/spi-geni-qcom.c
+++ b/drivers/spi/spi-geni-qcom.c
@@ -632,8 +632,10 @@ static int spi_geni_grab_gpi_chan(struct spi_geni_master *mas)
 	return ret;
 }
 
-static void spi_geni_release_dma_chan(struct spi_geni_master *mas)
+static void spi_geni_release_dma_chan(void *data)
 {
+	struct spi_geni_master *mas = data;
+
 	if (mas->rx) {
 		dma_release_channel(mas->rx);
 		mas->rx = NULL;
@@ -1132,6 +1134,12 @@ static int spi_geni_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	ret = devm_add_action_or_reset(dev, spi_geni_release_dma_chan, spi);
+	if (ret) {
+		dev_err(dev, "Unable to add action.\n");
+		return ret;
+	}
+
 	/*
 	 * check the mode supported and set_cs for fifo mode only
 	 * for dma (gsi) mode, the gsi will set cs based on params passed in
@@ -1146,33 +1154,15 @@ static int spi_geni_probe(struct platform_device *pdev)
 	if (mas->cur_xfer_mode == GENI_GPI_DMA)
 		spi->flags = SPI_CONTROLLER_MUST_TX;
 
-	ret = request_irq(mas->irq, geni_spi_isr, 0, dev_name(dev), spi);
+	ret = devm_request_irq(dev, mas->irq, geni_spi_isr, 0, dev_name(dev), spi);
 	if (ret)
-		goto spi_geni_release_dma;
+		return ret;
 
-	ret = spi_register_controller(spi);
+	ret = devm_spi_register_controller(dev, spi);
 	if (ret)
-		goto spi_geni_probe_free_irq;
+		return ret;
 
 	return 0;
-spi_geni_probe_free_irq:
-	free_irq(mas->irq, spi);
-spi_geni_release_dma:
-	spi_geni_release_dma_chan(mas);
-	return ret;
-}
-
-static void spi_geni_remove(struct platform_device *pdev)
-{
-	struct spi_controller *spi = platform_get_drvdata(pdev);
-	struct spi_geni_master *mas = spi_controller_get_devdata(spi);
-
-	/* Unregister _before_ disabling pm_runtime() so we stop transfers */
-	spi_unregister_controller(spi);
-
-	free_irq(mas->irq, spi);
-
-	spi_geni_release_dma_chan(mas);
 }
 
 static int __maybe_unused spi_geni_runtime_suspend(struct device *dev)
@@ -1254,7 +1244,6 @@ MODULE_DEVICE_TABLE(of, spi_geni_dt_match);
 
 static struct platform_driver spi_geni_driver = {
 	.probe  = spi_geni_probe,
-	.remove_new = spi_geni_remove,
 	.driver = {
 		.name = "geni_spi",
 		.pm = &spi_geni_pm_ops,
-- 
2.34.1

Re: [PATCH v3 1/3] spi: geni-qcom: Undo runtime PM changes at driver exit time

[Dmitry Baryshkov]

On Mon, Sep 09, 2024 at 03:31:39PM 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, switch to devm_pm_runtime_enable() to fix it, so the
> pm_runtime_disable() in probe error path and remove function
> can be removed.
> 
> Fixes: cfdab2cd85ec ("spi: spi-geni-qcom: Set an autosuspend delay of 250 ms")
> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
> Suggested-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
> v3:
> - Fix it with devm_pm_runtime_enable() as Dmitry suggested.
> - Adjust to be the first patch.
> - Add suggested-by.
> v2:
> - Fix it directly instead of use devm_pm_runtime_enable().
> ---
>  drivers/spi/spi-geni-qcom.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>


-- 
With best wishes
Dmitry

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

[Dmitry Baryshkov]

On Mon, Sep 09, 2024 at 03:31:40PM GMT, Jinjie Ruan wrote:
> In spi_geni_remove(), the free_irq() sequence is different from that
> on the probe error path. And 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>
> ---
> v3:
> - Rebased on the devm_pm_runtime_enable() patch.
> - Update the commit message.
> ---
>  drivers/spi/spi-geni-qcom.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

This matches the code in spi_geni_probe().


Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>


-- 
With best wishes
Dmitry

Re: [PATCH v3 3/3] spi: geni-qcom: Use devm functions to simplify code

[Dmitry Baryshkov]

On Mon, Sep 09, 2024 at 03:31:41PM GMT, Jinjie Ruan wrote:
> Use devm_pm_runtime_enable(), devm_request_irq() and
> devm_spi_register_controller() to simplify code.
> 
> And also register a callback spi_geni_release_dma_chan() with
> devm_add_action_or_reset(), to release dma channel in both error
> and device detach path, which can make sure the release sequence is
> consistent with the original one.
> 
> 1. Unregister spi controller.
> 2. Free the IRQ.
> 3. Free DMA chans
> 4. Disable runtime PM.
> 
> So the remove function can also be removed.
> 
> Suggested-by: Doug Anderson <dianders@chromium.org>
> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
> ---
> v3:
> - Land the rest of the cleanups afterwards.
> ---
>  drivers/spi/spi-geni-qcom.c | 37 +++++++++++++------------------------
>  1 file changed, 13 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
> index 6f4057330444..8b0039d14605 100644
> --- a/drivers/spi/spi-geni-qcom.c
> +++ b/drivers/spi/spi-geni-qcom.c
> @@ -632,8 +632,10 @@ static int spi_geni_grab_gpi_chan(struct spi_geni_master *mas)
>  	return ret;
>  }
>  
> -static void spi_geni_release_dma_chan(struct spi_geni_master *mas)
> +static void spi_geni_release_dma_chan(void *data)
>  {
> +	struct spi_geni_master *mas = data;
> +
>  	if (mas->rx) {
>  		dma_release_channel(mas->rx);
>  		mas->rx = NULL;
> @@ -1132,6 +1134,12 @@ static int spi_geni_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> +	ret = devm_add_action_or_reset(dev, spi_geni_release_dma_chan, spi);

This should be mas, not spi.

Doesn't looks like this was tested. Please correct me if I'm wrong.


-- 
With best wishes
Dmitry

Re: [PATCH v3 3/3] spi: geni-qcom: Use devm functions to simplify code

[Jinjie Ruan]

On 2024/9/9 17:49, Dmitry Baryshkov wrote:
> On Mon, Sep 09, 2024 at 03:31:41PM GMT, Jinjie Ruan wrote:
>> Use devm_pm_runtime_enable(), devm_request_irq() and
>> devm_spi_register_controller() to simplify code.
>>
>> And also register a callback spi_geni_release_dma_chan() with
>> devm_add_action_or_reset(), to release dma channel in both error
>> and device detach path, which can make sure the release sequence is
>> consistent with the original one.
>>
>> 1. Unregister spi controller.
>> 2. Free the IRQ.
>> 3. Free DMA chans
>> 4. Disable runtime PM.
>>
>> So the remove function can also be removed.
>>
>> Suggested-by: Doug Anderson <dianders@chromium.org>
>> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
>> ---
>> v3:
>> - Land the rest of the cleanups afterwards.
>> ---
>>  drivers/spi/spi-geni-qcom.c | 37 +++++++++++++------------------------
>>  1 file changed, 13 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
>> index 6f4057330444..8b0039d14605 100644
>> --- a/drivers/spi/spi-geni-qcom.c
>> +++ b/drivers/spi/spi-geni-qcom.c
>> @@ -632,8 +632,10 @@ static int spi_geni_grab_gpi_chan(struct spi_geni_master *mas)
>>  	return ret;
>>  }
>>  
>> -static void spi_geni_release_dma_chan(struct spi_geni_master *mas)
>> +static void spi_geni_release_dma_chan(void *data)
>>  {
>> +	struct spi_geni_master *mas = data;
>> +
>>  	if (mas->rx) {
>>  		dma_release_channel(mas->rx);
>>  		mas->rx = NULL;
>> @@ -1132,6 +1134,12 @@ static int spi_geni_probe(struct platform_device *pdev)
>>  	if (ret)
>>  		return ret;
>>  
>> +	ret = devm_add_action_or_reset(dev, spi_geni_release_dma_chan, spi);
> 
> This should be mas, not spi.
> 
> Doesn't looks like this was tested. Please correct me if I'm wrong.

Yes, you are right, the data should be struct spi_geni_master, which is
mas. Sorry, only compile passed.

> 
> 

Re: [PATCH v3 3/3] spi: geni-qcom: Use devm functions to simplify code

[Dmitry Baryshkov]

On Mon, 9 Sept 2024 at 14:46, Jinjie Ruan <ruanjinjie@huawei.com> wrote:
>
>
>
> On 2024/9/9 17:49, Dmitry Baryshkov wrote:
> > On Mon, Sep 09, 2024 at 03:31:41PM GMT, Jinjie Ruan wrote:
> >> Use devm_pm_runtime_enable(), devm_request_irq() and
> >> devm_spi_register_controller() to simplify code.
> >>
> >> And also register a callback spi_geni_release_dma_chan() with
> >> devm_add_action_or_reset(), to release dma channel in both error
> >> and device detach path, which can make sure the release sequence is
> >> consistent with the original one.
> >>
> >> 1. Unregister spi controller.
> >> 2. Free the IRQ.
> >> 3. Free DMA chans
> >> 4. Disable runtime PM.
> >>
> >> So the remove function can also be removed.
> >>
> >> Suggested-by: Doug Anderson <dianders@chromium.org>
> >> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
> >> ---
> >> v3:
> >> - Land the rest of the cleanups afterwards.
> >> ---
> >>  drivers/spi/spi-geni-qcom.c | 37 +++++++++++++------------------------
> >>  1 file changed, 13 insertions(+), 24 deletions(-)
> >>
> >> diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
> >> index 6f4057330444..8b0039d14605 100644
> >> --- a/drivers/spi/spi-geni-qcom.c
> >> +++ b/drivers/spi/spi-geni-qcom.c
> >> @@ -632,8 +632,10 @@ static int spi_geni_grab_gpi_chan(struct spi_geni_master *mas)
> >>      return ret;
> >>  }
> >>
> >> -static void spi_geni_release_dma_chan(struct spi_geni_master *mas)
> >> +static void spi_geni_release_dma_chan(void *data)
> >>  {
> >> +    struct spi_geni_master *mas = data;
> >> +
> >>      if (mas->rx) {
> >>              dma_release_channel(mas->rx);
> >>              mas->rx = NULL;
> >> @@ -1132,6 +1134,12 @@ static int spi_geni_probe(struct platform_device *pdev)
> >>      if (ret)
> >>              return ret;
> >>
> >> +    ret = devm_add_action_or_reset(dev, spi_geni_release_dma_chan, spi);
> >
> > This should be mas, not spi.
> >
> > Doesn't looks like this was tested. Please correct me if I'm wrong.
>
> Yes, you are right, the data should be struct spi_geni_master, which is
> mas. Sorry, only compile passed.

Please perform a runtime test or mention it in the cover letter that
it was only compile-tested.



-- 
With best wishes
Dmitry

Re: [PATCH v3 3/3] spi: geni-qcom: Use devm functions to simplify code

[Jinjie Ruan]

On 2024/9/9 19:51, Dmitry Baryshkov wrote:
> On Mon, 9 Sept 2024 at 14:46, Jinjie Ruan <ruanjinjie@huawei.com> wrote:
>>
>>
>>
>> On 2024/9/9 17:49, Dmitry Baryshkov wrote:
>>> On Mon, Sep 09, 2024 at 03:31:41PM GMT, Jinjie Ruan wrote:
>>>> Use devm_pm_runtime_enable(), devm_request_irq() and
>>>> devm_spi_register_controller() to simplify code.
>>>>
>>>> And also register a callback spi_geni_release_dma_chan() with
>>>> devm_add_action_or_reset(), to release dma channel in both error
>>>> and device detach path, which can make sure the release sequence is
>>>> consistent with the original one.
>>>>
>>>> 1. Unregister spi controller.
>>>> 2. Free the IRQ.
>>>> 3. Free DMA chans
>>>> 4. Disable runtime PM.
>>>>
>>>> So the remove function can also be removed.
>>>>
>>>> Suggested-by: Doug Anderson <dianders@chromium.org>
>>>> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
>>>> ---
>>>> v3:
>>>> - Land the rest of the cleanups afterwards.
>>>> ---
>>>>  drivers/spi/spi-geni-qcom.c | 37 +++++++++++++------------------------
>>>>  1 file changed, 13 insertions(+), 24 deletions(-)
>>>>
>>>> diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
>>>> index 6f4057330444..8b0039d14605 100644
>>>> --- a/drivers/spi/spi-geni-qcom.c
>>>> +++ b/drivers/spi/spi-geni-qcom.c
>>>> @@ -632,8 +632,10 @@ static int spi_geni_grab_gpi_chan(struct spi_geni_master *mas)
>>>>      return ret;
>>>>  }
>>>>
>>>> -static void spi_geni_release_dma_chan(struct spi_geni_master *mas)
>>>> +static void spi_geni_release_dma_chan(void *data)
>>>>  {
>>>> +    struct spi_geni_master *mas = data;
>>>> +
>>>>      if (mas->rx) {
>>>>              dma_release_channel(mas->rx);
>>>>              mas->rx = NULL;
>>>> @@ -1132,6 +1134,12 @@ static int spi_geni_probe(struct platform_device *pdev)
>>>>      if (ret)
>>>>              return ret;
>>>>
>>>> +    ret = devm_add_action_or_reset(dev, spi_geni_release_dma_chan, spi);
>>>
>>> This should be mas, not spi.
>>>
>>> Doesn't looks like this was tested. Please correct me if I'm wrong.
>>
>> Yes, you are right, the data should be struct spi_geni_master, which is
>> mas. Sorry, only compile passed.
> 
> Please perform a runtime test or mention it in the cover letter that
> it was only compile-tested.

Thank you! I'll add this information next version.

> 
> 
> 

Re: (subset) [PATCH v3 0/3] spi: geni-qcom: Undo runtime PM changes at driver exit time

[Mark Brown]

On Mon, 09 Sep 2024 15:31:38 +0800, Jinjie Ruan wrote:
> Fix two bugs for geni-qcom and use dev managed function
> to simplify code.
> 
> Changes in v3:
> - Adjust the runtime PM patch to be the first.
> - Use devm_pm_runtime_enable() to undo runtime PM changes.
> - Land the rest of the cleanups afterwards.
> - Update the commit message.
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next

Thanks!

[1/3] spi: geni-qcom: Undo runtime PM changes at driver exit time
      commit: 89e362c883c65ff94b76b9862285f63545fb5274
[2/3] spi: geni-qcom: Fix incorrect free_irq() sequence
      commit: b787a33864121a565aeb0e88561bf6062a19f99c

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark