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

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Tue, Feb 26, 2019 at 11:29:58PM -0800, Life is hard, and then you die wrote:
> 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:

> > > +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'?

Rule of common sense.

For example UEFI and ACPI may have some relations, SPI and ACPI kinda
orthogonal.

> > + #define DEV(applespi)           (&(applespi)->spi->dev)

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

Please remove this macro for good. Otherwise big subsystems / drivers usually do something like

#define foo_err(...)	dev_err(...)
...

Don't know if it would help here, the driver is standalone and not so big.

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

Perhaps, simple

	__set_bit(b, x)		->	x |= BIT(b);
	__clear_bit(b, x)	->	x &= ~BIT(b);

?

> > > +}

-- 
With Best Regards,
Andy Shevchenko