drivers: fsi: client fixes and refactor

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

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 <[email protected]>

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

 2 files changed, 119 insertions(+), 94 deletions(-)

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

index 1c37ff7..a4cd353 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,6 +51,8 @@
  #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;
  struct fsi_device *fsi_dev;

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

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

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

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

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 +147,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 +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;
 }

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

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

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

@@ -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 +1 -0

@@ -349,6 +352,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 +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,

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

-			xfrs);
+				       xfrs);

  if (!xfr)
  goto out_unlock;

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

@@ -388,8 +392,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 +400,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 +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)) {

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 +440,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 +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;

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

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

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

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

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

@@ -551,30 +563,37 @@ 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))) {
+		ret = -ERESTARTSYS;
+		goto out;

  }
+
  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 +8 -1

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

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

@@ -589,22 +608,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 +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;

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

-	xfr = sbefifo_enq_xfr(client);
+	xfr = sbefifo_enq_xfr(client);		/* this xfr queued up */

  if (!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 +8 -9

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

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

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

@@ -802,29 +826,27 @@ 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 -0

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

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

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

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

-			(unsigned long)sbefifo);
+		    (unsigned long)sbefifo);

  if (dev->of_node) {
  /* create platform devs for dts child nodes (occ, etc) */

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

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

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

-#endif /* __FSI_SBEFIFO_H__ */
+#endif /* LINUX_FSI_SBEFIFO_H */

Quoted 6 lines · Eddie James

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

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

Quoted 556 lines

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

---
  drivers/fsi/fsi-sbefifo.c   | 207 +++++++++++++++++++++++++-------------------
  include/linux/fsi-sbefifo.h |   6 +-
  2 files changed, 119 insertions(+), 94 deletions(-)

drivers/fsi/fsi-sbefifo.c +116 -91

index 1c37ff7..a4cd353 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,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 <[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]>

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 <[email protected]>

---
 drivers/fsi/fsi-sbefifo.c | 109 ++++++++++++++++++++++------------------------

 1 file changed, 52 insertions(+), 57 deletions(-)

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

index a4cd353..fa34bd8 100644
@@ -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;
  };

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

-static struct list_head sbefifo_fifos;
-

 static DEFINE_IDA(sbefifo_ida);

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

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

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

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

+	if (READ_ONCE(sbefifo->rc))
+		return ERR_PTR(sbefifo->rc);
+

  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

@@ -490,7 +490,7 @@ static void sbefifo_poll_timer(unsigned long data)
  	}

  sbefifo_put(sbefifo);

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

-	wake_up(&sbefifo->wait);
+	wake_up_interruptible(&sbefifo->wait);

 out_unlock:
  spin_unlock(&sbefifo->lock);

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

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

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

@@ -664,7 +664,7 @@ static ssize_t sbefifo_write_common(struct sbefifo_client *client,
  	}

  xfr = sbefifo_enq_xfr(client); /* this xfr queued up */

  spin_unlock_irq(&sbefifo->lock);
  ret = PTR_ERR(xfr);
  goto out;

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

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

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

@@ -850,69 +847,68 @@ static int sbefifo_probe(struct device *dev)
  		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", sbefifo->idx);

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

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

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

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

  }

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

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

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

-		device_for_each_child(dev, NULL, sbefifo_unregister_child);
+	misc_deregister(&sbefifo->mdev);
+	device_for_each_child(dev, NULL, sbefifo_unregister_child);

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

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

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 +4 -1

-		WRITE_ONCE(sbefifo->rc, -ENODEV);
+	spin_lock(&sbefifo->lock);
+	list_for_each_entry(xfr, &sbefifo->xfrs, xfrs)
+		kfree(xfr);
+	spin_unlock(&sbefifo->lock);

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

@@ -937,7 +933,6 @@ 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);
 }

Quoted 5 lines · Eddie James

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

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?

Quoted 60 lines

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

---
  drivers/fsi/fsi-sbefifo.c | 109 ++++++++++++++++++++++------------------------
  1 file changed, 52 insertions(+), 57 deletions(-)

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

index a4cd353..fa34bd8 100644
@@ -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);
  		}
  	}
 

drivers/fsi/fsi-sbefifo.c static ssize_t sbefifo_write_common(struct sbefifo_client *client, · line 667

Quoted 6 lines

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

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

Quoted 100 lines

drivers/fsi/fsi-sbefifo.c +33 -31

  		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;
 

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

Quoted 5 lines

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

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

Quoted 42 lines

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

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

Quoted 9 lines · Andrew Jeffery

Quoted 5 lines · Eddie James

From: "Edward A. James" <[email protected]>
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.

Quoted 53 lines

Quoted 53 lines

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

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

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

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

Quoted 8 lines

drivers/fsi/fsi-sbefifo.c static ssize_t sbefifo_write_common(struct sbefifo_client *client, · line 667

Quoted 7 lines

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

      }

@@ -664,7 +664,7 @@ static ssize_t sbefifo_write_common(struct
sbefifo_client *client,
}
xfr = sbefifo_enq_xfr(client); /* this xfr queued up */

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

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

Quoted 93 lines

Quoted 93 lines

spin_unlock_irq(&sbefifo->lock);
          ret = PTR_ERR(xfr);
          goto out;

drivers/fsi/fsi-sbefifo.c +33 -26

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

Quoted 8 lines

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

Quoted 6 lines

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

  -    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

Quoted 34 lines

Quoted 34 lines

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

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

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

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 <[email protected]>

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

 1 file changed, 6 insertions(+), 1 deletion(-)

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

index fa34bd8..a9fc8e9 100644
@@ -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);

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

-	return *ret || *n;
+	return *ret || *n ||
+		(xfr && test_bit(SBEFIFO_XFR_COMPLETE, &xfr->flags));

 }

 static ssize_t sbefifo_read_common(struct sbefifo_client *client,

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

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 <[email protected]>

---
 drivers/fsi/occ.c   | 145 ++++++++++++++++++++++++++++------------------------ include/linux/occ.h |  20 ++++++--

 2 files changed, 96 insertions(+), 69 deletions(-)

drivers/fsi/occ.c +8 -3

index 621fbf0..3313e35 100644
@@ -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)

drivers/fsi/occ.c +0 -8

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

drivers/fsi/occ.c +1 -1

-	u16 data_length;
+	__be16 data_length;

  u8 data[OCC_RESP_DATA_BYTES];

drivers/fsi/occ.c +1 -1

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

drivers/fsi/occ.c +3 -3

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

drivers/fsi/occ.c +1 -1

-	struct occ_client *client;
+	struct occ_client *client = kzalloc(sizeof(*client), GFP_KERNEL);

drivers/fsi/occ.c +0 -1

-	client = kzalloc(sizeof(*client), GFP_KERNEL);

  if (!client)
  return NULL;

drivers/fsi/occ.c +2 -1

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

drivers/fsi/occ.c +0 -0

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

drivers/fsi/occ.c +4 -2

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

drivers/fsi/occ.c +0 -0

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

drivers/fsi/occ.c +2 -1

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

drivers/fsi/occ.c +1 -1

-	} else
+	} else {

  memcpy(kbuf, &xfr->buf[client->read_offset], bytes);

drivers/fsi/occ.c +1 -0

+	}

  client->read_offset += bytes;

drivers/fsi/occ.c +0 -0

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

drivers/fsi/occ.c +0 -6

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

drivers/fsi/occ.c +0 -0

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

drivers/fsi/occ.c +2 -4

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

drivers/fsi/occ.c +5 -0

+	/* 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)) {

drivers/fsi/occ.c +0 -1

-			kfree(xfr);

  rc = -EFAULT;
  goto done;
  }

drivers/fsi/occ.c +1 -1

-	} else
+	} else {

  memcpy(&xfr->buf[1], kbuf, len);

drivers/fsi/occ.c +1 -0

+	}

  data_length = (xfr->buf[2] << 8) + xfr->buf[3]; if (data_length > OCC_CMD_DATA_BYTES) {

drivers/fsi/occ.c +0 -1

-		kfree(xfr);

  rc = -EINVAL;
  goto done;
  }

drivers/fsi/occ.c +0 -0

@@ -321,12 +319,6 @@ static ssize_t occ_write(struct file *file, const char __user *buf,
  {
  	struct occ_client *client = file->private_data;

drivers/fsi/occ.c +0 -6

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

drivers/fsi/occ.c +0 -0

@@ -366,7 +358,7 @@ static int occ_release_common(struct occ_client *client)
  		return 0;
  	}

drivers/fsi/occ.c +1 -1

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

drivers/fsi/occ.c +0 -0

@@ -403,7 +395,7 @@ static int occ_write_sbefifo(struct sbefifo_client *client, const char *buf,
  		total += rc;
  	} while (total < len);

drivers/fsi/occ.c +1 -1

-	return (total == len) ? 0 : -EMSGSIZE;
+	return (total == len) ? 0 : -ENOSPC;

 }

 static int occ_read_sbefifo(struct sbefifo_client *client, char *buf,

drivers/fsi/occ.c +0 -0

@@ -422,7 +414,7 @@ static int occ_read_sbefifo(struct sbefifo_client *client, char *buf,
  		total += rc;
  	} while (total < len);

drivers/fsi/occ.c +1 -1

-	return (total == len) ? 0 : -EMSGSIZE;
+	return (total == len) ? 0 : -ENODATA;

 }

 static int occ_getsram(struct device *sbefifo, u32 address, u8 *data,

drivers/fsi/occ.c +2 -2

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

drivers/fsi/occ.c +3 -0

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

drivers/fsi/occ.c +1 -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

drivers/fsi/occ.c +1 -1

-		rc = -EFAULT;
+		rc = -EBADMSG;

 free:
  kfree(resp);

drivers/fsi/occ.c +2 -2

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

drivers/fsi/occ.c +0 -0

@@ -490,6 +485,9 @@ static int occ_putsram(struct device *sbefifo, u32 address, u8 *data,
  	if (!buf)
  		return -ENOMEM;

drivers/fsi/occ.c +3 -0

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

drivers/fsi/occ.c +0 -0

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

drivers/fsi/occ.c +1 -1

-		rc = -EFAULT;
+		rc = -EBADMSG;

 done:
  sbefifo_drv_release(client);

drivers/fsi/occ.c +1 -1

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

drivers/fsi/occ.c +3 -0

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

drivers/fsi/occ.c +1 -1

-	buf[4] = cpu_to_be32(0x20010000);
+	buf[4] = cpu_to_be32(0x20010000);	/* trigger occ attention */

  buf[5] = 0;

  client = sbefifo_drv_open(sbefifo, 0);

drivers/fsi/occ.c +0 -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))

drivers/fsi/occ.c +1 -1

-		rc = -EFAULT;
+		rc = -EBADMSG;

 done:
  sbefifo_drv_release(client);

drivers/fsi/occ.c +1 -0

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

drivers/fsi/occ.c +1 -0

+	resp = (struct occ_response *)xfr->buf;

  set_bit(XFR_IN_PROGRESS, &xfr->flags);

  spin_unlock_irq(&occ->list_lock);
  mutex_lock(&occ->occ_lock);

drivers/fsi/occ.c +1 -0

+	/* write occ command */

  rc = occ_putsram(sbefifo, 0xFFFBE000, xfr->buf, xfr->cmd_data_length); if (rc)

drivers/fsi/occ.c +0 -0

@@ -592,13 +596,14 @@ static void occ_worker(struct work_struct *work)
  	if (rc)
  		goto done;

drivers/fsi/occ.c +1 -0

+	/* read occ response */

  rc = occ_getsram(sbefifo, 0xFFFBF000, xfr->buf, 8); if (rc)
  goto done;

drivers/fsi/occ.c +1 -1

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

drivers/fsi/occ.c +1 -1

-		rc = -EDOM;
+		rc = -EMSGSIZE;

  goto done;
  }

drivers/fsi/occ.c +0 -0

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

drivers/fsi/occ.c +3 -0

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

drivers/fsi/occ.c +3 -0

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

drivers/fsi/occ.c +3 -0

+	if (!client)
+		return;
+

  occ_release_common(client);
 }
 EXPORT_SYMBOL_GPL(occ_drv_release);

drivers/fsi/occ.c +0 -0

@@ -704,9 +718,6 @@ static int occ_probe(struct platform_device *pdev)
  	mutex_init(&occ->occ_lock);
  	INIT_WORK(&occ->work, occ_worker);

drivers/fsi/occ.c +0 -3

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

drivers/fsi/occ.c +0 -0

@@ -789,6 +800,8 @@ static void occ_exit(void)
  	destroy_workqueue(occ_wq);

  platform_driver_unregister(&occ_driver);

drivers/fsi/occ.c +2 -0

+
+	ida_destroy(&occ_ida);

 }

 module_init(occ_init);

include/linux/occ.h +0 -0

index d78332c..0a4a54a 100644
@@ -11,12 +11,26 @@
   * GNU General Public License for more details.
   */

include/linux/occ.h +2 -2

-#ifndef __OCC_H__
-#define __OCC_H__
+#ifndef LINUX_FSI_OCC_H
+#define LINUX_FSI_OCC_H

 struct device;
 struct occ_client;

include/linux/occ.h +14 -0

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

include/linux/occ.h +0 -0

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

include/linux/occ.h +1 -1

-#endif /* __OCC_H__ */
+#endif /* LINUX_FSI_OCC_H */