* Re: [PATCH v2] Input: uinput: Avoid Object-Already-Free with a global lock
From: Mukesh Ojha @ 2019-04-24 14:09 UTC (permalink / raw)
To: Al Viro
Cc: dmitry.torokhov@gmail.com, linux-input, linux-kernel,
Gaurav Kohli, Peter Hutterer, Martin Kepplinger, Paul E. McKenney
In-Reply-To: <20190424130711.GP2217@ZenIV.linux.org.uk>
On 4/24/2019 6:37 PM, Al Viro wrote:
> On Wed, Apr 24, 2019 at 05:40:40PM +0530, Mukesh Ojha wrote:
>> Al,
>>
>> i tried to put traceprintk inside ioctl after fdget and fdput on a simple
>> call of open => ioctl => close
> in a loop, and multithreaded, presumably?
>
>> on /dev/uinput.
>>
>> uinput-532 [002] .... 45.312044: SYSC_ioctl: 2 <= f_count
>>> <After fdget()
>> uinput-532 [002] .... 45.312055: SYSC_ioctl: 2
>> <After fdput()
>> uinput-532 [004] .... 45.313766: uinput_open: uinput: 1
>> uinput-532 [004] .... 45.313783: SYSC_ioctl: 1
>> uinput-532 [004] .... 45.313788: uinput_ioctl_handler:
>> uinput: uinput_ioctl_handler, 1
>> uinput-532 [004] .... 45.313835: SYSC_ioctl: 1
>> uinput-532 [004] .... 45.313843: uinput_release: uinput: 0
>>
>>
>> So while a ioctl is running the f_count is 1, so a fput could be run and do
>> atomic_long_dec_and_test
>> this could call release right ?
> Look at ksys_ioctl():
> int ksys_ioctl(unsigned int fd, unsigned int cmd, unsigned long arg)
> {
> int error;
> struct fd f = fdget(fd);
> an error or refcount bumped
> if (!f.file)
> return -EBADF;
> not an error, then. We know that ->release() won't be called
> until we drop the reference we've just acquired.
> error = security_file_ioctl(f.file, cmd, arg);
> if (!error)
> error = do_vfs_ioctl(f.file, fd, cmd, arg);
> ... and we are done with calling ->ioctl(), so
> fdput(f);
> ... we drop the reference we'd acquired.
>
> Seeing refcount 1 inside ->ioctl() is possible, all right:
>
> CPU1: ioctl(2) resolves fd to struct file *, refcount 2
> CPU2: close(2) rips struct file * from descriptor table and does fput() to drop it;
> refcount reaches 1 and fput() is done; no call of ->release() yet.
> CPU1: we get arouund to ->ioctl(), where your trace sees refcount 1
> CPU1: done with ->ioctl(), drop our reference. *NOW* refcount gets to 0, and
> ->release() is called.
Thanks for the detail reply, Al
This was my simple program no multithreading just to understand f_counting
int main()
{
int fd = open("/dev/uinput", O_WRONLY | O_NONBLOCK);
ioctl(fd, UI_SET_EVBIT, EV_KEY);
close(fd);
return 0;
}
uinput-532 [002] .... 45.312044: SYSC_ioctl: 2 <=
f_count > <After fdget()
uinput-532 [002] .... 45.312055: SYSC_ioctl:
2 <After fdput()
uinput-532 [004] .... 45.313766: uinput_open: uinput:
1 /* This is from the uinput driver uinput_open()*/
=>>>> /* All the above calls happened for the
open() in userspace*/
uinput-532 [004] .... 45.313783: SYSC_ioctl: 1 /* This
print is for the trace, i put after fdget */
uinput-532 [004] .... 45.313788: uinput_ioctl_handler:
uinput: uinput_ioctl_handler, 1 /* This print is from the uinput_ioctl
driver */
uinput-532 [004] .... 45.313835: SYSC_ioctl: 1 /* This
print is for the trace, i put after fdput*/
uinput-532 [004] .... 45.313843: uinput_release:
uinput: 0 /* And this is from the close() */
Should fdget not suppose to increment the f_count here, as it is coming 1 ?
This f_count to one is done at the open, but i have no idea how this
below f_count 2 came before open() for
this simple program.
uinput-532 [002] .... 45.312044: SYSC_ioctl: 2 <= f_count
> <After fdget()
uinput-532 [002] .... 45.312055: SYSC_ioctl:
2 <After fdput()
-Mukesh
> IOW, in your trace fput() has already been run by close(2); having somebody else
> do that again while we are in ->ioctl() would be a bug (to start with, where
> did they get that struct file * and why wasn't that reference contributing to
> struct file refcount?)
>
> In all cases we only call ->release() once all references gone - both
> the one(s) in descriptor tables and any transient ones acquired by
> fdget(), etc.
>
> I would really like to see a reproducer for the original use-after-free report...
^ permalink raw reply
* Re: [PATCH 1/3] mfd: apple-ibridge: Add Apple iBridge MFD driver.
From: Benjamin Tissoires @ 2019-04-24 14:18 UTC (permalink / raw)
To: Ronald Tschalär
Cc: Jiri Kosina, Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, Lee Jones, open list:HID CORE LAYER,
linux-iio, lkml
In-Reply-To: <20190422031251.11968-2-ronald@innovation.ch>
On Mon, Apr 22, 2019 at 5:13 AM Ronald Tschalär <ronald@innovation.ch> wrote:
>
> The iBridge device provides access to several devices, including:
> - the Touch Bar
> - the iSight webcam
> - the light sensor
> - the fingerprint sensor
>
> This driver provides the core support for managing the iBridge device
> and the access to the underlying devices. In particular, since the
> functionality for the touch bar and light sensor is exposed via USB HID
> interfaces, and the same HID device is used for multiple functions, this
> driver provides a multiplexing layer that allows multiple HID drivers to
> be registered for a given HID device. This allows the touch bar and ALS
> driver to be separated out into their own modules.
Sorry for coming late to the party, but IMO this series is far too
complex for what you need.
As I read this and the first comment of drivers/mfd/apple-ibridge.c,
you need to have a HID driver that multiplex 2 other sub drivers
through one USB communication.
For that, you are using MFD, platform driver and you own sauce instead
of creating a bus.
So, how about we reuse entirely the HID subsystem which already
provides the capability you need (assuming I am correct above).
hid-logitech-dj already does the same kind of stuff and you could:
- create drivers/hid/hid-ibridge.c that handles USB_ID_PRODUCT_IBRIDGE
- hid-ibridge will then register itself to the hid subsystem with a
call to hid_hw_start(hdev, HID_CONNECT_HIDRAW) and
hid_device_io_start(hdev) to enable the events (so you don't create
useless input nodes for it)
- then you add your 2 new devices by calling hid_allocate_device() and
then hid_add_device(). You can even create a new HID group
APPLE_IBRIDGE and allocate 2 new PIDs for them to distinguish them
from the actual USB device.
- then you have 2 brand new HID devices you can create their driver as
a regular ones.
hid-ibridge.c would just need to behave like any other hid transport
driver (see logi_dj_ll_driver in drivers/hid/hid-logitech-dj.c) and
you can get rid of at least the MFD and the platform part of your
drivers.
Does it makes sense or am I missing something obvious in the middle?
I have one other comment below.
Note that I haven't read the whole series as I'd like to first get
your feedback with my comment above.
>
> Signed-off-by: Ronald Tschalär <ronald@innovation.ch>
> ---
> drivers/mfd/Kconfig | 15 +
> drivers/mfd/Makefile | 1 +
> drivers/mfd/apple-ibridge.c | 883 ++++++++++++++++++++++++++++++
> include/linux/mfd/apple-ibridge.h | 39 ++
> 4 files changed, 938 insertions(+)
> create mode 100644 drivers/mfd/apple-ibridge.c
> create mode 100644 include/linux/mfd/apple-ibridge.h
>
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 76f9909cf396..d55fa77faacf 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -1916,5 +1916,20 @@ config RAVE_SP_CORE
> Select this to get support for the Supervisory Processor
> device found on several devices in RAVE line of hardware.
>
> +config MFD_APPLE_IBRIDGE
> + tristate "Apple iBridge chip"
> + depends on ACPI
> + depends on USB_HID
> + depends on X86 || COMPILE_TEST
> + select MFD_CORE
> + help
> + This MFD provides the core support for the Apple iBridge chip
> + found on recent MacBookPro's. The drivers for the Touch Bar
> + (apple-ib-tb) and light sensor (apple-ib-als) need to be
> + enabled separately.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called apple-ibridge.
> +
> endmenu
> endif
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 12980a4ad460..c364e0e9d313 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -241,4 +241,5 @@ obj-$(CONFIG_MFD_MXS_LRADC) += mxs-lradc.o
> obj-$(CONFIG_MFD_SC27XX_PMIC) += sprd-sc27xx-spi.o
> obj-$(CONFIG_RAVE_SP_CORE) += rave-sp.o
> obj-$(CONFIG_MFD_ROHM_BD718XX) += rohm-bd718x7.o
> +obj-$(CONFIG_MFD_APPLE_IBRIDGE) += apple-ibridge.o
>
> diff --git a/drivers/mfd/apple-ibridge.c b/drivers/mfd/apple-ibridge.c
> new file mode 100644
> index 000000000000..56d325396961
> --- /dev/null
> +++ b/drivers/mfd/apple-ibridge.c
> @@ -0,0 +1,883 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Apple iBridge Driver
> + *
> + * Copyright (c) 2018 Ronald Tschalär
> + */
> +
> +/**
> + * MacBookPro models with a Touch Bar (13,[23] and 14,[23]) have an Apple
> + * iBridge chip (also known as T1 chip) which exposes the touch bar,
> + * built-in webcam (iSight), ambient light sensor, and Secure Enclave
> + * Processor (SEP) for TouchID. It shows up in the system as a USB device
> + * with 3 configurations: 'Default iBridge Interfaces', 'Default iBridge
> + * Interfaces(OS X)', and 'Default iBridge Interfaces(Recovery)'. While
> + * the second one is used by MacOS to provide the fancy touch bar
> + * functionality with custom buttons etc, this driver just uses the first.
> + *
> + * In the first (default after boot) configuration, 4 usb interfaces are
> + * exposed: 2 related to the webcam, and 2 USB HID interfaces representing
> + * the touch bar and the ambient light sensor (and possibly the SEP,
> + * though at this point in time nothing is known about that). The webcam
> + * interfaces are already handled by the uvcvideo driver; furthermore, the
> + * handling of the input reports when "keys" on the touch bar are pressed
> + * is already handled properly by the generic USB HID core. This leaves
> + * the management of the touch bar modes (e.g. switching between function
> + * and special keys when the FN key is pressed), the touch bar display
> + * (dimming and turning off), the key-remapping when the FN key is
> + * pressed, and handling of the light sensor.
> + *
> + * This driver is implemented as an MFD driver, with the touch bar and ALS
> + * functions implemented by appropriate subdrivers (mfd cells). Because
> + * both those are basically hid drivers, but the current kernel driver
> + * structure does not allow more than one driver per device, this driver
> + * implements a demuxer for hid drivers: it registers itself as a hid
> + * driver with the core, and in turn it lets the subdrivers register
> + * themselves as hid drivers with this driver; the callbacks from the core
> + * are then forwarded to the subdrivers.
> + *
> + * Lastly, this driver also takes care of the power-management for the
> + * iBridge when suspending and resuming.
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/device.h>
> +#include <linux/hid.h>
> +#include <linux/list.h>
> +#include <linux/mfd/apple-ibridge.h>
> +#include <linux/mfd/core.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/rculist.h>
> +#include <linux/slab.h>
> +#include <linux/srcu.h>
> +#include <linux/usb.h>
> +
> +#include "../hid/usbhid/usbhid.h"
> +
> +#define USB_ID_VENDOR_APPLE 0x05ac
> +#define USB_ID_PRODUCT_IBRIDGE 0x8600
> +
> +#define APPLETB_BASIC_CONFIG 1
> +
> +#define LOG_DEV(ib_dev) (&(ib_dev)->acpi_dev->dev)
> +
> +struct appleib_device {
> + struct acpi_device *acpi_dev;
> + acpi_handle asoc_socw;
> + struct list_head hid_drivers;
> + struct list_head hid_devices;
> + struct mfd_cell *subdevs;
> + struct mutex update_lock; /* protect updates to all lists */
> + struct srcu_struct lists_srcu;
> + bool in_hid_probe;
> +};
> +
> +struct appleib_hid_drv_info {
> + struct list_head entry;
> + struct hid_driver *driver;
> + void *driver_data;
> +};
> +
> +struct appleib_hid_dev_info {
> + struct list_head entry;
> + struct list_head drivers;
> + struct hid_device *device;
> + const struct hid_device_id *device_id;
> + bool started;
> +};
> +
> +static const struct mfd_cell appleib_subdevs[] = {
> + { .name = PLAT_NAME_IB_TB },
> + { .name = PLAT_NAME_IB_ALS },
> +};
> +
> +static struct appleib_device *appleib_dev;
> +
> +#define call_void_driver_func(drv_info, fn, ...) \
> + do { \
> + if ((drv_info)->driver->fn) \
> + (drv_info)->driver->fn(__VA_ARGS__); \
> + } while (0)
> +
> +#define call_driver_func(drv_info, fn, ret_type, ...) \
> + ({ \
> + ret_type rc = 0; \
> + \
> + if ((drv_info)->driver->fn) \
> + rc = (drv_info)->driver->fn(__VA_ARGS__); \
> + \
> + rc; \
> + })
> +
> +static void appleib_remove_driver(struct appleib_device *ib_dev,
> + struct appleib_hid_drv_info *drv_info,
> + struct appleib_hid_dev_info *dev_info)
> +{
> + list_del_rcu(&drv_info->entry);
> + synchronize_srcu(&ib_dev->lists_srcu);
> +
> + call_void_driver_func(drv_info, remove, dev_info->device);
> +
> + kfree(drv_info);
> +}
> +
> +static int appleib_probe_driver(struct appleib_hid_drv_info *drv_info,
> + struct appleib_hid_dev_info *dev_info)
> +{
> + struct appleib_hid_drv_info *d;
> + int rc = 0;
> +
> + rc = call_driver_func(drv_info, probe, int, dev_info->device,
> + dev_info->device_id);
> + if (rc)
> + return rc;
> +
> + d = kmemdup(drv_info, sizeof(*drv_info), GFP_KERNEL);
> + if (!d) {
> + call_void_driver_func(drv_info, remove, dev_info->device);
> + return -ENOMEM;
> + }
> +
> + list_add_tail_rcu(&d->entry, &dev_info->drivers);
> + return 0;
> +}
> +
> +static void appleib_remove_driver_attachments(struct appleib_device *ib_dev,
> + struct appleib_hid_dev_info *dev_info,
> + struct hid_driver *driver)
> +{
> + struct appleib_hid_drv_info *drv_info;
> + struct appleib_hid_drv_info *tmp;
> +
> + list_for_each_entry_safe(drv_info, tmp, &dev_info->drivers, entry) {
> + if (!driver || drv_info->driver == driver)
> + appleib_remove_driver(ib_dev, drv_info, dev_info);
> + }
> +}
> +
> +/*
> + * Find all devices that are attached to this driver and detach them.
> + *
> + * Note: this must be run with update_lock held.
> + */
> +static void appleib_detach_devices(struct appleib_device *ib_dev,
> + struct hid_driver *driver)
> +{
> + struct appleib_hid_dev_info *dev_info;
> +
> + list_for_each_entry(dev_info, &ib_dev->hid_devices, entry)
> + appleib_remove_driver_attachments(ib_dev, dev_info, driver);
> +}
> +
> +/*
> + * Find all drivers that are attached to this device and detach them.
> + *
> + * Note: this must be run with update_lock held.
> + */
> +static void appleib_detach_drivers(struct appleib_device *ib_dev,
> + struct appleib_hid_dev_info *dev_info)
> +{
> + appleib_remove_driver_attachments(ib_dev, dev_info, NULL);
> +}
> +
> +/**
> + * Unregister a previously registered HID driver from us.
> + * @ib_dev: the appleib_device from which to unregister the driver
> + * @driver: the driver to unregister
> + */
> +int appleib_unregister_hid_driver(struct appleib_device *ib_dev,
> + struct hid_driver *driver)
> +{
> + struct appleib_hid_drv_info *drv_info;
> +
> + mutex_lock(&ib_dev->update_lock);
> +
> + list_for_each_entry(drv_info, &ib_dev->hid_drivers, entry) {
> + if (drv_info->driver == driver) {
> + appleib_detach_devices(ib_dev, driver);
> + list_del_rcu(&drv_info->entry);
> + mutex_unlock(&ib_dev->update_lock);
> + synchronize_srcu(&ib_dev->lists_srcu);
> + kfree(drv_info);
> + dev_info(LOG_DEV(ib_dev), "unregistered driver '%s'\n",
> + driver->name);
> + return 0;
> + }
> + }
> +
> + mutex_unlock(&ib_dev->update_lock);
> +
> + return -ENOENT;
> +}
> +EXPORT_SYMBOL_GPL(appleib_unregister_hid_driver);
> +
> +static int appleib_start_hid_events(struct appleib_hid_dev_info *dev_info)
> +{
> + struct hid_device *hdev = dev_info->device;
> + int rc;
> +
> + rc = hid_connect(hdev, HID_CONNECT_DEFAULT);
> + if (rc) {
> + hid_err(hdev, "ib: hid connect failed (%d)\n", rc);
> + return rc;
> + }
> +
> + rc = hid_hw_open(hdev);
> + if (rc) {
> + hid_err(hdev, "ib: failed to open hid: %d\n", rc);
> + hid_disconnect(hdev);
> + }
> +
> + if (!rc)
> + dev_info->started = true;
> +
> + return rc;
> +}
> +
> +static void appleib_stop_hid_events(struct appleib_hid_dev_info *dev_info)
> +{
> + if (dev_info->started) {
> + hid_hw_close(dev_info->device);
> + hid_disconnect(dev_info->device);
> + dev_info->started = false;
> + }
> +}
> +
> +/**
> + * Register a HID driver with us.
> + * @ib_dev: the appleib_device with which to register the driver
> + * @driver: the driver to register
> + * @data: the driver-data to associate with the driver; this is available
> + * from appleib_get_drvdata(...).
> + */
> +int appleib_register_hid_driver(struct appleib_device *ib_dev,
> + struct hid_driver *driver, void *data)
> +{
> + struct appleib_hid_drv_info *drv_info;
> + struct appleib_hid_dev_info *dev_info;
> + int rc;
> +
> + if (!driver->probe)
> + return -EINVAL;
> +
> + drv_info = kzalloc(sizeof(*drv_info), GFP_KERNEL);
> + if (!drv_info)
> + return -ENOMEM;
> +
> + drv_info->driver = driver;
> + drv_info->driver_data = data;
> +
> + mutex_lock(&ib_dev->update_lock);
> +
> + list_add_tail_rcu(&drv_info->entry, &ib_dev->hid_drivers);
> +
> + list_for_each_entry(dev_info, &ib_dev->hid_devices, entry) {
> + appleib_stop_hid_events(dev_info);
> +
> + appleib_probe_driver(drv_info, dev_info);
> +
> + rc = appleib_start_hid_events(dev_info);
> + if (rc)
> + appleib_detach_drivers(ib_dev, dev_info);
> + }
> +
> + mutex_unlock(&ib_dev->update_lock);
> +
> + dev_info(LOG_DEV(ib_dev), "registered driver '%s'\n", driver->name);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(appleib_register_hid_driver);
> +
> +/**
> + * Get the driver-specific data associated with the given, previously
> + * registered HID driver (provided in the appleib_register_hid_driver()
> + * call).
> + * @ib_dev: the appleib_device with which the driver was registered
> + * @driver: the driver for which to get the data
> + */
> +void *appleib_get_drvdata(struct appleib_device *ib_dev,
> + struct hid_driver *driver)
> +{
> + struct appleib_hid_drv_info *drv_info;
> + void *drv_data = NULL;
> + int idx;
> +
> + idx = srcu_read_lock(&ib_dev->lists_srcu);
> +
> + list_for_each_entry_rcu(drv_info, &ib_dev->hid_drivers, entry) {
> + if (drv_info->driver == driver) {
> + drv_data = drv_info->driver_data;
> + break;
> + }
> + }
> +
> + srcu_read_unlock(&ib_dev->lists_srcu, idx);
> +
> + return drv_data;
> +}
> +EXPORT_SYMBOL_GPL(appleib_get_drvdata);
> +
> +/*
> + * Forward a hid-driver callback to all registered sub-drivers. This is for
> + * callbacks that return a status as an int.
> + * @hdev the hid-device
> + * @forward a function that calls the callback on the given driver
> + * @args arguments for the forward function
> + */
> +static int appleib_forward_int_op(struct hid_device *hdev,
> + int (*forward)(struct appleib_hid_drv_info *,
> + struct hid_device *, void *),
> + void *args)
> +{
> + struct appleib_device *ib_dev = hid_get_drvdata(hdev);
> + struct appleib_hid_dev_info *dev_info;
> + struct appleib_hid_drv_info *drv_info;
> + int idx;
> + int rc = 0;
> +
> + idx = srcu_read_lock(&ib_dev->lists_srcu);
> +
> + list_for_each_entry_rcu(dev_info, &ib_dev->hid_devices, entry) {
> + if (dev_info->device != hdev)
> + continue;
> +
> + list_for_each_entry_rcu(drv_info, &dev_info->drivers, entry) {
> + rc = forward(drv_info, hdev, args);
> + if (rc)
> + break;
> + }
> +
> + break;
> + }
> +
> + srcu_read_unlock(&ib_dev->lists_srcu, idx);
> +
> + return rc;
> +}
> +
> +struct appleib_hid_event_args {
> + struct hid_field *field;
> + struct hid_usage *usage;
> + __s32 value;
> +};
> +
> +static int appleib_hid_event_fwd(struct appleib_hid_drv_info *drv_info,
> + struct hid_device *hdev, void *args)
> +{
> + struct appleib_hid_event_args *evt_args = args;
> +
> + return call_driver_func(drv_info, event, int, hdev, evt_args->field,
> + evt_args->usage, evt_args->value);
> +}
> +
> +static int appleib_hid_event(struct hid_device *hdev, struct hid_field *field,
> + struct hid_usage *usage, __s32 value)
> +{
> + struct appleib_hid_event_args args = {
> + .field = field,
> + .usage = usage,
> + .value = value,
> + };
> +
> + return appleib_forward_int_op(hdev, appleib_hid_event_fwd, &args);
> +}
> +
> +static __u8 *appleib_report_fixup(struct hid_device *hdev, __u8 *rdesc,
> + unsigned int *rsize)
> +{
> + /* Some fields have a size of 64 bits, which according to HID 1.11
> + * Section 8.4 is not valid ("An item field cannot span more than 4
> + * bytes in a report"). Furthermore, hid_field_extract() complains
this must have been fixed in 94a9992f7dbdfb28976b565af220e0c4a117144a
which is part of v5.1, so not sure you actually need the report
descriptor fixup at all.
Cheers,
Benjamin
> + * when encountering such a field. So turn them into two 32-bit fields
> + * instead.
> + */
> +
> + if (*rsize == 634 &&
> + /* Usage Page 0xff12 (vendor defined) */
> + rdesc[212] == 0x06 && rdesc[213] == 0x12 && rdesc[214] == 0xff &&
> + /* Usage 0x51 */
> + rdesc[416] == 0x09 && rdesc[417] == 0x51 &&
> + /* report size 64 */
> + rdesc[432] == 0x75 && rdesc[433] == 64 &&
> + /* report count 1 */
> + rdesc[434] == 0x95 && rdesc[435] == 1) {
> + rdesc[433] = 32;
> + rdesc[435] = 2;
> + hid_dbg(hdev, "Fixed up first 64-bit field\n");
> + }
> +
> + if (*rsize == 634 &&
> + /* Usage Page 0xff12 (vendor defined) */
> + rdesc[212] == 0x06 && rdesc[213] == 0x12 && rdesc[214] == 0xff &&
> + /* Usage 0x51 */
> + rdesc[611] == 0x09 && rdesc[612] == 0x51 &&
> + /* report size 64 */
> + rdesc[627] == 0x75 && rdesc[628] == 64 &&
> + /* report count 1 */
> + rdesc[629] == 0x95 && rdesc[630] == 1) {
> + rdesc[628] = 32;
> + rdesc[630] = 2;
> + hid_dbg(hdev, "Fixed up second 64-bit field\n");
> + }
> +
> + return rdesc;
> +}
> +
> +static int appleib_input_configured_fwd(struct appleib_hid_drv_info *drv_info,
> + struct hid_device *hdev, void *args)
> +{
> + return call_driver_func(drv_info, input_configured, int, hdev,
> + (struct hid_input *)args);
> +}
> +
> +static int appleib_input_configured(struct hid_device *hdev,
> + struct hid_input *hidinput)
> +{
> + return appleib_forward_int_op(hdev, appleib_input_configured_fwd,
> + hidinput);
> +}
> +
> +#ifdef CONFIG_PM
> +static int appleib_hid_suspend_fwd(struct appleib_hid_drv_info *drv_info,
> + struct hid_device *hdev, void *args)
> +{
> + return call_driver_func(drv_info, suspend, int, hdev,
> + *(pm_message_t *)args);
> +}
> +
> +static int appleib_hid_suspend(struct hid_device *hdev, pm_message_t message)
> +{
> + return appleib_forward_int_op(hdev, appleib_hid_suspend_fwd, &message);
> +}
> +
> +static int appleib_hid_resume_fwd(struct appleib_hid_drv_info *drv_info,
> + struct hid_device *hdev, void *args)
> +{
> + return call_driver_func(drv_info, resume, int, hdev);
> +}
> +
> +static int appleib_hid_resume(struct hid_device *hdev)
> +{
> + return appleib_forward_int_op(hdev, appleib_hid_resume_fwd, NULL);
> +}
> +
> +static int appleib_hid_reset_resume_fwd(struct appleib_hid_drv_info *drv_info,
> + struct hid_device *hdev, void *args)
> +{
> + return call_driver_func(drv_info, reset_resume, int, hdev);
> +}
> +
> +static int appleib_hid_reset_resume(struct hid_device *hdev)
> +{
> + return appleib_forward_int_op(hdev, appleib_hid_reset_resume_fwd, NULL);
> +}
> +#endif /* CONFIG_PM */
> +
> +/**
> + * Find the field in the report with the given usage.
> + * @report: the report to search
> + * @field_usage: the usage of the field to search for
> + */
> +struct hid_field *appleib_find_report_field(struct hid_report *report,
> + unsigned int field_usage)
> +{
> + int f, u;
> +
> + for (f = 0; f < report->maxfield; f++) {
> + struct hid_field *field = report->field[f];
> +
> + if (field->logical == field_usage)
> + return field;
> +
> + for (u = 0; u < field->maxusage; u++) {
> + if (field->usage[u].hid == field_usage)
> + return field;
> + }
> + }
> +
> + return NULL;
> +}
> +EXPORT_SYMBOL_GPL(appleib_find_report_field);
> +
> +/**
> + * Search all the reports of the device for the field with the given usage.
> + * @hdev: the device whose reports to search
> + * @application: the usage of application collection that the field must
> + * belong to
> + * @field_usage: the usage of the field to search for
> + */
> +struct hid_field *appleib_find_hid_field(struct hid_device *hdev,
> + unsigned int application,
> + unsigned int field_usage)
> +{
> + static const int report_types[] = { HID_INPUT_REPORT, HID_OUTPUT_REPORT,
> + HID_FEATURE_REPORT };
> + struct hid_report *report;
> + struct hid_field *field;
> + int t;
> +
> + for (t = 0; t < ARRAY_SIZE(report_types); t++) {
> + struct list_head *report_list =
> + &hdev->report_enum[report_types[t]].report_list;
> + list_for_each_entry(report, report_list, list) {
> + if (report->application != application)
> + continue;
> +
> + field = appleib_find_report_field(report, field_usage);
> + if (field)
> + return field;
> + }
> + }
> +
> + return NULL;
> +}
> +EXPORT_SYMBOL_GPL(appleib_find_hid_field);
> +
> +/**
> + * Return whether we're currently inside a hid_device_probe or not.
> + * @ib_dev: the appleib_device
> + */
> +bool appleib_in_hid_probe(struct appleib_device *ib_dev)
> +{
> + return ib_dev->in_hid_probe;
> +}
> +EXPORT_SYMBOL_GPL(appleib_in_hid_probe);
> +
> +static struct appleib_hid_dev_info *
> +appleib_add_device(struct appleib_device *ib_dev, struct hid_device *hdev,
> + const struct hid_device_id *id)
> +{
> + struct appleib_hid_dev_info *dev_info;
> + struct appleib_hid_drv_info *drv_info;
> +
> + /* allocate device-info for this device */
> + dev_info = kzalloc(sizeof(*dev_info), GFP_KERNEL);
> + if (!dev_info)
> + return NULL;
> +
> + INIT_LIST_HEAD(&dev_info->drivers);
> + dev_info->device = hdev;
> + dev_info->device_id = id;
> +
> + /* notify all our sub drivers */
> + mutex_lock(&ib_dev->update_lock);
> +
> + ib_dev->in_hid_probe = true;
> +
> + list_for_each_entry(drv_info, &ib_dev->hid_drivers, entry) {
> + appleib_probe_driver(drv_info, dev_info);
> + }
> +
> + ib_dev->in_hid_probe = false;
> +
> + /* remember this new device */
> + list_add_tail_rcu(&dev_info->entry, &ib_dev->hid_devices);
> +
> + mutex_unlock(&ib_dev->update_lock);
> +
> + return dev_info;
> +}
> +
> +static void appleib_remove_device(struct appleib_device *ib_dev,
> + struct appleib_hid_dev_info *dev_info)
> +{
> + list_del_rcu(&dev_info->entry);
> + synchronize_srcu(&ib_dev->lists_srcu);
> +
> + appleib_detach_drivers(ib_dev, dev_info);
> +
> + kfree(dev_info);
> +}
> +
> +static int appleib_hid_probe(struct hid_device *hdev,
> + const struct hid_device_id *id)
> +{
> + struct appleib_device *ib_dev;
> + struct appleib_hid_dev_info *dev_info;
> + struct usb_device *udev;
> + int rc;
> +
> + /* check usb config first */
> + udev = hid_to_usb_dev(hdev);
> +
> + if (udev->actconfig->desc.bConfigurationValue != APPLETB_BASIC_CONFIG) {
> + rc = usb_driver_set_configuration(udev, APPLETB_BASIC_CONFIG);
> + return rc ? rc : -ENODEV;
> + }
> +
> + /* Assign the driver data */
> + ib_dev = appleib_dev;
> + hid_set_drvdata(hdev, ib_dev);
> +
> + /* initialize the report info */
> + rc = hid_parse(hdev);
> + if (rc) {
> + hid_err(hdev, "ib: hid parse failed (%d)\n", rc);
> + goto error;
> + }
> +
> + /* alloc bufs etc so probe's can send requests; but connect later */
> + rc = hid_hw_start(hdev, 0);
> + if (rc) {
> + hid_err(hdev, "ib: hw start failed (%d)\n", rc);
> + goto error;
> + }
> +
> + /* add this hdev to our device list */
> + dev_info = appleib_add_device(ib_dev, hdev, id);
> + if (!dev_info) {
> + rc = -ENOMEM;
> + goto stop_hw;
> + }
> +
> + /* start the hid */
> + rc = appleib_start_hid_events(dev_info);
> + if (rc)
> + goto remove_dev;
> +
> + return 0;
> +
> +remove_dev:
> + mutex_lock(&ib_dev->update_lock);
> + appleib_remove_device(ib_dev, dev_info);
> + mutex_unlock(&ib_dev->update_lock);
> +stop_hw:
> + hid_hw_stop(hdev);
> +error:
> + return rc;
> +}
> +
> +static void appleib_hid_remove(struct hid_device *hdev)
> +{
> + struct appleib_device *ib_dev = hid_get_drvdata(hdev);
> + struct appleib_hid_dev_info *dev_info;
> +
> + mutex_lock(&ib_dev->update_lock);
> +
> + list_for_each_entry(dev_info, &ib_dev->hid_devices, entry) {
> + if (dev_info->device == hdev) {
> + appleib_stop_hid_events(dev_info);
> + appleib_remove_device(ib_dev, dev_info);
> + break;
> + }
> + }
> +
> + mutex_unlock(&ib_dev->update_lock);
> +
> + hid_hw_stop(hdev);
> +}
> +
> +static const struct hid_device_id appleib_hid_devices[] = {
> + { HID_USB_DEVICE(USB_ID_VENDOR_APPLE, USB_ID_PRODUCT_IBRIDGE) },
> + { },
> +};
> +
> +static struct hid_driver appleib_hid_driver = {
> + .name = "apple-ibridge-hid",
> + .id_table = appleib_hid_devices,
> + .probe = appleib_hid_probe,
> + .remove = appleib_hid_remove,
> + .event = appleib_hid_event,
> + .report_fixup = appleib_report_fixup,
> + .input_configured = appleib_input_configured,
> +#ifdef CONFIG_PM
> + .suspend = appleib_hid_suspend,
> + .resume = appleib_hid_resume,
> + .reset_resume = appleib_hid_reset_resume,
> +#endif
> +};
> +
> +static struct appleib_device *appleib_alloc_device(struct acpi_device *acpi_dev)
> +{
> + struct appleib_device *ib_dev;
> + acpi_status sts;
> + int rc;
> +
> + /* allocate */
> + ib_dev = kzalloc(sizeof(*ib_dev), GFP_KERNEL);
> + if (!ib_dev)
> + return ERR_PTR(-ENOMEM);
> +
> + /* init structures */
> + INIT_LIST_HEAD(&ib_dev->hid_drivers);
> + INIT_LIST_HEAD(&ib_dev->hid_devices);
> + mutex_init(&ib_dev->update_lock);
> + init_srcu_struct(&ib_dev->lists_srcu);
> +
> + ib_dev->acpi_dev = acpi_dev;
> +
> + /* get iBridge acpi power control method */
> + sts = acpi_get_handle(acpi_dev->handle, "SOCW", &ib_dev->asoc_socw);
> + if (ACPI_FAILURE(sts)) {
> + dev_err(LOG_DEV(ib_dev),
> + "Error getting handle for ASOC.SOCW method: %s\n",
> + acpi_format_exception(sts));
> + rc = -ENXIO;
> + goto free_mem;
> + }
> +
> + /* ensure iBridge is powered on */
> + sts = acpi_execute_simple_method(ib_dev->asoc_socw, NULL, 1);
> + if (ACPI_FAILURE(sts))
> + dev_warn(LOG_DEV(ib_dev), "SOCW(1) failed: %s\n",
> + acpi_format_exception(sts));
> +
> + return ib_dev;
> +
> +free_mem:
> + kfree(ib_dev);
> + return ERR_PTR(rc);
> +}
> +
> +static int appleib_probe(struct acpi_device *acpi)
> +{
> + struct appleib_device *ib_dev;
> + struct appleib_platform_data *pdata;
> + int i;
> + int ret;
> +
> + if (appleib_dev)
> + return -EBUSY;
> +
> + ib_dev = appleib_alloc_device(acpi);
> + if (IS_ERR_OR_NULL(ib_dev))
> + return PTR_ERR(ib_dev);
> +
> + ib_dev->subdevs = kmemdup(appleib_subdevs, sizeof(appleib_subdevs),
> + GFP_KERNEL);
> + if (!ib_dev->subdevs) {
> + ret = -ENOMEM;
> + goto free_dev;
> + }
> +
> + pdata = kzalloc(sizeof(*pdata), GFP_KERNEL);
> + if (!pdata) {
> + ret = -ENOMEM;
> + goto free_subdevs;
> + }
> +
> + pdata->ib_dev = ib_dev;
> + pdata->log_dev = LOG_DEV(ib_dev);
> + for (i = 0; i < ARRAY_SIZE(appleib_subdevs); i++) {
> + ib_dev->subdevs[i].platform_data = pdata;
> + ib_dev->subdevs[i].pdata_size = sizeof(*pdata);
> + }
> +
> + ret = mfd_add_devices(&acpi->dev, PLATFORM_DEVID_NONE,
> + ib_dev->subdevs, ARRAY_SIZE(appleib_subdevs),
> + NULL, 0, NULL);
> + if (ret) {
> + dev_err(LOG_DEV(ib_dev), "Error adding MFD devices: %d\n", ret);
> + goto free_pdata;
> + }
> +
> + acpi->driver_data = ib_dev;
> + appleib_dev = ib_dev;
> +
> + ret = hid_register_driver(&appleib_hid_driver);
> + if (ret) {
> + dev_err(LOG_DEV(ib_dev), "Error registering hid driver: %d\n",
> + ret);
> + goto rem_mfd_devs;
> + }
> +
> + return 0;
> +
> +rem_mfd_devs:
> + mfd_remove_devices(&acpi->dev);
> +free_pdata:
> + kfree(pdata);
> +free_subdevs:
> + kfree(ib_dev->subdevs);
> +free_dev:
> + appleib_dev = NULL;
> + acpi->driver_data = NULL;
> + kfree(ib_dev);
> + return ret;
> +}
> +
> +static int appleib_remove(struct acpi_device *acpi)
> +{
> + struct appleib_device *ib_dev = acpi_driver_data(acpi);
> +
> + mfd_remove_devices(&acpi->dev);
> + hid_unregister_driver(&appleib_hid_driver);
> +
> + if (appleib_dev == ib_dev)
> + appleib_dev = NULL;
> +
> + kfree(ib_dev->subdevs[0].platform_data);
> + kfree(ib_dev->subdevs);
> + kfree(ib_dev);
> +
> + return 0;
> +}
> +
> +static int appleib_suspend(struct device *dev)
> +{
> + struct acpi_device *adev;
> + struct appleib_device *ib_dev;
> + int rc;
> +
> + adev = to_acpi_device(dev);
> + ib_dev = acpi_driver_data(adev);
> +
> + rc = acpi_execute_simple_method(ib_dev->asoc_socw, NULL, 0);
> + if (ACPI_FAILURE(rc))
> + dev_warn(LOG_DEV(ib_dev), "SOCW(0) failed: %s\n",
> + acpi_format_exception(rc));
> +
> + return 0;
> +}
> +
> +static int appleib_resume(struct device *dev)
> +{
> + struct acpi_device *adev;
> + struct appleib_device *ib_dev;
> + int rc;
> +
> + adev = to_acpi_device(dev);
> + ib_dev = acpi_driver_data(adev);
> +
> + rc = acpi_execute_simple_method(ib_dev->asoc_socw, NULL, 1);
> + if (ACPI_FAILURE(rc))
> + dev_warn(LOG_DEV(ib_dev), "SOCW(1) failed: %s\n",
> + acpi_format_exception(rc));
> +
> + return 0;
> +}
> +
> +static const struct dev_pm_ops appleib_pm = {
> + .suspend = appleib_suspend,
> + .resume = appleib_resume,
> + .restore = appleib_resume,
> +};
> +
> +static const struct acpi_device_id appleib_acpi_match[] = {
> + { "APP7777", 0 },
> + { },
> +};
> +
> +MODULE_DEVICE_TABLE(acpi, appleib_acpi_match);
> +
> +static struct acpi_driver appleib_driver = {
> + .name = "apple-ibridge",
> + .class = "topcase", /* ? */
> + .owner = THIS_MODULE,
> + .ids = appleib_acpi_match,
> + .ops = {
> + .add = appleib_probe,
> + .remove = appleib_remove,
> + },
> + .drv = {
> + .pm = &appleib_pm,
> + },
> +};
> +
> +module_acpi_driver(appleib_driver)
> +
> +MODULE_AUTHOR("Ronald Tschalär");
> +MODULE_DESCRIPTION("Apple iBridge driver");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/mfd/apple-ibridge.h b/include/linux/mfd/apple-ibridge.h
> new file mode 100644
> index 000000000000..d321714767f7
> --- /dev/null
> +++ b/include/linux/mfd/apple-ibridge.h
> @@ -0,0 +1,39 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Apple iBridge Driver
> + *
> + * Copyright (c) 2018 Ronald Tschalär
> + */
> +
> +#ifndef __LINUX_MFD_APPLE_IBRDIGE_H
> +#define __LINUX_MFD_APPLE_IBRDIGE_H
> +
> +#include <linux/device.h>
> +#include <linux/hid.h>
> +
> +#define PLAT_NAME_IB_TB "apple-ib-tb"
> +#define PLAT_NAME_IB_ALS "apple-ib-als"
> +
> +struct appleib_device;
> +
> +struct appleib_platform_data {
> + struct appleib_device *ib_dev;
> + struct device *log_dev;
> +};
> +
> +int appleib_register_hid_driver(struct appleib_device *ib_dev,
> + struct hid_driver *driver, void *data);
> +int appleib_unregister_hid_driver(struct appleib_device *ib_dev,
> + struct hid_driver *driver);
> +
> +void *appleib_get_drvdata(struct appleib_device *ib_dev,
> + struct hid_driver *driver);
> +bool appleib_in_hid_probe(struct appleib_device *ib_dev);
> +
> +struct hid_field *appleib_find_report_field(struct hid_report *report,
> + unsigned int field_usage);
> +struct hid_field *appleib_find_hid_field(struct hid_device *hdev,
> + unsigned int application,
> + unsigned int field_usage);
> +
> +#endif
> --
> 2.20.1
>
^ permalink raw reply
* [PATCH AUTOSEL 5.0 02/66] HID: Increase maximum report size allowed by hid_field_extract()
From: Sasha Levin @ 2019-04-24 14:32 UTC (permalink / raw)
To: linux-kernel, stable; +Cc: Kai-Heng Feng, Jiri Kosina, Sasha Levin, linux-input
In-Reply-To: <20190424143341.27665-1-sashal@kernel.org>
From: Kai-Heng Feng <kai.heng.feng@canonical.com>
[ Upstream commit 94a9992f7dbdfb28976b565af220e0c4a117144a ]
Commit 71f6fa90a353 ("HID: increase maximum global item tag report size
to 256") increases the max report size from 128 to 256.
We also need to update the report size in hid_field_extract() otherwise
it complains and truncates now valid report size:
[ 406.165461] hid-sensor-hub 001F:8086:22D8.0002: hid_field_extract() called with n (192) > 32! (kworker/5:1)
BugLink: https://bugs.launchpad.net/bugs/1818547
Fixes: 71f6fa90a353 ("HID: increase maximum global item tag report size to 256")
Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
Signed-off-by: Sasha Levin (Microsoft) <sashal@kernel.org>
---
drivers/hid/hid-core.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 9993b692598f..860e21ec6a49 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -1301,10 +1301,10 @@ static u32 __extract(u8 *report, unsigned offset, int n)
u32 hid_field_extract(const struct hid_device *hid, u8 *report,
unsigned offset, unsigned n)
{
- if (n > 32) {
- hid_warn(hid, "hid_field_extract() called with n (%d) > 32! (%s)\n",
+ if (n > 256) {
+ hid_warn(hid, "hid_field_extract() called with n (%d) > 256! (%s)\n",
n, current->comm);
- n = 32;
+ n = 256;
}
return __extract(report, offset, n);
--
2.19.1
^ permalink raw reply related
* [PATCH AUTOSEL 5.0 03/66] HID: logitech: check the return value of create_singlethread_workqueue
From: Sasha Levin @ 2019-04-24 14:32 UTC (permalink / raw)
To: linux-kernel, stable; +Cc: Kangjie Lu, Jiri Kosina, Sasha Levin, linux-input
In-Reply-To: <20190424143341.27665-1-sashal@kernel.org>
From: Kangjie Lu <kjlu@umn.edu>
[ Upstream commit 6c44b15e1c9076d925d5236ddadf1318b0a25ce2 ]
create_singlethread_workqueue may fail and return NULL. The fix checks if it is
NULL to avoid NULL pointer dereference. Also, the fix moves the call of
create_singlethread_workqueue earlier to avoid resource-release issues.
Signed-off-by: Kangjie Lu <kjlu@umn.edu>
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
Signed-off-by: Sasha Levin (Microsoft) <sashal@kernel.org>
---
drivers/hid/hid-logitech-hidpp.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index f040c8a7f9a9..199cc256e9d9 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -2111,6 +2111,13 @@ static int hidpp_ff_init(struct hidpp_device *hidpp, u8 feature_index)
kfree(data);
return -ENOMEM;
}
+ data->wq = create_singlethread_workqueue("hidpp-ff-sendqueue");
+ if (!data->wq) {
+ kfree(data->effect_ids);
+ kfree(data);
+ return -ENOMEM;
+ }
+
data->hidpp = hidpp;
data->feature_index = feature_index;
data->version = version;
@@ -2155,7 +2162,6 @@ static int hidpp_ff_init(struct hidpp_device *hidpp, u8 feature_index)
/* ignore boost value at response.fap.params[2] */
/* init the hardware command queue */
- data->wq = create_singlethread_workqueue("hidpp-ff-sendqueue");
atomic_set(&data->workqueue_size, 0);
/* initialize with zero autocenter to get wheel in usable state */
--
2.19.1
^ permalink raw reply related
* [PATCH AUTOSEL 5.0 04/66] HID: debug: fix race condition with between rdesc_show() and device removal
From: Sasha Levin @ 2019-04-24 14:32 UTC (permalink / raw)
To: linux-kernel, stable
Cc: He, Bo, Zhang, Jun, Jiri Kosina, Sasha Levin, linux-input
In-Reply-To: <20190424143341.27665-1-sashal@kernel.org>
From: "He, Bo" <bo.he@intel.com>
[ Upstream commit cef0d4948cb0a02db37ebfdc320e127c77ab1637 ]
There is a race condition that could happen if hid_debug_rdesc_show()
is running while hdev is in the process of going away (device removal,
system suspend, etc) which could result in NULL pointer dereference:
BUG: unable to handle kernel paging request at 0000000783316040
CPU: 1 PID: 1512 Comm: getevent Tainted: G U O 4.19.20-quilt-2e5dc0ac-00029-gc455a447dd55 #1
RIP: 0010:hid_dump_device+0x9b/0x160
Call Trace:
hid_debug_rdesc_show+0x72/0x1d0
seq_read+0xe0/0x410
full_proxy_read+0x5f/0x90
__vfs_read+0x3a/0x170
vfs_read+0xa0/0x150
ksys_read+0x58/0xc0
__x64_sys_read+0x1a/0x20
do_syscall_64+0x55/0x110
entry_SYSCALL_64_after_hwframe+0x49/0xbe
Grab driver_input_lock to make sure the input device exists throughout the
whole process of dumping the rdesc.
[jkosina@suse.cz: update changelog a bit]
Signed-off-by: "he, bo" <bo.he@intel.com>
Signed-off-by: "Zhang, Jun" <jun.zhang@intel.com>
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
Signed-off-by: Sasha Levin (Microsoft) <sashal@kernel.org>
---
drivers/hid/hid-debug.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/hid/hid-debug.c b/drivers/hid/hid-debug.c
index ac9fda1b5a72..1384e57182af 100644
--- a/drivers/hid/hid-debug.c
+++ b/drivers/hid/hid-debug.c
@@ -1060,10 +1060,15 @@ static int hid_debug_rdesc_show(struct seq_file *f, void *p)
seq_printf(f, "\n\n");
/* dump parsed data and input mappings */
+ if (down_interruptible(&hdev->driver_input_lock))
+ return 0;
+
hid_dump_device(hdev, f);
seq_printf(f, "\n");
hid_dump_input_mapping(hdev, f);
+ up(&hdev->driver_input_lock);
+
return 0;
}
--
2.19.1
^ permalink raw reply related
* [PATCH AUTOSEL 5.0 21/66] HID: quirks: Fix keyboard + touchpad on Lenovo Miix 630
From: Sasha Levin @ 2019-04-24 14:32 UTC (permalink / raw)
To: linux-kernel, stable; +Cc: Jeffrey Hugo, Jiri Kosina, Sasha Levin, linux-input
In-Reply-To: <20190424143341.27665-1-sashal@kernel.org>
From: Jeffrey Hugo <jeffrey.l.hugo@gmail.com>
[ Upstream commit 2bafa1e9625400bec4c840a168d70ba52607a58d ]
Similar to commit edfc3722cfef ("HID: quirks: Fix keyboard + touchpad on
Toshiba Click Mini not working"), the Lenovo Miix 630 has a combo
keyboard/touchpad device with vid:pid of 04F3:0400, which is shared with
Elan touchpads. The combo on the Miix 630 has an ACPI id of QTEC0001,
which is not claimed by the elan_i2c driver, so key on that similar to
what was done for the Toshiba Click Mini.
Signed-off-by: Jeffrey Hugo <jeffrey.l.hugo@gmail.com>
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
Signed-off-by: Sasha Levin (Microsoft) <sashal@kernel.org>
---
drivers/hid/hid-quirks.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/hid-quirks.c b/drivers/hid/hid-quirks.c
index 94088c0ed68a..e24790c988c0 100644
--- a/drivers/hid/hid-quirks.c
+++ b/drivers/hid/hid-quirks.c
@@ -744,7 +744,6 @@ static const struct hid_device_id hid_ignore_list[] = {
{ HID_USB_DEVICE(USB_VENDOR_ID_DEALEXTREAME, USB_DEVICE_ID_DEALEXTREAME_RADIO_SI4701) },
{ HID_USB_DEVICE(USB_VENDOR_ID_DELORME, USB_DEVICE_ID_DELORME_EARTHMATE) },
{ HID_USB_DEVICE(USB_VENDOR_ID_DELORME, USB_DEVICE_ID_DELORME_EM_LT20) },
- { HID_I2C_DEVICE(USB_VENDOR_ID_ELAN, 0x0400) },
{ HID_USB_DEVICE(USB_VENDOR_ID_ESSENTIAL_REALITY, USB_DEVICE_ID_ESSENTIAL_REALITY_P5) },
{ HID_USB_DEVICE(USB_VENDOR_ID_ETT, USB_DEVICE_ID_TC5UH) },
{ HID_USB_DEVICE(USB_VENDOR_ID_ETT, USB_DEVICE_ID_TC4UM) },
@@ -1025,6 +1024,10 @@ bool hid_ignore(struct hid_device *hdev)
if (hdev->product == 0x0401 &&
strncmp(hdev->name, "ELAN0800", 8) != 0)
return true;
+ /* Same with product id 0x0400 */
+ if (hdev->product == 0x0400 &&
+ strncmp(hdev->name, "QTEC0001", 8) != 0)
+ return true;
break;
}
--
2.19.1
^ permalink raw reply related
* [PATCH AUTOSEL 5.0 38/66] HID: input: add mapping for Assistant key
From: Sasha Levin @ 2019-04-24 14:33 UTC (permalink / raw)
To: linux-kernel, stable
Cc: Dmitry Torokhov, Jiri Kosina, Sasha Levin, linux-input
In-Reply-To: <20190424143341.27665-1-sashal@kernel.org>
From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
[ Upstream commit ce856634af8cda3490947df8ac1ef5843e6356af ]
According to HUTRR89 usage 0x1cb from the consumer page was assigned to
allow launching desktop-aware assistant application, so let's add the
mapping.
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
Signed-off-by: Sasha Levin (Microsoft) <sashal@kernel.org>
---
drivers/hid/hid-input.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index 59a5608b8dc0..ff92a7b2fc89 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -995,6 +995,7 @@ static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_fiel
case 0x1b8: map_key_clear(KEY_VIDEO); break;
case 0x1bc: map_key_clear(KEY_MESSENGER); break;
case 0x1bd: map_key_clear(KEY_INFO); break;
+ case 0x1cb: map_key_clear(KEY_ASSISTANT); break;
case 0x201: map_key_clear(KEY_NEW); break;
case 0x202: map_key_clear(KEY_OPEN); break;
case 0x203: map_key_clear(KEY_CLOSE); break;
--
2.19.1
^ permalink raw reply related
* [PATCH AUTOSEL 4.19 02/52] HID: logitech: check the return value of create_singlethread_workqueue
From: Sasha Levin @ 2019-04-24 14:38 UTC (permalink / raw)
To: linux-kernel, stable; +Cc: Kangjie Lu, Jiri Kosina, Sasha Levin, linux-input
In-Reply-To: <20190424143911.28890-1-sashal@kernel.org>
From: Kangjie Lu <kjlu@umn.edu>
[ Upstream commit 6c44b15e1c9076d925d5236ddadf1318b0a25ce2 ]
create_singlethread_workqueue may fail and return NULL. The fix checks if it is
NULL to avoid NULL pointer dereference. Also, the fix moves the call of
create_singlethread_workqueue earlier to avoid resource-release issues.
Signed-off-by: Kangjie Lu <kjlu@umn.edu>
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
drivers/hid/hid-logitech-hidpp.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 19cc980eebce..8425d3548a41 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -1907,6 +1907,13 @@ static int hidpp_ff_init(struct hidpp_device *hidpp, u8 feature_index)
kfree(data);
return -ENOMEM;
}
+ data->wq = create_singlethread_workqueue("hidpp-ff-sendqueue");
+ if (!data->wq) {
+ kfree(data->effect_ids);
+ kfree(data);
+ return -ENOMEM;
+ }
+
data->hidpp = hidpp;
data->feature_index = feature_index;
data->version = version;
@@ -1951,7 +1958,6 @@ static int hidpp_ff_init(struct hidpp_device *hidpp, u8 feature_index)
/* ignore boost value at response.fap.params[2] */
/* init the hardware command queue */
- data->wq = create_singlethread_workqueue("hidpp-ff-sendqueue");
atomic_set(&data->workqueue_size, 0);
/* initialize with zero autocenter to get wheel in usable state */
--
2.19.1
^ permalink raw reply related
* [PATCH AUTOSEL 4.19 03/52] HID: debug: fix race condition with between rdesc_show() and device removal
From: Sasha Levin @ 2019-04-24 14:38 UTC (permalink / raw)
To: linux-kernel, stable
Cc: He, Bo, Zhang, Jun, Jiri Kosina, Sasha Levin, linux-input
In-Reply-To: <20190424143911.28890-1-sashal@kernel.org>
From: "He, Bo" <bo.he@intel.com>
[ Upstream commit cef0d4948cb0a02db37ebfdc320e127c77ab1637 ]
There is a race condition that could happen if hid_debug_rdesc_show()
is running while hdev is in the process of going away (device removal,
system suspend, etc) which could result in NULL pointer dereference:
BUG: unable to handle kernel paging request at 0000000783316040
CPU: 1 PID: 1512 Comm: getevent Tainted: G U O 4.19.20-quilt-2e5dc0ac-00029-gc455a447dd55 #1
RIP: 0010:hid_dump_device+0x9b/0x160
Call Trace:
hid_debug_rdesc_show+0x72/0x1d0
seq_read+0xe0/0x410
full_proxy_read+0x5f/0x90
__vfs_read+0x3a/0x170
vfs_read+0xa0/0x150
ksys_read+0x58/0xc0
__x64_sys_read+0x1a/0x20
do_syscall_64+0x55/0x110
entry_SYSCALL_64_after_hwframe+0x49/0xbe
Grab driver_input_lock to make sure the input device exists throughout the
whole process of dumping the rdesc.
[jkosina@suse.cz: update changelog a bit]
Signed-off-by: "he, bo" <bo.he@intel.com>
Signed-off-by: "Zhang, Jun" <jun.zhang@intel.com>
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
drivers/hid/hid-debug.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/hid/hid-debug.c b/drivers/hid/hid-debug.c
index ebc9ffde41e9..a353a011fbdf 100644
--- a/drivers/hid/hid-debug.c
+++ b/drivers/hid/hid-debug.c
@@ -1060,10 +1060,15 @@ static int hid_debug_rdesc_show(struct seq_file *f, void *p)
seq_printf(f, "\n\n");
/* dump parsed data and input mappings */
+ if (down_interruptible(&hdev->driver_input_lock))
+ return 0;
+
hid_dump_device(hdev, f);
seq_printf(f, "\n");
hid_dump_input_mapping(hdev, f);
+ up(&hdev->driver_input_lock);
+
return 0;
}
--
2.19.1
^ permalink raw reply related
* [PATCH AUTOSEL 4.19 15/52] HID: quirks: Fix keyboard + touchpad on Lenovo Miix 630
From: Sasha Levin @ 2019-04-24 14:38 UTC (permalink / raw)
To: linux-kernel, stable; +Cc: Jeffrey Hugo, Jiri Kosina, Sasha Levin, linux-input
In-Reply-To: <20190424143911.28890-1-sashal@kernel.org>
From: Jeffrey Hugo <jeffrey.l.hugo@gmail.com>
[ Upstream commit 2bafa1e9625400bec4c840a168d70ba52607a58d ]
Similar to commit edfc3722cfef ("HID: quirks: Fix keyboard + touchpad on
Toshiba Click Mini not working"), the Lenovo Miix 630 has a combo
keyboard/touchpad device with vid:pid of 04F3:0400, which is shared with
Elan touchpads. The combo on the Miix 630 has an ACPI id of QTEC0001,
which is not claimed by the elan_i2c driver, so key on that similar to
what was done for the Toshiba Click Mini.
Signed-off-by: Jeffrey Hugo <jeffrey.l.hugo@gmail.com>
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
drivers/hid/hid-quirks.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/hid-quirks.c b/drivers/hid/hid-quirks.c
index 94088c0ed68a..e24790c988c0 100644
--- a/drivers/hid/hid-quirks.c
+++ b/drivers/hid/hid-quirks.c
@@ -744,7 +744,6 @@ static const struct hid_device_id hid_ignore_list[] = {
{ HID_USB_DEVICE(USB_VENDOR_ID_DEALEXTREAME, USB_DEVICE_ID_DEALEXTREAME_RADIO_SI4701) },
{ HID_USB_DEVICE(USB_VENDOR_ID_DELORME, USB_DEVICE_ID_DELORME_EARTHMATE) },
{ HID_USB_DEVICE(USB_VENDOR_ID_DELORME, USB_DEVICE_ID_DELORME_EM_LT20) },
- { HID_I2C_DEVICE(USB_VENDOR_ID_ELAN, 0x0400) },
{ HID_USB_DEVICE(USB_VENDOR_ID_ESSENTIAL_REALITY, USB_DEVICE_ID_ESSENTIAL_REALITY_P5) },
{ HID_USB_DEVICE(USB_VENDOR_ID_ETT, USB_DEVICE_ID_TC5UH) },
{ HID_USB_DEVICE(USB_VENDOR_ID_ETT, USB_DEVICE_ID_TC4UM) },
@@ -1025,6 +1024,10 @@ bool hid_ignore(struct hid_device *hdev)
if (hdev->product == 0x0401 &&
strncmp(hdev->name, "ELAN0800", 8) != 0)
return true;
+ /* Same with product id 0x0400 */
+ if (hdev->product == 0x0400 &&
+ strncmp(hdev->name, "QTEC0001", 8) != 0)
+ return true;
break;
}
--
2.19.1
^ permalink raw reply related
* [PATCH AUTOSEL 4.19 30/52] HID: input: add mapping for Assistant key
From: Sasha Levin @ 2019-04-24 14:38 UTC (permalink / raw)
To: linux-kernel, stable
Cc: Dmitry Torokhov, Jiri Kosina, Sasha Levin, linux-input
In-Reply-To: <20190424143911.28890-1-sashal@kernel.org>
From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
[ Upstream commit ce856634af8cda3490947df8ac1ef5843e6356af ]
According to HUTRR89 usage 0x1cb from the consumer page was assigned to
allow launching desktop-aware assistant application, so let's add the
mapping.
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
drivers/hid/hid-input.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index a3916e58dbf5..e649940e065d 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -982,6 +982,7 @@ static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_fiel
case 0x1b8: map_key_clear(KEY_VIDEO); break;
case 0x1bc: map_key_clear(KEY_MESSENGER); break;
case 0x1bd: map_key_clear(KEY_INFO); break;
+ case 0x1cb: map_key_clear(KEY_ASSISTANT); break;
case 0x201: map_key_clear(KEY_NEW); break;
case 0x202: map_key_clear(KEY_OPEN); break;
case 0x203: map_key_clear(KEY_CLOSE); break;
--
2.19.1
^ permalink raw reply related
* [PATCH AUTOSEL 4.14 01/35] HID: logitech: check the return value of create_singlethread_workqueue
From: Sasha Levin @ 2019-04-24 14:46 UTC (permalink / raw)
To: linux-kernel, stable; +Cc: Kangjie Lu, Jiri Kosina, Sasha Levin, linux-input
From: Kangjie Lu <kjlu@umn.edu>
[ Upstream commit 6c44b15e1c9076d925d5236ddadf1318b0a25ce2 ]
create_singlethread_workqueue may fail and return NULL. The fix checks if it is
NULL to avoid NULL pointer dereference. Also, the fix moves the call of
create_singlethread_workqueue earlier to avoid resource-release issues.
Signed-off-by: Kangjie Lu <kjlu@umn.edu>
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
drivers/hid/hid-logitech-hidpp.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 614054af904a..b83d4173fc7f 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -1907,6 +1907,13 @@ static int hidpp_ff_init(struct hidpp_device *hidpp, u8 feature_index)
kfree(data);
return -ENOMEM;
}
+ data->wq = create_singlethread_workqueue("hidpp-ff-sendqueue");
+ if (!data->wq) {
+ kfree(data->effect_ids);
+ kfree(data);
+ return -ENOMEM;
+ }
+
data->hidpp = hidpp;
data->feature_index = feature_index;
data->version = version;
@@ -1951,7 +1958,6 @@ static int hidpp_ff_init(struct hidpp_device *hidpp, u8 feature_index)
/* ignore boost value at response.fap.params[2] */
/* init the hardware command queue */
- data->wq = create_singlethread_workqueue("hidpp-ff-sendqueue");
atomic_set(&data->workqueue_size, 0);
/* initialize with zero autocenter to get wheel in usable state */
--
2.19.1
^ permalink raw reply related
* [PATCH AUTOSEL 4.14 02/35] HID: debug: fix race condition with between rdesc_show() and device removal
From: Sasha Levin @ 2019-04-24 14:46 UTC (permalink / raw)
To: linux-kernel, stable
Cc: He, Bo, Zhang, Jun, Jiri Kosina, Sasha Levin, linux-input
In-Reply-To: <20190424144709.30215-1-sashal@kernel.org>
From: "He, Bo" <bo.he@intel.com>
[ Upstream commit cef0d4948cb0a02db37ebfdc320e127c77ab1637 ]
There is a race condition that could happen if hid_debug_rdesc_show()
is running while hdev is in the process of going away (device removal,
system suspend, etc) which could result in NULL pointer dereference:
BUG: unable to handle kernel paging request at 0000000783316040
CPU: 1 PID: 1512 Comm: getevent Tainted: G U O 4.19.20-quilt-2e5dc0ac-00029-gc455a447dd55 #1
RIP: 0010:hid_dump_device+0x9b/0x160
Call Trace:
hid_debug_rdesc_show+0x72/0x1d0
seq_read+0xe0/0x410
full_proxy_read+0x5f/0x90
__vfs_read+0x3a/0x170
vfs_read+0xa0/0x150
ksys_read+0x58/0xc0
__x64_sys_read+0x1a/0x20
do_syscall_64+0x55/0x110
entry_SYSCALL_64_after_hwframe+0x49/0xbe
Grab driver_input_lock to make sure the input device exists throughout the
whole process of dumping the rdesc.
[jkosina@suse.cz: update changelog a bit]
Signed-off-by: "he, bo" <bo.he@intel.com>
Signed-off-by: "Zhang, Jun" <jun.zhang@intel.com>
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
drivers/hid/hid-debug.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/hid/hid-debug.c b/drivers/hid/hid-debug.c
index a90967cd4987..a0bcbb633b67 100644
--- a/drivers/hid/hid-debug.c
+++ b/drivers/hid/hid-debug.c
@@ -1060,10 +1060,15 @@ static int hid_debug_rdesc_show(struct seq_file *f, void *p)
seq_printf(f, "\n\n");
/* dump parsed data and input mappings */
+ if (down_interruptible(&hdev->driver_input_lock))
+ return 0;
+
hid_dump_device(hdev, f);
seq_printf(f, "\n");
hid_dump_input_mapping(hdev, f);
+ up(&hdev->driver_input_lock);
+
return 0;
}
--
2.19.1
^ permalink raw reply related
* [PATCH AUTOSEL 4.14 19/35] HID: input: add mapping for Assistant key
From: Sasha Levin @ 2019-04-24 14:46 UTC (permalink / raw)
To: linux-kernel, stable
Cc: Dmitry Torokhov, Jiri Kosina, Sasha Levin, linux-input
In-Reply-To: <20190424144709.30215-1-sashal@kernel.org>
From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
[ Upstream commit ce856634af8cda3490947df8ac1ef5843e6356af ]
According to HUTRR89 usage 0x1cb from the consumer page was assigned to
allow launching desktop-aware assistant application, so let's add the
mapping.
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
drivers/hid/hid-input.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index d146a9b545ee..1aa7d268686b 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -973,6 +973,7 @@ static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_fiel
case 0x1b8: map_key_clear(KEY_VIDEO); break;
case 0x1bc: map_key_clear(KEY_MESSENGER); break;
case 0x1bd: map_key_clear(KEY_INFO); break;
+ case 0x1cb: map_key_clear(KEY_ASSISTANT); break;
case 0x201: map_key_clear(KEY_NEW); break;
case 0x202: map_key_clear(KEY_OPEN); break;
case 0x203: map_key_clear(KEY_CLOSE); break;
--
2.19.1
^ permalink raw reply related
* [PATCH AUTOSEL 4.9 01/28] HID: logitech: check the return value of create_singlethread_workqueue
From: Sasha Levin @ 2019-04-24 14:49 UTC (permalink / raw)
To: linux-kernel, stable; +Cc: Kangjie Lu, Jiri Kosina, Sasha Levin, linux-input
From: Kangjie Lu <kjlu@umn.edu>
[ Upstream commit 6c44b15e1c9076d925d5236ddadf1318b0a25ce2 ]
create_singlethread_workqueue may fail and return NULL. The fix checks if it is
NULL to avoid NULL pointer dereference. Also, the fix moves the call of
create_singlethread_workqueue earlier to avoid resource-release issues.
Signed-off-by: Kangjie Lu <kjlu@umn.edu>
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
drivers/hid/hid-logitech-hidpp.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 2e2515a4c070..3198faf5cff4 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -1282,6 +1282,13 @@ static int hidpp_ff_init(struct hidpp_device *hidpp, u8 feature_index)
kfree(data);
return -ENOMEM;
}
+ data->wq = create_singlethread_workqueue("hidpp-ff-sendqueue");
+ if (!data->wq) {
+ kfree(data->effect_ids);
+ kfree(data);
+ return -ENOMEM;
+ }
+
data->hidpp = hidpp;
data->feature_index = feature_index;
data->version = version;
@@ -1326,7 +1333,6 @@ static int hidpp_ff_init(struct hidpp_device *hidpp, u8 feature_index)
/* ignore boost value at response.fap.params[2] */
/* init the hardware command queue */
- data->wq = create_singlethread_workqueue("hidpp-ff-sendqueue");
atomic_set(&data->workqueue_size, 0);
/* initialize with zero autocenter to get wheel in usable state */
--
2.19.1
^ permalink raw reply related
* [PATCH AUTOSEL 4.9 02/28] HID: debug: fix race condition with between rdesc_show() and device removal
From: Sasha Levin @ 2019-04-24 14:49 UTC (permalink / raw)
To: linux-kernel, stable
Cc: He, Bo, Zhang, Jun, Jiri Kosina, Sasha Levin, linux-input
In-Reply-To: <20190424145012.30886-1-sashal@kernel.org>
From: "He, Bo" <bo.he@intel.com>
[ Upstream commit cef0d4948cb0a02db37ebfdc320e127c77ab1637 ]
There is a race condition that could happen if hid_debug_rdesc_show()
is running while hdev is in the process of going away (device removal,
system suspend, etc) which could result in NULL pointer dereference:
BUG: unable to handle kernel paging request at 0000000783316040
CPU: 1 PID: 1512 Comm: getevent Tainted: G U O 4.19.20-quilt-2e5dc0ac-00029-gc455a447dd55 #1
RIP: 0010:hid_dump_device+0x9b/0x160
Call Trace:
hid_debug_rdesc_show+0x72/0x1d0
seq_read+0xe0/0x410
full_proxy_read+0x5f/0x90
__vfs_read+0x3a/0x170
vfs_read+0xa0/0x150
ksys_read+0x58/0xc0
__x64_sys_read+0x1a/0x20
do_syscall_64+0x55/0x110
entry_SYSCALL_64_after_hwframe+0x49/0xbe
Grab driver_input_lock to make sure the input device exists throughout the
whole process of dumping the rdesc.
[jkosina@suse.cz: update changelog a bit]
Signed-off-by: "he, bo" <bo.he@intel.com>
Signed-off-by: "Zhang, Jun" <jun.zhang@intel.com>
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
drivers/hid/hid-debug.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/hid/hid-debug.c b/drivers/hid/hid-debug.c
index d7179dd3c9ef..3cafa1d28fed 100644
--- a/drivers/hid/hid-debug.c
+++ b/drivers/hid/hid-debug.c
@@ -1058,10 +1058,15 @@ static int hid_debug_rdesc_show(struct seq_file *f, void *p)
seq_printf(f, "\n\n");
/* dump parsed data and input mappings */
+ if (down_interruptible(&hdev->driver_input_lock))
+ return 0;
+
hid_dump_device(hdev, f);
seq_printf(f, "\n");
hid_dump_input_mapping(hdev, f);
+ up(&hdev->driver_input_lock);
+
return 0;
}
--
2.19.1
^ permalink raw reply related
* [PATCH AUTOSEL 4.4 01/15] HID: debug: fix race condition with between rdesc_show() and device removal
From: Sasha Levin @ 2019-04-24 14:51 UTC (permalink / raw)
To: linux-kernel, stable
Cc: He, Bo, Zhang, Jun, Jiri Kosina, Sasha Levin, linux-input
From: "He, Bo" <bo.he@intel.com>
[ Upstream commit cef0d4948cb0a02db37ebfdc320e127c77ab1637 ]
There is a race condition that could happen if hid_debug_rdesc_show()
is running while hdev is in the process of going away (device removal,
system suspend, etc) which could result in NULL pointer dereference:
BUG: unable to handle kernel paging request at 0000000783316040
CPU: 1 PID: 1512 Comm: getevent Tainted: G U O 4.19.20-quilt-2e5dc0ac-00029-gc455a447dd55 #1
RIP: 0010:hid_dump_device+0x9b/0x160
Call Trace:
hid_debug_rdesc_show+0x72/0x1d0
seq_read+0xe0/0x410
full_proxy_read+0x5f/0x90
__vfs_read+0x3a/0x170
vfs_read+0xa0/0x150
ksys_read+0x58/0xc0
__x64_sys_read+0x1a/0x20
do_syscall_64+0x55/0x110
entry_SYSCALL_64_after_hwframe+0x49/0xbe
Grab driver_input_lock to make sure the input device exists throughout the
whole process of dumping the rdesc.
[jkosina@suse.cz: update changelog a bit]
Signed-off-by: "he, bo" <bo.he@intel.com>
Signed-off-by: "Zhang, Jun" <jun.zhang@intel.com>
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
drivers/hid/hid-debug.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/hid/hid-debug.c b/drivers/hid/hid-debug.c
index d7179dd3c9ef..3cafa1d28fed 100644
--- a/drivers/hid/hid-debug.c
+++ b/drivers/hid/hid-debug.c
@@ -1058,10 +1058,15 @@ static int hid_debug_rdesc_show(struct seq_file *f, void *p)
seq_printf(f, "\n\n");
/* dump parsed data and input mappings */
+ if (down_interruptible(&hdev->driver_input_lock))
+ return 0;
+
hid_dump_device(hdev, f);
seq_printf(f, "\n");
hid_dump_input_mapping(hdev, f);
+ up(&hdev->driver_input_lock);
+
return 0;
}
--
2.19.1
^ permalink raw reply related
* [PATCH AUTOSEL 3.18 01/10] HID: debug: fix race condition with between rdesc_show() and device removal
From: Sasha Levin @ 2019-04-24 14:52 UTC (permalink / raw)
To: linux-kernel, stable
Cc: He, Bo, Zhang, Jun, Jiri Kosina, Sasha Levin, linux-input
From: "He, Bo" <bo.he@intel.com>
[ Upstream commit cef0d4948cb0a02db37ebfdc320e127c77ab1637 ]
There is a race condition that could happen if hid_debug_rdesc_show()
is running while hdev is in the process of going away (device removal,
system suspend, etc) which could result in NULL pointer dereference:
BUG: unable to handle kernel paging request at 0000000783316040
CPU: 1 PID: 1512 Comm: getevent Tainted: G U O 4.19.20-quilt-2e5dc0ac-00029-gc455a447dd55 #1
RIP: 0010:hid_dump_device+0x9b/0x160
Call Trace:
hid_debug_rdesc_show+0x72/0x1d0
seq_read+0xe0/0x410
full_proxy_read+0x5f/0x90
__vfs_read+0x3a/0x170
vfs_read+0xa0/0x150
ksys_read+0x58/0xc0
__x64_sys_read+0x1a/0x20
do_syscall_64+0x55/0x110
entry_SYSCALL_64_after_hwframe+0x49/0xbe
Grab driver_input_lock to make sure the input device exists throughout the
whole process of dumping the rdesc.
[jkosina@suse.cz: update changelog a bit]
Signed-off-by: "he, bo" <bo.he@intel.com>
Signed-off-by: "Zhang, Jun" <jun.zhang@intel.com>
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
drivers/hid/hid-debug.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/hid/hid-debug.c b/drivers/hid/hid-debug.c
index e930627d0c76..71b069bd2a24 100644
--- a/drivers/hid/hid-debug.c
+++ b/drivers/hid/hid-debug.c
@@ -1057,10 +1057,15 @@ static int hid_debug_rdesc_show(struct seq_file *f, void *p)
seq_printf(f, "\n\n");
/* dump parsed data and input mappings */
+ if (down_interruptible(&hdev->driver_input_lock))
+ return 0;
+
hid_dump_device(hdev, f);
seq_printf(f, "\n");
hid_dump_input_mapping(hdev, f);
+ up(&hdev->driver_input_lock);
+
return 0;
}
--
2.19.1
^ permalink raw reply related
* Re: [PATCH v3 7/8] HID: logitech: Enable high-resolution scrolling on Logitech mice
From: Clément VUCHENER @ 2019-04-24 15:34 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Harry Cutts, Peter Hutterer, open list:HID CORE LAYER,
Dmitry Torokhov, Jiri Kosina, Linus Torvalds, Nestor Lopez Casado,
lkml
In-Reply-To: <CAO-hwJK7pFvuYZJTKgwkj794=xinekDgN_eLBSViw3v_dWAecw@mail.gmail.com>
Hi Benjamin,
I tried again to add hi-res wheel support for the G500 with Hans de
Goede's latest patch series you've just merged in for-5.2/logitech, it
is much better but there is still some issues.
The first one is the device index, I need to use device index 0
instead 0xff. I added a quick and dirty quirk (stealing in the
QUIRK_CLASS range since the normal quirk range looks full) to change
the device index assigned in __hidpp_send_report. After that the
device is correctly initialized and the wheel multiplier is set.
The second issue is that wheel values are not actually scaled
according to the multiplier. I get 7/8 full scroll event for each
wheel step. I think it happens because the mouse is split in two
devices. The first device has the wheel events, and the second device
has the HID++ reports. The wheel multiplier is only set on the second
device (where the hi-res mode is enabled) and does not affect the
wheel events from the first one.
Le mer. 19 déc. 2018 à 21:35, Benjamin Tissoires
<benjamin.tissoires@redhat.com> a écrit :
>
> On Wed, Dec 19, 2018 at 11:57 AM Clément VUCHENER
> <clement.vuchener@gmail.com> wrote:
> >
> > Le sam. 15 déc. 2018 à 15:45, Clément VUCHENER
> > <clement.vuchener@gmail.com> a écrit :
> > >
> > > Le ven. 14 déc. 2018 à 19:37, Harry Cutts <hcutts@chromium.org> a écrit :
> > > >
> > > > Hi Clement,
> > > >
> > > > On Fri, 14 Dec 2018 at 05:47, Clément VUCHENER
> > > > <clement.vuchener@gmail.com> wrote:
> > > > > Hi, The G500s (and the G500 too, I think) does support the "scrolling
> > > > > acceleration" bit. If I set it, I get around 8 events for each wheel
> > > > > "click", this is what this driver expects, right? If I understood
> > > > > correctly, I should try this patch with the
> > > > > HIDPP_QUIRK_HI_RES_SCROLL_1P0 quirk set for my mouse.
> > > >
> > > > Thanks for the info! Yes, that should work.
> > >
> > > Well, it is not that simple. I get "Device not connected" errors for
> > > both interfaces of the mouse.
> >
> > I suspect the device is not responding because the hid device is not
> > started. When is hid_hw_start supposed to be called? It is called
> > early for HID_QUIRK_CLASS_G920 but later for other device. So the
> > device is not started when hidpp_is_connected is called. Is this
> > because most of the device in this driver are not real HID devices but
> > DJ devices? How should non-DJ devices be treated?
>
> Hi Clement,
>
> I have a series I sent last September that allows to support non DJ
> devices on logitech-hidpp
> (https://patchwork.kernel.org/project/linux-input/list/?series=16359).
>
> In its current form, with the latest upstream kernel, the series will
> oops during the .event() callback, which is easy enough to fix.
> However, I am currently trying to make it better as a second or third
> reading made me realized that there was a bunch of non-sense in it and
> a proper support would require slightly more work for the non unifying
> receiver case.
>
> I hope I'll be able to send out something by the end of the week.
>
> Cheers,
> Benjamin
^ permalink raw reply
* Re: [PATCH 1/2] HID: input: make sure the wheel high resolution multiplier is set
From: James Feeney @ 2019-04-24 15:42 UTC (permalink / raw)
To: Benjamin Tissoires, Jiri Kosina, Peter Hutterer
Cc: open list:HID CORE LAYER, lkml, 3.8+
In-Reply-To: <CAO-hwJLCL95pAzO9kco2jo2_uCV2=3f5OEf=P=AoB9EpEjFTAw@mail.gmail.com>
Hey Benjamin
On 4/24/19 7:30 AM, Benjamin Tissoires wrote:
> Given that this basically breaks a basic functionality of many
> Microsoft mice, I went ahead and applied this series to
> for-5.1/upstream-fixes
Is there some reason that both patches should not be applied immediately, to the 5.0 series?
Or - likely I am uninformed - are the 5.1 patches applied as a set separate from the 5.0 series revisions?
Thanks
James
^ permalink raw reply
* Re: [PATCH 1/2] HID: input: make sure the wheel high resolution multiplier is set
From: Benjamin Tissoires @ 2019-04-24 16:41 UTC (permalink / raw)
To: James Feeney
Cc: Jiri Kosina, Peter Hutterer, open list:HID CORE LAYER, lkml, 3.8+
In-Reply-To: <43a56e9b-6e44-76b7-efff-fa8996183fbc@nurealm.net>
On Wed, Apr 24, 2019 at 5:42 PM James Feeney <james@nurealm.net> wrote:
>
> Hey Benjamin
>
> On 4/24/19 7:30 AM, Benjamin Tissoires wrote:
> > Given that this basically breaks a basic functionality of many
> > Microsoft mice, I went ahead and applied this series to
> > for-5.1/upstream-fixes
>
> Is there some reason that both patches should not be applied immediately, to the 5.0 series?
>
> Or - likely I am uninformed - are the 5.1 patches applied as a set separate from the 5.0 series revisions?
For a patch to be picked up by stable, it first needs to go in Linus'
tree. Currently we are working on 5.1, so any stable patches need to
go in 5.1 first. Then, once they hit Linus' tree, the stable team will
pick them and backport them in the appropriate stable tree.
But distributions can backport them as they wish, so you can make a
request to your distribution to include them ASAP. They are officially
upstream, though yet to be sent to Linus.
Cheers,
Benjamin
^ permalink raw reply
* Re: [PATCH 1/3] mfd: apple-ibridge: Add Apple iBridge MFD driver.
From: Jonathan Cameron @ 2019-04-24 19:13 UTC (permalink / raw)
To: Life is hard, and then you die
Cc: Jiri Kosina, Benjamin Tissoires, Hartmut Knaack,
Lars-Peter Clausen, Peter Meerwald-Stadler, Lee Jones,
linux-input, linux-iio, linux-kernel
In-Reply-To: <20190424104718.GA31301@innovation.ch>
On Wed, 24 Apr 2019 03:47:18 -0700
"Life is hard, and then you die" <ronald@innovation.ch> wrote:
> Hi Jonathan,
>
> On Mon, Apr 22, 2019 at 12:34:26PM +0100, Jonathan Cameron wrote:
> > On Sun, 21 Apr 2019 20:12:49 -0700
> > Ronald Tschalär <ronald@innovation.ch> wrote:
> >
> > > The iBridge device provides access to several devices, including:
> > > - the Touch Bar
> > > - the iSight webcam
> > > - the light sensor
> > > - the fingerprint sensor
> > >
> > > This driver provides the core support for managing the iBridge device
> > > and the access to the underlying devices. In particular, since the
> > > functionality for the touch bar and light sensor is exposed via USB HID
> > > interfaces, and the same HID device is used for multiple functions, this
> > > driver provides a multiplexing layer that allows multiple HID drivers to
> > > be registered for a given HID device. This allows the touch bar and ALS
> > > driver to be separated out into their own modules.
> > >
> > > Signed-off-by: Ronald Tschalär <ronald@innovation.ch
> > Hi Ronald,
> >
> > I've only taken a fairly superficial look at this. A few global
> > things to note though.
>
> Thanks for this review.
>
> > 1. Please either use kernel-doc style for function descriptions, or
> > do not. Right now you are sort of half way there.
>
> Apologies, on re-reading the docs I realize what you mean here. Should
> be fixed now (next rev).
>
> > 2. There is quite a complex nest of separate structures being allocated,
> > so think about whether they can be simplified. In particular
> > use of container_of macros can allow a lot of forwards and backwards
> > pointers to be dropped if you embed the various structures directly.
>
> Done (see also below).
>
> [snip]
> > > +#define call_void_driver_func(drv_info, fn, ...) \
> >
> > This sort of macro may seem like a good idea because it saves a few lines
> > of code. However, that comes at the cost of readability, so just
> > put the code inline.
> >
> > > + do { \
> > > + if ((drv_info)->driver->fn) \
> > > + (drv_info)->driver->fn(__VA_ARGS__); \
> > > + } while (0)
> > > +
> > > +#define call_driver_func(drv_info, fn, ret_type, ...) \
> > > + ({ \
> > > + ret_type rc = 0; \
> > > + \
> > > + if ((drv_info)->driver->fn) \
> > > + rc = (drv_info)->driver->fn(__VA_ARGS__); \
> > > + \
> > > + rc; \
> > > + })
>
> Just to clarify, you're only talking about removing/inlining the
> call_void_driver_func() macro, not the call_driver_func() macro,
> right?
Both please. Neither adds much.
>
> [snip]
> > > +static struct appleib_hid_dev_info *
> > > +appleib_add_device(struct appleib_device *ib_dev, struct hid_device *hdev,
> > > + const struct hid_device_id *id)
> > > +{
> > > + struct appleib_hid_dev_info *dev_info;
> > > + struct appleib_hid_drv_info *drv_info;
> > > +
> > > + /* allocate device-info for this device */
> > > + dev_info = kzalloc(sizeof(*dev_info), GFP_KERNEL);
> > > + if (!dev_info)
> > > + return NULL;
> > > +
> > > + INIT_LIST_HEAD(&dev_info->drivers);
> > > + dev_info->device = hdev;
> > > + dev_info->device_id = id;
> > > +
> > > + /* notify all our sub drivers */
> > > + mutex_lock(&ib_dev->update_lock);
> > > +
> > This is interesting. I'd like to see a comment here on what
> > this flag is going to do.
>
> I'm not sure I follow: update_lock is simply a mutex protecting all
> driver and device update (i.e. add/remove) functions. Are you
> therefore looking for something like:
That ended up in the wrong place...
It was the in_hid_probe just after here that I was referring to.
>
> /* protect driver and device lists against concurrent updates */
> mutex_lock(&ib_dev->update_lock);
>
> [snip]
> > > +static int appleib_probe(struct acpi_device *acpi)
> > > +{
> > > + struct appleib_device *ib_dev;
> > > + struct appleib_platform_data *pdata;
> > Platform_data has a lot of historical meaning in Linux.
> > Also you have things in here that are not platform related
> > at all, such as the dev pointer. Hence I would rename it
> > as device_data or private or something like that.
>
> Ok. I guess I called in platform_data because it's stored in the mfd
> cell's "platform_data" field. Anyway, changed it per your suggestion.
>
> > > + int i;
> > > + int ret;
> > > +
> > > + if (appleib_dev)
> > This singleton bothers me a bit. I'm really not sure why it
> > is necessary. You can just put a pointer to this in
> > the pdata for the subdevs and I think that covers most of your
> > usecases. It's generally a bad idea to limit things to one instance
> > of a device unless that actually major simplifications.
> > I'm not seeing them here.
>
> Yes, this one is quite ugly. appleib_dev is static so that
> appleib_hid_probe() can find it. I could not find any other way to
> pass the appleib_dev instance to that probe function.
>
> However, on looking at this again, I realized that hid_device_id has
> a driver_data field which can be used for this; so if I added the
> hid_driver and hid_device_id structs to the appleib_device (instead of
> making them static like now) I could fill in the driver_data and avoid
> this hack. This looks much cleaner.
>
> Thanks for pointing this uglyness out again.
>
> [snip]
> > > + if (!ib_dev->subdevs) {
> > > + ret = -ENOMEM;
> > > + goto free_dev;
> > > + }
> > > +
> > > + pdata = kzalloc(sizeof(*pdata), GFP_KERNEL);
> >
> > Might as well embed this in ib_dev as well.
>
> Agreed.
>
> > That would let
> > you used container_of to avoid having to carry the ib_dev pointer
> > around in side pdata.
>
> I see. I guess my main reservation is that the functions exported to
> the sub-drivers would now take a 'struct appleib_device_data *'
> argument instead of a 'struct appleib_device *', which just seems a
> bit unnatural. E.g.
>
> int appleib_register_hid_driver(struct appleib_device_data *ib_ddata,
> struct hid_driver *driver, void *data);
>
> instead of (the current)
>
> int appleib_register_hid_driver(struct appleib_device *ib_dev,
> struct hid_driver *driver, void *data)
I'm not totally sure I can see why. You can go from backwards and forwards
from any of the pointers...
>
> [snip]
> > > + ret = mfd_add_devices(&acpi->dev, PLATFORM_DEVID_NONE,
> > > + ib_dev->subdevs, ARRAY_SIZE(appleib_subdevs),
> > > + NULL, 0, NULL);
> > > + if (ret) {
> > > + dev_err(LOG_DEV(ib_dev), "Error adding MFD devices: %d\n", ret);
> > > + goto free_pdata;
> > > + }
> > > +
> > > + acpi->driver_data = ib_dev;
> > > + appleib_dev = ib_dev;
> > > +
> > > + ret = hid_register_driver(&appleib_hid_driver);
> > > + if (ret) {
> > > + dev_err(LOG_DEV(ib_dev), "Error registering hid driver: %d\n",
> > > + ret);
> > > + goto rem_mfd_devs;
> > > + }
> > > +
> > > + return 0;
> > > +
> > > +rem_mfd_devs:
> > > + mfd_remove_devices(&acpi->dev);
> > > +free_pdata:
> > > + kfree(pdata);
> > > +free_subdevs:
> > > + kfree(ib_dev->subdevs);
> > > +free_dev:
> > > + appleib_dev = NULL;
> > > + acpi->driver_data = NULL;
> > Why at this point? It's not set to anything until much later in the
> > probe flow.
>
> If the hid_register_driver() call fails, we get here after driver_data
> has been assigned. However, looking at this again, acpi->driver_data
> is only used by the remove, suspend, and resume callbacks, and those
> will not be called until a successful return from probe; therefore I
> can safely move the setting of driver_data to after the
> hid_register_driver() call and avoid having to set it to NULL in the
> error cleanup.
>
> > May be worth thinking about devm_ managed allocations
> > to cleanup some of these allocations automatically and simplify
> > the error handling.
>
> Good point, thanks.
>
> [snip]
> > > +
> > > + rc = acpi_execute_simple_method(ib_dev->asoc_socw, NULL, 0);
> > > + if (ACPI_FAILURE(rc))
> > > + dev_warn(LOG_DEV(ib_dev), "SOCW(0) failed: %s\n",
> >
> > I can sort of see you might want to do the LOG_DEV for consistency
> > but here I'm fairly sure it's just dev which might be clearer.
>
> Sorry, you mean rename the macro LOG_DEV() to just DEV()?
No, just dev_warn(dev,....)
It's the same one I think at this particular location.
>
>
> Cheers,
>
> Ronald
>
^ permalink raw reply
* [PATCH 1/2] HID: wacom: Don't set tool type until we're in range
From: Gerecke, Jason @ 2019-04-24 22:12 UTC (permalink / raw)
To: linux-input, Benjamin Tissoires
Cc: Ping Cheng, Aaron Armstrong Skomra, Jason Gerecke, stable,
Aaron Armstrong Skomra
From: Jason Gerecke <jason.gerecke@wacom.com>
The serial number and tool type information that is reported by the tablet
while a pen is merely "in prox" instead of fully "in range" can be stale
and cause us to report incorrect tool information. Serial number, tool
type, and other information is only valid once the pen comes fully in range
so we should be careful to not use this information until that point.
In particular, this issue may cause the driver to incorectly report
BTN_TOOL_RUBBER after switching from the eraser tool back to the pen.
Fixes: a48324de6d ("HID: wacom: Bluetooth IRQ for Intuos Pro should handle prox/range")
Cc: <stable@vger.kernel.org> # 4.11+
Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com>
Reviewed-by: Aaron Armstrong Skomra <aaron.skomra@wacom.com>
---
drivers/hid/wacom_wac.c | 17 ++++++++++++++++-
1 file changed, 16 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
index 747730d32ab6..4c1bc239207e 100644
--- a/drivers/hid/wacom_wac.c
+++ b/drivers/hid/wacom_wac.c
@@ -1236,13 +1236,13 @@ static void wacom_intuos_pro2_bt_pen(struct wacom_wac *wacom)
/* Add back in missing bits of ID for non-USI pens */
wacom->id[0] |= (wacom->serial[0] >> 32) & 0xFFFFF;
}
- wacom->tool[0] = wacom_intuos_get_tool_type(wacom_intuos_id_mangle(wacom->id[0]));
for (i = 0; i < pen_frames; i++) {
unsigned char *frame = &data[i*pen_frame_len + 1];
bool valid = frame[0] & 0x80;
bool prox = frame[0] & 0x40;
bool range = frame[0] & 0x20;
+ bool invert = frame[0] & 0x10;
if (!valid)
continue;
@@ -1251,9 +1251,24 @@ static void wacom_intuos_pro2_bt_pen(struct wacom_wac *wacom)
wacom->shared->stylus_in_proximity = false;
wacom_exit_report(wacom);
input_sync(pen_input);
+
+ wacom->tool[0] = 0;
+ wacom->id[0] = 0;
+ wacom->serial[0] = 0;
return;
}
+
if (range) {
+ if (!wacom->tool[0]) { /* first in range */
+ /* Going into range select tool */
+ if (invert)
+ wacom->tool[0] = BTN_TOOL_RUBBER;
+ else if (wacom->id[0])
+ wacom->tool[0] = wacom_intuos_get_tool_type(wacom->id[0]);
+ else
+ wacom->tool[0] = BTN_TOOL_PEN;
+ }
+
input_report_abs(pen_input, ABS_X, get_unaligned_le16(&frame[1]));
input_report_abs(pen_input, ABS_Y, get_unaligned_le16(&frame[3]));
--
2.21.0
^ permalink raw reply related
* [PATCH 2/2] HID: wacom: Don't report anything prior to the tool entering range
From: Gerecke, Jason @ 2019-04-24 22:12 UTC (permalink / raw)
To: linux-input, Benjamin Tissoires
Cc: Ping Cheng, Aaron Armstrong Skomra, Jason Gerecke, stable,
Aaron Armstrong Skomra
In-Reply-To: <20190424221258.19992-1-jason.gerecke@wacom.com>
From: Jason Gerecke <jason.gerecke@wacom.com>
If the tool spends some time in prox before entering range, a series of
events (e.g. ABS_DISTANCE, MSC_SERIAL) can be sent before we or userspace
have any clue about the pen whose data is being reported. We need to hold
off on reporting anything until the pen has entered range. Since we still
want to report events that occur "in prox" after the pen has *left* range
we use 'wacom-tool[0]' as the indicator that the pen did at one point
enter range and provide us/userspace with tool type and serial number
information.
Fixes: a48324de6d ("HID: wacom: Bluetooth IRQ for Intuos Pro should handle prox/range")
Cc: <stable@vger.kernel.org> # 4.11+
Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com>
Reviewed-by: Aaron Armstrong Skomra <aaron.skomra@wacom.com>
---
drivers/hid/wacom_wac.c | 33 ++++++++++++++++++---------------
1 file changed, 18 insertions(+), 15 deletions(-)
diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
index 4c1bc239207e..613342bb9d6b 100644
--- a/drivers/hid/wacom_wac.c
+++ b/drivers/hid/wacom_wac.c
@@ -1290,23 +1290,26 @@ static void wacom_intuos_pro2_bt_pen(struct wacom_wac *wacom)
get_unaligned_le16(&frame[11]));
}
}
- input_report_abs(pen_input, ABS_PRESSURE, get_unaligned_le16(&frame[5]));
- if (wacom->features.type == INTUOSP2_BT) {
- input_report_abs(pen_input, ABS_DISTANCE,
- range ? frame[13] : wacom->features.distance_max);
- } else {
- input_report_abs(pen_input, ABS_DISTANCE,
- range ? frame[7] : wacom->features.distance_max);
- }
- input_report_key(pen_input, BTN_TOUCH, frame[0] & 0x01);
- input_report_key(pen_input, BTN_STYLUS, frame[0] & 0x02);
- input_report_key(pen_input, BTN_STYLUS2, frame[0] & 0x04);
+ if (wacom->tool[0]) {
+ input_report_abs(pen_input, ABS_PRESSURE, get_unaligned_le16(&frame[5]));
+ if (wacom->features.type == INTUOSP2_BT) {
+ input_report_abs(pen_input, ABS_DISTANCE,
+ range ? frame[13] : wacom->features.distance_max);
+ } else {
+ input_report_abs(pen_input, ABS_DISTANCE,
+ range ? frame[7] : wacom->features.distance_max);
+ }
+
+ input_report_key(pen_input, BTN_TOUCH, frame[0] & 0x01);
+ input_report_key(pen_input, BTN_STYLUS, frame[0] & 0x02);
+ input_report_key(pen_input, BTN_STYLUS2, frame[0] & 0x04);
- input_report_key(pen_input, wacom->tool[0], prox);
- input_event(pen_input, EV_MSC, MSC_SERIAL, wacom->serial[0]);
- input_report_abs(pen_input, ABS_MISC,
- wacom_intuos_id_mangle(wacom->id[0])); /* report tool id */
+ input_report_key(pen_input, wacom->tool[0], prox);
+ input_event(pen_input, EV_MSC, MSC_SERIAL, wacom->serial[0]);
+ input_report_abs(pen_input, ABS_MISC,
+ wacom_intuos_id_mangle(wacom->id[0])); /* report tool id */
+ }
wacom->shared->stylus_in_proximity = prox;
--
2.21.0
^ permalink raw reply related
* Re: [PATCH 3/5] input: edt-ft5x06 - Call devm_of_device_links_add() to create links
From: Dmitry Torokhov @ 2019-04-24 22:52 UTC (permalink / raw)
To: Benjamin Gaignard
Cc: rafael.j.wysocki, robh+dt, mark.rutland, hadess, frowand.list,
m.felsch, agx, yannick.fertre, arnd, linux-input, devicetree,
linux-kernel, linux-stm32, broonie
In-Reply-To: <20190424101913.1534-4-benjamin.gaignard@st.com>
Hi Benjamin,
On Wed, Apr 24, 2019 at 12:19:11PM +0200, Benjamin Gaignard wrote:
> From: Yannick Fertré <yannick.fertre@st.com>
>
> Add a call to devm_of_device_links_add() to create links with suppliers
> at probe time.
>
> Signed-off-by: Yannick Fertré <yannick.fertre@st.com>
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>
> ---
> drivers/input/touchscreen/edt-ft5x06.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c
> index 702bfda7ee77..ac9f7e85efb0 100644
> --- a/drivers/input/touchscreen/edt-ft5x06.c
> +++ b/drivers/input/touchscreen/edt-ft5x06.c
> @@ -1167,6 +1167,8 @@ static int edt_ft5x06_ts_probe(struct i2c_client *client,
>
> i2c_set_clientdata(client, tsdata);
>
> + devm_of_device_links_add(&client->dev);
> +
This seems pretty generic action and I believe it should be done in
generic code, either bus (i2c, spi, etc), or in device core.
Thanks.
--
Dmitry
^ 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