Re: [Linuxwacom-devel] [PATCH input-wacom 2/2] 2.6.32: Fix abs get/set accessors for 2.6.36/2.6.37

[Peter Hutterer]

On Mon, Apr 23, 2018 at 08:18:48AM -0700, Jason Gerecke wrote:
> On Sun, Apr 22, 2018 at 4:06 PM, Peter Hutterer
>  wrote:
> > On Fri, Apr 20, 2018 at 02:33:21PM -0700, Jason Gerecke wrote:
> >> The 2.6.36 kernel removes several members from `struct input_dev` (e.g.
> >> abs, absres) and replaces them with a structs and accessor functions.
> >> To allow the input-wacom code to compile under both old and new kernels,
> >> commits ca9786f and the mailinglist version of ab2ea683fb conditionally
> >> defined their own implementation of the accessor function for older
> >> kernels.
> >>
> >> It was noticed, however, that this did not compile correctly on RHEL 6
> >> systems. It seems that the accessor API introduced in 2.6.36 is provided
> >> in their customized "2.6.32" kernel. This results in a redefinition error
> >> that halts compilation. To work around this, commit 58d8320541 removed
> >> the condtional and renamed our implementation of the accessor. Commit
> >> ab2ea683fb was also similarly modified from its mailinglist version prior
> >> to being committed. This change prevented the redefinition on RHEL 6 and
> >> also worked fine for pre-2.6.36 kernels. The change ended up breaking
> >> compilation under stock 2.6.36 kernels since the members used by the
> >> renamed function were removed.
> >>
> >> To ensure the code compiles in all cases, we need to be a little more
> >> clever. We make use of the recently-added "WACOM_LINUX_TRY_COMPILE"
> >> configure macro to see if the kernel provides the accessor API or not.
> >> If it does, we make use of it; of not, we access the members directly.
> >>
> >> Fixes: 58d8320541 ("2.6.30: define wacom_input_abs_get_val")
> >> Fixes: ab2ea683fb ("2.6.32: Backport resolution support to 2.6.32")
> >> Signed-off-by: Jason Gerecke 
> >> ---
> >>  2.6.32/wacom_wac.c |  8 
> >>  configure.ac   | 15 +++
> >>  2 files changed, 23 insertions(+)
> >>
> >> diff --git a/2.6.32/wacom_wac.c b/2.6.32/wacom_wac.c
> >> index 3ad7aae..11c7513 100644
> >> --- a/2.6.32/wacom_wac.c
> >> +++ b/2.6.32/wacom_wac.c
> >> @@ -924,7 +924,11 @@ static int wacom_intuos_irq(struct wacom_wac *wacom)
> >>
> >>  static int wacom_input_abs_get_val(struct input_dev *input, unsigned int 
> >> axis)
> >>  {
> >> +#ifndef WACOM_ABSACCESSOR_36
> >>   return input->abs[axis];
> >> +#else
> >> + return input_abs_get_val(input, axis);
> >> +#endif
> >
> > I've been staring at this for a bit and quickly skimmed through the commits
> > metioned above but I'm still unsure why you couldn't just leave
> > input_abs_get_val() here and make that function's existence (or internal
> > implementation) dependent on the WACOM_ABSACCESSOR_36 define. Would save you
> > a buch of ifdefs.
> >
> 
> It would definitely be possible to do that, but I don't really see an
> ifdef savings. I suppose if we shuffled the two functions to be next
> to each other we could get away with one ifdef instead of two, but (at
> least for the moment) I'm perfectly happy leaving things where they
> are and named with the 'wacom_' prefix to make it clear at the point
> of use that we're not necessarily dealing with an upstream function.

the two benefits you get: the #ifdef in only one location, where the
accessor functions are defined. and a better ability to cherry-pick because
you should have less conflicts in those.

Cheers,
   Peter

> >>  }
> >>
> >>  static void wacom_multitouch_generic_finger(struct wacom_wac *wacom,
> >> @@ -2080,7 +2084,11 @@ void wacom_setup_device_quirks(struct wacom *wacom)
> >>
> >>  static inline void wacom_input_abs_set_res(struct input_dev *dev, 
> >> unsigned int axis, int val)
> >>  {
> >> +#ifndef WACOM_ABSACCESSOR_36
> >
> > personal nit: I prefer something like HAVE_INPUT_ABS_SET_RES because it
> > makes the whole thing more obvious when reading. WACOM_ABSACCESSOR_36 is a
> > bit harder to understand (naming it WACOM_ABSACESSOR_v2_36 might make it
> > easier, but still).
> >
> 
> I was copying the existing style introduced by Benjamin
> ('WACOM_POWERSUPPLY_41'), but now that you mention it, I also prefer
> the more boolean-like reading of your suggestion. Maybe we could use a
> follow-up patch to fix both of these :D
> 
> >>   dev->absres[axis] = val;
> >> +#else
> >> + input_abs_set_res(dev, axis, val);
> >> +#endif
> >>  }
> >>
> >>  static void wacom_abs_set_axis(struct input_dev *input_dev,
> >> diff --git a/configure.ac b/configure.ac
> >> index 1cb4394..a14a569 100644
> >> --- a/configure.ac
> >> +++ b/configure.ac
> >> @@ -194,6 +194,21 @@ struct power_supply_desc test;
> >>   AC_MSG_RESULT([pre-v4.1])
> >>  ])
> >>
> >> +dnl RedHat entreprise Linux 6.x backports abs accessor functions from 
> >> 2.6.36
> >
> > typo, also officially it's "Enterprise" (capital E)
> >
> 
> Good catch >.<
> 
> >> +AC_MSG_CHECKING(abs accessor version)
> >> +WACOM_LINUX_TRY_COMPILE([
> >> +#include 
> >> +],[
> >> +struct 

Re: [Linuxwacom-devel] [PATCH input-wacom 2/2] 2.6.32: Fix abs get/set accessors for 2.6.36/2.6.37

[Peter Hutterer]

On Fri, Apr 20, 2018 at 02:33:21PM -0700, Jason Gerecke wrote:
> The 2.6.36 kernel removes several members from `struct input_dev` (e.g.
> abs, absres) and replaces them with a structs and accessor functions.
> To allow the input-wacom code to compile under both old and new kernels,
> commits ca9786f and the mailinglist version of ab2ea683fb conditionally
> defined their own implementation of the accessor function for older
> kernels.
> 
> It was noticed, however, that this did not compile correctly on RHEL 6
> systems. It seems that the accessor API introduced in 2.6.36 is provided
> in their customized "2.6.32" kernel. This results in a redefinition error
> that halts compilation. To work around this, commit 58d8320541 removed
> the condtional and renamed our implementation of the accessor. Commit
> ab2ea683fb was also similarly modified from its mailinglist version prior
> to being committed. This change prevented the redefinition on RHEL 6 and
> also worked fine for pre-2.6.36 kernels. The change ended up breaking
> compilation under stock 2.6.36 kernels since the members used by the
> renamed function were removed.
> 
> To ensure the code compiles in all cases, we need to be a little more
> clever. We make use of the recently-added "WACOM_LINUX_TRY_COMPILE"
> configure macro to see if the kernel provides the accessor API or not.
> If it does, we make use of it; of not, we access the members directly.
> 
> Fixes: 58d8320541 ("2.6.30: define wacom_input_abs_get_val")
> Fixes: ab2ea683fb ("2.6.32: Backport resolution support to 2.6.32")
> Signed-off-by: Jason Gerecke 
> ---
>  2.6.32/wacom_wac.c |  8 
>  configure.ac   | 15 +++
>  2 files changed, 23 insertions(+)
> 
> diff --git a/2.6.32/wacom_wac.c b/2.6.32/wacom_wac.c
> index 3ad7aae..11c7513 100644
> --- a/2.6.32/wacom_wac.c
> +++ b/2.6.32/wacom_wac.c
> @@ -924,7 +924,11 @@ static int wacom_intuos_irq(struct wacom_wac *wacom)
>  
>  static int wacom_input_abs_get_val(struct input_dev *input, unsigned int 
> axis)
>  {
> +#ifndef WACOM_ABSACCESSOR_36
>   return input->abs[axis];
> +#else
> + return input_abs_get_val(input, axis);
> +#endif

I've been staring at this for a bit and quickly skimmed through the commits
metioned above but I'm still unsure why you couldn't just leave
input_abs_get_val() here and make that function's existence (or internal
implementation) dependent on the WACOM_ABSACCESSOR_36 define. Would save you
a buch of ifdefs.

>  }
>  
>  static void wacom_multitouch_generic_finger(struct wacom_wac *wacom,
> @@ -2080,7 +2084,11 @@ void wacom_setup_device_quirks(struct wacom *wacom)
>  
>  static inline void wacom_input_abs_set_res(struct input_dev *dev, unsigned 
> int axis, int val)
>  {
> +#ifndef WACOM_ABSACCESSOR_36

personal nit: I prefer something like HAVE_INPUT_ABS_SET_RES because it
makes the whole thing more obvious when reading. WACOM_ABSACCESSOR_36 is a
bit harder to understand (naming it WACOM_ABSACESSOR_v2_36 might make it
easier, but still).

>   dev->absres[axis] = val;
> +#else
> + input_abs_set_res(dev, axis, val);
> +#endif
>  }
>  
>  static void wacom_abs_set_axis(struct input_dev *input_dev,
> diff --git a/configure.ac b/configure.ac
> index 1cb4394..a14a569 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -194,6 +194,21 @@ struct power_supply_desc test;
>   AC_MSG_RESULT([pre-v4.1])
>  ])
>  
> +dnl RedHat entreprise Linux 6.x backports abs accessor functions from 2.6.36

typo, also officially it's "Enterprise" (capital E)

> +AC_MSG_CHECKING(abs accessor version)
> +WACOM_LINUX_TRY_COMPILE([
> +#include 
> +],[
> +struct input_dev test;
> +input_abs_get_res(, 0);

fwiw, just passing NULL should work here, makes the check just one line.

The input-wacom repo is decidedly yours, so this is all very much your
decision :)

Cheers,
   Peter

> +],[
> + HAVE_ABSACCESSOR_36=yes
> + AC_MSG_RESULT([v2.6.36+])
> + AC_DEFINE([WACOM_ABSACCESSOR_36], [], [kernel uses abs accessor macros 
> from v2.6.36+])
> +],[
> + HAVE_ABSACCESSOR_36=no
> + AC_MSG_RESULT([pre-v2.6.36])
> +])
>  
>  dnl Check which version of the driver we should compile
>  AC_DEFUN([WCM_EXPLODE], [$(echo "$1" | awk '{split($[0],x,"[[^0-9]]"); 
> printf("%03d%03d%03d\n",x[[1]],x[[2]],x[[3]]);}')])
> -- 
> 2.17.0
> 
> 
> --
> Check out the vibrant tech community on one of the world's most
> engaging tech sites, Slashdot.org! http://sdm.link/slashdot
> ___
> Linuxwacom-devel mailing list
> Linuxwacom-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linuxwacom-devel
> 

--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___

Re: [Linuxwacom-devel] [PATCH input-wacom 2/2] 2.6.32: Fix abs get/set accessors for 2.6.36/2.6.37

[Ping Cheng]

On Fri, Apr 20, 2018 at 2:33 PM, Jason Gerecke  wrote:

> The 2.6.36 kernel removes several members from `struct input_dev` (e.g.
> abs, absres) and replaces them with a structs and accessor functions.
> To allow the input-wacom code to compile under both old and new kernels,
> commits ca9786f and the mailinglist version of ab2ea683fb conditionally
> defined their own implementation of the accessor function for older
> kernels.
>
> It was noticed, however, that this did not compile correctly on RHEL 6
> systems. It seems that the accessor API introduced in 2.6.36 is provided
> in their customized "2.6.32" kernel. This results in a redefinition error
> that halts compilation. To work around this, commit 58d8320541 removed
> the condtional and renamed our implementation of the accessor. Commit
> ab2ea683fb was also similarly modified from its mailinglist version prior
> to being committed. This change prevented the redefinition on RHEL 6 and
> also worked fine for pre-2.6.36 kernels. The change ended up breaking
> compilation under stock 2.6.36 kernels since the members used by the
> renamed function were removed.
>

 That's quite an explanation.

To ensure the code compiles in all cases, we need to be a little more
> clever. We make use of the recently-added "WACOM_LINUX_TRY_COMPILE"
> configure macro to see if the kernel provides the accessor API or not.
> If it does, we make use of it; of not, we access the members directly.
>

 Yeah, we get to be clever the third time :D.


> Fixes: 58d8320541 ("2.6.30: define wacom_input_abs_get_val")
> Fixes: ab2ea683fb ("2.6.32: Backport resolution support to 2.6.32")
> Signed-off-by: Jason Gerecke 
>

Reviewed-by: Ping Cheng 

Thank you,
Ping


> ---
>  2.6.32/wacom_wac.c |  8 
>  configure.ac   | 15 +++
>  2 files changed, 23 insertions(+)
>
> diff --git a/2.6.32/wacom_wac.c b/2.6.32/wacom_wac.c
> index 3ad7aae..11c7513 100644
> --- a/2.6.32/wacom_wac.c
> +++ b/2.6.32/wacom_wac.c
> @@ -924,7 +924,11 @@ static int wacom_intuos_irq(struct wacom_wac *wacom)
>
>  static int wacom_input_abs_get_val(struct input_dev *input, unsigned int
> axis)
>  {
> +#ifndef WACOM_ABSACCESSOR_36
> return input->abs[axis];
> +#else
> +   return input_abs_get_val(input, axis);
> +#endif
>  }
>
>  static void wacom_multitouch_generic_finger(struct wacom_wac *wacom,
> @@ -2080,7 +2084,11 @@ void wacom_setup_device_quirks(struct wacom *wacom)
>
>  static inline void wacom_input_abs_set_res(struct input_dev *dev,
> unsigned int axis, int val)
>  {
> +#ifndef WACOM_ABSACCESSOR_36
> dev->absres[axis] = val;
> +#else
> +   input_abs_set_res(dev, axis, val);
> +#endif
>  }
>
>  static void wacom_abs_set_axis(struct input_dev *input_dev,
> diff --git a/configure.ac b/configure.ac
> index 1cb4394..a14a569 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -194,6 +194,21 @@ struct power_supply_desc test;
> AC_MSG_RESULT([pre-v4.1])
>  ])
>
> +dnl RedHat entreprise Linux 6.x backports abs accessor functions from
> 2.6.36
> +AC_MSG_CHECKING(abs accessor version)
> +WACOM_LINUX_TRY_COMPILE([
> +#include 
> +],[
> +struct input_dev test;
> +input_abs_get_res(, 0);
> +],[
> +   HAVE_ABSACCESSOR_36=yes
> +   AC_MSG_RESULT([v2.6.36+])
> +   AC_DEFINE([WACOM_ABSACCESSOR_36], [], [kernel uses abs accessor
> macros from v2.6.36+])
> +],[
> +   HAVE_ABSACCESSOR_36=no
> +   AC_MSG_RESULT([pre-v2.6.36])
> +])
>
>  dnl Check which version of the driver we should compile
>  AC_DEFUN([WCM_EXPLODE], [$(echo "$1" | awk '{split($[0],x,"[[^0-9]]");
> printf("%03d%03d%03d\n",x[[1]],x[[2]],x[[3]]);}')])
> --
> 2.17.0
>
>
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot___
Linuxwacom-devel mailing list
Linuxwacom-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxwacom-devel