[PATCH 0/2] drm/vc4: Correctly uninstall interrupts

[PATCH 0/2] drm/vc4: Correctly uninstall interrupts

[Stefan Schake]

This set of patches fixes issues with vc4_irq_uninstall.
The first patch fixes a NULL pointer dereference when the binner BO
would disappear during an in flight overflow mem work callback.

The second patch ensures we return with all interrupts disabled. This was
suspected to cause the NULL dereference but turned out to be unrelated.

Tested with a Raspberry Pi CM 3 that was previously stuck in a boot loop
due to the issue. With the patch applied, the NULL dereference was no
longer observed through numerous resets.

Stefan Schake (2):
  drm/vc4: Account for interrupts in flight
  drm/vc4: Ensure interrupts are disabled

 drivers/gpu/drm/vc4/vc4_irq.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

-- 
1.9.1

[PATCH 1/2] drm/vc4: Account for interrupts in flight

[Stefan Schake]

Synchronously disable the IRQ to make the following cancel_work_sync
invocation effective.

An interrupt in flight could enqueue further overflow mem work. As we
free the binner BO immediately following vc4_irq_uninstall this caused
a NULL pointer dereference in the work callback vc4_overflow_mem_work.

Link: https://github.com/anholt/linux/issues/114
Signed-off-by: Stefan Schake <stschake@gmail.com>
Fixes: d5b1a78a772f ("drm/vc4: Add support for drawing 3D frames.")
---
 drivers/gpu/drm/vc4/vc4_irq.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/vc4/vc4_irq.c b/drivers/gpu/drm/vc4/vc4_irq.c
index 7d7af3a..61b2e53 100644
--- a/drivers/gpu/drm/vc4/vc4_irq.c
+++ b/drivers/gpu/drm/vc4/vc4_irq.c
@@ -208,6 +208,9 @@
 {
 	struct vc4_dev *vc4 = to_vc4_dev(dev);
 
+	/* Undo the effects of a previous vc4_irq_uninstall. */
+	enable_irq(dev->irq);
+
 	/* Enable both the render done and out of memory interrupts. */
 	V3D_WRITE(V3D_INTENA, V3D_DRIVER_IRQS);
 
@@ -225,6 +228,9 @@
 	/* Clear any pending interrupts we might have left. */
 	V3D_WRITE(V3D_INTCTL, V3D_DRIVER_IRQS);
 
+	/* Finish any interrupt handler still in flight. */
+	disable_irq(dev->irq);
+
 	cancel_work_sync(&vc4->overflow_mem_work);
 }
 
-- 
1.9.1

[PATCH 2/2] drm/vc4: Ensure interrupts are disabled

[Stefan Schake]

The overflow mem work callback vc4_overflow_mem_work reenables its
associated interrupt upon completion. To ensure all interrupts are disabled
when we return from vc4_irq_uninstall, we need to disable it again if
cancel_work_sync indicated pending work.

Signed-off-by: Stefan Schake <stschake@gmail.com>
---
 drivers/gpu/drm/vc4/vc4_irq.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/vc4/vc4_irq.c b/drivers/gpu/drm/vc4/vc4_irq.c
index 61b2e53..7d780149d 100644
--- a/drivers/gpu/drm/vc4/vc4_irq.c
+++ b/drivers/gpu/drm/vc4/vc4_irq.c
@@ -231,7 +231,14 @@
 	/* Finish any interrupt handler still in flight. */
 	disable_irq(dev->irq);
 
-	cancel_work_sync(&vc4->overflow_mem_work);
+	if (cancel_work_sync(&vc4->overflow_mem_work)) {
+		/*
+		 * Work was still pending. The overflow mem work's
+		 * callback reenables the OUTOMEM interrupt upon
+		 * completion, so ensure it is disabled here.
+		 */
+		V3D_WRITE(V3D_INTDIS, V3D_INT_OUTOMEM);
+	}
 }
 
 /** Reinitializes interrupt registers when a GPU reset is performed. */
-- 
1.9.1

Re: [PATCH 2/2] drm/vc4: Ensure interrupts are disabled

[Eric Anholt]

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

Stefan Schake <stschake@gmail.com> writes:

> The overflow mem work callback vc4_overflow_mem_work reenables its
> associated interrupt upon completion. To ensure all interrupts are disabled
> when we return from vc4_irq_uninstall, we need to disable it again if
> cancel_work_sync indicated pending work.

Is there a reason we need the interrupts disabled at the V3D level while
we have the IRQ disabled at the irqchip level?  Once we re-enable at the
irqchip, we immediately V3D_WRITE(V3D_INTENA, V3D_DRIVER_IRQS) anyway.

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

Re: [PATCH 1/2] drm/vc4: Account for interrupts in flight

[Eric Anholt]

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

Stefan Schake <stschake@gmail.com> writes:

> Synchronously disable the IRQ to make the following cancel_work_sync
> invocation effective.
>
> An interrupt in flight could enqueue further overflow mem work. As we
> free the binner BO immediately following vc4_irq_uninstall this caused
> a NULL pointer dereference in the work callback vc4_overflow_mem_work.
>
> Link: https://github.com/anholt/linux/issues/114
> Signed-off-by: Stefan Schake <stschake@gmail.com>
> Fixes: d5b1a78a772f ("drm/vc4: Add support for drawing 3D frames.")

Reviewed and applied this one.  Thanks!

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

Re: [PATCH 2/2] drm/vc4: Ensure interrupts are disabled

[Stefan Schake]

On Tue, Nov 14, 2017 at 1:18 AM, Eric Anholt <eric@anholt.net> wrote:
> Stefan Schake <stschake@gmail.com> writes:
>
>> The overflow mem work callback vc4_overflow_mem_work reenables its
>> associated interrupt upon completion. To ensure all interrupts are disabled
>> when we return from vc4_irq_uninstall, we need to disable it again if
>> cancel_work_sync indicated pending work.
>
> Is there a reason we need the interrupts disabled at the V3D level while
> we have the IRQ disabled at the irqchip level?  Once we re-enable at the
> irqchip, we immediately V3D_WRITE(V3D_INTENA, V3D_DRIVER_IRQS) anyway.

irqchip will mask it in the ARM interrupt controller, so we will certainly never
see an interrupt. I'm not sure on the exact guarantees V3D_INTENA and
V3D_INTCTL make - does the state in INTENA affect if V3D will signal an
interrupt in INTCTL? We're not currently clearing the latter in postinstall.

>From a practical perspective, we're not doing anything in between uninstall
and postinstall that would trigger the interrupt. So in that sense
it's certainly
superfluous.

Re: [PATCH 2/2] drm/vc4: Ensure interrupts are disabled

[Eric Anholt]

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

Stefan Schake <stschake@gmail.com> writes:

> On Tue, Nov 14, 2017 at 1:18 AM, Eric Anholt <eric@anholt.net> wrote:
>> Stefan Schake <stschake@gmail.com> writes:
>>
>>> The overflow mem work callback vc4_overflow_mem_work reenables its
>>> associated interrupt upon completion. To ensure all interrupts are disabled
>>> when we return from vc4_irq_uninstall, we need to disable it again if
>>> cancel_work_sync indicated pending work.
>>
>> Is there a reason we need the interrupts disabled at the V3D level while
>> we have the IRQ disabled at the irqchip level?  Once we re-enable at the
>> irqchip, we immediately V3D_WRITE(V3D_INTENA, V3D_DRIVER_IRQS) anyway.
>
> irqchip will mask it in the ARM interrupt controller, so we will certainly never
> see an interrupt. I'm not sure on the exact guarantees V3D_INTENA and
> V3D_INTCTL make - does the state in INTENA affect if V3D will signal an
> interrupt in INTCTL? We're not currently clearing the latter in postinstall.

INTENA/INTDIS writes update the state of the single register that
controls which bits of INTCTL get ORed together to raise the interrupt
outside the V3D block.

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

Re: [PATCH 2/2] drm/vc4: Ensure interrupts are disabled

[Stefan Schake]

On Tue, Nov 14, 2017 at 8:44 PM, Eric Anholt <eric@anholt.net> wrote:
> Stefan Schake <stschake@gmail.com> writes:
>
>> On Tue, Nov 14, 2017 at 1:18 AM, Eric Anholt <eric@anholt.net> wrote:
>>> Stefan Schake <stschake@gmail.com> writes:
>>>
>>>> The overflow mem work callback vc4_overflow_mem_work reenables its
>>>> associated interrupt upon completion. To ensure all interrupts are disabled
>>>> when we return from vc4_irq_uninstall, we need to disable it again if
>>>> cancel_work_sync indicated pending work.
>>>
>>> Is there a reason we need the interrupts disabled at the V3D level while
>>> we have the IRQ disabled at the irqchip level?  Once we re-enable at the
>>> irqchip, we immediately V3D_WRITE(V3D_INTENA, V3D_DRIVER_IRQS) anyway.
>>
>> irqchip will mask it in the ARM interrupt controller, so we will certainly never
>> see an interrupt. I'm not sure on the exact guarantees V3D_INTENA and
>> V3D_INTCTL make - does the state in INTENA affect if V3D will signal an
>> interrupt in INTCTL? We're not currently clearing the latter in postinstall.
>
> INTENA/INTDIS writes update the state of the single register that
> controls which bits of INTCTL get ORed together to raise the interrupt
> outside the V3D block.

Then I certainly agree - this patch doesn't do anything and should be
dropped. Good call!

Thanks,
Stefan