Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <ff7605de-fe12-3bbf-cce9-aec18be9d54e@pengutronix.de>
Date:   Fri, 29 Apr 2022 16:33:48 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.7.0
From:   Ahmad Fatoum <a.fatoum@pengutronix.de>
Subject: Re: [RFC v2 04/39] char: impi, tpm: depend on HAS_IOPORT
To:     Niklas Schnelle <schnelle@linux.ibm.com>,
        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>
References: <20220429135108.2781579-1-schnelle@linux.ibm.com>
 <20220429135108.2781579-7-schnelle@linux.ibm.com>
 <07c39877d9e940a96be41e21e22fe45dbb73d949.camel@linux.ibm.com>
Content-Language: en-US
In-Reply-To: <07c39877d9e940a96be41e21e22fe45dbb73d949.camel@linux.ibm.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

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


> 
> 


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |