* [PATCH v2 0/2] firmware: arm_scmi: Ensure automatic module loading
From: Bjorn Andersson @ 2026-06-18 15:56 UTC (permalink / raw)
To: Sudeep Holla, Cristian Marussi, Nathan Chancellor, Nicolas Schier,
Michael Turquette
Cc: arm-scmi, linux-arm-kernel, linux-kernel, linux-kbuild,
Hans de Goede, Bjorn Andersson, Stephen Boyd, Brian Masney,
Rafael J. Wysocki, Viresh Kumar, Frank Li, Sascha Hauer,
Pengutronix Kernel Team, Fabio Estevam, Guenter Roeck,
Jyoti Bhayana, Jonathan Cameron, David Lechner, Nuno Sá,
Andy Shevchenko, Dmitry Torokhov, Ulf Hansson, Liam Girdwood,
Mark Brown, Philipp Zabel, Alexandre Belloni, linux-clk, linux-pm,
imx, linux-hwmon, linux-iio, linux-input, linux-rtc
SCMI drivers such as the Arm SCMI CPUfreq driver are allowed to built as
modules, but they are then not automatically loaded. Rework the SCMI
device table alias support to make modpost consume the information from
MODULE_DEVICE_TABLE(scmi, ...) and allow drivers to be loaded based on
this information, if known. Also add a protocol-based alias to also
trigger driver loading when only the SCMI protocol id is known.
Signed-off-by: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>
---
Changes in v2:
- Use request_module_nowait()
- Drop #include <linux/mod_devicetable.h> from scmi_protocol.h
- Link to v1: https://patch.msgid.link/20260616-scmi-modalias-v1-0-662b8dd52ab2@oss.qualcomm.com
To: Sudeep Holla <sudeep.holla@kernel.org>
To: Cristian Marussi <cristian.marussi@arm.com>
To: Michael Turquette <mturquette@baylibre.com>
To: Nicolas Schier <nsc@kernel.org>
Cc: Stephen Boyd <sboyd@kernel.org>
Cc: Brian Masney <bmasney@redhat.com>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: Viresh Kumar <viresh.kumar@linaro.org>
Cc: Frank Li <Frank.Li@nxp.com>
Cc: Sascha Hauer <s.hauer@pengutronix.de>
Cc: Pengutronix Kernel Team <kernel@pengutronix.de>
Cc: Fabio Estevam <festevam@gmail.com>
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: Jyoti Bhayana <jbhayana@google.com>
Cc: Jonathan Cameron <jic23@kernel.org>
Cc: David Lechner <dlechner@baylibre.com>
Cc: Nuno Sá <nuno.sa@analog.com>
Cc: Andy Shevchenko <andy@kernel.org>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Ulf Hansson <ulfh@kernel.org>
Cc: Liam Girdwood <lgirdwood@gmail.com>
Cc: Mark Brown <broonie@kernel.org>
Cc: Philipp Zabel <p.zabel@pengutronix.de>
Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>
Cc: Nathan Chancellor <nathan@kernel.org>
Cc: arm-scmi@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-clk@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: linux-pm@vger.kernel.org
Cc: imx@lists.linux.dev
Cc: linux-hwmon@vger.kernel.org
Cc: linux-iio@vger.kernel.org
Cc: linux-input@vger.kernel.org
Cc: linux-rtc@vger.kernel.org
Cc: linux-kbuild@vger.kernel.org
---
Bjorn Andersson (2):
module: add SCMI device table alias support
firmware: arm_scmi: request modules for discovered protocols
drivers/clk/clk-scmi.c | 1 +
drivers/cpufreq/scmi-cpufreq.c | 1 +
drivers/firmware/arm_scmi/bus.c | 20 ++++++++++----------
drivers/firmware/arm_scmi/driver.c | 3 +++
drivers/firmware/arm_scmi/scmi_power_control.c | 1 +
drivers/firmware/imx/sm-cpu.c | 1 +
drivers/firmware/imx/sm-lmm.c | 1 +
drivers/firmware/imx/sm-misc.c | 1 +
drivers/hwmon/scmi-hwmon.c | 1 +
drivers/iio/common/scmi_sensors/scmi_iio.c | 1 +
drivers/input/keyboard/imx-sm-bbm-key.c | 1 +
drivers/pmdomain/arm/scmi_perf_domain.c | 1 +
drivers/pmdomain/arm/scmi_pm_domain.c | 1 +
drivers/powercap/arm_scmi_powercap.c | 1 +
drivers/regulator/scmi-regulator.c | 1 +
drivers/reset/reset-scmi.c | 1 +
drivers/rtc/rtc-imx-sm-bbm.c | 1 +
include/linux/mod_devicetable.h | 12 ++++++++++++
include/linux/scmi_protocol.h | 5 +----
scripts/mod/devicetable-offsets.c | 4 ++++
scripts/mod/file2alias.c | 13 +++++++++++++
21 files changed, 58 insertions(+), 14 deletions(-)
---
base-commit: 8d6dbbbe3ba62de0a63e962ee004afb848c8e3ac
change-id: 20260616-scmi-modalias-0f32421bd452
Best regards,
--
Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>
^ permalink raw reply
* Re: [PATCH v8 2/7] mfd: Add driver for ASUS Transformer embedded controller
From: Svyatoslav Ryhel @ 2026-06-18 12:54 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: <20260618122605.GH1672911@google.com>
чт, 18 черв. 2026 р. о 15:26 Lee Jones <lee@kernel.org> пише:
>
> On Thu, 11 Jun 2026, Svyatoslav Ryhel wrote:
>
> > чт, 11 черв. 2026 р. о 14:17 Lee Jones <lee@kernel.org> пише:
> > >
> > > On Thu, 28 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 | 16 +
> > > > drivers/mfd/Makefile | 1 +
> > > > drivers/mfd/asus-transformer-ec.c | 542 ++++++++++++++++++++++++
> > > > include/linux/mfd/asus-transformer-ec.h | 92 ++++
> > > > 4 files changed, 651 insertions(+)
> > > > create mode 100644 drivers/mfd/asus-transformer-ec.c
> > > > create mode 100644 include/linux/mfd/asus-transformer-ec.h
>
> [...]
>
> > > > +static void asus_ec_clear_buffer(struct asus_ec_data *ddata)
> > > > +{
> > > > + int ret, retry = ASUSEC_RSP_BUFFER_SIZE;
> > > > +
> > > > + /*
> > > > + * Read the buffer till we get valid data by checking ASUSEC_OBF_MASK
> > > > + * of the status byte or till we reach end of the 256 byte buffer.
> > > > + */
> > > > + while (retry--) {
> > > > + ret = i2c_smbus_read_i2c_block_data(ddata->client, ASUSEC_READ_BUF,
> > > > + ASUSEC_ENTRY_SIZE,
> > > > + ddata->ec_buf);
> > > > + if (ret < ASUSEC_ENTRY_SIZE)
> > > > + continue;
> > > > +
> > > > + if (ddata->ec_buf[ASUSEC_IRQ_STATUS] & ASUSEC_OBF_MASK)
> > > > + continue;
> > > > +
> > > > + break;
> > > > + }
> > > > +}
> > > > +
> > > > +static int asus_ec_log_info(struct asus_ec_data *ddata, unsigned int reg,
> > > > + const char *name, const char **out)
>
> If we can avoid points to pointers, then please do.
>
> We already have ddata, so we can just set the name?
>
> It will remove a lot of the following complexity / ugliness.
>
> > > > +{
> > > > + struct device *dev = &ddata->client->dev;
> > > > + u8 buf[ASUSEC_ENTRY_BUFSIZE];
> > > > + int ret;
> > > > +
> > > > + memset(buf, 0, ASUSEC_ENTRY_BUFSIZE);
> > > > + ret = i2c_smbus_read_i2c_block_data(ddata->ec.dockram, reg,
> > > > + ASUSEC_ENTRY_SIZE, buf);
> > > > + if (ret < ASUSEC_ENTRY_SIZE)
> > > > + return ret;
> > >
> > > Same here. These should be negative.
> > >
> >
> > return ret < 0 ? ret : -EIO same as above
> >
> > > > +
> > > > + if (buf[0] > ASUSEC_ENTRY_SIZE) {
> > > > + dev_err(dev, "bad data len; buffer: %*ph; ret: %d\n",
> > > > + ASUSEC_ENTRY_BUFSIZE, buf, ret);
> > > > + return -EPROTO;
> > > > + }
> > > > +
> > > > + if (!ddata->logging_disabled) {
> > > > + dev_info(dev, "%-14s: %.*s\n", name, buf[0], buf + 1);
> > > > +
> > > > + if (out) {
> > > > + *out = devm_kasprintf(dev, GFP_KERNEL, "%.*s",
> > > > + buf[0], buf + 1);
> > > > + if (!*out)
> > > > + return -ENOMEM;
> > > > + }
> > > > + }
> > >
> > > FWIW, I hate this! What does it give you now that development is done?
> > >
> >
> > We have already discussed this, and you agreed that EC and firmware
> > prints may stay! This prints EC model and firmware info as well as EC
> > firmware behavior. It allows identify possible new revisions of EC -
> > Firmware combo and address possible regressions (check if it is chip
> > malfunction or firmware needs a new programming model) without
> > rebuilding kernel and digging downstream kernel for needed bits of
> > code.
>
> Right, so just print it out and remove all of the 'logging_disabled' and
> 'out' nonsense.
>
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static int asus_ec_reset(struct asus_ec_data *ddata)
> > > > +{
> > > > + int retry, ret;
> > > > +
> > > > + guard(mutex)(&ddata->ecreq_lock);
> > > > +
> > > > + for (retry = 0; retry < ASUSEC_RETRY_MAX; retry++) {
> > >
> > > for (int retry = ... is generally preferred for throwaway variables.
> > >
> >
> > Not that I care too much, but I am defining ret anyway, why not add
> > retry too there?
>
> This is the new and preferred way to use throw-away variables.
>
> ret is not a throw-away variable.
>
> [...]
>
> > > > +static int asus_ec_set_factory_mode(struct asus_ec_data *ddata,
> > > > + enum asusec_mode fmode)
> > > > +{
> > > > + dev_info(&ddata->client->dev, "Entering %s mode.\n",
> > > > + fmode == ASUSEC_MODE_FACTORY ? "factory" : "normal");
> > > > +
> > > > + return asus_dockram_access_ctl(ddata->ec.dockram, NULL,
> > > > + ASUSEC_CTL_FACTORY_MODE,
> > > > + fmode == ASUSEC_MODE_FACTORY ?
> > > > + ASUSEC_CTL_FACTORY_MODE : 0);
> > >
> > > Why not create make:
> > >
> > > ASUSEC_MODE_FACTORY == ASUSEC_CTL_FACTORY_MODE
> > >
> > > What happens to NORMAL?
> > >
> >
> > ASUSEC_CTL_FACTORY_MODE is a bit in the ctl register. For NORMAL mode
>
> I get that, but if the values can be shared, it make the code simpler.
>
> > bit is cleared,
> > for FACTORY bit it set, for NONE bit is ignored.
> >
> > > > +}
> > > > +
> > > > +static int asus_ec_detect(struct asus_ec_data *ddata)
> > > > +{
> > > > + int ret;
> > > > +
> > > > + ret = asus_ec_reset(ddata);
> > > > + if (ret)
> > > > + goto err_exit;
> > > > +
> > > > + asus_ec_clear_buffer(ddata);
> > > > +
> > > > + ret = asus_ec_log_info(ddata, ASUSEC_DOCKRAM_INFO_MODEL, "Model",
> > > > + &ddata->ec.model);
>
> Where is this model used?
>
Model is passed to cells to form names, particularly input device names.
> > > You can use 100-chars and make the code look beautiful! :)
> > >
> >
> > Not every subsystem permits 100 chars, some stick to 80 as a strict
> > rule, so it is better be safe.
>
> Right, but we are forward thinking here.
>
> You can and should use 100-chars in this subsystem.
>
> > > > + if (ret)
> > > > + goto err_exit;
> > > > +
> > > > + ret = asus_ec_log_info(ddata, ASUSEC_DOCKRAM_INFO_FW, "FW version",
> > > > + NULL);
> > > > + if (ret)
> > > > + goto err_exit;
> > > > +
> > > > + ret = asus_ec_log_info(ddata, ASUSEC_DOCKRAM_INFO_CFGFMT, "Config format",
> > > > + NULL);
> > > > + if (ret)
> > > > + goto err_exit;
> > > > +
> > > > + ret = asus_ec_log_info(ddata, ASUSEC_DOCKRAM_INFO_HW, "HW version",
> > > > + NULL);
> > > > + if (ret)
> > > > + goto err_exit;
> > > > +
> > > > + /* Disable logging on next EC request */
> > >
> > > Why, but why?
> > >
> >
> > Cause EC requests are frequent (handshake/reset) and constant logging
> > same data is not acceptable.
>
> Then rid the prints entirely or do them at a more appropriate point like
> during probe?
>
> Or maybe consider dev_info_once() and friends.
>
I totally forgot about dev_info_once(), thank you.
> > > > + ddata->logging_disabled = true;
> > > > +
> > > > + /* Check and inform about EC firmware behavior */
> > > > + ret = asus_ec_susb_on_status(ddata);
> > > > + if (ret)
> > > > + goto err_exit;
> > > > +
> > > > + ddata->ec.name = ddata->info->name;
> > > > +
> > > > + /* Some EC require factory mode to be set normal on each request */
> > > > + if (ddata->info->fmode)
> > > > + ret = asus_ec_set_factory_mode(ddata, ddata->info->fmode);
> > > > +
> > > > +err_exit:
> > > > + if (ret)
> > > > + dev_err(&ddata->client->dev, "failed to access EC: %d\n", ret);
> > > > +
> > > > + return ret;
> > > > +}
> > > > +
> > > > +static void asus_ec_handle_smi(struct asus_ec_data *ddata, unsigned int code)
> > > > +{
> > > > + switch (code) {
> > > > + case ASUSEC_SMI_HANDSHAKE:
> > > > + case ASUSEC_SMI_RESET:
> > > > + asus_ec_detect(ddata);
> > > > + break;
> > > > + }
> > > > +}
> > > > +
> > > > +static irqreturn_t asus_ec_interrupt(int irq, void *dev_id)
> > > > +{
> > > > + struct asus_ec_data *ddata = dev_id;
> > > > + unsigned long notify_action;
> > > > + int ret;
> > > > +
> > > > + ret = i2c_smbus_read_i2c_block_data(ddata->client, ASUSEC_READ_BUF,
> > > > + ASUSEC_ENTRY_SIZE, ddata->ec_buf);
> > > > + if (ret < ASUSEC_ENTRY_SIZE ||
> > > > + !(ddata->ec_buf[ASUSEC_IRQ_STATUS] & ASUSEC_OBF_MASK))
> > >
> > > Unwrap for readability.
> > >
> > > Also, I think a comment would be helpful.
> > >
> >
> > if (ret < ASUSEC_ENTRY_SIZE)
> > return IRQ_NONE;
> >
> > ret = ddata->ec_buf[ASUSEC_IRQ_STATUS] & ASUSEC_OBF_MASK;
> > if (!ret)
> > return IRQ_NONE;
> >
> > This would be acceptable? (I will add comments later on)
>
> Yes, better.
>
> If you're not using ret again, you could just put 'ddata.." inside the if().
>
I thought about this, but that would require combining it with (!)
which will not help with readabilty.
> > > > + return IRQ_NONE;
> > > > +
> > > > + notify_action = ddata->ec_buf[ASUSEC_IRQ_STATUS];
> > > > + if (notify_action & ASUSEC_SMI_MASK) {
> > > > + unsigned int code = ddata->ec_buf[ASUSEC_SMI_CODE];
> > > > +
> > > > + asus_ec_handle_smi(ddata, code);
> > > > +
> > > > + notify_action |= code << 8;
> > > > + }
> > > > +
> > > > + blocking_notifier_call_chain(&ddata->ec.notify_list,
> > > > + notify_action, ddata->ec_buf);
> > > > +
> > > > + return IRQ_HANDLED;
> > > > +}
> > > > +
> > > > +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 *ddata;
> > > > + int ret;
> > > > +
> > > > + dockram = i2c_new_ancillary_device(parent, "dockram",
> > > > + parent->addr + 2);
> > >
> > > Could we define a macro for the address offset '2' here to avoid using a magic
> > > number?
> > >
> >
> > It seems that you are excessively concerned with "magic numbers".
>
> Bingo! I HATE magic numbers.
>
> https://lore.kernel.org/all/?q=%22Lee+Jones%22+magic
>
> ~900 messages! =:-D
>
AHAHAH, ok, this makes things clearer. Lemmy have a note about this quirk.
> [...]
>
> > > > +static const struct asus_ec_chip_info asus_ec_tf600t_pad_data = {
> > > > + .name = "pad",
> > > > + .variant = ASUSEC_TF600T_PAD,
> > > > + .fmode = ASUSEC_MODE_NORMAL,
> > > > +};
> > >
> > > Any reason not to just pass the identifier (variant) and add the name
> > > and fmode attribues to the switch() above?
> >
> > Why not set it here, I am not passing any mfd or any other API via of data.
>
> I get that, and you're not breaking any of my golden rules.
>
> However, I just think doing everything in one place, usually a which
> based off of the 'variant' which you pass as a single value, is a nicer,
> more consolidated way of doing things.
>
Well, I would need to pass OF data regardless, so why not bundle it
with all required info and leave probes switch for strictly mfd cell
assignment. I suppose this is more a personal preference issue. I hope
you will not mind if I leave this as it is?
All other comments you have left and I did not answer directly were
read and acknowledged. Thank you!
> --
> Lee Jones
^ permalink raw reply
* Re: [PATCH v8 2/7] mfd: Add driver for ASUS Transformer embedded controller
From: Lee Jones @ 2026-06-18 12:26 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: <CAPVz0n0caBBt6A+AFeUpGdxvb3Qhoui7khLCt3747bPUKmMXhQ@mail.gmail.com>
On Thu, 11 Jun 2026, Svyatoslav Ryhel wrote:
> чт, 11 черв. 2026 р. о 14:17 Lee Jones <lee@kernel.org> пише:
> >
> > On Thu, 28 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 | 16 +
> > > drivers/mfd/Makefile | 1 +
> > > drivers/mfd/asus-transformer-ec.c | 542 ++++++++++++++++++++++++
> > > include/linux/mfd/asus-transformer-ec.h | 92 ++++
> > > 4 files changed, 651 insertions(+)
> > > create mode 100644 drivers/mfd/asus-transformer-ec.c
> > > create mode 100644 include/linux/mfd/asus-transformer-ec.h
[...]
> > > +static void asus_ec_clear_buffer(struct asus_ec_data *ddata)
> > > +{
> > > + int ret, retry = ASUSEC_RSP_BUFFER_SIZE;
> > > +
> > > + /*
> > > + * Read the buffer till we get valid data by checking ASUSEC_OBF_MASK
> > > + * of the status byte or till we reach end of the 256 byte buffer.
> > > + */
> > > + while (retry--) {
> > > + ret = i2c_smbus_read_i2c_block_data(ddata->client, ASUSEC_READ_BUF,
> > > + ASUSEC_ENTRY_SIZE,
> > > + ddata->ec_buf);
> > > + if (ret < ASUSEC_ENTRY_SIZE)
> > > + continue;
> > > +
> > > + if (ddata->ec_buf[ASUSEC_IRQ_STATUS] & ASUSEC_OBF_MASK)
> > > + continue;
> > > +
> > > + break;
> > > + }
> > > +}
> > > +
> > > +static int asus_ec_log_info(struct asus_ec_data *ddata, unsigned int reg,
> > > + const char *name, const char **out)
If we can avoid points to pointers, then please do.
We already have ddata, so we can just set the name?
It will remove a lot of the following complexity / ugliness.
> > > +{
> > > + struct device *dev = &ddata->client->dev;
> > > + u8 buf[ASUSEC_ENTRY_BUFSIZE];
> > > + int ret;
> > > +
> > > + memset(buf, 0, ASUSEC_ENTRY_BUFSIZE);
> > > + ret = i2c_smbus_read_i2c_block_data(ddata->ec.dockram, reg,
> > > + ASUSEC_ENTRY_SIZE, buf);
> > > + if (ret < ASUSEC_ENTRY_SIZE)
> > > + return ret;
> >
> > Same here. These should be negative.
> >
>
> return ret < 0 ? ret : -EIO same as above
>
> > > +
> > > + if (buf[0] > ASUSEC_ENTRY_SIZE) {
> > > + dev_err(dev, "bad data len; buffer: %*ph; ret: %d\n",
> > > + ASUSEC_ENTRY_BUFSIZE, buf, ret);
> > > + return -EPROTO;
> > > + }
> > > +
> > > + if (!ddata->logging_disabled) {
> > > + dev_info(dev, "%-14s: %.*s\n", name, buf[0], buf + 1);
> > > +
> > > + if (out) {
> > > + *out = devm_kasprintf(dev, GFP_KERNEL, "%.*s",
> > > + buf[0], buf + 1);
> > > + if (!*out)
> > > + return -ENOMEM;
> > > + }
> > > + }
> >
> > FWIW, I hate this! What does it give you now that development is done?
> >
>
> We have already discussed this, and you agreed that EC and firmware
> prints may stay! This prints EC model and firmware info as well as EC
> firmware behavior. It allows identify possible new revisions of EC -
> Firmware combo and address possible regressions (check if it is chip
> malfunction or firmware needs a new programming model) without
> rebuilding kernel and digging downstream kernel for needed bits of
> code.
Right, so just print it out and remove all of the 'logging_disabled' and
'out' nonsense.
> > > + return 0;
> > > +}
> > > +
> > > +static int asus_ec_reset(struct asus_ec_data *ddata)
> > > +{
> > > + int retry, ret;
> > > +
> > > + guard(mutex)(&ddata->ecreq_lock);
> > > +
> > > + for (retry = 0; retry < ASUSEC_RETRY_MAX; retry++) {
> >
> > for (int retry = ... is generally preferred for throwaway variables.
> >
>
> Not that I care too much, but I am defining ret anyway, why not add
> retry too there?
This is the new and preferred way to use throw-away variables.
ret is not a throw-away variable.
[...]
> > > +static int asus_ec_set_factory_mode(struct asus_ec_data *ddata,
> > > + enum asusec_mode fmode)
> > > +{
> > > + dev_info(&ddata->client->dev, "Entering %s mode.\n",
> > > + fmode == ASUSEC_MODE_FACTORY ? "factory" : "normal");
> > > +
> > > + return asus_dockram_access_ctl(ddata->ec.dockram, NULL,
> > > + ASUSEC_CTL_FACTORY_MODE,
> > > + fmode == ASUSEC_MODE_FACTORY ?
> > > + ASUSEC_CTL_FACTORY_MODE : 0);
> >
> > Why not create make:
> >
> > ASUSEC_MODE_FACTORY == ASUSEC_CTL_FACTORY_MODE
> >
> > What happens to NORMAL?
> >
>
> ASUSEC_CTL_FACTORY_MODE is a bit in the ctl register. For NORMAL mode
I get that, but if the values can be shared, it make the code simpler.
> bit is cleared,
> for FACTORY bit it set, for NONE bit is ignored.
>
> > > +}
> > > +
> > > +static int asus_ec_detect(struct asus_ec_data *ddata)
> > > +{
> > > + int ret;
> > > +
> > > + ret = asus_ec_reset(ddata);
> > > + if (ret)
> > > + goto err_exit;
> > > +
> > > + asus_ec_clear_buffer(ddata);
> > > +
> > > + ret = asus_ec_log_info(ddata, ASUSEC_DOCKRAM_INFO_MODEL, "Model",
> > > + &ddata->ec.model);
Where is this model used?
> > You can use 100-chars and make the code look beautiful! :)
> >
>
> Not every subsystem permits 100 chars, some stick to 80 as a strict
> rule, so it is better be safe.
Right, but we are forward thinking here.
You can and should use 100-chars in this subsystem.
> > > + if (ret)
> > > + goto err_exit;
> > > +
> > > + ret = asus_ec_log_info(ddata, ASUSEC_DOCKRAM_INFO_FW, "FW version",
> > > + NULL);
> > > + if (ret)
> > > + goto err_exit;
> > > +
> > > + ret = asus_ec_log_info(ddata, ASUSEC_DOCKRAM_INFO_CFGFMT, "Config format",
> > > + NULL);
> > > + if (ret)
> > > + goto err_exit;
> > > +
> > > + ret = asus_ec_log_info(ddata, ASUSEC_DOCKRAM_INFO_HW, "HW version",
> > > + NULL);
> > > + if (ret)
> > > + goto err_exit;
> > > +
> > > + /* Disable logging on next EC request */
> >
> > Why, but why?
> >
>
> Cause EC requests are frequent (handshake/reset) and constant logging
> same data is not acceptable.
Then rid the prints entirely or do them at a more appropriate point like
during probe?
Or maybe consider dev_info_once() and friends.
> > > + ddata->logging_disabled = true;
> > > +
> > > + /* Check and inform about EC firmware behavior */
> > > + ret = asus_ec_susb_on_status(ddata);
> > > + if (ret)
> > > + goto err_exit;
> > > +
> > > + ddata->ec.name = ddata->info->name;
> > > +
> > > + /* Some EC require factory mode to be set normal on each request */
> > > + if (ddata->info->fmode)
> > > + ret = asus_ec_set_factory_mode(ddata, ddata->info->fmode);
> > > +
> > > +err_exit:
> > > + if (ret)
> > > + dev_err(&ddata->client->dev, "failed to access EC: %d\n", ret);
> > > +
> > > + return ret;
> > > +}
> > > +
> > > +static void asus_ec_handle_smi(struct asus_ec_data *ddata, unsigned int code)
> > > +{
> > > + switch (code) {
> > > + case ASUSEC_SMI_HANDSHAKE:
> > > + case ASUSEC_SMI_RESET:
> > > + asus_ec_detect(ddata);
> > > + break;
> > > + }
> > > +}
> > > +
> > > +static irqreturn_t asus_ec_interrupt(int irq, void *dev_id)
> > > +{
> > > + struct asus_ec_data *ddata = dev_id;
> > > + unsigned long notify_action;
> > > + int ret;
> > > +
> > > + ret = i2c_smbus_read_i2c_block_data(ddata->client, ASUSEC_READ_BUF,
> > > + ASUSEC_ENTRY_SIZE, ddata->ec_buf);
> > > + if (ret < ASUSEC_ENTRY_SIZE ||
> > > + !(ddata->ec_buf[ASUSEC_IRQ_STATUS] & ASUSEC_OBF_MASK))
> >
> > Unwrap for readability.
> >
> > Also, I think a comment would be helpful.
> >
>
> if (ret < ASUSEC_ENTRY_SIZE)
> return IRQ_NONE;
>
> ret = ddata->ec_buf[ASUSEC_IRQ_STATUS] & ASUSEC_OBF_MASK;
> if (!ret)
> return IRQ_NONE;
>
> This would be acceptable? (I will add comments later on)
Yes, better.
If you're not using ret again, you could just put 'ddata.." inside the if().
> > > + return IRQ_NONE;
> > > +
> > > + notify_action = ddata->ec_buf[ASUSEC_IRQ_STATUS];
> > > + if (notify_action & ASUSEC_SMI_MASK) {
> > > + unsigned int code = ddata->ec_buf[ASUSEC_SMI_CODE];
> > > +
> > > + asus_ec_handle_smi(ddata, code);
> > > +
> > > + notify_action |= code << 8;
> > > + }
> > > +
> > > + blocking_notifier_call_chain(&ddata->ec.notify_list,
> > > + notify_action, ddata->ec_buf);
> > > +
> > > + return IRQ_HANDLED;
> > > +}
> > > +
> > > +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 *ddata;
> > > + int ret;
> > > +
> > > + dockram = i2c_new_ancillary_device(parent, "dockram",
> > > + parent->addr + 2);
> >
> > Could we define a macro for the address offset '2' here to avoid using a magic
> > number?
> >
>
> It seems that you are excessively concerned with "magic numbers".
Bingo! I HATE magic numbers.
https://lore.kernel.org/all/?q=%22Lee+Jones%22+magic
~900 messages! =:-D
[...]
> > > +static const struct asus_ec_chip_info asus_ec_tf600t_pad_data = {
> > > + .name = "pad",
> > > + .variant = ASUSEC_TF600T_PAD,
> > > + .fmode = ASUSEC_MODE_NORMAL,
> > > +};
> >
> > Any reason not to just pass the identifier (variant) and add the name
> > and fmode attribues to the switch() above?
>
> Why not set it here, I am not passing any mfd or any other API via of data.
I get that, and you're not breaking any of my golden rules.
However, I just think doing everything in one place, usually a which
based off of the 'variant' which you pass as a single value, is a nicer,
more consolidated way of doing things.
--
Lee Jones
^ permalink raw reply
* Re: [PATCH] HID: lg-g15: cancel pending work on remove to fix a use-after-free
From: Hans de Goede @ 2026-06-18 10:28 UTC (permalink / raw)
To: Maoyi Xie, Jiri Kosina, Benjamin Tissoires; +Cc: linux-input, linux-kernel
In-Reply-To: <178176639579.3377656.12792307991044339915@maoyixie.com>
Hi,
On 18-Jun-26 09:06, Maoyi Xie wrote:
> lg_g15_data is allocated with devm and holds a work item. The report
> handlers schedule that work straight from device input.
> lg_g15_event() and lg_g15_v2_event() do it on the backlight cycle key,
> and lg_g510_leds_event() does it too. The worker dereferences the
> lg_g15_data back through container_of.
>
> The driver had no remove callback and never cancelled the work. So if a
> report scheduled the work and the keyboard was then unplugged, devres
> freed lg_g15_data while the work was still pending or running, and the
> worker touched freed memory. This is a use-after-free. It is reachable
> as a race on device unplug.
>
> Add a remove callback that cancels the work before devres frees the
> state. g15->work is only initialized for the models that schedule it
> (G15, G15 v2, G510). The G13 and Z-10 leave it zeroed, so guard the
> cancel on g15->work.func to avoid cancelling a work that was never set
> up. The g15 NULL test mirrors the one already in lg_g15_raw_event().
>
> Fixes: 97b741aba918 ("HID: lg-g15: Add keyboard and LCD backlight control")
> Cc: stable@vger.kernel.org
> Suggested-by: Hans de Goede <hansg@kernel.org>
> Signed-off-by: Maoyi Xie <maoyixie.tju@gmail.com>
Thanks, patch looks good to me:
Reviewed-by: Hans de Goede <johannes.goede@oss.qualcomm.com>
Regards,
Hans
> ---
> drivers/hid/hid-lg-g15.c | 16 ++++++++++++++++
> 1 file changed, 16 insertions(+)
>
> diff --git a/drivers/hid/hid-lg-g15.c b/drivers/hid/hid-lg-g15.c
> index 1a88bc44ada4..02ef3e2094b4 100644
> --- a/drivers/hid/hid-lg-g15.c
> +++ b/drivers/hid/hid-lg-g15.c
> @@ -1374,11 +1374,27 @@ static const struct hid_device_id lg_g15_devices[] = {
> };
> MODULE_DEVICE_TABLE(hid, lg_g15_devices);
>
> +static void lg_g15_remove(struct hid_device *hdev)
> +{
> + struct lg_g15_data *g15 = hid_get_drvdata(hdev);
> +
> + /*
> + * g15->work is only initialized for the models that schedule it
> + * (G15, G15 v2, G510). The G13 and Z-10 leave it zeroed, so only
> + * cancel it when it was set up.
> + */
> + if (g15 && g15->work.func)
> + cancel_work_sync(&g15->work);
> +
> + hid_hw_stop(hdev);
> +}
> +
> static struct hid_driver lg_g15_driver = {
> .name = "lg-g15",
> .id_table = lg_g15_devices,
> .raw_event = lg_g15_raw_event,
> .probe = lg_g15_probe,
> + .remove = lg_g15_remove,
> };
> module_hid_driver(lg_g15_driver);
>
^ permalink raw reply
* Re: [PATCH v8 4/7] input: keyboard: Add driver for ASUS Transformer dock multimedia keys
From: Svyatoslav Ryhel @ 2026-06-18 9:45 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Lee Jones,
Pavel Machek, Sebastian Reichel, Ion Agorria,
Michał Mirosław, devicetree, linux-kernel, linux-input,
linux-leds, linux-pm
In-Reply-To: <CAPVz0n0r-1SXH_dfS9HkQJrF7e-6+O5Me2bPjcscnizmfTfjZg@mail.gmail.com>
чт, 18 черв. 2026 р. о 12:18 Svyatoslav Ryhel <clamor95@gmail.com> пише:
>
> ср, 17 черв. 2026 р. о 00:23 Dmitry Torokhov <dmitry.torokhov@gmail.com> пише:
> >
> > On Tue, Jun 16, 2026 at 09:25:25AM +0300, Svyatoslav Ryhel wrote:
> > > вт, 16 черв. 2026 р. о 07:26 Dmitry Torokhov <dmitry.torokhov@gmail.com> пише:
> > > >
> > > > Hi Svyatoslav,
> > > >
> > > > On Thu, May 28, 2026 at 08:32:00AM +0300, Svyatoslav Ryhel wrote:
> > > > > From: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> > > > >
> > > > > Add support for multimedia top button row of ASUS Transformer's Mobile
> > > > > Dock keyboard. Driver is made that function keys (F1-F12) are used by
> > > > > default which suits average Linux use better and with pressing
> > > > > ScreenLock + AltGr function keys layout is switched to multimedia keys.
> > > > > Since this only modifies codes sent by asus-ec-keys it doesn't affect
> > > > > normal keyboards at all.
> > > >
> > > > I think using input handler to intercept ScreenLock + AltGr is quite
> > > > awkward. I think this also passes the original key events (unless you
> > > > make it a filter not a regular handler).
> > > >
> > > > I do not see benefit for reacting to AltGr+ScreenLock on other keyboards
> > > > to activate the special mode on this one. So given the fact that you
> > > > already mange the data stream when you split it into "serio" ports,
> > > > maybe just intercept this key combo right there and create the input
> > > > device and signal input events right there?
> > > >
> > >
> > > Though it seems awkward at a first glance, media keys are integrated
> > > with a standard keyboard in a detachable dock. It is highly unlikely
> > > that media keys will be used with a different keyboard then the one
> > > that is integrated with dock. Additionally, the ScreenLock key has a
> > > code specific to this driver and is not in general use, so even if any
> > > standard keyboard has AltGr but none has ScreenLock specific to this
> > > driver except the dock itself. Handler is also set as observer so it
> > > should not interfere with work of other input devices.
> >
> > I am not concerned about it interfering with other drivers, I am
> > concerned about it unnecessarily connecting to unrelated devices
> > (anything that declares EV_KEY).
> >
I can add check in asus_ec_input_connect() to strictly connect only to
dock keyboard, will this be sufficient?
> > Again, I think having input handler is not appropriate here. I would
> > fold this patch into the patch that introduces the 2 serio ports,
> > enhanced the data stream analysis to detect your key combo, and then
> > report through this new input device. You do not need to have the round
> > trip through atkbd and the new input handler for this.
This sounds like a load of additional work, I would rather remove
layout swapping entirely (even though it is a useful feature) then
rewrite everything from scratch.
> >
>
> I will try with filtering first if you don't mind. If that will not
> work I'll consider folding. Do you have any good examples of handler
> with filter to use as an inspiration? Thank you.
>
> > Thanks.
> >
> > --
> > Dmitry
^ permalink raw reply
* Re: [PATCH v8 4/7] input: keyboard: Add driver for ASUS Transformer dock multimedia keys
From: Svyatoslav Ryhel @ 2026-06-18 9:18 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Lee Jones,
Pavel Machek, Sebastian Reichel, Ion Agorria,
Michał Mirosław, devicetree, linux-kernel, linux-input,
linux-leds, linux-pm
In-Reply-To: <ajGyejCSRMhY4G2R@google.com>
ср, 17 черв. 2026 р. о 00:23 Dmitry Torokhov <dmitry.torokhov@gmail.com> пише:
>
> On Tue, Jun 16, 2026 at 09:25:25AM +0300, Svyatoslav Ryhel wrote:
> > вт, 16 черв. 2026 р. о 07:26 Dmitry Torokhov <dmitry.torokhov@gmail.com> пише:
> > >
> > > Hi Svyatoslav,
> > >
> > > On Thu, May 28, 2026 at 08:32:00AM +0300, Svyatoslav Ryhel wrote:
> > > > From: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> > > >
> > > > Add support for multimedia top button row of ASUS Transformer's Mobile
> > > > Dock keyboard. Driver is made that function keys (F1-F12) are used by
> > > > default which suits average Linux use better and with pressing
> > > > ScreenLock + AltGr function keys layout is switched to multimedia keys.
> > > > Since this only modifies codes sent by asus-ec-keys it doesn't affect
> > > > normal keyboards at all.
> > >
> > > I think using input handler to intercept ScreenLock + AltGr is quite
> > > awkward. I think this also passes the original key events (unless you
> > > make it a filter not a regular handler).
> > >
> > > I do not see benefit for reacting to AltGr+ScreenLock on other keyboards
> > > to activate the special mode on this one. So given the fact that you
> > > already mange the data stream when you split it into "serio" ports,
> > > maybe just intercept this key combo right there and create the input
> > > device and signal input events right there?
> > >
> >
> > Though it seems awkward at a first glance, media keys are integrated
> > with a standard keyboard in a detachable dock. It is highly unlikely
> > that media keys will be used with a different keyboard then the one
> > that is integrated with dock. Additionally, the ScreenLock key has a
> > code specific to this driver and is not in general use, so even if any
> > standard keyboard has AltGr but none has ScreenLock specific to this
> > driver except the dock itself. Handler is also set as observer so it
> > should not interfere with work of other input devices.
>
> I am not concerned about it interfering with other drivers, I am
> concerned about it unnecessarily connecting to unrelated devices
> (anything that declares EV_KEY).
>
> Again, I think having input handler is not appropriate here. I would
> fold this patch into the patch that introduces the 2 serio ports,
> enhanced the data stream analysis to detect your key combo, and then
> report through this new input device. You do not need to have the round
> trip through atkbd and the new input handler for this.
>
I will try with filtering first if you don't mind. If that will not
work I'll consider folding. Do you have any good examples of handler
with filter to use as an inspiration? Thank you.
> Thanks.
>
> --
> Dmitry
^ permalink raw reply
* [BUG] Bug report in HID: input: Add HID_BATTERY_QUIRK_DYNAMIC for Elan touchscreens
From: Eric Lin @ 2026-06-18 9:06 UTC (permalink / raw)
To: Jiri Kosina; +Cc: linux-input
Hi, I found a bug booting Linux 7.1 on my MSI Prestiage 16 Flip AI+,
when I unplugged the embedded stylus (MSI Nano Pen) from this laptop,
I got kernel OOPS.
Here are the relevant logs I got from `dmesg`
```
[ 44.523130] BUG: unable to handle page fault for address: ffffffffffffffe4
[ 44.523146] #PF: supervisor read access in kernel mode
[ 44.523151] #PF: error_code(0x0000) - not-present page
[ 44.523156] PGD 767025067 P4D 767025067 PUD 767027067 PMD 0
[ 44.523164] Oops: Oops: 0000 [#1] SMP NOPTI
[ 44.523172] CPU: 3 UID: 0 PID: 567 Comm: irq/108-CUST000 Not
tainted 7.1.0-1-mainline #1 PREEMPT(full)
4b7fa715112ff45ec03c02ca0cc4f3e6646a47e2
[ 44.523180] Hardware name: Micro-Star International Co., Ltd.
Prestige 16 Flip AI+ C3MTG/MS-2622, BIOS E2622IMS.117 04/27/2026
[ 44.523184] RIP: 0010:hidinput_hid_event+0x3b1/0xad0
[ 44.523197] Code: 1f 40 00 66 66 2e 0f 1f 84 00 00 00 00 00 66 66
2e 0f 1f 84 00 00 00 00 00 48 8b 46 38 48 8d 70 c8 48 39 c2 0f 84 ef
fd ff ff <3b> 4e 1c 75 ea 48 85 f6 0f 84 e1 fd ff ff 41 8b 00 3d 44 00
85 00
[ 44.523202] RSP: 0018:ffffcb8ac13f7cc8 EFLAGS: 00010282
[ 44.523208] RAX: 0000000000000000 RBX: ffff8b7b862d6000 RCX: 0000000000000007
[ 44.523212] RDX: ffff8b7b862d7c20 RSI: ffffffffffffffc8 RDI: ffff8b7b862d6000
[ 44.523215] RBP: 0000000000000001 R08: ffff8b7b88267288 R09: 0000000000000000
[ 44.523217] R10: ffff8b7baa294000 R11: ffff8b7b88267200 R12: ffff8b7b88267288
[ 44.523219] R13: ffff8b7b862d6000 R14: ffffffffc0954100 R15: ffff8b7b88267200
[ 44.523223] FS: 0000000000000000(0000) GS:ffff8b8348813000(0000)
knlGS:0000000000000000
[ 44.523227] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 44.523231] CR2: ffffffffffffffe4 CR3: 0000000767022005 CR4: 0000000108f72ef0
[ 44.523235] PKRU: 55555554
[ 44.523237] Call Trace:
[ 44.523243] <TASK>
[ 44.523256] hid_process_event+0x118/0x130
[ 44.523267] hid_report_raw_event+0x367/0x530
[ 44.523276] __hid_input_report+0x178/0x240
[ 44.523284] ? pm_wakeup_dev_event+0x30/0x70
[ 44.523294] ? __pfx_irq_thread_fn+0x10/0x10
[ 44.523305] hid_safe_input_report+0x14/0x20
[ 44.523316] i2c_hid_irq+0xcf/0x170 [i2c_hid
bde5e468c566fa9b3a9febe814b6fe7c454afeff]
[ 44.523327] irq_thread_fn+0x25/0x60
[ 44.523336] irq_thread+0x1cb/0x330
[ 44.523343] ? __pfx_irq_thread_dtor+0x10/0x10
[ 44.523351] ? __pfx_irq_thread+0x10/0x10
[ 44.523358] kthread+0xe1/0x120
[ 44.523367] ? __pfx_kthread+0x10/0x10
[ 44.523373] ret_from_fork+0x2a7/0x330
[ 44.523381] ? __pfx_kthread+0x10/0x10
[ 44.523386] ret_from_fork_asm+0x1a/0x30
[ 44.523398] </TASK>
[ 44.523400] Modules linked in: snd_seq_dummy rfcomm snd_hrtimer
snd_seq snd_seq_device ccm algif_aead des_generic libdes md4
nft_reject_inet nf_reject_ipv4 nf_reject_ipv6 nft_reject nft_limit
nft_ct nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 uhid algif_hash
algif_skcipher af_alg nf_tables bnep nfnetlink f2fs vfat
lz4hc_compress fat lz4_compress snd_ctl_led snd_soc_sof_sdw
snd_sof_probes snd_soc_intel_hda_dsp_common snd_soc_rt712_sdca
snd_soc_rt1320_sdw regmap_sdw_mbq snd_hda_codec_intelhdmi regmap_sdw
snd_hda_codec_hdmi snd_soc_dmic snd_hda_intel snd_sof_pci_intel_ptl
snd_sof_pci_intel_lnl snd_sof_pci_intel_mtl snd_sof_intel_hda_generic
soundwire_intel snd_sof_intel_hda_sdw_bpt snd_sof_intel_hda_common
snd_soc_hdac_hda snd_sof_intel_hda_mlink snd_sof_intel_hda
soundwire_cadence snd_sof_pci snd_sof_xtensa_dsp snd_sof snd_sof_utils
snd_hda_ext_core snd_hda_codec snd_hda_core snd_intel_dspcfg
intel_rapl_msr snd_intel_sdw_acpi intel_uncore_frequency
intel_uncore_frequency_common snd_soc_acpi_intel_match
intel_tcc_cooling
[ 44.523493] snd_soc_acpi_intel_sdca_quirks x86_pkg_temp_thermal
soundwire_generic_allocation intel_powerclamp snd_soc_sdw_utils
coretemp snd_soc_acpi snd_hwdep soundwire_bus uvcvideo iwlmld
kvm_intel spi_nor snd_soc_sdca iTCO_wdt videobuf2_vmalloc
intel_pmc_bxt mac80211 mtd snd_soc_core uvc
processor_thermal_device_pci mei_gsc_proxy snd_compress kvm ptp
videobuf2_memops videobuf2_v4l2 processor_thermal_device pps_core
ac97_bus processor_thermal_wt_hint videobuf2_common libarc4 irqbypass
snd_pcm_dmaengine intel_cstate platform_temperature_control snd_pcm
hid_sensor_gyro_3d hid_sensor_custom_intel_hinge hid_sensor_rotation
hid_sensor_accel_3d hid_sensor_prox hid_sensor_incl_3d hid_sensor_als
hid_sensor_magn_3d processor_thermal_soc_slider snd_timer i2c_i801
hid_sensor_trigger dptf_power intel_uncore int3403_thermal
intel_pmc_core btintel_pcie processor_thermal_rfim videodev iwlwifi
kfifo_buf snd i2c_smbus processor_thermal_rapl hid_sensor_iio_common
btintel soundcore spi_intel_pci pcspkr pmt_telemetry wmi_bmof mc
[ 44.523586] msi_wmi_platform msi_wmi i2c_mux crc8 industrialio
int3400_thermal intel_rapl_common spi_intel intel_hid pmt_discovery
soc_button_array bluetooth acpi_thermal_rel acpi_pad acpi_tad
sparse_keymap cfg80211 processor_thermal_wt_req pmt_class mei_me
processor_thermal_power_floor processor_thermal_mbox mei intel_vpu
intel_pmc_ssram_telemetry rfkill int340x_thermal_zone igen6_edac
joydev mousedev mac_hid dm_crypt encrypted_keys trusted asn1_encoder
tee hid_sensor_custom intel_ishtp_hid xe drm_gpuvm drm_gpusvm_helper
drm_buddy ucsi_acpi gpu_sched typec_ucsi drm_exec roles hid_multitouch
drm_suballoc_helper aesni_intel video typec drm_ttm_helper gf128mul
hid_sensor_hub aead ttm nvme i2c_algo_bit i2c_hid_acpi wmi i2c_hid
nvme_core drm_display_helper intel_lpss_pci intel_ish_ipc nvme_keyring
intel_lpss pinctrl_intel_platform nvme_auth thunderbolt intel_ishtp
cec intel_vsec idma64 serio_raw dm_mirror dm_region_hash dm_log dm_mod
i2c_dev crypto_user ec_sys pkcs8_key_parser
[ 44.523684] CR2: ffffffffffffffe4
[ 44.523689] ---[ end trace 0000000000000000 ]---
[ 44.523694] RIP: 0010:hidinput_hid_event+0x3b1/0xad0
[ 44.523699] Code: 1f 40 00 66 66 2e 0f 1f 84 00 00 00 00 00 66 66
2e 0f 1f 84 00 00 00 00 00 48 8b 46 38 48 8d 70 c8 48 39 c2 0f 84 ef
fd ff ff <3b> 4e 1c 75 ea 48 85 f6 0f 84 e1 fd ff ff 41 8b 00 3d 44 00
85 00
[ 44.523702] RSP: 0018:ffffcb8ac13f7cc8 EFLAGS: 00010282
[ 44.523705] RAX: 0000000000000000 RBX: ffff8b7b862d6000 RCX: 0000000000000007
[ 44.523708] RDX: ffff8b7b862d7c20 RSI: ffffffffffffffc8 RDI: ffff8b7b862d6000
[ 44.523710] RBP: 0000000000000001 R08: ffff8b7b88267288 R09: 0000000000000000
[ 44.523711] R10: ffff8b7baa294000 R11: ffff8b7b88267200 R12: ffff8b7b88267288
[ 44.523713] R13: ffff8b7b862d6000 R14: ffffffffc0954100 R15: ffff8b7b88267200
[ 44.523715] FS: 0000000000000000(0000) GS:ffff8b8348813000(0000)
knlGS:0000000000000000
[ 44.523718] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 44.523720] CR2: ffffffffffffffe4 CR3: 0000000767022005 CR4: 0000000108f72ef0
[ 44.523723] PKRU: 55555554
[ 44.523726] note: irq/108-CUST000[567] exited with irqs disabled
[ 44.523748] kernel tried to execute NX-protected page - exploit
attempt? (uid: 0)
[ 44.523751] BUG: unable to handle page fault for address: ffff8b7ba8969bc0
[ 44.523753] #PF: supervisor instruction fetch in kernel mode
[ 44.523756] #PF: error_code(0x0011) - permissions violation
[ 44.523758] PGD 768201067 P4D 768201067 PUD 87f7ff067 PMD 10a5a6063
PTE 8000000128969163
[ 44.523764] Oops: Oops: 0011 [#2] SMP NOPTI
[ 44.523769] CPU: 3 UID: 0 PID: 567 Comm: irq/108-CUST000 Tainted: G
D 7.1.0-1-mainline #1 PREEMPT(full)
4b7fa715112ff45ec03c02ca0cc4f3e6646a47e2
[ 44.523774] Tainted: [D]=DIE
[ 44.523776] Hardware name: Micro-Star International Co., Ltd.
Prestige 16 Flip AI+ C3MTG/MS-2622, BIOS E2622IMS.117 04/27/2026
[ 44.523778] RIP: 0010:0xffff8b7ba8969bc0
[ 44.523873] Code: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
00 00 00 <00> 00 08 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
03 00
[ 44.523876] RSP: 0018:ffffcb8ac13f7eb8 EFLAGS: 00010286
[ 44.523880] RAX: ffff8b7ba8969bc0 RBX: 96f2eb95fa502b00 RCX: 0000000000000000
[ 44.523883] RDX: 0000000000000001 RSI: 0000000000000000 RDI: ffffcb8ac13f7ea0
[ 44.523885] RBP: ffff8b7ba8969bc0 R08: 0000000000000000 R09: 0000000000000000
[ 44.523888] R10: ffffcb8ac13f7de8 R11: ffffcb8ac13f7de0 R12: 0000000000000009
[ 44.523891] R13: ffff8b7ba8970000 R14: ffff8b7ba8969b01 R15: 0000000000000000
[ 44.523893] FS: 0000000000000000(0000) GS:ffff8b8348813000(0000)
knlGS:0000000000000000
[ 44.523896] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 44.523899] CR2: ffff8b7ba8969bc0 CR3: 0000000767022005 CR4: 0000000108f72ef0
[ 44.523902] PKRU: 55555554
[ 44.523905] Call Trace:
[ 44.523909] <TASK>
[ 44.523911] ? task_work_run+0x66/0xa0
[ 44.523920] ? do_exit+0x300/0xba0
[ 44.523930] ? make_task_dead+0x8d/0x90
[ 44.523936] ? rewind_stack_and_make_dead+0x16/0x20
[ 44.523946] </TASK>
[ 44.523948] Modules linked in: snd_seq_dummy rfcomm snd_hrtimer
snd_seq snd_seq_device ccm algif_aead des_generic libdes md4
nft_reject_inet nf_reject_ipv4 nf_reject_ipv6 nft_reject nft_limit
nft_ct nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 uhid algif_hash
algif_skcipher af_alg nf_tables bnep nfnetlink f2fs vfat
lz4hc_compress fat lz4_compress snd_ctl_led snd_soc_sof_sdw
snd_sof_probes snd_soc_intel_hda_dsp_common snd_soc_rt712_sdca
snd_soc_rt1320_sdw regmap_sdw_mbq snd_hda_codec_intelhdmi regmap_sdw
snd_hda_codec_hdmi snd_soc_dmic snd_hda_intel snd_sof_pci_intel_ptl
snd_sof_pci_intel_lnl snd_sof_pci_intel_mtl snd_sof_intel_hda_generic
soundwire_intel snd_sof_intel_hda_sdw_bpt snd_sof_intel_hda_common
snd_soc_hdac_hda snd_sof_intel_hda_mlink snd_sof_intel_hda
soundwire_cadence snd_sof_pci snd_sof_xtensa_dsp snd_sof snd_sof_utils
snd_hda_ext_core snd_hda_codec snd_hda_core snd_intel_dspcfg
intel_rapl_msr snd_intel_sdw_acpi intel_uncore_frequency
intel_uncore_frequency_common snd_soc_acpi_intel_match
intel_tcc_cooling
[ 44.524031] snd_soc_acpi_intel_sdca_quirks x86_pkg_temp_thermal
soundwire_generic_allocation intel_powerclamp snd_soc_sdw_utils
coretemp snd_soc_acpi snd_hwdep soundwire_bus uvcvideo iwlmld
kvm_intel spi_nor snd_soc_sdca iTCO_wdt videobuf2_vmalloc
intel_pmc_bxt mac80211 mtd snd_soc_core uvc
processor_thermal_device_pci mei_gsc_proxy snd_compress kvm ptp
videobuf2_memops videobuf2_v4l2 processor_thermal_device pps_core
ac97_bus processor_thermal_wt_hint videobuf2_common libarc4 irqbypass
snd_pcm_dmaengine intel_cstate platform_temperature_control snd_pcm
hid_sensor_gyro_3d hid_sensor_custom_intel_hinge hid_sensor_rotation
hid_sensor_accel_3d hid_sensor_prox hid_sensor_incl_3d hid_sensor_als
hid_sensor_magn_3d processor_thermal_soc_slider snd_timer i2c_i801
hid_sensor_trigger dptf_power intel_uncore int3403_thermal
intel_pmc_core btintel_pcie processor_thermal_rfim videodev iwlwifi
kfifo_buf snd i2c_smbus processor_thermal_rapl hid_sensor_iio_common
btintel soundcore spi_intel_pci pcspkr pmt_telemetry wmi_bmof mc
[ 44.524118] msi_wmi_platform msi_wmi i2c_mux crc8 industrialio
int3400_thermal intel_rapl_common spi_intel intel_hid pmt_discovery
soc_button_array bluetooth acpi_thermal_rel acpi_pad acpi_tad
sparse_keymap cfg80211 processor_thermal_wt_req pmt_class mei_me
processor_thermal_power_floor processor_thermal_mbox mei intel_vpu
intel_pmc_ssram_telemetry rfkill int340x_thermal_zone igen6_edac
joydev mousedev mac_hid dm_crypt encrypted_keys trusted asn1_encoder
tee hid_sensor_custom intel_ishtp_hid xe drm_gpuvm drm_gpusvm_helper
drm_buddy ucsi_acpi gpu_sched typec_ucsi drm_exec roles hid_multitouch
drm_suballoc_helper aesni_intel video typec drm_ttm_helper gf128mul
hid_sensor_hub aead ttm nvme i2c_algo_bit i2c_hid_acpi wmi i2c_hid
nvme_core drm_display_helper intel_lpss_pci intel_ish_ipc nvme_keyring
intel_lpss pinctrl_intel_platform nvme_auth thunderbolt intel_ishtp
cec intel_vsec idma64 serio_raw dm_mirror dm_region_hash dm_log dm_mod
i2c_dev crypto_user ec_sys pkcs8_key_parser
[ 44.524222] CR2: ffff8b7ba8969bc0
[ 44.524225] ---[ end trace 0000000000000000 ]---
[ 44.524228] RIP: 0010:hidinput_hid_event+0x3b1/0xad0
[ 44.524233] Code: 1f 40 00 66 66 2e 0f 1f 84 00 00 00 00 00 66 66
2e 0f 1f 84 00 00 00 00 00 48 8b 46 38 48 8d 70 c8 48 39 c2 0f 84 ef
fd ff ff <3b> 4e 1c 75 ea 48 85 f6 0f 84 e1 fd ff ff 41 8b 00 3d 44 00
85 00
[ 44.524236] RSP: 0018:ffffcb8ac13f7cc8 EFLAGS: 00010282
[ 44.524240] RAX: 0000000000000000 RBX: ffff8b7b862d6000 RCX: 0000000000000007
[ 44.524243] RDX: ffff8b7b862d7c20 RSI: ffffffffffffffc8 RDI: ffff8b7b862d6000
[ 44.524246] RBP: 0000000000000001 R08: ffff8b7b88267288 R09: 0000000000000000
[ 44.524249] R10: ffff8b7baa294000 R11: ffff8b7b88267200 R12: ffff8b7b88267288
[ 44.524252] R13: ffff8b7b862d6000 R14: ffffffffc0954100 R15: ffff8b7b88267200
[ 44.524255] FS: 0000000000000000(0000) GS:ffff8b8348813000(0000)
knlGS:0000000000000000
[ 44.524258] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 44.524261] CR2: ffff8b7ba8969bc0 CR3: 0000000767022005 CR4: 0000000108f72ef0
[ 44.524264] PKRU: 55555554
[ 44.524267] note: irq/108-CUST000[567] exited with irqs disabled
[ 44.524294] Fixing recursive fault but reboot is needed!
```
^ permalink raw reply
* Re: [PATCH v4 4/4] Input: snvs_pwrkey - report press event in interrupt handler
From: sashiko-bot @ 2026-06-18 7:41 UTC (permalink / raw)
To: joy.zou; +Cc: Frank.Li, imx, dmitry.torokhov, linux-input
In-Reply-To: <20260618-b4-pwrkey-v4-4-4bfda105bdf3@oss.nxp.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The spinlock scope is too narrow, allowing the timer softirq and interrupt handler to interleave their event reporting, which can leave the key permanently stuck in the pressed state.
--
commit 115416424bcbaae3f95f1195f01f6e828f579e10
Author: Joy Zou <joy.zou@nxp.com>
Input: snvs_pwrkey - report press event in interrupt handler
This patch modifies the driver to report key press events directly in the
interrupt handler during system suspend to prevent event loss. It also adds a
spinlock to protect the keystate and suspended tracking flag.
> diff --git a/drivers/input/keyboard/snvs_pwrkey.c b/drivers/input/keyboard/snvs_pwrkey.c
> --- a/drivers/input/keyboard/snvs_pwrkey.c
> +++ b/drivers/input/keyboard/snvs_pwrkey.c
[ ... ]
> @@ -49,14 +51,21 @@ static void imx_imx_snvs_check_for_events(struct timer_list *t)
> struct pwrkey_drv_data *pdata = timer_container_of(pdata, t,
> check_timer);
> struct input_dev *input = pdata->input;
> + bool state_changed = false;
> u32 state;
>
> regmap_read(pdata->snvs, SNVS_HPSR_REG, &state);
> state = state & SNVS_HPSR_BTN ? 1 : 0;
>
> - /* only report new event if status changed */
> - if (state ^ pdata->keystate) {
> - pdata->keystate = state;
> + scoped_guard(spinlock_irqsave, &pdata->lock) {
> + /* only report new event if status changed */
> + if (state ^ pdata->keystate) {
> + pdata->keystate = state;
> + state_changed = true;
> + }
> + }
> +
> + if (state_changed) {
> input_event(input, EV_KEY, pdata->keycode, state);
[Severity: High]
Is the spinlock scope here too narrow?
Because the lock is dropped before calling input_event(), it seems the
interrupt handler could preempt this thread and reorder the event delivery
to the input core.
> input_sync(input);
> pm_relax(pdata->input->dev.parent);
[ ... ]
> @@ -92,6 +103,21 @@ static irqreturn_t imx_snvs_pwrkey_interrupt(int irq, void *dev_id)
> input_sync(input);
> pm_relax(input->dev.parent);
> } else {
> + /*
> + * Report key press events directly in interrupt handler to prevent event
> + * loss during system suspend.
> + */
> + scoped_guard(spinlock_irqsave, &pdata->lock) {
> + suspended = pdata->suspended;
> + if (suspended) {
> + pdata->keystate = 1;
> + keystate = pdata->keystate;
> + }
> + }
> + if (suspended) {
> + input_report_key(input, pdata->keycode, keystate);
[Severity: High]
Could dropping the lock before calling input_report_key() lead to a race
with the timer softirq emitting its own input events?
For example, if the timer softirq reads state as 0 in
imx_imx_snvs_check_for_events(), updates pdata->keystate to 0, and drops
the lock, an interrupt could then preempt it:
CPU0 (timer softirq)
scoped_guard(...) {
pdata->keystate = 0;
}
CPU0 (hardirq preempts before input_event is called)
scoped_guard(...) {
pdata->keystate = 1;
}
input_report_key(..., 1); // PRESS emitted
CPU0 (timer softirq resumes)
input_event(..., 0); // RELEASE emitted
In this scenario, the input core would receive PRESS followed by RELEASE,
leaving the final input state as RELEASE (0). However, pdata->keystate
remains PRESS (1).
Would this cause the next physical key press to be ignored since the timer
will see state=1, but the XOR against keystate (already 1) evaluates to 0,
effectively losing the event permanently?
> + input_sync(input);
> + }
> mod_timer(&pdata->check_timer,
> jiffies + msecs_to_jiffies(DEBOUNCE_TIME));
> }
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260618-b4-pwrkey-v4-0-4bfda105bdf3@oss.nxp.com?part=4
^ permalink raw reply
* Re: [PATCH v4 3/4] Input: snvs_pwrkey - use local device pointer to simple code
From: sashiko-bot @ 2026-06-18 7:39 UTC (permalink / raw)
To: joy.zou; +Cc: linux-input, imx, dmitry.torokhov, Frank.Li
In-Reply-To: <20260618-b4-pwrkey-v4-3-4bfda105bdf3@oss.nxp.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] Missing cleanup for device wakeup and wake IRQ on driver unbind causes memory leaks and re-bind warnings.
--
commit aed6a36738ed05979b6e5650a6efcf08cfb05a1d
Author: Joy Zou <joy.zou@nxp.com>
Input: snvs_pwrkey - use local device pointer to simple code
This patch introduces a local device pointer in imx_snvs_pwrkey_probe()
to simplify the code. It replaces repeated references to the platform device
pointer with the new local variable.
> diff --git a/drivers/input/keyboard/snvs_pwrkey.c b/drivers/input/keyboard/snvs_pwrkey.c
> --- a/drivers/input/keyboard/snvs_pwrkey.c
> +++ b/drivers/input/keyboard/snvs_pwrkey.c
[ ... ]
> @@ -191,27 +192,27 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
[ ... ]
> - device_init_wakeup(&pdev->dev, pdata->wakeup);
> - error = dev_pm_set_wake_irq(&pdev->dev, pdata->irq);
> + device_init_wakeup(dev, pdata->wakeup);
> + error = dev_pm_set_wake_irq(dev, pdata->irq);
[Severity: High]
This is a pre-existing issue, but since there is no remove callback in this
driver, does this leak the wake IRQ allocation when the driver is unbound?
When the driver is unbound via sysfs, the wake_irq structure allocated by
dev_pm_set_wake_irq() seems to remain attached to dev->power.wakeirq.
If the driver is subsequently rebound, will dev_pm_set_wake_irq() fail with
-EEXIST and trigger a kernel warning about the wake irq already being
initialized?
Furthermore, because device_del() does not automatically free
dev->power.wakeirq, could this memory permanently leak when the platform
device is destroyed?
Would it be better to use devm_pm_set_wake_irq() here to ensure proper
cleanup?
> if (error)
> - dev_err(&pdev->dev, "irq wake enable failed.\n");
> + dev_err(dev, "irq wake enable failed.\n");
>
> return 0;
> }
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260618-b4-pwrkey-v4-0-4bfda105bdf3@oss.nxp.com?part=3
^ permalink raw reply
* Re: [PATCH v4 1/4] Input: snvs_pwrkey - make use of dev_err_probe()
From: sashiko-bot @ 2026-06-18 7:38 UTC (permalink / raw)
To: joy.zou; +Cc: linux-input, Frank.Li, imx, dmitry.torokhov
In-Reply-To: <20260618-b4-pwrkey-v4-1-4bfda105bdf3@oss.nxp.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [Medium] Memory leak of struct wake_irq on device unbind
--
commit 469e290fcf154d6638767e1e60cb2427638bea98
Author: Joy Zou <joy.zou@nxp.com>
Input: snvs_pwrkey - make use of dev_err_probe()
This commit updates the probe function of the snvs_pwrkey driver to
use dev_err_probe() for error handling. This simplifies the code and
improves logging for deferred probes.
> diff --git a/drivers/input/keyboard/snvs_pwrkey.c b/drivers/input/keyboard/snvs_pwrkey.c
> index 954055aaf6e29..8cc6863d26ed8 100644
> --- a/drivers/input/keyboard/snvs_pwrkey.c
> +++ b/drivers/input/keyboard/snvs_pwrkey.c
[ ... ]
> @@ -209,16 +201,12 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
> error = devm_request_irq(&pdev->dev, pdata->irq,
> imx_snvs_pwrkey_interrupt,
> 0, pdev->name, pdev);
> - if (error) {
> - dev_err(&pdev->dev, "interrupt not available.\n");
> - return error;
> - }
> + if (error)
> + return dev_err_probe(&pdev->dev, error, "interrupt not available.\n");
>
> error = input_register_device(input);
> - if (error < 0) {
> - dev_err(&pdev->dev, "failed to register input device\n");
> - return error;
> - }
> + if (error < 0)
> + return dev_err_probe(&pdev->dev, error, "failed to register input device\n");
>
> device_init_wakeup(&pdev->dev, pdata->wakeup);
> error = dev_pm_set_wake_irq(&pdev->dev, pdata->irq);
[Severity: Medium]
This is a pre-existing issue, but does this dev_pm_set_wake_irq() allocation
leak when the driver is unbound?
Because this driver relies entirely on devres for resource management and
does not implement a .remove callback, there is no explicit call to
dev_pm_clear_wake_irq() upon driver unbind.
The device core does not automatically free dev->power.wakeirq during
device_pm_remove(), so this allocated wake_irq structure appears to leak
every time the driver is unbound.
Could this be fixed by switching to the managed variant,
devm_pm_set_wake_irq()?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260618-b4-pwrkey-v4-0-4bfda105bdf3@oss.nxp.com?part=1
^ permalink raw reply
* [PATCH v4 4/4] Input: snvs_pwrkey - report press event in interrupt handler
From: joy.zou @ 2026-06-18 7:31 UTC (permalink / raw)
To: Dmitry Torokhov, Frank Li, Bough Chen, Peng Fan, Jacky Bai, Ye Li
Cc: Joy Zou, imx, linux-input, linux-kernel, Joy Zou
In-Reply-To: <20260618-b4-pwrkey-v4-0-4bfda105bdf3@oss.nxp.com>
From: Joy Zou <joy.zou@nxp.com>
The driver implements debounce protection using a timer-based mechanism:
when a key interrupt occurs, a timer is scheduled to verify the key state
after DEBOUNCE_TIME before reporting the event. This works well during
normal operation.
However, key press events can be lost during system resume on platforms
like i.MX8MQ-EVK because:
1. During the no_irq resume phase, PCIe driver restoration can take up to
200ms with IRQs disabled.
2. The power key interrupt remains pending during the no_irq phase.
3. If the key is released before IRQs are re-enabled, the timer eventually
runs but sees the key as released and skips reporting the event.
Report key press events directly in interrupt handler to prevent event
loss during system suspend. This is safe because:
1. Only one event is reported per suspend cycle.
2. Normal operation retains the existing timer-based debounce mechanism.
Signed-off-by: Joy Zou <joy.zou@nxp.com>
---
Changes for v3:
1. Add spinlock for pdata->keystate and pdata->suspended per AI review
comments.
2. Replace hardcode value 1 with local variable keystate in input_report_key()
under suspended.
Changes for v2:
1. Add a boolean variable suspended and PM callback functions to replace
the use of the is_suspended field per AI review comments.
2. Move event report handle to else branch in suspended state, since the
pdata->minor_rev == 0 branch has no debounce detection per AI review
comments.
3. Modify the commit message.
---
drivers/input/keyboard/snvs_pwrkey.c | 60 ++++++++++++++++++++++++++++++++++--
1 file changed, 57 insertions(+), 3 deletions(-)
diff --git a/drivers/input/keyboard/snvs_pwrkey.c b/drivers/input/keyboard/snvs_pwrkey.c
index cbe44a38d2b3..98c33d0bb1ec 100644
--- a/drivers/input/keyboard/snvs_pwrkey.c
+++ b/drivers/input/keyboard/snvs_pwrkey.c
@@ -39,6 +39,8 @@ struct pwrkey_drv_data {
int keycode;
int keystate; /* 1:pressed */
int wakeup;
+ bool suspended; /* Track suspend state */
+ spinlock_t lock; /* Protects keystate and suspended */
struct timer_list check_timer;
struct input_dev *input;
u8 minor_rev;
@@ -49,14 +51,21 @@ static void imx_imx_snvs_check_for_events(struct timer_list *t)
struct pwrkey_drv_data *pdata = timer_container_of(pdata, t,
check_timer);
struct input_dev *input = pdata->input;
+ bool state_changed = false;
u32 state;
regmap_read(pdata->snvs, SNVS_HPSR_REG, &state);
state = state & SNVS_HPSR_BTN ? 1 : 0;
- /* only report new event if status changed */
- if (state ^ pdata->keystate) {
- pdata->keystate = state;
+ scoped_guard(spinlock_irqsave, &pdata->lock) {
+ /* only report new event if status changed */
+ if (state ^ pdata->keystate) {
+ pdata->keystate = state;
+ state_changed = true;
+ }
+ }
+
+ if (state_changed) {
input_event(input, EV_KEY, pdata->keycode, state);
input_sync(input);
pm_relax(pdata->input->dev.parent);
@@ -74,6 +83,8 @@ static irqreturn_t imx_snvs_pwrkey_interrupt(int irq, void *dev_id)
struct platform_device *pdev = dev_id;
struct pwrkey_drv_data *pdata = platform_get_drvdata(pdev);
struct input_dev *input = pdata->input;
+ bool suspended;
+ int keystate;
u32 lp_status;
pm_wakeup_event(input->dev.parent, 0);
@@ -92,6 +103,21 @@ static irqreturn_t imx_snvs_pwrkey_interrupt(int irq, void *dev_id)
input_sync(input);
pm_relax(input->dev.parent);
} else {
+ /*
+ * Report key press events directly in interrupt handler to prevent event
+ * loss during system suspend.
+ */
+ scoped_guard(spinlock_irqsave, &pdata->lock) {
+ suspended = pdata->suspended;
+ if (suspended) {
+ pdata->keystate = 1;
+ keystate = pdata->keystate;
+ }
+ }
+ if (suspended) {
+ input_report_key(input, pdata->keycode, keystate);
+ input_sync(input);
+ }
mod_timer(&pdata->check_timer,
jiffies + msecs_to_jiffies(DEBOUNCE_TIME));
}
@@ -151,6 +177,7 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
if (pdata->irq < 0)
return pdata->irq;
+ spin_lock_init(&pdata->lock);
error = of_property_read_u32(np, "power-off-time-sec", &val);
if (!error) {
switch (val) {
@@ -217,6 +244,32 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
return 0;
}
+static int __maybe_unused imx_snvs_pwrkey_suspend(struct device *dev)
+{
+ struct platform_device *pdev = to_platform_device(dev);
+ struct pwrkey_drv_data *pdata = platform_get_drvdata(pdev);
+
+ scoped_guard(spinlock_irqsave, &pdata->lock)
+ pdata->suspended = true;
+
+ return 0;
+}
+
+static int __maybe_unused imx_snvs_pwrkey_resume(struct device *dev)
+{
+ struct platform_device *pdev = to_platform_device(dev);
+ struct pwrkey_drv_data *pdata = platform_get_drvdata(pdev);
+
+ scoped_guard(spinlock_irqsave, &pdata->lock)
+ pdata->suspended = false;
+
+ return 0;
+}
+
+static SIMPLE_DEV_PM_OPS(imx_snvs_pwrkey_pm_ops,
+ imx_snvs_pwrkey_suspend,
+ imx_snvs_pwrkey_resume);
+
static const struct of_device_id imx_snvs_pwrkey_ids[] = {
{ .compatible = "fsl,sec-v4.0-pwrkey" },
{ /* sentinel */ }
@@ -227,6 +280,7 @@ static struct platform_driver imx_snvs_pwrkey_driver = {
.driver = {
.name = "snvs_pwrkey",
.of_match_table = imx_snvs_pwrkey_ids,
+ .pm = &imx_snvs_pwrkey_pm_ops,
},
.probe = imx_snvs_pwrkey_probe,
};
--
2.34.1
^ permalink raw reply related
* [PATCH v4 3/4] Input: snvs_pwrkey - use local device pointer to simple code
From: joy.zou @ 2026-06-18 7:31 UTC (permalink / raw)
To: Dmitry Torokhov, Frank Li, Bough Chen, Peng Fan, Jacky Bai, Ye Li
Cc: Joy Zou, imx, linux-input, linux-kernel, Joy Zou
In-Reply-To: <20260618-b4-pwrkey-v4-0-4bfda105bdf3@oss.nxp.com>
From: Joy Zou <joy.zou@nxp.com>
Use local struct device pointer to avoid reference the platform_device
pointer every time.
No functional change.
Reviewed-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Joy Zou <joy.zou@nxp.com>
---
Changes for v4:
1. Add Reviewed-by tag.
Changes for v2:
1. Use dev instead of &pdev->dev for devm_input_allocate_device(),
which was missed in patch v1 per AI review comments.
2. Modify commit message.
---
drivers/input/keyboard/snvs_pwrkey.c | 41 ++++++++++++++++++------------------
1 file changed, 21 insertions(+), 20 deletions(-)
diff --git a/drivers/input/keyboard/snvs_pwrkey.c b/drivers/input/keyboard/snvs_pwrkey.c
index d58bbbe9fd58..cbe44a38d2b3 100644
--- a/drivers/input/keyboard/snvs_pwrkey.c
+++ b/drivers/input/keyboard/snvs_pwrkey.c
@@ -112,6 +112,7 @@ static void imx_snvs_pwrkey_act(void *pdata)
static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
{
+ struct device *dev = &pdev->dev;
struct pwrkey_drv_data *pdata;
struct input_dev *input;
struct device_node *np;
@@ -122,26 +123,26 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
u32 vid;
/* Get SNVS register Page */
- np = pdev->dev.of_node;
+ np = dev->of_node;
if (!np)
- return dev_err_probe(&pdev->dev, -ENODEV, "Device tree node not found\n");
+ return dev_err_probe(dev, -ENODEV, "Device tree node not found\n");
- pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
+ pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
if (!pdata)
return -ENOMEM;
pdata->snvs = syscon_regmap_lookup_by_phandle(np, "regmap");
if (IS_ERR(pdata->snvs))
- return dev_err_probe(&pdev->dev, PTR_ERR(pdata->snvs), "Can't get snvs syscon\n");
+ return dev_err_probe(dev, PTR_ERR(pdata->snvs), "Can't get snvs syscon\n");
if (of_property_read_u32(np, "linux,keycode", &pdata->keycode)) {
pdata->keycode = KEY_POWER;
- dev_warn(&pdev->dev, "KEY_POWER without setting in dts\n");
+ dev_warn(dev, "KEY_POWER without setting in dts\n");
}
- clk = devm_clk_get_optional_enabled(&pdev->dev, NULL);
+ clk = devm_clk_get_optional_enabled(dev, NULL);
if (IS_ERR(clk))
- return dev_err_probe(&pdev->dev, PTR_ERR(clk),
+ return dev_err_probe(dev, PTR_ERR(clk),
"Failed to get snvs clock (%pe)\n", clk);
pdata->wakeup = of_property_read_bool(np, "wakeup-source");
@@ -162,7 +163,7 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
bpt = (val / 5) - 1;
break;
default:
- return dev_err_probe(&pdev->dev, -EINVAL,
+ return dev_err_probe(dev, -EINVAL,
"power-off-time-sec %d out of range\n", val);
}
@@ -180,9 +181,9 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
timer_setup(&pdata->check_timer, imx_imx_snvs_check_for_events, 0);
- input = devm_input_allocate_device(&pdev->dev);
+ input = devm_input_allocate_device(dev);
if (!input)
- return dev_err_probe(&pdev->dev, -ENOMEM, "failed to allocate the input device\n");
+ return dev_err_probe(dev, -ENOMEM, "failed to allocate the input device\n");
input->name = pdev->name;
input->phys = "snvs-pwrkey/input0";
@@ -191,27 +192,27 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
input_set_capability(input, EV_KEY, pdata->keycode);
/* input customer action to cancel release timer */
- error = devm_add_action(&pdev->dev, imx_snvs_pwrkey_act, pdata);
+ error = devm_add_action(dev, imx_snvs_pwrkey_act, pdata);
if (error)
- return dev_err_probe(&pdev->dev, error, "failed to register remove action\n");
+ return dev_err_probe(dev, error, "failed to register remove action\n");
pdata->input = input;
platform_set_drvdata(pdev, pdata);
- error = devm_request_irq(&pdev->dev, pdata->irq,
- imx_snvs_pwrkey_interrupt,
- 0, pdev->name, pdev);
+ error = devm_request_irq(dev, pdata->irq,
+ imx_snvs_pwrkey_interrupt,
+ 0, pdev->name, pdev);
if (error)
- return dev_err_probe(&pdev->dev, error, "interrupt not available.\n");
+ return dev_err_probe(dev, error, "interrupt not available.\n");
error = input_register_device(input);
if (error < 0)
- return dev_err_probe(&pdev->dev, error, "failed to register input device\n");
+ return dev_err_probe(dev, error, "failed to register input device\n");
- device_init_wakeup(&pdev->dev, pdata->wakeup);
- error = dev_pm_set_wake_irq(&pdev->dev, pdata->irq);
+ device_init_wakeup(dev, pdata->wakeup);
+ error = dev_pm_set_wake_irq(dev, pdata->irq);
if (error)
- dev_err(&pdev->dev, "irq wake enable failed.\n");
+ dev_err(dev, "irq wake enable failed.\n");
return 0;
}
--
2.34.1
^ permalink raw reply related
* [PATCH v4 2/4] Input: snvs_pwrkey - propagate error code of platform_get_irq()
From: joy.zou @ 2026-06-18 7:31 UTC (permalink / raw)
To: Dmitry Torokhov, Frank Li, Bough Chen, Peng Fan, Jacky Bai, Ye Li
Cc: Joy Zou, imx, linux-input, linux-kernel, Joy Zou
In-Reply-To: <20260618-b4-pwrkey-v4-0-4bfda105bdf3@oss.nxp.com>
From: Joy Zou <joy.zou@nxp.com>
Hardcoding -EINVAL discards the actual error code, which breaks probe
deferral (-EPROBE_DEFER) and loses critical diagnostic information
needed for proper kernel error handling.
Reviewed-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Joy Zou <joy.zou@nxp.com>
---
Changes in v4:
1. modify the subject description.
2. add Reviewed-by tag.
---
drivers/input/keyboard/snvs_pwrkey.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/input/keyboard/snvs_pwrkey.c b/drivers/input/keyboard/snvs_pwrkey.c
index 8cc6863d26ed..d58bbbe9fd58 100644
--- a/drivers/input/keyboard/snvs_pwrkey.c
+++ b/drivers/input/keyboard/snvs_pwrkey.c
@@ -148,7 +148,7 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
pdata->irq = platform_get_irq(pdev, 0);
if (pdata->irq < 0)
- return -EINVAL;
+ return pdata->irq;
error = of_property_read_u32(np, "power-off-time-sec", &val);
if (!error) {
--
2.34.1
^ permalink raw reply related
* [PATCH v4 1/4] Input: snvs_pwrkey - make use of dev_err_probe()
From: joy.zou @ 2026-06-18 7:31 UTC (permalink / raw)
To: Dmitry Torokhov, Frank Li, Bough Chen, Peng Fan, Jacky Bai, Ye Li
Cc: Joy Zou, imx, linux-input, linux-kernel, Joy Zou
In-Reply-To: <20260618-b4-pwrkey-v4-0-4bfda105bdf3@oss.nxp.com>
From: Joy Zou <joy.zou@nxp.com>
Add dev_err_probe() at return path of probe() to support users to
identify issues easier.
Reviewed-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Joy Zou <joy.zou@nxp.com>
---
Changes in v4:
1. Use dev_err_probe() for devm_input_allocate_device() to simplify
error handling.
Changes in v2:
1. Drop dev_err_probe() change for platform_get_irq() per AI review comments.
2. Add Reviewed-by tag.
---
drivers/input/keyboard/snvs_pwrkey.c | 44 +++++++++++++-----------------------
1 file changed, 16 insertions(+), 28 deletions(-)
diff --git a/drivers/input/keyboard/snvs_pwrkey.c b/drivers/input/keyboard/snvs_pwrkey.c
index 954055aaf6e2..8cc6863d26ed 100644
--- a/drivers/input/keyboard/snvs_pwrkey.c
+++ b/drivers/input/keyboard/snvs_pwrkey.c
@@ -124,17 +124,15 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
/* Get SNVS register Page */
np = pdev->dev.of_node;
if (!np)
- return -ENODEV;
+ return dev_err_probe(&pdev->dev, -ENODEV, "Device tree node not found\n");
pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
if (!pdata)
return -ENOMEM;
pdata->snvs = syscon_regmap_lookup_by_phandle(np, "regmap");
- if (IS_ERR(pdata->snvs)) {
- dev_err(&pdev->dev, "Can't get snvs syscon\n");
- return PTR_ERR(pdata->snvs);
- }
+ if (IS_ERR(pdata->snvs))
+ return dev_err_probe(&pdev->dev, PTR_ERR(pdata->snvs), "Can't get snvs syscon\n");
if (of_property_read_u32(np, "linux,keycode", &pdata->keycode)) {
pdata->keycode = KEY_POWER;
@@ -142,10 +140,9 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
}
clk = devm_clk_get_optional_enabled(&pdev->dev, NULL);
- if (IS_ERR(clk)) {
- dev_err(&pdev->dev, "Failed to get snvs clock (%pe)\n", clk);
- return PTR_ERR(clk);
- }
+ if (IS_ERR(clk))
+ return dev_err_probe(&pdev->dev, PTR_ERR(clk),
+ "Failed to get snvs clock (%pe)\n", clk);
pdata->wakeup = of_property_read_bool(np, "wakeup-source");
@@ -165,9 +162,8 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
bpt = (val / 5) - 1;
break;
default:
- dev_err(&pdev->dev,
- "power-off-time-sec %d out of range\n", val);
- return -EINVAL;
+ return dev_err_probe(&pdev->dev, -EINVAL,
+ "power-off-time-sec %d out of range\n", val);
}
regmap_update_bits(pdata->snvs, SNVS_LPCR_REG, SNVS_LPCR_BPT_MASK,
@@ -185,10 +181,8 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
timer_setup(&pdata->check_timer, imx_imx_snvs_check_for_events, 0);
input = devm_input_allocate_device(&pdev->dev);
- if (!input) {
- dev_err(&pdev->dev, "failed to allocate the input device\n");
- return -ENOMEM;
- }
+ if (!input)
+ return dev_err_probe(&pdev->dev, -ENOMEM, "failed to allocate the input device\n");
input->name = pdev->name;
input->phys = "snvs-pwrkey/input0";
@@ -198,10 +192,8 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
/* input customer action to cancel release timer */
error = devm_add_action(&pdev->dev, imx_snvs_pwrkey_act, pdata);
- if (error) {
- dev_err(&pdev->dev, "failed to register remove action\n");
- return error;
- }
+ if (error)
+ return dev_err_probe(&pdev->dev, error, "failed to register remove action\n");
pdata->input = input;
platform_set_drvdata(pdev, pdata);
@@ -209,16 +201,12 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
error = devm_request_irq(&pdev->dev, pdata->irq,
imx_snvs_pwrkey_interrupt,
0, pdev->name, pdev);
- if (error) {
- dev_err(&pdev->dev, "interrupt not available.\n");
- return error;
- }
+ if (error)
+ return dev_err_probe(&pdev->dev, error, "interrupt not available.\n");
error = input_register_device(input);
- if (error < 0) {
- dev_err(&pdev->dev, "failed to register input device\n");
- return error;
- }
+ if (error < 0)
+ return dev_err_probe(&pdev->dev, error, "failed to register input device\n");
device_init_wakeup(&pdev->dev, pdata->wakeup);
error = dev_pm_set_wake_irq(&pdev->dev, pdata->irq);
--
2.34.1
^ permalink raw reply related
* [PATCH v4 0/4] snvs_pwrkey - code improvements and add report event
From: joy.zou @ 2026-06-18 7:31 UTC (permalink / raw)
To: Dmitry Torokhov, Frank Li, Bough Chen, Peng Fan, Jacky Bai, Ye Li
Cc: Joy Zou, imx, linux-input, linux-kernel, Joy Zou
This patch series improves the snvs_pwrkey driver with better code quality
and add report press event.
The main improvements include:
1. Clean up the code by using local device pointers and dev_err_probe() for
better readability and easier debugging.
2. Fix potential event loss during system suspend by reporting key press events
directly in the interrupt handler.
Signed-off-by: Joy Zou <joy.zou@oss.nxp.com>
---
Changes in v4:
- Use dev_err_probe() for devm_input_allocate_device() to simplify error handling
for patch #1.
- Add Reviewed-by tag for patch #2 and #3.
- Modify the subject description for patch #2.
- Link to v3: https://patch.msgid.link/20260615-b4-pwrkey-v3-0-9510b1173f6e@oss.nxp.com
Changes in v3:
- Add spinlock for pdata->keystate and pdata->suspended per AI review comments.
- Replace hardcode value 1 with local variable keystate in input_report_key()
under suspended.
- Link to v2: https://lore.kernel.org/r/20260604-pwrkey-cleanup-v2-0-59fa0ddf0d7c@nxp.com
Changes in v2:
- Drop dev_err_probe() change for platform_get_irq() per AI review comments.
- Add new patch #2 replace hardcoding -EINVAL with original error code per
AI review comments.
- Use dev instead of &pdev->dev for devm_input_allocate_device(), which was
missed in patch v1 per AI review comments.
- Add a boolean variable suspended and PM callback functions to replace
the use of the is_suspended field per AI review comments.
- Move event report handle to else branch in suspended state, since the
pdata->minor_rev == 0 branch has no debounce detection per AI review
comments.
- Modify patch #3 and #4 commit message.
- Add Reviewed-by tag for patch #1.
- Link to v1: https://lore.kernel.org/r/20260326-pwrkey-cleanup-v1-0-d85d7c0bf275@nxp.com
To: Dmitry Torokhov <dmitry.torokhov@gmail.com>
To: Frank Li <Frank.Li@nxp.com>
To: Bough Chen <haibo.chen@nxp.com>
To: Peng Fan <peng.fan@nxp.com>
To: Jacky Bai <ping.bai@nxp.com>
To: Ye Li <ye.li@nxp.com>
Cc: imx@lists.linux.dev
Cc: linux-input@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
Joy Zou (4):
Input: snvs_pwrkey - make use of dev_err_probe()
Input: snvs_pwrkey - propagate error code of platform_get_irq()
Input: snvs_pwrkey - use local device pointer to simple code
Input: snvs_pwrkey - report press event in interrupt handler
drivers/input/keyboard/snvs_pwrkey.c | 131 +++++++++++++++++++++++------------
1 file changed, 87 insertions(+), 44 deletions(-)
---
base-commit: f7af91adc230aa99e23330ecf85bc9badd9780ad
change-id: 20260615-b4-pwrkey-a60b2043cd29
Best regards,
--
Joy Zou <joy.zou@oss.nxp.com>
^ permalink raw reply
* [PATCH] HID: lg-g15: cancel pending work on remove to fix a use-after-free
From: Maoyi Xie @ 2026-06-18 7:06 UTC (permalink / raw)
To: Hans de Goede, Jiri Kosina, Benjamin Tissoires; +Cc: linux-input, linux-kernel
lg_g15_data is allocated with devm and holds a work item. The report
handlers schedule that work straight from device input.
lg_g15_event() and lg_g15_v2_event() do it on the backlight cycle key,
and lg_g510_leds_event() does it too. The worker dereferences the
lg_g15_data back through container_of.
The driver had no remove callback and never cancelled the work. So if a
report scheduled the work and the keyboard was then unplugged, devres
freed lg_g15_data while the work was still pending or running, and the
worker touched freed memory. This is a use-after-free. It is reachable
as a race on device unplug.
Add a remove callback that cancels the work before devres frees the
state. g15->work is only initialized for the models that schedule it
(G15, G15 v2, G510). The G13 and Z-10 leave it zeroed, so guard the
cancel on g15->work.func to avoid cancelling a work that was never set
up. The g15 NULL test mirrors the one already in lg_g15_raw_event().
Fixes: 97b741aba918 ("HID: lg-g15: Add keyboard and LCD backlight control")
Cc: stable@vger.kernel.org
Suggested-by: Hans de Goede <hansg@kernel.org>
Signed-off-by: Maoyi Xie <maoyixie.tju@gmail.com>
---
drivers/hid/hid-lg-g15.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/drivers/hid/hid-lg-g15.c b/drivers/hid/hid-lg-g15.c
index 1a88bc44ada4..02ef3e2094b4 100644
--- a/drivers/hid/hid-lg-g15.c
+++ b/drivers/hid/hid-lg-g15.c
@@ -1374,11 +1374,27 @@ static const struct hid_device_id lg_g15_devices[] = {
};
MODULE_DEVICE_TABLE(hid, lg_g15_devices);
+static void lg_g15_remove(struct hid_device *hdev)
+{
+ struct lg_g15_data *g15 = hid_get_drvdata(hdev);
+
+ /*
+ * g15->work is only initialized for the models that schedule it
+ * (G15, G15 v2, G510). The G13 and Z-10 leave it zeroed, so only
+ * cancel it when it was set up.
+ */
+ if (g15 && g15->work.func)
+ cancel_work_sync(&g15->work);
+
+ hid_hw_stop(hdev);
+}
+
static struct hid_driver lg_g15_driver = {
.name = "lg-g15",
.id_table = lg_g15_devices,
.raw_event = lg_g15_raw_event,
.probe = lg_g15_probe,
+ .remove = lg_g15_remove,
};
module_hid_driver(lg_g15_driver);
--
2.34.1
^ permalink raw reply related
* [PATCH] HID: logitech-dj: Fix maxfield check in DJ short report validation
From: HyeongJun An @ 2026-06-18 6:37 UTC (permalink / raw)
To: Benjamin Tissoires, Jiri Kosina
Cc: Filipe Laíns, Lee Jones, linux-input, linux-kernel,
HyeongJun An, stable
Commit b6a57912854e ("HID: logitech-dj: Prevent REPORT_ID_DJ_SHORT
related user initiated OOB write") added validation for the DJ short
output report, but the error path dereferences rep->field[0] even when
rep->maxfield is zero.
Commit 8b9a097eb2fc ("HID: logitech-dj: fix wrong detection of bad
DJ_SHORT output report") made the check conditional on rep being present,
but a crafted descriptor can still create report ID 0x20 with only padding
output items. hid-core registers the report, ignores the padding field,
and leaves rep->maxfield as zero.
In that case the validation enters the rep->maxfield < 1 branch and then
dereferences rep->field[0]->report_count while printing the error message,
causing a NULL pointer dereference during probe. This is reproducible with
uhid by emulating a Logitech receiver with a padding-only DJ short output
report:
BUG: KASAN: null-ptr-deref in logi_dj_probe+0xb1/0x754 [hid_logitech_dj]
Read of size 4 at addr 0000000000000028 by task kworker/4:1/129
...
Call Trace:
logi_dj_probe+0xb1/0x754 [hid_logitech_dj]
hid_device_probe+0x329/0x3f0 [hid]
really_probe+0x162/0x570
__device_attach+0x137/0x2c0
bus_probe_device+0x38/0xc0
device_add+0xa56/0xce0
hid_add_device+0x19c/0x280 [hid]
uhid_device_add_worker+0x2c/0xb0 [uhid]
Reject the zero-field report before printing the field report_count.
Fixes: b6a57912854e ("HID: logitech-dj: Prevent REPORT_ID_DJ_SHORT related user initiated OOB write")
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-opus-4-8
Signed-off-by: HyeongJun An <sammiee5311@gmail.com>
---
drivers/hid/hid-logitech-dj.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
index 381e4dc5aba7..9c574ab8b60b 100644
--- a/drivers/hid/hid-logitech-dj.c
+++ b/drivers/hid/hid-logitech-dj.c
@@ -1907,8 +1907,13 @@ static int logi_dj_probe(struct hid_device *hdev,
output_report_enum = &hdev->report_enum[HID_OUTPUT_REPORT];
rep = output_report_enum->report_id_hash[REPORT_ID_DJ_SHORT];
- if (rep && (rep->maxfield < 1 ||
- rep->field[0]->report_count != DJREPORT_SHORT_LENGTH - 1)) {
+ if (rep && rep->maxfield < 1) {
+ hid_err(hdev, "Expected size of DJ short report is %d, but got 0",
+ DJREPORT_SHORT_LENGTH - 1);
+ return -EINVAL;
+ }
+
+ if (rep && rep->field[0]->report_count != DJREPORT_SHORT_LENGTH - 1) {
hid_err(hdev, "Expected size of DJ short report is %d, but got %d",
DJREPORT_SHORT_LENGTH - 1, rep->field[0]->report_count);
return -EINVAL;
--
2.43.0
^ permalink raw reply related
* Re: [PATCH 1/2] HID: roccat: bound device-supplied profile index
From: sashiko-bot @ 2026-06-18 3:18 UTC (permalink / raw)
To: Michael Bommarito; +Cc: dmitry.torokhov, linux-input
In-Reply-To: <20260618030036.1880139-2-michael.bommarito@gmail.com>
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues:
- [High] Closing a stale roccat character device file descriptor after device removal causes a memory leak, state corruption and Use-After-Free.
- [High] `kone_get_profile()` uses an embedded, unaligned struct field (`&kone->profiles[i]`) as a DMA buffer for inbound USB control messages, leading to cache-line sharing corruption.
--
commit cda04d8cb7b50c97cbcee76f405a2121b1b4c56d
Author: Michael Bommarito <michael.bommarito@gmail.com>
HID: roccat: bound device-supplied profile index
kone_keep_values_up_to_date() and kone_profile_activated() use an
8-bit, device-supplied profile value as an index into the 5-element
kone->profiles[] array without a range check. A malicious USB device
claiming the Roccat Kone id can send a switch-profile event (or a
startup_profile read at probe) with an out-of-range value and make the
driver read out of bounds; the result is exposed via the actual_dpi
sysfs attribute.
> diff --git a/drivers/hid/hid-roccat-kone.c b/drivers/hid/hid-roccat-kone.c
> index 58654cf78f0df..17495fcc8b7da 100644
> --- a/drivers/hid/hid-roccat-kone.c
> +++ b/drivers/hid/hid-roccat-kone.c
[ ... ]
> @@ -793,8 +795,10 @@ static void kone_keep_values_up_to_date(struct kone_device *kone,
> {
> switch (event->event) {
> case kone_mouse_event_switch_profile:
> - kone->actual_dpi = kone->profiles[event->value - 1].
> - startup_dpi;
> + if (event->value >= 1 &&
> + event->value <= ARRAY_SIZE(kone->profiles))
> + kone->actual_dpi =
> + kone->profiles[event->value - 1].startup_dpi;
> fallthrough;
> case kone_mouse_event_osd_profile:
> kone->actual_profile = event->value;
[Severity: High]
This is a pre-existing issue, but does kone_get_profile() use an embedded,
unaligned array element as a DMA buffer?
In kone_init_kone_device_struct(), we call kone_get_profile() passing a
pointer to the embedded array element:
drivers/hid/hid-roccat-kone.c:kone_init_kone_device_struct() {
...
retval = kone_get_profile(usb_dev, &kone->profiles[i], i + 1);
...
}
Which then passes the unaligned buffer directly to usb_control_msg() for a
USB_DIR_IN transfer:
drivers/hid/hid-roccat-kone.c:kone_get_profile() {
...
len = usb_control_msg(usb_dev, usb_rcvctrlpipe(usb_dev, 0),
USB_REQ_CLEAR_FEATURE,
USB_TYPE_CLASS | USB_RECIP_INTERFACE | USB_DIR_IN,
kone_command_profile, number, buf,
sizeof(struct kone_profile), USB_CTRL_SET_TIMEOUT);
...
}
Since sizeof(struct kone_profile) is 975 bytes, the array elements are not
cache-line aligned and share cache lines with other struct kone_device
members. Because hid_hw_start() is called earlier, the device can generate
interrupts and concurrent writes to these shared fields may occur.
On architectures with non-coherent DMA caches, won't the cache invalidation
required for the inbound USB DMA wipe out these concurrent CPU writes,
leading to state corruption?
[Severity: High]
This is a pre-existing issue, but can roccat_release() cause a memory leak,
list corruption, and Use-After-Free if a stale file descriptor is closed
after device removal?
When closing the file descriptor, roccat_release() retrieves the device from
the global array instead of using the local reader->device:
drivers/hid/hid-roccat.c:roccat_release() {
...
device = devices[minor];
if (!device) {
mutex_unlock(&devices_lock);
pr_emerg("roccat device with minor %d doesn't exist\n", minor);
return -ENODEV;
}
...
}
If the original device was disconnected, devices[minor] is cleared, causing
roccat_release() to return -ENODEV and permanently leak the roccat_reader
allocation and the original roccat_device.
If a new device is plugged in before the stale file descriptor is closed,
devices[minor] resolves to the new device. roccat_release() then operates on
the new device, locking its readers_lock while modifying the old device's
list:
drivers/hid/hid-roccat.c:roccat_release() {
...
list_del(&reader->node);
...
if (!--device->open) {
...
}
Could this lead to list corruption and incorrectly decrement the new device's
open count, causing premature shutdown and a Use-After-Free of the new device
when legitimate users close their descriptors?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260618030036.1880139-1-michael.bommarito@gmail.com?part=1
^ permalink raw reply
* Re: [PATCH 2/2] HID: roccat: add KUnit test for kone profile-index bounds
From: sashiko-bot @ 2026-06-18 3:14 UTC (permalink / raw)
To: Michael Bommarito; +Cc: linux-input, dmitry.torokhov
In-Reply-To: <20260618030036.1880139-3-michael.bommarito@gmail.com>
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues:
- [High] A Use-After-Free vulnerability occurs because the `kone_device` structure is freed while hardware interrupts are still active, leaving a dangling pointer in `hid_drvdata`.
- [High] Uninitialized kernel memory is copied to the persistent `kone->settings` structure on a failed USB control message, leading to a heap information leak via sysfs.
--
commit e8aee21a8be54aaf776b4d76c32eeaefd255b5aa
Author: Michael Bommarito <michael.bommarito@gmail.com>
HID: roccat: add KUnit test for kone profile-index bounds
This commit adds a KUnit test for the Roccat Kone driver to verify bounds
checking on profile indices. It simulates a crafted switch-profile event to
ensure the driver does not read out-of-bounds.
> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> index ff2f580b660ba..3c6bc918aeb54 100644
> --- a/drivers/hid/Kconfig
> +++ b/drivers/hid/Kconfig
> @@ -1057,6 +1057,15 @@ config HID_ROCCAT
> Say Y here if you have a Roccat mouse or keyboard and want
> support for its special functionalities.
>
> +config HID_ROCCAT_KONE_KUNIT_TEST
> + bool "KUnit tests for the Roccat Kone driver" if !KUNIT_ALL_TESTS
> + depends on HID_ROCCAT=y && KUNIT=y
> + default KUNIT_ALL_TESTS
> + help
> + Enable the KUnit regression tests for the Roccat Kone driver,
> + covering bounds checking of device-supplied profile indices.
> + If unsure, say N.
> +
> config HID_SAITEK
> tristate "Saitek (Mad Catz) non-fully HID-compliant devices"
> help
> diff --git a/drivers/hid/hid-roccat-kone.c b/drivers/hid/hid-roccat-kone.c
> index 17495fcc8b7da..3dae9eaa0b6f4 100644
> --- a/drivers/hid/hid-roccat-kone.c
> +++ b/drivers/hid/hid-roccat-kone.c
[ ... ]
> @@ -919,3 +919,60 @@ module_exit(kone_exit);
> MODULE_AUTHOR("Stefan Achatz");
> MODULE_DESCRIPTION("USB Roccat Kone driver");
> MODULE_LICENSE("GPL v2");
> +
> +#if IS_ENABLED(CONFIG_HID_ROCCAT_KONE_KUNIT_TEST)
> +#include <kunit/test.h>
> +
> +/*
> + * Regression test for the out-of-bounds read in
> + * kone_keep_values_up_to_date(): a malicious USB device sends a
> + * "switch profile" HID event (event == kone_mouse_event_switch_profile)
> + * with an attacker-chosen value in 0..255, which is used unbounded as
> + * profiles[value - 1]. On an unpatched kernel the attack case triggers a
> + * KASAN slab-out-of-bounds read; the fix must leave actual_dpi unchanged.
> + */
> +static void kone_profile_index_oob_test(struct kunit *test)
> +{
> + struct kone_device *kone;
> + struct kone_mouse_event ev = {};
[ ... ]
> + /* attack: value == ARRAY_SIZE(profiles) + 1 reads profiles[5], one
> + * element past the array end -> KASAN slab-out-of-bounds read on an
> + * unpatched kernel. The fix must reject it (actual_dpi unchanged).
> + */
> + ev.value = ARRAY_SIZE(kone->profiles) + 1;
> + kone_keep_values_up_to_date(kone, &ev);
> + KUNIT_EXPECT_EQ(test, kone->actual_dpi, 0x42);
> +}
> +
> +static struct kunit_case kone_test_cases[] = {
> + KUNIT_CASE(kone_profile_index_oob_test),
> + {}
> +};
> +
> +static struct kunit_suite kone_test_suite = {
> + .name = "hid-roccat-kone",
> + .test_cases = kone_test_cases,
> +};
> +kunit_test_suite(kone_test_suite);
> +#endif /* CONFIG_HID_ROCCAT_KONE_KUNIT_TEST */
[Severity: High]
This isn't a bug introduced by this patch, but while reviewing I noticed a
potential use-after-free vulnerability in the driver initialization and
teardown paths.
In kone_probe(), hid_hw_start() is called before kone_init_specials(),
starting the hardware and enabling USB interrupts. If initialization fails,
the error path frees the driver data but does not clear the pointer:
drivers/hid/hid-roccat-kone.c:kone_init_specials() {
...
exit_free:
kfree(kone);
return retval;
}
Between kfree() and hid_hw_stop(), an incoming USB interrupt will trigger
kone_raw_event(), which dereferences the dangling pointer:
drivers/hid/hid-roccat-kone.c:kone_raw_event() {
...
struct kone_device *kone = hid_get_drvdata(hdev);
...
}
The same window exists in the teardown path, where the driver state is freed
before stopping the hardware:
drivers/hid/hid-roccat-kone.c:kone_remove() {
...
kone_remove_specials(hdev);
hid_hw_stop(hdev);
}
Is it possible for an attacker to trigger this use-after-free by
disconnecting the device or simulating a device error while hardware
interrupts are active?
[Severity: High]
This is a pre-existing issue, but there appears to be a potential heap
information leak when copying uninitialized memory after a failed USB
transfer.
In kone_receive(), a bounce buffer is allocated with kmalloc() and
unconditionally copied to the target even if the USB transfer fails:
drivers/hid/hid-roccat-kone.c:kone_receive() {
buf = kmalloc(size, GFP_KERNEL);
...
retval = usb_control_msg(usb_dev, usb_rcvctrlpipe(usb_dev, 0), ...
...
memcpy(data, buf, size);
kfree(buf);
return retval;
}
When called via kone_get_settings(), the uninitialized kernel heap memory
is copied to the persistent settings structure. If this occurs during a
sysfs operation where the error is handled, the corrupted structure remains
intact:
drivers/hid/hid-roccat-kone.c:kone_sysfs_set_tcu() {
...
retval = kone_get_settings(usb_dev, &kone->settings);
if (retval)
goto exit_no_settings;
...
}
A subsequent read of the settings sysfs attribute exposes this memory
directly to userspace:
drivers/hid/hid-roccat-kone.c:kone_sysfs_read_settings() {
...
memcpy(buf, ((char const *)&kone->settings) + off, count);
...
}
Could a malicious USB device intentionally fail the transfer to exploit
this leak? Should we check the return value of usb_control_msg() before
calling memcpy() in kone_receive()?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260618030036.1880139-1-michael.bommarito@gmail.com?part=2
^ permalink raw reply
* [PATCH 2/2] HID: roccat: add KUnit test for kone profile-index bounds
From: Michael Bommarito @ 2026-06-18 3:00 UTC (permalink / raw)
To: Stefan Achatz, Jiri Kosina, Benjamin Tissoires; +Cc: linux-input, linux-kernel
In-Reply-To: <20260618030036.1880139-1-michael.bommarito@gmail.com>
Drive kone_keep_values_up_to_date() with a crafted switch-profile event;
an out-of-range value reads past profiles[] (KASAN slab-out-of-bounds on
an unpatched tree). A benign control with an in-range value exercises the
same path. The test object is sized to end at profiles[] so the over-read
lands in the KASAN redzone.
Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Michael Bommarito <michael.bommarito@gmail.com>
---
drivers/hid/Kconfig | 9 ++++++
drivers/hid/hid-roccat-kone.c | 57 +++++++++++++++++++++++++++++++++++
2 files changed, 66 insertions(+)
diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index ff2f580b660ba..3c6bc918aeb54 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -1057,6 +1057,15 @@ config HID_ROCCAT
Say Y here if you have a Roccat mouse or keyboard and want
support for its special functionalities.
+config HID_ROCCAT_KONE_KUNIT_TEST
+ bool "KUnit tests for the Roccat Kone driver" if !KUNIT_ALL_TESTS
+ depends on HID_ROCCAT=y && KUNIT=y
+ default KUNIT_ALL_TESTS
+ help
+ Enable the KUnit regression tests for the Roccat Kone driver,
+ covering bounds checking of device-supplied profile indices.
+ If unsure, say N.
+
config HID_SAITEK
tristate "Saitek (Mad Catz) non-fully HID-compliant devices"
help
diff --git a/drivers/hid/hid-roccat-kone.c b/drivers/hid/hid-roccat-kone.c
index 17495fcc8b7da..3dae9eaa0b6f4 100644
--- a/drivers/hid/hid-roccat-kone.c
+++ b/drivers/hid/hid-roccat-kone.c
@@ -919,3 +919,60 @@ module_exit(kone_exit);
MODULE_AUTHOR("Stefan Achatz");
MODULE_DESCRIPTION("USB Roccat Kone driver");
MODULE_LICENSE("GPL v2");
+
+#if IS_ENABLED(CONFIG_HID_ROCCAT_KONE_KUNIT_TEST)
+#include <kunit/test.h>
+
+/*
+ * Regression test for the out-of-bounds read in
+ * kone_keep_values_up_to_date(): a malicious USB device sends a
+ * "switch profile" HID event (event == kone_mouse_event_switch_profile)
+ * with an attacker-chosen value in 0..255, which is used unbounded as
+ * profiles[value - 1]. On an unpatched kernel the attack case triggers a
+ * KASAN slab-out-of-bounds read; the fix must leave actual_dpi unchanged.
+ */
+static void kone_profile_index_oob_test(struct kunit *test)
+{
+ struct kone_device *kone;
+ struct kone_mouse_event ev = {};
+ /*
+ * Allocate only up to the end of profiles[] so that any index past
+ * the 5-element array is IMMEDIATELY out of bounds and lands in the
+ * KASAN redzone (a far over-read would hit unrelated valid memory and
+ * escape KASAN).
+ */
+ size_t sz = offsetof(struct kone_device, profiles) +
+ sizeof(kone->profiles);
+
+ kone = kunit_kzalloc(test, sz, GFP_KERNEL);
+ KUNIT_ASSERT_NOT_NULL(test, kone);
+ kone->profiles[0].startup_dpi = 0x42;
+
+ /* benign control: a valid in-range value drives the SAME path and
+ * must succeed (proves the trigger reaches the real code).
+ */
+ ev.event = kone_mouse_event_switch_profile;
+ ev.value = 1;
+ kone_keep_values_up_to_date(kone, &ev);
+ KUNIT_EXPECT_EQ(test, kone->actual_dpi, 0x42);
+
+ /* attack: value == ARRAY_SIZE(profiles) + 1 reads profiles[5], one
+ * element past the array end -> KASAN slab-out-of-bounds read on an
+ * unpatched kernel. The fix must reject it (actual_dpi unchanged).
+ */
+ ev.value = ARRAY_SIZE(kone->profiles) + 1;
+ kone_keep_values_up_to_date(kone, &ev);
+ KUNIT_EXPECT_EQ(test, kone->actual_dpi, 0x42);
+}
+
+static struct kunit_case kone_test_cases[] = {
+ KUNIT_CASE(kone_profile_index_oob_test),
+ {}
+};
+
+static struct kunit_suite kone_test_suite = {
+ .name = "hid-roccat-kone",
+ .test_cases = kone_test_cases,
+};
+kunit_test_suite(kone_test_suite);
+#endif /* CONFIG_HID_ROCCAT_KONE_KUNIT_TEST */
--
2.53.0
^ permalink raw reply related
* [PATCH 1/2] HID: roccat: bound device-supplied profile index
From: Michael Bommarito @ 2026-06-18 3:00 UTC (permalink / raw)
To: Stefan Achatz, Jiri Kosina, Benjamin Tissoires; +Cc: linux-input, linux-kernel
In-Reply-To: <20260618030036.1880139-1-michael.bommarito@gmail.com>
kone_keep_values_up_to_date() and kone_profile_activated() use an
8-bit, device-supplied profile value as an index into the 5-element
kone->profiles[] array without a range check. A malicious USB device
claiming the Roccat Kone id can send a switch-profile event (or a
startup_profile read at probe) with an out-of-range value and make the
driver read out of bounds; the result is exposed via the actual_dpi
sysfs attribute.
Reject out-of-range indices in both paths.
This was found with static analysis and confirmed with the KUnit test
added in the following patch (KASAN: slab-out-of-bounds).
Fixes: 14bf62cde7942 ("HID: add driver for Roccat Kone gaming mouse")
Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Michael Bommarito <michael.bommarito@gmail.com>
---
drivers/hid/hid-roccat-kone.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/hid/hid-roccat-kone.c b/drivers/hid/hid-roccat-kone.c
index 58654cf78f0df..17495fcc8b7da 100644
--- a/drivers/hid/hid-roccat-kone.c
+++ b/drivers/hid/hid-roccat-kone.c
@@ -36,6 +36,8 @@ static uint profile_numbers[5] = {0, 1, 2, 3, 4};
static void kone_profile_activated(struct kone_device *kone, uint new_profile)
{
+ if (new_profile < 1 || new_profile > ARRAY_SIZE(kone->profiles))
+ new_profile = 1;
kone->actual_profile = new_profile;
kone->actual_dpi = kone->profiles[new_profile - 1].startup_dpi;
}
@@ -793,8 +795,10 @@ static void kone_keep_values_up_to_date(struct kone_device *kone,
{
switch (event->event) {
case kone_mouse_event_switch_profile:
- kone->actual_dpi = kone->profiles[event->value - 1].
- startup_dpi;
+ if (event->value >= 1 &&
+ event->value <= ARRAY_SIZE(kone->profiles))
+ kone->actual_dpi =
+ kone->profiles[event->value - 1].startup_dpi;
fallthrough;
case kone_mouse_event_osd_profile:
kone->actual_profile = event->value;
--
2.53.0
^ permalink raw reply related
* [PATCH 0/2] HID: roccat: bound device-supplied profile index
From: Michael Bommarito @ 2026-06-18 3:00 UTC (permalink / raw)
To: Stefan Achatz, Jiri Kosina, Benjamin Tissoires; +Cc: linux-input, linux-kernel
The Roccat Kone driver uses an 8-bit value taken straight from a USB HID
interrupt report as an index into a fixed 5-element profiles[] array,
without any range check. A malicious or counterfeit device that claims
the Roccat Kone VID/PID can send a "switch profile" report with an
out-of-range value and make the driver read out of bounds; the same
unbounded index is also reachable at probe time from a device-supplied
startup_profile field. The read result is stored in actual_dpi and
exposed to user space through the actual_dpi sysfs attribute.
Michael Bommarito (2):
HID: roccat: bound device-supplied profile index
HID: roccat: add KUnit test for kone profile-index bounds
drivers/hid/Kconfig | 9 +++++
drivers/hid/hid-roccat-kone.c | 65 +++++++++++++++++++++++++++++++++--
2 files changed, 72 insertions(+), 2 deletions(-)
--
2.53.0
^ permalink raw reply
* Re: [PATCH 1/2] HID: logitech-hidpp: add HID++ 2.0 reprogrammable button support
From: Elliot Douglas @ 2026-06-18 1:16 UTC (permalink / raw)
To: Bastien Nocera; +Cc: linux-input, lains, jikos, bentiss, linux-kernel
In-Reply-To: <f741bafd41f7fc31fe0019020e14ad4f99fd193e.camel@hadess.net>
Thanks, that makes sense.
For Solaar, this is not continuously forced. The kernel only programs
temporary diversion when the device connects. Solaar can still issue HID++
commands through hidraw, so if Solaar changes reporting for the same controls
afterwards, the last writer wins.
If Solaar takes over those controls for custom actions, the kernel would stop
receiving the diverted button notifications for normal evdev reporting until
the kernel diverts the controls again, for example after reconnect. While the
controls remain diverted, hidraw clients should still receive the raw HID++
reports.
I have addressed the inline comments locally for v2:
- replaced the profile/count wrapper with NULL-terminated mapping arrays
- cached the selected mapping pointer in struct hidpp_device
I'll wait for Benjamin's input to send Patch v2.
On Wed, Jun 17, 2026 at 3:28 AM Bastien Nocera <hadess@hadess.net> wrote:
>
> On Sat, 2026-06-13 at 10:51 -0700, Elliot Douglas wrote:
> > Some Logitech HID++ 2.0 mice can report diverted reprogrammable
> > controls through HID++ feature 0x1b04, SpecialKeysMseButtons /
> > REPROG_CONTROLS_V4, instead of the normal HID mouse report.
> >
> > Add a quirk-gated event path for those controls. The handler temporarily
> > diverts verified per-product controls, parses divertedButtonsEvent as the
> > current pressed-control list, and reports the corresponding evdev key state
> > for every mapped control.
> >
> > Keep the control mappings in per-product profiles so adding support for
> > another mouse does not change the evdev capabilities advertised by
> > already-supported devices.
>
> How does this forced setting work/clash with the programmable buttons
> in Solaar?
>
> I've added some inline comments below.
>
> >
> > Documentation for feature 0x1b04 describes divertedButtonsEvent as a list
> > of currently pressed diverted buttons, which is the event format handled
> > here.
> >
> > Link: https://lekensteyn.nl/files/logitech/x1b04_specialkeysmsebuttons.html
> >
> > Signed-off-by: Elliot Douglas <edouglas7358@gmail.com>
> > ---
> > drivers/hid/hid-logitech-hidpp.c | 215 +++++++++++++++++++++++++++++++
> > 1 file changed, 215 insertions(+)
> >
> > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> > index 70ba1a5e40d8..24c9cfaa4f37 100644
> > --- a/drivers/hid/hid-logitech-hidpp.c
> > +++ b/drivers/hid/hid-logitech-hidpp.c
> > @@ -76,6 +76,7 @@ MODULE_PARM_DESC(disable_tap_to_click,
> > #define HIDPP_QUIRK_HI_RES_SCROLL_1P0 BIT(28)
> > #define HIDPP_QUIRK_WIRELESS_STATUS BIT(29)
> > #define HIDPP_QUIRK_RESET_HI_RES_SCROLL BIT(30)
> > +#define HIDPP_QUIRK_HIDPP_REPROG_CONTROLS_BTNS BIT(31)
> >
> > /* These are just aliases for now */
> > #define HIDPP_QUIRK_KBD_SCROLL_WHEEL HIDPP_QUIRK_HIDPP_WHEELS
> > @@ -205,6 +206,7 @@ struct hidpp_device {
> > struct hidpp_scroll_counter vertical_wheel_counter;
> >
> > u8 wireless_feature_index;
> > + u8 reprog_controls_feature_index;
> >
> > int hires_wheel_multiplier;
> > u8 hires_wheel_feature_index;
> > @@ -3601,6 +3603,209 @@ static int hidpp10_extra_mouse_buttons_raw_event(struct hidpp_device *hidpp,
> > return 1;
> > }
> >
> > +/* -------------------------------------------------------------------------- */
> > +/* HID++2.0 reprogrammable controls */
> > +/* -------------------------------------------------------------------------- */
> > +
> > +#define HIDPP_PAGE_REPROG_CONTROLS_V4 0x1b04
> > +
> > +#define HIDPP_REPROG_CONTROLS_GET_COUNT 0x00
> > +#define HIDPP_REPROG_CONTROLS_GET_CID_INFO 0x10
> > +#define HIDPP_REPROG_CONTROLS_SET_CONTROL_REPORTING 0x30
> > +
> > +#define HIDPP_REPROG_CONTROLS_FLAG_MOUSE BIT(0)
> > +#define HIDPP_REPROG_CONTROLS_FLAG_DIVERT BIT(5)
> > +
> > +#define HIDPP_REPROG_CONTROLS_TEMPORARY_DIVERTED BIT(0)
> > +#define HIDPP_REPROG_CONTROLS_CHANGE_TEMPORARY_DIVERT BIT(1)
> > +
> > +#define HIDPP_REPROG_CONTROLS_EVENT_DIVERTED 0x00
> > +
> > +struct hidpp_reprog_control_mapping {
> > + u16 control;
> > + u16 code;
> > +};
> > +
> > +struct hidpp_reprog_controls_profile {
> > + const struct hidpp_reprog_control_mapping *mappings;
>
> probably needs a __counted_by(), or maybe as it's static, it might be
> better to not require an intermediate struct, and return a NULL-
> terminated array instead.
>
> > + unsigned int mapping_count;
> > +};
> > +
> > +static const struct hidpp_reprog_controls_profile *
> > +hidpp20_reprog_controls_get_profile(struct hidpp_device *hidpp)
> > +{
> > + return NULL;
> > +}
> > +
> > +static int hidpp20_reprog_controls_get_count(struct hidpp_device *hidpp)
> > +{
> > + struct hidpp_report response;
> > + u8 feature_index = hidpp->reprog_controls_feature_index;
> > + u8 cmd = HIDPP_REPROG_CONTROLS_GET_COUNT;
> > + int ret;
> > +
> > + ret = hidpp_send_fap_command_sync(hidpp, feature_index, cmd, NULL, 0,
> > + &response);
> > + if (ret > 0)
> > + return -EPROTO;
> > + if (ret)
> > + return ret;
> > +
> > + return response.fap.params[0];
> > +}
> > +
> > +static int hidpp20_reprog_controls_get_cid_info(struct hidpp_device *hidpp,
> > + u8 index, u16 *control,
> > + u8 *flags)
> > +{
> > + struct hidpp_report response;
> > + u8 feature_index = hidpp->reprog_controls_feature_index;
> > + u8 cmd = HIDPP_REPROG_CONTROLS_GET_CID_INFO;
> > + int ret;
> > +
> > + ret = hidpp_send_fap_command_sync(hidpp, feature_index, cmd, &index,
> > + sizeof(index), &response);
> > + if (ret > 0)
> > + return -EPROTO;
> > + if (ret)
> > + return ret;
> > +
> > + *control = get_unaligned_be16(&response.fap.params[0]);
> > + *flags = response.fap.params[4];
> > +
> > + return 0;
> > +}
> > +
> > +static bool hidpp20_reprog_controls_find_control(struct hidpp_device *hidpp,
> > + u16 control)
> > +{
> > + int count, ret;
> > + u16 cid;
> > + u8 flags;
> > + int i;
> > +
> > + count = hidpp20_reprog_controls_get_count(hidpp);
> > + if (count < 0)
> > + return false;
> > +
> > + for (i = 0; i < count; i++) {
> > + ret = hidpp20_reprog_controls_get_cid_info(hidpp, i, &cid,
> > + &flags);
> > + if (ret)
> > + return false;
> > +
> > + if (cid == control)
> > + return (flags & HIDPP_REPROG_CONTROLS_FLAG_MOUSE) &&
> > + (flags & HIDPP_REPROG_CONTROLS_FLAG_DIVERT);
> > + }
> > +
> > + return false;
> > +}
> > +
> > +static int hidpp20_reprog_controls_set_control_reporting(struct hidpp_device *hidpp,
> > + u16 control, u8 flags)
> > +{
> > + struct hidpp_report response;
> > + u8 params[5];
> > +
> > + put_unaligned_be16(control, ¶ms[0]);
> > + params[2] = flags;
> > + put_unaligned_be16(control, ¶ms[3]);
> > +
> > + return hidpp_send_fap_command_sync(hidpp,
> > + hidpp->reprog_controls_feature_index,
> > + HIDPP_REPROG_CONTROLS_SET_CONTROL_REPORTING,
> > + params, sizeof(params), &response);
> > +}
> > +
> > +static void hidpp20_reprog_controls_connect(struct hidpp_device *hidpp)
> > +{
> > + const struct hidpp_reprog_controls_profile *profile;
> > + u8 flags = HIDPP_REPROG_CONTROLS_TEMPORARY_DIVERTED |
> > + HIDPP_REPROG_CONTROLS_CHANGE_TEMPORARY_DIVERT;
> > + unsigned int i;
> > +
> > + if (!(hidpp->quirks & HIDPP_QUIRK_HIDPP_REPROG_CONTROLS_BTNS))
> > + return;
> > +
> > + profile = hidpp20_reprog_controls_get_profile(hidpp);
>
> Could the profile be cached in the hidpp_device struct?
>
> > + if (!profile)
> > + return;
> > +
> > + if (hidpp_root_get_feature(hidpp, HIDPP_PAGE_REPROG_CONTROLS_V4,
> > + &hidpp->reprog_controls_feature_index))
> > + return;
> > +
> > + for (i = 0; i < profile->mapping_count; i++) {
> > + u16 control = profile->mappings[i].control;
> > +
> > + if (!hidpp20_reprog_controls_find_control(hidpp, control))
> > + continue;
> > +
> > + hidpp20_reprog_controls_set_control_reporting(hidpp, control, flags);
> > + }
> > +}
> > +
> > +static int hidpp20_reprog_controls_raw_event(struct hidpp_device *hidpp,
> > + u8 *data, int size)
> > +{
> > + const struct hidpp_reprog_controls_profile *profile;
> > + const struct hidpp_reprog_control_mapping *mapping;
> > + struct hidpp_report *report = (struct hidpp_report *)data;
> > + u16 controls[4];
> > + bool pressed;
> > + unsigned int i, j;
> > +
> > + if (!(hidpp->quirks & HIDPP_QUIRK_HIDPP_REPROG_CONTROLS_BTNS) ||
> > + !hidpp->input ||
> > + hidpp->reprog_controls_feature_index == 0xff)
> > + return 0;
> > +
> > + profile = hidpp20_reprog_controls_get_profile(hidpp);
> > + if (!profile)
> > + return 0;
> > +
> > + if (size < HIDPP_REPORT_LONG_LENGTH ||
> > + report->fap.feature_index != hidpp->reprog_controls_feature_index ||
> > + report->fap.funcindex_clientid != HIDPP_REPROG_CONTROLS_EVENT_DIVERTED)
> > + return 0;
> > +
> > + for (i = 0; i < ARRAY_SIZE(controls); i++)
> > + controls[i] = get_unaligned_be16(&report->fap.params[i * 2]);
> > +
> > + for (i = 0; i < profile->mapping_count; i++) {
> > + mapping = &profile->mappings[i];
> > + pressed = false;
> > +
> > + for (j = 0; j < ARRAY_SIZE(controls); j++) {
> > + if (controls[j] == mapping->control) {
> > + pressed = true;
> > + break;
> > + }
> > + }
> > +
> > + input_report_key(hidpp->input, mapping->code, pressed);
> > + }
> > +
> > + input_sync(hidpp->input);
> > +
> > + return 1;
> > +}
> > +
> > +static void hidpp20_reprog_controls_populate_input(struct hidpp_device *hidpp,
> > + struct input_dev *input_dev)
> > +{
> > + const struct hidpp_reprog_controls_profile *profile;
> > + unsigned int i;
> > +
> > + profile = hidpp20_reprog_controls_get_profile(hidpp);
> > + if (!profile)
> > + return;
> > +
> > + for (i = 0; i < profile->mapping_count; i++)
> > + input_set_capability(input_dev, EV_KEY, profile->mappings[i].code);
> > +}
> > +
> > static void hidpp10_extra_mouse_buttons_populate_input(
> > struct hidpp_device *hidpp, struct input_dev *input_dev)
> > {
> > @@ -3859,6 +4064,9 @@ static void hidpp_populate_input(struct hidpp_device *hidpp,
> >
> > if (hidpp->quirks & HIDPP_QUIRK_HIDPP_EXTRA_MOUSE_BTNS)
> > hidpp10_extra_mouse_buttons_populate_input(hidpp, input);
> > +
> > + if (hidpp->quirks & HIDPP_QUIRK_HIDPP_REPROG_CONTROLS_BTNS)
> > + hidpp20_reprog_controls_populate_input(hidpp, input);
> > }
> >
> > static int hidpp_input_configured(struct hid_device *hdev,
> > @@ -3971,6 +4179,10 @@ static int hidpp_raw_hidpp_event(struct hidpp_device *hidpp, u8 *data,
> > return ret;
> > }
> >
> > + ret = hidpp20_reprog_controls_raw_event(hidpp, data, size);
> > + if (ret != 0)
> > + return ret;
> > +
> > if (hidpp->quirks & HIDPP_QUIRK_HIDPP_CONSUMER_VENDOR_KEYS) {
> > ret = hidpp10_consumer_keys_raw_event(hidpp, data, size);
> > if (ret != 0)
> > @@ -4264,6 +4476,8 @@ static void hidpp_connect_event(struct work_struct *work)
> > return;
> > }
> >
> > + hidpp20_reprog_controls_connect(hidpp);
> > +
> > if (hidpp->quirks & HIDPP_QUIRK_HIDPP_CONSUMER_VENDOR_KEYS) {
> > ret = hidpp10_consumer_keys_connect(hidpp);
> > if (ret)
> > @@ -4436,6 +4650,7 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
> > hidpp->hid_dev = hdev;
> > hidpp->name = hdev->name;
> > hidpp->quirks = id->driver_data;
> > + hidpp->reprog_controls_feature_index = 0xff;
> > hid_set_drvdata(hdev, hidpp);
> >
> > ret = hid_parse(hdev);
^ permalink raw reply
* Re: [PATCH v4 10/10] Input: cap11xx - add support for CAP1114
From: sashiko-bot @ 2026-06-17 21:25 UTC (permalink / raw)
To: Jun Yan; +Cc: dmitry.torokhov, devicetree, linux-input, conor+dt, robh
In-Reply-To: <20260617150318.753148-11-jerrysteve1101@gmail.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] Out-of-bounds write in `cap11xx_i2c_probe` due to unvalidated Device Tree keycodes.
--
commit 42cef688f767435c4da8af0dd519be20d4164735
Author: Jun Yan <jerrysteve1101@gmail.com>
CAP1114 is a 14-channel capacitive touch sensor with 11 LED outputs
and hardware reset support.
This commit adds support for the CAP1114 touch sensor, updating the driver
to handle up to 14 channels and 11 LED outputs. It introduces logic to
read from a secondary status register to support the expanded channel count
and configures grouped sensors as independent inputs.
> diff --git a/drivers/input/keyboard/cap11xx.c b/drivers/input/keyboard/cap11xx.c
> index 275eb79a7193..865c58533eb9 100644
> --- a/drivers/input/keyboard/cap11xx.c
> +++ b/drivers/input/keyboard/cap11xx.c
[ ... ]
> @@ -292,6 +307,17 @@ static int cap11xx_init_keys(struct cap11xx_priv *priv)
> of_property_read_u32_array(node, "linux,keycodes",
> priv->keycodes, priv->model->num_channels);
[Severity: High]
This is a pre-existing issue, but I noticed that these keycodes are read
directly from the Device Tree into the priv->keycodes array without any
bounds validation.
Later during initialization in cap11xx_i2c_probe(), the driver loops over
these keycodes and sets bits using an unchecked macro:
__set_bit(priv->keycodes[i], priv->idev->keybit);
If an exceptionally large keycode (greater than KEY_MAX) is provided via a
Device Tree overlay, could this cause an out-of-bounds write on the
priv->idev->keybit array and corrupt adjacent heap memory? Should we verify
that each keycode read from the Device Tree is less than or equal to KEY_MAX?
> +
> + /*
> + * CAP1114 needs dedicated configuration to split
> + * grouped sensors into independent inputs.
> + */
> + if (priv->model->has_grouped_sensors) {
> + error = regmap_set_bits(priv->regmap, CAP1114_REG_CONFIG2,
> + CAP1114_REG_CONFIG2_VOL_UP_DOWN);
> + if (error)
> + return error;
> + }
> +
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260617150318.753148-1-jerrysteve1101@gmail.com?part=10
^ permalink raw reply
* Re: [PATCH v4 09/10] dt-bindings: input: microchip,cap11xx: Add CAP1114 support
From: sashiko-bot @ 2026-06-17 21:15 UTC (permalink / raw)
To: Jun Yan; +Cc: conor+dt, devicetree, robh, linux-input, dmitry.torokhov
In-Reply-To: <20260617150318.753148-10-jerrysteve1101@gmail.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Schema Validation Regression (Incomplete Constraints Update)
--
commit 74e08f50d786114a66be01956a59aeb6b7083fbc
Author: Jun Yan <jerrysteve1101@gmail.com>
dt-bindings: input: cap11xx: Add CAP1114 support
CAP1114 is a 14-channel capacitive touch sensor with 11 LED outputs
and hardware reset support. Add the compatible string for CAP1114, add
its datasheet URL, update the maximum of LED channel reg, and add
constraint for linux,keycodes.
> diff --git a/Documentation/devicetree/bindings/input/microchip,cap11xx.yaml b/Documentation/devicetree/bindings/input/microchip,cap11xx.yaml
> index b97e5b2735f1..2a37ac252c37 100644
> --- a/Documentation/devicetree/bindings/input/microchip,cap11xx.yaml
> +++ b/Documentation/devicetree/bindings/input/microchip,cap11xx.yaml
[ ... ]
> @@ -140,7 +144,7 @@ properties:
> The number of entries must correspond to the number of channels.
>
> patternProperties:
> - "^led@[0-7]$":
> + "^led@[0-9a]$":
[Severity: Medium]
Does this global regex update also require an update to the disablement block
for hardware variants without LEDs?
The allOf block that disables LEDs for specific variants (like CAP1106,
CAP1203, etc.) still uses "^led@[0-7]$": false.
Because of this new global regex, wouldn't nodes like led@8, led@9,
and led@a now bypass the false constraint and be silently permitted on
devices that are supposed to have zero LEDs?
> type: object
> description: CAP11xx LEDs
> $ref: /schemas/leds/common.yaml#
> @@ -149,7 +153,7 @@ patternProperties:
> reg:
> description: LED channel number
> minimum: 0
> - maximum: 7
> + maximum: 10
>
> label: true
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260617150318.753148-1-jerrysteve1101@gmail.com?part=9
^ 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