[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

[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

[PATCH v3] 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 v3:
- Add comment explaining why the report is only read at usage_index == 0
  and how failed reads rely on zero-initialized field->value[].
- Jump/fall through to parse_usage on read failure so usage_index == 0
  updates num_function_row_keys consistently with subsequent usages and
  preserves original behavior when only one usage is present.

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 | 34 +++++++++++++++++++++-----------
 1 file changed, 22 insertions(+), 12 deletions(-)

diff --git a/drivers/hid/hid-vivaldi-common.c b/drivers/hid/hid-vivaldi-common.c
index c3c9fe9e697b..2d85555289d8 100644
--- a/drivers/hid/hid-vivaldi-common.c
+++ b/drivers/hid/hid-vivaldi-common.c
@@ -33,21 +33,22 @@ 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;
+	/*
+	 * Read the feature report only at usage_index == 0, as a single read
+	 * populates field->value[] for all usages in the field. If the read
+	 * fails, subsequent usages will not re-read the report and will rely
+	 * on field->value[] being zero-initialized.
+	 */
+	if (usage->usage_index)
+		goto parse_usage;
 
 	report_data = buf = hid_alloc_report_buf(report, GFP_KERNEL);
 	if (!report_data)
@@ -72,7 +73,7 @@ void vivaldi_feature_mapping(struct hid_device *hdev,
 	if (ret < 0) {
 		dev_warn(&hdev->dev, "failed to fetch feature %d\n",
 			 field->report->id);
-		goto out;
+		goto parse_usage;
 	}
 
 	if (!report->id) {
@@ -86,11 +87,20 @@ void vivaldi_feature_mapping(struct hid_device *hdev,
 
 	ret = __hid_report_raw_event(hdev, HID_FEATURE_REPORT, report_data,
 				     report_len, report_len, 0);
-	if (ret) {
+	if (ret)
 		dev_warn(&hdev->dev, "failed to report feature %d\n",
 			 field->report->id);
+
+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

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

[Dmitry Torokhov]

Hi Zhengping,

On Tue, Oct 13, 2026 at 07:23:18PM +0000, Zhengping Jiang wrote:
> 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).

A minor note on the commit description: drv->feature_mapping() is invoked
by report_features(), not hidinput_configure_usages().

> diff --git a/drivers/hid/hid-vivaldi-common.c b/drivers/hid/hid-vivaldi-common.c
> index c3c9fe9e697b..2d85555289d8 100644
> --- a/drivers/hid/hid-vivaldi-common.c
> +++ b/drivers/hid/hid-vivaldi-common.c
> @@ -33,21 +33,22 @@ 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;
> +	/*
> +	 * Read the feature report only at usage_index == 0, as a single read
> +	 * populates field->value[] for all usages in the field. If the read
> +	 * fails, subsequent usages will not re-read the report and will rely
> +	 * on field->value[] being zero-initialized.
> +	 */
> +	if (usage->usage_index)
> +		goto parse_usage;
>  
>  	report_data = buf = hid_alloc_report_buf(report, GFP_KERNEL);
>  	if (!report_data)
> @@ -72,7 +73,7 @@ void vivaldi_feature_mapping(struct hid_device *hdev,
>  	if (ret < 0) {
>  		dev_warn(&hdev->dev, "failed to fetch feature %d\n",
>  			 field->report->id);
> -		goto out;
> +		goto parse_usage;
>  	}
>  
>  	if (!report->id) {
> @@ -86,11 +87,20 @@ void vivaldi_feature_mapping(struct hid_device *hdev,
>  
>  	ret = __hid_report_raw_event(hdev, HID_FEATURE_REPORT, report_data,
>  				     report_len, report_len, 0);

It looks like this patch was generated against a downstream ChromeOS
tree (__hid_report_raw_event() does not exist upstream; mainline uses
hid_report_raw_event()), so it does not apply cleanly.

> -	if (ret) {
> +	if (ret)
>  		dev_warn(&hdev->dev, "failed to report feature %d\n",
>  			 field->report->id);
> +
> +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];

If reading or parsing the feature report fails, we should not be
updating data->num_function_row_keys or populating
data->function_row_physmap[] with zeros: vivaldi_is_visible() checks
data->num_function_row_keys to decide whether to expose the
function_row_physmap sysfs attribute, and on a failed read we do not
want to expose a bogus all-zero keymap to userspace.

Also, keeping "out: kfree(buf);" at the very end of the function across
parse_usage makes the control flow and cleanup awkward (having to
initialize buf to NULL, jumping over the allocation on usage_index > 0,
and using goto out in parse_usage even though buf is no longer needed
once the report is processed), and it prevents using __free(kfree) for
buf.

Since vivaldi_feature_mapping() already has the entire field in hand,
why don't we bail out immediately when usage->usage_index != 0, fetch
the report once at usage_index == 0 using __free(kfree), and iterate
over all usages in the field right there?

Please rebase your patch on top of Shashikirana's patch extracting the
feature report value directly via hid_field_extract() before
resubmitting:
https://lore.kernel.org/r/20261013-upstream-vivaldi-fix-v2-1-ecc20329d24c@chromium.org

With that in place, vivaldi_feature_mapping() can simply become:

	if (field->logical != HID_USAGE_FN_ROW_PHYSMAP || usage->usage_index)
		return;

	u8 *buf __free(kfree) = hid_alloc_report_buf(report, GFP_KERNEL);
	if (!buf)
		return;

	report_len = hid_report_len(report) + !report->id;
	ret = hid_hw_raw_request(hdev, report->id, buf, report_len,
				 HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
	if (ret != report_len) {
		dev_warn(&hdev->dev, "failed to fetch feature %d: %d\n",
			 report->id, ret);
		return;
	}

	for (i = 0; i < field->maxusage && i < field->report_count; i++) {
		usage = &field->usage[i];

		if ((usage->hid & HID_USAGE_PAGE) != HID_UP_ORDINAL)
			continue;

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

		if (fn_key > data->num_function_row_keys)
			data->num_function_row_keys = fn_key;

		offset = field->report_offset + i * field->report_size;
		data->function_row_physmap[fn_key - MIN_FN_ROW_KEY] =
			hid_field_extract(hdev, buf + 1, offset,
					  field->report_size);
	}

Thanks.

-- 
Dmitry

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

[Zhengping Jiang]

vivaldi_feature_mapping() is invoked by report_features() 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, 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).

Bail out immediately when usage->usage_index != 0, fetch the feature
report once at usage_index == 0, and iterate over all usages in the
field to populate data->function_row_physmap[].

Fixes: 33bbe04a15f2 ("HID: google: extract Vivaldi hid feature mapping for use in hid-hammer")
Signed-off-by: Zhengping Jiang <jiangzp@google.com>
---
Based on "[PATCH v2] HID: vivaldi: Extract feature report value directly in vivaldi_feature_mapping()":
https://lore.kernel.org/r/20261013-upstream-vivaldi-fix-v2-1-ecc20329d24c@chromium.org

Changes in v4:
- Rebase on top of Shashikirana's patch using hid_field_extract() and
  __free(kfree), as suggested by Dmitry Torokhov.
- Return immediately when usage->usage_index != 0, fetch the report once
  at usage_index == 0, and loop over all usages in the field so failed
  reads do not update num_function_row_keys or expose an all-zero keymap.
- Fix caller name in commit description (report_features()).

Changes in v3:
- Add comment explaining why the report is only read at usage_index == 0
  and how failed reads rely on zero-initialized field->value[].
- Jump/fall through to parse_usage on read failure.

Changes in v2:
- Move per-usage validation (HID_UP_ORDINAL and fn_key bounds) after the
  usage_index == 0 hardware read.

 drivers/hid/hid-vivaldi-common.c | 33 +++++++++++++++++++-------------
 1 file changed, 20 insertions(+), 13 deletions(-)

diff --git a/drivers/hid/hid-vivaldi-common.c b/drivers/hid/hid-vivaldi-common.c
index 1359a107e63b..973393d09356 100644
--- a/drivers/hid/hid-vivaldi-common.c
+++ b/drivers/hid/hid-vivaldi-common.c
@@ -39,19 +39,11 @@ void vivaldi_feature_mapping(struct hid_device *hdev,
 	unsigned int offset;
 	u32 report_len;
 	int ret;
+	int i;
 
-	if (field->logical != HID_USAGE_FN_ROW_PHYSMAP ||
-	    (usage->hid & HID_USAGE_PAGE) != HID_UP_ORDINAL ||
-	    usage->usage_index >= field->report_count)
+	if (field->logical != HID_USAGE_FN_ROW_PHYSMAP || usage->usage_index)
 		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;
-
 	u8 *buf __free(kfree) = hid_alloc_report_buf(report, GFP_KERNEL);
 	if (!buf)
 		return;
@@ -74,9 +66,24 @@ void vivaldi_feature_mapping(struct hid_device *hdev,
 		return;
 	}
 
-	offset = field->report_offset + usage->usage_index * field->report_size;
-	data->function_row_physmap[fn_key - MIN_FN_ROW_KEY] =
-		hid_field_extract(hdev, buf + 1, offset, field->report_size);
+	for (i = 0; i < field->maxusage && i < field->report_count; i++) {
+		usage = &field->usage[i];
+
+		if ((usage->hid & HID_USAGE_PAGE) != HID_UP_ORDINAL)
+			continue;
+
+		fn_key = usage->hid & HID_USAGE;
+		if (fn_key < MIN_FN_ROW_KEY || fn_key > MAX_FN_ROW_KEY)
+			continue;
+
+		if (fn_key > data->num_function_row_keys)
+			data->num_function_row_keys = fn_key;
+
+		offset = field->report_offset + i * field->report_size;
+		data->function_row_physmap[fn_key - MIN_FN_ROW_KEY] =
+			hid_field_extract(hdev, buf + 1, offset,
+					  field->report_size);
+	}
 }
 EXPORT_SYMBOL_GPL(vivaldi_feature_mapping);
 
-- 
2.56.0.385.gd3acb90ef8-goog