On Mon, Nov 25, 2024, at 12:26, Andy Shevchenko wrote:
> On Mon, Nov 25, 2024 at 12:06:16PM +0100, Arnd Bergmann wrote:
>> What I suspect is going on with the f4c23a140d80 commit
>> is the same bug I mentioned earlier in this thread, where
>> __serial8250_isa_init_ports() just always registers
>> 'nr_uarts' (CONFIG_SERIAL_8250_RUNTIME_UARTS) ports,
>> unlike any other serial driver.
>
> But the configuration can give less than old_serial_port contains.
> See dozens of the explicit settings in the defconfigs.
I don't see any of the upstream defconfigs doing this
though, the only ones setting CONFIG_SERIAL_8250_RUNTIME_UARTS
are those that have an empty old_serial_port[].
Note that SERIAL_PORT_DFNS is only defined on x86, alpha
and m68k (for q40), which are the main PC-like platforms.
I see that all three have identical definitions of
SERIAL_PORT_DFNS, so I think these should just be moved
next to the __serial8250_isa_init_ports definition, with
the entire thing moved into a separate ISA driver or
an #ifdef around it. This is of course not the problem
at hand, but it would help separate the x86/isa and
non-x86 platform device cases further.
>> This used to be required before 9d86719f8769 ("serial:
>> 8250: Allow using ports higher than SERIAL_8250_RUNTIME_UARTS"),
>> but I don't see why this is still a thing now, other than
>> for using setserial on i486-class PCs with nonstandard ISA
>> ports.
>>
>> On non-x86 machines, it only ever seems to create extra
>> ports that are likely to crash the system if opened, either
>> because they lack proper serial_in/serial_out callbacks,
>> or because the default UPIO_PORT callbacks end up poking
>> unmapped memory.
>>
>> Do you see any reason why we can't just do the version below?
>
> Perhaps we may do this way (it seems better to me than previous
> suggestions), but it also needs to be carefully checked against
> those configurations that set it explicitly.
Yes, at least to make sure that the numbering of the uarts
does not change. I expect it's actually the same, but don't
know for sure.
> If we go this way, it would be also nice to add a comment explaining that
> this is module parameter (as it's done for those ones above).
>
>> +unsigned int nr_uarts = ARRAY_SIZE(old_serial_port);;
Right.
Arnd
