On 30/06/2024 13:41, Jonathan Cameron wrote:
> On Mon, 24 Jun 2024 23:45:42 +0200
> Javier Carrasco <javier.carrasco.cruz@xxxxxxxxx> wrote:
>
>> On 26/05/2024 15:48, Jonathan Cameron wrote:
>>> On Thu, 23 May 2024 17:47:16 +0200
>>> Javier Carrasco <javier.carrasco.cruz@xxxxxxxxx> 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@xxxxxxxxxxxxxxx>
>>>> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@xxxxxxxxx>
...
>
> 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
