__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,
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
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
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
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
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
Hi Dan,
Thanks, that makes sense now. The point isn't just showing the code is buggy, it's explaining why changing a years-old return value is safe, and that belongs in the commit message rather than the thread.
I'll write that reasoning in next time (what checks the return, what doesn't, and what could be affected) instead of leaving reviewers to ask.
Appreciate you taking the time to walk me through it.
Hao-Qun
-
Dan Carpenter -
Hao-Qun Huang