| 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: <CAMknhBHaSBF-a9nLZ0ZxB2-9HzYkPMBqUr4Aa4TthNnJMwtFgg@mail.gmail.com>
Date: Mon, 23 Jun 2025 16:44:41 -0600
From: David Lechner <dlechner@...libre.com>
To: jean-baptiste.maneyrol@....com
Cc: Jonathan Cameron <jic23@...nel.org>, Lars-Peter Clausen <lars@...afoo.de>, Nuno Sá <nuno.sa@...log.com>,
Andy Shevchenko <andy@...nel.org>, linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 2/3] iio: imu: inv_icm42600: add WoM support
On Mon, Jun 23, 2025 at 6:56 AM 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.
>
...
> +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)
> + msleep(sleep_ms);
> +
> + scoped_guard(mutex, &st->lock) {
> + ret = inv_icm42600_enable_wom(st);
> + if (ret)
> + break;
I reviewed this from the bottom up, so see comments on similar code below.
> + st->apex.on++;
> + st->apex.wom.enable = true;
> + }
> + if (ret)
> + goto error_suspend;
> +
> + 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) {
> + /*
> + * Consider that turning off WoM is always working to avoid
> + * blocking the chip in on mode and prevent going back to sleep.
> + * If there is an error, the chip will anyway go back to sleep
> + * and the feature will not work anymore.
> + */
> + st->apex.wom.enable = false;
> + st->apex.on--;
> + ret = inv_icm42600_disable_wom(st);
> + if (ret)
> + break;
The fact that scoped_guard() uses a for loop is an implementation
detail so using break here makes this look like improper C code. I
think this would be better to split out the protected section to a
separate function and just use the regular guard() macro.
> + /* 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);
Probably don't need the if here. msleep() won't sleep if we pass 0 to it.
> + pm_runtime_mark_last_busy(pdev);
> + pm_runtime_put_autosuspend(pdev);
> +
> + return ret;
> +}
> +
...
> +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
This is redundant else and can be removed.
> + 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);
> + u64 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)
> + return -EINVAL;
> +
> + if (val < 0 || val2 < 0)
> + return -EINVAL;
> +
> + value = (u64)val * MICRO + (u64)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);
At least we aren't using break here, but still this could be better
split out to a separate function so that we can use the regular return
on error pattern.
> + }
> + pm_runtime_mark_last_busy(dev);
> + pm_runtime_put_autosuspend(dev);
> +
> + return ret;
> +}
> +
Powered by blists - more mailing lists