linux-kernel - Re: [PATCH v8 02/10] property: Add functions to iterate named 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] [day] [month] [year] [list]
Message-ID: <726281b2-66f5-45e9-94f2-4f79b4ab159d@gmail.com>
Date: Thu, 20 Mar 2025 08:43:44 +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

On 19/03/2025 17:23, Sakari Ailus wrote:
> On Wed, Mar 19, 2025 at 08:02:24AM +0200, Matti Vaittinen wrote:
>> On 18/03/2025 17:24, Sakari Ailus wrote:
>>> 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:

...

>>>> --- 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?
> 
> I'd say so. Feel free to cc me to the last patch as well.

Thanks. I'll drop the fwnode_for_each_available_named_child_node() then.

> I guess one way to make this clearer is to switch to
> fwnode_for_each_child_node() in a separate patch before
> fwnode_for_each_named_child_node() conversion.

I suppose this makes sense.

I think this series can't make it to 6.15-rc1. Meaning, these 
*_named_*() APIs perhaps land in 6.16-rc1. I assume these *_named_*() 
APIs will go through the IIO. This rather simple IIO driver's review 
took longer than I predicted, with more versions I intended (as always) 
- and I kind of dislike respinning the whole series, with this large 
audience, when changes are not interesting to the most.

Maybe it is simplest to drop the thp7312 (and gianfar) from this series, 
and respin them only when the 6.16-rc1 is out. It's going to be couple 
of months though - so there's always a risk that I forget.

The proposed change for the thp7312, from 
fwnode_for_each_available_child_node() to fwnode_for_each_child_node() 
can be done earlier though.

> There are also just a handful of users of
> fwnode_for_each_available_child_node() and I guess these could be
> converted, too, but I think it's outside the scope of the set.

Definitely not in the scope of the bd79124 support :)

Yours,
	-- Matti