Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [Critical] drm/panel: panel_prepare aborts prematurely due to positive return check - [High] drm/panel: IOVCC regulator leaked in panel_prepare error path - [Medium] drm/panel: commit message and implementation mismatch - [Medium] drm/panel: DCS write errors silently ignored by PAGE/CMD macros --
commit 2f8c8459c9b015745342c7329f8ef342082a4f1a Author: Maxime Ripard <[email protected]> drm/panel: dsi-bpf: Add Raspberry Pi 7-inch panel BPF program This commit introduces a BPF program for initializing a Raspberry Pi MIPI-DSI panel. The patch translates a C-based DCS sequence into BPF macros. > drm/panel: dsi-bpf: Add Raspberry Pi 7-inch panel BPF program > > Translate the raspberrypi,dsi-7inch initialization sequence from > drivers/gpu/drm/panel/panel-ilitek-ili9881c.c into a BPF program. [Severity: Medium] The commit message explicitly claims to add the "Raspberry Pi 7-inch panel BPF program" and translates the 7-inch sequence. However, as seen in the code below, it implements the 5-inch variant. Should the commit message be updated to match the implemented code? > diff --git a/drivers/gpu/drm/panel/bpf/progs/Raspberrypi__dsi-5inch.bpf.c > b/drivers/gpu/drm/panel/bpf/progs/Raspberrypi__dsi-5inch.bpf.c > new file mode 100644 > index 0000000000000..1f9ff7f2fd3ce > --- /dev/null > +++ b/drivers/gpu/drm/panel/bpf/progs/Raspberrypi__dsi-5inch.bpf.c > @@ -0,0 +1,266 @@ [ ... ] > +#define PAGE(p) do { \ > + const __u8 _d[] = { 0x98, 0x81, (p) }; \ > + panel_bpf_mipi_dsi_dcs_write(pctx, 0xff, _d, sizeof(_d));\ > +} while (0) > + > +#define CMD(c, d) panel_bpf_mipi_dsi_dcs_write_byte(pctx, (c), (d)) [Severity: Medium] Do these macros need to check the return value of panel_bpf_mipi_dsi_dcs_write() and panel_bpf_mipi_dsi_dcs_write_byte()? By dropping the error codes, if a DSI transmission fails during the command sequence, it seems the BPF program will continue blindly instead of aborting the initialization. [ ... ] > +SEC(PANEL_BPF_MIPI_DSI_PREPARE) > +int BPF_PROG(panel_prepare, struct panel_bpf_mipi_dsi_ctx *pctx) > +{ > + int ret; > + > + ret = panel_bpf_mipi_dsi_regulator_enable_and_wait(pctx, > PANEL_BPF_MIPI_DSI_SUPPLY_IOVCC, 5); > + if (ret) > + return ret; > + > + ret = panel_bpf_mipi_dsi_regulator_enable_and_wait(pctx, > PANEL_BPF_MIPI_DSI_SUPPLY_VCC, 5); > + if (ret) > + return ret; [Severity: High] If enabling the VCC regulator fails here, does this code leak the previously enabled IOVCC regulator's enable count? It looks like we return immediately without calling panel_bpf_mipi_dsi_regulator_disable() to disable IOVCC. [ ... ] > + /* set_tear_on with VBLANK mode (0x00) */ > + CMD(0x35, 0x00); > + > + ret = panel_bpf_mipi_dsi_exit_sleep_mode(pctx); > + if (ret) > + return ret; [Severity: Critical] Does panel_bpf_mipi_dsi_exit_sleep_mode() return a positive number of bytes on success? Since it calls panel_bpf_mipi_dsi_dcs_write_and_wait(), which forwards the underlying mipi_dsi_dcs_write() result, a successful write would make ret > 0. This check would then evaluate to true on a successful transmission, causing panel_prepare() to abort prematurely before enabling the display. [ ... ] > +PANEL_BPF_MIPI_DSI_OPS(raspberrypi_dsi_5inch) = { > + .panel_id = "/soc/dsi@7e700000/panel@0", > + .compatible = "raspberrypi,dsi-5inch", [Severity: Medium] This isn't a code regression, but this compatible string and struct name bind the program strictly to the 5-inch panel, which contradicts the stated purpose in the commit message. > + .format = MIPI_DSI_FMT_RGB888, > + .lanes = 2, -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=4
