Re: [PATCH v2 2/2] drm/logicvc: Avoid using DRM resources after device is unplugged

[Maxime Ripard]

On Tue, Jun 30, 2026 at 11:10:11AM +0200, Romain Gantois wrote:
> Some DRM resources such as plane, CRTC or encoder objects could remain in
> use after the DRM device is removed. Use the drm_dev_enter/exit() mechanism
> to ensure that the DRM device is not unplugged before using its resources.
> 
> Fixes: efeeaefe9be56 ("drm: Add support for the LogiCVC display controller")                                                                        │
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Romain Gantois <romain.gantois@xxxxxxxxxxx>
> ---
>  drivers/gpu/drm/logicvc/logicvc_crtc.c      | 35 ++++++++++++++++-----
>  drivers/gpu/drm/logicvc/logicvc_drm.c       |  9 +++++-
>  drivers/gpu/drm/logicvc/logicvc_interface.c | 12 ++++++++
>  drivers/gpu/drm/logicvc/logicvc_layer.c     | 48 ++++++++++++++++++++---------
>  4 files changed, 81 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/gpu/drm/logicvc/logicvc_crtc.c b/drivers/gpu/drm/logicvc/logicvc_crtc.c
> index 3a4c347eaa648..f3a224a883b2f 100644
> --- a/drivers/gpu/drm/logicvc/logicvc_crtc.c
> +++ b/drivers/gpu/drm/logicvc/logicvc_crtc.c
> @@ -40,10 +40,15 @@ static void logicvc_crtc_atomic_begin(struct drm_crtc *drm_crtc,
>  				      struct drm_atomic_state *state)
>  {
>  	struct logicvc_crtc *crtc = logicvc_crtc(drm_crtc);
> -	struct drm_crtc_state *old_state =
> -		drm_atomic_get_old_crtc_state(state, drm_crtc);
>  	struct drm_device *drm_dev = drm_crtc->dev;
> +	struct drm_crtc_state *old_state;
>  	unsigned long flags;
> +	int idx;
> +
> +	if (!drm_dev_enter(drm_dev, &idx))
> +		return;
> +
> +	old_state = drm_atomic_get_old_crtc_state(state, drm_crtc);

You don't have to move the state around here, the states are always safe
to access in the atomic callbacks.

>  	/*
>  	 * We need to grab the pending event here if vblank was already enabled
> @@ -58,6 +63,8 @@ static void logicvc_crtc_atomic_begin(struct drm_crtc *drm_crtc,
>  
>  		spin_unlock_irqrestore(&drm_dev->event_lock, flags);
>  	}
> +
> +	drm_dev_exit(idx);

Only the device resources (ie, clocks, registers, etc. ) need to be
guarded. Any DRM facing resource can still used, and the vblank here
should still be signalled.

>  }
>  
>  static void logicvc_crtc_atomic_enable(struct drm_crtc *drm_crtc,
> @@ -65,17 +72,23 @@ static void logicvc_crtc_atomic_enable(struct drm_crtc *drm_crtc,
>  {
>  	struct logicvc_crtc *crtc = logicvc_crtc(drm_crtc);
>  	struct logicvc_drm *logicvc = logicvc_drm(drm_crtc->dev);
> -	struct drm_crtc_state *old_state =
> -		drm_atomic_get_old_crtc_state(state, drm_crtc);
> -	struct drm_crtc_state *new_state =
> -		drm_atomic_get_new_crtc_state(state, drm_crtc);
> -	struct drm_display_mode *mode = &new_state->adjusted_mode;
>  
>  	struct drm_device *drm_dev = drm_crtc->dev;
> +	struct drm_crtc_state *old_state;
> +	struct drm_crtc_state *new_state;
>  	unsigned int hact, hfp, hsl, hbp;
>  	unsigned int vact, vfp, vsl, vbp;
> +	struct drm_display_mode *mode;
>  	unsigned long flags;
>  	u32 ctrl;
> +	int idx;
> +
> +	if (!drm_dev_enter(drm_dev, &idx))
> +		return;
> +
> +	old_state = drm_atomic_get_old_crtc_state(state, drm_crtc);
> +	new_state = drm_atomic_get_new_crtc_state(state, drm_crtc);
> +	mode = &new_state->adjusted_mode;
>  
>  	/* Timings */
>  
> @@ -148,6 +161,8 @@ static void logicvc_crtc_atomic_enable(struct drm_crtc *drm_crtc,
>  		drm_crtc->state->event = NULL;
>  		spin_unlock_irqrestore(&drm_dev->event_lock, flags);
>  	}
> +
> +	drm_dev_exit(idx);
>  }
>  
>  static void logicvc_crtc_atomic_disable(struct drm_crtc *drm_crtc,
> @@ -155,6 +170,10 @@ static void logicvc_crtc_atomic_disable(struct drm_crtc *drm_crtc,
>  {
>  	struct logicvc_drm *logicvc = logicvc_drm(drm_crtc->dev);
>  	struct drm_device *drm_dev = drm_crtc->dev;
> +	int idx;
> +
> +	if (!drm_dev_enter(drm_dev, &idx))
> +		return;
>  
>  	drm_crtc_vblank_off(drm_crtc);
>  
> @@ -180,6 +199,8 @@ static void logicvc_crtc_atomic_disable(struct drm_crtc *drm_crtc,
>  		drm_crtc->state->event = NULL;
>  		spin_unlock_irq(&drm_dev->event_lock);
>  	}
> +
> +	drm_dev_exit(idx);
>  }
>  
>  static const struct drm_crtc_helper_funcs logicvc_crtc_helper_funcs = {
> diff --git a/drivers/gpu/drm/logicvc/logicvc_drm.c b/drivers/gpu/drm/logicvc/logicvc_drm.c
> index bbebf4fc7f51a..2112646386e36 100644
> --- a/drivers/gpu/drm/logicvc/logicvc_drm.c
> +++ b/drivers/gpu/drm/logicvc/logicvc_drm.c
> @@ -71,6 +71,7 @@ static irqreturn_t logicvc_drm_irq_handler(int irq, void *data)
>  	struct logicvc_drm *logicvc = data;
>  	irqreturn_t ret = IRQ_NONE;
>  	u32 stat = 0;
> +	int idx;
>  
>  	/* Get pending interrupt sources. */
>  	regmap_read(logicvc->regmap, LOGICVC_INT_STAT_REG, &stat);

strictly speaking, that regmap read here should be protected, but it's
not going to be executed after remove anyway because the interrupt
handler will be deregistered, so you don't have to bother here.

[...]

>  static void logicvc_plane_atomic_disable(struct drm_plane *drm_plane,
> @@ -239,6 +255,8 @@ static void logicvc_plane_atomic_disable(struct drm_plane *drm_plane,
>  	struct logicvc_drm *logicvc = logicvc_drm(drm_plane->dev);
>  	u32 index = layer->index;
>  
> +	/* No need for drm_dev_enter() here. The regmap outlives the DRM device. */
> +
>  	regmap_write(logicvc->regmap, LOGICVC_LAYER_CTRL_REG(index), 0);

No it doesn't? It's a devm allocated regmap, it's going to be destroyed
at remove. The DRM device will stick around after remove for as long as
there's a userspace application with a fd to it, so the DRM device far
outlives the regmap (and you end up with a dangling pointer to the regmap)

Maxime

Attachment: signature.asc
Description: PGP signature