[PATCH] 8250: option 'force_polling' for buggy IRQs

[PATCH] 8250: option 'force_polling' for buggy IRQs

[Max Staudt]

Some serial ports may not emit IRQs properly, or there may be a defect
in their routing on the motherboard.

This patch allows these ports to be used anyway (or until a better
workaround is known for a specific platform), though with no guarantees.

If you have such a buggy UART, boot Linux with 8250.force_polling=1 .

It is essentially the kernel level version of:

  setserial /dev/ttySn irq 0

and builds upon the polling code that is already in the kernel.
---
 drivers/tty/serial/8250/8250_core.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
index 13ad5c3..c6a7fd1 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -58,6 +58,11 @@ static struct uart_driver serial8250_reg;
 
 static unsigned int skip_txen_test; /* force skip of txen test at init time */
 
+/* Force polled mode for all newly detected ports.
+ * This can be used if IRQs don't arrive and similar buggyness.
+ */
+static unsigned int force_polling;
+
 #define PASS_LIMIT	512
 
 #include <asm/serial.h>
@@ -335,6 +340,13 @@ static int univ8250_setup_irq(struct uart_8250_port *up)
 			  uart_poll_timeout(port) + HZ / 5);
 	}
 
+	if (force_polling) {
+		pr_debug("ttyS%d - using polled mode instead of interrupt %u\n",
+			 serial_index(port),
+			 port->irq);
+		port->irq = 0;
+	}
+
 	/*
 	 * If the "interrupt" for this port doesn't correspond with any
 	 * hardware interrupt, we use a timer-based system.  The original
@@ -1206,6 +1218,9 @@ MODULE_PARM_DESC(nr_uarts, "Maximum number of UARTs supported. (1-" __MODULE_STR
 module_param(skip_txen_test, uint, 0644);
 MODULE_PARM_DESC(skip_txen_test, "Skip checking for the TXEN bug at init time");
 
+module_param(force_polling, uint, 0644);
+MODULE_PARM_DESC(force_polling, "Set ports to polling mode at init time");
+
 #ifdef CONFIG_SERIAL_8250_RSA
 module_param_array(probe_rsa, ulong, &probe_rsa_count, 0444);
 MODULE_PARM_DESC(probe_rsa, "Probe I/O ports for RSA");
@@ -1232,6 +1247,7 @@ static void __used s8250_options(void)
 	module_param_cb(share_irqs, &param_ops_uint, &share_irqs, 0644);
 	module_param_cb(nr_uarts, &param_ops_uint, &nr_uarts, 0644);
 	module_param_cb(skip_txen_test, &param_ops_uint, &skip_txen_test, 0644);
+	module_param_cb(force_polling, uint, 0644);
 #ifdef CONFIG_SERIAL_8250_RSA
 	__module_param_call(MODULE_PARAM_PREFIX, probe_rsa,
 		&param_array_ops, .arr = &__param_arr_probe_rsa,
-- 
2.6.6

Re: [PATCH] 8250: option 'force_polling' for buggy IRQs

[Greg KH]

On Mon, Jul 25, 2016 at 07:36:15PM +0200, Max Staudt wrote:
> Some serial ports may not emit IRQs properly, or there may be a defect
> in their routing on the motherboard.
> 
> This patch allows these ports to be used anyway (or until a better
> workaround is known for a specific platform), though with no guarantees.
> 
> If you have such a buggy UART, boot Linux with 8250.force_polling=1 .

Ick, don't add new module parameters if at all possible.

> It is essentially the kernel level version of:
> 
>   setserial /dev/ttySn irq 0

Why can't you just do this instead?

thanks,

greg k-h

Re: [PATCH] 8250: option 'force_polling' for buggy IRQs

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1936 bytes --]

Hi,

[auto build test ERROR on tty/tty-testing]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Max-Staudt/8250-option-force_polling-for-buggy-IRQs/20160726-013812
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git tty-testing
config: x86_64-randconfig-x012-201630 (attached as .config)
compiler: gcc-6 (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   drivers/tty/serial/8250/8250_core.c: In function 's8250_options':
>> drivers/tty/serial/8250/8250_core.c:1250:43: error: macro "module_param_cb" requires 4 arguments, but only 3 given
     module_param_cb(force_polling, uint, 0644);
                                              ^
>> drivers/tty/serial/8250/8250_core.c:1250:2: error: 'module_param_cb' undeclared (first use in this function)
     module_param_cb(force_polling, uint, 0644);
     ^~~~~~~~~~~~~~~
   drivers/tty/serial/8250/8250_core.c:1250:2: note: each undeclared identifier is reported only once for each function it appears in

vim +/module_param_cb +1250 drivers/tty/serial/8250/8250_core.c

  1244	#undef MODULE_PARAM_PREFIX
  1245	#define MODULE_PARAM_PREFIX "8250_core."
  1246	
  1247		module_param_cb(share_irqs, &param_ops_uint, &share_irqs, 0644);
  1248		module_param_cb(nr_uarts, &param_ops_uint, &nr_uarts, 0644);
  1249		module_param_cb(skip_txen_test, &param_ops_uint, &skip_txen_test, 0644);
> 1250		module_param_cb(force_polling, uint, 0644);
  1251	#ifdef CONFIG_SERIAL_8250_RSA
  1252		__module_param_call(MODULE_PARAM_PREFIX, probe_rsa,
  1253			&param_array_ops, .arr = &__param_arr_probe_rsa,

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 18880 bytes --]

Re: [PATCH] 8250: option 'force_polling' for buggy IRQs

[Max Staudt]

On 07/25/2016 07:47 PM, Greg KH wrote:
> On Mon, Jul 25, 2016 at 07:36:15PM +0200, Max Staudt wrote:
>> Some serial ports may not emit IRQs properly, or there may be a defect
>> in their routing on the motherboard.
>>
>> This patch allows these ports to be used anyway (or until a better
>> workaround is known for a specific platform), though with no guarantees.
>>
>> If you have such a buggy UART, boot Linux with 8250.force_polling=1 .
> 
> Ick, don't add new module parameters if at all possible.

I agree, I'd rather not add a parameter either, but...

- It's a hardware issue
- It needs to be handled at boot time
- It can't be auto-detected (AFAIK)


The idea is that this parameter allows for a workaround until someone comes
up with a workaround or autodetection (if ever). And it can be used to
debug future buggy hardware.


>> It is essentially the kernel level version of:
>>
>>   setserial /dev/ttySn irq 0
> 
> Why can't you just do this instead?

Because it's too late by the time we reach userspace.

In case of "console=ttyS0" the decision to use polling needs to happen before
ttyS0 is opened from userspace, as the system will otherwise hang for up to
30 seconds at a time. Input is mostly dropped, thus I can't even use BREAK+B
to force reboot it.

As it stands now, I can't even boot the system with "rdinit=/bin/bash".
The force_polling option makes the system somewhat usable, albeit the serial
output is very slow.


Curiously, the kernel's printk() is as fast as it should be. It's just
userspace that is slow. Any idea why that is the case?



The kbuild test bot spotted a mistake, I'll send a new patch.



Thanks,

Max

[PATCHv2] 8250: option 'force_polling' for buggy IRQs

[Max Staudt]

Some serial ports may not emit IRQs properly, or there may be a defect
in their routing on the motherboard.

This patch allows these ports to be used anyway (or until a better
workaround is known for a specific platform), though with no guarantees.

If you have such a buggy UART, boot Linux with 8250.force_polling=1 .

It is essentially the kernel level version of:

  setserial /dev/ttySn irq 0

and builds upon the polling code that is already in the kernel.
---

v2:
- Fixed module_param_cb() line
---
 drivers/tty/serial/8250/8250_core.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
index 13ad5c3..536e226 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -58,6 +58,11 @@ static struct uart_driver serial8250_reg;
 
 static unsigned int skip_txen_test; /* force skip of txen test at init time */
 
+/* Force polled mode for all newly detected ports.
+ * This can be used if IRQs don't arrive and similar buggyness.
+ */
+static unsigned int force_polling;
+
 #define PASS_LIMIT	512
 
 #include <asm/serial.h>
@@ -335,6 +340,13 @@ static int univ8250_setup_irq(struct uart_8250_port *up)
 			  uart_poll_timeout(port) + HZ / 5);
 	}
 
+	if (force_polling) {
+		pr_debug("ttyS%d - using polled mode instead of interrupt %u\n",
+			 serial_index(port),
+			 port->irq);
+		port->irq = 0;
+	}
+
 	/*
 	 * If the "interrupt" for this port doesn't correspond with any
 	 * hardware interrupt, we use a timer-based system.  The original
@@ -1206,6 +1218,9 @@ MODULE_PARM_DESC(nr_uarts, "Maximum number of UARTs supported. (1-" __MODULE_STR
 module_param(skip_txen_test, uint, 0644);
 MODULE_PARM_DESC(skip_txen_test, "Skip checking for the TXEN bug at init time");
 
+module_param(force_polling, uint, 0644);
+MODULE_PARM_DESC(force_polling, "Set ports to polling mode at init time");
+
 #ifdef CONFIG_SERIAL_8250_RSA
 module_param_array(probe_rsa, ulong, &probe_rsa_count, 0444);
 MODULE_PARM_DESC(probe_rsa, "Probe I/O ports for RSA");
@@ -1232,6 +1247,7 @@ static void __used s8250_options(void)
 	module_param_cb(share_irqs, &param_ops_uint, &share_irqs, 0644);
 	module_param_cb(nr_uarts, &param_ops_uint, &nr_uarts, 0644);
 	module_param_cb(skip_txen_test, &param_ops_uint, &skip_txen_test, 0644);
+	module_param_cb(force_polling, &param_ops_uint, &force_polling, 0644);
 #ifdef CONFIG_SERIAL_8250_RSA
 	__module_param_call(MODULE_PARAM_PREFIX, probe_rsa,
 		&param_array_ops, .arr = &__param_arr_probe_rsa,
-- 
2.6.6

[PATCHv3] 8250: option 'force_polling' for buggy IRQs

[Max Staudt]

Some serial ports may not emit IRQs properly, or there may be a defect
in their routing on the motherboard.

This patch allows these ports to be used anyway (or until a better
workaround is known for a specific platform), though with no guarantees.

If you have such a buggy UART, boot Linux with 8250.force_polling=1 .

It is essentially the kernel level version of:

  setserial /dev/ttySn irq 0

and builds upon the polling code that is already in the kernel.

Signed-off-by: Max Staudt <mstaudt@suse.de>
---

v2:
- Fixed module_param_cb() line

v3:
- Added Signed-off-by:
---
 drivers/tty/serial/8250/8250_core.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
index 13ad5c3..536e226 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -58,6 +58,11 @@ static struct uart_driver serial8250_reg;
 
 static unsigned int skip_txen_test; /* force skip of txen test at init time */
 
+/* Force polled mode for all newly detected ports.
+ * This can be used if IRQs don't arrive and similar buggyness.
+ */
+static unsigned int force_polling;
+
 #define PASS_LIMIT	512
 
 #include <asm/serial.h>
@@ -335,6 +340,13 @@ static int univ8250_setup_irq(struct uart_8250_port *up)
 			  uart_poll_timeout(port) + HZ / 5);
 	}
 
+	if (force_polling) {
+		pr_debug("ttyS%d - using polled mode instead of interrupt %u\n",
+			 serial_index(port),
+			 port->irq);
+		port->irq = 0;
+	}
+
 	/*
 	 * If the "interrupt" for this port doesn't correspond with any
 	 * hardware interrupt, we use a timer-based system.  The original
@@ -1206,6 +1218,9 @@ MODULE_PARM_DESC(nr_uarts, "Maximum number of UARTs supported. (1-" __MODULE_STR
 module_param(skip_txen_test, uint, 0644);
 MODULE_PARM_DESC(skip_txen_test, "Skip checking for the TXEN bug at init time");
 
+module_param(force_polling, uint, 0644);
+MODULE_PARM_DESC(force_polling, "Set ports to polling mode at init time");
+
 #ifdef CONFIG_SERIAL_8250_RSA
 module_param_array(probe_rsa, ulong, &probe_rsa_count, 0444);
 MODULE_PARM_DESC(probe_rsa, "Probe I/O ports for RSA");
@@ -1232,6 +1247,7 @@ static void __used s8250_options(void)
 	module_param_cb(share_irqs, &param_ops_uint, &share_irqs, 0644);
 	module_param_cb(nr_uarts, &param_ops_uint, &nr_uarts, 0644);
 	module_param_cb(skip_txen_test, &param_ops_uint, &skip_txen_test, 0644);
+	module_param_cb(force_polling, &param_ops_uint, &force_polling, 0644);
 #ifdef CONFIG_SERIAL_8250_RSA
 	__module_param_call(MODULE_PARAM_PREFIX, probe_rsa,
 		&param_array_ops, .arr = &__param_arr_probe_rsa,
-- 
2.6.6

Re: [PATCH] 8250: option 'force_polling' for buggy IRQs

[Greg KH]

On Tue, Jul 26, 2016 at 01:42:13PM +0200, Max Staudt wrote:
> On 07/25/2016 07:47 PM, Greg KH wrote:
> > On Mon, Jul 25, 2016 at 07:36:15PM +0200, Max Staudt wrote:
> >> Some serial ports may not emit IRQs properly, or there may be a defect
> >> in their routing on the motherboard.
> >>
> >> This patch allows these ports to be used anyway (or until a better
> >> workaround is known for a specific platform), though with no guarantees.
> >>
> >> If you have such a buggy UART, boot Linux with 8250.force_polling=1 .
> > 
> > Ick, don't add new module parameters if at all possible.
> 
> I agree, I'd rather not add a parameter either, but...
> 
> - It's a hardware issue
> - It needs to be handled at boot time

Why?

> - It can't be auto-detected (AFAIK)

Why not?  Can't you have a quirk for this specific, broken, device?

> The idea is that this parameter allows for a workaround until someone comes
> up with a workaround or autodetection (if ever). And it can be used to
> debug future buggy hardware.

module paramters are horrid, they don't scale (which uart is this for?),
and no one ever changes them.

> >> It is essentially the kernel level version of:
> >>
> >>   setserial /dev/ttySn irq 0
> > 
> > Why can't you just do this instead?
> 
> Because it's too late by the time we reach userspace.
> 
> In case of "console=ttyS0" the decision to use polling needs to happen before
> ttyS0 is opened from userspace, as the system will otherwise hang for up to
> 30 seconds at a time. Input is mostly dropped, thus I can't even use BREAK+B
> to force reboot it.
> 
> As it stands now, I can't even boot the system with "rdinit=/bin/bash".
> The force_polling option makes the system somewhat usable, albeit the serial
> output is very slow.
> 
> Curiously, the kernel's printk() is as fast as it should be. It's just
> userspace that is slow. Any idea why that is the case?

Ah, then something else might be wrong here, I suggest you track this
down please.

thanks,

greg k-h

Re: [PATCH] 8250: option 'force_polling' for buggy IRQs

[Max Staudt]

On 07/26/2016 05:08 PM, Greg KH wrote:
> On Tue, Jul 26, 2016 at 01:42:13PM +0200, Max Staudt wrote:
>> On 07/25/2016 07:47 PM, Greg KH wrote:
>>> Ick, don't add new module parameters if at all possible.
>>
>> I agree, I'd rather not add a parameter either, but...
>>
>> - It's a hardware issue
>> - It needs to be handled at boot time
> 
> Why?

Because even an early shell such as rdinit=/bin/bash locks the console up.


>> - It can't be auto-detected (AFAIK)
> 
> Why not?  Can't you have a quirk for this specific, broken, device?

No, I am not aware of a way to detect the bug itself (other than "there are
no interrupts coming in"). It is not a PCI serial port, either.

Also, it's not worth trying to detect the machine as it is a prototype.

It would rather be useful to have a workaround in place, should a future
system (whether prototype or production) have a similar problem. That way
we can get it into some usable state at all, and still use the other,
working features.

I simply thought this patch may be useful for other people as well, that's
why I sent it upstream.


>> The idea is that this parameter allows for a workaround until someone comes
>> up with a workaround or autodetection (if ever). And it can be used to
>> debug future buggy hardware.
> 
> module paramters are horrid, they don't scale (which uart is this for?),
> and no one ever changes them.

This is part of the generic 8250 code, so it should be valid for all 8250ish
UARTs.


>> Curiously, the kernel's printk() is as fast as it should be. It's just
>> userspace that is slow. Any idea why that is the case?
> 
> Ah, then something else might be wrong here, I suggest you track this
> down please.

The difference in speed is something I'd like to look into, but it's not
high on my priority list.

Maybe you have an idea where the speed difference comes from, so I can look
into it?




Max

Re: [PATCH] 8250: option 'force_polling' for buggy IRQs

[One Thousand Gnomes]

> I simply thought this patch may be useful for other people as well, that's
> why I sent it upstream.

If you set the IRQ to 0 it should poll anyway (0 means 'no IRQ') so I
don't think the option is needed. At least it seems sufficient to get me
by when I meet buggy PC BIOSes and the like

Alan

Re: [PATCH] 8250: option 'force_polling' for buggy IRQs

[Max Staudt]

On 07/27/2016 02:09 PM, One Thousand Gnomes wrote:
>> I simply thought this patch may be useful for other people as well, that's
>> why I sent it upstream.
> 
> If you set the IRQ to 0 it should poll anyway (0 means 'no IRQ') so I
> don't think the option is needed. At least it seems sufficient to get me
> by when I meet buggy PC BIOSes and the like

That's exactly what the patch does - but if there is already a way to set the
IRQ to 0, I would of course prefer to use that.


It seems I haven't found the trick you're using - could you please tell me how
you set the IRQ to 0?  I can't change it in the BIOS, so I have to do it at
the kernel level at the latest.



Thanks

Max

Re: [PATCH] 8250: option 'force_polling' for buggy IRQs

[Theodore Ts'o]

On Wed, Jul 27, 2016 at 02:14:24PM +0200, Max Staudt wrote:
> > If you set the IRQ to 0 it should poll anyway (0 means 'no IRQ') so I
> > don't think the option is needed. At least it seems sufficient to get me
> > by when I meet buggy PC BIOSes and the like
> 
> That's exactly what the patch does - but if there is already a way to set the
> IRQ to 0, I would of course prefer to use that.
> 
> It seems I haven't found the trick you're using - could you please tell me how
> you set the IRQ to 0?  I can't change it in the BIOS, so I have to do it at
> the kernel level at the latest.

So the problem is that you can't use setserial because you want to use
this port for your console?  And so you don't want to set it in an initscript?

The one thing which is really unfortunate with this patch is that it's
a global, so it forces polling for *all* serial ports.  And it may be
that it's only base ports on the motherboard which needs this hack.

I wonder if it would just be simpler to make it be a CONFIG option
which causes the irq value to zero in arch/x86/include/asm/serial.h?

					- Ted

Re: [PATCH] 8250: option 'force_polling' for buggy IRQs

[One Thousand Gnomes]

> I wonder if it would just be simpler to make it be a CONFIG option
> which causes the irq value to zero in arch/x86/include/asm/serial.h?

That assumes x86 and legacy ports.

For modern boxes its data from elsewhere Devicetree folks can just set
up their devicetree that way, the ACPI afflicted can't force the IRQ at
the boot line just the port.

Alan

Re: [PATCH] 8250: option 'force_polling' for buggy IRQs

[Max Staudt]

On 07/27/2016 03:33 PM, Theodore Ts'o wrote:
> On Wed, Jul 27, 2016 at 02:14:24PM +0200, Max Staudt wrote:
>>> If you set the IRQ to 0 it should poll anyway (0 means 'no IRQ') so I
>>> don't think the option is needed. At least it seems sufficient to get me
>>> by when I meet buggy PC BIOSes and the like
>>
>> That's exactly what the patch does - but if there is already a way to set the
>> IRQ to 0, I would of course prefer to use that.
>>
>> It seems I haven't found the trick you're using - could you please tell me how
>> you set the IRQ to 0?  I can't change it in the BIOS, so I have to do it at
>> the kernel level at the latest.
> 
> So the problem is that you can't use setserial because you want to use
> this port for your console?  And so you don't want to set it in an initscript?

Exactly. There is no initscript when I use rdinit=/bin/bash, so I have no choice
but to set it in the kernel. As soon as /bin/bash accesses /dev/ttyS0, the
console hangs.


> The one thing which is really unfortunate with this patch is that it's
> a global, so it forces polling for *all* serial ports.  And it may be
> that it's only base ports on the motherboard which needs this hack.

I agree, and I thought about it, but since this is meant for a very limited
audience, I think a catch-all is the easiest solution - both in terms of
implementation as well as ease of use.

It's not meant for general consumption anyway.

IMHO chances are that if someone actually goes as far as to install further serial
ports, he'll make sure they are properly working ones. Thus, the workaround will
no longer be needed, and if the broken port really needs to be used, it can be set
in userspace (after using one of the properly working ports for the console).


> I wonder if it would just be simpler to make it be a CONFIG option
> which causes the irq value to zero in arch/x86/include/asm/serial.h?

Alan hit the nail on the head in his reply to your email: I can't set the IRQ in
the system configuration. The other option is to compile a custom kernel for this
system, but that's exactly what I'd like to avoid.

My motivation for this patch is to have a boot time option that allows *any*
kernel to boot on this broken hardware. This allows for console boot on a broken
machine even when the machine has never been used in serial console mode before,
and thus the hardware bug has not been discovered before either.



Max

Re: [PATCH] 8250: option 'force_polling' for buggy IRQs

[Greg KH]

On Thu, Jul 28, 2016 at 11:59:20AM +0200, Max Staudt wrote:
> On 07/27/2016 03:33 PM, Theodore Ts'o wrote:
> > On Wed, Jul 27, 2016 at 02:14:24PM +0200, Max Staudt wrote:
> >>> If you set the IRQ to 0 it should poll anyway (0 means 'no IRQ') so I
> >>> don't think the option is needed. At least it seems sufficient to get me
> >>> by when I meet buggy PC BIOSes and the like
> >>
> >> That's exactly what the patch does - but if there is already a way to set the
> >> IRQ to 0, I would of course prefer to use that.
> >>
> >> It seems I haven't found the trick you're using - could you please tell me how
> >> you set the IRQ to 0?  I can't change it in the BIOS, so I have to do it at
> >> the kernel level at the latest.
> > 
> > So the problem is that you can't use setserial because you want to use
> > this port for your console?  And so you don't want to set it in an initscript?
> 
> Exactly. There is no initscript when I use rdinit=/bin/bash, so I have no choice
> but to set it in the kernel. As soon as /bin/bash accesses /dev/ttyS0, the
> console hangs.
> 
> 
> > The one thing which is really unfortunate with this patch is that it's
> > a global, so it forces polling for *all* serial ports.  And it may be
> > that it's only base ports on the motherboard which needs this hack.
> 
> I agree, and I thought about it, but since this is meant for a very limited
> audience, I think a catch-all is the easiest solution - both in terms of
> implementation as well as ease of use.
> 
> It's not meant for general consumption anyway.

Then I really don't want to have to maintain and support such a kernel
option for 20+ years, just because of one broken system out there like
this.  There are workarounds for this, as you have seen, including
booting with a "real" userspace, so I don't want to take this patch.

thanks,

greg k-h

Re: [PATCH] 8250: option 'force_polling' for buggy IRQs

[Theodore Ts'o]

On Thu, Jul 28, 2016 at 07:47:04AM -0700, Greg KH wrote:
> > I agree, and I thought about it, but since this is meant for a very limited
> > audience, I think a catch-all is the easiest solution - both in terms of
> > implementation as well as ease of use.
> > 
> > It's not meant for general consumption anyway.
> 
> Then I really don't want to have to maintain and support such a kernel
> option for 20+ years, just because of one broken system out there like
> this.  There are workarounds for this, as you have seen, including
> booting with a "real" userspace, so I don't want to take this patch.

One way to make this a bit more general purpose would be too add a way
to configure ttyS0 completely --- e.g., being able to specify the
port, irq, uart type, flags, etc.  It still wouldn't be needed except
for very rarely (since either serial ports use self-configuring buses
--- e.g., USB, PCI, etc.) or they use the legacy x86 ports.

So perhaps Greg might be more willing to take that kind of change, but
even then, it could be argued that the maintaining such a kernel
option for just a few non-standard or broken systems is still not
worth it, but it would at least be somewhat more general purpose.

						- Ted

Re: [PATCH] 8250: option 'force_polling' for buggy IRQs

[Eric W. Biederman]

Max Staudt <mstaudt@suse.de> writes:

> On 07/27/2016 03:33 PM, Theodore Ts'o wrote:
>
>> I wonder if it would just be simpler to make it be a CONFIG option
>> which causes the irq value to zero in arch/x86/include/asm/serial.h?
>
> Alan hit the nail on the head in his reply to your email: I can't set the IRQ in
> the system configuration. The other option is to compile a custom kernel for this
> system, but that's exactly what I'd like to avoid.
>
> My motivation for this patch is to have a boot time option that allows *any*
> kernel to boot on this broken hardware. This allows for console boot on a broken
> machine even when the machine has never been used in serial console mode before,
> and thus the hardware bug has not been discovered before either.

*Scratches my head*

There is already generic handling for this (that triggers polling) in
the irq code if an irq is screaming.  I am assuming the problem is that
your irq never fires?

Serial consoles are already polled for output.  So nothing should
care until userspace starts, and the full serial driver initializes.

Hmm.

The irq code already has handling for some of these situations,
and if that handling is not sufficient I suspect we could pretty
easily extend it for this case.

For irqs that are screaming we disable them and then poll them.
There there are the "irqfixup" and "irqpoll" command line options
that when an irq is received the code tries all of the irq handlers.

So I suspect either "irqfixup" or "irqpoll" would handle this for you.
If not I am certain a small tweak to some of that code would work.

That should be enough to get a system booted for debugging purposes,
which is the point here.  At which point you can dig in figure out what
is wrong and get the problem design fixed.

Eric

Re: [PATCH] 8250: option 'force_polling' for buggy IRQs

[One Thousand Gnomes]

> Serial consoles are already polled for output.  So nothing should
> care until userspace starts, and the full serial driver initializes.

At which point it hangs

> So I suspect either "irqfixup" or "irqpoll" would handle this for you.
> If not I am certain a small tweak to some of that code would work.

irqfixup won't usually help but irqpoll with HZ=1000 ought to, although
it has its own set of problems because not all devices with non shared
IRQ lines take kindly to irqpoll.

Alan

Re: [PATCH] 8250: option 'force_polling' for buggy IRQs

[Max Staudt]

On 07/29/2016 11:23 AM, One Thousand Gnomes wrote:
>> Serial consoles are already polled for output.  So nothing should
>> care until userspace starts, and the full serial driver initializes.
> 
> At which point it hangs

Yep, because the IRQ is never firing. It isn't screaming at all. :)


>> So I suspect either "irqfixup" or "irqpoll" would handle this for you.
>> If not I am certain a small tweak to some of that code would work.
> 
> irqfixup won't usually help but irqpoll with HZ=1000 ought to, although
> it has its own set of problems because not all devices with non shared
> IRQ lines take kindly to irqpoll.

Hmm, the kernel is compiled as tickless. I tried booting with
"irqpoll nohz=off" but that didn't help.


What I could try is to build an option like "irqfire=4,1000" which would
simulate an IRQ on line 4 at 1000 HZ and call the handler every time.
Whether the handling driver likes it is a different question though.

It sounds like "irqpoll" would do something similar, but based on the
kernel's global HZ setting, and calling all handlers unconditionally.
"irqfire" would be more specific.

What do you think?
Would this be useful for other broken systems, too?



Max

Re: [PATCH] 8250: option 'force_polling' for buggy IRQs

[Eric W. Biederman]

Max Staudt <mstaudt@suse.de> writes:

> On 07/29/2016 11:23 AM, One Thousand Gnomes wrote:
>>> Serial consoles are already polled for output.  So nothing should
>>> care until userspace starts, and the full serial driver initializes.
>> 
>> At which point it hangs
>
> Yep, because the IRQ is never firing. It isn't screaming at all. :)
>
>
>>> So I suspect either "irqfixup" or "irqpoll" would handle this for you.
>>> If not I am certain a small tweak to some of that code would work.
>> 
>> irqfixup won't usually help but irqpoll with HZ=1000 ought to, although
>> it has its own set of problems because not all devices with non shared
>> IRQ lines take kindly to irqpoll.

It might make sense to filter non-shared edge triggered interrupts out
of irqpoll for that reason.  Anything that supports a level triggered
interrupt should be fine.

> Hmm, the kernel is compiled as tickless. I tried booting with
> "irqpoll nohz=off" but that didn't help.
>
>
> What I could try is to build an option like "irqfire=4,1000" which would
> simulate an IRQ on line 4 at 1000 HZ and call the handler every time.
> Whether the handling driver likes it is a different question though.
>
> It sounds like "irqpoll" would do something similar, but based on the
> kernel's global HZ setting, and calling all handlers unconditionally.
> "irqfire" would be more specific.
>
> What do you think?
> Would this be useful for other broken systems, too?

I think so.   I think I would go simpler and start a simple recurring
timer in the irqpoll case.

All that is really important is that it is generally reliable and it isn't
too hard to make work.  Which makes me worry a little bit about your
irqfire example (aka someone has to figure out which irq is not firing),
which might be hard if you can't log in.

But shrug.  You are writing the patch.  I am just pointing out where we
have similar work arounds already and where another workaround to cover
your case (and to help others) would likely be appreciated in the kernel.

Eric

Re: [PATCH] 8250: option 'force_polling' for buggy IRQs

[One Thousand Gnomes]

> It might make sense to filter non-shared edge triggered interrupts out
> of irqpoll for that reason.  Anything that supports a level triggered
> interrupt should be fine.

That would also break some platforms that use it today 8(. We'd need
another irqpoll mode - at which point being able to force the serial port
IRQ to 0 would be useful for ACPI boxes (devicetree people just mod the
devicetree!)

Alan