[PATCH linux dev-4.13 0/3] i2c: fsi: Fix locking and simplify the driver

[PATCH linux dev-4.13 0/3] i2c: fsi: Fix locking and simplify the driver

[Eddie James]

This series fixes up the FSI-based I2C driver. Thanks to Ben H for the first
patch.

Eddie James (3):
  i2c: fsi: Don't take a lock around FSI accesses
  i2c: fsi: Use mutex lock instead of semaphore
  i2c: fsi: Use usleep instead of schedule calls

 drivers/i2c/busses/i2c-fsi.c | 107 +++++++++++--------------------------------
 1 file changed, 26 insertions(+), 81 deletions(-)

-- 
1.8.3.1

[PATCH linux dev-4.13 1/3] i2c: fsi: Don't take a lock around FSI accesses

[Eddie James]

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: Eddie James <eajames@linux.vnet.ibm.com>
---
This is Benjamin Herrenschmidt's <benh@kernel.crashing.org> patch, but I also
removed the reset_lock in the master structure and didn't add delay.h include;
thought I would just get these in one patch set for convenience.

 drivers/i2c/busses/i2c-fsi.c | 35 ++++++++++++-----------------------
 1 file changed, 12 insertions(+), 23 deletions(-)

diff --git a/drivers/i2c/busses/i2c-fsi.c b/drivers/i2c/busses/i2c-fsi.c
index 10f693f..3fcc14e 100644
--- a/drivers/i2c/busses/i2c-fsi.c
+++ b/drivers/i2c/busses/i2c-fsi.c
@@ -21,7 +21,6 @@
 #include <linux/of.h>
 #include <linux/sched.h>
 #include <linux/semaphore.h>
-#include <linux/spinlock.h>
 #include <linux/wait.h>
 
 #define FSI_ENGID_I2C		0x7
@@ -148,7 +147,6 @@ struct fsi_i2c_master {
 	struct list_head	ports;
 	wait_queue_head_t	wait;
 	struct semaphore	lock;
-	spinlock_t		reset_lock;
 };
 
 struct fsi_i2c_port {
@@ -427,71 +425,63 @@ 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);
 	if (rc)
-		goto done;
+		return rc;
 
 	/* re-init engine */
 	rc = fsi_i2c_dev_init(i2c);
 	if (rc)
-		goto done;
+		return rc;
 
 	rc = fsi_i2c_read_reg(i2c->fsi, I2C_FSI_MODE, &mode);
 	if (rc)
-		goto done;
+		return rc;
 
 	/* set port; default after reset is 0 */
 	if (port) {
 		mode = SETFIELD(I2C_MODE_PORT, mode, port);
 		rc = fsi_i2c_write_reg(i2c->fsi, I2C_FSI_MODE, &mode);
 		if (rc)
-			goto done;
+			return rc;
 	}
 
 	/* reset busy register; hw workaround */
 	dummy = I2C_PORT_BUSY_RESET;
 	rc = fsi_i2c_write_reg(i2c->fsi, I2C_FSI_PORT_BUSY, &dummy);
 	if (rc)
-		goto done;
+		return rc;
 
 	/* force bus reset */
 	rc = fsi_i2c_reset_bus(i2c);
 	if (rc)
-		goto done;
+		return rc;
 
 	/* reset errors */
 	dummy = 0;
 	rc = fsi_i2c_write_reg(i2c->fsi, I2C_FSI_RESET_ERR, &dummy);
 	if (rc)
-		goto done;
+		return rc;
 
 	/* 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)
-		goto done;
+		return rc;
 
 	if (stat & I2C_STAT_CMD_COMP)
-		goto done;
+		return rc;
 
 	/* failed to get command complete; reset engine again */
 	rc = fsi_i2c_write_reg(i2c->fsi, I2C_FSI_RESET_I2C, &dummy);
 	if (rc)
-		goto done;
+		return rc;
 
 	/* re-init engine again */
-	rc = fsi_i2c_dev_init(i2c);
-
-done:
-	spin_unlock_irqrestore(&i2c->reset_lock, flags);
-	return rc;
+	return fsi_i2c_dev_init(i2c);
 }
 
 static int fsi_i2c_abort(struct fsi_i2c_port *port, u32 status)
@@ -711,7 +701,6 @@ static int fsi_i2c_probe(struct device *dev)
 
 	init_waitqueue_head(&i2c->wait);
 	sema_init(&i2c->lock, 1);
-	spin_lock_init(&i2c->reset_lock);
 	i2c->fsi = to_fsi_dev(dev);
 	INIT_LIST_HEAD(&i2c->ports);
 
-- 
1.8.3.1

[PATCH linux dev-4.13 2/3] i2c: fsi: Use mutex lock instead of semaphore

[Eddie James]

Simplify the FSI I2C driver by using a mutex lock instead of a
semaphore. This removes the wait until timeout on the semaphore.

Signed-off-by: Eddie James <eajames@linux.vnet.ibm.com>
---
 drivers/i2c/busses/i2c-fsi.c | 46 ++++++++------------------------------------
 1 file changed, 8 insertions(+), 38 deletions(-)

diff --git a/drivers/i2c/busses/i2c-fsi.c b/drivers/i2c/busses/i2c-fsi.c
index 3fcc14e..adc2e38 100644
--- a/drivers/i2c/busses/i2c-fsi.c
+++ b/drivers/i2c/busses/i2c-fsi.c
@@ -18,10 +18,9 @@
 #include <linux/kernel.h>
 #include <linux/list.h>
 #include <linux/module.h>
+#include <linux/mutex.h>
 #include <linux/of.h>
 #include <linux/sched.h>
-#include <linux/semaphore.h>
-#include <linux/wait.h>
 
 #define FSI_ENGID_I2C		0x7
 
@@ -145,8 +144,7 @@ struct fsi_i2c_master {
 	struct fsi_device	*fsi;
 	u8			fifo_size;
 	struct list_head	ports;
-	wait_queue_head_t	wait;
-	struct semaphore	lock;
+	struct mutex		lock;
 };
 
 struct fsi_i2c_port {
@@ -180,29 +178,6 @@ static int fsi_i2c_write_reg(struct fsi_device *fsi, unsigned int reg,
 	return fsi_device_write(fsi, reg, &data_be, sizeof(data_be));
 }
 
-static int fsi_i2c_lock_master(struct fsi_i2c_master *i2c, int timeout)
-{
-	int rc;
-
-	rc = down_trylock(&i2c->lock);
-	if (!rc)
-		return 0;
-
-	rc = wait_event_interruptible_timeout(i2c->wait,
-					      !down_trylock(&i2c->lock),
-					      timeout);
-	if (rc > 0)
-		return 0;
-
-	return -EBUSY;
-}
-
-static void fsi_i2c_unlock_master(struct fsi_i2c_master *i2c)
-{
-	up(&i2c->lock);
-	wake_up(&i2c->wait);
-}
-
 static int fsi_i2c_dev_init(struct fsi_i2c_master *i2c)
 {
 	int rc;
@@ -626,11 +601,10 @@ static int fsi_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
 	int i, rc;
 	unsigned long start_time;
 	struct fsi_i2c_port *port = adap->algo_data;
+	struct fsi_i2c_master *master = port->master;
 	struct i2c_msg *msg;
 
-	rc = fsi_i2c_lock_master(port->master, adap->timeout);
-	if (rc)
-		return rc;
+	mutex_lock(&master->lock);
 
 	rc = fsi_i2c_set_port(port);
 	if (rc)
@@ -651,7 +625,7 @@ static int fsi_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
 	}
 
 unlock:
-	fsi_i2c_unlock_master(port->master);
+	mutex_unlock(&master->lock);
 	return rc;
 }
 
@@ -667,14 +641,11 @@ static int fsi_i2c_recover_bus(struct i2c_adapter *adap)
 	struct fsi_i2c_port *port = adap->algo_data;
 	struct fsi_i2c_master *master = port->master;
 
-	rc = fsi_i2c_lock_master(master, adap->timeout);
-	if (rc)
-		return rc;
+	mutex_lock(&master->lock);
 
 	rc = fsi_i2c_reset(master, port->port);
 
-	fsi_i2c_unlock_master(master);
-
+	mutex_unlock(&master->lock);
 	return rc;
 }
 
@@ -699,8 +670,7 @@ static int fsi_i2c_probe(struct device *dev)
 	if (!i2c)
 		return -ENOMEM;
 
-	init_waitqueue_head(&i2c->wait);
-	sema_init(&i2c->lock, 1);
+	mutex_init(&i2c->lock);
 	i2c->fsi = to_fsi_dev(dev);
 	INIT_LIST_HEAD(&i2c->ports);
 
-- 
1.8.3.1

[PATCH linux dev-4.13 3/3] i2c: fsi: Use usleep instead of schedule calls

[Eddie James]

Simplify the FSI I2C driver by calling usleep_range while waiting for
data to be made available instead of scheduling a timeout. The schedule
overhead isn't worth it for what should be a short time.

Signed-off-by: Eddie James <eajames@linux.vnet.ibm.com>
---
 drivers/i2c/busses/i2c-fsi.c | 26 ++++++--------------------
 1 file changed, 6 insertions(+), 20 deletions(-)

diff --git a/drivers/i2c/busses/i2c-fsi.c b/drivers/i2c/busses/i2c-fsi.c
index adc2e38..52a662c 100644
--- a/drivers/i2c/busses/i2c-fsi.c
+++ b/drivers/i2c/busses/i2c-fsi.c
@@ -20,7 +20,6 @@
 #include <linux/module.h>
 #include <linux/mutex.h>
 #include <linux/of.h>
-#include <linux/sched.h>
 
 #define FSI_ENGID_I2C		0x7
 
@@ -135,7 +134,8 @@
 
 #define I2C_PORT_BUSY_RESET	0x80000000
 
-#define I2C_LOCAL_WAIT_TIMEOUT	2		/* jiffies */
+#define I2C_LOCAL_SLEEP_MAX_US	500
+#define I2C_LOCAL_SLEEP_MIN_US	50
 
 /* choose timeout length from legacy driver; it's well tested */
 #define I2C_ABORT_TIMEOUT	msecs_to_jiffies(100)
@@ -490,10 +490,7 @@ static int fsi_i2c_abort(struct fsi_i2c_port *port, u32 status)
 		if (status & I2C_STAT_CMD_COMP)
 			return 0;
 
-		set_current_state(TASK_INTERRUPTIBLE);
-		if (schedule_timeout(I2C_LOCAL_WAIT_TIMEOUT) > 0)
-			return -EINTR;
-
+		usleep_range(I2C_LOCAL_SLEEP_MIN_US, I2C_LOCAL_SLEEP_MAX_US);
 	} while (time_after(start + I2C_ABORT_TIMEOUT, jiffies));
 
 	return -ETIME;
@@ -556,7 +553,7 @@ static int fsi_i2c_wait(struct fsi_i2c_port *port, struct i2c_msg *msg,
 			unsigned long timeout)
 {
 	u32 status = 0;
-	int rc, rc_abort;
+	int rc;
 	unsigned long start = jiffies;
 
 	do {
@@ -578,21 +575,10 @@ static int fsi_i2c_wait(struct fsi_i2c_port *port, struct i2c_msg *msg,
 			continue;
 		}
 
-		set_current_state(TASK_INTERRUPTIBLE);
-		if (schedule_timeout(I2C_LOCAL_WAIT_TIMEOUT) > 0) {
-			rc = -EINTR;
-			goto abort;
-		}
+		usleep_range(I2C_LOCAL_SLEEP_MIN_US, I2C_LOCAL_SLEEP_MAX_US);
 	} while (time_after(start + timeout, jiffies));
 
-	rc = -ETIME;
-
-abort:
-	rc_abort = fsi_i2c_abort(port, status);
-	if (rc_abort)
-		return rc_abort;
-
-	return rc;
+	return -ETIME;
 }
 
 static int fsi_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
-- 
1.8.3.1

Re: [PATCH linux dev-4.13 0/3] i2c: fsi: Fix locking and simplify the driver

[Joel Stanley]

On 30 May 2018 at 04:46, Eddie James <eajames@linux.vnet.ibm.com> wrote:
> This series fixes up the FSI-based I2C driver. Thanks to Ben H for the first
> patch.

Thanks for the series. I noticed you got some comments on the version
you sent upstream, so I'll let you re-spin this series to match those
comments.

On Monday I'll apply the upstream series to the dev-4.17 tree.
However, it would be nice to get these fixes into the 4.13 branch
before we move on.

Cheers,

Joel