Skip to contentSkip to conversation

i2c: fsi: Don't take a lock around FSI accesses

PATCH In review Benjamin Herrenschmidt 2 participants 3 messages May 29, 2018

Subsystems (2)
I2C subsystem host drivers FSI-attached I2C driver
lore
Share & tools
lore

Benjamin Herrenschmidt

author
#0 lore raw

PATCH 1/1 i2c: fsi: Don't take a lock around FSI accesses

That lock is incorrect as FSI can sleep and thus will trigger lockdep warnings. It is also unnecessary as it was meant to "speed up" the reset process for unspecified reasons, not to protect against anything.

The the master semaphore should prevent any concurrent user from seeing the intermediate state already and the recent improvements to the FSI layer should take care of potential speed issues.

Signed-off-by Benjamin Herrenschmidt <[email protected]>
---
 drivers/i2c/busses/i2c-fsi.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)
drivers/i2c/busses/i2c-fsi.c +2 -6
index 10f693fc855b..41cbffea4325 100644
@@ -23,6 +23,7 @@
 #include <linux/semaphore.h>
 #include <linux/spinlock.h>
 #include <linux/wait.h>
+#include <linux/delay.h>
 
 #define FSI_ENGID_I2C		0x7
 

@@ -427,10 +428,6 @@ static int fsi_i2c_reset(struct fsi_i2c_master *i2c, u16 port)
 {
 	int rc;
 	u32 mode, stat, dummy = 0;
-	unsigned long flags;
-
-	/* disable pre-emption; bus won't get left in a bad state for long */
-	spin_lock_irqsave(&i2c->reset_lock, flags);
 
 	/* reset engine */
 	rc = fsi_i2c_write_reg(i2c->fsi, I2C_FSI_RESET_I2C, &dummy);

@@ -472,7 +469,7 @@ static int fsi_i2c_reset(struct fsi_i2c_master *i2c, u16 port)
 		goto done;
 
 	/* wait for command complete; time from legacy driver */
-	udelay(1000);
+	msleep(1);
 
 	rc = fsi_i2c_read_reg(i2c->fsi, I2C_FSI_STAT, &stat);
 	if (rc)

@@ -490,7 +487,6 @@ static int fsi_i2c_reset(struct fsi_i2c_master *i2c, u16 port)
 	rc = fsi_i2c_dev_init(i2c);
 
 done:
-	spin_unlock_irqrestore(&i2c->reset_lock, flags);
 	return rc;
 }
Signature · 1 line
2.17.0
#1 lore raw
Quoted 19 lines · Benjamin Herrenschmidt

That lock is incorrect as FSI can sleep and thus will trigger lockdep warnings. It is also unnecessary as it was meant to "speed up" the reset process for unspecified reasons, not to protect against anything.

The the master semaphore should prevent any concurrent user from seeing the intermediate state already and the recent improvements to the FSI layer should take care of potential speed issues.

Signed-off-by Benjamin Herrenschmidt <[email protected]>
---
  drivers/i2c/busses/i2c-fsi.c | 8 ++------
  1 file changed, 2 insertions(+), 6 deletions(-)
drivers/i2c/busses/i2c-fsi.c +0 -0
index 10f693fc855b..41cbffea4325 100644

drivers/i2c/busses/i2c-fsi.c line 26

Quoted 4 lines
drivers/i2c/busses/i2c-fsi.c +1 -0
@@ -23,6 +23,7 @@
  #include <linux/semaphore.h>
  #include <linux/spinlock.h>
  #include <linux/wait.h>
+#include <linux/delay.h>

delay.h was already included :)

And probably need to remove the spinlock from the master structure, as it's unused.

Thanks,
Eddie

Quoted 31 lines
drivers/i2c/busses/i2c-fsi.c +1 -6
  #define FSI_ENGID_I2C		0x7
 

@@ -427,10 +428,6 @@ static int fsi_i2c_reset(struct fsi_i2c_master *i2c, u16 port)
  {
  	int rc;
  	u32 mode, stat, dummy = 0;
-	unsigned long flags;
-
-	/* disable pre-emption; bus won't get left in a bad state for long */
-	spin_lock_irqsave(&i2c->reset_lock, flags);
 
  	/* reset engine */
  	rc = fsi_i2c_write_reg(i2c->fsi, I2C_FSI_RESET_I2C, &dummy);

@@ -472,7 +469,7 @@ static int fsi_i2c_reset(struct fsi_i2c_master *i2c, u16 port)
  		goto done;
 
  	/* wait for command complete; time from legacy driver */
-	udelay(1000);
+	msleep(1);
 
  	rc = fsi_i2c_read_reg(i2c->fsi, I2C_FSI_STAT, &stat);
  	if (rc)

@@ -490,7 +487,6 @@ static int fsi_i2c_reset(struct fsi_i2c_master *i2c, u16 port)
  	rc = fsi_i2c_dev_init(i2c);
 
  done:
-	spin_unlock_irqrestore(&i2c->reset_lock, flags);
  	return rc;
  }
author
#2 lore raw
Quoted 28 lines · Eddie James
Quoted 19 lines · Benjamin Herrenschmidt

That lock is incorrect as FSI can sleep and thus will trigger lockdep warnings. It is also unnecessary as it was meant to "speed up" the reset process for unspecified reasons, not to protect against anything.

The the master semaphore should prevent any concurrent user from seeing the intermediate state already and the recent improvements to the FSI layer should take care of potential speed issues.

Signed-off-by Benjamin Herrenschmidt <[email protected]>
---
  drivers/i2c/busses/i2c-fsi.c | 8 ++------
  1 file changed, 2 insertions(+), 6 deletions(-)
drivers/i2c/busses/i2c-fsi.c +0 -0
index 10f693fc855b..41cbffea4325 100644

drivers/i2c/busses/i2c-fsi.c line 26

Quoted 4 lines
drivers/i2c/busses/i2c-fsi.c +1 -0
@@ -23,6 +23,7 @@
  #include <linux/semaphore.h>
  #include <linux/spinlock.h>
  #include <linux/wait.h>
+#include <linux/delay.h>

delay.h was already included :)

Quoted 2 lines

And probably need to remove the spinlock from the master structure, as it's unused.

Ah right.

Cheers,
Ben.

Quoted 34 lines

Thanks,
Eddie

Quoted 31 lines
drivers/i2c/busses/i2c-fsi.c +1 -6
  #define FSI_ENGID_I2C		0x7
 

@@ -427,10 +428,6 @@ static int fsi_i2c_reset(struct fsi_i2c_master *i2c, u16 port)
  {
  	int rc;
  	u32 mode, stat, dummy = 0;
-	unsigned long flags;
-
-	/* disable pre-emption; bus won't get left in a bad state for long */
-	spin_lock_irqsave(&i2c->reset_lock, flags);
 
  	/* reset engine */
  	rc = fsi_i2c_write_reg(i2c->fsi, I2C_FSI_RESET_I2C, &dummy);

@@ -472,7 +469,7 @@ static int fsi_i2c_reset(struct fsi_i2c_master *i2c, u16 port)
  		goto done;
 
  	/* wait for command complete; time from legacy driver */
-	udelay(1000);
+	msleep(1);
 
  	rc = fsi_i2c_read_reg(i2c->fsi, I2C_FSI_STAT, &stat);
  	if (rc)

@@ -490,7 +487,6 @@ static int fsi_i2c_reset(struct fsi_i2c_master *i2c, u16 port)
  	rc = fsi_i2c_dev_init(i2c);
 
  done:
-	spin_unlock_irqrestore(&i2c->reset_lock, flags);
  	return rc;
  }