* [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
* [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
* 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
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