Re: [PATCH] iio: cros: Add cros_ec_sensors_core_register

[Gwendal Grignou <gwendal@chromium.org>]

On Wed, Jun 22, 2022 at 5:44 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> The subject line could be a little clearer. Perhaps "Wait for sensors to
> probe before sending events" or something like that.
>
> Quoting Gwendal Grignou (2022-06-21 11:28:59)
> > Instead of registering callback to process sensor events right at
> > initialization time, wait for the sensor to be register in the iio
> > subsystem.
>
> Please elaborate. A crash is seen if sensor events come in while sensors
> are being registered.
The crash only happens to a sensor that is not upstream yet. (the
channels are allocated at probe time and we get a NULL dereference).
For existing sensors, the channels are allocated at compilation time,
so the kernel does not crash. We do send data to IIO core while it is
still initializing the sensor.
>
> >
> > Events can come at probe time (in case the kernel rebooted abruptly
> > without switching the sensor off for instance).
> >
> > Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
>
> Should add
>
> Reported-by: Douglas Anderson <dianders@chromium.org>
Done in v2.
>
> and also some sort of Fixes tag.
Done in v2.
>
> > diff --git a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
> > index e5ccedef13a80..5bf5cfb0e746b 100644
> > --- a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
> > +++ b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
> > @@ -340,17 +337,6 @@ int cros_ec_sensors_core_init(struct platform_device *pdev,
> >                         if (ret)
> >                                 return ret;
> >
> > -                       ret = cros_ec_sensorhub_register_push_data(
> > -                                       sensor_hub, sensor_platform->sensor_num,
> > -                                       indio_dev, push_data);
> > -                       if (ret)
> > -                               return ret;
> > -
> > -                       ret = devm_add_action_or_reset(
> > -                                       dev, cros_ec_sensors_core_clean, pdev);
> > -                       if (ret)
> > -                               return ret;
> > -
> >                         /* Timestamp coming from FIFO are in ns since boot. */
> >                         ret = iio_device_set_clock(indio_dev, CLOCK_BOOTTIME);
> >                         if (ret)
> > @@ -372,6 +358,39 @@ int cros_ec_sensors_core_init(struct platform_device *pdev,
> >  }
> >  EXPORT_SYMBOL_GPL(cros_ec_sensors_core_init);
> >
> > +/**
> > + * cros_ec_sensors_core_register() - Register callback to FIFO when sensor is
> > + * ready.
>
> Please document 'dev' as well here.
Done in v2.
>
> > + * @indio_dev:         iio device structure of the device
> > + * @push_data:          function to call when cros_ec_sensorhub receives
> > + *    a sample for that sensor.
> > + *
> > + * Return: 0 on success, -errno on failure.
> > + */
> > +int cros_ec_sensors_core_register(struct device *dev,
> > +                                 struct iio_dev *indio_dev,
> > +                                 cros_ec_sensorhub_push_data_cb_t push_data)
> > +{
> > +       struct cros_ec_sensor_platform *sensor_platform = dev_get_platdata(dev);
> > +       struct cros_ec_sensorhub *sensor_hub = dev_get_drvdata(dev->parent);
> > +       struct platform_device *pdev = to_platform_device(dev);
> > +       struct cros_ec_dev *ec = sensor_hub->ec;
> > +       int ret = 0;
> > +
> > +       if (cros_ec_check_features(ec, EC_FEATURE_MOTION_SENSE_FIFO)) {
>
> Just curious if this condition is ever false? Or if the case when it is
> true is when 'push_data' is cros_ec_sensors_push_data()?
It is false on older chromebooks, where sensor data needs to be polled
by the host.
>
> > +               ret = cros_ec_sensorhub_register_push_data(
> > +                               sensor_hub, sensor_platform->sensor_num,
> > +                               indio_dev, push_data);
> > +               if (ret)
> > +                       return ret;
> > +
> > +               ret = devm_add_action_or_reset(
> > +                               dev, cros_ec_sensors_core_clean, pdev);
> > +       }
> > +       return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(cros_ec_sensors_core_register);
> > +
> >  /**
> >   * cros_ec_motion_send_host_cmd() - send motion sense host command
> >   * @state:             pointer to state information for device
> > diff --git a/drivers/iio/pressure/cros_ec_baro.c b/drivers/iio/pressure/cros_ec_baro.c
> > index 25217279f3507..82f20c738a165 100644
> > --- a/drivers/iio/pressure/cros_ec_baro.c
> > +++ b/drivers/iio/pressure/cros_ec_baro.c
> > @@ -139,8 +139,7 @@ static int cros_ec_baro_probe(struct platform_device *pdev)
> >                 return -ENOMEM;
> >
> >         ret = cros_ec_sensors_core_init(pdev, indio_dev, true,
> > -                                       cros_ec_sensors_capture,
> > -                                       cros_ec_sensors_push_data);
> > +                                       cros_ec_sensors_capture);
> >         if (ret)
> >                 return ret;
> >
> > @@ -185,7 +184,13 @@ static int cros_ec_baro_probe(struct platform_device *pdev)
> >
> >         state->core.read_ec_sensors_data = cros_ec_sensors_read_cmd;
> >
> > -       return devm_iio_device_register(dev, indio_dev);
> > +       ret = devm_iio_device_register(dev, indio_dev);
> > +       if (ret)
> > +               return ret;
> > +
> > +       /* Ready to receive samples from the EC. */
> > +       return cros_ec_sensors_core_register(dev, indio_dev,
> > +                                            cros_ec_sensors_push_data);
>
> Would it make sense to add the devm_iio_device_register() call into
> cros_ec_sensors_core_register()? Then the caller can't mess up the order
> and register the push_data callback before the iio device.
Good idea, done in v2.
> And is the
> callback ever going to be not cros_ec_sensors_push_data() (or NULL in
> case of not motion sense)? Seems like that could be hardcoded into the
> function as well.
The callback can be different from the default cros_ec_sensors_push_data().