Re: [PATCH 2/2] drm/panel: Add Novatek NT36536 panel driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] Missing drm_panel_remove() in the probe error paths for backlight initialization leads to a use-after-free on the global panel list.
- [High] The panel is registered and exposed to the system via `drm_panel_add()` before its initialization is complete, causing a race condition.
- [High] Incomplete `drm_dsc_config` static definition leaves critical parameters like `pic_width`, `pic_height`, and rate-control values as 0, breaking DSC.
- [Medium] Unchecked DSI multi-context accumulated error in `novatek_prepare()` during DSC configuration masks transmission failures.
- [Medium] Missing rollback of hardware state and regulators when `backlight_enable()` fails in `novatek_prepare()`.
- [Medium] DCS backlight commands issued in `novatek_bl_update_status()` without verifying the panel power state.
- [Low] Missing Kconfig dependency on `DRM_DISPLAY_DSC_HELPER`.
--

commit 86b2b70476faa9f551d7227512ce180811421366
Author: Pengyu Luo <mitltlatltl@gmail.com>

drm/panel: Add Novatek NT36536 panel driver

Add a driver for panels using the Novatek NT36536 Display Driver IC,
including support for the CSOT PP8807HB1-1, a 10-bit panel found in
LENOVO Legion Y700 Gen4.

> diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
> index 7450b27622a23..3912b79a2ebb2 100644
> --- a/drivers/gpu/drm/panel/Kconfig
> +++ b/drivers/gpu/drm/panel/Kconfig
> @@ -620,6 +620,17 @@ config DRM_PANEL_NOVATEK_NT36523
>  	  around the Novatek NT36523 display controller, such as some
>  	  Boe panels used in Xiaomi Mi Pad 5 and 5 Pro tablets.
>  
> +config DRM_PANEL_NOVATEK_NT36536
> +	tristate "Novatek NT36536 panel driver"
> +	depends on OF
> +	depends on DRM_MIPI_DSI
> +	depends on BACKLIGHT_CLASS_DEVICE
> +	select DRM_KMS_HELPER

[Severity: Low]
Does this config also need to select DRM_DISPLAY_DSC_HELPER? The driver
utilizes DSC helper functions like drm_dsc_pps_payload_pack(), which could
cause a linker error if DRM_DISPLAY_DSC_HELPER is not selected by another
module.

> +	help
> +	  Say Y here if you want to enable support for Novatek NT36536-based
> +	  display panels, such as the one found in the LENOVO Legion Y700
> +	  Gen4.
> +
>  config DRM_PANEL_NOVATEK_NT36672A
[ ... ]
> diff --git a/drivers/gpu/drm/panel/panel-novatek-nt36536.c b/drivers/gpu/drm/panel/panel-novatek-nt36536.c
> new file mode 100644
> index 0000000000000..3ad21fcdf9f02
> --- /dev/null
> +++ b/drivers/gpu/drm/panel/panel-novatek-nt36536.c
[ ... ]
> +static int novatek_prepare(struct drm_panel *panel)
> +{
> +	struct novatek *ctx = to_novatek(panel);
> +	struct mipi_dsi_device *dsi = to_primary_dsi(ctx);
> +	struct mipi_dsi_multi_context dsi_ctx = { .dsi = dsi };
> +	struct drm_dsc_picture_parameter_set pps;
> +	struct device *dev = &dsi->dev;
> +	int ret;
> +
> +	ret = regulator_bulk_enable(ARRAY_SIZE(novatek_supplies),
> +				    ctx->supplies);
> +	if (ret < 0)
> +		return ret;
> +
> +	novatek_reset(ctx);
> +
> +	ret = ctx->desc->init_sequence(&dsi_ctx);
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to initialize panel: %d\n", ret);
> +		gpiod_set_value_cansleep(ctx->reset_gpio, 1);
> +		regulator_bulk_disable(ARRAY_SIZE(novatek_supplies),
> +				       ctx->supplies);
> +		return ret;
> +	}
> +
> +	drm_dsc_pps_payload_pack(&pps, &ctx->dsc);
> +	mipi_dsi_picture_parameter_set_multi(&dsi_ctx, &pps);
> +	mipi_dsi_compression_mode_multi(&dsi_ctx, true);
> +	mipi_dsi_msleep(&dsi_ctx, 28);
> +
> +	return backlight_enable(ctx->backlight);

[Severity: Medium]
Will the DSI command failures accumulated in dsi_ctx.accum_err be silently
ignored here? The mipi_dsi_*_multi() macros capture errors in the context,
but the function returns the result of backlight_enable() without checking
the accumulated error state.

[Severity: Medium]
If backlight_enable() returns an error, will the hardware state leak? The
function exits without rolling back the regulators or the reset GPIO state
that were enabled earlier in the function, and the DRM core will not call
novatek_unprepare() on a prepare failure.

> +}
[ ... ]
> +static int novatek_bl_update_status(struct backlight_device *bl)
> +{
> +	struct novatek *ctx = bl_get_data(bl);
> +	u16 brightness = backlight_get_brightness(bl);
> +
> +	return mipi_dsi_dcs_set_display_brightness_large(to_primary_dsi(ctx),
> +							 brightness);

[Severity: Medium]
Could this send DCS commands over a disabled DSI bus? If userspace modifies
the backlight brightness via sysfs while the panel is off (such as after
novatek_unprepare() has run), it looks like the commands are sent without
verifying if the panel is currently prepared and powered.

> +}
[ ... ]
> +static struct drm_dsc_config csot_pp8807hb1_1_dsc_cfg = {
> +	.dsc_version_major = 1,
> +	.dsc_version_minor = 2,
> +	.slice_height = 20,
> +	.slice_width = 476,
> +	.slice_count = 2,
> +	.bits_per_component = 10,
> +	.bits_per_pixel = 8 << 4,
> +	.block_pred_enable = true,
> +};

[Severity: High]
Is this DSC configuration missing dynamic parameters? Fields like pic_width,
pic_height, and rate-control parameters default to 0 and don't appear to be
dynamically populated by the driver via drm_dsc_compute_rc_parameters()
before being packed in novatek_prepare().

[ ... ]
> +static int novatek_probe(struct mipi_dsi_device *dsi)
> +{
[ ... ]
> +	ctx->panel.prepare_prev_first = true;
> +
> +	drm_panel_add(&ctx->panel);
> +
> +	for (i = 0; i < num_dsi; i++) {
> +		ctx->dsi[i]->lanes = desc->lanes;

[Severity: High]
Is it safe to expose the panel via drm_panel_add() before its initialization
is complete? Calling this early allows consumers like DRM bridges to access
the panel before ctx->dsi[i]->lanes is configured, devm_mipi_dsi_attach() is
executed, or the backlight is set up.

> +		ctx->dsi[i]->format = desc->format;
> +		ctx->dsi[i]->mode_flags = desc->mode_flags;
> +		ctx->dsi[i]->dsc = &ctx->dsc;
> +		ret = devm_mipi_dsi_attach(dev, ctx->dsi[i]);
> +		if (ret < 0) {
> +			drm_panel_remove(&ctx->panel);
> +			return dev_err_probe(dev, ret,
> +					     "Failed to attach to DSI host\n");
> +		}
> +	}
> +
> +	if (desc->has_dcs_backlight) {
> +		ctx->backlight = novatek_create_backlight(ctx);
> +		if (IS_ERR(ctx->backlight))
> +			return dev_err_probe(dev, PTR_ERR(ctx->backlight),
> +					     "Failed to create backlight\n");

[Severity: High]
Does this leak the panel registration on error? The function returns
directly without calling drm_panel_remove(&ctx->panel), leaving a dangling
pointer in the global panel_list.

> +	} else {
> +		ret = drm_panel_of_backlight(&ctx->panel);
> +		if (ret)
> +			return dev_err_probe(dev, ret, "Failed to get backlight\n");

[Severity: High]
Similarly, does this alternative error path also need to remove the panel
from the global list to prevent a use-after-free?

> +	}
> +
> +	return 0;
> +}
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260630023439.248861-1-mitltlatltl@gmail.com?part=2