Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [High] Missing `drm_panel_remove()` cleanup on the error path of `mipi_dsi_attach()` in probe leads to Use-After-Free. - [High] The reset GPIO is asserted after regulators are disabled in the unprepare sequence, risking back-powering the display controller. -- --- Patch [1]: [PATCH] drm/panel: ilitek-ili9881c: do not fail probe if iovcc is absent --- commit 20684573ac473e40824b8b3fb7de5a67cbfc4efa Author: David Oberhollenzer <[email protected]> drm/panel: ilitek-ili9881c: do not fail probe if iovcc is absent Commit 4c95b2b7d49e ("drm/panel: ilitek-ili9881c: support Waveshare 7.0" DSI panel") adds an additional iovcc regulator that other Ilitek ili9881c based panels apparently do not have or need. The commit goes out of its way to make usage of this new regulator optional, dutifully testing if the field in struct ili9881c is NULL before touching the new regulator. However, in the probe function, it unconditionally fails if devm_regulator_get_optional returns an error. > diff --git a/drivers/gpu/drm/panel/panel-ilitek-ili9881c.c > b/drivers/gpu/drm/panel/panel-ilitek-ili9881c.c > index 3abdd0870e831..c3d5ab67121d9 100644 > --- a/drivers/gpu/drm/panel/panel-ilitek-ili9881c.c > +++ b/drivers/gpu/drm/panel/panel-ilitek-ili9881c.c > @@ -2563,9 +2563,12 @@ static int ili9881c_dsi_probe(struct mipi_dsi_device > *dsi) > "Couldn't get our power regulator\n"); > > ctx->iovcc = devm_regulator_get_optional(&dsi->dev, "iovcc"); > - if (IS_ERR(ctx->iovcc)) > - return dev_err_probe(&dsi->dev, PTR_ERR(ctx->iovcc), > + if (IS_ERR(ctx->iovcc)) { > + if (PTR_ERR(ctx->iovcc) != -ENODEV) > + return dev_err_probe(&dsi->dev, PTR_ERR(ctx->iovcc), > "Couldn't get our iovcc regulator\n"); > + ctx->iovcc = NULL; > + } > > ctx->reset = devm_gpiod_get_optional(&dsi->dev, "reset", GPIOD_OUT_LOW); > if (IS_ERR(ctx->reset)) [Severity: High] This isn't a bug introduced by this patch, but while looking at ili9881c_dsi_probe(), it appears there is a missing cleanup if mipi_dsi_attach() fails later in the function: drm_panel_add(&ctx->panel); dsi->mode_flags = ctx->desc->mode_flags; dsi->format = MIPI_DSI_FMT_RGB888; dsi->lanes = ctx->desc->lanes; return mipi_dsi_attach(dsi); } If mipi_dsi_attach() returns an error, the ctx memory (allocated via devm_drm_panel_alloc()) is automatically freed upon probe failure. However, since drm_panel_remove() isn't called, does the freed memory remain linked in the global panel_list? Could this lead to a use-after-free crash if another driver traverses panel_list by calling of_drm_find_panel()? [Severity: High] This is another pre-existing issue, but I noticed a potential power sequencing problem in ili9881c_unprepare(): drivers/gpu/drm/panel/panel-ilitek-ili9881c.c:ili9881c_unprepare() { ... regulator_disable(ctx->power); if (ctx->iovcc) regulator_disable(ctx->iovcc); gpiod_set_value_cansleep(ctx->reset, 1); ... } Is it safe to disable the regulators before asserting the reset GPIO? If the regulators are disabled while the reset GPIO is still driving a high voltage, could the unpowered display panel be back-powered through the reset pin's ESD protection diodes? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
