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 <[email protected]>
[...]
>> +
>> +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
