* [PATCH v1 0/1] HID: ft260: add GPIO support
From: Michael Zaidman @ 2023-02-11 11:57 UTC (permalink / raw)
To: jikos; +Cc: linux-kernel, linux-input, Michael Zaidman
The FTDI FT260 chip implements USB to I2C/UART bridges through two USB
HID class interfaces. The first is for I2C, and the second is for UART.
Each interface is independent, and the kernel detects it as a separate
USB hidraw device. In addition, the chip implements 14 GPIOs via
multifunctional pins.
This patch set adds GPIO support.
Michael Zaidman (1):
HID: ft260: add GPIO support
drivers/hid/hid-ft260.c | 476 +++++++++++++++++++++++++++++++++++++---
1 file changed, 447 insertions(+), 29 deletions(-)
--
2.34.1
^ permalink raw reply
* [PATCH v1 1/1] HID: ft260: add GPIO support
From: Michael Zaidman @ 2023-02-11 11:57 UTC (permalink / raw)
To: jikos; +Cc: linux-kernel, linux-input, Michael Zaidman
In-Reply-To: <20230211115752.26276-1-michael.zaidman@gmail.com>
The FTDI FT260 chip has 14 multi-function pins. Each pin can be
set as I2C/UART-related function or GPIO. Some GPIO functions are
destined for applications like TX_ACTIVE, TX_LED, RX_LED for UART;
SUSPOUT_N, WAKEUP for USB; PWREN, and BCD_DET indicator for power
management. The driver enables the GPIO pins according to the
chip's current I2C/UART configuration.
The FT260 has three pins that have more than two functions:
GPIO 2(pin 14), GPIOA (pin 7), and GPIOG (pin 27). The user can
change the function of these pins via eFUSE, EEPROM, or sysfs
interface.
For GPIO usage examples and tips, see
https://github.com/MichaelZaidman/hid-ft260#readme
Signed-off-by: Michael Zaidman <michael.zaidman@gmail.com>
---
drivers/hid/hid-ft260.c | 476 +++++++++++++++++++++++++++++++++++++---
1 file changed, 447 insertions(+), 29 deletions(-)
diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c
index 333341e80b0e..d8ade83b3f10 100644
--- a/drivers/hid/hid-ft260.c
+++ b/drivers/hid/hid-ft260.c
@@ -13,6 +13,7 @@
#include <linux/i2c.h>
#include <linux/module.h>
#include <linux/usb.h>
+#include <linux/gpio/driver.h>
#ifdef DEBUG
static int ft260_debug = 1;
@@ -29,7 +30,7 @@ MODULE_PARM_DESC(debug, "Toggle FT260 debugging messages");
} while (0)
#define FT260_REPORT_MAX_LENGTH (64)
-#define FT260_I2C_DATA_REPORT_ID(len) (FT260_I2C_REPORT_MIN + (len - 1) / 4)
+#define FT260_I2C_DATA_REPORT_ID(len) (FT260_I2C_REPORT_MIN + ((len) - 1) / 4)
#define FT260_WAKEUP_NEEDED_AFTER_MS (4800) /* 5s minus 200ms margin */
@@ -47,6 +48,12 @@ MODULE_PARM_DESC(debug, "Toggle FT260 debugging messages");
#define FT260_RD_DATA_MAX (180)
#define FT260_WR_DATA_MAX (60)
+#define FT260_GPIOCHIP "ft260_gpio"
+#define FT260_GPIO_MAX (6)
+#define FT260_GPIO_EX_MAX (8)
+#define FT260_GPIO_TOTAL (FT260_GPIO_MAX + FT260_GPIO_EX_MAX)
+#define FT260_GPIO_MASK (~(0xffff << FT260_GPIO_TOTAL))
+
/*
* Device interface configuration.
* The FT260 has 2 interfaces that are controlled by DCNF0 and DCNF1 pins.
@@ -132,7 +139,55 @@ enum {
FT260_FLAG_START_STOP_REPEATED = 0x07,
};
-#define FT260_SET_REQUEST_VALUE(report_id) ((FT260_FEATURE << 8) | report_id)
+/* Multi-function pin functions */
+enum {
+ FT260_MFPIN_GPIO = 0x00,
+ FT260_MFPIN_SUSPOUT = 0x01,
+ FT260_MFPIN_PWREN = 0x02,
+ FT260_MFPIN_TX_ACTIVE = 0x03,
+ FT260_MFPIN_TX_LED = 0x04,
+ FT260_MFPIN_RX_LED = 0x05,
+ FT260_MFPIN_BCD_DET = 0x06,
+};
+
+enum {
+ FT260_GPIO_VALUE = 0x00,
+ FT260_GPIO_DIRECTION = 0x01,
+ FT260_GPIO_DIR_INPUT = 0x00,
+ FT260_GPIO_DIR_OUTPUT = 0x01,
+};
+
+/* GPIO offsets */
+enum {
+ FT260_GPIO_0 = (1 << 0),
+ FT260_GPIO_1 = (1 << 1),
+ FT260_GPIO_2 = (1 << 2),
+ FT260_GPIO_3 = (1 << 3),
+ FT260_GPIO_4 = (1 << 4),
+ FT260_GPIO_5 = (1 << 5),
+ FT260_GPIO_A = (1 << (FT260_GPIO_MAX + 0)),
+ FT260_GPIO_B = (1 << (FT260_GPIO_MAX + 1)),
+ FT260_GPIO_C = (1 << (FT260_GPIO_MAX + 2)),
+ FT260_GPIO_D = (1 << (FT260_GPIO_MAX + 3)),
+ FT260_GPIO_E = (1 << (FT260_GPIO_MAX + 4)),
+ FT260_GPIO_F = (1 << (FT260_GPIO_MAX + 5)),
+ FT260_GPIO_G = (1 << (FT260_GPIO_MAX + 6)),
+ FT260_GPIO_H = (1 << (FT260_GPIO_MAX + 7)),
+};
+
+/* GPIO groups */
+enum {
+ FT260_GPIO_WAKEUP = (FT260_GPIO_3),
+ FT260_GPIO_I2C_DEFAULT = (FT260_GPIO_0 | FT260_GPIO_1),
+ FT260_GPIO_UART_DCD_RI = (FT260_GPIO_4 | FT260_GPIO_5),
+ FT260_GPIO_UART = (FT260_GPIO_B | FT260_GPIO_C |
+ FT260_GPIO_D | FT260_GPIO_E |
+ FT260_GPIO_F | FT260_GPIO_H),
+ FT260_GPIO_UART_DEFAULT = (FT260_GPIO_UART |
+ FT260_GPIO_UART_DCD_RI),
+};
+
+#define FT260_SET_REQUEST_VALUE(report_id) ((FT260_FEATURE << 8) | (report_id))
/* Feature In reports */
@@ -152,10 +207,10 @@ struct ft260_get_system_status_report {
u8 uart_mode; /* 0 - OFF; 1 - RTS_CTS, 2 - DTR_DSR, */
/* 3 - XON_XOFF, 4 - No flow control */
u8 hid_over_i2c_en; /* 0 - disabled, 1 - enabled */
- u8 gpio2_function; /* 0 - GPIO, 1 - SUSPOUT, */
+ u8 gpio2_func; /* 0 - GPIO, 1 - SUSPOUT, */
/* 2 - PWREN, 4 - TX_LED */
- u8 gpioA_function; /* 0 - GPIO, 3 - TX_ACTIVE, 4 - TX_LED */
- u8 gpioG_function; /* 0 - GPIO, 2 - PWREN, */
+ u8 gpioa_func; /* 0 - GPIO, 3 - TX_ACTIVE, 4 - TX_LED */
+ u8 gpiog_func; /* 0 - GPIO, 2 - PWREN, */
/* 5 - RX_LED, 6 - BCD_DET */
u8 suspend_out_pol; /* 0 - active-high, 1 - active-low */
u8 enable_wakeup_int; /* 0 - disabled, 1 - enabled */
@@ -171,6 +226,18 @@ struct ft260_get_i2c_status_report {
u8 reserved;
} __packed;
+struct ft260_gpio_state {
+ u8 vals; /* GPIO[0-5] values in bits 0 - 5 */
+ u8 dirs; /* GPIO[0-5] directions, 0 - in, 1 - out */
+ u8 ex_vals; /* GPIO[A-H] values in bits 0 - 7 */
+ u8 ex_dirs; /* GPIO[A-H] directions, 0 - in, 1 - out */
+} __packed;
+
+struct ft260_gpio_read_request_report {
+ u8 report; /* FT260_GPIO */
+ struct ft260_gpio_state gpio;
+} __packed;
+
/* Feature Out reports */
struct ft260_set_system_clock_report {
@@ -203,6 +270,33 @@ struct ft260_set_i2c_speed_report {
__le16 clock; /* I2C bus clock in range 60-3400 KHz */
} __packed;
+struct ft260_set_gpio2_func_report {
+ u8 report; /* FT260_SYSTEM_SETTINGS */
+ u8 request; /* FT260_SELECT_GPIO2_FUNC */
+ u8 gpio2_func; /* Pin func: 0 - GPIO, 1 - SUSPOUT, */
+ /* 2 - PWREN# (active-low), 4 - TX_LED */
+} __packed;
+
+struct ft260_set_gpioa_func_report {
+ u8 report; /* FT260_SYSTEM_SETTINGS */
+ u8 request; /* FT260_SELECT_GPIOA_FUNC */
+ u8 gpioa_func; /* Pin func: 0 - GPIO, */
+ /* 3 - TX_ACTIVE, 4 - TX_LED */
+} __packed;
+
+struct ft260_set_gpiog_func_report {
+ u8 report; /* FT260_SYSTEM_SETTINGS */
+ u8 request; /* FT260_SELECT_GPIOG_FUNC */
+ u8 gpiog_func; /* Pin func: 0 - GPIO, */
+ /* 2 - PWREN# (active-low), */
+ /* 5 - RX_LED, 6 - BCD_DET */
+} __packed;
+
+struct ft260_gpio_write_request_report {
+ u8 report; /* FT260_GPIO */
+ struct ft260_gpio_state gpio;
+} __packed;
+
/* Data transfer reports */
struct ft260_i2c_write_request_report {
@@ -237,13 +331,17 @@ struct ft260_device {
struct i2c_adapter adap;
struct hid_device *hdev;
struct completion wait;
+ struct gpio_chip *gc;
struct mutex lock;
u8 write_buf[FT260_REPORT_MAX_LENGTH];
+ u8 feature_buf[FT260_REPORT_MAX_LENGTH];
unsigned long need_wakeup_at;
u8 *read_buf;
u16 read_idx;
u16 read_len;
u16 clock;
+ struct ft260_gpio_state gpio;
+ u16 gpio_en;
};
static int ft260_hid_feature_report_get(struct hid_device *hdev,
@@ -277,8 +375,6 @@ static int ft260_hid_feature_report_set(struct hid_device *hdev, u8 *data,
if (!buf)
return -ENOMEM;
- buf[0] = FT260_SYSTEM_SETTINGS;
-
ret = hid_hw_raw_request(hdev, buf[0], buf, len, HID_FEATURE_REPORT,
HID_REQ_SET_REPORT);
@@ -291,6 +387,7 @@ static int ft260_i2c_reset(struct hid_device *hdev)
struct ft260_set_i2c_reset_report report;
int ret;
+ report.report = FT260_SYSTEM_SETTINGS;
report.request = FT260_SET_I2C_RESET;
ret = ft260_hid_feature_report_set(hdev, (u8 *)&report, sizeof(report));
@@ -767,6 +864,243 @@ static const struct i2c_algorithm ft260_i2c_algo = {
.functionality = ft260_functionality,
};
+
+static int ft260_gpio_chip_match_name(struct gpio_chip *chip, void *data)
+{
+ return !strcmp(chip->label, data);
+}
+
+static void ft260_gpio_en_set(struct ft260_device *dev, u16 bitmap)
+{
+ dev->gpio_en |= bitmap & FT260_GPIO_MASK;
+}
+
+static void ft260_gpio_en_clr(struct ft260_device *dev, u16 bitmap)
+{
+ dev->gpio_en &= ~bitmap & FT260_GPIO_MASK;
+}
+
+static void ft260_gpio_en_update(struct hid_device *hdev, u8 req, u8 value)
+{
+ u16 bitmap;
+ struct ft260_device *dev = hid_get_drvdata(hdev);
+
+ switch (req) {
+ case FT260_SELECT_GPIO2_FUNC:
+ bitmap = FT260_GPIO_2;
+ break;
+ case FT260_SELECT_GPIOA_FUNC:
+ bitmap = FT260_GPIO_A;
+ break;
+ case FT260_SELECT_GPIOG_FUNC:
+ bitmap = FT260_GPIO_G;
+ break;
+ default:
+ return;
+ }
+
+ if (value == FT260_MFPIN_GPIO)
+ ft260_gpio_en_set(dev, bitmap);
+ else
+ ft260_gpio_en_clr(dev, bitmap);
+
+ hid_info(hdev, "enabled GPIOs: %04x\n", dev->gpio_en);
+}
+
+/*
+ * For GPIO, we use hid_hw_raw_request directly with preallocated buffer to not
+ * interfere with i2c operation.
+ */
+static void ft260_gpio_set(struct gpio_chip *gc, u32 offset, int value)
+{
+ int ret;
+ struct ft260_gpio_write_request_report *rep;
+ int len = sizeof(struct ft260_gpio_read_request_report);
+ struct ft260_device *dev = gpiochip_get_data(gc);
+ struct hid_device *hdev = dev->hdev;
+
+ if (offset >= FT260_GPIO_TOTAL) {
+ hid_err(hdev, "%s: invalid offset %d\n", __func__, offset);
+ return;
+ }
+
+ ft260_dbg("offset %d val %d\n", offset, value);
+
+ mutex_lock(&dev->lock);
+
+ if (!(dev->gpio_en & (1 << offset))) {
+ hid_err(hdev, "%s: wrong pin function %d\n", __func__, offset);
+ goto exit;
+ }
+
+ rep = (struct ft260_gpio_write_request_report *)&dev->feature_buf;
+
+ rep->gpio = dev->gpio;
+
+ if ( offset < FT260_GPIO_MAX) {
+ if (value)
+ rep->gpio.vals |= !!value << offset;
+ else
+ rep->gpio.vals &= ~(1 << offset);
+ } else {
+ offset = offset - FT260_GPIO_MAX;
+ if (value)
+ rep->gpio.ex_vals |= !!value << offset;
+ else
+ rep->gpio.ex_vals &= ~(1 << offset);
+ }
+
+ ft260_dbg("dirs %#02x vals %#02x ex_dir %#02x ex_vals %#02x\n",
+ rep->gpio.dirs, rep->gpio.vals,
+ rep->gpio.ex_dirs, rep->gpio.ex_vals);
+
+ ret = hid_hw_raw_request(hdev, FT260_GPIO, (u8 *)rep, len,
+ HID_FEATURE_REPORT, HID_REQ_SET_REPORT);
+ if (ret < 0) {
+ hid_err(hdev, "%s: error setting GPIO: %d\n", __func__, ret);
+ goto exit;
+ }
+
+ dev->gpio = rep->gpio;
+exit:
+ mutex_unlock(&dev->lock);
+}
+
+static int ft260_gpio_direction_set(struct gpio_chip *gc, u32 offset,
+ int value, int direction)
+{
+ int ret;
+ u8 *buf;
+ struct ft260_gpio_write_request_report *rep;
+ int len = sizeof(struct ft260_gpio_read_request_report);
+ struct ft260_device *dev = gpiochip_get_data(gc);
+ struct hid_device *hdev = dev->hdev;
+
+ if (offset >= FT260_GPIO_TOTAL) {
+ hid_err(hdev, "%s: invalid offset %d\n", __func__, offset);
+ return -EINVAL;
+ }
+
+ ft260_dbg("offset %d val %d direction %d\n", offset, value, direction);
+
+ mutex_lock(&dev->lock);
+
+ if (!(dev->gpio_en & (1 << offset))) {
+ hid_err(hdev, "%s: wrong pin function %d\n", __func__, offset);
+ ret = -EIO;
+ goto exit;
+ }
+
+ buf = (u8 *)&dev->feature_buf;
+
+ ret = hid_hw_raw_request(hdev, FT260_GPIO, buf, len,
+ HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
+ if (ret != len) {
+ ret = ret < 0 ? ret : -EIO;
+ hid_err(hdev, "%s: error getting GPIO: %d\n", __func__, ret);
+ goto exit;
+ }
+
+ rep = (struct ft260_gpio_write_request_report *)buf;
+ len = sizeof(struct ft260_gpio_write_request_report);
+
+ if (direction == FT260_GPIO_DIR_OUTPUT)
+ if ( offset < FT260_GPIO_MAX)
+ rep->gpio.dirs |= 1 << offset;
+ else
+ rep->gpio.ex_dirs |= 1 << (offset - FT260_GPIO_MAX);
+ else
+ if ( offset < FT260_GPIO_MAX)
+ rep->gpio.dirs &= ~(1 << offset);
+ else
+ rep->gpio.ex_dirs &= ~(1 << (offset - FT260_GPIO_MAX));
+
+ ft260_dbg("dirs %#02x val %#02x ex_dirs %#02x ex_vals %#02x\n",
+ rep->gpio.dirs, rep->gpio.vals,
+ rep->gpio.ex_dirs, rep->gpio.ex_vals);
+
+ ret = hid_hw_raw_request(hdev, FT260_GPIO, buf, len,
+ HID_FEATURE_REPORT, HID_REQ_SET_REPORT);
+
+ if (ret < 0) {
+ hid_err(hdev, "%s: error setting GPIO: %d\n", __func__, ret);
+ goto exit;
+ }
+
+ dev->gpio = rep->gpio;
+ mutex_unlock(&dev->lock);
+
+ if (direction == FT260_GPIO_DIR_OUTPUT)
+ ft260_gpio_set(gc, offset, value);
+
+ return 0;
+exit:
+ mutex_unlock(&dev->lock);
+ return ret;
+}
+
+static int ft260_gpio_direction_output(struct gpio_chip *gc,
+ u32 offset, int value)
+{
+ return ft260_gpio_direction_set(gc, offset, value,
+ FT260_GPIO_DIR_OUTPUT);
+}
+
+static int ft260_gpio_direction_input(struct gpio_chip *gc, u32 offset)
+{
+ return ft260_gpio_direction_set(gc, offset, 0,
+ FT260_GPIO_DIR_INPUT);
+}
+
+static int ft260_gpio_get_all(struct gpio_chip *gc, int item)
+{
+ int ret;
+ u8 *buf;
+ struct ft260_gpio_read_request_report *rep;
+ int len = sizeof(struct ft260_gpio_read_request_report);
+ struct ft260_device *dev = gpiochip_get_data(gc);
+ struct hid_device *hdev = dev->hdev;
+
+ buf = (u8 *)&dev->feature_buf;
+
+ mutex_lock(&dev->lock);
+ ret = hid_hw_raw_request(hdev, FT260_GPIO, buf, len,
+ HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
+
+ if (ret != len) {
+ ret = ret < 0 ? ret : -EIO;
+ hid_err(hdev, "%s: error getting GPIO: %d\n", __func__, ret);
+ goto exit;
+ }
+
+ rep = (struct ft260_gpio_read_request_report *)buf;
+ if (item == FT260_GPIO_VALUE)
+ ret = (rep->gpio.ex_vals << FT260_GPIO_MAX) |
+ rep->gpio.vals;
+ else
+ ret = (rep->gpio.ex_dirs << FT260_GPIO_MAX) |
+ rep->gpio.dirs;
+exit:
+ mutex_unlock(&dev->lock);
+ return ret;
+}
+
+static int ft260_gpio_get_direction(struct gpio_chip *gc, u32 offset)
+{
+ int ret = ft260_gpio_get_all(gc, FT260_GPIO_DIRECTION);
+ if (ret < 0)
+ return ret;
+ return (ret >> offset) & 1;
+}
+
+static int ft260_gpio_get(struct gpio_chip *gc, u32 offset)
+{
+ int ret = ft260_gpio_get_all(gc, FT260_GPIO_VALUE);
+ if (ret < 0)
+ return ret;
+ return (ret >> offset) & 1;
+}
+
static int ft260_get_system_config(struct hid_device *hdev,
struct ft260_get_system_status_report *cfg)
{
@@ -782,24 +1116,28 @@ static int ft260_get_system_config(struct hid_device *hdev,
return 0;
}
-static int ft260_is_interface_enabled(struct hid_device *hdev)
+static int ft260_is_interface_enabled(struct hid_device *hdev,
+ struct ft260_get_system_status_report *cfg)
{
- struct ft260_get_system_status_report cfg;
struct usb_interface *usbif = to_usb_interface(hdev->dev.parent);
int interface = usbif->cur_altsetting->desc.bInterfaceNumber;
int ret;
- ret = ft260_get_system_config(hdev, &cfg);
+ ret = ft260_get_system_config(hdev, cfg);
if (ret < 0)
return ret;
ft260_dbg("interface: 0x%02x\n", interface);
- ft260_dbg("chip mode: 0x%02x\n", cfg.chip_mode);
- ft260_dbg("clock_ctl: 0x%02x\n", cfg.clock_ctl);
- ft260_dbg("i2c_enable: 0x%02x\n", cfg.i2c_enable);
- ft260_dbg("uart_mode: 0x%02x\n", cfg.uart_mode);
-
- switch (cfg.chip_mode) {
+ ft260_dbg("chip mode: 0x%02x\n", cfg->chip_mode);
+ ft260_dbg("clock_ctl: 0x%02x\n", cfg->clock_ctl);
+ ft260_dbg("i2c_enable: 0x%02x\n", cfg->i2c_enable);
+ ft260_dbg("uart_mode: 0x%02x\n", cfg->uart_mode);
+ ft260_dbg("gpio2_func: 0x%02x\n", cfg->gpio2_func);
+ ft260_dbg("gpioA_func: 0x%02x\n", cfg->gpioa_func);
+ ft260_dbg("gpioG_func: 0x%02x\n", cfg->gpiog_func);
+ ft260_dbg("wakeup_int: 0x%02x\n", cfg->enable_wakeup_int);
+
+ switch (cfg->chip_mode) {
case FT260_MODE_ALL:
case FT260_MODE_BOTH:
if (interface == 1)
@@ -841,6 +1179,10 @@ static int ft260_word_show(struct hid_device *hdev, int id, u8 *cfg, int len,
return scnprintf(buf, PAGE_SIZE, "%d\n", le16_to_cpu(*field));
}
+static void ft260_attr_dummy_func(struct hid_device *hdev, u8 req, u16 value)
+{
+}
+
#define FT260_ATTR_SHOW(name, reptype, id, type, func) \
static ssize_t name##_show(struct device *kdev, \
struct device_attribute *attr, char *buf) \
@@ -861,37 +1203,42 @@ static int ft260_word_show(struct hid_device *hdev, int id, u8 *cfg, int len,
FT260_ATTR_SHOW(name, ft260_get_i2c_status_report, \
FT260_I2C_STATUS, __le16, ft260_word_show)
-#define FT260_ATTR_STORE(name, reptype, id, req, type, ctype, func) \
+#define FT260_ATTR_STORE(name, reptype, id, req, type, ctype, strtou, func) \
static ssize_t name##_store(struct device *kdev, \
struct device_attribute *attr, \
const char *buf, size_t count) \
{ \
struct reptype rep; \
struct hid_device *hdev = to_hid_device(kdev); \
+ struct ft260_device *dev = hid_get_drvdata(hdev); \
type name; \
int ret; \
\
- if (!func(buf, 10, (ctype *)&name)) { \
+ if (!strtou(buf, 10, (ctype *)&name)) { \
rep.name = name; \
rep.report = id; \
rep.request = req; \
+ mutex_lock(&dev->lock); \
ret = ft260_hid_feature_report_set(hdev, (u8 *)&rep, \
sizeof(rep)); \
- if (!ret) \
- ret = count; \
+ if (ret < 0) \
+ hid_err(hdev, "%s: failed!\n", __func__); \
+ else \
+ func(hdev, req, name); \
+ mutex_unlock(&dev->lock); \
} else { \
ret = -EINVAL; \
} \
return ret; \
}
-#define FT260_BYTE_ATTR_STORE(name, reptype, req) \
+#define FT260_BYTE_ATTR_STORE(name, reptype, req, func) \
FT260_ATTR_STORE(name, reptype, FT260_SYSTEM_SETTINGS, req, \
- u8, u8, kstrtou8)
+ u8, u8, kstrtou8, func)
-#define FT260_WORD_ATTR_STORE(name, reptype, req) \
+#define FT260_WORD_ATTR_STORE(name, reptype, req, func) \
FT260_ATTR_STORE(name, reptype, FT260_SYSTEM_SETTINGS, req, \
- __le16, u16, kstrtou16)
+ __le16, u16, kstrtou16, func)
FT260_SSTAT_ATTR_SHOW(chip_mode);
static DEVICE_ATTR_RO(chip_mode);
@@ -905,27 +1252,42 @@ static DEVICE_ATTR_RO(suspend_status);
FT260_SSTAT_ATTR_SHOW(hid_over_i2c_en);
static DEVICE_ATTR_RO(hid_over_i2c_en);
+FT260_SSTAT_ATTR_SHOW(gpio2_func);
+FT260_BYTE_ATTR_STORE(gpio2_func, ft260_set_gpio2_func_report,
+ FT260_SELECT_GPIO2_FUNC, ft260_gpio_en_update);
+static DEVICE_ATTR_RW(gpio2_func);
+
+FT260_SSTAT_ATTR_SHOW(gpioa_func);
+FT260_BYTE_ATTR_STORE(gpioa_func, ft260_set_gpioa_func_report,
+ FT260_SELECT_GPIOA_FUNC, ft260_gpio_en_update);
+static DEVICE_ATTR_RW(gpioa_func);
+
+FT260_SSTAT_ATTR_SHOW(gpiog_func);
+FT260_BYTE_ATTR_STORE(gpiog_func, ft260_set_gpiog_func_report,
+ FT260_SELECT_GPIOG_FUNC, ft260_gpio_en_update);
+static DEVICE_ATTR_RW(gpiog_func);
+
FT260_SSTAT_ATTR_SHOW(power_saving_en);
static DEVICE_ATTR_RO(power_saving_en);
FT260_SSTAT_ATTR_SHOW(i2c_enable);
FT260_BYTE_ATTR_STORE(i2c_enable, ft260_set_i2c_mode_report,
- FT260_SET_I2C_MODE);
+ FT260_SET_I2C_MODE, ft260_attr_dummy_func);
static DEVICE_ATTR_RW(i2c_enable);
FT260_SSTAT_ATTR_SHOW(uart_mode);
FT260_BYTE_ATTR_STORE(uart_mode, ft260_set_uart_mode_report,
- FT260_SET_UART_MODE);
+ FT260_SET_UART_MODE, ft260_attr_dummy_func);
static DEVICE_ATTR_RW(uart_mode);
FT260_SSTAT_ATTR_SHOW(clock_ctl);
FT260_BYTE_ATTR_STORE(clock_ctl, ft260_set_system_clock_report,
- FT260_SET_CLOCK);
+ FT260_SET_CLOCK, ft260_attr_dummy_func);
static DEVICE_ATTR_RW(clock_ctl);
FT260_I2CST_ATTR_SHOW(clock);
FT260_WORD_ATTR_STORE(clock, ft260_set_i2c_speed_report,
- FT260_SET_I2C_CLOCK_SPEED);
+ FT260_SET_I2C_CLOCK_SPEED, ft260_attr_dummy_func);
static DEVICE_ATTR_RW(clock);
static ssize_t i2c_reset_store(struct device *kdev,
@@ -949,6 +1311,9 @@ static const struct attribute_group ft260_attr_group = {
&dev_attr_hid_over_i2c_en.attr,
&dev_attr_power_saving_en.attr,
&dev_attr_i2c_enable.attr,
+ &dev_attr_gpio2_func.attr,
+ &dev_attr_gpioa_func.attr,
+ &dev_attr_gpiog_func.attr,
&dev_attr_uart_mode.attr,
&dev_attr_clock_ctl.attr,
&dev_attr_i2c_reset.attr,
@@ -961,6 +1326,8 @@ static int ft260_probe(struct hid_device *hdev, const struct hid_device_id *id)
{
struct ft260_device *dev;
struct ft260_get_chip_version_report version;
+ struct ft260_get_system_status_report cfg;
+ struct gpio_chip *chip;
int ret;
if (!hid_is_usb(hdev))
@@ -999,7 +1366,7 @@ static int ft260_probe(struct hid_device *hdev, const struct hid_device_id *id)
version.chip_code[0], version.chip_code[1],
version.chip_code[2], version.chip_code[3]);
- ret = ft260_is_interface_enabled(hdev);
+ ret = ft260_is_interface_enabled(hdev, &cfg);
if (ret <= 0)
goto err_hid_close;
@@ -1037,6 +1404,57 @@ static int ft260_probe(struct hid_device *hdev, const struct hid_device_id *id)
goto err_i2c_free;
}
+ chip = gpiochip_find(FT260_GPIOCHIP, ft260_gpio_chip_match_name);
+ if (chip)
+ return 0;
+
+ hid_info(hdev, "initialize gpio chip\n");
+
+ if (cfg.chip_mode) {
+ if (!(cfg.chip_mode & FT260_MODE_UART))
+ dev->gpio_en |= FT260_GPIO_UART_DEFAULT;
+
+ if (!(cfg.chip_mode & FT260_MODE_I2C))
+ dev->gpio_en |= FT260_GPIO_I2C_DEFAULT;
+ }
+
+ if (cfg.gpio2_func == FT260_MFPIN_GPIO)
+ dev->gpio_en |= FT260_GPIO_2;
+
+ if (cfg.enable_wakeup_int == FT260_MFPIN_GPIO)
+ dev->gpio_en |= FT260_GPIO_3;
+
+ if (cfg.gpioa_func == FT260_MFPIN_GPIO)
+ dev->gpio_en |= FT260_GPIO_A;
+
+ if (cfg.gpiog_func == FT260_MFPIN_GPIO)
+ dev->gpio_en |= FT260_GPIO_G;
+
+ hid_info(hdev, "enabled GPIOs: %04x\n", dev->gpio_en);
+
+ dev->gc = devm_kzalloc(&hdev->dev, sizeof(*dev->gc), GFP_KERNEL);
+ if (!dev->gc) {
+ ret = -ENOMEM;
+ goto err_i2c_free;
+ }
+
+ hid_set_drvdata(hdev, dev);
+
+ dev->gc->label = FT260_GPIOCHIP;
+ dev->gc->direction_input = ft260_gpio_direction_input;
+ dev->gc->direction_output = ft260_gpio_direction_output;
+ dev->gc->get_direction = ft260_gpio_get_direction;
+ dev->gc->set = ft260_gpio_set;
+ dev->gc->get = ft260_gpio_get;
+ dev->gc->base = -1;
+ dev->gc->ngpio = FT260_GPIO_TOTAL;
+ dev->gc->can_sleep = 1;
+ dev->gc->parent = &hdev->dev;
+
+ ret = devm_gpiochip_add_data(&hdev->dev, dev->gc, dev);
+ if (ret)
+ goto err_i2c_free;
+
return 0;
err_i2c_free:
--
2.34.1
^ permalink raw reply related
* Re: [PATCH v5 3/3] HID: cp2112: Fwnode Support
From: Andy Shevchenko @ 2023-02-11 12:09 UTC (permalink / raw)
To: Danny Kaehn
Cc: robh+dt, krzysztof.kozlowski+dt, jikos, benjamin.tissoires,
bartosz.golaszewski, dmitry.torokhov, devicetree, linux-input,
ethan.twardy
In-Reply-To: <20230210223638.12796-4-kaehndan@gmail.com>
On Fri, Feb 10, 2023 at 04:36:38PM -0600, Danny Kaehn wrote:
> Bind i2c and gpio interfaces to subnodes with names
> "i2c" and "gpio" if they exist, respectively. This
> allows the gpio and i2c controllers to be described
> in firmware as usual. Additionally, support configuring the
> i2c bus speed from the clock-frequency device property.
Entire series (code-wise, w/o DT bindings, not an expert there) looks good to
me, but one thing to address.
...
> + dev->gc.fwnode = device_get_named_child_node(&hdev->dev, "gpio");
Using like this bumps a reference count IIRC, so one need to drop it after use.
But please double check this.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v2 2/3] Input: add ABS_SND_PROFILE
From: Pavel Machek @ 2023-02-11 16:30 UTC (permalink / raw)
To: Gergo Koteles
Cc: Dmitry Torokhov, Jonathan Corbet, Andy Gross, Bjorn Andersson,
Konrad Dybcio, Rob Herring, Krzysztof Kozlowski, Jiri Kosina,
Benjamin Tissoires, linux-input, linux-kernel, linux-arm-msm,
devicetree, Caleb Connolly
In-Reply-To: <20230209232539.91500-1-soyer@irl.hu>
[-- Attachment #1: Type: text/plain, Size: 1398 bytes --]
Hi!
> ABS_SND_PROFILE used to describe the state of a multi-value sound profile
> switch. This will be used for the tri-state key on OnePlus phones or other
> phones.
Looks like ABS_PROFILE would be fine to use there, no need to create
new one, no?
Best regards,
Pavel
> Signed-off-by: Gergo Koteles <soyer@irl.hu>
> ---
> Documentation/input/event-codes.rst | 6 ++++++
> drivers/hid/hid-debug.c | 1 +
> include/uapi/linux/input-event-codes.h | 1 +
> 3 files changed, 8 insertions(+)
>
> diff --git a/Documentation/input/event-codes.rst b/Documentation/input/event-codes.rst
> index b4557462edd7..d43336e64d6a 100644
> --- a/Documentation/input/event-codes.rst
> +++ b/Documentation/input/event-codes.rst
> @@ -241,6 +241,12 @@ A few EV_ABS codes have special meanings:
> emitted only when the selected profile changes, indicating the newly
> selected profile value.
>
> +* ABS_SND_PROFILE:
> +
> + - Used to describe the state of a multi-value sound profile switch.
> + An event is emitted only when the selected profile changes,
> + indicating the newly selected profile value.
> +
> * ABS_MT_<name>:
>
> - Used to describe multitouch input events. Please see
> diff --git a/drivers/hid/hid-debug.c b/drivers/hid/hid-debug.c
--
People of Russia, stop Putin before his war on Ukraine escalates.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]
^ permalink raw reply
* Re: [PATCH v2 3/3] arm64: dts: qcom: sdm845-oneplus: add tri-state-key
From: Pavel Machek @ 2023-02-11 16:40 UTC (permalink / raw)
To: Gergo Koteles
Cc: Dmitry Torokhov, Andy Gross, Bjorn Andersson, Konrad Dybcio,
Rob Herring, Krzysztof Kozlowski, Jiri Kosina, Benjamin Tissoires,
linux-input, linux-kernel, linux-arm-msm, devicetree,
Caleb Connolly
In-Reply-To: <20230209232556.91554-1-soyer@irl.hu>
[-- Attachment #1: Type: text/plain, Size: 549 bytes --]
Hi!
> +++ b/arch/arm64/boot/dts/qcom/sdm845-oneplus-common.dtsi
> @@ -52,6 +52,43 @@ key-vol-up {
> };
> };
>
> + tri-state-key {
> + compatible = "gpio-keys";
> + label = "Tri-state key";
> + pinctrl-names = "default";
> + pinctrl-0 = <&tri_state_key_default>;
> + state-top {
> + label = "Tri-state key top";
"top/middle" is not too useful. Do we need the label at all? If so,
should it say "loud/vibrations only/mute"?
BR,
Pavel
--
People of Russia, stop Putin before his war on Ukraine escalates.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]
^ permalink raw reply
* Re: [PATCH 3/3] HID: apple-magic-backlight: Add driver for keyboard backlight on internal Magic Keyboards
From: Pavel Machek @ 2023-02-11 16:56 UTC (permalink / raw)
To: Aditya Garg
Cc: Jiri Kosina, jkosina@suse.cz, benjamin.tissoires@redhat.com,
Andy Shevchenko, andy.shevchenko@gmail.com, LKML,
linux-input@vger.kernel.org, ronald@innovation.ch,
kekrby@gmail.com, Orlando Chamberlain
In-Reply-To: <7D70F1FE-7F54-4D0A-8922-5466AA2AD364@live.com>
[-- Attachment #1: Type: text/plain, Size: 527 bytes --]
Hi!
> From: Orlando Chamberlain <orlandoch.dev@gmail.com>
>
> This driver adds support for the keyboard backlight on Intel T2 Macs
> with internal Magic Keyboards (MacBookPro16,x and MacBookAir9,1)
> + backlight->hdev = hdev;
> + backlight->cdev.name = "apple::kbd_backlight";
":white:kbd_backlight", plus document this in
Documentation/leds/well-known-leds.txt so that we keep it consistent
accross machines?
Thanks,
Pavel
--
People of Russia, stop Putin before his war on Ukraine escalates.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]
^ permalink raw reply
* Re: [PATCH v1 1/1] HID: ft260: add GPIO support
From: kernel test robot @ 2023-02-11 21:33 UTC (permalink / raw)
To: Michael Zaidman, jikos
Cc: oe-kbuild-all, linux-kernel, linux-input, Michael Zaidman
In-Reply-To: <20230211115752.26276-2-michael.zaidman@gmail.com>
Hi Michael,
I love your patch! Perhaps something to improve:
[auto build test WARNING on hid/for-next]
[also build test WARNING on linus/master v6.2-rc7 next-20230210]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Michael-Zaidman/HID-ft260-add-GPIO-support/20230211-195918
base: https://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git for-next
patch link: https://lore.kernel.org/r/20230211115752.26276-2-michael.zaidman%40gmail.com
patch subject: [PATCH v1 1/1] HID: ft260: add GPIO support
config: arm-randconfig-s041-20230210 (https://download.01.org/0day-ci/archive/20230212/202302120552.KrGf3GNw-lkp@intel.com/config)
compiler: arm-linux-gnueabi-gcc (GCC) 12.1.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# apt-get install sparse
# sparse version: v0.6.4-39-gce1a6720-dirty
# https://github.com/intel-lab-lkp/linux/commit/d3ddadc80df8c149f97dc8ab8fa51ba53f5638cb
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Michael-Zaidman/HID-ft260-add-GPIO-support/20230211-195918
git checkout d3ddadc80df8c149f97dc8ab8fa51ba53f5638cb
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=arm olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=arm SHELL=/bin/bash drivers/hid/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202302120552.KrGf3GNw-lkp@intel.com/
sparse warnings: (new ones prefixed by >>)
>> drivers/hid/hid-ft260.c:1289:1: sparse: sparse: incorrect type in argument 3 (different base types) @@ expected unsigned short [usertype] value @@ got restricted __le16 [addressable] [usertype] clock @@
drivers/hid/hid-ft260.c:1289:1: sparse: expected unsigned short [usertype] value
drivers/hid/hid-ft260.c:1289:1: sparse: got restricted __le16 [addressable] [usertype] clock
vim +1289 drivers/hid/hid-ft260.c
6a82582d9fa438 Michael Zaidman 2021-02-19 1287
6a82582d9fa438 Michael Zaidman 2021-02-19 1288 FT260_I2CST_ATTR_SHOW(clock);
6a82582d9fa438 Michael Zaidman 2021-02-19 @1289 FT260_WORD_ATTR_STORE(clock, ft260_set_i2c_speed_report,
d3ddadc80df8c1 Michael Zaidman 2023-02-11 1290 FT260_SET_I2C_CLOCK_SPEED, ft260_attr_dummy_func);
6a82582d9fa438 Michael Zaidman 2021-02-19 1291 static DEVICE_ATTR_RW(clock);
6a82582d9fa438 Michael Zaidman 2021-02-19 1292
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
^ permalink raw reply
* Re: [PATCH v3 1/2] HID: bigben: use spinlock to safely schedule workers
From: Pietro Borrello @ 2023-02-11 22:27 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Jiri Kosina, Hanno Zulla, Carlo Caione, Cristiano Giuffrida,
Bos, H.J., Jakob Koschel, Hillf Danton, Roderick Colenbrander,
linux-input, linux-kernel
In-Reply-To: <20230210142634.2exh6mvbvysy3hqo@mail.corp.redhat.com>
On Fri, 10 Feb 2023 at 15:26, Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
>
> [...]
> >
> > - if (bigben->removed || !report_field)
>
> You are removing an important test here: if (!report_field), please keep
> it.
To my understanding, that check was added in commit
918aa1ef104d ("HID: bigbenff: prevent null pointer dereference")
to prevent the NULL pointer crash, only fixing the crash point.
However, the true root cause was a missing check for output
reports patched in commit
c7bf714f8755 ("HID: check empty report_list in bigben_probe()"),
where the type-confused report list_entry was overlapping with
a NULL pointer, which was then causing the crash.
Let me know if there is any other path that may result in having a
report with no fields. In that case, it would make sense to keep the
check.
>
> > - return;
> > + spin_lock_irqsave(&bigben->lock, flags);
> >
> > if (bigben->work_led) {
> > bigben->work_led = false;
> > @@ -219,6 +229,8 @@ static void bigben_worker(struct work_struct *work)
> > report_field->value[7] = 0x00; /* padding */
> > hid_hw_request(bigben->hid, bigben->report, HID_REQ_SET_REPORT);
> > }
> > +
> > + spin_unlock_irqrestore(&bigben->lock, flags);
>
> Ouch, having hid_hw_request() called whithin a spinlock is definitely not
> something that should be done.
>
> However, the spinlock should be protecting 2 kinds of things:
> - any access to any value of struct bigben_device, but in an atomic way
> (i.e. copy everything you need locally in a spinlock, then release it
> and never read that struct again in that function).
> - the access to bigben->removed, which should be checked only in
> bigben_schedule_work() and in the .remove() function.
>
> Please note that this is what the playstation driver does: it prepares
> the report under the spinlock (which is really fast) before sending the
> report to the device which can be slow and be interrupted.
>
> With that being said, it is clear that we need 2 patches for this one:
> - the first one introduces the spinlock and protects the concurrent
> accesses to struct bigben_device (which is roughly everything below
> with the changes I just said)
> - the second one introduces bigben_schedule_work() and piggy backs on
> top of that new lock.
Thanks for clarifying. I will work on a v4 patch.
Best regards,
Pietro
^ permalink raw reply
* [PATCH] hid: bigben_probe(): validate report count
From: Pietro Borrello @ 2023-02-12 0:01 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Hanno Zulla
Cc: Jiri Kosina, linux-input, linux-kernel, Pietro Borrello
bigben_probe() does not validate that the output report has the
needed report values in the first field.
A malicious device registering a report with one field and a single
value causes an head OOB write in bigben_worker() when
accessing report_field->value[1] to report_field->value[7].
Use hid_validate_values() which takes care of all the needed checks.
Fixes: 256a90ed9e46 ("HID: hid-bigbenff: driver for BigBen Interactive PS3OFMINIPAD gamepad")
Signed-off-by: Pietro Borrello <borrello@diag.uniroma1.it>
---
drivers/hid/hid-bigbenff.c | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/drivers/hid/hid-bigbenff.c b/drivers/hid/hid-bigbenff.c
index e8b16665860d..1d0033a008ac 100644
--- a/drivers/hid/hid-bigbenff.c
+++ b/drivers/hid/hid-bigbenff.c
@@ -318,7 +318,6 @@ static int bigben_probe(struct hid_device *hid,
{
struct bigben_device *bigben;
struct hid_input *hidinput;
- struct list_head *report_list;
struct led_classdev *led;
char *name;
size_t name_sz;
@@ -343,14 +342,12 @@ static int bigben_probe(struct hid_device *hid,
return error;
}
- report_list = &hid->report_enum[HID_OUTPUT_REPORT].report_list;
- if (list_empty(report_list)) {
+ bigben->report = hid_validate_values(hid, HID_OUTPUT_REPORT, 0, 0, 8);
+ if (!bigben->report) {
hid_err(hid, "no output report found\n");
error = -ENODEV;
goto error_hw_stop;
}
- bigben->report = list_entry(report_list->next,
- struct hid_report, list);
if (list_empty(&hid->inputs)) {
hid_err(hid, "no inputs found\n");
---
base-commit: 4ec5183ec48656cec489c49f989c508b68b518e3
change-id: 20230211-bigben-oob-8dd66a3c3cb3
Best regards,
--
Pietro Borrello <borrello@diag.uniroma1.it>
^ permalink raw reply related
* Re: [PATCH v2 3/3] arm64: dts: qcom: sdm845-oneplus: add tri-state-key
From: Caleb Connolly @ 2023-02-12 1:58 UTC (permalink / raw)
To: Pavel Machek, Gergo Koteles
Cc: Dmitry Torokhov, Andy Gross, Bjorn Andersson, Konrad Dybcio,
Rob Herring, Krzysztof Kozlowski, Jiri Kosina, Benjamin Tissoires,
linux-input, linux-kernel, linux-arm-msm, devicetree
In-Reply-To: <Y+fE7gIMD4BDCffy@duo.ucw.cz>
On 11/02/2023 16:40, Pavel Machek wrote:
> Hi!
>
>> +++ b/arch/arm64/boot/dts/qcom/sdm845-oneplus-common.dtsi
>> @@ -52,6 +52,43 @@ key-vol-up {
>> };
>> };
>>
>> + tri-state-key {
>> + compatible = "gpio-keys";
>> + label = "Tri-state key";
>> + pinctrl-names = "default";
>> + pinctrl-0 = <&tri_state_key_default>;
>> + state-top {
>> + label = "Tri-state key top";
>
> "top/middle" is not too useful. Do we need the label at all? If so,
> should it say "loud/vibrations only/mute"?
"mute", "vibrate" and "ring" sound good to me.
Although it would be nice if users can easily map the physical key
position to the action when viewing the input device or remapping the
key in userspace.
Do you have any ideas or recommendations on how to do this?
>
> BR,
> Pavel
--
Kind Regards,
Caleb
^ permalink raw reply
* Re: [PATCH 3/3] HID: apple-magic-backlight: Add driver for keyboard backlight on internal Magic Keyboards
From: Orlando Chamberlain @ 2023-02-12 2:28 UTC (permalink / raw)
To: Pavel Machek
Cc: Aditya Garg, Jiri Kosina, jkosina@suse.cz,
benjamin.tissoires@redhat.com, Andy Shevchenko,
andy.shevchenko@gmail.com, LKML, linux-input@vger.kernel.org,
ronald@innovation.ch, kekrby@gmail.com
In-Reply-To: <Y+fIr5Fxozjkg6yo@duo.ucw.cz>
On Sat, 11 Feb 2023 17:56:15 +0100
Pavel Machek <pavel@ucw.cz> wrote:
> Hi!
>
> > From: Orlando Chamberlain <orlandoch.dev@gmail.com>
> >
> > This driver adds support for the keyboard backlight on Intel T2 Macs
> > with internal Magic Keyboards (MacBookPro16,x and MacBookAir9,1)
>
> > + backlight->hdev = hdev;
> > + backlight->cdev.name = "apple::kbd_backlight";
>
> ":white:kbd_backlight", plus document this in
> Documentation/leds/well-known-leds.txt so that we keep it consistent
> accross machines?
As in "apple:white:kbd_backlight" or just ":white:kbd_backlight"?
It seems like most examples I can see by grepping around use
"apple::kbd_backlight", "asus::kbd_backlight" etc, without any colour
information or anything between the two colons.
Would we want to start having the led names in new drivers (including
this one) as "$vendor:$colour:$location_or_description"?
I do notice you've also suggested doing that here
https://lore.kernel.org/all/Y+I7xNqkq%2FX6Lag+@duo.ucw.cz/, and that
conversation suggests adding the colour won't stop userspace tools from
finding the backlight, which is good.
One alternative would be to have the colour of the led stored in a
sysfs file in the led's sysfs folder, not sure if that'd be better (it
does mean the names stay the same), but also I'm not sure what the
empty space between the two colons in the current led names is there
for, if not colour.
>
> Thanks,
> Pavel
^ permalink raw reply
* Re: [PATCH 3/3] HID: apple-magic-backlight: Add driver for keyboard backlight on internal Magic Keyboards
From: Aditya Garg @ 2023-02-12 5:16 UTC (permalink / raw)
To: Pavel Machek
Cc: Jiri Kosina, jkosina@suse.cz, benjamin.tissoires@redhat.com,
Andy Shevchenko, andy.shevchenko@gmail.com, LKML,
linux-input@vger.kernel.org, ronald@innovation.ch,
kekrby@gmail.com, Orlando Chamberlain
In-Reply-To: <Y+fIr5Fxozjkg6yo@duo.ucw.cz>
> On 11-Feb-2023, at 10:26 PM, Pavel Machek <pavel@ucw.cz> wrote:
>
> Hi!
>
>> From: Orlando Chamberlain <orlandoch.dev@gmail.com>
>>
>> This driver adds support for the keyboard backlight on Intel T2 Macs
>> with internal Magic Keyboards (MacBookPro16,x and MacBookAir9,1)
>
>> + backlight->hdev = hdev;
>> + backlight->cdev.name = "apple::kbd_backlight";
>
> ":white:kbd_backlight", plus document this in
> Documentation/leds/well-known-leds.txt so that we keep it consistent
> accross machines?
We had used the same name for butterfly keyboards, the code being in hid-apple
Will that also be needed to be changed?
>
> Thanks,
> Pavel
> --
> People of Russia, stop Putin before his war on Ukraine escalates.
^ permalink raw reply
* Re: [PATCH 3/3] HID: apple-magic-backlight: Add driver for keyboard backlight on internal Magic Keyboards
From: Andy Shevchenko @ 2023-02-12 11:18 UTC (permalink / raw)
To: Aditya Garg
Cc: Jiri Kosina, jkosina@suse.cz, benjamin.tissoires@redhat.com, LKML,
linux-input@vger.kernel.org, ronald@innovation.ch,
kekrby@gmail.com, Orlando Chamberlain
In-Reply-To: <7D70F1FE-7F54-4D0A-8922-5466AA2AD364@live.com>
On Fri, Feb 10, 2023 at 03:45:15AM +0000, Aditya Garg wrote:
> From: Orlando Chamberlain <orlandoch.dev@gmail.com>
>
> This driver adds support for the keyboard backlight on Intel T2 Macs
> with internal Magic Keyboards (MacBookPro16,x and MacBookAir9,1)
...
> +#include <linux/hid.h>
> +#include <linux/usb.h>
Seems lack of some header inclusions, e.g. where struct led_classdev is defined
or -ERRNO codes.
> +#include "hid-ids.h"
...
> +static void apple_magic_backlight_power_set(struct apple_magic_backlight *backlight,
> + char power, char rate)
char is a beast, can we use u8 here and in similar cases?
...
> + /* Ensure this usb endpoint is for the keyboard backlight, not touchbar
> + * backlight.
> + */
/*
* Multi-line comment style
* goes like this.
*/
...
> + backlight = devm_kzalloc(&hdev->dev, sizeof(*backlight), GFP_KERNEL);
> +
Redundant blank line.
> + if (!backlight)
> + return -ENOMEM;
...
> +static struct hid_driver apple_magic_backlight_hid_driver = {
> + .name = "apple-magic-backlight",
> + .id_table = apple_magic_backlight_hid_ids,
> + .probe = apple_magic_backlight_probe,
> + .remove = apple_magic_backlight_remove,
> +};
> +
Redundant blank line.
> +module_hid_driver(apple_magic_backlight_hid_driver);
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: mtk-pmic-keys: Ignore power button if pressed before driver loads
From: Arınç ÜNAL @ 2023-02-12 11:29 UTC (permalink / raw)
To: Mattijs Korpershoek, Dmitry Torokhov, Matthias Brugger,
AngeloGioacchino Del Regno, Jonathan Cameron
Cc: linux-input, Linux ARM, moderated list:ARM/Mediatek SoC support,
linux-kernel, Frank Wunderlich, erkin.bozoglu
In-Reply-To: <87r0vcc51b.fsf@baylibre.com>
Hey Mattijs,
Sorry for the late response.
On 30.01.2023 20:21, Mattijs Korpershoek wrote:
> Hi Arınç,
>
> On lun., janv. 30, 2023 at 16:36, Arınç ÜNAL <arinc.unal@arinc9.com> wrote:
>
>> Hi all,
>>
>> The power button on my Bananapi BPI-R2 (MT7623NI SoC, mt6323-keys) is
>> shorted, so the device automatically boots when there's power. This
>> causes the device to reboot when KEYBOARD_MTK_PMIC is loaded because the
>> driver sees the power button being pressed.
>
> What evidence do you have that there is actually a "press" event being
> received by userspace? Did you tested this with evtest or something
> similar?
>
> If a "power button press" is generated, than I imagine that a userspace
> process must receive it and halt the system, right?
>
> The PMIC also has a feature to shutdown in case detect a long key-press,
> which is controlled by the mediatek,long-press-mode device-tree
> property.
> So is it the pmic that shutdown your board (probably no evidence in
> logs, just a "power cut" behaviour) or is it userspace?
Nothing appears on the kernel log and evtest doesn't detect anything.
The input device appears only after loading mtk-pmic-keys.ko so I have
to run evtest after the driver is loaded. After that, I see nothing
noticeable running evtest:
# evtest
No device specified, trying to scan all of /dev/input/event*
Available devices:
/dev/input/event0: mtk_cir
/dev/input/event1: mtk-pmic-keys
Select the device event number [0-1]: 1
Input driver version is 1.0.1
Input device ID: bus 0x19 vendor 0x1 product 0x1 version 0x1
Input device name: "mtk-pmic-keys"
Supported events:
Event type 0 (EV_SYN)
Event type 1 (EV_KEY)
Event code 114 (KEY_VOLUMEDOWN)
Event code 116 (KEY_POWER)
Properties:
Testing ... (interrupt to exit)
(Device reboots)
I've set this on the devicetree of Bananapi BPI-R2:
&mt6323keys {
mediatek,long-press-mode = <0>;
};
This prevents the device from rebooting after the driver is loaded so I
believe this proves that it's the driver that tries to shutdown the board.
>
>>
>> I was wondering if it's possible to change the driver in a way that
>> doesn't break in this situation. Maybe don't do anything if the first
>> state of the the power button the driver sees is being pressed, and if
>> the state doesn't change.
>
> If the driver is an issue, can't we blacklist it from being probed
> instead? or do you want to use the home key feature that that same
> driver provides?
Since it's a special case, specific to my own board, I just disabled the
key.
&mt6323keys {
power {
status = "disabled";
};
};
I also see there's no home key on this board so I may submit a patch to
disable the home button for this device.
Thanks for your help!
Cheers.
Arınç
^ permalink raw reply
* Re: [PATCH 1/3] HID: apple-ibridge: Add Apple iBridge HID driver for T1 chip.
From: Andy Shevchenko @ 2023-02-12 11:35 UTC (permalink / raw)
To: Aditya Garg
Cc: Jiri Kosina, jkosina@suse.cz, benjamin.tissoires@redhat.com, LKML,
linux-input@vger.kernel.org, ronald@innovation.ch,
kekrby@gmail.com, Orlando Chamberlain
In-Reply-To: <40274C3D-4F4F-479C-944C-EEBDC78F959C@live.com>
On Fri, Feb 10, 2023 at 03:43:24AM +0000, Aditya Garg wrote:
> From: Ronald Tschalär <ronald@innovation.ch>
>
> The iBridge device provides access to several devices, including:
> - the Touch Bar
> - the iSight webcam
> - the light sensor
> - the fingerprint sensor
>
> This driver provides the core support for managing the iBridge device
> and the access to the underlying devices. In particular, the
> functionality for the touch bar and light sensor is exposed via USB HID
> interfaces, and on devices with the T1 chip one of the HID devices is
> used for both functions. So this driver creates virtual HID devices, one
> per top-level report collection on each HID device (for a total of 3
> virtual HID devices). The sub-drivers then bind to these virtual HID
> devices.
>
> This way the Touch Bar and ALS drivers can be kept in their own modules,
> while at the same time making them look very much like as if they were
> connected to the real HID devices. And those drivers then work (mostly)
> without further changes on MacBooks with the T2 chip that don't need
> this driver.
...
> [Kerem Karabay: convert to a platform driver]
> [Kerem Karabay: fix appleib_forward_int_op]
> [Kerem Karabay: rely on HID core's parsing in appleib_add_device]
If somebody is going to update this (and update seems required for upstreaming)
the list of changes will grow. I suggest to consider Co-developed-by and move
these lines to cover-letter changelog.
> Signed-off-by: Kerem Karabay <kekrby@gmail.com>
...
> +#include <linux/platform_device.h>
> +#include <linux/acpi.h>
> +#include <linux/device.h>
> +#include <linux/hid.h>
> +#include <linux/list.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/usb.h>
Can we keep it sorted?
> +#include "hid-ids.h"
> +#include "../hid/usbhid/usbhid.h"
+ Blank line?
> +#include "apple-ibridge.h"
...
> +static struct hid_device_id appleib_sub_hid_ids[] = {
> + { HID_USB_DEVICE(USB_VENDOR_ID_LINUX_FOUNDATION,
> + USB_DEVICE_ID_IBRIDGE_TB) },
> + { HID_USB_DEVICE(USB_VENDOR_ID_LINUX_FOUNDATION,
> + USB_DEVICE_ID_IBRIDGE_ALS) },
> +};
> +
> +static struct {
> + unsigned int usage;
> + struct hid_device_id *dev_id;
> +} appleib_usage_map[] = {
> + /* Default iBridge configuration, key inputs and mode settings */
> + { 0x00010006, &appleib_sub_hid_ids[0] },
> + /* OS X iBridge configuration, digitizer inputs */
> + { 0x000D0005, &appleib_sub_hid_ids[0] },
> + /* All iBridge configurations, display/DFR settings */
> + { 0xFF120001, &appleib_sub_hid_ids[0] },
> + /* All iBridge configurations, ALS */
> + { 0x00200041, &appleib_sub_hid_ids[1] },
> +};
Shouldn't be other way around, i.e. via driver_data?
...
> +struct appleib_device {
> + acpi_handle asoc_socw;
> +};
What's the point of having struct out of a single member? Can you use it directly?
(you can try and see if it's not ugly, in some cases struct can be justified)
...
> + bool sub_open[ARRAY_SIZE(appleib_sub_hid_ids)];
Why not using bitmap?
DECLARE_BITMAP(sub_open, ARRAY_SIZE(...));
...
> +static __u8 *appleib_report_fixup(struct hid_device *hdev, __u8 *rdesc,
> + unsigned int *rsize)
Why __ types are in use? Is it part of ABI?
...
> +static int appleib_forward_int_op(struct hid_device *hdev,
> + int (*forward)(struct hid_driver *,
> + struct hid_device *, void *),
This can be on one line
> + void *args)
...
> + if (drv->suspend)
> + rc = drv->suspend(hdev, *(pm_message_t *)args);
This looks like a hack. What's going on here and why the pm_message_t is in
use? All new PM callbacks do not use it.
...
> + for (i = 0; i < ARRAY_SIZE(hdev_info->sub_hdevs); i++) {
> + /*
> + * hid_hw_open(), and hence appleib_ll_open(), is called
> + * from the driver's probe function, which in turn is called
> + * while adding the sub-hdev; but at this point we haven't yet
> + * added the sub-hdev to our list. So if we don't find the
> + * sub-hdev in our list assume it's in the process of being
> + * added and set the flag on the first unset sub-hdev.
> + */
> + if (hdev_info->sub_hdevs[i] == hdev ||
> + !hdev_info->sub_hdevs[i]) {
Unusual order of || operator arguments.
This will have a side effect, i.e. if hdev is equal to NULL it will go to the
true branch. Is it by design?
> + WRITE_ONCE(hdev_info->sub_open[i], open);
> + return 0;
> + }
> + }
...
> + while (i-- > 0)
while (i--) ?
> + hid_destroy_device(hdev_info->sub_hdevs[i]);
> + return (void *)hdev_info->sub_hdevs[i];
This casting is strange. And entire code piece. You will always return 0
element as a pointer here, why 'i'? Needs a lot of explanation.
...
> +static const struct hid_device_id appleib_hid_ids[] = {
> + { HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_IBRIDGE) },
> + { },
No comma for the terminator entry.
> +};
...
> +#ifdef CONFIG_PM
> + .suspend = appleib_hid_suspend,
> + .resume = appleib_hid_resume,
> + .reset_resume = appleib_hid_reset_resume,
> +#endif
Why not using
.driver = {
.pm = ...;
},
?
...
> + ret = hid_register_driver(&appleib_hid_driver);
> + if (ret) {
> + dev_err(&pdev->dev, "Error registering hid driver: %d\n",
> + ret);
> + return ret;
return dev_err_probe(...);
> + }
...
> +static int appleib_suspend(struct platform_device *pdev, pm_message_t message)
> +{
> + struct appleib_device *ib_dev;
> + int rc;
> + ib_dev = platform_get_drvdata(pdev);
Just unite it with the definition above.
Ditto for the similar cases here and there.
> + rc = acpi_execute_simple_method(ib_dev->asoc_socw, NULL, 0);
> + if (ACPI_FAILURE(rc))
> + dev_warn(&pdev->dev, "SOCW(0) failed: %s\n",
> + acpi_format_exception(rc));
> +
> + return 0;
> +}
...
> +static const struct acpi_device_id appleib_acpi_match[] = {
> + { "APP7777", 0 },
> + { },
No comma for terminator entry.
> +};
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH 2/3] HID: apple-touchbar: Add driver for the Touch Bar on MacBook Pros
From: Andy Shevchenko @ 2023-02-12 11:56 UTC (permalink / raw)
To: Aditya Garg
Cc: Jiri Kosina, jkosina@suse.cz, benjamin.tissoires@redhat.com, LKML,
linux-input@vger.kernel.org, ronald@innovation.ch,
kekrby@gmail.com, Orlando Chamberlain
In-Reply-To: <868AA58D-2399-4E4A-A6C6-73F88DB13992@live.com>
On Fri, Feb 10, 2023 at 03:44:26AM +0000, Aditya Garg wrote:
> From: Ronald Tschalär <ronald@innovation.ch>
>
> This driver enables basic touch bar functionality: enabling it, switching
> between modes on FN key press, and dimming and turning the display
> off/on when idle/active.
...
> Signed-off-by: Ronald Tschalär <ronald@innovation.ch>
> [Kerem Karabay: use USB product IDs from hid-ids.h]
> [Kerem Karabay: use hid_hw_raw_request except when setting the touchbar mode on T1 Macs]
> [Kerem Karabay: update Kconfig description]
> Signed-off-by: Kerem Karabay <kekrby@gmail.com>
> [Orlando Chamberlain: add usage check to not bind to keyboard backlight interface]
> Signed-off-by: Orlando Chamberlain <orlandoch.dev@gmail.com>
> [Aditya Garg: check if apple-touchbar is enabled in the special driver list]
> [Aditya Garg: fix suspend on T2 Macs]
Are you going to use this as a changelog? Not okay for a list of changes.
Consider Co-developed-by and proper Changelog in the cover letter.
> Signed-off-by: Aditya Garg <gargaditya08@live.com>
...
> +config HID_APPLE_TOUCHBAR
> + tristate "Apple Touch Bar"
> + depends on USB_HID
> + help
> + Say Y here if you want support for the Touch Bar on x86
> + MacBook Pros.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called apple-touchbar.
Wrong indentation for the help description. Missing two spaces.
...
> +#define dev_fmt(fmt) "tb: " fmt
Do we really need this?
...
> +static ssize_t idle_timeout_show(struct device *dev,
> + struct device_attribute *attr, char *buf);
> +static ssize_t idle_timeout_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t size);
> +static ssize_t dim_timeout_show(struct device *dev,
> + struct device_attribute *attr, char *buf);
> +static ssize_t dim_timeout_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t size);
> +static ssize_t fnmode_show(struct device *dev, struct device_attribute *attr,
> + char *buf);
> +static ssize_t fnmode_store(struct device *dev, struct device_attribute *attr,
> + const char *buf, size_t size);
Make sure you will have no unnecessary forward declarations.
...
> +static struct attribute *appletb_attrs[] = {
> + &dev_attr_idle_timeout.attr,
> + &dev_attr_dim_timeout.attr,
> + &dev_attr_fnmode.attr,
> + NULL,
No comma for terminator entry.
> +};
...
> +static struct appletb_device *appletb_dev;
Why is it global?
...
> +static bool appletb_disable_autopm(struct hid_device *hdev)
> +{
> + int rc;
> +
> + rc = hid_hw_power(hdev, PM_HINT_FULLON);
> +
Redundant blank line and see below.
> + if (rc == 0)
> + return true;
> +
> + hid_err(hdev,
> + "Failed to disable auto-pm on touch bar device (%d)\n", rc);
> + return false;
if (rc)
hid_err(...);
return rc == 0;
> +}
...
> +static bool appletb_any_tb_key_pressed(struct appletb_device *tb_dev)
> +{
> + return !!memchr_inv(tb_dev->last_tb_keys_pressed, 0,
> + sizeof(tb_dev->last_tb_keys_pressed));
Sounds like last_tb_keys_pressed should be declared as a bitmap and hence
return !bitmap_empty(...);
> +}
...
> +static ssize_t idle_timeout_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct appletb_device *tb_dev = dev_get_drvdata(dev);
> +
> + return snprintf(buf, PAGE_SIZE, "%d\n", tb_dev->idle_timeout);
sysfs_emit().
> +}
...
> +static ssize_t idle_timeout_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t size)
> +{
> + struct appletb_device *tb_dev = dev_get_drvdata(dev);
> + long new;
> + int rc;
> +
> + rc = kstrtol(buf, 0, &new);
> + if (rc || new > INT_MAX || new < -2)
> + return -EINVAL;
Do not shadow the error code.
if (rc)
return rc;
Why do you use INT_MAX check with to-long conversion instead of simply calling
kstrtoint()?
> + appletb_set_idle_timeout(tb_dev, new);
> + appletb_update_touchbar(tb_dev, true);
> +
> + return size;
> +}
...
> +static ssize_t dim_timeout_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct appletb_device *tb_dev = dev_get_drvdata(dev);
> +
> + return snprintf(buf, PAGE_SIZE, "%d\n",
> + tb_dev->dim_to_is_calc ? -2 : tb_dev->dim_timeout);
sysfs_emit()
> +}
> +
> +static ssize_t dim_timeout_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t size)
> +{
As per above.
> +}
> +
> +static ssize_t fnmode_show(struct device *dev, struct device_attribute *attr,
> + char *buf)
> +{
As per above.
> +}
> +
> +static ssize_t fnmode_store(struct device *dev, struct device_attribute *attr,
> + const char *buf, size_t size)
> +{
As per above.
> +}
...
> +static int appletb_tb_key_to_slot(unsigned int code)
> +{
> + switch (code) {
> + case KEY_ESC:
> + return 0;
> + case KEY_F1:
> + case KEY_F2:
> + case KEY_F3:
> + case KEY_F4:
> + case KEY_F5:
> + case KEY_F6:
> + case KEY_F7:
> + case KEY_F8:
> + case KEY_F9:
> + case KEY_F10:
> + return code - KEY_F1 + 1;
> + case KEY_F11:
> + case KEY_F12:
> + return code - KEY_F11 + 11;
> + default:
> + return -1;
Use appropriate error code from errno.h.
> + }
> +}
...
> + { }, /* Terminating zero entry */
No comma.
...
> +static bool appletb_match_internal_device(struct input_handler *handler,
> + struct input_dev *inp_dev)
> +{
> + struct device *dev = &inp_dev->dev;
> +
> + if (inp_dev->id.bustype == BUS_SPI)
> + return true;
> +
> + /* in kernel: dev && !is_usb_device(dev) */
> + while (dev && !(dev->type && dev->type->name &&
> + !strcmp(dev->type->name, "usb_device")))
> + dev = dev->parent;
I believe we have some helpers to mach like this.
> + /*
> + * Apple labels all their internal keyboards and trackpads as such,
> + * instead of maintaining an ever expanding list of product-id's we
> + * just look at the device's product name.
> + */
> + if (dev)
> + return !!strstr(to_usb_device(dev)->product, "Internal Keyboard");
> +
> + return false;
> +}
...
> +static int appletb_probe(struct hid_device *hdev,
> + const struct hid_device_id *id)
Can be a single line.
...
> + rc = hid_parse(hdev);
> + if (rc) {
> + dev_err(tb_dev->log_dev, "hid parse failed (%d)\n", rc);
> + goto error;
return dev_err_probe(...);
(error label seems useless)
> + }
...
> + if ((hdev->product == USB_DEVICE_ID_APPLE_TOUCHBAR_BACKLIGHT) &&
> + !(hdev->collection && hdev->collection[0].usage ==
> + HID_USAGE_APPLE_APP)) {
Broken indentation.
> + return -ENODEV;
> + }
...
> + if (rc) {
> + dev_err(tb_dev->log_dev, "hw start failed (%d)\n", rc);
dev_err_probe()
It will unite the style of error messaging.
> + goto clear_iface_info;
> + }
> + rc = hid_hw_open(hdev);
> + if (rc) {
> + dev_err(tb_dev->log_dev, "hw open failed (%d)\n", rc);
Ditto. And so on.
> + goto stop_hid;
> + }
...
> + /* initialize sysfs attributes */
> + rc = sysfs_create_group(&tb_dev->mode_iface.hdev->dev.kobj,
> + &appletb_attr_group);
> + if (rc) {
> + dev_err(tb_dev->log_dev,
> + "Failed to create sysfs attributes (%d)\n", rc);
> + goto unreg_handler;
> + }
Can't you use .dev_groups?
> + }
...
> + /* MacBook Pro's 2018, 2019, with T2 chip: iBridge Display */
> + { HID_USB_DEVICE(USB_VENDOR_ID_APPLE,
> + USB_DEVICE_ID_APPLE_TOUCHBAR_DISPLAY) },
> + { },
No comma.
...
> +
Redundant blank line.
> +MODULE_DEVICE_TABLE(hid, appletb_hid_ids);
...
> +#ifdef CONFIG_PM
> + .suspend = appletb_suspend,
> + .reset_resume = appletb_reset_resume,
> +#endif
Why not using .driver.pm ?
...
> +module_init(appletb_init);
> +module_exit(appletb_exit);
Just move them closer to the implementation.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* [PATCH v4 0/5] HID: use spinlocks to safely schedule led workers
From: Pietro Borrello @ 2023-02-12 18:59 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Hanno Zulla, Hanno Zulla,
Greg Kroah-Hartman
Cc: Cristiano Giuffrida, Bos, H.J., Jakob Koschel, Jiri Kosina,
Roderick Colenbrander, linux-input, linux-kernel, Pietro Borrello
I noticed a recurring pattern is present in multiple hid devices in the
Linux tree, where the LED controller of a device schedules a work_struct
to interact with the hardware.
The work_struct is embedded in the device structure and thus, is freed
at device removal.
The issue is that a LED worker may be scheduled by a timer concurrently
with device removal, causing the work_struct to be accessed after having
been freed.
I was able to trigger the issue in hid-bigbenff.c and hid-asus.c
where the work_structs may be scheduled by the LED controller
while the device is disconnecting, triggering use-after-frees.
I can attach the reproducer, but it's very simple USB configuration,
using the /dev/raw-gadget interface with some more USB interactions
to manage LEDs configuration and pass checks in asus_kbd_init()
and asus_kbd_get_functions() in case of hid-asus.c.
I triggered the issue by connecting a device and immediately
disconnecting it, so that the remove function runs before the LED one
which remains pending.
I am attaching multiple patches for asus and bigben drivers.
The proposed patches introduce safe wrappers to schedule the workers
safely with several spinlocks checks.
I attach the (partial for brevity) ODEBUG dumps:
```hid-bigbenff.c
[ 37.803135][ T1170] usb 1-1: USB disconnect, device number 2
[ 37.827979][ T1170] ODEBUG: free active (active state 0) object
type: work_struct hint: bigben_worker+0x0/0x860
[ 37.829634][ T1170] WARNING: CPU: 0 PID: 1170 at
lib/debugobjects.c:505 debug_check_no_obj_freed+0x43a/0x630
[ 37.830904][ T1170] Modules linked in:
[ 37.831413][ T1170] CPU: 0 PID: 1170 Comm: kworker/0:3 Not tainted
6.1.0-rc4-dirty #43
[ 37.832465][ T1170] Hardware name: QEMU Standard PC (i440FX + PIIX,
1996), BIOS 1.13.0-1ubuntu1.1 04/01/2014
[ 37.833751][ T1170] Workqueue: usb_hub_wq hub_event
[ 37.834409][ T1170] RIP: 0010:debug_check_no_obj_freed+0x43a/0x630
[ 37.835218][ T1170] Code: 48 89 ef e8 28 82 58 ff 49 8b 14 24 4c 8b
45 00 48 c7 c7 40 5f 09 87 48 c7 c6 60 5b 09 87 89 d9 4d 89 f9 31 c0
e8 46 25 ef fe <0f> 0b 4c 8b 64 24 20 48 ba 00 00 00 00 00 fc ff df ff
05 4f 7c 17
[ 37.837667][ T1170] RSP: 0018:ffffc900006fee60 EFLAGS: 00010246
[ 37.838503][ T1170] RAX: 0d2d19ffcded3d00 RBX: 0000000000000000
RCX: ffff888117fc9b00
[ 37.839519][ T1170] RDX: 0000000000000000 RSI: 0000000000000000
RDI: 0000000000000000
[ 37.840570][ T1170] RBP: ffffffff86e88380 R08: ffffffff8130793b
R09: fffff520000dfd85
[ 37.841618][ T1170] R10: fffff520000dfd85 R11: 0000000000000000
R12: ffffffff87095fb8
[ 37.842649][ T1170] R13: ffff888117770ad8 R14: ffff888117770acc
R15: ffffffff852b7420
[ 37.843728][ T1170] FS: 0000000000000000(0000)
GS:ffff8881f6600000(0000) knlGS:0000000000000000
[ 37.844877][ T1170] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 37.845749][ T1170] CR2: 00007f992eaab380 CR3: 000000011834b000
CR4: 00000000001006f0
[ 37.846794][ T1170] Call Trace:
[ 37.847245][ T1170] <TASK>
[ 37.847643][ T1170] slab_free_freelist_hook+0x89/0x160
[ 37.848409][ T1170] ? devres_release_all+0x262/0x350
[ 37.849156][ T1170] __kmem_cache_free+0x71/0x110
[ 37.849829][ T1170] devres_release_all+0x262/0x350
[ 37.850478][ T1170] ? devres_release+0x90/0x90
[ 37.851118][ T1170] device_release_driver_internal+0x5e5/0x8a0
[ 37.851944][ T1170] bus_remove_device+0x2ea/0x400
[ 37.852611][ T1170] device_del+0x64f/0xb40
[ 37.853212][ T1170] ? kill_device+0x150/0x150
[ 37.853831][ T1170] ? print_irqtrace_events+0x1f0/0x1f0
[ 37.854564][ T1170] hid_destroy_device+0x66/0x100
[ 37.855226][ T1170] usbhid_disconnect+0x9a/0xc0
[ 37.855887][ T1170] usb_unbind_interface+0x1e1/0x890
```
``` hid-asus.c
[ 77.409878][ T1169] usb 1-1: USB disconnect, device number 2
[ 77.423606][ T1169] ODEBUG: free active (active state 0) object
type: work_struct hint: asus_kbd_backlight_work+0x0/0x2c0
[ 77.425222][ T1169] WARNING: CPU: 0 PID: 1169 at
lib/debugobjects.c:505 debug_check_no_obj_freed+0x43a/0x630
[ 77.426599][ T1169] Modules linked in:
[ 77.427322][ T1169] CPU: 0 PID: 1169 Comm: kworker/0:3 Not tainted
6.1.0-rc4-dirty #43
[ 77.428404][ T1169] Hardware name: QEMU Standard PC (i440FX + PIIX,
1996), BIOS 1.13.0-1ubuntu1.1 04/01/2014
[ 77.429644][ T1169] Workqueue: usb_hub_wq hub_event
[ 77.430296][ T1169] RIP: 0010:debug_check_no_obj_freed+0x43a/0x630
[ 77.431142][ T1169] Code: 48 89 ef e8 28 82 58 ff 49 8b 14 24 4c 8b
45 00 48 c7 c7 40 5f 09 87 48 c7 c6 60 5b 09 87 89 d9 4d 89 f9 31 c0
e8 46 25 ef fe <0f> 0b 4c 8b 64 24 20 48 ba 00 00 00 00 00 fc ff df ff
05 4f 7c 17
[ 77.433691][ T1169] RSP: 0018:ffffc9000069ee60 EFLAGS: 00010246
[ 77.434470][ T1169] RAX: b85d2b40c12d7600 RBX: 0000000000000000
RCX: ffff888117a78000
[ 77.435507][ T1169] RDX: 0000000000000000 RSI: 0000000080000000
RDI: 0000000000000000
[ 77.436521][ T1169] RBP: ffffffff86e88380 R08: ffffffff8130793b
R09: ffffed103ecc4ed6
[ 77.437582][ T1169] R10: ffffed103ecc4ed6 R11: 0000000000000000
R12: ffffffff87095fb8
[ 77.438593][ T1169] R13: ffff88810e348fe0 R14: ffff88810e348fd4
R15: ffffffff852b5780
[ 77.439667][ T1169] FS: 0000000000000000(0000)
GS:ffff8881f6600000(0000) knlGS:0000000000000000
[ 77.440842][ T1169] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 77.441688][ T1169] CR2: 00007ffc05495ff0 CR3: 000000010cdf0000
CR4: 00000000001006f0
[ 77.442720][ T1169] Call Trace:
[ 77.443167][ T1169] <TASK>
[ 77.443555][ T1169] slab_free_freelist_hook+0x89/0x160
[ 77.444302][ T1169] ? devres_release_all+0x262/0x350
[ 77.444990][ T1169] __kmem_cache_free+0x71/0x110
[ 77.445638][ T1169] devres_release_all+0x262/0x350
[ 77.446309][ T1169] ? devres_release+0x90/0x90
[ 77.446978][ T1169] device_release_driver_internal+0x5e5/0x8a0
[ 77.447748][ T1169] bus_remove_device+0x2ea/0x400
[ 77.448421][ T1169] device_del+0x64f/0xb40
[ 77.448976][ T1169] ? kill_device+0x150/0x150
[ 77.449577][ T1169] ? print_irqtrace_events+0x1f0/0x1f0
[ 77.450307][ T1169] hid_destroy_device+0x66/0x100
[ 77.450938][ T1169] usbhid_disconnect+0x9a/0xc0
```
Signed-off-by: Pietro Borrello <borrello@diag.uniroma1.it>
---
Changes in v4:
- Split patches that add spinlock wrt the patches that check scheduling
- Smaller locked regions
- Link to v3: https://lore.kernel.org/r/20230125-hid-unregister-leds-v3-0-0a52ac225e00@diag.uniroma1.it
Changes in v3:
- use spinlocks to prevent workers scheduling
- drop patches on sony & playstation hid drivers
- Link to v2: https://lore.kernel.org/r/20230125-hid-unregister-leds-v2-0-689cc62fc878@diag.uniroma1.it
Changes in v2:
- dualshock4: Clarify UAF
- dualsense: Clarify UAF
- dualsense: Unregister multicolor led controller
- Link to v1: https://lore.kernel.org/r/20230125-hid-unregister-leds-v1-0-9a5192dcef16@diag.uniroma1.it
---
Pietro Borrello (5):
HID: bigben: use spinlock to protect concurrent accesses
HID: bigben_worker() remove unneeded check on report_field
HID: bigben: use spinlock to safely schedule workers
HID: asus: use spinlock to protect concurrent accesses
HID: asus: use spinlock to safely schedule workers
drivers/hid/hid-asus.c | 37 +++++++++++++++++++++----
drivers/hid/hid-bigbenff.c | 68 +++++++++++++++++++++++++++++++++++++++++-----
2 files changed, 93 insertions(+), 12 deletions(-)
---
base-commit: 2241ab53cbb5cdb08a6b2d4688feb13971058f65
change-id: 20230125-hid-unregister-leds-4cbf67099e1d
Best regards,
--
Pietro Borrello <borrello@diag.uniroma1.it>
^ permalink raw reply
* [PATCH v4 3/5] HID: bigben: use spinlock to safely schedule workers
From: Pietro Borrello @ 2023-02-12 19:00 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Hanno Zulla, Hanno Zulla,
Greg Kroah-Hartman
Cc: Cristiano Giuffrida, Bos, H.J., Jakob Koschel, Jiri Kosina,
Roderick Colenbrander, linux-input, linux-kernel, Pietro Borrello
In-Reply-To: <20230125-hid-unregister-leds-v4-0-7860c5763c38@diag.uniroma1.it>
Use spinlocks to deal with workers introducing a wrapper
bigben_schedule_work(), and several spinlock checks.
Otherwise, bigben_set_led() may schedule bigben->worker after the
structure has been freed, causing a use-after-free.
Fixes: 4eb1b01de5b9 ("HID: hid-bigbenff: fix race condition for scheduled work during removal")
Signed-off-by: Pietro Borrello <borrello@diag.uniroma1.it>
---
drivers/hid/hid-bigbenff.c | 18 ++++++++++++------
1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/drivers/hid/hid-bigbenff.c b/drivers/hid/hid-bigbenff.c
index b98c5f31c184..9d6560db762b 100644
--- a/drivers/hid/hid-bigbenff.c
+++ b/drivers/hid/hid-bigbenff.c
@@ -185,6 +185,15 @@ struct bigben_device {
struct work_struct worker;
};
+static inline void bigben_schedule_work(struct bigben_device *bigben)
+{
+ unsigned long flags;
+
+ spin_lock_irqsave(&bigben->lock, flags);
+ if (!bigben->removed)
+ schedule_work(&bigben->worker);
+ spin_unlock_irqrestore(&bigben->lock, flags);
+}
static void bigben_worker(struct work_struct *work)
{
@@ -197,9 +206,6 @@ static void bigben_worker(struct work_struct *work)
u32 len;
unsigned long flags;
- if (bigben->removed)
- return;
-
buf = hid_alloc_report_buf(bigben->report, GFP_KERNEL);
if (!buf)
return;
@@ -285,7 +291,7 @@ static int hid_bigben_play_effect(struct input_dev *dev, void *data,
bigben->work_ff = true;
spin_unlock_irqrestore(&bigben->lock, flags);
- schedule_work(&bigben->worker);
+ bigben_schedule_work(bigben);
}
return 0;
@@ -320,7 +326,7 @@ static void bigben_set_led(struct led_classdev *led,
if (work) {
bigben->work_led = true;
- schedule_work(&bigben->worker);
+ bigben_schedule_work(bigben);
}
return;
}
@@ -450,7 +456,7 @@ static int bigben_probe(struct hid_device *hid,
bigben->left_motor_force = 0;
bigben->work_led = true;
bigben->work_ff = true;
- schedule_work(&bigben->worker);
+ bigben_schedule_work(bigben);
hid_info(hid, "LED and force feedback support for BigBen gamepad\n");
--
2.25.1
^ permalink raw reply related
* [PATCH v4 2/5] HID: bigben_worker() remove unneeded check on report_field
From: Pietro Borrello @ 2023-02-12 19:00 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Hanno Zulla, Hanno Zulla,
Greg Kroah-Hartman
Cc: Cristiano Giuffrida, Bos, H.J., Jakob Koschel, Jiri Kosina,
Roderick Colenbrander, linux-input, linux-kernel, Pietro Borrello
In-Reply-To: <20230125-hid-unregister-leds-v4-0-7860c5763c38@diag.uniroma1.it>
bigben_worker() checks report_field to be non-NULL.
The check has been added in commit
918aa1ef104d ("HID: bigbenff: prevent null pointer dereference")
to prevent a NULL pointer crash.
However, the true root cause was a missing check for output
reports, patched in commit
c7bf714f8755 ("HID: check empty report_list in bigben_probe()"),
where the type-confused report list_entry was overlapping with
a NULL pointer, which was then causing the crash.
Fixes: 918aa1ef104d ("HID: bigbenff: prevent null pointer dereference")
Signed-off-by: Pietro Borrello <borrello@diag.uniroma1.it>
---
drivers/hid/hid-bigbenff.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/hid/hid-bigbenff.c b/drivers/hid/hid-bigbenff.c
index ed3d2d7bc1dd..b98c5f31c184 100644
--- a/drivers/hid/hid-bigbenff.c
+++ b/drivers/hid/hid-bigbenff.c
@@ -197,7 +197,7 @@ static void bigben_worker(struct work_struct *work)
u32 len;
unsigned long flags;
- if (bigben->removed || !report_field)
+ if (bigben->removed)
return;
buf = hid_alloc_report_buf(bigben->report, GFP_KERNEL);
--
2.25.1
^ permalink raw reply related
* [PATCH v4 1/5] HID: bigben: use spinlock to protect concurrent accesses
From: Pietro Borrello @ 2023-02-12 18:59 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Hanno Zulla, Hanno Zulla,
Greg Kroah-Hartman
Cc: Cristiano Giuffrida, Bos, H.J., Jakob Koschel, Jiri Kosina,
Roderick Colenbrander, linux-input, linux-kernel, Pietro Borrello
In-Reply-To: <20230125-hid-unregister-leds-v4-0-7860c5763c38@diag.uniroma1.it>
bigben driver has a worker that may access data concurrently.
Proct the accesses using a spinlock.
Fixes: 256a90ed9e46 ("HID: hid-bigbenff: driver for BigBen Interactive PS3OFMINIPAD gamepad")
Signed-off-by: Pietro Borrello <borrello@diag.uniroma1.it>
---
drivers/hid/hid-bigbenff.c | 52 ++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 50 insertions(+), 2 deletions(-)
diff --git a/drivers/hid/hid-bigbenff.c b/drivers/hid/hid-bigbenff.c
index e8b16665860d..ed3d2d7bc1dd 100644
--- a/drivers/hid/hid-bigbenff.c
+++ b/drivers/hid/hid-bigbenff.c
@@ -174,6 +174,7 @@ static __u8 pid0902_rdesc_fixed[] = {
struct bigben_device {
struct hid_device *hid;
struct hid_report *report;
+ spinlock_t lock;
bool removed;
u8 led_state; /* LED1 = 1 .. LED4 = 8 */
u8 right_motor_on; /* right motor off/on 0/1 */
@@ -190,12 +191,27 @@ static void bigben_worker(struct work_struct *work)
struct bigben_device *bigben = container_of(work,
struct bigben_device, worker);
struct hid_field *report_field = bigben->report->field[0];
+ bool do_work_led = false;
+ bool do_work_ff = false;
+ u8 *buf;
+ u32 len;
+ unsigned long flags;
if (bigben->removed || !report_field)
return;
+ buf = hid_alloc_report_buf(bigben->report, GFP_KERNEL);
+ if (!buf)
+ return;
+
+ len = hid_report_len(bigben->report);
+
+ /* LED work */
+ spin_lock_irqsave(&bigben->lock, flags);
+
if (bigben->work_led) {
bigben->work_led = false;
+ do_work_led = true;
report_field->value[0] = 0x01; /* 1 = led message */
report_field->value[1] = 0x08; /* reserved value, always 8 */
report_field->value[2] = bigben->led_state;
@@ -204,11 +220,22 @@ static void bigben_worker(struct work_struct *work)
report_field->value[5] = 0x00; /* padding */
report_field->value[6] = 0x00; /* padding */
report_field->value[7] = 0x00; /* padding */
- hid_hw_request(bigben->hid, bigben->report, HID_REQ_SET_REPORT);
+ hid_output_report(bigben->report, buf);
+ }
+
+ spin_unlock_irqrestore(&bigben->lock, flags);
+
+ if (do_work_led) {
+ hid_hw_raw_request(bigben->hid, bigben->report->id, buf, len,
+ bigben->report->type, HID_REQ_SET_REPORT);
}
+ /* FF work */
+ spin_lock_irqsave(&bigben->lock, flags);
+
if (bigben->work_ff) {
bigben->work_ff = false;
+ do_work_ff = true;
report_field->value[0] = 0x02; /* 2 = rumble effect message */
report_field->value[1] = 0x08; /* reserved value, always 8 */
report_field->value[2] = bigben->right_motor_on;
@@ -217,8 +244,17 @@ static void bigben_worker(struct work_struct *work)
report_field->value[5] = 0x00; /* padding */
report_field->value[6] = 0x00; /* padding */
report_field->value[7] = 0x00; /* padding */
- hid_hw_request(bigben->hid, bigben->report, HID_REQ_SET_REPORT);
+ hid_output_report(bigben->report, buf);
+ }
+
+ spin_unlock_irqrestore(&bigben->lock, flags);
+
+ if (do_work_ff) {
+ hid_hw_raw_request(bigben->hid, bigben->report->id, buf, len,
+ bigben->report->type, HID_REQ_SET_REPORT);
}
+
+ kfree(buf);
}
static int hid_bigben_play_effect(struct input_dev *dev, void *data,
@@ -228,6 +264,7 @@ static int hid_bigben_play_effect(struct input_dev *dev, void *data,
struct bigben_device *bigben = hid_get_drvdata(hid);
u8 right_motor_on;
u8 left_motor_force;
+ unsigned long flags;
if (!bigben) {
hid_err(hid, "no device data\n");
@@ -242,9 +279,12 @@ static int hid_bigben_play_effect(struct input_dev *dev, void *data,
if (right_motor_on != bigben->right_motor_on ||
left_motor_force != bigben->left_motor_force) {
+ spin_lock_irqsave(&bigben->lock, flags);
bigben->right_motor_on = right_motor_on;
bigben->left_motor_force = left_motor_force;
bigben->work_ff = true;
+ spin_unlock_irqrestore(&bigben->lock, flags);
+
schedule_work(&bigben->worker);
}
@@ -259,6 +299,7 @@ static void bigben_set_led(struct led_classdev *led,
struct bigben_device *bigben = hid_get_drvdata(hid);
int n;
bool work;
+ unsigned long flags;
if (!bigben) {
hid_err(hid, "no device data\n");
@@ -267,6 +308,7 @@ static void bigben_set_led(struct led_classdev *led,
for (n = 0; n < NUM_LEDS; n++) {
if (led == bigben->leds[n]) {
+ spin_lock_irqsave(&bigben->lock, flags);
if (value == LED_OFF) {
work = (bigben->led_state & BIT(n));
bigben->led_state &= ~BIT(n);
@@ -274,6 +316,7 @@ static void bigben_set_led(struct led_classdev *led,
work = !(bigben->led_state & BIT(n));
bigben->led_state |= BIT(n);
}
+ spin_unlock_irqrestore(&bigben->lock, flags);
if (work) {
bigben->work_led = true;
@@ -307,8 +350,12 @@ static enum led_brightness bigben_get_led(struct led_classdev *led)
static void bigben_remove(struct hid_device *hid)
{
struct bigben_device *bigben = hid_get_drvdata(hid);
+ unsigned long flags;
+ spin_lock_irqsave(&bigben->lock, flags);
bigben->removed = true;
+ spin_unlock_irqrestore(&bigben->lock, flags);
+
cancel_work_sync(&bigben->worker);
hid_hw_stop(hid);
}
@@ -362,6 +409,7 @@ static int bigben_probe(struct hid_device *hid,
set_bit(FF_RUMBLE, hidinput->input->ffbit);
INIT_WORK(&bigben->worker, bigben_worker);
+ spin_lock_init(&bigben->lock);
error = input_ff_create_memless(hidinput->input, NULL,
hid_bigben_play_effect);
--
2.25.1
^ permalink raw reply related
* [PATCH v4 4/5] HID: asus: use spinlock to protect concurrent accesses
From: Pietro Borrello @ 2023-02-12 19:00 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Hanno Zulla, Hanno Zulla,
Greg Kroah-Hartman
Cc: Cristiano Giuffrida, Bos, H.J., Jakob Koschel, Jiri Kosina,
Roderick Colenbrander, linux-input, linux-kernel, Pietro Borrello
In-Reply-To: <20230125-hid-unregister-leds-v4-0-7860c5763c38@diag.uniroma1.it>
asus driver has a worker that may access data concurrently.
Proct the accesses using a spinlock.
Fixes: af22a610bc38 ("HID: asus: support backlight on USB keyboards")
Signed-off-by: Pietro Borrello <borrello@diag.uniroma1.it>
---
drivers/hid/hid-asus.c | 22 +++++++++++++++++++++-
1 file changed, 21 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
index f99752b998f3..9f767baf39fb 100644
--- a/drivers/hid/hid-asus.c
+++ b/drivers/hid/hid-asus.c
@@ -98,6 +98,7 @@ struct asus_kbd_leds {
struct hid_device *hdev;
struct work_struct work;
unsigned int brightness;
+ spinlock_t lock;
bool removed;
};
@@ -495,7 +496,12 @@ static void asus_kbd_backlight_set(struct led_classdev *led_cdev,
{
struct asus_kbd_leds *led = container_of(led_cdev, struct asus_kbd_leds,
cdev);
+ unsigned long flags;
+
+ spin_lock_irqsave(&led->lock, flags);
led->brightness = brightness;
+ spin_unlock_irqrestore(&led->lock, flags);
+
schedule_work(&led->work);
}
@@ -503,8 +509,14 @@ static enum led_brightness asus_kbd_backlight_get(struct led_classdev *led_cdev)
{
struct asus_kbd_leds *led = container_of(led_cdev, struct asus_kbd_leds,
cdev);
+ enum led_brightness brightness;
+ unsigned long flags;
- return led->brightness;
+ spin_lock_irqsave(&led->lock, flags);
+ brightness = led->brightness;
+ spin_unlock_irqrestore(&led->lock, flags);
+
+ return brightness;
}
static void asus_kbd_backlight_work(struct work_struct *work)
@@ -512,11 +524,14 @@ static void asus_kbd_backlight_work(struct work_struct *work)
struct asus_kbd_leds *led = container_of(work, struct asus_kbd_leds, work);
u8 buf[] = { FEATURE_KBD_REPORT_ID, 0xba, 0xc5, 0xc4, 0x00 };
int ret;
+ unsigned long flags;
if (led->removed)
return;
+ spin_lock_irqsave(&led->lock, flags);
buf[4] = led->brightness;
+ spin_unlock_irqrestore(&led->lock, flags);
ret = asus_kbd_set_report(led->hdev, buf, sizeof(buf));
if (ret < 0)
@@ -584,6 +599,7 @@ static int asus_kbd_register_leds(struct hid_device *hdev)
drvdata->kbd_backlight->cdev.brightness_set = asus_kbd_backlight_set;
drvdata->kbd_backlight->cdev.brightness_get = asus_kbd_backlight_get;
INIT_WORK(&drvdata->kbd_backlight->work, asus_kbd_backlight_work);
+ spin_lock_init(&drvdata->kbd_backlight->lock);
ret = devm_led_classdev_register(&hdev->dev, &drvdata->kbd_backlight->cdev);
if (ret < 0) {
@@ -1119,9 +1135,13 @@ static int asus_probe(struct hid_device *hdev, const struct hid_device_id *id)
static void asus_remove(struct hid_device *hdev)
{
struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
+ unsigned long flags;
if (drvdata->kbd_backlight) {
+ spin_lock_irqsave(&drvdata->kbd_backlight->lock, flags);
drvdata->kbd_backlight->removed = true;
+ spin_unlock_irqrestore(&drvdata->kbd_backlight->lock, flags);
+
cancel_work_sync(&drvdata->kbd_backlight->work);
}
--
2.25.1
^ permalink raw reply related
* [PATCH v4 5/5] HID: asus: use spinlock to safely schedule workers
From: Pietro Borrello @ 2023-02-12 19:00 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Hanno Zulla, Hanno Zulla,
Greg Kroah-Hartman
Cc: Cristiano Giuffrida, Bos, H.J., Jakob Koschel, Jiri Kosina,
Roderick Colenbrander, linux-input, linux-kernel, Pietro Borrello
In-Reply-To: <20230125-hid-unregister-leds-v4-0-7860c5763c38@diag.uniroma1.it>
Use spinlocks to deal with workers introducing a wrapper
asus_schedule_work(), and several spinlock checks.
Otherwise, asus_kbd_backlight_set() may schedule led->work after the
structure has been freed, causing a use-after-free.
Fixes: af22a610bc38 ("HID: asus: support backlight on USB keyboards")
Signed-off-by: Pietro Borrello <borrello@diag.uniroma1.it>
---
drivers/hid/hid-asus.c | 15 +++++++++++----
1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
index 9f767baf39fb..d1094bb1aa42 100644
--- a/drivers/hid/hid-asus.c
+++ b/drivers/hid/hid-asus.c
@@ -491,6 +491,16 @@ static int rog_nkey_led_init(struct hid_device *hdev)
return ret;
}
+static void asus_schedule_work(struct asus_kbd_leds *led)
+{
+ unsigned long flags;
+
+ spin_lock_irqsave(&led->lock, flags);
+ if (!led->removed)
+ schedule_work(&led->work);
+ spin_unlock_irqrestore(&led->lock, flags);
+}
+
static void asus_kbd_backlight_set(struct led_classdev *led_cdev,
enum led_brightness brightness)
{
@@ -502,7 +512,7 @@ static void asus_kbd_backlight_set(struct led_classdev *led_cdev,
led->brightness = brightness;
spin_unlock_irqrestore(&led->lock, flags);
- schedule_work(&led->work);
+ asus_schedule_work(led);
}
static enum led_brightness asus_kbd_backlight_get(struct led_classdev *led_cdev)
@@ -526,9 +536,6 @@ static void asus_kbd_backlight_work(struct work_struct *work)
int ret;
unsigned long flags;
- if (led->removed)
- return;
-
spin_lock_irqsave(&led->lock, flags);
buf[4] = led->brightness;
spin_unlock_irqrestore(&led->lock, flags);
--
2.25.1
^ permalink raw reply related
* Re: [PATCH v2] HID: add KEY_CAMERA_FOCUS event in HID
From: qi feng @ 2023-02-13 6:09 UTC (permalink / raw)
To: jikos; +Cc: benjamin.tissoires, rydberg, linux-input, linux-kernel, fengqi
In-Reply-To: <CACOZ=ZU0zgRmoRu8X5bMUzUrXA9x-qoDJqrQroUs=+qKR58MQA@mail.gmail.com>
Thanks for your help to review
loop more
qi feng <fengqi706@gmail.com> 于2023年2月7日周二 10:35写道：
>
> Hi，
>
> Please help to review this v2 code
>
> thanks a lot
>
> Qi Feng <fengqi706@gmail.com> 于2023年1月31日周二 19:46写道：
> >
> > From: fengqi <fengqi@xiaomi.com>
> >
> > Our HID device need KEY_CAMERA_FOCUS event to control camera, but this
> > event is non-existent in current HID driver.we add this event in hid-input.c
> > We committed this v2 version following your previous suggestion
> >
> > Signed-off-by: fengqi <fengqi@xiaomi.com>
> > ---
> > drivers/hid/hid-input.c | 7 +++++++
> > include/linux/hid.h | 1 +
> > 2 files changed, 8 insertions(+)
> >
> > diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
> > index 9b59e436df0a..05fa3e191574 100644
> > --- a/drivers/hid/hid-input.c
> > +++ b/drivers/hid/hid-input.c
> > @@ -1221,6 +1221,13 @@ static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_fiel
> > return;
> > }
> > goto unknown;
> > + case HID_UP_CAMERA:
> > + switch (usage->hid & HID_USAGE){
> > + case 0x020: map_key_clear(KEY_CAMERA_FOCUS); break;
> > + case 0x021: map_key_clear(KEY_CAMERA); break;
> > + default: goto ignore;
> > + }
> > + break;
> >
> > case HID_UP_HPVENDOR: /* Reported on a Dutch layout HP5308 */
> > set_bit(EV_REP, input->evbit);
> > diff --git a/include/linux/hid.h b/include/linux/hid.h
> > index 8677ae38599e..e3daf2c7739c 100644
> > --- a/include/linux/hid.h
> > +++ b/include/linux/hid.h
> > @@ -155,6 +155,7 @@ struct hid_item {
> > #define HID_UP_DIGITIZER 0x000d0000
> > #define HID_UP_PID 0x000f0000
> > #define HID_UP_BATTERY 0x00850000
> > +#define HID_UP_CAMERA 0x00900000
> > #define HID_UP_HPVENDOR 0xff7f0000
> > #define HID_UP_HPVENDOR2 0xff010000
> > #define HID_UP_MSVENDOR 0xff000000
> > --
> > 2.39.0
> >
^ permalink raw reply
* Re: [PATCH v3 1/2] HID: bigben: use spinlock to safely schedule workers
From: Benjamin Tissoires @ 2023-02-13 8:25 UTC (permalink / raw)
To: Hillf Danton
Cc: Pietro Borrello, Jiri Kosina, Hanno Zulla, linux-input,
linux-kernel
In-Reply-To: <20230211020041.2613-1-hdanton@sina.com>
On Sat, Feb 11, 2023 at 3:01 AM Hillf Danton <hdanton@sina.com> wrote:
>
> On Fri, 10 Feb 2023 15:11:26 +0100 Benjamin Tissoires <benjamin.tissoires@redhat.com>
> > On Fri, Feb 10, 2023 at 2:24 PM Hillf Danton <hdanton@sina.com> wrote:
> > > On Thu, 09 Feb 2023 23:58:55 +0000 Pietro Borrello <borrello@diag.uniroma1.it>
> > > > Use spinlocks to deal with workers introducing a wrapper
> > > > bigben_schedule_work(), and several spinlock checks.
> > > > Otherwise, bigben_set_led() may schedule bigben->worker after the
> > > > structure has been freed, causing a use-after-free.
> > > >
> > > > Fixes: 4eb1b01de5b9 ("HID: hid-bigbenff: fix race condition for scheduled work during removal")
> > >
> > > Given the flag added in 4eb1b01de5b9 and the spinlock added in this
> > > patchset, devm_led_classdev_register() looks to not work for you.
> >
> > Actually, looking at the code now, it is clear that we need that lock.
> > The current code is happily changing the struct bigben_device from
> > multiple contexts, and pulls that without any barrier in the work
> > struct which should produce some interesting results :)
> >
> > And we can probably abuse that lock to prevent scheduling a new work
> > as it is done in hid-playstation.c
> >
> > I'll comment in the patch which parts need to be changed, because it
> > is true that this patch is definitely not mergeable as such and will
> > need another revision.
> >
> > >
> > > How about replacing the advanced devm_ method with the traditional plain
> > > pair of led_classdev_un/register(), with the flag mentioned cut off but
> > > without bothering to add another lock?
> > >
> >
> > As mentioned above, the lock is needed anyway, and will probably need
> > to be added in a separate patch.
> > Reverting to a non devm version of the led class would complexify the
> > driver for the error paths, and is probably not the best move IMO.
>
> After this patch,
>
> cpu 0 cpu 2
> --- ---
> bigben_remove()
> spin_lock_irqsave(&bigben->lock, flags);
> bigben->removed = true;
> spin_unlock_irqrestore(&bigben->lock, flags);
>
> spin_lock_irqsave(&bigben->lock, flags);
>
> what makes it safe for cpu2 to acquire lock after the removed flag is true?
The remove flag is just a way to prevent any other workqueue from
starting. It doesn't mean that the struct bigben has been freed, so
acquiring a lock at that point is fine.
We then rely on 2 things:
- devm_class_led to be released before struct bigben, because it was
devm-allocated *after* the struct bigben was devm-allocated
- we prevent any new workqueue to start and we guarantee that any
running workqueue is terminated before leaving the .remove function.
Given that the ledclass is gracefully shutting down all of its
potential queues, we don't have any other possibility to have an
unsafe call AFAIU.
Cheers,
Benjamin
^ permalink raw reply
* Re: [PATCH v3 1/2] HID: bigben: use spinlock to safely schedule workers
From: Benjamin Tissoires @ 2023-02-13 10:47 UTC (permalink / raw)
To: Hillf Danton
Cc: Pietro Borrello, Jiri Kosina, Hanno Zulla, linux-input,
linux-kernel
In-Reply-To: <20230213103658.3091-1-hdanton@sina.com>
On Mon, Feb 13, 2023 at 11:37 AM Hillf Danton <hdanton@sina.com> wrote:
>
> On Mon, 13 Feb 2023 09:25:37 +0100 Benjamin Tissoires <benjamin.tissoires@redhat.com>
> >
> > The remove flag is just a way to prevent any other workqueue from
> > starting. It doesn't mean that the struct bigben has been freed, so
> > acquiring a lock at that point is fine.
> > We then rely on 2 things:
> > - devm_class_led to be released before struct bigben, because it was
> > devm-allocated *after* the struct bigben was devm-allocated
>
> This is the current behavior and it is intact after this patch.
>
> > - we prevent any new workqueue to start and we guarantee that any
> > running workqueue is terminated before leaving the .remove function.
>
> If spinlock is added for not scheduling new workqueue then it is not
> needed, because the removed flag is set before running workqueue is
> terminated. Checking the flag is enough upon queuing new work.
>
I tend to disagree (based on Pietro's v4:
- no worker is running
- a led sysfs call is made
- the line "if (!bigben->removed)" is true
- this gets interrupted/or another CPU kicks in for the next one
-> .remove gets called
- bigben->removed is set to false
- cancel_work_sync() called
the led call continues, and schedules the work
.removes terminates, and devm kicks in, killing led_class and struct
bigben while the workqueue is running.
So having a simple spinlocks ensures the atomicity between checking
for bigben->removed and scheduling a workqueue.
Cheers,
Benjamin
^ 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