lkml.org 
[lkml]   [2012]   [May]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    SubjectRe: [PATCH] CHROMIUM: tpm: tpm_tis_i2c: Lock the I2C adapter for a sequence of requests.
    From
    Andi,

    On Wed, May 2, 2012 at 1:18 PM, Andi Shyti <andi.shyti@gmail.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@gmail.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@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2012-05-03 00:21    [W:0.032 / U:64.640 seconds]
    ©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site