usbhid: replace inappropriate ENOSYS with ENODEV

PATCH In review Heiner Kallweit 4 participants 5 messages Jan 20, 2016 – Jan 22, 2016

Subsystems (2)

HID USB HID/hidbp drivers (USB keyboards, mice, remote controls, ...)

lore

Share & tools

lore

Heiner Kallweit

author

2016-01-20 21:56 UTC #0 lore raw

PATCH 1/1 usbhid: replace inappropriate ENOSYS with ENODEV

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 <[email protected]>

---
 drivers/hid/usbhid/hid-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/hid/usbhid/hid-core.c +1 -1

index ad71160..64a8d9c 100644
@@ -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 */

Signature · 1 line

2.7.0

Jiri Kosina

maintainer

2016-01-21 14:18 UTC #1 lore raw

Quoted 9 lines · Heiner Kallweit

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.

Signature · 2 lines

Jiri Kosina
SUSE Labs

Dmitry Torokhov

2016-01-21 17:42 UTC #2 lore raw

Quoted 9 lines · Jiri Kosina

Quoted 7 lines · Heiner Kallweit
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.

Quoted 6 lines

Quoted 2 lines
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 <[email protected]>
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 <[email protected]>

Cc Pavel Machek <[email protected]>

Cc Michael Kerrisk <[email protected]>

Cc Joe Perches <[email protected]>

Signed-off-by Andrew Morton <[email protected]>

Signed-off-by Linus Torvalds <[email protected]>

Quoted 2 lines

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.

Signature · 1 line

Dmitry

Jiri Kosina

maintainer

2016-01-22 23:30 UTC #3 lore raw

Quoted 25 lines · Dmitry Torokhov

Quoted 9 lines · Jiri Kosina
Quoted 7 lines · Heiner Kallweit
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.
Quoted 6 lines
Quoted 2 lines
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 <[email protected]>
Date:   Thu Apr 16 12:44:44 2015 -0700
checkpatch.pl: new instances of ENOSYS are errors

Quoted 8 lines

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,

Signature · 2 lines

Jiri Kosina
SUSE Labs

Andy Lutomirski

2016-01-22 23:35 UTC #4 lore raw

Quoted 36 lines · Jiri Kosina

Quoted 25 lines · Dmitry Torokhov
Quoted 9 lines · Jiri Kosina
Quoted 7 lines · Heiner Kallweit
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.
Quoted 6 lines
Quoted 2 lines
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 <[email protected]>
Date:   Thu Apr 16 12:44:44 2015 -0700
checkpatch.pl: new instances of ENOSYS are errors
Quoted 8 lines
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.

Quoted 9 lines

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