diff mbox series

spi: mediatek: Enable irq before the spi registration

Message ID 20221225-mtk-spi-fixes-v1-0-bb6c14c232f8@chromium.org
State New
Headers
Series spi: mediatek: Enable irq before the spi registration |

Commit Message

Ricardo Ribalda Dec. 25, 2022, 8:37 a.m. UTC 
  If the irq is enabled after the spi si registered, there can be a race
with the initialization of the devices on the spi bus.

Eg:
mtk-spi 1100a000.spi: spi-mem transfer timeout
spi-nor: probe of spi0.0 failed with error -110
Unable to handle kernel NULL pointer dereference at virtual address
0000000000000010
...
Call trace:
 mtk_spi_can_dma+0x0/0x2c

Fixes: c6f7874687f7 ("spi: mediatek: Enable irq when pdata is ready")
Reported-by: Daniel Golle <daniel@makrotopia.org>
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
spi: mediatek: Fix init order (again)

Hi Mark

Here is a fixup of the previous patch. Daniel, can you confirm that it
works on your hardware? 

Thanks and sorry for annoyance. 

To: Mark Brown <broonie@kernel.org>
To: Matthias Brugger <matthias.bgg@gmail.com>
To: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Cc: linux-spi@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-mediatek@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
Cc: Daniel Golle <daniel@makrotopia.org>
---
 drivers/spi/spi-mt65xx.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)


---
base-commit: 45b3cd900bf8d1a3cd7cf48361df8c09ae5b4009
change-id: 20221225-mtk-spi-fixes-99c863a6fdf1

Best regards,

Comments

Daniel Golle Dec. 25, 2022, 2:06 p.m. UTC | #1 
On Sun, Dec 25, 2022 at 09:37:12AM +0100, Ricardo Ribalda wrote:
> If the irq is enabled after the spi si registered, there can be a race
> with the initialization of the devices on the spi bus.
> 
> Eg:
> mtk-spi 1100a000.spi: spi-mem transfer timeout
> spi-nor: probe of spi0.0 failed with error -110
> Unable to handle kernel NULL pointer dereference at virtual address
> 0000000000000010
> ...
> Call trace:
>  mtk_spi_can_dma+0x0/0x2c
> 
> Fixes: c6f7874687f7 ("spi: mediatek: Enable irq when pdata is ready")
> Reported-by: Daniel Golle <daniel@makrotopia.org>
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
> spi: mediatek: Fix init order (again)
> 
> Hi Mark
> 
> Here is a fixup of the previous patch. Daniel, can you confirm that it
> works on your hardware? 

Yes, this fixes the issue and SPI now works fine on MT7986 with the
patch applied.

Tested-by: Daniel Golle <daniel@makrotopia.org>

> 
> Thanks and sorry for annoyance. 
> 
> To: Mark Brown <broonie@kernel.org>
> To: Matthias Brugger <matthias.bgg@gmail.com>
> To: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> Cc: linux-spi@vger.kernel.org
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-mediatek@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org
> Cc: Daniel Golle <daniel@makrotopia.org>
> ---
>  drivers/spi/spi-mt65xx.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/spi/spi-mt65xx.c b/drivers/spi/spi-mt65xx.c
> index 6de8360e5c2a..9eab6c20dbc5 100644
> --- a/drivers/spi/spi-mt65xx.c
> +++ b/drivers/spi/spi-mt65xx.c
> @@ -1253,6 +1253,11 @@ static int mtk_spi_probe(struct platform_device *pdev)
>  		dev_notice(dev, "SPI dma_set_mask(%d) failed, ret:%d\n",
>  			   addr_bits, ret);
>  
> +	ret = devm_request_irq(dev, irq, mtk_spi_interrupt,
> +			       IRQF_TRIGGER_NONE, dev_name(dev), master);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to register irq\n");
> +
>  	pm_runtime_enable(dev);
>  
>  	ret = devm_spi_register_master(dev, master);
> @@ -1261,13 +1266,6 @@ static int mtk_spi_probe(struct platform_device *pdev)
>  		return dev_err_probe(dev, ret, "failed to register master\n");
>  	}
>  
> -	ret = devm_request_irq(dev, irq, mtk_spi_interrupt,
> -			       IRQF_TRIGGER_NONE, dev_name(dev), master);
> -	if (ret) {
> -		pm_runtime_disable(dev);
> -		return dev_err_probe(dev, ret, "failed to register irq\n");
> -	}
> -
>  	return 0;
>  }
>  
> 
> ---
> base-commit: 45b3cd900bf8d1a3cd7cf48361df8c09ae5b4009
> change-id: 20221225-mtk-spi-fixes-99c863a6fdf1
> 
> Best regards,
> -- 
> Ricardo Ribalda <ribalda@chromium.org>
Mark Brown Dec. 27, 2022, 11:57 a.m. UTC | #2 
On Sun, 25 Dec 2022 09:37:12 +0100, Ricardo Ribalda wrote:
> If the irq is enabled after the spi si registered, there can be a race
> with the initialization of the devices on the spi bus.
> 
> Eg:
> mtk-spi 1100a000.spi: spi-mem transfer timeout
> spi-nor: probe of spi0.0 failed with error -110
> Unable to handle kernel NULL pointer dereference at virtual address
> 0000000000000010
> ...
> Call trace:
>  mtk_spi_can_dma+0x0/0x2c
> 
> [...]

Applied to

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

Thanks!

[1/1] spi: mediatek: Enable irq before the spi registration
      commit: b24cded8c065d7cef8690b2c7b82b828cce57708

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
Mark Brown Dec. 27, 2022, 1:43 p.m. UTC | #3 
On Sun, 25 Dec 2022 09:37:12 +0100, Ricardo Ribalda wrote:
> If the irq is enabled after the spi si registered, there can be a race
> with the initialization of the devices on the spi bus.
> 
> Eg:
> mtk-spi 1100a000.spi: spi-mem transfer timeout
> spi-nor: probe of spi0.0 failed with error -110
> Unable to handle kernel NULL pointer dereference at virtual address
> 0000000000000010
> ...
> Call trace:
>  mtk_spi_can_dma+0x0/0x2c
> 
> [...]

Applied to

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

Thanks!

[1/1] spi: mediatek: Enable irq before the spi registration
      commit: b24cded8c065d7cef8690b2c7b82b828cce57708

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
Linux regression tracking (Thorsten Leemhuis) Jan. 4, 2023, 2:08 p.m. UTC | #4 
Hi Mark!

On 27.12.22 14:43, Mark Brown wrote:
> On Sun, 25 Dec 2022 09:37:12 +0100, Ricardo Ribalda wrote:
>> If the irq is enabled after the spi si registered, there can be a race
>> with the initialization of the devices on the spi bus.
>>
>> Eg:
>> mtk-spi 1100a000.spi: spi-mem transfer timeout
>> spi-nor: probe of spi0.0 failed with error -110
>> Unable to handle kernel NULL pointer dereference at virtual address
>> 0000000000000010
>> ...
>> Call trace:
>>  mtk_spi_can_dma+0x0/0x2c
>>
>> [...]
> 
> Applied to
> 
>    https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next
> 
> Thanks!
> 
> [1/1] spi: mediatek: Enable irq before the spi registration
>       commit: b24cded8c065d7cef8690b2c7b82b828cce57708
> 
> 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.
> [...]

Quick question: why did you queue this for the next merge window? This
change *afaics* is fixing a reported regression (a kernel oops)
introduced this cycle:
https://lore.kernel.org/lkml/Y6dL2ZWgd1BD6kew@makrotopia.org/

Or have I missed or confused something?

Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
--
Everything you wanna know about Linux kernel regression tracking:
https://linux-regtracking.leemhuis.info/about/#tldr
That page also explains what to do if mails like this annoy you.

#regzbot poke
Mark Brown Jan. 4, 2023, 2:35 p.m. UTC | #5 
On Wed, Jan 04, 2023 at 03:08:51PM +0100, Thorsten Leemhuis wrote:
> On 27.12.22 14:43, Mark Brown wrote:

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

> Quick question: why did you queue this for the next merge window? This
> change *afaics* is fixing a reported regression (a kernel oops)
> introduced this cycle:
> https://lore.kernel.org/lkml/Y6dL2ZWgd1BD6kew@makrotopia.org/

> Or have I missed or confused something?

You've missed something, it's queued for 6.2.
diff mbox series

Patch

diff --git a/drivers/spi/spi-mt65xx.c b/drivers/spi/spi-mt65xx.c
index 6de8360e5c2a..9eab6c20dbc5 100644
--- a/drivers/spi/spi-mt65xx.c
+++ b/drivers/spi/spi-mt65xx.c
@@ -1253,6 +1253,11 @@  static int mtk_spi_probe(struct platform_device *pdev)
 		dev_notice(dev, "SPI dma_set_mask(%d) failed, ret:%d\n",
 			   addr_bits, ret);
 
+	ret = devm_request_irq(dev, irq, mtk_spi_interrupt,
+			       IRQF_TRIGGER_NONE, dev_name(dev), master);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to register irq\n");
+
 	pm_runtime_enable(dev);
 
 	ret = devm_spi_register_master(dev, master);
@@ -1261,13 +1266,6 @@  static int mtk_spi_probe(struct platform_device *pdev)
 		return dev_err_probe(dev, ret, "failed to register master\n");
 	}
 
-	ret = devm_request_irq(dev, irq, mtk_spi_interrupt,
-			       IRQF_TRIGGER_NONE, dev_name(dev), master);
-	if (ret) {
-		pm_runtime_disable(dev);
-		return dev_err_probe(dev, ret, "failed to register irq\n");
-	}
-
 	return 0;
 }