On Mon, Oct 29, 2018 at 3:17 PM Liming Sun <lsun@xxxxxxxxxxxx> wrote:
> > >> > +/* Interrupt handler. */
> > >> > +static irqreturn_t tmfifo_irq_handler(int irq, void *dev_id)
> > >> > +{
> > >> > + int i = (uintptr_t)dev_id % sizeof(void *);
> > >> > + struct tmfifo *fifo = dev_id - i;
> > >> > +
> > >> > + if (i < TM_IRQ_CNT && !test_and_set_bit(i, &fifo->pend_events))
> > >> > + schedule_work(&fifo->work);
> > >> > +
> > >> > + return IRQ_HANDLED;
> > >> > +}
> > >>
> > >> Maybe using a request_threaded_irq() would be a better way to defer
> > >> the handler into IRQ context.
> > >
> > > Not sure if I understand this comment correctly... In this case, the
> > > implemented handler
> > > has some mutex_lock() used, which tries to make the logic simple since
> > > multiple services
> > > (network & console) are sharing the same fifo. Thus schedule_work() is
> > > used.
> >
> > schedule_work() and threaded IRQs are just two different ways of deferring
> > into process context where you can do the mutex_lock(). The effect is
> > almost the same, but work queues can be delayed for a substantial
> > amount of time depending on what other work functions have been
> > queued at the same time, and request_threaded_irq() is the more normal
> > way of doing this specifically for an IRQ handler, probably saving a couple
> > of lines of source code.
> >
> > If you have any kind of real-time requirement, you can also assign a
> > specific realtime priority to that interrupt thread.
>
> Good information! Currently this FIFO is mainly for mgmt purpose. I'll try the threaded
> IRQs approach to see whether it can be easily converted and make it into the v5 patch.
> If not easily, probably a separate commit to improve it later?
Sure, no problem. This is not an important change, but I also think it should
be easy to do, in particular as it is meant to simplify the code.
Arnd
