* [PATCH 4/6] tools/build: Allow versioning LLVM readelf
From: James Clark @ 2026-05-14 9:32 UTC (permalink / raw)
To: Nathan Chancellor, Nick Desaulniers, Bill Wendling, Justin Stitt,
Jiri Kosina, Benjamin Tissoires, Shuah Khan, Peter Zijlstra,
Ingo Molnar, Arnaldo Carvalho de Melo, Namhyung Kim, Mark Rutland,
Alexander Shishkin, Jiri Olsa, Ian Rogers, Adrian Hunter,
Andrii Nakryiko, Eduard Zingerman, Alexei Starovoitov,
Daniel Borkmann, Martin KaFai Lau, Kumar Kartikeya Dwivedi,
Song Liu, Yonghong Song
Cc: linux-kernel, llvm, linux-input, linux-kselftest, bpf,
linux-perf-users, James Clark, leo.yan
In-Reply-To: <20260514-james-perf-llvm-version-v1-0-6cac1a9a4c8d@linaro.org>
Documentation/kbuild/llvm.rst mentions that readelf is included in the
LLVM toolchain, but it's not currently included in this block.
Add it so that LLVM=... options also apply to readelf. Users in tools/
were Perf which was hardcoding it, and another was the BPF makefile.
Both already include Makefile.include so convert them to use the new
variable.
It also didn't have the cross compile prefix, so either readelf didn't
mind opening cross binaries, or it wasn't working for cross builds.
Signed-off-by: James Clark <james.clark@linaro.org>
---
tools/lib/bpf/Makefile | 8 ++++----
tools/perf/Makefile.perf | 1 -
tools/scripts/Makefile.include | 2 ++
3 files changed, 6 insertions(+), 5 deletions(-)
diff --git a/tools/lib/bpf/Makefile b/tools/lib/bpf/Makefile
index 168140f8e646..180dca9c57c8 100644
--- a/tools/lib/bpf/Makefile
+++ b/tools/lib/bpf/Makefile
@@ -114,12 +114,12 @@ PC_FILE := $(addprefix $(OUTPUT),$(PC_FILE))
TAGS_PROG := $(if $(shell which etags 2>/dev/null),etags,ctags)
-GLOBAL_SYM_COUNT = $(shell readelf -s --wide $(BPF_IN_SHARED) | \
+GLOBAL_SYM_COUNT = $(shell $(READELF) -s --wide $(BPF_IN_SHARED) | \
cut -d "@" -f1 | sed 's/_v[0-9]_[0-9]_[0-9].*//' | \
sed 's/\[.*\]//' | \
awk '/GLOBAL/ && /DEFAULT/ && !/UND|ABS/ {print $$NF}' | \
sort -u | wc -l)
-VERSIONED_SYM_COUNT = $(shell readelf --dyn-syms --wide $(OUTPUT)libbpf.so | \
+VERSIONED_SYM_COUNT = $(shell $(READELF) --dyn-syms --wide $(OUTPUT)libbpf.so | \
sed 's/\[.*\]//' | \
awk '/GLOBAL/ && /DEFAULT/ && !/UND|ABS/ {print $$NF}' | \
grep -Eo '[^ ]+@LIBBPF_' | cut -d@ -f1 | sort -u | wc -l)
@@ -182,12 +182,12 @@ check_abi: $(OUTPUT)libbpf.so $(VERSION_SCRIPT)
"versioned symbols in $^ ($(VERSIONED_SYM_COUNT))." \
"Please make sure all LIBBPF_API symbols are" \
"versioned in $(VERSION_SCRIPT)." >&2; \
- readelf -s --wide $(BPF_IN_SHARED) | \
+ $(READELF) -s --wide $(BPF_IN_SHARED) | \
cut -d "@" -f1 | sed 's/_v[0-9]_[0-9]_[0-9].*//' | \
sed 's/\[.*\]//' | \
awk '/GLOBAL/ && /DEFAULT/ && !/UND/ {print $$NF}'| \
sort -u > $(OUTPUT)libbpf_global_syms.tmp; \
- readelf --dyn-syms --wide $(OUTPUT)libbpf.so | \
+ $(READELF) --dyn-syms --wide $(OUTPUT)libbpf.so | \
sed 's/\[.*\]//' | \
awk '/GLOBAL/ && /DEFAULT/ && !/UND|ABS/ {print $$NF}'| \
grep -Eo '[^ ]+@LIBBPF_' | cut -d@ -f1 | \
diff --git a/tools/perf/Makefile.perf b/tools/perf/Makefile.perf
index 0aba14f22a06..63276bf55856 100644
--- a/tools/perf/Makefile.perf
+++ b/tools/perf/Makefile.perf
@@ -215,7 +215,6 @@ FLEX ?= flex
BISON ?= bison
STRIP = strip
AWK = awk
-READELF ?= readelf
# include Makefile.config by default and rule out
# non-config cases
diff --git a/tools/scripts/Makefile.include b/tools/scripts/Makefile.include
index e81e5b479c56..380ad84ac51e 100644
--- a/tools/scripts/Makefile.include
+++ b/tools/scripts/Makefile.include
@@ -73,6 +73,7 @@ ifneq ($(LLVM),)
$(call allow-override,LLC,$(LLVM_PREFIX)llc$(LLVM_SUFFIX))
$(call allow-override,LLVM_CONFIG,$(LLVM_PREFIX)llvm-config$(LLVM_SUFFIX))
$(call allow-override,LLVM_OBJCOPY,$(LLVM_PREFIX)llvm-objcopy$(LLVM_SUFFIX))
+ $(call allow-override,READELF,$(LLVM_PREFIX)llvm-readelf$(LLVM_SUFFIX))
else
# Allow setting various cross-compile vars or setting CROSS_COMPILE as a prefix.
$(call allow-override,CC,$(CROSS_COMPILE)gcc)
@@ -80,6 +81,7 @@ else
$(call allow-override,LD,$(CROSS_COMPILE)ld)
$(call allow-override,CXX,$(CROSS_COMPILE)g++)
$(call allow-override,STRIP,$(CROSS_COMPILE)strip)
+ $(call allow-override,READELF,$(CROSS_COMPILE)readelf)
# Host versions aren't prefixed
$(call allow-override,HOSTAR,ar)
--
2.34.1
^ permalink raw reply related
* [PATCH 5/6] tools/build: selftests: Allow versioning LLVM lld
From: James Clark @ 2026-05-14 9:32 UTC (permalink / raw)
To: Nathan Chancellor, Nick Desaulniers, Bill Wendling, Justin Stitt,
Jiri Kosina, Benjamin Tissoires, Shuah Khan, Peter Zijlstra,
Ingo Molnar, Arnaldo Carvalho de Melo, Namhyung Kim, Mark Rutland,
Alexander Shishkin, Jiri Olsa, Ian Rogers, Adrian Hunter,
Andrii Nakryiko, Eduard Zingerman, Alexei Starovoitov,
Daniel Borkmann, Martin KaFai Lau, Kumar Kartikeya Dwivedi,
Song Liu, Yonghong Song
Cc: linux-kernel, llvm, linux-input, linux-kselftest, bpf,
linux-perf-users, James Clark, leo.yan
In-Reply-To: <20260514-james-perf-llvm-version-v1-0-6cac1a9a4c8d@linaro.org>
Building with LLVM=... could result in a different version of lld being
used than the main toolchain for liburandom_read.so because it's
hardcoded to "lld" in this makefile.
Make it consistent with the rest of the LLVM toolchain by adding an LLD
variable to Makefile.include. Keep the fallback for other architectures
in tools/testing/selftests/bpf/Makefile as it seems like it's something
specific to this make rule and shouldn't be global.
Clang accepts either a full path or "ld.lld-15" style inputs to
-fuse-ld= so this will work with LLD defined the same way as the other
LLVM tools. However, for full paths, we need to use ".../ld.lld" instead
of the generic driver "lld", but I don't think the original use of "lld"
was significant as this is always a linux build.
Signed-off-by: James Clark <james.clark@linaro.org>
---
tools/scripts/Makefile.include | 2 ++
tools/testing/selftests/bpf/Makefile | 8 ++++----
2 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/tools/scripts/Makefile.include b/tools/scripts/Makefile.include
index 380ad84ac51e..5c2d505cba62 100644
--- a/tools/scripts/Makefile.include
+++ b/tools/scripts/Makefile.include
@@ -67,6 +67,7 @@ ifneq ($(LLVM),)
$(call allow-override,HOSTAR,$(LLVM_PREFIX)llvm-ar$(LLVM_SUFFIX))
$(call allow-override,LD,$(LLVM_PREFIX)ld.lld$(LLVM_SUFFIX))
$(call allow-override,HOSTLD,$(LLVM_PREFIX)ld.lld$(LLVM_SUFFIX))
+ $(call allow-override,LLD,$(LLVM_PREFIX)ld.lld$(LLVM_SUFFIX))
$(call allow-override,CXX,$(LLVM_PREFIX)clang++$(LLVM_SUFFIX))
$(call allow-override,STRIP,$(LLVM_PREFIX)llvm-strip$(LLVM_SUFFIX))
$(call allow-override,LLVM_STRIP,$(LLVM_PREFIX)llvm-strip$(LLVM_SUFFIX))
@@ -91,6 +92,7 @@ else
# Some tools still require Clang, LLC and/or LLVM utils
$(call allow-override,CLANG,clang)
$(call allow-override,LLC,llc)
+ $(call allow-override,LLD,ld.lld)
$(call allow-override,LLVM_CONFIG,llvm-config)
$(call allow-override,LLVM_OBJCOPY,llvm-objcopy)
$(call allow-override,LLVM_STRIP,llvm-strip)
diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index 6ef6872adbc3..44ba829e5d4d 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -245,9 +245,9 @@ $(OUTPUT)/%:%.c
# LLVM's ld.lld doesn't support all the architectures, so use it only on x86
ifeq ($(SRCARCH),$(filter $(SRCARCH),x86 riscv))
-LLD := lld
+USE_LD := $(LLD)
else
-LLD := $(shell command -v $(LD))
+USE_LD := $(shell command -v $(LD))
endif
# Filter out -static for liburandom_read.so and its dependent targets so that static builds
@@ -258,7 +258,7 @@ $(OUTPUT)/liburandom_read.so: urandom_read_lib1.c urandom_read_lib2.c liburandom
$(filter-out -static,$(CFLAGS) $(LDFLAGS)) \
$(filter %.c,$^) $(filter-out -static,$(LDLIBS)) \
-Wno-unused-command-line-argument \
- -fuse-ld=$(LLD) -Wl,-znoseparate-code -Wl,--build-id=sha1 \
+ -fuse-ld=$(USE_LD) -Wl,-znoseparate-code -Wl,--build-id=sha1 \
-Wl,--version-script=liburandom_read.map \
-fPIC -shared -o $@
@@ -268,7 +268,7 @@ $(OUTPUT)/urandom_read: urandom_read.c urandom_read_aux.c $(OUTPUT)/liburandom_r
$(filter-out -static,$(CFLAGS) $(LDFLAGS)) $(filter %.c,$^) \
-Wno-unused-command-line-argument \
-lurandom_read $(filter-out -static,$(LDLIBS)) -L$(OUTPUT) \
- -fuse-ld=$(LLD) -Wl,-znoseparate-code -Wl,--build-id=sha1 \
+ -fuse-ld=$(USE_LD) -Wl,-znoseparate-code -Wl,--build-id=sha1 \
-Wl,-rpath=. -o $@
$(OUTPUT)/sign-file: ../../../../scripts/sign-file.c
--
2.34.1
^ permalink raw reply related
* [PATCH 6/6] tools/build: selftests: Remove some duplicate toolchain definitions
From: James Clark @ 2026-05-14 9:32 UTC (permalink / raw)
To: Nathan Chancellor, Nick Desaulniers, Bill Wendling, Justin Stitt,
Jiri Kosina, Benjamin Tissoires, Shuah Khan, Peter Zijlstra,
Ingo Molnar, Arnaldo Carvalho de Melo, Namhyung Kim, Mark Rutland,
Alexander Shishkin, Jiri Olsa, Ian Rogers, Adrian Hunter,
Andrii Nakryiko, Eduard Zingerman, Alexei Starovoitov,
Daniel Borkmann, Martin KaFai Lau, Kumar Kartikeya Dwivedi,
Song Liu, Yonghong Song
Cc: linux-kernel, llvm, linux-input, linux-kselftest, bpf,
linux-perf-users, James Clark, leo.yan
In-Reply-To: <20260514-james-perf-llvm-version-v1-0-6cac1a9a4c8d@linaro.org>
Try to remove some, but not all duplicate toolchain definitions. In
these instances, their makefiles already include
tools/scripts/Makefile.include which defines these in a consistent way.
STRIP is the only one that was set with an '=', but I don't think it
was significant so that difference can be dropped.
Signed-off-by: James Clark <james.clark@linaro.org>
---
tools/bpf/resolve_btfids/Makefile | 3 ---
tools/lib/api/Makefile | 4 ----
tools/lib/subcmd/Makefile | 4 ----
tools/lib/symbol/Makefile | 4 ----
tools/perf/Makefile.perf | 6 ------
tools/testing/selftests/bpf/Makefile | 1 -
6 files changed, 22 deletions(-)
diff --git a/tools/bpf/resolve_btfids/Makefile b/tools/bpf/resolve_btfids/Makefile
index 7672208f65e4..6fdb6302e0a2 100644
--- a/tools/bpf/resolve_btfids/Makefile
+++ b/tools/bpf/resolve_btfids/Makefile
@@ -20,9 +20,6 @@ HOST_OVERRIDES := AR="$(HOSTAR)" CC="$(HOSTCC)" LD="$(HOSTLD)" ARCH="$(HOSTARCH)
CROSS_COMPILE="" CLANG_CROSS_FLAGS="" EXTRA_CFLAGS="$(HOSTCFLAGS)"
RM ?= rm
-HOSTCC ?= gcc
-HOSTLD ?= ld
-HOSTAR ?= ar
HOSTPKG_CONFIG ?= pkg-config
CROSS_COMPILE =
diff --git a/tools/lib/api/Makefile b/tools/lib/api/Makefile
index 8665c799e0fa..a228fdb5adba 100644
--- a/tools/lib/api/Makefile
+++ b/tools/lib/api/Makefile
@@ -9,10 +9,6 @@ srctree := $(patsubst %/,%,$(dir $(srctree)))
#$(info Determined 'srctree' to be $(srctree))
endif
-CC ?= $(CROSS_COMPILE)gcc
-AR ?= $(CROSS_COMPILE)ar
-LD ?= $(CROSS_COMPILE)ld
-
MAKEFLAGS += --no-print-directory
INSTALL = install
diff --git a/tools/lib/subcmd/Makefile b/tools/lib/subcmd/Makefile
index 8703ab487b68..9f1ddcf0504d 100644
--- a/tools/lib/subcmd/Makefile
+++ b/tools/lib/subcmd/Makefile
@@ -9,10 +9,6 @@ srctree := $(patsubst %/,%,$(dir $(srctree)))
#$(info Determined 'srctree' to be $(srctree))
endif
-CC ?= $(CROSS_COMPILE)gcc
-LD ?= $(CROSS_COMPILE)ld
-AR ?= $(CROSS_COMPILE)ar
-
RM = rm -f
MAKEFLAGS += --no-print-directory
diff --git a/tools/lib/symbol/Makefile b/tools/lib/symbol/Makefile
index 426b845edfac..d692abe8add6 100644
--- a/tools/lib/symbol/Makefile
+++ b/tools/lib/symbol/Makefile
@@ -9,10 +9,6 @@ srctree := $(patsubst %/,%,$(dir $(srctree)))
#$(info Determined 'srctree' to be $(srctree))
endif
-CC ?= $(CROSS_COMPILE)gcc
-AR ?= $(CROSS_COMPILE)ar
-LD ?= $(CROSS_COMPILE)ld
-
MAKEFLAGS += --no-print-directory
INSTALL = install
diff --git a/tools/perf/Makefile.perf b/tools/perf/Makefile.perf
index 63276bf55856..948abfd2ee8d 100644
--- a/tools/perf/Makefile.perf
+++ b/tools/perf/Makefile.perf
@@ -174,11 +174,6 @@ endef
LD += $(EXTRA_LDFLAGS)
-HOSTCC ?= gcc
-HOSTLD ?= ld
-HOSTAR ?= ar
-CLANG ?= clang
-
# Some distros provide the command $(CROSS_COMPILE)pkg-config for
# searching packges installed with Multiarch. Use it for cross
# compilation if it is existed.
@@ -213,7 +208,6 @@ FIND = find
INSTALL = install
FLEX ?= flex
BISON ?= bison
-STRIP = strip
AWK = awk
# include Makefile.config by default and rule out
diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index 44ba829e5d4d..b3e356c34479 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -3,7 +3,6 @@ include ../../../build/Build.include
include ../../../scripts/Makefile.arch
include ../../../scripts/Makefile.include
-CXX ?= $(CROSS_COMPILE)g++
OBJCOPY ?= $(CROSS_COMPILE)objcopy
CURDIR := $(abspath .)
--
2.34.1
^ permalink raw reply related
* Re: [PATCH v6 2/7] mfd: Add driver for ASUS Transformer embedded controller
From: Lee Jones @ 2026-05-14 10:02 UTC (permalink / raw)
To: Svyatoslav Ryhel
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Dmitry Torokhov,
Pavel Machek, Sebastian Reichel, Ion Agorria,
Michał Mirosław, devicetree, linux-kernel, linux-input,
linux-leds, linux-pm
In-Reply-To: <20260502124055.22475-3-clamor95@gmail.com>
On Sat, 02 May 2026, Svyatoslav Ryhel wrote:
> From: Michał Mirosław <mirq-linux@rere.qmqm.pl>
>
> Support Nuvoton NPCE795-based ECs as used in Asus Transformer TF201,
> TF300T, TF300TG, TF300TL and TF700T pad and dock, as well as TF101 dock
> and TF600T, P1801-T and TF701T pad. This is a glue driver handling
> detection and common operations for EC's functions.
>
> Co-developed-by: Svyatoslav Ryhel <clamor95@gmail.com>
> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> ---
> drivers/mfd/Kconfig | 14 +
> drivers/mfd/Makefile | 1 +
> drivers/mfd/asus-transformer-ec.c | 762 ++++++++++++++++++++++++
> include/linux/mfd/asus-transformer-ec.h | 162 +++++
> 4 files changed, 939 insertions(+)
> create mode 100644 drivers/mfd/asus-transformer-ec.c
> create mode 100644 include/linux/mfd/asus-transformer-ec.h
>
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 7192c9d1d268..5aa4facfd2df 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -137,6 +137,20 @@ config MFD_AAT2870_CORE
> additional drivers must be enabled in order to use the
> functionality of the device.
>
> +config MFD_ASUS_TRANSFORMER_EC
> + tristate "ASUS Transformer's embedded controller"
> + depends on I2C && OF
> + help
> + Support ECs found in ASUS Transformer's Pad and Mobile Dock.
> +
> + This provides shared glue for functional part drivers:
> + asus-transformer-ec-kbc, asus-transformer-ec-keys,
> + leds-asus-transformer-ec, asus-transformer-ec-battery
> + and asus-transformer-ec-charger.
A 760 line driver deserves more than just a list of leaf drivers.
> + This driver can also be built as a module. If so, the module
> + will be called asus-transformer-ec.
> +
> config MFD_AT91_USART
> tristate "AT91 USART Driver"
> select MFD_CORE
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index e75e8045c28a..fd80088d8a9a 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -10,6 +10,7 @@ obj-$(CONFIG_MFD_88PM805) += 88pm805.o 88pm80x.o
> obj-$(CONFIG_MFD_88PM886_PMIC) += 88pm886.o
> obj-$(CONFIG_MFD_ACT8945A) += act8945a.o
> obj-$(CONFIG_MFD_SM501) += sm501.o
> +obj-$(CONFIG_MFD_ASUS_TRANSFORMER_EC) += asus-transformer-ec.o
> obj-$(CONFIG_ARCH_BCM2835) += bcm2835-pm.o
> obj-$(CONFIG_MFD_BCM590XX) += bcm590xx.o
> obj-$(CONFIG_MFD_BD9571MWV) += bd9571mwv.o
> diff --git a/drivers/mfd/asus-transformer-ec.c b/drivers/mfd/asus-transformer-ec.c
> new file mode 100644
> index 000000000000..75aa7ab99387
> --- /dev/null
> +++ b/drivers/mfd/asus-transformer-ec.c
> @@ -0,0 +1,762 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +
> +#include <linux/array_size.h>
> +#include <linux/debugfs.h>
> +#include <linux/delay.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/device.h>
Should we sort the '#include' directives alphabetically? For instance,
'device.h' should typically appear before 'gpio/consumer.h'.
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/mfd/asus-transformer-ec.h>
> +#include <linux/mfd/core.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/property.h>
> +#include <linux/slab.h>
> +#include <linux/string.h>
> +#include <linux/types.h>
> +#include <linux/unaligned.h>
> +
> +#define ASUSEC_RSP_BUFFER_SIZE 8
> +
> +struct asus_ec_chip_data {
> + const char *name;
> + const struct mfd_cell *mfd_devices;
Use global `static const structs` instead.
Also, please do not pass MFD registration data through another
registration API like DT. It opens the gates to too much hackery. I'm
not saying that _this_ driver would do such a thing, but it's easier
just to keep the blanket rule in place.
> + unsigned int num_devices;
> + bool clr_fmode; /* clear Factory Mode bit in EC control register */
> +};
> +
> +struct asus_ec_data {
> + struct asusec_info info;
You have 'data' and 'info' which a) using non-forthcoming nomenclature
and doesn't tell me anything and then you b) put 'info' in the device's
driver_data attribute which is very confusing. driver_data should be
for what we call ddata which I assume is expressed as 'data' here.
> + struct mutex ecreq_lock; /* prevent simultaneous access */
> + struct gpio_desc *ecreq;
If I hadn't seen the declaration, I'd have no idea this was a GPIO
descriptor. Please improve the nomenclature throughout.
> + struct i2c_client *self;
Again, please use standard naming conventions:
% git grep "struct i2c_client" | grep "\*self" | wc -l
0
% git grep "struct i2c_client" | grep "\*client" | wc -l
6304
% git grep "struct i2c_client" | grep "\*i2c" | wc -l
903
> + const struct asus_ec_chip_data *data;
'data', 'priv' and 'info' should be improved.
> + char ec_data[DOCKRAM_ENTRY_BUFSIZE];
An array of chars called 'data'. This could be anything.
> + bool logging_disabled;
This debugging tool is probably never going to be used again.
Keep it local.
> +};
> +
> +struct dockram_ec_data {
> + struct mutex ctl_lock; /* prevent simultaneous access */
> + char ctl_data[DOCKRAM_ENTRY_BUFSIZE];
> +};
> +
> +#define to_ec_data(ec) \
> + container_of(ec, struct asus_ec_data, info)
> +
> +/**
> + * asus_dockram_read - Read a register from the DockRAM device.
> + * @client: Handle to the DockRAM device.
> + * @reg: Register to read.
> + * @buf: Byte array into which data will be read; must be large enough to
> + * hold the data returned by the DockRAM.
> + *
> + * This executes the DockRAM read based on the SMBus "block read" protocol
> + * or its emulation. It extracts DOCKRAM_ENTRY_SIZE bytes from the set
> + * register address.
> + *
> + * Returns a negative errno code else zero on success.
> + */
> +int asus_dockram_read(struct i2c_client *client, int reg, char *buf)
> +{
Have you considered using Regmap for register access instead of
implementing custom functions? Remaps already deals with caching and
locking mechanisms that you'd get for free.
This looks like it would be replaced with devm_regmap_init_i2c().
> + struct device *dev = &client->dev;
> + int ret;
> +
> + memset(buf, 0, DOCKRAM_ENTRY_BUFSIZE);
> + ret = i2c_smbus_read_i2c_block_data(client, reg,
> + DOCKRAM_ENTRY_BUFSIZE, buf);
> + if (ret < 0)
> + return ret;
> +
> + if (buf[0] > DOCKRAM_ENTRY_SIZE) {
> + dev_err(dev, "bad data len; buffer: %*ph; ret: %d\n",
> + DOCKRAM_ENTRY_BUFSIZE, buf, ret);
> + return -EPROTO;
> + }
> +
> + dev_dbg(dev, "got data; buffer: %*ph; ret: %d\n",
> + DOCKRAM_ENTRY_BUFSIZE, buf, ret);
Please remove all of these debug messages.
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(asus_dockram_read);
> +
> +/**
> + * asus_dockram_write - Write a byte array to a register of the DockRAM device.
> + * @client: Handle to the DockRAM device.
> + * @reg: Register to write to.
> + * @buf: Byte array to be written (up to DOCKRAM_ENTRY_SIZE bytes).
> + *
> + * This executes the DockRAM write based on the SMBus "block write"
> + * protocol or its emulation. It writes DOCKRAM_ENTRY_SIZE bytes to the
> + * specified register address.
> + *
> + * Returns a negative errno code else zero on success.
> + */
> +int asus_dockram_write(struct i2c_client *client, int reg, const char *buf)
> +{
> + if (buf[0] > DOCKRAM_ENTRY_SIZE)
> + return -EINVAL;
> +
> + dev_dbg(&client->dev, "sending data; buffer: %*ph\n", buf[0] + 1, buf);
> +
> + return i2c_smbus_write_i2c_block_data(client, reg, buf[0] + 1, buf);
> +}
> +EXPORT_SYMBOL_GPL(asus_dockram_write);
> +
> +/**
> + * asus_dockram_access_ctl - Read from or write to the DockRAM control register.
> + * @client: Handle to the DockRAM device.
> + * @out: Pointer to a variable where the register value will be stored.
> + * @mask: Bitmask of bits to be cleared.
> + * @xor: Bitmask of bits to be set (via XOR).
> + *
> + * This performs a control register read if @out is provided and both @mask
> + * and @xor are zero. Otherwise, it performs a control register update if
> + * @mask and @xor are provided.
> + *
> + * Returns a negative errno code else zero on success.
> + */
> +int asus_dockram_access_ctl(struct i2c_client *client, u64 *out, u64 mask,
> + u64 xor)
> +{
> + struct dockram_ec_data *priv = i2c_get_clientdata(client);
> + char *buf = priv->ctl_data;
> + u64 val;
> + int ret = 0;
> +
> + guard(mutex)(&priv->ctl_lock);
> +
> + ret = asus_dockram_read(client, ASUSEC_DOCKRAM_CONTROL, buf);
> + if (ret < 0)
Could we check for errors using 'if (ret)' instead of 'if (ret < 0)' here,
unless a positive return value has a specific meaning we need to handle?
> + goto exit;
> +
> + if (buf[0] != ASUSEC_CTL_SIZE) {
> + ret = -EPROTO;
> + goto exit;
> + }
> +
> + val = get_unaligned_le64(buf + 1);
> +
> + if (out)
> + *out = val;
> +
> + if (mask || xor) {
> + put_unaligned_le64((val & ~mask) ^ xor, buf + 1);
> + ret = asus_dockram_write(client, ASUSEC_DOCKRAM_CONTROL, buf);
> + }
> +
> +exit:
> + if (ret < 0)
> + dev_err(&client->dev, "Failed to access control flags: %d\n",
> + ret);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(asus_dockram_access_ctl);
> +
> +static void asus_ec_remove_notifier(struct device *dev, void *res)
> +{
> + struct asusec_info *ec = dev_get_drvdata(dev->parent);
> + struct notifier_block **nb = res;
> +
> + blocking_notifier_chain_unregister(&ec->notify_list, *nb);
> +}
> +
> +/**
> + * devm_asus_ec_register_notifier - Managed registration of notifier to an
> + * ASUS EC blocking notifier chain.
> + * @pdev: Device requesting the notifier (used for resource management).
> + * @nb: Notifier block to be registered.
> + *
> + * Register a notifier to the ASUS EC blocking notifier chain. The notifier
> + * will be automatically unregistered when the requesting device is detached.
> + *
> + * Return: 0 on success or a negative error code on failure.
> + */
> +int devm_asus_ec_register_notifier(struct platform_device *pdev,
> + struct notifier_block *nb)
> +{
Hand-rolling devres managed resources is usually reserved for subsystem
level API calls. Why do the usual device driver .remove() handling work
for you?
> + struct asusec_info *ec = dev_get_drvdata(pdev->dev.parent);
> + struct notifier_block **res;
> + int ret;
> +
> + res = devres_alloc(asus_ec_remove_notifier, sizeof(*res), GFP_KERNEL);
> + if (!res)
> + return -ENOMEM;
> +
> + *res = nb;
> + ret = blocking_notifier_chain_register(&ec->notify_list, nb);
> + if (ret) {
> + devres_free(res);
> + return ret;
> + }
> +
> + devres_add(&pdev->dev, res);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(devm_asus_ec_register_notifier);
> +
> +static int asus_ec_signal_request(const struct asusec_info *ec)
> +{
> + struct asus_ec_data *priv = to_ec_data(ec);
> +
> + guard(mutex)(&priv->ecreq_lock);
> +
> + dev_dbg(&priv->self->dev, "EC request\n");
> +
> + gpiod_set_value_cansleep(priv->ecreq, 1);
> + msleep(50);
> +
> + gpiod_set_value_cansleep(priv->ecreq, 0);
> + msleep(200);
> +
> + return 0;
> +}
> +
> +static int asus_ec_write(struct asus_ec_data *priv, u16 data)
> +{
> + int ret = i2c_smbus_write_word_data(priv->self, ASUSEC_WRITE_BUF, data);
> +
> + dev_dbg(&priv->self->dev, "EC write: %04x, ret = %d\n", data, ret);
> +
> + return ret;
> +}
> +
> +static int asus_ec_read(struct asus_ec_data *priv, bool in_irq)
> +{
> + int ret = i2c_smbus_read_i2c_block_data(priv->self, ASUSEC_READ_BUF,
> + sizeof(priv->ec_data),
> + priv->ec_data);
> +
> + dev_dbg(&priv->self->dev, "EC read: %*ph, ret = %d%s\n",
> + sizeof(priv->ec_data), priv->ec_data,
> + ret, in_irq ? "; in irq" : "");
> +
> + return ret;
> +}
No abstractions for the sake of it. All this goes away with Regmap anyway.
> +
> +/**
> + * asus_ec_i2c_command - Send a 16-bit command to the ASUS EC.
> + * @ec: Pointer to the shared ASUS EC structure.
> + * @data: The 16-bit command (word) to be sent.
> + *
> + * Return: 0 on success or a negative error code on failure.
> + */
> +int asus_ec_i2c_command(const struct asusec_info *ec, u16 data)
> +{
> + return asus_ec_write(to_ec_data(ec), data);
> +}
Is this wrapper function strictly necessary? We should try to avoid
superfluous abstractions that do nothing but call another function.
> +EXPORT_SYMBOL_GPL(asus_ec_i2c_command);
> +
> +static void asus_ec_clear_buffer(struct asus_ec_data *priv)
> +{
> + int retry = ASUSEC_RSP_BUFFER_SIZE;
> +
> + while (retry--) {
> + if (asus_ec_read(priv, false) < 0)
> + continue;
> +
> + if (priv->ec_data[1] & ASUSEC_OBF_MASK)
> + continue;
> +
> + break;
> + }
> +}
> +
> +static int asus_ec_log_info(struct asus_ec_data *priv, unsigned int reg,
> + const char *name, char **out)
> +{
> + char buf[DOCKRAM_ENTRY_BUFSIZE];
> + int ret;
> +
> + ret = asus_dockram_read(priv->info.dockram, reg, buf);
> + if (ret < 0)
> + return ret;
> +
> + if (!priv->logging_disabled)
> + dev_info(&priv->self->dev, "%-14s: %.*s\n", name,
> + buf[0], buf + 1);
> +
> + if (out)
> + *out = kstrndup(buf + 1, buf[0], GFP_KERNEL);
> +
> + return 0;
> +}
> +
> +static int asus_ec_reset(struct asus_ec_data *priv)
> +{
> + int retry, ret;
> +
> + for (retry = 0; retry < 3; retry++) {
> + ret = asus_ec_write(priv, 0);
> + if (!ret)
> + return 0;
> +
> + msleep(300);
> + }
> +
> + return ret;
> +}
> +
> +static int asus_ec_magic_debug(struct asus_ec_data *priv)
> +{
> + u64 flag;
> + int ret;
> +
> + ret = asus_ec_get_ctl(&priv->info, &flag);
> + if (ret < 0)
> + return ret;
> +
> + flag &= ASUSEC_CTL_SUSB_MODE;
> + dev_info(&priv->self->dev, "EC FW behaviour: %s\n",
> + flag ? "susb on when receive ec_req" :
> + "susb on when system wakeup");
> +
> + return 0;
> +}
> +
> +static int asus_ec_set_factory_mode(struct asus_ec_data *priv, bool on)
> +{
> + dev_info(&priv->self->dev, "Entering %s mode.\n", on ? "factory" :
> + "normal");
> +
> + return asus_ec_update_ctl(&priv->info, ASUSEC_CTL_FACTORY_MODE,
> + on ? ASUSEC_CTL_FACTORY_MODE : 0);
> +}
> +
> +static int asus_ec_detect(struct asus_ec_data *priv)
> +{
> + char *model = NULL;
> + int ret;
> +
> + ret = asus_ec_reset(priv);
> + if (ret)
> + goto err_exit;
> +
> + asus_ec_clear_buffer(priv);
> +
> + ret = asus_ec_log_info(priv, ASUSEC_DOCKRAM_INFO_MODEL, "model", &model);
> + if (ret)
> + goto err_exit;
> +
> + ret = asus_ec_log_info(priv, ASUSEC_DOCKRAM_INFO_FW, "FW version", NULL);
> + if (ret)
> + goto err_exit;
> +
> + ret = asus_ec_log_info(priv, ASUSEC_DOCKRAM_INFO_CFGFMT, "Config format", NULL);
> + if (ret)
> + goto err_exit;
> +
> + ret = asus_ec_log_info(priv, ASUSEC_DOCKRAM_INFO_HW, "HW version", NULL);
> + if (ret)
> + goto err_exit;
> +
> + priv->logging_disabled = true;
> +
> + ret = asus_ec_magic_debug(priv);
> + if (ret)
> + goto err_exit;
> +
> + priv->info.model = model;
> + priv->info.name = priv->data->name;
> +
> + if (priv->data->clr_fmode)
> + asus_ec_set_factory_mode(priv, false);
> +
> +err_exit:
> + if (ret)
> + dev_err(&priv->self->dev, "failed to access EC: %d\n", ret);
> +
> + return ret;
> +}
> +
> +static void asus_ec_handle_smi(struct asus_ec_data *priv, unsigned int code)
> +{
> + dev_dbg(&priv->self->dev, "SMI interrupt: 0x%02x\n", code);
> +
> + switch (code) {
> + case ASUSEC_SMI_HANDSHAKE:
> + case ASUSEC_SMI_RESET:
> + asus_ec_detect(priv);
> + break;
> + }
> +}
> +
> +static irqreturn_t asus_ec_interrupt(int irq, void *dev_id)
> +{
> + struct asus_ec_data *priv = dev_id;
> + unsigned long notify_action;
> + int ret;
> +
> + ret = asus_ec_read(priv, true);
> + if (ret <= 0 || !(priv->ec_data[1] & ASUSEC_OBF_MASK))
> + return IRQ_NONE;
> +
> + notify_action = priv->ec_data[1];
> + if (notify_action & ASUSEC_SMI_MASK) {
> + unsigned int code = priv->ec_data[2];
> +
> + asus_ec_handle_smi(priv, code);
> +
> + notify_action |= code << 8;
> + dev_dbg(&priv->self->dev, "SMI code: 0x%02x\n", code);
> + }
> +
> + blocking_notifier_call_chain(&priv->info.notify_list,
> + notify_action, priv->ec_data);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static ssize_t dockram_read(struct file *filp, char __user *buf,
> + size_t count, loff_t *ppos)
> +{
> + struct i2c_client *client = filp->private_data;
> + unsigned int reg, rsize;
> + ssize_t n_read = 0, val;
> + loff_t off = *ppos;
> + char *data;
> + int ret;
> +
> + reg = off / DOCKRAM_ENTRY_SIZE;
> + off %= DOCKRAM_ENTRY_SIZE;
> + rsize = DOCKRAM_ENTRIES * DOCKRAM_ENTRY_SIZE;
> +
> + if (!count)
> + return 0;
> +
> + data = kmalloc(DOCKRAM_ENTRY_BUFSIZE, GFP_KERNEL);
> +
> + while (reg < DOCKRAM_ENTRIES) {
> + unsigned int len = DOCKRAM_ENTRY_SIZE - off;
> +
> + if (len > rsize)
> + len = rsize;
> +
> + ret = asus_dockram_read(client, reg, data);
> + if (ret < 0) {
> + if (!n_read)
> + n_read = ret;
> + break;
> + }
> +
> + val = copy_to_user(buf, data + 1 + off, len);
> + if (val == len)
> + return -EFAULT;
> +
> + *ppos += len;
> + n_read += len;
> +
> + if (len == rsize)
> + break;
> +
> + rsize -= len;
> + buf += len;
> + off = 0;
> + ++reg;
> + }
> +
> + kfree(data);
> +
> + return n_read;
> +}
> +
> +static int dockram_write_one(struct i2c_client *client, int reg,
> + const char __user *buf, size_t count)
> +{
> + struct dockram_ec_data *priv = i2c_get_clientdata(client);
> + int ret;
> +
> + if (!count || count > DOCKRAM_ENTRY_SIZE)
> + return -EINVAL;
> + if (buf[0] != count - 1)
> + return -EINVAL;
> +
> + guard(mutex)(&priv->ctl_lock);
> +
> + priv->ctl_data[0] = (u8)count;
> + memcpy(priv->ctl_data + 1, buf, count);
> + ret = asus_dockram_write(client, reg, priv->ctl_data);
> +
> + return ret;
> +}
> +
> +static ssize_t dockram_write(struct file *filp, const char __user *buf,
> + size_t count, loff_t *ppos)
> +{
> + struct i2c_client *client = filp->private_data;
> + unsigned int reg;
> + loff_t off = *ppos;
> + int ret;
> +
> + if (off % DOCKRAM_ENTRY_SIZE != 0)
> + return -EINVAL;
> +
> + reg = off / DOCKRAM_ENTRY_SIZE;
> + if (reg >= DOCKRAM_ENTRIES)
> + return -EINVAL;
> +
> + ret = dockram_write_one(client, reg, buf, count);
> +
> + return ret < 0 ? ret : count;
> +}
> +
> +static const struct debugfs_short_fops dockram_fops = {
> + .read = dockram_read,
> + .write = dockram_write,
> + .llseek = default_llseek,
> +};
You should not be giving userspace free reign over device memory.
If you switch to Regmap, you can use regmap-debugfs.c for this.
> +static int control_reg_get(void *ec, u64 *val)
> +{
> + return asus_ec_get_ctl(ec, val);
> +}
> +
> +static int control_reg_set(void *ec, u64 val)
> +{
> + return asus_ec_update_ctl(ec, ~0ull, val);
> +}
> +
> +DEFINE_DEBUGFS_ATTRIBUTE(control_reg_fops, control_reg_get,
> + control_reg_set, "%016llx\n");
> +
> +static int ec_request_set(void *ec, u64 val)
> +{
> + if (val)
> + asus_ec_signal_request(ec);
> +
> + return 0;
> +}
> +
> +DEFINE_DEBUGFS_ATTRIBUTE(ec_request_fops, NULL, ec_request_set, "%llu\n");
> +
> +static int ec_irq_set(void *ec, u64 val)
> +{
> + struct asus_ec_data *priv = to_ec_data(ec);
> +
> + if (val)
> + irq_wake_thread(priv->self->irq, priv);
> +
> + return 0;
> +}
> +
> +DEFINE_DEBUGFS_ATTRIBUTE(ec_irq_fops, NULL, ec_irq_set, "%llu\n");
> +
> +static void asus_ec_debugfs_remove(void *debugfs_root)
> +{
> + debugfs_remove_recursive(debugfs_root);
> +}
> +
> +static void devm_asus_ec_debugfs_init(struct device *dev)
> +{
> + struct asusec_info *ec = dev_get_drvdata(dev);
> + struct asus_ec_data *priv = to_ec_data(ec);
> + struct dentry *debugfs_root, *dockram_dir;
> + char *name = devm_kasprintf(dev, GFP_KERNEL, "asus-ec-%s",
> + priv->data->name);
> +
> + debugfs_root = debugfs_create_dir(name, NULL);
> + dockram_dir = debugfs_create_dir("dockram", debugfs_root);
> +
> + debugfs_create_file("ec_irq", 0200, debugfs_root, ec,
> + &ec_irq_fops);
> + debugfs_create_file("ec_request", 0200, debugfs_root, ec,
> + &ec_request_fops);
> + debugfs_create_file("control_reg", 0644, dockram_dir, ec,
> + &control_reg_fops);
> + debugfs_create_file("dockram", 0644, dockram_dir,
> + priv->info.dockram, &dockram_fops);
> +
> + devm_add_action_or_reset(dev, asus_ec_debugfs_remove, debugfs_root);
> +}
Why is this being controlled via debugfs?
Use the correct kernel APIs instead.
If this is just for development, keep it and all of the extra verbose
printing in the BSP tree. It does not belong in the upstream kernel.
> +static void asus_ec_release_dockram_dev(void *client)
> +{
> + i2c_unregister_device(client);
> +}
> +
> +static struct i2c_client *devm_asus_dockram_get(struct device *dev)
> +{
> + struct i2c_client *parent = to_i2c_client(dev);
> + struct i2c_client *dockram;
> + struct dockram_ec_data *priv;
> + int ret;
> +
> + dockram = i2c_new_ancillary_device(parent, "dockram",
> + parent->addr + 2);
> + if (IS_ERR(dockram))
> + return dockram;
> +
> + ret = devm_add_action_or_reset(dev, asus_ec_release_dockram_dev,
> + dockram);
> + if (ret)
> + return ERR_PTR(ret);
> +
> + priv = devm_kzalloc(&dockram->dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return ERR_PTR(-ENOMEM);
> +
> + i2c_set_clientdata(dockram, priv);
> + mutex_init(&priv->ctl_lock);
> +
> + return dockram;
> +}
> +
> +static int asus_ec_probe(struct i2c_client *client)
> +{
> + struct device *dev = &client->dev;
> + struct asus_ec_data *priv;
Could we use a clearer, more specific name for the private data variable
instead of the generic term 'priv'? Using something like 'ddata' is usually
preferred.
> + int ret;
> +
> + if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_I2C_BLOCK))
> + return dev_err_probe(dev, -ENXIO,
> + "I2C bus is missing required SMBus block mode support\n");
> +
> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + priv->data = device_get_match_data(dev);
> + if (!priv->data)
> + return -ENODEV;
> +
> + i2c_set_clientdata(client, priv);
> + priv->self = client;
> +
> + priv->info.dockram = devm_asus_dockram_get(dev);
> + if (IS_ERR(priv->info.dockram))
> + return dev_err_probe(dev, PTR_ERR(priv->info.dockram),
> + "failed to get dockram\n");
> +
> + priv->ecreq = devm_gpiod_get(dev, "request", GPIOD_OUT_LOW);
> + if (IS_ERR(priv->ecreq))
> + return dev_err_probe(dev, PTR_ERR(priv->ecreq),
> + "failed to get request GPIO\n");
"get" or "request"
> + BLOCKING_INIT_NOTIFIER_HEAD(&priv->info.notify_list);
> + mutex_init(&priv->ecreq_lock);
> +
> + asus_ec_signal_request(&priv->info);
> +
> + ret = asus_ec_detect(priv);
> + if (ret)
> + return dev_err_probe(dev, ret, "failed to detect EC version\n");
> +
> + ret = devm_request_threaded_irq(dev, client->irq, NULL,
> + &asus_ec_interrupt,
> + IRQF_ONESHOT | IRQF_SHARED,
> + client->name, priv);
> + if (ret)
> + return dev_err_probe(dev, ret, "failed to register IRQ\n");
> +
> + /* Parent I2C controller uses DMA, ASUS EC and child devices do not */
> + client->dev.coherent_dma_mask = 0;
> + client->dev.dma_mask = &client->dev.coherent_dma_mask;
> +
> + if (IS_ENABLED(CONFIG_DEBUG_FS))
> + devm_asus_ec_debugfs_init(dev);
> +
> + return devm_mfd_add_devices(dev, 0, priv->data->mfd_devices,
> + priv->data->num_devices, NULL, 0, NULL);
> +}
> +
> +static const struct mfd_cell asus_ec_sl101_dock_mfd_devices[] = {
> + {
> + .name = "asus-transformer-ec-kbc",
> + },
> +};
> +
> +static const struct asus_ec_chip_data asus_ec_sl101_dock_data = {
> + .name = "dock",
> + .mfd_devices = asus_ec_sl101_dock_mfd_devices,
> + .num_devices = ARRAY_SIZE(asus_ec_sl101_dock_mfd_devices),
> + .clr_fmode = false,
> +};
> +
> +static const struct mfd_cell asus_ec_tf101_dock_mfd_devices[] = {
> + {
> + .name = "asus-transformer-ec-battery",
> + .id = 1,
Why doesn't PLATFORM_DEVID_AUTO already do the right thing?
> + }, {
> + .name = "asus-transformer-ec-charger",
> + .id = 1,
> + }, {
> + .name = "asus-transformer-ec-led",
> + .id = 1,
> + }, {
> + .name = "asus-transformer-ec-keys",
> + }, {
> + .name = "asus-transformer-ec-kbc",
> + },
> +};
> +
> +static const struct asus_ec_chip_data asus_ec_tf101_dock_data = {
> + .name = "dock",
> + .mfd_devices = asus_ec_tf101_dock_mfd_devices,
> + .num_devices = ARRAY_SIZE(asus_ec_tf101_dock_mfd_devices),
> + .clr_fmode = false,
> +};
> +
> +static const struct mfd_cell asus_ec_tf201_pad_mfd_devices[] = {
> + {
> + .name = "asus-transformer-ec-battery",
> + .id = 0,
> + }, {
> + .name = "asus-transformer-ec-led",
> + .id = 0,
> + },
> +};
> +
> +static const struct asus_ec_chip_data asus_ec_tf201_pad_data = {
> + .name = "pad",
> + .mfd_devices = asus_ec_tf201_pad_mfd_devices,
> + .num_devices = ARRAY_SIZE(asus_ec_tf201_pad_mfd_devices),
> + .clr_fmode = true,
> +};
> +
> +static const struct mfd_cell asus_ec_tf600t_pad_mfd_devices[] = {
> + {
> + .name = "asus-transformer-ec-battery",
> + .id = 0,
> + }, {
> + .name = "asus-transformer-ec-charger",
> + .id = 0,
> + }, {
> + .name = "asus-transformer-ec-led",
> + .id = 0,
> + },
> +};
> +
> +static const struct asus_ec_chip_data asus_ec_tf600t_pad_data = {
> + .name = "pad",
> + .mfd_devices = asus_ec_tf600t_pad_mfd_devices,
> + .num_devices = ARRAY_SIZE(asus_ec_tf600t_pad_mfd_devices),
> + .clr_fmode = true,
> +};
> +
> +static const struct of_device_id asus_ec_match[] = {
> + { .compatible = "asus,sl101-ec-dock", .data = &asus_ec_sl101_dock_data },
> + { .compatible = "asus,tf101-ec-dock", .data = &asus_ec_tf101_dock_data },
Could we use the match table's data field to store an 'enum' or integer ID
instead of passing platform data via the '.data' field? We could then use a
'switch' statement in the C code to select the correct 'mfd_cell' array.
> + { .compatible = "asus,tf201-ec-pad", .data = &asus_ec_tf201_pad_data },
> + { .compatible = "asus,tf600t-ec-pad", .data = &asus_ec_tf600t_pad_data },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, asus_ec_match);
> +
> +static struct i2c_driver asus_ec_driver = {
> + .driver = {
> + .name = "asus-transformer-ec",
> + .of_match_table = asus_ec_match,
> + },
> + .probe = asus_ec_probe,
> +};
> +module_i2c_driver(asus_ec_driver);
> +
> +MODULE_AUTHOR("Michał Mirosław <mirq-linux@rere.qmqm.pl>");
> +MODULE_AUTHOR("Svyatoslav Ryhel <clamor95@gmail.com>");
> +MODULE_DESCRIPTION("ASUS Transformer's EC driver");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/mfd/asus-transformer-ec.h b/include/linux/mfd/asus-transformer-ec.h
> new file mode 100644
> index 000000000000..0a72de40352e
> --- /dev/null
> +++ b/include/linux/mfd/asus-transformer-ec.h
> @@ -0,0 +1,162 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +#ifndef __MFD_ASUS_TRANSFORMER_EC_H
> +#define __MFD_ASUS_TRANSFORMER_EC_H
> +
> +#include <linux/notifier.h>
> +#include <linux/platform_device.h>
> +#include <linux/workqueue.h>
> +
> +struct i2c_client;
> +
> +struct asusec_info {
> + const char *model;
> + const char *name;
> + struct i2c_client *dockram;
> + struct workqueue_struct *wq;
> + struct blocking_notifier_head notify_list;
> +};
> +
> +#define DOCKRAM_ENTRIES 0x100
> +#define DOCKRAM_ENTRY_SIZE 32
> +#define DOCKRAM_ENTRY_BUFSIZE (DOCKRAM_ENTRY_SIZE + 1)
> +
> +/* interrupt sources */
> +#define ASUSEC_OBF_MASK BIT(0)
> +#define ASUSEC_KEY_MASK BIT(2)
> +#define ASUSEC_KBC_MASK BIT(3)
> +#define ASUSEC_AUX_MASK BIT(5)
> +#define ASUSEC_SCI_MASK BIT(6)
> +#define ASUSEC_SMI_MASK BIT(7)
> +
> +/* SMI notification codes */
> +#define ASUSEC_SMI_POWER_NOTIFY 0x31 /* [un]plugging USB cable */
> +#define ASUSEC_SMI_HANDSHAKE 0x50 /* response to ec_req edge */
> +#define ASUSEC_SMI_WAKE 0x53
> +#define ASUSEC_SMI_RESET 0x5f
> +#define ASUSEC_SMI_ADAPTER_EVENT 0x60 /* [un]plugging charger to dock */
> +#define ASUSEC_SMI_BACKLIGHT_ON 0x63
> +#define ASUSEC_SMI_AUDIO_DOCK_IN 0x70
> +
> +#define ASUSEC_SMI_ACTION(code) (ASUSEC_SMI_MASK | ASUSEC_OBF_MASK | \
> + (ASUSEC_SMI_##code << 8))
> +
> +/* control register [0x0a] layout */
> +#define ASUSEC_CTL_SIZE 8
> +
> +/*
> + * EC reports power from 40-pin connector in the LSB of the control
> + * register. The following values have been observed (xor 0x02):
> + *
> + * PAD-ec no-plug 0x40 / PAD-ec DOCK 0x20 / DOCK-ec no-plug 0x40
> + * PAD-ec AC 0x25 / PAD-ec DOCK+AC 0x24 / DOCK-ec AC 0x25
> + * PAD-ec USB 0x45 / PAD-ec DOCK+USB 0x24 / DOCK-ec USB 0x41
> + */
> +
> +#define ASUSEC_CTL_DIRECT_POWER_SOURCE BIT_ULL(0)
> +#define ASUSEC_STAT_CHARGING BIT_ULL(2)
> +#define ASUSEC_CTL_FULL_POWER_SOURCE BIT_ULL(5)
> +#define ASUSEC_CTL_SUSB_MODE BIT_ULL(9)
> +#define ASUSEC_CMD_SUSPEND_S3 BIT_ULL(33)
> +#define ASUSEC_CTL_TEST_DISCHARGE BIT_ULL(35)
> +#define ASUSEC_CMD_SUSPEND_INHIBIT BIT_ULL(37)
> +#define ASUSEC_CTL_FACTORY_MODE BIT_ULL(38)
> +#define ASUSEC_CTL_KEEP_AWAKE BIT_ULL(39)
> +#define ASUSEC_CTL_USB_CHARGE BIT_ULL(40)
> +#define ASUSEC_CTL_LED_BLINK BIT_ULL(40)
> +#define ASUSEC_CTL_LED_AMBER BIT_ULL(41)
> +#define ASUSEC_CTL_LED_GREEN BIT_ULL(42)
> +#define ASUSEC_CMD_SWITCH_HDMI BIT_ULL(56)
> +#define ASUSEC_CMD_WIN_SHUTDOWN BIT_ULL(62)
> +
> +#define ASUSEC_DOCKRAM_INFO_MODEL 0x01
> +#define ASUSEC_DOCKRAM_INFO_FW 0x02
> +#define ASUSEC_DOCKRAM_INFO_CFGFMT 0x03
> +#define ASUSEC_DOCKRAM_INFO_HW 0x04
> +#define ASUSEC_DOCKRAM_CONTROL 0x0a
> +#define ASUSEC_DOCKRAM_BATT_CTL 0x14
> +
> +#define ASUSEC_WRITE_BUF 0x64
> +#define ASUSEC_READ_BUF 0x6a
> +
> +/* dockram comm */
> +int asus_dockram_read(struct i2c_client *client, int reg, char *buf);
> +int asus_dockram_write(struct i2c_client *client, int reg, const char *buf);
> +int asus_dockram_access_ctl(struct i2c_client *client,
> + u64 *out, u64 mask, u64 xor);
> +
> +/* EC public API */
> +
> +/**
> + * cell_to_ec - Request the shared ASUS EC structure via a subdevice's pdev.
> + * @pdev: EC subdevice pdev requesting access to the shared ASUS EC structure.
> + *
> + * Returns a pointer to the asusec_info structure.
> + */
> +static inline struct asusec_info *cell_to_ec(struct platform_device *pdev)
> +{
> + return dev_get_drvdata(pdev->dev.parent);
> +}
This is both misleading and already exists.
> +
> +/**
> + * asus_ec_get_ctl - Read from the DockRAM control register.
> + * @ec: Pointer to the shared ASUS EC structure.
> + * @out: Pointer to the variable where the register value will be stored.
> + *
> + * Performs a control register read and stores the value in @out.
> + *
> + * Return: 0 on success, or a negative errno code on failure.
> + */
> +static inline int asus_ec_get_ctl(const struct asusec_info *ec, u64 *out)
> +{
> + return asus_dockram_access_ctl(ec->dockram, out, 0, 0);
> +}
> +
> +/**
> + * asus_ec_update_ctl - Update the DockRAM control register.
> + * @ec: Pointer to the shared ASUS EC structure.
> + * @mask: Bitmask of bits to be cleared.
> + * @xor: Bitmask of bits to be toggled or set (via XOR).
> + *
> + * Performs a read-modify-write update on the control register using
> + * the provided @mask and @xor values.
> + *
> + * Return: 0 on success, or a negative errno code on failure.
> + */
> +static inline int asus_ec_update_ctl(const struct asusec_info *ec,
> + u64 mask, u64 xor)
> +{
> + return asus_dockram_access_ctl(ec->dockram, NULL, mask, xor);
> +}
> +
> +/**
> + * asus_ec_set_ctl_bits - Sets bits of the DockRAM control register.
> + * @ec: Pointer to the shared ASUS EC structure.
> + * @mask: Bitmask of bits to be set.
> + *
> + * Sets bits of the control register using the provided @mask value.
> + *
> + * Return: 0 on success, or a negative errno code on failure.
> + */
> +static inline int asus_ec_set_ctl_bits(const struct asusec_info *ec, u64 mask)
> +{
> + return asus_dockram_access_ctl(ec->dockram, NULL, mask, mask);
> +}
> +
> +/**
> + * asus_ec_clear_ctl_bits - Clears bits of the DockRAM control register.
> + * @ec: Pointer to the shared ASUS EC structure.
> + * @mask: Bitmask of bits to be cleared.
> + *
> + * Clears bits of the control register using the provided @mask value.
> + *
> + * Return: 0 on success, or a negative errno code on failure.
> + */
> +static inline int asus_ec_clear_ctl_bits(const struct asusec_info *ec, u64 mask)
> +{
> + return asus_dockram_access_ctl(ec->dockram, NULL, mask, 0);
> +}
We don't typically allow abstraction for the sake of abstraction.
> +int asus_ec_i2c_command(const struct asusec_info *ec, u16 data);
> +int devm_asus_ec_register_notifier(struct platform_device *dev,
> + struct notifier_block *nb);
> +#endif /* __MFD_ASUS_TRANSFORMER_EC_H */
> --
> 2.51.0
>
>
--
Lee Jones
^ permalink raw reply
* Re: [PATCH 1/6] tools/build: Allow versioning of all LLVM tools defined in Makefile.include
From: bot+bpf-ci @ 2026-05-14 10:02 UTC (permalink / raw)
To: james.clark, nathan, nick.desaulniers+lkml, morbo, justinstitt,
jikos, bentiss, shuah, peterz, mingo, acme, namhyung,
mark.rutland, alexander.shishkin, jolsa, irogers, adrian.hunter,
andrii, eddyz87, ast, daniel, martin.lau, memxor, song,
yonghong.song
Cc: linux-kernel, llvm, linux-input, linux-kselftest, bpf,
linux-perf-users, james.clark, leo.yan, ast, andrii, daniel,
martin.lau, eddyz87, yonghong.song, clm, ihor.solodrai
In-Reply-To: <20260514-james-perf-llvm-version-v1-1-6cac1a9a4c8d@linaro.org>
[-- Attachment #1: Type: text/plain, Size: 1862 bytes --]
> tools/build: Allow versioning of all LLVM tools defined in Makefile.include
>
> The version of LLVM tools can be given on the build command with
> LLVM=-15, but this isn't applied to all tools. For example $(CC) gets
> versioned, but $(CLANG) doesn't. This causes a Perf build with LTO=1 to
> fail with an error about mixed clang versions:
>
> ld.lld: error: libperf/core.o: Unknown attribute kind (86)
> (Producer: 'LLVM18.1.8' Reader: 'LLVM 15.0.7')
>
> This file has two "ifneq ($(LLVM),)" blocks adjacent to each other, so
> merge these blocks making it obvious that all tools should be versioned
> consistently and there is nothing special about each block.
>
> This also reveals that ?= and "allow-override" are used inconsistently
> between the blocks. "allow-override" is technically only required for
> builtin variables, but isn't only used on them, and doesn't do any harm
> if used on a non-builtin. Make them all "allow-override" for
> consistency. The only functional difference this will cause is if there
> is a file level definition of one of the variables followed by an
> "#include of Makefile.include" which will now overwrite. But this isn't
> done and in a later commit some of the duplicate definitions will be
> removed for good measure.
>
> There are also some other LLVM tools that are not defined here and will
> be moved in a later commit.
>
> Signed-off-by: James Clark <james.clark@linaro.org>
This looks like a bug fix that corrects inconsistent LLVM tool versioning
introduced in an earlier commit. Should this include:
Fixes: e9c281928c24 ("kbuild: Make $(LLVM) more flexible")
---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md
CI run summary: https://github.com/kernel-patches/bpf/actions/runs/25853166705
^ permalink raw reply
* Re: [PATCH v6 5/7] leds: Add driver for ASUS Transformer LEDs
From: Lee Jones @ 2026-05-14 10:09 UTC (permalink / raw)
To: Svyatoslav Ryhel
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Dmitry Torokhov,
Pavel Machek, Sebastian Reichel, Ion Agorria,
Michał Mirosław, devicetree, linux-kernel, linux-input,
linux-leds, linux-pm
In-Reply-To: <20260502124055.22475-6-clamor95@gmail.com>
On Sat, 02 May 2026, Svyatoslav Ryhel wrote:
> From: Michał Mirosław <mirq-linux@rere.qmqm.pl>
>
> ASUS Transformer tablets have a green and an amber LED on both the Pad
> and the Dock. If both LEDs are enabled simultaneously, the emitted light
> will be yellow.
>
> Co-developed-by: Svyatoslav Ryhel <clamor95@gmail.com>
> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> ---
> drivers/leds/Kconfig | 11 ++++
> drivers/leds/Makefile | 1 +
> drivers/leds/leds-asus-transformer-ec.c | 79 +++++++++++++++++++++++++
> 3 files changed, 91 insertions(+)
> create mode 100644 drivers/leds/leds-asus-transformer-ec.c
>
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index f4a0a3c8c870..f637d23400a8 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -120,6 +120,17 @@ config LEDS_OSRAM_AMS_AS3668
> To compile this driver as a module, choose M here: the module
> will be called leds-as3668.
>
> +config LEDS_ASUS_TRANSFORMER_EC
> + tristate "LED Support for Asus Transformer charging LED"
> + depends on LEDS_CLASS
> + depends on MFD_ASUS_TRANSFORMER_EC
> + help
> + This option enables support for charging indicator on
> + Asus Transformer's Pad and it's Dock.
> +
> + To compile this driver as a module, choose M here: the module
> + will be called leds-asus-transformer-ec.
> +
> config LEDS_AW200XX
> tristate "LED support for Awinic AW20036/AW20054/AW20072/AW20108"
> depends on LEDS_CLASS
> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> index 8fdb45d5b439..d5395c3f1124 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -16,6 +16,7 @@ obj-$(CONFIG_LEDS_AN30259A) += leds-an30259a.o
> obj-$(CONFIG_LEDS_APU) += leds-apu.o
> obj-$(CONFIG_LEDS_ARIEL) += leds-ariel.o
> obj-$(CONFIG_LEDS_AS3668) += leds-as3668.o
> +obj-$(CONFIG_LEDS_ASUS_TRANSFORMER_EC) += leds-asus-transformer-ec.o
> obj-$(CONFIG_LEDS_AW200XX) += leds-aw200xx.o
> obj-$(CONFIG_LEDS_AW2013) += leds-aw2013.o
> obj-$(CONFIG_LEDS_BCM6328) += leds-bcm6328.o
> diff --git a/drivers/leds/leds-asus-transformer-ec.c b/drivers/leds/leds-asus-transformer-ec.c
> new file mode 100644
> index 000000000000..3186038e3be7
> --- /dev/null
> +++ b/drivers/leds/leds-asus-transformer-ec.c
> @@ -0,0 +1,79 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +#include <linux/err.h>
> +#include <linux/leds.h>
> +#include <linux/mfd/asus-transformer-ec.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +
> +static void asus_ec_led_set_brightness_amber(struct led_classdev *led,
> + enum led_brightness brightness)
> +{
> + const struct asusec_info *ec = dev_get_drvdata(led->dev->parent);
> +
> + if (brightness)
> + asus_ec_set_ctl_bits(ec, ASUSEC_CTL_LED_AMBER);
Why not Regmap?
> + else
> + asus_ec_clear_ctl_bits(ec, ASUSEC_CTL_LED_AMBER);
> +}
> +
> +static void asus_ec_led_set_brightness_green(struct led_classdev *led,
> + enum led_brightness brightness)
> +{
> + const struct asusec_info *ec = dev_get_drvdata(led->dev->parent);
> +
> + if (brightness)
> + asus_ec_set_ctl_bits(ec, ASUSEC_CTL_LED_GREEN);
> + else
> + asus_ec_clear_ctl_bits(ec, ASUSEC_CTL_LED_GREEN);
> +}
These are identical bar one variable.
> +static int asus_ec_led_probe(struct platform_device *pdev)
> +{
> + struct asusec_info *ec = cell_to_ec(pdev);
> + struct device *dev = &pdev->dev;
> + struct led_classdev *amber_led, *green_led;
> + int ret;
> +
> + platform_set_drvdata(pdev, ec);
This appears to be unused.
> + amber_led = devm_kzalloc(dev, sizeof(*amber_led), GFP_KERNEL);
> + if (!amber_led)
> + return -ENOMEM;
> +
> + amber_led->name = devm_kasprintf(dev, GFP_KERNEL, "%s::amber", ec->name);
> + amber_led->max_brightness = 1;
> + amber_led->flags = LED_CORE_SUSPENDRESUME | LED_RETAIN_AT_SHUTDOWN;
> + amber_led->brightness_set = asus_ec_led_set_brightness_amber;
> +
> + ret = devm_led_classdev_register(dev, amber_led);
> + if (ret)
> + return dev_err_probe(dev, ret, "failed to register amber LED\n");
> +
> + green_led = devm_kzalloc(dev, sizeof(*green_led), GFP_KERNEL);
> + if (!green_led)
> + return -ENOMEM;
> +
> + green_led->name = devm_kasprintf(dev, GFP_KERNEL, "%s::green", ec->name);
> + green_led->max_brightness = 1;
> + green_led->flags = LED_CORE_SUSPENDRESUME | LED_RETAIN_AT_SHUTDOWN;
> + green_led->brightness_set = asus_ec_led_set_brightness_green;
> +
> + ret = devm_led_classdev_register(dev, green_led);
> + if (ret)
> + return dev_err_probe(dev, ret, "failed to register green LED\n");
Imagine instead of 2 LEDs, you had 20. Would you copy and paste this 20
times? Please re-author this as though there were many more so it
remains efficient and scaleable.
> + return 0;
> +}
> +
> +static struct platform_driver asus_ec_led_driver = {
> + .driver.name = "asus-transformer-ec-led",
> + .probe = asus_ec_led_probe,
> +};
> +module_platform_driver(asus_ec_led_driver);
> +
> +MODULE_AUTHOR("Michał Mirosław <mirq-linux@rere.qmqm.pl>");
> +MODULE_AUTHOR("Svyatoslav Ryhel <clamor95@gmail.com>");
> +MODULE_DESCRIPTION("ASUS Transformer's charging LED driver");
> +MODULE_LICENSE("GPL");
> --
> 2.51.0
>
>
--
Lee Jones
^ permalink raw reply
* Re: [PATCH 1/6] tools/build: Allow versioning of all LLVM tools defined in Makefile.include
From: James Clark @ 2026-05-14 10:13 UTC (permalink / raw)
To: bot+bpf-ci
Cc: linux-kernel, llvm, linux-input, linux-kselftest, bpf,
linux-perf-users, leo.yan, martin.lau, clm, ihor.solodrai, nathan,
nick.desaulniers+lkml, morbo, justinstitt, jikos, bentiss, shuah,
peterz, mingo, acme, namhyung, mark.rutland, alexander.shishkin,
jolsa, irogers, adrian.hunter, andrii, eddyz87, ast, daniel,
martin.lau, memxor, song, yonghong.song
In-Reply-To: <998802d0c3bb407e9bcc61e4fe6db2e3eed7ac06c7c6eca0ed0dd5409a8c2801@mail.kernel.org>
On 14/05/2026 11:02 am, bot+bpf-ci@kernel.org wrote:
>> tools/build: Allow versioning of all LLVM tools defined in Makefile.include
>>
>> The version of LLVM tools can be given on the build command with
>> LLVM=-15, but this isn't applied to all tools. For example $(CC) gets
>> versioned, but $(CLANG) doesn't. This causes a Perf build with LTO=1 to
>> fail with an error about mixed clang versions:
>>
>> ld.lld: error: libperf/core.o: Unknown attribute kind (86)
>> (Producer: 'LLVM18.1.8' Reader: 'LLVM 15.0.7')
>>
>> This file has two "ifneq ($(LLVM),)" blocks adjacent to each other, so
>> merge these blocks making it obvious that all tools should be versioned
>> consistently and there is nothing special about each block.
>>
>> This also reveals that ?= and "allow-override" are used inconsistently
>> between the blocks. "allow-override" is technically only required for
>> builtin variables, but isn't only used on them, and doesn't do any harm
>> if used on a non-builtin. Make them all "allow-override" for
>> consistency. The only functional difference this will cause is if there
>> is a file level definition of one of the variables followed by an
>> "#include of Makefile.include" which will now overwrite. But this isn't
>> done and in a later commit some of the duplicate definitions will be
>> removed for good measure.
>>
>> There are also some other LLVM tools that are not defined here and will
>> be moved in a later commit.
>>
>> Signed-off-by: James Clark <james.clark@linaro.org>
>
> This looks like a bug fix that corrects inconsistent LLVM tool versioning
> introduced in an earlier commit. Should this include:
>
> Fixes: e9c281928c24 ("kbuild: Make $(LLVM) more flexible")
>
>
> ---
> AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
> See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md
>
> CI run summary: https://github.com/kernel-patches/bpf/actions/runs/25853166705
I did consider a fixes tag, but the only real bug is with Perf and
LTO=1, and building it in this way has obviously never worked, so you
could also argue this is new behavior.
I think it's quite and edge case and has some risk to break other tools
if backported so I left the tag off.
^ permalink raw reply
* Re: [PATCH v6 2/7] mfd: Add driver for ASUS Transformer embedded controller
From: Svyatoslav Ryhel @ 2026-05-14 10:31 UTC (permalink / raw)
To: Lee Jones
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Dmitry Torokhov,
Pavel Machek, Sebastian Reichel, Ion Agorria,
Michał Mirosław, devicetree, linux-kernel, linux-input,
linux-leds, linux-pm
In-Reply-To: <20260514100205.GG305027@google.com>
чт, 14 трав. 2026 р. о 13:02 Lee Jones <lee@kernel.org> пише:
>
> On Sat, 02 May 2026, Svyatoslav Ryhel wrote:
>
> > From: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> >
> > Support Nuvoton NPCE795-based ECs as used in Asus Transformer TF201,
> > TF300T, TF300TG, TF300TL and TF700T pad and dock, as well as TF101 dock
> > and TF600T, P1801-T and TF701T pad. This is a glue driver handling
> > detection and common operations for EC's functions.
> >
> > Co-developed-by: Svyatoslav Ryhel <clamor95@gmail.com>
> > Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> > Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> > ---
> > drivers/mfd/Kconfig | 14 +
> > drivers/mfd/Makefile | 1 +
> > drivers/mfd/asus-transformer-ec.c | 762 ++++++++++++++++++++++++
> > include/linux/mfd/asus-transformer-ec.h | 162 +++++
> > 4 files changed, 939 insertions(+)
> > create mode 100644 drivers/mfd/asus-transformer-ec.c
> > create mode 100644 include/linux/mfd/asus-transformer-ec.h
> >
> > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> > index 7192c9d1d268..5aa4facfd2df 100644
> > --- a/drivers/mfd/Kconfig
> > +++ b/drivers/mfd/Kconfig
> > @@ -137,6 +137,20 @@ config MFD_AAT2870_CORE
> > additional drivers must be enabled in order to use the
> > functionality of the device.
> >
> > +config MFD_ASUS_TRANSFORMER_EC
> > + tristate "ASUS Transformer's embedded controller"
> > + depends on I2C && OF
> > + help
> > + Support ECs found in ASUS Transformer's Pad and Mobile Dock.
> > +
> > + This provides shared glue for functional part drivers:
> > + asus-transformer-ec-kbc, asus-transformer-ec-keys,
> > + leds-asus-transformer-ec, asus-transformer-ec-battery
> > + and asus-transformer-ec-charger.
>
> A 760 line driver deserves more than just a list of leaf drivers.
>
Ok, I will make some meaningful description.
> > + This driver can also be built as a module. If so, the module
> > + will be called asus-transformer-ec.
> > +
> > config MFD_AT91_USART
> > tristate "AT91 USART Driver"
> > select MFD_CORE
> > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> > index e75e8045c28a..fd80088d8a9a 100644
> > --- a/drivers/mfd/Makefile
> > +++ b/drivers/mfd/Makefile
> > @@ -10,6 +10,7 @@ obj-$(CONFIG_MFD_88PM805) += 88pm805.o 88pm80x.o
> > obj-$(CONFIG_MFD_88PM886_PMIC) += 88pm886.o
> > obj-$(CONFIG_MFD_ACT8945A) += act8945a.o
> > obj-$(CONFIG_MFD_SM501) += sm501.o
> > +obj-$(CONFIG_MFD_ASUS_TRANSFORMER_EC) += asus-transformer-ec.o
> > obj-$(CONFIG_ARCH_BCM2835) += bcm2835-pm.o
> > obj-$(CONFIG_MFD_BCM590XX) += bcm590xx.o
> > obj-$(CONFIG_MFD_BD9571MWV) += bd9571mwv.o
> > diff --git a/drivers/mfd/asus-transformer-ec.c b/drivers/mfd/asus-transformer-ec.c
> > new file mode 100644
> > index 000000000000..75aa7ab99387
> > --- /dev/null
> > +++ b/drivers/mfd/asus-transformer-ec.c
> > @@ -0,0 +1,762 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +
> > +#include <linux/array_size.h>
> > +#include <linux/debugfs.h>
> > +#include <linux/delay.h>
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/device.h>
>
> Should we sort the '#include' directives alphabetically? For instance,
> 'device.h' should typically appear before 'gpio/consumer.h'.
>
Yes, it is my bad, I will fix it.
> > +#include <linux/err.h>
> > +#include <linux/i2c.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/mfd/asus-transformer-ec.h>
> > +#include <linux/mfd/core.h>
> > +#include <linux/mod_devicetable.h>
> > +#include <linux/module.h>
> > +#include <linux/mutex.h>
> > +#include <linux/property.h>
> > +#include <linux/slab.h>
> > +#include <linux/string.h>
> > +#include <linux/types.h>
> > +#include <linux/unaligned.h>
> > +
> > +#define ASUSEC_RSP_BUFFER_SIZE 8
> > +
> > +struct asus_ec_chip_data {
> > + const char *name;
> > + const struct mfd_cell *mfd_devices;
>
> Use global `static const structs` instead.
>
> Also, please do not pass MFD registration data through another
> registration API like DT. It opens the gates to too much hackery. I'm
> not saying that _this_ driver would do such a thing, but it's easier
> just to keep the blanket rule in place.
>
Yes, you have already made it clear in my CPCAP patchset. I have it
applied for the v7 here.
> > + unsigned int num_devices;
> > + bool clr_fmode; /* clear Factory Mode bit in EC control register */
> > +};
> > +
> > +struct asus_ec_data {
> > + struct asusec_info info;
>
> You have 'data' and 'info' which a) using non-forthcoming nomenclature
> and doesn't tell me anything and then you b) put 'info' in the device's
> driver_data attribute which is very confusing. driver_data should be
> for what we call ddata which I assume is expressed as 'data' here.
>
asusec_info is shared among all child devices and is exposed while
remaining elements of this struct are for internal use only.
> > + struct mutex ecreq_lock; /* prevent simultaneous access */
> > + struct gpio_desc *ecreq;
>
> If I hadn't seen the declaration, I'd have no idea this was a GPIO
> descriptor. Please improve the nomenclature throughout.
>
> > + struct i2c_client *self;
>
> Again, please use standard naming conventions:
>
> % git grep "struct i2c_client" | grep "\*self" | wc -l
> 0
>
> % git grep "struct i2c_client" | grep "\*client" | wc -l
> 6304
>
> % git grep "struct i2c_client" | grep "\*i2c" | wc -l
> 903
>
ok, noted.
> > + const struct asus_ec_chip_data *data;
>
> 'data', 'priv' and 'info' should be improved.
>
> > + char ec_data[DOCKRAM_ENTRY_BUFSIZE];
>
> An array of chars called 'data'. This could be anything.
>
Do you have a comprehensive list of name conventions you find suitable?
> > + bool logging_disabled;
>
> This debugging tool is probably never going to be used again.
>
> Keep it local.
>
> > +};
> > +
> > +struct dockram_ec_data {
> > + struct mutex ctl_lock; /* prevent simultaneous access */
> > + char ctl_data[DOCKRAM_ENTRY_BUFSIZE];
> > +};
> > +
> > +#define to_ec_data(ec) \
> > + container_of(ec, struct asus_ec_data, info)
> > +
> > +/**
> > + * asus_dockram_read - Read a register from the DockRAM device.
> > + * @client: Handle to the DockRAM device.
> > + * @reg: Register to read.
> > + * @buf: Byte array into which data will be read; must be large enough to
> > + * hold the data returned by the DockRAM.
> > + *
> > + * This executes the DockRAM read based on the SMBus "block read" protocol
> > + * or its emulation. It extracts DOCKRAM_ENTRY_SIZE bytes from the set
> > + * register address.
> > + *
> > + * Returns a negative errno code else zero on success.
> > + */
> > +int asus_dockram_read(struct i2c_client *client, int reg, char *buf)
> > +{
>
> Have you considered using Regmap for register access instead of
> implementing custom functions? Remaps already deals with caching and
> locking mechanisms that you'd get for free.
>
> This looks like it would be replaced with devm_regmap_init_i2c().
>
I will consider this, thank you.
> > + struct device *dev = &client->dev;
> > + int ret;
> > +
> > + memset(buf, 0, DOCKRAM_ENTRY_BUFSIZE);
> > + ret = i2c_smbus_read_i2c_block_data(client, reg,
> > + DOCKRAM_ENTRY_BUFSIZE, buf);
> > + if (ret < 0)
> > + return ret;
> > +
> > + if (buf[0] > DOCKRAM_ENTRY_SIZE) {
> > + dev_err(dev, "bad data len; buffer: %*ph; ret: %d\n",
> > + DOCKRAM_ENTRY_BUFSIZE, buf, ret);
> > + return -EPROTO;
> > + }
> > +
> > + dev_dbg(dev, "got data; buffer: %*ph; ret: %d\n",
> > + DOCKRAM_ENTRY_BUFSIZE, buf, ret);
>
> Please remove all of these debug messages.
>
Why debug messages cannot be preserved? They are specifically marked as dev_dbg
> > +
> > + return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(asus_dockram_read);
> > +
> > +/**
> > + * asus_dockram_write - Write a byte array to a register of the DockRAM device.
> > + * @client: Handle to the DockRAM device.
> > + * @reg: Register to write to.
> > + * @buf: Byte array to be written (up to DOCKRAM_ENTRY_SIZE bytes).
> > + *
> > + * This executes the DockRAM write based on the SMBus "block write"
> > + * protocol or its emulation. It writes DOCKRAM_ENTRY_SIZE bytes to the
> > + * specified register address.
> > + *
> > + * Returns a negative errno code else zero on success.
> > + */
> > +int asus_dockram_write(struct i2c_client *client, int reg, const char *buf)
> > +{
> > + if (buf[0] > DOCKRAM_ENTRY_SIZE)
> > + return -EINVAL;
> > +
> > + dev_dbg(&client->dev, "sending data; buffer: %*ph\n", buf[0] + 1, buf);
> > +
> > + return i2c_smbus_write_i2c_block_data(client, reg, buf[0] + 1, buf);
> > +}
> > +EXPORT_SYMBOL_GPL(asus_dockram_write);
> > +
> > +/**
> > + * asus_dockram_access_ctl - Read from or write to the DockRAM control register.
> > + * @client: Handle to the DockRAM device.
> > + * @out: Pointer to a variable where the register value will be stored.
> > + * @mask: Bitmask of bits to be cleared.
> > + * @xor: Bitmask of bits to be set (via XOR).
> > + *
> > + * This performs a control register read if @out is provided and both @mask
> > + * and @xor are zero. Otherwise, it performs a control register update if
> > + * @mask and @xor are provided.
> > + *
> > + * Returns a negative errno code else zero on success.
> > + */
> > +int asus_dockram_access_ctl(struct i2c_client *client, u64 *out, u64 mask,
> > + u64 xor)
> > +{
> > + struct dockram_ec_data *priv = i2c_get_clientdata(client);
> > + char *buf = priv->ctl_data;
> > + u64 val;
> > + int ret = 0;
> > +
> > + guard(mutex)(&priv->ctl_lock);
> > +
> > + ret = asus_dockram_read(client, ASUSEC_DOCKRAM_CONTROL, buf);
> > + if (ret < 0)
>
> Could we check for errors using 'if (ret)' instead of 'if (ret < 0)' here,
> unless a positive return value has a specific meaning we need to handle?
>
Sure, I will switch to 'if (ret)'
> > + goto exit;
> > +
> > + if (buf[0] != ASUSEC_CTL_SIZE) {
> > + ret = -EPROTO;
> > + goto exit;
> > + }
> > +
> > + val = get_unaligned_le64(buf + 1);
> > +
> > + if (out)
> > + *out = val;
> > +
> > + if (mask || xor) {
> > + put_unaligned_le64((val & ~mask) ^ xor, buf + 1);
> > + ret = asus_dockram_write(client, ASUSEC_DOCKRAM_CONTROL, buf);
> > + }
> > +
> > +exit:
> > + if (ret < 0)
> > + dev_err(&client->dev, "Failed to access control flags: %d\n",
> > + ret);
> > +
> > + return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(asus_dockram_access_ctl);
> > +
> > +static void asus_ec_remove_notifier(struct device *dev, void *res)
> > +{
> > + struct asusec_info *ec = dev_get_drvdata(dev->parent);
> > + struct notifier_block **nb = res;
> > +
> > + blocking_notifier_chain_unregister(&ec->notify_list, *nb);
> > +}
> > +
> > +/**
> > + * devm_asus_ec_register_notifier - Managed registration of notifier to an
> > + * ASUS EC blocking notifier chain.
> > + * @pdev: Device requesting the notifier (used for resource management).
> > + * @nb: Notifier block to be registered.
> > + *
> > + * Register a notifier to the ASUS EC blocking notifier chain. The notifier
> > + * will be automatically unregistered when the requesting device is detached.
> > + *
> > + * Return: 0 on success or a negative error code on failure.
> > + */
> > +int devm_asus_ec_register_notifier(struct platform_device *pdev,
> > + struct notifier_block *nb)
> > +{
>
> Hand-rolling devres managed resources is usually reserved for subsystem
> level API calls. Why do the usual device driver .remove() handling work
> for you?
>
This is used by 3 subdevices: serio, keys and charger, so this just
seems cleaner way to register and deregister notifier.
> > + struct asusec_info *ec = dev_get_drvdata(pdev->dev.parent);
> > + struct notifier_block **res;
> > + int ret;
> > +
> > + res = devres_alloc(asus_ec_remove_notifier, sizeof(*res), GFP_KERNEL);
> > + if (!res)
> > + return -ENOMEM;
> > +
> > + *res = nb;
> > + ret = blocking_notifier_chain_register(&ec->notify_list, nb);
> > + if (ret) {
> > + devres_free(res);
> > + return ret;
> > + }
> > +
> > + devres_add(&pdev->dev, res);
> > +
> > + return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(devm_asus_ec_register_notifier);
> > +
> > +static int asus_ec_signal_request(const struct asusec_info *ec)
> > +{
> > + struct asus_ec_data *priv = to_ec_data(ec);
> > +
> > + guard(mutex)(&priv->ecreq_lock);
> > +
> > + dev_dbg(&priv->self->dev, "EC request\n");
> > +
> > + gpiod_set_value_cansleep(priv->ecreq, 1);
> > + msleep(50);
> > +
> > + gpiod_set_value_cansleep(priv->ecreq, 0);
> > + msleep(200);
> > +
> > + return 0;
> > +}
> > +
> > +static int asus_ec_write(struct asus_ec_data *priv, u16 data)
> > +{
> > + int ret = i2c_smbus_write_word_data(priv->self, ASUSEC_WRITE_BUF, data);
> > +
> > + dev_dbg(&priv->self->dev, "EC write: %04x, ret = %d\n", data, ret);
> > +
> > + return ret;
> > +}
> > +
> > +static int asus_ec_read(struct asus_ec_data *priv, bool in_irq)
> > +{
> > + int ret = i2c_smbus_read_i2c_block_data(priv->self, ASUSEC_READ_BUF,
> > + sizeof(priv->ec_data),
> > + priv->ec_data);
> > +
> > + dev_dbg(&priv->self->dev, "EC read: %*ph, ret = %d%s\n",
> > + sizeof(priv->ec_data), priv->ec_data,
> > + ret, in_irq ? "; in irq" : "");
> > +
> > + return ret;
> > +}
>
> No abstractions for the sake of it. All this goes away with Regmap anyway.
> > +
> > +/**
> > + * asus_ec_i2c_command - Send a 16-bit command to the ASUS EC.
> > + * @ec: Pointer to the shared ASUS EC structure.
> > + * @data: The 16-bit command (word) to be sent.
> > + *
> > + * Return: 0 on success or a negative error code on failure.
> > + */
> > +int asus_ec_i2c_command(const struct asusec_info *ec, u16 data)
> > +{
> > + return asus_ec_write(to_ec_data(ec), data);
> > +}
>
> Is this wrapper function strictly necessary? We should try to avoid
> superfluous abstractions that do nothing but call another function.
>
This one is used by serio and keys too. I will see what I can do with regmaps.
> > +EXPORT_SYMBOL_GPL(asus_ec_i2c_command);
> > +
> > +static void asus_ec_clear_buffer(struct asus_ec_data *priv)
> > +{
> > + int retry = ASUSEC_RSP_BUFFER_SIZE;
> > +
> > + while (retry--) {
> > + if (asus_ec_read(priv, false) < 0)
> > + continue;
> > +
> > + if (priv->ec_data[1] & ASUSEC_OBF_MASK)
> > + continue;
> > +
> > + break;
> > + }
> > +}
> > +
> > +static int asus_ec_log_info(struct asus_ec_data *priv, unsigned int reg,
> > + const char *name, char **out)
> > +{
> > + char buf[DOCKRAM_ENTRY_BUFSIZE];
> > + int ret;
> > +
> > + ret = asus_dockram_read(priv->info.dockram, reg, buf);
> > + if (ret < 0)
> > + return ret;
> > +
> > + if (!priv->logging_disabled)
> > + dev_info(&priv->self->dev, "%-14s: %.*s\n", name,
> > + buf[0], buf + 1);
> > +
> > + if (out)
> > + *out = kstrndup(buf + 1, buf[0], GFP_KERNEL);
> > +
> > + return 0;
> > +}
> > +
> > +static int asus_ec_reset(struct asus_ec_data *priv)
> > +{
> > + int retry, ret;
> > +
> > + for (retry = 0; retry < 3; retry++) {
> > + ret = asus_ec_write(priv, 0);
> > + if (!ret)
> > + return 0;
> > +
> > + msleep(300);
> > + }
> > +
> > + return ret;
> > +}
> > +
> > +static int asus_ec_magic_debug(struct asus_ec_data *priv)
> > +{
> > + u64 flag;
> > + int ret;
> > +
> > + ret = asus_ec_get_ctl(&priv->info, &flag);
> > + if (ret < 0)
> > + return ret;
> > +
> > + flag &= ASUSEC_CTL_SUSB_MODE;
> > + dev_info(&priv->self->dev, "EC FW behaviour: %s\n",
> > + flag ? "susb on when receive ec_req" :
> > + "susb on when system wakeup");
> > +
> > + return 0;
> > +}
> > +
> > +static int asus_ec_set_factory_mode(struct asus_ec_data *priv, bool on)
> > +{
> > + dev_info(&priv->self->dev, "Entering %s mode.\n", on ? "factory" :
> > + "normal");
> > +
> > + return asus_ec_update_ctl(&priv->info, ASUSEC_CTL_FACTORY_MODE,
> > + on ? ASUSEC_CTL_FACTORY_MODE : 0);
> > +}
> > +
> > +static int asus_ec_detect(struct asus_ec_data *priv)
> > +{
> > + char *model = NULL;
> > + int ret;
> > +
> > + ret = asus_ec_reset(priv);
> > + if (ret)
> > + goto err_exit;
> > +
> > + asus_ec_clear_buffer(priv);
> > +
> > + ret = asus_ec_log_info(priv, ASUSEC_DOCKRAM_INFO_MODEL, "model", &model);
> > + if (ret)
> > + goto err_exit;
> > +
> > + ret = asus_ec_log_info(priv, ASUSEC_DOCKRAM_INFO_FW, "FW version", NULL);
> > + if (ret)
> > + goto err_exit;
> > +
> > + ret = asus_ec_log_info(priv, ASUSEC_DOCKRAM_INFO_CFGFMT, "Config format", NULL);
> > + if (ret)
> > + goto err_exit;
> > +
> > + ret = asus_ec_log_info(priv, ASUSEC_DOCKRAM_INFO_HW, "HW version", NULL);
> > + if (ret)
> > + goto err_exit;
> > +
> > + priv->logging_disabled = true;
> > +
> > + ret = asus_ec_magic_debug(priv);
> > + if (ret)
> > + goto err_exit;
> > +
> > + priv->info.model = model;
> > + priv->info.name = priv->data->name;
> > +
> > + if (priv->data->clr_fmode)
> > + asus_ec_set_factory_mode(priv, false);
> > +
> > +err_exit:
> > + if (ret)
> > + dev_err(&priv->self->dev, "failed to access EC: %d\n", ret);
> > +
> > + return ret;
> > +}
> > +
> > +static void asus_ec_handle_smi(struct asus_ec_data *priv, unsigned int code)
> > +{
> > + dev_dbg(&priv->self->dev, "SMI interrupt: 0x%02x\n", code);
> > +
> > + switch (code) {
> > + case ASUSEC_SMI_HANDSHAKE:
> > + case ASUSEC_SMI_RESET:
> > + asus_ec_detect(priv);
> > + break;
> > + }
> > +}
> > +
> > +static irqreturn_t asus_ec_interrupt(int irq, void *dev_id)
> > +{
> > + struct asus_ec_data *priv = dev_id;
> > + unsigned long notify_action;
> > + int ret;
> > +
> > + ret = asus_ec_read(priv, true);
> > + if (ret <= 0 || !(priv->ec_data[1] & ASUSEC_OBF_MASK))
> > + return IRQ_NONE;
> > +
> > + notify_action = priv->ec_data[1];
> > + if (notify_action & ASUSEC_SMI_MASK) {
> > + unsigned int code = priv->ec_data[2];
> > +
> > + asus_ec_handle_smi(priv, code);
> > +
> > + notify_action |= code << 8;
> > + dev_dbg(&priv->self->dev, "SMI code: 0x%02x\n", code);
> > + }
> > +
> > + blocking_notifier_call_chain(&priv->info.notify_list,
> > + notify_action, priv->ec_data);
> > +
> > + return IRQ_HANDLED;
> > +}
> > +
> > +static ssize_t dockram_read(struct file *filp, char __user *buf,
> > + size_t count, loff_t *ppos)
> > +{
> > + struct i2c_client *client = filp->private_data;
> > + unsigned int reg, rsize;
> > + ssize_t n_read = 0, val;
> > + loff_t off = *ppos;
> > + char *data;
> > + int ret;
> > +
> > + reg = off / DOCKRAM_ENTRY_SIZE;
> > + off %= DOCKRAM_ENTRY_SIZE;
> > + rsize = DOCKRAM_ENTRIES * DOCKRAM_ENTRY_SIZE;
> > +
> > + if (!count)
> > + return 0;
> > +
> > + data = kmalloc(DOCKRAM_ENTRY_BUFSIZE, GFP_KERNEL);
> > +
> > + while (reg < DOCKRAM_ENTRIES) {
> > + unsigned int len = DOCKRAM_ENTRY_SIZE - off;
> > +
> > + if (len > rsize)
> > + len = rsize;
> > +
> > + ret = asus_dockram_read(client, reg, data);
> > + if (ret < 0) {
> > + if (!n_read)
> > + n_read = ret;
> > + break;
> > + }
> > +
> > + val = copy_to_user(buf, data + 1 + off, len);
> > + if (val == len)
> > + return -EFAULT;
> > +
> > + *ppos += len;
> > + n_read += len;
> > +
> > + if (len == rsize)
> > + break;
> > +
> > + rsize -= len;
> > + buf += len;
> > + off = 0;
> > + ++reg;
> > + }
> > +
> > + kfree(data);
> > +
> > + return n_read;
> > +}
> > +
> > +static int dockram_write_one(struct i2c_client *client, int reg,
> > + const char __user *buf, size_t count)
> > +{
> > + struct dockram_ec_data *priv = i2c_get_clientdata(client);
> > + int ret;
> > +
> > + if (!count || count > DOCKRAM_ENTRY_SIZE)
> > + return -EINVAL;
> > + if (buf[0] != count - 1)
> > + return -EINVAL;
> > +
> > + guard(mutex)(&priv->ctl_lock);
> > +
> > + priv->ctl_data[0] = (u8)count;
> > + memcpy(priv->ctl_data + 1, buf, count);
> > + ret = asus_dockram_write(client, reg, priv->ctl_data);
> > +
> > + return ret;
> > +}
> > +
> > +static ssize_t dockram_write(struct file *filp, const char __user *buf,
> > + size_t count, loff_t *ppos)
> > +{
> > + struct i2c_client *client = filp->private_data;
> > + unsigned int reg;
> > + loff_t off = *ppos;
> > + int ret;
> > +
> > + if (off % DOCKRAM_ENTRY_SIZE != 0)
> > + return -EINVAL;
> > +
> > + reg = off / DOCKRAM_ENTRY_SIZE;
> > + if (reg >= DOCKRAM_ENTRIES)
> > + return -EINVAL;
> > +
> > + ret = dockram_write_one(client, reg, buf, count);
> > +
> > + return ret < 0 ? ret : count;
> > +}
> > +
> > +static const struct debugfs_short_fops dockram_fops = {
> > + .read = dockram_read,
> > + .write = dockram_write,
> > + .llseek = default_llseek,
> > +};
>
> You should not be giving userspace free reign over device memory.
>
> If you switch to Regmap, you can use regmap-debugfs.c for this.
>
Noted
> > +static int control_reg_get(void *ec, u64 *val)
> > +{
> > + return asus_ec_get_ctl(ec, val);
> > +}
> > +
> > +static int control_reg_set(void *ec, u64 val)
> > +{
> > + return asus_ec_update_ctl(ec, ~0ull, val);
> > +}
> > +
> > +DEFINE_DEBUGFS_ATTRIBUTE(control_reg_fops, control_reg_get,
> > + control_reg_set, "%016llx\n");
> > +
> > +static int ec_request_set(void *ec, u64 val)
> > +{
> > + if (val)
> > + asus_ec_signal_request(ec);
> > +
> > + return 0;
> > +}
> > +
> > +DEFINE_DEBUGFS_ATTRIBUTE(ec_request_fops, NULL, ec_request_set, "%llu\n");
> > +
> > +static int ec_irq_set(void *ec, u64 val)
> > +{
> > + struct asus_ec_data *priv = to_ec_data(ec);
> > +
> > + if (val)
> > + irq_wake_thread(priv->self->irq, priv);
> > +
> > + return 0;
> > +}
> > +
> > +DEFINE_DEBUGFS_ATTRIBUTE(ec_irq_fops, NULL, ec_irq_set, "%llu\n");
> > +
> > +static void asus_ec_debugfs_remove(void *debugfs_root)
> > +{
> > + debugfs_remove_recursive(debugfs_root);
> > +}
> > +
> > +static void devm_asus_ec_debugfs_init(struct device *dev)
> > +{
> > + struct asusec_info *ec = dev_get_drvdata(dev);
> > + struct asus_ec_data *priv = to_ec_data(ec);
> > + struct dentry *debugfs_root, *dockram_dir;
> > + char *name = devm_kasprintf(dev, GFP_KERNEL, "asus-ec-%s",
> > + priv->data->name);
> > +
> > + debugfs_root = debugfs_create_dir(name, NULL);
> > + dockram_dir = debugfs_create_dir("dockram", debugfs_root);
> > +
> > + debugfs_create_file("ec_irq", 0200, debugfs_root, ec,
> > + &ec_irq_fops);
> > + debugfs_create_file("ec_request", 0200, debugfs_root, ec,
> > + &ec_request_fops);
> > + debugfs_create_file("control_reg", 0644, dockram_dir, ec,
> > + &control_reg_fops);
> > + debugfs_create_file("dockram", 0644, dockram_dir,
> > + priv->info.dockram, &dockram_fops);
> > +
> > + devm_add_action_or_reset(dev, asus_ec_debugfs_remove, debugfs_root);
> > +}
>
> Why is this being controlled via debugfs?
>
> Use the correct kernel APIs instead.
>
> If this is just for development, keep it and all of the extra verbose
> printing in the BSP tree. It does not belong in the upstream kernel.
>
Prints are required to show Firmware type, version and behavior.
> > +static void asus_ec_release_dockram_dev(void *client)
> > +{
> > + i2c_unregister_device(client);
> > +}
> > +
> > +static struct i2c_client *devm_asus_dockram_get(struct device *dev)
> > +{
> > + struct i2c_client *parent = to_i2c_client(dev);
> > + struct i2c_client *dockram;
> > + struct dockram_ec_data *priv;
> > + int ret;
> > +
> > + dockram = i2c_new_ancillary_device(parent, "dockram",
> > + parent->addr + 2);
> > + if (IS_ERR(dockram))
> > + return dockram;
> > +
> > + ret = devm_add_action_or_reset(dev, asus_ec_release_dockram_dev,
> > + dockram);
> > + if (ret)
> > + return ERR_PTR(ret);
> > +
> > + priv = devm_kzalloc(&dockram->dev, sizeof(*priv), GFP_KERNEL);
> > + if (!priv)
> > + return ERR_PTR(-ENOMEM);
> > +
> > + i2c_set_clientdata(dockram, priv);
> > + mutex_init(&priv->ctl_lock);
> > +
> > + return dockram;
> > +}
> > +
> > +static int asus_ec_probe(struct i2c_client *client)
> > +{
> > + struct device *dev = &client->dev;
> > + struct asus_ec_data *priv;
>
> Could we use a clearer, more specific name for the private data variable
> instead of the generic term 'priv'? Using something like 'ddata' is usually
> preferred.
>
fine
> > + int ret;
> > +
> > + if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_I2C_BLOCK))
> > + return dev_err_probe(dev, -ENXIO,
> > + "I2C bus is missing required SMBus block mode support\n");
> > +
> > + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > + if (!priv)
> > + return -ENOMEM;
> > +
> > + priv->data = device_get_match_data(dev);
> > + if (!priv->data)
> > + return -ENODEV;
> > +
> > + i2c_set_clientdata(client, priv);
> > + priv->self = client;
> > +
> > + priv->info.dockram = devm_asus_dockram_get(dev);
> > + if (IS_ERR(priv->info.dockram))
> > + return dev_err_probe(dev, PTR_ERR(priv->info.dockram),
> > + "failed to get dockram\n");
> > +
> > + priv->ecreq = devm_gpiod_get(dev, "request", GPIOD_OUT_LOW);
> > + if (IS_ERR(priv->ecreq))
> > + return dev_err_probe(dev, PTR_ERR(priv->ecreq),
> > + "failed to get request GPIO\n");
>
> "get" or "request"
>
request is gpio's name, request gpio
> > + BLOCKING_INIT_NOTIFIER_HEAD(&priv->info.notify_list);
> > + mutex_init(&priv->ecreq_lock);
> > +
> > + asus_ec_signal_request(&priv->info);
> > +
> > + ret = asus_ec_detect(priv);
> > + if (ret)
> > + return dev_err_probe(dev, ret, "failed to detect EC version\n");
> > +
> > + ret = devm_request_threaded_irq(dev, client->irq, NULL,
> > + &asus_ec_interrupt,
> > + IRQF_ONESHOT | IRQF_SHARED,
> > + client->name, priv);
> > + if (ret)
> > + return dev_err_probe(dev, ret, "failed to register IRQ\n");
> > +
> > + /* Parent I2C controller uses DMA, ASUS EC and child devices do not */
> > + client->dev.coherent_dma_mask = 0;
> > + client->dev.dma_mask = &client->dev.coherent_dma_mask;
> > +
> > + if (IS_ENABLED(CONFIG_DEBUG_FS))
> > + devm_asus_ec_debugfs_init(dev);
> > +
> > + return devm_mfd_add_devices(dev, 0, priv->data->mfd_devices,
> > + priv->data->num_devices, NULL, 0, NULL);
> > +}
> > +
> > +static const struct mfd_cell asus_ec_sl101_dock_mfd_devices[] = {
> > + {
> > + .name = "asus-transformer-ec-kbc",
> > + },
> > +};
> > +
> > +static const struct asus_ec_chip_data asus_ec_sl101_dock_data = {
> > + .name = "dock",
> > + .mfd_devices = asus_ec_sl101_dock_mfd_devices,
> > + .num_devices = ARRAY_SIZE(asus_ec_sl101_dock_mfd_devices),
> > + .clr_fmode = false,
> > +};
> > +
> > +static const struct mfd_cell asus_ec_tf101_dock_mfd_devices[] = {
> > + {
> > + .name = "asus-transformer-ec-battery",
> > + .id = 1,
>
> Why doesn't PLATFORM_DEVID_AUTO already do the right thing?
>
There may be 2 of this EC in the device with similar set of cells,
last time I have tested EC that was registered second caused clash
with first EC cause of these cells. I will check if this behavior
changed now.
> > + }, {
> > + .name = "asus-transformer-ec-charger",
> > + .id = 1,
> > + }, {
> > + .name = "asus-transformer-ec-led",
> > + .id = 1,
> > + }, {
> > + .name = "asus-transformer-ec-keys",
> > + }, {
> > + .name = "asus-transformer-ec-kbc",
> > + },
> > +};
> > +
> > +static const struct asus_ec_chip_data asus_ec_tf101_dock_data = {
> > + .name = "dock",
> > + .mfd_devices = asus_ec_tf101_dock_mfd_devices,
> > + .num_devices = ARRAY_SIZE(asus_ec_tf101_dock_mfd_devices),
> > + .clr_fmode = false,
> > +};
> > +
> > +static const struct mfd_cell asus_ec_tf201_pad_mfd_devices[] = {
> > + {
> > + .name = "asus-transformer-ec-battery",
> > + .id = 0,
> > + }, {
> > + .name = "asus-transformer-ec-led",
> > + .id = 0,
> > + },
> > +};
> > +
> > +static const struct asus_ec_chip_data asus_ec_tf201_pad_data = {
> > + .name = "pad",
> > + .mfd_devices = asus_ec_tf201_pad_mfd_devices,
> > + .num_devices = ARRAY_SIZE(asus_ec_tf201_pad_mfd_devices),
> > + .clr_fmode = true,
> > +};
> > +
> > +static const struct mfd_cell asus_ec_tf600t_pad_mfd_devices[] = {
> > + {
> > + .name = "asus-transformer-ec-battery",
> > + .id = 0,
> > + }, {
> > + .name = "asus-transformer-ec-charger",
> > + .id = 0,
> > + }, {
> > + .name = "asus-transformer-ec-led",
> > + .id = 0,
> > + },
> > +};
> > +
> > +static const struct asus_ec_chip_data asus_ec_tf600t_pad_data = {
> > + .name = "pad",
> > + .mfd_devices = asus_ec_tf600t_pad_mfd_devices,
> > + .num_devices = ARRAY_SIZE(asus_ec_tf600t_pad_mfd_devices),
> > + .clr_fmode = true,
> > +};
> > +
> > +static const struct of_device_id asus_ec_match[] = {
> > + { .compatible = "asus,sl101-ec-dock", .data = &asus_ec_sl101_dock_data },
> > + { .compatible = "asus,tf101-ec-dock", .data = &asus_ec_tf101_dock_data },
>
> Could we use the match table's data field to store an 'enum' or integer ID
> instead of passing platform data via the '.data' field? We could then use a
> 'switch' statement in the C code to select the correct 'mfd_cell' array.
Yes
> > + { .compatible = "asus,tf201-ec-pad", .data = &asus_ec_tf201_pad_data },
> > + { .compatible = "asus,tf600t-ec-pad", .data = &asus_ec_tf600t_pad_data },
> > + { }
> > +};
> > +MODULE_DEVICE_TABLE(of, asus_ec_match);
> > +
> > +static struct i2c_driver asus_ec_driver = {
> > + .driver = {
> > + .name = "asus-transformer-ec",
> > + .of_match_table = asus_ec_match,
> > + },
> > + .probe = asus_ec_probe,
> > +};
> > +module_i2c_driver(asus_ec_driver);
> > +
> > +MODULE_AUTHOR("Michał Mirosław <mirq-linux@rere.qmqm.pl>");
> > +MODULE_AUTHOR("Svyatoslav Ryhel <clamor95@gmail.com>");
> > +MODULE_DESCRIPTION("ASUS Transformer's EC driver");
> > +MODULE_LICENSE("GPL");
> > diff --git a/include/linux/mfd/asus-transformer-ec.h b/include/linux/mfd/asus-transformer-ec.h
> > new file mode 100644
> > index 000000000000..0a72de40352e
> > --- /dev/null
> > +++ b/include/linux/mfd/asus-transformer-ec.h
> > @@ -0,0 +1,162 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +#ifndef __MFD_ASUS_TRANSFORMER_EC_H
> > +#define __MFD_ASUS_TRANSFORMER_EC_H
> > +
> > +#include <linux/notifier.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/workqueue.h>
> > +
> > +struct i2c_client;
> > +
> > +struct asusec_info {
> > + const char *model;
> > + const char *name;
> > + struct i2c_client *dockram;
> > + struct workqueue_struct *wq;
> > + struct blocking_notifier_head notify_list;
> > +};
> > +
> > +#define DOCKRAM_ENTRIES 0x100
> > +#define DOCKRAM_ENTRY_SIZE 32
> > +#define DOCKRAM_ENTRY_BUFSIZE (DOCKRAM_ENTRY_SIZE + 1)
> > +
> > +/* interrupt sources */
> > +#define ASUSEC_OBF_MASK BIT(0)
> > +#define ASUSEC_KEY_MASK BIT(2)
> > +#define ASUSEC_KBC_MASK BIT(3)
> > +#define ASUSEC_AUX_MASK BIT(5)
> > +#define ASUSEC_SCI_MASK BIT(6)
> > +#define ASUSEC_SMI_MASK BIT(7)
> > +
> > +/* SMI notification codes */
> > +#define ASUSEC_SMI_POWER_NOTIFY 0x31 /* [un]plugging USB cable */
> > +#define ASUSEC_SMI_HANDSHAKE 0x50 /* response to ec_req edge */
> > +#define ASUSEC_SMI_WAKE 0x53
> > +#define ASUSEC_SMI_RESET 0x5f
> > +#define ASUSEC_SMI_ADAPTER_EVENT 0x60 /* [un]plugging charger to dock */
> > +#define ASUSEC_SMI_BACKLIGHT_ON 0x63
> > +#define ASUSEC_SMI_AUDIO_DOCK_IN 0x70
> > +
> > +#define ASUSEC_SMI_ACTION(code) (ASUSEC_SMI_MASK | ASUSEC_OBF_MASK | \
> > + (ASUSEC_SMI_##code << 8))
> > +
> > +/* control register [0x0a] layout */
> > +#define ASUSEC_CTL_SIZE 8
> > +
> > +/*
> > + * EC reports power from 40-pin connector in the LSB of the control
> > + * register. The following values have been observed (xor 0x02):
> > + *
> > + * PAD-ec no-plug 0x40 / PAD-ec DOCK 0x20 / DOCK-ec no-plug 0x40
> > + * PAD-ec AC 0x25 / PAD-ec DOCK+AC 0x24 / DOCK-ec AC 0x25
> > + * PAD-ec USB 0x45 / PAD-ec DOCK+USB 0x24 / DOCK-ec USB 0x41
> > + */
> > +
> > +#define ASUSEC_CTL_DIRECT_POWER_SOURCE BIT_ULL(0)
> > +#define ASUSEC_STAT_CHARGING BIT_ULL(2)
> > +#define ASUSEC_CTL_FULL_POWER_SOURCE BIT_ULL(5)
> > +#define ASUSEC_CTL_SUSB_MODE BIT_ULL(9)
> > +#define ASUSEC_CMD_SUSPEND_S3 BIT_ULL(33)
> > +#define ASUSEC_CTL_TEST_DISCHARGE BIT_ULL(35)
> > +#define ASUSEC_CMD_SUSPEND_INHIBIT BIT_ULL(37)
> > +#define ASUSEC_CTL_FACTORY_MODE BIT_ULL(38)
> > +#define ASUSEC_CTL_KEEP_AWAKE BIT_ULL(39)
> > +#define ASUSEC_CTL_USB_CHARGE BIT_ULL(40)
> > +#define ASUSEC_CTL_LED_BLINK BIT_ULL(40)
> > +#define ASUSEC_CTL_LED_AMBER BIT_ULL(41)
> > +#define ASUSEC_CTL_LED_GREEN BIT_ULL(42)
> > +#define ASUSEC_CMD_SWITCH_HDMI BIT_ULL(56)
> > +#define ASUSEC_CMD_WIN_SHUTDOWN BIT_ULL(62)
> > +
> > +#define ASUSEC_DOCKRAM_INFO_MODEL 0x01
> > +#define ASUSEC_DOCKRAM_INFO_FW 0x02
> > +#define ASUSEC_DOCKRAM_INFO_CFGFMT 0x03
> > +#define ASUSEC_DOCKRAM_INFO_HW 0x04
> > +#define ASUSEC_DOCKRAM_CONTROL 0x0a
> > +#define ASUSEC_DOCKRAM_BATT_CTL 0x14
> > +
> > +#define ASUSEC_WRITE_BUF 0x64
> > +#define ASUSEC_READ_BUF 0x6a
> > +
> > +/* dockram comm */
> > +int asus_dockram_read(struct i2c_client *client, int reg, char *buf);
> > +int asus_dockram_write(struct i2c_client *client, int reg, const char *buf);
> > +int asus_dockram_access_ctl(struct i2c_client *client,
> > + u64 *out, u64 mask, u64 xor);
> > +
> > +/* EC public API */
> > +
> > +/**
> > + * cell_to_ec - Request the shared ASUS EC structure via a subdevice's pdev.
> > + * @pdev: EC subdevice pdev requesting access to the shared ASUS EC structure.
> > + *
> > + * Returns a pointer to the asusec_info structure.
> > + */
> > +static inline struct asusec_info *cell_to_ec(struct platform_device *pdev)
> > +{
> > + return dev_get_drvdata(pdev->dev.parent);
> > +}
>
> This is both misleading and already exists.
>
> > +
> > +/**
> > + * asus_ec_get_ctl - Read from the DockRAM control register.
> > + * @ec: Pointer to the shared ASUS EC structure.
> > + * @out: Pointer to the variable where the register value will be stored.
> > + *
> > + * Performs a control register read and stores the value in @out.
> > + *
> > + * Return: 0 on success, or a negative errno code on failure.
> > + */
> > +static inline int asus_ec_get_ctl(const struct asusec_info *ec, u64 *out)
> > +{
> > + return asus_dockram_access_ctl(ec->dockram, out, 0, 0);
> > +}
> > +
> > +/**
> > + * asus_ec_update_ctl - Update the DockRAM control register.
> > + * @ec: Pointer to the shared ASUS EC structure.
> > + * @mask: Bitmask of bits to be cleared.
> > + * @xor: Bitmask of bits to be toggled or set (via XOR).
> > + *
> > + * Performs a read-modify-write update on the control register using
> > + * the provided @mask and @xor values.
> > + *
> > + * Return: 0 on success, or a negative errno code on failure.
> > + */
> > +static inline int asus_ec_update_ctl(const struct asusec_info *ec,
> > + u64 mask, u64 xor)
> > +{
> > + return asus_dockram_access_ctl(ec->dockram, NULL, mask, xor);
> > +}
> > +
> > +/**
> > + * asus_ec_set_ctl_bits - Sets bits of the DockRAM control register.
> > + * @ec: Pointer to the shared ASUS EC structure.
> > + * @mask: Bitmask of bits to be set.
> > + *
> > + * Sets bits of the control register using the provided @mask value.
> > + *
> > + * Return: 0 on success, or a negative errno code on failure.
> > + */
> > +static inline int asus_ec_set_ctl_bits(const struct asusec_info *ec, u64 mask)
> > +{
> > + return asus_dockram_access_ctl(ec->dockram, NULL, mask, mask);
> > +}
> > +
> > +/**
> > + * asus_ec_clear_ctl_bits - Clears bits of the DockRAM control register.
> > + * @ec: Pointer to the shared ASUS EC structure.
> > + * @mask: Bitmask of bits to be cleared.
> > + *
> > + * Clears bits of the control register using the provided @mask value.
> > + *
> > + * Return: 0 on success, or a negative errno code on failure.
> > + */
> > +static inline int asus_ec_clear_ctl_bits(const struct asusec_info *ec, u64 mask)
> > +{
> > + return asus_dockram_access_ctl(ec->dockram, NULL, mask, 0);
> > +}
>
> We don't typically allow abstraction for the sake of abstraction.
>
> > +int asus_ec_i2c_command(const struct asusec_info *ec, u16 data);
> > +int devm_asus_ec_register_notifier(struct platform_device *dev,
> > + struct notifier_block *nb);
> > +#endif /* __MFD_ASUS_TRANSFORMER_EC_H */
> > --
> > 2.51.0
> >
> >
>
> --
> Lee Jones
^ permalink raw reply
* Re: [PATCH 5/7] hid-asus: Fix input mapping on dedicated vendor HID interfaces
From: sashiko-bot @ 2026-05-14 10:32 UTC (permalink / raw)
To: Paolo Pisati; +Cc: dmitry.torokhov, linux-input
In-Reply-To: <20260513163248.16483-6-p.pisati@gmail.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] The injected fake keyboard descriptor contains a malformed padding item that violates the HID specification.
--
commit 98dabe022a958136634ce6b477391fa33ffa21b6
Author: Joshua Leivenzon <hacker1024@users.sourceforge.net>
hid-asus: Fix input mapping on dedicated vendor HID interfaces
This commit adds a fake keyboard input to the vendor-specific interface's
report descriptor on some USB keyboards, such as the Zenbook Duo UX8406MA.
This allows the kernel to combine it with the vendor-specific collections
and process the custom mappings correctly.
> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index cf9184ed2d3ec..fe52fd63d61c6 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c
> @@ -1376,6 +1376,30 @@ static void asus_remove(struct hid_device *hdev)
> hid_hw_stop(hdev);
> }
>
> +/*
> + * Some USB keyboards, like the Zenbook Duo UX8406MA keyboard, have a dedicated
> + * USB interface for vendor-specific reports, separate to the generic HID
> + * keyboard or consumer control interfaces.
> + *
> + * The kernel does not register these vendor-specific interfaces as keyboards,
> + * or perform input mapping on them at all.
> + *
> + * To work around this, a fake keyboard input can be added to the
> + * vendor-specific interface's report descriptor. The kernel then combines it
> + * with the vendor-specific collections, and allows the interface to be used as
> + * a regular keyboard with our custom mappings.
> + */
> +static const __u8 asus_fake_keyboard_rdesc[] = {
> + 0x05, 0x01, /* Usage Page (Generic Desktop) */
> + 0x09, 0x06, /* Usage (Keyboard) */
> + 0xa1, 0x01, /* Collection (Application) */
> + 0x85, 0x01, /* Report ID (1) */
> + 0x75, 0x08, /* Report Size (8) */
> + 0x95, 0x01, /* Report Count (1) */
> + 0x81, 0x00, /* Input (Data,Arr,Abs) */
Does this Input item need to be declared as Constant?
According to the HID 1.11 specification, a field mapped as padding because
no usage is specified must be declared as Constant (e.g., 0x01 or 0x03).
Declaring it as Data (0x00) violates the protocol and could cause strict
user-space HID parsers reading the raw descriptor to reject it.
> + 0xc0, /* End Collection */
> +};
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260513163248.16483-1-p.pisati@gmail.com?part=5
^ permalink raw reply
* Re: [PATCH v6 2/7] mfd: Add driver for ASUS Transformer embedded controller
From: Svyatoslav Ryhel @ 2026-05-14 11:02 UTC (permalink / raw)
To: Lee Jones
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Dmitry Torokhov,
Pavel Machek, Sebastian Reichel, Ion Agorria,
Michał Mirosław, devicetree, linux-kernel, linux-input,
linux-leds, linux-pm
In-Reply-To: <20260514100205.GG305027@google.com>
чт, 14 трав. 2026 р. о 13:02 Lee Jones <lee@kernel.org> пише:
>
> On Sat, 02 May 2026, Svyatoslav Ryhel wrote:
>
> > From: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> >
> > Support Nuvoton NPCE795-based ECs as used in Asus Transformer TF201,
> > TF300T, TF300TG, TF300TL and TF700T pad and dock, as well as TF101 dock
> > and TF600T, P1801-T and TF701T pad. This is a glue driver handling
> > detection and common operations for EC's functions.
> >
> > Co-developed-by: Svyatoslav Ryhel <clamor95@gmail.com>
> > Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> > Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> > ---
> > drivers/mfd/Kconfig | 14 +
> > drivers/mfd/Makefile | 1 +
> > drivers/mfd/asus-transformer-ec.c | 762 ++++++++++++++++++++++++
> > include/linux/mfd/asus-transformer-ec.h | 162 +++++
> > 4 files changed, 939 insertions(+)
> > create mode 100644 drivers/mfd/asus-transformer-ec.c
> > create mode 100644 include/linux/mfd/asus-transformer-ec.h
> >
> > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> > index 7192c9d1d268..5aa4facfd2df 100644
> > --- a/drivers/mfd/Kconfig
> > +++ b/drivers/mfd/Kconfig
> > @@ -137,6 +137,20 @@ config MFD_AAT2870_CORE
> > additional drivers must be enabled in order to use the
> > functionality of the device.
> >
> > +config MFD_ASUS_TRANSFORMER_EC
> > + tristate "ASUS Transformer's embedded controller"
> > + depends on I2C && OF
> > + help
> > + Support ECs found in ASUS Transformer's Pad and Mobile Dock.
> > +
> > + This provides shared glue for functional part drivers:
> > + asus-transformer-ec-kbc, asus-transformer-ec-keys,
> > + leds-asus-transformer-ec, asus-transformer-ec-battery
> > + and asus-transformer-ec-charger.
>
> A 760 line driver deserves more than just a list of leaf drivers.
>
> > + This driver can also be built as a module. If so, the module
> > + will be called asus-transformer-ec.
> > +
> > config MFD_AT91_USART
> > tristate "AT91 USART Driver"
> > select MFD_CORE
> > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> > index e75e8045c28a..fd80088d8a9a 100644
> > --- a/drivers/mfd/Makefile
> > +++ b/drivers/mfd/Makefile
> > @@ -10,6 +10,7 @@ obj-$(CONFIG_MFD_88PM805) += 88pm805.o 88pm80x.o
> > obj-$(CONFIG_MFD_88PM886_PMIC) += 88pm886.o
> > obj-$(CONFIG_MFD_ACT8945A) += act8945a.o
> > obj-$(CONFIG_MFD_SM501) += sm501.o
> > +obj-$(CONFIG_MFD_ASUS_TRANSFORMER_EC) += asus-transformer-ec.o
> > obj-$(CONFIG_ARCH_BCM2835) += bcm2835-pm.o
> > obj-$(CONFIG_MFD_BCM590XX) += bcm590xx.o
> > obj-$(CONFIG_MFD_BD9571MWV) += bd9571mwv.o
> > diff --git a/drivers/mfd/asus-transformer-ec.c b/drivers/mfd/asus-transformer-ec.c
> > new file mode 100644
> > index 000000000000..75aa7ab99387
> > --- /dev/null
> > +++ b/drivers/mfd/asus-transformer-ec.c
> > @@ -0,0 +1,762 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +
> > +#include <linux/array_size.h>
> > +#include <linux/debugfs.h>
> > +#include <linux/delay.h>
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/device.h>
>
> Should we sort the '#include' directives alphabetically? For instance,
> 'device.h' should typically appear before 'gpio/consumer.h'.
>
> > +#include <linux/err.h>
> > +#include <linux/i2c.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/mfd/asus-transformer-ec.h>
> > +#include <linux/mfd/core.h>
> > +#include <linux/mod_devicetable.h>
> > +#include <linux/module.h>
> > +#include <linux/mutex.h>
> > +#include <linux/property.h>
> > +#include <linux/slab.h>
> > +#include <linux/string.h>
> > +#include <linux/types.h>
> > +#include <linux/unaligned.h>
> > +
> > +#define ASUSEC_RSP_BUFFER_SIZE 8
> > +
> > +struct asus_ec_chip_data {
> > + const char *name;
> > + const struct mfd_cell *mfd_devices;
>
> Use global `static const structs` instead.
>
> Also, please do not pass MFD registration data through another
> registration API like DT. It opens the gates to too much hackery. I'm
> not saying that _this_ driver would do such a thing, but it's easier
> just to keep the blanket rule in place.
>
> > + unsigned int num_devices;
> > + bool clr_fmode; /* clear Factory Mode bit in EC control register */
> > +};
> > +
> > +struct asus_ec_data {
> > + struct asusec_info info;
>
> You have 'data' and 'info' which a) using non-forthcoming nomenclature
> and doesn't tell me anything and then you b) put 'info' in the device's
> driver_data attribute which is very confusing. driver_data should be
> for what we call ddata which I assume is expressed as 'data' here.
>
> > + struct mutex ecreq_lock; /* prevent simultaneous access */
> > + struct gpio_desc *ecreq;
>
> If I hadn't seen the declaration, I'd have no idea this was a GPIO
> descriptor. Please improve the nomenclature throughout.
>
> > + struct i2c_client *self;
>
> Again, please use standard naming conventions:
>
> % git grep "struct i2c_client" | grep "\*self" | wc -l
> 0
>
> % git grep "struct i2c_client" | grep "\*client" | wc -l
> 6304
>
> % git grep "struct i2c_client" | grep "\*i2c" | wc -l
> 903
>
> > + const struct asus_ec_chip_data *data;
>
> 'data', 'priv' and 'info' should be improved.
>
> > + char ec_data[DOCKRAM_ENTRY_BUFSIZE];
>
> An array of chars called 'data'. This could be anything.
>
> > + bool logging_disabled;
>
> This debugging tool is probably never going to be used again.
>
> Keep it local.
>
> > +};
> > +
> > +struct dockram_ec_data {
> > + struct mutex ctl_lock; /* prevent simultaneous access */
> > + char ctl_data[DOCKRAM_ENTRY_BUFSIZE];
> > +};
> > +
> > +#define to_ec_data(ec) \
> > + container_of(ec, struct asus_ec_data, info)
> > +
> > +/**
> > + * asus_dockram_read - Read a register from the DockRAM device.
> > + * @client: Handle to the DockRAM device.
> > + * @reg: Register to read.
> > + * @buf: Byte array into which data will be read; must be large enough to
> > + * hold the data returned by the DockRAM.
> > + *
> > + * This executes the DockRAM read based on the SMBus "block read" protocol
> > + * or its emulation. It extracts DOCKRAM_ENTRY_SIZE bytes from the set
> > + * register address.
> > + *
> > + * Returns a negative errno code else zero on success.
> > + */
> > +int asus_dockram_read(struct i2c_client *client, int reg, char *buf)
> > +{
>
> Have you considered using Regmap for register access instead of
> implementing custom functions? Remaps already deals with caching and
> locking mechanisms that you'd get for free.
>
> This looks like it would be replaced with devm_regmap_init_i2c().
>
I don't see any instances regmap handling 256 bit registers.
> > + struct device *dev = &client->dev;
> > + int ret;
> > +
> > + memset(buf, 0, DOCKRAM_ENTRY_BUFSIZE);
> > + ret = i2c_smbus_read_i2c_block_data(client, reg,
> > + DOCKRAM_ENTRY_BUFSIZE, buf);
> > + if (ret < 0)
> > + return ret;
> > +
> > + if (buf[0] > DOCKRAM_ENTRY_SIZE) {
> > + dev_err(dev, "bad data len; buffer: %*ph; ret: %d\n",
> > + DOCKRAM_ENTRY_BUFSIZE, buf, ret);
> > + return -EPROTO;
> > + }
> > +
> > + dev_dbg(dev, "got data; buffer: %*ph; ret: %d\n",
> > + DOCKRAM_ENTRY_BUFSIZE, buf, ret);
>
> Please remove all of these debug messages.
>
> > +
> > + return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(asus_dockram_read);
> > +
> > +/**
> > + * asus_dockram_write - Write a byte array to a register of the DockRAM device.
> > + * @client: Handle to the DockRAM device.
> > + * @reg: Register to write to.
> > + * @buf: Byte array to be written (up to DOCKRAM_ENTRY_SIZE bytes).
> > + *
> > + * This executes the DockRAM write based on the SMBus "block write"
> > + * protocol or its emulation. It writes DOCKRAM_ENTRY_SIZE bytes to the
> > + * specified register address.
> > + *
> > + * Returns a negative errno code else zero on success.
> > + */
> > +int asus_dockram_write(struct i2c_client *client, int reg, const char *buf)
> > +{
> > + if (buf[0] > DOCKRAM_ENTRY_SIZE)
> > + return -EINVAL;
> > +
> > + dev_dbg(&client->dev, "sending data; buffer: %*ph\n", buf[0] + 1, buf);
> > +
> > + return i2c_smbus_write_i2c_block_data(client, reg, buf[0] + 1, buf);
> > +}
> > +EXPORT_SYMBOL_GPL(asus_dockram_write);
> > +
> > +/**
> > + * asus_dockram_access_ctl - Read from or write to the DockRAM control register.
> > + * @client: Handle to the DockRAM device.
> > + * @out: Pointer to a variable where the register value will be stored.
> > + * @mask: Bitmask of bits to be cleared.
> > + * @xor: Bitmask of bits to be set (via XOR).
> > + *
> > + * This performs a control register read if @out is provided and both @mask
> > + * and @xor are zero. Otherwise, it performs a control register update if
> > + * @mask and @xor are provided.
> > + *
> > + * Returns a negative errno code else zero on success.
> > + */
> > +int asus_dockram_access_ctl(struct i2c_client *client, u64 *out, u64 mask,
> > + u64 xor)
> > +{
> > + struct dockram_ec_data *priv = i2c_get_clientdata(client);
> > + char *buf = priv->ctl_data;
> > + u64 val;
> > + int ret = 0;
> > +
> > + guard(mutex)(&priv->ctl_lock);
> > +
> > + ret = asus_dockram_read(client, ASUSEC_DOCKRAM_CONTROL, buf);
> > + if (ret < 0)
>
> Could we check for errors using 'if (ret)' instead of 'if (ret < 0)' here,
> unless a positive return value has a specific meaning we need to handle?
>
> > + goto exit;
> > +
> > + if (buf[0] != ASUSEC_CTL_SIZE) {
> > + ret = -EPROTO;
> > + goto exit;
> > + }
> > +
> > + val = get_unaligned_le64(buf + 1);
> > +
> > + if (out)
> > + *out = val;
> > +
> > + if (mask || xor) {
> > + put_unaligned_le64((val & ~mask) ^ xor, buf + 1);
> > + ret = asus_dockram_write(client, ASUSEC_DOCKRAM_CONTROL, buf);
> > + }
> > +
> > +exit:
> > + if (ret < 0)
> > + dev_err(&client->dev, "Failed to access control flags: %d\n",
> > + ret);
> > +
> > + return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(asus_dockram_access_ctl);
> > +
> > +static void asus_ec_remove_notifier(struct device *dev, void *res)
> > +{
> > + struct asusec_info *ec = dev_get_drvdata(dev->parent);
> > + struct notifier_block **nb = res;
> > +
> > + blocking_notifier_chain_unregister(&ec->notify_list, *nb);
> > +}
> > +
> > +/**
> > + * devm_asus_ec_register_notifier - Managed registration of notifier to an
> > + * ASUS EC blocking notifier chain.
> > + * @pdev: Device requesting the notifier (used for resource management).
> > + * @nb: Notifier block to be registered.
> > + *
> > + * Register a notifier to the ASUS EC blocking notifier chain. The notifier
> > + * will be automatically unregistered when the requesting device is detached.
> > + *
> > + * Return: 0 on success or a negative error code on failure.
> > + */
> > +int devm_asus_ec_register_notifier(struct platform_device *pdev,
> > + struct notifier_block *nb)
> > +{
>
> Hand-rolling devres managed resources is usually reserved for subsystem
> level API calls. Why do the usual device driver .remove() handling work
> for you?
>
> > + struct asusec_info *ec = dev_get_drvdata(pdev->dev.parent);
> > + struct notifier_block **res;
> > + int ret;
> > +
> > + res = devres_alloc(asus_ec_remove_notifier, sizeof(*res), GFP_KERNEL);
> > + if (!res)
> > + return -ENOMEM;
> > +
> > + *res = nb;
> > + ret = blocking_notifier_chain_register(&ec->notify_list, nb);
> > + if (ret) {
> > + devres_free(res);
> > + return ret;
> > + }
> > +
> > + devres_add(&pdev->dev, res);
> > +
> > + return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(devm_asus_ec_register_notifier);
> > +
> > +static int asus_ec_signal_request(const struct asusec_info *ec)
> > +{
> > + struct asus_ec_data *priv = to_ec_data(ec);
> > +
> > + guard(mutex)(&priv->ecreq_lock);
> > +
> > + dev_dbg(&priv->self->dev, "EC request\n");
> > +
> > + gpiod_set_value_cansleep(priv->ecreq, 1);
> > + msleep(50);
> > +
> > + gpiod_set_value_cansleep(priv->ecreq, 0);
> > + msleep(200);
> > +
> > + return 0;
> > +}
> > +
> > +static int asus_ec_write(struct asus_ec_data *priv, u16 data)
> > +{
> > + int ret = i2c_smbus_write_word_data(priv->self, ASUSEC_WRITE_BUF, data);
> > +
> > + dev_dbg(&priv->self->dev, "EC write: %04x, ret = %d\n", data, ret);
> > +
> > + return ret;
> > +}
> > +
> > +static int asus_ec_read(struct asus_ec_data *priv, bool in_irq)
> > +{
> > + int ret = i2c_smbus_read_i2c_block_data(priv->self, ASUSEC_READ_BUF,
> > + sizeof(priv->ec_data),
> > + priv->ec_data);
> > +
> > + dev_dbg(&priv->self->dev, "EC read: %*ph, ret = %d%s\n",
> > + sizeof(priv->ec_data), priv->ec_data,
> > + ret, in_irq ? "; in irq" : "");
> > +
> > + return ret;
> > +}
>
> No abstractions for the sake of it. All this goes away with Regmap anyway.
> > +
> > +/**
> > + * asus_ec_i2c_command - Send a 16-bit command to the ASUS EC.
> > + * @ec: Pointer to the shared ASUS EC structure.
> > + * @data: The 16-bit command (word) to be sent.
> > + *
> > + * Return: 0 on success or a negative error code on failure.
> > + */
> > +int asus_ec_i2c_command(const struct asusec_info *ec, u16 data)
> > +{
> > + return asus_ec_write(to_ec_data(ec), data);
> > +}
>
> Is this wrapper function strictly necessary? We should try to avoid
> superfluous abstractions that do nothing but call another function.
>
> > +EXPORT_SYMBOL_GPL(asus_ec_i2c_command);
> > +
> > +static void asus_ec_clear_buffer(struct asus_ec_data *priv)
> > +{
> > + int retry = ASUSEC_RSP_BUFFER_SIZE;
> > +
> > + while (retry--) {
> > + if (asus_ec_read(priv, false) < 0)
> > + continue;
> > +
> > + if (priv->ec_data[1] & ASUSEC_OBF_MASK)
> > + continue;
> > +
> > + break;
> > + }
> > +}
> > +
> > +static int asus_ec_log_info(struct asus_ec_data *priv, unsigned int reg,
> > + const char *name, char **out)
> > +{
> > + char buf[DOCKRAM_ENTRY_BUFSIZE];
> > + int ret;
> > +
> > + ret = asus_dockram_read(priv->info.dockram, reg, buf);
> > + if (ret < 0)
> > + return ret;
> > +
> > + if (!priv->logging_disabled)
> > + dev_info(&priv->self->dev, "%-14s: %.*s\n", name,
> > + buf[0], buf + 1);
> > +
> > + if (out)
> > + *out = kstrndup(buf + 1, buf[0], GFP_KERNEL);
> > +
> > + return 0;
> > +}
> > +
> > +static int asus_ec_reset(struct asus_ec_data *priv)
> > +{
> > + int retry, ret;
> > +
> > + for (retry = 0; retry < 3; retry++) {
> > + ret = asus_ec_write(priv, 0);
> > + if (!ret)
> > + return 0;
> > +
> > + msleep(300);
> > + }
> > +
> > + return ret;
> > +}
> > +
> > +static int asus_ec_magic_debug(struct asus_ec_data *priv)
> > +{
> > + u64 flag;
> > + int ret;
> > +
> > + ret = asus_ec_get_ctl(&priv->info, &flag);
> > + if (ret < 0)
> > + return ret;
> > +
> > + flag &= ASUSEC_CTL_SUSB_MODE;
> > + dev_info(&priv->self->dev, "EC FW behaviour: %s\n",
> > + flag ? "susb on when receive ec_req" :
> > + "susb on when system wakeup");
> > +
> > + return 0;
> > +}
> > +
> > +static int asus_ec_set_factory_mode(struct asus_ec_data *priv, bool on)
> > +{
> > + dev_info(&priv->self->dev, "Entering %s mode.\n", on ? "factory" :
> > + "normal");
> > +
> > + return asus_ec_update_ctl(&priv->info, ASUSEC_CTL_FACTORY_MODE,
> > + on ? ASUSEC_CTL_FACTORY_MODE : 0);
> > +}
> > +
> > +static int asus_ec_detect(struct asus_ec_data *priv)
> > +{
> > + char *model = NULL;
> > + int ret;
> > +
> > + ret = asus_ec_reset(priv);
> > + if (ret)
> > + goto err_exit;
> > +
> > + asus_ec_clear_buffer(priv);
> > +
> > + ret = asus_ec_log_info(priv, ASUSEC_DOCKRAM_INFO_MODEL, "model", &model);
> > + if (ret)
> > + goto err_exit;
> > +
> > + ret = asus_ec_log_info(priv, ASUSEC_DOCKRAM_INFO_FW, "FW version", NULL);
> > + if (ret)
> > + goto err_exit;
> > +
> > + ret = asus_ec_log_info(priv, ASUSEC_DOCKRAM_INFO_CFGFMT, "Config format", NULL);
> > + if (ret)
> > + goto err_exit;
> > +
> > + ret = asus_ec_log_info(priv, ASUSEC_DOCKRAM_INFO_HW, "HW version", NULL);
> > + if (ret)
> > + goto err_exit;
> > +
> > + priv->logging_disabled = true;
> > +
> > + ret = asus_ec_magic_debug(priv);
> > + if (ret)
> > + goto err_exit;
> > +
> > + priv->info.model = model;
> > + priv->info.name = priv->data->name;
> > +
> > + if (priv->data->clr_fmode)
> > + asus_ec_set_factory_mode(priv, false);
> > +
> > +err_exit:
> > + if (ret)
> > + dev_err(&priv->self->dev, "failed to access EC: %d\n", ret);
> > +
> > + return ret;
> > +}
> > +
> > +static void asus_ec_handle_smi(struct asus_ec_data *priv, unsigned int code)
> > +{
> > + dev_dbg(&priv->self->dev, "SMI interrupt: 0x%02x\n", code);
> > +
> > + switch (code) {
> > + case ASUSEC_SMI_HANDSHAKE:
> > + case ASUSEC_SMI_RESET:
> > + asus_ec_detect(priv);
> > + break;
> > + }
> > +}
> > +
> > +static irqreturn_t asus_ec_interrupt(int irq, void *dev_id)
> > +{
> > + struct asus_ec_data *priv = dev_id;
> > + unsigned long notify_action;
> > + int ret;
> > +
> > + ret = asus_ec_read(priv, true);
> > + if (ret <= 0 || !(priv->ec_data[1] & ASUSEC_OBF_MASK))
> > + return IRQ_NONE;
> > +
> > + notify_action = priv->ec_data[1];
> > + if (notify_action & ASUSEC_SMI_MASK) {
> > + unsigned int code = priv->ec_data[2];
> > +
> > + asus_ec_handle_smi(priv, code);
> > +
> > + notify_action |= code << 8;
> > + dev_dbg(&priv->self->dev, "SMI code: 0x%02x\n", code);
> > + }
> > +
> > + blocking_notifier_call_chain(&priv->info.notify_list,
> > + notify_action, priv->ec_data);
> > +
> > + return IRQ_HANDLED;
> > +}
> > +
> > +static ssize_t dockram_read(struct file *filp, char __user *buf,
> > + size_t count, loff_t *ppos)
> > +{
> > + struct i2c_client *client = filp->private_data;
> > + unsigned int reg, rsize;
> > + ssize_t n_read = 0, val;
> > + loff_t off = *ppos;
> > + char *data;
> > + int ret;
> > +
> > + reg = off / DOCKRAM_ENTRY_SIZE;
> > + off %= DOCKRAM_ENTRY_SIZE;
> > + rsize = DOCKRAM_ENTRIES * DOCKRAM_ENTRY_SIZE;
> > +
> > + if (!count)
> > + return 0;
> > +
> > + data = kmalloc(DOCKRAM_ENTRY_BUFSIZE, GFP_KERNEL);
> > +
> > + while (reg < DOCKRAM_ENTRIES) {
> > + unsigned int len = DOCKRAM_ENTRY_SIZE - off;
> > +
> > + if (len > rsize)
> > + len = rsize;
> > +
> > + ret = asus_dockram_read(client, reg, data);
> > + if (ret < 0) {
> > + if (!n_read)
> > + n_read = ret;
> > + break;
> > + }
> > +
> > + val = copy_to_user(buf, data + 1 + off, len);
> > + if (val == len)
> > + return -EFAULT;
> > +
> > + *ppos += len;
> > + n_read += len;
> > +
> > + if (len == rsize)
> > + break;
> > +
> > + rsize -= len;
> > + buf += len;
> > + off = 0;
> > + ++reg;
> > + }
> > +
> > + kfree(data);
> > +
> > + return n_read;
> > +}
> > +
> > +static int dockram_write_one(struct i2c_client *client, int reg,
> > + const char __user *buf, size_t count)
> > +{
> > + struct dockram_ec_data *priv = i2c_get_clientdata(client);
> > + int ret;
> > +
> > + if (!count || count > DOCKRAM_ENTRY_SIZE)
> > + return -EINVAL;
> > + if (buf[0] != count - 1)
> > + return -EINVAL;
> > +
> > + guard(mutex)(&priv->ctl_lock);
> > +
> > + priv->ctl_data[0] = (u8)count;
> > + memcpy(priv->ctl_data + 1, buf, count);
> > + ret = asus_dockram_write(client, reg, priv->ctl_data);
> > +
> > + return ret;
> > +}
> > +
> > +static ssize_t dockram_write(struct file *filp, const char __user *buf,
> > + size_t count, loff_t *ppos)
> > +{
> > + struct i2c_client *client = filp->private_data;
> > + unsigned int reg;
> > + loff_t off = *ppos;
> > + int ret;
> > +
> > + if (off % DOCKRAM_ENTRY_SIZE != 0)
> > + return -EINVAL;
> > +
> > + reg = off / DOCKRAM_ENTRY_SIZE;
> > + if (reg >= DOCKRAM_ENTRIES)
> > + return -EINVAL;
> > +
> > + ret = dockram_write_one(client, reg, buf, count);
> > +
> > + return ret < 0 ? ret : count;
> > +}
> > +
> > +static const struct debugfs_short_fops dockram_fops = {
> > + .read = dockram_read,
> > + .write = dockram_write,
> > + .llseek = default_llseek,
> > +};
>
> You should not be giving userspace free reign over device memory.
>
> If you switch to Regmap, you can use regmap-debugfs.c for this.
>
> > +static int control_reg_get(void *ec, u64 *val)
> > +{
> > + return asus_ec_get_ctl(ec, val);
> > +}
> > +
> > +static int control_reg_set(void *ec, u64 val)
> > +{
> > + return asus_ec_update_ctl(ec, ~0ull, val);
> > +}
> > +
> > +DEFINE_DEBUGFS_ATTRIBUTE(control_reg_fops, control_reg_get,
> > + control_reg_set, "%016llx\n");
> > +
> > +static int ec_request_set(void *ec, u64 val)
> > +{
> > + if (val)
> > + asus_ec_signal_request(ec);
> > +
> > + return 0;
> > +}
> > +
> > +DEFINE_DEBUGFS_ATTRIBUTE(ec_request_fops, NULL, ec_request_set, "%llu\n");
> > +
> > +static int ec_irq_set(void *ec, u64 val)
> > +{
> > + struct asus_ec_data *priv = to_ec_data(ec);
> > +
> > + if (val)
> > + irq_wake_thread(priv->self->irq, priv);
> > +
> > + return 0;
> > +}
> > +
> > +DEFINE_DEBUGFS_ATTRIBUTE(ec_irq_fops, NULL, ec_irq_set, "%llu\n");
> > +
> > +static void asus_ec_debugfs_remove(void *debugfs_root)
> > +{
> > + debugfs_remove_recursive(debugfs_root);
> > +}
> > +
> > +static void devm_asus_ec_debugfs_init(struct device *dev)
> > +{
> > + struct asusec_info *ec = dev_get_drvdata(dev);
> > + struct asus_ec_data *priv = to_ec_data(ec);
> > + struct dentry *debugfs_root, *dockram_dir;
> > + char *name = devm_kasprintf(dev, GFP_KERNEL, "asus-ec-%s",
> > + priv->data->name);
> > +
> > + debugfs_root = debugfs_create_dir(name, NULL);
> > + dockram_dir = debugfs_create_dir("dockram", debugfs_root);
> > +
> > + debugfs_create_file("ec_irq", 0200, debugfs_root, ec,
> > + &ec_irq_fops);
> > + debugfs_create_file("ec_request", 0200, debugfs_root, ec,
> > + &ec_request_fops);
> > + debugfs_create_file("control_reg", 0644, dockram_dir, ec,
> > + &control_reg_fops);
> > + debugfs_create_file("dockram", 0644, dockram_dir,
> > + priv->info.dockram, &dockram_fops);
> > +
> > + devm_add_action_or_reset(dev, asus_ec_debugfs_remove, debugfs_root);
> > +}
>
> Why is this being controlled via debugfs?
>
> Use the correct kernel APIs instead.
>
> If this is just for development, keep it and all of the extra verbose
> printing in the BSP tree. It does not belong in the upstream kernel.
>
> > +static void asus_ec_release_dockram_dev(void *client)
> > +{
> > + i2c_unregister_device(client);
> > +}
> > +
> > +static struct i2c_client *devm_asus_dockram_get(struct device *dev)
> > +{
> > + struct i2c_client *parent = to_i2c_client(dev);
> > + struct i2c_client *dockram;
> > + struct dockram_ec_data *priv;
> > + int ret;
> > +
> > + dockram = i2c_new_ancillary_device(parent, "dockram",
> > + parent->addr + 2);
> > + if (IS_ERR(dockram))
> > + return dockram;
> > +
> > + ret = devm_add_action_or_reset(dev, asus_ec_release_dockram_dev,
> > + dockram);
> > + if (ret)
> > + return ERR_PTR(ret);
> > +
> > + priv = devm_kzalloc(&dockram->dev, sizeof(*priv), GFP_KERNEL);
> > + if (!priv)
> > + return ERR_PTR(-ENOMEM);
> > +
> > + i2c_set_clientdata(dockram, priv);
> > + mutex_init(&priv->ctl_lock);
> > +
> > + return dockram;
> > +}
> > +
> > +static int asus_ec_probe(struct i2c_client *client)
> > +{
> > + struct device *dev = &client->dev;
> > + struct asus_ec_data *priv;
>
> Could we use a clearer, more specific name for the private data variable
> instead of the generic term 'priv'? Using something like 'ddata' is usually
> preferred.
>
> > + int ret;
> > +
> > + if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_I2C_BLOCK))
> > + return dev_err_probe(dev, -ENXIO,
> > + "I2C bus is missing required SMBus block mode support\n");
> > +
> > + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > + if (!priv)
> > + return -ENOMEM;
> > +
> > + priv->data = device_get_match_data(dev);
> > + if (!priv->data)
> > + return -ENODEV;
> > +
> > + i2c_set_clientdata(client, priv);
> > + priv->self = client;
> > +
> > + priv->info.dockram = devm_asus_dockram_get(dev);
> > + if (IS_ERR(priv->info.dockram))
> > + return dev_err_probe(dev, PTR_ERR(priv->info.dockram),
> > + "failed to get dockram\n");
> > +
> > + priv->ecreq = devm_gpiod_get(dev, "request", GPIOD_OUT_LOW);
> > + if (IS_ERR(priv->ecreq))
> > + return dev_err_probe(dev, PTR_ERR(priv->ecreq),
> > + "failed to get request GPIO\n");
>
> "get" or "request"
>
> > + BLOCKING_INIT_NOTIFIER_HEAD(&priv->info.notify_list);
> > + mutex_init(&priv->ecreq_lock);
> > +
> > + asus_ec_signal_request(&priv->info);
> > +
> > + ret = asus_ec_detect(priv);
> > + if (ret)
> > + return dev_err_probe(dev, ret, "failed to detect EC version\n");
> > +
> > + ret = devm_request_threaded_irq(dev, client->irq, NULL,
> > + &asus_ec_interrupt,
> > + IRQF_ONESHOT | IRQF_SHARED,
> > + client->name, priv);
> > + if (ret)
> > + return dev_err_probe(dev, ret, "failed to register IRQ\n");
> > +
> > + /* Parent I2C controller uses DMA, ASUS EC and child devices do not */
> > + client->dev.coherent_dma_mask = 0;
> > + client->dev.dma_mask = &client->dev.coherent_dma_mask;
> > +
> > + if (IS_ENABLED(CONFIG_DEBUG_FS))
> > + devm_asus_ec_debugfs_init(dev);
> > +
> > + return devm_mfd_add_devices(dev, 0, priv->data->mfd_devices,
> > + priv->data->num_devices, NULL, 0, NULL);
> > +}
> > +
> > +static const struct mfd_cell asus_ec_sl101_dock_mfd_devices[] = {
> > + {
> > + .name = "asus-transformer-ec-kbc",
> > + },
> > +};
> > +
> > +static const struct asus_ec_chip_data asus_ec_sl101_dock_data = {
> > + .name = "dock",
> > + .mfd_devices = asus_ec_sl101_dock_mfd_devices,
> > + .num_devices = ARRAY_SIZE(asus_ec_sl101_dock_mfd_devices),
> > + .clr_fmode = false,
> > +};
> > +
> > +static const struct mfd_cell asus_ec_tf101_dock_mfd_devices[] = {
> > + {
> > + .name = "asus-transformer-ec-battery",
> > + .id = 1,
>
> Why doesn't PLATFORM_DEVID_AUTO already do the right thing?
>
> > + }, {
> > + .name = "asus-transformer-ec-charger",
> > + .id = 1,
> > + }, {
> > + .name = "asus-transformer-ec-led",
> > + .id = 1,
> > + }, {
> > + .name = "asus-transformer-ec-keys",
> > + }, {
> > + .name = "asus-transformer-ec-kbc",
> > + },
> > +};
> > +
> > +static const struct asus_ec_chip_data asus_ec_tf101_dock_data = {
> > + .name = "dock",
> > + .mfd_devices = asus_ec_tf101_dock_mfd_devices,
> > + .num_devices = ARRAY_SIZE(asus_ec_tf101_dock_mfd_devices),
> > + .clr_fmode = false,
> > +};
> > +
> > +static const struct mfd_cell asus_ec_tf201_pad_mfd_devices[] = {
> > + {
> > + .name = "asus-transformer-ec-battery",
> > + .id = 0,
> > + }, {
> > + .name = "asus-transformer-ec-led",
> > + .id = 0,
> > + },
> > +};
> > +
> > +static const struct asus_ec_chip_data asus_ec_tf201_pad_data = {
> > + .name = "pad",
> > + .mfd_devices = asus_ec_tf201_pad_mfd_devices,
> > + .num_devices = ARRAY_SIZE(asus_ec_tf201_pad_mfd_devices),
> > + .clr_fmode = true,
> > +};
> > +
> > +static const struct mfd_cell asus_ec_tf600t_pad_mfd_devices[] = {
> > + {
> > + .name = "asus-transformer-ec-battery",
> > + .id = 0,
> > + }, {
> > + .name = "asus-transformer-ec-charger",
> > + .id = 0,
> > + }, {
> > + .name = "asus-transformer-ec-led",
> > + .id = 0,
> > + },
> > +};
> > +
> > +static const struct asus_ec_chip_data asus_ec_tf600t_pad_data = {
> > + .name = "pad",
> > + .mfd_devices = asus_ec_tf600t_pad_mfd_devices,
> > + .num_devices = ARRAY_SIZE(asus_ec_tf600t_pad_mfd_devices),
> > + .clr_fmode = true,
> > +};
> > +
> > +static const struct of_device_id asus_ec_match[] = {
> > + { .compatible = "asus,sl101-ec-dock", .data = &asus_ec_sl101_dock_data },
> > + { .compatible = "asus,tf101-ec-dock", .data = &asus_ec_tf101_dock_data },
>
> Could we use the match table's data field to store an 'enum' or integer ID
> instead of passing platform data via the '.data' field? We could then use a
> 'switch' statement in the C code to select the correct 'mfd_cell' array.
>
> > + { .compatible = "asus,tf201-ec-pad", .data = &asus_ec_tf201_pad_data },
> > + { .compatible = "asus,tf600t-ec-pad", .data = &asus_ec_tf600t_pad_data },
> > + { }
> > +};
> > +MODULE_DEVICE_TABLE(of, asus_ec_match);
> > +
> > +static struct i2c_driver asus_ec_driver = {
> > + .driver = {
> > + .name = "asus-transformer-ec",
> > + .of_match_table = asus_ec_match,
> > + },
> > + .probe = asus_ec_probe,
> > +};
> > +module_i2c_driver(asus_ec_driver);
> > +
> > +MODULE_AUTHOR("Michał Mirosław <mirq-linux@rere.qmqm.pl>");
> > +MODULE_AUTHOR("Svyatoslav Ryhel <clamor95@gmail.com>");
> > +MODULE_DESCRIPTION("ASUS Transformer's EC driver");
> > +MODULE_LICENSE("GPL");
> > diff --git a/include/linux/mfd/asus-transformer-ec.h b/include/linux/mfd/asus-transformer-ec.h
> > new file mode 100644
> > index 000000000000..0a72de40352e
> > --- /dev/null
> > +++ b/include/linux/mfd/asus-transformer-ec.h
> > @@ -0,0 +1,162 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +#ifndef __MFD_ASUS_TRANSFORMER_EC_H
> > +#define __MFD_ASUS_TRANSFORMER_EC_H
> > +
> > +#include <linux/notifier.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/workqueue.h>
> > +
> > +struct i2c_client;
> > +
> > +struct asusec_info {
> > + const char *model;
> > + const char *name;
> > + struct i2c_client *dockram;
> > + struct workqueue_struct *wq;
> > + struct blocking_notifier_head notify_list;
> > +};
> > +
> > +#define DOCKRAM_ENTRIES 0x100
> > +#define DOCKRAM_ENTRY_SIZE 32
> > +#define DOCKRAM_ENTRY_BUFSIZE (DOCKRAM_ENTRY_SIZE + 1)
> > +
> > +/* interrupt sources */
> > +#define ASUSEC_OBF_MASK BIT(0)
> > +#define ASUSEC_KEY_MASK BIT(2)
> > +#define ASUSEC_KBC_MASK BIT(3)
> > +#define ASUSEC_AUX_MASK BIT(5)
> > +#define ASUSEC_SCI_MASK BIT(6)
> > +#define ASUSEC_SMI_MASK BIT(7)
> > +
> > +/* SMI notification codes */
> > +#define ASUSEC_SMI_POWER_NOTIFY 0x31 /* [un]plugging USB cable */
> > +#define ASUSEC_SMI_HANDSHAKE 0x50 /* response to ec_req edge */
> > +#define ASUSEC_SMI_WAKE 0x53
> > +#define ASUSEC_SMI_RESET 0x5f
> > +#define ASUSEC_SMI_ADAPTER_EVENT 0x60 /* [un]plugging charger to dock */
> > +#define ASUSEC_SMI_BACKLIGHT_ON 0x63
> > +#define ASUSEC_SMI_AUDIO_DOCK_IN 0x70
> > +
> > +#define ASUSEC_SMI_ACTION(code) (ASUSEC_SMI_MASK | ASUSEC_OBF_MASK | \
> > + (ASUSEC_SMI_##code << 8))
> > +
> > +/* control register [0x0a] layout */
> > +#define ASUSEC_CTL_SIZE 8
> > +
> > +/*
> > + * EC reports power from 40-pin connector in the LSB of the control
> > + * register. The following values have been observed (xor 0x02):
> > + *
> > + * PAD-ec no-plug 0x40 / PAD-ec DOCK 0x20 / DOCK-ec no-plug 0x40
> > + * PAD-ec AC 0x25 / PAD-ec DOCK+AC 0x24 / DOCK-ec AC 0x25
> > + * PAD-ec USB 0x45 / PAD-ec DOCK+USB 0x24 / DOCK-ec USB 0x41
> > + */
> > +
> > +#define ASUSEC_CTL_DIRECT_POWER_SOURCE BIT_ULL(0)
> > +#define ASUSEC_STAT_CHARGING BIT_ULL(2)
> > +#define ASUSEC_CTL_FULL_POWER_SOURCE BIT_ULL(5)
> > +#define ASUSEC_CTL_SUSB_MODE BIT_ULL(9)
> > +#define ASUSEC_CMD_SUSPEND_S3 BIT_ULL(33)
> > +#define ASUSEC_CTL_TEST_DISCHARGE BIT_ULL(35)
> > +#define ASUSEC_CMD_SUSPEND_INHIBIT BIT_ULL(37)
> > +#define ASUSEC_CTL_FACTORY_MODE BIT_ULL(38)
> > +#define ASUSEC_CTL_KEEP_AWAKE BIT_ULL(39)
> > +#define ASUSEC_CTL_USB_CHARGE BIT_ULL(40)
> > +#define ASUSEC_CTL_LED_BLINK BIT_ULL(40)
> > +#define ASUSEC_CTL_LED_AMBER BIT_ULL(41)
> > +#define ASUSEC_CTL_LED_GREEN BIT_ULL(42)
> > +#define ASUSEC_CMD_SWITCH_HDMI BIT_ULL(56)
> > +#define ASUSEC_CMD_WIN_SHUTDOWN BIT_ULL(62)
> > +
> > +#define ASUSEC_DOCKRAM_INFO_MODEL 0x01
> > +#define ASUSEC_DOCKRAM_INFO_FW 0x02
> > +#define ASUSEC_DOCKRAM_INFO_CFGFMT 0x03
> > +#define ASUSEC_DOCKRAM_INFO_HW 0x04
> > +#define ASUSEC_DOCKRAM_CONTROL 0x0a
> > +#define ASUSEC_DOCKRAM_BATT_CTL 0x14
> > +
> > +#define ASUSEC_WRITE_BUF 0x64
> > +#define ASUSEC_READ_BUF 0x6a
> > +
> > +/* dockram comm */
> > +int asus_dockram_read(struct i2c_client *client, int reg, char *buf);
> > +int asus_dockram_write(struct i2c_client *client, int reg, const char *buf);
> > +int asus_dockram_access_ctl(struct i2c_client *client,
> > + u64 *out, u64 mask, u64 xor);
> > +
> > +/* EC public API */
> > +
> > +/**
> > + * cell_to_ec - Request the shared ASUS EC structure via a subdevice's pdev.
> > + * @pdev: EC subdevice pdev requesting access to the shared ASUS EC structure.
> > + *
> > + * Returns a pointer to the asusec_info structure.
> > + */
> > +static inline struct asusec_info *cell_to_ec(struct platform_device *pdev)
> > +{
> > + return dev_get_drvdata(pdev->dev.parent);
> > +}
>
> This is both misleading and already exists.
>
> > +
> > +/**
> > + * asus_ec_get_ctl - Read from the DockRAM control register.
> > + * @ec: Pointer to the shared ASUS EC structure.
> > + * @out: Pointer to the variable where the register value will be stored.
> > + *
> > + * Performs a control register read and stores the value in @out.
> > + *
> > + * Return: 0 on success, or a negative errno code on failure.
> > + */
> > +static inline int asus_ec_get_ctl(const struct asusec_info *ec, u64 *out)
> > +{
> > + return asus_dockram_access_ctl(ec->dockram, out, 0, 0);
> > +}
> > +
> > +/**
> > + * asus_ec_update_ctl - Update the DockRAM control register.
> > + * @ec: Pointer to the shared ASUS EC structure.
> > + * @mask: Bitmask of bits to be cleared.
> > + * @xor: Bitmask of bits to be toggled or set (via XOR).
> > + *
> > + * Performs a read-modify-write update on the control register using
> > + * the provided @mask and @xor values.
> > + *
> > + * Return: 0 on success, or a negative errno code on failure.
> > + */
> > +static inline int asus_ec_update_ctl(const struct asusec_info *ec,
> > + u64 mask, u64 xor)
> > +{
> > + return asus_dockram_access_ctl(ec->dockram, NULL, mask, xor);
> > +}
> > +
> > +/**
> > + * asus_ec_set_ctl_bits - Sets bits of the DockRAM control register.
> > + * @ec: Pointer to the shared ASUS EC structure.
> > + * @mask: Bitmask of bits to be set.
> > + *
> > + * Sets bits of the control register using the provided @mask value.
> > + *
> > + * Return: 0 on success, or a negative errno code on failure.
> > + */
> > +static inline int asus_ec_set_ctl_bits(const struct asusec_info *ec, u64 mask)
> > +{
> > + return asus_dockram_access_ctl(ec->dockram, NULL, mask, mask);
> > +}
> > +
> > +/**
> > + * asus_ec_clear_ctl_bits - Clears bits of the DockRAM control register.
> > + * @ec: Pointer to the shared ASUS EC structure.
> > + * @mask: Bitmask of bits to be cleared.
> > + *
> > + * Clears bits of the control register using the provided @mask value.
> > + *
> > + * Return: 0 on success, or a negative errno code on failure.
> > + */
> > +static inline int asus_ec_clear_ctl_bits(const struct asusec_info *ec, u64 mask)
> > +{
> > + return asus_dockram_access_ctl(ec->dockram, NULL, mask, 0);
> > +}
>
> We don't typically allow abstraction for the sake of abstraction.
>
> > +int asus_ec_i2c_command(const struct asusec_info *ec, u16 data);
> > +int devm_asus_ec_register_notifier(struct platform_device *dev,
> > + struct notifier_block *nb);
> > +#endif /* __MFD_ASUS_TRANSFORMER_EC_H */
> > --
> > 2.51.0
> >
> >
>
> --
> Lee Jones
^ permalink raw reply
* Re: [PATCH 6/7] hid-asus: Allow adding custom hotkey handler logic
From: sashiko-bot @ 2026-05-14 11:14 UTC (permalink / raw)
To: Paolo Pisati; +Cc: dmitry.torokhov, linux-input
In-Reply-To: <20260513163248.16483-7-p.pisati@gmail.com>
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Calling platform_profile_cycle() from an atomic context causes a 'scheduling while atomic' bug.
- [High] Missing Kconfig dependency on ACPI_PLATFORM_PROFILE leads to a linker error.
- [Medium] Improper HID event return value causes spurious error logs and incorrect event propagation.
- [Low] Dead code due to unreachable break statement.
--
commit f4dcdddfbfb9d5acd09f6a961fcad0539b0cd8c7
Author: Joshua Leivenzon <hacker1024@users.sourceforge.net>
hid-asus: Allow adding custom hotkey handler logic
This commit modifies the Asus HID driver to handle a custom vendor hotkey
for cycling the platform profile when a specific usage code is received.
> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index fe52fd63d61c6..37e3aec612b6d 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c
> @@ -29,6 +29,7 @@
> #include <linux/input/mt.h>
> #include <linux/usb.h> /* For to_usb_interface for T100 touchpad intf check */
> #include <linux/power_supply.h>
> +#include <linux/platform_profile.h>
Does the HID_ASUS Kconfig option need to depend on or select
ACPI_PLATFORM_PROFILE? If the kernel is built with
CONFIG_ACPI_PLATFORM_PROFILE=n, won't this result in a linker error since
platform_profile_cycle() is used unconditionally and there are no empty dummy
stubs provided in the header?
[ ... ]
> @@ -362,8 +363,21 @@ static int asus_event(struct hid_device *hdev, struct hid_field *field,
> if ((usage->hid & HID_USAGE_PAGE) == HID_UP_ASUSVENDOR &&
> (usage->hid & HID_USAGE) != 0x00 &&
> (usage->hid & HID_USAGE) != 0xff && !usage->type) {
> - hid_warn(hdev, "Unmapped Asus vendor usagepage code 0x%02x\n",
> - usage->hid & HID_USAGE);
> +
> + /*
> + * Some reports do not map directly to standard keys, and need special
> + * handling.
> + */
> + switch (usage->hid & HID_USAGE) {
> + case 0x9d:
> + if (!value)
> + break;
> + return platform_profile_cycle();
Is it safe to call platform_profile_cycle() from here?
The asus_event() callback is invoked by the HID core in an atomic softirq
context when processing USB URB completions. However,
platform_profile_cycle() acquires profile_lock, which is a mutex. Will this
result in a scheduling while atomic panic? Should this action be deferred to
a workqueue, similar to how fn_lock_sync_work is handled?
Also, if platform_profile_cycle() returns a negative error code (like -EINVAL
when no profiles are registered), returning it directly here will cause
hid_process_event() to spam the kernel log with error messages on every key
press. Conversely, if it returns 0 on success, won't the HID core incorrectly
continue processing this event instead of treating it as consumed? Does this
need to handle the return value and return 1 to halt further event
processing?
> + break;
This isn't a bug, but this break statement is unreachable since the previous
line always returns.
> + default:
> + hid_warn(hdev, "Unmapped Asus vendor usagepage code 0x%02x\n",
> + usage->hid & HID_USAGE);
> + }
> }
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260513163248.16483-1-p.pisati@gmail.com?part=6
^ permalink raw reply
* Re: [PATCH v2 1/4] HID: hid-msi: Add MSI Claw configuration driver
From: sashiko-bot @ 2026-05-14 11:51 UTC (permalink / raw)
To: Derek J. Clark; +Cc: linux-input, dmitry.torokhov
In-Reply-To: <20260513231445.3213501-2-derekjohn.clark@gmail.com>
Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] The `guard(mutex)` macro in `claw_hw_output_report` is scoped to an `if` block, prematurely releasing the lock before the critical section.
- [High] The driver publishes `drvdata` via `hid_set_drvdata()` before fully initializing its mutex and completion objects, creating a race condition.
- [Medium] An invalid, uninitialized gamepad mode (0) may be sent to the hardware during system resume.
- [Medium] The 8ms timeout used in `claw_hw_output_report()` is vulnerable to jiffies rounding, leading to spurious read failures.
- [Medium] The commit message claims probe checks MCU RAM address variances, but the implementation is missing.
--
commit 13b388de96d7484eee908adeed127e8086fc289e
Author: Derek J. Clark <derekjohn.clark@gmail.com>
HID: hid-msi: Add MSI Claw configuration driver
This commit adds a configuration HID driver for the MSI Claw series of
handheld PCs. It provides support for switching the gamepad mode and changing
the behavior of the M-keys.
> Additionally, the MCU RAM address for writing configuration data has
> changed twice. Checks are done during probe to enumerate these variances.
The commit message states that checks are done during probe to enumerate MCU
RAM address variances.
Looking at claw_probe() and msi_probe(), there does not appear to be any code
that checks firmware versions or handles these variances.
Was this functionality dropped during refactoring, or should the commit message
be updated?
> diff --git a/drivers/hid/hid-msi.c b/drivers/hid/hid-msi.c
> new file mode 100644
> index 0000000000000..8915942af15e6
> --- /dev/null
> +++ b/drivers/hid/hid-msi.c
[ ... ]
> +static int claw_hw_output_report(struct hid_device *hdev, u8 index, u8 *data,
> + size_t len, unsigned int timeout)
> +{
> + unsigned char *dmabuf __free(kfree) = NULL;
> + u8 header[] = { CLAW_OUTPUT_REPORT_ID, 0, 0, 0x3c, index };
> + struct claw_drvdata *drvdata = hid_get_drvdata(hdev);
> + size_t header_size = ARRAY_SIZE(header);
> + int ret;
> +
> + if (header_size + len > CLAW_PACKET_SIZE)
> + return -EINVAL;
> +
> + /* We can't use a devm_alloc reusable buffer without side effects during suspend */
> + dmabuf = kzalloc(CLAW_PACKET_SIZE, GFP_KERNEL);
> + if (!dmabuf)
> + return -ENOMEM;
> +
> + memcpy(dmabuf, header, header_size);
> + if (data && len)
> + memcpy(dmabuf + header_size, data, len);
> +
> + /* Don't hold a mutex when timeout=0, those commands cause USB disconnect */
> + if (timeout) {
> + guard(mutex)(&drvdata->cfg_mutex);
> + reinit_completion(&drvdata->send_cmd_complete);
> + }
Does this drop the lock too early?
The guard() macro releases the lock at the end of its enclosing scope.
Because it is declared inside the if (timeout) block, the mutex will be
released immediately at the closing brace, leaving hid_hw_output_report() and
the subsequent wait unprotected against concurrent access.
> +
> + dev_dbg(&hdev->dev, "Send data as raw output report: [%*ph]\n",
> + CLAW_PACKET_SIZE, dmabuf);
> +
> + ret = hid_hw_output_report(hdev, dmabuf, CLAW_PACKET_SIZE);
> + if (ret < 0)
> + return ret;
> +
> + ret = ret == CLAW_PACKET_SIZE ? 0 : -EIO;
> + if (ret)
> + return ret;
> +
> + if (timeout) {
> + ret = wait_for_completion_interruptible_timeout(&drvdata->send_cmd_complete,
> + msecs_to_jiffies(timeout));
Could this timeout evaluate to 1 jiffy on systems configured with HZ=100?
Several callers, such as gamepad_mode_show(), pass an 8ms timeout to this
function. A 1-jiffy timeout can expire almost instantaneously if the operation
begins near the end of the current tick boundary, potentially leading to
spurious -EBUSY timeouts.
[ ... ]
> +static void cfg_resume_fn(struct work_struct *work)
> +{
> + struct delayed_work *dwork = container_of(work, struct delayed_work, work);
> + struct claw_drvdata *drvdata = container_of(dwork, struct claw_drvdata, cfg_resume);
> + u8 data[2] = { drvdata->gamepad_mode, drvdata->mkeys_function };
If the system suspends before cfg_setup_fn() executes, or if the device read in
cfg_setup_fn() fails, could drvdata->gamepad_mode still be 0 here?
Since 0 is not a valid mode (valid modes like CLAW_GAMEPAD_MODE_XINPUT start
at 1), will this send an uninitialized configuration state to the hardware?
> + int ret;
> +
> + ret = claw_hw_output_report(drvdata->hdev, CLAW_COMMAND_TYPE_SWITCH_MODE, data,
> + ARRAY_SIZE(data), 0);
> + if (ret)
> + dev_err(&drvdata->hdev->dev, "Failed to set gamepad mode settings: %d\n", ret);
> +}
> +
> +static int claw_probe(struct hid_device *hdev, u8 ep)
> +{
> + struct claw_drvdata *drvdata;
> + int ret;
> +
> + drvdata = devm_kzalloc(&hdev->dev, sizeof(*drvdata), GFP_KERNEL);
> + if (!drvdata)
> + return -ENOMEM;
> +
> + hid_set_drvdata(hdev, drvdata);
> + drvdata->hdev = hdev;
> + drvdata->ep = ep;
> +
> + mutex_init(&drvdata->cfg_mutex);
> + init_completion(&drvdata->send_cmd_complete);
Is it safe to publish drvdata via hid_set_drvdata() before initializing the
mutex and completion object?
Since hid_hw_start() was already called earlier in msi_probe(), the hardware
is active. If an input report arrives right after hid_set_drvdata(), the
interrupt handler msi_raw_event() will process it and call complete() on an
uninitialized waitqueue.
> +
> + /* For control interface: open the HID transport for sending commands. */
> + ret = hid_hw_open(hdev);
> + if (ret)
> + return ret;
> +
> + INIT_DELAYED_WORK(&drvdata->cfg_resume, &cfg_resume_fn);
> + INIT_DELAYED_WORK(&drvdata->cfg_setup, &cfg_setup_fn);
> + schedule_delayed_work(&drvdata->cfg_setup, msecs_to_jiffies(500));
> +
> + return 0;
> +}
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260513231445.3213501-1-derekjohn.clark@gmail.com?part=1
^ permalink raw reply
* Re: [PATCH 7/7] hid-asus: add prod-id, quirk for Zenbook Duo keyboard
From: sashiko-bot @ 2026-05-14 12:03 UTC (permalink / raw)
To: Paolo Pisati; +Cc: dmitry.torokhov, linux-input
In-Reply-To: <20260513163248.16483-8-p.pisati@gmail.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] Out-of-bounds read and heap buffer overflow in report descriptor fixup logic.
--
commit ad6b37250b171e419ae8bd990515ef1b0181a049
Author: Luke Jones <luke@ljones.dev>
hid-asus: add prod-id, quirk for Zenbook Duo keyboard
This commit adds product IDs and Bluetooth IDs for the 2024/2025/2026 ASUS
Zenbook Duo keyboards. It enables keyboard control, backlight, and Fn-lock
by applying the appropriate quirks.
> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index 37e3aec612b6d..ba093f11f802f 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c
> @@ -1583,6 +1583,24 @@ static const struct hid_device_id asus_devices[] = {
> { HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
> USB_DEVICE_ID_ASUSTEK_ROG_Z13_LIGHTBAR),
> QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
> + { HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
> + USB_DEVICE_ID_ASUSTEK_ZENBOOK_DUO_UX8406MA_KEYBOARD),
> + QUIRK_USE_KBD_BACKLIGHT | QUIRK_HID_FN_LOCK | QUIRK_ZENBOOK_DUO_KEYBOARD },
This isn't a bug introduced in this patch, but adding these new device IDs with
QUIRK_ZENBOOK_DUO_KEYBOARD exposes them to an existing out-of-bounds read and
heap buffer overflow in asus_report_fixup().
When a device with this quirk connects, the driver attempts to strip trailing
zeros from its report descriptor:
drivers/hid/hid-asus.c:asus_report_fixup() {
...
if (drvdata->quirks & (QUIRK_T100CHI | QUIRK_T90CHI | QUIRK_ZENBOOK_DUO_KEYBOARD)) {
...
if (drvdata->quirks & QUIRK_ZENBOOK_DUO_KEYBOARD) {
if (hid_is_usb(hdev)) {
rsize_orig = 90;
offs = 66;
...
}
if (*rsize == rsize_orig &&
rdesc[offs] == 0x09 && rdesc[offs + 1] == 0x76) {
__u8 *new_rdesc;
unsigned int new_rsize = rsize_orig;
/* Clear bogus trailing zero bytes. */
while (rdesc[new_rsize + 1] == 0)
--new_rsize;
...
}
Does this evaluate rdesc[new_rsize + 1] reading 2 bytes past the end of the
dynamically allocated descriptor buffer, since new_rsize is initialized to the
exact size of the descriptor?
Furthermore, if the out-of-bounds bytes are non-zero, new_rsize remains its
original value. The code then copies rsize_orig + 2 bytes from the
rsize_orig-sized buffer:
drivers/hid/hid-asus.c:asus_report_fixup() {
...
new_rsize += 2;
new_rdesc = devm_kzalloc(&hdev->dev, new_rsize, GFP_KERNEL);
...
memcpy(new_rdesc, rdesc, new_rsize);
...
}
Can this result in an unconditional slab-out-of-bounds read?
Finally, if new_rsize shrank below offs + 14 due to trailing zeros, could the
subsequent memmove and assignments write past the end of the newly allocated
buffer, causing a heap buffer overflow?
drivers/hid/hid-asus.c:asus_report_fixup() {
...
memmove(rdesc + offs + 4, rdesc + offs + 2, 12);
...
rdesc[offs + 14] = 0x00;
...
}
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260513163248.16483-1-p.pisati@gmail.com?part=7
^ permalink raw reply
* Re: [PATCH v2 2/4] HID: hid-msi: Add M-key mapping attributes
From: sashiko-bot @ 2026-05-14 12:22 UTC (permalink / raw)
To: Derek J. Clark; +Cc: dmitry.torokhov, linux-input
In-Reply-To: <20260513231445.3213501-3-derekjohn.clark@gmail.com>
Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] The `cfg_mutex` guard in `claw_hw_output_report()` is scoped to an `if` block, releasing the lock before the critical section executes.
- [High] Lockless concurrent access to `profile_pending` between process and interrupt contexts causes data corruption and sequence mismatches on delayed responses.
- [High] Unsafe assumption of USB transport in `claw_probe()` causes type confusion and invalid pointer dereference for non-USB devices.
- [High] Returning an error from `claw_profile_event()` for a spurious event unconditionally triggers `complete()`, causing premature wakeups of unrelated commands.
- [Medium] The function `claw_buttons_store()` illegally mixes scope-based `guard()` cleanup with `goto`-based error handling.
--
commit 42be71c046dc9b6b31d28598f5e0bb568d78b7a1
Author: Derek J. Clark <derekjohn.clark@gmail.com>
HID: hid-msi: Add M-key mapping attributes
Adds attributes that allow for remapping the M-keys with up to 5 values
when in macro mode.
> diff --git a/drivers/hid/hid-msi.c b/drivers/hid/hid-msi.c
> index 8915942af15e6..13ba2747fdb66 100644
> --- a/drivers/hid/hid-msi.c
> +++ b/drivers/hid/hid-msi.c
[ ... ]
> @@ -125,6 +294,31 @@ static int claw_gamepad_mode_event(struct claw_drvdata *drvdata,
> return 0;
> }
>
> +static int claw_profile_event(struct claw_drvdata *drvdata, struct claw_command_report *cmd_rep)
> +{
> + u8 *codes;
> + int i;
> +
> + switch (drvdata->profile_pending) {
> + case CLAW_M1_PENDING:
> + case CLAW_M2_PENDING:
> + codes = (drvdata->profile_pending == CLAW_M1_PENDING) ?
> + drvdata->m1_codes : drvdata->m2_codes;
> + /* Extract key codes; replace disabled (0xff) with 0x00, which is (null) in _show */
> + for (i = 0; i < CLAW_KEYS_MAX; i++)
> + codes[i] = (cmd_rep->data[6 + i] != 0xff) ? cmd_rep->data[6 + i] : 0x00;
> + break;
Is it safe to read and modify profile_pending locklessly here in the
interrupt handler?
Since claw_buttons_show() modifies this state in process context under
profile_mutex, if a sysfs request times out and a subsequent request starts,
could the delayed ACK from the first request cause this handler to see the
new profile_pending state and corrupt the new request's buffer?
> + default:
> + dev_warn(&drvdata->hdev->dev,
> + "Got profile event without changes pending from command: %x\n",
> + cmd_rep->cmd);
> + return -EINVAL;
> + }
> + drvdata->profile_pending = CLAW_NO_PENDING;
> +
> + return 0;
> +}
> +
> static int claw_raw_event(struct claw_drvdata *drvdata, struct hid_report *report,
> u8 *data, int size)
> {
[ ... ]
> @@ -146,6 +340,9 @@ static int claw_raw_event(struct claw_drvdata *drvdata, struct hid_report *repor
> case CLAW_COMMAND_TYPE_GAMEPAD_MODE_ACK:
> ret = claw_gamepad_mode_event(drvdata, cmd_rep);
> break;
> + case CLAW_COMMAND_TYPE_READ_PROFILE_ACK:
> + ret = claw_profile_event(drvdata, cmd_rep);
> + break;
> case CLAW_COMMAND_TYPE_ACK:
> break;
> default:
When claw_profile_event() returns -EINVAL for a spurious event,
claw_raw_event() breaks from the switch and unconditionally calls
complete(&drvdata->send_cmd_complete).
Could this prematurely wake up a thread waiting on a completely different
command, such as WRITE_PROFILE_DATA?
[ ... ]
> @@ -366,6 +563,161 @@ static ssize_t reset_store(struct device *dev, struct device_attribute *attr,
> }
> static DEVICE_ATTR_WO(reset);
>
> +static int button_mapping_name_to_code(const char *name)
> +{
[ ... ]
> +static int claw_buttons_store(struct device *dev, const char *buf, u8 mkey_idx)
> +{
> + struct hid_device *hdev = to_hid_device(dev);
> + struct claw_drvdata *drvdata = hid_get_drvdata(hdev);
> + u8 data[] = { 0x01, (drvdata->bmap_addr[mkey_idx] >> 8) & 0xff,
> + drvdata->bmap_addr[mkey_idx] & 0xff, 0x07,
> + 0x04, 0x00, 0xff, 0xff, 0xff, 0xff, 0xff };
> + size_t len = ARRAY_SIZE(data);
> + int ret, key_count, i;
> + char **raw_keys;
> +
> + raw_keys = argv_split(GFP_KERNEL, buf, &key_count);
> + if (!raw_keys)
> + return -ENOMEM;
> +
> + guard(mutex)(&drvdata->rom_mutex); /* all err_free paths must be in scope */
> + if (key_count > CLAW_KEYS_MAX) {
> + ret = -EINVAL;
> + goto err_free;
> + }
> +
> + if (key_count == 0)
> + goto set_buttons;
> +
> + for (i = 0; i < key_count; i++) {
> + ret = button_mapping_name_to_code(raw_keys[i]);
> + if (ret < 0)
> + goto err_free;
> +
> + data[6 + i] = ret;
> + }
> +
> +set_buttons:
> + ret = claw_hw_output_report(hdev, CLAW_COMMAND_TYPE_WRITE_PROFILE_DATA, data, len, 8);
While reviewing the addition of these sysfs attributes, I noticed a
pre-existing issue in claw_hw_output_report() that is exacerbated by
the concurrent sysfs requests added here:
drivers/hid/hid-msi.c:claw_hw_output_report() {
...
if (timeout) {
guard(mutex)(&drvdata->cfg_mutex);
reinit_completion(&drvdata->send_cmd_complete);
}
...
}
Since guard() is scoped to the if block, the mutex is immediately released at
the closing brace.
Doesn't this mean hid_hw_output_report() and
wait_for_completion_interruptible_timeout() execute entirely without
the lock, defeating the intended serialization and allowing concurrent
requests to race?
> + if (ret < 0)
> + goto err_free;
> +
> + ret = claw_hw_output_report(hdev, CLAW_COMMAND_TYPE_SYNC_TO_ROM, NULL, 0, 8);
> +
> +err_free:
> + argv_free(raw_keys);
> + return ret;
> +}
Is it safe to mix goto-based error handling with scope-based guard() cleanup
in the same function?
According to the cleanup guidelines, functions should either convert
entirely to scope-based cleanup, for example by using a DEFINE_FREE
wrapper for argv_free, or not use guard() at all to avoid confusing
ownership semantics.
[ ... ]
> @@ -432,8 +796,31 @@ static void cfg_resume_fn(struct work_struct *work)
> dev_err(&drvdata->hdev->dev, "Failed to set gamepad mode settings: %d\n", ret);
> }
>
> +static void claw_features_supported(struct claw_drvdata *drvdata)
> +{
[ ... ]
> static int claw_probe(struct hid_device *hdev, u8 ep)
> {
> + struct usb_interface *intf = to_usb_interface(hdev->dev.parent);
> + struct usb_device *udev = interface_to_usbdev(intf);
Is it guaranteed that this device is always connected via USB?
If this driver is bound to a device connected over a non-USB transport like
I2C, Bluetooth, or UHID, hdev->dev.parent would not be a valid USB
interface, which could cause an invalid pointer dereference here.
Could this code check hid_is_usb(hdev) first or read the version from
hdev->version instead?
> struct claw_drvdata *drvdata;
> int ret;
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260513231445.3213501-1-derekjohn.clark@gmail.com?part=2
^ permalink raw reply
* Re: [PATCH v2 3/4] HID: hid-msi: Add RGB control interface
From: sashiko-bot @ 2026-05-14 12:53 UTC (permalink / raw)
To: Derek J. Clark; +Cc: linux-input, dmitry.torokhov
In-Reply-To: <20260513231445.3213501-4-derekjohn.clark@gmail.com>
Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [Critical] The `speed_show` function incorrectly casts the LED class device to a HID device, resulting in an invalid pointer dereference and a guaranteed kernel panic.
- [Critical] Missing bounds check on `frame->frame_count` from the USB device leads to an out-of-bounds read of kernel memory.
- [High] Re-arming `rgb_queue` from sysfs attributes after it has been explicitly cancelled in `claw_remove` leads to a Use-After-Free.
- [High] The `guard(mutex)` macro in `claw_hw_output_report()` is incorrectly used inside a block scope, providing absolutely no synchronization.
- [High] Concurrent modification of shared RGB state without lock protection causes data races and memory corruption.
- [High] Every RGB state change triggers a `SYNC_TO_ROM` command, causing severe flash memory degradation.
- [Medium] An early return in `cfg_setup_fn` silently drops core gamepad sysfs attributes if RGB initialization fails.
--
commit a90ff4a0f03b6df7827bfbcd222f977138b9d280
Author: Derek J. Clark <derekjohn.clark@gmail.com>
HID: hid-msi: Add RGB control interface
Adds RGB control interface for MSI Claw devices. The MSI Claw uses a
fairly unique RGB interface. It has 9 total zones (4 per joystick ring
and 1 for the ABXY buttons), and supports up to 8 sequential frames of
RGB zone data.
> --- a/drivers/hid/hid-msi.c
> +++ b/drivers/hid/hid-msi.c
[ ... ]
> @@ -308,15 +368,52 @@ static int claw_profile_event(struct claw_drvdata *drvdata, struct claw_command_
> for (i = 0; i < CLAW_KEYS_MAX; i++)
> codes[i] = (cmd_rep->data[6 + i] != 0xff) ? cmd_rep->data[6 + i] : 0x00;
> break;
> + case CLAW_RGB_PENDING:
> + frame = (struct rgb_report *)cmd_rep->data;
> + rgb_addr = drvdata->rgb_addr;
> + read_addr = be16_to_cpu(frame->read_addr);
> + frame_calc = (read_addr - rgb_addr) / CLAW_RGB_FRAME_OFFSET;
> + if (frame_calc > U8_MAX) {
> + dev_err(drvdata->led_mc.led_cdev.dev, "Got unsupported frame index: %x\n",
> + frame_calc);
> + ret = -EINVAL;
> + goto err_pending;
> + }
> + f_idx = frame_calc;
> +
> + if (f_idx >= CLAW_RGB_MAX_FRAMES) {
> + dev_err(drvdata->led_mc.led_cdev.dev, "Got illegal frame index: %x\n",
> + f_idx);
> + ret = -EINVAL;
> + goto err_pending;
> + }
> +
> + /* Always treat the first frame as the truth for these constants */
> + if (f_idx == 0) {
> + drvdata->rgb_frame_count = frame->frame_count;
drvdata->rgb_frame_count is assigned directly from the hardware packet.
Should there be a bounds check here to ensure it is less than or equal to
CLAW_RGB_MAX_FRAMES?
Later, claw_write_rgb_state() uses drvdata->rgb_frame_count as the upper
bound for a loop reading from drvdata->rgb_frames. If a device sends a
frame count greater than 8, could this read out-of-bounds kernel memory
and send it back to the USB device?
> + /* Invert device speed (20-0) to sysfs speed (0-20) */
> + drvdata->rgb_speed = frame->speed;
> + drvdata->led_mc.led_cdev.brightness = frame->brightness;
> + drvdata->led_mc.subled_info[0].intensity = frame->zone_data.zone[0].red;
> + drvdata->led_mc.subled_info[1].intensity = frame->zone_data.zone[0].green;
> + drvdata->led_mc.subled_info[2].intensity = frame->zone_data.zone[0].blue;
> + }
> +
> + memcpy(&drvdata->rgb_frames[f_idx], &frame->zone_data,
> + sizeof(struct rgb_frame));
claw_profile_event() modifies drvdata->rgb_frames and
drvdata->rgb_frame_count from the HID raw event handler context.
Simultaneously, claw_rgb_queue_fn() runs in a workqueue and calls
functions that overwrite these exact same fields.
Since there are no locks protecting these updates, could a background
worker read torn state or corrupt the frame arrays while they are being
actively updated by the asynchronous device response?
[ ... ]
> @@ -759,6 +856,397 @@ static const struct attribute_group claw_gamepad_attr_group = {
> .is_visible = claw_gamepad_attr_is_visible,
> };
>
> +/* Read RGB config from device */
> +static int claw_read_rgb_config(struct hid_device *hdev)
> +{
[ ... ]
> +/* Send RGB configuration to device */
> +static int claw_write_rgb_state(struct claw_drvdata *drvdata)
> +{
> + struct rgb_report report = { 0x01, 0x0000, CLAW_RGB_FRAME_OFFSET, 0x00,
> + drvdata->rgb_frame_count, 0x09, drvdata->rgb_speed,
> + drvdata->led_mc.led_cdev.brightness };
> + u16 write_addr = drvdata->rgb_addr;
> + size_t len = sizeof(report);
> + int f, ret;
> +
> + if (!drvdata->rgb_addr)
> + return -ENODEV;
> +
> + if (!drvdata->rgb_frame_count)
> + return -EINVAL;
> +
> + guard(mutex)(&drvdata->rom_mutex);
> + /* Loop through (up to) 8 pages of RGB data */
> + for (f = 0; f < drvdata->rgb_frame_count; f++) {
> + report.zone_data = drvdata->rgb_frames[f];
> +
> + /* Set the MCU address to write the frame data to */
> + report.read_addr = cpu_to_be16(write_addr);
> +
> + /* Serialize the rgb_report and write it to MCU */
> + ret = claw_hw_output_report(drvdata->hdev, CLAW_COMMAND_TYPE_WRITE_PROFILE_DATA,
> + (u8 *)&report, len, 8);
> + if (ret)
> + return ret;
> +
> + /* Increment the write addr by the offset for the next frame */
> + write_addr += CLAW_RGB_FRAME_OFFSET;
> + }
> +
> + ret = claw_hw_output_report(drvdata->hdev, CLAW_COMMAND_TYPE_SYNC_TO_ROM, NULL, 0, 8);
Does sending CLAW_COMMAND_TYPE_SYNC_TO_ROM on every RGB state change
cause excessive flash memory degradation?
Because claw_apply_rgb_state() is triggered by unthrottled sysfs writes
(such as rapidly changing a brightness slider), this might cause hundreds
of NVRAM writes per second and potentially degrade the MCU's flash
endurance.
[ ... ]
> +static ssize_t effect_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct led_classdev *led_cdev = dev_get_drvdata(dev);
> + struct led_classdev_mc *led_mc = container_of(led_cdev, struct led_classdev_mc, led_cdev);
> + struct claw_drvdata *drvdata = container_of(led_mc, struct claw_drvdata, led_mc);
> + int ret;
> +
> + ret = sysfs_match_string(claw_rgb_effect_text, buf);
> + if (ret < 0)
> + return ret;
> +
> + drvdata->rgb_effect = ret;
> + mod_delayed_work(system_wq, &drvdata->rgb_queue, msecs_to_jiffies(50));
> +
> + return count;
> +}
Is there a potential use-after-free here if sysfs attributes are written
after the device is removed?
In claw_remove(), the driver attempts to tear down the worker using
cancel_delayed_work_sync(&drvdata->rgb_queue). However, the sysfs files
and LED endpoints remain active because devm cleanup executes after
claw_remove() completes.
A concurrent write to a sysfs attribute like effect_store could call
mod_delayed_work() and re-arm the work item, causing it to execute after
drvdata has been freed.
[ ... ]
> +static ssize_t speed_show(struct device *dev, struct device_attribute *attr,
> + char *buf)
> +{
> + struct hid_device *hdev = to_hid_device(dev);
> + struct claw_drvdata *drvdata = hid_get_drvdata(hdev);
Is the dev parameter passed to speed_show actually a hid_device?
The rgb_attr_group sysfs group is attached to the LED class device
(led_cdev.dev), not the parent HID device. The macro to_hid_device(dev)
blindly performs a container_of cast, which will yield an invalid pointer
for hdev.
Dereferencing it with hid_get_drvdata(hdev) will likely cause a kernel
panic when this world-readable attribute is accessed.
[ ... ]
> @@ -772,6 +1260,13 @@ static void cfg_setup_fn(struct work_struct *work)
> return;
> }
>
> + ret = claw_read_rgb_config(drvdata->hdev);
> + if (ret) {
> + dev_err(drvdata->led_mc.led_cdev.dev,
> + "Failed to setup device, can't read RGB config: %d\n", ret);
> + return;
> + }
> +
> /* Add sysfs attributes after we get the device state */
> ret = devm_device_add_group(&drvdata->hdev->dev, &claw_gamepad_attr_group);
If claw_read_rgb_config() fails due to a temporary USB error or
unsupported firmware, this early return skips the subsequent
devm_device_add_group call.
Does this unintentionally break the device's core gamepad configuration
interface simply because the RGB initialization failed?
This isn't a new bug introduced by this patch, but does the guard() in
claw_hw_output_report() provide any synchronization?
claw_hw_output_report() {
if (timeout) {
guard(mutex)(&drvdata->cfg_mutex);
reinit_completion(&drvdata->send_cmd_complete);
}
...
}
Because the guard is declared inside the block, the mutex is immediately
released at the closing brace before hid_hw_output_report() and
wait_for_completion_interruptible_timeout() are ever called. This seems
to leave the I/O operations unprotected against concurrent threads, which
this patch introduces via the new RGB workers.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260513231445.3213501-1-derekjohn.clark@gmail.com?part=3
^ permalink raw reply
* Re: [PATCH] HID: lenovo: Restore Fn-lock LED state on resume for tp10ubkbd devices
From: Kean @ 2026-05-14 12:54 UTC (permalink / raw)
To: sashiko-bot, derekjohn.clark
Cc: linux-input, dmitry.torokhov, mpearson-lenovo, jikos, bentiss,
linux-kernel, Kean
In-Reply-To: <20260513222748.D7A5FC19425@smtp.kernel.org>
Hi,
Thank you for the review.
Regarding the potential issue you raised:
> [High] Unprotected state modifications / TOCTOU Race leading to Deadlock
>
> Could this new call to lenovo_led_set_tp10ubkbd() cause a deadlock during
> runtime resume?
In my initial testing, I performed system suspend/resume (S3) cycles and the
behavior was normal — the Fn-lock LED state was correctly restored after
resume with no hangs or lockups observed. I tested two scenarios:
1. Normal suspend/resume: suspend the system with the keyboard attached,
then resume normally.
2. Detach-reconnect: suspend the system, physically detach the keyboard
during suspend, resume the system, then re-attach the keyboard.
Both scenarios appeared to work correctly without issues.
However, I realize my testing only covered system suspend/resume (S3), not
runtime PM resume (which may hold different locks in the HID core). Your
analysis about a potential deadlock is reasonable and worth investigating
thoroughly. I need to:
- Test runtime resume scenarios specifically (e.g., autosuspend of the USB
HID device).
- Verify there is no lock ordering conflict between led_report_mutex (held
inside lenovo_led_set_tp10ubkbd) and any HID core locks that may already
be held when reset_resume is called.
I will run additional tests covering these cases. If I can reproduce any
issue, I will submit a V2 with the appropriate fix. If not, I will follow
up with a detailed description of my testing methodology and conclusions.
Thanks again for your guidance.
Regards,
Kean
^ permalink raw reply
* [PATCH] HID: lenovo: Fix buffer over-read and unaligned access in X12 Tab raw_event handler
From: Kean @ 2026-05-14 12:58 UTC (permalink / raw)
To: sashiko-bot, derekjohn.clark
Cc: linux-input, mpearson-lenovo, jikos, bentiss, linux-kernel, Kean
In-Reply-To: <20260512044911.99B6DC2BCB0@smtp.kernel.org>
In lenovo_raw_event(), the X12 Tab keyboard handler reads a 4-byte
little-endian value from the raw HID report buffer but:
1. The size guard is size >= 3, while the access reads 4 bytes.
A malformed 3-byte report with ID 0x03 would over-read the
buffer by one byte.
2. Casting u8 *data directly to __le32 * can trigger unaligned
access faults on architectures like ARM, MIPS, and SPARC,
because HID input buffers carry no alignment guarantee.
(e.g. uhid payloads start at offset 6 in struct uhid_event,
giving only 2-byte alignment.)
Fix both by tightening the size check to >= 4 and replacing the
open-coded cast + le32_to_cpu() with get_unaligned_le32(), which
handles the LE-to-CPU conversion safely regardless of alignment.
Link: https://sashiko.dev/#/message/20260512044911.99B6DC2BCB0%40smtp.kernel.org
Assisted-by: CLAUDE:claude-4-sonnet
Signed-off-by: Kean <rh_king@163.com>
---
drivers/hid/hid-lenovo.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/hid/hid-lenovo.c b/drivers/hid/hid-lenovo.c
index a6b73e03c16b..c11957ae8b77 100644
--- a/drivers/hid/hid-lenovo.c
+++ b/drivers/hid/hid-lenovo.c
@@ -30,6 +30,7 @@
#include <linux/hid.h>
#include <linux/input.h>
#include <linux/leds.h>
+#include <linux/unaligned.h>
#include <linux/workqueue.h>
#include "hid-ids.h"
@@ -793,8 +794,8 @@ static int lenovo_raw_event(struct hid_device *hdev,
*/
if (unlikely((hdev->product == USB_DEVICE_ID_LENOVO_X12_TAB
|| hdev->product == USB_DEVICE_ID_LENOVO_X12_TAB2)
- && size >= 3 && report->id == 0x03))
- return lenovo_raw_event_TP_X12_tab(hdev, le32_to_cpu(*(__le32 *)data));
+ && size >= 4 && report->id == 0x03))
+ return lenovo_raw_event_TP_X12_tab(hdev, get_unaligned_le32(data));
return 0;
}
--
2.53.0
^ permalink raw reply related
* Re: [PATCH] input/evdev: move kill_fasync() outside buffer_lock to fix SOFTIRQ deadlock
From: Rik van Riel @ 2026-05-14 13:06 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input, linux-kernel, kernel-team
In-Reply-To: <agTXYr_fFGiJ7lAe@google.com>
On Wed, 13 May 2026 13:01:58 -0700
Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> On Wed, May 13, 2026 at 11:50:00AM -0400, Rik van Riel wrote:
> > buffer_lock is a SOFTIRQ-safe spinlock. kill_fasync() acquires fa_lock
> > (SOFTIRQ-unsafe), creating a potential SOFTIRQ-safe->SOFTIRQ-unsafe lock
> > ordering violation that lockdep flags as a deadlock.
> >
> > Fix by moving the kill_fasync() call to evdev_pass_values() after
> > buffer_lock is released, alongside the existing wake_up_interruptible_poll().
> >
> > The wakeup condition check is the same in __pass_event() and
> > evdev_pass_values()
>
> Does this really fix anything? This code is running holding
> input->event_lock with IRQs off...
You're right. The bug is real, but this patch does not fix it.
Would the Sashiko suggestion work, or is there a better way to
tackle it?
For reference, the lockdep splat is below:
=====================================================
WARNING: SOFTIRQ-safe -> SOFTIRQ-unsafe lock order detected
7.0.0-rc6-00259-g427a4f9708ee #82 Not tainted
-----------------------------------------------------
syz.6.15929/30382 [HC0[0]:SC0[0]:HE0:SE1] is trying to acquire:
ffff888101324558 (&new->fa_lock){...-}-{3:3}, at: kill_fasync_rcu fs/fcntl.c:1135 [inline]
ffff888101324558 (&new->fa_lock){...-}-{3:3}, at: kill_fasync fs/fcntl.c:1159 [inline]
ffff888101324558 (&new->fa_lock){...-}-{3:3}, at: kill_fasync+0x137/0x590 fs/fcntl.c:1152
and this task is already holding:
ffff88812f3d8028 (&client->buffer_lock){....}-{3:3}, at: spin_lock include/linux/spinlock.h:341 [inline]
ffff88812f3d8028 (&client->buffer_lock){....}-{3:3}, at: evdev_pass_values.part.0+0xf6/0x950 drivers/input/evdev.c:261
which would create a new lock dependency:
(&client->buffer_lock){....}-{3:3} -> (&new->fa_lock){...-}-{3:3}
but this new dependency connects a SOFTIRQ-irq-safe lock:
(&dev->event_lock){..-.}-{3:3}
... which became SOFTIRQ-irq-safe at:
lock_acquire kernel/locking/lockdep.c:5868 [inline]
lock_acquire+0x18c/0x300 kernel/locking/lockdep.c:5825
__raw_spin_lock_irqsave include/linux/spinlock_api_smp.h:132 [inline]
_raw_spin_lock_irqsave+0x3a/0x60 kernel/locking/spinlock.c:162
class_spinlock_irqsave_constructor include/linux/spinlock.h:618 [inline]
input_inject_event+0x9f/0x420 drivers/input/input.c:419
__led_set_brightness drivers/leds/led-core.c:52 [inline]
led_set_brightness_nopm drivers/leds/led-core.c:335 [inline]
led_set_brightness_nosleep drivers/leds/led-core.c:369 [inline]
led_set_brightness+0x217/0x290 drivers/leds/led-core.c:328
led_trigger_event drivers/leds/led-triggers.c:420 [inline]
led_trigger_event+0xda/0x2b0 drivers/leds/led-triggers.c:408
kbd_propagate_led_state drivers/tty/vt/keyboard.c:1065 [inline]
kbd_bh+0x263/0x350 drivers/tty/vt/keyboard.c:1244
tasklet_action_common+0x240/0x3c0 kernel/softirq.c:925
handle_softirqs+0x1b0/0x8d0 kernel/softirq.c:622
__do_softirq kernel/softirq.c:656 [inline]
invoke_softirq kernel/softirq.c:496 [inline]
__irq_exit_rcu+0xc4/0x100 kernel/softirq.c:723
irq_exit_rcu+0x9/0x20 kernel/softirq.c:739
instr_sysvec_apic_timer_interrupt arch/x86/kernel/apic/apic.c:1056 [inline]
sysvec_apic_timer_interrupt+0x70/0x80 arch/x86/kernel/apic/apic.c:1056
asm_sysvec_apic_timer_interrupt+0x1a/0x20 arch/x86/include/asm/idtentry.h:729
__preempt_count_dec_and_test arch/x86/include/asm/preempt.h:95 [inline]
__raw_spin_unlock_irqrestore include/linux/spinlock_api_smp.h:179 [inline]
_raw_spin_unlock_irqrestore+0x34/0x50 kernel/locking/spinlock.c:194
spin_unlock_irqrestore include/linux/spinlock.h:407 [inline]
class_spinlock_irqsave_destructor include/linux/spinlock.h:618 [inline]
input_inject_event+0x1bd/0x420 drivers/input/input.c:419
evdev_write+0x30a/0x460 drivers/input/evdev.c:528
vfs_write+0x2b1/0x11a0 fs/read_write.c:686
ksys_write+0x1ef/0x240 fs/read_write.c:740
do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline]
do_syscall_64+0x111/0x680 arch/x86/entry/syscall_64.c:94
entry_SYSCALL_64_after_hwframe+0x77/0x7f
to a SOFTIRQ-irq-unsafe lock:
(tasklist_lock){.+.+}-{3:3}
... which became SOFTIRQ-irq-unsafe at:
...
lock_acquire kernel/locking/lockdep.c:5868 [inline]
lock_acquire+0x18c/0x300 kernel/locking/lockdep.c:5825
__raw_read_lock include/linux/rwlock_api_smp.h:161 [inline]
_raw_read_lock+0x5c/0x70 kernel/locking/spinlock.c:228
__do_wait+0x105/0x880 kernel/exit.c:1678
do_wait+0x1cb/0x5a0 kernel/exit.c:1722
kernel_wait+0x9f/0x160 kernel/exit.c:1898
call_usermodehelper_exec_sync kernel/umh.c:136 [inline]
call_usermodehelper_exec_work+0xf9/0x180 kernel/umh.c:163
process_one_work+0x920/0x1ac0 kernel/workqueue.c:3276
process_scheduled_works kernel/workqueue.c:3359 [inline]
worker_thread+0x693/0xeb0 kernel/workqueue.c:3440
kthread+0x385/0x490 kernel/kthread.c:436
ret_from_fork+0x67a/0xab0 arch/x86/kernel/process.c:158
ret_from_fork_asm+0x1a/0x30 arch/x86/entry/entry_64.S:245
other info that might help us debug this:
Chain exists of:
&dev->event_lock --> &client->buffer_lock --> tasklist_lock
Possible interrupt unsafe locking scenario:
CPU0 CPU1
---- ----
lock(tasklist_lock);
local_irq_disable();
lock(&dev->event_lock);
lock(&client->buffer_lock);
<Interrupt>
lock(&dev->event_lock);
*** DEADLOCK ***
7 locks held by syz.6.15929/30382:
#0: ffff88810452a118 (&evdev->mutex){+.+.}-{4:4}, at: evdev_write+0x161/0x460 drivers/input/evdev.c:511
#1: ffff8881038c5230 (&dev->event_lock){..-.}-{3:3}, at: class_spinlock_irqsave_constructor include/linux/spinlock.h:618 [inline]
#1: ffff8881038c5230 (&dev->event_lock){..-.}-{3:3}, at: input_inject_event+0x9f/0x420 drivers/input/input.c:419
#2: ffffffffbbcac800 (rcu_read_lock){....}-{1:3}, at: rcu_lock_acquire include/linux/rcupdate.h:312 [inline]
#2: ffffffffbbcac800 (rcu_read_lock){....}-{1:3}, at: rcu_read_lock include/linux/rcupdate.h:850 [inline]
#2: ffffffffbbcac800 (rcu_read_lock){....}-{1:3}, at: class_rcu_constructor include/linux/rcupdate.h:1193 [inline]
#2: ffffffffbbcac800 (rcu_read_lock){....}-{1:3}, at: input_inject_event+0xbb/0x420 drivers/input/input.c:420
#3: ffffffffbbcac800 (rcu_read_lock){....}-{1:3}, at: rcu_lock_acquire include/linux/rcupdate.h:312 [inline]
#3: ffffffffbbcac800 (rcu_read_lock){....}-{1:3}, at: rcu_read_lock include/linux/rcupdate.h:850 [inline]
#3: ffffffffbbcac800 (rcu_read_lock){....}-{1:3}, at: class_rcu_constructor include/linux/rcupdate.h:1193 [inline]
#3: ffffffffbbcac800 (rcu_read_lock){....}-{1:3}, at: input_pass_values+0x80/0x8b0 drivers/input/input.c:119
#4: ffffffffbbcac800 (rcu_read_lock){....}-{1:3}, at: rcu_lock_acquire include/linux/rcupdate.h:312 [inline]
#4: ffffffffbbcac800 (rcu_read_lock){....}-{1:3}, at: rcu_read_lock include/linux/rcupdate.h:850 [inline]
#4: ffffffffbbcac800 (rcu_read_lock){....}-{1:3}, at: evdev_events+0x80/0x4e0 drivers/input/evdev.c:298
#5: ffff88812f3d8028 (&client->buffer_lock){....}-{3:3}, at: spin_lock include/linux/spinlock.h:341 [inline]
#5: ffff88812f3d8028 (&client->buffer_lock){....}-{3:3}, at: evdev_pass_values.part.0+0xf6/0x950 drivers/input/evdev.c:261
#6: ffffffffbbcac800 (rcu_read_lock){....}-{1:3}, at: rcu_lock_acquire include/linux/rcupdate.h:312 [inline]
#6: ffffffffbbcac800 (rcu_read_lock){....}-{1:3}, at: rcu_read_lock include/linux/rcupdate.h:850 [inline]
#6: ffffffffbbcac800 (rcu_read_lock){....}-{1:3}, at: kill_fasync fs/fcntl.c:1158 [inline]
#6: ffffffffbbcac800 (rcu_read_lock){....}-{1:3}, at: kill_fasync+0x61/0x590 fs/fcntl.c:1152
the dependencies between SOFTIRQ-irq-safe lock and the holding lock:
-> (&dev->event_lock){..-.}-{3:3} {
IN-SOFTIRQ-W at:
lock_acquire kernel/locking/lockdep.c:5868 [inline]
lock_acquire+0x18c/0x300 kernel/locking/lockdep.c:5825
__raw_spin_lock_irqsave include/linux/spinlock_api_smp.h:132 [inline]
_raw_spin_lock_irqsave+0x3a/0x60 kernel/locking/spinlock.c:162
class_spinlock_irqsave_constructor include/linux/spinlock.h:618 [inline]
input_inject_event+0x9f/0x420 drivers/input/input.c:419
__led_set_brightness drivers/leds/led-core.c:52 [inline]
led_set_brightness_nopm drivers/leds/led-core.c:335 [inline]
led_set_brightness_nosleep drivers/leds/led-core.c:369 [inline]
led_set_brightness+0x217/0x290 drivers/leds/led-core.c:328
led_trigger_event drivers/leds/led-triggers.c:420 [inline]
led_trigger_event+0xda/0x2b0 drivers/leds/led-triggers.c:408
kbd_propagate_led_state drivers/tty/vt/keyboard.c:1065 [inline]
kbd_bh+0x263/0x350 drivers/tty/vt/keyboard.c:1244
tasklet_action_common+0x240/0x3c0 kernel/softirq.c:925
handle_softirqs+0x1b0/0x8d0 kernel/softirq.c:622
__do_softirq kernel/softirq.c:656 [inline]
invoke_softirq kernel/softirq.c:496 [inline]
__irq_exit_rcu+0xc4/0x100 kernel/softirq.c:723
irq_exit_rcu+0x9/0x20 kernel/softirq.c:739
instr_sysvec_apic_timer_interrupt arch/x86/kernel/apic/apic.c:1056 [inline]
sysvec_apic_timer_interrupt+0x70/0x80 arch/x86/kernel/apic/apic.c:1056
asm_sysvec_apic_timer_interrupt+0x1a/0x20 arch/x86/include/asm/idtentry.h:729
__preempt_count_dec_and_test arch/x86/include/asm/preempt.h:95 [inline]
__raw_spin_unlock_irqrestore include/linux/spinlock_api_smp.h:179 [inline]
_raw_spin_unlock_irqrestore+0x34/0x50 kernel/locking/spinlock.c:194
spin_unlock_irqrestore include/linux/spinlock.h:407 [inline]
class_spinlock_irqsave_destructor include/linux/spinlock.h:618 [inline]
input_inject_event+0x1bd/0x420 drivers/input/input.c:419
evdev_write+0x30a/0x460 drivers/input/evdev.c:528
vfs_write+0x2b1/0x11a0 fs/read_write.c:686
ksys_write+0x1ef/0x240 fs/read_write.c:740
do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline]
do_syscall_64+0x111/0x680 arch/x86/entry/syscall_64.c:94
entry_SYSCALL_64_after_hwframe+0x77/0x7f
INITIAL USE at:
lock_acquire kernel/locking/lockdep.c:5868 [inline]
lock_acquire+0x18c/0x300 kernel/locking/lockdep.c:5825
__raw_spin_lock_irqsave include/linux/spinlock_api_smp.h:132 [inline]
_raw_spin_lock_irqsave+0x3a/0x60 kernel/locking/spinlock.c:162
class_spinlock_irqsave_constructor include/linux/spinlock.h:618 [inline]
input_inject_event+0x9f/0x420 drivers/input/input.c:419
__led_set_brightness drivers/leds/led-core.c:52 [inline]
led_set_brightness_nopm drivers/leds/led-core.c:335 [inline]
led_set_brightness_nosleep drivers/leds/led-core.c:369 [inline]
led_set_brightness+0x217/0x290 drivers/leds/led-core.c:328
kbd_led_trigger_activate+0xcd/0x110 drivers/tty/vt/keyboard.c:1021
led_trigger_set+0x4c9/0xaa0 drivers/leds/led-triggers.c:220
led_match_default_trigger drivers/leds/led-triggers.c:277 [inline]
led_match_default_trigger drivers/leds/led-triggers.c:271 [inline]
led_trigger_set_default drivers/leds/led-triggers.c:300 [inline]
led_trigger_set_default+0x1e7/0x2e0 drivers/leds/led-triggers.c:284
led_classdev_register_ext+0x63a/0x980 drivers/leds/led-class.c:578
led_classdev_register include/linux/leds.h:274 [inline]
input_leds_connect+0x4c5/0x900 drivers/input/input-leds.c:145
input_attach_handler+0x17b/0x260 drivers/input/input.c:994
input_register_device+0xa1e/0x1070 drivers/input/input.c:2378
atkbd_connect+0x6c2/0xb60 drivers/input/keyboard/atkbd.c:1340
serio_connect_driver drivers/input/serio/serio.c:44 [inline]
serio_driver_probe+0x84/0xe0 drivers/input/serio/serio.c:748
call_driver_probe drivers/base/dd.c:643 [inline]
really_probe+0x260/0x840 drivers/base/dd.c:721
__driver_probe_device+0x1e7/0x390 drivers/base/dd.c:863
driver_probe_device+0x4e/0x2e0 drivers/base/dd.c:893
__driver_attach drivers/base/dd.c:1287 [inline]
__driver_attach+0x1d6/0x5d0 drivers/base/dd.c:1227
bus_for_each_dev+0x12c/0x1c0 drivers/base/bus.c:383
serio_attach_driver drivers/input/serio/serio.c:777 [inline]
serio_handle_event+0x234/0x980 drivers/input/serio/serio.c:214
process_one_work+0x920/0x1ac0 kernel/workqueue.c:3276
process_scheduled_works kernel/workqueue.c:3359 [inline]
worker_thread+0x693/0xeb0 kernel/workqueue.c:3440
kthread+0x385/0x490 kernel/kthread.c:436
ret_from_fork+0x67a/0xab0 arch/x86/kernel/process.c:158
ret_from_fork_asm+0x1a/0x30 arch/x86/entry/entry_64.S:245
}
... key at: [<ffffffffbe892e60>] __key.4+0x0/0x40
-> (&client->buffer_lock){....}-{3:3} {
INITIAL USE at:
lock_acquire kernel/locking/lockdep.c:5868 [inline]
lock_acquire+0x18c/0x300 kernel/locking/lockdep.c:5825
__raw_spin_lock_irq include/linux/spinlock_api_smp.h:142 [inline]
_raw_spin_lock_irq+0x33/0x50 kernel/locking/spinlock.c:170
spin_lock_irq include/linux/spinlock.h:371 [inline]
evdev_fetch_next_event drivers/input/evdev.c:543 [inline]
evdev_read+0x4ee/0xc70 drivers/input/evdev.c:584
vfs_read+0x1e6/0xc70 fs/read_write.c:572
ksys_read+0x1ef/0x240 fs/read_write.c:717
do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline]
do_syscall_64+0x111/0x680 arch/x86/entry/syscall_64.c:94
entry_SYSCALL_64_after_hwframe+0x77/0x7f
}
... key at: [<ffffffffbe893060>] __key.84+0x0/0x40
... acquired at:
lock_acquire kernel/locking/lockdep.c:5868 [inline]
lock_acquire+0x18c/0x300 kernel/locking/lockdep.c:5825
__raw_spin_lock include/linux/spinlock_api_smp.h:158 [inline]
_raw_spin_lock+0x2b/0x40 kernel/locking/spinlock.c:154
spin_lock include/linux/spinlock.h:341 [inline]
evdev_handle_get_val+0x70/0x620 drivers/input/evdev.c:898
evdev_do_ioctl+0x908/0x1a80 drivers/input/evdev.c:1157
evdev_ioctl_handler drivers/input/evdev.c:1270 [inline]
evdev_ioctl+0x17e/0x1f0 drivers/input/evdev.c:1279
vfs_ioctl fs/ioctl.c:51 [inline]
__do_sys_ioctl fs/ioctl.c:597 [inline]
__se_sys_ioctl fs/ioctl.c:583 [inline]
__x64_sys_ioctl+0x18f/0x210 fs/ioctl.c:583
do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline]
do_syscall_64+0x111/0x680 arch/x86/entry/syscall_64.c:94
entry_SYSCALL_64_after_hwframe+0x77/0x7f
the dependencies between the lock to be acquired
and SOFTIRQ-irq-unsafe lock:
-> (tasklist_lock){.+.+}-{3:3} {
HARDIRQ-ON-R at:
lock_acquire kernel/locking/lockdep.c:5868 [inline]
lock_acquire+0x18c/0x300 kernel/locking/lockdep.c:5825
__raw_read_lock include/linux/rwlock_api_smp.h:161 [inline]
_raw_read_lock+0x5c/0x70 kernel/locking/spinlock.c:228
__do_wait+0x105/0x880 kernel/exit.c:1678
do_wait+0x1cb/0x5a0 kernel/exit.c:1722
kernel_wait+0x9f/0x160 kernel/exit.c:1898
call_usermodehelper_exec_sync kernel/umh.c:136 [inline]
call_usermodehelper_exec_work+0xf9/0x180 kernel/umh.c:163
process_one_work+0x920/0x1ac0 kernel/workqueue.c:3276
process_scheduled_works kernel/workqueue.c:3359 [inline]
worker_thread+0x693/0xeb0 kernel/workqueue.c:3440
kthread+0x385/0x490 kernel/kthread.c:436
ret_from_fork+0x67a/0xab0 arch/x86/kernel/process.c:158
ret_from_fork_asm+0x1a/0x30 arch/x86/entry/entry_64.S:245
SOFTIRQ-ON-R at:
lock_acquire kernel/locking/lockdep.c:5868 [inline]
lock_acquire+0x18c/0x300 kernel/locking/lockdep.c:5825
__raw_read_lock include/linux/rwlock_api_smp.h:161 [inline]
_raw_read_lock+0x5c/0x70 kernel/locking/spinlock.c:228
__do_wait+0x105/0x880 kernel/exit.c:1678
do_wait+0x1cb/0x5a0 kernel/exit.c:1722
kernel_wait+0x9f/0x160 kernel/exit.c:1898
call_usermodehelper_exec_sync kernel/umh.c:136 [inline]
call_usermodehelper_exec_work+0xf9/0x180 kernel/umh.c:163
process_one_work+0x920/0x1ac0 kernel/workqueue.c:3276
process_scheduled_works kernel/workqueue.c:3359 [inline]
worker_thread+0x693/0xeb0 kernel/workqueue.c:3440
kthread+0x385/0x490 kernel/kthread.c:436
ret_from_fork+0x67a/0xab0 arch/x86/kernel/process.c:158
ret_from_fork_asm+0x1a/0x30 arch/x86/entry/entry_64.S:245
INITIAL USE at:
lock_acquire kernel/locking/lockdep.c:5868 [inline]
lock_acquire+0x18c/0x300 kernel/locking/lockdep.c:5825
__raw_write_lock_irq include/linux/rwlock_api_smp.h:211 [inline]
_raw_write_lock_irq+0x33/0x50 kernel/locking/spinlock.c:326
copy_process+0x4547/0x7440 kernel/fork.c:2369
kernel_clone+0xea/0x830 kernel/fork.c:2653
user_mode_thread+0xc8/0x110 kernel/fork.c:2729
rest_init+0x25/0x320 init/main.c:725
start_kernel+0x400/0x530 init/main.c:1210
x86_64_start_reservations+0x18/0x30 arch/x86/kernel/head64.c:310
x86_64_start_kernel+0x112/0x130 arch/x86/kernel/head64.c:291
common_startup_64+0x13e/0x148
INITIAL READ USE at:
lock_acquire kernel/locking/lockdep.c:5868 [inline]
lock_acquire+0x18c/0x300 kernel/locking/lockdep.c:5825
__raw_read_lock include/linux/rwlock_api_smp.h:161 [inline]
_raw_read_lock+0x5c/0x70 kernel/locking/spinlock.c:228
__do_wait+0x105/0x880 kernel/exit.c:1678
do_wait+0x1cb/0x5a0 kernel/exit.c:1722
kernel_wait+0x9f/0x160 kernel/exit.c:1898
call_usermodehelper_exec_sync kernel/umh.c:136 [inline]
call_usermodehelper_exec_work+0xf9/0x180 kernel/umh.c:163
process_one_work+0x920/0x1ac0 kernel/workqueue.c:3276
process_scheduled_works kernel/workqueue.c:3359 [inline]
worker_thread+0x693/0xeb0 kernel/workqueue.c:3440
kthread+0x385/0x490 kernel/kthread.c:436
ret_from_fork+0x67a/0xab0 arch/x86/kernel/process.c:158
ret_from_fork_asm+0x1a/0x30 arch/x86/entry/entry_64.S:245
}
... key at: [<ffffffffbba0c098>] tasklist_lock+0x18/0x40
... acquired at:
lock_acquire kernel/locking/lockdep.c:5868 [inline]
lock_acquire+0x18c/0x300 kernel/locking/lockdep.c:5825
__raw_read_lock include/linux/rwlock_api_smp.h:161 [inline]
_raw_read_lock+0x5c/0x70 kernel/locking/spinlock.c:228
send_sigio+0xb8/0x420 fs/fcntl.c:932
kill_fasync_rcu fs/fcntl.c:1144 [inline]
kill_fasync fs/fcntl.c:1159 [inline]
kill_fasync+0x218/0x590 fs/fcntl.c:1152
sock_wake_async+0xd6/0x160 net/socket.c:1509
sk_wake_async_rcu include/net/sock.h:2579 [inline]
sk_wake_async_rcu include/net/sock.h:2576 [inline]
sock_def_readable+0x55f/0x660 net/core/sock.c:3613
__netlink_sendskb net/netlink/af_netlink.c:1263 [inline]
netlink_sendskb net/netlink/af_netlink.c:1269 [inline]
netlink_unicast+0x745/0x870 net/netlink/af_netlink.c:1359
nlmsg_unicast include/net/netlink.h:1198 [inline]
netlink_ack+0x6b6/0xb90 net/netlink/af_netlink.c:2512
netlink_rcv_skb+0x344/0x430 net/netlink/af_netlink.c:2556
nfnetlink_rcv+0x1af/0x420 net/netfilter/nfnetlink.c:669
netlink_unicast_kernel net/netlink/af_netlink.c:1318 [inline]
netlink_unicast+0x5a7/0x870 net/netlink/af_netlink.c:1344
netlink_sendmsg+0x8a3/0xda0 net/netlink/af_netlink.c:1894
sock_sendmsg_nosec net/socket.c:727 [inline]
__sock_sendmsg net/socket.c:742 [inline]
____sys_sendmsg+0x9c4/0xb30 net/socket.c:2592
___sys_sendmsg+0x11c/0x1b0 net/socket.c:2646
__sys_sendmsg+0x150/0x200 net/socket.c:2678
do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline]
do_syscall_64+0x111/0x680 arch/x86/entry/syscall_64.c:94
entry_SYSCALL_64_after_hwframe+0x77/0x7f
-> (&f_owner->lock){....}-{3:3} {
INITIAL USE at:
lock_acquire kernel/locking/lockdep.c:5868 [inline]
lock_acquire+0x18c/0x300 kernel/locking/lockdep.c:5825
__raw_write_lock_irq include/linux/rwlock_api_smp.h:211 [inline]
_raw_write_lock_irq+0x33/0x50 kernel/locking/spinlock.c:326
__f_setown+0x60/0x3c0 fs/fcntl.c:136
fcntl_dirnotify+0x623/0xb60 fs/notify/dnotify/dnotify.c:369
do_fcntl+0x235/0x1580 fs/fcntl.c:538
__do_sys_fcntl fs/fcntl.c:602 [inline]
__se_sys_fcntl fs/fcntl.c:587 [inline]
__x64_sys_fcntl+0x163/0x200 fs/fcntl.c:587
do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline]
do_syscall_64+0x111/0x680 arch/x86/entry/syscall_64.c:94
entry_SYSCALL_64_after_hwframe+0x77/0x7f
INITIAL READ USE at:
lock_acquire kernel/locking/lockdep.c:5868 [inline]
lock_acquire+0x18c/0x300 kernel/locking/lockdep.c:5825
__raw_read_lock_irqsave include/linux/rwlock_api_smp.h:172 [inline]
_raw_read_lock_irqsave+0x75/0x90 kernel/locking/spinlock.c:236
send_sigio+0x31/0x420 fs/fcntl.c:918
kill_fasync_rcu fs/fcntl.c:1144 [inline]
kill_fasync fs/fcntl.c:1159 [inline]
kill_fasync+0x218/0x590 fs/fcntl.c:1152
lease_break_callback+0x23/0x30 fs/locks.c:577
__break_lease+0x7e4/0x1b50 fs/locks.c:1657
break_lease include/linux/filelock.h:484 [inline]
break_lease include/linux/filelock.h:469 [inline]
vfs_truncate+0x3e1/0x4e0 fs/open.c:112
do_sys_truncate+0xd6/0x180 fs/open.c:142
__do_sys_truncate fs/open.c:154 [inline]
__se_sys_truncate fs/open.c:152 [inline]
__x64_sys_truncate+0x54/0x80 fs/open.c:152
do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline]
do_syscall_64+0x111/0x680 arch/x86/entry/syscall_64.c:94
entry_SYSCALL_64_after_hwframe+0x77/0x7f
}
... key at: [<ffffffffbe845aa0>] __key.1+0x0/0x40
... acquired at:
lock_acquire kernel/locking/lockdep.c:5868 [inline]
lock_acquire+0x18c/0x300 kernel/locking/lockdep.c:5825
__raw_read_lock_irqsave include/linux/rwlock_api_smp.h:172 [inline]
_raw_read_lock_irqsave+0x75/0x90 kernel/locking/spinlock.c:236
send_sigio+0x31/0x420 fs/fcntl.c:918
kill_fasync_rcu fs/fcntl.c:1144 [inline]
kill_fasync fs/fcntl.c:1159 [inline]
kill_fasync+0x218/0x590 fs/fcntl.c:1152
lease_break_callback+0x23/0x30 fs/locks.c:577
__break_lease+0x7e4/0x1b50 fs/locks.c:1657
break_lease include/linux/filelock.h:484 [inline]
break_lease include/linux/filelock.h:469 [inline]
vfs_truncate+0x3e1/0x4e0 fs/open.c:112
do_sys_truncate+0xd6/0x180 fs/open.c:142
__do_sys_truncate fs/open.c:154 [inline]
__se_sys_truncate fs/open.c:152 [inline]
__x64_sys_truncate+0x54/0x80 fs/open.c:152
do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline]
do_syscall_64+0x111/0x680 arch/x86/entry/syscall_64.c:94
entry_SYSCALL_64_after_hwframe+0x77/0x7f
-> (&new->fa_lock){...-}-{3:3} {
IN-SOFTIRQ-R at:
lock_acquire kernel/locking/lockdep.c:5868 [inline]
lock_acquire+0x18c/0x300 kernel/locking/lockdep.c:5825
__raw_read_lock_irqsave include/linux/rwlock_api_smp.h:172 [inline]
_raw_read_lock_irqsave+0x46/0x90 kernel/locking/spinlock.c:236
kill_fasync_rcu fs/fcntl.c:1135 [inline]
kill_fasync fs/fcntl.c:1159 [inline]
kill_fasync+0x137/0x590 fs/fcntl.c:1152
sock_wake_async+0xd6/0x160 net/socket.c:1509
sk_wake_async_rcu include/net/sock.h:2579 [inline]
sk_wake_async_rcu include/net/sock.h:2576 [inline]
sock_def_readable+0x55f/0x660 net/core/sock.c:3613
packet_rcv+0xec8/0x1740 net/packet/af_packet.c:2209
dev_queue_xmit_nit+0x713/0xb00 net/core/dev.c:2606
xmit_one net/core/dev.c:3884 [inline]
dev_hard_start_xmit+0x605/0x720 net/core/dev.c:3904
__dev_queue_xmit+0x1649/0x3f60 net/core/dev.c:4854
dev_queue_xmit include/linux/netdevice.h:3385 [inline]
neigh_hh_output include/net/neighbour.h:540 [inline]
neigh_output include/net/neighbour.h:554 [inline]
ip_finish_output2+0xb1c/0x1ce0 net/ipv4/ip_output.c:237
__ip_finish_output.part.0+0x1bb/0x350 net/ipv4/ip_output.c:315
__ip_finish_output net/ipv4/ip_output.c:303 [inline]
ip_finish_output net/ipv4/ip_output.c:325 [inline]
NF_HOOK_COND include/linux/netfilter.h:307 [inline]
ip_output+0x3a9/0xd00 net/ipv4/ip_output.c:438
dst_output include/net/dst.h:470 [inline]
ip_local_out+0x1b4/0x200 net/ipv4/ip_output.c:131
__ip_queue_xmit+0x899/0x1f40 net/ipv4/ip_output.c:534
__tcp_transmit_skb+0x2f93/0x4780 net/ipv4/tcp_output.c:1693
__tcp_send_ack.part.0+0x3ce/0x670 net/ipv4/tcp_output.c:4503
__tcp_send_ack net/ipv4/tcp_output.c:4509 [inline]
tcp_send_ack+0x83/0xa0 net/ipv4/tcp_output.c:4509
tcp_delack_timer_handler net/ipv4/tcp_timer.c:345 [inline]
tcp_delack_timer_handler+0x2b8/0x460 net/ipv4/tcp_timer.c:308
tcp_delack_timer+0x232/0x3c0 net/ipv4/tcp_timer.c:376
call_timer_fn+0x189/0x5c0 kernel/time/timer.c:1748
expire_timers kernel/time/timer.c:1799 [inline]
__run_timers+0x6cd/0xb00 kernel/time/timer.c:2373
__run_timer_base kernel/time/timer.c:2385 [inline]
__run_timer_base kernel/time/timer.c:2377 [inline]
run_timer_base kernel/time/timer.c:2394 [inline]
run_timer_softirq+0x117/0x210 kernel/time/timer.c:2404
handle_softirqs+0x1b0/0x8d0 kernel/softirq.c:622
__do_softirq kernel/softirq.c:656 [inline]
invoke_softirq kernel/softirq.c:496 [inline]
__irq_exit_rcu+0xc4/0x100 kernel/softirq.c:723
irq_exit_rcu+0x9/0x20 kernel/softirq.c:739
instr_sysvec_apic_timer_interrupt arch/x86/kernel/apic/apic.c:1056 [inline]
sysvec_apic_timer_interrupt+0x70/0x80 arch/x86/kernel/apic/apic.c:1056
asm_sysvec_apic_timer_interrupt+0x1a/0x20 arch/x86/include/asm/idtentry.h:729
native_safe_halt arch/x86/include/asm/irqflags.h:48 [inline]
pv_native_safe_halt+0x1e/0x30 arch/x86/kernel/paravirt.c:62
arch_safe_halt arch/x86/include/asm/paravirt.h:73 [inline]
default_idle+0xe/0x20 arch/x86/kernel/process.c:767
default_idle_call+0x6c/0xb0 kernel/sched/idle.c:122
cpuidle_idle_call kernel/sched/idle.c:199 [inline]
do_idle+0x31f/0x580 kernel/sched/idle.c:352
cpu_startup_entry+0x4f/0x60 kernel/sched/idle.c:451
start_secondary+0x1c7/0x230 arch/x86/kernel/smpboot.c:312
common_startup_64+0x13e/0x148
INITIAL USE at:
lock_acquire kernel/locking/lockdep.c:5868 [inline]
lock_acquire+0x18c/0x300 kernel/locking/lockdep.c:5825
__raw_write_lock_irq include/linux/rwlock_api_smp.h:211 [inline]
_raw_write_lock_irq+0x33/0x50 kernel/locking/spinlock.c:326
fasync_remove_entry+0xb2/0x1e0 fs/fcntl.c:1012
fasync_helper+0xa6/0xc0 fs/fcntl.c:1115
pipe_fasync+0xce/0x210 fs/pipe.c:758
__fput+0x94b/0xb50 fs/file_table.c:466
task_work_run+0x16b/0x260 kernel/task_work.c:233
exit_task_work include/linux/task_work.h:40 [inline]
do_exit+0x8c3/0x29e0 kernel/exit.c:976
__do_sys_exit kernel/exit.c:1085 [inline]
__se_sys_exit kernel/exit.c:1083 [inline]
__x64_sys_exit+0x42/0x50 kernel/exit.c:1083
x64_sys_call+0x18d8/0x18e0 arch/x86/include/generated/asm/syscalls_64.h:61
do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline]
do_syscall_64+0x111/0x680 arch/x86/entry/syscall_64.c:94
entry_SYSCALL_64_after_hwframe+0x77/0x7f
INITIAL READ USE at:
lock_acquire kernel/locking/lockdep.c:5868 [inline]
lock_acquire+0x18c/0x300 kernel/locking/lockdep.c:5825
__raw_read_lock_irqsave include/linux/rwlock_api_smp.h:172 [inline]
_raw_read_lock_irqsave+0x75/0x90 kernel/locking/spinlock.c:236
kill_fasync_rcu fs/fcntl.c:1135 [inline]
kill_fasync fs/fcntl.c:1159 [inline]
kill_fasync+0x137/0x590 fs/fcntl.c:1152
fsnotify_insert_event+0x379/0x480 fs/notify/notification.c:128
fsnotify_add_event include/linux/fsnotify_backend.h:739 [inline]
inotify_handle_inode_event+0x2a7/0x420 fs/notify/inotify/inotify_fsnotify.c:126
fsnotify_handle_inode_event.isra.0+0x1df/0x410 fs/notify/fsnotify.c:272
fsnotify_handle_event fs/notify/fsnotify.c:327 [inline]
send_to_group fs/notify/fsnotify.c:375 [inline]
fsnotify+0x147d/0x1a10 fs/notify/fsnotify.c:592
__fsnotify_parent+0x781/0xca0 fs/notify/fsnotify.c:238
fsnotify_parent include/linux/fsnotify.h:96 [inline]
fsnotify_dentry include/linux/fsnotify.h:108 [inline]
fsnotify_change include/linux/fsnotify.h:495 [inline]
notify_change+0x96b/0x1330 fs/attr.c:561
chown_common+0x3fe/0x690 fs/open.c:778
do_fchownat+0x18b/0x1e0 fs/open.c:806
__do_sys_lchown fs/open.c:831 [inline]
__se_sys_lchown fs/open.c:829 [inline]
__x64_sys_lchown+0x7e/0xc0 fs/open.c:829
do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline]
do_syscall_64+0x111/0x680 arch/x86/entry/syscall_64.c:94
entry_SYSCALL_64_after_hwframe+0x77/0x7f
}
... key at: [<ffffffffbe845a60>] __key.0+0x0/0x40
... acquired at:
check_prevs_add kernel/locking/lockdep.c:3284 [inline]
validate_chain kernel/locking/lockdep.c:3908 [inline]
__lock_acquire+0x15c0/0x2030 kernel/locking/lockdep.c:5237
lock_acquire kernel/locking/lockdep.c:5868 [inline]
lock_acquire+0x18c/0x300 kernel/locking/lockdep.c:5825
__raw_read_lock_irqsave include/linux/rwlock_api_smp.h:172 [inline]
_raw_read_lock_irqsave+0x75/0x90 kernel/locking/spinlock.c:236
kill_fasync_rcu fs/fcntl.c:1135 [inline]
kill_fasync fs/fcntl.c:1159 [inline]
kill_fasync+0x137/0x590 fs/fcntl.c:1152
__pass_event drivers/input/evdev.c:240 [inline]
evdev_pass_values.part.0+0x63a/0x950 drivers/input/evdev.c:278
evdev_pass_values drivers/input/evdev.c:253 [inline]
evdev_events+0x282/0x4e0 drivers/input/evdev.c:306
input_pass_values+0x767/0x8b0 drivers/input/input.c:128
input_event_dispose drivers/input/input.c:342 [inline]
input_handle_event+0xe43/0x1510 drivers/input/input.c:370
input_inject_event+0x1e5/0x420 drivers/input/input.c:424
evdev_write+0x30a/0x460 drivers/input/evdev.c:528
vfs_write+0x2b1/0x11a0 fs/read_write.c:686
ksys_write+0x1ef/0x240 fs/read_write.c:740
do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline]
do_syscall_64+0x111/0x680 arch/x86/entry/syscall_64.c:94
entry_SYSCALL_64_after_hwframe+0x77/0x7f
stack backtrace:
CPU: 2 UID: 0 PID: 30382 Comm: syz.6.15929 Kdump: loaded Not tainted 7.0.0-rc6-00259-g427a4f9708ee #82 PREEMPT(lazy)
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.3-5.el9 11/05/2023
Call Trace:
<TASK>
__dump_stack lib/dump_stack.c:94 [inline]
dump_stack_lvl+0xca/0x120 lib/dump_stack.c:120
print_bad_irq_dependency kernel/locking/lockdep.c:2616 [inline]
check_irq_usage+0x8a0/0xc50 kernel/locking/lockdep.c:2857
check_prev_add+0xfd/0xcf0 kernel/locking/lockdep.c:3169
check_prevs_add kernel/locking/lockdep.c:3284 [inline]
validate_chain kernel/locking/lockdep.c:3908 [inline]
__lock_acquire+0x15c0/0x2030 kernel/locking/lockdep.c:5237
lock_acquire kernel/locking/lockdep.c:5868 [inline]
lock_acquire+0x18c/0x300 kernel/locking/lockdep.c:5825
__raw_read_lock_irqsave include/linux/rwlock_api_smp.h:172 [inline]
_raw_read_lock_irqsave+0x75/0x90 kernel/locking/spinlock.c:236
kill_fasync_rcu fs/fcntl.c:1135 [inline]
kill_fasync fs/fcntl.c:1159 [inline]
kill_fasync+0x137/0x590 fs/fcntl.c:1152
__pass_event drivers/input/evdev.c:240 [inline]
evdev_pass_values.part.0+0x63a/0x950 drivers/input/evdev.c:278
evdev_pass_values drivers/input/evdev.c:253 [inline]
evdev_events+0x282/0x4e0 drivers/input/evdev.c:306
input_pass_values+0x767/0x8b0 drivers/input/input.c:128
input_event_dispose drivers/input/input.c:342 [inline]
input_handle_event+0xe43/0x1510 drivers/input/input.c:370
input_inject_event+0x1e5/0x420 drivers/input/input.c:424
evdev_write+0x30a/0x460 drivers/input/evdev.c:528
vfs_write+0x2b1/0x11a0 fs/read_write.c:686
ksys_write+0x1ef/0x240 fs/read_write.c:740
do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline]
do_syscall_64+0x111/0x680 arch/x86/entry/syscall_64.c:94
entry_SYSCALL_64_after_hwframe+0x77/0x7f
RIP: 0033:0x7fbb7b9b282d
Code: 5b 41 5c c3 66 0f 1f 84 00 00 00 00 00 f3 0f 1e fa 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 8b 0d bb 15 0f 00 f7 d8 64 89 01 48
RSP: 002b:00007fbb7a499cb8 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
RAX: ffffffffffffffda RBX: 00000000005d61c0 RCX: 00007fbb7b9b282d
RDX: 0000000000000048 RSI: 0000200000000480 RDI: 0000000000000003
RBP: 00000000005d61c0 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000000
R13: 00000000005d6258 R14: 00007fbb7b934fe0 R15: 0000000000000000
</TASK>
--
All rights reversed.
^ permalink raw reply
* Re: [PATCH v2 1/4] HID: hid-msi: Add MSI Claw configuration driver
From: Derek John Clark @ 2026-05-14 13:43 UTC (permalink / raw)
To: sashiko-reviews; +Cc: linux-input, dmitry.torokhov
In-Reply-To: <20260514115126.BCAD8C2BCB3@smtp.kernel.org>
On Thu, May 14, 2026 at 4:51 AM <sashiko-bot@kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
> - [High] The `guard(mutex)` macro in `claw_hw_output_report` is scoped to an `if` block, prematurely releasing the lock before the critical section.
> - [High] The driver publishes `drvdata` via `hid_set_drvdata()` before fully initializing its mutex and completion objects, creating a race condition.
> - [Medium] An invalid, uninitialized gamepad mode (0) may be sent to the hardware during system resume.
> - [Medium] The 8ms timeout used in `claw_hw_output_report()` is vulnerable to jiffies rounding, leading to spurious read failures.
> - [Medium] The commit message claims probe checks MCU RAM address variances, but the implementation is missing.
> --
>
> commit 13b388de96d7484eee908adeed127e8086fc289e
> Author: Derek J. Clark <derekjohn.clark@gmail.com>
>
> HID: hid-msi: Add MSI Claw configuration driver
>
> This commit adds a configuration HID driver for the MSI Claw series of
> handheld PCs. It provides support for switching the gamepad mode and changing
> the behavior of the M-keys.
>
> > Additionally, the MCU RAM address for writing configuration data has
> > changed twice. Checks are done during probe to enumerate these variances.
>
> The commit message states that checks are done during probe to enumerate MCU
> RAM address variances.
>
> Looking at claw_probe() and msi_probe(), there does not appear to be any code
> that checks firmware versions or handles these variances.
>
> Was this functionality dropped during refactoring, or should the commit message
> be updated?
>
> > diff --git a/drivers/hid/hid-msi.c b/drivers/hid/hid-msi.c
> > new file mode 100644
> > index 0000000000000..8915942af15e6
> > --- /dev/null
> > +++ b/drivers/hid/hid-msi.c
>
> [ ... ]
>
> > +static int claw_hw_output_report(struct hid_device *hdev, u8 index, u8 *data,
> > + size_t len, unsigned int timeout)
> > +{
> > + unsigned char *dmabuf __free(kfree) = NULL;
> > + u8 header[] = { CLAW_OUTPUT_REPORT_ID, 0, 0, 0x3c, index };
> > + struct claw_drvdata *drvdata = hid_get_drvdata(hdev);
> > + size_t header_size = ARRAY_SIZE(header);
> > + int ret;
> > +
> > + if (header_size + len > CLAW_PACKET_SIZE)
> > + return -EINVAL;
> > +
> > + /* We can't use a devm_alloc reusable buffer without side effects during suspend */
> > + dmabuf = kzalloc(CLAW_PACKET_SIZE, GFP_KERNEL);
> > + if (!dmabuf)
> > + return -ENOMEM;
> > +
> > + memcpy(dmabuf, header, header_size);
> > + if (data && len)
> > + memcpy(dmabuf + header_size, data, len);
> > +
> > + /* Don't hold a mutex when timeout=0, those commands cause USB disconnect */
> > + if (timeout) {
> > + guard(mutex)(&drvdata->cfg_mutex);
> > + reinit_completion(&drvdata->send_cmd_complete);
> > + }
>
> Does this drop the lock too early?
>
Hi robot,
Will fix.
> The guard() macro releases the lock at the end of its enclosing scope.
> Because it is declared inside the if (timeout) block, the mutex will be
> released immediately at the closing brace, leaving hid_hw_output_report() and
> the subsequent wait unprotected against concurrent access.
>
> > +
> > + dev_dbg(&hdev->dev, "Send data as raw output report: [%*ph]\n",
> > + CLAW_PACKET_SIZE, dmabuf);
> > +
> > + ret = hid_hw_output_report(hdev, dmabuf, CLAW_PACKET_SIZE);
> > + if (ret < 0)
> > + return ret;
> > +
> > + ret = ret == CLAW_PACKET_SIZE ? 0 : -EIO;
> > + if (ret)
> > + return ret;
> > +
> > + if (timeout) {
> > + ret = wait_for_completion_interruptible_timeout(&drvdata->send_cmd_complete,
> > + msecs_to_jiffies(timeout));
>
> Could this timeout evaluate to 1 jiffy on systems configured with HZ=100?
This seems like an unlikely config for a performance gaming system.
> Several callers, such as gamepad_mode_show(), pass an 8ms timeout to this
> function. A 1-jiffy timeout can expire almost instantaneously if the operation
> begins near the end of the current tick boundary, potentially leading to
> spurious -EBUSY timeouts.
>
> [ ... ]
>
> > +static void cfg_resume_fn(struct work_struct *work)
> > +{
> > + struct delayed_work *dwork = container_of(work, struct delayed_work, work);
> > + struct claw_drvdata *drvdata = container_of(dwork, struct claw_drvdata, cfg_resume);
> > + u8 data[2] = { drvdata->gamepad_mode, drvdata->mkeys_function };
>
> If the system suspends before cfg_setup_fn() executes, or if the device read in
> cfg_setup_fn() fails, could drvdata->gamepad_mode still be 0 here?
>
> Since 0 is not a valid mode (valid modes like CLAW_GAMEPAD_MODE_XINPUT start
> at 1), will this send an uninitialized configuration state to the hardware?
I'll add a valid default value.
> > + int ret;
> > +
> > + ret = claw_hw_output_report(drvdata->hdev, CLAW_COMMAND_TYPE_SWITCH_MODE, data,
> > + ARRAY_SIZE(data), 0);
> > + if (ret)
> > + dev_err(&drvdata->hdev->dev, "Failed to set gamepad mode settings: %d\n", ret);
> > +}
> > +
> > +static int claw_probe(struct hid_device *hdev, u8 ep)
> > +{
> > + struct claw_drvdata *drvdata;
> > + int ret;
> > +
> > + drvdata = devm_kzalloc(&hdev->dev, sizeof(*drvdata), GFP_KERNEL);
> > + if (!drvdata)
> > + return -ENOMEM;
> > +
> > + hid_set_drvdata(hdev, drvdata);
> > + drvdata->hdev = hdev;
> > + drvdata->ep = ep;
> > +
> > + mutex_init(&drvdata->cfg_mutex);
> > + init_completion(&drvdata->send_cmd_complete);
>
> Is it safe to publish drvdata via hid_set_drvdata() before initializing the
> mutex and completion object?
>
> Since hid_hw_start() was already called earlier in msi_probe(), the hardware
> is active. If an input report arrives right after hid_set_drvdata(), the
> interrupt handler msi_raw_event() will process it and call complete() on an
> uninitialized waitqueue.
>
I'll reorder this.
> > +
> > + /* For control interface: open the HID transport for sending commands. */
> > + ret = hid_hw_open(hdev);
> > + if (ret)
> > + return ret;
> > +
> > + INIT_DELAYED_WORK(&drvdata->cfg_resume, &cfg_resume_fn);
> > + INIT_DELAYED_WORK(&drvdata->cfg_setup, &cfg_setup_fn);
> > + schedule_delayed_work(&drvdata->cfg_setup, msecs_to_jiffies(500));
> > +
> > + return 0;
> > +}
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260513231445.3213501-1-derekjohn.clark@gmail.com?part=1
^ permalink raw reply
* Re: [PATCH v2 2/4] HID: hid-msi: Add M-key mapping attributes
From: Derek John Clark @ 2026-05-14 13:48 UTC (permalink / raw)
To: sashiko-reviews; +Cc: dmitry.torokhov, linux-input
In-Reply-To: <20260514122200.E52C4C2BCB3@smtp.kernel.org>
On Thu, May 14, 2026 at 5:22 AM <sashiko-bot@kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
> - [High] The `cfg_mutex` guard in `claw_hw_output_report()` is scoped to an `if` block, releasing the lock before the critical section executes.
> - [High] Lockless concurrent access to `profile_pending` between process and interrupt contexts causes data corruption and sequence mismatches on delayed responses.
> - [High] Unsafe assumption of USB transport in `claw_probe()` causes type confusion and invalid pointer dereference for non-USB devices.
> - [High] Returning an error from `claw_profile_event()` for a spurious event unconditionally triggers `complete()`, causing premature wakeups of unrelated commands.
> - [Medium] The function `claw_buttons_store()` illegally mixes scope-based `guard()` cleanup with `goto`-based error handling.
> --
>
> commit 42be71c046dc9b6b31d28598f5e0bb568d78b7a1
> Author: Derek J. Clark <derekjohn.clark@gmail.com>
>
> HID: hid-msi: Add M-key mapping attributes
>
> Adds attributes that allow for remapping the M-keys with up to 5 values
> when in macro mode.
>
> > diff --git a/drivers/hid/hid-msi.c b/drivers/hid/hid-msi.c
> > index 8915942af15e6..13ba2747fdb66 100644
> > --- a/drivers/hid/hid-msi.c
> > +++ b/drivers/hid/hid-msi.c
>
> [ ... ]
>
> > @@ -125,6 +294,31 @@ static int claw_gamepad_mode_event(struct claw_drvdata *drvdata,
> > return 0;
> > }
> >
> > +static int claw_profile_event(struct claw_drvdata *drvdata, struct claw_command_report *cmd_rep)
> > +{
> > + u8 *codes;
> > + int i;
> > +
> > + switch (drvdata->profile_pending) {
> > + case CLAW_M1_PENDING:
> > + case CLAW_M2_PENDING:
> > + codes = (drvdata->profile_pending == CLAW_M1_PENDING) ?
> > + drvdata->m1_codes : drvdata->m2_codes;
> > + /* Extract key codes; replace disabled (0xff) with 0x00, which is (null) in _show */
> > + for (i = 0; i < CLAW_KEYS_MAX; i++)
> > + codes[i] = (cmd_rep->data[6 + i] != 0xff) ? cmd_rep->data[6 + i] : 0x00;
> > + break;
>
> Is it safe to read and modify profile_pending locklessly here in the
> interrupt handler?
>
> Since claw_buttons_show() modifies this state in process context under
> profile_mutex, if a sysfs request times out and a subsequent request starts,
> could the delayed ACK from the first request cause this handler to see the
> new profile_pending state and corrupt the new request's buffer?
The lock covers this case as it is only released after the completion
fires or time out, after this.
> > + default:
> > + dev_warn(&drvdata->hdev->dev,
> > + "Got profile event without changes pending from command: %x\n",
> > + cmd_rep->cmd);
> > + return -EINVAL;
> > + }
> > + drvdata->profile_pending = CLAW_NO_PENDING;
> > +
> > + return 0;
> > +}
> > +
> > static int claw_raw_event(struct claw_drvdata *drvdata, struct hid_report *report,
> > u8 *data, int size)
> > {
>
> [ ... ]
>
> > @@ -146,6 +340,9 @@ static int claw_raw_event(struct claw_drvdata *drvdata, struct hid_report *repor
> > case CLAW_COMMAND_TYPE_GAMEPAD_MODE_ACK:
> > ret = claw_gamepad_mode_event(drvdata, cmd_rep);
> > break;
> > + case CLAW_COMMAND_TYPE_READ_PROFILE_ACK:
> > + ret = claw_profile_event(drvdata, cmd_rep);
> > + break;
> > case CLAW_COMMAND_TYPE_ACK:
> > break;
> > default:
>
> When claw_profile_event() returns -EINVAL for a spurious event,
> claw_raw_event() breaks from the switch and unconditionally calls
> complete(&drvdata->send_cmd_complete).
That case is unlikely as the profile events are serialized by a mutex.
Holding the completion until timeout would only result in delaying
subsequent calls.
> Could this prematurely wake up a thread waiting on a completely different
> command, such as WRITE_PROFILE_DATA?
>
> [ ... ]
>
> > @@ -366,6 +563,161 @@ static ssize_t reset_store(struct device *dev, struct device_attribute *attr,
> > }
> > static DEVICE_ATTR_WO(reset);
> >
> > +static int button_mapping_name_to_code(const char *name)
> > +{
>
> [ ... ]
>
> > +static int claw_buttons_store(struct device *dev, const char *buf, u8 mkey_idx)
> > +{
> > + struct hid_device *hdev = to_hid_device(dev);
> > + struct claw_drvdata *drvdata = hid_get_drvdata(hdev);
> > + u8 data[] = { 0x01, (drvdata->bmap_addr[mkey_idx] >> 8) & 0xff,
> > + drvdata->bmap_addr[mkey_idx] & 0xff, 0x07,
> > + 0x04, 0x00, 0xff, 0xff, 0xff, 0xff, 0xff };
> > + size_t len = ARRAY_SIZE(data);
> > + int ret, key_count, i;
> > + char **raw_keys;
> > +
> > + raw_keys = argv_split(GFP_KERNEL, buf, &key_count);
> > + if (!raw_keys)
> > + return -ENOMEM;
> > +
> > + guard(mutex)(&drvdata->rom_mutex); /* all err_free paths must be in scope */
> > + if (key_count > CLAW_KEYS_MAX) {
> > + ret = -EINVAL;
> > + goto err_free;
> > + }
> > +
> > + if (key_count == 0)
> > + goto set_buttons;
> > +
> > + for (i = 0; i < key_count; i++) {
> > + ret = button_mapping_name_to_code(raw_keys[i]);
> > + if (ret < 0)
> > + goto err_free;
> > +
> > + data[6 + i] = ret;
> > + }
> > +
> > +set_buttons:
> > + ret = claw_hw_output_report(hdev, CLAW_COMMAND_TYPE_WRITE_PROFILE_DATA, data, len, 8);
>
> While reviewing the addition of these sysfs attributes, I noticed a
> pre-existing issue in claw_hw_output_report() that is exacerbated by
> the concurrent sysfs requests added here:
>
> drivers/hid/hid-msi.c:claw_hw_output_report() {
> ...
> if (timeout) {
> guard(mutex)(&drvdata->cfg_mutex);
> reinit_completion(&drvdata->send_cmd_complete);
> }
> ...
> }
>
> Since guard() is scoped to the if block, the mutex is immediately released at
> the closing brace.
>
> Doesn't this mean hid_hw_output_report() and
> wait_for_completion_interruptible_timeout() execute entirely without
> the lock, defeating the intended serialization and allowing concurrent
> requests to race?
>
> > + if (ret < 0)
> > + goto err_free;
> > +
> > + ret = claw_hw_output_report(hdev, CLAW_COMMAND_TYPE_SYNC_TO_ROM, NULL, 0, 8);
> > +
> > +err_free:
> > + argv_free(raw_keys);
> > + return ret;
> > +}
>
> Is it safe to mix goto-based error handling with scope-based guard() cleanup
> in the same function?
I'll make this a scoped guard over claw_hw_output_report.
> According to the cleanup guidelines, functions should either convert
> entirely to scope-based cleanup, for example by using a DEFINE_FREE
> wrapper for argv_free, or not use guard() at all to avoid confusing
> ownership semantics.
>
> [ ... ]
>
> > @@ -432,8 +796,31 @@ static void cfg_resume_fn(struct work_struct *work)
> > dev_err(&drvdata->hdev->dev, "Failed to set gamepad mode settings: %d\n", ret);
> > }
> >
> > +static void claw_features_supported(struct claw_drvdata *drvdata)
> > +{
>
> [ ... ]
>
> > static int claw_probe(struct hid_device *hdev, u8 ep)
> > {
> > + struct usb_interface *intf = to_usb_interface(hdev->dev.parent);
> > + struct usb_device *udev = interface_to_usbdev(intf);
>
> Is it guaranteed that this device is always connected via USB?
This function is only called by msi_probe, which checks for a valid
USB device as a first step.
> If this driver is bound to a device connected over a non-USB transport like
> I2C, Bluetooth, or UHID, hdev->dev.parent would not be a valid USB
> interface, which could cause an invalid pointer dereference here.
>
> Could this code check hid_is_usb(hdev) first or read the version from
> hdev->version instead?
>
> > struct claw_drvdata *drvdata;
> > int ret;
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260513231445.3213501-1-derekjohn.clark@gmail.com?part=2
^ permalink raw reply
* Re: [PATCH v2 3/4] HID: hid-msi: Add RGB control interface
From: Derek John Clark @ 2026-05-14 13:54 UTC (permalink / raw)
To: sashiko-reviews; +Cc: linux-input, dmitry.torokhov
In-Reply-To: <20260514125337.950E8C2BCB3@smtp.kernel.org>
On Thu, May 14, 2026 at 5:53 AM <sashiko-bot@kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
> - [Critical] The `speed_show` function incorrectly casts the LED class device to a HID device, resulting in an invalid pointer dereference and a guaranteed kernel panic.
> - [Critical] Missing bounds check on `frame->frame_count` from the USB device leads to an out-of-bounds read of kernel memory.
> - [High] Re-arming `rgb_queue` from sysfs attributes after it has been explicitly cancelled in `claw_remove` leads to a Use-After-Free.
> - [High] The `guard(mutex)` macro in `claw_hw_output_report()` is incorrectly used inside a block scope, providing absolutely no synchronization.
> - [High] Concurrent modification of shared RGB state without lock protection causes data races and memory corruption.
> - [High] Every RGB state change triggers a `SYNC_TO_ROM` command, causing severe flash memory degradation.
> - [Medium] An early return in `cfg_setup_fn` silently drops core gamepad sysfs attributes if RGB initialization fails.
> --
>
> commit a90ff4a0f03b6df7827bfbcd222f977138b9d280
> Author: Derek J. Clark <derekjohn.clark@gmail.com>
>
> HID: hid-msi: Add RGB control interface
>
> Adds RGB control interface for MSI Claw devices. The MSI Claw uses a
> fairly unique RGB interface. It has 9 total zones (4 per joystick ring
> and 1 for the ABXY buttons), and supports up to 8 sequential frames of
> RGB zone data.
>
> > --- a/drivers/hid/hid-msi.c
> > +++ b/drivers/hid/hid-msi.c
>
> [ ... ]
>
> > @@ -308,15 +368,52 @@ static int claw_profile_event(struct claw_drvdata *drvdata, struct claw_command_
> > for (i = 0; i < CLAW_KEYS_MAX; i++)
> > codes[i] = (cmd_rep->data[6 + i] != 0xff) ? cmd_rep->data[6 + i] : 0x00;
> > break;
> > + case CLAW_RGB_PENDING:
> > + frame = (struct rgb_report *)cmd_rep->data;
> > + rgb_addr = drvdata->rgb_addr;
> > + read_addr = be16_to_cpu(frame->read_addr);
> > + frame_calc = (read_addr - rgb_addr) / CLAW_RGB_FRAME_OFFSET;
> > + if (frame_calc > U8_MAX) {
> > + dev_err(drvdata->led_mc.led_cdev.dev, "Got unsupported frame index: %x\n",
> > + frame_calc);
> > + ret = -EINVAL;
> > + goto err_pending;
> > + }
> > + f_idx = frame_calc;
> > +
> > + if (f_idx >= CLAW_RGB_MAX_FRAMES) {
> > + dev_err(drvdata->led_mc.led_cdev.dev, "Got illegal frame index: %x\n",
> > + f_idx);
> > + ret = -EINVAL;
> > + goto err_pending;
> > + }
> > +
> > + /* Always treat the first frame as the truth for these constants */
> > + if (f_idx == 0) {
> > + drvdata->rgb_frame_count = frame->frame_count;
>
> drvdata->rgb_frame_count is assigned directly from the hardware packet.
> Should there be a bounds check here to ensure it is less than or equal to
> CLAW_RGB_MAX_FRAMES?
>
> Later, claw_write_rgb_state() uses drvdata->rgb_frame_count as the upper
> bound for a loop reading from drvdata->rgb_frames. If a device sends a
> frame count greater than 8, could this read out-of-bounds kernel memory
> and send it back to the USB device?
I'll add a bounds check, though again it is unlikely with the previous checks.
> > + /* Invert device speed (20-0) to sysfs speed (0-20) */
> > + drvdata->rgb_speed = frame->speed;
> > + drvdata->led_mc.led_cdev.brightness = frame->brightness;
> > + drvdata->led_mc.subled_info[0].intensity = frame->zone_data.zone[0].red;
> > + drvdata->led_mc.subled_info[1].intensity = frame->zone_data.zone[0].green;
> > + drvdata->led_mc.subled_info[2].intensity = frame->zone_data.zone[0].blue;
> > + }
> > +
> > + memcpy(&drvdata->rgb_frames[f_idx], &frame->zone_data,
> > + sizeof(struct rgb_frame));
>
> claw_profile_event() modifies drvdata->rgb_frames and
> drvdata->rgb_frame_count from the HID raw event handler context.
> Simultaneously, claw_rgb_queue_fn() runs in a workqueue and calls
> functions that overwrite these exact same fields.
>
> Since there are no locks protecting these updates, could a background
> worker read torn state or corrupt the frame arrays while they are being
> actively updated by the asynchronous device response?
I'll add another mutex I suppose.
> [ ... ]
>
> > @@ -759,6 +856,397 @@ static const struct attribute_group claw_gamepad_attr_group = {
> > .is_visible = claw_gamepad_attr_is_visible,
> > };
> >
> > +/* Read RGB config from device */
> > +static int claw_read_rgb_config(struct hid_device *hdev)
> > +{
>
> [ ... ]
>
> > +/* Send RGB configuration to device */
> > +static int claw_write_rgb_state(struct claw_drvdata *drvdata)
> > +{
> > + struct rgb_report report = { 0x01, 0x0000, CLAW_RGB_FRAME_OFFSET, 0x00,
> > + drvdata->rgb_frame_count, 0x09, drvdata->rgb_speed,
> > + drvdata->led_mc.led_cdev.brightness };
> > + u16 write_addr = drvdata->rgb_addr;
> > + size_t len = sizeof(report);
> > + int f, ret;
> > +
> > + if (!drvdata->rgb_addr)
> > + return -ENODEV;
> > +
> > + if (!drvdata->rgb_frame_count)
> > + return -EINVAL;
> > +
> > + guard(mutex)(&drvdata->rom_mutex);
> > + /* Loop through (up to) 8 pages of RGB data */
> > + for (f = 0; f < drvdata->rgb_frame_count; f++) {
> > + report.zone_data = drvdata->rgb_frames[f];
> > +
> > + /* Set the MCU address to write the frame data to */
> > + report.read_addr = cpu_to_be16(write_addr);
> > +
> > + /* Serialize the rgb_report and write it to MCU */
> > + ret = claw_hw_output_report(drvdata->hdev, CLAW_COMMAND_TYPE_WRITE_PROFILE_DATA,
> > + (u8 *)&report, len, 8);
> > + if (ret)
> > + return ret;
> > +
> > + /* Increment the write addr by the offset for the next frame */
> > + write_addr += CLAW_RGB_FRAME_OFFSET;
> > + }
> > +
> > + ret = claw_hw_output_report(drvdata->hdev, CLAW_COMMAND_TYPE_SYNC_TO_ROM, NULL, 0, 8);
>
> Does sending CLAW_COMMAND_TYPE_SYNC_TO_ROM on every RGB state change
> cause excessive flash memory degradation?
>
> Because claw_apply_rgb_state() is triggered by unthrottled sysfs writes
> (such as rapidly changing a brightness slider), this might cause hundreds
> of NVRAM writes per second and potentially degrade the MCU's flash
> endurance.
This is throttled by mod_delayed_work, where each subsequent call
under 50ms resets the timer.
> [ ... ]
>
> > +static ssize_t effect_store(struct device *dev,
> > + struct device_attribute *attr,
> > + const char *buf, size_t count)
> > +{
> > + struct led_classdev *led_cdev = dev_get_drvdata(dev);
> > + struct led_classdev_mc *led_mc = container_of(led_cdev, struct led_classdev_mc, led_cdev);
> > + struct claw_drvdata *drvdata = container_of(led_mc, struct claw_drvdata, led_mc);
> > + int ret;
> > +
> > + ret = sysfs_match_string(claw_rgb_effect_text, buf);
> > + if (ret < 0)
> > + return ret;
> > +
> > + drvdata->rgb_effect = ret;
> > + mod_delayed_work(system_wq, &drvdata->rgb_queue, msecs_to_jiffies(50));
> > +
> > + return count;
> > +}
>
> Is there a potential use-after-free here if sysfs attributes are written
> after the device is removed?
>
> In claw_remove(), the driver attempts to tear down the worker using
> cancel_delayed_work_sync(&drvdata->rgb_queue). However, the sysfs files
> and LED endpoints remain active because devm cleanup executes after
> claw_remove() completes.
>
> A concurrent write to a sysfs attribute like effect_store could call
> mod_delayed_work() and re-arm the work item, causing it to execute after
> drvdata has been freed.
> [ ... ]
>
> > +static ssize_t speed_show(struct device *dev, struct device_attribute *attr,
> > + char *buf)
> > +{
> > + struct hid_device *hdev = to_hid_device(dev);
> > + struct claw_drvdata *drvdata = hid_get_drvdata(hdev);
>
> Is the dev parameter passed to speed_show actually a hid_device?
>
I forgot to fix this.
> The rgb_attr_group sysfs group is attached to the LED class device
> (led_cdev.dev), not the parent HID device. The macro to_hid_device(dev)
> blindly performs a container_of cast, which will yield an invalid pointer
> for hdev.
>
> Dereferencing it with hid_get_drvdata(hdev) will likely cause a kernel
> panic when this world-readable attribute is accessed.
>
> [ ... ]
>
> > @@ -772,6 +1260,13 @@ static void cfg_setup_fn(struct work_struct *work)
> > return;
> > }
> >
> > + ret = claw_read_rgb_config(drvdata->hdev);
> > + if (ret) {
> > + dev_err(drvdata->led_mc.led_cdev.dev,
> > + "Failed to setup device, can't read RGB config: %d\n", ret);
> > + return;
> > + }
> > +
> > /* Add sysfs attributes after we get the device state */
> > ret = devm_device_add_group(&drvdata->hdev->dev, &claw_gamepad_attr_group);
>
> If claw_read_rgb_config() fails due to a temporary USB error or
> unsupported firmware, this early return skips the subsequent
> devm_device_add_group call.
>
> Does this unintentionally break the device's core gamepad configuration
> interface simply because the RGB initialization failed?
The expectation is that if any of these error there is a problem with
the kernel or the device hardware. This is intentional.
> This isn't a new bug introduced by this patch, but does the guard() in
> claw_hw_output_report() provide any synchronization?
>
> claw_hw_output_report() {
> if (timeout) {
> guard(mutex)(&drvdata->cfg_mutex);
> reinit_completion(&drvdata->send_cmd_complete);
> }
> ...
> }
>
> Because the guard is declared inside the block, the mutex is immediately
> released at the closing brace before hid_hw_output_report() and
> wait_for_completion_interruptible_timeout() are ever called. This seems
> to leave the I/O operations unprotected against concurrent threads, which
> this patch introduces via the new RGB workers.
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260513231445.3213501-1-derekjohn.clark@gmail.com?part=3
^ permalink raw reply
* Re: [PATCH v5 1/2] dt-bindings: input: Document Imagis ISA1200 haptic motor driver
From: Rob Herring @ 2026-05-14 14:42 UTC (permalink / raw)
To: sashiko-reviews
Cc: Svyatoslav Ryhel, linux-input, conor+dt, dmitry.torokhov, krzk+dt,
devicetree
In-Reply-To: <20260513201558.808C4C19425@smtp.kernel.org>
On Wed, May 13, 2026 at 08:15:57PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] Missing scalar type definitions for `imagis,period-ns` and `imagis,duty-cycle-ns` cause schema bounds validation to be silently bypassed.
> --
>
> commit c2a7ef2ca3d62317c69bc5b6a450107172eeeac7
> Author: Svyatoslav Ryhel <clamor95@gmail.com>
>
> dt-bindings: input: Document Imagis ISA1200 haptic motor driver
>
> This commit documents the Imagis ISA1200 haptic motor driver. It introduces
> a new YAML binding schema for the driver, specifying properties like
> control-gpios, clocks, and internal PWM configurations.
>
> > diff --git a/Documentation/devicetree/bindings/input/imagis,isa1200.yaml b/Documentation/devicetree/bindings/input/imagis,isa1200.yaml
> > new file mode 100644
> > index 0000000000000..bbe6f99d39c19
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/input/imagis,isa1200.yaml
> [ ... ]
> > @@ -0,0 +1,140 @@
> [ ... ]
> > + imagis,mode:
> > + $ref: /schemas/types.yaml#/definitions/uint32
> > + description: |
> > + Defines the motor type isa1200 drives
> > + 0 - LRA (Linear Resonant Actuator)
> > + 1 - ERM (Eccentric Rotating Mass)
> > + enum: [0, 1]
> > + default: 0
> > +
> > + imagis,period-ns:
> > + description:
> > + Period of the internal PWM channel in nanoseconds.
> > + minimum: 10000
> > + maximum: 30000
> > +
> > + imagis,duty-cycle-ns:
> > + description:
> > + Duty cycle of the external/internal PWM channel in nanoseconds,
> > + defaults to 50% of the channel's period
>
> Will the minimum and maximum bounds on imagis,period-ns be silently ignored
> during validation?
>
> The core property-units.yaml schema automatically assigns an array type to
> properties ending in -ns. Without an explicit reference to
> /schemas/types.yaml#/definitions/uint32, these properties are evaluated as
> arrays rather than scalar integers.
>
> In standard JSON Schema, numeric constraints like minimum and maximum are
> ignored when applied to an array instance. Adding the uint32 type reference
> for both imagis,period-ns and imagis,duty-cycle-ns would ensure the
> constraints are properly enforced on a single scalar value.
Wrong.
While that's true for json-schema, the standard unit properties
schemas will be fixed up to the right type.
Rob
^ permalink raw reply
* Re: [PATCH v5 1/2] dt-bindings: input: Document Imagis ISA1200 haptic motor driver
From: Rob Herring @ 2026-05-14 14:55 UTC (permalink / raw)
To: Svyatoslav Ryhel
Cc: Dmitry Torokhov, Krzysztof Kozlowski, Conor Dooley, Linus Walleij,
linux-input, devicetree, linux-kernel
In-Reply-To: <20260512102445.55372-2-clamor95@gmail.com>
On Tue, May 12, 2026 at 01:24:44PM +0300, Svyatoslav Ryhel wrote:
> Document the Imagis ISA1200 haptic motor driver, used primarily in mobile
> handheld devices and capable of supporting up to two motors.
>
> The exact datasheet for the ISA1200 is not available; all data was modeled
> based on available downstream kernel sources for various devices and
> fragments of information scattered across the internet.
>
> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> ---
> .../bindings/input/imagis,isa1200.yaml | 140 ++++++++++++++++++
> 1 file changed, 140 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/input/imagis,isa1200.yaml
>
> diff --git a/Documentation/devicetree/bindings/input/imagis,isa1200.yaml b/Documentation/devicetree/bindings/input/imagis,isa1200.yaml
> new file mode 100644
> index 000000000000..bbe6f99d39c1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/imagis,isa1200.yaml
> @@ -0,0 +1,140 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/input/imagis,isa1200.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Imagis ISA1200 haptic motor driver
> +
> +maintainers:
> + - Svyatoslav Ryhel <clamor95@gmail.com>
> + - Linus Walleij <linusw@kernel.org>
> +
> +description:
> + The ISA1200 is a high-performance enhanced haptic motor driver designed
> + for mobile hand-held devices. It supports various voltages for both ERM
> + (Eccentric Rotating Mass) and LRA (Linear Resonant Actuator) type
> + actuators. Thanks to an embedded LDO, battery power can be used directly
> + in handheld applications.
> +
> +properties:
> + compatible:
> + const: imagis,isa1200
> +
> + reg:
> + maxItems: 1
> +
> + control-gpios:
> + description:
> + One or two GPIOs flagged as active high linked to HEN and LEN pins
minItems: 1
With that,
Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
> + maxItems: 2
^ permalink raw reply
* Re: [PATCH v5 1/2] dt-bindings: input: Document Imagis ISA1200 haptic motor driver
From: Svyatoslav Ryhel @ 2026-05-14 15:00 UTC (permalink / raw)
To: Rob Herring
Cc: Dmitry Torokhov, Krzysztof Kozlowski, Conor Dooley, Linus Walleij,
linux-input, devicetree, linux-kernel
In-Reply-To: <20260514145541.GB472306-robh@kernel.org>
чт, 14 трав. 2026 р. о 17:55 Rob Herring <robh@kernel.org> пише:
>
> On Tue, May 12, 2026 at 01:24:44PM +0300, Svyatoslav Ryhel wrote:
> > Document the Imagis ISA1200 haptic motor driver, used primarily in mobile
> > handheld devices and capable of supporting up to two motors.
> >
> > The exact datasheet for the ISA1200 is not available; all data was modeled
> > based on available downstream kernel sources for various devices and
> > fragments of information scattered across the internet.
> >
> > Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> > ---
> > .../bindings/input/imagis,isa1200.yaml | 140 ++++++++++++++++++
> > 1 file changed, 140 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/input/imagis,isa1200.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/input/imagis,isa1200.yaml b/Documentation/devicetree/bindings/input/imagis,isa1200.yaml
> > new file mode 100644
> > index 000000000000..bbe6f99d39c1
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/input/imagis,isa1200.yaml
> > @@ -0,0 +1,140 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/input/imagis,isa1200.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Imagis ISA1200 haptic motor driver
> > +
> > +maintainers:
> > + - Svyatoslav Ryhel <clamor95@gmail.com>
> > + - Linus Walleij <linusw@kernel.org>
> > +
> > +description:
> > + The ISA1200 is a high-performance enhanced haptic motor driver designed
> > + for mobile hand-held devices. It supports various voltages for both ERM
> > + (Eccentric Rotating Mass) and LRA (Linear Resonant Actuator) type
> > + actuators. Thanks to an embedded LDO, battery power can be used directly
> > + in handheld applications.
> > +
> > +properties:
> > + compatible:
> > + const: imagis,isa1200
> > +
> > + reg:
> > + maxItems: 1
> > +
> > + control-gpios:
> > + description:
> > + One or two GPIOs flagged as active high linked to HEN and LEN pins
>
> minItems: 1
>
In theory there may be no GPIOs and both pins can be hooked to power.
This is unlikely scenario but since it is possible I did not set
minItems and did not make control-gpios a required property.
> With that,
>
> Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
>
> > + maxItems: 2
^ permalink raw reply
* Re: [PATCH v6 2/7] mfd: Add driver for ASUS Transformer embedded controller
From: Lee Jones @ 2026-05-14 15:50 UTC (permalink / raw)
To: Svyatoslav Ryhel
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Dmitry Torokhov,
Pavel Machek, Sebastian Reichel, Ion Agorria,
Michał Mirosław, devicetree, linux-kernel, linux-input,
linux-leds, linux-pm
In-Reply-To: <CAPVz0n07EKiF=Gi=Po0zFVSuU=g4pbhJam7VHgiQsPTwtT2wQg@mail.gmail.com>
On Thu, 14 May 2026, Svyatoslav Ryhel wrote:
> чт, 14 трав. 2026 р. о 13:02 Lee Jones <lee@kernel.org> пише:
> >
> > On Sat, 02 May 2026, Svyatoslav Ryhel wrote:
> >
> > > From: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> > >
> > > Support Nuvoton NPCE795-based ECs as used in Asus Transformer TF201,
> > > TF300T, TF300TG, TF300TL and TF700T pad and dock, as well as TF101 dock
> > > and TF600T, P1801-T and TF701T pad. This is a glue driver handling
> > > detection and common operations for EC's functions.
> > >
> > > Co-developed-by: Svyatoslav Ryhel <clamor95@gmail.com>
> > > Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> > > Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> > > ---
> > > drivers/mfd/Kconfig | 14 +
> > > drivers/mfd/Makefile | 1 +
> > > drivers/mfd/asus-transformer-ec.c | 762 ++++++++++++++++++++++++
> > > include/linux/mfd/asus-transformer-ec.h | 162 +++++
> > > 4 files changed, 939 insertions(+)
> > > create mode 100644 drivers/mfd/asus-transformer-ec.c
> > > create mode 100644 include/linux/mfd/asus-transformer-ec.h
[...]
> > > + unsigned int num_devices;
> > > + bool clr_fmode; /* clear Factory Mode bit in EC control register */
> > > +};
> > > +
> > > +struct asus_ec_data {
> > > + struct asusec_info info;
> >
> > You have 'data' and 'info' which a) using non-forthcoming nomenclature
> > and doesn't tell me anything and then you b) put 'info' in the device's
> > driver_data attribute which is very confusing. driver_data should be
> > for what we call ddata which I assume is expressed as 'data' here.
> >
>
> asusec_info is shared among all child devices and is exposed while
> remaining elements of this struct are for internal use only.
Our terminology for that is usually ddata, that gets stored in
'struct devices' device_data attribute.
> > > + struct mutex ecreq_lock; /* prevent simultaneous access */
> > > + struct gpio_desc *ecreq;
> >
> > If I hadn't seen the declaration, I'd have no idea this was a GPIO
> > descriptor. Please improve the nomenclature throughout.
> >
> > > + struct i2c_client *self;
> >
> > Again, please use standard naming conventions:
> >
> > % git grep "struct i2c_client" | grep "\*self" | wc -l
> > 0
> >
> > % git grep "struct i2c_client" | grep "\*client" | wc -l
> > 6304
> >
> > % git grep "struct i2c_client" | grep "\*i2c" | wc -l
> > 903
> >
>
> ok, noted.
>
> > > + const struct asus_ec_chip_data *data;
> >
> > 'data', 'priv' and 'info' should be improved.
> >
> > > + char ec_data[DOCKRAM_ENTRY_BUFSIZE];
> >
> > An array of chars called 'data'. This could be anything.
> >
>
> Do you have a comprehensive list of name conventions you find suitable?
Anything descriptive that alludes to the type of data being held there.
There are 100's of good examples, but a handful of generic / bad ones.
> > > + bool logging_disabled;
> >
> > This debugging tool is probably never going to be used again.
> >
> > Keep it local.
> >
> > > +};
> > > +
> > > +struct dockram_ec_data {
> > > + struct mutex ctl_lock; /* prevent simultaneous access */
> > > + char ctl_data[DOCKRAM_ENTRY_BUFSIZE];
> > > +};
> > > +
> > > +#define to_ec_data(ec) \
> > > + container_of(ec, struct asus_ec_data, info)
> > > +
> > > +/**
> > > + * asus_dockram_read - Read a register from the DockRAM device.
> > > + * @client: Handle to the DockRAM device.
> > > + * @reg: Register to read.
> > > + * @buf: Byte array into which data will be read; must be large enough to
> > > + * hold the data returned by the DockRAM.
> > > + *
> > > + * This executes the DockRAM read based on the SMBus "block read" protocol
> > > + * or its emulation. It extracts DOCKRAM_ENTRY_SIZE bytes from the set
> > > + * register address.
> > > + *
> > > + * Returns a negative errno code else zero on success.
> > > + */
> > > +int asus_dockram_read(struct i2c_client *client, int reg, char *buf)
> > > +{
> >
> > Have you considered using Regmap for register access instead of
> > implementing custom functions? Remaps already deals with caching and
> > locking mechanisms that you'd get for free.
> >
> > This looks like it would be replaced with devm_regmap_init_i2c().
> >
>
> I will consider this, thank you.
>
> > > + struct device *dev = &client->dev;
> > > + int ret;
> > > +
> > > + memset(buf, 0, DOCKRAM_ENTRY_BUFSIZE);
> > > + ret = i2c_smbus_read_i2c_block_data(client, reg,
> > > + DOCKRAM_ENTRY_BUFSIZE, buf);
> > > + if (ret < 0)
> > > + return ret;
> > > +
> > > + if (buf[0] > DOCKRAM_ENTRY_SIZE) {
> > > + dev_err(dev, "bad data len; buffer: %*ph; ret: %d\n",
> > > + DOCKRAM_ENTRY_BUFSIZE, buf, ret);
> > > + return -EPROTO;
> > > + }
> > > +
> > > + dev_dbg(dev, "got data; buffer: %*ph; ret: %d\n",
> > > + DOCKRAM_ENTRY_BUFSIZE, buf, ret);
> >
> > Please remove all of these debug messages.
> >
>
> Why debug messages cannot be preserved? They are specifically marked as dev_dbg
It's a general convention.
After initial development, they tend to just litter the code-base.
Debug prints can be useful higher up the stack though.
[...]
> > > +/**
> > > + * devm_asus_ec_register_notifier - Managed registration of notifier to an
> > > + * ASUS EC blocking notifier chain.
> > > + * @pdev: Device requesting the notifier (used for resource management).
> > > + * @nb: Notifier block to be registered.
> > > + *
> > > + * Register a notifier to the ASUS EC blocking notifier chain. The notifier
> > > + * will be automatically unregistered when the requesting device is detached.
> > > + *
> > > + * Return: 0 on success or a negative error code on failure.
> > > + */
> > > +int devm_asus_ec_register_notifier(struct platform_device *pdev,
> > > + struct notifier_block *nb)
> > > +{
> >
> > Hand-rolling devres managed resources is usually reserved for subsystem
> > level API calls. Why do the usual device driver .remove() handling work
> > for you?
> >
>
> This is used by 3 subdevices: serio, keys and charger, so this just
> seems cleaner way to register and deregister notifier.
Clean to me would be to use the infrastructure that's put in place
already. Unless I am missing the point of all of this.
[...]
> > > + int ret;
> > > +
> > > + if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_I2C_BLOCK))
> > > + return dev_err_probe(dev, -ENXIO,
> > > + "I2C bus is missing required SMBus block mode support\n");
> > > +
> > > + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > > + if (!priv)
> > > + return -ENOMEM;
> > > +
> > > + priv->data = device_get_match_data(dev);
> > > + if (!priv->data)
> > > + return -ENODEV;
> > > +
> > > + i2c_set_clientdata(client, priv);
> > > + priv->self = client;
> > > +
> > > + priv->info.dockram = devm_asus_dockram_get(dev);
> > > + if (IS_ERR(priv->info.dockram))
> > > + return dev_err_probe(dev, PTR_ERR(priv->info.dockram),
> > > + "failed to get dockram\n");
> > > +
> > > + priv->ecreq = devm_gpiod_get(dev, "request", GPIOD_OUT_LOW);
> > > + if (IS_ERR(priv->ecreq))
> > > + return dev_err_probe(dev, PTR_ERR(priv->ecreq),
> > > + "failed to get request GPIO\n");
> >
> > "get" or "request"
> >
>
> request is gpio's name, request gpio
Ah yes. Maybe use 's to help with that. Right now is just reads strangely.
[...]
--
Lee Jones
^ 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