| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAMCVhVNUrcWD5avaBGotnEgQ-kS9HMPaDpkYczscozSg5H-NfA@mail.gmail.com>
Date: Wed, 12 Nov 2025 17:45:16 -0600
From: Jonathan Denose <jdenose@...gle.com>
To: Dmitry Torokhov <dmitry.torokhov@...il.com>
Cc: Jiri Kosina <jikos@...nel.org>, Benjamin Tissoires <bentiss@...nel.org>, linux-input@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/2] Input: Add lid switch notifier
Hi Dmitry,
Thanks for your review.
On Tue, Nov 11, 2025 at 4:34 PM Dmitry Torokhov
<dmitry.torokhov@...il.com> wrote:
>
> Hi Jonathan,
>
> On Tue, Nov 11, 2025 at 09:34:06PM +0000, Jonathan Denose wrote:
> > This change creates a new input handler which can be included in the
> > build via a new Kconfig option CONFIG_INPUT_LID_NOTIFIER. This input
> > handler listens for lid switch events and publishes them through an
> > atomic notification chain. Other modules may register for events
> > through this notification chain with register_lid_notifier.
> >
> > Signed-off-by: Jonathan Denose <jdenose@...gle.com>
> > ---
> > drivers/input/Kconfig | 11 +++++
> > drivers/input/Makefile | 1 +
> > drivers/input/lid-notifier.c | 98 ++++++++++++++++++++++++++++++++++++++++++++
> > include/linux/input.h | 2 +
> > 4 files changed, 112 insertions(+)
> >
> > diff --git a/drivers/input/Kconfig b/drivers/input/Kconfig
> > index 88ecdf5218ee9ba35e1efec6341f8605b621bd49..16f6d24fd04ac8cb5af9d36cc47155ea9be0e177 100644
> > --- a/drivers/input/Kconfig
> > +++ b/drivers/input/Kconfig
> > @@ -38,6 +38,17 @@ config INPUT_LEDS
> > To compile this driver as a module, choose M here: the
> > module will be called input-leds.
> >
> > +config INPUT_LID_NOTIFIER
> > + tristate "Include notifier for lid switch events"
> > + help
> > + Say Y here if you would like to create a notifier to publish lid switch
> > + events.
> > +
> > + If unsure, say N.
> > +
> > + To compile this driver as a module, choose M here: the
> > + module will be called lid-notifier.
>
> I think this better not surfaced to users but rather interested drivers
> 'select' it.
That makes sense to me, but how could I get the lid-notifier.c file to
compile in that case? That was my only reason for adding it as a
config option.
> > +
> > config INPUT_FF_MEMLESS
> > tristate "Support for memoryless force-feedback devices"
> > help
> > diff --git a/drivers/input/Makefile b/drivers/input/Makefile
> > index 2cd6e1c9a77844fe09cd3d99533e5d3efb038c7d..1efdba04f79a97e2a122b9198341b18a1855b4b9 100644
> > --- a/drivers/input/Makefile
> > +++ b/drivers/input/Makefile
> > @@ -15,6 +15,7 @@ obj-$(CONFIG_INPUT_MATRIXKMAP) += matrix-keymap.o
> > obj-$(CONFIG_INPUT_VIVALDIFMAP) += vivaldi-fmap.o
> >
> > obj-$(CONFIG_INPUT_LEDS) += input-leds.o
> > +obj-$(CONFIG_INPUT_LID_NOTIFIER) += lid-notifier.o
> > obj-$(CONFIG_INPUT_MOUSEDEV) += mousedev.o
> > obj-$(CONFIG_INPUT_JOYDEV) += joydev.o
> > obj-$(CONFIG_INPUT_EVDEV) += evdev.o
> > diff --git a/drivers/input/lid-notifier.c b/drivers/input/lid-notifier.c
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..954b9855532dbd0514860e309d0b76982e947673
> > --- /dev/null
> > +++ b/drivers/input/lid-notifier.c
> > @@ -0,0 +1,98 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Lid event notifier
> > + *
> > + * Copyright (c) 2025 Jonathan Denose <jdenose@...gle.com>
> > + */
> > +
> > +#include <linux/device.h>
> > +#include <linux/input.h>
> > +#include <linux/notifier.h>
> > +
> > +static struct input_handler lid_handler;
> > +static struct atomic_notifier_head input_notifier_head;
> > +
> > +int register_lid_notifier(struct notifier_block *notifier)
> > +{
> > + return atomic_notifier_chain_register(&input_notifier_head, notifier);
> > +}
> > +EXPORT_SYMBOL(register_lid_notifier);
>
> I wonder if we want to expose the "raw" notifier or if we want to
> provide a higher-level API that would allocate a notifier blocki, set up
> the callback, and return a "cookie" that can be used to free notifier
> block later. This way we do not need to worry that some enterprising
> driver suppresses notifications for the rest by returning NOTIFY_STOP.
>
> > +
> > +static int lid_handler_connect(struct input_handler *handler,
> > + struct input_dev *input_dev, const struct input_device_id *id)
>
> Proper alignment of the arguments please.
>
> > +{
> > + struct input_handle *handle;
> > + int error;
> > +
> > + handle = devm_kzalloc(&input_dev->dev, sizeof(struct input_handle), GFP_KERNEL);
>
> This is not driver probe path so devm_kzalloc must not be used here.
> Also "sizeof(*handle)".
So in that case, would I just use kzalloc?
> > + if (!handle)
> > + return -ENOMEM;
> > +
> > + handle->dev = input_dev;
> > + handle->handler = handler;
> > + handle->name = "lid";
> > +
> > + error = input_register_handle(handle);
> > + if (error)
> > + goto err_free_handle;
> > +
> > + error = input_open_device(handle);
> > + if (error)
> > + goto err_unregister_handle;
> > +
> > + return 0;
> > +
> > + err_unregister_handle:
> > + input_unregister_handle(handle);
> > + err_free_handle:
> > + kfree(handle);
>
> Just FYI: One must never use kfree() with devm_kalloc()ed memory.
Noted!
> > + return error;
> > +}
> > +
> > +static void lid_handler_disconnect(struct input_handle *handle)
> > +{
> > + input_close_device(handle);
> > + input_unregister_handle(handle);
>
> kfree(handle);
>
> > +}
> > +
> > +static void lid_handler_event(struct input_handle *handle, unsigned int type,
> > + unsigned int code, int value)
> > +{
> > + if (type == EV_SW && code == SW_LID)
> > + atomic_notifier_call_chain(&input_notifier_head, value, handle->dev);
>
> Why do you need to pass the device from which SW_LID originated?
It isn't needed, I'll pass NULL instead.
> > +}
> > +
> > +static const struct input_device_id lid_handler_ids[] = {
> > + {
> > + .flags = INPUT_DEVICE_ID_MATCH_EVBIT | INPUT_DEVICE_ID_MATCH_SWBIT
> > + | INPUT_DEVICE_ID_MATCH_BUS,
> > + .evbit = { BIT_MASK(EV_SW) },
> > + .swbit = { [BIT_WORD(SW_LID)] = BIT_MASK(SW_LID) },
> > + .bustype = 0x19
>
> Why do we need to match in bus type? The LID does not have to always
> come from ACPI.
On the specific chromebook where this touchpad is found, when the lid
switch is opened or closed, `cros_ec_buttons` also sends SW_LID
events, so lid_handler_event was unnecessarily called twice. To
prevent that, I'm filtering on the bus type to get only the events
originating from the lid switch.
> > + },
> > + { },
> > +};
> > +
> > +static struct input_handler lid_handler = {
> > + .connect = lid_handler_connect,
> > + .disconnect = lid_handler_disconnect,
> > + .event = lid_handler_event,
> > + .name = "lid",
> > + .id_table = lid_handler_ids
> > +};
> > +
> > +static int __init lid_notifier_init(void)
> > +{
> > + return input_register_handler(&lid_handler);
> > +}
> > +module_init(lid_notifier_init);
> > +
> > +static void __exit lid_notifier_exit(void)
> > +{
> > + input_unregister_handler(&lid_handler);
> > +}
> > +module_exit(lid_notifier_exit);
> > +
> > +MODULE_AUTHOR("Jonathan Denose <jdenose@...gle.com>");
> > +MODULE_DESCRIPTION("Lid event notifier");
> > +MODULE_LICENSE("GPL");
> > diff --git a/include/linux/input.h b/include/linux/input.h
> > index 7d7cb0593a63e93c4906c49cde430188db2d1ab5..023eb92c77d9e8721d482b9787632a671671de08 100644
> > --- a/include/linux/input.h
> > +++ b/include/linux/input.h
> > @@ -592,3 +592,5 @@ int input_ff_create_memless(struct input_dev *dev, void *data,
> > int (*play_effect)(struct input_dev *, void *, struct ff_effect *));
> >
> > #endif
>
> I think this should go into include/linux/lid-notifier.h.
>
> > +
> > +int register_lid_notifier(struct notifier_block *notifier);
>
> Thanks.
>
> --
> Dmitry
--
Jonathan
Powered by blists - more mailing lists