Re: [PATCH 2/2] drm: panel: add support for Samsung S6E8AA5X01 panel controller

[Kaustabh Chakraborty <kauschluss@disroot.org>]

On 2025-06-13 09:39, Thomas Zimmermann wrote:
> Hi
> 
> Am 12.06.25 um 16:52 schrieb Kaustabh Chakraborty:
>> Samsung S6E8AA5X01 is an AMOLED MIPI DSI panel controller. Implement
>> a basic panel driver for such panels.
>>
>> The driver also initializes a backlight device, which works by changing
>> the panel's gamma values and aid brightness levels appropriately, with
>> the help of look-up tables acquired from downstream kernel sources.
>>
>> Signed-off-by: Kaustabh Chakraborty <kauschluss@disroot.org>

[...]

>> +
>> +static void s6e8aa5x01_mcs_protect(struct mipi_dsi_multi_context *dsi,
>> +				   struct s6e8aa5x01_ctx *ctx, bool protect)
> 
> I found this interface confusing. Rather split it up into .  It also does two different things AFAICT.
> 
> - The mcs_mutex protects against concurrent access from update_status and enable

mcs_mutex is meant to prevent any early access protection of the MCS commands.
Suppose there are two functions, A and B, accessing MCS.

ENTRY: A()
(access protection disabled)
...

ENTRY: B()
(access protection disabled)
...
(access protection enabled)
EXIT: B()

[!] cannot access MCS commands here anymore
(access protection enabled)
EXIT: A()

And to avoid such errors a mutex is provided.

> 
> - MSC_ACCESSPROT enable access to hardware state.
> 
> Maybe try this:
> 
> - Move msc_mutex into the callers, so that ->update_status and ->enable acquire and release the lock.
> 
> - Move MCS_ACCESSPROT into ->enable and ->disable and leave it accessible, if the hardware allows that.

Yeah this is a good idea, I'll try it.

>> +{
>> +	if (protect) {
>> +		mipi_dsi_dcs_write_seq_multi(dsi, MCS_ACCESSPROT, 0xa5, 0xa5);
>> +		mutex_unlock(&ctx->mcs_mutex);
>> +	} else {
>> +		mutex_lock(&ctx->mcs_mutex);
>> +		mipi_dsi_dcs_write_seq_multi(dsi, MCS_ACCESSPROT, 0x5a, 0x5a);
>> +	}
>> +}
>> +
>> +static int s6e8aa5x01_update_brightness(struct backlight_device *backlight)#
> 
> Maybe call this function s6e8aa5x01_update_status() to match the callback.
> 
>> +{
>> +	struct mipi_dsi_multi_context dsi = { .dsi = bl_get_data(backlight) };
>> +	struct s6e8aa5x01_ctx *ctx = mipi_dsi_get_drvdata(dsi.dsi);
>> +	u16 lvl = backlight->props.brightness;
> 
> backlight_get_brightness() here ?
> 
> 
> I think you should also check panel->enabled and return if false. AFAIU there will be no gamma changes on disabled hardware anyway.
>

The enable function is never executed when the panel is disabled. This is
because flag checking is done by drm_panel anyway. See drm_panel_enable()
in drivers/gpu/drm/drm_panel.c [1]

>> +
>> +static int s6e8aa5x01_probe(struct mipi_dsi_device *dsi)
>> +{
>> +	struct device *dev = &dsi->dev;
>> +	struct s6e8aa5x01_ctx *ctx;
>> +	int ret;
>> +
>> +	ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
> 
> You're possibly using the instance after the hardware device has been removed. Alloc with drmm_kzalloc() or you might end up with UAF errors.

Hmm, none of the panel drivers are using drmm_kzalloc(), or even any
drmm_*(). Are you sure I must use it?

>> +	ret = devm_mutex_init(dev, &ctx->mcs_mutex);
> 
> You're taking this mutex in DRM code, so rather use drmm_mutex_init() here.

(The comment by me above applies here too)

> 
> Best regards
> Thomas

[1] https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/drivers/gpu/drm/drm_panel.c#n209