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@xxxxxxxxxxxxxxxxxxxx>
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
