| Message ID | 20240216201721.239791-3-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Delegated to: | Hans de Goede |
| Headers | show |
| Series | platform/x86: x86-android-tablets: 2 regression fixes | expand |
On Fri, Feb 16, 2024 at 09:17:19PM +0100, Hans de Goede wrote: > In some cases UART attached devices which require an in kernel driver, > e.g. UART attached Bluetooth HCIs are described in the ACPI tables > by an ACPI device with a broken or missing UartSerialBusV2() resource. > > This causes the kernel to create a /dev/ttyS# char-device for the UART > instead of creating an in kernel serdev-controller + serdev-device pair > for the in kernel driver. > > The quirk handling in acpi_quirk_skip_serdev_enumeration() makes the kernel > create a serdev-controller device for these UARTs instead of a /dev/ttyS#. > > Instantiating the actual serdev-device to bind to is up to pdx86 code, > so far this was handled by the x86-android-tablets code. But since > commit b286f4e87e32 ("serial: core: Move tty and serdev to be children of > serial core port device") the serdev-controller device has moved in the > device hierarchy from (e.g.) /sys/devices/pci0000:00/8086228A:00/serial0 to > /sys/devices/pci0000:00/8086228A:00/8086228A:00:0/8086228A:00:0.0/serial0 . > > This makes this a bit trickier to do and another driver is in the works > which will also need this functionality. > > Add a new helper to get the serdev-controller device, so that the new > code for this can be shared. The above doesn't explain why the new code is h-file. ... > +#include <linux/acpi.h> + err.h > +#include <linux/device.h> > +#include <linux/printk.h> + sprintf.h + string.h
Hi Andy, On 2/16/24 22:24, Andy Shevchenko wrote: > On Fri, Feb 16, 2024 at 09:17:19PM +0100, Hans de Goede wrote: >> In some cases UART attached devices which require an in kernel driver, >> e.g. UART attached Bluetooth HCIs are described in the ACPI tables >> by an ACPI device with a broken or missing UartSerialBusV2() resource. >> >> This causes the kernel to create a /dev/ttyS# char-device for the UART >> instead of creating an in kernel serdev-controller + serdev-device pair >> for the in kernel driver. >> >> The quirk handling in acpi_quirk_skip_serdev_enumeration() makes the kernel >> create a serdev-controller device for these UARTs instead of a /dev/ttyS#. >> >> Instantiating the actual serdev-device to bind to is up to pdx86 code, >> so far this was handled by the x86-android-tablets code. But since >> commit b286f4e87e32 ("serial: core: Move tty and serdev to be children of >> serial core port device") the serdev-controller device has moved in the >> device hierarchy from (e.g.) /sys/devices/pci0000:00/8086228A:00/serial0 to >> /sys/devices/pci0000:00/8086228A:00/8086228A:00:0/8086228A:00:0.0/serial0 . >> >> This makes this a bit trickier to do and another driver is in the works >> which will also need this functionality. >> >> Add a new helper to get the serdev-controller device, so that the new >> code for this can be shared. > > The above doesn't explain why the new code is h-file. It is in a h file because as metioned: "another driver is in the works" which will also need this. And the code is large/complicated enough that I don't want to copy and paste it. Yet small enough that it would be silly to put it in its own .ko file. Regards, Hans p.s. About the other driver. I recently learned that some Dell AIOs (1) use a backlight controller board connected to an UART. Canonical even submitted a driver for this in 2017, but never followed-up on getting it merged: https://lkml.org/lkml/2017/10/26/78 This UART has a DELL0501 HID with CID set to PNP0501 so that the UART is still handled by 8250_pnp.c. Unfortunately there is no separate ACPI device with an UartSerialBusV2() resource to model the backlight-controller. My patch series for this will use acpi_quirk_skip_serdev_enumeration() to still create a serdev for this for a backlight driver to bind to instead of creating a /dev/ttyS0. Like other cases where the UartSerialBusV2() resource is missing or broken this will only create the serdev-controller device and the serdev-device itself will need to be instantiated by a pdx86 driver. This driver will use this new helper to create the serdev-device (client) itself. 1) All In One a monitor with a PC builtin
On Sat, Feb 17, 2024 at 12:36 AM Hans de Goede <hdegoede@redhat.com> wrote: > On 2/16/24 22:24, Andy Shevchenko wrote: > > On Fri, Feb 16, 2024 at 09:17:19PM +0100, Hans de Goede wrote: ... > > The above doesn't explain why the new code is h-file. > > It is in a h file because as metioned: "another driver is in the works" > which will also need this. Implied, but quite unclear. Can you rephrase? > And the code is large/complicated enough that I don't want to copy > and paste it. Yet small enough that it would be silly to put it > in its own .ko file. We have even smaller code in the separate module. So I don't consider this as a strong argument.
Hi Andy, On 2/17/24 19:09, Andy Shevchenko wrote: > On Sat, Feb 17, 2024 at 12:36 AM Hans de Goede <hdegoede@redhat.com> wrote: >> On 2/16/24 22:24, Andy Shevchenko wrote: >>> On Fri, Feb 16, 2024 at 09:17:19PM +0100, Hans de Goede wrote: > > ... > >>> The above doesn't explain why the new code is h-file. >> >> It is in a h file because as metioned: "another driver is in the works" >> which will also need this. > > Implied, but quite unclear. Can you rephrase? Ack will do and I'll also add the missing headers which you pointed out. Regards, Hans
* Hans de Goede <hdegoede@redhat.com> [240216 20:17]: > +static inline struct device * > +get_serdev_controller(const char *serial_ctrl_hid, > + const char *serial_ctrl_uid, > + int serial_ctrl_port, > + const char *serdev_ctrl_name) > +{ > + struct device *ctrl_dev, *child; > + struct acpi_device *ctrl_adev; > + char name[32]; > + int i; > + > + ctrl_adev = acpi_dev_get_first_match_dev(serial_ctrl_hid, serial_ctrl_uid, -1); > + if (!ctrl_adev) { > + pr_err("error could not get %s/%s serial-ctrl adev\n", > + serial_ctrl_hid, serial_ctrl_uid); > + return ERR_PTR(-ENODEV); > + } Maybe split get_serdev_controller() into two functions, an acpi specific function, and a serdev specific function? And I also think these should not be in the header file, maybe splitting will also help with that :) > + /* get_first_physical_node() returns a weak ref */ > + ctrl_dev = get_device(acpi_get_first_physical_node(ctrl_adev)); > + if (!ctrl_dev) { > + pr_err("error could not get %s/%s serial-ctrl physical node\n", > + serial_ctrl_hid, serial_ctrl_uid); > + ctrl_dev = ERR_PTR(-ENODEV); > + goto put_ctrl_adev; > + } > + > + /* Walk host -> uart-ctrl -> port -> serdev-ctrl */ > + for (i = 0; i < 3; i++) { > + switch (i) { > + case 0: > + snprintf(name, sizeof(name), "%s:0", dev_name(ctrl_dev)); Note that in theory it's possible we will encounter a device that has multiple serial core controller instances as noted by Jiri earlier. And each controller may have one or more ports. For the multiport test case, you can use qemu with options like below FYI: -chardev null,id=s1 -chardev null,id=s2 \ -device pci-serial-2x,chardev1=s1,chardev2=s2 Regards, Tony
diff --git a/drivers/platform/x86/serdev_helpers.h b/drivers/platform/x86/serdev_helpers.h new file mode 100644 index 000000000000..825979f6736b --- /dev/null +++ b/drivers/platform/x86/serdev_helpers.h @@ -0,0 +1,77 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ +/* + * In some cases UART attached devices which require an in kernel driver, + * e.g. UART attached Bluetooth HCIs are described in the ACPI tables + * by an ACPI device with a broken or missing UartSerialBusV2() resource. + * + * This causes the kernel to create a /dev/ttyS# char-device for the UART + * instead of creating an in kernel serdev-controller + serdev-device pair + * for the in kernel driver. + * + * The quirk handling in acpi_quirk_skip_serdev_enumeration() makes the kernel + * create a serdev-controller device for these UARTs instead of a /dev/ttyS#. + * + * Instantiating the actual serdev-device to bind to is up to pdx86 code, + * this header provides a helper for getting the serdev-controller device. + */ +#include <linux/acpi.h> +#include <linux/device.h> +#include <linux/printk.h> + +static inline struct device * +get_serdev_controller(const char *serial_ctrl_hid, + const char *serial_ctrl_uid, + int serial_ctrl_port, + const char *serdev_ctrl_name) +{ + struct device *ctrl_dev, *child; + struct acpi_device *ctrl_adev; + char name[32]; + int i; + + ctrl_adev = acpi_dev_get_first_match_dev(serial_ctrl_hid, serial_ctrl_uid, -1); + if (!ctrl_adev) { + pr_err("error could not get %s/%s serial-ctrl adev\n", + serial_ctrl_hid, serial_ctrl_uid); + return ERR_PTR(-ENODEV); + } + + /* get_first_physical_node() returns a weak ref */ + ctrl_dev = get_device(acpi_get_first_physical_node(ctrl_adev)); + if (!ctrl_dev) { + pr_err("error could not get %s/%s serial-ctrl physical node\n", + serial_ctrl_hid, serial_ctrl_uid); + ctrl_dev = ERR_PTR(-ENODEV); + goto put_ctrl_adev; + } + + /* Walk host -> uart-ctrl -> port -> serdev-ctrl */ + for (i = 0; i < 3; i++) { + switch (i) { + case 0: + snprintf(name, sizeof(name), "%s:0", dev_name(ctrl_dev)); + break; + case 1: + snprintf(name, sizeof(name), "%s.%d", + dev_name(ctrl_dev), serial_ctrl_port); + break; + case 2: + strscpy(name, serdev_ctrl_name, sizeof(name)); + break; + } + + child = device_find_child_by_name(ctrl_dev, name); + put_device(ctrl_dev); + if (!child) { + pr_err("error could not find '%s' device\n", name); + ctrl_dev = ERR_PTR(-ENODEV); + goto put_ctrl_adev; + } + + ctrl_dev = child; + } + +put_ctrl_adev: + acpi_dev_put(ctrl_adev); + return ctrl_dev; +}
In some cases UART attached devices which require an in kernel driver, e.g. UART attached Bluetooth HCIs are described in the ACPI tables by an ACPI device with a broken or missing UartSerialBusV2() resource. This causes the kernel to create a /dev/ttyS# char-device for the UART instead of creating an in kernel serdev-controller + serdev-device pair for the in kernel driver. The quirk handling in acpi_quirk_skip_serdev_enumeration() makes the kernel create a serdev-controller device for these UARTs instead of a /dev/ttyS#. Instantiating the actual serdev-device to bind to is up to pdx86 code, so far this was handled by the x86-android-tablets code. But since commit b286f4e87e32 ("serial: core: Move tty and serdev to be children of serial core port device") the serdev-controller device has moved in the device hierarchy from (e.g.) /sys/devices/pci0000:00/8086228A:00/serial0 to /sys/devices/pci0000:00/8086228A:00/8086228A:00:0/8086228A:00:0.0/serial0 . This makes this a bit trickier to do and another driver is in the works which will also need this functionality. Add a new helper to get the serdev-controller device, so that the new code for this can be shared. Fixes: b286f4e87e32 ("serial: core: Move tty and serdev to be children of serial core port device") Cc: Tony Lindgren <tony@atomide.com> Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/platform/x86/serdev_helpers.h | 77 +++++++++++++++++++++++++++ 1 file changed, 77 insertions(+) create mode 100644 drivers/platform/x86/serdev_helpers.h