* [PATCH] drm/cma-helper: fixup compilation
@ 2013-02-15 10:24 Daniel Vetter
2013-02-19 7:43 ` Thierry Reding
0 siblings, 1 reply; 6+ messages in thread
From: Daniel Vetter @ 2013-02-15 10:24 UTC (permalink / raw)
To: Dave Airlie; +Cc: Daniel Vetter, DRI Development
/me grabs a few brown paper bags
So it looks like I've broken compilation in
commit 6aed8ec3f76a22217c9ae183d32b1aa990bed069
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date: Sun Jan 20 17:32:21 2013 +0100
drm: review locking for drm_fb_helper_restore_fbdev_mode
Fix it up again.
Reported-by: Wu Fengguang <fengguang.wu@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
drivers/gpu/drm/drm_fb_cma_helper.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c
index e851658..ef68e34 100644
--- a/drivers/gpu/drm/drm_fb_cma_helper.c
+++ b/drivers/gpu/drm/drm_fb_cma_helper.c
@@ -377,6 +377,8 @@ EXPORT_SYMBOL_GPL(drm_fbdev_cma_fini);
*/
void drm_fbdev_cma_restore_mode(struct drm_fbdev_cma *fbdev_cma)
{
+ struct drm_device *dev = fbdev_cma->fb_helper.dev;
+
drm_modeset_lock_all(dev);
if (fbdev_cma)
drm_fb_helper_restore_fbdev_mode(&fbdev_cma->fb_helper);
--
1.7.10.4
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] drm/cma-helper: fixup compilation 2013-02-15 10:24 [PATCH] drm/cma-helper: fixup compilation Daniel Vetter @ 2013-02-19 7:43 ` Thierry Reding 2013-02-19 10:16 ` Daniel Vetter 2013-02-19 10:18 ` Daniel Vetter 0 siblings, 2 replies; 6+ messages in thread From: Thierry Reding @ 2013-02-19 7:43 UTC (permalink / raw) To: Daniel Vetter; +Cc: DRI Development [-- Attachment #1.1: Type: text/plain, Size: 2048 bytes --] On Fri, Feb 15, 2013 at 11:24:35AM +0100, Daniel Vetter wrote: > /me grabs a few brown paper bags > > So it looks like I've broken compilation in > > commit 6aed8ec3f76a22217c9ae183d32b1aa990bed069 > Author: Daniel Vetter <daniel.vetter@ffwll.ch> > Date: Sun Jan 20 17:32:21 2013 +0100 > > drm: review locking for drm_fb_helper_restore_fbdev_mode > > Fix it up again. > > Reported-by: Wu Fengguang <fengguang.wu@intel.com> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > drivers/gpu/drm/drm_fb_cma_helper.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c > index e851658..ef68e34 100644 > --- a/drivers/gpu/drm/drm_fb_cma_helper.c > +++ b/drivers/gpu/drm/drm_fb_cma_helper.c > @@ -377,6 +377,8 @@ EXPORT_SYMBOL_GPL(drm_fbdev_cma_fini); > */ > void drm_fbdev_cma_restore_mode(struct drm_fbdev_cma *fbdev_cma) > { > + struct drm_device *dev = fbdev_cma->fb_helper.dev; > + > drm_modeset_lock_all(dev); > if (fbdev_cma) > drm_fb_helper_restore_fbdev_mode(&fbdev_cma->fb_helper); The above check indicates that fbdev_cma might be NULL, so you're potentially dereferencing NULL when assigning the dev variable. Perhaps a better way would be to move the locking into the if block, as in the patch below. Thierry diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c index e851658..54a250f 100644 --- a/drivers/gpu/drm/drm_fb_cma_helper.c +++ b/drivers/gpu/drm/drm_fb_cma_helper.c @@ -377,10 +377,11 @@ EXPORT_SYMBOL_GPL(drm_fbdev_cma_fini); */ void drm_fbdev_cma_restore_mode(struct drm_fbdev_cma *fbdev_cma) { - drm_modeset_lock_all(dev); - if (fbdev_cma) + if (fbdev_cma) { + drm_modeset_lock_all(fbdev_cma->fb_helper.dev); drm_fb_helper_restore_fbdev_mode(&fbdev_cma->fb_helper); - drm_modeset_unlock_all(dev); + drm_modeset_unlock_all(fbdev_cma->fb_helper.dev); + } } EXPORT_SYMBOL_GPL(drm_fbdev_cma_restore_mode); [-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --] [-- Attachment #2: Type: text/plain, Size: 159 bytes --] _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/cma-helper: fixup compilation 2013-02-19 7:43 ` Thierry Reding @ 2013-02-19 10:16 ` Daniel Vetter 2013-02-19 10:25 ` Thierry Reding 2013-02-19 10:18 ` Daniel Vetter 1 sibling, 1 reply; 6+ messages in thread From: Daniel Vetter @ 2013-02-19 10:16 UTC (permalink / raw) To: Thierry Reding; +Cc: Laurent Pinchart, DRI Development On Tue, Feb 19, 2013 at 8:43 AM, Thierry Reding <thierry.reding@avionic-design.de> wrote: > On Fri, Feb 15, 2013 at 11:24:35AM +0100, Daniel Vetter wrote: >> /me grabs a few brown paper bags >> >> So it looks like I've broken compilation in >> >> commit 6aed8ec3f76a22217c9ae183d32b1aa990bed069 >> Author: Daniel Vetter <daniel.vetter@ffwll.ch> >> Date: Sun Jan 20 17:32:21 2013 +0100 >> >> drm: review locking for drm_fb_helper_restore_fbdev_mode >> >> Fix it up again. >> >> Reported-by: Wu Fengguang <fengguang.wu@intel.com> >> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> >> --- >> drivers/gpu/drm/drm_fb_cma_helper.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c >> index e851658..ef68e34 100644 >> --- a/drivers/gpu/drm/drm_fb_cma_helper.c >> +++ b/drivers/gpu/drm/drm_fb_cma_helper.c >> @@ -377,6 +377,8 @@ EXPORT_SYMBOL_GPL(drm_fbdev_cma_fini); >> */ >> void drm_fbdev_cma_restore_mode(struct drm_fbdev_cma *fbdev_cma) >> { >> + struct drm_device *dev = fbdev_cma->fb_helper.dev; >> + >> drm_modeset_lock_all(dev); >> if (fbdev_cma) >> drm_fb_helper_restore_fbdev_mode(&fbdev_cma->fb_helper); > > The above check indicates that fbdev_cma might be NULL, so you're > potentially dereferencing NULL when assigning the dev variable. Right, looks like a need more brown-paper bags. > Perhaps a better way would be to move the locking into the if block, as > in the patch below. Otoh I think it's a bit funny that you can pass NULL to restore_mode and hotplug_event in the cma fb helper code. Imo it would be better to just ditch those checks ... Laurent? I'll update the patch meanwhile. -Daniel > > Thierry > > diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c > index e851658..54a250f 100644 > --- a/drivers/gpu/drm/drm_fb_cma_helper.c > +++ b/drivers/gpu/drm/drm_fb_cma_helper.c > @@ -377,10 +377,11 @@ EXPORT_SYMBOL_GPL(drm_fbdev_cma_fini); > */ > void drm_fbdev_cma_restore_mode(struct drm_fbdev_cma *fbdev_cma) > { > - drm_modeset_lock_all(dev); > - if (fbdev_cma) > + if (fbdev_cma) { > + drm_modeset_lock_all(fbdev_cma->fb_helper.dev); > drm_fb_helper_restore_fbdev_mode(&fbdev_cma->fb_helper); > - drm_modeset_unlock_all(dev); > + drm_modeset_unlock_all(fbdev_cma->fb_helper.dev); > + } > } > EXPORT_SYMBOL_GPL(drm_fbdev_cma_restore_mode); -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/cma-helper: fixup compilation 2013-02-19 10:16 ` Daniel Vetter @ 2013-02-19 10:25 ` Thierry Reding 0 siblings, 0 replies; 6+ messages in thread From: Thierry Reding @ 2013-02-19 10:25 UTC (permalink / raw) To: Daniel Vetter; +Cc: Laurent Pinchart, DRI Development [-- Attachment #1.1: Type: text/plain, Size: 2286 bytes --] On Tue, Feb 19, 2013 at 11:16:08AM +0100, Daniel Vetter wrote: > On Tue, Feb 19, 2013 at 8:43 AM, Thierry Reding > <thierry.reding@avionic-design.de> wrote: > > On Fri, Feb 15, 2013 at 11:24:35AM +0100, Daniel Vetter wrote: > >> /me grabs a few brown paper bags > >> > >> So it looks like I've broken compilation in > >> > >> commit 6aed8ec3f76a22217c9ae183d32b1aa990bed069 > >> Author: Daniel Vetter <daniel.vetter@ffwll.ch> > >> Date: Sun Jan 20 17:32:21 2013 +0100 > >> > >> drm: review locking for drm_fb_helper_restore_fbdev_mode > >> > >> Fix it up again. > >> > >> Reported-by: Wu Fengguang <fengguang.wu@intel.com> > >> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > >> --- > >> drivers/gpu/drm/drm_fb_cma_helper.c | 2 ++ > >> 1 file changed, 2 insertions(+) > >> > >> diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c > >> index e851658..ef68e34 100644 > >> --- a/drivers/gpu/drm/drm_fb_cma_helper.c > >> +++ b/drivers/gpu/drm/drm_fb_cma_helper.c > >> @@ -377,6 +377,8 @@ EXPORT_SYMBOL_GPL(drm_fbdev_cma_fini); > >> */ > >> void drm_fbdev_cma_restore_mode(struct drm_fbdev_cma *fbdev_cma) > >> { > >> + struct drm_device *dev = fbdev_cma->fb_helper.dev; > >> + > >> drm_modeset_lock_all(dev); > >> if (fbdev_cma) > >> drm_fb_helper_restore_fbdev_mode(&fbdev_cma->fb_helper); > > > > The above check indicates that fbdev_cma might be NULL, so you're > > potentially dereferencing NULL when assigning the dev variable. > > Right, looks like a need more brown-paper bags. > > > Perhaps a better way would be to move the locking into the if block, as > > in the patch below. > > Otoh I think it's a bit funny that you can pass NULL to restore_mode > and hotplug_event in the cma fb helper code. Imo it would be better to > just ditch those checks ... Laurent? I'll update the patch meanwhile. I agree, it is probably safe to drop the checks and consider it a driver error (and let it oops accordingly) if NULL is ever passed in. I expect drivers will usually bail out if drm_fbdev_cma_init() fails. If they choose to continue maybe they should be responsible for not calling drm_fbdev_cma_restore_mode() if fbdev is NULL. Thierry [-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --] [-- Attachment #2: Type: text/plain, Size: 159 bytes --] _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] drm/cma-helper: fixup compilation 2013-02-19 7:43 ` Thierry Reding 2013-02-19 10:16 ` Daniel Vetter @ 2013-02-19 10:18 ` Daniel Vetter 2013-02-19 10:22 ` Thierry Reding 1 sibling, 1 reply; 6+ messages in thread From: Daniel Vetter @ 2013-02-19 10:18 UTC (permalink / raw) To: Dave Airlie; +Cc: Daniel Vetter, DRI Development /me grabs a few brown paper bags So it looks like I've broken compilation in commit 6aed8ec3f76a22217c9ae183d32b1aa990bed069 Author: Daniel Vetter <daniel.vetter@ffwll.ch> Date: Sun Jan 20 17:32:21 2013 +0100 drm: review locking for drm_fb_helper_restore_fbdev_mode Fix it up again. v2: Only deref fbdev_cma once we're sure it's non-NULL, noticed by Thierry Reding. Reported-by: Wu Fengguang <fengguang.wu@intel.com> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> --- drivers/gpu/drm/drm_fb_cma_helper.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c index e851658..1c8549d 100644 --- a/drivers/gpu/drm/drm_fb_cma_helper.c +++ b/drivers/gpu/drm/drm_fb_cma_helper.c @@ -377,10 +377,13 @@ EXPORT_SYMBOL_GPL(drm_fbdev_cma_fini); */ void drm_fbdev_cma_restore_mode(struct drm_fbdev_cma *fbdev_cma) { - drm_modeset_lock_all(dev); - if (fbdev_cma) + if (fbdev_cma) { + struct drm_device *dev = fbdev_cma->fb_helper.dev; + + drm_modeset_lock_all(dev); drm_fb_helper_restore_fbdev_mode(&fbdev_cma->fb_helper); - drm_modeset_unlock_all(dev); + drm_modeset_unlock_all(dev); + } } EXPORT_SYMBOL_GPL(drm_fbdev_cma_restore_mode); -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/cma-helper: fixup compilation 2013-02-19 10:18 ` Daniel Vetter @ 2013-02-19 10:22 ` Thierry Reding 0 siblings, 0 replies; 6+ messages in thread From: Thierry Reding @ 2013-02-19 10:22 UTC (permalink / raw) To: Daniel Vetter; +Cc: DRI Development [-- Attachment #1.1: Type: text/plain, Size: 1514 bytes --] On Tue, Feb 19, 2013 at 11:18:04AM +0100, Daniel Vetter wrote: > /me grabs a few brown paper bags > > So it looks like I've broken compilation in > > commit 6aed8ec3f76a22217c9ae183d32b1aa990bed069 > Author: Daniel Vetter <daniel.vetter@ffwll.ch> > Date: Sun Jan 20 17:32:21 2013 +0100 > > drm: review locking for drm_fb_helper_restore_fbdev_mode > > Fix it up again. > > v2: Only deref fbdev_cma once we're sure it's non-NULL, noticed by > Thierry Reding. > > Reported-by: Wu Fengguang <fengguang.wu@intel.com> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > drivers/gpu/drm/drm_fb_cma_helper.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c > index e851658..1c8549d 100644 > --- a/drivers/gpu/drm/drm_fb_cma_helper.c > +++ b/drivers/gpu/drm/drm_fb_cma_helper.c > @@ -377,10 +377,13 @@ EXPORT_SYMBOL_GPL(drm_fbdev_cma_fini); > */ > void drm_fbdev_cma_restore_mode(struct drm_fbdev_cma *fbdev_cma) > { > - drm_modeset_lock_all(dev); > - if (fbdev_cma) > + if (fbdev_cma) { > + struct drm_device *dev = fbdev_cma->fb_helper.dev; > + > + drm_modeset_lock_all(dev); > drm_fb_helper_restore_fbdev_mode(&fbdev_cma->fb_helper); > - drm_modeset_unlock_all(dev); > + drm_modeset_unlock_all(dev); > + } > } > EXPORT_SYMBOL_GPL(drm_fbdev_cma_restore_mode); > Reviewed-by: Thierry Reding <thierry.reding@avionic-design.de> [-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --] [-- Attachment #2: Type: text/plain, Size: 159 bytes --] _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-02-19 10:25 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-02-15 10:24 [PATCH] drm/cma-helper: fixup compilation Daniel Vetter 2013-02-19 7:43 ` Thierry Reding 2013-02-19 10:16 ` Daniel Vetter 2013-02-19 10:25 ` Thierry Reding 2013-02-19 10:18 ` Daniel Vetter 2013-02-19 10:22 ` Thierry Reding
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox