[PATCH] of: Turn of_match_node into a static inline when CONFIG_OF isn't set

[PATCH] of: Turn of_match_node into a static inline when CONFIG_OF isn't set

[Laurent Pinchart]

when CONFIG_OF is disabled of_match_node is defined as a macro that
evaluates to NULL. This breaks compilation of drivers that dereference
the function's return value directly. Fix it by turning the macro into a
static inline function that returns NULL.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.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 70c64ba..719e69f 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -560,7 +560,13 @@ static inline const char *of_prop_next_string(struct property *prop,
 }
 
 #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 */
 
 #if defined(CONFIG_OF) && defined(CONFIG_NUMA)
-- 
Regards,

Laurent Pinchart

Re: [PATCH] of: Turn of_match_node into a static inline when CONFIG_OF isn't set

[Josh Cartwright]

Hey Laurent-

On Tue, Feb 11, 2014 at 01:36:51PM +0100, Laurent Pinchart wrote:
> when CONFIG_OF is disabled of_match_node is defined as a macro that
> evaluates to NULL. This breaks compilation of drivers that dereference
> the function's return value directly. Fix it by turning the macro into a
> static inline function that returns NULL.

Just this past week I did the same thing, but noticed that it breaks the
following usecase:

	#ifdef CONFIG_OF
	static const struct of_device_id foobar_matches[] = {
		{ .compatible = "foobar,whatsit", },
		{ },
	};
	#endif
	
	static int probeme(struct platform_device *pdev)
	{
		struct of_device_id *id;
	
		id = of_match_node(foobar_matches, pdev->dev.of_node);
		if (id) {
			/* ... */
		}
		return 0;
	}

When !CONFIG_OF and with your change, this will fail to build due to
foobar_matches being undefined.

   Josh

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH] of: Turn of_match_node into a static inline when CONFIG_OF isn't set

[Laurent Pinchart]

Hi Josh,

On Tuesday 11 February 2014 08:41:08 Josh Cartwright wrote:
> On Tue, Feb 11, 2014 at 01:36:51PM +0100, Laurent Pinchart wrote:
> > when CONFIG_OF is disabled of_match_node is defined as a macro that
> > evaluates to NULL. This breaks compilation of drivers that dereference
> > the function's return value directly. Fix it by turning the macro into a
> > static inline function that returns NULL.
> 
> Just this past week I did the same thing, but noticed that it breaks the
> following usecase:
> 
> 	#ifdef CONFIG_OF
> 	static const struct of_device_id foobar_matches[] = {
> 		{ .compatible = "foobar,whatsit", },
> 		{ },
> 	};
> 	#endif
> 
> 	static int probeme(struct platform_device *pdev)
> 	{
> 		struct of_device_id *id;
> 
> 		id = of_match_node(foobar_matches, pdev->dev.of_node);
> 		if (id) {
> 			/* ... */
> 		}
> 		return 0;
> 	}
> 
> When !CONFIG_OF and with your change, this will fail to build due to
> foobar_matches being undefined.

Good point. What would you think about

#define of_match_node(_matches, _node)  ((const struct of_device_id *)NULL)

?

-- 
Regards,

Laurent Pinchart

Re: [PATCH] of: Turn of_match_node into a static inline when CONFIG_OF isn't set

[Josh Cartwright]

On Tue, Feb 11, 2014 at 03:55:35PM +0100, Laurent Pinchart wrote:
> Hi Josh,
> 
> On Tuesday 11 February 2014 08:41:08 Josh Cartwright wrote:
> > On Tue, Feb 11, 2014 at 01:36:51PM +0100, Laurent Pinchart wrote:
> > > when CONFIG_OF is disabled of_match_node is defined as a macro that
> > > evaluates to NULL. This breaks compilation of drivers that dereference
> > > the function's return value directly. Fix it by turning the macro into a
> > > static inline function that returns NULL.
> > 
> > Just this past week I did the same thing, but noticed that it breaks the
> > following usecase:
> > 
> > 	#ifdef CONFIG_OF
> > 	static const struct of_device_id foobar_matches[] = {
> > 		{ .compatible = "foobar,whatsit", },
> > 		{ },
> > 	};
> > 	#endif
> > 
> > 	static int probeme(struct platform_device *pdev)
> > 	{
> > 		struct of_device_id *id;
> > 
> > 		id = of_match_node(foobar_matches, pdev->dev.of_node);
> > 		if (id) {
> > 			/* ... */
> > 		}
> > 		return 0;
> > 	}
> > 
> > When !CONFIG_OF and with your change, this will fail to build due to
> > foobar_matches being undefined.
> 
> Good point. What would you think about
> 
> #define of_match_node(_matches, _node)  ((const struct of_device_id *)NULL)

I've just sent out a patchset that cleans up at least a couple of users
that directly do of_match_node(matches, np)->data using
of_find_matching_node_and_match.  Not sure if that will fix the usage
you have in mind, though.

I am a bit weary about having an of_match_node() user that both directly
dereferences the result (i.e. of_match_node(matches, np)->data) _and_
builds when !CONFIG_OF; most likely due to a traumatic childhood event
where demons flew out my nose.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH] of: Turn of_match_node into a static inline when CONFIG_OF isn't set

[Laurent Pinchart]

Hi Josh,

On Tuesday 11 February 2014 10:48:26 Josh Cartwright wrote:
> On Tue, Feb 11, 2014 at 03:55:35PM +0100, Laurent Pinchart wrote:
> > On Tuesday 11 February 2014 08:41:08 Josh Cartwright wrote:
> > > On Tue, Feb 11, 2014 at 01:36:51PM +0100, Laurent Pinchart wrote:
> > > > when CONFIG_OF is disabled of_match_node is defined as a macro that
> > > > evaluates to NULL. This breaks compilation of drivers that dereference
> > > > the function's return value directly. Fix it by turning the macro into
> > > > a static inline function that returns NULL.
> > > 
> > > Just this past week I did the same thing, but noticed that it breaks the
> > > following usecase:
> > >
> > > 	#ifdef CONFIG_OF
> > > 	static const struct of_device_id foobar_matches[] = {
> > > 		{ .compatible = "foobar,whatsit", },
> > > 		{ },
> > > 	};
> > > 	#endif
> > > 	
> > > 	static int probeme(struct platform_device *pdev)
> > > 	{
> > > 		struct of_device_id *id;
> > > 		
> > > 		id = of_match_node(foobar_matches, pdev->dev.of_node);
> > > 		if (id) {
> > > 			/* ... */
> > > 		}
> > > 		return 0;
> > > 	
> > > 	}
> > > 
> > > When !CONFIG_OF and with your change, this will fail to build due to
> > > foobar_matches being undefined.
> > 
> > Good point. What would you think about
> > 
> > #define of_match_node(_matches, _node)  ((const struct of_device_id
> > *)NULL)
> 
> I've just sent out a patchset that cleans up at least a couple of users
> that directly do of_match_node(matches, np)->data using
> of_find_matching_node_and_match.  Not sure if that will fix the usage
> you have in mind, though.

Not quite. My use case is pretty simple:

	state->info = of_match_node(adv7604_of_id, np)->data;

> I am a bit weary about having an of_match_node() user that both directly
> dereferences the result (i.e. of_match_node(matches, np)->data) _and_
> builds when !CONFIG_OF; most likely due to a traumatic childhood event
> where demons flew out my nose.

I can assign the intermediate value to a variable before dereferencing it and 
drop my of_match_node() patch if it makes everybody happier.

-- 
Regards,

Laurent Pinchart

Re: [PATCH] of: Turn of_match_node into a static inline when CONFIG_OF isn't set

[Josh Cartwright]

On Tue, Feb 11, 2014 at 06:20:49PM +0100, Laurent Pinchart wrote:
> On Tuesday 11 February 2014 10:48:26 Josh Cartwright wrote:
> > On Tue, Feb 11, 2014 at 03:55:35PM +0100, Laurent Pinchart wrote:
> > > On Tuesday 11 February 2014 08:41:08 Josh Cartwright wrote:
> > > > On Tue, Feb 11, 2014 at 01:36:51PM +0100, Laurent Pinchart wrote:
[..]
> > I am a bit weary about having an of_match_node() user that both directly
> > dereferences the result (i.e. of_match_node(matches, np)->data) _and_
> > builds when !CONFIG_OF; most likely due to a traumatic childhood event
> > where demons flew out my nose.
>
> I can assign the intermediate value to a variable before dereferencing it and
> drop my of_match_node() patch if it makes everybody happier.

Assuming you also intend to handle the case of_match_node() may return
NULL, or otherwise prevent the execution of this codepath when
!CONFIG_OF, then, yes, that sounds good.

It sure would be convenient if platform_device had a 'const struct
of_device_id *of_id_entry' member similar to the existing struct
platform_device_id one, that was set up during platform device matching.
Most platform_driver users of of_match_node() would simply go away.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH] of: Turn of_match_node into a static inline when CONFIG_OF isn't set

[Geert Uytterhoeven]

On Tue, Feb 11, 2014 at 7:08 PM, Josh Cartwright <joshc@codeaurora.org> wrote:
> It sure would be convenient if platform_device had a 'const struct
> of_device_id *of_id_entry' member similar to the existing struct
> platform_device_id one, that was set up during platform device matching.
> Most platform_driver users of of_match_node() would simply go away.

Can't the entry be shared for both platform_device_id and of_device_id?
Only one of them can be valid at the same time, right?

Ideally, all xxx_device_id look like

    struct xxx_device_id {
            ... /* bus-specific ID information */
            kernel_ulong_t driver_data;
    };

This may be formalized in some way, using a base class, but thay may
require reordering the fields, like:

    struct base_device_id {
            kernel_ulong_t driver_data;
            long id[0];
    };

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] of: Turn of_match_node into a static inline when CONFIG_OF isn't set

[Arnd Bergmann]

On Tuesday 11 February 2014 19:29:19 Geert Uytterhoeven wrote:
> On Tue, Feb 11, 2014 at 7:08 PM, Josh Cartwright <joshc@codeaurora.org> wrote:

> Ideally, all xxx_device_id look like
> 
>     struct xxx_device_id {
>             ... /* bus-specific ID information */
>             kernel_ulong_t driver_data;
>     };
> 
> This may be formalized in some way, using a base class, but thay may
> require reordering the fields, like:
> 
>     struct base_device_id {
>             kernel_ulong_t driver_data;
>             long id[0];
>     };
> 

You can't reorder the fields because they are shared with user
space in form of the module-init-tools.

	Arnd

Re: [PATCH] of: Turn of_match_node into a static inline when CONFIG_OF isn't set

[Geert Uytterhoeven]

On Tue, Feb 11, 2014 at 9:06 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Tuesday 11 February 2014 19:29:19 Geert Uytterhoeven wrote:
>> On Tue, Feb 11, 2014 at 7:08 PM, Josh Cartwright <joshc@codeaurora.org> wrote:
>> Ideally, all xxx_device_id look like
>>
>>     struct xxx_device_id {
>>             ... /* bus-specific ID information */
>>             kernel_ulong_t driver_data;
>>     };
>>
>> This may be formalized in some way, using a base class, but thay may
>> require reordering the fields, like:
>>
>>     struct base_device_id {
>>             kernel_ulong_t driver_data;
>>             long id[0];
>>     };
>>
>
> You can't reorder the fields because they are shared with user
> space in form of the module-init-tools.

Sure, that's part of the ABI.

But that doesn't mean we can't change the ID as stored in the platform_device.
Many drivers don't want to know the ID, only the driver_data part.
Having that in a uniform way across the different ID types would help.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] of: Turn of_match_node into a static inline when CONFIG_OF isn't set

[Josh Cartwright]

On Wed, Feb 12, 2014 at 10:54:37PM +0100, Geert Uytterhoeven wrote:
> On Tue, Feb 11, 2014 at 9:06 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Tuesday 11 February 2014 19:29:19 Geert Uytterhoeven wrote:
[..]
> > You can't reorder the fields because they are shared with user
> > space in form of the module-init-tools.
> 
> Sure, that's part of the ABI.
> 
> But that doesn't mean we can't change the ID as stored in the platform_device.
> Many drivers don't want to know the ID, only the driver_data part.
> Having that in a uniform way across the different ID types would help.

I think I convinced myself that the existing platform_device::id_entry
manipulation has the same issue as the device::of_match_ptr had before
it was reverted[1], it's just gone unnoticed.

The codepath in question is the platform_driver_register()/driver_attach(), and
as far as I can tell there is nothing in place to prevent the following
scenario:

  Thread 1                                      Thread 2
  platform_driver_register(pdrv1)
    driver_attach(drv1)
     driver_match_device(drv1, dev)
      platform_match(drv1, dev)
       platform_match_id(drv1->id_table, pdev)
        pdev->id_entry = id1;
                                                platform_driver_register(pdrv2)
                                                 driver_attach(drv2)
                                                  driver_match_device(drv2, dev)
                                                   platform_match(drv2, dev)
                                                    platform_match_id(drv2->id_table, pdev)
                                                     pdev->id_entry = id2;
     device_lock(dev)
     driver_probe_device(drv1, dev)
     device_unlock(dev)

So, in this scenario, it's possible that even though 'drv1' is bound to 'dev',
it's id_entry is pointing to somewhere pdrv2's id_table :(.

Fortunately, the chances we'd hit this are slim, as it would require at least
two drivers which match the same device, and at least one of those drivers
would have to make use of id_entry.  However, relying on this still seems
broken.

I suspect it's not generally advisable for a bus to be touching device state
during ->match().

[1]: Thanks to Rob for pointing me at b1608d69cb80 ("drivercore: revert
     addition of of_match to struct device")
-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH] of: Turn of_match_node into a static inline when CONFIG_OF isn't set

[Rob Herring]

On Tue, Feb 11, 2014 at 12:29 PM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> On Tue, Feb 11, 2014 at 7:08 PM, Josh Cartwright <joshc@codeaurora.org> wrote:
>> It sure would be convenient if platform_device had a 'const struct
>> of_device_id *of_id_entry' member similar to the existing struct
>> platform_device_id one, that was set up during platform device matching.
>> Most platform_driver users of of_match_node() would simply go away.
>
> Can't the entry be shared for both platform_device_id and of_device_id?
> Only one of them can be valid at the same time, right?
>
> Ideally, all xxx_device_id look like
>
>     struct xxx_device_id {
>             ... /* bus-specific ID information */
>             kernel_ulong_t driver_data;
>     };
>
> This may be formalized in some way, using a base class, but thay may
> require reordering the fields, like:
>
>     struct base_device_id {
>             kernel_ulong_t driver_data;
>             long id[0];
>     };
>

I believe this is the reason drivers have to call of_match_device:

commit b1608d69cb804e414d0887140ba08a9398e4e638
Author: Grant Likely <grant.likely@secretlab.ca>
Date:   Wed May 18 11:19:24 2011 -0600

    drivercore: revert addition of of_match to struct device

    Commit b826291c, "drivercore/dt: add a match table pointer to struct
    device" added an of_match pointer to struct device to cache the
    of_match_table entry discovered at driver match time.  This was unsafe
    because matching is not an atomic operation with probing a driver.  If
    two or more drivers are attempted to be matched to a driver at the
    same time, then the cached matching entry pointer could get
    overwritten.

    This patch reverts the of_match cache pointer and reworks all users to
    call of_match_device() directly instead.

    Signed-off-by: Grant Likely <grant.likely@secretlab.ca>

Rob

Re: [PATCH] of: Turn of_match_node into a static inline when CONFIG_OF isn't set

[Josh Cartwright]

On Tue, Feb 11, 2014 at 03:30:33PM -0600, Rob Herring wrote:
> On Tue, Feb 11, 2014 at 12:29 PM, Geert Uytterhoeven
> <geert@linux-m68k.org> wrote:
> > On Tue, Feb 11, 2014 at 7:08 PM, Josh Cartwright <joshc@codeaurora.org> wrote:
> >> It sure would be convenient if platform_device had a 'const struct
> >> of_device_id *of_id_entry' member similar to the existing struct
> >> platform_device_id one, that was set up during platform device matching.
> >> Most platform_driver users of of_match_node() would simply go away.
> >
> > Can't the entry be shared for both platform_device_id and of_device_id?
> > Only one of them can be valid at the same time, right?
> >
[..]
> 
> I believe this is the reason drivers have to call of_match_device:
> 
> commit b1608d69cb804e414d0887140ba08a9398e4e638
> Author: Grant Likely <grant.likely@secretlab.ca>
> Date:   Wed May 18 11:19:24 2011 -0600
>
>     drivercore: revert addition of of_match to struct device
>
>     Commit b826291c, "drivercore/dt: add a match table pointer to struct
>     device" added an of_match pointer to struct device to cache the
>     of_match_table entry discovered at driver match time.  This was unsafe
>     because matching is not an atomic operation with probing a driver.  If
>     two or more drivers are attempted to be matched to a driver at the
>     same time, then the cached matching entry pointer could get
>     overwritten.
>
>     This patch reverts the of_match cache pointer and reworks all users to
>     call of_match_device() directly instead.
>
>     Signed-off-by: Grant Likely <grant.likely@secretlab.ca>

Interesting, thanks for the history!  I'm wondering if this same problem
exists for the existing platform_device_id cached pointer as well.

Okay, so maybe caching a pointer in the device isn't the best option,
what if we considered extending the platform_driver callbacks to include
a set of per-method (?) probe callbacks which do provide a handle to
matched identifiers.

In the case of a totally contrived platform_driver supporting ACPI, OF,
and !OF configurations, it might look something like:

	static const struct of_device_id acme_of_table[] = {
		/* ... */
		{ },
	};
	MODULE_DEVICE_TABLE(of, acme_of_table);

	static int acme_probe_of(struct platform_device *pdev,
				 const struct of_device_id *id)
	{
		/* ... */
		return 0;
	}

	static const struct acpi_device_id acme_acpi_table[] = {
		/* ... */
		{ },
	};
	MODULE_DEVICE_TABLE(acpi, acme_acpi_table);

	static int acme_probe_acpi(struct platform_device *pdev,
				   const struct acpi_device_id *id)
	{
		/* ... */
		return 0;
	}

	static const struct platform_device_id acme_platform_table[] = {
		/* ... */
		{ },
	};
	MODULE_DEVICE_TABLE(platform, acme_platform_table);

	static int acme_probe_acpi(struct platform_device *pdev,
				   const struct platform_device_id *id)
	{
		/* ... */
		return 0;
	}

	static int acme_probe_name(struct platform_device *pdev)
	{
		/* ... */
		return 0;
	}

	static struct platform_driver acme_driver = {
		.probe_of	= acme_probe_of,
		.probe_acpi	= acme_probe_acpi,
		.probe_platform	= acme_probe_platform,
		.probe_name	= acme_probe_name,
		.remove		= acme_remove,
		.driver		= {
			.name			= "acme",
			.of_match_table		= of_match_ptr(acme_of_table),
			.acpi_match_table	= ACPI_PTR(acme_acpi_table),
		},
	};
	module_platform_driver(acme_driver);

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH] of: Turn of_match_node into a static inline when CONFIG_OF isn't set

[Rob Herring]

On Tue, Feb 11, 2014 at 3:55 PM, Josh Cartwright <joshc@codeaurora.org> wrote:
> On Tue, Feb 11, 2014 at 03:30:33PM -0600, Rob Herring wrote:
>> On Tue, Feb 11, 2014 at 12:29 PM, Geert Uytterhoeven
>> <geert@linux-m68k.org> wrote:
>> > On Tue, Feb 11, 2014 at 7:08 PM, Josh Cartwright <joshc@codeaurora.org> wrote:
>> >> It sure would be convenient if platform_device had a 'const struct
>> >> of_device_id *of_id_entry' member similar to the existing struct
>> >> platform_device_id one, that was set up during platform device matching.
>> >> Most platform_driver users of of_match_node() would simply go away.
>> >
>> > Can't the entry be shared for both platform_device_id and of_device_id?
>> > Only one of them can be valid at the same time, right?
>> >
> [..]
>>
>> I believe this is the reason drivers have to call of_match_device:
>>
>> commit b1608d69cb804e414d0887140ba08a9398e4e638
>> Author: Grant Likely <grant.likely@secretlab.ca>
>> Date:   Wed May 18 11:19:24 2011 -0600
>>
>>     drivercore: revert addition of of_match to struct device
>>
>>     Commit b826291c, "drivercore/dt: add a match table pointer to struct
>>     device" added an of_match pointer to struct device to cache the
>>     of_match_table entry discovered at driver match time.  This was unsafe
>>     because matching is not an atomic operation with probing a driver.  If
>>     two or more drivers are attempted to be matched to a driver at the
>>     same time, then the cached matching entry pointer could get
>>     overwritten.
>>
>>     This patch reverts the of_match cache pointer and reworks all users to
>>     call of_match_device() directly instead.
>>
>>     Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
>
> Interesting, thanks for the history!  I'm wondering if this same problem
> exists for the existing platform_device_id cached pointer as well.
>
> Okay, so maybe caching a pointer in the device isn't the best option,
> what if we considered extending the platform_driver callbacks to include
> a set of per-method (?) probe callbacks which do provide a handle to
> matched identifiers.
>
> In the case of a totally contrived platform_driver supporting ACPI, OF,
> and !OF configurations, it might look something like:
>
>         static const struct of_device_id acme_of_table[] = {
>                 /* ... */
>                 { },
>         };
>         MODULE_DEVICE_TABLE(of, acme_of_table);
>
>         static int acme_probe_of(struct platform_device *pdev,
>                                  const struct of_device_id *id)

I don't think this is the right direction. You might want to look at
of_platform_driver in git history...

We still have something like this for macio_bus though.

Rob

Re: [PATCH] of: Turn of_match_node into a static inline when CONFIG_OF isn't set

[Josh Cartwright]

On Tue, Feb 11, 2014 at 05:14:51PM -0600, Rob Herring wrote:
> On Tue, Feb 11, 2014 at 3:55 PM, Josh Cartwright <joshc@codeaurora.org> wrote:
[..]
> > Okay, so maybe caching a pointer in the device isn't the best option,
> > what if we considered extending the platform_driver callbacks to include
> > a set of per-method (?) probe callbacks which do provide a handle to
> > matched identifiers.
> >
> > In the case of a totally contrived platform_driver supporting ACPI, OF,
> > and !OF configurations, it might look something like:
> >
> >         static const struct of_device_id acme_of_table[] = {
> >                 /* ... */
> >                 { },
> >         };
> >         MODULE_DEVICE_TABLE(of, acme_of_table);
> >
> >         static int acme_probe_of(struct platform_device *pdev,
> >                                  const struct of_device_id *id)
> 
> I don't think this is the right direction. You might want to look at
> of_platform_driver in git history...

Thanks for the pointer, of_platform_driver was on it's way out the door
while I was still coming up to speed on devicetree.

To be clear, I'm not proposing that we reintroduce a new bus_type, or
duplicate driver instances, or anything that made of_platform_driver a
pain to deal with.

I'm only suggesting that we consider providing a set of interfaces that
1) provide a simpler/more convenient way for a driver to get at matched
id table entries, and 2) provide a clearer, declarative mechanism by
which a platform driver might isolate it's firmware-specific glue bits
from core driver logic.

Anyway, thanks for hearing me out!

  Josh

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH] of: Turn of_match_node into a static inline when CONFIG_OF isn't set

[Laurent Pinchart]

Hi Josh,

On Tuesday 11 February 2014 12:08:46 Josh Cartwright wrote:
> On Tue, Feb 11, 2014 at 06:20:49PM +0100, Laurent Pinchart wrote:
> > On Tuesday 11 February 2014 10:48:26 Josh Cartwright wrote:
> > > On Tue, Feb 11, 2014 at 03:55:35PM +0100, Laurent Pinchart wrote:
> > > > On Tuesday 11 February 2014 08:41:08 Josh Cartwright wrote:
> > > > > On Tue, Feb 11, 2014 at 01:36:51PM +0100, Laurent Pinchart wrote:
> [..]
> 
> > > I am a bit weary about having an of_match_node() user that both directly
> > > dereferences the result (i.e. of_match_node(matches, np)->data) _and_
> > > builds when !CONFIG_OF; most likely due to a traumatic childhood event
> > > where demons flew out my nose.
> > 
> > I can assign the intermediate value to a variable before dereferencing it
> > and drop my of_match_node() patch if it makes everybody happier.
> 
> Assuming you also intend to handle the case of_match_node() may return
> NULL, or otherwise prevent the execution of this codepath when
> !CONFIG_OF, then, yes, that sounds good.

I'll condition the call of that function to IS_ENABLED(CONFIG_OF), yes.

> It sure would be convenient if platform_device had a 'const struct
> of_device_id *of_id_entry' member similar to the existing struct
> platform_device_id one, that was set up during platform device matching.
> Most platform_driver users of of_match_node() would simply go away.

There's definitely room for improvement (although in this particular case the 
driver handles an I2C device, not a platform device).

I've always been slightly bothered by the duplication of matching information 
between native/legacy device IDs and OF device IDs. I haven't really given 
this a thought though, but I believe something should be done.

I've just remembered that, in the I2C case, the I2C core selects the 
appropriate i2c_device_id matching entry based on the compatible string, 
provided that the i2c_device_id table matches the of_device_id table with I2C 
device names set to the OF compatible string with the vendor prefix stripped 
off. Just food for thought.

-- 
Regards,

Laurent Pinchart

Re: [PATCH] of: Turn of_match_node into a static inline when CONFIG_OF isn't set

[Grant Likely]

On Tue, 11 Feb 2014 10:48:26 -0600, Josh Cartwright <joshc@codeaurora.org> wrote:
> On Tue, Feb 11, 2014 at 03:55:35PM +0100, Laurent Pinchart wrote:
> > Hi Josh,
> > 
> > On Tuesday 11 February 2014 08:41:08 Josh Cartwright wrote:
> > > On Tue, Feb 11, 2014 at 01:36:51PM +0100, Laurent Pinchart wrote:
> > > > when CONFIG_OF is disabled of_match_node is defined as a macro that
> > > > evaluates to NULL. This breaks compilation of drivers that dereference
> > > > the function's return value directly. Fix it by turning the macro into a
> > > > static inline function that returns NULL.
> > > 
> > > Just this past week I did the same thing, but noticed that it breaks the
> > > following usecase:
> > > 
> > > 	#ifdef CONFIG_OF
> > > 	static const struct of_device_id foobar_matches[] = {
> > > 		{ .compatible = "foobar,whatsit", },
> > > 		{ },
> > > 	};
> > > 	#endif
> > > 
> > > 	static int probeme(struct platform_device *pdev)
> > > 	{
> > > 		struct of_device_id *id;
> > > 
> > > 		id = of_match_node(foobar_matches, pdev->dev.of_node);
> > > 		if (id) {
> > > 			/* ... */
> > > 		}
> > > 		return 0;
> > > 	}
> > > 
> > > When !CONFIG_OF and with your change, this will fail to build due to
> > > foobar_matches being undefined.
> > 
> > Good point. What would you think about
> > 
> > #define of_match_node(_matches, _node)  ((const struct of_device_id *)NULL)
> 
> I've just sent out a patchset that cleans up at least a couple of users
> that directly do of_match_node(matches, np)->data using
> of_find_matching_node_and_match.  Not sure if that will fix the usage
> you have in mind, though.
> 
> I am a bit weary about having an of_match_node() user that both directly
> dereferences the result (i.e. of_match_node(matches, np)->data) _and_
> builds when !CONFIG_OF; most likely due to a traumatic childhood event
> where demons flew out my nose.

As is supposed to happen when code dereferrences pointers blindly.

g.

Re: [PATCH] of: Turn of_match_node into a static inline when CONFIG_OF isn't set

[Grant Likely]

On Tue, 11 Feb 2014 13:36:51 +0100, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote:
> when CONFIG_OF is disabled of_match_node is defined as a macro that
> evaluates to NULL. This breaks compilation of drivers that dereference
> the function's return value directly. Fix it by turning the macro into a
> static inline function that returns NULL.

Is it not more problematic that code is dereferencing the return value
directly *without* checking to see if it is NULL first? I suppose it
will cause a runtime oops on all platforms except no-mmu, but still.
Compile time catches are better.

g.

> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.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 70c64ba..719e69f 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -560,7 +560,13 @@ static inline const char *of_prop_next_string(struct property *prop,
>  }
>  
>  #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 */
>  
>  #if defined(CONFIG_OF) && defined(CONFIG_NUMA)
> -- 
> Regards,
> 
> Laurent Pinchart
> 

Re: [PATCH] of: Turn of_match_node into a static inline when CONFIG_OF isn't set

[Laurent Pinchart]

Hi Grant,

On Monday 17 February 2014 18:18:29 Grant Likely wrote:
> On Tue, 11 Feb 2014 13:36:51 +0100, Laurent Pinchart wrote:
> > when CONFIG_OF is disabled of_match_node is defined as a macro that
> > evaluates to NULL. This breaks compilation of drivers that dereference
> > the function's return value directly. Fix it by turning the macro into a
> > static inline function that returns NULL.
> 
> Is it not more problematic that code is dereferencing the return value
> directly *without* checking to see if it is NULL first? I suppose it
> will cause a runtime oops on all platforms except no-mmu, but still.
> Compile time catches are better.

There should be no risk of oops if the of_match_ptr() call is guarded by a 
compiler (as opposed to preprocessor) IS_ENABLED(CONFIG_OF) check. Now, if the 
check is missing, we'll oops at runtime, but the problem already exists before 
this patch.

Anyway I've decided to drop this patch and use an intermediate variable.

> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.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 70c64ba..719e69f 100644
> > --- a/include/linux/of.h
> > +++ b/include/linux/of.h
> > @@ -560,7 +560,13 @@ static inline const char *of_prop_next_string(struct
> > property *prop,> 
> >  }
> >  
> >  #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 */
> >  
> >  #if defined(CONFIG_OF) && defined(CONFIG_NUMA)

-- 
Regards,

Laurent Pinchart