linux-kernel - Re: [PATCH 1/5] iio: gyro: adi16136: construct adis data on probe vs static on driver

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <02afd05b9d1563bd1bb1bedf231e20c8363f23d3.camel@analog.com>
Date:   Tue, 7 Jan 2020 09:36:41 +0000
From:   "Sa, Nuno" <Nuno.Sa@...log.com>
To:     "jic23@...nel.org" <jic23@...nel.org>,
        "Ardelean, Alexandru" <alexandru.Ardelean@...log.com>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>
Subject: Re: [PATCH 1/5] iio: gyro: adi16136: construct adis data on probe vs
 static on driver

Hi all,

On Mon, 2019-12-16 at 07:49 +0000, Ardelean, Alexandru wrote:
> On Sun, 2019-12-15 at 16:18 +0000, Jonathan Cameron wrote:
> > 
> > On Fri, 13 Dec 2019 18:03:08 +0200
> > Alexandru Ardelean <alexandru.ardelean@...log.com> wrote:
> > 
> > > This change is done in preparation of adding an `struct
> > > adis_timeout`
> > > type.
> > > Some ADIS drivers support multiple drivers, with various
> > > combinations
> > > of
> > > timeouts. Creating static tables for each driver is possible, but
> > > that
> > > also
> > > creates quite a lot of duplication.
> > > 
> > > Signed-off-by: Nuno Sá <nuno.sa@...log.com>
> > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@...log.com>
> > 
> > There are considerable advantages to using constant structures,
> > (security - not that relevant here probably, XiP, general
> > readability)
> > 
> > So to take a series like this I want to see evidence that it makes
> > a significant difference.  So far you just have cases where we end
> > up
> > with a worse result.  More code, harder to read...
> > 
> > Hence it will take a lot to persuade me to take this series without
> > the follow up patches where I assume significant advantages are
> > seen.
> > 
> 
> Well, we've have some discussion about this, and how to do it.
> There are several alternatives.
> 
> Some of the ideas were:
> 1. Keep the static data and clone it + populate the adis_timeout data
> as
> needed during probe [based on each device's chip-info]
> 2. Rework all the chip-info data to include the adis_data types/info
> 
> 2. may require more work ; 1. require fewer patches
> 
> This implementation [in this series] is 1. but without keeping the
> static
> data and template.
> I guess the idea was to reduce memory usage [by keeping the static
> data]. I
> admit the memory usage is not that big.
> 
> I'll take a look at this again, and see if 2. can work more nicely.
> It might be that 1. would be the end-result, but who knows?
> 
> Thanks
> Alex

I guess we also need to prepare/send the following patches to show
Jonathan why we need to dynamically allocate the data structure in some
drivers. In the end is because some devices require different timeouts
(handled by the adis core library) than the others and, in some cases
these differences are quite significative. It was even happening that
in some cases, we were not sleeping enough time (eg: after a reset
command). In the next patches, a timeout structure is included that
needs to be filled for each device.

Alex, maybe we should include more patches in this series to show the
"big picture" and then we can discuss if this is the best approach.

Nuno Sá