[PATCH] HID: vivaldi: avoid redundant feature report reads per usage

[PATCH] HID: vivaldi: avoid redundant feature report reads per usage

[Zhengping Jiang]

vivaldi_feature_mapping() is invoked by hidinput_configure_usages() in
ascending usage_index order for each usage in the top-row physical keymap
field. Because fetching the feature report at usage_index == 0 already
reads the entire report from hardware and populates field->value[] for all
usages in the field, calling hid_hw_raw_request() again for subsequent
usages redundantly reads the same report N times (e.g., 12 sequential BLE
GATT reads taking ~2s on Vivaldi BLE keyboards).

Only fetch the feature report from hardware on the first usage
(usage->usage_index == 0) and reuse field->value[usage->usage_index] for
subsequent usages in the field.

Fixes: 33bbe04a15f2 ("HID: google: extract Vivaldi hid feature mapping for use in hid-hammer")
Signed-off-by: Zhengping Jiang <jiangzp@google.com>
---
 drivers/hid/hid-vivaldi-common.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/hid/hid-vivaldi-common.c b/drivers/hid/hid-vivaldi-common.c
index c3c9fe9e697b..cfee7be4d759 100644
--- a/drivers/hid/hid-vivaldi-common.c
+++ b/drivers/hid/hid-vivaldi-common.c
@@ -49,6 +49,12 @@ void vivaldi_feature_mapping(struct hid_device *hdev,
 	if (fn_key > data->num_function_row_keys)
 		data->num_function_row_keys = fn_key;
 
+	if (usage->usage_index) {
+		data->function_row_physmap[fn_key - MIN_FN_ROW_KEY] =
+			field->value[usage->usage_index];
+		return;
+	}
+
 	report_data = buf = hid_alloc_report_buf(report, GFP_KERNEL);
 	if (!report_data)
 		return;
-- 
2.56.0.385.gd3acb90ef8-goog

Re: [PATCH] HID: vivaldi: avoid redundant feature report reads per usage

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] hid: feature report fetch bypassed if usage_index == 0 fails early validation
--

commit 31203c2d78508ee9933c6f629cd9827611acfce0
Author: Zhengping Jiang <jiangzp@google.com>

HID: vivaldi: avoid redundant feature report reads per usage

This commit optimizes the vivaldi_feature_mapping() initialization loop by
fetching the hardware feature report only on the first usage. Subsequent
usages in the same field skip the hardware read and reuse the previously
fetched field values.

> diff --git a/drivers/hid/hid-vivaldi-common.c b/drivers/hid/hid-vivaldi-common.c
> index b12bb5cc091aa..69df3a64eb91a 100644
> --- a/drivers/hid/hid-vivaldi-common.c
> +++ b/drivers/hid/hid-vivaldi-common.c
> @@ -49,6 +49,12 @@ void vivaldi_feature_mapping(struct hid_device *hdev,
>  	if (fn_key > data->num_function_row_keys)
>  		data->num_function_row_keys = fn_key;
>  
> +	if (usage->usage_index) {
> +		data->function_row_physmap[fn_key - MIN_FN_ROW_KEY] =
> +			field->value[usage->usage_index];
> +		return;
> +	}
> +

[Severity: Medium]
Does this fast-path correctly handle cases where the first usage in the
field is bypassed by early validation checks?

Looking at the start of vivaldi_feature_mapping(), there are early returns
for non-ordinal pages and out-of-bounds fn_key values:

void vivaldi_feature_mapping(...)
{
	...
	if (field->logical != HID_USAGE_FN_ROW_PHYSMAP ||
	    (usage->hid & HID_USAGE_PAGE) != HID_UP_ORDINAL)
		return;

	fn_key = usage->hid & HID_USAGE;
	if (fn_key < MIN_FN_ROW_KEY || fn_key > MAX_FN_ROW_KEY)
		return;

If the usage at usage_index == 0 triggers one of these early returns, the
hardware fetch at the end of the function is bypassed.

When the loop processes a valid subsequent usage, such as usage_index == 1,
it will hit this new fast-path. Because the hardware fetch was never
performed, will this copy the initial zeroed memory from field->value into
the physical map and misconfigure the function keys?

>  	report_data = buf = hid_alloc_report_buf(report, GFP_KERNEL);
>  	if (!report_data)
>  		return;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261012212902.3491839-1-jiangzp@google.com?part=1

[PATCH v2] HID: vivaldi: avoid redundant feature report reads per usage

[Zhengping Jiang]

vivaldi_feature_mapping() is invoked by hidinput_configure_usages() in
ascending usage_index order for each usage in the top-row physical keymap
field. Because fetching the feature report at usage_index == 0 already
reads the entire report from hardware and populates field->value[] for all
usages in the field, calling hid_hw_raw_request() again for subsequent
usages redundantly reads the same report N times (e.g., 12 sequential BLE
GATT reads taking ~2s on Vivaldi BLE keyboards).

Fetch the feature report from hardware once per HID_USAGE_FN_ROW_PHYSMAP
field at usage_index == 0 before validating individual usage entries, and
reuse field->value[usage->usage_index] for subsequent usages in the field.

Fixes: 33bbe04a15f2 ("HID: google: extract Vivaldi hid feature mapping for use in hid-hammer")
Signed-off-by: Zhengping Jiang <jiangzp@google.com>
---
Changes in v2:
- Move per-usage validation (HID_UP_ORDINAL and fn_key bounds) after the
  usage_index == 0 hardware read so the report is still fetched even if
  usage[0] is non-ordinal or out of bounds.

 drivers/hid/hid-vivaldi-common.c | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

diff --git a/drivers/hid/hid-vivaldi-common.c b/drivers/hid/hid-vivaldi-common.c
index c3c9fe9e697b..94caaf254f69 100644
--- a/drivers/hid/hid-vivaldi-common.c
+++ b/drivers/hid/hid-vivaldi-common.c
@@ -33,21 +33,16 @@ void vivaldi_feature_mapping(struct hid_device *hdev,
 {
 	struct vivaldi_data *data = hid_get_drvdata(hdev);
 	struct hid_report *report = field->report;
-	u8 *report_data, *buf;
+	u8 *report_data, *buf = NULL;
 	u32 report_len;
 	unsigned int fn_key;
 	int ret;
 
-	if (field->logical != HID_USAGE_FN_ROW_PHYSMAP ||
-	    (usage->hid & HID_USAGE_PAGE) != HID_UP_ORDINAL)
+	if (field->logical != HID_USAGE_FN_ROW_PHYSMAP)
 		return;
 
-	fn_key = usage->hid & HID_USAGE;
-	if (fn_key < MIN_FN_ROW_KEY || fn_key > MAX_FN_ROW_KEY)
-		return;
-
-	if (fn_key > data->num_function_row_keys)
-		data->num_function_row_keys = fn_key;
+	if (usage->usage_index)
+		goto parse_usage;
 
 	report_data = buf = hid_alloc_report_buf(report, GFP_KERNEL);
 	if (!report_data)
@@ -92,6 +87,17 @@ void vivaldi_feature_mapping(struct hid_device *hdev,
 		goto out;
 	}
 
+parse_usage:
+	if ((usage->hid & HID_USAGE_PAGE) != HID_UP_ORDINAL)
+		goto out;
+
+	fn_key = usage->hid & HID_USAGE;
+	if (fn_key < MIN_FN_ROW_KEY || fn_key > MAX_FN_ROW_KEY)
+		goto out;
+
+	if (fn_key > data->num_function_row_keys)
+		data->num_function_row_keys = fn_key;
+
 	data->function_row_physmap[fn_key - MIN_FN_ROW_KEY] =
 		field->value[usage->usage_index];
 
-- 
2.56.0.385.gd3acb90ef8-goog