[PATCH v4 0/3] drm/bridge: ti-sn65dsi83: Various fixes

[PATCH v4 0/3] drm/bridge: ti-sn65dsi83: Various fixes

[Esben Haabendal]

This small series adds support for using SN65DSI84 in single-link mode with
output to LVDS channel B, and provides a couple of small fixes for
error-handling in sn65dsi83_reset_pipe() and sn65dsi83_reset_work().

Signed-off-by: Esben Haabendal <esben@geanix.com>
---
Changes in v4:
- Swapped patch 1 and 2 for proper ordering.
- Ensure enable_irq() is called in sn65dsi83_reset_work() error handling
  path.
- Added Fixes and Cc: stable tags to patch 1 and 2.
- Link to v3: https://patch.msgid.link/20260803-ti-sn65dsi83-fixes-v3-0-9b403a405ccd@geanix.com

Changes in v3:
- Rename panel_port variable to output_port.
- Propagate error code from drm_bridge_helper_reset_crtc() in
  sn65dsi83_reset_pipe().
- Drop with fix for premature PLL locking. Should be fixed in DSI host
  driver (or upstream bridge driver) instead.
  See https://lore.kernel.org/all/48f6f55f-43fd-4355-88b9-d2e2d0e26d07@kontron.de/
- Link to v2: https://patch.msgid.link/20260715-ti-sn65dsi83-fixes-v2-0-ebc4c3fe29b6@geanix.com

Changes in v2:
- Fix error handling in sn65dsi83_atomic_enable() to ensure
  drm_brige_exit() is always called on exit.
- Change logging level from warn to dbg for two messages in
  sn65dsi83_parse_dt().
- Removed trailing dot in comment lines.
- Added patch with fix so that DRM bridge critical section is exited on
  error in sn65dsi83_reset_work().
- Added Fixes and Cc: stable tags to the premature PLL patch.
- Re-ordered patch so fixes comes first.
- Link to v1: https://patch.msgid.link/20260711-ti-sn65dsi83-fixes-v1-0-d85eb5342b98@geanix.com

To: Luca Ceresoli <luca.ceresoli@bootlin.com>
To: Herve Codina <herve.codina@bootlin.com>
To: Andrzej Hajda <andrzej.hajda@intel.com>
To: Neil Armstrong <neil.armstrong@linaro.org>
To: Robert Foss <rfoss@kernel.org>
To: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
To: Jonas Karlman <jonas@kwiboo.se>
To: Jernej Skrabec <jernej.skrabec@gmail.com>
To: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
To: Maxime Ripard <mripard@kernel.org>
To: Thomas Zimmermann <tzimmermann@suse.de>
To: David Airlie <airlied@gmail.com>
To: Simona Vetter <simona@ffwll.ch>
To: Linus Walleij <linusw@kernel.org>
To: Frieder Schrempf <frieder.schrempf@kontron.de>
To: Marek Vasut <marex@denx.de>
Cc: dri-devel@lists.freedesktop.org
Cc: linux-kernel@vger.kernel.org

---
Esben Haabendal (3):
      drm/bridge: ti-sn65dsi83: Fix error handling in sn65dsi83_reset_work()
      drm/bridge: ti-sn65dsi83: Propagate error in sn65dsi83_reset_pipe()
      drm/bridge: ti-sn65dsi83: Support LVDS Channel B on SN65DSI84

 drivers/gpu/drm/bridge/ti-sn65dsi83.c | 41 ++++++++++++++++++++++-------------
 1 file changed, 26 insertions(+), 15 deletions(-)
---
base-commit: 075b74841bd0065a3bda3440873c747938e69b68
change-id: 20260710-ti-sn65dsi83-fixes-1d08e0ac67b3

Best regards,
--  
Esben Haabendal <esben@geanix.com>

[PATCH v4 1/3] drm/bridge: ti-sn65dsi83: Fix error handling in sn65dsi83_reset_work()

[Esben Haabendal]

The error handling of sn65dsi83_reset_pipe() failure in
sn65dsi83_reset_work() has been a bit strange all the time, missing both
enable_irq() and drm_bridge_exit() in case of failure. But since
sn65dsi83_reset_pipe() have never been failing (always returning 0), it has
not caused any problems.

As we are going to change sn65dsi83_reset_pipe() to return error values in
case of failure, we need to fix this.

Fixes: ad5c6ecef27e ("drm: bridge: ti-sn65dsi83: Add error recovery mechanism")
Cc: stable@vger.kernel.org
Signed-off-by: Esben Haabendal <esben@geanix.com>
---
 drivers/gpu/drm/bridge/ti-sn65dsi83.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
index 42b451432bbb..0ccfff401b2a 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
@@ -417,10 +417,9 @@ static void sn65dsi83_reset_work(struct work_struct *ws)
 
 	/* Reset the pipe */
 	ret = sn65dsi83_reset_pipe(ctx);
-	if (ret) {
+	if (ret)
 		dev_err(ctx->dev, "reset pipe failed %pe\n", ERR_PTR(ret));
-		return;
-	}
+
 	if (ctx->irq)
 		enable_irq(ctx->irq);
 

-- 
2.55.0

[PATCH v4 2/3] drm/bridge: ti-sn65dsi83: Propagate error in sn65dsi83_reset_pipe()

[Esben Haabendal]

Propagate the error code from drm_bridge_helper_reset_crtc(), so that
callers has a chance to handle the failure.

Fixes: ad5c6ecef27e ("drm: bridge: ti-sn65dsi83: Add error recovery mechanism")
Cc: stable@vger.kernel.org
Reviewed-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
Tested-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
Signed-off-by: Esben Haabendal <esben@geanix.com>
---
 drivers/gpu/drm/bridge/ti-sn65dsi83.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
index 0ccfff401b2a..475224c5a5d8 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
@@ -403,7 +403,7 @@ static int sn65dsi83_reset_pipe(struct sn65dsi83 *sn65dsi83)
 	drm_modeset_drop_locks(&ctx);
 	drm_modeset_acquire_fini(&ctx);
 
-	return 0;
+	return err;
 }
 
 static void sn65dsi83_reset_work(struct work_struct *ws)

-- 
2.55.0

[PATCH v4 3/3] drm/bridge: ti-sn65dsi83: Support LVDS Channel B on SN65DSI84

[Esben Haabendal]

This adds support for using SN65DSI84 in single-link mode with output to
LVDS Channel B.

Reviewed-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
Tested-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
Signed-off-by: Esben Haabendal <esben@geanix.com>
---
 drivers/gpu/drm/bridge/ti-sn65dsi83.c | 34 +++++++++++++++++++++++-----------
 1 file changed, 23 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
index 475224c5a5d8..33cf33d127c6 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
@@ -11,6 +11,8 @@
  *   = 1x Single-link DSI ~ 2x Single-link or 1x Dual-link LVDS
  *   - Supported
  *   - Dual-link LVDS mode tested
+ *   - Single-link to LVDS Channel A tested
+ *   - Single-link to LVDS Channel B tested
  *   - 2x Single-link LVDS mode unsupported
  *     (should be easy to add by someone who has the HW)
  * - SN65DSI85
@@ -162,7 +164,7 @@ struct sn65dsi83 {
 	struct gpio_desc		*enable_gpio;
 	struct regulator		*vcc;
 	bool				lvds_dual_link;
-	bool				lvds_dual_link_even_odd_swap;
+	bool				lvds_channel_swap;
 	int				lvds_vod_swing_conf[2];
 	int				lvds_term_conf[2];
 	int				irq;
@@ -641,7 +643,7 @@ static void sn65dsi83_atomic_pre_enable(struct drm_bridge *bridge,
 			REG_LVDS_VCOM_CHA_LVDS_VOD_SWING(ctx->lvds_vod_swing_conf[CHANNEL_A]) |
 			REG_LVDS_VCOM_CHB_LVDS_VOD_SWING(ctx->lvds_vod_swing_conf[CHANNEL_B]));
 	regmap_write(ctx->regmap, REG_LVDS_LANE,
-		     (ctx->lvds_dual_link_even_odd_swap ?
+		     (ctx->lvds_channel_swap ?
 		      REG_LVDS_LANE_EVEN_ODD_SWAP : 0) |
 		     (ctx->lvds_term_conf[CHANNEL_A] ?
 			  REG_LVDS_LANE_CHA_LVDS_TERM : 0) |
@@ -892,6 +894,7 @@ static int sn65dsi83_parse_dt(struct sn65dsi83 *ctx, enum sn65dsi83_model model)
 {
 	struct drm_bridge *panel_bridge;
 	struct device *dev = ctx->dev;
+	u32 output_port = 2;
 	int ret;
 
 	ret = sn65dsi83_parse_lvds_endpoint(ctx, CHANNEL_A);
@@ -903,29 +906,38 @@ static int sn65dsi83_parse_dt(struct sn65dsi83 *ctx, enum sn65dsi83_model model)
 		return ret;
 
 	ctx->lvds_dual_link = false;
-	ctx->lvds_dual_link_even_odd_swap = false;
+	ctx->lvds_channel_swap = false;
 	if (model != MODEL_SN65DSI83) {
-		struct device_node *port2, *port3;
+		struct device_node *port0, *port1, *port2, *port3;
 		int dual_link;
 
+		port0 = of_graph_get_port_by_id(dev->of_node, 0);
+		port1 = of_graph_get_port_by_id(dev->of_node, 1);
 		port2 = of_graph_get_port_by_id(dev->of_node, 2);
 		port3 = of_graph_get_port_by_id(dev->of_node, 3);
 		dual_link = drm_of_lvds_get_dual_link_pixel_order(port2, port3);
-		of_node_put(port2);
-		of_node_put(port3);
 
 		if (dual_link == DRM_LVDS_DUAL_LINK_ODD_EVEN_PIXELS) {
-			ctx->lvds_dual_link = true;
 			/* Odd pixels to LVDS Channel A, even pixels to B */
-			ctx->lvds_dual_link_even_odd_swap = false;
-		} else if (dual_link == DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS) {
 			ctx->lvds_dual_link = true;
+		} else if (dual_link == DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS) {
 			/* Even pixels to LVDS Channel A, odd pixels to B */
-			ctx->lvds_dual_link_even_odd_swap = true;
+			ctx->lvds_dual_link = true;
+			ctx->lvds_channel_swap = true;
+		} else if (port0 && !port1 && port2 && !port3) {
+			/* DSI Channel A to LVDS Channel A */
+		} else if (port0 && !port1 && !port2 && port3) {
+			/* DSI Channel A to LVDS Channel B */
+			ctx->lvds_channel_swap = true;
+			output_port = 3;
 		}
+		of_node_put(port0);
+		of_node_put(port1);
+		of_node_put(port2);
+		of_node_put(port3);
 	}
 
-	panel_bridge = devm_drm_of_get_bridge(dev, dev->of_node, 2, 0);
+	panel_bridge = devm_drm_of_get_bridge(dev, dev->of_node, output_port, 0);
 	if (IS_ERR(panel_bridge))
 		return dev_err_probe(dev, PTR_ERR(panel_bridge), "Failed to get panel bridge\n");
 

-- 
2.55.0

Re: [PATCH v4 1/3] drm/bridge: ti-sn65dsi83: Fix error handling in sn65dsi83_reset_work()

[sashiko-bot]

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

New issues:
- [High] Unconditionally re-enabling the interrupt after a failed hardware reset introduces a severe interrupt and workqueue storm.

Pre-existing issues:
- [Critical] Missing workqueue cancellation in the driver remove path leads to a Use-After-Free of the driver context.
--

--- Patch [1]: [PATCH v4 1/3] drm/bridge: ti-sn65dsi83: Fix error handling in sn65dsi83_reset_work() ---
commit 9e0c74443fddb87d6c513a0e21cb5c153b2b2c3a
Author: Esben Haabendal <esben@geanix.com>

drm/bridge: ti-sn65dsi83: Fix error handling in sn65dsi83_reset_work()

This commit changes the error handling in sn65dsi83_reset_work() so that
enable_irq() and drm_bridge_exit() are called even if sn65dsi83_reset_pipe()
fails.

> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> index 42b451432bbb6..0ccfff401b2a6 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> @@ -417,10 +417,9 @@ static void sn65dsi83_reset_work(struct work_struct *ws)
>  
>  	/* Reset the pipe */
>  	ret = sn65dsi83_reset_pipe(ctx);
> -	if (ret) {
> +	if (ret)
>  		dev_err(ctx->dev, "reset pipe failed %pe\n", ERR_PTR(ret));
> -		return;
> -	}
> +
>  	if (ctx->irq)
>  		enable_irq(ctx->irq);

[Severity: High]
Does unconditionally re-enabling the interrupt after a failed hardware reset
introduce an interrupt and workqueue storm regression?

If sn65dsi83_reset_pipe() fails, the hardware IRQ status register
(REG_IRQ_STAT) remains uncleared because the reset sequence did not
successfully reach sn65dsi83_atomic_enable().

Since enable_irq(ctx->irq) is now called in sn65dsi83_reset_work() even on
failure, will the hardware immediately trigger the interrupt again? This would
cause the interrupt handler to run, disable the IRQ, and schedule reset_work.
The work runs, fails again, and re-enables the IRQ, creating an infinite loop
that could lock up a CPU core.

[Severity: Critical]
This is a pre-existing issue, but does the driver missing workqueue
cancellation in the remove path lead to a use-after-free of the driver context?

The driver context ctx is dynamically allocated via devm_drm_bridge_alloc() and
is automatically freed by devres after sn65dsi83_remove() returns:

drivers/gpu/drm/bridge/ti-sn65dsi83.c:sn65dsi83_remove() {
	struct sn65dsi83 *ctx = i2c_get_clientdata(client);

	drm_bridge_unplug(&ctx->bridge);
}

The teardown path never calls cancel_work_sync(&ctx->reset_work). If an
interrupt schedules the work just before removal, and it hasn't completed when
remove() returns, the scheduled work will run after devres frees ctx.

The work function sn65dsi83_reset_work() then dereferences the freed ctx:

drivers/gpu/drm/bridge/ti-sn65dsi83.c:sn65dsi83_reset_work() {
	struct sn65dsi83 *ctx = container_of(ws, struct sn65dsi83, reset_work);
	...
	if (!drm_bridge_enter(&ctx->bridge, &idx))
		return;
	...
}

Should cancel_work_sync() be added to sn65dsi83_remove() to prevent this?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260810-ti-sn65dsi83-fixes-v4-0-ae9c95c8d162@geanix.com?part=1

Re: [PATCH v4 2/3] drm/bridge: ti-sn65dsi83: Propagate error in sn65dsi83_reset_pipe()

[sashiko-bot]

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

Pre-existing issues:
- [High] The background work `reset_work` is not cancelled when the device is unbound, leading to a Use-After-Free when the workqueue executes the pending work item after the driver data structure has been freed.
- [High] Re-enabling the interrupt after a failed bridge reset creates an interrupt storm and infinite workqueue loop.
- [High] Preexisting double-unlock of connection_mutex in drm_bridge_helper_reset_crtc() triggered during pipeline reset.
--

--- Patch [2]: [PATCH v4 2/3] drm/bridge: ti-sn65dsi83: Propagate error in sn65dsi83_reset_pipe() ---
commit a871b12bfaa133da03afd275986f3c4abe9be9c0
Author: Esben Haabendal <esben@geanix.com>
drm/bridge: ti-sn65dsi83: Propagate error in sn65dsi83_reset_pipe()

This commit modifies sn65dsi83_reset_pipe() to propagate the error code from
drm_bridge_helper_reset_crtc() instead of unconditionally returning 0. This
gives callers the opportunity to handle reset failures.

> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> index 0ccfff401b2a6..475224c5a5d83 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> @@ -403,7 +403,7 @@ static int sn65dsi83_reset_pipe(struct sn65dsi83 *sn65dsi83)
>  	drm_modeset_drop_locks(&ctx);
>  	drm_modeset_acquire_fini(&ctx);
>  
> -	return 0;
> +	return err;

[Severity: High]
This is a pre-existing issue, but returning the error here exposes a
problem in drm_bridge_helper_reset_crtc() which is called earlier in
this function.

Does drm_bridge_helper_reset_crtc() suffer from a double-unlock of
connection_mutex?

It manually acquires connection_mutex, then calls:
drm_atomic_get_connector_for_encoder(encoder, ctx)

This helper appears to unconditionally unlock connection_mutex before returning:

drivers/gpu/drm/drm_atomic.c:drm_atomic_get_connector_for_encoder() {
    ...
    ret = drm_modeset_lock(&dev->mode_config.connection_mutex, ctx);
    ...
    drm_modeset_unlock(&dev->mode_config.connection_mutex);
    return out_connector;
}

Then drm_bridge_helper_reset_crtc() explicitly drops it again:

drivers/gpu/drm/drm_bridge_helper.c:drm_bridge_helper_reset_crtc() {
    ...
    connector = drm_atomic_get_connector_for_encoder(encoder, ctx);
    ...
out:
    drm_modeset_unlock(&dev->mode_config.connection_mutex);
    return ret;
}

Could this lead to lock state corruption or kernel panics when the reset path
is executed?

>  }
>  
>  static void sn65dsi83_reset_work(struct work_struct *ws)

[Severity: High]
This isn't a bug introduced by this patch, but now that
sn65dsi83_reset_pipe() propagates errors, will this workqueue re-enable the
interrupt even if the reset failed?

If sn65dsi83_reset_pipe() returns an error, the hardware might not be reset
and the error condition could remain asserted. If we unconditionally call
enable_irq(ctx->irq) at the end of sn65dsi83_reset_work():

drivers/gpu/drm/bridge/ti-sn65dsi83.c:sn65dsi83_reset_work() {
    ...
    ret = sn65dsi83_reset_pipe(ctx);
    if (ret)
        dev_err(ctx->dev, "reset pipe failed %pe\n", ERR_PTR(ret));

    if (ctx->irq)
        enable_irq(ctx->irq);
}

Will the interrupt immediately fire again, causing an interrupt storm and an
infinite workqueue loop?

[Severity: High]
This is another pre-existing issue, but there appears to be a problem with
reset_work during driver removal.

Does sn65dsi83_remove() fail to cancel reset_work before the device is
unbound?

drivers/gpu/drm/bridge/ti-sn65dsi83.c:sn65dsi83_remove() {
    struct sn65dsi83 *ctx = i2c_get_clientdata(client);

    drm_bridge_unplug(&ctx->bridge);
}

If an error interrupt fires and schedules reset_work, and the driver is
unbound before the work executes, will devres free the sn65dsi83 structure
while the workqueue still holds a dangling pointer to it, leading to a
use-after-free?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260810-ti-sn65dsi83-fixes-v4-0-ae9c95c8d162@geanix.com?part=2