Re: [PATCH v6 04/15] drm: client: add splash client

[sashiko-bot@kernel.org]

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 <francesco@valla.it>

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.

> 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
> @@ -0,0 +1,976 @@

[ ... ]

> +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;

[ ... ]

> +	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);
> +}

[ ... ]

> +/* 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