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

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

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.