Q: weird hidraw behaviour

Hi,

I noticed that after commit 212a871a393 (HID: hidraw: correctly deallocate memory on device disconnect) hidraw doesn't close the underlying hid device when the device node is closed last time.

For example I have a touch panel (HID over I2C) device with added debug prints in i2c_hid_open()/i2c_hid_close():

	# od -x /dev/hidraw0
	[   41.363813] i2c_hid 1-004c: i2c_hid_power lvl:32
	[   41.368464] i2c_hid 1-004c: i2c_hid_set_power
	[   41.372831] i2c_hid 1-004c: __i2c_hid_command: cmd=54 01 00 08
	[   41.451455] i2c_hid 1-004c: i2c_hid_open
	^C

	# od -x /dev/hidraw0
	[   58.420928] i2c_hid 1-004c: i2c_hid_power lvl:32
	[   58.425577] i2c_hid 1-004c: i2c_hid_set_power
	[   58.429945] i2c_hid 1-004c: __i2c_hid_command: cmd=54 01 00 08
	[   58.525276] i2c_hid 1-004c: i2c_hid_open
	^C

i2c_hid_close() is never called. Is this intended or am I missing something?

Thanks.

Hi Mika,

Can you please try the attached patch and see if it fixes this issue?

Thanks
-Manoj

-----Original Message-----
From: Mika Westerberg [mailto:[email protected]]
Sent: Tuesday, September 24, 2013 2:26 PM
To: Jiri Kosina; Manoj Chourasia
Cc: [email protected]
Subject: Q: weird hidraw behaviour

Hi,

I noticed that after commit 212a871a393 (HID: hidraw: correctly deallocate memory on device disconnect) hidraw doesn't close the underlying hid device when the device node is closed last time.

For example I have a touch panel (HID over I2C) device with added debug prints in i2c_hid_open()/i2c_hid_close():

	# od -x /dev/hidraw0
	[   41.363813] i2c_hid 1-004c: i2c_hid_power lvl:32
	[   41.368464] i2c_hid 1-004c: i2c_hid_set_power
	[   41.372831] i2c_hid 1-004c: __i2c_hid_command: cmd=54 01 00 08
	[   41.451455] i2c_hid 1-004c: i2c_hid_open
	^C

	# od -x /dev/hidraw0
	[   58.420928] i2c_hid 1-004c: i2c_hid_power lvl:32
	[   58.425577] i2c_hid 1-004c: i2c_hid_set_power
	[   58.429945] i2c_hid 1-004c: __i2c_hid_command: cmd=54 01 00 08
	[   58.525276] i2c_hid 1-004c: i2c_hid_open
	^C

i2c_hid_close() is never called. Is this intended or am I missing something?

Thanks.

-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain confidential information. Any unauthorized review, use, disclosure or distribution is prohibited. If you are not the intended recipient, please contact the sender by reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------

From 05b85899534eacce21db0b39b86beca61d7799c8 Mon Sep 17 00:00:00 2001
From: Manoj Chourasia <[email protected]>
Date: Tue, 1 Oct 2013 15:39:00 +0530
Subject: [PATCH] HID: hidraw: close underlying device at removal of last
reader

Even though device exist bit is set the underlying HW device should be closed when the last reader of the device is closed i.e. open count drops to zero.

---
 drivers/hid/hidraw.c |   22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

index dbfe300..4faf728 100644
@@ -305,19 +305,27 @@ static int hidraw_fasync(int fd, struct file *file, int on)
 static void drop_ref(struct hidraw *hidraw, int exists_bit)
 {
 	if (exists_bit) {
-		hid_hw_close(hidraw->hid);
 		hidraw->exist = 0;
-		if (hidraw->open)
+		if (hidraw->open) {
+			hid_hw_close(hidraw->hid);
 			wake_up_interruptible(&hidraw->wait);
+		}
 	} else {
 		--hidraw->open;
 	}
 
-	if (!hidraw->open && !hidraw->exist) {
-		device_destroy(hidraw_class, MKDEV(hidraw_major, hidraw->minor));
-		hidraw_table[hidraw->minor] = NULL;
-		kfree(hidraw);
-	}
+	if (!hidraw->open)
+		if (!hidraw->exist) {
+			device_destroy(hidraw_class,
+				MKDEV(hidraw_major, hidraw->minor));
+			hidraw_table[hidraw->minor] = NULL;
+			kfree(hidraw);
+		} else {
+			/* close device for last reader */
+			hid_hw_power(hidraw->hid, PM_HINT_NORMAL);
+			hid_hw_close(hidraw->hid);
+		}
+
 }
 
 static int hidraw_release(struct inode * inode, struct file * file)

1.7.9.5

Yes, that patch fixes the issue for me, thanks!

It introduces following warning, though:

drivers/hid/hidraw.c: In function ‘drop_ref’:
drivers/hid/hidraw.c:320:5: warning: suggest explicit braces to avoid ambiguous ‘else’ [-Wparentheses]
if (!hidraw->open)
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Thanks Mika,

I will be fix the warning and post a patch for Jiri to review.

-Manoj

-----Original Message-----
From: Mika Westerberg [mailto:[email protected]]
Sent: Tuesday, October 01, 2013 6:13 PM
To: Manoj Chourasia
Cc: [email protected]; Jiri Kosina
Subject: Re: weird hidraw behaviour

Yes, that patch fixes the issue for me, thanks!

It introduces following warning, though:

drivers/hid/hidraw.c: In function ‘drop_ref’:
drivers/hid/hidraw.c:320:5: warning: suggest explicit braces to avoid ambiguous ‘else’ [-Wparentheses]
if (!hidraw->open)

-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain confidential information. Any unauthorized review, use, disclosure or distribution is prohibited. If you are not the intended recipient, please contact the sender by reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------

Hi Mika,

I have posted the patch in mailing list for review. Attaching it here too. Please test it one more time as I have fixed the warning.

Regards
-Manoj

-----Original Message-----
From: Mika Westerberg [mailto:[email protected]]
Sent: Tuesday, October 01, 2013 6:13 PM
To: Manoj Chourasia
Cc: [email protected]; Jiri Kosina
Subject: Re: weird hidraw behaviour

Yes, that patch fixes the issue for me, thanks!

It introduces following warning, though:

drivers/hid/hidraw.c: In function ‘drop_ref’:
drivers/hid/hidraw.c:320:5: warning: suggest explicit braces to avoid ambiguous ‘else’ [-Wparentheses]
if (!hidraw->open)

-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain confidential information. Any unauthorized review, use, disclosure or distribution is prohibited. If you are not the intended recipient, please contact the sender by reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------

From b37519b65be0d51b90c5fc177c0dc0cd2f6358b1 Mon Sep 17 00:00:00 2001
From: Manoj Chourasia <[email protected]>
Date: Tue, 1 Oct 2013 15:39:00 +0530
Subject: [PATCH] HID: hidraw: close underlying device at removal of last
reader

Even though device exist is set the underlying HW device should be closed when the last reader of the device is closed i.e. open count drops to zero.

---
 drivers/hid/hidraw.c |   21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

index dbfe300..3c0dd44 100644
@@ -305,19 +305,28 @@ static int hidraw_fasync(int fd, struct file *file, int on)
 static void drop_ref(struct hidraw *hidraw, int exists_bit)
 {
 	if (exists_bit) {
-		hid_hw_close(hidraw->hid);
 		hidraw->exist = 0;
-		if (hidraw->open)
+		if (hidraw->open) {
+			hid_hw_close(hidraw->hid);
 			wake_up_interruptible(&hidraw->wait);
+		}
 	} else {
 		--hidraw->open;
 	}
 
-	if (!hidraw->open && !hidraw->exist) {
-		device_destroy(hidraw_class, MKDEV(hidraw_major, hidraw->minor));
-		hidraw_table[hidraw->minor] = NULL;
-		kfree(hidraw);
+	if (!hidraw->open) {
+		if (!hidraw->exist) {
+			device_destroy(hidraw_class,
+				MKDEV(hidraw_major, hidraw->minor));
+			hidraw_table[hidraw->minor] = NULL;
+			kfree(hidraw);
+		} else {
+			/* close device for last reader */
+			hid_hw_power(hidraw->hid, PM_HINT_NORMAL);
+			hid_hw_close(hidraw->hid);
+		}
 	}
+
 }
 
 static int hidraw_release(struct inode * inode, struct file * file)

1.7.9.5

Still works for me, thanks!

Hi

I don't know whether it's intentional, but it is hardcoded this way now. Logic is, ->close() is called on hidraw_disconnect() that is, when hidraw is unloaded on a device. It no longer depends on user-space processes.

Any reason to change it back? It's no bug, so if no-one cares I'd leave it as it is now. Otherwise, we can try to change it again.

Regards
David

Well, if you open the device and close it from userspace, the device stays opened forever. I'm not sure if that's the intention.

I think that fix for this was already merged.

Hi

Indeed, fixed upstream in:
http://git.kernel.org/cgit/linux/kernel/git/jikos/hid.git/commit/?h=for-3.12/upstream-fixes&id=0f5a24c6602063e014ee48892ebf56093241106e

Thanks
David