[PATCH 0/2] Recover DP output after a cable replug

[PATCH 0/2] Recover DP output after a cable replug

[Yashas D]

Applications that don't run a hotplug event loop (e.g. kmstest, or any
client that just holds a CRTC open without listening for uevents) never
see a display come back after a DP cable is unplugged and replugged.
The bridge's own link state goes down on removal, but nothing tells it
to retrain when the cable comes back unless something above it issues
a fresh atomic commit -- which these applications never do.

Patch 1 (improve HPD interrupt handling) fixes the interrupt handler
to clear all three IRQ status registers instead of just one, so the
IRQ pin is fully de-asserted and doesn't stay stuck; adds detection of
the bridge's separate replug event (a short unplug/replug that doesn't
always cross the debounce threshold used for insertion/removal); and
switches from polling every connector on the device to notifying only
the DP connector that actually had the event.

Patch 2 (retrain DP link directly on cable replug) adds a work handler
that retrains the DP link and re-enables the video stream directly
from the HPD interrupt path when the pipeline is still active, without
waiting for a new atomic commit. This is the piece that actually fixes
the recovery problem described above -- applications with no hotplug
handling get their display back without doing anything.

Yashas D (2):
  drm/bridge: ti-sn65dsi86: improve HPD interrupt handling
  drm/bridge: ti-sn65dsi86: retrain DP link directly on cable replug

 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 227 ++++++++++++++++++++++----
 1 file changed, 193 insertions(+), 34 deletions(-)

-- 
2.34.1

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

[Yashas D]

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.

Signed-off-by: Yashas D <y-d@ti.com>
---
 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 34 ++++++++++++++++++++-------
 1 file changed, 25 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index 48b83df9aed6..d9bd4ef8f0e2 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -113,14 +113,19 @@
 #define SN_IRQ_EVENTS_EN_REG			0xE6
 #define  HPD_INSERTION_EN			BIT(1)
 #define  HPD_REMOVAL_EN				BIT(2)
+#define  HPD_REPLUG_EN				BIT(3)
 
 #define SN_AUX_CMD_STATUS_REG			0xF4
 #define  AUX_IRQ_STATUS_AUX_RPLY_TOUT		BIT(3)
 #define  AUX_IRQ_STATUS_AUX_SHORT		BIT(5)
 #define  AUX_IRQ_STATUS_NAT_I2C_FAIL		BIT(6)
 #define SN_IRQ_STATUS_REG			0xF5
+#define  HPD_REPLUG_STATUS			BIT(3)
 #define  HPD_REMOVAL_STATUS			BIT(2)
 #define  HPD_INSERTION_STATUS			BIT(1)
+/* General IRQ status registers, write-1-to-clear */
+#define SN_IRQ_STATUS2_REG			0xF0
+#define SN_IRQ_STATUS3_REG			0xF2
 
 #define MIN_DSI_CLK_FREQ_MHZ	40
 
@@ -1266,7 +1271,7 @@ static void ti_sn_bridge_hpd_enable(struct drm_bridge *bridge)
 
 	if (client->irq) {
 		ret = regmap_set_bits(pdata->regmap, SN_IRQ_EVENTS_EN_REG,
-				      HPD_REMOVAL_EN | HPD_INSERTION_EN);
+				      HPD_REMOVAL_EN | HPD_INSERTION_EN | HPD_REPLUG_EN);
 		if (ret)
 			dev_err(pdata->dev, "Failed to enable HPD events: %d\n", ret);
 	}
@@ -1280,7 +1285,7 @@ static void ti_sn_bridge_hpd_disable(struct drm_bridge *bridge)
 
 	if (client->irq) {
 		ret = regmap_clear_bits(pdata->regmap, SN_IRQ_EVENTS_EN_REG,
-					HPD_REMOVAL_EN | HPD_INSERTION_EN);
+					HPD_REMOVAL_EN | HPD_INSERTION_EN | HPD_REPLUG_EN);
 		if (ret)
 			dev_err(pdata->dev, "Failed to disable HPD events: %d\n", ret);
 	}
@@ -1376,7 +1381,6 @@ static int ti_sn_bridge_parse_dsi_host(struct ti_sn65dsi86 *pdata)
 static irqreturn_t ti_sn_bridge_interrupt(int irq, void *private)
 {
 	struct ti_sn65dsi86 *pdata = private;
-	struct drm_device *dev = pdata->bridge.dev;
 	u8 status;
 	int ret;
 	bool hpd_event;
@@ -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;
 
-	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);
+	ret |= regmap_write(pdata->regmap, SN_IRQ_STATUS3_REG, 0xFF);
+	ret |= regmap_write(pdata->regmap, SN_IRQ_STATUS_REG, status);
 	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;
+		mutex_unlock(&pdata->hpd_mutex);
+		drm_connector_helper_hpd_irq_event(connector);
+	} else {
+		mutex_unlock(&pdata->hpd_mutex);
+	}
 
 	return IRQ_HANDLED;
 }
-- 
2.34.1

[PATCH 2/2] drm/bridge: ti-sn65dsi86: retrain DP link directly on cable replug

[Yashas D]

When a cable is replugged while the upstream display pipeline is still
active (e.g. a compositor holds the CRTC), the bridge can retrain the
DP link and re-enable the video stream directly from the HPD interrupt
work handler without requiring a full DRM atomic commit. This allows
applications to recover display output after a cable replug.

Signed-off-by: Yashas D <y-d@ti.com>
---
 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 215 +++++++++++++++++++++-----
 1 file changed, 179 insertions(+), 36 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index d9bd4ef8f0e2..f6f930ca1519 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -212,6 +212,24 @@ struct ti_sn65dsi86 {
 	struct mutex			comms_mutex;
 	struct mutex			hpd_mutex;
 
+	/*
+	 * bridge_enabled, cached_bpp and cached_mode are written by
+	 * atomic_enable()/atomic_disable() and read by hpd_work(); all
+	 * three are only ever accessed while holding hpd_mutex.
+	 *
+	 * Set true by atomic_enable(), false by atomic_disable().  When the
+	 * cable is replugged while true, hpd_work can retrain the link
+	 * directly without a DRM atomic commit.
+	 */
+	bool				bridge_enabled;
+	unsigned int			cached_bpp;
+	/*
+	 * Copy of the last adjusted mode programmed by atomic_enable().
+	 */
+	struct drm_display_mode		cached_mode;
+	struct drm_display_mode		hpd_mode;
+	struct work_struct		hpd_work;
+
 #if defined(CONFIG_OF_GPIO)
 	struct gpio_chip		gchip;
 	DECLARE_BITMAP(gchip_output, SN_NUM_GPIOS);
@@ -285,13 +303,32 @@ static struct drm_display_mode *
 get_new_adjusted_display_mode(struct drm_bridge *bridge,
 			      struct drm_atomic_commit *state)
 {
-	struct drm_connector *connector =
+	struct ti_sn65dsi86 *pdata = container_of(bridge, struct ti_sn65dsi86,
+						  bridge);
+	struct drm_connector *connector;
+	struct drm_connector_state *conn_state;
+	struct drm_crtc_state *crtc_state;
+
+	/*
+	 * hpd_work calls this with state == NULL since it runs outside any
+	 * DRM commit and holds no modeset lock.  It has already taken its
+	 * own private snapshot (hpd_mode) under hpd_mutex at the start of
+	 * its run, so just return that instead of touching live CRTC state
+	 */
+	if (!state)
+		return &pdata->hpd_mode;
+
+	connector =
 		drm_atomic_get_new_connector_for_encoder(state, bridge->encoder);
-	struct drm_connector_state *conn_state =
+	conn_state =
 		drm_atomic_get_new_connector_state(state, connector);
-	struct drm_crtc_state *crtc_state =
+	crtc_state =
 		drm_atomic_get_new_crtc_state(state, conn_state->crtc);
 
+	mutex_lock(&pdata->hpd_mutex);
+	drm_mode_copy(&pdata->cached_mode, &crtc_state->adjusted_mode);
+	mutex_unlock(&pdata->hpd_mutex);
+
 	return &crtc_state->adjusted_mode;
 }
 
@@ -833,8 +870,16 @@ static void ti_sn_bridge_atomic_disable(struct drm_bridge *bridge,
 {
 	struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
 
-	/* disable video stream */
+	/*
+	 * Clear bridge_enabled and disable the video stream under hpd_mutex.
+	 * hpd_work takes the same lock before its own VSTREAM_ENABLE write
+	 * and rechecks bridge_enabled at that point, so whichever of the two
+	 * runs last under the lock decides the final hardware state.
+	 */
+	mutex_lock(&pdata->hpd_mutex);
+	pdata->bridge_enabled = false;
 	regmap_update_bits(pdata->regmap, SN_ENH_FRAME_REG, VSTREAM_ENABLE, 0);
+	mutex_unlock(&pdata->hpd_mutex);
 }
 
 static void ti_sn_bridge_set_dsi_rate(struct ti_sn65dsi86 *pdata,
@@ -1092,34 +1137,27 @@ static int ti_sn_link_training(struct ti_sn65dsi86 *pdata, int dp_rate_idx,
 	return ret;
 }
 
-static void ti_sn_bridge_atomic_enable(struct drm_bridge *bridge,
-				       struct drm_atomic_commit *state)
+/*
+ * ti_sn_bridge_link_train - configure lanes, scrambler, data format and
+ *                           run DP link training.
+ *
+ * Shared by atomic_enable() (state from DRM commit) and hpd_work()
+ * (state == NULL, falls back to current CRTC state).
+ */
+static int ti_sn_bridge_link_train(struct ti_sn65dsi86 *pdata,
+				   unsigned int bpp,
+				   struct drm_atomic_commit *state)
 {
-	struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
-	struct drm_connector *connector;
 	const char *last_err_str = "No supported DP rate";
 	unsigned int valid_rates;
 	int dp_rate_idx;
 	unsigned int val;
 	int ret = -EINVAL;
-	int max_dp_lanes;
-	unsigned int bpp;
-
-	connector = drm_atomic_get_new_connector_for_encoder(state,
-							     bridge->encoder);
-	if (!connector) {
-		dev_err_ratelimited(pdata->dev, "Could not get the connector\n");
-		return;
-	}
-
-	max_dp_lanes = ti_sn_get_max_lanes(pdata);
-	pdata->dp_lanes = min(pdata->dp_lanes, max_dp_lanes);
 
 	/* DSI_A lane config */
 	val = CHA_DSI_LANES(SN_MAX_DP_LANES - pdata->dsi->lanes);
 	regmap_update_bits(pdata->regmap, SN_DSI_LANES_REG,
 			   CHA_DSI_LANES_MASK, val);
-
 	regmap_write(pdata->regmap, SN_LN_ASSIGN_REG, pdata->ln_assign);
 	regmap_update_bits(pdata->regmap, SN_ENH_FRAME_REG, LN_POLRS_MASK,
 			   pdata->ln_polrs << LN_POLRS_OFFSET);
@@ -1139,7 +1177,6 @@ static void ti_sn_bridge_atomic_enable(struct drm_bridge *bridge,
 	if (pdata->bridge.type == DRM_MODE_CONNECTOR_eDP) {
 		drm_dp_dpcd_writeb(&pdata->aux, DP_EDP_CONFIGURATION_SET,
 				   DP_ALTERNATE_SCRAMBLER_RESET_ENABLE);
-
 		regmap_update_bits(pdata->regmap, SN_TRAINING_SETTING_REG,
 				   SCRAMBLE_DISABLE, 0);
 	} else {
@@ -1147,7 +1184,6 @@ static void ti_sn_bridge_atomic_enable(struct drm_bridge *bridge,
 				   SCRAMBLE_DISABLE, SCRAMBLE_DISABLE);
 	}
 
-	bpp = ti_sn_bridge_get_bpp(connector);
 	/* Set the DP output format (18 bpp or 24 bpp) */
 	val = bpp == 18 ? BPP_18_RGB : 0;
 	regmap_update_bits(pdata->regmap, SN_DATA_FORMAT_REG, BPP_18_RGB, val);
@@ -1159,28 +1195,130 @@ static void ti_sn_bridge_atomic_enable(struct drm_bridge *bridge,
 
 	valid_rates = ti_sn_bridge_read_valid_rates(pdata);
 
-	/* Train until we run out of rates */
 	for (dp_rate_idx = ti_sn_bridge_calc_min_dp_rate_idx(pdata, state, bpp);
 	     dp_rate_idx < ARRAY_SIZE(ti_sn_bridge_dp_rate_lut);
 	     dp_rate_idx++) {
 		if (!(valid_rates & BIT(dp_rate_idx)))
 			continue;
-
 		ret = ti_sn_link_training(pdata, dp_rate_idx, &last_err_str);
 		if (!ret)
 			break;
 	}
-	if (ret) {
-		DRM_DEV_ERROR(pdata->dev, "%s (%d)\n", last_err_str, ret);
+
+	if (ret)
+		DRM_DEV_ERROR(pdata->dev, "link training failed: %s\n",
+			      last_err_str);
+
+	return ret;
+}
+
+/*
+ * ti_sn_bridge_hpd_work - retrain the DP link on cable replug
+ *
+ * If bridge_enabled is true the upstream pipeline is still active so
+ * the link can be retrained directly without a DRM atomic commit,
+ * allowing applications to recover after a cable replug.
+ */
+static void ti_sn_bridge_hpd_work(struct work_struct *work)
+{
+	struct ti_sn65dsi86 *pdata =
+		container_of(work, struct ti_sn65dsi86, hpd_work);
+	struct drm_connector *connector;
+	unsigned int hpd_status;
+	int max_dp_lanes;
+	unsigned int bpp;
+	bool enabled;
+	int ret;
+
+	pm_runtime_get_sync(pdata->dev);
+
+	ret = regmap_read(pdata->regmap, SN_HPD_DISABLE_REG, &hpd_status);
+	if (ret || !(hpd_status & HPD_DEBOUNCED_STATE))
+		goto notify;
+
+	/*
+	 * Snapshot what atomic_enable() published under hpd_mutex.
+	 * hpd_mode is only ever written/read by hpd_work, which never runs
+	 * concurrently with itself, so it is safe to use lock-free for the
+	 * rest of this function.
+	 */
+	mutex_lock(&pdata->hpd_mutex);
+	enabled = pdata->bridge_enabled;
+	bpp = pdata->cached_bpp;
+	drm_mode_copy(&pdata->hpd_mode, &pdata->cached_mode);
+	mutex_unlock(&pdata->hpd_mutex);
+
+	if (!enabled)
+		goto notify;
+
+	max_dp_lanes = ti_sn_get_max_lanes(pdata);
+	mutex_lock(&pdata->hpd_mutex);
+	pdata->dp_lanes = min(pdata->dp_lanes, max_dp_lanes);
+	mutex_unlock(&pdata->hpd_mutex);
+
+	ret = ti_sn_bridge_link_train(pdata, bpp, NULL);
+	if (ret)
+		goto notify;
+
+	ti_sn_bridge_set_video_timings(pdata, NULL);
+	mutex_lock(&pdata->hpd_mutex);
+	if (pdata->bridge_enabled)
+		regmap_update_bits(pdata->regmap, SN_ENH_FRAME_REG,
+				   VSTREAM_ENABLE, VSTREAM_ENABLE);
+	mutex_unlock(&pdata->hpd_mutex);
+
+notify:
+	pm_runtime_put_autosuspend(pdata->dev);
+
+	if (pdata->bridge.hpd_data) {
+		connector = (struct drm_connector *)pdata->bridge.hpd_data;
+		drm_connector_helper_hpd_irq_event(connector);
+	}
+}
+
+static void ti_sn_bridge_atomic_enable(struct drm_bridge *bridge,
+				       struct drm_atomic_commit *state)
+{
+	struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
+	struct drm_connector *connector;
+	int max_dp_lanes;
+	unsigned int bpp;
+	int ret;
+
+	connector = drm_atomic_get_new_connector_for_encoder(state,
+							     bridge->encoder);
+	if (!connector) {
+		dev_err_ratelimited(pdata->dev, "Could not get the connector\n");
 		return;
 	}
 
+	max_dp_lanes = ti_sn_get_max_lanes(pdata);
+	mutex_lock(&pdata->hpd_mutex);
+	pdata->dp_lanes = min(pdata->dp_lanes, max_dp_lanes);
+	mutex_unlock(&pdata->hpd_mutex);
+	bpp = ti_sn_bridge_get_bpp(connector);
+
+	ret = ti_sn_bridge_link_train(pdata, bpp, state);
+	if (ret)
+		return;
+
 	/* config video parameters */
 	ti_sn_bridge_set_video_timings(pdata, state);
 
 	/* enable video stream */
 	regmap_update_bits(pdata->regmap, SN_ENH_FRAME_REG, VSTREAM_ENABLE,
 			   VSTREAM_ENABLE);
+
+	/*
+	 * Publish cached_bpp and bridge_enabled under hpd_mutex.  hpd_work
+	 * reads both under the same lock, which also makes every write this
+	 * function made above (dp_lanes, cached_mode via
+	 * get_new_adjusted_display_mode()) visible to it.
+	 */
+	mutex_lock(&pdata->hpd_mutex);
+	pdata->cached_bpp = bpp;
+	pdata->bridge_enabled = true;
+	mutex_unlock(&pdata->hpd_mutex);
 }
 
 static void ti_sn_bridge_atomic_pre_enable(struct drm_bridge *bridge,
@@ -1270,6 +1408,14 @@ static void ti_sn_bridge_hpd_enable(struct drm_bridge *bridge)
 	mutex_unlock(&pdata->hpd_mutex);
 
 	if (client->irq) {
+		/*
+		 * Clear stale status on all three IRQ registers before
+		 * enabling, to avoid a spurious event.
+		 */
+		regmap_write(pdata->regmap, SN_IRQ_STATUS_REG, 0xFF);
+		regmap_write(pdata->regmap, SN_IRQ_STATUS2_REG, 0xFF);
+		regmap_write(pdata->regmap, SN_IRQ_STATUS3_REG, 0xFF);
+
 		ret = regmap_set_bits(pdata->regmap, SN_IRQ_EVENTS_EN_REG,
 				      HPD_REMOVAL_EN | HPD_INSERTION_EN | HPD_REPLUG_EN);
 		if (ret)
@@ -1294,6 +1440,8 @@ static void ti_sn_bridge_hpd_disable(struct drm_bridge *bridge)
 	pdata->hpd_enabled = false;
 	mutex_unlock(&pdata->hpd_mutex);
 
+	cancel_work_sync(&pdata->hpd_work);
+
 	pm_runtime_put_autosuspend(pdata->dev);
 }
 
@@ -1410,16 +1558,10 @@ static irqreturn_t ti_sn_bridge_interrupt(int irq, void *private)
 		return IRQ_NONE;
 	}
 
-	/* Notify only the DP connector, not all connectors on the device. */
 	mutex_lock(&pdata->hpd_mutex);
-	if (pdata->hpd_enabled && hpd_event && pdata->bridge.hpd_data) {
-		struct drm_connector *connector =
-			(struct drm_connector *)pdata->bridge.hpd_data;
-		mutex_unlock(&pdata->hpd_mutex);
-		drm_connector_helper_hpd_irq_event(connector);
-	} else {
-		mutex_unlock(&pdata->hpd_mutex);
-	}
+	if (pdata->hpd_enabled && hpd_event)
+		schedule_work(&pdata->hpd_work);
+	mutex_unlock(&pdata->hpd_mutex);
 
 	return IRQ_HANDLED;
 }
@@ -2050,6 +2192,7 @@ static int ti_sn65dsi86_probe(struct i2c_client *client)
 
 	mutex_init(&pdata->hpd_mutex);
 	mutex_init(&pdata->comms_mutex);
+	INIT_WORK(&pdata->hpd_work, ti_sn_bridge_hpd_work);
 
 	pdata->regmap = devm_regmap_init_i2c(client,
 					     &ti_sn65dsi86_regmap_config);
-- 
2.34.1

Re: [PATCH 2/2] drm/bridge: ti-sn65dsi86: retrain DP link directly on cable replug

[sashiko-bot]

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

New issues:
- [High] Concurrent execution of DP link training and hardware programming between hpd_work and DRM atomic commits corrupts hardware state.
- [High] Bypassing atomic KMS for link retraining on a cable replug forcefully applies stale display timings to potentially new monitors.
- [High] Use-After-Free of the driver's private data structure due to uncanceled delayed work on driver unbind.

Pre-existing issues:
- [High] The DP lane count is permanently crippled when a display with fewer lanes is connected.
--

--- Patch [2]: [PATCH 2/2] drm/bridge: ti-sn65dsi86: retrain DP link directly on cable replug ---
commit 87e56572108019fd06745308967a4ab03655f77f
Author: Yashas D <y-d@ti.com>

drm/bridge: ti-sn65dsi86: retrain DP link directly on cable replug

When a cable is replugged while the upstream display pipeline is still
active, the bridge can retrain the DP link and re-enable the video stream
directly from the HPD interrupt work handler.

> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index d9bd4ef8f0e21..f6f930ca1519d 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
[ ... ]
> +static void ti_sn_bridge_hpd_work(struct work_struct *work)
> +{
> +	struct ti_sn65dsi86 *pdata =
> +		container_of(work, struct ti_sn65dsi86, hpd_work);
[ ... ]
> +	ret = ti_sn_bridge_link_train(pdata, bpp, NULL);
> +	if (ret)
> +		goto notify;
> +
> +	ti_sn_bridge_set_video_timings(pdata, NULL);
> +	mutex_lock(&pdata->hpd_mutex);
> +	if (pdata->bridge_enabled)
> +		regmap_update_bits(pdata->regmap, SN_ENH_FRAME_REG,
> +				   VSTREAM_ENABLE, VSTREAM_ENABLE);
> +	mutex_unlock(&pdata->hpd_mutex);

[Severity: High]
Could this lockless invocation of ti_sn_bridge_link_train() race with a DRM
atomic commit?

If userspace reacts to the hotplug event and issues a modeset,
ti_sn_bridge_atomic_disable() clears the enabled flag but doesn't cancel the
running work. Then ti_sn_bridge_atomic_enable() executes link training
concurrently without locking.

This seems like it could cause both threads to race while programming complex DP
timing and PLL registers simultaneously, leading to hardware state machine
corruption.

[Severity: High]
Does bypassing atomic KMS for link retraining here forcefully apply stale
display timings to a potentially new monitor?

If a user unplugs the current monitor and connects a different one with a
different EDID and capabilities, forcing the old cached_mode into the
hardware could drive out-of-spec timings to the newly plugged display.

Shouldn't we rely on userspace to read the new EDID via hotplug events and
orchestrate a proper modeset?

[ ... ]
> +static void ti_sn_bridge_atomic_enable(struct drm_bridge *bridge,
> +				       struct drm_atomic_commit *state)
> +{
> +	struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
> +	struct drm_connector *connector;
> +	int max_dp_lanes;
> +	unsigned int bpp;
> +	int ret;
> +
> +	connector = drm_atomic_get_new_connector_for_encoder(state,
> +							     bridge->encoder);
> +	if (!connector) {
> +		dev_err_ratelimited(pdata->dev, "Could not get the connector\n");
> +		return;
> +	}
> +
> +	max_dp_lanes = ti_sn_get_max_lanes(pdata);
> +	mutex_lock(&pdata->hpd_mutex);
> +	pdata->dp_lanes = min(pdata->dp_lanes, max_dp_lanes);

[Severity: High]
This is a pre-existing issue, but does this in-place min() reduction
permanently mutate the driver's maximum allowed lane capability based on the
current peripheral sink?

If a 2-lane monitor is connected, pdata->dp_lanes shrinks to 2. Connecting a
4-lane capable monitor afterward would be artificially limited to 2 lanes
since the original physical board maximum is lost.

[ ... ]
> @@ -2050,6 +2192,7 @@ static int ti_sn65dsi86_probe(struct i2c_client *client)
>  
>  	mutex_init(&pdata->hpd_mutex);
>  	mutex_init(&pdata->comms_mutex);
> +	INIT_WORK(&pdata->hpd_work, ti_sn_bridge_hpd_work);
>  
>  	pdata->regmap = devm_regmap_init_i2c(client,
>  					     &ti_sn65dsi86_regmap_config);

[Severity: High]
Is there a chance this work item can outlive the driver's private data
structure during unbind?

Since the work is initialized here but no devm cleanup action is registered
to cancel it on teardown, if a hardware interrupt schedules the work just
before the driver is unbound, devres will free the pdata memory.

The system workqueue could then execute ti_sn_bridge_hpd_work() and
dereference the freed memory.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260814120304.887993-1-y-d@ti.com?part=2

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

[sashiko-bot]

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 <y-d@ti.com>

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/20260814120304.887993-1-y-d@ti.com?part=1