Re: [PATCH v3 6/9] drm/panel: novatek-nt36672a: Add 3 panels used in Xiaomi phones

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] drm: panel: missing accum_err reset skips panel sleep on deinit error
- [Medium] drm/panel: nt36672a: regulator leak in prepare() error path
--

commit 219f9f92346f508583b813b9a7fbcd8b92f75a49
Author: Alexey Minnekhanov <alexeymin@minlexx.ru>

drm/panel: novatek-nt36672a: Add 3 panels used in Xiaomi phones

Add support for 3 panels used in 2 Xiaomi smartphones, all based on
Qualcomm SDM636/660 SoC:

* Shenchao panel used in Xiaomi Redmi Note 7 (xiaomi-lavender), the
  exact model string is unknown
* Tianma TL063FVMC43-02  for Xiaomi Redmi Note 6 Pro (xiaomi-tulip)
* Tianma TL063FVMCA01-00 for Xiaomi Redmi Note 7 (xiaomi-lavender)

> diff --git a/drivers/gpu/drm/panel/panel-novatek-nt36672a.c b/drivers/gpu/drm/panel/panel-novatek-nt36672a.c
> index a445c8067aa5f..d356348d39608 100644
> --- a/drivers/gpu/drm/panel/panel-novatek-nt36672a.c
> +++ b/drivers/gpu/drm/panel/panel-novatek-nt36672a.c

[ ... ]

> +static void lavender_tulip_deinit_cmds(struct mipi_dsi_multi_context *dsi_ctx)
> +{
> +	dsi_ctx->dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;
> +
> +	mipi_dsi_dcs_set_display_off_multi(dsi_ctx);
> +	mipi_dsi_msleep(dsi_ctx, 20);
> +	mipi_dsi_dcs_enter_sleep_mode_multi(dsi_ctx);
> +	mipi_dsi_msleep(dsi_ctx, 120);
> +}

[Severity: Medium]
If mipi_dsi_dcs_set_display_off_multi() fails, it sets dsi_ctx->accum_err to a
negative error code. Since there is no dsi_ctx->accum_err = 0; after it, will
the subsequent mipi_dsi_msleep() and mipi_dsi_dcs_enter_sleep_mode_multi()
calls immediately exit as no-ops?

This seems like it would abort the deinitialization sequence prematurely,
skipping the panel sleep command and required delays before system regulators
disable power.

For comparison, tianma_fhd_video_send_deinit_cmds() explicitly resets accum_err
to continue with the power-down sequence even if prior commands fail.

While looking at how the new send_init_cmds() function interacts with the
preparation path:

drivers/gpu/drm/panel/panel-novatek-nt36672a.c:nt36672a_panel_prepare() {
	...
	dsi_ctx.accum_err = nt36672a_panel_power_on(pinfo);

	/* send init cmds */
	if (pinfo->desc->send_init_cmds)
		pinfo->desc->send_init_cmds(&dsi_ctx);

	if (dsi_ctx.accum_err < 0)
		gpiod_set_value(pinfo->reset_gpio, 0);

	return dsi_ctx.accum_err;
}

[Severity: Medium]
If send_init_cmds() fails (for example due to a MIPI DSI write timeout) and
returns a negative dsi_ctx.accum_err, does this leak the regulator reference
count?

It looks like the error path pulls the reset GPIO low and returns the error
code, but fails to call nt36672a_panel_power_off() or regulator_bulk_disable()
to clean up the regulators enabled earlier by nt36672a_panel_power_on().

Since the DRM core's drm_panel_prepare() does not set the panel's prepared
flag to true on error, drm_panel_unprepare() will skip the unprepared panel,
leaving the regulators permanently enabled.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261011-nt36672a-panels-v3-0-9ea238627f8a@minlexx.ru?part=6