openbmc.lists.ozlabs.org archive mirror
 help / color / mirror / Atom feed
From: Eddie James <eajames@linux.vnet.ibm.com>
To: openbmc@lists.ozlabs.org
Cc: joel@jms.id.au, benh@kernel.crashing.org,
	Eddie James <eajames@linux.vnet.ibm.com>
Subject: [PATCH linux dev-4.13 1/3] i2c: fsi: Don't take a lock around FSI accesses
Date: Tue, 29 May 2018 14:16:36 -0500	[thread overview]
Message-ID: <1527621398-26526-2-git-send-email-eajames@linux.vnet.ibm.com> (raw)
In-Reply-To: <1527621398-26526-1-git-send-email-eajames@linux.vnet.ibm.com>

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

  reply	other threads:[~2018-05-29 19:16 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-05-29 19:16 [PATCH linux dev-4.13 0/3] i2c: fsi: Fix locking and simplify the driver Eddie James
2018-05-29 19:16 ` Eddie James [this message]
2018-05-29 19:16 ` [PATCH linux dev-4.13 2/3] i2c: fsi: Use mutex lock instead of semaphore Eddie James
2018-05-29 19:16 ` [PATCH linux dev-4.13 3/3] i2c: fsi: Use usleep instead of schedule calls Eddie James
2018-05-30 14:21 ` [PATCH linux dev-4.13 0/3] i2c: fsi: Fix locking and simplify the driver Joel Stanley

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1527621398-26526-2-git-send-email-eajames@linux.vnet.ibm.com \
    --to=eajames@linux.vnet.ibm.com \
    --cc=benh@kernel.crashing.org \
    --cc=joel@jms.id.au \
    --cc=openbmc@lists.ozlabs.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).