Re: [PATCH 2/2] HID: leds: move led_mode attribute to led-class devices in MSI GT683R driver

[Johan Hovold <johan@kernel.org>]

On Tue, Jun 24, 2014 at 03:56:51PM -0400, Greg KH wrote:
> On Tue, Jun 24, 2014 at 10:38:38PM +0300, Janne Kanniainen wrote:

> >  static int gt683r_led_probe(struct hid_device *hdev,
> >  			const struct hid_device_id *id)
> > @@ -261,6 +272,7 @@ static int gt683r_led_probe(struct hid_device *hdev,
> >  		led->led_devs[i].name = name;
> >  		led->led_devs[i].max_brightness = 1;
> >  		led->led_devs[i].brightness_set = gt683r_brightness_set;
> > +
> >  		ret = led_classdev_register(&hdev->dev, &led->led_devs[i]);
> >  		if (ret) {
> >  			hid_err(hdev, "could not register led device\n");
> > @@ -268,17 +280,29 @@ static int gt683r_led_probe(struct hid_device *hdev,
> >  		}
> >  	}
> >  
> > -	ret = device_create_file(&led->hdev->dev, &dev_attr_leds_mode);
> > -	if (ret) {
> > -		hid_err(hdev, "could not make mode attribute file\n");
> > -		goto fail;
> > +	for (i = 0; i < GT683R_LED_COUNT; i++) {
> > +		led->dev_attr_group[i] = &gt683r_group;
> > +
> > +		ret = sysfs_create_group(&led->led_devs[i].dev->kobj,
> > +					led->dev_attr_group[i]);
> 
> why not use sysfs_create_groups()?
> 
> And why are you doing it this way, the led device should have the
> attribute group and it should be created automatically by the driver
> core, no driver should need to create it like you are doing so here.

Turns out it was easier said than done when I asked Janne to move the
attribute. And all led drivers with custom attributes currently suffer
from this race.

Janne, you'll need to apply patch below and then set the groups field of
the led_devs before registering them. You can use the ATTRIBUTE_GROUPS
macro in the following way:

	static struct attribute *gt683r_led_attrs[] = {
       		&dev_attr_msi_mode.attr,
       		NULL,
	};
	ATTRIBUTE_GROUPS(gt683r_led);

and then set .groups to gt683r_led_groups.

I have started fixing some of the other led drivers and I'll try to
submit a series (including the below patch) later today.

Thanks,
Johan


>From 226f1de5e094f2ec99f45d486652505ef915af73 Mon Sep 17 00:00:00 2001
From: Johan Hovold <johan@kernel.org>
Date: Wed, 25 Jun 2014 12:33:18 +0200
Subject: [PATCH] leds: add led-class attribute-group support

Allow led-class devices to be created with optional attribute groups.

This is needed in order to allow led drivers to create custom device
attributes in a race-free manner.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/leds/led-class.c | 5 +++--
 include/linux/leds.h     | 2 ++
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index f37d63cf726b..aa29198fca3e 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -210,8 +210,9 @@ static const struct dev_pm_ops leds_class_dev_pm_ops = {
  */
 int led_classdev_register(struct device *parent, struct led_classdev *led_cdev)
 {
-	led_cdev->dev = device_create(leds_class, parent, 0, led_cdev,
-				      "%s", led_cdev->name);
+	led_cdev->dev = device_create_with_groups(leds_class, parent, 0,
+					led_cdev, led_cdev->groups,
+					"%s", led_cdev->name);
 	if (IS_ERR(led_cdev->dev))
 		return PTR_ERR(led_cdev->dev);
 
diff --git a/include/linux/leds.h b/include/linux/leds.h
index 0287ab296689..e43686472197 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -63,6 +63,8 @@ struct led_classdev {
 				     unsigned long *delay_off);
 
 	struct device		*dev;
+	const struct attribute_group	**groups;
+
 	struct list_head	 node;			/* LED Device list */
 	const char		*default_trigger;	/* Trigger to use */
 
-- 
1.8.5.5