linux-kernel - Re: [PATCH v7 2/2] Input: add Apple SPI keyboard and trackpad driver.

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 for Android: free password hash cracker in your pocket

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

Date:   Wed, 17 Jul 2019 03:24:47 -0700
From:   "Life is hard, and then you die" <ronald@...ovation.ch>
To:     Dmitry Torokhov <dmitry.torokhov@...il.com>
Cc:     Henrik Rydberg <rydberg@...math.org>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Andrzej Hajda <a.hajda@...sung.com>,
        Inki Dae <inki.dae@...sung.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Lukas Wunner <lukas@...ner.de>,
        Federico Lorenzi <federico@...velground.com>,
        Laurent Pinchart <laurent.pinchart@...asonboard.com>,
        linux-input@...r.kernel.org, dri-devel@...ts.freedesktop.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v7 2/2] Input: add Apple SPI keyboard and trackpad driver.


  Hi Dmitry,

thanks for taking a look at this!

On Tue, Jul 16, 2019 at 08:47:44PM +0200, Dmitry Torokhov wrote:
> Hi Ronald,
> 
> On Fri, Apr 19, 2019 at 01:19:26AM -0700, Ronald Tschalär wrote:
> > The keyboard and trackpad on recent MacBook's (since 8,1) and
> > MacBookPro's (13,* and 14,*) are attached to an SPI controller instead
> > of USB, as previously. The higher level protocol is not publicly
> > documented and hence has been reverse engineered. As a consequence there
> > are still a number of unknown fields and commands. However, the known
> > parts have been working well and received extensive testing and use.
> > 
> > In order for this driver to work, the proper SPI drivers need to be
> > loaded too; for MB8,1 these are spi_pxa2xx_platform and spi_pxa2xx_pci;
> > for all others they are spi_pxa2xx_platform and intel_lpss_pci. For this
> > reason enabling this driver in the config implies enabling the above
> > drivers.
> 
> I applied the patch (but changed __u8 to u8 as that's preferred form for
> inside the kernel, and added error handling for input_mt_init_slots) but
> we need to do some more work on the driver.

Looks good.

> My main issue is with registering touchpad device asynchronously,
> independent from the probe() function. This means (as far as I can tell)
> that any error is not really appropriately handled (as by that time it
> is too late to signal errors from probe()) and devm functions are not
> going to be called, leaving remnants of the resources in memory on
> driver unload. It also brings in issues with suspend/resume (what
> happens if you suspend really quickly while device is not registered
> yet?), etc, etc.

Yes, the lack of error propagation also bothered me a bit when I
introduced the dev-info command, but I thought doing synchronous
I/O operations in a probe function wasn't kosher. Happy to rectify
that though.

> Can we switch to calling DEV_INFO command synchronously from probe()? If
> we are concerned about it taking relatively long time we can always
> annotate the driver as having probe_type = PROBE_PREFER_ASYNCHRONOUS so
> that other devices can be probed simultaneously with applespi.

Normally the dev-info retrieval takes about 15ms - I presume that's
fast enough?

Attached is a patch that does this now (on top of your changes above).
The cancelling of outstanding spi requests in the error case is a bit
ugly (I wish there were an exported spi-flush/spi-wait-for-queue-empty
function or similar), but otherwise it's fairly straightforward.


  Cheers,

  Ronald


View attachment "0001-Input-applespi-register-touchpad-device-synchronousl.patch" of type "text/plain" (7794 bytes)