Re: [OSS-Tools] [PATCH 4/5] libdt: add CONFIG_TEST_LOOPBACK

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

On 31.05.23 17:31, Ahmad Fatoum wrote:
> We have quite a bit of tricky udev and device tree parsing code that is
> only tested manually. Best case we would have tests for this in QEMU,
> but until we do, let's test it with loop devices:
> 
> The downside is that we can only test block devices and that we need
> a tiny bit of code that's not used normally, but on the upside, we
> have tests. :-)
> 
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
>  configure.ac |  2 ++
>  meson.build  |  1 +
>  src/dt/dt.h  |  1 -
>  src/libdt.c  | 50 +++++++++++++++++++++++++++++++++++++++++++++++---
>  4 files changed, 50 insertions(+), 4 deletions(-)
> 
> diff --git a/configure.ac b/configure.ac
> index 117a1e169ba9..5b5c74c2b582 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -40,6 +40,8 @@ AC_DEFINE(CONFIG_MTD, [1], [Statically define to be enabled to harmonize barebox
>  
>  AC_DEFINE(CONFIG_STATE, [1], [Statically define to be enabled to harmonize barebox' & dt-utils' code base.])
>  
> +AC_DEFINE(CONFIG_TEST_LOOPBACK, [0], [Only enabled in meson for testing.])
> +
>  AC_CHECK_FUNCS([__secure_getenv secure_getenv])
>  
>  my_CFLAGS="-Wall \
> diff --git a/meson.build b/meson.build
> index 1bc32274af07..be92446f137a 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -21,6 +21,7 @@ conf.set10('CONFIG_MTD', true)
>  conf.set10('CONFIG_STATE', true)
>  conf.set10('CONFIG_STATE_BACKWARD_COMPATIBLE', get_option('state-backward-compatibility'))
>  conf.set10('CONFIG_LOCK_DEVICE_NODE', get_option('lock-device'))
> +conf.set10('CONFIG_TEST_LOOPBACK', get_option('tests'))
>  
>  meson.add_dist_script(
>    find_program('check-news.sh').path(),
> diff --git a/src/dt/dt.h b/src/dt/dt.h
> index a4213d49606a..153b56e09a21 100644
> --- a/src/dt/dt.h
> +++ b/src/dt/dt.h
> @@ -220,7 +220,6 @@ extern int of_set_root_node(struct device_node *node);
>  extern int of_platform_populate(struct device_node *root,
>  				const struct of_device_id *matches,
>  				struct device_d *parent);
> -extern struct device_d *of_find_device_by_node(struct device_node *np);
>  
>  int of_device_is_stdout_path(struct device_d *dev);
>  const char *of_get_model(void);
> diff --git a/src/libdt.c b/src/libdt.c
> index af28de6f17c6..7c24c9197c7f 100644
> --- a/src/libdt.c
> +++ b/src/libdt.c
> @@ -2091,6 +2091,22 @@ struct udev_of_path {
>  
>  static LIST_HEAD(udev_of_devices);
>  
> +static const char *udev_device_get_of_path(struct udev_device *dev)
> +{
> +	const char *of_path;
> +
> +	of_path = udev_device_get_property_value(dev, "OF_FULLNAME");
> +	if (of_path)
> +		return of_path;
> +
> +	if (IS_ENABLED(CONFIG_TEST_LOOPBACK) &&
> +	    !strcmp(udev_device_get_subsystem(dev), "block") &&
> +	    !strncmp(udev_device_get_sysname(dev), "loop", 4))
> +		return udev_device_get_sysname(dev);
> +
> +	return NULL;
> +}
> +
>  static void of_scan_udev_devices(void)
>  {
>  	struct udev *udev;
> @@ -2111,6 +2127,8 @@ static void of_scan_udev_devices(void)
>  	udev_enumerate_add_match_subsystem(enumerate, "spi");
>  	udev_enumerate_add_match_subsystem(enumerate, "mtd");
>  	udev_enumerate_add_match_subsystem(enumerate, "amba");
> +	if (IS_ENABLED(CONFIG_TEST_LOOPBACK))
> +		udev_enumerate_add_match_subsystem(enumerate, "block");
>  	udev_enumerate_scan_devices(enumerate);
>  	devices = udev_enumerate_get_list_entry(enumerate);
>  
> @@ -2124,7 +2142,7 @@ static void of_scan_udev_devices(void)
>  		path = udev_list_entry_get_name(dev_list_entry);
>  		dev = udev_device_new_from_syspath(udev, path);
>  
> -		of_path = udev_device_get_property_value(dev, "OF_FULLNAME");
> +		of_path = udev_device_get_of_path(dev);
>  		if (!of_path)
>  			continue;
>  
> @@ -2148,11 +2166,37 @@ struct udev_device *of_find_device_by_node_path(const char *of_full_path)
>  			ret = udev_of_path->udev;
>  			break;
>  		}
> +		if (!strcmp(udev_of_path->of_path, of_full_path)) {
> +			ret = udev_of_path->udev;
> +			break;
> +		}

This if clause is duplicated. Will remove when applying.

>  	}
>  
>  	return ret;
>  }
>  
> +static struct udev_device *of_find_device_by_node(struct device_node *np)
> +{
> +	struct udev_of_path *udev_of_path;
> +	struct udev_device *dev;
> +	const char *filename;
> +
> +	dev = of_find_device_by_node_path(np->full_name);
> +	if (dev)
> +		return dev;
> +
> +	if (IS_ENABLED(CONFIG_TEST_LOOPBACK) &&
> +	    !of_property_read_string(np, "barebox,filename", &filename) &&
> +	    !strncmp(filename, "/dev/", 5)) {
> +		list_for_each_entry(udev_of_path, &udev_of_devices, list) {
> +			if (!strcmp(udev_of_path->of_path, filename + 5))
> +				return udev_of_path->udev;
> +		}
> +	}
> +
> +	return NULL;
> +}
> +
>  static struct udev_device *device_find_mtd_partition(struct udev_device *dev,
>  		const char *name)
>  {
> @@ -2546,7 +2590,7 @@ static int __of_cdev_find(struct device_node *partition_node, struct cdev *cdev)
>  	 * 42e9401bd146 ("mtd: Add partition device node to mtd partition
>  	 * devices").
>  	 */
> -	dev = of_find_device_by_node_path(partition_node->full_name);
> +	dev = of_find_device_by_node(partition_node);
>  	if (dev) {
>  		if (udev_device_is_eeprom(dev))
>  			return udev_parse_eeprom(dev, &cdev->devpath);
> @@ -2619,7 +2663,7 @@ static int __of_cdev_find(struct device_node *partition_node, struct cdev *cdev)
>  		}
>  	}
>  	else {
> -		dev = of_find_device_by_node_path(node->full_name);
> +		dev = of_find_device_by_node(node);
>  		if (!dev) {
>  			fprintf(stderr, "%s: cannot find device from node %s\n", __func__,
>  					node->full_name);

-- 
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 |