Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Message-ID: <438c88e740f674ad334cdc88004fcec5b9ec57f4.camel@linux.ibm.com>
Subject: Re: [RFC v2 04/39] char: impi, tpm: depend on HAS_IOPORT
From:   Niklas Schnelle <schnelle@linux.ibm.com>
To:     Ahmad Fatoum <a.fatoum@pengutronix.de>,
        Arnd Bergmann <arnd@arndb.de>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        linux-kernel@vger.kernel.org, linux-arch@vger.kernel.org,
        linux-pci@vger.kernel.org, Arnd Bergmann <arnd@kernel.org>,
        Corey Minyard <minyard@acm.org>,
        Peter Huewe <peterhuewe@gmx.de>,
        Jarkko Sakkinen <jarkko@kernel.org>,
        "moderated list:IPMI SUBSYSTEM" 
        <openipmi-developer@lists.sourceforge.net>,
        "open list:TPM DEVICE DRIVER" <linux-integrity@vger.kernel.org>,
        Pengutronix Kernel Team <kernel@pengutronix.de>
Date:   Mon, 02 May 2022 16:34:10 +0200
In-Reply-To: <ff7605de-fe12-3bbf-cce9-aec18be9d54e@pengutronix.de>
References: <20220429135108.2781579-1-schnelle@linux.ibm.com>
         <20220429135108.2781579-7-schnelle@linux.ibm.com>
         <07c39877d9e940a96be41e21e22fe45dbb73d949.camel@linux.ibm.com>
         <ff7605de-fe12-3bbf-cce9-aec18be9d54e@pengutronix.de>
Content-Type: text/plain; charset="UTF-8"
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Fri, 2022-04-29 at 16:33 +0200, Ahmad Fatoum wrote:
> Hello Niklas,
> 
> On 29.04.22 16:23, Niklas Schnelle wrote:
> > > Hello Niklas,
> > > 
> > > On 29.04.22 15:50, Niklas Schnelle wrote:
> > > > In a future patch HAS_IOPORT=n will result in inb()/outb() and friends
> > > > not being declared. We thus need to add this dependency and ifdef
> > > > sections of code using inb()/outb() as alternative access methods.
> > > > 
> > > > Co-developed-by: Arnd Bergmann <arnd@kernel.org>
> > > > Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
> > > 
> > > [snip]
> > > 
> > > > diff --git a/drivers/char/tpm/tpm_infineon.c b/drivers/char/tpm/tpm_infineon.c
> > > > index 9c924a1440a9..2d2ae37153ba 100644
> > > > --- a/drivers/char/tpm/tpm_infineon.c
> > > > +++ b/drivers/char/tpm/tpm_infineon.c
> > > > @@ -51,34 +51,40 @@ static struct tpm_inf_dev tpm_dev;
> > > >  
> > > >  static inline void tpm_data_out(unsigned char data, unsigned char offset)
> > > >  {
> > > > +#ifdef CONFIG_HAS_IOPORT
> > > >       if (tpm_dev.iotype == TPM_INF_IO_PORT)
> > > >               outb(data, tpm_dev.data_regs + offset);
> > > >       else
> > > > +#endif
> > > 
> > > This looks ugly. Can't you declare inb/outb anyway and skip the definition,
> > > so you can use IS_ENABLED() here instead?
> > > 
> > > You can mark the declarations with __compiletime_error("some message"), so
> > > if an IS_ENABLED() reference is not removed at compile time, you get some
> > > readable error message instead of a link error.
> > > 
> > > Cheers,
> > > Ahmad
> > 
> > I didn't know about __compiletime_error() that certainly sounds
> > interesting even when using a normal #ifdef.
> > 
> > That said either with the function not being declared or this
> > __compiletime_error() mechanism I would think that using IS_ENABLED()
> > relies on compiler optimizations not to compile in the missing/error
> > function call, right? I'm not sure if that is something we should do.
> 
> Yes, it assumes your compiler is able to discard the body of an if (0),
> which we already assume, otherwise it wouldn't make sense for any existing
> code to use __compiletime_error().
> 
> To me this sounds much cleaner than #ifdefs in the midst of functions,
> which are a detriment to maintainability.
> 
> Cheers,
> Ahmad
> 

Ok, makes sense. I'll look into using __compiletime_error() and
IS_ENABLED().