| Message ID | 20250225145332.1116557-1-arnd@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] Input: stmpe-ts - mark OF related data as maybe unused | expand |
On Tue, Feb 25, 2025 at 03:53:26PM +0100, Arnd Bergmann wrote: > From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > > When compile tested with W=1 on x86_64 with driver as built-in: > > stmpe-ts.c:371:34: error: unused variable 'stmpe_ts_ids' [-Werror,-Wunused-const-variable] > > Ideally this would be referenced from the platform_driver, but since > the compatible string is already matched by the mfd driver for its > parent device, that would break probing. > > In this case, the of_device_id table just serves as a module alias > for loading the driver, while the device itself is probed using > the platform device name. > > Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > Link: https://lore.kernel.org/lkml/20240403080702.3509288-8-arnd@kernel.org/ > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > drivers/input/touchscreen/stmpe-ts.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/input/touchscreen/stmpe-ts.c b/drivers/input/touchscreen/stmpe-ts.c > index a94a1997f96b..3900aa2e3a90 100644 > --- a/drivers/input/touchscreen/stmpe-ts.c > +++ b/drivers/input/touchscreen/stmpe-ts.c > @@ -366,7 +366,7 @@ static struct platform_driver stmpe_ts_driver = { > }; > module_platform_driver(stmpe_ts_driver); > > -static const struct of_device_id stmpe_ts_ids[] = { > +static const struct of_device_id stmpe_ts_ids[] __maybe_unused = { > { .compatible = "st,stmpe-ts", }, > { }, > }; Following this we have MODULE_DEVICE_TABLE(of, stmpe_ts_ids); . With diff --git a/include/linux/module.h b/include/linux/module.h index 30e5b19bafa9..014f033ef1ba 100644 --- a/include/linux/module.h +++ b/include/linux/module.h @@ -250,7 +250,8 @@ extern void cleanup_module(void); extern typeof(name) __mod_device_table__##type##__##name \ __attribute__ ((unused, alias(__stringify(name)))) #else /* !MODULE */ -#define MODULE_DEVICE_TABLE(type, name) +#define MODULE_DEVICE_TABLE(type, name) \ +static const typeof(name) *__mod_device_table__##type##__##name##_ptr __attribute__((unused)) = &(name) #endif /* Version of form [<epoch>:]<version>[-<extra-version>]. the warning goes away and stmpe_ts_ids isn't included in the .o file without having to add __maybe_unused to the driver. I would consider that a superior approach. Best regards Uwe
On Tue, Feb 25, 2025, at 16:47, Uwe Kleine-König wrote: > On Tue, Feb 25, 2025 at 03:53:26PM +0100, Arnd Bergmann wrote: >> diff --git a/drivers/input/touchscreen/stmpe-ts.c b/drivers/input/touchscreen/ > > With > > diff --git a/include/linux/module.h b/include/linux/module.h > index 30e5b19bafa9..014f033ef1ba 100644 > --- a/include/linux/module.h > +++ b/include/linux/module.h > @@ -250,7 +250,8 @@ extern void cleanup_module(void); > extern typeof(name) __mod_device_table__##type##__##name \ > __attribute__ ((unused, alias(__stringify(name)))) > #else /* !MODULE */ > -#define MODULE_DEVICE_TABLE(type, name) > +#define MODULE_DEVICE_TABLE(type, name) \ > +static const typeof(name) *__mod_device_table__##type##__##name##_ptr > __attribute__((unused)) = &(name) > #endif > > /* Version of form [<epoch>:]<version>[-<extra-version>]. > > the warning goes away and stmpe_ts_ids isn't included in the .o file > without having to add __maybe_unused to the driver. > > I would consider that a superior approach. Not sure, I can see how this avoids some warnings, but this is currently the only remaining instance of this problem (I fixed another two recently), and in most cases a MODULE_DEVICE_TABLE() entry that is completely unused ends up pointing to a real bug, where there is a table but it's not also part of the device_driver definition. Arnd
Hello Arnd, On Tue, Feb 25, 2025 at 05:25:05PM +0100, Arnd Bergmann wrote: > On Tue, Feb 25, 2025, at 16:47, Uwe Kleine-König wrote: > > On Tue, Feb 25, 2025 at 03:53:26PM +0100, Arnd Bergmann wrote: > >> diff --git a/drivers/input/touchscreen/stmpe-ts.c b/drivers/input/touchscreen/ > > > > With > > > > diff --git a/include/linux/module.h b/include/linux/module.h > > index 30e5b19bafa9..014f033ef1ba 100644 > > --- a/include/linux/module.h > > +++ b/include/linux/module.h > > @@ -250,7 +250,8 @@ extern void cleanup_module(void); > > extern typeof(name) __mod_device_table__##type##__##name \ > > __attribute__ ((unused, alias(__stringify(name)))) > > #else /* !MODULE */ > > -#define MODULE_DEVICE_TABLE(type, name) > > +#define MODULE_DEVICE_TABLE(type, name) \ > > +static const typeof(name) *__mod_device_table__##type##__##name##_ptr > > __attribute__((unused)) = &(name) > > #endif > > > > /* Version of form [<epoch>:]<version>[-<extra-version>]. Hu? > > the warning goes away and stmpe_ts_ids isn't included in the .o file > > without having to add __maybe_unused to the driver. > > > > I would consider that a superior approach. > > Not sure, I can see how this avoids some warnings, but this is > currently the only remaining instance of this problem (I fixed > another two recently), and in most cases a MODULE_DEVICE_TABLE() > entry that is completely unused ends up pointing to a real bug, > where there is a table but it's not also part of the > device_driver definition. It might be the only instance without __maybe_unused and so triggering a warning. But there is also: $ git grep -E 'of_device_id.*__maybe_unused' | wc -l 231 $ git grep -E 'mdio_device_id.*__maybe_unused' | wc -l 58 Best regards Uwe
On Tue, Feb 25, 2025, at 21:17, Uwe Kleine-König wrote: > On Tue, Feb 25, 2025 at 05:25:05PM +0100, Arnd Bergmann wrote: >> On Tue, Feb 25, 2025, at 16:47, Uwe Kleine-König wrote: >> > On Tue, Feb 25, 2025 at 03:53:26PM +0100, Arnd Bergmann wrote: >> > the warning goes away and stmpe_ts_ids isn't included in the .o file >> > without having to add __maybe_unused to the driver. >> > >> > I would consider that a superior approach. >> >> Not sure, I can see how this avoids some warnings, but this is >> currently the only remaining instance of this problem (I fixed >> another two recently), and in most cases a MODULE_DEVICE_TABLE() >> entry that is completely unused ends up pointing to a real bug, >> where there is a table but it's not also part of the >> device_driver definition. > > It might be the only instance without __maybe_unused and so triggering a > warning. But there is also: > > $ git grep -E 'of_device_id.*__maybe_unused' | wc -l > 231 > > $ git grep -E 'mdio_device_id.*__maybe_unused' | wc -l > 58 I'm not really worried about these at the moment, other than not wanting to pile on to that mess with more __maybe_unused annotations. My goal here is to get the point of enabling -Wunused-const-variable by default in order to find other bugs before they make it into the kernel. Andy Shevchenko really wants to remove the of_match_ptr() macro so we can stop adding pointless __maybe_unused annotations for every driver that accidentally uses of_match_ptr(). This is certainly a good idea as well, just not what I'm trying to do this time. Apparently we have already accumulated a bunch of drivers that ended up with __maybe_unused but no actual reference from of_match_ptr(): $ git grep -l 'of_device_id.*__maybe_unused' |xargs grep -Lw of_match_ptr | wc -l but only a couple of drivers that don't use of_match_ptr() or of_match_node(): $ git grep -l 'of_device_id.*__maybe_unused' |xargs grep -Lw 'of_match_table\|of_match_node' drivers/cpufreq/armada-37xx-cpufreq.c drivers/cpufreq/armada-8k-cpufreq.c drivers/cpufreq/highbank-cpufreq.c drivers/cpufreq/sti-cpufreq.c drivers/hwmon/isl28022.c drivers/input/touchscreen/stmpe-ts.c drivers/mfd/twl6030-irq.c drivers/tty/serial/sc16is7xx.c I do think it makes sense to change of_match_node() to have a reference to its arguments, as in the patch below. That probably needs a few extra fixups. Arnd diff --git a/include/linux/of.h b/include/linux/of.h index 9d6b8a61607f..83cfa6c26ee4 100644 --- a/include/linux/of.h +++ b/include/linux/of.h @@ -907,7 +907,11 @@ static inline const void *of_device_get_match_data(const struct device *dev) } #define of_match_ptr(_ptr) NULL -#define of_match_node(_matches, _node) NULL +static inline const struct of_device_id *of_match_node( + const struct of_device_id *matches, const struct device_node *node) +{ + return NULL; +} #endif /* CONFIG_OF */ /* Default string compare functions, Allow arch asm/prom.h to override */ diff --git a/drivers/crypto/caam/ctrl.c b/drivers/crypto/caam/ctrl.c index d4b39184dbdb..bd418dea586d 100644 --- a/drivers/crypto/caam/ctrl.c +++ b/drivers/crypto/caam/ctrl.c @@ -80,7 +80,6 @@ static void build_deinstantiation_desc(u32 *desc, int handle) append_jump(desc, JUMP_CLASS_CLASS1 | JUMP_TYPE_HALT); } -#ifdef CONFIG_OF static const struct of_device_id imx8m_machine_match[] = { { .compatible = "fsl,imx8mm", }, { .compatible = "fsl,imx8mn", }, @@ -89,7 +88,6 @@ static const struct of_device_id imx8m_machine_match[] = { { .compatible = "fsl,imx8ulp", }, { } }; -#endif /* * run_descriptor_deco0 - runs a descriptor on DECO0, under direct control of diff --git a/drivers/dma/dw/rzn1-dmamux.c b/drivers/dma/dw/rzn1-dmamux.c index 4fb8508419db..9dcba3a3ffaa 100644 --- a/drivers/dma/dw/rzn1-dmamux.c +++ b/drivers/dma/dw/rzn1-dmamux.c @@ -104,12 +104,10 @@ static void *rzn1_dmamux_route_allocate(struct of_phandle_args *dma_spec, return ERR_PTR(ret); } -#ifdef CONFIG_OF static const struct of_device_id rzn1_dmac_match[] = { { .compatible = "renesas,rzn1-dma" }, {} }; -#endif static int rzn1_dmamux_probe(struct platform_device *pdev) { diff --git a/drivers/i2c/busses/i2c-at91-core.c b/drivers/i2c/busses/i2c-at91-core.c index edc047e3e535..3a9be06dd967 100644 --- a/drivers/i2c/busses/i2c-at91-core.c +++ b/drivers/i2c/busses/i2c-at91-core.c @@ -108,7 +108,6 @@ static const struct platform_device_id at91_twi_devtypes[] = { } }; -#if defined(CONFIG_OF) static struct at91_twi_pdata at91sam9x5_config = { .clk_max_div = 7, .clk_offset = 4, @@ -178,7 +177,6 @@ static const struct of_device_id atmel_twi_dt_ids[] = { } }; MODULE_DEVICE_TABLE(of, atmel_twi_dt_ids); -#endif static struct at91_twi_pdata *at91_twi_get_driver_data( struct platform_device *pdev) diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c index dc1e46d834dc..969e08e4d4f4 100644 --- a/drivers/i2c/busses/i2c-xiic.c +++ b/drivers/i2c/busses/i2c-xiic.c @@ -1409,7 +1409,6 @@ static const struct i2c_adapter xiic_adapter = { .algo = &xiic_algorithm, }; -#if defined(CONFIG_OF) static const struct xiic_version_data xiic_2_00 = { .quirks = DYNAMIC_MODE_READ_BROKEN_BIT, }; @@ -1420,7 +1419,6 @@ static const struct of_device_id xiic_of_match[] = { {}, }; MODULE_DEVICE_TABLE(of, xiic_of_match); -#endif static int xiic_i2c_probe(struct platform_device *pdev) { diff --git a/drivers/misc/atmel-ssc.c b/drivers/misc/atmel-ssc.c index 35a196341534..3db592f3b451 100644 --- a/drivers/misc/atmel-ssc.c +++ b/drivers/misc/atmel-ssc.c @@ -111,7 +111,6 @@ static const struct platform_device_id atmel_ssc_devtypes[] = { } }; -#ifdef CONFIG_OF static const struct of_device_id atmel_ssc_dt_ids[] = { { .compatible = "atmel,at91rm9200-ssc", @@ -127,7 +126,6 @@ static const struct of_device_id atmel_ssc_dt_ids[] = { } }; MODULE_DEVICE_TABLE(of, atmel_ssc_dt_ids); -#endif static inline const struct atmel_ssc_platform_data * atmel_ssc_get_driver_data(struct platform_device *pdev) diff --git a/drivers/net/can/at91_can.c b/drivers/net/can/at91_can.c index 191707d7e3da..9fbbf3587b0c 100644 --- a/drivers/net/can/at91_can.c +++ b/drivers/net/can/at91_can.c @@ -1013,7 +1013,6 @@ static const struct attribute_group at91_sysfs_attr_group = { .attrs = at91_sysfs_attrs, }; -#if defined(CONFIG_OF) static const struct of_device_id at91_can_dt_ids[] = { { .compatible = "atmel,at91sam9x5-can", @@ -1026,7 +1025,6 @@ static const struct of_device_id at91_can_dt_ids[] = { } }; MODULE_DEVICE_TABLE(of, at91_can_dt_ids); -#endif static const struct at91_devtype_data *at91_can_get_driver_data(struct platform_device *pdev) { diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c index 6c462de81f20..f942c6e54a1b 100644 --- a/drivers/net/ethernet/cadence/macb_main.c +++ b/drivers/net/ethernet/cadence/macb_main.c @@ -4425,7 +4425,6 @@ static const struct macb_usrio_config macb_default_usrio = { .refclk = MACB_BIT(CLKEN), }; -#if defined(CONFIG_OF) /* 1518 rounded up */ #define AT91ETHER_MAX_RBUFF_SZ 0x600 /* max number of receive buffers */ @@ -5144,7 +5143,6 @@ static const struct of_device_id macb_dt_ids[] = { { /* sentinel */ } }; MODULE_DEVICE_TABLE(of, macb_dt_ids); -#endif /* CONFIG_OF */ static const struct macb_config default_gem_config = { .caps = MACB_CAPS_GIGABIT_MODE_AVAILABLE |
diff --git a/drivers/input/touchscreen/stmpe-ts.c b/drivers/input/touchscreen/stmpe-ts.c index a94a1997f96b..3900aa2e3a90 100644 --- a/drivers/input/touchscreen/stmpe-ts.c +++ b/drivers/input/touchscreen/stmpe-ts.c @@ -366,7 +366,7 @@ static struct platform_driver stmpe_ts_driver = { }; module_platform_driver(stmpe_ts_driver); -static const struct of_device_id stmpe_ts_ids[] = { +static const struct of_device_id stmpe_ts_ids[] __maybe_unused = { { .compatible = "st,stmpe-ts", }, { }, };