* [PATCH v3 3/4] input: touchscreen: add I2C support for Goodix Berlin Touchscreen IC
From: Neil Armstrong @ 2023-06-22 14:29 UTC (permalink / raw)
To: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Bastien Nocera, Hans de Goede, Henrik Rydberg, Jeff LaBundy
Cc: linux-input, linux-arm-msm, devicetree, linux-kernel,
Neil Armstrong
In-Reply-To: <20230606-topic-goodix-berlin-upstream-initial-v3-0-f0577cead709@linaro.org>
Add initial support for the new Goodix "Berlin" touchscreen ICs
over the I2C interface.
This initial driver is derived from the Goodix goodix_ts_berlin
available at [1] and [2] and only supports the GT9916 IC
present on the Qualcomm SM8550 MTP & QRD touch panel.
The current implementation only supports BerlinD, aka GT9916.
[1] https://github.com/goodix/goodix_ts_berlin
[2] https://git.codelinaro.org/clo/la/platform/vendor/opensource/touch-drivers
Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
---
drivers/input/touchscreen/Kconfig | 14 ++++++
drivers/input/touchscreen/Makefile | 1 +
drivers/input/touchscreen/goodix_berlin_i2c.c | 69 +++++++++++++++++++++++++++
3 files changed, 84 insertions(+)
diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
index 1a6f6f6da991..5e21cca6025d 100644
--- a/drivers/input/touchscreen/Kconfig
+++ b/drivers/input/touchscreen/Kconfig
@@ -421,6 +421,20 @@ config TOUCHSCREEN_GOODIX_BERLIN_CORE
depends on REGMAP
tristate
+config TOUCHSCREEN_GOODIX_BERLIN_I2C
+ tristate "Goodix Berlin I2C touchscreen"
+ depends on I2C
+ depends on REGMAP_I2C
+ select TOUCHSCREEN_GOODIX_BERLIN_CORE
+ help
+ Say Y here if you have a Goodix Berlin IC connected to
+ your system via I2C.
+
+ If unsure, say N.
+
+ To compile this driver as a module, choose M here: the
+ module will be called goodix_berlin_i2c.
+
config TOUCHSCREEN_HIDEEP
tristate "HiDeep Touch IC"
depends on I2C
diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
index 29cdb042e104..921a2da0c2be 100644
--- a/drivers/input/touchscreen/Makefile
+++ b/drivers/input/touchscreen/Makefile
@@ -48,6 +48,7 @@ obj-$(CONFIG_TOUCHSCREEN_EXC3000) += exc3000.o
obj-$(CONFIG_TOUCHSCREEN_FUJITSU) += fujitsu_ts.o
obj-$(CONFIG_TOUCHSCREEN_GOODIX) += goodix_ts.o
obj-$(CONFIG_TOUCHSCREEN_GOODIX_BERLIN_CORE) += goodix_berlin_core.o
+obj-$(CONFIG_TOUCHSCREEN_GOODIX_BERLIN_I2C) += goodix_berlin_i2c.o
obj-$(CONFIG_TOUCHSCREEN_HIDEEP) += hideep.o
obj-$(CONFIG_TOUCHSCREEN_HYNITRON_CSTXXX) += hynitron_cstxxx.o
obj-$(CONFIG_TOUCHSCREEN_ILI210X) += ili210x.o
diff --git a/drivers/input/touchscreen/goodix_berlin_i2c.c b/drivers/input/touchscreen/goodix_berlin_i2c.c
new file mode 100644
index 000000000000..6407b2258eb1
--- /dev/null
+++ b/drivers/input/touchscreen/goodix_berlin_i2c.c
@@ -0,0 +1,69 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Goodix Berlin Touchscreen Driver
+ *
+ * Copyright (C) 2020 - 2021 Goodix, Inc.
+ * Copyright (C) 2023 Linaro Ltd.
+ *
+ * Based on goodix_ts_berlin driver.
+ */
+#include <linux/i2c.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+
+#include "goodix_berlin.h"
+
+#define I2C_MAX_TRANSFER_SIZE 256
+
+static const struct regmap_config goodix_berlin_i2c_regmap_conf = {
+ .reg_bits = 32,
+ .val_bits = 8,
+ .max_raw_read = I2C_MAX_TRANSFER_SIZE,
+ .max_raw_write = I2C_MAX_TRANSFER_SIZE,
+};
+
+/* vendor & product left unassigned here, should probably be updated from fw info */
+static const struct input_id goodix_berlin_i2c_input_id = {
+ .bustype = BUS_I2C,
+};
+
+static int goodix_berlin_i2c_probe(struct i2c_client *client)
+{
+ struct regmap *regmap;
+
+ regmap = devm_regmap_init_i2c(client, &goodix_berlin_i2c_regmap_conf);
+ if (IS_ERR(regmap))
+ return PTR_ERR(regmap);
+
+ return goodix_berlin_probe(&client->dev, client->irq,
+ &goodix_berlin_i2c_input_id, regmap);
+}
+
+static const struct i2c_device_id goodix_berlin_i2c_id[] = {
+ { "gt9916", 0 },
+ { }
+};
+
+MODULE_DEVICE_TABLE(i2c, goodix_berlin_i2c_id);
+
+static const struct of_device_id goodix_berlin_i2c_of_match[] = {
+ { .compatible = "goodix,gt9916", },
+ { }
+};
+MODULE_DEVICE_TABLE(of, goodix_berlin_i2c_of_match);
+
+static struct i2c_driver goodix_berlin_i2c_driver = {
+ .driver = {
+ .name = "goodix-berlin-i2c",
+ .of_match_table = goodix_berlin_i2c_of_match,
+ .pm = pm_sleep_ptr(&goodix_berlin_pm_ops),
+ },
+ .probe = goodix_berlin_i2c_probe,
+ .id_table = goodix_berlin_i2c_id,
+};
+module_i2c_driver(goodix_berlin_i2c_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Goodix Berlin I2C Touchscreen driver");
+MODULE_AUTHOR("Neil Armstrong <neil.armstrong@linaro.org>");
--
2.34.1
^ permalink raw reply related
* [PATCH v3 2/4] input: touchscreen: add core support for Goodix Berlin Touchscreen IC
From: Neil Armstrong @ 2023-06-22 14:29 UTC (permalink / raw)
To: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Bastien Nocera, Hans de Goede, Henrik Rydberg, Jeff LaBundy
Cc: linux-input, linux-arm-msm, devicetree, linux-kernel,
Neil Armstrong
In-Reply-To: <20230606-topic-goodix-berlin-upstream-initial-v3-0-f0577cead709@linaro.org>
Add initial support for the new Goodix "Berlin" touchscreen ICs.
These touchscreen ICs support SPI, I2C and I3C interface, up to
10 finger touch, stylus and gestures events.
This initial driver is derived from the Goodix goodix_ts_berlin
available at [1] and [2] and only supports the GT9916 IC
present on the Qualcomm SM8550 MTP & QRD touch panel.
The current implementation only supports BerlinD, aka GT9916.
Support for advanced features like:
- Firmware & config update
- Stylus events
- Gestures events
- Previous revisions support (BerlinA or BerlinB)
is not included in current version.
The current support will work with currently flashed firmware
and config, and bail out if firmware or config aren't flashed yet.
[1] https://github.com/goodix/goodix_ts_berlin
[2] https://git.codelinaro.org/clo/la/platform/vendor/opensource/touch-drivers
Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
---
drivers/input/touchscreen/Kconfig | 5 +
drivers/input/touchscreen/Makefile | 1 +
drivers/input/touchscreen/goodix_berlin.h | 159 +++++++
drivers/input/touchscreen/goodix_berlin_core.c | 584 +++++++++++++++++++++++++
4 files changed, 749 insertions(+)
diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
index c2cbd332af1d..1a6f6f6da991 100644
--- a/drivers/input/touchscreen/Kconfig
+++ b/drivers/input/touchscreen/Kconfig
@@ -416,6 +416,11 @@ config TOUCHSCREEN_GOODIX
To compile this driver as a module, choose M here: the
module will be called goodix.
+config TOUCHSCREEN_GOODIX_BERLIN_CORE
+ depends on GPIOLIB || COMPILE_TEST
+ depends on REGMAP
+ tristate
+
config TOUCHSCREEN_HIDEEP
tristate "HiDeep Touch IC"
depends on I2C
diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
index 159cd5136fdb..29cdb042e104 100644
--- a/drivers/input/touchscreen/Makefile
+++ b/drivers/input/touchscreen/Makefile
@@ -47,6 +47,7 @@ obj-$(CONFIG_TOUCHSCREEN_EGALAX_SERIAL) += egalax_ts_serial.o
obj-$(CONFIG_TOUCHSCREEN_EXC3000) += exc3000.o
obj-$(CONFIG_TOUCHSCREEN_FUJITSU) += fujitsu_ts.o
obj-$(CONFIG_TOUCHSCREEN_GOODIX) += goodix_ts.o
+obj-$(CONFIG_TOUCHSCREEN_GOODIX_BERLIN_CORE) += goodix_berlin_core.o
obj-$(CONFIG_TOUCHSCREEN_HIDEEP) += hideep.o
obj-$(CONFIG_TOUCHSCREEN_HYNITRON_CSTXXX) += hynitron_cstxxx.o
obj-$(CONFIG_TOUCHSCREEN_ILI210X) += ili210x.o
diff --git a/drivers/input/touchscreen/goodix_berlin.h b/drivers/input/touchscreen/goodix_berlin.h
new file mode 100644
index 000000000000..235f44947a28
--- /dev/null
+++ b/drivers/input/touchscreen/goodix_berlin.h
@@ -0,0 +1,159 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * Goodix Touchscreen Driver
+ * Copyright (C) 2020 - 2021 Goodix, Inc.
+ * Copyright (C) 2023 Linaro Ltd.
+ *
+ * Based on goodix_berlin_berlin driver.
+ */
+
+#ifndef __GOODIX_BERLIN_H_
+#define __GOODIX_BERLIN_H_
+
+#include <linux/gpio/consumer.h>
+#include <linux/input.h>
+#include <linux/input/touchscreen.h>
+#include <linux/regulator/consumer.h>
+#include <linux/sizes.h>
+
+#define GOODIX_BERLIN_MAX_TOUCH 10
+
+#define GOODIX_BERLIN_NORMAL_RESET_DELAY_MS 100
+
+#define GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN 8
+#define GOODIX_BERLIN_STATUS_OFFSET 0
+#define GOODIX_BERLIN_REQUEST_TYPE_OFFSET 2
+
+#define GOODIX_BERLIN_BYTES_PER_POINT 8
+#define GOODIX_BERLIN_COOR_DATA_CHECKSUM_SIZE 2
+#define GOODIX_BERLIN_COOR_DATA_CHECKSUM_MASK GENMASK(15, 0)
+
+/* Read n finger events */
+#define GOODIX_BERLIN_IRQ_READ_LEN(n) (GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN + \
+ (GOODIX_BERLIN_BYTES_PER_POINT * (n)) + \
+ GOODIX_BERLIN_COOR_DATA_CHECKSUM_SIZE)
+
+#define GOODIX_BERLIN_TOUCH_EVENT BIT(7)
+#define GOODIX_BERLIN_REQUEST_EVENT BIT(6)
+#define GOODIX_BERLIN_TOUCH_COUNT_MASK GENMASK(3, 0)
+
+#define GOODIX_BERLIN_REQUEST_CODE_RESET 3
+
+#define GOODIX_BERLIN_POINT_TYPE_MASK GENMASK(3, 0)
+#define GOODIX_BERLIN_POINT_TYPE_STYLUS_HOVER 1
+#define GOODIX_BERLIN_POINT_TYPE_STYLUS 3
+
+#define GOODIX_BERLIN_TOUCH_ID_MASK GENMASK(7, 4)
+
+#define GOODIX_BERLIN_DEV_CONFIRM_VAL 0xAA
+#define GOODIX_BERLIN_BOOTOPTION_ADDR 0x10000
+#define GOODIX_BERLIN_FW_VERSION_INFO_ADDR 0x10014
+
+#define GOODIX_BERLIN_IC_INFO_MAX_LEN SZ_1K
+#define GOODIX_BERLIN_IC_INFO_ADDR 0x10070
+
+struct goodix_berlin_fw_version {
+ u8 rom_pid[6];
+ u8 rom_vid[3];
+ u8 rom_vid_reserved;
+ u8 patch_pid[8];
+ u8 patch_vid[4];
+ u8 patch_vid_reserved;
+ u8 sensor_id;
+ u8 reserved[2];
+ __le16 checksum;
+} __packed;
+
+struct goodix_berlin_ic_info_version {
+ u8 info_customer_id;
+ u8 info_version_id;
+ u8 ic_die_id;
+ u8 ic_version_id;
+ __le32 config_id;
+ u8 config_version;
+ u8 frame_data_customer_id;
+ u8 frame_data_version_id;
+ u8 touch_data_customer_id;
+ u8 touch_data_version_id;
+ u8 reserved[3];
+} __packed;
+
+struct goodix_berlin_ic_info_feature {
+ __le16 freqhop_feature;
+ __le16 calibration_feature;
+ __le16 gesture_feature;
+ __le16 side_touch_feature;
+ __le16 stylus_feature;
+} __packed;
+
+struct goodix_berlin_ic_info_misc {
+ __le32 cmd_addr;
+ __le16 cmd_max_len;
+ __le32 cmd_reply_addr;
+ __le16 cmd_reply_len;
+ __le32 fw_state_addr;
+ __le16 fw_state_len;
+ __le32 fw_buffer_addr;
+ __le16 fw_buffer_max_len;
+ __le32 frame_data_addr;
+ __le16 frame_data_head_len;
+ __le16 fw_attr_len;
+ __le16 fw_log_len;
+ u8 pack_max_num;
+ u8 pack_compress_version;
+ __le16 stylus_struct_len;
+ __le16 mutual_struct_len;
+ __le16 self_struct_len;
+ __le16 noise_struct_len;
+ __le32 touch_data_addr;
+ __le16 touch_data_head_len;
+ __le16 point_struct_len;
+ __le16 reserved1;
+ __le16 reserved2;
+ __le32 mutual_rawdata_addr;
+ __le32 mutual_diffdata_addr;
+ __le32 mutual_refdata_addr;
+ __le32 self_rawdata_addr;
+ __le32 self_diffdata_addr;
+ __le32 self_refdata_addr;
+ __le32 iq_rawdata_addr;
+ __le32 iq_refdata_addr;
+ __le32 im_rawdata_addr;
+ __le16 im_readata_len;
+ __le32 noise_rawdata_addr;
+ __le16 noise_rawdata_len;
+ __le32 stylus_rawdata_addr;
+ __le16 stylus_rawdata_len;
+ __le32 noise_data_addr;
+ __le32 esd_addr;
+} __packed;
+
+struct goodix_berlin_touch_data {
+ u8 id;
+ u8 unused;
+ __le16 x;
+ __le16 y;
+ __le16 w;
+} __packed;
+
+struct goodix_berlin_core {
+ struct device *dev;
+ struct regmap *regmap;
+ struct regulator *avdd;
+ struct regulator *iovdd;
+ struct gpio_desc *reset_gpio;
+ struct touchscreen_properties props;
+ struct goodix_berlin_fw_version fw_version;
+ struct input_dev *input_dev;
+ int irq;
+
+ /* Runtime parameters extracted from IC_INFO buffer */
+ u32 touch_data_addr;
+};
+
+int goodix_berlin_probe(struct device *dev, int irq, const struct input_id *id,
+ struct regmap *regmap);
+
+extern const struct dev_pm_ops goodix_berlin_pm_ops;
+
+#endif
diff --git a/drivers/input/touchscreen/goodix_berlin_core.c b/drivers/input/touchscreen/goodix_berlin_core.c
new file mode 100644
index 000000000000..af3e73bbb3ec
--- /dev/null
+++ b/drivers/input/touchscreen/goodix_berlin_core.c
@@ -0,0 +1,584 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Goodix Touchscreen Driver
+ * Copyright (C) 2020 - 2021 Goodix, Inc.
+ * Copyright (C) 2023 Linaro Ltd.
+ *
+ * Based on goodix_ts_berlin driver.
+ */
+#include <asm/unaligned.h>
+#include <linux/input/mt.h>
+#include <linux/input/touchscreen.h>
+#include <linux/regmap.h>
+
+#include "goodix_berlin.h"
+
+/*
+ * Goodix "Berlin" Touchscreen ID driver
+ *
+ * This driver is distinct from goodix.c since hardware interface
+ * is different enough to require a new driver.
+ * None of the register address or data structure are close enough
+ * to the previous generations.
+ *
+ * Currently only handles Multitouch events with already
+ * programmed firmware and "config" for "Revision D" Berlin IC.
+ *
+ * Support is missing for:
+ * - ESD Management
+ * - Firmware update/flashing
+ * - "Config" update/flashing
+ * - Stylus Events
+ * - Gesture Events
+ * - Support for older revisions (A & B)
+ */
+
+static bool goodix_berlin_checksum_valid(const u8 *data, int size)
+{
+ u32 cal_checksum = 0;
+ u16 r_checksum;
+ u32 i;
+
+ if (size < GOODIX_BERLIN_COOR_DATA_CHECKSUM_SIZE)
+ return false;
+
+ for (i = 0; i < size - GOODIX_BERLIN_COOR_DATA_CHECKSUM_SIZE; i++)
+ cal_checksum += data[i];
+
+ r_checksum = get_unaligned_le16(&data[i]);
+
+ return FIELD_GET(GOODIX_BERLIN_COOR_DATA_CHECKSUM_MASK, cal_checksum) == r_checksum;
+}
+
+static bool goodix_berlin_is_dummy_data(struct goodix_berlin_core *cd,
+ const u8 *data, int size)
+{
+ int i;
+
+ /*
+ * If the device is missing or doesn't respond the buffer
+ * could be filled with bus default line state, 0x00 or 0xff,
+ * so declare success the first time we encounter neither.
+ */
+ for (i = 0; i < size; i++)
+ if (data[i] > 0 && data[i] < 0xff)
+ return false;
+
+ return true;
+}
+
+static int goodix_berlin_dev_confirm(struct goodix_berlin_core *cd)
+{
+ u8 tx_buf[8], rx_buf[8];
+ int retry = 3;
+ int error;
+
+ memset(tx_buf, GOODIX_BERLIN_DEV_CONFIRM_VAL, sizeof(tx_buf));
+ while (retry--) {
+ error = regmap_raw_write(cd->regmap, GOODIX_BERLIN_BOOTOPTION_ADDR, tx_buf,
+ sizeof(tx_buf));
+ if (error)
+ return error;
+
+ error = regmap_raw_read(cd->regmap, GOODIX_BERLIN_BOOTOPTION_ADDR, rx_buf,
+ sizeof(rx_buf));
+ if (error)
+ return error;
+
+ if (!memcmp(tx_buf, rx_buf, sizeof(tx_buf)))
+ return 0;
+
+ usleep_range(5000, 5100);
+ }
+
+ dev_err(cd->dev, "device confirm failed, rx_buf: %*ph\n", 8, rx_buf);
+
+ return -EINVAL;
+}
+
+static int goodix_berlin_power_on(struct goodix_berlin_core *cd, bool on)
+{
+ int error = 0;
+
+ if (on) {
+ error = regulator_enable(cd->iovdd);
+ if (error) {
+ dev_err(cd->dev, "Failed to enable iovdd: %d\n", error);
+ return error;
+ }
+
+ /* Vendor waits 3ms for IOVDD to settle */
+ usleep_range(3000, 3100);
+
+ error = regulator_enable(cd->avdd);
+ if (error) {
+ dev_err(cd->dev, "Failed to enable avdd: %d\n", error);
+ goto error_avdd_regulator;
+ }
+
+ /* Vendor waits 15ms for IOVDD to settle */
+ usleep_range(15000, 15100);
+
+ gpiod_set_value(cd->reset_gpio, 0);
+
+ /* Vendor waits 4ms for Firmware to initialize */
+ usleep_range(4000, 4100);
+
+ error = goodix_berlin_dev_confirm(cd);
+ if (error)
+ goto error_dev_confirm;
+
+ /* Vendor waits 100ms for Firmware to fully boot */
+ msleep(GOODIX_BERLIN_NORMAL_RESET_DELAY_MS);
+
+ return 0;
+ }
+
+error_dev_confirm:
+ gpiod_set_value(cd->reset_gpio, 1);
+
+ regulator_disable(cd->avdd);
+
+error_avdd_regulator:
+ regulator_disable(cd->iovdd);
+
+ return error;
+}
+
+static int goodix_berlin_read_version(struct goodix_berlin_core *cd)
+{
+ u8 buf[sizeof(struct goodix_berlin_fw_version)];
+ int error;
+
+ error = regmap_raw_read(cd->regmap, GOODIX_BERLIN_FW_VERSION_INFO_ADDR, buf, sizeof(buf));
+ if (error) {
+ dev_err(cd->dev, "error reading fw version\n");
+ return error;
+ }
+
+ if (!goodix_berlin_checksum_valid(buf, sizeof(buf))) {
+ dev_err(cd->dev, "invalid fw version: checksum error\n");
+ return -EINVAL;
+ }
+
+ memcpy(&cd->fw_version, buf, sizeof(cd->fw_version));
+
+ return 0;
+}
+
+/* Only extract necessary data for runtime */
+static int goodix_berlin_convert_ic_info(struct goodix_berlin_core *cd,
+ const u8 *data, u16 length)
+{
+ struct goodix_berlin_ic_info_misc misc;
+ unsigned int offset = 0;
+ u8 param_num;
+
+ offset += sizeof(__le16); /* length */
+ offset += sizeof(struct goodix_berlin_ic_info_version);
+ offset += sizeof(struct goodix_berlin_ic_info_feature);
+
+ /* IC_INFO Parameters, variable width structure */
+ offset += 4 * sizeof(u8); /* drv_num, sen_num, button_num, force_num */
+
+ if (offset >= length)
+ goto invalid_offset;
+
+ param_num = data[offset++]; /* active_scan_rate_num */
+
+ offset += param_num * sizeof(__le16);
+
+ if (offset >= length)
+ goto invalid_offset;
+
+ param_num = data[offset++]; /* mutual_freq_num*/
+
+ offset += param_num * sizeof(__le16);
+
+ if (offset >= length)
+ goto invalid_offset;
+
+ param_num = data[offset++]; /* self_tx_freq_num */
+
+ offset += param_num * sizeof(__le16);
+
+ if (offset >= length)
+ goto invalid_offset;
+
+ param_num = data[offset++]; /* self_rx_freq_num */
+
+ offset += param_num * sizeof(__le16);
+
+ if (offset >= length)
+ goto invalid_offset;
+
+ param_num = data[offset++]; /* stylus_freq_num */
+
+ offset += param_num * sizeof(__le16);
+
+ if (offset + sizeof(misc) > length)
+ goto invalid_offset;
+
+ /* goodix_berlin_ic_info_misc */
+ memcpy(&misc, &data[offset], sizeof(misc));
+
+ cd->touch_data_addr = le32_to_cpu(misc.touch_data_addr);
+
+ return 0;
+
+invalid_offset:
+ dev_err(cd->dev, "ic_info length is invalid (offset %d length %d)\n",
+ offset, length);
+ return -EINVAL;
+}
+
+static int goodix_berlin_get_ic_info(struct goodix_berlin_core *cd)
+{
+ u8 afe_data[GOODIX_BERLIN_IC_INFO_MAX_LEN];
+ __le16 length_raw;
+ u16 length;
+ int error;
+
+ error = regmap_raw_read(cd->regmap, GOODIX_BERLIN_IC_INFO_ADDR,
+ &length_raw, sizeof(length_raw));
+ if (error) {
+ dev_info(cd->dev, "failed get ic info length, %d\n", error);
+ return error;
+ }
+
+ length = le16_to_cpu(length_raw);
+ if (length >= GOODIX_BERLIN_IC_INFO_MAX_LEN) {
+ dev_info(cd->dev, "invalid ic info length %d\n", length);
+ return -EINVAL;
+ }
+
+ error = regmap_raw_read(cd->regmap, GOODIX_BERLIN_IC_INFO_ADDR,
+ afe_data, length);
+ if (error) {
+ dev_info(cd->dev, "failed get ic info data, %d\n", error);
+ return error;
+ }
+
+ /* check whether the data is valid (ex. bus default values) */
+ if (goodix_berlin_is_dummy_data(cd, (const uint8_t *)afe_data, length)) {
+ dev_err(cd->dev, "fw info data invalid\n");
+ return -EINVAL;
+ }
+
+ if (!goodix_berlin_checksum_valid((const uint8_t *)afe_data, length)) {
+ dev_info(cd->dev, "fw info checksum error\n");
+ return -EINVAL;
+ }
+
+ error = goodix_berlin_convert_ic_info(cd, afe_data, length);
+ if (error) {
+ dev_err(cd->dev, "error converting ic info\n");
+ return error;
+ }
+
+ /* check some key info */
+ if (!cd->touch_data_addr) {
+ dev_err(cd->dev, "touch_data_addr is null\n");
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static void goodix_berlin_parse_finger(struct goodix_berlin_core *cd,
+ const void *buf, int touch_num)
+{
+ const struct goodix_berlin_touch_data *touch_data = buf;
+ int i;
+
+ /* Check for data validity */
+ for (i = 0; i < touch_num; i++) {
+ unsigned int id;
+
+ id = FIELD_GET(GOODIX_BERLIN_TOUCH_ID_MASK, touch_data[i].id);
+
+ if (id >= GOODIX_BERLIN_MAX_TOUCH) {
+ dev_warn(cd->dev, "invalid finger id %d\n", id);
+ return;
+ }
+ }
+
+ /* Report finger touches */
+ for (i = 0; i < touch_num; i++) {
+ input_mt_slot(cd->input_dev,
+ FIELD_GET(GOODIX_BERLIN_TOUCH_ID_MASK,
+ touch_data[i].id));
+ input_mt_report_slot_state(cd->input_dev, MT_TOOL_FINGER, true);
+
+ touchscreen_report_pos(cd->input_dev, &cd->props,
+ __le16_to_cpu(touch_data[i].x),
+ __le16_to_cpu(touch_data[i].y),
+ true);
+ input_report_abs(cd->input_dev, ABS_MT_TOUCH_MAJOR,
+ __le16_to_cpu(touch_data[i].w));
+ }
+
+ input_mt_sync_frame(cd->input_dev);
+ input_sync(cd->input_dev);
+}
+
+static void goodix_berlin_touch_handler(struct goodix_berlin_core *cd,
+ const void *pre_buf, u32 pre_buf_len)
+{
+ static u8 buffer[GOODIX_BERLIN_IRQ_READ_LEN(GOODIX_BERLIN_MAX_TOUCH)];
+ u8 point_type = 0;
+ u8 touch_num = 0;
+ int error = 0;
+
+ /* copy pre-data to buffer */
+ memcpy(buffer, pre_buf, pre_buf_len);
+
+ touch_num = FIELD_GET(GOODIX_BERLIN_TOUCH_COUNT_MASK,
+ buffer[GOODIX_BERLIN_REQUEST_TYPE_OFFSET]);
+
+ if (touch_num > GOODIX_BERLIN_MAX_TOUCH) {
+ dev_warn(cd->dev, "invalid touch num %d\n", touch_num);
+ return;
+ }
+
+ if (touch_num) {
+ /* read more data if more than 2 touch events */
+ if (unlikely(touch_num > 2)) {
+ error = regmap_raw_read(cd->regmap,
+ cd->touch_data_addr + pre_buf_len,
+ &buffer[pre_buf_len],
+ (touch_num - 2) * GOODIX_BERLIN_BYTES_PER_POINT);
+ if (error) {
+ dev_err_ratelimited(cd->dev, "failed get touch data\n");
+ return;
+ }
+ }
+
+ point_type = FIELD_GET(GOODIX_BERLIN_POINT_TYPE_MASK,
+ buffer[GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN]);
+
+ if (point_type == GOODIX_BERLIN_POINT_TYPE_STYLUS ||
+ point_type == GOODIX_BERLIN_POINT_TYPE_STYLUS_HOVER) {
+ dev_warn_once(cd->dev, "Stylus event type not handled\n");
+ return;
+ }
+
+ if (!goodix_berlin_checksum_valid(&buffer[GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN],
+ touch_num * GOODIX_BERLIN_BYTES_PER_POINT + 2)) {
+ dev_dbg(cd->dev, "touch data checksum error\n");
+ dev_dbg(cd->dev, "data: %*ph\n",
+ touch_num * GOODIX_BERLIN_BYTES_PER_POINT + 2,
+ &buffer[GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN]);
+ return;
+ }
+ }
+
+ goodix_berlin_parse_finger(cd, &buffer[GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN],
+ touch_num);
+}
+
+static int goodix_berlin_request_handle_reset(struct goodix_berlin_core *cd)
+{
+ gpiod_set_value(cd->reset_gpio, 1);
+ usleep_range(2000, 2100);
+ gpiod_set_value(cd->reset_gpio, 0);
+
+ msleep(GOODIX_BERLIN_NORMAL_RESET_DELAY_MS);
+
+ return 0;
+}
+
+static irqreturn_t goodix_berlin_threadirq_func(int irq, void *data)
+{
+ struct goodix_berlin_core *cd = data;
+ u8 buf[GOODIX_BERLIN_IRQ_READ_LEN(2)];
+ u8 event_status;
+ int error;
+
+ /* First, read buffer with space for 2 touch events */
+ error = regmap_raw_read(cd->regmap, cd->touch_data_addr, buf,
+ GOODIX_BERLIN_IRQ_READ_LEN(2));
+ if (error) {
+ dev_err_ratelimited(cd->dev, "failed get event head data\n");
+ return IRQ_HANDLED;
+ }
+
+ if (buf[GOODIX_BERLIN_STATUS_OFFSET] == 0)
+ return IRQ_HANDLED;
+
+ if (!goodix_berlin_checksum_valid(buf, GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN)) {
+ dev_warn_ratelimited(cd->dev, "touch head checksum err : %*ph\n",
+ GOODIX_BERLIN_IRQ_EVENT_HEAD_LEN, buf);
+ return IRQ_HANDLED;
+ }
+
+ event_status = buf[GOODIX_BERLIN_STATUS_OFFSET];
+
+ if (event_status & GOODIX_BERLIN_TOUCH_EVENT)
+ goodix_berlin_touch_handler(cd, buf, GOODIX_BERLIN_IRQ_READ_LEN(2));
+
+ if (event_status & GOODIX_BERLIN_REQUEST_EVENT) {
+ switch (buf[GOODIX_BERLIN_REQUEST_TYPE_OFFSET]) {
+ case GOODIX_BERLIN_REQUEST_CODE_RESET:
+ goodix_berlin_request_handle_reset(cd);
+ break;
+
+ default:
+ dev_warn(cd->dev, "unsupported request code 0x%x\n",
+ buf[GOODIX_BERLIN_REQUEST_TYPE_OFFSET]);
+ }
+ }
+
+ /* Clear up status field */
+ regmap_write(cd->regmap, cd->touch_data_addr, 0);
+
+ return IRQ_HANDLED;
+}
+
+static int goodix_berlin_input_dev_config(struct goodix_berlin_core *cd,
+ const struct input_id *id)
+{
+ struct input_dev *input_dev;
+ int error;
+
+ input_dev = devm_input_allocate_device(cd->dev);
+ if (!input_dev)
+ return -ENOMEM;
+
+ cd->input_dev = input_dev;
+ input_set_drvdata(input_dev, cd);
+
+ input_dev->name = "Goodix Berlin Capacitive TouchScreen";
+ input_dev->phys = "input/ts";
+
+ input_dev->id = *id;
+
+ input_set_abs_params(cd->input_dev, ABS_MT_POSITION_X, 0, SZ_64K - 1, 0, 0);
+ input_set_abs_params(cd->input_dev, ABS_MT_POSITION_Y, 0, SZ_64K - 1, 0, 0);
+ input_set_abs_params(cd->input_dev, ABS_MT_TOUCH_MAJOR, 0, 255, 0, 0);
+
+ touchscreen_parse_properties(cd->input_dev, true, &cd->props);
+
+ error = input_mt_init_slots(cd->input_dev, GOODIX_BERLIN_MAX_TOUCH,
+ INPUT_MT_DIRECT | INPUT_MT_DROP_UNUSED);
+ if (error)
+ return error;
+
+ return input_register_device(cd->input_dev);
+}
+
+static int goodix_berlin_pm_suspend(struct device *dev)
+{
+ struct goodix_berlin_core *cd = dev_get_drvdata(dev);
+
+ disable_irq(cd->irq);
+
+ return goodix_berlin_power_on(cd, false);
+}
+
+static int goodix_berlin_pm_resume(struct device *dev)
+{
+ struct goodix_berlin_core *cd = dev_get_drvdata(dev);
+ int error;
+
+ error = goodix_berlin_power_on(cd, true);
+ if (error)
+ return error;
+
+ enable_irq(cd->irq);
+
+ return 0;
+}
+
+EXPORT_GPL_SIMPLE_DEV_PM_OPS(goodix_berlin_pm_ops,
+ goodix_berlin_pm_suspend,
+ goodix_berlin_pm_resume);
+
+static void goodix_berlin_power_off(void *data)
+{
+ struct goodix_berlin_core *cd = data;
+
+ goodix_berlin_power_on(cd, false);
+}
+
+int goodix_berlin_probe(struct device *dev, int irq, const struct input_id *id,
+ struct regmap *regmap)
+{
+ struct goodix_berlin_core *cd;
+ int error;
+
+ if (irq <= 0) {
+ dev_err(dev, "Missing interrupt number\n");
+ return -EINVAL;
+ }
+
+ cd = devm_kzalloc(dev, sizeof(*cd), GFP_KERNEL);
+ if (!cd)
+ return -ENOMEM;
+
+ cd->dev = dev;
+ cd->regmap = regmap;
+ cd->irq = irq;
+
+ cd->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
+ if (IS_ERR(cd->reset_gpio))
+ return dev_err_probe(dev, PTR_ERR(cd->reset_gpio),
+ "Failed to request reset gpio\n");
+
+ cd->avdd = devm_regulator_get(dev, "avdd");
+ if (IS_ERR(cd->avdd))
+ return dev_err_probe(dev, PTR_ERR(cd->avdd),
+ "Failed to request avdd regulator\n");
+
+ cd->iovdd = devm_regulator_get(dev, "iovdd");
+ if (IS_ERR(cd->iovdd))
+ return dev_err_probe(dev, PTR_ERR(cd->iovdd),
+ "Failed to request iovdd regulator\n");
+
+ error = goodix_berlin_power_on(cd, true);
+ if (error) {
+ dev_err(dev, "failed power on");
+ return error;
+ }
+
+ error = devm_add_action_or_reset(dev, goodix_berlin_power_off, cd);
+ if (error)
+ return error;
+
+ error = goodix_berlin_read_version(cd);
+ if (error) {
+ dev_err(dev, "failed to get version info");
+ return error;
+ }
+
+ error = goodix_berlin_get_ic_info(cd);
+ if (error) {
+ dev_err(dev, "invalid ic info, abort");
+ return error;
+ }
+
+ error = goodix_berlin_input_dev_config(cd, id);
+ if (error) {
+ dev_err(dev, "failed set input device");
+ return error;
+ }
+
+ error = devm_request_threaded_irq(dev, irq, NULL,
+ goodix_berlin_threadirq_func,
+ IRQF_ONESHOT, "goodix-berlin", cd);
+ if (error) {
+ dev_err(dev, "request threaded irq failed: %d\n", error);
+ return error;
+ }
+
+ dev_set_drvdata(dev, cd);
+
+ dev_dbg(dev, "Goodix Berlin %s Touchscreen Controller", cd->fw_version.patch_pid);
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(goodix_berlin_probe);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Goodix Berlin Core Touchscreen driver");
+MODULE_AUTHOR("Neil Armstrong <neil.armstrong@linaro.org>");
--
2.34.1
^ permalink raw reply related
* [PATCH v3 0/4] input: touchscreen: add initial support for Goodix Berlin touchscreen IC
From: Neil Armstrong @ 2023-06-22 14:28 UTC (permalink / raw)
To: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Bastien Nocera, Hans de Goede, Henrik Rydberg, Jeff LaBundy
Cc: linux-input, linux-arm-msm, devicetree, linux-kernel,
Neil Armstrong, Rob Herring
These touchscreen ICs support SPI, I2C and I3C interface, up to
10 finger touch, stylus and gestures events.
This initial driver is derived from the Goodix goodix_ts_berlin
available at [1] and [2] and only supports the GT9916 IC
present on the Qualcomm SM8550 MTP & QRD touch panel.
The current implementation only supports BerlinD, aka GT9916.
Support for advanced features like:
- Firmware & config update
- Stylus events
- Gestures events
- Previous revisions support (BerlinA or BerlinB)
is not included in current version.
The current support will work with currently flashed firmware
and config, and bail out if firmware or config aren't flashed yet.
[1] https://github.com/goodix/goodix_ts_berlin
[2] https://git.codelinaro.org/clo/la/platform/vendor/opensource/touch-drivers
Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
---
Changes in v3:
- Another guge cleanups after Jeff's review:
- appended goodix_berlin_ before all defines
- removed some unused defines
- removed retries on most of read functions, can be added back later
- added __le to ic_info structures
- reworked and simplified irq handling, dropped enum and ts_event structs
- added struct for touch data
- simplified and cleaned goodix_berlin_check_checksum & goodix_berlin_is_dummy_data
- moved touch_data_addr to the end of the main code_data
- reworked probe to get_irq last and right before setip input device
- cleaned probe by removing the "cd->dev"
- added short paragraph to justify new driver for berlin devices
- defined all offsets & masks
- Added bindings review tag
- Link to v2: https://lore.kernel.org/r/20230606-topic-goodix-berlin-upstream-initial-v2-0-26bc8fe1e90e@linaro.org
Changes in v2:
- Huge cleanups after Jeff's review:
- switch to error instead of ret
- drop dummy vendor/product ids
- drop unused defined/enums
- drop unused ic_info and only keep needes values
- cleanup namings and use goodix_berlin_ everywhere
- fix regulator setup
- fix default variables value when assigned afterwars
- removed indirections
- dropped debugfs
- cleaned input_dev setup
- dropped _remove()
- sync'ed i2c and spi drivers
- fixed yaml bindings
- Link to v1: https://lore.kernel.org/r/20230606-topic-goodix-berlin-upstream-initial-v1-0-4a0741b8aefd@linaro.org
---
Neil Armstrong (4):
dt-bindings: input: document Goodix Berlin Touchscreen IC
input: touchscreen: add core support for Goodix Berlin Touchscreen IC
input: touchscreen: add I2C support for Goodix Berlin Touchscreen IC
input: touchscreen: add SPI support for Goodix Berlin Touchscreen IC
.../bindings/input/touchscreen/goodix,gt9916.yaml | 95 ++++
drivers/input/touchscreen/Kconfig | 32 ++
drivers/input/touchscreen/Makefile | 3 +
drivers/input/touchscreen/goodix_berlin.h | 159 ++++++
drivers/input/touchscreen/goodix_berlin_core.c | 584 +++++++++++++++++++++
drivers/input/touchscreen/goodix_berlin_i2c.c | 69 +++
drivers/input/touchscreen/goodix_berlin_spi.c | 172 ++++++
7 files changed, 1114 insertions(+)
---
base-commit: 6db29e14f4fb7bce9eb5290288e71b05c2b0d118
change-id: 20230606-topic-goodix-berlin-upstream-initial-ba97e8ec8f4c
Best regards,
--
Neil Armstrong <neil.armstrong@linaro.org>
^ permalink raw reply
* [PATCH v3 1/4] dt-bindings: input: document Goodix Berlin Touchscreen IC
From: Neil Armstrong @ 2023-06-22 14:28 UTC (permalink / raw)
To: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Bastien Nocera, Hans de Goede, Henrik Rydberg, Jeff LaBundy
Cc: linux-input, linux-arm-msm, devicetree, linux-kernel,
Neil Armstrong, Rob Herring
In-Reply-To: <20230606-topic-goodix-berlin-upstream-initial-v3-0-f0577cead709@linaro.org>
Document the Goodix GT9916 wich is part of the "Berlin" serie
of Touchscreen controllers IC from Goodix.
Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
---
.../bindings/input/touchscreen/goodix,gt9916.yaml | 95 ++++++++++++++++++++++
1 file changed, 95 insertions(+)
diff --git a/Documentation/devicetree/bindings/input/touchscreen/goodix,gt9916.yaml b/Documentation/devicetree/bindings/input/touchscreen/goodix,gt9916.yaml
new file mode 100644
index 000000000000..d90f045ac06c
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/touchscreen/goodix,gt9916.yaml
@@ -0,0 +1,95 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/input/touchscreen/goodix,gt9916.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Goodix Berlin series touchscreen controller
+
+description: The Goodix Berlin series of touchscreen controllers
+ be connected to either I2C or SPI buses.
+
+maintainers:
+ - Neil Armstrong <neil.armstrong@linaro.org>
+
+allOf:
+ - $ref: touchscreen.yaml#
+ - $ref: /schemas/spi/spi-peripheral-props.yaml#
+
+properties:
+ compatible:
+ enum:
+ - goodix,gt9916
+
+ reg:
+ maxItems: 1
+
+ interrupts:
+ maxItems: 1
+
+ reset-gpios:
+ maxItems: 1
+
+ avdd-supply:
+ description: Analog power supply regulator on AVDD pin
+
+ vddio-supply:
+ description: power supply regulator on VDDIO pin
+
+ spi-max-frequency: true
+ touchscreen-inverted-x: true
+ touchscreen-inverted-y: true
+ touchscreen-size-x: true
+ touchscreen-size-y: true
+ touchscreen-swapped-x-y: true
+
+additionalProperties: false
+
+required:
+ - compatible
+ - reg
+ - interrupts
+ - avdd-supply
+ - touchscreen-size-x
+ - touchscreen-size-y
+
+examples:
+ - |
+ #include <dt-bindings/interrupt-controller/irq.h>
+ #include <dt-bindings/gpio/gpio.h>
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ touchscreen@5d {
+ compatible = "goodix,gt9916";
+ reg = <0x5d>;
+ interrupt-parent = <&gpio>;
+ interrupts = <25 IRQ_TYPE_LEVEL_LOW>;
+ reset-gpios = <&gpio1 1 GPIO_ACTIVE_LOW>;
+ avdd-supply = <&ts_avdd>;
+ touchscreen-size-x = <1024>;
+ touchscreen-size-y = <768>;
+ };
+ };
+ - |
+ #include <dt-bindings/interrupt-controller/irq.h>
+ #include <dt-bindings/gpio/gpio.h>
+ spi {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ num-cs = <1>;
+ cs-gpios = <&gpio 2 GPIO_ACTIVE_HIGH>;
+ touchscreen@0 {
+ compatible = "goodix,gt9916";
+ reg = <0>;
+ interrupt-parent = <&gpio>;
+ interrupts = <25 IRQ_TYPE_LEVEL_LOW>;
+ reset-gpios = <&gpio1 1 GPIO_ACTIVE_LOW>;
+ avdd-supply = <&ts_avdd>;
+ spi-max-frequency = <1000000>;
+ touchscreen-size-x = <1024>;
+ touchscreen-size-y = <768>;
+ };
+ };
+
+...
--
2.34.1
^ permalink raw reply related
* Re: [PATCH v2 1/5] dt-bindings: mmc: fsl-imx-esdhc: Add imx6ul support
From: Ulf Hansson @ 2023-06-22 9:11 UTC (permalink / raw)
To: Oleksij Rempel
Cc: Abel Vesa, Michael Turquette, Stephen Boyd, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Shawn Guo, Sascha Hauer,
Herbert Xu, David S. Miller, Dmitry Torokhov, kernel, Peng Fan,
Fabio Estevam, NXP Linux Team, Daniel Lezcano, Thomas Gleixner,
Michael Trimarchi, Mark Brown, Dario Binacchi, Marek Vasut,
linux-clk, devicetree, linux-arm-kernel, linux-kernel,
linux-crypto, linux-input, linux-mmc
In-Reply-To: <20230621093245.78130-2-o.rempel@pengutronix.de>
On Wed, 21 Jun 2023 at 11:32, Oleksij Rempel <o.rempel@pengutronix.de> wrote:
>
> Add the 'fsl,imx6ul-usdhc' value to the compatible properties list in
> the fsl-imx-esdhc.yaml file. This is required to match the compatible
> strings present in the 'mmc@2190000' node of 'imx6ul-prti6g.dtb'. This
> commit addresses the following dtbs_check warning:
> imx6ul-prti6g.dtb:0:0: /soc/bus@2100000/mmc@2190000: failed to match any schema with compatible: ['fsl,imx6ul-usdhc', 'fsl,imx6sx-usdhc']
>
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
Applied for next, thanks!
Kind regards
Uffe
> ---
> Documentation/devicetree/bindings/mmc/fsl-imx-esdhc.yaml | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/Documentation/devicetree/bindings/mmc/fsl-imx-esdhc.yaml b/Documentation/devicetree/bindings/mmc/fsl-imx-esdhc.yaml
> index fbfd822b92707..82eb7a24c8578 100644
> --- a/Documentation/devicetree/bindings/mmc/fsl-imx-esdhc.yaml
> +++ b/Documentation/devicetree/bindings/mmc/fsl-imx-esdhc.yaml
> @@ -42,6 +42,7 @@ properties:
> - enum:
> - fsl,imx6sll-usdhc
> - fsl,imx6ull-usdhc
> + - fsl,imx6ul-usdhc
> - const: fsl,imx6sx-usdhc
> - items:
> - const: fsl,imx7d-usdhc
> --
> 2.39.2
>
^ permalink raw reply
* Re: Clarification about the hidraw documentation on HIDIOCGFEATURE
From: Xiaofan Chen @ 2023-06-22 9:08 UTC (permalink / raw)
To: Benjamin Tissoires; +Cc: Jiri Kosina, Ihor Dutchak, linux-input
In-Reply-To: <1f24c83c-fe36-d2ab-c755-e83fc6a265eb@redhat.com>
On Wed, Jun 21, 2023 at 11:05 PM Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
>
> On Wed, Jun 21, 2023 at 1:27 PM Xiaofan Chen <xiaofanc@gmail.com> wrote:
> >
> > On Tue, Jun 20, 2023 at 7:28 AM Xiaofan Chen <xiaofanc@gmail.com> wrote:
> > >
> > > On Mon, Jun 19, 2023 at 11:14 PM Xiaofan Chen <xiaofanc@gmail.com> wrote:
> > > >
> > > > On Mon, Jun 19, 2023 at 2:09 PM Xiaofan Chen <xiaofanc@gmail.com> wrote:
> > > > >
> > > > > I know that thurrent documentation has been there since it was created by
> > > > > Alan Ott many years ago. And he started the HIDAPI project around that
> > > > > time as well. However, I am starting to doubt whether it is correct or not
> > > > > based on the testing using HIDAPI.
> > > > >
> > > > > Please help to clarify. Thanks.
> > > > >
> > > > > https://docs.kernel.org/hid/hidraw.html
> > > > > +++++++++++++++++++++++++++++++++++++++++++++++++++++
> > > > > HIDIOCGFEATURE(len):
> > > > >
> > > > > Get a Feature Report
> > > > >
> > > > > This ioctl will request a feature report from the device using the
> > > > > control endpoint. The first byte of the supplied buffer should be
> > > > > set to the report number of the requested report. For devices
> > > > > which do not use numbered reports, set the first byte to 0. The
> > > > > returned report buffer will contain the report number in the first
> > > > > byte, followed by the report data read from the device. For devices
> > > > > which do not use numbered reports, the report data will begin at the
> > > > > first byte of the returned buffer.
>
>
> Yep, this is wrong.
>
> This should be read:
> ```
> The returned report buffer will contain the report number in the first
> byte or 0 when the device doesn't use numbered reports. The data read
> from the device will be stored in the following bytes.
> ```
>
> FWIW, the difficulty to find out what the code does is because this part
> is handled in each HID transport driver: USB, Bluetooth, I2C.
>
> Looking at drivers/hid/usbhid/hid-core.c, lines 869+, the function
> usbhid_get_raw_report() is the one used by hidraw in the end and is
> still the original code from Alan:
>
> ```
> /* Byte 0 is the report number. Report data starts at byte 1.*/
> buf[0] = report_number;
> if (report_number == 0x0) {
> /* Offset the return buffer by 1, so that the report ID
> will remain in byte 0. */
> buf++;
> count--;
> skipped_report_id = 1;
> }
> ```
>
> > Hi Jiri and Benjamin,
> >
> > Sorry to ping the two maintainers, hopefully you two can give the
> > answer. Thanks.
>
> See above, I also think the documentation is wrong.
>
Thanks a lot for the confirmation.
On the same note, I think it is the same for HIDIOCGINPUT as well, but there
is no need to update the documentation once the documentation is fixed
for HIDIOCGFEATURE.
++++++++++++++
HIDIOCGINPUT(len):
Get an Input Report
This ioctl will request an input report from the device using the
control endpoint. This
is slower on most devices where a dedicated In endpoint exists for
regular input reports,
but allows the host to request the value of a specific report number.
Typically, this is
used to request the initial states of an input report of a device,
before an application
listens for normal reports via the regular device read() interface.
The format of the
buffer issued with this report is identical to that of HIDIOCGFEATURE.
++++++++++++++
--
Xiaofan
^ permalink raw reply
* Re: [PATCH 1/1] HID: Add introduction about HID for non-kernel programmers
From: Peter Hutterer @ 2023-06-22 7:59 UTC (permalink / raw)
To: Marco Morandini
Cc: Jiri Kosina, Benjamin Tissoires, linux-input, Jonathan Corbet,
linux-doc
In-Reply-To: <99a679e8-6b01-6805-1e33-ce02485e0063@polimi.it>
First: standing ovations for adding better documentation to the kernel.
Second: my condolences that you had to learn how hid report descriptors
work :)
A few typos and suggested rewordings below, feel free to take those as
you see fit. For a more high-level comment: it's a very fine (and
blurry) line that you are treading here because we really shouldn't
provide too much information and instead refer readers to the official
spec. The doc here should ideally provide just enough high-level
overview that the reader roughly understands what report descriptors and
reports are, and why the parsers (and quirks) are necessary.
I think for that particular purpose, the examples are probably the most
useful parts anyway because they provide a tangible association of bits
to functionality. Try to avoid getting stuck in the minutes of the HID
specs, it just doesn't matter for the scope of this document.
On Tue, Jun 20, 2023 at 02:33:55PM +0200, Marco Morandini wrote:
> This patch adds an introduction about HID
> meant for the casual programmers that is trying
> either to fix his device or to understand what
> is going wrong
> ---
> Documentation/hid/hidintro.rst | 558 +++++++++++++++++++++++++++++++++
> Documentation/hid/index.rst | 1 +
> 2 files changed, 559 insertions(+)
> create mode 100644 Documentation/hid/hidintro.rst
>
> diff --git a/Documentation/hid/hidintro.rst b/Documentation/hid/hidintro.rst
> new file mode 100644
> index 000000000000..96afb8d807a6
> --- /dev/null
> +++ b/Documentation/hid/hidintro.rst
> @@ -0,0 +1,558 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +======================================
> +Introduction to HID report descriptors
> +======================================
> +
> +This chapter is meant to give a broad overview
> +of what HID report descriptors are, and of how a casual (non kernel)
> +programmer can deal with an HID device that
> +is not working well with Linux.
shameless plug! :) I wrote this post a few years ago, feel free to
incorporate bits from there into here
https://who-t.blogspot.com/2018/12/understanding-hid-report-descriptors.html
> +
> +.. contents::
> + :local:
> + :depth: 2
> +
> +
> +Introduction
> +============
> +
> +HID stands for Human Interface Device, and can be whatever device
> +you are using to interact with a computer, be it a mouse,
> +a touchpad, a tablet, a microphone.
> +
> +Many HID work out the box, even if their hardware is different.
See Benjamin's comment too, I think it's better to use "HID device"
here, primarily because we have the "HID protocol", "HID parser", "HID
quirks", so it's more a qualifier than what the acronym actually stands
for.
> +For example, mouses can have a different number of buttons; they
s/a different/any/ - otherwise it's "different to what?"
> +can have (or not) a wheel; their movement sensitivity can be
I'd use "may have a wheel" but it's correct this way too
> +significantly different, and so on. Nonetheless,
maybe something like "movement sensitivity differs between different
models"
> +most of the time everything just works, without the need
> +to have specialized code in the kernel for any mouse model
s/any/every/
> +developed since 1970.
> +
> +This is because most (if not all) of the modern HIDs do advertise
> +the number and type of signal that can be exchanged, and
> +describe how such signal are exchanged with the computer
> +by means of *HID events*.
> +This is done through the *HID report descriptor*, basically a bunch of numbers
> +that allow the operating system to understand that the mouse at hand
> +has (say) five rather than three buttons.
I think the above lines are at risk of confusing newbies, how about
something like this:
This is because modern HID devices do advertise
their capabilities through the *HID report descriptor*, a
fixed set of bytes describing exactly what *HID reports*
may be sent between the device and the host and the meaning of each
individual bit in those reports. For example, a HID Report Descriptor
may specify that "in a report with ID 3 the second byte is the delta x
coordinate of a mouse".
The HID report itself then merely carries the actual data values
without any extra meta information. Note that HID reports may be sent
from the device ("Input Reports", i.e. input events), to the device
("Output Reports" to e.g. change LEDs) or used for device
configuration ("Feature reports"). A device may support one or more
HID reports.
> +The HID subsystem is in charge of parsing the HID report descriptors,
> +and of converts HID events into normal input
s/of//
> +device interfaces (see Documentation/hid/hiddev.rst).
> +Whenever something does not work as it should this can be
> +because the HID report descriptor provided by the device is wrong,
"Devices may misbehave because the HID report descriptor ..."
> +or because it needs to be dealt with in a special way,
> +or because the some special device or interaction mode
> +is not handled by the default code.
> +
> +The format of HID report descriptors is described by two documents,
> +available from the `USB Implementers Forum <https://www.usb.org/>`_
> +at `this <https://www.usb.org/hid>`_ addresses:
> +
> + * the `HID USB Device Class Definition <https://www.usb.org/document-library/device-class-definition-hid-111>`_ (HIDUDC from now on)
Benjamin may have more opinion on that but to me this was only ever "the
HID spec" :) The term "HIDUDC" also doesn't provide anything useful in
the search engines so anyone reading over this bit and skipping ahead
will be punished for it.
> + * the `HID Usage Tables <https://usb.org/document-library/hid-usage-tables-14>`_ (HIDUT from now on)
can we name this "HUT" please? because that's the only term I've ever
seen referred it to other than as HID Usage Tables.
> +
> +This does not means that the HID subsystem can deal with USB devices only;
s/means/mean/
> +rather, different transport drivers, such as I2C or Bluetooth, can be dealt
> +with, see Documentation/hid/hid-transport.rst.
> +
> +Parsing an HID descriptor
> +=========================
I'm pretty sure that most people just say "hid" like a word rather than
spelling out H-I-D, so this would mean it would be "a HID descriptor".
In practise, this particular nasty corner of the english language can be
avoided by always calling them the plural HID report descriptors and
thus avoiding the windmill battles of "a" vs "an".
Also, I'd rather you alway use "report descriptor" over "descriptor"
since that is the full term and non-ambiguous.
> +
> +The current list of HID devices can be found at ``/sys/bus/hid/devices/``.
> +For each device, say ``/sys/bus/hid/devices/0003\:093A\:2510.0002/``,
> +one can read the corresponding report descriptor::
> +
> + marco@sun:~> hexdump -C /sys/bus/hid/devices/0003\:093A\:2510.0002/report_descriptor
> + 00000000 05 01 09 02 a1 01 09 01 a1 00 05 09 19 01 29 03 |..............).|
> + 00000010 15 00 25 01 75 01 95 03 81 02 75 05 95 01 81 01 |..%.u.....u.....|
> + 00000020 05 01 09 30 09 31 09 38 15 81 25 7f 75 08 95 03 |...0.1.8..%.u...|
> + 00000030 81 06 c0 c0 |....|
> + 00000034
> +
> +Alternatively, the HID report descriptor can be read by accessig the hidraw
typo: accessing
> +driver, see Documentation/output/hid/hidraw.rst and
> +file samples/hidraw/hid-example.c for a simple example.
> +The output of ``hid-example`` would be, for the same device::
> +
> + marco@sun:~> sudo ./hid-example
> + Report Descriptor Size: 52
> + Report Descriptor:
> + 5 1 9 2 a1 1 9 1 a1 0 5 9 19 1 29 3 15 0 25 1 75 1 95 3 81 2 75 5 95 1 81 1 5 1 9 30 9 31 9 38 15 81 25 7f 75 8 95 3 81 6 c0 c0
> +
> + Raw Name: PixArt USB Optical Mouse
> + Raw Phys: usb-0000:05:00.4-2.3/input0
> + Raw Info:
> + bustype: 3 (USB)
> + vendor: 0x093a
> + product: 0x2510
> + HIDIOCSFEATURE: Broken pipe
> + HIDIOCGFEATURE: Broken pipe
> + Error: 32
> + write: Broken pipe
> + read: Resource temporarily unavailable
I don't think you need to include the error messages.
Also: you now already have 2 tools to look at hid and then you use two
more later (hidrrd and hid-tools). I'd say it's best to just stick to
one tool to reduce the mental load of having to map different tool
outputs to what is the same base data.
> +
> +The basic structure of a HID report descriptor is defined in the HIDUDC manual, while
> +HIDUT "defines constants that can be interpreted by an application to
> +identify the purpose and meaning of a data
> +field in a HID report". Each entry is defined by at least two bytes,
> +where the first one defines what type of value is following,
> +and is described in the HIDUDC manual,
> +and the second one carries the actual value,
> +and is described in the HIDUT manual.
> +
> +Let consider the first number, 0x05: according to
s/let/let's/
> +HIDUDC, Sec. 6.2.2.2, "Short Items"
> +
> + * the first least significant two bits
> + define the number of the following data bytes (either 0, 1, 2 or 4
> + for the values 0, 1, 2 or 3, respectively)
> + * the second least significant two bits identify the type of the item:
> +
> + * 0: ``Main``
> + * 1: ``Global``
> + * 2: ``Local``
> + * 3: ``Reserved``
> + * the remaining four bits give a numeric expression specifying
> + the function of the item (see below);
> +
> +This means that ``0x05`` (i.e. ``00000101``) stands for
> +1 byte of data to be read, and Global type.
> +Since we are dealing with a Global item the meaning
> +of the most significant four bits
> +is defines in HIDUC manual, Sec. 6.2.2.7, "Global Items".
typo with "HIDUC"
> +The bits are ``0000``, thus we have a ``Usage Page``.
I'm guessing you *don't* want to explain the full "how bit's being parsed"
so a simpler approach would be a basic byte diagram with references
to the official spec:
Then you get something like:
"""
Let's consider the first number, 0x05 which carries 2 bits for the
length of the item, 2 bits for the type of the item and 4 bits for the
function:
+----------+
| 00000101 |
+----------+
^^
---- Length of data (see UDC 6.2.2.2)
^^
------ Type of the item (see UDC 6.2.2.7)
^^^^
--------- Function of the item (see HUT Sec 3)
In our case, the length is 1 byte, the type a "Global" and the function
is "Usage Page", thus we need to refer to HUT Sec 3 which indicates that
the value 0x01 in the second byte stands for ``Generic Desktop Page``.
"""
> +
> +
> +The second number is the actual data, and its meaning can
> +be found in the HICUT manual.
> +We have an ``Usage Page``, thus we need to refer to HICUT Sec. 3,
> +"Usage Pages"; from there, it is clear that the ``0x01``
> +stands for a ``Generic Desktop Page``.
note "Generic Desktop Page", you can skip the "a" here
> +
> +Moving now to the second two bytes, and following the same scheme, ``0x09``
> +(i.e. ``00001001``) will be followed by one byte (``01``)
> +and is a ``Local`` item.
> +Thus, the meaning of the remaining four bits (``0000``)
> +is given in HIDUDC Sec. 6.2.2.8 "Local Items", so that we have an ``Usage``.
> +
> +In this way the HID report descriptor can be painstakingly
> +parsed, byte by byte. In practice you need not to do this,
> +and almost no one does
> +this: everyone resorts to specialized parsers. Among all the available ones
I would be harsher here: "In practice you should not do this, use an
existing parser" because the people who first learn about HID here
should probably not immediately write a parser (mostly fo their own
sanity :)
> +
> + * the online `USB Descriptor and Request Parser
> + <http://eleccelerator.com/usbdescreqparser/>`_;
> + * `hidrdd <https://github.com/abend0c1/hidrdd>`_,
> + that provides very detailed and somewhat verbose descriptions
> + (verbosity can be useful if you are not familiar with HID report descriptors);
> + * `hid-tools <https://gitlab.freedesktop.org/libevdev/hid-tools>`_,
> + a complete utility set that allows, among other things,
> + to record and replay the raw HID reports and to debug
> + and replay HID devices.
> + It is being actively developed by the Linux HID subsystem mantainers.
> +
> +.. Parsing the mouse HID report descriptor with `hidrdd <https://github.com/abend0c1/hidrdd>`_ one gets::
> +
> + marco@sun:~> rexx ./rd.rex -x -d --hex 05 01 09 02 a1 01 09 01 a1 00 05 09 19 01 29 03 15 00 25 01 75 01 95 03 81 02 75 05 95 01 81 01 05 01 09 30 09 31 09 38 15 81 25 7f 75 08 95 03 81 06 c0 c0
> +
> + //--------------------------------------------------------------------------------
> + // Report descriptor data in hex (length 52 bytes)
> + //--------------------------------------------------------------------------------
> +
> +
> + // 05010902 A1010901 A1000509 19012903 15002501 75019503 81027505 95018101
> + // 05010930 09310938 1581257F 75089503 8106C0C0
> +
> +
> + //--------------------------------------------------------------------------------
> + // Decoded Application Collection
> + //--------------------------------------------------------------------------------
> +
> + /*
> + 05 01 (GLOBAL) USAGE_PAGE 0x0001 Generic Desktop Page
> + 09 02 (LOCAL) USAGE 0x00010002 Mouse (Application Collection)
> + A1 01 (MAIN) COLLECTION 0x01 Application (Usage=0x00010002: Page=Generic Desktop Page, Usage=Mouse, Type=Application Collection)
> + 09 01 (LOCAL) USAGE 0x00010001 Pointer (Physical Collection)
> + A1 00 (MAIN) COLLECTION 0x00 Physical (Usage=0x00010001: Page=Generic Desktop Page, Usage=Pointer, Type=Physical Collection)
> + 05 09 (GLOBAL) USAGE_PAGE 0x0009 Button Page
> + 19 01 (LOCAL) USAGE_MINIMUM 0x00090001 Button 1 Primary/trigger (Selector, On/Off Control, Momentary Control, or One Shot Control)
> + 29 03 (LOCAL) USAGE_MAXIMUM 0x00090003 Button 3 Tertiary (Selector, On/Off Control, Momentary Control, or One Shot Control)
> + 15 00 (GLOBAL) LOGICAL_MINIMUM 0x00 (0) <-- Info: Consider replacing 15 00 with 14
> + 25 01 (GLOBAL) LOGICAL_MAXIMUM 0x01 (1)
> + 75 01 (GLOBAL) REPORT_SIZE 0x01 (1) Number of bits per field
> + 95 03 (GLOBAL) REPORT_COUNT 0x03 (3) Number of fields
> + 81 02 (MAIN) INPUT 0x00000002 (3 fields x 1 bit) 0=Data 1=Variable 0=Absolute 0=NoWrap 0=Linear 0=PrefState 0=NoNull 0=NonVolatile 0=Bitmap
> + 75 05 (GLOBAL) REPORT_SIZE 0x05 (5) Number of bits per field
> + 95 01 (GLOBAL) REPORT_COUNT 0x01 (1) Number of fields
> + 81 01 (MAIN) INPUT 0x00000001 (1 field x 5 bits) 1=Constant 0=Array 0=Absolute
> + 05 01 (GLOBAL) USAGE_PAGE 0x0001 Generic Desktop Page
> + 09 30 (LOCAL) USAGE 0x00010030 X (Dynamic Value)
> + 09 31 (LOCAL) USAGE 0x00010031 Y (Dynamic Value)
> + 09 38 (LOCAL) USAGE 0x00010038 Wheel (Dynamic Value)
> + 15 81 (GLOBAL) LOGICAL_MINIMUM 0x81 (-127)
> + 25 7F (GLOBAL) LOGICAL_MAXIMUM 0x7F (127)
> + 75 08 (GLOBAL) REPORT_SIZE 0x08 (8) Number of bits per field
> + 95 03 (GLOBAL) REPORT_COUNT 0x03 (3) Number of fields
> + 81 06 (MAIN) INPUT 0x00000006 (3 fields x 8 bits) 0=Data 1=Variable 1=Relative 0=NoWrap 0=Linear 0=PrefState 0=NoNull 0=NonVolatile 0=Bitmap
> + C0 (MAIN) END_COLLECTION Physical
> + C0 (MAIN) END_COLLECTION Application
> + */
> +
> + // All structure fields should be byte-aligned...
> + #pragma pack(push,1)
> +
> + //--------------------------------------------------------------------------------
> + // Button Page inputReport (Device --> Host)
> + //--------------------------------------------------------------------------------
> +
> + typedef struct
> + {
> + // No REPORT ID byte
> + // Collection: CA:Mouse CP:Pointer
> + uint8_t BTN_MousePointerButton1 : 1; // Usage 0x00090001: Button 1 Primary/trigger, Value = 0 to 1
> + uint8_t BTN_MousePointerButton2 : 1; // Usage 0x00090002: Button 2 Secondary, Value = 0 to 1
> + uint8_t BTN_MousePointerButton3 : 1; // Usage 0x00090003: Button 3 Tertiary, Value = 0 to 1
> + uint8_t : 5; // Pad
> + int8_t GD_MousePointerX; // Usage 0x00010030: X, Value = -127 to 127
> + int8_t GD_MousePointerY; // Usage 0x00010031: Y, Value = -127 to 127
> + int8_t GD_MousePointerWheel; // Usage 0x00010038: Wheel, Value = -127 to 127
> + } inputReport_t;
> +
> + #pragma pack(pop)
> +
> +Parsing the mouse HID report descriptor with `hid-tools <https://gitlab.freedesktop.org/libevdev/hid-tools>`_ leads to::
> +
> + marco@sun:~/Programmi/linux/hid-tools (master =)> ./hid-decode /sys/bus/hid/devices/0003\:093A\:2510.0002/report_descriptor
> + # device 0:0
> + # 0x05, 0x01, // Usage Page (Generic Desktop) 0
> + # 0x09, 0x02, // Usage (Mouse) 2
> + # 0xa1, 0x01, // Collection (Application) 4
> + # 0x09, 0x01, // Usage (Pointer) 6
> + # 0xa1, 0x00, // Collection (Physical) 8
> + # 0x05, 0x09, // Usage Page (Button) 10
> + # 0x19, 0x01, // Usage Minimum (1) 12
> + # 0x29, 0x03, // Usage Maximum (3) 14
> + # 0x15, 0x00, // Logical Minimum (0) 16
> + # 0x25, 0x01, // Logical Maximum (1) 18
> + # 0x75, 0x01, // Report Size (1) 20
> + # 0x95, 0x03, // Report Count (3) 22
> + # 0x81, 0x02, // Input (Data,Var,Abs) 24
> + # 0x75, 0x05, // Report Size (5) 26
> + # 0x95, 0x01, // Report Count (1) 28
> + # 0x81, 0x01, // Input (Cnst,Arr,Abs) 30
> + # 0x05, 0x01, // Usage Page (Generic Desktop) 32
> + # 0x09, 0x30, // Usage (X) 34
> + # 0x09, 0x31, // Usage (Y) 36
> + # 0x09, 0x38, // Usage (Wheel) 38
> + # 0x15, 0x81, // Logical Minimum (-127) 40
> + # 0x25, 0x7f, // Logical Maximum (127) 42
> + # 0x75, 0x08, // Report Size (8) 44
> + # 0x95, 0x03, // Report Count (3) 46
> + # 0x81, 0x06, // Input (Data,Var,Rel) 48
> + # 0xc0, // End Collection 50
> + # 0xc0, // End Collection 51
> + #
> + R: 52 05 01 09 02 a1 01 09 01 a1 00 05 09 19 01 29 03 15 00 25 01 75 01 95 03 81 02 75 05 95 01 81 01 05 01 09 30 09 31 09 38 15 81 25 7f 75 08 95 03 81 06 c0 c0
> + N: device 0:0
> + I: 3 0001 0001
Benjamin already said this but: pick one tool and use its output, there
is no need to show different tools being different. Since Benjamin is
both the kernel maintainer and the hid-tools maintainer, that one should
be the favourite ;)
> +
> +From it we undesratnd that
typo
> +
> + * the mouse has three (from ``Usage Minimum (1)`` to
> + ``Usage Maximum (3)``) buttons (``Usage Page (Button)``);
> + * buttons can take values ranging from ``0`` to ``1``;
> + (from ``Logical Minimum (0)`` to ``Logical Maximum (1)``);
so, remembering my early HID learnings - if you just throw out "Usage
Minimum" and "Maximum" that is mostly meaningless unless one also reads
the definition of what those mean. For me it took me a while to wrap my
head around the term "Usage" to begin with, it not really being an
english word that you'd encounter every day.
I think it'll be more understandable annotating the button line by line
with layperson's terms - anything specific needs to be externally looked
up anyway. So you get something like this:
"""
> + # 0x05, 0x09, // Usage Page (Button) 10
what follows is a button
> + # 0x19, 0x01, // Usage Minimum (1) 12
> + # 0x29, 0x03, // Usage Maximum (3) 14
first button is button number 1, last button is button number 3
> + # 0x15, 0x00, // Logical Minimum (0) 16
> + # 0x25, 0x01, // Logical Maximum (1) 18
each button can send values from 0 up to including 1 (i.e. they're
binary buttons)
> + # 0x75, 0x01, // Report Size (1) 20
each button is sent as exactly one bit
> + # 0x95, 0x03, // Report Count (3) 22
and there are three of those bits (matching our three buttons)
> + # 0x81, 0x02, // Input (Data,Var,Abs) 24
it's actual Data (not constant padding), they represent a single
variable (Var) and the value are Absolute (not relative).
See HIDUDC Sec. 6.2.2.5 "Input, Output, and Feature Items.
"""
> + * information is encoded into three bits: one bit has
> + ``Report Size (1)``,
> + but there are three of them since ``Report Count (3)``;
> + * the value of these bits can change
> + (``Data`` in ``Input (Data,Var,Abs)``);
> + * each field represents data from a physical control;
> + * the number of bits reserved for each field is determined
> + by the preceding ``Report Size``/``Report Count``
> + items (``Var`` in ``Input (Data,Var,Abs)``);
tbh, I think that's a mischaracterization of Var vs Arr (and tbh the
exact detail what those mean doesn't really matter here anyway).
> + * the data is *absolute* (i.e it does not represent the
> + change from the last report, ``Abs`` in ``Input (Data,Var,Abs)``).
> +
> +The meaning of the ``Input``
> +items is explained in HIDUDC Sec. 6.2.2.5 "Input, Output, and Feature Items.
> +
> +There are five additional padding bits, that are needed
> +to reach a byte: see ``Report Size (5)``, that is
> +repeated only once (``Report Count (1)``).
> +These bits take *constant* values (``Cnst`` in
> +``Input (Cnst,Arr,Abs)``).
> +
> +The mouse has also two physical positions (``Usage (X)``, ``Usage (Y)``) and a wheel
> +(``Usage (Wheel)``).
> +
> +Each of them take values ranging from ``-127`` to ``127``
> +(from ``Logical Minimum (-127)`` to ``Logical Maximum (-127)``),
> +it is represented by eight bits (``Report Size (8)``)
> +and there are three of these set of bits (``Report Count (3)``).
> +
> +This time the data do represent the change from the previous configuration
> +(``Rel`` in ``Input (Data,Var,Rel)``).
> +
> +All in all, the mouse input will be transmitted using four bytes:
I would say "the Report Descriptor tells us that mouse input will be..."
because I think that makes it a bit more obvious
> +the first one for the buttons (three bits used, five for padding),
> +the last three for the mouse X, Y and wheel changes, respectively.
> +
> +Indeed, for any event, the mouse will send a *report* of four bytes.
> +We can easily check the values sent by resorting e.g.
s/easily// because it may not be for some users
> +to the `hid-recorder` tool, from `hid-tools
> +<https://gitlab.freedesktop.org/libevdev/hid-tools>`_:
> +The sequence of bytes sent by clicking and releasing
> +button 1, then button 2, then button 3 is::
> +
> + marco@sun:~/> sudo ./hid-recorder /dev/hidraw1
> +
> + ....
> + output of hid-decode
> + ....
> +
> + # Button: 1 0 0 | # | X: 0 | Y: 0 | Wheel: 0
> + E: 000000.000000 4 01 00 00 00
> + # Button: 0 0 0 | # | X: 0 | Y: 0 | Wheel: 0
> + E: 000000.183949 4 00 00 00 00
> + # Button: 0 1 0 | # | X: 0 | Y: 0 | Wheel: 0
> + E: 000001.959698 4 02 00 00 00
> + # Button: 0 0 0 | # | X: 0 | Y: 0 | Wheel: 0
> + E: 000002.103899 4 00 00 00 00
> + # Button: 0 0 1 | # | X: 0 | Y: 0 | Wheel: 0
> + E: 000004.855799 4 04 00 00 00
> + # Button: 0 0 0 | # | X: 0 | Y: 0 | Wheel: 0
> + E: 000005.103864 4 00 00 00 00
> +
> +where it's clear that, for example, when button 2 is clicked
> +the bytes ``02 00 00 00`` are sent, and the immediately subsequent
> +event (``00 00 00 00``) is the release of button 2 (no buttons are pressed,
> +remember that the data is *absolute*).
"where it's clear" can be problematic for readers for whom it's not
clear at all :) In this case you can write
"this example shows that when button 2 is clicked..." to avoid
that issue. I think there are a few uses of that phrase or similar in
this document.
> +
> +If instead one clicks and holds button 1, then clicks and holds button 2,
> +releases button 1, and finally releases button 2, the reports are::
> +
> + # Button: 1 0 0 | # | X: 0 | Y: 0 | Wheel: 0
> + E: 000044.175830 4 01 00 00 00
> + # Button: 1 1 0 | # | X: 0 | Y: 0 | Wheel: 0
> + E: 000045.975997 4 03 00 00 00
> + # Button: 0 1 0 | # | X: 0 | Y: 0 | Wheel: 0
> + E: 000047.407930 4 02 00 00 00
> + # Button: 0 0 0 | # | X: 0 | Y: 0 | Wheel: 0
> + E: 000049.199919 4 00 00 00 00
> +
> +where with ``03 00 00 00`` both buttons are pressed, and with the
> +subsequent ``02 00 00 00`` button 1 is released while button 2 is still
> +active.
> +
> +Outputs and Inputs
> +------------------
> +
> +An HID devices can have inputs, like
I would refer to those as "Input Reports" and "Output Reports", simply
because for me it was much easier to adjust my head to accept that you
can send something to the mouse than that the mouse "has an output".
> +in the mouse example, and outputs.
> +"Output" means that the information is fed
> +from the device to the human; for examples,
> +a joystick with force feedback will have
> +some output.
> +
> +
> +Report IDs and Evdev events
> +===========================
> +
> +A single device can logically group
> +data into different, independent sets.
> +It is *as if* the HID presents
> +itself as different devices, each exchanging
> +its own data. The HID report descriptor is unique,
> +but the different reports are identified by means
> +of different ``Report ID`` fields. Whenever a ``Report ID``
> +is needed it is transmitted as the first byte of any report.
This is a bit ambiguous since it's the collections and applications that
split the device into different sets, the reports are just different
ways of reporting data that belongs to one (or more? not 100% sure) of
those sets then.
And the example below works because you have different collections on
your device here but that's largely orthogonal to the use of reports.
> +
> +Consider the following HID report descriptor::
> +
> + 05 01 09 02 A1 01 85 01 05 09 19 01 29 05 15 00
> + 25 01 95 05 75 01 81 02 95 01 75 03 81 01 05 01
> + 09 30 09 31 16 00 F8 26 FF 07 75 0C 95 02 81 06
> + 09 38 15 80 25 7F 75 08 95 01 81 06 05 0C 0A 38
> + 02 15 80 25 7F 75 08 95 01 81 06 C0 05 01 09 02
> + A1 01 85 02 05 09 19 01 29 05 15 00 25 01 95 05
> + 75 01 81 02 95 01 75 03 81 01 05 01 09 30 09 31
> + 16 00 F8 26 FF 07 75 0C 95 02 81 06 09 38 15 80
> + 25 7F 75 08 95 01 81 06 05 0C 0A 38 02 15 80 25
> + 7F 75 08 95 01 81 06 C0 05 01 09 07 A1 01 85 05
> + 05 07 15 00 25 01 09 29 09 3E 09 4B 09 4E 09 E3
> + 09 E8 09 E8 09 E8 75 01 95 08 81 02 95 00 81 01
> + C0 05 0C 09 01 A1 01 85 06 15 00 25 01 75 01 95
> + 01 09 3F 81 06 09 3F 81 06 09 3F 81 06 09 3F 81
> + 06 09 3F 81 06 09 3F 81 06 09 3F 81 06 09 3F 81
> + 06 C0 05 0C 09 01 A1 01 85 03 09 05 15 00 26 FF
> + 00 75 08 95 02 B1 02 C0
> +
> +After parsing it (try to parse it on your own using
> +the suggested tools!)
> +one can see that the device presents two mouses
> +(Reports IDs 1 and 2, respectively),
> +a Keypad (Report ID 5) and two consumer controls
> +(Report IDs 6 and 3).
> +The data sent for each of these report ids
> +will begin with the Report ID byte, and will be followed
> +by the corresponding information. For example, the
> +report defined for the last consumer
> +control::
> +
> + 0x05, 0x0C, // Usage Page (Consumer)
> + 0x09, 0x01, // Usage (Consumer Control)
> + 0xA1, 0x01, // Collection (Application)
> + 0x85, 0x03, // Report ID (3)
> + 0x09, 0x05, // Usage (Headphone)
> + 0x15, 0x00, // Logical Minimum (0)
> + 0x26, 0xFF, 0x00, // Logical Maximum (255)
> + 0x75, 0x08, // Report Size (8)
> + 0x95, 0x02, // Report Count (2)
> + 0xB1, 0x02, // Feature (Data,Var,Abs,No Wrap,Linear,Preferred State,No Null Position,Non-volatile)
> + 0xC0, // End Collection
> +
> +will be of three bytes: the first for the Report ID (5), the next two
> +for the headphone, with two (``Report Count (2)``) bytes
> +(``Report Size (8)``) each ranging from 0 (``Logical Minimum (0)`` to 255
> +(``Logical Maximum (255)``).
> +
> +
> +Events
> +======
> +
> +One can expect that different ``/dev/input/event*`` are created for different
> +Report IDs.
as Benjamin already mentioned, this is incorrect given how the kernel
works, so definitely needs rewording.
> Going back to the mouse example, and repeating the sequence where
> +one clicks and holds button 1, then clicks and holds button 2,
> +releases button 1, and finally releases button 2, one gets::
> +
> + marco@sun:~> sudo evtest /dev/input/event4
> + Input driver version is 1.0.1
> + Input device ID: bus 0x3 vendor 0x3f0 product 0x94a version 0x111
> + Input device name: "PixArt HP USB Optical Mouse"
> + Supported events:
> + Event type 0 (EV_SYN)
> + Event type 1 (EV_KEY)
> + Event code 272 (BTN_LEFT)
> + Event code 273 (BTN_RIGHT)
> + Event code 274 (BTN_MIDDLE)
> + Event type 2 (EV_REL)
> + Event code 0 (REL_X)
> + Event code 1 (REL_Y)
> + Event code 8 (REL_WHEEL)
> + Event code 11 (REL_WHEEL_HI_RES)
> + Event type 4 (EV_MSC)
> + Event code 4 (MSC_SCAN)
> + Properties:
> + Testing ... (interrupt to exit)
> + Event: time 1687254626.454252, type 4 (EV_MSC), code 4 (MSC_SCAN), value 90001
> + Event: time 1687254626.454252, type 1 (EV_KEY), code 272 (BTN_LEFT), value 1
> + Event: time 1687254626.454252, -------------- SYN_REPORT ------------
> + Event: time 1687254627.342093, type 4 (EV_MSC), code 4 (MSC_SCAN), value 90002
> + Event: time 1687254627.342093, type 1 (EV_KEY), code 273 (BTN_RIGHT), value 1
> + Event: time 1687254627.342093, -------------- SYN_REPORT ------------
> + Event: time 1687254627.974282, type 4 (EV_MSC), code 4 (MSC_SCAN), value 90001
> + Event: time 1687254627.974282, type 1 (EV_KEY), code 272 (BTN_LEFT), value 0
> + Event: time 1687254627.974282, -------------- SYN_REPORT ------------
> + Event: time 1687254628.798240, type 4 (EV_MSC), code 4 (MSC_SCAN), value 90002
> + Event: time 1687254628.798240, type 1 (EV_KEY), code 273 (BTN_RIGHT), value 0
> + Event: time 1687254628.798240, -------------- SYN_REPORT ------------
> +
> +
> +When everything works well
> +==========================
> +
> +* The HID report descriptor makes sense;
> +* It is possible to verify, by reading the raw hid data, that
> + the HID report descriptor *does match* what is sent by the device;
> +* The HID report descriptor does not need any "quirk"s (see later on)
"quirks", can have that inside the quotes
> +* For any Report ID a corresponding ``/dev/input/event*`` is created,
> + and the events there match what you would expect
> +
> +When something does not work
> +============================
you can fold the "everything works" section into here with some prelude
of "There can be a number of reasons for why a device does not behave
correctly, for example..."
> +
> +Sometimes not everything does work as it should.
> +
> +Quirks
> +------
> +
> +A possible reason is that the HID
> +has some common quirk. Should this be the case,
> +it sould be enough to add the required quirk,
> +in the kernel, for the device at hand.
this phrasing is a bit ambiguous, because in the first sentence the
quirks is the bug in the device, but in the second sentence the quirk
is something the kernel provides. You can rephrase this less ambiguously
with something like:
There are some known peculiarities of HID devices that the kernel
knows how to fix - these are called the HID quirks and a list of those
are available in `include/linux/hid.h`.
> +This can be done in file drivers/hid/hid-quirks.c .
> +How to do it should be straightforward after looking into the file.
> +
> +The list of currently defined quirks, from
> +include/linux/hid.h , is
> +
> + * ``HID_QUIRK_NOTOUCH``, defined as ``BIT(1)``:
> + * ``HID_QUIRK_IGNORE``, defined as ``BIT(2)``:
> + * ``HID_QUIRK_NOGET``, defined as ``BIT(3)``:
> + * ``HID_QUIRK_HIDDEV_FORCE``, defined as ``BIT(4)``:
> + * ``HID_QUIRK_BADPAD``, defined as ``BIT(5)``:
> + * ``HID_QUIRK_MULTI_INPUT``, defined as ``BIT(6)``:
> + * ``HID_QUIRK_HIDINPUT_FORCE``, defined as ``BIT(7)``:
> + * ``HID_QUIRK_ALWAYS_POLL``, defined as ``BIT(10)``:
> + * ``HID_QUIRK_INPUT_PER_APP``, defined as ``BIT(11)``:
> + * ``HID_QUIRK_X_INVERT``, defined as ``BIT(12)``:
> + * ``HID_QUIRK_Y_INVERT``, defined as ``BIT(13)``:
> + * ``HID_QUIRK_SKIP_OUTPUT_REPORTS``, defined as ``BIT(16)``:
> + * ``HID_QUIRK_SKIP_OUTPUT_REPORT_ID``, defined as ``BIT(17)``:
> + * ``HID_QUIRK_NO_OUTPUT_REPORTS_ON_INTR_EP``, defined as ``BIT(18)``:
> + * ``HID_QUIRK_HAVE_SPECIAL_DRIVER``, defined as ``BIT(19)``:
> + * ``HID_QUIRK_INCREMENT_USAGE_ON_DUPLICATE``, defined as ``BIT(20)``:
> + * ``HID_QUIRK_FULLSPEED_INTERVAL``, defined as ``BIT(28)``:
> + * ``HID_QUIRK_NO_INIT_REPORTS``, defined as ``BIT(29)``:
> + * ``HID_QUIRK_NO_IGNORE``, defined as ``BIT(30)``:
> + * ``HID_QUIRK_NO_INPUT_SYNC``, defined as ``BIT(31)``:
You *definitely* don't want to include this here because it will get out
of sync (and the actual bit value just doesn't matter anyway). Use one
or two as example just so a reader gets familiar with the
HID_QUIRK_FOOBAR notation and can use grep to find where quirks are
used and go from there.
> +
> +
> +FIXME: ADD A SHORT EXPLANATION FOR EACH QUIRK
as Benjamin already mentioned, this should be documented in situ and
linked to from here rather than split across multiple files.
> +
> +Quirks for USB devices can be specified run-time,
"at runtime"
> +see ``modinfo usbhid``, although the proper fix
> +should go into hid-quirks.c and be submitted upstream.
> +Quirks for other busses need to go into hid-quirks.c
> +
> +Fixing the HID report descriptor
> +--------------------------------
> +
> +Should you need to patch the HID report descriptor
> +the easiest way is to resort to eBPF, as described
> +in Documentation/output/hid/hid-bpf.rst.
> +
> +Basically, you can change any byte of the original report descriptor.
> +The examples in samples/hid should be relatively straightforward,
> +see e.g. samples/hid_mouse.bpf.c::
> +
> + SEC("fmod_ret/hid_bpf_rdesc_fixup")
> + int BPF_PROG(hid_rdesc_fixup, struct hid_bpf_ctx *hctx)
> + {
> + ....
> + data[39] = 0x31;
> + data[41] = 0x30;
> + return 0;
> + }
> +
> +Of course this can be also done within the kernel source
> +code, see e.g. drivers/hid/hid-aureal.c or
> +drivers/hid/hid-samsung.c for a slightly more complex file.
> +
> +
> +
> +Modifying on the fly the transmitted data
> +-----------------------------------------
"Modifying the transmitted data on the fly"
> +
> +It is also possible, always using eBPF, to modify
> +on the fly the data exchanged with the device.
> +See, again, the examples is samples/hid.
Using eBPF it is also possible to modify the data exchanged
with the device. See again the examples in samples/hid
Cheers,
Peter
> +
> +Writing a specialized driver
> +----------------------------
> +
> +This should really be your last resort.
> +
> diff --git a/Documentation/hid/index.rst b/Documentation/hid/index.rst
> index b2028f382f11..af02cf7cfa82 100644
> --- a/Documentation/hid/index.rst
> +++ b/Documentation/hid/index.rst
> @@ -7,6 +7,7 @@ Human Interface Devices (HID)
> .. toctree::
> :maxdepth: 1
>
> + hidintro
> hiddev
> hidraw
> hid-sensor
> --
> 2.41.0
>
>
>
^ permalink raw reply
* [syzbot] [input?] [usb?] KASAN: slab-use-after-free Read in remove_wait_queue
From: syzbot @ 2023-06-22 6:12 UTC (permalink / raw)
To: benjamin.tissoires, jikos, linux-input, linux-kernel, linux-usb,
syzkaller-bugs
Hello,
syzbot found the following issue on:
HEAD commit: 99ec1ed7c2ed Merge tag '6.4-rc6-smb3-server-fixes' of git:..
git tree: upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=14f9cdf3280000
kernel config: https://syzkaller.appspot.com/x/.config?x=2cbd298d0aff1140
dashboard link: https://syzkaller.appspot.com/bug?extid=e0b9084463edf54b83c4
compiler: gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2
Unfortunately, I don't have any reproducer for this issue yet.
Downloadable assets:
disk image: https://storage.googleapis.com/syzbot-assets/8ce811aa7d80/disk-99ec1ed7.raw.xz
vmlinux: https://storage.googleapis.com/syzbot-assets/d2b30638505a/vmlinux-99ec1ed7.xz
kernel image: https://storage.googleapis.com/syzbot-assets/93293b228847/bzImage-99ec1ed7.xz
IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: syzbot+e0b9084463edf54b83c4@syzkaller.appspotmail.com
==================================================================
BUG: KASAN: slab-use-after-free in __lock_acquire+0x41b9/0x5f30 kernel/locking/lockdep.c:4956
Read of size 8 at addr ffff888020c4fd28 by task syz-executor.0/25827
CPU: 1 PID: 25827 Comm: syz-executor.0 Not tainted 6.4.0-rc7-syzkaller-00019-g99ec1ed7c2ed #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 05/27/2023
Call Trace:
<TASK>
__dump_stack lib/dump_stack.c:88 [inline]
dump_stack_lvl+0xd9/0x150 lib/dump_stack.c:106
print_address_description.constprop.0+0x2c/0x3c0 mm/kasan/report.c:351
print_report mm/kasan/report.c:462 [inline]
kasan_report+0x11c/0x130 mm/kasan/report.c:572
__lock_acquire+0x41b9/0x5f30 kernel/locking/lockdep.c:4956
lock_acquire kernel/locking/lockdep.c:5705 [inline]
lock_acquire+0x1b1/0x520 kernel/locking/lockdep.c:5670
__raw_spin_lock_irqsave include/linux/spinlock_api_smp.h:110 [inline]
_raw_spin_lock_irqsave+0x3d/0x60 kernel/locking/spinlock.c:162
remove_wait_queue+0x21/0x180 kernel/sched/wait.c:54
hidraw_read+0x5e8/0xa80 drivers/hid/hidraw.c:74
vfs_read+0x1e1/0x8a0 fs/read_write.c:468
ksys_read+0x12b/0x250 fs/read_write.c:613
do_syscall_x64 arch/x86/entry/common.c:50 [inline]
do_syscall_64+0x39/0xb0 arch/x86/entry/common.c:80
entry_SYSCALL_64_after_hwframe+0x63/0xcd
RIP: 0033:0x7f245ac8c389
Code: 28 00 00 00 75 05 48 83 c4 28 c3 e8 f1 19 00 00 90 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 b8 ff ff ff f7 d8 64 89 01 48
RSP: 002b:00007f245ba30168 EFLAGS: 00000246 ORIG_RAX: 0000000000000000
RAX: ffffffffffffffda RBX: 00007f245adac050 RCX: 00007f245ac8c389
RDX: 00000000200001da RSI: 0000000020000100 RDI: 0000000000000005
RBP: 00007f245acd7493 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000000
R13: 00007f245aecfb1f R14: 00007f245ba30300 R15: 0000000000022000
</TASK>
Allocated by task 4746:
kasan_save_stack+0x22/0x40 mm/kasan/common.c:45
kasan_set_track+0x25/0x30 mm/kasan/common.c:52
____kasan_kmalloc mm/kasan/common.c:374 [inline]
____kasan_kmalloc mm/kasan/common.c:333 [inline]
__kasan_kmalloc+0xa2/0xb0 mm/kasan/common.c:383
kmalloc include/linux/slab.h:559 [inline]
kzalloc include/linux/slab.h:680 [inline]
hidraw_connect+0x4f/0x450 drivers/hid/hidraw.c:545
hid_connect+0x10b8/0x18c0 drivers/hid/hid-core.c:2195
hid_hw_start drivers/hid/hid-core.c:2302 [inline]
hid_hw_start+0xa6/0x130 drivers/hid/hid-core.c:2293
hid_device_probe+0x383/0x3d0 drivers/hid/hid-core.c:2637
call_driver_probe drivers/base/dd.c:579 [inline]
really_probe+0x240/0xca0 drivers/base/dd.c:658
__driver_probe_device+0x1df/0x4b0 drivers/base/dd.c:800
driver_probe_device+0x4c/0x1a0 drivers/base/dd.c:830
__device_attach_driver+0x1d4/0x2e0 drivers/base/dd.c:958
bus_for_each_drv+0x149/0x1d0 drivers/base/bus.c:457
__device_attach+0x1e4/0x4b0 drivers/base/dd.c:1030
bus_probe_device+0x17c/0x1c0 drivers/base/bus.c:532
device_add+0x112d/0x1a40 drivers/base/core.c:3625
hid_add_device+0x377/0xa60 drivers/hid/hid-core.c:2785
usbhid_probe+0xc49/0x1100 drivers/hid/usbhid/hid-core.c:1429
usb_probe_interface+0x30f/0x960 drivers/usb/core/driver.c:396
call_driver_probe drivers/base/dd.c:579 [inline]
really_probe+0x240/0xca0 drivers/base/dd.c:658
__driver_probe_device+0x1df/0x4b0 drivers/base/dd.c:800
driver_probe_device+0x4c/0x1a0 drivers/base/dd.c:830
__device_attach_driver+0x1d4/0x2e0 drivers/base/dd.c:958
bus_for_each_drv+0x149/0x1d0 drivers/base/bus.c:457
__device_attach+0x1e4/0x4b0 drivers/base/dd.c:1030
bus_probe_device+0x17c/0x1c0 drivers/base/bus.c:532
device_add+0x112d/0x1a40 drivers/base/core.c:3625
usb_set_configuration+0x1196/0x1bc0 drivers/usb/core/message.c:2211
usb_generic_driver_probe+0xcf/0x130 drivers/usb/core/generic.c:238
usb_probe_device+0xd8/0x2c0 drivers/usb/core/driver.c:293
call_driver_probe drivers/base/dd.c:579 [inline]
really_probe+0x240/0xca0 drivers/base/dd.c:658
__driver_probe_device+0x1df/0x4b0 drivers/base/dd.c:800
driver_probe_device+0x4c/0x1a0 drivers/base/dd.c:830
__device_attach_driver+0x1d4/0x2e0 drivers/base/dd.c:958
bus_for_each_drv+0x149/0x1d0 drivers/base/bus.c:457
__device_attach+0x1e4/0x4b0 drivers/base/dd.c:1030
bus_probe_device+0x17c/0x1c0 drivers/base/bus.c:532
device_add+0x112d/0x1a40 drivers/base/core.c:3625
usb_new_device+0xcb2/0x19d0 drivers/usb/core/hub.c:2575
hub_port_connect drivers/usb/core/hub.c:5407 [inline]
hub_port_connect_change drivers/usb/core/hub.c:5551 [inline]
port_event drivers/usb/core/hub.c:5711 [inline]
hub_event+0x2d9e/0x4e40 drivers/usb/core/hub.c:5793
process_one_work+0x99a/0x15e0 kernel/workqueue.c:2405
worker_thread+0x67d/0x10c0 kernel/workqueue.c:2552
kthread+0x344/0x440 kernel/kthread.c:379
ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:308
Freed by task 4746:
kasan_save_stack+0x22/0x40 mm/kasan/common.c:45
kasan_set_track+0x25/0x30 mm/kasan/common.c:52
kasan_save_free_info+0x2e/0x40 mm/kasan/generic.c:521
____kasan_slab_free mm/kasan/common.c:236 [inline]
____kasan_slab_free+0x160/0x1c0 mm/kasan/common.c:200
kasan_slab_free include/linux/kasan.h:162 [inline]
slab_free_hook mm/slub.c:1781 [inline]
slab_free_freelist_hook+0x8b/0x1c0 mm/slub.c:1807
slab_free mm/slub.c:3786 [inline]
__kmem_cache_free+0xaf/0x2d0 mm/slub.c:3799
drop_ref+0x28f/0x390 drivers/hid/hidraw.c:335
hidraw_disconnect+0x4c/0x70 drivers/hid/hidraw.c:601
hid_disconnect+0x134/0x1b0 drivers/hid/hid-core.c:2277
hid_hw_stop drivers/hid/hid-core.c:2322 [inline]
hid_device_remove+0x174/0x210 drivers/hid/hid-core.c:2664
device_remove+0xc8/0x170 drivers/base/dd.c:567
__device_release_driver drivers/base/dd.c:1272 [inline]
device_release_driver_internal+0x443/0x610 drivers/base/dd.c:1295
bus_remove_device+0x22c/0x420 drivers/base/bus.c:574
device_del+0x399/0xa30 drivers/base/core.c:3814
hid_remove_device drivers/hid/hid-core.c:2835 [inline]
hid_destroy_device+0xe5/0x150 drivers/hid/hid-core.c:2855
usbhid_disconnect+0xa3/0xe0 drivers/hid/usbhid/hid-core.c:1456
usb_unbind_interface+0x1dc/0x8e0 drivers/usb/core/driver.c:458
device_remove drivers/base/dd.c:569 [inline]
device_remove+0x11f/0x170 drivers/base/dd.c:561
__device_release_driver drivers/base/dd.c:1272 [inline]
device_release_driver_internal+0x443/0x610 drivers/base/dd.c:1295
bus_remove_device+0x22c/0x420 drivers/base/bus.c:574
device_del+0x399/0xa30 drivers/base/core.c:3814
usb_disable_device+0x360/0x7b0 drivers/usb/core/message.c:1420
usb_disconnect+0x2db/0x8a0 drivers/usb/core/hub.c:2238
hub_port_connect drivers/usb/core/hub.c:5246 [inline]
hub_port_connect_change drivers/usb/core/hub.c:5551 [inline]
port_event drivers/usb/core/hub.c:5711 [inline]
hub_event+0x1fbf/0x4e40 drivers/usb/core/hub.c:5793
process_one_work+0x99a/0x15e0 kernel/workqueue.c:2405
worker_thread+0x67d/0x10c0 kernel/workqueue.c:2552
kthread+0x344/0x440 kernel/kthread.c:379
ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:308
The buggy address belongs to the object at ffff888020c4fd00
which belongs to the cache kmalloc-192 of size 192
The buggy address is located 40 bytes inside of
freed 192-byte region [ffff888020c4fd00, ffff888020c4fdc0)
The buggy address belongs to the physical page:
page:ffffea00008313c0 refcount:1 mapcount:0 mapping:0000000000000000 index:0xffff888020c4f700 pfn:0x20c4f
flags: 0xfff00000000200(slab|node=0|zone=1|lastcpupid=0x7ff)
page_type: 0xffffffff()
raw: 00fff00000000200 ffff888012441a00 ffffea0001e0f100 dead000000000002
raw: ffff888020c4f700 000000008010000f 00000001ffffffff 0000000000000000
page dumped because: kasan: bad access detected
page_owner tracks the page as allocated
page last allocated via order 0, migratetype Unmovable, gfp_mask 0x112cc0(GFP_USER|__GFP_NOWARN|__GFP_NORETRY), pid 5055, tgid 5055 (syz-executor.4), ts 451103824327, free_ts 451062516182
set_page_owner include/linux/page_owner.h:31 [inline]
post_alloc_hook+0x2db/0x350 mm/page_alloc.c:1731
prep_new_page mm/page_alloc.c:1738 [inline]
get_page_from_freelist+0xf41/0x2c00 mm/page_alloc.c:3502
__alloc_pages+0x1cb/0x4a0 mm/page_alloc.c:4768
__alloc_pages_node include/linux/gfp.h:237 [inline]
alloc_slab_page mm/slub.c:1853 [inline]
allocate_slab+0xa7/0x390 mm/slub.c:1998
new_slab mm/slub.c:2051 [inline]
___slab_alloc+0xa91/0x1400 mm/slub.c:3192
__slab_alloc.constprop.0+0x56/0xa0 mm/slub.c:3291
__slab_alloc_node mm/slub.c:3344 [inline]
slab_alloc_node mm/slub.c:3441 [inline]
__kmem_cache_alloc_node+0x136/0x320 mm/slub.c:3490
__do_kmalloc_node mm/slab_common.c:965 [inline]
__kmalloc_node+0x51/0x1a0 mm/slab_common.c:973
kmalloc_array_node include/linux/slab.h:657 [inline]
kcalloc_node include/linux/slab.h:662 [inline]
memcg_alloc_slab_cgroups+0x8f/0x150 mm/memcontrol.c:2928
account_slab mm/slab.h:597 [inline]
allocate_slab+0x2d6/0x390 mm/slub.c:2016
new_slab mm/slub.c:2051 [inline]
___slab_alloc+0xa91/0x1400 mm/slub.c:3192
__slab_alloc.constprop.0+0x56/0xa0 mm/slub.c:3291
__slab_alloc_node mm/slub.c:3344 [inline]
slab_alloc_node mm/slub.c:3441 [inline]
slab_alloc mm/slub.c:3459 [inline]
__kmem_cache_alloc_lru mm/slub.c:3466 [inline]
kmem_cache_alloc+0x38e/0x3b0 mm/slub.c:3475
vma_lock_alloc kernel/fork.c:463 [inline]
vm_area_dup+0x55/0x300 kernel/fork.c:516
dup_mmap+0x713/0x19d0 kernel/fork.c:713
dup_mm kernel/fork.c:1692 [inline]
copy_mm kernel/fork.c:1741 [inline]
copy_process+0x6663/0x75c0 kernel/fork.c:2507
page last free stack trace:
reset_page_owner include/linux/page_owner.h:24 [inline]
free_pages_prepare mm/page_alloc.c:1302 [inline]
free_unref_page_prepare+0x62e/0xcb0 mm/page_alloc.c:2564
free_unref_page_list+0xe3/0xa70 mm/page_alloc.c:2705
release_pages+0xcd8/0x1380 mm/swap.c:1042
tlb_batch_pages_flush+0xa8/0x1a0 mm/mmu_gather.c:97
tlb_flush_mmu_free mm/mmu_gather.c:292 [inline]
tlb_flush_mmu mm/mmu_gather.c:299 [inline]
tlb_finish_mmu+0x14b/0x7e0 mm/mmu_gather.c:391
exit_mmap+0x2b2/0x930 mm/mmap.c:3120
__mmput+0x128/0x4c0 kernel/fork.c:1351
mmput+0x60/0x70 kernel/fork.c:1373
exit_mm kernel/exit.c:567 [inline]
do_exit+0x9b0/0x29b0 kernel/exit.c:861
do_group_exit+0xd4/0x2a0 kernel/exit.c:1024
get_signal+0x2318/0x25b0 kernel/signal.c:2876
arch_do_signal_or_restart+0x79/0x5c0 arch/x86/kernel/signal.c:306
exit_to_user_mode_loop kernel/entry/common.c:168 [inline]
exit_to_user_mode_prepare+0x11f/0x240 kernel/entry/common.c:204
__syscall_exit_to_user_mode_work kernel/entry/common.c:286 [inline]
syscall_exit_to_user_mode+0x1d/0x50 kernel/entry/common.c:297
do_syscall_64+0x46/0xb0 arch/x86/entry/common.c:86
entry_SYSCALL_64_after_hwframe+0x63/0xcd
Memory state around the buggy address:
ffff888020c4fc00: fa fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
ffff888020c4fc80: fb fb fb fb fb fb fb fb fc fc fc fc fc fc fc fc
>ffff888020c4fd00: fa fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
^
ffff888020c4fd80: fb fb fb fb fb fb fb fb fc fc fc fc fc fc fc fc
ffff888020c4fe00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
==================================================================
---
This report is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzkaller@googlegroups.com.
syzbot will keep track of this issue. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
If the bug is already fixed, let syzbot know by replying with:
#syz fix: exact-commit-title
If you want to change bug's subsystems, reply with:
#syz set subsystems: new-subsystem
(See the list of subsystem names on the web dashboard)
If the bug is a duplicate of another bug, reply with:
#syz dup: exact-subject-of-another-report
If you want to undo deduplication, reply with:
#syz undup
^ permalink raw reply
* Re: amd_sfh driver causes kernel oops during boot
From: Malte Starostik @ 2023-06-21 23:41 UTC (permalink / raw)
To: Linux regressions mailing list, Benjamin Tissoires,
Limonciello, Mario
Cc: Bagas Sanjaya, basavaraj.natikar, linux-input, linux, stable
In-Reply-To: <602504a5-334d-92e8-2fd3-f7e8662b714e@amd.com>
Am Dienstag, 20. Juni 2023, 22:03:00 CEST schrieb Limonciello, Mario:
> On 6/20/2023 1:50 PM, Limonciello, Mario wrote:
> > Anyways; I just double checked the Z13 I have on my hand. I don't
> > have the PCI device for SFH (1022:164a) present on the system.
> >
> > Can you please double check you are on the latest BIOS?
> >
> > I'm on the latest release from LVFS, 0.1.57 according to fwupdmgr.
I was on 0.1.27 while running the tests.
At least when I saw the errors first, there was no update offered. Haven't re-
checked until now.
> Hopefully the newer BIOS fixes it for you, but if it doesn't I did come
> up with another patch I've sent out that I guess could be another
> solution.
After updating to 0.1.57, it looks like I cannot reproduce the error anymore
either.
> https://lore.kernel.org/linux-input/20230620200117.22261-1-mario.limonciello
> @amd.com/T/#u
I tested your patch before performing the firmware update. Still got the Oops
just like before.
BR Malte
^ permalink raw reply
* Re: [PATCH v2 4/5] dt-bindings: clock: imx6ul: Support optional enet*_ref_pad clocks
From: Conor Dooley @ 2023-06-21 20:56 UTC (permalink / raw)
To: Oleksij Rempel
Cc: Abel Vesa, Michael Turquette, Stephen Boyd, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Shawn Guo, Sascha Hauer,
Herbert Xu, David S. Miller, Dmitry Torokhov, Ulf Hansson, kernel,
Peng Fan, Fabio Estevam, NXP Linux Team, Daniel Lezcano,
Thomas Gleixner, Michael Trimarchi, Mark Brown, Dario Binacchi,
Marek Vasut, linux-clk, devicetree, linux-arm-kernel,
linux-kernel, linux-crypto, linux-input, linux-mmc
In-Reply-To: <20230621093245.78130-5-o.rempel@pengutronix.de>
[-- Attachment #1: Type: text/plain, Size: 1810 bytes --]
On Wed, Jun 21, 2023 at 11:32:44AM +0200, Oleksij Rempel wrote:
> Extend the 'clocks' and 'clock-names' properties to support optional
> 'enet1_ref_pad' and 'enet2_ref_pad' clocks to resolve the following
> dtbs_check warning:
> imx6ul-prti6g.dtb: clock-controller@20c4000: clocks: [[17], [18], [19], [20], [21]] is too long
> imx6ul-prti6g.dtb: clock-controller@20c4000: clock-names: ['ckil', 'osc', 'ipp_di0', 'ipp_di1', 'enet1_ref_pad'] is too long
>
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
> Documentation/devicetree/bindings/clock/imx6ul-clock.yaml | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/clock/imx6ul-clock.yaml b/Documentation/devicetree/bindings/clock/imx6ul-clock.yaml
> index be54d4df5afa2..3b71ebc100bf6 100644
> --- a/Documentation/devicetree/bindings/clock/imx6ul-clock.yaml
> +++ b/Documentation/devicetree/bindings/clock/imx6ul-clock.yaml
> @@ -28,18 +28,24 @@ properties:
> const: 1
>
> clocks:
> + minItems: 4
> items:
> - description: 32k osc
> - description: 24m osc
> - description: ipp_di0 clock input
> - description: ipp_di1 clock input
> + - description: Optional lenet1_ref_pad or enet2_ref_pad clocks
> + - description: Optional lenet1_ref_pad or enet2_ref_pad clocks
^
Is the l intentional?
Otherwise,
Acked-by: Conor Dooley <conor.dooley@microchip.com>
Cheers,
Conor.
>
> clock-names:
> + minItems: 4
> items:
> - const: ckil
> - const: osc
> - const: ipp_di0
> - const: ipp_di1
> + - pattern: '^enet[12]_ref_pad$'
> + - pattern: '^enet[12]_ref_pad$'
>
> required:
> - compatible
> --
> 2.39.2
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH v2] HID: wacom: Use ktime_t rather than int when dealing with timestamps
From: Jason Gerecke @ 2023-06-21 20:52 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: linux-input, linux-kernel, Jiri Kosina, Ping Cheng,
Aaron Armstrong Skomra, Joshua Dickens, Peter Hutterer,
Jason Gerecke, stable
In-Reply-To: <CAO-hwJJC12dRhmykE+P_LBcEJ2G0gHy3Nh1gvWULjdA=4qa-ZQ@mail.gmail.com>
On Wed, Jun 21, 2023 at 9:04 AM Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
>
> On Wed, Jun 21, 2023 at 5:18 PM Jason Gerecke <killertofu@gmail.com> wrote:
> >
> > Following up since no action seems to have been taken on this patch yet.
>
> Sorry, this went through the cracks (I seem to have a lot of cracks recently...)
>
> >
> > On Thu, Jun 8, 2023 at 2:38 PM Jason Gerecke <killertofu@gmail.com> wrote:
> > >
> > > Code which interacts with timestamps needs to use the ktime_t type
> > > returned by functions like ktime_get. The int type does not offer
> > > enough space to store these values, and attempting to use it is a
> > > recipe for problems. In this particular case, overflows would occur
> > > when calculating/storing timestamps leading to incorrect values being
> > > reported to userspace. In some cases these bad timestamps cause input
> > > handling in userspace to appear hung.
>
> I have to ask, is this something we should consider writing a test for? :)
>
Very good point! I'm happy to work on such a test.
Are you open to merging this patch without delay while I write a
testcase? I don't like the idea of this (apparent) system freeze being
affecting users any longer than absolutely necessary.
> Also, you are missing the rev-by from Peter, not sure if the tools
> will pick it up automatically then.
>
> Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>
Oof, good catch. Apologies to Peter :)
Jason
> Cheers,
> Benjamin
>
> > >
> > > Link: https://gitlab.freedesktop.org/libinput/libinput/-/issues/901
> > > Fixes: 17d793f3ed53 ("HID: wacom: insert timestamp to packed Bluetooth (BT) events")
> > > CC: stable@vger.kernel.org
> > > Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com>
> > > ---
> > > v2: Use div_u64 to perform division to deal with ARC and ARM architectures
> > > (as found by the kernel test robot)
> > >
> > > drivers/hid/wacom_wac.c | 6 +++---
> > > drivers/hid/wacom_wac.h | 2 +-
> > > 2 files changed, 4 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
> > > index 2ccf838371343..174bf03908d7c 100644
> > > --- a/drivers/hid/wacom_wac.c
> > > +++ b/drivers/hid/wacom_wac.c
> > > @@ -1314,7 +1314,7 @@ static void wacom_intuos_pro2_bt_pen(struct wacom_wac *wacom)
> > > struct input_dev *pen_input = wacom->pen_input;
> > > unsigned char *data = wacom->data;
> > > int number_of_valid_frames = 0;
> > > - int time_interval = 15000000;
> > > + ktime_t time_interval = 15000000;
> > > ktime_t time_packet_received = ktime_get();
> > > int i;
> > >
> > > @@ -1348,7 +1348,7 @@ static void wacom_intuos_pro2_bt_pen(struct wacom_wac *wacom)
> > > if (number_of_valid_frames) {
> > > if (wacom->hid_data.time_delayed)
> > > time_interval = ktime_get() - wacom->hid_data.time_delayed;
> > > - time_interval /= number_of_valid_frames;
> > > + time_interval = div_u64(time_interval, number_of_valid_frames);
> > > wacom->hid_data.time_delayed = time_packet_received;
> > > }
> > >
> > > @@ -1359,7 +1359,7 @@ static void wacom_intuos_pro2_bt_pen(struct wacom_wac *wacom)
> > > bool range = frame[0] & 0x20;
> > > bool invert = frame[0] & 0x10;
> > > int frames_number_reversed = number_of_valid_frames - i - 1;
> > > - int event_timestamp = time_packet_received - frames_number_reversed * time_interval;
> > > + ktime_t event_timestamp = time_packet_received - frames_number_reversed * time_interval;
> > >
> > > if (!valid)
> > > continue;
> > > diff --git a/drivers/hid/wacom_wac.h b/drivers/hid/wacom_wac.h
> > > index 1a40bb8c5810c..ee21bb260f22f 100644
> > > --- a/drivers/hid/wacom_wac.h
> > > +++ b/drivers/hid/wacom_wac.h
> > > @@ -324,7 +324,7 @@ struct hid_data {
> > > int ps_connected;
> > > bool pad_input_event_flag;
> > > unsigned short sequence_number;
> > > - int time_delayed;
> > > + ktime_t time_delayed;
> > > };
> > >
> > > struct wacom_remote_data {
> > > --
> > > 2.41.0
> > >
> >
>
^ permalink raw reply
* Re: [PATCH v2 3/5] dt-bindings: timer: gpt: Support 3rd clock for i.MX6DL
From: Conor Dooley @ 2023-06-21 20:48 UTC (permalink / raw)
To: Oleksij Rempel
Cc: Abel Vesa, Michael Turquette, Stephen Boyd, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Shawn Guo, Sascha Hauer,
Herbert Xu, David S. Miller, Dmitry Torokhov, Ulf Hansson, kernel,
Peng Fan, Fabio Estevam, NXP Linux Team, Daniel Lezcano,
Thomas Gleixner, Michael Trimarchi, Mark Brown, Dario Binacchi,
Marek Vasut, linux-clk, devicetree, linux-arm-kernel,
linux-kernel, linux-crypto, linux-input, linux-mmc
In-Reply-To: <20230621093245.78130-4-o.rempel@pengutronix.de>
[-- Attachment #1: Type: text/plain, Size: 509 bytes --]
On Wed, Jun 21, 2023 at 11:32:43AM +0200, Oleksij Rempel wrote:
> Add support for a 3rd clock, 'osc_per', for i.MX6DL to the 'fsl,imxgpt'
> binding to resolve the following dtbs_check warning:
> imx6dl-alti6p.dtb: timer@2098000: clocks: [[2, 119], [2, 120], [2, 237]] is too long
> imx6dl-alti6p.dtb: timer@2098000: clock-names: ['ipg', 'per', 'osc_per'] is too long
>
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
Acked-by: Conor Dooley <conor.dooley@microchip.com>
Cheers,
Conor.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH v2 2/5] dt-bindings: timer: gpt: Add i.MX6UL support
From: Conor Dooley @ 2023-06-21 20:47 UTC (permalink / raw)
To: Oleksij Rempel
Cc: Abel Vesa, Michael Turquette, Stephen Boyd, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Shawn Guo, Sascha Hauer,
Herbert Xu, David S. Miller, Dmitry Torokhov, Ulf Hansson, kernel,
Peng Fan, Fabio Estevam, NXP Linux Team, Daniel Lezcano,
Thomas Gleixner, Michael Trimarchi, Mark Brown, Dario Binacchi,
Marek Vasut, linux-clk, devicetree, linux-arm-kernel,
linux-kernel, linux-crypto, linux-input, linux-mmc
In-Reply-To: <20230621093245.78130-3-o.rempel@pengutronix.de>
[-- Attachment #1: Type: text/plain, Size: 1369 bytes --]
Hey Oleksij,
On Wed, Jun 21, 2023 at 11:32:42AM +0200, Oleksij Rempel wrote:
> Add 'fsl,imx6ul-gpt' compatible to resolve the following dtbs_check
> warning:
> imx6ull-jozacp.dtb:0:0: /soc/bus@2000000/timer@2098000: failed to match any schema with compatible: ['fsl,imx6ul-gpt', 'fsl,imx6sx-gpt']
hmm, "imx6ull-jozacp" but the compatible is "imx6ul-gpt".
Is that not incorrect in the first place?
Also, this diff has already made it in - it is in next as 451d69d2f1f9
("dt-bindings: imxgpt: add imx6ul compatible"), applied on 20.05.2023
or similar.
Cheers,
Conor.
>
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
> Documentation/devicetree/bindings/timer/fsl,imxgpt.yaml | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/timer/fsl,imxgpt.yaml b/Documentation/devicetree/bindings/timer/fsl,imxgpt.yaml
> index 716c6afcca1fa..685137338ac99 100644
> --- a/Documentation/devicetree/bindings/timer/fsl,imxgpt.yaml
> +++ b/Documentation/devicetree/bindings/timer/fsl,imxgpt.yaml
> @@ -34,6 +34,9 @@ properties:
> - fsl,imxrt1050-gpt
> - fsl,imxrt1170-gpt
> - const: fsl,imx6dl-gpt
> + - items:
> + - const: fsl,imx6ul-gpt
> + - const: fsl,imx6sx-gpt
>
> reg:
> maxItems: 1
> --
> 2.39.2
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH v2 1/5] dt-bindings: mmc: fsl-imx-esdhc: Add imx6ul support
From: Conor Dooley @ 2023-06-21 20:42 UTC (permalink / raw)
To: Oleksij Rempel
Cc: Abel Vesa, Michael Turquette, Stephen Boyd, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Shawn Guo, Sascha Hauer,
Herbert Xu, David S. Miller, Dmitry Torokhov, Ulf Hansson, kernel,
Peng Fan, Fabio Estevam, NXP Linux Team, Daniel Lezcano,
Thomas Gleixner, Michael Trimarchi, Mark Brown, Dario Binacchi,
Marek Vasut, linux-clk, devicetree, linux-arm-kernel,
linux-kernel, linux-crypto, linux-input, linux-mmc
In-Reply-To: <20230621093245.78130-2-o.rempel@pengutronix.de>
[-- Attachment #1: Type: text/plain, Size: 1347 bytes --]
On Wed, Jun 21, 2023 at 11:32:41AM +0200, Oleksij Rempel wrote:
> Add the 'fsl,imx6ul-usdhc' value to the compatible properties list in
> the fsl-imx-esdhc.yaml file. This is required to match the compatible
> strings present in the 'mmc@2190000' node of 'imx6ul-prti6g.dtb'. This
> commit addresses the following dtbs_check warning:
> imx6ul-prti6g.dtb:0:0: /soc/bus@2100000/mmc@2190000: failed to match any schema with compatible: ['fsl,imx6ul-usdhc', 'fsl,imx6sx-usdhc']
>
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
Seems harmless...
Acked-by: Conor Dooley <conor.dooley@microchip.com>
Cheers,
Conor.
> ---
> Documentation/devicetree/bindings/mmc/fsl-imx-esdhc.yaml | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/Documentation/devicetree/bindings/mmc/fsl-imx-esdhc.yaml b/Documentation/devicetree/bindings/mmc/fsl-imx-esdhc.yaml
> index fbfd822b92707..82eb7a24c8578 100644
> --- a/Documentation/devicetree/bindings/mmc/fsl-imx-esdhc.yaml
> +++ b/Documentation/devicetree/bindings/mmc/fsl-imx-esdhc.yaml
> @@ -42,6 +42,7 @@ properties:
> - enum:
> - fsl,imx6sll-usdhc
> - fsl,imx6ull-usdhc
> + - fsl,imx6ul-usdhc
> - const: fsl,imx6sx-usdhc
> - items:
> - const: fsl,imx7d-usdhc
> --
> 2.39.2
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH] HID: logitech-hidpp: Handle timeout differently from busy
From: Linux regression tracking (Thorsten Leemhuis) @ 2023-06-21 17:19 UTC (permalink / raw)
To: Benjamin Tissoires, Linux regressions mailing list
Cc: Mark Lord, Jiri Kosina, Bastien Nocera, linux-input, linux-kernel,
Peter F . Patel-Schneider, Filipe Laíns, Nestor Lopez Casado
In-Reply-To: <CAO-hwJLyA==_Wkyi-gTn-FOAAne2JKDfNMY2EaELoFDo5Qbe-A@mail.gmail.com>
On 21.06.23 17:10, Benjamin Tissoires wrote:
> On Wed, Jun 21, 2023 at 4:09 PM Linux regression tracking (Thorsten
> Leemhuis) <regressions@leemhuis.info> wrote:
>>
>> On 15.06.23 13:24, Mark Lord wrote:
>>> On 2023-06-15 03:24 AM, Linux regression tracking (Thorsten Leemhuis) wrote:
>>> ...
>>>> https://bugzilla.kernel.org/show_bug.cgi?id=217412
>>>>
>>>> --- Comment #47 from Mark Blakeney ---
>>>> @Juha, kernel 6.3.7 adds the 2 patches intended to fix this bug and the
>>>> startup delay is now gone. However, I have had 2 cases over the last 5
>>>> days in which I have been running 6.3.7 where my mouse fails to be
>>>> detected at all after startup. I have to pull the Logitech receiver
>>>> out/in to get the mouse working. Never seen this issue before so I
>>>> suspect the patches are not right.
>>>> ```
>>>
>>> I too have had that happen with recent kernels,
>>
>> Ahh, good to know!
>>
>>> but have not yet put
>>> a finger to a specific version or cause.
>>>
>>> Just toggling the power button on the wireless mouse is enough for
>>> it to "re-appear".
>>>
>>> The 5.4.xx kernels never had this issue. I went straight from those
>>> to the 6.3.xx ones, where it does happen sometimes, both with and without
>>> the recent "delay" fixes.
>>
>> From Mark Blakeney it sounded a lot like this is something that started
>> with 6.3, but would be good to confirm. Which brings me to the reason
>> why I write this mail:
>>
>> Is anyone still working on this? There was radio silence already for a
>> week now. Okay, it's not really urgent, but I guess this should not fall
>> through the cracks.
>
> Sorry for the radio silence.
No worries, we all have much to do. But I thought it was time for a
quick status inquiry.
> I worked on hidpp yesterday and submitted
> a new patch for a problem that was overlooked in the previous fixes:
> https://lore.kernel.org/linux-input/20230621-logitech-fixes-v1-1-32e70933c0b0@redhat.com/
Great, many thx!
> The loop was not properly initializing all its local states, meaning
> that when we encountered a "please retry" from the device, we could
> never do the actual retry and returned a different error than in the
> 6.2 series.
>
> Would anyone give it a shot?
Might be worth mentioning it in the bugzilla ticket, as I sadly can't CC
those people here. I'll do this for once (konstantin's bugbot will soon
solve this problem...)
Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
--
Everything you wanna know about Linux kernel regression tracking:
https://linux-regtracking.leemhuis.info/about/#tldr
If I did something stupid, please tell me, as explained on that page.
^ permalink raw reply
* Re: [PATCH v2 00/10] drm/panel and i2c-hid: Allow panels and touchscreens to power sequence together
From: Doug Anderson @ 2023-06-21 16:31 UTC (permalink / raw)
To: Maxime Ripard
Cc: Jiri Kosina, Benjamin Tissoires, Bjorn Andersson, Konrad Dybcio,
Rob Herring, Frank Rowand, Krzysztof Kozlowski, Conor Dooley,
Neil Armstrong, Sam Ravnborg, Maarten Lankhorst,
Thomas Zimmermann, dri-devel, Dmitry Torokhov, linux-input,
Daniel Vetter, linux-kernel, hsinyi, cros-qcom-dts-watchers,
devicetree, yangcong5, linux-arm-msm, Chris Morgan
In-Reply-To: <CAD=FV=VO=GE5BEw6kKK19Qj9tcia509Pb-bvMcq0uA05sVLvHw@mail.gmail.com>
Maxime,
On Tue, Jun 13, 2023 at 8:56 AM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Tue, Jun 13, 2023 at 5:06 AM Maxime Ripard <mripard@kernel.org> wrote:
> >
> > > > What I'm trying to say is: could we just make it work by passing a bunch
> > > > of platform_data, 2-3 callbacks and a device registration from the panel
> > > > driver directly?
> > >
> > > I think I'm still confused about what you're proposing. Sorry! :( Let
> > > me try rephrasing why I'm confused and perhaps we can get on the same
> > > page. :-)
> > >
> > > First, I guess I'm confused about how you have one of these devices
> > > "register" the other device.
> > >
> > > I can understand how one device might "register" its sub-devices in
> > > the MFD case. To make it concrete, we can look at a PMIC like
> > > max77686.c. The parent MFD device gets probed and then it's in charge
> > > of creating all of its sub-devices. These sub-devices are intimately
> > > tied to one another. They have shared data structures and can
> > > coordinate power sequencing and whatnot. All good.
> >
> > We don't necessarily need to use MFD, but yeah, we could just register a
> > device for the i2c-hid driver to probe from (using
> > i2c_new_client_device?)
>
> I think this can work for devices where the panel and touchscreen are
> truly integrated where the panel driver knows enough about the related
> touchscreen to fully describe and instantiate it. It doesn't work
> quite as well for cases where the power and reset lines are shared
> just because of what a given board designer did. To handle that, each
> panel driver would need to get enough DT properties added to it so
> that it could fully describe any arbitrary touchscreen, right?
>
> Let's think about the generic panel-edp driver. This driver runs the
> panel on many sc7180-trogdor laptops, including coachz, lazor, and
> pompom. All three of those boards have a shared power rail for the
> touchscreen and panel. If you look at "sc7180-trogdor-coachz.dtsi",
> you can see the touchscreen currently looks like this:
>
> ap_ts: touchscreen@5d {
> compatible = "goodix,gt7375p";
> reg = <0x5d>;
> pinctrl-names = "default";
> pinctrl-0 = <&ts_int_l>, <&ts_reset_l>;
>
> interrupt-parent = <&tlmm>;
> interrupts = <9 IRQ_TYPE_LEVEL_LOW>;
>
> reset-gpios = <&tlmm 8 GPIO_ACTIVE_LOW>;
>
> vdd-supply = <&pp3300_ts>;
> };
>
> In "sc7180-trogdor-lazor.dtsi" we have:
>
> ap_ts: touchscreen@10 {
> compatible = "hid-over-i2c";
> reg = <0x10>;
> pinctrl-names = "default";
> pinctrl-0 = <&ts_int_l>, <&ts_reset_l>;
>
> interrupt-parent = <&tlmm>;
> interrupts = <9 IRQ_TYPE_LEVEL_LOW>;
>
> post-power-on-delay-ms = <20>;
> hid-descr-addr = <0x0001>;
>
> vdd-supply = <&pp3300_ts>;
> };
>
> In both cases "pp3300_ts" is simply another name for "pp3300_dx_edp"
>
> So I think to do what you propose, we need to add this information to
> the panel-edp DT node so that it could dynamically construct the i2c
> device for the touchscreen:
>
> a) Which touchscreen is actually connected (generic hid-over-i2c,
> goodix, ...). I guess this would be a "compatible" string?
>
> b) Which i2c bus that device is hooked up to.
>
> c) Which i2c address that device is hooked up to.
>
> d) What the touchscreen interrupt GPIO is.
>
> e) Possibly what the "hid-descr-addr" for the touchscreen is.
>
> f) Any extra timing information needed to be passed to the touchscreen
> driver, like "post-power-on-delay-ms"
>
> The "pinctrl" stuff would be easy to subsume into the panel's DT node,
> at least. ...and, in this case, we could skip the "vdd-supply" since
> the panel and eDP are sharing power rails (which is what got us into
> this situation). ...but, the above is still a lot. At this point, it
> would make sense to have a sub-node under the panel to describe it,
> which we could do but it starts to feel weird. We'd essentially be
> describing an i2c device but not under the i2c controller it belongs
> to.
>
> I guess I'd also say that the above design also need additional code
> if/when someone had a touchscreen that used a different communication
> method, like SPI.
>
>
> So I guess the tl;dr of all the above is that I think it could all work if:
>
> 1. We described the touchscreen in a sub-node of the panel.
>
> 2. We added a property to the panel saying what the true parent of the
> touchscreen was (an I2C controller, a SPI controller, ...) and what
> type of controller it was ("SPI" vs "I2C").
>
> 3. We added some generic helpers that panels could call that would
> understand how to instantiate the touchscreen under the appropriate
> controller.
>
> 4. From there, we added a new private / generic API between panels and
> touchscreens letting them know that the panel was turning on/off.
>
> That seems much more complex to me, though. It also seems like an
> awkward way to describe it in DT.
>
>
> > > In any case, is there any chance that we're in violent agreement
> >
> > Is it even violent? Sorry if it came across that way, it's really isn't
> > on my end.
>
> Sorry, maybe a poor choice of words on my end. I've heard that term
> thrown about when two people spend a lot of time discussing something
> / trying to persuade the other person only to find out in the end that
> they were both on the same side of the issue. ;-)
>
>
> > > and that if you dig into my design more you might like it? Other than
> > > the fact that the panel doesn't "register" the touchscreen device, it
> > > kinda sounds as if what my patches are already doing is roughly what
> > > you're describing. The touchscreen and panel driver are really just
> > > coordinating with each other through a shared data structure (struct
> > > drm_panel_follower) that has a few callback functions. Just like with
> > > "hdmi-codec", the devices probe separately but find each other through
> > > a phandle. The coordination between the two happens through a few
> > > simple helper functions.
> >
> > I guess we very much agree on the end-goal, and I'd really like to get
> > this addressed somehow. There's a couple of things I'm not really
> > sold on with your proposal though:
> >
> > - It creates a ad-hoc KMS API for some problem that looks fairly
> > generic. It's also redundant with the notifier mechanism without
> > using it (probably for the best though).
> >
> > - MIPI-DSI panel probe sequence is already fairly complex and fragile
> > (See https://www.kernel.org/doc/html/latest/gpu/drm-kms-helpers.html#special-care-with-mipi-dsi-bridges).
> > I'd rather avoid creating a new dependency in that graph.
> >
> > - And yeah, to some extent it's inconsistent with how we dealt with
> > secondary devices in KMS so far.
>
> Hmmmm. To a large extent, my current implementation actually has no
> impact on the DRM probe sequence. The panel itself never looks for the
> touchscreen code and everything DRM-related can register without a
> care in the world. From reading your bullet points, I guess that's
> both a strength and a weakness of my current proposal. It's really
> outside the world of bridge chains and DRM components which makes it a
> special snowflake that people need to understand on its own. ...but,
> at the same time, the fact that it is outside all the rest of that
> stuff means it doesn't add complexity to an already complex system.
>
> I guess I'd point to the panel backlight as a preexisting design
> that's not totally unlike what I'm doing. The backlight is not part of
> the DRM bridge chain and doesn't fit in like other components. This
> actually makes sense since the backlight doesn't take in or put out
> video data and it's simply something associated with the panel. The
> backlight also has a loose connection to the panel driver and a given
> panel could be associated with any number of different backlight
> drivers depending on the board design. I guess one difference between
> the backlight and what I'm doing with "panel follower" is that we
> typically don't let the panel probe until after the backlight has
> probed. In the case of my "panel follower" proposal it's the opposite.
> As per above, from a DRM probe point of view this actually makes my
> proposal less intrusive. I guess also a difference between backlight
> and "panel follower" is that I allow an arbitrary number of followers
> but there's only one backlight.
>
> One additional note: if I actually make the panel probe function start
> registering the touchscreen, that actually _does_ add more complexity
> to the already complex DRM probe ordering. It's yet another thing that
> could fail and/or defer...
>
> Also, I'm curious: would my proposal be more or less palatable if I
> made it less generic? Instead of "panel follower", I could hardcode it
> to "touchscreen" and then remove all the list management. From a DRM
> point of view this would make it even more like the preexisting
> "backlight" except for the ordering difference.
I'm trying to figure out what the next steps are here. I can send a v3
and address Benjamin's comments on the i2c-hid side, but I'd like to
get some resolution on our conversation here, too. Did my thoughts
above make sense? I know that "panel follower" isn't a
beautiful/elegant framework, but IMO it isn't not too bad. It
accomplishes the goal and mostly stays out of the way.
If you don't have time to dig into all of this now, would you object
if I can find someone else willing to review my series from a drm
perspective?
Thanks!
-Doug
^ permalink raw reply
* Re: [PATCH v2] HID: wacom: Use ktime_t rather than int when dealing with timestamps
From: Benjamin Tissoires @ 2023-06-21 16:04 UTC (permalink / raw)
To: Jason Gerecke
Cc: linux-input, linux-kernel, Jiri Kosina, Ping Cheng,
Aaron Armstrong Skomra, Joshua Dickens, Peter Hutterer,
Jason Gerecke, stable
In-Reply-To: <CANRwn3R-XbfB+mP9AQ-J7R_19jLi4eS3MhswaWjL+LCEih-7pg@mail.gmail.com>
On Wed, Jun 21, 2023 at 5:18 PM Jason Gerecke <killertofu@gmail.com> wrote:
>
> Following up since no action seems to have been taken on this patch yet.
Sorry, this went through the cracks (I seem to have a lot of cracks recently...)
>
> On Thu, Jun 8, 2023 at 2:38 PM Jason Gerecke <killertofu@gmail.com> wrote:
> >
> > Code which interacts with timestamps needs to use the ktime_t type
> > returned by functions like ktime_get. The int type does not offer
> > enough space to store these values, and attempting to use it is a
> > recipe for problems. In this particular case, overflows would occur
> > when calculating/storing timestamps leading to incorrect values being
> > reported to userspace. In some cases these bad timestamps cause input
> > handling in userspace to appear hung.
I have to ask, is this something we should consider writing a test for? :)
Also, you are missing the rev-by from Peter, not sure if the tools
will pick it up automatically then.
Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Cheers,
Benjamin
> >
> > Link: https://gitlab.freedesktop.org/libinput/libinput/-/issues/901
> > Fixes: 17d793f3ed53 ("HID: wacom: insert timestamp to packed Bluetooth (BT) events")
> > CC: stable@vger.kernel.org
> > Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com>
> > ---
> > v2: Use div_u64 to perform division to deal with ARC and ARM architectures
> > (as found by the kernel test robot)
> >
> > drivers/hid/wacom_wac.c | 6 +++---
> > drivers/hid/wacom_wac.h | 2 +-
> > 2 files changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
> > index 2ccf838371343..174bf03908d7c 100644
> > --- a/drivers/hid/wacom_wac.c
> > +++ b/drivers/hid/wacom_wac.c
> > @@ -1314,7 +1314,7 @@ static void wacom_intuos_pro2_bt_pen(struct wacom_wac *wacom)
> > struct input_dev *pen_input = wacom->pen_input;
> > unsigned char *data = wacom->data;
> > int number_of_valid_frames = 0;
> > - int time_interval = 15000000;
> > + ktime_t time_interval = 15000000;
> > ktime_t time_packet_received = ktime_get();
> > int i;
> >
> > @@ -1348,7 +1348,7 @@ static void wacom_intuos_pro2_bt_pen(struct wacom_wac *wacom)
> > if (number_of_valid_frames) {
> > if (wacom->hid_data.time_delayed)
> > time_interval = ktime_get() - wacom->hid_data.time_delayed;
> > - time_interval /= number_of_valid_frames;
> > + time_interval = div_u64(time_interval, number_of_valid_frames);
> > wacom->hid_data.time_delayed = time_packet_received;
> > }
> >
> > @@ -1359,7 +1359,7 @@ static void wacom_intuos_pro2_bt_pen(struct wacom_wac *wacom)
> > bool range = frame[0] & 0x20;
> > bool invert = frame[0] & 0x10;
> > int frames_number_reversed = number_of_valid_frames - i - 1;
> > - int event_timestamp = time_packet_received - frames_number_reversed * time_interval;
> > + ktime_t event_timestamp = time_packet_received - frames_number_reversed * time_interval;
> >
> > if (!valid)
> > continue;
> > diff --git a/drivers/hid/wacom_wac.h b/drivers/hid/wacom_wac.h
> > index 1a40bb8c5810c..ee21bb260f22f 100644
> > --- a/drivers/hid/wacom_wac.h
> > +++ b/drivers/hid/wacom_wac.h
> > @@ -324,7 +324,7 @@ struct hid_data {
> > int ps_connected;
> > bool pad_input_event_flag;
> > unsigned short sequence_number;
> > - int time_delayed;
> > + ktime_t time_delayed;
> > };
> >
> > struct wacom_remote_data {
> > --
> > 2.41.0
> >
>
^ permalink raw reply
* Re: [PATCH] HID: hidraw: fix data race on device refcount
From: Benjamin Tissoires @ 2023-06-21 15:55 UTC (permalink / raw)
To: Jiri Kosina, André Almeida, Ludvig Michaelsson
Cc: Benjamin Tissoires, linux-input, linux-kernel
In-Reply-To: <20230621-hidraw-race-v1-1-a58e6ac69bab@yubico.com>
On Wed, 21 Jun 2023 13:17:43 +0200, Ludvig Michaelsson wrote:
> The hidraw_open() function increments the hidraw device reference
> counter. The counter has no dedicated synchronization mechanism,
> resulting in a potential data race when concurrently opening a device.
>
> The race is a regression introduced by commit 8590222e4b02 ("HID:
> hidraw: Replace hidraw device table mutex with a rwsem"). While
> minors_rwsem is intended to protect the hidraw_table itself, by instead
> acquiring the lock for writing, the reference counter is also protected.
> This is symmetrical to hidraw_release().
>
> [...]
Added stable@ cc tags and
Applied to hid/hid.git (for-6.4/upstream-fixes), thanks!
[1/1] HID: hidraw: fix data race on device refcount
https://git.kernel.org/hid/hid/c/944ee77dc6ec
Cheers,
--
Benjamin Tissoires <benjamin.tissoires@redhat.com>
^ permalink raw reply
* Re: [PATCH v2] HID: wacom: Use ktime_t rather than int when dealing with timestamps
From: Jason Gerecke @ 2023-06-21 15:18 UTC (permalink / raw)
To: linux-input, linux-kernel, Benjamin Tissoires, Jiri Kosina
Cc: Ping Cheng, Aaron Armstrong Skomra, Joshua Dickens,
Peter Hutterer, Jason Gerecke, stable
In-Reply-To: <20230608213828.2108-1-jason.gerecke@wacom.com>
Following up since no action seems to have been taken on this patch yet.
On Thu, Jun 8, 2023 at 2:38 PM Jason Gerecke <killertofu@gmail.com> wrote:
>
> Code which interacts with timestamps needs to use the ktime_t type
> returned by functions like ktime_get. The int type does not offer
> enough space to store these values, and attempting to use it is a
> recipe for problems. In this particular case, overflows would occur
> when calculating/storing timestamps leading to incorrect values being
> reported to userspace. In some cases these bad timestamps cause input
> handling in userspace to appear hung.
>
> Link: https://gitlab.freedesktop.org/libinput/libinput/-/issues/901
> Fixes: 17d793f3ed53 ("HID: wacom: insert timestamp to packed Bluetooth (BT) events")
> CC: stable@vger.kernel.org
> Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com>
> ---
> v2: Use div_u64 to perform division to deal with ARC and ARM architectures
> (as found by the kernel test robot)
>
> drivers/hid/wacom_wac.c | 6 +++---
> drivers/hid/wacom_wac.h | 2 +-
> 2 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
> index 2ccf838371343..174bf03908d7c 100644
> --- a/drivers/hid/wacom_wac.c
> +++ b/drivers/hid/wacom_wac.c
> @@ -1314,7 +1314,7 @@ static void wacom_intuos_pro2_bt_pen(struct wacom_wac *wacom)
> struct input_dev *pen_input = wacom->pen_input;
> unsigned char *data = wacom->data;
> int number_of_valid_frames = 0;
> - int time_interval = 15000000;
> + ktime_t time_interval = 15000000;
> ktime_t time_packet_received = ktime_get();
> int i;
>
> @@ -1348,7 +1348,7 @@ static void wacom_intuos_pro2_bt_pen(struct wacom_wac *wacom)
> if (number_of_valid_frames) {
> if (wacom->hid_data.time_delayed)
> time_interval = ktime_get() - wacom->hid_data.time_delayed;
> - time_interval /= number_of_valid_frames;
> + time_interval = div_u64(time_interval, number_of_valid_frames);
> wacom->hid_data.time_delayed = time_packet_received;
> }
>
> @@ -1359,7 +1359,7 @@ static void wacom_intuos_pro2_bt_pen(struct wacom_wac *wacom)
> bool range = frame[0] & 0x20;
> bool invert = frame[0] & 0x10;
> int frames_number_reversed = number_of_valid_frames - i - 1;
> - int event_timestamp = time_packet_received - frames_number_reversed * time_interval;
> + ktime_t event_timestamp = time_packet_received - frames_number_reversed * time_interval;
>
> if (!valid)
> continue;
> diff --git a/drivers/hid/wacom_wac.h b/drivers/hid/wacom_wac.h
> index 1a40bb8c5810c..ee21bb260f22f 100644
> --- a/drivers/hid/wacom_wac.h
> +++ b/drivers/hid/wacom_wac.h
> @@ -324,7 +324,7 @@ struct hid_data {
> int ps_connected;
> bool pad_input_event_flag;
> unsigned short sequence_number;
> - int time_delayed;
> + ktime_t time_delayed;
> };
>
> struct wacom_remote_data {
> --
> 2.41.0
>
^ permalink raw reply
* Re: [PATCH] HID: logitech-hidpp: Handle timeout differently from busy
From: Benjamin Tissoires @ 2023-06-21 15:10 UTC (permalink / raw)
To: Linux regressions mailing list
Cc: Mark Lord, Jiri Kosina, Bastien Nocera, linux-input, linux-kernel,
Peter F . Patel-Schneider, Filipe Laíns, Nestor Lopez Casado
In-Reply-To: <ed27ca39-3609-695c-9f04-65c0bad343c2@leemhuis.info>
On Wed, Jun 21, 2023 at 4:09 PM Linux regression tracking (Thorsten
Leemhuis) <regressions@leemhuis.info> wrote:
>
> On 15.06.23 13:24, Mark Lord wrote:
> > On 2023-06-15 03:24 AM, Linux regression tracking (Thorsten Leemhuis) wrote:
> > ...
> >> https://bugzilla.kernel.org/show_bug.cgi?id=217412
> >>
> >> --- Comment #47 from Mark Blakeney ---
> >> @Juha, kernel 6.3.7 adds the 2 patches intended to fix this bug and the
> >> startup delay is now gone. However, I have had 2 cases over the last 5
> >> days in which I have been running 6.3.7 where my mouse fails to be
> >> detected at all after startup. I have to pull the Logitech receiver
> >> out/in to get the mouse working. Never seen this issue before so I
> >> suspect the patches are not right.
> >> ```
> >
> > I too have had that happen with recent kernels,
>
> Ahh, good to know!
>
> > but have not yet put
> > a finger to a specific version or cause.
> >
> > Just toggling the power button on the wireless mouse is enough for
> > it to "re-appear".
> >
> > The 5.4.xx kernels never had this issue. I went straight from those
> > to the 6.3.xx ones, where it does happen sometimes, both with and without
> > the recent "delay" fixes.
>
> From Mark Blakeney it sounded a lot like this is something that started
> with 6.3, but would be good to confirm. Which brings me to the reason
> why I write this mail:
>
> Is anyone still working on this? There was radio silence already for a
> week now. Okay, it's not really urgent, but I guess this should not fall
> through the cracks.
Sorry for the radio silence. I worked on hidpp yesterday and submitted
a new patch for a problem that was overlooked in the previous fixes:
https://lore.kernel.org/linux-input/20230621-logitech-fixes-v1-1-32e70933c0b0@redhat.com/
The loop was not properly initializing all its local states, meaning
that when we encountered a "please retry" from the device, we could
never do the actual retry and returned a different error than in the
6.2 series.
Would anyone give it a shot?
Cheers,
Benjamin
>
> Bastian, are you back?
>
> If not: Does anyone know if there is hope that Bastien will be able to
> look into this in the not too far future?
>
> Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
> --
> Everything you wanna know about Linux kernel regression tracking:
> https://linux-regtracking.leemhuis.info/about/#tldr
> If I did something stupid, please tell me, as explained on that page.
>
^ permalink raw reply
* Re: Clarification about the hidraw documentation on HIDIOCGFEATURE
From: Benjamin Tissoires @ 2023-06-21 15:05 UTC (permalink / raw)
To: Xiaofan Chen; +Cc: Jiri Kosina, Ihor Dutchak, linux-input
In-Reply-To: <CAGjSPUC4q_tGmC8EZ4CMTVGa7e9AV9jkWOgwexJAtE-=rFDHHA@mail.gmail.com>
On Wed, Jun 21, 2023 at 1:27 PM Xiaofan Chen <xiaofanc@gmail.com> wrote:
>
> On Tue, Jun 20, 2023 at 7:28 AM Xiaofan Chen <xiaofanc@gmail.com> wrote:
> >
> > On Mon, Jun 19, 2023 at 11:14 PM Xiaofan Chen <xiaofanc@gmail.com> wrote:
> > >
> > > On Mon, Jun 19, 2023 at 2:09 PM Xiaofan Chen <xiaofanc@gmail.com> wrote:
> > > >
> > > > I know that thurrent documentation has been there since it was created by
> > > > Alan Ott many years ago. And he started the HIDAPI project around that
> > > > time as well. However, I am starting to doubt whether it is correct or not
> > > > based on the testing using HIDAPI.
> > > >
> > > > Please help to clarify. Thanks.
> > > >
> > > > https://docs.kernel.org/hid/hidraw.html
> > > > +++++++++++++++++++++++++++++++++++++++++++++++++++++
> > > > HIDIOCGFEATURE(len):
> > > >
> > > > Get a Feature Report
> > > >
> > > > This ioctl will request a feature report from the device using the
> > > > control endpoint. The first byte of the supplied buffer should be
> > > > set to the report number of the requested report. For devices
> > > > which do not use numbered reports, set the first byte to 0. The
> > > > returned report buffer will contain the report number in the first
> > > > byte, followed by the report data read from the device. For devices
> > > > which do not use numbered reports, the report data will begin at the
> > > > first byte of the returned buffer.
Yep, this is wrong.
This should be read:
```
The returned report buffer will contain the report number in the first
byte or 0 when the device doesn't use numbered reports. The data read
from the device will be stored in the following bytes.
```
FWIW, the difficulty to find out what the code does is because this part
is handled in each HID transport driver: USB, Bluetooth, I2C.
Looking at drivers/hid/usbhid/hid-core.c, lines 869+, the function
usbhid_get_raw_report() is the one used by hidraw in the end and is
still the original code from Alan:
```
/* Byte 0 is the report number. Report data starts at byte 1.*/
buf[0] = report_number;
if (report_number == 0x0) {
/* Offset the return buffer by 1, so that the report ID
will remain in byte 0. */
buf++;
count--;
skipped_report_id = 1;
}
```
>
> > > > ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > > >
> > > > I have doubts about the last sentence. It seems to me that for
> > > > devices which do not use numbered reports, the first byte will
> > > > be 0 and the report data begins in the second byte.
> > > >
> > > > This is based on testing results using hidapi and hidapitester, which
> > > > use the ioctl.
> > > > int HID_API_EXPORT hid_get_feature_report(hid_device *dev, unsigned
> > > > char *data, size_t length)
> > > > {
> > > > int res;
> > > >
> > > > register_device_error(dev, NULL);
> > > >
> > > > res = ioctl(dev->device_handle, HIDIOCGFEATURE(length), data);
> > > > if (res < 0)
> > > > register_device_error_format(dev, "ioctl (GFEATURE): %s",
> > > > strerror(errno));
> > > >
> > > > return res;
> > > > }
> > > >
> > > > Reference discussion:
> > > > https://github.com/libusb/hidapi/issues/589
> > > >
> > > > Test device is Jan Axelson's generic HID example which I have tested using
> > > > libusb and hidapi across platforms as well as using Windows HID API.
> > > > So I believe the FW is good.
> > > > http://janaxelson.com/hidpage.htm#MyExampleCode (USB PIC MCU)
> > > >
> > >
> > > Modified testing code from the following Linux kernel
> > > samples/hidraw/hid-example.c
> > > https://github.com/libusb/hidapi/issues/589#issuecomment-1597356054
> > >
> > > Results are shown here. We can clearly see that the "Fake Report ID" 0 is
> > > in the first byte of the returned buffer, matching the output from
> > > hidapi/hidapitester,
> > >
> > > mcuee@UbuntuSwift3:~/build/hid/hidraw$ gcc -o myhid-example myhid-example.c
> > >
> > > mcuee@UbuntuSwift3:~/build/hid/hidraw$ sudo ./myhid-example
> > > Report Descriptor Size: 47
> > > Report Descriptor:
> > > 6 a0 ff 9 1 a1 1 9 3 15 0 26 ff 0 75 8 95 3f 81 2 9 4 15 0 26 ff 0 75
> > > 8 95 3f 91 2 9 5 15 0 26 ff 0 75 8 95 3f b1 2 c0
> > >
> > > Raw Name: Microchip Technology Inc. Generic HID
> > > Raw Phys: usb-0000:00:14.0-1/input0
> > > Raw Info:
> > > bustype: 3 (USB)
> > > vendor: 0x0925
> > > product: 0x7001
> > > ioctl HIDIOCSFEATURE returned: 64
> > > ioctl HIDIOCGFEATURE returned: 64
> > > Report data:
> > > 0 41 42 43 44 45 46 47 48 14 4 18 4 4d 72 31 50 51 52 53 54 55 56 57
> > > 58 59 5a 5b 5c 5d 5e 5f 60 61 62 63 64 65 66 67 68 69 6a 6b 6c 6d 6e
> > > 6f 70 71 72 73 74 75 76 77 78 79 7a 7b 7c 7d 7e 7f
> > >
> > > write() wrote 64 bytes
> > > read() read 63 bytes:
> > > 21 22 23 24 25 26 27 28 29 2a 2b 2c 2d 2e 2f 30 31 32 33 34 35 36 37
> > > 38 39 3a 3b 3c 3d 3e 3f 40 41 42 43 44 45 46 47 48 49 4a 4b 4c 4d 4e
> > > 4f 50 51 52 53 54 55 56 57 58 59 5a 5b 5c 5d 5e 5f
> > >
> >
> > Moreover the kernel codes here seem to prove that the documentation
> > is wrong for both HIDIOCGFEATURE and HIDIOCGINPUT.
> >
> > https://github.com/torvalds/linux/blob/master/include/uapi/linux/hidraw.h
> >
> > /* The first byte of SFEATURE and GFEATURE is the report number */
> > #define HIDIOCSFEATURE(len) _IOC(_IOC_WRITE|_IOC_READ, 'H', 0x06, len)
> > #define HIDIOCGFEATURE(len) _IOC(_IOC_WRITE|_IOC_READ, 'H', 0x07, len)
> > #define HIDIOCGRAWUNIQ(len) _IOC(_IOC_READ, 'H', 0x08, len)
> > /* The first byte of SINPUT and GINPUT is the report number */
> > #define HIDIOCSINPUT(len) _IOC(_IOC_WRITE|_IOC_READ, 'H', 0x09, len)
> > #define HIDIOCGINPUT(len) _IOC(_IOC_WRITE|_IOC_READ, 'H', 0x0A, len)
> > /* The first byte of SOUTPUT and GOUTPUT is the report number */
> > #define HIDIOCSOUTPUT(len) _IOC(_IOC_WRITE|_IOC_READ, 'H', 0x0B, len)
> > #define HIDIOCGOUTPUT(len) _IOC(_IOC_WRITE|_IOC_READ, 'H', 0x0C, len)
>
> Hi Jiri and Benjamin,
>
> Sorry to ping the two maintainers, hopefully you two can give the
> answer. Thanks.
See above, I also think the documentation is wrong.
Cheers,
Benjamin
>
>
> --
> Xiaofan
>
^ permalink raw reply
* Re: [PATCH] HID: logitech-hidpp: Handle timeout differently from busy
From: Linux regression tracking (Thorsten Leemhuis) @ 2023-06-21 14:03 UTC (permalink / raw)
To: Mark Lord, Linux regressions mailing list, Jiri Kosina,
Bastien Nocera, Benjamin Tissoires
Cc: linux-input, linux-kernel, Peter F . Patel-Schneider,
Filipe Laíns, Nestor Lopez Casado
In-Reply-To: <aa0e3371-dad1-3296-18fb-1957b92aa4d1@pobox.com>
On 15.06.23 13:24, Mark Lord wrote:
> On 2023-06-15 03:24 AM, Linux regression tracking (Thorsten Leemhuis) wrote:
> ...
>> https://bugzilla.kernel.org/show_bug.cgi?id=217412
>>
>> --- Comment #47 from Mark Blakeney ---
>> @Juha, kernel 6.3.7 adds the 2 patches intended to fix this bug and the
>> startup delay is now gone. However, I have had 2 cases over the last 5
>> days in which I have been running 6.3.7 where my mouse fails to be
>> detected at all after startup. I have to pull the Logitech receiver
>> out/in to get the mouse working. Never seen this issue before so I
>> suspect the patches are not right.
>> ```
>
> I too have had that happen with recent kernels,
Ahh, good to know!
> but have not yet put
> a finger to a specific version or cause.
>
> Just toggling the power button on the wireless mouse is enough for
> it to "re-appear".
>
> The 5.4.xx kernels never had this issue. I went straight from those
> to the 6.3.xx ones, where it does happen sometimes, both with and without
> the recent "delay" fixes.
From Mark Blakeney it sounded a lot like this is something that started
with 6.3, but would be good to confirm. Which brings me to the reason
why I write this mail:
Is anyone still working on this? There was radio silence already for a
week now. Okay, it's not really urgent, but I guess this should not fall
through the cracks.
Bastian, are you back?
If not: Does anyone know if there is hope that Bastien will be able to
look into this in the not too far future?
Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
--
Everything you wanna know about Linux kernel regression tracking:
https://linux-regtracking.leemhuis.info/about/#tldr
If I did something stupid, please tell me, as explained on that page.
^ permalink raw reply
* Re: Clarification about the hidraw documentation on HIDIOCGFEATURE
From: Xiaofan Chen @ 2023-06-21 11:26 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires; +Cc: Ihor Dutchak, linux-input
In-Reply-To: <CAGjSPUAnGndHOzEkux2DcjOKZ14BKv+Cccn0Hk3=VhMzoTbC5A@mail.gmail.com>
On Tue, Jun 20, 2023 at 7:28 AM Xiaofan Chen <xiaofanc@gmail.com> wrote:
>
> On Mon, Jun 19, 2023 at 11:14 PM Xiaofan Chen <xiaofanc@gmail.com> wrote:
> >
> > On Mon, Jun 19, 2023 at 2:09 PM Xiaofan Chen <xiaofanc@gmail.com> wrote:
> > >
> > > I know that thurrent documentation has been there since it was created by
> > > Alan Ott many years ago. And he started the HIDAPI project around that
> > > time as well. However, I am starting to doubt whether it is correct or not
> > > based on the testing using HIDAPI.
> > >
> > > Please help to clarify. Thanks.
> > >
> > > https://docs.kernel.org/hid/hidraw.html
> > > +++++++++++++++++++++++++++++++++++++++++++++++++++++
> > > HIDIOCGFEATURE(len):
> > >
> > > Get a Feature Report
> > >
> > > This ioctl will request a feature report from the device using the
> > > control endpoint. The first byte of the supplied buffer should be
> > > set to the report number of the requested report. For devices
> > > which do not use numbered reports, set the first byte to 0. The
> > > returned report buffer will contain the report number in the first
> > > byte, followed by the report data read from the device. For devices
> > > which do not use numbered reports, the report data will begin at the
> > > first byte of the returned buffer.
> > > ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > >
> > > I have doubts about the last sentence. It seems to me that for
> > > devices which do not use numbered reports, the first byte will
> > > be 0 and the report data begins in the second byte.
> > >
> > > This is based on testing results using hidapi and hidapitester, which
> > > use the ioctl.
> > > int HID_API_EXPORT hid_get_feature_report(hid_device *dev, unsigned
> > > char *data, size_t length)
> > > {
> > > int res;
> > >
> > > register_device_error(dev, NULL);
> > >
> > > res = ioctl(dev->device_handle, HIDIOCGFEATURE(length), data);
> > > if (res < 0)
> > > register_device_error_format(dev, "ioctl (GFEATURE): %s",
> > > strerror(errno));
> > >
> > > return res;
> > > }
> > >
> > > Reference discussion:
> > > https://github.com/libusb/hidapi/issues/589
> > >
> > > Test device is Jan Axelson's generic HID example which I have tested using
> > > libusb and hidapi across platforms as well as using Windows HID API.
> > > So I believe the FW is good.
> > > http://janaxelson.com/hidpage.htm#MyExampleCode (USB PIC MCU)
> > >
> >
> > Modified testing code from the following Linux kernel
> > samples/hidraw/hid-example.c
> > https://github.com/libusb/hidapi/issues/589#issuecomment-1597356054
> >
> > Results are shown here. We can clearly see that the "Fake Report ID" 0 is
> > in the first byte of the returned buffer, matching the output from
> > hidapi/hidapitester,
> >
> > mcuee@UbuntuSwift3:~/build/hid/hidraw$ gcc -o myhid-example myhid-example.c
> >
> > mcuee@UbuntuSwift3:~/build/hid/hidraw$ sudo ./myhid-example
> > Report Descriptor Size: 47
> > Report Descriptor:
> > 6 a0 ff 9 1 a1 1 9 3 15 0 26 ff 0 75 8 95 3f 81 2 9 4 15 0 26 ff 0 75
> > 8 95 3f 91 2 9 5 15 0 26 ff 0 75 8 95 3f b1 2 c0
> >
> > Raw Name: Microchip Technology Inc. Generic HID
> > Raw Phys: usb-0000:00:14.0-1/input0
> > Raw Info:
> > bustype: 3 (USB)
> > vendor: 0x0925
> > product: 0x7001
> > ioctl HIDIOCSFEATURE returned: 64
> > ioctl HIDIOCGFEATURE returned: 64
> > Report data:
> > 0 41 42 43 44 45 46 47 48 14 4 18 4 4d 72 31 50 51 52 53 54 55 56 57
> > 58 59 5a 5b 5c 5d 5e 5f 60 61 62 63 64 65 66 67 68 69 6a 6b 6c 6d 6e
> > 6f 70 71 72 73 74 75 76 77 78 79 7a 7b 7c 7d 7e 7f
> >
> > write() wrote 64 bytes
> > read() read 63 bytes:
> > 21 22 23 24 25 26 27 28 29 2a 2b 2c 2d 2e 2f 30 31 32 33 34 35 36 37
> > 38 39 3a 3b 3c 3d 3e 3f 40 41 42 43 44 45 46 47 48 49 4a 4b 4c 4d 4e
> > 4f 50 51 52 53 54 55 56 57 58 59 5a 5b 5c 5d 5e 5f
> >
>
> Moreover the kernel codes here seem to prove that the documentation
> is wrong for both HIDIOCGFEATURE and HIDIOCGINPUT.
>
> https://github.com/torvalds/linux/blob/master/include/uapi/linux/hidraw.h
>
> /* The first byte of SFEATURE and GFEATURE is the report number */
> #define HIDIOCSFEATURE(len) _IOC(_IOC_WRITE|_IOC_READ, 'H', 0x06, len)
> #define HIDIOCGFEATURE(len) _IOC(_IOC_WRITE|_IOC_READ, 'H', 0x07, len)
> #define HIDIOCGRAWUNIQ(len) _IOC(_IOC_READ, 'H', 0x08, len)
> /* The first byte of SINPUT and GINPUT is the report number */
> #define HIDIOCSINPUT(len) _IOC(_IOC_WRITE|_IOC_READ, 'H', 0x09, len)
> #define HIDIOCGINPUT(len) _IOC(_IOC_WRITE|_IOC_READ, 'H', 0x0A, len)
> /* The first byte of SOUTPUT and GOUTPUT is the report number */
> #define HIDIOCSOUTPUT(len) _IOC(_IOC_WRITE|_IOC_READ, 'H', 0x0B, len)
> #define HIDIOCGOUTPUT(len) _IOC(_IOC_WRITE|_IOC_READ, 'H', 0x0C, len)
Hi Jiri and Benjamin,
Sorry to ping the two maintainers, hopefully you two can give the
answer. Thanks.
--
Xiaofan
^ permalink raw reply
* [PATCH] HID: hidraw: fix data race on device refcount
From: Ludvig Michaelsson via B4 Relay @ 2023-06-21 11:17 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, André Almeida
Cc: linux-input, linux-kernel, Ludvig Michaelsson
From: Ludvig Michaelsson <ludvig.michaelsson@yubico.com>
The hidraw_open() function increments the hidraw device reference
counter. The counter has no dedicated synchronization mechanism,
resulting in a potential data race when concurrently opening a device.
The race is a regression introduced by commit 8590222e4b02 ("HID:
hidraw: Replace hidraw device table mutex with a rwsem"). While
minors_rwsem is intended to protect the hidraw_table itself, by instead
acquiring the lock for writing, the reference counter is also protected.
This is symmetrical to hidraw_release().
Link: https://github.com/systemd/systemd/issues/27947
Fixes: 8590222e4b02 ("HID: hidraw: Replace hidraw device table mutex with a rwsem")
Signed-off-by: Ludvig Michaelsson <ludvig.michaelsson@yubico.com>
---
drivers/hid/hidraw.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/hid/hidraw.c b/drivers/hid/hidraw.c
index 93e62b161501..e63c56a0d57f 100644
--- a/drivers/hid/hidraw.c
+++ b/drivers/hid/hidraw.c
@@ -272,7 +272,12 @@ static int hidraw_open(struct inode *inode, struct file *file)
goto out;
}
- down_read(&minors_rwsem);
+ /*
+ * Technically not writing to the hidraw_table but a write lock is
+ * required to protect the device refcount. This is symmetrical to
+ * hidraw_release().
+ */
+ down_write(&minors_rwsem);
if (!hidraw_table[minor] || !hidraw_table[minor]->exist) {
err = -ENODEV;
goto out_unlock;
@@ -301,7 +306,7 @@ static int hidraw_open(struct inode *inode, struct file *file)
spin_unlock_irqrestore(&hidraw_table[minor]->list_lock, flags);
file->private_data = list;
out_unlock:
- up_read(&minors_rwsem);
+ up_write(&minors_rwsem);
out:
if (err < 0)
kfree(list);
---
base-commit: 45a3e24f65e90a047bef86f927ebdc4c710edaa1
change-id: 20230621-hidraw-race-b51b11bf11ed
Best regards,
--
Ludvig Michaelsson <ludvig.michaelsson@yubico.com>
^ permalink raw reply related
* Re: [PATCH] HID: logitech-hidpp: rework one more time the retries attempts
From: Greg KH @ 2023-06-21 10:50 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Filipe Laíns, Bastien Nocera, Jiri Kosina, linux-input,
linux-kernel, stable
In-Reply-To: <20230621-logitech-fixes-v1-1-32e70933c0b0@redhat.com>
On Wed, Jun 21, 2023 at 11:42:30AM +0200, Benjamin Tissoires wrote:
> Make the code looks less like Pascal.
>
> Extract the internal code inside a helper function, fix the
> initialization of the parameters used in the helper function
> (`hidpp->answer_available` was not reset and `*response` wasn't too),
> and use a `do {...} while();` loop.
>
> Fixes: 586e8fede795 ("HID: logitech-hidpp: Retry commands when device is busy")
> Cc: stable@vger.kernel.org
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> ---
> as requested by https://lore.kernel.org/all/CAHk-=wiMbF38KCNhPFiargenpSBoecSXTLQACKS2UMyo_Vu2ww@mail.gmail.com/
> This is a rewrite of that particular piece of code.
> ---
> drivers/hid/hid-logitech-hidpp.c | 102 +++++++++++++++++++++++----------------
> 1 file changed, 61 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> index dfe8e09a18de..3d1ffe199f08 100644
> --- a/drivers/hid/hid-logitech-hidpp.c
> +++ b/drivers/hid/hid-logitech-hidpp.c
> @@ -275,21 +275,20 @@ static int __hidpp_send_report(struct hid_device *hdev,
> }
>
> /*
> - * hidpp_send_message_sync() returns 0 in case of success, and something else
> - * in case of a failure.
> - * - If ' something else' is positive, that means that an error has been raised
> - * by the protocol itself.
> - * - If ' something else' is negative, that means that we had a classic error
> - * (-ENOMEM, -EPIPE, etc...)
> + * Effectively send the message to the device, waiting for its answer.
> + *
> + * Must be called with hidpp->send_mutex locked
> + *
> + * Same return protocol than hidpp_send_message_sync():
> + * - success on 0
> + * - negative error means transport error
> + * - positive value means protocol error
> */
> -static int hidpp_send_message_sync(struct hidpp_device *hidpp,
> +static int __do_hidpp_send_message_sync(struct hidpp_device *hidpp,
> struct hidpp_report *message,
> struct hidpp_report *response)
__must_hold(&hidpp->send_mutex) ?
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox