| Message ID | 20201007133036.1541639-1-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/fb-helper: Add locking to sysrq handling | expand |
Hi Am 07.10.20 um 15:30 schrieb Daniel Vetter: > We didn't take the kernel_fb_helper_lock mutex, which protects that > code. While at it, simplify the code > - inline the function (originally shared with kgdb I think) > - drop the error tracking and all the complications > - drop the pointless early out, it served nothing > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Cc: Maxime Ripard <mripard@kernel.org> > Cc: Thomas Zimmermann <tzimmermann@suse.de> > Cc: David Airlie <airlied@linux.ie> > Cc: Daniel Vetter <daniel@ffwll.ch> > --- > drivers/gpu/drm/drm_fb_helper.c | 26 +++++--------------------- > 1 file changed, 5 insertions(+), 21 deletions(-) > > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c > index 8697554ccd41..c2f72bb6afb1 100644 > --- a/drivers/gpu/drm/drm_fb_helper.c > +++ b/drivers/gpu/drm/drm_fb_helper.c > @@ -281,18 +281,12 @@ int drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper) > EXPORT_SYMBOL(drm_fb_helper_restore_fbdev_mode_unlocked); > > #ifdef CONFIG_MAGIC_SYSRQ > -/* > - * restore fbcon display for all kms driver's using this helper, used for sysrq > - * and panic handling. > - */ > -static bool drm_fb_helper_force_kernel_mode(void) > +/* emergency restore, don't bother with error reporting */ > +static void drm_fb_helper_restore_work_fn(struct work_struct *ignored) > { > - bool ret, error = false; > struct drm_fb_helper *helper; > > - if (list_empty(&kernel_fb_helper_list)) > - return false; > - > + mutex_lock(&kernel_fb_helper_lock); > list_for_each_entry(helper, &kernel_fb_helper_list, kernel_fb_list) { > struct drm_device *dev = helper->dev; > > @@ -300,22 +294,12 @@ static bool drm_fb_helper_force_kernel_mode(void) > continue; > > mutex_lock(&helper->lock); > - ret = drm_client_modeset_commit_locked(&helper->client); > - if (ret) > - error = true; > + drm_client_modeset_commit_locked(&helper->client); > mutex_unlock(&helper->lock); > } > - return error; > + mutex_unlock(&kernel_fb_helper_lock); > } > > -static void drm_fb_helper_restore_work_fn(struct work_struct *ignored) > -{ > - bool ret; > - > - ret = drm_fb_helper_force_kernel_mode(); > - if (ret == true) > - DRM_ERROR("Failed to restore crtc configuration\n"); Is there a specific reason for removing that warning? Even if it doesn't show up on screen, is it not helpful in the kernel's log? In any case, the rest looks good. Acked-by: Thomas Zimmermann <tzimmermann@suse.de> Best regards Thomas > -} > static DECLARE_WORK(drm_fb_helper_restore_work, drm_fb_helper_restore_work_fn); > > static void drm_fb_helper_sysrq(int dummy1) >
On Thu, Oct 8, 2020 at 11:22 AM Thomas Zimmermann <tzimmermann@suse.de> wrote: > > Hi > > Am 07.10.20 um 15:30 schrieb Daniel Vetter: > > We didn't take the kernel_fb_helper_lock mutex, which protects that > > code. While at it, simplify the code > > - inline the function (originally shared with kgdb I think) > > - drop the error tracking and all the complications > > - drop the pointless early out, it served nothing > > > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > Cc: Maxime Ripard <mripard@kernel.org> > > Cc: Thomas Zimmermann <tzimmermann@suse.de> > > Cc: David Airlie <airlied@linux.ie> > > Cc: Daniel Vetter <daniel@ffwll.ch> > > --- > > drivers/gpu/drm/drm_fb_helper.c | 26 +++++--------------------- > > 1 file changed, 5 insertions(+), 21 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c > > index 8697554ccd41..c2f72bb6afb1 100644 > > --- a/drivers/gpu/drm/drm_fb_helper.c > > +++ b/drivers/gpu/drm/drm_fb_helper.c > > @@ -281,18 +281,12 @@ int drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper) > > EXPORT_SYMBOL(drm_fb_helper_restore_fbdev_mode_unlocked); > > > > #ifdef CONFIG_MAGIC_SYSRQ > > -/* > > - * restore fbcon display for all kms driver's using this helper, used for sysrq > > - * and panic handling. > > - */ > > -static bool drm_fb_helper_force_kernel_mode(void) > > +/* emergency restore, don't bother with error reporting */ > > +static void drm_fb_helper_restore_work_fn(struct work_struct *ignored) > > { > > - bool ret, error = false; > > struct drm_fb_helper *helper; > > > > - if (list_empty(&kernel_fb_helper_list)) > > - return false; > > - > > + mutex_lock(&kernel_fb_helper_lock); > > list_for_each_entry(helper, &kernel_fb_helper_list, kernel_fb_list) { > > struct drm_device *dev = helper->dev; > > > > @@ -300,22 +294,12 @@ static bool drm_fb_helper_force_kernel_mode(void) > > continue; > > > > mutex_lock(&helper->lock); > > - ret = drm_client_modeset_commit_locked(&helper->client); > > - if (ret) > > - error = true; > > + drm_client_modeset_commit_locked(&helper->client); > > mutex_unlock(&helper->lock); > > } > > - return error; > > + mutex_unlock(&kernel_fb_helper_lock); > > } > > > > -static void drm_fb_helper_restore_work_fn(struct work_struct *ignored) > > -{ > > - bool ret; > > - > > - ret = drm_fb_helper_force_kernel_mode(); > > - if (ret == true) > > - DRM_ERROR("Failed to restore crtc configuration\n"); > > Is there a specific reason for removing that warning? Even if it doesn't > show up on screen, is it not helpful in the kernel's log? I just found it really unhelpful, if you're trying to force-show fbcon, what's the point if it doesn't work out? The user will notice. Also, if we're really in a dire situation where you want this, in my experience there's a bunch of random other reasons why this can fail, mostly when the work we have to schedule for locking reasons never runs. So it's also unreliable. > In any case, the rest looks good. > > Acked-by: Thomas Zimmermann <tzimmermann@suse.de> Thanks for taking a look, I'll apply it. -Daniel > > Best regards > Thomas > > > -} > > static DECLARE_WORK(drm_fb_helper_restore_work, drm_fb_helper_restore_work_fn); > > > > static void drm_fb_helper_sysrq(int dummy1) > > > > -- > Thomas Zimmermann > Graphics Driver Developer > SUSE Software Solutions Germany GmbH > Maxfeldstr. 5, 90409 Nürnberg, Germany > (HRB 36809, AG Nürnberg) > Geschäftsführer: Felix Imendörffer >
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index 8697554ccd41..c2f72bb6afb1 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -281,18 +281,12 @@ int drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper) EXPORT_SYMBOL(drm_fb_helper_restore_fbdev_mode_unlocked); #ifdef CONFIG_MAGIC_SYSRQ -/* - * restore fbcon display for all kms driver's using this helper, used for sysrq - * and panic handling. - */ -static bool drm_fb_helper_force_kernel_mode(void) +/* emergency restore, don't bother with error reporting */ +static void drm_fb_helper_restore_work_fn(struct work_struct *ignored) { - bool ret, error = false; struct drm_fb_helper *helper; - if (list_empty(&kernel_fb_helper_list)) - return false; - + mutex_lock(&kernel_fb_helper_lock); list_for_each_entry(helper, &kernel_fb_helper_list, kernel_fb_list) { struct drm_device *dev = helper->dev; @@ -300,22 +294,12 @@ static bool drm_fb_helper_force_kernel_mode(void) continue; mutex_lock(&helper->lock); - ret = drm_client_modeset_commit_locked(&helper->client); - if (ret) - error = true; + drm_client_modeset_commit_locked(&helper->client); mutex_unlock(&helper->lock); } - return error; + mutex_unlock(&kernel_fb_helper_lock); } -static void drm_fb_helper_restore_work_fn(struct work_struct *ignored) -{ - bool ret; - - ret = drm_fb_helper_force_kernel_mode(); - if (ret == true) - DRM_ERROR("Failed to restore crtc configuration\n"); -} static DECLARE_WORK(drm_fb_helper_restore_work, drm_fb_helper_restore_work_fn); static void drm_fb_helper_sysrq(int dummy1)
We didn't take the kernel_fb_helper_lock mutex, which protects that code. While at it, simplify the code - inline the function (originally shared with kgdb I think) - drop the error tracking and all the complications - drop the pointless early out, it served nothing Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Cc: Maxime Ripard <mripard@kernel.org> Cc: Thomas Zimmermann <tzimmermann@suse.de> Cc: David Airlie <airlied@linux.ie> Cc: Daniel Vetter <daniel@ffwll.ch> --- drivers/gpu/drm/drm_fb_helper.c | 26 +++++--------------------- 1 file changed, 5 insertions(+), 21 deletions(-)