Date: Mon, 1 Jul 2024 11:35:41 +0200
From: Javier Carrasco <javier.carrasco.cruz@...il.com>
To: Jonathan Cameron <jic23@...nel.org>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
 "Rafael J. Wysocki" <rafael@...nel.org>,
 Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
 Daniel Scally <djrscally@...il.com>,
 Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
 Sakari Ailus <sakari.ailus@...ux.intel.com>, Jean Delvare
 <jdelvare@...e.com>, Guenter Roeck <linux@...ck-us.net>,
 Antoniu Miclaus <antoniu.miclaus@...log.com>, linux-acpi@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-hwmon@...r.kernel.org,
 Rob Herring <robh@...nel.org>, devicetree@...r.kernel.org
Subject: Re: [PATCH v2 3/3] hwmon: (ltc2992) Use
 fwnode_for_each_available_child_node_scoped()

On 30/06/2024 13:41, Jonathan Cameron wrote:
> On Mon, 24 Jun 2024 23:45:42 +0200
> Javier Carrasco <javier.carrasco.cruz@...il.com> wrote:
> 
>> On 26/05/2024 15:48, Jonathan Cameron wrote:
>>> On Thu, 23 May 2024 17:47:16 +0200
>>> Javier Carrasco <javier.carrasco.cruz@...il.com> wrote:
>>>   
>>>> The scoped version of the fwnode_for_each_available_child_node() macro
>>>> automates object recfount decrement, avoiding possible memory leaks
>>>> in new error paths inside the loop like it happened when
>>>> commit '10b029020487 ("hwmon: (ltc2992) Avoid division by zero")'
>>>> was added.
>>>>
>>>> The new macro removes the need to manually call fwnode_handle_put() in
>>>> the existing error paths and in any future addition. It also removes the
>>>> need for the current child node declaration as well, as it is internally
>>>> declared.
>>>>
>>>> Reviewed-by: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
>>>> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@...il.com>  

...

> 
> Straw man for people to shoot at:
> 
> I think where possible rely on device_for_each_child_node[_scoped]()
> actually meaning the available nodes.  In cases where it applies that
> is normally cleaner anyway.
> 
> If you find cases where there is no relevant device (I'm sure there are some)
> just provide fwnode_for_each_available_child_node() and not the non-available
> one.  If that means switching some drivers to use the available form as
> part of cleanups, at that point we consider if there is a special reason
> it actually wants the non available modes.
> 
> Ideally we also add documentation to say the device_for_each_child_node()
> will (at least mostly) not consider non available nodes.  It might
> be always, I'm still personally not sure on that!
> 
> Jonathan

There are multiple cases where fwnode_for_each_available_child_node()
seems to be used just to get a macro that explicitly guarantees node
availability i.e. they retrieve ’fwnode’ out of ’device' by means of
dev_fwnode() to pass it to the loop.

In those cases, device_for_each_child_node[_scoped]() could be used if
it guarantees availability, which no one could refute so far.

On the other hand, there are other uses that do need the fwnode_*
variants because they iterate over nodes inside another node which is
usually retrieved via device_get_named_child_node().

If there are no objections or better proposals, I will proceed as follows:

1. Document that device_for_each_child_node() means availability.
2. Use device_for_each_child_node[_scoped]() instead of the fwnode_*
variant where it makes sense.
3. Provide fwnode_*_scoped() macros.
4. Use the new macros where needed.
5. Use fwnode_for_each_available_child_node() as the default where
unavailable nodes are not explicitly required.

Any additional feedback, especially to clarify _availability_ in the
device_for_each_child_node macros, or to provide a case where
unavailable nodes must be considered (Nuno mentioned CPUs, but just as a
vague idea) is more than welcome.

Best regards,
Javier Carrasco

Powered by blists - more mailing lists