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
