Q: weird hidraw behaviour

Q: weird hidraw behaviour

[Mika Westerberg]

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.

RE: weird hidraw behaviour

[Manoj Chourasia]

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

Hi Mika,

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

Thanks
-Manoj

-----Original Message-----
From: Mika Westerberg [mailto:mika.westerberg@linux.intel.com] 
Sent: Tuesday, September 24, 2013 2:26 PM
To: Jiri Kosina; Manoj Chourasia
Cc: linux-input@vger.kernel.org
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.
-----------------------------------------------------------------------------------

[-- Attachment #2: HID-hidraw-close-underlying-device-at-removal-of-las.patch --]
[-- Type: application/octet-stream, Size: 1644 bytes --]

From 05b85899534eacce21db0b39b86beca61d7799c8 Mon Sep 17 00:00:00 2001
From: Manoj Chourasia <mchourasia@nvidia.com>
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.

Signed-off-by: Manoj Chourasia <mchourasia@nvidia.com>
---
 drivers/hid/hidraw.c |   22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/drivers/hid/hidraw.c b/drivers/hid/hidraw.c
index dbfe300..4faf728 100644
--- a/drivers/hid/hidraw.c
+++ b/drivers/hid/hidraw.c
@@ -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

Re: weird hidraw behaviour

[Mika Westerberg]

On Tue, Oct 01, 2013 at 05:13:16PM +0530, Manoj Chourasia wrote:
> Can you please try the attached patch and see if it fixes this issue?

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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: weird hidraw behaviour

[Manoj Chourasia]

Thanks Mika,

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

-Manoj


-----Original Message-----
From: Mika Westerberg [mailto:mika.westerberg@linux.intel.com] 
Sent: Tuesday, October 01, 2013 6:13 PM
To: Manoj Chourasia
Cc: linux-input@vger.kernel.org; Jiri Kosina
Subject: Re: weird hidraw behaviour

On Tue, Oct 01, 2013 at 05:13:16PM +0530, Manoj Chourasia wrote:
> Can you please try the attached patch and see if it fixes this issue?

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.
-----------------------------------------------------------------------------------

RE: weird hidraw behaviour

[Manoj Chourasia]

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

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:mika.westerberg@linux.intel.com] 
Sent: Tuesday, October 01, 2013 6:13 PM
To: Manoj Chourasia
Cc: linux-input@vger.kernel.org; Jiri Kosina
Subject: Re: weird hidraw behaviour

On Tue, Oct 01, 2013 at 05:13:16PM +0530, Manoj Chourasia wrote:
> Can you please try the attached patch and see if it fixes this issue?

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.
-----------------------------------------------------------------------------------

[-- Attachment #2: PATCH-hidraw-close-underlying-device-at-removal-of-las.patch --]
[-- Type: application/octet-stream, Size: 1641 bytes --]

From b37519b65be0d51b90c5fc177c0dc0cd2f6358b1 Mon Sep 17 00:00:00 2001
From: Manoj Chourasia <mchourasia@nvidia.com>
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.

Signed-off-by: Manoj Chourasia <mchourasia@nvidia.com>
---
 drivers/hid/hidraw.c |   21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/drivers/hid/hidraw.c b/drivers/hid/hidraw.c
index dbfe300..3c0dd44 100644
--- a/drivers/hid/hidraw.c
+++ b/drivers/hid/hidraw.c
@@ -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

Re: weird hidraw behaviour

[Mika Westerberg]

On Tue, Oct 01, 2013 at 07:07:57PM +0530, Manoj Chourasia wrote:
> 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.

Still works for me, thanks!

Re: Q: weird hidraw behaviour

[David Herrmann]

Hi

On Tue, Sep 24, 2013 at 10:56 AM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> 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?

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

Re: Q: weird hidraw behaviour

[Mika Westerberg]

On Thu, Oct 10, 2013 at 11:26:45AM +0200, David Herrmann wrote:
> Hi
> 
> On Tue, Sep 24, 2013 at 10:56 AM, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> > 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?
> 
> 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.

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.

Re: Q: weird hidraw behaviour

[David Herrmann]

Hi

On Thu, Oct 10, 2013 at 11:39 AM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> On Thu, Oct 10, 2013 at 11:26:45AM +0200, David Herrmann wrote:
>> 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.
>
> 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.

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