drivers/fsi: Fixup client remove functions

From: "Edward A. James" <[email protected]>

Includes various fixes:
- check for complete while waiting for data in read()
- fix probe if probe fails
- general cleanup
- slightly safer (earlier) get_client()
- reorder some of the remove() operations for safety

Signed-off-by Edward A. James <[email protected]>

---
  drivers/fsi/fsi-sbefifo.c   | 329 ++++++++++++++++++++++++--------------------
  include/linux/fsi-sbefifo.h |   6 +-
  2 files changed, 180 insertions(+), 155 deletions(-)

drivers/fsi/fsi-sbefifo.c +177 -152

index 1c37ff7..5d25ade 100644
@@ -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,14 +51,15 @@
  #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;
-	struct miscdevice mdev;
+	struct fsi_device *fsi_dev;	/* parent fsi device */
+	struct miscdevice mdev;		/* /dev/ entry */
  	wait_queue_head_t wait;
-	struct list_head link;
  	struct list_head xfrs;
-	struct kref kref;
+	struct kref kref;		/* reference counter */
  	spinlock_t lock;
  	char name[32];
  	int idx;

@@ -87,14 +95,12 @@ 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)
  {
  	int rc;
-	u32 raw_word;
+	__be32 raw_word;
 
  	rc = fsi_device_read(sbefifo->fsi_dev, reg, &raw_word,
  			     sizeof(raw_word));

@@ -102,17 +108,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 +144,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 +218,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 +238,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);
  }
 

@@ -255,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;

@@ -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;
 

@@ -309,10 +312,11 @@ static struct sbefifo_client *sbefifo_new_client(struct sbefifo *sbefifo)
 
  static void sbefifo_client_release(struct kref *kref)
  {
-	struct sbefifo_client *client;
  	struct sbefifo_xfr *xfr;
+	struct sbefifo_client *client = container_of(kref,
+						     struct sbefifo_client,
+						     kref);
 
-	client = container_of(kref, struct sbefifo_client, kref);
  	list_for_each_entry(xfr, &client->xfrs, client) {
  		/*
  		 * The client left with pending or running xfrs.

@@ -349,6 +353,7 @@ static struct sbefifo_xfr *sbefifo_next_xfr(struct sbefifo *sbefifo)
  			kfree(xfr);
  			continue;
  		}
+
  		return xfr;
  	}
 

@@ -370,7 +375,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 +393,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 +401,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 +418,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 +441,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 +469,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 +479,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 +500,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 +538,19 @@ 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)
+{
+	struct sbefifo_xfr *xfr = list_first_entry_or_null(&client->xfrs,
+							   struct sbefifo_xfr,
+							   client);
+
+	*n = sbefifo_buf_nbreadable(&client->rbuf);
+
+	return READ_ONCE(sbefifo->rc) || *n ||
+		(xfr && test_bit(SBEFIFO_XFR_COMPLETE, &xfr->flags));
+}
+
  static ssize_t sbefifo_read_common(struct sbefifo_client *client,
  				   char __user *ubuf, char *kbuf, size_t len)
  {

@@ -551,30 +567,38 @@ 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 = -ERESTARTSYS;
+		goto out;
  	}
+
+	ret = READ_ONCE(sbefifo->rc);
  	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 */
+			wake_up(&sbefifo->wait);
+			goto out;
+		}
+
  		if (!test_bit(SBEFIFO_XFR_COMPLETE, &xfr->flags)) {
  			/*
  			 * Fill the read buffer back up.

@@ -589,22 +613,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 +645,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 +656,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);
-	if (!xfr) {
+	xfr = sbefifo_enq_xfr(client);		/* this xfr queued up */
+	if (IS_ERR(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 +683,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) {
+
+		ret = READ_ONCE(sbefifo->rc);
+		if (ret) {
  			INIT_LIST_HEAD(&client->xfrs);
-			sbefifo_put_client(client);
-			return 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 +736,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);
  }
 

@@ -742,18 +771,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;
  }

@@ -802,95 +828,92 @@ 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;
  	}
 
-	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);
+		 sbefifo->idx);
 
  	/* 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(&sbefifo->fsi_dev->dev,
-					 "failed to create child node dev\n");
-		}
+		    (unsigned long)sbefifo);
+
+	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;
+	}
+
+	/* 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);
  	}
 
-	list_add(&sbefifo->link, &sbefifo_fifos);
-	
-	return misc_register(&sbefifo->mdev);
+	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;
-
-		device_for_each_child(dev, NULL, sbefifo_unregister_child);
+	WRITE_ONCE(sbefifo->rc, -ENODEV);
+	wake_up(&sbefifo->wait);
 
-		misc_deregister(&sbefifo->mdev);
-		list_del(&sbefifo->link);
-		ida_simple_remove(&sbefifo_ida, sbefifo->idx);
+	misc_deregister(&sbefifo->mdev);
+	device_for_each_child(dev, NULL, sbefifo_unregister_child);
 
-		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;
  }

@@ -915,17 +938,19 @@ static int sbefifo_remove(struct device *dev)
 
  static int sbefifo_init(void)
  {
-	INIT_LIST_HEAD(&sbefifo_fifos);
  	return fsi_driver_register(&sbefifo_drv);
  }
 
  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 <[email protected]>");
-MODULE_DESCRIPTION("Linux device interface to the POWER self boot engine");
+MODULE_AUTHOR("Eddie James <[email protected]>");
+MODULE_DESCRIPTION("Linux device interface to the POWER Self Boot Engine");

include/linux/fsi-sbefifo.h +3 -3

index 1b46c63..8e55891 100644
@@ -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 */

From: "Edward A. James" <[email protected]>

Includes various fixes:
 - check for complete while waiting for data in read()
 - fix probe if probe fails
 - general cleanup
 - slightly safer (earlier) get_client()
 - reorder some of the remove() operations for safety

Signed-off-by Edward A. James <[email protected]>

---
 drivers/fsi/fsi-sbefifo.c   | 329 ++++++++++++++++++++++++-------------------- include/linux/fsi-sbefifo.h |   6 +-

 2 files changed, 180 insertions(+), 155 deletions(-)

drivers/fsi/fsi-sbefifo.c +0 -0

index 1c37ff7..5d25ade 100644
@@ -11,20 +11,27 @@
   * GNU General Public License for more details.
   */

drivers/fsi/fsi-sbefifo.c +1 -1

-#include <linux/delay.h>
+#include <linux/device.h>

 #include <linux/errno.h>

drivers/fsi/fsi-sbefifo.c +1 -1

-#include <linux/idr.h>
+#include <linux/fs.h>

 #include <linux/fsi.h>

drivers/fsi/fsi-sbefifo.c +4 -0

+#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

drivers/fsi/fsi-sbefifo.c +0 -0

@@ -44,14 +51,15 @@
  #define   SBEFIFO_EOT_MAGIC		0xffffffff
  #define SBEFIFO_EOT_ACK		0x14

drivers/fsi/fsi-sbefifo.c +2 -0

+#define SBEFIFO_RESCHEDULE	msecs_to_jiffies(500)
+

 struct sbefifo {
  struct timer_list poll_timer;

 static DEFINE_IDA(sbefifo_ida);

 static int sbefifo_inw(struct sbefifo *sbefifo, int reg, u32 *word) {
  int rc;

drivers/fsi/fsi-sbefifo.c +1 -1

-	u32 raw_word;
+	__be32 raw_word;

  rc = fsi_device_read(sbefifo->fsi_dev, reg, &raw_word, sizeof(raw_word));

drivers/fsi/fsi-sbefifo.c +0 -0

@@ -102,17 +108,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) {

drivers/fsi/fsi-sbefifo.c +1 -1

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

drivers/fsi/fsi-sbefifo.c +0 -0

@@ -136,7 +144,7 @@ static int sbefifo_ack_eot(struct sbefifo *sbefifo)
  		return ret;

  return sbefifo_outw(sbefifo, SBEFIFO_DWN | SBEFIFO_EOT_ACK,

drivers/fsi/fsi-sbefifo.c +1 -1

-			SBEFIFO_EOT_MAGIC);
+			    SBEFIFO_EOT_MAGIC);

 }

 static size_t sbefifo_dev_nwreadable(u32 sts)

drivers/fsi/fsi-sbefifo.c +0 -0

@@ -210,6 +218,7 @@ static bool sbefifo_buf_readnb(struct sbefifo_buf *buf, size_t n)
  		rpos = buf->buf;

  WRITE_ONCE(buf->rpos, rpos);
+
  return rpos == wpos;
 }

drivers/fsi/fsi-sbefifo.c +0 -0

@@ -229,14 +238,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)
 {

drivers/fsi/fsi-sbefifo.c +1 -1

-	struct sbefifo *sbefifo;
+	struct sbefifo *sbefifo = container_of(kref, struct sbefifo, kref);

drivers/fsi/fsi-sbefifo.c +0 -1

-	sbefifo = container_of(kref, struct sbefifo, kref);

  kfree(sbefifo);
 }

drivers/fsi/fsi-sbefifo.c +0 -0

@@ -255,9 +264,12 @@ static struct sbefifo_xfr *sbefifo_enq_xfr(struct sbefifo_client *client)
  	struct sbefifo *sbefifo = client->dev;
  	struct sbefifo_xfr *xfr;

+
  xfr = kzalloc(sizeof(*xfr), GFP_KERNEL);
  if (!xfr)

drivers/fsi/fsi-sbefifo.c +1 -1

-		return NULL;
+		return ERR_PTR(-ENOMEM);

  xfr->rbuf = &client->rbuf;
  xfr->wbuf = &client->wbuf;

drivers/fsi/fsi-sbefifo.c +0 -0

@@ -267,21 +279,12 @@ static struct sbefifo_xfr *sbefifo_enq_xfr(struct sbefifo_client *client)
  	return xfr;
  }

drivers/fsi/fsi-sbefifo.c +0 -10

-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) {

drivers/fsi/fsi-sbefifo.c +3 -1

-	struct sbefifo_xfr *xfr;
+	struct sbefifo_xfr *xfr = list_first_entry_or_null(&client->xfrs,
+							   struct sbefifo_xfr,
+							   client);

drivers/fsi/fsi-sbefifo.c +0 -1

-	xfr = sbefifo_client_next_xfr(client);

  if (xfr && test_bit(SBEFIFO_XFR_RESP_PENDING, &xfr->flags)) return true;

drivers/fsi/fsi-sbefifo.c +0 -0

@@ -309,10 +312,11 @@ static struct sbefifo_client *sbefifo_new_client(struct sbefifo *sbefifo)

 static void sbefifo_client_release(struct kref *kref) {

drivers/fsi/fsi-sbefifo.c +0 -1

-	struct sbefifo_client *client;

  struct sbefifo_xfr *xfr;

drivers/fsi/fsi-sbefifo.c +3 -0

+	struct sbefifo_client *client = container_of(kref,
+						     struct sbefifo_client,
+						     kref);

  list_for_each_entry(xfr, &client->xfrs, client) { /*
   * The client left with pending or running xfrs.

drivers/fsi/fsi-sbefifo.c +1 -0

@@ -349,6 +353,7 @@ static struct sbefifo_xfr *sbefifo_next_xfr(struct sbefifo *sbefifo)
  			kfree(xfr);
  			continue;
  		}
+
  		return xfr;
  	}

drivers/fsi/fsi-sbefifo.c +0 -0

@@ -370,7 +375,7 @@ static void sbefifo_poll_timer(unsigned long data)

  spin_lock(&sbefifo->lock);
  xfr = list_first_entry_or_null(&sbefifo->xfrs, struct sbefifo_xfr,

drivers/fsi/fsi-sbefifo.c +1 -1

-			xfrs);
+				       xfrs);

  if (!xfr)
  goto out_unlock;

drivers/fsi/fsi-sbefifo.c +0 -0

@@ -388,8 +393,7 @@ static void sbefifo_poll_timer(unsigned long data)

   /* Drain the write buffer. */
  while ((bufn = sbefifo_buf_nbreadable(wbuf))) {

drivers/fsi/fsi-sbefifo.c +1 -2

-		ret = sbefifo_inw(sbefifo, SBEFIFO_UP | SBEFIFO_STS,
-				&sts);
+		ret = sbefifo_inw(sbefifo, SBEFIFO_UP | SBEFIFO_STS, &sts);

  if (ret)
  goto out;

drivers/fsi/fsi-sbefifo.c +0 -0

@@ -397,7 +401,7 @@ static void sbefifo_poll_timer(unsigned long data)
  		if (devn == 0) {
  			/* No open slot for write.  Reschedule. */

  sbefifo->poll_timer.expires = jiffies +

drivers/fsi/fsi-sbefifo.c +1 -1

-				msecs_to_jiffies(500);
+				SBEFIFO_RESCHEDULE;

  add_timer(&sbefifo->poll_timer);

  goto out_unlock;
  }

drivers/fsi/fsi-sbefifo.c +0 -0

@@ -414,9 +418,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)) {

drivers/fsi/fsi-sbefifo.c +2 -3

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

drivers/fsi/fsi-sbefifo.c +0 -0

@@ -438,7 +441,7 @@ static void sbefifo_poll_timer(unsigned long data)
  		if (devn == 0) {
  			/* No data yet.  Reschedule. */

  sbefifo->poll_timer.expires = jiffies +

drivers/fsi/fsi-sbefifo.c +1 -1

-				msecs_to_jiffies(500);
+				SBEFIFO_RESCHEDULE;

  add_timer(&sbefifo->poll_timer); goto out_unlock;
  }

drivers/fsi/fsi-sbefifo.c +3 -3

@@ -466,7 +469,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 +479,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 +500,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;

drivers/fsi/fsi-sbefifo.c +0 -0

@@ -535,6 +538,19 @@ static unsigned int sbefifo_poll(struct file *file, poll_table *wait)
  	return mask;
  }

drivers/fsi/fsi-sbefifo.c +13 -0

+static bool sbefifo_read_ready(struct sbefifo *sbefifo,
+			       struct sbefifo_client *client, size_t *n)
+{
+	struct sbefifo_xfr *xfr = list_first_entry_or_null(&client->xfrs,
+							   struct sbefifo_xfr,
+							   client);
+
+	*n = sbefifo_buf_nbreadable(&client->rbuf);
+
+	return READ_ONCE(sbefifo->rc) || *n ||
+		(xfr && test_bit(SBEFIFO_XFR_COMPLETE, &xfr->flags));
+}
+

 static ssize_t sbefifo_read_common(struct sbefifo_client *client, char __user *ubuf, char *kbuf, size_t len) {

drivers/fsi/fsi-sbefifo.c +0 -0

@@ -551,30 +567,38 @@ static ssize_t sbefifo_read_common(struct sbefifo_client *client,

  sbefifo_get_client(client);
  if (wait_event_interruptible(sbefifo->wait,

drivers/fsi/fsi-sbefifo.c +4 -5

-				(ret = READ_ONCE(sbefifo->rc)) ||
-				(n = sbefifo_buf_nbreadable(
-						&client->rbuf)))) {
-		sbefifo_put_client(client);
-		return -ERESTARTSYS;
+				     sbefifo_read_ready(sbefifo, client,
+							&n))) {
+		ret = -ERESTARTSYS;
+		goto out;

  }

drivers/fsi/fsi-sbefifo.c +2 -0

+
+	ret = READ_ONCE(sbefifo->rc);

  if (ret) {
  INIT_LIST_HEAD(&client->xfrs);

drivers/fsi/fsi-sbefifo.c +1 -2

-		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)) {

drivers/fsi/fsi-sbefifo.c +2 -2

-			sbefifo_put_client(client);
-			return -EFAULT;
+			ret = -EFAULT;
+			goto out;

  }

drivers/fsi/fsi-sbefifo.c +1 -1

-	} else
+	} else {

  memcpy(kbuf, READ_ONCE(client->rbuf.rpos), n);

drivers/fsi/fsi-sbefifo.c +1 -0

+	}

  if (sbefifo_buf_readnb(&client->rbuf, n)) {

drivers/fsi/fsi-sbefifo.c +4 -0

+			wake_up(&sbefifo->wait);
+			goto out;
+		}
+

  if (!test_bit(SBEFIFO_XFR_COMPLETE, &xfr->flags)) { /*
   * Fill the read buffer back up.

drivers/fsi/fsi-sbefifo.c +0 -0

@@ -589,22 +613,31 @@ static ssize_t sbefifo_read_common(struct sbefifo_client *client,
  		}
  	}

drivers/fsi/fsi-sbefifo.c +1 -1

-	sbefifo_put_client(client);
+	ret = n;

drivers/fsi/fsi-sbefifo.c +3 -1

-	return n;
+out:
+	sbefifo_put_client(client);
+	return ret;

 }

drivers/fsi/fsi-sbefifo.c +2 -2

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

drivers/fsi/fsi-sbefifo.c +2 -1

-	WARN_ON(*offset);
+	return sbefifo_read_common(client, buf, NULL, len);
+}

drivers/fsi/fsi-sbefifo.c +7 -2

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

drivers/fsi/fsi-sbefifo.c +2 -1

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

drivers/fsi/fsi-sbefifo.c +1 -0

@@ -612,6 +645,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 +656,26 @@ static ssize_t sbefifo_write_common(struct sbefifo_client *client,
  	if (!len)
  		return 0;

drivers/fsi/fsi-sbefifo.c +2 -1

-	n = sbefifo_buf_nbwriteable(&client->wbuf);
+	sbefifo_get_client(client);
+	n = sbefifo_buf_nbwriteable(wbuf);

  spin_lock_irq(&sbefifo->lock);

drivers/fsi/fsi-sbefifo.c +0 -1

-	xfr = sbefifo_next_xfr(sbefifo);

drivers/fsi/fsi-sbefifo.c +1 -0

+	xfr = sbefifo_next_xfr(sbefifo);	/* next xfr to be executed */

  if ((client->f_flags & O_NONBLOCK) && xfr && n < len) { spin_unlock_irq(&sbefifo->lock);

drivers/fsi/fsi-sbefifo.c +2 -1

-		return -EAGAIN;
+		ret = -EAGAIN;
+		goto out;

  }

drivers/fsi/fsi-sbefifo.c +2 -2

-	xfr = sbefifo_enq_xfr(client);
-	if (!xfr) {
+	xfr = sbefifo_enq_xfr(client);		/* this xfr queued up */
+	if (IS_ERR(xfr)) {

  spin_unlock_irq(&sbefifo->lock);

drivers/fsi/fsi-sbefifo.c +2 -1

-		return -ENOMEM;
+		ret = PTR_ERR(xfr);
+		goto out;

  }

drivers/fsi/fsi-sbefifo.c +0 -1

-	spin_unlock_irq(&sbefifo->lock);

drivers/fsi/fsi-sbefifo.c +1 -1

-	sbefifo_get_client(client);
+	spin_unlock_irq(&sbefifo->lock);

  /*
   * Partial writes are not really allowed in that EOT is sent exactly

drivers/fsi/fsi-sbefifo.c +9 -10

@@ -647,49 +683,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) {
+
+		ret = READ_ONCE(sbefifo->rc);
+		if (ret) {
  			INIT_LIST_HEAD(&client->xfrs);
-			sbefifo_put_client(client);
-			return sbefifo->rc;
+			goto out;
  		}

  n = min_t(size_t, n, len);

  if (ubuf) {

drivers/fsi/fsi-sbefifo.c +1 -2

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

drivers/fsi/fsi-sbefifo.c +2 -2

-				sbefifo_put_client(client);
-				return -EFAULT;
+				ret = -EFAULT;
+				goto out;

  }

  ubuf += n;
  } else {

drivers/fsi/fsi-sbefifo.c +1 -1

-			memcpy(READ_ONCE(client->wbuf.wpos), kbuf, n);
+			memcpy(READ_ONCE(wbuf->wpos), kbuf, n);

  kbuf += n;
  }

drivers/fsi/fsi-sbefifo.c +1 -1

-		sbefifo_buf_wrotenb(&client->wbuf, n);
+		sbefifo_buf_wrotenb(wbuf, n);

  len -= n;
  ret += n;

drivers/fsi/fsi-sbefifo.c +2 -2

-		/* 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);

drivers/fsi/fsi-sbefifo.c +0 -0

@@ -702,21 +736,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,

drivers/fsi/fsi-sbefifo.c +1 -1

-		size_t len, loff_t *offset)
+			     size_t len, loff_t *offset)

 {
  struct sbefifo_client *client = file->private_data;

drivers/fsi/fsi-sbefifo.c +0 -5

-	WARN_ON(*offset);
-
-	if (!access_ok(VERIFY_READ, buf, len))
-		return -EFAULT;
-

  return sbefifo_write_common(client, buf, NULL, len); }

drivers/fsi/fsi-sbefifo.c +2 -3

@@ -742,18 +771,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);

drivers/fsi/fsi-sbefifo.c +2 -3

-	list_for_each_entry(sbefifo, &sbefifo_fifos, link) {
-		if (sbefifo->fsi_dev != fsi_dev)
-			continue;
+	if (!sbefifo)
+		return NULL;

drivers/fsi/fsi-sbefifo.c +3 -4

-		client = sbefifo_new_client(sbefifo);
-		if (client)
-			client->f_flags = flags;
-	}
+	client = sbefifo_new_client(sbefifo);
+	if (client)
+		client->f_flags = flags;

  return client;
 }

drivers/fsi/fsi-sbefifo.c +0 -0

@@ -802,95 +828,92 @@ static int sbefifo_probe(struct device *dev)
  	u32 sts;
  	int ret, child_idx = 0;

drivers/fsi/fsi-sbefifo.c +0 -1

-	dev_dbg(dev, "Found sbefifo device\n");

  sbefifo = kzalloc(sizeof(*sbefifo), GFP_KERNEL); if (!sbefifo)
  return -ENOMEM;

  sbefifo->fsi_dev = fsi_dev;

drivers/fsi/fsi-sbefifo.c +1 -2

-	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)) {

drivers/fsi/fsi-sbefifo.c +1 -2

-		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)) {

drivers/fsi/fsi-sbefifo.c +1 -2

-		dev_err(&sbefifo->fsi_dev->dev,
-				"Found data in downstream fifo\n");
+		dev_err(dev, "found data in downstream fifo\n");

  return -EIO;
  }

drivers/fsi/fsi-sbefifo.c +0 -4

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

drivers/fsi/fsi-sbefifo.c +2 -0

+	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",

drivers/fsi/fsi-sbefifo.c +1 -3

-			sbefifo->idx);
-	init_waitqueue_head(&sbefifo->wait);
-	INIT_LIST_HEAD(&sbefifo->xfrs);
+		 sbefifo->idx);

  /* This bit of silicon doesn't offer any interrupts... */ setup_timer(&sbefifo->poll_timer, sbefifo_poll_timer,

drivers/fsi/fsi-sbefifo.c +22 -12

-			(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(&sbefifo->fsi_dev->dev,
-					 "failed to create child node dev\n");
-		}
+		    (unsigned long)sbefifo);
+
+	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;
+	}
+
+	/* 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);

  }

drivers/fsi/fsi-sbefifo.c +3 -3

-	list_add(&sbefifo->link, &sbefifo_fifos);
-	
-	return misc_register(&sbefifo->mdev);
+	dev_set_drvdata(dev, sbefifo);
+
+	return 0;

 }

 static int sbefifo_remove(struct device *dev)
 {

drivers/fsi/fsi-sbefifo.c +1 -2

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

drivers/fsi/fsi-sbefifo.c +2 -5

-	list_for_each_entry_safe(sbefifo, sbefifo_tmp, &sbefifo_fifos, link) {
-		if (sbefifo->fsi_dev != fsi_dev)
-			continue;
-
-		device_for_each_child(dev, NULL, sbefifo_unregister_child);
+	WRITE_ONCE(sbefifo->rc, -ENODEV);
+	wake_up(&sbefifo->wait);

drivers/fsi/fsi-sbefifo.c +2 -3

-		misc_deregister(&sbefifo->mdev);
-		list_del(&sbefifo->link);
-		ida_simple_remove(&sbefifo_ida, sbefifo->idx);
+	misc_deregister(&sbefifo->mdev);
+	device_for_each_child(dev, NULL, sbefifo_unregister_child);

drivers/fsi/fsi-sbefifo.c +1 -2

-		if (del_timer_sync(&sbefifo->poll_timer))
-			sbefifo_put(sbefifo);
+	ida_simple_remove(&sbefifo_ida, sbefifo->idx);

drivers/fsi/fsi-sbefifo.c +2 -4

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

drivers/fsi/fsi-sbefifo.c +1 -3

-		wake_up(&sbefifo->wait);
-		sbefifo_put(sbefifo);
-	}
+	sbefifo_put(sbefifo);

  return 0;
 }

drivers/fsi/fsi-sbefifo.c +0 -0

@@ -915,17 +938,19 @@ static int sbefifo_remove(struct device *dev)

 static int sbefifo_init(void)
 {

drivers/fsi/fsi-sbefifo.c +0 -1

-	INIT_LIST_HEAD(&sbefifo_fifos);

  return fsi_driver_register(&sbefifo_drv);
 }

 static void sbefifo_exit(void)
 {
  fsi_driver_unregister(&sbefifo_drv);

drivers/fsi/fsi-sbefifo.c +2 -0

+
+	ida_destroy(&sbefifo_ida);

 }

 module_init(sbefifo_init); module_exit(sbefifo_exit);
 MODULE_LICENSE("GPL");
 MODULE_AUTHOR("Brad Bishop <[email protected]>");

drivers/fsi/fsi-sbefifo.c +2 -1

-MODULE_DESCRIPTION("Linux device interface to the POWER self boot engine");
+MODULE_AUTHOR("Eddie James <[email protected]>");
+MODULE_DESCRIPTION("Linux device interface to the POWER Self Boot Engine");

include/linux/fsi-sbefifo.h +0 -0

index 1b46c63..8e55891 100644
@@ -13,8 +13,8 @@
   * GNU General Public License for more details.
   */

include/linux/fsi-sbefifo.h +2 -2

-#ifndef __FSI_SBEFIFO_H__
-#define __FSI_SBEFIFO_H__
+#ifndef LINUX_FSI_SBEFIFO_H
+#define LINUX_FSI_SBEFIFO_H

 struct device;
 struct sbefifo_client;

include/linux/fsi-sbefifo.h +0 -0

@@ -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);

Quoted 6 lines

 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;

Quoted 8 lines

-     list_for_each_entry_safe(sbefifo, sbefifo_tmp, &sbefifo_fifos, link) { -             if (sbefifo->fsi_dev != fsi_dev)
-                     continue;
-
-             device_for_each_child(dev, NULL, sbefifo_unregister_child); +     WRITE_ONCE(sbefifo->rc, -ENODEV);
+     wake_up(&sbefifo->wait);

Quoted 11 lines

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

Quoted 8 lines

-             device_for_each_child(dev, NULL, sbefifo_unregister_child); +     WRITE_ONCE(sbefifo->rc, -ENODEV);
+     wake_up(&sbefifo->wait);

-             misc_deregister(&sbefifo->mdev);
-             list_del(&sbefifo->link);
-             ida_simple_remove(&sbefifo_ida, sbefifo->idx); +     misc_deregister(&sbefifo->mdev);

Quoted 4 lines

+     device_for_each_child(dev, NULL, sbefifo_unregister_child);
-             if (del_timer_sync(&sbefifo->poll_timer)) -                     sbefifo_put(sbefifo);

Quoted 8 lines

+     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);

Hi, it's me again.

Quoted 15 lines · Andrew Jeffery

Quoted 15 lines

  static int sbefifo_remove(struct device *dev)
  {

drivers/fsi/fsi-sbefifo.c +3 -7

-     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;
-
-             device_for_each_child(dev, NULL, sbefifo_unregister_child);
+     WRITE_ONCE(sbefifo->rc, -ENODEV);
+     wake_up(&sbefifo->wait);

Quoted 4 lines

drivers/fsi/fsi-sbefifo.c static int sbefifo_probe(struct device *dev) · line 903

Quoted 4 lines

drivers/fsi/fsi-sbefifo.c +1 -3

-             misc_deregister(&sbefifo->mdev);
-             list_del(&sbefifo->link);
-             ida_simple_remove(&sbefifo_ida, sbefifo->idx);
+     misc_deregister(&sbefifo->mdev);

I think we should do this before the wake_up() to ensure that we wake all the submitted jobs. Otherwise we might miss some.

Quoted 7 lines

Quoted 7 lines

drivers/fsi/fsi-sbefifo.c +2 -2

+     device_for_each_child(dev, NULL, sbefifo_unregister_child);
 
-             if (del_timer_sync(&sbefifo->poll_timer))
-                     sbefifo_put(sbefifo);
+     ida_simple_remove(&sbefifo_ida, sbefifo->idx);

Quoted 6 lines

drivers/fsi/fsi-sbefifo.c static int sbefifo_probe(struct device *dev) · line 909

Quoted 6 lines

drivers/fsi/fsi-sbefifo.c +2 -4

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

Shouldn't we be doing this before WRITE_ONCE(sbefifo->rc, -ENODEV) to ensure we avoid a race on the value of ->rc? Otherwise the timer could fire between the WRITE_ONCE() and here and potentially write ->rc or cause another job to hang in the FIFO.

From: "Edward A. James" <[email protected]>

Includes various fixes:
- Fix probe failure scenario
- General cleanup
- Reorder remove() operations for safety
- Add cancel boolean to prevent further xfrs when we're removing

Signed-off-by Edward A. James <[email protected]>

---
  drivers/fsi/occ.c   | 227 ++++++++++++++++++++++++++++++++--------------------
  include/linux/occ.h |  20 ++++-
  2 files changed, 155 insertions(+), 92 deletions(-)

drivers/fsi/occ.c +138 -89

index 621fbf0..4dd5048 100644
@@ -10,16 +10,22 @@
  #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/sched.h>
  #include <linux/slab.h>
+#include <linux/spinlock.h>
  #include <linux/uaccess.h>
  #include <linux/wait.h>
  #include <linux/workqueue.h>

@@ -28,49 +34,45 @@
  #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];
  	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;
+	bool cancel;
  };
 
  #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 +94,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 +106,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;

@@ -116,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);

@@ -131,14 +136,15 @@ 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;
+	struct occ_client *client = kzalloc(sizeof(*client), GFP_KERNEL);
 
-	client = kzalloc(sizeof(*client), GFP_KERNEL);
-	if (!client)
+	if (!client || occ->cancel)
  		return NULL;
 
  	client->occ = occ;

@@ -172,6 +178,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;

@@ -183,8 +190,9 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
  		if (client->read_offset) {
  			rc = 0;
  			client->read_offset = 0;
-		} else
+		} else {
  			rc = -ENOMSG;
+		}
 
  		goto done;
  	}

@@ -200,8 +208,11 @@ 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) ||
+					      occ->cancel);
 
  		spin_lock_irq(&client->lock);
 

@@ -225,12 +236,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 +263,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 +279,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)) {
+
+	if (test_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;
  	}

@@ -306,9 +316,11 @@ static ssize_t occ_write_common(struct occ_client *client,
 
  	xfr->cmd_data_length = data_length + 6;
  	client->read_offset = 0;
+	rc = occ_enqueue_xfr(xfr);
+	if (rc)
+		goto done;
 
-	occ_enqueue_xfr(xfr);
-
+	set_bit(CLIENT_XFR_PENDING, &client->flags);
  	rc = len;
 
  done:

@@ -321,12 +333,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 +372,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 +409,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 : -ENODATA;
  }
 
  static int occ_read_sbefifo(struct sbefifo_client *client, char *buf,

@@ -422,7 +428,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 +436,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 +456,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 +476,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 +490,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 +499,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 +527,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 +539,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 +567,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);

@@ -564,12 +579,19 @@ 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;
  	struct occ *occ = container_of(work, struct occ, 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);

@@ -578,11 +600,14 @@ 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 */
+	start = jiffies;
  	rc = occ_putsram(sbefifo, 0xFFFBE000, xfr->buf,
  			 xfr->cmd_data_length);
  	if (rc)

@@ -592,13 +617,26 @@ static void occ_worker(struct work_struct *work)
  	if (rc)
  		goto done;
 
-	rc = occ_getsram(sbefifo, 0xFFFBF000, xfr->buf, 8);
-	if (rc)
-		goto done;
+	/* read occ response */
+	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;
 
-	resp_data_length = (xfr->buf[3] << 8) + xfr->buf[4];
+			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) {
-		rc = -EDOM;
+		rc = -EMSGSIZE;
  		goto done;
  	}
 

@@ -704,9 +742,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) {

@@ -716,23 +751,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;

@@ -742,20 +767,42 @@ 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;
+	list_for_each_entry(xfr, &occ->xfrs, link) {
+		client = to_client(xfr);
+		wake_up_interruptible(&client->wait);
+	}
 
-	flush_work(&occ->work);
  	misc_deregister(&occ->mdev);
  	device_for_each_child(&pdev->dev, NULL, occ_unregister_child);
+
+	flush_work(&occ->work);
+
  	ida_simple_remove(&occ_ida, occ->idx);
 
  	return 0;

@@ -789,6 +836,8 @@ static void occ_exit(void)
  	destroy_workqueue(occ_wq);
 
  	platform_driver_unregister(&occ_driver);
+
+	ida_destroy(&occ_ida);
  }
 
  module_init(occ_init);

include/linux/occ.h +17 -3

index d78332c..0a4a54a 100644
@@ -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 */