RE: [PATCH] firmware: arm_scmi: fix i.MX build dependency

[Peng Fan <peng.fan@xxxxxxx>]

> Subject: Re: [PATCH] firmware: arm_scmi: fix i.MX build dependency
> 
> On Sun, Nov 17, 2024, at 11:03, Cristian Marussi wrote:
> > On Sat, Nov 16, 2024 at 12:05:18AM +0100, Arnd Bergmann wrote:
> >>
> >> arm-linux-gnueabi-ld: sound/soc/fsl/fsl_mqs.o: in function
> `fsl_mqs_sm_write':
> >> fsl_mqs.c:(.text+0x1aa): undefined reference to
> `scmi_imx_misc_ctrl_set'
> >> arm-linux-gnueabi-ld: sound/soc/fsl/fsl_mqs.o: in function
> `fsl_mqs_sm_read':
> >> fsl_mqs.c:(.text+0x1ee): undefined reference to
> `scmi_imx_misc_ctrl_get'
> >>
> >
> > The SCMI drivers, like the newly added IMX_SCMI_MISC_DRV,
> generally
> > make ue of the related vendor protocol like IMX_SCMI_MISC_EXT,
> BUT the
> > SCMI stack is designed in a way that NO symbols are needed to be
> > exported by the protocol layer (to avoid a huge and growing number
> of
> > symbols exports)...so usually the current DRV-->PROTO dependency is
> fine.
> >
> > In this case, AFAIU, it is the SCMI driver that in turn exports a few
> > helpers that are used by another driver fsl_mqs, which in turn could
> > be compiled and work with or without the SCMI stack, so with this
> > patch we are artificially reversing the DRV<--PROTO dependency to
> > solve this scenario in all the compillation scenarios...
> >
> > ....BUT given that the IMX_SCMI_MISC_DRV is the one that should
> export
> > the missing symbols could NOT this solved in a cleaner way, without
> > adding the fake reverse dependency, by instead modifying the header
> of
> > the driver with something like the classic:
> 
> > --->8-----
> > diff --git a/include/linux/firmware/imx/sm.h
> > b/include/linux/firmware/imx/sm.h index
> 9b85a3f028d1..3a7a3ec367c5
> > 100644
> > --- a/include/linux/firmware/imx/sm.h
> > +++ b/include/linux/firmware/imx/sm.h
> > @@ -17,7 +17,19 @@
> >  #define SCMI_IMX_CTRL_SAI4_MCLK                4       /* WAKE SAI4
> MCLK */
> >  #define SCMI_IMX_CTRL_SAI5_MCLK                5       /* WAKE SAI5
> MCLK */
> >
> > +#ifdef IMX_SCMI_MISC_DRV
> >  int scmi_imx_misc_ctrl_get(u32 id, u32 *num, u32 *val);  int
> > scmi_imx_misc_ctrl_set(u32 id, u32 val);
> > +#else
> > +static inline int scmi_imx_misc_ctrl_get(u32 id, u32 *num, u32 *val)
> > +{
> > +       return 0;
> > +}
> > +
> > +static inline int scmi_imx_misc_ctrl_set(u32 id, u32 val) {
> > +       return 0;
> > +}
> > +#endif
> 
> This usually doesn't work if the provider of these interfaces can be in a
> loadable module. The #ifdef above means this won't be usable when
> CONFIG_IMX_SCMI_MISC_DRV=m, while changing it to
> IS_ENABLED(CONFIG_IMX_SCMI_MISC_DRV) still produces a link error
> when the consumer is built-in. Changing it to IS_REACHABLE() in turn is
> even worse because it avoids the link failure but makes it silently do
> the wrong thing in some configurations.
> 
> >  #endif
> > ----->8-----------
> >
> > ....to just support compilation in all the scenarios.
> >
> >> This however only works after changing the dependency in the
> >> SND_SOC_FSL_MQS driver as well, which uses 'select
> IMX_SCMI_MISC_DRV'
> >> to turn on a driver it depends on. This is generally a bad idea, so
> >> the best solution is to change that into a dependency.
> >>
> >> To allow the ASoC driver to keep building with the SCMI support,
> this
> >> needs to be an optional dependency that enforces the link-time
> >> dependency if IMX_SCMI_MISC_DRV is a loadable module but not
> depend
> >> on it if that is disabled.
> >>
> >
> > ...and maybe with the above additions you could avoid also these
> other
> > dep changes...
> >
> > ...not sure if I am missing something and I have definitely not tested
> > any of my babbling above...
> 
> In my experience, there is no way to avoid reflecting the dependencies
> correctly in Kconfig: if one driver has an EXPORT_SYMBOL that gets
> picked up by another driver, you need a matching 'depends on'.

Oh. Thanks for sharing the knowledge. I am ok with your change.

Thanks,
Peng.

> 
>        Arnd