+ i2c-fix-i2c-mpc-driver-for-multi-master-i2c-busses.patch added to -mm tree

+ i2c-fix-i2c-mpc-driver-for-multi-master-i2c-busses.patch added to -mm tree

[akpm]

The patch titled
     i2c: fix i2c-mpc driver for multi-master i2c busses
has been added to the -mm tree.  Its filename is
     i2c-fix-i2c-mpc-driver-for-multi-master-i2c-busses.patch

Before you just go and hit "reply", please:
   a) Consider who else should be cc'ed
   b) Prefer to cc a suitable mailing list as well
   c) Ideally: find the original patch on the mailing list and do a
      reply-to-all to that, adding suitable additional cc's

*** Remember to use Documentation/SubmitChecklist when testing your code ***

See http://userweb.kernel.org/~akpm/stuff/added-to-mm.txt to find
out what to do about this

The current -mm tree may be found at http://userweb.kernel.org/~akpm/mmotm/

------------------------------------------------------
Subject: i2c: fix i2c-mpc driver for multi-master i2c busses
From: Clifford Wolf <clifford@clifford.at>

Simply retry on arbitration lost until xfer is successfull, a
non-arbitration-lost error is triggered or the 1s timeout is hit.

Tested with a freescale MPC8349E host cpu.

Signed-off-by: Clifford Wolf <clifford@clifford.at>
Cc: Jean Delvare <khali@linux-fr.org>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 drivers/i2c/busses/i2c-mpc.c |   22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff -puN drivers/i2c/busses/i2c-mpc.c~i2c-fix-i2c-mpc-driver-for-multi-master-i2c-busses drivers/i2c/busses/i2c-mpc.c
--- a/drivers/i2c/busses/i2c-mpc.c~i2c-fix-i2c-mpc-driver-for-multi-master-i2c-busses
+++ a/drivers/i2c/busses/i2c-mpc.c
@@ -134,13 +134,13 @@ static int i2c_wait(struct mpc_i2c *i2c,
 	if (result < 0)
 		return result;
 
-	if (!(x & CSR_MCF)) {
-		pr_debug("I2C: unfinished\n");
-		return -EIO;
-	}
-
 	if (x & CSR_MAL) {
 		pr_debug("I2C: MAL\n");
+		return -EAGAIN;
+	}
+
+	if (!(x & CSR_MCF)) {
+		pr_debug("I2C: unfinished\n");
 		return -EIO;
 	}
 
@@ -257,11 +257,11 @@ static int mpc_read(struct mpc_i2c *i2c,
 static int mpc_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
 {
 	struct i2c_msg *pmsg;
-	int i;
-	int ret = 0;
+	int i, ret;
 	unsigned long orig_jiffies = jiffies;
 	struct mpc_i2c *i2c = i2c_get_adapdata(adap);
 
+restart:
 	mpc_i2c_start(i2c);
 
 	/* Allow bus up to 1s to become not busy */
@@ -281,7 +281,7 @@ static int mpc_xfer(struct i2c_adapter *
 		schedule();
 	}
 
-	for (i = 0; ret >= 0 && i < num; i++) {
+	for (i = 0, ret = 0; ret >= 0 && i < num; i++) {
 		pmsg = &msgs[i];
 		pr_debug("Doing %s %d bytes to 0x%02x - %d of %d messages\n",
 			 pmsg->flags & I2C_M_RD ? "read" : "write",
@@ -292,7 +292,13 @@ static int mpc_xfer(struct i2c_adapter *
 		else
 			ret =
 			    mpc_write(i2c, pmsg->addr, pmsg->buf, pmsg->len, i);
+		if (ret == -EAGAIN) {
+			pr_debug("Lost i2c arbitration -> retry.\n");
+			mpc_i2c_stop(i2c);
+			goto restart;
+		}
 	}
+
 	mpc_i2c_stop(i2c);
 	return (ret < 0) ? ret : num;
 }
_

Patches currently in -mm which might be from clifford@clifford.at are

i2c-fix-i2c-mpc-driver-for-multi-master-i2c-busses.patch

Re: + i2c-fix-i2c-mpc-driver-for-multi-master-i2c-busses.patch added to -mm tree

[Jean Delvare]

Hi David,

Switching to the i2c list, so that others can participate...

On Sun, 15 Feb 2009 16:53:36 -0800, David Brownell wrote:
> On Saturday 14 February 2009, Jean Delvare wrote:
> > From: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
> > Subject: i2c: Automatically retry transfers on arbitration loss
> > 
> > Automatically retry transfers on arbitration loss. Each i2c_adapter
> > controls how many attempts will be made, using the retries value.
> > 
> > Signed-off-by: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
> > Cc: Clifford Wolf <clifford-cPpHkPqGOEfk7+2FdBfRIA@public.gmane.org>
> > Cc: David Brownell <david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org>
> 
> Seems fair to me, except that the retry count is coupled
> to the adapter not to the specific request.  Automatic
> retry also seems problematic, unless the caller wants it.
> 
> There's no guarantee of idempotency in messages; only
> callers can know if retryig a given partially completed
> message is safe.  And since fault reporting is still goofy,
> we can't know just where arbitration was lost... in the
> first master transmit, second (after repeated START),
> third, etc.

As already explained by Clifford, arbitration loss is about messages
which have not been transmitted at all. So retrying is always OK.

> Note that only multi-master configurations will arbitrate
> for master transmit ... which means only I2C busses, not
> SMBus ones.  Thus it's odd to see i2c_smbus_transfer()
> arbitrate.

SMBus can have multiple masters just as I2C does. From the SMBus 2.0
specification document, section 4.3.1:

"A situation may occur in which more than one master is trying to place
clock signals on the bus at the same time."

And even if not, i2c_smbus_transfer() is meant for both I2C and SMBus,
the whole point of this function being to allow for portable drivers
working on both bus types.

> So I think it might be better to just create a new I2C
> call taking a specified number of retries ... and get rid
> of the old adap->retries field, as I though was the plan.

That was the plan, before Clifford came up with a legitimate use case.
Now I'd rather reuse the existing infrastructure to handle arbitration.

> Analyse the SMBus calls to see which ones (if any) are
> appropriate for automagic retries; include that answer
> in source code comment, and the function definitions.

There's such thing as "SMBus call which are appropriate for automagic
retries". It's all about the failure reason (arbitration loss or
something else) not the transfer type.

> I have no problem with using a retry count instead of a
> timeout.  Again there is an SMBus difference here; SMBus
> specifies (low level) protocol timeouts, but I2C allows
> transfers to be arbitrarily long.
> 
> One issue is how should you measure a timeout in the I2C
> stack.  Normally I'd expect it to start from issuing the
> request ... that is, to include any queue delays incurred
> while waiting for previous transfers from this host/master to
> complete, which might include letting other masters finish.
> 
> But i2c-core doesn't actually enforce such timeouts.  Again,
> this should be a per-request thing.

I see very little use for per-request timeout values in practice.

> I confess the only time I've actually needed to cope with
> arbitration was when doing some I2C slave firmware, with
> the SMBALERT# protocol.  Slave transmit there arbitrates
> which alert gets to the host; not Linux code at all.  And
> there's no issue of non-idempotent messages, it's a case
> of responding to the ARA until the response is acked, and
> then stop pulling SMBALERT# low.  (Other slaves may still
> pull it low, keeping the IRQ active.)

SMBALERT# protocol patches which are still pending, BTW... Are you ever
going to polish them and send them to me?

Thanks,
-- 
Jean Delvare

Re: + i2c-fix-i2c-mpc-driver-for-multi-master-i2c-busses.patch added to -mm tree

[David Brownell]

On Monday 16 February 2009, Jean Delvare wrote:
> > There's no guarantee of idempotency in messages; only
> > callers can know if retrying a given partially completed
> > message is safe.  And since fault reporting is still goofy,
> > we can't know just where arbitration was lost... in the
> > first master transmit, second (after repeated START),
> > third, etc.
> 
> As already explained by Clifford, arbitration loss is about messages
> which have not been transmitted at all. So retrying is always OK.

I must have missed that.  It's not correct, in any case.
Messages can easily have been *partially* transmitted.

Loss of arbitration appears at the first transmitted bit
where one master sends '0' and overrides another, which
is sending '1' instead.  Ideally it's while addressing a
device, but it could be after some data bytes have been
sent ... and, depending on the slave, acted upon.  Even
after one or more repeated starts.

- Dave

Re: + i2c-fix-i2c-mpc-driver-for-multi-master-i2c-busses.patch added to -mm tree

[Clifford Wolf]

Hi,

On Mon, Feb 16, 2009 at 03:58:47AM -0800, David Brownell wrote:
> I must have missed that.  It's not correct, in any case.
> Messages can easily have been *partially* transmitted.
> 
> Loss of arbitration appears at the first transmitted bit
> where one master sends '0' and overrides another, which
> is sending '1' instead.  Ideally it's while addressing a
> device, but it could be after some data bytes have been
> sent ... and, depending on the slave, acted upon.  Even
> after one or more repeated starts.

the master then wins the conflict continues sending its message to the
slave. so the slave never recieves a partial message.

therefore the partially transmitted problem is none and retrying after
arbritration lost is always safe.

yours,
 - clifford

-- 
Relax, its only ONES and ZEROS!

Re: Handling of i2c arbitration loss (Was: i2c-fix-i2c-mpc-driver-for-multi-master-i2c-busses.patch added to -mm tree)

[Jean Delvare]

Hi Dave,

On Mon, 16 Feb 2009 03:58:47 -0800, David Brownell wrote:
> On Monday 16 February 2009, Jean Delvare wrote:
> > > There's no guarantee of idempotency in messages; only
> > > callers can know if retrying a given partially completed
> > > message is safe.  And since fault reporting is still goofy,
> > > we can't know just where arbitration was lost... in the
> > > first master transmit, second (after repeated START),
> > > third, etc.
> > 
> > As already explained by Clifford, arbitration loss is about messages
> > which have not been transmitted at all. So retrying is always OK.
> 
> I must have missed that.  It's not correct, in any case.
> Messages can easily have been *partially* transmitted.

Sorry, but no, really. On bus error or device error, yes, but not on
arbitration loss, by definition of arbitration on I2C buses.

> Loss of arbitration appears at the first transmitted bit
> where one master sends '0' and overrides another, which
> is sending '1' instead.  Ideally it's while addressing a
> device, but it could be after some data bytes have been
> sent ... and, depending on the slave, acted upon.  Even
> after one or more repeated starts.

Please think about it all again. If two masters talk at the same time,
as long as they send the same bits, none loses arbitration. The
addressed slave OTOH has no clue that there are two masters talking, it
sees the exact same data as if only one master was talking. At the
moment one of the master loses arbitration it'll stop talking, and from
the slave's point of view everything is as if it had never talked in
the first place.

The funny case being if both masters actually send the exact same
message, because neither will lose arbitration, both think they
succeeded, but the slave has really only received the message once, not
twice. Which may or may not be a problem depending on what the slave is
supposed to do out of the message in question - but this is a hardware
limitation and up to the designers to think of and deal with.

-- 
Jean Delvare

Re: Handling of i2c arbitration loss

[Jean Delvare]

Hi Clifford,

On Mon, 16 Feb 2009 14:10:26 +0100, Clifford Wolf wrote:
> On Sun, Feb 15, 2009 at 11:31:22AM +0100, Jean Delvare wrote:
> > Care to improve my patch to actually implement your idea? I'm a bit
> > short on spare time these days.
> 
> here is my improved version. unfortunatly I was only able to compile-test
> it since I have unrelated problems (open firmware changes in recent kernels) 
> with my i2c test platform...
> 
> I have copied the code from the i2c-mpc timeout handling which I have
> tested some weeks ago, so I am 99.9% sure that it does exactly what it
> should do.. ;-)
> 
> --snip--
> i2c: Automatically retry transfers on arbitration loss.
> 
> Each i2c_adapter controls how many attempts will be made and for how long
> retransmitting the message is tried, using the retries and timout values.
> 
> Signed-off-by: Clifford Wolf <clifford-cPpHkPqGOEfk7+2FdBfRIA@public.gmane.org>
> Cc: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
> Cc: David Brownell <david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org>
> 
> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> index b346a68..f3cb4f8 100644
> --- a/drivers/i2c/i2c-core.c
> +++ b/drivers/i2c/i2c-core.c
> @@ -1000,7 +1000,8 @@ module_exit(i2c_exit);
>   */
>  int i2c_transfer(struct i2c_adapter * adap, struct i2c_msg *msgs, int num)
>  {
> -	int ret;
> +	unsigned long orig_jiffies = jiffies;

I think you should initialize orig_jiffies *after* you get the bus
lock. Otherwise the behavior depends on how long you had to wait to get
control of the bus. Or was is intended?

> +	int ret, try;
>  
>  	/* REVISIT the fault reporting model here is weak:
>  	 *
> @@ -1038,7 +1039,14 @@ int i2c_transfer(struct i2c_adapter * adap, struct i2c_msg *msgs, int num)
>  			mutex_lock_nested(&adap->bus_lock, adap->level);
>  		}
>  
> -		ret = adap->algo->master_xfer(adap,msgs,num);
> +		/* Retry automatically on arbitration loss */
> +		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))

This assumes that adap->timeout is expressed in jiffies. Which
according to a comment in include/linux/i2c-dev.h, is the case, but
this must be for historical reasons. This never made sense to me, as it
means that the actual timeout value depends on the value of HZ.

The key problem here is that the timeout value is exported to
user-space and can be changed by user-space. And user-space normally
doesn't know the value of HZ... So the timeout value should be
expressed in a constant time unit.

I see 3 possible solutions:
1* Ignore the problem and keep using jiffies for the timeout value. Most
   drivers will need to be fixed to specify a timeout in jiffies (e.g.
   msecs_to_jiffies(100)) and not a raw number.
2* Say that the value is expressed in units of 10 ms. This is
   historically equivalent to a jiffy (as Linux started with HZ=100) to
   backwards compatibility is preserved. Drivers have to be updated to
   convert the value to jiffies if calling time_after() or equivalent.
   The only drawback I see is that this isn't a terribly natural unit.
3* Say that the value is expressed in ms. This means that all drivers
   have to be updated both for timeout initialization and use, AND
   user-space may be surprised by the change. The only positive side is
   that ms is a natural, finer-grained unit.

The second solution sounds the best to me. Opinions?

This problem isn't directly related to your patch, however so far
drivers were handling the timeout internally so I didn't really care.
Now that your patch will make use of the timeout value in i2c-core, I'd
like to clarify the situation first.


> +				break;
> +		}
>  		mutex_unlock(&adap->bus_lock);
>  
>  		return ret;
> @@ -1956,9 +1964,20 @@ s32 i2c_smbus_xfer(struct i2c_adapter * adapter, u16 addr, unsigned short flags,
>  	flags &= I2C_M_TEN | I2C_CLIENT_PEC;
>  
>  	if (adapter->algo->smbus_xfer) {
> +		unsigned long orig_jiffies = jiffies;
> +		int try;
> +
>  		mutex_lock(&adapter->bus_lock);
> -		res = adapter->algo->smbus_xfer(adapter,addr,flags,read_write,
> -						command, protocol, data);
> +		/* Retry automatically on arbitration loss */
> +		for (res = 0, try = 0; try <= adapter->retries; try++) {
> +			res = adapter->algo->smbus_xfer(adapter, addr, flags,
> +							read_write, command,
> +							protocol, data);
> +			if (res != -EAGAIN)
> +				break;
> +			if (time_after(jiffies, orig_jiffies + adapter->timeout))
> +				break;
> +		}
>  		mutex_unlock(&adapter->bus_lock);
>  	} else
>  		res = i2c_smbus_xfer_emulated(adapter,addr,flags,read_write,
> 


-- 
Jean Delvare

Re: Handling of i2c arbitration loss (Was: i2c-fix-i2c-mpc-driver-for-multi-master-i2c-busses.patch added to -mm tree)

[David Brownell]

On Monday 16 February 2009, Jean Delvare wrote:
> Hi Dave,
> 
> On Mon, 16 Feb 2009 03:58:47 -0800, David Brownell wrote:
> > On Monday 16 February 2009, Jean Delvare wrote:
> > > > There's no guarantee of idempotency in messages; only
> > > > callers can know if retrying a given partially completed
> > > > message is safe.  And since fault reporting is still goofy,
> > > > we can't know just where arbitration was lost... in the
> > > > first master transmit, second (after repeated START),
> > > > third, etc.
> > > 
> > > As already explained by Clifford, arbitration loss is about messages
> > > which have not been transmitted at all. So retrying is always OK.
> > 
> > I must have missed that.  It's not correct, in any case.
> > Messages can easily have been *partially* transmitted.
> 
> Sorry, but no, really. On bus error or device error, yes, but not on
> arbitration loss, by definition of arbitration on I2C buses.

One "struct i2c_msg" can be, for example:

	- Transfer #1: write bytes to some address
	- Transfer #2: write bytes to some address
	- STOP always signifies message end

Now, arbitration happens only during TX, of address or
data.  (A "read" message includes two transfers.)  In
that example, there are two transfers which can trigger
arbitration loss faults.

So:  if two masters are using the bus at the same time,
it's possible they both have the same Transfer #1 but
don't do the same thing afterwards.  QED:  one master
observes a partial message transfer, losing arbitration
part way through the message.  The other observes no
fault at all.


> > Loss of arbitration appears at the first transmitted bit
> > where one master sends '0' and overrides another, which
> > is sending '1' instead.  Ideally it's while addressing a
> > device, but it could be after some data bytes have been
> > sent ... and, depending on the slave, acted upon.  Even
> > after one or more repeated starts.
> 
> Please think about it all again. If two masters talk at the same time,
> as long as they send the same bits, none loses arbitration. The
> addressed slave OTOH has no clue that there are two masters talking, it
> sees the exact same data as if only one master was talking. At the
> moment one of the master loses arbitration it'll stop talking, and from
> the slave's point of view everything is as if it had never talked in
> the first place.

That's within the scope of a *single* transfer.  A message
can include any number of such transfers, each of which will
be individually arbitrated ... and it's clearly possible that
one such transfer can succeed, but a later one doesn't (due
to arbitration loss).


> The funny case being if both masters actually send the exact same
> message,

Same "transfer", not message ...


> 	because neither will lose arbitration, both think they 
> succeeded, but the slave has really only received the message once, not
> twice. Which may or may not be a problem depending on what the slave is
> supposed to do out of the message in question - but this is a hardware
> limitation and up to the designers to think of and deal with.

Yes, but don't equate "transfer" with "message".  And the
point I'm making about partial failures is relevant very
specifically because it constrains what designers can do.

Consider the example above where both transfers have side
effects, and are non-idempotent.  It would be wrong to have
the i2c core assume it's safe to reissue Transfer #1 on the
assumption that it's safe to do so.

- Dave

Re: Handling of i2c arbitration loss (Was: i2c-fix-i2c-mpc-driver-for-multi-master-i2c-busses.patch added to -mm tree)

[Clifford Wolf]

Hi,

On Thu, Feb 19, 2009 at 11:15:07AM -0800, David Brownell wrote:
> > > I must have missed that.  It's not correct, in any case.
> > > Messages can easily have been *partially* transmitted.
> > 
> > Sorry, but no, really. On bus error or device error, yes, but not on
> > arbitration loss, by definition of arbitration on I2C buses.
> 
> One "struct i2c_msg" can be, for example:
> 
> 	- Transfer #1: write bytes to some address
> 	- Transfer #2: write bytes to some address
> 	- STOP always signifies message end
> 
> Now, arbitration happens only during TX, of address or
> data.  (A "read" message includes two transfers.)  In
> that example, there are two transfers which can trigger
> arbitration loss faults.
> 
> So:  if two masters are using the bus at the same time,
> it's possible they both have the same Transfer #1 but
> don't do the same thing afterwards.  QED:  one master
> observes a partial message transfer, losing arbitration
> part way through the message.  The other observes no
> fault at all.

still: no.

there are two scenarios:

1.) the transfers are seperated with a restart condition.

in this case no new arbitration happens and so an arbitration loss may only
happen when both masters sent the "message" from the first transfer on in
sync. obviously there is no problem there.

2.) the transfers are seperated by a stop and a new start condition.

in this case we have new arbitration. that means that a 2nd master could
always get arbitration and interrupt the message by sending something else
inbetween the two transfers. this is a problem - but not related to
arbitration lost but related to sending a stop and a start condition where
a restart condition should have been sent. this is imo clearly a bug in the
i2c adapter driver or hardware and should never happen...

> Consider the example above where both transfers have side
> effects, and are non-idempotent. It would be wrong to have
> the i2c core assume it's safe to reissue Transfer #1 on the
> assumption that it's safe to do so.

so as you can hopefully see now:
it is always safe to restart transfering a message after arbitration loss.

but it is vital that in case of an arbitration loss the whole message, not
only the last transfer is retransmitted. I haven't checked the code but I'm
pretty sure it is done this way in the current code..

yours,
 - clifford

-- 
Prof: So the American government went to IBM to come up with
a data encryption standard and they came up with ...
Student: EBCDIC!

Re: Handling of i2c arbitration loss

[Clifford Wolf]

Hi Jean,

On Thu, Feb 19, 2009 at 05:26:05PM +0100, Jean Delvare wrote:
> > @@ -1000,7 +1000,8 @@ module_exit(i2c_exit);
> >   */
> >  int i2c_transfer(struct i2c_adapter * adap, struct i2c_msg *msgs, int num)
> >  {
> > -	int ret;
> > +	unsigned long orig_jiffies = jiffies;
> 
> I think you should initialize orig_jiffies *after* you get the bus
> lock. Otherwise the behavior depends on how long you had to wait to get
> control of the bus. Or was is intended?

phew! this is a good question...

to be honest: I haven't thought about that one yet.

I think both approaches (including the wait for the lock in the timeout on
the one hand and just counting the time spent after getting the lock on the
other hand) would be valid..

But I think it would be better to not include the wait-for-lock time and
move the initialization of orig_jiffies to after locking the mutex.

In the case of including the wait-for-lock time there should be a timeout
handling added to the lock code so it only tries to get the lock for not
longer than the timeout... So my code is broken eighter way... ;-)

> > +		/* Retry automatically on arbitration loss */
> > +		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))
> 
> This assumes that adap->timeout is expressed in jiffies. Which
> according to a comment in include/linux/i2c-dev.h, is the case, but
> this must be for historical reasons. This never made sense to me, as it
> means that the actual timeout value depends on the value of HZ.
> 
> The key problem here is that the timeout value is exported to
> user-space and can be changed by user-space. And user-space normally
> doesn't know the value of HZ... So the timeout value should be
> expressed in a constant time unit.

I haven't had a look at the code but would it be so hard to convert the
timeout value to ms or something alike when exporting the value to
user-space and convert in the other direction on reading from user-space?

imo jiffies is the best unit for time within the kernel and I would prefer
having one place for the convertion instead of duplicating it in every
single driver and multiple places in i2c-core..

> This problem isn't directly related to your patch, however so far
> drivers were handling the timeout internally so I didn't really care.
> Now that your patch will make use of the timeout value in i2c-core, I'd
> like to clarify the situation first.


yours,
 - clifford

-- 
"Perfection [in design] is achieved not when there is nothing left to
add, but rather when there is nothing left to take away."
 - Antoine de Saint-Exupery

Re: Handling of i2c arbitration loss (Was: i2c-fix-i2c-mpc-driver-for-multi-master-i2c-busses.patch added to -mm tree)

[Jean Delvare]

Hi David,

On Thu, 19 Feb 2009 11:15:07 -0800, David Brownell wrote:
> On Monday 16 February 2009, Jean Delvare wrote:
> > On Mon, 16 Feb 2009 03:58:47 -0800, David Brownell wrote:
> > > I must have missed that.  It's not correct, in any case.
> > > Messages can easily have been *partially* transmitted.
> > 
> > Sorry, but no, really. On bus error or device error, yes, but not on
> > arbitration loss, by definition of arbitration on I2C buses.
> 
> One "struct i2c_msg" can be, for example:
> 
> 	- Transfer #1: write bytes to some address
> 	- Transfer #2: write bytes to some address
> 	- STOP always signifies message end

Actually, no, one struct i2c_msg can't be that. An array of 2 struct
i2c_msg, yes.

> Now, arbitration happens only during TX, of address or
> data.  (A "read" message includes two transfers.)  In

The other way around...

> that example, there are two transfers which can trigger
> arbitration loss faults.
> 
> So:  if two masters are using the bus at the same time,
> it's possible they both have the same Transfer #1 but
> don't do the same thing afterwards.  QED:  one master
> observes a partial message transfer, losing arbitration
> part way through the message.  The other observes no
> fault at all.

No. I am sorry I have to insist. There is no such thing as a partial
message transfer followed by an arbitration loss. If a master ever
loses arbitration, then _everything_ it sent over the wire since the
previous start (_not_ repeated start) condition has simply never
existed for anybody else on the bus (masters and slave alike). Whether
this happens while sending the slave address of the first message, or
the last data byte of the last message, doesn't make any difference.

> > > Loss of arbitration appears at the first transmitted bit
> > > where one master sends '0' and overrides another, which
> > > is sending '1' instead.  Ideally it's while addressing a
> > > device, but it could be after some data bytes have been
> > > sent ... and, depending on the slave, acted upon.  Even
> > > after one or more repeated starts.
> > 
> > Please think about it all again. If two masters talk at the same time,
> > as long as they send the same bits, none loses arbitration. The
> > addressed slave OTOH has no clue that there are two masters talking, it
> > sees the exact same data as if only one master was talking. At the
> > moment one of the master loses arbitration it'll stop talking, and from
> > the slave's point of view everything is as if it had never talked in
> > the first place.
> 
> That's within the scope of a *single* transfer.  A message
> can include any number of such transfers, each of which will
> be individually arbitrated ... and it's clearly possible that
> one such transfer can succeed, but a later one doesn't (due
> to arbitration loss).

Arbitration is per transfer (from start condition to stop condition)
not per individual message (from start condition to repeated start
condition or repeated start condition to stop condition.)

> > The funny case being if both masters actually send the exact same
> > message,
> 
> Same "transfer", not message ...

Funny that you dare correcting me on this when you are the one being
confused here ;) As far as Linux' i2c implementation is concerned, an
I2C transfer can made of one or more messages (i2c_transfer takes an
array of struct i2c_msg as a parameter). The I2C specification doesn't
seem to really make a difference, as far as I can see it uses "message"
for everything.

> > 	because neither will lose arbitration, both think they 
> > succeeded, but the slave has really only received the message once, not
> > twice. Which may or may not be a problem depending on what the slave is
> > supposed to do out of the message in question - but this is a hardware
> > limitation and up to the designers to think of and deal with.
> 
> Yes, but don't equate "transfer" with "message".  And the
> point I'm making about partial failures is relevant very
> specifically because it constrains what designers can do.
> 
> Consider the example above where both transfers have side
> effects, and are non-idempotent.  It would be wrong to have
> the i2c core assume it's safe to reissue Transfer #1 on the
> assumption that it's safe to do so.

We don't care if the message has side effects, because it never existed
in the first place.

As far as non-idempotent messages are concerned, the problem is
inherent to multi-master I2C buses. If two masters send the exact same
message at the exact same time, arbitration doesn't even trigger, and
the message only goes once on the bus. There is nothing the OS can do
about this.

-- 
Jean Delvare

Re: Handling of i2c arbitration loss

[Jean Delvare]

On Thu, 19 Feb 2009 22:23:25 +0100, Clifford Wolf wrote:
> Hi Jean,
> 
> On Thu, Feb 19, 2009 at 05:26:05PM +0100, Jean Delvare wrote:
> > > @@ -1000,7 +1000,8 @@ module_exit(i2c_exit);
> > >   */
> > >  int i2c_transfer(struct i2c_adapter * adap, struct i2c_msg *msgs, int num)
> > >  {
> > > -	int ret;
> > > +	unsigned long orig_jiffies = jiffies;
> > 
> > I think you should initialize orig_jiffies *after* you get the bus
> > lock. Otherwise the behavior depends on how long you had to wait to get
> > control of the bus. Or was is intended?
> 
> phew! this is a good question...
> 
> to be honest: I haven't thought about that one yet.
> 
> I think both approaches (including the wait for the lock in the timeout on
> the one hand and just counting the time spent after getting the lock on the
> other hand) would be valid..
>
> But I think it would be better to not include the wait-for-lock time and
> move the initialization of orig_jiffies to after locking the mutex.

I agree... Please send an updated patch.

> In the case of including the wait-for-lock time there should be a timeout
> handling added to the lock code so it only tries to get the lock for not
> longer than the timeout... So my code is broken eighter way... ;-)

That's a different issue. But I don't think we really need to handle
this case. You should never wait long before you get access to the bus.
If you do then there is a design problem which needs to be solved, and
timing out if you don't get the bus lock in a given amount of time
isn't going to solve it. If one really can't afford waiting for the bus
then the i2c_transfer() function should return immediately (as it
already does in the can't-sleep case.)

> > This assumes that adap->timeout is expressed in jiffies. Which
> > according to a comment in include/linux/i2c-dev.h, is the case, but
> > this must be for historical reasons. This never made sense to me, as it
> > means that the actual timeout value depends on the value of HZ.
> > 
> > The key problem here is that the timeout value is exported to
> > user-space and can be changed by user-space. And user-space normally
> > doesn't know the value of HZ... So the timeout value should be
> > expressed in a constant time unit.
> 
> I haven't had a look at the code but would it be so hard to convert the
> timeout value to ms or something alike when exporting the value to
> user-space and convert in the other direction on reading from user-space?

You are perfectly right. I came out to the same conclusion as I woke up
this morning. Sleep over it...

> imo jiffies is the best unit for time within the kernel and I would prefer
> having one place for the convertion instead of duplicating it in every
> single driver and multiple places in i2c-core..

Well you have to do the conversion in each driver either way, as
hard-coding a number of jiffies is a bad idea (makes the timeout depend
on the value of HZ). But I agree it is better to convert from ms to
jiffies once at initialization rather than each time we need to use the
timeout value.

I'll send a patch to the list now, addressing the issue in i2c-dev.
Your own work is unaffected by this.

-- 
Jean Delvare

Re: Handling of i2c arbitration loss

[Jean Delvare]

On Fri, 20 Feb 2009 12:45:46 +0100, Jean Delvare wrote:
> On Thu, 19 Feb 2009 22:23:25 +0100, Clifford Wolf wrote:
> > Hi Jean,
> > 
> > On Thu, Feb 19, 2009 at 05:26:05PM +0100, Jean Delvare wrote:
> > > > @@ -1000,7 +1000,8 @@ module_exit(i2c_exit);
> > > >   */
> > > >  int i2c_transfer(struct i2c_adapter * adap, struct i2c_msg *msgs, int num)
> > > >  {
> > > > -	int ret;
> > > > +	unsigned long orig_jiffies = jiffies;
> > > 
> > > I think you should initialize orig_jiffies *after* you get the bus
> > > lock. Otherwise the behavior depends on how long you had to wait to get
> > > control of the bus. Or was is intended?
> > 
> > phew! this is a good question...
> > 
> > to be honest: I haven't thought about that one yet.
> > 
> > I think both approaches (including the wait for the lock in the timeout on
> > the one hand and just counting the time spent after getting the lock on the
> > other hand) would be valid..
> >
> > But I think it would be better to not include the wait-for-lock time and
> > move the initialization of orig_jiffies to after locking the mutex.
> 
> I agree... Please send an updated patch.

Clifford, any news? I didn't get any update for this patch. If I don't
get one soon, I'll have to just discard it.

-- 
Jean Delvare

Re: Handling of i2c arbitration loss

[Clifford Wolf]

Hi Jean,

On Thu, Apr 09, 2009 at 08:47:26AM +0200, Jean Delvare wrote:
> Clifford, any news? I didn't get any update for this patch. If I don't
> get one soon, I'll have to just discard it.

oops. has been pretty busy the last weeks...

here is an updated version of the patch (based on the current linus git
tree). The only change is that I have moved the initialization of the
orig_jiffies variables. Afair we agreed on having adapter->timeout in
jiffies and I remeber that you also have made already a patch to unify this
for the entire i2c subsystem..

I can't test it until next week, but the changes are trivial. I also have
on my todo list for next week to cleanup my i2c-mpc patches and send those
parts which are still required after the changes in i2c-core again (I think
they have all been removed from the -mm tree already)..

yours,
 - clifford

--snip--

i2c: Retry automatically on arbitration loss

some small changes in i2c core to retry i2c xfers until eighter the
maximum number of retries or the timeout is hit.

Signed-of-by: Clifford Wolf <clifford-cPpHkPqGOEfk7+2FdBfRIA@public.gmane.org>
---
 drivers/i2c/i2c-core.c |   29 +++++++++++++++++++++++++----
 1 files changed, 25 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index b6f3a0d..de8a0bf 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -1023,7 +1023,8 @@ module_exit(i2c_exit);
  */
 int i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
 {
-	int ret;
+	unsigned long orig_jiffies;
+	int ret, try;
 
 	/* REVISIT the fault reporting model here is weak:
 	 *
@@ -1061,7 +1062,15 @@ int i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
 			mutex_lock_nested(&adap->bus_lock, adap->level);
 		}
 
-		ret = adap->algo->master_xfer(adap,msgs,num);
+		/* 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;
+		}
 		mutex_unlock(&adap->bus_lock);
 
 		return ret;
@@ -1996,14 +2005,26 @@ s32 i2c_smbus_xfer(struct i2c_adapter *adapter, u16 addr, unsigned short flags,
 		   char read_write, u8 command, int protocol,
 		   union i2c_smbus_data *data)
 {
+	unsigned long orig_jiffies;
+	int try;
 	s32 res;
 
 	flags &= I2C_M_TEN | I2C_CLIENT_PEC;
 
 	if (adapter->algo->smbus_xfer) {
 		mutex_lock(&adapter->bus_lock);
-		res = adapter->algo->smbus_xfer(adapter,addr,flags,read_write,
-						command, protocol, data);
+
+		/* Retry automatically on arbitration loss */
+		orig_jiffies = jiffies;
+		for (res = 0, try = 0; try <= adapter->retries; try++) {
+			res = adapter->algo->smbus_xfer(adapter, addr, flags,
+							read_write, command,
+							protocol, data);
+			if (res != -EAGAIN)
+				break;
+			if (time_after(jiffies, orig_jiffies + adapter->timeout))
+				break;
+		}
 		mutex_unlock(&adapter->bus_lock);
 	} else
 		res = i2c_smbus_xfer_emulated(adapter,addr,flags,read_write,
-- 
1.5.6.1

Re: Handling of i2c arbitration loss

[Jean Delvare]

On Thu, 9 Apr 2009 10:35:53 +0200, Clifford Wolf wrote:
> Hi Jean,
> 
> On Thu, Apr 09, 2009 at 08:47:26AM +0200, Jean Delvare wrote:
> > Clifford, any news? I didn't get any update for this patch. If I don't
> > get one soon, I'll have to just discard it.
> 
> oops. has been pretty busy the last weeks...

Same here, as you can see :(

> 
> here is an updated version of the patch (based on the current linus git
> tree). The only change is that I have moved the initialization of the
> orig_jiffies variables. Afair we agreed on having adapter->timeout in
> jiffies and I remeber that you also have made already a patch to unify this
> for the entire i2c subsystem..
> 
> I can't test it until next week, but the changes are trivial. I also have
> on my todo list for next week to cleanup my i2c-mpc patches and send those
> parts which are still required after the changes in i2c-core again (I think
> they have all been removed from the -mm tree already)..
> 
> yours,
>  - clifford
> 
> --snip--
> 
> i2c: Retry automatically on arbitration loss
> 
> some small changes in i2c core to retry i2c xfers until eighter the
> maximum number of retries or the timeout is hit.
> 
> Signed-of-by: Clifford Wolf <clifford-cPpHkPqGOEfk7+2FdBfRIA@public.gmane.org>

It's "off" not "of" ;)

> ---
>  drivers/i2c/i2c-core.c |   29 +++++++++++++++++++++++++----
>  1 files changed, 25 insertions(+), 4 deletions(-)

Patch applied, thanks. It's in linux-next for now, will go in 2.6.31 if
no problem is reported.

-- 
Jean Delvare