Re: [PATCH v2 3/3] hwmon: (ltc2992) Use fwnode_for_each_available_child_node_scoped()

[Javier Carrasco <javier.carrasco.cruz@gmail.com>]

On 30/06/2024 13:41, Jonathan Cameron wrote:
> On Mon, 24 Jun 2024 23:45:42 +0200
> Javier Carrasco <javier.carrasco.cruz@gmail.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@gmail.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@linux.intel.com>
>>>> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.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