| 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:
<FR2PPF4571F02BC178BD74B06CAAD14FFB08C33A@FR2PPF4571F02BC.DEUP281.PROD.OUTLOOK.COM>
Date: Wed, 20 Aug 2025 13:34:09 +0000
From: Remi Buisson <Remi.Buisson@....com>
To: Jonathan Cameron <jic23@...nel.org>
CC: Remi Buisson via B4 Relay <devnull+remi.buisson.tdk.com@...nel.org>,
David
Lechner <dlechner@...libre.com>,
Nuno Sá
<nuno.sa@...log.com>,
Andy Shevchenko <andy@...nel.org>, Rob Herring
<robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley
<conor+dt@...nel.org>,
"linux-kernel@...r.kernel.org"
<linux-kernel@...r.kernel.org>,
"linux-iio@...r.kernel.org"
<linux-iio@...r.kernel.org>,
"devicetree@...r.kernel.org"
<devicetree@...r.kernel.org>
Subject: RE: [PATCH v2 3/8] iio: imu: inv_icm45600: add buffer support in iio
devices
>
>
>From: Jonathan Cameron <jic23@...nel.org>
>Sent: Saturday, August 16, 2025 1:17 PM
>To: Remi Buisson <Remi.Buisson@....com>
>Cc: Remi Buisson via B4 Relay <devnull+remi.buisson.tdk.com@...nel.org>; David Lechner <dlechner@...libre.com>; Nuno Sá <nuno.sa@...log.com>; Andy Shevchenko <andy@...nel.org>; Rob Herring <robh@...nel.org>; Krzysztof Kozlowski <krzk+dt@...nel.org>; Conor Dooley <conor+dt@...nel.org>; linux-kernel@...r.kernel.org; linux-iio@...r.kernel.org; devicetree@...r.kernel.org
>Subject: Re: [PATCH v2 3/8] iio: imu: inv_icm45600: add buffer support in iio devices
>
>On Mon, 11 Aug 2025 14: 13: 54 +0000 Remi Buisson <Remi. Buisson@ tdk. com> wrote: > > > > > >From: Jonathan Cameron <jic23@ kernel. org> > >Sent: Thursday, July 17, 2025 4: 34 PM > >To: Remi Buisson via B4
>On Mon, 11 Aug 2025 14:13:54 +0000
>Remi Buisson <Remi.Buisson@....com> wrote:
>
>> >
>> >
>> >From: Jonathan Cameron <jic23@...nel.org>
>> >Sent: Thursday, July 17, 2025 4:34 PM
>> >To: Remi Buisson via B4 Relay <devnull+remi.buisson.tdk.com@...nel.org>
>> >Cc: Remi Buisson <Remi.Buisson@....com>; David Lechner <dlechner@...libre.com>; Nuno Sá <nuno.sa@...log.com>; Andy Shevchenko <andy@...nel.org>; Rob Herring <robh@...nel.org>; Krzysztof Kozlowski <krzk+dt@...nel.org>; Conor Dooley <conor+dt@...nel.org>; linux-kernel@...r.kernel.org; linux-iio@...r.kernel.org; devicetree@...r.kernel.org
>> >Subject: Re: [PATCH v2 3/8] iio: imu: inv_icm45600: add buffer support in iio devices
>> >On Thu, 10 Jul 2025 08:57:58 +0000
>> >Remi Buisson via B4 Relay <devnull+remi.buisson.tdk.com@...nel.org> wrote:
>> >
>> >> From: Remi Buisson <remi.buisson@....com>
>> >>
>> >> Add FIFO control functions.
>> >> Support hwfifo watermark by multiplexing gyro and accel settings.
>> >> Support hwfifo flush.
>> >>
>> >> Signed-off-by: Remi Buisson <remi.buisson@....com>
>> >Hi Remi,
>> >
>> >Sorry for delay - hectic week.
>> >
>> >Jonathan
>> No problem, thanks for the review ! (and sorry for my late reply)
>> >
>> >> ---
>> >> drivers/iio/imu/inv_icm45600/Makefile | 1 +
>> >> drivers/iio/imu/inv_icm45600/inv_icm45600.h | 4 +
>> >> drivers/iio/imu/inv_icm45600/inv_icm45600_buffer.c | 514 +++++++++++++++++++++
>> >> drivers/iio/imu/inv_icm45600/inv_icm45600_buffer.h | 99 ++++
>> >> drivers/iio/imu/inv_icm45600/inv_icm45600_core.c | 137 +++++-
>> >We used to do the buffer / core split a lot but it often ends up more trouble
>> >that it is worth and we no longer make buffer support a build time option (which was
>> >what motivated the separate files) Consider how much simplification you'd get by squashing them into
>> >one file.
>> I understand the point.
>> However merging files will allow to remove 5 lines at most,
>> while the length of the core file will increase a lot.
>> I'm not sure of the benefit in the end, but
>> please let me know if you really want me to proceed with the merge.
>
>It's only a combined 1.5k. That would be fine even if the savings are fairly small.
>
>It's not something I care that much about though.
>
>
I'd rather keep it this way then.
>
>
>
>> >> +const struct iio_buffer_setup_ops inv_icm45600_buffer_ops = {
>> >> + .preenable = inv_icm45600_buffer_preenable,
>> >> + .postenable = inv_icm45600_buffer_postenable,
>> >> + .predisable = inv_icm45600_buffer_predisable,
>> >> + .postdisable = inv_icm45600_buffer_postdisable,
>> >> +};
>> >> +
>> >> +int inv_icm45600_buffer_fifo_read(struct inv_icm45600_state *st,
>> >> + unsigned int max)
>> >What is max here? Seems to be passed 0 in the only caller.
>> Function call with max > 0 is implemented later in the same patch
>> (in 4/8, from inv_icm45600_buffer_hwfifo_flush).
>
>Maybe push the parameter being introduced to ther.
I will do that.
>
>> >> +{
>> >> + const ssize_t packet_size = INV_ICM45600_FIFO_2SENSORS_PACKET_SIZE;
>> >> + __le16 *raw_fifo_count;
>> >> + size_t fifo_nb, i;
>> >> + ssize_t size;
>> >> + const struct inv_icm45600_fifo_sensor_data *accel, *gyro;
>> >> + const __le16 *timestamp;
>> >> + const s8 *temp;
>> >> + unsigned int odr;
>> >> + int ret;
>> >> +
>> >> + /* Reset all samples counters. */
>> >> + st->fifo.count = 0;
>> >> + st->fifo.nb.gyro = 0;
>> >> + st->fifo.nb.accel = 0;
>> >> + st->fifo.nb.total = 0;
>> >> +
>> >> + /* Read FIFO count value. */
>> >> + raw_fifo_count = &st->buffer.u16;
>> >> + ret = regmap_bulk_read(st->map, INV_ICM45600_REG_FIFO_COUNT,
>> >> + raw_fifo_count, sizeof(*raw_fifo_count));
>> >
>> >For IIO drivers at least we still operated under some guidance the regmap maintainer
>> >gave years ago to never assume regmap (for busses that otherwise require DMA safe
>> >buffers) will always bounce the data. So bulk reads with SPI buffers need
>> >DMA safe buffers. Easiest is usually an __aligned(IIO_DMA_MINALIGN) buffer
>> >(or set of buffers) at the end of st.
>> >
>> >In practice last time I checked regmap doesn't bother with the zero copy
>> >optimization that would need this safety so you won't actually hit this
>> >issue.
>> From my understanding, alignment of &st->buffer.u16 is correct because the union is aligned,
>>
>> union {
>> u8 buff[2];
>> __le16 u16;
>> } buffer __aligned(IIO_DMA_MINALIGN);
>>
>> Please let me know if my answer is not correct.
>Ah. I probably just missed that. can't remember!
No prob.
>
>> >
>
Powered by blists - more mailing lists