Re: [PATCH] CHROMIUM: tpm: tpm_tis_i2c: Lock the I2C adapter for asequence of requests.

[Bryan Freed]

Andi,

On Wed, May 2, 2012 at 1:18 PM, Andi Shyti <andi.shyti@xxxxxxxxx> wrote:
> Hi,
>
> On Wed, May 02, 2012 at 12:06:14PM -0700, Bryan Freed wrote:
>> Hi Andi,
>>
>> On Wed, May 2, 2012 at 11:14 AM, Andi Shyti <andi.shyti@xxxxxxxxx> wrote:
>> > Hi Bryan,
>> >
>> >> > try to have a look to the i2c_smbus* function, you could avoid
>> >> > lot of code
>> > On Wed, May 02, 2012 at 10:25:09AM -0700, Bryan Freed wrote:
>> >> (Sorry for resending...)
>> >>
>> >> Andi, it is not clear what i2c_smbus_* functions in particular will
>> >> improve upon this change.
>> >>
>> >> All i2c_smbus_* functions go through i2c_smbus_xfer() which at some
>> >> point will i2c_lock_adapter() for each request.
>> >> This is true for adapters that support SMBUS (where the lock occurs
>> >> directly in i2c_smbus_xfer()) or just I2C (where the lock occurs in
>> >> i2c_transfer() called through i2c_smbus_xfer_emulated()).
>> >>
>> >> The goal of this change is for the tpm_tis_i2c driver to be able to
>> >> lock an adapter over the duration of several I2C requests.
>> >> Presumably, that is why i2c_lock_adapter() is exported.
>> >
>> > the i2c_smbus_* functions will not improve anything to the
>> > driver, it will increase the readability and it will allow you to
>> > do the same stuff with less code.
>>
>> I think I see what you are saying.
>> We could (in the unmodified version of this driver) replace all the
>> iic_tpm_read() calls of one byte (which sends an address byte before
>> reading a data byte) with an i2c_smbus_read_byte_command() call which
>> does the same thing.
>> Switching to the SMBUS calls in this driver will still work on
>> adapters that only support I2C because of i2c_smbus_xfer_emulated().
>> Right?
>> > All the i2c communication implementation you wrote here, is
>> > already written in the i2c-core.c file.
>>
>> Right.  Unfortunately, we cannot use any of the i2c_smbus_* functions
>> in this driver.  The device will fail because the adapter lock is not
>> held long enough to prevent I2C traffic going to other devices on the
>> same bus.  That other traffic to other devices confuses the i2c core
>> in this device.  Our only driver solution is to lock the adapter for a
>> longer duration.
>>
>> Yes, the code we have here is copied from the i2c-core.c file.  In
>> fact, we comment this with, "Copy i2c-core:i2c_transfer() as close as
>> possible without the adapter locks
>> and algorithm check".
>>
>> And that really is the problem with using the i2c-core.c calls.  This
>> driver needs to lock the adapter for an extended duration.
>
> mmhhh... you still haven't convinced me. I always thought that
> every dublicated code is useless.

Hey, I agree with you on that point.  Duplicated code has its own problems.
A better solution would require i2c-core.c mods, mainly:

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index e9c1893..64cb9c2 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -1346,17 +1346,8 @@ int i2c_transfer(struct i2c_adapter *adap,
struct i2c_msg *msgs, int num)
                        i2c_lock_adapter(adap);
                }

-               /* Retry automatically on arbitration loss */
-               orig_jiffies = jiffies;
-               for (ret = 0, try = 0; try <= adap->retries; try++) {
-                       ret = adap->algo->master_xfer(adap, msgs, num);
-                       if (ret != -EAGAIN)
-                               break;
-                       if (time_after(jiffies, orig_jiffies + adap->timeout))
-                               break;
-               }
+               ret = i2c_transfer_nolock(&adap-, msgs, num);
                i2c_unlock_adapter(adap);
-
                return ret;
        } else {
                dev_dbg(&adap->dev, "I2C level transfers not supported\n");
@@ -1365,6 +1356,25 @@ int i2c_transfer(struct i2c_adapter *adap,
struct i2c_msg *msgs, int num)
 }
 EXPORT_SYMBOL(i2c_transfer);

+int i2c_transfer_nolock(struct i2c_adapter *adap, struct i2c_msg
*msgs, int num)
+{
+       unsigned long orig_jiffies;
+       int ret, try;
+
+       /* Retry automatically on arbitration loss */
+       orig_jiffies = jiffies;
+       for (ret = 0, try = 0; try <= adap->retries; try++) {
+               ret = adap->algo->master_xfer(adap, msgs, num);
+               if (ret != -EAGAIN)
+                       break;
+               if (time_after(jiffies, orig_jiffies + adap->timeout))
+                       break;
+       }
+
+       return ret;
+}
+EXPORT_SYMBOL(i2c_transfer_nolock);
+

Then I would not need my own copy of i2c_transfer_nolock().
But making these ugly changes in the driver for one buggy device is
easier/safer than making it in the general I2C code.

bryan.

> You may have good reasons to do that, but I could still try to
> find out a way on how to simplify it.
>
> Thanks for the explanation,
> Andi
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/