Re: [PATCH 1/2] drm/bridge: ti-sn65dsi86: improve HPD interrupt handling

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] Early return bypasses secondary status clearing, causing an IRQ storm.
- [High] Casting opaque `hpd_data` to `struct drm_connector *` causes type confusion and potential kernel panic.
- [High] Lockless connector access introduces a Use-After-Free window.
- [Medium] Unconditionally writing 0xFF to write-1-to-clear status registers causes lost hardware events.
- [Medium] Bitwise ORing negative error codes produces meaningless values.
--

--- Patch [1]: [PATCH 1/2] drm/bridge: ti-sn65dsi86: improve HPD interrupt handling ---
commit 7011d7627c6af8ae9fd09f6d42480bb03a988154
Author: Yashas D <[email protected]>

drm/bridge: ti-sn65dsi86: improve HPD interrupt handling

Fix the interrupt handler to clear all three IRQ status registers
to fully de-assert the IRQ pin, enable replug event detection, and
use per-connector hotplug notification instead of polling all
connectors on every DP HPD event.

> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index 48b83df9aed61..d9bd4ef8f0e21 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c

[ ... ]

> @@ -1387,23 +1391,35 @@ static irqreturn_t ti_sn_bridge_interrupt(int irq, void *private)
>  		return IRQ_NONE;
>  	}
>  
> -	hpd_event = status & (HPD_REMOVAL_STATUS | HPD_INSERTION_STATUS);
> +	hpd_event = status & (HPD_REMOVAL_STATUS | HPD_INSERTION_STATUS |
> +			      HPD_REPLUG_STATUS);
>  
>  	dev_dbg(pdata->dev, "(SN_IRQ_STATUS_REG = %#x)\n", status);
>  	if (!status)
>  		return IRQ_NONE;

[Severity: Critical]
Is this a regression where the early return bypasses the newly added clearing
logic? 

If the interrupt was triggered exclusively by an event in SN_IRQ_STATUS2_REG 
or SN_IRQ_STATUS3_REG, status will be 0 and the handler returns immediately. 
Will the hardware IRQ pin remain asserted, leading to an infinite interrupt
loop?

>  
> -	ret = regmap_write(pdata->regmap, SN_IRQ_STATUS_REG, status);
> +	/*
> +	 * Clear all three IRQ status registers to fully de-assert
> +	 * the IRQ pin
> +	 */
> +	ret  = regmap_write(pdata->regmap, SN_IRQ_STATUS2_REG, 0xFF);

[Severity: Medium]
Will this unconditionally acknowledge and clear all pending events in these 
write-1-to-clear registers? 

Because the registers aren't read first, might this cause the driver to 
permanently lose secondary hardware interrupt events or error flags?

> +	ret |= regmap_write(pdata->regmap, SN_IRQ_STATUS3_REG, 0xFF);
> +	ret |= regmap_write(pdata->regmap, SN_IRQ_STATUS_REG, status);

[Severity: Medium]
Does bitwise ORing the returns of regmap_write() corrupt the error values?

Since regmap_write() returns negative error codes (e.g., -EIO), ORing them 
combines their two's complement bits to produce an unrelated negative number, 
which could mangle the error code passed to dev_err().

>  	if (ret) {
>  		dev_err(pdata->dev, "Failed to clear IRQ status: %d\n", ret);
>  		return IRQ_NONE;
>  	}
>  
> -	/* Only send the HPD event if we are bound with a device. */
> +	/* Notify only the DP connector, not all connectors on the device. */
>  	mutex_lock(&pdata->hpd_mutex);
> -	if (pdata->hpd_enabled && hpd_event)
> -		drm_kms_helper_hotplug_event(dev);
> -	mutex_unlock(&pdata->hpd_mutex);
> +	if (pdata->hpd_enabled && hpd_event && pdata->bridge.hpd_data) {
> +		struct drm_connector *connector =
> +			(struct drm_connector *)pdata->bridge.hpd_data;

[Severity: High]
Does casting the opaque bridge.hpd_data directly to a drm_connector pointer 
cause type confusion? 

The hpd_data pointer is an opaque private context owned by the bridge 
consumer. If a driver passes a different structure or a drm_device pointer, 
will this cause a kernel panic when dereferenced by 
drm_connector_helper_hpd_irq_event()?

> +		mutex_unlock(&pdata->hpd_mutex);
> +		drm_connector_helper_hpd_irq_event(connector);

[Severity: High]
Does dropping the hpd_mutex lock here introduce a use-after-free regression? 

The lifetime of hpd_data is only guaranteed until drm_bridge_hpd_disable() 
returns. If a concurrent unbind operation completes and frees the connector 
memory while this interrupt handler is executing 
drm_connector_helper_hpd_irq_event(), can this result in memory corruption? 

Would it be safer to retain the lock or take a reference via 
drm_connector_get() before unlocking?

> +	} else {
> +		mutex_unlock(&pdata->hpd_mutex);
> +	}
>  
>  	return IRQ_HANDLED;
>  }

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