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