Re: [PATCH] drm: Check for connector->state NULL in drm_atomic_add_affected_connectors

[Ville Syrjälä]

On Sun, Mar 06, 2016 at 05:29:51PM +0100, Daniel Vetter wrote:
> On Fri, Mar 04, 2016 at 03:43:53PM -0800, Douglas Anderson wrote:
> > On a system I'm doing development on I found a crash.  The crawl looked
> > like:
> > 
> >   PC is at drm_atomic_add_affected_connectors+0x98/0xe8
> >   ...
> >   drm_atomic_add_affected_connectors from __drm_atomic_helper_set_config+0x218/0x344
> >   __drm_atomic_helper_set_config from restore_fbdev_mode+0x108/0x250
> >   restore_fbdev_mode from drm_fb_helper_restore_fbdev_mode_unlocked+0x3c/0x80
> >   drm_fb_helper_restore_fbdev_mode_unlocked from rockchip_drm_lastclose+0x1c/0x20
> >   rockchip_drm_lastclose from drm_lastclose+0x4c/0x104
> >   drm_lastclose from drm_release+0x424/0x47c
> >   drm_release from __fput+0xf8/0x1d4
> >   __fput from ____fput+0x18/0x1c
> >   ____fput from task_work_run+0xa8/0xbc
> >   task_work_run from do_exit+0x448/0x91c
> >   do_exit from do_group_exit+0x5c/0xcc
> >   do_group_exit from get_signal+0x4dc/0x57c
> >   get_signal from do_signal+0x9c/0x3b4
> >   do_signal from do_work_pending+0x60/0xb8
> >   do_work_pending from slow_work_pending+0xc/0x20
> > 
> > I found that I could fix the crash by checking connector->state against
> > NULL.  This isn't code I'm familiar with and I didn't dig too deep, so
> > I'd appreciate any opinions about whether this is a sane thing to do.
> > Note that I don't actually have graphics up on my system at the moment,
> > so perhaps this is all just a symptom of the strange state I'm in.
> > 
> > Signed-off-by: Douglas Anderson <dianders@xxxxxxxxxxxx>
> 
> This is a driver bug - under atomic the assumption is that there is
> _always_ a current software state. Most driver set up the initial
> "everything off" state in the ->reset functions.

Except if it's due to kzalloc() failing in the atomic helper reset
implementation. There's no way to propagate the error upwards, so
the current implementation just silently sets state=NULL and goes on
like nothing happened.

-- 
Ville Syrjälä
Intel OTC