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