[PATCH v2] Revert "i2c: rcar: remove spinlock"

[PATCH v2] Revert "i2c: rcar: remove spinlock"

[Sergei Shtylyov]

This reverts commit 150b8be3cda54412ad7b54f5392b513b25c0aaa7.

The I2C core's per-adapter locks can't protect from IRQs, so the driver still
needs a spinlock to protect the register accesses.

Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
Cc: stable@vger.kernel.org # 3.16+

---
The patch is against Wolfram Sang's 'linux.git' repo.

Changes in version 2:
- fixed summary.

 drivers/i2c/busses/i2c-rcar.c |   22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

Index: linux/drivers/i2c/busses/i2c-rcar.c
===================================================================
--- linux.orig/drivers/i2c/busses/i2c-rcar.c
+++ linux/drivers/i2c/busses/i2c-rcar.c
@@ -34,6 +34,7 @@
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
 #include <linux/slab.h>
+#include <linux/spinlock.h>
 
 /* register offsets */
 #define ICSCR	0x00	/* slave ctrl */
@@ -95,6 +96,7 @@ struct rcar_i2c_priv {
 	struct i2c_msg	*msg;
 	struct clk *clk;
 
+	spinlock_t lock;
 	wait_queue_head_t wait;
 
 	int pos;
@@ -365,6 +367,9 @@ static irqreturn_t rcar_i2c_irq(int irq,
 	struct rcar_i2c_priv *priv = ptr;
 	u32 msr;
 
+	/*-------------- spin lock -----------------*/
+	spin_lock(&priv->lock);
+
 	msr = rcar_i2c_read(priv, ICMSR);
 
 	/* Arbitration lost */
@@ -400,6 +405,9 @@ out:
 		wake_up(&priv->wait);
 	}
 
+	spin_unlock(&priv->lock);
+	/*-------------- spin unlock -----------------*/
+
 	return IRQ_HANDLED;
 }
 
@@ -409,14 +417,21 @@ static int rcar_i2c_master_xfer(struct i
 {
 	struct rcar_i2c_priv *priv = i2c_get_adapdata(adap);
 	struct device *dev = rcar_i2c_priv_to_dev(priv);
+	unsigned long flags;
 	int i, ret, timeout;
 
 	pm_runtime_get_sync(dev);
 
+	/*-------------- spin lock -----------------*/
+	spin_lock_irqsave(&priv->lock, flags);
+
 	rcar_i2c_init(priv);
 	/* start clock */
 	rcar_i2c_write(priv, ICCCR, priv->icccr);
 
+	spin_unlock_irqrestore(&priv->lock, flags);
+	/*-------------- spin unlock -----------------*/
+
 	ret = rcar_i2c_bus_barrier(priv);
 	if (ret < 0)
 		goto out;
@@ -428,6 +443,9 @@ static int rcar_i2c_master_xfer(struct i
 			break;
 		}
 
+		/*-------------- spin lock -----------------*/
+		spin_lock_irqsave(&priv->lock, flags);
+
 		/* init each data */
 		priv->msg	= &msgs[i];
 		priv->pos	= 0;
@@ -437,6 +455,9 @@ static int rcar_i2c_master_xfer(struct i
 
 		ret = rcar_i2c_prepare_msg(priv);
 
+		spin_unlock_irqrestore(&priv->lock, flags);
+		/*-------------- spin unlock -----------------*/
+
 		if (ret < 0)
 			break;
 
@@ -540,6 +561,7 @@ static int rcar_i2c_probe(struct platfor
 
 	irq = platform_get_irq(pdev, 0);
 	init_waitqueue_head(&priv->wait);
+	spin_lock_init(&priv->lock);
 
 	adap = &priv->adap;
 	adap->nr = pdev->id;

Re: [PATCH v2] Revert "i2c: rcar: remove spinlock"

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 386 bytes --]

On Sun, Aug 24, 2014 at 12:44:09AM +0400, Sergei Shtylyov wrote:
> This reverts commit 150b8be3cda54412ad7b54f5392b513b25c0aaa7.
> 
> The I2C core's per-adapter locks can't protect from IRQs, so the driver still
> needs a spinlock to protect the register accesses.

Looks to me like a plain revert. What about the issues you mentioned?
Don't you want to fix them? Confused...


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v2] Revert "i2c: rcar: remove spinlock"

[Sergei Shtylyov]

Hello.

On 8/24/2014 10:45 AM, Wolfram Sang wrote:

>> This reverts commit 150b8be3cda54412ad7b54f5392b513b25c0aaa7.

>> The I2C core's per-adapter locks can't protect from IRQs, so the driver still
>> needs a spinlock to protect the register accesses.

> Looks to me like a plain revert. What about the issues you mentioned?
> Don't you want to fix them? Confused...

    You told me to do 2 patches, one of them just revert. Here you are. As for 
the follow up patch, I need to think a bit more, so all in a good time.

WBR, Sergei

Re: [PATCH v2] Revert "i2c: rcar: remove spinlock"

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 388 bytes --]


>    You told me to do 2 patches, one of them just revert.

No no, I asked for one patch having both, see: "... feel free to send
one patch with the revert and the updates you mentioned."

> for the follow up patch, I need to think a bit more, so all in a good time.

OK, I'll wait for the proper solution from you or do it on my own
somewhen, whatever comes first.

Thanks,

   Wolfram

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v2] Revert "i2c: rcar: remove spinlock"

[Sergei Shtylyov]

Hello.

On 8/25/2014 7:40 AM, Wolfram Sang wrote:

>>     You told me to do 2 patches, one of them just revert.

> No no, I asked for one patch having both, see: "... feel free to send
> one patch with the revert and the updates you mentioned."

    Ah, I misunderstood you. However, I don't quite understand why you want to 
combine revert and the mentioned updates...

> Thanks,

>     Wolfram

WBR, Sergei

Re: [PATCH v2] Revert "i2c: rcar: remove spinlock"

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 261 bytes --]

>    Ah, I misunderstood you. However, I don't quite understand why you want
> to combine revert and the mentioned updates...

As I understood from your previous mail, a plain revert is less broken
but still broken. Applying a correct fix is my preferred way.


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v2] Revert "i2c: rcar: remove spinlock"

[Sergei Shtylyov]

Hello.

On 08/25/2014 06:33 PM, Wolfram Sang wrote:

>>     Ah, I misunderstood you. However, I don't quite understand why you want
>> to combine revert and the mentioned updates...

> As I understood from your previous mail, a plain revert is less broken
> but still broken. Applying a correct fix is my preferred way.

    Actually, from the -stable kernel perspective, a single patch would 
complicate backporting. So could you please reconsider?

WBR, Sergei

Re: [PATCH v2] Revert "i2c: rcar: remove spinlock"

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 471 bytes --]


> >As I understood from your previous mail, a plain revert is less broken
> >but still broken. Applying a correct fix is my preferred way.
> 
>    Actually, from the -stable kernel perspective, a single patch would
> complicate backporting. So could you please reconsider?

I don't see why. If we have two patches, the state inbetween them is
broken. And we don't have two patches yet, just the revert. So, the
result is broken anyhow.

Can you elaborate?


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v2] Revert "i2c: rcar: remove spinlock"

[Sergei Shtylyov]

On 09/02/2014 09:18 PM, Wolfram Sang wrote:

>>> As I understood from your previous mail, a plain revert is less broken
>>> but still broken. Applying a correct fix is my preferred way.

>>     Actually, from the -stable kernel perspective, a single patch would
>> complicate backporting. So could you please reconsider?

> I don't see why. If we have two patches, the state inbetween them is
> broken.

    Even so, it has always been broken, we don't make it more broken by 
reverting your change.

> And we don't have two patches yet, just the revert. So, the

    I'm going to consider the spinlock issue ASAP, after I check whether the 
I2C clock frequency really has any influence on the unexpected read NACK issue 
I've been chasing for several days.

> result is broken anyhow.

> Can you elaborate?

    Your patch removing the spinlock went into 3.16 only but we'd have to 
backport the assumed single patch to the -stable kernels older than that. This 
means that I'd have to provide the "delta" patch (i.e. the separate patch that 
I'd like to provide now atop of the revert) for these kernels instead since 
the original single patch wouldn't apply anyway.

WBR, Sergei

Re: [PATCH v2] Revert "i2c: rcar: remove spinlock"

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1313 bytes --]


> >I don't see why. If we have two patches, the state inbetween them is
> >broken.
> 
>    Even so, it has always been broken, we don't make it more broken by
> reverting your change.

Yes. Still, if I send something to *stable*, less broken is not an
option for me, if I know there is a fix possible. And we are at -rc3
now, so there is still time for that.

> >And we don't have two patches yet, just the revert. So, the
> 
>    I'm going to consider the spinlock issue ASAP, after I check whether the
> I2C clock frequency really has any influence on the unexpected read NACK
> issue I've been chasing for several days.

Good luck with that! Such bugs are truly annoying :(

>    Your patch removing the spinlock went into 3.16 only but we'd have to
> backport the assumed single patch to the -stable kernels older than that.
> This means that I'd have to provide the "delta" patch (i.e. the separate
> patch that I'd like to provide now atop of the revert) for these kernels
> instead since the original single patch wouldn't apply anyway.

With all my changes in 3.16, I wonder if the patch with your addition to
the revert will apply anyhow. But, okay, you send two patches, and I
will decide how I apply them and deal with delta-patches. Okay?

All the best,

   Wolfram


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v2] Revert "i2c: rcar: remove spinlock"

[Sergei Shtylyov]

On 09/02/2014 09:45 PM, Wolfram Sang wrote:

>>> I don't see why. If we have two patches, the state inbetween them is
>>> broken.

>>     Even so, it has always been broken, we don't make it more broken by
>> reverting your change.

> Yes. Still, if I send something to *stable*, less broken is not an
> option for me, if I know there is a fix possible. And we are at -rc3
> now, so there is still time for that.

    Not as much time for me since I'm leaving for vacations at the start of 
the next week. And I have other areas to care about before that.

>>> And we don't have two patches yet, just the revert. So, the

>>     I'm going to consider the spinlock issue ASAP, after I check whether the
>> I2C clock frequency really has any influence on the unexpected read NACK
>> issue I've been chasing for several days.

> Good luck with that! Such bugs are truly annoying :(

   Sigh, that whole issue looks like hardware bug to me (the vendor kernel 
reportedly doesn't have it though)... overall, this hardware seems excessively 
buggy, despite I2C protocol is not really rocket science.

>>     Your patch removing the spinlock went into 3.16 only but we'd have to
>> backport the assumed single patch to the -stable kernels older than that.
>> This means that I'd have to provide the "delta" patch (i.e. the separate
>> patch that I'd like to provide now atop of the revert) for these kernels
>> instead since the original single patch wouldn't apply anyway.

> With all my changes in 3.16, I wonder if the patch with your addition to
> the revert will apply anyhow.

    Resolution of the context issues is different from the patch principally 
not applying as the changes it has are already there.

> But, okay, you send two patches, and I
> will decide how I apply them and deal with delta-patches. Okay?

    So you mean that I need to repost the revert? I'm OK with that since the 
context has most probably changed with my recent patch...

> All the best,

>     Wolfram

WBR, Sergei

Re: [PATCH v2] Revert "i2c: rcar: remove spinlock"

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 424 bytes --]

On Sun, Aug 24, 2014 at 12:44:09AM +0400, Sergei Shtylyov wrote:
> This reverts commit 150b8be3cda54412ad7b54f5392b513b25c0aaa7.
> 
> The I2C core's per-adapter locks can't protect from IRQs, so the driver still
> needs a spinlock to protect the register accesses.
> 
> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> Cc: stable@vger.kernel.org # 3.16+
> 

Applied to for-current, thanks!


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]