[PATCH] drivers: spi: sunxi: Enable irq after the initialization is done

[PATCH] drivers: spi: sunxi: Enable irq after the initialization is done

[qianfanguijin]

From: qianfan Zhao <qianfanguijin@163.com>

Interrupts are opened prematurely before some initialization work is finished.
When some uninitialized variables are used in interrupt functions,
the kernel crashes.

A typical case is the second kernel loaded with kdump starting when the first
kernel transfering spi messages.

The kernel error messages is as follows:

[    1.362449] sun6i-spi 1c06000.spi: Failed to request RX DMA channel
[    1.369654] 8<--- cut here ---
[    1.372716] Unable to handle kernel paging request at virtual address fffffffc
[    1.379928] pgd = (ptrval)
[    1.382632] [fffffffc] *pgd=6bef6861, *pte=00000000, *ppte=00000000
[    1.388907] Internal error: Oops: 37 [#1] SMP ARM
...
[    1.784024] [<c0159c54>] (swake_up_locked.part.0) from [<c0159d9c>] (complete+0x30/0x40)
[    1.792114] [<c0159d9c>] (complete) from [<c0491ab0>] (sun6i_spi_handler+0x168/0x194)
[    1.799947] [<c0491ab0>] (sun6i_spi_handler) from [<c0169070>] (__handle_irq_event_percpu+0x50/0x120)
[    1.809168] [<c0169070>] (__handle_irq_event_percpu) from [<c0169170>] (handle_irq_event_percpu+0x30/0x78)
[    1.818818] [<c0169170>] (handle_irq_event_percpu) from [<c01691fc>] (handle_irq_event+0x44/0x68)
[    1.827687] [<c01691fc>] (handle_irq_event) from [<c016d96c>] (handle_fasteoi_irq+0xac/0x118)
[    1.836210] [<c016d96c>] (handle_fasteoi_irq) from [<c01689bc>] (handle_domain_irq+0x5c/0x78)
[    1.844731] [<c01689bc>] (handle_domain_irq) from [<c03bc058>] (gic_handle_irq+0x7c/0x90)
[    1.852910] [<c03bc058>] (gic_handle_irq) from [<c0100afc>] (__irq_svc+0x5c/0x78)

Signed-off-by: qianfan Zhao <qianfanguijin@163.com>
---
 drivers/spi/spi-sun4i.c | 4 +++-
 drivers/spi/spi-sun6i.c | 4 +++-
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi-sun4i.c b/drivers/spi/spi-sun4i.c
index 1fdfc6e6691d..2f797b903692 100644
--- a/drivers/spi/spi-sun4i.c
+++ b/drivers/spi/spi-sun4i.c
@@ -452,7 +452,7 @@ static int sun4i_spi_probe(struct platform_device *pdev)
 	}
 
 	ret = devm_request_irq(&pdev->dev, irq, sun4i_spi_handler,
-			       0, "sun4i-spi", sspi);
+			       IRQF_NO_AUTOEN, "sun4i-spi", sspi);
 	if (ret) {
 		dev_err(&pdev->dev, "Cannot request IRQ\n");
 		goto err_free_master;
@@ -506,6 +506,8 @@ static int sun4i_spi_probe(struct platform_device *pdev)
 		goto err_pm_disable;
 	}
 
+	enable_irq(irq);
+
 	return 0;
 
 err_pm_disable:
diff --git a/drivers/spi/spi-sun6i.c b/drivers/spi/spi-sun6i.c
index 1151d8592656..0eec2d0e312b 100644
--- a/drivers/spi/spi-sun6i.c
+++ b/drivers/spi/spi-sun6i.c
@@ -583,7 +583,7 @@ static int sun6i_spi_probe(struct platform_device *pdev)
 	}
 
 	ret = devm_request_irq(&pdev->dev, irq, sun6i_spi_handler,
-			       0, "sun6i-spi", sspi);
+			       IRQF_NO_AUTOEN, "sun6i-spi", sspi);
 	if (ret) {
 		dev_err(&pdev->dev, "Cannot request IRQ\n");
 		goto err_free_master;
@@ -675,6 +675,8 @@ static int sun6i_spi_probe(struct platform_device *pdev)
 		goto err_pm_disable;
 	}
 
+	enable_irq(irq);
+
 	return 0;
 
 err_pm_disable:
-- 
2.25.1

Re: [PATCH] drivers: spi: sunxi: Enable irq after the initialization is done

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1464 bytes --]

On Sun, Apr 23, 2023 at 10:30:56AM +0800, qianfanguijin@163.com wrote:

> 
> The kernel error messages is as follows:
> 
> [    1.362449] sun6i-spi 1c06000.spi: Failed to request RX DMA channel
> [    1.369654] 8<--- cut here ---
> [    1.372716] Unable to handle kernel paging request at virtual address fffffffc
> [    1.379928] pgd = (ptrval)
> [    1.382632] [fffffffc] *pgd=6bef6861, *pte=00000000, *ppte=00000000
> [    1.388907] Internal error: Oops: 37 [#1] SMP ARM
> ...
> [    1.784024] [<c0159c54>] (swake_up_locked.part.0) from [<c0159d9c>] (complete+0x30/0x40)

Please think hard before including complete backtraces in upstream
reports, they are very large and contain almost no useful information
relative to their size so often obscure the relevant content in your
message. If part of the backtrace is usefully illustrative (it often is
for search engines if nothing else) then it's usually better to pull out
the relevant sections.

>  	ret = devm_request_irq(&pdev->dev, irq, sun4i_spi_handler,
> -			       0, "sun4i-spi", sspi);
> +			       IRQF_NO_AUTOEN, "sun4i-spi", sspi);
>  	if (ret) {
>  		dev_err(&pdev->dev, "Cannot request IRQ\n");
>  		goto err_free_master;
> @@ -506,6 +506,8 @@ static int sun4i_spi_probe(struct platform_device *pdev)
>  		goto err_pm_disable;
>  	}
>  
> +	enable_irq(irq);
> +

The usual approach would be to move the requesting of the interrupt
later.  Why do this instead?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] drivers: spi: sunxi: Enable irq after the initialization is done

[qianfan]

在 2023/4/24 19:46, Mark Brown 写道:
> On Sun, Apr 23, 2023 at 10:30:56AM +0800, qianfanguijin@163.com wrote:
>
>> The kernel error messages is as follows:
>>
>> [    1.362449] sun6i-spi 1c06000.spi: Failed to request RX DMA channel
>> [    1.369654] 8<--- cut here ---
>> [    1.372716] Unable to handle kernel paging request at virtual address fffffffc
>> [    1.379928] pgd = (ptrval)
>> [    1.382632] [fffffffc] *pgd=6bef6861, *pte=00000000, *ppte=00000000
>> [    1.388907] Internal error: Oops: 37 [#1] SMP ARM
>> ...
>> [    1.784024] [<c0159c54>] (swake_up_locked.part.0) from [<c0159d9c>] (complete+0x30/0x40)
> Please think hard before including complete backtraces in upstream
> reports, they are very large and contain almost no useful information
> relative to their size so often obscure the relevant content in your
> message. If part of the backtrace is usefully illustrative (it often is
> for search engines if nothing else) then it's usually better to pull out
> the relevant sections.
Thanks and I will drop the backtrace messages.
>
>>   	ret = devm_request_irq(&pdev->dev, irq, sun4i_spi_handler,
>> -			       0, "sun4i-spi", sspi);
>> +			       IRQF_NO_AUTOEN, "sun4i-spi", sspi);
>>   	if (ret) {
>>   		dev_err(&pdev->dev, "Cannot request IRQ\n");
>>   		goto err_free_master;
>> @@ -506,6 +506,8 @@ static int sun4i_spi_probe(struct platform_device *pdev)
>>   		goto err_pm_disable;
>>   	}
>>   
>> +	enable_irq(irq);
>> +
> The usual approach would be to move the requesting of the interrupt
> later.  Why do this instead?
Nothing special, this way does not change the goto logic.

Re: [PATCH] drivers: spi: sunxi: Enable irq after the initialization is done

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 560 bytes --]

On Tue, Apr 25, 2023 at 08:22:16AM +0800, qianfan wrote:
> 在 2023/4/24 19:46, Mark Brown 写道:
> > On Sun, Apr 23, 2023 at 10:30:56AM +0800, qianfanguijin@163.com wrote:

> > > +	enable_irq(irq);

> > The usual approach would be to move the requesting of the interrupt
> > later.  Why do this instead?

> Nothing special, this way does not change the goto logic.

It would be better to restructure the code, enable_irq() is a fairly
specialist function and should only be used if it is really needed -
it's a big warning sign to see it used.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]