Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider: - [Critical] drm: Use-after-free of GPIO descriptors during bridge atomic_disable - [High] drm: Double free of panel bridge resource via manual release - [High] gpio: devm resource leak on transient GPIO allocation failures - [Medium] drm: display controller platform device reference leak in attach - [Medium] gpio: devm_gpiod_put(NULL) on absent optional GPIOs triggers WARN_ON - [Medium] drm: Duplicate drm_panel_enable()/disable() causes bridge orchestration warnings - [Medium] drm: Missing drm_panel_put() causes panel reference leak --
commit 7416e1ec1121b26f661bbd6bfc2ff22ea8f657f1 Author: Svyatoslav Ryhel <[email protected]> drm/tegra: Add support for 8-bit CPU interface The display controller in Tegra20/30 SoCs features an 8-bit SPI interface that closely resembles the MIPI DBI Type B protocol and is referred to as '8-bit CPU'. Each display controller provides two such interfaces, which can be used to send MIPI DCS commands to initialize and control the panel while image data is transmitted via 16/18/24-line RGB. > diff --git a/drivers/gpu/drm/tegra/cpu-bridge.c > b/drivers/gpu/drm/tegra/cpu-bridge.c > new file mode 100644 > index 0000000000000..63ca87a0be909 > --- /dev/null > +++ b/drivers/gpu/drm/tegra/cpu-bridge.c [ ... ] > +static int tegra_cpu_bridge_prepare_gpios(struct tegra_cpu_bridge_priv *priv) > +{ > + struct device *dev = priv->dev; > + > + if (priv->prepared) > + return 0; > + > + /* > + * Control and data GPIOs are shared between SPI and DPI. The bridge > + * driver must get and put the required GPIOs each time DCS commands > + * are transmitted. > + */ > + > + priv->data_gpios = devm_gpiod_get_array_optional(dev, "data", > GPIOD_OUT_LOW); > + if (IS_ERR(priv->data_gpios)) { > + dev_err(dev, "Failed to get data gpios %ld\n", > PTR_ERR(priv->data_gpios)); > + return PTR_ERR(priv->data_gpios); > + } > + > + if (priv->data_gpios && priv->data_gpios->ndescs > > CPU_BRIDGE_DATA_PINS_MAX) { > + dev_err(dev, "Too many data gpios\n"); > + return -EINVAL; > + } > + > + priv->cs_gpio = devm_gpiod_get_optional(dev, "cs", GPIOD_OUT_HIGH); > + if (IS_ERR(priv->cs_gpio)) { > + dev_err(dev, "Failed to get CS GPIO: %ld\n", > PTR_ERR(priv->cs_gpio)); > + return PTR_ERR(priv->cs_gpio); [Severity: High] If one of these subsequent allocations fails, does it leak the memory allocated for the earlier devm_gpiod_get_array_optional()? Since this function is called inside a repeatable lifecycle function (the atomic enable/disable paths), a transient failure could repeatedly allocate devres memory without freeing it. This could permanently bind the GPIO to the device and prevent successful recovery on subsequent modesets. [ ... ] > +static void tegra_cpu_bridge_atomic_enable(struct drm_bridge *bridge, > + struct drm_atomic_commit *state) > +{ [ ... ] > + ret = tegra_cpu_bridge_prepare_gpios(priv); > + if (ret) > + return; > + > + if (priv->output.panel) > + drm_panel_enable(priv->output.panel); [Severity: Medium] Does the DRM core already handle this? The panel is part of the bridge chain via devm_drm_panel_bridge_add_typed(), meaning the atomic helpers should automatically call drm_panel_enable() and drm_panel_disable(). Manually calling it here could duplicate the work and trigger a "Skipping enable of already enabled panel" warning in the kernel logs. > + > + gpiod_set_value(priv->cs_gpio, 0); > + > + devm_gpiod_put(priv->dev, priv->dc_gpio); > + devm_gpiod_put(priv->dev, priv->rw_gpio); > + devm_gpiod_put(priv->dev, priv->cs_gpio); [Severity: Medium] What happens if the optional GPIOs are absent and return NULL during allocation? devm_gpiod_get_optional() returns NULL without adding a devres cleanup action. Unconditionally calling devm_gpiod_put() on a NULL pointer could cause the devres subsystem to search for a NULL action and trigger a WARN_ON stack trace in devm_release_action(). > + > + devm_gpiod_put_array(priv->dev, priv->data_gpios); [Severity: Critical] Could freeing the GPIO array here lead to a use-after-free during the disable sequence? During a modeset disable, the panel's disable callback is invoked first. This sends MIPI DCS commands via tegra_cpu_bridge_dbi_command(), which calls tegra_cpu_bridge_write(). tegra_cpu_bridge_write() accesses priv->data_gpios: gpiod_set_array_value(priv->data_gpios->ndescs, priv->data_gpios->desc, priv->data_gpios->info, value_bitmap); But since tegra_cpu_bridge_atomic_disable() hasn't run yet to re-allocate the GPIOs, this would execute using the dangling priv->data_gpios pointer that was freed here during the previous enable. [ ... ] > +static int tegra_cpu_bridge_attach(struct drm_bridge *bridge, > + struct drm_encoder *encoder, > + enum drm_bridge_attach_flags flags) > +{ [ ... ] > + dc_pdev = of_find_device_by_node(dc_node); > + of_node_put(dc_node); > + if (!dc_pdev) > + return -ENODEV; > + > + priv->dc = platform_get_drvdata(dc_pdev); > + if (!priv->dc) > + return -ENODEV; [Severity: Medium] Does this code leak the reference count to the platform device? of_find_device_by_node() increments the kobject reference count for the device. Is it necessary to call put_device() on the returned device after getting the driver data so it isn't permanently pinned? [ ... ] > +static int tegra_cpu_bridge_probe(struct platform_device *pdev) > +{ [ ... ] > + 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] If we return an error here, do we leak the panel reference? drm_of_find_panel_or_bridge() acquires a reference to the panel object, but there doesn't seem to be a matching drm_panel_put() on this error path or during normal module teardown in tegra_cpu_bridge_remove(). [ ... ] > +static void tegra_cpu_bridge_remove(struct platform_device *pdev) > +{ > + struct tegra_cpu_bridge_priv *priv = platform_get_drvdata(pdev); > + > + drm_bridge_remove(&priv->bridge); > + if (priv->output.panel) > + drm_panel_bridge_remove(priv->output.bridge); [Severity: High] Does this explicitly free a resource that is already managed by devres? The bridge was allocated using devm_drm_panel_bridge_add_typed(), which schedules automatic cleanup. Manually calling drm_panel_bridge_remove() here will decrement the bridge reference count, likely causing a double-free when the devres subsystem executes its own teardown action on module removal. > +} -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=4
