Re: [PATCH v2 5/8] common: add initial barebox deep-probe support

[Ahmad Fatoum <a.fatoum@pengutronix.de>]

On 9/30/20 10:47 AM, Marco Felsch wrote:
> The barebox 'deep probe' or 'probe on demand' mechanism is the answer of
> unwanted -EPROBE_DEFER failures. The EPROBE_DEFER error code was
> introduced by commit ab3da15bc14c ("base: Introduce deferred probing")
> and since then it causes a few problems.
> 
> The error is returned if either the device is not yet present or the
> driver is not yet registered. This makes sense on linux systems where
> modules and hot-plug devices are used very often but not for barebox.
> The module support is rarely used and devices aren't hot pluggable.
> 
> The current barebox behaviour populates all devices before the drivers
> are registered so all devices are present during the driver
> registration. So the driver probe() function gets called immediately
> after the driver registration and causes the -EPROBE_DEFER error if this
> driver depends on an other not yet registered driver.
> 
> To get rid of the EPROBE_DEFER error code we need to reorder the device
> population and the driver registration. All drivers must be registered
> first. In an ideal world all driver can be registered by the same
> initcall level. Then devices are getting populated which causes calling
> the driver probe() function but this time resources/devices are created
> on demand if not yet available.
> 
> Dependencies between devices are normally expressed as references to
> other device nodes. With deep probe barebox provides helper functions
> which take a device node and probe the device behind that node if
> necessary. This means instead of returning -EPROBE_DEFER, we can now
> make the desired resources available once we need them.
> 
> If the resource can't be created we are returning -ENODEV since we are
> not supporting hot-plugging. Dropping EPROBE_DEFER is the long-term
> goal, avoid initcall shifting is the short-term goal.
> 
> Call it deep-probe since the on-demand device creation can greate very
> deep stacks. This commit adds the initial support for: spi, i2c, reset,
> regulator and clk resource on-demand creation. The deep-probe mechanism
> must be enabled for each board to avoid breaking changes using
> deep_probe_enable(). This can be changed later after all boards are
> converted to the new mechanism.
> 
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
> v2:
> - add deep-probe state to verify it once
> - use a linker list array instead of initcalls and runtime allocation
> - make of_device_create_on_demand an internal function
> - convert to of_device_ensure_probed* and of_devices_ensure_probed*
> - add missing function docs
> - force the existence of the driver to avoid EPROBE_DEFER (see long-term
>   goal)
> - return int instead of device_d
> - fix stub funcs
> - add BUG_ON() to track undefined behaviour
> - add comment to of_platform_device_create() to make it clear that
>   calling it again for an already populated device is not allowed
> - i2c: only set of_node->dev if it is available
> 
>  common/Makefile                   |   1 +
>  common/deep-probe.c               |  36 +++++++
>  drivers/base/driver.c             |  11 +-
>  drivers/clk/clk.c                 |   5 +
>  drivers/i2c/i2c.c                 |   8 ++
>  drivers/of/base.c                 |  13 ++-
>  drivers/of/platform.c             | 165 ++++++++++++++++++++++++++++++
>  drivers/regulator/core.c          |   6 ++
>  drivers/reset/core.c              |   4 +
>  drivers/spi/spi.c                 |   2 +
>  include/asm-generic/barebox.lds.h |  10 +-
>  include/deep-probe.h              |  26 +++++
>  include/of.h                      |  32 +++++-
>  13 files changed, 314 insertions(+), 5 deletions(-)
>  create mode 100644 common/deep-probe.c
>  create mode 100644 include/deep-probe.h
> 
> diff --git a/common/Makefile b/common/Makefile
> index c3ae3ca1b9..8525240422 100644
> --- a/common/Makefile
> +++ b/common/Makefile
> @@ -3,6 +3,7 @@ obj-y				+= memory_display.o
>  pbl-$(CONFIG_PBL_CONSOLE)	+= memory_display.o
>  obj-y				+= clock.o
>  obj-y				+= console_common.o
> +obj-y				+= deep-probe.o
>  obj-y				+= startup.o
>  obj-y				+= misc.o
>  obj-pbl-y			+= memsize.o
> diff --git a/common/deep-probe.c b/common/deep-probe.c
> new file mode 100644
> index 0000000000..af53abdc74
> --- /dev/null
> +++ b/common/deep-probe.c
> @@ -0,0 +1,36 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +#include <common.h>
> +#include <deep-probe.h>
> +#include <of.h>
> +
> +enum deep_probe_state {
> +	DEEP_PROBE_UNKONW,

UNKNOWN*

> +	DEEP_PROBE_SUPPORTED,
> +	DEEP_PROBE_NOT_SUPPORTED
> +};
> +
> +static enum deep_probe_state boardstate;
> +
> +bool deep_probe_is_supported(void)
> +{
> +	struct deep_probe_entry *board;
> +
> +	if (boardstate == DEEP_PROBE_NOT_SUPPORTED)
> +		return false;
> +	else if (boardstate == DEEP_PROBE_SUPPORTED)
> +		return true;

If you set UNKNOWN to -ENOSYS, SUPPORTED to 1 and NOT_SUPPORTED to 0,
you could just do if (boardstate >= 0) return boardstate; here
(Even if you want to keep it verbose, I like the enum constants having
expectable values)


> +
> +	/* determine boardstate */
> +	for (board = &__barebox_deep_probe_start;
> +	     board != &__barebox_deep_probe_end; board++) {
> +		if (of_machine_is_compatible(board->compatible)) {
> +			boardstate = DEEP_PROBE_SUPPORTED;
> +			return true;
> +		}
> +	}
> +
> +	boardstate = DEEP_PROBE_NOT_SUPPORTED;
> +	return false;
> +}
> +EXPORT_SYMBOL_GPL(deep_probe_is_supported);
> diff --git a/drivers/base/driver.c b/drivers/base/driver.c
> index 412db6c406..b797655c15 100644
> --- a/drivers/base/driver.c
> +++ b/drivers/base/driver.c
> @@ -21,6 +21,7 @@
>  
>  #include <common.h>
>  #include <command.h>
> +#include <deep-probe.h>
>  #include <driver.h>
>  #include <malloc.h>
>  #include <console.h>
> @@ -95,7 +96,15 @@ int device_probe(struct device_d *dev)
>  	if (ret == -EPROBE_DEFER) {
>  		list_del(&dev->active);
>  		list_add(&dev->active, &deferred);
> -		dev_dbg(dev, "probe deferred\n");
> +
> +		/*
> +		 * -EPROBE_DEFER should never appear on a deep-probe machine so
> +		 * inform the user immediately.
> +		 */
> +		if (deep_probe_is_supported())
> +			dev_warn(dev, "probe deferred\n");
> +		else
> +			dev_dbg(dev, "probe deferred\n");
>  		return ret;
>  	}
>  
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index b04d44593b..e40dfae9ce 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -439,6 +439,11 @@ struct clk *of_clk_get_from_provider(struct of_phandle_args *clkspec)
>  {
>  	struct of_clk_provider *provider;
>  	struct clk *clk = ERR_PTR(-EPROBE_DEFER);
> +	int ret;
> +
> +	ret = of_device_ensure_probed(clkspec->np);
> +	if (ret)
> +		return ERR_PTR(ret);
>  
>  	/* Check if we have such a provider in our array */
>  	list_for_each_entry(provider, &of_clk_providers, link) {
> diff --git a/drivers/i2c/i2c.c b/drivers/i2c/i2c.c
> index 57d8c7017f..12aac42794 100644
> --- a/drivers/i2c/i2c.c
> +++ b/drivers/i2c/i2c.c
> @@ -407,6 +407,9 @@ static struct i2c_client *i2c_new_device(struct i2c_adapter *adapter,
>  	}
>  	client->dev.info = i2c_info;
>  
> +	if (chip->of_node)
> +		chip->of_node->dev = &client->dev;
> +
>  	return client;
>  }
>  
> @@ -548,6 +551,11 @@ struct i2c_adapter *i2c_get_adapter(int busnum)
>  struct i2c_adapter *of_find_i2c_adapter_by_node(struct device_node *node)
>  {
>  	struct i2c_adapter *adap;
> +	int ret;
> +
> +	ret = of_device_ensure_probed(node);
> +	if (ret)
> +		return ERR_PTR(ret);
>  
>  	for_each_i2c_adapter(adap)
>  		if (adap->dev.device_node == node)
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 8ba151dbde..a2f6c39ad1 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -15,6 +15,7 @@
>   * GNU General Public License for more details.
>   */
>  #include <common.h>
> +#include <deep-probe.h>
>  #include <of.h>
>  #include <of_address.h>
>  #include <errno.h>
> @@ -1567,6 +1568,15 @@ int of_set_root_node(struct device_node *node)
>  	return 0;
>  }
>  
> +static int barebox_of_populate(void)
> +{
> +	if (IS_ENABLED(CONFIG_OFDEVICE) && deep_probe_is_supported())
> +		of_probe();

return of_probe(); ?

> +
> +	return 0;
> +}
> +of_populate_initcall(barebox_of_populate);

This function's name should reflect that it's deep probe specific

> +
>  void barebox_register_of(struct device_node *root)
>  {
>  	if (root_node)
> @@ -1577,7 +1587,8 @@ void barebox_register_of(struct device_node *root)
>  
>  	if (IS_ENABLED(CONFIG_OFDEVICE)) {
>  		of_clk_init(root, NULL);
> -		of_probe();
> +		if (!deep_probe_is_supported())
> +			of_probe();
>  	}
>  }
>  
> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index 01de6f98af..0368b1485a 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -15,6 +15,7 @@
>   * GNU General Public License for more details.
>   */
>  #include <common.h>
> +#include <deep-probe.h>
>  #include <malloc.h>
>  #include <of.h>
>  #include <of_address.h>
> @@ -29,6 +30,12 @@
>  struct device_d *of_find_device_by_node(struct device_node *np)
>  {
>  	struct device_d *dev;
> +	int ret;
> +
> +	ret = of_device_ensure_probed(np);
> +	if (ret)
> +		return NULL;
> +

If you associate a dev with the np on deep probe, can't you just
return it deep_probe_is_supported() ?

>  	for_each_device(dev)
>  		if (dev->device_node == np)
>  			return dev;
> @@ -353,3 +360,161 @@ int of_platform_populate(struct device_node *root,
>  	return rc;
>  }
>  EXPORT_SYMBOL_GPL(of_platform_populate);
> +
> +static struct device_d *of_device_create_on_demand(struct device_node *np)
> +{
> +	struct device_node *parent;
> +	struct device_d *parent_dev, *dev;
> +
> +	parent = of_get_parent(np);
> +	if (!parent)
> +		return NULL;
> +
> +	/* Create all parent devices needed for the requested device */
> +	parent_dev = parent->dev ? : of_device_create_on_demand(parent);
> +	if (IS_ERR(parent_dev))
> +		return parent_dev;
> +
> +	/*
> +	 * Parent devices like i2c/spi controllers are populating their own
> +	 * devices. So it can be that the requested device already exist after
> +	 * the parent device creation.
> +	 */
> +	if (np->dev)
> +		return np->dev;
> +
> +	pr_debug("%s: Create %s (%s) on demand\n", __func__,
> +		 np->name, np->full_name);
> +
> +	if (of_device_is_compatible(np, "arm,primecell"))
> +		dev = of_amba_device_create(np);
> +	else
> +		dev = of_platform_device_create(np, parent_dev);
> +
> +	return dev ? : ERR_PTR(-ENODEV);
> +}
> +
> +/**
> + * of_device_ensure_probed() - ensures that a device is probed
> + *
> + * @np: the device_node handle which should be probed
> + *
> + * Ensures that the device is populated and probed so frameworks can make use of
> + * it.
> + *
> + * Return: %0 on success
> + *	   %-ENODEV if either the device can't be populated, the driver is
> + *	     missing or the driver probe returns an error.
> + */
> +int of_device_ensure_probed(struct device_node *np)
> +{
> +	struct device_d *dev;
> +
> +	if (!deep_probe_is_supported())
> +		return 0;
> +
> +	dev = of_device_create_on_demand(np);
> +	if (IS_ERR(dev))
> +		return PTR_ERR(dev);
> +
> +	BUG_ON(!dev);
> +
> +	/*
> +	 * The deep-probe mechanism relies on the fact that all necessary
> +	 * drivers are added before the device creation. Furthermore deep-probe
> +	 * is the answer of the EPROBE_DEFER errno so we must ensure that the

answer to*

> +	 * driver was probed succesfully after the device creation. Both

successfully

> +	 * requirments are fullfilled if 'dev->driver' is not NULL.

requirements, fulfilled 

> +	 */
> +	if (!dev->driver)
> +		return -ENODEV;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(of_device_ensure_probed);
> +
> +/**
> + * of_device_ensure_probed_by_alias() - ensures that a device is probed
> + *
> + * @alias: the alias string to search for a device
> + *
> + * The function search for a given alias string and ensures that the device is
> + * populated and probed if found.
> + *
> + * Return: %0 on success
> + *	   %-ENODEV if either the device can't be populated, the driver is
> + *	     missing or the driver probe returns an error

I don't think it would be nice to just pass along driver probe errors as-is.

> + *	   %-EINVAL if alias can't be found
> + */
> +int of_device_ensure_probed_by_alias(const char *alias)
> +{
> +	struct device_node *dev_node;
> +
> +	dev_node = of_find_node_by_alias(NULL, alias);
> +	if (!dev_node)
> +		return -EINVAL;
> +
> +	return of_device_ensure_probed(dev_node);
> +}
> +EXPORT_SYMBOL_GPL(of_device_ensure_probed_by_alias);
> +
> +/**
> + * of_devices_ensure_probed_by_dev_id() - ensures that a devices are probed
> + *
> + * @np: the start point device_node handle
> + * @ids: the matching 'struct of_device_id' ids
> + *
> + * The function start searching the device tree from @np and populates and
> + * probes devices which matches @ids.
> + *
> + * Return: %0 on success
> + *	   %-ENODEV if either the device wasn't found, can't be populated,
> + *	     the driver is missing or the driver probe returns an error

Likewise

> + */
> +int of_devices_ensure_probed_by_dev_id(struct device_node *np,
> +				       const struct of_device_id *ids)
> +{
> +	struct device_node *child;
> +
> +	if (of_match_node(ids, np))
> +		return of_device_ensure_probed(np);
> +
> +	for_each_child_of_node(np, child) {
> +		int ret;
> +
> +		ret = of_devices_ensure_probed_by_dev_id(child, ids);
> +		if (!ret)
> +			return 0;
> +	}
> +
> +	return -ENODEV;
> +}
> +EXPORT_SYMBOL_GPL(of_devices_ensure_probed_by_dev_id);
> +
> +/**
> + * of_devices_ensure_probed_by_property() - ensures that a devices are probed
> + *
> + * @property_name: The property name to search for
> + *
> + * The function start searching the whole device tree and populates and probe
> + * devices which matches @property_name.
> + *
> + * Return: %0 on success
> + *	   %-ENODEV if either the device wasn't found, can't be populated,
> + *	     the driver is missing or the driver probe returns an error

Likewise

> + */
> +int of_devices_ensure_probed_by_property(const char *property_name)
> +{
> +	struct device_node *node;
> +
> +	for_each_node_with_property(node, property_name) {
> +		int ret;
> +
> +		ret = of_device_ensure_probed(node);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(of_devices_ensure_probed_by_property);
> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> index 6ea21a4609..803b66bd0c 100644
> --- a/drivers/regulator/core.c
> +++ b/drivers/regulator/core.c
> @@ -175,6 +175,7 @@ int of_regulator_register(struct regulator_dev *rd, struct device_node *node)
>  		return PTR_ERR(ri);
>  
>  	ri->node = node;
> +	node->dev = rd->dev;
>  
>  	of_property_read_u32(node, "regulator-enable-ramp-delay",
>  			&ri->enable_time_us);
> @@ -191,6 +192,7 @@ static struct regulator_internal *of_regulator_get(struct device_d *dev, const c
>  	char *propname;
>  	struct regulator_internal *ri;
>  	struct device_node *node;
> +	int ret;
>  
>  	propname = basprintf("%s-supply", supply);
>  
> @@ -222,6 +224,10 @@ static struct regulator_internal *of_regulator_get(struct device_d *dev, const c
>  		goto out;
>  	}
>  
> +	ret = of_device_ensure_probed(node);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
>  	list_for_each_entry(ri, &regulator_list, list) {
>  		if (ri->node == node) {
>  			dev_dbg(dev, "Using %s regulator from %s\n",
> diff --git a/drivers/reset/core.c b/drivers/reset/core.c
> index 99b9c80655..ee2de58c34 100644
> --- a/drivers/reset/core.c
> +++ b/drivers/reset/core.c
> @@ -168,6 +168,10 @@ static struct reset_control *of_reset_control_get(struct device_node *node,
>  	if (ret)
>  		return ERR_PTR(ret);
>  
> +	ret = of_device_ensure_probed(args.np);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
>  	rcdev = NULL;
>  	list_for_each_entry(r, &reset_controller_list, list) {
>  		if (args.np == r->of_node) {
> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index 8421d9d7c1..d1d3bdcc41 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -107,6 +107,8 @@ struct spi_device *spi_new_device(struct spi_controller *ctrl,
>  	if (status)
>  		goto fail;
>  
> +	chip->device_node->dev = &proxy->dev;
> +
>  	return proxy;
>  fail:
>  	free(proxy);
> diff --git a/include/asm-generic/barebox.lds.h b/include/asm-generic/barebox.lds.h
> index a7d32160d1..c5f9d97547 100644
> --- a/include/asm-generic/barebox.lds.h
> +++ b/include/asm-generic/barebox.lds.h
> @@ -114,6 +114,13 @@
>  	KEEP(*(.rsa_keys.rodata.*));		\
>  	__rsa_keys_end = .;			\
>  
> +#define BAREBOX_DEEP_PROBE			\
> +	STRUCT_ALIGN();				\
> +	__barebox_deep_probe_start = .;		\
> +	KEEP(*(SORT_BY_NAME(.barebox_deep_probe*)))	\
> +	__barebox_deep_probe_end = .;
> +
> +
>  #ifdef CONFIG_CONSTRUCTORS
>  #define KERNEL_CTORS()  . = ALIGN(8);                      \
>  			__ctors_start = .;                 \
> @@ -136,7 +143,8 @@
>  	BAREBOX_CLK_TABLE			\
>  	BAREBOX_DTB				\
>  	BAREBOX_RSA_KEYS			\
> -	BAREBOX_PCI_FIXUP
> +	BAREBOX_PCI_FIXUP			\
> +	BAREBOX_DEEP_PROBE
>  
>  #if defined(CONFIG_ARCH_BAREBOX_MAX_BARE_INIT_SIZE) && \
>  CONFIG_ARCH_BAREBOX_MAX_BARE_INIT_SIZE < CONFIG_BAREBOX_MAX_BARE_INIT_SIZE
> diff --git a/include/deep-probe.h b/include/deep-probe.h
> new file mode 100644
> index 0000000000..4f6673a6f1
> --- /dev/null
> +++ b/include/deep-probe.h
> @@ -0,0 +1,26 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +#ifndef __DEEP_PROBE_H
> +#define __DEEP_PROBE_H
> +
> +#include <linux/stringify.h>
> +#include <linux/types.h>
> +
> +struct deep_probe_entry {
> +	const char *compatible;
> +};
> +
> +bool deep_probe_is_supported(void);
> +
> +extern struct deep_probe_entry __barebox_deep_probe_start;
> +extern struct deep_probe_entry __barebox_deep_probe_end;
> +
> +#define _DEEP_PROBE_ENTRY(_entry)		\
> +	__barebox_deep_probe_ ## _entry
> +
> +#define BAREBOX_DEEP_PROBE_ENABLE(_entry,_compatible)			\
> +	const struct deep_probe_entry _DEEP_PROBE_ENTRY(_entry)		\
> +        __attribute__ ((unused,section (".barebox_deep_probe_" __stringify(_entry)))) = { \
> +		.compatible = _compatible,				\
> +	}
> +
> +#endif /* __DEEP_PROBE_H */
> diff --git a/include/of.h b/include/of.h
> index f2dad9a6c2..3042ce261d 100644
> --- a/include/of.h
> +++ b/include/of.h
> @@ -267,6 +267,12 @@ extern struct device_d *of_device_enable_and_register_by_name(const char *name);
>  extern struct device_d *of_device_enable_and_register_by_alias(
>  							const char *alias);
>  
> +extern int of_device_ensure_probed(struct device_node *np);
> +extern int of_device_ensure_probed_by_alias(const char *alias);
> +extern int of_devices_ensure_probed_by_property(const char *property_name);
> +extern int of_devices_ensure_probed_by_dev_id(struct device_node *np,
> +					      const struct of_device_id *ids);
> +
>  struct cdev *of_parse_partition(struct cdev *cdev, struct device_node *node);
>  int of_parse_partitions(struct cdev *cdev, struct device_node *node);
>  int of_partitions_register_fixup(struct cdev *cdev);
> @@ -332,12 +338,34 @@ static inline int of_set_root_node(struct device_node *node)
>  	return -ENOSYS;
>  }
>  
> -static inline struct device_d *of_platform_device_create(struct device_node *np,
> -							 struct device_d *parent)
> +static inline struct device_d *
> +of_platform_device_create(struct device_node *np, struct device_d *parent)

Unrelated change?

>  {
>  	return NULL;
>  }
>  
> +static inline int of_device_ensure_probed(struct device_node *np)
> +{
> +	return 0;
> +}
> +
> +static inline int of_device_ensure_probed_by_alias(const char *alias)
> +{
> +	return 0;
> +}
> +
> +static inline int of_devices_ensure_probed_by_property(const char *property_name)
> +{
> +	return 0;
> +}
> +
> +static inline int
> +of_devices_ensure_probed_by_dev_id(struct device_node *np,
> +				   const struct of_device_id *ids)
> +{
> +	return 0;
> +}
> +
>  static inline int of_bus_n_addr_cells(struct device_node *np)
>  {
>  	return 0;
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox