| 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
| ||
|
Date: Thu, 10 Oct 2019 12:22:42 +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 v2] serdev: Add ACPI devices by ResourceSource field
Hi,
On 24-09-2019 18:22, 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.
>
> This patch is based on the existing acpi serial bus implementations in
> drivers/i2c/i2c-core-acpi.c and drivers/spi/spi.c, specifically commit
> 4c3c59544f33e97cf8557f27e05a9904ead16363 ("spi/acpi: enumerate all SPI
> slaves in the namespace").
>
> Signed-off-by: Maximilian Luz <luzmaximilian@...il.com>
Thank you for the new version.
This patch looks good to me and it works on my test hw with serial
attached BT HCI:
Reviewed-by: Hans de Goede <hdegoede@...hat.com>
Tested-by: Hans de Goede <hdegoede@...hat.com>
Regards,
Hans
> ---
> Changes compared to v1:
> - Patch now reflects the behavior of the existing ACPI serial bus
> implementations (drivers/i2c/i2c-core-acpi.c and drivers/spi/spi.c),
> with a maximum of one serdev client device per ACPI device node
> allocated.
>
> - Ignores and continues on errors from AML code execution and resource
> parsing.
>
> Notes:
> The resource lookup is kept generic (similarly to the implementations
> it is based on), meaning that it should be fairly simple to extend
> acpi_serdev_parse_resource and acpi_serdev_check_resources to get and
> return more information about the serdev client device (e.g. initial
> baud rate) once this is required.
>
> If multiple device definitions inside a single _CRS block ever become
> a concern, the lookup function can be instructed as to which
> UARTSerialBusV2 resource should be considered by spefifying its index
> in acpi_serdev_lookup.index. This is again based on the I2C
> implementation. Currently the last resource definition is chosen (i.e.
> index = -1) to reflect the behavior of the other ACPI serial bus
> implementations.
> ---
> drivers/tty/serdev/core.c | 111 +++++++++++++++++++++++++++++++++-----
> 1 file changed, 99 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
> index a0ac16ee6575..226adeec2aed 100644
> --- a/drivers/tty/serdev/core.c
> +++ b/drivers/tty/serdev/core.c
> @@ -552,16 +552,97 @@ static int of_serdev_register_devices(struct serdev_controller *ctrl)
> }
>
> #ifdef CONFIG_ACPI
> +
> +#define SERDEV_ACPI_MAX_SCAN_DEPTH 32
> +
> +struct acpi_serdev_lookup {
> + acpi_handle device_handle;
> + acpi_handle controller_handle;
> + int n;
> + int index;
> +};
> +
> +static int acpi_serdev_parse_resource(struct acpi_resource *ares, void *data)
> +{
> + struct acpi_serdev_lookup *lookup = data;
> + struct acpi_resource_uart_serialbus *sb;
> + acpi_status status;
> +
> + if (ares->type != ACPI_RESOURCE_TYPE_SERIAL_BUS)
> + return 1;
> +
> + if (ares->data.common_serial_bus.type != ACPI_RESOURCE_SERIAL_TYPE_UART)
> + return 1;
> +
> + if (lookup->index != -1 && lookup->n++ != lookup->index)
> + return 1;
> +
> + sb = &ares->data.uart_serial_bus;
> +
> + status = acpi_get_handle(lookup->device_handle,
> + sb->resource_source.string_ptr,
> + &lookup->controller_handle);
> + if (ACPI_FAILURE(status))
> + return 1;
> +
> + /*
> + * NOTE: Ideally, we would also want to retreive other properties here,
> + * once setting them before opening the device is supported by serdev.
> + */
> +
> + return 1;
> +}
> +
> +static int acpi_serdev_do_lookup(struct acpi_device *adev,
> + struct acpi_serdev_lookup *lookup)
> +{
> + struct list_head resource_list;
> + int ret;
> +
> + lookup->device_handle = acpi_device_handle(adev);
> + lookup->controller_handle = NULL;
> + lookup->n = 0;
> +
> + INIT_LIST_HEAD(&resource_list);
> + ret = acpi_dev_get_resources(adev, &resource_list,
> + acpi_serdev_parse_resource, lookup);
> + acpi_dev_free_resource_list(&resource_list);
> +
> + if (ret < 0)
> + return -EINVAL;
> +
> + return 0;
> +}
> +
> +static int acpi_serdev_check_resources(struct serdev_controller *ctrl,
> + struct acpi_device *adev)
> +{
> + struct acpi_serdev_lookup lookup;
> + int ret;
> +
> + if (acpi_bus_get_status(adev) || !adev->status.present)
> + return -EINVAL;
> +
> + /* Look for UARTSerialBusV2 resource */
> + lookup.index = -1; // we only care for the last device
> +
> + ret = acpi_serdev_do_lookup(adev, &lookup);
> + if (ret)
> + return ret;
> +
> + /* Make sure controller and ResourceSource handle match */
> + if (ACPI_HANDLE(ctrl->dev.parent) != lookup.controller_handle)
> + return -ENODEV;
> +
> + return 0;
> +}
> +
> static acpi_status acpi_serdev_register_device(struct serdev_controller *ctrl,
> - struct acpi_device *adev)
> + struct acpi_device *adev)
> {
> - struct serdev_device *serdev = NULL;
> + struct serdev_device *serdev;
> int err;
>
> - if (acpi_bus_get_status(adev) || !adev->status.present ||
> - acpi_device_enumerated(adev))
> - return AE_OK;
> -
> serdev = serdev_device_alloc(ctrl);
> if (!serdev) {
> dev_err(&ctrl->dev, "failed to allocate serdev device for %s\n",
> @@ -583,7 +664,7 @@ static acpi_status acpi_serdev_register_device(struct serdev_controller *ctrl,
> }
>
> static acpi_status acpi_serdev_add_device(acpi_handle handle, u32 level,
> - void *data, void **return_value)
> + void *data, void **return_value)
> {
> struct serdev_controller *ctrl = data;
> struct acpi_device *adev;
> @@ -591,22 +672,28 @@ static acpi_status acpi_serdev_add_device(acpi_handle handle, u32 level,
> if (acpi_bus_get_device(handle, &adev))
> return AE_OK;
>
> + if (acpi_device_enumerated(adev))
> + return AE_OK;
> +
> + if (acpi_serdev_check_resources(ctrl, adev))
> + return AE_OK;
> +
> return acpi_serdev_register_device(ctrl, adev);
> }
>
> +
> static int acpi_serdev_register_devices(struct serdev_controller *ctrl)
> {
> acpi_status status;
> - acpi_handle handle;
>
> - handle = ACPI_HANDLE(ctrl->dev.parent);
> - if (!handle)
> + if (!has_acpi_companion(ctrl->dev.parent))
> return -ENODEV;
>
> - status = acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, 1,
> + status = acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT,
> + SERDEV_ACPI_MAX_SCAN_DEPTH,
> acpi_serdev_add_device, NULL, ctrl, NULL);
> if (ACPI_FAILURE(status))
> - dev_dbg(&ctrl->dev, "failed to enumerate serdev slaves\n");
> + dev_warn(&ctrl->dev, "failed to enumerate serdev slaves\n");
>
> if (!ctrl->serdev)
> return -ENODEV;
>
Powered by blists - more mailing lists