diff mbox

Shared i2c adapter locking

Message ID 20091105141122.56b6b4f8@hyperion.delvare
State Not Applicable, archived
Delegated to: David Miller
Headers show

Commit Message

Jean Delvare Nov. 5, 2009, 1:11 p.m. UTC 
Hi Ben,

On Thu, 29 Oct 2009 15:09:36 +0000, Ben Hutchings wrote:
> On Thu, 2009-10-29 at 15:43 +0100, Jean Delvare wrote:
> > Hi Stephen,
> > 
> > On Mon, 26 Oct 2009 13:37:57 +1100, Stephen Rothwell wrote:
> > > 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.
> > 
> > Thanks for fixing it. The core problem here IMHO is that the sfc
> > network driver touches i2c internals which it would rather leave alone.
> 
> 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
> multiplexing is a hack.
> 
> > 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);
> > }
> [...]
> > I'm not really sure if I have a preference yet, so please speak up if
> > you do.
> 
> 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.

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(-)

Comments

Ben Hutchings Nov. 5, 2009, 1:57 p.m. UTC | #1 
On Thu, 2009-11-05 at 14:11 +0100, Jean Delvare wrote:
[...]
> 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>
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.
Jean Delvare Nov. 5, 2009, 2:07 p.m. UTC | #2 
On Thu, 05 Nov 2009 13:57:24 +0000, Ben Hutchings wrote:
> On Thu, 2009-11-05 at 14:11 +0100, Jean Delvare wrote:
> [...]
> > 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>
> Acked-by: Ben Hutchings <bhutchings@solarflare.com>
> 
> Presumably this is meant for net-next-2.6, and you'll implement

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
conflicts in linux-next.

> i2c_{lock,unlock}_adapter() using rt_mutex in your i2c tree?

Correct.
Stephen Rothwell Nov. 17, 2009, 8:33 a.m. UTC | #3 
Hi Jean,

On Thu, 5 Nov 2009 15:07:15 +0100 Jean Delvare <khali@linux-fr.org> wrote:
>
> > Presumably this is meant for net-next-2.6, and you'll implement
> 
> 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
> conflicts in linux-next.

Any word on this?
Jean Delvare Nov. 17, 2009, 9:35 a.m. UTC | #4 
Hi Stephen,

On Tue, 17 Nov 2009 19:33:13 +1100, Stephen Rothwell wrote:
> Hi Jean,
> 
> On Thu, 5 Nov 2009 15:07:15 +0100 Jean Delvare <khali@linux-fr.org> wrote:
> >
> > > Presumably this is meant for net-next-2.6, and you'll implement
> > 
> > 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
> > conflicts in linux-next.
> 
> Any word on this?

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.
David Miller Nov. 17, 2009, 11:51 a.m. UTC | #5 
From: Jean Delvare <khali@linux-fr.org>
Date: Tue, 17 Nov 2009 10:35:54 +0100

> 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.

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?
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ben Hutchings Nov. 17, 2009, 1:32 p.m. UTC | #6 
On Tue, 2009-11-17 at 03:51 -0800, David Miller wrote:
> From: Jean Delvare <khali@linux-fr.org>
> Date: Tue, 17 Nov 2009 10:35:54 +0100
> 
> > 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.
> 
> 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?

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.
David Miller Nov. 17, 2009, 2:13 p.m. UTC | #7 
From: Ben Hutchings <bhutchings@solarflare.com>
Date: Tue, 17 Nov 2009 13:32:54 +0000

> 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.

Ok, I'll sort this out after I next push to Linus, thanks!
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stephen Rothwell Nov. 17, 2009, 8:59 p.m. UTC | #8 
Hi Jean,

On Tue, 17 Nov 2009 10:35:54 +0100 Jean Delvare <khali@linux-fr.org> wrote:
>
> Haven't you read this:
> http://www.spinics.net/lists/linux-next/msg07839.html
> 
> and the 3 following posts?

Somehow I missed that, sorry :-(

> 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.

OK, I will await developments :-)
David Miller Nov. 19, 2009, 6:53 a.m. UTC | #9 
From: Stephen Rothwell <sfr@canb.auug.org.au>
Date: Wed, 18 Nov 2009 07:59:17 +1100

>> 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.
> 
> OK, I will await developments :-)

I just took care of all of this.

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stephen Rothwell Nov. 19, 2009, 7:05 a.m. UTC | #10 
Hi Dave,

On Wed, 18 Nov 2009 22:53:32 -0800 (PST) David Miller <davem@davemloft.net> wrote:
>
> From: Stephen Rothwell <sfr@canb.auug.org.au>
> Date: Wed, 18 Nov 2009 07:59:17 +1100
> 
> >> 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.
> > 
> > OK, I will await developments :-)
> 
> I just took care of all of this.

Excellent, thanks.
diff mbox

Patch

--- 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);
+}
+
+/**
+ * i2c_unlock_adapter - Reauthorize access to an I2C bus segment
+ * @adapter: Target I2C bus segment
+ */
+static inline void i2c_unlock_adapter(struct i2c_adapter *adapter)
+{
+	mutex_unlock(&adapter->bus_lock);
+}
+
 /*flags for the client struct: */
 #define I2C_CLIENT_PEC	0x04		/* Use Packet Error Checking */
 #define I2C_CLIENT_TEN	0x10		/* we have a ten bit chip address */