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

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