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+omCoto4djKuZUooeD2A-szjrH4e9=YJCptR_raOdQ8g3-A@mail.gmail.com>
Date:	Thu, 28 Jun 2012 15:51:34 +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 Thu, Jun 28, 2012 at 12:07 AM, Jean Delvare <khali@...ux-fr.org> 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.
>
>> +
>> +     if (timeout > MAX_RETRIES)
>> +             dev_dbg(&priv->pci_dev->dev, "INTR Timeout!\n");
>> +
>> +     outb_p(status, SMBHSTSTS(priv));
>
> Wouldn't it be more correct to only write the INTR bit? Writing back
> the whole 8 bits frightens me a little especially because of bit
> INUSE_STS. If we ever want to support this feature, I think we have to
> first ban writing back the whole status to register HST_STS. And this
> is the only place where we still do AFAICS.

It looks like the current code does it this way to clear any error
bits that may be set in the timeout case (errors set, but no INTR).
For example, if there was a bus / arbitration error while waiting for
PEC.

Perhaps we can split the difference (I tested this and it has no
obvious regression):

+     /* Clear INTR, or in case of timeout, any other status bits. */
+     outb_p(status & STATUS_FLAGS, SMBHSTSTS(priv));

But in a separate patch...

>
> (This isn't a regression from your patch, the old code was already
> doing that, but it might be the opportunity to fix it.)
>
> --
> 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/