Re: [PATCH] HID: roccat: Remove use of the "exist" field
flat view
From: Benjamin Tissoires <hidden>
Date: 2016-08-29 10:15:21
Also in:
lkml
Hi Silvan, On Aug 21 2016 or thereabouts, Silvan Jegen wrote:
The "exist" field is only checked when "roccat_open" has already been called or when we have made sure that the corresponding roccat_device is not NULL. Since the value of the "open" field has been increased by the "roccat_open" call, instead of checking "exist" we can just check if "open" is equal to zero to the same effect and remove the "exist" field as well as the code that touches it. Signed-off-by: Silvan Jegen <redacted>
Well, if you look at the history, since v4.4 this driver is deprecated (see https://patchwork.kernel.org/patch/7422131/), so I am not so sure you should put a lot of effort in cleaning this up.
quoted hunk ↗ jump to hunk
--- I have tested this patch with the only Roccat hardware I own, a Roccat Kone Pure. Testing the patch with several pieces of Roccat hardware connected at the same time would be desirable. drivers/hid/hid-roccat.c | 20 +++++++------------- 1 file changed, 7 insertions(+), 13 deletions(-)diff --git a/drivers/hid/hid-roccat.c b/drivers/hid/hid-roccat.c index 76d06cf..7552a1e 100644 --- a/drivers/hid/hid-roccat.c +++ b/drivers/hid/hid-roccat.c@@ -43,7 +43,6 @@ struct roccat_device { unsigned int minor; int report_size; int open; - int exist; wait_queue_head_t wait; struct device *dev; struct hid_device *hid;@@ -99,7 +98,7 @@ static ssize_t roccat_read(struct file *file, char __user *buffer, retval = -ERESTARTSYS; break; } - if (!device->exist) { + if (device->open == 0) {
It feels weird to check for the device we are currently reading to be opened (by the caller?). I think this changes a little bit the way the flag was designed for. This flag is controlled when the physical HW is removed/added. But when the physical HW is removed, you might have some readers to the special chardev. And so it needs to have a way to stop the readers that are waiting for incoming data (read or poll). Stefan might have a deeper look and ACK/NACK it, but to me, the patch looks wrong :/ Cheers, Benjamin
quoted hunk ↗ jump to hunk
retval = -EIO; break; }@@ -143,7 +142,7 @@ static unsigned int roccat_poll(struct file *file, poll_table *wait) poll_wait(file, &reader->device->wait, wait); if (reader->cbuf_start != reader->device->cbuf_end) return POLLIN | POLLRDNORM; - if (!reader->device->exist) + if (reader->device->open == 0) return POLLERR | POLLHUP; return 0; }@@ -224,13 +223,11 @@ static int roccat_release(struct inode *inode, struct file *file) kfree(reader); if (!--device->open) { - /* removing last reader */ - if (device->exist) { - hid_hw_power(device->hid, PM_HINT_NORMAL); - hid_hw_close(device->hid); - } else { - kfree(device); - } + /* we have removed the last reader */ + kfree(device); + } else { + hid_hw_power(device->hid, PM_HINT_NORMAL); + hid_hw_close(device->hid); } mutex_unlock(&devices_lock);@@ -340,7 +337,6 @@ int roccat_connect(struct class *klass, struct hid_device *hid, int report_size) mutex_init(&device->cbuf_lock); device->minor = minor; device->hid = hid; - device->exist = 1; device->cbuf_end = 0; device->report_size = report_size;@@ -359,8 +355,6 @@ void roccat_disconnect(int minor) device = devices[minor]; mutex_unlock(&devices_lock); - device->exist = 0; /* TODO exist maybe not needed */ - device_destroy(device->dev->class, MKDEV(roccat_major, minor)); mutex_lock(&devices_lock);-- 2.9.3