[v1] staging: fbtft: make dirty_lock IRQ-safe

[PATCH] staging: fbtft: make dirty_lock IRQ-safe

Posted by sh_def@163.com 1 month, 4 weeks ago

From: Hui Su <sh_def@163.com>

fbtft_mkdirty() can be reached from the fbcon rendering path while
processing printk() in hardirq context. Meanwhile, dirty_lock is also
taken by fbtft_deferred_io() in workqueue context with local interrupts
enabled.

Lockdep reports a possible IRQ lock inversion involving dirty_lock and
console_owner. A hardirq can interrupt a CPU holding dirty_lock and
enter the console rendering path, which can attempt to acquire
dirty_lock again.

The following lockdep report was observed on an RK3566 system with
CONFIG_PROVE_LOCKING enabled:

  WARNING: possible irq lock inversion dependency detected
  swapper/2/0 just changed the state of lock:
  (console_owner){-...}-{0:0}
  but this lock took another, HARDIRQ-unsafe lock in the past:
  (&par->dirty_lock){+.+.}-{2:2}

  CPU0                    CPU1
  ----                    ----
  lock(&par->dirty_lock);
                         local_irq_disable();
                         lock(console_owner);
                         lock(&par->dirty_lock);
  <Interrupt>
    lock(console_owner);

  *** DEADLOCK ***

Use spin_lock_irqsave() for both dirty_lock critical sections. They
only access the dirty line range, so the IRQ-off regions remain short.

Fixes: c296d5f9957c ("staging: fbtft: core support")
Signed-off-by: Hui Su <sh_def@163.com>
---
 drivers/staging/fbtft/fbtft-core.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/fbtft/fbtft-core.c b/drivers/staging/fbtft/fbtft-core.c
index ca0c38221c16..193643d0329d 100644
--- a/drivers/staging/fbtft/fbtft-core.c
+++ b/drivers/staging/fbtft/fbtft-core.c
@@ -298,14 +298,20 @@ static void fbtft_mkdirty(struct fb_info *info, int y, int height)
 {
 	struct fbtft_par *par = info->par;
 	struct fb_deferred_io *fbdefio = info->fbdefio;
+	unsigned long flags;
 
 	/* Mark display lines/area as dirty */
-	spin_lock(&par->dirty_lock);
+	/*
+	 * fbcon takes dirty_lock while holding console_owner. Disable local
+	 * interrupts here so a printk hardirq cannot acquire console_owner
+	 * while dirty_lock is held and create the inverse lock ordering.
+	 */
+	spin_lock_irqsave(&par->dirty_lock, flags);
 	if (y < par->dirty_lines_start)
 		par->dirty_lines_start = y;
 	if (y + height - 1 > par->dirty_lines_end)
 		par->dirty_lines_end = y + height - 1;
-	spin_unlock(&par->dirty_lock);
+	spin_unlock_irqrestore(&par->dirty_lock, flags);
 
 	/* Schedule deferred_io to update display (no-op if already on queue)*/
 	schedule_delayed_work(&info->deferred_work, fbdefio->delay);
@@ -317,14 +323,15 @@ static void fbtft_deferred_io(struct fb_info *info, struct list_head *pagereflis
 	unsigned int dirty_lines_start, dirty_lines_end;
 	struct fb_deferred_io_pageref *pageref;
 	unsigned int y_low = 0, y_high = 0;
+	unsigned long flags;
 
-	spin_lock(&par->dirty_lock);
+	spin_lock_irqsave(&par->dirty_lock, flags);
 	dirty_lines_start = par->dirty_lines_start;
 	dirty_lines_end = par->dirty_lines_end;
 	/* set display line markers as clean */
 	par->dirty_lines_start = par->info->var.yres - 1;
 	par->dirty_lines_end = 0;
-	spin_unlock(&par->dirty_lock);
+	spin_unlock_irqrestore(&par->dirty_lock, flags);
 
 	/* Mark display lines as dirty */
 	list_for_each_entry(pageref, pagereflist, list) {
-- 
2.43.0

Re: [PATCH] staging: fbtft: make dirty_lock IRQ-safe

Posted by Nam Cao 1 month, 3 weeks ago

sh_def@163.com writes:
> diff --git a/drivers/staging/fbtft/fbtft-core.c b/drivers/staging/fbtft/fbtft-core.c
> index ca0c38221c16..193643d0329d 100644
> --- a/drivers/staging/fbtft/fbtft-core.c
> +++ b/drivers/staging/fbtft/fbtft-core.c
> @@ -298,14 +298,20 @@ static void fbtft_mkdirty(struct fb_info *info, int y, int height)
>  {
>  	struct fbtft_par *par = info->par;
>  	struct fb_deferred_io *fbdefio = info->fbdefio;
> +	unsigned long flags;
>  
>  	/* Mark display lines/area as dirty */
> -	spin_lock(&par->dirty_lock);
> +	/*
> +	 * fbcon takes dirty_lock while holding console_owner. Disable local
> +	 * interrupts here so a printk hardirq cannot acquire console_owner
> +	 * while dirty_lock is held and create the inverse lock ordering.
> +	 */

Beside that reason, we also need spin_lock_irqsave() because
fbtft_mkdirty() can be called in both task context and hardirq
context. And this reason alone suffices and usually is why
spin_lock_irqsave() is used, so I think a comment is not necessary.

But I am fine with it either way.

> +	spin_lock_irqsave(&par->dirty_lock, flags);
>  	if (y < par->dirty_lines_start)
>  		par->dirty_lines_start = y;
>  	if (y + height - 1 > par->dirty_lines_end)
>  		par->dirty_lines_end = y + height - 1;
> -	spin_unlock(&par->dirty_lock);
> +	spin_unlock_irqrestore(&par->dirty_lock, flags);
>  
>  	/* Schedule deferred_io to update display (no-op if already on queue)*/
>  	schedule_delayed_work(&info->deferred_work, fbdefio->delay);
> @@ -317,14 +323,15 @@ static void fbtft_deferred_io(struct fb_info *info, struct list_head *pagereflis
>  	unsigned int dirty_lines_start, dirty_lines_end;
>  	struct fb_deferred_io_pageref *pageref;
>  	unsigned int y_low = 0, y_high = 0;
> +	unsigned long flags;
>  
> -	spin_lock(&par->dirty_lock);
> +	spin_lock_irqsave(&par->dirty_lock, flags);
>  	dirty_lines_start = par->dirty_lines_start;
>  	dirty_lines_end = par->dirty_lines_end;
>  	/* set display line markers as clean */
>  	par->dirty_lines_start = par->info->var.yres - 1;
>  	par->dirty_lines_end = 0;
> -	spin_unlock(&par->dirty_lock);
> +	spin_unlock_irqrestore(&par->dirty_lock, flags);

fbtft_deferred_io() is executed in workqueue with interrupt enabled. So
it can use spin_lock_irq() instead of spin_lock_irqsave(), right?

Nam

Re: [PATCH] staging: fbtft: make dirty_lock IRQ-safe

Posted by Hui Su 1 month, 3 weeks ago

From: sh_def <sh_def@163.com>

> sh_def@163.com writes:
> > diff --git a/drivers/staging/fbtft/fbtft-core.c b/drivers/staging/fbtft/fbtft-core.c
> > index ca0c38221c16..193643d0329d 100644
> > --- a/drivers/staging/fbtft/fbtft-core.c
> > +++ b/drivers/staging/fbtft/fbtft-core.c
> > @@ -298,14 +298,20 @@ static void fbtft_mkdirty(struct fb_info *info, int y, int height)
> >  {
> >      struct fbtft_par *par = info->par;
> >      struct fb_deferred_io *fbdefio = info->fbdefio;
> > +    unsigned long flags;
> >
> >      /* Mark display lines/area as dirty */
> > -    spin_lock(&par->dirty_lock);
> > +    /*
> > +     * fbcon takes dirty_lock while holding console_owner. Disable local
> > +     * interrupts here so a printk hardirq cannot acquire console_owner
> > +     * while dirty_lock is held and create the inverse lock ordering.
> > +     */
>
> Beside that reason, we also need spin_lock_irqsave() because
> fbtft_mkdirty() can be called in both task context and hardirq
> context. And this reason alone suffices and usually is why
> spin_lock_irqsave() is used, so I think a comment is not necessary.
>

Thanks.
I will drop it in v2.

> But I am fine with it either way.
>
> >      spin_lock_irqsave(&par->dirty_lock, flags);
> >      if (y < par->dirty_lines_start)
> >          par->dirty_lines_start = y;
> >      if (y + height - 1 > par->dirty_lines_end)
> >          par->dirty_lines_end = y + height - 1;
> > -    spin_unlock(&par->dirty_lock);
> > +    spin_unlock_irqrestore(&par->dirty_lock, flags);
> >
> >      /* Schedule deferred_io to update display (no-op if already on queue)*/
> >      schedule_delayed_work(&info->deferred_work, fbdefio->delay);
> > @@ -317,14 +323,15 @@ static void fbtft_deferred_io(struct fb_info *info, struct list_head *pagereflis
> >      unsigned int dirty_lines_start, dirty_lines_end;
> >      struct fb_deferred_io_pageref *pageref;
> >      unsigned int y_low = 0, y_high = 0;
> > +    unsigned long flags;
> >
> > -    spin_lock(&par->dirty_lock);
> > +    spin_lock_irqsave(&par->dirty_lock, flags);
> >      dirty_lines_start = par->dirty_lines_start;
> >      dirty_lines_end = par->dirty_lines_end;
> >      /* set display line markers as clean */
> >      par->dirty_lines_start = par->info->var.yres - 1;
> >      par->dirty_lines_end = 0;
> > -    spin_unlock(&par->dirty_lock);
> > +    spin_unlock_irqrestore(&par->dirty_lock, flags);
>
> fbtft_deferred_io() is executed in workqueue with interrupt enabled. So
> it can use spin_lock_irq() instead of spin_lock_irqsave(), right?

Yes, agreed. I will use spin_lock_irq() here in v2.

Thanks for the review.

>
> Nam

Re: [PATCH] staging: fbtft: make dirty_lock IRQ-safe

Posted by Dan Carpenter 1 month, 3 weeks ago

On Fri, Aug 07, 2026 at 02:53:26PM +0200, Nam Cao wrote:
> sh_def@163.com writes:
> > diff --git a/drivers/staging/fbtft/fbtft-core.c b/drivers/staging/fbtft/fbtft-core.c
> > index ca0c38221c16..193643d0329d 100644
> > --- a/drivers/staging/fbtft/fbtft-core.c
> > +++ b/drivers/staging/fbtft/fbtft-core.c
> > @@ -298,14 +298,20 @@ static void fbtft_mkdirty(struct fb_info *info, int y, int height)
> >  {
> >  	struct fbtft_par *par = info->par;
> >  	struct fb_deferred_io *fbdefio = info->fbdefio;
> > +	unsigned long flags;
> >  
> >  	/* Mark display lines/area as dirty */
> > -	spin_lock(&par->dirty_lock);
> > +	/*
> > +	 * fbcon takes dirty_lock while holding console_owner. Disable local
> > +	 * interrupts here so a printk hardirq cannot acquire console_owner
> > +	 * while dirty_lock is held and create the inverse lock ordering.
> > +	 */
> 
> Beside that reason, we also need spin_lock_irqsave() because
> fbtft_mkdirty() can be called in both task context and hardirq
> context. And this reason alone suffices and usually is why
> spin_lock_irqsave() is used, so I think a comment is not necessary.
> 
> But I am fine with it either way.

AI always adds comments.  If we keep alowing obvious comments, the kernel
will turn into reading the Terms and Conditions which are impossible to
read in a single human life time.

regards,
dan carpenter