Re: [PATCH v2 03/10] drm/bridge: analogix_dp: Add IRQF_ONESHOT and simplify IRQ handling

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

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the unconditional clearing of all hotplug interrupt status bits causes permanently lost plug/unplug events.
- [High] This problem wasn't introduced by this patch, but using devm_ allocation for bridge registration inside a component bind callback causes list corruption and memory leaks.
- [High] This problem wasn't introduced by this patch, but the driver translates `-EPROBE_DEFER` from `platform_get_irq` into `-EINVAL`, permanently breaking deferred probing.
--

--- Patch [3]: [PATCH v2 03/10] drm/bridge: analogix_dp: Add IRQF_ONESHOT and simplify IRQ handling ---
commit baa2e5313ef615e574d902fb610ffce244dfc46e
Author: Damon Ding <[email protected]>

drm/bridge: analogix_dp: Add IRQF_ONESHOT and simplify IRQ handling

This patch adds IRQF_ONESHOT to the threaded IRQ request to prevent hardirq
preemption, avoiding a read-modify-write race on HPD interrupt mask
registers. It also simplifies the IRQ handler by removing manual mute/unmute
operations and reordering the interrupt clear call.

> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> index a735b828df7e9..a97a0c9ac9b92 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c

[ ... ]

> @@ -725,6 +723,9 @@ static irqreturn_t analogix_dp_irq_thread(int irq, void *arg)
>  	u32 irq_type;
>  
>  	irq_type = analogix_dp_get_irq_type(dp);
> +	if (irq_type)
> +		analogix_dp_clear_hotplug_interrupts(dp);
> +

[Severity: High]
This isn't a bug introduced by this patch, but does the unconditional
clearing of all hotplug interrupt status bits cause permanently lost
plug/unplug events?

If a new HPD interrupt (e.g., HPD_LOST) asserts in hardware after
analogix_dp_get_irq_type() reads ANALOGIX_DP_COMMON_INT_STA_4, but before
analogix_dp_clear_hotplug_interrupts() unconditionally writes the fixed mask
HOTPLUG_CHG | HPD_LOST | PLUG to it, won't the new event be silently wiped?

Because this is a W1C (write-1-to-clear) register, writing 1s for bits that
were not read or processed could cause the threaded handler to miss the new
event entirely.

[ ... ]

> @@ -1407,10 +1403,11 @@ analogix_dp_probe(struct device *dev, struct analogix_dp_plat_data *plat_data)
>  		 * that we can get the current state of the GPIO.
>  		 */
>  		dp->irq = gpiod_to_irq(dp->hpd_gpiod);
> -		irq_flags = IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING | IRQF_NO_AUTOEN;
> +		irq_flags = IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING | IRQF_NO_AUTOEN |
> +			    IRQF_ONESHOT;
>  	} else {
>  		dp->irq = platform_get_irq(pdev, 0);
> -		irq_flags = IRQF_NO_AUTOEN;
> +		irq_flags = IRQF_NO_AUTOEN | IRQF_ONESHOT;
>  	}
>  
>  	if (dp->irq == -ENXIO) {

[Severity: High]
This is a pre-existing issue, but does this logic translate -EPROBE_DEFER
from platform_get_irq() into -EINVAL, permanently breaking deferred probing?

platform_get_irq() returns -EPROBE_DEFER (-517) if the IRQ provider is not
yet ready. Since the driver explicitly checks only if (dp->irq == -ENXIO)
to catch errors, the -EPROBE_DEFER value is passed down to
devm_request_threaded_irq().

devm_request_threaded_irq() rejects negative IRQ numbers and returns
-EINVAL. By propagating -EINVAL instead of -EPROBE_DEFER, the driver core
will not retry probing, and the display driver permanently fails to
initialize.

[ ... ]

> static int analogix_dp_bind(struct device *dev, struct device *master,
>                             void *data)
> {
>         ...
>         ret = devm_drm_bridge_add(dp->dev, &dp->bridge);
>         ...
> }

[Severity: High]
This isn't a bug introduced by this patch, but does using devm_ allocation
for bridge registration inside a component bind callback cause list
corruption and memory leaks?

analogix_dp_bind() is a component bind callback (triggered by the DRM
master). Calling devm_drm_bridge_add() here binds the bridge's cleanup to
the dp->dev platform device's probe/remove lifecycle, rather than the
component bind/unbind lifecycle.

When the DRM master is unbound, analogix_dp_unbind() executes but the
devres cleanup is not triggered, leaving the bridge in the global
bridge_list.

When the DRM master binds again, analogix_dp_bind() calls
devm_drm_bridge_add() on the same &dp->bridge object. Could this cause
list_add_tail() to double-add a node that is already in the list, resulting
in immediate list corruption?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3