* Re: [PATCH 1/3] mfd: apple-ibridge: Add Apple iBridge MFD driver.
From: Life is hard, and then you die @ 2019-04-25 8:19 UTC (permalink / raw)
To: Benjamin Tissoires
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: <CAO-hwJLMZvBGheEtH_iU+mBtE4T3OsT6W+=1x-ewzGGP5Fxp4Q@mail.gmail.com>
Hi Benjamin,
Thank you for looking at this.
On Wed, Apr 24, 2019 at 04:18:23PM +0200, Benjamin Tissoires wrote:
> 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.
Basically correct. To be a bit more precise, there are currently two
hid-devices and two drivers (touchbar and als) involved, with
connections as follows (pardon the ugly ascii art):
hdev1 --- tb-drv
/
/
/
hdev2 --- als-drv
i.e. the touchbar driver talks to both hdev's, and hdev2's events
(reports) are processed by both drivers (though each handles different
reports).
> 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?
Yes, I think I understand, and I think this can work. Basically,
instead of demux'ing at the hid-driver level as I am doing now (i.e.
the iBridge hid-driver forwarding calls to the sub-hid-drivers), we
demux at the hid-device level (events forwarded from iBridge hdev to
all "virtual" sub-hdev's, and requests from sub-hdev's forwarded to
the original hdev via an iBridge ll_driver attached to the
sub-hdev's).
So I would need to create 3 new "virtual" hid-devices (instances) as
follows:
hdev1 --- vhdev1 --- tb-drv
/
-- vhdev2 --
/
hdev2 --- vhdev3 --- als-drv
(vhdev1 is probably not strictly necessary, but makes things more
consistent).
> 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.
Agreed: let's first get the overall strategy stabilized (also so I
can avoid having to rewrite the code too many more times ;-) ).
[snip]
> > +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.
Wasn't aware of this change - thanks. Yes, with that the warning
message should be gone and this fixup can be avoided.
One thing I find strange, though, is that while 94a9992f7dbd changes
the condition at which the warning is emitted, it still truncates the
value to 32 bits, albeit completely silently now for lengths between
32 and 256 bits. I.e. I'm somewhat surprised that hid_field_extract()
(and __extract() ) weren't updated to actually return the full values
for longer fields. Either that, or the callers of hid_field_extract()
changed to read longer fields in 32 bit chunks.
Cheers,
Ronald
^ permalink raw reply
* Re: [PATCH] HID: logitech-dj: Fix build error without CONFIG_USB_HID
From: Benjamin Tissoires @ 2019-04-25 7:42 UTC (permalink / raw)
To: Yue Haibing; +Cc: Jiri Kosina, Hans de Goede, lkml, open list:HID CORE LAYER
In-Reply-To: <20190425073942.36980-1-yuehaibing@huawei.com>
Hi,
On Thu, Apr 25, 2019 at 9:40 AM Yue Haibing <yuehaibing@huawei.com> wrote:
>
> From: YueHaibing <yuehaibing@huawei.com>
>
> During randconfig builds, I occasionally run into an invalid configuration
>
> drivers/hid/hid-logitech-dj.o: In function `logi_dj_probe':
> hid-logitech-dj.c:(.text+0x32dc): undefined reference to `usb_hid_driver'
>
> This is because CONFIG_USB_HID is not set, So this
> patch selects it.
>
> Reported-by: Hulk Robot <hulkci@huawei.com>
> Fixes: da12b224b7d5 ("HID: logitech-dj: deal with some KVMs adding an extra interface to the usbdev")
> Signed-off-by: YueHaibing <yuehaibing@huawei.com>
Thanks a lot, but I already scheduled
https://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git/commit/?h=for-5.2/logitech&id=c08f38e9fd0b5486957ed42438ec8fa9b6ebbf4f
in for-next.
It's weird it doesn't show up in your bot... :/
Cheers,
Benjamin
> ---
> drivers/hid/Kconfig | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> index 76d8206..c3c390c 100644
> --- a/drivers/hid/Kconfig
> +++ b/drivers/hid/Kconfig
> @@ -521,6 +521,7 @@ config HID_LOGITECH
>
> config HID_LOGITECH_DJ
> tristate "Logitech Unifying receivers full support"
> + depends on USB_HID
> depends on HIDRAW
> depends on HID_LOGITECH
> select HID_LOGITECH_HIDPP
> --
> 2.7.4
>
>
^ permalink raw reply
* Re: [PATCH v3 7/8] HID: logitech: Enable high-resolution scrolling on Logitech mice
From: Benjamin Tissoires @ 2019-04-25 7:40 UTC (permalink / raw)
To: Clément VUCHENER
Cc: Harry Cutts, Peter Hutterer, open list:HID CORE LAYER,
Dmitry Torokhov, Jiri Kosina, Linus Torvalds, Nestor Lopez Casado,
lkml
In-Reply-To: <CAM4jgCrYBr+M+F2iAn8-pPFjiZQ-nPT_H16WcyPKp2PFCM=qhQ@mail.gmail.com>
Hi Clément,
On Wed, Apr 24, 2019 at 5:34 PM Clément VUCHENER
<clement.vuchener@gmail.com> wrote:
>
> 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.
Hmm, maybe we should restrain a little bit the reserved quirks...
But actually, .driver_data and .quirks are both unsigned long, so you
should be able to use the 64 bits.
>
> 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.
I would think this have to do with the device not accepting the
command instead. Can you share some raw logs of the events (ideally
with hid-recorder from
https://gitlab.freedesktop.org/libevdev/hid-tools)?
Cheers,
Benjamin
>
> 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
* [PATCH] HID: logitech-dj: Fix build error without CONFIG_USB_HID
From: Yue Haibing @ 2019-04-25 7:39 UTC (permalink / raw)
To: jikos, benjamin.tissoires, hdegoede; +Cc: linux-kernel, linux-input, YueHaibing
From: YueHaibing <yuehaibing@huawei.com>
During randconfig builds, I occasionally run into an invalid configuration
drivers/hid/hid-logitech-dj.o: In function `logi_dj_probe':
hid-logitech-dj.c:(.text+0x32dc): undefined reference to `usb_hid_driver'
This is because CONFIG_USB_HID is not set, So this
patch selects it.
Reported-by: Hulk Robot <hulkci@huawei.com>
Fixes: da12b224b7d5 ("HID: logitech-dj: deal with some KVMs adding an extra interface to the usbdev")
Signed-off-by: YueHaibing <yuehaibing@huawei.com>
---
drivers/hid/Kconfig | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index 76d8206..c3c390c 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -521,6 +521,7 @@ config HID_LOGITECH
config HID_LOGITECH_DJ
tristate "Logitech Unifying receivers full support"
+ depends on USB_HID
depends on HIDRAW
depends on HID_LOGITECH
select HID_LOGITECH_HIDPP
--
2.7.4
^ permalink raw reply related
* Re: [PATCH 3/5] input: edt-ft5x06 - Call devm_of_device_links_add() to create links
From: Benjamin GAIGNARD @ 2019-04-25 7:22 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: rafael.j.wysocki@intel.com, robh+dt@kernel.org,
mark.rutland@arm.com, hadess@hadess.net, frowand.list@gmail.com,
m.felsch@pengutronix.de, agx@sigxcpu.org, Yannick FERTRE,
arnd@arndb.de, linux-input@vger.kernel.org,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-stm32@st-md-mailman.stormreply.com, broonie@kernel.org
In-Reply-To: <20190424225235.GA225055@dtor-ws>
On 4/25/19 12:52 AM, Dmitry Torokhov wrote:
> 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.
It is done (or could be done) in most of the buses or framework (like
for regulator)
but I think that adding it in device core add a device-tree node parsing
for all the
drivers while only a few of them will really need this feature. That why
I have put the
call here.
Benjamin
>
> Thanks.
>
^ permalink raw reply
* RE: [RESEND] input: keyboard: imx: make sure keyboard can always wake up system
From: Anson Huang @ 2019-04-25 1:49 UTC (permalink / raw)
To: dmitry.torokhov@gmail.com, shawnguo@kernel.org,
s.hauer@pengutronix.de, kernel@pengutronix.de, festevam@gmail.com,
linux-input@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org
Cc: dl-linux-imx
In-Reply-To: <1554341727-16084-1-git-send-email-Anson.Huang@nxp.com>
Gentle ping...
> -----Original Message-----
> From: Anson Huang
> Sent: Thursday, April 4, 2019 9:40 AM
> To: dmitry.torokhov@gmail.com; shawnguo@kernel.org;
> s.hauer@pengutronix.de; kernel@pengutronix.de; festevam@gmail.com;
> linux-input@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
> kernel@vger.kernel.org
> Cc: dl-linux-imx <linux-imx@nxp.com>
> Subject: [RESEND] input: keyboard: imx: make sure keyboard can always
> wake up system
>
> There are several scenarios that keyboard can NOT wake up system from
> suspend, e.g., if a keyboard is depressed between system device suspend
> phase and device noirq suspend phase, the keyboard ISR will be called and
> both keyboard depress and release interrupts will be disabled, then
> keyboard will no longer be able to wake up system. Another scenario would
> be, if a keyboard is kept depressed, and then system goes into suspend, the
> expected behavior would be when keyboard is released, system will be
> waked up, but current implementation can NOT achieve that, because both
> depress and release interrupts are disabled in ISR, and the event check is still
> in progress.
>
> To fix these issues, need to make sure keyboard's depress or release
> interrupt is enabled after noirq device suspend phase, this patch moves the
> suspend/resume callback to noirq suspend/resume phase, and enable the
> corresponding interrupt according to current keyboard status.
>
> Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> ---
> drivers/input/keyboard/imx_keypad.c | 18 ++++++++++++++----
> 1 file changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/input/keyboard/imx_keypad.c
> b/drivers/input/keyboard/imx_keypad.c
> index cf08f4a..97500a2 100644
> --- a/drivers/input/keyboard/imx_keypad.c
> +++ b/drivers/input/keyboard/imx_keypad.c
> @@ -524,11 +524,12 @@ static int imx_keypad_probe(struct
> platform_device *pdev)
> return 0;
> }
>
> -static int __maybe_unused imx_kbd_suspend(struct device *dev)
> +static int __maybe_unused imx_kbd_noirq_suspend(struct device *dev)
> {
> struct platform_device *pdev = to_platform_device(dev);
> struct imx_keypad *kbd = platform_get_drvdata(pdev);
> struct input_dev *input_dev = kbd->input_dev;
> + unsigned short reg_val = readw(kbd->mmio_base + KPSR);
>
> /* imx kbd can wake up system even clock is disabled */
> mutex_lock(&input_dev->mutex);
> @@ -538,13 +539,20 @@ static int __maybe_unused
> imx_kbd_suspend(struct device *dev)
>
> mutex_unlock(&input_dev->mutex);
>
> - if (device_may_wakeup(&pdev->dev))
> + if (device_may_wakeup(&pdev->dev)) {
> + if (reg_val & KBD_STAT_KPKD)
> + reg_val |= KBD_STAT_KRIE;
> + if (reg_val & KBD_STAT_KPKR)
> + reg_val |= KBD_STAT_KDIE;
> + writew(reg_val, kbd->mmio_base + KPSR);
> +
> enable_irq_wake(kbd->irq);
> + }
>
> return 0;
> }
>
> -static int __maybe_unused imx_kbd_resume(struct device *dev)
> +static int __maybe_unused imx_kbd_noirq_resume(struct device *dev)
> {
> struct platform_device *pdev = to_platform_device(dev);
> struct imx_keypad *kbd = platform_get_drvdata(pdev); @@ -568,7
> +576,9 @@ static int __maybe_unused imx_kbd_resume(struct device *dev)
> return ret;
> }
>
> -static SIMPLE_DEV_PM_OPS(imx_kbd_pm_ops, imx_kbd_suspend,
> imx_kbd_resume);
> +static const struct dev_pm_ops imx_kbd_pm_ops = {
> + SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(imx_kbd_noirq_suspend,
> +imx_kbd_noirq_resume) };
>
> static struct platform_driver imx_keypad_driver = {
> .driver = {
> --
> 2.7.4
^ permalink raw reply
* Re: [PATCH v2] Input: uinput: Avoid Object-Already-Free with a global lock
From: Al Viro @ 2019-04-24 22:56 UTC (permalink / raw)
To: Mukesh Ojha
Cc: dmitry.torokhov@gmail.com, linux-input, linux-kernel,
Gaurav Kohli, Peter Hutterer, Martin Kepplinger, Paul E. McKenney
In-Reply-To: <5b02ac1e-df3e-d9cd-ecf3-fe60cda2cece@codeaurora.org>
On Wed, Apr 24, 2019 at 07:39:03PM +0530, Mukesh Ojha wrote:
> 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
Er... So how does it manage to hit ioctl(2) before open(2)? Confused...
> > <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.
If descriptor table is not shared, fdget() will simply return you the reference
from there, without bothering to bump the refcount. _And_ having it marked
"don't drop refcount" in struct fd.
Rationale: since it's not shared, nobody other than our process can modify
it. So unless we remove (and drop) the reference from it ourselves (which
we certainly have no business doing in ->ioctl() and do not do anywhere
in drivers/input), it will remain there until we return from syscall.
Nobody is allowed to modify descriptor table other than their own.
And if it's not shared, no other owners can appear while the only
existing one is in the middle of syscall other than clone() (with
CLONE_FILES in flags, at that).
For shared descriptor table fdget() bumps file refcount, since there
the reference in descriptor table itself could be removed and dropped
by another thread.
And fdget() marks whether fput() is needed in fd.flags, so that
fdput() does the right thing.
^ permalink raw reply
* 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
* [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
* [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
* 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
* 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/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 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
* [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
* [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 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.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.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.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 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.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.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 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 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
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