Re: [Outreachy kernel] iio meter patches

* Re: [Outreachy kernel] iio meter patches - same!]
@ 2017-03-22 17:41 Alison Schofield
  2017-03-23  8:11 ` Daniel Baluta
  0 siblings, 1 reply; 6+ messages in thread
From: Alison Schofield @ 2017-03-22 17:41 UTC (permalink / raw)
  To: daniel.baluta; +Cc: outreachy-kernel

Daniel - Find "Daniel" near end of msg

----- Forwarded message from Alison Schofield <amsfield22@gmail.com> -----

Date: Tue, 21 Mar 2017 20:48:33 -0700
From: Alison Schofield <amsfield22@gmail.com>
To: Gargi Sharma <gs051095@gmail.com>
Cc: Arushi Singhal <arushisinghal19971997@gmail.com>, simran singhal
	<singhalsimran0@gmail.com>, sayli karnik <karniksayli1995@gmail.com>,
	outreachy-kernel@googlegroups.com
Subject: Re: [Outreachy kernel] iio meter patches - same!
User-Agent: Mutt/1.5.23 (2014-03-12)

On Wed, Mar 22, 2017 at 04:21:48AM +0530, Gargi Sharma wrote:
> On Wed, Mar 22, 2017 at 2:01 AM, Alison Schofield <amsfield22@gmail.com> wrote:
> >
> > Simran, Gargi, Sayli, Arushi,
> >
> > With respect to these...
> >
> > meter/ade7753.c: simran singhal
> > meter/ade7754.c: Gargi Sharma
> > meter/ade7758_core.c: Sayli Karnik
> > meter/ade7759.c: ArushiSinghal
> >
> > They are all near identical and therefore should have near identical
> > solutions.  You could work together, or follow one another, but, in
> > the end we don't want to see 4 different flavors of the solution.
> > That makes it harder to maintain.
> >
> > Also - if 'we' can get one reviewed/ACK'd/applied, then we make the
> > reviewers and maintainers life easier when we follow it with 3
> > similar patches.
> >
> > Here's where I think you all dancing around...
> > You have Lars' feedback from ade7753:
> > > It might make sense to reuse the existing lock which currently
> > > protects the
> > > read/write functions. You can do this by introducing a variant of
> > > ade7753_spi_{read,write}_reg_16() that does not take a lock and use
> > > these to
> > > implement the read-modify-write cycle in a protected section.
> >
> > I'll add to that:
> > They were all using mlock to try to protect the spi read followed by
> > write operation.
> I have a question here: why do we want to protect the spi read?, I mean
> usually we want to protect writes since there might be a race condition.
> Or is it because if two threads are running the same function
> concurrently we want to give access of the function to the thread
> number 2 only once thread number 1 has written to the register?
> 
> >I don't think the lock was actually doing that.
> > I don't see a guarantee that another spi write could not intervene,
> > So perhaps there was no real protection.
> Again, if we have two threads 1 and 2, won't the structure indio_dev
> be same for the both of them? And only one thread can hold the lock at
> a time.
> >
> > I'm thinking one function that locks the existing transaction lock, does
> > the read, does the write, unlocks.
> Simran wrote a function write_then_read in this patch
> http://marc.info/?l=linux-driver-devel&m=149001635402891&w=2. Do we
> want to do something similar?
>
Good questions Gargi!  

Code was doing this - lock mlock, read, write, unlock mlock.
I actually think that sysfs would handle allowing only one reader/write
of that frequency at a time.  And, the driver doesn't use mlock anywhere
else. The vulnerable place was between the read and the write...

If you trace the code down to the spi subsystem calls you'll see that
the read you are questioning, is actually a write followed by a read. 
Now, most iio developers would just surmise that was needed, but let's 
follow that to the data sheet:

http://www.analog.com/media/en/technical-documentation/data-sheets/ADE7754.pdf
page 32 tells us "Each register is accessed by first writing to the
communications register, then transferring the register data."

The driver needs to read the current value of the register,
change the values and write the register back out to the device.
That is what you see happening in ade7754_write_frequency() 
We want to protect that set of transactions - the "read_modify_write"
(Yes - it's like Simran's other patch- but opposite I guess ;))

I think the vulnerability here is that we don't want anyone coming in
and changing the designated register that we are about to write. 
Another spi_read at this point could reset the register pointer,
and an spi_write type operation could write trash to the frequency
register.

The answer won't be to replace mlock with a new lock, because that
won't prevent other types of spi reads or writes from happening.
That would only protect this path.

BTW - I don't think you are just migrating away from mlock usage
anymore.  You are fixing a bug :)

OK- that's all my input for tonite. btw - I'm looking this all up
alongside you.  I'm not holding back.  I don't have a solution in
mind.  This needs to simmer in my brain overnite.  I look forward
to seeing what you all come up with while I'm sleeping.

alisons

Hi Daniel,

Can you sanity check this?  Above is what I thought yesterday.

We've been headed down the path of wanting to make the read
followed by write safe/atomic.  Now, I'm wondering why we are
even doing the read.  Can we just save the current register
value in global data, modify and write that out as needed?
(ade7754: ade7754_write_frequency())

That would be the simplest solution.  Too easy?

thanks,
alisons

> > > > Can you all coordinate that please.  This way the next patch we see on
> > this is agreed upon by all four.  Once that is applauded ;), the others
> > can follow easily!
> >
> Yeah that makes sense and is the most productive use of all of our ability! :)
> 
> Thanks!
> Gargi
> 
> > thanks,
> > alison
> >

----- End forwarded message -----

^ permalink raw reply	[flat|nested] 6+ messages in thread