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

<[email protected]> 于2026年7月27日周一 16:20写道：
>
> 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 <[email protected]>
> 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?

From Neil:
No we extract the low byte and high byte of the 16bit number converted
to the cpu endianness,
so it's the right code here.

>
> [ ... ]
> > +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?

Yes, the sequence is send twice intentionally. As there are 2 panel controllers
as 1 logic panel in DRM high level system.
>
> [ ... ]
> > +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?

It's very improbable this happens, panel cmds and backlight are called
at very different times.
>
> [ ... ]
> > +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?

Technically yes, but practically it is an unrecoverable hardware state. So it is
a low risk issue and can be ignored.

>
> [ ... ]
> > +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?

Yes, I will add below logic in next version.

+++ b/drivers/gpu/drm/panel/panel-synaptics-r63455.c
@@ -456,6 +456,9 @@ 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);

+       if (!r63455_ctx->panel.enabled)
+               return 0;
+

>
> [ ... ]
> > +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?

Right, -EINVAL is proper value to return.
>
> [ ... ]
> > +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.
>
Right, a check shall be added in next version.

> [ ... ]
> > +     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?

It is OK per Neil's answer.
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6