Re: [PATCH v2 2/3] tpm: Address !chip->auth in tpm_buf_append_name()

[James Bottomley <James.Bottomley@HansenPartnership.com>]

On Thu, 2024-07-04 at 10:07 -0700, Linus Torvalds wrote:
> On Wed, 3 Jul 2024 at 13:11, James Bottomley
> <James.Bottomley@hansenpartnership.com> wrote:
> > 
> > if (__and(IS_ENABLED(CONFIG_TCG_TPM2_HMAC), chip->auth))
> 
> Augh. Please don't do this.
> 
> That "__and()" thing may work, but it's entirely accidental that it
> does.
> 
> It's designed for config options _only_, and the fact that it then
> happens to work for "first argument is config option, second argument
> is C conditional".
> 
> The comment says that it's implementing "&&" using preprocessor
> expansion only, but it's a *really* limited form of it. The arguments
> are *not* arbitrary.
> 
> So no. Don't do this.
> 
> Just create a helper inline like
> 
>     static inline struct tpm2_auth *chip_auth(struct tpm_chip *chip)
>     {
>     #ifdef CONFIG_TCG_TPM2_HMAC
>         return chip->auth;
>     #else
>         return NULL;
>     #endif
>     }
> 
> and if we really want to have some kind of automatic way of doing
> this, we will *NOT* be using __and(), we'd do something like
> 
>         /* Return zero or 'value' depending on whether OPTION is
> enabled or not */
>         #define IF_ENABLED(option, value) __and(IS_ENABLED(option),
> value)
> 
> that actually would be documented and meaningful.
> 
> Not this internal random __and() implementation that is purely a
> kconfig.h helper macro and SHOULD NOT be used anywhere else.

I actually like the latter version, but instinct tells me that if this
is the first time the kernel has ever needed something like this then
perhaps we should go with the former because that's how everyone must
have handled it in the past.

James