Re: [PATCH 1/1] tty: serial: handle HAS_IOPORT dependencies

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Mon, Nov 25, 2024 at 12:06:16PM +0100, Arnd Bergmann wrote:
> On Mon, Nov 25, 2024, at 11:33, Andy Shevchenko wrote:
> > On Mon, Nov 25, 2024 at 10:53:46AM +0100, Arnd Bergmann wrote:
> >> 
> >> This does however revert f4c23a140d80 ("serial: 8250:
> >> fix null-ptr-deref in serial8250_start_tx()") and
> >> might bring back the bug came from opening an
> >> uninitialized uart. On the other hand, I don't see
> >> why that doesn't also crash from accessing the invalid
> >> I/O port on the same architectures.
> >
> > AFAICS it does only partial revert, so I don't see how your patch
> > may break that.
> 
> I think it's a complete revert, it's just not entirely obvious
> since serial8250_setup_port() got split out by 9d86719f8769
> ("serial: 8250: Allow using ports higher than
> SERIAL_8250_RUNTIME_UARTS") and the original callsite got
> moved by your ffd8e8bd26e9 ("serial: 8250: Extract platform
> driver").

Ah, okay.

> 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.

> 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.

...

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);;

...

>  	/*
> -	 * Set up initial ISA ports based on nr_uart module param, or else
> -	 * default to CONFIG_SERIAL_8250_RUNTIME_UARTS. Note that we do not
> -	 * need to increase nr_uarts when setting up the initial ISA ports.

> +	 * Set up initial ISA ports based on nr_uart module param. Note that 

Just in case it has a trailing whitespace.

> +	 * we do not need to increase nr_uarts when setting up the initial
> +	 * ISA ports.
>  	 */

-- 
With Best Regards,
Andy Shevchenko