[PATCH] platform/x86: dell-uart-backlight: recover from stray EC response byte

[PATCH] platform/x86: dell-uart-backlight: recover from stray EC response byte

[chitranshmandhaniya]

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

Re: [PATCH] platform/x86: dell-uart-backlight: recover from stray EC response byte

[Ilpo Järvinen]

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.

[PATCH v2 0/2] platform/x86: dell-uart-backlight: recover from stray EC response byte

[chitranshmandhaniya]

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, causing checksum
mismatches and -EIO.

Patch 1 extracts the response parsing into a dell_uart_bl_receive_data()
helper so it can be called again after resyncing.

Patch 2 adds the stray byte recovery: on checksum mismatch, drop the
byte at the first payload position, shift data down, and call the helper
again to re-fetch the checksum byte from the wire.

v1 -> v2:
  - Extract dell_uart_bl_receive_data() helper instead of using a
    for-loop with pass counter, call it again after resync (Ilpo)
  - Split into two patches: refactor + feature (Ilpo)

Chitransh Mandhaniya (2):
  platform/x86: dell-uart-backlight: extract dell_uart_bl_receive_data()
    helper
  platform/x86: dell-uart-backlight: recover from stray EC response byte

 .../platform/x86/dell/dell-uart-backlight.c   | 63 ++++++++++++++-----
 1 file changed, 48 insertions(+), 15 deletions(-)

-- 
2.43.0

[PATCH v2 1/2] platform/x86: dell-uart-backlight: extract dell_uart_bl_receive_data() helper

[chitranshmandhaniya]

From: Chitransh Mandhaniya <Chitransh_Mandhaniya@dell.com>

Move the response-byte processing and checksum validation out of
dell_uart_bl_receive() into a new dell_uart_bl_receive_data() helper.

No functional change. This is a preparatory refactor so that a
follow-up patch can call the helper again after resyncing on a stray
byte from the EC.

Signed-off-by: Chitransh Mandhaniya <Chitransh_Mandhaniya@dell.com>
---
 .../platform/x86/dell/dell-uart-backlight.c   | 42 ++++++++++++-------
 1 file changed, 27 insertions(+), 15 deletions(-)

diff --git a/drivers/platform/x86/dell/dell-uart-backlight.c b/drivers/platform/x86/dell/dell-uart-backlight.c
index f323a667dc2d..b61499d7b3f8 100644
--- a/drivers/platform/x86/dell/dell-uart-backlight.c
+++ b/drivers/platform/x86/dell/dell-uart-backlight.c
@@ -205,20 +205,13 @@ static const struct backlight_ops dell_uart_backlight_ops = {
 	.get_brightness = dell_uart_get_brightness_op,
 };
 
-static size_t dell_uart_bl_receive(struct serdev_device *serdev, const u8 *data, size_t len)
+/* Process received UART data; returns bytes consumed or -1 if incomplete. */
+static int dell_uart_bl_receive_data(struct dell_uart_backlight *dell_bl,
+				     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;
-	}
-
 	i = 0;
 	while (i < len && dell_bl->resp_idx != dell_bl->resp_len) {
 		dell_bl->resp[dell_bl->resp_idx] = data[i++];
@@ -231,7 +224,7 @@ static size_t dell_uart_bl_receive(struct serdev_device *serdev, const u8 *data,
 				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;
+				return i;
 			}
 			break;
 		case RESP_CMD: /* CMD byte */
@@ -239,7 +232,7 @@ static size_t dell_uart_bl_receive(struct serdev_device *serdev, const u8 *data,
 				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;
+				return i;
 			}
 			break;
 		}
@@ -247,7 +240,7 @@ static size_t dell_uart_bl_receive(struct serdev_device *serdev, const u8 *data,
 	}
 
 	if (dell_bl->resp_idx != dell_bl->resp_len)
-		return len; /* Response not complete yet */
+		return -1; /* Response not complete yet */
 
 	csum = dell_uart_checksum(dell_bl->resp, dell_bl->resp_len - 1);
 	if (dell_bl->resp[dell_bl->resp_len - 1] == csum) {
@@ -257,11 +250,30 @@ static size_t dell_uart_bl_receive(struct serdev_device *serdev, const u8 *data,
 			dell_bl->resp[dell_bl->resp_len - 1], csum);
 		dell_bl->status = -EIO;
 	}
-wakeup:
-	wake_up(&dell_bl->wait_queue);
 	return i;
 }
 
+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);
+	int ret;
+
+	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;
+	}
+
+	ret = dell_uart_bl_receive_data(dell_bl, data, len);
+	if (ret < 0)
+		return len; /* Response not complete yet */
+
+	wake_up(&dell_bl->wait_queue);
+	return ret;
+}
+
 static const struct serdev_device_ops dell_uart_bl_serdev_ops = {
 	.receive_buf = dell_uart_bl_receive,
 	.write_wakeup = serdev_device_write_wakeup,
-- 
2.43.0

[PATCH v2 2/2] platform/x86: dell-uart-backlight: recover from stray EC response byte

[chitranshmandhaniya]

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.

On the first checksum failure for a frame longer than the minimum size,
drop the byte at the first payload offset, shift the remaining data down
by one, and call dell_uart_bl_receive_data() again to re-fetch the
checksum byte from the wire 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.

Tested on Dell OptiPlex AIO 7410 65W (0TKWC4) where the EC inserts a
stray byte on every get-brightness response -- resync fixes it
transparently.

Signed-off-by: Chitransh Mandhaniya <Chitransh_Mandhaniya@dell.com>
---
 .../platform/x86/dell/dell-uart-backlight.c   | 23 ++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/x86/dell/dell-uart-backlight.c b/drivers/platform/x86/dell/dell-uart-backlight.c
index b61499d7b3f8..085dcc055554 100644
--- a/drivers/platform/x86/dell/dell-uart-backlight.c
+++ b/drivers/platform/x86/dell/dell-uart-backlight.c
@@ -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);
@@ -270,8 +272,27 @@ static size_t dell_uart_bl_receive(struct serdev_device *serdev, const u8 *data,
 	if (ret < 0)
 		return len; /* Response not complete yet */
 
+	/* Stray byte after cmd-echo? Drop it, shift data down, re-fetch checksum. */
+	if (dell_bl->status == -EIO &&
+	    !dell_bl->resync_used && dell_bl->resp_len > MIN_RESP_LEN) {
+		dell_bl->resync_used = true;
+		dell_bl->status = -EBUSY;
+
+		dev_dbg(dell_bl->dev, "Resync: dropping stray byte 0x%02x at offset %d\n",
+			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;
+
+		/* Re-fetch checksum byte from remaining data */
+		ret = dell_uart_bl_receive_data(dell_bl, data + ret, len - ret);
+		if (ret < 0)
+			return len; /* Need more data for checksum byte */
+	}
+
 	wake_up(&dell_bl->wait_queue);
-	return ret;
+	return len;
 }
 
 static const struct serdev_device_ops dell_uart_bl_serdev_ops = {
-- 
2.43.0

Re: [PATCH v2 2/2] platform/x86: dell-uart-backlight: recover from stray EC response byte

[Ilpo Järvinen]

On Thu, 15 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.
> 
> On the first checksum failure for a frame longer than the minimum size,
> drop the byte at the first payload offset, shift the remaining data down
> by one, and call dell_uart_bl_receive_data() again to re-fetch the
> checksum byte from the wire 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.
> 
> Tested on Dell OptiPlex AIO 7410 65W (0TKWC4) where the EC inserts a
> stray byte on every get-brightness response -- resync fixes it
> transparently.
> 
> Signed-off-by: Chitransh Mandhaniya <Chitransh_Mandhaniya@dell.com>
> ---
>  .../platform/x86/dell/dell-uart-backlight.c   | 23 ++++++++++++++++++-
>  1 file changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/dell/dell-uart-backlight.c b/drivers/platform/x86/dell/dell-uart-backlight.c
> index b61499d7b3f8..085dcc055554 100644
> --- a/drivers/platform/x86/dell/dell-uart-backlight.c
> +++ b/drivers/platform/x86/dell/dell-uart-backlight.c
> @@ -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);
> @@ -270,8 +272,27 @@ static size_t dell_uart_bl_receive(struct serdev_device *serdev, const u8 *data,
>  	if (ret < 0)
>  		return len; /* Response not complete yet */
>  
> +	/* Stray byte after cmd-echo? Drop it, shift data down, re-fetch checksum. */
> +	if (dell_bl->status == -EIO &&
> +	    !dell_bl->resync_used && dell_bl->resp_len > MIN_RESP_LEN) {
> +		dell_bl->resync_used = true;
> +		dell_bl->status = -EBUSY;
> +
> +		dev_dbg(dell_bl->dev, "Resync: dropping stray byte 0x%02x at offset %d\n",
> +			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;
> +
> +		/* Re-fetch checksum byte from remaining data */
> +		ret = dell_uart_bl_receive_data(dell_bl, data + ret, len - ret);
> +		if (ret < 0)
> +			return len; /* Need more data for checksum byte */
> +	}
> +
>  	wake_up(&dell_bl->wait_queue);
> -	return ret;
> +	return len;

Thanks. Structuring it this way makes it much easier to follow than the 
loop.

What I'm left a bit unsure is how this last return behaves, and why there 
is the change from ret -> len in this second patch which should be only 
about adding the extra byte trickery.

If ret is always expected to be equal to len before this patch, it would 
make more sense to make that change in patch 1?

(I admit this entire code looks a bit more convoluted than what I'd 
expect to be needed, so I'm not sure if I can follow all this fully.)



Unrelated to this change/series, there seems to be mutex_init() call in 
dell_uart_bl_serdev_probe() that should be changed to devm_mutex_init() + 
error handling so one doesn't need to have to make mutex_destroy() call
manually (it is currently missing).

-- 
 i.

Re: [PATCH v2 2/2] platform/x86: dell-uart-backlight: recover from stray EC response byte

[Ilpo Järvinen]

On Thu, 15 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.
> 
> On the first checksum failure for a frame longer than the minimum size,
> drop the byte at the first payload offset, shift the remaining data down
> by one, and call dell_uart_bl_receive_data() again to re-fetch the
> checksum byte from the wire 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.
> 
> Tested on Dell OptiPlex AIO 7410 65W (0TKWC4) where the EC inserts a
> stray byte on every get-brightness response -- resync fixes it
> transparently.
> 
> Signed-off-by: Chitransh Mandhaniya <Chitransh_Mandhaniya@dell.com>
> ---
>  .../platform/x86/dell/dell-uart-backlight.c   | 23 ++++++++++++++++++-
>  1 file changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/dell/dell-uart-backlight.c b/drivers/platform/x86/dell/dell-uart-backlight.c
> index b61499d7b3f8..085dcc055554 100644
> --- a/drivers/platform/x86/dell/dell-uart-backlight.c
> +++ b/drivers/platform/x86/dell/dell-uart-backlight.c
> @@ -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);
> @@ -270,8 +272,27 @@ static size_t dell_uart_bl_receive(struct serdev_device *serdev, const u8 *data,
>  	if (ret < 0)
>  		return len; /* Response not complete yet */
>  
> +	/* Stray byte after cmd-echo? Drop it, shift data down, re-fetch checksum. */
> +	if (dell_bl->status == -EIO &&
> +	    !dell_bl->resync_used && dell_bl->resp_len > MIN_RESP_LEN) {
> +		dell_bl->resync_used = true;
> +		dell_bl->status = -EBUSY;
> +
> +		dev_dbg(dell_bl->dev, "Resync: dropping stray byte 0x%02x at offset %d\n",
> +			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);

sashiko.dev (an AI review tool so be reasonably critical to its findings) 
mentions that if ->resp_len check fails due to len being too large, 
->status is -EIO and this may end up moving more than there's room in the 
buffer.

> +		dell_bl->resp_idx = dell_bl->resp_len - 1;
> +
> +		/* Re-fetch checksum byte from remaining data */
> +		ret = dell_uart_bl_receive_data(dell_bl, data + ret, len - ret);
> +		if (ret < 0)
> +			return len; /* Need more data for checksum byte */
> +	}
> +
>  	wake_up(&dell_bl->wait_queue);
> -	return ret;
> +	return len;
>  }
>  
>  static const struct serdev_device_ops dell_uart_bl_serdev_ops = {
> 

-- 
 i.

[PATCH v3 0/2] platform/x86: dell-uart-backlight: stray byte recovery and cleanup

[chitranshmandhaniya]

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, causing checksum
mismatches and -EIO.

Patch 1 extracts the response parsing into a dell_uart_bl_receive_data()
helper that always returns bytes consumed. The caller checks
dell_bl->status to determine the outcome (-EBUSY = incomplete, 0 =
success, -EIO = error).

Patch 2 adds the stray byte recovery: on checksum mismatch for a fully
received frame, drop the byte at the first payload position, shift data
down, and call the helper again to re-fetch the checksum byte. Total
consumed bytes are tracked and returned to serdev exactly.  Also
switches mutex_init() to devm_mutex_init() to fix a missing
mutex_destroy() on the teardown path (noted by Ilpo during v2 review).

v2 -> v3:
  - Helper always returns bytes consumed, caller checks
    dell_bl->status instead (Ilpo)
  - Track total consumed bytes with a separate variable instead of
    returning len, so serdev gets the exact byte count (Ilpo)
  - Guard resync with resp_idx == resp_len to prevent memmove on
    truncated/out-of-range length field (sashiko.dev via Ilpo)
  - Switch mutex_init() to devm_mutex_init() in patch 2 (Ilpo)

v1 -> v2:
  - Extract dell_uart_bl_receive_data() helper instead of using a
    for-loop with pass counter, call it again after resync (Ilpo)
  - Split into two patches: refactor + feature (Ilpo)

Chitransh Mandhaniya (2):
  platform/x86: dell-uart-backlight: extract dell_uart_bl_receive_data()
    helper
  platform/x86: dell-uart-backlight: recover from stray EC response byte

 .../platform/x86/dell/dell-uart-backlight.c   | 71 ++++++++++++++-----
 1 file changed, 55 insertions(+), 16 deletions(-)

-- 
2.43.0

[PATCH v3 1/2] platform/x86: dell-uart-backlight: extract

[chitranshmandhaniya]

From: Chitransh Mandhaniya <Chitransh_Mandhaniya@dell.com>

Move the response-byte processing and checksum validation out of
dell_uart_bl_receive() into a new dell_uart_bl_receive_data() helper.

The helper always returns the number of bytes consumed from the input
buffer. The caller checks dell_bl->status to determine the outcome:
-EBUSY means the response is still incomplete, 0 means success, -EIO
means an error occurred. This preserves the original behaviour.

No functional change. This is a preparatory refactor so that a
follow-up patch can call the helper again after resyncing on a stray
byte from the EC.

Signed-off-by: Chitransh Mandhaniya <Chitransh_Mandhaniya@dell.com>
---
 .../platform/x86/dell/dell-uart-backlight.c   | 42 ++++++++++++-------
 1 file changed, 27 insertions(+), 15 deletions(-)

diff --git a/drivers/platform/x86/dell/dell-uart-backlight.c b/drivers/platform/x86/dell/dell-uart-backlight.c
index f323a667dc2d..af9c5f9a566f 100644
--- a/drivers/platform/x86/dell/dell-uart-backlight.c
+++ b/drivers/platform/x86/dell/dell-uart-backlight.c
@@ -205,20 +205,13 @@ static const struct backlight_ops dell_uart_backlight_ops = {
 	.get_brightness = dell_uart_get_brightness_op,
 };
 
-static size_t dell_uart_bl_receive(struct serdev_device *serdev, const u8 *data, size_t len)
+/* Process received UART data; returns number of bytes consumed. */
+static size_t dell_uart_bl_receive_data(struct dell_uart_backlight *dell_bl,
+					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;
-	}
-
 	i = 0;
 	while (i < len && dell_bl->resp_idx != dell_bl->resp_len) {
 		dell_bl->resp[dell_bl->resp_idx] = data[i++];
@@ -231,7 +224,7 @@ static size_t dell_uart_bl_receive(struct serdev_device *serdev, const u8 *data,
 				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;
+				return i;
 			}
 			break;
 		case RESP_CMD: /* CMD byte */
@@ -239,7 +232,7 @@ static size_t dell_uart_bl_receive(struct serdev_device *serdev, const u8 *data,
 				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;
+				return i;
 			}
 			break;
 		}
@@ -247,7 +240,7 @@ static size_t dell_uart_bl_receive(struct serdev_device *serdev, const u8 *data,
 	}
 
 	if (dell_bl->resp_idx != dell_bl->resp_len)
-		return len; /* Response not complete yet */
+		return i; /* Response not complete, status stays -EBUSY */
 
 	csum = dell_uart_checksum(dell_bl->resp, dell_bl->resp_len - 1);
 	if (dell_bl->resp[dell_bl->resp_len - 1] == csum) {
@@ -257,11 +250,30 @@ static size_t dell_uart_bl_receive(struct serdev_device *serdev, const u8 *data,
 			dell_bl->resp[dell_bl->resp_len - 1], csum);
 		dell_bl->status = -EIO;
 	}
-wakeup:
-	wake_up(&dell_bl->wait_queue);
 	return i;
 }
 
+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 ret;
+
+	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;
+	}
+
+	ret = dell_uart_bl_receive_data(dell_bl, data, len);
+	if (dell_bl->status == -EBUSY)
+		return len; /* Response not complete yet */
+
+	wake_up(&dell_bl->wait_queue);
+	return ret;
+}
+
 static const struct serdev_device_ops dell_uart_bl_serdev_ops = {
 	.receive_buf = dell_uart_bl_receive,
 	.write_wakeup = serdev_device_write_wakeup,
-- 
2.43.0

[PATCH v3 2/2] platform/x86: dell-uart-backlight: recover from stray

[chitranshmandhaniya]

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.

On the first checksum failure for a fully received frame, drop the byte
at the first payload offset, shift the remaining data down by one, and
call dell_uart_bl_receive_data() again to re-fetch the checksum byte
from the wire. Track total consumed bytes so the exact count is
returned to serdev.

The resync is guarded by resp_idx == resp_len to ensure a complete frame
was received, preventing memmove on a truncated or out-of-range length
field.

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 never enter the recovery path.

While at it, switch mutex_init() to devm_mutex_init() so the mutex is
automatically destroyed via devres, fixing a missing mutex_destroy()
on the teardown path.

Tested on Dell OptiPlex AIO 7410 65W (0TKWC4) where the EC inserts a
stray byte on every get-brightness response -- resync fixes it
transparently.

Suggested-by: ilpo.jarvinen@linux.intel.com
Signed-off-by: Chitransh Mandhaniya <Chitransh_Mandhaniya@dell.com>
---
 .../platform/x86/dell/dell-uart-backlight.c   | 35 ++++++++++++++++---
 1 file changed, 31 insertions(+), 4 deletions(-)

diff --git a/drivers/platform/x86/dell/dell-uart-backlight.c b/drivers/platform/x86/dell/dell-uart-backlight.c
index af9c5f9a566f..b0df479a4658 100644
--- a/drivers/platform/x86/dell/dell-uart-backlight.c
+++ b/drivers/platform/x86/dell/dell-uart-backlight.c
@@ -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);
@@ -256,7 +258,7 @@ static size_t dell_uart_bl_receive_data(struct dell_uart_backlight *dell_bl,
 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 ret;
+	size_t consumed;
 
 	dev_dbg(dell_bl->dev, "Recv: %*ph\n", (int)len, data);
 
@@ -266,12 +268,35 @@ static size_t dell_uart_bl_receive(struct serdev_device *serdev, const u8 *data,
 		return len;
 	}
 
-	ret = dell_uart_bl_receive_data(dell_bl, data, len);
+	consumed = dell_uart_bl_receive_data(dell_bl, data, len);
 	if (dell_bl->status == -EBUSY)
 		return len; /* Response not complete yet */
 
+	/*
+	 * Stray byte after cmd-echo? Drop it, shift data down, re-fetch
+	 * checksum. Only attempt when a full frame was received (resp_idx ==
+	 * resp_len) to avoid acting on a truncated length field.
+	 */
+	if (dell_bl->status == -EIO && !dell_bl->resync_used &&
+	    dell_bl->resp_idx == dell_bl->resp_len &&
+	    dell_bl->resp_len > MIN_RESP_LEN) {
+		dell_bl->resync_used = true;
+		dell_bl->status = -EBUSY;
+
+		dev_dbg(dell_bl->dev, "Resync: dropping stray byte 0x%02x at offset %d\n",
+			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;
+
+		consumed += dell_uart_bl_receive_data(dell_bl, data + consumed, len - consumed);
+		if (dell_bl->status == -EBUSY)
+			return len; /* Need more data for checksum byte */
+	}
+
 	wake_up(&dell_bl->wait_queue);
-	return ret;
+	return consumed;
 }
 
 static const struct serdev_device_ops dell_uart_bl_serdev_ops = {
@@ -291,7 +316,9 @@ static int dell_uart_bl_serdev_probe(struct serdev_device *serdev)
 	if (!dell_bl)
 		return -ENOMEM;
 
-	mutex_init(&dell_bl->mutex);
+	ret = devm_mutex_init(dev, &dell_bl->mutex);
+	if (ret)
+		return ret;
 	init_waitqueue_head(&dell_bl->wait_queue);
 	dell_bl->dev = dev;
 
-- 
2.43.0

Re: [PATCH v3 2/2] platform/x86: dell-uart-backlight: recover from stray

[Ilpo Järvinen]

On Fri, 16 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.
> 
> On the first checksum failure for a fully received frame, drop the byte
> at the first payload offset, shift the remaining data down by one, and
> call dell_uart_bl_receive_data() again to re-fetch the checksum byte
> from the wire. Track total consumed bytes so the exact count is
> returned to serdev.
> 
> The resync is guarded by resp_idx == resp_len to ensure a complete frame
> was received, preventing memmove on a truncated or out-of-range length
> field.
> 
> 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 never enter the recovery path.
> 
> While at it, switch mutex_init() to devm_mutex_init() so the mutex is
> automatically destroyed via devres, fixing a missing mutex_destroy()
> on the teardown path.
> 
> Tested on Dell OptiPlex AIO 7410 65W (0TKWC4) where the EC inserts a
> stray byte on every get-brightness response -- resync fixes it
> transparently.
> 
> Suggested-by: ilpo.jarvinen@linux.intel.com

This is not the correct formatting for a tag (please see 
Documentation/process/submitting-patches.rst). And my Suggested-by should 
only be in the devm_mutex_init() change (more on this below).

> Signed-off-by: Chitransh Mandhaniya <Chitransh_Mandhaniya@dell.com>
> ---
>  .../platform/x86/dell/dell-uart-backlight.c   | 35 ++++++++++++++++---
>  1 file changed, 31 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/platform/x86/dell/dell-uart-backlight.c b/drivers/platform/x86/dell/dell-uart-backlight.c
> index af9c5f9a566f..b0df479a4658 100644
> --- a/drivers/platform/x86/dell/dell-uart-backlight.c
> +++ b/drivers/platform/x86/dell/dell-uart-backlight.c
> @@ -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);
> @@ -256,7 +258,7 @@ static size_t dell_uart_bl_receive_data(struct dell_uart_backlight *dell_bl,
>  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 ret;
> +	size_t consumed;
>  
>  	dev_dbg(dell_bl->dev, "Recv: %*ph\n", (int)len, data);
>  
> @@ -266,12 +268,35 @@ static size_t dell_uart_bl_receive(struct serdev_device *serdev, const u8 *data,
>  		return len;
>  	}
>  
> -	ret = dell_uart_bl_receive_data(dell_bl, data, len);
> +	consumed = dell_uart_bl_receive_data(dell_bl, data, len);
>  	if (dell_bl->status == -EBUSY)
>  		return len; /* Response not complete yet */

So again I'm not following logic here. dell_uart_bl_receive_data() 
consumed all of len but response is still not complete, right? Why we 
return len and not consumed? I'd expect them to be same in that case but 
is there a case where they differ?

Subtle things like this may make the code harder to follow, because reader 
will have to ask this kind of questions and then attempt to figure out 
what case he/she doesn't understand.

> +	/*
> +	 * Stray byte after cmd-echo? Drop it, shift data down, re-fetch
> +	 * checksum. Only attempt when a full frame was received (resp_idx ==
> +	 * resp_len) to avoid acting on a truncated length field.
> +	 */
> +	if (dell_bl->status == -EIO && !dell_bl->resync_used &&
> +	    dell_bl->resp_idx == dell_bl->resp_len &&
> +	    dell_bl->resp_len > MIN_RESP_LEN) {
> +		dell_bl->resync_used = true;
> +		dell_bl->status = -EBUSY;
> +
> +		dev_dbg(dell_bl->dev, "Resync: dropping stray byte 0x%02x at offset %d\n",
> +			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;
> +
> +		consumed += dell_uart_bl_receive_data(dell_bl, data + consumed, len - consumed);
> +		if (dell_bl->status == -EBUSY)
> +			return len; /* Need more data for checksum byte */

I don't follow why consumed is what should be returned because we've eaten 
everything of len but still are short of bytes? So consumed should be same 
as len by my own logic but would be more logical to return (if they're 
equal).

Please don't just mindlessly change these to consumed, I could be wrong 
as I don't fully understand the related logic as it's somewhat messy so 
following all control flow paths is bit of a challenge (to put it mildly).

> +	}
> +
>  	wake_up(&dell_bl->wait_queue);
> -	return ret;
> +	return consumed;
>  }
>  
>  static const struct serdev_device_ops dell_uart_bl_serdev_ops = {
> @@ -291,7 +316,9 @@ static int dell_uart_bl_serdev_probe(struct serdev_device *serdev)
>  	if (!dell_bl)
>  		return -ENOMEM;
>  
> -	mutex_init(&dell_bl->mutex);
> +	ret = devm_mutex_init(dev, &dell_bl->mutex);
> +	if (ret)
> +		return ret;

Thanks but when I says something is "unrelated" to the patch/series. It 
means I've just noticed another issue while reviewing the patch/series,
such extra changes should be made in own patch(es).

We try to make all changes such that they contain only one logical change 
in its minimal form, and no more. In rare cases we need to compromise from 
that rule to not break anything but this is not such a case and could 
definitely be in own patch.

When you have to write in changelog an itemized list or say "while at 
it", "in addition", etc. it's often indicator that you're combining more 
than one logical change. Always when writing such phrase it's good to 
stop for a moment, and ask oneself if that thing could be safely moved to 
own patch instead. Very often the answer is yes.

-- 
 i.