linux-kernel - Re: [PATCH] macintosh/via-pmu: Fix build failure when CONFIG

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <64b6d65-5d5d-23d2-dc8a-c31fc5853349@linux-m68k.org>
Date:   Sun, 20 Mar 2022 11:19:18 +1100 (AEDT)
From:   Finn Thain <fthain@...ux-m68k.org>
To:     Geert Uytterhoeven <geert@...ux-m68k.org>
cc:     Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        Michael Ellerman <mpe@...erman.id.au>,
        Randy Dunlap <rdunlap@...radead.org>,
        linuxppc-dev <linuxppc-dev@...ts.ozlabs.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] macintosh/via-pmu: Fix build failure when CONFIG_INPUT
 is disabled

On Sat, 19 Mar 2022, Geert Uytterhoeven wrote:

> On Sat, Mar 19, 2022 at 5:23 AM Finn Thain <fthain@...ux-m68k.org> wrote:
> > drivers/macintosh/via-pmu-event.o: In function `via_pmu_event':
> > via-pmu-event.c:(.text+0x44): undefined reference to `input_event'
> > via-pmu-event.c:(.text+0x68): undefined reference to `input_event'
> > via-pmu-event.c:(.text+0x94): undefined reference to `input_event'
> > via-pmu-event.c:(.text+0xb8): undefined reference to `input_event'
> > drivers/macintosh/via-pmu-event.o: In function `via_pmu_event_init':
> > via-pmu-event.c:(.init.text+0x20): undefined reference to `input_allocate_device'
> > via-pmu-event.c:(.init.text+0xc4): undefined reference to `input_register_device'
> > via-pmu-event.c:(.init.text+0xd4): undefined reference to `input_free_device'
> > make[1]: *** [Makefile:1155: vmlinux] Error 1
> > make: *** [Makefile:350: __build_one_by_one] Error 2
> >
> > Don't call into the input subsystem unless CONFIG_INPUT is built-in.
> >
> > Reported-by: kernel test robot <lkp@...el.com>
> > Cc: Michael Ellerman <mpe@...erman.id.au>
> > Cc: Geert Uytterhoeven <geert@...ux-m68k.org>
> > Signed-off-by: Finn Thain <fthain@...ux-m68k.org>
> 
> Thanks for your patch!
> 

Thanks for your review.

> > --- a/drivers/macintosh/Makefile
> > +++ b/drivers/macintosh/Makefile
> > @@ -12,7 +12,10 @@ obj-$(CONFIG_MAC_EMUMOUSEBTN)        += mac_hid.o
> >  obj-$(CONFIG_INPUT_ADBHID)     += adbhid.o
> >  obj-$(CONFIG_ANSLCD)           += ans-lcd.o
> >
> > -obj-$(CONFIG_ADB_PMU)          += via-pmu.o via-pmu-event.o
> > +obj-$(CONFIG_ADB_PMU)          += via-pmu.o
> > +ifeq ($(CONFIG_INPUT), y)
> > +obj-$(CONFIG_ADB_PMU)          += via-pmu-event.o
> > +endif
> 
> Alternatively, you can introduce an invisible Kconfig symbol that
> is y if ADB_PMU && INPUT, to control the build of via-pmu.o.
> 

There is some precent for that (DVB_AV7110_IR, PINCTRL_FALCON, 
DVB_AV7110_IR) in recent code but it's a bit of a stretch.

Adding the new Kconfig symbol would add complexity and it would only get 
used in two places.

I appreciate the general preference for declarative style over imperative. 
But is there a stronger argument against conditionals in Makefiles?

> >  obj-$(CONFIG_ADB_PMU_LED)      += via-pmu-led.o
> >  obj-$(CONFIG_PMAC_BACKLIGHT)   += via-pmu-backlight.o
> >  obj-$(CONFIG_ADB_CUDA)         += via-cuda.o
> > diff --git a/drivers/macintosh/via-pmu.c b/drivers/macintosh/via-pmu.c
> > index 4b98bc26a94b..55afa6dfa263 100644
> > --- a/drivers/macintosh/via-pmu.c
> > +++ b/drivers/macintosh/via-pmu.c
> > @@ -1457,12 +1457,14 @@ pmu_handle_data(unsigned char *data, int len)
> >                 if (pmu_battery_count)
> >                         query_battery_state();
> >                 pmu_pass_intr(data, len);
> > +#ifdef CONFIG_INPUT
> >                 /* len == 6 is probably a bad check. But how do I
> >                  * know what PMU versions send what events here? */
> >                 if (len == 6) {
> >                         via_pmu_event(PMU_EVT_POWER, !!(data[1]&8));
> >                         via_pmu_event(PMU_EVT_LID, data[1]&1);
> >                 }
> > +#endif
> 
> Additionally, if that new symbol is not enabled, a dummy via_pmu_event()
> can be provided, so you don't need to add an #ifdef to the driver anymore.
> 

Adding a dummy function to be used in only one place seems questionable to 
me. I'm not expecting new call sites to appear outside of that ifdef.

Some of the arguments against ifdefs (reduced test and checker coverage, 
readability harm) don't really apply in this case.