Re: [PATCH v2] platform/chrome: sensorhub: Fix dropped timestamp events and log spam

On Wed, Jul 15, 2026 at 6:28 PM Tzung-Bi Shih <[email protected]> wrote:
>
> On Wed, Jul 15, 2026 at 04:15:22PM +0900, Tomasz Figa wrote:
> > On Wed, Jul 15, 2026 at 11:45 AM Tzung-Bi Shih <[email protected]> wrote:
> > >
> > > Commit 833740a2333c ("platform/chrome: sensorhub: Bound the EC-reported
> > > sensor number") evaluated the `sensor_num` against the bounds limit even
> > > for timestamp events.  A timestamp event typically has a `sensor_num` of
> > > 0xff [1], causing the driver to flag it as invalid and skip to the next
> > > event.
> > >
> > > As a result, we'd see a flooding of "Invalid sensor number 255 from EC"
> > > warning logs and these timestamp events were being dropped.
> > >
> > > Move the bounds-check into cros_ec_sensor_ring_process_event() and
> > > evaluate it only after standalone timestamp events have already been
> > > processed and returned early.
> > >
> > > [1] https://crrev.com/219ca6ef82ba266da788b673ee4ad50bd3ea1285/common/motion_sense_fifo.c#427
> > >
> > > Fixes: 833740a2333c ("platform/chrome: sensorhub: Bound the EC-reported sensor number")
> > > Signed-off-by: Tzung-Bi Shih <[email protected]>
> > > ---
> > > Bryam: sorry for reverting the decision (moving back the check to
> > > cros_ec_sensor_ring_process_event()).  I realized we need to prevent
> > > dropping timestamp events and flooding the logs.
> > > ---
> > > v2:
> > > - Move the check to cros_ec_sensor_ring_process_event().
> > > - Return earlier for standalone timestamp events.
> > >
> > > v1: https://lore.kernel.org/all/[email protected]
> > > ---
> > >  .../platform/chrome/cros_ec_sensorhub_ring.c  | 27 ++++++++++---------
> > >  1 file changed, 15 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/drivers/platform/chrome/cros_ec_sensorhub_ring.c b/drivers/platform/chrome/cros_ec_sensorhub_ring.c
> > > index 92941924c347..d92b60213720 100644
> > > --- a/drivers/platform/chrome/cros_ec_sensorhub_ring.c
> > > +++ b/drivers/platform/chrome/cros_ec_sensorhub_ring.c
> > > @@ -475,6 +475,21 @@ cros_ec_sensor_ring_process_event(struct cros_ec_sensorhub *sensorhub,
> > >                                                   fifo_timestamp,
> > >                                                   *current_timestamp,
> > >                                                   now);
> > > +
> > > +               /*
> > > +                * A standalone timestamp event typically has a sensor_num of
> > > +                * 0xff.  Return early here to prevent it from hitting the
> > > +                * bounds check below and spamming the logs.
> > > +                */
> > > +               return false;
> > > +       }
> > > +
> > > +       /* Skip event if sensor_num from EC is out of bounds. */
> > > +       if (in->sensor_num >= sensorhub->sensor_num) {
> > > +               dev_warn_ratelimited(sensorhub->dev,
> > > +                                    "Invalid sensor number %u from EC\n",
> > > +                                    in->sensor_num);
> > > +               return false;
> > >         }
> > >
> > >         if (in->flags & MOTIONSENSE_SENSOR_FLAG_ODR) {
> > > @@ -502,10 +517,6 @@ cros_ec_sensor_ring_process_event(struct cros_ec_sensorhub *sensorhub,
> > >                 return true;
> > >         }
> > >
> > > -       if (in->flags & MOTIONSENSE_SENSOR_FLAG_TIMESTAMP)
> > > -               /* If we just have a timestamp, skip this entry. */
> > > -               return false;
> > > -
> >
> > Isn't this still needed for the case of
> > MOTIONSENSE_SENSOR_FLAG_TIMESTAMP and (MOTIONSENSE_SENSOR_FLAG_ODR
> > and/or MOTIONSENSE_SENSOR_FLAG_FLUSH)?
>
> The block serves for standalone MOTIONSENSE_SENSOR_FLAG_TIMESTAMP events.
> If the flags contains ODR or FLUSH, it returns in other branches.
>
> After applying the patch, the flow of cros_ec_sensor_ring_process_event()
> looks like:
>
>   If TIMESTAMP but (!ODR and !FLUSH):
>      ...
>      return   <--- The patch merges the block and returns early.
>
>   Check bounds for sensor_num
>
>   If ODR:
>      ...
>      return
>
>   If FLUSH:
>      ...
>      return

Ah, right, I didn't notice that those blocks already returned. Sorry
for the noise.

Reviewed-by: Tomasz Figa <[email protected]>

Best,
Tomasz