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

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

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.