Re: [PATCH v6 6/9] leds: multicolor: Introduce a multicolor class definition

[Greg KH <gregkh@linuxfoundation.org>]

On Wed, Sep 18, 2019 at 12:09:12PM -0500, Dan Murphy wrote:
> Greg
> 
> <snip>
> 
> > +static int led_multicolor_init_color(struct led_classdev_mc_data *data,
> > +				     struct led_classdev_mc *mcled_cdev,
> > +				     int color_id, int color_index)
> > +{
> > +	struct led_classdev *led_cdev = mcled_cdev->led_cdev;
> > +	struct led_mc_color_entry *mc_priv;
> > +	int ret;
> > +
> > +	mc_priv = devm_kzalloc(led_cdev->dev, sizeof(*mc_priv), GFP_KERNEL);
> > +	if (!mc_priv)
> > +		return -ENOMEM;
> > +
> > +	mc_priv->led_color_id = color_id;
> > +	mc_priv->mcled_cdev = mcled_cdev;
> > +
> > +	led_color_group.name = led_colors[color_id];
> > +	ret = sysfs_create_group(data->color_kobj, &led_color_group);
> > +	if (ret)
> > +		return ret;
> > +
> > +	sysfs_attr_init(&mc_priv->intensity_attr.attr);
> > +	mc_priv->intensity_attr.attr.name = "intensity";
> > +	mc_priv->intensity_attr.attr.mode = 666;
> > +	mc_priv->intensity_attr.store = intensity_store;
> > +	mc_priv->intensity_attr.show = intensity_show;
> > +	ret = sysfs_add_file_to_group(data->color_kobj,
> > +				      &mc_priv->intensity_attr.attr,
> > +				      led_color_group.name);
> > +	if (ret)
> > +		return ret;
> > +
> > +	sysfs_attr_init(&mc_priv->max_intensity_attr.attr);
> > +	mc_priv->max_intensity_attr.attr.name = "max_intensity";
> > +	mc_priv->max_intensity_attr.attr.mode = 444;
> > +	mc_priv->max_intensity_attr.show = max_intensity_show;
> > +	ret = sysfs_add_file_to_group(data->color_kobj,
> > +				      &mc_priv->max_intensity_attr.attr,
> > +				      led_color_group.name);
> > +	if (ret)
> > +		goto err_out;
> > +
> > +	mc_priv->max_intensity = LED_FULL;
> > +	list_add_tail(&mc_priv->list, &data->color_list);
> > +
> > +err_out:
> > +	return ret;
> > +}
> > +
> > +static int led_multicolor_init_color_dir(struct led_classdev_mc_data *data,
> > +					 struct led_classdev_mc *mcled_cdev)
> > +{
> > +	struct led_classdev *led_cdev = mcled_cdev->led_cdev;
> > +	u32 color_id;
> > +	int ret;
> > +	int i, j = 0;
> > +
> > +	data->color_kobj = kobject_create_and_add("colors",
> > +						  &led_cdev->dev->kobj);
> 
> We need some guidance here on how to properly create sub directories more
> then 1 level deep.

Short answer, don't.

Long answer, use a 'struct device' but ONLY IF YOU KNOW WHAT YOU ARE
DOING!

Follow the short answer please.

> In short under the LED class device parent directory we want to create a
> directory called "colors".

Ok, that's simple.

> Under that directory we want to create a directory corresponding to the
> monochrome LED color.

Why?

> Under that directory we have the files to for intensity and max_intensity
> for the monochrome LED.

Why not just have colors/monochrome_intensity and
colors/monochrome_max_intensity as your files in the colors/ directory?

> We can use the LED class kobject to create the colors directory using the
> sysfs calls but the issue comes when creating the LED color directory.

Yes.

> We don't have a kobj for colors to associate those directories to. 

And you shouldn't :)

> The only API we see to use the kobject_create_and_add which then gives
> us the colors directory kobj.

Don't do that, you will break userspace code hard if you do that.

NEVER put a raw kobject after a 'struct device' in the sysfs tree if you
expect normal userspace libraries to be able to understand what is going
on.  That's why this is "hard", you are not supposed to be doing it.

> So the directory structure would look like this which is explained in this
> patch https://lore.kernel.org/patchwork/patch/1128444/
> 
> Directory Layout Example
> ========================
> root:/sys/class/leds/rgb:grouped_leds# ls -lR colors/
> colors/:
> drwxr-xr-x    2 root     root             0 Jun 28 20:21 blue
> rwxr-xr-x    2 root     root             0 Jun 28 20:21 green
> drwxr-xr-x    2 root     root             0 Jun 28 20:21 red
> 
> colors/blue:
> -rw-------    1 root     root          4096 Jun 28 20:21 intensity
> -r--------    1 root     root          4096 Jun 28 20:27 max_intensity
> +colors/green:
> -rw-------    1 root     root          4096 Jun 28 20:22 intensity
> -r--------    1 root     root          4096 Jun 28 20:27 max_intensity
> 
> colors/red:
> -rw-------    1 root     root          4096 Jun 28 20:21 intensity
> -r--------    1 root     root          4096 Jun 28 20:27 max_intensity


No, just add blue, green, red to the prefix of those files and all
should be fine.  Don't try to get fancy and use subdirs, that way lies
madness.

> I have reviewed your example code and read your blogs and papers on the
> subject but nothing really talks about sub-sub directories.

Because you shouldn't do it, I didn't think I had to describe everything
you should not do :)

> Now if this is a no-no in the kernel that is fine we can adjust but Jacek
> wanted to get your opinon/guidance on this topic.

Yes, don't do it.

did I say it enough times?  :)

thanks,

greg k-h