linux-kernel - Re: [PATCH] i2c: designware: do not disable adapter after transfer

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]

Date:	Mon, 2 May 2016 12:11:35 +0200
From:	Christian Ruppert <christian.ruppert@...tech.com>
To:	Lucas De Marchi <lucas.de.marchi@...il.com>,
	Lucas De Marchi <lucas.demarchi@...el.com>
Cc:	"linux-i2c@...r.kernel.org" <linux-i2c@...r.kernel.org>,
	"wsa@...-dreams.de" <wsa@...-dreams.de>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"mika.westerberg@...ux.intel.com" <mika.westerberg@...ux.intel.com>,
	"jarkko.nikula@...ux.intel.com" <jarkko.nikula@...ux.intel.com>
Subject: Re: [PATCH] i2c: designware: do not disable adapter after transfer

Dear Lucas,

On 22.04.2016 17:19, Lucas De Marchi wrote:
> CC'ing Christian.
> 
> On Fri, Apr 22, 2016 at 12:08 PM, Lucas De Marchi
> <lucas.demarchi@...el.com> wrote:
>> Disabling the adapter after each transfer is pretty bad for sensors and
>> other devices doing small transfers at a high rate. It slows down the
>> transfer rate a lot since each of them have to wait the adapter to be
>> enabled again.
>>
>> During the transfer init we check the status register for no activity
>> and TX buffer being empty since otherwise we can't change IC_TAR
>> dynamically.
>>
>> When a transfer fails the adapter will still be disabled - this is a
>> conservative approach. When transfers succeed, the adapter is left
>> enabled and it's configured so to disable interrupts.
> 
> Christian, this is the updated patch. Now adapter starts disabled and
> is disabled when there's a failed transfer. I hope this can work with
> your hardware.

Good news: The system now boots without deadlocks.

Bad  news: Not all transfers seem to take place as they should.
           I don't have the time for a deep analysis but a few quick
           experiments seem to indicate that the adapter needs to be
           disabled while updating TAR to a value different from the
           previous one. Disabling the adapter does not seem to be
           required if TAR doesn't change from one transfer to the next.
           I don't know if there are any conditions under which we can
           leave the adapter enabled while changing TAR but at least in
           some cases it seems to work.

The adapter seems to work reliably with the following diff on top of
your patch (included a dirty version of Jarkko's comments as well, not
sure if that's required to make everything work):

diff --git a/drivers/i2c/busses/i2c-designware-core.c
b/drivers/i2c/busses/i2c-designware-core.c
index 8a08e68..e927285 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -421,8 +421,8 @@ static void i2c_dw_xfer_init(struct dw_i2c_dev *dev)

                /* check ic_tar and ic_con can be dynamically updated */
                ic_status = dw_readl(dev, DW_IC_STATUS);
-               if (ic_status & DW_IC_STATUS_ACTIVITY
-                       || !(ic_status & DW_IC_STATUS_TX_EMPTY)) {
+               if (ic_status & (0x1<<5)
+                       || !(ic_status & (0x1<<1))) {
                        __i2c_dw_enable(dev, false);
                }
        }
@@ -442,13 +442,18 @@ static void i2c_dw_xfer_init(struct dw_i2c_dev *dev)
                ic_con &= ~DW_IC_CON_10BITADDR_MASTER;
        }

+       ic_tar |= msgs[dev->msg_write_idx].addr;
+
        dw_writel(dev, ic_con, DW_IC_CON);

        /*
         * Set the slave (target) address and enable 10-bit addressing mode
         * if applicable.
         */
-       dw_writel(dev, msgs[dev->msg_write_idx].addr | ic_tar, DW_IC_TAR);
+       if (ic_tar != dw_readl(dev, DW_IC_TAR))
+               __i2c_dw_enable(dev, false);
+
+       dw_writel(dev, ic_tar, DW_IC_TAR);

        /* enforce disabled interrupts (due to HW issues) */
        i2c_dw_disable_int(dev);