[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <cffb5885-3cbc-480c-ab6d-4a442d1afb8a@gmail.com>
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