linux-kernel - Re: [PATCH v4 5/6] iio: imu: adis16550: add adis16550 support

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

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <c5bbfd1555a8a141caca944f2bb1b6eccb88fd67.camel@gmail.com>
Date: Mon, 13 Jan 2025 09:36:38 +0000
From: Nuno Sá <noname.nuno@...il.com>
To: Jonathan Cameron <jic23@...nel.org>, Robert Budai
 <robert.budai@...log.com>
Cc: Nuno Sa <nuno.sa@...log.com>, Ramona Gradinariu	
 <ramona.gradinariu@...log.com>, Antoniu Miclaus
 <antoniu.miclaus@...log.com>,  Lars-Peter Clausen	 <lars@...afoo.de>,
 Michael Hennerich <Michael.Hennerich@...log.com>, Rob Herring
 <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley	
 <conor+dt@...nel.org>, Jonathan Corbet <corbet@....net>, Alex Lanzano	
 <lanzano.alex@...il.com>, linux-iio@...r.kernel.org,
 devicetree@...r.kernel.org, 	linux-kernel@...r.kernel.org,
 linux-doc@...r.kernel.org
Subject: Re: [PATCH v4 5/6] iio: imu: adis16550: add adis16550 support

On Sun, 2025-01-12 at 16:11 +0000, Jonathan Cameron wrote:
> On Fri, 10 Jan 2025 09:42:53 +0200
> Robert Budai <robert.budai@...log.com> wrote:
> 
> > The ADIS16550 is a complete inertial system that includes a triaxis
> > gyroscope and a triaxis accelerometer. Each inertial sensor in
> > the ADIS16550 combines industry leading MEMS only technology
> > with signal conditioning that optimizes dynamic performance. The
> > factory calibration characterizes each sensor for sensitivity, bias,
> > and alignment. As a result, each sensor has its own dynamic com-
> > pensation formulas that provide accurate sensor measurements
> > 
> > Co-developed-by: Ramona Gradinariu <ramona.gradinariu@...log.com>
> > Signed-off-by: Ramona Gradinariu <ramona.gradinariu@...log.com>
> > Co-developed-by: Antoniu Miclaus <antoniu.miclaus@...log.com>
> > Signed-off-by: Antoniu Miclaus <antoniu.miclaus@...log.com>
> > Signed-off-by: Nuno Sá <nuno.sa@...log.com>
> > Signed-off-by: Robert Budai <robert.budai@...log.com>
> > ---
> > 
> > 4:
> > - reorganized channels to match the order in the datasheet
> > - removed extra checks and goto statements
> > - for buffer memory allocation used only kfree, since adis library already
> > takes care of freeing the buffer
> 
> That last bit makes for a mess wrt to who owns the buffer and lifetime
> management. Suggestions inline.
> 
> 

...

> > +
> > +	/*
> > +	 * Allocate the xfer and buffer for the burst read operation. The
> > buffer
> > +	 * is used to store the burst data and the xfer is used to send the
> > burst
> > +	 * command and receive the data. On device remove the adis libraary
> > is going
> 
> library.
> 
> > +	 * to free the xfer and buffer.
> 
> That is a horrible lifetime control.  They should either be allocated in the
> library
> and freed in the library or allocated and freed in the driver.  Mixing that
> doesn't
> make sense.

Agreed. TBH, I did not remembered at all to suggest ignoring the library and let
it call kfree() on NULL. I also don't like that much of how things are done in
the lib but I don't think it would be that straight to change things for the non
'burst mode' cases. For the devices implementing 'burst mode' doing the free +
allocation on every update_scan_modes seems unnecessary. Anyways, maybe
something worth looking at in the future.

- Nuno Sá