| 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
| ||
|
Message-ID: <3c0422d9-8ce9-64d8-f332-e07bc8e207c4@infradead.org>
Date: Wed, 9 Mar 2022 23:13:03 -0800
From: Randy Dunlap <rdunlap@...radead.org>
To: Yusuf Khan <yusisamerican@...il.com>, linux-kernel@...r.kernel.org
Cc: jasowang@...hat.com, mikelley@...rosoft.com, mst@...hat.com,
gregkh@...uxfoundation.org, javier@...igon.com, arnd@...db.de,
will@...nel.org, axboe@...nel.dk
Subject: Re: [PATCH v4] drivers: ddcci: upstream DDCCI driver
On 3/9/22 21:59, Yusuf Khan wrote:
> This patch upstreams the DDCCI driver by Christoph Grenz into
> the kernel. The original gitlab page is loacted at https://gitlab
> .com/ddcci-driver-linux/ddcci-driver-linux/-/tree/master.
>
> Signed-off-by: Yusuf Khan <yusisamerican@...il.com>
> ---
> v2: Fix typos.
>
> v3: Add documentation, move files around, replace semaphores with
> mutexes, and replaced <asm-generic/fcntl.h> with <linux/fcntl.h>.
> "imirkin"(which due to his involvement in the dri-devel irc channel
> I cant help but assume to be a DRM developer) said that the DDC/CI
> bus does not intefere with the buses that DRM is involved with.
>
> v4: Move some documentation, fix grammer mistakes, remove usages of
> likely(), and clarify some documentation.
> ---
> Documentation/ABI/testing/sysfs-driver-ddcci | 37 +
> Documentation/driver-api/ddcci.rst | 107 +
> drivers/char/Kconfig | 10 +
> drivers/char/Makefile | 1 +
> drivers/char/ddcci.c | 1887 ++++++++++++++++++
> drivers/video/backlight/Kconfig | 10 +
> drivers/video/backlight/Makefile | 1 +
> drivers/video/backlight/ddcci-backlight.c | 411 ++++
> include/linux/ddcci.h | 164 ++
> 9 files changed, 2628 insertions(+)
> create mode 100644 Documentation/ABI/testing/sysfs-driver-ddcci
> create mode 100644 Documentation/driver-api/ddcci.rst
> create mode 100644 drivers/char/ddcci.c
> create mode 100644 drivers/video/backlight/ddcci-backlight.c
> create mode 100644 include/linux/ddcci.h
>
> diff --git a/Documentation/driver-api/ddcci.rst b/Documentation/driver-api/ddcci.rst
> new file mode 100644
> index 000000000000..b4a4b694d326
> --- /dev/null
> +++ b/Documentation/driver-api/ddcci.rst
> @@ -0,0 +1,107 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
...
> +
> +options ddcci dyndbg
> +options ddcci-backlight dyndbg
> \ No newline at end of file
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> diff --git a/drivers/char/Kconfig b/drivers/char/Kconfig
> index 740811893c57..ec36fae7ee38 100644
> --- a/drivers/char/Kconfig
> +++ b/drivers/char/Kconfig
> @@ -451,4 +451,14 @@ config RANDOM_TRUST_BOOTLOADER
> pool. Otherwise, say N here so it will be regarded as device input that
> only mixes the entropy pool.
>
> +config DDCCI
> + tristate "DDCCI display protocol support"
Add:
depends on I2C
> + help
> + Display Data Channel Command Interface is a
is an
(as in my previous comments)
> + interface that allows the kernel to "talk"
> + to most displays made after 2005. Check your
> + display's specification to see if it has
> + support for this. This depends on I2C to
> + compile.
> +
> endmenu
> diff --git a/drivers/char/ddcci.c b/drivers/char/ddcci.c
> new file mode 100644
> index 000000000000..0a7bc9f63261
> --- /dev/null
> +++ b/drivers/char/ddcci.c
> @@ -0,0 +1,1887 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * DDC/CI sub-bus driver
> + *
> + * Copyright (c) 2015 Christoph Grenz
> + */
> +
> +/*
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published by the Free
> + * Software Foundation; either version 2 of the License, or (at your option)
> + * any later version.
> + */
> +
...
> +/* Write a message to the DDC/CI bus using i2c_smbus_write_byte() */
> +static int __ddcci_write_bytewise(struct i2c_client *client, unsigned char addr,
> + bool p_flag, const unsigned char *__restrict buf,
> + unsigned char len)
> +{
> + int ret = 0;
> + unsigned char outer_addr = (unsigned char)(client->addr << 1);
> + unsigned int xor = outer_addr; /* initial xor value */
> +
> + /* Consistency checks */
> + if (len > 127)
> + return -EINVAL;
> +
> + /* Special case: sender to 0x6E is always 0x51 */
What does that comment mean?
> + if (addr == DDCCI_DEFAULT_DEVICE_ADDR) {
> + addr = DDCCI_HOST_ADDR_ODD;
> + } else {
> + /* When sending the odd address is used */
> + addr = addr | 1;
> + }
> +
> + /* first byte: sender address */
> + xor ^= addr;
> + ret = i2c_smbus_write_byte(client, addr);
> + if (ret < 0)
> + return ret;
> +
> + /* second byte: protocol flag and message size */
> + xor ^= ((p_flag << 7) | len);
> + ret = i2c_smbus_write_byte(client, (p_flag << 7)|len);
> + if (ret < 0)
> + return ret;
> +
> + /* send payload */
> + while (len--) {
> + xor ^= (*buf);
> + ret = i2c_smbus_write_byte(client, (*buf));
> + if (ret < 0)
> + return ret;
> + buf++;
> + }
> +
> + /* send checksum */
> + ret = i2c_smbus_write_byte(client, xor);
> + return ret;
> +}
> +
> +/* Write a message to the DDC/CI bus using i2c_master_send() */
> +static int __ddcci_write_block(struct i2c_client *client, unsigned char addr,
> + unsigned char *sendbuf, bool p_flag,
> + const unsigned char *data, unsigned char len)
> +{
> + unsigned char outer_addr = (unsigned char)(client->addr << 1);
> + unsigned int xor = outer_addr; /* initial xor value */
> + unsigned char *ptr = sendbuf;
> +
> + /* Consistency checks */
> + if (len > 127)
> + return -EINVAL;
> +
> + /* Special case: sender to 0x6E is always 0x51 */
> + if (addr == DDCCI_DEFAULT_DEVICE_ADDR) {
> + addr = DDCCI_HOST_ADDR_ODD;
> + } else {
> + /* When sending the odd address is used */
> + addr = addr | 1;
> + }
> +
> + /* first byte: sender address */
> + xor ^= addr;
> + *(ptr++) = addr;
> + /* second byte: protocol flag and message size */
> + xor ^= ((p_flag << 7) | len);
> + *(ptr++) = (p_flag << 7)|len;
Fix spacing above. (add some spaces)
> + /* payload */
> + while (len--) {
> + xor ^= (*data);
> + *(ptr++) = (*data);
> + data++;
> + }
> + /* checksum */
> + (*ptr) = xor;
> +
> + /* Send it */
> + return i2c_master_send(client, sendbuf, ptr - sendbuf + 1);
> +}
> +
> +/*
> + * Write a message to the DDC/CI bus.
> + *
> + * You must hold the bus mutex when calling this function.
> + */
> +static int ddcci_write(struct i2c_client *client, unsigned char addr,
> + bool p_flag, const unsigned char *data,
> + unsigned char len)
> +{
> + struct ddcci_bus_drv_data *drv_data;
> + unsigned char *sendbuf;
> + int ret;
> +
> + drv_data = i2c_get_clientdata(client);
> +
> +
> + pr_debug("sending to %d:%02x:%02x: %*ph\n", client->adapter->nr,
> + client->addr << 1, addr, len, data);
> + if (drv_data->quirks & DDCCI_QUIRK_WRITE_BYTEWISE) {
> + ret = __ddcci_write_bytewise(client, addr, p_flag, data, len);
> + } else {
> + sendbuf = drv_data->recv_buffer;
> + ret = __ddcci_write_block(client, addr, sendbuf, p_flag, data, len);
> + }
> +
> + return ret;
> +}
> +
> +/*
> + * Read a response from the DDC/CI bus with headers directly into a buffer.
> + * Always check for DDCCI_QUIRK_SKIP_FIRST_BYTE when using this function.
> + * The returned length contains the whole unmodified response.
> + * If -EMSGSIZE is returned, the buffer contains the response up to `len`.
> + * If any other negative error code is returned, the buffer content is
> + * unspecified.
> + */
> +static int __ddcci_read(struct i2c_client *client, unsigned char addr,
> + bool p_flag, unsigned long quirks, unsigned char *buf,
> + unsigned char len)
> +{
> + int i, payload_len, packet_length, ret;
> + unsigned char xor = DDCCI_HOST_ADDR_EVEN;
> +
> + /* Consistency checks */
> + if (len < 3)
> + return -EINVAL;
> +
> + /* Read frame */
> + ret = i2c_master_recv(client, buf, len);
> + if (ret < 0)
> + goto out_err;
> + packet_length = ret;
> +
> + /* Skip first byte if quirk active */
> + if ((quirks & DDCCI_QUIRK_SKIP_FIRST_BYTE) && ret > 0 && len > 0) {
> + ret--;
> + len--;
> + buf++;
> + }
> +
> + /* If answer too short (= incomplete) break out */
> + if (ret < 3) {
> + ret = -EIO;
> + goto out_err;
> + }
> +
> + /* validate first byte */
> + if (unlikely(buf[0] != addr)) {
> + ret = (buf[0] == '\0') ? -EAGAIN : -EIO;
> + goto out_err;
> + }
> +
> + /* validate second byte (protocol flag) */
> + if (unlikely((buf[1] & 0x80) != (p_flag << 7))) {
> + if (!p_flag || !(quirks & DDCCI_QUIRK_NO_PFLAG)) {
> + ret = -EIO;
> + goto out_err;
> + }
> + }
> +
> + /* get and check payload length */
> + payload_len = buf[1] & 0x7F;
> + if (3+payload_len > packet_length)
> + return -EBADMSG;
> + if (3+payload_len > len)
> + return -EMSGSIZE;
> +
> + /* calculate checksum */
> + for (i = 0; i < 3+payload_len; i++)
> + xor ^= buf[i];
> +
> + /* verify checksum */
> + if (xor != 0) {
> + dev_err(&client->dev, "invalid DDC/CI response, corrupted data - xor is 0x%02x, length 0x%02x\n",
> + xor, payload_len);
> + ret = -EBADMSG;
> + goto out_err;
> + }
> +
> + /* return result */
> + ret = payload_len+3+((quirks & DDCCI_QUIRK_SKIP_FIRST_BYTE)?1:0);
ugly :(
> +
> +out_err:
> + return ret;
> +}
> +
and it still contains too much use of likely() and unlikely().
> diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig
> index e32694c13da5..8b7efde28cfd 100644
> --- a/drivers/video/backlight/Kconfig
> +++ b/drivers/video/backlight/Kconfig
> @@ -289,6 +289,16 @@ config BACKLIGHT_QCOM_WLED
> If you have the Qualcomm PMIC, say Y to enable a driver for the
> WLED block. Currently it supports PM8941 and PMI8998.
>
> +config BACKLIGHT_DDCCI
> + tristate "DDCCI Backlight Driver"
add line:
depends on I2C
> + help
> + If you have a DDC/CI supporing monitor, say Y to enable a driver
supporting
> + to control its backlight using DDC/CI. This could be useful if
> + your monitor does not include a backlight driver. For this to be
> + useful you need to enable DDCCI support which can be found in
> + Device Drivers -> Character devices and that further depends on
> + I2C.
> +
> config BACKLIGHT_RT4831
> tristate "Richtek RT4831 Backlight Driver"
> depends on MFD_RT4831
--
~Randy
Powered by blists - more mailing lists