[PATCH] input: uinput: Drop checks for abs_min > abs_max

[PATCH] input: uinput: Drop checks for abs_min > abs_max

[Chris Morgan]

From: Chris Morgan <macromorgan@hotmail.com>

Stop checking if the minimum abs value is greater than the maximum abs
value. When the axis is inverted this condition is allowed. Without
relaxing this check, it is not possible to use uinput on devices in
userspace with an inverted axis, such as the adc-joystick found on
many handheld gaming devices.

Signed-off-by: Chris Morgan <macromorgan@hotmail.com>
---
 drivers/input/misc/uinput.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
index d98212d55108..e90dbf2c0b34 100644
--- a/drivers/input/misc/uinput.c
+++ b/drivers/input/misc/uinput.c
@@ -403,14 +403,7 @@ static int uinput_validate_absinfo(struct input_dev *dev, unsigned int code,
 	min = abs->minimum;
 	max = abs->maximum;
 
-	if ((min != 0 || max != 0) && max < min) {
-		printk(KERN_DEBUG
-		       "%s: invalid abs[%02x] min:%d max:%d\n",
-		       UINPUT_NAME, code, min, max);
-		return -EINVAL;
-	}
-
-	if (!check_sub_overflow(max, min, &range) && abs->flat > range) {
+	if (!check_sub_overflow(max, min, &range) && abs->flat > abs(range)) {
 		printk(KERN_DEBUG
 		       "%s: abs_flat #%02x out of range: %d (min:%d/max:%d)\n",
 		       UINPUT_NAME, code, abs->flat, min, max);
-- 
2.34.1

Re: [PATCH] input: uinput: Drop checks for abs_min > abs_max

[Peter Hutterer]

On Mon, Dec 18, 2023 at 11:16:53AM -0600, Chris Morgan wrote:
> From: Chris Morgan <macromorgan@hotmail.com>
> 
> Stop checking if the minimum abs value is greater than the maximum abs
> value. When the axis is inverted this condition is allowed. Without
> relaxing this check, it is not possible to use uinput on devices in
> userspace with an inverted axis, such as the adc-joystick found on
> many handheld gaming devices.

As mentioned in the other thread [1] a fair bit of userspace relies on
that general assumption so removing it will likely cause all sorts of
issues.

Cheers,
   Petre

[1] https://lore.kernel.org/linux-input/20231219234803.GA3396969@quokka/T/#t
> 
> Signed-off-by: Chris Morgan <macromorgan@hotmail.com>
> ---
>  drivers/input/misc/uinput.c | 9 +--------
>  1 file changed, 1 insertion(+), 8 deletions(-)
> 
> diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
> index d98212d55108..e90dbf2c0b34 100644
> --- a/drivers/input/misc/uinput.c
> +++ b/drivers/input/misc/uinput.c
> @@ -403,14 +403,7 @@ static int uinput_validate_absinfo(struct input_dev *dev, unsigned int code,
>  	min = abs->minimum;
>  	max = abs->maximum;
>  
> -	if ((min != 0 || max != 0) && max < min) {
> -		printk(KERN_DEBUG
> -		       "%s: invalid abs[%02x] min:%d max:%d\n",
> -		       UINPUT_NAME, code, min, max);
> -		return -EINVAL;
> -	}
> -
> -	if (!check_sub_overflow(max, min, &range) && abs->flat > range) {
> +	if (!check_sub_overflow(max, min, &range) && abs->flat > abs(range)) {
>  		printk(KERN_DEBUG
>  		       "%s: abs_flat #%02x out of range: %d (min:%d/max:%d)\n",
>  		       UINPUT_NAME, code, abs->flat, min, max);
> -- 
> 2.34.1
> 
> 

Re: [PATCH] input: uinput: Drop checks for abs_min > abs_max

[Paul Cercueil]

Hi Peter,

Le mercredi 20 décembre 2023 à 09:51 +1000, Peter Hutterer a écrit :
> On Mon, Dec 18, 2023 at 11:16:53AM -0600, Chris Morgan wrote:
> > From: Chris Morgan <macromorgan@hotmail.com>
> > 
> > Stop checking if the minimum abs value is greater than the maximum
> > abs
> > value. When the axis is inverted this condition is allowed. Without
> > relaxing this check, it is not possible to use uinput on devices in
> > userspace with an inverted axis, such as the adc-joystick found on
> > many handheld gaming devices.
> 
> As mentioned in the other thread [1] a fair bit of userspace relies
> on
> that general assumption so removing it will likely cause all sorts of
> issues.

There is some userspace that works with it though, so why restrict it
artificially?

The fact that some other userspace code wouldn't work with it sounds a
bit irrelevant. They just never encountered that min>max usage before.

And removing this check won't cause all sort of issues, why would it?
It's not like the current software actively probes min>max and crash
badly if it doesn't return -EINVAL...

Cheers,
-Paul

> 
> Cheers,
>    Petre
> 
> [1]
> https://lore.kernel.org/linux-input/20231219234803.GA3396969@quokka/T/#t
> > 
> > Signed-off-by: Chris Morgan <macromorgan@hotmail.com>
> > ---
> >  drivers/input/misc/uinput.c | 9 +--------
> >  1 file changed, 1 insertion(+), 8 deletions(-)
> > 
> > diff --git a/drivers/input/misc/uinput.c
> > b/drivers/input/misc/uinput.c
> > index d98212d55108..e90dbf2c0b34 100644
> > --- a/drivers/input/misc/uinput.c
> > +++ b/drivers/input/misc/uinput.c
> > @@ -403,14 +403,7 @@ static int uinput_validate_absinfo(struct
> > input_dev *dev, unsigned int code,
> >  	min = abs->minimum;
> >  	max = abs->maximum;
> >  
> > -	if ((min != 0 || max != 0) && max < min) {
> > -		printk(KERN_DEBUG
> > -		       "%s: invalid abs[%02x] min:%d max:%d\n",
> > -		       UINPUT_NAME, code, min, max);
> > -		return -EINVAL;
> > -	}
> > -
> > -	if (!check_sub_overflow(max, min, &range) && abs->flat >
> > range) {
> > +	if (!check_sub_overflow(max, min, &range) && abs->flat >
> > abs(range)) {
> >  		printk(KERN_DEBUG
> >  		       "%s: abs_flat #%02x out of range: %d
> > (min:%d/max:%d)\n",
> >  		       UINPUT_NAME, code, abs->flat, min, max);
> > -- 
> > 2.34.1
> > 
> > 

Re: [PATCH] input: uinput: Drop checks for abs_min > abs_max

[Dmitry Torokhov]

Hi Paul,

On Wed, Dec 20, 2023 at 01:38:39AM +0100, Paul Cercueil wrote:
> Hi Peter,
> 
> Le mercredi 20 décembre 2023 à 09:51 +1000, Peter Hutterer a écrit :
> > On Mon, Dec 18, 2023 at 11:16:53AM -0600, Chris Morgan wrote:
> > > From: Chris Morgan <macromorgan@hotmail.com>
> > > 
> > > Stop checking if the minimum abs value is greater than the maximum
> > > abs
> > > value. When the axis is inverted this condition is allowed. Without
> > > relaxing this check, it is not possible to use uinput on devices in
> > > userspace with an inverted axis, such as the adc-joystick found on
> > > many handheld gaming devices.
> > 
> > As mentioned in the other thread [1] a fair bit of userspace relies
> > on
> > that general assumption so removing it will likely cause all sorts of
> > issues.
> 
> There is some userspace that works with it though, so why restrict it
> artificially?
> 
> The fact that some other userspace code wouldn't work with it sounds a
> bit irrelevant. They just never encountered that min>max usage before.
> 
> And removing this check won't cause all sort of issues, why would it?
> It's not like the current software actively probes min>max and crash
> badly if it doesn't return -EINVAL...

It will cause weird movements because calculations expect min be the
minimum, and max the maximum, and not encode left/right or up/down.
Putting this into adc joystick binding was a mistake.

This is the definition of absinfo:

/**
 * struct input_absinfo - used by EVIOCGABS/EVIOCSABS ioctls
 * @value: latest reported value for the axis.
 * @minimum: specifies minimum value for the axis.
 * @maximum: specifies maximum value for the axis.
 * @fuzz: specifies fuzz value that is used to filter noise from
 *	the event stream.
 * @flat: values that are within this value will be discarded by
 *	joydev interface and reported as 0 instead.
 * @resolution: specifies resolution for the values reported for
 *	the axis.
 *
 * Note that input core does not clamp reported values to the
 * [minimum, maximum] limits, such task is left to userspace.
...
 */

(We should change wording of the last sentence to "... does not always
clamp ..." since when we do inversion/swap we do clamp values.)

And note that the userspace that can handle swapped min and max will
work fine if the kernel provides normalized data, but other software
will/may not work.

Thanks.

-- 
Dmitry

Re: [PATCH] input: uinput: Drop checks for abs_min > abs_max

[Paul Cercueil]

Hi Dmitry,

Le mardi 19 décembre 2023 à 17:53 -0800, Dmitry Torokhov a écrit :
> Hi Paul,
> 
> On Wed, Dec 20, 2023 at 01:38:39AM +0100, Paul Cercueil wrote:
> > Hi Peter,
> > 
> > Le mercredi 20 décembre 2023 à 09:51 +1000, Peter Hutterer a
> > écrit :
> > > On Mon, Dec 18, 2023 at 11:16:53AM -0600, Chris Morgan wrote:
> > > > From: Chris Morgan <macromorgan@hotmail.com>
> > > > 
> > > > Stop checking if the minimum abs value is greater than the
> > > > maximum
> > > > abs
> > > > value. When the axis is inverted this condition is allowed.
> > > > Without
> > > > relaxing this check, it is not possible to use uinput on
> > > > devices in
> > > > userspace with an inverted axis, such as the adc-joystick found
> > > > on
> > > > many handheld gaming devices.
> > > 
> > > As mentioned in the other thread [1] a fair bit of userspace
> > > relies
> > > on
> > > that general assumption so removing it will likely cause all
> > > sorts of
> > > issues.
> > 
> > There is some userspace that works with it though, so why restrict
> > it
> > artificially?
> > 
> > The fact that some other userspace code wouldn't work with it
> > sounds a
> > bit irrelevant. They just never encountered that min>max usage
> > before.
> > 
> > And removing this check won't cause all sort of issues, why would
> > it?
> > It's not like the current software actively probes min>max and
> > crash
> > badly if it doesn't return -EINVAL...
> 
> It will cause weird movements because calculations expect min be the
> minimum, and max the maximum, and not encode left/right or up/down.
> Putting this into adc joystick binding was a mistake.

I don't see why it was a mistake, it's only one of the ways to specify
that the axis is inverted. This information is between the firmware
(DT) and the kernel, that doesn't mean the information has to be
relayed as-is to the userspace.

Unlike what you wrote in your other answer, when talking about input
the kernel doesn't really normalize anything - it gives you the min/max
values, and the raw samples, not normalized samples (they don't get
translated to a pre-specified range, or even clamped).

I don't really like the idea of having the driver tamper with the
samples, but if the specification really is that max>min, then it would
be up to evdev/joydev (if the individual drivers are allowed min>max)
or adc-joystick (if they are not) to process the samples.

Cheers,
-Paul

> This is the definition of absinfo:
> 
> /**
>  * struct input_absinfo - used by EVIOCGABS/EVIOCSABS ioctls
>  * @value: latest reported value for the axis.
>  * @minimum: specifies minimum value for the axis.
>  * @maximum: specifies maximum value for the axis.
>  * @fuzz: specifies fuzz value that is used to filter noise from
>  *	the event stream.
>  * @flat: values that are within this value will be discarded by
>  *	joydev interface and reported as 0 instead.
>  * @resolution: specifies resolution for the values reported for
>  *	the axis.
>  *
>  * Note that input core does not clamp reported values to the
>  * [minimum, maximum] limits, such task is left to userspace.
> ...
>  */
> 
> (We should change wording of the last sentence to "... does not
> always
> clamp ..." since when we do inversion/swap we do clamp values.)
> 
> And note that the userspace that can handle swapped min and max will
> work fine if the kernel provides normalized data, but other software
> will/may not work.
> 
> Thanks.
> 

Re: [PATCH] input: uinput: Drop checks for abs_min > abs_max

[Chris Morgan]

On Wed, Dec 20, 2023 at 02:39:14PM +0100, Paul Cercueil wrote:
> Hi Dmitry,
> 
> Le mardi 19 décembre 2023 à 17:53 -0800, Dmitry Torokhov a écrit :
> > Hi Paul,
> > 
> > On Wed, Dec 20, 2023 at 01:38:39AM +0100, Paul Cercueil wrote:
> > > Hi Peter,
> > > 
> > > Le mercredi 20 décembre 2023 à 09:51 +1000, Peter Hutterer a
> > > écrit :
> > > > On Mon, Dec 18, 2023 at 11:16:53AM -0600, Chris Morgan wrote:
> > > > > From: Chris Morgan <macromorgan@hotmail.com>
> > > > > 
> > > > > Stop checking if the minimum abs value is greater than the
> > > > > maximum
> > > > > abs
> > > > > value. When the axis is inverted this condition is allowed.
> > > > > Without
> > > > > relaxing this check, it is not possible to use uinput on
> > > > > devices in
> > > > > userspace with an inverted axis, such as the adc-joystick found
> > > > > on
> > > > > many handheld gaming devices.
> > > > 
> > > > As mentioned in the other thread [1] a fair bit of userspace
> > > > relies
> > > > on
> > > > that general assumption so removing it will likely cause all
> > > > sorts of
> > > > issues.
> > > 
> > > There is some userspace that works with it though, so why restrict
> > > it
> > > artificially?
> > > 
> > > The fact that some other userspace code wouldn't work with it
> > > sounds a
> > > bit irrelevant. They just never encountered that min>max usage
> > > before.
> > > 
> > > And removing this check won't cause all sort of issues, why would
> > > it?
> > > It's not like the current software actively probes min>max and
> > > crash
> > > badly if it doesn't return -EINVAL...
> > 
> > It will cause weird movements because calculations expect min be the
> > minimum, and max the maximum, and not encode left/right or up/down.
> > Putting this into adc joystick binding was a mistake.
> 
> I don't see why it was a mistake, it's only one of the ways to specify
> that the axis is inverted. This information is between the firmware
> (DT) and the kernel, that doesn't mean the information has to be
> relayed as-is to the userspace.
> 
> Unlike what you wrote in your other answer, when talking about input
> the kernel doesn't really normalize anything - it gives you the min/max
> values, and the raw samples, not normalized samples (they don't get
> translated to a pre-specified range, or even clamped).
> 
> I don't really like the idea of having the driver tamper with the
> samples, but if the specification really is that max>min, then it would
> be up to evdev/joydev (if the individual drivers are allowed min>max)
> or adc-joystick (if they are not) to process the samples.
> 
> Cheers,
> -Paul

Forgive me for such a naive question, but what would it take to get a
bool of `inverted` added to the struct input_absinfo, and then an ioctl
to get the inverted status? I'm honestly not sure of the ramifications
of what this may do to userspace (good/bad/ugly?)

From what I am seeing thus far it sounds like we should not be setting
the values as inverted for now because it can cause problems. We can
either fix this in the adc-joystick driver, or my honest preference is
to just set the range as defined as min < max and add a comment stating
that the axis is inverted. If we can't handle reversed values
throughout the stack I don't see any reason to handle it special just
for the adc-joystick driver.

Thank you,
Chris

> 
> > This is the definition of absinfo:
> > 
> > /**
> >  * struct input_absinfo - used by EVIOCGABS/EVIOCSABS ioctls
> >  * @value: latest reported value for the axis.
> >  * @minimum: specifies minimum value for the axis.
> >  * @maximum: specifies maximum value for the axis.
> >  * @fuzz: specifies fuzz value that is used to filter noise from
> >  *	the event stream.
> >  * @flat: values that are within this value will be discarded by
> >  *	joydev interface and reported as 0 instead.
> >  * @resolution: specifies resolution for the values reported for
> >  *	the axis.
> >  *
> >  * Note that input core does not clamp reported values to the
> >  * [minimum, maximum] limits, such task is left to userspace.
> > ...
> >  */
> > 
> > (We should change wording of the last sentence to "... does not
> > always
> > clamp ..." since when we do inversion/swap we do clamp values.)
> > 
> > And note that the userspace that can handle swapped min and max will
> > work fine if the kernel provides normalized data, but other software
> > will/may not work.
> > 
> > Thanks.
> > 
> 

Re: [PATCH] input: uinput: Drop checks for abs_min > abs_max

[Peter Hutterer]

On Fri, Dec 22, 2023 at 11:09:46AM -0600, Chris Morgan wrote:
> On Wed, Dec 20, 2023 at 02:39:14PM +0100, Paul Cercueil wrote:
> > Hi Dmitry,
> > 
> > Le mardi 19 décembre 2023 à 17:53 -0800, Dmitry Torokhov a écrit :
> > > Hi Paul,
> > > 
> > > On Wed, Dec 20, 2023 at 01:38:39AM +0100, Paul Cercueil wrote:
> > > > Hi Peter,
> > > > 
> > > > Le mercredi 20 décembre 2023 à 09:51 +1000, Peter Hutterer a
> > > > écrit :
> > > > > On Mon, Dec 18, 2023 at 11:16:53AM -0600, Chris Morgan wrote:
> > > > > > From: Chris Morgan <macromorgan@hotmail.com>
> > > > > > 
> > > > > > Stop checking if the minimum abs value is greater than the
> > > > > > maximum
> > > > > > abs
> > > > > > value. When the axis is inverted this condition is allowed.
> > > > > > Without
> > > > > > relaxing this check, it is not possible to use uinput on
> > > > > > devices in
> > > > > > userspace with an inverted axis, such as the adc-joystick found
> > > > > > on
> > > > > > many handheld gaming devices.
> > > > > 
> > > > > As mentioned in the other thread [1] a fair bit of userspace
> > > > > relies
> > > > > on
> > > > > that general assumption so removing it will likely cause all
> > > > > sorts of
> > > > > issues.
> > > > 
> > > > There is some userspace that works with it though, so why restrict
> > > > it
> > > > artificially?
> > > > 
> > > > The fact that some other userspace code wouldn't work with it
> > > > sounds a
> > > > bit irrelevant. They just never encountered that min>max usage
> > > > before.
> > > > 
> > > > And removing this check won't cause all sort of issues, why would
> > > > it?
> > > > It's not like the current software actively probes min>max and
> > > > crash
> > > > badly if it doesn't return -EINVAL...
> > > 
> > > It will cause weird movements because calculations expect min be the
> > > minimum, and max the maximum, and not encode left/right or up/down.
> > > Putting this into adc joystick binding was a mistake.
> > 
> > I don't see why it was a mistake, it's only one of the ways to specify
> > that the axis is inverted. This information is between the firmware
> > (DT) and the kernel, that doesn't mean the information has to be
> > relayed as-is to the userspace.
> > 
> > Unlike what you wrote in your other answer, when talking about input
> > the kernel doesn't really normalize anything - it gives you the min/max
> > values, and the raw samples, not normalized samples (they don't get
> > translated to a pre-specified range, or even clamped).
> > 
> > I don't really like the idea of having the driver tamper with the
> > samples, but if the specification really is that max>min, then it would
> > be up to evdev/joydev (if the individual drivers are allowed min>max)
> > or adc-joystick (if they are not) to process the samples.
> > 
> > Cheers,
> > -Paul
> 
> Forgive me for such a naive question, but what would it take to get a
> bool of `inverted` added to the struct input_absinfo, and then an ioctl
> to get the inverted status? I'm honestly not sure of the ramifications
> of what this may do to userspace (good/bad/ugly?)

Perfectly valid question IMO :)

Unfortunately the struct is API and ABI, so it'll be a significant
effort. You need some negotiation with the kernel to know the size of
the struct (with our without that boolean) - that effectively requires a
new ioctl. Userspace will then need to handle both code paths (including
ifdefs) for the foreseeable future. And that's only the C layers, there
are plenty of other evdev consumers in python/rust/... that would all
need to adjust.

Assuming you can do all that, you then still have most of userspace that
doesn't know about this ioctl. And given how rare this situation seems
to be, it'll be quite a while until everything catches up.

Fixing it in the driver means userspace just works as-is. Which seems
like the better option ;)

Cheers,
  Peter


> From what I am seeing thus far it sounds like we should not be setting
> the values as inverted for now because it can cause problems. We can
> either fix this in the adc-joystick driver, or my honest preference is
> to just set the range as defined as min < max and add a comment stating
> that the axis is inverted. If we can't handle reversed values
> throughout the stack I don't see any reason to handle it special just
> for the adc-joystick driver.
> 
> Thank you,
> Chris
> 
> > 
> > > This is the definition of absinfo:
> > > 
> > > /**
> > >  * struct input_absinfo - used by EVIOCGABS/EVIOCSABS ioctls
> > >  * @value: latest reported value for the axis.
> > >  * @minimum: specifies minimum value for the axis.
> > >  * @maximum: specifies maximum value for the axis.
> > >  * @fuzz: specifies fuzz value that is used to filter noise from
> > >  *	the event stream.
> > >  * @flat: values that are within this value will be discarded by
> > >  *	joydev interface and reported as 0 instead.
> > >  * @resolution: specifies resolution for the values reported for
> > >  *	the axis.
> > >  *
> > >  * Note that input core does not clamp reported values to the
> > >  * [minimum, maximum] limits, such task is left to userspace.
> > > ...
> > >  */
> > > 
> > > (We should change wording of the last sentence to "... does not
> > > always
> > > clamp ..." since when we do inversion/swap we do clamp values.)
> > > 
> > > And note that the userspace that can handle swapped min and max will
> > > work fine if the kernel provides normalized data, but other software
> > > will/may not work.
> > > 
> > > Thanks.
> > > 
> > 

Re: [PATCH] input: uinput: Drop checks for abs_min > abs_max

[Hans de Goede]

Hi Paul,

On 12/20/23 14:39, Paul Cercueil wrote:
> Hi Dmitry,
> 
> Le mardi 19 décembre 2023 à 17:53 -0800, Dmitry Torokhov a écrit :
>> Hi Paul,
>>
>> On Wed, Dec 20, 2023 at 01:38:39AM +0100, Paul Cercueil wrote:
>>> Hi Peter,
>>>
>>> Le mercredi 20 décembre 2023 à 09:51 +1000, Peter Hutterer a
>>> écrit :
>>>> On Mon, Dec 18, 2023 at 11:16:53AM -0600, Chris Morgan wrote:
>>>>> From: Chris Morgan <macromorgan@hotmail.com>
>>>>>
>>>>> Stop checking if the minimum abs value is greater than the
>>>>> maximum
>>>>> abs
>>>>> value. When the axis is inverted this condition is allowed.
>>>>> Without
>>>>> relaxing this check, it is not possible to use uinput on
>>>>> devices in
>>>>> userspace with an inverted axis, such as the adc-joystick found
>>>>> on
>>>>> many handheld gaming devices.
>>>>
>>>> As mentioned in the other thread [1] a fair bit of userspace
>>>> relies
>>>> on
>>>> that general assumption so removing it will likely cause all
>>>> sorts of
>>>> issues.
>>>
>>> There is some userspace that works with it though, so why restrict
>>> it
>>> artificially?
>>>
>>> The fact that some other userspace code wouldn't work with it
>>> sounds a
>>> bit irrelevant. They just never encountered that min>max usage
>>> before.
>>>
>>> And removing this check won't cause all sort of issues, why would
>>> it?
>>> It's not like the current software actively probes min>max and
>>> crash
>>> badly if it doesn't return -EINVAL...
>>
>> It will cause weird movements because calculations expect min be the
>> minimum, and max the maximum, and not encode left/right or up/down.
>> Putting this into adc joystick binding was a mistake.
> 
> I don't see why it was a mistake, it's only one of the ways to specify
> that the axis is inverted. This information is between the firmware
> (DT) and the kernel, that doesn't mean the information has to be
> relayed as-is to the userspace.
> 
> Unlike what you wrote in your other answer, when talking about input
> the kernel doesn't really normalize anything - it gives you the min/max
> values, and the raw samples, not normalized samples (they don't get
> translated to a pre-specified range, or even clamped).
> 
> I don't really like the idea of having the driver tamper with the
> samples, but if the specification really is that max>min, then it would
> be up to evdev/joydev (if the individual drivers are allowed min>max)
> or adc-joystick (if they are not) to process the samples.

I don't see why a driver, especially a userspace driver which
then injects things back into the kernel using uinput, would
not take care of inverting the samples itself and then just
present userspace with normalized data where min is simply 0
(as result of normalization as part of inversion) and
max is (original_max - original_min).

Note that this is exactly what is being done for touchscreens,
where having the touchscreen mounted e.g. upside-down is
a long standing issue and this is thus also a long solved issue,
see: drivers/input/touchscreen.c which contains generic
code for parsing device-properties including swapped / inverted
axis as well as generic code for reporting the position to the
input core, where the helpers from drivers/input/touchscreen.c
take care of the swap + invert including normalization when
doing inversion.

Specifically this contains in touchscreen_parse_properties() :

        prop->max_x = input_abs_get_max(input, axis_x);
        prop->max_y = input_abs_get_max(input, axis_y);

        if (prop->invert_x) {
                absinfo = &input->absinfo[axis_x];
                absinfo->maximum -= absinfo->minimum;
                absinfo->minimum = 0;
        }

        if (prop->invert_y) {
                absinfo = &input->absinfo[axis_y];
                absinfo->maximum -= absinfo->minimum;
                absinfo->minimum = 0;
        }

and then when reporting touches:

void touchscreen_report_pos(struct input_dev *input,
                            const struct touchscreen_properties *prop,
                            unsigned int x, unsigned int y,
                            bool multitouch)
{
        if (prop->invert_x)
                x = prop->max_x - x;

        if (prop->invert_y)
                y = prop->max_y - y;

        if (prop->swap_x_y)
                swap(x, y);

        input_report_abs(input, multitouch ? ABS_MT_POSITION_X : ABS_X, x);
        input_report_abs(input, multitouch ? ABS_MT_POSITION_Y : ABS_Y, y);
}

One of the tasks of a driver / the kernel is to provide some
level of hardware abstraction to isolate userspace from
hw details. IMHO taking care of the axis-inversion for userspace
with something like the above is part of the kernels' HAL task.

Regards,

Hans

Re: [PATCH] input: uinput: Drop checks for abs_min > abs_max

[Paul Cercueil]

Hi Hans,

Le samedi 23 décembre 2023 à 15:29 +0100, Hans de Goede a écrit :
> Hi Paul,
> 
> On 12/20/23 14:39, Paul Cercueil wrote:
> > Hi Dmitry,
> > 
> > Le mardi 19 décembre 2023 à 17:53 -0800, Dmitry Torokhov a écrit :
> > > Hi Paul,
> > > 
> > > On Wed, Dec 20, 2023 at 01:38:39AM +0100, Paul Cercueil wrote:
> > > > Hi Peter,
> > > > 
> > > > Le mercredi 20 décembre 2023 à 09:51 +1000, Peter Hutterer a
> > > > écrit :
> > > > > On Mon, Dec 18, 2023 at 11:16:53AM -0600, Chris Morgan wrote:
> > > > > > From: Chris Morgan <macromorgan@hotmail.com>
> > > > > > 
> > > > > > Stop checking if the minimum abs value is greater than the
> > > > > > maximum
> > > > > > abs
> > > > > > value. When the axis is inverted this condition is allowed.
> > > > > > Without
> > > > > > relaxing this check, it is not possible to use uinput on
> > > > > > devices in
> > > > > > userspace with an inverted axis, such as the adc-joystick
> > > > > > found
> > > > > > on
> > > > > > many handheld gaming devices.
> > > > > 
> > > > > As mentioned in the other thread [1] a fair bit of userspace
> > > > > relies
> > > > > on
> > > > > that general assumption so removing it will likely cause all
> > > > > sorts of
> > > > > issues.
> > > > 
> > > > There is some userspace that works with it though, so why
> > > > restrict
> > > > it
> > > > artificially?
> > > > 
> > > > The fact that some other userspace code wouldn't work with it
> > > > sounds a
> > > > bit irrelevant. They just never encountered that min>max usage
> > > > before.
> > > > 
> > > > And removing this check won't cause all sort of issues, why
> > > > would
> > > > it?
> > > > It's not like the current software actively probes min>max and
> > > > crash
> > > > badly if it doesn't return -EINVAL...
> > > 
> > > It will cause weird movements because calculations expect min be
> > > the
> > > minimum, and max the maximum, and not encode left/right or
> > > up/down.
> > > Putting this into adc joystick binding was a mistake.
> > 
> > I don't see why it was a mistake, it's only one of the ways to
> > specify
> > that the axis is inverted. This information is between the firmware
> > (DT) and the kernel, that doesn't mean the information has to be
> > relayed as-is to the userspace.
> > 
> > Unlike what you wrote in your other answer, when talking about
> > input
> > the kernel doesn't really normalize anything - it gives you the
> > min/max
> > values, and the raw samples, not normalized samples (they don't get
> > translated to a pre-specified range, or even clamped).
> > 
> > I don't really like the idea of having the driver tamper with the
> > samples, but if the specification really is that max>min, then it
> > would
> > be up to evdev/joydev (if the individual drivers are allowed
> > min>max)
> > or adc-joystick (if they are not) to process the samples.
> 
> I don't see why a driver, especially a userspace driver which
> then injects things back into the kernel using uinput, would
> not take care of inverting the samples itself and then just
> present userspace with normalized data where min is simply 0
> (as result of normalization as part of inversion) and
> max is (original_max - original_min).

Yes, I totally agree.

What I was saying is, as Chris is only "piping" events from adc-
joystick into uinput, that the problem is more that evdev/joydev don't
handle axis inversion and provide min>max values that most of the
userspace (and some kernel drivers e.g. uinput) don't support.

> Note that this is exactly what is being done for touchscreens,
> where having the touchscreen mounted e.g. upside-down is
> a long standing issue and this is thus also a long solved issue,
> see: drivers/input/touchscreen.c which contains generic
> code for parsing device-properties including swapped / inverted
> axis as well as generic code for reporting the position to the
> input core, where the helpers from drivers/input/touchscreen.c
> take care of the swap + invert including normalization when
> doing inversion.
> 
> Specifically this contains in touchscreen_parse_properties() :
> 
>         prop->max_x = input_abs_get_max(input, axis_x);
>         prop->max_y = input_abs_get_max(input, axis_y);
> 
>         if (prop->invert_x) {
>                 absinfo = &input->absinfo[axis_x];
>                 absinfo->maximum -= absinfo->minimum;
>                 absinfo->minimum = 0;
>         }
> 
>         if (prop->invert_y) {
>                 absinfo = &input->absinfo[axis_y];
>                 absinfo->maximum -= absinfo->minimum;
>                 absinfo->minimum = 0;
>         }
> 
> and then when reporting touches:
> 
> void touchscreen_report_pos(struct input_dev *input,
>                             const struct touchscreen_properties
> *prop,
>                             unsigned int x, unsigned int y,
>                             bool multitouch)
> {
>         if (prop->invert_x)
>                 x = prop->max_x - x;
> 
>         if (prop->invert_y)
>                 y = prop->max_y - y;
> 
>         if (prop->swap_x_y)
>                 swap(x, y);
> 
>         input_report_abs(input, multitouch ? ABS_MT_POSITION_X :
> ABS_X, x);
>         input_report_abs(input, multitouch ? ABS_MT_POSITION_Y :
> ABS_Y, y);
> }
> 
> One of the tasks of a driver / the kernel is to provide some
> level of hardware abstraction to isolate userspace from
> hw details. IMHO taking care of the axis-inversion for userspace
> with something like the above is part of the kernels' HAL task.

Totally agree, but this is not done anywhere, is it? evdev seems to
just pass the hardware values alongside some basic meta-data (min/max
values, fuzz etc.), it does not tamper with the data. Should evdev
handle axis inversion? Should it be in adc-joystick (and every other
driver that needs that) instead?

> 
> Regards,
> 
> Hans

Cheers,
-Paul

Re: [PATCH] input: uinput: Drop checks for abs_min > abs_max

[Hans de Goede]

Hi,

On 12/23/23 16:01, Paul Cercueil wrote:
> Hi Hans,
> 
> Le samedi 23 décembre 2023 à 15:29 +0100, Hans de Goede a écrit :
>> Hi Paul,
>>
>> On 12/20/23 14:39, Paul Cercueil wrote:
>>> Hi Dmitry,
>>>
>>> Le mardi 19 décembre 2023 à 17:53 -0800, Dmitry Torokhov a écrit :
>>>> Hi Paul,
>>>>
>>>> On Wed, Dec 20, 2023 at 01:38:39AM +0100, Paul Cercueil wrote:
>>>>> Hi Peter,
>>>>>
>>>>> Le mercredi 20 décembre 2023 à 09:51 +1000, Peter Hutterer a
>>>>> écrit :
>>>>>> On Mon, Dec 18, 2023 at 11:16:53AM -0600, Chris Morgan wrote:
>>>>>>> From: Chris Morgan <macromorgan@hotmail.com>
>>>>>>>
>>>>>>> Stop checking if the minimum abs value is greater than the
>>>>>>> maximum
>>>>>>> abs
>>>>>>> value. When the axis is inverted this condition is allowed.
>>>>>>> Without
>>>>>>> relaxing this check, it is not possible to use uinput on
>>>>>>> devices in
>>>>>>> userspace with an inverted axis, such as the adc-joystick
>>>>>>> found
>>>>>>> on
>>>>>>> many handheld gaming devices.
>>>>>>
>>>>>> As mentioned in the other thread [1] a fair bit of userspace
>>>>>> relies
>>>>>> on
>>>>>> that general assumption so removing it will likely cause all
>>>>>> sorts of
>>>>>> issues.
>>>>>
>>>>> There is some userspace that works with it though, so why
>>>>> restrict
>>>>> it
>>>>> artificially?
>>>>>
>>>>> The fact that some other userspace code wouldn't work with it
>>>>> sounds a
>>>>> bit irrelevant. They just never encountered that min>max usage
>>>>> before.
>>>>>
>>>>> And removing this check won't cause all sort of issues, why
>>>>> would
>>>>> it?
>>>>> It's not like the current software actively probes min>max and
>>>>> crash
>>>>> badly if it doesn't return -EINVAL...
>>>>
>>>> It will cause weird movements because calculations expect min be
>>>> the
>>>> minimum, and max the maximum, and not encode left/right or
>>>> up/down.
>>>> Putting this into adc joystick binding was a mistake.
>>>
>>> I don't see why it was a mistake, it's only one of the ways to
>>> specify
>>> that the axis is inverted. This information is between the firmware
>>> (DT) and the kernel, that doesn't mean the information has to be
>>> relayed as-is to the userspace.
>>>
>>> Unlike what you wrote in your other answer, when talking about
>>> input
>>> the kernel doesn't really normalize anything - it gives you the
>>> min/max
>>> values, and the raw samples, not normalized samples (they don't get
>>> translated to a pre-specified range, or even clamped).
>>>
>>> I don't really like the idea of having the driver tamper with the
>>> samples, but if the specification really is that max>min, then it
>>> would
>>> be up to evdev/joydev (if the individual drivers are allowed
>>> min>max)
>>> or adc-joystick (if they are not) to process the samples.
>>
>> I don't see why a driver, especially a userspace driver which
>> then injects things back into the kernel using uinput, would
>> not take care of inverting the samples itself and then just
>> present userspace with normalized data where min is simply 0
>> (as result of normalization as part of inversion) and
>> max is (original_max - original_min).
> 
> Yes, I totally agree.
> 
> What I was saying is, as Chris is only "piping" events from adc-
> joystick into uinput, that the problem is more that evdev/joydev don't
> handle axis inversion and provide min>max values that most of the
> userspace (and some kernel drivers e.g. uinput) don't support.

Ah I see, that sounds like a joydev adc-joystick / driver bug
to me then.

>> Note that this is exactly what is being done for touchscreens,
>> where having the touchscreen mounted e.g. upside-down is
>> a long standing issue and this is thus also a long solved issue,
>> see: drivers/input/touchscreen.c which contains generic
>> code for parsing device-properties including swapped / inverted
>> axis as well as generic code for reporting the position to the
>> input core, where the helpers from drivers/input/touchscreen.c
>> take care of the swap + invert including normalization when
>> doing inversion.
>>
>> Specifically this contains in touchscreen_parse_properties() :
>>
>>         prop->max_x = input_abs_get_max(input, axis_x);
>>         prop->max_y = input_abs_get_max(input, axis_y);
>>
>>         if (prop->invert_x) {
>>                 absinfo = &input->absinfo[axis_x];
>>                 absinfo->maximum -= absinfo->minimum;
>>                 absinfo->minimum = 0;
>>         }
>>
>>         if (prop->invert_y) {
>>                 absinfo = &input->absinfo[axis_y];
>>                 absinfo->maximum -= absinfo->minimum;
>>                 absinfo->minimum = 0;
>>         }
>>
>> and then when reporting touches:
>>
>> void touchscreen_report_pos(struct input_dev *input,
>>                             const struct touchscreen_properties
>> *prop,
>>                             unsigned int x, unsigned int y,
>>                             bool multitouch)
>> {
>>         if (prop->invert_x)
>>                 x = prop->max_x - x;
>>
>>         if (prop->invert_y)
>>                 y = prop->max_y - y;
>>
>>         if (prop->swap_x_y)
>>                 swap(x, y);
>>
>>         input_report_abs(input, multitouch ? ABS_MT_POSITION_X :
>> ABS_X, x);
>>         input_report_abs(input, multitouch ? ABS_MT_POSITION_Y :
>> ABS_Y, y);
>> }
>>
>> One of the tasks of a driver / the kernel is to provide some
>> level of hardware abstraction to isolate userspace from
>> hw details. IMHO taking care of the axis-inversion for userspace
>> with something like the above is part of the kernels' HAL task.
> 
> Totally agree, but this is not done anywhere, is it? evdev seems to
> just pass the hardware values alongside some basic meta-data (min/max
> values, fuzz etc.), it does not tamper with the data. Should evdev
> handle axis inversion? Should it be in adc-joystick (and every other
> driver that needs that) instead?

For touchcreens we have chosen to have a set of generic helpers
and then make using those helpers the responsibility of the driver.

Part of the reason for doing this is because some touchscreen drivers
already were doing axis inversion inside the driver triggering on
things like e.g. DMI matches, or maybe custom pre standardization
device properties, etc.

So the decision was made to add a set of helpers and convert drivers
one by one. Where drivers can e.g. still set prop->invert_x manually,
but then they also need to take care of the min/max adjustments
manually (min is typically 0 for touchscreens though).

I expect that there will also be enough existing special handling
in the joystick code that piece-meal conversion using helpers
is likely best.

With that said having axis inversion support in the evdev core
does sound interesting, but that means also storing the max-value
inside the core for abs axis and this will likely be a big
change / lots of work.

Regards,

Hans

Re: [PATCH] input: uinput: Drop checks for abs_min > abs_max

[Dmitry Torokhov]

On Sat, Dec 23, 2023 at 04:16:46PM +0100, Hans de Goede wrote:
> Hi,
> 
> On 12/23/23 16:01, Paul Cercueil wrote:
> > Hi Hans,
> > 
> > Le samedi 23 décembre 2023 à 15:29 +0100, Hans de Goede a écrit :
> >> Hi Paul,
> >>
> >> On 12/20/23 14:39, Paul Cercueil wrote:
> >>> Hi Dmitry,
> >>>
> >>> Le mardi 19 décembre 2023 à 17:53 -0800, Dmitry Torokhov a écrit :
> >>>> Hi Paul,
> >>>>
> >>>> On Wed, Dec 20, 2023 at 01:38:39AM +0100, Paul Cercueil wrote:
> >>>>> Hi Peter,
> >>>>>
> >>>>> Le mercredi 20 décembre 2023 à 09:51 +1000, Peter Hutterer a
> >>>>> écrit :
> >>>>>> On Mon, Dec 18, 2023 at 11:16:53AM -0600, Chris Morgan wrote:
> >>>>>>> From: Chris Morgan <macromorgan@hotmail.com>
> >>>>>>>
> >>>>>>> Stop checking if the minimum abs value is greater than the
> >>>>>>> maximum
> >>>>>>> abs
> >>>>>>> value. When the axis is inverted this condition is allowed.
> >>>>>>> Without
> >>>>>>> relaxing this check, it is not possible to use uinput on
> >>>>>>> devices in
> >>>>>>> userspace with an inverted axis, such as the adc-joystick
> >>>>>>> found
> >>>>>>> on
> >>>>>>> many handheld gaming devices.
> >>>>>>
> >>>>>> As mentioned in the other thread [1] a fair bit of userspace
> >>>>>> relies
> >>>>>> on
> >>>>>> that general assumption so removing it will likely cause all
> >>>>>> sorts of
> >>>>>> issues.
> >>>>>
> >>>>> There is some userspace that works with it though, so why
> >>>>> restrict
> >>>>> it
> >>>>> artificially?
> >>>>>
> >>>>> The fact that some other userspace code wouldn't work with it
> >>>>> sounds a
> >>>>> bit irrelevant. They just never encountered that min>max usage
> >>>>> before.
> >>>>>
> >>>>> And removing this check won't cause all sort of issues, why
> >>>>> would
> >>>>> it?
> >>>>> It's not like the current software actively probes min>max and
> >>>>> crash
> >>>>> badly if it doesn't return -EINVAL...
> >>>>
> >>>> It will cause weird movements because calculations expect min be
> >>>> the
> >>>> minimum, and max the maximum, and not encode left/right or
> >>>> up/down.
> >>>> Putting this into adc joystick binding was a mistake.
> >>>
> >>> I don't see why it was a mistake, it's only one of the ways to
> >>> specify
> >>> that the axis is inverted. This information is between the firmware
> >>> (DT) and the kernel, that doesn't mean the information has to be
> >>> relayed as-is to the userspace.
> >>>
> >>> Unlike what you wrote in your other answer, when talking about
> >>> input
> >>> the kernel doesn't really normalize anything - it gives you the
> >>> min/max
> >>> values, and the raw samples, not normalized samples (they don't get
> >>> translated to a pre-specified range, or even clamped).
> >>>
> >>> I don't really like the idea of having the driver tamper with the
> >>> samples, but if the specification really is that max>min, then it
> >>> would
> >>> be up to evdev/joydev (if the individual drivers are allowed
> >>> min>max)
> >>> or adc-joystick (if they are not) to process the samples.
> >>
> >> I don't see why a driver, especially a userspace driver which
> >> then injects things back into the kernel using uinput, would
> >> not take care of inverting the samples itself and then just
> >> present userspace with normalized data where min is simply 0
> >> (as result of normalization as part of inversion) and
> >> max is (original_max - original_min).
> > 
> > Yes, I totally agree.
> > 
> > What I was saying is, as Chris is only "piping" events from adc-
> > joystick into uinput, that the problem is more that evdev/joydev don't
> > handle axis inversion and provide min>max values that most of the
> > userspace (and some kernel drivers e.g. uinput) don't support.
> 
> Ah I see, that sounds like a joydev adc-joystick / driver bug
> to me then.

joydev/mousedev/evdev are simply consumers of events coming from the
drivers that handle hardware. Even though they reside in the kernel,
they still consumers of events, much like userspace is, and they operate
under the same assumption that if min and max are specified then max is
not less than min.

We always had HW drivers invert the axis to match our coordinate system
(for absolute coordinates 0,0 is in the lower left corner, for relative
right and up are positive and left and down are negative). You can see
that in psmouse (psmouse_report_standard_motion) and synaptics drivers,
one of the earliest in the kernel.

The rest of the stack operates under this assumption.

> 
> >> Note that this is exactly what is being done for touchscreens,
> >> where having the touchscreen mounted e.g. upside-down is
> >> a long standing issue and this is thus also a long solved issue,
> >> see: drivers/input/touchscreen.c which contains generic
> >> code for parsing device-properties including swapped / inverted
> >> axis as well as generic code for reporting the position to the
> >> input core, where the helpers from drivers/input/touchscreen.c
> >> take care of the swap + invert including normalization when
> >> doing inversion.
> >>
> >> Specifically this contains in touchscreen_parse_properties() :
> >>
> >>         prop->max_x = input_abs_get_max(input, axis_x);
> >>         prop->max_y = input_abs_get_max(input, axis_y);
> >>
> >>         if (prop->invert_x) {
> >>                 absinfo = &input->absinfo[axis_x];
> >>                 absinfo->maximum -= absinfo->minimum;
> >>                 absinfo->minimum = 0;
> >>         }
> >>
> >>         if (prop->invert_y) {
> >>                 absinfo = &input->absinfo[axis_y];
> >>                 absinfo->maximum -= absinfo->minimum;
> >>                 absinfo->minimum = 0;
> >>         }
> >>
> >> and then when reporting touches:
> >>
> >> void touchscreen_report_pos(struct input_dev *input,
> >>                             const struct touchscreen_properties
> >> *prop,
> >>                             unsigned int x, unsigned int y,
> >>                             bool multitouch)
> >> {
> >>         if (prop->invert_x)
> >>                 x = prop->max_x - x;
> >>
> >>         if (prop->invert_y)
> >>                 y = prop->max_y - y;
> >>
> >>         if (prop->swap_x_y)
> >>                 swap(x, y);
> >>
> >>         input_report_abs(input, multitouch ? ABS_MT_POSITION_X :
> >> ABS_X, x);
> >>         input_report_abs(input, multitouch ? ABS_MT_POSITION_Y :
> >> ABS_Y, y);
> >> }
> >>
> >> One of the tasks of a driver / the kernel is to provide some
> >> level of hardware abstraction to isolate userspace from
> >> hw details. IMHO taking care of the axis-inversion for userspace
> >> with something like the above is part of the kernels' HAL task.
> > 
> > Totally agree, but this is not done anywhere, is it? evdev seems to
> > just pass the hardware values alongside some basic meta-data (min/max
> > values, fuzz etc.), it does not tamper with the data. Should evdev
> > handle axis inversion? Should it be in adc-joystick (and every other
> > driver that needs that) instead?
> 
> For touchcreens we have chosen to have a set of generic helpers
> and then make using those helpers the responsibility of the driver.
> 
> Part of the reason for doing this is because some touchscreen drivers
> already were doing axis inversion inside the driver triggering on
> things like e.g. DMI matches, or maybe custom pre standardization
> device properties, etc.
> 
> So the decision was made to add a set of helpers and convert drivers
> one by one. Where drivers can e.g. still set prop->invert_x manually,
> but then they also need to take care of the min/max adjustments
> manually (min is typically 0 for touchscreens though).
> 
> I expect that there will also be enough existing special handling
> in the joystick code that piece-meal conversion using helpers
> is likely best.

Not only touchscreens and joysticks. As I mentioned, PS/2 mice have
their reported relative Y motion inverted since forever, touchpads like
Synaptics or Elan invert Y axis as well, and so on.

> 
> With that said having axis inversion support in the evdev core
> does sound interesting, but that means also storing the max-value
> inside the core for abs axis and this will likely be a big
> change / lots of work.

evdev is not the only consumer, so if anything it should be in the input
core, but there are enough quirks that I think touchscreen helpers are
the best, at least for now.

Thanks.

-- 
Dmitry

Re: [PATCH] input: uinput: Drop checks for abs_min > abs_max

[Chris Morgan]

On Sun, Dec 24, 2023 at 12:03:18AM -0800, Dmitry Torokhov wrote:
> On Sat, Dec 23, 2023 at 04:16:46PM +0100, Hans de Goede wrote:
> > Hi,
> > 
> > On 12/23/23 16:01, Paul Cercueil wrote:
> > > Hi Hans,
> > > 
> > > Le samedi 23 décembre 2023 à 15:29 +0100, Hans de Goede a écrit :
> > >> Hi Paul,
> > >>
> > >> On 12/20/23 14:39, Paul Cercueil wrote:
> > >>> Hi Dmitry,
> > >>>
> > >>> Le mardi 19 décembre 2023 à 17:53 -0800, Dmitry Torokhov a écrit :
> > >>>> Hi Paul,
> > >>>>
> > >>>> On Wed, Dec 20, 2023 at 01:38:39AM +0100, Paul Cercueil wrote:
> > >>>>> Hi Peter,
> > >>>>>
> > >>>>> Le mercredi 20 décembre 2023 à 09:51 +1000, Peter Hutterer a
> > >>>>> écrit :
> > >>>>>> On Mon, Dec 18, 2023 at 11:16:53AM -0600, Chris Morgan wrote:
> > >>>>>>> From: Chris Morgan <macromorgan@hotmail.com>
> > >>>>>>>
> > >>>>>>> Stop checking if the minimum abs value is greater than the
> > >>>>>>> maximum
> > >>>>>>> abs
> > >>>>>>> value. When the axis is inverted this condition is allowed.
> > >>>>>>> Without
> > >>>>>>> relaxing this check, it is not possible to use uinput on
> > >>>>>>> devices in
> > >>>>>>> userspace with an inverted axis, such as the adc-joystick
> > >>>>>>> found
> > >>>>>>> on
> > >>>>>>> many handheld gaming devices.
> > >>>>>>
> > >>>>>> As mentioned in the other thread [1] a fair bit of userspace
> > >>>>>> relies
> > >>>>>> on
> > >>>>>> that general assumption so removing it will likely cause all
> > >>>>>> sorts of
> > >>>>>> issues.
> > >>>>>
> > >>>>> There is some userspace that works with it though, so why
> > >>>>> restrict
> > >>>>> it
> > >>>>> artificially?
> > >>>>>
> > >>>>> The fact that some other userspace code wouldn't work with it
> > >>>>> sounds a
> > >>>>> bit irrelevant. They just never encountered that min>max usage
> > >>>>> before.
> > >>>>>
> > >>>>> And removing this check won't cause all sort of issues, why
> > >>>>> would
> > >>>>> it?
> > >>>>> It's not like the current software actively probes min>max and
> > >>>>> crash
> > >>>>> badly if it doesn't return -EINVAL...
> > >>>>
> > >>>> It will cause weird movements because calculations expect min be
> > >>>> the
> > >>>> minimum, and max the maximum, and not encode left/right or
> > >>>> up/down.
> > >>>> Putting this into adc joystick binding was a mistake.
> > >>>
> > >>> I don't see why it was a mistake, it's only one of the ways to
> > >>> specify
> > >>> that the axis is inverted. This information is between the firmware
> > >>> (DT) and the kernel, that doesn't mean the information has to be
> > >>> relayed as-is to the userspace.
> > >>>
> > >>> Unlike what you wrote in your other answer, when talking about
> > >>> input
> > >>> the kernel doesn't really normalize anything - it gives you the
> > >>> min/max
> > >>> values, and the raw samples, not normalized samples (they don't get
> > >>> translated to a pre-specified range, or even clamped).
> > >>>
> > >>> I don't really like the idea of having the driver tamper with the
> > >>> samples, but if the specification really is that max>min, then it
> > >>> would
> > >>> be up to evdev/joydev (if the individual drivers are allowed
> > >>> min>max)
> > >>> or adc-joystick (if they are not) to process the samples.
> > >>
> > >> I don't see why a driver, especially a userspace driver which
> > >> then injects things back into the kernel using uinput, would
> > >> not take care of inverting the samples itself and then just
> > >> present userspace with normalized data where min is simply 0
> > >> (as result of normalization as part of inversion) and
> > >> max is (original_max - original_min).
> > > 
> > > Yes, I totally agree.
> > > 
> > > What I was saying is, as Chris is only "piping" events from adc-
> > > joystick into uinput, that the problem is more that evdev/joydev don't
> > > handle axis inversion and provide min>max values that most of the
> > > userspace (and some kernel drivers e.g. uinput) don't support.
> > 
> > Ah I see, that sounds like a joydev adc-joystick / driver bug
> > to me then.
> 
> joydev/mousedev/evdev are simply consumers of events coming from the
> drivers that handle hardware. Even though they reside in the kernel,
> they still consumers of events, much like userspace is, and they operate
> under the same assumption that if min and max are specified then max is
> not less than min.
> 
> We always had HW drivers invert the axis to match our coordinate system
> (for absolute coordinates 0,0 is in the lower left corner, for relative
> right and up are positive and left and down are negative). You can see
> that in psmouse (psmouse_report_standard_motion) and synaptics drivers,
> one of the earliest in the kernel.
> 
> The rest of the stack operates under this assumption.
> 
> > 
> > >> Note that this is exactly what is being done for touchscreens,
> > >> where having the touchscreen mounted e.g. upside-down is
> > >> a long standing issue and this is thus also a long solved issue,
> > >> see: drivers/input/touchscreen.c which contains generic
> > >> code for parsing device-properties including swapped / inverted
> > >> axis as well as generic code for reporting the position to the
> > >> input core, where the helpers from drivers/input/touchscreen.c
> > >> take care of the swap + invert including normalization when
> > >> doing inversion.
> > >>
> > >> Specifically this contains in touchscreen_parse_properties() :
> > >>
> > >>         prop->max_x = input_abs_get_max(input, axis_x);
> > >>         prop->max_y = input_abs_get_max(input, axis_y);
> > >>
> > >>         if (prop->invert_x) {
> > >>                 absinfo = &input->absinfo[axis_x];
> > >>                 absinfo->maximum -= absinfo->minimum;
> > >>                 absinfo->minimum = 0;
> > >>         }
> > >>
> > >>         if (prop->invert_y) {
> > >>                 absinfo = &input->absinfo[axis_y];
> > >>                 absinfo->maximum -= absinfo->minimum;
> > >>                 absinfo->minimum = 0;
> > >>         }
> > >>
> > >> and then when reporting touches:
> > >>
> > >> void touchscreen_report_pos(struct input_dev *input,
> > >>                             const struct touchscreen_properties
> > >> *prop,
> > >>                             unsigned int x, unsigned int y,
> > >>                             bool multitouch)
> > >> {
> > >>         if (prop->invert_x)
> > >>                 x = prop->max_x - x;
> > >>
> > >>         if (prop->invert_y)
> > >>                 y = prop->max_y - y;
> > >>
> > >>         if (prop->swap_x_y)
> > >>                 swap(x, y);
> > >>
> > >>         input_report_abs(input, multitouch ? ABS_MT_POSITION_X :
> > >> ABS_X, x);
> > >>         input_report_abs(input, multitouch ? ABS_MT_POSITION_Y :
> > >> ABS_Y, y);
> > >> }
> > >>
> > >> One of the tasks of a driver / the kernel is to provide some
> > >> level of hardware abstraction to isolate userspace from
> > >> hw details. IMHO taking care of the axis-inversion for userspace
> > >> with something like the above is part of the kernels' HAL task.
> > > 
> > > Totally agree, but this is not done anywhere, is it? evdev seems to
> > > just pass the hardware values alongside some basic meta-data (min/max
> > > values, fuzz etc.), it does not tamper with the data. Should evdev
> > > handle axis inversion? Should it be in adc-joystick (and every other
> > > driver that needs that) instead?
> > 
> > For touchcreens we have chosen to have a set of generic helpers
> > and then make using those helpers the responsibility of the driver.
> > 
> > Part of the reason for doing this is because some touchscreen drivers
> > already were doing axis inversion inside the driver triggering on
> > things like e.g. DMI matches, or maybe custom pre standardization
> > device properties, etc.
> > 
> > So the decision was made to add a set of helpers and convert drivers
> > one by one. Where drivers can e.g. still set prop->invert_x manually,
> > but then they also need to take care of the min/max adjustments
> > manually (min is typically 0 for touchscreens though).
> > 
> > I expect that there will also be enough existing special handling
> > in the joystick code that piece-meal conversion using helpers
> > is likely best.
> 
> Not only touchscreens and joysticks. As I mentioned, PS/2 mice have
> their reported relative Y motion inverted since forever, touchpads like
> Synaptics or Elan invert Y axis as well, and so on.
> 
> > 
> > With that said having axis inversion support in the evdev core
> > does sound interesting, but that means also storing the max-value
> > inside the core for abs axis and this will likely be a big
> > change / lots of work.
> 
> evdev is not the only consumer, so if anything it should be in the input
> core, but there are enough quirks that I think touchscreen helpers are
> the best, at least for now.
> 
> Thanks.

I feel like what I've seen here leads me to believe that we should
handle it in the driver. The only other *thing* I've seen that's
leading me to my solution is that it should theoretically be possible
for a joystick to have negative values (every place I check shows
int or s32 or something). I do see the way the touchscreen handles it
in touchscreen_parse_properties() and touchscreen_apply_prop_to_x_y()
and I was thinking there might be a better way to do it.

I'm under the assumption (still new, sorry) that I can't simply modify
an existing struct since userspace relies heavily on it and we can't
modify userspace... specifically by adding a bool to the input_absinfo.
So that said I was thinking we should add a helper to the input.c
that inverts an abs when requested by taking the absmin, absmax, and
value, then inverting it by doing ((absmax + absmin) - value). By doing
it this way we should be able to invert the values correctly, even when
dealing with negatives... With this example ((1023 + 15) - 15) would
give me 1023 which is what I would expect the inverted value to be, and
((1023 + -1023) - -1023) should give me 1023 which is again what I
would expect the inverse of -1023 to be in this case. We'll leave it up
to the individual driver to actually track the inverted status and to
apply inversion as necessary.

If you think this is the best course I'll add a helper to the input.c
function and then make use of it in the adc-joystick.c for both
polled and non-polled cases.

Thank you,
Chris

> 
> -- 
> Dmitry