* [syzbot] [wpan?] [input?] [usb?] memory leak in hwsim_add_one (2)
From: syzbot @ 2023-09-22 1:55 UTC (permalink / raw)
To: alex.aring, davem, edumazet, kuba, linux-input, linux-kernel,
linux-usb, linux-wpan, miquel.raynal, netdev, pabeni, stefan,
syzkaller-bugs
Hello,
syzbot found the following issue on:
HEAD commit: e789286468a9 Merge tag 'x86-urgent-2023-09-17' of git://gi..
git tree: upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=16db487fa80000
kernel config: https://syzkaller.appspot.com/x/.config?x=943a94479fa8e863
dashboard link: https://syzkaller.appspot.com/bug?extid=d2aa0f55c4ae66a9b75d
compiler: gcc (Debian 12.2.0-14) 12.2.0, GNU ld (GNU Binutils for Debian) 2.40
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=15cc8372680000
Downloadable assets:
disk image: https://storage.googleapis.com/syzbot-assets/60bec5b60566/disk-e7892864.raw.xz
vmlinux: https://storage.googleapis.com/syzbot-assets/509a449f2ff0/vmlinux-e7892864.xz
kernel image: https://storage.googleapis.com/syzbot-assets/36581da19789/bzImage-e7892864.xz
IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: syzbot+d2aa0f55c4ae66a9b75d@syzkaller.appspotmail.com
BUG: memory leak
unreferenced object 0xffff8881042a8940 (size 64):
comm "swapper/0", pid 1, jiffies 4294937901 (age 1085.750s)
hex dump (first 32 bytes):
00 0d 00 00 00 00 00 00 ff ff ff ff 00 00 00 00 ................
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
backtrace:
[<ffffffff81573dc5>] kmalloc_trace+0x25/0x90 mm/slab_common.c:1114
[<ffffffff831b5a6a>] kmalloc include/linux/slab.h:599 [inline]
[<ffffffff831b5a6a>] kzalloc include/linux/slab.h:720 [inline]
[<ffffffff831b5a6a>] hwsim_add_one+0x14a/0x650 drivers/net/ieee802154/mac802154_hwsim.c:949
[<ffffffff831b5f93>] hwsim_probe+0x23/0xe0 drivers/net/ieee802154/mac802154_hwsim.c:1022
[<ffffffff82c14f93>] platform_probe+0x83/0x110 drivers/base/platform.c:1404
[<ffffffff82c10fc6>] call_driver_probe drivers/base/dd.c:579 [inline]
[<ffffffff82c10fc6>] really_probe+0x126/0x440 drivers/base/dd.c:658
[<ffffffff82c113a3>] __driver_probe_device+0xc3/0x190 drivers/base/dd.c:800
[<ffffffff82c1149a>] driver_probe_device+0x2a/0x120 drivers/base/dd.c:830
[<ffffffff82c117f7>] __driver_attach drivers/base/dd.c:1216 [inline]
[<ffffffff82c117f7>] __driver_attach+0x107/0x1f0 drivers/base/dd.c:1156
[<ffffffff82c0e2f3>] bus_for_each_dev+0xb3/0x110 drivers/base/bus.c:368
[<ffffffff82c0fdf6>] bus_add_driver+0x126/0x2a0 drivers/base/bus.c:673
[<ffffffff82c12ee5>] driver_register+0x85/0x180 drivers/base/driver.c:246
[<ffffffff8756e3a6>] hwsim_init_module+0xc6/0x110 drivers/net/ieee802154/mac802154_hwsim.c:1073
[<ffffffff81001cb6>] do_one_initcall+0x76/0x430 init/main.c:1232
[<ffffffff874d76ea>] do_initcall_level init/main.c:1294 [inline]
[<ffffffff874d76ea>] do_initcalls init/main.c:1310 [inline]
[<ffffffff874d76ea>] do_basic_setup init/main.c:1329 [inline]
[<ffffffff874d76ea>] kernel_init_freeable+0x25a/0x460 init/main.c:1547
[<ffffffff84b3628b>] kernel_init+0x1b/0x290 init/main.c:1437
[<ffffffff81149e35>] ret_from_fork+0x45/0x50 arch/x86/kernel/process.c:147
BUG: memory leak
unreferenced object 0xffff8881042a8780 (size 64):
comm "swapper/0", pid 1, jiffies 4294937902 (age 1085.740s)
hex dump (first 32 bytes):
00 0d 00 00 00 00 00 00 ff ff ff ff 00 00 00 00 ................
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
backtrace:
[<ffffffff81573dc5>] kmalloc_trace+0x25/0x90 mm/slab_common.c:1114
[<ffffffff831b5a6a>] kmalloc include/linux/slab.h:599 [inline]
[<ffffffff831b5a6a>] kzalloc include/linux/slab.h:720 [inline]
[<ffffffff831b5a6a>] hwsim_add_one+0x14a/0x650 drivers/net/ieee802154/mac802154_hwsim.c:949
[<ffffffff831b5fb6>] hwsim_probe+0x46/0xe0 drivers/net/ieee802154/mac802154_hwsim.c:1022
[<ffffffff82c14f93>] platform_probe+0x83/0x110 drivers/base/platform.c:1404
[<ffffffff82c10fc6>] call_driver_probe drivers/base/dd.c:579 [inline]
[<ffffffff82c10fc6>] really_probe+0x126/0x440 drivers/base/dd.c:658
[<ffffffff82c113a3>] __driver_probe_device+0xc3/0x190 drivers/base/dd.c:800
[<ffffffff82c1149a>] driver_probe_device+0x2a/0x120 drivers/base/dd.c:830
[<ffffffff82c117f7>] __driver_attach drivers/base/dd.c:1216 [inline]
[<ffffffff82c117f7>] __driver_attach+0x107/0x1f0 drivers/base/dd.c:1156
[<ffffffff82c0e2f3>] bus_for_each_dev+0xb3/0x110 drivers/base/bus.c:368
[<ffffffff82c0fdf6>] bus_add_driver+0x126/0x2a0 drivers/base/bus.c:673
[<ffffffff82c12ee5>] driver_register+0x85/0x180 drivers/base/driver.c:246
[<ffffffff8756e3a6>] hwsim_init_module+0xc6/0x110 drivers/net/ieee802154/mac802154_hwsim.c:1073
[<ffffffff81001cb6>] do_one_initcall+0x76/0x430 init/main.c:1232
[<ffffffff874d76ea>] do_initcall_level init/main.c:1294 [inline]
[<ffffffff874d76ea>] do_initcalls init/main.c:1310 [inline]
[<ffffffff874d76ea>] do_basic_setup init/main.c:1329 [inline]
[<ffffffff874d76ea>] kernel_init_freeable+0x25a/0x460 init/main.c:1547
[<ffffffff84b3628b>] kernel_init+0x1b/0x290 init/main.c:1437
[<ffffffff81149e35>] ret_from_fork+0x45/0x50 arch/x86/kernel/process.c:147
BUG: memory leak
unreferenced object 0xffff8881007cc000 (size 768):
comm "udevd", pid 4480, jiffies 4295045154 (age 13.270s)
hex dump (first 32 bytes):
01 00 00 00 03 00 00 00 08 00 00 00 00 00 00 00 ................
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
backtrace:
[<ffffffff83e8ff95>] alloc_inode_sb include/linux/fs.h:2909 [inline]
[<ffffffff83e8ff95>] sock_alloc_inode+0x25/0x90 net/socket.c:308
[<ffffffff816c39d3>] alloc_inode+0x23/0x100 fs/inode.c:259
[<ffffffff816c4bf6>] new_inode_pseudo+0x16/0x50 fs/inode.c:1004
[<ffffffff83e8f2ab>] sock_alloc+0x1b/0x90 net/socket.c:634
[<ffffffff83e8f8cd>] __sock_create+0xbd/0x2e0 net/socket.c:1516
[<ffffffff83e92d58>] sock_create net/socket.c:1603 [inline]
[<ffffffff83e92d58>] __sys_socket_create net/socket.c:1640 [inline]
[<ffffffff83e92d58>] __sys_socket+0xb8/0x1a0 net/socket.c:1691
[<ffffffff83e92e5b>] __do_sys_socket net/socket.c:1705 [inline]
[<ffffffff83e92e5b>] __se_sys_socket net/socket.c:1703 [inline]
[<ffffffff83e92e5b>] __x64_sys_socket+0x1b/0x20 net/socket.c:1703
[<ffffffff84b30fc8>] do_syscall_x64 arch/x86/entry/common.c:50 [inline]
[<ffffffff84b30fc8>] do_syscall_64+0x38/0xb0 arch/x86/entry/common.c:80
[<ffffffff84c0008b>] entry_SYSCALL_64_after_hwframe+0x63/0xcd
BUG: memory leak
unreferenced object 0xffff88810c7019a0 (size 32):
comm "udevd", pid 4480, jiffies 4295045154 (age 13.270s)
hex dump (first 32 bytes):
b8 c1 7c 00 81 88 ff ff 70 3d 34 82 ff ff ff ff ..|.....p=4.....
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
backtrace:
[<ffffffff823457a2>] kmem_cache_zalloc include/linux/slab.h:710 [inline]
[<ffffffff823457a2>] lsm_inode_alloc security/security.c:633 [inline]
[<ffffffff823457a2>] security_inode_alloc+0x32/0xd0 security/security.c:1494
[<ffffffff816c01ad>] inode_init_always+0x1ed/0x230 fs/inode.c:230
[<ffffffff816c39f0>] alloc_inode+0x40/0x100 fs/inode.c:266
[<ffffffff816c4bf6>] new_inode_pseudo+0x16/0x50 fs/inode.c:1004
[<ffffffff83e8f2ab>] sock_alloc+0x1b/0x90 net/socket.c:634
[<ffffffff83e8f8cd>] __sock_create+0xbd/0x2e0 net/socket.c:1516
[<ffffffff83e92d58>] sock_create net/socket.c:1603 [inline]
[<ffffffff83e92d58>] __sys_socket_create net/socket.c:1640 [inline]
[<ffffffff83e92d58>] __sys_socket+0xb8/0x1a0 net/socket.c:1691
[<ffffffff83e92e5b>] __do_sys_socket net/socket.c:1705 [inline]
[<ffffffff83e92e5b>] __se_sys_socket net/socket.c:1703 [inline]
[<ffffffff83e92e5b>] __x64_sys_socket+0x1b/0x20 net/socket.c:1703
[<ffffffff84b30fc8>] do_syscall_x64 arch/x86/entry/common.c:50 [inline]
[<ffffffff84b30fc8>] do_syscall_64+0x38/0xb0 arch/x86/entry/common.c:80
[<ffffffff84c0008b>] entry_SYSCALL_64_after_hwframe+0x63/0xcd
[
---
This report is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzkaller@googlegroups.com.
syzbot will keep track of this issue. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
If the bug is already fixed, let syzbot know by replying with:
#syz fix: exact-commit-title
If you want syzbot to run the reproducer, reply with:
#syz test: git://repo/address.git branch-or-commit-hash
If you attach or paste a git patch, syzbot will apply it before testing.
If you want to overwrite bug's subsystems, reply with:
#syz set subsystems: new-subsystem
(See the list of subsystem names on the web dashboard)
If the bug is a duplicate of another bug, reply with:
#syz dup: exact-subject-of-another-report
If you want to undo deduplication, reply with:
#syz undup
^ permalink raw reply
* [RFC PATCH] of: device: Support 2nd sources of probeable but undiscoverable devices
From: Douglas Anderson @ 2023-09-21 17:24 UTC (permalink / raw)
To: Rob Herring, Krzysztof Kozlowski, Conor Dooley, devicetree
Cc: Benjamin Tissoires, Chen-Yu Tsai, linux-input, Jiri Kosina,
Hsin-Yi Wang, linux-gpio, linus.walleij, Dmitry Torokhov,
Johan Hovold, Douglas Anderson, andriy.shevchenko, broonie,
frowand.list, gregkh, hdegoede, james.clark, james, keescook,
linux-kernel, petr.tesarik.ext, rafael, tglx
Support for multiple "equivalent" sources for components (also known
as second sourcing components) is a standard practice that helps keep
cost down and also makes sure that if one component is unavailable due
to a shortage that we don't need to stop production for the whole
product.
Some components are very easy to second source. eMMC, for instance, is
fully discoverable and probable so you can stuff a wide variety of
similar eMMC chips on your board and things will work without a hitch.
Some components are more difficult to second source, specifically
because it's difficult for software to probe what component is present
on any given board. In cases like this software is provided
supplementary information to help it, like a GPIO strap or a SKU ID
programmed into an EEPROM. This helpful information can allow the
bootloader to select a different device tree. The various different
"SKUs" of different Chromebooks are examples of this.
Some components are somewhere in between. These in-between components
are the subject of this patch. Specifically, these components are
easily "probeable" but not easily "discoverable".
A good example of a probeable but undiscoverable device is an
i2c-connected touchscreen or trackpad. Two separate components may be
electrically compatible with each other and may have compatible power
sequencing requirements but may require different software. If
software is told about the different possible components (because it
can't discover them), it can safely probe them to figure out which
ones are present.
On systems using device tree, if we want to tell the OS about all of
the different components we need to list them all in the device
tree. This leads to a problem. The multiple sources for components
likely use the same resources (GPIOs, interrupts, regulators). If the
OS tries to probe all of these components at the same time then it
will detect a resource conflict and that's a fatal error.
The fact that Linux can't handle these probeable but undiscoverable
devices well has had a few consequences:
1. In some cases, we've abandoned the idea of second sourcing
components for a given board, which increases cost / generates
manufacturing headaches.
2. In some cases, we've been forced to add some sort of strapping /
EEPROM to indicate which component is present. This adds difficulty
to manufacturing / refurb processes.
3. In some cases, we've managed to make things work by the skin of our
teeth through slightly hacky solutions. Specifically, if we remove
the "pinctrl" entry from the various options then it won't
conflict. Regulators inherently can have more than one consumer, so
as long as there are no GPIOs involved in power sequencing and
probing devices then things can work. This is how
"sc8280xp-lenovo-thinkpad-x13s" works and also how
"mt8173-elm-hana" works.
Let's attempt to do something better. Specifically, we'll allow
tagging nodes in the device tree as mutually exclusive from one
another. This says that only one of the components in this group is
present on any given board. To make it concrete, in my proposal this
looks like:
/ {
tp_ex_group: trackpad-exclusion-group {
};
};
&i2c_bus {
tp1: trackpad@10 {
...
mutual-exclusion-group = <&tp_ex_group>;
};
tp2: trackpad@20 {
...
mutual-exclusion-group = <&tp_ex_group>;
};
tp3: trackpad@30 {
...
mutual-exclusion-group = <&tp_ex_group>;
};
};
In Linux, we can make things work by simply only probing one of the
devices in the group at a time. We can make a mutex per group and
enforce locking that mutex around probe. If the first device that gets
the mutex fails to probe then it won't try again. If it succeeds then
it will acquire the shared resources and future devices (which we know
can't be present) will fail to get the shared resources. Future
patches could quiet down errors about failing to acquire shared
resources or failing to probe if a device is in a
mutual-exclusion-group.
A traditional response to a proposal to express this type of
information in the device tree is that it's a "hack" to work around
Linux's quirks and is not a proper hardware description.
One often proposed solution instead of this "hack" is that firmware
should be probing the hardware and it should ensure that the device
tree only expresses the hardware that's present. This has a few
serious downsides:
1. It slows down boot. Powering up a component to probe it could take
hundreds of milliseconds and, in the bootloader, it can't be
parallelized with anything else.
2. It adds complexity to firmware. By its nature, firmware is harder
to update regularly and impossible to keep "lockstep" with the
kernel. This leads to the general principle that if we can keep
code out of firmware then we should.
3. Not all firmware can be updated. If a device originally shipped as
a Windows laptop or an Android phone, the bootloader might not be
open source and easy to update.
Another proposed solution instead of this "hack" is that Linux should
automagically handle this. The idea here is that during probe a device
should get its resources provisionally and not commit to them until
the probe is a success. While possible, this is difficult to implement
in a generic way across all possible resources.
Instead of thinking of this as a "hack", it doesn't seem too
unreasonable to think of this as a hardware description even if it's
an inexact one. We are describing that the hardware has one of N
different variants and we describe the non-discoverable properties of
those components.
For some prior discussions:
- We discussed a bit of this recently in a patch that Johan posted to
make simple i2c-hid devices (those that don't need reset GPIOs) work
again [1].
- Johan pointed to a previous discussion with Rob [2].
- Dmitry did some previous prototyping of trying to handle this
automagically for GPIOs [3].
[1] https://lore.kernel.org/r/20230918125851.310-1-johan+linaro@kernel.org
[2] https://lore.kernel.org/r/Y3teH14YduOQQkNn@hovoldconsulting.com/
[3] https://crrev.com/c/461349
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
I definitely understand that, if we decide to go this way, somewhere
in DT documentation we need to document it. However, I wasn't sure
where that should happen. I'd love advice!
drivers/base/core.c | 1 +
drivers/base/dd.c | 7 +++++
drivers/of/device.c | 54 +++++++++++++++++++++++++++++++++++++++
include/linux/device.h | 5 ++++
include/linux/of_device.h | 6 +++++
5 files changed, 73 insertions(+)
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 4d8b315c48a1..adeceea331df 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -3109,6 +3109,7 @@ void device_initialize(struct device *dev)
dev->dma_coherent = dma_default_coherent;
#endif
swiotlb_dev_init(dev);
+ of_device_init(dev);
}
EXPORT_SYMBOL_GPL(device_initialize);
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index a528cec24264..476d411b5443 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -789,6 +789,9 @@ static int __driver_probe_device(struct device_driver *drv, struct device *dev)
pr_debug("bus: '%s': %s: matched device %s with driver %s\n",
drv->bus->name, __func__, dev_name(dev), drv->name);
+ if (dev->probe_mutex)
+ mutex_lock(dev->probe_mutex);
+
pm_runtime_get_suppliers(dev);
if (dev->parent)
pm_runtime_get_sync(dev->parent);
@@ -804,6 +807,10 @@ static int __driver_probe_device(struct device_driver *drv, struct device *dev)
pm_runtime_put(dev->parent);
pm_runtime_put_suppliers(dev);
+
+ if (dev->probe_mutex)
+ mutex_unlock(dev->probe_mutex);
+
return ret;
}
diff --git a/drivers/of/device.c b/drivers/of/device.c
index 1ca42ad9dd15..c58c716507e8 100644
--- a/drivers/of/device.c
+++ b/drivers/of/device.c
@@ -304,3 +304,57 @@ int of_device_uevent_modalias(const struct device *dev, struct kobj_uevent_env *
return 0;
}
EXPORT_SYMBOL_GPL(of_device_uevent_modalias);
+
+struct of_mutex_list_node {
+ struct list_head list;
+ struct device_node *np;
+ struct mutex mutex;
+};
+
+static DEFINE_MUTEX(of_mutex_list_lock);
+static LIST_HEAD(of_mutex_list);
+
+/**
+ * of_device_init() - Init a OF-related elements in a new struct device
+ * @dev: the new struct device
+ *
+ * The only initialization we need done at the moment is to init the
+ * "probe_mutex" if this device is part of a mutual-exclusion-group.
+ */
+void of_device_init(struct device *dev)
+{
+ struct of_mutex_list_node *node;
+ struct device_node *mutex_np;
+
+ mutex_np = of_parse_phandle(dev->of_node, "mutual-exclusion-group", 0);
+ if (!mutex_np)
+ return;
+
+ mutex_lock(&of_mutex_list_lock);
+
+ /*
+ * Check to see if we've already created a mutex for this group. If
+ * so then we're done.
+ */
+ list_for_each_entry(node, &of_mutex_list, list) {
+ if (node->np == mutex_np) {
+ of_node_put(mutex_np);
+ dev->probe_mutex = &node->mutex;
+ goto exit;
+ }
+ }
+
+ /*
+ * We need to create a new mutex. We'll never free the memory for this
+ * (nor release the referenced to the mutual-exclusion-group node) but
+ * there is only one object per group.
+ */
+ node = kzalloc(sizeof(*node), GFP_KERNEL);
+ mutex_init(&node->mutex);
+ node->np = mutex_np;
+ list_add_tail(&node->list, &of_mutex_list);
+ dev->probe_mutex = &node->mutex;
+
+exit:
+ mutex_unlock(&of_mutex_list_lock);
+}
diff --git a/include/linux/device.h b/include/linux/device.h
index 56d93a1ffb7b..f3cecf535bca 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -672,6 +672,9 @@ struct device_physical_location {
* @iommu: Per device generic IOMMU runtime data
* @physical_location: Describes physical location of the device connection
* point in the system housing.
+ * @probe_mutex: If non-NULL, this mutex will be held during device probe
+ * to allow mutual exclusion between multiple sources of probable
+ * but non-discoverable devices with conflicting resources.
* @removable: Whether the device can be removed from the system. This
* should be set by the subsystem / bus driver that discovered
* the device.
@@ -790,6 +793,8 @@ struct device {
struct device_physical_location *physical_location;
+ struct mutex *probe_mutex;
+
enum device_removable removable;
bool offline_disabled:1;
diff --git a/include/linux/of_device.h b/include/linux/of_device.h
index 2c7a3d4bc775..8ebaf4d58ecd 100644
--- a/include/linux/of_device.h
+++ b/include/linux/of_device.h
@@ -30,6 +30,7 @@ extern ssize_t of_device_modalias(struct device *dev, char *str, ssize_t len);
extern void of_device_uevent(const struct device *dev, struct kobj_uevent_env *env);
extern int of_device_uevent_modalias(const struct device *dev, struct kobj_uevent_env *env);
+extern void of_device_init(struct device *dev);
int of_dma_configure_id(struct device *dev,
struct device_node *np,
@@ -82,6 +83,11 @@ static inline int of_dma_configure(struct device *dev,
{
return 0;
}
+
+static inline void of_device_init(struct device *dev)
+{
+}
+
#endif /* CONFIG_OF */
#endif /* _LINUX_OF_DEVICE_H */
--
2.42.0.515.g380fc7ccd1-goog
^ permalink raw reply related
* Re: [PATCH] HID: intel-ish-hid: ipc: Rework EHL OOB wakeup
From: Kai-Heng Feng @ 2023-09-21 6:08 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: srinivas pandruvada, Xu, Even, jikos@kernel.org,
benjamin.tissoires@redhat.com, linux-pm@vger.kernel.org,
linux-pci@vger.kernel.org, Lee, Jian Hui, Zhang, Lixu,
Ba, Najumon, linux-input@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <CAJZ5v0iFLxpWHW=sDZ7=Wne3Yt=8_EwhW9SeCmRP6REpVqo8rA@mail.gmail.com>
On Wed, Sep 20, 2023 at 2:00 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Tue, Sep 19, 2023 at 6:54 PM srinivas pandruvada
> <srinivas.pandruvada@linux.intel.com> wrote:
> >
> > On Tue, 2023-09-19 at 15:36 +0800, Kai-Heng Feng wrote:
> > > On Mon, Sep 18, 2023 at 11:57 PM srinivas pandruvada
> > > <srinivas.pandruvada@linux.intel.com> wrote:
> > > >
> > > > Hi Kai-Heng,
> > > > On Mon, 2023-09-18 at 09:17 +0800, Kai-Heng Feng wrote:
> > > > > Hi Even,
> > > > >
> > > > > On Mon, Sep 18, 2023 at 8:33 AM Xu, Even <even.xu@intel.com>
> > > > > wrote:
> > > > > >
> > > > > > Hi, Kai-Heng,
> > > > > >
> > > > > > I just got feedback, for testing EHL S5 wakeup feature, you
> > > > > > need
> > > > > > several steps to setup and access
> > > > > > "https://portal.devicewise.com/things/browse" to trigger wake.
> > > > > > But currently, our test account of this website are all out of
> > > > > > data.
> > > > > > So maybe you need double check with the team who required you
> > > > > > preparing the patch for the verification.
> > > > >
> > > > > The patch is to solve the GPE refcount overflow, while
> > > > > maintaining S5
> > > > > wakeup. I don't have any mean to test S5 wake.
> > > > >
> > > > The issue is not calling acpi_disable_gpe(). To reduce the scope of
> > > > change can we just add that instead of a adding new callbacks. This
> > > > way
> > > > scope is reduced.
> > >
> > > This patch does exactly the same thing by letting PCI and ACPI handle
> > > the PME and GPE.
> > > Though the change seems to be bigger, it actually reduces the duped
> > > code, while keep the S5 wakeup ability intact.
> > It may be doing the same. But with long chain of calls without
> > verification, I am not comfortable.
> > This can be another patch by itself to use the framework.
>
> I agree.
>
> Let's change one thing at a time.
>
> > But you are targeting a fix for overflow issue, which is separate from
> > the use of PCI/ACPI framework.
>
> Yes, let's fix the bug first and make things look nicer separately.
Right, please use the fix from Srinivas and I'll send a separate patch
to make things looks better.
Kai-Heng
^ permalink raw reply
* Re: [PATCH v2 1/2] dt-bindings: input: gpio-keys: Allow optional dedicated wakeirq
From: Rob Herring @ 2023-09-21 15:29 UTC (permalink / raw)
To: Tony Lindgren
Cc: Dmitry Torokhov, Krzysztof Kozlowski, Conor Dooley, linux-input,
devicetree, linux-kernel, linux-arm-kernel
In-Reply-To: <20230920115044.53098-1-tony@atomide.com>
On Wed, Sep 20, 2023 at 02:50:43PM +0300, Tony Lindgren wrote:
> Allow configuring an optional dedicated wakeirq for gpio-keys that
> some SoCs have.
>
> Let's use the common interrupt naming "irq" and "wakeup" that we already
> have in use for some drivers and subsystems like i2c framework.
>
> Note that the gpio-keys interrupt property is optional. If only a gpio
> property is specified, the driver tries to translate the gpio into an
> interrupt.
>
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
>
> Changes since v1:
>
> - Run make dt_binding_check on the binding
>
> - Add better checks for interrupt-names as suggested by Rob, it is
> now required if two interrupts are configured
>
> - Add more decription entries
>
> - Add a new example for key-wakeup
>
> ---
> .../devicetree/bindings/input/gpio-keys.yaml | 41 ++++++++++++++++++-
> 1 file changed, 40 insertions(+), 1 deletion(-)
With the indentation fixed,
Reviewed-by: Rob Herring <robh@kernel.org>
^ permalink raw reply
* Re: [PATCH] HID: intel-ish-hid: ipc: Rework EHL OOB wakeup
From: Rafael J. Wysocki @ 2023-09-21 9:33 UTC (permalink / raw)
To: Kai-Heng Feng
Cc: Rafael J. Wysocki, srinivas pandruvada, Xu, Even,
jikos@kernel.org, benjamin.tissoires@redhat.com,
linux-pm@vger.kernel.org, linux-pci@vger.kernel.org,
Lee, Jian Hui, Zhang, Lixu, Ba, Najumon,
linux-input@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <CAAd53p67tiP0xXwhn=NviU_rvrSveSxbAhDieYG9AmUWF2e__Q@mail.gmail.com>
On Thu, Sep 21, 2023 at 8:08 AM Kai-Heng Feng
<kai.heng.feng@canonical.com> wrote:
>
> On Wed, Sep 20, 2023 at 2:00 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> >
> > On Tue, Sep 19, 2023 at 6:54 PM srinivas pandruvada
> > <srinivas.pandruvada@linux.intel.com> wrote:
> > >
> > > On Tue, 2023-09-19 at 15:36 +0800, Kai-Heng Feng wrote:
> > > > On Mon, Sep 18, 2023 at 11:57 PM srinivas pandruvada
> > > > <srinivas.pandruvada@linux.intel.com> wrote:
> > > > >
> > > > > Hi Kai-Heng,
> > > > > On Mon, 2023-09-18 at 09:17 +0800, Kai-Heng Feng wrote:
> > > > > > Hi Even,
> > > > > >
> > > > > > On Mon, Sep 18, 2023 at 8:33 AM Xu, Even <even.xu@intel.com>
> > > > > > wrote:
> > > > > > >
> > > > > > > Hi, Kai-Heng,
> > > > > > >
> > > > > > > I just got feedback, for testing EHL S5 wakeup feature, you
> > > > > > > need
> > > > > > > several steps to setup and access
> > > > > > > "https://portal.devicewise.com/things/browse" to trigger wake.
> > > > > > > But currently, our test account of this website are all out of
> > > > > > > data.
> > > > > > > So maybe you need double check with the team who required you
> > > > > > > preparing the patch for the verification.
> > > > > >
> > > > > > The patch is to solve the GPE refcount overflow, while
> > > > > > maintaining S5
> > > > > > wakeup. I don't have any mean to test S5 wake.
> > > > > >
> > > > > The issue is not calling acpi_disable_gpe(). To reduce the scope of
> > > > > change can we just add that instead of a adding new callbacks. This
> > > > > way
> > > > > scope is reduced.
> > > >
> > > > This patch does exactly the same thing by letting PCI and ACPI handle
> > > > the PME and GPE.
> > > > Though the change seems to be bigger, it actually reduces the duped
> > > > code, while keep the S5 wakeup ability intact.
> > > It may be doing the same. But with long chain of calls without
> > > verification, I am not comfortable.
> > > This can be another patch by itself to use the framework.
> >
> > I agree.
> >
> > Let's change one thing at a time.
> >
> > > But you are targeting a fix for overflow issue, which is separate from
> > > the use of PCI/ACPI framework.
> >
> > Yes, let's fix the bug first and make things look nicer separately.
>
> Right, please use the fix from Srinivas and I'll send a separate patch
> to make things looks better.
OK
Srinivas, please resend the patch with a changelog etc.
^ permalink raw reply
* [PATCH] Input: synaptics-rmi4 - replace deprecated strncpy
From: Justin Stitt @ 2023-09-21 9:58 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input, linux-kernel, linux-hardening, Justin Stitt
`strncpy` is deprecated for use on NUL-terminated destination strings [1]
Let's use memcpy() as the bounds have already been checked and this
decays into a simple byte copy from one buffer to another removing any
ambiguity that strncpy has.
Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1]
Link: https://github.com/KSPP/linux/issues/90
Cc: linux-hardening@vger.kernel.org
Signed-off-by: Justin Stitt <justinstitt@google.com>
---
Note: build-tested only.
Similar to Kees' suggestion here [2]
[2]: https://lore.kernel.org/all/202309142045.7CBAE940E@keescook/
---
drivers/input/rmi4/rmi_f34.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/input/rmi4/rmi_f34.c b/drivers/input/rmi4/rmi_f34.c
index 0d9a5756e3f5..3b3ac71e53dc 100644
--- a/drivers/input/rmi4/rmi_f34.c
+++ b/drivers/input/rmi4/rmi_f34.c
@@ -471,7 +471,7 @@ static ssize_t rmi_driver_update_fw_store(struct device *dev,
if (buf[count - 1] == '\0' || buf[count - 1] == '\n')
copy_count -= 1;
- strncpy(fw_name, buf, copy_count);
+ memcpy(fw_name, buf, copy_count);
fw_name[copy_count] = '\0';
ret = request_firmware(&fw, fw_name, dev);
---
base-commit: 2cf0f715623872823a72e451243bbf555d10d032
change-id: 20230921-strncpy-drivers-input-rmi4-rmi_f34-c-4a6945316cea
Best regards,
--
Justin Stitt <justinstitt@google.com>
^ permalink raw reply related
* Re: [PATCH] hid: cp2112: Fix duplicate workqueue initialization
From: andriy.shevchenko @ 2023-09-21 9:42 UTC (permalink / raw)
To: Danny Kaehn
Cc: jikos@kernel.org, benjamin.tissoires@redhat.com,
linux-input@vger.kernel.org, Ethan Twardy
In-Reply-To: <216f73660b7c55ab247345120468d0cd9463e622.camel@plexus.com>
On Wed, Sep 20, 2023 at 07:10:15PM +0000, Danny Kaehn wrote:
> On Wed, 2023-09-20 at 16:04 +0300, Andy Shevchenko wrote:
> > On Tue, Sep 19, 2023 at 04:22:45PM -0500, Danny Kaehn wrote:
> > > Previously the cp2112 driver called INIT_DELAYED_WORK within
> > > cp2112_gpio_irq_startup, resulting in duplicate initilizations of the
> > > workqueue on subsequent IRQ startups following an initial request. This
> > > resulted in a warning in set_work_data in workqueue.c, as well as a rare
> > > NULL dereference within process_one_work in workqueue.c.
> > >
> > > Initialize the workqueue within _probe instead.
> >
> > Does it deserve a Fixes tag?
>
> I'm not sure -- it does technically fix 13de9cca514ed63604263cad87ca8cb36e9b6489
> (HID: cp2112: add IRQ chip handling), but does not apply cleanly to that
> revision (a.e. with git am)
It's not a problem.
> (my understanding is that 'Fixes' is used for stable kernel maintenance?)
Not only, for anybody to track the issues and fixes.
For stable it's more important to follow their procedure, where the Fixes
is just one small piece of.
Fixes: 13de9cca514e ("HID: cp2112: add IRQ chip handling")
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* [PATCH] Input: axp20x-pek - refactor deprecated strncpy
From: Justin Stitt @ 2023-09-21 9:17 UTC (permalink / raw)
To: Dmitry Torokhov, Chen-Yu Tsai
Cc: linux-input, linux-kernel, linux-hardening, Justin Stitt
`strncpy` is deprecated for use on NUL-terminated destination strings
[1] and as such we should prefer more robust and less ambiguous string
interfaces.
Ensuring we have a trailing NUL-byte and checking the length of bytes
copied are both intrinsic behavior of strscpy.
Therefore, a suitable replacement is `strscpy` [2] due to the fact that
it guarantees NUL-termination on the destination buffer without
unnecessarily NUL-padding.
It should be noted that the original code can silently truncate and so
can this refactoring. However, a check could be added if truncation
is an issue:
| len = strscpy(val_str, buf, sizeof(val_str));
| if (len < 0) { // add this
| return -E2BIG; // or -EINVAL
| }
Also, now check for `len > 0` instead of just a truthy `len` because
`len` is now a signed type and we could run into problems if strscpy
returned -E2BIG which would pass the truthy test.
Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1]
Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [2]
Link: https://github.com/KSPP/linux/issues/90
Cc: linux-hardening@vger.kernel.org
Signed-off-by: Justin Stitt <justinstitt@google.com>
---
Note: build-tested only.
---
drivers/input/misc/axp20x-pek.c | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/drivers/input/misc/axp20x-pek.c b/drivers/input/misc/axp20x-pek.c
index 4581606a28d6..abcf78785b45 100644
--- a/drivers/input/misc/axp20x-pek.c
+++ b/drivers/input/misc/axp20x-pek.c
@@ -134,16 +134,14 @@ static ssize_t axp20x_store_attr(struct device *dev,
{
struct axp20x_pek *axp20x_pek = dev_get_drvdata(dev);
char val_str[20];
- size_t len;
+ ssize_t len;
int ret, i;
unsigned int val, idx = 0;
unsigned int best_err = UINT_MAX;
- val_str[sizeof(val_str) - 1] = '\0';
- strncpy(val_str, buf, sizeof(val_str) - 1);
- len = strlen(val_str);
+ len = strscpy(val_str, buf, sizeof(val_str));
- if (len && val_str[len - 1] == '\n')
+ if (len > 0 && val_str[len - 1] == '\n')
val_str[len - 1] = '\0';
ret = kstrtouint(val_str, 10, &val);
---
base-commit: 2cf0f715623872823a72e451243bbf555d10d032
change-id: 20230921-strncpy-drivers-input-misc-axp20x-pek-c-3c4708924bbd
Best regards,
--
Justin Stitt <justinstitt@google.com>
^ permalink raw reply related
* [PATCH 1/2] HID: uclogic: Fix user-memory-access bug in uclogic_params_ugee_v2_init_event_hooks()
From: Jinjie Ruan @ 2023-09-21 13:38 UTC (permalink / raw)
To: jikos, benjamin.tissoires, jose.exposito89, linux-input; +Cc: ruanjinjie
In-Reply-To: <20230921133824.605700-1-ruanjinjie@huawei.com>
When CONFIG_HID_UCLOGIC=y and CONFIG_KUNIT_ALL_TESTS=y, launch kernel and
then the below user-memory-access bug occurs.
In hid_test_uclogic_params_cleanup_event_hooks(),it call
uclogic_params_ugee_v2_init_event_hooks() with the first arg=NULL, so
when it calls uclogic_params_ugee_v2_has_battery(), the hid_get_drvdata()
will access hdev->dev with hdev=NULL, which will cause below
user-memory-access.
So add a fake_device with quirks member and call hid_set_drvdata()
to assign hdev->dev->driver_data which avoids the null-ptr-def bug
for drvdata->quirks in uclogic_params_ugee_v2_has_battery(). After applying
this patch, the below user-memory-access bug never occurs.
general protection fault, probably for non-canonical address 0xdffffc0000000329: 0000 [#1] PREEMPT SMP KASAN
KASAN: probably user-memory-access in range [0x0000000000001948-0x000000000000194f]
CPU: 5 PID: 2189 Comm: kunit_try_catch Tainted: G B W N 6.6.0-rc2+ #30
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.15.0-1 04/01/2014
RIP: 0010:uclogic_params_ugee_v2_init_event_hooks+0x87/0x600
Code: f3 f3 65 48 8b 14 25 28 00 00 00 48 89 54 24 60 31 d2 48 89 fa c7 44 24 30 00 00 00 00 48 c7 44 24 28 02 f8 02 01 48 c1 ea 03 <80> 3c 02 00 0f 85 2c 04 00 00 48 8b 9d 48 19 00 00 48 b8 00 00 00
RSP: 0000:ffff88810679fc88 EFLAGS: 00010202
RAX: dffffc0000000000 RBX: 0000000000000004 RCX: 0000000000000000
RDX: 0000000000000329 RSI: ffff88810679fd88 RDI: 0000000000001948
RBP: 0000000000000000 R08: 0000000000000000 R09: ffffed1020f639f0
R10: ffff888107b1cf87 R11: 0000000000000400 R12: 1ffff11020cf3f92
R13: ffff88810679fd88 R14: ffff888100b97b08 R15: ffff8881030bb080
FS: 0000000000000000(0000) GS:ffff888119e80000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000000000 CR3: 0000000005286001 CR4: 0000000000770ee0
DR0: ffffffff8fdd6cf4 DR1: ffffffff8fdd6cf5 DR2: ffffffff8fdd6cf6
DR3: ffffffff8fdd6cf7 DR6: 00000000fffe0ff0 DR7: 0000000000000600
PKRU: 55555554
Call Trace:
<TASK>
? die_addr+0x3d/0xa0
? exc_general_protection+0x144/0x220
? asm_exc_general_protection+0x22/0x30
? uclogic_params_ugee_v2_init_event_hooks+0x87/0x600
? sched_clock_cpu+0x69/0x550
? uclogic_parse_ugee_v2_desc_gen_params+0x70/0x70
? load_balance+0x2950/0x2950
? rcu_trc_cmpxchg_need_qs+0x67/0xa0
hid_test_uclogic_params_cleanup_event_hooks+0x9e/0x1a0
? uclogic_params_ugee_v2_init_event_hooks+0x600/0x600
? __switch_to+0x5cf/0xe60
? migrate_enable+0x260/0x260
? __kthread_parkme+0x83/0x150
? kunit_try_run_case_cleanup+0xe0/0xe0
kunit_generic_run_threadfn_adapter+0x4a/0x90
? kunit_try_catch_throw+0x80/0x80
kthread+0x2b5/0x380
? kthread_complete_and_exit+0x20/0x20
ret_from_fork+0x2d/0x70
? kthread_complete_and_exit+0x20/0x20
ret_from_fork_asm+0x11/0x20
</TASK>
Modules linked in:
Dumping ftrace buffer:
(ftrace buffer empty)
---[ end trace 0000000000000000 ]---
RIP: 0010:uclogic_params_ugee_v2_init_event_hooks+0x87/0x600
Code: f3 f3 65 48 8b 14 25 28 00 00 00 48 89 54 24 60 31 d2 48 89 fa c7 44 24 30 00 00 00 00 48 c7 44 24 28 02 f8 02 01 48 c1 ea 03 <80> 3c 02 00 0f 85 2c 04 00 00 48 8b 9d 48 19 00 00 48 b8 00 00 00
RSP: 0000:ffff88810679fc88 EFLAGS: 00010202
RAX: dffffc0000000000 RBX: 0000000000000004 RCX: 0000000000000000
RDX: 0000000000000329 RSI: ffff88810679fd88 RDI: 0000000000001948
RBP: 0000000000000000 R08: 0000000000000000 R09: ffffed1020f639f0
R10: ffff888107b1cf87 R11: 0000000000000400 R12: 1ffff11020cf3f92
R13: ffff88810679fd88 R14: ffff888100b97b08 R15: ffff8881030bb080
FS: 0000000000000000(0000) GS:ffff888119e80000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000000000 CR3: 0000000005286001 CR4: 0000000000770ee0
DR0: ffffffff8fdd6cf4 DR1: ffffffff8fdd6cf5 DR2: ffffffff8fdd6cf6
DR3: ffffffff8fdd6cf7 DR6: 00000000fffe0ff0 DR7: 0000000000000600
PKRU: 55555554
Kernel panic - not syncing: Fatal exception
Dumping ftrace buffer:
(ftrace buffer empty)
Kernel Offset: disabled
Rebooting in 1 seconds..
Fixes: a251d6576d2a ("HID: uclogic: Handle wireless device reconnection")
Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
---
drivers/hid/hid-uclogic-params-test.c | 15 ++++++++++++++-
1 file changed, 14 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/hid-uclogic-params-test.c b/drivers/hid/hid-uclogic-params-test.c
index 678f50cbb160..3938bae25982 100644
--- a/drivers/hid/hid-uclogic-params-test.c
+++ b/drivers/hid/hid-uclogic-params-test.c
@@ -174,12 +174,22 @@ static void hid_test_uclogic_parse_ugee_v2_desc(struct kunit *test)
KUNIT_EXPECT_EQ(test, params->frame_type, frame_type);
}
+struct fake_device {
+ unsigned long quirks;
+};
+
static void hid_test_uclogic_params_cleanup_event_hooks(struct kunit *test)
{
int res, n;
+ struct hid_device *hdev;
+ struct fake_device *fake_dev;
struct uclogic_params p = {0, };
- res = uclogic_params_ugee_v2_init_event_hooks(NULL, &p);
+ hdev = kzalloc(sizeof(struct hid_device), GFP_KERNEL);
+ fake_dev = kzalloc(sizeof(struct fake_device), GFP_KERNEL);
+ hid_set_drvdata(hdev, fake_dev);
+
+ res = uclogic_params_ugee_v2_init_event_hooks(hdev, &p);
KUNIT_ASSERT_EQ(test, res, 0);
/* Check that the function can be called repeatedly */
@@ -187,6 +197,9 @@ static void hid_test_uclogic_params_cleanup_event_hooks(struct kunit *test)
uclogic_params_cleanup_event_hooks(&p);
KUNIT_EXPECT_PTR_EQ(test, p.event_hooks, NULL);
}
+
+ kfree(fake_dev);
+ kfree(hdev);
}
static struct kunit_case hid_uclogic_params_test_cases[] = {
--
2.34.1
^ permalink raw reply related
* [PATCH 0/2] HID: uclogic: Fix two bugs in uclogic
From: Jinjie Ruan @ 2023-09-21 13:38 UTC (permalink / raw)
To: jikos, benjamin.tissoires, jose.exposito89, linux-input; +Cc: ruanjinjie
When CONFIG_HID_UCLOGIC=y and CONFIG_KUNIT_ALL_TESTS=y, launch
kernel and then there are a user-memory-access bug and a work->entry
not empty bug. This patchset fix these issues.
Jinjie Ruan (2):
HID: uclogic: Fix user-memory-access bug in
uclogic_params_ugee_v2_init_event_hooks()
HID: uclogic: Fix a work->entry not empty bug in __queue_work()
drivers/hid/hid-uclogic-core-test.c | 7 +++++++
drivers/hid/hid-uclogic-params-test.c | 15 ++++++++++++++-
2 files changed, 21 insertions(+), 1 deletion(-)
--
2.34.1
^ permalink raw reply
* [PATCH 2/2] HID: uclogic: Fix a work->entry not empty bug in __queue_work()
From: Jinjie Ruan @ 2023-09-21 13:38 UTC (permalink / raw)
To: jikos, benjamin.tissoires, jose.exposito89, linux-input; +Cc: ruanjinjie
In-Reply-To: <20230921133824.605700-1-ruanjinjie@huawei.com>
When CONFIG_HID_UCLOGIC=y and CONFIG_KUNIT_ALL_TESTS=y, launch
kernel and then the below work->entry not empty bug occurs.
In hid_test_uclogic_exec_event_hook_test(), the filter->work is not
initialized to be added to p.event_hooks->list, and then the
schedule_work() in uclogic_exec_event_hook() will call __queue_work(),
which check whether the work->entry is empty and cause the below
warning call trace.
So call INIT_WORK() with a fake work to solve the issue. After applying
this patch, the below work->entry not empty bug never occurs.
WARNING: CPU: 0 PID: 2177 at kernel/workqueue.c:1787 __queue_work.part.0+0x780/0xad0
Modules linked in:
CPU: 0 PID: 2177 Comm: kunit_try_catch Tainted: G B W N 6.6.0-rc2+ #30
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.15.0-1 04/01/2014
RIP: 0010:__queue_work.part.0+0x780/0xad0
Code: 44 24 20 0f b6 00 84 c0 74 08 3c 03 0f 8e 52 03 00 00 f6 83 00 01 00 00 02 74 6f 4c 89 ef e8 c7 d8 f1 02 f3 90 e9 e5 f8 ff ff <0f> 0b e9 63 fc ff ff 89 e9 49 8d 57 68 4c 89 e6 4c 89 ff 83 c9 02
RSP: 0000:ffff888102bb7ce8 EFLAGS: 00010086
RAX: 0000000000000000 RBX: ffff888106b8e460 RCX: ffffffff84141cc7
RDX: 1ffff11020d71c8c RSI: 0000000000000004 RDI: ffff8881001d0118
RBP: dffffc0000000000 R08: 0000000000000001 R09: ffffed1020576f92
R10: 0000000000000003 R11: ffff888102bb7980 R12: ffff888106b8e458
R13: ffff888119c38800 R14: 0000000000000000 R15: ffff8881001d0100
FS: 0000000000000000(0000) GS:ffff888119c00000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: ffff888119506000 CR3: 0000000005286001 CR4: 0000000000770ef0
DR0: ffffffff8fdd6ce0 DR1: ffffffff8fdd6ce1 DR2: ffffffff8fdd6ce3
DR3: ffffffff8fdd6ce5 DR6: 00000000fffe0ff0 DR7: 0000000000000600
PKRU: 55555554
Call Trace:
<TASK>
? __warn+0xc9/0x260
? __queue_work.part.0+0x780/0xad0
? report_bug+0x345/0x400
? handle_bug+0x3c/0x70
? exc_invalid_op+0x14/0x40
? asm_exc_invalid_op+0x16/0x20
? _raw_spin_lock+0x87/0xe0
? __queue_work.part.0+0x780/0xad0
? __queue_work.part.0+0x249/0xad0
queue_work_on+0x48/0x50
uclogic_exec_event_hook.isra.0+0xf7/0x160
hid_test_uclogic_exec_event_hook_test+0x2f1/0x5d0
? try_to_wake_up+0x151/0x13e0
? uclogic_exec_event_hook.isra.0+0x160/0x160
? _raw_spin_lock_irqsave+0x8d/0xe0
? __sched_text_end+0xa/0xa
? __sched_text_end+0xa/0xa
? migrate_enable+0x260/0x260
? kunit_try_run_case_cleanup+0xe0/0xe0
kunit_generic_run_threadfn_adapter+0x4a/0x90
? kunit_try_catch_throw+0x80/0x80
kthread+0x2b5/0x380
? kthread_complete_and_exit+0x20/0x20
ret_from_fork+0x2d/0x70
? kthread_complete_and_exit+0x20/0x20
ret_from_fork_asm+0x11/0x20
</TASK>
Fixes: a251d6576d2a ("HID: uclogic: Handle wireless device reconnection")
Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
---
drivers/hid/hid-uclogic-core-test.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/drivers/hid/hid-uclogic-core-test.c b/drivers/hid/hid-uclogic-core-test.c
index 2bb916226a38..cb274cde3ad2 100644
--- a/drivers/hid/hid-uclogic-core-test.c
+++ b/drivers/hid/hid-uclogic-core-test.c
@@ -56,6 +56,11 @@ static struct uclogic_raw_event_hook_test test_events[] = {
},
};
+static void fake_work(struct work_struct *work)
+{
+
+}
+
static void hid_test_uclogic_exec_event_hook_test(struct kunit *test)
{
struct uclogic_params p = {0, };
@@ -77,6 +82,8 @@ static void hid_test_uclogic_exec_event_hook_test(struct kunit *test)
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, filter->event);
memcpy(filter->event, &hook_events[n].event[0], filter->size);
+ INIT_WORK(&filter->work, fake_work);
+
list_add_tail(&filter->list, &p.event_hooks->list);
}
--
2.34.1
^ permalink raw reply related
* [PATCH] HID: lenovo: Fix middle-button behaviour for system suspend
From: Martin Kepplinger @ 2023-09-21 9:21 UTC (permalink / raw)
To: jikos, benjamin.tissoires, jm, linux-kernel
Cc: linux-input, Martin Kepplinger
After system suspend the middle-button mode is being reset to
compatibility mode which simply breaks functionality for the devices
where native mode is configured during probe().
Fix this by setting native mode in reset_resume() for the appropriate
devices.
Fixes: 94eefa271323 ("HID: lenovo: Use native middle-button mode for compact keyboards")
Signed-off-by: Martin Kepplinger <martink@posteo.de>
---
drivers/hid/hid-lenovo.c | 25 +++++++++++++++++++++++++
1 file changed, 25 insertions(+)
diff --git a/drivers/hid/hid-lenovo.c b/drivers/hid/hid-lenovo.c
index 44763c0da444..d20562b9eca6 100644
--- a/drivers/hid/hid-lenovo.c
+++ b/drivers/hid/hid-lenovo.c
@@ -1344,6 +1344,28 @@ static int lenovo_input_configured(struct hid_device *hdev,
return 0;
}
+static int __maybe_unused lenovo_resume(struct hid_device *hdev)
+{
+ int ret;
+
+ switch (hdev->product) {
+ case USB_DEVICE_ID_LENOVO_CUSBKBD:
+ case USB_DEVICE_ID_LENOVO_CBTKBD:
+ case USB_DEVICE_ID_LENOVO_TPIIUSBKBD:
+ case USB_DEVICE_ID_LENOVO_TPIIBTKBD:
+ /* Switch middle button to native mode again */
+ ret = lenovo_send_cmd_cptkbd(hdev, 0x09, 0x01);
+ if (ret)
+ hid_warn(hdev, "Failed to switch middle button: %d\n",
+ ret);
+ break;
+ default:
+ ret = 0;
+ break;
+ }
+
+ return ret;
+}
static const struct hid_device_id lenovo_devices[] = {
{ HID_USB_DEVICE(USB_VENDOR_ID_LENOVO, USB_DEVICE_ID_LENOVO_TPKBD) },
@@ -1380,6 +1402,9 @@ static struct hid_driver lenovo_driver = {
.raw_event = lenovo_raw_event,
.event = lenovo_event,
.report_fixup = lenovo_report_fixup,
+#ifdef CONFIG_PM
+ .reset_resume = lenovo_resume,
+#endif
};
module_hid_driver(lenovo_driver);
--
2.39.2
^ permalink raw reply related
* Re: [PATCH 04/52] input: iqs62x-keys - Convert to platform remove callback returning void
From: Jeff LaBundy @ 2023-09-21 11:29 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Dmitry Torokhov, Mattijs Korpershoek, linux-input, kernel
In-Reply-To: <20230920125829.1478827-5-u.kleine-koenig@pengutronix.de>
On Wed, Sep 20, 2023 at 02:57:41PM +0200, Uwe Kleine-König wrote:
> The .remove() callback for a platform driver returns an int which makes
> many driver authors wrongly assume it's possible to do error handling by
> returning an error code. However the value returned is ignored (apart
> from emitting a warning) and this typically results in resource leaks.
> To improve here there is a quest to make the remove callback return
> void. In the first step of this quest all drivers are converted to
> .remove_new() which already returns void. Eventually after all drivers
> are converted, .remove_new() will be renamed to .remove().
>
> Trivially convert this driver from always returning zero in the remove
> callback to the void returning variant.
>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Reviewed-by: Jeff LaBundy <jeff@labundy.com>
> ---
> drivers/input/keyboard/iqs62x-keys.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/input/keyboard/iqs62x-keys.c b/drivers/input/keyboard/iqs62x-keys.c
> index 02ceebad7bda..688d61244b5f 100644
> --- a/drivers/input/keyboard/iqs62x-keys.c
> +++ b/drivers/input/keyboard/iqs62x-keys.c
> @@ -310,7 +310,7 @@ static int iqs62x_keys_probe(struct platform_device *pdev)
> return ret;
> }
>
> -static int iqs62x_keys_remove(struct platform_device *pdev)
> +static void iqs62x_keys_remove(struct platform_device *pdev)
> {
> struct iqs62x_keys_private *iqs62x_keys = platform_get_drvdata(pdev);
> int ret;
> @@ -319,8 +319,6 @@ static int iqs62x_keys_remove(struct platform_device *pdev)
> &iqs62x_keys->notifier);
> if (ret)
> dev_err(&pdev->dev, "Failed to unregister notifier: %d\n", ret);
> -
> - return 0;
> }
>
> static struct platform_driver iqs62x_keys_platform_driver = {
> @@ -328,7 +326,7 @@ static struct platform_driver iqs62x_keys_platform_driver = {
> .name = "iqs62x-keys",
> },
> .probe = iqs62x_keys_probe,
> - .remove = iqs62x_keys_remove,
> + .remove_new = iqs62x_keys_remove,
> };
> module_platform_driver(iqs62x_keys_platform_driver);
>
> --
> 2.40.1
>
^ permalink raw reply
* [PATCH v7] HID: mcp2200: added driver for GPIOs of MCP2200
From: Johannes Roith @ 2023-09-21 16:49 UTC (permalink / raw)
To: jikos
Cc: sergeantsagara, andi.shyti, benjamin.tissoires,
christophe.jaillet, johannes, linux-input, linux-kernel, rdunlap
Added a gpiochip compatible driver to control the 8 GPIOs of
the MCP2200 by using the HID interface.
Using GPIOs with alternative functions (GP0<->SSPND, GP1<->USBCFG,
GP6<->RXLED, GP7<->TXLED) will reset the functions, if set (unset by
default).
The driver was tested while also using the UART of the chip. Setting
and reading the GPIOs has no effect on the UART communication. However,
a reset is triggered after the CONFIGURE command. If the GPIO Direction
is constantly changed, this will affect the communication at low baud
rates. This is a hardware problem of the MCP2200 and is not caused by
the driver.
Signed-off-by: Johannes Roith <johannes@gnu-linux.rocks>
---
drivers/hid/Kconfig | 9 +
drivers/hid/Makefile | 1 +
drivers/hid/hid-ids.h | 1 +
drivers/hid/hid-mcp2200.c | 392 ++++++++++++++++++++++++++++++++++++++
4 files changed, 403 insertions(+)
create mode 100644 drivers/hid/hid-mcp2200.c
diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index 0cea301cc9a9..3c14644b593d 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -1298,6 +1298,15 @@ config HID_ALPS
Say Y here if you have a Alps touchpads over i2c-hid or usbhid
and want support for its special functionalities.
+config HID_MCP2200
+ tristate "Microchip MCP2200 HID USB-to-GPIO bridge"
+ depends on USB_HID && GPIOLIB
+ help
+ Provides GPIO functionality over USB-HID through MCP2200 device.
+
+ To compile this driver as a module, choose M here: the module
+ will be called hid-mcp2200.ko.
+
config HID_MCP2221
tristate "Microchip MCP2221 HID USB-to-I2C/SMbus host support"
depends on USB_HID && I2C
diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
index 8a06d0f840bc..082a728eac60 100644
--- a/drivers/hid/Makefile
+++ b/drivers/hid/Makefile
@@ -79,6 +79,7 @@ obj-$(CONFIG_HID_LOGITECH_HIDPP) += hid-logitech-hidpp.o
obj-$(CONFIG_HID_MACALLY) += hid-macally.o
obj-$(CONFIG_HID_MAGICMOUSE) += hid-magicmouse.o
obj-$(CONFIG_HID_MALTRON) += hid-maltron.o
+obj-$(CONFIG_HID_MCP2200) += hid-mcp2200.o
obj-$(CONFIG_HID_MCP2221) += hid-mcp2221.o
obj-$(CONFIG_HID_MAYFLASH) += hid-mf.o
obj-$(CONFIG_HID_MEGAWORLD_FF) += hid-megaworld.o
diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index 7e499992a793..bb87ad4eb2aa 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -915,6 +915,7 @@
#define USB_DEVICE_ID_PICK16F1454 0x0042
#define USB_DEVICE_ID_PICK16F1454_V2 0xf2f7
#define USB_DEVICE_ID_LUXAFOR 0xf372
+#define USB_DEVICE_ID_MCP2200 0x00df
#define USB_DEVICE_ID_MCP2221 0x00dd
#define USB_VENDOR_ID_MICROSOFT 0x045e
diff --git a/drivers/hid/hid-mcp2200.c b/drivers/hid/hid-mcp2200.c
new file mode 100644
index 000000000000..bf57f7f6caa0
--- /dev/null
+++ b/drivers/hid/hid-mcp2200.c
@@ -0,0 +1,392 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * MCP2200 - Microchip USB to GPIO bridge
+ *
+ * Copyright (c) 2023, Johannes Roith <johannes@gnu-linux.rocks>
+ *
+ * Datasheet: https://ww1.microchip.com/downloads/en/DeviceDoc/22228A.pdf
+ * App Note for HID: https://ww1.microchip.com/downloads/en/DeviceDoc/93066A.pdf
+ */
+#include <linux/completion.h>
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/gpio/driver.h>
+#include <linux/hid.h>
+#include <linux/hidraw.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include "hid-ids.h"
+
+/* Commands codes in a raw output report */
+#define SET_CLEAR_OUTPUTS 0x08
+#define CONFIGURE 0x10
+#define READ_EE 0x20
+#define WRITE_EE 0x40
+#define READ_ALL 0x80
+
+/* MCP GPIO direction encoding */
+enum MCP_IO_DIR {
+ MCP2200_DIR_OUT = 0x00,
+ MCP2200_DIR_IN = 0x01,
+};
+
+/* Altternative pin assignments */
+#define TXLED 2
+#define RXLED 3
+#define USBCFG 6
+#define SSPND 7
+#define MCP_NGPIO 8
+
+/* CMD to set or clear a GPIO output */
+struct mcp_set_clear_outputs {
+ u8 cmd;
+ u8 dummys1[10];
+ u8 set_bmap;
+ u8 clear_bmap;
+ u8 dummys2[3];
+} __packed;
+
+/* CMD to configure the IOs */
+struct mcp_configure {
+ u8 cmd;
+ u8 dummys1[3];
+ u8 io_bmap;
+ u8 config_alt_pins;
+ u8 io_default_val_bmap;
+ u8 config_alt_options;
+ u8 baud_h;
+ u8 baud_l;
+ u8 dummys2[6];
+} __packed;
+
+/* CMD to read all parameters */
+struct mcp_read_all {
+ u8 cmd;
+ u8 dummys[15];
+} __packed;
+
+/* Response to the read all cmd */
+struct mcp_read_all_resp {
+ u8 cmd;
+ u8 eep_addr;
+ u8 dummy;
+ u8 eep_val;
+ u8 io_bmap;
+ u8 config_alt_pins;
+ u8 io_default_val_bmap;
+ u8 config_alt_options;
+ u8 baud_h;
+ u8 baud_l;
+ u8 io_port_val_bmap;
+ u8 dummys[5];
+} __packed;
+
+struct mcp2200 {
+ struct hid_device *hdev;
+ struct mutex lock;
+ struct completion wait_in_report;
+ u8 gpio_dir;
+ u8 gpio_val;
+ u8 gpio_inval;
+ u8 baud_h;
+ u8 baud_l;
+ u8 config_alt_pins;
+ u8 gpio_reset_val;
+ u8 config_alt_options;
+ int status;
+ struct gpio_chip gc;
+ u8 hid_report[16];
+};
+
+/* this executes the READ_ALL cmd */
+static int mcp_cmd_read_all(struct mcp2200 *mcp)
+{
+ struct mcp_read_all *read_all;
+ int len, t;
+
+ reinit_completion(&mcp->wait_in_report);
+
+ mutex_lock(&mcp->lock);
+
+ read_all = (struct mcp_read_all *) mcp->hid_report;
+ read_all->cmd = READ_ALL;
+ len = hid_hw_output_report(mcp->hdev, (u8 *) read_all,
+ sizeof(struct mcp_read_all));
+
+ mutex_unlock(&mcp->lock);
+
+ if (len != sizeof(struct mcp_read_all))
+ return -EINVAL;
+
+ t = wait_for_completion_timeout(&mcp->wait_in_report,
+ msecs_to_jiffies(4000));
+ if (!t)
+ return -ETIMEDOUT;
+
+ /* return status, negative value if wrong response was received */
+ return mcp->status;
+}
+
+static void mcp_set_multiple(struct gpio_chip *gc, unsigned long *mask,
+ unsigned long *bits)
+{
+ struct mcp2200 *mcp = gpiochip_get_data(gc);
+ u8 value;
+ int status;
+ struct mcp_set_clear_outputs *cmd;
+
+ mutex_lock(&mcp->lock);
+ cmd = (struct mcp_set_clear_outputs *) mcp->hid_report;
+
+ value = mcp->gpio_val & ~*mask;
+ value |= (*mask & *bits);
+
+ cmd->cmd = SET_CLEAR_OUTPUTS;
+ cmd->set_bmap = value;
+ cmd->clear_bmap = ~(value);
+
+ status = hid_hw_output_report(mcp->hdev, (u8 *) cmd,
+ sizeof(struct mcp_set_clear_outputs));
+
+ if (status == sizeof(struct mcp_set_clear_outputs))
+ mcp->gpio_val = value;
+
+ mutex_unlock(&mcp->lock);
+}
+
+static void mcp_set(struct gpio_chip *gc, unsigned int gpio_nr, int value)
+{
+ unsigned long mask = 1 << gpio_nr;
+ unsigned long bmap_value = value << gpio_nr;
+
+ mcp_set_multiple(gc, &mask, &bmap_value);
+}
+
+static int mcp_get_multiple(struct gpio_chip *gc, unsigned long *mask,
+ unsigned long *bits)
+{
+ u32 val;
+ struct mcp2200 *mcp = gpiochip_get_data(gc);
+ int status;
+
+ status = mcp_cmd_read_all(mcp);
+ if (status)
+ return status;
+
+ val = mcp->gpio_inval;
+ *bits = (val & *mask);
+ return 0;
+}
+
+static int mcp_get(struct gpio_chip *gc, unsigned int gpio_nr)
+{
+ unsigned long mask = 0, bits = 0;
+
+ mask = (1 << gpio_nr);
+ mcp_get_multiple(gc, &mask, &bits);
+ return bits > 0;
+}
+
+static int mcp_get_direction(struct gpio_chip *gc, unsigned int gpio_nr)
+{
+ struct mcp2200 *mcp = gpiochip_get_data(gc);
+
+ return (mcp->gpio_dir & (MCP2200_DIR_IN << gpio_nr))
+ ? GPIO_LINE_DIRECTION_IN : GPIO_LINE_DIRECTION_OUT;
+}
+
+static int mcp_set_direction(struct gpio_chip *gc, unsigned int gpio_nr,
+ enum MCP_IO_DIR io_direction)
+{
+ struct mcp2200 *mcp = gpiochip_get_data(gc);
+ struct mcp_configure *conf;
+ int status;
+ /* after the configure cmd we will need to set the outputs again */
+ unsigned long mask = ~(mcp->gpio_dir); /* only set outputs */
+ unsigned long bits = mcp->gpio_val;
+ /* Offsets of alternative pins in config_alt_pins, 0 is not used */
+ u8 alt_pin_conf[8] = {SSPND, USBCFG, 0, 0, 0, 0, RXLED, TXLED};
+ u8 config_alt_pins = mcp->config_alt_pins;
+
+ /* Read in the reset baudrate first, we need it later */
+ status = mcp_cmd_read_all(mcp);
+ if (status != 0)
+ return status;
+
+ mutex_lock(&mcp->lock);
+ conf = (struct mcp_configure *) mcp->hid_report;
+
+ /* configure will reset the chip! */
+ conf->cmd = CONFIGURE;
+ conf->io_bmap = (mcp->gpio_dir & ~(1 << gpio_nr))
+ | (io_direction << gpio_nr);
+ /* Don't overwrite the reset parameters */
+ conf->baud_h = mcp->baud_h;
+ conf->baud_l = mcp->baud_l;
+ conf->config_alt_options = mcp->config_alt_options;
+ conf->io_default_val_bmap = mcp->gpio_reset_val;
+ /* Adjust alt. func if necessary */
+ if (alt_pin_conf[gpio_nr])
+ config_alt_pins &= ~(1 << alt_pin_conf[gpio_nr]);
+ conf->config_alt_pins = config_alt_pins;
+
+ status = hid_hw_output_report(mcp->hdev, (u8 *) conf,
+ sizeof(struct mcp_set_clear_outputs));
+
+ if (status == sizeof(struct mcp_set_clear_outputs)) {
+ mcp->gpio_dir = conf->io_bmap;
+ mcp->config_alt_pins = config_alt_pins;
+ } else {
+ mutex_unlock(&mcp->lock);
+ return -EIO;
+ }
+
+ mutex_unlock(&mcp->lock);
+
+ /* Configure CMD will clear all IOs -> rewrite them */
+ mcp_set_multiple(gc, &mask, &bits);
+ return 0;
+}
+
+static int mcp_direction_input(struct gpio_chip *gc, unsigned int gpio_nr)
+{
+ return mcp_set_direction(gc, gpio_nr, MCP2200_DIR_IN);
+}
+
+static int mcp_direction_output(struct gpio_chip *gc, unsigned int gpio_nr,
+ int value)
+{
+ int ret;
+ unsigned long mask, bmap_value;
+
+ mask = 1 << gpio_nr;
+ bmap_value = value << gpio_nr;
+
+ ret = mcp_set_direction(gc, gpio_nr, MCP2200_DIR_OUT);
+ if (!ret)
+ mcp_set_multiple(gc, &mask, &bmap_value);
+ return ret;
+}
+
+static const struct gpio_chip template_chip = {
+ .label = "mcp2200",
+ .owner = THIS_MODULE,
+ .get_direction = mcp_get_direction,
+ .direction_input = mcp_direction_input,
+ .direction_output = mcp_direction_output,
+ .set = mcp_set,
+ .set_multiple = mcp_set_multiple,
+ .get = mcp_get,
+ .get_multiple = mcp_get_multiple,
+ .base = -1,
+ .ngpio = MCP_NGPIO,
+ .can_sleep = true,
+};
+
+/*
+ * MCP2200 uses interrupt endpoint for input reports. This function
+ * is called by HID layer when it receives i/p report from mcp2200,
+ * which is actually a response to the previously sent command.
+ */
+static int mcp2200_raw_event(struct hid_device *hdev, struct hid_report *report,
+ u8 *data, int size)
+{
+ struct mcp2200 *mcp = hid_get_drvdata(hdev);
+ struct mcp_read_all_resp *all_resp;
+
+ switch (data[0]) {
+ case READ_ALL:
+ all_resp = (struct mcp_read_all_resp *) data;
+ mcp->status = 0;
+ mcp->gpio_inval = all_resp->io_port_val_bmap;
+ mcp->baud_h = all_resp->baud_h;
+ mcp->baud_l = all_resp->baud_l;
+ mcp->gpio_reset_val = all_resp->io_default_val_bmap;
+ mcp->config_alt_pins = all_resp->config_alt_pins;
+ mcp->config_alt_options = all_resp->config_alt_options;
+ break;
+ default:
+ mcp->status = -EIO;
+ break;
+ }
+
+ complete(&mcp->wait_in_report);
+ return 0;
+}
+
+static int mcp2200_probe(struct hid_device *hdev, const struct hid_device_id *id)
+{
+ int ret;
+ struct mcp2200 *mcp;
+
+ mcp = devm_kzalloc(&hdev->dev, sizeof(*mcp), GFP_KERNEL);
+ if (!mcp)
+ return -ENOMEM;
+
+ ret = hid_parse(hdev);
+ if (ret) {
+ hid_err(hdev, "can't parse reports\n");
+ return ret;
+ }
+
+ ret = hid_hw_start(hdev, 0);
+ if (ret) {
+ hid_err(hdev, "can't start hardware\n");
+ return ret;
+ }
+
+ hid_info(hdev, "USB HID v%x.%02x Device [%s] on %s\n", hdev->version >> 8,
+ hdev->version & 0xff, hdev->name, hdev->phys);
+
+ ret = hid_hw_open(hdev);
+ if (ret) {
+ hid_err(hdev, "can't open device\n");
+ hid_hw_stop(hdev);
+ return ret;
+ }
+
+ mutex_init(&mcp->lock);
+ init_completion(&mcp->wait_in_report);
+ hid_set_drvdata(hdev, mcp);
+ mcp->hdev = hdev;
+
+ mcp->gc = template_chip;
+ mcp->gc.parent = &hdev->dev;
+
+ ret = devm_gpiochip_add_data(&hdev->dev, &mcp->gc, mcp);
+ if (ret < 0) {
+ hid_err(hdev, "Unable to register gpiochip\n");
+ hid_hw_close(hdev);
+ hid_hw_stop(hdev);
+ return ret;
+ }
+
+ return 0;
+}
+
+static void mcp2200_remove(struct hid_device *hdev)
+{
+ hid_hw_close(hdev);
+ hid_hw_stop(hdev);
+}
+
+static const struct hid_device_id mcp2200_devices[] = {
+ { HID_USB_DEVICE(USB_VENDOR_ID_MICROCHIP, USB_DEVICE_ID_MCP2200) },
+ { }
+};
+MODULE_DEVICE_TABLE(hid, mcp2200_devices);
+
+static struct hid_driver mcp2200_driver = {
+ .name = "mcp2200",
+ .id_table = mcp2200_devices,
+ .probe = mcp2200_probe,
+ .remove = mcp2200_remove,
+ .raw_event = mcp2200_raw_event,
+};
+
+/* Register with HID core */
+module_hid_driver(mcp2200_driver);
+
+MODULE_AUTHOR("Johannes Roith <johannes@gnu-linux.rocks>");
+MODULE_DESCRIPTION("MCP2200 Microchip HID USB to GPIO bridge");
+MODULE_LICENSE("GPL");
--
2.42.0
^ permalink raw reply related
* Re: [PATCH 02/52] input: cros_ec_keyb - Convert to platform remove callback returning void
From: Tzung-Bi Shih @ 2023-09-21 3:21 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Dmitry Torokhov, Benson Leung, Guenter Roeck, Greg Kroah-Hartman,
Jonathan Cameron, joewu (吳仲振), linux-input,
chrome-platform, kernel
In-Reply-To: <20230920125829.1478827-3-u.kleine-koenig@pengutronix.de>
On Wed, Sep 20, 2023 at 02:57:39PM +0200, Uwe Kleine-König wrote:
> The .remove() callback for a platform driver returns an int which makes
> many driver authors wrongly assume it's possible to do error handling by
> returning an error code. However the value returned is ignored (apart
> from emitting a warning) and this typically results in resource leaks.
> To improve here there is a quest to make the remove callback return
> void. In the first step of this quest all drivers are converted to
> .remove_new() which already returns void. Eventually after all drivers
> are converted, .remove_new() will be renamed to .remove().
>
> Trivially convert this driver from always returning zero in the remove
> callback to the void returning variant.
>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Reviewed-by: Tzung-Bi Shih <tzungbi@kernel.org>
^ permalink raw reply
* Re: [PATCH v6] HID: mcp2200: added driver for GPIOs of MCP2200
From: Rahul Rameshbabu @ 2023-09-21 2:43 UTC (permalink / raw)
To: Johannes Roith
Cc: jikos, andi.shyti, benjamin.tissoires, christophe.jaillet,
linux-input, linux-kernel, rdunlap
In-Reply-To: <20230918151644.568998-1-johannes@gnu-linux.rocks>
Hi Johannes,
On Mon, 18 Sep, 2023 17:16:44 +0200 "Johannes Roith" <johannes@gnu-linux.rocks> wrote:
> Added a gpiochip compatible driver to control the 8 GPIOs of
> the MCP2200 by using the HID interface.
>
> Using GPIOs with alternative functions (GP0<->SSPND, GP1<->USBCFG,
> GP6<->RXLED, GP7<->TXLED) will reset the functions, if set (unset by
> default).
>
> The driver was tested while also using the UART of the chip. Setting
> and reading the GPIOs has no effect on the UART communication. However,
> a reset is triggered after the CONFIGURE command. If the GPIO Direction
> is constantly changed, this will affect the communication at low baud
> rates. This is a hardware problem of the MCP2200 and is not caused by
> the driver.
>
> Signed-off-by: Johannes Roith <johannes@gnu-linux.rocks>
> ---
> drivers/hid/Kconfig | 9 +
> drivers/hid/Makefile | 1 +
> drivers/hid/hid-ids.h | 1 +
> drivers/hid/hid-mcp2200.c | 390 ++++++++++++++++++++++++++++++++++++++
> 4 files changed, 401 insertions(+)
> create mode 100644 drivers/hid/hid-mcp2200.c
>
> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> index 0cea301cc9a9..3c14644b593d 100644
> --- a/drivers/hid/Kconfig
> +++ b/drivers/hid/Kconfig
> @@ -1298,6 +1298,15 @@ config HID_ALPS
> Say Y here if you have a Alps touchpads over i2c-hid or usbhid
> and want support for its special functionalities.
>
> +config HID_MCP2200
> + tristate "Microchip MCP2200 HID USB-to-GPIO bridge"
> + depends on USB_HID && GPIOLIB
> + help
> + Provides GPIO functionality over USB-HID through MCP2200 device.
> +
> + To compile this driver as a module, choose M here: the module
> + will be called hid-mcp2200.ko.
> +
> config HID_MCP2221
> tristate "Microchip MCP2221 HID USB-to-I2C/SMbus host support"
> depends on USB_HID && I2C
> diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
> index 8a06d0f840bc..082a728eac60 100644
> --- a/drivers/hid/Makefile
> +++ b/drivers/hid/Makefile
> @@ -79,6 +79,7 @@ obj-$(CONFIG_HID_LOGITECH_HIDPP) += hid-logitech-hidpp.o
> obj-$(CONFIG_HID_MACALLY) += hid-macally.o
> obj-$(CONFIG_HID_MAGICMOUSE) += hid-magicmouse.o
> obj-$(CONFIG_HID_MALTRON) += hid-maltron.o
> +obj-$(CONFIG_HID_MCP2200) += hid-mcp2200.o
> obj-$(CONFIG_HID_MCP2221) += hid-mcp2221.o
> obj-$(CONFIG_HID_MAYFLASH) += hid-mf.o
> obj-$(CONFIG_HID_MEGAWORLD_FF) += hid-megaworld.o
> diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
> index 7e499992a793..bb87ad4eb2aa 100644
> --- a/drivers/hid/hid-ids.h
> +++ b/drivers/hid/hid-ids.h
> @@ -915,6 +915,7 @@
> #define USB_DEVICE_ID_PICK16F1454 0x0042
> #define USB_DEVICE_ID_PICK16F1454_V2 0xf2f7
> #define USB_DEVICE_ID_LUXAFOR 0xf372
> +#define USB_DEVICE_ID_MCP2200 0x00df
> #define USB_DEVICE_ID_MCP2221 0x00dd
>
> #define USB_VENDOR_ID_MICROSOFT 0x045e
> diff --git a/drivers/hid/hid-mcp2200.c b/drivers/hid/hid-mcp2200.c
> new file mode 100644
> index 000000000000..477a3915d2f0
> --- /dev/null
> +++ b/drivers/hid/hid-mcp2200.c
> @@ -0,0 +1,390 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * MCP2200 - Microchip USB to GPIO bridge
> + *
> + * Copyright (c) 2023, Johannes Roith <johannes@gnu-linux.rocks>
> + *
> + * Datasheet: https://ww1.microchip.com/downloads/en/DeviceDoc/22228A.pdf
> + * App Note for HID: https://ww1.microchip.com/downloads/en/DeviceDoc/93066A.pdf
> + */
> +#include <linux/completion.h>
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/gpio/driver.h>
> +#include <linux/hid.h>
> +#include <linux/hidraw.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include "hid-ids.h"
> +
> +/* Commands codes in a raw output report */
> +#define SET_CLEAR_OUTPUTS 0x08
> +#define CONFIGURE 0x10
> +#define READ_EE 0x20
> +#define WRITE_EE 0x40
> +#define READ_ALL 0x80
> +
> +/* MCP GPIO direction encoding */
> +enum MCP_IO_DIR {
> + MCP2200_DIR_OUT = 0x00,
> + MCP2200_DIR_IN = 0x01,
> +};
> +
> +/* Altternative pin assignments */
> +#define TXLED 2
> +#define RXLED 3
> +#define USBCFG 6
> +#define SSPND 7
> +#define MCP_NGPIO 8
> +
> +/* CMD to set or clear a GPIO output */
> +struct mcp_set_clear_outputs {
> + u8 cmd;
> + u8 dummys1[10];
> + u8 set_bmap;
> + u8 clear_bmap;
> + u8 dummys2[3];
> +} __packed;
> +
> +/* CMD to configure the IOs */
> +struct mcp_configure {
> + u8 cmd;
> + u8 dummys1[3];
> + u8 io_bmap;
> + u8 config_alt_pins;
> + u8 io_default_val_bmap;
> + u8 config_alt_options;
> + u8 baud_h;
> + u8 baud_l;
> + u8 dummys2[6];
> +} __packed;
> +
> +/* CMD to read all parameters */
> +struct mcp_read_all {
> + u8 cmd;
> + u8 dummys[15];
> +} __packed;
> +
> +/* Response to the read all cmd */
> +struct mcp_read_all_resp {
> + u8 cmd;
> + u8 eep_addr;
> + u8 dummy;
> + u8 eep_val;
> + u8 io_bmap;
> + u8 config_alt_pins;
> + u8 io_default_val_bmap;
> + u8 config_alt_options;
> + u8 baud_h;
> + u8 baud_l;
> + u8 io_port_val_bmap;
> + u8 dummys[5];
> +} __packed;
> +
> +struct mcp2200 {
> + struct hid_device *hdev;
> + struct mutex lock;
> + struct completion wait_in_report;
> + u8 gpio_dir;
> + u8 gpio_val;
> + u8 gpio_inval;
> + u8 baud_h;
> + u8 baud_l;
> + u8 config_alt_pins;
> + u8 gpio_reset_val;
> + u8 config_alt_options;
> + int status;
> + struct gpio_chip gc;
> + u8 hid_report[16];
> +};
> +
> +/* this executes the READ_ALL cmd */
> +static int mcp_cmd_read_all(struct mcp2200 *mcp)
> +{
> + struct mcp_read_all *read_all;
> + int len, t;
> +
> + reinit_completion(&mcp->wait_in_report);
> +
> + mutex_lock(&mcp->lock);
> +
> + read_all = (struct mcp_read_all *) mcp->hid_report;
> + read_all->cmd = READ_ALL;
> + len = hid_hw_output_report(mcp->hdev, (u8 *) read_all,
> + sizeof(struct mcp_read_all));
> +
> + mutex_unlock(&mcp->lock);
> +
> + if (len != sizeof(struct mcp_read_all))
> + return -EINVAL;
> +
> + t = wait_for_completion_timeout(&mcp->wait_in_report,
> + msecs_to_jiffies(4000));
> + if (!t)
> + return -ETIMEDOUT;
> +
> + /* return status, negative value if wrong response was received */
> + return mcp->status;
> +}
> +
> +static void mcp_set_multiple(struct gpio_chip *gc, unsigned long *mask,
> + unsigned long *bits)
> +{
> + struct mcp2200 *mcp = gpiochip_get_data(gc);
> + u8 value;
> + int status;
> + struct mcp_set_clear_outputs *cmd;
> +
> + mutex_lock(&mcp->lock);
> + cmd = (struct mcp_set_clear_outputs *) mcp->hid_report;
> +
> + value = mcp->gpio_val & ~*mask;
> + value |= (*mask & *bits);
> +
> + cmd->cmd = SET_CLEAR_OUTPUTS;
> + cmd->set_bmap = value;
> + cmd->clear_bmap = ~(value);
> +
> + status = hid_hw_output_report(mcp->hdev, (u8 *) cmd,
> + sizeof(struct mcp_set_clear_outputs));
> +
> + if (status == sizeof(struct mcp_set_clear_outputs))
> + mcp->gpio_val = value;
> +
> + mutex_unlock(&mcp->lock);
> +}
> +
> +static void mcp_set(struct gpio_chip *gc, unsigned int gpio_nr, int value)
> +{
> + unsigned long mask = 1 << gpio_nr;
> + unsigned long bmap_value = value << gpio_nr;
> +
> + mcp_set_multiple(gc, &mask, &bmap_value);
> +}
> +
> +static int mcp_get_multiple(struct gpio_chip *gc, unsigned long *mask,
> + unsigned long *bits)
> +{
> + u32 val;
> + struct mcp2200 *mcp = gpiochip_get_data(gc);
> + int status;
> +
> + status = mcp_cmd_read_all(mcp);
> + if (status)
> + return status;
> +
> + val = mcp->gpio_inval;
> + *bits = (val & *mask);
> + return 0;
> +}
> +
> +static int mcp_get(struct gpio_chip *gc, unsigned int gpio_nr)
> +{
> + unsigned long mask = 0, bits = 0;
> +
> + mask = (1 << gpio_nr);
> + mcp_get_multiple(gc, &mask, &bits);
> + return bits > 0;
> +}
> +
> +static int mcp_get_direction(struct gpio_chip *gc, unsigned int gpio_nr)
> +{
> + struct mcp2200 *mcp = gpiochip_get_data(gc);
> +
> + return (mcp->gpio_dir & (MCP2200_DIR_IN << gpio_nr))
> + ? GPIO_LINE_DIRECTION_IN : GPIO_LINE_DIRECTION_OUT;
> +}
> +
> +static int mcp_set_direction(struct gpio_chip *gc, unsigned int gpio_nr,
> + enum MCP_IO_DIR io_direction)
> +{
> + struct mcp2200 *mcp = gpiochip_get_data(gc);
> + struct mcp_configure *conf;
> + int status;
> + /* after the configure cmd we will need to set the outputs again */
> + unsigned long mask = ~(mcp->gpio_dir); /* only set outputs */
> + unsigned long bits = mcp->gpio_val;
> + /* Offsets of alternative pins in config_alt_pins, 0 is not used */
> + u8 alt_pin_conf[8] = {SSPND, USBCFG, 0, 0, 0, 0, RXLED, TXLED};
> + u8 config_alt_pins = mcp->config_alt_pins;
> +
> + /* Read in the reset baudrate first, we need it later */
> + status = mcp_cmd_read_all(mcp);
> + if (status != 0)
> + return status;
> +
> + mutex_lock(&mcp->lock);
> + conf = (struct mcp_configure *) mcp->hid_report;
> +
> + /* configure will reset the chip! */
> + conf->cmd = CONFIGURE;
> + conf->io_bmap = (mcp->gpio_dir & ~(1 << gpio_nr))
> + | (io_direction << gpio_nr);
> + /* Don't overwrite the reset parameters */
> + conf->baud_h = mcp->baud_h;
> + conf->baud_l = mcp->baud_l;
> + conf->config_alt_options = mcp->config_alt_options;
> + conf->io_default_val_bmap = mcp->gpio_reset_val;
> + /* Adjust alt. func if necessary */
> + if (alt_pin_conf[gpio_nr])
> + config_alt_pins &= ~(1 << alt_pin_conf[gpio_nr]);
> + conf->config_alt_pins = config_alt_pins;
> +
> + status = hid_hw_output_report(mcp->hdev, (u8 *) conf,
> + sizeof(struct mcp_set_clear_outputs));
> +
> + if (status == sizeof(struct mcp_set_clear_outputs)) {
> + mcp->gpio_dir = conf->io_bmap;
> + mcp->config_alt_pins = config_alt_pins;
> + } else {
> + mutex_unlock(&mcp->lock);
> + return -EIO;
> + }
> +
> + mutex_unlock(&mcp->lock);
> +
> + /* Configure CMD will clear all IOs -> rewrite them */
> + mcp_set_multiple(gc, &mask, &bits);
> + return 0;
> +}
> +
> +static int mcp_direction_input(struct gpio_chip *gc, unsigned int gpio_nr)
> +{
> + return mcp_set_direction(gc, gpio_nr, MCP2200_DIR_IN);
> +}
> +
> +static int mcp_direction_output(struct gpio_chip *gc, unsigned int gpio_nr,
> + int value)
> +{
> + int ret;
> + unsigned long mask, bmap_value;
> +
> + mask = 1 << gpio_nr;
> + bmap_value = value << gpio_nr;
> +
> + ret = mcp_set_direction(gc, gpio_nr, MCP2200_DIR_OUT);
> + if (!ret)
> + mcp_set_multiple(gc, &mask, &bmap_value);
> + return ret;
> +}
> +
> +static const struct gpio_chip template_chip = {
> + .label = "mcp2200",
> + .owner = THIS_MODULE,
> + .get_direction = mcp_get_direction,
> + .direction_input = mcp_direction_input,
> + .direction_output = mcp_direction_output,
> + .set = mcp_set,
> + .set_multiple = mcp_set_multiple,
> + .get = mcp_get,
> + .get_multiple = mcp_get_multiple,
> + .base = -1,
> + .ngpio = MCP_NGPIO,
> + .can_sleep = true,
> +};
> +
> +/*
> + * MCP2200 uses interrupt endpoint for input reports. This function
> + * is called by HID layer when it receives i/p report from mcp2200,
> + * which is actually a response to the previously sent command.
> + */
> +static int mcp2200_raw_event(struct hid_device *hdev, struct hid_report *report,
> + u8 *data, int size)
> +{
> + struct mcp2200 *mcp = hid_get_drvdata(hdev);
> + struct mcp_read_all_resp *all_resp;
> +
> + switch (data[0]) {
> + case READ_ALL:
> + all_resp = (struct mcp_read_all_resp *) data;
> + mcp->status = 0;
> + mcp->gpio_inval = all_resp->io_port_val_bmap;
> + mcp->baud_h = all_resp->baud_h;
> + mcp->baud_l = all_resp->baud_l;
> + mcp->gpio_reset_val = all_resp->io_default_val_bmap;
> + mcp->config_alt_pins = all_resp->config_alt_pins;
> + mcp->config_alt_options = all_resp->config_alt_options;
> + break;
> + default:
> + mcp->status = -EIO;
> + break;
> + }
> +
> + complete(&mcp->wait_in_report);
> + return 0;
> +}
> +
> +static int mcp2200_probe(struct hid_device *hdev, const struct hid_device_id *id)
> +{
> + int ret;
> + struct mcp2200 *mcp;
> +
> + mcp = devm_kzalloc(&hdev->dev, sizeof(*mcp), GFP_KERNEL);
> + if (!mcp)
> + return -ENOMEM;
> +
> + ret = hid_parse(hdev);
> + if (ret) {
> + hid_err(hdev, "can't parse reports\n");
> + return ret;
> + }
> +
> + ret = hid_hw_start(hdev, 0);
> + if (ret) {
> + hid_err(hdev, "can't start hardware\n");
> + return ret;
> + }
> +
> + hid_info(hdev, "USB HID v%x.%02x Device [%s] on %s\n", hdev->version >> 8,
> + hdev->version & 0xff, hdev->name, hdev->phys);
> +
> + ret = hid_hw_open(hdev);
> + if (ret) {
> + hid_err(hdev, "can't open device\n");
> + hid_hw_stop(hdev);
> + return ret;
> + }
> +
> + mutex_init(&mcp->lock);
> + init_completion(&mcp->wait_in_report);
> + hid_set_drvdata(hdev, mcp);
> + mcp->hdev = hdev;
> +
> + mcp->gc = template_chip;
> + mcp->gc.parent = &hdev->dev;
> +
> + ret = devm_gpiochip_add_data(&hdev->dev, &mcp->gc, mcp);
> + if (ret < 0) {
You will want to call mcp2200_remove here in this error path.
> + hid_err(hdev, "Unable to register gpiochip\n");
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static void mcp2200_remove(struct hid_device *hdev)
> +{
> + hid_hw_close(hdev);
> + hid_hw_stop(hdev);
> +}
> +
> +static const struct hid_device_id mcp2200_devices[] = {
> + { HID_USB_DEVICE(USB_VENDOR_ID_MICROCHIP, USB_DEVICE_ID_MCP2200) },
> + { }
> +};
> +MODULE_DEVICE_TABLE(hid, mcp2200_devices);
> +
> +static struct hid_driver mcp2200_driver = {
> + .name = "mcp2200",
> + .id_table = mcp2200_devices,
> + .probe = mcp2200_probe,
> + .remove = mcp2200_remove,
> + .raw_event = mcp2200_raw_event,
> +};
> +
> +/* Register with HID core */
> +module_hid_driver(mcp2200_driver);
> +
> +MODULE_AUTHOR("Johannes Roith <johannes@gnu-linux.rocks>");
> +MODULE_DESCRIPTION("MCP2200 Microchip HID USB to GPIO bridge");
> +MODULE_LICENSE("GPL");
This patch looks great. I just have one minor comment with regards to
the refactor now that devm is not being used to handle hid cleanup.
--
Thanks,
Rahul Rameshbabu
^ permalink raw reply
* Re: [PATCH] hid: cp2112: Fix IRQ shutdown stopping polling for all IRQs on chip
From: Danny Kaehn @ 2023-09-20 19:12 UTC (permalink / raw)
To: andriy.shevchenko@linux.intel.com
Cc: jikos@kernel.org, benjamin.tissoires@redhat.com,
linux-input@vger.kernel.org, Ethan Twardy
In-Reply-To: <ZQr0r3Ux/rkWQ8N5@smile.fi.intel.com>
On Wed, 2023-09-20 at 16:33 +0300, Andy Shevchenko wrote:
> On Tue, Sep 19, 2023 at 04:24:26PM -0500, Danny Kaehn wrote:
> > Previously cp2112_gpio_irq_shutdown always cancelled the
>
> cp2112_gpio_irq_shutdown()
> > _remove occurred (a.e. the cp2112 is unplugged or system rebooted).
>
> _remove()
>
> > + if (!dev->irq_mask)
> > + {
>
> The style tells to use these on a single line.
ACK -- will fix all in v2. Thanks for the comments.
Danny Kaehn
^ permalink raw reply
* Re: [PATCH] hid: cp2112: Fix duplicate workqueue initialization
From: Danny Kaehn @ 2023-09-20 19:10 UTC (permalink / raw)
To: andriy.shevchenko@linux.intel.com
Cc: jikos@kernel.org, benjamin.tissoires@redhat.com,
linux-input@vger.kernel.org, Ethan Twardy
In-Reply-To: <ZQrt6k/usVXlB223@smile.fi.intel.com>
On Wed, 2023-09-20 at 16:04 +0300, Andy Shevchenko wrote:
> On Tue, Sep 19, 2023 at 04:22:45PM -0500, Danny Kaehn wrote:
> > Previously the cp2112 driver called INIT_DELAYED_WORK within
> > cp2112_gpio_irq_startup, resulting in duplicate initilizations of the
> > workqueue on subsequent IRQ startups following an initial request. This
> > resulted in a warning in set_work_data in workqueue.c, as well as a rare
> > NULL dereference within process_one_work in workqueue.c.
> >
> > Initialize the workqueue within _probe instead.
>
> Does it deserve a Fixes tag?
I'm not sure -- it does technically fix 13de9cca514ed63604263cad87ca8cb36e9b6489
(HID: cp2112: add IRQ chip handling), but does not apply cleanly to that
revision (a.e. with git am)
(my understanding is that 'Fixes' is used for stable kernel maintenance?)
Thanks,
Danny Kaehn
^ permalink raw reply
* Re: [PATCH] Input: powermate - fix use-after-free in powermate_config_complete
From: kernel test robot @ 2023-09-20 18:35 UTC (permalink / raw)
To: Javier Carrasco, Dmitry Torokhov
Cc: oe-kbuild-all, linux-input, linux-kernel, Javier Carrasco,
syzbot+0434ac83f907a1dbdd1e
In-Reply-To: <20230916-topic-powermate_use_after_free-v1-1-2ffa46652869@gmail.com>
Hi Javier,
kernel test robot noticed the following build errors:
[auto build test ERROR on 0bb80ecc33a8fb5a682236443c1e740d5c917d1d]
url: https://github.com/intel-lab-lkp/linux/commits/Javier-Carrasco/Input-powermate-fix-use-after-free-in-powermate_config_complete/20230917-052943
base: 0bb80ecc33a8fb5a682236443c1e740d5c917d1d
patch link: https://lore.kernel.org/r/20230916-topic-powermate_use_after_free-v1-1-2ffa46652869%40gmail.com
patch subject: [PATCH] Input: powermate - fix use-after-free in powermate_config_complete
config: powerpc-ppc6xx_defconfig (https://download.01.org/0day-ci/archive/20230921/202309210232.d7MpKEIm-lkp@intel.com/config)
compiler: powerpc-linux-gcc (GCC) 11.3.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230921/202309210232.d7MpKEIm-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202309210232.d7MpKEIm-lkp@intel.com/
All errors (new ones prefixed by >>):
drivers/input/misc/powermate.c: In function 'powermate_config_complete':
>> drivers/input/misc/powermate.c:201:21: error: 'status' undeclared (first use in this function); did you mean 'kstatfs'?
201 | if (status == -ENOENT || status == -ESHUTDOWN)
| ^~~~~~
| kstatfs
drivers/input/misc/powermate.c:201:21: note: each undeclared identifier is reported only once for each function it appears in
vim +201 drivers/input/misc/powermate.c
192
193 /* Called when our asynchronous control message completes. We may need to issue another immediately */
194 static void powermate_config_complete(struct urb *urb)
195 {
196 struct powermate_device *pm = urb->context;
197 unsigned long flags;
198
199 if (urb->status) {
200 printk(KERN_ERR "powermate: config urb returned %d\n", urb->status);
> 201 if (status == -ENOENT || status == -ESHUTDOWN)
202 return;
203 }
204
205 spin_lock_irqsave(&pm->lock, flags);
206 powermate_sync_state(pm);
207 spin_unlock_irqrestore(&pm->lock, flags);
208 }
209
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply
* Re: [PATCH v2 7/9] iio: hid-sensor-als: Add light chromaticity support
From: srinivas pandruvada @ 2023-09-20 17:10 UTC (permalink / raw)
To: Basavaraj Natikar, jikos, benjamin.tissoires, jic23, lars,
linux-input, linux-iio
In-Reply-To: <20230919081054.2050714-8-Basavaraj.Natikar@amd.com>
On Tue, 2023-09-19 at 13:40 +0530, Basavaraj Natikar wrote:
> In most cases, ambient color sensors also support the x and y light
> colors, which represent the coordinates on the CIE 1931 chromaticity
> diagram. Thus, add light chromaticity x and y.
>
> Signed-off-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com>
Acked-by: Srinivas Pandruvada<srinivas.pandruvada@linux.intel.com>
> ---
> drivers/iio/light/hid-sensor-als.c | 63
> ++++++++++++++++++++++++++++++
> include/linux/hid-sensor-ids.h | 3 ++
> 2 files changed, 66 insertions(+)
>
> diff --git a/drivers/iio/light/hid-sensor-als.c
> b/drivers/iio/light/hid-sensor-als.c
> index 16a3f1941c27..c9d114ff080a 100644
> --- a/drivers/iio/light/hid-sensor-als.c
> +++ b/drivers/iio/light/hid-sensor-als.c
> @@ -17,6 +17,8 @@ enum {
> CHANNEL_SCAN_INDEX_INTENSITY,
> CHANNEL_SCAN_INDEX_ILLUM,
> CHANNEL_SCAN_INDEX_COLOR_TEMP,
> + CHANNEL_SCAN_INDEX_CHROMATICITY_X,
> + CHANNEL_SCAN_INDEX_CHROMATICITY_Y,
> CHANNEL_SCAN_INDEX_MAX
> };
>
> @@ -76,6 +78,30 @@ static const struct iio_chan_spec als_channels[] =
> {
> BIT(IIO_CHAN_INFO_HYSTERESIS_RELATIVE),
> .scan_index = CHANNEL_SCAN_INDEX_COLOR_TEMP,
> },
> + {
> + .type = IIO_CHROMATICITY,
> + .modified = 1,
> + .channel2 = IIO_MOD_X,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_OFFSET)
> |
> + BIT(IIO_CHAN_INFO_SCALE) |
> + BIT(IIO_CHAN_INFO_SAMP_FREQ) |
> + BIT(IIO_CHAN_INFO_HYSTERESIS) |
> + BIT(IIO_CHAN_INFO_HYSTERESIS_RELATIVE),
> + .scan_index = CHANNEL_SCAN_INDEX_CHROMATICITY_X,
> + },
> + {
> + .type = IIO_CHROMATICITY,
> + .modified = 1,
> + .channel2 = IIO_MOD_Y,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_OFFSET)
> |
> + BIT(IIO_CHAN_INFO_SCALE) |
> + BIT(IIO_CHAN_INFO_SAMP_FREQ) |
> + BIT(IIO_CHAN_INFO_HYSTERESIS) |
> + BIT(IIO_CHAN_INFO_HYSTERESIS_RELATIVE),
> + .scan_index = CHANNEL_SCAN_INDEX_CHROMATICITY_Y,
> + },
> IIO_CHAN_SOFT_TIMESTAMP(CHANNEL_SCAN_INDEX_TIMESTAMP)
> };
>
> @@ -119,6 +145,16 @@ static int als_read_raw(struct iio_dev
> *indio_dev,
> min = als_state->als[chan-
> >scan_index].logical_minimum;
> address =
> HID_USAGE_SENSOR_LIGHT_COLOR_TEMPERATURE;
> break;
> + case CHANNEL_SCAN_INDEX_CHROMATICITY_X:
> + report_id = als_state->als[chan-
> >scan_index].report_id;
> + min = als_state->als[chan-
> >scan_index].logical_minimum;
> + address =
> HID_USAGE_SENSOR_LIGHT_CHROMATICITY_X;
> + break;
> + case CHANNEL_SCAN_INDEX_CHROMATICITY_Y:
> + report_id = als_state->als[chan-
> >scan_index].report_id;
> + min = als_state->als[chan-
> >scan_index].logical_minimum;
> + address =
> HID_USAGE_SENSOR_LIGHT_CHROMATICITY_Y;
> + break;
> default:
> report_id = -1;
> break;
> @@ -243,6 +279,14 @@ static int als_capture_sample(struct
> hid_sensor_hub_device *hsdev,
> als_state->scan.illum[CHANNEL_SCAN_INDEX_COLOR_TEMP]
> = sample_data;
> ret = 0;
> break;
> + case HID_USAGE_SENSOR_LIGHT_CHROMATICITY_X:
> + als_state-
> >scan.illum[CHANNEL_SCAN_INDEX_CHROMATICITY_X] = sample_data;
> + ret = 0;
> + break;
> + case HID_USAGE_SENSOR_LIGHT_CHROMATICITY_Y:
> + als_state-
> >scan.illum[CHANNEL_SCAN_INDEX_CHROMATICITY_Y] = sample_data;
> + ret = 0;
> + break;
> case HID_USAGE_SENSOR_TIME_TIMESTAMP:
> als_state->timestamp =
> hid_sensor_convert_timestamp(&als_state->common_attributes,
>
> *(s64 *)raw_data);
> @@ -291,6 +335,25 @@ static int als_parse_report(struct
> platform_device *pdev,
> st->als[CHANNEL_SCAN_INDEX_COLOR_TEMP].index,
> st->als[CHANNEL_SCAN_INDEX_COLOR_TEMP].report_id);
>
> + for (i = 0; i < 2; i++) {
> + int next_scan_index =
> CHANNEL_SCAN_INDEX_CHROMATICITY_X + i;
> +
> + ret = sensor_hub_input_get_attribute_info(hsdev,
> + HID_INPUT_REPORT, usage_id,
> + HID_USAGE_SENSOR_LIGHT_CHROMATICITY_X
> + i,
> + &st->als[next_scan_index]);
> + if (ret < 0)
> + return ret;
> +
> + als_adjust_channel_bit_mask(channels,
> + CHANNEL_SCAN_INDEX_CHROMATICI
> TY_X + i,
> + st-
> >als[next_scan_index].size);
> +
> + dev_dbg(&pdev->dev, "als %x:%x\n",
> + st->als[next_scan_index].index,
> + st->als[next_scan_index].report_id);
> + }
> +
> st->scale_precision = hid_sensor_format_scale(usage_id,
> &st-
> >als[CHANNEL_SCAN_INDEX_INTENSITY],
> &st->scale_pre_decml, &st-
> >scale_post_decml);
> diff --git a/include/linux/hid-sensor-ids.h b/include/linux/hid-
> sensor-ids.h
> index 8af4fb3e0254..6730ee900ee1 100644
> --- a/include/linux/hid-sensor-ids.h
> +++ b/include/linux/hid-sensor-ids.h
> @@ -22,6 +22,9 @@
> #define
> HID_USAGE_SENSOR_DATA_LIGHT 0x2004d0
> #define
> HID_USAGE_SENSOR_LIGHT_ILLUM 0x2004d1
> #define
> HID_USAGE_SENSOR_LIGHT_COLOR_TEMPERATURE 0x2004d2
> +#define
> HID_USAGE_SENSOR_LIGHT_CHROMATICITY 0x2004d3
> +#define
> HID_USAGE_SENSOR_LIGHT_CHROMATICITY_X 0x2004d4
> +#define
> HID_USAGE_SENSOR_LIGHT_CHROMATICITY_Y 0x2004d5
>
> /* PROX (200011) */
> #define HID_USAGE_SENSOR_PROX
> 0x200011
^ permalink raw reply
* Re: [PATCH v2 3/9] iio: hid-sensor-als: Add light color temperature support
From: srinivas pandruvada @ 2023-09-20 17:00 UTC (permalink / raw)
To: Basavaraj Natikar, jikos, benjamin.tissoires, jic23, lars,
linux-input, linux-iio
In-Reply-To: <20230919081054.2050714-4-Basavaraj.Natikar@amd.com>
On Tue, 2023-09-19 at 13:40 +0530, Basavaraj Natikar wrote:
> In most cases, ambient color sensors also support light color
> temperature.
> As a result, add support of light color temperature.
>
> Signed-off-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com>
Acked-by: Srinivas Pandruvada<srinivas.pandruvada@linux.intel.com>
> ---
> drivers/iio/light/hid-sensor-als.c | 37
> ++++++++++++++++++++++++++++--
> include/linux/hid-sensor-ids.h | 1 +
> 2 files changed, 36 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/iio/light/hid-sensor-als.c
> b/drivers/iio/light/hid-sensor-als.c
> index efb1f8862b28..16a3f1941c27 100644
> --- a/drivers/iio/light/hid-sensor-als.c
> +++ b/drivers/iio/light/hid-sensor-als.c
> @@ -14,8 +14,9 @@
> #include "../common/hid-sensors/hid-sensor-trigger.h"
>
> enum {
> - CHANNEL_SCAN_INDEX_INTENSITY = 0,
> - CHANNEL_SCAN_INDEX_ILLUM = 1,
> + CHANNEL_SCAN_INDEX_INTENSITY,
> + CHANNEL_SCAN_INDEX_ILLUM,
> + CHANNEL_SCAN_INDEX_COLOR_TEMP,
> CHANNEL_SCAN_INDEX_MAX
> };
>
> @@ -65,6 +66,16 @@ static const struct iio_chan_spec als_channels[] =
> {
> BIT(IIO_CHAN_INFO_HYSTERESIS_RELATIVE),
> .scan_index = CHANNEL_SCAN_INDEX_ILLUM,
> },
> + {
> + .type = IIO_COLORTEMP,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_OFFSET)
> |
> + BIT(IIO_CHAN_INFO_SCALE) |
> + BIT(IIO_CHAN_INFO_SAMP_FREQ) |
> + BIT(IIO_CHAN_INFO_HYSTERESIS) |
> + BIT(IIO_CHAN_INFO_HYSTERESIS_RELATIVE),
> + .scan_index = CHANNEL_SCAN_INDEX_COLOR_TEMP,
> + },
> IIO_CHAN_SOFT_TIMESTAMP(CHANNEL_SCAN_INDEX_TIMESTAMP)
> };
>
> @@ -103,6 +114,11 @@ static int als_read_raw(struct iio_dev
> *indio_dev,
> min = als_state->als[chan-
> >scan_index].logical_minimum;
> address = HID_USAGE_SENSOR_LIGHT_ILLUM;
> break;
> + case CHANNEL_SCAN_INDEX_COLOR_TEMP:
> + report_id = als_state->als[chan-
> >scan_index].report_id;
> + min = als_state->als[chan-
> >scan_index].logical_minimum;
> + address =
> HID_USAGE_SENSOR_LIGHT_COLOR_TEMPERATURE;
> + break;
> default:
> report_id = -1;
> break;
> @@ -223,6 +239,10 @@ static int als_capture_sample(struct
> hid_sensor_hub_device *hsdev,
> als_state->scan.illum[CHANNEL_SCAN_INDEX_ILLUM] =
> sample_data;
> ret = 0;
> break;
> + case HID_USAGE_SENSOR_LIGHT_COLOR_TEMPERATURE:
> + als_state->scan.illum[CHANNEL_SCAN_INDEX_COLOR_TEMP]
> = sample_data;
> + ret = 0;
> + break;
> case HID_USAGE_SENSOR_TIME_TIMESTAMP:
> als_state->timestamp =
> hid_sensor_convert_timestamp(&als_state->common_attributes,
>
> *(s64 *)raw_data);
> @@ -258,6 +278,19 @@ static int als_parse_report(struct
> platform_device *pdev,
> st->als[i].report_id);
> }
>
> + ret = sensor_hub_input_get_attribute_info(hsdev,
> HID_INPUT_REPORT,
> + usage_id,
> + HID_USAGE_SENSOR_LIGHT_COLOR_TEMPERAT
> URE,
> + &st-
> >als[CHANNEL_SCAN_INDEX_COLOR_TEMP]);
> + if (ret < 0)
> + return ret;
> + als_adjust_channel_bit_mask(channels,
> CHANNEL_SCAN_INDEX_COLOR_TEMP,
> + st-
> >als[CHANNEL_SCAN_INDEX_COLOR_TEMP].size);
> +
> + dev_dbg(&pdev->dev, "als %x:%x\n",
> + st->als[CHANNEL_SCAN_INDEX_COLOR_TEMP].index,
> + st->als[CHANNEL_SCAN_INDEX_COLOR_TEMP].report_id);
> +
> st->scale_precision = hid_sensor_format_scale(usage_id,
> &st-
> >als[CHANNEL_SCAN_INDEX_INTENSITY],
> &st->scale_pre_decml, &st-
> >scale_post_decml);
> diff --git a/include/linux/hid-sensor-ids.h b/include/linux/hid-
> sensor-ids.h
> index 13b1e65fbdcc..8af4fb3e0254 100644
> --- a/include/linux/hid-sensor-ids.h
> +++ b/include/linux/hid-sensor-ids.h
> @@ -21,6 +21,7 @@
> #define
> HID_USAGE_SENSOR_ALS 0x200041
> #define
> HID_USAGE_SENSOR_DATA_LIGHT 0x2004d0
> #define
> HID_USAGE_SENSOR_LIGHT_ILLUM 0x2004d1
> +#define
> HID_USAGE_SENSOR_LIGHT_COLOR_TEMPERATURE 0x2004d2
>
> /* PROX (200011) */
> #define HID_USAGE_SENSOR_PROX
> 0x200011
^ permalink raw reply
* Re: [PATCH v2 1/9] iio: hid-sensor-als: Use channel index to support more hub attributes
From: srinivas pandruvada @ 2023-09-20 16:59 UTC (permalink / raw)
To: Basavaraj Natikar, jikos, benjamin.tissoires, jic23, lars,
linux-input, linux-iio
In-Reply-To: <20230919081054.2050714-2-Basavaraj.Natikar@amd.com>
On Tue, 2023-09-19 at 13:40 +0530, Basavaraj Natikar wrote:
> Sensor hub attributes can be extended to support more channels.
> Repeat
> the reading for the two existing channels and store them separately.
> It
> still operates in the same manner as before where there was just one
> entry. So in order to support more sensor hub attributes for ALS use
> channel index to get specific sensor hub attributes.
>
> Signed-off-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com>
Acked-by: Srinivas Pandruvada<srinivas.pandruvada@linux.intel.com>
> ---
> drivers/iio/light/hid-sensor-als.c | 38 ++++++++++++++++------------
> --
> 1 file changed, 20 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/iio/light/hid-sensor-als.c
> b/drivers/iio/light/hid-sensor-als.c
> index eb1aedad7edc..efb1f8862b28 100644
> --- a/drivers/iio/light/hid-sensor-als.c
> +++ b/drivers/iio/light/hid-sensor-als.c
> @@ -24,7 +24,7 @@ enum {
> struct als_state {
> struct hid_sensor_hub_callbacks callbacks;
> struct hid_sensor_common common_attributes;
> - struct hid_sensor_hub_attribute_info als_illum;
> + struct hid_sensor_hub_attribute_info
> als[CHANNEL_SCAN_INDEX_MAX];
> struct {
> u32 illum[CHANNEL_SCAN_INDEX_MAX];
> u64 timestamp __aligned(8);
> @@ -99,8 +99,8 @@ static int als_read_raw(struct iio_dev *indio_dev,
> switch (chan->scan_index) {
> case CHANNEL_SCAN_INDEX_INTENSITY:
> case CHANNEL_SCAN_INDEX_ILLUM:
> - report_id = als_state->als_illum.report_id;
> - min = als_state->als_illum.logical_minimum;
> + report_id = als_state->als[chan-
> >scan_index].report_id;
> + min = als_state->als[chan-
> >scan_index].logical_minimum;
> address = HID_USAGE_SENSOR_LIGHT_ILLUM;
> break;
> default:
> @@ -242,22 +242,24 @@ static int als_parse_report(struct
> platform_device *pdev,
> struct als_state *st)
> {
> int ret;
> + int i;
> +
> + for (i = 0; i <= CHANNEL_SCAN_INDEX_ILLUM; ++i) {
> + ret = sensor_hub_input_get_attribute_info(hsdev,
> + HID_INPUT_REPORT,
> + usage_id,
> + HID_USAGE_SENSOR_LIGH
> T_ILLUM,
> + &st->als[i]);
> + if (ret < 0)
> + return ret;
> + als_adjust_channel_bit_mask(channels, i, st-
> >als[i].size);
> +
> + dev_dbg(&pdev->dev, "als %x:%x\n", st->als[i].index,
> + st->als[i].report_id);
> + }
>
> - ret = sensor_hub_input_get_attribute_info(hsdev,
> HID_INPUT_REPORT,
> - usage_id,
> - HID_USAGE_SENSOR_LIGHT_ILLUM,
> - &st->als_illum);
> - if (ret < 0)
> - return ret;
> - als_adjust_channel_bit_mask(channels,
> CHANNEL_SCAN_INDEX_INTENSITY,
> - st->als_illum.size);
> - als_adjust_channel_bit_mask(channels,
> CHANNEL_SCAN_INDEX_ILLUM,
> - st->als_illum.size);
> -
> - dev_dbg(&pdev->dev, "als %x:%x\n", st->als_illum.index,
> - st->als_illum.report_id);
> -
> - st->scale_precision = hid_sensor_format_scale(usage_id, &st-
> >als_illum,
> + st->scale_precision = hid_sensor_format_scale(usage_id,
> + &st-
> >als[CHANNEL_SCAN_INDEX_INTENSITY],
> &st->scale_pre_decml, &st-
> >scale_post_decml);
>
> return ret;
^ permalink raw reply
* Re: [PATCH 42/52] input: sun4i-ps2 - Convert to platform remove callback returning void
From: Jernej Škrabec @ 2023-09-20 16:07 UTC (permalink / raw)
To: Dmitry Torokhov, Uwe Kleine-König
Cc: Chen-Yu Tsai, Samuel Holland, linux-input, linux-arm-kernel,
linux-sunxi, kernel
In-Reply-To: <20230920125829.1478827-43-u.kleine-koenig@pengutronix.de>
Dne sreda, 20. september 2023 ob 14:58:19 CEST je Uwe Kleine-König napisal(a):
> The .remove() callback for a platform driver returns an int which makes
> many driver authors wrongly assume it's possible to do error handling by
> returning an error code. However the value returned is ignored (apart
> from emitting a warning) and this typically results in resource leaks.
> To improve here there is a quest to make the remove callback return
> void. In the first step of this quest all drivers are converted to
> .remove_new() which already returns void. Eventually after all drivers
> are converted, .remove_new() will be renamed to .remove().
>
> Trivially convert this driver from always returning zero in the remove
> callback to the void returning variant.
>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Acked-by: Jernej Skrabec <jernej.skrabec@gmail.com>
Best regards,
Jernej
> ---
> drivers/input/serio/sun4i-ps2.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/input/serio/sun4i-ps2.c
> b/drivers/input/serio/sun4i-ps2.c index eb262640192e..aec66d9f5176 100644
> --- a/drivers/input/serio/sun4i-ps2.c
> +++ b/drivers/input/serio/sun4i-ps2.c
> @@ -297,7 +297,7 @@ static int sun4i_ps2_probe(struct platform_device *pdev)
> return error;
> }
>
> -static int sun4i_ps2_remove(struct platform_device *pdev)
> +static void sun4i_ps2_remove(struct platform_device *pdev)
> {
> struct sun4i_ps2data *drvdata = platform_get_drvdata(pdev);
>
> @@ -311,8 +311,6 @@ static int sun4i_ps2_remove(struct platform_device
> *pdev) iounmap(drvdata->reg_base);
>
> kfree(drvdata);
> -
> - return 0;
> }
>
> static const struct of_device_id sun4i_ps2_match[] = {
> @@ -324,7 +322,7 @@ MODULE_DEVICE_TABLE(of, sun4i_ps2_match);
>
> static struct platform_driver sun4i_ps2_driver = {
> .probe = sun4i_ps2_probe,
> - .remove = sun4i_ps2_remove,
> + .remove_new = sun4i_ps2_remove,
> .driver = {
> .name = DRIVER_NAME,
> .of_match_table = sun4i_ps2_match,
^ permalink raw reply
* Re: [PATCH 49/52] input: sun4i-ts - Convert to platform remove callback returning void
From: Jernej Škrabec @ 2023-09-20 16:07 UTC (permalink / raw)
To: Dmitry Torokhov, Uwe Kleine-König
Cc: Chen-Yu Tsai, Samuel Holland, Rafael J. Wysocki,
Krzysztof Kozlowski, Daniel Lezcano, Jonathan Corbet, linux-input,
linux-arm-kernel, linux-sunxi, kernel
In-Reply-To: <20230920125829.1478827-50-u.kleine-koenig@pengutronix.de>
Dne sreda, 20. september 2023 ob 14:58:26 CEST je Uwe Kleine-König napisal(a):
> The .remove() callback for a platform driver returns an int which makes
> many driver authors wrongly assume it's possible to do error handling by
> returning an error code. However the value returned is ignored (apart
> from emitting a warning) and this typically results in resource leaks.
> To improve here there is a quest to make the remove callback return
> void. In the first step of this quest all drivers are converted to
> .remove_new() which already returns void. Eventually after all drivers
> are converted, .remove_new() will be renamed to .remove().
>
> Trivially convert this driver from always returning zero in the remove
> callback to the void returning variant.
>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Acked-by: Jernej Skrabec <jernej.skrabec@gmail.com>
Best regards,
Jernej
> ---
> drivers/input/touchscreen/sun4i-ts.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/input/touchscreen/sun4i-ts.c
> b/drivers/input/touchscreen/sun4i-ts.c index bb3c6072fc82..92b2b840b4b7
> 100644
> --- a/drivers/input/touchscreen/sun4i-ts.c
> +++ b/drivers/input/touchscreen/sun4i-ts.c
> @@ -375,7 +375,7 @@ static int sun4i_ts_probe(struct platform_device *pdev)
> return 0;
> }
>
> -static int sun4i_ts_remove(struct platform_device *pdev)
> +static void sun4i_ts_remove(struct platform_device *pdev)
> {
> struct sun4i_ts_data *ts = platform_get_drvdata(pdev);
>
> @@ -385,8 +385,6 @@ static int sun4i_ts_remove(struct platform_device *pdev)
>
> /* Deactivate all IRQs */
> writel(0, ts->base + TP_INT_FIFOC);
> -
> - return 0;
> }
>
> static const struct of_device_id sun4i_ts_of_match[] = {
> @@ -403,7 +401,7 @@ static struct platform_driver sun4i_ts_driver = {
> .of_match_table = sun4i_ts_of_match,
> },
> .probe = sun4i_ts_probe,
> - .remove = sun4i_ts_remove,
> + .remove_new = sun4i_ts_remove,
> };
>
> module_platform_driver(sun4i_ts_driver);
^ permalink raw reply
* Re: [PATCH] HID: i2c-hid: fix handling of unpopulated devices
From: Doug Anderson @ 2023-09-20 15:41 UTC (permalink / raw)
To: Johan Hovold
Cc: Johan Hovold, Jiri Kosina, Benjamin Tissoires, linux-input,
linux-kernel, Maxime Ripard, Dmitry Torokhov, LinusW, Rob Herring,
Krzysztof Kozlowski, Conor Dooley,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
open list:GPIO SUBSYSTEM
In-Reply-To: <ZQqemN8P2VKgxhsV@hovoldconsulting.com>
Hi,
On Wed, Sep 20, 2023 at 12:26 AM Johan Hovold <johan@kernel.org> wrote:
>
> On Tue, Sep 19, 2023 at 11:15:46AM -0700, Doug Anderson wrote:
> > On Tue, Sep 19, 2023 at 12:07 AM Johan Hovold <johan@kernel.org> wrote:
>
> > > But regardless of what a long-term proper solution to this may look
> > > like, we need to fix the regression in 6.6-rc1 by restoring the old
> > > behaviour.
> >
> > OK, fair enough. I'll take a look at your patch, though I think the
> > person that really needs to approve it is Benjamin...
> >
> > Style-wise, I will say that Benjamin really wanted to keep the "panel
> > follower" code out of the main probe routine. Some of my initial
> > patches adding "panel follower" looked more like the results after
> > your patch but Benjamin really wasn't happy until there were no
> > special cases for panel-followers in the main probe routine. This is
> > why the code is structured as it is.
>
> Ok, I prefer not hiding away things like that as it obscures what's
> really going on, for example, in this case, that you register a device
> without really having probed it.
I can see your reasoning and I think that intuition is why the earlier
versions of my patches had explicit "panel follower" logic in probe.
However, Benjamin really liked the logic abstracted out.
> As I alluded to in the commit message, you probably want to be able to
> support second-source touchscreen panel followers as well at some point
> and then deferring checking whether device is populated until the panel
> is powered on is not going to work.
Yeah, I've been pondering this too. I _think_ it would work OK-ish if
both devices probed and then only one of the two would actually make
the sub-HID devices. So you'd actually see both devices succeed at
probing but only one of them would actually be functional. It's a bit
ugly, though. :( Maybe marginally better would be if we could figure
out how to have the device which fails to get its interrupt later
unbind itself, if that's possible...
The only other thought I had would be to have the parent i2c bus
understand that it had children that were panel followers, which it
should be able to do by seeing the "panel" attribute in their device
tree. Then the i2c bus could itself register as a panel follower and
could wait to probe its children until they were powered on. This
could happen in the i2c core so we didn't have to add code like this
to all i2c bus drivers. ...and, if necessary, we could add this to
other busses like SPI. It feels a little awkward but could work.
> I skimmed the thread were you added this, but I'm not sure I saw any
> reason for why powering on the panel follower temporarily during probe
> would not work?
My first instinct says we can't do this, but let's think about it...
In general the "panel follower" API is designed to give all the
decision making about when to power things on and off to the panel
driver, which is controlled by DRM.
The reason for this is from experience I had when dealing with the
Samsung ATNA33XC20 panel that's on "sc7180-trogdor-homestar". The TCON
on that panel tended to die if you didn't sequence it just right.
Specifically, if you were sending pixels to the panel and then stopped
then you absolutely needed to power the panel off and on again. Folks
I talked to even claimed that the panel was working "to spec" since,
in the "Power Sequencing" section of the eDP spec it clearly shows
that you _must_ turn the panel off and on again after you stop giving
it bits. ...this is despite the fact that no other panel I've worked
with cares. ;-)
On homestar, since we didn't have the "panel follower" API, we ended
up adding cost to the hardware and putting the panel and touchscreens
on different power rails. However, I wanted to make sure that if we
ran into a similar situation in the future (or maybe if we were trying
to make hardware work that we didn't have control over) that we could
solve it.
The other reason for giving full control to the panel driver is just
how userspace usually works. Right now userspace tends to power off
panels if they're not used (like if a lid is closed on a laptop) but
doesn't necessarily power off the touchscreen. Thus if the touchscreen
has the ability to keep things powered on then we'd never get to a low
power state.
The above all explains why panel followers like the touchscreen
shouldn't be able to keep power on. However, you are specifically
suggesting that we just turn the power on temporarily during probe. As
I think about that, it might be possible? I guess you'd have to
temporarily block DRM from changing the state of the panel while the
touchscreen is probing. Then if the panel was off then you'd turn it
on briefly, do your probe, and then turn it off again. If the panel
was on then by blocking DRM you'd ensure that it stayed on. I'm not
sure how palatable that would be or if there are any other tricky
parts I'm not thinking about.
> > Thinking that way, is there any reason you can't just move the
> > i2c_hid_init_irq() into __do_i2c_hid_core_initial_power_up()? You
> > could replace the call to enable_irq() with it and then remove the
> > `IRQF_NO_AUTOEN` flag? I think that would also solve the issue if you
> > wanted to use a 2nd source + the panel follower concept? Both devices
> > would probe, but only one of them would actually grab the interrupt
> > and only one of them would actually create real HID devices. We might
> > need to do some work to keep from trying again at every poweron of the
> > panel, but it would probably be workable? I think this would also be a
> > smaller change...
>
> That was my first idea as well, but conceptually it is more correct to
> request resources at probe time and not at some later point when you can
> no longer fail probe.
>
> You'd also need to handle the fact that the interrupt may never have
> been requested when remove() is called, which adds unnecessary
> complexity.
I don't think it's a lot of complexity, is it? Just an extra "if" statement...
-Doug
^ 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