Re: [PATCH] serdev: Add ACPI devices by ResourceSource field
From: Hans de Goede
Date: Fri Sep 20 2019 - 11:00:41 EST
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@xxxxxxxxx>
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");