Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] drm/client: splash: deferred restore during suspend skipped when n_scanout is 0
--
commit e78c6d64f4fdb91cb45b16a96f7a38008255ec63
Author: Francesco Valla [off-list ref]
drm: client: add splash client
This commit adds a DRM splash client capable of displaying an EFI BGRT logo,
a firmware-loaded 24bpp uncompressed BMP image, or a solid background color.
The client draws from a work item, handles hotplug events while keeping the
previous scanouts on screen until new ones are committed, and defers drawing
and restore commits while the device is suspended.
quoted hunk ↗ jump to hunk
diff --git a/drivers/gpu/drm/clients/drm_splash.c b/drivers/gpu/drm/clients/drm_splash.c
new file mode 100644
index 0000000000000..337c7160c4a1b
--- /dev/null
+++ b/drivers/gpu/drm/clients/drm_splash.c
[ ... ]
quoted hunk ↗ jump to hunk
+static void drm_splash_work(struct work_struct *work)
+{
+ struct drm_splash *splash = container_of(work, struct drm_splash, work);
+ struct drm_client_dev *client = &splash->client;
+ const u8 *img_data = NULL;
+ size_t img_data_len = 0;
+ bool img_loaded;
+ unsigned int i;
+ int ret;
+
+ guard(mutex)(&splash->lock);
+
+ /*
+ * Not while the device sleeps, not even drawing, which flushes the
* buffers to the hardware: resume queues the work again
+ */
+ if (splash->suspended) {
+ splash->work_skipped = true;
+ return;
+ }
+
+ if (!splash->n_scanout)
+ return;
[ ... ]
quoted hunk ↗ jump to hunk
+ ret = drm_client_modeset_commit(client);
+ if (ret) {
+ /* What a hotplug replaced is still on screen: it stays */
+ drm_dbg(client->dev, "splash: commit: %d\n", ret);
+ return;
+ }
+
+ /* Whatever a hotplug replaced is no longer on screen */
+ drm_splash_delete_scanouts(&splash->old_scanout, &splash->n_old_scanout);
+}
[ ... ]
quoted hunk ↗ jump to hunk
+/* An output without a buffer has no mode either: the commit switches it off */
+static int drm_splash_client_restore(struct drm_client_dev *client, bool force)
+{
+ struct drm_splash *splash = client_to_drm_splash(client);
+ int ret;
+
+ guard(mutex)(&splash->lock);
+
+ /* Not while the device sleeps: resume runs the work, which commits */
+ if (splash->suspended) {
+ splash->work_skipped = true;
+ return 0;
+ }
+
+ if (force)
+ ret = drm_client_modeset_commit_locked(client);
+ else
+ ret = drm_client_modeset_commit(client);
+
+ /* Whatever a hotplug replaced is no longer on screen */
+ if (!ret)
+ drm_splash_delete_scanouts(&splash->old_scanout,
+ &splash->n_old_scanout);
+
+ return ret;
+}
[Severity: Medium]
In drivers/gpu/drm/clients/drm_splash.c, can a restore deferred during
suspend be skipped on resume when splash->n_scanout is 0, or lose the force
flag?
If a hotplug event finds no usable modesets or fails buffer allocation,
drm_splash_client_hotplug() moves the previously active scanouts to
splash->old_scanout and calls drm_splash_free_scanout(), which clears
client->modesets via drm_splash_modeset_off() and leaves splash->n_scanout
at 0.
If drm_splash_client_restore() is then called while splash->suspended is
true (for example, via drm_lastclose() when a userspace master closes its
DRM file descriptor), it sets splash->work_skipped = true and returns 0,
relying on drm_splash_client_resume() to queue drm_splash_work() to perform
the commit.
However, when drm_splash_work() runs on resume, it checks:
if (!splash->n_scanout)
return;
and returns before calling drm_client_modeset_commit(client) and
drm_splash_delete_scanouts(&splash->old_scanout, &splash->n_old_scanout).
Wouldn't this leave the disabled modesets uncommitted and splash->old_scanout
unfreed?
Additionally, if drm_splash_client_restore(client, true) is called while
suspended, the force argument is not recorded, so drm_splash_work() will
call drm_client_modeset_commit() instead of
drm_client_modeset_commit_locked() and fail with -EBUSY if a DRM master is
still active.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261027135342.2003632-1-maximpedraza@gmail.com?part=4