On 2024-11-14 06:40:03-0800, Guenter Roeck wrote:
> On 11/13/24 23:27, Thomas Weißschuh wrote:
> > On 2024-11-13 22:51:37-0800, Guenter Roeck wrote:
> > > On 11/13/24 20:40, Thomas Weißschuh wrote:
> > > > On 2024-11-12 22:52:36-0800, Guenter Roeck wrote:
> > > > > On 11/12/24 20:39, Thomas Weißschuh wrote:
> > > > > > Using an #ifdef in a C source files to have different definitions
> > > > > > of the same symbol makes the code harder to read and understand.
> > > > > > Furthermore it makes it harder to test compilation of the different
> > > > > > branches.
> > > > > >
> > > > > > Replace the ifdeffery with IS_ENABLED() which is just a normal
> > > > > > conditional.
> > > > > > The resulting binary is still the same as before as the compiler
> > > > > > optimizes away all the unused code and definitions.
> > > > > >
> > > > > > Signed-off-by: Thomas Weißschuh <linux@xxxxxxxxxxxxxx>
> > > > > > ---
> > > > > > This confused me a bit while looking at the implementation of
> > > > > > HWMON_C_REGISTER_TZ.
> > > > > > ---
> > > > > > drivers/hwmon/hwmon.c | 21 ++++++---------------
> > > > > > 1 file changed, 6 insertions(+), 15 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c
> > > > > > index 9c35c4d0369d7aad7ea61ccd25f4f63fc98b9e02..86fb674c85d3f54d475be014c3fd3dd74c815c57 100644
> > > > > > --- a/drivers/hwmon/hwmon.c
> > > > > > +++ b/drivers/hwmon/hwmon.c
> > > > > > @@ -147,11 +147,6 @@ static DEFINE_IDA(hwmon_ida);
> > > > > > /* Thermal zone handling */
> > > > > > -/*
> > > > > > - * The complex conditional is necessary to avoid a cyclic dependency
> > > > > > - * between hwmon and thermal_sys modules.
> > > > > > - */
> > > > > > -#ifdef CONFIG_THERMAL_OF
> > > > > > static int hwmon_thermal_get_temp(struct thermal_zone_device *tz, int *temp)
> > > > > > {
> > > > > > struct hwmon_thermal_data *tdata = thermal_zone_device_priv(tz);
> > > > > > @@ -257,6 +252,9 @@ static int hwmon_thermal_register_sensors(struct device *dev)
> > > > > > void *drvdata = dev_get_drvdata(dev);
> > > > > > int i;
> > > > > > + if (!IS_ENABLED(CONFIG_THERMAL_OF))
> > > > > > + return 0;
> > > > > > +
> > > > > > for (i = 1; info[i]; i++) {
> > > > > > int j;
> > > > > > @@ -285,6 +283,9 @@ static void hwmon_thermal_notify(struct device *dev, int index)
> > > > > > struct hwmon_device *hwdev = to_hwmon_device(dev);
> > > > > > struct hwmon_thermal_data *tzdata;
> > > > > > + if (!IS_ENABLED(CONFIG_THERMAL_OF))
> > > > > > + return;
> > > > > > +
> > > > > > list_for_each_entry(tzdata, &hwdev->tzdata, node) {
> > > > > > if (tzdata->index == index) {
> > > > > > thermal_zone_device_update(tzdata->tzd,
> > > > >
> > > > > There is no dummy function for thermal_zone_device_update().
> > > > > I really don't want to trust the compiler/linker to remove that code
> > > > > unless someone points me to a document explaining that it is guaranteed
> > > > > to not cause any problems.
> > > >
> > > > I'm fairly sure that a declaration should be enough, and believe
> > > > to remember seeing such advise somewhere.
> > > > However there is not even a function declaration with !CONFIG_THERMAL.
> > > > So I can add an actual stub for it for v2.
> > > >
> > > > What do you think?
> > > >
> > > You mean an extern declaration without the actual function ?
> >
> > Stub as in empty inline function:
> >
> > static inline void thermal_zone_device_update(struct thermal_zone_device *,
> > enum thermal_notify_event)
> > { }
> >
>
> Sure, that would work, but it would have to be declared in the thermal subsystem.
Of course.
> > > I'd really want to see that documented. It would seem rather unusual.
> >
> > > From Documentation/process/coding-style.rst
> >
> > 21) Conditional Compilation
> > ---------------------------
> >
> > Wherever possible, don't use preprocessor conditionals (#if, #ifdef) in .c
> > files; doing so makes code harder to read and logic harder to follow. Instead,
> > use such conditionals in a header file defining functions for use in those .c
> > files, providing no-op stub versions in the #else case, and then call those
> > functions unconditionally from .c files. The compiler will avoid generating
> > any code for the stub calls, producing identical results, but the logic will
> > remain easy to follow.
> >
> > [..]
> >
> > Within code, where possible, use the IS_ENABLED macro to convert a Kconfig
> > symbol into a C boolean expression, and use it in a normal C conditional:
> >
> > .. code-block:: c
> >
> > if (IS_ENABLED(CONFIG_SOMETHING)) {
> > ...
> > }
> >
> > The compiler will constant-fold the conditional away, and include or exclude
> > the block of code just as with an #ifdef, so this will not add any runtime
> > overhead.
> >
> > [..]
> >
> > While this primarily talks about stubs, the fact that
> > "the compiler will constant-fold the conditional away" can be understood
> > that the linker will never see those function calls and therefore the
> > functions don't have to be present during linking.
>
> Yes, I am aware of that. However, that is not a formal language definition.
Formal as in ANSI/ISO? I don't think these ever say anything about
optimizations. And also the compilers don't really write down the
details AFAIK.
> Yes, in normal builds with a modern compiler it will be optimized away.
> However, I don't think that will happen if the kernel is built with -O0.
The kernel is never built with -O0. It's either -O2 or -Os.
It's a Kconfig choice between CC_OPTIMIZE_FOR_PERFORMANCE or
CC_OPTIMIZE_FOR_SIZE, one is always enabled.
This is not clear from the logic in Makefile.
With -O0 more or less everything breaks.
> > So a declaration would be enough. But an actual stub doesn't hurt either.
> >
>
> I disagree. You did not point to a formal language definition saying that dead code
> shall be optimized away and that functions called by such dead code don't have
> to actually exist.
>
> Do we really have to argue about this ? Please provide examples from elsewhere
> in the kernel which implement what you have suggested (not just the use of
> IS_ENABLED(), but the call to functions without stub which don't exist
> if the code is not enabled), and we can go from there.
None of the hwmon functions have stubs if !CONFIG_HWMON, only declarations.
And there are multiple drivers that use the pattern from above.
One example from drivers/net/wireless/mediatek/mt76/mt7921/init.c
static int mt7921_thermal_init(struct mt792x_phy *phy)
{
struct wiphy *wiphy = phy->mt76->hw->wiphy;
struct device *hwmon;
const char *name;
if (!IS_REACHABLE(CONFIG_HWMON))
return 0;
name = devm_kasprintf(&wiphy->dev, GFP_KERNEL, "mt7921_%s",
wiphy_name(wiphy));
if (!name)
return -ENOMEM;
hwmon = devm_hwmon_device_register_with_groups(&wiphy->dev, name, phy,
mt7921_hwmon_groups);
return PTR_ERR_OR_ZERO(hwmon);
}
*But* the thermal subsystem is actually using stubs.
So the same should be done for thermal_zone_device_update().
As mentioned before, my original claim that declarations of the thermal
functions are already usable when !CONFIG_THERMAL was wrong.
And if the thermal header is to be touched, it should as well be a stub
for consistency.
Given that there are already stubs for all kinds of thermal functions,
this doesn't seem like it would be an issue.
Thomas
