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

[Niklas Schnelle]

On Fri, 2024-10-04 at 12:48 +0000, Arnd Bergmann wrote:
> On Fri, Oct 4, 2024, at 10:09, Niklas Schnelle wrote:
> 
> > I'm working on a new proposal for 8250 now. Basically I think we can
> > put the warning/error in serial8250_pci_setup_port(). And then for
> > those 8250_pci.c "sub drivers" that require I/O ports instead of just
> > ifdeffing out their setup/init/exit we can define anything but setup to
> > NULL and setup to pci_default_setup() such that the latter will find
> > the I/O port BAR via FL_GET_BASE() and subsequently cause the error
> > print in serial8250_pci_setup_port(). It's admittedly a bit odd but it
> > also keeps the #ifdefs to just around the code that wouldn't compile.
> 
> Right, makes sense.
> 
> > One thing I'm not happy with yet is the handling around
> > is_upf_fourport(port) in 8250_pci.c. With !HAS_IOPORT this is defined
> > as false. With that it makes sure that inb_p()/outb_p() aren't called
> > but I think this only works because the compiler usually drops the dead
> > if clause. I think there were previous discussions around this but I
> > think just like IS_ENABLED() checks this isn't quite correct.
> 
> Not sure what you mean, we rely on dead code elimination in all
> kinds of places. The only bit to be aware of is that normal
> 'inline' functions may not get constant-folded all the time,
> but anything that is either a macro or __always_inline does
> work.

Ah ok, didn't know this was okay to rely on. Then I can roll the extra
#ifdefs back. Need to address the test bot's finding anyway. There we
can get #ifdef __i386__ but also !HAS_IOPORT on um.

> 
> I just verified that the version below does what Maciej
> suggested with IS_ENABLED() in 8250-pci, and this works fine.
> 
>        Arnd

Your version below is also pretty nice a bit more spread out but less
#ifdefs. That said at least in my NeoVim setup the #ifdefs are nicely
grayed out via clang-analyzer / lsp which to me actually makes #ifdefs
quite easy to see at a glance but that's probably a niche opinion.

> 
> diff --git a/drivers/tty/serial/8250/8250_pci.c b/drivers/tty/serial/8250/8250_pci.c
> index 6709b6a5f301..784190824aad 100644
> --- a/drivers/tty/serial/8250/8250_pci.c
> +++ b/drivers/tty/serial/8250/8250_pci.c
> @@ -928,6 +928,14 @@ static int pci_netmos_init(struct pci_dev *dev)
>  	return num_serial;
>  }
>  
> +static int serial_8250_need_ioport(struct pci_dev *dev)
> +{
> +	dev_warn(&dev->dev,
> +		 "Serial port not supported because of missing I/O resource\n");
> +
> +	return -ENXIO;
> +}
> +
---8<---