[PATCH v4] HID: lg4ff: Remove sysfs iface before deallocating memory

[PATCH v4] HID: lg4ff: Remove sysfs iface before deallocating memory

[Michal Malý]

[-- Attachment #1: Type: text/plain, Size: 1970 bytes --]

Hi,

This patch fixes a possible race condition caused by the sysfs
interface being removed after the memory used by the interface
was already kfree'd.
Please note that this patch depends on "hid-lg: Allow for custom
device-specific properties to be stored in private drv data" because
it also fixes a tiny glitch - a leftover #include in the hid-lg.h

I have another lg4ff patch pending that makes use of the new possibility to
store device-specific data which is currently being review by Simon.
The patch as it is right now will depend on this patch.

Signed-off-by: Michal Malý <madcatsxter@gmail.com>

---
 drivers/hid/hid-lg.h    |    2 --
 drivers/hid/hid-lg4ff.c |    6 ++++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/hid/hid-lg.h b/drivers/hid/hid-lg.h
index 500457b..d64cf8d 100644
--- a/drivers/hid/hid-lg.h
+++ b/drivers/hid/hid-lg.h
@@ -1,8 +1,6 @@
 #ifndef __HID_LG_H
 #define __HID_LG_H
 
-#include <linux/spinlock.h>
-
 struct lg_drv_data {
 	unsigned long quirks;
 	void *device_props;	/* Device specific properties */
diff --git a/drivers/hid/hid-lg4ff.c b/drivers/hid/hid-lg4ff.c
index 6ecc9e2..1145292 100644
--- a/drivers/hid/hid-lg4ff.c
+++ b/drivers/hid/hid-lg4ff.c
@@ -466,6 +466,9 @@ int lg4ff_deinit(struct hid_device *hid)
 	bool found = 0;
 	struct lg4ff_device_entry *entry;
 	struct list_head *h, *g;
+	
+	device_remove_file(&hid->dev, &dev_attr_range);
+
 	list_for_each_safe(h, g, &device_list.list) {
 		entry = list_entry(h, struct lg4ff_device_entry, list);
 		if (strcmp(entry->device_id, (&hid->dev)->kobj.name) == 0) {
@@ -478,11 +481,10 @@ int lg4ff_deinit(struct hid_device *hid)
 	}
 
 	if (!found) {
-		dbg_hid("Device entry not found!\n");
+		hid_err(hid, "Device entry not found!\n");
 		return -1;
 	}
 
-	device_remove_file(&hid->dev, &dev_attr_range);
 	dbg_hid("Device successfully unregistered\n");
 	return 0;
 }
-- 
1.7.9.5


[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

Re: [PATCH v4] HID: lg4ff: Remove sysfs iface before deallocating memory

[simon]

Hi all,
Looks good to me, built and tested working with a G27.

I also have the 3rd patch (mentioned by Michal) and a 4th which implements
LED control on the G27. How do you want these sent? (they need to be built
on top of Michal's patches)

Simon

Signed-off-by: simon@mungewell.org

> Hi,
>
> This patch fixes a possible race condition caused by the sysfs
> interface being removed after the memory used by the interface
> was already kfree'd.
> Please note that this patch depends on "hid-lg: Allow for custom
> device-specific properties to be stored in private drv data" because
> it also fixes a tiny glitch - a leftover #include in the hid-lg.h
>
> I have another lg4ff patch pending that makes use of the new possibility
> to
> store device-specific data which is currently being review by Simon.
> The patch as it is right now will depend on this patch.
>
> Signed-off-by: Michal Malý <madcatsxter@gmail.com>
>

--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/2] HID: hid-lg4ff: Use Private Data

[Simon Wood]

Use private data in hid-lg4ff to store device properties.

This code was writen by Michal, he asked me to check it and forward
it on to the list.

Signed-off-by: Michal Maly <madcatxster@gmail.com>
Signed-off-by: Simon Wood <simon@mungewell.org>
---
 drivers/hid/hid-lg4ff.c |   96 +++++++++++++++++++++-------------------------
 1 files changed, 44 insertions(+), 52 deletions(-)

diff --git a/drivers/hid/hid-lg4ff.c b/drivers/hid/hid-lg4ff.c
index 1145292..c3146e0 100644
--- a/drivers/hid/hid-lg4ff.c
+++ b/drivers/hid/hid-lg4ff.c
@@ -51,10 +51,7 @@ static ssize_t lg4ff_range_store(struct device *dev, struct device_attribute *at
 
 static DEVICE_ATTR(range, S_IRWXU | S_IRWXG | S_IRWXO, lg4ff_range_show, lg4ff_range_store);
 
-static bool list_inited;
-
 struct lg4ff_device_entry {
-	char  *device_id;	/* Use name in respective kobject structure's address as the ID */
 	__u16 range;
 	__u16 min_range;
 	__u16 max_range;
@@ -63,8 +60,6 @@ struct lg4ff_device_entry {
 	void (*set_range)(struct hid_device *hid, u16 range);
 };
 
-static struct lg4ff_device_entry device_list;
-
 static const signed short lg4ff_wheel_effects[] = {
 	FF_CONSTANT,
 	FF_AUTOCENTER,
@@ -285,18 +280,20 @@ static void hid_lg4ff_switch_native(struct hid_device *hid, const struct lg4ff_n
 /* Read current range and display it in terminal */
 static ssize_t lg4ff_range_show(struct device *dev, struct device_attribute *attr, char *buf)
 {
-	struct lg4ff_device_entry *uninitialized_var(entry);
-	struct list_head *h;
 	struct hid_device *hid = to_hid_device(dev);
+	struct lg4ff_device_entry *uninitialized_var(entry);
+	struct lg_drv_data *uninitialized_var(drv_data);
 	size_t count;
 
-	list_for_each(h, &device_list.list) {
-		entry = list_entry(h, struct lg4ff_device_entry, list);
-		if (strcmp(entry->device_id, (&hid->dev)->kobj.name) == 0)
-			break;
+	drv_data = hid_get_drvdata(hid);
+	if (!drv_data) {
+		hid_err(hid, "Private driver data not found!\n");
+		return 0;
 	}
-	if (h == &device_list.list) {
-		dbg_hid("Device not found!");
+
+	entry = drv_data->device_props;
+	if (!entry) {
+		hid_err(hid, "Device properties not found!\n");
 		return 0;
 	}
 
@@ -308,19 +305,21 @@ static ssize_t lg4ff_range_show(struct device *dev, struct device_attribute *att
  * according to the type of the wheel */
 static ssize_t lg4ff_range_store(struct device *dev, struct device_attribute *attr, const char *buf, size_t count)
 {
-	struct lg4ff_device_entry *uninitialized_var(entry);
-	struct list_head *h;
 	struct hid_device *hid = to_hid_device(dev);
+	struct lg4ff_device_entry *uninitialized_var(entry);
+	struct lg_drv_data *drv_data;
 	__u16 range = simple_strtoul(buf, NULL, 10);
 
-	list_for_each(h, &device_list.list) {
-		entry = list_entry(h, struct lg4ff_device_entry, list);
-		if (strcmp(entry->device_id, (&hid->dev)->kobj.name) == 0)
-			break;
+	drv_data = hid_get_drvdata(hid);
+	if (!drv_data) {
+		hid_err(hid, "Private driver data not found!\n");
+		return 0;
 	}
-	if (h == &device_list.list) {
-		dbg_hid("Device not found!");
-		return count;
+
+	entry = drv_data->device_props;
+	if (!entry) {
+		hid_err(hid, "Device properties not found!\n");
+		return 0;
 	}
 
 	if (range == 0)
@@ -344,6 +343,7 @@ int lg4ff_init(struct hid_device *hid)
 	struct hid_report *report;
 	struct hid_field *field;
 	struct lg4ff_device_entry *entry;
+	struct lg_drv_data *drv_data;
 	struct usb_device_descriptor *udesc;
 	int error, i, j;
 	__u16 bcdDevice, rev_maj, rev_min;
@@ -423,28 +423,24 @@ int lg4ff_init(struct hid_device *hid)
 		dev->ff->set_autocenter(dev, 0);
 	}
 
-		/* Initialize device_list if this is the first device to handle by lg4ff */
-	if (!list_inited) {
-		INIT_LIST_HEAD(&device_list.list);
-		list_inited = 1;
+	/* Get private driver data */
+	drv_data = hid_get_drvdata(hid);
+	if (!drv_data) {
+		hid_err(hid, "Cannot add device, private driver data not allocated\n");
+		return -1;
 	}
-
-	/* Add the device to device_list */
+	
+	/* Initialize device properties */
 	entry = kzalloc(sizeof(struct lg4ff_device_entry), GFP_KERNEL);
 	if (!entry) {
-		hid_err(hid, "Cannot add device, insufficient memory.\n");
-		return -ENOMEM;
-	}
-	entry->device_id = kstrdup((&hid->dev)->kobj.name, GFP_KERNEL);
-	if (!entry->device_id) {
-		hid_err(hid, "Cannot set device_id, insufficient memory.\n");
-		kfree(entry);
+		hid_err(hid, "Cannot add device, insufficient memory to allocate device properties.\n");
 		return -ENOMEM;
 	}
+	drv_data->device_props = entry;
+
 	entry->min_range = lg4ff_devices[i].min_range;
 	entry->max_range = lg4ff_devices[i].max_range;
 	entry->set_range = lg4ff_devices[i].set_range;
-	list_add(&entry->list, &device_list.list);
 
 	/* Create sysfs interface */
 	error = device_create_file(&hid->dev, &dev_attr_range);
@@ -463,27 +459,23 @@ int lg4ff_init(struct hid_device *hid)
 
 int lg4ff_deinit(struct hid_device *hid)
 {
-	bool found = 0;
-	struct lg4ff_device_entry *entry;
-	struct list_head *h, *g;
+	struct lg4ff_device_entry *uninitialized_var(entry);
+	struct lg_drv_data *uninitialized_var(drv_data);
 	
 	device_remove_file(&hid->dev, &dev_attr_range);
-
-	list_for_each_safe(h, g, &device_list.list) {
-		entry = list_entry(h, struct lg4ff_device_entry, list);
-		if (strcmp(entry->device_id, (&hid->dev)->kobj.name) == 0) {
-			list_del(h);
-			kfree(entry->device_id);
-			kfree(entry);
-			found = 1;
-			break;
-		}
+	
+	drv_data = hid_get_drvdata(hid);
+	if (!drv_data) {
+		hid_err(hid, "Error while deinitializing device, no private driver data.\n");
+		return -1;
 	}
-
-	if (!found) {
-		hid_err(hid, "Device entry not found!\n");
+	entry = drv_data->device_props;
+	if (!entry) {
+		hid_err(hid, "Error while deinitializing device, no device properties data.\n");
 		return -1;
 	}
+	/* Deallocate memory */
+	kfree(entry);
 
 	dbg_hid("Device successfully unregistered\n");
 	return 0;
-- 
1.7.4.1

[PATCH 2/2] HID: hid-lg4ff: Add support for G27 leds

[Simon Wood]

This patch adds supports for controlling the LED 'tachometer' on
the G27 wheel, via the LED subsystem.

The 5 LEDs are arranged from right (1=grn, 2=grn, 3=yel, 4=yel, 5=red)
and 'mirrored' to the left (10 LEDs in total).

Signed-off-by: Simon Wood <simon@mungewell.org>
---
 drivers/hid/hid-lg4ff.c |  159 ++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 158 insertions(+), 1 deletions(-)

diff --git a/drivers/hid/hid-lg4ff.c b/drivers/hid/hid-lg4ff.c
index c3146e0..afd13ee 100644
--- a/drivers/hid/hid-lg4ff.c
+++ b/drivers/hid/hid-lg4ff.c
@@ -55,7 +55,8 @@ struct lg4ff_device_entry {
 	__u16 range;
 	__u16 min_range;
 	__u16 max_range;
-	__u8  leds;
+	__u8  led_state;
+	struct led_classdev *led[5];
 	struct list_head list;
 	void (*set_range)(struct hid_device *hid, u16 range);
 };
@@ -335,6 +336,92 @@ static ssize_t lg4ff_range_store(struct device *dev, struct device_attribute *at
 	return count;
 }
 
+static void lg4ff_set_leds(struct hid_device *hid, __u8 leds)
+{
+	struct list_head *report_list = &hid->report_enum[HID_OUTPUT_REPORT].report_list;
+	struct hid_report *report = list_entry(report_list->next, struct hid_report, list);
+
+	report->field[0]->value[0] = 0xf8;
+	report->field[0]->value[1] = 0x12;
+	report->field[0]->value[2] = leds;
+	report->field[0]->value[3] = 0x00;
+	report->field[0]->value[4] = 0x00;
+	report->field[0]->value[5] = 0x00;
+	report->field[0]->value[6] = 0x00;
+	usbhid_submit_report(hid, report, USB_DIR_OUT);
+}
+
+static void lg4ff_led_set_brightness(struct led_classdev *led_cdev,
+			enum led_brightness value)
+{
+	struct device *dev;
+	struct hid_device *hid;
+	struct lg4ff_device_entry *uninitialized_var(entry);
+	int i, state = 0;
+	struct lg_drv_data* drv_data;
+	dev = led_cdev->dev->parent;
+	hid = container_of(dev, struct hid_device, dev);
+	drv_data = (struct lg_drv_data *)hid_get_drvdata(hid);
+
+	if (!drv_data) {
+		hid_err(hid, "Device data not found.");
+		return;
+	}
+
+	entry = (struct lg4ff_device_entry *)drv_data->device_props; 
+
+	if (!entry) {
+		hid_err(hid, "Device properties not found.");
+		return;
+	}
+
+	for (i = 0; i < 5; i++) {
+		if (led_cdev != entry->led[i])
+			continue;
+		state = (entry->led_state >> i) & 1;
+		if (value == LED_OFF && state) {
+			entry->led_state &= ~(1 << i);
+			lg4ff_set_leds(hid, entry->led_state);
+		} else if (value != LED_OFF && !state) {
+			entry->led_state |= 1 << i;
+			lg4ff_set_leds(hid, entry->led_state);
+		}
+		break;
+	}
+}
+
+static enum led_brightness lg4ff_led_get_brightness(struct led_classdev *led_cdev)
+{
+	struct device *dev;
+	struct hid_device *hid;
+	struct lg4ff_device_entry *uninitialized_var(entry);
+	int i, value = 0;
+	struct lg_drv_data* drv_data;
+	dev = led_cdev->dev->parent;
+	hid = container_of(dev, struct hid_device, dev);
+	drv_data = (struct lg_drv_data *)hid_get_drvdata(hid);
+
+	if (!drv_data) {
+		hid_err(hid, "Device data not found.");
+		return LED_OFF;
+	}
+
+	entry = (struct lg4ff_device_entry *)drv_data->device_props; 
+
+	if (!entry) {
+		hid_err(hid, "Device properties not found.");
+		return LED_OFF;
+	}
+
+	for (i = 0; i < 5; i++)
+		if (led_cdev == entry->led[i]) {
+			value = (entry->led_state >> i) & 1;
+			break;
+		}
+
+	return value ? LED_FULL : LED_OFF;
+}
+
 int lg4ff_init(struct hid_device *hid)
 {
 	struct hid_input *hidinput = list_entry(hid->inputs.next, struct hid_input, list);
@@ -347,6 +434,9 @@ int lg4ff_init(struct hid_device *hid)
 	struct usb_device_descriptor *udesc;
 	int error, i, j;
 	__u16 bcdDevice, rev_maj, rev_min;
+	struct led_classdev *led;
+	size_t name_sz;
+	char *name;
 
 	/* Find the report to use */
 	if (list_empty(report_list)) {
@@ -453,14 +543,70 @@ int lg4ff_init(struct hid_device *hid)
 	if (entry->set_range != NULL)
 		entry->set_range(hid, entry->range);
 
+	/* register led subsystem - G27 only */
+	entry->led_state = 0;
+	entry->led[0] = NULL;
+	entry->led[1] = NULL;
+	entry->led[2] = NULL;
+	entry->led[3] = NULL;
+	entry->led[4] = NULL;
+	entry->led[5] = NULL;
+
+	if (lg4ff_devices[i].product_id == USB_DEVICE_ID_LOGITECH_G27_WHEEL) {
+		lg4ff_set_leds(hid, 0);
+
+		name_sz = strlen(dev_name(&hid->dev)) + 8;
+
+		for (i = 0; i < 5; i++) {
+			led = kzalloc(sizeof(struct led_classdev)+name_sz, GFP_KERNEL);
+			if (!led) {
+				hid_err(hid, "can't allocate memory for LED %d\n", i);
+				error = -ENOMEM;
+				goto err;
+			}
+
+			name = (void *)(&led[1]);
+			snprintf(name, name_sz, "%s::RPM%d", dev_name(&hid->dev), i+1);
+			led->name = name;
+			led->brightness = 0;
+			led->max_brightness = 1;
+			led->brightness_get = lg4ff_led_get_brightness;
+			led->brightness_set = lg4ff_led_set_brightness;
+
+			entry->led[i] = led;
+			error = led_classdev_register(&hid->dev, led);
+			if (error) {
+				hid_err(hid, "failed to register LED %d. Aborting.\n", i);
+				goto err;
+			}
+		}
+
+		dbg_hid("sysfs interface created for leds\n");
+	}
+
 	hid_info(hid, "Force feedback for Logitech Speed Force Wireless by Simon Wood <simon@mungewell.org>\n");
 	return 0;
+
+err:
+	/* Deregister LEDs (if any) but let the driver continue */
+	for (i = 0; i < 5; i++) {
+		led = entry->led[i];
+		entry->led[i] = NULL;
+		if (!led)
+			continue;
+		led_classdev_unregister(led);
+		kfree(led);
+	}
+
+	return 0;
 }
 
 int lg4ff_deinit(struct hid_device *hid)
 {
 	struct lg4ff_device_entry *uninitialized_var(entry);
 	struct lg_drv_data *uninitialized_var(drv_data);
+	int i;
+	struct led_classdev *led;
 	
 	device_remove_file(&hid->dev, &dev_attr_range);
 	
@@ -474,6 +620,17 @@ int lg4ff_deinit(struct hid_device *hid)
 		hid_err(hid, "Error while deinitializing device, no device properties data.\n");
 		return -1;
 	}
+
+	/* Deregister LEDs (if any) */
+	for (i = 0; i < 5; i++) {
+		led = entry->led[i];
+		entry->led[i] = NULL;
+		if (!led)
+			continue;
+		led_classdev_unregister(led);
+		kfree(led);
+	}
+
 	/* Deallocate memory */
 	kfree(entry);
 
-- 
1.7.4.1

Re: [PATCH 1/2] HID: hid-lg4ff: Use Private Data

[Jiri Slaby]

On 04/02/2012 04:54 PM, Simon Wood wrote:
> Use private data in hid-lg4ff to store device properties.
> 
> This code was writen by Michal, he asked me to check it and forward
> it on to the list.

So this patch is missing proper From: header.

> Signed-off-by: Michal Maly <madcatxster@gmail.com>
> Signed-off-by: Simon Wood <simon@mungewell.org>
> ---
>  drivers/hid/hid-lg4ff.c |   96 +++++++++++++++++++++-------------------------
>  1 files changed, 44 insertions(+), 52 deletions(-)
> 
> diff --git a/drivers/hid/hid-lg4ff.c b/drivers/hid/hid-lg4ff.c
> index 1145292..c3146e0 100644
> --- a/drivers/hid/hid-lg4ff.c
> +++ b/drivers/hid/hid-lg4ff.c
> @@ -285,18 +280,20 @@ static void hid_lg4ff_switch_native(struct hid_device *hid, const struct lg4ff_n
>  /* Read current range and display it in terminal */
>  static ssize_t lg4ff_range_show(struct device *dev, struct device_attribute *attr, char *buf)
>  {
> -	struct lg4ff_device_entry *uninitialized_var(entry);
> -	struct list_head *h;
>  	struct hid_device *hid = to_hid_device(dev);
> +	struct lg4ff_device_entry *uninitialized_var(entry);
> +	struct lg_drv_data *uninitialized_var(drv_data);

You don't need uninitialized_var bloat anymroe, right?

Here and at the other places too...

>  	size_t count;
>  
> -	list_for_each(h, &device_list.list) {
> -		entry = list_entry(h, struct lg4ff_device_entry, list);
> -		if (strcmp(entry->device_id, (&hid->dev)->kobj.name) == 0)
> -			break;
> +	drv_data = hid_get_drvdata(hid);
> +	if (!drv_data) {
> +		hid_err(hid, "Private driver data not found!\n");
> +		return 0;
>  	}
> -	if (h == &device_list.list) {
> -		dbg_hid("Device not found!");
> +
> +	entry = drv_data->device_props;
> +	if (!entry) {
> +		hid_err(hid, "Device properties not found!\n");
>  		return 0;
>  	}
>  

-- 
js
suse labs

Re: [PATCH 2/2] HID: hid-lg4ff: Add support for G27 leds

[Jiri Slaby]

On 04/02/2012 04:54 PM, Simon Wood wrote:
> This patch adds supports for controlling the LED 'tachometer' on
> the G27 wheel, via the LED subsystem.
> 
> The 5 LEDs are arranged from right (1=grn, 2=grn, 3=yel, 4=yel, 5=red)
> and 'mirrored' to the left (10 LEDs in total).
> 
> Signed-off-by: Simon Wood <simon@mungewell.org>
> ---
>  drivers/hid/hid-lg4ff.c |  159 ++++++++++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 158 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/hid/hid-lg4ff.c b/drivers/hid/hid-lg4ff.c
> index c3146e0..afd13ee 100644
> --- a/drivers/hid/hid-lg4ff.c
> +++ b/drivers/hid/hid-lg4ff.c
> @@ -55,7 +55,8 @@ struct lg4ff_device_entry {
>  	__u16 range;
>  	__u16 min_range;
>  	__u16 max_range;
> -	__u8  leds;
> +	__u8  led_state;
> +	struct led_classdev *led[5];
>  	struct list_head list;
>  	void (*set_range)(struct hid_device *hid, u16 range);
>  };
> @@ -335,6 +336,92 @@ static ssize_t lg4ff_range_store(struct device *dev, struct device_attribute *at
>  	return count;
>  }
>  
> +static void lg4ff_set_leds(struct hid_device *hid, __u8 leds)
> +{
> +	struct list_head *report_list = &hid->report_enum[HID_OUTPUT_REPORT].report_list;
> +	struct hid_report *report = list_entry(report_list->next, struct hid_report, list);
> +
> +	report->field[0]->value[0] = 0xf8;
> +	report->field[0]->value[1] = 0x12;
> +	report->field[0]->value[2] = leds;
> +	report->field[0]->value[3] = 0x00;
> +	report->field[0]->value[4] = 0x00;
> +	report->field[0]->value[5] = 0x00;
> +	report->field[0]->value[6] = 0x00;
> +	usbhid_submit_report(hid, report, USB_DIR_OUT);
> +}
> +
> +static void lg4ff_led_set_brightness(struct led_classdev *led_cdev,
> +			enum led_brightness value)
> +{
> +	struct device *dev;
> +	struct hid_device *hid;
> +	struct lg4ff_device_entry *uninitialized_var(entry);
> +	int i, state = 0;
> +	struct lg_drv_data* drv_data;
> +	dev = led_cdev->dev->parent;
> +	hid = container_of(dev, struct hid_device, dev);
> +	drv_data = (struct lg_drv_data *)hid_get_drvdata(hid);
...
> +static enum led_brightness lg4ff_led_get_brightness(struct led_classdev *led_cdev)
> +{
> +	struct device *dev;
> +	struct hid_device *hid;
> +	struct lg4ff_device_entry *uninitialized_var(entry);
> +	int i, value = 0;
> +	struct lg_drv_data* drv_data;
> +	dev = led_cdev->dev->parent;
> +	hid = container_of(dev, struct hid_device, dev);
> +	drv_data = (struct lg_drv_data *)hid_get_drvdata(hid);

What a mess. Do you need all the uninitialized_var's, casts here and there?

Also this would be readable if you did all the stuff directly as an
initializer for all the local variables.

And make sure to check the patch by scripts/checkpatch.pl.

> +	if (!drv_data) {
> +		hid_err(hid, "Device data not found.");
> +		return LED_OFF;
> +	}
> +
> +	entry = (struct lg4ff_device_entry *)drv_data->device_props; 
> +
> +	if (!entry) {
> +		hid_err(hid, "Device properties not found.");
> +		return LED_OFF;
> +	}
> +
> +	for (i = 0; i < 5; i++)
> +		if (led_cdev == entry->led[i]) {
> +			value = (entry->led_state >> i) & 1;
> +			break;
> +		}
> +
> +	return value ? LED_FULL : LED_OFF;
> +}

thanks,
-- 
js
suse labs

Re: [PATCH 1/2] HID: hid-lg4ff: Use Private Data

[simon]

>> +	struct lg4ff_device_entry *uninitialized_var(entry);
>> +	struct lg_drv_data *uninitialized_var(drv_data);
>
> You don't need uninitialized_var bloat anymroe, right?

I guess I don't fully understand the 'unitialized_var()' macro, and was
just following the previous uses in the code.

Google'ing doesn't really help either. Is there a definitive guide as when
to use and not?

Simon

Re: [PATCH 1/2] HID: hid-lg4ff: Use Private Data

[Jiri Slaby]

On 04/02/2012 07:01 PM, simon@mungewell.org wrote:
> 
>>> +	struct lg4ff_device_entry *uninitialized_var(entry);
>>> +	struct lg_drv_data *uninitialized_var(drv_data);
>>
>> You don't need uninitialized_var bloat anymroe, right?
> 
> I guess I don't fully understand the 'unitialized_var()' macro, and was
> just following the previous uses in the code.
> 
> Google'ing doesn't really help either. Is there a definitive guide as when
> to use and not?

The simple answer is: never.

It serves the purpose to shut up the compiler when it complains that the
variable might be uninitialized, but the programmer is sure it cannot.
In your case, the compiler should not even complain about that.

Advantage of the macro is that it adds no assembly, but still silents
the compiler.

thanks,
-- 
js
suse labs

Re: [PATCH v4] HID: lg4ff: Remove sysfs iface before deallocating memory

[Jiri Kosina]

On Sat, 31 Mar 2012, Michal Malý wrote:

> Hi,
> 
> This patch fixes a possible race condition caused by the sysfs
> interface being removed after the memory used by the interface
> was already kfree'd.
> Please note that this patch depends on "hid-lg: Allow for custom
> device-specific properties to be stored in private drv data" because
> it also fixes a tiny glitch - a leftover #include in the hid-lg.h

Applied, thanks.

-- 
Jiri Kosina
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] HID: hid-lg4ff: Use Private Data

[Jiri Kosina]

On Mon, 2 Apr 2012, Simon Wood wrote:

> Use private data in hid-lg4ff to store device properties.

I'd like to have a more verbose changelog -- namely what advantage this 
brings compared to the previous state.

Thanks.

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH 1/2] HID: hid-lg4ff: Use Private Data

[Jiri Kosina]

On Mon, 2 Apr 2012, simon@mungewell.org wrote:

> >> +	struct lg4ff_device_entry *uninitialized_var(entry);
> >> +	struct lg_drv_data *uninitialized_var(drv_data);
> >
> > You don't need uninitialized_var bloat anymroe, right?
> 
> I guess I don't fully understand the 'unitialized_var()' macro, and was
> just following the previous uses in the code.
> 
> Google'ing doesn't really help either. Is there a definitive guide as when
> to use and not?

As Jiri Slaby already explained, it's rather a hack to shut up gcc in 
cases in which it gets the unused warning wrong.

But even that is considered controversial by some people ... 

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH 1/2] HID: hid-lg4ff: Use Private Data

[Michal Malý]

[-- Attachment #1: Type: text/plain, Size: 1151 bytes --]

On Tuesday 03. of April 2012 4:13:37 you wrote:
> On Mon, 2 Apr 2012, Simon Wood wrote:
> > Use private data in hid-lg4ff to store device properties.
> 
> I'd like to have a more verbose changelog -- namely what advantage this
> brings compared to the previous state.
> 
> Thanks.
Hi,

the previous implementation used a linked list to store the device 
configuration (at this point it was just the operating range, but I have plans 
for further expansion, I guess Simon's LEDs patch could use that too). 
Searching through the list every time the user adjusted the range wasn't 
exactly fast, particularly because the list used kobj name as the ID. I 
originally opted for that solution because I didn't want to mess with the code 
of the whole hid-lg driver.
This patch takes advantage of the changes introduced in my patches from 
2011/04/02. lg4ff now calls hid_get/set_drvdata() to read or store device 
configuration. The way I understand it, this is how all HID drivers store their 
private data.

Since J. Slabý complained about unnecessary uninitialized_var() macros, I'll 
update the patch to remove them.

Michal M.

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 490 bytes --]