Re: [PATCH] fix i801_transaction() and ioctl return values for i2c-i801

Re: [PATCH] fix i801_transaction() and ioctl return values for i2c-i801

[Jean Delvare]

Hi Amit,

On Fri, 6 Jun 2008 12:44:47 +0100, Amit Walambe wrote:
> Hello Mark and Jean!

Mark is no longer involved in kernel driver development. I am adding
the i2c mailing list in Cc instead, in case others developers or users
are interested.

> We have a Pentium M board which was running into i2c issues at a
> customer site. They extensively use i2c to manipulate LEDs on the front
> panel of the enclosure. 
> We use i2c-regs utility and some times the read and write operations
> were failing with the following message : 
> write_smbus_register: Operation not permitted
> read_smbus_register: Operation not permitted
> (Attaching the script used to reproduce the failures and i2c-regs
> utility source code)

Out of curiosity, why are you using this proprietary utility instead of
the standard i2cget and i2cset programs?

> This is the relevant line from the lspci output : 
> 00:1f.3 SMBus: Intel Corp. 82801DB/DBL/DBM (ICH4/ICH4-L/ICH4-M) SMBus
> Controller (rev 02)

Which kernel is your customer using? There have been a number of
improvements done to the i2c-i801 driver over the last year. And what
motherboard is this? Please provide the output of i2cdetect for this
bus.

Is there a second master on the SMBus on the customer's system?


> Enabling debug in kernel produced following messages : 
> i801_smbus 0000:00:1f.3: Failed reset at end of transaction(01)
> i801_smbus 0000:00:1f.3: SMBus busy (01). Resetting... 
> i801_smbus 0000:00:1f.3: Failed! (01)
> i801_smbus 0000:00:1f.3: SMBus busy (01). Resetting... 
> i801_smbus 0000:00:1f.3: Failed! (01)
> i801_smbus 0000:00:1f.3: SMBus busy (02). Resetting... 
> i801_smbus 0000:00:1f.3: Successfull!
> (I commented out 'Transaction ([pre]/[post])' debug prints as they were
> creating a little too much data)

I agree that these pre/post debug messages are very noisy and not
necessarily very interesting now that the driver development is done.
I'll post a patch removing them.

> The fix involved was to write a loop to wait for the final reset to go
> through. After that, we still see following messages, but atleast it is
> not failing over : 
> i801_smbus 0000:00:1f.3: SMBus busy (02).Resetting... 
> i801_smbus 0000:00:1f.3: Successfull! 
> i801_smbus 0000:00:1f.3: SMBus busy (02). Resetting... 
> i801_smbus 0000:00:1f.3: Successfull!

Comparing this to the log above, I don't see any change. Resetting when
register value was 02 was already working. It's resetting when register
value was 01 that was failing, so that's what you need to check with
the patch applied.

> Secondly, the i2c-regs utility was failing with
> 'Operation not permitted' error, which was a bit misleading. It happens
> because i801_transaction returns -1 for all the failures and that value
> is taken up to the user level. So I have changed the return values to
> suit the cause better. This is also included in the same patch. 

We already have a fix pending for this problem:
http://khali.linux-fr.org/devel/linux-2.6/jdelvare-i2c/i2c-adapters-return-proper-error-codes.patch
It will be merged in kernel 2.6.27.

> Please let me know what you think.

First of all: please note that the kernel code uses tabs for
indentation, while your patch uses 4 spaces. You'll have to fix your
patch. Also remove the change of error codes, as we already have a
patch fixing them.

I am very worried by the code 01 which triggers the reset (which is
probably not the right thing to do, BTW.) The host should never be busy
when we start a transaction. If the problem happens frequently, this
strongly suggests that some other code is making use of the host
controller. This could for example be ACPI code. Please provide the
DSDT for this machine (in private.)

If something else is making use of the SMBus host then the i2c-i801
driver can't use it safely, no matter how you look at it. So my feeling
is that your patch, if it does anything, is not actually solving the
problem but only hiding it or making it less likely to happen.

-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [PATCH] fix i801_transaction() and ioctl return values for i2c-i801

[Amit Walambe]

Hi!
Over the weekend, I have investigated a bit more and the problem doesn't
appear on 2.6.23 from Fedora 8. So, this discussion holds value only if
anyone thinks the problem is a function of latencies (due to 
responsiveness of the controller and/or other factors) and can surface 
some other time. Otherwise, put a return 0 right here :). 

I still think the patch is applicable even to the latest kernels. Please 
bear with me for a bit longish reply.

On Sun, 8 Jun 2008 20:18:28 +0200
"Jean Delvare" <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
> Out of curiosity, why are you using this proprietary utility instead
> of the standard i2cget and i2cset programs?
> 
> > This is the relevant line from the lspci output : 
> > 00:1f.3 SMBus: Intel Corp. 82801DB/DBL/DBM (ICH4/ICH4-L/ICH4-M)
> > SMBus Controller (rev 02)
> 
> Which kernel is your customer using? There have been a number of
> improvements done to the i2c-i801 driver over the last year. And what
> motherboard is this? Please provide the output of i2cdetect for this
> bus.
The board is a SBC named Apollo from Eurotech. Kernel is 2.6.9 from
Fedora 3. (At first, I looked through the latest kernel code to see if
I can backport any fixes, but there wasn't anything relevant to the
problem at hand.) 

i2regs is still around because 'if it works, don't change it' thing.
Also, it has 'mask' functionality which some people use. We can put
masks into standard utilities if you wish to do so.

Here is the i2cdetect output : 
#./i2cdetect -l
i2c-0   smbus           SMBus I801 adapter at 1880              SMBus
adapter

> Is there a second master on the SMBus on the customer's system?
I think you are pointing to the i801 errattum about the second host on
the same bus. There is a I2C master in ethernet device (for remote
monitoring etc.) on the same bus but it is disabled in the EPROM
itself. So it can't generate any traffic. Just to be safe about this,
we had protocol analyser running on the system and there was no other
i2c activities going on except that by the script.

> > Enabling debug in kernel produced following messages : 
> > i801_smbus 0000:00:1f.3: Failed reset at end of transaction(01)
> > i801_smbus 0000:00:1f.3: SMBus busy (01). Resetting... 
> > i801_smbus 0000:00:1f.3: Failed! (01)
> > i801_smbus 0000:00:1f.3: SMBus busy (01). Resetting... 
> > i801_smbus 0000:00:1f.3: Failed! (01)
> > i801_smbus 0000:00:1f.3: SMBus busy (02). Resetting... 
> > i801_smbus 0000:00:1f.3: Successfull!
> > The fix involved was to write a loop to wait for the final reset to
> > go through. After that, we still see following messages, but
> > atleast it is not failing over : 
> > i801_smbus 0000:00:1f.3: SMBus busy (02).Resetting... 
> > i801_smbus 0000:00:1f.3: Successfull! 
> > i801_smbus 0000:00:1f.3: SMBus busy (02). Resetting... 
> > i801_smbus 0000:00:1f.3: Successfull!
> 
> Comparing this to the log above, I don't see any change. Resetting
> when register value was 02 was already working. It's resetting when
> register value was 01 that was failing, so that's what you need to
> check with the patch applied.
I think the problem was that resetting after a transaction was taking a
bit longer and as we are doing heavy read/writes, the next request
would arrive, only to find the host controller busy.
The patch only waits till the operation finishes, so that no subsequent
transaction gets 01 (busy) error.
I guess the above log shows that the scenario is taken care by the
patch. The second condition (02 : INTR bit set : transaction complete)
is seen by the next transaction rarely and is a status message, not
failure.

> > happens because i801_transaction returns -1 for all the failures
> > and that value is taken up to the user level. So I have changed the
> > return values to suit the cause better. This is also included in
> > the same patch. 
> 
> We already have a fix pending for this problem:
> http://khali.linux-fr.org/devel/linux-2.6/jdelvare-i2c/i2c-adapters-return-proper-error-codes.patch
> It will be merged in kernel 2.6.27.
I missed the boat then :)

> First of all: please note that the kernel code uses tabs for
> indentation, while your patch uses 4 spaces. You'll have to fix your
> patch.
I am aware of this. I use vi with 'set tabstop=4' and obtained patch
using 'git diff > i2c-i801.patch'. Mail client (claws-mail) couldn't
have messed it up as I used the patch as attachment (and not inline as
it is expected).
I am not sure where it went wrong, will check up. Sorry about that.

> I am very worried by the code 01 which triggers the reset (which is
> probably not the right thing to do, BTW.) The host should never be
> busy when we start a transaction. If the problem happens frequently,
I think we have a case where it is busy because the final operation
(resetting the status bits) in the last transaction didn't finish. 
I first tried to reduce reads/writes for the reset to make it "less
busy" and added a small delay, but it didn't work : 
diff --git a/drivers/i2c/busses/i2c-i801.c
b/drivers/i2c/busses/i2c-i801.c index b0f771f..0dde673 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -195,9 +195,10 @@ static int i801_transaction(int xact)
                dev_dbg(&I801_dev->dev, "Error: no response!\n");
        }
 
-       if ((inb_p(SMBHSTSTS) & 0x1f) != 0x00)
-               outb_p(inb(SMBHSTSTS), SMBHSTSTS);
+       if ((temp = (inb_p(SMBHSTSTS) & 0x1f)) != 0x00)
+               outb_p(temp, SMBHSTSTS);
 
+       msleep(1);
        if ((temp = (0x1f & inb_p(SMBHSTSTS))) != 0x00) {
                dev_dbg(&I801_dev->dev, "Failed reset at end of
transaction " "(%02x)\n", temp);

> this strongly suggests that some other code is making use of the host
> controller. This could for example be ACPI code. Please provide the
> DSDT for this machine (in private.)
I can assure you that there are no other transactions on the bus. We
verified using an analyser.

> If something else is making use of the SMBus host then the i2c-i801
> driver can't use it safely, no matter how you look at it. So my
> feeling is that your patch, if it does anything, is not actually
> solving the problem but only hiding it or making it less likely to
> happen.
I think I didn't provide sufficient information last time. Apologies.
Please let me know what you think now. This is the patch which I ran
over the weekend without any failures : 
diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index b0f771f..d165829 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -198,9 +198,17 @@ static int i801_transaction(int xact)
        if ((inb_p(SMBHSTSTS) & 0x1f) != 0x00)
                outb_p(inb(SMBHSTSTS), SMBHSTSTS);
 
-       if ((temp = (0x1f & inb_p(SMBHSTSTS))) != 0x00) {
-               dev_dbg(&I801_dev->dev, "Failed reset at end of transaction "
-                       "(%02x)\n", temp);
+       /* We wait for a fraction of a second! */
+       do {
+               msleep(1);
+               temp = inb_p(SMBHSTSTS);
+       } while ((temp & 0x01) && (timeout++ < MAX_TIMEOUT));
+
+       /* If the SMBus is still busy, we give up */
+       if (timeout >= MAX_TIMEOUT) {
+               result = -EBUSY;
+               dev_dbg(&I801_dev->dev, "Failed reset at end of transaction"
+                               "(%02x)\n", temp);
        }
        dev_dbg(&I801_dev->dev, "Transaction (post): CNT=%02x, CMD=%02x, "
                "ADD=%02x, DAT0=%02x, DAT1=%02x\n", inb_p(SMBHSTCNT),

Thanks and regards

-- 
Amit Walambe
Design Engineer

Eurotech Ltd,
3 Clifton Court,
Cambridge CB1 7BN,
United Kingdom.

Tel: +44 (0)1223 403465 Direct
Tel: +44 (0)1223 411200 Switchboard
Fax: +44 (0)1223 410457
E-Mail: amit.walambe-ymFC7zkAqBLaL3YF3Y4TSbVCufUGDwFn@public.gmane.org
Web: http://www.eurotech-ltd.co.uk

Eurotech Ltd is a subsidiary of Eurotech S.p.A
VAT No. GB 314961067    Registered in England 1608562
Registered Office: 3 Clifton Court, Clifton Road, Cambridge CB1 7BN  UK

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [PATCH] fix i801_transaction() and ioctl return values for i2c-i801

[Jean Delvare]

Hi Amit,

On Mon, 9 Jun 2008 11:56:56 +0100, Amit Walambe wrote:
> Over the weekend, I have investigated a bit more and the problem doesn't
> appear on 2.6.23 from Fedora 8. So, this discussion holds value only if
> anyone thinks the problem is a function of latencies (due to 
> responsiveness of the controller and/or other factors) and can surface 
> some other time. Otherwise, put a return 0 right here :). 
> 
> I still think the patch is applicable even to the latest kernels. Please 
> bear with me for a bit longish reply.

I don't understand. You just said yourself that the problem didn't show
on a 2.6.23 kernel, meaning that we somehow fixed the bug already, but
still you want your patch to be applied?

> On Sun, 8 Jun 2008 20:18:28 +0200
> "Jean Delvare" <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
> > Out of curiosity, why are you using this proprietary utility instead
> > of the standard i2cget and i2cset programs?
> > 
> > > This is the relevant line from the lspci output : 
> > > 00:1f.3 SMBus: Intel Corp. 82801DB/DBL/DBM (ICH4/ICH4-L/ICH4-M)
> > > SMBus Controller (rev 02)
> > 
> > Which kernel is your customer using? There have been a number of
> > improvements done to the i2c-i801 driver over the last year. And what
> > motherboard is this? Please provide the output of i2cdetect for this
> > bus.
> The board is a SBC named Apollo from Eurotech. Kernel is 2.6.9 from
> Fedora 3. (At first, I looked through the latest kernel code to see if
> I can backport any fixes, but there wasn't anything relevant to the
> problem at hand.)

2.6.9 is very old. There have been so many changes made to the i2c-i801
driver since then that it's quite difficult to make sure there's no
relevant change, unless you're intimate with this driver and device.
For example, this patch:
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=ca8b9e32a11a7cbfecbef00c8451a79fe1af392e
seems pretty relevant to me, as it attempts to cleanly stop the
controller on transaction timeout. It went into kernel 2.6.23.

> i2regs is still around because 'if it works, don't change it' thing.
> Also, it has 'mask' functionality which some people use. We can put
> masks into standard utilities if you wish to do so.

i2cset supports this functionality since October 2004.

> Here is the i2cdetect output : 
> #./i2cdetect -l
> i2c-0   smbus           SMBus I801 adapter at 1880              SMBus adapter

I asked for the output of i2cdetect (i2cdetect 0 in your case), not
i2cdetect -l. I wanted to have an idea what chips were present on the
bus.

> > > Enabling debug in kernel produced following messages : 
> > > i801_smbus 0000:00:1f.3: Failed reset at end of transaction(01)
> > > i801_smbus 0000:00:1f.3: SMBus busy (01). Resetting... 
> > > i801_smbus 0000:00:1f.3: Failed! (01)
> > > i801_smbus 0000:00:1f.3: SMBus busy (01). Resetting... 
> > > i801_smbus 0000:00:1f.3: Failed! (01)
> > > i801_smbus 0000:00:1f.3: SMBus busy (02). Resetting... 
> > > i801_smbus 0000:00:1f.3: Successfull!
> > > The fix involved was to write a loop to wait for the final reset to
> > > go through. After that, we still see following messages, but
> > > atleast it is not failing over : 
> > > i801_smbus 0000:00:1f.3: SMBus busy (02).Resetting... 
> > > i801_smbus 0000:00:1f.3: Successfull! 
> > > i801_smbus 0000:00:1f.3: SMBus busy (02). Resetting... 
> > > i801_smbus 0000:00:1f.3: Successfull!
> > 
> > Comparing this to the log above, I don't see any change. Resetting
> > when register value was 02 was already working. It's resetting when
> > register value was 01 that was failing, so that's what you need to
> > check with the patch applied.
>
> I think the problem was that resetting after a transaction was taking a
> bit longer and as we are doing heavy read/writes, the next request
> would arrive, only to find the host controller busy.

Please note that "resetting" is not really the correct term for what
the driver is doing. We should fix this debug message to avoid the
confusion. The driver is merely clearing status bits, it's not actually
resetting anything. At least it wasn't in 2.6.9. Since 2.6.23 it kills
the stuck transactions, which is kind of a reset, but that's a
different piece of code than the part you wanted to modify.

One important thing to notice is that the driver is still doing
something wrong in this area. If the host is found busy (bit 0 of the
status register) the driver attempts to write 1 to that bit. Given that
the bit in question is read-only, I'd say the chances that it works are
thin... If the host is busy then this strongly suggests that somebody
else is using it and we should abort immediately.

> The patch only waits till the operation finishes, so that no subsequent
> transaction gets 01 (busy) error.

As written above, 01 can't be cleared by the driver with the code in
2.6.9. So your patch is waiting for the bit to clear (which makes some
sense) but the "reset" operation is not responsible for this. The bit
would have cleared itself after some time anyway. The "reset" operation
as it exists in 2.6.9 is merely a bit clearing operation, as I wrote
above, and this doesn't take time to complete. If the next check shows
that any cleared status bit is up (other than 01), it's rather "up
again" than "still up", I think.

> I guess the above log shows that the scenario is taken care by the
> patch. The second condition (02 : INTR bit set : transaction complete)
> is seen by the next transaction rarely and is a status message, not
> failure.

Ah, OK, I get the idea now.

> > I am very worried by the code 01 which triggers the reset (which is
> > probably not the right thing to do, BTW.) The host should never be
> > busy when we start a transaction. If the problem happens frequently,
>
> I think we have a case where it is busy because the final operation
> (resetting the status bits) in the last transaction didn't finish. 

Then I believe that Oleg Ryjkov's patch (URL above) is the proper fix
for your problem.

> I first tried to reduce reads/writes for the reset to make it "less
> busy" and added a small delay, but it didn't work : 
> diff --git a/drivers/i2c/busses/i2c-i801.c
> b/drivers/i2c/busses/i2c-i801.c index b0f771f..0dde673 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -195,9 +195,10 @@ static int i801_transaction(int xact)
>                 dev_dbg(&I801_dev->dev, "Error: no response!\n");
>         }
>  
> -       if ((inb_p(SMBHSTSTS) & 0x1f) != 0x00)
> -               outb_p(inb(SMBHSTSTS), SMBHSTSTS);
> +       if ((temp = (inb_p(SMBHSTSTS) & 0x1f)) != 0x00)
> +               outb_p(temp, SMBHSTSTS);
>  
> +       msleep(1);
>         if ((temp = (0x1f & inb_p(SMBHSTSTS))) != 0x00) {
>                 dev_dbg(&I801_dev->dev, "Failed reset at end of transaction "
>                         "(%02x)\n", temp);
> 
> > this strongly suggests that some other code is making use of the host
> > controller. This could for example be ACPI code. Please provide the
> > DSDT for this machine (in private.)
>
> I can assure you that there are no other transactions on the bus. We
> verified using an analyser.

OK, good thing. Then indeed the reason would be improperly terminated
transactions affecting the next transaction, as you suggested.

> Please let me know what you think now. This is the patch which I ran
> over the weekend without any failures : 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index b0f771f..d165829 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -198,9 +198,17 @@ static int i801_transaction(int xact)
>         if ((inb_p(SMBHSTSTS) & 0x1f) != 0x00)
>                 outb_p(inb(SMBHSTSTS), SMBHSTSTS);
>  
> -       if ((temp = (0x1f & inb_p(SMBHSTSTS))) != 0x00) {
> -               dev_dbg(&I801_dev->dev, "Failed reset at end of transaction "
> -                       "(%02x)\n", temp);
> +       /* We wait for a fraction of a second! */
> +       do {
> +               msleep(1);
> +               temp = inb_p(SMBHSTSTS);
> +       } while ((temp & 0x01) && (timeout++ < MAX_TIMEOUT));
> +
> +       /* If the SMBus is still busy, we give up */
> +       if (timeout >= MAX_TIMEOUT) {
> +               result = -EBUSY;
> +               dev_dbg(&I801_dev->dev, "Failed reset at end of transaction"
> +                               "(%02x)\n", temp);
>         }
>         dev_dbg(&I801_dev->dev, "Transaction (post): CNT=%02x, CMD=%02x, "
>                 "ADD=%02x, DAT0=%02x, DAT1=%02x\n", inb_p(SMBHSTCNT),

As written above, I think your problem is already fixed in 2.6.23 so no
patch is needed. I wouldn't want your patch as is anyway, as it adds a
sleep unconditionally, making the driver twice as slow as the original
code.

One thing that might still be worth investigating is why some
transactions do not complete in the first place.

This might be related to the fact that the timeout loop was written
with HZ=100 in mind and is broken by design. It timeouts in count (100,
where all but one drivers have 500), not time. Depending on various
parameters (amongst which the value of HZ), the actual time it waits
for is very different. At HZ=100 it was typically 2 seconds, but at
HZ=1000 it would be 200 ms, which isn't that much. Although at 16 kHz,
a 32-byte I2C block read transaction would take 20 ms if I count
correctly, so it still fits.

Could it be that some slave on the customer's I2C bus is misbehaving
and holding the clock line low for longer than it should?

-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [PATCH] fix i801_transaction() and ioctl return values for i2c-i801

[Amit Walambe]

Jean Delvare <khali@...> writes:

> 
> Hi Amit,
Hi Jean!
I didn't receive this reply in my inbox. Otherwise I wouldn't have sent my
corrected patch. Sorry about that.

> I don't understand. You just said yourself that the problem didn't show
> on a 2.6.23 kernel, meaning that we somehow fixed the bug already, but
> still you want your patch to be applied?
My patch only waits till the busy bit clears. As you have said in your reply, it
adds unconditional delay of atleast 1 ms, but it avoided a few subsequent user
requests from failing.
I agree this happened on a very old kernel with vastly different behaviour, but
there is nothing in the current code to prevent it from happening. Still, I make
it your call if too cautious (but slow) patch is a good idea or not.
Please let me know if you think.

> For example, this patch:
>
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=ca8b9e32a11a7cbfecbef00c8451a79fe1af392e
> seems pretty relevant to me, as it attempts to cleanly stop the
> controller on transaction timeout. It went into kernel 2.6.23.
This patch is useful to handle the failure that has occured (a new request fails
since controller is still busy with previous operation). But it doesn't avoid
this situation.

> i2cset supports this functionality since October 2004.
Apologies.

> I asked for the output of i2cdetect (i2cdetect 0 in your case), not
> i2cdetect -l. I wanted to have an idea what chips were present on the
> bus.
     0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f
00:          -- -- -- -- -- 08 -- -- -- -- -- -- -- 
10: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 
20: 20 -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 
30: 30 -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 
40: -- -- -- -- 44 -- -- -- 48 -- -- -- -- -- -- -- 
50: 50 -- -- -- 54 -- -- -- -- -- -- -- -- -- -- -- 
60: -- -- -- -- -- -- -- -- -- 69 6a -- -- -- -- -- 
70: -- -- -- -- -- -- -- --                         

> Please note that "resetting" is not really the correct term for what
I used the "resetting the status bits" at some other places in the my email.
Shouldn't have used it interchangebly.

> One important thing to notice is that the driver is still doing
> something wrong in this area. If the host is found busy (bit 0 of the
> status register) the driver attempts to write 1 to that bit. Given that
> the bit in question is read-only, I'd say the chances that it works are
> thin... 
Ok, so the bit manipulation part is useless and wait part is only relevant.

> One thing that might still be worth investigating is why some
> transactions do not complete in the first place.
> 
> This might be related to the fact that the timeout loop was written
> with HZ=100 in mind and is broken by design. It timeouts in count (100,
> where all but one drivers have 500), not time. Depending on various
> parameters (amongst which the value of HZ), the actual time it waits
> for is very different. At HZ=100 it was typically 2 seconds, but at
> HZ=1000 it would be 200 ms, which isn't that much. Although at 16 kHz,
> a 32-byte I2C block read transaction would take 20 ms if I count
> correctly, so it still fits.
> 
> Could it be that some slave on the customer's I2C bus is misbehaving
> and holding the clock line low for longer than it should?
I will investigate more on this line. 

Thanks a lot for your help. 
- Amit


_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [PATCH] fix i801_transaction() and ioctl return values for i2c-i801

[Jean Delvare]

Hi Amit,

Sorry for the late reply, I wanted to have some code to show before
replying and it took longer to write than expected.

On Mon, 9 Jun 2008 15:27:50 +0000 (UTC), Amit Walambe wrote:
> > I don't understand. You just said yourself that the problem didn't show
> > on a 2.6.23 kernel, meaning that we somehow fixed the bug already, but
> > still you want your patch to be applied?
> My patch only waits till the busy bit clears. As you have said in your reply, it
> adds unconditional delay of atleast 1 ms, but it avoided a few subsequent user
> requests from failing.
> I agree this happened on a very old kernel with vastly different behaviour, but
> there is nothing in the current code to prevent it from happening. Still, I make
> it your call if too cautious (but slow) patch is a good idea or not.
> Please let me know if you think.

The slowdown caused by your patch is clearly not acceptable. It would
be very easy to rewrite it to only delay as necessary. But the reason
why I dislike your patch goes beyond this: I think it's broken by
design. I can't see how it is different from simply changing the value
of MAX_TIMEOUT from 100 to 200 as far as delay is concerned. And
increasing MAX_TIMEOUT to 200 would have the added benefit that the
first transaction would even succeed instead of failing (if the issue
is that the transaction takes more time than the driver expects.)

Have you tried increasing MAX_TIMEOUT to 200 or even 500, to see if it
would solve your problem?

> > For example, this patch:
> >
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=ca8b9e32a11a7cbfecbef00c8451a79fe1af392e
> > seems pretty relevant to me, as it attempts to cleanly stop the
> > controller on transaction timeout. It went into kernel 2.6.23.
> This patch is useful to handle the failure that has occured (a new request fails
> since controller is still busy with previous operation). But it doesn't avoid
> this situation.

It is supposed to address exactly the problem you reported, by not
leaving the controller in a busy state at the end of a "failing"
transaction (where "failing" can mean "takes too long" too). Whether
this actually works or not, is another story... I gave it a try today
on an ICH5-based system, and the only error case I could test (SMBus
block read with invalid size) was not recovered properly. The code
looks good though, so maybe it's a hardware issue. I would really like
to know if it works in your case.

> > I asked for the output of i2cdetect (i2cdetect 0 in your case), not
> > i2cdetect -l. I wanted to have an idea what chips were present on the
> > bus.
>      0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f
> 00:          -- -- -- -- -- 08 -- -- -- -- -- -- -- 
> 10: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 
> 20: 20 -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 
> 30: 30 -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 
> 40: -- -- -- -- 44 -- -- -- 48 -- -- -- -- -- -- -- 
> 50: 50 -- -- -- 54 -- -- -- -- -- -- -- -- -- -- -- 
> 60: -- -- -- -- -- -- -- -- -- 69 6a -- -- -- -- -- 
> 70: -- -- -- -- -- -- -- --                         

I/O, EEPROM, thermal sensor, clock chips... lot of stuff on this bus!

> > One important thing to notice is that the driver is still doing
> > something wrong in this area. If the host is found busy (bit 0 of the
> > status register) the driver attempts to write 1 to that bit. Given that
> > the bit in question is read-only, I'd say the chances that it works are
> > thin... 
> Ok, so the bit manipulation part is useless and wait part is only relevant.

Correct.

As you may have seen already, I've just sent the i2c-i801 patches I've
been working on. The 4th one:
  http://lists.lm-sensors.org/pipermail/i2c/2008-June/004070.html
fixes the handling of the HOST_BUSY bit, amongst other error handling
cleanups. It would be great if you could test it, although I understand
that it might be difficult, or even impossible, if your customer has to
stick to kernel 2.6.9. But one thing for sure, is that I won't take
your patch, unless you test both kernel 2.6.25 and Linus' latest with
my patches applied, and both keep failing in your situation.

-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [PATCH] fix i801_transaction() and ioctl return values for i2c-i801

[Amit Walambe]

Hi Jean!
Thanks a ton for your efforts! I will try my best to test these patches
and get back to you.
Thanks again.
- Amit

On Sat, 14 Jun 2008 22:26:35 +0200
"Jean Delvare" <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:

> Hi Amit,
> 
> Sorry for the late reply, I wanted to have some code to show before
> replying and it took longer to write than expected.
> 
> On Mon, 9 Jun 2008 15:27:50 +0000 (UTC), Amit Walambe wrote:
> > > I don't understand. You just said yourself that the problem didn't show
> > > on a 2.6.23 kernel, meaning that we somehow fixed the bug already, but
> > > still you want your patch to be applied?
> > My patch only waits till the busy bit clears. As you have said in your reply, it
> > adds unconditional delay of atleast 1 ms, but it avoided a few subsequent user
> > requests from failing.
> > I agree this happened on a very old kernel with vastly different behaviour, but
> > there is nothing in the current code to prevent it from happening. Still, I make
> > it your call if too cautious (but slow) patch is a good idea or not.
> > Please let me know if you think.
> 
> The slowdown caused by your patch is clearly not acceptable. It would
> be very easy to rewrite it to only delay as necessary. But the reason
> why I dislike your patch goes beyond this: I think it's broken by
> design. I can't see how it is different from simply changing the value
> of MAX_TIMEOUT from 100 to 200 as far as delay is concerned. And
> increasing MAX_TIMEOUT to 200 would have the added benefit that the
> first transaction would even succeed instead of failing (if the issue
> is that the transaction takes more time than the driver expects.)
> 
> Have you tried increasing MAX_TIMEOUT to 200 or even 500, to see if it
> would solve your problem?
> 
> > > For example, this patch:
> > >
> > http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=ca8b9e32a11a7cbfecbef00c8451a79fe1af392e
> > > seems pretty relevant to me, as it attempts to cleanly stop the
> > > controller on transaction timeout. It went into kernel 2.6.23.
> > This patch is useful to handle the failure that has occured (a new request fails
> > since controller is still busy with previous operation). But it doesn't avoid
> > this situation.
> 
> It is supposed to address exactly the problem you reported, by not
> leaving the controller in a busy state at the end of a "failing"
> transaction (where "failing" can mean "takes too long" too). Whether
> this actually works or not, is another story... I gave it a try today
> on an ICH5-based system, and the only error case I could test (SMBus
> block read with invalid size) was not recovered properly. The code
> looks good though, so maybe it's a hardware issue. I would really like
> to know if it works in your case.
> 
> > > I asked for the output of i2cdetect (i2cdetect 0 in your case), not
> > > i2cdetect -l. I wanted to have an idea what chips were present on the
> > > bus.
> >      0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f
> > 00:          -- -- -- -- -- 08 -- -- -- -- -- -- -- 
> > 10: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 
> > 20: 20 -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 
> > 30: 30 -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 
> > 40: -- -- -- -- 44 -- -- -- 48 -- -- -- -- -- -- -- 
> > 50: 50 -- -- -- 54 -- -- -- -- -- -- -- -- -- -- -- 
> > 60: -- -- -- -- -- -- -- -- -- 69 6a -- -- -- -- -- 
> > 70: -- -- -- -- -- -- -- --                         
> 
> I/O, EEPROM, thermal sensor, clock chips... lot of stuff on this bus!
> 
> > > One important thing to notice is that the driver is still doing
> > > something wrong in this area. If the host is found busy (bit 0 of the
> > > status register) the driver attempts to write 1 to that bit. Given that
> > > the bit in question is read-only, I'd say the chances that it works are
> > > thin... 
> > Ok, so the bit manipulation part is useless and wait part is only relevant.
> 
> Correct.
> 
> As you may have seen already, I've just sent the i2c-i801 patches I've
> been working on. The 4th one:
>   http://lists.lm-sensors.org/pipermail/i2c/2008-June/004070.html
> fixes the handling of the HOST_BUSY bit, amongst other error handling
> cleanups. It would be great if you could test it, although I understand
> that it might be difficult, or even impossible, if your customer has to
> stick to kernel 2.6.9. But one thing for sure, is that I won't take
> your patch, unless you test both kernel 2.6.25 and Linus' latest with
> my patches applied, and both keep failing in your situation.
> 


-- 
Amit Walambe
Design Engineer

Eurotech Ltd,
3 Clifton Court,
Cambridge CB1 7BN,
United Kingdom.

Tel: +44 (0)1223 403465 Direct
Tel: +44 (0)1223 411200 Switchboard
Fax: +44 (0)1223 410457
E-Mail: amit.walambe-ymFC7zkAqBLaL3YF3Y4TSbVCufUGDwFn@public.gmane.org
Web: http://www.eurotech-ltd.co.uk

Eurotech Ltd is a subsidiary of Eurotech S.p.A
VAT No. GB 314961067    Registered in England 1608562
Registered Office: 3 Clifton Court, Clifton Road, Cambridge CB1 7BN  UK

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [PATCH] fix i801_transaction() and ioctl return values for i2c-i801

[Amit Walambe]

On Mon, 9 Jun 2008 15:23:52 +0200
"Jean Delvare" <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:

> Hi Amit,
Hi Jean!
The gmane website or my company's emailing system is acting up. I am
receiving emails very late and my postings are not showing up.
So, I am resending my reply.
 
> I don't understand. You just said yourself that the problem didn't show
> on a 2.6.23 kernel, meaning that we somehow fixed the bug already, but
> still you want your patch to be applied?
The problem not appearing on newer kernels can be a function of
different latency behaviour. There is no code in the driver to handle
the situation. Current driver will return failure for a new request if
it finds host controller busy with previous one; which is not
acceptable in our case. Hence a patch to simply wait till the
operation completes.

I think this discussion basically boils down to whether the driver
should complete the current operation or just start it and return,
hoping to have it finished before the next request arrives. 

> For example, this patch:
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=ca8b9e32a11a7cbfecbef00c8451a79fe1af392e
> seems pretty relevant to me, as it attempts to cleanly stop the
> controller on transaction timeout. It went into kernel 2.6.23.
This patch handles the case when actual command fails (by aborting the
command). What if, as in our case, resetting of status bits takes long?

Why such a simple operation is taking long is something I don't know (but 
I am sure no other operation is being carried out).

> i2cset supports this functionality since October 2004.
I didn't look into it. Apologies!

> I asked for the output of i2cdetect (i2cdetect 0 in your case), not
> i2cdetect -l. I wanted to have an idea what chips were present on the
> bus.
     0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f
00:          -- -- -- -- -- 08 -- -- -- -- -- -- -- 
10: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 
20: 20 -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 
30: 30 -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 
40: -- -- -- -- 44 -- -- -- 48 -- -- -- -- -- -- -- 
50: 50 -- -- -- 54 -- -- -- -- -- -- -- -- -- -- -- 
60: -- -- -- -- -- -- -- -- -- 69 6a -- -- -- -- -- 
70: -- -- -- -- -- -- -- --                         

> Please note that "resetting" is not really the correct term for what
I used "resetting the status bits" somewhere else in the message.
Shouldn't have used 'reset' interchangeably.

> As written above, I think your problem is already fixed in 2.6.23 so no
> patch is needed. I wouldn't want your patch as is anyway, as it adds a
> sleep unconditionally, making the driver twice as slow as the original
> code.
ok. I am not insisting it goes mainline. I understand your concerns. Please 
suggest if you can think of a better way to handle this condition.

> One thing that might still be worth investigating is why some
> transactions do not complete in the first place.
> 
> This might be related to the fact that the timeout loop was written
> with HZ=100 in mind and is broken by design. It timeouts in count (100,
> where all but one drivers have 500), not time. Depending on various
> parameters (amongst which the value of HZ), the actual time it waits
> for is very different. At HZ=100 it was typically 2 seconds, but at
> HZ=1000 it would be 200 ms, which isn't that much. Although at 16 kHz,
> a 32-byte I2C block read transaction would take 20 ms if I count
> correctly, so it still fits.
I will investigate more on the line of the above suggestions. Thanks! Any 
pointers for the first one (as to why resetting the status bits is taking
this long) would be more than welcome.

Thanks a lot for the help!
-- 
Amit Walambe
Design Engineer

Eurotech Ltd,
3 Clifton Court,
Cambridge CB1 7BN,
United Kingdom.

Tel: +44 (0)1223 403465 Direct
Tel: +44 (0)1223 411200 Switchboard
Fax: +44 (0)1223 410457
E-Mail: amit.walambe-ymFC7zkAqBLaL3YF3Y4TSbVCufUGDwFn@public.gmane.org
Web: http://www.eurotech-ltd.co.uk

Eurotech Ltd is a subsidiary of Eurotech S.p.A
VAT No. GB 314961067    Registered in England 1608562
Registered Office: 3 Clifton Court, Clifton Road, Cambridge CB1 7BN  UK

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [PATCH] fix i801_transaction() and ioctl return values for i2c-i801

[Amit Walambe]

Correcting an initialisation error..
On Mon, 9 Jun 2008 11:56:56 +0100
Amit Walambe <amit.walambe-ymFC7zkAqBLaL3YF3Y4TSbVCufUGDwFn@public.gmane.org> wrote:

> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index b0f771f..d165829 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -198,9 +198,17 @@ static int i801_transaction(int xact)
>         if ((inb_p(SMBHSTSTS) & 0x1f) != 0x00)
>                 outb_p(inb(SMBHSTSTS), SMBHSTSTS);
>  
> -       if ((temp = (0x1f & inb_p(SMBHSTSTS))) != 0x00) {
> -               dev_dbg(&I801_dev->dev, "Failed reset at end of transaction "
> -                       "(%02x)\n", temp);
> +       /* We wait for a fraction of a second! */
> +       do {
> +               msleep(1);
> +               temp = inb_p(SMBHSTSTS);
> +       } while ((temp & 0x01) && (timeout++ < MAX_TIMEOUT));
> +
> +       /* If the SMBus is still busy, we give up */
> +       if (timeout >= MAX_TIMEOUT) {
> +               result = -EBUSY;
> +               dev_dbg(&I801_dev->dev, "Failed reset at end of transaction"
> +                               "(%02x)\n", temp);
>         }
>         dev_dbg(&I801_dev->dev, "Transaction (post): CNT=%02x, CMD=%02x, "
>                 "ADD=%02x, DAT0=%02x, DAT1=%02x\n", inb_p(SMBHSTCNT),
> 
> Thanks and regards
> 
diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index b0f771f..f6b4598 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -198,9 +198,18 @@ static int i801_transaction(int xact)
        if ((inb_p(SMBHSTSTS) & 0x1f) != 0x00)
                outb_p(inb(SMBHSTSTS), SMBHSTSTS);
 
-       if ((temp = (0x1f & inb_p(SMBHSTSTS))) != 0x00) {
-               dev_dbg(&I801_dev->dev, "Failed reset at end of transaction "
-                       "(%02x)\n", temp);
+       timeout = 0;
+       /* We wait for a fraction of a second! */
+       do {
+               msleep(1);
+               temp = inb_p(SMBHSTSTS);
+       } while ((temp & 0x01) && (timeout++ < MAX_TIMEOUT));
+
+       /* If the SMBus is still busy, we give up */
+       if (timeout >= MAX_TIMEOUT) {
+               result = -EBUSY;
+               dev_dbg(&I801_dev->dev, "Failed reset at end of transaction"
+                               "(%02x)\n", temp);
        }
        dev_dbg(&I801_dev->dev, "Transaction (post): CNT=%02x, CMD=%02x, "
                "ADD=%02x, DAT0=%02x, DAT1=%02x\n", inb_p(SMBHSTCNT),

-- 
Amit Walambe
Design Engineer

Eurotech Ltd,
3 Clifton Court,
Cambridge CB1 7BN,
United Kingdom.

Tel: +44 (0)1223 403465 Direct
Tel: +44 (0)1223 411200 Switchboard
Fax: +44 (0)1223 410457
E-Mail: amit.walambe-ymFC7zkAqBLaL3YF3Y4TSbVCufUGDwFn@public.gmane.org
Web: http://www.eurotech-ltd.co.uk

Eurotech Ltd is a subsidiary of Eurotech S.p.A
VAT No. GB 314961067    Registered in England 1608562
Registered Office: 3 Clifton Court, Clifton Road, Cambridge CB1 7BN  UK

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c