linux-kernel - Re: Question about patch "i2c: omap: resize fifos before each message"

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20141203174938.GJ16138@saruman>
Date:	Wed, 3 Dec 2014 11:49:38 -0600
From:	Felipe Balbi <balbi@...com>
To:	Alexander Kochetkov <al.kochet@...il.com>
CC:	<balbi@...com>, Kevin Hilman <khilman@...nel.org>,
	Tony Lindgren <tony@...mide.com>,
	Wolfram Sang <wsa@...-dreams.de>,
	linux-omap <linux-omap@...r.kernel.org>,
	<linux-i2c@...r.kernel.org>, LKML <linux-kernel@...r.kernel.org>
Subject: Re: Question about patch "i2c: omap: resize fifos before each
 message"

Hi,

On Wed, Dec 03, 2014 at 08:34:21PM +0300, Alexander Kochetkov wrote:
> >> 2. I want to avoid changing fifos before message submission, because
> >> IP can start receiving message in a slave mode (race).
> > 
> > I2C is not full-duplex. There's no way it will receive any data while
> > you're transmitting, right ?
> > 
> 
> I2C is half duplex, right. But, IP work in slave receiver and master
> transmitter modes.  And IP switch to slave receiver mode after master

it has more modes than that. It can also be a master receiver and a
slave transmitter. Note also that MST bit does *not* auto clear. This
means that as the driver is today, IP will always be in Master mode
which further strengthens my suspicion that what you describe can't
possibly happen.

> transfer (simply clear MST bit and ready for reception and that TRM
> state about). 

this is wrong. Clear MST bit and IP switches to slave mode. Transmit or
receive is selected through another bit (TRX -> bit 9 on I2C_CON). Also,
the IP won't do anything (considering it's always in master mode) until
STT bit is set again, so there's really no way for what you describe
below to ever happen with current driver.

> And question sounds like: what happen if we reset or change FIFO
> threshold value (in order to submit new master transfer) when IP start
> receiving data to the fifo.  How many bytes we have to read on RRDY?

I don't see how this would ever happen.

> That race I'am talking about. And there is only one place where race
> couldn't happen: it's ISR (thread). So I want to move almost all
> master initialization code into ISR.

if you do that, you'll end up with an IRQ handler that takes a lot of
time to run, because at every IRQ you'll reprogram the thing, it's
better to program things from omap_i2c_xfer_msg() and wait for the IRQ
to happen. Just as it is today.

> >> 3. dev->threshold is changed in range 1-fifo_size/2. So instead of RDR
> >> we get RRDY and for messages larger then fifo_size/2 we still get RRDY
> >> and RDR.
> > 
> > we will only get RDR if message_size % threshold > 0. If we have a 16
> > byte transfer and we program threshold to 8 bytes, we will get two RRDY
> > IRQs.
> > 
> >> Felipe, do you have in mind why do you want to avoid RDR and XDR events?
> >> Something about errata?
> > 
> > nothing about errata. As the commit log say (or tried to say), if the
> > entire message fits into the FIFO we save one interrupt. It's a
> > micro-optimization.
> 
> I see, thank you. But due to error only half of fifo is utilized for
> that.

right, it also tries to use double buffering, so that while IP is
shifting data onto SDA, we can feed the other half of the FIFO with more
data.

> And, I'll try to move fifo threshold init code into ISR. Don't see
> something wrong.

I wouldn't do that. It's just too late... look, IRQs won't fire until
I2C_CON is setup to start a transfer (transmit or receive), you *must*
resize FIFO before starting a transfer otherwise, well, you know...

-- 
balbi

Download attachment "signature.asc" of type "application/pgp-signature" (820 bytes)