* Q: weird hidraw behaviour @ 2013-09-24 8:56 Mika Westerberg 2013-10-01 11:43 ` Manoj Chourasia 2013-10-10 9:26 ` Q: " David Herrmann 0 siblings, 2 replies; 9+ messages in thread From: Mika Westerberg @ 2013-09-24 8:56 UTC (permalink / raw) To: Jiri Kosina, Manoj Chourasia; +Cc: linux-input 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: weird hidraw behaviour 2013-09-24 8:56 Q: weird hidraw behaviour Mika Westerberg @ 2013-10-01 11:43 ` Manoj Chourasia 2013-10-01 12:43 ` Mika Westerberg 2013-10-10 9:26 ` Q: " David Herrmann 1 sibling, 1 reply; 9+ messages in thread From: Manoj Chourasia @ 2013-10-01 11:43 UTC (permalink / raw) To: Mika Westerberg; +Cc: linux-input@vger.kernel.org, Jiri Kosina [-- 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: weird hidraw behaviour 2013-10-01 11:43 ` Manoj Chourasia @ 2013-10-01 12:43 ` Mika Westerberg 2013-10-01 13:18 ` Manoj Chourasia 2013-10-01 13:37 ` Manoj Chourasia 0 siblings, 2 replies; 9+ messages in thread From: Mika Westerberg @ 2013-10-01 12:43 UTC (permalink / raw) To: Manoj Chourasia; +Cc: linux-input@vger.kernel.org, Jiri Kosina 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: weird hidraw behaviour 2013-10-01 12:43 ` Mika Westerberg @ 2013-10-01 13:18 ` Manoj Chourasia 2013-10-01 13:37 ` Manoj Chourasia 1 sibling, 0 replies; 9+ messages in thread From: Manoj Chourasia @ 2013-10-01 13:18 UTC (permalink / raw) To: Mika Westerberg; +Cc: linux-input@vger.kernel.org, Jiri Kosina 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. ----------------------------------------------------------------------------------- ^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: weird hidraw behaviour 2013-10-01 12:43 ` Mika Westerberg 2013-10-01 13:18 ` Manoj Chourasia @ 2013-10-01 13:37 ` Manoj Chourasia 2013-10-01 13:56 ` Mika Westerberg 1 sibling, 1 reply; 9+ messages in thread From: Manoj Chourasia @ 2013-10-01 13:37 UTC (permalink / raw) To: Mika Westerberg; +Cc: linux-input@vger.kernel.org, Jiri Kosina [-- 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: weird hidraw behaviour 2013-10-01 13:37 ` Manoj Chourasia @ 2013-10-01 13:56 ` Mika Westerberg 0 siblings, 0 replies; 9+ messages in thread From: Mika Westerberg @ 2013-10-01 13:56 UTC (permalink / raw) To: Manoj Chourasia; +Cc: linux-input@vger.kernel.org, Jiri Kosina 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! ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Q: weird hidraw behaviour 2013-09-24 8:56 Q: weird hidraw behaviour Mika Westerberg 2013-10-01 11:43 ` Manoj Chourasia @ 2013-10-10 9:26 ` David Herrmann 2013-10-10 9:39 ` Mika Westerberg 1 sibling, 1 reply; 9+ messages in thread From: David Herrmann @ 2013-10-10 9:26 UTC (permalink / raw) To: Mika Westerberg; +Cc: Jiri Kosina, Manoj Chourasia, open list:HID CORE LAYER 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Q: weird hidraw behaviour 2013-10-10 9:26 ` Q: " David Herrmann @ 2013-10-10 9:39 ` Mika Westerberg 2013-10-10 9:41 ` David Herrmann 0 siblings, 1 reply; 9+ messages in thread From: Mika Westerberg @ 2013-10-10 9:39 UTC (permalink / raw) To: David Herrmann; +Cc: Jiri Kosina, Manoj Chourasia, open list:HID CORE LAYER 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Q: weird hidraw behaviour 2013-10-10 9:39 ` Mika Westerberg @ 2013-10-10 9:41 ` David Herrmann 0 siblings, 0 replies; 9+ messages in thread From: David Herrmann @ 2013-10-10 9:41 UTC (permalink / raw) To: Mika Westerberg; +Cc: Jiri Kosina, Manoj Chourasia, open list:HID CORE LAYER 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2013-10-10 9:41 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-09-24 8:56 Q: weird hidraw behaviour Mika Westerberg 2013-10-01 11:43 ` Manoj Chourasia 2013-10-01 12:43 ` Mika Westerberg 2013-10-01 13:18 ` Manoj Chourasia 2013-10-01 13:37 ` Manoj Chourasia 2013-10-01 13:56 ` Mika Westerberg 2013-10-10 9:26 ` Q: " David Herrmann 2013-10-10 9:39 ` Mika Westerberg 2013-10-10 9:41 ` David Herrmann
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).