[PATCH] usbhid: replace inappropriate ENOSYS with ENODEV

Subsystems: hid core layer, the rest, usb hid/hidbp drivers (usb keyboards, mice, remote controls, ...)

STALE3847d

5 messages, 4 authors, 2016-01-22 · open the first message on its own page

[PATCH] usbhid: replace inappropriate ENOSYS with ENODEV

From: Heiner Kallweit <hkallweit1@gmail.com>
Date: 2016-01-20 21:56:41

Primary meaning of ENOSYS is "system call not available" but it's also used
with meaning "function not implemented". Both are not applicable here.

Typically this error occurs when the device was unplugged.
usbhid_raw_request returns -ENODEV in such a case what seems to be more
reasonable. Therefore use -ENODEV also here.

Primary motivation for this change is a change in the LED subsystem to
ignore -ENODEV if the device was most likely unplugged.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/hid/usbhid/hid-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index ad71160..64a8d9c 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -928,7 +928,7 @@ static int usbhid_output_report(struct hid_device *hid, __u8 *buf, size_t count)
 	int actual_length, skipped_report_id = 0, ret;
 
 	if (!usbhid->urbout)
-		return -ENOSYS;
+		return -ENODEV;
 
 	if (buf[0] == 0x0) {
 		/* Don't send the Report ID */
-- 
2.7.0

Re: [PATCH] usbhid: replace inappropriate ENOSYS with ENODEV

From: Jiri Kosina <jikos@kernel.org>
Date: 2016-01-21 14:18:24

On Wed, 20 Jan 2016, Heiner Kallweit wrote:
Primary meaning of ENOSYS is "system call not available" but it's also used
with meaning "function not implemented". Both are not applicable here.

Typically this error occurs when the device was unplugged.
usbhid_raw_request returns -ENODEV in such a case what seems to be more
reasonable. Therefore use -ENODEV also here.

Primary motivation for this change is a change in the LED subsystem to
ignore -ENODEV if the device was most likely unplugged.
I believe POSIX defines ENOSYS as "Function not implemented". In this 
case, we are signalling there is no "URB OUT" function.

I don't have strong preference either way, but ENODEV doesn't seem to be 
particularly good fit for this purpose either.

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH] usbhid: replace inappropriate ENOSYS with ENODEV

From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Date: 2016-01-21 17:42:31

On Thu, Jan 21, 2016 at 6:18 AM, Jiri Kosina [off-list ref] wrote:
On Wed, 20 Jan 2016, Heiner Kallweit wrote:
quoted
Primary meaning of ENOSYS is "system call not available" but it's also used
with meaning "function not implemented". Both are not applicable here.

Typically this error occurs when the device was unplugged.
usbhid_raw_request returns -ENODEV in such a case what seems to be more
reasonable. Therefore use -ENODEV also here.

Primary motivation for this change is a change in the LED subsystem to
ignore -ENODEV if the device was most likely unplugged.
I believe POSIX defines ENOSYS as "Function not implemented". In this
case, we are signalling there is no "URB OUT" function.
Well, it looks like ENOSYS is really reserved for unimplemented system calls:

commit 91c9afaf97ee554d2cd3042a5ad01ad21c99e8c4
Author: Andy Lutomirski [off-list ref]
Date:   Thu Apr 16 12:44:44 2015 -0700

    checkpatch.pl: new instances of ENOSYS are errors

    ENOSYS means that a nonexistent system call was called.  We have a
    bad habit of using it for things like invalid operations on
    otherwise valid syscalls.  We should avoid this in new code.

    Pervasive incorrect usage of ENOSYS came up at the kernel summit ABI
    review discussion.  Let's see if checkpatch can help.

    I'll submit a separate patch for include/uapi/asm-generic/errno.h.

    Signed-off-by: Andy Lutomirski [off-list ref]
    Cc: Pavel Machek [off-list ref]
    Cc: Michael Kerrisk [off-list ref]
    Cc: Joe Perches [off-list ref]
    Signed-off-by: Andrew Morton [off-list ref]
    Signed-off-by: Linus Torvalds [off-list ref]
I don't have strong preference either way, but ENODEV doesn't seem to be
particularly good fit for this purpose either.
If there is no urbout during normal operation then -EINVAL I think is
applicable - upper layers are trying to send request to the device
that can not be executed, i.e. invalid request. If it happens during
removal then it is either a) there is a race or b) we are not actually
racing with whomever is resetting urbout to NULL but we need to check
if device is actually gone and return -ENXIO in this case.

Thanks.

-- 
Dmitry

Re: [PATCH] usbhid: replace inappropriate ENOSYS with ENODEV

From: Jiri Kosina <jikos@kernel.org>
Date: 2016-01-22 23:30:26

On Thu, 21 Jan 2016, Dmitry Torokhov wrote:
On Thu, Jan 21, 2016 at 6:18 AM, Jiri Kosina [off-list ref] wrote:
quoted
On Wed, 20 Jan 2016, Heiner Kallweit wrote:
quoted
Primary meaning of ENOSYS is "system call not available" but it's also used
with meaning "function not implemented". Both are not applicable here.

Typically this error occurs when the device was unplugged.
usbhid_raw_request returns -ENODEV in such a case what seems to be more
reasonable. Therefore use -ENODEV also here.

Primary motivation for this change is a change in the LED subsystem to
ignore -ENODEV if the device was most likely unplugged.
I believe POSIX defines ENOSYS as "Function not implemented". In this
case, we are signalling there is no "URB OUT" function.
Well, it looks like ENOSYS is really reserved for unimplemented system calls:

commit 91c9afaf97ee554d2cd3042a5ad01ad21c99e8c4
Author: Andy Lutomirski [off-list ref]
Date:   Thu Apr 16 12:44:44 2015 -0700

    checkpatch.pl: new instances of ENOSYS are errors

    ENOSYS means that a nonexistent system call was called.  We have a
    bad habit of using it for things like invalid operations on
    otherwise valid syscalls.  We should avoid this in new code.

    Pervasive incorrect usage of ENOSYS came up at the kernel summit ABI
    review discussion.  Let's see if checkpatch can help.

    I'll submit a separate patch for include/uapi/asm-generic/errno.h.
Let's add Andy to CC.

Andy, my understanding of POSIX is that ENOSYS is "function not 
implemented", meaning whatever internal function of kernel, not strictly 
just syscall entrypoint.

Where does your explanation of ENOSYS as "syscall not implemented" come 
from, please?

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH] usbhid: replace inappropriate ENOSYS with ENODEV

From: Andy Lutomirski <luto@amacapital.net>
Date: 2016-01-22 23:35:24

On Fri, Jan 22, 2016 at 3:30 PM, Jiri Kosina [off-list ref] wrote:
On Thu, 21 Jan 2016, Dmitry Torokhov wrote:
quoted
On Thu, Jan 21, 2016 at 6:18 AM, Jiri Kosina [off-list ref] wrote:
quoted
On Wed, 20 Jan 2016, Heiner Kallweit wrote:
quoted
Primary meaning of ENOSYS is "system call not available" but it's also used
with meaning "function not implemented". Both are not applicable here.

Typically this error occurs when the device was unplugged.
usbhid_raw_request returns -ENODEV in such a case what seems to be more
reasonable. Therefore use -ENODEV also here.

Primary motivation for this change is a change in the LED subsystem to
ignore -ENODEV if the device was most likely unplugged.
I believe POSIX defines ENOSYS as "Function not implemented". In this
case, we are signalling there is no "URB OUT" function.
Well, it looks like ENOSYS is really reserved for unimplemented system calls:

commit 91c9afaf97ee554d2cd3042a5ad01ad21c99e8c4
Author: Andy Lutomirski [off-list ref]
Date:   Thu Apr 16 12:44:44 2015 -0700

    checkpatch.pl: new instances of ENOSYS are errors

    ENOSYS means that a nonexistent system call was called.  We have a
    bad habit of using it for things like invalid operations on
    otherwise valid syscalls.  We should avoid this in new code.

    Pervasive incorrect usage of ENOSYS came up at the kernel summit ABI
    review discussion.  Let's see if checkpatch can help.

    I'll submit a separate patch for include/uapi/asm-generic/errno.h.
Let's add Andy to CC.

Andy, my understanding of POSIX is that ENOSYS is "function not
implemented", meaning whatever internal function of kernel, not strictly
just syscall entrypoint.

Where does your explanation of ENOSYS as "syscall not implemented" come
from, please?
Userspace likes to be able to tell whether a given syscall is
implemented by the running kernel.  Unimplemented syscalls return
-ENOSYS, and having syscalls that *are* implemented return -ENOSYS can
confuse things.

--Andy
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help