linux-kernel - Re: [PATCH v4 2/3] iio: add MEMSensing MSA311 3-axis accelerometer 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: <20220813163854.7815e0ee@jic23-huawei>
Date:   Sat, 13 Aug 2022 16:39:04 +0100
From:   Jonathan Cameron <jic23@...nel.org>
To:     Dmitry Rokosov <DDRokosov@...rdevices.ru>
Cc:     Andy Shevchenko <andy.shevchenko@...il.com>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "stano.jakubek@...il.com" <stano.jakubek@...il.com>,
        "shawnguo@...nel.org" <shawnguo@...nel.org>,
        "lars@...afoo.de" <lars@...afoo.de>,
        "stephan@...hold.net" <stephan@...hold.net>,
        "linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        kernel <kernel@...rdevices.ru>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v4 2/3] iio: add MEMSensing MSA311 3-axis accelerometer
 driver

On Sat, 13 Aug 2022 16:27:15 +0100
Jonathan Cameron <jic23@...nel.org> wrote:

> On Tue, 9 Aug 2022 10:35:19 +0000
> Dmitry Rokosov <DDRokosov@...rdevices.ru> wrote:
> 
> > On Tue, Aug 09, 2022 at 12:05:12PM +0200, Andy Shevchenko wrote:  
> > > > > > > > +       indio_dev->modes = 0; /* setup buffered mode later */    
> > > > > > >
> > > > > > > Why explicit assignment to 0? Doesn't kzalloc() do it for you?    
> > > > > >
> > > > > > kzalloc() will do it for me, of course. Previously, I initialized modes to
> > > > > > INDIO_DIRECT_MODE to just provide default value for that. Jonathan
> > > > > > suggested to replace it with 0.    
> > > > >
> > > > > I did?  I wonder what I was smoking that day.
> > > > > Should be set to INDIO_DIRECT_MODE as you had it previously.
> > > > >
> > > > > (From what I recall it will work either way but we have in the past had
> > > > > core code that checked this and may do again in the future so drivers should
> > > > > still be setting it to specify they provide sysfs interfaces to directly read
> > > > > the channels).    
> > > >
> > > > Jonathan, really sorry I referred to you. I'm confused. This comment was
> > > > from Andy in the v3 discussion:
> > > >
> > > > https://lore.kernel.org/linux-iio/CAHp75Vc0+ckNnm2tzLMPrjeFRjwoj3zy0C4koNShFRG3kP8b6w@mail.gmail.com/    
> > > 
> > > Indeed. I was confused by the comment. My understanding at that time
> > > was that the triggered mode is inevitable and hence assigning to
> > > something which _will_ be reassigned later makes a little sense. So,
> > > does it mean that triggered mode is optional and might not be set? In
> > > such a case the comment is misleading.    
> > 
> > Actually, this comment was introduced in the early MSA311 driver
> > versions, when I have made buffer setup only if HW irq is enabled. In
> > the newest versions buffer is setup unconditionally, because buffer mode
> > can be used based on hrtimer software trigger.
> > 
> > Jonathan, why we shouldn't delete INDIO_DIRECT_MODE initialization if
> > after couple of lines we always setup buffer mode?
> >   
> 
> The buffered mode setup does
> modes |= INDIO_BUFFER_TRIGGERED;
> https://elixir.bootlin.com/linux/latest/source/drivers/iio/buffer/industrialio-triggered-buffer.c#L71
> 
> Direct mode indicates that it is possible to read the channels without
> using any of the triggered modes (there are devices - though rare - where
> it is not set as they are only accessible through FIFOs for example).
> 
> We don't make much use of IIO_DIRECT_MODE today (though we did until fairly
> recently).  It could be replaced with a specific check on provision of
> raw / processed channels I guess - but I'm not that keen to see it go without
> thinking hard about whether we should be using that flag to catch misconfiguration
> in some cases.  So I'd rather postpone any changes in that for now.

I remembered a bit more about this.  INDIO_DIRECT_MODE is used internally
in the state machine for currentmode (now moved to the opaque structure but
accessible via appropriate function). It's there to provide an explicit
state rather than 0 which would be not in a particular state (that I don't
like).  We'd have a somewhat odd (though possible) disconnect if we didn't
have the state in the ->modes bitmask.  (We could reserve the first bit
for example). However I'm still not keen in cleaning it up further.

Either way, we'd still need the various mode setting functions to carry only
doing |= as the driver is permitted to support multiple buffered modes
(unusual, but there are a few drivers doing so IIRC).

J
> 
> Jonathan