Re: [PATCH v3 2/4] drm/panel: Add driver for Novatek NT37705 panel

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] drm/panel: resource leak in nt37705_prepare() error path
- [High] drm/panel: unsafe DSI access from backlight sysfs when panel is unpowered
- [Medium] drm/panel: race condition and state corruption in backlight sysfs handler
- [Medium] drm/panel: DCS protocol violation sending extra parameters
- [Medium] drm/panel: premature exposure via devm_drm_panel_add() before DSC init
--

commit 512ec900afa4de1ceeb7c942bc90e389f2a7b45d
Author: Luca Weiss <luca.weiss@fairphone.com>

    drm/panel: Add driver for Novatek NT37705 panel
    
    Add support for the 2484x1116 AMOLED panel from BOE (BJ631JHM-T71-D900)
    bundled with a NT37705 driver IC, as found on the Fairphone (Gen. 6)
    smartphone.
    
    The panel can also be configured in 10-bit (RGB101010) mode, however
    currently it's configured in 8-bit (RGB888) since there's some issues in
    the Qualcomm DPU driver when driving this panel in 10-bit.

> diff --git a/drivers/gpu/drm/panel/panel-novatek-nt37705.c b/drivers/gpu/drm/panel/panel-novatek-nt37705.c
> new file mode 100644
> index 0000000000000..3cf092a1f360d
> --- /dev/null
> +++ b/drivers/gpu/drm/panel/panel-novatek-nt37705.c
[ ... ]
> +	mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0x11, 0x00);
> +	mipi_dsi_msleep(&dsi_ctx, 120);
> +	mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0x29, 0x00);
> +	mipi_dsi_msleep(&dsi_ctx, 22);
> +
> +	return dsi_ctx.accum_err;
> +}

[Severity: Medium]
Does this code send extra parameters for zero-parameter DCS commands?

The DCS specification defines MIPI_DCS_EXIT_SLEEP_MODE (0x11) and
MIPI_DCS_SET_DISPLAY_ON (0x29) as zero-parameter commands. By appending a
0x00 argument here, the DSI core will send a DCS Short Write with 1
parameter (Data Type 0x15) instead of a DCS Short Write with 0 parameters
(Data Type 0x05).

Could this protocol violation cause the panel or DSI host to reject the
command?

> +static int nt37705_off(struct nt37705_panel *ctx)
> +{
> +	struct mipi_dsi_multi_context dsi_ctx = { .dsi = ctx->dsi };
> +
> +	ctx->dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;
> +
> +	mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0x28, 0x00);
> +	mipi_dsi_msleep(&dsi_ctx, 20);
> +	mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0x10, 0x00);
> +	mipi_dsi_msleep(&dsi_ctx, 120);

[Severity: Medium]
Similarly, MIPI_DCS_SET_DISPLAY_OFF (0x28) and MIPI_DCS_ENTER_SLEEP_MODE
(0x10) are also zero-parameter commands.

Should the trailing 0x00 parameter byte be removed to prevent DCS protocol
violations here as well?

[ ... ]
> +	ret = mipi_dsi_picture_parameter_set(ctx->dsi, &pps);
> +	if (ret < 0) {
> +		dev_err(panel->dev, "failed to transmit PPS: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = mipi_dsi_compression_mode(ctx->dsi, true);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to enable compression mode: %d\n", ret);
> +		return ret;
> +	}
> +
> +	msleep(28); /* TODO: Is this panel-dependent? */
> +
> +	return 0;
> +}

[Severity: High]
Does this code leak the enabled regulators and leave the reset GPIO asserted
on these error paths?

If either mipi_dsi_picture_parameter_set() or mipi_dsi_compression_mode()
fails, the function returns immediately. Since the DRM framework does not
invoke unprepare() when prepare() returns an error, this skips the
regulator_bulk_disable() cleanup.

This could cause a permanent resource leak and leave the hardware powered on
in an inconsistent state.

[ ... ]
> +static int nt37705_bl_update_status(struct backlight_device *bl)
> +{
> +	struct mipi_dsi_device *dsi = bl_get_data(bl);
> +	u16 brightness = backlight_get_brightness(bl);
> +	int ret;
> +
> +	dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;
> +
> +	ret = mipi_dsi_dcs_set_display_brightness_large(dsi, brightness);
> +	if (ret < 0)
> +		return ret;
> +
> +	dsi->mode_flags |= MIPI_DSI_MODE_LPM;
> +
> +	return 0;
> +}

[Severity: Medium]
Are these non-atomic bitwise operations on dsi->mode_flags safe from race
conditions?

This sysfs handler can execute concurrently with DRM modeset operations
(such as nt37705_on()) which also modify dsi->mode_flags, potentially
causing a data race and state corruption.

Additionally, if mipi_dsi_dcs_set_display_brightness_large() fails, the
function returns early without restoring MIPI_DSI_MODE_LPM.

Could this permanently corrupt the DSI mode flags for all subsequent
sequences?

[Severity: High]
Is it safe to issue DCS writes here without verifying if the panel is
currently powered on?

The backlight sysfs node is registered in nt37705_probe() before the DSI
device is attached:

    ctx->panel.backlight = nt37705_create_backlight(dsi);
    ...
    return devm_mipi_dsi_attach(dev, dsi);

If userspace writes to the brightness node while the screen is off
(regulators disabled, reset asserted) or before the driver finishes
probing, it forces a DCS write to an unpowered or unattached DSI bus.

Could this cause DSI transmission timeouts or wedge the host controller?

[ ... ]
> +	ctx->panel.backlight = nt37705_create_backlight(dsi);
> +	if (IS_ERR(ctx->panel.backlight))
> +		return dev_err_probe(dev, PTR_ERR(ctx->panel.backlight),
> +				     "Failed to create backlight\n");
> +
> +	ret = devm_drm_panel_add(dev, &ctx->panel);
> +	if (ret)
> +		return ret;
> +
> +	/* This panel only supports DSC; unconditionally enable it */
> +	dsi->dsc = &ctx->dsc;
> +
> +	ctx->dsc.dsc_version_major = 1;
> +	ctx->dsc.dsc_version_minor = 1;
> +
> +	/* TODO: Pass slice_per_pkt = 2 */
> +	ctx->dsc.slice_height = 12;
> +	ctx->dsc.slice_width = 558;

[Severity: Medium]
Is it safe to publish the panel to the system before its structural setup is
complete?

devm_drm_panel_add() is called here, immediately exposing the panel to DRM
bridge consumers. However, critical DSC configuration fields (like dsi->dsc,
slice_height, and slice_width) are initialized after this call.

If a consumer binds concurrently, could it acquire the panel and invoke
drm_panel_prepare(), triggering drm_dsc_pps_payload_pack() on uninitialized
or zeroed DSC parameters, leading to malformed payload generation or division
by zero panics?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261002-fp6-panel-v3-0-4b80f83bc936@fairphone.com?part=2