[PATCH] leds: lp55xx: roll back engine sysfs group on failure

[PATCH] leds: lp55xx: roll back engine sysfs group on failure

[Pengpeng Hou]

lp55xx_register_sysfs() creates the common engine sysfs group before
creating the optional chip-specific sysfs group.

If the chip-specific group creation fails, the function returns the
error directly and leaves the engine group published even though probe
fails and the chip state will be torn down.

Remove the engine group when the later chip-specific group creation
fails.

Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
---
 drivers/leds/leds-lp55xx-common.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/leds/leds-lp55xx-common.c b/drivers/leds/leds-lp55xx-common.c
index ea131177de96..501762b02667 100644
--- a/drivers/leds/leds-lp55xx-common.c
+++ b/drivers/leds/leds-lp55xx-common.c
@@ -1090,6 +1090,7 @@ static int lp55xx_register_sysfs(struct lp55xx_chip *chip)
 {
 	struct device *dev = &chip->cl->dev;
 	const struct lp55xx_device_config *cfg = chip->cfg;
+	bool engine_group_created = false;
 	int ret;
 
 	if (!cfg->run_engine || !cfg->firmware_cb)
@@ -1098,10 +1099,17 @@ static int lp55xx_register_sysfs(struct lp55xx_chip *chip)
 	ret = sysfs_create_group(&dev->kobj, &lp55xx_engine_attr_group);
 	if (ret)
 		return ret;
+	engine_group_created = true;
 
 dev_specific_attrs:
-	return cfg->dev_attr_group ?
-		sysfs_create_group(&dev->kobj, cfg->dev_attr_group) : 0;
+	if (!cfg->dev_attr_group)
+		return 0;
+
+	ret = sysfs_create_group(&dev->kobj, cfg->dev_attr_group);
+	if (ret && engine_group_created)
+		sysfs_remove_group(&dev->kobj, &lp55xx_engine_attr_group);
+
+	return ret;
 }
 
 static void lp55xx_unregister_sysfs(struct lp55xx_chip *chip)
-- 
2.50.1 (Apple Git-155)

Re: [PATCH] leds: lp55xx: roll back engine sysfs group on failure

[Lee Jones]

On Mon, 15 Jun 2026, Pengpeng Hou wrote:

> lp55xx_register_sysfs() creates the common engine sysfs group before
> creating the optional chip-specific sysfs group.
> 
> If the chip-specific group creation fails, the function returns the
> error directly and leaves the engine group published even though probe
> fails and the chip state will be torn down.
> 
> Remove the engine group when the later chip-specific group creation
> fails.
> 
> Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
> ---
>  drivers/leds/leds-lp55xx-common.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/leds/leds-lp55xx-common.c b/drivers/leds/leds-lp55xx-common.c
> index ea131177de96..501762b02667 100644
> --- <mark>drivers/leds/leds-lp55xx-common.c</mark>
> +++ b/drivers/leds/leds-lp55xx-common.c
> @@ -1090,6 +1090,7 @@ static int lp55xx_register_sysfs(struct lp55xx_chip *chip)
>  {
>  	struct device *dev = &chip->cl->dev;
>  	const struct lp55xx_device_config *cfg = chip->cfg;
> +	bool engine_group_created = false;

This is almost certainly a NACK.

>  	int ret;
>  
>  	if (!cfg->run_engine || !cfg->firmware_cb)
> @@ -1098,10 +1099,17 @@ static int lp55xx_register_sysfs(struct lp55xx_chip *chip)
>  	ret = sysfs_create_group(&dev->kobj, &lp55xx_engine_attr_group);
>  	if (ret)
>  		return ret;
> +	engine_group_created = true;
>  
>  dev_specific_attrs:
> -	return cfg->dev_attr_group ?
> -		sysfs_create_group(&dev->kobj, cfg->dev_attr_group) : 0;
> +	if (!cfg->dev_attr_group)
> +		return 0;
> +
> +	ret = sysfs_create_group(&dev->kobj, cfg->dev_attr_group);
> +	if (ret && engine_group_created)

3 things:

Firstly, doesn't the driver core unwind these for us if probe fails?

Secondly, isn't sysfs_remove_group() okay to call regardless?

And lastly, _even_ checking 'cfg->run_engine' and 'cfg->firmware_cb'
would be better than introducing a new random variable to track this.

> +		sysfs_remove_group(&dev->kobj, &lp55xx_engine_attr_group);
> +
> +	return ret;
>  }
>  
>  static void lp55xx_unregister_sysfs(struct lp55xx_chip *chip)
> -- 
> 2.50.1 (Apple Git-155)
> 

-- 
Lee Jones