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

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

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

> -	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

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