linux-kernel - Re: [PATCH 1/1] tty: serial: handle HAS

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <d5c4e2aa1cc04bf1259fb6bd44f269621693b114.camel@linux.ibm.com>
Date: Fri, 04 Oct 2024 18:03:45 +0200
From: Niklas Schnelle <schnelle@...ux.ibm.com>
To: Arnd Bergmann <arnd@...nel.org>, "Maciej W. Rozycki" <macro@...am.me.uk>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jiri Slaby
 <jirislaby@...nel.org>,
        Ilpo Järvinen
 <ilpo.jarvinen@...ux.intel.com>,
        linux-serial@...r.kernel.org, Heiko
 Carstens <hca@...ux.ibm.com>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/1] tty: serial: handle HAS_IOPORT dependencies

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