Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] drm/panel: tx10d07vm0baa: DBI commands in disable() use freed tegra-dbi GPIOs - [High] drm/panel: unchecked priv->dbi in hitachi_tx10d07vm0baa_enable() - [Medium] drm/panel: hitachi-tx10d07vm0baa: swapped width_mm and height_mm in mode --
commit a653f9af8d9f186ab20cfe9e9245165b2be8f4e3 Author: Svyatoslav Ryhel <[email protected]> drm/panel: Add Hitachi TX10D07VM0BAA MIPI DBI Type B panel driver This commit adds a DRM panel driver for the Hitachi TX10D07VM0BAA 4" WVGA MIPI DBI Type B panel used in the LG Optimus 2X P990 smartphone. > diff --git a/drivers/gpu/drm/panel/panel-hitachi-tx10d07vm0baa.c > b/drivers/gpu/drm/panel/panel-hitachi-tx10d07vm0baa.c > new file mode 100644 > index 0000000000000..c3434cb96c86c > --- /dev/null > +++ b/drivers/gpu/drm/panel/panel-hitachi-tx10d07vm0baa.c > @@ -0,0 +1,297 @@ [ ... ] > +static int hitachi_tx10d07vm0baa_prepare(struct drm_panel *panel) > +{ > + struct hitachi_tx10d07vm0baa *priv = to_hitachi_tx10d07vm0baa(panel); > + struct device *dev = panel->dev; > + int ret; > + > + priv->dbi = dev_get_drvdata(dev->parent); > + if (!priv->dbi) > + return -ENODEV; > + > + gpiod_set_value_cansleep(priv->reset_gpio, 1); > + > + ret = regulator_bulk_enable(ARRAY_SIZE(hitachi_tx10d07vm0baa_supplies), > priv->supplies); > + if (ret) { > + dev_err(dev, "failed to enable power supplies: %d\n", ret); > + return ret; > + } > + > + usleep_range(1000, 2000); > + > + gpiod_set_value_cansleep(priv->reset_gpio, 0); > + > + usleep_range(10000, 11000); > + > + return 0; > +}; > + > +static int hitachi_tx10d07vm0baa_enable(struct drm_panel *panel) > +{ > + struct hitachi_tx10d07vm0baa *priv = to_hitachi_tx10d07vm0baa(panel); > + > + panel_dbi_command(priv, MIPI_DCS_SET_PARTIAL_ROWS, 0x00, 0x00, 0x03, > 0x1f); [Severity: High] Can priv->dbi be NULL when hitachi_tx10d07vm0baa_enable() (and later hitachi_tx10d07vm0baa_disable()) calls panel_dbi_command()? In hitachi_tx10d07vm0baa_probe(), priv->dbi is not initialized. Instead, hitachi_tx10d07vm0baa_prepare() looks up dev_get_drvdata(dev->parent) and returns -ENODEV if priv->dbi is NULL (for instance, if the panel device tree node is not a child of a bound DBI parent device, or if the parent has no drvdata set). During an atomic modeset commit, the prepare path runs: drm_atomic_helper_commit_modeset_enables() drm_atomic_bridge_chain_pre_enable() panel_bridge_atomic_pre_enable() drm_panel_prepare() hitachi_tx10d07vm0baa_prepare() -> returns -ENODEV When hitachi_tx10d07vm0baa_prepare() returns an error, drm_panel_prepare() logs the error and skips setting panel->prepared = true, but does not abort the atomic commit. Next in the same commit: drm_atomic_helper_commit_modeset_enables() drm_atomic_bridge_chain_enable() panel_bridge_atomic_enable() drm_panel_enable() hitachi_tx10d07vm0baa_enable() Because drm_panel_enable() only checks panel and panel->enabled without checking panel->prepared, it unconditionally calls hitachi_tx10d07vm0baa_enable(). That passes a NULL priv->dbi to panel_dbi_command() -> mipi_dbi_command_stackbuf() -> mipi_dbi_command_buf(), which dereferences dbi at mutex_lock(&dbi->cmdlock). > + panel_dbi_command(priv, MIPI_DCS_SET_SCROLL_AREA, 0x00, 0x00, 0x03, > 0x20, > + 0x00, 0x00); [ ... ] > +static int hitachi_tx10d07vm0baa_disable(struct drm_panel *panel) > +{ > + struct hitachi_tx10d07vm0baa *priv = to_hitachi_tx10d07vm0baa(panel); > + > + panel_dbi_command(priv, MIPI_DCS_SET_DISPLAY_OFF); > + msleep(35); > + > + panel_dbi_command(priv, MIPI_DCS_ENTER_SLEEP_MODE); > + msleep(50); > + > + return 0; > +} [Severity: High] When this panel is paired with its parent tegra-dbi bridge, does hitachi_tx10d07vm0baa_disable() end up using freed GPIO descriptors when sending MIPI_DCS_SET_DISPLAY_OFF and MIPI_DCS_ENTER_SLEEP_MODE? In drivers/gpu/drm/tegra/dbi.c, tegra_dbi_probe() wraps the panel in a panel bridge via devm_drm_panel_bridge_add_typed() and attaches it downstream of tegra_dbi->bridge in tegra_dbi_attach(). At the end of tegra_dbi_atomic_enable(), tegra-dbi releases its shared DBI GPIOs with gpiod_put() and gpiod_put_array(priv->data_gpios) without clearing the pointers. When the display pipeline is disabled, drm_atomic_bridge_chain_disable() iterates encoder->bridge_chain in reverse order: drm_atomic_bridge_chain_disable() panel_bridge_atomic_disable() drm_panel_disable() hitachi_tx10d07vm0baa_disable() panel_dbi_command() mipi_dbi_command_stackbuf() tegra_dbi_dbi_command() tegra_dbi_write() Because panel_bridge_atomic_disable() runs before tegra_dbi_atomic_disable(), tegra_dbi_write() sees the non-NULL dangling priv->data_gpios pointer and dereferences the freed struct gpio_descs memory and freed GPIO descriptors before tegra_dbi_atomic_disable() has a chance to call tegra_dbi_prepare_gpios(): drivers/gpu/drm/tegra/dbi.c:tegra_dbi_atomic_disable() { struct tegra_dbi_priv *priv = bridge_to_tegra_dbi(bridge); int ret; ret = tegra_dbi_prepare_gpios(priv); if (ret) return; if (priv->output.panel) drm_panel_disable(priv->output.panel); ... } When tegra_dbi_atomic_disable() subsequently runs, its call to drm_panel_disable() becomes a no-op because panel->enabled is already false. > + > +static int hitachi_tx10d07vm0baa_unprepare(struct drm_panel *panel) > +{ [ ... ] > +static const struct drm_display_mode hitachi_tx10d07vm0baa_mode = { > + .clock = (480 + 10 + 10 + 10) * (800 + 4 + 4 + 4) * 60 / 1000, > + .hdisplay = 480, > + .hsync_start = 480 + 10, > + .hsync_end = 480 + 10 + 10, > + .htotal = 480 + 10 + 10 + 10, > + .vdisplay = 800, > + .vsync_start = 800 + 4, > + .vsync_end = 800 + 4 + 4, > + .vtotal = 800 + 4 + 4 + 4, > + .width_mm = 86, > + .height_mm = 52, > + .type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED, > +}; [Severity: Medium] Are width_mm and height_mm swapped in hitachi_tx10d07vm0baa_mode? The active mode resolution is portrait (hdisplay = 480, vdisplay = 800, matching MIPI_DCS_SET_PARTIAL_ROWS 0..799 and MIPI_DCS_SET_SCROLL_AREA 800 in hitachi_tx10d07vm0baa_enable()), whereas width_mm = 86 and height_mm = 52 are landscape dimensions. When hitachi_tx10d07vm0baa_get_modes() calls drm_connector_helper_get_modes_fixed(), those values are copied to connector->display_info, resulting in non-square DPI calculations in userspace. Should width_mm be 52 and height_mm be 86? [ ... ] -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=6
