* Re: [PATCH v5 0/2] Remove false-positive VLAs when using max()
From: Linus Torvalds @ 2018-03-16 20:19 UTC (permalink / raw)
To: Miguel Ojeda
Cc: Al Viro, Florian Weimer, Kees Cook, Andrew Morton, Josh Poimboeuf,
Rasmus Villemoes, Randy Dunlap, Ingo Molnar, David Laight,
Ian Abbott, linux-input, linux-btrfs, Network Development,
Linux Kernel Mailing List, Kernel Hardening
In-Reply-To: <CA+55aFxLpdfyfTqhkHQoedQuqTcLRw3bYOgyz1s0eRW6cBmFTA@mail.gmail.com>
On Fri, Mar 16, 2018 at 1:14 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> It does not work with gcc-4.1.x, but works with gcc-4.4.x.
>
> I can't seem to see the errors any way, I wonder if
> __builtin_choose_expr() simply didn't exist back then.
No, that goes further back.
It seems to be -Wvla itself that doesn't exist in 4.1, so the test
build failed simply because I used that flag ;)
Linus
^ permalink raw reply
* Re: [PATCH v5 0/2] Remove false-positive VLAs when using max()
From: Linus Torvalds @ 2018-03-16 20:25 UTC (permalink / raw)
To: David Laight
Cc: Florian Weimer, Kees Cook, Andrew Morton, Josh Poimboeuf,
Rasmus Villemoes, Randy Dunlap, Miguel Ojeda, Ingo Molnar,
Ian Abbott, linux-input, linux-btrfs, Network Development,
Linux Kernel Mailing List, Kernel Hardening
In-Reply-To: <a37f7b0da04f4ede9fac2c88f7079a8a@AcuMS.aculab.com>
On Fri, Mar 16, 2018 at 10:44 AM, David Laight <David.Laight@aculab.com> wrote:
>
> I looked at the generated code for one of the constant sized VLA that
> the compiler barfed at.
> It seemed to subtract constants from %sp separately for the VLA.
> So it looks like the compiler treats them as VLA even though it
> knows the size.
> That is probably missing optimisation.
Looking at the code is definitely an option.
In fact, instead of depending on -Wvla, we could just make 'objtool'
warn about real variable-sized stack frames.
That said, if that "sizeof()" trick of Al's actually works with older
gcc versions too (it *should*, but it's not like
__builtin_choose_expr() and __builtin_constant_p() have well-defined
rules in the standard), that may just be the solution.
And if gcc ends up generating bad code for those "constant sized vlas"
anyway, then -Wvla would effectively warn about that code generation
problem.
Linus
^ permalink raw reply
* Re: [PATCH 01/14] Input: atmel_mxt_ts - do not pass suspend mode in platform data
From: Nick Dyer @ 2018-03-16 20:40 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input, Benson Leung, Olof Johansson, linux-kernel
In-Reply-To: <20180315235621.GA60361@dtor-ws>
On Thu, Mar 15, 2018 at 04:56:21PM -0700, Dmitry Torokhov wrote:
> On Wed, Mar 14, 2018 at 08:51:24PM +0000, Nick Dyer wrote:
> > On Mon, Mar 12, 2018 at 12:08:54PM -0700, Dmitry Torokhov wrote:
> > > The way we are supposed to put controller to sleep and wake it up does not
> > > depend on the platform, but rather on controller itself. Controllers using
> > > T9 require manipulating T9 control register, while others, using newer
> > > T100, should be put to sleep by adjusting T7 power config.
> >
> > I'm afraid this is actually a misconception. If you look at object table
> > for the older T9 device, you'll find it has the T7 object and it in fact
> > works exactly the same way as the T100-based device.
> >
> > The MXT_SUSPEND_T9_CTRL is in there because on your older Pixel devices
> > the config saved into NVRAM on the touch controller has a zero byte in
> > the T9 CTRL setting, meaning the touch controller will never wake up
> > unless the driver knows to write 0x83 into it.
>
> Ah, OK, I see. I would really like to drop this pdata->suspend_mode
> stuff and I do not want to create "pixel-screwed-up" property either...
> I guess for the time being I'll put a DMI quirk for Link to restore T9
> control method, and then look into cleaning it all up. We have quite a
> bit different code in chromeos kernel trees and I'd like to reconcile
> it.
Yes, it would be great to get rid of it. The driver does have the
ability to download configuration via the firmware loader interface. So
you would be able to grab a copy of the config by saving it with
mxt-app, tweak it to ensure that the T9 CTRL byte is set correctly, then
ship it somehow (presumably it could be added to linux-firmware). This
would override what's currently stored in NVRAM on all those units and
mean we could remove the T9_CTRL stuff.
I'm happy to talk you through sorting that out in more detail if you
want to give it a go. I don't have any Pixel 1 hardware available at the
moment, unfortunately.
N
^ permalink raw reply
* Re: [PATCH 14/14] Input: atmel_mxt_ts - remove platform data support
From: Nick Dyer @ 2018-03-16 20:41 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input, Benson Leung, Olof Johansson, linux-kernel
In-Reply-To: <20180316000614.GB60361@dtor-ws>
On Thu, Mar 15, 2018 at 05:06:14PM -0700, Dmitry Torokhov wrote:
> On Wed, Mar 14, 2018 at 08:59:38PM +0000, Nick Dyer wrote:
> > On Mon, Mar 12, 2018 at 12:09:07PM -0700, Dmitry Torokhov wrote:
> > > - /*
> > > - * Ignore ACPI devices representing bootloader mode.
> > > - *
> > > - * This is a bit of a hack: Google Chromebook BIOS creates ACPI
> > > - * devices for both application and bootloader modes, but we are
> > > - * interested in application mode only (if device is in bootloader
> > > - * mode we'll end up switching into application anyway). So far
> > > - * application mode addresses were all above 0x40, so we'll use it
> > > - * as a threshold.
> > > - */
> > > - if (client->addr < 0x40)
> > > - return -ENXIO;
> >
> > Could you use 0x4a, since that's the lowest application mode address,
> > same in the other place.
>
> OK, this is existing code though, so I'll make a separate patch.
Ah, I see now. In which case it's no big deal.
Acked-by: Nick Dyer <nick@shmanahar.org>
^ permalink raw reply
* Re: [PATCH v5 0/2] Remove false-positive VLAs when using max()
From: Miguel Ojeda @ 2018-03-17 0:48 UTC (permalink / raw)
To: Linus Torvalds
Cc: Al Viro, Florian Weimer, Kees Cook, Andrew Morton, Josh Poimboeuf,
Rasmus Villemoes, Randy Dunlap, Ingo Molnar, David Laight,
Ian Abbott, linux-input, linux-btrfs, Network Development,
Linux Kernel Mailing List, Kernel Hardening
In-Reply-To: <CA+55aFxLpdfyfTqhkHQoedQuqTcLRw3bYOgyz1s0eRW6cBmFTA@mail.gmail.com>
On Fri, Mar 16, 2018 at 9:14 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Fri, Mar 16, 2018 at 1:03 PM, Miguel Ojeda
> <miguel.ojeda.sandonis@gmail.com> wrote:
>>>
>>> Kees - is there some online "gcc-4.4 checker" somewhere? This does
>>> seem to work with my gcc. I actually tested some of those files you
>>> pointed at now.
>>
>> I use this one:
>>
>> https://godbolt.org/
>
> Well, my *test* code works on that one and -Wvla -Werror.
>
> It does not work with gcc-4.1.x, but works with gcc-4.4.x.
>
> I can't seem to see the errors any way, I wonder if
> __builtin_choose_expr() simply didn't exist back then.
>
> Odd that you can't view warnings/errors with it.
Just click on the button left/bottom of the compilation window.
Cheers,
Miguel
^ permalink raw reply
* Re: [PATCH v5 0/2] Remove false-positive VLAs when using max()
From: Miguel Ojeda @ 2018-03-17 1:49 UTC (permalink / raw)
To: Linus Torvalds
Cc: Al Viro, Florian Weimer, Kees Cook, Andrew Morton, Josh Poimboeuf,
Rasmus Villemoes, Randy Dunlap, Ingo Molnar, David Laight,
Ian Abbott, linux-input, linux-btrfs, Network Development,
Linux Kernel Mailing List, Kernel Hardening
In-Reply-To: <CA+55aFxLpdfyfTqhkHQoedQuqTcLRw3bYOgyz1s0eRW6cBmFTA@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1815 bytes --]
On Fri, Mar 16, 2018 at 9:14 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Fri, Mar 16, 2018 at 1:03 PM, Miguel Ojeda
> <miguel.ojeda.sandonis@gmail.com> wrote:
>>>
>>> Kees - is there some online "gcc-4.4 checker" somewhere? This does
>>> seem to work with my gcc. I actually tested some of those files you
>>> pointed at now.
>>
>> I use this one:
>>
>> https://godbolt.org/
>
> Well, my *test* code works on that one and -Wvla -Werror.
>
> It does not work with gcc-4.1.x, but works with gcc-4.4.x.
>
> I can't seem to see the errors any way, I wonder if
> __builtin_choose_expr() simply didn't exist back then.
>
> Odd that you can't view warnings/errors with it.
>
> But it's possible that it fails on more complex stuff in the kernel.
>
> I've done a "allmodconfig" build with that patch, and the only issue
> it found was that (real) type issue in tpm_tis_core.h.
Just tested your max() with a Python script I wrote yesterday to try a
lot of combinations for this thing. Your version gives some warnings
in some cases, like:
warning: signed and unsigned type in conditional expression [-Wsign-compare]
#define __cmp(a,b,op) ((a)op(b)?(a):(b))
warning: comparison between signed and unsigned integer expressions
[-Wsign-compare]
#define const_max(a,b) __careful_cmp(a,b,>)
warning: comparison of distinct pointer types lacks a cast
(!!(sizeof((typeof(a)*)1==(typeof(b)*)1)))
But it fails on something like (with warnings):
int a[const_max(-30, 60u)];
Sorry... :-(
Has anyone taken a look at the last one I sent? Patch attached with
the draft changes on the kernel. It compiles fine the cases Kees
cleaned up in the other patch, but also works without a explicit type,
for mixed types, and for both positive and negative values.
Cheers,
Miguel
[-- Attachment #2: patch.diff --]
[-- Type: text/x-patch, Size: 2260 bytes --]
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 3fd291503576..f83658a4f15d 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -819,6 +819,49 @@ static inline void ftrace_dump(enum ftrace_dump_mode oops_dump_mode) { }
__UNIQUE_ID(max1_), __UNIQUE_ID(max2_), \
x, y)
+size_t __error_not_const_arg(void) \
+__compiletime_error("const_max() used with non-compile-time constant arg");
+size_t __error_too_big(void) \
+__compiletime_error("const_max() used with an arg too big");
+
+#define INTMAXT_MAX LLONG_MAX
+typedef int64_t intmax_t;
+
+#define const_cmp(x, y, op) \
+ __builtin_choose_expr( \
+ !__builtin_constant_p(x) || !__builtin_constant_p(y), \
+ __error_not_const_arg(), \
+ __builtin_choose_expr( \
+ (x) > INTMAXT_MAX || (y) > INTMAXT_MAX, \
+ __error_too_big(), \
+ __builtin_choose_expr( \
+ (intmax_t)(x) op (intmax_t)(y), \
+ (x), \
+ (y) \
+ ) \
+ ) \
+ )
+
+/**
+ * const_min - returns minimum of two compile-time constant values
+ * @x: first compile-time constant value
+ * @y: second compile-time constant value
+ *
+ * The result has the type of the winner of the comparison. Works for any
+ * value up to LLONG_MAX.
+ */
+#define const_min(x, y) const_cmp((x), (y), <=)
+
+/**
+ * const_max - returns maximum of two compile-time constant values
+ * @x: first compile-time constant value
+ * @y: second compile-time constant value
+ *
+ * The result has the type of the winner of the comparison. Works for any
+ * value up to LLONG_MAX.
+ */
+#define const_max(x, y) const_cmp((x), (y), >=)
+
/**
* min3 - return minimum of three values
* @x: first value
^ permalink raw reply related
* [PATCH] Input: atmel_mxt_ts: Add hook for Chromebooks with upstream coreboot
From: Jean Lucas @ 2018-03-17 4:37 UTC (permalink / raw)
To: Nick Dyer, Dmitry Torokhov; +Cc: linux-input, linux-kernel
In-Reply-To: <158c166b-bd85-53ab-dc02-920667c7409d@4ray.co>
Chromebooks use coreboot for system initialization. coreboot has always
had the default mainboard vendor string for Google machines set to
"Google". Google engineers set this string to "GOOGLE" in the coreboot
copies of the Chromium OS tree. The atmel_mxt_ts driver in it's current
state is set to match the latter case, i.e. it will only bind to a
Chromebook's touchscreen either if the device uses the downstream
coreboot firmware (providing the matching mainboard vendor string), or
if a user running upstream coreboot has manually set the string to
"GOOGLE". This patch adds a match for coreboot's default.
Signed-off-by: Jean Lucas <jean@4ray.co>
---
drivers/input/touchscreen/atmel_mxt_ts.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index 7659bc48f1db..8c74a3e13cca 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -3038,6 +3038,14 @@ static const struct dmi_system_id mxt_dmi_table[] = {
},
.driver_data = chromebook_platform_data,
},
+ {
+ /* Chromebooks with upstream coreboot */
+ .ident = "Chromebook",
+ .matches = {
+ DMI_MATCH(DMI_SYS_VENDOR, "Google"),
+ },
+ .driver_data = chromebook_platform_data,
+ },
{ }
};
--
2.16.2
^ permalink raw reply related
* Re: [PATCH v5 0/2] Remove false-positive VLAs when using max()
From: Kees Cook @ 2018-03-17 7:27 UTC (permalink / raw)
To: Linus Torvalds
Cc: Al Viro, Florian Weimer, Andrew Morton, Josh Poimboeuf,
Rasmus Villemoes, Randy Dunlap, Miguel Ojeda, Ingo Molnar,
David Laight, Ian Abbott, linux-input, linux-btrfs,
Network Development, Linux Kernel Mailing List, Kernel Hardening
In-Reply-To: <CA+55aFzsrVF7W+YQye=EeF4Wk3yDOTbb_vSiM8s1zkKbE4JV4Q@mail.gmail.com>
On Fri, Mar 16, 2018 at 12:27 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> Kees - is there some online "gcc-4.4 checker" somewhere? This does
> seem to work with my gcc. I actually tested some of those files you
> pointed at now.
Unfortunately my 4.4 test fails quickly:
./include/linux/jiffies.h: In function ‘jiffies_delta_to_clock_t’:
./include/linux/jiffies.h:444: error: first argument to
‘__builtin_choose_expr’ not a constant
static inline clock_t jiffies_delta_to_clock_t(long delta)
{
return jiffies_to_clock_t(max(0L, delta));
}
I think this is the same problem of using __builtin_constant_p() in
4.4 that we hit earlier? :(
-Kees
--
Kees Cook
Pixel Security
^ permalink raw reply
* Re: [PATCH v5 07/36] drm/bridge: analogix_dp: Move enable video into config_video()
From: Heiko Stuebner @ 2018-03-17 16:00 UTC (permalink / raw)
To: Archit Taneja
Cc: airlied, dri-devel, thierry.reding, Laurent.pinchart, ykk, kernel,
m.szyprowski, linux-samsung-soc, rydberg, krzk, linux-rockchip,
kgene, linux-input, orjan.eide, wxt, linux-kernel, jeffy.chen,
Stéphane Marchesin, linux-arm-kernel, mark.yao, wzz, hl,
jingoohan1, sw0312.kim, dianders, tfiga, kyungmin.park,
Enric Balletbo i Serra, kuankuan.y, hshi
In-Reply-To: <79009f48-d677-ccc0-4d61-3b379d8448a8@codeaurora.org>
Hi Archit,
Am Mittwoch, 14. März 2018, 06:59:59 CET schrieb Archit Taneja:
> On Saturday 10 March 2018 03:52 AM, Enric Balletbo i Serra wrote:
> > From: Lin Huang <hl@rock-chips.com>
> >
> > We need to enable video before analogix_dp_is_video_stream_on(), so
> > we can get the right video stream status.
> >
> > Cc: 征增 王 <wzz@rock-chips.com>
> > Cc: Stéphane Marchesin <marcheu@chromium.org>
> > Signed-off-by: Lin Huang <hl@rock-chips.com>
> > Signed-off-by: Sean Paul <seanpaul@chromium.org>
> > Signed-off-by: Thierry Escande <thierry.escande@collabora.com>
> > Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
> > Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> > Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
> > ---
> >
> > drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 11 +++++------
> > 1 file changed, 5 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> > index 5a2e35dc41e3..f9661b410cb9 100644
> > --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> > +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> > @@ -819,11 +819,10 @@ static int analogix_dp_config_video(struct analogix_dp_device *dp)
> > if (analogix_dp_is_slave_video_stream_clock_on(dp) == 0)
> > break;
> > if (timeout_loop > DP_TIMEOUT_LOOP_COUNT) {
> > - dev_err(dp->dev, "Timeout of video streamclk ok\n");
> > + dev_err(dp->dev, "Timeout of slave video streamclk ok\n");
> > return -ETIMEDOUT;
> > }
> > -
> > - usleep_range(1, 2);
> > + usleep_range(1000, 1001);
>
> Could we briefly explain in the commit message why we need to increase
> the delay in the timeout loop? Is it a consequence of calling
> analogix_dp_start_video() earlier, or is this the preferred time
> mentioned in the specs?
I asked Lin, the original author of this patch, response quoted below:
"There is random "Timeout of video streamclk ok" message happen when
debug edp panel,
So we extend this time, this time do not define in the spec."
So it looks like it was working by chance before and this move triggered
some sort of timing issue.
Heiko
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply
* Re: [PATCH 01/14] Input: atmel_mxt_ts - do not pass suspend mode in platform data
From: Dmitry Torokhov @ 2018-03-17 17:42 UTC (permalink / raw)
To: Nick Dyer; +Cc: linux-input, Benson Leung, Olof Johansson, linux-kernel
In-Reply-To: <20180316204002.GA24333@bm.shmanahar.org>
On Fri, Mar 16, 2018 at 08:40:02PM +0000, Nick Dyer wrote:
> On Thu, Mar 15, 2018 at 04:56:21PM -0700, Dmitry Torokhov wrote:
> > On Wed, Mar 14, 2018 at 08:51:24PM +0000, Nick Dyer wrote:
> > > On Mon, Mar 12, 2018 at 12:08:54PM -0700, Dmitry Torokhov wrote:
> > > > The way we are supposed to put controller to sleep and wake it up does not
> > > > depend on the platform, but rather on controller itself. Controllers using
> > > > T9 require manipulating T9 control register, while others, using newer
> > > > T100, should be put to sleep by adjusting T7 power config.
> > >
> > > I'm afraid this is actually a misconception. If you look at object table
> > > for the older T9 device, you'll find it has the T7 object and it in fact
> > > works exactly the same way as the T100-based device.
> > >
> > > The MXT_SUSPEND_T9_CTRL is in there because on your older Pixel devices
> > > the config saved into NVRAM on the touch controller has a zero byte in
> > > the T9 CTRL setting, meaning the touch controller will never wake up
> > > unless the driver knows to write 0x83 into it.
> >
> > Ah, OK, I see. I would really like to drop this pdata->suspend_mode
> > stuff and I do not want to create "pixel-screwed-up" property either...
> > I guess for the time being I'll put a DMI quirk for Link to restore T9
> > control method, and then look into cleaning it all up. We have quite a
> > bit different code in chromeos kernel trees and I'd like to reconcile
> > it.
>
> Yes, it would be great to get rid of it. The driver does have the
> ability to download configuration via the firmware loader interface. So
> you would be able to grab a copy of the config by saving it with
> mxt-app, tweak it to ensure that the T9 CTRL byte is set correctly, then
> ship it somehow (presumably it could be added to linux-firmware). This
> would override what's currently stored in NVRAM on all those units and
> mean we could remove the T9_CTRL stuff.
We can't really rely on people fetching updated config. Do you think we
could see if the device has only T9 and not T100 and if coming out of
suspend the T9 CTRL byte is 0 we overwrite it with the 0x83?
>
> I'm happy to talk you through sorting that out in more detail if you
> want to give it a go. I don't have any Pixel 1 hardware available at the
> moment, unfortunately.
Yes, I have several Chromebooks with Atmel so I can try on a few of
them.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [RESEND PATCH v5] input: pxrc: new driver for PhoenixRC Flight Controller Adapter
From: Dmitry Torokhov @ 2018-03-17 18:10 UTC (permalink / raw)
To: Marcus Folkesson; +Cc: Jonathan Corbet, linux-input, linux-doc, linux-kernel
In-Reply-To: <20180218161747.21110-1-marcus.folkesson@gmail.com>
On Sun, Feb 18, 2018 at 05:17:46PM +0100, Marcus Folkesson wrote:
> This driver let you plug in your RC controller to the adapter and
> use it as input device in various RC simulators.
>
> Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
Applied, thank you.
> ---
>
> v5:
> - Drop autosuspend support
> - Use pm_mutex instead of input_dev->mutex
> - Use pxrc->is_open instead of input_dev->users
> v4:
> - Add call to usb_mark_last_busy() in irq
> - Move code from pxrc_resume() to pxrc_reset_resume()
> v3:
> - Use RUDDER and MISC instead of TILT_X and TILT_Y
> - Drop kref and anchor
> - Rework URB handling
> - Add PM support
> v2:
> - Change module license to GPLv2 to match SPDX tag
>
>
> Documentation/input/devices/pxrc.rst | 57 +++++++
> drivers/input/joystick/Kconfig | 9 ++
> drivers/input/joystick/Makefile | 1 +
> drivers/input/joystick/pxrc.c | 303 +++++++++++++++++++++++++++++++++++
> 4 files changed, 370 insertions(+)
> create mode 100644 Documentation/input/devices/pxrc.rst
> create mode 100644 drivers/input/joystick/pxrc.c
>
> diff --git a/Documentation/input/devices/pxrc.rst b/Documentation/input/devices/pxrc.rst
> new file mode 100644
> index 000000000000..ca11f646bae8
> --- /dev/null
> +++ b/Documentation/input/devices/pxrc.rst
> @@ -0,0 +1,57 @@
> +=======================================================
> +pxrc - PhoenixRC Flight Controller Adapter
> +=======================================================
> +
> +:Author: Marcus Folkesson <marcus.folkesson@gmail.com>
> +
> +This driver let you use your own RC controller plugged into the
> +adapter that comes with PhoenixRC [1]_ or other compatible adapters.
> +
> +The adapter supports 7 analog channels and 1 digital input switch.
> +
> +Notes
> +=====
> +
> +Many RC controllers is able to configure which stick goes to which channel.
> +This is also configurable in most simulators, so a matching is not necessary.
> +
> +The driver is generating the following input event for analog channels:
> +
> ++---------+----------------+
> +| Channel | Event |
> ++=========+================+
> +| 1 | ABS_X |
> ++---------+----------------+
> +| 2 | ABS_Y |
> ++---------+----------------+
> +| 3 | ABS_RX |
> ++---------+----------------+
> +| 4 | ABS_RY |
> ++---------+----------------+
> +| 5 | ABS_RUDDER |
> ++---------+----------------+
> +| 6 | ABS_THROTTLE |
> ++---------+----------------+
> +| 7 | ABS_MISC |
> ++---------+----------------+
> +
> +The digital input switch is generated as an `BTN_A` event.
> +
> +Manual Testing
> +==============
> +
> +To test this driver's functionality you may use `input-event` which is part of
> +the `input layer utilities` suite [2]_.
> +
> +For example::
> +
> + > modprobe pxrc
> + > input-events <devnr>
> +
> +To print all input events from input `devnr`.
> +
> +References
> +==========
> +
> +.. [1] http://www.phoenix-sim.com/
> +.. [2] https://www.kraxel.org/cgit/input/
> diff --git a/drivers/input/joystick/Kconfig b/drivers/input/joystick/Kconfig
> index f3c2f6ea8b44..332c0cc1b2ab 100644
> --- a/drivers/input/joystick/Kconfig
> +++ b/drivers/input/joystick/Kconfig
> @@ -351,4 +351,13 @@ config JOYSTICK_PSXPAD_SPI_FF
>
> To drive rumble motor a dedicated power supply is required.
>
> +config JOYSTICK_PXRC
> + tristate "PhoenixRC Flight Controller Adapter"
> + depends on USB_ARCH_HAS_HCD
> + depends on USB
> + help
> + Say Y here if you want to use the PhoenixRC Flight Controller Adapter.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called pxrc.
> endif
> diff --git a/drivers/input/joystick/Makefile b/drivers/input/joystick/Makefile
> index 67651efda2e1..dd0492ebbed7 100644
> --- a/drivers/input/joystick/Makefile
> +++ b/drivers/input/joystick/Makefile
> @@ -23,6 +23,7 @@ obj-$(CONFIG_JOYSTICK_JOYDUMP) += joydump.o
> obj-$(CONFIG_JOYSTICK_MAGELLAN) += magellan.o
> obj-$(CONFIG_JOYSTICK_MAPLE) += maplecontrol.o
> obj-$(CONFIG_JOYSTICK_PSXPAD_SPI) += psxpad-spi.o
> +obj-$(CONFIG_JOYSTICK_PXRC) += pxrc.o
> obj-$(CONFIG_JOYSTICK_SIDEWINDER) += sidewinder.o
> obj-$(CONFIG_JOYSTICK_SPACEBALL) += spaceball.o
> obj-$(CONFIG_JOYSTICK_SPACEORB) += spaceorb.o
> diff --git a/drivers/input/joystick/pxrc.c b/drivers/input/joystick/pxrc.c
> new file mode 100644
> index 000000000000..07a0dbd3ced2
> --- /dev/null
> +++ b/drivers/input/joystick/pxrc.c
> @@ -0,0 +1,303 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Driver for Phoenix RC Flight Controller Adapter
> + *
> + * Copyright (C) 2018 Marcus Folkesson <marcus.folkesson@gmail.com>
> + *
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/errno.h>
> +#include <linux/slab.h>
> +#include <linux/module.h>
> +#include <linux/uaccess.h>
> +#include <linux/usb.h>
> +#include <linux/usb/input.h>
> +#include <linux/mutex.h>
> +#include <linux/input.h>
> +
> +#define PXRC_VENDOR_ID (0x1781)
> +#define PXRC_PRODUCT_ID (0x0898)
> +
> +static const struct usb_device_id pxrc_table[] = {
> + { USB_DEVICE(PXRC_VENDOR_ID, PXRC_PRODUCT_ID) },
> + { }
> +};
> +MODULE_DEVICE_TABLE(usb, pxrc_table);
> +
> +struct pxrc {
> + struct input_dev *input;
> + struct usb_device *udev;
> + struct usb_interface *intf;
> + struct urb *urb;
> + struct mutex pm_mutex;
> + bool is_open;
> + __u8 epaddr;
> + char phys[64];
> + unsigned char *data;
> + size_t bsize;
> +};
> +
> +static void pxrc_usb_irq(struct urb *urb)
> +{
> + struct pxrc *pxrc = urb->context;
> + int error;
> +
> + switch (urb->status) {
> + case 0:
> + /* success */
> + break;
> + case -ETIME:
> + /* this urb is timing out */
> + dev_dbg(&pxrc->intf->dev,
> + "%s - urb timed out - was the device unplugged?\n",
> + __func__);
> + return;
> + case -ECONNRESET:
> + case -ENOENT:
> + case -ESHUTDOWN:
> + case -EPIPE:
> + /* this urb is terminated, clean up */
> + dev_dbg(&pxrc->intf->dev, "%s - urb shutting down with status: %d\n",
> + __func__, urb->status);
> + return;
> + default:
> + dev_dbg(&pxrc->intf->dev, "%s - nonzero urb status received: %d\n",
> + __func__, urb->status);
> + goto exit;
> + }
> +
> + if (urb->actual_length == 8) {
> + input_report_abs(pxrc->input, ABS_X, pxrc->data[0]);
> + input_report_abs(pxrc->input, ABS_Y, pxrc->data[2]);
> + input_report_abs(pxrc->input, ABS_RX, pxrc->data[3]);
> + input_report_abs(pxrc->input, ABS_RY, pxrc->data[4]);
> + input_report_abs(pxrc->input, ABS_RUDDER, pxrc->data[5]);
> + input_report_abs(pxrc->input, ABS_THROTTLE, pxrc->data[6]);
> + input_report_abs(pxrc->input, ABS_MISC, pxrc->data[7]);
> +
> + input_report_key(pxrc->input, BTN_A, pxrc->data[1]);
> + }
> +
> +exit:
> + /* Resubmit to fetch new fresh URBs */
> + error = usb_submit_urb(urb, GFP_ATOMIC);
> + if (error && error != -EPERM)
> + dev_err(&pxrc->intf->dev,
> + "%s - usb_submit_urb failed with result: %d",
> + __func__, error);
> +}
> +
> +static int pxrc_open(struct input_dev *input)
> +{
> + struct pxrc *pxrc = input_get_drvdata(input);
> + int retval;
> +
> + mutex_lock(&pxrc->pm_mutex);
> + retval = usb_submit_urb(pxrc->urb, GFP_KERNEL);
> + if (retval) {
> + dev_err(&pxrc->intf->dev,
> + "%s - usb_submit_urb failed, error: %d\n",
> + __func__, retval);
> + retval = -EIO;
> + goto out;
> + }
> +
> + pxrc->is_open = true;
> +
> +out:
> + mutex_unlock(&pxrc->pm_mutex);
> + return retval;
> +}
> +
> +static void pxrc_close(struct input_dev *input)
> +{
> + struct pxrc *pxrc = input_get_drvdata(input);
> +
> + mutex_lock(&pxrc->pm_mutex);
> + usb_kill_urb(pxrc->urb);
> + pxrc->is_open = false;
> + mutex_unlock(&pxrc->pm_mutex);
> +}
> +
> +static int pxrc_usb_init(struct pxrc *pxrc)
> +{
> + struct usb_endpoint_descriptor *epirq;
> + unsigned int pipe;
> + int retval;
> +
> + /* Set up the endpoint information */
> + /* This device only has an interrupt endpoint */
> + retval = usb_find_common_endpoints(pxrc->intf->cur_altsetting,
> + NULL, NULL, &epirq, NULL);
> + if (retval) {
> + dev_err(&pxrc->intf->dev,
> + "Could not find endpoint\n");
> + goto error;
> + }
> +
> + pxrc->bsize = usb_endpoint_maxp(epirq);
> + pxrc->epaddr = epirq->bEndpointAddress;
> + pxrc->data = devm_kmalloc(&pxrc->intf->dev, pxrc->bsize, GFP_KERNEL);
> + if (!pxrc->data) {
> + retval = -ENOMEM;
> + goto error;
> + }
> +
> + usb_set_intfdata(pxrc->intf, pxrc);
> + usb_make_path(pxrc->udev, pxrc->phys, sizeof(pxrc->phys));
> + strlcat(pxrc->phys, "/input0", sizeof(pxrc->phys));
> +
> + pxrc->urb = usb_alloc_urb(0, GFP_KERNEL);
> + if (!pxrc->urb) {
> + retval = -ENOMEM;
> + goto error;
> + }
> +
> + pipe = usb_rcvintpipe(pxrc->udev, pxrc->epaddr),
> + usb_fill_int_urb(pxrc->urb, pxrc->udev, pipe, pxrc->data, pxrc->bsize,
> + pxrc_usb_irq, pxrc, 1);
> +
> +error:
> + return retval;
> +
> +
> +}
> +
> +static int pxrc_input_init(struct pxrc *pxrc)
> +{
> + pxrc->input = devm_input_allocate_device(&pxrc->intf->dev);
> + if (pxrc->input == NULL) {
> + dev_err(&pxrc->intf->dev, "couldn't allocate input device\n");
> + return -ENOMEM;
> + }
> +
> + pxrc->input->name = "PXRC Flight Controller Adapter";
> + pxrc->input->phys = pxrc->phys;
> + usb_to_input_id(pxrc->udev, &pxrc->input->id);
> +
> + pxrc->input->open = pxrc_open;
> + pxrc->input->close = pxrc_close;
> +
> + input_set_capability(pxrc->input, EV_KEY, BTN_A);
> + input_set_abs_params(pxrc->input, ABS_X, 0, 255, 0, 0);
> + input_set_abs_params(pxrc->input, ABS_Y, 0, 255, 0, 0);
> + input_set_abs_params(pxrc->input, ABS_RX, 0, 255, 0, 0);
> + input_set_abs_params(pxrc->input, ABS_RY, 0, 255, 0, 0);
> + input_set_abs_params(pxrc->input, ABS_RUDDER, 0, 255, 0, 0);
> + input_set_abs_params(pxrc->input, ABS_THROTTLE, 0, 255, 0, 0);
> + input_set_abs_params(pxrc->input, ABS_MISC, 0, 255, 0, 0);
> +
> + input_set_drvdata(pxrc->input, pxrc);
> +
> + return input_register_device(pxrc->input);
> +}
> +
> +static int pxrc_probe(struct usb_interface *intf,
> + const struct usb_device_id *id)
> +{
> + struct pxrc *pxrc;
> + int retval;
> +
> + pxrc = devm_kzalloc(&intf->dev, sizeof(*pxrc), GFP_KERNEL);
> + if (!pxrc)
> + return -ENOMEM;
> +
> + mutex_init(&pxrc->pm_mutex);
> + pxrc->udev = usb_get_dev(interface_to_usbdev(intf));
> + pxrc->intf = intf;
> +
> + retval = pxrc_usb_init(pxrc);
> + if (retval)
> + goto error;
> +
> + retval = pxrc_input_init(pxrc);
> + if (retval)
> + goto err_free_urb;
> +
> + return 0;
> +
> +err_free_urb:
> + usb_free_urb(pxrc->urb);
> +
> +error:
> + return retval;
> +}
> +
> +static void pxrc_disconnect(struct usb_interface *intf)
> +{
> + struct pxrc *pxrc = usb_get_intfdata(intf);
> +
> + usb_free_urb(pxrc->urb);
> + usb_set_intfdata(intf, NULL);
> +}
> +
> +static int pxrc_suspend(struct usb_interface *intf, pm_message_t message)
> +{
> + struct pxrc *pxrc = usb_get_intfdata(intf);
> +
> + mutex_lock(&pxrc->pm_mutex);
> + if (pxrc->is_open)
> + usb_kill_urb(pxrc->urb);
> + mutex_unlock(&pxrc->pm_mutex);
> +
> + return 0;
> +}
> +
> +static int pxrc_resume(struct usb_interface *intf)
> +{
> + struct pxrc *pxrc = usb_get_intfdata(intf);
> + int retval = 0;
> +
> + mutex_lock(&pxrc->pm_mutex);
> + if (pxrc->is_open && usb_submit_urb(pxrc->urb, GFP_KERNEL) < 0)
> + retval = -EIO;
> +
> + mutex_unlock(&pxrc->pm_mutex);
> + return retval;
> +}
> +
> +static int pxrc_pre_reset(struct usb_interface *intf)
> +{
> + struct pxrc *pxrc = usb_get_intfdata(intf);
> +
> + mutex_lock(&pxrc->pm_mutex);
> + usb_kill_urb(pxrc->urb);
> + return 0;
> +}
> +
> +static int pxrc_post_reset(struct usb_interface *intf)
> +{
> + struct pxrc *pxrc = usb_get_intfdata(intf);
> + int retval = 0;
> +
> + if (pxrc->is_open && usb_submit_urb(pxrc->urb, GFP_KERNEL) < 0)
> + retval = -EIO;
> +
> + mutex_unlock(&pxrc->pm_mutex);
> +
> + return retval;
> +}
> +
> +static int pxrc_reset_resume(struct usb_interface *intf)
> +{
> + return pxrc_resume(intf);
> +}
> +
> +static struct usb_driver pxrc_driver = {
> + .name = "pxrc",
> + .probe = pxrc_probe,
> + .disconnect = pxrc_disconnect,
> + .id_table = pxrc_table,
> + .suspend = pxrc_suspend,
> + .resume = pxrc_resume,
> + .pre_reset = pxrc_pre_reset,
> + .post_reset = pxrc_post_reset,
> + .reset_resume = pxrc_reset_resume,
> +};
> +
> +module_usb_driver(pxrc_driver);
> +
> +MODULE_AUTHOR("Marcus Folkesson <marcus.folkesson@gmail.com>");
> +MODULE_DESCRIPTION("PhoenixRC Flight Controller Adapter");
> +MODULE_LICENSE("GPL v2");
> --
> 2.15.1
>
--
Dmitry
^ permalink raw reply
* Re: [RESEND PATCH 1/6] input: synaptics_usb: fix deadlock in autosuspend
From: Dmitry Torokhov @ 2018-03-17 18:10 UTC (permalink / raw)
To: Marcus Folkesson; +Cc: Arvind Yadav, linux-input, linux-kernel
In-Reply-To: <20180228133803.30040-2-marcus.folkesson@gmail.com>
On Wed, Feb 28, 2018 at 02:37:58PM +0100, Marcus Folkesson wrote:
> usb_autopm_get_interface() that is called in synusb_open() does an
> autoresume if the device is suspended.
>
> input_dev->mutex used in synusb_resume() is in this case already
> taken by the input subsystem and will cause a deadlock.
>
> Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
Applied, thank you.
> ---
> drivers/input/mouse/synaptics_usb.c | 22 ++++++++++++++--------
> 1 file changed, 14 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/input/mouse/synaptics_usb.c b/drivers/input/mouse/synaptics_usb.c
> index cb7d15d826d0..2c66913cf5a2 100644
> --- a/drivers/input/mouse/synaptics_usb.c
> +++ b/drivers/input/mouse/synaptics_usb.c
> @@ -82,6 +82,9 @@ struct synusb {
> struct urb *urb;
> unsigned char *data;
>
> + /* serialize access to open/suspend */
> + struct mutex pm_mutex;
> +
> /* input device related data structures */
> struct input_dev *input;
> char name[128];
> @@ -252,6 +255,7 @@ static int synusb_open(struct input_dev *dev)
> return retval;
> }
>
> + mutex_lock(&synusb->pm_mutex);
> retval = usb_submit_urb(synusb->urb, GFP_KERNEL);
> if (retval) {
> dev_err(&synusb->intf->dev,
> @@ -264,6 +268,7 @@ static int synusb_open(struct input_dev *dev)
> synusb->intf->needs_remote_wakeup = 1;
>
> out:
> + mutex_unlock(&synusb->pm_mutex);
> usb_autopm_put_interface(synusb->intf);
> return retval;
> }
> @@ -275,8 +280,10 @@ static void synusb_close(struct input_dev *dev)
>
> autopm_error = usb_autopm_get_interface(synusb->intf);
>
> + mutex_lock(&synusb->pm_mutex);
> usb_kill_urb(synusb->urb);
> synusb->intf->needs_remote_wakeup = 0;
> + mutex_unlock(&synusb->pm_mutex);
>
> if (!autopm_error)
> usb_autopm_put_interface(synusb->intf);
> @@ -315,6 +322,7 @@ static int synusb_probe(struct usb_interface *intf,
> synusb->udev = udev;
> synusb->intf = intf;
> synusb->input = input_dev;
> + mutex_init(&synusb->pm_mutex);
>
> synusb->flags = id->driver_info;
> if (synusb->flags & SYNUSB_COMBO) {
> @@ -466,11 +474,10 @@ static void synusb_disconnect(struct usb_interface *intf)
> static int synusb_suspend(struct usb_interface *intf, pm_message_t message)
> {
> struct synusb *synusb = usb_get_intfdata(intf);
> - struct input_dev *input_dev = synusb->input;
>
> - mutex_lock(&input_dev->mutex);
> + mutex_lock(&synusb->pm_mutex);
> usb_kill_urb(synusb->urb);
> - mutex_unlock(&input_dev->mutex);
> + mutex_unlock(&synusb->pm_mutex);
>
> return 0;
> }
> @@ -481,14 +488,14 @@ static int synusb_resume(struct usb_interface *intf)
> struct input_dev *input_dev = synusb->input;
> int retval = 0;
>
> - mutex_lock(&input_dev->mutex);
> + mutex_lock(&synusb->pm_mutex);
>
> if ((input_dev->users || (synusb->flags & SYNUSB_IO_ALWAYS)) &&
> usb_submit_urb(synusb->urb, GFP_NOIO) < 0) {
> retval = -EIO;
> }
>
> - mutex_unlock(&input_dev->mutex);
> + mutex_unlock(&synusb->pm_mutex);
>
> return retval;
> }
> @@ -496,9 +503,8 @@ static int synusb_resume(struct usb_interface *intf)
> static int synusb_pre_reset(struct usb_interface *intf)
> {
> struct synusb *synusb = usb_get_intfdata(intf);
> - struct input_dev *input_dev = synusb->input;
>
> - mutex_lock(&input_dev->mutex);
> + mutex_lock(&synusb->pm_mutex);
> usb_kill_urb(synusb->urb);
>
> return 0;
> @@ -515,7 +521,7 @@ static int synusb_post_reset(struct usb_interface *intf)
> retval = -EIO;
> }
>
> - mutex_unlock(&input_dev->mutex);
> + mutex_unlock(&synusb->pm_mutex);
>
> return retval;
> }
> --
> 2.16.2
>
--
Dmitry
^ permalink raw reply
* Re: [RESEND PATCH 2/6] input: synaptics_usb: do not rely on input_dev->users
From: Dmitry Torokhov @ 2018-03-17 18:10 UTC (permalink / raw)
To: Marcus Folkesson; +Cc: Arvind Yadav, linux-input, linux-kernel
In-Reply-To: <20180228133803.30040-3-marcus.folkesson@gmail.com>
On Wed, Feb 28, 2018 at 02:37:59PM +0100, Marcus Folkesson wrote:
> If the device is unused and suspended, a call to open will cause the
> device to autoresume through the call to usb_autopm_get_interface().
>
> input_dev->users is already incremented by the input subsystem,
> therefore this expression will always be evaluated to true:
>
> if ((input_dev->users || (synusb->flags & SYNUSB_IO_ALWAYS)) &&
> usb_submit_urb(synusb->urb, GFP_NOIO) < 0) {
> retval = -EIO;
> }
>
> The same URB will then be fail when resubmitted in synusb_open().
>
> Introduce synusb->is_open to keep track of the state instead.
>
> Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
Applied, thank you.
> ---
> drivers/input/mouse/synaptics_usb.c | 9 +++++----
> 1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/input/mouse/synaptics_usb.c b/drivers/input/mouse/synaptics_usb.c
> index 2c66913cf5a2..e2b726751220 100644
> --- a/drivers/input/mouse/synaptics_usb.c
> +++ b/drivers/input/mouse/synaptics_usb.c
> @@ -84,6 +84,7 @@ struct synusb {
>
> /* serialize access to open/suspend */
> struct mutex pm_mutex;
> + bool is_open;
>
> /* input device related data structures */
> struct input_dev *input;
> @@ -266,6 +267,7 @@ static int synusb_open(struct input_dev *dev)
> }
>
> synusb->intf->needs_remote_wakeup = 1;
> + synusb->is_open = 1;
>
> out:
> mutex_unlock(&synusb->pm_mutex);
> @@ -283,6 +285,7 @@ static void synusb_close(struct input_dev *dev)
> mutex_lock(&synusb->pm_mutex);
> usb_kill_urb(synusb->urb);
> synusb->intf->needs_remote_wakeup = 0;
> + synusb->is_open = 0;
> mutex_unlock(&synusb->pm_mutex);
>
> if (!autopm_error)
> @@ -485,12 +488,11 @@ static int synusb_suspend(struct usb_interface *intf, pm_message_t message)
> static int synusb_resume(struct usb_interface *intf)
> {
> struct synusb *synusb = usb_get_intfdata(intf);
> - struct input_dev *input_dev = synusb->input;
> int retval = 0;
>
> mutex_lock(&synusb->pm_mutex);
>
> - if ((input_dev->users || (synusb->flags & SYNUSB_IO_ALWAYS)) &&
> + if ((synusb->is_open || (synusb->flags & SYNUSB_IO_ALWAYS)) &&
> usb_submit_urb(synusb->urb, GFP_NOIO) < 0) {
> retval = -EIO;
> }
> @@ -513,10 +515,9 @@ static int synusb_pre_reset(struct usb_interface *intf)
> static int synusb_post_reset(struct usb_interface *intf)
> {
> struct synusb *synusb = usb_get_intfdata(intf);
> - struct input_dev *input_dev = synusb->input;
> int retval = 0;
>
> - if ((input_dev->users || (synusb->flags & SYNUSB_IO_ALWAYS)) &&
> + if ((synusb->is_open || (synusb->flags & SYNUSB_IO_ALWAYS)) &&
> usb_submit_urb(synusb->urb, GFP_NOIO) < 0) {
> retval = -EIO;
> }
> --
> 2.16.2
>
--
Dmitry
^ permalink raw reply
* Re: [RESEND PATCH 3/6] input: pagasus_notetaker: fix deadlock in autosuspend
From: Dmitry Torokhov @ 2018-03-17 18:10 UTC (permalink / raw)
To: Marcus Folkesson; +Cc: Arvind Yadav, linux-input, linux-kernel
In-Reply-To: <20180228133803.30040-4-marcus.folkesson@gmail.com>
On Wed, Feb 28, 2018 at 02:38:00PM +0100, Marcus Folkesson wrote:
> usb_autopm_get_interface() that is called in pegasus_open() does an
> autoresume if the device is suspended.
>
> input_dev->mutex used in pegasus_resume() is in this case already
> taken by the input subsystem and will cause a deadlock.
>
> Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
Applied, thank you.
> ---
> drivers/input/tablet/pegasus_notetaker.c | 25 +++++++++++++++++++------
> 1 file changed, 19 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/input/tablet/pegasus_notetaker.c b/drivers/input/tablet/pegasus_notetaker.c
> index 47de5a81172f..9ab1ed5e20e7 100644
> --- a/drivers/input/tablet/pegasus_notetaker.c
> +++ b/drivers/input/tablet/pegasus_notetaker.c
> @@ -41,6 +41,7 @@
> #include <linux/usb/input.h>
> #include <linux/slab.h>
> #include <linux/workqueue.h>
> +#include <linux/mutex.h>
>
> /* USB HID defines */
> #define USB_REQ_GET_REPORT 0x01
> @@ -76,6 +77,10 @@ struct pegasus {
> struct usb_device *usbdev;
> struct usb_interface *intf;
> struct urb *irq;
> +
> + /* serialize access to open/suspend */
> + struct mutex pm_mutex;
> +
> char name[128];
> char phys[64];
> struct work_struct init;
> @@ -216,6 +221,7 @@ static int pegasus_open(struct input_dev *dev)
> if (error)
> return error;
>
> + mutex_lock(&pegasus->pm_mutex);
> pegasus->irq->dev = pegasus->usbdev;
> if (usb_submit_urb(pegasus->irq, GFP_KERNEL)) {
> error = -EIO;
> @@ -226,12 +232,14 @@ static int pegasus_open(struct input_dev *dev)
> if (error)
> goto err_kill_urb;
>
> + mutex_unlock(&pegasus->pm_mutex);
> return 0;
>
> err_kill_urb:
> usb_kill_urb(pegasus->irq);
> cancel_work_sync(&pegasus->init);
> err_autopm_put:
> + mutex_unlock(&pegasus->pm_mutex);
> usb_autopm_put_interface(pegasus->intf);
> return error;
> }
> @@ -240,8 +248,11 @@ static void pegasus_close(struct input_dev *dev)
> {
> struct pegasus *pegasus = input_get_drvdata(dev);
>
> + mutex_lock(&pegasus->pm_mutex);
> usb_kill_urb(pegasus->irq);
> cancel_work_sync(&pegasus->init);
> + mutex_unlock(&pegasus->pm_mutex);
> +
> usb_autopm_put_interface(pegasus->intf);
> }
>
> @@ -274,6 +285,8 @@ static int pegasus_probe(struct usb_interface *intf,
> goto err_free_mem;
> }
>
> + mutex_init(&pegasus->pm_mutex);
> +
> pegasus->usbdev = dev;
> pegasus->dev = input_dev;
> pegasus->intf = intf;
> @@ -388,10 +401,10 @@ static int pegasus_suspend(struct usb_interface *intf, pm_message_t message)
> {
> struct pegasus *pegasus = usb_get_intfdata(intf);
>
> - mutex_lock(&pegasus->dev->mutex);
> + mutex_lock(&pegasus->pm_mutex);
> usb_kill_urb(pegasus->irq);
> cancel_work_sync(&pegasus->init);
> - mutex_unlock(&pegasus->dev->mutex);
> + mutex_unlock(&pegasus->pm_mutex);
>
> return 0;
> }
> @@ -401,10 +414,10 @@ static int pegasus_resume(struct usb_interface *intf)
> struct pegasus *pegasus = usb_get_intfdata(intf);
> int retval = 0;
>
> - mutex_lock(&pegasus->dev->mutex);
> + mutex_lock(&pegasus->pm_mutex);
> if (pegasus->dev->users && usb_submit_urb(pegasus->irq, GFP_NOIO) < 0)
> retval = -EIO;
> - mutex_unlock(&pegasus->dev->mutex);
> + mutex_unlock(&pegasus->pm_mutex);
>
> return retval;
> }
> @@ -414,14 +427,14 @@ static int pegasus_reset_resume(struct usb_interface *intf)
> struct pegasus *pegasus = usb_get_intfdata(intf);
> int retval = 0;
>
> - mutex_lock(&pegasus->dev->mutex);
> + mutex_lock(&pegasus->pm_mutex);
> if (pegasus->dev->users) {
> retval = pegasus_set_mode(pegasus, PEN_MODE_XY,
> NOTETAKER_LED_MOUSE);
> if (!retval && usb_submit_urb(pegasus->irq, GFP_NOIO) < 0)
> retval = -EIO;
> }
> - mutex_unlock(&pegasus->dev->mutex);
> + mutex_unlock(&pegasus->pm_mutex);
>
> return retval;
> }
> --
> 2.16.2
>
--
Dmitry
^ permalink raw reply
* Re: [RESEND PATCH 4/6] input: pegasus_notetaker: do not rely on input_dev->users
From: Dmitry Torokhov @ 2018-03-17 18:11 UTC (permalink / raw)
To: Marcus Folkesson; +Cc: Arvind Yadav, linux-input, linux-kernel
In-Reply-To: <20180228133803.30040-5-marcus.folkesson@gmail.com>
On Wed, Feb 28, 2018 at 02:38:01PM +0100, Marcus Folkesson wrote:
> If the device is unused and suspended, a call to open will cause the
> device to autoresume through the call to usb_autopm_get_interface().
>
> input_dev->users is already incremented by the input subsystem,
> therefore this expression will always be evaluated to true:
>
> if (pegasus->dev->users && usb_submit_urb(pegasus->irq, GFP_NOIO) < 0)
> retval = -EIO;
>
> The same URB will then be fail when resubmitted in pegasus_open().
>
> Introduce pegasus->is_open to keep track of the state instead.
>
> Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
Applied, thank you.
> ---
> drivers/input/tablet/pegasus_notetaker.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/input/tablet/pegasus_notetaker.c b/drivers/input/tablet/pegasus_notetaker.c
> index 9ab1ed5e20e7..74c93e09c337 100644
> --- a/drivers/input/tablet/pegasus_notetaker.c
> +++ b/drivers/input/tablet/pegasus_notetaker.c
> @@ -80,6 +80,7 @@ struct pegasus {
>
> /* serialize access to open/suspend */
> struct mutex pm_mutex;
> + bool is_open;
>
> char name[128];
> char phys[64];
> @@ -232,6 +233,7 @@ static int pegasus_open(struct input_dev *dev)
> if (error)
> goto err_kill_urb;
>
> + pegasus->is_open = 1;
> mutex_unlock(&pegasus->pm_mutex);
> return 0;
>
> @@ -251,6 +253,7 @@ static void pegasus_close(struct input_dev *dev)
> mutex_lock(&pegasus->pm_mutex);
> usb_kill_urb(pegasus->irq);
> cancel_work_sync(&pegasus->init);
> + pegasus->is_open = 0;
> mutex_unlock(&pegasus->pm_mutex);
>
> usb_autopm_put_interface(pegasus->intf);
> @@ -415,7 +418,7 @@ static int pegasus_resume(struct usb_interface *intf)
> int retval = 0;
>
> mutex_lock(&pegasus->pm_mutex);
> - if (pegasus->dev->users && usb_submit_urb(pegasus->irq, GFP_NOIO) < 0)
> + if (pegasus->is_open && usb_submit_urb(pegasus->irq, GFP_NOIO) < 0)
> retval = -EIO;
> mutex_unlock(&pegasus->pm_mutex);
>
> @@ -428,7 +431,7 @@ static int pegasus_reset_resume(struct usb_interface *intf)
> int retval = 0;
>
> mutex_lock(&pegasus->pm_mutex);
> - if (pegasus->dev->users) {
> + if (pegasus->is_open) {
> retval = pegasus_set_mode(pegasus, PEN_MODE_XY,
> NOTETAKER_LED_MOUSE);
> if (!retval && usb_submit_urb(pegasus->irq, GFP_NOIO) < 0)
> --
> 2.16.2
>
--
Dmitry
^ permalink raw reply
* Re: [RESEND PATCH 5/6] input: usbtouchscreen: fix deadlock in autosuspend
From: Dmitry Torokhov @ 2018-03-17 18:11 UTC (permalink / raw)
To: Marcus Folkesson; +Cc: Arvind Yadav, linux-input, linux-kernel
In-Reply-To: <20180228133803.30040-6-marcus.folkesson@gmail.com>
On Wed, Feb 28, 2018 at 02:38:02PM +0100, Marcus Folkesson wrote:
> usb_autopm_get_interface() that is called in usbtouch_open() does an
> autoresume if the device is suspended.
>
> input_dev->mutex used in usbtouch_resume() is in this case already
> taken by the input subsystem and will cause a deadlock.
>
> Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
Applied, thank you.
> ---
> drivers/input/touchscreen/usbtouchscreen.c | 15 +++++++++++----
> 1 file changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/input/touchscreen/usbtouchscreen.c b/drivers/input/touchscreen/usbtouchscreen.c
> index 2c41107240de..e964658203d8 100644
> --- a/drivers/input/touchscreen/usbtouchscreen.c
> +++ b/drivers/input/touchscreen/usbtouchscreen.c
> @@ -54,6 +54,7 @@
> #include <linux/usb.h>
> #include <linux/usb/input.h>
> #include <linux/hid.h>
> +#include <linux/mutex.h>
>
>
> #define DRIVER_VERSION "v0.6"
> @@ -112,6 +113,7 @@ struct usbtouch_usb {
> struct usb_interface *interface;
> struct input_dev *input;
> struct usbtouch_device_info *type;
> + struct mutex pm_mutex; /* serialize access to open/suspend */
> char name[128];
> char phys[64];
> void *priv;
> @@ -1455,6 +1457,7 @@ static int usbtouch_open(struct input_dev *input)
> if (r < 0)
> goto out;
>
> + mutex_lock(&usbtouch->pm_mutex);
> if (!usbtouch->type->irq_always) {
> if (usb_submit_urb(usbtouch->irq, GFP_KERNEL)) {
> r = -EIO;
> @@ -1464,6 +1467,7 @@ static int usbtouch_open(struct input_dev *input)
>
> usbtouch->interface->needs_remote_wakeup = 1;
> out_put:
> + mutex_unlock(&usbtouch->pm_mutex);
> usb_autopm_put_interface(usbtouch->interface);
> out:
> return r;
> @@ -1474,8 +1478,11 @@ static void usbtouch_close(struct input_dev *input)
> struct usbtouch_usb *usbtouch = input_get_drvdata(input);
> int r;
>
> + mutex_lock(&usbtouch->pm_mutex);
> if (!usbtouch->type->irq_always)
> usb_kill_urb(usbtouch->irq);
> + mutex_lock(&usbtouch->pm_mutex);
> +
> r = usb_autopm_get_interface(usbtouch->interface);
> usbtouch->interface->needs_remote_wakeup = 0;
> if (!r)
> @@ -1498,10 +1505,10 @@ static int usbtouch_resume(struct usb_interface *intf)
> struct input_dev *input = usbtouch->input;
> int result = 0;
>
> - mutex_lock(&input->mutex);
> + mutex_lock(&usbtouch->pm_mutex);
> if (input->users || usbtouch->type->irq_always)
> result = usb_submit_urb(usbtouch->irq, GFP_NOIO);
> - mutex_unlock(&input->mutex);
> + mutex_unlock(&usbtouch->pm_mutex);
>
> return result;
> }
> @@ -1524,10 +1531,10 @@ static int usbtouch_reset_resume(struct usb_interface *intf)
> }
>
> /* restart IO if needed */
> - mutex_lock(&input->mutex);
> + mutex_lock(&usbtouch->pm_mutex);
> if (input->users)
> err = usb_submit_urb(usbtouch->irq, GFP_NOIO);
> - mutex_unlock(&input->mutex);
> + mutex_unlock(&usbtouch->pm_mutex);
>
> return err;
> }
> --
> 2.16.2
>
--
Dmitry
^ permalink raw reply
* Re: [RESEND PATCH 6/6] input: usbtouchscreen: do not rely on input_dev->users
From: Dmitry Torokhov @ 2018-03-17 18:11 UTC (permalink / raw)
To: Marcus Folkesson; +Cc: Arvind Yadav, linux-input, linux-kernel
In-Reply-To: <20180228133803.30040-7-marcus.folkesson@gmail.com>
On Wed, Feb 28, 2018 at 02:38:03PM +0100, Marcus Folkesson wrote:
> If the device is unused and suspended, a call to open will cause the
> device to autoresume through the call to usb_autopm_get_interface().
>
> input_dev->users is already incremented by the input subsystem,
> therefore this expression will always be evaluated to true:
>
> if (input->users || usbtouch->type->irq_always)
> result = usb_submit_urb(usbtouch->irq, GFP_NOIO);
>
> The same URB will then be fail when resubmitted in usbtouch_open().
>
> Introduce usbtouch->is_open to keep track of the state instead.
>
> Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
Applied, thank you.
> ---
> drivers/input/touchscreen/usbtouchscreen.c | 9 +++++----
> 1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/input/touchscreen/usbtouchscreen.c b/drivers/input/touchscreen/usbtouchscreen.c
> index e964658203d8..0356ad792e40 100644
> --- a/drivers/input/touchscreen/usbtouchscreen.c
> +++ b/drivers/input/touchscreen/usbtouchscreen.c
> @@ -114,6 +114,7 @@ struct usbtouch_usb {
> struct input_dev *input;
> struct usbtouch_device_info *type;
> struct mutex pm_mutex; /* serialize access to open/suspend */
> + bool is_open;
> char name[128];
> char phys[64];
> void *priv;
> @@ -1466,6 +1467,7 @@ static int usbtouch_open(struct input_dev *input)
> }
>
> usbtouch->interface->needs_remote_wakeup = 1;
> + usbtouch->is_open = 1;
> out_put:
> mutex_unlock(&usbtouch->pm_mutex);
> usb_autopm_put_interface(usbtouch->interface);
> @@ -1481,6 +1483,7 @@ static void usbtouch_close(struct input_dev *input)
> mutex_lock(&usbtouch->pm_mutex);
> if (!usbtouch->type->irq_always)
> usb_kill_urb(usbtouch->irq);
> + usbtouch->is_open = 0;
> mutex_lock(&usbtouch->pm_mutex);
>
> r = usb_autopm_get_interface(usbtouch->interface);
> @@ -1502,11 +1505,10 @@ static int usbtouch_suspend
> static int usbtouch_resume(struct usb_interface *intf)
> {
> struct usbtouch_usb *usbtouch = usb_get_intfdata(intf);
> - struct input_dev *input = usbtouch->input;
> int result = 0;
>
> mutex_lock(&usbtouch->pm_mutex);
> - if (input->users || usbtouch->type->irq_always)
> + if (usbtouch->is_open || usbtouch->type->irq_always)
> result = usb_submit_urb(usbtouch->irq, GFP_NOIO);
> mutex_unlock(&usbtouch->pm_mutex);
>
> @@ -1516,7 +1518,6 @@ static int usbtouch_resume(struct usb_interface *intf)
> static int usbtouch_reset_resume(struct usb_interface *intf)
> {
> struct usbtouch_usb *usbtouch = usb_get_intfdata(intf);
> - struct input_dev *input = usbtouch->input;
> int err = 0;
>
> /* reinit the device */
> @@ -1532,7 +1533,7 @@ static int usbtouch_reset_resume(struct usb_interface *intf)
>
> /* restart IO if needed */
> mutex_lock(&usbtouch->pm_mutex);
> - if (input->users)
> + if (usbtouch->is_open)
> err = usb_submit_urb(usbtouch->irq, GFP_NOIO);
> mutex_unlock(&usbtouch->pm_mutex);
>
> --
> 2.16.2
>
--
Dmitry
^ permalink raw reply
* Re: [PATCH v5 0/2] Remove false-positive VLAs when using max()
From: Linus Torvalds @ 2018-03-17 18:52 UTC (permalink / raw)
To: Kees Cook
Cc: Al Viro, Florian Weimer, Andrew Morton, Josh Poimboeuf,
Rasmus Villemoes, Randy Dunlap, Miguel Ojeda, Ingo Molnar,
David Laight, Ian Abbott, linux-input, linux-btrfs,
Network Development, Linux Kernel Mailing List, Kernel Hardening
In-Reply-To: <CAGXu5jJmAAgaq3VCsUN2yjpHLSc7P-kNrM_4c4cqP=ysFcETXA@mail.gmail.com>
On Sat, Mar 17, 2018 at 12:27 AM, Kees Cook <keescook@chromium.org> wrote:
>
> Unfortunately my 4.4 test fails quickly:
>
> ./include/linux/jiffies.h: In function ‘jiffies_delta_to_clock_t’:
> ./include/linux/jiffies.h:444: error: first argument to
> ‘__builtin_choose_expr’ not a constant
Ok, so it really looks like that same "__builtin_constant_p() doesn't
return a constant".
Which is really odd, but there you have it.
I wonder if you can use that "sizeof()" to force evaluation of it,
because sizeof() really does end up being magical when it comes to
"integer constant expression".
So instead of this:
#define __no_side_effects(a,b) \
(__builtin_constant_p(a)&&__builtin_constant_p(b))
that just assumes that __builtin_constant_p() itself always counts as
a constant expression, what happens if you do
#define __is_constant(a) \
(sizeof(char[__builtin_constant_p(a)]))
#define __no_side_effects(a,b) \
(__is_constant(a) && __is_constant(b))
I realize that the above looks completely insane: the whole point is
to *not* have VLA's, and we know that __builtin_constant_p() isn't
always evaliated as a constant.
But hear me out: if the issue is that there's some evaluation ordering
between the two builtins, and the problem is that the
__builtin_choose_expr() part of the expression is expanded *before*
the __builtin_constant_p() has been expanded, then just hiding it
inside that bat-shit-crazy sizeof() will force that to be evaluated
first (because a sizeof() is defined to be a integer constant
expression.
So the above is completely insane, bit there is actually a chance that
using that completely crazy "x -> sizeof(char[x])" conversion actually
helps, because it really does have a (very odd) evaluation-time
change. sizeof() has to be evaluated as part of the constant
expression evaluation, in ways that "__builtin_constant_p()" isn't
specified to be done.
But it is also definitely me grasping at straws. If that doesn't work
for 4.4, there's nothing else I can possibly see.
Linus
^ permalink raw reply
* Re: [PATCH v5 0/2] Remove false-positive VLAs when using max()
From: Kees Cook @ 2018-03-17 20:07 UTC (permalink / raw)
To: Linus Torvalds
Cc: Al Viro, Florian Weimer, Andrew Morton, Josh Poimboeuf,
Rasmus Villemoes, Randy Dunlap, Miguel Ojeda, Ingo Molnar,
David Laight, Ian Abbott, linux-input, linux-btrfs,
Network Development, Linux Kernel Mailing List, Kernel Hardening
In-Reply-To: <CA+55aFwv0VC67cx2W9yrf7qSFKf6ncVRrsVyLGqdmfSBn4yTYw@mail.gmail.com>
On Sat, Mar 17, 2018 at 11:52 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> So the above is completely insane, bit there is actually a chance that
> using that completely crazy "x -> sizeof(char[x])" conversion actually
> helps, because it really does have a (very odd) evaluation-time
> change. sizeof() has to be evaluated as part of the constant
> expression evaluation, in ways that "__builtin_constant_p()" isn't
> specified to be done.
>
> But it is also definitely me grasping at straws. If that doesn't work
> for 4.4, there's nothing else I can possibly see.
No luck! :( gcc 4.4 refuses to play along. And, hilariously, not only
does it not change the complaint about __builtin_choose_expr(), it
also thinks that's a VLA now.
./include/linux/mm.h: In function ‘get_mm_hiwater_rss’:
./include/linux/mm.h:1567: warning: variable length array is used
./include/linux/mm.h:1567: error: first argument to
‘__builtin_choose_expr’ not a constant
6.8 is happy with it (of course).
I do think the earlier version (without the
sizeof-hiding-builting_constant_p) provides a template for a
const_max() that both you and Rasmus would be happy with, though!
-Kees
--
Kees Cook
Pixel Security
^ permalink raw reply
* Re: [PATCH 01/14] Input: atmel_mxt_ts - do not pass suspend mode in platform data
From: Nick Dyer @ 2018-03-17 20:42 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input, Benson Leung, Olof Johansson, linux-kernel
In-Reply-To: <20180317174240.GC183240@dtor-ws>
On Sat, Mar 17, 2018 at 10:42:40AM -0700, Dmitry Torokhov wrote:
> On Fri, Mar 16, 2018 at 08:40:02PM +0000, Nick Dyer wrote:
> > On Thu, Mar 15, 2018 at 04:56:21PM -0700, Dmitry Torokhov wrote:
> > > Ah, OK, I see. I would really like to drop this
> > > pdata->suspend_mode stuff and I do not want to create
> > > "pixel-screwed-up" property either... I guess for the time being
> > > I'll put a DMI quirk for Link to restore T9 control method, and
> > > then look into cleaning it all up. We have quite a bit different
> > > code in chromeos kernel trees and I'd like to reconcile
> > > it.
> >
> > Yes, it would be great to get rid of it. The driver does have the
> > ability to download configuration via the firmware loader interface.
> > So you would be able to grab a copy of the config by saving it with
> > mxt-app, tweak it to ensure that the T9 CTRL byte is set correctly,
> > then ship it somehow (presumably it could be added to
> > linux-firmware). This would override what's currently stored in
> > NVRAM on all those units and mean we could remove the T9_CTRL stuff.
>
> We can't really rely on people fetching updated config. Do you think we
> could see if the device has only T9 and not T100 and if coming out of
> suspend the T9 CTRL byte is 0 we overwrite it with the 0x83?
I think that all we need to do is add something to
mxt_read_t9_resolution (and probably rename it to mxt_init_t9_config)
that reads the 1st (CTRL) byte, and if it's zero, writes 0x83 (and
probably a dev_dbg() wouldn't go amiss)
Also call the same logic on reset (look for "Detect reset"), because
that wipes out the config.
Once we've done that, we can get rid of the MXT_SUSPEND_T9_CTRL and use
the normal T7 power up/down logic for suspend/resume on all devices.
FWIW there may be two instances of T9, but I've never seen a device that
actually had two screens and it's not supported really anyway with this
driver.
N
^ permalink raw reply
* Re: [PATCH v5 0/4] new driver for Valve Steam Controller
From: Pierre-Loup A. Griffais @ 2018-03-17 21:54 UTC (permalink / raw)
To: Rodrigo Rivas Costa, Benjamin Tissoires
Cc: Clément VUCHENER, Jiri Kosina, Cameron Gutman, lkml,
linux-input
In-Reply-To: <20180315210659.GA16037@casa>
On 03/15/2018 02:06 PM, Rodrigo Rivas Costa wrote:
> On Wed, Mar 14, 2018 at 05:39:25PM +0100, Benjamin Tissoires wrote:
>> On Mon, Mar 12, 2018 at 9:51 PM, Rodrigo Rivas Costa
>> <rodrigorivascosta@gmail.com> wrote:
>>> On Mon, Mar 12, 2018 at 03:30:43PM +0100, Clément VUCHENER wrote:
>>>> 2018-03-11 20:58 GMT+01:00 Rodrigo Rivas Costa <rodrigorivascosta@gmail.com>:
>>>>> This patchset implements a driver for Valve Steam Controller, based on a
>>>>> reverse analysis by myself.
>>>>>
>>>>> Sorry, I've been out of town for a few weeks and couldn't keep up with this...
>>>>>
>>>>> @Pierre-Loup and @Clément, could you please have another look at this and
>>>>> check if it is worthy? Benjamin will not commit it without an express ACK from
>>>>> Valve. Of course he is right to be cautious, but I checked this driver with
>>>>> the Steam Client and all seems to go just fine. I think that there is a lot of
>>>>> Linux out of the desktop that could use this driver and cannot use the Steam
>>>>> Client. Worst case scenario, this driver can now be blacklisted, but I hope
>>>>> that will not be needed.
>>>>
>>>> I tested the driver with my 4.15 fedora kernel (I only built the
>>>> module not the whole kernel) and I got double inputs (your driver
>>>> input device + steam uinput device) when testing Shovel Knight with
>>>> Steam Big Picture. It seems to work fine when the inputs are the same,
>>>> but after changing the controller configuration in Steam, the issue
>>>> became apparent.
>>>
>>> I assumed that when several joysticks are available, games would listen
>>> to one one of them. It looks like I'm wrong, and some (many?) games will
>>> listen to all available joysticks at the same time. Thus having two
>>> logical joysticks that represent the same physical one is not good.
>>
>> Yeah, the general rule of thumb is "think of the worst thing that can
>> happen, someone will do something worst".
>>
>>>
>>> An easy solution would be that Steam Client grabs this driver
>>> (ioctl(EVIOCGRAB)) when creating the uinput device. Another solution
>>> would be that Steam Client blacklists this driver, of course.
>>
>> This is 2 solutions that rely on a userspace change, and this is not
>> acceptable in its current form. What if people do not upgrade Steam
>> client but upgrade their kernel? Well, Steam might be able to force
>> people to always run the latest shiny available version, but for other
>> games, you can't expect people to have a compatible version of the
>> userspace stack.
>
> Well, if you don't have Steam then you don't have the double input in
> the first place. Unless you are using a different user-mode driver, of
> course.
>>
>> Also, "blacklisting the driver" from Steam client is something the OS
>> can do, but not the client when you run on a different distribution.
>> You need root for that, and I don't want to give root permissions to
>> Steam (or to any user space client that shouldn't have root privileges
>> for what it matters).
>
> Actually Steam needs a system installation that adds udev rules to grant
> uaccess to /dev/uinput and the USB raw device for the controller.
> Adding a /etc/modprobe.d/steam.conf should be possible, too. It would be
> a bit inconvenient because you'll need a distro update of the steam
> package, not just the usual user-mode-only auto-update.
It's definitely a bit tricky; we've rolled out an update to our
reference package whenever we've added support for new devices (the
final Steam Controller, direct PS4 gamepad led/gyro access through HID,
HTC Vive and its myriad of onboard devices, bootloaders of all these
things for firmware updates, etc). Whenever we have to do that, the
rollout never is as smooth as desired: many users aren't using our own
package; even on the distributions we support directly. Then downstream
distributions adopt these udev changes with various delays (sometimes
never), and port them to their own mechanism of doing things, since
everyone has their own idea of a robust security model. I wish local
sessions always had proper access to HID devices connected to the
physical computer the user is sitting at, but I realize that the basic
gaming desktop is just one of many usecases distros out there have to
support and it'd be unreasonable to expect them to focus exclusively on it.
>
>>>
>>>> And without Steam and your external tool, you get double inputs too. I
>>>> tried RetroArch and it was unusable because of the keyboard inputs
>>>> from the lizard mode (e.g. pressing B also presses Esc and quits
>>>> RetroArch). Having to download and compile an external tool to make
>>>> the driver work properly may be too difficult for the user. Your goal
>>>> was to provide an alternative to user space drivers but now you
>>>> actually depend on (a very simple) one.
>>>
>>> Yes, I noticed that. TBH, this driver without Steam Client or the
>>> user-space tool is not very nice, precisely because you'll get constant
>>> Escape and Enter presses, and most games react to those.
>>>
>>> Frankly speaking, I'm not sure how to proceed. I can think of the
>>> following options:
>>> 1.Steam Client installation could add a file to blacklist
>>> hid-steam, just as it adds a few udev rules.
>>
>> But what about RetroArch? And what if you install Steam but want to
>> play SDL games that could benefit from your driver?
>
> That is an issue of solution 1. I actually have the module blacklisted
> in my PC, and run `sudo modprobe hid-steam` to use SDL.
>
>>> 2.The default CONFIG_HID_STEAM can be changed to "n". Maybe only
>>> on the architectures for which there is a Steam Client available.
>>> This way DIY projects will still be able to use it.
>>
>> But this will make the decision to include or not the driver in
>> distributions harder. And if no distribution uses it, you won't have
>> free tests, and you will be alone to maintain it. So that's not ideal
>> either
>
> Could we set the default to 'y' in non-PC systems. It would be enabled
> in my Raspbian, for example... better than nothing.
>>
>>> 3.This driver could be abandoned :-(. Just use Steam Client if possible or
>>> any of the user-mode drivers available.
>>
>> This would be a waste for everybody as it's always better when we share.
>
> Indeed!
>
> I tried a new option:
> 4. The driver detects whether the DEVUSB/HIDRAW device is in use, and
> if that is the case it will disable itself. If the DEVUSB/HIDRAW is
> not in use, then the driver will work normally. A bit hackish maybe
> but it should work.
>
> I tried doing this option 4, but I'm unable to do it properly. I don't
> even know if it is possible...
>
>>>
>>> If we decide for 1 or 2, then the lizard mode could be disabled without
>>> ill effects. We could even enable the gyro and all the other gadgets
>>> without worring about current compatibility.
>>
>> To me, 1 is out of the question. The kernel can't expect a user space
>> change especially if you are knowingly introducing a bug for the end
>> user.
>>
>> 2 is still on the table IMO, and 3 would be a shame.
>>
>> I know we already discussed about sysfs and module parameters, but if
>> the driver will conflict with a userspace stack, the only way would be
>> to have a (dynamic) parameter "enhanced_mode" or
>> "kernel_awesome_support" or whatever which would be a boolean, that
>> defaults to false that Steam can eventually lookup if they want so in
>> the future we can default it to true. When this parameter is set, the
>> driver will create the inputs and toggle the various modes, while when
>> it's toggled off, it'll clean up itself and keep the device as if it
>> were connected to hid-generic. Bonus point, this removes the need for
>> the simple user space tool that enables the mode.
>
> That is doable, but that sysfs/parameter can be changed by a non-root
> user? I looked for a udev rule to grant access to those but found
> nothing.
>
> IIUC, when this parameter is false the driver will do nothing, right?
> The user will just need to change it to true to be able to use it, but
> that will have to be done by root.
>
> I'll try doing this, but I'd appreciate your advice about what approach
> would be better: sysfs? a module parameter? a cdev? or even a EV_MSC?
>
>>> At the end of the day, I think that it is up to Valve what to do.
>>
>> Again, Valve is a big player here, but do not underestimate other
>> projects (like RetroArch mentioned above) because if you break their
>> workflow, they will have the right to request a revert of the commit
>> because it breaks some random user playing games in the far end of
>> Antarctica (yes, penguins do love to play games :-P )
>
> And everybody loves penguins! If we take away Steam (say a RaspberryPi
> as a canonical example) and disable the lizard mode, then this driver is
> just a regular gamepad. RetroArch should be happy with that. Unless they
> already have an user mode driver for the steam-controller, of course...
Both of these things seem reasonable to me, with a few caveats:
- If there's an opt-in mechanism available, it would be good to ensure
we have a way to reliably query its state without requiring extra
permissions. This way, if we know it's likely to affect Steam client
functionality, we'll have the right mechanism to properly message the
situation to users.
- If you find a way for the client to be able to program an opt out
when it's running that is not automatic (eg. by detecting the hidraw
device being opened), we'd be happy to participate with that scheme
assuming it doesn't require extra permissions. As soon as the API is
figured out, we can include it in the client, just send me a heads-up.
The one thing that I'd be cautious of is robust behavior against
abnormal client termination. If it's a sysfs entry we have to poke, I'm
worried that if the client crashes we might not always be able to opt
the driver back out. It'd be nice if it was based on opening an fd
instead, this way the kernel would robustly clean up after us and your
driver would kick back in.
Note that there's a general desire on our side to create a reference
userspace implementation that would more or less have the current
functionality of the Steam client, but would be easily usable from other
platforms where the client doesn't currentl run. Unfortunately it's
quite a bit of work, so it's unclear what the timeframe would be, if it
ever does happen.
Thanks,
- Pierre-Loup
>
> Best regards.
> Rodrigo
>
>> Cheers,
>> Benjamin
>>
>>> Best Regards.
>>> Rodrigo.
>>>
>>>> Also the button and axis codes do not match the gamepad API doc
>>>> (https://www.kernel.org/doc/Documentation/input/gamepad.txt).
>>>>
>>>>>
>>>>> For full reference, I'm adding a full changelog of this patchset.
>>>>>
>>>>> Changes in v5:
>>>>> * Fix license SPDX to GPL-2.0+.
>>>>> * Minor stylistic changes (BIT(3) instead 0x08 and so on).
>>>>>
>>>>> Changes in v4:
>>>>> * Add command to check the wireless connection status on probe, without
>>>>> waiting for a message (thanks to Clément Vuchener for the tip).
>>>>> * Removed the error code on redundant connection/disconnection messages. That
>>>>> was harmless but polluted dmesg.
>>>>> * Added buttons for touching the left-pad and right-pad.
>>>>> * Fixed a misplaced #include from 2/4 to 1/4.
>>>>>
>>>>> Changes in v3:
>>>>> * Use RCU to do the dynamic connec/disconnect of wireless devices.
>>>>> * Remove entries in hid-quirks.c as they are no longer needed. This allows
>>>>> this module to be blacklisted without side effects.
>>>>> * Do not bypass the virtual keyboard/mouse HID devices to avoid breaking
>>>>> existing use cases (lizard mode). A user-space tool to do that is
>>>>> linked.
>>>>> * Fully separated axes for joystick and left-pad. As it happens.
>>>>> * Add fuzz values for left/right pad axes, they are a little wiggly.
>>>>>
>>>>> Changes in v2:
>>>>> * Remove references to USB. Now the interesting interfaces are selected by
>>>>> looking for the ones with feature reports.
>>>>> * Feature reports buffers are allocated with hid_alloc_report_buf().
>>>>> * Feature report length is checked, to avoid overflows in case of
>>>>> corrupt/malicius USB devices.
>>>>> * Resolution added to the ABS axes.
>>>>> * A lot of minor cleanups.
>>>>>
>>>>> Rodrigo Rivas Costa (4):
>>>>> HID: add driver for Valve Steam Controller
>>>>> HID: steam: add serial number information.
>>>>> HID: steam: command to check wireless connection
>>>>> HID: steam: add battery device.
>>>>>
>>>>> drivers/hid/Kconfig | 8 +
>>>>> drivers/hid/Makefile | 1 +
>>>>> drivers/hid/hid-ids.h | 4 +
>>>>> drivers/hid/hid-steam.c | 794 ++++++++++++++++++++++++++++++++++++++++++++++++
>>>>> 4 files changed, 807 insertions(+)
>>>>> create mode 100644 drivers/hid/hid-steam.c
>>>>>
>>>>> --
>>>>> 2.16.2
>>>>>
>
^ permalink raw reply
* Re: [PATCH v5 0/2] Remove false-positive VLAs when using max()
From: Josh Poimboeuf @ 2018-03-17 22:55 UTC (permalink / raw)
To: Kees Cook
Cc: Linus Torvalds, Al Viro, Florian Weimer, Andrew Morton,
Rasmus Villemoes, Randy Dunlap, Miguel Ojeda, Ingo Molnar,
David Laight, Ian Abbott, linux-input, linux-btrfs,
Network Development, Linux Kernel Mailing List, Kernel Hardening
In-Reply-To: <CAGXu5jJ=ZYpf=30H6hsWn-R-CEVYAgVMHxjmoLUC00QYq0r17g@mail.gmail.com>
On Sat, Mar 17, 2018 at 01:07:32PM -0700, Kees Cook wrote:
> On Sat, Mar 17, 2018 at 11:52 AM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> > So the above is completely insane, bit there is actually a chance that
> > using that completely crazy "x -> sizeof(char[x])" conversion actually
> > helps, because it really does have a (very odd) evaluation-time
> > change. sizeof() has to be evaluated as part of the constant
> > expression evaluation, in ways that "__builtin_constant_p()" isn't
> > specified to be done.
> >
> > But it is also definitely me grasping at straws. If that doesn't work
> > for 4.4, there's nothing else I can possibly see.
>
> No luck! :( gcc 4.4 refuses to play along. And, hilariously, not only
> does it not change the complaint about __builtin_choose_expr(), it
> also thinks that's a VLA now.
>
> ./include/linux/mm.h: In function ‘get_mm_hiwater_rss’:
> ./include/linux/mm.h:1567: warning: variable length array is used
> ./include/linux/mm.h:1567: error: first argument to
> ‘__builtin_choose_expr’ not a constant
>
> 6.8 is happy with it (of course).
>
> I do think the earlier version (without the
> sizeof-hiding-builting_constant_p) provides a template for a
> const_max() that both you and Rasmus would be happy with, though!
I thought we were dropping support for 4.4 (for other reasons). Isn't
it 4.6 we should be looking at?
--
Josh
^ permalink raw reply
* Re: [PATCH v5 0/2] Remove false-positive VLAs when using max()
From: Rasmus Villemoes @ 2018-03-18 21:13 UTC (permalink / raw)
To: Linus Torvalds, Kees Cook
Cc: Al Viro, Florian Weimer, Andrew Morton, Josh Poimboeuf,
Rasmus Villemoes, Randy Dunlap, Miguel Ojeda, Ingo Molnar,
David Laight, Ian Abbott, linux-input, linux-btrfs,
Network Development, Linux Kernel Mailing List, Kernel Hardening
In-Reply-To: <CA+55aFwv0VC67cx2W9yrf7qSFKf6ncVRrsVyLGqdmfSBn4yTYw@mail.gmail.com>
On 2018-03-17 19:52, Linus Torvalds wrote:
> On Sat, Mar 17, 2018 at 12:27 AM, Kees Cook <keescook@chromium.org> wrote:
>>
>> Unfortunately my 4.4 test fails quickly:
>>
>> ./include/linux/jiffies.h: In function ‘jiffies_delta_to_clock_t’:
>> ./include/linux/jiffies.h:444: error: first argument to
>> ‘__builtin_choose_expr’ not a constant
>
> Ok, so it really looks like that same "__builtin_constant_p() doesn't
> return a constant".
>
> Which is really odd, but there you have it.
Not really. We do rely on builtin_constant_p not being folded too
quickly to a 0/1 answer, so that gcc still generates good code even if
the argument is only known to be constant at a late(r) optimization
stage (through inlining and all). So unlike types_compatible_p, which
can obviously be answered early during parsing, builtin_constant_p is
most of the time a yes/no/maybe/it's complicated thing. Sure, when the
argument is just a literal or perhaps even any kind of ICE, gcc can fold
it to "yes", and I think it does (though the details of when and if gcc
does that can obviously be very version-dependent, which may be some of
what we've seen). But when it's not that obvious, gcc leaves it in the
undetermined state. That's not good enough for builtin_choose_expr,
because even the type of the resulting expression depends on that first
argument, so that really must be resolved early.
So to have some kind of builtin_constant_p control a
builtin_choose_expr, it would need to be a "builtin_ice_p" or
"builtin_obviously_constant_p" that would always be folded to 0/1 as
part of evaluating ICEs.
So I don't think there's any way around creating a separate macro for
use with compile-time constants.
Rasmus
^ permalink raw reply
* Re: [PATCH v5 0/2] Remove false-positive VLAs when using max()
From: Linus Torvalds @ 2018-03-18 21:33 UTC (permalink / raw)
To: Rasmus Villemoes
Cc: Kees Cook, Al Viro, Florian Weimer, Andrew Morton, Josh Poimboeuf,
Randy Dunlap, Miguel Ojeda, Ingo Molnar, David Laight, Ian Abbott,
linux-input, linux-btrfs, Network Development,
Linux Kernel Mailing List, Kernel Hardening
In-Reply-To: <42b4342b-aefc-a16a-0d43-9f9c0d63ba7a@rasmusvillemoes.dk>
On Sun, Mar 18, 2018 at 2:13 PM, Rasmus Villemoes
<linux@rasmusvillemoes.dk> wrote:
> On 2018-03-17 19:52, Linus Torvalds wrote:
>>
>> Ok, so it really looks like that same "__builtin_constant_p() doesn't
>> return a constant".
>>
>> Which is really odd, but there you have it.
>
> Not really. We do rely on builtin_constant_p not being folded too
> quickly to a 0/1 answer, so that gcc still generates good code even if
> the argument is only known to be constant at a late(r) optimization
> stage (through inlining and all).
Hmm. That makes sense. It just doesn't work for our case when we
really want to choose the expression based on side effects or not.
> So unlike types_compatible_p, which
> can obviously be answered early during parsing, builtin_constant_p is
> most of the time a yes/no/maybe/it's complicated thing.
The silly thing is, the thing we *really* want to know _is_ actually
easily accessible during the early parsing, exactly like
__builtin_compatible_p(): it's not really that we care about the
expressions being constant, as much as the "can this have side
effects" question.
We only really use __builtin_constant_p() as a (bad) approximation of
that in this case, since it's the best we can do.
So the real use would be to say "can we use the simple direct macro
that just happens to also fold into a nice integer constant
expression" for __builtin_choose_expr().
I tried to find something like that, but it really doesn't exist, even
though I would actually have expected it to be a somewhat common
concern for macro writers: write a macro that works in any arbitrary
environment.
I guess array sizes are really the only true limiting environment
(static initializers is another one).
How annoying. Odd that newer gcc's seem to do so much better (ie gcc-5
seems to be fine). So _something_ must have changed.
Linus
^ permalink raw reply
* Re: [PATCH v5 3/4] clk: lpc32xx: Set name of regmap_config
From: Vladimir Zapolskiy @ 2018-03-18 21:57 UTC (permalink / raw)
To: Jeffy Chen, linux-kernel, broonie, alexandre.belloni
Cc: Kate Stewart, Mark Rutland, Emil Renner Berthing, stephen lu,
Catalin Marinas, Michael Turquette, Will Deacon,
Alexandre Belloni, Joseph Lo, lee.jones, linux-clk,
Heiko Stuebner, Brian Norris, linux-rockchip, Matthias Kaehlcke,
linux-input, Arvind Yadav, Jerome Brunet, Guenter Roeck,
devicetree, Arnd Bergmann, Sylvain Lemieux, Rob Herring,
Thomas Gleixner
In-Reply-To: <20180312095142.30854-4-jeffy.chen@rock-chips.com>
Hi Jeffy,
On 03/12/2018 11:51 AM, Jeffy Chen wrote:
> We are now allowing to register debugfs without a valid device, and not
> having a valid name will end up using "dummy*" to create debugfs dir.
>
> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
> ---
>
> Changes in v5: None
> Changes in v4: None
> Changes in v3: None
>
> drivers/clk/nxp/clk-lpc32xx.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/clk/nxp/clk-lpc32xx.c b/drivers/clk/nxp/clk-lpc32xx.c
> index f5d815f577e0..a2a0a7f3bc57 100644
> --- a/drivers/clk/nxp/clk-lpc32xx.c
> +++ b/drivers/clk/nxp/clk-lpc32xx.c
> @@ -67,6 +67,7 @@
> #define LPC32XX_USB_CLK_STS 0xF8
>
> static struct regmap_config lpc32xx_scb_regmap_config = {
> + .name = "lpc32xx-scb",
please rename it to "scb", LPC32xx SoC name is already known from the context.
When it's done, feel free to add to the next version my
Acked-by: Vladimir Zapolskiy <vz@mleia.com>
> .reg_bits = 32,
> .val_bits = 32,
> .reg_stride = 4,
>
Thank you for the change.
--
With best wishes,
Vladimir
^ 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