| 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:
<FR3P281MB175789F6AADF5D0D15BC89A1CE70A@FR3P281MB1757.DEUP281.PROD.OUTLOOK.COM>
Date: Mon, 16 Jun 2025 07:42:16 +0000
From: Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@....com>
To: Jonathan Cameron <jic23@...nel.org>,
Jean-Baptiste Maneyrol via B4 Relay
<devnull+jean-baptiste.maneyrol.tdk.com@...nel.org>
CC: 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 v4 1/2] iio: imu: inv_icm42600: add WoM support
>
>________________________________________
>From: Jonathan Cameron <jic23@...nel.org>
>Sent: Saturday, June 14, 2025 14:53
>To: Jean-Baptiste Maneyrol via B4 Relay <devnull+jean-baptiste.maneyrol.tdk.com@...nel.org>
>Cc: Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@....com>; 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 v4 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, 13 Jun 2025 09:34:26 +0200
>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.
>>
>> Signed-off-by: Jean-Baptiste Maneyrol <jean-baptiste.maneyrol@....com>
>Hi Jean-Baptiste.
>
>A couple of comments inline.
>Ideally pull the movement of the timestamp struct to before the DMA safe
>buffers to a precursor patch. That is a bit subtle to have hiding in here.
>
>The guards thing can be for next time you are doing a cleanup series on this
>driver if you prefer.
>
>Jonathan
Hello Jonathan,
concerning the full driver rewrite asked by Andy to switch to uXX/sXX kernel types,
can I put it inside this series?
Otherwise, should it be in a separate patch and perhaps with a fixed tag so it
can be backported to enable automatic backport of further fix patches?
Or can it be after this series is accepted? I would prefer that.
Thanks for your help here.
>
>> ---
>> drivers/iio/imu/inv_icm42600/inv_icm42600.h | 54 +++-
>> drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c | 289 ++++++++++++++++++++-
>> drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c | 2 +-
>> drivers/iio/imu/inv_icm42600/inv_icm42600_core.c | 58 +++++
>> 4 files changed, 395 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600.h b/drivers/iio/imu/inv_icm42600/inv_icm42600.h
>> index f893dbe6996506a33eb5d3be47e6765a923665c9..413a15493bcb880dc00b20da3b3168d5addd32a9 100644
>> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600.h
>> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600.h
>> @@ -135,6 +135,14 @@ struct inv_icm42600_suspended {
>> bool temp;
>> };
>>
>> +struct inv_icm42600_apex {
>> + unsigned int on;
>> + struct {
>> + uint64_t value;
>> + bool enable;
>> + } wom;
>> +};
>> +
>> /**
>> * struct inv_icm42600_state - driver state variables
>> * @lock: lock for serializing multiple registers access.
>> @@ -148,9 +156,10 @@ struct inv_icm42600_suspended {
>> * @suspended: suspended sensors configuration.
>> * @indio_gyro: gyroscope IIO device.
>> * @indio_accel: accelerometer IIO device.
>> - * @buffer: data transfer buffer aligned for DMA.
>> - * @fifo: FIFO management structure.
>> * @timestamp: interrupt timestamps.
>> + * @apex: APEX (Advanced Pedometer and Event detection) management
>> + * @fifo: FIFO management structure.
>> + * @buffer: data transfer buffer aligned for DMA.
>> */
>> struct inv_icm42600_state {
>> struct mutex lock;
>> @@ -164,12 +173,13 @@ struct inv_icm42600_state {
>> struct inv_icm42600_suspended suspended;
>> struct iio_dev *indio_gyro;
>> struct iio_dev *indio_accel;
>> - uint8_t buffer[2] __aligned(IIO_DMA_MINALIGN);
>> - struct inv_icm42600_fifo fifo;
>> struct {
>> int64_t gyro;
>> int64_t accel;
>> } timestamp;
>This was a bit subtle and had me going for a minute.
>The timestamp should never have been at this location in the structure because
>it's mid way through various regions with forced alignment. It isn't actually a bug
>I think though (beyond unnecessary padding) because the fifo struct obeyed c spec rule
>that anything after it must be aligned to it's largest aligned element which was
>IIO_DMA_MINALIGN.
>
>Maybe move this in a precursor patch where you can talk about whether it was a problem
>or not?
I can move it in a separate patch at the beginning of the series. This fix was asked
by you to avoid potential hard bugs, but it dates sorry.
>
>> + struct inv_icm42600_apex apex;
>> + struct inv_icm42600_fifo fifo;
>> + uint8_t buffer[3] __aligned(IIO_DMA_MINALIGN);
>> };
>>
>> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
>> index e6cd9dcb0687d19554e63a69dc60f065c58d70ee..9a2089527a9426b70eb796d4e9c234d8804c508b 100644
>> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
>> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
>
>
>
>> @@ -860,6 +911,13 @@ static int inv_icm42600_resume(struct device *dev)
>> if (ret)
>> goto out_unlock;
>>
>> + /* restore APEX features */
>> + if (st->apex.wom.enable) {
>> + ret = inv_icm42600_enable_wom(st);
>> + if (ret)
>> + goto out_unlock;
>
>One for another day, but this would definitely benefit from some guard() magic
>and there are a few other bits of existing code that would as well.
Same here, it was decided long ago in the first series to not switch to guard()
yet but later.
>
>
>> + }
>> +
>> /* restore FIFO data streaming */
>> if (st->fifo.on) {
>> inv_sensors_timestamp_reset(&gyro_st->ts);
>>
>
>
Thanks,
JB
Powered by blists - more mailing lists