Re: [PATCH v2 4/7] drm/tegra: Add support for 8-bit CPU interface

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [Critical] drm/tegra: UAF of GPIO descriptors during MIPI DCS panel disable
- [High] drm/tegra: panel init commands in prepare() silently dropped
- [High] drm/tegra: sleeping in atomic context in bridge callbacks
- [Medium] drm/tegra: tegra-dbi platform device leaked on dc probe error paths
- [Medium] drm/tegra: double-enable/disable warnings from redundant manual panel calls
- [Medium] drm/tegra: leaked DRM panel reference from drm_of_find_panel_or_bridge
- [Low] drm/tegra: unused idx variable in tegra_dbi_priv
--

commit 0271b6e2272161d6c9c79ab99f61fc6699770373
Author: Svyatoslav Ryhel <clamor95@gmail.com>
Subject: 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/dbi.c b/drivers/gpu/drm/tegra/dbi.c
> new file mode 100644
> index 0000000000000..74a1051b86778
> --- /dev/null
> +++ b/drivers/gpu/drm/tegra/dbi.c
> @@ -0,0 +1,416 @@
[ ... ]
> +struct tegra_dbi_priv {
> +	struct mipi_dbi dbi;		/* must be first */
> +	struct device *dev;
> +	struct tegra_dc *dc;
> +
> +	struct gpio_desc *dc_gpio;
> +	struct gpio_desc *rw_gpio;
> +	struct gpio_desc *cs_gpio;
> +	struct gpio_descs *data_gpios;
> +
> +	struct drm_bridge bridge;
> +	struct tegra_dbi_output output;
> +
> +	u32 spi_init_seq[4];
> +	u32 bus_width;
> +	u32 idx;

[Severity: Low]
Is the idx field needed here? It doesn't appear to be used anywhere in the
driver.

> +	bool prepared;
> +};
[ ... ]
> +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);

[Severity: Critical]
Could this dereference freed memory during display teardown?

During a disable cycle, panel_bridge_disable() runs before
tegra_dbi_atomic_disable() and sends DCS commands through
tegra_dbi_dbi_command() -> tegra_dbi_write(). However, the GPIO descriptors
were already freed by gpiod_put_array() at the end of tegra_dbi_atomic_enable(),
leaving dangling pointers in priv->data_gpios.

> +
> +	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;

[Severity: High]
Does this silently drop panel initialization commands?

The drm_panel_prepare() hook executes during panel_bridge_pre_enable(), which
happens before tegra_dbi_atomic_enable(). Because data_gpios is only allocated
later during atomic_enable, priv->data_gpios is NULL here and the initialization
commands get ignored.

> +
> +	tegra_dbi_write(priv, TEGRA_DBI_COMMAND, command);
[ ... ]
> +static int tegra_dbi_prepare_gpios(struct tegra_dbi_priv *priv)
> +{
> +	struct device *dev = priv->dev;
> +	int ret;
> +
> +	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->cs_gpio = gpiod_get(dev, "cs", GPIOD_OUT_HIGH);
[ ... ]
> +	priv->data_gpios = gpiod_get_array(dev, "data", GPIOD_OUT_LOW);

[Severity: High]
Can this sleep in atomic context?

tegra_dbi_prepare_gpios() is called unconditionally by tegra_dbi_atomic_enable(),
which executes in an atomic context during non-blocking commits. Both
gpiod_get() and gpiod_get_array() can sleep as they acquire mutexes and perform
GFP_KERNEL allocations.

> +	if (IS_ERR(priv->data_gpios)) {
[ ... ]
> +static void tegra_dbi_atomic_enable(struct drm_bridge *bridge,
> +				    struct drm_atomic_commit *state)
> +{
[ ... ]
> +	if (priv->output.panel)
> +		drm_panel_enable(priv->output.panel);

[Severity: Medium]
Is this manual panel enablement redundant?

The DRM core automatically manages panel enablement via the attached panel_bridge
in the bridge chain. Calling drm_panel_enable() here duplicates the operation
that panel_bridge_enable() will perform immediately afterward, which will cause
kernel warnings about enabling an already enabled 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);

[Severity: Medium]
Same issue as above. Does this redundant call conflict with the panel_bridge's
automatic lifecycle management, causing warnings about disabling an already
disabled panel?

> +
> +	gpiod_set_value(priv->cs_gpio, 0);
[ ... ]
> +static int tegra_dbi_probe(struct platform_device *pdev)
> +{
[ ... ]
> +	/* 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);

[Severity: Medium]
Does this code leak the panel reference?

drm_of_find_panel_or_bridge() acquires a reference to the panel, but
drm_panel_put() is never called in the error paths, and there doesn't appear
to be a driver remove function to drop the reference when the driver unbinds.

> +	if (ret)
> +		return ret;
[ ... ]
> diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
> index 5c67928bcabfa..dae2b14f79e31 100644
> --- a/drivers/gpu/drm/tegra/dc.c
> +++ b/drivers/gpu/drm/tegra/dc.c
[ ... ]
> @@ -3382,6 +3383,23 @@ 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 = tegra_dc_rgb_probe(dc);
>  	if (err < 0 && err != -ENODEV)
>  		return dev_err_probe(&pdev->dev, err,
>  				     "failed to probe RGB output\n");

[Severity: Medium]
Are we leaking the dc->dbi platform device on error paths?

If tegra_dc_rgb_probe() or host1x_client_register() fails, the function returns
without calling platform_device_unregister() to clean up the registered dc->dbi
device. This cleanup also appears to be missing in the disable_pm error label.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261004163114.181800-1-clamor95@gmail.com?part=4