Re: [PATCH v5 6/6] drm/panel: Add driver for Synaptics R63455 DSI panel

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
- [High] A copy-paste error in the BOE panel initialization sequence duplicates command bytes, corrupting the DSI payload.
- [High] Concurrent non-atomic read-modify-write operations on DSI `mode_flags` can corrupt the transmission mode state.
- [High] Missing IS_ERR() check on the return value of devm_drm_panel_alloc() leads to an immediate dereference of an error pointer.
- [High] The backlight sysfs interface is exposed to userspace before the DSI device is fully configured and attached.
- [Medium] The macros LE16_BYTE0/1 and BE16_BYTE0/1 use endianness conversion functions before bitwise masking, causing incorrect byte order on Big Endian architectures.
- [Medium] Returning PTR_ERR() on a valid pointer when the GPIO array size is incorrect leaks a kernel heap address and produces a positive error code.
- [Medium] The backlight update_status callback blindly sends DCS commands without checking the panel's power state.
- [Low] An early return in the panel disable path on DSI command failure skips disabling the backlight.
--

--- Patch [6]: [PATCH v5 6/6] drm/panel: Add driver for Synaptics R63455 DSI panel ---
commit 89f5e4df3241a9188e302ee5028a24a7b0bc9542
Author: Jun Nie <jun.nie@linaro.org>
Subject: drm/panel: Add driver for Synaptics R63455 DSI panel

This commit adds support for the Synaptics R63455 DSI panel used in virtual
reality devices. It exposes a dual-panel configuration as a single logical
panel to the DRM framework.

> --- a/drivers/gpu/drm/panel/panel-synaptics-r63455.c
> +++ b/drivers/gpu/drm/panel/panel-synaptics-r63455.c
[ ... ]
> +/*
> + * Extract the low byte and high byte of the 16bit number converted
> + * to the cpu endianness.
> + */
> +#define LE16_BYTE0(val) (le16_to_cpu(val) & 0xff)
> +#define LE16_BYTE1(val) ((le16_to_cpu(val) >> 8) & 0xff)
> +#define BE16_BYTE0(val) (be16_to_cpu(val) & 0xff)
> +#define BE16_BYTE1(val) ((be16_to_cpu(val) >> 8) & 0xff)

[Severity: Medium]
Will this generate the correct byte order on Big Endian architectures?

Since be16_to_cpu is a no-op on Big Endian systems, applying the bitwise
shift and mask afterward will extract the bytes in Little Endian order
rather than Big Endian. This will cause the panel initialization sequence to
receive payload bytes in reversed order.

Could we directly shift and mask without the endian conversion macros to
safely serialize CPU-native integers across all architectures?

[ ... ]
> +static int r63455_panel_on_boe_vs026c4m_n52_26000(struct drm_panel *panel)
> +{
[ ... ]
> +	r63455_dsi_write_seq(ctx, dsi_ctx, R63455_GEN_OUTPIN_SET,
> +			     0xf, 0xe4, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> +			     0xf, 0xb2,
> +			     0xf, 0xb2, 0x00, 0x64);

[Severity: High]
Does this sequence intentionally send the 0xf, 0xb2 bytes twice for the BOE
panel initialization?

This looks like it could be a copy-paste error that sends 14 bytes instead
of the expected 12 bytes. Could this transmit malformed configuration data
to the display controller and cause initialization failures?

[ ... ]
> +static int r63455_set_brightness(struct r63455_ctx *ctx, u16 brightness,
> +				  u16 pulse_offset_rows)
> +{
[ ... ]
> +	ctx->dsi[0]->mode_flags &= ~MIPI_DSI_MODE_LPM;
> +	ctx->dsi[1]->mode_flags &= ~MIPI_DSI_MODE_LPM;

[Severity: High]
Are these bitwise operations on mode_flags safe from race conditions?

Since r63455_set_brightness executes under the backlight sysfs lock, it can
run concurrently with DRM atomic commit handlers like r63455_enable or
r63455_disable which use independent locks.

Could a concurrent read-modify-write here corrupt the transmission mode state,
such as unintentionally dropping the MIPI_DSI_MODE_VIDEO flag?

[ ... ]
> +static int r63455_disable(struct drm_panel *panel)
> +{
[ ... ]
> +	r63455_dsi_write_seq(ctx, dsi_ctx, MIPI_DCS_ENTER_SLEEP_MODE);
> +
> +	mipi_dsi_msleep(&dsi_ctx, 200);
> +
> +	if (dsi_ctx.accum_err)
> +		return dsi_ctx.accum_err;
> +
> +	/* required by hardware to be after MIPI_DCS_ENTER_SLEEP_MODE */
> +	return backlight_disable(ctx->backlight);

[Severity: Low]
Does returning early here leave the backlight subsystem in an active state?

If the DSI ENTER_SLEEP_MODE sequence fails, the driver skips calling
backlight_disable. While the physical regulators are disabled later in
r63455_unprepare, could this leave the backlight framework out of sync?

[ ... ]
> +static int r63455_bl_update_status(struct backlight_device *bl)
> +{
> +	struct mipi_dsi_device *dsi = bl_get_data(bl);
> +	struct r63455_ctx *r63455_ctx = mipi_dsi_get_drvdata(dsi);
> +
> +	return r63455_set_brightness(r63455_ctx, bl->props.brightness,
> +				     bl->props.pulse_offset_rows);
> +}

[Severity: Medium]
Does this function need to verify the panel power state before sending DCS
commands?

Userspace can modify the brightness via sysfs while the DRM display is
disabled. If this occurs, it might trigger MIPI DSI transmissions to a
powered-off panel or unclocked DSI host.

Could this lead to hardware lockups or DSI timeouts?

[ ... ]
> +static int r63455_panel_add(struct r63455_ctx *ctx)
> +{
[ ... ]
> +	ctx->reset_gpios = devm_gpiod_get_array(dev, "reset", GPIOD_OUT_LOW);
> +	if (IS_ERR(ctx->reset_gpios) || ctx->reset_gpios->ndescs != 2)
> +		return PTR_ERR(ctx->reset_gpios);

[Severity: Medium]
If the GPIO array has a count other than 2, will this return a positive value
derived from a valid heap pointer?

When devm_gpiod_get_array succeeds but returns an unexpected number of
descriptors, ctx->reset_gpios is a valid pointer rather than an error pointer.

Could we return an explicit error code like -EINVAL here instead of leaking
the pointer address through PTR_ERR?

[ ... ]
> +static int r63455_probe(struct mipi_dsi_device *dsi)
> +{
[ ... ]
> +	ctx = devm_drm_panel_alloc(&dsi->dev, __typeof(*ctx), panel,
> +				   &r63455_drm_funcs, DRM_MODE_CONNECTOR_DSI);
> +
> +	ctx->dsi_init_seq = of_device_get_match_data(dev);

[Severity: High]
Should we check the return value of devm_drm_panel_alloc for an error pointer?

If memory allocation fails during driver probe, devm_drm_panel_alloc returns
ERR_PTR(-ENOMEM). Dereferencing ctx on the very next line to assign
dsi_init_seq will result in a kernel panic.

[ ... ]
> +	ret = r63455_panel_add(ctx);
> +	if (ret) {
> +		DRM_DEV_ERROR(dev, "failed to add panel %d\n", ret);
> +		return ret;
> +	}
> +
> +	for (i = 0; i < 2; i++) {
> +		dsi_dev = ctx->dsi[i];
> +		dsi_dev->lanes = 3;
> +		dsi_dev->format = MIPI_DSI_FMT_RGB888;
> +		dsi_dev->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_LPM |
> +			MIPI_DSI_CLOCK_NON_CONTINUOUS;
> +		ret = devm_mipi_dsi_attach(dev, dsi_dev);

[Severity: High]
Is it safe to register the backlight device before the DSI host is configured
and attached?

Calling r63455_panel_add exposes the backlight sysfs interface to userspace.
However, the lanes, format, mode_flags, and devm_mipi_dsi_attach are not
set up until the loop that follows.

If a udev rule writes to the brightness node during this window, could it
trigger DSI transmissions on an unattached device and lock up the host?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260727-sm8650-7-1-bonded-dsi-v5-0-c042266b9eeb@linaro.org?part=6