On 01/11/2022 12.44, Ilpo Järvinen wrote:
On Tue, 1 Nov 2022, Martin Hundebøll wrote:
On 28/10/2022 11.40, Ilpo Järvinen wrote:
On Fri, 28 Oct 2022, Ilpo Järvinen wrote:
On Wed, 26 Oct 2022, Ilpo Järvinen wrote:
On Tue, 25 Oct 2022, Martin Hundebøll wrote:
Skip registration of the platform device used for built-in ports, if
no
such ports are configured/created.
Signed-off-by: Martin Hundebøll <martin@xxxxxxxxxx>
For patches 1-3:
Tested-by: Ilpo Järvinen <ilpo.jarvinen@xxxxxxxxxxxxxxx>
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@xxxxxxxxxxxxxxx>
Please include these tags into the next version of your submission.
Thank you.
Actually, I just found out that some set of cmdline parameters do no
longer work the same. So I'm retracting both of my tags for now.
=0 did work as expected due to this change which I tested and some other
values >4 but there now seems to be problem of the console not showing up
like previously when I don't give nr_uarts at all.
NAK from me until the problem is resolved adequately.
Already the patch 1/4 causes an unacceptable reassignment of ttySx
targets. This is going to break people's cmdline console setups so you
need to find a better way.
Before any of these patches:
[ 0.000000] Command line: console=ttyS0,115200n8 8250.nr_uarts=4
[ 0.021031] Kernel command line: console=ttyS0,115200n8 8250.nr_uarts=4
[ 0.441924] printk: console [ttyS0] enabled
[ 2.243165] printk: console [ttyS0] disabled
[ 2.245682] dw-apb-uart.6: ttyS0 at MMIO 0x4010006000 (irq = 33,
base_baud = 115200) is a 16550A
[ 4.010237] printk: console [ttyS0] enabled
[ 5.933887] dw-apb-uart.7: ttyS1 at MMIO 0x4010007000 (irq = 16,
base_baud = 6250000) is a 16550A
[ 5.952829] dw-apb-uart.8: ttyS2 at MMIO 0x4010008000 (irq = 17,
base_baud = 6250000) is a 16550A
After 1/4 ttyS0 is no longer the same:
[ 0.000000] Command line: console=ttyS0,115200n8 8250.nr_uarts=4
[ 0.021023] Kernel command line: console=ttyS0,115200n8 8250.nr_uarts=4
[ 0.441872] printk: console [ttyS0] enabled
[ 2.233584] dw-apb-uart.6: ttyS4 at MMIO 0x4010006000 (irq = 33,
base_baud = 115200) is a 16550A
[ 2.241955] dw-apb-uart.7: ttyS5 at MMIO 0x4010007000 (irq = 16,
base_baud = 6250000) is a 16550A
[ 2.249804] dw-apb-uart.8: ttyS6 at MMIO 0x4010008000 (irq = 17,
base_baud = 6250000) is a 16550A
Thanks for testing this.
The old behavior is wrong: your designware ports replace the built-in ones (0
to 3) instead of using the unused ones (4 to 31). With these patches, it acts
as I'd expect: the built-in ports are kept, and any later discovered ports
follow.
Yes, breaking existing systems in this way is unacceptable. I'm not sure how
to approach this, but I'm inclined to introduce a new config variable to keep
the broken behavior?
To me this looks now more an attempt to repurpose the nr_uarts to mean
something it really hasn't. Clearly it hasn't meant non-discoverable
ports earlier but something related to the maximum number ports. This also
explains why we had that misunderstanding about the meaning of nr_uarts
between us earlier.
I was confused about the relation between SERIAL_8250_NR_UARTS and SERIAL_8250_RUNTIME_UARTS, and started digging through the code. The description of NR_UARTS clearly states that it controls the number of *supported* uarts, but I found that it was in fact controlled by RUNTIME_UARTS (8250.nr_uarts).
With the current behavior, ever setting NR_UARTS to anything different from RUNTIME_UARTS makes no sense, since the code only loops until RUNTIME_UARTS. Both Arch Linux and Fedora have come to the same conclusion, since they set both configs to 32. So now I have 32 useless /dev/ttyS* nodes on my laptop. If you run ubuntu on a machine with no uart hardware attached, you'll get 48 useless ttyS* nodes, of which only the first 32 will ever be available for real hardware.
If you look at uevents from 8250 when the designware ports are discovered, you'll see a DELETE of the built-in ttyS0 first, and then an ADD of the designware ttyS0. My understanding is that we should never have had that built-in ttyS0 in the first place?
The code which makes this "wrong behavior" to happen is the last loop in
serial8250_find_match_or_unused(). Given its comment, I think it has been
very much intentional behavior. Pretending that non-discoverable port is
more real/precious than a port that was later _discovered_ and is very
much a real one is what brings you to this renumbering issue.
The serial8250_find_match_or_unused() logic exists to relate non-discoverable ports to those discovered later through ACPI PNP. Some of my systems announces those built-in ports with ACPI, in which case the discovered ports are assigned to their "built-in" numbers.
I believe all systems running today announce these built-in ports through ACPI PNP, but they are still usable as early consoles, so this special logic is still needed?
I'm not anymore sure about your goals really. Now it sounds one of the
goals is preserving the non-discoverable ports, whereas previously it was
just about allowing 0 of them.
My initial goal was to rely on discoverable ports only. Some of our systems have no built-in uarts, so the bash init script failed to even start, because the kernel command line had console=ttyS0, and bash tried to do set terminal attributes on it. I am only trying to fix the code "the right way", but that might not be possible without breaking numbering fo existing systems.
How about you add entirely new CONFIG and/or param for this minimum number
of non-discoverable ports and make the last resort loop (or perhaps all
but the first loop) in serial8250_find_match_or_unused() to honor that
(start looking only from port above that index). If it defaults to 0, I
think this renumbering issue is avoided. Would it work for all the goals
you have?
That would work for me. But shouldn't we clean up the SERIAL_8250_NR_UARTS vs. SERIAL_8250_RUNTIME_UARTS also?
