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

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

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.