[PATCH] staging: imx/drm: request irq only after adding the crtc

[PATCH] staging: imx/drm: request irq only after adding the crtc

[Philipp Zabel]

If the bootloader already enabled the display, the interrupt handler
will be called as soon as it is registered. If the CRTC is not already
added at this time, the call to imx_drm_handle_vblank will result in
a NULL pointer dereference.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/staging/imx-drm/ipuv3-crtc.c |   23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/drivers/staging/imx-drm/ipuv3-crtc.c b/drivers/staging/imx-drm/ipuv3-crtc.c
index 1892006..d454a16 100644
--- a/drivers/staging/imx-drm/ipuv3-crtc.c
+++ b/drivers/staging/imx-drm/ipuv3-crtc.c
@@ -483,17 +483,6 @@ static int ipu_get_resources(struct ipu_crtc *ipu_crtc,
 		goto err_out;
 	}
 
-	ipu_crtc->irq = ipu_idmac_channel_irq(ipu, ipu_crtc->ipu_ch,
-			IPU_IRQ_EOF);
-	ret = devm_request_irq(ipu_crtc->dev, ipu_crtc->irq, ipu_irq_handler, 0,
-			"imx_drm", ipu_crtc);
-	if (ret < 0) {
-		dev_err(ipu_crtc->dev, "irq request failed with %d.\n", ret);
-		goto err_out;
-	}
-
-	disable_irq(ipu_crtc->irq);
-
 	return 0;
 err_out:
 	ipu_put_resources(ipu_crtc);
@@ -504,6 +493,7 @@ err_out:
 static int ipu_crtc_init(struct ipu_crtc *ipu_crtc,
 		struct ipu_client_platformdata *pdata)
 {
+	struct ipu_soc *ipu = dev_get_drvdata(ipu_crtc->dev->parent);
 	int ret;
 
 	ret = ipu_get_resources(ipu_crtc, pdata);
@@ -522,6 +512,17 @@ static int ipu_crtc_init(struct ipu_crtc *ipu_crtc,
 		goto err_put_resources;
 	}
 
+	ipu_crtc->irq = ipu_idmac_channel_irq(ipu, ipu_crtc->ipu_ch,
+			IPU_IRQ_EOF);
+	ret = devm_request_irq(ipu_crtc->dev, ipu_crtc->irq, ipu_irq_handler, 0,
+			"imx_drm", ipu_crtc);
+	if (ret < 0) {
+		dev_err(ipu_crtc->dev, "irq request failed with %d.\n", ret);
+		goto err_put_resources;
+	}
+
+	disable_irq(ipu_crtc->irq);
+
 	return 0;
 
 err_put_resources:
-- 
1.7.10.4

[PATCH] staging: imx/drm: request irq only after adding the crtc

[Shawn Guo]

On Tue, Feb 19, 2013 at 03:35:59PM +0100, Philipp Zabel wrote:
> If the bootloader already enabled the display, the interrupt handler
> will be called as soon as it is registered. If the CRTC is not already
> added at this time, the call to imx_drm_handle_vblank will result in
> a NULL pointer dereference.
> 
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>

Tested-by: Shawn Guo <shawn.guo@linaro.org>

Greg,

The patch fixes a kernel panic [1], which has been on linux-next since
Jan 8 [2].  Can you please apply it if it looks good to you?

Shawn

[1] http://thread.gmane.org/gmane.linux.ports.arm.kernel/218858
[2] http://thread.gmane.org/gmane.linux.ports.arm.kernel/208192

> ---
>  drivers/staging/imx-drm/ipuv3-crtc.c |   23 ++++++++++++-----------
>  1 file changed, 12 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/staging/imx-drm/ipuv3-crtc.c b/drivers/staging/imx-drm/ipuv3-crtc.c
> index 1892006..d454a16 100644
> --- a/drivers/staging/imx-drm/ipuv3-crtc.c
> +++ b/drivers/staging/imx-drm/ipuv3-crtc.c
> @@ -483,17 +483,6 @@ static int ipu_get_resources(struct ipu_crtc *ipu_crtc,
>  		goto err_out;
>  	}
>  
> -	ipu_crtc->irq = ipu_idmac_channel_irq(ipu, ipu_crtc->ipu_ch,
> -			IPU_IRQ_EOF);
> -	ret = devm_request_irq(ipu_crtc->dev, ipu_crtc->irq, ipu_irq_handler, 0,
> -			"imx_drm", ipu_crtc);
> -	if (ret < 0) {
> -		dev_err(ipu_crtc->dev, "irq request failed with %d.\n", ret);
> -		goto err_out;
> -	}
> -
> -	disable_irq(ipu_crtc->irq);
> -
>  	return 0;
>  err_out:
>  	ipu_put_resources(ipu_crtc);
> @@ -504,6 +493,7 @@ err_out:
>  static int ipu_crtc_init(struct ipu_crtc *ipu_crtc,
>  		struct ipu_client_platformdata *pdata)
>  {
> +	struct ipu_soc *ipu = dev_get_drvdata(ipu_crtc->dev->parent);
>  	int ret;
>  
>  	ret = ipu_get_resources(ipu_crtc, pdata);
> @@ -522,6 +512,17 @@ static int ipu_crtc_init(struct ipu_crtc *ipu_crtc,
>  		goto err_put_resources;
>  	}
>  
> +	ipu_crtc->irq = ipu_idmac_channel_irq(ipu, ipu_crtc->ipu_ch,
> +			IPU_IRQ_EOF);
> +	ret = devm_request_irq(ipu_crtc->dev, ipu_crtc->irq, ipu_irq_handler, 0,
> +			"imx_drm", ipu_crtc);
> +	if (ret < 0) {
> +		dev_err(ipu_crtc->dev, "irq request failed with %d.\n", ret);
> +		goto err_put_resources;
> +	}
> +
> +	disable_irq(ipu_crtc->irq);
> +
>  	return 0;
>  
>  err_put_resources:
> -- 
> 1.7.10.4
> 

[PATCH] staging: imx/drm: request irq only after adding the crtc

[Matt Sealey]

On Tue, Feb 19, 2013 at 8:35 AM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> If the bootloader already enabled the display, the interrupt handler
> will be called as soon as it is registered. If the CRTC is not already
> added at this time, the call to imx_drm_handle_vblank will result in
> a NULL pointer dereference.
>
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>

Doesn't this imply that the bootloader doesn't properly quiesce all
interrupt-causing modules and halt all dma operations?

Sean Bean said it best: One does not simply walk into Mordor!

I am not nacking the patch (since it would not change behavior at all
for those scenarios where quiescence of interrupts and dma was done
properly before the handover) but I am really unsure that this sets a
good precedent.. is this all about getting a "glitch free" splash
screen displayed between bootloader and kernel?

--
Matt Sealey <matt@genesi-usa.com>
Product Development Analyst, Genesi USA, Inc.

[PATCH] staging: imx/drm: request irq only after adding the crtc

[Lothar Waßmann]

Hi,

Matt Sealey writes:
> On Tue, Feb 19, 2013 at 8:35 AM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> > If the bootloader already enabled the display, the interrupt handler
> > will be called as soon as it is registered. If the CRTC is not already
> > added at this time, the call to imx_drm_handle_vblank will result in
> > a NULL pointer dereference.
> >
> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> 
> Doesn't this imply that the bootloader doesn't properly quiesce all
> interrupt-causing modules and halt all dma operations?
> 
> Sean Bean said it best: One does not simply walk into Mordor!
> 
> I am not nacking the patch (since it would not change behavior at all
> for those scenarios where quiescence of interrupts and dma was done
> properly before the handover) but I am really unsure that this sets a
> good precedent.. is this all about getting a "glitch free" splash
> screen displayed between bootloader and kernel?
> 
IMO its more about defensive programming. Registering an interrupt
handler only after everything that the handler needs has been
initialized is a Good Thing(TM), no matter whether the interrupt could
possibly occur or not.


Lothar Wa?mann
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstra?e 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Gesch?ftsf?hrer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info at karo-electronics.de
___________________________________________________________

[PATCH] staging: imx/drm: request irq only after adding the crtc

[Russell King - ARM Linux]

On Thu, Feb 21, 2013 at 08:10:31AM +0100, Lothar Wa?mann wrote:
> IMO its more about defensive programming. Registering an interrupt
> handler only after everything that the handler needs has been
> initialized is a Good Thing(TM), no matter whether the interrupt could
> possibly occur or not.

There are debugging modes in the kernel explicitly for this, although
wrt shared interrupts.  They explicitly trigger a call to the interrupt
handler as soon as the handler is registered.

It's there to trip up exactly these kinds of programming errors.  And
yes, it is _bad_ _practice_ to register an interrupt handler which is
not ready to be run.  You should expect the handler to be callable as
soon as it is registered.

As for ensuring that interrupts are disabled on the device before
registering the handler, that's up to the driver author to decide, but
it's also a common sense issue - if you're not ready to handle interrupts
then make sure they're not unmasked on the device.

So... sensible programming with interrupts:
1. don't register your interrupt handler before the point where it's able
   to run without causing any problems.
2. don't assume that interrupts will be masked when you're entered.

As long as we have the kexec and crashdump stuff (which effectively means
that the replacement kernel can be entered with devices in any state what
so ever), arguments around "handoff" between boot loaders and the kernel
are completely irrelevant and misguided.