Re: [PATCH v4 2/5] drm/panel: add Novatek NT36850 panel driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] drm/panel: nt36850: variable shadowing in nt36850_disable() masks DCS errors
- [Medium] drm/panel: nt36850: unconditional dereference of dsi1 in nt36850_disable() breaks single-DSI
--

commit 08efb97fa82649f7024ae6605a2b0d90177ccd63
Author: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>

drm/panel: add Novatek NT36850 panel driver
    
Add support for the Truly WQHD dual-DSI command mode panel built around
the Novatek NT36850, found on the display card for the Qualcomm SDM845
and SM8150 HDK.

> diff --git a/drivers/gpu/drm/panel/panel-novatek-nt36850.c b/drivers/gpu/drm/panel/panel-novatek-nt36850.c
> --- /dev/null
> +++ b/drivers/gpu/drm/panel/panel-novatek-nt36850.c
> @@ -0,0 +1,351 @@
> [ ... ]
> +static int nt36850_disable(struct drm_panel *panel)
> +{
> +	struct panel_info *pinfo = to_panel_info(panel);
> +	struct mipi_dsi_device *dsi0 = pinfo->dsi[0];
> +	struct mipi_dsi_device *dsi1 = pinfo->dsi[1];
> +	struct mipi_dsi_multi_context dsi_ctx = { .dsi = NULL };
> +
> +	dsi0->mode_flags &= ~MIPI_DSI_MODE_LPM;
> +	dsi1->mode_flags &= ~MIPI_DSI_MODE_LPM;

[Severity: Medium]
Does this code introduce a regression by unconditionally dereferencing
a NULL pointer if a single-DSI panel is supported in the future?

The dynamic nt36850_num_dsi_hosts check later in this function implies
it is intended to support single-DSI setups, but dsi1 is used directly
here without verification.

> +
> +	for (int i = 0; i < nt36850_num_dsi_hosts(pinfo); i++) {
> +		struct mipi_dsi_multi_context dsi_ctx = { .dsi = pinfo->dsi[i]};
> +
> +		mipi_dsi_dcs_set_display_off_multi(&dsi_ctx);
> +	}

[Severity: Medium]
Is it a regression that the inner dsi_ctx shadows the function-scoped
dsi_ctx?

By declaring a new dsi_ctx inside the loop, the error state from
mipi_dsi_dcs_set_display_off_multi gets instantly discarded when the
block ends, rather than accumulating.

> +	mipi_dsi_msleep(&dsi_ctx, 120);

[Severity: Medium]
Because the inner dsi_ctx shadowed the outer one, does this blindly
execute the delay using the unmodified outer context, even if the display
off sequence commands failed?

> +
> +	for (int i = 0; i < nt36850_num_dsi_hosts(pinfo); i++) {
> +		struct mipi_dsi_multi_context dsi_ctx = { .dsi = pinfo->dsi[i]};
> +
> +		mipi_dsi_dcs_enter_sleep_mode_multi(&dsi_ctx);
> +	}
> +	mipi_dsi_msleep(&dsi_ctx, 120);
> +
> +	dsi0->mode_flags |= MIPI_DSI_MODE_LPM;
> +	dsi1->mode_flags |= MIPI_DSI_MODE_LPM;

[Severity: Medium]
Could this result in another NULL pointer dereference regression for
single-DSI panels here when restoring the mode flags?

> +
> +	return dsi_ctx.accum_err;
> +}

[Severity: Medium]
Since the function-level dsi_ctx remains unmodified throughout, will
this always return 0 and mask any hardware failures that occurred during
the disable sequence?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015-sdm845-hdk-v4-0-b532b9f4029c@oss.qualcomm.com?part=2