linux-kernel - RE: [PATCH v3 1/5] Input: goodix

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]

Date:	Thu, 10 Sep 2015 14:04:56 +0000
From:	"Tirdea, Irina" <irina.tirdea@...el.com>
To:	Bastien Nocera <hadess@...ess.net>,
	"linux-input@...r.kernel.org" <linux-input@...r.kernel.org>
CC:	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Rob Herring <robh+dt@...nel.org>,
	Pawel Moll <pawel.moll@....com>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	Kumar Gala <galak@...eaurora.org>,
	"Purdila, Octavian" <octavian.purdila@...el.com>,
	Dmitry Torokhov <dmitry.torokhov@...il.com>,
	Mark Rutland <mark.rutland@....com>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>
Subject: RE: [PATCH v3 1/5] Input: goodix - reset device at init

> -----Original Message-----
> From: Bastien Nocera [mailto:hadess@...ess.net]
> Sent: 09 September, 2015 20:03
> To: Tirdea, Irina; linux-input@...r.kernel.org
> Cc: linux-kernel@...r.kernel.org; Rob Herring; Pawel Moll; Ian Campbell; Kumar Gala; Purdila, Octavian; Dmitry Torokhov; Mark
> Rutland; devicetree@...r.kernel.org
> Subject: Re: [PATCH v3 1/5] Input: goodix - reset device at init
> 
> On Thu, 2015-07-30 at 11:27 +0000, Tirdea, Irina wrote:
> > I can send some additional patches that will simplify testing the
> > configuration update to the Goodix device. I think this feature is
> > the easiest
> > to test so we can determine if writing to the interrupt pin actually
> > works.
> > However, even if it is a BIOS problem and the code will work, the
> > warning
> > will still be printed in dmesg.
> 
> 
> Somehow missed this mail before replying to the current patchset. I'd
> be fine with that, though it's still not clear to me whether the
> BIOS/hardware is at fault, or the code that's being added to the driver
> ;)
> 

The reset procedure is described in the Goodix GT911 datasheet [1] and is
used for power-on reset and power management. The power-on sequence
is described in chapter 6.1. I2C Timing, in the Power-on Timing diagram.
The sequence for putting the device to sleep is described in chapter
7.1. Operating Modes, c) Sleep mode. These sequences use the interrupt
pin as output.

The warning you mentioned comes from the following code in the goodix
driver, which sets the interrupt to be used as output:

+       error = gpiod_direction_output(ts->gpiod_int, ts->client->addr == 0x14);

The gpiod_direction_output() call ends up in drivers/pinctrl/intel/pinctrl-baytrail.c:
/*
 * Before making any direction modifications, do a check if gpio
 * is set for direct IRQ.  On baytrail, setting GPIO to output does
 * not make sense, so let's at least warn the caller before they shoot
 * themselves in the foot.
 */
WARN(readl(conf_reg) & BYT_DIRECT_IRQ_EN,
	"Potential Error: Setting GPIO with direct_irq_en to output");

So the problem comes from using the gpio interrupt pin as output, which 
should not work on Baytrail if BYT_DIRECT_IRQ_EN is set by BIOS.
The above warning is introduced and discussed in [2] and [3]. As I mentioned,
this could be a real HW issue or the BIOS sets BYT_DIRECT_IRQ_EN when
it should not. I have also tested these patches on a Baytrail plarform
(that uses the same pinctrl driver), but I did not see any issues since
BYT_DIRECT_IRQ_EN is not set in my case for the interrupt gpio pin.

To determine if using the interrupt pin as output works, you can test updating
the goodix configuration [4]. Normally if something goes wrong with the reset
procedure, the configuration will not be updated. To make that easier, I have
already included a sysfs interface to dump the current configuration [5] and
a script to help generate a new configuration [6] (with details on how to use it
in the commit message). Please let me know if you need something more to
test this.

Thanks,
Irina

[1] https://drive.google.com/a/intel.com/folderview?id=0BxCVOQS3ZymGfmJyY2RKbE5XbVlKNlktVTlwV0lxNEdxd2dzeWZER094cmJPVnMxN1F0Yzg
[2] https://lkml.org/lkml/2014/6/2/654
[3] https://lkml.org/lkml/2014/9/18/129
[4] https://lkml.org/lkml/2015/9/7/339
[5] https://lkml.org/lkml/2015/9/7/337
[6] https://lkml.org/lkml/2015/7/31/706