[PATCH] serdev: Add ACPI devices by ResourceSource field

[PATCH] serdev: Add ACPI devices by ResourceSource field

[Maximilian Luz]

When registering a serdev controller, ACPI needs to be checked for
devices attached to it. Currently, all immediate children of the ACPI
node of the controller are assumed to be UART client devices for this
controller. Furthermore, these devices are not searched elsewhere.

This is incorrect: Similar to SPI and I2C devices, the UART client
device definition (via UARTSerialBusV2) can reside anywhere in the ACPI
namespace as resource definition inside the _CRS method and points to
the controller via its ResourceSource field. This field may either
contain a fully qualified or relative path, indicating the controller
device. To address this, we need to walk over the whole ACPI namespace,
looking at each resource definition, and match the client device to the
controller via this field.

Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
---
This patch is similar to the the implementations in drivers/spi/spi.c
(see commit 4c3c59544f33e97cf8557f27e05a9904ead16363) and
drivers/i2c/i2c-core-acpi.c. However, I think that there may be an
issues with these two implementations: Both walk over the whole ACPI
namespace, but only match the first SPI or I2C resource (respectively),
so I think there may be problems when multiple SPI or I2C resources are
defined under the same ACPI device node (as in second or third SPI/I2C
resource definitions being ignored). Please note, however, that I am by
no means qualified with regards to this, and this might be totally fine.
Nevertheless I'd appreciate if anyone with more knowledge on the subject
could have a look at it. This patch would avoid this problem (for UART)
by simply walking all resource definitions via acpi_walk_resources.

There is a further issue in the serdev ACPI implementation that this
patch does not address: ACPI UART resource definitions contain things
like the initial baud-rate, parity, flow-control, etc. As far as I know,
these things can currently only be set once the device is opened.
Furthermore, some option values, such as ParityTypeMark, are not (yet)
supported. I'd be willing to try and implement setting the currently
supported values based on ACPI for a future patch, if anyone can provide
me with some pointers on how to do that.

I have personally tested this patch on a Microsoft Surface Book 2, which
like all newer MS Surface devices has a UART EC, and it has been in use
(in some form or another) for a couple of months on other Surface
devices via a patched kernel [1, 2, 3]. I can, however, not speak for
any non-Microsoft devices or potential Apple ACPI quirks.

[1]: https://github.com/jakeday/linux-surface/
[2]: https://github.com/qzed/linux-surface/
[3]: https://github.com/qzed/linux-surfacegen5-acpi/

 drivers/tty/serdev/core.c | 64 ++++++++++++++++++++++++++++++++++-----
 1 file changed, 56 insertions(+), 8 deletions(-)

diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
index a0ac16ee6575..1c8360deea77 100644
--- a/drivers/tty/serdev/core.c
+++ b/drivers/tty/serdev/core.c
@@ -582,18 +582,64 @@ static acpi_status acpi_serdev_register_device(struct serdev_controller *ctrl,
 	return AE_OK;
 }
 
-static acpi_status acpi_serdev_add_device(acpi_handle handle, u32 level,
-				       void *data, void **return_value)
+struct acpi_serdev_resource_context {
+	struct serdev_controller *controller;
+	struct acpi_device *device;
+};
+
+static acpi_status
+acpi_serdev_add_device_from_resource(struct acpi_resource *resource, void *data)
 {
-	struct serdev_controller *ctrl = data;
-	struct acpi_device *adev;
+	struct acpi_serdev_resource_context *ctx
+		= (struct acpi_serdev_resource_context *)data;
+	struct acpi_resource_source *ctrl_name;
+	acpi_handle ctrl_handle;
+
+	if (resource->type != ACPI_RESOURCE_TYPE_SERIAL_BUS)
+		return AE_OK;
 
-	if (acpi_bus_get_device(handle, &adev))
+	if (resource->data.common_serial_bus.type
+	    != ACPI_RESOURCE_SERIAL_TYPE_UART)
 		return AE_OK;
 
-	return acpi_serdev_register_device(ctrl, adev);
+	ctrl_name = &resource->data.common_serial_bus.resource_source;
+	if (ctrl_name->string_length == 0 || !ctrl_name->string_ptr)
+		return AE_OK;
+
+	if (acpi_get_handle(ctx->device->handle, ctrl_name->string_ptr,
+			    &ctrl_handle))
+		return AE_OK;
+
+	if (ctrl_handle == ACPI_HANDLE(ctx->controller->dev.parent))
+		return acpi_serdev_register_device(ctx->controller,
+						   ctx->device);
+
+	return AE_OK;
 }
 
+static acpi_status
+acpi_serdev_add_devices_from_resources(acpi_handle handle, u32 level,
+				       void *data, void **return_value)
+{
+	struct acpi_serdev_resource_context ctx;
+	acpi_status status;
+
+	ctx.controller = (struct serdev_controller *)data;
+	status = acpi_bus_get_device(handle, &ctx.device);
+	if (status)
+		return AE_OK;		// ignore device if not present
+
+	status = acpi_walk_resources(handle, METHOD_NAME__CRS,
+				     acpi_serdev_add_device_from_resource,
+				     &ctx);
+	if (status == AE_NOT_FOUND)
+		return AE_OK;		// ignore if _CRS is not found
+	else
+		return status;
+}
+
+#define SERDEV_ACPI_ENUMERATE_MAX_DEPTH		32
+
 static int acpi_serdev_register_devices(struct serdev_controller *ctrl)
 {
 	acpi_status status;
@@ -603,8 +649,10 @@ static int acpi_serdev_register_devices(struct serdev_controller *ctrl)
 	if (!handle)
 		return -ENODEV;
 
-	status = acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, 1,
-				     acpi_serdev_add_device, NULL, ctrl, NULL);
+	status = acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT,
+				     SERDEV_ACPI_ENUMERATE_MAX_DEPTH,
+				     acpi_serdev_add_devices_from_resources,
+				     NULL, ctrl, NULL);
 	if (ACPI_FAILURE(status))
 		dev_dbg(&ctrl->dev, "failed to enumerate serdev slaves\n");
 
-- 
2.23.0

Re: [PATCH] serdev: Add ACPI devices by ResourceSource field

[Hans de Goede]

Hi Maximilian,

Interesting patch. Some comments about the i2c situation below.

Also I will give this a test-run on some of the existing devices
which rely on the instantiation of serdev devices for ACPI
devices which are childs of the uart device.

On 19-09-2019 21:56, Maximilian Luz wrote:
> When registering a serdev controller, ACPI needs to be checked for
> devices attached to it. Currently, all immediate children of the ACPI
> node of the controller are assumed to be UART client devices for this
> controller. Furthermore, these devices are not searched elsewhere.
> 
> This is incorrect: Similar to SPI and I2C devices, the UART client
> device definition (via UARTSerialBusV2) can reside anywhere in the ACPI
> namespace as resource definition inside the _CRS method and points to
> the controller via its ResourceSource field. This field may either
> contain a fully qualified or relative path, indicating the controller
> device. To address this, we need to walk over the whole ACPI namespace,
> looking at each resource definition, and match the client device to the
> controller via this field.
> 
> Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
> ---
> This patch is similar to the the implementations in drivers/spi/spi.c
> (see commit 4c3c59544f33e97cf8557f27e05a9904ead16363) and
> drivers/i2c/i2c-core-acpi.c. However, I think that there may be an
> issues with these two implementations: Both walk over the whole ACPI
> namespace, but only match the first SPI or I2C resource (respectively),
> so I think there may be problems when multiple SPI or I2C resources are
> defined under the same ACPI device node (as in second or third SPI/I2C
> resource definitions being ignored).

Right, so from the i2c side of things, the story with multiple
I2cSerialBusV2 resources is that normally we want to instantiate
only 1 kernel "struct device" for 1 ACPI "Device()" definition.

If a single I2C chip/device listens on multiple addresses then
usually the other addresses can be derived from the first one and
the device-driver can get a handle to access the other addresses by
using e.g. i2c_new_dummy or i2c_new_secondary_device.

With that said of course there are exceptions where vendors get
creative and put multiple I2cSerialBusV2 resources in a single
ACPI "Device()" even though they point to separate chips.

For this we have some special handling in:
drivers/platform/x86/i2c-multi-instantiate.c

Also note how drivers/platform/x86/i2c-multi-instantiate.c maps the
original ACPI HID as e.g. "BSG1160" to per device match strings,
because if there are multiple I2cSerialBusV2 resources and they
point to separate chips, then we need something to get the right
driver to bind to each I2cSerialBusV2 address, so the normal
modalias of e.g. acpi:BSG1160 is no good, we need a different modalias
for each I2cSerialBusV2 address.

Another way of looking at this is a typical _CRS for a device with
*SerialBusV2 resources will also have 1 or more Interrupt resources
and 1 or more GPIO resources. We are not instantiating separate
"struct device"-s in the kernel for each of those, since all the
resources together describe a single device, so we instantiate e.g.
an i2c_client and then the i2c_driver's probe method calls e.g.
platform_get_irq() to get the IRQ(s).

Given the above I think you may want to also limit your patch to
only instantiate a "struct device" for the first UARTSerialBusV2
in an ACPI "Device()"'s  .

I hope this sheds some clarity on the (muddy) situation wrt
I2cSerialBusV2 handling.

Regards,

Hans

Re: [PATCH] serdev: Add ACPI devices by ResourceSource field

[Maximilian Luz]

Hi,

On 9/20/19 10:50 AM, Hans de Goede wrote:
> Also I will give this a test-run on some of the existing devices
> which rely on the instantiation of serdev devices for ACPI
> devices which are childs of the uart device.

Thank you for testing! Will get to your other mail shortly.

> Given the above I think you may want to also limit your patch to
> only instantiate a "struct device" for the first UARTSerialBusV2
> in an ACPI "Device()"'s  .

Right, I will change this for a v2 once the issue revealed by your
testing has been resolved.

> I hope this sheds some clarity on the (muddy) situation wrt
> I2cSerialBusV2 handling.

Thank you again, this has definitely helped clear things up for me and
your write-up is much appreciated!

Maximilian

Re: [PATCH] serdev: Add ACPI devices by ResourceSource field

[Hans de Goede]

Hi,

On 9/19/19 9:56 PM, Maximilian Luz wrote:
> When registering a serdev controller, ACPI needs to be checked for
> devices attached to it. Currently, all immediate children of the ACPI
> node of the controller are assumed to be UART client devices for this
> controller. Furthermore, these devices are not searched elsewhere.
> 
> This is incorrect: Similar to SPI and I2C devices, the UART client
> device definition (via UARTSerialBusV2) can reside anywhere in the ACPI
> namespace as resource definition inside the _CRS method and points to
> the controller via its ResourceSource field. This field may either
> contain a fully qualified or relative path, indicating the controller
> device. To address this, we need to walk over the whole ACPI namespace,
> looking at each resource definition, and match the client device to the
> controller via this field.
> 
> Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>

So as promised I've given this patch a try, unfortunately it breaks
existing users of ACPI serdev device instantation.

After adding this patch "ls /sys/bus/serial/devices" is empty,
where as before it gives:

[root@dhcp-45-50 ~]# ls -l /sys/bus/serial/devices/
total 0
lrwxrwxrwx. 1 root root 0 Sep 20 16:43 serial0 -> ../../../devices/pci0000:00/8086228A:00/serial0
lrwxrwxrwx. 1 root root 0 Sep 20 16:43 serial0-0 -> ../../../devices/pci0000:00/8086228A:00/serial0/serial0-0

And since the serdev is missing bluetooth does not work.

(ACPI instantiated serdev is used for UART attached Blueooth HCI-s on
many Cherry Trail devices).

I haven't looked why your patch is breakig things, I have a large backlog
so I do not have time for that.

But if you can provide me with a version of the patch with a bunch of
debug printk-s added I'm happy to run that for you.

I'll also send you the DSDT of the device I tested on off-list.

Regards,

Hans




> ---
> This patch is similar to the the implementations in drivers/spi/spi.c
> (see commit 4c3c59544f33e97cf8557f27e05a9904ead16363) and
> drivers/i2c/i2c-core-acpi.c. However, I think that there may be an
> issues with these two implementations: Both walk over the whole ACPI
> namespace, but only match the first SPI or I2C resource (respectively),
> so I think there may be problems when multiple SPI or I2C resources are
> defined under the same ACPI device node (as in second or third SPI/I2C
> resource definitions being ignored). Please note, however, that I am by
> no means qualified with regards to this, and this might be totally fine.
> Nevertheless I'd appreciate if anyone with more knowledge on the subject
> could have a look at it. This patch would avoid this problem (for UART)
> by simply walking all resource definitions via acpi_walk_resources.
> 
> There is a further issue in the serdev ACPI implementation that this
> patch does not address: ACPI UART resource definitions contain things
> like the initial baud-rate, parity, flow-control, etc. As far as I know,
> these things can currently only be set once the device is opened.
> Furthermore, some option values, such as ParityTypeMark, are not (yet)
> supported. I'd be willing to try and implement setting the currently
> supported values based on ACPI for a future patch, if anyone can provide
> me with some pointers on how to do that.
> 
> I have personally tested this patch on a Microsoft Surface Book 2, which
> like all newer MS Surface devices has a UART EC, and it has been in use
> (in some form or another) for a couple of months on other Surface
> devices via a patched kernel [1, 2, 3]. I can, however, not speak for
> any non-Microsoft devices or potential Apple ACPI quirks.
> 
> [1]: https://github.com/jakeday/linux-surface/
> [2]: https://github.com/qzed/linux-surface/
> [3]: https://github.com/qzed/linux-surfacegen5-acpi/
> 
>   drivers/tty/serdev/core.c | 64 ++++++++++++++++++++++++++++++++++-----
>   1 file changed, 56 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
> index a0ac16ee6575..1c8360deea77 100644
> --- a/drivers/tty/serdev/core.c
> +++ b/drivers/tty/serdev/core.c
> @@ -582,18 +582,64 @@ static acpi_status acpi_serdev_register_device(struct serdev_controller *ctrl,
>   	return AE_OK;
>   }
>   
> -static acpi_status acpi_serdev_add_device(acpi_handle handle, u32 level,
> -				       void *data, void **return_value)
> +struct acpi_serdev_resource_context {
> +	struct serdev_controller *controller;
> +	struct acpi_device *device;
> +};
> +
> +static acpi_status
> +acpi_serdev_add_device_from_resource(struct acpi_resource *resource, void *data)
>   {
> -	struct serdev_controller *ctrl = data;
> -	struct acpi_device *adev;
> +	struct acpi_serdev_resource_context *ctx
> +		= (struct acpi_serdev_resource_context *)data;
> +	struct acpi_resource_source *ctrl_name;
> +	acpi_handle ctrl_handle;
> +
> +	if (resource->type != ACPI_RESOURCE_TYPE_SERIAL_BUS)
> +		return AE_OK;
>   
> -	if (acpi_bus_get_device(handle, &adev))
> +	if (resource->data.common_serial_bus.type
> +	    != ACPI_RESOURCE_SERIAL_TYPE_UART)
>   		return AE_OK;
>   
> -	return acpi_serdev_register_device(ctrl, adev);
> +	ctrl_name = &resource->data.common_serial_bus.resource_source;
> +	if (ctrl_name->string_length == 0 || !ctrl_name->string_ptr)
> +		return AE_OK;
> +
> +	if (acpi_get_handle(ctx->device->handle, ctrl_name->string_ptr,
> +			    &ctrl_handle))
> +		return AE_OK;
> +
> +	if (ctrl_handle == ACPI_HANDLE(ctx->controller->dev.parent))
> +		return acpi_serdev_register_device(ctx->controller,
> +						   ctx->device);
> +
> +	return AE_OK;
>   }
>   
> +static acpi_status
> +acpi_serdev_add_devices_from_resources(acpi_handle handle, u32 level,
> +				       void *data, void **return_value)
> +{
> +	struct acpi_serdev_resource_context ctx;
> +	acpi_status status;
> +
> +	ctx.controller = (struct serdev_controller *)data;
> +	status = acpi_bus_get_device(handle, &ctx.device);
> +	if (status)
> +		return AE_OK;		// ignore device if not present
> +
> +	status = acpi_walk_resources(handle, METHOD_NAME__CRS,
> +				     acpi_serdev_add_device_from_resource,
> +				     &ctx);
> +	if (status == AE_NOT_FOUND)
> +		return AE_OK;		// ignore if _CRS is not found
> +	else
> +		return status;
> +}
> +
> +#define SERDEV_ACPI_ENUMERATE_MAX_DEPTH		32
> +
>   static int acpi_serdev_register_devices(struct serdev_controller *ctrl)
>   {
>   	acpi_status status;
> @@ -603,8 +649,10 @@ static int acpi_serdev_register_devices(struct serdev_controller *ctrl)
>   	if (!handle)
>   		return -ENODEV;
>   
> -	status = acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, 1,
> -				     acpi_serdev_add_device, NULL, ctrl, NULL);
> +	status = acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT,
> +				     SERDEV_ACPI_ENUMERATE_MAX_DEPTH,
> +				     acpi_serdev_add_devices_from_resources,
> +				     NULL, ctrl, NULL);
>   	if (ACPI_FAILURE(status))
>   		dev_dbg(&ctrl->dev, "failed to enumerate serdev slaves\n");
>   
> 

Re: [PATCH] serdev: Add ACPI devices by ResourceSource field

[Maximilian Luz]

Hi,

> So as promised I've given this patch a try, unfortunately it breaks
> existing users of ACPI serdev device instantation.

I've only had a short look at it so far. As far as I can tell, there are
two options: Either the device does not match/is being skipped, or there
are errors (which are currently only reported with dev_dbg, based on the
pre-patch implementation) causing the search to terminate early. I'll
keep investigating this and report back once I've got a better
understanding of the possible sources for this.

> I haven't looked why your patch is breakig things, I have a large backlog
> so I do not have time for that.

No worries, I'll try to figure this out.

> But if you can provide me with a version of the patch with a bunch of
> debug printk-s added I'm happy to run that for you.

Thank you for this offer, I will probably come back to it once I have
more of an idea what could cause the breakage.

Regards,

Maximilian

Re: [PATCH] serdev: Add ACPI devices by ResourceSource field

[Maximilian Luz]

Hi all,

On 9/20/19 5:00 PM, Hans de Goede wrote:
> So as promised I've given this patch a try, unfortunately it breaks
> existing users of ACPI serdev device instantation.
> 
> After adding this patch "ls /sys/bus/serial/devices" is empty,
> where as before it gives:
> 
> [root@dhcp-45-50 ~]# ls -l /sys/bus/serial/devices/
> total 0
> lrwxrwxrwx. 1 root root 0 Sep 20 16:43 serial0 -> ../../../devices/pci0000:00/8086228A:00/serial0
> lrwxrwxrwx. 1 root root 0 Sep 20 16:43 serial0-0 -> ../../../devices/pci0000:00/8086228A:00/serial0/serial0-0
> 
> And since the serdev is missing bluetooth does not work.

Thanks to some testing by Hans, it turns out that the reason for this is
that acpi_walk_resources fails with AE_AML_INVALID_RESOURCE_TYPE for a
specific device. If anyone is interested, the _CRS of the device in
question is

     Method (_CRS, 0, NotSerialized)  // _CRS: Current Resource Settings
     {
         Name (NAM, Buffer (0x14)
         {
             "\\_SB.PCI0.SDHB.BRC1"
         })
         Name (SPB, Buffer (0x0C)
         {
             /* 0000 */  0x8E, 0x1D, 0x00, 0x01, 0x00, 0xC0, 0x02, 0x00,
             /* 0008 */  0x00, 0x01, 0x00, 0x00
         })
         Name (END, ResourceTemplate ()
         {
         })
         Concatenate (SPB, NAM, Local0)
         Concatenate (Local0, END, Local1)
         Return (Local1)
     }

To solve this, I propose ignoring errors that occur when evaluating the
_CRS method. Note that with the previously discussed change for v2,
where we will only look at the first device in _CRS, we should be able
to handle errors from the actual serdev device allocation separately
(and only ignore AML evaluation errors).

Further, I think it might also make sense to move the status and
already-enumerated checks out of acpi_serdev_register_device to before
looking at _CRS. On one hand, this might save us from unnecessarily
checking the _CRS on devices that are not present, but on the other
hand, moving the status check may cause more AML code execution, as we'd
be checking the status of every device, even if it doesn't have a _CRS.
Maybe a better solution would be something like: Check if device has
already been enumerated, then check for _CRS presence, then for
status/device-presence, and finally look at _CRS contents and
potentially allocate serdev client?

Regards,

Maximilian

Re: [PATCH] serdev: Add ACPI devices by ResourceSource field

[Hans de Goede]

Hi,

On 23-09-2019 00:29, Maximilian Luz wrote:
> Hi all,
> 
> On 9/20/19 5:00 PM, Hans de Goede wrote:
>> So as promised I've given this patch a try, unfortunately it breaks
>> existing users of ACPI serdev device instantation.
>>
>> After adding this patch "ls /sys/bus/serial/devices" is empty,
>> where as before it gives:
>>
>> [root@dhcp-45-50 ~]# ls -l /sys/bus/serial/devices/
>> total 0
>> lrwxrwxrwx. 1 root root 0 Sep 20 16:43 serial0 -> ../../../devices/pci0000:00/8086228A:00/serial0
>> lrwxrwxrwx. 1 root root 0 Sep 20 16:43 serial0-0 -> ../../../devices/pci0000:00/8086228A:00/serial0/serial0-0
>>
>> And since the serdev is missing bluetooth does not work.
> 
> Thanks to some testing by Hans, it turns out that the reason for this is
> that acpi_walk_resources fails with AE_AML_INVALID_RESOURCE_TYPE for a
> specific device. If anyone is interested, the _CRS of the device in
> question is
> 
>      Method (_CRS, 0, NotSerialized)  // _CRS: Current Resource Settings
>      {
>          Name (NAM, Buffer (0x14)
>          {
>              "\\_SB.PCI0.SDHB.BRC1"
>          })
>          Name (SPB, Buffer (0x0C)
>          {
>              /* 0000 */  0x8E, 0x1D, 0x00, 0x01, 0x00, 0xC0, 0x02, 0x00,
>              /* 0008 */  0x00, 0x01, 0x00, 0x00
>          })
>          Name (END, ResourceTemplate ()
>          {
>          })
>          Concatenate (SPB, NAM, Local0)
>          Concatenate (Local0, END, Local1)
>          Return (Local1)
>      }
> 
> To solve this, I propose ignoring errors that occur when evaluating the
> _CRS method. Note that with the previously discussed change for v2,
> where we will only look at the first device in _CRS, we should be able
> to handle errors from the actual serdev device allocation separately
> (and only ignore AML evaluation errors).
> 
> Further, I think it might also make sense to move the status and
> already-enumerated checks out of acpi_serdev_register_device to before
> looking at _CRS.

Ack, this is what drivers/i2c/i2c-core-acpi.c is doing and more in general
all ACPI enumeration code always first checks _STA before doing anything
else, so I think it would be best to do this here too.

Actually I think it might be best to fully copy how drivers/i2c/i2c-core-acpi.c
does things.

Regards,

Hans

Re: [PATCH] serdev: Add ACPI devices by ResourceSource field

[Maximilian Luz]

Hi,

On 9/23/19 10:14 AM, Hans de Goede wrote:
> Ack, this is what drivers/i2c/i2c-core-acpi.c is doing and more in general
> all ACPI enumeration code always first checks _STA before doing anything
> else, so I think it would be best to do this here too.
> 
> Actually I think it might be best to fully copy how drivers/i2c/i2c-core-acpi.c
> does things.

Right, I will do that then.

Thanks,

Maximilian

Re: [PATCH] serdev: Add ACPI devices by ResourceSource field

[Maximilian Luz]

Hi,

I have just submitted v2 of this patch.

Regards,

Maximilian