Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1450543 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] 8250: option 'force_polling' for buggy IRQs Max Staudt <mstaudt@suse.de> - 2016-07-26 13:50 +0200
    Re: [PATCH] 8250: option 'force_polling' for buggy IRQs Greg KH <gregkh@linuxfoundation.org> - 2016-07-26 17:10 +0200
      Re: [PATCH] 8250: option 'force_polling' for buggy IRQs Max Staudt <mstaudt@suse.de> - 2016-07-26 18:20 +0200
        Re: [PATCH] 8250: option 'force_polling' for buggy IRQs One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk> - 2016-07-27 14:10 +0200
          Re: [PATCH] 8250: option 'force_polling' for buggy IRQs Max Staudt <mstaudt@suse.de> - 2016-07-27 14:20 +0200
            Re: [PATCH] 8250: option 'force_polling' for buggy IRQs Theodore Ts'o <tytso@mit.edu> - 2016-07-27 15:40 +0200
              Re: [PATCH] 8250: option 'force_polling' for buggy IRQs One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk> - 2016-07-27 22:10 +0200
              Re: [PATCH] 8250: option 'force_polling' for buggy IRQs Max Staudt <mstaudt@suse.de> - 2016-07-28 12:00 +0200
                Re: [PATCH] 8250: option 'force_polling' for buggy IRQs Greg KH <gregkh@linuxfoundation.org> - 2016-07-28 16:50 +0200
                  Re: [PATCH] 8250: option 'force_polling' for buggy IRQs Theodore Ts'o <tytso@mit.edu> - 2016-07-28 18:10 +0200
                Re: [PATCH] 8250: option 'force_polling' for buggy IRQs ebiederm@xmission.com (Eric W. Biederman) - 2016-07-28 21:00 +0200
                  Re: [PATCH] 8250: option 'force_polling' for buggy IRQs One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk> - 2016-07-29 11:30 +0200
                    Re: [PATCH] 8250: option 'force_polling' for buggy IRQs Max Staudt <mstaudt@suse.de> - 2016-07-29 12:00 +0200
                      Re: [PATCH] 8250: option 'force_polling' for buggy IRQs ebiederm@xmission.com (Eric W. Biederman) - 2016-07-29 20:00 +0200
                        Re: [PATCH] 8250: option 'force_polling' for buggy IRQs One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk> - 2016-08-01 17:30 +0200

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

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

[toc] | [next] | [standalone]

#1450644

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

[toc] | [prev] | [next] | [standalone]

#1450667

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

[toc] | [prev] | [next] | [standalone]

#1451253

> 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

[toc] | [prev] | [next] | [standalone]

#1451256

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

[toc] | [prev] | [next] | [standalone]

#1451286

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

[toc] | [prev] | [next] | [standalone]

#1451492

> 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

[toc] | [prev] | [next] | [standalone]

#1451847

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

[toc] | [prev] | [next] | [standalone]

#1451968

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

[toc] | [prev] | [next] | [standalone]

#1451996

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

[toc] | [prev] | [next] | [standalone]

#1452054

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

[toc] | [prev] | [next] | [standalone]

#1452347

> 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

[toc] | [prev] | [next] | [standalone]

#1452362

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

[toc] | [prev] | [next] | [standalone]

#1452501

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

[toc] | [prev] | [next] | [standalone]

#1453273

> 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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web