Re: [PATCH v3 12/15] drm/tidss: oldi: Convert OLDI to an aux driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:

New issues:
- [High] Hardware is left powered on due to cancellation of asynchronous suspend during device removal.
- [High] The auxiliary driver can be unbound via sysfs, freeing the DRM bridge while it is still attached.
- [High] `device_link_add()` fails unconditionally because the consumer device is not yet registered.
- [High] Ignoring `pm_runtime_get_sync()` failure leads to unsafe hardware access on unpowered registers.
- [Medium] Device tree node reference leak when `auxiliary_device_init()` fails.
- [Medium] Missing `pm_runtime_mark_last_busy()` before `pm_runtime_put_autosuspend()` causes immediate suspend.

Pre-existing issues:
- [High] Out-of-bounds write to `tidss->oldis` array due to missing bounds check.
--

commit 73c9b05b12dc4ef6e124725962ba5094aea8a043
Author: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
Date:   Fri May 29 08:45:42 2026 +0000

drm/tidss: oldi: Convert OLDI to an aux driver

This patch converts the OLDI components from child devices parsed directly 
by tidss into auxiliary devices with their own auxiliary driver. This 
allows the driver framework to automatically manage the power domains 
associated with the OLDI nodes.

> diff --git a/drivers/gpu/drm/tidss/tidss_oldi.c b/drivers/gpu/drm/tidss/tidss_oldi.c
> index e925ddaa4fd6a..188f31b57c6a2 100644
> --- a/drivers/gpu/drm/tidss/tidss_oldi.c
> +++ b/drivers/gpu/drm/tidss/tidss_oldi.c

[ ... ]

> @@ -251,6 +259,8 @@ static void tidss_oldi_atomic_pre_enable(struct drm_bridge *bridge,
>  	if (oldi->link_type == OLDI_MODE_SECONDARY_CLONE_SINGLE_LINK)
>  		return;
>  
> +	WARN_ON(pm_runtime_get_sync(oldi->dev) < 0);
> +

[Severity: High]
If pm_runtime_get_sync() fails and returns a negative error code, does this 
code continue without returning?

Proceeding to access hardware registers via tidss_oldi_config() on an 
unclocked or unpowered domain can cause bus timeouts or synchronous 
external aborts.

>  	connector = drm_atomic_get_new_connector_for_encoder(state,
>  							     bridge->encoder);
>  	if (WARN_ON(!connector))

[ ... ]

> @@ -296,6 +306,8 @@ static void tidss_oldi_atomic_post_disable(struct drm_bridge *bridge,
>  
>  	/* Clear OLDI Config */
>  	tidss_disable_oldi(oldi->tidss, oldi->parent_vp);
> +
> +	pm_runtime_put_autosuspend(oldi->dev);
>  }

[Severity: Medium]
Is pm_runtime_mark_last_busy() missing before the call to 
pm_runtime_put_autosuspend()?

Without updating the device's last busy timestamp, the PM core's autosuspend 
timer might see the time elapsed as greater than the 500ms delay and suspend 
the device immediately, bypassing the autosuspend window.

>  
>  #define MAX_INPUT_SEL_FORMATS	1

[ ... ]

> +static void tidss_oldi_remove(struct auxiliary_device *auxdev)
> +{
> +	struct tidss_oldi *oldi = auxiliary_get_drvdata(auxdev);
> +	struct tidss_device *tidss = oldi->tidss;
> +	struct device *dev = &auxdev->dev;
>  
> -	tidss->num_oldis = 0;
> +	pm_runtime_dont_use_autosuspend(dev);
> +	pm_runtime_disable(dev);

[Severity: High]
Does this leave the hardware powered on during unbind?

During driver unbind, tidss_remove() shuts down the display by calling 
drm_atomic_helper_shutdown(), which invokes tidss_oldi_atomic_post_disable() 
and schedules an asynchronous suspend via pm_runtime_put_autosuspend(). 

Immediately afterward, tidss_remove() triggers tidss_oldi_remove(), which 
calls pm_runtime_disable() here. The PM subsystem cancels pending 
asynchronous suspend work when pm_runtime_disable() is called, leaving the 
device indefinitely in the active state.

Would it be safer to flush the suspend work or use pm_runtime_put_sync() 
during shutdown?

> +
> +	if (oldi->link_type != OLDI_MODE_SECONDARY_DUAL_LINK) {
> +		drm_bridge_remove(&oldi->bridge);
> +
> +		tidss->is_ext_vp_clk[oldi->parent_vp] = false;
> +	}
> +
> +	clk_put(oldi->serial);
> +}
> +
> +static const struct auxiliary_device_id tidss_oldi_aux_id_table[] = {
> +	{ .name = "tidss.oldi" },
> +	{}
> +};
> +
> +static struct auxiliary_driver oldi_aux_driver = {
> +	.name = "oldi",
> +	.probe = tidss_oldi_probe,
> +	.remove = tidss_oldi_remove,
> +	.id_table = tidss_oldi_aux_id_table,
> +};

[Severity: High]
Can userspace independently unbind the auxiliary driver via sysfs and cause 
a use-after-free?

Unlike the main tidss driver, this auxiliary driver does not set 
suppress_bind_attrs = true on its driver struct. Unbinding the device through 
sysfs would trigger devres cleanup, freeing the struct tidss_oldi and the 
embedded drm_bridge.

Because the main tidss DRM device remains active, the KMS pipeline would 
retain pointers to the freed bridge, leading to memory corruption on the 
next atomic commit or teardown.

> +
> +static void tidss_oldi_aux_device_release(struct device *dev)
> +{

[ ... ]

> +	/*
> +	 * Create a device-link between the primary and the secondary, so that
> +	 * the secondary will be powered on when the primary is used.
> +	 */
> +	if (companion_auxdev) {
> +		link = device_link_add(&auxdev->dev, &companion_auxdev->dev,
> +				       DL_FLAG_PM_RUNTIME |
> +					       DL_FLAG_AUTOREMOVE_CONSUMER |
> +					       DL_FLAG_AUTOREMOVE_SUPPLIER);

[Severity: High]
Does this device_link_add() call unconditionally fail because the consumer 
device is not yet registered?

The device link core requires the consumer device to be registered with 
sysfs when the DL_FLAG_AUTOREMOVE_CONSUMER flag is used. At this point, 
auxdev has only been initialized via auxiliary_device_init(), and 
auxiliary_device_add() is not called until later in this function.

This would cause device_link_add() to return NULL, breaking dual-link OLDI 
setups.

> +		if (!link) {
> +			dev_err(tidss->dev,
> +				"device_link_add failed between primary and secondary OLDI\n");
> +			ret = -EINVAL;
> +			goto err_uninit_auxdev;
>  		}
> +	}
>  
> -		oldi->parent_vp = parent_vp;
> -		oldi->oldi_instance = oldi_instance;
> -		oldi->companion_instance = companion_instance;
> -		oldi->link_type = link_type;
> -		oldi->dev = tidss->dev;
> -		oldi->next_bridge = bridge;
> +	ret = auxiliary_device_add(auxdev);
> +	if (ret) {
> +		dev_err(tidss->dev, "OLDI auxiliary_device_add failed: %d\n",
> +			ret);
> +		goto err_link_del;
> +	}
>  
> -		/*
> -		 * Only the primary OLDI needs to reference the io-ctrl system
> -		 * registers, and the serial clock.
> -		 * We don't require a check for secondary OLDI in dual-link mode
> -		 * because the driver will not create a drm_bridge instance.
> -		 * But the driver will need to create a drm_bridge instance,
> -		 * for secondary OLDI in clone mode (once it is supported).
> -		 */
> -		if (link_type != OLDI_MODE_SECONDARY_CLONE_SINGLE_LINK) {
> -			oldi->io_ctrl = syscon_regmap_lookup_by_phandle(child,
> -									"ti,oldi-io-ctrl");
> -			if (IS_ERR(oldi->io_ctrl)) {
> -				ret = dev_err_probe(oldi->dev, PTR_ERR(oldi->io_ctrl),
> -						    "OLDI%u: syscon_regmap_lookup_by_phandle failed.\n",
> -						    oldi_instance);
> -				goto err_put_node;
> -			}
> +	tidss->oldis[tidss->num_oldis++] = auxdev;

[Severity: High]
This is a pre-existing issue, but can this cause an out-of-bounds write to 
the tidss->oldis array?

The oldis array in struct tidss_device has a fixed size of 
TIDSS_MAX_OLDI_TXES (2). The devices are appended here without a bounds 
check. If a malformed device tree provides more than two OLDI transmitter 
nodes, this could overwrite adjacent members like irq and irq_lock.

>  
> -		/* Register the bridge. */
> -		oldi->bridge.of_node = child;
> -		oldi->bridge.driver_private = oldi;
> +	return auxdev;
>  
> -		tidss->oldis[tidss->num_oldis++] = oldi;
> -		tidss->is_ext_vp_clk[oldi->parent_vp] = true;
> -		oldi->tidss = tidss;
> +err_link_del:
> +	if (link)
> +		device_link_del(link);
> +err_uninit_auxdev:
> +	auxiliary_device_uninit(auxdev);
> +	/* return here, as the rest are done in auxdev's release */
> +	return ERR_PTR(ret);
>  
> -		drm_bridge_add(&oldi->bridge);
> +err_free_auxdev:
> +	kfree(auxdev);
> +err_free_pdata:
> +	kfree(oldi_pdata);
> +err_free_ida:
> +	ida_free(&tidss_oldi_ida, oldi_aux_id);
>  
> -	of_node_put(child);
> -	of_node_put(oldi_parent);
> +	return ERR_PTR(ret);
> +}

[Severity: Medium]
Does this error path leak the OF node reference?

Earlier in this function, an OF node reference is explicitly acquired and 
assigned to auxdev->dev.of_node:

    .of_node = of_node_get(oldi_tx),

If auxiliary_device_init(auxdev) fails, the err_free_auxdev path frees the 
auxdev structure but does not call of_node_put(), permanently leaking the 
reference.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260529-beagley-ai-display-v3-0-7fefdc5d1adf@ideasonboard.com?part=12