Re: [PATCH v2] leds: blinkm: Clean up sysfs group on LED registration failure

[Greg KH]

On Thu, Oct 15, 2026 at 12:54:33PM +0100, Lee Jones wrote:
> On Fri, 25 Sep 2026, Myeonghun Pak wrote:
> 
> > blinkm_probe() creates the BlinkM sysfs group before registering the LED
> > class devices. If multicolor sub-LED allocation fails, probe returns
> > without removing the group. If multicolor LED registration fails, the
> > helper removes the group but returns success, leaving the device bound
> > without an LED class device.
> >
> > Keep LED class device rollback in the registration helpers, but remove
> > the BlinkM sysfs group in blinkm_probe() when either registration mode
> > fails. Propagate the multicolor registration error so probe can perform
> > this cleanup and report the failure.
> >
> > Fixes: 56e8c56c9af0 ("leds: Add multicolor support to BlinkM LED driver")
> > Cc: stable@xxxxxxxxxxxxxxx
> > Assisted-by: LLM
> > Co-developed-by: Ijae Kim <ae878000@xxxxxxxxx>
> > Signed-off-by: Ijae Kim <ae878000@xxxxxxxxx>
> > Signed-off-by: Myeonghun Pak <mhun512@xxxxxxxxx>
> > ---
> > Changes in v2:
> > - Move sysfs group cleanup to blinkm_probe() for both LED registration modes.
> > - Propagate multicolor LED registration errors to blinkm_probe().
> >
> >  drivers/leds/leds-blinkm.c | 15 ++++++---------
> >  1 file changed, 6 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/leds/leds-blinkm.c b/drivers/leds/leds-blinkm.c
> > --- a/drivers/leds/leds-blinkm.c
> > +++ b/drivers/leds/leds-blinkm.c
> > @@ -671,7 +671,6 @@ failblue:
> >  failgreen:
> >  	led_classdev_unregister(&leds[RED]->cdev.led_cdev);
> >  failred:
> > -	sysfs_remove_group(&client->dev.kobj, &blinkm_group);
> >
> >  	return err;
> >  }
> > @@ -714,9 +713,8 @@ static int register_multicolor(struct i2c_client *client, struct blinkm_data *da
> >  	if (err < 0) {
> >  		dev_err(&client->dev, "couldn't register LED %s\n",
> >  				mc_led->cdev.led_cdev.name);
> > -		sysfs_remove_group(&client->dev.kobj, &blinkm_group);
> >  	}
> > -	return 0;
> > +	return err;
> >  }
> >
> >  static int blinkm_probe(struct i2c_client *client)
> > @@ -749,13 +747,12 @@ static int blinkm_probe(struct i2c_client *client)
> >  	}
> >
> > -	if (!IS_ENABLED(CONFIG_LEDS_BLINKM_MULTICOLOR)) {
> > +	if (!IS_ENABLED(CONFIG_LEDS_BLINKM_MULTICOLOR))
> >  		err = register_separate_colors(client, data);
> > -		if (err < 0)
> > -			return err;
> > -	} else {
> > +	else
> >  		err = register_multicolor(client, data);
> 
> Please add a newline after the 'if-else' block.
> 
> > -		if (err < 0)
> > -			return err;
> > +	if (err < 0) {
> > +		sysfs_remove_group(&client->dev.kobj, &blinkm_group);
> 
> How about using 'devm_device_add_group() to automate clean-up?

Wait, why is a driver manually adding sysfs files at all?  Shouldn't
this "just work" if it's a default group for the driver?  This way (or
with the devm_device_add_group() call) will race with userspace and
loose.

thanks,

greg k-h