linux-kernel - Re: [PATCH v3 1/4] device property: document device_for_each_child

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZrFCbVdFIvgX-iXq@kekkonen.localdomain>
Date: Mon, 5 Aug 2024 21:21:49 +0000
From: Sakari Ailus <sakari.ailus@...ux.intel.com>
To: Javier Carrasco <javier.carrasco.cruz@...il.com>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	"Rafael J. Wysocki" <rafael@...nel.org>,
	Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
	Jonathan Cameron <jic23@...nel.org>, Rob Herring <robh@...nel.org>,
	Daniel Scally <djrscally@...il.com>,
	Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
	Jean Delvare <jdelvare@...e.com>,
	Guenter Roeck <linux@...ck-us.net>, Pavel Machek <pavel@....cz>,
	Lee Jones <lee@...nel.org>,
	Marcin Wojtas <marcin.s.wojtas@...il.com>,
	Russell King <linux@...linux.org.uk>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Andreas Kemnade <andreas@...nade.info>, linux-acpi@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-hwmon@...r.kernel.org,
	linux-leds@...r.kernel.org, netdev@...r.kernel.org,
	Jonathan Cameron <Jonathan.Cameron@...wei.com>
Subject: Re: [PATCH v3 1/4] device property: document
 device_for_each_child_node macro

Hi Javier,

Thanks for the patch.

On Mon, Aug 05, 2024 at 04:49:44PM +0200, Javier Carrasco wrote:
> There have been some misconceptions about this macro, which iterates
> over available child nodes from different backends.
> 
> As that is not obvious by its name, some users have opted for the
> `fwnode_for_each_available_child_node()` macro instead.
> That requires an unnecessary, explicit access to the fwnode member
> of the device structure.
> 
> Passing the device to `device_for_each_child_node()` is shorter,
> reflects more clearly the nature of the child nodes, and renders the
> same result.
> 
> In general, `fwnode_for_each_available_child_node()` should be used
> whenever the parent node of the children to iterate over is a firmware
> node, and not the device itself.
> 
> Document the `device_for_each_child node(dev, child)` macro to clarify
> its functionality.
> 
> Suggested-by: Jonathan Cameron <jic23@...nel.org>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@...wei.com>
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@...il.com>
> ---
>  include/linux/property.h | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/include/linux/property.h b/include/linux/property.h
> index 61fc20e5f81f..da8f1208de38 100644
> --- a/include/linux/property.h
> +++ b/include/linux/property.h
> @@ -171,6 +171,16 @@ struct fwnode_handle *fwnode_get_next_available_child_node(
>  struct fwnode_handle *device_get_next_child_node(const struct device *dev,
>  						 struct fwnode_handle *child);
>  
> +/**
> + * device_for_each_child_node - iterate over available child nodes of a device
> + * @dev: Pointer to the struct device
> + * @child: Pointer to an available child node in each loop iteration
> + *
> + * Unavailable nodes are skipped i.e. this macro is implicitly _available_.

Is it?

I think this was brought up previously but I understand there's an
available check on OF but not on ACPI, so on OF the availability-agnostic
and availability-aware variants are the same.

Both in OF and ACPI a node that's been marked not available simply isn't
there so as far as I understand, the semantics are the same. There's a
difference though: in OF every node can be tested for availability whereas
on ACPI only device nodes can (there are data nodes, too, for which the
availability test is always false as it's pointless to test them in the
first place).

So overall I guess the code is mostly ok be but the caller does need to be
careful with the differences.

> + * The reference to the child node must be dropped on early exits.
> + * See fwnode_handle_put().
> + * For a scoped version of this macro, use device_for_each_child_node_scoped().
> + */
>  #define device_for_each_child_node(dev, child)				\
>  	for (child = device_get_next_child_node(dev, NULL); child;	\
>  	     child = device_get_next_child_node(dev, child))
> 

-- 
Kind regards,

Sakari Ailus