Re: [PATCH] LEDS: fix race between LED device uevent and actual attributes creation

[Florian Fainelli <ffainelli@freebox.fr>]

Hi Richard,

For some reason, this version appears to work, while it actually should not 
because device_add_attributes has no mean to stop iterating over 
leds_class_attrs members.

Also, a LED device removal would have made the device attributes be really 
removed in led_classdev_unregister, and then tried to be removed a second time 
by device_remove_attr. Since we never check the return value of 
device_remove_file and the underlying calls which are made to the syfs 
functions, no error is produced, but this is actually wrong.

I just resent a new version which really fix the issue and is cleaner. Sorry 
for the noise.

On Wednesday 10 March 2010 15:11:44  Fainelli wrote:
> If we were to dynamically register/unregister leds and have udev or other
> daemons handle the leds class uevents, we would be notified of the adding
>  of a new LED and if the daemon immediately tries to open one of the
>  attributes of the led device, it would fail with a "no such file or
>  directory" error since the attributes are not yet created. Fix this by
>  switching attributes to be class-wide, such that the driver core will
>  register these attributes with device_add_attrs and then emit the
>  kobject_uevent ADD signal.
> 
> Signed-off-by: Florian Fainelli <ffainelli@freebox.fr>
> CC: stable@kernel.org
> ---
> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> index 782f958..0be0310 100644
> --- a/drivers/leds/led-class.c
> +++ b/drivers/leds/led-class.c
> @@ -78,6 +78,14 @@ static DEVICE_ATTR(max_brightness, 0444,
>  led_max_brightness_show, NULL); static DEVICE_ATTR(trigger, 0644,
>  led_trigger_show, led_trigger_store); #endif
> 
> +static struct device_attribute *leds_class_attrs[] = {
> +	&dev_attr_brightness,
> +	&dev_attr_max_brightness,
> +#ifdef CONFIG_LEDS_TRIGGERS
> +	&dev_attr_trigger,
> +#endif
> +};
> +
>  /**
>   * led_classdev_suspend - suspend an led_classdev.
>   * @led_cdev: the led_classdev to suspend.
> @@ -127,18 +135,11 @@ static int led_resume(struct device *dev)
>   */
>  int led_classdev_register(struct device *parent, struct led_classdev
>  *led_cdev) {
> -	int rc;
> -
>  	led_cdev->dev = device_create(leds_class, parent, 0, led_cdev,
>  				      "%s", led_cdev->name);
>  	if (IS_ERR(led_cdev->dev))
>  		return PTR_ERR(led_cdev->dev);
> 
> -	/* register the attributes */
> -	rc = device_create_file(led_cdev->dev, &dev_attr_brightness);
> -	if (rc)
> -		goto err_out;
> -
>  #ifdef CONFIG_LEDS_TRIGGERS
>  	init_rwsem(&led_cdev->trigger_lock);
>  #endif
> @@ -150,17 +151,9 @@ int led_classdev_register(struct device *parent,
>  struct led_classdev *led_cdev) if (!led_cdev->max_brightness)
>  		led_cdev->max_brightness = LED_FULL;
> 
> -	rc = device_create_file(led_cdev->dev, &dev_attr_max_brightness);
> -	if (rc)
> -		goto err_out_attr_max;
> -
>  	led_update_brightness(led_cdev);
> 
>  #ifdef CONFIG_LEDS_TRIGGERS
> -	rc = device_create_file(led_cdev->dev, &dev_attr_trigger);
> -	if (rc)
> -		goto err_out_led_list;
> -
>  	led_trigger_set_default(led_cdev);
>  #endif
> 
> @@ -168,18 +161,8 @@ int led_classdev_register(struct device *parent,
>  struct led_classdev *led_cdev) led_cdev->name);
> 
>  	return 0;
> -
> -#ifdef CONFIG_LEDS_TRIGGERS
> -err_out_led_list:
> -	device_remove_file(led_cdev->dev, &dev_attr_max_brightness);
> -#endif
> -err_out_attr_max:
> -	device_remove_file(led_cdev->dev, &dev_attr_brightness);
> -	list_del(&led_cdev->node);
> -err_out:
> -	device_unregister(led_cdev->dev);
> -	return rc;
>  }
> +
>  EXPORT_SYMBOL_GPL(led_classdev_register);
> 
>  /**
> @@ -215,6 +198,7 @@ static int __init leds_init(void)
>  		return PTR_ERR(leds_class);
>  	leds_class->suspend = led_suspend;
>  	leds_class->resume = led_resume;
> +	leds_class->dev_attrs = *leds_class_attrs;
>  	return 0;
>  }
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>