[PATCH v2] drm/ssd130x: Fix an oops when attempting to update a disabled plane
From: Javier Martinez Canillas
Date: Mon Jul 17 2023 - 06:30:35 EST
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 000=
00000
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
[<c0303d90>] ssd130x_update_rect.isra.0+0x13c/0x340
[<c0304200>] ssd130x_primary_plane_helper_atomic_update+0x26c/0x284
[<c02f8d54>] drm_atomic_helper_commit_planes+0xfc/0x27c
[<c02f9314>] drm_atomic_helper_commit_tail+0x5c/0xb4
[<c02f94fc>] commit_tail+0x190/0x1b8
[<c02f99fc>] drm_atomic_helper_commit+0x194/0x1c0
[<c02c5d00>] drm_atomic_commit+0xa4/0xe4
[<c02cce40>] drm_client_modeset_commit_atomic+0x244/0x278
[<c02ccef0>] drm_client_modeset_commit_locked+0x7c/0x1bc
[<c02cd064>] drm_client_modeset_commit+0x34/0x64
[<c0301a78>] __drm_fb_helper_restore_fbdev_mode_unlocked+0xc4/0xe8
[<c0303424>] drm_fb_helper_set_par+0x38/0x58
[<c027c410>] 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_mode_config_funcs
.fb_create, instead of doing it when the encoder is enabled.
Also, make a couple of improvements to the ssd130x_buf_alloc() function:
* Make the allocation smarter to only allocate the intermediate buffer
if the native R1 format is not used. Otherwise is not needed, since
there is no XRGB8888 to R1 format conversion during plane updates.
* Allocate the buffers as device managed resources, this is enough and
simplifies the logic since there is no need to explicitly free them.
Reported-by: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
Signed-off-by: Javier Martinez Canillas <javierm@xxxxxxxxxx>
---
Changes in v2:
- Move the buffers allocation to .fb_create instead of preventing atomic
updates for disable planes.
- Don't allocate the intermediate buffer when using the native R1 format.
- Allocate buffers as device managed resources.
drivers/gpu/drm/solomon/ssd130x.c | 54 +++++++++++++++++--------------
1 file changed, 29 insertions(+), 25 deletions(-)
diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ss=
d130x.c
index b8c90d507a35..615805a066de 100644
--- a/drivers/gpu/drm/solomon/ssd130x.c
+++ b/drivers/gpu/drm/solomon/ssd130x.c
@@ -146,38 +146,35 @@ static inline struct ssd130x_device *drm_to_ssd130x(s=
truct drm_device *drm)
return container_of(drm, struct ssd130x_device, drm);
}
=20
-static int ssd130x_buf_alloc(struct ssd130x_device *ssd130x)
+static int ssd130x_buf_alloc(struct ssd130x_device *ssd130x, __u32 pixel_f=
ormat)
{
unsigned int page_height =3D ssd130x->device_info->page_height;
unsigned int pages =3D DIV_ROUND_UP(ssd130x->height, page_height);
const struct drm_format_info *fi;
unsigned int pitch;
=20
- fi =3D drm_format_info(DRM_FORMAT_R1);
- if (!fi)
- return -EINVAL;
+ /* If the native format is not used an intermediate buffer is needed */
+ if (pixel_format !=3D DRM_FORMAT_R1) {
+ fi =3D drm_format_info(DRM_FORMAT_R1);
+ if (!fi)
+ return -EINVAL;
=20
- pitch =3D drm_format_info_min_pitch(fi, 0, ssd130x->width);
+ pitch =3D drm_format_info_min_pitch(fi, 0, ssd130x->width);
=20
- ssd130x->buffer =3D kcalloc(pitch, ssd130x->height, GFP_KERNEL);
- if (!ssd130x->buffer)
- return -ENOMEM;
+ ssd130x->buffer =3D devm_kcalloc(ssd130x->dev, pitch, ssd130x->height,
+ GFP_KERNEL);
+ if (!ssd130x->buffer)
+ return -ENOMEM;
+ }
=20
- ssd130x->data_array =3D kcalloc(ssd130x->width, pages, GFP_KERNEL);
- if (!ssd130x->data_array) {
- kfree(ssd130x->buffer);
+ ssd130x->data_array =3D devm_kcalloc(ssd130x->dev, ssd130x->width, pages,
+ GFP_KERNEL);
+ if (!ssd130x->data_array)
return -ENOMEM;
- }
=20
return 0;
}
=20
-static void ssd130x_buf_free(struct ssd130x_device *ssd130x)
-{
- kfree(ssd130x->data_array);
- kfree(ssd130x->buffer);
-}
-
/*
* Helper to write data (SSD130X_DATA) to the device.
*/
@@ -741,10 +738,6 @@ static void ssd130x_encoder_helper_atomic_enable(struc=
t drm_encoder *encoder,
if (ret)
goto power_off;
=20
- ret =3D ssd130x_buf_alloc(ssd130x);
- if (ret)
- goto power_off;
-
ssd130x_write_cmd(ssd130x, 1, SSD130X_DISPLAY_ON);
=20
backlight_enable(ssd130x->bl_dev);
@@ -766,8 +759,6 @@ static void ssd130x_encoder_helper_atomic_disable(struc=
t drm_encoder *encoder,
=20
ssd130x_write_cmd(ssd130x, 1, SSD130X_DISPLAY_OFF);
=20
- ssd130x_buf_free(ssd130x);
-
ssd130x_power_off(ssd130x);
}
=20
@@ -811,8 +802,21 @@ static const struct drm_connector_funcs ssd130x_connec=
tor_funcs =3D {
.atomic_destroy_state =3D drm_atomic_helper_connector_destroy_state,
};
=20
+static struct drm_framebuffer *ssd130x_fb_create(struct drm_device *dev, s=
truct drm_file *file,
+ const struct drm_mode_fb_cmd2 *mode_cmd)
+{
+ struct ssd130x_device *ssd130x =3D drm_to_ssd130x(dev);
+ int ret;
+
+ ret =3D ssd130x_buf_alloc(ssd130x, mode_cmd->pixel_format);
+ if (ret)
+ return ERR_PTR(ret);
+
+ return drm_gem_fb_create_with_dirty(dev, file, mode_cmd);
+}
+
static const struct drm_mode_config_funcs ssd130x_mode_config_funcs =3D {
- .fb_create =3D drm_gem_fb_create_with_dirty,
+ .fb_create =3D ssd130x_fb_create,
.atomic_check =3D drm_atomic_helper_check,
.atomic_commit =3D drm_atomic_helper_commit,
};
--=20
2.41.0
--=20
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat