This series switches from the device_for_each_child_node() macro to its scoped variant, which in general makes the code more robust if new early exits are added to the loops, because there is no need for explicit calls to fwnode_handle_put(). Depending on the complexity of the loop and its error handling, the code gets simplified and it gets easier to follow.
The non-scoped variant of the macro is error-prone, and it has been the source of multiple bugs where the child node refcount was not decremented accordingly in error paths within the loops. The first patch of this series is a good example, which fixes that kind of bug that is regularly found in node iterators.
The uses of device_for_each_child_node() with no early exits have been left untouched because their simpilicty justifies the non-scoped variant.
Note that the child node is now declared in the macro, and therefore the explicit declaration is no longer required.
The general functionality should not be affected by this modification. If functional changes are found, please report them back as errors.
Signed-off-by: Javier Carrasco javier.carrasco.cruz@gmail.com --- Javier Carrasco (18): leds: flash: mt6360: fix device_for_each_child_node() refcounting in error paths leds: flash: mt6370: switch to device_for_each_child_node_scoped() leds: flash: leds-qcom-flash: switch to device_for_each_child_node_scoped() leds: aw200xx: switch to device_for_each_child_node_scoped() leds: cr0014114: switch to device_for_each_child_node_scoped() leds: el15203000: switch to device_for_each_child_node_scoped() leds: gpio: switch to device_for_each_child_node_scoped() leds: lm3532: switch to device_for_each_child_node_scoped() leds: lm3697: switch to device_for_each_child_node_scoped() leds: lp50xx: switch to device_for_each_child_node_scoped() leds: max77650: switch to device_for_each_child_node_scoped() leds: ns2: switch to device_for_each_child_node_scoped() leds: pca963x: switch to device_for_each_child_node_scoped() leds: pwm: switch to device_for_each_child_node_scoped() leds: sun50i-a100: switch to device_for_each_child_node_scoped() leds: tca6507: switch to device_for_each_child_node_scoped() leds: rgb: ktd202x: switch to device_for_each_child_node_scoped() leds: rgb: mt6370: switch to device_for_each_child_node_scoped()
drivers/leds/flash/leds-mt6360.c | 3 +-- drivers/leds/flash/leds-mt6370-flash.c | 11 +++------- drivers/leds/flash/leds-qcom-flash.c | 4 +--- drivers/leds/leds-aw200xx.c | 7 ++----- drivers/leds/leds-cr0014114.c | 4 +--- drivers/leds/leds-el15203000.c | 14 ++++--------- drivers/leds/leds-gpio.c | 9 +++------ drivers/leds/leds-lm3532.c | 18 +++++++---------- drivers/leds/leds-lm3697.c | 18 ++++++----------- drivers/leds/leds-lp50xx.c | 21 +++++++------------ drivers/leds/leds-max77650.c | 18 ++++++----------- drivers/leds/leds-ns2.c | 7 ++----- drivers/leds/leds-pca963x.c | 11 +++------- drivers/leds/leds-pwm.c | 15 ++++---------- drivers/leds/leds-sun50i-a100.c | 27 +++++++++---------------- drivers/leds/leds-tca6507.c | 7 ++----- drivers/leds/rgb/leds-ktd202x.c | 8 +++----- drivers/leds/rgb/leds-mt6370-rgb.c | 37 ++++++++++------------------------ 18 files changed, 75 insertions(+), 164 deletions(-) --- base-commit: 92fc9636d1471b7f68bfee70c776f7f77e747b97 change-id: 20240926-leds_device_for_each_child_node_scoped-5a95255413fa
Best regards,
The device_for_each_child_node() macro requires explicit calls to fwnode_handle_put() upon early exits to avoid memory leaks, and in this case the error paths are handled after jumping to 'out_flash_realease', which misses that required call to to decrement the refcount of the child node.
A more elegant and robust solution is using the scoped variant of the loop, which automatically handles such early exits.
Fix the child node refcounting in the error paths by using device_for_each_child_node_scoped().
Cc: stable@vger.kernel.org Fixes: 679f8652064b ("leds: Add mt6360 driver") Signed-off-by: Javier Carrasco javier.carrasco.cruz@gmail.com --- drivers/leds/flash/leds-mt6360.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/leds/flash/leds-mt6360.c b/drivers/leds/flash/leds-mt6360.c index 4c74f1cf01f0..676236c19ec4 100644 --- a/drivers/leds/flash/leds-mt6360.c +++ b/drivers/leds/flash/leds-mt6360.c @@ -784,7 +784,6 @@ static void mt6360_v4l2_flash_release(struct mt6360_priv *priv) static int mt6360_led_probe(struct platform_device *pdev) { struct mt6360_priv *priv; - struct fwnode_handle *child; size_t count; int i = 0, ret;
@@ -811,7 +810,7 @@ static int mt6360_led_probe(struct platform_device *pdev) return -ENODEV; }
- device_for_each_child_node(&pdev->dev, child) { + device_for_each_child_node_scoped(&pdev->dev, child) { struct mt6360_led *led = priv->leds + i; struct led_init_data init_data = { .fwnode = child, }; u32 reg, led_color;
On Fri, 27 Sep 2024 01:20:51 +0200, Javier Carrasco wrote:
This series switches from the device_for_each_child_node() macro to its scoped variant, which in general makes the code more robust if new early exits are added to the loops, because there is no need for explicit calls to fwnode_handle_put(). Depending on the complexity of the loop and its error handling, the code gets simplified and it gets easier to follow.
[...]
Applied, thanks!
[01/18] leds: flash: mt6360: fix device_for_each_child_node() refcounting in error paths commit: 73b03b27736e440e3009fe1319cbc82d2cd1290c [02/18] leds: flash: mt6370: switch to device_for_each_child_node_scoped() commit: 19d1cc765e7d477070ddda02c9a07a1ebcdf4b2d [03/18] leds: flash: leds-qcom-flash: switch to device_for_each_child_node_scoped() commit: f64dd42a4f939fb5acc3f3568ef2118487617996 [04/18] leds: aw200xx: switch to device_for_each_child_node_scoped() commit: a361af3c1622d4b8ede54493fa88633fb12201d0 [05/18] leds: cr0014114: switch to device_for_each_child_node_scoped() commit: 65135e2ccf5ad0853c1df0ffeefc372066a62909 [06/18] leds: el15203000: switch to device_for_each_child_node_scoped() commit: 9e445e28ae0c6fe24369127cf2302cd4f3a1b42b [07/18] leds: gpio: switch to device_for_each_child_node_scoped() commit: 42b49671602f9badb14fd2c32e6791a24d8cbf02 [08/18] leds: lm3532: switch to device_for_each_child_node_scoped() commit: 7bd4b9277b9831d115f14d26000c0ba32c83d109 [09/18] leds: lm3697: switch to device_for_each_child_node_scoped() commit: 6e2d1d83b70bd736228529fd1cb4f98e0ab77eb8 [10/18] leds: lp50xx: switch to device_for_each_child_node_scoped() commit: ba35b9a4c1b074218880c47ca09d19a3c69f904d [11/18] leds: max77650: switch to device_for_each_child_node_scoped() commit: 4ab3ae432da1706b5e1624ecea3c670faaec39d7 [12/18] leds: ns2: switch to device_for_each_child_node_scoped() commit: 5b5d936db0d2fb9e81d240ed91d062b8c8f0d224 [13/18] leds: pca963x: switch to device_for_each_child_node_scoped() commit: dea90acb09324efe640ab69766c12d8d387ee97f [14/18] leds: pwm: switch to device_for_each_child_node_scoped() commit: e3456071853597229012622c97b76109c0fa8754 [15/18] leds: sun50i-a100: switch to device_for_each_child_node_scoped() commit: 8cf103de9a002fb02125491c06d9cd60762d70e5 [16/18] leds: tca6507: switch to device_for_each_child_node_scoped() commit: 01728d041986a6992d0b2499e88db4569e65a535 [17/18] leds: rgb: ktd202x: switch to device_for_each_child_node_scoped() commit: 48259638fe5986afe8ed2a49e35f0641d953c311 [18/18] leds: rgb: mt6370: switch to device_for_each_child_node_scoped() commit: bf3fba727695dcd1ac3f9d17d88845223f56c14f
-- Lee Jones [李琼斯]
Fri, Sep 27, 2024 at 01:20:51AM +0200, Javier Carrasco kirjoitti:
This series switches from the device_for_each_child_node() macro to its scoped variant, which in general makes the code more robust if new early exits are added to the loops, because there is no need for explicit calls to fwnode_handle_put(). Depending on the complexity of the loop and its error handling, the code gets simplified and it gets easier to follow.
The non-scoped variant of the macro is error-prone, and it has been the source of multiple bugs where the child node refcount was not decremented accordingly in error paths within the loops. The first patch of this series is a good example, which fixes that kind of bug that is regularly found in node iterators.
The uses of device_for_each_child_node() with no early exits have been left untouched because their simpilicty justifies the non-scoped variant.
Note that the child node is now declared in the macro, and therefore the explicit declaration is no longer required.
The general functionality should not be affected by this modification. If functional changes are found, please report them back as errors.
Thank you for this series. It may now benefit from the next steps:
1) converting to return dev_err_probe() or dev_warn_probe() directly since the goto had been replaced by direct return, hence saving even more LoCs;
2) dropping or refactoring the complex conditions due to your nice series being applied.
I'll comment individually on some to show what I meant by the above.
linux-stable-mirror@lists.linaro.org
-
Andy Shevchenko -
Javier Carrasco -
Lee Jones