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

[PATCH linux dev-4.10 v4 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.

Apologies for spamming the list with so many patches...

Changes since v3:
 * switch checks for OCC client NULL to the common read/write/close functions.
 * add helper function for ugly wait_event_interruptible in OCC read().
 * removed useless check for cancel in occ open().

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           | 345 ++++++++++++++++++++++++++------------------
 drivers/hwmon/occ/p9_sbe.c  |  47 +++---
 include/linux/fsi-sbefifo.h |   6 +-
 include/linux/occ.h         |  20 ++-
 5 files changed, 426 insertions(+), 303 deletions(-)

-- 
1.8.3.1

[PATCH linux dev-4.10 v4 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>
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
-- 
1.8.3.1

[PATCH linux dev-4.10 v4 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>
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;
 		}
-- 
1.8.3.1

[PATCH linux dev-4.10 v4 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>
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,
-- 
1.8.3.1

[PATCH linux dev-4.10 v4 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>
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");
 		}
 	}
-- 
1.8.3.1

[PATCH linux dev-4.10 v4 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>
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(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 v4 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

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

[Andrew Jeffery]

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

On Thu, 2017-10-05 at 21:05 -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.
> 
> Signed-off-by: Edward A. James <eajames@us.ibm.com>

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

> ---
>  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 --]

[PATCH linux dev-4.10 v4 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>
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;
 }
 
-- 
1.8.3.1

[PATCH linux dev-4.10 v4 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>
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.
-- 
1.8.3.1

[PATCH linux dev-4.10 v4 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>
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);
 }
 
-- 
1.8.3.1

[PATCH linux dev-4.10 v4 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>
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);
-- 
1.8.3.1

[PATCH linux dev-4.10 v4 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>
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");
-- 
1.8.3.1

[PATCH linux dev-4.10 v4 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>
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 */
-- 
1.8.3.1

[PATCH linux dev-4.10 v4 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>
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);
 }
 
-- 
1.8.3.1

[PATCH linux dev-4.10 v4 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>
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,
-- 
1.8.3.1

[PATCH linux dev-4.10 v4 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>
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>
-- 
1.8.3.1

[PATCH linux dev-4.10 v4 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>
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;
 	}
-- 
1.8.3.1

[PATCH linux dev-4.10 v4 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>
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;
-- 
1.8.3.1

[PATCH linux dev-4.10 v4 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>
Reviewed-by: Andrew Jeffery <andrew@aj.id.au>
---
 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 v4 19/31] drivers: fsi: occ: Return ENODEV if client is NULL

[Eddie James]

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

Safeguard the access functions.

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

diff --git a/drivers/fsi/occ.c b/drivers/fsi/occ.c
index 55f293d..322af5a 100644
--- a/drivers/fsi/occ.c
+++ b/drivers/fsi/occ.c
@@ -168,11 +168,16 @@ 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_xfr *xfr;
+
+	if (!client)
+		return -ENODEV;
 
 	if (len > OCC_SRAM_BYTES)
 		return -EINVAL;
 
+	xfr = &client->xfr;
+
 	spin_lock_irq(&client->lock);
 
 	if (!test_bit(CLIENT_XFR_PENDING, &client->flags)) {
@@ -263,11 +268,16 @@ static ssize_t occ_write_common(struct occ_client *client,
 	int rc;
 	unsigned int i;
 	u16 data_length, checksum = 0;
-	struct occ_xfr *xfr = &client->xfr;
+	struct occ_xfr *xfr;
+
+	if (!client)
+		return -ENODEV;
 
 	if (len > (OCC_CMD_DATA_BYTES + 3) || len < 3)
 		return -EINVAL;
 
+	xfr = &client->xfr;
+
 	spin_lock_irq(&client->lock);
 	if (test_and_set_bit(CLIENT_XFR_PENDING, &client->flags)) {
 		rc = -EBUSY;
@@ -327,8 +337,14 @@ static ssize_t occ_write(struct file *file, const char __user *buf,
 
 static int occ_release_common(struct occ_client *client)
 {
-	struct occ_xfr *xfr = &client->xfr;
-	struct occ *occ = client->occ;
+	struct occ *occ;
+	struct occ_xfr *xfr;
+
+	if (!client)
+		return -ENODEV;
+
+	xfr = &client->xfr;
+	occ = client->occ;
 
 	spin_lock_irq(&client->lock);
 
-- 
1.8.3.1

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

[Andrew Jeffery]

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

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

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

> ---
>  drivers/fsi/occ.c | 24 ++++++++++++++++++++----
>  1 file changed, 20 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/fsi/occ.c b/drivers/fsi/occ.c
> index 55f293d..322af5a 100644
> --- a/drivers/fsi/occ.c
> +++ b/drivers/fsi/occ.c
> @@ -168,11 +168,16 @@ 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_xfr *xfr;
> +
> +	if (!client)
> +		return -ENODEV;
>  
>  	if (len > OCC_SRAM_BYTES)
>  		return -EINVAL;
>  
> +	xfr = &client->xfr;
> +
>  	spin_lock_irq(&client->lock);
>  
>  	if (!test_bit(CLIENT_XFR_PENDING, &client->flags)) {
> @@ -263,11 +268,16 @@ static ssize_t occ_write_common(struct occ_client *client,
>  	int rc;
>  	unsigned int i;
>  	u16 data_length, checksum = 0;
> -	struct occ_xfr *xfr = &client->xfr;
> +	struct occ_xfr *xfr;
> +
> +	if (!client)
> +		return -ENODEV;
>  
>  	if (len > (OCC_CMD_DATA_BYTES + 3) || len < 3)
>  		return -EINVAL;
>  
> +	xfr = &client->xfr;
> +
>  	spin_lock_irq(&client->lock);
>  	if (test_and_set_bit(CLIENT_XFR_PENDING, &client->flags)) {
>  		rc = -EBUSY;
> @@ -327,8 +337,14 @@ static ssize_t occ_write(struct file *file, const char __user *buf,
>  
>  static int occ_release_common(struct occ_client *client)
>  {
> -	struct occ_xfr *xfr = &client->xfr;
> -	struct occ *occ = client->occ;
> +	struct occ *occ;
> +	struct occ_xfr *xfr;
> +
> +	if (!client)
> +		return -ENODEV;
> +
> +	xfr = &client->xfr;
> +	occ = client->occ;
>  
>  	spin_lock_irq(&client->lock);
>  

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

[PATCH linux dev-4.10 v4 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>
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 322af5a..7e2b770 100644
--- a/drivers/fsi/occ.c
+++ b/drivers/fsi/occ.c
@@ -252,12 +252,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);
 }
 
@@ -326,12 +320,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 v4 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>
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 7e2b770..c2fc14c 100644
--- a/drivers/fsi/occ.c
+++ b/drivers/fsi/occ.c
@@ -402,7 +402,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,
@@ -421,7 +421,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,
@@ -466,7 +466,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);
@@ -514,7 +514,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);
@@ -551,7 +551,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);
@@ -599,7 +599,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 v4 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 | 28 ++++++++++++++++++----------
 1 file changed, 18 insertions(+), 10 deletions(-)

diff --git a/drivers/fsi/occ.c b/drivers/fsi/occ.c
index c2fc14c..6e20bb3 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;
 
@@ -163,6 +162,12 @@ static int occ_open(struct inode *inode, struct file *file)
 	return 0;
 }
 
+static inline bool occ_read_ready(struct occ_xfr *xfr)
+{
+	return test_bit(XFR_COMPLETE, &xfr->flags) ||
+		test_bit(XFR_CANCELED, &xfr->flags);
+}
+
 static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
 			       char *kbuf, size_t len)
 {
@@ -185,8 +190,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;
 	}
@@ -202,8 +208,7 @@ 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));
+					      occ_read_ready(xfr));
 
 		spin_lock_irq(&client->lock);
 
@@ -227,12 +232,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;
 
@@ -273,6 +280,7 @@ static ssize_t occ_write_common(struct occ_client *client,
 	xfr = &client->xfr;
 
 	spin_lock_irq(&client->lock);
+
 	if (test_and_set_bit(CLIENT_XFR_PENDING, &client->flags)) {
 		rc = -EBUSY;
 		goto done;
@@ -280,7 +288,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) {
@@ -288,8 +295,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) {
@@ -446,7 +454,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

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

[Andrew Jeffery]

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

On Thu, 2017-10-05 at 21:05 -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>

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

> ---
>  drivers/fsi/occ.c | 28 ++++++++++++++++++----------
>  1 file changed, 18 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/fsi/occ.c b/drivers/fsi/occ.c
> index c2fc14c..6e20bb3 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;
>  
> @@ -163,6 +162,12 @@ static int occ_open(struct inode *inode, struct file *file)
>  	return 0;
>  }
>  
> +static inline bool occ_read_ready(struct occ_xfr *xfr)
> +{
> +	return test_bit(XFR_COMPLETE, &xfr->flags) ||
> +		test_bit(XFR_CANCELED, &xfr->flags);
> +}
> +
>  static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
>  			       char *kbuf, size_t len)
>  {
> @@ -185,8 +190,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;
>  	}
> @@ -202,8 +208,7 @@ 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));
> +					      occ_read_ready(xfr));
>  
>  		spin_lock_irq(&client->lock);
>  
> @@ -227,12 +232,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;
>  
> @@ -273,6 +280,7 @@ static ssize_t occ_write_common(struct occ_client *client,
>  	xfr = &client->xfr;
>  
>  	spin_lock_irq(&client->lock);
> +
>  	if (test_and_set_bit(CLIENT_XFR_PENDING, &client->flags)) {
>  		rc = -EBUSY;
>  		goto done;
> @@ -280,7 +288,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) {
> @@ -288,8 +295,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) {
> @@ -446,7 +454,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 --]

[PATCH linux dev-4.10 v4 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>
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 6e20bb3..6eba40b 100644
--- a/drivers/fsi/occ.c
+++ b/drivers/fsi/occ.c
@@ -798,6 +798,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 v4 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>
Reviewed-by: Andrew Jeffery <andrew@aj.id.au>
---
 drivers/fsi/occ.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/fsi/occ.c b/drivers/fsi/occ.c
index 6eba40b..5590a72 100644
--- a/drivers/fsi/occ.c
+++ b/drivers/fsi/occ.c
@@ -713,9 +713,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 v4 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>
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 5590a72..3e27307 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;
@@ -286,10 +286,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;
@@ -373,7 +378,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;
@@ -438,9 +443,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);
@@ -489,7 +498,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;
 
@@ -497,6 +506,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);
@@ -537,11 +550,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);
@@ -592,6 +609,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)
@@ -601,6 +619,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 v4 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>
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 */
-- 
1.8.3.1

[PATCH linux dev-4.10 v4 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>
Acked-by: Andrew Jeffery <andrew@aj.id.au>
---
 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 3e27307..bd0ad98 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];
@@ -588,6 +592,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;
@@ -609,6 +616,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);
@@ -620,9 +629,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 v4 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 | 71 +++++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 50 insertions(+), 21 deletions(-)

diff --git a/drivers/fsi/occ.c b/drivers/fsi/occ.c
index bd0ad98..ec42fc0 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,6 +136,8 @@ 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)
@@ -166,10 +172,10 @@ static int occ_open(struct inode *inode, struct file *file)
 	return 0;
 }
 
-static inline bool occ_read_ready(struct occ_xfr *xfr)
+static inline bool occ_read_ready(struct occ_xfr *xfr, struct occ *occ)
 {
 	return test_bit(XFR_COMPLETE, &xfr->flags) ||
-		test_bit(XFR_CANCELED, &xfr->flags);
+		test_bit(XFR_CANCELED, &xfr->flags) || occ->cancel;
 }
 
 static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
@@ -178,6 +184,7 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
 	int rc;
 	size_t bytes;
 	struct occ_xfr *xfr;
+	struct occ *occ;
 
 	if (!client)
 		return -ENODEV;
@@ -186,6 +193,7 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
 		return -EINVAL;
 
 	xfr = &client->xfr;
+	occ = client->occ;
 
 	spin_lock_irq(&client->lock);
 
@@ -212,7 +220,7 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
 		spin_unlock_irq(&client->lock);
 
 		rc = wait_event_interruptible(client->wait,
-					      occ_read_ready(xfr));
+					      occ_read_ready(xfr, occ));
 
 		spin_lock_irq(&client->lock);
 
@@ -285,7 +293,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;
 	}
@@ -323,8 +331,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:
@@ -602,6 +613,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);
@@ -762,23 +776,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;
@@ -788,20 +792,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

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

[Andrew Jeffery]

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

On Thu, 2017-10-05 at 21:05 -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>

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

> ---
>  drivers/fsi/occ.c | 71 +++++++++++++++++++++++++++++++++++++++----------------
>  1 file changed, 50 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/fsi/occ.c b/drivers/fsi/occ.c
> index bd0ad98..ec42fc0 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,6 +136,8 @@ 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)
> @@ -166,10 +172,10 @@ static int occ_open(struct inode *inode, struct file *file)
>  	return 0;
>  }
>  
> -static inline bool occ_read_ready(struct occ_xfr *xfr)
> +static inline bool occ_read_ready(struct occ_xfr *xfr, struct occ *occ)
>  {
>  	return test_bit(XFR_COMPLETE, &xfr->flags) ||
> -		test_bit(XFR_CANCELED, &xfr->flags);
> +		test_bit(XFR_CANCELED, &xfr->flags) || occ->cancel;
>  }
>  
>  static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
> @@ -178,6 +184,7 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
>  	int rc;
>  	size_t bytes;
>  	struct occ_xfr *xfr;
> +	struct occ *occ;
>  
>  	if (!client)
>  		return -ENODEV;
> @@ -186,6 +193,7 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
>  		return -EINVAL;
>  
>  	xfr = &client->xfr;
> +	occ = client->occ;
>  
>  	spin_lock_irq(&client->lock);
>  
> @@ -212,7 +220,7 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
>  		spin_unlock_irq(&client->lock);
>  
>  		rc = wait_event_interruptible(client->wait,
> -					      occ_read_ready(xfr));
> +					      occ_read_ready(xfr, occ));
>  
>  		spin_lock_irq(&client->lock);
>  
> @@ -285,7 +293,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;
>  	}
> @@ -323,8 +331,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:
> @@ -602,6 +613,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);
> @@ -762,23 +776,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;
> @@ -788,20 +792,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 v4 28/31] drivers: fsi: occ: Add cancel to remove() and fix probe()

[Brad Bishop]

> On Oct 5, 2017, at 10:05 PM, Eddie James <eajames@linux.vnet.ibm.com> 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 | 71 +++++++++++++++++++++++++++++++++++++++----------------
> 1 file changed, 50 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/fsi/occ.c b/drivers/fsi/occ.c
> index bd0ad98..ec42fc0 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;

Can we have ENODEV here?  That seems to be typical for this scenario and
ECANCELED seems to be for canceled AIO operations.

> +
> 	spin_lock_irq(&occ->list_lock);
> 
> 	empty = list_empty(&occ->xfrs);
> @@ -132,6 +136,8 @@ 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)
> @@ -166,10 +172,10 @@ static int occ_open(struct inode *inode, struct file *file)
> 	return 0;
> }
> 
> -static inline bool occ_read_ready(struct occ_xfr *xfr)
> +static inline bool occ_read_ready(struct occ_xfr *xfr, struct occ *occ)
> {
> 	return test_bit(XFR_COMPLETE, &xfr->flags) ||
> -		test_bit(XFR_CANCELED, &xfr->flags);
> +		test_bit(XFR_CANCELED, &xfr->flags) || occ->cancel;
> }
> 
> static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
> @@ -178,6 +184,7 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
> 	int rc;
> 	size_t bytes;
> 	struct occ_xfr *xfr;
> +	struct occ *occ;
> 
> 	if (!client)
> 		return -ENODEV;
> @@ -186,6 +193,7 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
> 		return -EINVAL;
> 
> 	xfr = &client->xfr;
> +	occ = client->occ;
> 
> 	spin_lock_irq(&client->lock);
> 
> @@ -212,7 +220,7 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
> 		spin_unlock_irq(&client->lock);
> 
> 		rc = wait_event_interruptible(client->wait,
> -					      occ_read_ready(xfr));
> +					      occ_read_ready(xfr, occ));
> 
> 		spin_lock_irq(&client->lock);
> 
> @@ -285,7 +293,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;
> 	}
> @@ -323,8 +331,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:
> @@ -602,6 +613,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);
> @@ -762,23 +776,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;
> @@ -788,20 +792,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

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

[Joel Stanley]

On Mon, Oct 9, 2017 at 10:35 AM, Brad Bishop
<bradleyb@fuzziesquirrel.com> wrote:
>
>> On Oct 5, 2017, at 10:05 PM, Eddie James <eajames@linux.vnet.ibm.com> wrote:
>> -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;
>
> Can we have ENODEV here?  That seems to be typical for this scenario and
> ECANCELED seems to be for canceled AIO operations.

I agree.

Eddie, I will fold this change in when I merge if you're okay with it?

Cheers,

Joel

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

[Eddie James]

On 10/08/2017 08:53 PM, Joel Stanley wrote:
> On Mon, Oct 9, 2017 at 10:35 AM, Brad Bishop
> <bradleyb@fuzziesquirrel.com> wrote:
>>> On Oct 5, 2017, at 10:05 PM, Eddie James <eajames@linux.vnet.ibm.com> wrote:
>>> -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;
>> Can we have ENODEV here?  That seems to be typical for this scenario and
>> ECANCELED seems to be for canceled AIO operations.
> I agree.
>
> Eddie, I will fold this change in when I merge if you're okay with it?

Sounds good! Thanks
Eddie

>
> Cheers,
>
> Joel
>

[PATCH linux dev-4.10 v4 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 ec42fc0..800e8b6 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 = kzalloc(sizeof(*client), GFP_KERNEL);
@@ -148,6 +163,7 @@ static struct occ_client *occ_open_common(struct occ *occ, unsigned long flags)
 		return NULL;
 
 	client->occ = occ;
+	kref_init(&client->kref);
 	spin_lock_init(&client->lock);
 	init_waitqueue_head(&client->wait);
 
@@ -192,6 +208,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);
 	xfr = &client->xfr;
 	occ = client->occ;
 
@@ -215,8 +232,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,
@@ -224,15 +239,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;
 		}
 	}
@@ -263,6 +275,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;
 }
 
@@ -289,6 +302,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);
 	xfr = &client->xfr;
 
 	spin_lock_irq(&client->lock);
@@ -340,6 +354,7 @@ static ssize_t occ_write_common(struct occ_client *client,
 
 done:
 	spin_unlock_irq(&client->lock);
+	occ_put_client(client);
 	return rc;
 }
 
@@ -364,38 +379,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;
 }
 
@@ -601,7 +604,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);
@@ -624,6 +627,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);
 
@@ -679,29 +684,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

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

[Andrew Jeffery]

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

On Thu, 2017-10-05 at 21:05 -0500, Eddie James wrote:
> 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>

This is hairy, but given the current design I understand the need. I feel that
ideas similar to I have for the SBEFIFO might be able to clean it up. 

For the moment:

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

> ---
>  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 ec42fc0..800e8b6 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 = kzalloc(sizeof(*client), GFP_KERNEL);
> @@ -148,6 +163,7 @@ static struct occ_client *occ_open_common(struct occ *occ, unsigned long flags)
>  		return NULL;
>  
>  	client->occ = occ;
> +	kref_init(&client->kref);
>  	spin_lock_init(&client->lock);
>  	init_waitqueue_head(&client->wait);
>  
> @@ -192,6 +208,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);
>  	xfr = &client->xfr;
>  	occ = client->occ;
>  
> @@ -215,8 +232,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,
> @@ -224,15 +239,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;
>  		}
>  	}
> @@ -263,6 +275,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;
>  }
>  
> @@ -289,6 +302,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);
>  	xfr = &client->xfr;
>  
>  	spin_lock_irq(&client->lock);
> @@ -340,6 +354,7 @@ static ssize_t occ_write_common(struct occ_client *client,
>  
>  done:
>  	spin_unlock_irq(&client->lock);
> +	occ_put_client(client);
>  	return rc;
>  }
>  
> @@ -364,38 +379,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;
>  }
>  
> @@ -601,7 +604,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);
> @@ -624,6 +627,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);
>  
> @@ -679,29 +684,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;

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

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

[Eddie James]

On 10/05/2017 09:05 PM, Eddie James wrote:
> 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 ec42fc0..800e8b6 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 = kzalloc(sizeof(*client), GFP_KERNEL);
> @@ -148,6 +163,7 @@ static struct occ_client *occ_open_common(struct occ *occ, unsigned long flags)
>   		return NULL;
>
>   	client->occ = occ;
> +	kref_init(&client->kref);
>   	spin_lock_init(&client->lock);
>   	init_waitqueue_head(&client->wait);
>
> @@ -192,6 +208,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);
>   	xfr = &client->xfr;
>   	occ = client->occ;
>
> @@ -215,8 +232,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,
> @@ -224,15 +239,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;

We probably need this ENODEV too. We get here if the client is released 
or the device is removed while we're waiting for a read to complete.

> +			else
> +				rc = -EINTR;
> +
>   			goto done;
>   		}
>   	}
> @@ -263,6 +275,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;
>   }
>
> @@ -289,6 +302,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);
>   	xfr = &client->xfr;
>
>   	spin_lock_irq(&client->lock);
> @@ -340,6 +354,7 @@ static ssize_t occ_write_common(struct occ_client *client,
>
>   done:
>   	spin_unlock_irq(&client->lock);
> +	occ_put_client(client);
>   	return rc;
>   }
>
> @@ -364,38 +379,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;
>   }
>
> @@ -601,7 +604,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);
> @@ -624,6 +627,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);
>
> @@ -679,29 +684,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;

[PATCH linux dev-4.10 v4 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>
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);
-- 
1.8.3.1

[PATCH linux dev-4.10 v4 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 v4 31/31] drivers: hwmon: occ: Cancel occ operations in remove()

[Andrew Jeffery]

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

On Thu, 2017-10-05 at 21:05 -0500, Eddie James wrote:
> 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;

Can you please explain the semantics of this lock in a comment or in
the commit message? Who should hold it, when and why, and in what order
with respect to other locks if applicable?

We've discussed it in private previously but I admit the details have
escaped me, and the way it gets used in this patch looks pretty
strange.

Andrew


>  };
>  
>  #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);

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