| 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:
<FR3P281MB17573A98ECB6B89306DB8DCBCE6AA@FR3P281MB1757.DEUP281.PROD.OUTLOOK.COM>
Date: Tue, 10 Jun 2025 14:13:38 +0000
From: Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@....com>
To: Andy Shevchenko <andy@...nel.org>
CC: Jonathan Cameron <jic23@...nel.org>, Lars-Peter Clausen <lars@...afoo.de>,
David Lechner <dlechner@...libre.com>,
Nuno Sá
<nuno.sa@...log.com>,
linux-iio <linux-iio@...r.kernel.org>,
LKML
<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 1/2] iio: imu: inv_icm42600: add WoM support
Hello Andy,
sorry for the very late response, here are my answers.
Thanks,
JB
>________________________________________
>From: Andy Shevchenko <andy@...nel.org>
>Sent: Saturday, April 19, 2025 17:39
>To: Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@....com>
>Cc: Jonathan Cameron <jic23@...nel.org>; Lars-Peter Clausen <lars@...afoo.de>; David Lechner <dlechner@...libre.com>; Nuno Sá <nuno.sa@...log.com>; linux-iio@...r.kernel.org <linux-iio@...r.kernel.org>; linux-kernel@...r.kernel.org <linux-kernel@...r.kernel.org>
>Subject: Re: [PATCH v3 1/2] iio: imu: inv_icm42600: add WoM support
>
>This Message Is From an External Sender
>This message came from outside your organization.
>
>On Fri, Apr 18, 2025 at 06:19:02PM +0200, Jean-Baptiste Maneyrol via B4 Relay wrote:
>> From: Jean-Baptiste Maneyrol <jean-baptiste.maneyrol@....com>
>>
>> Add WoM as accel roc rising x|y|z event.
>
>...
>
>> +static unsigned int inv_icm42600_accel_convert_roc_to_wom(uint64_t roc,
>> + int accel_hz, int accel_uhz)
>> +{
>> + /* 1000/256mg per LSB converted in µm/s² */
>> + const unsigned int convert = (1000U * 9807U) / 256U;
>
>Wondering if KILO (or MILLI?) is a good suit here...
This one is a little complex, since we have gravity value in mm/s² multiplied
by 1000 to go to µm/s².
If you have an idea of better writing that, I will do.
>
>> + uint64_t value;
>> + uint64_t freq_uhz;
>> +
>> + /* return 0 only if roc is 0 */
>> + if (roc == 0)
>> + return 0;
>> +
>> + freq_uhz = (uint64_t)accel_hz * MICRO + (uint64_t)accel_uhz;
>> + value = div64_u64(roc * MICRO, freq_uhz * (uint64_t)convert);
>> +
>> + /* limit value to 8 bits and prevent 0 */
>> + return clamp(value, 1, 255);
>> +}
>> +
>> +static uint64_t inv_icm42600_accel_convert_wom_to_roc(unsigned int threshold,
>> + int accel_hz, int accel_uhz)
>> +{
>> + /* 1000/256mg per LSB converted in µm/s² */
>> + const unsigned int convert = (1000U * 9807U) / 256U;
>
>Ditto.
Same as above.
>
>> + uint64_t value;
>> + uint64_t freq_uhz;
>> +
>> + value = threshold * convert;
>> + freq_uhz = (uint64_t)accel_hz * MICRO + (uint64_t)accel_uhz;
>> +
>> + /* compute the differential by multiplying by the frequency */
>> + return div_u64(value * freq_uhz, MICRO);
>> +}
>
>...
>
>> +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--;
>
>Hmm... Even if the below fails we consider it successful? Why?
If it fails, there is no easy way to restore functioning. Better consider
everything is disabled to not prevent the chip go into sleep (which will
disable the feature anyway) and give a chance to reenable it afterward.
>
>> + 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)
>> + msleep(sleep_ms);
>> + pm_runtime_mark_last_busy(pdev);
>> + pm_runtime_put_autosuspend(pdev);
>> +
>> + return ret;
>> +}
>
>...
>
>> +static int inv_icm42600_accel_read_event_config(struct iio_dev *indio_dev,
>> + const struct iio_chan_spec *chan,
>> + enum iio_event_type type,
>> + enum iio_event_direction dir)
>> +{
>> + struct inv_icm42600_state *st = iio_device_get_drvdata(indio_dev);
>> +
>> + guard(mutex)(&st->lock);
>> +
>> + /* handle WoM (roc rising) event */
>> + if (type == IIO_EV_TYPE_ROC && dir == IIO_EV_DIR_RISING)
>> + return st->apex.wom.enable ? 1 : 0;
>
>Invert conditional as below?
No problem, will do.
>
>> + return -EINVAL;
>> +}
>> +
>> +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 only WoM (roc rising) event */
>> + if (type != IIO_EV_TYPE_ROC || dir != IIO_EV_DIR_RISING)
>> + return -EINVAL;
>> +
>> + scoped_guard(mutex, &st->lock) {
>> + if (st->apex.wom.enable == state)
>> + return 0;
>> + }
>> +
>> + if (state)
>> + return inv_icm42600_accel_enable_wom(indio_dev);
>> + else
>> + return inv_icm42600_accel_disable_wom(indio_dev);
>> +}
>
>...
>
>> +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;
>> + unsigned int accel_hz, accel_uhz;
>> + int ret;
>> +
>> + /* handle only WoM (roc rising) event value */
>> + if (type != IIO_EV_TYPE_ROC || dir != IIO_EV_DIR_RISING || val < 0 || val2 < 0)
>> + return -EINVAL;
>
>Hmm... I think that splitting this will be logically better, as we see the
>type/dir conditionals in many functions, and values checks only
>exceptionally.
No problem, will do.
>
> if (type != IIO_EV_TYPE_ROC || dir != IIO_EV_DIR_RISING)
> return -EINVAL;
>
> if (val < 0 || val2 < 0)
> return -EINVAL;
>
>> + value = (uint64_t)val * MICRO + (uint64_t)val2;
>> + 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;
>> +}
>
>--
>With Best Regards,
>Andy Shevchenko
>
>
>
>
Powered by blists - more mailing lists