[PATCH 0/3] Apple SPI properties

[PATCH 0/3] Apple SPI properties

[Lukas Wunner]

Invoke an Apple-specific _DSM on ACPI scan, convert the resulting device
properties to _DSD format for further consumption by the ACPI core and
take advantage of these properties when setting up an SPI slave if _CRS
data is empty.

The series intends to pave the way for mainlining an out-of-tree driver
for the SPI keyboard built into recent MacBooks and MacBook Pros:
https://github.com/cb22/macbook12-spi-driver/

It follows up on a discussion on linux-acpi@ in March 2016.  This post
contained an excerpt of the DSDT of an affected MacBook:
https://www.spinics.net/lists/linux-spi/msg06964.html

I tested the ACPI patches extensively on my own machine.  The SPI patch
however needs a Tested-by from one of the developers of the out-of-tree
driver (cc: Ronald, Federico).  The SPI patch compiles without the ACPI
patches, so need not go in through the same tree.

Special thanks to Andy Shevchenko for doing some early reviewing of the
patches on GitHub.

Kind regards,

Lukas


Lukas Wunner (3):
  ACPI / property: Don't evaluate objects for devices w/o handle
  ACPI / property: Support Apple _DSM properties
  spi: Use Apple device properties in absence of ACPI resources

 drivers/acpi/property.c | 123 ++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/spi/spi.c       |  30 ++++++++++++
 2 files changed, 153 insertions(+)

-- 
2.11.0

[PATCH 1/3] ACPI / property: Don't evaluate objects for devices w/o handle

[Lukas Wunner]

Fabricated devices such as LNXPWRBN lack a handle, causing evaluation
of _CCA and _DSD to always fail with AE_BAD_PARAMETER.  While that is
merely a (negligible) waste of processing power, evaluating a _DSM for
them (such as Apple's device properties _DSM which we're about to add)
results in an ugly error:

    ACPI: \: failed to evaluate _DSM (0x1001)

Avoid by not evaluating _DSD and the upcoming _DSM for devices without
handle.

Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/acpi/property.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
index 9364398204e9..27a9294c843c 100644
--- a/drivers/acpi/property.c
+++ b/drivers/acpi/property.c
@@ -338,6 +338,9 @@ void acpi_init_properties(struct acpi_device *adev)
 
 	INIT_LIST_HEAD(&adev->data.subnodes);
 
+	if (!adev->handle)
+		return;
+
 	/*
 	 * Check if ACPI_DT_NAMESPACE_HID is present and inthat case we fill in
 	 * Device Tree compatible properties for this device.
-- 
2.11.0

[PATCH 2/3] ACPI / property: Support Apple _DSM properties

[Lukas Wunner]

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.

Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Federico Lorenzi <florenzi@gmail.com>
Cc: Ronald Tschalär <ronald@innovation.ch>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/acpi/property.c | 120 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 120 insertions(+)

diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
index 27a9294c843c..545188207b8d 100644
--- a/drivers/acpi/property.c
+++ b/drivers/acpi/property.c
@@ -15,6 +15,7 @@
 
 #include <linux/acpi.h>
 #include <linux/device.h>
+#include <linux/dmi.h>
 #include <linux/export.h>
 
 #include "internal.h"
@@ -34,6 +35,121 @@ static const u8 ads_uuid[16] = {
 	0xe6, 0xe3, 0xb8, 0xdb, 0x86, 0x58, 0xa6, 0x4b,
 	0x87, 0x95, 0x13, 0x19, 0xf5, 0x2a, 0x96, 0x6b
 };
+/* Apple _DSM device properties GUID: a0b5b7c6-1318-441c-b0c9-fe695eaf949b */
+static const u8 apple_prp_uuid[16] = {
+	0xc6, 0xb7, 0xb5, 0xa0, 0x18, 0x13, 0x1c, 0x44,
+	0xb0, 0xc9, 0xfe, 0x69, 0x5e, 0xaf, 0x94, 0x9b
+};
+
+/**
+ * acpi_retrieve_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.
+ */
+static void acpi_retrieve_apple_properties(struct acpi_device *adev)
+{
+	unsigned int i, j, version, newsize = 0, numprops, skipped = 0;
+	union acpi_object *props, *newprops;
+	void *free_space;
+
+	props = acpi_evaluate_dsm_typed(adev->handle, apple_prp_uuid, 1, 0,
+					NULL, ACPI_TYPE_BUFFER);
+	if (!props || !props->buffer.length)
+		goto out_free;
+
+	version = props->buffer.pointer[0];
+	ACPI_FREE(props);
+	if (version != 3) {
+		acpi_handle_err(adev->handle,
+				"unsupported properties version %u\n", version);
+		return;
+	}
+
+	props = acpi_evaluate_dsm_typed(adev->handle, apple_prp_uuid, 1, 1,
+					NULL, ACPI_TYPE_PACKAGE);
+	if (!props)
+		return;
+
+	/* newsize = key length + value length of each tuple */
+	numprops = props->package.count / 2;
+	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)) {
+			key->type = ACPI_TYPE_ANY; /* mark as to be skipped */
+			skipped++;
+			continue;
+		}
+		newsize += key->string.length + 1;
+		if ( val->type == ACPI_TYPE_BUFFER)
+			newsize += val->buffer.length;
+	}
+
+	if (skipped)
+		acpi_handle_err(adev->handle,
+				"skipped %u properties: wrong type\n", skipped);
+	if (skipped == numprops)
+		goto out_free;
+
+	/* newsize += top-level package + 3 objects for each key/value tuple */
+	newsize	+= (1 + 3 * (numprops - skipped)) * 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	   = numprops - skipped;
+	newprops->package.elements = &newprops[1];
+	free_space		   = &newprops[1 + 3 * (numprops - skipped)];
+
+	for (i = 0, j = 0; i < numprops; i++) {
+		union acpi_object *key = &props->package.elements[i * 2];
+		union acpi_object *val = &props->package.elements[i * 2 + 1];
+		unsigned int k	       = (1 + numprops - skipped) + j * 2;
+		unsigned int v	       = k + 1; /* index into newprops */
+
+		if (key->type == ACPI_TYPE_ANY)
+			continue; /* skipped */
+
+		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++; /* not incremented for skipped properties */
+	}
+	WARN_ON(free_space != (void *)newprops + newsize);
+
+	adev->data.properties = newprops;
+	adev->data.pointer = newprops;
+
+out_free:
+	ACPI_FREE(props);
+}
 
 static bool acpi_enumerate_nondev_subnodes(acpi_handle scope,
 					   const union acpi_object *desc,
@@ -375,6 +491,10 @@ 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 (!adev->data.pointer && IS_ENABLED(CONFIG_X86) &&
+	    dmi_match(DMI_SYS_VENDOR, "Apple Inc."))
+		acpi_retrieve_apple_properties(adev);
 }
 
 static void acpi_destroy_nondev_subnodes(struct list_head *list)
-- 
2.11.0

[PATCH 3/3] spi: Use Apple device properties in absence of ACPI resources

[Lukas Wunner]

MacBooks and MacBook Pros introduced since 2015 return empty _CRS data
for SPI slaves, causing device initialization to fail.  Most of the
information that would normally be conveyed via _CRS is available
through ACPI device properties instead, so take advantage of them.

The meaning and appropriate usage of the device properties was reverse
engineered by Ronald Tschalär and carried over from these commits
authored by him:

https://github.com/cb22/macbook12-spi-driver/commit/9a416d699ef4
https://github.com/cb22/macbook12-spi-driver/commit/0c34936ed9a1

According to Ronald, the device properties have the following meaning:

spiSclkPeriod   /* period in ns */
spiWordSize     /* in number of bits */
spiBitOrder     /* 1 = MSB_FIRST, 0 = LSB_FIRST */
spiSPO          /* clock polarity: 0 = low, 1 = high */
spiSPH          /* clock phase: 0 = first, 1 = second */
spiCSDelay      /* delay between cs and receive on reads in 10 us */
resetA2RUsec    /* active-to-receive delay? */
resetRecUsec    /* receive delay? */

Cc: Mark Brown <broonie@kernel.org>
Cc: Federico Lorenzi <florenzi@gmail.com>
Cc: Ronald Tschalär <ronald@innovation.ch>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/spi/spi.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 89254a55eb2e..a4066b75d02b 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -21,6 +21,8 @@
 #include <linux/cache.h>
 #include <linux/dma-mapping.h>
 #include <linux/dmaengine.h>
+#include <linux/dmi.h>
+#include <linux/math64.h>
 #include <linux/mutex.h>
 #include <linux/of_device.h>
 #include <linux/of_irq.h>
@@ -1685,6 +1687,29 @@ static void of_register_spi_devices(struct spi_master *master) { }
 #endif
 
 #ifdef CONFIG_ACPI
+static void acpi_spi_parse_apple_properties(struct spi_device *spi)
+{
+	struct acpi_device *dev = ACPI_COMPANION(&spi->dev);
+	const union acpi_object *o;
+
+	if (!acpi_dev_get_property(dev, "spiSclkPeriod", ACPI_TYPE_BUFFER, &o))
+		spi->max_speed_hz  = div64_u64(NSEC_PER_SEC,
+					       *(u64 *)o->buffer.pointer);
+
+	if (!acpi_dev_get_property(dev, "spiWordSize", ACPI_TYPE_BUFFER, &o))
+		spi->bits_per_word = *(u64 *)o->buffer.pointer;
+
+	if (!acpi_dev_get_property(dev, "spiBitOrder", ACPI_TYPE_BUFFER, &o) &&
+	    !*(u64 *)o->buffer.pointer)
+		spi->mode |= SPI_LSB_FIRST;
+	if (!acpi_dev_get_property(dev, "spiSPO", ACPI_TYPE_BUFFER, &o) &&
+	     *(u64 *)o->buffer.pointer)
+		spi->mode |= SPI_CPOL;
+	if (!acpi_dev_get_property(dev, "spiSPH", ACPI_TYPE_BUFFER, &o) &&
+	     *(u64 *)o->buffer.pointer)
+		spi->mode |= SPI_CPHA;
+}
+
 static int acpi_spi_add_resource(struct acpi_resource *ares, void *data)
 {
 	struct spi_device *spi = data;
@@ -1758,6 +1783,11 @@ static acpi_status acpi_register_spi_device(struct spi_master *master,
 				     acpi_spi_add_resource, spi);
 	acpi_dev_free_resource_list(&resource_list);
 
+	/* Zero ACPI resources?  Try Apple device properties instead. */
+	if (!ret && IS_ENABLED(CONFIG_X86) &&
+	    dmi_match(DMI_SYS_VENDOR, "Apple Inc."))
+		acpi_spi_parse_apple_properties(spi);
+
 	if (ret < 0 || !spi->max_speed_hz) {
 		spi_dev_put(spi);
 		return AE_OK;
-- 
2.11.0

Re: [PATCH 2/3] ACPI / property: Support Apple _DSM properties

[Rafael J. Wysocki]

On Wednesday, June 21, 2017 08:05:53 PM 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.
> 
> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Federico Lorenzi <florenzi@gmail.com>
> Cc: Ronald Tschalär <ronald@innovation.ch>
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> ---
>  drivers/acpi/property.c | 120 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 120 insertions(+)
> 
> diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
> index 27a9294c843c..545188207b8d 100644
> --- a/drivers/acpi/property.c
> +++ b/drivers/acpi/property.c
> @@ -15,6 +15,7 @@
>  
>  #include <linux/acpi.h>
>  #include <linux/device.h>
> +#include <linux/dmi.h>
>  #include <linux/export.h>
>  
>  #include "internal.h"
> @@ -34,6 +35,121 @@ static const u8 ads_uuid[16] = {
>  	0xe6, 0xe3, 0xb8, 0xdb, 0x86, 0x58, 0xa6, 0x4b,
>  	0x87, 0x95, 0x13, 0x19, 0xf5, 0x2a, 0x96, 0x6b
>  };
> +/* Apple _DSM device properties GUID: a0b5b7c6-1318-441c-b0c9-fe695eaf949b */
> +static const u8 apple_prp_uuid[16] = {
> +	0xc6, 0xb7, 0xb5, 0xa0, 0x18, 0x13, 0x1c, 0x44,
> +	0xb0, 0xc9, 0xfe, 0x69, 0x5e, 0xaf, 0x94, 0x9b
> +};
> +
> +/**
> + * acpi_retrieve_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.
> + */
> +static void acpi_retrieve_apple_properties(struct acpi_device *adev)
> +{
> +	unsigned int i, j, version, newsize = 0, numprops, skipped = 0;
> +	union acpi_object *props, *newprops;
> +	void *free_space;
> +
> +	props = acpi_evaluate_dsm_typed(adev->handle, apple_prp_uuid, 1, 0,
> +					NULL, ACPI_TYPE_BUFFER);

The handling of UUIDs is going to change in 4.13, so this needs to be rebased.

Thanks,
Rafael

Re: [PATCH 2/3] ACPI / property: Support Apple _DSM properties

[Mika Westerberg]

On Wed, Jun 21, 2017 at 08:05:53PM +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.
> 
> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Federico Lorenzi <florenzi@gmail.com>
> Cc: Ronald Tschalär <ronald@innovation.ch>
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> ---
>  drivers/acpi/property.c | 120 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 120 insertions(+)
> 
> diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
> index 27a9294c843c..545188207b8d 100644
> --- a/drivers/acpi/property.c
> +++ b/drivers/acpi/property.c
> @@ -15,6 +15,7 @@
>  
>  #include <linux/acpi.h>
>  #include <linux/device.h>
> +#include <linux/dmi.h>
>  #include <linux/export.h>
>  
>  #include "internal.h"
> @@ -34,6 +35,121 @@ static const u8 ads_uuid[16] = {
>  	0xe6, 0xe3, 0xb8, 0xdb, 0x86, 0x58, 0xa6, 0x4b,
>  	0x87, 0x95, 0x13, 0x19, 0xf5, 0x2a, 0x96, 0x6b
>  };
> +/* Apple _DSM device properties GUID: a0b5b7c6-1318-441c-b0c9-fe695eaf949b */
> +static const u8 apple_prp_uuid[16] = {
> +	0xc6, 0xb7, 0xb5, 0xa0, 0x18, 0x13, 0x1c, 0x44,
> +	0xb0, 0xc9, 0xfe, 0x69, 0x5e, 0xaf, 0x94, 0x9b
> +};
> +
> +/**
> + * acpi_retrieve_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.
> + */
> +static void acpi_retrieve_apple_properties(struct acpi_device *adev)
> +{
> +	unsigned int i, j, version, newsize = 0, numprops, skipped = 0;
> +	union acpi_object *props, *newprops;
> +	void *free_space;
> +
> +	props = acpi_evaluate_dsm_typed(adev->handle, apple_prp_uuid, 1, 0,
> +					NULL, ACPI_TYPE_BUFFER);
> +	if (!props || !props->buffer.length)
> +		goto out_free;
> +
> +	version = props->buffer.pointer[0];
> +	ACPI_FREE(props);
> +	if (version != 3) {
> +		acpi_handle_err(adev->handle,
> +				"unsupported properties version %u\n", version);

I don't think this is error. More like debug if anything.

> +		return;
> +	}
> +
> +	props = acpi_evaluate_dsm_typed(adev->handle, apple_prp_uuid, 1, 1,
> +					NULL, ACPI_TYPE_PACKAGE);
> +	if (!props)
> +		return;
> +
> +	/* newsize = key length + value length of each tuple */
> +	numprops = props->package.count / 2;
> +	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)) {
> +			key->type = ACPI_TYPE_ANY; /* mark as to be skipped */
> +			skipped++;
> +			continue;
> +		}
> +		newsize += key->string.length + 1;
> +		if ( val->type == ACPI_TYPE_BUFFER)
> +			newsize += val->buffer.length;
> +	}
> +
> +	if (skipped)
> +		acpi_handle_err(adev->handle,
> +				"skipped %u properties: wrong type\n", skipped);

Same here.

> +	if (skipped == numprops)
> +		goto out_free;
> +
> +	/* newsize += top-level package + 3 objects for each key/value tuple */
> +	newsize	+= (1 + 3 * (numprops - skipped)) * 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	   = numprops - skipped;
> +	newprops->package.elements = &newprops[1];
> +	free_space		   = &newprops[1 + 3 * (numprops - skipped)];

I would prefer you you don't align the assignments:

	/* layout: top-level package | packages | key/value tuples | strings */
	newprops->type = ACPI_TYPE_PACKAGE;
	newprops->package.count = numprops - skipped;
	newprops->package.elements = &newprops[1];
	free_space = &newprops[1 + 3 * (numprops - skipped)];

This is consistent with the rest of the file.

> +
> +	for (i = 0, j = 0; i < numprops; i++) {
> +		union acpi_object *key = &props->package.elements[i * 2];
> +		union acpi_object *val = &props->package.elements[i * 2 + 1];
> +		unsigned int k	       = (1 + numprops - skipped) + j * 2;
> +		unsigned int v	       = k + 1; /* index into newprops */
> +
> +		if (key->type == ACPI_TYPE_ANY)
> +			continue; /* skipped */
> +
> +		newprops[1 + j].type		   = ACPI_TYPE_PACKAGE;
> +		newprops[1 + j].package.count	   = 2;
> +		newprops[1 + j].package.elements   = &newprops[k];

Ditto.

> +
> +		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++; /* not incremented for skipped properties */
> +	}
> +	WARN_ON(free_space != (void *)newprops + newsize);

Again, there is not need to scare the user if we can't parse these. We
can log an error here and give up but definitely no need to trigger
backtrace and register dump.

> +
> +	adev->data.properties = newprops;
> +	adev->data.pointer = newprops;
> +
> +out_free:
> +	ACPI_FREE(props);
> +}
>  
>  static bool acpi_enumerate_nondev_subnodes(acpi_handle scope,
>  					   const union acpi_object *desc,
> @@ -375,6 +491,10 @@ 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 (!adev->data.pointer && IS_ENABLED(CONFIG_X86) &&
> +	    dmi_match(DMI_SYS_VENDOR, "Apple Inc."))
> +		acpi_retrieve_apple_properties(adev);

I think it looks nicer if you move all those conditionals to
acpi_retrieve_apple_properties() and then do:

	if (!adev->data.pointer)
		acpi_retrieve_apple_properties(adev);

Re: [PATCH 3/3] spi: Use Apple device properties in absence of ACPI resources

[Mika Westerberg]

On Wed, Jun 21, 2017 at 08:05:53PM +0200, Lukas Wunner wrote:
> MacBooks and MacBook Pros introduced since 2015 return empty _CRS data
> for SPI slaves, causing device initialization to fail.  Most of the
> information that would normally be conveyed via _CRS is available
> through ACPI device properties instead, so take advantage of them.
> 
> The meaning and appropriate usage of the device properties was reverse
> engineered by Ronald Tschalär and carried over from these commits
> authored by him:
> 
> https://github.com/cb22/macbook12-spi-driver/commit/9a416d699ef4
> https://github.com/cb22/macbook12-spi-driver/commit/0c34936ed9a1
> 
> According to Ronald, the device properties have the following meaning:
> 
> spiSclkPeriod   /* period in ns */
> spiWordSize     /* in number of bits */
> spiBitOrder     /* 1 = MSB_FIRST, 0 = LSB_FIRST */
> spiSPO          /* clock polarity: 0 = low, 1 = high */
> spiSPH          /* clock phase: 0 = first, 1 = second */
> spiCSDelay      /* delay between cs and receive on reads in 10 us */
> resetA2RUsec    /* active-to-receive delay? */
> resetRecUsec    /* receive delay? */
> 
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Federico Lorenzi <florenzi@gmail.com>
> Cc: Ronald Tschalär <ronald@innovation.ch>
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> ---
>  drivers/spi/spi.c | 30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
> 
> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index 89254a55eb2e..a4066b75d02b 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -21,6 +21,8 @@
>  #include <linux/cache.h>
>  #include <linux/dma-mapping.h>
>  #include <linux/dmaengine.h>
> +#include <linux/dmi.h>
> +#include <linux/math64.h>
>  #include <linux/mutex.h>
>  #include <linux/of_device.h>
>  #include <linux/of_irq.h>
> @@ -1685,6 +1687,29 @@ static void of_register_spi_devices(struct spi_master *master) { }
>  #endif
>  
>  #ifdef CONFIG_ACPI
> +static void acpi_spi_parse_apple_properties(struct spi_device *spi)
> +{
> +	struct acpi_device *dev = ACPI_COMPANION(&spi->dev);
> +	const union acpi_object *o;
> +
> +	if (!acpi_dev_get_property(dev, "spiSclkPeriod", ACPI_TYPE_BUFFER, &o))
> +		spi->max_speed_hz  = div64_u64(NSEC_PER_SEC,
> +					       *(u64 *)o->buffer.pointer);
> +
> +	if (!acpi_dev_get_property(dev, "spiWordSize", ACPI_TYPE_BUFFER, &o))
> +		spi->bits_per_word = *(u64 *)o->buffer.pointer;
> +
> +	if (!acpi_dev_get_property(dev, "spiBitOrder", ACPI_TYPE_BUFFER, &o) &&
> +	    !*(u64 *)o->buffer.pointer)
> +		spi->mode |= SPI_LSB_FIRST;
> +	if (!acpi_dev_get_property(dev, "spiSPO", ACPI_TYPE_BUFFER, &o) &&
> +	     *(u64 *)o->buffer.pointer)
> +		spi->mode |= SPI_CPOL;
> +	if (!acpi_dev_get_property(dev, "spiSPH", ACPI_TYPE_BUFFER, &o) &&
> +	     *(u64 *)o->buffer.pointer)
> +		spi->mode |= SPI_CPHA;
> +}
> +
>  static int acpi_spi_add_resource(struct acpi_resource *ares, void *data)
>  {
>  	struct spi_device *spi = data;
> @@ -1758,6 +1783,11 @@ static acpi_status acpi_register_spi_device(struct spi_master *master,
>  				     acpi_spi_add_resource, spi);
>  	acpi_dev_free_resource_list(&resource_list);
>  
> +	/* Zero ACPI resources?  Try Apple device properties instead. */
> +	if (!ret && IS_ENABLED(CONFIG_X86) &&
> +	    dmi_match(DMI_SYS_VENDOR, "Apple Inc."))
> +		acpi_spi_parse_apple_properties(spi);

Same comment here, move the conditionals to
acpi_spi_parse_apple_properties() and then do

	if (!ret)
		acpi_spi_parse_apple_properties(spi);

Re: [PATCH 3/3] spi: Use Apple device properties in absence of ACPI resources

[Lukas Wunner]

On Thu, Jun 22, 2017 at 11:10:48AM +0300, Mika Westerberg wrote:
> On Wed, Jun 21, 2017 at 08:05:53PM +0200, Lukas Wunner wrote:
> > MacBooks and MacBook Pros introduced since 2015 return empty _CRS data
> > for SPI slaves, causing device initialization to fail.  Most of the
> > information that would normally be conveyed via _CRS is available
> > through ACPI device properties instead, so take advantage of them.
> > 
> > The meaning and appropriate usage of the device properties was reverse
> > engineered by Ronald Tschalär and carried over from these commits
> > authored by him:
> > 
> > https://github.com/cb22/macbook12-spi-driver/commit/9a416d699ef4
> > https://github.com/cb22/macbook12-spi-driver/commit/0c34936ed9a1
> > 
> > According to Ronald, the device properties have the following meaning:
> > 
> > spiSclkPeriod   /* period in ns */
> > spiWordSize     /* in number of bits */
> > spiBitOrder     /* 1 = MSB_FIRST, 0 = LSB_FIRST */
> > spiSPO          /* clock polarity: 0 = low, 1 = high */
> > spiSPH          /* clock phase: 0 = first, 1 = second */
> > spiCSDelay      /* delay between cs and receive on reads in 10 us */
> > resetA2RUsec    /* active-to-receive delay? */
> > resetRecUsec    /* receive delay? */
> > 
> > Cc: Mark Brown <broonie@kernel.org>
> > Cc: Federico Lorenzi <florenzi@gmail.com>
> > Cc: Ronald Tschalär <ronald@innovation.ch>
> > Signed-off-by: Lukas Wunner <lukas@wunner.de>
> > ---
> >  drivers/spi/spi.c | 30 ++++++++++++++++++++++++++++++
> >  1 file changed, 30 insertions(+)
> > 
> > diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> > index 89254a55eb2e..a4066b75d02b 100644
> > --- a/drivers/spi/spi.c
> > +++ b/drivers/spi/spi.c
> > @@ -21,6 +21,8 @@
> >  #include <linux/cache.h>
> >  #include <linux/dma-mapping.h>
> >  #include <linux/dmaengine.h>
> > +#include <linux/dmi.h>
> > +#include <linux/math64.h>
> >  #include <linux/mutex.h>
> >  #include <linux/of_device.h>
> >  #include <linux/of_irq.h>
> > @@ -1685,6 +1687,29 @@ static void of_register_spi_devices(struct spi_master *master) { }
> >  #endif
> >  
> >  #ifdef CONFIG_ACPI
> > +static void acpi_spi_parse_apple_properties(struct spi_device *spi)
> > +{
> > +	struct acpi_device *dev = ACPI_COMPANION(&spi->dev);
> > +	const union acpi_object *o;
> > +
> > +	if (!acpi_dev_get_property(dev, "spiSclkPeriod", ACPI_TYPE_BUFFER, &o))
> > +		spi->max_speed_hz  = div64_u64(NSEC_PER_SEC,
> > +					       *(u64 *)o->buffer.pointer);
> > +
> > +	if (!acpi_dev_get_property(dev, "spiWordSize", ACPI_TYPE_BUFFER, &o))
> > +		spi->bits_per_word = *(u64 *)o->buffer.pointer;
> > +
> > +	if (!acpi_dev_get_property(dev, "spiBitOrder", ACPI_TYPE_BUFFER, &o) &&
> > +	    !*(u64 *)o->buffer.pointer)
> > +		spi->mode |= SPI_LSB_FIRST;
> > +	if (!acpi_dev_get_property(dev, "spiSPO", ACPI_TYPE_BUFFER, &o) &&
> > +	     *(u64 *)o->buffer.pointer)
> > +		spi->mode |= SPI_CPOL;
> > +	if (!acpi_dev_get_property(dev, "spiSPH", ACPI_TYPE_BUFFER, &o) &&
> > +	     *(u64 *)o->buffer.pointer)
> > +		spi->mode |= SPI_CPHA;
> > +}
> > +
> >  static int acpi_spi_add_resource(struct acpi_resource *ares, void *data)
> >  {
> >  	struct spi_device *spi = data;
> > @@ -1758,6 +1783,11 @@ static acpi_status acpi_register_spi_device(struct spi_master *master,
> >  				     acpi_spi_add_resource, spi);
> >  	acpi_dev_free_resource_list(&resource_list);
> >  
> > +	/* Zero ACPI resources?  Try Apple device properties instead. */
> > +	if (!ret && IS_ENABLED(CONFIG_X86) &&
> > +	    dmi_match(DMI_SYS_VENDOR, "Apple Inc."))
> > +		acpi_spi_parse_apple_properties(spi);
> 
> Same comment here, move the conditionals to
> acpi_spi_parse_apple_properties() and then do
> 
> 	if (!ret)
> 		acpi_spi_parse_apple_properties(spi);

Good idea.  Actually, I think I'll just call the function unconditionally
to let it augment or take precedence over _CRS data.

Thanks!

Lukas

Re: [PATCH 2/3] ACPI / property: Support Apple _DSM properties

[Lukas Wunner]

On Thu, Jun 22, 2017 at 01:05:24AM +0200, Rafael J. Wysocki wrote:
> On Wednesday, June 21, 2017 08:05:53 PM Lukas Wunner wrote:
> > +/* Apple _DSM device properties GUID: a0b5b7c6-1318-441c-b0c9-fe695eaf949b */
> > +static const u8 apple_prp_uuid[16] = {
> > +	0xc6, 0xb7, 0xb5, 0xa0, 0x18, 0x13, 0x1c, 0x44,
> > +	0xb0, 0xc9, 0xfe, 0x69, 0x5e, 0xaf, 0x94, 0x9b
> > +};
> > +
> > +/**
> > + * acpi_retrieve_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.
> > + */
> > +static void acpi_retrieve_apple_properties(struct acpi_device *adev)
> > +{
> > +	unsigned int i, j, version, newsize = 0, numprops, skipped = 0;
> > +	union acpi_object *props, *newprops;
> > +	void *free_space;
> > +
> > +	props = acpi_evaluate_dsm_typed(adev->handle, apple_prp_uuid, 1, 0,
> > +					NULL, ACPI_TYPE_BUFFER);
> 
> The handling of UUIDs is going to change in 4.13, so this needs to be rebased.

Right.  The series is based on your "bleeding-edge" branch, which hasn't
merged the "uuid-types" branch into it.  I'll base future revisions on
"bleeding-edge" with "uuid-types" manually merged into it, but then you'll
have to remember not to apply the series until the uuid-types branch
has landed in Linus' tree and you've backmerged Linus' master branch.

Alternatively, you could merge the uuid-types branch.  There are no conflicts
and Andy says it's to be considered immutable:

http://git.infradead.org/users/hch/uuid.git/shortlog/refs/heads/uuid-types

Thanks!

Lukas

Re: [PATCH 3/3] spi: Use Apple device properties in absence of ACPI resources

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 566 bytes --]

On Thu, Jun 22, 2017 at 11:10:48AM +0300, Mika Westerberg wrote:
> On Wed, Jun 21, 2017 at 08:05:53PM +0200, Lukas Wunner wrote:
> > MacBooks and MacBook Pros introduced since 2015 return empty _CRS data
> > for SPI slaves, causing device initialization to fail.  Most of the
> > information that would normally be conveyed via _CRS is available

Please delete unneeded context from mails when replying.  Doing this
makes it much easier to find your reply in the message, helping ensure
it won't be missed by people scrolling through the irrelevant quoted
material.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 2/3] ACPI / property: Support Apple _DSM properties

[Lukas Wunner]

On Thu, Jun 22, 2017 at 11:06:55AM +0300, Mika Westerberg wrote:
> On Wed, Jun 21, 2017 at 08:05:53PM +0200, Lukas Wunner wrote:
> > +	props = acpi_evaluate_dsm_typed(adev->handle, apple_prp_uuid, 1, 0,
> > +					NULL, ACPI_TYPE_BUFFER);
> > +	if (!props || !props->buffer.length)
> > +		goto out_free;
> > +
> > +	version = props->buffer.pointer[0];
> > +	ACPI_FREE(props);
> > +	if (version != 3) {
> > +		acpi_handle_err(adev->handle,
> > +				"unsupported properties version %u\n", version);
> 
> I don't think this is error. More like debug if anything.

It would be good to be able to google for this message to detect if there
are ever Macs out there which return something different than "3" to this
_DSM call.  How about KERN_WARN or KERN_INFO?

The AppleACPIPlatform.kext which shipped with 10.4.11 (2007) already checked
for a return value of 3 and the newest Macs still return "3", so this has
never changed in at least 10 years and the whole issue is thus theoretical.
I just wanted to be compatible to what macOS does, they check for "3" and
abort if the return value is different.


> > +	if (skipped)
> > +		acpi_handle_err(adev->handle,
> > +				"skipped %u properties: wrong type\n", skipped);
> 
> Same here.

This would be a firmware bug.  We've got FW_BUG, FW_WARN, FW_INFO defined in
include/linux/printk.h.  Granted this is not high priority, but FW_WARN or
at least FW_INFO would seem to be justified.


> > +	WARN_ON(free_space != (void *)newprops + newsize);
> 
> Again, there is not need to scare the user if we can't parse these. We
> can log an error here and give up but definitely no need to trigger
> backtrace and register dump.

This is not for parse errors but programming errors.  free_space is a
pointer into the newprops allocation.

If (free_space < newprops + newsize) then the allocation was too large
and we're wasting memory.

If (free_space > newprops + newsize) then the allocation was too small
and we've corrupted memory behind the allocation.

I'm fairly sure the WARN_ON will never trigger but what do I know? :-)


Thanks!

Lukas

Re: [PATCH 2/3] ACPI / property: Support Apple _DSM properties

[Mika Westerberg]

On Thu, Jun 22, 2017 at 11:54:39AM +0200, Lukas Wunner wrote:
> On Thu, Jun 22, 2017 at 11:06:55AM +0300, Mika Westerberg wrote:
> > On Wed, Jun 21, 2017 at 08:05:53PM +0200, Lukas Wunner wrote:
> > > +	props = acpi_evaluate_dsm_typed(adev->handle, apple_prp_uuid, 1, 0,
> > > +					NULL, ACPI_TYPE_BUFFER);
> > > +	if (!props || !props->buffer.length)
> > > +		goto out_free;
> > > +
> > > +	version = props->buffer.pointer[0];
> > > +	ACPI_FREE(props);
> > > +	if (version != 3) {
> > > +		acpi_handle_err(adev->handle,
> > > +				"unsupported properties version %u\n", version);
> > 
> > I don't think this is error. More like debug if anything.
> 
> It would be good to be able to google for this message to detect if there
> are ever Macs out there which return something different than "3" to this
> _DSM call.  How about KERN_WARN or KERN_INFO?

Well, this is something the user might see and then gets confused on the
grounds: is my machine somehow broken?

> The AppleACPIPlatform.kext which shipped with 10.4.11 (2007) already checked
> for a return value of 3 and the newest Macs still return "3", so this has
> never changed in at least 10 years and the whole issue is thus theoretical.
> I just wanted to be compatible to what macOS does, they check for "3" and
> abort if the return value is different.

Yes, we can check for the return value but there is no need to spam
dmesg for this.

> > > +	if (skipped)
> > > +		acpi_handle_err(adev->handle,
> > > +				"skipped %u properties: wrong type\n", skipped);
> > 
> > Same here.
> 
> This would be a firmware bug.  We've got FW_BUG, FW_WARN, FW_INFO defined in
> include/linux/printk.h.  Granted this is not high priority, but FW_WARN or
> at least FW_INFO would seem to be justified.

or debug :-)

> > > +	WARN_ON(free_space != (void *)newprops + newsize);
> > 
> > Again, there is not need to scare the user if we can't parse these. We
> > can log an error here and give up but definitely no need to trigger
> > backtrace and register dump.
> 
> This is not for parse errors but programming errors.  free_space is a
> pointer into the newprops allocation.
> 
> If (free_space < newprops + newsize) then the allocation was too large
> and we're wasting memory.
> 
> If (free_space > newprops + newsize) then the allocation was too small
> and we've corrupted memory behind the allocation.
> 
> I'm fairly sure the WARN_ON will never trigger but what do I know? :-)

Fair enough.

Re: [PATCH 2/3] ACPI / property: Support Apple _DSM properties

[Lukas Wunner]

On Thu, Jun 22, 2017 at 01:04:22PM +0300, Mika Westerberg wrote:
> On Thu, Jun 22, 2017 at 11:54:39AM +0200, Lukas Wunner wrote:
> > On Thu, Jun 22, 2017 at 11:06:55AM +0300, Mika Westerberg wrote:
> > > On Wed, Jun 21, 2017 at 08:05:53PM +0200, Lukas Wunner wrote:
> > > > +	props = acpi_evaluate_dsm_typed(adev->handle, apple_prp_uuid, 1, 0,
> > > > +					NULL, ACPI_TYPE_BUFFER);
> > > > +	if (!props || !props->buffer.length)
> > > > +		goto out_free;
> > > > +
> > > > +	version = props->buffer.pointer[0];
> > > > +	ACPI_FREE(props);
> > > > +	if (version != 3) {
> > > > +		acpi_handle_err(adev->handle,
> > > > +				"unsupported properties version %u\n", version);
> > > 
> > > I don't think this is error. More like debug if anything.
> > 
> > It would be good to be able to google for this message to detect if there
> > are ever Macs out there which return something different than "3" to this
> > _DSM call.  How about KERN_WARN or KERN_INFO?
> 
> Well, this is something the user might see and then gets confused on the
> grounds: is my machine somehow broken?

And indeed it might be:  Conceivably, Apple may change the format of the
properties and then once again the SPI slave won't be able to initialize.
In that case it seems beneficial to provide a clue as to the cause in dmesg.

Thanks,

Lukas

Re: [PATCH 2/3] ACPI / property: Support Apple _DSM properties

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 847 bytes --]

On Thu, Jun 22, 2017 at 01:24:28PM +0200, Lukas Wunner wrote:
> On Thu, Jun 22, 2017 at 01:04:22PM +0300, Mika Westerberg wrote:
> > On Thu, Jun 22, 2017 at 11:54:39AM +0200, Lukas Wunner wrote:

> > > It would be good to be able to google for this message to detect if there
> > > are ever Macs out there which return something different than "3" to this
> > > _DSM call.  How about KERN_WARN or KERN_INFO?

> > Well, this is something the user might see and then gets confused on the
> > grounds: is my machine somehow broken?

> And indeed it might be:  Conceivably, Apple may change the format of the
> properties and then once again the SPI slave won't be able to initialize.
> In that case it seems beneficial to provide a clue as to the cause in dmesg.

This seems especially important when we're working with a reverse
engineered binding.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 2/3] ACPI / property: Support Apple _DSM properties

[Rafael J. Wysocki]

On Thursday, June 22, 2017 10:57:10 AM Lukas Wunner wrote:
> On Thu, Jun 22, 2017 at 01:05:24AM +0200, Rafael J. Wysocki wrote:
> > On Wednesday, June 21, 2017 08:05:53 PM Lukas Wunner wrote:
> > > +/* Apple _DSM device properties GUID: a0b5b7c6-1318-441c-b0c9-fe695eaf949b */
> > > +static const u8 apple_prp_uuid[16] = {
> > > +	0xc6, 0xb7, 0xb5, 0xa0, 0x18, 0x13, 0x1c, 0x44,
> > > +	0xb0, 0xc9, 0xfe, 0x69, 0x5e, 0xaf, 0x94, 0x9b
> > > +};
> > > +
> > > +/**
> > > + * acpi_retrieve_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.
> > > + */
> > > +static void acpi_retrieve_apple_properties(struct acpi_device *adev)
> > > +{
> > > +	unsigned int i, j, version, newsize = 0, numprops, skipped = 0;
> > > +	union acpi_object *props, *newprops;
> > > +	void *free_space;
> > > +
> > > +	props = acpi_evaluate_dsm_typed(adev->handle, apple_prp_uuid, 1, 0,
> > > +					NULL, ACPI_TYPE_BUFFER);
> > 
> > The handling of UUIDs is going to change in 4.13, so this needs to be rebased.
> 
> Right.  The series is based on your "bleeding-edge" branch, which hasn't
> merged the "uuid-types" branch into it.  I'll base future revisions on
> "bleeding-edge" with "uuid-types" manually merged into it, but then you'll
> have to remember not to apply the series until the uuid-types branch
> has landed in Linus' tree and you've backmerged Linus' master branch.
> 
> Alternatively, you could merge the uuid-types branch.  There are no conflicts
> and Andy says it's to be considered immutable:
> 
> http://git.infradead.org/users/hch/uuid.git/shortlog/refs/heads/uuid-types

Yes, that's what I'm going to do.  I actually have other reasons to do that. :-)

Thanks,
Rafael