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

["Théo Lebrun" <theo.lebrun@bootlin.com>]

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