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

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 <[email protected]>
> ---
>  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