Re: [PATCH 2/2] drm/panel: Add panel driver for Chipone ICNA35XX based panels

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] NULL pointer dereference of `connector` in `icna35xx_get_current_mode()`
- [High] Mutation of shared global static data (`dsi->dsc`)
- [High] Data races, state leaks, and unprotected hardware access in backlight operations
- [Medium] Reset GPIO left deasserted on `icna35xx_prepare` error path
- [Low] Missing Kconfig dependency for DRM_DISPLAY_DSC_HELPER
--

commit c36140d9bdc899b722496ec403d1552b4c5aaf5a
Author: Teguh Sobirin <teguh@sobir.in>

drm/panel: Add panel driver for Chipone ICNA35XX based panels

This commit adds support for the ICNA3512 and ICNA3520 DDICs used in the AYN
Odin 2 Portal, Odin 3, and Thor top panels.

> diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
> index 7450b27622a23..1368b5a0b6c91 100644
> --- a/drivers/gpu/drm/panel/Kconfig
> +++ b/drivers/gpu/drm/panel/Kconfig
> @@ -105,6 +105,17 @@ config DRM_PANEL_BOE_TV101WUM_LL2
>  	  Say Y here if you want to support for BOE TV101WUM-LL2
>  	  WUXGA PANEL DSI Video Mode panel
>  
> +config DRM_PANEL_CHIPONE_ICNA35XX
> +	tristate "Chipone ICNA35XX panel driver"
> +	depends on OF
> +	depends on DRM_MIPI_DSI
> +	depends on BACKLIGHT_CLASS_DEVICE
> +	select DRM_DISPLAY_HELPER

Since the driver calls drm_dsc_pps_payload_pack(), should this also select
DRM_DISPLAY_DSC_HELPER? Otherwise, it might cause unresolved symbol linker
errors if the DSC helper isn't pulled in by another driver.

> +	help
> +	  Say Y here if you want to enable support for the panels built
> +	  around the Chipone ICNA3512 and ICNA3520 display controllers,
> +	  such as some Tianma panels used in AYN Odin2 Portal and Thor.

[ ... ]
> diff --git a/drivers/gpu/drm/panel/panel-chipone-icna35xx.c b/drivers/gpu/drm/panel/panel-chipone-icna35xx.c
> new file mode 100644
> index 0000000000000..1be903270db8e
> --- /dev/null
> +++ b/drivers/gpu/drm/panel/panel-chipone-icna35xx.c
[ ... ]
> +static int icna35xx_get_current_mode(struct panel_info *pinfo)
> +{
> +	struct drm_connector *connector = pinfo->connector;
> +	struct drm_crtc_state *crtc_state;
> +	int i;
> +
> +	/* Return the default (first) mode if no info available yet */
> +	if (!connector->state || !connector->state->crtc)
> +		return 0;

Can connector be NULL here? The pinfo->connector field is only populated
when icna35xx_get_modes() is invoked. If drm_panel_prepare() is called
before modes are probed, connector will be NULL and this will cause a NULL
pointer dereference.

[ ... ]
> +static int icna35xx_prepare(struct drm_panel *panel)
> +{
> +	struct panel_info *pinfo = to_panel_info(panel);
> +	int ret;
> +
> +	ret = regulator_bulk_enable(ARRAY_SIZE(panel_supplies), pinfo->supplies);
> +	if (ret < 0) {
> +		dev_err(panel->dev, "failed to enable regulators: %d\n", ret);
> +		return ret;
> +	}
> +
> +	icna35xx_reset(pinfo);
> +
> +	ret = pinfo->desc->init_sequence(pinfo);
> +	if (ret < 0) {
> +		regulator_bulk_disable(ARRAY_SIZE(panel_supplies), pinfo->supplies);

Does this error path leave the reset GPIO deasserted? When initialization
fails, it might be necessary to assert the reset GPIO
(gpiod_set_value_cansleep(pinfo->reset_gpio, 1)) before disabling the
regulators, similar to what icna35xx_unprepare() does.

> +		dev_err(panel->dev, "failed to initialize panel: %d\n", ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
[ ... ]
> +static int icna35xx_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;

Are there potential data races or state leaks here? The mode_flags are modified
using non-atomic bitwise operations without any locking, which could race with
icna35xx_disable() and icna35xx_prepare().

Also, if mipi_dsi_dcs_set_display_brightness_large() fails, the function
returns early without restoring MIPI_DSI_MODE_LPM, which permanently alters the
panel's mode state for subsequent operations.

Finally, could this send DCS commands to a suspended panel? If userspace
alters brightness while the panel is off, there is no check ensuring the
hardware is powered on, which might trigger DSI bus timeouts.

> +
> +	dsi->mode_flags |= MIPI_DSI_MODE_LPM;
> +
> +	return 0;
> +}
> +
> +static int icna35xx_bl_get_brightness(struct backlight_device *bl)
> +{
> +	struct mipi_dsi_device *dsi = bl_get_data(bl);
> +	u16 brightness;
> +	int ret;
> +
> +	dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;
> +
> +	ret = mipi_dsi_dcs_get_display_brightness_large(dsi, &brightness);
> +	if (ret < 0)
> +		return ret;

Similar to icna35xx_bl_update_status(), does this early return leak the
MIPI_DSI_MODE_LPM state? And is it safe to modify mode_flags here without
locking or checking if the panel is powered on?

> +
> +	dsi->mode_flags |= MIPI_DSI_MODE_LPM;
> +
> +	return brightness;
> +}
[ ... ]
> +static int icna35xx_probe(struct mipi_dsi_device *dsi)
> +{
[ ... ]
> +	pinfo->dsi->lanes = pinfo->desc->lanes;
> +	pinfo->dsi->format = pinfo->desc->format;
> +	pinfo->dsi->mode_flags = pinfo->desc->mode_flags;
> +	pinfo->dsi->dsc = &pinfo->desc->dsc;

Is it safe to point dsi->dsc directly to the static global pinfo->desc->dsc?
DSI host drivers like MSM call drm_dsc_compute_rc_parameters(dsi->dsc), which
computes and writes Rate Control parameters directly into the struct. This
modifies the globally shared descriptor in the .data section, potentially
corrupting state if multiple panels exist or if the driver is unbound and
rebound. Consider copying the DSC configuration to a dynamically allocated
structure.

> +
> +	ret = mipi_dsi_attach(pinfo->dsi);
> +	if (ret < 0) {
> +		dev_err_probe(dev, ret, "Failed to attach to DSI host\n");
> +		drm_panel_remove(&pinfo->panel);
> +		return ret;
> +	}
> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260509-icna35xx-v1-0-688d3d4e10f9@gmail.com?part=2