| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <50b016a1-ed4a-b848-4658-a05731727d7e@redhat.com>
Date: Fri, 20 Sep 2019 17:00:29 +0200
From: Hans de Goede <hdegoede@...hat.com>
To: Maximilian Luz <luzmaximilian@...il.com>
Cc: Rob Herring <robh@...nel.org>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Jiri Slaby <jslaby@...e.com>, Johan Hovold <johan@...nel.org>,
"Rafael J . Wysocki" <rjw@...ysocki.net>,
Len Brown <lenb@...nel.org>, linux-serial@...r.kernel.org,
linux-acpi@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] serdev: Add ACPI devices by ResourceSource field
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@...il.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@...p-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");
>
>
Powered by blists - more mailing lists