[PATCH net-next 0/3] net, device property: fix led node releases in mv88e6xxx with new macro

[PATCH net-next 0/3] net, device property: fix led node releases in mv88e6xxx with new macro

[Javier Carrasco]

This series introduces the scoped variant of
fwnode_for_each_available_child_node() to avoid the common issue the
non-scoped variant triggers: forgetting the required fwnode_handle_put()
upon early exits. It is strongly based on the existing
device_for_each_child_node_scoped(), using
fwnode_get_next_available_child_node() instead, and of course fwnode
instead of dev.

A fix with this new macro has been proposed in this series, as well as a
second fix for a node outside the loop by means of the __free()
automatic cleanup mechanism. Given that the bugs were recently
introduced, new approaches can be used without affecting stable kernels.
Of course the issue could be solved by adding multiple calls to
fwnode_handle_put(), but the scoped macros and the automatic cleanup has
already showed multiple times that they can save us from introducing
bugs and also help us fix existing ones.

Note that the declaration of 'leds' has been moved to its initialization
as it is preferred when using this mechanism to avoid releasing
uninitialized nodes. It could be left on top with a NULL initialization,
but that pattern is less common.

A more important modification is the use of the _available_ variant of
the macro, as it seems that there is no need to walk over unavailable
nodes. If that is not the case, please let me know why unavailable nodes
are required, as a scoped variant of the current loop could be
introduced instead.

This series has been compile-checked and statically analyzed, as I
don't have the hardware to test it. I have been using the scoped macro
for a while downstream, but that is not saying much, and actual tests
with the real hardware would be more than welcome.

Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
Javier Carrasco (3):
      device property: Introduce fwnode_for_each_available_child_node_scoped()
      net: dsa: mv88e6xxx: leds: fix led refcount in error path
      net: dsa: mv88e6xxx: leds: fix leds refcount

 drivers/net/dsa/mv88e6xxx/leds.c | 6 +++---
 include/linux/property.h         | 5 +++++
 2 files changed, 8 insertions(+), 3 deletions(-)
---
base-commit: 489cee4caeba4f70a29b7215cfd18152dcadab7f
change-id: 20241008-mv88e6xxx_leds_fwnode_put-93946e7a87a8

Best regards,
-- 
Javier Carrasco <javier.carrasco.cruz@gmail.com>

[PATCH net-next 1/3] device property: Introduce fwnode_for_each_available_child_node_scoped()

[Javier Carrasco]

Introduce the scoped variant of the
fwnode_for_each_available_child_node() to automatically decrement the
child's refcount when it goes out of scope, removing the need for
explicit calls to fwnode_handle_put().

Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
 include/linux/property.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/linux/property.h b/include/linux/property.h
index 61fc20e5f81f..b37508ecf606 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -168,6 +168,11 @@ struct fwnode_handle *fwnode_get_next_available_child_node(
 	for (child = fwnode_get_next_available_child_node(fwnode, NULL); child;\
 	     child = fwnode_get_next_available_child_node(fwnode, child))
 
+#define fwnode_for_each_available_child_node_scoped(fwnode, child)	       \
+	for (struct fwnode_handle *child __free(fwnode_handle) =	       \
+		fwnode_get_next_available_child_node(fwnode, NULL); child;     \
+	     child = fwnode_get_next_available_child_node(fwnode, child))
+
 struct fwnode_handle *device_get_next_child_node(const struct device *dev,
 						 struct fwnode_handle *child);
 

-- 
2.43.0

Re: [PATCH net-next 1/3] device property: Introduce fwnode_for_each_available_child_node_scoped()

[Andy Shevchenko]

On Tue, Oct 08, 2024 at 06:10:27PM +0200, Javier Carrasco wrote:
> Introduce the scoped variant of the
> fwnode_for_each_available_child_node() to automatically decrement the
> child's refcount when it goes out of scope, removing the need for
> explicit calls to fwnode_handle_put().

...

> +#define fwnode_for_each_available_child_node_scoped(fwnode, child)	       \
> +	for (struct fwnode_handle *child __free(fwnode_handle) =	       \
> +		fwnode_get_next_available_child_node(fwnode, NULL); child;     \
> +	     child = fwnode_get_next_available_child_node(fwnode, child))

I like the wrapping you have done here.
Can you align the device_for_each_child_node_scoped() to follow your variant?

(probably in an additional patch)

For this one
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH net-next 1/3] device property: Introduce fwnode_for_each_available_child_node_scoped()

[Sakari Ailus]

Hi Javier,

On Tue, Oct 08, 2024 at 06:10:27PM +0200, Javier Carrasco wrote:
> Introduce the scoped variant of the
> fwnode_for_each_available_child_node() to automatically decrement the
> child's refcount when it goes out of scope, removing the need for
> explicit calls to fwnode_handle_put().
> 
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
> ---
>  include/linux/property.h | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/include/linux/property.h b/include/linux/property.h
> index 61fc20e5f81f..b37508ecf606 100644
> --- a/include/linux/property.h
> +++ b/include/linux/property.h
> @@ -168,6 +168,11 @@ struct fwnode_handle *fwnode_get_next_available_child_node(
>  	for (child = fwnode_get_next_available_child_node(fwnode, NULL); child;\
>  	     child = fwnode_get_next_available_child_node(fwnode, child))
>  
> +#define fwnode_for_each_available_child_node_scoped(fwnode, child)	       \
> +	for (struct fwnode_handle *child __free(fwnode_handle) =	       \
> +		fwnode_get_next_available_child_node(fwnode, NULL); child;     \
> +	     child = fwnode_get_next_available_child_node(fwnode, child))
> +

On OF, the implementation of the .get_next_child_node() fwnode op is:

static struct fwnode_handle *
of_fwnode_get_next_child_node(const struct fwnode_handle *fwnode,
                              struct fwnode_handle *child)
{
        return of_fwnode_handle(of_get_next_available_child(to_of_node(fwnode),
                                                            to_of_node(child)));
}

On ACPI we currently have .device_is_available() returning false but that
probably should be returning true instead (it's been virtually unused
previously).

That makes fwnode_get_next_available_child_node() and
fwnode_get_next_child_node() equivalent on both ACPI and OF. Presumably
creating unavailable nodes would be useless on swnode, too.

So my question is: what do we gain by adding all these fwnode_*available()
helpers?

>  struct fwnode_handle *device_get_next_child_node(const struct device *dev,
>  						 struct fwnode_handle *child);

-- 
Regards,

Sakari Ailus

Re: [PATCH net-next 1/3] device property: Introduce fwnode_for_each_available_child_node_scoped()

[Javier Carrasco]

On 11/10/2024 07:39, Sakari Ailus wrote:
> Hi Javier,
> 
> On Tue, Oct 08, 2024 at 06:10:27PM +0200, Javier Carrasco wrote:
>> Introduce the scoped variant of the
>> fwnode_for_each_available_child_node() to automatically decrement the
>> child's refcount when it goes out of scope, removing the need for
>> explicit calls to fwnode_handle_put().
>>
>> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
>> ---
>>  include/linux/property.h | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/include/linux/property.h b/include/linux/property.h
>> index 61fc20e5f81f..b37508ecf606 100644
>> --- a/include/linux/property.h
>> +++ b/include/linux/property.h
>> @@ -168,6 +168,11 @@ struct fwnode_handle *fwnode_get_next_available_child_node(
>>  	for (child = fwnode_get_next_available_child_node(fwnode, NULL); child;\
>>  	     child = fwnode_get_next_available_child_node(fwnode, child))
>>  
>> +#define fwnode_for_each_available_child_node_scoped(fwnode, child)	       \
>> +	for (struct fwnode_handle *child __free(fwnode_handle) =	       \
>> +		fwnode_get_next_available_child_node(fwnode, NULL); child;     \
>> +	     child = fwnode_get_next_available_child_node(fwnode, child))
>> +
> 
> On OF, the implementation of the .get_next_child_node() fwnode op is:
> 
> static struct fwnode_handle *
> of_fwnode_get_next_child_node(const struct fwnode_handle *fwnode,
>                               struct fwnode_handle *child)
> {
>         return of_fwnode_handle(of_get_next_available_child(to_of_node(fwnode),
>                                                             to_of_node(child)));
> }
> 
> On ACPI we currently have .device_is_available() returning false but that
> probably should be returning true instead (it's been virtually unused
> previously).
> 
> That makes fwnode_get_next_available_child_node() and
> fwnode_get_next_child_node() equivalent on both ACPI and OF. Presumably
> creating unavailable nodes would be useless on swnode, too.
> 
> So my question is: what do we gain by adding all these fwnode_*available()
> helpers?
> 
>>  struct fwnode_handle *device_get_next_child_node(const struct device *dev,
>>  						 struct fwnode_handle *child);
> 

Hi Sakari, thanks for your feedback.

I thought that the difference is not in OF (which either way ends up
calling __of_device_is_available()), but in ACPI.

For fwnode_for_each_child_node(), the ACPI callback is
acpi_get_next_subnode(), and I don't see that the device_is_available()
callback is used in that case.

For fwnode_for_each_available_child_node(),
fwnode_get_next_available_child_node() is used, which checks
fwnode_device_is_available(), which then calls device_is_available().

What's the catch?

Thanks again and best regards,
Javier Carrasco

Re: [PATCH net-next 1/3] device property: Introduce fwnode_for_each_available_child_node_scoped()

[Sakari Ailus]

Hi Javier,

On Fri, Oct 11, 2024 at 10:34:32AM +0200, Javier Carrasco wrote:
> On 11/10/2024 07:39, Sakari Ailus wrote:
> > Hi Javier,
> > 
> > On Tue, Oct 08, 2024 at 06:10:27PM +0200, Javier Carrasco wrote:
> >> Introduce the scoped variant of the
> >> fwnode_for_each_available_child_node() to automatically decrement the
> >> child's refcount when it goes out of scope, removing the need for
> >> explicit calls to fwnode_handle_put().
> >>
> >> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
> >> ---
> >>  include/linux/property.h | 5 +++++
> >>  1 file changed, 5 insertions(+)
> >>
> >> diff --git a/include/linux/property.h b/include/linux/property.h
> >> index 61fc20e5f81f..b37508ecf606 100644
> >> --- a/include/linux/property.h
> >> +++ b/include/linux/property.h
> >> @@ -168,6 +168,11 @@ struct fwnode_handle *fwnode_get_next_available_child_node(
> >>  	for (child = fwnode_get_next_available_child_node(fwnode, NULL); child;\
> >>  	     child = fwnode_get_next_available_child_node(fwnode, child))
> >>  
> >> +#define fwnode_for_each_available_child_node_scoped(fwnode, child)	       \
> >> +	for (struct fwnode_handle *child __free(fwnode_handle) =	       \
> >> +		fwnode_get_next_available_child_node(fwnode, NULL); child;     \
> >> +	     child = fwnode_get_next_available_child_node(fwnode, child))
> >> +
> > 
> > On OF, the implementation of the .get_next_child_node() fwnode op is:
> > 
> > static struct fwnode_handle *
> > of_fwnode_get_next_child_node(const struct fwnode_handle *fwnode,
> >                               struct fwnode_handle *child)
> > {
> >         return of_fwnode_handle(of_get_next_available_child(to_of_node(fwnode),
> >                                                             to_of_node(child)));
> > }
> > 
> > On ACPI we currently have .device_is_available() returning false but that
> > probably should be returning true instead (it's been virtually unused
> > previously).
> > 
> > That makes fwnode_get_next_available_child_node() and
> > fwnode_get_next_child_node() equivalent on both ACPI and OF. Presumably
> > creating unavailable nodes would be useless on swnode, too.
> > 
> > So my question is: what do we gain by adding all these fwnode_*available()
> > helpers?
> > 
> >>  struct fwnode_handle *device_get_next_child_node(const struct device *dev,
> >>  						 struct fwnode_handle *child);
> > 
> 
> Hi Sakari, thanks for your feedback.
> 
> I thought that the difference is not in OF (which either way ends up
> calling __of_device_is_available()), but in ACPI.
> 
> For fwnode_for_each_child_node(), the ACPI callback is
> acpi_get_next_subnode(), and I don't see that the device_is_available()
> callback is used in that case.

fwnode_get_next_available_child_node() also calls
fwnode_device_is_available() and that returns false on all non-device nodes
right now. As noted above, fwnode_device_is_available() should probably
return true for non-device nodes on ACPI. I'll post a patch.

> 
> For fwnode_for_each_available_child_node(),
> fwnode_get_next_available_child_node() is used, which checks
> fwnode_device_is_available(), which then calls device_is_available().
> 
> What's the catch?

-- 
Kind regards,

Sakari Ailus

Re: [PATCH net-next 1/3] device property: Introduce fwnode_for_each_available_child_node_scoped()

[Javier Carrasco]

On 11/10/2024 11:54, Sakari Ailus wrote:
> Hi Javier,
> 
> On Fri, Oct 11, 2024 at 10:34:32AM +0200, Javier Carrasco wrote:
>> On 11/10/2024 07:39, Sakari Ailus wrote:
>>> Hi Javier,
>>>
>>> On Tue, Oct 08, 2024 at 06:10:27PM +0200, Javier Carrasco wrote:
>>>> Introduce the scoped variant of the
>>>> fwnode_for_each_available_child_node() to automatically decrement the
>>>> child's refcount when it goes out of scope, removing the need for
>>>> explicit calls to fwnode_handle_put().
>>>>
>>>> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
>>>> ---
>>>>  include/linux/property.h | 5 +++++
>>>>  1 file changed, 5 insertions(+)
>>>>
>>>> diff --git a/include/linux/property.h b/include/linux/property.h
>>>> index 61fc20e5f81f..b37508ecf606 100644
>>>> --- a/include/linux/property.h
>>>> +++ b/include/linux/property.h
>>>> @@ -168,6 +168,11 @@ struct fwnode_handle *fwnode_get_next_available_child_node(
>>>>  	for (child = fwnode_get_next_available_child_node(fwnode, NULL); child;\
>>>>  	     child = fwnode_get_next_available_child_node(fwnode, child))
>>>>  
>>>> +#define fwnode_for_each_available_child_node_scoped(fwnode, child)	       \
>>>> +	for (struct fwnode_handle *child __free(fwnode_handle) =	       \
>>>> +		fwnode_get_next_available_child_node(fwnode, NULL); child;     \
>>>> +	     child = fwnode_get_next_available_child_node(fwnode, child))
>>>> +
>>>
>>> On OF, the implementation of the .get_next_child_node() fwnode op is:
>>>
>>> static struct fwnode_handle *
>>> of_fwnode_get_next_child_node(const struct fwnode_handle *fwnode,
>>>                               struct fwnode_handle *child)
>>> {
>>>         return of_fwnode_handle(of_get_next_available_child(to_of_node(fwnode),
>>>                                                             to_of_node(child)));
>>> }
>>>
>>> On ACPI we currently have .device_is_available() returning false but that
>>> probably should be returning true instead (it's been virtually unused
>>> previously).
>>>
>>> That makes fwnode_get_next_available_child_node() and
>>> fwnode_get_next_child_node() equivalent on both ACPI and OF. Presumably
>>> creating unavailable nodes would be useless on swnode, too.
>>>
>>> So my question is: what do we gain by adding all these fwnode_*available()
>>> helpers?
>>>
>>>>  struct fwnode_handle *device_get_next_child_node(const struct device *dev,
>>>>  						 struct fwnode_handle *child);
>>>
>>
>> Hi Sakari, thanks for your feedback.
>>
>> I thought that the difference is not in OF (which either way ends up
>> calling __of_device_is_available()), but in ACPI.
>>
>> For fwnode_for_each_child_node(), the ACPI callback is
>> acpi_get_next_subnode(), and I don't see that the device_is_available()
>> callback is used in that case.
> 
> fwnode_get_next_available_child_node() also calls
> fwnode_device_is_available() and that returns false on all non-device nodes
> right now. As noted above, fwnode_device_is_available() should probably
> return true for non-device nodes on ACPI. I'll post a patch.
> 

fwnode_device_is_available() is indeed called in
fwnode_get_next_available_child_node(), as I stated a couple of lines below.

My question on the other hand was how that is called in
fwnode_for_each_child_node(), as I could not see any call to check
availability in acpi_get_next_subnode().
That is what confused me about the _available_ macros being the same as
their counterparts without the _available_.

Could you please clarify that? Thanks again.

>>
>> For fwnode_for_each_available_child_node(),
>> fwnode_get_next_available_child_node() is used, which checks
>> fwnode_device_is_available(), which then calls device_is_available().
>>
>> What's the catch?
> 

[PATCH net-next 2/3] net: dsa: mv88e6xxx: leds: fix led refcount in error path

[Javier Carrasco]

The 'led' fwnode_handle within fwnode_for_each_child_node() must be
released upon early exits by means of an explicit call to
fwnode_handle_put(), which in this case is missing.

Instead of adding the missing call, and considering that this driver was
recently introduced, use a scoped variant of the loop to automatically
decrement the child's refcount when it goes out of scope.

Note that the _avaialable_ version of the loop has been used, as there
is no apparent reason to walk over unavailable nodes.

Fixes: 94a2a84f5e9e ("net: dsa: mv88e6xxx: Support LED control")
Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
 drivers/net/dsa/mv88e6xxx/leds.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/leds.c b/drivers/net/dsa/mv88e6xxx/leds.c
index 1c88bfaea46b..92a57552beda 100644
--- a/drivers/net/dsa/mv88e6xxx/leds.c
+++ b/drivers/net/dsa/mv88e6xxx/leds.c
@@ -744,7 +744,7 @@ mv88e6xxx_led1_hw_control_get_device(struct led_classdev *ldev)
 
 int mv88e6xxx_port_setup_leds(struct mv88e6xxx_chip *chip, int port)
 {
-	struct fwnode_handle *led = NULL, *leds = NULL;
+	struct fwnode_handle *leds = NULL;
 	struct led_init_data init_data = { };
 	enum led_default_state state;
 	struct mv88e6xxx_port *p;
@@ -770,7 +770,7 @@ int mv88e6xxx_port_setup_leds(struct mv88e6xxx_chip *chip, int port)
 		return 0;
 	}
 
-	fwnode_for_each_child_node(leds, led) {
+	fwnode_for_each_available_child_node_scoped(leds, led) {
 		/* Reg represent the led number of the port, max 2
 		 * LEDs can be connected to each port, in some designs
 		 * only one LED is connected.

-- 
2.43.0

Re: [PATCH net-next 2/3] net: dsa: mv88e6xxx: leds: fix led refcount in error path

[Andrew Lunn]

On Tue, Oct 08, 2024 at 06:10:28PM +0200, Javier Carrasco wrote:
> The 'led' fwnode_handle within fwnode_for_each_child_node() must be
> released upon early exits by means of an explicit call to
> fwnode_handle_put(), which in this case is missing.
> 
> Instead of adding the missing call, and considering that this driver was
> recently introduced, use a scoped variant of the loop to automatically
> decrement the child's refcount when it goes out of scope.
> 
> Note that the _avaialable_ version of the loop has been used, as there
> is no apparent reason to walk over unavailable nodes.
> 
> Fixes: 94a2a84f5e9e ("net: dsa: mv88e6xxx: Support LED control")
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next 2/3] net: dsa: mv88e6xxx: leds: fix led refcount in error path

[Andy Shevchenko]

On Tue, Oct 08, 2024 at 06:10:28PM +0200, Javier Carrasco wrote:
> The 'led' fwnode_handle within fwnode_for_each_child_node() must be
> released upon early exits by means of an explicit call to
> fwnode_handle_put(), which in this case is missing.
> 
> Instead of adding the missing call, and considering that this driver was
> recently introduced, use a scoped variant of the loop to automatically
> decrement the child's refcount when it goes out of scope.
> 
> Note that the _avaialable_ version of the loop has been used, as there
> is no apparent reason to walk over unavailable nodes.

...

> -	struct fwnode_handle *led = NULL, *leds = NULL;
> +	struct fwnode_handle *leds = NULL;

Can it be const now?

-- 
With Best Regards,
Andy Shevchenko

[PATCH net-next 3/3] net: dsa: mv88e6xxx: leds: fix leds refcount

[Javier Carrasco]

The 'leds' fwnode_handle is initialized by calling
fwnode_get_named_child_node(), which requires an explicit call to
fwnode_handle_put() when the node is not required anymore.

Instead of adding the missing call, and considering that this driver was
recently introduced, use the automatic clenaup mechanism to release the
node when it goes out of scope.

Fixes: 94a2a84f5e9e ("net: dsa: mv88e6xxx: Support LED control")
Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
 drivers/net/dsa/mv88e6xxx/leds.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/leds.c b/drivers/net/dsa/mv88e6xxx/leds.c
index 92a57552beda..b9959e1f3c9e 100644
--- a/drivers/net/dsa/mv88e6xxx/leds.c
+++ b/drivers/net/dsa/mv88e6xxx/leds.c
@@ -744,7 +744,6 @@ mv88e6xxx_led1_hw_control_get_device(struct led_classdev *ldev)
 
 int mv88e6xxx_port_setup_leds(struct mv88e6xxx_chip *chip, int port)
 {
-	struct fwnode_handle *leds = NULL;
 	struct led_init_data init_data = { };
 	enum led_default_state state;
 	struct mv88e6xxx_port *p;
@@ -763,7 +762,8 @@ int mv88e6xxx_port_setup_leds(struct mv88e6xxx_chip *chip, int port)
 
 	dev = chip->dev;
 
-	leds = fwnode_get_named_child_node(p->fwnode, "leds");
+	struct fwnode_handle *leds __free(fwnode_handle) =
+		fwnode_get_named_child_node(p->fwnode, "leds");
 	if (!leds) {
 		dev_dbg(dev, "No Leds node specified in device tree for port %d!\n",
 			port);

-- 
2.43.0

Re: [PATCH net-next 3/3] net: dsa: mv88e6xxx: leds: fix leds refcount

[Andrew Lunn]

> -	leds = fwnode_get_named_child_node(p->fwnode, "leds");
> +	struct fwnode_handle *leds __free(fwnode_handle) =
> +		fwnode_get_named_child_node(p->fwnode, "leds");

https://docs.kernel.org/process/maintainer-netdev.html#using-device-managed-and-cleanup-h-constructs

  Low level cleanup constructs (such as __free()) can be used when
  building APIs and helpers, especially scoped iterators. However,
  direct use of __free() within networking core and drivers is
  discouraged. Similar guidance applies to declaring variables
  mid-function.

    Andrew

---
pw-bot: cr

Re: [PATCH net-next 3/3] net: dsa: mv88e6xxx: leds: fix leds refcount

[Javier Carrasco]

On 08/10/2024 18:40, Andrew Lunn wrote:
>> -	leds = fwnode_get_named_child_node(p->fwnode, "leds");
>> +	struct fwnode_handle *leds __free(fwnode_handle) =
>> +		fwnode_get_named_child_node(p->fwnode, "leds");
> 
> https://docs.kernel.org/process/maintainer-netdev.html#using-device-managed-and-cleanup-h-constructs
> 
>   Low level cleanup constructs (such as __free()) can be used when
>   building APIs and helpers, especially scoped iterators. However,
>   direct use of __free() within networking core and drivers is
>   discouraged. Similar guidance applies to declaring variables
>   mid-function.
> 
>     Andrew
> 
> ---
> pw-bot: cr


Hi Andrew,

Thanks for your review. I have seen that the __free() macro is used in
multiple net drivers, especially (but not only) __free(kfree). Why would
this one would be discouraged?

I would have nothing against declaring the variable at the top and
initializing it to NULL if that is the preferred way in the networking
subsystem, but the __free() macro seems to be well established, and it
simplifies the code.

Otherwise 4 calls to fwnode_handle_put() or a couple of goto jumps will
be required to get the same result. Moreover, if any other error path is
introduced, the mechanism will automatically account for it.

Best regards,
Javier Carrasco

Re: [PATCH net-next 3/3] net: dsa: mv88e6xxx: leds: fix leds refcount

[Andy Shevchenko]

On Tue, Oct 08, 2024 at 06:10:29PM +0200, Javier Carrasco wrote:
> The 'leds' fwnode_handle is initialized by calling
> fwnode_get_named_child_node(), which requires an explicit call to
> fwnode_handle_put() when the node is not required anymore.
> 
> Instead of adding the missing call, and considering that this driver was
> recently introduced, use the automatic clenaup mechanism to release the
> node when it goes out of scope.

...

> -	leds = fwnode_get_named_child_node(p->fwnode, "leds");
> +	struct fwnode_handle *leds __free(fwnode_handle) =
> +		fwnode_get_named_child_node(p->fwnode, "leds");

Can it be const?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH net-next 3/3] net: dsa: mv88e6xxx: leds: fix leds refcount

[Javier Carrasco]

On 10/10/2024 16:33, Andy Shevchenko wrote:
> On Tue, Oct 08, 2024 at 06:10:29PM +0200, Javier Carrasco wrote:
>> The 'leds' fwnode_handle is initialized by calling
>> fwnode_get_named_child_node(), which requires an explicit call to
>> fwnode_handle_put() when the node is not required anymore.
>>
>> Instead of adding the missing call, and considering that this driver was
>> recently introduced, use the automatic clenaup mechanism to release the
>> node when it goes out of scope.
> 
> ...
> 
>> -	leds = fwnode_get_named_child_node(p->fwnode, "leds");
>> +	struct fwnode_handle *leds __free(fwnode_handle) =
>> +		fwnode_get_named_child_node(p->fwnode, "leds");
> 
> Can it be const?
> 

Hi Andy,

in its current form, it could be const as its only assignment occurs in
its declaration. But if the final decision is moving it to the top and
giving it an initial NULL value, then that will not be possible for
obvious reasons.

I am fine with any of those options for v2.

Best regards,
Javier Carrasco