[PATCH] of: replace of_match_node() macro by a function when !CONFIG_OF

[PATCH] of: replace of_match_node() macro by a function when !CONFIG_OF

[Théo Lebrun]

In the !CONFIG_OF case, replace the of_match_node() macro implementation
by a static function. This ensures drivers calling of_match_node() can
be COMPILE_TESTed.

include/linux/of.h declares of_match_node() like this:

	#ifdef CONFIG_OF
	extern const struct of_device_id *of_match_node(
		const struct of_device_id *matches, const struct device_node *node);
	#else
	#define of_match_node(_matches, _node)	NULL
	#endif

When used inside an expression, those two implementations behave truly
differently. The macro implementation has (at least) two pitfalls:

 - Arguments are removed by the preprocessor meaning they do not appear
   to the compiler. This can give "defined but not used" warnings.

 - The returned value type is (void *)
   versus (const struct of_device_id *).
   It works okay if the value is stored in a variable, thanks to C's
   implicit void pointer casting rules. It causes build errors if used
   like `of_match_data(...)->data`.

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 include/linux/of.h | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/include/linux/of.h b/include/linux/of.h
index a0bedd038a05..f973ae119504 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -891,7 +891,13 @@ 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 */

---
base-commit: 256abd8e550ce977b728be79a74e1729438b4948
change-id: 20240708-of-match-node-ad30140a0a9c

Best regards,
-- 
Théo Lebrun <theo.lebrun@bootlin.com>

Re: [PATCH] of: replace of_match_node() macro by a function when !CONFIG_OF

[Rob Herring]

On Mon, Jul 8, 2024 at 2:55 AM Théo Lebrun <theo.lebrun@bootlin.com> wrote:
>
> In the !CONFIG_OF case, replace the of_match_node() macro implementation
> by a static function. This ensures drivers calling of_match_node() can
> be COMPILE_TESTed.
>
> include/linux/of.h declares of_match_node() like this:
>
>         #ifdef CONFIG_OF
>         extern const struct of_device_id *of_match_node(
>                 const struct of_device_id *matches, const struct device_node *node);
>         #else
>         #define of_match_node(_matches, _node)  NULL
>         #endif
>
> When used inside an expression, those two implementations behave truly
> differently. The macro implementation has (at least) two pitfalls:
>
>  - Arguments are removed by the preprocessor meaning they do not appear
>    to the compiler. This can give "defined but not used" warnings.

It also means the arguments don't have to be defined at all which is
the reasoning the commit adding the macro gave:

    I have chosen to use a macro instead of a function to
    be able to avoid defining the first parameter.
    In fact, this "struct of_device_id *" first parameter
    is usualy not defined as well on non-dt builds.

We could change our mind here, but I suspect applying this would
result in some build failures.

>  - The returned value type is (void *)
>    versus (const struct of_device_id *).
>    It works okay if the value is stored in a variable, thanks to C's
>    implicit void pointer casting rules. It causes build errors if used
>    like `of_match_data(...)->data`.

Really, the only places of_match_node() should be used are ones
without a struct device. Otherwise, of_device_get_match_data() or
device_get_match_data() should be used instead.

Rob

Re: [PATCH] of: replace of_match_node() macro by a function when !CONFIG_OF

[Théo Lebrun]

Hello Rob,

On Tue Jul 9, 2024 at 12:24 AM CEST, Rob Herring wrote:
> On Mon, Jul 8, 2024 at 2:55 AM Théo Lebrun <theo.lebrun@bootlin.com> wrote:
> >
> > In the !CONFIG_OF case, replace the of_match_node() macro implementation
> > by a static function. This ensures drivers calling of_match_node() can
> > be COMPILE_TESTed.
> >
> > include/linux/of.h declares of_match_node() like this:
> >
> >         #ifdef CONFIG_OF
> >         extern const struct of_device_id *of_match_node(
> >                 const struct of_device_id *matches, const struct device_node *node);
> >         #else
> >         #define of_match_node(_matches, _node)  NULL
> >         #endif
> >
> > When used inside an expression, those two implementations behave truly
> > differently. The macro implementation has (at least) two pitfalls:
> >
> >  - Arguments are removed by the preprocessor meaning they do not appear
> >    to the compiler. This can give "defined but not used" warnings.
>
> It also means the arguments don't have to be defined at all which is
> the reasoning the commit adding the macro gave:
>
>     I have chosen to use a macro instead of a function to
>     be able to avoid defining the first parameter.
>     In fact, this "struct of_device_id *" first parameter
>     is usualy not defined as well on non-dt builds.
>
> We could change our mind here, but I suspect applying this would
> result in some build failures.

It appears like it would and I did not think about this edge-case. It
doesn't appear like it is a lot of drivers. I'm seeing 221 files with
calls to of_match_node(). Out of those, 22 match for CONFIG_OF.

Out of those, only 9 have their of_device_id table guarded but not the
of_match_node() call. Remainders fall into two categories:
 - call is guarded by #ifdef CONFIG_OF as well,
 - neither of_device_id table nor of_match_node() call are guarded.

The list of remaining culprits:
	drivers/dma/at_hdmac.c
	drivers/dma/dw/rzn1-dmamux.c
	drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
	drivers/i2c/busses/i2c-at91-core.c
	drivers/i2c/busses/i2c-xiic.c
	drivers/misc/atmel-ssc.c
	drivers/net/can/at91_can.c
	drivers/net/ethernet/cadence/macb_main.c
	sound/soc/codecs/wm8904.c

There could be build errors on drivers that do not match for CONFIG_OF,
as well.

> >  - The returned value type is (void *)
> >    versus (const struct of_device_id *).
> >    It works okay if the value is stored in a variable, thanks to C's
> >    implicit void pointer casting rules. It causes build errors if used
> >    like `of_match_data(...)->data`.
>
> Really, the only places of_match_node() should be used are ones
> without a struct device. Otherwise, of_device_get_match_data() or
> device_get_match_data() should be used instead.

I completely agree.

Regards,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com