[PATCH linux dev-4.10 v2 0/9] drivers: fsi: client fixes and refactor

[PATCH linux dev-4.10 v2 0/9] drivers: fsi: client fixes and refactor

[Eddie James]

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

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

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

Edward A. James (9):
  drivers: fsi: SBEFIFO: General clean-up
  drivers: fsi: SBEFIFO: Fix probe() and remove()
  drivers: fsi: SBEFIFO: check for xfr complete in read wait_event
  drivers: fsi: occ: General clean-up
  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   | 321 +++++++++++++++++++++++-------------------
 drivers/fsi/occ.c           | 336 ++++++++++++++++++++++++++------------------
 drivers/hwmon/occ/p9_sbe.c  |  47 ++++---
 include/linux/fsi-sbefifo.h |   6 +-
 include/linux/occ.h         |  20 ++-
 5 files changed, 421 insertions(+), 309 deletions(-)

-- 
1.8.3.1

[PATCH linux dev-4.10 v2 1/9] drivers: fsi: SBEFIFO: General clean-up

[Eddie James]

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

Correct formatting and improve the flow a bit. Remove warnings for
offset in read/write functions, and remove the user pointer verification
since the copy_from/to_user will do that anyway. Correct the include
guards in the header file.

Signed-off-by: Edward A. James <eajames@us.ibm.com>
---
 drivers/fsi/fsi-sbefifo.c   | 207 +++++++++++++++++++++++++-------------------
 include/linux/fsi-sbefifo.h |   6 +-
 2 files changed, 119 insertions(+), 94 deletions(-)

diff --git a/drivers/fsi/fsi-sbefifo.c b/drivers/fsi/fsi-sbefifo.c
index 1c37ff7..a4cd353 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
@@ -44,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;
@@ -94,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));
@@ -102,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,
@@ -136,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)
@@ -210,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;
 }
 
@@ -229,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);
 }
 
@@ -267,21 +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;
 
@@ -349,6 +352,7 @@ static struct sbefifo_xfr *sbefifo_next_xfr(struct sbefifo *sbefifo)
 			kfree(xfr);
 			continue;
 		}
+
 		return xfr;
 	}
 
@@ -370,7 +374,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;
 
@@ -388,8 +392,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;
 
@@ -397,7 +400,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;
 		}
@@ -414,9 +417,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;
 
@@ -438,7 +440,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;
 		}
@@ -466,7 +468,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;
 		}
@@ -476,7 +478,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);
@@ -497,7 +499,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;
 
@@ -535,6 +537,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)
 {
@@ -551,30 +563,37 @@ 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_put_client(client);
-		return -ERESTARTSYS;
+				     sbefifo_read_ready(sbefifo, client,
+							&n, &ret))) {
+		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
+	} else {
 		memcpy(kbuf, READ_ONCE(client->rbuf.rpos), n);
+	}
 
 	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");
+			goto out;
+		}
+
 		if (!test_bit(SBEFIFO_XFR_COMPLETE, &xfr->flags)) {
 			/*
 			 * Fill the read buffer back up.
@@ -589,22 +608,31 @@ 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, 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;
 
-	WARN_ON(*offset);
+	return sbefifo_read_common(client, buf, NULL, len);
+}
 
-	if (!access_ok(VERIFY_WRITE, buf, len))
-		return -EFAULT;
+static bool sbefifo_write_ready(struct sbefifo *sbefifo,
+				struct sbefifo_xfr *xfr,
+				struct sbefifo_client *client, size_t *n)
+{
+	struct sbefifo_xfr *next = list_first_entry_or_null(&client->xfrs,
+							    struct sbefifo_xfr,
+							    client);
 
-	return sbefifo_read_common(client, buf, NULL, len);
+	*n = sbefifo_buf_nbwriteable(&client->wbuf);
+	return READ_ONCE(sbefifo->rc) || (next == xfr && *n);
 }
 
 static ssize_t sbefifo_write_common(struct sbefifo_client *client,
@@ -612,6 +640,7 @@ static ssize_t sbefifo_write_common(struct sbefifo_client *client,
 				    size_t len)
 {
 	struct sbefifo *sbefifo = client->dev;
+	struct sbefifo_buf *wbuf = &client->wbuf;
 	struct sbefifo_xfr *xfr;
 	ssize_t ret = 0;
 	size_t n;
@@ -622,24 +651,26 @@ static ssize_t sbefifo_write_common(struct sbefifo_client *client,
 	if (!len)
 		return 0;
 
-	n = sbefifo_buf_nbwriteable(&client->wbuf);
+	sbefifo_get_client(client);
+	n = sbefifo_buf_nbwriteable(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);
-		return -EAGAIN;
+		ret = -EAGAIN;
+		goto out;
 	}
 
-	xfr = sbefifo_enq_xfr(client);
+	xfr = sbefifo_enq_xfr(client);		/* this xfr queued up */
 	if (!xfr) {
 		spin_unlock_irq(&sbefifo->lock);
-		return -ENOMEM;
+		ret = PTR_ERR(xfr);
+		goto out;
 	}
-	spin_unlock_irq(&sbefifo->lock);
 
-	sbefifo_get_client(client);
+	spin_unlock_irq(&sbefifo->lock);
 
 	/*
 	 * Partial writes are not really allowed in that EOT is sent exactly
@@ -647,49 +678,47 @@ 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))
 				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);
 
 		if (ubuf) {
-			if (copy_from_user(READ_ONCE(client->wbuf.wpos), ubuf,
-			    n)) {
+			if (copy_from_user(READ_ONCE(wbuf->wpos), ubuf, n)) {
 				set_bit(SBEFIFO_XFR_CANCEL, &xfr->flags);
 				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;
 		} else {
-			memcpy(READ_ONCE(client->wbuf.wpos), kbuf, n);
+			memcpy(READ_ONCE(wbuf->wpos), kbuf, n);
 			kbuf += n;
 		}
 
-		sbefifo_buf_wrotenb(&client->wbuf, n);
+		sbefifo_buf_wrotenb(wbuf, n);
 		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);
@@ -702,21 +731,16 @@ static ssize_t sbefifo_write_common(struct sbefifo_client *client,
 			sbefifo_put(sbefifo);
 	}
 
+out:
 	sbefifo_put_client(client);
-
 	return ret;
 }
 
 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;
 
-	WARN_ON(*offset);
-
-	if (!access_ok(VERIFY_READ, buf, len))
-		return -EFAULT;
-
 	return sbefifo_write_common(client, buf, NULL, len);
 }
 
@@ -802,29 +826,27 @@ static int sbefifo_probe(struct device *dev)
 	u32 sts;
 	int ret, child_idx = 0;
 
-	dev_dbg(dev, "Found sbefifo device\n");
 	sbefifo = kzalloc(sizeof(*sbefifo), GFP_KERNEL);
 	if (!sbefifo)
 		return -ENOMEM;
 
 	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;
 	}
 
@@ -837,13 +859,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) */
@@ -852,7 +874,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");
 		}
 	}
@@ -922,10 +944,13 @@ static int sbefifo_init(void)
 static void sbefifo_exit(void)
 {
 	fsi_driver_unregister(&sbefifo_drv);
+
+	ida_destroy(&sbefifo_ida);
 }
 
 module_init(sbefifo_init);
 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");
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

Re: [PATCH linux dev-4.10 v2 1/9] drivers: fsi: SBEFIFO: General clean-up

[Andrew Jeffery]

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

On Fri, 2017-09-29 at 17:41 -0500, Eddie James wrote:
> From: "Edward A. James" <eajames@us.ibm.com>
> 
> Correct formatting and improve the flow a bit. Remove warnings for
> offset in read/write functions, and remove the user pointer verification
> since the copy_from/to_user will do that anyway. Correct the include
> guards in the header file.

This is similar to what I complained about last time, just without bullet
points in the description.

I tried to read the diff but really struggled to make sense of it. Keeping
track of the added/removed/changed functions - amongst formatting and control
flow changes - to decide whether they were appropriate is just *really*
difficult, and I can't say with confidence that as a whole this patch is okay.

So, apologies, but I don't plan on acking this as it stands, and I don't think
it should be submitted unless several people are ready to weld Tested-by tags
to it.

Andrew

> 
> Signed-off-by: Edward A. James <eajames@us.ibm.com>
> ---
>  drivers/fsi/fsi-sbefifo.c   | 207 +++++++++++++++++++++++++-------------------
>  include/linux/fsi-sbefifo.h |   6 +-
>  2 files changed, 119 insertions(+), 94 deletions(-)
> 
> diff --git a/drivers/fsi/fsi-sbefifo.c b/drivers/fsi/fsi-sbefifo.c
> index 1c37ff7..a4cd353 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
> @@ -44,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;
> @@ -94,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));
> @@ -102,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,
> @@ -136,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)
> @@ -210,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;
>  }
>  
> @@ -229,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);
>  }
>  
> @@ -267,21 +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;
>  
> @@ -349,6 +352,7 @@ static struct sbefifo_xfr *sbefifo_next_xfr(struct sbefifo *sbefifo)
>  			kfree(xfr);
>  			continue;
>  		}
> +
>  		return xfr;
>  	}
>  
> @@ -370,7 +374,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;
>  
> @@ -388,8 +392,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;
>  
> @@ -397,7 +400,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;
>  		}
> @@ -414,9 +417,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;
>  
> @@ -438,7 +440,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;
>  		}
> @@ -466,7 +468,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;
>  		}
> @@ -476,7 +478,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);
> @@ -497,7 +499,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;
>  
> @@ -535,6 +537,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)
>  {
> @@ -551,30 +563,37 @@ 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_put_client(client);
> -		return -ERESTARTSYS;
> +				     sbefifo_read_ready(sbefifo, client,
> +							&n, &ret))) {
> +		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
> +	} else {
>  		memcpy(kbuf, READ_ONCE(client->rbuf.rpos), n);
> +	}
>  
>  	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");
> +			goto out;
> +		}
> +
>  		if (!test_bit(SBEFIFO_XFR_COMPLETE, &xfr->flags)) {
>  			/*
>  			 * Fill the read buffer back up.
> @@ -589,22 +608,31 @@ 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, 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;
>  
> -	WARN_ON(*offset);
> +	return sbefifo_read_common(client, buf, NULL, len);
> +}
>  
> -	if (!access_ok(VERIFY_WRITE, buf, len))
> -		return -EFAULT;
> +static bool sbefifo_write_ready(struct sbefifo *sbefifo,
> +				struct sbefifo_xfr *xfr,
> +				struct sbefifo_client *client, size_t *n)
> +{
> +	struct sbefifo_xfr *next = list_first_entry_or_null(&client->xfrs,
> +							    struct sbefifo_xfr,
> +							    client);
>  
> -	return sbefifo_read_common(client, buf, NULL, len);
> +	*n = sbefifo_buf_nbwriteable(&client->wbuf);
> +	return READ_ONCE(sbefifo->rc) || (next == xfr && *n);
>  }
>  
>  static ssize_t sbefifo_write_common(struct sbefifo_client *client,
> @@ -612,6 +640,7 @@ static ssize_t sbefifo_write_common(struct sbefifo_client *client,
>  				    size_t len)
>  {
>  	struct sbefifo *sbefifo = client->dev;
> +	struct sbefifo_buf *wbuf = &client->wbuf;
>  	struct sbefifo_xfr *xfr;
>  	ssize_t ret = 0;
>  	size_t n;
> @@ -622,24 +651,26 @@ static ssize_t sbefifo_write_common(struct sbefifo_client *client,
>  	if (!len)
>  		return 0;
>  
> -	n = sbefifo_buf_nbwriteable(&client->wbuf);
> +	sbefifo_get_client(client);
> +	n = sbefifo_buf_nbwriteable(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);
> -		return -EAGAIN;
> +		ret = -EAGAIN;
> +		goto out;
>  	}
>  
> -	xfr = sbefifo_enq_xfr(client);
> +	xfr = sbefifo_enq_xfr(client);		/* this xfr queued up */
>  	if (!xfr) {
>  		spin_unlock_irq(&sbefifo->lock);
> -		return -ENOMEM;
> +		ret = PTR_ERR(xfr);
> +		goto out;
>  	}
> -	spin_unlock_irq(&sbefifo->lock);
>  
> -	sbefifo_get_client(client);
> +	spin_unlock_irq(&sbefifo->lock);
>  
>  	/*
>  	 * Partial writes are not really allowed in that EOT is sent exactly
> @@ -647,49 +678,47 @@ 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))
>  				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);
>  
>  		if (ubuf) {
> -			if (copy_from_user(READ_ONCE(client->wbuf.wpos), ubuf,
> -			    n)) {
> +			if (copy_from_user(READ_ONCE(wbuf->wpos), ubuf, n)) {
>  				set_bit(SBEFIFO_XFR_CANCEL, &xfr->flags);
>  				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;
>  		} else {
> -			memcpy(READ_ONCE(client->wbuf.wpos), kbuf, n);
> +			memcpy(READ_ONCE(wbuf->wpos), kbuf, n);
>  			kbuf += n;
>  		}
>  
> -		sbefifo_buf_wrotenb(&client->wbuf, n);
> +		sbefifo_buf_wrotenb(wbuf, n);
>  		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);
> @@ -702,21 +731,16 @@ static ssize_t sbefifo_write_common(struct sbefifo_client *client,
>  			sbefifo_put(sbefifo);
>  	}
>  
> +out:
>  	sbefifo_put_client(client);
> -
>  	return ret;
>  }
>  
>  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;
>  
> -	WARN_ON(*offset);
> -
> -	if (!access_ok(VERIFY_READ, buf, len))
> -		return -EFAULT;
> -
>  	return sbefifo_write_common(client, buf, NULL, len);
>  }
>  
> @@ -802,29 +826,27 @@ static int sbefifo_probe(struct device *dev)
>  	u32 sts;
>  	int ret, child_idx = 0;
>  
> -	dev_dbg(dev, "Found sbefifo device\n");
>  	sbefifo = kzalloc(sizeof(*sbefifo), GFP_KERNEL);
>  	if (!sbefifo)
>  		return -ENOMEM;
>  
>  	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;
>  	}
>  
> @@ -837,13 +859,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) */
> @@ -852,7 +874,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");
>  		}
>  	}
> @@ -922,10 +944,13 @@ static int sbefifo_init(void)
>  static void sbefifo_exit(void)
>  {
>  	fsi_driver_unregister(&sbefifo_drv);
> +
> +	ida_destroy(&sbefifo_ida);
>  }
>  
>  module_init(sbefifo_init);
>  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");
> diff --git a/include/linux/fsi-sbefifo.h b/include/linux/fsi-sbefifo.h
> index 1b46c63..8e55891 100644
> --- a/include/linux/fsi-sbefifo.h
> +++ b/include/linux/fsi-sbefifo.h
> @@ -13,8 +13,8 @@
>   * GNU General Public License for more details.
>   */
>  
> -#ifndef __FSI_SBEFIFO_H__
> -#define __FSI_SBEFIFO_H__
> +#ifndef LINUX_FSI_SBEFIFO_H
> +#define LINUX_FSI_SBEFIFO_H
>  
>  struct device;
>  struct sbefifo_client;
> @@ -27,4 +27,4 @@ extern int sbefifo_drv_write(struct sbefifo_client *client, const char *buf,
>  			     size_t len);
>  extern void sbefifo_drv_release(struct sbefifo_client *client);
>  
> -#endif /* __FSI_SBEFIFO_H__ */
> +#endif /* LINUX_FSI_SBEFIFO_H */

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

Re: [PATCH linux dev-4.10 v2 1/9] drivers: fsi: SBEFIFO: General clean-up

[Eddie James]

On 10/04/2017 06:59 PM, Andrew Jeffery wrote:
> On Fri, 2017-09-29 at 17:41 -0500, Eddie James wrote:
>> From: "Edward A. James" <eajames@us.ibm.com>
>>   
>> Correct formatting and improve the flow a bit. Remove warnings for
>> offset in read/write functions, and remove the user pointer verification
>> since the copy_from/to_user will do that anyway. Correct the include
>> guards in the header file.
> This is similar to what I complained about last time, just without bullet
> points in the description.
>
> I tried to read the diff but really struggled to make sense of it. Keeping
> track of the added/removed/changed functions - amongst formatting and control
> flow changes - to decide whether they were appropriate is just *really*
> difficult, and I can't say with confidence that as a whole this patch is okay.
>
> So, apologies, but I don't plan on acking this as it stands, and I don't think
> it should be submitted unless several people are ready to weld Tested-by tags
> to it.

OK, we can just drop this patch, as it has no real functional changes. 
We'll just be out of sync with what I send upstream.

Thanks,
Eddie

>
> Andrew
>
>>   
>> Signed-off-by: Edward A. James <eajames@us.ibm.com>
>> ---
>>   drivers/fsi/fsi-sbefifo.c   | 207 +++++++++++++++++++++++++-------------------
>>   include/linux/fsi-sbefifo.h |   6 +-
>>   2 files changed, 119 insertions(+), 94 deletions(-)
>>   
>> diff --git a/drivers/fsi/fsi-sbefifo.c b/drivers/fsi/fsi-sbefifo.c
>> index 1c37ff7..a4cd353 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
>> @@ -44,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;
>> @@ -94,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));
>> @@ -102,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,
>> @@ -136,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)
>> @@ -210,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;
>>   }
>>   
>> @@ -229,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);
>>   }
>>   
>> @@ -267,21 +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;
>>   
>> @@ -349,6 +352,7 @@ static struct sbefifo_xfr *sbefifo_next_xfr(struct sbefifo *sbefifo)
>>   			kfree(xfr);
>>   			continue;
>>   		}
>> +
>>   		return xfr;
>>   	}
>>   
>> @@ -370,7 +374,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;
>>   
>> @@ -388,8 +392,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;
>>   
>> @@ -397,7 +400,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;
>>   		}
>> @@ -414,9 +417,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;
>>   
>> @@ -438,7 +440,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;
>>   		}
>> @@ -466,7 +468,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;
>>   		}
>> @@ -476,7 +478,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);
>> @@ -497,7 +499,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;
>>   
>> @@ -535,6 +537,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)
>>   {
>> @@ -551,30 +563,37 @@ 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_put_client(client);
>> -		return -ERESTARTSYS;
>> +				     sbefifo_read_ready(sbefifo, client,
>> +							&n, &ret))) {
>> +		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
>> +	} else {
>>   		memcpy(kbuf, READ_ONCE(client->rbuf.rpos), n);
>> +	}
>>   
>>   	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");
>> +			goto out;
>> +		}
>> +
>>   		if (!test_bit(SBEFIFO_XFR_COMPLETE, &xfr->flags)) {
>>   			/*
>>   			 * Fill the read buffer back up.
>> @@ -589,22 +608,31 @@ 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, 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;
>>   
>> -	WARN_ON(*offset);
>> +	return sbefifo_read_common(client, buf, NULL, len);
>> +}
>>   
>> -	if (!access_ok(VERIFY_WRITE, buf, len))
>> -		return -EFAULT;
>> +static bool sbefifo_write_ready(struct sbefifo *sbefifo,
>> +				struct sbefifo_xfr *xfr,
>> +				struct sbefifo_client *client, size_t *n)
>> +{
>> +	struct sbefifo_xfr *next = list_first_entry_or_null(&client->xfrs,
>> +							    struct sbefifo_xfr,
>> +							    client);
>>   
>> -	return sbefifo_read_common(client, buf, NULL, len);
>> +	*n = sbefifo_buf_nbwriteable(&client->wbuf);
>> +	return READ_ONCE(sbefifo->rc) || (next == xfr && *n);
>>   }
>>   
>>   static ssize_t sbefifo_write_common(struct sbefifo_client *client,
>> @@ -612,6 +640,7 @@ static ssize_t sbefifo_write_common(struct sbefifo_client *client,
>>   				    size_t len)
>>   {
>>   	struct sbefifo *sbefifo = client->dev;
>> +	struct sbefifo_buf *wbuf = &client->wbuf;
>>   	struct sbefifo_xfr *xfr;
>>   	ssize_t ret = 0;
>>   	size_t n;
>> @@ -622,24 +651,26 @@ static ssize_t sbefifo_write_common(struct sbefifo_client *client,
>>   	if (!len)
>>   		return 0;
>>   
>> -	n = sbefifo_buf_nbwriteable(&client->wbuf);
>> +	sbefifo_get_client(client);
>> +	n = sbefifo_buf_nbwriteable(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);
>> -		return -EAGAIN;
>> +		ret = -EAGAIN;
>> +		goto out;
>>   	}
>>   
>> -	xfr = sbefifo_enq_xfr(client);
>> +	xfr = sbefifo_enq_xfr(client);		/* this xfr queued up */
>>   	if (!xfr) {
>>   		spin_unlock_irq(&sbefifo->lock);
>> -		return -ENOMEM;
>> +		ret = PTR_ERR(xfr);
>> +		goto out;
>>   	}
>> -	spin_unlock_irq(&sbefifo->lock);
>>   
>> -	sbefifo_get_client(client);
>> +	spin_unlock_irq(&sbefifo->lock);
>>   
>>   	/*
>>   	 * Partial writes are not really allowed in that EOT is sent exactly
>> @@ -647,49 +678,47 @@ 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))
>>   				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);
>>   
>>   		if (ubuf) {
>> -			if (copy_from_user(READ_ONCE(client->wbuf.wpos), ubuf,
>> -			    n)) {
>> +			if (copy_from_user(READ_ONCE(wbuf->wpos), ubuf, n)) {
>>   				set_bit(SBEFIFO_XFR_CANCEL, &xfr->flags);
>>   				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;
>>   		} else {
>> -			memcpy(READ_ONCE(client->wbuf.wpos), kbuf, n);
>> +			memcpy(READ_ONCE(wbuf->wpos), kbuf, n);
>>   			kbuf += n;
>>   		}
>>   
>> -		sbefifo_buf_wrotenb(&client->wbuf, n);
>> +		sbefifo_buf_wrotenb(wbuf, n);
>>   		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);
>> @@ -702,21 +731,16 @@ static ssize_t sbefifo_write_common(struct sbefifo_client *client,
>>   			sbefifo_put(sbefifo);
>>   	}
>>   
>> +out:
>>   	sbefifo_put_client(client);
>> -
>>   	return ret;
>>   }
>>   
>>   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;
>>   
>> -	WARN_ON(*offset);
>> -
>> -	if (!access_ok(VERIFY_READ, buf, len))
>> -		return -EFAULT;
>> -
>>   	return sbefifo_write_common(client, buf, NULL, len);
>>   }
>>   
>> @@ -802,29 +826,27 @@ static int sbefifo_probe(struct device *dev)
>>   	u32 sts;
>>   	int ret, child_idx = 0;
>>   
>> -	dev_dbg(dev, "Found sbefifo device\n");
>>   	sbefifo = kzalloc(sizeof(*sbefifo), GFP_KERNEL);
>>   	if (!sbefifo)
>>   		return -ENOMEM;
>>   
>>   	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;
>>   	}
>>   
>> @@ -837,13 +859,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) */
>> @@ -852,7 +874,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");
>>   		}
>>   	}
>> @@ -922,10 +944,13 @@ static int sbefifo_init(void)
>>   static void sbefifo_exit(void)
>>   {
>>   	fsi_driver_unregister(&sbefifo_drv);
>> +
>> +	ida_destroy(&sbefifo_ida);
>>   }
>>   
>>   module_init(sbefifo_init);
>>   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");
>> 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 */

[PATCH linux dev-4.10 v2 2/9] drivers: fsi: SBEFIFO: Fix probe() and remove()

[Eddie James]

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

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

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

diff --git a/drivers/fsi/fsi-sbefifo.c b/drivers/fsi/fsi-sbefifo.c
index a4cd353..fa34bd8 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);
@@ -604,7 +604,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,7 +664,7 @@ 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 = PTR_ERR(xfr);
 		goto out;
@@ -766,18 +766,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;
 }
@@ -850,69 +847,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_interruptible_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;
 }
@@ -937,7 +933,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

Re: [PATCH linux dev-4.10 v2 2/9] drivers: fsi: SBEFIFO: Fix probe() and remove()

[Andrew Jeffery]

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

On Fri, 2017-09-29 at 17:41 -0500, Eddie James wrote:
> From: "Edward A. James" <eajames@us.ibm.com>
> 
> Probe didn't handle a failed misc device registration properly. Remove
> had the potential for hangs. Also, remove the list of SBEFIFOs and
> instead just use the device "driver data" pointer.

What was the purpose of the list of SBEFIFOs? Are we losing something from the
intended design here? The list could be stored in the driver data pointer
right?

> 
> Signed-off-by: Edward A. James <eajames@us.ibm.com>
> ---
>  drivers/fsi/fsi-sbefifo.c | 109 ++++++++++++++++++++++------------------------
>  1 file changed, 52 insertions(+), 57 deletions(-)
> 
> diff --git a/drivers/fsi/fsi-sbefifo.c b/drivers/fsi/fsi-sbefifo.c
> index a4cd353..fa34bd8 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);
> @@ -604,7 +604,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,7 +664,7 @@ 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)) {

Should this be in what is currently patch 0/9?

>  		spin_unlock_irq(&sbefifo->lock);
>  		ret = PTR_ERR(xfr);
>  		goto out;
> @@ -766,18 +766,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;
>  }
> @@ -850,69 +847,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_interruptible_all(&sbefifo->wait);

If we're removing, we want to wake up non-interruptible tasks as well, so I
think wake_up_all() is more appropriate.

>  
> -		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;
>  }
> @@ -937,7 +933,6 @@ static int sbefifo_remove(struct device *dev)
>  
>  static int sbefifo_init(void)
>  {
> -	INIT_LIST_HEAD(&sbefifo_fifos);
>  	return fsi_driver_register(&sbefifo_drv);
>  }
>  

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

Re: [PATCH linux dev-4.10 v2 2/9] drivers: fsi: SBEFIFO: Fix probe() and remove()

[Eddie James]

On 10/04/2017 07:32 PM, Andrew Jeffery wrote:
> On Fri, 2017-09-29 at 17:41 -0500, Eddie James wrote:
>> From: "Edward A. James" <eajames@us.ibm.com>
>>   
>> Probe didn't handle a failed misc device registration properly. Remove
>> had the potential for hangs. Also, remove the list of SBEFIFOs and
>> instead just use the device "driver data" pointer.
> What was the purpose of the list of SBEFIFOs? Are we losing something from the
> intended design here? The list could be stored in the driver data pointer
> right?

Right, originally we weren't sure if we could use the fsi device driver 
data pointer. The only purpose of the list is to be able to find our 
sbefifo device when given an fsi device pointer.

>
>>   
>> Signed-off-by: Edward A. James <eajames@us.ibm.com>
>> ---
>>   drivers/fsi/fsi-sbefifo.c | 109 ++++++++++++++++++++++------------------------
>>   1 file changed, 52 insertions(+), 57 deletions(-)
>>   
>> diff --git a/drivers/fsi/fsi-sbefifo.c b/drivers/fsi/fsi-sbefifo.c
>> index a4cd353..fa34bd8 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);
>> @@ -604,7 +604,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,7 +664,7 @@ 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)) {
> Should this be in what is currently patch 0/9?

No. This patch set changes the return value of sbefifo_enq_xfr to return 
an appropriate error pointer.

>
>>   		spin_unlock_irq(&sbefifo->lock);
>>   		ret = PTR_ERR(xfr);
>>   		goto out;
>> @@ -766,18 +766,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;
>>   }
>> @@ -850,69 +847,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_interruptible_all(&sbefifo->wait);
> If we're removing, we want to wake up non-interruptible tasks as well, so I
> think wake_up_all() is more appropriate.

OK. Though there are no non-interruptible wait_event calls in this driver.

Thanks,
Eddie

>
>>   
>> -		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;
>>   }
>> @@ -937,7 +933,6 @@ static int sbefifo_remove(struct device *dev)
>>   
>>   static int sbefifo_init(void)
>>   {
>> -	INIT_LIST_HEAD(&sbefifo_fifos);
>>   	return fsi_driver_register(&sbefifo_drv);
>>   }
>>   

Re: [PATCH linux dev-4.10 v2 2/9] drivers: fsi: SBEFIFO: Fix probe() and remove()

[Eddie James]

On 10/05/2017 10:15 AM, Eddie James wrote:
>
>
> On 10/04/2017 07:32 PM, Andrew Jeffery wrote:
>> On Fri, 2017-09-29 at 17:41 -0500, Eddie James wrote:
>>> From: "Edward A. James" <eajames@us.ibm.com>
>>>   Probe didn't handle a failed misc device registration properly. 
>>> Remove
>>> had the potential for hangs. Also, remove the list of SBEFIFOs and
>>> instead just use the device "driver data" pointer.
>> What was the purpose of the list of SBEFIFOs? Are we losing something 
>> from the
>> intended design here? The list could be stored in the driver data 
>> pointer
>> right?
>
> Right, originally we weren't sure if we could use the fsi device 
> driver data pointer. The only purpose of the list is to be able to 
> find our sbefifo device when given an fsi device pointer.
>
>>
>>>   Signed-off-by: Edward A. James <eajames@us.ibm.com>
>>> ---
>>>   drivers/fsi/fsi-sbefifo.c | 109 
>>> ++++++++++++++++++++++------------------------
>>>   1 file changed, 52 insertions(+), 57 deletions(-)
>>>   diff --git a/drivers/fsi/fsi-sbefifo.c b/drivers/fsi/fsi-sbefifo.c
>>> index a4cd353..fa34bd8 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);
>>> @@ -604,7 +604,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,7 +664,7 @@ 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)) {
>> Should this be in what is currently patch 0/9?
>
> No. This patch set changes the return value of sbefifo_enq_xfr to 
> return an appropriate error pointer.

Ah, I see, I had the PTR_ERR(xfr) change below in the wrong patch set. 
Will fix.

>
>>
>>> spin_unlock_irq(&sbefifo->lock);
>>>           ret = PTR_ERR(xfr);
>>>           goto out;
>>> @@ -766,18 +766,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;
>>>   }
>>> @@ -850,69 +847,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_interruptible_all(&sbefifo->wait);
>> If we're removing, we want to wake up non-interruptible tasks as 
>> well, so I
>> think wake_up_all() is more appropriate.
>
> OK. Though there are no non-interruptible wait_event calls in this 
> driver.
>
> Thanks,
> Eddie
>
>>
>>>   -        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;
>>>   }
>>> @@ -937,7 +933,6 @@ static int sbefifo_remove(struct device *dev)
>>>     static int sbefifo_init(void)
>>>   {
>>> -    INIT_LIST_HEAD(&sbefifo_fifos);
>>>       return fsi_driver_register(&sbefifo_drv);
>>>   }
>

[PATCH linux dev-4.10 v2 3/9] drivers: fsi: SBEFIFO: check for xfr complete in read wait_event

[Eddie James]

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

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

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

diff --git a/drivers/fsi/fsi-sbefifo.c b/drivers/fsi/fsi-sbefifo.c
index fa34bd8..a9fc8e9 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

Re: [PATCH linux dev-4.10 v2 3/9] drivers: fsi: SBEFIFO: check for xfr complete in read wait_event

[Andrew Jeffery]

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

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

Acked-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 fa34bd8..a9fc8e9 100644
> --- a/drivers/fsi/fsi-sbefifo.c
> +++ b/drivers/fsi/fsi-sbefifo.c
> @@ -541,10 +541,15 @@ static bool sbefifo_read_ready(struct sbefifo *sbefifo,
>  			       struct sbefifo_client *client, size_t *n,
>  			       size_t *ret)
>  {
> +	struct sbefifo_xfr *xfr = list_first_entry_or_null(&client->xfrs,
> +							   struct sbefifo_xfr,
> +							   client);
> +
>  	*n = sbefifo_buf_nbreadable(&client->rbuf);
>  	*ret = READ_ONCE(sbefifo->rc);
>  
> -	return *ret || *n;
> +	return *ret || *n ||
> +		(xfr && test_bit(SBEFIFO_XFR_COMPLETE, &xfr->flags));
>  }
>  
>  static ssize_t sbefifo_read_common(struct sbefifo_client *client,

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

[PATCH linux dev-4.10 v2 4/9] drivers: fsi: occ: General clean-up

[Eddie James]

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

Fix include files. Cleanup formatting and add some comments. Fix an
errant kfree in an error case. Fix some errnos. Add OCC response
definitions to the header file.

Signed-off-by: Edward A. James <eajames@us.ibm.com>
---
 drivers/fsi/occ.c   | 145 ++++++++++++++++++++++++++++------------------------
 include/linux/occ.h |  20 ++++++--
 2 files changed, 96 insertions(+), 69 deletions(-)

diff --git a/drivers/fsi/occ.c b/drivers/fsi/occ.c
index 621fbf0..3313e35 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>
@@ -34,43 +39,35 @@ 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;
 };
 
 #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;
 	u8 return_status;
-	u16 data_length;
+	__be16 data_length;
 	u8 data[OCC_RESP_DATA_BYTES];
-	u16 checksum;
+	__be16 checksum;
 } __packed;
 
 /*
  * 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,
@@ -92,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,
@@ -104,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;
@@ -135,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;
 
@@ -183,8 +179,9 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
 		if (client->read_offset) {
 			rc = 0;
 			client->read_offset = 0;
-		} else
+		} else {
 			rc = -ENOMSG;
+		}
 
 		goto done;
 	}
@@ -200,8 +197,10 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
 		spin_unlock_irq(&client->lock);
 
 		rc = wait_event_interruptible(client->wait,
-			test_bit(XFR_COMPLETE, &xfr->flags) ||
-			test_bit(XFR_CANCELED, &xfr->flags));
+					      test_bit(XFR_COMPLETE,
+						       &xfr->flags) ||
+					      test_bit(XFR_CANCELED,
+						       &xfr->flags));
 
 		spin_lock_irq(&client->lock);
 
@@ -225,12 +224,14 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
 
 	bytes = min(len, xfr->resp_data_length - client->read_offset);
 	if (ubuf) {
-		if (copy_to_user(ubuf, &xfr->buf[client->read_offset], bytes)) {
+		if (copy_to_user(ubuf, &xfr->buf[client->read_offset],
+				 bytes)) {
 			rc = -EFAULT;
 			goto done;
 		}
-	} else
+	} else {
 		memcpy(kbuf, &xfr->buf[client->read_offset], bytes);
+	}
 
 	client->read_offset += bytes;
 
@@ -250,12 +251,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);
 }
 
@@ -272,28 +267,31 @@ static ssize_t occ_write_common(struct occ_client *client,
 		return -EINVAL;
 
 	spin_lock_irq(&client->lock);
+
 	if (test_and_set_bit(CLIENT_XFR_PENDING, &client->flags)) {
 		rc = -EBUSY;
 		goto done;
 	}
 
-	/* 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)) {
-			kfree(xfr);
 			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) {
-		kfree(xfr);
 		rc = -EINVAL;
 		goto done;
 	}
@@ -321,12 +319,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);
 }
 
@@ -366,7 +358,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;
@@ -403,7 +395,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,
@@ -422,7 +414,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,
@@ -430,10 +422,13 @@ static int occ_getsram(struct device *sbefifo, u32 address, u8 *data,
 {
 	int rc;
 	u8 *resp;
-	u32 buf[5];
-	u32 data_len = ((len + 7) / 8) * 8;
+	__be32 buf[5];
+	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);
@@ -447,7 +442,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;
@@ -467,7 +462,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);
@@ -481,8 +476,8 @@ static int occ_putsram(struct device *sbefifo, u32 address, u8 *data,
 		       ssize_t len)
 {
 	int rc;
-	u32 *buf;
-	u32 data_len = ((len + 7) / 8) * 8;
+	__be32 *buf;
+	u32 data_len = ((len + 7) / 8) * 8;	/* must be multiples of 8 B */
 	size_t cmd_len = data_len + 20;
 	struct sbefifo_client *client;
 
@@ -490,6 +485,9 @@ 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);
@@ -515,7 +513,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);
@@ -527,14 +525,17 @@ 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;
 
+	/* 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);
@@ -552,7 +553,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);
@@ -565,6 +566,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;
@@ -578,11 +580,13 @@ 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);
 	mutex_lock(&occ->occ_lock);
 
+	/* write occ command */
 	rc = occ_putsram(sbefifo, 0xFFFBE000, xfr->buf,
 			 xfr->cmd_data_length);
 	if (rc)
@@ -592,13 +596,14 @@ 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;
 
-	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;
+		rc = -EMSGSIZE;
 		goto done;
 	}
 
@@ -657,18 +662,27 @@ struct occ_client *occ_drv_open(struct device *dev, unsigned long flags)
 
 int occ_drv_read(struct occ_client *client, char *buf, size_t len)
 {
+	if (!client)
+		return -ENODEV;
+
 	return occ_read_common(client, NULL, buf, len);
 }
 EXPORT_SYMBOL_GPL(occ_drv_read);
 
 int occ_drv_write(struct occ_client *client, const char *buf, size_t len)
 {
+	if (!client)
+		return -ENODEV;
+
 	return occ_write_common(client, NULL, buf, len);
 }
 EXPORT_SYMBOL_GPL(occ_drv_write);
 
 void occ_drv_release(struct occ_client *client)
 {
+	if (!client)
+		return;
+
 	occ_release_common(client);
 }
 EXPORT_SYMBOL_GPL(occ_drv_release);
@@ -704,9 +718,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) {
@@ -789,6 +800,8 @@ static void occ_exit(void)
 	destroy_workqueue(occ_wq);
 
 	platform_driver_unregister(&occ_driver);
+
+	ida_destroy(&occ_ida);
 }
 
 module_init(occ_init);
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

Re: [PATCH linux dev-4.10 v2 4/9] drivers: fsi: occ: General clean-up

[Andrew Jeffery]

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

On Fri, 2017-09-29 at 17:41 -0500, Eddie James wrote:
> From: "Edward A. James" <eajames@us.ibm.com>
> 
> Fix include files. Cleanup formatting and add some comments. Fix an
> errant kfree in an error case. Fix some errnos. Add OCC response
> definitions to the header file.

Unlike the SBEFIFO patch I haven't studied the diff here, but I also don't
intend on ack'ing this as it stands. There's just too much going on to judge
that it's reasonable.

Andrew

> 
> Signed-off-by: Edward A. James <eajames@us.ibm.com>
> ---
>  drivers/fsi/occ.c   | 145 ++++++++++++++++++++++++++++------------------------
>  include/linux/occ.h |  20 ++++++--
>  2 files changed, 96 insertions(+), 69 deletions(-)
> 
> diff --git a/drivers/fsi/occ.c b/drivers/fsi/occ.c
> index 621fbf0..3313e35 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>
> @@ -34,43 +39,35 @@ 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;
>  };
>  
>  #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;
>  	u8 return_status;
> -	u16 data_length;
> +	__be16 data_length;
>  	u8 data[OCC_RESP_DATA_BYTES];
> -	u16 checksum;
> +	__be16 checksum;
>  } __packed;
>  
>  /*
>   * 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,
> @@ -92,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,
> @@ -104,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;
> @@ -135,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;
>  
> @@ -183,8 +179,9 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
>  		if (client->read_offset) {
>  			rc = 0;
>  			client->read_offset = 0;
> -		} else
> +		} else {
>  			rc = -ENOMSG;
> +		}
>  
>  		goto done;
>  	}
> @@ -200,8 +197,10 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
>  		spin_unlock_irq(&client->lock);
>  
>  		rc = wait_event_interruptible(client->wait,
> -			test_bit(XFR_COMPLETE, &xfr->flags) ||
> -			test_bit(XFR_CANCELED, &xfr->flags));
> +					      test_bit(XFR_COMPLETE,
> +						       &xfr->flags) ||
> +					      test_bit(XFR_CANCELED,
> +						       &xfr->flags));
>  
>  		spin_lock_irq(&client->lock);
>  
> @@ -225,12 +224,14 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
>  
>  	bytes = min(len, xfr->resp_data_length - client->read_offset);
>  	if (ubuf) {
> -		if (copy_to_user(ubuf, &xfr->buf[client->read_offset], bytes)) {
> +		if (copy_to_user(ubuf, &xfr->buf[client->read_offset],
> +				 bytes)) {
>  			rc = -EFAULT;
>  			goto done;
>  		}
> -	} else
> +	} else {
>  		memcpy(kbuf, &xfr->buf[client->read_offset], bytes);
> +	}
>  
>  	client->read_offset += bytes;
>  
> @@ -250,12 +251,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);
>  }
>  
> @@ -272,28 +267,31 @@ static ssize_t occ_write_common(struct occ_client *client,
>  		return -EINVAL;
>  
>  	spin_lock_irq(&client->lock);
> +
>  	if (test_and_set_bit(CLIENT_XFR_PENDING, &client->flags)) {
>  		rc = -EBUSY;
>  		goto done;
>  	}
>  
> -	/* 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)) {
> -			kfree(xfr);
>  			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) {
> -		kfree(xfr);
>  		rc = -EINVAL;
>  		goto done;
>  	}
> @@ -321,12 +319,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);
>  }
>  
> @@ -366,7 +358,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;
> @@ -403,7 +395,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,
> @@ -422,7 +414,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,
> @@ -430,10 +422,13 @@ static int occ_getsram(struct device *sbefifo, u32 address, u8 *data,
>  {
>  	int rc;
>  	u8 *resp;
> -	u32 buf[5];
> -	u32 data_len = ((len + 7) / 8) * 8;
> +	__be32 buf[5];
> +	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);
> @@ -447,7 +442,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;
> @@ -467,7 +462,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);
> @@ -481,8 +476,8 @@ static int occ_putsram(struct device *sbefifo, u32 address, u8 *data,
>  		       ssize_t len)
>  {
>  	int rc;
> -	u32 *buf;
> -	u32 data_len = ((len + 7) / 8) * 8;
> +	__be32 *buf;
> +	u32 data_len = ((len + 7) / 8) * 8;	/* must be multiples of 8 B */
>  	size_t cmd_len = data_len + 20;
>  	struct sbefifo_client *client;
>  
> @@ -490,6 +485,9 @@ 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);
> @@ -515,7 +513,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);
> @@ -527,14 +525,17 @@ 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;
>  
> +	/* 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);
> @@ -552,7 +553,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);
> @@ -565,6 +566,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;
> @@ -578,11 +580,13 @@ 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);
>  	mutex_lock(&occ->occ_lock);
>  
> +	/* write occ command */
>  	rc = occ_putsram(sbefifo, 0xFFFBE000, xfr->buf,
>  			 xfr->cmd_data_length);
>  	if (rc)
> @@ -592,13 +596,14 @@ 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;
>  
> -	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;
> +		rc = -EMSGSIZE;
>  		goto done;
>  	}
>  
> @@ -657,18 +662,27 @@ struct occ_client *occ_drv_open(struct device *dev, unsigned long flags)
>  
>  int occ_drv_read(struct occ_client *client, char *buf, size_t len)
>  {
> +	if (!client)
> +		return -ENODEV;
> +
>  	return occ_read_common(client, NULL, buf, len);
>  }
>  EXPORT_SYMBOL_GPL(occ_drv_read);
>  
>  int occ_drv_write(struct occ_client *client, const char *buf, size_t len)
>  {
> +	if (!client)
> +		return -ENODEV;
> +
>  	return occ_write_common(client, NULL, buf, len);
>  }
>  EXPORT_SYMBOL_GPL(occ_drv_write);
>  
>  void occ_drv_release(struct occ_client *client)
>  {
> +	if (!client)
> +		return;
> +
>  	occ_release_common(client);
>  }
>  EXPORT_SYMBOL_GPL(occ_drv_release);
> @@ -704,9 +718,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) {
> @@ -789,6 +800,8 @@ static void occ_exit(void)
>  	destroy_workqueue(occ_wq);
>  
>  	platform_driver_unregister(&occ_driver);
> +
> +	ida_destroy(&occ_ida);
>  }
>  
>  module_init(occ_init);
> diff --git a/include/linux/occ.h b/include/linux/occ.h
> index d78332c..0a4a54a 100644
> --- a/include/linux/occ.h
> +++ b/include/linux/occ.h
> @@ -11,12 +11,26 @@
>   * GNU General Public License for more details.
>   */
>  
> -#ifndef __OCC_H__
> -#define __OCC_H__
> +#ifndef LINUX_FSI_OCC_H
> +#define LINUX_FSI_OCC_H
>  
>  struct device;
>  struct occ_client;
>  
> +#define OCC_RESP_CMD_IN_PRG		0xFF
> +#define OCC_RESP_SUCCESS		0
> +#define OCC_RESP_CMD_INVAL		0x11
> +#define OCC_RESP_CMD_LEN_INVAL		0x12
> +#define OCC_RESP_DATA_INVAL		0x13
> +#define OCC_RESP_CHKSUM_ERR		0x14
> +#define OCC_RESP_INT_ERR		0x15
> +#define OCC_RESP_BAD_STATE		0x16
> +#define OCC_RESP_CRIT_EXCEPT		0xE0
> +#define OCC_RESP_CRIT_INIT		0xE1
> +#define OCC_RESP_CRIT_WATCHDOG		0xE2
> +#define OCC_RESP_CRIT_OCB		0xE3
> +#define OCC_RESP_CRIT_HW		0xE4
> +
>  extern struct occ_client *occ_drv_open(struct device *dev,
>  				       unsigned long flags);
>  extern int occ_drv_read(struct occ_client *client, char *buf, size_t len);
> @@ -24,4 +38,4 @@ extern int occ_drv_write(struct occ_client *client, const char *buf,
>  			 size_t len);
>  extern void occ_drv_release(struct occ_client *client);
>  
> -#endif /* __OCC_H__ */
> +#endif /* LINUX_FSI_OCC_H */

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

Re: [PATCH linux dev-4.10 v2 4/9] drivers: fsi: occ: General clean-up

[Eddie James]

On 10/04/2017 07:37 PM, Andrew Jeffery wrote:
> On Fri, 2017-09-29 at 17:41 -0500, Eddie James wrote:
>> From: "Edward A. James" <eajames@us.ibm.com>
>>   
>> Fix include files. Cleanup formatting and add some comments. Fix an
>> errant kfree in an error case. Fix some errnos. Add OCC response
>> definitions to the header file.
> Unlike the SBEFIFO patch I haven't studied the diff here, but I also don't
> intend on ack'ing this as it stands. There's just too much going on to judge
> that it's reasonable.

We can just drop this one too. It's just formatting changes and changing 
some errnos. Except for the kfree thing, which I can separate out.

I guess the problem here is that normally this is the kind of stuff that 
would be different in a v1 and v2 of a patchset, just cleaning stuff up. 
And in that case, it wouldn't be a problem to make these kinds of 
changes. But since this is already in our tree, it comes out to a big 
diff. I'd love to have 4.10 be nice and clean and match with what goes 
upstream, but it's quite time consuming to break everything up into such 
small chunks.

Thanks,
Eddie

>
> Andrew
>
>>   
>> Signed-off-by: Edward A. James <eajames@us.ibm.com>
>> ---
>>   drivers/fsi/occ.c   | 145 ++++++++++++++++++++++++++++------------------------
>>   include/linux/occ.h |  20 ++++++--
>>   2 files changed, 96 insertions(+), 69 deletions(-)
>>   
>> diff --git a/drivers/fsi/occ.c b/drivers/fsi/occ.c
>> index 621fbf0..3313e35 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>
>> @@ -34,43 +39,35 @@ 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;
>>   };
>>   
>>   #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;
>>   	u8 return_status;
>> -	u16 data_length;
>> +	__be16 data_length;
>>   	u8 data[OCC_RESP_DATA_BYTES];
>> -	u16 checksum;
>> +	__be16 checksum;
>>   } __packed;
>>   
>>   /*
>>    * 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,
>> @@ -92,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,
>> @@ -104,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;
>> @@ -135,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;
>>   
>> @@ -183,8 +179,9 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
>>   		if (client->read_offset) {
>>   			rc = 0;
>>   			client->read_offset = 0;
>> -		} else
>> +		} else {
>>   			rc = -ENOMSG;
>> +		}
>>   
>>   		goto done;
>>   	}
>> @@ -200,8 +197,10 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
>>   		spin_unlock_irq(&client->lock);
>>   
>>   		rc = wait_event_interruptible(client->wait,
>> -			test_bit(XFR_COMPLETE, &xfr->flags) ||
>> -			test_bit(XFR_CANCELED, &xfr->flags));
>> +					      test_bit(XFR_COMPLETE,
>> +						       &xfr->flags) ||
>> +					      test_bit(XFR_CANCELED,
>> +						       &xfr->flags));
>>   
>>   		spin_lock_irq(&client->lock);
>>   
>> @@ -225,12 +224,14 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
>>   
>>   	bytes = min(len, xfr->resp_data_length - client->read_offset);
>>   	if (ubuf) {
>> -		if (copy_to_user(ubuf, &xfr->buf[client->read_offset], bytes)) {
>> +		if (copy_to_user(ubuf, &xfr->buf[client->read_offset],
>> +				 bytes)) {
>>   			rc = -EFAULT;
>>   			goto done;
>>   		}
>> -	} else
>> +	} else {
>>   		memcpy(kbuf, &xfr->buf[client->read_offset], bytes);
>> +	}
>>   
>>   	client->read_offset += bytes;
>>   
>> @@ -250,12 +251,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);
>>   }
>>   
>> @@ -272,28 +267,31 @@ static ssize_t occ_write_common(struct occ_client *client,
>>   		return -EINVAL;
>>   
>>   	spin_lock_irq(&client->lock);
>> +
>>   	if (test_and_set_bit(CLIENT_XFR_PENDING, &client->flags)) {
>>   		rc = -EBUSY;
>>   		goto done;
>>   	}
>>   
>> -	/* 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)) {
>> -			kfree(xfr);
>>   			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) {
>> -		kfree(xfr);
>>   		rc = -EINVAL;
>>   		goto done;
>>   	}
>> @@ -321,12 +319,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);
>>   }
>>   
>> @@ -366,7 +358,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;
>> @@ -403,7 +395,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,
>> @@ -422,7 +414,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,
>> @@ -430,10 +422,13 @@ static int occ_getsram(struct device *sbefifo, u32 address, u8 *data,
>>   {
>>   	int rc;
>>   	u8 *resp;
>> -	u32 buf[5];
>> -	u32 data_len = ((len + 7) / 8) * 8;
>> +	__be32 buf[5];
>> +	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);
>> @@ -447,7 +442,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;
>> @@ -467,7 +462,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);
>> @@ -481,8 +476,8 @@ static int occ_putsram(struct device *sbefifo, u32 address, u8 *data,
>>   		       ssize_t len)
>>   {
>>   	int rc;
>> -	u32 *buf;
>> -	u32 data_len = ((len + 7) / 8) * 8;
>> +	__be32 *buf;
>> +	u32 data_len = ((len + 7) / 8) * 8;	/* must be multiples of 8 B */
>>   	size_t cmd_len = data_len + 20;
>>   	struct sbefifo_client *client;
>>   
>> @@ -490,6 +485,9 @@ 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);
>> @@ -515,7 +513,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);
>> @@ -527,14 +525,17 @@ 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;
>>   
>> +	/* 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);
>> @@ -552,7 +553,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);
>> @@ -565,6 +566,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;
>> @@ -578,11 +580,13 @@ 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);
>>   	mutex_lock(&occ->occ_lock);
>>   
>> +	/* write occ command */
>>   	rc = occ_putsram(sbefifo, 0xFFFBE000, xfr->buf,
>>   			 xfr->cmd_data_length);
>>   	if (rc)
>> @@ -592,13 +596,14 @@ 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;
>>   
>> -	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;
>> +		rc = -EMSGSIZE;
>>   		goto done;
>>   	}
>>   
>> @@ -657,18 +662,27 @@ struct occ_client *occ_drv_open(struct device *dev, unsigned long flags)
>>   
>>   int occ_drv_read(struct occ_client *client, char *buf, size_t len)
>>   {
>> +	if (!client)
>> +		return -ENODEV;
>> +
>>   	return occ_read_common(client, NULL, buf, len);
>>   }
>>   EXPORT_SYMBOL_GPL(occ_drv_read);
>>   
>>   int occ_drv_write(struct occ_client *client, const char *buf, size_t len)
>>   {
>> +	if (!client)
>> +		return -ENODEV;
>> +
>>   	return occ_write_common(client, NULL, buf, len);
>>   }
>>   EXPORT_SYMBOL_GPL(occ_drv_write);
>>   
>>   void occ_drv_release(struct occ_client *client)
>>   {
>> +	if (!client)
>> +		return;
>> +
>>   	occ_release_common(client);
>>   }
>>   EXPORT_SYMBOL_GPL(occ_drv_release);
>> @@ -704,9 +718,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) {
>> @@ -789,6 +800,8 @@ static void occ_exit(void)
>>   	destroy_workqueue(occ_wq);
>>   
>>   	platform_driver_unregister(&occ_driver);
>> +
>> +	ida_destroy(&occ_ida);
>>   }
>>   
>>   module_init(occ_init);
>> 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 */

[PATCH linux dev-4.10 v2 5/9] drivers: fsi: occ: Poll while receiving "command in progress"

[Eddie James]

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

This should only be done at this level, instead of clients repeating
the entire transfer when they receive "command in progress"

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

diff --git a/drivers/fsi/occ.c b/drivers/fsi/occ.c
index 3313e35..a554550 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];
@@ -565,6 +569,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;
@@ -586,6 +593,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);
@@ -597,9 +606,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

Re: [PATCH linux dev-4.10 v2 5/9] drivers: fsi: occ: Poll while receiving "command in progress"

[Andrew Jeffery]

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

On Fri, 2017-09-29 at 17:41 -0500, Eddie James wrote:
> From: "Edward A. James" <eajames@us.ibm.com>
> 
> This should only be done at this level,

Bit of a nit with the grammar here: You're using 'this' in multiple contexts
and I feel that explanation of the contexts is a bit lacking. Can you please
expand this sentence to cover those points?

The intent of the change seems reasonable though.

Andrew

> instead of clients repeating
> the entire transfer when they receive "command in progress"
> 
> Signed-off-by: Edward A. James <eajames@us.ibm.com>
> ---
>  drivers/fsi/occ.c | 27 ++++++++++++++++++++++++---
>  1 file changed, 24 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/fsi/occ.c b/drivers/fsi/occ.c
> index 3313e35..a554550 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

The SBEFIFO spec defines different timeout periods for different classes of
command. One class uses a 1s timeout, whilst another uses 30s. Does the OCC
only use commands from the former command class?

> +#define OCC_CMD_IN_PRG_WAIT_MS	50

This is less than the 500ms reschedule period in the SBEFIFO. Does 50ms make
sense here?

> +
>  struct occ {
>  	struct device *sbefifo;
>  	char name[32];
> @@ -565,6 +569,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;
> @@ -586,6 +593,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);
> @@ -597,9 +606,21 @@ static void occ_worker(struct work_struct *work)
>  		goto done;
>  
>  	/* read occ response */
> -	rc = occ_getsram(sbefifo, 0xFFFBF000, xfr->buf, 8);
> -	if (rc)
> -		goto done;
> +	do {
> +		rc = occ_getsram(sbefifo, 0xFFFBF000, xfr->buf, 8);
> +		if (rc)
> +			goto done;
> +
> +		if (resp->return_status == OCC_RESP_CMD_IN_PRG) {
> +			rc = -EALREADY;
> +
> +			if (time_after(jiffies, start + timeout))
> +				break;
> +
> +			set_current_state(TASK_INTERRUPTIBLE);
> +			schedule_timeout(wait_time);
> +		}
> +	} while (rc);
>  
>  	resp_data_length = get_unaligned_be16(&resp->data_length);
>  	if (resp_data_length > OCC_RESP_DATA_BYTES) {

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

Re: [PATCH linux dev-4.10 v2 5/9] drivers: fsi: occ: Poll while receiving "command in progress"

[Eddie James]

On 10/04/2017 08:01 PM, Andrew Jeffery wrote:
> On Fri, 2017-09-29 at 17:41 -0500, Eddie James wrote:
>> From: "Edward A. James" <eajames@us.ibm.com>
>>   
>> This should only be done at this level,
> Bit of a nit with the grammar here: You're using 'this' in multiple contexts
> and I feel that explanation of the contexts is a bit lacking. Can you please
> expand this sentence to cover those points?
>
> The intent of the change seems reasonable though.
>
> Andrew
>
>> instead of clients repeating
>> the entire transfer when they receive "command in progress"
>>   
>> Signed-off-by: Edward A. James <eajames@us.ibm.com>
>> ---
>>   drivers/fsi/occ.c | 27 ++++++++++++++++++++++++---
>>   1 file changed, 24 insertions(+), 3 deletions(-)
>>   
>> diff --git a/drivers/fsi/occ.c b/drivers/fsi/occ.c
>> index 3313e35..a554550 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
> The SBEFIFO spec defines different timeout periods for different classes of
> command. One class uses a 1s timeout, whilst another uses 30s. Does the OCC
> only use commands from the former command class?

These are the long timeout, but I don't see that this value corresponds 
to the SBE timeout. If the SBE is timing out, we're done for anyway. 
This is to timeout if we see OCC "command in progress" response for too 
long.

>
>> +#define OCC_CMD_IN_PRG_WAIT_MS	50
> This is less than the 500ms reschedule period in the SBEFIFO. Does 50ms make
> sense here?

Well that reschedule in SBEFIFO is only if you have to wait for more 
data. A transfer doesn't necessarily have to wait that long. In this 
case I'd be hopeful that the op can go through all at once since it's 
just 8 bytes. Haven't done any benchmarking.

Thanks,
Eddie

>
>> +
>>   struct occ {
>>   	struct device *sbefifo;
>>   	char name[32];
>> @@ -565,6 +569,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;
>> @@ -586,6 +593,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);
>> @@ -597,9 +606,21 @@ static void occ_worker(struct work_struct *work)
>>   		goto done;
>>   
>>   	/* read occ response */
>> -	rc = occ_getsram(sbefifo, 0xFFFBF000, xfr->buf, 8);
>> -	if (rc)
>> -		goto done;
>> +	do {
>> +		rc = occ_getsram(sbefifo, 0xFFFBF000, xfr->buf, 8);
>> +		if (rc)
>> +			goto done;
>> +
>> +		if (resp->return_status == OCC_RESP_CMD_IN_PRG) {
>> +			rc = -EALREADY;
>> +
>> +			if (time_after(jiffies, start + timeout))
>> +				break;
>> +
>> +			set_current_state(TASK_INTERRUPTIBLE);
>> +			schedule_timeout(wait_time);
>> +		}
>> +	} while (rc);
>>   
>>   	resp_data_length = get_unaligned_be16(&resp->data_length);
>>   	if (resp_data_length > OCC_RESP_DATA_BYTES) {

Re: [PATCH linux dev-4.10 v2 5/9] drivers: fsi: occ: Poll while receiving "command in progress"

[Andrew Jeffery]

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

On Thu, 2017-10-05 at 10:38 -0500, Eddie James wrote:
> 
> On 10/04/2017 08:01 PM, Andrew Jeffery wrote:
> > On Fri, 2017-09-29 at 17:41 -0500, Eddie James wrote:
> > > > > > From: "Edward A. James" <eajames@us.ibm.com>
> > >   
> > > This should only be done at this level,
> > 
> > Bit of a nit with the grammar here: You're using 'this' in multiple contexts
> > and I feel that explanation of the contexts is a bit lacking. Can you please
> > expand this sentence to cover those points?
> > 
> > The intent of the change seems reasonable though.
> > 
> > Andrew
> > 
> > > instead of clients repeating
> > > the entire transfer when they receive "command in progress"
> > >   
> > > > > > Signed-off-by: Edward A. James <eajames@us.ibm.com>
> > > ---
> > >   drivers/fsi/occ.c | 27 ++++++++++++++++++++++++---
> > >   1 file changed, 24 insertions(+), 3 deletions(-)
> > >   
> > > diff --git a/drivers/fsi/occ.c b/drivers/fsi/occ.c
> > > index 3313e35..a554550 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
> > 
> > The SBEFIFO spec defines different timeout periods for different classes of
> > command. One class uses a 1s timeout, whilst another uses 30s. Does the OCC
> > only use commands from the former command class?
> 
> These are the long timeout, but I don't see that this value corresponds 
> to the SBE timeout.

It could if the OCC wasn't responding with "command in progress", as
the SBE driver wouldn't be able to tell the difference. Don't know
whether that's possible though.

>  If the SBE is timing out, we're done for anyway. 

Sure.

> This is to timeout if we see OCC "command in progress" response for too 
> long.

Right.

> 
> > 
> > > +#define OCC_CMD_IN_PRG_WAIT_MS	50
> > 
> > This is less than the 500ms reschedule period in the SBEFIFO. Does 50ms make
> > sense here?
> 
> Well that reschedule in SBEFIFO is only if you have to wait for more 
> data. A transfer doesn't necessarily have to wait that long. In this 
> case I'd be hopeful that the op can go through all at once since it's 
> just 8 bytes. Haven't done any benchmarking.

Okay, thanks for clearing that up.

> 
> Thanks,
> Eddie
> 
> > 
> > > +
> > >   struct occ {
> > > > > >   	struct device *sbefifo;
> > > > > >   	char name[32];
> > > @@ -565,6 +569,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;
> > > @@ -586,6 +593,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);
> > > @@ -597,9 +606,21 @@ static void occ_worker(struct work_struct *work)
> > > > > >   		goto done;
> > >   
> > > > > >   	/* read occ response */
> > > > > > -	rc = occ_getsram(sbefifo, 0xFFFBF000, xfr->buf, 8);
> > > > > > -	if (rc)
> > > > > > -		goto done;
> > > > > > +	do {
> > > > > > +		rc = occ_getsram(sbefifo, 0xFFFBF000, xfr->buf, 8);
> > > > > > +		if (rc)
> > > > > > +			goto done;
> > > +
> > > > > > +		if (resp->return_status == OCC_RESP_CMD_IN_PRG) {
> > > > > > +			rc = -EALREADY;
> > > +
> > > > > > +			if (time_after(jiffies, start + timeout))
> > > > > > +				break;
> > > +
> > > > > > +			set_current_state(TASK_INTERRUPTIBLE);
> > > > > > +			schedule_timeout(wait_time);
> > > > > > +		}
> > > > > > +	} while (rc);
> > >   
> > > > > >   	resp_data_length = get_unaligned_be16(&resp->data_length);
> > >   	if (resp_data_length > OCC_RESP_DATA_BYTES) {
> 
> 

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

[PATCH linux dev-4.10 v2 6/9] drivers: fsi: occ: Add cancel to remove() and fix probe()

[Eddie James]

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

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

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

diff --git a/drivers/fsi/occ.c b/drivers/fsi/occ.c
index a554550..550ae11 100644
--- a/drivers/fsi/occ.c
+++ b/drivers/fsi/occ.c
@@ -46,6 +46,7 @@ struct occ {
 	spinlock_t list_lock;		/* lock access to the xfrs list */
 	struct mutex occ_lock;		/* lock access to the hardware */
 	struct work_struct work;
+	bool cancel;
 };
 
 #define to_occ(x)	container_of((x), struct occ, mdev)
@@ -117,12 +118,15 @@ struct occ_client {
 
 static DEFINE_IDA(occ_ida);
 
-static void occ_enqueue_xfr(struct occ_xfr *xfr)
+static int occ_enqueue_xfr(struct occ_xfr *xfr)
 {
 	int empty;
 	struct occ_client *client = to_client(xfr);
 	struct occ *occ = client->occ;
 
+	if (occ->cancel)
+		return -ECANCELED;
+
 	spin_lock_irq(&occ->list_lock);
 
 	empty = list_empty(&occ->xfrs);
@@ -132,14 +136,20 @@ static void occ_enqueue_xfr(struct occ_xfr *xfr)
 
 	if (empty)
 		queue_work(occ_wq, &occ->work);
+
+	return 0;
 }
 
 static struct occ_client *occ_open_common(struct occ *occ, unsigned long flags)
 {
-	struct occ_client *client = kzalloc(sizeof(*client), GFP_KERNEL);
+	struct occ_client *client;
+
+	if (occ->cancel)
+		return ERR_PTR(-ECANCELED);
 
+	client = kzalloc(sizeof(*client), GFP_KERNEL);
 	if (!client)
-		return NULL;
+		return ERR_PTR(-ENOMEM);
 
 	client->occ = occ;
 	spin_lock_init(&client->lock);
@@ -158,8 +168,8 @@ static int occ_open(struct inode *inode, struct file *file)
 	struct occ *occ = to_occ(mdev);
 
 	client = occ_open_common(occ, file->f_flags);
-	if (!client)
-		return -ENOMEM;
+	if (IS_ERR(client))
+		return PTR_ERR(client);
 
 	file->private_data = client;
 
@@ -172,6 +182,7 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
 	int rc;
 	size_t bytes;
 	struct occ_xfr *xfr = &client->xfr;
+	struct occ *occ = client->occ;
 
 	if (len > OCC_SRAM_BYTES)
 		return -EINVAL;
@@ -204,7 +215,8 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
 					      test_bit(XFR_COMPLETE,
 						       &xfr->flags) ||
 					      test_bit(XFR_CANCELED,
-						       &xfr->flags));
+						       &xfr->flags) ||
+					      occ->cancel);
 
 		spin_lock_irq(&client->lock);
 
@@ -272,7 +284,7 @@ static ssize_t occ_write_common(struct occ_client *client,
 
 	spin_lock_irq(&client->lock);
 
-	if (test_and_set_bit(CLIENT_XFR_PENDING, &client->flags)) {
+	if (test_bit(CLIENT_XFR_PENDING, &client->flags)) {
 		rc = -EBUSY;
 		goto done;
 	}
@@ -309,8 +321,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:
@@ -579,6 +594,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);
@@ -672,12 +690,17 @@ static void occ_worker(struct work_struct *work)
 
 struct occ_client *occ_drv_open(struct device *dev, unsigned long flags)
 {
+	struct occ_client *client;
 	struct occ *occ = dev_get_drvdata(dev);
 
 	if (!occ)
 		return NULL;
 
-	return occ_open_common(occ, flags);
+	client = occ_open_common(occ, flags);
+	if (IS_ERR(client))
+		return NULL;
+
+	return client;
 }
 EXPORT_SYMBOL_GPL(occ_drv_open);
 
@@ -748,23 +771,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;
@@ -774,20 +787,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_interruptible(&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 v2 6/9] drivers: fsi: occ: Add cancel to remove() and fix probe()

[Andrew Jeffery]

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

On Fri, 2017-09-29 at 17:41 -0500, Eddie James wrote:
> From: "Edward A. James" <eajames@us.ibm.com>
> 
> Need some data to indicate to clients and the rest of the driver when
> the device is being removed, so add a cancel boolean. Fix up both the
> probe and remove functions to properly handle failures and prevent
> deadlocks.
> 
> Signed-off-by: Edward A. James <eajames@us.ibm.com>
> ---
>  drivers/fsi/occ.c | 86 +++++++++++++++++++++++++++++++++++++++----------------
>  1 file changed, 62 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/fsi/occ.c b/drivers/fsi/occ.c
> index a554550..550ae11 100644
> --- a/drivers/fsi/occ.c
> +++ b/drivers/fsi/occ.c
> @@ -46,6 +46,7 @@ struct occ {
>  	spinlock_t list_lock;		/* lock access to the xfrs list */
>  	struct mutex occ_lock;		/* lock access to the hardware */
>  	struct work_struct work;
> +	bool cancel;
>  };
>  
>  #define to_occ(x)	container_of((x), struct occ, mdev)
> @@ -117,12 +118,15 @@ struct occ_client {
>  
>  static DEFINE_IDA(occ_ida);
>  
> -static void occ_enqueue_xfr(struct occ_xfr *xfr)
> +static int occ_enqueue_xfr(struct occ_xfr *xfr)
>  {
>  	int empty;
>  	struct occ_client *client = to_client(xfr);
>  	struct occ *occ = client->occ;
>  
> +	if (occ->cancel)
> +		return -ECANCELED;
> +
>  	spin_lock_irq(&occ->list_lock);
>  
>  	empty = list_empty(&occ->xfrs);
> @@ -132,14 +136,20 @@ static void occ_enqueue_xfr(struct occ_xfr *xfr)
>  
>  	if (empty)
>  		queue_work(occ_wq, &occ->work);
> +
> +	return 0;
>  }
>  
>  static struct occ_client *occ_open_common(struct occ *occ, unsigned long flags)
>  {
> -	struct occ_client *client = kzalloc(sizeof(*client), GFP_KERNEL);
> +	struct occ_client *client;
> +
> +	if (occ->cancel)
> +		return ERR_PTR(-ECANCELED);
>  
> +	client = kzalloc(sizeof(*client), GFP_KERNEL);
>  	if (!client)
> -		return NULL;
> +		return ERR_PTR(-ENOMEM);
>  
>  	client->occ = occ;
>  	spin_lock_init(&client->lock);
> @@ -158,8 +168,8 @@ static int occ_open(struct inode *inode, struct file *file)
>  	struct occ *occ = to_occ(mdev);
>  
>  	client = occ_open_common(occ, file->f_flags);
> -	if (!client)
> -		return -ENOMEM;
> +	if (IS_ERR(client))
> +		return PTR_ERR(client);
>  
>  	file->private_data = client;
>  
> @@ -172,6 +182,7 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
>  	int rc;
>  	size_t bytes;
>  	struct occ_xfr *xfr = &client->xfr;
> +	struct occ *occ = client->occ;
>  
>  	if (len > OCC_SRAM_BYTES)
>  		return -EINVAL;
> @@ -204,7 +215,8 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
>  					      test_bit(XFR_COMPLETE,
>  						       &xfr->flags) ||
>  					      test_bit(XFR_CANCELED,
> -						       &xfr->flags));
> +						       &xfr->flags) ||
> +					      occ->cancel);
>  
>  		spin_lock_irq(&client->lock);
>  
> @@ -272,7 +284,7 @@ static ssize_t occ_write_common(struct occ_client *client,
>  
>  	spin_lock_irq(&client->lock);
>  
> -	if (test_and_set_bit(CLIENT_XFR_PENDING, &client->flags)) {
> +	if (test_bit(CLIENT_XFR_PENDING, &client->flags)) {
>  		rc = -EBUSY;
>  		goto done;
>  	}
> @@ -309,8 +321,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:
> @@ -579,6 +594,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);
> @@ -672,12 +690,17 @@ static void occ_worker(struct work_struct *work)
>  
>  struct occ_client *occ_drv_open(struct device *dev, unsigned long flags)
>  {
> +	struct occ_client *client;
>  	struct occ *occ = dev_get_drvdata(dev);
>  
>  	if (!occ)
>  		return NULL;
>  
> -	return occ_open_common(occ, flags);
> +	client = occ_open_common(occ, flags);
> +	if (IS_ERR(client))
> +		return NULL;
> +
> +	return client;
>  }
>  EXPORT_SYMBOL_GPL(occ_drv_open);
>  
> @@ -748,23 +771,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;
> @@ -774,20 +787,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_interruptible(&client->wait);

Again, would go for just wake_up() here, and maybe wake_up_all()?

Andrew

> +	}
> +	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 v2 6/9] drivers: fsi: occ: Add cancel to remove() and fix probe()

[Eddie James]

On 10/04/2017 08:07 PM, Andrew Jeffery wrote:
> On Fri, 2017-09-29 at 17:41 -0500, Eddie James wrote:
>> From: "Edward A. James" <eajames@us.ibm.com>
>>   
>> Need some data to indicate to clients and the rest of the driver when
>> the device is being removed, so add a cancel boolean. Fix up both the
>> probe and remove functions to properly handle failures and prevent
>> deadlocks.
>>   
>> Signed-off-by: Edward A. James <eajames@us.ibm.com>
>> ---
>>   drivers/fsi/occ.c | 86 +++++++++++++++++++++++++++++++++++++++----------------
>>   1 file changed, 62 insertions(+), 24 deletions(-)
>>   
>> diff --git a/drivers/fsi/occ.c b/drivers/fsi/occ.c
>> index a554550..550ae11 100644
>> --- a/drivers/fsi/occ.c
>> +++ b/drivers/fsi/occ.c
>> @@ -46,6 +46,7 @@ struct occ {
>>   	spinlock_t list_lock;		/* lock access to the xfrs list */
>>   	struct mutex occ_lock;		/* lock access to the hardware */
>>   	struct work_struct work;
>> +	bool cancel;
>>   };
>>   
>>   #define to_occ(x)	container_of((x), struct occ, mdev)
>> @@ -117,12 +118,15 @@ struct occ_client {
>>   
>>   static DEFINE_IDA(occ_ida);
>>   
>> -static void occ_enqueue_xfr(struct occ_xfr *xfr)
>> +static int occ_enqueue_xfr(struct occ_xfr *xfr)
>>   {
>>   	int empty;
>>   	struct occ_client *client = to_client(xfr);
>>   	struct occ *occ = client->occ;
>>   
>> +	if (occ->cancel)
>> +		return -ECANCELED;
>> +
>>   	spin_lock_irq(&occ->list_lock);
>>   
>>   	empty = list_empty(&occ->xfrs);
>> @@ -132,14 +136,20 @@ static void occ_enqueue_xfr(struct occ_xfr *xfr)
>>   
>>   	if (empty)
>>   		queue_work(occ_wq, &occ->work);
>> +
>> +	return 0;
>>   }
>>   
>>   static struct occ_client *occ_open_common(struct occ *occ, unsigned long flags)
>>   {
>> -	struct occ_client *client = kzalloc(sizeof(*client), GFP_KERNEL);
>> +	struct occ_client *client;
>> +
>> +	if (occ->cancel)
>> +		return ERR_PTR(-ECANCELED);
>>   
>> +	client = kzalloc(sizeof(*client), GFP_KERNEL);
>>   	if (!client)
>> -		return NULL;
>> +		return ERR_PTR(-ENOMEM);
>>   
>>   	client->occ = occ;
>>   	spin_lock_init(&client->lock);
>> @@ -158,8 +168,8 @@ static int occ_open(struct inode *inode, struct file *file)
>>   	struct occ *occ = to_occ(mdev);
>>   
>>   	client = occ_open_common(occ, file->f_flags);
>> -	if (!client)
>> -		return -ENOMEM;
>> +	if (IS_ERR(client))
>> +		return PTR_ERR(client);
>>   
>>   	file->private_data = client;
>>   
>> @@ -172,6 +182,7 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
>>   	int rc;
>>   	size_t bytes;
>>   	struct occ_xfr *xfr = &client->xfr;
>> +	struct occ *occ = client->occ;
>>   
>>   	if (len > OCC_SRAM_BYTES)
>>   		return -EINVAL;
>> @@ -204,7 +215,8 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
>>   					      test_bit(XFR_COMPLETE,
>>   						       &xfr->flags) ||
>>   					      test_bit(XFR_CANCELED,
>> -						       &xfr->flags));
>> +						       &xfr->flags) ||
>> +					      occ->cancel);
>>   
>>   		spin_lock_irq(&client->lock);
>>   
>> @@ -272,7 +284,7 @@ static ssize_t occ_write_common(struct occ_client *client,
>>   
>>   	spin_lock_irq(&client->lock);
>>   
>> -	if (test_and_set_bit(CLIENT_XFR_PENDING, &client->flags)) {
>> +	if (test_bit(CLIENT_XFR_PENDING, &client->flags)) {
>>   		rc = -EBUSY;
>>   		goto done;
>>   	}
>> @@ -309,8 +321,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:
>> @@ -579,6 +594,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);
>> @@ -672,12 +690,17 @@ static void occ_worker(struct work_struct *work)
>>   
>>   struct occ_client *occ_drv_open(struct device *dev, unsigned long flags)
>>   {
>> +	struct occ_client *client;
>>   	struct occ *occ = dev_get_drvdata(dev);
>>   
>>   	if (!occ)
>>   		return NULL;
>>   
>> -	return occ_open_common(occ, flags);
>> +	client = occ_open_common(occ, flags);
>> +	if (IS_ERR(client))
>> +		return NULL;
>> +
>> +	return client;
>>   }
>>   EXPORT_SYMBOL_GPL(occ_drv_open);
>>   
>> @@ -748,23 +771,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;
>> @@ -774,20 +787,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_interruptible(&client->wait);
> Again, would go for just wake_up() here, and maybe wake_up_all()?

Sure, though again, there are no non-interruptible wait_events in the 
driver. We should switch to _all() though, thanks.

Thanks,
Eddie

>
> Andrew
>
>> +	}
>> +	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;

[PATCH linux dev-4.10 v2 7/9] 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 550ae11..c3d9976 100644
--- a/drivers/fsi/occ.c
+++ b/drivers/fsi/occ.c
@@ -70,15 +70,11 @@ struct occ_response {
  *  and cleared if the transfer fails or occ_worker_getsram completes.
  * XFR_COMPLETE is set when a transfer fails or finishes occ_worker_getsram.
  * XFR_CANCELED is set when the transfer's client is released.
- * XFR_WAITING is set from read() if the transfer isn't complete and
- *  O_NONBLOCK wasn't specified. Cleared in read() when transfer completes or
- *  fails.
  */
 enum {
 	XFR_IN_PROGRESS,
 	XFR_COMPLETE,
 	XFR_CANCELED,
-	XFR_WAITING,
 };
 
 struct occ_xfr {
@@ -104,6 +100,7 @@ enum {
 };
 
 struct occ_client {
+	struct kref kref;
 	struct occ *occ;
 	struct occ_xfr xfr;
 	spinlock_t lock;		/* lock access to the client state */
@@ -140,6 +137,24 @@ static int occ_enqueue_xfr(struct occ_xfr *xfr)
 	return 0;
 }
 
+static void occ_get_client(struct occ_client *client)
+{
+	kref_get(&client->kref);
+}
+
+static void occ_client_release(struct kref *kref)
+{
+	struct occ_client *client = container_of(kref, struct occ_client,
+						 kref);
+
+	kfree(client);
+}
+
+static void occ_put_client(struct occ_client *client)
+{
+	kref_put(&client->kref, occ_client_release);
+}
+
 static struct occ_client *occ_open_common(struct occ *occ, unsigned long flags)
 {
 	struct occ_client *client;
@@ -152,6 +167,7 @@ static struct occ_client *occ_open_common(struct occ *occ, unsigned long flags)
 		return ERR_PTR(-ENOMEM);
 
 	client->occ = occ;
+	kref_init(&client->kref);
 	spin_lock_init(&client->lock);
 	init_waitqueue_head(&client->wait);
 
@@ -187,6 +203,7 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
 	if (len > OCC_SRAM_BYTES)
 		return -EINVAL;
 
+	occ_get_client(client);
 	spin_lock_irq(&client->lock);
 
 	if (!test_bit(CLIENT_XFR_PENDING, &client->flags)) {
@@ -207,8 +224,6 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
 			goto done;
 		}
 
-		set_bit(XFR_WAITING, &xfr->flags);
-
 		spin_unlock_irq(&client->lock);
 
 		rc = wait_event_interruptible(client->wait,
@@ -220,15 +235,12 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
 
 		spin_lock_irq(&client->lock);
 
-		if (test_bit(XFR_CANCELED, &xfr->flags)) {
-			spin_unlock_irq(&client->lock);
-			kfree(client);
-			return -EBADFD;
-		}
-
-		clear_bit(XFR_WAITING, &xfr->flags);
 		if (!test_bit(XFR_COMPLETE, &xfr->flags)) {
-			rc = -EINTR;
+			if (occ->cancel || test_bit(XFR_CANCELED, &xfr->flags))
+				rc = -ECANCELED;
+			else
+				rc = -EINTR;
+
 			goto done;
 		}
 	}
@@ -259,6 +271,7 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
 
 done:
 	spin_unlock_irq(&client->lock);
+	occ_put_client(client);
 	return rc;
 }
 
@@ -282,6 +295,7 @@ static ssize_t occ_write_common(struct occ_client *client,
 	if (len > (OCC_CMD_DATA_BYTES + 3) || len < 3)
 		return -EINVAL;
 
+	occ_get_client(client);
 	spin_lock_irq(&client->lock);
 
 	if (test_bit(CLIENT_XFR_PENDING, &client->flags)) {
@@ -330,6 +344,7 @@ static ssize_t occ_write_common(struct occ_client *client,
 
 done:
 	spin_unlock_irq(&client->lock);
+	occ_put_client(client);
 	return rc;
 }
 
@@ -348,38 +363,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_interruptible(&client->wait);
+
+done:
 	spin_unlock_irq(&client->lock);
+
+	occ_put_client(client);
 	return 0;
 }
 
@@ -582,7 +585,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);
@@ -605,6 +608,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);
 
@@ -660,29 +665,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 v2 7/9] drivers: fsi: occ: Fix client memory management

[Andrew Jeffery]

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

On Fri, 2017-09-29 at 17:41 -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>
> ---
>  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 550ae11..c3d9976 100644
> --- a/drivers/fsi/occ.c
> +++ b/drivers/fsi/occ.c
> @@ -70,15 +70,11 @@ struct occ_response {
>   *  and cleared if the transfer fails or occ_worker_getsram completes.
>   * XFR_COMPLETE is set when a transfer fails or finishes occ_worker_getsram.
>   * XFR_CANCELED is set when the transfer's client is released.
> - * XFR_WAITING is set from read() if the transfer isn't complete and
> - *  O_NONBLOCK wasn't specified. Cleared in read() when transfer completes or
> - *  fails.
>   */
>  enum {
>  	XFR_IN_PROGRESS,
>  	XFR_COMPLETE,
>  	XFR_CANCELED,
> -	XFR_WAITING,
>  };
>  
>  struct occ_xfr {
> @@ -104,6 +100,7 @@ enum {
>  };
>  
>  struct occ_client {
> +	struct kref kref;
>  	struct occ *occ;
>  	struct occ_xfr xfr;
>  	spinlock_t lock;		/* lock access to the client state */
> @@ -140,6 +137,24 @@ static int occ_enqueue_xfr(struct occ_xfr *xfr)
>  	return 0;
>  }
>  
> +static void occ_get_client(struct occ_client *client)
> +{
> +	kref_get(&client->kref);
> +}
> +
> +static void occ_client_release(struct kref *kref)
> +{
> +	struct occ_client *client = container_of(kref, struct occ_client,
> +						 kref);
> +
> +	kfree(client);
> +}
> +
> +static void occ_put_client(struct occ_client *client)
> +{
> +	kref_put(&client->kref, occ_client_release);
> +}
> +
>  static struct occ_client *occ_open_common(struct occ *occ, unsigned long flags)
>  {
>  	struct occ_client *client;
> @@ -152,6 +167,7 @@ static struct occ_client *occ_open_common(struct occ *occ, unsigned long flags)
>  		return ERR_PTR(-ENOMEM);
>  
>  	client->occ = occ;
> +	kref_init(&client->kref);
>  	spin_lock_init(&client->lock);
>  	init_waitqueue_head(&client->wait);
>  
> @@ -187,6 +203,7 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
>  	if (len > OCC_SRAM_BYTES)
>  		return -EINVAL;
>  
> +	occ_get_client(client);
>  	spin_lock_irq(&client->lock);
>  
>  	if (!test_bit(CLIENT_XFR_PENDING, &client->flags)) {
> @@ -207,8 +224,6 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
>  			goto done;
>  		}
>  
> -		set_bit(XFR_WAITING, &xfr->flags);
> -
>  		spin_unlock_irq(&client->lock);
>  
>  		rc = wait_event_interruptible(client->wait,
> @@ -220,15 +235,12 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
>  
>  		spin_lock_irq(&client->lock);
>  
> -		if (test_bit(XFR_CANCELED, &xfr->flags)) {
> -			spin_unlock_irq(&client->lock);
> -			kfree(client);
> -			return -EBADFD;
> -		}
> -
> -		clear_bit(XFR_WAITING, &xfr->flags);
>  		if (!test_bit(XFR_COMPLETE, &xfr->flags)) {
> -			rc = -EINTR;
> +			if (occ->cancel || test_bit(XFR_CANCELED, &xfr->flags))
> +				rc = -ECANCELED;
> +			else
> +				rc = -EINTR;
> +
>  			goto done;
>  		}
>  	}
> @@ -259,6 +271,7 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
>  
>  done:
>  	spin_unlock_irq(&client->lock);
> +	occ_put_client(client);
>  	return rc;
>  }
>  
> @@ -282,6 +295,7 @@ static ssize_t occ_write_common(struct occ_client *client,
>  	if (len > (OCC_CMD_DATA_BYTES + 3) || len < 3)
>  		return -EINVAL;
>  
> +	occ_get_client(client);
>  	spin_lock_irq(&client->lock);
>  
>  	if (test_bit(CLIENT_XFR_PENDING, &client->flags)) {
> @@ -330,6 +344,7 @@ static ssize_t occ_write_common(struct occ_client *client,
>  
>  done:
>  	spin_unlock_irq(&client->lock);
> +	occ_put_client(client);
>  	return rc;
>  }
>  
> @@ -348,38 +363,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_interruptible(&client->wait);

I think we we want to be doing wake_up_all() here too?

Andrew

> +
> +done:
>  	spin_unlock_irq(&client->lock);
> +
> +	occ_put_client(client);
>  	return 0;
>  }
>  
> @@ -582,7 +585,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);
> @@ -605,6 +608,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);
>  
> @@ -660,29 +665,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 --]

[PATCH linux dev-4.10 v2 8/9] drivers/hwmon/occ: Remove repeated ops for OCC command in progress

[Eddie James]

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

This is now handled in the occ driver.

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

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

Re: [PATCH linux dev-4.10 v2 8/9] drivers/hwmon/occ: Remove repeated ops for OCC command in progress

[Andrew Jeffery]

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

On Fri, 2017-09-29 at 17:41 -0500, Eddie James wrote:
> From: "Edward A. James" <eajames@us.ibm.com>
> 
> This is now handled in the occ driver.
> 
> Signed-off-by: Edward A. James <eajames@us.ibm.com>

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

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

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

[PATCH linux dev-4.10 v2 9/9] 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 | 33 ++++++++++++++++++++++++++-------
 1 file changed, 26 insertions(+), 7 deletions(-)

diff --git a/drivers/hwmon/occ/p9_sbe.c b/drivers/hwmon/occ/p9_sbe.c
index c7e0d9c..ffd6829 100644
--- a/drivers/hwmon/occ/p9_sbe.c
+++ b/drivers/hwmon/occ/p9_sbe.c
@@ -14,37 +14,53 @@
 #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);
+	p9_sbe_occ->client = occ_drv_open(p9_sbe_occ->sbe, 0);
+	if (!p9_sbe_occ->client) {
 		rc = -ENODEV;
+		spin_unlock_irq(&p9_sbe_occ->lock);
 		goto assign;
 	}
+	spin_unlock_irq(&p9_sbe_occ->lock);
 
-	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 +88,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 +148,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 +169,9 @@ 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_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 v2 9/9] drivers: hwmon: occ: Cancel occ operations in remove()

[Andrew Jeffery]

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

On Fri, 2017-09-29 at 17:41 -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 | 33 ++++++++++++++++++++++++++-------
>  1 file changed, 26 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/hwmon/occ/p9_sbe.c b/drivers/hwmon/occ/p9_sbe.c
> index c7e0d9c..ffd6829 100644
> --- a/drivers/hwmon/occ/p9_sbe.c
> +++ b/drivers/hwmon/occ/p9_sbe.c
> @@ -14,37 +14,53 @@
>  #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);

I think we should be using spin_lock_irqsave(), otherwise
spin_unlock_irq() will unconditionally re-enable interrupts.

> +	p9_sbe_occ->client = occ_drv_open(p9_sbe_occ->sbe, 0);
> +	if (!p9_sbe_occ->client) {
>  		rc = -ENODEV;
> +		spin_unlock_irq(&p9_sbe_occ->lock);
>  		goto assign;
>  	}
> +	spin_unlock_irq(&p9_sbe_occ->lock);

Rather than this dance with locking, you can do:

    spin_lock_irqsave(&p9_sbe_occ->lock, flags);
    p9_sbe_occ->client = occ_drv_open(p9_sbe_occ->sbe, 0);
spin_unlock_irqrestore(&p9_sbe_occ->lock, flags);
if (!p9_sbe_occ->client) {
        ...
}

Please take a look at the rest of the code for this pattern as well.

>  
> -	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 +88,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 +148,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 +169,9 @@ 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_close_client(p9_sbe_occ);
>  	occ_remove_status_attrs(occ);

Shouldn't we be removing the status attributes before releasing the occ
to prevent use-after-free or useless calls into read/write? This seems
racy.

Andrew

>  
>  	atomic_dec(&occ_num_occs);

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

Re: [PATCH linux dev-4.10 v2 9/9] drivers: hwmon: occ: Cancel occ operations in remove()

[Eddie James]

On 10/04/2017 08:20 PM, Andrew Jeffery wrote:
> On Fri, 2017-09-29 at 17:41 -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 | 33 ++++++++++++++++++++++++++-------
>>   1 file changed, 26 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/hwmon/occ/p9_sbe.c b/drivers/hwmon/occ/p9_sbe.c
>> index c7e0d9c..ffd6829 100644
>> --- a/drivers/hwmon/occ/p9_sbe.c
>> +++ b/drivers/hwmon/occ/p9_sbe.c
>> @@ -14,37 +14,53 @@
>>   #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);
> I think we should be using spin_lock_irqsave(), otherwise
> spin_unlock_irq() will unconditionally re-enable interrupts.

This is all over the sbefifo and occ drivers too... do we need to change 
those? No one has mentioned anything about it.

>
>> +	p9_sbe_occ->client = occ_drv_open(p9_sbe_occ->sbe, 0);
>> +	if (!p9_sbe_occ->client) {
>>   		rc = -ENODEV;
>> +		spin_unlock_irq(&p9_sbe_occ->lock);
>>   		goto assign;
>>   	}
>> +	spin_unlock_irq(&p9_sbe_occ->lock);
> Rather than this dance with locking, you can do:
>
>      spin_lock_irqsave(&p9_sbe_occ->lock, flags);
>      p9_sbe_occ->client = occ_drv_open(p9_sbe_occ->sbe, 0);
> spin_unlock_irqrestore(&p9_sbe_occ->lock, flags);
> if (!p9_sbe_occ->client) {
>          ...
> }
>
> Please take a look at the rest of the code for this pattern as well.
>
>>   
>> -	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 +88,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 +148,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 +169,9 @@ 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_close_client(p9_sbe_occ);
>>   	occ_remove_status_attrs(occ);
> Shouldn't we be removing the status attributes before releasing the occ
> to prevent use-after-free or useless calls into read/write? This seems
> racy.

This is to prevent hanging. If we're stuck in a sysfs read(), I believe 
the remove will wait for the read to complete... can't happen if we 
don't close the client and cancel the op first. The 
p9_occ_sbe_send_command function is quite careful about not using the 
client after it's been released.

Besides, hwmon attributes are always removed after remove() is complete 
and the device is released.

It would be a good idea to set the p9_sbe_occ->sbe pointer to NULL so 
that we can't start any new operations during remove though.

Thanks,
Eddie

>
> Andrew
>
>>   
>>   	atomic_dec(&occ_num_occs);