[PATCH v2 0/2] platform/x86: Add new Dell UART backlight driver

[PATCH v2 0/2] platform/x86: Add new Dell UART backlight driver

[Hans de Goede]

Hi All,

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:
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. An
ACPI quirk has been merged recently to deal with this and create a serdev
controller for the UART despite the missing UartSerialBusV2() resource:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=99b572e6136eab69a8c91d72cf8595b256e304b5

Patch 1 in this series adds a driver binding to the "dell-uart-backlight"
device created by this quirk. This drivers creates a serdev-device for
the DELL0501 serdev-controller and registers a serdev backlight driver
which binds to this serdev-device.

Patch 2 contains a small emulator for the UART attached backlight
controller found on this Dell AOIs, I wrote and used this to develop
the driver since I did not have access to such an AOI myself.

This has been successfully tested by Roman Bogoyev (who originally
reported the missing driver to me by email) on a Dell Inspiron 27 7000
(7790) and by Kai-Heng Feng on a newer Dell AOI model.

Changes in v2:
- Address all the review-remarks Ilpo and Andy had on Patch 1/2,
  thank you for the extensive review Ilpo and Andy

Regards,

Hans


Hans de Goede (2):
  platform/x86: Add new Dell UART backlight driver
  tools arch x86: Add dell-uart-backlight-emulator

 drivers/platform/x86/dell/Kconfig             |  15 +
 drivers/platform/x86/dell/Makefile            |   1 +
 .../platform/x86/dell/dell-uart-backlight.c   | 409 ++++++++++++++++++
 .../dell-uart-backlight-emulator/.gitignore   |   1 +
 .../x86/dell-uart-backlight-emulator/Makefile |  19 +
 .../x86/dell-uart-backlight-emulator/README   |  46 ++
 .../dell-uart-backlight-emulator.c            | 167 +++++++
 7 files changed, 658 insertions(+)
 create mode 100644 drivers/platform/x86/dell/dell-uart-backlight.c
 create mode 100644 tools/arch/x86/dell-uart-backlight-emulator/.gitignore
 create mode 100644 tools/arch/x86/dell-uart-backlight-emulator/Makefile
 create mode 100644 tools/arch/x86/dell-uart-backlight-emulator/README
 create mode 100644 tools/arch/x86/dell-uart-backlight-emulator/dell-uart-backlight-emulator.c

-- 
2.44.0

[PATCH v2 1/2] platform/x86: Add new Dell UART backlight driver

[Hans de Goede]

Dell All In One (AIO) models released after 2017 use a backlight controller
board connected to an UART.

In DSDT this uart port will be defined as:

   Name (_HID, "DELL0501")
   Name (_CID, EisaId ("PNP0501")

Instead of having a separate ACPI device with an UartSerialBusV2() resource
to model the backlight-controller, which would be the standard way to do
this.

The acpi_quirk_skip_serdev_enumeration() has special handling for this
and it will make the serial port code create a serdev controller device
for the UART instead of a /dev/ttyS0 char-dev. It will also create
a dell-uart-backlight driver platform device for this driver to bind too.

This new kernel module contains 2 drivers for this:

1. A simple platform driver which creates the actual serdev device
   (with the serdev controller device as parent)

2. A serdev driver for the created serdev device which exports
   the backlight functionality uses a standard backlight class device.

Reported-by: Roman Bogoyev <roman@computercheck.com.au>
Tested-by: Roman Bogoyev <roman@computercheck.com.au>
Tested-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
Co-developed-by: AceLan Kao <acelan.kao@canonical.com>
Signed-off-by: AceLan Kao <acelan.kao@canonical.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
- Address various review remarks from Ilpo and Andy
---
 drivers/platform/x86/dell/Kconfig             |  15 +
 drivers/platform/x86/dell/Makefile            |   1 +
 .../platform/x86/dell/dell-uart-backlight.c   | 409 ++++++++++++++++++
 3 files changed, 425 insertions(+)
 create mode 100644 drivers/platform/x86/dell/dell-uart-backlight.c

diff --git a/drivers/platform/x86/dell/Kconfig b/drivers/platform/x86/dell/Kconfig
index bd9f445974cc..195a8bf532cc 100644
--- a/drivers/platform/x86/dell/Kconfig
+++ b/drivers/platform/x86/dell/Kconfig
@@ -145,6 +145,21 @@ config DELL_SMO8800
 	  To compile this driver as a module, choose M here: the module will
 	  be called dell-smo8800.
 
+config DELL_UART_BACKLIGHT
+	tristate "Dell AIO UART Backlight driver"
+	depends on ACPI
+	depends on BACKLIGHT_CLASS_DEVICE
+	depends on SERIAL_DEV_BUS
+	help
+	  Say Y here if you want to support Dell AIO UART backlight interface.
+	  The Dell AIO machines released after 2017 come with a UART interface
+	  to communicate with the backlight scalar board. This driver creates
+	  a standard backlight interface and talks to the scalar board through
+	  UART to adjust the AIO screen brightness.
+
+	  To compile this driver as a module, choose M here: the module will
+	  be called dell_uart_backlight.
+
 config DELL_WMI
 	tristate "Dell WMI notifications"
 	default m
diff --git a/drivers/platform/x86/dell/Makefile b/drivers/platform/x86/dell/Makefile
index 1b8942426622..8176a257d9c3 100644
--- a/drivers/platform/x86/dell/Makefile
+++ b/drivers/platform/x86/dell/Makefile
@@ -14,6 +14,7 @@ dell-smbios-objs			:= dell-smbios-base.o
 dell-smbios-$(CONFIG_DELL_SMBIOS_WMI)	+= dell-smbios-wmi.o
 dell-smbios-$(CONFIG_DELL_SMBIOS_SMM)	+= dell-smbios-smm.o
 obj-$(CONFIG_DELL_SMO8800)		+= dell-smo8800.o
+obj-$(CONFIG_DELL_UART_BACKLIGHT)	+= dell-uart-backlight.o
 obj-$(CONFIG_DELL_WMI)			+= dell-wmi.o
 dell-wmi-objs				:= dell-wmi-base.o
 dell-wmi-$(CONFIG_DELL_WMI_PRIVACY)	+= dell-wmi-privacy.o
diff --git a/drivers/platform/x86/dell/dell-uart-backlight.c b/drivers/platform/x86/dell/dell-uart-backlight.c
new file mode 100644
index 000000000000..bf5b12efcb19
--- /dev/null
+++ b/drivers/platform/x86/dell/dell-uart-backlight.c
@@ -0,0 +1,409 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Dell AIO Serial Backlight Driver
+ *
+ * Copyright (C) 2024 Hans de Goede <hansg@kernel.org>
+ * Copyright (C) 2017 AceLan Kao <acelan.kao@canonical.com>
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/acpi.h>
+#include <linux/backlight.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/platform_device.h>
+#include <linux/serdev.h>
+#include <linux/string.h>
+#include <linux/types.h>
+#include <linux/wait.h>
+#include "../serdev_helpers.h"
+
+/* The backlight controller must respond within 1 second */
+#define DELL_BL_TIMEOUT		msecs_to_jiffies(1000)
+#define DELL_BL_MAX_BRIGHTNESS	100
+
+/* Defines for the commands send to the controller */
+
+/* 1st byte Start Of Frame 3 MSB bits: cmd-len + 01010 SOF marker */
+#define SOF(len)			(((len) << 5) | 0x0a)
+#define GET_CMD_LEN			3
+#define SET_CMD_LEN			4
+
+/* 2nd byte command */
+#define CMD_GET_VERSION			0x06
+#define CMD_SET_BRIGHTNESS		0x0b
+#define CMD_GET_BRIGHTNESS		0x0c
+#define CMD_SET_BL_POWER		0x0e
+
+/* Indexes and other defines for response received from the controller */
+#define RESP_LEN			0
+#define RESP_CMD			1 /* Echo of CMD byte from command */
+#define RESP_DATA			2 /* Start of received data */
+
+#define SET_RESP_LEN			3
+#define GET_RESP_LEN			4
+#define MIN_RESP_LEN			3
+#define MAX_RESP_LEN			80
+
+struct dell_uart_backlight {
+	struct mutex mutex;
+	wait_queue_head_t wait_queue;
+	struct device *dev;
+	struct backlight_device *bl;
+	u8 *resp;
+	u8 resp_idx;
+	u8 resp_len;
+	u8 resp_max_len;
+	u8 pending_cmd;
+	int status;
+	int power;
+};
+
+/* Checksum: SUM(Length and Cmd and Data) xor 0xFF */
+static u8 dell_uart_checksum(u8 *buf, int len)
+{
+	u8 val = 0;
+
+	while (len-- > 0)
+		val += buf[len];
+
+	return val ^ 0xff;
+}
+
+static int dell_uart_bl_command(struct dell_uart_backlight *dell_bl,
+				const u8 *cmd, int cmd_len,
+				u8 *resp, int resp_max_len)
+{
+	int ret;
+
+	ret = mutex_lock_killable(&dell_bl->mutex);
+	if (ret)
+		return ret;
+
+	dell_bl->status = -EBUSY;
+	dell_bl->resp = resp;
+	dell_bl->resp_idx = 0;
+	dell_bl->resp_max_len = resp_max_len;
+	dell_bl->pending_cmd = cmd[1];
+
+	/* The TTY buffer should be big enough to take the entire cmd in one go */
+	ret = serdev_device_write_buf(to_serdev_device(dell_bl->dev), cmd, cmd_len);
+	if (ret != cmd_len) {
+		dev_err(dell_bl->dev, "Error writing command: %d\n", ret);
+		ret = (ret < 0) ? ret : -EIO;
+		goto out;
+	}
+
+	ret = wait_event_timeout(dell_bl->wait_queue, dell_bl->status != -EBUSY,
+				 DELL_BL_TIMEOUT);
+	if (ret == 0) {
+		dev_err(dell_bl->dev, "Timed out waiting for response.\n");
+		ret = -ETIMEDOUT;
+	} else {
+		ret = dell_bl->status;
+	}
+
+out:
+	mutex_unlock(&dell_bl->mutex);
+	return ret;
+}
+
+static int dell_uart_set_brightness(struct dell_uart_backlight *dell_bl, int brightness)
+{
+	u8 set_brightness[SET_CMD_LEN], resp[SET_RESP_LEN];
+
+	set_brightness[0] = SOF(SET_CMD_LEN);
+	set_brightness[1] = CMD_SET_BRIGHTNESS;
+	set_brightness[2] = brightness;
+	set_brightness[3] = dell_uart_checksum(set_brightness, 3);
+
+	return dell_uart_bl_command(dell_bl, set_brightness, SET_CMD_LEN, resp, SET_RESP_LEN);
+}
+
+static int dell_uart_get_brightness(struct dell_uart_backlight *dell_bl)
+{
+	u8 get_brightness[GET_CMD_LEN], resp[GET_RESP_LEN];
+	int ret;
+
+	get_brightness[0] = SOF(GET_CMD_LEN);
+	get_brightness[1] = CMD_GET_BRIGHTNESS;
+	get_brightness[2] = dell_uart_checksum(get_brightness, 2);
+
+	ret = dell_uart_bl_command(dell_bl, get_brightness, GET_CMD_LEN, resp, GET_RESP_LEN);
+	if (ret)
+		return ret;
+
+	if (resp[RESP_LEN] != GET_RESP_LEN) {
+		dev_err(dell_bl->dev, "Unexpected get brightness response length: %d\n",
+			resp[RESP_LEN]);
+		return -EIO;
+	}
+
+	if (resp[RESP_DATA] > DELL_BL_MAX_BRIGHTNESS) {
+		dev_err(dell_bl->dev, "Unexpected get brightness response: %d\n",
+			resp[RESP_DATA]);
+		return -EIO;
+	}
+
+	return resp[RESP_DATA];
+}
+
+static int dell_uart_set_bl_power(struct dell_uart_backlight *dell_bl, int power)
+{
+	u8 set_power[SET_CMD_LEN], resp[SET_RESP_LEN];
+	int ret;
+
+	set_power[0] = SOF(SET_CMD_LEN);
+	set_power[1] = CMD_SET_BL_POWER;
+	set_power[2] = (power == FB_BLANK_UNBLANK) ? 1 : 0;
+	set_power[3] = dell_uart_checksum(set_power, 3);
+
+	ret = dell_uart_bl_command(dell_bl, set_power, SET_CMD_LEN, resp, SET_RESP_LEN);
+	if (ret)
+		return ret;
+
+	dell_bl->power = power;
+	return 0;
+}
+
+/*
+ * There is no command to get backlight power status,
+ * so we set the backlight power to "on" while initializing,
+ * and then track and report its status by power variable.
+ */
+static int dell_uart_get_bl_power(struct dell_uart_backlight *dell_bl)
+{
+	return dell_bl->power;
+}
+
+static int dell_uart_update_status(struct backlight_device *bd)
+{
+	struct dell_uart_backlight *dell_bl = bl_get_data(bd);
+	int ret;
+
+	ret = dell_uart_set_brightness(dell_bl, bd->props.brightness);
+	if (ret)
+		return ret;
+
+	if (bd->props.power != dell_uart_get_bl_power(dell_bl))
+		return dell_uart_set_bl_power(dell_bl, bd->props.power);
+
+	return 0;
+}
+
+static int dell_uart_get_brightness_op(struct backlight_device *bd)
+{
+	return dell_uart_get_brightness(bl_get_data(bd));
+}
+
+static const struct backlight_ops dell_uart_backlight_ops = {
+	.update_status = dell_uart_update_status,
+	.get_brightness = dell_uart_get_brightness_op,
+};
+
+static size_t dell_uart_bl_receive(struct serdev_device *serdev, const u8 *data, size_t len)
+{
+	struct dell_uart_backlight *dell_bl = serdev_device_get_drvdata(serdev);
+	size_t i;
+	u8 csum;
+
+	dev_dbg(dell_bl->dev, "Recv: %*ph\n", (int)len, data);
+
+	/* Throw away unexpected bytes / remainder of response after an error */
+	if (dell_bl->status != -EBUSY) {
+		dev_warn(dell_bl->dev, "Bytes received out of band, dropping them.\n");
+		return len;
+	}
+
+	for (i = 0; i < len; i++) {
+		dell_bl->resp[dell_bl->resp_idx] = data[i];
+
+		switch (dell_bl->resp_idx) {
+		case RESP_LEN: /* Length byte */
+			dell_bl->resp_len = dell_bl->resp[RESP_LEN];
+			if (dell_bl->resp_len < MIN_RESP_LEN) {
+				dev_err(dell_bl->dev, "Response length too small %d < %d\n",
+					dell_bl->resp_len, MIN_RESP_LEN);
+				dell_bl->status = -EIO;
+				goto wakeup;
+			}
+
+			if (dell_bl->resp_len > dell_bl->resp_max_len) {
+				dev_err(dell_bl->dev, "Response length too big %d > %d\n",
+					dell_bl->resp_len, dell_bl->resp_max_len);
+				dell_bl->status = -EIO;
+				goto wakeup;
+			}
+			break;
+		case RESP_CMD: /* CMD byte */
+			if (dell_bl->resp[RESP_CMD] != dell_bl->pending_cmd) {
+				dev_err(dell_bl->dev, "Response cmd 0x%02x != pending 0x%02x\n",
+					dell_bl->resp[RESP_CMD], dell_bl->pending_cmd);
+				dell_bl->status = -EIO;
+				goto wakeup;
+			}
+			break;
+		}
+
+		dell_bl->resp_idx++;
+		if (dell_bl->resp_idx < dell_bl->resp_len)
+			continue;
+
+		csum = dell_uart_checksum(dell_bl->resp, dell_bl->resp_len - 1);
+		if (dell_bl->resp[dell_bl->resp_len - 1] != csum) {
+			dev_err(dell_bl->dev, "Checksum mismatch got 0x%02x expected 0x%02x\n",
+				dell_bl->resp[dell_bl->resp_len - 1], csum);
+			dell_bl->status = -EIO;
+			goto wakeup;
+		}
+
+		dell_bl->status = 0; /* Success */
+		goto wakeup;
+	}
+
+	return len;
+
+wakeup:
+	wake_up(&dell_bl->wait_queue);
+	return i + 1;
+}
+
+static const struct serdev_device_ops dell_uart_bl_serdev_ops = {
+	.receive_buf = dell_uart_bl_receive,
+	.write_wakeup = serdev_device_write_wakeup,
+};
+
+static int dell_uart_bl_serdev_probe(struct serdev_device *serdev)
+{
+	u8 get_version[GET_CMD_LEN], resp[MAX_RESP_LEN];
+	struct backlight_properties props = {};
+	struct dell_uart_backlight *dell_bl;
+	struct device *dev = &serdev->dev;
+	int ret;
+
+	dell_bl = devm_kzalloc(dev, sizeof(*dell_bl), GFP_KERNEL);
+	if (!dell_bl)
+		return -ENOMEM;
+
+	mutex_init(&dell_bl->mutex);
+	init_waitqueue_head(&dell_bl->wait_queue);
+	dell_bl->dev = dev;
+
+	ret = devm_serdev_device_open(dev, serdev);
+	if (ret)
+		return dev_err_probe(dev, ret, "opening UART device\n");
+
+	/* 9600 bps, no flow control, these are the default but set them to be sure */
+	serdev_device_set_baudrate(serdev, 9600);
+	serdev_device_set_flow_control(serdev, false);
+	serdev_device_set_drvdata(serdev, dell_bl);
+	serdev_device_set_client_ops(serdev, &dell_uart_bl_serdev_ops);
+
+	get_version[0] = SOF(GET_CMD_LEN);
+	get_version[1] = CMD_GET_VERSION;
+	get_version[2] = dell_uart_checksum(get_version, 2);
+
+	ret = dell_uart_bl_command(dell_bl, get_version, GET_CMD_LEN, resp, MAX_RESP_LEN);
+	if (ret)
+		return dev_err_probe(dev, ret, "getting firmware version\n");
+
+	dev_dbg(dev, "Firmware version: %.*s\n", resp[RESP_LEN] - 3, resp + RESP_DATA);
+
+	/* Initialize bl_power to a known value */
+	ret = dell_uart_set_bl_power(dell_bl, FB_BLANK_UNBLANK);
+	if (ret)
+		return ret;
+
+	ret = dell_uart_get_brightness(dell_bl);
+	if (ret < 0)
+		return ret;
+
+	props.type = BACKLIGHT_PLATFORM;
+	props.brightness = ret;
+	props.max_brightness = DELL_BL_MAX_BRIGHTNESS;
+	props.power = dell_bl->power;
+
+	dell_bl->bl = devm_backlight_device_register(dev, "dell_uart_backlight",
+						     dev, dell_bl,
+						     &dell_uart_backlight_ops,
+						     &props);
+	return PTR_ERR_OR_ZERO(dell_bl->bl);
+}
+
+struct serdev_device_driver dell_uart_bl_serdev_driver = {
+	.probe = dell_uart_bl_serdev_probe,
+	.driver = {
+		.name = KBUILD_MODNAME,
+	},
+};
+
+static int dell_uart_bl_pdev_probe(struct platform_device *pdev)
+{
+	struct serdev_device *serdev;
+	struct device *ctrl_dev;
+	int ret;
+
+	ctrl_dev = get_serdev_controller("DELL0501", NULL, 0, "serial0");
+	if (IS_ERR(ctrl_dev))
+		return PTR_ERR(ctrl_dev);
+
+	serdev = serdev_device_alloc(to_serdev_controller(ctrl_dev));
+	put_device(ctrl_dev);
+	if (!serdev)
+		return -ENOMEM;
+
+	ret = serdev_device_add(serdev);
+	if (ret) {
+		dev_err(&pdev->dev, "error %d adding serdev\n", ret);
+		serdev_device_put(serdev);
+		return ret;
+	}
+
+	ret = serdev_device_driver_register(&dell_uart_bl_serdev_driver);
+	if (ret)
+		goto err_remove_serdev;
+
+	/*
+	 * serdev device <-> driver matching relies on OF or ACPI matches and
+	 * neither is available here, manually bind the driver.
+	 */
+	ret = device_driver_attach(&dell_uart_bl_serdev_driver.driver, &serdev->dev);
+	if (ret)
+		goto err_unregister_serdev_driver;
+
+	/* So that dell_uart_bl_pdev_remove() can remove the serdev */
+	platform_set_drvdata(pdev, serdev);
+	return 0;
+
+err_unregister_serdev_driver:
+	serdev_device_driver_unregister(&dell_uart_bl_serdev_driver);
+err_remove_serdev:
+	serdev_device_remove(serdev);
+	return ret;
+}
+
+static void dell_uart_bl_pdev_remove(struct platform_device *pdev)
+{
+	struct serdev_device *serdev = platform_get_drvdata(pdev);
+
+	serdev_device_driver_unregister(&dell_uart_bl_serdev_driver);
+	serdev_device_remove(serdev);
+}
+
+static struct platform_driver dell_uart_bl_pdev_driver = {
+	.probe = dell_uart_bl_pdev_probe,
+	.remove_new = dell_uart_bl_pdev_remove,
+	.driver = {
+		.name = "dell-uart-backlight",
+	},
+};
+module_platform_driver(dell_uart_bl_pdev_driver);
+
+MODULE_ALIAS("platform:dell-uart-backlight");
+MODULE_DESCRIPTION("Dell AIO Serial Backlight driver");
+MODULE_AUTHOR("Hans de Goede <hansg@kernel.org>");
+MODULE_LICENSE("GPL");
-- 
2.44.0

[PATCH v2 2/2] tools arch x86: Add dell-uart-backlight-emulator

[Hans de Goede]

Dell All In One (AIO) models released after 2017 use a backlight controller
board connected to an UART.

Add a small emulator to allow development and testing of
the drivers/platform/x86/dell/dell-uart-backlight.c driver for
this board, without requiring access to an actual Dell All In One.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
- Ensure clean exit (return 0) when the emulator is quit by Ctrl+C
---
 .../dell-uart-backlight-emulator/.gitignore   |   1 +
 .../x86/dell-uart-backlight-emulator/Makefile |  19 ++
 .../x86/dell-uart-backlight-emulator/README   |  46 +++++
 .../dell-uart-backlight-emulator.c            | 167 ++++++++++++++++++
 4 files changed, 233 insertions(+)
 create mode 100644 tools/arch/x86/dell-uart-backlight-emulator/.gitignore
 create mode 100644 tools/arch/x86/dell-uart-backlight-emulator/Makefile
 create mode 100644 tools/arch/x86/dell-uart-backlight-emulator/README
 create mode 100644 tools/arch/x86/dell-uart-backlight-emulator/dell-uart-backlight-emulator.c

diff --git a/tools/arch/x86/dell-uart-backlight-emulator/.gitignore b/tools/arch/x86/dell-uart-backlight-emulator/.gitignore
new file mode 100644
index 000000000000..5c8cad8d72b9
--- /dev/null
+++ b/tools/arch/x86/dell-uart-backlight-emulator/.gitignore
@@ -0,0 +1 @@
+dell-uart-backlight-emulator
diff --git a/tools/arch/x86/dell-uart-backlight-emulator/Makefile b/tools/arch/x86/dell-uart-backlight-emulator/Makefile
new file mode 100644
index 000000000000..6ea1d9fd534b
--- /dev/null
+++ b/tools/arch/x86/dell-uart-backlight-emulator/Makefile
@@ -0,0 +1,19 @@
+# SPDX-License-Identifier: GPL-2.0
+# Makefile for Intel Software Defined Silicon provisioning tool
+
+dell-uart-backlight-emulator: dell-uart-backlight-emulator.c
+
+BINDIR ?= /usr/bin
+
+override CFLAGS += -O2 -Wall
+
+%: %.c
+	$(CC) $(CFLAGS) -o $@ $< $(LDFLAGS)
+
+.PHONY : clean
+clean :
+	@rm -f dell-uart-backlight-emulator
+
+install : dell-uart-backlight-emulator
+	install -d $(DESTDIR)$(BINDIR)
+	install -m 755 -p dell-uart-backlight-emulator $(DESTDIR)$(BINDIR)/dell-uart-backlight-emulator
diff --git a/tools/arch/x86/dell-uart-backlight-emulator/README b/tools/arch/x86/dell-uart-backlight-emulator/README
new file mode 100644
index 000000000000..c0d8e52046ee
--- /dev/null
+++ b/tools/arch/x86/dell-uart-backlight-emulator/README
@@ -0,0 +1,46 @@
+Emulator for DELL0501 UART attached backlight controller
+--------------------------------------------------------
+
+Dell All In One (AIO) models released after 2017 use a backlight controller
+board connected to an UART.
+
+In DSDT this uart port will be defined as:
+
+   Name (_HID, "DELL0501")
+   Name (_CID, EisaId ("PNP0501")
+
+With the DELL0501 indicating that we are dealing with an UART with
+the backlight controller board attached.
+
+This small emulator allows testing
+the drivers/platform/x86/dell/dell-uart-backlight.c driver without access
+to an actual Dell All In One.
+
+This requires:
+1. A (desktop) PC with a 16550 UART on the motherboard and a standard DB9
+   connector connected to this UART.
+2. A DB9 NULL modem cable.
+3. A second DB9 serial port, this can e.g. be a USB to serial converter
+   with a DB9 connector plugged into the same desktop PC.
+4. A DSDT overlay for the desktop PC replacing the _HID of the 16550 UART
+   ACPI Device() with "DELL0501" and adding a _CID of "PNP0501", see
+   DSDT.patch for an example of the necessary DSDT changes.
+
+With everything setup and the NULL modem cable connected between
+the 2 serial ports run:
+
+./dell-uart-backlight-emulator <path-to-/dev/tty*S#-for-second-port>
+
+For example when using an USB to serial converter for the second port:
+
+./dell-uart-backlight-emulator /dev/ttyUSB0
+
+And then (re)load the dell-uart-backlight driver:
+
+sudo rmmod dell-uart-backlight; sudo modprobe dell-uart-backlight dyndbg
+
+After this check "dmesg" to see if the driver correctly received
+the firmware version string from the emulator. If this works there
+should be a /sys/class/backlight/dell_uart_backlight/ directory now
+and writes to the brightness or bl_power files should be reflected
+by matching output from the emulator.
diff --git a/tools/arch/x86/dell-uart-backlight-emulator/dell-uart-backlight-emulator.c b/tools/arch/x86/dell-uart-backlight-emulator/dell-uart-backlight-emulator.c
new file mode 100644
index 000000000000..1cd264631a8f
--- /dev/null
+++ b/tools/arch/x86/dell-uart-backlight-emulator/dell-uart-backlight-emulator.c
@@ -0,0 +1,167 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Dell AIO Serial Backlight board emulator for testing
+ * the Linux dell-uart-backlight driver.
+ *
+ * Copyright (C) 2024 Hans de Goede <hansg@kernel.org>
+ */
+#include <errno.h>
+#include <fcntl.h>
+#include <signal.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sys/ioctl.h>
+#include <sys/stat.h>
+#include <sys/types.h>
+#include <sys/un.h>
+#include <termios.h>
+#include <unistd.h>
+
+int serial_fd;
+int brightness = 50;
+
+static unsigned char dell_uart_checksum(unsigned char *buf, int len)
+{
+	unsigned char val = 0;
+
+	while (len-- > 0)
+		val += buf[len];
+
+	return val ^ 0xff;
+}
+
+/* read() will return -1 on SIGINT / SIGTERM causing the mainloop to cleanly exit */
+void signalhdlr(int signum)
+{
+}
+
+int main(int argc, char *argv[])
+{
+	struct sigaction sigact = { .sa_handler = signalhdlr };
+	unsigned char buf[4], csum, response[32];
+	const char *version_str = "PHI23-V321";
+	struct termios tty, saved_tty;
+	int ret, idx, len = 0;
+
+	if (argc != 2) {
+		fprintf(stderr, "Invalid or missing arguments\n");
+		fprintf(stderr, "Usage: %s <serial-port>\n", argv[0]);
+		return 1;
+	}
+
+	serial_fd = open(argv[1], O_RDWR | O_NOCTTY);
+	if (serial_fd == -1) {
+		fprintf(stderr, "Error opening %s: %s\n", argv[1], strerror(errno));
+		return 1;
+	}
+
+	ret = tcgetattr(serial_fd, &tty);
+	if (ret == -1) {
+		fprintf(stderr, "Error getting tcattr: %s\n", strerror(errno));
+		goto out_close;
+	}
+	saved_tty = tty;
+
+	cfsetspeed(&tty, 9600);
+	cfmakeraw(&tty);
+	tty.c_cflag &= ~CSTOPB;
+	tty.c_cflag &= ~CRTSCTS;
+	tty.c_cflag |= CLOCAL | CREAD;
+
+	ret = tcsetattr(serial_fd, TCSANOW, &tty);
+	if (ret == -1) {
+		fprintf(stderr, "Error setting tcattr: %s\n", strerror(errno));
+		goto out_restore;
+	}
+
+	sigaction(SIGINT, &sigact, 0);
+	sigaction(SIGTERM, &sigact, 0);
+
+	idx = 0;
+	while (read(serial_fd, &buf[idx], 1) == 1) {
+		if (idx == 0) {
+			switch (buf[0]) {
+			/* 3 MSB bits: cmd-len + 01010 SOF marker */
+			case 0x6a: len = 3; break;
+			case 0x8a: len = 4; break;
+			default:
+				fprintf(stderr, "Error unexpected first byte: 0x%02x\n", buf[0]);
+				continue; /* Try to sync up with sender */
+			}
+		}
+
+		/* Process msg when len bytes have been received */
+		if (idx != (len - 1)) {
+			idx++;
+			continue;
+		}
+
+		/* Reset idx for next command */
+		idx = 0;
+
+		csum = dell_uart_checksum(buf, len - 1);
+		if (buf[len - 1] != csum) {
+			fprintf(stderr, "Error checksum mismatch got 0x%02x expected 0x%02x\n",
+				buf[len - 1], csum);
+			continue;
+		}
+
+		switch ((buf[0] << 8) | buf[1]) {
+		case 0x6a06:
+			/* cmd = 0x06, get version */
+			len = strlen(version_str);
+			strcpy((char *)&response[2], version_str);
+			printf("Get version, reply: %s\n", version_str);
+			break;
+		case 0x8a0b: /* 3 MSB bits: cmd-len + 01010 SOF marker */
+			/* cmd = 0x0b, set brightness */
+			if (buf[2] > 100) {
+				fprintf(stderr, "Error invalid brightness param: %d\n", buf[2]);
+				continue;
+			}
+
+			len = 0;
+			brightness = buf[2];
+			printf("Set brightness %d\n", brightness);
+			break;
+		case 0x6a0c:
+			/* cmd = 0x0c, get brightness */
+			len = 1;
+			response[2] = brightness;
+			printf("Get brightness, reply: %d\n", brightness);
+			break;
+		case 0x8a0e:
+			/* cmd = 0x0e, set backlight power */
+			if (buf[2] != 0 && buf[2] != 1) {
+				fprintf(stderr, "Error invalid set power param: %d\n", buf[2]);
+				continue;
+			}
+
+			len = 0;
+			printf("Set power %d\n", buf[2]);
+			break;
+		default:
+			fprintf(stderr, "Error unknown cmd 0x%04x\n",
+				(buf[0] << 8) | buf[1]);
+			continue;
+		}
+
+		/* Respond with <total-len> <cmd> <data...> <csum> */
+		response[0] = len + 3; /* response length in bytes */
+		response[1] = buf[1];  /* ack cmd */
+		csum = dell_uart_checksum(response, len + 2);
+		response[len + 2] = csum;
+		ret = write(serial_fd, response, len + 3);
+		if (ret != (len + 3))
+			fprintf(stderr, "Error writing %d bytes: %d\n",
+				len + 3, ret);
+	}
+
+	ret = 0;
+out_restore:
+	tcsetattr(serial_fd, TCSANOW, &saved_tty);
+out_close:
+	close(serial_fd);
+	return ret;
+}
-- 
2.44.0

Re: [PATCH v2 2/2] tools arch x86: Add dell-uart-backlight-emulator

[Andy Shevchenko]

On Mon, May 13, 2024 at 01:15:51PM +0200, Hans de Goede wrote:
> Dell All In One (AIO) models released after 2017 use a backlight controller
> board connected to an UART.
> 
> Add a small emulator to allow development and testing of
> the drivers/platform/x86/dell/dell-uart-backlight.c driver for
> this board, without requiring access to an actual Dell All In One.

...

> +	if (argc != 2) {
> +		fprintf(stderr, "Invalid or missing arguments\n");
> +		fprintf(stderr, "Usage: %s <serial-port>\n", argv[0]);
> +		return 1;
> +	}
> +
> +	serial_fd = open(argv[1], O_RDWR | O_NOCTTY);
> +	if (serial_fd == -1) {
> +		fprintf(stderr, "Error opening %s: %s\n", argv[1], strerror(errno));
> +		return 1;

So, looking at the `man error` it works like your custom approach with an
additional things.

Also, don't you want to use either different error codes (above is +1 and all
below seems using -1), or be consistent and return -1 always?

> +	}

...

> +		/* Respond with <total-len> <cmd> <data...> <csum> */
> +		response[0] = len + 3; /* response length in bytes */
> +		response[1] = buf[1];  /* ack cmd */
> +		csum = dell_uart_checksum(response, len + 2);
> +		response[len + 2] = csum;

> +		ret = write(serial_fd, response, len + 3);

response[0] can be reused here.

> +		if (ret != (len + 3))

And here.

> +			fprintf(stderr, "Error writing %d bytes: %d\n",
> +				len + 3, ret);
> +	}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/2] platform/x86: Add new Dell UART backlight driver

[Andy Shevchenko]

On Mon, May 13, 2024 at 01:15:50PM +0200, Hans de Goede wrote:
> Dell All In One (AIO) models released after 2017 use a backlight controller
> board connected to an UART.
> 
> In DSDT this uart port will be defined as:
> 
>    Name (_HID, "DELL0501")
>    Name (_CID, EisaId ("PNP0501")
> 
> Instead of having a separate ACPI device with an UartSerialBusV2() resource
> to model the backlight-controller, which would be the standard way to do
> this.
> 
> The acpi_quirk_skip_serdev_enumeration() has special handling for this
> and it will make the serial port code create a serdev controller device
> for the UART instead of a /dev/ttyS0 char-dev. It will also create
> a dell-uart-backlight driver platform device for this driver to bind too.
> 
> This new kernel module contains 2 drivers for this:
> 
> 1. A simple platform driver which creates the actual serdev device
>    (with the serdev controller device as parent)
> 
> 2. A serdev driver for the created serdev device which exports
>    the backlight functionality uses a standard backlight class device.

...

> Reported-by: Roman Bogoyev <roman@computercheck.com.au>

Privately? I mean no links to the report?

...

> +config DELL_UART_BACKLIGHT
> +	tristate "Dell AIO UART Backlight driver"
> +	depends on ACPI

Can it be compile-tested in non-ACPI kernels?

> +	depends on BACKLIGHT_CLASS_DEVICE
> +	depends on SERIAL_DEV_BUS

...

> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

Yeah, I don't like this, can we move it into header itself?

> +#include <linux/acpi.h>
> +#include <linux/backlight.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/platform_device.h>
> +#include <linux/serdev.h>
> +#include <linux/string.h>
> +#include <linux/types.h>
> +#include <linux/wait.h>

> +#include "../serdev_helpers.h"

...

> +/* 1st byte Start Of Frame 3 MSB bits: cmd-len + 01010 SOF marker */
> +#define SOF(len)			(((len) << 5) | 0x0a)

This kinda too short to be somehow unique, potential collision might be if
somebody introduces this in the header which somehow will be chain-included
here. Perhaps a namespace? DELL_SOF?

...

> +static int dell_uart_bl_command(struct dell_uart_backlight *dell_bl,
> +				const u8 *cmd, int cmd_len,
> +				u8 *resp, int resp_max_len)
> +{
> +	int ret;
> +
> +	ret = mutex_lock_killable(&dell_bl->mutex);

Can't be called via cleanup.h?

> +	if (ret)
> +		return ret;
> +
> +	dell_bl->status = -EBUSY;
> +	dell_bl->resp = resp;
> +	dell_bl->resp_idx = 0;
> +	dell_bl->resp_max_len = resp_max_len;
> +	dell_bl->pending_cmd = cmd[1];
> +
> +	/* The TTY buffer should be big enough to take the entire cmd in one go */
> +	ret = serdev_device_write_buf(to_serdev_device(dell_bl->dev), cmd, cmd_len);
> +	if (ret != cmd_len) {
> +		dev_err(dell_bl->dev, "Error writing command: %d\n", ret);
> +		ret = (ret < 0) ? ret : -EIO;
> +		goto out;
> +	}
> +
> +	ret = wait_event_timeout(dell_bl->wait_queue, dell_bl->status != -EBUSY,
> +				 DELL_BL_TIMEOUT);
> +	if (ret == 0) {
> +		dev_err(dell_bl->dev, "Timed out waiting for response.\n");
> +		ret = -ETIMEDOUT;
> +	} else {
> +		ret = dell_bl->status;
> +	}
> +
> +out:
> +	mutex_unlock(&dell_bl->mutex);
> +	return ret;
> +}

> +static int dell_uart_get_brightness(struct dell_uart_backlight *dell_bl)
> +{

	struct device *dev = dell_bl->dev;

> +	u8 get_brightness[GET_CMD_LEN], resp[GET_RESP_LEN];
> +	int ret;
> +
> +	get_brightness[0] = SOF(GET_CMD_LEN);
> +	get_brightness[1] = CMD_GET_BRIGHTNESS;
> +	get_brightness[2] = dell_uart_checksum(get_brightness, 2);

> +	ret = dell_uart_bl_command(dell_bl, get_brightness, GET_CMD_LEN, resp, GET_RESP_LEN);
> +	if (ret)
> +		return ret;
> +
> +	if (resp[RESP_LEN] != GET_RESP_LEN) {
> +		dev_err(dell_bl->dev, "Unexpected get brightness response length: %d\n",
> +			resp[RESP_LEN]);

		dev_err(dev, "Unexpected get brightness response length: %d\n", resp[RESP_LEN]);

> +		return -EIO;
> +	}
> +
> +	if (resp[RESP_DATA] > DELL_BL_MAX_BRIGHTNESS) {
> +		dev_err(dell_bl->dev, "Unexpected get brightness response: %d\n",
> +			resp[RESP_DATA]);

		dev_err(dev, "Unexpected get brightness response: %d\n", resp[RESP_DATA]);

> +		return -EIO;
> +	}
> +
> +	return resp[RESP_DATA];
> +}

...

> +		case RESP_CMD: /* CMD byte */
> +			if (dell_bl->resp[RESP_CMD] != dell_bl->pending_cmd) {
> +				dev_err(dell_bl->dev, "Response cmd 0x%02x != pending 0x%02x\n",
> +					dell_bl->resp[RESP_CMD], dell_bl->pending_cmd);
> +				dell_bl->status = -EIO;
> +				goto wakeup;
> +			}
> +			break;

No default case?

...

> +	dev_dbg(dev, "Firmware version: %.*s\n", resp[RESP_LEN] - 3, resp + RESP_DATA);

I would be on the safest side, i.e. not trusting that it will be NUL-terminated
string, hence something like %*pE?


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/2] platform/x86: Add new Dell UART backlight driver

[Hans de Goede]

Hi,

On 5/13/24 2:58 PM, Andy Shevchenko wrote:
> On Mon, May 13, 2024 at 01:15:50PM +0200, Hans de Goede wrote:
>> Dell All In One (AIO) models released after 2017 use a backlight controller
>> board connected to an UART.
>>
>> In DSDT this uart port will be defined as:
>>
>>    Name (_HID, "DELL0501")
>>    Name (_CID, EisaId ("PNP0501")
>>
>> Instead of having a separate ACPI device with an UartSerialBusV2() resource
>> to model the backlight-controller, which would be the standard way to do
>> this.
>>
>> The acpi_quirk_skip_serdev_enumeration() has special handling for this
>> and it will make the serial port code create a serdev controller device
>> for the UART instead of a /dev/ttyS0 char-dev. It will also create
>> a dell-uart-backlight driver platform device for this driver to bind too.
>>
>> This new kernel module contains 2 drivers for this:
>>
>> 1. A simple platform driver which creates the actual serdev device
>>    (with the serdev controller device as parent)
>>
>> 2. A serdev driver for the created serdev device which exports
>>    the backlight functionality uses a standard backlight class device.
> 
> ...
> 
>> Reported-by: Roman Bogoyev <roman@computercheck.com.au>
> 
> Privately? I mean no links to the report?

Yes by private email.
 
<snip>


>> +/* 1st byte Start Of Frame 3 MSB bits: cmd-len + 01010 SOF marker */
>> +#define SOF(len)			(((len) << 5) | 0x0a)
> 
> This kinda too short to be somehow unique, potential collision might be if
> somebody introduces this in the header which somehow will be chain-included
> here. Perhaps a namespace? DELL_SOF?

Ack will fix for v3.

> 
> ...
> 
>> +static int dell_uart_bl_command(struct dell_uart_backlight *dell_bl,
>> +				const u8 *cmd, int cmd_len,
>> +				u8 *resp, int resp_max_len)
>> +{
>> +	int ret;
>> +
>> +	ret = mutex_lock_killable(&dell_bl->mutex);
> 
> Can't be called via cleanup.h?

I prefer to have the locking explicit rather then use cleanup.h .

<snip>

>> +static int dell_uart_get_brightness(struct dell_uart_backlight *dell_bl)
>> +{
> 
> 	struct device *dev = dell_bl->dev;
> 
>> +	u8 get_brightness[GET_CMD_LEN], resp[GET_RESP_LEN];
>> +	int ret;
>> +
>> +	get_brightness[0] = SOF(GET_CMD_LEN);
>> +	get_brightness[1] = CMD_GET_BRIGHTNESS;
>> +	get_brightness[2] = dell_uart_checksum(get_brightness, 2);
> 
>> +	ret = dell_uart_bl_command(dell_bl, get_brightness, GET_CMD_LEN, resp, GET_RESP_LEN);
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (resp[RESP_LEN] != GET_RESP_LEN) {
>> +		dev_err(dell_bl->dev, "Unexpected get brightness response length: %d\n",
>> +			resp[RESP_LEN]);
> 
> 		dev_err(dev, "Unexpected get brightness response length: %d\n", resp[RESP_LEN]);
> 
>> +		return -EIO;
>> +	}
>> +
>> +	if (resp[RESP_DATA] > DELL_BL_MAX_BRIGHTNESS) {
>> +		dev_err(dell_bl->dev, "Unexpected get brightness response: %d\n",
>> +			resp[RESP_DATA]);
> 
> 		dev_err(dev, "Unexpected get brightness response: %d\n", resp[RESP_DATA]);
> 

Ack will fix for v3.

>> +		return -EIO;
>> +	}
>> +
>> +	return resp[RESP_DATA];
>> +}
> 
> ...
> 
>> +		case RESP_CMD: /* CMD byte */
>> +			if (dell_bl->resp[RESP_CMD] != dell_bl->pending_cmd) {
>> +				dev_err(dell_bl->dev, "Response cmd 0x%02x != pending 0x%02x\n",
>> +					dell_bl->resp[RESP_CMD], dell_bl->pending_cmd);
>> +				dell_bl->status = -EIO;
>> +				goto wakeup;
>> +			}
>> +			break;
> 
> No default case?

Nope, this checks the validity of the first 2 bytes / the header. The data is not checked.

> 
> ...
> 
>> +	dev_dbg(dev, "Firmware version: %.*s\n", resp[RESP_LEN] - 3, resp + RESP_DATA);
> 
> I would be on the safest side, i.e. not trusting that it will be NUL-terminated
> string, hence something like %*pE?

Right, this is why the existing dev_dbg() already passes a precision and we do
want to actually stop if there is a 0 there, which %pE does not do.

Regards,

Hans



> 
> 

Re: [PATCH v2 2/2] tools arch x86: Add dell-uart-backlight-emulator

[Hans de Goede]

Hi,

On 5/13/24 2:46 PM, Andy Shevchenko wrote:
> On Mon, May 13, 2024 at 01:15:51PM +0200, Hans de Goede wrote:
>> Dell All In One (AIO) models released after 2017 use a backlight controller
>> board connected to an UART.
>>
>> Add a small emulator to allow development and testing of
>> the drivers/platform/x86/dell/dell-uart-backlight.c driver for
>> this board, without requiring access to an actual Dell All In One.
> 
> ...
> 
>> +	if (argc != 2) {
>> +		fprintf(stderr, "Invalid or missing arguments\n");
>> +		fprintf(stderr, "Usage: %s <serial-port>\n", argv[0]);
>> +		return 1;
>> +	}
>> +
>> +	serial_fd = open(argv[1], O_RDWR | O_NOCTTY);
>> +	if (serial_fd == -1) {
>> +		fprintf(stderr, "Error opening %s: %s\n", argv[1], strerror(errno));
>> +		return 1;
> 
> So, looking at the `man error` it works like your custom approach with an
> additional things.

But that is a GNU / glibc extension. I would prefer to stick with plain libc
here, so that this can be build with other libc-s too. 
> Also, don't you want to use either different error codes (above is +1 and all
> below seems using -1), or be consistent and return -1 always?

the +1 code is for when the cmdline argument is likely wrong , -1 / 255 is
for IO errors.

> 
>> +	}
> 
> ...
> 
>> +		/* Respond with <total-len> <cmd> <data...> <csum> */
>> +		response[0] = len + 3; /* response length in bytes */
>> +		response[1] = buf[1];  /* ack cmd */
>> +		csum = dell_uart_checksum(response, len + 2);
>> +		response[len + 2] = csum;
> 
>> +		ret = write(serial_fd, response, len + 3);
> 
> response[0] can be reused here.
> 
>> +		if (ret != (len + 3))
> 
> And here.

Ack will fix.

Regards,

Hans


> 
>> +			fprintf(stderr, "Error writing %d bytes: %d\n",
>> +				len + 3, ret);
>> +	}
> 

Re: [PATCH v2 1/2] platform/x86: Add new Dell UART backlight driver

[Andy Shevchenko]

On Mon, May 13, 2024 at 03:18:10PM +0200, Hans de Goede wrote:
> On 5/13/24 2:58 PM, Andy Shevchenko wrote:
> > On Mon, May 13, 2024 at 01:15:50PM +0200, Hans de Goede wrote:

...

> >> +static int dell_uart_bl_command(struct dell_uart_backlight *dell_bl,
> >> +				const u8 *cmd, int cmd_len,
> >> +				u8 *resp, int resp_max_len)
> >> +{
> >> +	int ret;
> >> +
> >> +	ret = mutex_lock_killable(&dell_bl->mutex);
> > 
> > Can't be called via cleanup.h?
> 
> I prefer to have the locking explicit rather then use cleanup.h .

Hmm... interesting, so you push-back the cleanup.h usage?

<snip>

> >> +		case RESP_CMD: /* CMD byte */
> >> +			if (dell_bl->resp[RESP_CMD] != dell_bl->pending_cmd) {
> >> +				dev_err(dell_bl->dev, "Response cmd 0x%02x != pending 0x%02x\n",
> >> +					dell_bl->resp[RESP_CMD], dell_bl->pending_cmd);
> >> +				dell_bl->status = -EIO;
> >> +				goto wakeup;
> >> +			}
> >> +			break;
> > 
> > No default case?
> 
> Nope, this checks the validity of the first 2 bytes / the header. The data is not checked.

Why not

		default:
			/* We do not check the data */
			break;

?

...

> >> +	dev_dbg(dev, "Firmware version: %.*s\n", resp[RESP_LEN] - 3, resp + RESP_DATA);
> > 
> > I would be on the safest side, i.e. not trusting that it will be NUL-terminated
> > string, hence something like %*pE?
> 
> Right, this is why the existing dev_dbg() already passes a precision and we do
> want to actually stop if there is a 0 there, which %pE does not do.

I'm talking about the opposite, when it might go over the boundary.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/2] platform/x86: Add new Dell UART backlight driver

[Hans de Goede]

Hi,

On 5/13/24 5:19 PM, Andy Shevchenko wrote:
> On Mon, May 13, 2024 at 03:18:10PM +0200, Hans de Goede wrote:
>> On 5/13/24 2:58 PM, Andy Shevchenko wrote:
>>> On Mon, May 13, 2024 at 01:15:50PM +0200, Hans de Goede wrote:
> 
> ...
> 
>>>> +static int dell_uart_bl_command(struct dell_uart_backlight *dell_bl,
>>>> +				const u8 *cmd, int cmd_len,
>>>> +				u8 *resp, int resp_max_len)
>>>> +{
>>>> +	int ret;
>>>> +
>>>> +	ret = mutex_lock_killable(&dell_bl->mutex);
>>>
>>> Can't be called via cleanup.h?
>>
>> I prefer to have the locking explicit rather then use cleanup.h .
> 
> Hmm... interesting, so you push-back the cleanup.h usage?

I'm in favor of the guard(mutex)(&smne_mutex); syntax, but this
is a mutex_lock_killable() for which that does not work AFAIK.

So in this case AFAICT we would need to use the cleanup stuff manually
and in that case I believe that in that case just sticking with
the current code is better.

> 
> <snip>
> 
>>>> +		case RESP_CMD: /* CMD byte */
>>>> +			if (dell_bl->resp[RESP_CMD] != dell_bl->pending_cmd) {
>>>> +				dev_err(dell_bl->dev, "Response cmd 0x%02x != pending 0x%02x\n",
>>>> +					dell_bl->resp[RESP_CMD], dell_bl->pending_cmd);
>>>> +				dell_bl->status = -EIO;
>>>> +				goto wakeup;
>>>> +			}
>>>> +			break;
>>>
>>> No default case?
>>
>> Nope, this checks the validity of the first 2 bytes / the header. The data is not checked.
> 
> Why not
> 
> 		default:
> 			/* We do not check the data */
> 			break;
> 
> ?

TBH I don't see any added value in adding that.

> 
> ...
> 
>>>> +	dev_dbg(dev, "Firmware version: %.*s\n", resp[RESP_LEN] - 3, resp + RESP_DATA);
>>>
>>> I would be on the safest side, i.e. not trusting that it will be NUL-terminated
>>> string, hence something like %*pE?
>>
>> Right, this is why the existing dev_dbg() already passes a precision and we do
>> want to actually stop if there is a 0 there, which %pE does not do.
> 
> I'm talking about the opposite, when it might go over the boundary.

AFAIK the way the precision is used in the current code limits things to at max the boundary,
stopping earlier if a 0 is encountered earlier.

Regards,

Hans

Re: [PATCH v2 1/2] platform/x86: Add new Dell UART backlight driver

[Andy Shevchenko]

On Mon, May 13, 2024 at 05:33:10PM +0200, Hans de Goede wrote:
> On 5/13/24 5:19 PM, Andy Shevchenko wrote:
> > On Mon, May 13, 2024 at 03:18:10PM +0200, Hans de Goede wrote:
> >> On 5/13/24 2:58 PM, Andy Shevchenko wrote:
> >>> On Mon, May 13, 2024 at 01:15:50PM +0200, Hans de Goede wrote:

...

> >>>> +static int dell_uart_bl_command(struct dell_uart_backlight *dell_bl,
> >>>> +				const u8 *cmd, int cmd_len,
> >>>> +				u8 *resp, int resp_max_len)
> >>>> +{
> >>>> +	int ret;
> >>>> +
> >>>> +	ret = mutex_lock_killable(&dell_bl->mutex);
> >>>
> >>> Can't be called via cleanup.h?
> >>
> >> I prefer to have the locking explicit rather then use cleanup.h .
> > 
> > Hmm... interesting, so you push-back the cleanup.h usage?
> 
> I'm in favor of the guard(mutex)(&smne_mutex); syntax, but this
> is a mutex_lock_killable() for which that does not work AFAIK.
> 
> So in this case AFAICT we would need to use the cleanup stuff manually
> and in that case I believe that in that case just sticking with
> the current code is better.

There is scoped_cond_guard().
But there is no DEFINE_GUARD_COND() for mutex_lock_killable().

...

> >>>> +	dev_dbg(dev, "Firmware version: %.*s\n", resp[RESP_LEN] - 3, resp + RESP_DATA);
> >>>
> >>> I would be on the safest side, i.e. not trusting that it will be NUL-terminated
> >>> string, hence something like %*pE?
> >>
> >> Right, this is why the existing dev_dbg() already passes a precision and we do
> >> want to actually stop if there is a 0 there, which %pE does not do.
> > 
> > I'm talking about the opposite, when it might go over the boundary.
> 
> AFAIK the way the precision is used in the current code limits things to at max the boundary,
> stopping earlier if a 0 is encountered earlier.

Indeed, I refreshed my memory about %.*s.

So, the only part left is the potential terminal sequences appear in
the message.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/2] platform/x86: Add new Dell UART backlight driver

[Andy Shevchenko]

On Mon, May 13, 2024 at 06:44:37PM +0300, Andy Shevchenko wrote:
> On Mon, May 13, 2024 at 05:33:10PM +0200, Hans de Goede wrote:
> > On 5/13/24 5:19 PM, Andy Shevchenko wrote:
> > > On Mon, May 13, 2024 at 03:18:10PM +0200, Hans de Goede wrote:
> > >> On 5/13/24 2:58 PM, Andy Shevchenko wrote:
> > >>> On Mon, May 13, 2024 at 01:15:50PM +0200, Hans de Goede wrote:

...

> > >>>> +	dev_dbg(dev, "Firmware version: %.*s\n", resp[RESP_LEN] - 3, resp + RESP_DATA);
> > >>>
> > >>> I would be on the safest side, i.e. not trusting that it will be NUL-terminated
> > >>> string, hence something like %*pE?
> > >>
> > >> Right, this is why the existing dev_dbg() already passes a precision and we do
> > >> want to actually stop if there is a 0 there, which %pE does not do.
> > > 
> > > I'm talking about the opposite, when it might go over the boundary.
> > 
> > AFAIK the way the precision is used in the current code limits things to at max the boundary,
> > stopping earlier if a 0 is encountered earlier.
> 
> Indeed, I refreshed my memory about %.*s.
> 
> So, the only part left is the potential terminal sequences appear in
> the message.

Just for clarity, the %*pE would require strnlen() call to have similar effect.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/2] platform/x86: Add new Dell UART backlight driver

[Ilpo Järvinen]

On Mon, 13 May 2024, Andy Shevchenko wrote:

> On Mon, May 13, 2024 at 05:33:10PM +0200, Hans de Goede wrote:
> > On 5/13/24 5:19 PM, Andy Shevchenko wrote:
> > > On Mon, May 13, 2024 at 03:18:10PM +0200, Hans de Goede wrote:
> > >> On 5/13/24 2:58 PM, Andy Shevchenko wrote:
> > >>> On Mon, May 13, 2024 at 01:15:50PM +0200, Hans de Goede wrote:
> 
> > >>>> +static int dell_uart_bl_command(struct dell_uart_backlight *dell_bl,
> > >>>> +				const u8 *cmd, int cmd_len,
> > >>>> +				u8 *resp, int resp_max_len)
> > >>>> +{
> > >>>> +	int ret;
> > >>>> +
> > >>>> +	ret = mutex_lock_killable(&dell_bl->mutex);
> > >>>
> > >>> Can't be called via cleanup.h?
> > >>
> > >> I prefer to have the locking explicit rather then use cleanup.h .
> > > 
> > > Hmm... interesting, so you push-back the cleanup.h usage?
> > 
> > I'm in favor of the guard(mutex)(&smne_mutex); syntax, but this
> > is a mutex_lock_killable() for which that does not work AFAIK.
> > 
> > So in this case AFAICT we would need to use the cleanup stuff manually
> > and in that case I believe that in that case just sticking with
> > the current code is better.
> 
> There is scoped_cond_guard().
> But there is no DEFINE_GUARD_COND() for mutex_lock_killable().

Is there a way to return the original error code with scoped_cond_guard() 
or does that it force overriding the original return value with a 
hard-coded one provided by the caller which seems a step backwards?


-- 
 i.

Re: [PATCH v2 1/2] platform/x86: Add new Dell UART backlight driver

[Andy Shevchenko]

On Mon, May 13, 2024 at 06:54:25PM +0300, Ilpo Järvinen wrote:
> On Mon, 13 May 2024, Andy Shevchenko wrote:
> > On Mon, May 13, 2024 at 05:33:10PM +0200, Hans de Goede wrote:
> > > On 5/13/24 5:19 PM, Andy Shevchenko wrote:
> > > > On Mon, May 13, 2024 at 03:18:10PM +0200, Hans de Goede wrote:
> > > >> On 5/13/24 2:58 PM, Andy Shevchenko wrote:
> > > >>> On Mon, May 13, 2024 at 01:15:50PM +0200, Hans de Goede wrote:
> > 
> > > >>>> +static int dell_uart_bl_command(struct dell_uart_backlight *dell_bl,
> > > >>>> +				const u8 *cmd, int cmd_len,
> > > >>>> +				u8 *resp, int resp_max_len)
> > > >>>> +{
> > > >>>> +	int ret;
> > > >>>> +
> > > >>>> +	ret = mutex_lock_killable(&dell_bl->mutex);
> > > >>>
> > > >>> Can't be called via cleanup.h?
> > > >>
> > > >> I prefer to have the locking explicit rather then use cleanup.h .
> > > > 
> > > > Hmm... interesting, so you push-back the cleanup.h usage?
> > > 
> > > I'm in favor of the guard(mutex)(&smne_mutex); syntax, but this
> > > is a mutex_lock_killable() for which that does not work AFAIK.
> > > 
> > > So in this case AFAICT we would need to use the cleanup stuff manually
> > > and in that case I believe that in that case just sticking with
> > > the current code is better.
> > 
> > There is scoped_cond_guard().
> > But there is no DEFINE_GUARD_COND() for mutex_lock_killable().
> 
> Is there a way to return the original error code with scoped_cond_guard() 
> or does that it force overriding the original return value with a 
> hard-coded one provided by the caller which seems a step backwards?

Yeah, that's also a downside. Perhaps you can reply to the original thread
about this. But it seems it was developed for boolean type of functions w/o
thinking a lot about returned values.

-- 
With Best Regards,
Andy Shevchenko