Hi all,

Today's linux-next merge of the net tree got a conflict in
drivers/net/sfc/sfe4001.c between commit
3f7c0648f727a6d5baf6117653e4001dc877b90b ("i2c: Prevent priority
inversion on top of bus lock") from the i2c tree and commit
c9597d4f89565b6562bd3026adbe6eac6c317f47 ("sfc: Merge sfe4001.c into
falcon_boards.c") from the net tree.

I have applied the following merge fixup patch (after removing
drivers/net/sfc/sfe4001.c) and can carry it as necessary.

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au

From: Stephen Rothwell <sfr@canb.auug.org.au>
Date: Mon, 26 Oct 2009 12:24:55 +1100
Subject: [PATCH] net: merge fixup for drivers/net/sfc/falcon_boards.c

Signed-off-by: Stephen Rothwell <sfr@canb.auug.org.au>
---
 drivers/net/sfc/falcon_boards.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/sfc/falcon_boards.c b/drivers/net/sfc/falcon_boards.c
index 99f7372..788b336 100644
--- a/drivers/net/sfc/falcon_boards.c
+++ b/drivers/net/sfc/falcon_boards.c
@@ -327,7 +327,7 @@ static int sfn4111t_reset(struct efx_nic *efx)
 	efx_oword_t reg;
 
 	/* GPIO 3 and the GPIO register are shared with I2C, so block that */
-	mutex_lock(&efx->i2c_adap.bus_lock);
+	rt_mutex_lock(&efx->i2c_adap.bus_lock);
 
 	/* Pull RST_N (GPIO 2) low then let it up again, setting the
 	 * FLASH_CFG_1 strap (GPIO 3) appropriately.  Only change the
@@ -343,7 +343,7 @@ static int sfn4111t_reset(struct efx_nic *efx)
 	efx_writeo(efx, &reg, FR_AB_GPIO_CTL);
 	msleep(1);
 
-	mutex_unlock(&efx->i2c_adap.bus_lock);
+	rt_mutex_unlock(&efx->i2c_adap.bus_lock);
 
 	ssleep(1);
 	return 0;
-- 
1.6.5

--

[ message continues ]

Acked-by: Ben Hutchings <bhutchings@solarflare.com>

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

--

[ message continues ]

Hi Stephen,


Thanks for fixing it. The core problem here IMHO is that the sfc
network driver touches i2c internals which it would rather leave alone.
This is the only driver I know of which does this.

I can think of 3 different ways to address the issue.

Method #1: add a public API to grab/release an I2C segment.

void i2c_adapter_lock(struct i2c_adapter *adapter)
{
	rt_mutex_lock(&adapter->bus_lock);
}

void i2c_adapter_unlock(struct i2c_adapter *adapter)
{
	rt_mutex_unlock(&adapter->bus_lock);
}

It has the advantage of simplicity. The problem is that it is not
symmetric. Whatever shares the pins with I2C, I2C is considered the
main user in that its mutex is used to guarantee mutual exclusion. If
the other subsystem also has a mandatory locking mechanism, there will
have to be a decision on who is locked first. If not all drivers agree,
lockdep will get confused.

Method #2: let the driver implement its own "main" locking, and have
all pin users (i2c etc.) take it in addition to any subsystem-specific
mutex. As far as the sfc network driver is concerned, that would mean
adding pre-xfer and post-xfer hooks to i2c-algo-bit, where the "main"
mutex in question would be taken resp. released.

It has the advantage of symmetry. The problem is performance, as
regular operation will require to take and release two mutexes instead
of just one. But it is a fairly generic approach which could solve
other issues too.

Method #3: let individual bus drivers implement segment locking
themselves, with custom locking/unlocking hooks. This way, a device
sharing pins between several functions can have its own mutex
protecting these pins globally, and all user subsystems (i2c etc.) use
this single mutex.

It has the advantage of performance and symmetry, at the price of
fragility. If the i2c bus driver implements locking improperly... I am
also worried by inconsistencies that may arise, if different i2c
adapters are protected by different mutex types (mutex vs. real-time
mutex ...
[ message continues ]

I'm just a little proud of having the idea that we could avoid using an
I/O-expander on this board, but yes, the software side of this

Indirect lock operations are a recipe for deadlock, and there doesn't
seem to be any other user for this, so method 1 seems best.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

--

[ message continues ]

Hi Ben,


Well, all 3 methods rely on indirect lock operations to some degree.
But I am fine with method #1 for now. We can always move to something
more complex if the need ever arises.

What about the following patch?

From: Jean Delvare <khali@linux-fr.org>
Subject: i2c: Add an interface to lock/unlock I2C bus segment

Some drivers need to be able to prevent access to an I2C bus segment
for a specific period of time. Add an interface for them to do so
without twiddling with i2c-core internals.

Signed-off-by: Jean Delvare <khali@linux-fr.org>
Cc: Ben Hutchings <bhutchings@solarflare.com>
---
 drivers/net/sfc/sfe4001.c |    4 ++--
 include/linux/i2c.h       |   18 ++++++++++++++++++
 2 files changed, 20 insertions(+), 2 deletions(-)

--- linux-2.6.32-rc6.orig/drivers/net/sfc/sfe4001.c	2009-11-05 10:51:56.000000000 +0100
+++ linux-2.6.32-rc6/drivers/net/sfc/sfe4001.c	2009-11-05 13:40:17.000000000 +0100
@@ -188,7 +188,7 @@ static int sfn4111t_reset(struct efx_nic
 	efx_oword_t reg;
 
 	/* GPIO 3 and the GPIO register are shared with I2C, so block that */
-	mutex_lock(&efx->i2c_adap.bus_lock);
+	i2c_lock_adapter(&efx->i2c_adap);
 
 	/* Pull RST_N (GPIO 2) low then let it up again, setting the
 	 * FLASH_CFG_1 strap (GPIO 3) appropriately.  Only change the
@@ -204,7 +204,7 @@ static int sfn4111t_reset(struct efx_nic
 	falcon_write(efx, &reg, GPIO_CTL_REG_KER);
 	msleep(1);
 
-	mutex_unlock(&efx->i2c_adap.bus_lock);
+	i2c_unlock_adapter(&efx->i2c_adap);
 
 	ssleep(1);
 	return 0;
--- linux-2.6.32-rc6.orig/include/linux/i2c.h	2009-11-05 10:51:56.000000000 +0100
+++ linux-2.6.32-rc6/include/linux/i2c.h	2009-11-05 14:03:53.000000000 +0100
@@ -361,6 +361,24 @@ static inline void i2c_set_adapdata(stru
 	dev_set_drvdata(&dev->dev, data);
 }
 
+/**
+ * i2c_lock_adapter - Prevent access to an I2C bus segment
+ * @adapter: Target I2C bus segment
+ */
+static inline void i2c_lock_adapter(struct i2c_adapter *adapter)
+{
+	mutex_lock(&adapter->bus_lock);
+}
+
+/**
+ ...
[ message continues ]

On Thu, 2009-11-05 at 14:11 +0100, Jean Delvare wrote:
Acked-by: Ben Hutchings <bhutchings@solarflare.com>

Presumably this is meant for net-next-2.6, and you'll implement
i2c_{lock,unlock}_adapter() using rt_mutex in your i2c tree?

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

--

[ message continues ]

Actually I meant to push this to Linus immediately, through my i2c
tree. This is essentially a no-op: the binary code will be the same as
before the patch, so guaranteed to be safe, and this will solve

Correct.

-- 
Jean Delvare
--

[ message continues ]

Hi Jean,


Any word on this?

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/
[ message continues ]

Hi Stephen,


Haven't you read this:
http://www.spinics.net/lists/linux-next/msg07839.html

and the 3 following posts?

If you did and something is still not clear, please let me know. My
understanding is that the action token is in David's hands now.

-- 
Jean Delvare
--

[ message continues ]

From: Jean Delvare <khali@linux-fr.org>

So what exactly do I need to do?

All I see that needs to happen is to pull from Linus's tree into
net-2.6

But frankly I don't see how that helps resolve anything in linux-next
since Stephen starts with Linus's tree then pulls in everyone's work.

Or is that pull necessary so that some other patch can be applied
to net-2.6 or similar?
--

[ message continues ]

There is now a conflict between this in Linus's tree:

commit afa08974fe80c198b8650f73ed8ab59135ca10d0
Author: Jean Delvare <khali@linux-fr.org>
Date:   Sat Nov 7 13:10:46 2009 +0100

    i2c: Add an interface to lock/unlock an I2C bus segment

and this in net-next-2.6:

commit c9597d4f89565b6562bd3026adbe6eac6c317f47
Author: Ben Hutchings <bhutchings@solarflare.com>
Date:   Fri Oct 23 08:29:33 2009 +0000

    sfc: Merge sfe4001.c into falcon_boards.c

You will need to merge Linus's tree into net-next-2.6 and resolve the
conflict by applying Jean's changes to drivers/net/sfc/falcon_boards.c.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

--

[ message continues ]

From: Ben Hutchings <bhutchings@solarflare.com>

Ok, I'll sort this out after I next push to Linus, thanks!
--

[ message continues ]

Hi Jean,



OK, I will await developments :-)

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/
[ message continues ]

From: Stephen Rothwell <sfr@canb.auug.org.au>

I just took care of all of this.

Thanks.
--

[ message continues ]

Hi Dave,


Excellent, thanks.

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/
[ message continues ]

Hi Stephen, Ben,


I've merged the new API to get and release the i2c_adapter mutex:
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=afa08974fe...

Ben, you can adjust your own patches to make use of this API instead of
accessing the i2c_adapter mutex directly. That way, you are no longer
dependent of implementation changes, and this should solve the conflict.

Stephen, you can then drop your fixup patch.

Thanks,
-- 
Jean Delvare
--

[ message continues ]

I don't think so, since the conflict resulted from joining two files
including sfe4001.c in net-next-2.6.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

--

[ message continues ]

My patch series no longer touches sfe4001.c, so how would the conflict
remain?

-- 
Jean Delvare
--

[ message continues ]

Because your patch to introduce i2c_{lock,unlock}_adapter() are not in
net-next-2.6 yet.

David, you might want to pull from Linus and resolve this, giving
Stephen a break.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

--

[ message continues ]