Re: [PATCH v4 1/5] drm/bridge: it6505: quiesce event sources and work on remove()

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] TOCTOU race in it6505_remove_notifier_module() can trigger a devres WARN_ON.

Pre-existing issues:
- [High] State mismatch causing permanently disabled IRQs when regulator_disable() fails in it6505_poweroff().
--

--- Patch [1]: [PATCH v4 1/5] drm/bridge: it6505: quiesce event sources and work on remove() ---
commit edc834b1a3770ae6b495236b92078500aa7563c6
Author: Daniel Golle <daniel@makrotopia.org>
Subject: drm/bridge: it6505: quiesce event sources and work on remove()

This patch tears down the bridge and cancels background work in the correct
order during remove(). It unregisters the extcon notifier and disables
interrupts before cancelling work items to prevent them from requeuing.

> diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c
> index 8ecb43611dba8..7873ae6df3aff 100644
> --- a/drivers/gpu/drm/bridge/ite-it6505.c
> +++ b/drivers/gpu/drm/bridge/ite-it6505.c
[ ... ]
> @@ -2951,13 +2951,14 @@ static int it6505_use_notifier_module(struct it6505 *it6505)
>  
>  static void it6505_remove_notifier_module(struct it6505 *it6505)
>  {
> -	if (it6505->extcon) {
> -		devm_extcon_unregister_notifier(it6505->dev,
> -						it6505->extcon,	EXTCON_DISP_DP,
> -						&it6505->event_nb);
> +	if (!it6505->extcon || !it6505->event_nb.notifier_call)
> +		return;
>  
> -		flush_work(&it6505->extcon_wq);
> -	}
> +	devm_extcon_unregister_notifier(it6505->dev, it6505->extcon,
> +					EXTCON_DISP_DP, &it6505->event_nb);
> +	it6505->event_nb.notifier_call = NULL;

[Severity: Medium]
Could this introduce a time-of-check to time-of-use (TOCTOU) race condition?

The commit message mentions making this function idempotent so it can be called
from both .detach() and remove(). However, the check of
it6505->event_nb.notifier_call and its subsequent modification don't appear to
be synchronized.

If .detach() and remove() run concurrently, could both threads pass the check
simultaneously and call devm_extcon_unregister_notifier(), triggering a WARN_ON
in devres_release() due to a double-free attempt?

> +
> +	flush_work(&it6505->extcon_wq);
>  }
>  
[ ... ]
> @@ -3647,6 +3649,12 @@ static void it6505_i2c_remove(struct i2c_client *client)
>  	drm_bridge_remove(&it6505->bridge);
>  	drm_dp_aux_unregister(&it6505->aux);
>  	it6505_debugfs_remove(it6505);
> +	it6505_remove_notifier_module(it6505);
> +	disable_irq(it6505->irq);
> +	cancel_work_sync(&it6505->link_works);
> +	cancel_work_sync(&it6505->hdcp_wait_ksv_list);
> +	cancel_delayed_work_sync(&it6505->hdcp_work);
> +	cancel_work_sync(&it6505->extcon_wq);
>  	it6505_poweroff(it6505);

[Severity: High]
This isn't a bug introduced by this patch, but could this power off sequence
lead to permanently disabled IRQs?

Looking at it6505_poweroff(), if regulator_disable() fails:

drivers/gpu/drm/bridge/ite-it6505.c:it6505_poweroff() {
    ...
    disable_irq_nosync(it6505->irq);
    ...
    if (pdata->pwr18) {
        err = regulator_disable(pdata->pwr18);
        if (err)
            return err;
    }
    ...
    it6505->powered = false;
}

The early return would leave it6505->powered == true despite the IRQ being
disabled.

When the device is powered back on, it6505_poweron() checks the state:

drivers/gpu/drm/bridge/ite-it6505.c:it6505_poweron() {
    if (it6505->powered) {
        DRM_DEV_DEBUG_DRIVER(dev, "it6505 already powered on");
        return 0;
    }
    ...
    enable_irq(it6505->irq);
    ...
}

Would this early return skip enable_irq(), leaving the IRQ permanently disabled
and the hardware deaf to hotplug and link training interrupts?

>  	it6505_remove_edid(it6505);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1784681487.git.daniel@makrotopia.org?part=1