linux-kernel - Re: [PATCH] drm/ssd130x: Add drm

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Fri, 21 Jun 2024 17:42:53 +0200
From: Javier Martinez Canillas <javierm@...hat.com>
To: Jocelyn Falempe <jfalempe@...hat.com>, linux-kernel@...r.kernel.org
Cc: Maxime Ripard <mripard@...nel.org>, Daniel Vetter <daniel@...ll.ch>,
 David Airlie <airlied@...il.com>, Maarten Lankhorst
 <maarten.lankhorst@...ux.intel.com>, Thomas Zimmermann
 <tzimmermann@...e.de>, dri-devel@...ts.freedesktop.org
Subject: Re: [PATCH] drm/ssd130x: Add drm_panic support

Javier Martinez Canillas <javierm@...hat.com> writes:

> Jocelyn Falempe <jfalempe@...hat.com> writes:
>
> Hello Jocelyn, thanks for your feedback!
>
>> On 21/06/2024 00:22, Javier Martinez Canillas wrote:
>>> Add support for the drm_panic infrastructure, which allows to display
>>> a user friendly message on the screen when a Linux kernel panic occurs.
>>> 
>>> The display controller doesn't scanout the framebuffer, but instead the
>>> pixels are sent to the device using a transport bus. For this reason, a
>>> .panic_flush handler is needed to flush the panic image to the display.
>>
>> Thanks for this patch, that's really cool that drm_panic can work on 
>> this device too.
>>
>
> Indeed, that's why I did it. Just to see if it could work :)
>
> [...]
>
>>> +static void ssd130x_primary_plane_helper_panic_flush(struct drm_plane *plane)
>>> +{
>>> +	struct drm_plane_state *plane_state = plane->state;
>>> +	struct ssd130x_plane_state *ssd130x_plane_state = to_ssd130x_plane_state(plane_state);
>>> +	struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(plane_state);
>>> +	struct drm_crtc *crtc = plane_state->crtc;
>>> +	struct ssd130x_crtc_state *ssd130x_crtc_state = to_ssd130x_crtc_state(crtc->state);
>>> +
>>> +	ssd130x_fb_blit_rect(plane_state->fb, &shadow_plane_state->data[0], &plane_state->dst,
>>> +			     ssd130x_plane_state->buffer, ssd130x_crtc_state->data_array,
>>> +			     &shadow_plane_state->fmtcnv_state);
>>
>> ssd130x_fb_blit_rect() will call regmap->write(), which involve mutex 
>> and might sleep. And if the mutex is taken when the panic occurs, it 
>> might deadlock the panic handling.
>
> That's a good point and I something haven't considered...
>
>> One solution would be to configure the regmap with config->fast_io and 
>> config->use_raw_spinlock, and check that the lock is available with 
>> try_lock(map->raw_spin_lock)
>> But that means it will waste cpu cycle with busy waiting for normal 
>> operation, which is not good.
>>
>
> Yeah, I would prefer to not change the driver for normal operation.
>

Another option, that I believe makes more sense, is to just disable the
regmap locking (using struct regmap_config.disable_locking field [0]).

Since this regmap is not shared with other drivers and so any concurrent
access should already be prevented by the DRM core locking scheme.

Is my understanding correct?

[0]: https://elixir.bootlin.com/linux/v6.10-rc1/source/include/linux/regmap.h#L326

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat