linux-kernel - Re: 4.1 touchscreen regression on n900 -- pinpointed [was Re: linux-n900 v4.1-rc4]

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20150529213649.GB7429@dtor-ws>
Date:	Fri, 29 May 2015 14:36:49 -0700
From:	Dmitry Torokhov <dmitry.torokhov@...il.com>
To:	Pavel Machek <pavel@....cz>
Cc:	Maxime Ripard <maxime.ripard@...e-electrons.com>,
	Sebastian Reichel <sre@...nel.org>,
	kernel list <linux-kernel@...r.kernel.org>,
	Pali Rohár <pali.rohar@...il.com>,
	Ivaylo Dimitrov <ivo.g.dimitrov.75@...il.com>
Subject: Re: 4.1 touchscreen regression on n900 -- pinpointed [was Re:
 linux-n900 v4.1-rc4]

On Fri, May 29, 2015 at 11:17:22PM +0200, Pavel Machek wrote:
> On Fri 2015-05-29 21:57:40, Maxime Ripard wrote:
> > On Fri, May 29, 2015 at 09:25:05PM +0200, Pavel Machek wrote:
> > > On Fri 2015-05-29 21:08:16, Pavel Machek wrote:
> > > > Hi!
> > > > 
> > > > > mh I remember having problems with tsc2005 before. It helped to
> > > > > reset the controller (should actually happen automatically when it
> > > > > hangs, but I'm not sure, that it actually works).
> > > > 
> > > > Ok, I did some more testing, and found out rather bogus values in
> > > > evtest:
> > > > 
> > > > Input device name: "TSC2005 touchscreen"
> > > > Supported events:
> > > >   Event type 0 (EV_SYN)
> > > >   Event type 1 (EV_KEY)
> > > >   Event code 330 (BTN_TOUCH)
> > > >   Event type 3 (EV_ABS)
> > > >   Event code 0 (ABS_X)
> > > >   Value   2514
> > > >  Min        0
> > > >  Max        0
> > > >  Fuzz       4
> > > > 
> > > > Which made me go through the git logs, and these patches looked
> > > > suspicious. After a revert... yes, touchscreen works as well as it
> > > > worked before.
> > > > 
> > > > 0a363a380954e10fece7cd9931b66056eeb07d56
> > > > 3eea8b5d68c801fec788b411582b803463834752
> > > > 
> > > > (It is impossible to revert just 3eea..)
> > > 
> > > Hmm, I see:
> > > 
> > >                 touchscreen-max-x = <4096>;
> > >                 touchscreen-max-y = <4096>;
> > > ...that's n900 dts.. this should be size-x/size-y... so we have a bug
> > > in dts.
> > > 
> > > But the 3eea8b5d68c801fec788b411582b803463834752 is buggy, it should
> > > not overwrite ->maximum for axis it has no devicetree data for.
> > 
> > What do you mean? touchscreen-max-* _is_ device tree data for an axis.
> > 
> > > Maybe replacing
> > > 
> > > +       if (maximum || fuzz)
> > > 
> > > in 3eea to (maximum && fuzz)... would help, but it is late in the
> > > cycle now, so I'd suggest just reverting 3eea8b.
> > 
> > No, both maximum and fuzz are optional. You can perfectly have one
> > without another.
> 
> Yes, which is something you got wrong.
> 
> Apply this on top of 3eea8b5d68c801fec788b411582b803463834752, and
> you'll get simpler code, that works like it did before.
> 
> Maxime, since you caused the regression, can I ask you to put the
> other patches on top of it, test it still works for you and submit it
> properly?
> 
> [If you want to return in !test_bit case, and are sure it does not
> break anything, feel free to do that.]
> 
> Thanks,
> 								Pavel
> 
> Signed-off-by: Pavel Machek <pavel@....cz>
> 
> diff --git a/drivers/input/touchscreen/of_touchscreen.c b/drivers/input/touchscreen/of_touchscreen.c
> index b82b520..e626c8a 100644
> --- a/drivers/input/touchscreen/of_touchscreen.c
> +++ b/drivers/input/touchscreen/of_touchscreen.c
> @@ -11,39 +11,23 @@
>  
>  #include <linux/of.h>
>  #include <linux/input.h>
> -#include <linux/input/mt.h>
>  #include <linux/input/touchscreen.h>
>  
> -static u32 of_get_optional_u32(struct device_node *np,
> -			       const char *property)
> -{
> -	u32 val = 0;
> -
> -	of_property_read_u32(np, property, &val);
> -
> -	return val;
> -}
> -
>  static void touchscreen_set_params(struct input_dev *dev,
>  				   unsigned long axis,
> -				   int max, int fuzz)
> +				   char *max, char *fuzz)
>  {
>  	struct input_absinfo *absinfo;
> +	struct device_node *np = dev->dev.parent->of_node;
>  
>  	if (!test_bit(axis, dev->absbit)) {
> -		/*
> -		 * Emit a warning only if the axis is not a multitouch
> -		 * axis, which might not be set by the driver.
> -		 */
> -		if (!input_is_mt_axis(axis))
> -			dev_warn(&dev->dev,
> -				 "DT specifies parameters but the axis is not set up\n");
> -		return;
> +		dev_warn(&dev->dev,
> +			 "DT specifies parameters but the axis is not set up\n");

No, this is not right either. We do not want to bitch about axis not
being set up if there is nothing in DT mentioning this axis.

Plus the code structure won't work well if we decide to add more to it
(such as min, or flat, or resolution).

Thanks.

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/