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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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