[PATCH linux dev-4.10 v3 00/31] drivers: fsi: client fixes and refactor

[PATCH linux dev-4.10 v3 00/31] drivers: fsi: client fixes and refactor

[Eddie James]

From: "Edward A. James" <eajames@us.ibm.com>

This series refactors the FSI client device drivers. It adds a number of fixes
discovered from bugs over the past several months. Most critically, it fixes
various race conditions in the unbind/remove pathway of the occ-hwmon, occ,
and sbefifo drivers.

Changes since v2:
 * split "General clean-up" patches into many more patches
 * Change to wake_up_all() in various places
 * Set sbe pointer NULL and add check in occ-hwmon. For race safety.

Changes since v1:
 * split into multiple patches.
 * add memory management patch.

Edward A. James (31):
  drivers: fsi: sbefifo: Fix includes
  drivers: fsi: sbefifo: Use a defined reschedule length
  drivers: fsi: sbefifo: Use __be32 for big endian values
  drivers: fsi: sbefifo: white space fixes
  drivers: fsi: sbefifo: replace awkward wait_event expression
  drivers: fsi: sbefifo: remove redundant function
  drivers: fsi: sbefifo: Use goto to reduce put statements
  drivers: fsi: sbefifo: Do an earlier get_client call
  drivers: fsi: sbefifo: Remove warning and user data access check
  drivers: fsi: sbefifo: destroy the ida list on exit
  drivers: fsi: sbefifo: Fix module authors and comments
  drivers: fsi: sbefifo: Fix include guards in header file
  drivers: fsi: SBEFIFO: Fix probe() and remove()
  drivers: fsi: SBEFIFO: check for xfr complete in read wait_event
  drivers: fsi: occ: Fix includes
  drivers: fsi: occ: Fix errant kfree calls
  drivers: fsi: occ: remove unused occ_command structure
  drivers: fsi: occ: Use big-endian values
  drivers: fsi: occ: Return ENODEV if client is NULL
  drivers: fsi: occ: Remove early user buffer checking
  drivers: fsi: occ: Switch to more logical errnos
  drivers: fsi: occ: fix white space and bracket problems
  drivers: fsi: occ: Destroy the ida list on exit
  drivers: fsi: occ: Remove unnecessary platform_set_drvdata call
  drivers: fsi: occ: Add comments for clarity
  drivers: fsi: occ: Add OCC response definitions to header
  drivers: fsi: occ: Poll while receiving "command in progress"
  drivers: fsi: occ: Add cancel to remove() and fix probe()
  drivers: fsi: occ: Fix client memory management
  drivers/hwmon/occ: Remove repeated ops for OCC command in progress
  drivers: hwmon: occ: Cancel occ operations in remove()

 drivers/fsi/fsi-sbefifo.c   | 311 ++++++++++++++++++++++------------------
 drivers/fsi/occ.c           | 340 ++++++++++++++++++++++++++------------------
 drivers/hwmon/occ/p9_sbe.c  |  47 +++---
 include/linux/fsi-sbefifo.h |   6 +-
 include/linux/occ.h         |  20 ++-
 5 files changed, 423 insertions(+), 301 deletions(-)

-- 
1.8.3.1

[PATCH linux dev-4.10 v3 01/31] drivers: fsi: sbefifo: Fix includes

[Eddie James]

From: "Edward A. James" <eajames@us.ibm.com>

Need to include everything we use and not rely on recursive includes.

Signed-off-by: Edward A. James <eajames@us.ibm.com>
---
 drivers/fsi/fsi-sbefifo.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/fsi/fsi-sbefifo.c b/drivers/fsi/fsi-sbefifo.c
index 1c37ff7..6fa75a8 100644
--- a/drivers/fsi/fsi-sbefifo.c
+++ b/drivers/fsi/fsi-sbefifo.c
@@ -11,20 +11,27 @@
  * GNU General Public License for more details.
  */
 
-#include <linux/delay.h>
+#include <linux/device.h>
 #include <linux/errno.h>
-#include <linux/idr.h>
+#include <linux/fs.h>
 #include <linux/fsi.h>
+#include <linux/fsi-sbefifo.h>
+#include <linux/idr.h>
+#include <linux/kernel.h>
+#include <linux/kref.h>
 #include <linux/list.h>
 #include <linux/miscdevice.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/of_device.h>
 #include <linux/of_platform.h>
 #include <linux/poll.h>
 #include <linux/sched.h>
 #include <linux/slab.h>
+#include <linux/spinlock.h>
 #include <linux/timer.h>
 #include <linux/uaccess.h>
+#include <linux/wait.h>
 
 /*
  * The SBEFIFO is a pipe-like FSI device for communicating with
-- 
1.8.3.1

[PATCH linux dev-4.10 v3 02/31] drivers: fsi: sbefifo: Use a defined reschedule length

[Eddie James]

From: "Edward A. James" <eajames@us.ibm.com>

Switch to a #defined reschedule period instead of using 500 ms twice.

Signed-off-by: Edward A. James <eajames@us.ibm.com>
---
 drivers/fsi/fsi-sbefifo.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/fsi/fsi-sbefifo.c b/drivers/fsi/fsi-sbefifo.c
index 6fa75a8..2e45a75 100644
--- a/drivers/fsi/fsi-sbefifo.c
+++ b/drivers/fsi/fsi-sbefifo.c
@@ -51,6 +51,8 @@
 #define   SBEFIFO_EOT_MAGIC		0xffffffff
 #define SBEFIFO_EOT_ACK		0x14
 
+#define SBEFIFO_RESCHEDULE	msecs_to_jiffies(500)
+
 struct sbefifo {
 	struct timer_list poll_timer;
 	struct fsi_device *fsi_dev;
@@ -404,7 +406,7 @@ static void sbefifo_poll_timer(unsigned long data)
 		if (devn == 0) {
 			/* No open slot for write.  Reschedule. */
 			sbefifo->poll_timer.expires = jiffies +
-				msecs_to_jiffies(500);
+				SBEFIFO_RESCHEDULE;
 			add_timer(&sbefifo->poll_timer);
 			goto out_unlock;
 		}
@@ -445,7 +447,7 @@ static void sbefifo_poll_timer(unsigned long data)
 		if (devn == 0) {
 			/* No data yet.  Reschedule. */
 			sbefifo->poll_timer.expires = jiffies +
-				msecs_to_jiffies(500);
+				SBEFIFO_RESCHEDULE;
 			add_timer(&sbefifo->poll_timer);
 			goto out_unlock;
 		}
-- 
1.8.3.1

[PATCH linux dev-4.10 v3 03/31] drivers: fsi: sbefifo: Use __be32 for big endian values

[Eddie James]

From: "Edward A. James" <eajames@us.ibm.com>

Use a real __be32 value instead of u32 for big endian values.

Signed-off-by: Edward A. James <eajames@us.ibm.com>
---
 drivers/fsi/fsi-sbefifo.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/fsi/fsi-sbefifo.c b/drivers/fsi/fsi-sbefifo.c
index 2e45a75..c5eeace 100644
--- a/drivers/fsi/fsi-sbefifo.c
+++ b/drivers/fsi/fsi-sbefifo.c
@@ -103,7 +103,7 @@ struct sbefifo_client {
 static int sbefifo_inw(struct sbefifo *sbefifo, int reg, u32 *word)
 {
 	int rc;
-	u32 raw_word;
+	__be32 raw_word;
 
 	rc = fsi_device_read(sbefifo->fsi_dev, reg, &raw_word,
 			     sizeof(raw_word));
@@ -111,17 +111,19 @@ static int sbefifo_inw(struct sbefifo *sbefifo, int reg, u32 *word)
 		return rc;
 
 	*word = be32_to_cpu(raw_word);
+
 	return 0;
 }
 
 static int sbefifo_outw(struct sbefifo *sbefifo, int reg, u32 word)
 {
-	u32 raw_word = cpu_to_be32(word);
+	__be32 raw_word = cpu_to_be32(word);
 
 	return fsi_device_write(sbefifo->fsi_dev, reg, &raw_word,
 				sizeof(raw_word));
 }
 
+/* Don't flip endianness of data to/from FIFO, just pass through. */
 static int sbefifo_readw(struct sbefifo *sbefifo, u32 *word)
 {
 	return fsi_device_read(sbefifo->fsi_dev, SBEFIFO_DWN, word,
-- 
1.8.3.1

[PATCH linux dev-4.10 v3 04/31] drivers: fsi: sbefifo: white space fixes

[Eddie James]

From: "Edward A. James" <eajames@us.ibm.com>

Fix some spacing issues.

Signed-off-by: Edward A. James <eajames@us.ibm.com>
---
 drivers/fsi/fsi-sbefifo.c | 66 ++++++++++++++++++++++++-----------------------
 1 file changed, 34 insertions(+), 32 deletions(-)

diff --git a/drivers/fsi/fsi-sbefifo.c b/drivers/fsi/fsi-sbefifo.c
index c5eeace..561a7f6 100644
--- a/drivers/fsi/fsi-sbefifo.c
+++ b/drivers/fsi/fsi-sbefifo.c
@@ -147,7 +147,7 @@ static int sbefifo_ack_eot(struct sbefifo *sbefifo)
 		return ret;
 
 	return sbefifo_outw(sbefifo, SBEFIFO_DWN | SBEFIFO_EOT_ACK,
-			SBEFIFO_EOT_MAGIC);
+			    SBEFIFO_EOT_MAGIC);
 }
 
 static size_t sbefifo_dev_nwreadable(u32 sts)
@@ -221,6 +221,7 @@ static bool sbefifo_buf_readnb(struct sbefifo_buf *buf, size_t n)
 		rpos = buf->buf;
 
 	WRITE_ONCE(buf->rpos, rpos);
+
 	return rpos == wpos;
 }
 
@@ -240,14 +241,14 @@ static bool sbefifo_buf_wrotenb(struct sbefifo_buf *buf, size_t n)
 		set_bit(SBEFIFO_BUF_FULL, &buf->flags);
 
 	WRITE_ONCE(buf->wpos, wpos);
+
 	return rpos == wpos;
 }
 
 static void sbefifo_free(struct kref *kref)
 {
-	struct sbefifo *sbefifo;
+	struct sbefifo *sbefifo = container_of(kref, struct sbefifo, kref);
 
-	sbefifo = container_of(kref, struct sbefifo, kref);
 	kfree(sbefifo);
 }
 
@@ -284,8 +285,7 @@ static struct sbefifo_xfr *sbefifo_client_next_xfr(
 	if (list_empty(&client->xfrs))
 		return NULL;
 
-	return container_of(client->xfrs.next, struct sbefifo_xfr,
-			client);
+	return container_of(client->xfrs.next, struct sbefifo_xfr, client);
 }
 
 static bool sbefifo_xfr_rsp_pending(struct sbefifo_client *client)
@@ -360,6 +360,7 @@ static struct sbefifo_xfr *sbefifo_next_xfr(struct sbefifo *sbefifo)
 			kfree(xfr);
 			continue;
 		}
+
 		return xfr;
 	}
 
@@ -381,7 +382,7 @@ static void sbefifo_poll_timer(unsigned long data)
 
 	spin_lock(&sbefifo->lock);
 	xfr = list_first_entry_or_null(&sbefifo->xfrs, struct sbefifo_xfr,
-			xfrs);
+				       xfrs);
 	if (!xfr)
 		goto out_unlock;
 
@@ -399,8 +400,7 @@ static void sbefifo_poll_timer(unsigned long data)
 
 	 /* Drain the write buffer. */
 	while ((bufn = sbefifo_buf_nbreadable(wbuf))) {
-		ret = sbefifo_inw(sbefifo, SBEFIFO_UP | SBEFIFO_STS,
-				&sts);
+		ret = sbefifo_inw(sbefifo, SBEFIFO_UP | SBEFIFO_STS, &sts);
 		if (ret)
 			goto out;
 
@@ -425,9 +425,8 @@ static void sbefifo_poll_timer(unsigned long data)
 
 	 /* Send EOT if the writer is finished. */
 	if (test_and_clear_bit(SBEFIFO_XFR_WRITE_DONE, &xfr->flags)) {
-		ret = sbefifo_outw(sbefifo,
-				SBEFIFO_UP | SBEFIFO_EOT_RAISE,
-				SBEFIFO_EOT_MAGIC);
+		ret = sbefifo_outw(sbefifo, SBEFIFO_UP | SBEFIFO_EOT_RAISE,
+				   SBEFIFO_EOT_MAGIC);
 		if (ret)
 			goto out;
 
@@ -477,7 +476,7 @@ static void sbefifo_poll_timer(unsigned long data)
 			set_bit(SBEFIFO_XFR_COMPLETE, &xfr->flags);
 			list_del(&xfr->xfrs);
 			if (unlikely(test_bit(SBEFIFO_XFR_CANCEL,
-							&xfr->flags)))
+					      &xfr->flags)))
 				kfree(xfr);
 			break;
 		}
@@ -487,7 +486,7 @@ static void sbefifo_poll_timer(unsigned long data)
 	if (unlikely(ret)) {
 		sbefifo->rc = ret;
 		dev_err(&sbefifo->fsi_dev->dev,
-				"Fatal bus access failure: %d\n", ret);
+			"Fatal bus access failure: %d\n", ret);
 		list_for_each_entry(xfr, &sbefifo->xfrs, xfrs)
 			kfree(xfr);
 		INIT_LIST_HEAD(&sbefifo->xfrs);
@@ -508,7 +507,7 @@ static void sbefifo_poll_timer(unsigned long data)
 static int sbefifo_open(struct inode *inode, struct file *file)
 {
 	struct sbefifo *sbefifo = container_of(file->private_data,
-			struct sbefifo, mdev);
+					       struct sbefifo, mdev);
 	struct sbefifo_client *client;
 	int ret;
 
@@ -562,12 +561,13 @@ static ssize_t sbefifo_read_common(struct sbefifo_client *client,
 
 	sbefifo_get_client(client);
 	if (wait_event_interruptible(sbefifo->wait,
-				(ret = READ_ONCE(sbefifo->rc)) ||
-				(n = sbefifo_buf_nbreadable(
-						&client->rbuf)))) {
+				     (ret = READ_ONCE(sbefifo->rc)) ||
+				     (n = sbefifo_buf_nbreadable(
+					&client->rbuf)))) {
 		sbefifo_put_client(client);
 		return -ERESTARTSYS;
 	}
+
 	if (ret) {
 		INIT_LIST_HEAD(&client->xfrs);
 		sbefifo_put_client(client);
@@ -581,8 +581,9 @@ static ssize_t sbefifo_read_common(struct sbefifo_client *client,
 			sbefifo_put_client(client);
 			return -EFAULT;
 		}
-	} else
+	} else {
 		memcpy(kbuf, READ_ONCE(client->rbuf.rpos), n);
+	}
 
 	if (sbefifo_buf_readnb(&client->rbuf, n)) {
 		xfr = sbefifo_client_next_xfr(client);
@@ -605,8 +606,8 @@ static ssize_t sbefifo_read_common(struct sbefifo_client *client,
 	return n;
 }
 
-static ssize_t sbefifo_read(struct file *file, char __user *buf,
-		size_t len, loff_t *offset)
+static ssize_t sbefifo_read(struct file *file, char __user *buf, size_t len,
+			    loff_t *offset)
 {
 	struct sbefifo_client *client = file->private_data;
 
@@ -648,6 +649,7 @@ static ssize_t sbefifo_write_common(struct sbefifo_client *client,
 		spin_unlock_irq(&sbefifo->lock);
 		return -ENOMEM;
 	}
+
 	spin_unlock_irq(&sbefifo->lock);
 
 	sbefifo_get_client(client);
@@ -659,8 +661,8 @@ static ssize_t sbefifo_write_common(struct sbefifo_client *client,
 	while (len) {
 		if (wait_event_interruptible(sbefifo->wait,
 				READ_ONCE(sbefifo->rc) ||
-					(sbefifo_client_next_xfr(client) == xfr &&
-					 (n = sbefifo_buf_nbwriteable(
+				(sbefifo_client_next_xfr(client) == xfr &&
+				 (n = sbefifo_buf_nbwriteable(
 							&client->wbuf))))) {
 			set_bit(SBEFIFO_XFR_CANCEL, &xfr->flags);
 			sbefifo_get(sbefifo);
@@ -670,6 +672,7 @@ static ssize_t sbefifo_write_common(struct sbefifo_client *client,
 			sbefifo_put_client(client);
 			return -ERESTARTSYS;
 		}
+
 		if (sbefifo->rc) {
 			INIT_LIST_HEAD(&client->xfrs);
 			sbefifo_put_client(client);
@@ -719,7 +722,7 @@ static ssize_t sbefifo_write_common(struct sbefifo_client *client,
 }
 
 static ssize_t sbefifo_write(struct file *file, const char __user *buf,
-		size_t len, loff_t *offset)
+			     size_t len, loff_t *offset)
 {
 	struct sbefifo_client *client = file->private_data;
 
@@ -820,22 +823,21 @@ static int sbefifo_probe(struct device *dev)
 
 	sbefifo->fsi_dev = fsi_dev;
 
-	ret = sbefifo_inw(sbefifo,
-			SBEFIFO_UP | SBEFIFO_STS, &sts);
+	ret = sbefifo_inw(sbefifo, SBEFIFO_UP | SBEFIFO_STS, &sts);
 	if (ret)
 		return ret;
+
 	if (!(sts & SBEFIFO_EMPTY)) {
-		dev_err(&sbefifo->fsi_dev->dev,
-				"Found data in upstream fifo\n");
+		dev_err(dev, "Found data in upstream fifo\n");
 		return -EIO;
 	}
 
 	ret = sbefifo_inw(sbefifo, SBEFIFO_DWN | SBEFIFO_STS, &sts);
 	if (ret)
 		return ret;
+
 	if (!(sts & SBEFIFO_EMPTY)) {
-		dev_err(&sbefifo->fsi_dev->dev,
-				"Found data in downstream fifo\n");
+		dev_err(dev, "Found data in downstream fifo\n");
 		return -EIO;
 	}
 
@@ -848,13 +850,13 @@ static int sbefifo_probe(struct device *dev)
 
 	sbefifo->idx = ida_simple_get(&sbefifo_ida, 1, INT_MAX, GFP_KERNEL);
 	snprintf(sbefifo->name, sizeof(sbefifo->name), "sbefifo%d",
-			sbefifo->idx);
+		 sbefifo->idx);
 	init_waitqueue_head(&sbefifo->wait);
 	INIT_LIST_HEAD(&sbefifo->xfrs);
 
 	/* This bit of silicon doesn't offer any interrupts... */
 	setup_timer(&sbefifo->poll_timer, sbefifo_poll_timer,
-			(unsigned long)sbefifo);
+		    (unsigned long)sbefifo);
 
 	if (dev->of_node) {
 		/* create platform devs for dts child nodes (occ, etc) */
@@ -863,7 +865,7 @@ static int sbefifo_probe(struct device *dev)
 				 sbefifo->name, child_idx++);
 			child = of_platform_device_create(np, child_name, dev);
 			if (!child)
-				dev_warn(&sbefifo->fsi_dev->dev,
+				dev_warn(dev,
 					 "failed to create child node dev\n");
 		}
 	}
-- 
1.8.3.1

[PATCH linux dev-4.10 v3 05/31] drivers: fsi: sbefifo: replace awkward wait_event expression

[Eddie James]

From: "Edward A. James" <eajames@us.ibm.com>

Use ready() functions to replace awkward boolean expressions in the
wait_event_interruptible calls.

Signed-off-by: Edward A. James <eajames@us.ibm.com>
---
 drivers/fsi/fsi-sbefifo.c | 32 +++++++++++++++++++++++++-------
 1 file changed, 25 insertions(+), 7 deletions(-)

diff --git a/drivers/fsi/fsi-sbefifo.c b/drivers/fsi/fsi-sbefifo.c
index 561a7f6..dbac0c3 100644
--- a/drivers/fsi/fsi-sbefifo.c
+++ b/drivers/fsi/fsi-sbefifo.c
@@ -545,6 +545,16 @@ static unsigned int sbefifo_poll(struct file *file, poll_table *wait)
 	return mask;
 }
 
+static bool sbefifo_read_ready(struct sbefifo *sbefifo,
+			       struct sbefifo_client *client, size_t *n,
+			       size_t *ret)
+{
+	*n = sbefifo_buf_nbreadable(&client->rbuf);
+	*ret = READ_ONCE(sbefifo->rc);
+
+	return *ret || *n;
+}
+
 static ssize_t sbefifo_read_common(struct sbefifo_client *client,
 				   char __user *ubuf, char *kbuf, size_t len)
 {
@@ -561,9 +571,8 @@ static ssize_t sbefifo_read_common(struct sbefifo_client *client,
 
 	sbefifo_get_client(client);
 	if (wait_event_interruptible(sbefifo->wait,
-				     (ret = READ_ONCE(sbefifo->rc)) ||
-				     (n = sbefifo_buf_nbreadable(
-					&client->rbuf)))) {
+				     sbefifo_read_ready(sbefifo, client, &n,
+							&ret))) {
 		sbefifo_put_client(client);
 		return -ERESTARTSYS;
 	}
@@ -619,6 +628,16 @@ static ssize_t sbefifo_read(struct file *file, char __user *buf, size_t len,
 	return sbefifo_read_common(client, buf, NULL, len);
 }
 
+static bool sbefifo_write_ready(struct sbefifo *sbefifo,
+				struct sbefifo_xfr *xfr,
+				struct sbefifo_client *client, size_t *n)
+{
+	struct sbefifo_xfr *next = sbefifo_client_next_xfr(client);
+
+	*n = sbefifo_buf_nbwriteable(&client->wbuf);
+	return READ_ONCE(sbefifo->rc) || (next == xfr && *n);
+}
+
 static ssize_t sbefifo_write_common(struct sbefifo_client *client,
 				    const char __user *ubuf, const char *kbuf,
 				    size_t len)
@@ -660,10 +679,9 @@ static ssize_t sbefifo_write_common(struct sbefifo_client *client,
 	 */
 	while (len) {
 		if (wait_event_interruptible(sbefifo->wait,
-				READ_ONCE(sbefifo->rc) ||
-				(sbefifo_client_next_xfr(client) == xfr &&
-				 (n = sbefifo_buf_nbwriteable(
-							&client->wbuf))))) {
+					     sbefifo_write_ready(sbefifo, xfr,
+								 client,
+								 &n))) {
 			set_bit(SBEFIFO_XFR_CANCEL, &xfr->flags);
 			sbefifo_get(sbefifo);
 			if (mod_timer(&sbefifo->poll_timer, jiffies))
-- 
1.8.3.1

[PATCH linux dev-4.10 v3 06/31] drivers: fsi: sbefifo: remove redundant function

[Eddie James]

From: "Edward A. James" <eajames@us.ibm.com>

sbefifo_client_next_xfr function just does what list_first_entry_or_null
does.

Signed-off-by: Edward A. James <eajames@us.ibm.com>
---
 drivers/fsi/fsi-sbefifo.c | 28 +++++++++++++++-------------
 1 file changed, 15 insertions(+), 13 deletions(-)

diff --git a/drivers/fsi/fsi-sbefifo.c b/drivers/fsi/fsi-sbefifo.c
index dbac0c3..173cd03 100644
--- a/drivers/fsi/fsi-sbefifo.c
+++ b/drivers/fsi/fsi-sbefifo.c
@@ -279,20 +279,12 @@ static struct sbefifo_xfr *sbefifo_enq_xfr(struct sbefifo_client *client)
 	return xfr;
 }
 
-static struct sbefifo_xfr *sbefifo_client_next_xfr(
-		struct sbefifo_client *client)
-{
-	if (list_empty(&client->xfrs))
-		return NULL;
-
-	return container_of(client->xfrs.next, struct sbefifo_xfr, client);
-}
-
 static bool sbefifo_xfr_rsp_pending(struct sbefifo_client *client)
 {
-	struct sbefifo_xfr *xfr;
+	struct sbefifo_xfr *xfr = list_first_entry_or_null(&client->xfrs,
+							   struct sbefifo_xfr,
+							   client);
 
-	xfr = sbefifo_client_next_xfr(client);
 	if (xfr && test_bit(SBEFIFO_XFR_RESP_PENDING, &xfr->flags))
 		return true;
 
@@ -595,7 +587,15 @@ static ssize_t sbefifo_read_common(struct sbefifo_client *client,
 	}
 
 	if (sbefifo_buf_readnb(&client->rbuf, n)) {
-		xfr = sbefifo_client_next_xfr(client);
+		xfr = list_first_entry_or_null(&client->xfrs,
+					       struct sbefifo_xfr, client);
+		if (!xfr) {
+			/* should be impossible to not have an xfr here */
+			WARN_ONCE(1, "no xfr in queue");
+			sbefifo_put_client(client);
+			return -EPROTO;
+		}
+
 		if (!test_bit(SBEFIFO_XFR_COMPLETE, &xfr->flags)) {
 			/*
 			 * Fill the read buffer back up.
@@ -632,7 +632,9 @@ static bool sbefifo_write_ready(struct sbefifo *sbefifo,
 				struct sbefifo_xfr *xfr,
 				struct sbefifo_client *client, size_t *n)
 {
-	struct sbefifo_xfr *next = sbefifo_client_next_xfr(client);
+	struct sbefifo_xfr *next = list_first_entry_or_null(&client->xfrs,
+							    struct sbefifo_xfr,
+							    client);
 
 	*n = sbefifo_buf_nbwriteable(&client->wbuf);
 	return READ_ONCE(sbefifo->rc) || (next == xfr && *n);
-- 
1.8.3.1

[PATCH linux dev-4.10 v3 07/31] drivers: fsi: sbefifo: Use goto to reduce put statements

[Eddie James]

From: "Edward A. James" <eajames@us.ibm.com>

Use a goto in the read() and write() functions so that we can just have
one get_client and put_client statement.

Signed-off-by: Edward A. James <eajames@us.ibm.com>
---
 drivers/fsi/fsi-sbefifo.c | 35 ++++++++++++++++++-----------------
 1 file changed, 18 insertions(+), 17 deletions(-)

diff --git a/drivers/fsi/fsi-sbefifo.c b/drivers/fsi/fsi-sbefifo.c
index 173cd03..a5bdc75 100644
--- a/drivers/fsi/fsi-sbefifo.c
+++ b/drivers/fsi/fsi-sbefifo.c
@@ -565,22 +565,21 @@ static ssize_t sbefifo_read_common(struct sbefifo_client *client,
 	if (wait_event_interruptible(sbefifo->wait,
 				     sbefifo_read_ready(sbefifo, client, &n,
 							&ret))) {
-		sbefifo_put_client(client);
-		return -ERESTARTSYS;
+		ret = -ERESTARTSYS;
+		goto out;
 	}
 
 	if (ret) {
 		INIT_LIST_HEAD(&client->xfrs);
-		sbefifo_put_client(client);
-		return ret;
+		goto out;
 	}
 
 	n = min_t(size_t, n, len);
 
 	if (ubuf) {
 		if (copy_to_user(ubuf, READ_ONCE(client->rbuf.rpos), n)) {
-			sbefifo_put_client(client);
-			return -EFAULT;
+			ret = -EFAULT;
+			goto out;
 		}
 	} else {
 		memcpy(kbuf, READ_ONCE(client->rbuf.rpos), n);
@@ -592,8 +591,8 @@ static ssize_t sbefifo_read_common(struct sbefifo_client *client,
 		if (!xfr) {
 			/* should be impossible to not have an xfr here */
 			WARN_ONCE(1, "no xfr in queue");
-			sbefifo_put_client(client);
-			return -EPROTO;
+			ret = -EPROTO;
+			goto out;
 		}
 
 		if (!test_bit(SBEFIFO_XFR_COMPLETE, &xfr->flags)) {
@@ -610,9 +609,11 @@ static ssize_t sbefifo_read_common(struct sbefifo_client *client,
 		}
 	}
 
-	sbefifo_put_client(client);
+	ret = n;
 
-	return n;
+out:
+	sbefifo_put_client(client);
+	return ret;
 }
 
 static ssize_t sbefifo_read(struct file *file, char __user *buf, size_t len,
@@ -689,14 +690,14 @@ static ssize_t sbefifo_write_common(struct sbefifo_client *client,
 			if (mod_timer(&sbefifo->poll_timer, jiffies))
 				sbefifo_put(sbefifo);
 
-			sbefifo_put_client(client);
-			return -ERESTARTSYS;
+			ret = -ERESTARTSYS;
+			goto out;
 		}
 
 		if (sbefifo->rc) {
 			INIT_LIST_HEAD(&client->xfrs);
-			sbefifo_put_client(client);
-			return sbefifo->rc;
+			ret = sbefifo->rc;
+			goto out;
 		}
 
 		n = min_t(size_t, n, len);
@@ -708,8 +709,8 @@ static ssize_t sbefifo_write_common(struct sbefifo_client *client,
 				sbefifo_get(sbefifo);
 				if (mod_timer(&sbefifo->poll_timer, jiffies))
 					sbefifo_put(sbefifo);
-				sbefifo_put_client(client);
-				return -EFAULT;
+				ret = -EFAULT;
+				goto out;
 			}
 
 			ubuf += n;
@@ -736,8 +737,8 @@ static ssize_t sbefifo_write_common(struct sbefifo_client *client,
 			sbefifo_put(sbefifo);
 	}
 
+out:
 	sbefifo_put_client(client);
-
 	return ret;
 }
 
-- 
1.8.3.1

[PATCH linux dev-4.10 v3 08/31] drivers: fsi: sbefifo: Do an earlier get_client call

[Eddie James]

From: "Edward A. James" <eajames@us.ibm.com>

Get client before we start using it.

Signed-off-by: Edward A. James <eajames@us.ibm.com>
---
 drivers/fsi/fsi-sbefifo.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/fsi/fsi-sbefifo.c b/drivers/fsi/fsi-sbefifo.c
index a5bdc75..5efba30 100644
--- a/drivers/fsi/fsi-sbefifo.c
+++ b/drivers/fsi/fsi-sbefifo.c
@@ -656,6 +656,7 @@ static ssize_t sbefifo_write_common(struct sbefifo_client *client,
 	if (!len)
 		return 0;
 
+	sbefifo_get_client(client);
 	n = sbefifo_buf_nbwriteable(&client->wbuf);
 
 	spin_lock_irq(&sbefifo->lock);
@@ -663,19 +664,19 @@ static ssize_t sbefifo_write_common(struct sbefifo_client *client,
 
 	if ((client->f_flags & O_NONBLOCK) && xfr && n < len) {
 		spin_unlock_irq(&sbefifo->lock);
-		return -EAGAIN;
+		ret = -EAGAIN;
+		goto out;
 	}
 
 	xfr = sbefifo_enq_xfr(client);
 	if (!xfr) {
 		spin_unlock_irq(&sbefifo->lock);
-		return -ENOMEM;
+		ret = -ENOMEM;
+		goto out;
 	}
 
 	spin_unlock_irq(&sbefifo->lock);
 
-	sbefifo_get_client(client);
-
 	/*
 	 * Partial writes are not really allowed in that EOT is sent exactly
 	 * once per write.
-- 
1.8.3.1

[PATCH linux dev-4.10 v3 09/31] drivers: fsi: sbefifo: Remove warning and user data access check

[Eddie James]

From: "Edward A. James" <eajames@us.ibm.com>

We check for user buffer access when we copy the data. No reason to
throw a kernel warning if there is an offset provided; just ignore it.

Signed-off-by: Edward A. James <eajames@us.ibm.com>
---
 drivers/fsi/fsi-sbefifo.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/drivers/fsi/fsi-sbefifo.c b/drivers/fsi/fsi-sbefifo.c
index 5efba30..60b133d 100644
--- a/drivers/fsi/fsi-sbefifo.c
+++ b/drivers/fsi/fsi-sbefifo.c
@@ -621,11 +621,6 @@ static ssize_t sbefifo_read(struct file *file, char __user *buf, size_t len,
 {
 	struct sbefifo_client *client = file->private_data;
 
-	WARN_ON(*offset);
-
-	if (!access_ok(VERIFY_WRITE, buf, len))
-		return -EFAULT;
-
 	return sbefifo_read_common(client, buf, NULL, len);
 }
 
@@ -748,11 +743,6 @@ static ssize_t sbefifo_write(struct file *file, const char __user *buf,
 {
 	struct sbefifo_client *client = file->private_data;
 
-	WARN_ON(*offset);
-
-	if (!access_ok(VERIFY_READ, buf, len))
-		return -EFAULT;
-
 	return sbefifo_write_common(client, buf, NULL, len);
 }
 
-- 
1.8.3.1

[PATCH linux dev-4.10 v3 10/31] drivers: fsi: sbefifo: destroy the ida list on exit

[Eddie James]

From: "Edward A. James" <eajames@us.ibm.com>

Need to clean this up.

Signed-off-by: Edward A. James <eajames@us.ibm.com>
---
 drivers/fsi/fsi-sbefifo.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/fsi/fsi-sbefifo.c b/drivers/fsi/fsi-sbefifo.c
index 60b133d..0ff5209 100644
--- a/drivers/fsi/fsi-sbefifo.c
+++ b/drivers/fsi/fsi-sbefifo.c
@@ -947,6 +947,8 @@ static int sbefifo_init(void)
 static void sbefifo_exit(void)
 {
 	fsi_driver_unregister(&sbefifo_drv);
+
+	ida_destroy(&sbefifo_ida);
 }
 
 module_init(sbefifo_init);
-- 
1.8.3.1

[PATCH linux dev-4.10 v3 11/31] drivers: fsi: sbefifo: Fix module authors and comments

[Eddie James]

From: "Edward A. James" <eajames@us.ibm.com>

Clarify the xfr situation in write() and fix module authors

Signed-off-by: Edward A. James <eajames@us.ibm.com>
---
 drivers/fsi/fsi-sbefifo.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/fsi/fsi-sbefifo.c b/drivers/fsi/fsi-sbefifo.c
index 0ff5209..a2acf76 100644
--- a/drivers/fsi/fsi-sbefifo.c
+++ b/drivers/fsi/fsi-sbefifo.c
@@ -655,7 +655,7 @@ static ssize_t sbefifo_write_common(struct sbefifo_client *client,
 	n = sbefifo_buf_nbwriteable(&client->wbuf);
 
 	spin_lock_irq(&sbefifo->lock);
-	xfr = sbefifo_next_xfr(sbefifo);
+	xfr = sbefifo_next_xfr(sbefifo);	/* next xfr to be executed */
 
 	if ((client->f_flags & O_NONBLOCK) && xfr && n < len) {
 		spin_unlock_irq(&sbefifo->lock);
@@ -663,7 +663,7 @@ static ssize_t sbefifo_write_common(struct sbefifo_client *client,
 		goto out;
 	}
 
-	xfr = sbefifo_enq_xfr(client);
+	xfr = sbefifo_enq_xfr(client);		/* this xfr queued up */
 	if (!xfr) {
 		spin_unlock_irq(&sbefifo->lock);
 		ret = -ENOMEM;
@@ -719,8 +719,9 @@ static ssize_t sbefifo_write_common(struct sbefifo_client *client,
 		len -= n;
 		ret += n;
 
-		/* set flag before starting the worker, as it may run through
-		 * and check the flag before we exit this loop!
+		/*
+		 * Set this before starting timer to avoid race condition on
+		 * this flag with the timer function writer.
 		 */
 		if (!len)
 			set_bit(SBEFIFO_XFR_WRITE_DONE, &xfr->flags);
@@ -955,4 +956,5 @@ static void sbefifo_exit(void)
 module_exit(sbefifo_exit);
 MODULE_LICENSE("GPL");
 MODULE_AUTHOR("Brad Bishop <bradleyb@fuzziesquirrel.com>");
-MODULE_DESCRIPTION("Linux device interface to the POWER self boot engine");
+MODULE_AUTHOR("Eddie James <eajames@linux.vnet.ibm.com>");
+MODULE_DESCRIPTION("Linux device interface to the POWER Self Boot Engine");
-- 
1.8.3.1

[PATCH linux dev-4.10 v3 12/31] drivers: fsi: sbefifo: Fix include guards in header file

[Eddie James]

From: "Edward A. James" <eajames@us.ibm.com>

Make them conform to Linux standard headers.

Signed-off-by: Edward A. James <eajames@us.ibm.com>
---
 include/linux/fsi-sbefifo.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/linux/fsi-sbefifo.h b/include/linux/fsi-sbefifo.h
index 1b46c63..8e55891 100644
--- a/include/linux/fsi-sbefifo.h
+++ b/include/linux/fsi-sbefifo.h
@@ -13,8 +13,8 @@
  * GNU General Public License for more details.
  */
 
-#ifndef __FSI_SBEFIFO_H__
-#define __FSI_SBEFIFO_H__
+#ifndef LINUX_FSI_SBEFIFO_H
+#define LINUX_FSI_SBEFIFO_H
 
 struct device;
 struct sbefifo_client;
@@ -27,4 +27,4 @@ extern int sbefifo_drv_write(struct sbefifo_client *client, const char *buf,
 			     size_t len);
 extern void sbefifo_drv_release(struct sbefifo_client *client);
 
-#endif /* __FSI_SBEFIFO_H__ */
+#endif /* LINUX_FSI_SBEFIFO_H */
-- 
1.8.3.1

[PATCH linux dev-4.10 v3 13/31] drivers: fsi: SBEFIFO: Fix probe() and remove()

[Eddie James]

From: "Edward A. James" <eajames@us.ibm.com>

Probe didn't handle a failed misc device registration properly. Remove
had the potential for hangs. Also, remove the list of SBEFIFOs and
instead just use the device "driver data" pointer.

Signed-off-by: Edward A. James <eajames@us.ibm.com>
---
 drivers/fsi/fsi-sbefifo.c | 111 ++++++++++++++++++++++------------------------
 1 file changed, 53 insertions(+), 58 deletions(-)

diff --git a/drivers/fsi/fsi-sbefifo.c b/drivers/fsi/fsi-sbefifo.c
index a2acf76..9b560ec 100644
--- a/drivers/fsi/fsi-sbefifo.c
+++ b/drivers/fsi/fsi-sbefifo.c
@@ -58,7 +58,6 @@ struct sbefifo {
 	struct fsi_device *fsi_dev;
 	struct miscdevice mdev;
 	wait_queue_head_t wait;
-	struct list_head link;
 	struct list_head xfrs;
 	struct kref kref;
 	spinlock_t lock;
@@ -96,8 +95,6 @@ struct sbefifo_client {
 	unsigned long f_flags;
 };
 
-static struct list_head sbefifo_fifos;
-
 static DEFINE_IDA(sbefifo_ida);
 
 static int sbefifo_inw(struct sbefifo *sbefifo, int reg, u32 *word)
@@ -267,9 +264,12 @@ static struct sbefifo_xfr *sbefifo_enq_xfr(struct sbefifo_client *client)
 	struct sbefifo *sbefifo = client->dev;
 	struct sbefifo_xfr *xfr;
 
+	if (READ_ONCE(sbefifo->rc))
+		return ERR_PTR(sbefifo->rc);
+
 	xfr = kzalloc(sizeof(*xfr), GFP_KERNEL);
 	if (!xfr)
-		return NULL;
+		return ERR_PTR(-ENOMEM);
 
 	xfr->rbuf = &client->rbuf;
 	xfr->wbuf = &client->wbuf;
@@ -490,7 +490,7 @@ static void sbefifo_poll_timer(unsigned long data)
 	}
 
 	sbefifo_put(sbefifo);
-	wake_up(&sbefifo->wait);
+	wake_up_interruptible(&sbefifo->wait);
 
 out_unlock:
 	spin_unlock(&sbefifo->lock);
@@ -605,7 +605,7 @@ static ssize_t sbefifo_read_common(struct sbefifo_client *client,
 		} else {
 			kfree(xfr);
 			list_del(&xfr->client);
-			wake_up(&sbefifo->wait);
+			wake_up_interruptible(&sbefifo->wait);
 		}
 	}
 
@@ -664,9 +664,9 @@ static ssize_t sbefifo_write_common(struct sbefifo_client *client,
 	}
 
 	xfr = sbefifo_enq_xfr(client);		/* this xfr queued up */
-	if (!xfr) {
+	if (IS_ERR(xfr)) {
 		spin_unlock_irq(&sbefifo->lock);
-		ret = -ENOMEM;
+		ret = PTR_ERR(xfr);
 		goto out;
 	}
 
@@ -769,18 +769,15 @@ static int sbefifo_release(struct inode *inode, struct file *file)
 struct sbefifo_client *sbefifo_drv_open(struct device *dev,
 					unsigned long flags)
 {
-	struct sbefifo_client *client = NULL;
-	struct sbefifo *sbefifo;
-	struct fsi_device *fsi_dev = to_fsi_dev(dev);
+	struct sbefifo_client *client;
+	struct sbefifo *sbefifo = dev_get_drvdata(dev);
 
-	list_for_each_entry(sbefifo, &sbefifo_fifos, link) {
-		if (sbefifo->fsi_dev != fsi_dev)
-			continue;
+	if (!sbefifo)
+		return NULL;
 
-		client = sbefifo_new_client(sbefifo);
-		if (client)
-			client->f_flags = flags;
-	}
+	client = sbefifo_new_client(sbefifo);
+	if (client)
+		client->f_flags = flags;
 
 	return client;
 }
@@ -854,69 +851,68 @@ static int sbefifo_probe(struct device *dev)
 		return -EIO;
 	}
 
-	sbefifo->mdev.minor = MISC_DYNAMIC_MINOR;
-	sbefifo->mdev.fops = &sbefifo_fops;
-	sbefifo->mdev.name = sbefifo->name;
-	sbefifo->mdev.parent = dev;
 	spin_lock_init(&sbefifo->lock);
 	kref_init(&sbefifo->kref);
+	init_waitqueue_head(&sbefifo->wait);
+	INIT_LIST_HEAD(&sbefifo->xfrs);
 
 	sbefifo->idx = ida_simple_get(&sbefifo_ida, 1, INT_MAX, GFP_KERNEL);
 	snprintf(sbefifo->name, sizeof(sbefifo->name), "sbefifo%d",
 		 sbefifo->idx);
-	init_waitqueue_head(&sbefifo->wait);
-	INIT_LIST_HEAD(&sbefifo->xfrs);
 
 	/* This bit of silicon doesn't offer any interrupts... */
 	setup_timer(&sbefifo->poll_timer, sbefifo_poll_timer,
 		    (unsigned long)sbefifo);
 
-	if (dev->of_node) {
-		/* create platform devs for dts child nodes (occ, etc) */
-		for_each_child_of_node(dev->of_node, np) {
-			snprintf(child_name, sizeof(child_name), "%s-dev%d",
-				 sbefifo->name, child_idx++);
-			child = of_platform_device_create(np, child_name, dev);
-			if (!child)
-				dev_warn(dev,
-					 "failed to create child node dev\n");
-		}
+	sbefifo->mdev.minor = MISC_DYNAMIC_MINOR;
+	sbefifo->mdev.fops = &sbefifo_fops;
+	sbefifo->mdev.name = sbefifo->name;
+	sbefifo->mdev.parent = dev;
+	ret = misc_register(&sbefifo->mdev);
+	if (ret) {
+		dev_err(dev, "failed to register miscdevice: %d\n", ret);
+		ida_simple_remove(&sbefifo_ida, sbefifo->idx);
+		sbefifo_put(sbefifo);
+		return ret;
 	}
 
-	list_add(&sbefifo->link, &sbefifo_fifos);
-	
-	return misc_register(&sbefifo->mdev);
+	/* create platform devs for dts child nodes (occ, etc) */
+	for_each_available_child_of_node(dev->of_node, np) {
+		snprintf(child_name, sizeof(child_name), "%s-dev%d",
+			 sbefifo->name, child_idx++);
+		child = of_platform_device_create(np, child_name, dev);
+		if (!child)
+			dev_warn(dev, "failed to create child %s dev\n",
+				 child_name);
+	}
+
+	dev_set_drvdata(dev, sbefifo);
+
+	return 0;
 }
 
 static int sbefifo_remove(struct device *dev)
 {
-	struct fsi_device *fsi_dev = to_fsi_dev(dev);
-	struct sbefifo *sbefifo, *sbefifo_tmp;
+	struct sbefifo *sbefifo = dev_get_drvdata(dev);
 	struct sbefifo_xfr *xfr;
 
-	list_for_each_entry_safe(sbefifo, sbefifo_tmp, &sbefifo_fifos, link) {
-		if (sbefifo->fsi_dev != fsi_dev)
-			continue;
+	WRITE_ONCE(sbefifo->rc, -ENODEV);
+	wake_up_all(&sbefifo->wait);
 
-		device_for_each_child(dev, NULL, sbefifo_unregister_child);
+	misc_deregister(&sbefifo->mdev);
+	device_for_each_child(dev, NULL, sbefifo_unregister_child);
 
-		misc_deregister(&sbefifo->mdev);
-		list_del(&sbefifo->link);
-		ida_simple_remove(&sbefifo_ida, sbefifo->idx);
-
-		if (del_timer_sync(&sbefifo->poll_timer))
-			sbefifo_put(sbefifo);
+	ida_simple_remove(&sbefifo_ida, sbefifo->idx);
 
-		spin_lock(&sbefifo->lock);
-		list_for_each_entry(xfr, &sbefifo->xfrs, xfrs)
-			kfree(xfr);
-		spin_unlock(&sbefifo->lock);
+	if (del_timer_sync(&sbefifo->poll_timer))
+		sbefifo_put(sbefifo);
 
-		WRITE_ONCE(sbefifo->rc, -ENODEV);
+	spin_lock(&sbefifo->lock);
+	list_for_each_entry(xfr, &sbefifo->xfrs, xfrs)
+		kfree(xfr);
+	spin_unlock(&sbefifo->lock);
 
-		wake_up(&sbefifo->wait);
-		sbefifo_put(sbefifo);
-	}
+	sbefifo_put(sbefifo);
 
 	return 0;
 }
@@ -941,7 +937,6 @@ static int sbefifo_remove(struct device *dev)
 
 static int sbefifo_init(void)
 {
-	INIT_LIST_HEAD(&sbefifo_fifos);
 	return fsi_driver_register(&sbefifo_drv);
 }
 
-- 
1.8.3.1

[PATCH linux dev-4.10 v3 14/31] drivers: fsi: SBEFIFO: check for xfr complete in read wait_event

[Eddie James]

From: "Edward A. James" <eajames@us.ibm.com>

We should check to see if the XFR is complete, not just for a failure
or for available data. If we hit EOT without getting more data, we may
wait forever.

Signed-off-by: Edward A. James <eajames@us.ibm.com>
---
 drivers/fsi/fsi-sbefifo.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/fsi/fsi-sbefifo.c b/drivers/fsi/fsi-sbefifo.c
index 9b560ec..5d73437 100644
--- a/drivers/fsi/fsi-sbefifo.c
+++ b/drivers/fsi/fsi-sbefifo.c
@@ -541,10 +541,15 @@ static bool sbefifo_read_ready(struct sbefifo *sbefifo,
 			       struct sbefifo_client *client, size_t *n,
 			       size_t *ret)
 {
+	struct sbefifo_xfr *xfr = list_first_entry_or_null(&client->xfrs,
+							   struct sbefifo_xfr,
+							   client);
+
 	*n = sbefifo_buf_nbreadable(&client->rbuf);
 	*ret = READ_ONCE(sbefifo->rc);
 
-	return *ret || *n;
+	return *ret || *n ||
+		(xfr && test_bit(SBEFIFO_XFR_COMPLETE, &xfr->flags));
 }
 
 static ssize_t sbefifo_read_common(struct sbefifo_client *client,
-- 
1.8.3.1

[PATCH linux dev-4.10 v3 15/31] drivers: fsi: occ: Fix includes

[Eddie James]

From: "Edward A. James" <eajames@us.ibm.com>

Need to include everything we use and not rely on recursive includes.

Signed-off-by: Edward A. James <eajames@us.ibm.com>
---
 drivers/fsi/occ.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/fsi/occ.c b/drivers/fsi/occ.c
index 621fbf0..2d7df8a 100644
--- a/drivers/fsi/occ.c
+++ b/drivers/fsi/occ.c
@@ -10,16 +10,21 @@
 #include <asm/unaligned.h>
 #include <linux/device.h>
 #include <linux/err.h>
+#include <linux/errno.h>
+#include <linux/fs.h>
 #include <linux/fsi-sbefifo.h>
-#include <linux/init.h>
+#include <linux/idr.h>
 #include <linux/kernel.h>
+#include <linux/list.h>
 #include <linux/miscdevice.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
+#include <linux/occ.h>
 #include <linux/of.h>
 #include <linux/of_platform.h>
 #include <linux/platform_device.h>
 #include <linux/slab.h>
+#include <linux/spinlock.h>
 #include <linux/uaccess.h>
 #include <linux/wait.h>
 #include <linux/workqueue.h>
-- 
1.8.3.1

[PATCH linux dev-4.10 v3 16/31] drivers: fsi: occ: Fix errant kfree calls

[Eddie James]

From: "Edward A. James" <eajames@us.ibm.com>

Left over from when xfr was an independant structure. Remove these calls
as xfr is a part of the client struct.

Signed-off-by: Edward A. James <eajames@us.ibm.com>
---
 drivers/fsi/occ.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/fsi/occ.c b/drivers/fsi/occ.c
index 2d7df8a..cd60c5a 100644
--- a/drivers/fsi/occ.c
+++ b/drivers/fsi/occ.c
@@ -289,7 +289,6 @@ static ssize_t occ_write_common(struct occ_client *client,
 
 	if (ubuf) {
 		if (copy_from_user(&xfr->buf[1], ubuf, len)) {
-			kfree(xfr);
 			rc = -EFAULT;
 			goto done;
 		}
@@ -298,7 +297,6 @@ static ssize_t occ_write_common(struct occ_client *client,
 
 	data_length = (xfr->buf[2] << 8) + xfr->buf[3];
 	if (data_length > OCC_CMD_DATA_BYTES) {
-		kfree(xfr);
 		rc = -EINVAL;
 		goto done;
 	}
-- 
1.8.3.1

[PATCH linux dev-4.10 v3 17/31] drivers: fsi: occ: remove unused occ_command structure

[Eddie James]

From: "Edward A. James" <eajames@us.ibm.com>

Signed-off-by: Edward A. James <eajames@us.ibm.com>
---
 drivers/fsi/occ.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/drivers/fsi/occ.c b/drivers/fsi/occ.c
index cd60c5a..82a60c5 100644
--- a/drivers/fsi/occ.c
+++ b/drivers/fsi/occ.c
@@ -46,14 +46,6 @@ struct occ {
 
 #define to_occ(x)	container_of((x), struct occ, mdev)
 
-struct occ_command {
-	u8 seq_no;
-	u8 cmd_type;
-	u16 data_length;
-	u8 data[OCC_CMD_DATA_BYTES];
-	u16 checksum;
-} __packed;
-
 struct occ_response {
 	u8 seq_no;
 	u8 cmd_type;
-- 
1.8.3.1

[PATCH linux dev-4.10 v3 18/31] drivers: fsi: occ: Use big-endian values

[Eddie James]

From: "Edward A. James" <eajames@us.ibm.com>

Switch to __be16 from u16 and __be32 from u32. Also, use kernel access
of unaligned be16 instead of manually creating a big-endian value from
the occ response buffer.

Signed-off-by: Edward A. James <eajames@us.ibm.com>
---
 drivers/fsi/occ.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/fsi/occ.c b/drivers/fsi/occ.c
index 82a60c5..55f293d 100644
--- a/drivers/fsi/occ.c
+++ b/drivers/fsi/occ.c
@@ -50,9 +50,9 @@ struct occ_response {
 	u8 seq_no;
 	u8 cmd_type;
 	u8 return_status;
-	u16 data_length;
+	__be16 data_length;
 	u8 data[OCC_RESP_DATA_BYTES];
-	u16 checksum;
+	__be16 checksum;
 } __packed;
 
 /*
@@ -425,7 +425,7 @@ static int occ_getsram(struct device *sbefifo, u32 address, u8 *data,
 {
 	int rc;
 	u8 *resp;
-	u32 buf[5];
+	__be32 buf[5];
 	u32 data_len = ((len + 7) / 8) * 8;
 	struct sbefifo_client *client;
 
@@ -476,7 +476,7 @@ static int occ_putsram(struct device *sbefifo, u32 address, u8 *data,
 		       ssize_t len)
 {
 	int rc;
-	u32 *buf;
+	__be32 *buf;
 	u32 data_len = ((len + 7) / 8) * 8;
 	size_t cmd_len = data_len + 20;
 	struct sbefifo_client *client;
@@ -522,7 +522,7 @@ static int occ_putsram(struct device *sbefifo, u32 address, u8 *data,
 static int occ_trigger_attn(struct device *sbefifo)
 {
 	int rc;
-	u32 buf[6];
+	__be32 buf[6];
 	struct sbefifo_client *client;
 
 	buf[0] = cpu_to_be32(0x6);
@@ -560,6 +560,7 @@ static void occ_worker(struct work_struct *work)
 	int rc = 0, empty, waiting, canceled;
 	u16 resp_data_length;
 	struct occ_xfr *xfr;
+	struct occ_response *resp;
 	struct occ_client *client;
 	struct occ *occ = container_of(work, struct occ, work);
 	struct device *sbefifo = occ->sbefifo;
@@ -573,6 +574,7 @@ static void occ_worker(struct work_struct *work)
 		return;
 	}
 
+	resp = (struct occ_response *)xfr->buf;
 	set_bit(XFR_IN_PROGRESS, &xfr->flags);
 
 	spin_unlock_irq(&occ->list_lock);
@@ -591,7 +593,7 @@ static void occ_worker(struct work_struct *work)
 	if (rc)
 		goto done;
 
-	resp_data_length = (xfr->buf[3] << 8) + xfr->buf[4];
+	resp_data_length = get_unaligned_be16(&resp->data_length);
 	if (resp_data_length > OCC_RESP_DATA_BYTES) {
 		rc = -EDOM;
 		goto done;
-- 
1.8.3.1

[PATCH linux dev-4.10 v3 19/31] drivers: fsi: occ: Return ENODEV if client is NULL

[Eddie James]

From: "Edward A. James" <eajames@us.ibm.com>

Safeguard the in-kernel api functions.

Signed-off-by: Edward A. James <eajames@us.ibm.com>
---
 drivers/fsi/occ.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/fsi/occ.c b/drivers/fsi/occ.c
index 55f293d..a551349 100644
--- a/drivers/fsi/occ.c
+++ b/drivers/fsi/occ.c
@@ -654,18 +654,27 @@ struct occ_client *occ_drv_open(struct device *dev, unsigned long flags)
 
 int occ_drv_read(struct occ_client *client, char *buf, size_t len)
 {
+	if (!client)
+		return -ENODEV;
+
 	return occ_read_common(client, NULL, buf, len);
 }
 EXPORT_SYMBOL_GPL(occ_drv_read);
 
 int occ_drv_write(struct occ_client *client, const char *buf, size_t len)
 {
+	if (!client)
+		return -ENODEV;
+
 	return occ_write_common(client, NULL, buf, len);
 }
 EXPORT_SYMBOL_GPL(occ_drv_write);
 
 void occ_drv_release(struct occ_client *client)
 {
+	if (!client)
+		return;
+
 	occ_release_common(client);
 }
 EXPORT_SYMBOL_GPL(occ_drv_release);
-- 
1.8.3.1

[PATCH linux dev-4.10 v3 20/31] drivers: fsi: occ: Remove early user buffer checking

[Eddie James]

From: "Edward A. James" <eajames@us.ibm.com>

We check user buffer access when we copy the data in read()/write()
anyway.

Signed-off-by: Edward A. James <eajames@us.ibm.com>
---
 drivers/fsi/occ.c | 12 ------------
 1 file changed, 12 deletions(-)

diff --git a/drivers/fsi/occ.c b/drivers/fsi/occ.c
index a551349..7ced9b6 100644
--- a/drivers/fsi/occ.c
+++ b/drivers/fsi/occ.c
@@ -247,12 +247,6 @@ static ssize_t occ_read(struct file *file, char __user *buf, size_t len,
 {
 	struct occ_client *client = file->private_data;
 
-	/* check this ahead of time so we don't go changing the xfr state
-	 * needlessly
-	 */
-	if (!access_ok(VERIFY_WRITE, buf, len))
-		return -EFAULT;
-
 	return occ_read_common(client, buf, NULL, len);
 }
 
@@ -316,12 +310,6 @@ static ssize_t occ_write(struct file *file, const char __user *buf,
 {
 	struct occ_client *client = file->private_data;
 
-	/* check this ahead of time so we don't go changing the xfr state
-	 * needlessly
-	 */
-	if (!access_ok(VERIFY_READ, buf, len))
-		return -EFAULT;
-
 	return occ_write_common(client, buf, NULL, len);
 }
 
-- 
1.8.3.1

[PATCH linux dev-4.10 v3 21/31] drivers: fsi: occ: Switch to more logical errnos

[Eddie James]

From: "Edward A. James" <eajames@us.ibm.com>

Signed-off-by: Edward A. James <eajames@us.ibm.com>
---
 drivers/fsi/occ.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/fsi/occ.c b/drivers/fsi/occ.c
index 7ced9b6..4e677c6 100644
--- a/drivers/fsi/occ.c
+++ b/drivers/fsi/occ.c
@@ -386,7 +386,7 @@ static int occ_write_sbefifo(struct sbefifo_client *client, const char *buf,
 		total += rc;
 	} while (total < len);
 
-	return (total == len) ? 0 : -EMSGSIZE;
+	return (total == len) ? 0 : -ENOSPC;
 }
 
 static int occ_read_sbefifo(struct sbefifo_client *client, char *buf,
@@ -405,7 +405,7 @@ static int occ_read_sbefifo(struct sbefifo_client *client, char *buf,
 		total += rc;
 	} while (total < len);
 
-	return (total == len) ? 0 : -EMSGSIZE;
+	return (total == len) ? 0 : -ENODATA;
 }
 
 static int occ_getsram(struct device *sbefifo, u32 address, u8 *data,
@@ -450,7 +450,7 @@ static int occ_getsram(struct device *sbefifo, u32 address, u8 *data,
 	    (be32_to_cpu(buf[1]) == 0xC0DEA403))
 		memcpy(data, resp, len);
 	else
-		rc = -EFAULT;
+		rc = -EBADMSG;
 
 free:
 	kfree(resp);
@@ -498,7 +498,7 @@ static int occ_putsram(struct device *sbefifo, u32 address, u8 *data,
 	/* check for good response */
 	if ((be32_to_cpu(buf[0]) != data_len) ||
 	    (be32_to_cpu(buf[1]) != 0xC0DEA404))
-		rc = -EFAULT;
+		rc = -EBADMSG;
 
 done:
 	sbefifo_drv_release(client);
@@ -535,7 +535,7 @@ static int occ_trigger_attn(struct device *sbefifo)
 	/* check for good response */
 	if ((be32_to_cpu(buf[0]) != 0xC0DEA202) ||
 	    (be32_to_cpu(buf[1]) & 0x0FFFFFFF))
-		rc = -EFAULT;
+		rc = -EBADMSG;
 
 done:
 	sbefifo_drv_release(client);
@@ -583,7 +583,7 @@ static void occ_worker(struct work_struct *work)
 
 	resp_data_length = get_unaligned_be16(&resp->data_length);
 	if (resp_data_length > OCC_RESP_DATA_BYTES) {
-		rc = -EDOM;
+		rc = -EMSGSIZE;
 		goto done;
 	}
 
-- 
1.8.3.1

[PATCH linux dev-4.10 v3 22/31] drivers: fsi: occ: fix white space and bracket problems

[Eddie James]

From: "Edward A. James" <eajames@us.ibm.com>

For better flow and to meet Linux standards.

Signed-off-by: Edward A. James <eajames@us.ibm.com>
---
 drivers/fsi/occ.c | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/drivers/fsi/occ.c b/drivers/fsi/occ.c
index 4e677c6..f27345c 100644
--- a/drivers/fsi/occ.c
+++ b/drivers/fsi/occ.c
@@ -132,9 +132,8 @@ static void occ_enqueue_xfr(struct occ_xfr *xfr)
 
 static struct occ_client *occ_open_common(struct occ *occ, unsigned long flags)
 {
-	struct occ_client *client;
+	struct occ_client *client = kzalloc(sizeof(*client), GFP_KERNEL);
 
-	client = kzalloc(sizeof(*client), GFP_KERNEL);
 	if (!client)
 		return NULL;
 
@@ -180,8 +179,9 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
 		if (client->read_offset) {
 			rc = 0;
 			client->read_offset = 0;
-		} else
+		} else {
 			rc = -ENOMSG;
+		}
 
 		goto done;
 	}
@@ -197,8 +197,10 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
 		spin_unlock_irq(&client->lock);
 
 		rc = wait_event_interruptible(client->wait,
-			test_bit(XFR_COMPLETE, &xfr->flags) ||
-			test_bit(XFR_CANCELED, &xfr->flags));
+					      test_bit(XFR_COMPLETE,
+						       &xfr->flags) ||
+					      test_bit(XFR_CANCELED,
+						       &xfr->flags));
 
 		spin_lock_irq(&client->lock);
 
@@ -222,12 +224,14 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
 
 	bytes = min(len, xfr->resp_data_length - client->read_offset);
 	if (ubuf) {
-		if (copy_to_user(ubuf, &xfr->buf[client->read_offset], bytes)) {
+		if (copy_to_user(ubuf, &xfr->buf[client->read_offset],
+				 bytes)) {
 			rc = -EFAULT;
 			goto done;
 		}
-	} else
+	} else {
 		memcpy(kbuf, &xfr->buf[client->read_offset], bytes);
+	}
 
 	client->read_offset += bytes;
 
@@ -263,6 +267,7 @@ static ssize_t occ_write_common(struct occ_client *client,
 		return -EINVAL;
 
 	spin_lock_irq(&client->lock);
+
 	if (test_and_set_bit(CLIENT_XFR_PENDING, &client->flags)) {
 		rc = -EBUSY;
 		goto done;
@@ -270,7 +275,6 @@ static ssize_t occ_write_common(struct occ_client *client,
 
 	/* clear out the transfer */
 	memset(xfr, 0, sizeof(*xfr));
-
 	xfr->buf[0] = 1;
 
 	if (ubuf) {
@@ -278,8 +282,9 @@ static ssize_t occ_write_common(struct occ_client *client,
 			rc = -EFAULT;
 			goto done;
 		}
-	} else
+	} else {
 		memcpy(&xfr->buf[1], kbuf, len);
+	}
 
 	data_length = (xfr->buf[2] << 8) + xfr->buf[3];
 	if (data_length > OCC_CMD_DATA_BYTES) {
@@ -430,7 +435,7 @@ static int occ_getsram(struct device *sbefifo, u32 address, u8 *data,
 	rc = occ_write_sbefifo(client, (const char *)buf, sizeof(buf));
 	if (rc)
 		goto done;
-	
+
 	resp = kzalloc(data_len, GFP_KERNEL);
 	if (!resp) {
 		rc = -ENOMEM;
-- 
1.8.3.1

[PATCH linux dev-4.10 v3 23/31] drivers: fsi: occ: Destroy the ida list on exit

[Eddie James]

From: "Edward A. James" <eajames@us.ibm.com>

Need to clean up the index list structure.

Signed-off-by: Edward A. James <eajames@us.ibm.com>
---
 drivers/fsi/occ.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/fsi/occ.c b/drivers/fsi/occ.c
index f27345c..916c342 100644
--- a/drivers/fsi/occ.c
+++ b/drivers/fsi/occ.c
@@ -788,6 +788,8 @@ static void occ_exit(void)
 	destroy_workqueue(occ_wq);
 
 	platform_driver_unregister(&occ_driver);
+
+	ida_destroy(&occ_ida);
 }
 
 module_init(occ_init);
-- 
1.8.3.1

[PATCH linux dev-4.10 v3 24/31] drivers: fsi: occ: Remove unnecessary platform_set_drvdata call

[Eddie James]

From: "Edward A. James" <eajames@us.ibm.com>

The purpose was to set that pointer null before probing. However, it is
set null by the kernel by default.

Signed-off-by: Edward A. James <eajames@us.ibm.com>
---
 drivers/fsi/occ.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/fsi/occ.c b/drivers/fsi/occ.c
index 916c342..561b029 100644
--- a/drivers/fsi/occ.c
+++ b/drivers/fsi/occ.c
@@ -703,9 +703,6 @@ static int occ_probe(struct platform_device *pdev)
 	mutex_init(&occ->occ_lock);
 	INIT_WORK(&occ->work, occ_worker);
 
-	/* ensure NULL before we probe children, so they don't hang FSI */
-	platform_set_drvdata(pdev, NULL);
-
 	if (dev->of_node) {
 		rc = of_property_read_u32(dev->of_node, "reg", &reg);
 		if (!rc) {
-- 
1.8.3.1

[PATCH linux dev-4.10 v3 25/31] drivers: fsi: occ: Add comments for clarity

[Eddie James]

From: "Edward A. James" <eajames@us.ibm.com>

Fixup some existing comments as well.

Signed-off-by: Edward A. James <eajames@us.ibm.com>
---
 drivers/fsi/occ.c | 55 +++++++++++++++++++++++++++++++++++++------------------
 1 file changed, 37 insertions(+), 18 deletions(-)

diff --git a/drivers/fsi/occ.c b/drivers/fsi/occ.c
index 561b029..29a17ed 100644
--- a/drivers/fsi/occ.c
+++ b/drivers/fsi/occ.c
@@ -39,8 +39,8 @@ struct occ {
 	int idx;
 	struct miscdevice mdev;
 	struct list_head xfrs;
-	spinlock_t list_lock;
-	struct mutex occ_lock;
+	spinlock_t list_lock;		/* lock access to the xfrs list */
+	struct mutex occ_lock;		/* lock access to the hardware */
 	struct work_struct work;
 };
 
@@ -57,17 +57,17 @@ struct occ_response {
 
 /*
  * transfer flags are NOT mutually exclusive
- * 
+ *
  * Initial flags are none; transfer is created and queued from write(). All
- * 	flags are cleared when the transfer is completed by closing the file or
- * 	reading all of the available response data.
+ *  flags are cleared when the transfer is completed by closing the file or
+ *  reading all of the available response data.
  * XFR_IN_PROGRESS is set when a transfer is started from occ_worker_putsram,
- * 	and cleared if the transfer fails or occ_worker_getsram completes.
+ *  and cleared if the transfer fails or occ_worker_getsram completes.
  * XFR_COMPLETE is set when a transfer fails or finishes occ_worker_getsram.
  * XFR_CANCELED is set when the transfer's client is released.
  * XFR_WAITING is set from read() if the transfer isn't complete and
- * 	NONBLOCKING wasn't specified. Cleared in read() when transfer completes
- * 	or fails.
+ *  O_NONBLOCK wasn't specified. Cleared in read() when transfer completes or
+ *  fails.
  */
 enum {
 	XFR_IN_PROGRESS,
@@ -89,9 +89,9 @@ struct occ_xfr {
  * client flags
  *
  * CLIENT_NONBLOCKING is set during open() if the file was opened with the
- * 	O_NONBLOCKING flag.
+ *  O_NONBLOCK flag.
  * CLIENT_XFR_PENDING is set during write() and cleared when all data has been
- * 	read.
+ *  read.
  */
 enum {
 	CLIENT_NONBLOCKING,
@@ -101,7 +101,7 @@ enum {
 struct occ_client {
 	struct occ *occ;
 	struct occ_xfr xfr;
-	spinlock_t lock;
+	spinlock_t lock;		/* lock access to the client state */
 	wait_queue_head_t wait;
 	size_t read_offset;
 	unsigned long flags;
@@ -273,10 +273,15 @@ static ssize_t occ_write_common(struct occ_client *client,
 		goto done;
 	}
 
-	/* clear out the transfer */
-	memset(xfr, 0, sizeof(*xfr));
-	xfr->buf[0] = 1;
+	memset(xfr, 0, sizeof(*xfr));	/* clear out the transfer */
+	xfr->buf[0] = 1;		/* occ sequence number */
 
+	/*
+	 * Assume user data follows the occ command format.
+	 * byte 0: command type
+	 * bytes 1-2: data length (msb first)
+	 * bytes 3-n: data
+	 */
 	if (ubuf) {
 		if (copy_from_user(&xfr->buf[1], ubuf, len)) {
 			rc = -EFAULT;
@@ -354,7 +359,7 @@ static int occ_release_common(struct occ_client *client)
 		return 0;
 	}
 
-	/* operation is in progress; let worker clean up*/
+	/* operation is in progress; let worker clean up */
 	spin_unlock_irq(&occ->list_lock);
 	spin_unlock_irq(&client->lock);
 	return 0;
@@ -419,9 +424,13 @@ static int occ_getsram(struct device *sbefifo, u32 address, u8 *data,
 	int rc;
 	u8 *resp;
 	__be32 buf[5];
-	u32 data_len = ((len + 7) / 8) * 8;
+	u32 data_len = ((len + 7) / 8) * 8;	/* must be multiples of 8 B */
 	struct sbefifo_client *client;
 
+	/*
+	 * Magic sequence to do SBE getsram command. SBE will fetch data from
+	 * specified SRAM address.
+	 */
 	buf[0] = cpu_to_be32(0x5);
 	buf[1] = cpu_to_be32(0xa403);
 	buf[2] = cpu_to_be32(1);
@@ -470,7 +479,7 @@ static int occ_putsram(struct device *sbefifo, u32 address, u8 *data,
 {
 	int rc;
 	__be32 *buf;
-	u32 data_len = ((len + 7) / 8) * 8;
+	u32 data_len = ((len + 7) / 8) * 8;	/* must be multiples of 8 B */
 	size_t cmd_len = data_len + 20;
 	struct sbefifo_client *client;
 
@@ -478,6 +487,10 @@ static int occ_putsram(struct device *sbefifo, u32 address, u8 *data,
 	if (!buf)
 		return -ENOMEM;
 
+	/*
+	 * Magic sequence to do SBE putsram command. SBE will transfer
+	 * data to specified SRAM address.
+	 */
 	buf[0] = cpu_to_be32(0x5 + (data_len / 4));
 	buf[1] = cpu_to_be32(0xa404);
 	buf[2] = cpu_to_be32(1);
@@ -518,11 +531,15 @@ static int occ_trigger_attn(struct device *sbefifo)
 	__be32 buf[6];
 	struct sbefifo_client *client;
 
+	/*
+	 * Magic sequence to do SBE putscom command. SBE will write 8 bytes to
+	 * specified SCOM address.
+	 */
 	buf[0] = cpu_to_be32(0x6);
 	buf[1] = cpu_to_be32(0xa202);
 	buf[2] = 0;
 	buf[3] = cpu_to_be32(0x6D035);
-	buf[4] = cpu_to_be32(0x20010000);
+	buf[4] = cpu_to_be32(0x20010000);	/* trigger occ attention */
 	buf[5] = 0;
 
 	client = sbefifo_drv_open(sbefifo, 0);
@@ -573,6 +590,7 @@ static void occ_worker(struct work_struct *work)
 	spin_unlock_irq(&occ->list_lock);
 	mutex_lock(&occ->occ_lock);
 
+	/* write occ command */
 	rc = occ_putsram(sbefifo, 0xFFFBE000, xfr->buf,
 			 xfr->cmd_data_length);
 	if (rc)
@@ -582,6 +600,7 @@ static void occ_worker(struct work_struct *work)
 	if (rc)
 		goto done;
 
+	/* read occ response */
 	rc = occ_getsram(sbefifo, 0xFFFBF000, xfr->buf, 8);
 	if (rc)
 		goto done;
-- 
1.8.3.1

[PATCH linux dev-4.10 v3 26/31] drivers: fsi: occ: Add OCC response definitions to header

[Eddie James]

From: "Edward A. James" <eajames@us.ibm.com>

Also fix include guards.

Signed-off-by: Edward A. James <eajames@us.ibm.com>
---
 include/linux/occ.h | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/include/linux/occ.h b/include/linux/occ.h
index d78332c..0a4a54a 100644
--- a/include/linux/occ.h
+++ b/include/linux/occ.h
@@ -11,12 +11,26 @@
  * GNU General Public License for more details.
  */
 
-#ifndef __OCC_H__
-#define __OCC_H__
+#ifndef LINUX_FSI_OCC_H
+#define LINUX_FSI_OCC_H
 
 struct device;
 struct occ_client;
 
+#define OCC_RESP_CMD_IN_PRG		0xFF
+#define OCC_RESP_SUCCESS		0
+#define OCC_RESP_CMD_INVAL		0x11
+#define OCC_RESP_CMD_LEN_INVAL		0x12
+#define OCC_RESP_DATA_INVAL		0x13
+#define OCC_RESP_CHKSUM_ERR		0x14
+#define OCC_RESP_INT_ERR		0x15
+#define OCC_RESP_BAD_STATE		0x16
+#define OCC_RESP_CRIT_EXCEPT		0xE0
+#define OCC_RESP_CRIT_INIT		0xE1
+#define OCC_RESP_CRIT_WATCHDOG		0xE2
+#define OCC_RESP_CRIT_OCB		0xE3
+#define OCC_RESP_CRIT_HW		0xE4
+
 extern struct occ_client *occ_drv_open(struct device *dev,
 				       unsigned long flags);
 extern int occ_drv_read(struct occ_client *client, char *buf, size_t len);
@@ -24,4 +38,4 @@ extern int occ_drv_write(struct occ_client *client, const char *buf,
 			 size_t len);
 extern void occ_drv_release(struct occ_client *client);
 
-#endif /* __OCC_H__ */
+#endif /* LINUX_FSI_OCC_H */
-- 
1.8.3.1

[PATCH linux dev-4.10 v3 27/31] drivers: fsi: occ: Poll while receiving "command in progress"

[Eddie James]

From: "Edward A. James" <eajames@us.ibm.com>

Waiting for OCC to return something other than "command in progress"
should only be done in the worker function, instead of clients repeating
the entire transfer when they receive "command in progress." In this way
clients don't have to check for that return status.

Signed-off-by: Edward A. James <eajames@us.ibm.com>
---
 drivers/fsi/occ.c | 27 ++++++++++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)

diff --git a/drivers/fsi/occ.c b/drivers/fsi/occ.c
index 29a17ed..8555e99 100644
--- a/drivers/fsi/occ.c
+++ b/drivers/fsi/occ.c
@@ -23,6 +23,7 @@
 #include <linux/of.h>
 #include <linux/of_platform.h>
 #include <linux/platform_device.h>
+#include <linux/sched.h>
 #include <linux/slab.h>
 #include <linux/spinlock.h>
 #include <linux/uaccess.h>
@@ -33,6 +34,9 @@
 #define OCC_CMD_DATA_BYTES	4090
 #define OCC_RESP_DATA_BYTES	4089
 
+#define OCC_TIMEOUT_MS		1000
+#define OCC_CMD_IN_PRG_WAIT_MS	50
+
 struct occ {
 	struct device *sbefifo;
 	char name[32];
@@ -569,6 +573,9 @@ static void occ_worker(struct work_struct *work)
 {
 	int rc = 0, empty, waiting, canceled;
 	u16 resp_data_length;
+	unsigned long start;
+	const unsigned long timeout = msecs_to_jiffies(OCC_TIMEOUT_MS);
+	const long int wait_time = msecs_to_jiffies(OCC_CMD_IN_PRG_WAIT_MS);
 	struct occ_xfr *xfr;
 	struct occ_response *resp;
 	struct occ_client *client;
@@ -590,6 +597,8 @@ static void occ_worker(struct work_struct *work)
 	spin_unlock_irq(&occ->list_lock);
 	mutex_lock(&occ->occ_lock);
 
+	start = jiffies;
+
 	/* write occ command */
 	rc = occ_putsram(sbefifo, 0xFFFBE000, xfr->buf,
 			 xfr->cmd_data_length);
@@ -601,9 +610,21 @@ static void occ_worker(struct work_struct *work)
 		goto done;
 
 	/* read occ response */
-	rc = occ_getsram(sbefifo, 0xFFFBF000, xfr->buf, 8);
-	if (rc)
-		goto done;
+	do {
+		rc = occ_getsram(sbefifo, 0xFFFBF000, xfr->buf, 8);
+		if (rc)
+			goto done;
+
+		if (resp->return_status == OCC_RESP_CMD_IN_PRG) {
+			rc = -EALREADY;
+
+			if (time_after(jiffies, start + timeout))
+				break;
+
+			set_current_state(TASK_INTERRUPTIBLE);
+			schedule_timeout(wait_time);
+		}
+	} while (rc);
 
 	resp_data_length = get_unaligned_be16(&resp->data_length);
 	if (resp_data_length > OCC_RESP_DATA_BYTES) {
-- 
1.8.3.1

[PATCH linux dev-4.10 v3 28/31] drivers: fsi: occ: Add cancel to remove() and fix probe()

[Eddie James]

From: "Edward A. James" <eajames@us.ibm.com>

Need some data to indicate to clients and the rest of the driver when
the device is being removed, so add a cancel boolean. Fix up both the
probe and remove functions to properly handle failures and prevent
deadlocks.

Signed-off-by: Edward A. James <eajames@us.ibm.com>
---
 drivers/fsi/occ.c | 86 +++++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 62 insertions(+), 24 deletions(-)

diff --git a/drivers/fsi/occ.c b/drivers/fsi/occ.c
index 8555e99..1770823 100644
--- a/drivers/fsi/occ.c
+++ b/drivers/fsi/occ.c
@@ -46,6 +46,7 @@ struct occ {
 	spinlock_t list_lock;		/* lock access to the xfrs list */
 	struct mutex occ_lock;		/* lock access to the hardware */
 	struct work_struct work;
+	bool cancel;
 };
 
 #define to_occ(x)	container_of((x), struct occ, mdev)
@@ -117,12 +118,15 @@ struct occ_client {
 
 static DEFINE_IDA(occ_ida);
 
-static void occ_enqueue_xfr(struct occ_xfr *xfr)
+static int occ_enqueue_xfr(struct occ_xfr *xfr)
 {
 	int empty;
 	struct occ_client *client = to_client(xfr);
 	struct occ *occ = client->occ;
 
+	if (occ->cancel)
+		return -ECANCELED;
+
 	spin_lock_irq(&occ->list_lock);
 
 	empty = list_empty(&occ->xfrs);
@@ -132,14 +136,20 @@ static void occ_enqueue_xfr(struct occ_xfr *xfr)
 
 	if (empty)
 		queue_work(occ_wq, &occ->work);
+
+	return 0;
 }
 
 static struct occ_client *occ_open_common(struct occ *occ, unsigned long flags)
 {
-	struct occ_client *client = kzalloc(sizeof(*client), GFP_KERNEL);
+	struct occ_client *client;
+
+	if (occ->cancel)
+		return ERR_PTR(-ECANCELED);
 
+	client = kzalloc(sizeof(*client), GFP_KERNEL);
 	if (!client)
-		return NULL;
+		return ERR_PTR(-ENOMEM);
 
 	client->occ = occ;
 	spin_lock_init(&client->lock);
@@ -158,8 +168,8 @@ static int occ_open(struct inode *inode, struct file *file)
 	struct occ *occ = to_occ(mdev);
 
 	client = occ_open_common(occ, file->f_flags);
-	if (!client)
-		return -ENOMEM;
+	if (IS_ERR(client))
+		return PTR_ERR(client);
 
 	file->private_data = client;
 
@@ -172,6 +182,7 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
 	int rc;
 	size_t bytes;
 	struct occ_xfr *xfr = &client->xfr;
+	struct occ *occ = client->occ;
 
 	if (len > OCC_SRAM_BYTES)
 		return -EINVAL;
@@ -204,7 +215,8 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
 					      test_bit(XFR_COMPLETE,
 						       &xfr->flags) ||
 					      test_bit(XFR_CANCELED,
-						       &xfr->flags));
+						       &xfr->flags) ||
+					      occ->cancel);
 
 		spin_lock_irq(&client->lock);
 
@@ -272,7 +284,7 @@ static ssize_t occ_write_common(struct occ_client *client,
 
 	spin_lock_irq(&client->lock);
 
-	if (test_and_set_bit(CLIENT_XFR_PENDING, &client->flags)) {
+	if (test_bit(CLIENT_XFR_PENDING, &client->flags)) {
 		rc = -EBUSY;
 		goto done;
 	}
@@ -310,8 +322,11 @@ static ssize_t occ_write_common(struct occ_client *client,
 	xfr->cmd_data_length = data_length + 6;
 	client->read_offset = 0;
 
-	occ_enqueue_xfr(xfr);
+	rc = occ_enqueue_xfr(xfr);
+	if (rc)
+		goto done;
 
+	set_bit(CLIENT_XFR_PENDING, &client->flags);
 	rc = len;
 
 done:
@@ -583,6 +598,9 @@ static void occ_worker(struct work_struct *work)
 	struct device *sbefifo = occ->sbefifo;
 
 again:
+	if (occ->cancel)
+		return;
+
 	spin_lock_irq(&occ->list_lock);
 
 	xfr = list_first_entry_or_null(&occ->xfrs, struct occ_xfr, link);
@@ -676,12 +694,17 @@ static void occ_worker(struct work_struct *work)
 
 struct occ_client *occ_drv_open(struct device *dev, unsigned long flags)
 {
+	struct occ_client *client;
 	struct occ *occ = dev_get_drvdata(dev);
 
 	if (!occ)
 		return NULL;
 
-	return occ_open_common(occ, flags);
+	client = occ_open_common(occ, flags);
+	if (IS_ERR(client))
+		return NULL;
+
+	return client;
 }
 EXPORT_SYMBOL_GPL(occ_drv_open);
 
@@ -752,23 +775,13 @@ static int occ_probe(struct platform_device *pdev)
 			if (occ->idx < 0)
 				occ->idx = ida_simple_get(&occ_ida, 1, INT_MAX,
 							  GFP_KERNEL);
-		} else
+		} else {
 			occ->idx = ida_simple_get(&occ_ida, 1, INT_MAX,
 						  GFP_KERNEL);
-
-		/* create platform devs for dts child nodes (hwmon, etc) */
-		for_each_child_of_node(dev->of_node, np) {
-			snprintf(child_name, sizeof(child_name), "occ%d-dev%d",
-				 occ->idx, child_idx++);
-			child = of_platform_device_create(np, child_name, dev);
-			if (!child)
-				dev_warn(dev,
-					 "failed to create child node dev\n");
 		}
-	} else
+	} else {
 		occ->idx = ida_simple_get(&occ_ida, 1, INT_MAX, GFP_KERNEL);
-
-	platform_set_drvdata(pdev, occ);
+	}
 
 	snprintf(occ->name, sizeof(occ->name), "occ%d", occ->idx);
 	occ->mdev.fops = &occ_fops;
@@ -778,20 +791,45 @@ static int occ_probe(struct platform_device *pdev)
 
 	rc = misc_register(&occ->mdev);
 	if (rc) {
-		dev_err(dev, "failed to register miscdevice\n");
+		dev_err(dev, "failed to register miscdevice: %d\n", rc);
+		ida_simple_remove(&occ_ida, occ->idx);
 		return rc;
 	}
 
+	/* create platform devs for dts child nodes (hwmon, etc) */
+	for_each_available_child_of_node(dev->of_node, np) {
+		snprintf(child_name, sizeof(child_name), "occ%d-dev%d",
+			 occ->idx, child_idx++);
+		child = of_platform_device_create(np, child_name, dev);
+		if (!child)
+			dev_warn(dev, "failed to create child node dev\n");
+	}
+
+	platform_set_drvdata(pdev, occ);
+
 	return 0;
 }
 
 static int occ_remove(struct platform_device *pdev)
 {
 	struct occ *occ = platform_get_drvdata(pdev);
+	struct occ_xfr *xfr;
+	struct occ_client *client;
+
+	occ->cancel = true;
+
+	spin_lock_irq(&occ->list_lock);
+	list_for_each_entry(xfr, &occ->xfrs, link) {
+		client = to_client(xfr);
+		wake_up_all(&client->wait);
+	}
+	spin_unlock_irq(&occ->list_lock);
 
-	flush_work(&occ->work);
 	misc_deregister(&occ->mdev);
 	device_for_each_child(&pdev->dev, NULL, occ_unregister_child);
+
+	cancel_work_sync(&occ->work);
+
 	ida_simple_remove(&occ_ida, occ->idx);
 
 	return 0;
-- 
1.8.3.1

[PATCH linux dev-4.10 v3 29/31] drivers: fsi: occ: Fix client memory management

[Eddie James]

From: "Edward A. James" <eajames@us.ibm.com>

Potential for bad memory access in the worker function. Now fixed by
using reference counters.

Signed-off-by: Edward A. James <eajames@us.ibm.com>
---
 drivers/fsi/occ.c | 92 ++++++++++++++++++++++++++-----------------------------
 1 file changed, 43 insertions(+), 49 deletions(-)

diff --git a/drivers/fsi/occ.c b/drivers/fsi/occ.c
index 1770823..f2b5b95 100644
--- a/drivers/fsi/occ.c
+++ b/drivers/fsi/occ.c
@@ -70,15 +70,11 @@ struct occ_response {
  *  and cleared if the transfer fails or occ_worker_getsram completes.
  * XFR_COMPLETE is set when a transfer fails or finishes occ_worker_getsram.
  * XFR_CANCELED is set when the transfer's client is released.
- * XFR_WAITING is set from read() if the transfer isn't complete and
- *  O_NONBLOCK wasn't specified. Cleared in read() when transfer completes or
- *  fails.
  */
 enum {
 	XFR_IN_PROGRESS,
 	XFR_COMPLETE,
 	XFR_CANCELED,
-	XFR_WAITING,
 };
 
 struct occ_xfr {
@@ -104,6 +100,7 @@ enum {
 };
 
 struct occ_client {
+	struct kref kref;
 	struct occ *occ;
 	struct occ_xfr xfr;
 	spinlock_t lock;		/* lock access to the client state */
@@ -140,6 +137,24 @@ static int occ_enqueue_xfr(struct occ_xfr *xfr)
 	return 0;
 }
 
+static void occ_get_client(struct occ_client *client)
+{
+	kref_get(&client->kref);
+}
+
+static void occ_client_release(struct kref *kref)
+{
+	struct occ_client *client = container_of(kref, struct occ_client,
+						 kref);
+
+	kfree(client);
+}
+
+static void occ_put_client(struct occ_client *client)
+{
+	kref_put(&client->kref, occ_client_release);
+}
+
 static struct occ_client *occ_open_common(struct occ *occ, unsigned long flags)
 {
 	struct occ_client *client;
@@ -152,6 +167,7 @@ static struct occ_client *occ_open_common(struct occ *occ, unsigned long flags)
 		return ERR_PTR(-ENOMEM);
 
 	client->occ = occ;
+	kref_init(&client->kref);
 	spin_lock_init(&client->lock);
 	init_waitqueue_head(&client->wait);
 
@@ -187,6 +203,7 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
 	if (len > OCC_SRAM_BYTES)
 		return -EINVAL;
 
+	occ_get_client(client);
 	spin_lock_irq(&client->lock);
 
 	if (!test_bit(CLIENT_XFR_PENDING, &client->flags)) {
@@ -207,8 +224,6 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
 			goto done;
 		}
 
-		set_bit(XFR_WAITING, &xfr->flags);
-
 		spin_unlock_irq(&client->lock);
 
 		rc = wait_event_interruptible(client->wait,
@@ -220,15 +235,12 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
 
 		spin_lock_irq(&client->lock);
 
-		if (test_bit(XFR_CANCELED, &xfr->flags)) {
-			spin_unlock_irq(&client->lock);
-			kfree(client);
-			return -EBADFD;
-		}
-
-		clear_bit(XFR_WAITING, &xfr->flags);
 		if (!test_bit(XFR_COMPLETE, &xfr->flags)) {
-			rc = -EINTR;
+			if (occ->cancel || test_bit(XFR_CANCELED, &xfr->flags))
+				rc = -ECANCELED;
+			else
+				rc = -EINTR;
+
 			goto done;
 		}
 	}
@@ -259,6 +271,7 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
 
 done:
 	spin_unlock_irq(&client->lock);
+	occ_put_client(client);
 	return rc;
 }
 
@@ -282,6 +295,7 @@ static ssize_t occ_write_common(struct occ_client *client,
 	if (len > (OCC_CMD_DATA_BYTES + 3) || len < 3)
 		return -EINVAL;
 
+	occ_get_client(client);
 	spin_lock_irq(&client->lock);
 
 	if (test_bit(CLIENT_XFR_PENDING, &client->flags)) {
@@ -331,6 +345,7 @@ static ssize_t occ_write_common(struct occ_client *client,
 
 done:
 	spin_unlock_irq(&client->lock);
+	occ_put_client(client);
 	return rc;
 }
 
@@ -349,38 +364,26 @@ static int occ_release_common(struct occ_client *client)
 
 	spin_lock_irq(&client->lock);
 
-	if (!test_bit(CLIENT_XFR_PENDING, &client->flags)) {
-		spin_unlock_irq(&client->lock);
-		kfree(client);
-		return 0;
-	}
+	set_bit(XFR_CANCELED, &xfr->flags);
+	if (!test_bit(CLIENT_XFR_PENDING, &client->flags))
+		goto done;
 
 	spin_lock_irq(&occ->list_lock);
 
-	set_bit(XFR_CANCELED, &xfr->flags);
 	if (!test_bit(XFR_IN_PROGRESS, &xfr->flags)) {
 		/* already deleted from list if complete */
 		if (!test_bit(XFR_COMPLETE, &xfr->flags))
 			list_del(&xfr->link);
-
-		spin_unlock_irq(&occ->list_lock);
-
-		if (test_bit(XFR_WAITING, &xfr->flags)) {
-			/* blocking read; let reader clean up */
-			wake_up_interruptible(&client->wait);
-			spin_unlock_irq(&client->lock);
-			return 0;
-		}
-
-		spin_unlock_irq(&client->lock);
-
-		kfree(client);
-		return 0;
 	}
 
-	/* operation is in progress; let worker clean up */
 	spin_unlock_irq(&occ->list_lock);
+
+	wake_up_all(&client->wait);
+
+done:
 	spin_unlock_irq(&client->lock);
+
+	occ_put_client(client);
 	return 0;
 }
 
@@ -586,7 +589,7 @@ static int occ_trigger_attn(struct device *sbefifo)
 
 static void occ_worker(struct work_struct *work)
 {
-	int rc = 0, empty, waiting, canceled;
+	int rc = 0, empty;
 	u16 resp_data_length;
 	unsigned long start;
 	const unsigned long timeout = msecs_to_jiffies(OCC_TIMEOUT_MS);
@@ -609,6 +612,8 @@ static void occ_worker(struct work_struct *work)
 		return;
 	}
 
+	client = to_client(xfr);
+	occ_get_client(client);
 	resp = (struct occ_response *)xfr->buf;
 	set_bit(XFR_IN_PROGRESS, &xfr->flags);
 
@@ -664,29 +669,18 @@ static void occ_worker(struct work_struct *work)
 	mutex_unlock(&occ->occ_lock);
 
 	xfr->rc = rc;
-	client = to_client(xfr);
-
-	/* lock client to prevent race with read() */
-	spin_lock_irq(&client->lock);
-
 	set_bit(XFR_COMPLETE, &xfr->flags);
-	waiting = test_bit(XFR_WAITING, &xfr->flags);
-
-	spin_unlock_irq(&client->lock);
 
 	spin_lock_irq(&occ->list_lock);
 
 	clear_bit(XFR_IN_PROGRESS, &xfr->flags);
 	list_del(&xfr->link);
 	empty = list_empty(&occ->xfrs);
-	canceled = test_bit(XFR_CANCELED, &xfr->flags);
 
 	spin_unlock_irq(&occ->list_lock);
 
-	if (waiting)
-		wake_up_interruptible(&client->wait);
-	else if (canceled)
-		kfree(client);
+	wake_up_interruptible(&client->wait);
+	occ_put_client(client);
 
 	if (!empty)
 		goto again;
-- 
1.8.3.1

[PATCH linux dev-4.10 v3 30/31] drivers/hwmon/occ: Remove repeated ops for OCC command in progress

[Eddie James]

From: "Edward A. James" <eajames@us.ibm.com>

This is now handled in the occ driver.

Signed-off-by: Edward A. James <eajames@us.ibm.com>
---
 drivers/hwmon/occ/p9_sbe.c | 14 +-------------
 1 file changed, 1 insertion(+), 13 deletions(-)

diff --git a/drivers/hwmon/occ/p9_sbe.c b/drivers/hwmon/occ/p9_sbe.c
index 2d50a94..c7e0d9c 100644
--- a/drivers/hwmon/occ/p9_sbe.c
+++ b/drivers/hwmon/occ/p9_sbe.c
@@ -26,14 +26,10 @@ struct p9_sbe_occ {
 static int p9_sbe_occ_send_cmd(struct occ *occ, u8 *cmd)
 {
 	int rc, error;
-	unsigned long start;
 	struct occ_client *client;
 	struct occ_response *resp = &occ->resp;
 	struct p9_sbe_occ *p9_sbe_occ = to_p9_sbe_occ(occ);
 
-	start = jiffies;
-
-retry:
 	client = occ_drv_open(p9_sbe_occ->sbe, 0);
 	if (!client) {
 		rc = -ENODEV;
@@ -52,15 +48,7 @@ static int p9_sbe_occ_send_cmd(struct occ *occ, u8 *cmd)
 
 	switch (resp->return_status) {
 	case RESP_RETURN_CMD_IN_PRG:
-		if (time_after(jiffies,
-			       start + msecs_to_jiffies(OCC_TIMEOUT_MS)))
-			rc = -EALREADY;
-		else {
-			set_current_state(TASK_INTERRUPTIBLE);
-			schedule_timeout(msecs_to_jiffies(OCC_CMD_IN_PRG_MS));
-
-			goto retry;
-		}
+		rc = -ETIMEDOUT;
 		break;
 	case RESP_RETURN_SUCCESS:
 		occ_reset_error(occ);
-- 
1.8.3.1

[PATCH linux dev-4.10 v3 31/31] drivers: hwmon: occ: Cancel occ operations in remove()

[Eddie James]

From: "Edward A. James" <eajames@us.ibm.com>

Prevent hanging forever waiting for OCC ops to complete.

Signed-off-by: Edward A. James <eajames@us.ibm.com>
---
 drivers/hwmon/occ/p9_sbe.c | 35 ++++++++++++++++++++++++++++-------
 1 file changed, 28 insertions(+), 7 deletions(-)

diff --git a/drivers/hwmon/occ/p9_sbe.c b/drivers/hwmon/occ/p9_sbe.c
index c7e0d9c..9842e0d 100644
--- a/drivers/hwmon/occ/p9_sbe.c
+++ b/drivers/hwmon/occ/p9_sbe.c
@@ -14,37 +14,54 @@
 #include <linux/platform_device.h>
 #include <linux/occ.h>
 #include <linux/sched.h>
+#include <linux/spinlock.h>
 #include <linux/workqueue.h>
 
 struct p9_sbe_occ {
 	struct occ occ;
 	struct device *sbe;
+	struct occ_client *client;
+	spinlock_t lock;
 };
 
 #define to_p9_sbe_occ(x)	container_of((x), struct p9_sbe_occ, occ)
 
+static void p9_sbe_occ_close_client(struct p9_sbe_occ *occ)
+{
+	struct occ_client *tmp_client;
+
+	spin_lock_irq(&occ->lock);
+	tmp_client = occ->client;
+	occ->client = NULL;
+	occ_drv_release(tmp_client);
+	spin_unlock_irq(&occ->lock);
+}
+
 static int p9_sbe_occ_send_cmd(struct occ *occ, u8 *cmd)
 {
 	int rc, error;
-	struct occ_client *client;
 	struct occ_response *resp = &occ->resp;
 	struct p9_sbe_occ *p9_sbe_occ = to_p9_sbe_occ(occ);
 
-	client = occ_drv_open(p9_sbe_occ->sbe, 0);
-	if (!client) {
+	spin_lock_irq(&p9_sbe_occ->lock);
+	if (p9_sbe_occ->sbe)
+		p9_sbe_occ->client = occ_drv_open(p9_sbe_occ->sbe, 0);
+	spin_unlock_irq(&p9_sbe_occ->lock);
+
+	if (!p9_sbe_occ->client) {
 		rc = -ENODEV;
 		goto assign;
 	}
 
-	rc = occ_drv_write(client, (const char *)&cmd[1], 7);
+	rc = occ_drv_write(p9_sbe_occ->client, (const char *)&cmd[1], 7);
 	if (rc < 0)
 		goto err;
 
-	rc = occ_drv_read(client, (char *)resp, sizeof(*resp));
+	rc = occ_drv_read(p9_sbe_occ->client, (char *)resp, sizeof(*resp));
 	if (rc < 0)
 		goto err;
 
-	occ_drv_release(client);
+	p9_sbe_occ_close_client(p9_sbe_occ);
 
 	switch (resp->return_status) {
 	case RESP_RETURN_CMD_IN_PRG:
@@ -72,7 +89,7 @@ static int p9_sbe_occ_send_cmd(struct occ *occ, u8 *cmd)
 	goto done;
 
 err:
-	occ_drv_release(client);
+	p9_sbe_occ_close_client(p9_sbe_occ);
 	dev_err(occ->bus_dev, "occ bus op failed rc:%d\n", rc);
 assign:
 	error = rc;
@@ -132,6 +149,7 @@ static int p9_sbe_occ_probe(struct platform_device *pdev)
 	p9_sbe_occ->sbe = pdev->dev.parent;
 
 	occ = &p9_sbe_occ->occ;
+	spin_lock_init(&p9_sbe_occ->lock);
 	occ->bus_dev = &pdev->dev;
 	occ->groups[0] = &occ->group;
 	occ->poll_cmd_data = 0x20;
@@ -152,7 +170,10 @@ static int p9_sbe_occ_probe(struct platform_device *pdev)
 static int p9_sbe_occ_remove(struct platform_device *pdev)
 {
 	struct occ *occ = platform_get_drvdata(pdev);
+	struct p9_sbe_occ *p9_sbe_occ = to_p9_sbe_occ(occ);
 
+	p9_sbe_occ->sbe = NULL;
+	p9_sbe_occ_close_client(p9_sbe_occ);
 	occ_remove_status_attrs(occ);
 
 	atomic_dec(&occ_num_occs);
-- 
1.8.3.1

Re: [PATCH linux dev-4.10 v3 01/31] drivers: fsi: sbefifo: Fix includes

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 1438 bytes --]

On Thu, 2017-10-05 at 14:23 -0500, Eddie James wrote:
> From: "Edward A. James" <eajames@us.ibm.com>
> 
> Need to include everything we use and not rely on recursive includes.
> 
> Signed-off-by: Edward A. James <eajames@us.ibm.com>

Acked-by: Andrew Jeffery <andrew@aj.id.au>

> ---
>  drivers/fsi/fsi-sbefifo.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/fsi/fsi-sbefifo.c b/drivers/fsi/fsi-sbefifo.c
> index 1c37ff7..6fa75a8 100644
> --- a/drivers/fsi/fsi-sbefifo.c
> +++ b/drivers/fsi/fsi-sbefifo.c
> @@ -11,20 +11,27 @@
>   * GNU General Public License for more details.
>   */
>  
> -#include <linux/delay.h>
> +#include <linux/device.h>
>  #include <linux/errno.h>
> -#include <linux/idr.h>
> +#include <linux/fs.h>
>  #include <linux/fsi.h>
> +#include <linux/fsi-sbefifo.h>
> +#include <linux/idr.h>
> +#include <linux/kernel.h>
> +#include <linux/kref.h>
>  #include <linux/list.h>
>  #include <linux/miscdevice.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
> +#include <linux/of_device.h>
>  #include <linux/of_platform.h>
>  #include <linux/poll.h>
>  #include <linux/sched.h>
>  #include <linux/slab.h>
> +#include <linux/spinlock.h>
>  #include <linux/timer.h>
>  #include <linux/uaccess.h>
> +#include <linux/wait.h>
>  
>  /*
>   * The SBEFIFO is a pipe-like FSI device for communicating with

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH linux dev-4.10 v3 02/31] drivers: fsi: sbefifo: Use a defined reschedule length

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 1482 bytes --]

On Thu, 2017-10-05 at 14:23 -0500, Eddie James wrote:
> From: "Edward A. James" <eajames@us.ibm.com>
> 
> Switch to a #defined reschedule period instead of using 500 ms twice.
> 
> Signed-off-by: Edward A. James <eajames@us.ibm.com>

Acked-by: Andrew Jeffery <andrew@aj.id.au>

> ---
>  drivers/fsi/fsi-sbefifo.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/fsi/fsi-sbefifo.c b/drivers/fsi/fsi-sbefifo.c
> index 6fa75a8..2e45a75 100644
> --- a/drivers/fsi/fsi-sbefifo.c
> +++ b/drivers/fsi/fsi-sbefifo.c
> @@ -51,6 +51,8 @@
>  #define   SBEFIFO_EOT_MAGIC		0xffffffff
>  #define SBEFIFO_EOT_ACK		0x14
>  
> +#define SBEFIFO_RESCHEDULE	msecs_to_jiffies(500)
> +
>  struct sbefifo {
>  	struct timer_list poll_timer;
>  	struct fsi_device *fsi_dev;
> @@ -404,7 +406,7 @@ static void sbefifo_poll_timer(unsigned long data)
>  		if (devn == 0) {
>  			/* No open slot for write.  Reschedule. */
>  			sbefifo->poll_timer.expires = jiffies +
> -				msecs_to_jiffies(500);
> +				SBEFIFO_RESCHEDULE;
>  			add_timer(&sbefifo->poll_timer);
>  			goto out_unlock;
>  		}
> @@ -445,7 +447,7 @@ static void sbefifo_poll_timer(unsigned long data)
>  		if (devn == 0) {
>  			/* No data yet.  Reschedule. */
>  			sbefifo->poll_timer.expires = jiffies +
> -				msecs_to_jiffies(500);
> +				SBEFIFO_RESCHEDULE;
>  			add_timer(&sbefifo->poll_timer);
>  			goto out_unlock;
>  		}

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH linux dev-4.10 v3 03/31] drivers: fsi: sbefifo: Use __be32 for big endian values

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 1520 bytes --]

On Thu, 2017-10-05 at 14:23 -0500, Eddie James wrote:
> From: "Edward A. James" <eajames@us.ibm.com>
> 
> Use a real __be32 value instead of u32 for big endian values.
> 
> Signed-off-by: Edward A. James <eajames@us.ibm.com>

Reviewed-by: Andrew Jeffery <andrew@aj.id.au>

> ---
>  drivers/fsi/fsi-sbefifo.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/fsi/fsi-sbefifo.c b/drivers/fsi/fsi-sbefifo.c
> index 2e45a75..c5eeace 100644
> --- a/drivers/fsi/fsi-sbefifo.c
> +++ b/drivers/fsi/fsi-sbefifo.c
> @@ -103,7 +103,7 @@ struct sbefifo_client {
>  static int sbefifo_inw(struct sbefifo *sbefifo, int reg, u32 *word)
>  {
>  	int rc;
> -	u32 raw_word;
> +	__be32 raw_word;
>  
>  	rc = fsi_device_read(sbefifo->fsi_dev, reg, &raw_word,
>  			     sizeof(raw_word));
> @@ -111,17 +111,19 @@ static int sbefifo_inw(struct sbefifo *sbefifo, int reg, u32 *word)
>  		return rc;
>  
>  	*word = be32_to_cpu(raw_word);
> +
>  	return 0;
>  }
>  
>  static int sbefifo_outw(struct sbefifo *sbefifo, int reg, u32 word)
>  {
> -	u32 raw_word = cpu_to_be32(word);
> +	__be32 raw_word = cpu_to_be32(word);
>  
>  	return fsi_device_write(sbefifo->fsi_dev, reg, &raw_word,
>  				sizeof(raw_word));
>  }
>  
> +/* Don't flip endianness of data to/from FIFO, just pass through. */
>  static int sbefifo_readw(struct sbefifo *sbefifo, u32 *word)
>  {
>  	return fsi_device_read(sbefifo->fsi_dev, SBEFIFO_DWN, word,

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH linux dev-4.10 v3 04/31] drivers: fsi: sbefifo: white space fixes

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 8643 bytes --]

On Thu, 2017-10-05 at 14:23 -0500, Eddie James wrote:
> From: "Edward A. James" <eajames@us.ibm.com>
> 
> Fix some spacing issues.
> 
> Signed-off-by: Edward A. James <eajames@us.ibm.com>

Reviewed-by: Andrew Jeffery <andrew@aj.id.au>

> ---
>  drivers/fsi/fsi-sbefifo.c | 66 ++++++++++++++++++++++++-----------------------
>  1 file changed, 34 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/fsi/fsi-sbefifo.c b/drivers/fsi/fsi-sbefifo.c
> index c5eeace..561a7f6 100644
> --- a/drivers/fsi/fsi-sbefifo.c
> +++ b/drivers/fsi/fsi-sbefifo.c
> @@ -147,7 +147,7 @@ static int sbefifo_ack_eot(struct sbefifo *sbefifo)
>  		return ret;
>  
>  	return sbefifo_outw(sbefifo, SBEFIFO_DWN | SBEFIFO_EOT_ACK,
> -			SBEFIFO_EOT_MAGIC);
> +			    SBEFIFO_EOT_MAGIC);
>  }
>  
>  static size_t sbefifo_dev_nwreadable(u32 sts)
> @@ -221,6 +221,7 @@ static bool sbefifo_buf_readnb(struct sbefifo_buf *buf, size_t n)
>  		rpos = buf->buf;
>  
>  	WRITE_ONCE(buf->rpos, rpos);
> +
>  	return rpos == wpos;
>  }
>  
> @@ -240,14 +241,14 @@ static bool sbefifo_buf_wrotenb(struct sbefifo_buf *buf, size_t n)
>  		set_bit(SBEFIFO_BUF_FULL, &buf->flags);
>  
>  	WRITE_ONCE(buf->wpos, wpos);
> +
>  	return rpos == wpos;
>  }
>  
>  static void sbefifo_free(struct kref *kref)
>  {
> -	struct sbefifo *sbefifo;
> +	struct sbefifo *sbefifo = container_of(kref, struct sbefifo, kref);
>  
> -	sbefifo = container_of(kref, struct sbefifo, kref);
>  	kfree(sbefifo);
>  }
>  
> @@ -284,8 +285,7 @@ static struct sbefifo_xfr *sbefifo_client_next_xfr(
>  	if (list_empty(&client->xfrs))
>  		return NULL;
>  
> -	return container_of(client->xfrs.next, struct sbefifo_xfr,
> -			client);
> +	return container_of(client->xfrs.next, struct sbefifo_xfr, client);
>  }
>  
>  static bool sbefifo_xfr_rsp_pending(struct sbefifo_client *client)
> @@ -360,6 +360,7 @@ static struct sbefifo_xfr *sbefifo_next_xfr(struct sbefifo *sbefifo)
>  			kfree(xfr);
>  			continue;
>  		}
> +
>  		return xfr;
>  	}
>  
> @@ -381,7 +382,7 @@ static void sbefifo_poll_timer(unsigned long data)
>  
>  	spin_lock(&sbefifo->lock);
>  	xfr = list_first_entry_or_null(&sbefifo->xfrs, struct sbefifo_xfr,
> -			xfrs);
> +				       xfrs);
>  	if (!xfr)
>  		goto out_unlock;
>  
> @@ -399,8 +400,7 @@ static void sbefifo_poll_timer(unsigned long data)
>  
>  	 /* Drain the write buffer. */
>  	while ((bufn = sbefifo_buf_nbreadable(wbuf))) {
> -		ret = sbefifo_inw(sbefifo, SBEFIFO_UP | SBEFIFO_STS,
> -				&sts);
> +		ret = sbefifo_inw(sbefifo, SBEFIFO_UP | SBEFIFO_STS, &sts);
>  		if (ret)
>  			goto out;
>  
> @@ -425,9 +425,8 @@ static void sbefifo_poll_timer(unsigned long data)
>  
>  	 /* Send EOT if the writer is finished. */
>  	if (test_and_clear_bit(SBEFIFO_XFR_WRITE_DONE, &xfr->flags)) {
> -		ret = sbefifo_outw(sbefifo,
> -				SBEFIFO_UP | SBEFIFO_EOT_RAISE,
> -				SBEFIFO_EOT_MAGIC);
> +		ret = sbefifo_outw(sbefifo, SBEFIFO_UP | SBEFIFO_EOT_RAISE,
> +				   SBEFIFO_EOT_MAGIC);
>  		if (ret)
>  			goto out;
>  
> @@ -477,7 +476,7 @@ static void sbefifo_poll_timer(unsigned long data)
>  			set_bit(SBEFIFO_XFR_COMPLETE, &xfr->flags);
>  			list_del(&xfr->xfrs);
>  			if (unlikely(test_bit(SBEFIFO_XFR_CANCEL,
> -							&xfr->flags)))
> +					      &xfr->flags)))
>  				kfree(xfr);
>  			break;
>  		}
> @@ -487,7 +486,7 @@ static void sbefifo_poll_timer(unsigned long data)
>  	if (unlikely(ret)) {
>  		sbefifo->rc = ret;
>  		dev_err(&sbefifo->fsi_dev->dev,
> -				"Fatal bus access failure: %d\n", ret);
> +			"Fatal bus access failure: %d\n", ret);
>  		list_for_each_entry(xfr, &sbefifo->xfrs, xfrs)
>  			kfree(xfr);
>  		INIT_LIST_HEAD(&sbefifo->xfrs);
> @@ -508,7 +507,7 @@ static void sbefifo_poll_timer(unsigned long data)
>  static int sbefifo_open(struct inode *inode, struct file *file)
>  {
>  	struct sbefifo *sbefifo = container_of(file->private_data,
> -			struct sbefifo, mdev);
> +					       struct sbefifo, mdev);
>  	struct sbefifo_client *client;
>  	int ret;
>  
> @@ -562,12 +561,13 @@ static ssize_t sbefifo_read_common(struct sbefifo_client *client,
>  
>  	sbefifo_get_client(client);
>  	if (wait_event_interruptible(sbefifo->wait,
> -				(ret = READ_ONCE(sbefifo->rc)) ||
> -				(n = sbefifo_buf_nbreadable(
> -						&client->rbuf)))) {
> +				     (ret = READ_ONCE(sbefifo->rc)) ||
> +				     (n = sbefifo_buf_nbreadable(
> +					&client->rbuf)))) {
>  		sbefifo_put_client(client);
>  		return -ERESTARTSYS;
>  	}
> +
>  	if (ret) {
>  		INIT_LIST_HEAD(&client->xfrs);
>  		sbefifo_put_client(client);
> @@ -581,8 +581,9 @@ static ssize_t sbefifo_read_common(struct sbefifo_client *client,
>  			sbefifo_put_client(client);
>  			return -EFAULT;
>  		}
> -	} else
> +	} else {
>  		memcpy(kbuf, READ_ONCE(client->rbuf.rpos), n);
> +	}
>  
>  	if (sbefifo_buf_readnb(&client->rbuf, n)) {
>  		xfr = sbefifo_client_next_xfr(client);
> @@ -605,8 +606,8 @@ static ssize_t sbefifo_read_common(struct sbefifo_client *client,
>  	return n;
>  }
>  
> -static ssize_t sbefifo_read(struct file *file, char __user *buf,
> -		size_t len, loff_t *offset)
> +static ssize_t sbefifo_read(struct file *file, char __user *buf, size_t len,
> +			    loff_t *offset)
>  {
>  	struct sbefifo_client *client = file->private_data;
>  
> @@ -648,6 +649,7 @@ static ssize_t sbefifo_write_common(struct sbefifo_client *client,
>  		spin_unlock_irq(&sbefifo->lock);
>  		return -ENOMEM;
>  	}
> +
>  	spin_unlock_irq(&sbefifo->lock);
>  
>  	sbefifo_get_client(client);
> @@ -659,8 +661,8 @@ static ssize_t sbefifo_write_common(struct sbefifo_client *client,
>  	while (len) {
>  		if (wait_event_interruptible(sbefifo->wait,
>  				READ_ONCE(sbefifo->rc) ||
> -					(sbefifo_client_next_xfr(client) == xfr &&
> -					 (n = sbefifo_buf_nbwriteable(
> +				(sbefifo_client_next_xfr(client) == xfr &&
> +				 (n = sbefifo_buf_nbwriteable(
>  							&client->wbuf))))) {
>  			set_bit(SBEFIFO_XFR_CANCEL, &xfr->flags);
>  			sbefifo_get(sbefifo);
> @@ -670,6 +672,7 @@ static ssize_t sbefifo_write_common(struct sbefifo_client *client,
>  			sbefifo_put_client(client);
>  			return -ERESTARTSYS;
>  		}
> +
>  		if (sbefifo->rc) {
>  			INIT_LIST_HEAD(&client->xfrs);
>  			sbefifo_put_client(client);
> @@ -719,7 +722,7 @@ static ssize_t sbefifo_write_common(struct sbefifo_client *client,
>  }
>  
>  static ssize_t sbefifo_write(struct file *file, const char __user *buf,
> -		size_t len, loff_t *offset)
> +			     size_t len, loff_t *offset)
>  {
>  	struct sbefifo_client *client = file->private_data;
>  
> @@ -820,22 +823,21 @@ static int sbefifo_probe(struct device *dev)
>  
>  	sbefifo->fsi_dev = fsi_dev;
>  
> -	ret = sbefifo_inw(sbefifo,
> -			SBEFIFO_UP | SBEFIFO_STS, &sts);
> +	ret = sbefifo_inw(sbefifo, SBEFIFO_UP | SBEFIFO_STS, &sts);
>  	if (ret)
>  		return ret;
> +
>  	if (!(sts & SBEFIFO_EMPTY)) {
> -		dev_err(&sbefifo->fsi_dev->dev,
> -				"Found data in upstream fifo\n");
> +		dev_err(dev, "Found data in upstream fifo\n");
>  		return -EIO;
>  	}
>  
>  	ret = sbefifo_inw(sbefifo, SBEFIFO_DWN | SBEFIFO_STS, &sts);
>  	if (ret)
>  		return ret;
> +
>  	if (!(sts & SBEFIFO_EMPTY)) {
> -		dev_err(&sbefifo->fsi_dev->dev,
> -				"Found data in downstream fifo\n");
> +		dev_err(dev, "Found data in downstream fifo\n");
>  		return -EIO;
>  	}
>  
> @@ -848,13 +850,13 @@ static int sbefifo_probe(struct device *dev)
>  
>  	sbefifo->idx = ida_simple_get(&sbefifo_ida, 1, INT_MAX, GFP_KERNEL);
>  	snprintf(sbefifo->name, sizeof(sbefifo->name), "sbefifo%d",
> -			sbefifo->idx);
> +		 sbefifo->idx);
>  	init_waitqueue_head(&sbefifo->wait);
>  	INIT_LIST_HEAD(&sbefifo->xfrs);
>  
>  	/* This bit of silicon doesn't offer any interrupts... */
>  	setup_timer(&sbefifo->poll_timer, sbefifo_poll_timer,
> -			(unsigned long)sbefifo);
> +		    (unsigned long)sbefifo);
>  
>  	if (dev->of_node) {
>  		/* create platform devs for dts child nodes (occ, etc) */
> @@ -863,7 +865,7 @@ static int sbefifo_probe(struct device *dev)
>  				 sbefifo->name, child_idx++);
>  			child = of_platform_device_create(np, child_name, dev);
>  			if (!child)
> -				dev_warn(&sbefifo->fsi_dev->dev,
> +				dev_warn(dev,
>  					 "failed to create child node dev\n");
>  		}
>  	}

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH linux dev-4.10 v3 05/31] drivers: fsi: sbefifo: replace awkward wait_event expression

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 2892 bytes --]

On Thu, 2017-10-05 at 14:23 -0500, Eddie James wrote:
> From: "Edward A. James" <eajames@us.ibm.com>
> 
> Use ready() functions to replace awkward boolean expressions in the
> wait_event_interruptible calls.
> 
> Signed-off-by: Edward A. James <eajames@us.ibm.com>

Reviewed-by: Andrew Jeffery <andrew@aj.id.au>

> ---
>  drivers/fsi/fsi-sbefifo.c | 32 +++++++++++++++++++++++++-------
>  1 file changed, 25 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/fsi/fsi-sbefifo.c b/drivers/fsi/fsi-sbefifo.c
> index 561a7f6..dbac0c3 100644
> --- a/drivers/fsi/fsi-sbefifo.c
> +++ b/drivers/fsi/fsi-sbefifo.c
> @@ -545,6 +545,16 @@ static unsigned int sbefifo_poll(struct file
*file, poll_table *wait)
>  	return mask;
>  }
>  
> +static bool sbefifo_read_ready(struct sbefifo *sbefifo,
> +			       struct sbefifo_client *client, size_t
*n,
> +			       size_t *ret)
> +{
> +	*n = sbefifo_buf_nbreadable(&client->rbuf);
> +	*ret = READ_ONCE(sbefifo->rc);
> +
> +	return *ret || *n;
> +}
> +
>  static ssize_t sbefifo_read_common(struct sbefifo_client *client,
>  				   char __user *ubuf, char *kbuf,
size_t len)
>  {
> @@ -561,9 +571,8 @@ static ssize_t sbefifo_read_common(struct
sbefifo_client *client,
>  
>  	sbefifo_get_client(client);
>  	if (wait_event_interruptible(sbefifo->wait,
> -				     (ret = READ_ONCE(sbefifo->rc))
||
> -				     (n = sbefifo_buf_nbreadable(
> -					&client->rbuf)))) {
> +				     sbefifo_read_ready(sbefifo,
client, &n,
> +							&ret))) {
>  		sbefifo_put_client(client);
>  		return -ERESTARTSYS;
>  	}
> @@ -619,6 +628,16 @@ static ssize_t sbefifo_read(struct file *file,
char __user *buf, size_t len,
>  	return sbefifo_read_common(client, buf, NULL, len);
>  }
>  
> +static bool sbefifo_write_ready(struct sbefifo *sbefifo,
> +				struct sbefifo_xfr *xfr,
> +				struct sbefifo_client *client,
size_t *n)
> +{
> +	struct sbefifo_xfr *next = sbefifo_client_next_xfr(client);
> +
> +	*n = sbefifo_buf_nbwriteable(&client->wbuf);
> +	return READ_ONCE(sbefifo->rc) || (next == xfr && *n);
> +}
> +
>  static ssize_t sbefifo_write_common(struct sbefifo_client *client,
>  				    const char __user *ubuf, const
char *kbuf,
>  				    size_t len)
> @@ -660,10 +679,9 @@ static ssize_t sbefifo_write_common(struct
sbefifo_client *client,
>  	 */
>  	while (len) {
>  		if (wait_event_interruptible(sbefifo->wait,
> -				READ_ONCE(sbefifo->rc) ||
> -				(sbefifo_client_next_xfr(client) ==
xfr &&
> -				 (n = sbefifo_buf_nbwriteable(
> -							&client-
>wbuf))))) {
> +					     sbefifo_write_ready(sbe
fifo, xfr,
> +								 cli
ent,
> +								 &n)
)) {
>  			set_bit(SBEFIFO_XFR_CANCEL, &xfr->flags);
>  			sbefifo_get(sbefifo);
>  			if (mod_timer(&sbefifo->poll_timer,
jiffies))

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH linux dev-4.10 v3 06/31] drivers: fsi: sbefifo: remove redundant function

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 2538 bytes --]

On Thu, 2017-10-05 at 14:23 -0500, Eddie James wrote:
> From: "Edward A. James" <eajames@us.ibm.com>
> 
> sbefifo_client_next_xfr function just does what list_first_entry_or_null
> does.

Any reason you didn't implement sbefifo_client_next_xfr() in terms of
list_first_entry_or_null() rather than open-code it?

> 
> Signed-off-by: Edward A. James <eajames@us.ibm.com>
> ---
>  drivers/fsi/fsi-sbefifo.c | 28 +++++++++++++++-------------
>  1 file changed, 15 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/fsi/fsi-sbefifo.c b/drivers/fsi/fsi-sbefifo.c
> index dbac0c3..173cd03 100644
> --- a/drivers/fsi/fsi-sbefifo.c
> +++ b/drivers/fsi/fsi-sbefifo.c
> @@ -279,20 +279,12 @@ static struct sbefifo_xfr *sbefifo_enq_xfr(struct sbefifo_client *client)
>  	return xfr;
>  }
>  
> -static struct sbefifo_xfr *sbefifo_client_next_xfr(
> -		struct sbefifo_client *client)
> -{
> -	if (list_empty(&client->xfrs))
> -		return NULL;
> -
> -	return container_of(client->xfrs.next, struct sbefifo_xfr, client);
> -}
> -
>  static bool sbefifo_xfr_rsp_pending(struct sbefifo_client *client)
>  {
> -	struct sbefifo_xfr *xfr;
> +	struct sbefifo_xfr *xfr = list_first_entry_or_null(&client->xfrs,
> +							   struct sbefifo_xfr,
> +							   client);
>  
> -	xfr = sbefifo_client_next_xfr(client);
>  	if (xfr && test_bit(SBEFIFO_XFR_RESP_PENDING, &xfr->flags))
>  		return true;
>  
> @@ -595,7 +587,15 @@ static ssize_t sbefifo_read_common(struct sbefifo_client *client,
>  	}
>  
>  	if (sbefifo_buf_readnb(&client->rbuf, n)) {
> -		xfr = sbefifo_client_next_xfr(client);
> +		xfr = list_first_entry_or_null(&client->xfrs,
> +					       struct sbefifo_xfr, client);
> +		if (!xfr) {
> +			/* should be impossible to not have an xfr here */
> +			WARN_ONCE(1, "no xfr in queue");
> +			sbefifo_put_client(client);
> +			return -EPROTO;
> +		}
> +
>  		if (!test_bit(SBEFIFO_XFR_COMPLETE, &xfr->flags)) {
>  			/*
>  			 * Fill the read buffer back up.
> @@ -632,7 +632,9 @@ static bool sbefifo_write_ready(struct sbefifo *sbefifo,
>  				struct sbefifo_xfr *xfr,
>  				struct sbefifo_client *client, size_t *n)
>  {
> -	struct sbefifo_xfr *next = sbefifo_client_next_xfr(client);
> +	struct sbefifo_xfr *next = list_first_entry_or_null(&client->xfrs,
> +							    struct sbefifo_xfr,
> +							    client);
>  
>  	*n = sbefifo_buf_nbwriteable(&client->wbuf);
>  	return READ_ONCE(sbefifo->rc) || (next == xfr && *n);

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH linux dev-4.10 v3 07/31] drivers: fsi: sbefifo: Use goto to reduce put statements

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 3217 bytes --]

On Thu, 2017-10-05 at 14:24 -0500, Eddie James wrote:
> From: "Edward A. James" <eajames@us.ibm.com>
> 
> Use a goto in the read() and write() functions so that we can just have
> one get_client and put_client statement.
> 
> Signed-off-by: Edward A. James <eajames@us.ibm.com>

Reviewed-by: Andrew Jeffery <andrew@aj.id.au>

> ---
>  drivers/fsi/fsi-sbefifo.c | 35 ++++++++++++++++++-----------------
>  1 file changed, 18 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/fsi/fsi-sbefifo.c b/drivers/fsi/fsi-sbefifo.c
> index 173cd03..a5bdc75 100644
> --- a/drivers/fsi/fsi-sbefifo.c
> +++ b/drivers/fsi/fsi-sbefifo.c
> @@ -565,22 +565,21 @@ static ssize_t sbefifo_read_common(struct sbefifo_client *client,
>  	if (wait_event_interruptible(sbefifo->wait,
>  				     sbefifo_read_ready(sbefifo, client, &n,
>  							&ret))) {
> -		sbefifo_put_client(client);
> -		return -ERESTARTSYS;
> +		ret = -ERESTARTSYS;
> +		goto out;
>  	}
>  
>  	if (ret) {
>  		INIT_LIST_HEAD(&client->xfrs);
> -		sbefifo_put_client(client);
> -		return ret;
> +		goto out;
>  	}
>  
>  	n = min_t(size_t, n, len);
>  
>  	if (ubuf) {
>  		if (copy_to_user(ubuf, READ_ONCE(client->rbuf.rpos), n)) {
> -			sbefifo_put_client(client);
> -			return -EFAULT;
> +			ret = -EFAULT;
> +			goto out;
>  		}
>  	} else {
>  		memcpy(kbuf, READ_ONCE(client->rbuf.rpos), n);
> @@ -592,8 +591,8 @@ static ssize_t sbefifo_read_common(struct sbefifo_client *client,
>  		if (!xfr) {
>  			/* should be impossible to not have an xfr here */
>  			WARN_ONCE(1, "no xfr in queue");
> -			sbefifo_put_client(client);
> -			return -EPROTO;
> +			ret = -EPROTO;
> +			goto out;
>  		}
>  
>  		if (!test_bit(SBEFIFO_XFR_COMPLETE, &xfr->flags)) {
> @@ -610,9 +609,11 @@ static ssize_t sbefifo_read_common(struct sbefifo_client *client,
>  		}
>  	}
>  
> -	sbefifo_put_client(client);
> +	ret = n;
>  
> -	return n;
> +out:
> +	sbefifo_put_client(client);
> +	return ret;
>  }
>  
>  static ssize_t sbefifo_read(struct file *file, char __user *buf, size_t len,
> @@ -689,14 +690,14 @@ static ssize_t sbefifo_write_common(struct sbefifo_client *client,
>  			if (mod_timer(&sbefifo->poll_timer, jiffies))
>  				sbefifo_put(sbefifo);
>  
> -			sbefifo_put_client(client);
> -			return -ERESTARTSYS;
> +			ret = -ERESTARTSYS;
> +			goto out;
>  		}
>  
>  		if (sbefifo->rc) {
>  			INIT_LIST_HEAD(&client->xfrs);
> -			sbefifo_put_client(client);
> -			return sbefifo->rc;
> +			ret = sbefifo->rc;
> +			goto out;
>  		}
>  
>  		n = min_t(size_t, n, len);
> @@ -708,8 +709,8 @@ static ssize_t sbefifo_write_common(struct sbefifo_client *client,
>  				sbefifo_get(sbefifo);
>  				if (mod_timer(&sbefifo->poll_timer, jiffies))
>  					sbefifo_put(sbefifo);
> -				sbefifo_put_client(client);
> -				return -EFAULT;
> +				ret = -EFAULT;
> +				goto out;
>  			}
>  
>  			ubuf += n;
> @@ -736,8 +737,8 @@ static ssize_t sbefifo_write_common(struct sbefifo_client *client,
>  			sbefifo_put(sbefifo);
>  	}
>  
> +out:
>  	sbefifo_put_client(client);
> -
>  	return ret;
>  }
>  

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH linux dev-4.10 v3 08/31] drivers: fsi: sbefifo: Do an earlier get_client call

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 1532 bytes --]

On Thu, 2017-10-05 at 14:24 -0500, Eddie James wrote:
> From: "Edward A. James" <eajames@us.ibm.com>
> 
> Get client before we start using it.
> 
> Signed-off-by: Edward A. James <eajames@us.ibm.com>

I still think the refounting of clients is a bit odd, but if we need to do
it this seems reasonable.

Acked-by: Andrew Jeffery <andrew@aj.id.au>

> ---
>  drivers/fsi/fsi-sbefifo.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/fsi/fsi-sbefifo.c b/drivers/fsi/fsi-sbefifo.c
> index a5bdc75..5efba30 100644
> --- a/drivers/fsi/fsi-sbefifo.c
> +++ b/drivers/fsi/fsi-sbefifo.c
> @@ -656,6 +656,7 @@ static ssize_t sbefifo_write_common(struct sbefifo_client *client,
>  	if (!len)
>  		return 0;
>  
> +	sbefifo_get_client(client);
>  	n = sbefifo_buf_nbwriteable(&client->wbuf);
>  
>  	spin_lock_irq(&sbefifo->lock);
> @@ -663,19 +664,19 @@ static ssize_t sbefifo_write_common(struct sbefifo_client *client,
>  
>  	if ((client->f_flags & O_NONBLOCK) && xfr && n < len) {
>  		spin_unlock_irq(&sbefifo->lock);
> -		return -EAGAIN;
> +		ret = -EAGAIN;
> +		goto out;
>  	}
>  
>  	xfr = sbefifo_enq_xfr(client);
>  	if (!xfr) {
>  		spin_unlock_irq(&sbefifo->lock);
> -		return -ENOMEM;
> +		ret = -ENOMEM;
> +		goto out;
>  	}
>  
>  	spin_unlock_irq(&sbefifo->lock);
>  
> -	sbefifo_get_client(client);
> -
>  	/*
>  	 * Partial writes are not really allowed in that EOT is sent exactly
>  	 * once per write.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH linux dev-4.10 v3 09/31] drivers: fsi: sbefifo: Remove warning and user data access check

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 1312 bytes --]

On Thu, 2017-10-05 at 14:24 -0500, Eddie James wrote:
> From: "Edward A. James" <eajames@us.ibm.com>
> 
> We check for user buffer access when we copy the data. No reason to
> throw a kernel warning if there is an offset provided; just ignore it.
> 
> Signed-off-by: Edward A. James <eajames@us.ibm.com>

Reviewed-by: Andrew Jeffery <andrew@aj.id.au>

> ---
>  drivers/fsi/fsi-sbefifo.c | 10 ----------
>  1 file changed, 10 deletions(-)
> 
> diff --git a/drivers/fsi/fsi-sbefifo.c b/drivers/fsi/fsi-sbefifo.c
> index 5efba30..60b133d 100644
> --- a/drivers/fsi/fsi-sbefifo.c
> +++ b/drivers/fsi/fsi-sbefifo.c
> @@ -621,11 +621,6 @@ static ssize_t sbefifo_read(struct file *file, char __user *buf, size_t len,
>  {
>  	struct sbefifo_client *client = file->private_data;
>  
> -	WARN_ON(*offset);
> -
> -	if (!access_ok(VERIFY_WRITE, buf, len))
> -		return -EFAULT;
> -
>  	return sbefifo_read_common(client, buf, NULL, len);
>  }
>  
> @@ -748,11 +743,6 @@ static ssize_t sbefifo_write(struct file *file, const char __user *buf,
>  {
>  	struct sbefifo_client *client = file->private_data;
>  
> -	WARN_ON(*offset);
> -
> -	if (!access_ok(VERIFY_READ, buf, len))
> -		return -EFAULT;
> -
>  	return sbefifo_write_common(client, buf, NULL, len);
>  }
>  

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH linux dev-4.10 v3 10/31] drivers: fsi: sbefifo: destroy the ida list on exit

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 721 bytes --]

On Thu, 2017-10-05 at 14:24 -0500, Eddie James wrote:
> From: "Edward A. James" <eajames@us.ibm.com>
> 
> Need to clean this up.
> 
> Signed-off-by: Edward A. James <eajames@us.ibm.com>

Reviewed-by: Andrew Jeffery <andrew@aj.id.au>

> ---
>  drivers/fsi/fsi-sbefifo.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/fsi/fsi-sbefifo.c b/drivers/fsi/fsi-sbefifo.c
> index 60b133d..0ff5209 100644
> --- a/drivers/fsi/fsi-sbefifo.c
> +++ b/drivers/fsi/fsi-sbefifo.c
> @@ -947,6 +947,8 @@ static int sbefifo_init(void)
>  static void sbefifo_exit(void)
>  {
>  	fsi_driver_unregister(&sbefifo_drv);
> +
> +	ida_destroy(&sbefifo_ida);
>  }
>  
>  module_init(sbefifo_init);

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH linux dev-4.10 v3 11/31] drivers: fsi: sbefifo: Fix module authors and comments

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 2130 bytes --]

On Thu, 2017-10-05 at 14:24 -0500, Eddie James wrote:
> From: "Edward A. James" <eajames@us.ibm.com>
> 
> Clarify the xfr situation in write() and fix module authors
> 
> Signed-off-by: Edward A. James <eajames@us.ibm.com>

Reviewed-by: Andrew Jeffery <andrew@aj.id.au>

> ---
>  drivers/fsi/fsi-sbefifo.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/fsi/fsi-sbefifo.c b/drivers/fsi/fsi-sbefifo.c
> index 0ff5209..a2acf76 100644
> --- a/drivers/fsi/fsi-sbefifo.c
> +++ b/drivers/fsi/fsi-sbefifo.c
> @@ -655,7 +655,7 @@ static ssize_t sbefifo_write_common(struct sbefifo_client *client,
>  	n = sbefifo_buf_nbwriteable(&client->wbuf);
>  
>  	spin_lock_irq(&sbefifo->lock);
> -	xfr = sbefifo_next_xfr(sbefifo);
> +	xfr = sbefifo_next_xfr(sbefifo);	/* next xfr to be executed */
>  
>  	if ((client->f_flags & O_NONBLOCK) && xfr && n < len) {
>  		spin_unlock_irq(&sbefifo->lock);
> @@ -663,7 +663,7 @@ static ssize_t sbefifo_write_common(struct sbefifo_client *client,
>  		goto out;
>  	}
>  
> -	xfr = sbefifo_enq_xfr(client);
> +	xfr = sbefifo_enq_xfr(client);		/* this xfr queued up */
>  	if (!xfr) {
>  		spin_unlock_irq(&sbefifo->lock);
>  		ret = -ENOMEM;
> @@ -719,8 +719,9 @@ static ssize_t sbefifo_write_common(struct sbefifo_client *client,
>  		len -= n;
>  		ret += n;
>  
> -		/* set flag before starting the worker, as it may run through
> -		 * and check the flag before we exit this loop!
> +		/*
> +		 * Set this before starting timer to avoid race condition on
> +		 * this flag with the timer function writer.
>  		 */
>  		if (!len)
>  			set_bit(SBEFIFO_XFR_WRITE_DONE, &xfr->flags);
> @@ -955,4 +956,5 @@ static void sbefifo_exit(void)
>  module_exit(sbefifo_exit);
>  MODULE_LICENSE("GPL");
>  MODULE_AUTHOR("Brad Bishop <bradleyb@fuzziesquirrel.com>");
> -MODULE_DESCRIPTION("Linux device interface to the POWER self boot engine");
> +MODULE_AUTHOR("Eddie James <eajames@linux.vnet.ibm.com>");
> +MODULE_DESCRIPTION("Linux device interface to the POWER Self Boot Engine");

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH linux dev-4.10 v3 12/31] drivers: fsi: sbefifo: Fix include guards in header file

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 1098 bytes --]

On Thu, 2017-10-05 at 14:24 -0500, Eddie James wrote:
> From: "Edward A. James" <eajames@us.ibm.com>
> 
> Make them conform to Linux standard headers.
> 
> Signed-off-by: Edward A. James <eajames@us.ibm.com>

Reviewed-by: Andrew Jeffery <andrew@aj.id.au>

> ---
>  include/linux/fsi-sbefifo.h | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/fsi-sbefifo.h b/include/linux/fsi-sbefifo.h
> index 1b46c63..8e55891 100644
> --- a/include/linux/fsi-sbefifo.h
> +++ b/include/linux/fsi-sbefifo.h
> @@ -13,8 +13,8 @@
>   * GNU General Public License for more details.
>   */
>  
> -#ifndef __FSI_SBEFIFO_H__
> -#define __FSI_SBEFIFO_H__
> +#ifndef LINUX_FSI_SBEFIFO_H
> +#define LINUX_FSI_SBEFIFO_H
>  
>  struct device;
>  struct sbefifo_client;
> @@ -27,4 +27,4 @@ extern int sbefifo_drv_write(struct sbefifo_client *client, const char *buf,
>  			     size_t len);
>  extern void sbefifo_drv_release(struct sbefifo_client *client);
>  
> -#endif /* __FSI_SBEFIFO_H__ */
> +#endif /* LINUX_FSI_SBEFIFO_H */

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH linux dev-4.10 v3 13/31] drivers: fsi: SBEFIFO: Fix probe() and remove()

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 7119 bytes --]

On Thu, 2017-10-05 at 14:24 -0500, Eddie James wrote:
> From: "Edward A. James" <eajames@us.ibm.com>
> 
> Probe didn't handle a failed misc device registration properly. Remove
> had the potential for hangs. Also, remove the list of SBEFIFOs and
> instead just use the device "driver data" pointer.
> 
> Signed-off-by: Edward A. James <eajames@us.ibm.com>

Reviewed-by: Andrew Jeffery <andrew@aj.id.au>

> ---
>  drivers/fsi/fsi-sbefifo.c | 111 ++++++++++++++++++++++------------------------
>  1 file changed, 53 insertions(+), 58 deletions(-)
> 
> diff --git a/drivers/fsi/fsi-sbefifo.c b/drivers/fsi/fsi-sbefifo.c
> index a2acf76..9b560ec 100644
> --- a/drivers/fsi/fsi-sbefifo.c
> +++ b/drivers/fsi/fsi-sbefifo.c
> @@ -58,7 +58,6 @@ struct sbefifo {
>  	struct fsi_device *fsi_dev;
>  	struct miscdevice mdev;
>  	wait_queue_head_t wait;
> -	struct list_head link;
>  	struct list_head xfrs;
>  	struct kref kref;
>  	spinlock_t lock;
> @@ -96,8 +95,6 @@ struct sbefifo_client {
>  	unsigned long f_flags;
>  };
>  
> -static struct list_head sbefifo_fifos;
> -
>  static DEFINE_IDA(sbefifo_ida);
>  
>  static int sbefifo_inw(struct sbefifo *sbefifo, int reg, u32 *word)
> @@ -267,9 +264,12 @@ static struct sbefifo_xfr *sbefifo_enq_xfr(struct sbefifo_client *client)
>  	struct sbefifo *sbefifo = client->dev;
>  	struct sbefifo_xfr *xfr;
>  
> +	if (READ_ONCE(sbefifo->rc))
> +		return ERR_PTR(sbefifo->rc);
> +
>  	xfr = kzalloc(sizeof(*xfr), GFP_KERNEL);
>  	if (!xfr)
> -		return NULL;
> +		return ERR_PTR(-ENOMEM);
>  
>  	xfr->rbuf = &client->rbuf;
>  	xfr->wbuf = &client->wbuf;
> @@ -490,7 +490,7 @@ static void sbefifo_poll_timer(unsigned long data)
>  	}
>  
>  	sbefifo_put(sbefifo);
> -	wake_up(&sbefifo->wait);
> +	wake_up_interruptible(&sbefifo->wait);
>  
>  out_unlock:
>  	spin_unlock(&sbefifo->lock);
> @@ -605,7 +605,7 @@ static ssize_t sbefifo_read_common(struct sbefifo_client *client,
>  		} else {
>  			kfree(xfr);
>  			list_del(&xfr->client);
> -			wake_up(&sbefifo->wait);
> +			wake_up_interruptible(&sbefifo->wait);
>  		}
>  	}
>  
> @@ -664,9 +664,9 @@ static ssize_t sbefifo_write_common(struct sbefifo_client *client,
>  	}
>  
>  	xfr = sbefifo_enq_xfr(client);		/* this xfr queued up */
> -	if (!xfr) {
> +	if (IS_ERR(xfr)) {
>  		spin_unlock_irq(&sbefifo->lock);
> -		ret = -ENOMEM;
> +		ret = PTR_ERR(xfr);
>  		goto out;
>  	}
>  
> @@ -769,18 +769,15 @@ static int sbefifo_release(struct inode *inode, struct file *file)
>  struct sbefifo_client *sbefifo_drv_open(struct device *dev,
>  					unsigned long flags)
>  {
> -	struct sbefifo_client *client = NULL;
> -	struct sbefifo *sbefifo;
> -	struct fsi_device *fsi_dev = to_fsi_dev(dev);
> +	struct sbefifo_client *client;
> +	struct sbefifo *sbefifo = dev_get_drvdata(dev);
>  
> -	list_for_each_entry(sbefifo, &sbefifo_fifos, link) {
> -		if (sbefifo->fsi_dev != fsi_dev)
> -			continue;
> +	if (!sbefifo)
> +		return NULL;
>  
> -		client = sbefifo_new_client(sbefifo);
> -		if (client)
> -			client->f_flags = flags;
> -	}
> +	client = sbefifo_new_client(sbefifo);
> +	if (client)
> +		client->f_flags = flags;
>  
>  	return client;
>  }
> @@ -854,69 +851,68 @@ static int sbefifo_probe(struct device *dev)
>  		return -EIO;
>  	}
>  
> -	sbefifo->mdev.minor = MISC_DYNAMIC_MINOR;
> -	sbefifo->mdev.fops = &sbefifo_fops;
> -	sbefifo->mdev.name = sbefifo->name;
> -	sbefifo->mdev.parent = dev;
>  	spin_lock_init(&sbefifo->lock);
>  	kref_init(&sbefifo->kref);
> +	init_waitqueue_head(&sbefifo->wait);
> +	INIT_LIST_HEAD(&sbefifo->xfrs);
>  
>  	sbefifo->idx = ida_simple_get(&sbefifo_ida, 1, INT_MAX, GFP_KERNEL);
>  	snprintf(sbefifo->name, sizeof(sbefifo->name), "sbefifo%d",
>  		 sbefifo->idx);
> -	init_waitqueue_head(&sbefifo->wait);
> -	INIT_LIST_HEAD(&sbefifo->xfrs);
>  
>  	/* This bit of silicon doesn't offer any interrupts... */
>  	setup_timer(&sbefifo->poll_timer, sbefifo_poll_timer,
>  		    (unsigned long)sbefifo);
>  
> -	if (dev->of_node) {
> -		/* create platform devs for dts child nodes (occ, etc) */
> -		for_each_child_of_node(dev->of_node, np) {
> -			snprintf(child_name, sizeof(child_name), "%s-dev%d",
> -				 sbefifo->name, child_idx++);
> -			child = of_platform_device_create(np, child_name, dev);
> -			if (!child)
> -				dev_warn(dev,
> -					 "failed to create child node dev\n");
> -		}
> +	sbefifo->mdev.minor = MISC_DYNAMIC_MINOR;
> +	sbefifo->mdev.fops = &sbefifo_fops;
> +	sbefifo->mdev.name = sbefifo->name;
> +	sbefifo->mdev.parent = dev;
> +	ret = misc_register(&sbefifo->mdev);
> +	if (ret) {
> +		dev_err(dev, "failed to register miscdevice: %d\n", ret);
> +		ida_simple_remove(&sbefifo_ida, sbefifo->idx);
> +		sbefifo_put(sbefifo);
> +		return ret;
>  	}
>  
> -	list_add(&sbefifo->link, &sbefifo_fifos);
> -	
> -	return misc_register(&sbefifo->mdev);
> +	/* create platform devs for dts child nodes (occ, etc) */
> +	for_each_available_child_of_node(dev->of_node, np) {
> +		snprintf(child_name, sizeof(child_name), "%s-dev%d",
> +			 sbefifo->name, child_idx++);
> +		child = of_platform_device_create(np, child_name, dev);
> +		if (!child)
> +			dev_warn(dev, "failed to create child %s dev\n",
> +				 child_name);
> +	}
> +
> +	dev_set_drvdata(dev, sbefifo);
> +
> +	return 0;
>  }
>  
>  static int sbefifo_remove(struct device *dev)
>  {
> -	struct fsi_device *fsi_dev = to_fsi_dev(dev);
> -	struct sbefifo *sbefifo, *sbefifo_tmp;
> +	struct sbefifo *sbefifo = dev_get_drvdata(dev);
>  	struct sbefifo_xfr *xfr;
>  
> -	list_for_each_entry_safe(sbefifo, sbefifo_tmp, &sbefifo_fifos, link) {
> -		if (sbefifo->fsi_dev != fsi_dev)
> -			continue;
> +	WRITE_ONCE(sbefifo->rc, -ENODEV);
> +	wake_up_all(&sbefifo->wait);
>  
> -		device_for_each_child(dev, NULL, sbefifo_unregister_child);
> +	misc_deregister(&sbefifo->mdev);
> +	device_for_each_child(dev, NULL, sbefifo_unregister_child);
>  
> -		misc_deregister(&sbefifo->mdev);
> -		list_del(&sbefifo->link);
> -		ida_simple_remove(&sbefifo_ida, sbefifo->idx);
> -
> -		if (del_timer_sync(&sbefifo->poll_timer))
> -			sbefifo_put(sbefifo);
> +	ida_simple_remove(&sbefifo_ida, sbefifo->idx);
>  
> -		spin_lock(&sbefifo->lock);
> -		list_for_each_entry(xfr, &sbefifo->xfrs, xfrs)
> -			kfree(xfr);
> -		spin_unlock(&sbefifo->lock);
> +	if (del_timer_sync(&sbefifo->poll_timer))
> +		sbefifo_put(sbefifo);
>  
> -		WRITE_ONCE(sbefifo->rc, -ENODEV);
> +	spin_lock(&sbefifo->lock);
> +	list_for_each_entry(xfr, &sbefifo->xfrs, xfrs)
> +		kfree(xfr);
> +	spin_unlock(&sbefifo->lock);
>  
> -		wake_up(&sbefifo->wait);
> -		sbefifo_put(sbefifo);
> -	}
> +	sbefifo_put(sbefifo);
>  
>  	return 0;
>  }
> @@ -941,7 +937,6 @@ static int sbefifo_remove(struct device *dev)
>  
>  static int sbefifo_init(void)
>  {
> -	INIT_LIST_HEAD(&sbefifo_fifos);
>  	return fsi_driver_register(&sbefifo_drv);
>  }
>  

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH linux dev-4.10 v3 14/31] drivers: fsi: SBEFIFO: check for xfr complete in read wait_event

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 1274 bytes --]

On Thu, 2017-10-05 at 14:24 -0500, Eddie James wrote:
> From: "Edward A. James" <eajames@us.ibm.com>
> 
> We should check to see if the XFR is complete, not just for a failure
> or for available data. If we hit EOT without getting more data, we may
> wait forever.
> 
> Signed-off-by: Edward A. James <eajames@us.ibm.com>

Reviewed-by: Andrew Jeffery <andrew@aj.id.au>

> ---
>  drivers/fsi/fsi-sbefifo.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/fsi/fsi-sbefifo.c b/drivers/fsi/fsi-sbefifo.c
> index 9b560ec..5d73437 100644
> --- a/drivers/fsi/fsi-sbefifo.c
> +++ b/drivers/fsi/fsi-sbefifo.c
> @@ -541,10 +541,15 @@ static bool sbefifo_read_ready(struct sbefifo *sbefifo,
>  			       struct sbefifo_client *client, size_t *n,
>  			       size_t *ret)
>  {
> +	struct sbefifo_xfr *xfr = list_first_entry_or_null(&client->xfrs,
> +							   struct sbefifo_xfr,
> +							   client);
> +
>  	*n = sbefifo_buf_nbreadable(&client->rbuf);
>  	*ret = READ_ONCE(sbefifo->rc);
>  
> -	return *ret || *n;
> +	return *ret || *n ||
> +		(xfr && test_bit(SBEFIFO_XFR_COMPLETE, &xfr->flags));
>  }
>  
>  static ssize_t sbefifo_read_common(struct sbefifo_client *client,

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH linux dev-4.10 v3 15/31] drivers: fsi: occ: Fix includes

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 1225 bytes --]

On Thu, 2017-10-05 at 14:24 -0500, Eddie James wrote:
> From: "Edward A. James" <eajames@us.ibm.com>
> 
> Need to include everything we use and not rely on recursive includes.
> 
> Signed-off-by: Edward A. James <eajames@us.ibm.com>

Acked-by: Andrew Jeffery <andrew@aj.id.au>

> ---
>  drivers/fsi/occ.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/fsi/occ.c b/drivers/fsi/occ.c
> index 621fbf0..2d7df8a 100644
> --- a/drivers/fsi/occ.c
> +++ b/drivers/fsi/occ.c
> @@ -10,16 +10,21 @@
>  #include <asm/unaligned.h>
>  #include <linux/device.h>
>  #include <linux/err.h>
> +#include <linux/errno.h>
> +#include <linux/fs.h>
>  #include <linux/fsi-sbefifo.h>
> -#include <linux/init.h>
> +#include <linux/idr.h>
>  #include <linux/kernel.h>
> +#include <linux/list.h>
>  #include <linux/miscdevice.h>
>  #include <linux/module.h>
>  #include <linux/mutex.h>
> +#include <linux/occ.h>
>  #include <linux/of.h>
>  #include <linux/of_platform.h>
>  #include <linux/platform_device.h>
>  #include <linux/slab.h>
> +#include <linux/spinlock.h>
>  #include <linux/uaccess.h>
>  #include <linux/wait.h>
>  #include <linux/workqueue.h>

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH linux dev-4.10 v3 16/31] drivers: fsi: occ: Fix errant kfree calls

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 1039 bytes --]

On Thu, 2017-10-05 at 14:24 -0500, Eddie James wrote:
> From: "Edward A. James" <eajames@us.ibm.com>
> 
> Left over from when xfr was an independant structure. Remove these
calls
> as xfr is a part of the client struct.
> 
> Signed-off-by: Edward A. James <eajames@us.ibm.com>

Reviewed-by: Andrew Jeffery <andrew@aj.id.au>

> ---
>  drivers/fsi/occ.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/fsi/occ.c b/drivers/fsi/occ.c
> index 2d7df8a..cd60c5a 100644
> --- a/drivers/fsi/occ.c
> +++ b/drivers/fsi/occ.c
> @@ -289,7 +289,6 @@ static ssize_t occ_write_common(struct occ_client
*client,
>  
>  	if (ubuf) {
>  		if (copy_from_user(&xfr->buf[1], ubuf, len)) {
> -			kfree(xfr);
>  			rc = -EFAULT;
>  			goto done;
>  		}
> @@ -298,7 +297,6 @@ static ssize_t occ_write_common(struct occ_client
*client,
>  
>  	data_length = (xfr->buf[2] << 8) + xfr->buf[3];
>  	if (data_length > OCC_CMD_DATA_BYTES) {
> -		kfree(xfr);
>  		rc = -EINVAL;
>  		goto done;
>  	}

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH linux dev-4.10 v3 17/31] drivers: fsi: occ: remove unused occ_command structure

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 761 bytes --]

On Thu, 2017-10-05 at 14:24 -0500, Eddie James wrote:
> From: "Edward A. James" <eajames@us.ibm.com>
> 
> Signed-off-by: Edward A. James <eajames@us.ibm.com>

Reviewed-by: Andrew Jeffery <andrew@aj.id.au>

> ---
>  drivers/fsi/occ.c | 8 --------
>  1 file changed, 8 deletions(-)
> 
> diff --git a/drivers/fsi/occ.c b/drivers/fsi/occ.c
> index cd60c5a..82a60c5 100644
> --- a/drivers/fsi/occ.c
> +++ b/drivers/fsi/occ.c
> @@ -46,14 +46,6 @@ struct occ {
>  
>  #define to_occ(x)	container_of((x), struct occ, mdev)
>  
> -struct occ_command {
> -	u8 seq_no;
> -	u8 cmd_type;
> -	u16 data_length;
> -	u8 data[OCC_CMD_DATA_BYTES];
> -	u16 checksum;
> -} __packed;
> -
>  struct occ_response {
>  	u8 seq_no;
>  	u8 cmd_type;

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH linux dev-4.10 v3 18/31] drivers: fsi: occ: Use big-endian values

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 2642 bytes --]

On Thu, 2017-10-05 at 14:24 -0500, Eddie James wrote:
> From: "Edward A. James" <eajames@us.ibm.com>
> 
> Switch to __be16 from u16 and __be32 from u32. Also, use kernel access
> of unaligned be16 instead of manually creating a big-endian value from
> the occ response buffer.
> 
> Signed-off-by: Edward A. James <eajames@us.ibm.com>
> ---
>  drivers/fsi/occ.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/fsi/occ.c b/drivers/fsi/occ.c
> index 82a60c5..55f293d 100644
> --- a/drivers/fsi/occ.c
> +++ b/drivers/fsi/occ.c
> @@ -50,9 +50,9 @@ struct occ_response {
>  	u8 seq_no;
>  	u8 cmd_type;
>  	u8 return_status;
> -	u16 data_length;
> +	__be16 data_length;
>  	u8 data[OCC_RESP_DATA_BYTES];
> -	u16 checksum;
> +	__be16 checksum;
>  } __packed;
>  
>  /*
> @@ -425,7 +425,7 @@ static int occ_getsram(struct device *sbefifo, u32 address, u8 *data,
>  {
>  	int rc;
>  	u8 *resp;
> -	u32 buf[5];
> +	__be32 buf[5];
>  	u32 data_len = ((len + 7) / 8) * 8;
>  	struct sbefifo_client *client;
>  
> @@ -476,7 +476,7 @@ static int occ_putsram(struct device *sbefifo, u32 address, u8 *data,
>  		       ssize_t len)
>  {
>  	int rc;
> -	u32 *buf;
> +	__be32 *buf;
>  	u32 data_len = ((len + 7) / 8) * 8;
>  	size_t cmd_len = data_len + 20;
>  	struct sbefifo_client *client;
> @@ -522,7 +522,7 @@ static int occ_putsram(struct device *sbefifo, u32 address, u8 *data,
>  static int occ_trigger_attn(struct device *sbefifo)
>  {
>  	int rc;
> -	u32 buf[6];
> +	__be32 buf[6];
>  	struct sbefifo_client *client;
>  
>  	buf[0] = cpu_to_be32(0x6);
> @@ -560,6 +560,7 @@ static void occ_worker(struct work_struct *work)
>  	int rc = 0, empty, waiting, canceled;
>  	u16 resp_data_length;
>  	struct occ_xfr *xfr;
> +	struct occ_response *resp;
>  	struct occ_client *client;
>  	struct occ *occ = container_of(work, struct occ, work);
>  	struct device *sbefifo = occ->sbefifo;
> @@ -573,6 +574,7 @@ static void occ_worker(struct work_struct *work)
>  		return;
>  	}
>  
> +	resp = (struct occ_response *)xfr->buf;
>  	set_bit(XFR_IN_PROGRESS, &xfr->flags);
>  
>  	spin_unlock_irq(&occ->list_lock);
> @@ -591,7 +593,7 @@ static void occ_worker(struct work_struct *work)
>  	if (rc)
>  		goto done;
>  
> -	resp_data_length = (xfr->buf[3] << 8) + xfr->buf[4];
> +	resp_data_length = get_unaligned_be16(&resp->data_length);
>  	if (resp_data_length > OCC_RESP_DATA_BYTES) {

You need to do be16_to_cpu() before any comparisons.

>  		rc = -EDOM;
>  		goto done;

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH linux dev-4.10 v3 19/31] drivers: fsi: occ: Return ENODEV if client is NULL

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 1287 bytes --]

On Thu, 2017-10-05 at 14:24 -0500, Eddie James wrote:
> From: "Edward A. James" <eajames@us.ibm.com>
> 
> Safeguard the in-kernel api functions.

Why just the in-kernel API functions? Why not do the check in
*{read,write,release}_common()?

> 
> Signed-off-by: Edward A. James <eajames@us.ibm.com>
> ---
>  drivers/fsi/occ.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/fsi/occ.c b/drivers/fsi/occ.c
> index 55f293d..a551349 100644
> --- a/drivers/fsi/occ.c
> +++ b/drivers/fsi/occ.c
> @@ -654,18 +654,27 @@ struct occ_client *occ_drv_open(struct device
*dev, unsigned long flags)
>  
>  int occ_drv_read(struct occ_client *client, char *buf, size_t len)
>  {
> +	if (!client)
> +		return -ENODEV;
> +
>  	return occ_read_common(client, NULL, buf, len);
>  }
>  EXPORT_SYMBOL_GPL(occ_drv_read);
>  
>  int occ_drv_write(struct occ_client *client, const char *buf, size_t
len)
>  {
> +	if (!client)
> +		return -ENODEV;
> +
>  	return occ_write_common(client, NULL, buf, len);
>  }
>  EXPORT_SYMBOL_GPL(occ_drv_write);
>  
>  void occ_drv_release(struct occ_client *client)
>  {
> +	if (!client)
> +		return;
> +
>  	occ_release_common(client);
>  }
>  EXPORT_SYMBOL_GPL(occ_drv_release);

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH linux dev-4.10 v3 20/31] drivers: fsi: occ: Remove early user buffer checking

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 1333 bytes --]

On Thu, 2017-10-05 at 14:24 -0500, Eddie James wrote:
> From: "Edward A. James" <eajames@us.ibm.com>
> 
> We check user buffer access when we copy the data in read()/write()
> anyway.
> 
> Signed-off-by: Edward A. James <eajames@us.ibm.com>

Reviewed-by: Andrew Jeffery <andrew@aj.id.au>

> ---
>  drivers/fsi/occ.c | 12 ------------
>  1 file changed, 12 deletions(-)
> 
> diff --git a/drivers/fsi/occ.c b/drivers/fsi/occ.c
> index a551349..7ced9b6 100644
> --- a/drivers/fsi/occ.c
> +++ b/drivers/fsi/occ.c
> @@ -247,12 +247,6 @@ static ssize_t occ_read(struct file *file, char __user *buf, size_t len,
>  {
>  	struct occ_client *client = file->private_data;
>  
> -	/* check this ahead of time so we don't go changing the xfr state
> -	 * needlessly
> -	 */
> -	if (!access_ok(VERIFY_WRITE, buf, len))
> -		return -EFAULT;
> -
>  	return occ_read_common(client, buf, NULL, len);
>  }
>  
> @@ -316,12 +310,6 @@ static ssize_t occ_write(struct file *file, const char __user *buf,
>  {
>  	struct occ_client *client = file->private_data;
>  
> -	/* check this ahead of time so we don't go changing the xfr state
> -	 * needlessly
> -	 */
> -	if (!access_ok(VERIFY_READ, buf, len))
> -		return -EFAULT;
> -
>  	return occ_write_common(client, buf, NULL, len);
>  }
>  

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH linux dev-4.10 v3 21/31] drivers: fsi: occ: Switch to more logical errnos

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 2269 bytes --]

On Thu, 2017-10-05 at 14:24 -0500, Eddie James wrote:
> From: "Edward A. James" <eajames@us.ibm.com>
> 
> Signed-off-by: Edward A. James <eajames@us.ibm.com>

Acked-by: Andrew Jeffery <andrew@aj.id.au>

> ---
>  drivers/fsi/occ.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/fsi/occ.c b/drivers/fsi/occ.c
> index 7ced9b6..4e677c6 100644
> --- a/drivers/fsi/occ.c
> +++ b/drivers/fsi/occ.c
> @@ -386,7 +386,7 @@ static int occ_write_sbefifo(struct
sbefifo_client *client, const char *buf,
>  		total += rc;
>  	} while (total < len);
>  
> -	return (total == len) ? 0 : -EMSGSIZE;
> +	return (total == len) ? 0 : -ENOSPC;
>  }
>  
>  static int occ_read_sbefifo(struct sbefifo_client *client, char
*buf,
> @@ -405,7 +405,7 @@ static int occ_read_sbefifo(struct sbefifo_client
*client, char *buf,
>  		total += rc;
>  	} while (total < len);
>  
> -	return (total == len) ? 0 : -EMSGSIZE;
> +	return (total == len) ? 0 : -ENODATA;
>  }
>  
>  static int occ_getsram(struct device *sbefifo, u32 address, u8
*data,
> @@ -450,7 +450,7 @@ static int occ_getsram(struct device *sbefifo,
u32 address, u8 *data,
>  	    (be32_to_cpu(buf[1]) == 0xC0DEA403))
>  		memcpy(data, resp, len);
>  	else
> -		rc = -EFAULT;
> +		rc = -EBADMSG;
>  
>  free:
>  	kfree(resp);
> @@ -498,7 +498,7 @@ static int occ_putsram(struct device *sbefifo,
u32 address, u8 *data,
>  	/* check for good response */
>  	if ((be32_to_cpu(buf[0]) != data_len) ||
>  	    (be32_to_cpu(buf[1]) != 0xC0DEA404))
> -		rc = -EFAULT;
> +		rc = -EBADMSG;
>  
>  done:
>  	sbefifo_drv_release(client);
> @@ -535,7 +535,7 @@ static int occ_trigger_attn(struct device
*sbefifo)
>  	/* check for good response */
>  	if ((be32_to_cpu(buf[0]) != 0xC0DEA202) ||
>  	    (be32_to_cpu(buf[1]) & 0x0FFFFFFF))
> -		rc = -EFAULT;
> +		rc = -EBADMSG;
>  
>  done:
>  	sbefifo_drv_release(client);
> @@ -583,7 +583,7 @@ static void occ_worker(struct work_struct *work)
>  
>  	resp_data_length = get_unaligned_be16(&resp->data_length);
>  	if (resp_data_length > OCC_RESP_DATA_BYTES) {
> -		rc = -EDOM;
> +		rc = -EMSGSIZE;
>  		goto done;
>  	}
>  

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH linux dev-4.10 v3 22/31] drivers: fsi: occ: fix white space and bracket problems

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 3326 bytes --]

On Thu, 2017-10-05 at 14:24 -0500, Eddie James wrote:
> From: "Edward A. James" <eajames@us.ibm.com>
> 
> For better flow and to meet Linux standards.
> 
> Signed-off-by: Edward A. James <eajames@us.ibm.com>
> ---
>  drivers/fsi/occ.c | 25 +++++++++++++++----------
>  1 file changed, 15 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/fsi/occ.c b/drivers/fsi/occ.c
> index 4e677c6..f27345c 100644
> --- a/drivers/fsi/occ.c
> +++ b/drivers/fsi/occ.c
> @@ -132,9 +132,8 @@ static void occ_enqueue_xfr(struct occ_xfr *xfr)
>  
>  static struct occ_client *occ_open_common(struct occ *occ, unsigned long flags)
>  {
> -	struct occ_client *client;
> +	struct occ_client *client = kzalloc(sizeof(*client), GFP_KERNEL);
>  
> -	client = kzalloc(sizeof(*client), GFP_KERNEL);
>  	if (!client)
>  		return NULL;
>  
> @@ -180,8 +179,9 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
>  		if (client->read_offset) {
>  			rc = 0;
>  			client->read_offset = 0;
> -		} else
> +		} else {
>  			rc = -ENOMSG;
> +		}
>  
>  		goto done;
>  	}
> @@ -197,8 +197,10 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
>  		spin_unlock_irq(&client->lock);
>  
>  		rc = wait_event_interruptible(client->wait,
> -			test_bit(XFR_COMPLETE, &xfr->flags) ||
> -			test_bit(XFR_CANCELED, &xfr->flags));
> +					      test_bit(XFR_COMPLETE,
> +						       &xfr->flags) ||
> +					      test_bit(XFR_CANCELED,
> +						       &xfr->flags));

This looks worse. Maybe it's worth pulling the condition out into a helper
function?

>  
>  		spin_lock_irq(&client->lock);
>  
> @@ -222,12 +224,14 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
>  
>  	bytes = min(len, xfr->resp_data_length - client->read_offset);
>  	if (ubuf) {
> -		if (copy_to_user(ubuf, &xfr->buf[client->read_offset], bytes)) {
> +		if (copy_to_user(ubuf, &xfr->buf[client->read_offset],
> +				 bytes)) {
>  			rc = -EFAULT;
>  			goto done;
>  		}
> -	} else
> +	} else {
>  		memcpy(kbuf, &xfr->buf[client->read_offset], bytes);
> +	}
>  
>  	client->read_offset += bytes;
>  
> @@ -263,6 +267,7 @@ static ssize_t occ_write_common(struct occ_client *client,
>  		return -EINVAL;
>  
>  	spin_lock_irq(&client->lock);
> +
>  	if (test_and_set_bit(CLIENT_XFR_PENDING, &client->flags)) {
>  		rc = -EBUSY;
>  		goto done;
> @@ -270,7 +275,6 @@ static ssize_t occ_write_common(struct occ_client *client,
>  
>  	/* clear out the transfer */
>  	memset(xfr, 0, sizeof(*xfr));
> -
>  	xfr->buf[0] = 1;
>  
>  	if (ubuf) {
> @@ -278,8 +282,9 @@ static ssize_t occ_write_common(struct occ_client *client,
>  			rc = -EFAULT;
>  			goto done;
>  		}
> -	} else
> +	} else {
>  		memcpy(&xfr->buf[1], kbuf, len);
> +	}
>  
>  	data_length = (xfr->buf[2] << 8) + xfr->buf[3];
>  	if (data_length > OCC_CMD_DATA_BYTES) {
> @@ -430,7 +435,7 @@ static int occ_getsram(struct device *sbefifo, u32 address, u8 *data,
>  	rc = occ_write_sbefifo(client, (const char *)buf, sizeof(buf));
>  	if (rc)
>  		goto done;
> -	
> +
>  	resp = kzalloc(data_len, GFP_KERNEL);
>  	if (!resp) {
>  		rc = -ENOMEM;

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH linux dev-4.10 v3 23/31] drivers: fsi: occ: Destroy the ida list on exit

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 693 bytes --]

On Thu, 2017-10-05 at 14:24 -0500, Eddie James wrote:
> From: "Edward A. James" <eajames@us.ibm.com>
> 
> Need to clean up the index list structure.
> 
> Signed-off-by: Edward A. James <eajames@us.ibm.com>

Reviewed-by: Andrew Jeffery <andrew@aj.id.au>

> ---
>  drivers/fsi/occ.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/fsi/occ.c b/drivers/fsi/occ.c
> index f27345c..916c342 100644
> --- a/drivers/fsi/occ.c
> +++ b/drivers/fsi/occ.c
> @@ -788,6 +788,8 @@ static void occ_exit(void)
>  	destroy_workqueue(occ_wq);
>  
>  	platform_driver_unregister(&occ_driver);
> +
> +	ida_destroy(&occ_ida);
>  }
>  
>  module_init(occ_init);

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH linux dev-4.10 v3 24/31] drivers: fsi: occ: Remove unnecessary platform_set_drvdata call

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 973 bytes --]

On Thu, 2017-10-05 at 14:24 -0500, Eddie James wrote:
> From: "Edward A. James" <eajames@us.ibm.com>
> 
> The purpose was to set that pointer null before probing. However, it is
> set null by the kernel by default.
> 
> Signed-off-by: Edward A. James <eajames@us.ibm.com>
> ---
>  drivers/fsi/occ.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/fsi/occ.c b/drivers/fsi/occ.c
> index 916c342..561b029 100644
> --- a/drivers/fsi/occ.c
> +++ b/drivers/fsi/occ.c
> @@ -703,9 +703,6 @@ static int occ_probe(struct platform_device *pdev)
>  	mutex_init(&occ->occ_lock);
>  	INIT_WORK(&occ->work, occ_worker);
>  
> -	/* ensure NULL before we probe children, so they don't hang FSI */
> -	platform_set_drvdata(pdev, NULL);
> -

Bizarre that it was even there in the first place - do you know the history of
the comment?

>  	if (dev->of_node) {
>  		rc = of_property_read_u32(dev->of_node, "reg", &reg);
>  		if (!rc) {

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH linux dev-4.10 v3 25/31] drivers: fsi: occ: Add comments for clarity

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 5960 bytes --]

On Thu, 2017-10-05 at 14:24 -0500, Eddie James wrote:
> From: "Edward A. James" <eajames@us.ibm.com>
> 
> Fixup some existing comments as well.
> 
> Signed-off-by: Edward A. James <eajames@us.ibm.com>

Reviewed-by: Andrew Jeffery <andrew@aj.id.au>

> ---
>  drivers/fsi/occ.c | 55 +++++++++++++++++++++++++++++++++++++------
------------
>  1 file changed, 37 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/fsi/occ.c b/drivers/fsi/occ.c
> index 561b029..29a17ed 100644
> --- a/drivers/fsi/occ.c
> +++ b/drivers/fsi/occ.c
> @@ -39,8 +39,8 @@ struct occ {
>  	int idx;
>  	struct miscdevice mdev;
>  	struct list_head xfrs;
> -	spinlock_t list_lock;
> -	struct mutex occ_lock;
> +	spinlock_t list_lock;		/* lock access to the
xfrs list */
> +	struct mutex occ_lock;		/* lock access to the
hardware */
>  	struct work_struct work;
>  };
>  
> @@ -57,17 +57,17 @@ struct occ_response {
>  
>  /*
>   * transfer flags are NOT mutually exclusive
> - * 
> + *
>   * Initial flags are none; transfer is created and queued from
write(). All
> - * 	flags are cleared when the transfer is completed by
closing the file or
> - * 	reading all of the available response data.
> + *  flags are cleared when the transfer is completed by closing the
file or
> + *  reading all of the available response data.
>   * XFR_IN_PROGRESS is set when a transfer is started from
occ_worker_putsram,
> - * 	and cleared if the transfer fails or occ_worker_getsram
completes.
> + *  and cleared if the transfer fails or occ_worker_getsram
completes.
>   * XFR_COMPLETE is set when a transfer fails or finishes
occ_worker_getsram.
>   * XFR_CANCELED is set when the transfer's client is released.
>   * XFR_WAITING is set from read() if the transfer isn't complete and
> - * 	NONBLOCKING wasn't specified. Cleared in read() when
transfer completes
> - * 	or fails.
> + *  O_NONBLOCK wasn't specified. Cleared in read() when transfer
completes or
> + *  fails.
>   */
>  enum {
>  	XFR_IN_PROGRESS,
> @@ -89,9 +89,9 @@ struct occ_xfr {
>   * client flags
>   *
>   * CLIENT_NONBLOCKING is set during open() if the file was opened
with the
> - * 	O_NONBLOCKING flag.
> + *  O_NONBLOCK flag.
>   * CLIENT_XFR_PENDING is set during write() and cleared when all
data has been
> - * 	read.
> + *  read.
>   */
>  enum {
>  	CLIENT_NONBLOCKING,
> @@ -101,7 +101,7 @@ enum {
>  struct occ_client {
>  	struct occ *occ;
>  	struct occ_xfr xfr;
> -	spinlock_t lock;
> +	spinlock_t lock;		/* lock access to the client
state */
>  	wait_queue_head_t wait;
>  	size_t read_offset;
>  	unsigned long flags;
> @@ -273,10 +273,15 @@ static ssize_t occ_write_common(struct
occ_client *client,
>  		goto done;
>  	}
>  
> -	/* clear out the transfer */
> -	memset(xfr, 0, sizeof(*xfr));
> -	xfr->buf[0] = 1;
> +	memset(xfr, 0, sizeof(*xfr));	/* clear out the
transfer */
> +	xfr->buf[0] = 1;		/* occ sequence number */
>  
> +	/*
> +	 * Assume user data follows the occ command format.
> +	 * byte 0: command type
> +	 * bytes 1-2: data length (msb first)
> +	 * bytes 3-n: data
> +	 */
>  	if (ubuf) {
>  		if (copy_from_user(&xfr->buf[1], ubuf, len)) {
>  			rc = -EFAULT;
> @@ -354,7 +359,7 @@ static int occ_release_common(struct occ_client
*client)
>  		return 0;
>  	}
>  
> -	/* operation is in progress; let worker clean up*/
> +	/* operation is in progress; let worker clean up */
>  	spin_unlock_irq(&occ->list_lock);
>  	spin_unlock_irq(&client->lock);
>  	return 0;
> @@ -419,9 +424,13 @@ static int occ_getsram(struct device *sbefifo,
u32 address, u8 *data,
>  	int rc;
>  	u8 *resp;
>  	__be32 buf[5];
> -	u32 data_len = ((len + 7) / 8) * 8;
> +	u32 data_len = ((len + 7) / 8) * 8;	/* must be
multiples of 8 B */
>  	struct sbefifo_client *client;
>  
> +	/*
> +	 * Magic sequence to do SBE getsram command. SBE will fetch
data from
> +	 * specified SRAM address.
> +	 */
>  	buf[0] = cpu_to_be32(0x5);
>  	buf[1] = cpu_to_be32(0xa403);
>  	buf[2] = cpu_to_be32(1);
> @@ -470,7 +479,7 @@ static int occ_putsram(struct device *sbefifo,
u32 address, u8 *data,
>  {
>  	int rc;
>  	__be32 *buf;
> -	u32 data_len = ((len + 7) / 8) * 8;
> +	u32 data_len = ((len + 7) / 8) * 8;	/* must be
multiples of 8 B */
>  	size_t cmd_len = data_len + 20;
>  	struct sbefifo_client *client;
>  
> @@ -478,6 +487,10 @@ static int occ_putsram(struct device *sbefifo,
u32 address, u8 *data,
>  	if (!buf)
>  		return -ENOMEM;
>  
> +	/*
> +	 * Magic sequence to do SBE putsram command. SBE will
transfer
> +	 * data to specified SRAM address.
> +	 */
>  	buf[0] = cpu_to_be32(0x5 + (data_len / 4));
>  	buf[1] = cpu_to_be32(0xa404);
>  	buf[2] = cpu_to_be32(1);
> @@ -518,11 +531,15 @@ static int occ_trigger_attn(struct device
*sbefifo)
>  	__be32 buf[6];
>  	struct sbefifo_client *client;
>  
> +	/*
> +	 * Magic sequence to do SBE putscom command. SBE will write
8 bytes to
> +	 * specified SCOM address.
> +	 */
>  	buf[0] = cpu_to_be32(0x6);
>  	buf[1] = cpu_to_be32(0xa202);
>  	buf[2] = 0;
>  	buf[3] = cpu_to_be32(0x6D035);
> -	buf[4] = cpu_to_be32(0x20010000);
> +	buf[4] = cpu_to_be32(0x20010000);	/* trigger occ
attention */
>  	buf[5] = 0;
>  
>  	client = sbefifo_drv_open(sbefifo, 0);
> @@ -573,6 +590,7 @@ static void occ_worker(struct work_struct *work)
>  	spin_unlock_irq(&occ->list_lock);
>  	mutex_lock(&occ->occ_lock);
>  
> +	/* write occ command */
>  	rc = occ_putsram(sbefifo, 0xFFFBE000, xfr->buf,
>  			 xfr->cmd_data_length);
>  	if (rc)
> @@ -582,6 +600,7 @@ static void occ_worker(struct work_struct *work)
>  	if (rc)
>  		goto done;
>  
> +	/* read occ response */
>  	rc = occ_getsram(sbefifo, 0xFFFBF000, xfr->buf, 8);
>  	if (rc)
>  		goto done;

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH linux dev-4.10 v3 26/31] drivers: fsi: occ: Add OCC response definitions to header

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 1673 bytes --]

On Thu, 2017-10-05 at 14:24 -0500, Eddie James wrote:
> From: "Edward A. James" <eajames@us.ibm.com>
> 
> Also fix include guards.
> 
> Signed-off-by: Edward A. James <eajames@us.ibm.com>

Acked-by: Andrew Jeffery <andrew@aj.id.au>

> ---
>  include/linux/occ.h | 20 +++++++++++++++++---
>  1 file changed, 17 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/occ.h b/include/linux/occ.h
> index d78332c..0a4a54a 100644
> --- a/include/linux/occ.h
> +++ b/include/linux/occ.h
> @@ -11,12 +11,26 @@
>   * GNU General Public License for more details.
>   */
>  
> -#ifndef __OCC_H__
> -#define __OCC_H__
> +#ifndef LINUX_FSI_OCC_H
> +#define LINUX_FSI_OCC_H
>  
>  struct device;
>  struct occ_client;
>  
> +#define OCC_RESP_CMD_IN_PRG		0xFF
> +#define OCC_RESP_SUCCESS		0
> +#define OCC_RESP_CMD_INVAL		0x11
> +#define OCC_RESP_CMD_LEN_INVAL		0x12
> +#define OCC_RESP_DATA_INVAL		0x13
> +#define OCC_RESP_CHKSUM_ERR		0x14
> +#define OCC_RESP_INT_ERR		0x15
> +#define OCC_RESP_BAD_STATE		0x16
> +#define OCC_RESP_CRIT_EXCEPT		0xE0
> +#define OCC_RESP_CRIT_INIT		0xE1
> +#define OCC_RESP_CRIT_WATCHDOG		0xE2
> +#define OCC_RESP_CRIT_OCB		0xE3
> +#define OCC_RESP_CRIT_HW		0xE4
> +
>  extern struct occ_client *occ_drv_open(struct device *dev,
>  				       unsigned long flags);
>  extern int occ_drv_read(struct occ_client *client, char *buf, size_t len);
> @@ -24,4 +38,4 @@ extern int occ_drv_write(struct occ_client *client, const char *buf,
>  			 size_t len);
>  extern void occ_drv_release(struct occ_client *client);
>  
> -#endif /* __OCC_H__ */
> +#endif /* LINUX_FSI_OCC_H */

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH linux dev-4.10 v3 27/31] drivers: fsi: occ: Poll while receiving "command in progress"

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 2798 bytes --]

On Thu, 2017-10-05 at 14:24 -0500, Eddie James wrote:
> From: "Edward A. James" <eajames@us.ibm.com>
> 
> Waiting for OCC to return something other than "command in progress"
> should only be done in the worker function, instead of clients repeating
> the entire transfer when they receive "command in progress." In this way
> clients don't have to check for that return status.
> 
> Signed-off-by: Edward A. James <eajames@us.ibm.com>
> ---
>  drivers/fsi/occ.c | 27 ++++++++++++++++++++++++---
>  1 file changed, 24 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/fsi/occ.c b/drivers/fsi/occ.c
> index 29a17ed..8555e99 100644
> --- a/drivers/fsi/occ.c
> +++ b/drivers/fsi/occ.c
> @@ -23,6 +23,7 @@
>  #include <linux/of.h>
>  #include <linux/of_platform.h>
>  #include <linux/platform_device.h>
> +#include <linux/sched.h>
>  #include <linux/slab.h>
>  #include <linux/spinlock.h>
>  #include <linux/uaccess.h>
> @@ -33,6 +34,9 @@
>  #define OCC_CMD_DATA_BYTES	4090
>  #define OCC_RESP_DATA_BYTES	4089
>  
> +#define OCC_TIMEOUT_MS		1000
> +#define OCC_CMD_IN_PRG_WAIT_MS	50

Where doe 50ms come from? Is it a bum-plucked number? Or should I dig out my
OCC documentation?

The rest looks okay though, so:

Acked-by: Andrew Jeffery <andrew@aj.id.au>

> +
>  struct occ {
>  	struct device *sbefifo;
>  	char name[32];
> @@ -569,6 +573,9 @@ static void occ_worker(struct work_struct *work)
>  {
>  	int rc = 0, empty, waiting, canceled;
>  	u16 resp_data_length;
> +	unsigned long start;
> +	const unsigned long timeout = msecs_to_jiffies(OCC_TIMEOUT_MS);
> +	const long int wait_time = msecs_to_jiffies(OCC_CMD_IN_PRG_WAIT_MS);
>  	struct occ_xfr *xfr;
>  	struct occ_response *resp;
>  	struct occ_client *client;
> @@ -590,6 +597,8 @@ static void occ_worker(struct work_struct *work)
>  	spin_unlock_irq(&occ->list_lock);
>  	mutex_lock(&occ->occ_lock);
>  
> +	start = jiffies;
> +
>  	/* write occ command */
>  	rc = occ_putsram(sbefifo, 0xFFFBE000, xfr->buf,
>  			 xfr->cmd_data_length);
> @@ -601,9 +610,21 @@ static void occ_worker(struct work_struct *work)
>  		goto done;
>  
>  	/* read occ response */
> -	rc = occ_getsram(sbefifo, 0xFFFBF000, xfr->buf, 8);
> -	if (rc)
> -		goto done;
> +	do {
> +		rc = occ_getsram(sbefifo, 0xFFFBF000, xfr->buf, 8);
> +		if (rc)
> +			goto done;
> +
> +		if (resp->return_status == OCC_RESP_CMD_IN_PRG) {
> +			rc = -EALREADY;
> +
> +			if (time_after(jiffies, start + timeout))
> +				break;
> +
> +			set_current_state(TASK_INTERRUPTIBLE);
> +			schedule_timeout(wait_time);
> +		}
> +	} while (rc);
>  
>  	resp_data_length = get_unaligned_be16(&resp->data_length);
>  	if (resp_data_length > OCC_RESP_DATA_BYTES) {

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH linux dev-4.10 v3 28/31] drivers: fsi: occ: Add cancel to remove() and fix probe()

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 6973 bytes --]

On Thu, 2017-10-05 at 14:24 -0500, Eddie James wrote:
> From: "Edward A. James" <eajames@us.ibm.com>
> 
> Need some data to indicate to clients and the rest of the driver when
> the device is being removed, so add a cancel boolean. Fix up both the
> probe and remove functions to properly handle failures and prevent
> deadlocks.
> 
> Signed-off-by: Edward A. James <eajames@us.ibm.com>
> ---
>  drivers/fsi/occ.c | 86 +++++++++++++++++++++++++++++++++++++++----------------
>  1 file changed, 62 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/fsi/occ.c b/drivers/fsi/occ.c
> index 8555e99..1770823 100644
> --- a/drivers/fsi/occ.c
> +++ b/drivers/fsi/occ.c
> @@ -46,6 +46,7 @@ struct occ {
>  	spinlock_t list_lock;		/* lock access to the xfrs list */
>  	struct mutex occ_lock;		/* lock access to the hardware */
>  	struct work_struct work;
> +	bool cancel;
>  };
>  
>  #define to_occ(x)	container_of((x), struct occ, mdev)
> @@ -117,12 +118,15 @@ struct occ_client {
>  
>  static DEFINE_IDA(occ_ida);
>  
> -static void occ_enqueue_xfr(struct occ_xfr *xfr)
> +static int occ_enqueue_xfr(struct occ_xfr *xfr)
>  {
>  	int empty;
>  	struct occ_client *client = to_client(xfr);
>  	struct occ *occ = client->occ;
>  
> +	if (occ->cancel)
> +		return -ECANCELED;
> +
>  	spin_lock_irq(&occ->list_lock);
>  
>  	empty = list_empty(&occ->xfrs);
> @@ -132,14 +136,20 @@ static void occ_enqueue_xfr(struct occ_xfr *xfr)
>  
>  	if (empty)
>  		queue_work(occ_wq, &occ->work);
> +
> +	return 0;
>  }
>  
>  static struct occ_client *occ_open_common(struct occ *occ, unsigned long flags)
>  {
> -	struct occ_client *client = kzalloc(sizeof(*client), GFP_KERNEL);
> +	struct occ_client *client;
> +
> +	if (occ->cancel)
> +		return ERR_PTR(-ECANCELED);

I don't think this test is helpful, in that it's racy. What if remove() gets
called immediately after the thread calling into open() has passed the test?

>  
> +	client = kzalloc(sizeof(*client), GFP_KERNEL);
>  	if (!client)
> -		return NULL;
> +		return ERR_PTR(-ENOMEM);
>  
>  	client->occ = occ;
>  	spin_lock_init(&client->lock);
> @@ -158,8 +168,8 @@ static int occ_open(struct inode *inode, struct file *file)
>  	struct occ *occ = to_occ(mdev);
>  
>  	client = occ_open_common(occ, file->f_flags);
> -	if (!client)
> -		return -ENOMEM;
> +	if (IS_ERR(client))
> +		return PTR_ERR(client);
>  
>  	file->private_data = client;
>  
> @@ -172,6 +182,7 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
>  	int rc;
>  	size_t bytes;
>  	struct occ_xfr *xfr = &client->xfr;
> +	struct occ *occ = client->occ;
>  
>  	if (len > OCC_SRAM_BYTES)
>  		return -EINVAL;
> @@ -204,7 +215,8 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
>  					      test_bit(XFR_COMPLETE,
>  						       &xfr->flags) ||
>  					      test_bit(XFR_CANCELED,
> -						       &xfr->flags));
> +						       &xfr->flags) ||
> +					      occ->cancel);
>  
>  		spin_lock_irq(&client->lock);
>  
> @@ -272,7 +284,7 @@ static ssize_t occ_write_common(struct occ_client *client,
>  
>  	spin_lock_irq(&client->lock);
>  
> -	if (test_and_set_bit(CLIENT_XFR_PENDING, &client->flags)) {
> +	if (test_bit(CLIENT_XFR_PENDING, &client->flags)) {
>  		rc = -EBUSY;
>  		goto done;
>  	}
> @@ -310,8 +322,11 @@ static ssize_t occ_write_common(struct occ_client *client,
>  	xfr->cmd_data_length = data_length + 6;
>  	client->read_offset = 0;
>  
> -	occ_enqueue_xfr(xfr);
> +	rc = occ_enqueue_xfr(xfr);
> +	if (rc)
> +		goto done;
>  
> +	set_bit(CLIENT_XFR_PENDING, &client->flags);
>  	rc = len;
>  
>  done:
> @@ -583,6 +598,9 @@ static void occ_worker(struct work_struct *work)
>  	struct device *sbefifo = occ->sbefifo;
>  
>  again:
> +	if (occ->cancel)
> +		return;
> +
>  	spin_lock_irq(&occ->list_lock);
>  
>  	xfr = list_first_entry_or_null(&occ->xfrs, struct occ_xfr, link);
> @@ -676,12 +694,17 @@ static void occ_worker(struct work_struct *work)
>  
>  struct occ_client *occ_drv_open(struct device *dev, unsigned long flags)
>  {
> +	struct occ_client *client;
>  	struct occ *occ = dev_get_drvdata(dev);
>  
>  	if (!occ)
>  		return NULL;
>  
> -	return occ_open_common(occ, flags);
> +	client = occ_open_common(occ, flags);
> +	if (IS_ERR(client))
> +		return NULL;
> +
> +	return client;
>  }
>  EXPORT_SYMBOL_GPL(occ_drv_open);
>  
> @@ -752,23 +775,13 @@ static int occ_probe(struct platform_device *pdev)
>  			if (occ->idx < 0)
>  				occ->idx = ida_simple_get(&occ_ida, 1, INT_MAX,
>  							  GFP_KERNEL);
> -		} else
> +		} else {
>  			occ->idx = ida_simple_get(&occ_ida, 1, INT_MAX,
>  						  GFP_KERNEL);
> -
> -		/* create platform devs for dts child nodes (hwmon, etc) */
> -		for_each_child_of_node(dev->of_node, np) {
> -			snprintf(child_name, sizeof(child_name), "occ%d-dev%d",
> -				 occ->idx, child_idx++);
> -			child = of_platform_device_create(np, child_name, dev);
> -			if (!child)
> -				dev_warn(dev,
> -					 "failed to create child node dev\n");
>  		}
> -	} else
> +	} else {
>  		occ->idx = ida_simple_get(&occ_ida, 1, INT_MAX, GFP_KERNEL);
> -
> -	platform_set_drvdata(pdev, occ);
> +	}
>  
>  	snprintf(occ->name, sizeof(occ->name), "occ%d", occ->idx);
>  	occ->mdev.fops = &occ_fops;
> @@ -778,20 +791,45 @@ static int occ_probe(struct platform_device *pdev)
>  
>  	rc = misc_register(&occ->mdev);
>  	if (rc) {
> -		dev_err(dev, "failed to register miscdevice\n");
> +		dev_err(dev, "failed to register miscdevice: %d\n", rc);
> +		ida_simple_remove(&occ_ida, occ->idx);
>  		return rc;
>  	}
>  
> +	/* create platform devs for dts child nodes (hwmon, etc) */
> +	for_each_available_child_of_node(dev->of_node, np) {
> +		snprintf(child_name, sizeof(child_name), "occ%d-dev%d",
> +			 occ->idx, child_idx++);
> +		child = of_platform_device_create(np, child_name, dev);
> +		if (!child)
> +			dev_warn(dev, "failed to create child node dev\n");
> +	}
> +
> +	platform_set_drvdata(pdev, occ);
> +
>  	return 0;
>  }
>  
>  static int occ_remove(struct platform_device *pdev)
>  {
>  	struct occ *occ = platform_get_drvdata(pdev);
> +	struct occ_xfr *xfr;
> +	struct occ_client *client;
> +
> +	occ->cancel = true;
> +
> +	spin_lock_irq(&occ->list_lock);
> +	list_for_each_entry(xfr, &occ->xfrs, link) {
> +		client = to_client(xfr);
> +		wake_up_all(&client->wait);
> +	}
> +	spin_unlock_irq(&occ->list_lock);
>  
> -	flush_work(&occ->work);
>  	misc_deregister(&occ->mdev);
>  	device_for_each_child(&pdev->dev, NULL, occ_unregister_child);
> +
> +	cancel_work_sync(&occ->work);
> +
>  	ida_simple_remove(&occ_ida, occ->idx);
>  
>  	return 0;

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH linux dev-4.10 v3 30/31] drivers/hwmon/occ: Remove repeated ops for OCC command in progress

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 1509 bytes --]

On Thu, 2017-10-05 at 14:24 -0500, Eddie James wrote:
> From: "Edward A. James" <eajames@us.ibm.com>
> 
> This is now handled in the occ driver.
> 
> Signed-off-by: Edward A. James <eajames@us.ibm.com>

Reviewed-by: Andrew Jeffery <andrew@aj.id.au>

> ---
>  drivers/hwmon/occ/p9_sbe.c | 14 +-------------
>  1 file changed, 1 insertion(+), 13 deletions(-)
> 
> diff --git a/drivers/hwmon/occ/p9_sbe.c b/drivers/hwmon/occ/p9_sbe.c
> index 2d50a94..c7e0d9c 100644
> --- a/drivers/hwmon/occ/p9_sbe.c
> +++ b/drivers/hwmon/occ/p9_sbe.c
> @@ -26,14 +26,10 @@ struct p9_sbe_occ {
>  static int p9_sbe_occ_send_cmd(struct occ *occ, u8 *cmd)
>  {
>  	int rc, error;
> -	unsigned long start;
>  	struct occ_client *client;
>  	struct occ_response *resp = &occ->resp;
>  	struct p9_sbe_occ *p9_sbe_occ = to_p9_sbe_occ(occ);
>  
> -	start = jiffies;
> -
> -retry:
>  	client = occ_drv_open(p9_sbe_occ->sbe, 0);
>  	if (!client) {
>  		rc = -ENODEV;
> @@ -52,15 +48,7 @@ static int p9_sbe_occ_send_cmd(struct occ *occ, u8 *cmd)
>  
>  	switch (resp->return_status) {
>  	case RESP_RETURN_CMD_IN_PRG:
> -		if (time_after(jiffies,
> -			       start + msecs_to_jiffies(OCC_TIMEOUT_MS)))
> -			rc = -EALREADY;
> -		else {
> -			set_current_state(TASK_INTERRUPTIBLE);
> -			schedule_timeout(msecs_to_jiffies(OCC_CMD_IN_PRG_MS));
> -
> -			goto retry;
> -		}
> +		rc = -ETIMEDOUT;
>  		break;
>  	case RESP_RETURN_SUCCESS:
>  		occ_reset_error(occ);

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH linux dev-4.10 v3 24/31] drivers: fsi: occ: Remove unnecessary platform_set_drvdata call

[Eddie James]

On 10/05/2017 06:14 PM, Andrew Jeffery wrote:
> On Thu, 2017-10-05 at 14:24 -0500, Eddie James wrote:
>> From: "Edward A. James" <eajames@us.ibm.com>
>>   
>> The purpose was to set that pointer null before probing. However, it is
>> set null by the kernel by default.
>>   
>> Signed-off-by: Edward A. James <eajames@us.ibm.com>
>> ---
>>   drivers/fsi/occ.c | 3 ---
>>   1 file changed, 3 deletions(-)
>>   
>> diff --git a/drivers/fsi/occ.c b/drivers/fsi/occ.c
>> index 916c342..561b029 100644
>> --- a/drivers/fsi/occ.c
>> +++ b/drivers/fsi/occ.c
>> @@ -703,9 +703,6 @@ static int occ_probe(struct platform_device *pdev)
>>   	mutex_init(&occ->occ_lock);
>>   	INIT_WORK(&occ->work, occ_worker);
>>   
>> -	/* ensure NULL before we probe children, so they don't hang FSI */
>> -	platform_set_drvdata(pdev, NULL);
>> -
> Bizarre that it was even there in the first place - do you know the history of
> the comment?

I made the comment. I wasn't sure if there were situations where that 
wouldn't be NULL before probe() somehow. Regarding the hanging FSI... 
that was before some FSI core fixes were in that prevent that situation 
during probe().

>
>>   	if (dev->of_node) {
>>   		rc = of_property_read_u32(dev->of_node, "reg", &reg);
>>   		if (!rc) {

Re: [PATCH linux dev-4.10 v3 18/31] drivers: fsi: occ: Use big-endian values

[Eddie James]

On 10/05/2017 06:03 PM, Andrew Jeffery wrote:
> On Thu, 2017-10-05 at 14:24 -0500, Eddie James wrote:
>> From: "Edward A. James" <eajames@us.ibm.com>
>>   
>> Switch to __be16 from u16 and __be32 from u32. Also, use kernel access
>> of unaligned be16 instead of manually creating a big-endian value from
>> the occ response buffer.
>>   
>> Signed-off-by: Edward A. James <eajames@us.ibm.com>
>> ---
>>   drivers/fsi/occ.c | 14 ++++++++------
>>   1 file changed, 8 insertions(+), 6 deletions(-)
>>   
>> diff --git a/drivers/fsi/occ.c b/drivers/fsi/occ.c
>> index 82a60c5..55f293d 100644
>> --- a/drivers/fsi/occ.c
>> +++ b/drivers/fsi/occ.c
>> @@ -50,9 +50,9 @@ struct occ_response {
>>   	u8 seq_no;
>>   	u8 cmd_type;
>>   	u8 return_status;
>> -	u16 data_length;
>> +	__be16 data_length;
>>   	u8 data[OCC_RESP_DATA_BYTES];
>> -	u16 checksum;
>> +	__be16 checksum;
>>   } __packed;
>>   
>>   /*
>> @@ -425,7 +425,7 @@ static int occ_getsram(struct device *sbefifo, u32 address, u8 *data,
>>   {
>>   	int rc;
>>   	u8 *resp;
>> -	u32 buf[5];
>> +	__be32 buf[5];
>>   	u32 data_len = ((len + 7) / 8) * 8;
>>   	struct sbefifo_client *client;
>>   
>> @@ -476,7 +476,7 @@ static int occ_putsram(struct device *sbefifo, u32 address, u8 *data,
>>   		       ssize_t len)
>>   {
>>   	int rc;
>> -	u32 *buf;
>> +	__be32 *buf;
>>   	u32 data_len = ((len + 7) / 8) * 8;
>>   	size_t cmd_len = data_len + 20;
>>   	struct sbefifo_client *client;
>> @@ -522,7 +522,7 @@ static int occ_putsram(struct device *sbefifo, u32 address, u8 *data,
>>   static int occ_trigger_attn(struct device *sbefifo)
>>   {
>>   	int rc;
>> -	u32 buf[6];
>> +	__be32 buf[6];
>>   	struct sbefifo_client *client;
>>   
>>   	buf[0] = cpu_to_be32(0x6);
>> @@ -560,6 +560,7 @@ static void occ_worker(struct work_struct *work)
>>   	int rc = 0, empty, waiting, canceled;
>>   	u16 resp_data_length;
>>   	struct occ_xfr *xfr;
>> +	struct occ_response *resp;
>>   	struct occ_client *client;
>>   	struct occ *occ = container_of(work, struct occ, work);
>>   	struct device *sbefifo = occ->sbefifo;
>> @@ -573,6 +574,7 @@ static void occ_worker(struct work_struct *work)
>>   		return;
>>   	}
>>   
>> +	resp = (struct occ_response *)xfr->buf;
>>   	set_bit(XFR_IN_PROGRESS, &xfr->flags);
>>   
>>   	spin_unlock_irq(&occ->list_lock);
>> @@ -591,7 +593,7 @@ static void occ_worker(struct work_struct *work)
>>   	if (rc)
>>   		goto done;
>>   
>> -	resp_data_length = (xfr->buf[3] << 8) + xfr->buf[4];
>> +	resp_data_length = get_unaligned_be16(&resp->data_length);
>>   	if (resp_data_length > OCC_RESP_DATA_BYTES) {
> You need to do be16_to_cpu() before any comparisons.

Hmm, I'm not convinced. I see this in access_ok.h

static  __always_inline 
<http://elixir.free-electrons.com/linux/latest/ident/__always_inline>  u16  get_unaligned_be16 
<http://elixir.free-electrons.com/linux/latest/ident/get_unaligned_be16>(const  void  *p)
{
	return  be16_to_cpup 
<http://elixir.free-electrons.com/linux/latest/ident/be16_to_cpup>((__be16 
<http://elixir.free-electrons.com/linux/latest/ident/__be16>  *)p);
} I think it will do the translation as well. Thanks, Eddie

>
>>   		rc = -EDOM;
>>   		goto done;

Re: [PATCH linux dev-4.10 v3 24/31] drivers: fsi: occ: Remove unnecessary platform_set_drvdata call

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 1597 bytes --]

On Thu, 2017-10-05 at 19:33 -0500, Eddie James wrote:
> 
> On 10/05/2017 06:14 PM, Andrew Jeffery wrote:
> > On Thu, 2017-10-05 at 14:24 -0500, Eddie James wrote:
> > > > > > From: "Edward A. James" <eajames@us.ibm.com>
> > >   
> > > The purpose was to set that pointer null before probing. However, it is
> > > set null by the kernel by default.
> > >   
> > > > > > Signed-off-by: Edward A. James <eajames@us.ibm.com>
> > > ---
> > >   drivers/fsi/occ.c | 3 ---
> > >   1 file changed, 3 deletions(-)
> > >   
> > > diff --git a/drivers/fsi/occ.c b/drivers/fsi/occ.c
> > > index 916c342..561b029 100644
> > > --- a/drivers/fsi/occ.c
> > > +++ b/drivers/fsi/occ.c
> > > @@ -703,9 +703,6 @@ static int occ_probe(struct platform_device *pdev)
> > > > > >   	mutex_init(&occ->occ_lock);
> > > > > >   	INIT_WORK(&occ->work, occ_worker);
> > >   
> > > > > > -	/* ensure NULL before we probe children, so they don't hang FSI */
> > > > > > -	platform_set_drvdata(pdev, NULL);
> > > -
> > 
> > Bizarre that it was even there in the first place - do you know the history of
> > the comment?
> 
> I made the comment. I wasn't sure if there were situations where that 
> wouldn't be NULL before probe() somehow. Regarding the hanging FSI... 
> that was before some FSI core fixes were in that prevent that situation 
> during probe().

Fair enough.

Reviewed-by: Andrew Jeffery <andrew@aj.id.au>

> 
> > 
> > > > > >   	if (dev->of_node) {
> > > > > >   		rc = of_property_read_u32(dev->of_node, "reg", &reg);
> > >   		if (!rc) {
> 
> 

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH linux dev-4.10 v3 18/31] drivers: fsi: occ: Use big-endian values

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 4109 bytes --]

On Thu, 2017-10-05 at 19:41 -0500, Eddie James wrote:
> 
> On 10/05/2017 06:03 PM, Andrew Jeffery wrote:
> > On Thu, 2017-10-05 at 14:24 -0500, Eddie James wrote:
> > > > > > From: "Edward A. James" <eajames@us.ibm.com>
> > >   
> > > Switch to __be16 from u16 and __be32 from u32. Also, use kernel access
> > > of unaligned be16 instead of manually creating a big-endian value from
> > > the occ response buffer.
> > >   
> > > > > > Signed-off-by: Edward A. James <eajames@us.ibm.com>
> > > ---
> > >   drivers/fsi/occ.c | 14 ++++++++------
> > >   1 file changed, 8 insertions(+), 6 deletions(-)
> > >   
> > > diff --git a/drivers/fsi/occ.c b/drivers/fsi/occ.c
> > > index 82a60c5..55f293d 100644
> > > --- a/drivers/fsi/occ.c
> > > +++ b/drivers/fsi/occ.c
> > > @@ -50,9 +50,9 @@ struct occ_response {
> > > > > >   	u8 seq_no;
> > > > > >   	u8 cmd_type;
> > > > > >   	u8 return_status;
> > > > > > -	u16 data_length;
> > > > > > +	__be16 data_length;
> > > > > >   	u8 data[OCC_RESP_DATA_BYTES];
> > > > > > -	u16 checksum;
> > > > > > +	__be16 checksum;
> > >   } __packed;
> > >   
> > >   /*
> > > @@ -425,7 +425,7 @@ static int occ_getsram(struct device *sbefifo, u32 address, u8 *data,
> > >   {
> > > > > >   	int rc;
> > > > > >   	u8 *resp;
> > > > > > -	u32 buf[5];
> > > > > > +	__be32 buf[5];
> > > > > >   	u32 data_len = ((len + 7) / 8) * 8;
> > > > > >   	struct sbefifo_client *client;
> > >   
> > > @@ -476,7 +476,7 @@ static int occ_putsram(struct device *sbefifo, u32 address, u8 *data,
> > > > > >   		       ssize_t len)
> > >   {
> > > > > >   	int rc;
> > > > > > -	u32 *buf;
> > > > > > +	__be32 *buf;
> > > > > >   	u32 data_len = ((len + 7) / 8) * 8;
> > > > > >   	size_t cmd_len = data_len + 20;
> > > > > >   	struct sbefifo_client *client;
> > > @@ -522,7 +522,7 @@ static int occ_putsram(struct device *sbefifo, u32 address, u8 *data,
> > >   static int occ_trigger_attn(struct device *sbefifo)
> > >   {
> > > > > >   	int rc;
> > > > > > -	u32 buf[6];
> > > > > > +	__be32 buf[6];
> > > > > >   	struct sbefifo_client *client;
> > >   
> > > > > >   	buf[0] = cpu_to_be32(0x6);
> > > @@ -560,6 +560,7 @@ static void occ_worker(struct work_struct *work)
> > > > > >   	int rc = 0, empty, waiting, canceled;
> > > > > >   	u16 resp_data_length;
> > > > > >   	struct occ_xfr *xfr;
> > > > > > +	struct occ_response *resp;
> > > > > >   	struct occ_client *client;
> > > > > >   	struct occ *occ = container_of(work, struct occ, work);
> > > > > >   	struct device *sbefifo = occ->sbefifo;
> > > @@ -573,6 +574,7 @@ static void occ_worker(struct work_struct *work)
> > > > > >   		return;
> > > > > >   	}
> > >   
> > > > > > +	resp = (struct occ_response *)xfr->buf;
> > > > > >   	set_bit(XFR_IN_PROGRESS, &xfr->flags);
> > >   
> > > > > >   	spin_unlock_irq(&occ->list_lock);
> > > 

> > > @@ -591,7 +593,7 @@ static void occ_worker(struct work_struct *work)
> > > > > >   	if (rc)
> > > > > >   		goto done;
> > >   
> > > > > > -	resp_data_length = (xfr->buf[3] << 8) + xfr->buf[4];
> > > > > > +	resp_data_length = get_unaligned_be16(&resp->data_length);
> > >   	if (resp_data_length > OCC_RESP_DATA_BYTES) {
> > 
> > You need to do be16_to_cpu() before any comparisons.
> 
> Hmm, I'm not convinced. I see this in access_ok.h
> 
> static  __always_inline 
> > <http://elixir.free-electrons.com/linux/latest/ident/__always_inline>  u16  get_unaligned_be16 
> <http://elixir.free-electrons.com/linux/latest/ident/get_unaligned_be16>(const  void  *p)
> {
> 	return  be16_to_cpup 
> <http://elixir.free-electrons.com/linux/latest/ident/be16_to_cpup>((__be16 
> > <http://elixir.free-electrons.com/linux/latest/ident/__be16>  *)p);
> } I think it will do the translation as well. Thanks, Eddie

Ugh, yeah, brain fart. Sorry about that.

Reviewed-by: Andrew Jeffery <andrew@aj.id.au>


> 
> > 
> > > > > >   		rc = -EDOM;
> > >   		goto done;
> 
> 

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH linux dev-4.10 v3 27/31] drivers: fsi: occ: Poll while receiving "command in progress"

[Eddie James]

On 10/05/2017 06:27 PM, Andrew Jeffery wrote:
> On Thu, 2017-10-05 at 14:24 -0500, Eddie James wrote:
>> From: "Edward A. James" <eajames@us.ibm.com>
>>   
>> Waiting for OCC to return something other than "command in progress"
>> should only be done in the worker function, instead of clients repeating
>> the entire transfer when they receive "command in progress." In this way
>> clients don't have to check for that return status.
>>   
>> Signed-off-by: Edward A. James <eajames@us.ibm.com>
>> ---
>>   drivers/fsi/occ.c | 27 ++++++++++++++++++++++++---
>>   1 file changed, 24 insertions(+), 3 deletions(-)
>>   
>> diff --git a/drivers/fsi/occ.c b/drivers/fsi/occ.c
>> index 29a17ed..8555e99 100644
>> --- a/drivers/fsi/occ.c
>> +++ b/drivers/fsi/occ.c
>> @@ -23,6 +23,7 @@
>>   #include <linux/of.h>
>>   #include <linux/of_platform.h>
>>   #include <linux/platform_device.h>
>> +#include <linux/sched.h>
>>   #include <linux/slab.h>
>>   #include <linux/spinlock.h>
>>   #include <linux/uaccess.h>
>> @@ -33,6 +34,9 @@
>>   #define OCC_CMD_DATA_BYTES	4090
>>   #define OCC_RESP_DATA_BYTES	4089
>>   
>> +#define OCC_TIMEOUT_MS		1000
>> +#define OCC_CMD_IN_PRG_WAIT_MS	50
> Where doe 50ms come from? Is it a bum-plucked number? Or should I dig out my
> OCC documentation?
>
> The rest looks okay though, so:

I just fabricated it. OCC spec doesn't provide any info on what to do 
here. Just a way to do polling on the "command in progress" response.

Thanks,
Eddie

>
> Acked-by: Andrew Jeffery <andrew@aj.id.au>
>
>> +
>>   struct occ {
>>   	struct device *sbefifo;
>>   	char name[32];
>> @@ -569,6 +573,9 @@ static void occ_worker(struct work_struct *work)
>>   {
>>   	int rc = 0, empty, waiting, canceled;
>>   	u16 resp_data_length;
>> +	unsigned long start;
>> +	const unsigned long timeout = msecs_to_jiffies(OCC_TIMEOUT_MS);
>> +	const long int wait_time = msecs_to_jiffies(OCC_CMD_IN_PRG_WAIT_MS);
>>   	struct occ_xfr *xfr;
>>   	struct occ_response *resp;
>>   	struct occ_client *client;
>> @@ -590,6 +597,8 @@ static void occ_worker(struct work_struct *work)
>>   	spin_unlock_irq(&occ->list_lock);
>>   	mutex_lock(&occ->occ_lock);
>>   
>> +	start = jiffies;
>> +
>>   	/* write occ command */
>>   	rc = occ_putsram(sbefifo, 0xFFFBE000, xfr->buf,
>>   			 xfr->cmd_data_length);
>> @@ -601,9 +610,21 @@ static void occ_worker(struct work_struct *work)
>>   		goto done;
>>   
>>   	/* read occ response */
>> -	rc = occ_getsram(sbefifo, 0xFFFBF000, xfr->buf, 8);
>> -	if (rc)
>> -		goto done;
>> +	do {
>> +		rc = occ_getsram(sbefifo, 0xFFFBF000, xfr->buf, 8);
>> +		if (rc)
>> +			goto done;
>> +
>> +		if (resp->return_status == OCC_RESP_CMD_IN_PRG) {
>> +			rc = -EALREADY;
>> +
>> +			if (time_after(jiffies, start + timeout))
>> +				break;
>> +
>> +			set_current_state(TASK_INTERRUPTIBLE);
>> +			schedule_timeout(wait_time);
>> +		}
>> +	} while (rc);
>>   
>>   	resp_data_length = get_unaligned_be16(&resp->data_length);
>>   	if (resp_data_length > OCC_RESP_DATA_BYTES) {

Re: [PATCH linux dev-4.10 v3 28/31] drivers: fsi: occ: Add cancel to remove() and fix probe()

[Eddie James]

On 10/05/2017 06:36 PM, Andrew Jeffery wrote:
> On Thu, 2017-10-05 at 14:24 -0500, Eddie James wrote:
>> From: "Edward A. James" <eajames@us.ibm.com>
>>   
>> Need some data to indicate to clients and the rest of the driver when
>> the device is being removed, so add a cancel boolean. Fix up both the
>> probe and remove functions to properly handle failures and prevent
>> deadlocks.
>>   
>> Signed-off-by: Edward A. James <eajames@us.ibm.com>
>> ---
>>   drivers/fsi/occ.c | 86 +++++++++++++++++++++++++++++++++++++++----------------
>>   1 file changed, 62 insertions(+), 24 deletions(-)
>>   
>> diff --git a/drivers/fsi/occ.c b/drivers/fsi/occ.c
>> index 8555e99..1770823 100644
>> --- a/drivers/fsi/occ.c
>> +++ b/drivers/fsi/occ.c
>> @@ -46,6 +46,7 @@ struct occ {
>>   	spinlock_t list_lock;		/* lock access to the xfrs list */
>>   	struct mutex occ_lock;		/* lock access to the hardware */
>>   	struct work_struct work;
>> +	bool cancel;
>>   };
>>   
>>   #define to_occ(x)	container_of((x), struct occ, mdev)
>> @@ -117,12 +118,15 @@ struct occ_client {
>>   
>>   static DEFINE_IDA(occ_ida);
>>   
>> -static void occ_enqueue_xfr(struct occ_xfr *xfr)
>> +static int occ_enqueue_xfr(struct occ_xfr *xfr)
>>   {
>>   	int empty;
>>   	struct occ_client *client = to_client(xfr);
>>   	struct occ *occ = client->occ;
>>   
>> +	if (occ->cancel)
>> +		return -ECANCELED;
>> +
>>   	spin_lock_irq(&occ->list_lock);
>>   
>>   	empty = list_empty(&occ->xfrs);
>> @@ -132,14 +136,20 @@ static void occ_enqueue_xfr(struct occ_xfr *xfr)
>>   
>>   	if (empty)
>>   		queue_work(occ_wq, &occ->work);
>> +
>> +	return 0;
>>   }
>>   
>>   static struct occ_client *occ_open_common(struct occ *occ, unsigned long flags)
>>   {
>> -	struct occ_client *client = kzalloc(sizeof(*client), GFP_KERNEL);
>> +	struct occ_client *client;
>> +
>> +	if (occ->cancel)
>> +		return ERR_PTR(-ECANCELED);
> I don't think this test is helpful, in that it's racy. What if remove() gets
> called immediately after the thread calling into open() has passed the test?

Yep true. This was helpful before I fixed the memory management of the 
client, though obviously still racy. No need for it now.

Thnaks,
Eddie
>
>>   
>> +	client = kzalloc(sizeof(*client), GFP_KERNEL);
>>   	if (!client)
>> -		return NULL;
>> +		return ERR_PTR(-ENOMEM);
>>   
>>   	client->occ = occ;
>>   	spin_lock_init(&client->lock);
>> @@ -158,8 +168,8 @@ static int occ_open(struct inode *inode, struct file *file)
>>   	struct occ *occ = to_occ(mdev);
>>   
>>   	client = occ_open_common(occ, file->f_flags);
>> -	if (!client)
>> -		return -ENOMEM;
>> +	if (IS_ERR(client))
>> +		return PTR_ERR(client);
>>   
>>   	file->private_data = client;
>>   
>> @@ -172,6 +182,7 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
>>   	int rc;
>>   	size_t bytes;
>>   	struct occ_xfr *xfr = &client->xfr;
>> +	struct occ *occ = client->occ;
>>   
>>   	if (len > OCC_SRAM_BYTES)
>>   		return -EINVAL;
>> @@ -204,7 +215,8 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
>>   					      test_bit(XFR_COMPLETE,
>>   						       &xfr->flags) ||
>>   					      test_bit(XFR_CANCELED,
>> -						       &xfr->flags));
>> +						       &xfr->flags) ||
>> +					      occ->cancel);
>>   
>>   		spin_lock_irq(&client->lock);
>>   
>> @@ -272,7 +284,7 @@ static ssize_t occ_write_common(struct occ_client *client,
>>   
>>   	spin_lock_irq(&client->lock);
>>   
>> -	if (test_and_set_bit(CLIENT_XFR_PENDING, &client->flags)) {
>> +	if (test_bit(CLIENT_XFR_PENDING, &client->flags)) {
>>   		rc = -EBUSY;
>>   		goto done;
>>   	}
>> @@ -310,8 +322,11 @@ static ssize_t occ_write_common(struct occ_client *client,
>>   	xfr->cmd_data_length = data_length + 6;
>>   	client->read_offset = 0;
>>   
>> -	occ_enqueue_xfr(xfr);
>> +	rc = occ_enqueue_xfr(xfr);
>> +	if (rc)
>> +		goto done;
>>   
>> +	set_bit(CLIENT_XFR_PENDING, &client->flags);
>>   	rc = len;
>>   
>>   done:
>> @@ -583,6 +598,9 @@ static void occ_worker(struct work_struct *work)
>>   	struct device *sbefifo = occ->sbefifo;
>>   
>>   again:
>> +	if (occ->cancel)
>> +		return;
>> +
>>   	spin_lock_irq(&occ->list_lock);
>>   
>>   	xfr = list_first_entry_or_null(&occ->xfrs, struct occ_xfr, link);
>> @@ -676,12 +694,17 @@ static void occ_worker(struct work_struct *work)
>>   
>>   struct occ_client *occ_drv_open(struct device *dev, unsigned long flags)
>>   {
>> +	struct occ_client *client;
>>   	struct occ *occ = dev_get_drvdata(dev);
>>   
>>   	if (!occ)
>>   		return NULL;
>>   
>> -	return occ_open_common(occ, flags);
>> +	client = occ_open_common(occ, flags);
>> +	if (IS_ERR(client))
>> +		return NULL;
>> +
>> +	return client;
>>   }
>>   EXPORT_SYMBOL_GPL(occ_drv_open);
>>   
>> @@ -752,23 +775,13 @@ static int occ_probe(struct platform_device *pdev)
>>   			if (occ->idx < 0)
>>   				occ->idx = ida_simple_get(&occ_ida, 1, INT_MAX,
>>   							  GFP_KERNEL);
>> -		} else
>> +		} else {
>>   			occ->idx = ida_simple_get(&occ_ida, 1, INT_MAX,
>>   						  GFP_KERNEL);
>> -
>> -		/* create platform devs for dts child nodes (hwmon, etc) */
>> -		for_each_child_of_node(dev->of_node, np) {
>> -			snprintf(child_name, sizeof(child_name), "occ%d-dev%d",
>> -				 occ->idx, child_idx++);
>> -			child = of_platform_device_create(np, child_name, dev);
>> -			if (!child)
>> -				dev_warn(dev,
>> -					 "failed to create child node dev\n");
>>   		}
>> -	} else
>> +	} else {
>>   		occ->idx = ida_simple_get(&occ_ida, 1, INT_MAX, GFP_KERNEL);
>> -
>> -	platform_set_drvdata(pdev, occ);
>> +	}
>>   
>>   	snprintf(occ->name, sizeof(occ->name), "occ%d", occ->idx);
>>   	occ->mdev.fops = &occ_fops;
>> @@ -778,20 +791,45 @@ static int occ_probe(struct platform_device *pdev)
>>   
>>   	rc = misc_register(&occ->mdev);
>>   	if (rc) {
>> -		dev_err(dev, "failed to register miscdevice\n");
>> +		dev_err(dev, "failed to register miscdevice: %d\n", rc);
>> +		ida_simple_remove(&occ_ida, occ->idx);
>>   		return rc;
>>   	}
>>   
>> +	/* create platform devs for dts child nodes (hwmon, etc) */
>> +	for_each_available_child_of_node(dev->of_node, np) {
>> +		snprintf(child_name, sizeof(child_name), "occ%d-dev%d",
>> +			 occ->idx, child_idx++);
>> +		child = of_platform_device_create(np, child_name, dev);
>> +		if (!child)
>> +			dev_warn(dev, "failed to create child node dev\n");
>> +	}
>> +
>> +	platform_set_drvdata(pdev, occ);
>> +
>>   	return 0;
>>   }
>>   
>>   static int occ_remove(struct platform_device *pdev)
>>   {
>>   	struct occ *occ = platform_get_drvdata(pdev);
>> +	struct occ_xfr *xfr;
>> +	struct occ_client *client;
>> +
>> +	occ->cancel = true;
>> +
>> +	spin_lock_irq(&occ->list_lock);
>> +	list_for_each_entry(xfr, &occ->xfrs, link) {
>> +		client = to_client(xfr);
>> +		wake_up_all(&client->wait);
>> +	}
>> +	spin_unlock_irq(&occ->list_lock);
>>   
>> -	flush_work(&occ->work);
>>   	misc_deregister(&occ->mdev);
>>   	device_for_each_child(&pdev->dev, NULL, occ_unregister_child);
>> +
>> +	cancel_work_sync(&occ->work);
>> +
>>   	ida_simple_remove(&occ_ida, occ->idx);
>>   
>>   	return 0;

Re: [PATCH linux dev-4.10 v3 06/31] drivers: fsi: sbefifo: remove redundant function

[Eddie James]

On 10/05/2017 04:48 PM, Andrew Jeffery wrote:
> On Thu, 2017-10-05 at 14:23 -0500, Eddie James wrote:
>> From: "Edward A. James" <eajames@us.ibm.com>
>>   
>> sbefifo_client_next_xfr function just does what list_first_entry_or_null
>> does.
> Any reason you didn't implement sbefifo_client_next_xfr() in terms of
> list_first_entry_or_null() rather than open-code it?

I prefer simplicity of using the kernel helper. With a custom function, 
it's not clear if it's doing some additional work unless you go look at 
it. With the list function, it's clear it's just returning the top entry.

Thanks for the review!
Eddie

>
>>   
>> Signed-off-by: Edward A. James <eajames@us.ibm.com>
>> ---
>>   drivers/fsi/fsi-sbefifo.c | 28 +++++++++++++++-------------
>>   1 file changed, 15 insertions(+), 13 deletions(-)
>>   
>> diff --git a/drivers/fsi/fsi-sbefifo.c b/drivers/fsi/fsi-sbefifo.c
>> index dbac0c3..173cd03 100644
>> --- a/drivers/fsi/fsi-sbefifo.c
>> +++ b/drivers/fsi/fsi-sbefifo.c
>> @@ -279,20 +279,12 @@ static struct sbefifo_xfr *sbefifo_enq_xfr(struct sbefifo_client *client)
>>   	return xfr;
>>   }
>>   
>> -static struct sbefifo_xfr *sbefifo_client_next_xfr(
>> -		struct sbefifo_client *client)
>> -{
>> -	if (list_empty(&client->xfrs))
>> -		return NULL;
>> -
>> -	return container_of(client->xfrs.next, struct sbefifo_xfr, client);
>> -}
>> -
>>   static bool sbefifo_xfr_rsp_pending(struct sbefifo_client *client)
>>   {
>> -	struct sbefifo_xfr *xfr;
>> +	struct sbefifo_xfr *xfr = list_first_entry_or_null(&client->xfrs,
>> +							   struct sbefifo_xfr,
>> +							   client);
>>   
>> -	xfr = sbefifo_client_next_xfr(client);
>>   	if (xfr && test_bit(SBEFIFO_XFR_RESP_PENDING, &xfr->flags))
>>   		return true;
>>   
>> @@ -595,7 +587,15 @@ static ssize_t sbefifo_read_common(struct sbefifo_client *client,
>>   	}
>>   
>>   	if (sbefifo_buf_readnb(&client->rbuf, n)) {
>> -		xfr = sbefifo_client_next_xfr(client);
>> +		xfr = list_first_entry_or_null(&client->xfrs,
>> +					       struct sbefifo_xfr, client);
>> +		if (!xfr) {
>> +			/* should be impossible to not have an xfr here */
>> +			WARN_ONCE(1, "no xfr in queue");
>> +			sbefifo_put_client(client);
>> +			return -EPROTO;
>> +		}
>> +
>>   		if (!test_bit(SBEFIFO_XFR_COMPLETE, &xfr->flags)) {
>>   			/*
>>   			 * Fill the read buffer back up.
>> @@ -632,7 +632,9 @@ static bool sbefifo_write_ready(struct sbefifo *sbefifo,
>>   				struct sbefifo_xfr *xfr,
>>   				struct sbefifo_client *client, size_t *n)
>>   {
>> -	struct sbefifo_xfr *next = sbefifo_client_next_xfr(client);
>> +	struct sbefifo_xfr *next = list_first_entry_or_null(&client->xfrs,
>> +							    struct sbefifo_xfr,
>> +							    client);
>>   
>>   	*n = sbefifo_buf_nbwriteable(&client->wbuf);
>>   	return READ_ONCE(sbefifo->rc) || (next == xfr && *n);

Re: [PATCH linux dev-4.10 v3 06/31] drivers: fsi: sbefifo: remove redundant function

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 3592 bytes --]

On Thu, 2017-10-05 at 21:03 -0500, Eddie James wrote:
> 
> On 10/05/2017 04:48 PM, Andrew Jeffery wrote:
> > On Thu, 2017-10-05 at 14:23 -0500, Eddie James wrote:
> > > > > > From: "Edward A. James" <eajames@us.ibm.com>
> > >   
> > > sbefifo_client_next_xfr function just does what list_first_entry_or_null
> > > does.
> > 
> > Any reason you didn't implement sbefifo_client_next_xfr() in terms of
> > list_first_entry_or_null() rather than open-code it?
> 
> I prefer simplicity of using the kernel helper. With a custom function, 
> it's not clear if it's doing some additional work unless you go look at 
> it. With the list function, it's clear it's just returning the top entry.

Sure. I'm not terribly fussed either way, was just interested.

Reviewed-by: Andrew Jeffery <andrew@aj.id.au>

> 
> Thanks for the review!
> Eddie
> 
> > 
> > >   
> > > > > > Signed-off-by: Edward A. James <eajames@us.ibm.com>
> > > ---
> > >   drivers/fsi/fsi-sbefifo.c | 28 +++++++++++++++-------------
> > >   1 file changed, 15 insertions(+), 13 deletions(-)
> > >   
> > > diff --git a/drivers/fsi/fsi-sbefifo.c b/drivers/fsi/fsi-sbefifo.c
> > > index dbac0c3..173cd03 100644
> > > --- a/drivers/fsi/fsi-sbefifo.c
> > > +++ b/drivers/fsi/fsi-sbefifo.c
> > > @@ -279,20 +279,12 @@ static struct sbefifo_xfr *sbefifo_enq_xfr(struct sbefifo_client *client)
> > > > > >   	return xfr;
> > >   }
> > >   
> > > -static struct sbefifo_xfr *sbefifo_client_next_xfr(
> > > > > > -		struct sbefifo_client *client)
> > > -{
> > > > > > -	if (list_empty(&client->xfrs))
> > > > > > -		return NULL;
> > > -
> > > > > > -	return container_of(client->xfrs.next, struct sbefifo_xfr, client);
> > > -}
> > > -
> > >   static bool sbefifo_xfr_rsp_pending(struct sbefifo_client *client)
> > >   {
> > > > > > -	struct sbefifo_xfr *xfr;
> > > > > > +	struct sbefifo_xfr *xfr = list_first_entry_or_null(&client->xfrs,
> > > > > > +							   struct sbefifo_xfr,
> > > > > > +							   client);
> > >   
> > > > > > -	xfr = sbefifo_client_next_xfr(client);
> > > > > >   	if (xfr && test_bit(SBEFIFO_XFR_RESP_PENDING, &xfr->flags))
> > > > > >   		return true;
> > >   
> > > @@ -595,7 +587,15 @@ static ssize_t sbefifo_read_common(struct sbefifo_client *client,
> > > > > >   	}
> > >   
> > > > > >   	if (sbefifo_buf_readnb(&client->rbuf, n)) {
> > > > > > -		xfr = sbefifo_client_next_xfr(client);
> > > > > > +		xfr = list_first_entry_or_null(&client->xfrs,
> > > > > > +					       struct sbefifo_xfr, client);
> > > > > > +		if (!xfr) {
> > > > > > +			/* should be impossible to not have an xfr here */
> > > > > > +			WARN_ONCE(1, "no xfr in queue");
> > > > > > +			sbefifo_put_client(client);
> > > > > > +			return -EPROTO;
> > > > > > +		}
> > > +
> > > > > >   		if (!test_bit(SBEFIFO_XFR_COMPLETE, &xfr->flags)) {
> > > > > >   			/*
> > > > > >   			 * Fill the read buffer back up.
> > > @@ -632,7 +632,9 @@ static bool sbefifo_write_ready(struct sbefifo *sbefifo,
> > > > > >   				struct sbefifo_xfr *xfr,
> > > > > >   				struct sbefifo_client *client, size_t *n)
> > >   {
> > > > > > -	struct sbefifo_xfr *next = sbefifo_client_next_xfr(client);
> > > > > > +	struct sbefifo_xfr *next = list_first_entry_or_null(&client->xfrs,
> > > > > > +							    struct sbefifo_xfr,
> > > > > > +							    client);
> > >   
> > > > > >   	*n = sbefifo_buf_nbwriteable(&client->wbuf);
> > >   	return READ_ONCE(sbefifo->rc) || (next == xfr && *n);
> 
> 

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]