[PATCH] staging: greybus: hid: fix SET_REPORT return value

[PATCH] staging: greybus: hid: fix SET_REPORT return value

[Hao-Qun Huang]

__gb_hid_output_raw_report() stores the result of gb_hid_set_report()
in ret and even adjusts it to account for the report ID byte, but then
always returns 0.

This hides Greybus transport errors from HID_REQ_SET_REPORT callers,
and makes hidraw report zero bytes written to user space on success,
although hid_hw_raw_request() is expected to return the number of
bytes transferred or a negative errno. The sibling GET_REPORT path,
__gb_hid_get_raw_report(), already follows this convention.

Return ret like the other HID transport drivers do.

Fixes: 96eab779e198 ("greybus: hid: add HID class driver")
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-fable-5
Signed-off-by: Hao-Qun Huang <alvinhuang0603@gmail.com>
---
diff --git a/drivers/staging/greybus/hid.c b/drivers/staging/greybus/hid.c
index f1f9f6fbc00e..1d7186eecd23 100644
--- a/drivers/staging/greybus/hid.c
+++ b/drivers/staging/greybus/hid.c
@@ -256,7 +256,7 @@ static int __gb_hid_output_raw_report(struct hid_device *hid, __u8 *buf,
 	if (report_id && ret >= 0)
 		ret++; /* add report_id to the number of transferred bytes */
 
-	return 0;
+	return ret;
 }
 
 static int gb_hid_raw_request(struct hid_device *hid, unsigned char reportnum,

Re: [PATCH] staging: greybus: hid: fix SET_REPORT return value

[Dan Carpenter]

On Sat, Jul 04, 2026 at 04:16:13PM +0800, Hao-Qun Huang wrote:
> __gb_hid_output_raw_report() stores the result of gb_hid_set_report()
> in ret and even adjusts it to account for the report ID byte, but then
> always returns 0.
> 
> This hides Greybus transport errors from HID_REQ_SET_REPORT callers,
> and makes hidraw report zero bytes written to user space on success,
> although hid_hw_raw_request() is expected to return the number of
> bytes transferred or a negative errno. The sibling GET_REPORT path,
> __gb_hid_get_raw_report(), already follows this convention.
> 
> Return ret like the other HID transport drivers do.
> 
> Fixes: 96eab779e198 ("greybus: hid: add HID class driver")
> Cc: stable@vger.kernel.org
> Assisted-by: Claude:claude-fable-5
> Signed-off-by: Hao-Qun Huang <alvinhuang0603@gmail.com>
> ---

These kinds of changes require testing.  How have you tested this
change?

regards,
dan carpenter

Re: [PATCH] staging: greybus: hid: fix SET_REPORT return value

[Hao-Qun Huang]

On Jul 9, 2026 at 6:52 PM, Dan Carpenter <error27@gmail.com> wrote:
> These kinds of changes require testing.  How have you tested this
> change?

I compile-tested it (W=1, building gb-hid, greybus and hid together) and
traced the return path by hand.  I don't have Greybus HID hardware and
couldn't find a working emulator (gbsim has been dead since Ara), so I
haven't run it on a live device.

The bug is that gb_hid_set_report() returns -errno on failure and len on
success, and __gb_hid_output_raw_report() computes that into ret (even
adding one back for the report ID byte) and then returns 0, discarding
it.  So a successful hidraw write reports 0 bytes written and a failed
SET_REPORT looks like success.  The GET path next to it already returns
the count, as do usbhid, i2c-hid and uhid, so callers already handle a
positive return and greybus HID was the only one returning 0.

Thanks,
Hao-Qun

Re: [PATCH] staging: greybus: hid: fix SET_REPORT return value

[Dan Carpenter]

On Fri, Jul 10, 2026 at 02:06:44AM +0800, Hao-Qun Huang wrote:
> On Jul 9, 2026 at 6:52 PM, Dan Carpenter <error27@gmail.com> wrote:
> > These kinds of changes require testing.  How have you tested this
> > change?
> 
> I compile-tested it (W=1, building gb-hid, greybus and hid together) and
> traced the return path by hand.  I don't have Greybus HID hardware and
> couldn't find a working emulator (gbsim has been dead since Ara), so I
> haven't run it on a live device.
> 
> The bug is that gb_hid_set_report() returns -errno on failure and len on
> success, and __gb_hid_output_raw_report() computes that into ret (even
> adding one back for the report ID byte) and then returns 0, discarding
> it.  So a successful hidraw write reports 0 bytes written and a failed
> SET_REPORT looks like success.  The GET path next to it already returns
> the count, as do usbhid, i2c-hid and uhid, so callers already handle a
> positive return and greybus HID was the only one returning 0.

The bug is not hard to understand, the issue is that this changes the
function completely...  Was nothing checking the return before?

regards,
dan carpenter

Re: [PATCH] staging: greybus: hid: fix SET_REPORT return value

[Hao-Qun Huang]

On Jul 10, 2026 at 2:50 AM, Dan Carpenter <error27@gmail.com> wrote:
> The bug is not hard to understand, the issue is that this changes the
> function completely...  Was nothing checking the return before?

It gets checked.  The value flows unchanged through __hid_hw_raw_request()
to the callers, and they look at it two ways:

 - hidraw returns it straight to userspace (write(), HIDIOCSFEATURE),
   where it is the number of bytes transferred.

 - in-kernel SET_REPORT callers, some testing "ret < 0" (hid-multitouch,
   hid-sony), some testing "ret != size" (hid-gt683r, hid-lenovo,
   hid-razer).

So the old return 0 was wrong both ways: the first group had a failed
SET_REPORT masked as success, and the second saw every SET_REPORT as a
failure.  Returning the count or a negative errno is what GET already
does in this driver and what usbhid/i2c-hid/uhid return, so nothing
working with those relied on the 0.

Thanks,
Hao-Qun

Re: [PATCH] staging: greybus: hid: fix SET_REPORT return value

[Dan Carpenter]

So the patch has already been merged and it's fine.  The fix is
correct.

On Fri, Jul 10, 2026 at 12:40:50PM +0800, Hao-Qun Huang wrote:
> On Jul 10, 2026 at 2:50 AM, Dan Carpenter <error27@gmail.com> wrote:
> > The bug is not hard to understand, the issue is that this changes the
> > function completely...  Was nothing checking the return before?
> 
> It gets checked.  The value flows unchanged through __hid_hw_raw_request()
> to the callers, and they look at it two ways:
> 
>  - hidraw returns it straight to userspace (write(), HIDIOCSFEATURE),
>    where it is the number of bytes transferred.
> 
>  - in-kernel SET_REPORT callers, some testing "ret < 0" (hid-multitouch,
>    hid-sony), some testing "ret != size" (hid-gt683r, hid-lenovo,
>    hid-razer).
> 
> So the old return 0 was wrong both ways: the first group had a failed
> SET_REPORT masked as success, and the second saw every SET_REPORT as a
> failure.  Returning the count or a negative errno is what GET already
> does in this driver and what usbhid/i2c-hid/uhid return, so nothing
> working with those relied on the 0.

What I'm trying to say is, sure, it's easy to see the code is buggy but
it's been that way for years.  Your patch changes the return completely
from always returning zero to never returning zero.  When we're reviewing
this patch we want to know how making that change is safe.

In staging often the answer is that nothing was calling that function and
we can delete it...

But here, the real answer is that almost nothing checks for errors.  For
the few places that do, almost all of them only check for negatives.
That's probably how the code was able to work as it is...  The commit
message it should explain the risks.

"Changing this code is fine because almost nothing checks for errors.
There are a one or two in kernel checks which care about the exact
positive return and this patch will fix that but basically not much is
affected.  And hopefully userspace doesn't check either or it only checks
for negative errors.  But in the spirit of correctness, lets change this
return to be return the number of bytes and if userspace needs adjusting
we will deal with that when users file their bug reports."

regards,
dan carpenter