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:

quoted

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.

quoted

+config KEYBOARD_APPLESPI
+	tristate "Apple SPI keyboard and trackpad"

quoted

+	depends on ACPI && SPI && EFI

I would rather want to see separate line for SPI...

quoted

+	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]

quoted

+	if (memcmp(applespi->tx_status, status_ok, APPLESPI_STATUS_SIZE)) {

quoted

+		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]

quoted

+static void
+applespi_remap_fn_key(struct keyboard_protocol *keyboard_protocol)
+{
+	unsigned char tmp;

quoted

+	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).

quoted

+
+	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)

quoted

+		__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);
 }

quoted

+}

quoted

+					   applespi->last_keys_fn_pressed[i]);
+		input_report_key(applespi->keyboard_input_dev, key, 0);
+		applespi->last_keys_fn_pressed[i] = 0;
+	}

quoted

+	for (i = 0; i < MAX_MODIFIERS; i++) {

quoted

+		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