Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1437063 > unrolled thread

[PATCH 00/27] HID: wacom: cleanup/EKR/LED

Back to article view | Back to linux.kernel

Contents

  [PATCH 00/27] HID: wacom: cleanup/EKR/LED Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2016-07-05 16:40 +0200
    [PATCH 23/27] HID: wacom: leds: use the ledclass instead of custom made sysfs files Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2016-07-05 16:50 +0200
    [PATCH 16/27] HID: wacom: rework fail path in probe() and parse_and_register() Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2016-07-05 16:50 +0200
    [PATCH 17/27] HID: wacom: EKR: have proper allocator and destructor Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2016-07-05 16:50 +0200
    [PATCH 11/27] HID: wacom: use devres to allocate driver data Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2016-07-05 16:50 +0200
    [PATCH 07/27] HID: wacom: switch inputs to devres Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2016-07-05 16:50 +0200
    [PATCH 25/27] HID: wacom: leds: fix ordering of LED banks Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2016-07-05 16:50 +0200
    [PATCH 08/27] HID: wacom: put the managed resources in a group Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2016-07-05 16:50 +0200
    [PATCH 26/27] HID: wacom: leds: handle the switch of the LEDs directly in the kernel Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2016-07-05 16:50 +0200
      Re: [PATCH 26/27] HID: wacom: leds: handle the switch of the LEDs  directly in the kernel kbuild test robot <lkp@intel.com> - 2016-07-05 18:10 +0200
        Re: [PATCH 26/27] HID: wacom: leds: handle the switch of the LEDs  directly in the kernel Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2016-07-05 23:40 +0200
    [PATCH 02/27] HID: wacom: store the type in wacom->shared for INTUOSHT and INTUOSHT2 Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2016-07-05 16:50 +0200
    [PATCH 10/27] HID: wacom: use devm_kasprintf for allocating the name of the remote Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2016-07-05 16:50 +0200
    [PATCH 22/27] HID: wacom: EKR: attach the power_supply on first connection Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2016-07-05 16:50 +0200
    [PATCH 01/27] HID: wacom: actually report the battery level for wireless connected Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2016-07-05 16:50 +0200
    [PATCH 21/27] HID: wacom: EKR: have one power_supply per remote Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2016-07-05 16:50 +0200
    [PATCH 06/27] HID: wacom: switch battery to devres Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2016-07-05 16:50 +0200
    [PATCH 09/27] HID: wacom: convert LEDs to devres Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2016-07-05 16:50 +0200
    [PATCH 20/27] HID: wacom: EKR: allocate one input node per remote Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2016-07-05 16:50 +0200
    [PATCH 27/27] HID: wacom: leds: handle Cintiq 24HD leds buttons Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2016-07-05 16:50 +0200
      Re: [PATCH 27/27] HID: wacom: leds: handle Cintiq 24HD leds buttons kbuild test robot <lkp@intel.com> - 2016-07-05 18:30 +0200
    [PATCH 19/27] HID: wacom: EKR: have one array of struct remotes instead of many arrays Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2016-07-05 16:50 +0200
    [PATCH 12/27] HID: wacom: devres manage the shared data too Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2016-07-05 16:50 +0200
    [PATCH 18/27] HID: wacom: EKR: use devres groups to manage resources Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2016-07-05 16:50 +0200
    [PATCH 13/27] HID: wacom: leds: dynamically allocate LED groups Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2016-07-05 16:50 +0200
    [PATCH 15/27] HID: wacom: EKR: have the wacom resources dynamically allocated Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2016-07-05 16:50 +0200
    [PATCH 14/27] HID: wacom: EKR: add a worker to add/remove resources on addition/removal Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2016-07-05 16:50 +0200
    [PATCH 24/27] HID: wacom: leds: actually release the LEDs on disconnect Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2016-07-05 16:50 +0200
    [PATCH 05/27] HID: wacom: use one work queue per task Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2016-07-05 16:50 +0200

Page 2 of 2 — ← Prev page 1 [2]

#1437178 — Re: [PATCH 27/27] HID: wacom: leds: handle Cintiq 24HD leds buttons

[Multipart message — attachments visible in raw view] — view raw

Hi,

[auto build test ERROR on hid/for-next]
[also build test ERROR on v4.7-rc6 next-20160705]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Benjamin-Tissoires/HID-wacom-cleanup-EKR-LED/20160705-225431
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jikos/hid.git for-next
config: i386-randconfig-a0-201627 (attached as .config)
compiler: gcc-6 (Debian 6.1.1-1) 6.1.1 20160430
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   In file included from include/linux/kernel.h:13:0,
                    from include/asm-generic/bug.h:13,
                    from arch/x86/include/asm/bug.h:35,
                    from include/linux/bug.h:4,
                    from include/linux/mmdebug.h:4,
                    from include/linux/gfp.h:4,
                    from include/linux/slab.h:14,
                    from include/linux/hid.h:30,
                    from drivers/hid/wacom_wac.h:13,
                    from drivers/hid/wacom_wac.c:15:
   drivers/hid/wacom_wac.c: In function 'wacom_update_led':
>> drivers/hid/wacom_wac.c:2844:20: error: 'struct led_trigger' has no member named 'name'
      next_led->trigger.name,
                       ^
   include/linux/printk.h:264:33: note: in definition of macro 'pr_err'
     printk(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
                                    ^~~~~~~~~~~

vim +2844 drivers/hid/wacom_wac.c

  2838			return;
  2839	
  2840		pr_err("%s group: %d led: (%d -> %d) t: %s %s:%d\n", __func__,
  2841			led->group,
  2842			led->id,
  2843			next_led->id,
> 2844			next_led->trigger.name,
  2845			__FILE__, __LINE__);
  2846	
  2847		next_led->held = true;

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[toc] | [prev] | [next] | [standalone]

#1437086 — [PATCH 19/27] HID: wacom: EKR: have one array of struct remotes instead of many arrays

No functional changes, just a prep patch for the one after.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/wacom.h     |  6 ++++--
 drivers/hid/wacom_sys.c | 53 +++++++++++++++++++++++++------------------------
 drivers/hid/wacom_wac.c |  2 +-
 3 files changed, 32 insertions(+), 29 deletions(-)

diff --git a/drivers/hid/wacom.h b/drivers/hid/wacom.h
index 110ea67..6b8df67 100644
--- a/drivers/hid/wacom.h
+++ b/drivers/hid/wacom.h
@@ -120,8 +120,10 @@ struct wacom_remote {
 	spinlock_t remote_lock;
 	struct kfifo remote_fifo;
 	struct kobject *remote_dir;
-	struct attribute_group remote_group[WACOM_MAX_REMOTES];
-	__u32 serial[WACOM_MAX_REMOTES];
+	struct {
+		struct attribute_group group;
+		u32 serial;
+	} remotes[WACOM_MAX_REMOTES];
 };
 
 struct wacom {
diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
index 9121e72..c3b2692 100644
--- a/drivers/hid/wacom_sys.c
+++ b/drivers/hid/wacom_sys.c
@@ -1308,16 +1308,16 @@ static int wacom_remote_create_attr_group(struct wacom *wacom, __u32 serial,
 	int error = 0;
 	struct wacom_remote *remote = wacom->remote;
 
-	remote->remote_group[index].name = devm_kasprintf(&wacom->hdev->dev,
+	remote->remotes[index].group.name = devm_kasprintf(&wacom->hdev->dev,
 							  GFP_KERNEL,
 							  "%d", serial);
-	if (!remote->remote_group[index].name)
+	if (!remote->remotes[index].group.name)
 		return -ENOMEM;
 
 	error = __wacom_devm_sysfs_create_group(wacom, remote->remote_dir,
-						&remote->remote_group[index]);
+						&remote->remotes[index].group);
 	if (error) {
-		remote->remote_group[index].name = NULL;
+		remote->remotes[index].group.name = NULL;
 		hid_err(wacom->hdev,
 			"cannot create sysfs group err: %d\n", error);
 		return error;
@@ -1421,11 +1421,11 @@ static int wacom_initialize_remotes(struct wacom *wacom)
 		return -ENOMEM;
 	}
 
-	remote->remote_group[0] = remote0_serial_group;
-	remote->remote_group[1] = remote1_serial_group;
-	remote->remote_group[2] = remote2_serial_group;
-	remote->remote_group[3] = remote3_serial_group;
-	remote->remote_group[4] = remote4_serial_group;
+	remote->remotes[0].group = remote0_serial_group;
+	remote->remotes[1].group = remote1_serial_group;
+	remote->remotes[2].group = remote2_serial_group;
+	remote->remotes[3].group = remote3_serial_group;
+	remote->remotes[4].group = remote4_serial_group;
 
 	remote->remote_dir = kobject_create_and_add("wacom_remote",
 						    &wacom->hdev->dev.kobj);
@@ -1442,7 +1442,7 @@ static int wacom_initialize_remotes(struct wacom *wacom)
 
 	for (i = 0; i < WACOM_MAX_REMOTES; i++) {
 		wacom->led.groups[i].select = WACOM_STATUS_UNKNOWN;
-		remote->serial[i] = 0;
+		remote->remotes[i].serial = 0;
 	}
 
 	error = devm_add_action_or_reset(&wacom->hdev->dev,
@@ -1903,16 +1903,17 @@ fail:
 static void wacom_remote_destroy_one(struct wacom *wacom, unsigned int index)
 {
 	struct wacom_remote *remote = wacom->remote;
-	u32 serial = remote->serial[index];
+	u32 serial = remote->remotes[index].serial;
 	int i;
 
-	if (remote->remote_group[index].name)
-		devres_release_group(&wacom->hdev->dev, &remote->serial[index]);
+	if (remote->remotes[index].group.name)
+		devres_release_group(&wacom->hdev->dev,
+				     &remote->remotes[index]);
 
 	for (i = 0; i < WACOM_MAX_REMOTES; i++) {
-		if (remote->serial[i] == serial) {
-			remote->serial[i] = 0;
-			remote->remote_group[i].name = NULL;
+		if (remote->remotes[i].serial == serial) {
+			remote->remotes[i].serial = 0;
+			remote->remotes[i].group.name = NULL;
 			wacom->led.groups[i].select = WACOM_STATUS_UNKNOWN;
 		}
 	}
@@ -1929,30 +1930,30 @@ static int wacom_remote_create_one(struct wacom *wacom, u32 serial,
 	 * check to make sure this serial isn't already paired.
 	 */
 	for (k = 0; k < WACOM_MAX_REMOTES; k++) {
-		if (remote->serial[k] == serial)
+		if (remote->remotes[k].serial == serial)
 			break;
 	}
 
 	if (k < WACOM_MAX_REMOTES) {
-		remote->serial[index] = serial;
+		remote->remotes[index].serial = serial;
 		return 0;
 	}
 
-	if (!devres_open_group(dev, &remote->serial[index], GFP_KERNEL))
+	if (!devres_open_group(dev, &remote->remotes[index], GFP_KERNEL))
 		return -ENOMEM;
 
 	error = wacom_remote_create_attr_group(wacom, serial, index);
 	if (error)
 		goto fail;
 
-	remote->serial[index] = serial;
+	remote->remotes[index].serial = serial;
 
-	devres_close_group(dev, &remote->serial[index]);
+	devres_close_group(dev, &remote->remotes[index]);
 	return 0;
 
 fail:
-	devres_release_group(dev, &remote->serial[index]);
-	remote->serial[index] = 0;
+	devres_release_group(dev, &remote->remotes[index]);
+	remote->remotes[index].serial = 0;
 	return error;
 }
 
@@ -1986,15 +1987,15 @@ static void wacom_remote_work(struct work_struct *work)
 		serial = data.remote[i].serial;
 		if (data.remote[i].connected) {
 
-			if (remote->serial[i] == serial)
+			if (remote->remotes[i].serial == serial)
 				continue;
 
-			if (remote->serial[i])
+			if (remote->remotes[i].serial)
 				wacom_remote_destroy_one(wacom, i);
 
 			wacom_remote_create_one(wacom, serial, i);
 
-		} else if (remote->serial[i]) {
+		} else if (remote->remotes[i].serial) {
 			wacom_remote_destroy_one(wacom, i);
 		}
 	}
diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
index d5c180d..1162dc5 100644
--- a/drivers/hid/wacom_wac.c
+++ b/drivers/hid/wacom_wac.c
@@ -808,7 +808,7 @@ static int wacom_remote_irq(struct wacom_wac *wacom_wac, size_t len)
 	touch_ring_mode = (data[11] & 0xC0) >> 6;
 
 	for (i = 0; i < WACOM_MAX_REMOTES; i++) {
-		if (remote->serial[i] == serial)
+		if (remote->remotes[i].serial == serial)
 			wacom->led.groups[i].select = touch_ring_mode;
 	}
 
-- 
2.5.5

[toc] | [prev] | [next] | [standalone]

#1437087 — [PATCH 12/27] HID: wacom: devres manage the shared data too

wacom_release_shared_data() and wacom_remove_shared_data() are moved up
so they can be referenced in wacom_add_shared_data().

There is no point in explicitly setting wacom_wac1->shared->type to 0 in
wacom_wireless_work() (plus this would give an oops).

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/wacom_sys.c | 73 ++++++++++++++++++++++++++-----------------------
 1 file changed, 39 insertions(+), 34 deletions(-)

diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
index 56d62e8..d0f5764 100644
--- a/drivers/hid/wacom_sys.c
+++ b/drivers/hid/wacom_sys.c
@@ -567,6 +567,38 @@ static struct wacom_hdev_data *wacom_get_hdev_data(struct hid_device *hdev)
 	return NULL;
 }
 
+static void wacom_release_shared_data(struct kref *kref)
+{
+	struct wacom_hdev_data *data =
+		container_of(kref, struct wacom_hdev_data, kref);
+
+	mutex_lock(&wacom_udev_list_lock);
+	list_del(&data->list);
+	mutex_unlock(&wacom_udev_list_lock);
+
+	kfree(data);
+}
+
+static void wacom_remove_shared_data(void *res)
+{
+	struct wacom *wacom = res;
+	struct wacom_hdev_data *data;
+	struct wacom_wac *wacom_wac = &wacom->wacom_wac;
+
+	if (wacom_wac->shared) {
+		data = container_of(wacom_wac->shared, struct wacom_hdev_data,
+				    shared);
+
+		if (wacom_wac->shared->touch == wacom->hdev)
+			wacom_wac->shared->touch = NULL;
+		else if (wacom_wac->shared->pen == wacom->hdev)
+			wacom_wac->shared->pen = NULL;
+
+		kref_put(&data->kref, wacom_release_shared_data);
+		wacom_wac->shared = NULL;
+	}
+}
+
 static int wacom_add_shared_data(struct hid_device *hdev)
 {
 	struct wacom *wacom = hid_get_drvdata(hdev);
@@ -591,6 +623,13 @@ static int wacom_add_shared_data(struct hid_device *hdev)
 
 	wacom_wac->shared = &data->shared;
 
+	retval = devm_add_action(&hdev->dev, wacom_remove_shared_data, wacom);
+	if (retval) {
+		mutex_unlock(&wacom_udev_list_lock);
+		wacom_remove_shared_data(wacom);
+		return retval;
+	}
+
 	if (wacom_wac->features.device_type & WACOM_DEVICETYPE_TOUCH)
 		wacom_wac->shared->touch = hdev;
 	else if (wacom_wac->features.device_type & WACOM_DEVICETYPE_PEN)
@@ -601,37 +640,6 @@ out:
 	return retval;
 }
 
-static void wacom_release_shared_data(struct kref *kref)
-{
-	struct wacom_hdev_data *data =
-		container_of(kref, struct wacom_hdev_data, kref);
-
-	mutex_lock(&wacom_udev_list_lock);
-	list_del(&data->list);
-	mutex_unlock(&wacom_udev_list_lock);
-
-	kfree(data);
-}
-
-static void wacom_remove_shared_data(struct wacom *wacom)
-{
-	struct wacom_hdev_data *data;
-	struct wacom_wac *wacom_wac = &wacom->wacom_wac;
-
-	if (wacom_wac->shared) {
-		data = container_of(wacom_wac->shared, struct wacom_hdev_data,
-				    shared);
-
-		if (wacom_wac->shared->touch == wacom->hdev)
-			wacom_wac->shared->touch = NULL;
-		else if (wacom_wac->shared->pen == wacom->hdev)
-			wacom_wac->shared->pen = NULL;
-
-		kref_put(&data->kref, wacom_release_shared_data);
-		wacom_wac->shared = NULL;
-	}
-}
-
 static int wacom_led_control(struct wacom *wacom)
 {
 	unsigned char *buf;
@@ -1731,7 +1739,6 @@ fail_remote:
 fail_leds:
 fail_register_inputs:
 fail_battery:
-	wacom_remove_shared_data(wacom);
 fail_shared_data:
 fail_parsed:
 fail_allocate_inputs:
@@ -1771,7 +1778,6 @@ static void wacom_wireless_work(struct work_struct *work)
 
 	if (wacom_wac->pid == 0) {
 		hid_info(wacom->hdev, "wireless tablet disconnected\n");
-		wacom_wac1->shared->type = 0;
 	} else {
 		const struct hid_device_id *id = wacom_ids;
 
@@ -1912,7 +1918,6 @@ static void wacom_remove(struct hid_device *hdev)
 	kobject_put(wacom->remote_dir);
 	if (hdev->bus == BUS_BLUETOOTH)
 		device_remove_file(&hdev->dev, &dev_attr_speed);
-	wacom_remove_shared_data(wacom);
 
 	hid_set_drvdata(hdev, NULL);
 }
-- 
2.5.5

[toc] | [prev] | [next] | [standalone]

#1437088 — [PATCH 18/27] HID: wacom: EKR: use devres groups to manage resources

This will be useful when each remote will be assigned its own input device.
We won't need to unregister each input and sysfs and other elements one
at a time.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/wacom_sys.c | 44 ++++++++++++++++++++++++++++----------------
 1 file changed, 28 insertions(+), 16 deletions(-)

diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
index 27ca147..9121e72 100644
--- a/drivers/hid/wacom_sys.c
+++ b/drivers/hid/wacom_sys.c
@@ -934,8 +934,9 @@ static void wacom_devm_sysfs_group_release(struct device *dev, void *res)
 	sysfs_remove_group(kobj, devres->group);
 }
 
-static int wacom_devm_sysfs_create_group(struct wacom *wacom,
-					 struct attribute_group *group)
+static int __wacom_devm_sysfs_create_group(struct wacom *wacom,
+					   struct kobject *root,
+					   struct attribute_group *group)
 {
 	struct wacom_sysfs_group_devres *devres;
 	int error;
@@ -947,7 +948,7 @@ static int wacom_devm_sysfs_create_group(struct wacom *wacom,
 		return -ENOMEM;
 
 	devres->group = group;
-	devres->root = &wacom->hdev->dev.kobj;
+	devres->root = root;
 
 	error = sysfs_create_group(devres->root, group);
 	if (error)
@@ -958,6 +959,13 @@ static int wacom_devm_sysfs_create_group(struct wacom *wacom,
 	return 0;
 }
 
+static int wacom_devm_sysfs_create_group(struct wacom *wacom,
+					 struct attribute_group *group)
+{
+	return __wacom_devm_sysfs_create_group(wacom, &wacom->hdev->dev.kobj,
+					       group);
+}
+
 static void wacom_led_groups_release(void *data)
 {
 	struct wacom *wacom = data;
@@ -1306,9 +1314,10 @@ static int wacom_remote_create_attr_group(struct wacom *wacom, __u32 serial,
 	if (!remote->remote_group[index].name)
 		return -ENOMEM;
 
-	error = sysfs_create_group(remote->remote_dir,
-				   &remote->remote_group[index]);
+	error = __wacom_devm_sysfs_create_group(wacom, remote->remote_dir,
+						&remote->remote_group[index]);
 	if (error) {
+		remote->remote_group[index].name = NULL;
 		hid_err(wacom->hdev,
 			"cannot create sysfs group err: %d\n", error);
 		return error;
@@ -1317,15 +1326,6 @@ static int wacom_remote_create_attr_group(struct wacom *wacom, __u32 serial,
 	return 0;
 }
 
-static void wacom_remote_destroy_attr_group(struct wacom *wacom, unsigned int i)
-{
-	struct wacom_remote *remote = wacom->remote;
-
-	sysfs_remove_group(remote->remote_dir, &remote->remote_group[i]);
-	devm_kfree(&wacom->hdev->dev, (char *)remote->remote_group[i].name);
-	remote->remote_group[i].name = NULL;
-}
-
 static int wacom_cmd_unpair_remote(struct wacom *wacom, unsigned char selector)
 {
 	const size_t buf_size = 2;
@@ -1906,11 +1906,13 @@ static void wacom_remote_destroy_one(struct wacom *wacom, unsigned int index)
 	u32 serial = remote->serial[index];
 	int i;
 
-	wacom_remote_destroy_attr_group(wacom, index);
+	if (remote->remote_group[index].name)
+		devres_release_group(&wacom->hdev->dev, &remote->serial[index]);
 
 	for (i = 0; i < WACOM_MAX_REMOTES; i++) {
 		if (remote->serial[i] == serial) {
 			remote->serial[i] = 0;
+			remote->remote_group[i].name = NULL;
 			wacom->led.groups[i].select = WACOM_STATUS_UNKNOWN;
 		}
 	}
@@ -1920,6 +1922,7 @@ static int wacom_remote_create_one(struct wacom *wacom, u32 serial,
 				   unsigned int index)
 {
 	struct wacom_remote *remote = wacom->remote;
+	struct device *dev = &wacom->hdev->dev;
 	int error, k;
 
 	/* A remote can pair more than once with an EKR,
@@ -1935,13 +1938,22 @@ static int wacom_remote_create_one(struct wacom *wacom, u32 serial,
 		return 0;
 	}
 
+	if (!devres_open_group(dev, &remote->serial[index], GFP_KERNEL))
+		return -ENOMEM;
+
 	error = wacom_remote_create_attr_group(wacom, serial, index);
 	if (error)
-		return error;
+		goto fail;
 
 	remote->serial[index] = serial;
 
+	devres_close_group(dev, &remote->serial[index]);
 	return 0;
+
+fail:
+	devres_release_group(dev, &remote->serial[index]);
+	remote->serial[index] = 0;
+	return error;
 }
 
 static void wacom_remote_work(struct work_struct *work)
-- 
2.5.5

[toc] | [prev] | [next] | [standalone]

#1437089 — [PATCH 13/27] HID: wacom: leds: dynamically allocate LED groups

We need to add an action to ensure wacom->led.groups is null when
wacom_led_control() gets called after the resources has been freed.

This also prevents to send a LED command when there is no support
from the device.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/wacom.h     |  8 +++--
 drivers/hid/wacom_sys.c | 85 +++++++++++++++++++++++++++++++++++++++++--------
 drivers/hid/wacom_wac.c |  2 +-
 3 files changed, 78 insertions(+), 17 deletions(-)

diff --git a/drivers/hid/wacom.h b/drivers/hid/wacom.h
index bcfeb51..8ac1eb8 100644
--- a/drivers/hid/wacom.h
+++ b/drivers/hid/wacom.h
@@ -110,6 +110,10 @@ enum wacom_worker {
 	WACOM_WORKER_BATTERY,
 };
 
+struct wacom_group_leds {
+	u8 select; /* status led selector (0..3) */
+};
+
 struct wacom {
 	struct usb_device *usbdev;
 	struct usb_interface *intf;
@@ -118,8 +122,8 @@ struct wacom {
 	struct mutex lock;
 	struct work_struct wireless_work;
 	struct work_struct battery_work;
-	struct wacom_led {
-		u8 select[5]; /* status led selector (0..3) */
+	struct wacom_leds {
+		struct wacom_group_leds *groups;
 		u8 llv;       /* status led brightness no button (1..127) */
 		u8 hlv;       /* status led brightness button pressed (1..127) */
 		u8 img_lum;   /* OLED matrix display brightness */
diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
index d0f5764..4d4c737 100644
--- a/drivers/hid/wacom_sys.c
+++ b/drivers/hid/wacom_sys.c
@@ -647,6 +647,9 @@ static int wacom_led_control(struct wacom *wacom)
 	unsigned char report_id = WAC_CMD_LED_CONTROL;
 	int buf_size = 9;
 
+	if (!wacom->led.groups)
+		return -ENOTSUPP;
+
 	if (wacom->wacom_wac.pid) { /* wireless connected */
 		report_id = WAC_CMD_WL_LED_CONTROL;
 		buf_size = 13;
@@ -662,7 +665,7 @@ static int wacom_led_control(struct wacom *wacom)
 		 * one of four values:
 		 *    0 = Low; 1 = Medium; 2 = High; 3 = Off
 		 */
-		int ring_led = wacom->led.select[0] & 0x03;
+		int ring_led = wacom->led.groups[0].select & 0x03;
 		int ring_lum = (((wacom->led.llv & 0x60) >> 5) - 1) & 0x03;
 		int crop_lum = 0;
 		unsigned char led_bits = (crop_lum << 4) | (ring_lum << 2) | (ring_led);
@@ -677,11 +680,11 @@ static int wacom_led_control(struct wacom *wacom)
 			buf[1] = led_bits;
 	}
 	else {
-		int led = wacom->led.select[0] | 0x4;
+		int led = wacom->led.groups[0].select | 0x4;
 
 		if (wacom->wacom_wac.features.type == WACOM_21UX2 ||
 		    wacom->wacom_wac.features.type == WACOM_24HD)
-			led |= (wacom->led.select[1] << 4) | 0x40;
+			led |= (wacom->led.groups[1].select << 4) | 0x40;
 
 		buf[0] = report_id;
 		buf[1] = led;
@@ -753,7 +756,7 @@ static ssize_t wacom_led_select_store(struct device *dev, int set_id,
 
 	mutex_lock(&wacom->lock);
 
-	wacom->led.select[set_id] = id & 0x3;
+	wacom->led.groups[set_id].select = id & 0x3;
 	err = wacom_led_control(wacom);
 
 	mutex_unlock(&wacom->lock);
@@ -773,7 +776,7 @@ static ssize_t wacom_led##SET_ID##_select_show(struct device *dev,	\
 	struct hid_device *hdev = to_hid_device(dev);\
 	struct wacom *wacom = hid_get_drvdata(hdev);			\
 	return scnprintf(buf, PAGE_SIZE, "%d\n",			\
-			 wacom->led.select[SET_ID]);			\
+			 wacom->led.groups[SET_ID].select);		\
 }									\
 static DEVICE_ATTR(status_led##SET_ID##_select, DEV_ATTR_RW_PERM,	\
 		    wacom_led##SET_ID##_select_show,			\
@@ -955,6 +958,35 @@ static int wacom_devm_sysfs_create_group(struct wacom *wacom,
 	return 0;
 }
 
+static void wacom_led_groups_release(void *data)
+{
+	struct wacom *wacom = data;
+
+	wacom->led.groups = NULL;
+}
+
+static int wacom_led_groups_allocate(struct wacom *wacom, int count)
+{
+	struct wacom_group_leds *groups;
+	int error;
+
+	groups = devm_kzalloc(&wacom->hdev->dev,
+			      sizeof(struct wacom_group_leds) * count,
+			      GFP_KERNEL);
+	if (!groups)
+		return -ENOMEM;
+
+	error = devm_add_action_or_reset(&wacom->hdev->dev,
+					 wacom_led_groups_release,
+					 wacom);
+	if (error)
+		return error;
+
+	wacom->led.groups = groups;
+
+	return 0;
+}
+
 static int wacom_initialize_leds(struct wacom *wacom)
 {
 	int error;
@@ -968,23 +1000,34 @@ static int wacom_initialize_leds(struct wacom *wacom)
 	case INTUOS4:
 	case INTUOS4WL:
 	case INTUOS4L:
-		wacom->led.select[0] = 0;
-		wacom->led.select[1] = 0;
 		wacom->led.llv = 10;
 		wacom->led.hlv = 20;
 		wacom->led.img_lum = 10;
+
+		error = wacom_led_groups_allocate(wacom, 1);
+		if (error) {
+			hid_err(wacom->hdev,
+				"cannot create leds err: %d\n", error);
+			return error;
+		}
+
 		error = wacom_devm_sysfs_create_group(wacom,
 						      &intuos4_led_attr_group);
 		break;
 
 	case WACOM_24HD:
 	case WACOM_21UX2:
-		wacom->led.select[0] = 0;
-		wacom->led.select[1] = 0;
 		wacom->led.llv = 0;
 		wacom->led.hlv = 0;
 		wacom->led.img_lum = 0;
 
+		error = wacom_led_groups_allocate(wacom, 2);
+		if (error) {
+			hid_err(wacom->hdev,
+				"cannot create leds err: %d\n", error);
+			return error;
+		}
+
 		error = wacom_devm_sysfs_create_group(wacom,
 						      &cintiq_led_attr_group);
 		break;
@@ -995,16 +1038,30 @@ static int wacom_initialize_leds(struct wacom *wacom)
 	case INTUOSPS:
 	case INTUOSPM:
 	case INTUOSPL:
-		wacom->led.select[0] = 0;
-		wacom->led.select[1] = 0;
 		wacom->led.llv = 32;
 		wacom->led.hlv = 0;
 		wacom->led.img_lum = 0;
 
+		error = wacom_led_groups_allocate(wacom, 1);
+		if (error) {
+			hid_err(wacom->hdev,
+				"cannot create leds err: %d\n", error);
+			return error;
+		}
+
 		error = wacom_devm_sysfs_create_group(wacom,
 						      &intuos5_led_attr_group);
 		break;
 
+	case REMOTE:
+		error = wacom_led_groups_allocate(wacom, 5);
+		if (error) {
+			hid_err(wacom->hdev,
+				"cannot create leds err: %d\n", error);
+			return error;
+		}
+		return 0;
+
 	default:
 		return 0;
 	}
@@ -1204,7 +1261,7 @@ static ssize_t wacom_show_remote_mode(struct kobject *kobj,
 	struct wacom *wacom = hid_get_drvdata(hdev);
 	u8 mode;
 
-	mode = wacom->led.select[index];
+	mode = wacom->led.groups[index].select;
 	if (mode >= 0 && mode < 3)
 		return snprintf(buf, PAGE_SIZE, "%d\n", mode);
 	else
@@ -1272,7 +1329,7 @@ void wacom_remote_destroy_attr_group(struct wacom *wacom, __u32 serial)
 	for (i = 0; i < WACOM_MAX_REMOTES; i++) {
 		if (wacom_wac->serial[i] == serial) {
 			wacom_wac->serial[i] = 0;
-			wacom->led.select[i] = WACOM_STATUS_UNKNOWN;
+			wacom->led.groups[i].select = WACOM_STATUS_UNKNOWN;
 			if (wacom->remote_group[i].name) {
 				sysfs_remove_group(wacom->remote_dir,
 						   &wacom->remote_group[i]);
@@ -1369,7 +1426,7 @@ static int wacom_initialize_remote(struct wacom *wacom)
 	}
 
 	for (i = 0; i < WACOM_MAX_REMOTES; i++) {
-		wacom->led.select[i] = WACOM_STATUS_UNKNOWN;
+		wacom->led.groups[i].select = WACOM_STATUS_UNKNOWN;
 		wacom_wac->serial[i] = 0;
 	}
 
diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
index 3e2c903..932d3ee 100644
--- a/drivers/hid/wacom_wac.c
+++ b/drivers/hid/wacom_wac.c
@@ -808,7 +808,7 @@ static int wacom_remote_irq(struct wacom_wac *wacom_wac, size_t len)
 
 	for (i = 0; i < WACOM_MAX_REMOTES; i++) {
 		if (wacom_wac->serial[i] == serial)
-			wacom->led.select[i] = touch_ring_mode;
+			wacom->led.groups[i].select = touch_ring_mode;
 	}
 
 	if (!wacom->battery &&
-- 
2.5.5

[toc] | [prev] | [next] | [standalone]

#1437090 — [PATCH 15/27] HID: wacom: EKR: have the wacom resources dynamically allocated

If we want to have one input device per remote, it's better to have our
own struct wacom_remote which is dynamically allocated.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/wacom.h     |  13 +++--
 drivers/hid/wacom_sys.c | 133 ++++++++++++++++++++++++++++--------------------
 drivers/hid/wacom_wac.c |  10 ++--
 drivers/hid/wacom_wac.h |   2 +-
 4 files changed, 93 insertions(+), 65 deletions(-)

diff --git a/drivers/hid/wacom.h b/drivers/hid/wacom.h
index 1797e4b..110ea67 100644
--- a/drivers/hid/wacom.h
+++ b/drivers/hid/wacom.h
@@ -116,6 +116,14 @@ struct wacom_group_leds {
 	u8 select; /* status led selector (0..3) */
 };
 
+struct wacom_remote {
+	spinlock_t remote_lock;
+	struct kfifo remote_fifo;
+	struct kobject *remote_dir;
+	struct attribute_group remote_group[WACOM_MAX_REMOTES];
+	__u32 serial[WACOM_MAX_REMOTES];
+};
+
 struct wacom {
 	struct usb_device *usbdev;
 	struct usb_interface *intf;
@@ -125,8 +133,7 @@ struct wacom {
 	struct work_struct wireless_work;
 	struct work_struct battery_work;
 	struct work_struct remote_work;
-	spinlock_t remote_lock;
-	struct kfifo remote_fifo;
+	struct wacom_remote *remote;
 	struct wacom_leds {
 		struct wacom_group_leds *groups;
 		u8 llv;       /* status led brightness no button (1..127) */
@@ -137,8 +144,6 @@ struct wacom {
 	struct power_supply *ac;
 	struct power_supply_desc battery_desc;
 	struct power_supply_desc ac_desc;
-	struct kobject *remote_dir;
-	struct attribute_group remote_group[5];
 	bool resources;
 };
 
diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
index 7e01117..64acb71 100644
--- a/drivers/hid/wacom_sys.c
+++ b/drivers/hid/wacom_sys.c
@@ -1298,18 +1298,18 @@ static int wacom_remote_create_attr_group(struct wacom *wacom, __u32 serial,
 					  int index)
 {
 	int error = 0;
-	struct wacom_wac *wacom_wac = &wacom->wacom_wac;
+	struct wacom_remote *remote = wacom->remote;
 
-	wacom_wac->serial[index] = serial;
+	remote->serial[index] = serial;
 
-	wacom->remote_group[index].name = devm_kasprintf(&wacom->hdev->dev,
-							 GFP_KERNEL,
-							 "%d", serial);
-	if (!wacom->remote_group[index].name)
+	remote->remote_group[index].name = devm_kasprintf(&wacom->hdev->dev,
+							  GFP_KERNEL,
+							  "%d", serial);
+	if (!remote->remote_group[index].name)
 		return -ENOMEM;
 
-	error = sysfs_create_group(wacom->remote_dir,
-				   &wacom->remote_group[index]);
+	error = sysfs_create_group(remote->remote_dir,
+				   &remote->remote_group[index]);
 	if (error) {
 		hid_err(wacom->hdev,
 			"cannot create sysfs group err: %d\n", error);
@@ -1321,22 +1321,22 @@ static int wacom_remote_create_attr_group(struct wacom *wacom, __u32 serial,
 
 static void wacom_remote_destroy_attr_group(struct wacom *wacom, __u32 serial)
 {
-	struct wacom_wac *wacom_wac = &wacom->wacom_wac;
+	struct wacom_remote *remote = wacom->remote;
 	int i;
 
 	if (!serial)
 		return;
 
 	for (i = 0; i < WACOM_MAX_REMOTES; i++) {
-		if (wacom_wac->serial[i] == serial) {
-			wacom_wac->serial[i] = 0;
+		if (remote->serial[i] == serial) {
+			remote->serial[i] = 0;
 			wacom->led.groups[i].select = WACOM_STATUS_UNKNOWN;
-			if (wacom->remote_group[i].name) {
-				sysfs_remove_group(wacom->remote_dir,
-						   &wacom->remote_group[i]);
+			if (remote->remote_group[i].name) {
+				sysfs_remove_group(remote->remote_dir,
+						   &remote->remote_group[i]);
 				devm_kfree(&wacom->hdev->dev,
-					   (char *)wacom->remote_group[i].name);
-				wacom->remote_group[i].name = NULL;
+					  (char *)remote->remote_group[i].name);
+				remote->remote_group[i].name = NULL;
 			}
 		}
 	}
@@ -1398,27 +1398,57 @@ static const struct attribute *remote_unpair_attrs[] = {
 	NULL
 };
 
-static int wacom_initialize_remote(struct wacom *wacom)
+static void wacom_remotes_destroy(void *data)
+{
+	struct wacom *wacom = data;
+	struct wacom_remote *remote = wacom->remote;
+
+	if (!remote)
+		return;
+
+	kobject_put(remote->remote_dir);
+	kfifo_free(&remote->remote_fifo);
+	wacom->remote = NULL;
+}
+
+static int wacom_initialize_remotes(struct wacom *wacom)
 {
 	int error = 0;
-	struct wacom_wac *wacom_wac = &(wacom->wacom_wac);
+	struct wacom_remote *remote;
 	int i;
 
 	if (wacom->wacom_wac.features.type != REMOTE)
 		return 0;
 
-	wacom->remote_group[0] = remote0_serial_group;
-	wacom->remote_group[1] = remote1_serial_group;
-	wacom->remote_group[2] = remote2_serial_group;
-	wacom->remote_group[3] = remote3_serial_group;
-	wacom->remote_group[4] = remote4_serial_group;
+	remote = devm_kzalloc(&wacom->hdev->dev, sizeof(*wacom->remote),
+			      GFP_KERNEL);
+	if (!remote)
+		return -ENOMEM;
 
-	wacom->remote_dir = kobject_create_and_add("wacom_remote",
-						   &wacom->hdev->dev.kobj);
-	if (!wacom->remote_dir)
+	wacom->remote = remote;
+
+	spin_lock_init(&remote->remote_lock);
+
+	error = kfifo_alloc(&remote->remote_fifo,
+			5 * sizeof(struct wacom_remote_data),
+			GFP_KERNEL);
+	if (error) {
+		hid_err(wacom->hdev, "failed allocating remote_fifo\n");
 		return -ENOMEM;
+	}
 
-	error = sysfs_create_files(wacom->remote_dir, remote_unpair_attrs);
+	remote->remote_group[0] = remote0_serial_group;
+	remote->remote_group[1] = remote1_serial_group;
+	remote->remote_group[2] = remote2_serial_group;
+	remote->remote_group[3] = remote3_serial_group;
+	remote->remote_group[4] = remote4_serial_group;
+
+	remote->remote_dir = kobject_create_and_add("wacom_remote",
+						    &wacom->hdev->dev.kobj);
+	if (!remote->remote_dir)
+		return -ENOMEM;
+
+	error = sysfs_create_files(remote->remote_dir, remote_unpair_attrs);
 
 	if (error) {
 		hid_err(wacom->hdev,
@@ -1428,9 +1458,14 @@ static int wacom_initialize_remote(struct wacom *wacom)
 
 	for (i = 0; i < WACOM_MAX_REMOTES; i++) {
 		wacom->led.groups[i].select = WACOM_STATUS_UNKNOWN;
-		wacom_wac->serial[i] = 0;
+		remote->serial[i] = 0;
 	}
 
+	error = devm_add_action_or_reset(&wacom->hdev->dev,
+					 wacom_remotes_destroy, wacom);
+	if (error)
+		return error;
+
 	return 0;
 }
 
@@ -1740,7 +1775,7 @@ static int wacom_parse_and_register(struct wacom *wacom, bool wireless)
 		if (error)
 			goto fail_leds;
 
-		error = wacom_initialize_remote(wacom);
+		error = wacom_initialize_remotes(wacom);
 		if (error)
 			goto fail_remote;
 	}
@@ -1792,7 +1827,6 @@ static int wacom_parse_and_register(struct wacom *wacom, bool wireless)
 fail_quirks:
 	hid_hw_stop(hdev);
 fail_hw_start:
-	kobject_put(wacom->remote_dir);
 fail_remote:
 fail_leds:
 fail_register_inputs:
@@ -1893,58 +1927,58 @@ fail:
 static void wacom_remote_work(struct work_struct *work)
 {
 	struct wacom *wacom = container_of(work, struct wacom, remote_work);
-	struct wacom_wac *wacom_wac = &wacom->wacom_wac;
+	struct wacom_remote *remote = wacom->remote;
 	struct wacom_remote_data data;
 	unsigned long flags;
 	unsigned int count;
 	u32 serial;
 	int i, k;
 
-	spin_lock_irqsave(&wacom->remote_lock, flags);
+	spin_lock_irqsave(&remote->remote_lock, flags);
 
-	count = kfifo_out(&wacom->remote_fifo, &data, sizeof(data));
+	count = kfifo_out(&remote->remote_fifo, &data, sizeof(data));
 
 	if (count != sizeof(data)) {
 		hid_err(wacom->hdev,
 			"workitem triggered without status available\n");
-		spin_unlock_irqrestore(&wacom->remote_lock, flags);
+		spin_unlock_irqrestore(&remote->remote_lock, flags);
 		return;
 	}
 
-	if (!kfifo_is_empty(&wacom->remote_fifo))
+	if (!kfifo_is_empty(&remote->remote_fifo))
 		wacom_schedule_work(&wacom->wacom_wac, WACOM_WORKER_REMOTE);
 
-	spin_unlock_irqrestore(&wacom->remote_lock, flags);
+	spin_unlock_irqrestore(&remote->remote_lock, flags);
 
 	for (i = 0; i < WACOM_MAX_REMOTES; i++) {
 		serial = data.remote[i].serial;
 		if (data.remote[i].connected) {
 
-			if (wacom_wac->serial[i] == serial)
+			if (remote->serial[i] == serial)
 				continue;
 
-			if (wacom_wac->serial[i]) {
+			if (remote->serial[i]) {
 				wacom_remote_destroy_attr_group(wacom,
-							wacom_wac->serial[i]);
+							remote->serial[i]);
 			}
 
 			/* A remote can pair more than once with an EKR,
 			 * check to make sure this serial isn't already paired.
 			 */
 			for (k = 0; k < WACOM_MAX_REMOTES; k++) {
-				if (wacom_wac->serial[k] == serial)
+				if (remote->serial[k] == serial)
 					break;
 			}
 
 			if (k < WACOM_MAX_REMOTES) {
-				wacom_wac->serial[i] = serial;
+				remote->serial[i] = serial;
 				continue;
 			}
 			wacom_remote_create_attr_group(wacom, serial, i);
 
-		} else if (wacom_wac->serial[i]) {
+		} else if (remote->serial[i]) {
 			wacom_remote_destroy_attr_group(wacom,
-							wacom_wac->serial[i]);
+							remote->serial[i]);
 		}
 	}
 }
@@ -1992,16 +2026,6 @@ static int wacom_probe(struct hid_device *hdev,
 	INIT_WORK(&wacom->wireless_work, wacom_wireless_work);
 	INIT_WORK(&wacom->battery_work, wacom_battery_work);
 	INIT_WORK(&wacom->remote_work, wacom_remote_work);
-	spin_lock_init(&wacom->remote_lock);
-
-	if (kfifo_alloc(&wacom->remote_fifo,
-			5 * sizeof(struct wacom_remote_data),
-			GFP_KERNEL)) {
-		dev_err(&hdev->dev,
-			"%s:failed allocating remote_fifo\n", __func__);
-		error = -ENOMEM;
-		goto fail_type;
-	}
 
 	/* ask for the report descriptor to be loaded by HID */
 	error = hid_parse(hdev);
@@ -2025,7 +2049,6 @@ static int wacom_probe(struct hid_device *hdev,
 	return 0;
 
 fail_parse:
-	kfifo_free(&wacom->remote_fifo);
 fail_type:
 	hid_set_drvdata(hdev, NULL);
 	return error;
@@ -2045,8 +2068,6 @@ static void wacom_remove(struct hid_device *hdev)
 	cancel_work_sync(&wacom->wireless_work);
 	cancel_work_sync(&wacom->battery_work);
 	cancel_work_sync(&wacom->remote_work);
-	kfifo_free(&wacom->remote_fifo);
-	kobject_put(wacom->remote_dir);
 	if (hdev->bus == BUS_BLUETOOTH)
 		device_remove_file(&hdev->dev, &dev_attr_speed);
 
diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
index 4f4a2b9..d5c180d 100644
--- a/drivers/hid/wacom_wac.c
+++ b/drivers/hid/wacom_wac.c
@@ -753,6 +753,7 @@ static int wacom_remote_irq(struct wacom_wac *wacom_wac, size_t len)
 	unsigned char *data = wacom_wac->data;
 	struct input_dev *input = wacom_wac->pad_input;
 	struct wacom *wacom = container_of(wacom_wac, struct wacom, wacom_wac);
+	struct wacom_remote *remote = wacom->remote;
 	struct wacom_features *features = &wacom_wac->features;
 	int bat_charging, bat_percent, touch_ring_mode;
 	__u32 serial;
@@ -807,7 +808,7 @@ static int wacom_remote_irq(struct wacom_wac *wacom_wac, size_t len)
 	touch_ring_mode = (data[11] & 0xC0) >> 6;
 
 	for (i = 0; i < WACOM_MAX_REMOTES; i++) {
-		if (wacom_wac->serial[i] == serial)
+		if (remote->serial[i] == serial)
 			wacom->led.groups[i].select = touch_ring_mode;
 	}
 
@@ -827,6 +828,7 @@ static int wacom_remote_status_irq(struct wacom_wac *wacom_wac, size_t len)
 {
 	struct wacom *wacom = container_of(wacom_wac, struct wacom, wacom_wac);
 	unsigned char *data = wacom_wac->data;
+	struct wacom_remote *remote = wacom->remote;
 	struct wacom_remote_data remote_data = {0};
 	unsigned long flags;
 	int i, ret;
@@ -843,15 +845,15 @@ static int wacom_remote_status_irq(struct wacom_wac *wacom_wac, size_t len)
 		remote_data.remote[i].connected = connected;
 	}
 
-	spin_lock_irqsave(&wacom->remote_lock, flags);
+	spin_lock_irqsave(&remote->remote_lock, flags);
 
-	ret = kfifo_in(&wacom->remote_fifo, &remote_data, sizeof(remote_data));
+	ret = kfifo_in(&remote->remote_fifo, &remote_data, sizeof(remote_data));
 	if (ret != sizeof(remote_data)) {
 		hid_err(wacom->hdev, "Can't queue Remote status event.\n");
 		return 0;
 	}
 
-	spin_unlock_irqrestore(&wacom->remote_lock, flags);
+	spin_unlock_irqrestore(&remote->remote_lock, flags);
 
 	wacom_schedule_work(wacom_wac, WACOM_WORKER_REMOTE);
 
diff --git a/drivers/hid/wacom_wac.h b/drivers/hid/wacom_wac.h
index c2c65b8..6be6cae 100644
--- a/drivers/hid/wacom_wac.h
+++ b/drivers/hid/wacom_wac.h
@@ -234,7 +234,7 @@ struct wacom_wac {
 	unsigned char data[WACOM_PKGLEN_MAX];
 	int tool[2];
 	int id[2];
-	__u32 serial[5];
+	__u32 serial[2];
 	bool reporting_data;
 	struct wacom_features features;
 	struct wacom_shared *shared;
-- 
2.5.5

[toc] | [prev] | [next] | [standalone]

#1437091 — [PATCH 14/27] HID: wacom: EKR: add a worker to add/remove resources on addition/removal

wacom_remote_status_irq() sends information of addition/removal of EKR.
We want to allocate one input node per remote, so better having this
in a separate worker, not handled in the IRQ directly.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/wacom.h     | 11 +++++--
 drivers/hid/wacom_sys.c | 80 +++++++++++++++++++++++++++++++++++++++++++++++--
 drivers/hid/wacom_wac.c | 41 +++++++++----------------
 drivers/hid/wacom_wac.h |  7 +++++
 4 files changed, 106 insertions(+), 33 deletions(-)

diff --git a/drivers/hid/wacom.h b/drivers/hid/wacom.h
index 8ac1eb8..1797e4b 100644
--- a/drivers/hid/wacom.h
+++ b/drivers/hid/wacom.h
@@ -90,6 +90,7 @@
 #include <linux/module.h>
 #include <linux/mod_devicetable.h>
 #include <linux/hid.h>
+#include <linux/kfifo.h>
 #include <linux/usb/input.h>
 #include <linux/power_supply.h>
 #include <asm/unaligned.h>
@@ -108,6 +109,7 @@
 enum wacom_worker {
 	WACOM_WORKER_WIRELESS,
 	WACOM_WORKER_BATTERY,
+	WACOM_WORKER_REMOTE,
 };
 
 struct wacom_group_leds {
@@ -122,6 +124,9 @@ struct wacom {
 	struct mutex lock;
 	struct work_struct wireless_work;
 	struct work_struct battery_work;
+	struct work_struct remote_work;
+	spinlock_t remote_lock;
+	struct kfifo remote_fifo;
 	struct wacom_leds {
 		struct wacom_group_leds *groups;
 		u8 llv;       /* status led brightness no button (1..127) */
@@ -149,6 +154,9 @@ static inline void wacom_schedule_work(struct wacom_wac *wacom_wac,
 	case WACOM_WORKER_BATTERY:
 		schedule_work(&wacom->battery_work);
 		break;
+	case WACOM_WORKER_REMOTE:
+		schedule_work(&wacom->remote_work);
+		break;
 	}
 }
 
@@ -168,7 +176,4 @@ int wacom_wac_event(struct hid_device *hdev, struct hid_field *field,
 		struct hid_usage *usage, __s32 value);
 void wacom_wac_report(struct hid_device *hdev, struct hid_report *report);
 void wacom_battery_work(struct work_struct *work);
-int wacom_remote_create_attr_group(struct wacom *wacom, __u32 serial,
-				   int index);
-void wacom_remote_destroy_attr_group(struct wacom *wacom, __u32 serial);
 #endif
diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
index 4d4c737..7e01117 100644
--- a/drivers/hid/wacom_sys.c
+++ b/drivers/hid/wacom_sys.c
@@ -1294,7 +1294,8 @@ DEVICE_EKR_ATTR_GROUP(2);
 DEVICE_EKR_ATTR_GROUP(3);
 DEVICE_EKR_ATTR_GROUP(4);
 
-int wacom_remote_create_attr_group(struct wacom *wacom, __u32 serial, int index)
+static int wacom_remote_create_attr_group(struct wacom *wacom, __u32 serial,
+					  int index)
 {
 	int error = 0;
 	struct wacom_wac *wacom_wac = &wacom->wacom_wac;
@@ -1318,7 +1319,7 @@ int wacom_remote_create_attr_group(struct wacom *wacom, __u32 serial, int index)
 	return 0;
 }
 
-void wacom_remote_destroy_attr_group(struct wacom *wacom, __u32 serial)
+static void wacom_remote_destroy_attr_group(struct wacom *wacom, __u32 serial)
 {
 	struct wacom_wac *wacom_wac = &wacom->wacom_wac;
 	int i;
@@ -1889,6 +1890,65 @@ fail:
 	return;
 }
 
+static void wacom_remote_work(struct work_struct *work)
+{
+	struct wacom *wacom = container_of(work, struct wacom, remote_work);
+	struct wacom_wac *wacom_wac = &wacom->wacom_wac;
+	struct wacom_remote_data data;
+	unsigned long flags;
+	unsigned int count;
+	u32 serial;
+	int i, k;
+
+	spin_lock_irqsave(&wacom->remote_lock, flags);
+
+	count = kfifo_out(&wacom->remote_fifo, &data, sizeof(data));
+
+	if (count != sizeof(data)) {
+		hid_err(wacom->hdev,
+			"workitem triggered without status available\n");
+		spin_unlock_irqrestore(&wacom->remote_lock, flags);
+		return;
+	}
+
+	if (!kfifo_is_empty(&wacom->remote_fifo))
+		wacom_schedule_work(&wacom->wacom_wac, WACOM_WORKER_REMOTE);
+
+	spin_unlock_irqrestore(&wacom->remote_lock, flags);
+
+	for (i = 0; i < WACOM_MAX_REMOTES; i++) {
+		serial = data.remote[i].serial;
+		if (data.remote[i].connected) {
+
+			if (wacom_wac->serial[i] == serial)
+				continue;
+
+			if (wacom_wac->serial[i]) {
+				wacom_remote_destroy_attr_group(wacom,
+							wacom_wac->serial[i]);
+			}
+
+			/* A remote can pair more than once with an EKR,
+			 * check to make sure this serial isn't already paired.
+			 */
+			for (k = 0; k < WACOM_MAX_REMOTES; k++) {
+				if (wacom_wac->serial[k] == serial)
+					break;
+			}
+
+			if (k < WACOM_MAX_REMOTES) {
+				wacom_wac->serial[i] = serial;
+				continue;
+			}
+			wacom_remote_create_attr_group(wacom, serial, i);
+
+		} else if (wacom_wac->serial[i]) {
+			wacom_remote_destroy_attr_group(wacom,
+							wacom_wac->serial[i]);
+		}
+	}
+}
+
 static int wacom_probe(struct hid_device *hdev,
 		const struct hid_device_id *id)
 {
@@ -1931,6 +1991,17 @@ static int wacom_probe(struct hid_device *hdev,
 	mutex_init(&wacom->lock);
 	INIT_WORK(&wacom->wireless_work, wacom_wireless_work);
 	INIT_WORK(&wacom->battery_work, wacom_battery_work);
+	INIT_WORK(&wacom->remote_work, wacom_remote_work);
+	spin_lock_init(&wacom->remote_lock);
+
+	if (kfifo_alloc(&wacom->remote_fifo,
+			5 * sizeof(struct wacom_remote_data),
+			GFP_KERNEL)) {
+		dev_err(&hdev->dev,
+			"%s:failed allocating remote_fifo\n", __func__);
+		error = -ENOMEM;
+		goto fail_type;
+	}
 
 	/* ask for the report descriptor to be loaded by HID */
 	error = hid_parse(hdev);
@@ -1953,8 +2024,9 @@ static int wacom_probe(struct hid_device *hdev,
 
 	return 0;
 
-fail_type:
 fail_parse:
+	kfifo_free(&wacom->remote_fifo);
+fail_type:
 	hid_set_drvdata(hdev, NULL);
 	return error;
 }
@@ -1972,6 +2044,8 @@ static void wacom_remove(struct hid_device *hdev)
 
 	cancel_work_sync(&wacom->wireless_work);
 	cancel_work_sync(&wacom->battery_work);
+	cancel_work_sync(&wacom->remote_work);
+	kfifo_free(&wacom->remote_fifo);
 	kobject_put(wacom->remote_dir);
 	if (hdev->bus == BUS_BLUETOOTH)
 		device_remove_file(&hdev->dev, &dev_attr_speed);
diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
index 932d3ee..4f4a2b9 100644
--- a/drivers/hid/wacom_wac.c
+++ b/drivers/hid/wacom_wac.c
@@ -827,7 +827,9 @@ static int wacom_remote_status_irq(struct wacom_wac *wacom_wac, size_t len)
 {
 	struct wacom *wacom = container_of(wacom_wac, struct wacom, wacom_wac);
 	unsigned char *data = wacom_wac->data;
-	int i;
+	struct wacom_remote_data remote_data = {0};
+	unsigned long flags;
+	int i, ret;
 
 	if (data[0] != WACOM_REPORT_DEVICE_LIST)
 		return 0;
@@ -837,36 +839,21 @@ static int wacom_remote_status_irq(struct wacom_wac *wacom_wac, size_t len)
 		int serial = (data[j+6] << 16) + (data[j+5] << 8) + data[j+4];
 		bool connected = data[j+2];
 
-		if (connected) {
-			int k;
-
-			if (wacom_wac->serial[i] == serial)
-				continue;
+		remote_data.remote[i].serial = serial;
+		remote_data.remote[i].connected = connected;
+	}
 
-			if (wacom_wac->serial[i]) {
-				wacom_remote_destroy_attr_group(wacom,
-							wacom_wac->serial[i]);
-			}
+	spin_lock_irqsave(&wacom->remote_lock, flags);
 
-			/* A remote can pair more than once with an EKR,
-			 * check to make sure this serial isn't already paired.
-			 */
-			for (k = 0; k < WACOM_MAX_REMOTES; k++) {
-				if (wacom_wac->serial[k] == serial)
-					break;
-			}
+	ret = kfifo_in(&wacom->remote_fifo, &remote_data, sizeof(remote_data));
+	if (ret != sizeof(remote_data)) {
+		hid_err(wacom->hdev, "Can't queue Remote status event.\n");
+		return 0;
+	}
 
-			if (k < WACOM_MAX_REMOTES) {
-				wacom_wac->serial[i] = serial;
-				continue;
-			}
-			wacom_remote_create_attr_group(wacom, serial, i);
+	spin_unlock_irqrestore(&wacom->remote_lock, flags);
 
-		} else if (wacom_wac->serial[i]) {
-			wacom_remote_destroy_attr_group(wacom,
-							wacom_wac->serial[i]);
-		}
-	}
+	wacom_schedule_work(wacom_wac, WACOM_WORKER_REMOTE);
 
 	return 0;
 }
diff --git a/drivers/hid/wacom_wac.h b/drivers/hid/wacom_wac.h
index d5b973d..c2c65b8 100644
--- a/drivers/hid/wacom_wac.h
+++ b/drivers/hid/wacom_wac.h
@@ -218,6 +218,13 @@ struct hid_data {
 	int num_received;
 };
 
+struct wacom_remote_data {
+	struct {
+		u32 serial;
+		bool connected;
+	} remote[WACOM_MAX_REMOTES];
+};
+
 struct wacom_wac {
 	char pen_name[WACOM_NAME_MAX];
 	char touch_name[WACOM_NAME_MAX];
-- 
2.5.5

[toc] | [prev] | [next] | [standalone]

#1437092 — [PATCH 24/27] HID: wacom: leds: actually release the LEDs on disconnect

There is a bug (?) in devm_led_classdev_register() in which its increments
the refcount of the parent. If the parent is an input device, that means
the ref count never reaches 0 when devm_input_device_release() gets called.
This means that the LEDs and all the devres resources attached to the
input device are not released.

Manually force the release of the group so that the leds are released once
we are done using them.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/wacom.h     |  1 +
 drivers/hid/wacom_sys.c | 27 ++++++++++++++++++++++++++-
 2 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/wacom.h b/drivers/hid/wacom.h
index 0c498c4..9159dd3 100644
--- a/drivers/hid/wacom.h
+++ b/drivers/hid/wacom.h
@@ -130,6 +130,7 @@ struct wacom_group_leds {
 	u8 select; /* status led selector (0..3) */
 	struct wacom_led *leds;
 	unsigned int count;
+	struct device *dev;
 };
 
 struct wacom_battery {
diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
index 2354d73..b88896c 100644
--- a/drivers/hid/wacom_sys.c
+++ b/drivers/hid/wacom_sys.c
@@ -1064,6 +1064,13 @@ static int wacom_led_register_one(struct device *dev, struct wacom *wacom,
 	return 0;
 }
 
+static void wacom_led_groups_release_one(void *data)
+{
+	struct wacom_group_leds *group = data;
+
+	devres_release_group(group->dev, group);
+}
+
 static int wacom_led_groups_alloc_and_register_one(struct device *dev,
 						   struct wacom *wacom,
 						   int group_id, int count,
@@ -1094,7 +1101,25 @@ static int wacom_led_groups_alloc_and_register_one(struct device *dev,
 			goto err;
 	}
 
-	devres_remove_group(dev, &wacom->led.groups[group_id]);
+	wacom->led.groups[group_id].dev = dev;
+
+	devres_close_group(dev, &wacom->led.groups[group_id]);
+
+	/*
+	 * There is a bug (?) in devm_led_classdev_register() in which its
+	 * increments the refcount of the parent. If the parent is an input
+	 * device, that means the ref count never reaches 0 when
+	 * devm_input_device_release() gets called.
+	 * This means that the LEDs are still there after disconnect.
+	 * Manually force the release of the group so that the leds are released
+	 * once we are done using them.
+	 */
+	error = devm_add_action_or_reset(&wacom->hdev->dev,
+					 wacom_led_groups_release_one,
+					 &wacom->led.groups[group_id]);
+	if (error)
+		return error;
+
 	return 0;
 
 err:
-- 
2.5.5

[toc] | [prev] | [next] | [standalone]

#1437093 — [PATCH 05/27] HID: wacom: use one work queue per task

Looks like the battery hijacked the wireless worker. That's not fair so
use a work queue per task.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/wacom.h     | 21 ++++++++++++++++++---
 drivers/hid/wacom_sys.c | 10 ++++++----
 drivers/hid/wacom_wac.c | 13 +++++--------
 3 files changed, 29 insertions(+), 15 deletions(-)

diff --git a/drivers/hid/wacom.h b/drivers/hid/wacom.h
index 4681a65..a968fbb 100644
--- a/drivers/hid/wacom.h
+++ b/drivers/hid/wacom.h
@@ -105,13 +105,19 @@
 #define USB_VENDOR_ID_WACOM	0x056a
 #define USB_VENDOR_ID_LENOVO	0x17ef
 
+enum wacom_worker {
+	WACOM_WORKER_WIRELESS,
+	WACOM_WORKER_BATTERY,
+};
+
 struct wacom {
 	struct usb_device *usbdev;
 	struct usb_interface *intf;
 	struct wacom_wac wacom_wac;
 	struct hid_device *hdev;
 	struct mutex lock;
-	struct work_struct work;
+	struct work_struct wireless_work;
+	struct work_struct battery_work;
 	struct wacom_led {
 		u8 select[5]; /* status led selector (0..3) */
 		u8 llv;       /* status led brightness no button (1..127) */
@@ -127,10 +133,19 @@ struct wacom {
 	struct attribute_group remote_group[5];
 };
 
-static inline void wacom_schedule_work(struct wacom_wac *wacom_wac)
+static inline void wacom_schedule_work(struct wacom_wac *wacom_wac,
+				       enum wacom_worker which)
 {
 	struct wacom *wacom = container_of(wacom_wac, struct wacom, wacom_wac);
-	schedule_work(&wacom->work);
+
+	switch (which) {
+	case WACOM_WORKER_WIRELESS:
+		schedule_work(&wacom->wireless_work);
+		break;
+	case WACOM_WORKER_BATTERY:
+		schedule_work(&wacom->battery_work);
+		break;
+	}
 }
 
 extern const struct hid_device_id wacom_ids[];
diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
index af2801d..54f0260 100644
--- a/drivers/hid/wacom_sys.c
+++ b/drivers/hid/wacom_sys.c
@@ -1527,7 +1527,7 @@ static void wacom_calculate_res(struct wacom_features *features)
 
 void wacom_battery_work(struct work_struct *work)
 {
-	struct wacom *wacom = container_of(work, struct wacom, work);
+	struct wacom *wacom = container_of(work, struct wacom, battery_work);
 
 	if ((wacom->wacom_wac.features.quirks & WACOM_QUIRK_BATTERY) &&
 	     !wacom->battery) {
@@ -1743,7 +1743,7 @@ fail_allocate_inputs:
 
 static void wacom_wireless_work(struct work_struct *work)
 {
-	struct wacom *wacom = container_of(work, struct wacom, work);
+	struct wacom *wacom = container_of(work, struct wacom, wireless_work);
 	struct usb_device *usbdev = wacom->usbdev;
 	struct wacom_wac *wacom_wac = &wacom->wacom_wac;
 	struct hid_device *hdev1, *hdev2;
@@ -1871,7 +1871,8 @@ static int wacom_probe(struct hid_device *hdev,
 	wacom->usbdev = dev;
 	wacom->intf = intf;
 	mutex_init(&wacom->lock);
-	INIT_WORK(&wacom->work, wacom_wireless_work);
+	INIT_WORK(&wacom->wireless_work, wacom_wireless_work);
+	INIT_WORK(&wacom->battery_work, wacom_battery_work);
 
 	/* ask for the report descriptor to be loaded by HID */
 	error = hid_parse(hdev);
@@ -1912,7 +1913,8 @@ static void wacom_remove(struct hid_device *hdev)
 
 	hid_hw_stop(hdev);
 
-	cancel_work_sync(&wacom->work);
+	cancel_work_sync(&wacom->wireless_work);
+	cancel_work_sync(&wacom->battery_work);
 	kobject_put(wacom->remote_dir);
 	wacom_destroy_leds(wacom);
 	wacom_clean_inputs(wacom);
diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
index d6746ca..c7707b2 100644
--- a/drivers/hid/wacom_wac.c
+++ b/drivers/hid/wacom_wac.c
@@ -814,8 +814,7 @@ static int wacom_remote_irq(struct wacom_wac *wacom_wac, size_t len)
 	if (!wacom->battery &&
 	    !(features->quirks & WACOM_QUIRK_BATTERY)) {
 		features->quirks |= WACOM_QUIRK_BATTERY;
-		INIT_WORK(&wacom->work, wacom_battery_work);
-		wacom_schedule_work(wacom_wac);
+		wacom_schedule_work(wacom_wac, WACOM_WORKER_BATTERY);
 	}
 
 	wacom_notify_battery(wacom_wac, bat_percent, bat_charging, 1,
@@ -2150,7 +2149,7 @@ static int wacom_wireless_irq(struct wacom_wac *wacom, size_t len)
 		charging = !!(data[5] & 0x80);
 		if (wacom->pid != pid) {
 			wacom->pid = pid;
-			wacom_schedule_work(wacom);
+			wacom_schedule_work(wacom, WACOM_WORKER_WIRELESS);
 		}
 
 		if (w->battery)
@@ -2159,7 +2158,7 @@ static int wacom_wireless_irq(struct wacom_wac *wacom, size_t len)
 	} else if (wacom->pid != 0) {
 		/* disconnected while previously connected */
 		wacom->pid = 0;
-		wacom_schedule_work(wacom);
+		wacom_schedule_work(wacom, WACOM_WORKER_WIRELESS);
 		wacom_notify_battery(wacom, 0, 0, 0, 0);
 	}
 
@@ -2194,15 +2193,13 @@ static int wacom_status_irq(struct wacom_wac *wacom_wac, size_t len)
 		if (!wacom->battery &&
 		    !(features->quirks & WACOM_QUIRK_BATTERY)) {
 			features->quirks |= WACOM_QUIRK_BATTERY;
-			INIT_WORK(&wacom->work, wacom_battery_work);
-			wacom_schedule_work(wacom_wac);
+			wacom_schedule_work(wacom_wac, WACOM_WORKER_BATTERY);
 		}
 	}
 	else if ((features->quirks & WACOM_QUIRK_BATTERY) &&
 		 wacom->battery) {
 		features->quirks &= ~WACOM_QUIRK_BATTERY;
-		INIT_WORK(&wacom->work, wacom_battery_work);
-		wacom_schedule_work(wacom_wac);
+		wacom_schedule_work(wacom_wac, WACOM_WORKER_BATTERY);
 		wacom_notify_battery(wacom_wac, 0, 0, 0, 0);
 	}
 	return 0;
-- 
2.5.5

[toc] | [prev] | [standalone]

Page 2 of 2 — ← Prev page 1 [2]

Back to top | Article view | linux.kernel

csiph-web