| Message ID | 20241008-mv88e6xxx_leds_fwnode_put-v1-3-cfd7758cd176@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net, device property: fix led node releases in mv88e6xxx with new macro | expand |
> - 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
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?
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
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);
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(-)