| Message ID | 1401263086-13720-9-git-send-email-wsa@the-dreams.de |
|---|---|
| State | Accepted |
| Headers | show |
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-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> >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
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-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> 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 --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;