| Message ID | 20221216115338.7150-1-marex@denx.de |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] serial: stm32: Merge hard IRQ and threaded IRQ handling into single IRQ handler | expand |
On Fri, Dec 16, 2022 at 12:53:38PM +0100, Marek Vasut wrote: > Requesting an interrupt with IRQF_ONESHOT will run the primary handler > in the hard-IRQ context even in the force-threaded mode. The > force-threaded mode is used by PREEMPT_RT in order to avoid acquiring > sleeping locks (spinlock_t) in hard-IRQ context. This combination > makes it impossible and leads to "sleeping while atomic" warnings. > > Use one interrupt handler for both handlers (primary and secondary) > and drop the IRQF_ONESHOT flag which is not needed. > > Fixes: e359b4411c283 ("serial: stm32: fix threaded interrupt handling") I don't think a Fixes tag is warranted as this is only needed due to this undocumented quirk of PREEMPT_RT. And this should not be backported in any case. > Reviewed-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> > Signed-off-by: Marek Vasut <marex@denx.de> > --- > Cc: Alexandre Torgue <alexandre.torgue@foss.st.com> > Cc: Erwan Le Ray <erwan.leray@foss.st.com> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: Jiri Slaby <jirislaby@kernel.org> > Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com> > Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de> > Cc: Thomas Gleixner <tglx@linutronix.de> > Cc: Valentin Caron <valentin.caron@foss.st.com> > Cc: linux-arm-kernel@lists.infradead.org > Cc: linux-stm32@st-md-mailman.stormreply.com > To: linux-serial@vger.kernel.org > --- > V2: - Update patch subject, was: > serial: stm32: Move hard IRQ handling to threaded interrupt context > - Use request_irq() instead, rename the IRQ handler function > V3: - Update the commit message per suggestion from Sebastian > - Add RB from Sebastian > - Add Fixes tag > --- > drivers/tty/serial/stm32-usart.c | 29 +++++++---------------------- > 1 file changed, 7 insertions(+), 22 deletions(-) > > diff --git a/drivers/tty/serial/stm32-usart.c b/drivers/tty/serial/stm32-usart.c > index dfdbcf092facc..bbbab8dc2bfa9 100644 > --- a/drivers/tty/serial/stm32-usart.c > +++ b/drivers/tty/serial/stm32-usart.c > @@ -752,8 +752,9 @@ static irqreturn_t stm32_usart_interrupt(int irq, void *ptr) > struct tty_port *tport = &port->state->port; > struct stm32_port *stm32_port = to_stm32_port(port); > const struct stm32_usart_offsets *ofs = &stm32_port->info->ofs; > - u32 sr; > + unsigned long flags; > unsigned int size; > + u32 sr; > > sr = readl_relaxed(port->membase + ofs->isr); > > @@ -793,27 +794,13 @@ static irqreturn_t stm32_usart_interrupt(int irq, void *ptr) > } > > if ((sr & USART_SR_TXE) && !(stm32_port->tx_ch)) { > - spin_lock(&port->lock); > + spin_lock_irqsave(&port->lock, flags); > stm32_usart_transmit_chars(port); > - spin_unlock(&port->lock); > + spin_unlock_irqrestore(&port->lock, flags); This is not needed as the handler runs with interrupts disabled. > } > > - if (stm32_usart_rx_dma_enabled(port)) > - return IRQ_WAKE_THREAD; > - else > - return IRQ_HANDLED; > -} > - > -static irqreturn_t stm32_usart_threaded_interrupt(int irq, void *ptr) > -{ > - struct uart_port *port = ptr; > - struct tty_port *tport = &port->state->port; > - struct stm32_port *stm32_port = to_stm32_port(port); > - unsigned int size; > - unsigned long flags; > - > /* Receiver timeout irq for DMA RX */ > - if (!stm32_port->throttled) { > + if (stm32_usart_rx_dma_enabled(port) && !stm32_port->throttled) { > spin_lock_irqsave(&port->lock, flags); But you could change this to spin_lock() now. > size = stm32_usart_receive_chars(port, false); > uart_unlock_and_check_sysrq_irqrestore(port, flags); > @@ -1016,10 +1003,8 @@ static int stm32_usart_startup(struct uart_port *port) > u32 val; > int ret; > > - ret = request_threaded_irq(port->irq, stm32_usart_interrupt, > - stm32_usart_threaded_interrupt, > - IRQF_ONESHOT | IRQF_NO_SUSPEND, > - name, port); > + ret = request_irq(port->irq, stm32_usart_interrupt, > + IRQF_NO_SUSPEND, name, port); > if (ret) > return ret; You should also remove /* * Using DMA and threaded handler for the console could lead to * deadlocks. */ if (uart_console(port)) return -ENODEV; from stm32_usart_of_dma_rx_probe() when removing the threaded handler. Johan
Hi Marek, It is OK for me. Tested-by: Valentin Caron <valentin.caron@foss.st.com> Thanks, Valentin On 12/16/22 12:53, Marek Vasut wrote: > Requesting an interrupt with IRQF_ONESHOT will run the primary handler > in the hard-IRQ context even in the force-threaded mode. The > force-threaded mode is used by PREEMPT_RT in order to avoid acquiring > sleeping locks (spinlock_t) in hard-IRQ context. This combination > makes it impossible and leads to "sleeping while atomic" warnings. > > Use one interrupt handler for both handlers (primary and secondary) > and drop the IRQF_ONESHOT flag which is not needed. > > Fixes: e359b4411c283 ("serial: stm32: fix threaded interrupt handling") > Reviewed-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> > Signed-off-by: Marek Vasut <marex@denx.de> > --- > Cc: Alexandre Torgue <alexandre.torgue@foss.st.com> > Cc: Erwan Le Ray <erwan.leray@foss.st.com> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: Jiri Slaby <jirislaby@kernel.org> > Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com> > Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de> > Cc: Thomas Gleixner <tglx@linutronix.de> > Cc: Valentin Caron <valentin.caron@foss.st.com> > Cc: linux-arm-kernel@lists.infradead.org > Cc: linux-stm32@st-md-mailman.stormreply.com > To: linux-serial@vger.kernel.org > --- > V2: - Update patch subject, was: > serial: stm32: Move hard IRQ handling to threaded interrupt context > - Use request_irq() instead, rename the IRQ handler function > V3: - Update the commit message per suggestion from Sebastian > - Add RB from Sebastian > - Add Fixes tag > --- > drivers/tty/serial/stm32-usart.c | 29 +++++++---------------------- > 1 file changed, 7 insertions(+), 22 deletions(-) > > diff --git a/drivers/tty/serial/stm32-usart.c b/drivers/tty/serial/stm32-usart.c > index dfdbcf092facc..bbbab8dc2bfa9 100644 > --- a/drivers/tty/serial/stm32-usart.c > +++ b/drivers/tty/serial/stm32-usart.c > @@ -752,8 +752,9 @@ static irqreturn_t stm32_usart_interrupt(int irq, void *ptr) > struct tty_port *tport = &port->state->port; > struct stm32_port *stm32_port = to_stm32_port(port); > const struct stm32_usart_offsets *ofs = &stm32_port->info->ofs; > - u32 sr; > + unsigned long flags; > unsigned int size; > + u32 sr; > > sr = readl_relaxed(port->membase + ofs->isr); > > @@ -793,27 +794,13 @@ static irqreturn_t stm32_usart_interrupt(int irq, void *ptr) > } > > if ((sr & USART_SR_TXE) && !(stm32_port->tx_ch)) { > - spin_lock(&port->lock); > + spin_lock_irqsave(&port->lock, flags); > stm32_usart_transmit_chars(port); > - spin_unlock(&port->lock); > + spin_unlock_irqrestore(&port->lock, flags); > } > > - if (stm32_usart_rx_dma_enabled(port)) > - return IRQ_WAKE_THREAD; > - else > - return IRQ_HANDLED; > -} > - > -static irqreturn_t stm32_usart_threaded_interrupt(int irq, void *ptr) > -{ > - struct uart_port *port = ptr; > - struct tty_port *tport = &port->state->port; > - struct stm32_port *stm32_port = to_stm32_port(port); > - unsigned int size; > - unsigned long flags; > - > /* Receiver timeout irq for DMA RX */ > - if (!stm32_port->throttled) { > + if (stm32_usart_rx_dma_enabled(port) && !stm32_port->throttled) { > spin_lock_irqsave(&port->lock, flags); > size = stm32_usart_receive_chars(port, false); > uart_unlock_and_check_sysrq_irqrestore(port, flags); > @@ -1016,10 +1003,8 @@ static int stm32_usart_startup(struct uart_port *port) > u32 val; > int ret; > > - ret = request_threaded_irq(port->irq, stm32_usart_interrupt, > - stm32_usart_threaded_interrupt, > - IRQF_ONESHOT | IRQF_NO_SUSPEND, > - name, port); > + ret = request_irq(port->irq, stm32_usart_interrupt, > + IRQF_NO_SUSPEND, name, port); > if (ret) > return ret; >
On 12/27/22 15:56, Johan Hovold wrote: [...] >> @@ -793,27 +794,13 @@ static irqreturn_t stm32_usart_interrupt(int irq, void *ptr) >> } >> >> if ((sr & USART_SR_TXE) && !(stm32_port->tx_ch)) { >> - spin_lock(&port->lock); >> + spin_lock_irqsave(&port->lock, flags); >> stm32_usart_transmit_chars(port); >> - spin_unlock(&port->lock); >> + spin_unlock_irqrestore(&port->lock, flags); > > This is not needed as the handler runs with interrupts disabled. On SMP system, another thread on another core can call stm32_usart_transmit_chars() . I don't think removing the locking is correct ? > >> } >> >> - if (stm32_usart_rx_dma_enabled(port)) >> - return IRQ_WAKE_THREAD; >> - else >> - return IRQ_HANDLED; >> -} >> - >> -static irqreturn_t stm32_usart_threaded_interrupt(int irq, void *ptr) >> -{ >> - struct uart_port *port = ptr; >> - struct tty_port *tport = &port->state->port; >> - struct stm32_port *stm32_port = to_stm32_port(port); >> - unsigned int size; >> - unsigned long flags; >> - >> /* Receiver timeout irq for DMA RX */ >> - if (!stm32_port->throttled) { >> + if (stm32_usart_rx_dma_enabled(port) && !stm32_port->throttled) { >> spin_lock_irqsave(&port->lock, flags); > > But you could change this to spin_lock() now. [...]
On 1/5/23 15:56, Valentin CARON wrote: > Hi Marek, Hello Valentin, > It is OK for me. > > Tested-by: Valentin Caron <valentin.caron@foss.st.com> Thank you. I might need one more test for V4 in a bit.
On Thu, Jan 05, 2023 at 09:46:57PM +0100, Marek Vasut wrote: > On 12/27/22 15:56, Johan Hovold wrote: > > [...] > > >> @@ -793,27 +794,13 @@ static irqreturn_t stm32_usart_interrupt(int irq, void *ptr) > >> } > >> > >> if ((sr & USART_SR_TXE) && !(stm32_port->tx_ch)) { > >> - spin_lock(&port->lock); > >> + spin_lock_irqsave(&port->lock, flags); > >> stm32_usart_transmit_chars(port); > >> - spin_unlock(&port->lock); > >> + spin_unlock_irqrestore(&port->lock, flags); > > > > This is not needed as the handler runs with interrupts disabled. > > On SMP system, another thread on another core can call > stm32_usart_transmit_chars() . I don't think removing the locking is > correct ? I didn't say that you should remove the locking, which is very much needed. There's just no need to disable interrupts in a (non-threaded) interrupt handler as that has already been done by IRQ core (and, yes, that is also the case with forced threading). Johan
On 1/6/23 11:56, Johan Hovold wrote: > On Thu, Jan 05, 2023 at 09:46:57PM +0100, Marek Vasut wrote: >> On 12/27/22 15:56, Johan Hovold wrote: >> >> [...] >> >>>> @@ -793,27 +794,13 @@ static irqreturn_t stm32_usart_interrupt(int irq, void *ptr) >>>> } >>>> >>>> if ((sr & USART_SR_TXE) && !(stm32_port->tx_ch)) { >>>> - spin_lock(&port->lock); >>>> + spin_lock_irqsave(&port->lock, flags); >>>> stm32_usart_transmit_chars(port); >>>> - spin_unlock(&port->lock); >>>> + spin_unlock_irqrestore(&port->lock, flags); >>> >>> This is not needed as the handler runs with interrupts disabled. >> >> On SMP system, another thread on another core can call >> stm32_usart_transmit_chars() . I don't think removing the locking is >> correct ? > > I didn't say that you should remove the locking, which is very much > needed. There's just no need to disable interrupts in a (non-threaded) > interrupt handler as that has already been done by IRQ core (and, yes, > that is also the case with forced threading). Ah, understood.
On Mon, Jan 09, 2023 at 11:13:15AM +0100, Sebastian Andrzej Siewior wrote: > On 2022-12-27 15:56:47 [+0100], Johan Hovold wrote: > > On Fri, Dec 16, 2022 at 12:53:38PM +0100, Marek Vasut wrote: > > > Requesting an interrupt with IRQF_ONESHOT will run the primary handler > > > in the hard-IRQ context even in the force-threaded mode. The > > > force-threaded mode is used by PREEMPT_RT in order to avoid acquiring > > > sleeping locks (spinlock_t) in hard-IRQ context. This combination > > > makes it impossible and leads to "sleeping while atomic" warnings. > > > > > > Use one interrupt handler for both handlers (primary and secondary) > > > and drop the IRQF_ONESHOT flag which is not needed. > > > > > > Fixes: e359b4411c283 ("serial: stm32: fix threaded interrupt handling") > > > > I don't think a Fixes tag is warranted as this is only needed due to > > this undocumented quirk of PREEMPT_RT. > > It is not an undocumented quirk of PREEMPT_RT. The part that might not > be well documented is that IRQF_ONESHOT won't run the primary handler as > a threaded handler. This is also the case for IRQF_NO_THREAD and > IRQF_PERCPU but might be more obvious. Yeah, that not being documented seems like an oversight as generally drivers should not need be changed to continue working on PREEMPT_RT (or with forced-threading generally). > Anyway, if the primary handler is not threaded then it runs in HARDIRQ > context and here you must not use a spinlock_t. This is documented in > Documentation/locking/locktypes.rst and there is also a LOCKDEP warning > for this on !RT which is off by default because it triggers with printk > (and this is worked on). All interrupt handlers typically run in hard interrupt context unless explicitly requested to be threaded on !RT and drivers still use spinlock_t for that so not sure how that lockdep warning is supposed to work. Or do you mean that you have a lockdep warning specifically for IRQF_ONESHOT primary handlers? > > And this should not be backported in any case. > > Such things have been backported via -stable in the past. If you > disagree, please keep me in loop while is merged so I can poke people to > backport it as part of the RT patch for the relevant kernels. The author did not seem to think this was stable material as there is no cc-stable tag and it also seems fairly intrusive. But if the ST guys or whoever cares about this driver are fine with this being backported, I don't really mind either. Johan
On 1/12/23 14:13, Johan Hovold wrote: > On Mon, Jan 09, 2023 at 11:13:15AM +0100, Sebastian Andrzej Siewior wrote: >> On 2022-12-27 15:56:47 [+0100], Johan Hovold wrote: >>> On Fri, Dec 16, 2022 at 12:53:38PM +0100, Marek Vasut wrote: >>>> Requesting an interrupt with IRQF_ONESHOT will run the primary handler >>>> in the hard-IRQ context even in the force-threaded mode. The >>>> force-threaded mode is used by PREEMPT_RT in order to avoid acquiring >>>> sleeping locks (spinlock_t) in hard-IRQ context. This combination >>>> makes it impossible and leads to "sleeping while atomic" warnings. >>>> >>>> Use one interrupt handler for both handlers (primary and secondary) >>>> and drop the IRQF_ONESHOT flag which is not needed. >>>> >>>> Fixes: e359b4411c283 ("serial: stm32: fix threaded interrupt handling") >>> >>> I don't think a Fixes tag is warranted as this is only needed due to >>> this undocumented quirk of PREEMPT_RT. >> >> It is not an undocumented quirk of PREEMPT_RT. The part that might not >> be well documented is that IRQF_ONESHOT won't run the primary handler as >> a threaded handler. This is also the case for IRQF_NO_THREAD and >> IRQF_PERCPU but might be more obvious. > > Yeah, that not being documented seems like an oversight as generally > drivers should not need be changed to continue working on PREEMPT_RT (or > with forced-threading generally). > >> Anyway, if the primary handler is not threaded then it runs in HARDIRQ >> context and here you must not use a spinlock_t. This is documented in >> Documentation/locking/locktypes.rst and there is also a LOCKDEP warning >> for this on !RT which is off by default because it triggers with printk >> (and this is worked on). > > All interrupt handlers typically run in hard interrupt context unless > explicitly requested to be threaded on !RT and drivers still use > spinlock_t for that so not sure how that lockdep warning is supposed to > work. Or do you mean that you have a lockdep warning specifically for > IRQF_ONESHOT primary handlers? > >>> And this should not be backported in any case. >> >> Such things have been backported via -stable in the past. If you >> disagree, please keep me in loop while is merged so I can poke people to >> backport it as part of the RT patch for the relevant kernels. > > The author did not seem to think this was stable material as there is no > cc-stable tag and it also seems fairly intrusive. The author does not have enough experience with preempt-rt to make that determination, hence deferred to Sebastian for better judgement. > But if the ST guys or whoever cares about this driver are fine with this > being backported, I don't really mind either. [...]
On Thu, Jan 12, 2023 at 05:38:48PM +0100, Marek Vasut wrote: > On 1/12/23 14:13, Johan Hovold wrote: > > The author did not seem to think this was stable material as there is no > > cc-stable tag and it also seems fairly intrusive. > > The author does not have enough experience with preempt-rt to make that > determination, hence deferred to Sebastian for better judgement. Fair enough. And it's not obvious that the stable team should backport patches that only concern PREEMPT_RT either (e.g. as parts of it are still out-of-tree). The stable tag is still missing from the final revision though. > > But if the ST guys or whoever cares about this driver are fine with this > > being backported, I don't really mind either. Johan
On 1/12/23 18:09, Johan Hovold wrote: > On Thu, Jan 12, 2023 at 05:38:48PM +0100, Marek Vasut wrote: >> On 1/12/23 14:13, Johan Hovold wrote: > >>> The author did not seem to think this was stable material as there is no >>> cc-stable tag and it also seems fairly intrusive. >> >> The author does not have enough experience with preempt-rt to make that >> determination, hence deferred to Sebastian for better judgement. > > Fair enough. And it's not obvious that the stable team should backport > patches that only concern PREEMPT_RT either (e.g. as parts of it are > still out-of-tree). > > The stable tag is still missing from the final revision though. Please pardon my ignorance, which stable tag is missing ? Can you maybe just comment on the V4 and point this out to me ? I'll send a V5 then. Thanks !
On Thu, Jan 12, 2023 at 06:50:34PM +0100, Marek Vasut wrote: > On 1/12/23 18:09, Johan Hovold wrote: > > Fair enough. And it's not obvious that the stable team should backport > > patches that only concern PREEMPT_RT either (e.g. as parts of it are > > still out-of-tree). > > > > The stable tag is still missing from the final revision though. > > Please pardon my ignorance, which stable tag is missing ? > > Can you maybe just comment on the V4 and point this out to me ? I'll > send a V5 then. It's gone from my inbox. But as per Documentation/process/stable-kernel-rules.rst: To have the patch automatically included in the stable tree, add the tag .. code-block:: none Cc: stable@vger.kernel.org in the sign-off area. Once the patch is merged it will be applied to the stable tree without anything else needing to be done by the author or subsystem maintainer. A Fixes tag only indicates which commit introduced a bug, not necessarily that the patch should be backported to stable (even if autosel is likely to pick it up these days). Johan
diff --git a/drivers/tty/serial/stm32-usart.c b/drivers/tty/serial/stm32-usart.c index dfdbcf092facc..bbbab8dc2bfa9 100644 --- a/drivers/tty/serial/stm32-usart.c +++ b/drivers/tty/serial/stm32-usart.c @@ -752,8 +752,9 @@ static irqreturn_t stm32_usart_interrupt(int irq, void *ptr) struct tty_port *tport = &port->state->port; struct stm32_port *stm32_port = to_stm32_port(port); const struct stm32_usart_offsets *ofs = &stm32_port->info->ofs; - u32 sr; + unsigned long flags; unsigned int size; + u32 sr; sr = readl_relaxed(port->membase + ofs->isr); @@ -793,27 +794,13 @@ static irqreturn_t stm32_usart_interrupt(int irq, void *ptr) } if ((sr & USART_SR_TXE) && !(stm32_port->tx_ch)) { - spin_lock(&port->lock); + spin_lock_irqsave(&port->lock, flags); stm32_usart_transmit_chars(port); - spin_unlock(&port->lock); + spin_unlock_irqrestore(&port->lock, flags); } - if (stm32_usart_rx_dma_enabled(port)) - return IRQ_WAKE_THREAD; - else - return IRQ_HANDLED; -} - -static irqreturn_t stm32_usart_threaded_interrupt(int irq, void *ptr) -{ - struct uart_port *port = ptr; - struct tty_port *tport = &port->state->port; - struct stm32_port *stm32_port = to_stm32_port(port); - unsigned int size; - unsigned long flags; - /* Receiver timeout irq for DMA RX */ - if (!stm32_port->throttled) { + if (stm32_usart_rx_dma_enabled(port) && !stm32_port->throttled) { spin_lock_irqsave(&port->lock, flags); size = stm32_usart_receive_chars(port, false); uart_unlock_and_check_sysrq_irqrestore(port, flags); @@ -1016,10 +1003,8 @@ static int stm32_usart_startup(struct uart_port *port) u32 val; int ret; - ret = request_threaded_irq(port->irq, stm32_usart_interrupt, - stm32_usart_threaded_interrupt, - IRQF_ONESHOT | IRQF_NO_SUSPEND, - name, port); + ret = request_irq(port->irq, stm32_usart_interrupt, + IRQF_NO_SUSPEND, name, port); if (ret) return ret;
Requesting an interrupt with IRQF_ONESHOT will run the primary handler in the hard-IRQ context even in the force-threaded mode. The force-threaded mode is used by PREEMPT_RT in order to avoid acquiring sleeping locks (spinlock_t) in hard-IRQ context. This combination makes it impossible and leads to "sleeping while atomic" warnings. Use one interrupt handler for both handlers (primary and secondary) and drop the IRQF_ONESHOT flag which is not needed. Fixes: e359b4411c283 ("serial: stm32: fix threaded interrupt handling") Reviewed-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> Signed-off-by: Marek Vasut <marex@denx.de> --- Cc: Alexandre Torgue <alexandre.torgue@foss.st.com> Cc: Erwan Le Ray <erwan.leray@foss.st.com> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Cc: Jiri Slaby <jirislaby@kernel.org> Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com> Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Valentin Caron <valentin.caron@foss.st.com> Cc: linux-arm-kernel@lists.infradead.org Cc: linux-stm32@st-md-mailman.stormreply.com To: linux-serial@vger.kernel.org --- V2: - Update patch subject, was: serial: stm32: Move hard IRQ handling to threaded interrupt context - Use request_irq() instead, rename the IRQ handler function V3: - Update the commit message per suggestion from Sebastian - Add RB from Sebastian - Add Fixes tag --- drivers/tty/serial/stm32-usart.c | 29 +++++++---------------------- 1 file changed, 7 insertions(+), 22 deletions(-)