| 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
| ||
|
Date: Wed, 2 May 2012 14:58:45 -0700
From: Bryan Freed <bfreed@...omium.org>
To: Bryan Freed <bfreed@...omium.org>,
Peter Huewe <peter.huewe@...ineon.com>,
Rajiv Andrade <srajiv@...ux.vnet.ibm.com>, tpmdd@...horst.net,
tpmdd-devel@...ts.sourceforge.net, linux-kernel@...r.kernel.org,
Olof Johansson <olof@...om.net>,
Luigi Semenzato <semenzato@...gle.com>
Subject: Re: [PATCH] CHROMIUM: tpm: tpm_tis_i2c: Lock the I2C adapter for a
sequence of requests.
Andi,
On Wed, May 2, 2012 at 1:18 PM, Andi Shyti <andi.shyti@...il.com> 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@...il.com> 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@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists