| Message ID | 20240706-device_for_each_child_node-available-v1-0-8a3f7615e41c@gmail.com |
|---|---|
| Headers | show |
| Series | use device_for_each_child_node() to access device child nodes | expand |
On Sat, 06 Jul 2024 17:23:33 +0200 Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote: > There have been some misconceptions about this macro, which iterates > over available child nodes from different backends. > > As that is not obvious by its name, some users have opted for the > `fwnode_for_each_available_child_node()` macro instead. > That requires an unnecessary, explicit access to the fwnode member > of the device structure. > > Passing the device to `device_for_each_child_node()` is shorter, > reflects more clearly the nature of the child nodes, and renders the > same result. > > In general, `fwnode_for_each_available_child_node()` should be used > whenever the parent node of the children to iterate over is a firmware > node, and not the device itself. > > Document the `device_for_each_child node(dev, child)` macro to clarify > its functionality. > > Suggested-by: Jonathan Cameron <jic23@kernel.org> > Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com> LGTM but I think needs at least a DT and ACPI ack. Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> One trivial tweak inline. > --- > include/linux/property.h | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/include/linux/property.h b/include/linux/property.h > index 61fc20e5f81f..ba8a3dc54786 100644 > --- a/include/linux/property.h > +++ b/include/linux/property.h > @@ -171,6 +171,16 @@ struct fwnode_handle *fwnode_get_next_available_child_node( > struct fwnode_handle *device_get_next_child_node(const struct device *dev, > struct fwnode_handle *child); > > +/** > + * device_for_each_child_node - iterate over available child nodes of a device > + * @dev: Pointer to the struct device > + * @child: Pointer to an available child node in each loop iteration, if found If it's not found then there are no loop iterations. So could drop the ,if found I think. > + * > + * Unavailable nodes are skipped i.e. this macro is implicitly _available_. > + * The reference to the child node must be dropped on early exits. > + * See fwnode_handle_put(). > + * For a scoped version of this macro, use device_for_each_child_node_scoped(). > + */ > #define device_for_each_child_node(dev, child) \ > for (child = device_get_next_child_node(dev, NULL); child; \ > child = device_get_next_child_node(dev, child)) >
On Sat, 06 Jul 2024 17:23:35 +0200 Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote: > The iterated nodes are direct children of the device node, and the > `device_for_each_child_node()` macro accounts for child node > availability. > > `fwnode_for_each_available_child_node()` is meant to access the child > nodes of an fwnode, and therefore not direct child nodes of the device > node. > > Use `device_for_each_child_node()` to indicate device's direct child > nodes. > > Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com> Why not the scoped variant? There look to be two error paths in there which would be simplified. > --- > drivers/leds/leds-bd2606mvv.c | 7 +++---- > 1 file changed, 3 insertions(+), 4 deletions(-) > > diff --git a/drivers/leds/leds-bd2606mvv.c b/drivers/leds/leds-bd2606mvv.c > index 3fda712d2f80..4f38b7b4d9d1 100644 > --- a/drivers/leds/leds-bd2606mvv.c > +++ b/drivers/leds/leds-bd2606mvv.c > @@ -69,7 +69,7 @@ static const struct regmap_config bd2606mvv_regmap = { > > static int bd2606mvv_probe(struct i2c_client *client) > { > - struct fwnode_handle *np, *child; > + struct fwnode_handle *child; > struct device *dev = &client->dev; > struct bd2606mvv_priv *priv; > struct fwnode_handle *led_fwnodes[BD2606_MAX_LEDS] = { 0 }; > @@ -77,8 +77,7 @@ static int bd2606mvv_probe(struct i2c_client *client) > int err, reg; > int i; > > - np = dev_fwnode(dev); > - if (!np) > + if (!dev_fwnode(dev)) > return -ENODEV; > > priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); > @@ -94,7 +93,7 @@ static int bd2606mvv_probe(struct i2c_client *client) > > i2c_set_clientdata(client, priv); > > - fwnode_for_each_available_child_node(np, child) { > + device_for_each_child_node(dev, child) { > struct bd2606mvv_led *led; > > err = fwnode_property_read_u32(child, "reg", ®); >
On Sat, 06 Jul 2024 17:23:37 +0200 Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote: > The iterated nodes are direct children of the device node, and the > `device_for_each_child_node()` macro accounts for child node > availability. > > `fwnode_for_each_available_child_node()` is meant to access the child > nodes of an fwnode, and therefore not direct child nodes of the device > node. > > Use `device_for_each_child_node()` to indicate device's direct child > nodes. > > Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
On 07/07/2024 18:57, Jonathan Cameron wrote: > On Sat, 06 Jul 2024 17:23:35 +0200 > Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote: > >> The iterated nodes are direct children of the device node, and the >> `device_for_each_child_node()` macro accounts for child node >> availability. >> >> `fwnode_for_each_available_child_node()` is meant to access the child >> nodes of an fwnode, and therefore not direct child nodes of the device >> node. >> >> Use `device_for_each_child_node()` to indicate device's direct child >> nodes. >> >> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com> > Why not the scoped variant? > There look to be two error paths in there which would be simplified. > I did not use the scoped variant because "child" is used outside the loop. On the other hand, I think an fwnode_handle_get() is missing for every "led_fwnodes[reg] = child" because a simple assignment does not increment the refcount. After adding fwnode_handle_get(), the scoped variant could be used, and the call to fwnode_handle_put() would act on led_fwnodes[reg] instead. >> --- >> drivers/leds/leds-bd2606mvv.c | 7 +++---- >> 1 file changed, 3 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/leds/leds-bd2606mvv.c b/drivers/leds/leds-bd2606mvv.c >> index 3fda712d2f80..4f38b7b4d9d1 100644 >> --- a/drivers/leds/leds-bd2606mvv.c >> +++ b/drivers/leds/leds-bd2606mvv.c >> @@ -69,7 +69,7 @@ static const struct regmap_config bd2606mvv_regmap = { >> >> static int bd2606mvv_probe(struct i2c_client *client) >> { >> - struct fwnode_handle *np, *child; >> + struct fwnode_handle *child; >> struct device *dev = &client->dev; >> struct bd2606mvv_priv *priv; >> struct fwnode_handle *led_fwnodes[BD2606_MAX_LEDS] = { 0 }; >> @@ -77,8 +77,7 @@ static int bd2606mvv_probe(struct i2c_client *client) >> int err, reg; >> int i; >> >> - np = dev_fwnode(dev); >> - if (!np) >> + if (!dev_fwnode(dev)) >> return -ENODEV; >> >> priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); >> @@ -94,7 +93,7 @@ static int bd2606mvv_probe(struct i2c_client *client) >> >> i2c_set_clientdata(client, priv); >> >> - fwnode_for_each_available_child_node(np, child) { >> + device_for_each_child_node(dev, child) { >> struct bd2606mvv_led *led; >> >> err = fwnode_property_read_u32(child, "reg", ®); >> >
No On 8 July 2024 09:14:44 BST, Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote: >On 07/07/2024 18:57, Jonathan Cameron wrote: >> On Sat, 06 Jul 2024 17:23:35 +0200 >> Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote: >> >>> The iterated nodes are direct children of the device node, and the >>> `device_for_each_child_node()` macro accounts for child node >>> availability. >>> >>> `fwnode_for_each_available_child_node()` is meant to access the child >>> nodes of an fwnode, and therefore not direct child nodes of the device >>> node. >>> >>> Use `device_for_each_child_node()` to indicate device's direct child >>> nodes. >>> >>> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com> >> Why not the scoped variant? >> There look to be two error paths in there which would be simplified. >> > >I did not use the scoped variant because "child" is used outside the loop. Ah missed that. Good sign that things are wrong... > >On the other hand, I think an fwnode_handle_get() is missing for every >"led_fwnodes[reg] = child" because a simple assignment does not >increment the refcount. Yes. Looks like a bug to me as well. > >After adding fwnode_handle_get(), the scoped variant could be used, and >the call to fwnode_handle_put() would act on led_fwnodes[reg] instead. There looks to be another bug as it only frees one handle on error. Right now it shouldnt free any but once you fix that you will need to free any not freed otherwise. Can it be squashed into one loop? J > >>> --- >>> drivers/leds/leds-bd2606mvv.c | 7 +++---- >>> 1 file changed, 3 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/leds/leds-bd2606mvv.c b/drivers/leds/leds-bd2606mvv.c >>> index 3fda712d2f80..4f38b7b4d9d1 100644 >>> --- a/drivers/leds/leds-bd2606mvv.c >>> +++ b/drivers/leds/leds-bd2606mvv.c >>> @@ -69,7 +69,7 @@ static const struct regmap_config bd2606mvv_regmap = { >>> >>> static int bd2606mvv_probe(struct i2c_client *client) >>> { >>> - struct fwnode_handle *np, *child; >>> + struct fwnode_handle *child; >>> struct device *dev = &client->dev; >>> struct bd2606mvv_priv *priv; >>> struct fwnode_handle *led_fwnodes[BD2606_MAX_LEDS] = { 0 }; >>> @@ -77,8 +77,7 @@ static int bd2606mvv_probe(struct i2c_client *client) >>> int err, reg; >>> int i; >>> >>> - np = dev_fwnode(dev); >>> - if (!np) >>> + if (!dev_fwnode(dev)) >>> return -ENODEV; >>> >>> priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); >>> @@ -94,7 +93,7 @@ static int bd2606mvv_probe(struct i2c_client *client) >>> >>> i2c_set_clientdata(client, priv); >>> >>> - fwnode_for_each_available_child_node(np, child) { >>> + device_for_each_child_node(dev, child) { >>> struct bd2606mvv_led *led; >>> >>> err = fwnode_property_read_u32(child, "reg", ®); >>> >> >
On 08/07/2024 10:14, Javier Carrasco wrote: > On 07/07/2024 18:57, Jonathan Cameron wrote: >> On Sat, 06 Jul 2024 17:23:35 +0200 >> Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote: >> >>> The iterated nodes are direct children of the device node, and the >>> `device_for_each_child_node()` macro accounts for child node >>> availability. >>> >>> `fwnode_for_each_available_child_node()` is meant to access the child >>> nodes of an fwnode, and therefore not direct child nodes of the device >>> node. >>> >>> Use `device_for_each_child_node()` to indicate device's direct child >>> nodes. >>> >>> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com> >> Why not the scoped variant? >> There look to be two error paths in there which would be simplified. >> > > I did not use the scoped variant because "child" is used outside the loop. > > On the other hand, I think an fwnode_handle_get() is missing for every > "led_fwnodes[reg] = child" because a simple assignment does not > increment the refcount. > > After adding fwnode_handle_get(), the scoped variant could be used, and > the call to fwnode_handle_put() would act on led_fwnodes[reg] instead. > Actually, the whole trouble comes from doing the processing in two consecutive loops, where the second loop accesses a child node that gets released at the end of the first one. It seems that some code got moved from one loop to a new one between two versions of the patchset. @Andreas Kemnade: I see that you had a single loop until v4 (the driver got applied with v6), and then you split it into two loops, but I don't see any mention to this modification in the change log. What was the reason for this modification? Apparently, similar drivers do everything in one loop to avoid such issues. Maybe refactoring to have a single loop again (if possible) would be the cleanest solution. Otherwise a get/put mechanism might be necessary. >>> --- >>> drivers/leds/leds-bd2606mvv.c | 7 +++---- >>> 1 file changed, 3 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/leds/leds-bd2606mvv.c b/drivers/leds/leds-bd2606mvv.c >>> index 3fda712d2f80..4f38b7b4d9d1 100644 >>> --- a/drivers/leds/leds-bd2606mvv.c >>> +++ b/drivers/leds/leds-bd2606mvv.c >>> @@ -69,7 +69,7 @@ static const struct regmap_config bd2606mvv_regmap = { >>> >>> static int bd2606mvv_probe(struct i2c_client *client) >>> { >>> - struct fwnode_handle *np, *child; >>> + struct fwnode_handle *child; >>> struct device *dev = &client->dev; >>> struct bd2606mvv_priv *priv; >>> struct fwnode_handle *led_fwnodes[BD2606_MAX_LEDS] = { 0 }; >>> @@ -77,8 +77,7 @@ static int bd2606mvv_probe(struct i2c_client *client) >>> int err, reg; >>> int i; >>> >>> - np = dev_fwnode(dev); >>> - if (!np) >>> + if (!dev_fwnode(dev)) >>> return -ENODEV; >>> >>> priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); >>> @@ -94,7 +93,7 @@ static int bd2606mvv_probe(struct i2c_client *client) >>> >>> i2c_set_clientdata(client, priv); >>> >>> - fwnode_for_each_available_child_node(np, child) { >>> + device_for_each_child_node(dev, child) { >>> struct bd2606mvv_led *led; >>> >>> err = fwnode_property_read_u32(child, "reg", ®); >>> >> >
On Mon, 8 Jul 2024 17:45:43 +0200 Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote: > On 08/07/2024 10:14, Javier Carrasco wrote: > > On 07/07/2024 18:57, Jonathan Cameron wrote: > >> On Sat, 06 Jul 2024 17:23:35 +0200 > >> Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote: > >> > >>> The iterated nodes are direct children of the device node, and the > >>> `device_for_each_child_node()` macro accounts for child node > >>> availability. > >>> > >>> `fwnode_for_each_available_child_node()` is meant to access the > >>> child nodes of an fwnode, and therefore not direct child nodes of > >>> the device node. > >>> > >>> Use `device_for_each_child_node()` to indicate device's direct > >>> child nodes. > >>> > >>> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com> > >> Why not the scoped variant? > >> There look to be two error paths in there which would be > >> simplified. > > > > I did not use the scoped variant because "child" is used outside > > the loop. > > > > On the other hand, I think an fwnode_handle_get() is missing for > > every "led_fwnodes[reg] = child" because a simple assignment does > > not increment the refcount. > > > > After adding fwnode_handle_get(), the scoped variant could be used, > > and the call to fwnode_handle_put() would act on led_fwnodes[reg] > > instead. > > Actually, the whole trouble comes from doing the processing in two > consecutive loops, where the second loop accesses a child node that > gets released at the end of the first one. It seems that some code > got moved from one loop to a new one between two versions of the > patchset. > > @Andreas Kemnade: I see that you had a single loop until v4 (the > driver got applied with v6), and then you split it into two loops, > but I don't see any mention to this modification in the change log. > > What was the reason for this modification? Apparently, similar drivers > do everything in one loop to avoid such issues. > The reason for two loops is that we check in the first loop whether broghtness can be individually controlled so we can set max_brightness in the second loop. I had the assumption that max_brightness should be set before registering leds. Some LEDs share brightness register, in the case where leds are defined with a shared register, we revert to on-off. > Maybe refactoring to have a single loop again (if possible) would be > the cleanest solution. Otherwise a get/put mechanism might be > necessary. > I had no idea how to do it the time I wrote the patch. Regards, Andreas
On 12/07/2024 23:06, Andreas Kemnade wrote: > On Mon, 8 Jul 2024 17:45:43 +0200 > Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote: > >> On 08/07/2024 10:14, Javier Carrasco wrote: >> What was the reason for this modification? Apparently, similar drivers >> do everything in one loop to avoid such issues. >> > The reason for two loops is that we check in the first loop whether > broghtness can be individually controlled so we can set max_brightness > in the second loop. I had the assumption that max_brightness should be > set before registering leds. > > Some LEDs share brightness register, in the case where leds are defined > with a shared register, we revert to on-off. > >> Maybe refactoring to have a single loop again (if possible) would be >> the cleanest solution. Otherwise a get/put mechanism might be >> necessary. >> > I had no idea how to do it the time I wrote the patch. > > Regards, > Andreas Then we could leave the two loops, and fix them. I am thinking of something like this: static int bd2606mvv_probe(struct i2c_client *client) { - struct fwnode_handle *child; struct device *dev = &client->dev; struct bd2606mvv_priv *priv; struct fwnode_handle *led_fwnodes[BD2606_MAX_LEDS] = { 0 }; int active_pairs[BD2606_MAX_LEDS / 2] = { 0 }; int err, reg; - int i; + int i, j; if (!dev_fwnode(dev)) return -ENODEV; @@ -93,20 +92,18 @@ static int bd2606mvv_probe(struct i2c_client *client) i2c_set_clientdata(client, priv); - device_for_each_child_node(dev, child) { + device_for_each_child_node_scoped(dev, child) { struct bd2606mvv_led *led; err = fwnode_property_read_u32(child, "reg", ®); - if (err) { - fwnode_handle_put(child); + if (err) return err; - } - if (reg < 0 || reg >= BD2606_MAX_LEDS || led_fwnodes[reg]) { - fwnode_handle_put(child); + + if (reg < 0 || reg >= BD2606_MAX_LEDS || led_fwnodes[reg]) return -EINVAL; - } + led = &priv->leds[reg]; - led_fwnodes[reg] = child; + led_fwnodes[reg] = fwnode_handle_get(child); active_pairs[reg / 2]++; led->priv = priv; led->led_no = reg; @@ -129,7 +126,8 @@ static int bd2606mvv_probe(struct i2c_client *client) &priv->leds[i].ldev, &init_data); if (err < 0) { - fwnode_handle_put(child); + for (j = i; j < BD2606_MAX_LEDS; j++) + fwnode_handle_put(led_fwnodes[j]); return dev_err_probe(dev, err, "couldn't register LED %s\n", priv->leds[i].ldev.name); Thanks to the call to fwnode_get_handle(child), the child nodes get their refcount incremented to be used in the second loop, where all child nodes that have not been registered are released in case of error. The first loop becomes a scoped one, keeping the `child` variable from being accessed anywhere else. Any feedback before I send a v2 with this is very welcome. Best regards, Javier Carrasco
This series aims to clarify the use cases of: - device_for_each_child_node[_scoped]() - fwnode_for_each_available_child_node[_scoped]() to access firmware nodes. There have been multiple discussions [1][2] about what the first macro implies in the sense of availability, and a number of users have opted for the second macro in cases where the first one should have been preferred. The second macro is intended to be used over child nodes of a firmware node, not direct child nodes of the device node. Instead, those users retrieve the fwnode member from the device struct just to have access to a macro that explicitly indicates node availability. That workaround is not necessary because `device_for_each_child_node()` implies availability for the existing backends (ACPI, DT, swnode). This series does not cover other points discussed in [2] like addressing uses of `fwnode_for_each_child_node()` where `device_*` should have been used, using the `_avaialble_` variant of the fwnode loop whenever possible, or adding new `_scoped` macros. Such points will be covered by subsequent series to keep focus on the "availability" issue. The conversion has been validated with an LTC2992 hwmon sensor, which is one of the affected drivers. The rest of the drivers could only be compiled and checked with static-analysis tools. Link: https://lore.kernel.org/all/20211205190101.26de4a57@jic23-huawei/ [1] Link: https://lore.kernel.org/all/20240523-fwnode_for_each_available_child_node_scoped-v2-0-701f3a03f2fb@gmail.com/ [2] Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com> --- Javier Carrasco (6): device property: document device_for_each_child_node macro hwmon: (ltc2992) use device_for_each_child_node_scoped() to access child nodes leds: bd2606mvv: use device_for_each_child_node() to access device child nodes leds: is31fl319x: use device_for_each_child_node_scoped() to access child nodes leds: pca995x: use device_for_each_child_node() to access device child nodes net: mvpp2: use device_for_each_child_node() to access device child nodes drivers/hwmon/ltc2992.c | 19 ++++---------- drivers/leds/leds-bd2606mvv.c | 7 +++-- drivers/leds/leds-is31fl319x.c | 34 ++++++++----------------- drivers/leds/leds-pca995x.c | 15 ++++------- drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c | 13 +++------- include/linux/property.h | 10 ++++++++ 6 files changed, 38 insertions(+), 60 deletions(-) --- base-commit: 0b58e108042b0ed28a71cd7edf5175999955b233 change-id: 20240701-device_for_each_child_node-available-1c1eca4b6495 Best regards,