| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID:
<FR3P281MB17570E56798219E17F8C0814CEBC2@FR3P281MB1757.DEUP281.PROD.OUTLOOK.COM>
Date: Thu, 17 Apr 2025 14:25:41 +0000
From: Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@....com>
To: Andy Shevchenko <andy.shevchenko@...il.com>
CC: Jonathan Cameron <jic23@...nel.org>, Lars-Peter Clausen <lars@...afoo.de>,
David Lechner <dlechner@...libre.com>,
Nuno Sá
<nuno.sa@...log.com>,
Andy Shevchenko <andy@...nel.org>,
"linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 1/2] iio: imu: inv_icm42600: add WoM support
> On Tue, Apr 15, 2025 at 5:47 PM Jean-Baptiste Maneyrol via B4 Relay
> <devnull+jean-baptiste.maneyrol.tdk.com@...nel.org> wrote:
> >
> > From: Jean-Baptiste Maneyrol <jean-baptiste.maneyrol@....com>
> >
> > Add WoM as accel roc rising x|y|z event.
>
> ...
>
> > +struct inv_icm42600_apex {
> > + unsigned int on;
> > + struct {
> > + bool enable;
> > + uint64_t value;
>
> Why not kernel types? It seems in many places in the driver the
> *intXX_t are used instead of simple uXX/sXX.
>
> > + } wom;
>
> Have you run `pahole`? To me it seems swapping the members in the
> outer struct will save some memory at run time.
You're right, we can inverse order and gain some memory. pahole is confirming.
>
> > +};
>
> ...
>
> > struct {
> > int64_t gyro;
> > int64_t accel;
>
> s64 ?
>
> > } timestamp;
>
> ...
>
> > + uint8_t buffer[3] __aligned(IIO_DMA_MINALIGN);
>
> u8 ?
>
> ...
>
> > +static unsigned int inv_icm42600_accel_convert_roc_to_wom(uint64_t roc,
>
> u64
>
> > + int accel_hz, int accel_uhz)
> > +{
> > + /* 1000/256mg per LSB converted in m/s² in micro (1000000) */
>
> That 1000000 is redundant, just properly spell the units.
I will use um/s²
>
> > + const unsigned int convert = (1000U * 9807U) / 256U;
> > + uint64_t value;
> > + uint64_t freq_uhz;
>
> u64
>
> > + /* return 0 only if roc is 0 */
> > + if (roc == 0)
> > + return 0;
> > +
> > + freq_uhz = (uint64_t)accel_hz * 1000000U + (uint64_t)accel_uhz;
>
> u64
>
> MICRO?
Good idea, I didn't know it exists.
>
> > + value = div64_u64(roc * 1000000U, freq_uhz * (uint64_t)convert);
>
> MICRO
No problem, yes.
>
> > + /* limit value to 8 bits and prevent 0 */
> > + return min(255, max(1, value));
>
> Reinvention of the clamp() ?
It was a copy-paste of an older driver, at the time clamp was not here.
I will replace by clamp, it is much more readable.
>
> > +}
>
> ...
>
> > +static uint64_t inv_icm42600_accel_convert_wom_to_roc(unsigned int threshold,
> > + int accel_hz, int accel_uhz)
>
> Similar comments as per above.
>
> ...
>
> > +static int inv_icm42600_accel_enable_wom(struct iio_dev *indio_dev)
> > +{
> > + struct inv_icm42600_state *st = iio_device_get_drvdata(indio_dev);
> > + struct inv_icm42600_sensor_state *accel_st = iio_priv(indio_dev);
> > + struct device *pdev = regmap_get_device(st->map);
> > + struct inv_icm42600_sensor_conf conf = INV_ICM42600_SENSOR_CONF_INIT;
> > + unsigned int sleep_ms = 0;
> > + int ret;
> > +
> > + ret = pm_runtime_resume_and_get(pdev);
> > + if (ret)
> > + return ret;
> > +
> > + scoped_guard(mutex, &st->lock) {
> > + /* turn on accel sensor */
> > + conf.mode = accel_st->power_mode;
> > + conf.filter = accel_st->filter;
> > + ret = inv_icm42600_set_accel_conf(st, &conf, &sleep_ms);
> > + if (ret)
> > + goto error_suspend;
> > + }
>
> > + if (sleep_ms)
>
> Do you need this check?
We need this check in the case sleep_ms is 0. It will happen if accel is
already on. msleep(0) was usually doing a sleep before. I don't know if it
is still the case.
>
> > + msleep(sleep_ms);
> > +
> > + scoped_guard(mutex, &st->lock) {
> > + ret = inv_icm42600_enable_wom(st);
> > + if (ret)
> > + goto error_suspend;
> > + st->apex.on++;
> > + st->apex.wom.enable = true;
> > + }
> > +
> > + return 0;
> > +
> > +error_suspend:
> > + pm_runtime_mark_last_busy(pdev);
> > + pm_runtime_put_autosuspend(pdev);
> > + return ret;
> > +}
> > +
> > +static int inv_icm42600_accel_disable_wom(struct iio_dev *indio_dev)
> > +{
> > + struct inv_icm42600_state *st = iio_device_get_drvdata(indio_dev);
> > + struct device *pdev = regmap_get_device(st->map);
> > + struct inv_icm42600_sensor_conf conf = INV_ICM42600_SENSOR_CONF_INIT;
> > + unsigned int sleep_ms = 0;
> > + int ret;
> > +
> > + scoped_guard(mutex, &st->lock) {
> > + st->apex.wom.enable = false;
> > + st->apex.on--;
> > + ret = inv_icm42600_disable_wom(st);
> > + if (ret)
> > + break;
> > + /* turn off accel sensor if not used */
> > + if (!st->apex.on && !iio_buffer_enabled(indio_dev)) {
> > + conf.mode = INV_ICM42600_SENSOR_MODE_OFF;
> > + ret = inv_icm42600_set_accel_conf(st, &conf, &sleep_ms);
> > + if (ret)
> > + break;
> > + }
> > + }
>
> > + if (sleep_ms)
>
> Ditto.
Same thing, it is to avoid possible msleep(0).
>
> > + msleep(sleep_ms);
>
>
> > + pm_runtime_mark_last_busy(pdev);
> > + pm_runtime_put_autosuspend(pdev);
> > +
> > + return ret;
> > +}
>
> ...
>
> > +void inv_icm42600_accel_handle_events(struct iio_dev *indio_dev,
> > + unsigned int status2, unsigned int status3,
> > + int64_t timestamp)
>
> Okay, I believe here int64_t is inherited from IIO definitions.
>
> > +{
> > + struct inv_icm42600_state *st = iio_device_get_drvdata(indio_dev);
> > + uint64_t ev_code;
>
> u64.
>
> > + /* handle WoM event */
> > + if (st->apex.wom.enable && (status2 & INV_ICM42600_INT_STATUS2_WOM_INT)) {
> > + ev_code = IIO_MOD_EVENT_CODE(IIO_ACCEL, 0, IIO_MOD_X_OR_Y_OR_Z,
> > + IIO_EV_TYPE_ROC, IIO_EV_DIR_RISING);
> > + iio_push_event(indio_dev, ev_code, timestamp);
> > + }
> > +}
>
> ...
>
> > +static int inv_icm42600_accel_write_event_config(struct iio_dev *indio_dev,
> > + const struct iio_chan_spec *chan,
> > + enum iio_event_type type,
> > + enum iio_event_direction dir,
> > + bool state)
> > +{
> > + struct inv_icm42600_state *st = iio_device_get_drvdata(indio_dev);
> > +
> > + /* handle WoM (roc rising) event */
> > + if (type == IIO_EV_TYPE_ROC && dir == IIO_EV_DIR_RISING) {
>
> Invert the conditional to reduce indentation of the below. It will
> also follow the traditional pattern, i.e. checking for the errors
> first.
It was done like this because I am planning to add other events like tilt.
But I will reverse it, it can be changed latter.
>
> > + scoped_guard(mutex, &st->lock) {
> > + if (st->apex.wom.enable == state)
> > + return 0;
> > + }
> > + if (state)
> > + return inv_icm42600_accel_enable_wom(indio_dev);
>
> > + else
>
> Redundant 'else', but in this case for the sake of symmetry it can be left.
>
> > + return inv_icm42600_accel_disable_wom(indio_dev);
> > + }
> > +
> > + return -EINVAL;
> > +}
>
> ...
>
> > + if (type == IIO_EV_TYPE_ROC && dir == IIO_EV_DIR_RISING) {
>
> Invert the conditional.
No problem either.
>
> > + /* return value in micro */
> > + *val = div_u64_rem(st->apex.wom.value, 1000000U, &rem);
> > + *val2 = rem;
> > + return IIO_VAL_INT_PLUS_MICRO;
> > + }
> > +
> > + return -EINVAL;
>
> ...
>
> > +static int inv_icm42600_accel_write_event_value(struct iio_dev *indio_dev,
> > + const struct iio_chan_spec *chan,
> > + enum iio_event_type type,
> > + enum iio_event_direction dir,
> > + enum iio_event_info info,
> > + int val, int val2)
> > +{
> > + struct inv_icm42600_state *st = iio_device_get_drvdata(indio_dev);
> > + struct device *dev = regmap_get_device(st->map);
> > + uint64_t value;
>
> u64
>
> > + unsigned int accel_hz, accel_uhz;
> > + int ret;
> > +
> > + /* handle WoM (roc rising) event value */
> > + if (type == IIO_EV_TYPE_ROC && dir == IIO_EV_DIR_RISING) {
>
> Invert the conditional.
No problem also.
>
> > + if (val < 0 || val2 < 0)
> > + return -EINVAL;
>
> This can be checked even before anything else, but see above. With
> that it will automatically get on the same indentation level as
> previous one.
I will merge with previous check with inverted conditional.
>
> > + value = (uint64_t)val * 1000000ULL + (uint64_t)val2;
>
> MICRO.
> ULL is not needed, the first one already ULL.
Will do.
>
> > + pm_runtime_get_sync(dev);
> > + scoped_guard(mutex, &st->lock) {
> > + ret = inv_icm42600_accel_read_odr(st, &accel_hz, &accel_uhz);
> > + if (ret >= 0)
> > + ret = inv_icm42600_accel_set_wom_threshold(st, value,
> > + accel_hz, accel_uhz);
> > + }
> > + pm_runtime_mark_last_busy(dev);
> > + pm_runtime_put_autosuspend(dev);
> > + return ret;
> > + }
> > +
> > + return -EINVAL;
> > +}
>
> --
> With Best Regards,
> Andy Shevchenko
Thanks,
JB
Powered by blists - more mailing lists