[PATCH] leds: ncp5623: release multi-led fwnode on remove

[PATCH] leds: ncp5623: release multi-led fwnode on remove

[Guangshuo Li]

ncp5623_probe() obtains a reference to the multi-led firmware node with
device_get_named_child_node(). This reference must remain valid after
probe because led_classdev_register_ext() associates the node with the
LED class device using device_set_node(), which does not acquire an
additional reference.

The probe error paths correctly release mc_node, while the successful
path intentionally keeps the reference alive for the lifetime of the
registered LED device. However, ncp5623_remove() unregisters the LED
class device without subsequently dropping that reference, leaking the
firmware node on driver unbind.

Save the firmware node pointer before unregistering the LED device and
drop its reference afterwards, when the LED device can no longer use
it.

This issue was found by manual code inspection.

Fixes: 7b7e50f8f5e0 ("leds: Add NCP5623 multi-led driver")
Cc: stable@vger.kernel.org
Signed-off-by: Guangshuo Li <lgs201920130244@gmail.com>
---
 drivers/leds/rgb/leds-ncp5623.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/leds/rgb/leds-ncp5623.c b/drivers/leds/rgb/leds-ncp5623.c
index f2528f06507d..f96f15b7e9a1 100644
--- a/drivers/leds/rgb/leds-ncp5623.c
+++ b/drivers/leds/rgb/leds-ncp5623.c
@@ -227,6 +227,8 @@ static int ncp5623_probe(struct i2c_client *client)
 static void ncp5623_remove(struct i2c_client *client)
 {
 	struct ncp5623 *ncp = i2c_get_clientdata(client);
+	struct fwnode_handle *mc_node =
+		dev_fwnode(ncp->mc_dev.led_cdev.dev);
 
 	mutex_lock(&ncp->lock);
 	ncp->delay = 0;
@@ -234,6 +236,7 @@ static void ncp5623_remove(struct i2c_client *client)
 
 	ncp5623_write(client, NCP5623_DIMMING_TIME_REG, 0);
 	led_classdev_multicolor_unregister(&ncp->mc_dev);
+	fwnode_handle_put(mc_node);
 	mutex_destroy(&ncp->lock);
 }
 
-- 
2.43.0

Re: [PATCH] leds: ncp5623: release multi-led fwnode on remove

[Laurent Pinchart]

On Mon, Sep 14, 2026 at 10:07:42PM +0800, Guangshuo Li wrote:
> ncp5623_probe() obtains a reference to the multi-led firmware node with
> device_get_named_child_node(). This reference must remain valid after
> probe because led_classdev_register_ext() associates the node with the
> LED class device using device_set_node(), which does not acquire an
> additional reference.
> 
> The probe error paths correctly release mc_node, while the successful
> path intentionally keeps the reference alive for the lifetime of the
> registered LED device. However, ncp5623_remove() unregisters the LED
> class device without subsequently dropping that reference, leaking the
> firmware node on driver unbind.
> 
> Save the firmware node pointer before unregistering the LED device and
> drop its reference afterwards, when the LED device can no longer use
> it.
> 
> This issue was found by manual code inspection.
> 
> Fixes: 7b7e50f8f5e0 ("leds: Add NCP5623 multi-led driver")
> Cc: stable@vger.kernel.org
> Signed-off-by: Guangshuo Li <lgs201920130244@gmail.com>
> ---
>  drivers/leds/rgb/leds-ncp5623.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/leds/rgb/leds-ncp5623.c b/drivers/leds/rgb/leds-ncp5623.c
> index f2528f06507d..f96f15b7e9a1 100644
> --- a/drivers/leds/rgb/leds-ncp5623.c
> +++ b/drivers/leds/rgb/leds-ncp5623.c
> @@ -227,6 +227,8 @@ static int ncp5623_probe(struct i2c_client *client)
>  static void ncp5623_remove(struct i2c_client *client)
>  {
>  	struct ncp5623 *ncp = i2c_get_clientdata(client);
> +	struct fwnode_handle *mc_node =
> +		dev_fwnode(ncp->mc_dev.led_cdev.dev);
>  
>  	mutex_lock(&ncp->lock);
>  	ncp->delay = 0;
> @@ -234,6 +236,7 @@ static void ncp5623_remove(struct i2c_client *client)
>  
>  	ncp5623_write(client, NCP5623_DIMMING_TIME_REG, 0);
>  	led_classdev_multicolor_unregister(&ncp->mc_dev);
> +	fwnode_handle_put(mc_node);

If the explanation in the commit message is true, it would indicate a
bad API defect with led_classdev_multicolor_register_ext() and
led_classdev_multicolor_unregister(). This patch would be a hack to work
around the problem at best, and isn't the right solution.

>  	mutex_destroy(&ncp->lock);
>  }
>  

-- 
Regards,

Laurent Pinchart