* [PATCH] platform/x86: dell-uart-backlight: recover from stray EC response byte
@ 2026-10-04 11:03 chitranshmandhaniya
2026-10-14 11:50 ` Ilpo Järvinen
0 siblings, 1 reply; 2+ messages in thread
From: chitranshmandhaniya @ 2026-10-04 11:03 UTC (permalink / raw)
To: hansg
Cc: ilpo.jarvinen, platform-driver-x86, linux-kernel,
Chitransh Mandhaniya
From: Chitransh Mandhaniya <Chitransh_Mandhaniya@dell.com>
On some Dell AIO machines the EC occasionally inserts a spurious byte
after the cmd-echo in UART backlight response frames. A get-brightness
reply "04 0c 0a e5" arrives as "04 0c 0d 0a e5". The length field still
reflects the original frame size, so the parser consumes the stray byte
as payload and mistakes the real payload for the checksum, returning
-EIO.
Handle this in dell_uart_bl_receive(): on the first checksum failure for
a frame longer than the minimum size, drop the byte at the first payload
offset, shift the rest down by one, read one more byte as the new
checksum, and re-validate. Limit this single-byte resync to once per
command via a resync_used flag cleared at the start of each transaction.
A second mismatch is still a hard error.
Well-formed replies pass on the first check and never enter the recovery
path.
Signed-off-by: Chitransh Mandhaniya <Chitransh_Mandhaniya@dell.com>
---
.../platform/x86/dell/dell-uart-backlight.c | 92 +++++++++++++------
1 file changed, 62 insertions(+), 30 deletions(-)
diff --git a/drivers/platform/x86/dell/dell-uart-backlight.c b/drivers/platform/x86/dell/dell-uart-backlight.c
index f323a667dc2d..068b91c21bf4 100644
--- a/drivers/platform/x86/dell/dell-uart-backlight.c
+++ b/drivers/platform/x86/dell/dell-uart-backlight.c
@@ -51,7 +51,7 @@
#define MAX_RESP_LEN 80
struct dell_uart_backlight {
- struct mutex mutex;
+ struct mutex mutex; /* Protects command + response state below */
wait_queue_head_t wait_queue;
struct device *dev;
struct backlight_device *bl;
@@ -60,6 +60,7 @@ struct dell_uart_backlight {
u8 resp_len;
u8 resp_max_len;
u8 pending_cmd;
+ bool resync_used;
int status;
int power;
};
@@ -91,6 +92,7 @@ static int dell_uart_bl_command(struct dell_uart_backlight *dell_bl,
dell_bl->resp_len = -1; /* Invalid / unset */
dell_bl->resp_max_len = resp_max_len;
dell_bl->pending_cmd = cmd[1];
+ dell_bl->resync_used = false;
/* 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);
@@ -208,8 +210,10 @@ static const struct backlight_ops dell_uart_backlight_ops = {
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);
+ const int max_passes = 2;
size_t i;
u8 csum;
+ int pass;
dev_dbg(dell_bl->dev, "Recv: %*ph\n", (int)len, data);
@@ -220,42 +224,70 @@ static size_t dell_uart_bl_receive(struct serdev_device *serdev, const u8 *data,
}
i = 0;
- while (i < len && dell_bl->resp_idx != dell_bl->resp_len) {
- 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 ||
- dell_bl->resp_len > dell_bl->resp_max_len) {
- dev_err(dell_bl->dev, "Response length %d out if range %d - %d\n",
- dell_bl->resp_len, MIN_RESP_LEN, dell_bl->resp_max_len);
- dell_bl->status = -EIO;
- goto wakeup;
+ for (pass = 0; pass < max_passes; pass++) {
+ while (i < len && dell_bl->resp_idx != dell_bl->resp_len) {
+ 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 ||
+ dell_bl->resp_len > dell_bl->resp_max_len) {
+ dev_err(dell_bl->dev,
+ "Response length %d out if range %d - %d\n",
+ dell_bl->resp_len, MIN_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;
}
- 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++;
}
- dell_bl->resp_idx++;
- }
- if (dell_bl->resp_idx != dell_bl->resp_len)
- return len; /* Response not complete yet */
+ if (dell_bl->resp_idx != dell_bl->resp_len)
+ return i; /* Response not complete yet, wait for more data */
+
+ csum = dell_uart_checksum(dell_bl->resp, dell_bl->resp_len - 1);
+ if (dell_bl->resp[dell_bl->resp_len - 1] == csum) {
+ dell_bl->status = 0; /* Success */
+ dev_dbg(dell_bl->dev,
+ "Response OK for cmd 0x%02x, len %d: %*ph\n",
+ dell_bl->pending_cmd, dell_bl->resp_len,
+ dell_bl->resp_len, dell_bl->resp);
+ goto wakeup;
+ }
+
+ /*
+ * Some EC firmware inserts a stray byte right after the cmd-echo
+ * byte. Try once to drop it and re-fetch the checksum byte from the wire.
+ */
+ if (!dell_bl->resync_used && dell_bl->resp_len > MIN_RESP_LEN) {
+ dell_bl->resync_used = true;
+
+ dev_warn(dell_bl->dev,
+ "Checksum mismatch got 0x%02x expected 0x%02x, dropping stray byte 0x%02x at offset %d\n",
+ dell_bl->resp[dell_bl->resp_len - 1], csum,
+ dell_bl->resp[RESP_DATA], RESP_DATA);
+
+ memmove(&dell_bl->resp[RESP_DATA], &dell_bl->resp[RESP_DATA + 1],
+ dell_bl->resp_len - RESP_DATA - 1);
+ dell_bl->resp_idx = dell_bl->resp_len - 1;
+ continue; /* re-fetch the last (checksum) byte and re-validate */
+ }
- csum = dell_uart_checksum(dell_bl->resp, dell_bl->resp_len - 1);
- if (dell_bl->resp[dell_bl->resp_len - 1] == csum) {
- dell_bl->status = 0; /* Success */
- } else {
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;
}
wakeup:
wake_up(&dell_bl->wait_queue);
--
2.43.0
^ permalink raw reply related [flat|nested] 2+ messages in thread* Re: [PATCH] platform/x86: dell-uart-backlight: recover from stray EC response byte
2026-10-04 11:03 [PATCH] platform/x86: dell-uart-backlight: recover from stray EC response byte chitranshmandhaniya
@ 2026-10-14 11:50 ` Ilpo Järvinen
0 siblings, 0 replies; 2+ messages in thread
From: Ilpo Järvinen @ 2026-10-14 11:50 UTC (permalink / raw)
To: chitranshmandhaniya
Cc: Hans de Goede, platform-driver-x86, LKML, Chitransh Mandhaniya
On Sun, 4 Oct 2026, chitranshmandhaniya@gmail.com wrote:
> From: Chitransh Mandhaniya <Chitransh_Mandhaniya@dell.com>
>
> On some Dell AIO machines the EC occasionally inserts a spurious byte
> after the cmd-echo in UART backlight response frames. A get-brightness
> reply "04 0c 0a e5" arrives as "04 0c 0d 0a e5". The length field still
> reflects the original frame size, so the parser consumes the stray byte
> as payload and mistakes the real payload for the checksum, returning
> -EIO.
>
> Handle this in dell_uart_bl_receive(): on the first checksum failure for
> a frame longer than the minimum size, drop the byte at the first payload
> offset, shift the rest down by one, read one more byte as the new
> checksum, and re-validate. Limit this single-byte resync to once per
> command via a resync_used flag cleared at the start of each transaction.
> A second mismatch is still a hard error.
>
> Well-formed replies pass on the first check and never enter the recovery
> path.
>
> Signed-off-by: Chitransh Mandhaniya <Chitransh_Mandhaniya@dell.com>
> ---
> .../platform/x86/dell/dell-uart-backlight.c | 92 +++++++++++++------
> 1 file changed, 62 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/platform/x86/dell/dell-uart-backlight.c b/drivers/platform/x86/dell/dell-uart-backlight.c
> index f323a667dc2d..068b91c21bf4 100644
> --- a/drivers/platform/x86/dell/dell-uart-backlight.c
> +++ b/drivers/platform/x86/dell/dell-uart-backlight.c
> @@ -51,7 +51,7 @@
> #define MAX_RESP_LEN 80
>
> struct dell_uart_backlight {
> - struct mutex mutex;
> + struct mutex mutex; /* Protects command + response state below */
> wait_queue_head_t wait_queue;
> struct device *dev;
> struct backlight_device *bl;
> @@ -60,6 +60,7 @@ struct dell_uart_backlight {
> u8 resp_len;
> u8 resp_max_len;
> u8 pending_cmd;
> + bool resync_used;
> int status;
> int power;
> };
> @@ -91,6 +92,7 @@ static int dell_uart_bl_command(struct dell_uart_backlight *dell_bl,
> dell_bl->resp_len = -1; /* Invalid / unset */
> dell_bl->resp_max_len = resp_max_len;
> dell_bl->pending_cmd = cmd[1];
> + dell_bl->resync_used = false;
>
> /* 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);
> @@ -208,8 +210,10 @@ static const struct backlight_ops dell_uart_backlight_ops = {
> 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);
> + const int max_passes = 2;
> size_t i;
> u8 csum;
> + int pass;
>
> dev_dbg(dell_bl->dev, "Recv: %*ph\n", (int)len, data);
>
> @@ -220,42 +224,70 @@ static size_t dell_uart_bl_receive(struct serdev_device *serdev, const u8 *data,
> }
>
> i = 0;
> - while (i < len && dell_bl->resp_idx != dell_bl->resp_len) {
> - 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 ||
> - dell_bl->resp_len > dell_bl->resp_max_len) {
> - dev_err(dell_bl->dev, "Response length %d out if range %d - %d\n",
> - dell_bl->resp_len, MIN_RESP_LEN, dell_bl->resp_max_len);
> - dell_bl->status = -EIO;
> - goto wakeup;
> + for (pass = 0; pass < max_passes; pass++) {
> + while (i < len && dell_bl->resp_idx != dell_bl->resp_len) {
Hi,
Could the inner part be moved into a function so you wouldn't need to add
the pass based approach but could just call the function again? I think
that would be much easier to follow than adding a loop for retry.
If you end up doing that, please make the function refactoring in a
separate change before adding the extra byte handling.
--
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 ||
> + dell_bl->resp_len > dell_bl->resp_max_len) {
> + dev_err(dell_bl->dev,
> + "Response length %d out if range %d - %d\n",
> + dell_bl->resp_len, MIN_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;
> }
> - 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++;
> }
> - dell_bl->resp_idx++;
> - }
>
> - if (dell_bl->resp_idx != dell_bl->resp_len)
> - return len; /* Response not complete yet */
> + if (dell_bl->resp_idx != dell_bl->resp_len)
> + return i; /* Response not complete yet, wait for more data */
> +
> + csum = dell_uart_checksum(dell_bl->resp, dell_bl->resp_len - 1);
> + if (dell_bl->resp[dell_bl->resp_len - 1] == csum) {
> + dell_bl->status = 0; /* Success */
> + dev_dbg(dell_bl->dev,
> + "Response OK for cmd 0x%02x, len %d: %*ph\n",
> + dell_bl->pending_cmd, dell_bl->resp_len,
> + dell_bl->resp_len, dell_bl->resp);
> + goto wakeup;
> + }
> +
> + /*
> + * Some EC firmware inserts a stray byte right after the cmd-echo
> + * byte. Try once to drop it and re-fetch the checksum byte from the wire.
> + */
> + if (!dell_bl->resync_used && dell_bl->resp_len > MIN_RESP_LEN) {
> + dell_bl->resync_used = true;
> +
> + dev_warn(dell_bl->dev,
> + "Checksum mismatch got 0x%02x expected 0x%02x, dropping stray byte 0x%02x at offset %d\n",
> + dell_bl->resp[dell_bl->resp_len - 1], csum,
> + dell_bl->resp[RESP_DATA], RESP_DATA);
> +
> + memmove(&dell_bl->resp[RESP_DATA], &dell_bl->resp[RESP_DATA + 1],
> + dell_bl->resp_len - RESP_DATA - 1);
> + dell_bl->resp_idx = dell_bl->resp_len - 1;
> + continue; /* re-fetch the last (checksum) byte and re-validate */
> + }
>
> - csum = dell_uart_checksum(dell_bl->resp, dell_bl->resp_len - 1);
> - if (dell_bl->resp[dell_bl->resp_len - 1] == csum) {
> - dell_bl->status = 0; /* Success */
> - } else {
> 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;
> }
> wakeup:
> wake_up(&dell_bl->wait_queue);
>
--
i.
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2026-10-14 11:50 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-10-04 11:03 [PATCH] platform/x86: dell-uart-backlight: recover from stray EC response byte chitranshmandhaniya
2026-10-14 11:50 ` Ilpo Järvinen
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.