diff mbox

[08/11] i2c: rcar: remove spinlock

Message ID 1401263086-13720-9-git-send-email-wsa@the-dreams.de (mailing list archive)
State Awaiting Upstream
Headers show

Commit Message

Wolfram Sang May 28, 2014, 7:44 a.m. UTC
From: Wolfram Sang <wsa+renesas@sang-engineering.com>

The i2c core has per-adapter locks, so no need to protect again.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/i2c/busses/i2c-rcar.c | 22 ----------------------
 1 file changed, 22 deletions(-)

Comments

Sergei Shtylyov Aug. 22, 2014, 11:54 p.m. UTC | #1
Hello.

On 05/28/2014 11:44 AM, Wolfram Sang wrote:

> From: Wolfram Sang <wsa+renesas@sang-engineering.com>

> The i2c core has per-adapter locks, so no need to protect again.

    The core's lock is unable to protect from the IRQs. So I'm proposing to 
revert this patch. It's a pity I hadn't noticed this issue when the patch was 
posted.

> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>   drivers/i2c/busses/i2c-rcar.c | 22 ----------------------
>   1 file changed, 22 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
> index e82d64b5acef..4b088e67f2fd 100644
> --- a/drivers/i2c/busses/i2c-rcar.c
> +++ b/drivers/i2c/busses/i2c-rcar.c
[...]
> @@ -110,7 +109,6 @@ struct rcar_i2c_priv {
>   	struct i2c_msg	*msg;
>   	struct clk *clk;
>
> -	spinlock_t lock;

    Needed a comment (that it protects the I2C registers).

[...]
> @@ -462,21 +454,14 @@ static int rcar_i2c_master_xfer(struct i2c_adapter *adap,
>   {
>   	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 -----------------*/
> -

    I'm afraid this unlock is misplaced, the code continues to access the 
registers.

>   	ret = rcar_i2c_bus_barrier(priv);

    Should probably unlock here instead?

>   	if (ret < 0)
>   		goto out;

WBR, Sergei

--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Wolfram Sang Aug. 23, 2014, 3:08 a.m. UTC | #2
> >The i2c core has per-adapter locks, so no need to protect again.
> 
>    The core's lock is unable to protect from the IRQs. So I'm proposing to
> revert this patch. It's a pity I hadn't noticed this issue when the patch
> was posted.

Yes, you are right. I noticed a while ago, too, but then got
side-tracked :(

>    I'm afraid this unlock is misplaced, the code continues to access the
> registers.
> 
> >  	ret = rcar_i2c_bus_barrier(priv);
> 
>    Should probably unlock here instead?

I can check details next week earliest. If you are confident with your
suggestions, feel free to send one patch with the revert and the updates
you mentioned.

Thanks,

   Wolfram
Sergei Shtylyov Sept. 2, 2014, 10:10 p.m. UTC | #3
On 08/23/2014 03:54 AM, Sergei Shtylyov wrote:

>> From: Wolfram Sang <wsa+renesas@sang-engineering.com>

>> The i2c core has per-adapter locks, so no need to protect again.

>     The core's lock is unable to protect from the IRQs. So I'm proposing to
> revert this patch. It's a pity I hadn't noticed this issue when the patch was
> posted.

>> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
>> ---
>>   drivers/i2c/busses/i2c-rcar.c | 22 ----------------------
>>   1 file changed, 22 deletions(-)
>> diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
>> index e82d64b5acef..4b088e67f2fd 100644
>> --- a/drivers/i2c/busses/i2c-rcar.c
>> +++ b/drivers/i2c/busses/i2c-rcar.c
[...]
>> @@ -462,21 +454,14 @@ static int rcar_i2c_master_xfer(struct i2c_adapter *adap,
>>   {
>>       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 -----------------*/
>> -

>     I'm afraid this unlock is misplaced, the code continues to access the
> registers.

>>       ret = rcar_i2c_bus_barrier(priv);

>     Should probably unlock here instead?

    After looking at the code, it looks like it was a false alarm on my side.
The I2C interrupts are disabled here and other threads should be locked out by 
the I2C core locks. So it doesn't make sense to hold IRQs disabled during a 
possibly long polling loop in rcar_i2c_bus_barrier(). So I'll just repost the 
revert patch (if still needed).

>>       if (ret < 0)
>>           goto out;

WBR, Sergei

--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Wolfram Sang Sept. 2, 2014, 10:18 p.m. UTC | #4
>    After looking at the code, it looks like it was a false alarm on my side.
> The I2C interrupts are disabled here and other threads should be locked out
> by the I2C core locks. So it doesn't make sense to hold IRQs disabled during
> a possibly long polling loop in rcar_i2c_bus_barrier(). So I'll just repost
> the revert patch (if still needed).

Nope. Thanks for looking into it.
diff mbox

Patch

diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
index e82d64b5acef..4b088e67f2fd 100644
--- a/drivers/i2c/busses/i2c-rcar.c
+++ b/drivers/i2c/busses/i2c-rcar.c
@@ -36,7 +36,6 @@ 
 #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 */
@@ -110,7 +109,6 @@  struct rcar_i2c_priv {
 	struct i2c_msg	*msg;
 	struct clk *clk;
 
-	spinlock_t lock;
 	wait_queue_head_t wait;
 
 	int pos;
@@ -394,9 +392,6 @@  static irqreturn_t rcar_i2c_irq(int irq, void *ptr)
 	struct device *dev = rcar_i2c_priv_to_dev(priv);
 	u32 msr;
 
-	/*-------------- spin lock -----------------*/
-	spin_lock(&priv->lock);
-
 	msr = rcar_i2c_read(priv, ICMSR);
 
 	/*
@@ -450,9 +445,6 @@  out:
 		wake_up(&priv->wait);
 	}
 
-	spin_unlock(&priv->lock);
-	/*-------------- spin unlock -----------------*/
-
 	return IRQ_HANDLED;
 }
 
@@ -462,21 +454,14 @@  static int rcar_i2c_master_xfer(struct i2c_adapter *adap,
 {
 	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;
@@ -488,9 +473,6 @@  static int rcar_i2c_master_xfer(struct i2c_adapter *adap,
 			break;
 		}
 
-		/*-------------- spin lock -----------------*/
-		spin_lock_irqsave(&priv->lock, flags);
-
 		/* init each data */
 		priv->msg	= &msgs[i];
 		priv->pos	= 0;
@@ -500,9 +482,6 @@  static int rcar_i2c_master_xfer(struct i2c_adapter *adap,
 
 		ret = rcar_i2c_prepare_msg(priv);
 
-		spin_unlock_irqrestore(&priv->lock, flags);
-		/*-------------- spin unlock -----------------*/
-
 		if (ret < 0)
 			break;
 
@@ -611,7 +590,6 @@  static int rcar_i2c_probe(struct platform_device *pdev)
 
 	irq = platform_get_irq(pdev, 0);
 	init_waitqueue_head(&priv->wait);
-	spin_lock_init(&priv->lock);
 
 	adap			= &priv->adap;
 	adap->nr		= pdev->id;