Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider: - [High] drm/panel: resource leak in nt37705_prepare() error path - [High] drm/panel: unsafe DSI access from backlight sysfs when panel is unpowered - [Medium] drm/panel: race condition and state corruption in backlight sysfs handler - [Medium] drm/panel: DCS protocol violation sending extra parameters - [Medium] drm/panel: premature exposure via devm_drm_panel_add() before DSC init --
commit 512ec900afa4de1ceeb7c942bc90e389f2a7b45d Author: Luca Weiss <[email protected]> drm/panel: Add driver for Novatek NT37705 panel Add support for the 2484x1116 AMOLED panel from BOE (BJ631JHM-T71-D900) bundled with a NT37705 driver IC, as found on the Fairphone (Gen. 6) smartphone. The panel can also be configured in 10-bit (RGB101010) mode, however currently it's configured in 8-bit (RGB888) since there's some issues in the Qualcomm DPU driver when driving this panel in 10-bit. > diff --git a/drivers/gpu/drm/panel/panel-novatek-nt37705.c > b/drivers/gpu/drm/panel/panel-novatek-nt37705.c > new file mode 100644 > index 0000000000000..3cf092a1f360d > --- /dev/null > +++ b/drivers/gpu/drm/panel/panel-novatek-nt37705.c [ ... ] > + mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0x11, 0x00); > + mipi_dsi_msleep(&dsi_ctx, 120); > + mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0x29, 0x00); > + mipi_dsi_msleep(&dsi_ctx, 22); > + > + return dsi_ctx.accum_err; > +} [Severity: Medium] Does this code send extra parameters for zero-parameter DCS commands? The DCS specification defines MIPI_DCS_EXIT_SLEEP_MODE (0x11) and MIPI_DCS_SET_DISPLAY_ON (0x29) as zero-parameter commands. By appending a 0x00 argument here, the DSI core will send a DCS Short Write with 1 parameter (Data Type 0x15) instead of a DCS Short Write with 0 parameters (Data Type 0x05). Could this protocol violation cause the panel or DSI host to reject the command? > +static int nt37705_off(struct nt37705_panel *ctx) > +{ > + struct mipi_dsi_multi_context dsi_ctx = { .dsi = ctx->dsi }; > + > + ctx->dsi->mode_flags &= ~MIPI_DSI_MODE_LPM; > + > + mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0x28, 0x00); > + mipi_dsi_msleep(&dsi_ctx, 20); > + mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0x10, 0x00); > + mipi_dsi_msleep(&dsi_ctx, 120); [Severity: Medium] Similarly, MIPI_DCS_SET_DISPLAY_OFF (0x28) and MIPI_DCS_ENTER_SLEEP_MODE (0x10) are also zero-parameter commands. Should the trailing 0x00 parameter byte be removed to prevent DCS protocol violations here as well? [ ... ] > + ret = mipi_dsi_picture_parameter_set(ctx->dsi, &pps); > + if (ret < 0) { > + dev_err(panel->dev, "failed to transmit PPS: %d\n", ret); > + return ret; > + } > + > + ret = mipi_dsi_compression_mode(ctx->dsi, true); > + if (ret < 0) { > + dev_err(dev, "failed to enable compression mode: %d\n", ret); > + return ret; > + } > + > + msleep(28); /* TODO: Is this panel-dependent? */ > + > + return 0; > +} [Severity: High] Does this code leak the enabled regulators and leave the reset GPIO asserted on these error paths? If either mipi_dsi_picture_parameter_set() or mipi_dsi_compression_mode() fails, the function returns immediately. Since the DRM framework does not invoke unprepare() when prepare() returns an error, this skips the regulator_bulk_disable() cleanup. This could cause a permanent resource leak and leave the hardware powered on in an inconsistent state. [ ... ] > +static int nt37705_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; > + > + dsi->mode_flags |= MIPI_DSI_MODE_LPM; > + > + return 0; > +} [Severity: Medium] Are these non-atomic bitwise operations on dsi->mode_flags safe from race conditions? This sysfs handler can execute concurrently with DRM modeset operations (such as nt37705_on()) which also modify dsi->mode_flags, potentially causing a data race and state corruption. Additionally, if mipi_dsi_dcs_set_display_brightness_large() fails, the function returns early without restoring MIPI_DSI_MODE_LPM. Could this permanently corrupt the DSI mode flags for all subsequent sequences? [Severity: High] Is it safe to issue DCS writes here without verifying if the panel is currently powered on? The backlight sysfs node is registered in nt37705_probe() before the DSI device is attached: ctx->panel.backlight = nt37705_create_backlight(dsi); ... return devm_mipi_dsi_attach(dev, dsi); If userspace writes to the brightness node while the screen is off (regulators disabled, reset asserted) or before the driver finishes probing, it forces a DCS write to an unpowered or unattached DSI bus. Could this cause DSI transmission timeouts or wedge the host controller? [ ... ] > + ctx->panel.backlight = nt37705_create_backlight(dsi); > + if (IS_ERR(ctx->panel.backlight)) > + return dev_err_probe(dev, PTR_ERR(ctx->panel.backlight), > + "Failed to create backlight\n"); > + > + ret = devm_drm_panel_add(dev, &ctx->panel); > + if (ret) > + return ret; > + > + /* This panel only supports DSC; unconditionally enable it */ > + dsi->dsc = &ctx->dsc; > + > + ctx->dsc.dsc_version_major = 1; > + ctx->dsc.dsc_version_minor = 1; > + > + /* TODO: Pass slice_per_pkt = 2 */ > + ctx->dsc.slice_height = 12; > + ctx->dsc.slice_width = 558; [Severity: Medium] Is it safe to publish the panel to the system before its structural setup is complete? devm_drm_panel_add() is called here, immediately exposing the panel to DRM bridge consumers. However, critical DSC configuration fields (like dsi->dsc, slice_height, and slice_width) are initialized after this call. If a consumer binds concurrently, could it acquire the panel and invoke drm_panel_prepare(), triggering drm_dsc_pps_payload_pack() on uninitialized or zeroed DSC parameters, leading to malformed payload generation or division by zero panics? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
