drm/i915: Fix crash when failing to parse MIPI VBT

Commit Message

[e-mail] July 24, 2014, 2:16 p.m. UTC

From: Rafael Barbalho <[e-mail]>

This particular nasty presented itself while trying to register the
intelfb device (intel_fbdev.c). During the process of registering the device
the driver will disable the crtc via i9xx_crtc_disable. These will
also disable the panel using the generic mipi panel functions in
dsi_mod_vbt_generic.c. The stale MIPI generic data sequence pointers would
cause a crash within those functions. However, all of this is happening
while console_lock is held from do_register_framebuffer inside fbcon.c. Which
means that you got kernel log and just the device appearing to reboot/hang for
no apparent reason.

The fault started from the FB_EVENT_FB_REGISTERED event using the
fb_notifier_call_chain call in fbcon.c.

Cc: Shobhit Kumar <[e-mail]>
Signed-off-by: Rafael Barbalho <[e-mail]>
---
 drivers/gpu/drm/i915/intel_bios.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Kumar, Shobhit July 25, 2014, 6:32 a.m. UTC | #1

On 7/24/2014 7:46 PM, [e-mail] wrote:
> From: Rafael Barbalho <[e-mail]>
>
> This particular nasty presented itself while trying to register the
> intelfb device (intel_fbdev.c). During the process of registering the device
> the driver will disable the crtc via i9xx_crtc_disable. These will
> also disable the panel using the generic mipi panel functions in
> dsi_mod_vbt_generic.c. The stale MIPI generic data sequence pointers would
> cause a crash within those functions. However, all of this is happening
> while console_lock is held from do_register_framebuffer inside fbcon.c. Which
> means that you got kernel log and just the device appearing to reboot/hang for
> no apparent reason.
>
> The fault started from the FB_EVENT_FB_REGISTERED event using the
> fb_notifier_call_chain call in fbcon.c.
>
> Cc: Shobhit Kumar <[e-mail]>
> Signed-off-by: Rafael Barbalho <[e-mail]>
> ---
>   drivers/gpu/drm/i915/intel_bios.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
> index 608ed30..a669550 100644
> --- a/drivers/gpu/drm/i915/intel_bios.c
> +++ b/drivers/gpu/drm/i915/intel_bios.c
> @@ -878,7 +878,7 @@ err:
>
>   	/* error during parsing so set all pointers to null
>   	 * because of partial parsing */
> -	memset(dev_priv->vbt.dsi.sequence, 0, MIPI_SEQ_MAX);
> +	memset(dev_priv->vbt.dsi.sequence, 0, sizeof(dev_priv->vbt.dsi.sequence));

Ouch !! This mistake hurts. This is manifesting now because the VBT 
probably you tested had sequences not supported by the driver. I am 
influencing a TLV based VBT structure design for the sequence which when 
done will ensure proper parsing for all that is known and unknown 
pointers will remain NULL. But for now

Reviewed-by: Shobhit Kumar <[e-mail]>

Regards
Shobhit

Daniel Vetter July 25, 2014, 7:47 a.m. UTC | #2

On Thu, Jul 24, 2014 at 03:16:12PM +0100, [e-mail] wrote:
> From: Rafael Barbalho <[e-mail]>
> 
> This particular nasty presented itself while trying to register the
> intelfb device (intel_fbdev.c). During the process of registering the device
> the driver will disable the crtc via i9xx_crtc_disable. These will
> also disable the panel using the generic mipi panel functions in
> dsi_mod_vbt_generic.c. The stale MIPI generic data sequence pointers would
> cause a crash within those functions. However, all of this is happening
> while console_lock is held from do_register_framebuffer inside fbcon.c. Which
> means that you got kernel log and just the device appearing to reboot/hang for
> no apparent reason.

CONFIG_I915_FBDEV=n for when the console_lock gets in the way.

> The fault started from the FB_EVENT_FB_REGISTERED event using the
> fb_notifier_call_chain call in fbcon.c.
> 
> Cc: Shobhit Kumar <[e-mail]>
> Signed-off-by: Rafael Barbalho <[e-mail]>

Queued for -next, thanks for the patch.
-Daniel
> ---
>  drivers/gpu/drm/i915/intel_bios.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
> index 608ed30..a669550 100644
> --- a/drivers/gpu/drm/i915/intel_bios.c
> +++ b/drivers/gpu/drm/i915/intel_bios.c
> @@ -878,7 +878,7 @@ err:
>  
>  	/* error during parsing so set all pointers to null
>  	 * because of partial parsing */
> -	memset(dev_priv->vbt.dsi.sequence, 0, MIPI_SEQ_MAX);
> +	memset(dev_priv->vbt.dsi.sequence, 0, sizeof(dev_priv->vbt.dsi.sequence));
>  }
>  
>  static void parse_ddi_port(struct drm_i915_private *dev_priv, enum port port,
> -- 
> 2.0.2
> 
> _______________________________________________
> Intel-gfx mailing list
> [e-mail]
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Daniel Vetter July 25, 2014, 7:50 a.m. UTC | #3

On Fri, Jul 25, 2014 at 09:47:06AM +0200, Daniel Vetter wrote:
> On Thu, Jul 24, 2014 at 03:16:12PM +0100, [e-mail] wrote:
> > From: Rafael Barbalho <[e-mail]>
> > 
> > This particular nasty presented itself while trying to register the
> > intelfb device (intel_fbdev.c). During the process of registering the device
> > the driver will disable the crtc via i9xx_crtc_disable. These will
> > also disable the panel using the generic mipi panel functions in
> > dsi_mod_vbt_generic.c. The stale MIPI generic data sequence pointers would
> > cause a crash within those functions. However, all of this is happening
> > while console_lock is held from do_register_framebuffer inside fbcon.c. Which
> > means that you got kernel log and just the device appearing to reboot/hang for
> > no apparent reason.
> 
> CONFIG_I915_FBDEV=n for when the console_lock gets in the way.
> 
> > The fault started from the FB_EVENT_FB_REGISTERED event using the
> > fb_notifier_call_chain call in fbcon.c.
> > 
> > Cc: Shobhit Kumar <[e-mail]>
> > Signed-off-by: Rafael Barbalho <[e-mail]>
> 
> Queued for -next, thanks for the patch.

Actually this is for fixes since 3.16 has dsi support. Also for
regressions please cite the commit that introduced the offending
behaviour. I've added that.
-Daniel

> -Daniel
> > ---
> >  drivers/gpu/drm/i915/intel_bios.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
> > index 608ed30..a669550 100644
> > --- a/drivers/gpu/drm/i915/intel_bios.c
> > +++ b/drivers/gpu/drm/i915/intel_bios.c
> > @@ -878,7 +878,7 @@ err:
> >  
> >  	/* error during parsing so set all pointers to null
> >  	 * because of partial parsing */
> > -	memset(dev_priv->vbt.dsi.sequence, 0, MIPI_SEQ_MAX);
> > +	memset(dev_priv->vbt.dsi.sequence, 0, sizeof(dev_priv->vbt.dsi.sequence));
> >  }
> >  
> >  static void parse_ddi_port(struct drm_i915_private *dev_priv, enum port port,
> > -- 
> > 2.0.2
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > [e-mail]
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> [phone] - http://blog.ffwll.ch

Kumar, Shobhit July 25, 2014, 9:41 a.m. UTC | #4

On 7/25/2014 1:20 PM, Daniel Vetter wrote:
> On Fri, Jul 25, 2014 at 09:47:06AM +0200, Daniel Vetter wrote:
>> On Thu, Jul 24, 2014 at 03:16:12PM +0100, [e-mail] wrote:
>>> From: Rafael Barbalho <[e-mail]>
>>>
>>> This particular nasty presented itself while trying to register the
>>> intelfb device (intel_fbdev.c). During the process of registering the device
>>> the driver will disable the crtc via i9xx_crtc_disable. These will
>>> also disable the panel using the generic mipi panel functions in
>>> dsi_mod_vbt_generic.c. The stale MIPI generic data sequence pointers would
>>> cause a crash within those functions. However, all of this is happening
>>> while console_lock is held from do_register_framebuffer inside fbcon.c. Which
>>> means that you got kernel log and just the device appearing to reboot/hang for
>>> no apparent reason.
>>
>> CONFIG_I915_FBDEV=n for when the console_lock gets in the way.
>>
>>> The fault started from the FB_EVENT_FB_REGISTERED event using the
>>> fb_notifier_call_chain call in fbcon.c.
>>>
>>> Cc: Shobhit Kumar <[e-mail]>
>>> Signed-off-by: Rafael Barbalho <[e-mail]>
>>
>> Queued for -next, thanks for the patch.
>
> Actually this is for fixes since 3.16 has dsi support. Also for
> regressions please cite the commit that introduced the offending
> behaviour. I've added that.

Also this reminds me that there is still a WARN dump in 3.16 which will 
be fixed by -
[v2] drm/i915: Add correct hw/sw config check for DSI encoder

Assuming this can go in -fixes if it okay, this is waiting for review

Regards
Shobhit

Patch

diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
index 608ed30..a669550 100644
--- a/drivers/gpu/drm/i915/intel_bios.c
+++ b/drivers/gpu/drm/i915/intel_bios.c
@@ -878,7 +878,7 @@  err:
 
 	/* error during parsing so set all pointers to null
 	 * because of partial parsing */
-	memset(dev_priv->vbt.dsi.sequence, 0, MIPI_SEQ_MAX);
+	memset(dev_priv->vbt.dsi.sequence, 0, sizeof(dev_priv->vbt.dsi.sequence));
 }
 
 static void parse_ddi_port(struct drm_i915_private *dev_priv, enum port port,