[v2,01/12] device: property: delay device-driver matches
diff mbox

Message ID 1435743667-11987-2-git-send-email-tomeu.vizoso@collabora.com
State New
Headers show

Commit Message

Tomeu Vizoso July 1, 2015, 9:40 a.m. UTC
Delay matches of platform devices until late_initcall, when we are sure
that all built-in drivers have been registered already. This is needed
to prevent deferred probes because of some dependencies' drivers not
having registered yet.

This reduces the total amount of work that the kernel does during boot
because it won't try to match devices to drivers when built-in drivers
are still registering but also reduces some parallelism, so total boot
time might slightly increase or decrease depending on the platform and
kernel configuration.

This change will make make possible to prevent any deferred probes once
devices are probed in dependency order.

Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
---

Changes in v2:
- Instead of delaying all probes until late_initcall, only delay matches
  of platform devices that have a firmware node attached.

 drivers/base/property.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

Comments

Rafael J. Wysocki July 1, 2015, 11:29 p.m. UTC | #1
On Wednesday, July 01, 2015 11:40:56 AM Tomeu Vizoso wrote:
> Delay matches of platform devices until late_initcall, when we are sure
> that all built-in drivers have been registered already. This is needed
> to prevent deferred probes because of some dependencies' drivers not
> having registered yet.
> 
> This reduces the total amount of work that the kernel does during boot
> because it won't try to match devices to drivers when built-in drivers
> are still registering but also reduces some parallelism, so total boot
> time might slightly increase or decrease depending on the platform and
> kernel configuration.
> 
> This change will make make possible to prevent any deferred probes once
> devices are probed in dependency order.
> 
> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> ---
> 
> Changes in v2:
> - Instead of delaying all probes until late_initcall, only delay matches
>   of platform devices that have a firmware node attached.
> 
>  drivers/base/property.c | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/drivers/base/property.c b/drivers/base/property.c
> index 8528eb9..8ead1ba 100644
> --- a/drivers/base/property.c
> +++ b/drivers/base/property.c
> @@ -16,8 +16,11 @@
>  #include <linux/of.h>
>  #include <linux/of_address.h>
>  #include <linux/of_device.h>
> +#include <linux/platform_device.h>
>  #include <linux/property.h>
>  
> +static bool fwnode_match_enable = false;
> +
>  /**
>   * device_add_property_set - Add a collection of properties to a device object.
>   * @dev: Device to add properties to.
> @@ -604,6 +607,15 @@ EXPORT_SYMBOL_GPL(fwnode_is_compatible);
>  bool fwnode_driver_match_device(struct device *dev,
>  				const struct device_driver *drv)
>  {
> +	/*
> +	 * Delay matches of platform devices until late_initcall, when we are
> +	 * sure that all built-in drivers have been registered already. This
> +	 * is needed to prevent deferred probes because of some drivers
> +	 * not having registered yet.
> +	 */
> +	if(dev->bus == &platform_bus_type && !fwnode_match_enable)
> +		return false;

I'm not particularly enthusiastic about referring to specific bus types in
generic code like that.

What about having a special version of fwnode_driver_match_device() specifically
for the platform bus type that will do the check?

> +
>  	if (is_of_node(dev->fwnode))
>  		return of_driver_match_device(dev, drv);
>  	else if (is_acpi_node(dev->fwnode))
> @@ -612,3 +624,20 @@ bool fwnode_driver_match_device(struct device *dev,
>  	return false;
>  }
>  EXPORT_SYMBOL_GPL(fwnode_driver_match_device);
> +
> +static int __device_attach(struct device *dev, void *data)
> +{
> +	device_initial_probe(dev);
> +
> +	return 0;
> +}
> +
> +static int fwnode_match_initcall(void)
> +{
> +	fwnode_match_enable = true;
> +
> +	bus_for_each_dev(&platform_bus_type, NULL, NULL, __device_attach);
> +
> +	return 0;
> +}
> +late_initcall(fwnode_match_initcall);
>
Tomeu Vizoso July 10, 2015, 11:39 a.m. UTC | #2
On 2 July 2015 at 01:29, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Wednesday, July 01, 2015 11:40:56 AM Tomeu Vizoso wrote:
>> Delay matches of platform devices until late_initcall, when we are sure
>> that all built-in drivers have been registered already. This is needed
>> to prevent deferred probes because of some dependencies' drivers not
>> having registered yet.
>>
>> This reduces the total amount of work that the kernel does during boot
>> because it won't try to match devices to drivers when built-in drivers
>> are still registering but also reduces some parallelism, so total boot
>> time might slightly increase or decrease depending on the platform and
>> kernel configuration.
>>
>> This change will make make possible to prevent any deferred probes once
>> devices are probed in dependency order.
>>
>> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
>> ---
>>
>> Changes in v2:
>> - Instead of delaying all probes until late_initcall, only delay matches
>>   of platform devices that have a firmware node attached.
>>
>>  drivers/base/property.c | 29 +++++++++++++++++++++++++++++
>>  1 file changed, 29 insertions(+)
>>
>> diff --git a/drivers/base/property.c b/drivers/base/property.c
>> index 8528eb9..8ead1ba 100644
>> --- a/drivers/base/property.c
>> +++ b/drivers/base/property.c
>> @@ -16,8 +16,11 @@
>>  #include <linux/of.h>
>>  #include <linux/of_address.h>
>>  #include <linux/of_device.h>
>> +#include <linux/platform_device.h>
>>  #include <linux/property.h>
>>
>> +static bool fwnode_match_enable = false;
>> +
>>  /**
>>   * device_add_property_set - Add a collection of properties to a device object.
>>   * @dev: Device to add properties to.
>> @@ -604,6 +607,15 @@ EXPORT_SYMBOL_GPL(fwnode_is_compatible);
>>  bool fwnode_driver_match_device(struct device *dev,
>>                               const struct device_driver *drv)
>>  {
>> +     /*
>> +      * Delay matches of platform devices until late_initcall, when we are
>> +      * sure that all built-in drivers have been registered already. This
>> +      * is needed to prevent deferred probes because of some drivers
>> +      * not having registered yet.
>> +      */
>> +     if(dev->bus == &platform_bus_type && !fwnode_match_enable)
>> +             return false;
>
> I'm not particularly enthusiastic about referring to specific bus types in
> generic code like that.

Agreed.

> What about having a special version of fwnode_driver_match_device() specifically
> for the platform bus type that will do the check?

Have moved all this code to base/platform.c instead, as I don't see
any reason to have it in property.c.

Thanks,

Tomeu

>> +
>>       if (is_of_node(dev->fwnode))
>>               return of_driver_match_device(dev, drv);
>>       else if (is_acpi_node(dev->fwnode))
>> @@ -612,3 +624,20 @@ bool fwnode_driver_match_device(struct device *dev,
>>       return false;
>>  }
>>  EXPORT_SYMBOL_GPL(fwnode_driver_match_device);
>> +
>> +static int __device_attach(struct device *dev, void *data)
>> +{
>> +     device_initial_probe(dev);
>> +
>> +     return 0;
>> +}
>> +
>> +static int fwnode_match_initcall(void)
>> +{
>> +     fwnode_match_enable = true;
>> +
>> +     bus_for_each_dev(&platform_bus_type, NULL, NULL, __device_attach);
>> +
>> +     return 0;
>> +}
>> +late_initcall(fwnode_match_initcall);
>>
>
> --
> I speak only for myself.
> Rafael J. Wysocki, Intel Open Source Technology Center.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
Mark Brown July 16, 2015, 8:23 p.m. UTC | #3
On Wed, Jul 01, 2015 at 11:40:56AM +0200, Tomeu Vizoso wrote:

> Delay matches of platform devices until late_initcall, when we are sure
> that all built-in drivers have been registered already. This is needed
> to prevent deferred probes because of some dependencies' drivers not
> having registered yet.

I have to say I'm still not 100% clear that special casing platform
devices makes sense here - I can see that platform devices are usually
the first devices to instantiate but there are other kinds of devices
and it's not obvious what the benefit of specifically picking out
platform devices as opposed to just deferring all devices is.
Rafael J. Wysocki July 16, 2015, 11:41 p.m. UTC | #4
Hi Mark,

On Thu, Jul 16, 2015 at 10:23 PM, Mark Brown <broonie@kernel.org> wrote:
> On Wed, Jul 01, 2015 at 11:40:56AM +0200, Tomeu Vizoso wrote:
>
>> Delay matches of platform devices until late_initcall, when we are sure
>> that all built-in drivers have been registered already. This is needed
>> to prevent deferred probes because of some dependencies' drivers not
>> having registered yet.
>
> I have to say I'm still not 100% clear that special casing platform
> devices makes sense here - I can see that platform devices are usually
> the first devices to instantiate but there are other kinds of devices
> and it's not obvious what the benefit of specifically picking out
> platform devices as opposed to just deferring all devices is.

Some existing devices cannot be deferred without redesigning things quite a bit.

What I was talking about, though, was to use an opt-in mechanism for
that which could be set for all platform devices, for example, by
default, but it might be set for other bus types too if that's useful.

Thanks,
Rafael
Mark Brown July 17, 2015, 12:06 a.m. UTC | #5
On Fri, Jul 17, 2015 at 01:41:16AM +0200, Rafael J. Wysocki wrote:
> On Thu, Jul 16, 2015 at 10:23 PM, Mark Brown <broonie@kernel.org> wrote:
> > On Wed, Jul 01, 2015 at 11:40:56AM +0200, Tomeu Vizoso wrote:

> > I have to say I'm still not 100% clear that special casing platform
> > devices makes sense here - I can see that platform devices are usually
> > the first devices to instantiate but there are other kinds of devices
> > and it's not obvious what the benefit of specifically picking out
> > platform devices as opposed to just deferring all devices is.

> Some existing devices cannot be deferred without redesigning things quite a bit.

OK, that should go in the changelog then - right now it's just a bit
obtuse why we're doing this (and as you say it's a bit awkward).  Now
you mention this I'm thinking that some of the affected devices might be
platform devices on some systems, IOMMUs spring to mind for example...
they're one of the main bits of the system I'm aware of that still rely
on probe ordering and they do tend to be platform devices.

> What I was talking about, though, was to use an opt-in mechanism for
> that which could be set for all platform devices, for example, by
> default, but it might be set for other bus types too if that's useful.

Sure, I got that and do agree with you that a mechanism like you suggest
would be good.  I just wasn't clear why we were targetting platform
devices in the first place.

Patch
diff mbox

diff --git a/drivers/base/property.c b/drivers/base/property.c
index 8528eb9..8ead1ba 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -16,8 +16,11 @@ 
 #include <linux/of.h>
 #include <linux/of_address.h>
 #include <linux/of_device.h>
+#include <linux/platform_device.h>
 #include <linux/property.h>
 
+static bool fwnode_match_enable = false;
+
 /**
  * device_add_property_set - Add a collection of properties to a device object.
  * @dev: Device to add properties to.
@@ -604,6 +607,15 @@  EXPORT_SYMBOL_GPL(fwnode_is_compatible);
 bool fwnode_driver_match_device(struct device *dev,
 				const struct device_driver *drv)
 {
+	/*
+	 * Delay matches of platform devices until late_initcall, when we are
+	 * sure that all built-in drivers have been registered already. This
+	 * is needed to prevent deferred probes because of some drivers
+	 * not having registered yet.
+	 */
+	if(dev->bus == &platform_bus_type && !fwnode_match_enable)
+		return false;
+
 	if (is_of_node(dev->fwnode))
 		return of_driver_match_device(dev, drv);
 	else if (is_acpi_node(dev->fwnode))
@@ -612,3 +624,20 @@  bool fwnode_driver_match_device(struct device *dev,
 	return false;
 }
 EXPORT_SYMBOL_GPL(fwnode_driver_match_device);
+
+static int __device_attach(struct device *dev, void *data)
+{
+	device_initial_probe(dev);
+
+	return 0;
+}
+
+static int fwnode_match_initcall(void)
+{
+	fwnode_match_enable = true;
+
+	bus_for_each_dev(&platform_bus_type, NULL, NULL, __device_attach);
+
+	return 0;
+}
+late_initcall(fwnode_match_initcall);