From 40eba398198212383af53f165e4cd4510b9244b4 Mon Sep 17 00:00:00 2001 From: Javier Martinez Canillas Date: Mon, 17 Jul 2023 18:30:22 +0200 Subject: [PATCH] drm/ssd130x: Fix an oops when attempting to update a disabled plane Geert reports that the following NULL pointer dereference happens for him after commit 49d7d581ceaf ("drm/ssd130x: Don't allocate buffers on each plane update"): [drm] Initialized ssd130x 1.0.0 20220131 for 0-003c on minor 0 ssd130x-i2c 0-003c: [drm] surface width(128), height(32), bpp(1) and format(R1 little-endian (0x20203152)) Unable to handle kernel NULL pointer dereference at virtual address 00000000 Oops [#1] CPU: 0 PID: 1 Comm: swapper Not tainted 6.5.0-rc1-orangecrab-02219-g0a529a1e4bf4 #565 epc : ssd130x_update_rect.isra.0+0x13c/0x340 ra : ssd130x_update_rect.isra.0+0x2bc/0x340 ... status: 00000120 badaddr: 00000000 cause: 0000000f [] ssd130x_update_rect.isra.0+0x13c/0x340 [] ssd130x_primary_plane_helper_atomic_update+0x26c/0x284 [] drm_atomic_helper_commit_planes+0xfc/0x27c [] drm_atomic_helper_commit_tail+0x5c/0xb4 [] commit_tail+0x190/0x1b8 [] drm_atomic_helper_commit+0x194/0x1c0 [] drm_atomic_commit+0xa4/0xe4 [] drm_client_modeset_commit_atomic+0x244/0x278 [] drm_client_modeset_commit_locked+0x7c/0x1bc [] drm_client_modeset_commit+0x34/0x64 [] __drm_fb_helper_restore_fbdev_mode_unlocked+0xc4/0xe8 [] drm_fb_helper_set_par+0x38/0x58 [] fbcon_init+0x294/0x534 ... The problem is that fbcon calls fbcon_init() which triggers a DRM modeset and this leads to drm_atomic_helper_commit_planes() attempting to commit the atomic state for all planes, even the ones whose CRTC is not enabled. Since the primary plane buffer is allocated in the encoder .atomic_enable callback, this happens after that initial modeset commit and leads to the mentioned NULL pointer dereference. Fix this by allocating the buffers in the struct drm_plane_helper_funcs .begin_fb_access callback and free them in to .end_fb_access callback, instead of doing it when the encoder is enabled. Fixes: 49d7d581ceaf ("drm/ssd130x: Don't allocate buffers on each plane update") Reported-by: Geert Uytterhoeven Signed-off-by: Javier Martinez Canillas --- drivers/gpu/drm/solomon/ssd130x.c | 33 ++++++++++++++++++++++++------- 1 file changed, 26 insertions(+), 7 deletions(-) diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c index afb08a8aa9fcda..be68b63200a0a9 100644 --- a/drivers/gpu/drm/solomon/ssd130x.c +++ b/drivers/gpu/drm/solomon/ssd130x.c @@ -612,6 +612,30 @@ static int ssd130x_fb_blit_rect(struct drm_framebuffer *fb, const struct iosys_m return ret; } +static int ssd130x_primary_plane_helper_begin_fb_access(struct drm_plane *plane, + struct drm_plane_state *state) +{ + struct drm_device *drm = plane->dev; + struct ssd130x_device *ssd130x = drm_to_ssd130x(drm); + int ret = ssd130x_buf_alloc(ssd130x); + + if (ret) + return ret; + + return drm_gem_begin_shadow_fb_access(plane, state); +} + +static void ssd130x_primary_plane_helper_end_fb_access(struct drm_plane *plane, + struct drm_plane_state *state) +{ + struct drm_device *drm = plane->dev; + struct ssd130x_device *ssd130x = drm_to_ssd130x(drm); + + ssd130x_buf_free(ssd130x); + + return drm_gem_end_shadow_fb_access(plane, state); +} + static void ssd130x_primary_plane_helper_atomic_update(struct drm_plane *plane, struct drm_atomic_state *state) { @@ -656,7 +680,8 @@ static void ssd130x_primary_plane_helper_atomic_disable(struct drm_plane *plane, } static const struct drm_plane_helper_funcs ssd130x_primary_plane_helper_funcs = { - DRM_GEM_SHADOW_PLANE_HELPER_FUNCS, + .begin_fb_access = ssd130x_primary_plane_helper_begin_fb_access, + .end_fb_access = ssd130x_primary_plane_helper_end_fb_access, .atomic_check = drm_plane_helper_atomic_check, .atomic_update = ssd130x_primary_plane_helper_atomic_update, .atomic_disable = ssd130x_primary_plane_helper_atomic_disable, @@ -719,10 +744,6 @@ static void ssd130x_encoder_helper_atomic_enable(struct drm_encoder *encoder, if (ret) goto power_off; - ret = ssd130x_buf_alloc(ssd130x); - if (ret) - goto power_off; - ssd130x_write_cmd(ssd130x, 1, SSD130X_DISPLAY_ON); backlight_enable(ssd130x->bl_dev); @@ -744,8 +765,6 @@ static void ssd130x_encoder_helper_atomic_disable(struct drm_encoder *encoder, ssd130x_write_cmd(ssd130x, 1, SSD130X_DISPLAY_OFF); - ssd130x_buf_free(ssd130x); - ssd130x_power_off(ssd130x); }