[PATCH 1/2] genirq: irq_pipeline: Don't run handlers of disabled lines on replay

[PATCH 1/2] genirq: irq_pipeline: Don't run handlers of disabled lines on replay

[Richard Weinberger]

The pipelined handle_level_irq() function only checks unsing irq_can_handle()
on the IRQ_FLOW_START path, assuming that an event which reaches
the in-band replay path already passed this check when it was deferred
at pipeline entry.
This assumption does not hold for interrupts demultiplexed by an
in-band parent action handler.

This was observed on qcom hardware, the qcom-ipcc summary interrupt is
an in-band line, hence its demux handler runs from the replay path.
qcom_glink_smem requests its child IPCC interrupt with IRQF_NO_AUTOEN
and only enables it once smem->glink has been assigned, but the demux
delivered the interrupt early, dereferencing the still-NULL pointer:

Unable to handle kernel NULL pointer dereference at 00000000000000a0
Call trace:
_raw_spin_lock_irqsave+0x40/0x9c
__wake_up+0x28/0x70
qcom_glink_native_rx+0x58/0x6a4
qcom_glink_smem_intr+0x14/0x24
__handle_irq_event_percpu+0x4c/0x1c8
handle_irq_event+0x54/0x11c
handle_level_irq+0x150/0x24c
generic_handle_irq+0x24/0x30
qcom_ipcc_irq_fn+0x5c/0xb4
__handle_irq_event_percpu+0x4c/0x1c8
handle_irq_event+0x54/0x11c
handle_fasteoi_irq+0x164/0x278
arch_do_IRQ_pipelined+0x44/0x6c
sync_current_irq_stage+0x150/0x1e0

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 kernel/irq/chip.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
index 562cc856a3871..1b33a9599c46d 100644
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -948,6 +948,11 @@ void handle_level_irq(struct irq_desc *desc)
 		return;
 	}
 
+	if (!irq_can_handle_actions(desc)) {
+		mask_irq(desc);
+		return;
+	}
+
 	kstat_incr_irqs_this_cpu(desc);
 	handle_irq_event(desc);
 
-- 
2.51.0

[PATCH 2/2] mailbox: qcom-ipcc: Mark it as irq pipeline safe

[Richard Weinberger]

With the summary IRQ OOB capable, child IRQs can also
run OOB and the driver is irq  pipeline safe.

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 drivers/mailbox/qcom-ipcc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/mailbox/qcom-ipcc.c b/drivers/mailbox/qcom-ipcc.c
index d957d989c0cee..c316d8b4c03c4 100644
--- a/drivers/mailbox/qcom-ipcc.c
+++ b/drivers/mailbox/qcom-ipcc.c
@@ -110,7 +110,7 @@ static struct irq_chip qcom_ipcc_irq_chip = {
 	.name = "ipcc",
 	.irq_mask = qcom_ipcc_mask_irq,
 	.irq_unmask = qcom_ipcc_unmask_irq,
-	.flags = IRQCHIP_SKIP_SET_WAKE,
+	.flags = IRQCHIP_SKIP_SET_WAKE | IRQCHIP_PIPELINE_SAFE,
 };
 
 static int qcom_ipcc_domain_map(struct irq_domain *d, unsigned int irq,
@@ -322,7 +322,7 @@ static int qcom_ipcc_probe(struct platform_device *pdev)
 
 	ret = devm_request_irq(&pdev->dev, ipcc->irq, qcom_ipcc_irq_fn,
 			       IRQF_TRIGGER_HIGH | IRQF_NO_SUSPEND |
-			       IRQF_NO_THREAD, name, ipcc);
+			       IRQF_NO_THREAD | IRQF_OOB, name, ipcc);
 	if (ret < 0) {
 		dev_err(&pdev->dev, "Failed to register the irq: %d\n", ret);
 		goto err_req_irq;
-- 
2.51.0

Re: [PATCH 1/2] genirq: irq_pipeline: Don't run handlers of disabled lines on replay

[Philippe Gerum]

Hi Richard,

Richard Weinberger <richard@nod.at> writes:

> The pipelined handle_level_irq() function only checks unsing irq_can_handle()
> on the IRQ_FLOW_START path, assuming that an event which reaches
> the in-band replay path already passed this check when it was deferred
> at pipeline entry.
> This assumption does not hold for interrupts demultiplexed by an
> in-band parent action handler.
>
> This was observed on qcom hardware, the qcom-ipcc summary interrupt is
> an in-band line, hence its demux handler runs from the replay path.
> qcom_glink_smem requests its child IPCC interrupt with IRQF_NO_AUTOEN
> and only enables it once smem->glink has been assigned, but the demux
> delivered the interrupt early, dereferencing the still-NULL pointer:
>

Nice catch, merged, thanks.

-- 
Philippe.

Re: [PATCH 2/2] mailbox: qcom-ipcc: Mark it as irq pipeline safe

[Philippe Gerum]

Richard Weinberger <richard@nod.at> writes:

> With the summary IRQ OOB capable, child IRQs can also
> run OOB and the driver is irq  pipeline safe.
>
> Signed-off-by: Richard Weinberger <richard@nod.at>
> ---
>  drivers/mailbox/qcom-ipcc.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mailbox/qcom-ipcc.c b/drivers/mailbox/qcom-ipcc.c
> index d957d989c0cee..c316d8b4c03c4 100644
> --- a/drivers/mailbox/qcom-ipcc.c
> +++ b/drivers/mailbox/qcom-ipcc.c
> @@ -110,7 +110,7 @@ static struct irq_chip qcom_ipcc_irq_chip = {
>  	.name = "ipcc",
>  	.irq_mask = qcom_ipcc_mask_irq,
>  	.irq_unmask = qcom_ipcc_unmask_irq,
> -	.flags = IRQCHIP_SKIP_SET_WAKE,
> +	.flags = IRQCHIP_SKIP_SET_WAKE | IRQCHIP_PIPELINE_SAFE,
>  };
>  
>  static int qcom_ipcc_domain_map(struct irq_domain *d, unsigned int irq,
> @@ -322,7 +322,7 @@ static int qcom_ipcc_probe(struct platform_device *pdev)
>  
>  	ret = devm_request_irq(&pdev->dev, ipcc->irq, qcom_ipcc_irq_fn,
>  			       IRQF_TRIGGER_HIGH | IRQF_NO_SUSPEND |
> -			       IRQF_NO_THREAD, name, ipcc);
> +			       IRQF_NO_THREAD | IRQF_OOB, name, ipcc);
>  	if (ret < 0) {
>  		dev_err(&pdev->dev, "Failed to register the irq: %d\n", ret);
>  		goto err_req_irq;

Merged, thanks.

-- 
Philippe.