Re: [PATCH v3] led/led-class: Handle LEDs with the same name

[Sakari Ailus <sakari.ailus@iki.fi>]

Hi Ricardo,

(Cc Jacek.)

I had similar thoughts when reviewing Jacek's V4L2 flash API patches. See
below.

On Sat, Mar 14, 2015 at 12:05:39AM +0100, Ricardo Ribalda Delgado wrote:
> The current code expected that every LED had an unique name. This is a
> legit expectation when the device tree can no be modified or extended.
> But with device tree overlays this requirement can be easily broken.
> 
> This patch finds out if the name is already in use and adds the suffix
> _1, _2... if not.
> 
> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
> ---
> v3: Suggested by Bryan Wu <cooloney@gmail.com>
> 
> -Do not allocate dynamically the name
>  drivers/leds/led-class.c | 31 +++++++++++++++++++++++++++++--
>  1 file changed, 29 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> index 768d33a..4ca37b8 100644
> --- a/drivers/leds/led-class.c
> +++ b/drivers/leds/led-class.c
> @@ -212,6 +212,26 @@ static const struct dev_pm_ops leds_class_dev_pm_ops = {
>  	.resume         = led_resume,
>  };
>  
> +static int match_name(struct device *dev, const void *data)
> +{
> +	if (!dev_name(dev))
> +		return 0;
> +	return !strcmp(dev_name(dev), (char *)data);
> +}
> +
> +static int led_classdev_next_name(const char *init_name, char *name,
> +				  size_t len)
> +{
> +	int i = 0;
> +
> +	strncpy(name, init_name, len);
> +
> +	while (class_find_device(leds_class, NULL, name, match_name))
> +		snprintf(name, len, "%s_%d", init_name, ++i);
> +
> +	return i;
> +}
> +
>  /**
>   * led_classdev_register - register a new object of led_classdev class.
>   * @parent: The device to register.
> @@ -219,12 +239,19 @@ static const struct dev_pm_ops leds_class_dev_pm_ops = {
>   */
>  int led_classdev_register(struct device *parent, struct led_classdev *led_cdev)
>  {
> +	char name[64];
> +	int ret;
> +
> +	ret = led_classdev_next_name(led_cdev->name, name, sizeof(name));
>  	led_cdev->dev = device_create_with_groups(leds_class, parent, 0,
> -					led_cdev, led_cdev->groups,
> -					"%s", led_cdev->name);
> +					led_cdev, led_cdev->groups, name);
>  	if (IS_ERR(led_cdev->dev))
>  		return PTR_ERR(led_cdev->dev);
>  
> +	if (ret)
> +		dev_info(parent, "Led %s renamed to %s due to name collision",
> +				led_cdev->name, dev_name(led_cdev->dev));

I'd use dev_warn() or even WARN_ON(ret). This is a rather serious issue as
now the probe order defines the name of the device. There might be something
better such as the I2C bus/address.

If the registration order for some reason changes, that'd revert the device
names. In Media controller this is part of the user space API.

I suggested Jacek adding the uniqueness requirement to documentation as
well, ePAPR by itself does not require it for label properties.

> +
>  #ifdef CONFIG_LEDS_TRIGGERS
>  	init_rwsem(&led_cdev->trigger_lock);
>  #endif

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk