[PATCH] HID: uhid: actually use the err number from userspace

This can help debugging the situation

Signed-off-by: Benjamin Tissoires <redacted>
---

Hi,

not entirely sure if we can use this in a such simple way.

However, this is useful to mimic device behaviour from userspace.

Cheers,
Benjamin

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

diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c
index fa0cc0899827..2fa32e7fc733 100644
--- a/drivers/hid/uhid.c
+++ b/drivers/hid/uhid.c

@@ -284,7 +284,7 @@ static int uhid_hid_set_report(struct hid_device *hid, unsigned char rnum,
 		goto unlock;
 
 	if (uhid->report_buf.u.set_report_reply.err)
-		ret = -EIO;
+		ret = -uhid->report_buf.u.set_report_reply.err;
 	else
 		ret = count;
 

-- 
2.19.2

Hey

On Mon, Aug 12, 2019 at 6:21 PM Benjamin Tissoires
[off-list ref] wrote:

I am generally in favor of this. But:

1) can you do this for both set_report *and* get_report?

2) I think you have to filter some of the error codes. For instance,
if you return one of the -ERESTARTSYS codes, this might cause the
syscall to restart (if auto-restart is enabled on this context). At
the same time, this is not *that* bad. It might even be useful for the
userspace driver to trigger an EINTR. At least we should be aware of
this. So maybe filters are not necessary.. Mhhh. Comments?

Thanks
David

        else
                ret = count;

--
2.19.2

On Tue, Aug 13, 2019 at 12:50 PM David Rheinsberg
[off-list ref] wrote:

Hey

On Mon, Aug 12, 2019 at 6:21 PM Benjamin Tissoires
[off-list ref] wrote:

quoted

This can help debugging the situation

Signed-off-by: Benjamin Tissoires <redacted>
---

Hi,

not entirely sure if we can use this in a such simple way.

However, this is useful to mimic device behaviour from userspace.

Cheers,
Benjamin

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

diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c
index fa0cc0899827..2fa32e7fc733 100644
--- a/drivers/hid/uhid.c
+++ b/drivers/hid/uhid.c

@@ -284,7 +284,7 @@ static int uhid_hid_set_report(struct hid_device *hid, unsigned char rnum,
                goto unlock;

        if (uhid->report_buf.u.set_report_reply.err)
-               ret = -EIO;
+               ret = -uhid->report_buf.u.set_report_reply.err;

I am generally in favor of this. But:

1) can you do this for both set_report *and* get_report?

right :)

2) I think you have to filter some of the error codes. For instance,
if you return one of the -ERESTARTSYS codes, this might cause the
syscall to restart (if auto-restart is enabled on this context). At
the same time, this is not *that* bad. It might even be useful for the
userspace driver to trigger an EINTR. At least we should be aware of
this. So maybe filters are not necessary.. Mhhh. Comments?

I haven't thought at all of the side effects of letting the user
return a random error code.
I have the impression that anything below EHWPOISON (133) is
relatively safe. So maybe we should just make sure the error code is
below 134?

The ERESTARTSYS has a few warnings in the include file, so I guess the
side effects might be too much for what we want to deal with.

Cheers,
Benjamin

Thanks
David

quoted

        else
                ret = count;

--
2.19.2

Hey

quoted

2) I think you have to filter some of the error codes. For instance,
if you return one of the -ERESTARTSYS codes, this might cause the
syscall to restart (if auto-restart is enabled on this context). At
the same time, this is not *that* bad. It might even be useful for the
userspace driver to trigger an EINTR. At least we should be aware of
this. So maybe filters are not necessary.. Mhhh. Comments?

I haven't thought at all of the side effects of letting the user
return a random error code.
I have the impression that anything below EHWPOISON (133) is
relatively safe. So maybe we should just make sure the error code is
below 134?

The ERESTARTSYS has a few warnings in the include file, so I guess the
side effects might be too much for what we want to deal with.

How about `err < ERESTARTSYS`? That is, we grant user-space the entire
range [1-511]. This seems to be the range reserved for uapi.

I think the ERESTART* codes would be fine as well, but I also don't
believe there to be any actual use-case for them. Anyway, I am fine
with either range.

Thanks
David