| 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
| ||
|
Message-ID: <b6b62ddd-ab59-4112-8f6e-c72618c45910@gmail.com>
Date: Wed, 19 Mar 2025 08:02:24 +0200
From: Matti Vaittinen <mazziesaccount@...il.com>
To: Sakari Ailus <sakari.ailus@...ux.intel.com>
Cc: Matti Vaittinen <matti.vaittinen@...rohmeurope.com>,
Jonathan Cameron <jic23@...nel.org>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
<conor+dt@...nel.org>, Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
Daniel Scally <djrscally@...il.com>,
Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
"Rafael J. Wysocki" <rafael@...nel.org>, Danilo Krummrich <dakr@...nel.org>,
Claudiu Manoil <claudiu.manoil@....com>, Andrew Lunn
<andrew+netdev@...n.ch>, "David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
Paolo Abeni <pabeni@...hat.com>, linux-iio@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-acpi@...r.kernel.org, netdev@...r.kernel.org,
Laurent Pinchart <laurent.pinchart@...asonboard.com>
Subject: Re: [PATCH v8 02/10] property: Add functions to iterate named child
Moro Sakari,
Thanks for the review.
On 18/03/2025 17:24, Sakari Ailus wrote:
> Moi,
>
> On Mon, Mar 17, 2025 at 05:50:38PM +0200, Matti Vaittinen wrote:
>> There are a few use-cases where child nodes with a specific name need to
>> be parsed. Code like:
>>
>> fwnode_for_each_child_node()
>> if (fwnode_name_eq())
>> ...
>>
>> can be found from a various drivers/subsystems. Adding a macro for this
>> can simplify things a bit.
>>
>> In a few cases the data from the found nodes is later added to an array,
>> which is allocated based on the number of found nodes. One example of
>> such use is the IIO subsystem's ADC channel nodes, where the relevant
>> nodes are named as channel[@N].
>>
>> Add helpers for iterating and counting device's sub-nodes with certain
>> name instead of open-coding this in every user.
>>
>> Suggested-by: Jonathan Cameron <jic23@...nel.org>
>> Signed-off-by: Matti Vaittinen <mazziesaccount@...il.com>
>> Reviewed-by: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
>> Reviewed-by: Marcelo Schmitt <marcelo.schmitt1@...il.com>
>> ---
>> Revision history:
>> v7 => v8:
>> - Fix the example in fwnode_get_named_child_node_count() documentation
>> to use the fwnode_get_named_child_node_count() and not the
>> device_get_named_child_node_count()
>> - Fix the rest of the new macro's indentiations
>> v6 => v7:
>> - Improve kerneldoc
>> - Inline device_get_named_child_node_count() and change it to call
>> fwnode_get_named_child_node_count() inside
>> - Fix indentiation of the new macros
>> v5 => v6:
>> - Add helpers to also iterate through the nodes.
>> v4 => v5:
>> - Use given name instead of string 'channel' when counting the nodes
>> - Add also fwnode_get_child_node_count_named() as suggested by Rob.
>> v3 => v4:
>> - New patch as suggested by Jonathan, see discussion in:
>> https://lore.kernel.org/lkml/20250223161338.5c896280@jic23-huawei/
>> ---
>> drivers/base/property.c | 27 +++++++++++++++++++++++++++
>> include/linux/property.h | 24 ++++++++++++++++++++++++
>> 2 files changed, 51 insertions(+)
>>
>> diff --git a/drivers/base/property.c b/drivers/base/property.c
>> index c1392743df9c..f42f32ff45fc 100644
>> --- a/drivers/base/property.c
>> +++ b/drivers/base/property.c
>> @@ -945,6 +945,33 @@ unsigned int device_get_child_node_count(const struct device *dev)
>> }
>> EXPORT_SYMBOL_GPL(device_get_child_node_count);
>>
>> +/**
>> + * fwnode_get_named_child_node_count - number of child nodes with given name
>> + * @fwnode: Node which child nodes are counted.
>> + * @name: String to match child node name against.
>> + *
>> + * Scan child nodes and count all the nodes with a specific name. Potential
>> + * 'number' -ending after the 'at sign' for scanned names is ignored.
>> + * E.g.::
>> + * fwnode_get_named_child_node_count(fwnode, "channel");
>> + * would match all the nodes::
>> + * channel { }, channel@0 {}, channel@...bba {}...
>> + *
>> + * Return: the number of child nodes with a matching name for a given device.
>> + */
>> +unsigned int fwnode_get_named_child_node_count(const struct fwnode_handle *fwnode,
>> + const char *name)
>> +{
>> + struct fwnode_handle *child;
>> + unsigned int count = 0;
>> +
>> + fwnode_for_each_named_child_node(fwnode, child, name)
>> + count++;
>> +
>> + return count;
>> +}
>> +EXPORT_SYMBOL_GPL(fwnode_get_named_child_node_count);
>> +
>> bool device_dma_supported(const struct device *dev)
>> {
>> return fwnode_call_bool_op(dev_fwnode(dev), device_dma_supported);
>> diff --git a/include/linux/property.h b/include/linux/property.h
>> index e214ecd241eb..a1856e6b714c 100644
>> --- a/include/linux/property.h
>> +++ b/include/linux/property.h
>> @@ -167,10 +167,18 @@ struct fwnode_handle *fwnode_get_next_available_child_node(
>> for (child = fwnode_get_next_child_node(fwnode, NULL); child; \
>> child = fwnode_get_next_child_node(fwnode, child))
>>
>> +#define fwnode_for_each_named_child_node(fwnode, child, name) \
>> + fwnode_for_each_child_node(fwnode, child) \
>> + if (!fwnode_name_eq(child, name)) { } else
>> +
>> #define fwnode_for_each_available_child_node(fwnode, child) \
>> for (child = fwnode_get_next_available_child_node(fwnode, NULL); child;\
>> child = fwnode_get_next_available_child_node(fwnode, child))
>>
>> +#define fwnode_for_each_available_named_child_node(fwnode, child, name) \
>> + fwnode_for_each_available_child_node(fwnode, child) \
>> + if (!fwnode_name_eq(child, name)) { } else
>> +
>
> OF only enumerates available nodes via the fwnode API, software nodes don't
> have the concept but on ACPI I guess you could have a difference in nodes
> where you have device sub-nodes that aren't available. Still, these ACPI
> device nodes don't have meaningful names in this context (they're
> 4-character object names) so you wouldn't use them like this anyway.
I believe you have far better understanding on these concepts than I do.
The reason behind adding fwnode_for_each_available_child_node() was the
patch 10/10:
- fwnode_for_each_available_child_node(sensors, node) {
- if (fwnode_name_eq(node, "sensor")) {
- if (!thp7312_sensor_parse_dt(thp7312, node))
- num_sensors++;
- }
+ fwnode_for_each_available_named_child_node(sensors, node, "sensor") {
+ if (!thp7312_sensor_parse_dt(thp7312, node))
+ num_sensors++;
}
> So my question is: is it useful to provide this besides
> fwnode_for_each_named_child_node(), given that both are effectively the
> same?
So, I suppose you're saying the existing thp7312 -driver has no real
reason to use the 'fwnode_for_each_available_child_node()', but it could
be using fwnode_for_each_child_node() instead?
If so, I am Ok with dropping the
'fwnode_for_each_available_named_child_node()' and changing the 10/10 to:
- fwnode_for_each_available_child_node(sensors, node) {
- if (fwnode_name_eq(node, "sensor")) {
- if (!thp7312_sensor_parse_dt(thp7312, node))
- num_sensors++;
- }
+ fwnode_for_each_named_child_node(sensors, node, "sensor") {
+ if (!thp7312_sensor_parse_dt(thp7312, node))
+ num_sensors++;
}
Do you think that'd be correct?
Yours,
-- Matti
Powered by blists - more mailing lists