| Message ID | 20241019-mv88e6xxx_chip-fwnode_handle_put-v1-1-fc92c4f16831@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: dsa: mv88e6xxx: fix unreleased fwnode_handle in setup_port() | expand |
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
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
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
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
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"); }
'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(+) --- base-commit: 160a810b2a8588187ec2b1536d0355c0aab8981c change-id: 20241019-mv88e6xxx_chip-fwnode_handle_put-acc4ed165268 Best regards,