On Fri, Aug 08, 2025 at 03:24:29PM +0200, Luca Ceresoli wrote:
> To support hot-unplug of this bridge we need to protect access to device
> resources in case sn65dsi83_remove() happens concurrently to other code.
>
> Some care is needed for the case when the unplug happens before
> sn65dsi83_atomic_disable() has a chance to enter the critical section
> (i.e. a successful drm_bridge_enter() call), which occurs whenever the
> hardware is removed while the display is active. When that happens,
> sn65dsi83_atomic_disable() in unable to release some resources, thus this
> needs to be done in sn65dsi83_remove() after drm_bridge_unplug().
>
> Signed-off-by: Luca Ceresoli <luca.ceres...@bootlin.com>
> ---
> drivers/gpu/drm/bridge/ti-sn65dsi83.c | 53
> +++++++++++++++++++++++++++++++++++
> 1 file changed, 53 insertions(+)
>
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> index
> 033c44326552ab167d4e8d9b74957c585e4c6fb7..9e4cecf4f7cb056f0c34e87007fcebf50780e49c
> 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> @@ -157,6 +157,7 @@ struct sn65dsi83 {
> struct drm_bridge *panel_bridge;
> struct gpio_desc *enable_gpio;
> struct regulator *vcc;
> + bool disable_resources_needed;
> bool lvds_dual_link;
> bool lvds_dual_link_even_odd_swap;
> int lvds_vod_swing_conf[2];
> @@ -406,6 +407,10 @@ static void sn65dsi83_reset_work(struct work_struct *ws)
> {
> struct sn65dsi83 *ctx = container_of(ws, struct sn65dsi83, reset_work);
> int ret;
> + int idx;
> +
> + if (!drm_bridge_enter(&ctx->bridge, &idx))
> + return;
>
> /* Reset the pipe */
> ret = sn65dsi83_reset_pipe(ctx);
> @@ -415,12 +420,18 @@ static void sn65dsi83_reset_work(struct work_struct *ws)
> }
> if (ctx->irq)
> enable_irq(ctx->irq);
> +
> + drm_bridge_exit(idx);
> }
>
> static void sn65dsi83_handle_errors(struct sn65dsi83 *ctx)
> {
> unsigned int irq_stat;
> int ret;
> + int idx;
> +
> + if (!drm_bridge_enter(&ctx->bridge, &idx))
> + return;
>
> /*
> * Schedule a reset in case of:
> @@ -441,6 +452,8 @@ static void sn65dsi83_handle_errors(struct sn65dsi83 *ctx)
>
> schedule_work(&ctx->reset_work);
> }
> +
> + drm_bridge_exit(idx);
> }
>
> static void sn65dsi83_monitor_work(struct work_struct *work)
> @@ -478,10 +491,15 @@ static void sn65dsi83_atomic_pre_enable(struct
> drm_bridge *bridge,
> __le16 le16val;
> u16 val;
> int ret;
> + int idx;
> +
> + if (!drm_bridge_enter(bridge, &idx))
> + return;
>
> ret = regulator_enable(ctx->vcc);
> if (ret) {
> dev_err(ctx->dev, "Failed to enable vcc: %d\n", ret);
> + drm_bridge_exit(idx);
> return;
> }
>
> @@ -625,6 +643,8 @@ static void sn65dsi83_atomic_pre_enable(struct drm_bridge
> *bridge,
> dev_err(ctx->dev, "failed to lock PLL, ret=%i\n", ret);
> /* On failure, disable PLL again and exit. */
> regmap_write(ctx->regmap, REG_RC_PLL_EN, 0x00);
> + ctx->disable_resources_needed = true;
> + drm_bridge_exit(idx);
> return;
> }
>
> @@ -633,6 +653,9 @@ static void sn65dsi83_atomic_pre_enable(struct drm_bridge
> *bridge,
>
> /* Wait for 10ms after soft reset as specified in datasheet */
> usleep_range(10000, 12000);
> +
> + ctx->disable_resources_needed = true;
> + drm_bridge_exit(idx);
> }
>
> static void sn65dsi83_atomic_enable(struct drm_bridge *bridge,
> @@ -640,6 +663,10 @@ static void sn65dsi83_atomic_enable(struct drm_bridge
> *bridge,
> {
> struct sn65dsi83 *ctx = bridge_to_sn65dsi83(bridge);
> unsigned int pval;
> + int idx;
> +
> + if (!drm_bridge_enter(bridge, &idx))
> + return;
>
> /* Clear all errors that got asserted during initialization. */
> regmap_read(ctx->regmap, REG_IRQ_STAT, &pval);
> @@ -659,6 +686,8 @@ static void sn65dsi83_atomic_enable(struct drm_bridge
> *bridge,
> /* Use the polling task */
> sn65dsi83_monitor_start(ctx);
> }
> +
> + drm_bridge_exit(idx);
> }
>
> static void sn65dsi83_atomic_disable(struct drm_bridge *bridge,
> @@ -666,6 +695,10 @@ static void sn65dsi83_atomic_disable(struct drm_bridge
> *bridge,
> {
> struct sn65dsi83 *ctx = bridge_to_sn65dsi83(bridge);
> int ret;
> + int idx;
> +
> + if (!drm_bridge_enter(bridge, &idx))
> + return;
>
> if (ctx->irq) {
> /* Disable irq */
> @@ -685,6 +718,9 @@ static void sn65dsi83_atomic_disable(struct drm_bridge
> *bridge,
> dev_err(ctx->dev, "Failed to disable vcc: %d\n", ret);
>
> regcache_mark_dirty(ctx->regmap);
> +
> + ctx->disable_resources_needed = false;
> + drm_bridge_exit(idx);
> }
>
> static enum drm_mode_status
> @@ -1005,7 +1041,24 @@ static void sn65dsi83_remove(struct i2c_client *client)
> {
> struct sn65dsi83 *ctx = i2c_get_clientdata(client);
>
> + drm_bridge_unplug(&ctx->bridge);
> drm_bridge_remove(&ctx->bridge);
Shouldn't we merge drm_bridge_unplug with the release part of
devm_drm_bridge_alloc?
> +
> + /*
> + * sn65dsi83_atomic_disable() should release some resources, but it
> + * cannot if we call drm_bridge_unplug() before it can
> + * drm_bridge_enter(). If that happens, let's release those
> + * resources now.
> + */
> + if (ctx->disable_resources_needed) {
> + if (!ctx->irq)
> + sn65dsi83_monitor_stop(ctx);
> +
> + gpiod_set_value_cansleep(ctx->enable_gpio, 0);
> + usleep_range(10000, 11000);
> +
> + regulator_disable(ctx->vcc);
> + }
I'm not sure you need this. Wouldn't registering a devm action do the
same thing?
Maxime
signature.asc
Description: PGP signature
