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

[Hui Su <sh_def@163.com>]

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