| 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:
<FR3P281MB1757B4BF12AD9F6AC5F8F361CEBD2@FR3P281MB1757.DEUP281.PROD.OUTLOOK.COM>
Date: Wed, 16 Apr 2025 12:40:59 +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
Hello Andy,
concerning usage of kernel types, my understanding was that we should conform to existing types usage in a driver. That's why I'm keeping the standard types instead of kernel ones. I could change them in the patch, but it would mix usage of both standard and kernel types.
Another thing not related, by reading the coding style documentation I would think that for new driver we can use whatever we prefer between standard types and kernel types. Is it not the case?
Thanks for your help,
JB
________________________________________
From: Andy Shevchenko <andy.shevchenko@...il.com>
Sent: Tuesday, April 15, 2025 20:18
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>; 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
This Message Is From an External Sender
This message came from outside your organization.
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.
> +};
...
> 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.
> + 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?
> + value = div64_u64(roc * 1000000U, freq_uhz * (uint64_t)convert);
MICRO
> + /* limit value to 8 bits and prevent 0 */
> + return min(255, max(1, value));
Reinvention of the clamp() ?
> +}
...
> +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?
> + 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.
> + 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.
> + 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.
> + /* 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.
> + 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.
> + value = (uint64_t)val * 1000000ULL + (uint64_t)val2;
MICRO.
ULL is not needed, the first one already ULL.
> + 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
Powered by blists - more mailing lists