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

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

[Jonathan Cameron]

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>

This looks like another instances of the lack of clarify about 
what device_for_each_child_node[_scoped]() guarantees about node availability.
On DT it guarantees the node is available as ultimately calls
of_get_next_available_child()

On ACPI it doesn't (I think).
For swnode, there isn't an obvious concept of available.

It would be much better if we reached some agreement on this and
hence could avoid using the fwnode variants just to get the _available_ form
as done here.  Or just add the device_for_each_available_child_node[_scoped]()
and call that in almost all cases.

In generic code, do we ever want to walk unavailable child nodes?

Jonathan





> ---
>  drivers/hwmon/ltc2992.c | 15 ++++-----------
>  1 file changed, 4 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/hwmon/ltc2992.c b/drivers/hwmon/ltc2992.c
> index d4a93223cd3b..3feee400ecf8 100644
> --- a/drivers/hwmon/ltc2992.c
> +++ b/drivers/hwmon/ltc2992.c
> @@ -855,32 +855,25 @@ static const struct regmap_config ltc2992_regmap_config = {
>  static int ltc2992_parse_dt(struct ltc2992_state *st)
>  {
>  	struct fwnode_handle *fwnode;
> -	struct fwnode_handle *child;
>  	u32 addr;
>  	u32 val;
>  	int ret;
>  
>  	fwnode = dev_fwnode(&st->client->dev);
>  
> -	fwnode_for_each_available_child_node(fwnode, child) {
> +	fwnode_for_each_available_child_node_scoped(fwnode, child) {
>  		ret = fwnode_property_read_u32(child, "reg", &addr);
> -		if (ret < 0) {
> -			fwnode_handle_put(child);
> +		if (ret < 0)
>  			return ret;
> -		}
>  
> -		if (addr > 1) {
> -			fwnode_handle_put(child);
> +		if (addr > 1)
>  			return -EINVAL;
> -		}
>  
>  		ret = fwnode_property_read_u32(child, "shunt-resistor-micro-ohms", &val);
>  		if (!ret) {
> -			if (!val) {
> -				fwnode_handle_put(child);
> +			if (!val)
>  				return dev_err_probe(&st->client->dev, -EINVAL,
>  						     "shunt resistor value cannot be zero\n");
> -			}
>  			st->r_sense_uohm[addr] = val;
>  		}
>  	}
> 

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

[Andy Shevchenko]

Sun, May 26, 2024 at 02:48:51PM +0100, Jonathan Cameron kirjoitti:
> 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>
> 
> This looks like another instances of the lack of clarify about 
> what device_for_each_child_node[_scoped]() guarantees about node availability.
> On DT it guarantees the node is available as ultimately calls
> of_get_next_available_child()
> 
> On ACPI it doesn't (I think).
> For swnode, there isn't an obvious concept of available.
> 
> It would be much better if we reached some agreement on this and
> hence could avoid using the fwnode variants just to get the _available_ form
> as done here.

> Or just add the device_for_each_available_child_node[_scoped]()
> and call that in almost all cases.

device_for_each*() _implies_ availability. You need to talk to Rob about all
this. The design of the device_for_each*() was exactly done in accordance with
his suggestions...

> In generic code, do we ever want to walk unavailable child nodes?

...which are most likely like your question here, i.e. why we ever need to
traverse over unavailable nodes.


-- 
With Best Regards,
Andy Shevchenko

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

[Jonathan Cameron]

On Mon, 27 May 2024 17:30:10 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> Sun, May 26, 2024 at 02:48:51PM +0100, Jonathan Cameron kirjoitti:
> > 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>  
> > 
> > This looks like another instances of the lack of clarify about 
> > what device_for_each_child_node[_scoped]() guarantees about node availability.
> > On DT it guarantees the node is available as ultimately calls
> > of_get_next_available_child()
> > 
> > On ACPI it doesn't (I think).
> > For swnode, there isn't an obvious concept of available.
> > 
> > It would be much better if we reached some agreement on this and
> > hence could avoid using the fwnode variants just to get the _available_ form
> > as done here.  
> 
> > Or just add the device_for_each_available_child_node[_scoped]()
> > and call that in almost all cases.  
> 
> device_for_each*() _implies_ availability. You need to talk to Rob about all
> this. The design of the device_for_each*() was exactly done in accordance with
> his suggestions...
> 

Does it imply that for ACPI? I can't find a query of _STA in the callbacks
(which is there for the for fwnode_*available calls.
Mind you it wouldn't be the first time I've missed something in the ACPI parsing
code, so maybe it is there indirectly.

I know from previous discussions that the DT version was intentional, but
I'm nervous that the same assumptions don't apply to ACPI.

> > In generic code, do we ever want to walk unavailable child nodes?  
> 
> ...which are most likely like your question here, i.e. why we ever need to
> traverse over unavailable nodes.
> 

Jonathan

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

[Andy Shevchenko]

On Mon, May 27, 2024 at 03:57:17PM +0100, Jonathan Cameron wrote:
> On Mon, 27 May 2024 17:30:10 +0300
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > Sun, May 26, 2024 at 02:48:51PM +0100, Jonathan Cameron kirjoitti:
> > > On Thu, 23 May 2024 17:47:16 +0200
> > > Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote:

...

> > > This looks like another instances of the lack of clarify about 
> > > what device_for_each_child_node[_scoped]() guarantees about node availability.
> > > On DT it guarantees the node is available as ultimately calls
> > > of_get_next_available_child()
> > > 
> > > On ACPI it doesn't (I think).
> > > For swnode, there isn't an obvious concept of available.
> > > 
> > > It would be much better if we reached some agreement on this and
> > > hence could avoid using the fwnode variants just to get the _available_ form
> > > as done here.  
> > 
> > > Or just add the device_for_each_available_child_node[_scoped]()
> > > and call that in almost all cases.  
> > 
> > device_for_each*() _implies_ availability. You need to talk to Rob about all
> > this. The design of the device_for_each*() was exactly done in accordance with
> > his suggestions...
> 
> Does it imply that for ACPI? I can't find a query of _STA in the callbacks
> (which is there for the for fwnode_*available calls.

IIRC for ACPI/swnode the availability is always "yes" as long as property can
be found. Basically it means the fwnode_*() == fwnode_*available() for these
back-ends.

AFAIU ACPI concept here is that once parsed and namespaced (in terms of putting
the respective part of description table into ACPI namespace) it's lways
available. Otherwise it's not, but at the same time the respective child node
(property) may not be found

> Mind you it wouldn't be the first time I've missed something in the ACPI parsing
> code, so maybe it is there indirectly.

I might have a weak memory, but see my understanding above.

> I know from previous discussions that the DT version was intentional, but
> I'm nervous that the same assumptions don't apply to ACPI.
> 
> > > In generic code, do we ever want to walk unavailable child nodes?  
> > 
> > ...which are most likely like your question here, i.e. why we ever need to
> > traverse over unavailable nodes.

-- 
With Best Regards,
Andy Shevchenko

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

[Javier Carrasco]

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>
> 
> This looks like another instances of the lack of clarify about 
> what device_for_each_child_node[_scoped]() guarantees about node availability.
> On DT it guarantees the node is available as ultimately calls
> of_get_next_available_child()
> 
> On ACPI it doesn't (I think).
> For swnode, there isn't an obvious concept of available.
> 
> It would be much better if we reached some agreement on this and
> hence could avoid using the fwnode variants just to get the _available_ form
> as done here.  Or just add the device_for_each_available_child_node[_scoped]()
> and call that in almost all cases.
> 
> In generic code, do we ever want to walk unavailable child nodes?
> 
> Jonathan
> 

Hi,

if I did not miss anything, the discussion about the convenience of the
fwnode_for_each_available_child_node_scoped() macro stalled without a
clear outcome.

At this point there are multiple users of both
fwnode_for_each_child_node() and fwnode_for_each_available_child_node(),
and I wonder how many of them use the non-scoped version for a different
reason than not having/knowing the _available_ variant back then.

Maybe touching that now could turn into regressions if someone is just
ignoring that some nodes are actually disabled. Their bad, but still
painful. But maybe there is a better reason to have both macros I don't
know.

As I am still interested in this matter for new users that only want to
iterate over available nodes, and I want to have a scoped solution, I
would like to revive this discussion.

Thanks and best regards,
Javier Carrasco

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

[Nuno Sá]

On Mon, 2024-05-27 at 17:30 +0300, Andy Shevchenko wrote:
> Sun, May 26, 2024 at 02:48:51PM +0100, Jonathan Cameron kirjoitti:
> > 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>
> > 
> > This looks like another instances of the lack of clarify about 
> > what device_for_each_child_node[_scoped]() guarantees about node
> > availability.
> > On DT it guarantees the node is available as ultimately calls
> > of_get_next_available_child()
> > 
> > On ACPI it doesn't (I think).
> > For swnode, there isn't an obvious concept of available.
> > 
> > It would be much better if we reached some agreement on this and
> > hence could avoid using the fwnode variants just to get the _available_ form
> > as done here.
> 
> > Or just add the device_for_each_available_child_node[_scoped]()
> > and call that in almost all cases.
> 
> device_for_each*() _implies_ availability. You need to talk to Rob about all
> this. The design of the device_for_each*() was exactly done in accordance with
> his suggestions...
> 
> > In generic code, do we ever want to walk unavailable child nodes?
> 
> ...which are most likely like your question here, i.e. why we ever need to
> traverse over unavailable nodes.
> 

I have some vague idea of Rob talking about CPUs being one of the reasons for
the current design. Don't remember for sure. At least (if not already) having
this clearly documented would be nice.

- Nuno Sá

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

[Jonathan Cameron]

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>  
> > 
> > This looks like another instances of the lack of clarify about 
> > what device_for_each_child_node[_scoped]() guarantees about node availability.
> > On DT it guarantees the node is available as ultimately calls
> > of_get_next_available_child()
> > 
> > On ACPI it doesn't (I think).
> > For swnode, there isn't an obvious concept of available.
> > 
> > It would be much better if we reached some agreement on this and
> > hence could avoid using the fwnode variants just to get the _available_ form
> > as done here.  Or just add the device_for_each_available_child_node[_scoped]()
> > and call that in almost all cases.
> > 
> > In generic code, do we ever want to walk unavailable child nodes?
> > 
> > Jonathan
> >   
> 
> Hi,
> 
> if I did not miss anything, the discussion about the convenience of the
> fwnode_for_each_available_child_node_scoped() macro stalled without a
> clear outcome.
> 
> At this point there are multiple users of both
> fwnode_for_each_child_node() and fwnode_for_each_available_child_node(),
> and I wonder how many of them use the non-scoped version for a different
> reason than not having/knowing the _available_ variant back then.
> 
> Maybe touching that now could turn into regressions if someone is just
> ignoring that some nodes are actually disabled. Their bad, but still
> painful. But maybe there is a better reason to have both macros I don't
> know.
> 
> As I am still interested in this matter for new users that only want to
> iterate over available nodes, and I want to have a scoped solution, I
> would like to revive this discussion.

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

> 
> Thanks and best regards,
> Javier Carrasco

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

[Javier Carrasco]

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