Skip to contentSkip to conversation

drivers: fsi: client fixes and refactor

PATCH In review Eddie James 4 participants 42 messages Oct 6, 2017 – Oct 9, 2017

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

v4 cover drivers: fsi: client fixes and refactor

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

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

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

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

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

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

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

 drivers/fsi/fsi-sbefifo.c   | 311 +++++++++++++++++++++------------------
 drivers/fsi/occ.c           | 345 ++++++++++++++++++++++++++------------------
 drivers/hwmon/occ/p9_sbe.c  |  47 +++---
 include/linux/fsi-sbefifo.h |   6 +-
 include/linux/occ.h         |  20 ++-
 5 files changed, 426 insertions(+), 303 deletions(-)
Signature · 1 line
1.8.3.1
#1 lore raw

PATCH v4 1/31 drivers: fsi: sbefifo: Fix includes

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

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

---
 drivers/fsi/fsi-sbefifo.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)
drivers/fsi/fsi-sbefifo.c +9 -2
index 1c37ff7..6fa75a8 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
Signature · 1 line
1.8.3.1
#2 lore raw

PATCH v4 2/31 drivers: fsi: sbefifo: Use a defined reschedule length

In reply to Eddie James

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

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

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

PATCH v4 3/31 drivers: fsi: sbefifo: Use __be32 for big endian values

In reply to Eddie James

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

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

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

PATCH v4 4/31 drivers: fsi: sbefifo: white space fixes

In reply to Eddie James

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

Fix some spacing issues.

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

PATCH v4 5/31 drivers: fsi: sbefifo: replace awkward wait_event expression

In reply to Eddie James

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

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

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

PATCH v4 6/31 drivers: fsi: sbefifo: remove redundant function

In reply to Eddie James

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

sbefifo_client_next_xfr function just does what list_first_entry_or_null does.

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

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

Quoted 3 lines · Eddie James

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

sbefifo_client_next_xfr function just does what

list_first_entry_or_null

Quoted 3 lines

does.

drivers/fsi/fsi-sbefifo.c static struct sbefifo_xfr

Quoted 9 lines

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

 1 file changed, 15 insertions(+), 13 deletions(-)

drivers/fsi/fsi-sbefifo.c +0 -0
index dbac0c3..173cd03 100644
@@ -279,20 +279,12 @@ static struct sbefifo_xfr

*sbefifo_enq_xfr(struct sbefifo_client *client)

drivers/fsi/fsi-sbefifo.c static struct sbefifo_xfr *sbefifo_enq_xfr(struct sbefifo_client *client)

Quoted 10 lines
drivers/fsi/fsi-sbefifo.c +0 -0
  	return xfr;
  }

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

drivers/fsi/fsi-sbefifo.c

Quoted 8 lines
drivers/fsi/fsi-sbefifo.c +0 -2
-}
-

 static bool sbefifo_xfr_rsp_pending(struct sbefifo_client *client) {

drivers/fsi/fsi-sbefifo.c +1 -1
-	struct sbefifo_xfr *xfr;
+	struct sbefifo_xfr *xfr = list_first_entry_or_null(&client-

xfrs,

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

sbefifo_xfr,

drivers/fsi/fsi-sbefifo.c static ssize_t sbefifo_read_common(struct

Quoted 7 lines
drivers/fsi/fsi-sbefifo.c +1 -0
+							   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
@@ -595,7 +587,15 @@ static ssize_t sbefifo_read_common(struct

sbefifo_client *client,

drivers/fsi/fsi-sbefifo.c static ssize_t sbefifo_read_common(struct sbefifo_client *client,

Quoted 6 lines
drivers/fsi/fsi-sbefifo.c +0 -0
  	}


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

drivers/fsi/fsi-sbefifo.c +2 -1
-		xfr = sbefifo_client_next_xfr(client);
+		xfr = list_first_entry_or_null(&client->xfrs,
+					       struct sbefifo_xfr,

client);

drivers/fsi/fsi-sbefifo.c

Quoted 2 lines
drivers/fsi/fsi-sbefifo.c +2 -0
+		if (!xfr) {
+			/* should be impossible to not have an xfr

here */

drivers/fsi/fsi-sbefifo.c static bool sbefifo_write_ready(struct sbefifo

Quoted 9 lines
drivers/fsi/fsi-sbefifo.c +5 -0
+			WARN_ONCE(1, "no xfr in queue");
+			sbefifo_put_client(client);
+			return -EPROTO;
+		}
+

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

drivers/fsi/fsi-sbefifo.c +0 -0
@@ -632,7 +632,9 @@ static bool sbefifo_write_ready(struct sbefifo

*sbefifo,

drivers/fsi/fsi-sbefifo.c static bool sbefifo_write_ready(struct sbefifo · line 633

Quoted 2 lines
drivers/fsi/fsi-sbefifo.c +0 -0
  				struct sbefifo_xfr *xfr,
  				struct sbefifo_client *client,

size_t *n)

drivers/fsi/fsi-sbefifo.c static bool sbefifo_write_ready(struct sbefifo · line 637

Quoted 5 lines
drivers/fsi/fsi-sbefifo.c +2 -1
  {
-	struct sbefifo_xfr *next = sbefifo_client_next_xfr(client);
+	struct sbefifo_xfr *next = list_first_entry_or_null(&client-
 xfrs,
+							    struct

sbefifo_xfr,

Quoted 4 lines
drivers/fsi/fsi-sbefifo.c +1 -0
+							    client);


  *n = sbefifo_buf_nbwriteable(&client->wbuf); return READ_ONCE(sbefifo->rc) || (next == xfr && *n);

#8 lore raw

PATCH v4 7/31 drivers: fsi: sbefifo: Use goto to reduce put statements

In reply to Eddie James

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

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

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

PATCH v4 8/31 drivers: fsi: sbefifo: Do an earlier get_client call

In reply to Eddie James

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

Get client before we start using it.

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

PATCH v4 9/31 drivers: fsi: sbefifo: Remove warning and user data access check

In reply to Eddie James

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

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

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

PATCH v4 10/31 drivers: fsi: sbefifo: destroy the ida list on exit

In reply to Eddie James

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

Need to clean this up.

---
 drivers/fsi/fsi-sbefifo.c | 2 ++
 1 file changed, 2 insertions(+)
drivers/fsi/fsi-sbefifo.c +2 -0
index 60b133d..0ff5209 100644
@@ -947,6 +947,8 @@ static int sbefifo_init(void)
 static void sbefifo_exit(void)
 {
 	fsi_driver_unregister(&sbefifo_drv);
+
+	ida_destroy(&sbefifo_ida);
 }
 
 module_init(sbefifo_init);
Signature · 1 line
1.8.3.1
#12 lore raw

PATCH v4 11/31 drivers: fsi: sbefifo: Fix module authors and comments

In reply to Eddie James

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

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

---
 drivers/fsi/fsi-sbefifo.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)
drivers/fsi/fsi-sbefifo.c +7 -5
index 0ff5209..a2acf76 100644
@@ -655,7 +655,7 @@ static ssize_t sbefifo_write_common(struct sbefifo_client *client,
 	n = sbefifo_buf_nbwriteable(&client->wbuf);
 
 	spin_lock_irq(&sbefifo->lock);
-	xfr = sbefifo_next_xfr(sbefifo);
+	xfr = sbefifo_next_xfr(sbefifo);	/* next xfr to be executed */
 
 	if ((client->f_flags & O_NONBLOCK) && xfr && n < len) {
 		spin_unlock_irq(&sbefifo->lock);
@@ -663,7 +663,7 @@ static ssize_t sbefifo_write_common(struct sbefifo_client *client,
 		goto out;
 	}
 
-	xfr = sbefifo_enq_xfr(client);
+	xfr = sbefifo_enq_xfr(client);		/* this xfr queued up */
 	if (!xfr) {
 		spin_unlock_irq(&sbefifo->lock);
 		ret = -ENOMEM;
@@ -719,8 +719,9 @@ static ssize_t sbefifo_write_common(struct sbefifo_client *client,
 		len -= n;
 		ret += n;
 
-		/* set flag before starting the worker, as it may run through
-		 * and check the flag before we exit this loop!
+		/*
+		 * Set this before starting timer to avoid race condition on
+		 * this flag with the timer function writer.
 		 */
 		if (!len)
 			set_bit(SBEFIFO_XFR_WRITE_DONE, &xfr->flags);
@@ -955,4 +956,5 @@ static void sbefifo_exit(void)
 module_exit(sbefifo_exit);
 MODULE_LICENSE("GPL");
 MODULE_AUTHOR("Brad Bishop <[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");
Signature · 1 line
1.8.3.1
#13 lore raw

PATCH v4 12/31 drivers: fsi: sbefifo: Fix include guards in header file

In reply to Eddie James

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

Make them conform to Linux standard headers.

---
 include/linux/fsi-sbefifo.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
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 */
Signature · 1 line
1.8.3.1
#14 lore raw

PATCH v4 13/31 drivers: fsi: SBEFIFO: Fix probe() and remove()

In reply to 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.

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

PATCH v4 14/31 drivers: fsi: SBEFIFO: check for xfr complete in read wait_event

In reply to Eddie James

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.

---
 drivers/fsi/fsi-sbefifo.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)
drivers/fsi/fsi-sbefifo.c +6 -1
index 9b560ec..5d73437 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);
 
-	return *ret || *n;
+	return *ret || *n ||
+		(xfr && test_bit(SBEFIFO_XFR_COMPLETE, &xfr->flags));
 }
 
 static ssize_t sbefifo_read_common(struct sbefifo_client *client,
Signature · 1 line
1.8.3.1
#16 lore raw

PATCH v4 15/31 drivers: fsi: occ: Fix includes

In reply to Eddie James

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

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

---
 drivers/fsi/occ.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)
drivers/fsi/occ.c +6 -1
index 621fbf0..2d7df8a 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>
Signature · 1 line
1.8.3.1
#17 lore raw

PATCH v4 16/31 drivers: fsi: occ: Fix errant kfree calls

In reply to Eddie James

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

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

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

PATCH v4 17/31 drivers: fsi: occ: remove unused occ_command structure

In reply to Eddie James

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

---
 drivers/fsi/occ.c | 8 --------
 1 file changed, 8 deletions(-)
drivers/fsi/occ.c +0 -8
index cd60c5a..82a60c5 100644
@@ -46,14 +46,6 @@ struct occ {
 
 #define to_occ(x)	container_of((x), struct occ, mdev)
 
-struct occ_command {
-	u8 seq_no;
-	u8 cmd_type;
-	u16 data_length;
-	u8 data[OCC_CMD_DATA_BYTES];
-	u16 checksum;
-} __packed;
-
 struct occ_response {
 	u8 seq_no;
 	u8 cmd_type;
Signature · 1 line
1.8.3.1
#19 lore raw

PATCH v4 18/31 drivers: fsi: occ: Use big-endian values

In reply to Eddie James

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

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

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

PATCH v4 19/31 drivers: fsi: occ: Return ENODEV if client is NULL

In reply to Eddie James

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

Safeguard the access functions.

---
 drivers/fsi/occ.c | 24 ++++++++++++++++++++----
 1 file changed, 20 insertions(+), 4 deletions(-)
drivers/fsi/occ.c +20 -4
index 55f293d..322af5a 100644
@@ -168,11 +168,16 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
 {
 	int rc;
 	size_t bytes;
-	struct occ_xfr *xfr = &client->xfr;
+	struct occ_xfr *xfr;
+
+	if (!client)
+		return -ENODEV;
 
 	if (len > OCC_SRAM_BYTES)
 		return -EINVAL;
 
+	xfr = &client->xfr;
+
 	spin_lock_irq(&client->lock);
 
 	if (!test_bit(CLIENT_XFR_PENDING, &client->flags)) {
@@ -263,11 +268,16 @@ static ssize_t occ_write_common(struct occ_client *client,
 	int rc;
 	unsigned int i;
 	u16 data_length, checksum = 0;
-	struct occ_xfr *xfr = &client->xfr;
+	struct occ_xfr *xfr;
+
+	if (!client)
+		return -ENODEV;
 
 	if (len > (OCC_CMD_DATA_BYTES + 3) || len < 3)
 		return -EINVAL;
 
+	xfr = &client->xfr;
+
 	spin_lock_irq(&client->lock);
 	if (test_and_set_bit(CLIENT_XFR_PENDING, &client->flags)) {
 		rc = -EBUSY;
@@ -327,8 +337,14 @@ static ssize_t occ_write(struct file *file, const char __user *buf,
 
 static int occ_release_common(struct occ_client *client)
 {
-	struct occ_xfr *xfr = &client->xfr;
-	struct occ *occ = client->occ;
+	struct occ *occ;
+	struct occ_xfr *xfr;
+
+	if (!client)
+		return -ENODEV;
+
+	xfr = &client->xfr;
+	occ = client->occ;
 
 	spin_lock_irq(&client->lock);
 
Signature · 1 line
1.8.3.1
Reviewed-by on v4 19/31
#21 lore raw

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

Quoted 5 lines · Eddie James

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

Safeguard the access functions.

Quoted 61 lines

---
 drivers/fsi/occ.c | 24 ++++++++++++++++++++----

 1 file changed, 20 insertions(+), 4 deletions(-)

drivers/fsi/occ.c +4 -1
index 55f293d..322af5a 100644
@@ -168,11 +168,16 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
  {
  	int rc;
  	size_t bytes;
-	struct occ_xfr *xfr = &client->xfr;
+	struct occ_xfr *xfr;
+
+	if (!client)
+		return -ENODEV;


  if (len > OCC_SRAM_BYTES)
  return -EINVAL;

drivers/fsi/occ.c +2 -0
+	xfr = &client->xfr;
+

  spin_lock_irq(&client->lock);

  if (!test_bit(CLIENT_XFR_PENDING, &client->flags)) {

drivers/fsi/occ.c +4 -1
@@ -263,11 +268,16 @@ static ssize_t occ_write_common(struct occ_client *client,
  	int rc;
  	unsigned int i;
  	u16 data_length, checksum = 0;
-	struct occ_xfr *xfr = &client->xfr;
+	struct occ_xfr *xfr;
+
+	if (!client)
+		return -ENODEV;


  if (len > (OCC_CMD_DATA_BYTES + 3) || len < 3) return -EINVAL;

drivers/fsi/occ.c +2 -0
+	xfr = &client->xfr;
+

  spin_lock_irq(&client->lock);
  if (test_and_set_bit(CLIENT_XFR_PENDING, &client->flags)) { rc = -EBUSY;

drivers/fsi/occ.c +0 -0
@@ -327,8 +337,14 @@ static ssize_t occ_write(struct file *file, const char __user *buf,


 static int occ_release_common(struct occ_client *client) {

drivers/fsi/occ.c +8 -2
-	struct occ_xfr *xfr = &client->xfr;
-	struct occ *occ = client->occ;
+	struct occ *occ;
+	struct occ_xfr *xfr;
+
+	if (!client)
+		return -ENODEV;
+
+	xfr = &client->xfr;
+	occ = client->occ;


  spin_lock_irq(&client->lock);

#22 lore raw

PATCH v4 20/31 drivers: fsi: occ: Remove early user buffer checking

In reply to Eddie James

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

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

---
 drivers/fsi/occ.c | 12 ------------
 1 file changed, 12 deletions(-)
drivers/fsi/occ.c +0 -12
index 322af5a..7e2b770 100644
@@ -252,12 +252,6 @@ static ssize_t occ_read(struct file *file, char __user *buf, size_t len,
 {
 	struct occ_client *client = file->private_data;
 
-	/* check this ahead of time so we don't go changing the xfr state
-	 * needlessly
-	 */
-	if (!access_ok(VERIFY_WRITE, buf, len))
-		return -EFAULT;
-
 	return occ_read_common(client, buf, NULL, len);
 }
 
@@ -326,12 +320,6 @@ static ssize_t occ_write(struct file *file, const char __user *buf,
 {
 	struct occ_client *client = file->private_data;
 
-	/* check this ahead of time so we don't go changing the xfr state
-	 * needlessly
-	 */
-	if (!access_ok(VERIFY_READ, buf, len))
-		return -EFAULT;
-
 	return occ_write_common(client, buf, NULL, len);
 }
 
Signature · 1 line
1.8.3.1
#23 lore raw

PATCH v4 21/31 drivers: fsi: occ: Switch to more logical errnos

In reply to Eddie James

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

---
 drivers/fsi/occ.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)
drivers/fsi/occ.c +6 -6
index 7e2b770..c2fc14c 100644
@@ -402,7 +402,7 @@ static int occ_write_sbefifo(struct sbefifo_client *client, const char *buf,
 		total += rc;
 	} while (total < len);
 
-	return (total == len) ? 0 : -EMSGSIZE;
+	return (total == len) ? 0 : -ENOSPC;
 }
 
 static int occ_read_sbefifo(struct sbefifo_client *client, char *buf,
@@ -421,7 +421,7 @@ static int occ_read_sbefifo(struct sbefifo_client *client, char *buf,
 		total += rc;
 	} while (total < len);
 
-	return (total == len) ? 0 : -EMSGSIZE;
+	return (total == len) ? 0 : -ENODATA;
 }
 
 static int occ_getsram(struct device *sbefifo, u32 address, u8 *data,
@@ -466,7 +466,7 @@ static int occ_getsram(struct device *sbefifo, u32 address, u8 *data,
 	    (be32_to_cpu(buf[1]) == 0xC0DEA403))
 		memcpy(data, resp, len);
 	else
-		rc = -EFAULT;
+		rc = -EBADMSG;
 
 free:
 	kfree(resp);
@@ -514,7 +514,7 @@ static int occ_putsram(struct device *sbefifo, u32 address, u8 *data,
 	/* check for good response */
 	if ((be32_to_cpu(buf[0]) != data_len) ||
 	    (be32_to_cpu(buf[1]) != 0xC0DEA404))
-		rc = -EFAULT;
+		rc = -EBADMSG;
 
 done:
 	sbefifo_drv_release(client);
@@ -551,7 +551,7 @@ static int occ_trigger_attn(struct device *sbefifo)
 	/* check for good response */
 	if ((be32_to_cpu(buf[0]) != 0xC0DEA202) ||
 	    (be32_to_cpu(buf[1]) & 0x0FFFFFFF))
-		rc = -EFAULT;
+		rc = -EBADMSG;
 
 done:
 	sbefifo_drv_release(client);
@@ -599,7 +599,7 @@ static void occ_worker(struct work_struct *work)
 
 	resp_data_length = get_unaligned_be16(&resp->data_length);
 	if (resp_data_length > OCC_RESP_DATA_BYTES) {
-		rc = -EDOM;
+		rc = -EMSGSIZE;
 		goto done;
 	}
 
Signature · 1 line
1.8.3.1
#24 lore raw

PATCH v4 22/31 drivers: fsi: occ: fix white space and bracket problems

In reply to Eddie James

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

For better flow and to meet Linux standards.

---
 drivers/fsi/occ.c | 28 ++++++++++++++++++----------
 1 file changed, 18 insertions(+), 10 deletions(-)
drivers/fsi/occ.c +18 -10
index c2fc14c..6e20bb3 100644
@@ -132,9 +132,8 @@ static void occ_enqueue_xfr(struct occ_xfr *xfr)
 
 static struct occ_client *occ_open_common(struct occ *occ, unsigned long flags)
 {
-	struct occ_client *client;
+	struct occ_client *client = kzalloc(sizeof(*client), GFP_KERNEL);
 
-	client = kzalloc(sizeof(*client), GFP_KERNEL);
 	if (!client)
 		return NULL;
 
@@ -163,6 +162,12 @@ static int occ_open(struct inode *inode, struct file *file)
 	return 0;
 }
 
+static inline bool occ_read_ready(struct occ_xfr *xfr)
+{
+	return test_bit(XFR_COMPLETE, &xfr->flags) ||
+		test_bit(XFR_CANCELED, &xfr->flags);
+}
+
 static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
 			       char *kbuf, size_t len)
 {
@@ -185,8 +190,9 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
 		if (client->read_offset) {
 			rc = 0;
 			client->read_offset = 0;
-		} else
+		} else {
 			rc = -ENOMSG;
+		}
 
 		goto done;
 	}
@@ -202,8 +208,7 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
 		spin_unlock_irq(&client->lock);
 
 		rc = wait_event_interruptible(client->wait,
-			test_bit(XFR_COMPLETE, &xfr->flags) ||
-			test_bit(XFR_CANCELED, &xfr->flags));
+					      occ_read_ready(xfr));
 
 		spin_lock_irq(&client->lock);
 
@@ -227,12 +232,14 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
 
 	bytes = min(len, xfr->resp_data_length - client->read_offset);
 	if (ubuf) {
-		if (copy_to_user(ubuf, &xfr->buf[client->read_offset], bytes)) {
+		if (copy_to_user(ubuf, &xfr->buf[client->read_offset],
+				 bytes)) {
 			rc = -EFAULT;
 			goto done;
 		}
-	} else
+	} else {
 		memcpy(kbuf, &xfr->buf[client->read_offset], bytes);
+	}
 
 	client->read_offset += bytes;
 
@@ -273,6 +280,7 @@ static ssize_t occ_write_common(struct occ_client *client,
 	xfr = &client->xfr;
 
 	spin_lock_irq(&client->lock);
+
 	if (test_and_set_bit(CLIENT_XFR_PENDING, &client->flags)) {
 		rc = -EBUSY;
 		goto done;
@@ -280,7 +288,6 @@ static ssize_t occ_write_common(struct occ_client *client,
 
 	/* clear out the transfer */
 	memset(xfr, 0, sizeof(*xfr));
-
 	xfr->buf[0] = 1;
 
 	if (ubuf) {
@@ -288,8 +295,9 @@ static ssize_t occ_write_common(struct occ_client *client,
 			rc = -EFAULT;
 			goto done;
 		}
-	} else
+	} else {
 		memcpy(&xfr->buf[1], kbuf, len);
+	}
 
 	data_length = (xfr->buf[2] << 8) + xfr->buf[3];
 	if (data_length > OCC_CMD_DATA_BYTES) {
@@ -446,7 +454,7 @@ static int occ_getsram(struct device *sbefifo, u32 address, u8 *data,
 	rc = occ_write_sbefifo(client, (const char *)buf, sizeof(buf));
 	if (rc)
 		goto done;
-	
+
 	resp = kzalloc(data_len, GFP_KERNEL);
 	if (!resp) {
 		rc = -ENOMEM;
Signature · 1 line
1.8.3.1
Acked-by on v4 22/31
#25 lore raw

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

Quoted 5 lines · Eddie James

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

For better flow and to meet Linux standards.

Quoted 106 lines

---
 drivers/fsi/occ.c | 28 ++++++++++++++++++----------

 1 file changed, 18 insertions(+), 10 deletions(-)

drivers/fsi/occ.c +0 -0
index c2fc14c..6e20bb3 100644
@@ -132,9 +132,8 @@ static void occ_enqueue_xfr(struct occ_xfr *xfr)


 static struct occ_client *occ_open_common(struct occ *occ, unsigned long flags) {

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 +0 -0
@@ -163,6 +162,12 @@ static int occ_open(struct inode *inode, struct file *file)
  	return 0;
  }

drivers/fsi/occ.c +6 -0
+static inline bool occ_read_ready(struct occ_xfr *xfr)
+{
+	return test_bit(XFR_COMPLETE, &xfr->flags) ||
+		test_bit(XFR_CANCELED, &xfr->flags);
+}
+

 static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf, char *kbuf, size_t len)
 {

drivers/fsi/occ.c +2 -1
@@ -185,8 +190,9 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
  		if (client->read_offset) {
  			rc = 0;
  			client->read_offset = 0;
-		} else
+		} else {
  			rc = -ENOMSG;
+		}


  goto done;
  }

drivers/fsi/occ.c +0 -0
@@ -202,8 +208,7 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
  		spin_unlock_irq(&client->lock);


  rc = wait_event_interruptible(client->wait,

drivers/fsi/occ.c +1 -2
-			test_bit(XFR_COMPLETE, &xfr->flags) ||
-			test_bit(XFR_CANCELED, &xfr->flags));
+					      occ_read_ready(xfr));


  spin_lock_irq(&client->lock);

drivers/fsi/occ.c +0 -0
@@ -227,12 +232,14 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,


  bytes = min(len, xfr->resp_data_length - client->read_offset); if (ubuf) {

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
@@ -273,6 +280,7 @@ static ssize_t occ_write_common(struct occ_client *client,
  	xfr = &client->xfr;


  spin_lock_irq(&client->lock);
+
  if (test_and_set_bit(CLIENT_XFR_PENDING, &client->flags)) { rc = -EBUSY;
  goto done;

drivers/fsi/occ.c +0 -0
@@ -280,7 +288,6 @@ static ssize_t occ_write_common(struct occ_client *client,


  /* clear out the transfer */
  memset(xfr, 0, sizeof(*xfr));
-
  xfr->buf[0] = 1;

  if (ubuf) {

drivers/fsi/occ.c +2 -1
@@ -288,8 +295,9 @@ static ssize_t occ_write_common(struct occ_client *client,
  			rc = -EFAULT;
  			goto done;
  		}
-	} else
+	} else {
  		memcpy(&xfr->buf[1], kbuf, len);
+	}


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

drivers/fsi/occ.c +1 -1
@@ -446,7 +454,7 @@ static int occ_getsram(struct device *sbefifo, u32 address, u8 *data,
  	rc = occ_write_sbefifo(client, (const char *)buf, sizeof(buf));
  	if (rc)
  		goto done;
-	
+
  	resp = kzalloc(data_len, GFP_KERNEL);
  	if (!resp) {
  		rc = -ENOMEM;
#26 lore raw

PATCH v4 23/31 drivers: fsi: occ: Destroy the ida list on exit

In reply to Eddie James

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

Need to clean up the index list structure.

---
 drivers/fsi/occ.c | 2 ++
 1 file changed, 2 insertions(+)
drivers/fsi/occ.c +2 -0
index 6e20bb3..6eba40b 100644
@@ -798,6 +798,8 @@ static void occ_exit(void)
 	destroy_workqueue(occ_wq);
 
 	platform_driver_unregister(&occ_driver);
+
+	ida_destroy(&occ_ida);
 }
 
 module_init(occ_init);
Signature · 1 line
1.8.3.1
#27 lore raw

PATCH v4 24/31 drivers: fsi: occ: Remove unnecessary platform_set_drvdata call

In reply to Eddie James

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

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

---
 drivers/fsi/occ.c | 3 ---
 1 file changed, 3 deletions(-)
drivers/fsi/occ.c +0 -3
index 6eba40b..5590a72 100644
@@ -713,9 +713,6 @@ static int occ_probe(struct platform_device *pdev)
 	mutex_init(&occ->occ_lock);
 	INIT_WORK(&occ->work, occ_worker);
 
-	/* ensure NULL before we probe children, so they don't hang FSI */
-	platform_set_drvdata(pdev, NULL);
-
 	if (dev->of_node) {
 		rc = of_property_read_u32(dev->of_node, "reg", &reg);
 		if (!rc) {
Signature · 1 line
1.8.3.1
#28 lore raw

PATCH v4 25/31 drivers: fsi: occ: Add comments for clarity

In reply to Eddie James

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

Fixup some existing comments as well.

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

PATCH v4 26/31 drivers: fsi: occ: Add OCC response definitions to header

In reply to Eddie James

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

Also fix include guards.

---
 include/linux/occ.h | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)
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 */
Signature · 1 line
1.8.3.1
#30 lore raw

PATCH v4 27/31 drivers: fsi: occ: Poll while receiving "command in progress"

In reply to Eddie James

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

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

---
 drivers/fsi/occ.c | 27 ++++++++++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)
drivers/fsi/occ.c +24 -3
index 3e27307..bd0ad98 100644
@@ -23,6 +23,7 @@
 #include <linux/of.h>
 #include <linux/of_platform.h>
 #include <linux/platform_device.h>
+#include <linux/sched.h>
 #include <linux/slab.h>
 #include <linux/spinlock.h>
 #include <linux/uaccess.h>
@@ -33,6 +34,9 @@
 #define OCC_CMD_DATA_BYTES	4090
 #define OCC_RESP_DATA_BYTES	4089
 
+#define OCC_TIMEOUT_MS		1000
+#define OCC_CMD_IN_PRG_WAIT_MS	50
+
 struct occ {
 	struct device *sbefifo;
 	char name[32];
@@ -588,6 +592,9 @@ static void occ_worker(struct work_struct *work)
 {
 	int rc = 0, empty, waiting, canceled;
 	u16 resp_data_length;
+	unsigned long start;
+	const unsigned long timeout = msecs_to_jiffies(OCC_TIMEOUT_MS);
+	const long int wait_time = msecs_to_jiffies(OCC_CMD_IN_PRG_WAIT_MS);
 	struct occ_xfr *xfr;
 	struct occ_response *resp;
 	struct occ_client *client;
@@ -609,6 +616,8 @@ static void occ_worker(struct work_struct *work)
 	spin_unlock_irq(&occ->list_lock);
 	mutex_lock(&occ->occ_lock);
 
+	start = jiffies;
+
 	/* write occ command */
 	rc = occ_putsram(sbefifo, 0xFFFBE000, xfr->buf,
 			 xfr->cmd_data_length);
@@ -620,9 +629,21 @@ static void occ_worker(struct work_struct *work)
 		goto done;
 
 	/* read occ response */
-	rc = occ_getsram(sbefifo, 0xFFFBF000, xfr->buf, 8);
-	if (rc)
-		goto done;
+	do {
+		rc = occ_getsram(sbefifo, 0xFFFBF000, xfr->buf, 8);
+		if (rc)
+			goto done;
+
+		if (resp->return_status == OCC_RESP_CMD_IN_PRG) {
+			rc = -EALREADY;
+
+			if (time_after(jiffies, start + timeout))
+				break;
+
+			set_current_state(TASK_INTERRUPTIBLE);
+			schedule_timeout(wait_time);
+		}
+	} while (rc);
 
 	resp_data_length = get_unaligned_be16(&resp->data_length);
 	if (resp_data_length > OCC_RESP_DATA_BYTES) {
Signature · 1 line
1.8.3.1
#31 lore raw

PATCH v4 28/31 drivers: fsi: occ: Add cancel to remove() and fix probe()

In reply to Eddie James

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

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

---
 drivers/fsi/occ.c | 71 +++++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 50 insertions(+), 21 deletions(-)
drivers/fsi/occ.c +50 -21
index bd0ad98..ec42fc0 100644
@@ -46,6 +46,7 @@ struct occ {
 	spinlock_t list_lock;		/* lock access to the xfrs list */
 	struct mutex occ_lock;		/* lock access to the hardware */
 	struct work_struct work;
+	bool cancel;
 };
 
 #define to_occ(x)	container_of((x), struct occ, mdev)
@@ -117,12 +118,15 @@ struct occ_client {
 
 static DEFINE_IDA(occ_ida);
 
-static void occ_enqueue_xfr(struct occ_xfr *xfr)
+static int occ_enqueue_xfr(struct occ_xfr *xfr)
 {
 	int empty;
 	struct occ_client *client = to_client(xfr);
 	struct occ *occ = client->occ;
 
+	if (occ->cancel)
+		return -ECANCELED;
+
 	spin_lock_irq(&occ->list_lock);
 
 	empty = list_empty(&occ->xfrs);
@@ -132,6 +136,8 @@ static void occ_enqueue_xfr(struct occ_xfr *xfr)
 
 	if (empty)
 		queue_work(occ_wq, &occ->work);
+
+	return 0;
 }
 
 static struct occ_client *occ_open_common(struct occ *occ, unsigned long flags)
@@ -166,10 +172,10 @@ static int occ_open(struct inode *inode, struct file *file)
 	return 0;
 }
 
-static inline bool occ_read_ready(struct occ_xfr *xfr)
+static inline bool occ_read_ready(struct occ_xfr *xfr, struct occ *occ)
 {
 	return test_bit(XFR_COMPLETE, &xfr->flags) ||
-		test_bit(XFR_CANCELED, &xfr->flags);
+		test_bit(XFR_CANCELED, &xfr->flags) || occ->cancel;
 }
 
 static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
@@ -178,6 +184,7 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
 	int rc;
 	size_t bytes;
 	struct occ_xfr *xfr;
+	struct occ *occ;
 
 	if (!client)
 		return -ENODEV;
@@ -186,6 +193,7 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
 		return -EINVAL;
 
 	xfr = &client->xfr;
+	occ = client->occ;
 
 	spin_lock_irq(&client->lock);
 
@@ -212,7 +220,7 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
 		spin_unlock_irq(&client->lock);
 
 		rc = wait_event_interruptible(client->wait,
-					      occ_read_ready(xfr));
+					      occ_read_ready(xfr, occ));
 
 		spin_lock_irq(&client->lock);
 
@@ -285,7 +293,7 @@ static ssize_t occ_write_common(struct occ_client *client,
 
 	spin_lock_irq(&client->lock);
 
-	if (test_and_set_bit(CLIENT_XFR_PENDING, &client->flags)) {
+	if (test_bit(CLIENT_XFR_PENDING, &client->flags)) {
 		rc = -EBUSY;
 		goto done;
 	}
@@ -323,8 +331,11 @@ static ssize_t occ_write_common(struct occ_client *client,
 	xfr->cmd_data_length = data_length + 6;
 	client->read_offset = 0;
 
-	occ_enqueue_xfr(xfr);
+	rc = occ_enqueue_xfr(xfr);
+	if (rc)
+		goto done;
 
+	set_bit(CLIENT_XFR_PENDING, &client->flags);
 	rc = len;
 
 done:
@@ -602,6 +613,9 @@ static void occ_worker(struct work_struct *work)
 	struct device *sbefifo = occ->sbefifo;
 
 again:
+	if (occ->cancel)
+		return;
+
 	spin_lock_irq(&occ->list_lock);
 
 	xfr = list_first_entry_or_null(&occ->xfrs, struct occ_xfr, link);
@@ -762,23 +776,13 @@ static int occ_probe(struct platform_device *pdev)
 			if (occ->idx < 0)
 				occ->idx = ida_simple_get(&occ_ida, 1, INT_MAX,
 							  GFP_KERNEL);
-		} else
+		} else {
 			occ->idx = ida_simple_get(&occ_ida, 1, INT_MAX,
 						  GFP_KERNEL);
-
-		/* create platform devs for dts child nodes (hwmon, etc) */
-		for_each_child_of_node(dev->of_node, np) {
-			snprintf(child_name, sizeof(child_name), "occ%d-dev%d",
-				 occ->idx, child_idx++);
-			child = of_platform_device_create(np, child_name, dev);
-			if (!child)
-				dev_warn(dev,
-					 "failed to create child node dev\n");
 		}
-	} else
+	} else {
 		occ->idx = ida_simple_get(&occ_ida, 1, INT_MAX, GFP_KERNEL);
-
-	platform_set_drvdata(pdev, occ);
+	}
 
 	snprintf(occ->name, sizeof(occ->name), "occ%d", occ->idx);
 	occ->mdev.fops = &occ_fops;
@@ -788,20 +792,45 @@ static int occ_probe(struct platform_device *pdev)
 
 	rc = misc_register(&occ->mdev);
 	if (rc) {
-		dev_err(dev, "failed to register miscdevice\n");
+		dev_err(dev, "failed to register miscdevice: %d\n", rc);
+		ida_simple_remove(&occ_ida, occ->idx);
 		return rc;
 	}
 
+	/* create platform devs for dts child nodes (hwmon, etc) */
+	for_each_available_child_of_node(dev->of_node, np) {
+		snprintf(child_name, sizeof(child_name), "occ%d-dev%d",
+			 occ->idx, child_idx++);
+		child = of_platform_device_create(np, child_name, dev);
+		if (!child)
+			dev_warn(dev, "failed to create child node dev\n");
+	}
+
+	platform_set_drvdata(pdev, occ);
+
 	return 0;
 }
 
 static int occ_remove(struct platform_device *pdev)
 {
 	struct occ *occ = platform_get_drvdata(pdev);
+	struct occ_xfr *xfr;
+	struct occ_client *client;
+
+	occ->cancel = true;
+
+	spin_lock_irq(&occ->list_lock);
+	list_for_each_entry(xfr, &occ->xfrs, link) {
+		client = to_client(xfr);
+		wake_up_all(&client->wait);
+	}
+	spin_unlock_irq(&occ->list_lock);
 
-	flush_work(&occ->work);
 	misc_deregister(&occ->mdev);
 	device_for_each_child(&pdev->dev, NULL, occ_unregister_child);
+
+	cancel_work_sync(&occ->work);
+
 	ida_simple_remove(&occ_ida, occ->idx);
 
 	return 0;
Signature · 1 line
1.8.3.1
Reviewed-by on v4 28/31
#32 lore raw

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

Quoted 8 lines · Eddie James

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

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

Quoted 187 lines

---
 drivers/fsi/occ.c | 71 +++++++++++++++++++++++++++++++++++++++----------------

 1 file changed, 50 insertions(+), 21 deletions(-)

drivers/fsi/occ.c +0 -0
index bd0ad98..ec42fc0 100644
@@ -46,6 +46,7 @@ struct occ {

  spinlock_t list_lock; /* lock access to the xfrs list */
  struct mutex occ_lock; /* lock access to the hardware */
  struct work_struct work;

drivers/fsi/occ.c +1 -0
+	bool cancel;

 };

 #define to_occ(x) container_of((x), struct occ, mdev)

drivers/fsi/occ.c +0 -0
@@ -117,12 +118,15 @@ struct occ_client {


 static DEFINE_IDA(occ_ida);

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

drivers/fsi/occ.c +3 -0
+	if (occ->cancel)
+		return -ECANCELED;
+

  spin_lock_irq(&occ->list_lock);

  empty = list_empty(&occ->xfrs);

drivers/fsi/occ.c +0 -0
@@ -132,6 +136,8 @@ static void occ_enqueue_xfr(struct occ_xfr *xfr)


  if (empty)
  queue_work(occ_wq, &occ->work);

drivers/fsi/occ.c +2 -0
+
+	return 0;

 }

 static struct occ_client *occ_open_common(struct occ *occ, unsigned long flags)

drivers/fsi/occ.c +0 -0
@@ -166,10 +172,10 @@ static int occ_open(struct inode *inode, struct file *file)
  	return 0;
  }

drivers/fsi/occ.c +1 -1
-static inline bool occ_read_ready(struct occ_xfr *xfr)
+static inline bool occ_read_ready(struct occ_xfr *xfr, struct occ *occ)

 {
  return test_bit(XFR_COMPLETE, &xfr->flags) ||

drivers/fsi/occ.c +1 -1
-		test_bit(XFR_CANCELED, &xfr->flags);
+		test_bit(XFR_CANCELED, &xfr->flags) || occ->cancel;

 }

 static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,

drivers/fsi/occ.c +1 -0
@@ -178,6 +184,7 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
  	int rc;
  	size_t bytes;
  	struct occ_xfr *xfr;
+	struct occ *occ;


  if (!client)
  return -ENODEV;

drivers/fsi/occ.c +0 -0
@@ -186,6 +193,7 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
  		return -EINVAL;


  xfr = &client->xfr;

drivers/fsi/occ.c +1 -0
+	occ = client->occ;


  spin_lock_irq(&client->lock);

drivers/fsi/occ.c +0 -0
@@ -212,7 +220,7 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
  		spin_unlock_irq(&client->lock);


  rc = wait_event_interruptible(client->wait,

drivers/fsi/occ.c +1 -1
-					      occ_read_ready(xfr));
+					      occ_read_ready(xfr, occ));


  spin_lock_irq(&client->lock);

drivers/fsi/occ.c +0 -0
@@ -285,7 +293,7 @@ static ssize_t occ_write_common(struct occ_client *client,


  spin_lock_irq(&client->lock);

drivers/fsi/occ.c +1 -1
-	if (test_and_set_bit(CLIENT_XFR_PENDING, &client->flags)) {
+	if (test_bit(CLIENT_XFR_PENDING, &client->flags)) {

  rc = -EBUSY;
  goto done;
  }

drivers/fsi/occ.c +0 -0
@@ -323,8 +331,11 @@ static ssize_t occ_write_common(struct occ_client *client,
  	xfr->cmd_data_length = data_length + 6;
  	client->read_offset = 0;

drivers/fsi/occ.c +3 -1
-	occ_enqueue_xfr(xfr);
+	rc = occ_enqueue_xfr(xfr);
+	if (rc)
+		goto done;

drivers/fsi/occ.c +1 -0
+	set_bit(CLIENT_XFR_PENDING, &client->flags);

  rc = len;

 done:

drivers/fsi/occ.c +0 -0
@@ -602,6 +613,9 @@ static void occ_worker(struct work_struct *work)
  	struct device *sbefifo = occ->sbefifo;


 again:

drivers/fsi/occ.c +3 -0
+	if (occ->cancel)
+		return;
+

  spin_lock_irq(&occ->list_lock);

  xfr = list_first_entry_or_null(&occ->xfrs, struct occ_xfr, link);

drivers/fsi/occ.c +3 -13
@@ -762,23 +776,13 @@ static int occ_probe(struct platform_device *pdev)
  			if (occ->idx < 0)
  				occ->idx = ida_simple_get(&occ_ida, 1, INT_MAX,
  							  GFP_KERNEL);
-		} else
+		} else {
  			occ->idx = ida_simple_get(&occ_ida, 1, INT_MAX,
  						  GFP_KERNEL);
-
-		/* create platform devs for dts child nodes (hwmon, etc) */
-		for_each_child_of_node(dev->of_node, np) {
-			snprintf(child_name, sizeof(child_name), "occ%d-dev%d",
-				 occ->idx, child_idx++);
-			child = of_platform_device_create(np, child_name, dev);
-			if (!child)
-				dev_warn(dev,
-					 "failed to create child node dev\n");
  		}
-	} else
+	} else {
  		occ->idx = ida_simple_get(&occ_ida, 1, INT_MAX, GFP_KERNEL);
-
-	platform_set_drvdata(pdev, occ);
+	}


  snprintf(occ->name, sizeof(occ->name), "occ%d", occ->idx); occ->mdev.fops = &occ_fops;

drivers/fsi/occ.c +0 -0
@@ -788,20 +792,45 @@ static int occ_probe(struct platform_device *pdev)


  rc = misc_register(&occ->mdev);
  if (rc) {

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

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

drivers/fsi/occ.c +11 -0
+	struct occ_xfr *xfr;
+	struct occ_client *client;
+
+	occ->cancel = true;
+
+	spin_lock_irq(&occ->list_lock);
+	list_for_each_entry(xfr, &occ->xfrs, link) {
+		client = to_client(xfr);
+		wake_up_all(&client->wait);
+	}
+	spin_unlock_irq(&occ->list_lock);

drivers/fsi/occ.c +0 -1
-	flush_work(&occ->work);

  misc_deregister(&occ->mdev); device_for_each_child(&pdev->dev, NULL, occ_unregister_child);

drivers/fsi/occ.c +3 -0
+
+	cancel_work_sync(&occ->work);
+

  ida_simple_remove(&occ_ida, occ->idx);

  return 0;

#33 lore raw

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

In reply to Eddie James
Quoted 30 lines · Eddie James

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

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

---
drivers/fsi/occ.c | 71 +++++++++++++++++++++++++++++++++++++++----------------

1 file changed, 50 insertions(+), 21 deletions(-)
drivers/fsi/occ.c +1 -0
index bd0ad98..ec42fc0 100644
@@ -46,6 +46,7 @@ struct occ {
 	spinlock_t list_lock;		/* lock access to the xfrs list */
 	struct mutex occ_lock;		/* lock access to the hardware */
 	struct work_struct work;
+	bool cancel;
 };
 
 #define to_occ(x)	container_of((x), struct occ, mdev)
@@ -117,12 +118,15 @@ struct occ_client {
 
 static DEFINE_IDA(occ_ida);

drivers/fsi/occ.c struct occ_client { · line 128

Quoted 10 lines
drivers/fsi/occ.c +3 -1
 
-static void occ_enqueue_xfr(struct occ_xfr *xfr)
+static int occ_enqueue_xfr(struct occ_xfr *xfr)
 {
 	int empty;
 	struct occ_client *client = to_client(xfr);
 	struct occ *occ = client->occ;
 
+	if (occ->cancel)
+		return -ECANCELED;

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

Quoted 160 lines
drivers/fsi/occ.c +13 -5
+
 	spin_lock_irq(&occ->list_lock);
 
 	empty = list_empty(&occ->xfrs);
@@ -132,6 +136,8 @@ static void occ_enqueue_xfr(struct occ_xfr *xfr)
 
 	if (empty)
 		queue_work(occ_wq, &occ->work);
+
+	return 0;
 }
 
 static struct occ_client *occ_open_common(struct occ *occ, unsigned long flags)
@@ -166,10 +172,10 @@ static int occ_open(struct inode *inode, struct file *file)
 	return 0;
 }
 
-static inline bool occ_read_ready(struct occ_xfr *xfr)
+static inline bool occ_read_ready(struct occ_xfr *xfr, struct occ *occ)
 {
 	return test_bit(XFR_COMPLETE, &xfr->flags) ||
-		test_bit(XFR_CANCELED, &xfr->flags);
+		test_bit(XFR_CANCELED, &xfr->flags) || occ->cancel;
 }
 
 static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
@@ -178,6 +184,7 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
 	int rc;
 	size_t bytes;
 	struct occ_xfr *xfr;
+	struct occ *occ;
 
 	if (!client)
 		return -ENODEV;
@@ -186,6 +193,7 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
 		return -EINVAL;
 
 	xfr = &client->xfr;
+	occ = client->occ;
 
 	spin_lock_irq(&client->lock);
 
@@ -212,7 +220,7 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
 		spin_unlock_irq(&client->lock);
 
 		rc = wait_event_interruptible(client->wait,
-					      occ_read_ready(xfr));
+					      occ_read_ready(xfr, occ));
 
 		spin_lock_irq(&client->lock);
 
@@ -285,7 +293,7 @@ static ssize_t occ_write_common(struct occ_client *client,
 
 	spin_lock_irq(&client->lock);
 
-	if (test_and_set_bit(CLIENT_XFR_PENDING, &client->flags)) {
+	if (test_bit(CLIENT_XFR_PENDING, &client->flags)) {
 		rc = -EBUSY;
 		goto done;
 	}
@@ -323,8 +331,11 @@ static ssize_t occ_write_common(struct occ_client *client,
 	xfr->cmd_data_length = data_length + 6;
 	client->read_offset = 0;
 
-	occ_enqueue_xfr(xfr);
+	rc = occ_enqueue_xfr(xfr);
+	if (rc)
+		goto done;
 
+	set_bit(CLIENT_XFR_PENDING, &client->flags);
 	rc = len;
 

done:

drivers/fsi/occ.c +0 -0
@@ -602,6 +613,9 @@ static void occ_worker(struct work_struct *work)
 	struct device *sbefifo = occ->sbefifo;
 

again:

drivers/fsi/occ.c +3 -0
+	if (occ->cancel)
+		return;
+
	spin_lock_irq(&occ->list_lock);

	xfr = list_first_entry_or_null(&occ->xfrs, struct occ_xfr, link);
drivers/fsi/occ.c +30 -15
@@ -762,23 +776,13 @@ static int occ_probe(struct platform_device *pdev)
 			if (occ->idx < 0)
 				occ->idx = ida_simple_get(&occ_ida, 1, INT_MAX,
 							  GFP_KERNEL);
-		} else
+		} else {
 			occ->idx = ida_simple_get(&occ_ida, 1, INT_MAX,
 						  GFP_KERNEL);
-
-		/* create platform devs for dts child nodes (hwmon, etc) */
-		for_each_child_of_node(dev->of_node, np) {
-			snprintf(child_name, sizeof(child_name), "occ%d-dev%d",
-				 occ->idx, child_idx++);
-			child = of_platform_device_create(np, child_name, dev);
-			if (!child)
-				dev_warn(dev,
-					 "failed to create child node dev\n");
 		}
-	} else
+	} else {
 		occ->idx = ida_simple_get(&occ_ida, 1, INT_MAX, GFP_KERNEL);
-
-	platform_set_drvdata(pdev, occ);
+	}
 
 	snprintf(occ->name, sizeof(occ->name), "occ%d", occ->idx);
 	occ->mdev.fops = &occ_fops;
@@ -788,20 +792,45 @@ static int occ_probe(struct platform_device *pdev)
 
 	rc = misc_register(&occ->mdev);
 	if (rc) {
-		dev_err(dev, "failed to register miscdevice\n");
+		dev_err(dev, "failed to register miscdevice: %d\n", rc);
+		ida_simple_remove(&occ_ida, occ->idx);
 		return rc;
 	}
 
+	/* create platform devs for dts child nodes (hwmon, etc) */
+	for_each_available_child_of_node(dev->of_node, np) {
+		snprintf(child_name, sizeof(child_name), "occ%d-dev%d",
+			 occ->idx, child_idx++);
+		child = of_platform_device_create(np, child_name, dev);
+		if (!child)
+			dev_warn(dev, "failed to create child node dev\n");
+	}
+
+	platform_set_drvdata(pdev, occ);
+
 	return 0;
 }
 
 static int occ_remove(struct platform_device *pdev)
 {
 	struct occ *occ = platform_get_drvdata(pdev);
+	struct occ_xfr *xfr;
+	struct occ_client *client;
+
+	occ->cancel = true;
+
+	spin_lock_irq(&occ->list_lock);
+	list_for_each_entry(xfr, &occ->xfrs, link) {
+		client = to_client(xfr);
+		wake_up_all(&client->wait);
+	}
+	spin_unlock_irq(&occ->list_lock);
 
-	flush_work(&occ->work);
 	misc_deregister(&occ->mdev);
 	device_for_each_child(&pdev->dev, NULL, occ_unregister_child);
+
+	cancel_work_sync(&occ->work);
+
 	ida_simple_remove(&occ_ida, occ->idx);
 
 	return 0;
Signature · 1 line
1.8.3.1
#34 lore raw

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

Quoted 9 lines · Brad Bishop
Quoted 9 lines · Eddie James
drivers/fsi/occ.c +1 -1
-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;
Quoted 5 lines

drivers/fsi/occ.c struct occ_client { · line 128

Quoted 2 lines
drivers/fsi/occ.c +2 -0
+     if (occ->cancel)
+             return -ECANCELED;

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

I agree.

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

Cheers,

Joel

#35 lore raw

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

Quoted 10 lines · Joel Stanley
Quoted 8 lines · Brad Bishop
Quoted 8 lines · Eddie James
drivers/fsi/occ.c +1 -1
-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;
Quoted 7 lines
Quoted 4 lines

drivers/fsi/occ.c struct occ_client { · line 128

Quoted 2 lines
drivers/fsi/occ.c +2 -0
+     if (occ->cancel)
+             return -ECANCELED;

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

I agree.

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

Sounds good! Thanks
Eddie

Quoted 5 lines

Cheers,

Joel

#36 lore raw

PATCH v4 29/31 drivers: fsi: occ: Fix client memory management

In reply to Eddie James

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

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

---
 drivers/fsi/occ.c | 92 ++++++++++++++++++++++++++-----------------------------
 1 file changed, 43 insertions(+), 49 deletions(-)
drivers/fsi/occ.c +43 -49
index ec42fc0..800e8b6 100644
@@ -70,15 +70,11 @@ struct occ_response {
  *  and cleared if the transfer fails or occ_worker_getsram completes.
  * XFR_COMPLETE is set when a transfer fails or finishes occ_worker_getsram.
  * XFR_CANCELED is set when the transfer's client is released.
- * XFR_WAITING is set from read() if the transfer isn't complete and
- *  O_NONBLOCK wasn't specified. Cleared in read() when transfer completes or
- *  fails.
  */
 enum {
 	XFR_IN_PROGRESS,
 	XFR_COMPLETE,
 	XFR_CANCELED,
-	XFR_WAITING,
 };
 
 struct occ_xfr {
@@ -104,6 +100,7 @@ enum {
 };
 
 struct occ_client {
+	struct kref kref;
 	struct occ *occ;
 	struct occ_xfr xfr;
 	spinlock_t lock;		/* lock access to the client state */
@@ -140,6 +137,24 @@ static int occ_enqueue_xfr(struct occ_xfr *xfr)
 	return 0;
 }
 
+static void occ_get_client(struct occ_client *client)
+{
+	kref_get(&client->kref);
+}
+
+static void occ_client_release(struct kref *kref)
+{
+	struct occ_client *client = container_of(kref, struct occ_client,
+						 kref);
+
+	kfree(client);
+}
+
+static void occ_put_client(struct occ_client *client)
+{
+	kref_put(&client->kref, occ_client_release);
+}
+
 static struct occ_client *occ_open_common(struct occ *occ, unsigned long flags)
 {
 	struct occ_client *client = kzalloc(sizeof(*client), GFP_KERNEL);
@@ -148,6 +163,7 @@ static struct occ_client *occ_open_common(struct occ *occ, unsigned long flags)
 		return NULL;
 
 	client->occ = occ;
+	kref_init(&client->kref);
 	spin_lock_init(&client->lock);
 	init_waitqueue_head(&client->wait);
 
@@ -192,6 +208,7 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
 	if (len > OCC_SRAM_BYTES)
 		return -EINVAL;
 
+	occ_get_client(client);
 	xfr = &client->xfr;
 	occ = client->occ;
 
@@ -215,8 +232,6 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
 			goto done;
 		}
 
-		set_bit(XFR_WAITING, &xfr->flags);
-
 		spin_unlock_irq(&client->lock);
 
 		rc = wait_event_interruptible(client->wait,
@@ -224,15 +239,12 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
 
 		spin_lock_irq(&client->lock);
 
-		if (test_bit(XFR_CANCELED, &xfr->flags)) {
-			spin_unlock_irq(&client->lock);
-			kfree(client);
-			return -EBADFD;
-		}
-
-		clear_bit(XFR_WAITING, &xfr->flags);
 		if (!test_bit(XFR_COMPLETE, &xfr->flags)) {
-			rc = -EINTR;
+			if (occ->cancel || test_bit(XFR_CANCELED, &xfr->flags))
+				rc = -ECANCELED;
+			else
+				rc = -EINTR;
+
 			goto done;
 		}
 	}
@@ -263,6 +275,7 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
 
 done:
 	spin_unlock_irq(&client->lock);
+	occ_put_client(client);
 	return rc;
 }
 
@@ -289,6 +302,7 @@ static ssize_t occ_write_common(struct occ_client *client,
 	if (len > (OCC_CMD_DATA_BYTES + 3) || len < 3)
 		return -EINVAL;
 
+	occ_get_client(client);
 	xfr = &client->xfr;
 
 	spin_lock_irq(&client->lock);
@@ -340,6 +354,7 @@ static ssize_t occ_write_common(struct occ_client *client,
 
 done:
 	spin_unlock_irq(&client->lock);
+	occ_put_client(client);
 	return rc;
 }
 
@@ -364,38 +379,26 @@ static int occ_release_common(struct occ_client *client)
 
 	spin_lock_irq(&client->lock);
 
-	if (!test_bit(CLIENT_XFR_PENDING, &client->flags)) {
-		spin_unlock_irq(&client->lock);
-		kfree(client);
-		return 0;
-	}
+	set_bit(XFR_CANCELED, &xfr->flags);
+	if (!test_bit(CLIENT_XFR_PENDING, &client->flags))
+		goto done;
 
 	spin_lock_irq(&occ->list_lock);
 
-	set_bit(XFR_CANCELED, &xfr->flags);
 	if (!test_bit(XFR_IN_PROGRESS, &xfr->flags)) {
 		/* already deleted from list if complete */
 		if (!test_bit(XFR_COMPLETE, &xfr->flags))
 			list_del(&xfr->link);
-
-		spin_unlock_irq(&occ->list_lock);
-
-		if (test_bit(XFR_WAITING, &xfr->flags)) {
-			/* blocking read; let reader clean up */
-			wake_up_interruptible(&client->wait);
-			spin_unlock_irq(&client->lock);
-			return 0;
-		}
-
-		spin_unlock_irq(&client->lock);
-
-		kfree(client);
-		return 0;
 	}
 
-	/* operation is in progress; let worker clean up */
 	spin_unlock_irq(&occ->list_lock);
+
+	wake_up_all(&client->wait);
+
+done:
 	spin_unlock_irq(&client->lock);
+
+	occ_put_client(client);
 	return 0;
 }
 
@@ -601,7 +604,7 @@ static int occ_trigger_attn(struct device *sbefifo)
 
 static void occ_worker(struct work_struct *work)
 {
-	int rc = 0, empty, waiting, canceled;
+	int rc = 0, empty;
 	u16 resp_data_length;
 	unsigned long start;
 	const unsigned long timeout = msecs_to_jiffies(OCC_TIMEOUT_MS);
@@ -624,6 +627,8 @@ static void occ_worker(struct work_struct *work)
 		return;
 	}
 
+	client = to_client(xfr);
+	occ_get_client(client);
 	resp = (struct occ_response *)xfr->buf;
 	set_bit(XFR_IN_PROGRESS, &xfr->flags);
 
@@ -679,29 +684,18 @@ static void occ_worker(struct work_struct *work)
 	mutex_unlock(&occ->occ_lock);
 
 	xfr->rc = rc;
-	client = to_client(xfr);
-
-	/* lock client to prevent race with read() */
-	spin_lock_irq(&client->lock);
-
 	set_bit(XFR_COMPLETE, &xfr->flags);
-	waiting = test_bit(XFR_WAITING, &xfr->flags);
-
-	spin_unlock_irq(&client->lock);
 
 	spin_lock_irq(&occ->list_lock);
 
 	clear_bit(XFR_IN_PROGRESS, &xfr->flags);
 	list_del(&xfr->link);
 	empty = list_empty(&occ->xfrs);
-	canceled = test_bit(XFR_CANCELED, &xfr->flags);
 
 	spin_unlock_irq(&occ->list_lock);
 
-	if (waiting)
-		wake_up_interruptible(&client->wait);
-	else if (canceled)
-		kfree(client);
+	wake_up_interruptible(&client->wait);
+	occ_put_client(client);
 
 	if (!empty)
 		goto again;
Signature · 1 line
1.8.3.1
Acked-by on v4 29/31
#37 lore raw

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

Quoted 6 lines · Eddie James

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

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

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

For the moment:

Quoted 225 lines

---
 drivers/fsi/occ.c | 92 ++++++++++++++++++++++++++-----------------------------

 1 file changed, 43 insertions(+), 49 deletions(-)

drivers/fsi/occ.c +0 -4
index ec42fc0..800e8b6 100644
@@ -70,15 +70,11 @@ struct occ_response {
   *  and cleared if the transfer fails or occ_worker_getsram completes.
   * XFR_COMPLETE is set when a transfer fails or finishes occ_worker_getsram.
   * XFR_CANCELED is set when the transfer's client is released.
- * XFR_WAITING is set from read() if the transfer isn't complete and
- *  O_NONBLOCK wasn't specified. Cleared in read() when transfer completes or
- *  fails.
   */
  enum {
  	XFR_IN_PROGRESS,
  	XFR_COMPLETE,
  	XFR_CANCELED,
-	XFR_WAITING,
  };


 struct occ_xfr {

drivers/fsi/occ.c +0 -0
@@ -104,6 +100,7 @@ enum {
  };


 struct occ_client {

drivers/fsi/occ.c +1 -0
+	struct kref kref;

  struct occ *occ;
  struct occ_xfr xfr;
  spinlock_t lock; /* lock access to the client state */

drivers/fsi/occ.c +0 -0
@@ -140,6 +137,24 @@ static int occ_enqueue_xfr(struct occ_xfr *xfr)
  	return 0;
  }

drivers/fsi/occ.c +18 -0
+static void occ_get_client(struct occ_client *client)
+{
+	kref_get(&client->kref);
+}
+
+static void occ_client_release(struct kref *kref)
+{
+	struct occ_client *client = container_of(kref, struct occ_client,
+						 kref);
+
+	kfree(client);
+}
+
+static void occ_put_client(struct occ_client *client)
+{
+	kref_put(&client->kref, occ_client_release);
+}
+

 static struct occ_client *occ_open_common(struct occ *occ, unsigned long flags) {
  struct occ_client *client = kzalloc(sizeof(*client), GFP_KERNEL);

drivers/fsi/occ.c +0 -0
@@ -148,6 +163,7 @@ static struct occ_client *occ_open_common(struct occ *occ, unsigned long flags)
  		return NULL;


  client->occ = occ;

drivers/fsi/occ.c +1 -0
+	kref_init(&client->kref);

  spin_lock_init(&client->lock);
  init_waitqueue_head(&client->wait);

drivers/fsi/occ.c +0 -0
@@ -192,6 +208,7 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
  	if (len > OCC_SRAM_BYTES)
  		return -EINVAL;

drivers/fsi/occ.c +1 -0
+	occ_get_client(client);

  xfr = &client->xfr;
  occ = client->occ;

drivers/fsi/occ.c +0 -0
@@ -215,8 +232,6 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
  			goto done;
  		}

drivers/fsi/occ.c +0 -2
-		set_bit(XFR_WAITING, &xfr->flags);
-

  spin_unlock_irq(&client->lock);

  rc = wait_event_interruptible(client->wait,

drivers/fsi/occ.c +0 -0
@@ -224,15 +239,12 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,


  spin_lock_irq(&client->lock);

drivers/fsi/occ.c +0 -7
-		if (test_bit(XFR_CANCELED, &xfr->flags)) {
-			spin_unlock_irq(&client->lock);
-			kfree(client);
-			return -EBADFD;
-		}
-
-		clear_bit(XFR_WAITING, &xfr->flags);

  if (!test_bit(XFR_COMPLETE, &xfr->flags)) {

drivers/fsi/occ.c +5 -1
-			rc = -EINTR;
+			if (occ->cancel || test_bit(XFR_CANCELED, &xfr->flags))
+				rc = -ECANCELED;
+			else
+				rc = -EINTR;
+

  goto done;
  }
  }

drivers/fsi/occ.c +0 -0
@@ -263,6 +275,7 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,


 done:
  spin_unlock_irq(&client->lock);

drivers/fsi/occ.c +1 -0
+	occ_put_client(client);

  return rc;
 }

drivers/fsi/occ.c +0 -0
@@ -289,6 +302,7 @@ static ssize_t occ_write_common(struct occ_client *client,
  	if (len > (OCC_CMD_DATA_BYTES + 3) || len < 3)
  		return -EINVAL;

drivers/fsi/occ.c +1 -0
+	occ_get_client(client);

  xfr = &client->xfr;

  spin_lock_irq(&client->lock);

drivers/fsi/occ.c +0 -0
@@ -340,6 +354,7 @@ static ssize_t occ_write_common(struct occ_client *client,


 done:
  spin_unlock_irq(&client->lock);

drivers/fsi/occ.c +1 -0
+	occ_put_client(client);

  return rc;
 }

drivers/fsi/occ.c +0 -0
@@ -364,38 +379,26 @@ static int occ_release_common(struct occ_client *client)


  spin_lock_irq(&client->lock);

drivers/fsi/occ.c +3 -5
-	if (!test_bit(CLIENT_XFR_PENDING, &client->flags)) {
-		spin_unlock_irq(&client->lock);
-		kfree(client);
-		return 0;
-	}
+	set_bit(XFR_CANCELED, &xfr->flags);
+	if (!test_bit(CLIENT_XFR_PENDING, &client->flags))
+		goto done;


  spin_lock_irq(&occ->list_lock);

drivers/fsi/occ.c +0 -1
-	set_bit(XFR_CANCELED, &xfr->flags);

  if (!test_bit(XFR_IN_PROGRESS, &xfr->flags)) { /* already deleted from list if complete */ if (!test_bit(XFR_COMPLETE, &xfr->flags)) list_del(&xfr->link);

drivers/fsi/occ.c +0 -14
-
-		spin_unlock_irq(&occ->list_lock);
-
-		if (test_bit(XFR_WAITING, &xfr->flags)) {
-			/* blocking read; let reader clean up */
-			wake_up_interruptible(&client->wait);
-			spin_unlock_irq(&client->lock);
-			return 0;
-		}
-
-		spin_unlock_irq(&client->lock);
-
-		kfree(client);
-		return 0;

  }

drivers/fsi/occ.c +0 -1
-	/* operation is in progress; let worker clean up */

  spin_unlock_irq(&occ->list_lock);

drivers/fsi/occ.c +4 -0
+
+	wake_up_all(&client->wait);
+
+done:

  spin_unlock_irq(&client->lock);

drivers/fsi/occ.c +2 -0
+
+	occ_put_client(client);

  return 0;
 }

drivers/fsi/occ.c +0 -0
@@ -601,7 +604,7 @@ static int occ_trigger_attn(struct device *sbefifo)


 static void occ_worker(struct work_struct *work)
 {

drivers/fsi/occ.c +1 -1
-	int rc = 0, empty, waiting, canceled;
+	int rc = 0, empty;

  u16 resp_data_length;
  unsigned long start;
  const unsigned long timeout = msecs_to_jiffies(OCC_TIMEOUT_MS);

drivers/fsi/occ.c +0 -0
@@ -624,6 +627,8 @@ static void occ_worker(struct work_struct *work)
  		return;
  	}

drivers/fsi/occ.c +2 -0
+	client = to_client(xfr);
+	occ_get_client(client);

  resp = (struct occ_response *)xfr->buf;
  set_bit(XFR_IN_PROGRESS, &xfr->flags);

drivers/fsi/occ.c +0 -0
@@ -679,29 +684,18 @@ static void occ_worker(struct work_struct *work)
  	mutex_unlock(&occ->occ_lock);


  xfr->rc = rc;

drivers/fsi/occ.c +0 -5
-	client = to_client(xfr);
-
-	/* lock client to prevent race with read() */
-	spin_lock_irq(&client->lock);
-

  set_bit(XFR_COMPLETE, &xfr->flags);

drivers/fsi/occ.c +0 -3
-	waiting = test_bit(XFR_WAITING, &xfr->flags);
-
-	spin_unlock_irq(&client->lock);


  spin_lock_irq(&occ->list_lock);

  clear_bit(XFR_IN_PROGRESS, &xfr->flags);
  list_del(&xfr->link);
  empty = list_empty(&occ->xfrs);

drivers/fsi/occ.c +0 -1
-	canceled = test_bit(XFR_CANCELED, &xfr->flags);


  spin_unlock_irq(&occ->list_lock);

drivers/fsi/occ.c +2 -4
-	if (waiting)
-		wake_up_interruptible(&client->wait);
-	else if (canceled)
-		kfree(client);
+	wake_up_interruptible(&client->wait);
+	occ_put_client(client);


  if (!empty)
  goto again;

#38 lore raw

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

In reply to Eddie James
Quoted 93 lines · Eddie James

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

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

---
  drivers/fsi/occ.c | 92 ++++++++++++++++++++++++++-----------------------------
  1 file changed, 43 insertions(+), 49 deletions(-)
drivers/fsi/occ.c +21 -7
index ec42fc0..800e8b6 100644
@@ -70,15 +70,11 @@ struct occ_response {
   *  and cleared if the transfer fails or occ_worker_getsram completes.
   * XFR_COMPLETE is set when a transfer fails or finishes occ_worker_getsram.
   * XFR_CANCELED is set when the transfer's client is released.
- * XFR_WAITING is set from read() if the transfer isn't complete and
- *  O_NONBLOCK wasn't specified. Cleared in read() when transfer completes or
- *  fails.
   */
  enum {
  	XFR_IN_PROGRESS,
  	XFR_COMPLETE,
  	XFR_CANCELED,
-	XFR_WAITING,
  };
 
  struct occ_xfr {
@@ -104,6 +100,7 @@ enum {
  };
 
  struct occ_client {
+	struct kref kref;
  	struct occ *occ;
  	struct occ_xfr xfr;
  	spinlock_t lock;		/* lock access to the client state */
@@ -140,6 +137,24 @@ static int occ_enqueue_xfr(struct occ_xfr *xfr)
  	return 0;
  }
 
+static void occ_get_client(struct occ_client *client)
+{
+	kref_get(&client->kref);
+}
+
+static void occ_client_release(struct kref *kref)
+{
+	struct occ_client *client = container_of(kref, struct occ_client,
+						 kref);
+
+	kfree(client);
+}
+
+static void occ_put_client(struct occ_client *client)
+{
+	kref_put(&client->kref, occ_client_release);
+}
+
  static struct occ_client *occ_open_common(struct occ *occ, unsigned long flags)
  {
  	struct occ_client *client = kzalloc(sizeof(*client), GFP_KERNEL);
@@ -148,6 +163,7 @@ static struct occ_client *occ_open_common(struct occ *occ, unsigned long flags)
  		return NULL;
 
  	client->occ = occ;
+	kref_init(&client->kref);
  	spin_lock_init(&client->lock);
  	init_waitqueue_head(&client->wait);
 
@@ -192,6 +208,7 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
  	if (len > OCC_SRAM_BYTES)
  		return -EINVAL;
 
+	occ_get_client(client);
  	xfr = &client->xfr;
  	occ = client->occ;
 
@@ -215,8 +232,6 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
  			goto done;
  		}
 
-		set_bit(XFR_WAITING, &xfr->flags);
-
  		spin_unlock_irq(&client->lock);
 
  		rc = wait_event_interruptible(client->wait,
@@ -224,15 +239,12 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
 
  		spin_lock_irq(&client->lock);
 
-		if (test_bit(XFR_CANCELED, &xfr->flags)) {

drivers/fsi/occ.c static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf, · line 244

Quoted 10 lines
drivers/fsi/occ.c +2 -7
-			spin_unlock_irq(&client->lock);
-			kfree(client);
-			return -EBADFD;
-		}
-
-		clear_bit(XFR_WAITING, &xfr->flags);
  		if (!test_bit(XFR_COMPLETE, &xfr->flags)) {
-			rc = -EINTR;
+			if (occ->cancel || test_bit(XFR_CANCELED, &xfr->flags))
+				rc = -ECANCELED;

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

Quoted 128 lines
drivers/fsi/occ.c +20 -35
+			else
+				rc = -EINTR;
+
  			goto done;
  		}
  	}
@@ -263,6 +275,7 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
 
  done:
  	spin_unlock_irq(&client->lock);
+	occ_put_client(client);
  	return rc;
  }
 
@@ -289,6 +302,7 @@ static ssize_t occ_write_common(struct occ_client *client,
  	if (len > (OCC_CMD_DATA_BYTES + 3) || len < 3)
  		return -EINVAL;
 
+	occ_get_client(client);
  	xfr = &client->xfr;
 
  	spin_lock_irq(&client->lock);
@@ -340,6 +354,7 @@ static ssize_t occ_write_common(struct occ_client *client,
 
  done:
  	spin_unlock_irq(&client->lock);
+	occ_put_client(client);
  	return rc;
  }
 
@@ -364,38 +379,26 @@ static int occ_release_common(struct occ_client *client)
 
  	spin_lock_irq(&client->lock);
 
-	if (!test_bit(CLIENT_XFR_PENDING, &client->flags)) {
-		spin_unlock_irq(&client->lock);
-		kfree(client);
-		return 0;
-	}
+	set_bit(XFR_CANCELED, &xfr->flags);
+	if (!test_bit(CLIENT_XFR_PENDING, &client->flags))
+		goto done;
 
  	spin_lock_irq(&occ->list_lock);
 
-	set_bit(XFR_CANCELED, &xfr->flags);
  	if (!test_bit(XFR_IN_PROGRESS, &xfr->flags)) {
  		/* already deleted from list if complete */
  		if (!test_bit(XFR_COMPLETE, &xfr->flags))
  			list_del(&xfr->link);
-
-		spin_unlock_irq(&occ->list_lock);
-
-		if (test_bit(XFR_WAITING, &xfr->flags)) {
-			/* blocking read; let reader clean up */
-			wake_up_interruptible(&client->wait);
-			spin_unlock_irq(&client->lock);
-			return 0;
-		}
-
-		spin_unlock_irq(&client->lock);
-
-		kfree(client);
-		return 0;
  	}
 
-	/* operation is in progress; let worker clean up */
  	spin_unlock_irq(&occ->list_lock);
+
+	wake_up_all(&client->wait);
+
+done:
  	spin_unlock_irq(&client->lock);
+
+	occ_put_client(client);
  	return 0;
  }
 
@@ -601,7 +604,7 @@ static int occ_trigger_attn(struct device *sbefifo)
 
  static void occ_worker(struct work_struct *work)
  {
-	int rc = 0, empty, waiting, canceled;
+	int rc = 0, empty;
  	u16 resp_data_length;
  	unsigned long start;
  	const unsigned long timeout = msecs_to_jiffies(OCC_TIMEOUT_MS);
@@ -624,6 +627,8 @@ static void occ_worker(struct work_struct *work)
  		return;
  	}
 
+	client = to_client(xfr);
+	occ_get_client(client);
  	resp = (struct occ_response *)xfr->buf;
  	set_bit(XFR_IN_PROGRESS, &xfr->flags);
 
@@ -679,29 +684,18 @@ static void occ_worker(struct work_struct *work)
  	mutex_unlock(&occ->occ_lock);
 
  	xfr->rc = rc;
-	client = to_client(xfr);
-
-	/* lock client to prevent race with read() */
-	spin_lock_irq(&client->lock);
-
  	set_bit(XFR_COMPLETE, &xfr->flags);
-	waiting = test_bit(XFR_WAITING, &xfr->flags);
-
-	spin_unlock_irq(&client->lock);
 
  	spin_lock_irq(&occ->list_lock);
 
  	clear_bit(XFR_IN_PROGRESS, &xfr->flags);
  	list_del(&xfr->link);
  	empty = list_empty(&occ->xfrs);
-	canceled = test_bit(XFR_CANCELED, &xfr->flags);
 
  	spin_unlock_irq(&occ->list_lock);
 
-	if (waiting)
-		wake_up_interruptible(&client->wait);
-	else if (canceled)
-		kfree(client);
+	wake_up_interruptible(&client->wait);
+	occ_put_client(client);
 
  	if (!empty)
  		goto again;
#39 lore raw

PATCH v4 30/31 drivers/hwmon/occ: Remove repeated ops for OCC command in progress

In reply to Eddie James

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

This is now handled in the occ driver.

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

PATCH v4 31/31 drivers: hwmon: occ: Cancel occ operations in remove()

In reply to Eddie James

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

Prevent hanging forever waiting for OCC ops to complete.

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

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

Quoted 23 lines · Eddie James

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

Prevent hanging forever waiting for OCC ops to complete.

---
 drivers/hwmon/occ/p9_sbe.c | 35 ++++++++++++++++++++++++++++-------

 1 file changed, 28 insertions(+), 7 deletions(-)
drivers/hwmon/occ/p9_sbe.c +1 -0
index c7e0d9c..9842e0d 100644
@@ -14,37 +14,54 @@
  #include <linux/platform_device.h>
  #include <linux/occ.h>
  #include <linux/sched.h>
+#include <linux/spinlock.h>
  #include <linux/workqueue.h>


 struct p9_sbe_occ {
  struct occ occ;
  struct device *sbe;

drivers/hwmon/occ/p9_sbe.c line 24

Quoted 2 lines
drivers/hwmon/occ/p9_sbe.c +2 -0
+	struct occ_client *client;
+	spinlock_t lock;

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

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

Andrew

Quoted 84 lines

 };

 #define to_p9_sbe_occ(x) container_of((x), struct p9_sbe_occ,
occ)

drivers/hwmon/occ/p9_sbe.c +11 -0
+static void p9_sbe_occ_close_client(struct p9_sbe_occ *occ)
+{
+	struct occ_client *tmp_client;
+
+	spin_lock_irq(&occ->lock);
+	tmp_client = occ->client;
+	occ->client = NULL;
+	occ_drv_release(tmp_client);
+	spin_unlock_irq(&occ->lock);
+}
+

 static int p9_sbe_occ_send_cmd(struct occ *occ, u8 *cmd) {
  int rc, error;

drivers/hwmon/occ/p9_sbe.c +0 -1
-	struct occ_client *client;

  struct occ_response *resp = &occ->resp; struct p9_sbe_occ *p9_sbe_occ = to_p9_sbe_occ(occ);

drivers/hwmon/occ/p9_sbe.c +3 -2
-	client = occ_drv_open(p9_sbe_occ->sbe, 0);
-	if (!client) {
+	spin_lock_irq(&p9_sbe_occ->lock);
+	if (p9_sbe_occ->sbe)
+		p9_sbe_occ->client = occ_drv_open(p9_sbe_occ->sbe,

0);

drivers/hwmon/occ/p9_sbe.c +3 -0
+	spin_unlock_irq(&p9_sbe_occ->lock);
+
+	if (!p9_sbe_occ->client) {

  rc = -ENODEV;
  goto assign;
  }

drivers/hwmon/occ/p9_sbe.c +1 -1
-	rc = occ_drv_write(client, (const char *)&cmd[1], 7);
+	rc = occ_drv_write(p9_sbe_occ->client, (const char

*)&cmd[1], 7);
  if (rc < 0)
  goto err;

drivers/hwmon/occ/p9_sbe.c +1 -1
-	rc = occ_drv_read(client, (char *)resp, sizeof(*resp));
+	rc = occ_drv_read(p9_sbe_occ->client, (char *)resp,

sizeof(*resp));
  if (rc < 0)
  goto err;

drivers/hwmon/occ/p9_sbe.c +1 -1
-	occ_drv_release(client);
+	p9_sbe_occ_close_client(p9_sbe_occ);


  switch (resp->return_status) {
  case RESP_RETURN_CMD_IN_PRG:

drivers/hwmon/occ/p9_sbe.c +0 -0
@@ -72,7 +89,7 @@ static int p9_sbe_occ_send_cmd(struct occ *occ, u8
 *cmd)
  	goto done;


 err:

drivers/hwmon/occ/p9_sbe.c +1 -1
-	occ_drv_release(client);
+	p9_sbe_occ_close_client(p9_sbe_occ);

  dev_err(occ->bus_dev, "occ bus op failed rc:%d\n", rc); assign:
  error = rc;

drivers/hwmon/occ/p9_sbe.c +0 -0
@@ -132,6 +149,7 @@ static int p9_sbe_occ_probe(struct
 platform_device *pdev)
  	p9_sbe_occ->sbe = pdev->dev.parent;


  occ = &p9_sbe_occ->occ;

drivers/hwmon/occ/p9_sbe.c +1 -0
+	spin_lock_init(&p9_sbe_occ->lock);

  occ->bus_dev = &pdev->dev;
  occ->groups[0] = &occ->group;
  occ->poll_cmd_data = 0x20;

drivers/hwmon/occ/p9_sbe.c +1 -0
@@ -152,7 +170,10 @@ static int p9_sbe_occ_probe(struct
 platform_device *pdev)
  static int p9_sbe_occ_remove(struct platform_device *pdev)
  {
  	struct occ *occ = platform_get_drvdata(pdev);
+	struct p9_sbe_occ *p9_sbe_occ = to_p9_sbe_occ(occ);

drivers/hwmon/occ/p9_sbe.c +2 -0
+	p9_sbe_occ->sbe = NULL;
+	p9_sbe_occ_close_client(p9_sbe_occ);

  occ_remove_status_attrs(occ);

  atomic_dec(&occ_num_occs);