linux-kernel - Re: [PATCH 3/8 v3] i2c: i801: check INTR after every transaction

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: <CAGS+omDV_ynBL-PN0qmLmDRiHGbQFf+TwqC2-+KJY8zy9FDhrA@mail.gmail.com>
Date:	Mon, 2 Jul 2012 09:19:24 +0800
From:	Daniel Kurtz <djkurtz@...omium.org>
To:	Jean Delvare <khali@...ux-fr.org>
Cc:	Ben Dooks <ben-linux@...ff.org>,
	Wolfram Sang <w.sang@...gutronix.de>,
	Seth Heasley <seth.heasley@...el.com>,
	Olof Johansson <olof@...om.net>,
	Benson Leung <bleung@...omium.org>, linux-i2c@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/8 v3] i2c: i801: check INTR after every transaction

On Mon, Jul 2, 2012 at 5:20 AM, Jean Delvare <khali@...ux-fr.org> wrote:
> Hi again Daniel,
>
> On Wed, 27 Jun 2012 18:07:24 +0200, Jean Delvare wrote:
>> On Wed, 27 Jun 2012 21:54:10 +0800, Daniel Kurtz wrote:
>> > Per ICH10 datasheet [1] pg. 711, after completing a block transaction,
>> > INTR should be checked & cleared separately, only after BYTE_DONE is
>> > first cleared:
>> >
>> >   When the last byte of a block message is received, the host controller
>> > sets DS. However, it does not set the INTR bit (and generate another
>> > interrupt) until DS is cleared. Thus, for a block message of n bytes,
>> > the ICH10 will generate n+1 interrupts.
>> >
>> > [1] http://www.intel.com/content/dam/doc/datasheet/io-controller-hub-10-family-datasheet.pdf
>> >
>> > Currently, the INTR bit was only checked & cleared separately if the PEC
>> > was used.
>> > This patch checks and clears INTR at the very end of every successful
>> > transaction, regardless of whether the PEC is used.
>> >
>> > Signed-off-by: Daniel Kurtz <djkurtz@...omium.org>
>> > ---
>> >  drivers/i2c/busses/i2c-i801.c |   46 ++++++++++++++++++++--------------------
>> >  1 files changed, 23 insertions(+), 23 deletions(-)
>> >
>> > diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
>> > index 8b74e1e..6a53338 100644
>> > --- a/drivers/i2c/busses/i2c-i801.c
>> > +++ b/drivers/i2c/busses/i2c-i801.c
>> > @@ -257,6 +257,24 @@ static int i801_check_post(struct i801_priv *priv, int status, int timeout)
>> >     return result;
>> >  }
>> >
>> > +/* wait for INTR bit as advised by Intel */
>> > +static void i801_wait_intr(struct i801_priv *priv)
>> > +{
>> > +   int timeout = 0;
>> > +   int status;
>> > +
>> > +   status = inb_p(SMBHSTSTS(priv));
>> > +   while ((!(status & SMBHSTSTS_INTR)) && (timeout++ < MAX_RETRIES)) {
>> > +           usleep_range(250, 500);
>> > +           status = inb_p(SMBHSTSTS(priv));
>> > +   }
>>
>> Per my comment on previous patch, I've swapped the logic here to be in
>> line with what we had before. I have no objection to trying this change
>> again, but later, and only if you have actual numbers to back it up.
>
> I've done some performance measurements, and it turns out that, with the
> version of this patch modified by me, all short transactions are twice
> as slow as before. This is because i801_transaction waits twice now:
> once for BUSY to be clear, and then again once for INTR to be set.

Does a fast sequence of such transactions actually take any longer? Or
just a single short transaction?

My understanding is that the INTR wait is really waiting for the
entire transaction to complete (ie., including i2c STOP condition),
not just the byte transfer phase.

By waiting here at the end of a transaction, we make sure the status
bits are actually clear before starting the next transaction.

-Dan

> I don't think this makes much sense, as both events normally happen at
> the same time. As a matter of fact, the original code did clear INTR at
> the end of i801_transaction without checking that it was set. Also, we
> call i801_check_post() _before_ i801_wait_intr(), which makes no sense
> IMHO. If INTR was really not set yet, then checking for errors was
> wrong, it was too early.
>
> I'm not even sure why we rely on the BUSY bit in the first place. It
> would seem easier to just wait for INTR.
>
> Thinking about it some more, the idea of function i801_wait_intr() is a
> little strange. Normally, you'd wait for INTR, then do what you have
> to, and lastly clear the INTR bit. Having a function which waits for
> INTR and clears it immediately seems wrong.
>
> --
> Jean Delvare
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/