| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <a88a6e48-8c55-410b-b553-8942dac3608a@suse.de>
Date: Wed, 19 Feb 2025 08:52:44 +0100
From: Thomas Zimmermann <tzimmermann@...e.de>
To: Aditya Garg <gargaditya08@...e.com>
Cc: "maarten.lankhorst@...ux.intel.com" <maarten.lankhorst@...ux.intel.com>,
"mripard@...nel.org" <mripard@...nel.org>,
"airlied@...il.com" <airlied@...il.com>, "simona@...ll.ch"
<simona@...ll.ch>, Kerem Karabay <kekrby@...il.com>,
Atharva Tiwari <evepolonium@...il.com>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
"dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>
Subject: Re: [PATCH 2/2] drm/tiny: add driver for Apple Touch Bars in x86 Macs
Hi
Am 18.02.25 um 14:49 schrieb Aditya Garg:
[...]
>> This code runs in the middle of the atomic update. It's then too late to do error handling. If these allocs fail, the display hardware will remain in undefined state.
>>
>> It is much preferable to allocate this memory in the atomic_check. To do so, you need a plane-state structure. Allocate the memory for request and response in the plane's atomic-check helper. If that fails, you can return an error there. Store the pointers and the request size in your plane-state structure and fetch them here. The memory should later be freed in the plane's destroy callback. For an example, see how the ssd130x driver treats a buffer for a similar use case at [1].
>>
>> [1] https://elixir.bootlin.com/linux/v6.13.2/source/drivers/gpu/drm/solomon/ssd130x.c#L1136
> I’ve tried these changes, seem to be breaking the driver:
>
> —>8—
> From 16c920cabf65ec664663ebe1611c0ccf6e81de4a Mon Sep 17 00:00:00 2001
> From: Aditya Garg <gargaditya08@...e.com>
> Date: Tue, 18 Feb 2025 18:54:10 +0530
> Subject: [PATCH] better error handling
>
> ---
> .../apple-touchbar-advanced-0.1/appletbdrm.c | 68 +++++++++++++------
> 1 file changed, 46 insertions(+), 22 deletions(-)
>
> diff --git a/usr/src/apple-touchbar-advanced-0.1/appletbdrm.c b/usr/src/apple-touchbar-advanced-0.1/appletbdrm.c
> index f2d9113..cb13b36 100644
> --- a/usr/src/apple-touchbar-advanced-0.1/appletbdrm.c
> +++ b/usr/src/apple-touchbar-advanced-0.1/appletbdrm.c
> @@ -133,6 +133,17 @@ struct appletbdrm_device {
> struct drm_encoder encoder;
> };
>
> +struct appletbdrm_plane_state {
> + struct drm_shadow_plane_state base;
> + u8 *request_buffer;
> + u8 *response_buffer;
> +};
> +
> +static inline struct appletbdrm_plane_state *to_appletbdrm_plane_state(struct drm_plane_state *state)
> +{
> + return container_of(state, struct appletbdrm_plane_state, base.base);
> +}
> +
> static int appletbdrm_send_request(struct appletbdrm_device *adev,
> struct appletbdrm_msg_request_header *request, size_t size)
> {
> @@ -311,24 +322,6 @@ static int appletbdrm_flush_damage(struct appletbdrm_device *adev,
> if (!frames_size)
> return 0;
>
> - request_size = ALIGN(sizeof(*request) + frames_size + sizeof(*footer), 16);
> -
> - request = kzalloc(request_size, GFP_KERNEL);
> - if (!request)
> - return -ENOMEM;
> -
> - response = kzalloc(sizeof(*response), GFP_KERNEL);
> - if (!response) {
> - ret = -ENOMEM;
> - goto free_request;
> - }
> -
> - ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
> - if (ret) {
> - drm_err(drm, "Failed to start CPU framebuffer access (%d)\n", ret);
> - goto free_response;
> - }
> -
> request->header.unk_00 = cpu_to_le16(2);
> request->header.unk_02 = cpu_to_le16(0x12);
> request->header.unk_04 = cpu_to_le32(9);
> @@ -389,10 +382,6 @@ static int appletbdrm_flush_damage(struct appletbdrm_device *adev,
>
> end_fb_cpu_access:
> drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
> -free_response:
> - kfree(response);
> -free_request:
> - kfree(request);
>
> return ret;
> }
> @@ -415,6 +404,15 @@ static int appletbdrm_primary_plane_helper_atomic_check(struct drm_plane *plane,
> struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(state, plane);
> struct drm_crtc *new_crtc = new_plane_state->crtc;
> struct drm_crtc_state *new_crtc_state = NULL;
> + struct appletbdrm_plane_state *appletbdrm_state = to_appletbdrm_plane_state(new_plane_state);
> + struct drm_device *drm = plane->dev;
> + struct drm_plane_state *plane_state = plane->state;
> + struct appletbdrm_fb_request_response *response;
> + struct appletbdrm_fb_request_footer *footer;
> + struct drm_framebuffer *fb = plane_state->fb;
> + struct appletbdrm_fb_request *request;
> + size_t frames_size = 0;
> + size_t request_size;
> int ret;
>
> if (new_crtc)
> @@ -429,6 +427,22 @@ static int appletbdrm_primary_plane_helper_atomic_check(struct drm_plane *plane,
> else if (!new_plane_state->visible)
> return 0;
>
> + request_size = ALIGN(sizeof(*request) + frames_size + sizeof(*footer), 16);
> +
> + appletbdrm_state->request_buffer = kzalloc(request_size, GFP_KERNEL);
> + if (!request)
> + return -ENOMEM;
> +
> + appletbdrm_state->response_buffer = kzalloc(sizeof(*response), GFP_KERNEL);
> + if (!response) {
> + ret = -ENOMEM;
> + }
> +
> + ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
> + if (ret) {
> + drm_err(drm, "Failed to start CPU framebuffer access (%d)\n", ret);
> + }
> +
> return 0;
> }
>
> @@ -464,6 +478,15 @@ static void appletbdrm_primary_plane_helper_atomic_disable(struct drm_plane *pla
> drm_dev_exit(idx);
> }
>
> +static void appletbdrm_primary_plane_destroy_state(struct drm_plane *plane,
> + struct drm_plane_state *state)
> +{
> + struct appletbdrm_plane_state *appletbdrm_state = to_appletbdrm_plane_state(state);
> +
> + kfree(appletbdrm_state->request_buffer);
> + kfree(appletbdrm_state->response_buffer);
> +}
> +
> static const struct drm_plane_helper_funcs appletbdrm_primary_plane_helper_funcs = {
> DRM_GEM_SHADOW_PLANE_HELPER_FUNCS,
> .atomic_check = appletbdrm_primary_plane_helper_atomic_check,
> @@ -474,6 +497,7 @@ static const struct drm_plane_helper_funcs appletbdrm_primary_plane_helper_funcs
> static const struct drm_plane_funcs appletbdrm_primary_plane_funcs = {
> .update_plane = drm_atomic_helper_update_plane,
> .disable_plane = drm_atomic_helper_disable_plane,
> + .atomic_destroy_state = appletbdrm_primary_plane_destroy_state,
> .destroy = drm_plane_cleanup,
> DRM_GEM_SHADOW_PLANE_FUNCS,
You don't allocate struct appletbdrm_plane_state. Instead of this macro,
you also have to set your own helpers for the plane's .reset and
.atomic_duplicate_state There's again example code in the ssd130x driver.
Best regards
Thomas
> };
--
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)
Powered by blists - more mailing lists