| Message ID | 20210511151955.28071-1-vigneshr@ti.com |
|---|---|
| State | New |
| Headers | show |
| Series | serial: 8250: 8250_omap: Fix possible interrupt storm | expand |
On Tue, May 11, 2021 at 08:49:55PM +0530, Vignesh Raghavendra wrote: > It is possible that RX TIMEOUT is signalled after RX FIFO has been > drained, in which case a dummy read of RX FIFO is required to clear RX > TIMEOUT condition. Otherwise, RX TIMEOUT condition is not cleared > leading to an interrupt storm > > Cc: stable@vger.kernel.org How far back does this need to go? What commit id does this fix? What caused this to just show up now vs. previously? thanks, greg k-h
Hi, On 5/28/21 11:09 AM, Tony Lindgren wrote: > Hi Greg, Vignesh & Jan, > > * Greg Kroah-Hartman <gregkh@linuxfoundation.org> [210513 14:17]: >> On Tue, May 11, 2021 at 08:49:55PM +0530, Vignesh Raghavendra wrote: >>> It is possible that RX TIMEOUT is signalled after RX FIFO has been >>> drained, in which case a dummy read of RX FIFO is required to clear RX >>> TIMEOUT condition. Otherwise, RX TIMEOUT condition is not cleared >>> leading to an interrupt storm >>> >>> Cc: stable@vger.kernel.org >> >> How far back does this need to go? What commit id does this fix? What >> caused this to just show up now vs. previously? Sorry, I missed this reply. Issue was reported on AM65x SoC with custom test case from Jan Kiszka that stressed UART with rapid baudrate changes from 9600 to 4M along with data transfer. Based on the condition that led to interrupt storm, I inferred it to affect all SoCs with 8250 OMAP UARTs. But that seems thats not the best idea as seen from OMAP3 regression. Greg, Could you please drop the patch? Very sorry for the inconvenience.. > > I just noticed this causes the following regression in Linux next when > pressing a key on uart console after boot at least on omap3. This seems > to happen on serial_port_in(port, UART_RX) in the quirk handling. > > Vignesh, it seems this quirk needs some soc specific flag added to > it maybe? Or maybe UART_OMAP_RX_LVL register is not available for > all the SoCs? > Yes indeed :( > I think it's best to drop this patch until the issues are resolved, > also there are some open comments above that might be answered by > limiting this quirk to a specific range of SoCs :) > Oops, I did test patch AM33xx assuming its equivalent to OMAP3, but UART IP is quite different. I will respin the patch making sure, workaround applies only to AM65x and K3 SoCs. Regards Vignesh
On Fri, May 28, 2021 at 11:41:36AM +0530, Vignesh Raghavendra wrote: > Hi, > > On 5/28/21 11:09 AM, Tony Lindgren wrote: > > Hi Greg, Vignesh & Jan, > > > > * Greg Kroah-Hartman <gregkh@linuxfoundation.org> [210513 14:17]: > >> On Tue, May 11, 2021 at 08:49:55PM +0530, Vignesh Raghavendra wrote: > >>> It is possible that RX TIMEOUT is signalled after RX FIFO has been > >>> drained, in which case a dummy read of RX FIFO is required to clear RX > >>> TIMEOUT condition. Otherwise, RX TIMEOUT condition is not cleared > >>> leading to an interrupt storm > >>> > >>> Cc: stable@vger.kernel.org > >> > >> How far back does this need to go? What commit id does this fix? What > >> caused this to just show up now vs. previously? > > Sorry, I missed this reply. Issue was reported on AM65x SoC with custom > test case from Jan Kiszka that stressed UART with rapid baudrate changes > from 9600 to 4M along with data transfer. > > Based on the condition that led to interrupt storm, I inferred it to > affect all SoCs with 8250 OMAP UARTs. But that seems thats not the best > idea as seen from OMAP3 regression. > > Greg, > > Could you please drop the patch? Very sorry for the inconvenience.. Now reverted, thanks. greg k-h
On 28.05.21 08:11, Vignesh Raghavendra wrote: > Hi, > > On 5/28/21 11:09 AM, Tony Lindgren wrote: >> Hi Greg, Vignesh & Jan, >> >> * Greg Kroah-Hartman <gregkh@linuxfoundation.org> [210513 14:17]: >>> On Tue, May 11, 2021 at 08:49:55PM +0530, Vignesh Raghavendra wrote: >>>> It is possible that RX TIMEOUT is signalled after RX FIFO has been >>>> drained, in which case a dummy read of RX FIFO is required to clear RX >>>> TIMEOUT condition. Otherwise, RX TIMEOUT condition is not cleared >>>> leading to an interrupt storm >>>> >>>> Cc: stable@vger.kernel.org >>> >>> How far back does this need to go? What commit id does this fix? What >>> caused this to just show up now vs. previously? > > Sorry, I missed this reply. Issue was reported on AM65x SoC with custom > test case from Jan Kiszka that stressed UART with rapid baudrate changes > from 9600 to 4M along with data transfer. > > Based on the condition that led to interrupt storm, I inferred it to > affect all SoCs with 8250 OMAP UARTs. But that seems thats not the best > idea as seen from OMAP3 regression. > > Greg, > > Could you please drop the patch? Very sorry for the inconvenience.. > >> >> I just noticed this causes the following regression in Linux next when >> pressing a key on uart console after boot at least on omap3. This seems >> to happen on serial_port_in(port, UART_RX) in the quirk handling. >> >> Vignesh, it seems this quirk needs some soc specific flag added to >> it maybe? Or maybe UART_OMAP_RX_LVL register is not available for >> all the SoCs? >> > > Yes indeed :( > >> I think it's best to drop this patch until the issues are resolved, >> also there are some open comments above that might be answered by >> limiting this quirk to a specific range of SoCs :) >> > > Oops, I did test patch AM33xx assuming its equivalent to OMAP3, but UART > IP is quite different. I will respin the patch making sure, workaround > applies only to AM65x and K3 SoCs. > > Regards > Vignesh > What's the status here for AM65x? The issue remains present on that platform, and I was hoping to see a quick follow up that limit the fix to that target. Jan -- Siemens AG, T RDA IOT Corporate Competence Center Embedded Linux
On 6/22/21 11:45 AM, Jan Kiszka wrote: >>> Vignesh, it seems this quirk needs some soc specific flag added to >>> it maybe? Or maybe UART_OMAP_RX_LVL register is not available for >>> all the SoCs? >>> >> Yes indeed :( >> >>> I think it's best to drop this patch until the issues are resolved, >>> also there are some open comments above that might be answered by >>> limiting this quirk to a specific range of SoCs :) >>> >> Oops, I did test patch AM33xx assuming its equivalent to OMAP3, but UART >> IP is quite different. I will respin the patch making sure, workaround >> applies only to AM65x and K3 SoCs. >> >> Regards >> Vignesh >> > What's the status here for AM65x? The issue remains present on that > platform, and I was hoping to see a quick follow up that limit the fix > to that target. > Sorry for the delay, I am trying to find which other TI SoCs are affected by this issue. But that exercise will need a bit more time. Will send a fix to address K3 SoCs like AM65x today/tomo.
Tue, Jun 22, 2021 at 11:53:38AM +0530, Vignesh Raghavendra kirjoitti: > On 6/22/21 11:45 AM, Jan Kiszka wrote: > >>> Vignesh, it seems this quirk needs some soc specific flag added to > >>> it maybe? Or maybe UART_OMAP_RX_LVL register is not available for > >>> all the SoCs? > >>> > >> Yes indeed :( > >> > >>> I think it's best to drop this patch until the issues are resolved, > >>> also there are some open comments above that might be answered by > >>> limiting this quirk to a specific range of SoCs :) > >>> > >> Oops, I did test patch AM33xx assuming its equivalent to OMAP3, but UART > >> IP is quite different. I will respin the patch making sure, workaround > >> applies only to AM65x and K3 SoCs. > >> > >> Regards > >> Vignesh > >> > > What's the status here for AM65x? The issue remains present on that > > platform, and I was hoping to see a quick follow up that limit the fix > > to that target. > > Sorry for the delay, I am trying to find which other TI SoCs are > affected by this issue. But that exercise will need a bit more time. > Will send a fix to address K3 SoCs like AM65x today/tomo. This all reminds me the very similar issue one found on Intel integrated (Synopsys DesignWare based) UARTs: https://lore.kernel.org/linux-serial/20170206233000.3021-1-dianders@chromium.org/ https://lore.kernel.org/linux-serial/1440015124-28393-1-git-send-email-california.l.sullivan@intel.com/
On 7/13/21 1:57 AM, andy@surfacebook.localdomain wrote: > Tue, Jun 22, 2021 at 11:53:38AM +0530, Vignesh Raghavendra kirjoitti: >> On 6/22/21 11:45 AM, Jan Kiszka wrote: >>>>> Vignesh, it seems this quirk needs some soc specific flag added to >>>>> it maybe? Or maybe UART_OMAP_RX_LVL register is not available for >>>>> all the SoCs? >>>>> >>>> Yes indeed :( >>>> >>>>> I think it's best to drop this patch until the issues are resolved, >>>>> also there are some open comments above that might be answered by >>>>> limiting this quirk to a specific range of SoCs :) >>>>> >>>> Oops, I did test patch AM33xx assuming its equivalent to OMAP3, but UART >>>> IP is quite different. I will respin the patch making sure, workaround >>>> applies only to AM65x and K3 SoCs. >>>> >>>> Regards >>>> Vignesh >>>> >>> What's the status here for AM65x? The issue remains present on that >>> platform, and I was hoping to see a quick follow up that limit the fix >>> to that target. >> >> Sorry for the delay, I am trying to find which other TI SoCs are >> affected by this issue. But that exercise will need a bit more time. >> Will send a fix to address K3 SoCs like AM65x today/tomo. > > This all reminds me the very similar issue one found on Intel integrated > (Synopsys DesignWare based) UARTs: > Hmm, yes, seems like common problem with some 8250 UARTs although not all TI SoCs show this behavior even though they all claim 8250 compatible. > https://lore.kernel.org/linux-serial/20170206233000.3021-1-dianders@chromium.org/ I am not sure if reading UART_LSR is a good idea in the above patch. Some flags in LSR register are cleared on read (at least that's the case for UARTs on TI SoCs) and thus can result in loss of error/FIFO status information. > https://lore.kernel.org/linux-serial/1440015124-28393-1-git-send-email-california.l.sullivan@intel.com/ > Looks like this never made it. Given the quirks associated with 8250 UARTs, workarounds would need to be tied to specific variants, so I don't know if its possible to implement the fix in 8250 core IRQ handler. PS: v2 of $patch is already merged. Regards Vignesh
On Tue, Jul 13, 2021 at 11:54 AM Vignesh Raghavendra <vigneshr@ti.com> wrote: > On 7/13/21 1:57 AM, andy@surfacebook.localdomain wrote: > > Tue, Jun 22, 2021 at 11:53:38AM +0530, Vignesh Raghavendra kirjoitti: ... > > https://lore.kernel.org/linux-serial/20170206233000.3021-1-dianders@chromium.org/ > > I am not sure if reading UART_LSR is a good idea in the above patch. > Some flags in LSR register are cleared on read (at least that's the case > for UARTs on TI SoCs) and thus can result in loss of error/FIFO status > information. > > > https://lore.kernel.org/linux-serial/1440015124-28393-1-git-send-email-california.l.sullivan@intel.com/ > > Looks like this never made it. Forgot to react to the above. Yes, they never made it because I believe due to the exact reason you mentioned above. Also California set up different experiments IIRC and it shows that the problem didn;t fully disappear with his approach. But maybe yours will work better (at least it's not the first time I have seen it on different hardware according to people's contributions).
* Andy Shevchenko <andy.shevchenko@gmail.com> [210713 09:14]: > On Tue, Jul 13, 2021 at 11:54 AM Vignesh Raghavendra <vigneshr@ti.com> wrote: > > On 7/13/21 1:57 AM, andy@surfacebook.localdomain wrote: > > > Tue, Jun 22, 2021 at 11:53:38AM +0530, Vignesh Raghavendra kirjoitti: > > ... > > > > https://lore.kernel.org/linux-serial/20170206233000.3021-1-dianders@chromium.org/ > > > > I am not sure if reading UART_LSR is a good idea in the above patch. > > Some flags in LSR register are cleared on read (at least that's the case > > for UARTs on TI SoCs) and thus can result in loss of error/FIFO status > > information. > > > > > https://lore.kernel.org/linux-serial/1440015124-28393-1-git-send-email-california.l.sullivan@intel.com/ > > > > Looks like this never made it. > > Forgot to react to the above. Yes, they never made it because I > believe due to the exact reason you mentioned above. Also California > set up different experiments IIRC and it shows that the problem didn;t > fully disappear with his approach. But maybe yours will work better > (at least it's not the first time I have seen it on different hardware > according to people's contributions). Not sure if this is the same issue with noisy lines, but see also the following in case it's related: [PATCH 2/2] serial: 8250_omap: Handle optional overrun-throttle-ms property Also available at [0] below. Regards, Tony [0] https://lore.kernel.org/linux-omap/20210727103533.51547-1-tony@atomide.com/T/#m5f9da26c32503f2937d3d5977310ca337fa0cb5a
diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c index 8ac11eaeca51..c71bd766fa56 100644 --- a/drivers/tty/serial/8250/8250_omap.c +++ b/drivers/tty/serial/8250/8250_omap.c @@ -104,6 +104,9 @@ #define UART_OMAP_EFR2 0x23 #define UART_OMAP_EFR2_TIMEOUT_BEHAVE BIT(6) +/* RX FIFO occupancy indicator */ +#define UART_OMAP_RX_LVL 0x64 + struct omap8250_priv { int line; u8 habit; @@ -625,6 +628,15 @@ static irqreturn_t omap8250_irq(int irq, void *dev_id) serial8250_rpm_get(up); iir = serial_port_in(port, UART_IIR); ret = serial8250_handle_irq(port, iir); + /* + * It is possible that RX TIMEOUT is signalled after FIFO + * has been drained, in which case a dummy read of RX FIFO is + * required to clear RX TIMEOUT condition. + */ + if ((iir & UART_IIR_RX_TIMEOUT) == UART_IIR_RX_TIMEOUT) { + if (serial_port_in(port, UART_OMAP_RX_LVL) == 0) + serial_port_in(port, UART_RX); + } serial8250_rpm_put(up); return IRQ_RETVAL(ret);
It is possible that RX TIMEOUT is signalled after RX FIFO has been drained, in which case a dummy read of RX FIFO is required to clear RX TIMEOUT condition. Otherwise, RX TIMEOUT condition is not cleared leading to an interrupt storm Cc: stable@vger.kernel.org Reported-by: Jan Kiszka <jan.kiszka@siemens.com> Signed-off-by: Vignesh Raghavendra <vigneshr@ti.com> --- drivers/tty/serial/8250/8250_omap.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)