Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider: - [Critical] drm/tegra: dbi: use-after-free and ERR_PTR deref of priv->data_gpios - [High] drm/tegra: dbi: duplicate panel enable/disable and broken disable order - [High] drm/tegra: dbi: wrong V_PULSE position regs in tegra_dbi_atomic_enable() - [High] drm/tegra: dc: unregistering dbi device on probe deferral blocks reprobe - [Medium] drm/tegra: dbi: drm_panel reference leak in tegra_dbi_probe() - [Medium] drm/tegra: dbi: missing MODULE_ALIAS() in tegra_dbi_driver --
commit 3433ae394a211380cb312bda5b319e1df0574de3 Author: Svyatoslav Ryhel <[email protected]> drm/tegra: Add support for 8-bit CPU interface This commit adds a DRM bridge driver for the Tegra20/30 8-bit CPU (MIPI DBI Type B) display controller interface and registers its platform device from the Tegra DC probe path. > diff --git a/drivers/gpu/drm/tegra/dbi.c b/drivers/gpu/drm/tegra/dbi.c > new file mode 100644 > index 0000000000000..4b3966dea0438 > --- /dev/null > +++ b/drivers/gpu/drm/tegra/dbi.c > @@ -0,0 +1,415 @@ [ ... ] > +static void tegra_dbi_write(struct tegra_dbi_priv *priv, u8 type, u8 value) > +{ > + DECLARE_BITMAP(value_bitmap, 8); > + > + value_bitmap[0] = value; > + > + gpiod_set_value(priv->dc_gpio, type); > + > + gpiod_set_value(priv->cs_gpio, 0); > + gpiod_set_value(priv->rw_gpio, 0); > + > + gpiod_set_array_value(priv->data_gpios->ndescs, priv->data_gpios->desc, > + priv->data_gpios->info, value_bitmap); > + > + gpiod_set_value(priv->cs_gpio, 1); > + gpiod_set_value(priv->rw_gpio, 1); > + > + udelay(10); > +} > + > +static int tegra_dbi_dbi_command(struct mipi_dbi *dbi, u8 *cmd, u8 *param, > size_t num) > +{ > + struct tegra_dbi_priv *priv = mipi_dbi_to_tegra_dbi(dbi); > + u8 command = *cmd; > + > + if (!priv->data_gpios) > + return 0; > + > + tegra_dbi_write(priv, TEGRA_DBI_COMMAND, command); > + > + for (int i = 0; i < num; i++) > + tegra_dbi_write(priv, TEGRA_DBI_DATA, param[i]); > + > + return 0; > +} > + > +static int tegra_dbi_prepare_gpios(struct tegra_dbi_priv *priv) > +{ > + struct device *dev = priv->dev; > + int ret; > + > + if (priv->prepared) > + return 0; [ ... ] > + priv->data_gpios = gpiod_get_array(dev, "data", GPIOD_OUT_LOW); > + if (IS_ERR(priv->data_gpios)) { > + ret = PTR_ERR(priv->data_gpios); > + dev_err(dev, "Failed to get data gpios %d\n", ret); > + goto free_dc; > + } > + > + if (priv->data_gpios && priv->data_gpios->ndescs > > TEGRA_DBI_DATA_PINS_MAX) { > + ret = -EINVAL; > + dev_err(dev, "Too many data gpios\n"); > + goto free_data; > + } > + > + priv->prepared = true; > + > + return 0; > + > +free_data: > + gpiod_put_array(priv->data_gpios); > +free_dc: > + gpiod_put(priv->dc_gpio); > +free_rw: > + gpiod_put(priv->rw_gpio); > +free_cs: > + gpiod_put(priv->cs_gpio); > + > + return ret; > +} [Severity: Critical] Can tegra_dbi_dbi_command() -> tegra_dbi_write() dereference a freed or ERR_PTR priv->data_gpios pointer? In tegra_dbi_atomic_enable() and tegra_dbi_atomic_disable(), the driver releases priv->dc_gpio, priv->rw_gpio, and priv->cs_gpio via gpiod_put(), frees priv->data_gpios via gpiod_put_array() (which calls kfree(descs)), and sets priv->prepared = false without holding priv->dbi.cmdlock or resetting the pointers to NULL. Similarly, in tegra_dbi_prepare_gpios(), if gpiod_get_array() fails, priv->data_gpios is left holding a non-NULL ERR_PTR(), and if priv->data_gpios->ndescs > TEGRA_DBI_DATA_PINS_MAX, gpiod_put_array() frees priv->data_gpios without clearing the pointer. Because tegra_dbi_dbi_command() checks: if (!priv->data_gpios) return 0; instead of checking priv->prepared, any MIPI DBI command sent while priv->prepared is false (such as during modeset disable when drm_atomic_bridge_chain_disable() runs panel_bridge_atomic_disable() -> drm_panel_disable() before tegra_dbi_atomic_disable()) will pass a dangling pointer or ERR_PTR to tegra_dbi_write(), dereferencing priv->data_gpios and passing released GPIO descriptors to gpiod_set_value(). > +static void tegra_dbi_atomic_enable(struct drm_bridge *bridge, > + struct drm_atomic_commit *state) > +{ > + struct tegra_dbi_priv *priv = bridge_to_tegra_dbi(bridge); > + struct tegra_dc *dc = priv->dc; > + u32 value; > + int ret; [ ... ] > + tegra_dc_writel(dc, V_PULSE1_ENABLE, DC_DISP_DISP_SIGNAL_OPTIONS0); > + tegra_dc_writel(dc, PULSE_POLARITY_LOW, DC_DISP_V_PULSE1_CONTROL); > + > + tegra_dc_writel(dc, PULSE_END(1), DC_DISP_V_PULSE0_POSITION_A); > + tegra_dc_writel(dc, 0, DC_DISP_V_PULSE0_POSITION_B); > + tegra_dc_writel(dc, 0, DC_DISP_V_PULSE0_POSITION_C); [Severity: High] Should these writes in tegra_dbi_atomic_enable() target DC_DISP_V_PULSE1_POSITION_A, DC_DISP_V_PULSE1_POSITION_B, and DC_DISP_V_PULSE1_POSITION_C instead of the V_PULSE0 position registers? The surrounding code enables V_PULSE1_ENABLE, configures DC_DISP_V_PULSE1_CONTROL, selects DC_SIGNAL_VPULSE1 in DC_DISP_INIT_SEQ_CONTROL, and routes LDC_OUTPUT_SELECT_V_PULSE1, but leaves DC_DISP_V_PULSE1_POSITION_A/B/C unconfigured while overwriting V_PULSE0. [ ... ] > + ret = tegra_dbi_prepare_gpios(priv); > + if (ret) > + return; > + > + if (priv->output.panel) > + drm_panel_enable(priv->output.panel); > + > + gpiod_set_value(priv->cs_gpio, 0); > + > + gpiod_put(priv->dc_gpio); > + gpiod_put(priv->rw_gpio); > + gpiod_put(priv->cs_gpio); > + > + gpiod_put_array(priv->data_gpios); > + priv->prepared = false; > +} > + > +static void tegra_dbi_atomic_disable(struct drm_bridge *bridge, > + struct drm_atomic_commit *state) > +{ > + 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); > + > + gpiod_set_value(priv->cs_gpio, 0); > + > + gpiod_put(priv->dc_gpio); > + gpiod_put(priv->rw_gpio); > + gpiod_put(priv->cs_gpio); > + > + gpiod_put_array(priv->data_gpios); > + priv->prepared = false; > +} [Severity: High] Does wrapping the panel with devm_drm_panel_bridge_add_typed() in tegra_dbi_probe() and attaching it in tegra_dbi_attach() cause duplicate drm_panel_enable() and drm_panel_disable() calls and broken disable ordering? Because priv->output.bridge is attached to encoder->bridge_chain after priv->bridge: 1. On enable, drm_atomic_bridge_chain_pre_enable() calls panel_bridge_atomic_pre_enable() -> drm_panel_prepare() before tegra_dbi_atomic_enable() acquires the DBI GPIOs. Then drm_atomic_bridge_chain_enable() runs tegra_dbi_atomic_enable(), which calls drm_panel_enable(priv->output.panel), and immediately afterward runs panel_bridge_atomic_enable() -> drm_panel_enable(), triggering: dev_warn(panel->dev, "Skipping enable of already enabled panel\n"); 2. On disable, drm_atomic_bridge_chain_disable() walks the bridge chain in reverse order. It runs panel_bridge_atomic_disable() -> drm_panel_disable() before tegra_dbi_atomic_disable() has prepared the DBI GPIOs, and sets panel->enabled = false. When tegra_dbi_atomic_disable() then runs, prepares the GPIOs, and calls drm_panel_disable(priv->output.panel), drm_panel_disable() sees !panel->enabled, logs: dev_warn(panel->dev, "Skipping disable of already disabled panel\n"); and returns without invoking panel->funcs->disable(). [ ... ] > +static int tegra_dbi_probe(struct platform_device *pdev) > +{ [ ... ] > + ret = devm_of_platform_populate(dev); > + if (ret) > + return dev_err_probe(dev, ret, "Failed to probe children\n"); > + > + /* Initialize MIPI DBI interface */ > + mutex_init(&priv->dbi.cmdlock); > + priv->dbi.command = tegra_dbi_dbi_command; > + > + ret = drm_of_find_panel_or_bridge(np, 1, 0, &panel, &bridge); > + if (ret) > + return ret; > + > + if (panel) { > + bridge = devm_drm_panel_bridge_add_typed(dev, panel, > + > DRM_MODE_CONNECTOR_DPI); > + if (IS_ERR(bridge)) > + return PTR_ERR(bridge); > + } [Severity: Medium] Does tegra_dbi_probe() leak the drm_panel reference acquired by drm_of_find_panel_or_bridge()? drm_of_find_panel_or_bridge() -> of_drm_find_panel() increments the panel refcount via drm_panel_get(), and devm_drm_panel_bridge_add_typed() takes an additional devres-managed reference on &panel->bridge, so the reference from drm_of_find_panel_or_bridge() is never dropped with drm_panel_put(). [ ... ] > +static struct platform_driver tegra_dbi_driver = { > + .driver.name = "tegra-dbi", > + .probe = tegra_dbi_probe, > +}; > +module_platform_driver(tegra_dbi_driver); > + > +MODULE_AUTHOR("Svyatoslav Ryhel <[email protected]>"); > +MODULE_DESCRIPTION("Nvidia Tegra20/30 MIPI-DBI Type B driver"); > +MODULE_LICENSE("GPL"); [Severity: Medium] Should tegra_dbi_driver include MODULE_ALIAS("platform:tegra-dbi") (or a platform_device_id table) so the module auto-loads when CONFIG_DRM_TEGRA_DBI=m? In tegra_dc_probe(), the device is registered with .name = "tegra-dbi" and .of_node_reused = true, which causes platform_uevent() to emit MODALIAS=platform:tegra-dbi rather than an OF modalias. > diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c > index 5c67928bcabfa..8f12d30cd2d8b 100644 > --- a/drivers/gpu/drm/tegra/dc.c > +++ b/drivers/gpu/drm/tegra/dc.c [ ... ] > @@ -3382,6 +3389,28 @@ static int tegra_dc_probe(struct platform_device *pdev) > } > } > > + dbi_np = of_get_available_child_by_name(pdev->dev.of_node, "dbi"); > + if (dbi_np) { > + struct platform_device_info dbi_info = { > + .parent = &pdev->dev, > + .name = "tegra-dbi", > + .id = PLATFORM_DEVID_AUTO, > + .fwnode = of_fwnode_handle(dbi_np), > + .of_node_reused = true, > + }; > + > + dc->dbi = platform_device_register_full(&dbi_info); > + of_node_put(dbi_np); > + if (IS_ERR(dc->dbi)) > + return dev_err_probe(&pdev->dev, PTR_ERR(dc->dbi), > + "Failed to create DBI device\n"); > + > + err = devm_add_action_or_reset(&pdev->dev, > tegra_dc_release_dbi_pdev, > + dc->dbi); > + if (err) > + return err; > + } > + > err = tegra_dc_rgb_probe(dc); > if (err < 0 && err != -ENODEV) > return dev_err_probe(&pdev->dev, err, [Severity: High] What happens when tegra_dc_rgb_probe() or tegra_dbi_probe() returns -EPROBE_DEFER? In tegra_dc_probe(), dc->dbi is registered right before tegra_dc_rgb_probe() (which calls tegra_output_probe() -> drm_of_find_panel_or_bridge() to look up the bridge on the dbi node). Similarly, tegra_dbi_probe() calls devm_of_platform_populate(dev) and immediately requires the child panel in drm_of_find_panel_or_bridge() before calling devm_drm_bridge_add(). If tegra-dbi or the child panel driver has not bound yet (for example, when CONFIG_DRM_TEGRA_DBI=m, the panel driver is a module, or a panel dependency defers), tegra_dbi_probe() unpopulates the child panel via devres and tegra_dc_probe() returns -EPROBE_DEFER. When tegra_dc_probe() fails with -EPROBE_DEFER, devres_release_all(&pdev->dev) runs tegra_dc_release_dbi_pdev() -> platform_device_unregister(dc->dbi). With both the tegra-dbi device and its child panel device removed from the platform bus, what will match and bind to trigger a deferred reprobe of tegra_dc? > 完美! Let's double-check all constraints: - Plain text only, no markdown, no backticks, no ALL CAPS in commentary. - Wrapped at 78 characters. - Ends with a blank line. - No line numbers mentioned in commentary. - Severity tags `[Severity: ...]` on their own line right above each inline comment. - All 6 findings (VF1, VF2, VF3, VF4, VF5, PVF1) included. Everything is clean and accurate. Ready to output! -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4
