| Message ID | 20210907024900.873850-1-marex@denx.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: mxsfb: Fix NULL pointer dereference crash on unload | expand |
On Tue, Sep 07, 2021 at 04:49:00AM +0200, Marek Vasut wrote: > The mxsfb->crtc.funcs may already be NULL when unloading the driver, > in which case calling mxsfb_irq_disable() via drm_irq_uninstall() from > mxsfb_unload() leads to NULL pointer dereference. > > Since all we care about is masking the IRQ and mxsfb->base is still > valid, just use that to clear and mask the IRQ. > > Fixes: ae1ed00932819 ("drm: mxsfb: Stop using DRM simple display pipeline helper") > Signed-off-by: Marek Vasut <marex@denx.de> > Cc: Daniel Abrecht <public@danielabrecht.ch> > Cc: Emil Velikov <emil.l.velikov@gmail.com> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Cc: Sam Ravnborg <sam@ravnborg.org> > Cc: Stefan Agner <stefan@agner.ch> You probably want a drm_atomic_helper_shutdown instead of trying to do all that manually. We've also added a bunch more devm and drmm_ functions to automate the cleanup a lot more here, e.g. your drm_mode_config_cleanup is in the wrong place. Also I'm confused because I'm not even seeing this function anywhere in upstream. -Daniel > --- > drivers/gpu/drm/mxsfb/mxsfb_drv.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.c b/drivers/gpu/drm/mxsfb/mxsfb_drv.c > index ec0432fe1bdf8..86d78634a9799 100644 > --- a/drivers/gpu/drm/mxsfb/mxsfb_drv.c > +++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.c > @@ -173,7 +173,11 @@ static void mxsfb_irq_disable(struct drm_device *drm) > struct mxsfb_drm_private *mxsfb = drm->dev_private; > > mxsfb_enable_axi_clk(mxsfb); > - mxsfb->crtc.funcs->disable_vblank(&mxsfb->crtc); > + > + /* Disable and clear VBLANK IRQ */ > + writel(CTRL1_CUR_FRAME_DONE_IRQ_EN, mxsfb->base + LCDC_CTRL1 + REG_CLR); > + writel(CTRL1_CUR_FRAME_DONE_IRQ, mxsfb->base + LCDC_CTRL1 + REG_CLR); > + > mxsfb_disable_axi_clk(mxsfb); > } > > -- > 2.33.0 >
On 9/8/21 8:24 PM, Daniel Vetter wrote: > On Tue, Sep 07, 2021 at 04:49:00AM +0200, Marek Vasut wrote: >> The mxsfb->crtc.funcs may already be NULL when unloading the driver, >> in which case calling mxsfb_irq_disable() via drm_irq_uninstall() from >> mxsfb_unload() leads to NULL pointer dereference. >> >> Since all we care about is masking the IRQ and mxsfb->base is still >> valid, just use that to clear and mask the IRQ. >> >> Fixes: ae1ed00932819 ("drm: mxsfb: Stop using DRM simple display pipeline helper") >> Signed-off-by: Marek Vasut <marex@denx.de> >> Cc: Daniel Abrecht <public@danielabrecht.ch> >> Cc: Emil Velikov <emil.l.velikov@gmail.com> >> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >> Cc: Sam Ravnborg <sam@ravnborg.org> >> Cc: Stefan Agner <stefan@agner.ch> > > You probably want a drm_atomic_helper_shutdown instead of trying to do all > that manually. We've also added a bunch more devm and drmm_ functions to > automate the cleanup a lot more here, e.g. your drm_mode_config_cleanup is > in the wrong place. > > Also I'm confused because I'm not even seeing this function anywhere in > upstream. It is still here: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/gpu/drm/mxsfb/mxsfb_drv.c#n171 as of: 999569d59a0aa ("Add linux-next specific files for 20210908") Is there some other tree I should be looking at ?
Hi Marek, On Wed, Sep 08, 2021 at 08:24:20PM +0200, Daniel Vetter wrote: > On Tue, Sep 07, 2021 at 04:49:00AM +0200, Marek Vasut wrote: > > The mxsfb->crtc.funcs may already be NULL when unloading the driver, > > in which case calling mxsfb_irq_disable() via drm_irq_uninstall() from > > mxsfb_unload() leads to NULL pointer dereference. > > > > Since all we care about is masking the IRQ and mxsfb->base is still > > valid, just use that to clear and mask the IRQ. > > > > Fixes: ae1ed00932819 ("drm: mxsfb: Stop using DRM simple display pipeline helper") > > Signed-off-by: Marek Vasut <marex@denx.de> > > Cc: Daniel Abrecht <public@danielabrecht.ch> > > Cc: Emil Velikov <emil.l.velikov@gmail.com> > > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Cc: Sam Ravnborg <sam@ravnborg.org> > > Cc: Stefan Agner <stefan@agner.ch> > > You probably want a drm_atomic_helper_shutdown instead of trying to do all > that manually. We've also added a bunch more devm and drmm_ functions to > automate the cleanup a lot more here, e.g. your drm_mode_config_cleanup is > in the wrong place. I have applied v2 of this patch today - but failed to see any response in v2 to this comment from Daniel. Was it lost somehow? Sam
On 10/17/21 7:52 PM, Sam Ravnborg wrote: > Hi Marek, > > On Wed, Sep 08, 2021 at 08:24:20PM +0200, Daniel Vetter wrote: >> On Tue, Sep 07, 2021 at 04:49:00AM +0200, Marek Vasut wrote: >>> The mxsfb->crtc.funcs may already be NULL when unloading the driver, >>> in which case calling mxsfb_irq_disable() via drm_irq_uninstall() from >>> mxsfb_unload() leads to NULL pointer dereference. >>> >>> Since all we care about is masking the IRQ and mxsfb->base is still >>> valid, just use that to clear and mask the IRQ. >>> >>> Fixes: ae1ed00932819 ("drm: mxsfb: Stop using DRM simple display pipeline helper") >>> Signed-off-by: Marek Vasut <marex@denx.de> >>> Cc: Daniel Abrecht <public@danielabrecht.ch> >>> Cc: Emil Velikov <emil.l.velikov@gmail.com> >>> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >>> Cc: Sam Ravnborg <sam@ravnborg.org> >>> Cc: Stefan Agner <stefan@agner.ch> >> >> You probably want a drm_atomic_helper_shutdown instead of trying to do all >> that manually. We've also added a bunch more devm and drmm_ functions to >> automate the cleanup a lot more here, e.g. your drm_mode_config_cleanup is >> in the wrong place. > > I have applied v2 of this patch today - but failed to see any response > in v2 to this comment from Daniel. Was it lost somehow? Hmmm, I'll investigate that ^ in the next round of mxsfb clean ups.
On Sun, Oct 17, 2021 at 10:05 PM Marek Vasut <marex@denx.de> wrote: > > On 10/17/21 7:52 PM, Sam Ravnborg wrote: > > Hi Marek, > > > > On Wed, Sep 08, 2021 at 08:24:20PM +0200, Daniel Vetter wrote: > >> On Tue, Sep 07, 2021 at 04:49:00AM +0200, Marek Vasut wrote: > >>> The mxsfb->crtc.funcs may already be NULL when unloading the driver, > >>> in which case calling mxsfb_irq_disable() via drm_irq_uninstall() from > >>> mxsfb_unload() leads to NULL pointer dereference. > >>> > >>> Since all we care about is masking the IRQ and mxsfb->base is still > >>> valid, just use that to clear and mask the IRQ. > >>> > >>> Fixes: ae1ed00932819 ("drm: mxsfb: Stop using DRM simple display pipeline helper") > >>> Signed-off-by: Marek Vasut <marex@denx.de> > >>> Cc: Daniel Abrecht <public@danielabrecht.ch> > >>> Cc: Emil Velikov <emil.l.velikov@gmail.com> > >>> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >>> Cc: Sam Ravnborg <sam@ravnborg.org> > >>> Cc: Stefan Agner <stefan@agner.ch> > >> > >> You probably want a drm_atomic_helper_shutdown instead of trying to do all > >> that manually. We've also added a bunch more devm and drmm_ functions to > >> automate the cleanup a lot more here, e.g. your drm_mode_config_cleanup is > >> in the wrong place. > > > > I have applied v2 of this patch today - but failed to see any response > > in v2 to this comment from Daniel. Was it lost somehow? > > Hmmm, I'll investigate that ^ in the next round of mxsfb clean ups. Yeah this was just a quick comment about how to do this cleanly, not meant to hold up a bugfix or anything like that. -Daniel
diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.c b/drivers/gpu/drm/mxsfb/mxsfb_drv.c index ec0432fe1bdf8..86d78634a9799 100644 --- a/drivers/gpu/drm/mxsfb/mxsfb_drv.c +++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.c @@ -173,7 +173,11 @@ static void mxsfb_irq_disable(struct drm_device *drm) struct mxsfb_drm_private *mxsfb = drm->dev_private; mxsfb_enable_axi_clk(mxsfb); - mxsfb->crtc.funcs->disable_vblank(&mxsfb->crtc); + + /* Disable and clear VBLANK IRQ */ + writel(CTRL1_CUR_FRAME_DONE_IRQ_EN, mxsfb->base + LCDC_CTRL1 + REG_CLR); + writel(CTRL1_CUR_FRAME_DONE_IRQ, mxsfb->base + LCDC_CTRL1 + REG_CLR); + mxsfb_disable_axi_clk(mxsfb); }
The mxsfb->crtc.funcs may already be NULL when unloading the driver, in which case calling mxsfb_irq_disable() via drm_irq_uninstall() from mxsfb_unload() leads to NULL pointer dereference. Since all we care about is masking the IRQ and mxsfb->base is still valid, just use that to clear and mask the IRQ. Fixes: ae1ed00932819 ("drm: mxsfb: Stop using DRM simple display pipeline helper") Signed-off-by: Marek Vasut <marex@denx.de> Cc: Daniel Abrecht <public@danielabrecht.ch> Cc: Emil Velikov <emil.l.velikov@gmail.com> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Cc: Sam Ravnborg <sam@ravnborg.org> Cc: Stefan Agner <stefan@agner.ch> --- drivers/gpu/drm/mxsfb/mxsfb_drv.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)