[PATCH] rpmb: use IS_REACHABLE instead of IS_ENABLED

[PATCH] rpmb: use IS_REACHABLE instead of IS_ENABLED

[Jens Wiklander]

Use the macro IS_REACHABLE instead of IS_ENABLED in <linux/rpmb.h> when
deciding if prototypes or stubbed static inline functions should be
provided. This fixes link errors when the calling code is builtin while
the RPMB subsystem is a module.

Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202409021448.RSvcBPzt-lkp@intel.com/
Fixes: 1e9046e3a154 ("rpmb: add Replay Protected Memory Block (RPMB) subsystem")
Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
---
 include/linux/rpmb.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/rpmb.h b/include/linux/rpmb.h
index cccda73eea4d..37b5273c4027 100644
--- a/include/linux/rpmb.h
+++ b/include/linux/rpmb.h
@@ -61,7 +61,7 @@ struct rpmb_dev {
 
 #define to_rpmb_dev(x)		container_of((x), struct rpmb_dev, dev)
 
-#if IS_ENABLED(CONFIG_RPMB)
+#if IS_REACHABLE(CONFIG_RPMB)
 struct rpmb_dev *rpmb_dev_get(struct rpmb_dev *rdev);
 void rpmb_dev_put(struct rpmb_dev *rdev);
 struct rpmb_dev *rpmb_dev_find_device(const void *data,
-- 
2.34.1

Re: [PATCH] rpmb: use IS_REACHABLE instead of IS_ENABLED

[Arnd Bergmann]

On Mon, Sep 2, 2024, at 08:07, Jens Wiklander wrote:
> Use the macro IS_REACHABLE instead of IS_ENABLED in <linux/rpmb.h> when
> deciding if prototypes or stubbed static inline functions should be
> provided. This fixes link errors when the calling code is builtin while
> the RPMB subsystem is a module.
>
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: 
> https://lore.kernel.org/oe-kbuild-all/202409021448.RSvcBPzt-lkp@intel.com/
> Fixes: 1e9046e3a154 ("rpmb: add Replay Protected Memory Block (RPMB) 
> subsystem")
> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>

Please don't work around a bug like this, fix it properly instead.

> diff --git a/include/linux/rpmb.h b/include/linux/rpmb.h
> index cccda73eea4d..37b5273c4027 100644
> --- a/include/linux/rpmb.h
> +++ b/include/linux/rpmb.h
> @@ -61,7 +61,7 @@ struct rpmb_dev {
> 
>  #define to_rpmb_dev(x)		container_of((x), struct rpmb_dev, dev)
> 
> -#if IS_ENABLED(CONFIG_RPMB)
> +#if IS_REACHABLE(CONFIG_RPMB)
>  struct rpmb_dev *rpmb_dev_get(struct rpmb_dev *rdev);
>  void rpmb_dev_put(struct rpmb_dev *rdev);

This gives very unexpected runtime behavior where both RPMB and
its user are enabled, but it doesn't work.

I think what you want here is a dependency like

      depends on RPMB || !RPMB

for every caller. This enforces at build time that the MMC core can
be built either when RPMB is disabled, of when it is reachable.

    Arnd

Re: [PATCH] rpmb: use IS_REACHABLE instead of IS_ENABLED

[Jens Wiklander]

Hi Arnd,

On Mon, Sep 2, 2024 at 10:31 AM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Mon, Sep 2, 2024, at 08:07, Jens Wiklander wrote:
> > Use the macro IS_REACHABLE instead of IS_ENABLED in <linux/rpmb.h> when
> > deciding if prototypes or stubbed static inline functions should be
> > provided. This fixes link errors when the calling code is builtin while
> > the RPMB subsystem is a module.
> >
> > Reported-by: kernel test robot <lkp@intel.com>
> > Closes:
> > https://lore.kernel.org/oe-kbuild-all/202409021448.RSvcBPzt-lkp@intel.com/
> > Fixes: 1e9046e3a154 ("rpmb: add Replay Protected Memory Block (RPMB)
> > subsystem")
> > Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
>
> Please don't work around a bug like this, fix it properly instead.
>
> > diff --git a/include/linux/rpmb.h b/include/linux/rpmb.h
> > index cccda73eea4d..37b5273c4027 100644
> > --- a/include/linux/rpmb.h
> > +++ b/include/linux/rpmb.h
> > @@ -61,7 +61,7 @@ struct rpmb_dev {
> >
> >  #define to_rpmb_dev(x)               container_of((x), struct rpmb_dev, dev)
> >
> > -#if IS_ENABLED(CONFIG_RPMB)
> > +#if IS_REACHABLE(CONFIG_RPMB)
> >  struct rpmb_dev *rpmb_dev_get(struct rpmb_dev *rdev);
> >  void rpmb_dev_put(struct rpmb_dev *rdev);
>
> This gives very unexpected runtime behavior where both RPMB and
> its user are enabled, but it doesn't work.
>
> I think what you want here is a dependency like
>
>       depends on RPMB || !RPMB
>
> for every caller. This enforces at build time that the MMC core can
> be built either when RPMB is disabled, of when it is reachable.

Thanks for the suggestion.

I tried adding the dependency above for MMC_BLOCK and OPTEE.

Setting CONFIG_RPMB to "y" works as expected CONFIG_MMC_BLOCK and
CONFIG_OPTEE can be configured as both "y" or "m".

Setting CONFIG_RPMB to "m" will make CONFIG_MMC_BLOCK and CONFIG_OPTEE
"m" too, even if they are set as "y" in the defconfig. So the module
status seems to spread to the options with the dependency. For
tristate options, it guarantees that the expected features are
available even if it changes from built-in to a loadable module.

It will do nothing for bool options, but that's a hypothetical case
for now.  Wouldn't spreading the built-in status to CONFIG_RPMB be
better? So CONFIG_RPMB turns into "y" if an option configured as
built-in depends on it. I don't know how to do that though, so that
could be saved for when it's needed.

I'll send out patches for MMC_BLOCK and OPTEE options shortly if we're
happy with the "depends on RPMB || !RPMB" approach.

Cheers,
Jens

Re: [PATCH] rpmb: use IS_REACHABLE instead of IS_ENABLED

[Arnd Bergmann]

On Mon, Sep 2, 2024, at 09:54, Jens Wiklander wrote:
>
> Thanks for the suggestion.
>
> I tried adding the dependency above for MMC_BLOCK and OPTEE.
>
> Setting CONFIG_RPMB to "y" works as expected CONFIG_MMC_BLOCK and
> CONFIG_OPTEE can be configured as both "y" or "m".
>
> Setting CONFIG_RPMB to "m" will make CONFIG_MMC_BLOCK and CONFIG_OPTEE
> "m" too, even if they are set as "y" in the defconfig. So the module
> status seems to spread to the options with the dependency. For
> tristate options, it guarantees that the expected features are
> available even if it changes from built-in to a loadable module.

Correct.

> It will do nothing for bool options, but that's a hypothetical case
> for now.  Wouldn't spreading the built-in status to CONFIG_RPMB be
> better? So CONFIG_RPMB turns into "y" if an option configured as
> built-in depends on it. I don't know how to do that though, so that
> could be saved for when it's needed.

That would require using 'select' instead of 'depends on', but that has
other problems.

If a symbol that uses RPMB is 'bool', you need to do one of two things:

- if this is a driver that is always built-in, it would instead need
  'depends on RPMB!=m' or 'depends on RPMB=y || !RPMB', both have
  the same effect.

- If there is a bool symbol that turns on an optional feature in
  a modular driver, it would have to be something like this:

config MMC_USE_RPMB
     bool "Build MMC support for RPMB"
     depends on MMC && RPMB
     depends on RPMB=y || RPMB=MMC

> I'll send out patches for MMC_BLOCK and OPTEE options shortly if we're
> happy with the "depends on RPMB || !RPMB" approach.

Sounds good, thanks.

      Arnd