Re: [PATCH 2/2] chrome: lightbar: Add support for large sequence

[Tzung-Bi Shih <tzungbi@kernel.org>]

On Sat, Jan 24, 2026 at 12:59:14AM -0800, Gwendal Grignou wrote:
> Current sequences are limited to 192 bytes. Increase support to whatever
> the EC support. If the sequence is too long, the EC will return an
> OVERFLOW error.
> 
> Test: Check sending a large sequence is received by the EC.

Please use "platform/chrome" prefix for the commit title.

> +/*
> + * Lightbar version
> + */
> +static int lb_version = -1;
> +

Does it really need to initialize to -1?

>  static ssize_t program_store(struct device *dev, struct device_attribute *attr,
>  			     const char *buf, size_t count)
>  {
> -	int extra_bytes, max_size, ret;
> +	struct cros_ec_dev *ec = to_cros_ec_dev(dev);
>  	struct ec_params_lightbar *param;
>  	struct cros_ec_command *msg;
> -	struct cros_ec_dev *ec = to_cros_ec_dev(dev);
> +	size_t extra_bytes, max_size;
> +	int ret;

`ret` and `ec` are reordered.  They look irrelevant to the patch.

> @@ -478,14 +484,22 @@ static ssize_t program_store(struct device *dev, struct device_attribute *attr,
>  	 * and send a program that is too big for the protocol. In order
>  	 * to ensure the latter, we also need to ensure we have extra bytes
>  	 * to represent the rest of the packet.
> +	 * With V3, larger program can be sent, limited only by the EC.
> +	 * Only the protocol limit the payload size.
>  	 */
> -	extra_bytes = sizeof(*param) - sizeof(param->set_program.data);
> -	max_size = min(EC_LB_PROG_LEN, ec->ec_dev->max_request - extra_bytes);
> -	if (count > max_size) {
> -		dev_err(dev, "Program is %u bytes, too long to send (max: %u)",
> -			(unsigned int)count, max_size);
> -
> -		return -EINVAL;
> +	if (lb_version < 3) {
> +		extra_bytes = sizeof(*param) - sizeof(param->set_program.data);
> +		max_size = min(EC_LB_PROG_LEN, ec->ec_dev->max_request - extra_bytes);
> +		if (count > max_size) {
> +			dev_err(dev, "Program is %zu bytes, too long to send (max: %zu)",
> +					count, max_size);
> +
> +			return -EINVAL;
> +		}
> +	} else {
> +		extra_bytes = sizeof(*param) - sizeof(param->set_program) +
> +			sizeof(param->set_program_ex);

I don't understand the code.  What does `extra_bytes` want to count here?
Why `sizeof(param->set_program)` is involved?  I thought it should be:

  extra_bytes = sizeof(*param) - sizeof(param->set_program_ex.data);

I.e., (regardless of `lb_version`)

  extra_bytes = sizeof(*param) - EC_LB_PROG_LEN;

> +		max_size = ec->ec_dev->max_request - extra_bytes;

Is it possible that `max_size` > EC_LB_PROG_LEN?

> +	} else {
> +		size_t offset = 0;
> +		size_t payload = 0;
> +
> +		param->cmd = LIGHTBAR_CMD_SET_PROGRAM_EX;
> +		while (offset < count) {
> +			payload = min(max_size, count - offset);
> +			param->set_program_ex.offset = offset;
> +			param->set_program_ex.size = payload;
> +			memcpy(param->set_program_ex.data, &buf[offset], payload);

Follow up question, if `max_size` is possible greater than EC_LB_PROG_LEN,
it looks like the memcpy can write out of the boundary of struct
lightbar_program_ex.

> diff --git a/include/linux/platform_data/cros_ec_commands.h b/include/linux/platform_data/cros_ec_commands.h
> index 9cbf024f56c3..5a2fd6acdbb9 100644
> --- a/include/linux/platform_data/cros_ec_commands.h
> +++ b/include/linux/platform_data/cros_ec_commands.h
> @@ -2020,6 +2020,16 @@ struct lightbar_program {
>  	uint8_t data[EC_LB_PROG_LEN];
>  } __ec_todo_unpacked;
>  
> +/*
> + * Lightbar program for large sequences. Sequences are sent in pieces, with increasing offset.
> + * The sequences ar still limited by the amunt reserved in EC RAM.
                      ^ e                   ^^^^^ amount
> + */
> +struct lightbar_program_ex {
> +	uint16_t offset;
> +	uint8_t size;
> +	uint8_t data[EC_LB_PROG_LEN];
> +} __ec_todo_unpacked;
> +
>  struct ec_params_lightbar {
>  	uint8_t cmd;		      /* Command (see enum lightbar_command) */
>  	union {
> @@ -2066,6 +2076,7 @@ struct ec_params_lightbar {
>  		struct lightbar_params_v2_colors set_v2par_colors;
>  
>  		struct lightbar_program set_program;
> +		struct lightbar_program_ex set_program_ex;
>  	};
>  } __ec_todo_packed;
>  
> @@ -2154,6 +2165,7 @@ enum lightbar_command {
>  	LIGHTBAR_CMD_GET_PARAMS_V2_COLORS = 32,
>  	LIGHTBAR_CMD_SET_PARAMS_V2_COLORS = 33,
>  	LIGHTBAR_CMD_GET_PARAMS_V3 = 34,
> +	LIGHTBAR_CMD_SET_PROGRAM_EX = 35,
>  	LIGHTBAR_NUM_CMDS
>  };

Haven't seen the change in
https://chromium.googlesource.com/chromiumos/platform/ec/+/refs/heads/main/include/ec_commands.h,
do we need to wait until the change landed in EC firmware?