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

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

[Gwendal Grignou]

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.

Signed-off-by: Gwendal Grignou <gwendal@google.com>
---
 drivers/platform/chrome/cros_ec_lightbar.c    | 80 +++++++++++++------
 .../linux/platform_data/cros_ec_commands.h    | 12 +++
 2 files changed, 68 insertions(+), 24 deletions(-)

diff --git a/drivers/platform/chrome/cros_ec_lightbar.c b/drivers/platform/chrome/cros_ec_lightbar.c
index 85f30c8dfba3..d8d66343e2e7 100644
--- a/drivers/platform/chrome/cros_ec_lightbar.c
+++ b/drivers/platform/chrome/cros_ec_lightbar.c
@@ -30,6 +30,11 @@ static unsigned long lb_interval_jiffies = 50 * HZ / 1000;
  */
 static bool userspace_control;
 
+/*
+ * Lightbar version
+ */
+static int lb_version = -1;
+
 static ssize_t interval_msec_show(struct device *dev,
 				  struct device_attribute *attr, char *buf)
 {
@@ -467,10 +472,11 @@ static ssize_t sequence_store(struct device *dev, struct device_attribute *attr,
 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;
 
 	/*
 	 * We might need to reject the program for size reasons. The EC
@@ -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);
+		max_size = ec->ec_dev->max_request - extra_bytes;
 	}
 
 	msg = alloc_lightbar_cmd_msg(ec);
@@ -495,26 +509,44 @@ static ssize_t program_store(struct device *dev, struct device_attribute *attr,
 	ret = lb_throttle();
 	if (ret)
 		goto exit;
+	param = (struct ec_params_lightbar *)msg->data;
 
-	dev_info(dev, "Copying %zu byte program to EC", count);
+	if (lb_version < 3) {
+		dev_info(dev, "Copying %zu byte program to EC", count);
 
-	param = (struct ec_params_lightbar *)msg->data;
-	param->cmd = LIGHTBAR_CMD_SET_PROGRAM;
+		param->cmd = LIGHTBAR_CMD_SET_PROGRAM;
 
-	param->set_program.size = count;
-	memcpy(param->set_program.data, buf, count);
+		param->set_program.size = count;
+		memcpy(param->set_program.data, buf, count);
 
-	/*
-	 * We need to set the message size manually or else it will use
-	 * EC_LB_PROG_LEN. This might be too long, and the program
-	 * is unlikely to use all of the space.
-	 */
-	msg->outsize = count + extra_bytes;
+		/*
+		 * We need to set the message size manually or else it will use
+		 * EC_LB_PROG_LEN. This might be too long, and the program
+		 * is unlikely to use all of the space.
+		 */
+		msg->outsize = count + extra_bytes;
 
-	ret = cros_ec_cmd_xfer_status(ec->ec_dev, msg);
-	if (ret < 0)
-		goto exit;
+		ret = cros_ec_cmd_xfer_status(ec->ec_dev, msg);
+		if (ret < 0)
+			goto exit;
+	} 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);
+			msg->outsize = payload + extra_bytes;
 
+			ret = cros_ec_cmd_xfer_status(ec->ec_dev, msg);
+			if (ret < 0)
+				goto exit;
+			offset += payload;
+		}
+	}
 	ret = count;
 exit:
 	kfree(msg);
@@ -592,7 +624,7 @@ static int cros_ec_lightbar_probe(struct platform_device *pd)
 	 * Ask then for the lightbar version, if it's 0 then the 'cros_ec'
 	 * doesn't have a lightbar.
 	 */
-	if (!get_lightbar_version(ec_dev, NULL, NULL))
+	if (!get_lightbar_version(ec_dev, &lb_version, NULL))
 		return -ENODEV;
 
 	/* Take control of the lightbar from the EC. */
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.
+ */
+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
 };
 
-- 
2.52.0.457.g6b5491de43-goog

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

[Tzung-Bi Shih]

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?

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

[Gwendal Grignou]

On Sun, Jan 25, 2026 at 11:14 PM Tzung-Bi Shih <tzungbi@kernel.org> wrote:
>
> 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?
Removed.
>
> >  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.
I reorder the local variable declaration to look like a "reverse xmas
tree". Removed.
>
> > @@ -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);
`set_program_ex.data` is a variable array of size 0.
`set_program.data` is a real array set to 192.
|*param| does contain the payload array for `set_program`, so we need
to remove it first to calculate our payload:

0 ----a ------- x --------------- 192+x -- maximal command size
|    sizeof(*param)                |     <<<< size of the whole command
|     | sizeof(set_program)        |     <<<< size of the largest subcommand
|     | .._ex   |
|  extra_bytes  | available for the payload |


>
> I.e., (regardless of `lb_version`)
>
>   extra_bytes = sizeof(*param) - EC_LB_PROG_LEN;
No, because `set_program_ex` contains an extra field `offset`.
>
> > +             max_size = ec->ec_dev->max_request - extra_bytes;
>
> Is it possible that `max_size` > EC_LB_PROG_LEN?
Yes. EC transfer on some protocol allows 256 bytes commands. It makes
sense to optimize for what the transport supports.
>
> > +     } 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.
It will always be outside of lightbar_program_ex since I use a variable array.
>
> > 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
Fixed in v2 (and crrev.com/c/7515877)
> > + */
> > +struct lightbar_program_ex {
> > +     uint16_t offset;
> > +     uint8_t size;
> > +     uint8_t data[EC_LB_PROG_LEN];
I see the problem: it should be [0] here. I tested with 0, I messed up
during "cleanup" when synchronising between firmware and kernel.
> > +} __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?
Will post v2 once the EC firmware change is approved.

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

[Tzung-Bi Shih]

On Mon, Jan 26, 2026 at 01:19:16AM -0800, Gwendal Grignou wrote:
> On Sun, Jan 25, 2026 at 11:14 PM Tzung-Bi Shih <tzungbi@kernel.org> wrote:
> > On Sat, Jan 24, 2026 at 12:59:14AM -0800, Gwendal Grignou wrote:
> > > @@ -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);
> `set_program_ex.data` is a variable array of size 0.
> `set_program.data` is a real array set to 192.
> |*param| does contain the payload array for `set_program`, so we need
> to remove it first to calculate our payload:
> 
> 0 ----a ------- x --------------- 192+x -- maximal command size
> |    sizeof(*param)                |     <<<< size of the whole command
> |     | sizeof(set_program)        |     <<<< size of the largest subcommand
> |     | .._ex   |
> |  extra_bytes  | available for the payload |

It looks fragile as it knows `set_program` is the largest member in the
union in advance.  How about something like:

  extra_bytes = offsetof(typeof(*param), set_program_ex) +
		sizeof(param->set_program_ex);

> > > diff --git a/include/linux/platform_data/cros_ec_commands.h b/include/linux/platform_data/cros_ec_commands.h
> > > +struct lightbar_program_ex {
> > > +     uint16_t offset;
> > > +     uint8_t size;
> > > +     uint8_t data[EC_LB_PROG_LEN];
> I see the problem: it should be [0] here. I tested with 0, I messed up
> during "cleanup" when synchronising between firmware and kernel.

Yeah, I was confused by it.

For the case, I think the content has to diverge from the firmware's header.

The flexible array member `data` should be annotated with __counted_by().

AFAIK, there is/was an on-going effort for avoiding
-Wflex-array-member-not-at-end warnings.  E.g., a26c23e0d679
("KEYS: Avoid -Wflex-array-member-not-at-end warning").  Please take a look
if it needs to leverage any flex helpers.