Re: [PATCH v3 4/6] ACPI / property: Support Apple _DSM properties

[Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>]

On Fri, 2017-07-14 at 00:36 +0200, Lukas Wunner wrote:
> While the rest of the world has standardized on _DSD as the way to
> store
> device properties in AML (introduced with ACPI 5.1 in 2014), Apple has
> been using a custom _DSM to achieve the same for much longer (ever
> since
> they switched from DeviceTree-based PowerPC to Intel in 2005, verified
> with MacOS X 10.4.11).
> 
> The theory of operation on macOS is as
> follows:  AppleACPIPlatform.kext
> invokes mergeEFIproperties() and mergeDSMproperties() for each device
> to
> merge properties conveyed by EFI drivers as well as properties stored
> in
> AML into the I/O Kit registry from which they can be retrieved by
> drivers.  We've been supporting EFI properties since commit
> 58c5475aba67
> ("x86/efi: Retrieve and assign Apple device properties").  The present
> commit adds support for _DSM properties, thereby completing our
> support
> for Apple device properties.  The _DSM properties are made available
> under the primary fwnode, the EFI properties under the secondary
> fwnode.
> So for devices which possess both property types, they can all be
> elegantly accessed with the uniform API in <linux/property.h>.
> 
> Until recently we had no need to support _DSM properties, they
> contained
> only uninteresting garbage.  The situation has changed with MacBooks
> and
> MacBook Pros introduced since 2015:  Their keyboard is attached with
> SPI
> instead of USB and the _CRS data which is necessary to initialize the
> spi driver only contains valid information if OSPM responds "false" to
> _OSI("Darwin").  If OSPM responds "true", _CRS is empty and the spi
> driver fails to initialize.  The rationale is very simple, Apple only
> cares about macOS and Windows:  On Windows, _CRS contains valid data,
> whereas on macOS it is empty.  Instead, macOS gleans the necessary
> data
> from the _DSM properties.
> 
> Since Linux deliberately defaults to responding "true" to
> _OSI("Darwin"),
> we need to emulate macOS' behaviour by initializing the spi driver
> with
> data returned by the _DSM.
> 
> An out-of-tree driver for the SPI keyboard exists which currently
> binds
> to the ACPI device, invokes the _DSM, parses the returned package and
> instantiates an SPI device with the data gleaned from the _DSM:
> https://github.com/cb22/macbook12-spi-driver/commit/9a416d699ef4
> https://github.com/cb22/macbook12-spi-driver/commit/0c34936ed9a1
> 
> By adding support for Apple's _DSM properties in generic ACPI code,
> the
> out-of-tree driver will be able to register as a regular SPI device,
> significantly reducing its amount of code and improving its chances to
> be mainlined.
> 
> The SPI keyboard will not be the only user of this commit:  E.g. on
> the
> MacBook8,1, the UART-attached Bluetooth device likewise returns empty
> _CRS data if OSPM returns "true" to _OSI("Darwin").
> 
> The _DSM returns a Package whose format unfortunately deviates
> slightly
> from the _DSD spec:  The properties are marshalled up in a single
> Package
> as alternating key/value elements, unlike _DSD which stores them as a
> Package of 2-element Packages.  The present commit therefore converts
> the Package to _DSD format and the ACPI core can then treat the data
> as
> if Apple would follow the standard.
> 
> Well, except for one small annoyance:  The properties returned by the
> _DSM only ever have one of two types, Integer or Buffer.  The former
> is
> retrievable as usual with device_property_read_u64(), but the latter
> is
> not part of the _DSD spec and it is not possible to retrieve Buffer
> properties with the device_property_read_*() functions due to the type
> checking performed in drivers/acpi/property.c.  It is however possible
> to retrieve them with acpi_dev_get_property().  Apple is using the
> Buffer type somewhat sloppily to store null-terminated strings but
> also
> integers.  The real data type is not distinguishable by the ACPI core
> and the onus is on the caller to use the contents of the Buffer in an
> appropriate way.
> 
> In case Apple moves to _DSD in the future, this commit first checks
> for
> _DSD and falls back to _DSM only if _DSD is not found.
> 

Reviewed-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>

(though most of the comments were given on Github page)

> Cc: Federico Lorenzi <florenzi@xxxxxxxxx>
> Cc: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> Tested-by: Ronald Tschalär <ronald@xxxxxxxxxxxxx>
> Acked-by: Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx>
> Signed-off-by: Lukas Wunner <lukas@xxxxxxxxx>
> ---
> Changes v2 -> v3:
> - Use bitmap to keep track of valid properties.  Move to x86/apple.c
>   to avoid cluttering up generic ACPI code. (Andy, Rafael)
> 
>  drivers/acpi/Makefile    |   1 +
>  drivers/acpi/internal.h  |   6 +++
>  drivers/acpi/property.c  |   3 ++
>  drivers/acpi/x86/apple.c | 137
> +++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 147 insertions(+)
>  create mode 100644 drivers/acpi/x86/apple.c
> 
> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
> index b1aacfc62b1f..90265ab4437a 100644
> --- a/drivers/acpi/Makefile
> +++ b/drivers/acpi/Makefile
> @@ -50,6 +50,7 @@ acpi-$(CONFIG_ACPI_REDUCED_HARDWARE_ONLY) += evged.o
>  acpi-y				+= sysfs.o
>  acpi-y				+= property.o
>  acpi-$(CONFIG_X86)		+= acpi_cmos_rtc.o
> +acpi-$(CONFIG_X86)		+= x86/apple.o
>  acpi-$(CONFIG_X86)		+= x86/utils.o
>  acpi-$(CONFIG_DEBUG_FS)		+= debugfs.o
>  acpi-$(CONFIG_ACPI_NUMA)	+= numa.o
> diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
> index be79f7db1850..79ee29777909 100644
> --- a/drivers/acpi/internal.h
> +++ b/drivers/acpi/internal.h
> @@ -229,6 +229,12 @@ static inline void suspend_nvs_restore(void) {}
>  void acpi_init_properties(struct acpi_device *adev);
>  void acpi_free_properties(struct acpi_device *adev);
>  
> +#ifdef CONFIG_X86
> +void acpi_extract_apple_properties(struct acpi_device *adev);
> +#else
> +static inline void acpi_extract_apple_properties(struct acpi_device
> *adev) {}
> +#endif
> +
>  /*-------------------------------------------------------------------
> -------
>  				Watchdog
>    -------------------------------------------------------------------
> ------- */
> diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
> index 27a9294c843c..d05f4f97c963 100644
> --- a/drivers/acpi/property.c
> +++ b/drivers/acpi/property.c
> @@ -375,6 +375,9 @@ void acpi_init_properties(struct acpi_device
> *adev)
>  	if (acpi_of && !adev->flags.of_compatible_ok)
>  		acpi_handle_info(adev->handle,
>  			 ACPI_DT_NAMESPACE_HID " requires
> 'compatible' property\n");
> +
> +	if (is_apple_system && !adev->data.pointer)
> +		acpi_extract_apple_properties(adev);
>  }
>  
>  static void acpi_destroy_nondev_subnodes(struct list_head *list)
> diff --git a/drivers/acpi/x86/apple.c b/drivers/acpi/x86/apple.c
> new file mode 100644
> index 000000000000..bffa38f7460e
> --- /dev/null
> +++ b/drivers/acpi/x86/apple.c
> @@ -0,0 +1,137 @@
> +/*
> + * apple.c - Apple ACPI quirks
> + * Copyright (C) 2017 Lukas Wunner <lukas@xxxxxxxxx>
> + *
> + * This program is free software; you can redistribute it and/or
> modify
> + * it under the terms of the GNU General Public License (version 2)
> as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/bitmap.h>
> +#include <linux/uuid.h>
> +
> +/* Apple _DSM device properties GUID */
> +static const guid_t apple_prp_guid =
> +	GUID_INIT(0xA0B5B7C6, 0x1318, 0x441C,
> +		  0xB0, 0xC9, 0xFE, 0x69, 0x5E, 0xAF, 0x94, 0x9B);
> +
> +/**
> + * acpi_extract_apple_properties - retrieve and convert Apple _DSM
> properties
> + * @adev: ACPI device for which to retrieve the properties
> + *
> + * Invoke Apple's custom _DSM once to check the protocol version and
> once more
> + * to retrieve the properties.  They are marshalled up in a single
> package as
> + * alternating key/value elements, unlike _DSD which stores them as a
> package
> + * of 2-element packages.  Convert to _DSD format and make them
> available under
> + * the primary fwnode.
> + */
> +void acpi_extract_apple_properties(struct acpi_device *adev)
> +{
> +	unsigned int i, j = 0, newsize = 0, numprops, numvalid;
> +	union acpi_object *props, *newprops;
> +	unsigned long *valid = NULL;
> +	void *free_space;
> +
> +	props = acpi_evaluate_dsm_typed(adev->handle,
> &apple_prp_guid, 1, 0,
> +					NULL, ACPI_TYPE_BUFFER);
> +	if (!props)
> +		return;
> +
> +	if (!props->buffer.length)
> +		goto out_free;
> +
> +	if (props->buffer.pointer[0] != 3) {
> +		acpi_handle_info(adev->handle, FW_INFO
> +				 "unsupported properties version
> %*ph\n",
> +				 props->buffer.length, props-
> >buffer.pointer);
> +		goto out_free;
> +	}
> +
> +	ACPI_FREE(props);
> +	props = acpi_evaluate_dsm_typed(adev->handle,
> &apple_prp_guid, 1, 1,
> +					NULL, ACPI_TYPE_PACKAGE);
> +	if (!props)
> +		return;
> +
> +	numprops = props->package.count / 2;
> +	if (!numprops)
> +		goto out_free;
> +
> +	valid = kcalloc(BITS_TO_LONGS(numprops), sizeof(long),
> GFP_KERNEL);
> +	if (!valid)
> +		goto out_free;
> +
> +	/* newsize = key length + value length of each tuple */
> +	for (i = 0; i < numprops; i++) {
> +		union acpi_object *key = &props->package.elements[i *
> 2];
> +		union acpi_object *val = &props->package.elements[i *
> 2 + 1];
> +
> +		if ( key->type != ACPI_TYPE_STRING ||
> +		    (val->type != ACPI_TYPE_INTEGER &&
> +		     val->type != ACPI_TYPE_BUFFER))
> +			continue; /* skip invalid properties */
> +
> +		__set_bit(i, valid);
> +		newsize += key->string.length + 1;
> +		if ( val->type == ACPI_TYPE_BUFFER)
> +			newsize += val->buffer.length;
> +	}
> +
> +	numvalid = bitmap_weight(valid, numprops);
> +	if (numprops > numvalid)
> +		acpi_handle_info(adev->handle, FW_INFO
> +				 "skipped %u properties: wrong
> type\n",
> +				 numprops - numvalid);
> +	if (numvalid == 0)
> +		goto out_free;
> +
> +	/* newsize += top-level package + 3 objects for each
> key/value tuple */
> +	newsize	+= (1 + 3 * numvalid) * sizeof(union
> acpi_object);
> +	newprops = ACPI_ALLOCATE_ZEROED(newsize);
> +	if (!newprops)
> +		goto out_free;
> +
> +	/* layout: top-level package | packages | key/value tuples |
> strings */
> +	newprops->type = ACPI_TYPE_PACKAGE;
> +	newprops->package.count = numvalid;
> +	newprops->package.elements = &newprops[1];
> +	free_space = &newprops[1 + 3 * numvalid];
> +
> +	for_each_set_bit(i, valid, numprops) {
> +		union acpi_object *key = &props->package.elements[i *
> 2];
> +		union acpi_object *val = &props->package.elements[i *
> 2 + 1];
> +		unsigned int k = 1 + numvalid + j * 2; /* index into
> newprops */
> +		unsigned int v = k + 1;
> +
> +		newprops[1 + j].type = ACPI_TYPE_PACKAGE;
> +		newprops[1 + j].package.count = 2;
> +		newprops[1 + j].package.elements = &newprops[k];
> +
> +		newprops[k].type = ACPI_TYPE_STRING;
> +		newprops[k].string.length = key->string.length;
> +		newprops[k].string.pointer = free_space;
> +		memcpy(free_space, key->string.pointer, key-
> >string.length);
> +		free_space += key->string.length + 1;
> +
> +		newprops[v].type = val->type;
> +		if (val->type == ACPI_TYPE_INTEGER) {
> +			newprops[v].integer.value = val-
> >integer.value;
> +		} else {
> +			newprops[v].buffer.length = val-
> >buffer.length;
> +			newprops[v].buffer.pointer = free_space;
> +			memcpy(free_space, val->buffer.pointer,
> +			       val->buffer.length);
> +			free_space += val->buffer.length;
> +		}
> +		j++; /* count valid properties */
> +	}
> +	WARN_ON(free_space != (void *)newprops + newsize);
> +
> +	adev->data.properties = newprops;
> +	adev->data.pointer = newprops;
> +
> +out_free:
> +	ACPI_FREE(props);
> +	kfree(valid);
> +}

-- 
Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
Intel Finland Oy
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html