[PATCH 1/3] drm/i915/sdvo: Fully translate sync flags in the dtd->mode conversion

[PATCH 1/3] drm/i915/sdvo: Fully translate sync flags in the dtd->mode conversion

[Daniel Vetter]

Instead of just a flag bit for each of the positive/negative sync
modes drm actually uses a separate flag for each ... This upsets the
modeset checker since the adjusted mode filled out at modeset time
doesn't match the one reconstructed at check time (since the
->get_config callback already gets this right).

Reported-by: Knut Petersen <Knut_Petersen@t-online.de>
Cc: Knut Petersen <Knut_Petersen@t-online.de>
References: http://www.gossamer-threads.com/lists/linux/kernel/1778688?do=post_view_threaded
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_sdvo.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
index 85037b9..5033c74 100644
--- a/drivers/gpu/drm/i915/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/intel_sdvo.c
@@ -866,8 +866,12 @@ static void intel_sdvo_get_mode_from_dtd(struct drm_display_mode * mode,
 		mode->flags |= DRM_MODE_FLAG_INTERLACE;
 	if (dtd->part2.dtd_flags & DTD_FLAG_HSYNC_POSITIVE)
 		mode->flags |= DRM_MODE_FLAG_PHSYNC;
+	else
+		mode->flags |= DRM_MODE_FLAG_NHSYNC;
 	if (dtd->part2.dtd_flags & DTD_FLAG_VSYNC_POSITIVE)
 		mode->flags |= DRM_MODE_FLAG_PVSYNC;
+	else
+		mode->flags |= DRM_MODE_FLAG_NVSYNC;
 }
 
 static bool intel_sdvo_check_supp_encode(struct intel_sdvo *intel_sdvo)
-- 
1.8.4.rc3

[PATCH 2/3] drm/i915/sdvo: Robustify the dtd<->drm_mode conversions

[Daniel Vetter]

We've failed to properly clear out the flags when converting a dtd to
a drm mode. For more paranoia just memset the entire structure (and
drop the now redundant clears).

Also since

commit 135c81b8c3c9a70d7b55758c9c2a247a4abb7b64
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Sun Jul 21 21:37:09 2013 +0200

    drm/i915: clean up crtc timings computation

we don't update the crtc timings any more properly, so do that again.

Cc: Rodrigo Vivi <rodrigo.vivi@gmail.com>
Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Reported-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_sdvo.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
index 5033c74..d4a046d 100644
--- a/drivers/gpu/drm/i915/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/intel_sdvo.c
@@ -788,6 +788,8 @@ static void intel_sdvo_get_dtd_from_mode(struct intel_sdvo_dtd *dtd,
 	uint16_t h_sync_offset, v_sync_offset;
 	int mode_clock;
 
+	memset(dtd, 0, sizeof(*dtd));
+
 	width = mode->hdisplay;
 	height = mode->vdisplay;
 
@@ -830,14 +832,14 @@ static void intel_sdvo_get_dtd_from_mode(struct intel_sdvo_dtd *dtd,
 	if (mode->flags & DRM_MODE_FLAG_PVSYNC)
 		dtd->part2.dtd_flags |= DTD_FLAG_VSYNC_POSITIVE;
 
-	dtd->part2.sdvo_flags = 0;
 	dtd->part2.v_sync_off_high = v_sync_offset & 0xc0;
-	dtd->part2.reserved = 0;
 }
 
 static void intel_sdvo_get_mode_from_dtd(struct drm_display_mode * mode,
 					 const struct intel_sdvo_dtd *dtd)
 {
+	memset(mode, 0, sizeof(*mode));
+
 	mode->hdisplay = dtd->part1.h_active;
 	mode->hdisplay += ((dtd->part1.h_high >> 4) & 0x0f) << 8;
 	mode->hsync_start = mode->hdisplay + dtd->part2.h_sync_off;
@@ -872,6 +874,8 @@ static void intel_sdvo_get_mode_from_dtd(struct drm_display_mode * mode,
 		mode->flags |= DRM_MODE_FLAG_PVSYNC;
 	else
 		mode->flags |= DRM_MODE_FLAG_NVSYNC;
+
+	drm_mode_set_crtcinfo(mode);
 }
 
 static bool intel_sdvo_check_supp_encode(struct intel_sdvo *intel_sdvo)
-- 
1.8.4.rc3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/3] drm/i915/dvo: set crtc timings again for panel fixed modes

[Daniel Vetter]

Yet another regression due to

commit 135c81b8c3c9a70d7b55758c9c2a247a4abb7b64
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Sun Jul 21 21:37:09 2013 +0200

    drm/i915: clean up crtc timings computation

I'm starting to wonder whether this was worth it ...

Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_dvo.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_dvo.c b/drivers/gpu/drm/i915/intel_dvo.c
index 406303b..fd21ba4 100644
--- a/drivers/gpu/drm/i915/intel_dvo.c
+++ b/drivers/gpu/drm/i915/intel_dvo.c
@@ -263,6 +263,8 @@ static bool intel_dvo_compute_config(struct intel_encoder *encoder,
 		C(vtotal);
 		C(clock);
 #undef C
+
+		drm_mode_set_crtcinfo(adjusted_mode);
 	}
 
 	if (intel_dvo->dev.dev_ops->mode_fixup)
-- 
1.8.4.rc3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/3] drm/i915/sdvo: Robustify the dtd<->drm_mode conversions

[Ville Syrjälä]

On Tue, Sep 10, 2013 at 12:06:51PM +0200, Daniel Vetter wrote:
> We've failed to properly clear out the flags when converting a dtd to
> a drm mode. For more paranoia just memset the entire structure (and
> drop the now redundant clears).
> 
> Also since
> 
> commit 135c81b8c3c9a70d7b55758c9c2a247a4abb7b64
> Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> Date:   Sun Jul 21 21:37:09 2013 +0200
> 
>     drm/i915: clean up crtc timings computation
> 
> we don't update the crtc timings any more properly, so do that again.
> 
> Cc: Rodrigo Vivi <rodrigo.vivi@gmail.com>
> Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Reported-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_sdvo.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
> index 5033c74..d4a046d 100644
> --- a/drivers/gpu/drm/i915/intel_sdvo.c
> +++ b/drivers/gpu/drm/i915/intel_sdvo.c
> @@ -788,6 +788,8 @@ static void intel_sdvo_get_dtd_from_mode(struct intel_sdvo_dtd *dtd,
>  	uint16_t h_sync_offset, v_sync_offset;
>  	int mode_clock;
>  
> +	memset(dtd, 0, sizeof(*dtd));
> +
>  	width = mode->hdisplay;
>  	height = mode->vdisplay;
>  
> @@ -830,14 +832,14 @@ static void intel_sdvo_get_dtd_from_mode(struct intel_sdvo_dtd *dtd,
>  	if (mode->flags & DRM_MODE_FLAG_PVSYNC)
>  		dtd->part2.dtd_flags |= DTD_FLAG_VSYNC_POSITIVE;
>  
> -	dtd->part2.sdvo_flags = 0;
>  	dtd->part2.v_sync_off_high = v_sync_offset & 0xc0;
> -	dtd->part2.reserved = 0;
>  }
>  
>  static void intel_sdvo_get_mode_from_dtd(struct drm_display_mode * mode,
>  					 const struct intel_sdvo_dtd *dtd)
>  {
> +	memset(mode, 0, sizeof(*mode));

I have a theoretical worry that someone might end up calling this on a
mode that sits on some list or was actually allocated and has a proper
object id which we'd leak here.

To make it totally safe you could populate a pristine mode struct and
use drm_mode_copy() to overwrite adjusted_mode. Assuming we're not so
short on stack space that our oversized mode struct would cause issues.
Other options would be to add some WARNs to catch wrongdoers, or embed
a temp mode for this purpose inside the intel_sdvo struct.

> +
>  	mode->hdisplay = dtd->part1.h_active;
>  	mode->hdisplay += ((dtd->part1.h_high >> 4) & 0x0f) << 8;
>  	mode->hsync_start = mode->hdisplay + dtd->part2.h_sync_off;
> @@ -872,6 +874,8 @@ static void intel_sdvo_get_mode_from_dtd(struct drm_display_mode * mode,

Somewhere around these parts we have:

'mode->flags &= ~(DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC);'

It can now be eliminated.


>  		mode->flags |= DRM_MODE_FLAG_PVSYNC;
>  	else
>  		mode->flags |= DRM_MODE_FLAG_NVSYNC;
> +
> +	drm_mode_set_crtcinfo(mode);
>  }
>  
>  static bool intel_sdvo_check_supp_encode(struct intel_sdvo *intel_sdvo)
> -- 
> 1.8.4.rc3

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 2/3] drm/i915/sdvo: Robustify the dtd<->drm_mode conversions

[Daniel Vetter]

On Tue, Sep 10, 2013 at 12:26 PM, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
>>  static void intel_sdvo_get_mode_from_dtd(struct drm_display_mode * mode,
>>                                        const struct intel_sdvo_dtd *dtd)
>>  {
>> +     memset(mode, 0, sizeof(*mode));
>
> I have a theoretical worry that someone might end up calling this on a
> mode that sits on some list or was actually allocated and has a proper
> object id which we'd leak here.
>
> To make it totally safe you could populate a pristine mode struct and
> use drm_mode_copy() to overwrite adjusted_mode. Assuming we're not so
> short on stack space that our oversized mode struct would cause issues.
> Other options would be to add some WARNs to catch wrongdoers, or embed
> a temp mode for this purpose inside the intel_sdvo struct.

We can't really check for this since list_empty on stack garbage won't
work too well, either. And e.g. ->get_config has the pipe config on
the stack. So I think we just need to do review here. I also think the
risk is pretty low, this is all used in internal structures around
pipe_config, where the mode is never linked.

>> +
>>       mode->hdisplay = dtd->part1.h_active;
>>       mode->hdisplay += ((dtd->part1.h_high >> 4) & 0x0f) << 8;
>>       mode->hsync_start = mode->hdisplay + dtd->part2.h_sync_off;
>> @@ -872,6 +874,8 @@ static void intel_sdvo_get_mode_from_dtd(struct drm_display_mode * mode,
>
> Somewhere around these parts we have:
>
> 'mode->flags &= ~(DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC);'
>
> It can now be eliminated.

Will fix and resend.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH] drm/i915/sdvo: Robustify the dtd<->drm_mode conversions

[Daniel Vetter]

We've failed to properly clear out the flags when converting a dtd to
a drm mode. For more paranoia just memset the entire structure (and
drop the now redundant clears).

Also since

commit 135c81b8c3c9a70d7b55758c9c2a247a4abb7b64
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Sun Jul 21 21:37:09 2013 +0200

    drm/i915: clean up crtc timings computation

we don't update the crtc timings any more properly, so do that again.

v2: Remove more redundant clearing, spotted by Ville.

Cc: Rodrigo Vivi <rodrigo.vivi@gmail.com>
Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Reported-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_sdvo.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
index 5033c74..bf36d4f 100644
--- a/drivers/gpu/drm/i915/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/intel_sdvo.c
@@ -788,6 +788,8 @@ static void intel_sdvo_get_dtd_from_mode(struct intel_sdvo_dtd *dtd,
 	uint16_t h_sync_offset, v_sync_offset;
 	int mode_clock;
 
+	memset(dtd, 0, sizeof(*dtd));
+
 	width = mode->hdisplay;
 	height = mode->vdisplay;
 
@@ -830,14 +832,14 @@ static void intel_sdvo_get_dtd_from_mode(struct intel_sdvo_dtd *dtd,
 	if (mode->flags & DRM_MODE_FLAG_PVSYNC)
 		dtd->part2.dtd_flags |= DTD_FLAG_VSYNC_POSITIVE;
 
-	dtd->part2.sdvo_flags = 0;
 	dtd->part2.v_sync_off_high = v_sync_offset & 0xc0;
-	dtd->part2.reserved = 0;
 }
 
 static void intel_sdvo_get_mode_from_dtd(struct drm_display_mode * mode,
 					 const struct intel_sdvo_dtd *dtd)
 {
+	memset(mode, 0, sizeof(*mode));
+
 	mode->hdisplay = dtd->part1.h_active;
 	mode->hdisplay += ((dtd->part1.h_high >> 4) & 0x0f) << 8;
 	mode->hsync_start = mode->hdisplay + dtd->part2.h_sync_off;
@@ -861,7 +863,6 @@ static void intel_sdvo_get_mode_from_dtd(struct drm_display_mode * mode,
 
 	mode->clock = dtd->part1.clock * 10;
 
-	mode->flags &= ~(DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC);
 	if (dtd->part2.dtd_flags & DTD_FLAG_INTERLACE)
 		mode->flags |= DRM_MODE_FLAG_INTERLACE;
 	if (dtd->part2.dtd_flags & DTD_FLAG_HSYNC_POSITIVE)
@@ -872,6 +873,8 @@ static void intel_sdvo_get_mode_from_dtd(struct drm_display_mode * mode,
 		mode->flags |= DRM_MODE_FLAG_PVSYNC;
 	else
 		mode->flags |= DRM_MODE_FLAG_NVSYNC;
+
+	drm_mode_set_crtcinfo(mode);
 }
 
 static bool intel_sdvo_check_supp_encode(struct intel_sdvo *intel_sdvo)
-- 
1.8.4.rc3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 1/2] drm/i915/sdvo: Robustify the dtd<->drm_mode conversions

[Daniel Vetter]

We've failed to properly clear out the flags when converting a dtd to
a drm mode. For more paranoia just memset the entire structure (and
drop the now redundant clears).

Also since

commit 135c81b8c3c9a70d7b55758c9c2a247a4abb7b64
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Sun Jul 21 21:37:09 2013 +0200

    drm/i915: clean up crtc timings computation

we don't update the crtc timings any more properly, so do that again.

v2: Remove more redundant clearing, spotted by Ville.

v3: Actually make it compile. Oops.

Cc: Rodrigo Vivi <rodrigo.vivi@gmail.com>
Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Reported-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_sdvo.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
index 5033c74..0d9305b 100644
--- a/drivers/gpu/drm/i915/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/intel_sdvo.c
@@ -788,6 +788,8 @@ static void intel_sdvo_get_dtd_from_mode(struct intel_sdvo_dtd *dtd,
 	uint16_t h_sync_offset, v_sync_offset;
 	int mode_clock;
 
+	memset(dtd, 0, sizeof(*dtd));
+
 	width = mode->hdisplay;
 	height = mode->vdisplay;
 
@@ -830,14 +832,14 @@ static void intel_sdvo_get_dtd_from_mode(struct intel_sdvo_dtd *dtd,
 	if (mode->flags & DRM_MODE_FLAG_PVSYNC)
 		dtd->part2.dtd_flags |= DTD_FLAG_VSYNC_POSITIVE;
 
-	dtd->part2.sdvo_flags = 0;
 	dtd->part2.v_sync_off_high = v_sync_offset & 0xc0;
-	dtd->part2.reserved = 0;
 }
 
 static void intel_sdvo_get_mode_from_dtd(struct drm_display_mode * mode,
 					 const struct intel_sdvo_dtd *dtd)
 {
+	memset(mode, 0, sizeof(*mode));
+
 	mode->hdisplay = dtd->part1.h_active;
 	mode->hdisplay += ((dtd->part1.h_high >> 4) & 0x0f) << 8;
 	mode->hsync_start = mode->hdisplay + dtd->part2.h_sync_off;
@@ -861,7 +863,6 @@ static void intel_sdvo_get_mode_from_dtd(struct drm_display_mode * mode,
 
 	mode->clock = dtd->part1.clock * 10;
 
-	mode->flags &= ~(DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC);
 	if (dtd->part2.dtd_flags & DTD_FLAG_INTERLACE)
 		mode->flags |= DRM_MODE_FLAG_INTERLACE;
 	if (dtd->part2.dtd_flags & DTD_FLAG_HSYNC_POSITIVE)
@@ -872,6 +873,8 @@ static void intel_sdvo_get_mode_from_dtd(struct drm_display_mode * mode,
 		mode->flags |= DRM_MODE_FLAG_PVSYNC;
 	else
 		mode->flags |= DRM_MODE_FLAG_NVSYNC;
+
+	drm_mode_set_crtcinfo(mode, 0);
 }
 
 static bool intel_sdvo_check_supp_encode(struct intel_sdvo *intel_sdvo)
-- 
1.8.4.rc3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/2] drm/i915/dvo: set crtc timings again for panel fixed modes

[Daniel Vetter]

Yet another regression due to

commit 135c81b8c3c9a70d7b55758c9c2a247a4abb7b64
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Sun Jul 21 21:37:09 2013 +0200

    drm/i915: clean up crtc timings computation

I'm starting to wonder whether this was worth it ...

v2: Actually make it compile.

Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_dvo.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_dvo.c b/drivers/gpu/drm/i915/intel_dvo.c
index 406303b..7fa7df5 100644
--- a/drivers/gpu/drm/i915/intel_dvo.c
+++ b/drivers/gpu/drm/i915/intel_dvo.c
@@ -263,6 +263,8 @@ static bool intel_dvo_compute_config(struct intel_encoder *encoder,
 		C(vtotal);
 		C(clock);
 #undef C
+
+		drm_mode_set_crtcinfo(adjusted_mode, 0);
 	}
 
 	if (intel_dvo->dev.dev_ops->mode_fixup)
-- 
1.8.4.rc3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/3] drm/i915/sdvo: Robustify the dtd<->drm_mode conversions

[Ville Syrjälä]

On Tue, Sep 10, 2013 at 12:50:25PM +0200, Daniel Vetter wrote:
> On Tue, Sep 10, 2013 at 12:26 PM, Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote:
> >>  static void intel_sdvo_get_mode_from_dtd(struct drm_display_mode * mode,
> >>                                        const struct intel_sdvo_dtd *dtd)
> >>  {
> >> +     memset(mode, 0, sizeof(*mode));
> >
> > I have a theoretical worry that someone might end up calling this on a
> > mode that sits on some list or was actually allocated and has a proper
> > object id which we'd leak here.
> >
> > To make it totally safe you could populate a pristine mode struct and
> > use drm_mode_copy() to overwrite adjusted_mode. Assuming we're not so
> > short on stack space that our oversized mode struct would cause issues.
> > Other options would be to add some WARNs to catch wrongdoers, or embed
> > a temp mode for this purpose inside the intel_sdvo struct.
> 
> We can't really check for this since list_empty on stack garbage won't
> work too well, either. And e.g. ->get_config has the pipe config on
> the stack. So I think we just need to do review here. I also think the
> risk is pretty low, this is all used in internal structures around
> pipe_config, where the mode is never linked.

Well, another idea would be to add drm_mode_clear() what would do the
memset() but preserve the id and list head.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 2/3] drm/i915/sdvo: Robustify the dtd<->drm_mode conversions

[Daniel Vetter]

On Tue, Sep 10, 2013 at 1:00 PM, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
> On Tue, Sep 10, 2013 at 12:50:25PM +0200, Daniel Vetter wrote:
>> On Tue, Sep 10, 2013 at 12:26 PM, Ville Syrjälä
>> <ville.syrjala@linux.intel.com> wrote:
>> >>  static void intel_sdvo_get_mode_from_dtd(struct drm_display_mode * mode,
>> >>                                        const struct intel_sdvo_dtd *dtd)
>> >>  {
>> >> +     memset(mode, 0, sizeof(*mode));
>> >
>> > I have a theoretical worry that someone might end up calling this on a
>> > mode that sits on some list or was actually allocated and has a proper
>> > object id which we'd leak here.
>> >
>> > To make it totally safe you could populate a pristine mode struct and
>> > use drm_mode_copy() to overwrite adjusted_mode. Assuming we're not so
>> > short on stack space that our oversized mode struct would cause issues.
>> > Other options would be to add some WARNs to catch wrongdoers, or embed
>> > a temp mode for this purpose inside the intel_sdvo struct.
>>
>> We can't really check for this since list_empty on stack garbage won't
>> work too well, either. And e.g. ->get_config has the pipe config on
>> the stack. So I think we just need to do review here. I also think the
>> risk is pretty low, this is all used in internal structures around
>> pipe_config, where the mode is never linked.
>
> Well, another idea would be to add drm_mode_clear() what would do the
> memset() but preserve the id and list head.

At least for the adjusted mode embedded into the pipe config that
won't work either since we want to memset the entire thing to not miss
any fields ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 2/3] drm/i915/sdvo: Robustify the dtd<->drm_mode conversions

[Ville Syrjälä]

On Tue, Sep 10, 2013 at 02:26:10PM +0200, Daniel Vetter wrote:
> On Tue, Sep 10, 2013 at 1:00 PM, Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote:
> > On Tue, Sep 10, 2013 at 12:50:25PM +0200, Daniel Vetter wrote:
> >> On Tue, Sep 10, 2013 at 12:26 PM, Ville Syrjälä
> >> <ville.syrjala@linux.intel.com> wrote:
> >> >>  static void intel_sdvo_get_mode_from_dtd(struct drm_display_mode * mode,
> >> >>                                        const struct intel_sdvo_dtd *dtd)
> >> >>  {
> >> >> +     memset(mode, 0, sizeof(*mode));
> >> >
> >> > I have a theoretical worry that someone might end up calling this on a
> >> > mode that sits on some list or was actually allocated and has a proper
> >> > object id which we'd leak here.
> >> >
> >> > To make it totally safe you could populate a pristine mode struct and
> >> > use drm_mode_copy() to overwrite adjusted_mode. Assuming we're not so
> >> > short on stack space that our oversized mode struct would cause issues.
> >> > Other options would be to add some WARNs to catch wrongdoers, or embed
> >> > a temp mode for this purpose inside the intel_sdvo struct.
> >>
> >> We can't really check for this since list_empty on stack garbage won't
> >> work too well, either. And e.g. ->get_config has the pipe config on
> >> the stack. So I think we just need to do review here. I also think the
> >> risk is pretty low, this is all used in internal structures around
> >> pipe_config, where the mode is never linked.
> >
> > Well, another idea would be to add drm_mode_clear() what would do the
> > memset() but preserve the id and list head.
> 
> At least for the adjusted mode embedded into the pipe config that
> won't work either since we want to memset the entire thing to not miss
> any fields ...

drm_mode_clear() would skip only the obj id and list head just like
drm_mode_copy().

Also isn't the pipe config supposed to be entirely zeroed to start
with anyway? And we already use drm_mode_copy() to fill the initial
values for adjusted_mode. drm_mode_clear() would overwrite exactly
the same fields as drm_mode_copy() filled in.

-- 
Ville Syrjälä
Intel OTC