Re: [PATCH v2 2/2] Input: add Apple SPI keyboard and trackpad driver.

[Life is hard, and then you die]

On Tue, Feb 26, 2019 at 11:20:59AM +0200, Andy Shevchenko wrote:
> On Thu, Feb 21, 2019 at 02:56:09AM -0800, 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.
> 
> > +config KEYBOARD_APPLESPI
> > +	tristate "Apple SPI keyboard and trackpad"
> 
> > +	depends on ACPI && SPI && EFI
> 
> I would rather want to see separate line for SPI...
> 
> > +	depends on X86 || COMPILE_TEST
> 
> ...like here
> 
> 	depends on SPI

Sure. Generally, what is the criteria/rule here for splitting
conjunctions into separate 'depends'?

[snip]
> + #define DEV(applespi)           (&(applespi)->spi->dev)
[snip]
> > +	if (memcmp(applespi->tx_status, status_ok, APPLESPI_STATUS_SIZE)) {
> 
> > +		dev_warn(DEV(applespi), "Error writing to device: %*ph\n",
> 
> Hmm... DEV() is too generic name for custom macro. And frankly I don't think
> it's good to have in the first place.

Yeah, I've been having trouble coming up with a better (but still
succinct) name - CORE_DEV()? RAW_DEV()? DEV_OF()? However, because
this expression is used in many places throughout the driver (mostly,
but not only, for logging statements) I feel like it's good to factor
it out. But I'll defer to your .

[snip]
> > +static void
> > +applespi_remap_fn_key(struct keyboard_protocol *keyboard_protocol)
> > +{
> > +	unsigned char tmp;
> 
> > +	unsigned long *modifiers =
> > +			(unsigned long *)&keyboard_protocol->modifiers;
> 
> I would leave it on one online despite checkpatch warning (also, instead of
> (unsigned long *) the (void *) might be used as a small trick).
> 
> > +
> > +	if (!fnremap || fnremap > ARRAY_SIZE(applespi_controlcodes) ||
> > +	    !applespi_controlcodes[fnremap - 1])
> > +		return;
> > +
> > +	tmp = keyboard_protocol->fn_pressed;
> > +	keyboard_protocol->fn_pressed = test_bit(fnremap - 1, modifiers);
> > +	if (tmp)
> 
> > +		__set_bit(fnremap - 1, modifiers);
> > +	else
> > +		__clear_bit(fnremap - 1, modifiers);
> 
> Oh, this is not good. modifiers should be really unsigned long bounary,
> otherwise it is potential overflow.
> 
> Best to fix is to define them as unsigned long in the first place.

Can't do that directly, because keyboard_protocol->modifiers is a
field in the data received from the device, i.e. defined by that
protocol. Instead I could make a copy of the modifiers and pass that
around separately (i.e. in addition to the keyboard_protocol struct).

However, the implied size assertions here would basically still apply:

  MAX_MODIFIERS == sizeof(keyboard_protocol->modifiers) * 8
  ARRAY_SIZE(applespi_controlcodes) == sizeof(keyboard_protocol->modifiers) * 8

(hmm, MAX_MODIFIERS is really redundant - getting rid of it...)

Would using compiletime_assert()'s be an acceptable alternate approach
here? It would serve to both document the size constraint and to
protect against overflow due to an error in some future edit. E.g.

 applespi_remap_fn_key(struct keyboard_protocol *keyboard_protocol)
 {
        unsigned char tmp;
        unsigned long *modifiers = (void *)&keyboard_protocol->modifiers;
+
+       compiletime_assert(ARRAY_SIZE(applespi_controlcodes) ==
+                          sizeof_field(struct keyboard_protocol, modifiers) * 8,
+                          "applespi_controlcodes has wrong number of entries");
 
        if (!fnremap || fnremap > ARRAY_SIZE(applespi_controlcodes) ||
            !applespi_controlcodes[fnremap - 1])
 		return;
 
 	tmp = keyboard_protocol->fn_pressed;
 	keyboard_protocol->fn_pressed = test_bit(fnremap - 1, modifiers);
 	if (tmp)
 
 		__set_bit(fnremap - 1, modifiers);
 	else
 		__clear_bit(fnremap - 1, modifiers);
 }

> > +}
> 
> > +					   applespi->last_keys_fn_pressed[i]);
> > +		input_report_key(applespi->keyboard_input_dev, key, 0);
> > +		applespi->last_keys_fn_pressed[i] = 0;
> > +	}
> 
> > +	for (i = 0; i < MAX_MODIFIERS; i++) {
> 
> > +		u8 *modifiers = &keyboard_protocol->modifiers;
> > +
> > +		if (test_bit(i, (unsigned long *)modifiers))
> 
> Oh, this is not good idea, see above.

See above. (I presume duplicating the compiletime_assert() here isn't
necessary, if going that route?)


  Cheers,

  Ronald