Re: [PATCH] drm/bridge: adv7511: cancel HPD work during teardown

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] drm_bridge_add() is called prematurely in adv7511_probe(), exposing the bridge to concurrent access before initialization completes and risking a Use-After-Free if later probe steps fail.
- [High] Hardware regulators are disabled before the DRM bridge is unregistered in adv7511_remove(), allowing concurrent I2C access to powered-down hardware.
- [High] Concurrent execution of HPD work and bridge detection modifies hardware sequences and state without synchronization.
--

--- Patch [1]: [PATCH] drm/bridge: adv7511: cancel HPD work during teardown ---
commit b44080d3f93c1a423486c2876b7db016031c7403
Author: Hongyan Xu <getshell@seu.edu.cn>

drm/bridge: adv7511: cancel HPD work during teardown

The threaded IRQ handler queues hpd_work. The work accesses the regmap
and calls drm_bridge_hpd_notify(), so it must not run after the bridge or
the devm-allocated driver data has been released.

Free the IRQ before cancelling hpd_work so the handler cannot queue the
work again. Do this in remove and when adv7533_attach_dsi() fails after
the IRQ has been registered.

The remove-side race was reported previously, but the proposed fix did
not cover the late probe failure path.

This issue was found by a static analysis tool.

Link: https://lore.kernel.org/r/20230413065943.198088-1-zyytlz.wz@163.com

> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> index db99e252aeebc..316b9f80d7429 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> @@ -1400,11 +1400,15 @@ static int adv7511_probe(struct i2c_client *i2c)
>  	if (adv7511->info->has_dsi) {
>  		ret = adv7533_attach_dsi(adv7511);
>  		if (ret)
> -			goto err_unregister_audio;
> +			goto err_free_irq;
>  	}
>  
>  	return 0;
>  
> +err_free_irq:
> +	if (i2c->irq)
> +		devm_free_irq(dev, i2c->irq, adv7511);
> +	cancel_work_sync(&adv7511->hpd_work);
>  err_unregister_audio:
>  	drm_bridge_remove(&adv7511->bridge);

[Severity: High]
This is a pre-existing issue, but is it safe to expose the bridge before probe
completes?

drm_bridge_add() is called earlier in adv7511_probe(), before critical
structures like the waitqueue are initialized:

	drm_bridge_add(&adv7511->bridge);

	if (i2c->irq) {
		init_waitqueue_head(&adv7511->wq);

If a concurrent KMS driver discovers the bridge and initiates operations,
could it crash on the uninitialized waitqueue?

For example, adv7511_wait_for_edid() accesses this waitqueue:

		ret = wait_event_interruptible_timeout(adv7511->wq,
				adv7511->edid_read, msecs_to_jiffies(timeout));

Also, if the probe subsequently fails and we reach this cleanup path,
could the concurrent KMS driver be left with a dangling pointer to
devres-allocated memory?

[ ... ]
> @@ -1425,6 +1429,10 @@ static void adv7511_remove(struct i2c_client *i2c)
>  {
>  	struct adv7511 *adv7511 = i2c_get_clientdata(i2c);
>  
> +	if (i2c->irq)
> +		devm_free_irq(&i2c->dev, i2c->irq, adv7511);
> +	cancel_work_sync(&adv7511->hpd_work);
> +

[Severity: High]
This isn't a bug introduced by this patch, but does the driver have an
unsynchronized concurrency issue between HPD work and bridge detection?

adv7511_hpd_work() runs asynchronously and modifies state:

	if (status == connector_status_connected &&
	    adv7511->status == connector_status_disconnected &&
	    adv7511->powered) {
		regcache_mark_dirty(adv7511->regmap);
		adv7511_power_on(adv7511);
	}

	if (adv7511->status != status) {
		adv7511->status = status;

At the same time, KMS can concurrently call adv7511_detect():

	if (status == connector_status_connected && hpd && adv7511->powered) {
		regcache_mark_dirty(adv7511->regmap);
		adv7511_power_on(adv7511);
		if (adv7511->status == connector_status_connected)
			status = connector_status_disconnected;
	}

Since neither function holds a lock while reading and modifying
adv7511->status and adv7511->powered, could both evaluate the state as
requiring a power cycle?

This might cause both to concurrently invoke adv7511_power_on(),
resulting in non-atomic, interleaved I2C register writes and overlapping
regcache_sync operations.

>  	of_node_put(adv7511->host_node);
>  
>  	adv7511_uninit_regulators(adv7511);
>  
>  	drm_bridge_remove(&adv7511->bridge);

[Severity: High]
This is a pre-existing issue, but does removing regulators before the
bridge is unregistered allow concurrent I2C access to powered-down
hardware?

In adv7511_remove(), adv7511_uninit_regulators() cuts power to the chip.

If a concurrent modeset, hotplug polling thread, or sysfs EDID read
invokes bridge operations (like adv7511_detect() or adv7511_edid_read())
before drm_bridge_remove() executes, could the driver attempt I2C
transactions on an unresponsive chip?

This could potentially cause hardware crashes, I2C bus timeouts, or
driver lockups.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260728135205.859-1-getshell@seu.edu.cn?part=1