From mboxrd@z Thu Jan 1 00:00:00 1970 From: Arnd Bergmann Subject: Re: [PATCH v4 1/4] soc: Add TmFifo driver for Mellanox BlueField Soc Date: Mon, 29 Oct 2018 15:52:46 +0100 Message-ID: References: <1540403734-137721-1-git-send-email-lsun@mellanox.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=m.gmane.org@lists.infradead.org To: Liming Sun Cc: DTML , David Woods , arm-soc , Olof Johansson , Robin Murphy , Linux ARM List-Id: devicetree@vger.kernel.org On Mon, Oct 29, 2018 at 3:17 PM Liming Sun 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 From mboxrd@z Thu Jan 1 00:00:00 1970 From: arnd@arndb.de (Arnd Bergmann) Date: Mon, 29 Oct 2018 15:52:46 +0100 Subject: [PATCH v4 1/4] soc: Add TmFifo driver for Mellanox BlueField Soc In-Reply-To: References: <1540403734-137721-1-git-send-email-lsun@mellanox.com> Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Mon, Oct 29, 2018 at 3:17 PM Liming Sun 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