From: Helge Deller <deller@gmx.de>
To: Arnd Bergmann <arnd@kernel.org>,
Niklas Schnelle <schnelle@linux.ibm.com>,
Helge Deller <deller@kernel.org>
Cc: linux-fbdev@vger.kernel.org, dri-devel@lists.freedesktop.org,
Heiko Carstens <hca@linux.ibm.com>,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/1] video: Handle HAS_IOPORT dependencies
Date: Wed, 24 Apr 2024 21:29:20 +0200 [thread overview]
Message-ID: <3eb5b0b4-f025-4396-9d52-05c59d6b904b@gmx.de> (raw)
In-Reply-To: <a666e39d-a894-4e27-aac4-65d11a18358a@app.fastmail.com>
On 4/22/24 21:28, Arnd Bergmann wrote:
> On Mon, Apr 22, 2024, at 10:34, Niklas Schnelle wrote:
>> On Thu, 2024-04-11 at 16:00 +0200, Helge Deller wrote:
>>> * Niklas Schnelle <schnelle@linux.ibm.com>:
>>>> In a future patch HAS_IOPORT=n will disable inb()/outb() and friends at
>>>> compile time. We thus need to #ifdef functions and their callsites which
>>>> unconditionally use these I/O accessors. In the include/video/vga.h
>>>> these are conveniently all those functions with the vga_io_* prefix.
>>>
>>> Why don't you code it like in the patch below?
>>> inb_p(), outb_p() and outw() would then need to be defined externally
>>> without an implementation so that they would generate link time errors
>>> (instead of compile time errors).
>>
>> This may be personal preference but I feel like link time errors would
>> be very late to catch a configuration that can't work. Also this would
>> bypass the __compiletime_error("inb()) requires CONFIG_HAS_IOPORT");
>> added instead of the in*()/out*() helpers to make it easy to spot the
>> problem.
>>
>> I'm not a fan of #ifdeffery either but I think in this case it is
>> simple, well enough contained and overall there aren't that many spots
>> where we need to exclude just some sections of code vs entire drivers
>> with vga.h probably being the worst of them all.
>
> Agreed. I also tried to see if we can move stuff out of vga.h
> to have it included in fewer places, as almost everything that
> uses this header already has a HAS_IOPORT dependency, but that
> would be a lot more work.
>
> The other one that gains a few ugly #ifdefs is the 8250 driver,
> everything else is already merged in linux-next or needs a simple
> Kconfig dependency.
>
> I think we can make the vga.h file a little more readable
> by duplicating the functions and still keep the __compiletime_error()
> version in asm/io.h, see below.
Ok.
I assume you or Niklas will then send an updated patch?
Helge
> diff --git a/include/video/vga.h b/include/video/vga.h
> index 947c0abd04ef..7e1d8252b732 100644
> --- a/include/video/vga.h
> +++ b/include/video/vga.h
> @@ -197,6 +197,23 @@ struct vgastate {
> extern int save_vga(struct vgastate *state);
> extern int restore_vga(struct vgastate *state);
>
> +static inline unsigned char vga_mm_r (void __iomem *regbase, unsigned short port)
> +{
> + return readb (regbase + port);
> +}
> +
> +static inline void vga_mm_w (void __iomem *regbase, unsigned short port, unsigned char val)
> +{
> + writeb (val, regbase + port);
> +}
> +
> +static inline void vga_mm_w_fast (void __iomem *regbase, unsigned short port,
> + unsigned char reg, unsigned char val)
> +{
> + writew (VGA_OUT16VAL (val, reg), regbase + port);
> +}
> +
> +#ifdef CONFIG_HAS_IOPORT
> /*
> * generic VGA port read/write
> */
> @@ -217,22 +234,6 @@ static inline void vga_io_w_fast (unsigned short port, unsigned char reg,
> outw(VGA_OUT16VAL (val, reg), port);
> }
>
> -static inline unsigned char vga_mm_r (void __iomem *regbase, unsigned short port)
> -{
> - return readb (regbase + port);
> -}
> -
> -static inline void vga_mm_w (void __iomem *regbase, unsigned short port, unsigned char val)
> -{
> - writeb (val, regbase + port);
> -}
> -
> -static inline void vga_mm_w_fast (void __iomem *regbase, unsigned short port,
> - unsigned char reg, unsigned char val)
> -{
> - writew (VGA_OUT16VAL (val, reg), regbase + port);
> -}
> -
> static inline unsigned char vga_r (void __iomem *regbase, unsigned short port)
> {
> if (regbase)
> @@ -258,7 +259,25 @@ static inline void vga_w_fast (void __iomem *regbase, unsigned short port,
> else
> vga_io_w_fast (port, reg, val);
> }
> +#else
>
> +static inline unsigned char vga_r (void __iomem *regbase, unsigned short port)
> +{
> + return vga_mm_r(regbase, port);
> +}
> +
> +static inline void vga_w(void __iomem *regbase, unsigned short port, unsigned char val)
> +{
> + vga_mm_w (regbase, port, val);
> +}
> +
> +static inline void vga_w_fast (void __iomem *regbase, unsigned short port,
> + unsigned char reg, unsigned char val)
> +{
> + vga_mm_w_fast(regbase, port, reg, val);
> +}
> +
> +#endif
>
> /*
> * VGA CRTC register read/write
prev parent reply other threads:[~2024-04-24 19:29 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-10 14:23 [PATCH 0/1] video: Handle HAS_IOPORT dependencies Niklas Schnelle
2024-04-10 14:23 ` [PATCH 1/1] " Niklas Schnelle
2024-04-11 14:00 ` Helge Deller
2024-04-22 8:34 ` Niklas Schnelle
2024-04-22 19:28 ` Arnd Bergmann
2024-04-24 19:29 ` Helge Deller [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=3eb5b0b4-f025-4396-9d52-05c59d6b904b@gmx.de \
--to=deller@gmx.de \
--cc=arnd@kernel.org \
--cc=deller@kernel.org \
--cc=dri-devel@lists.freedesktop.org \
--cc=hca@linux.ibm.com \
--cc=linux-fbdev@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=schnelle@linux.ibm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).