| Message ID | 1361284559-25125-1-git-send-email-p.zabel@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 >
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.
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
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.
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:
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(-)