linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/2] if_enabled.h: Add IF_ENABLED_OR_ELSE() and IF_ENABLED() macros
@ 2020-12-08 16:48 Paul Cercueil
  2020-12-08 16:48 ` [PATCH 2/2] pinctrl: ingenic: Only support SoCs enabled in config Paul Cercueil
                   ` (3 more replies)
  0 siblings, 4 replies; 10+ messages in thread
From: Paul Cercueil @ 2020-12-08 16:48 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Arnd Bergmann, od, linux-arm-kernel, linux-mips, linux-kernel,
	Paul Cercueil

Introduce a new header <linux/if_enabled.h>, that brings two new macros:
IF_ENABLED_OR_ELSE() and IF_ENABLED().

IF_ENABLED_OR_ELSE(CONFIG_FOO, a, b) evaluates to (a) if CONFIG_FOO is set
to 'y' or 'm', (b) otherwise. It is used internally to define the
IF_ENABLED() macro. The (a) and (b) arguments must be of the same type.

IF_ENABLED(CONFIG_FOO, ptr) evaluates to (ptr) if CONFIG_FOO is set to 'y'
or 'm', NULL otherwise. The (ptr) argument must be a pointer.

The IF_ENABLED() macro can be very useful to help GCC drop dead code.

For instance, consider the following:

    #ifdef CONFIG_FOO_SUSPEND
    static int foo_suspend(struct device *dev)
    {
       ...
    }
    #endif

    static struct pm_ops foo_ops = {
	#ifdef CONFIG_FOO_SUSPEND
        .suspend = foo_suspend,
	#endif
    };

While this works, the foo_suspend() macro is compiled conditionally,
only when CONFIG_FOO_SUSPEND is set. This is problematic, as there could
be a build bug in this function, we wouldn't have a way to know unless
the config option is set.

An alternative is to declare foo_suspend() always, but mark it as maybe
unused:

    static int __maybe_unused foo_suspend(struct device *dev)
    {
       ...
    }

    static struct pm_ops foo_ops = {
	#ifdef CONFIG_FOO_SUSPEND
        .suspend = foo_suspend,
	#endif
    };

Again, this works, but the __maybe_unused attribute is required to
instruct the compiler that the function may not be referenced anywhere,
and is safe to remove without making a fuss about it. This makes the
programmer responsible for tagging the functions that can be
garbage-collected.

With this patch, it is now possible to write the following:

    static int foo_suspend(struct device *dev)
    {
       ...
    }

    static struct pm_ops foo_ops = {
        .suspend = IF_ENABLED(CONFIG_FOO_SUSPEND, foo_suspend),
    };

The foo_suspend() function will now be automatically dropped by the
compiler, and it does not require any specific attribute.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 include/linux/if_enabled.h | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)
 create mode 100644 include/linux/if_enabled.h

diff --git a/include/linux/if_enabled.h b/include/linux/if_enabled.h
new file mode 100644
index 000000000000..8189d1402340
--- /dev/null
+++ b/include/linux/if_enabled.h
@@ -0,0 +1,22 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __LINUX_IF_ENABLED_H
+#define __LINUX_IF_ENABLED_H
+
+#include <linux/build_bug.h>
+#include <linux/compiler_types.h>
+#include <linux/kconfig.h>
+
+/*
+ * IF_ENABLED_OR_ELSE(CONFIG_FOO, a, b) evaluates to (a) if CONFIG_FOO is set
+ * to 'y' or 'm', (b) otherwise.
+ */
+#define IF_ENABLED_OR_ELSE(option, a, b) \
+	(BUILD_BUG_ON_ZERO(__same_type((a), (b))) || IS_ENABLED(option) ? (a) : (b))
+
+/*
+ * IF_ENABLED(CONFIG_FOO, ptr) evaluates to (ptr) if CONFIG_FOO is set to 'y'
+ * or 'm', NULL otherwise.
+ */
+#define IF_ENABLED(option, ptr) IF_ENABLED_OR_ELSE(option, ptr, NULL)
+
+#endif /* __LINUX_IF_ENABLED_H */
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 2/2] pinctrl: ingenic: Only support SoCs enabled in config
  2020-12-08 16:48 [PATCH 1/2] if_enabled.h: Add IF_ENABLED_OR_ELSE() and IF_ENABLED() macros Paul Cercueil
@ 2020-12-08 16:48 ` Paul Cercueil
  2020-12-09 10:13   ` Daniel Palmer
  2020-12-08 17:39 ` [PATCH 1/2] if_enabled.h: Add IF_ENABLED_OR_ELSE() and IF_ENABLED() macros Randy Dunlap
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 10+ messages in thread
From: Paul Cercueil @ 2020-12-08 16:48 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Arnd Bergmann, od, linux-arm-kernel, linux-mips, linux-kernel,
	Paul Cercueil

Tested on a JZ4740 system (ARCH=mips make qi_lb60_defconfig), this saves
about 14 KiB, by allowing the compiler to garbage-collect all the
functions and tables that correspond to SoCs that were disabled in the
config.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 drivers/pinctrl/pinctrl-ingenic.c | 61 +++++++++++++++++++++++++------
 1 file changed, 49 insertions(+), 12 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-ingenic.c b/drivers/pinctrl/pinctrl-ingenic.c
index a14938a7cc30..11f1bc90632d 100644
--- a/drivers/pinctrl/pinctrl-ingenic.c
+++ b/drivers/pinctrl/pinctrl-ingenic.c
@@ -9,6 +9,7 @@
 
 #include <linux/compiler.h>
 #include <linux/gpio/driver.h>
+#include <linux/if_enabled.h>
 #include <linux/interrupt.h>
 #include <linux/io.h>
 #include <linux/of_device.h>
@@ -2384,6 +2385,12 @@ static int __init ingenic_pinctrl_probe(struct platform_device *pdev)
 	unsigned int i;
 	int err;
 
+	chip_info = of_device_get_match_data(dev);
+	if (!chip_info) {
+		dev_err(dev, "Unsupported SoC\n");
+		return -EINVAL;
+	}
+
 	jzpc = devm_kzalloc(dev, sizeof(*jzpc), GFP_KERNEL);
 	if (!jzpc)
 		return -ENOMEM;
@@ -2400,7 +2407,7 @@ static int __init ingenic_pinctrl_probe(struct platform_device *pdev)
 	}
 
 	jzpc->dev = dev;
-	jzpc->info = chip_info = of_device_get_match_data(dev);
+	jzpc->info = chip_info;
 
 	pctl_desc = devm_kzalloc(&pdev->dev, sizeof(*pctl_desc), GFP_KERNEL);
 	if (!pctl_desc)
@@ -2470,17 +2477,47 @@ static int __init ingenic_pinctrl_probe(struct platform_device *pdev)
 }
 
 static const struct of_device_id ingenic_pinctrl_of_match[] = {
-	{ .compatible = "ingenic,jz4740-pinctrl", .data = &jz4740_chip_info },
-	{ .compatible = "ingenic,jz4725b-pinctrl", .data = &jz4725b_chip_info },
-	{ .compatible = "ingenic,jz4760-pinctrl", .data = &jz4760_chip_info },
-	{ .compatible = "ingenic,jz4760b-pinctrl", .data = &jz4760_chip_info },
-	{ .compatible = "ingenic,jz4770-pinctrl", .data = &jz4770_chip_info },
-	{ .compatible = "ingenic,jz4780-pinctrl", .data = &jz4780_chip_info },
-	{ .compatible = "ingenic,x1000-pinctrl", .data = &x1000_chip_info },
-	{ .compatible = "ingenic,x1000e-pinctrl", .data = &x1000_chip_info },
-	{ .compatible = "ingenic,x1500-pinctrl", .data = &x1500_chip_info },
-	{ .compatible = "ingenic,x1830-pinctrl", .data = &x1830_chip_info },
-	{},
+	{
+		.compatible = "ingenic,jz4740-pinctrl",
+		.data = IF_ENABLED(CONFIG_MACH_JZ4740, &jz4740_chip_info)
+	},
+	{
+		.compatible = "ingenic,jz4725b-pinctrl",
+		.data = IF_ENABLED(CONFIG_MACH_JZ4725B, &jz4725b_chip_info)
+	},
+	{
+		.compatible = "ingenic,jz4760-pinctrl",
+		.data = IF_ENABLED(CONFIG_MACH_JZ4760, &jz4760_chip_info)
+	},
+	{
+		.compatible = "ingenic,jz4760b-pinctrl",
+		.data = IF_ENABLED(CONFIG_MACH_JZ4760, &jz4760_chip_info)
+	},
+	{
+		.compatible = "ingenic,jz4770-pinctrl",
+		.data = IF_ENABLED(CONFIG_MACH_JZ4770, &jz4770_chip_info)
+	},
+	{
+		.compatible = "ingenic,jz4780-pinctrl",
+		.data = IF_ENABLED(CONFIG_MACH_JZ4780, &jz4780_chip_info)
+	},
+	{
+		.compatible = "ingenic,x1000-pinctrl",
+		.data = IF_ENABLED(CONFIG_MACH_X1000, &x1000_chip_info)
+	},
+	{
+		.compatible = "ingenic,x1000e-pinctrl",
+		.data = IF_ENABLED(CONFIG_MACH_X1000, &x1000_chip_info)
+	},
+	{
+		.compatible = "ingenic,x1500-pinctrl",
+		.data = IF_ENABLED(CONFIG_MACH_X1500, &x1500_chip_info)
+	},
+	{
+		.compatible = "ingenic,x1830-pinctrl",
+		.data = IF_ENABLED(CONFIG_MACH_X1830, &x1830_chip_info)
+	},
+	{ /* sentinel */ },
 };
 
 static struct platform_driver ingenic_pinctrl_driver = {
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] if_enabled.h: Add IF_ENABLED_OR_ELSE() and IF_ENABLED() macros
  2020-12-08 16:48 [PATCH 1/2] if_enabled.h: Add IF_ENABLED_OR_ELSE() and IF_ENABLED() macros Paul Cercueil
  2020-12-08 16:48 ` [PATCH 2/2] pinctrl: ingenic: Only support SoCs enabled in config Paul Cercueil
@ 2020-12-08 17:39 ` Randy Dunlap
  2020-12-08 19:00   ` Paul Cercueil
  2020-12-09  8:59 ` Linus Walleij
  2020-12-09  9:38 ` Ard Biesheuvel
  3 siblings, 1 reply; 10+ messages in thread
From: Randy Dunlap @ 2020-12-08 17:39 UTC (permalink / raw)
  To: Paul Cercueil, Linus Walleij
  Cc: Arnd Bergmann, od, linux-arm-kernel, linux-mips, linux-kernel

On 12/8/20 8:48 AM, Paul Cercueil wrote:
> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>

Hi Paul,

Why not just add these 2 new macros to <linux/kconfig.h> ?

Maybe you don't want to add the other 2 headers there also?

> ---
>  include/linux/if_enabled.h | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
>  create mode 100644 include/linux/if_enabled.h
> 
> diff --git a/include/linux/if_enabled.h b/include/linux/if_enabled.h
> new file mode 100644
> index 000000000000..8189d1402340
> --- /dev/null
> +++ b/include/linux/if_enabled.h
> @@ -0,0 +1,22 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __LINUX_IF_ENABLED_H
> +#define __LINUX_IF_ENABLED_H
> +
> +#include <linux/build_bug.h>
> +#include <linux/compiler_types.h>
> +#include <linux/kconfig.h>
> +
> +/*
> + * IF_ENABLED_OR_ELSE(CONFIG_FOO, a, b) evaluates to (a) if CONFIG_FOO is set
> + * to 'y' or 'm', (b) otherwise.
> + */
> +#define IF_ENABLED_OR_ELSE(option, a, b) \
> +	(BUILD_BUG_ON_ZERO(__same_type((a), (b))) || IS_ENABLED(option) ? (a) : (b))
> +
> +/*
> + * IF_ENABLED(CONFIG_FOO, ptr) evaluates to (ptr) if CONFIG_FOO is set to 'y'
> + * or 'm', NULL otherwise.
> + */
> +#define IF_ENABLED(option, ptr) IF_ENABLED_OR_ELSE(option, ptr, NULL)
> +
> +#endif /* __LINUX_IF_ENABLED_H */
> 


thanks.
-- 
~Randy


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] if_enabled.h: Add IF_ENABLED_OR_ELSE() and  IF_ENABLED() macros
  2020-12-08 17:39 ` [PATCH 1/2] if_enabled.h: Add IF_ENABLED_OR_ELSE() and IF_ENABLED() macros Randy Dunlap
@ 2020-12-08 19:00   ` Paul Cercueil
  0 siblings, 0 replies; 10+ messages in thread
From: Paul Cercueil @ 2020-12-08 19:00 UTC (permalink / raw)
  To: Randy Dunlap
  Cc: Linus Walleij, Arnd Bergmann, od, linux-arm-kernel, linux-mips,
	linux-kernel

Hi Randy,

Le mar. 8 déc. 2020 à 9:39, Randy Dunlap <rdunlap@infradead.org> a 
écrit :
> On 12/8/20 8:48 AM, Paul Cercueil wrote:
>> 
>>  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> 
> Hi Paul,
> 
> Why not just add these 2 new macros to <linux/kconfig.h> ?
> 
> Maybe you don't want to add the other 2 headers there also?

That means including <linux/compiler.h> in <linux/kconfig.h>, which 
causes build failures:

  LD      vmlinux
  SORTTAB vmlinux
  SYSMAP  System.map
  AS      arch/mips/boot/compressed/head.o
  CC      arch/mips/boot/compressed/decompress.o
  CC      arch/mips/boot/compressed/string.o
  CC      arch/mips/boot/compressed/dummy.o
  OBJCOPY arch/mips/boot/compressed/vmlinux.bin
  HOSTCC  arch/mips/boot/compressed/calc_vmlinuz_load_addr
  GZIP    arch/mips/boot/compressed/vmlinux.bin.z
In file included from ./include/linux/kcsan-checks.h:7,
                 from ./include/asm-generic/rwonce.h:27,
                 from ./arch/mips/include/generated/asm/rwonce.h:1,
                 from ./include/linux/compiler.h:246,
                 from ././include/linux/kconfig.h:8,
                 from <command-line>:32:
/include/linux/compiler_attributes.h:64: warning: "__always_inline" 
redefined
   64 | #define __always_inline                 inline 
__attribute__((__always_inline__))
      |
In file included from ./include/linux/stddef.h:5,
                 from ./include/uapi/linux/posix_types.h:5,
                 from ./include/uapi/linux/types.h:14,
                 from ./include/linux/types.h:6,
                 from ./include/linux/kasan-checks.h:5,
                 from ./include/asm-generic/rwonce.h:26,
                 from ./arch/mips/include/generated/asm/rwonce.h:1,
                 from ./include/linux/compiler.h:246,
                 from ././include/linux/kconfig.h:8,
                 from <command-line>:32:
/include/uapi/linux/stddef.h:5: note: this is the location of the 
previous definition
    5 | #define __always_inline inline
      |
In file included from ./arch/mips/include/generated/asm/rwonce.h:1,
                 from ./include/linux/compiler.h:246,
                 from ././include/linux/kconfig.h:8,
                 from <command-line>:32:
/include/asm-generic/rwonce.h:64:31: error: expected ‘;’ before 
‘unsigned’
   64 | static __no_sanitize_or_inline
      |                               ^
      |                               ;
   65 | unsigned long __read_once_word_nocheck(const void *addr)
      | ~~~~~~~~
/include/asm-generic/rwonce.h:65:15: warning: no previous prototype 
for ‘__read_once_word_nocheck’ [-Wmissing-prototypes]
   65 | unsigned long __read_once_word_nocheck(const void *addr)
      |               ^~~~~~~~~~~~~~~~~~~~~~~~
/include/asm-generic/rwonce.h:82:28: error: expected ‘;’ before 
‘unsigned’
   82 | static __no_kasan_or_inline
      |                            ^
      |                            ;
   83 | unsigned long read_word_at_a_time(const void *addr)
      | ~~~~~~~~
/include/asm-generic/rwonce.h:83:15: warning: no previous prototype 
for ‘read_word_at_a_time’ [-Wmissing-prototypes]
   83 | unsigned long read_word_at_a_time(const void *addr)
      |               ^~~~~~~~~~~~~~~~~~~


That's why I opted for a new header.

Cheers,
-Paul

>>  ---
>>   include/linux/if_enabled.h | 22 ++++++++++++++++++++++
>>   1 file changed, 22 insertions(+)
>>   create mode 100644 include/linux/if_enabled.h
>> 
>>  diff --git a/include/linux/if_enabled.h b/include/linux/if_enabled.h
>>  new file mode 100644
>>  index 000000000000..8189d1402340
>>  --- /dev/null
>>  +++ b/include/linux/if_enabled.h
>>  @@ -0,0 +1,22 @@
>>  +/* SPDX-License-Identifier: GPL-2.0 */
>>  +#ifndef __LINUX_IF_ENABLED_H
>>  +#define __LINUX_IF_ENABLED_H
>>  +
>>  +#include <linux/build_bug.h>
>>  +#include <linux/compiler_types.h>
>>  +#include <linux/kconfig.h>
>>  +
>>  +/*
>>  + * IF_ENABLED_OR_ELSE(CONFIG_FOO, a, b) evaluates to (a) if 
>> CONFIG_FOO is set
>>  + * to 'y' or 'm', (b) otherwise.
>>  + */
>>  +#define IF_ENABLED_OR_ELSE(option, a, b) \
>>  +	(BUILD_BUG_ON_ZERO(__same_type((a), (b))) || IS_ENABLED(option) ? 
>> (a) : (b))
>>  +
>>  +/*
>>  + * IF_ENABLED(CONFIG_FOO, ptr) evaluates to (ptr) if CONFIG_FOO is 
>> set to 'y'
>>  + * or 'm', NULL otherwise.
>>  + */
>>  +#define IF_ENABLED(option, ptr) IF_ENABLED_OR_ELSE(option, ptr, 
>> NULL)
>>  +
>>  +#endif /* __LINUX_IF_ENABLED_H */
>> 
> 
> 
> thanks.
> --
> ~Randy
> 



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] if_enabled.h: Add IF_ENABLED_OR_ELSE() and IF_ENABLED() macros
  2020-12-08 16:48 [PATCH 1/2] if_enabled.h: Add IF_ENABLED_OR_ELSE() and IF_ENABLED() macros Paul Cercueil
  2020-12-08 16:48 ` [PATCH 2/2] pinctrl: ingenic: Only support SoCs enabled in config Paul Cercueil
  2020-12-08 17:39 ` [PATCH 1/2] if_enabled.h: Add IF_ENABLED_OR_ELSE() and IF_ENABLED() macros Randy Dunlap
@ 2020-12-09  8:59 ` Linus Walleij
  2020-12-09 11:31   ` Paul Cercueil
  2020-12-09  9:38 ` Ard Biesheuvel
  3 siblings, 1 reply; 10+ messages in thread
From: Linus Walleij @ 2020-12-09  8:59 UTC (permalink / raw)
  To: Paul Cercueil; +Cc: Arnd Bergmann, od, Linux ARM, linux-mips, linux-kernel

On Tue, Dec 8, 2020 at 5:48 PM Paul Cercueil <paul@crapouillou.net> wrote:

> Introduce a new header <linux/if_enabled.h>, that brings two new macros:
> IF_ENABLED_OR_ELSE() and IF_ENABLED().

I understand what the patch is trying to do, but when we already have
IS_ENABLED() in <linux/kconfig.h> this syntax becomes a big cognitive
confusion for the mind.

At least the commit needs to explain why it doesn't work to use
IS_ENABLED() instead so that this is needed.

Certainly the build failures must be possible to solve so that this
can live with the sibling IS_ENABLED() inside <linux/kconfig.h>,
it can't be too hard.

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] if_enabled.h: Add IF_ENABLED_OR_ELSE() and IF_ENABLED() macros
  2020-12-08 16:48 [PATCH 1/2] if_enabled.h: Add IF_ENABLED_OR_ELSE() and IF_ENABLED() macros Paul Cercueil
                   ` (2 preceding siblings ...)
  2020-12-09  8:59 ` Linus Walleij
@ 2020-12-09  9:38 ` Ard Biesheuvel
  2020-12-09 11:27   ` Paul Cercueil
  3 siblings, 1 reply; 10+ messages in thread
From: Ard Biesheuvel @ 2020-12-09  9:38 UTC (permalink / raw)
  To: Paul Cercueil
  Cc: Linus Walleij, Arnd Bergmann, open list:MIPS,
	Linux Kernel Mailing List, od, Linux ARM

On Tue, 8 Dec 2020 at 17:49, Paul Cercueil <paul@crapouillou.net> wrote:
>
> Introduce a new header <linux/if_enabled.h>, that brings two new macros:
> IF_ENABLED_OR_ELSE() and IF_ENABLED().
>
> IF_ENABLED_OR_ELSE(CONFIG_FOO, a, b) evaluates to (a) if CONFIG_FOO is set
> to 'y' or 'm', (b) otherwise. It is used internally to define the
> IF_ENABLED() macro. The (a) and (b) arguments must be of the same type.
>
> IF_ENABLED(CONFIG_FOO, ptr) evaluates to (ptr) if CONFIG_FOO is set to 'y'
> or 'm', NULL otherwise. The (ptr) argument must be a pointer.
>
> The IF_ENABLED() macro can be very useful to help GCC drop dead code.
>
> For instance, consider the following:
>
>     #ifdef CONFIG_FOO_SUSPEND
>     static int foo_suspend(struct device *dev)
>     {
>        ...
>     }
>     #endif
>
>     static struct pm_ops foo_ops = {
>         #ifdef CONFIG_FOO_SUSPEND
>         .suspend = foo_suspend,
>         #endif
>     };
>
> While this works, the foo_suspend() macro is compiled conditionally,
> only when CONFIG_FOO_SUSPEND is set. This is problematic, as there could
> be a build bug in this function, we wouldn't have a way to know unless
> the config option is set.
>
> An alternative is to declare foo_suspend() always, but mark it as maybe
> unused:
>
>     static int __maybe_unused foo_suspend(struct device *dev)
>     {
>        ...
>     }
>
>     static struct pm_ops foo_ops = {
>         #ifdef CONFIG_FOO_SUSPEND
>         .suspend = foo_suspend,
>         #endif
>     };
>
> Again, this works, but the __maybe_unused attribute is required to
> instruct the compiler that the function may not be referenced anywhere,
> and is safe to remove without making a fuss about it. This makes the
> programmer responsible for tagging the functions that can be
> garbage-collected.
>
> With this patch, it is now possible to write the following:
>
>     static int foo_suspend(struct device *dev)
>     {
>        ...
>     }
>
>     static struct pm_ops foo_ops = {
>         .suspend = IF_ENABLED(CONFIG_FOO_SUSPEND, foo_suspend),
>     };
>
> The foo_suspend() function will now be automatically dropped by the
> compiler, and it does not require any specific attribute.
>
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>

Hi Paul,

I understand the issue you are trying to address here, but please note
that there are many cases where the struct member in question will not
even be declared if the associated CONFIG option is not set, in which
case this will cause a compile error.










> ---
>  include/linux/if_enabled.h | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
>  create mode 100644 include/linux/if_enabled.h
>
> diff --git a/include/linux/if_enabled.h b/include/linux/if_enabled.h
> new file mode 100644
> index 000000000000..8189d1402340
> --- /dev/null
> +++ b/include/linux/if_enabled.h
> @@ -0,0 +1,22 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __LINUX_IF_ENABLED_H
> +#define __LINUX_IF_ENABLED_H
> +
> +#include <linux/build_bug.h>
> +#include <linux/compiler_types.h>
> +#include <linux/kconfig.h>
> +
> +/*
> + * IF_ENABLED_OR_ELSE(CONFIG_FOO, a, b) evaluates to (a) if CONFIG_FOO is set
> + * to 'y' or 'm', (b) otherwise.
> + */
> +#define IF_ENABLED_OR_ELSE(option, a, b) \
> +       (BUILD_BUG_ON_ZERO(__same_type((a), (b))) || IS_ENABLED(option) ? (a) : (b))
> +
> +/*
> + * IF_ENABLED(CONFIG_FOO, ptr) evaluates to (ptr) if CONFIG_FOO is set to 'y'
> + * or 'm', NULL otherwise.
> + */
> +#define IF_ENABLED(option, ptr) IF_ENABLED_OR_ELSE(option, ptr, NULL)
> +
> +#endif /* __LINUX_IF_ENABLED_H */
> --
> 2.29.2
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] pinctrl: ingenic: Only support SoCs enabled in config
  2020-12-08 16:48 ` [PATCH 2/2] pinctrl: ingenic: Only support SoCs enabled in config Paul Cercueil
@ 2020-12-09 10:13   ` Daniel Palmer
  2020-12-09 11:04     ` Arnd Bergmann
  0 siblings, 1 reply; 10+ messages in thread
From: Daniel Palmer @ 2020-12-09 10:13 UTC (permalink / raw)
  To: Paul Cercueil
  Cc: Linus Walleij, Arnd Bergmann, od, linux-arm-kernel, linux-mips,
	Linux Kernel Mailing List

Hi Paul and others,

Sorry to hijack this but I actually want to do something similar to
this in some other drivers.
The targets I'm working with have only 64MB of ram so I want to remove
code wherever possible.
Is there any reason to do it like this instead of wrapping the whole
unneeded of_device_id struct in an #ifdef?
For example there is a rule that the compatible strings have to be
present even if the driver isn't usable or something?

Thanks,

Daniel

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] pinctrl: ingenic: Only support SoCs enabled in config
  2020-12-09 10:13   ` Daniel Palmer
@ 2020-12-09 11:04     ` Arnd Bergmann
  0 siblings, 0 replies; 10+ messages in thread
From: Arnd Bergmann @ 2020-12-09 11:04 UTC (permalink / raw)
  To: Daniel Palmer
  Cc: Paul Cercueil, Linus Walleij, od, linux-arm-kernel,
	open list:BROADCOM NVRAM DRIVER, Linux Kernel Mailing List

On Wed, Dec 9, 2020 at 11:13 AM Daniel Palmer <daniel@0x0f.com> wrote:
>
> Hi Paul and others,
>
> Sorry to hijack this but I actually want to do something similar to
> this in some other drivers.
> The targets I'm working with have only 64MB of ram so I want to remove
> code wherever possible.
> Is there any reason to do it like this instead of wrapping the whole
> unneeded of_device_id struct in an #ifdef?
> For example there is a rule that the compatible strings have to be
> present even if the driver isn't usable or something?

No, there is no such rule, but adding lots of #ifdef checks in this
file would be much less readable and more error-prone, as you'd
have to make sure the two #ifdef blocks around the structure
match the one for the ID table, and any function that is called
by more than one SoC has the correct combination of A || B || D
checks, and nobody ever gets that right.

      Arnd

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] if_enabled.h: Add IF_ENABLED_OR_ELSE() and  IF_ENABLED() macros
  2020-12-09  9:38 ` Ard Biesheuvel
@ 2020-12-09 11:27   ` Paul Cercueil
  0 siblings, 0 replies; 10+ messages in thread
From: Paul Cercueil @ 2020-12-09 11:27 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: Linus Walleij, Arnd Bergmann, MIPS, Linux Kernel Mailing List,
	od, Linux ARM

Hi Ard,

Le mer. 9 déc. 2020 à 10:38, Ard Biesheuvel <ardb@kernel.org> a 
écrit :
> On Tue, 8 Dec 2020 at 17:49, Paul Cercueil <paul@crapouillou.net> 
> wrote:
>> 
>>  Introduce a new header <linux/if_enabled.h>, that brings two new 
>> macros:
>>  IF_ENABLED_OR_ELSE() and IF_ENABLED().
>> 
>>  IF_ENABLED_OR_ELSE(CONFIG_FOO, a, b) evaluates to (a) if CONFIG_FOO 
>> is set
>>  to 'y' or 'm', (b) otherwise. It is used internally to define the
>>  IF_ENABLED() macro. The (a) and (b) arguments must be of the same 
>> type.
>> 
>>  IF_ENABLED(CONFIG_FOO, ptr) evaluates to (ptr) if CONFIG_FOO is set 
>> to 'y'
>>  or 'm', NULL otherwise. The (ptr) argument must be a pointer.
>> 
>>  The IF_ENABLED() macro can be very useful to help GCC drop dead 
>> code.
>> 
>>  For instance, consider the following:
>> 
>>      #ifdef CONFIG_FOO_SUSPEND
>>      static int foo_suspend(struct device *dev)
>>      {
>>         ...
>>      }
>>      #endif
>> 
>>      static struct pm_ops foo_ops = {
>>          #ifdef CONFIG_FOO_SUSPEND
>>          .suspend = foo_suspend,
>>          #endif
>>      };
>> 
>>  While this works, the foo_suspend() macro is compiled conditionally,
>>  only when CONFIG_FOO_SUSPEND is set. This is problematic, as there 
>> could
>>  be a build bug in this function, we wouldn't have a way to know 
>> unless
>>  the config option is set.
>> 
>>  An alternative is to declare foo_suspend() always, but mark it as 
>> maybe
>>  unused:
>> 
>>      static int __maybe_unused foo_suspend(struct device *dev)
>>      {
>>         ...
>>      }
>> 
>>      static struct pm_ops foo_ops = {
>>          #ifdef CONFIG_FOO_SUSPEND
>>          .suspend = foo_suspend,
>>          #endif
>>      };
>> 
>>  Again, this works, but the __maybe_unused attribute is required to
>>  instruct the compiler that the function may not be referenced 
>> anywhere,
>>  and is safe to remove without making a fuss about it. This makes the
>>  programmer responsible for tagging the functions that can be
>>  garbage-collected.
>> 
>>  With this patch, it is now possible to write the following:
>> 
>>      static int foo_suspend(struct device *dev)
>>      {
>>         ...
>>      }
>> 
>>      static struct pm_ops foo_ops = {
>>          .suspend = IF_ENABLED(CONFIG_FOO_SUSPEND, foo_suspend),
>>      };
>> 
>>  The foo_suspend() function will now be automatically dropped by the
>>  compiler, and it does not require any specific attribute.
>> 
>>  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> 
> Hi Paul,
> 
> I understand the issue you are trying to address here, but please note
> that there are many cases where the struct member in question will not
> even be declared if the associated CONFIG option is not set, in which
> case this will cause a compile error.

Of course. This is for the case where the field is always present. For 
instance, (struct device_driver *)->pm is always present, independently 
of CONFIG_PM.

Cheers,
-Paul

> 
>>  ---
>>   include/linux/if_enabled.h | 22 ++++++++++++++++++++++
>>   1 file changed, 22 insertions(+)
>>   create mode 100644 include/linux/if_enabled.h
>> 
>>  diff --git a/include/linux/if_enabled.h b/include/linux/if_enabled.h
>>  new file mode 100644
>>  index 000000000000..8189d1402340
>>  --- /dev/null
>>  +++ b/include/linux/if_enabled.h
>>  @@ -0,0 +1,22 @@
>>  +/* SPDX-License-Identifier: GPL-2.0 */
>>  +#ifndef __LINUX_IF_ENABLED_H
>>  +#define __LINUX_IF_ENABLED_H
>>  +
>>  +#include <linux/build_bug.h>
>>  +#include <linux/compiler_types.h>
>>  +#include <linux/kconfig.h>
>>  +
>>  +/*
>>  + * IF_ENABLED_OR_ELSE(CONFIG_FOO, a, b) evaluates to (a) if 
>> CONFIG_FOO is set
>>  + * to 'y' or 'm', (b) otherwise.
>>  + */
>>  +#define IF_ENABLED_OR_ELSE(option, a, b) \
>>  +       (BUILD_BUG_ON_ZERO(__same_type((a), (b))) || 
>> IS_ENABLED(option) ? (a) : (b))
>>  +
>>  +/*
>>  + * IF_ENABLED(CONFIG_FOO, ptr) evaluates to (ptr) if CONFIG_FOO is 
>> set to 'y'
>>  + * or 'm', NULL otherwise.
>>  + */
>>  +#define IF_ENABLED(option, ptr) IF_ENABLED_OR_ELSE(option, ptr, 
>> NULL)
>>  +
>>  +#endif /* __LINUX_IF_ENABLED_H */
>>  --
>>  2.29.2
>> 
>> 
>>  _______________________________________________
>>  linux-arm-kernel mailing list
>>  linux-arm-kernel@lists.infradead.org
>>  http://lists.infradead.org/mailman/listinfo/linux-arm-kernel



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] if_enabled.h: Add IF_ENABLED_OR_ELSE() and  IF_ENABLED() macros
  2020-12-09  8:59 ` Linus Walleij
@ 2020-12-09 11:31   ` Paul Cercueil
  0 siblings, 0 replies; 10+ messages in thread
From: Paul Cercueil @ 2020-12-09 11:31 UTC (permalink / raw)
  To: Linus Walleij; +Cc: Arnd Bergmann, od, Linux ARM, linux-mips, linux-kernel

Hi Linus,

Le mer. 9 déc. 2020 à 9:59, Linus Walleij <linus.walleij@linaro.org> 
a écrit :
> On Tue, Dec 8, 2020 at 5:48 PM Paul Cercueil <paul@crapouillou.net> 
> wrote:
> 
>>  Introduce a new header <linux/if_enabled.h>, that brings two new 
>> macros:
>>  IF_ENABLED_OR_ELSE() and IF_ENABLED().
> 
> I understand what the patch is trying to do, but when we already have
> IS_ENABLED() in <linux/kconfig.h> this syntax becomes a big cognitive
> confusion for the mind.
> 
> At least the commit needs to explain why it doesn't work to use
> IS_ENABLED() instead so that this is needed.

You can use IS_ENABLED(). Then you'd write:

field = IS_ENABLED(CONFIG_FOO) ? &my_ptr : NULL,

the IF_ENABLED() macro makes it a bit cleaner by allowing you to write:

field = IF_ENABLED(CONFIG_FOO, &my_ptr),

Cheers,
-Paul

> Certainly the build failures must be possible to solve so that this
> can live with the sibling IS_ENABLED() inside <linux/kconfig.h>,
> it can't be too hard.
> 
> Yours,
> Linus Walleij



^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2020-12-09 11:32 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-08 16:48 [PATCH 1/2] if_enabled.h: Add IF_ENABLED_OR_ELSE() and IF_ENABLED() macros Paul Cercueil
2020-12-08 16:48 ` [PATCH 2/2] pinctrl: ingenic: Only support SoCs enabled in config Paul Cercueil
2020-12-09 10:13   ` Daniel Palmer
2020-12-09 11:04     ` Arnd Bergmann
2020-12-08 17:39 ` [PATCH 1/2] if_enabled.h: Add IF_ENABLED_OR_ELSE() and IF_ENABLED() macros Randy Dunlap
2020-12-08 19:00   ` Paul Cercueil
2020-12-09  8:59 ` Linus Walleij
2020-12-09 11:31   ` Paul Cercueil
2020-12-09  9:38 ` Ard Biesheuvel
2020-12-09 11:27   ` Paul Cercueil

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).