[PATCH net-next] net: dsa: mv88e6xxx: fix unreleased fwnode_handle in setup_port()

[PATCH net-next] net: dsa: mv88e6xxx: fix unreleased fwnode_handle in setup_port()

[Javier Carrasco]

'ports_fwnode' is initialized via device_get_named_child_node(), which
requires a call to fwnode_handle_put() when the variable is no longer
required to avoid leaking memory.

Add the missing fwnode_handle_put() after 'ports_fwnode' has been used
and is no longer required.

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

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index da19a3b05549..8c6797af8777 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -3379,6 +3379,7 @@ static int mv88e6xxx_setup_port(struct mv88e6xxx_chip *chip, int port)
 				break;
 			}
 		}
+		fwnode_handle_put(ports_fwnode);
 	} else {
 		dev_dbg(chip->dev, "no ethernet ports node defined for the device\n");
 	}

---
base-commit: 160a810b2a8588187ec2b1536d0355c0aab8981c
change-id: 20241019-mv88e6xxx_chip-fwnode_handle_put-acc4ed165268

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

Re: [PATCH net-next] net: dsa: mv88e6xxx: fix unreleased fwnode_handle in setup_port()

[Andrew Lunn]

On Sat, Oct 19, 2024 at 10:16:49PM +0200, Javier Carrasco wrote:
> 'ports_fwnode' is initialized via device_get_named_child_node(), which
> requires a call to fwnode_handle_put() when the variable is no longer
> required to avoid leaking memory.
> 
> Add the missing fwnode_handle_put() after 'ports_fwnode' has been used
> and is no longer required.

As you point out, the handle is obtained with
device_get_named_child_node(). It seems odd to use a fwnode_ function
not a device_ function to release the handle. Is there a device_
function?

	Andrew

Re: [PATCH net-next] net: dsa: mv88e6xxx: fix unreleased fwnode_handle in setup_port()

[Javier Carrasco]

On 19/10/2024 23:59, Andrew Lunn wrote:
> On Sat, Oct 19, 2024 at 10:16:49PM +0200, Javier Carrasco wrote:
>> 'ports_fwnode' is initialized via device_get_named_child_node(), which
>> requires a call to fwnode_handle_put() when the variable is no longer
>> required to avoid leaking memory.
>>
>> Add the missing fwnode_handle_put() after 'ports_fwnode' has been used
>> and is no longer required.
> 
> As you point out, the handle is obtained with
> device_get_named_child_node(). It seems odd to use a fwnode_ function
> not a device_ function to release the handle. Is there a device_
> function?
> 
> 	Andrew


Hi Andrew,

device_get_named_child_node() receives a pointer to a *device*, and
returns a child node (a pointer to an *fwnode_handle*). That is what has
to be released, and therefore fwnode_handle_put() is the right one.

Note that device_get_named_child_node() documents how to release the
fwnode pointer:

"The caller is responsible for calling fwnode_handle_put() on the
returned fwnode pointer."

Best regards,
Javier Carrasco

Re: [PATCH net-next] net: dsa: mv88e6xxx: fix unreleased fwnode_handle in setup_port()

[Andrew Lunn]

On Sun, Oct 20, 2024 at 12:21:07AM +0200, Javier Carrasco wrote:
> On 19/10/2024 23:59, Andrew Lunn wrote:
> > On Sat, Oct 19, 2024 at 10:16:49PM +0200, Javier Carrasco wrote:
> >> 'ports_fwnode' is initialized via device_get_named_child_node(), which
> >> requires a call to fwnode_handle_put() when the variable is no longer
> >> required to avoid leaking memory.
> >>
> >> Add the missing fwnode_handle_put() after 'ports_fwnode' has been used
> >> and is no longer required.
> > 
> > As you point out, the handle is obtained with
> > device_get_named_child_node(). It seems odd to use a fwnode_ function
> > not a device_ function to release the handle. Is there a device_
> > function?
> > 
> > 	Andrew
> 
> 
> Hi Andrew,
> 
> device_get_named_child_node() receives a pointer to a *device*, and
> returns a child node (a pointer to an *fwnode_handle*). That is what has
> to be released, and therefore fwnode_handle_put() is the right one.
> 
> Note that device_get_named_child_node() documents how to release the
> fwnode pointer:
> 
> "The caller is responsible for calling fwnode_handle_put() on the
> returned fwnode pointer."

O.K. I just don't like asymmetric APIs. They often lead to bugs, just
look wrong, and make reviewers ask questions...

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

    Andrew

Re: [PATCH net-next] net: dsa: mv88e6xxx: fix unreleased fwnode_handle in setup_port()

[Linus Walleij]

On Sat, Oct 19, 2024 at 10:16 PM Javier Carrasco
<javier.carrasco.cruz@gmail.com> wrote:

> 'ports_fwnode' is initialized via device_get_named_child_node(), which
> requires a call to fwnode_handle_put() when the variable is no longer
> required to avoid leaking memory.
>
> Add the missing fwnode_handle_put() after 'ports_fwnode' has been used
> and is no longer required.
>
> Fixes: 94a2a84f5e9e ("net: dsa: mv88e6xxx: Support LED control")
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>

I was as puzzled as Andrew but I buy the explanation.
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH net-next] net: dsa: mv88e6xxx: fix unreleased fwnode_handle in setup_port()

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net-next.git (main)
by David S. Miller <davem@davemloft.net>:

On Sat, 19 Oct 2024 22:16:49 +0200 you wrote:
> 'ports_fwnode' is initialized via device_get_named_child_node(), which
> requires a call to fwnode_handle_put() when the variable is no longer
> required to avoid leaking memory.
> 
> Add the missing fwnode_handle_put() after 'ports_fwnode' has been used
> and is no longer required.
> 
> [...]

Here is the summary with links:
  - [net-next] net: dsa: mv88e6xxx: fix unreleased fwnode_handle in setup_port()
    https://git.kernel.org/netdev/net-next/c/b8ee7a11c754

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html