drivers: fsi: client fixes and refactor

PATCH Superseded Eddie James 2 participants 69 messages Oct 5, 2017 – Oct 6, 2017

Subsystems (2)

Hardware monitoring FSI

lore

Share & tools

lore

Eddie James

author

2017-10-05 19:24 UTC #0 lore raw

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

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

Signature · 1 line

1.8.3.1

Eddie James

author

2017-10-05 19:24 UTC #1 lore raw

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

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

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

Andrew Jeffery

Acked-by on v3 1/31

2017-10-05 21:30 UTC #2 lore raw

Re: [PATCH linux dev-4.10 v3 01/31] drivers: fsi: sbefifo: Fix includes

Quoted 5 lines · Eddie James

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

Need to include everything we use and not rely on recursive includes.
Signed-off-by Edward A. James <[email protected]>

Acked-by Andrew Jeffery <[email protected]>

Quoted 38 lines

---
drivers/fsi/fsi-sbefifo.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)
drivers/fsi/fsi-sbefifo.c +0 -0
index 1c37ff7..6fa75a8 100644
@@ -11,20 +11,27 @@
   * GNU General Public License for more details.
   */
drivers/fsi/fsi-sbefifo.c +1 -1
-#include <linux/delay.h>
+#include <linux/device.h>
#include <linux/errno.h>
drivers/fsi/fsi-sbefifo.c +1 -1
-#include <linux/idr.h>
+#include <linux/fs.h>
#include <linux/fsi.h>
drivers/fsi/fsi-sbefifo.c +4 -0
+#include <linux/fsi-sbefifo.h>
+#include <linux/idr.h>
+#include <linux/kernel.h>
+#include <linux/kref.h>
#include <linux/list.h>
#include <linux/miscdevice.h>
#include <linux/module.h>
#include <linux/of.h>
+#include <linux/of_device.h>
#include <linux/of_platform.h>
#include <linux/poll.h>
#include <linux/sched.h>
#include <linux/slab.h>
+#include <linux/spinlock.h>
#include <linux/timer.h>
#include <linux/uaccess.h>
+#include <linux/wait.h>

/*
* The SBEFIFO is a pipe-like FSI device for communicating with

Eddie James

author

2017-10-05 19:24 UTC #3 lore raw

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

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

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

Andrew Jeffery

Acked-by on v3 2/31

2017-10-05 21:32 UTC #4 lore raw

Re: [PATCH linux dev-4.10 v3 02/31] drivers: fsi: sbefifo: Use a defined reschedule length

Quoted 5 lines · Eddie James

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

Switch to a #defined reschedule period instead of using 500 ms twice.
Signed-off-by Edward A. James <[email protected]>

Acked-by Andrew Jeffery <[email protected]>

Quoted 35 lines

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

Eddie James

author

2017-10-05 19:24 UTC #5 lore raw

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

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

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

Andrew Jeffery

Reviewed-by on v3 3/31

2017-10-05 21:37 UTC #6 lore raw

Re: [PATCH linux dev-4.10 v3 03/31] drivers: fsi: sbefifo: Use __be32 for big endian values

Quoted 5 lines · Eddie James

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

Use a real __be32 value instead of u32 for big endian values.
Signed-off-by Edward A. James <[email protected]>

Reviewed-by Andrew Jeffery <[email protected]>

Quoted 38 lines

---
drivers/fsi/fsi-sbefifo.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
drivers/fsi/fsi-sbefifo.c +1 -1
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));
drivers/fsi/fsi-sbefifo.c +0 -0
@@ -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) {
drivers/fsi/fsi-sbefifo.c +1 -1
-	u32 raw_word = cpu_to_be32(word);
+	__be32 raw_word = cpu_to_be32(word);
return fsi_device_write(sbefifo->fsi_dev, reg, &raw_word, sizeof(raw_word));
}

+/* Don't flip endianness of data to/from FIFO, just pass through. */ static int sbefifo_readw(struct sbefifo *sbefifo, u32 *word) {
return fsi_device_read(sbefifo->fsi_dev, SBEFIFO_DWN, word,

Eddie James

author

2017-10-05 19:24 UTC #7 lore raw

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

In reply to Eddie James

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

Fix some spacing issues.

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

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

Andrew Jeffery

Reviewed-by on v3 4/31

2017-10-05 21:43 UTC #8 lore raw

Re: [PATCH linux dev-4.10 v3 04/31] drivers: fsi: sbefifo: white space fixes

Quoted 5 lines · Eddie James

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

Fix some spacing issues.
Signed-off-by Edward A. James <[email protected]>

Reviewed-by Andrew Jeffery <[email protected]>

Quoted 246 lines

---
drivers/fsi/fsi-sbefifo.c | 66 ++++++++++++++++++++++++-----------------------
 1 file changed, 34 insertions(+), 32 deletions(-)
drivers/fsi/fsi-sbefifo.c +0 -0
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,
drivers/fsi/fsi-sbefifo.c +1 -1
-			SBEFIFO_EOT_MAGIC);
+			    SBEFIFO_EOT_MAGIC);
}

static size_t sbefifo_dev_nwreadable(u32 sts)
drivers/fsi/fsi-sbefifo.c +0 -0
@@ -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;
}
drivers/fsi/fsi-sbefifo.c +0 -0
@@ -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)
{
drivers/fsi/fsi-sbefifo.c +1 -1
-	struct sbefifo *sbefifo;
+	struct sbefifo *sbefifo = container_of(kref, struct sbefifo, kref);
drivers/fsi/fsi-sbefifo.c +0 -1
-	sbefifo = container_of(kref, struct sbefifo, kref);
kfree(sbefifo);
}
drivers/fsi/fsi-sbefifo.c +0 -0
@@ -284,8 +285,7 @@ static struct sbefifo_xfr *sbefifo_client_next_xfr(
  	if (list_empty(&client->xfrs))
  		return NULL;
drivers/fsi/fsi-sbefifo.c +1 -2
-	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)
drivers/fsi/fsi-sbefifo.c +1 -0
@@ -360,6 +360,7 @@ static struct sbefifo_xfr *sbefifo_next_xfr(struct sbefifo *sbefifo)
  			kfree(xfr);
  			continue;
  		}
+
  		return xfr;
  	}
drivers/fsi/fsi-sbefifo.c +0 -0
@@ -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,
drivers/fsi/fsi-sbefifo.c +1 -1
-			xfrs);
+				       xfrs);
if (!xfr)
goto out_unlock;
drivers/fsi/fsi-sbefifo.c +0 -0
@@ -399,8 +400,7 @@ static void sbefifo_poll_timer(unsigned long data)
/* Drain the write buffer. */
while ((bufn = sbefifo_buf_nbreadable(wbuf))) {
drivers/fsi/fsi-sbefifo.c +1 -2
-		ret = sbefifo_inw(sbefifo, SBEFIFO_UP | SBEFIFO_STS,
-				&sts);
+		ret = sbefifo_inw(sbefifo, SBEFIFO_UP | SBEFIFO_STS, &sts);
if (ret)
goto out;
drivers/fsi/fsi-sbefifo.c +0 -0
@@ -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)) {
drivers/fsi/fsi-sbefifo.c +2 -3
-		ret = sbefifo_outw(sbefifo,
-				SBEFIFO_UP | SBEFIFO_EOT_RAISE,
-				SBEFIFO_EOT_MAGIC);
+		ret = sbefifo_outw(sbefifo, SBEFIFO_UP | SBEFIFO_EOT_RAISE,
+				   SBEFIFO_EOT_MAGIC);
if (ret)
goto out;
drivers/fsi/fsi-sbefifo.c +3 -3
@@ -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;
drivers/fsi/fsi-sbefifo.c +0 -0
@@ -562,12 +561,13 @@ static ssize_t sbefifo_read_common(struct sbefifo_client *client,
sbefifo_get_client(client);
if (wait_event_interruptible(sbefifo->wait,
drivers/fsi/fsi-sbefifo.c +3 -3
-				(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);
drivers/fsi/fsi-sbefifo.c +2 -1
@@ -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);
drivers/fsi/fsi-sbefifo.c +0 -0
@@ -605,8 +606,8 @@ static ssize_t sbefifo_read_common(struct sbefifo_client *client,
  	return n;
  }
drivers/fsi/fsi-sbefifo.c +2 -2
-static ssize_t sbefifo_read(struct file *file, char __user *buf,
-		size_t len, loff_t *offset)
+static ssize_t sbefifo_read(struct file *file, char __user *buf, size_t len,
+			    loff_t *offset)
{
struct sbefifo_client *client = file->private_data;
drivers/fsi/fsi-sbefifo.c +1 -0
@@ -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);
drivers/fsi/fsi-sbefifo.c +0 -0
@@ -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) ||
drivers/fsi/fsi-sbefifo.c +2 -2
-					(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);
drivers/fsi/fsi-sbefifo.c +1 -0
@@ -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,
drivers/fsi/fsi-sbefifo.c +1 -1
-		size_t len, loff_t *offset)
+			     size_t len, loff_t *offset)
{
struct sbefifo_client *client = file->private_data;
drivers/fsi/fsi-sbefifo.c +0 -0
@@ -820,22 +823,21 @@ static int sbefifo_probe(struct device *dev)
sbefifo->fsi_dev = fsi_dev;
drivers/fsi/fsi-sbefifo.c +1 -2
-	ret = sbefifo_inw(sbefifo,
-			SBEFIFO_UP | SBEFIFO_STS, &sts);
+	ret = sbefifo_inw(sbefifo, SBEFIFO_UP | SBEFIFO_STS, &sts);
if (ret)
return ret;
+
if (!(sts & SBEFIFO_EMPTY)) {
drivers/fsi/fsi-sbefifo.c +1 -2
-		dev_err(&sbefifo->fsi_dev->dev,
-				"Found data in upstream fifo\n");
+		dev_err(dev, "Found data in upstream fifo\n");
return -EIO;
}

ret = sbefifo_inw(sbefifo, SBEFIFO_DWN | SBEFIFO_STS, &sts); if (ret)
return ret;
+
if (!(sts & SBEFIFO_EMPTY)) {
drivers/fsi/fsi-sbefifo.c +1 -2
-		dev_err(&sbefifo->fsi_dev->dev,
-				"Found data in downstream fifo\n");
+		dev_err(dev, "Found data in downstream fifo\n");
return -EIO;
}
drivers/fsi/fsi-sbefifo.c +0 -0
@@ -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",
drivers/fsi/fsi-sbefifo.c +1 -1
-			sbefifo->idx);
+		 sbefifo->idx);
init_waitqueue_head(&sbefifo->wait); INIT_LIST_HEAD(&sbefifo->xfrs);

/* This bit of silicon doesn't offer any interrupts... */ setup_timer(&sbefifo->poll_timer, sbefifo_poll_timer,
drivers/fsi/fsi-sbefifo.c +1 -1
-			(unsigned long)sbefifo);
+		    (unsigned long)sbefifo);
if (dev->of_node) {
/* create platform devs for dts child nodes (occ, etc) */
drivers/fsi/fsi-sbefifo.c +1 -1
@@ -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");
  		}
  	}

Eddie James

author

2017-10-05 19:25 UTC #9 lore raw

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

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

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

Andrew Jeffery

Reviewed-by on v3 5/31

2017-10-05 21:46 UTC #10 lore raw

Re: [PATCH linux dev-4.10 v3 05/31] drivers: fsi: sbefifo: replace awkward wait_event expression

Quoted 6 lines · Eddie James

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

Use ready() functions to replace awkward boolean expressions in the wait_event_interruptible calls.
Signed-off-by Edward A. James <[email protected]>

Reviewed-by Andrew Jeffery <[email protected]>

drivers/fsi/fsi-sbefifo.c static unsigned int sbefifo_poll(struct file

Quoted 9 lines

---
drivers/fsi/fsi-sbefifo.c | 32 +++++++++++++++++++++++++-------
 1 file changed, 25 insertions(+), 7 deletions(-)
drivers/fsi/fsi-sbefifo.c +0 -0
index 561a7f6..dbac0c3 100644
@@ -545,6 +545,16 @@ static unsigned int sbefifo_poll(struct file

*file, poll_table *wait)

drivers/fsi/fsi-sbefifo.c

Quoted 5 lines

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

  	return mask;
  }

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

+static bool sbefifo_read_ready(struct sbefifo *sbefifo,
+			       struct sbefifo_client *client, size_t

*n,

Quoted 10 lines

drivers/fsi/fsi-sbefifo.c +8 -0
+			       size_t *ret)
+{
+	*n = sbefifo_buf_nbreadable(&client->rbuf);
+	*ret = READ_ONCE(sbefifo->rc);
+
+	return *ret || *n;
+}
+
static ssize_t sbefifo_read_common(struct sbefifo_client *client, char __user *ubuf, char *kbuf,

size_t len)

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

Quoted 2 lines

{
drivers/fsi/fsi-sbefifo.c +0 -0
@@ -561,9 +571,8 @@ static ssize_t sbefifo_read_common(struct

sbefifo_client *client,

drivers/fsi/fsi-sbefifo.c

Quoted 4 lines

sbefifo_get_client(client);
if (wait_event_interruptible(sbefifo->wait,
drivers/fsi/fsi-sbefifo.c +0 -1
-				     (ret = READ_ONCE(sbefifo->rc))

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

Quoted 3 lines

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

-				     (n = sbefifo_buf_nbreadable(
-					&client->rbuf)))) {
+				     sbefifo_read_ready(sbefifo,

client, &n,

drivers/fsi/fsi-sbefifo.c static ssize_t sbefifo_read(struct file *file,

Quoted 5 lines

drivers/fsi/fsi-sbefifo.c +1 -0
+							&ret))) {
sbefifo_put_client(client);
return -ERESTARTSYS;
}
drivers/fsi/fsi-sbefifo.c +0 -0
@@ -619,6 +628,16 @@ static ssize_t sbefifo_read(struct file *file,

char __user *buf, size_t len,

drivers/fsi/fsi-sbefifo.c static ssize_t sbefifo_read(struct file *file, char __user *buf, size_t len,

Quoted 6 lines

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

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

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

+static bool sbefifo_write_ready(struct sbefifo *sbefifo,
+				struct sbefifo_xfr *xfr,
+				struct sbefifo_client *client,

size_t *n)

Quoted 9 lines

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

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

Quoted 2 lines

size_t len)
drivers/fsi/fsi-sbefifo.c +0 -0
@@ -660,10 +679,9 @@ static ssize_t sbefifo_write_common(struct

sbefifo_client *client,

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

Quoted 5 lines

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

  	 */
  	while (len) {
  		if (wait_event_interruptible(sbefifo->wait,
-				READ_ONCE(sbefifo->rc) ||
-				(sbefifo_client_next_xfr(client) ==

xfr &&

drivers/fsi/fsi-sbefifo.c static ssize_t sbefifo_write_common(struct · line 683

Quoted 4 lines

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

-				 (n = sbefifo_buf_nbwriteable(
-							&client-
 wbuf))))) {
+					     sbefifo_write_ready(sbe

fifo, xfr,

drivers/fsi/fsi-sbefifo.c static ssize_t sbefifo_write_common(struct · line 684

Quoted 1 line

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

ent,

drivers/fsi/fsi-sbefifo.c static ssize_t sbefifo_write_common(struct · line 685

Quoted 1 line

drivers/fsi/fsi-sbefifo.c +1 -0
+								 &n)

)) {

Quoted 3 lines

drivers/fsi/fsi-sbefifo.c +0 -0
  			set_bit(SBEFIFO_XFR_CANCEL, &xfr->flags);
  			sbefifo_get(sbefifo);
if (mod_timer(&sbefifo->poll_timer,

jiffies))

Eddie James

author

2017-10-05 19:24 UTC #11 lore raw

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

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

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

Andrew Jeffery

2017-10-05 21:48 UTC #12 lore raw

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

Quoted 4 lines · Eddie James

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

sbefifo_client_next_xfr function just does what list_first_entry_or_null does.

Any reason you didn't implement sbefifo_client_next_xfr() in terms of list_first_entry_or_null() rather than open-code it?

Quoted 62 lines

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

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

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

-static struct sbefifo_xfr *sbefifo_client_next_xfr(
-		struct sbefifo_client *client)
-{
-	if (list_empty(&client->xfrs))
-		return NULL;
-
-	return container_of(client->xfrs.next, struct sbefifo_xfr, client);
-}
-

static bool sbefifo_xfr_rsp_pending(struct sbefifo_client *client) {

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

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

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

-	xfr = sbefifo_client_next_xfr(client);

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

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

@@ -595,7 +587,15 @@ static ssize_t sbefifo_read_common(struct sbefifo_client *client,
  	}

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

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

-		xfr = sbefifo_client_next_xfr(client);
+		xfr = list_first_entry_or_null(&client->xfrs,
+					       struct sbefifo_xfr, client);
+		if (!xfr) {
+			/* should be impossible to not have an xfr here */
+			WARN_ONCE(1, "no xfr in queue");
+			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 +3 -1

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

Eddie James

author

2017-10-06 02:03 UTC #13 lore raw

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

Quoted 7 lines · Andrew Jeffery

Quoted 4 lines · Eddie James
From: "Edward A. James" <[email protected]>
sbefifo_client_next_xfr function just does what list_first_entry_or_null does.
Any reason you didn't implement sbefifo_client_next_xfr() in terms of list_first_entry_or_null() rather than open-code it?

I prefer simplicity of using the kernel helper. With a custom function, it's not clear if it's doing some additional work unless you go look at it. With the list function, it's clear it's just returning the top entry.

Thanks for the review!
Eddie

Quoted 63 lines

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

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

Andrew Jeffery

Reviewed-by

2017-10-06 02:04 UTC #14 lore raw

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

Quoted 8 lines · Eddie James

Quoted 6 lines · Andrew Jeffery
Quoted 4 lines · Eddie James
Quoted 1 line
Quoted 1 line
Quoted 1 line
From: "Edward A. James" <[email protected]>

sbefifo_client_next_xfr function just does what list_first_entry_or_null does.

Quoted 6 lines

Quoted 2 lines
Any reason you didn't implement sbefifo_client_next_xfr() in terms of list_first_entry_or_null() rather than open-code it?
I prefer simplicity of using the kernel helper. With a custom function, it's not clear if it's doing some additional work unless you go look at it. With the list function, it's clear it's just returning the top entry.

Sure. I'm not terribly fussed either way, was just interested.

Reviewed-by Andrew Jeffery <[email protected]>

Quoted 69 lines

Thanks for the review!
Eddie
Quoted 63 lines
Quoted 63 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
Signed-off-by Edward A. James <[email protected]>
---
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)
 > > >   	return xfr;
   }
-static struct sbefifo_xfr *sbefifo_client_next_xfr(
Quoted 1 line
Quoted 1 line
drivers/fsi/fsi-sbefifo.c static struct sbefifo_xfr *sbefifo_enq_xfr(struct sbefifo_client *client) · line 283
Quoted 1 line
drivers/fsi/fsi-sbefifo.c +0 -1
-		struct sbefifo_client *client)
-{
Quoted 2 lines
Quoted 2 lines
drivers/fsi/fsi-sbefifo.c static struct sbefifo_xfr *sbefifo_enq_xfr(struct sbefifo_client *client) · line 286
Quoted 2 lines
drivers/fsi/fsi-sbefifo.c +0 -2
-	if (list_empty(&client->xfrs))
-		return NULL;
-
Quoted 1 line
Quoted 1 line
Quoted 1 line
drivers/fsi/fsi-sbefifo.c +0 -1
-	return container_of(client->xfrs.next, struct sbefifo_xfr, client);
drivers/fsi/fsi-sbefifo.c +0 -2
-}
-
static bool sbefifo_xfr_rsp_pending(struct sbefifo_client *client) {
Quoted 4 lines
Quoted 4 lines
drivers/fsi/fsi-sbefifo.c static struct sbefifo_xfr *sbefifo_enq_xfr(struct sbefifo_client *client) · line 286
Quoted 4 lines
drivers/fsi/fsi-sbefifo.c +3 -1
-	struct sbefifo_xfr *xfr;
+	struct sbefifo_xfr *xfr = list_first_entry_or_null(&client->xfrs,
+							   struct sbefifo_xfr,
+							   client);
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
(quoted diff) +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,
 > > >   	}
Quoted 10 lines
Quoted 10 lines
drivers/fsi/fsi-sbefifo.c static ssize_t sbefifo_read_common(struct sbefifo_client *client, · line 597
Quoted 10 lines
if (sbefifo_buf_readnb(&client->rbuf, n)) {
drivers/fsi/fsi-sbefifo.c +8 -1
-		xfr = sbefifo_client_next_xfr(client);
+		xfr = list_first_entry_or_null(&client->xfrs,
+					       struct sbefifo_xfr, client);
+		if (!xfr) {
+			/* should be impossible to not have an xfr here */
+			WARN_ONCE(1, "no xfr in queue");
+			sbefifo_put_client(client);
+			return -EPROTO;
+		}
+
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
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,
 > > >   				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);
Quoted 1 line
Quoted 1 line
Quoted 1 line
*n = sbefifo_buf_nbwriteable(&client->wbuf);
return READ_ONCE(sbefifo->rc) || (next == xfr && *n);

Eddie James

author

2017-10-05 19:24 UTC #15 lore raw

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

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

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

Andrew Jeffery

Reviewed-by on v3 7/31

2017-10-05 21:52 UTC #16 lore raw

Re: [PATCH linux dev-4.10 v3 07/31] drivers: fsi: sbefifo: Use goto to reduce put statements

Quoted 6 lines · 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.
Signed-off-by Edward A. James <[email protected]>

Reviewed-by Andrew Jeffery <[email protected]>

Quoted 101 lines

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

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

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

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

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

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

}

n = min_t(size_t, n, len);

if (ubuf) {
if (copy_to_user(ubuf, READ_ONCE(client->rbuf.rpos), n)) {

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

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

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

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

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

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

@@ -610,9 +609,11 @@ static ssize_t sbefifo_read_common(struct sbefifo_client *client,
  		}
  	}

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

-	sbefifo_put_client(client);
+	ret = n;

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

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

}

static ssize_t sbefifo_read(struct file *file, char __user *buf, size_t len,

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

@@ -689,14 +690,14 @@ static ssize_t sbefifo_write_common(struct sbefifo_client *client,
  			if (mod_timer(&sbefifo->poll_timer, jiffies))
  				sbefifo_put(sbefifo);

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

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

}

if (sbefifo->rc) { INIT_LIST_HEAD(&client->xfrs);

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

-			sbefifo_put_client(client);
-			return sbefifo->rc;
+			ret = sbefifo->rc;
+			goto out;

}

n = min_t(size_t, n, len);

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

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

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

@@ -736,8 +737,8 @@ static ssize_t sbefifo_write_common(struct sbefifo_client *client,
  			sbefifo_put(sbefifo);
  	}

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

Eddie James

author

2017-10-05 19:24 UTC #17 lore raw

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

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

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

Andrew Jeffery

Acked-by on v3 8/31

2017-10-05 21:58 UTC #18 lore raw

Re: [PATCH linux dev-4.10 v3 08/31] drivers: fsi: sbefifo: Do an earlier get_client call

Quoted 5 lines · Eddie James

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

Get client before we start using it.
Signed-off-by Edward A. James <[email protected]>

I still think the refounting of clients is a bit odd, but if we need to do it this seems reasonable.

Acked-by Andrew Jeffery <[email protected]>

Quoted 40 lines

---
drivers/fsi/fsi-sbefifo.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)
drivers/fsi/fsi-sbefifo.c +0 -0
index a5bdc75..5efba30 100644
@@ -656,6 +656,7 @@ static ssize_t sbefifo_write_common(struct sbefifo_client *client,
  	if (!len)
  		return 0;
drivers/fsi/fsi-sbefifo.c +1 -0
+	sbefifo_get_client(client);
n = sbefifo_buf_nbwriteable(&client->wbuf);
spin_lock_irq(&sbefifo->lock);
drivers/fsi/fsi-sbefifo.c +0 -0
@@ -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);
drivers/fsi/fsi-sbefifo.c +2 -1
-		return -EAGAIN;
+		ret = -EAGAIN;
+		goto out;
}

xfr = sbefifo_enq_xfr(client);
if (!xfr) {
spin_unlock_irq(&sbefifo->lock);
drivers/fsi/fsi-sbefifo.c +2 -1
-		return -ENOMEM;
+		ret = -ENOMEM;
+		goto out;
}

spin_unlock_irq(&sbefifo->lock);
drivers/fsi/fsi-sbefifo.c +0 -2
-	sbefifo_get_client(client);
-
/*
* Partial writes are not really allowed in that EOT is sent exactly
* once per write.

Eddie James

author

2017-10-05 19:24 UTC #19 lore raw

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

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

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

Andrew Jeffery

Reviewed-by on v3 9/31

2017-10-05 22:01 UTC #20 lore raw

Re: [PATCH linux dev-4.10 v3 09/31] drivers: fsi: sbefifo: Remove warning and user data access check

Quoted 6 lines · 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.
Signed-off-by Edward A. James <[email protected]>

Reviewed-by Andrew Jeffery <[email protected]>

Quoted 32 lines

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

 1 file changed, 10 deletions(-)

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

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;

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

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

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

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

@@ -748,11 +743,6 @@ static ssize_t sbefifo_write(struct file *file, const char __user *buf,
  {
  	struct sbefifo_client *client = file->private_data;

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

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

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

Eddie James

author

2017-10-05 19:24 UTC #21 lore raw

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

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

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

Andrew Jeffery

Reviewed-by on v3 10/31

2017-10-05 22:06 UTC #22 lore raw

Re: [PATCH linux dev-4.10 v3 10/31] drivers: fsi: sbefifo: destroy the ida list on exit

Quoted 5 lines · Eddie James

From: "Edward A. James" <[email protected]>
Need to clean this up.
Signed-off-by Edward A. James <[email protected]>

Reviewed-by Andrew Jeffery <[email protected]>

Quoted 17 lines

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

Eddie James

author

2017-10-05 19:24 UTC #23 lore raw

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

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

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

Andrew Jeffery

Reviewed-by on v3 11/31

2017-10-05 22:14 UTC #24 lore raw

Re: [PATCH linux dev-4.10 v3 11/31] drivers: fsi: sbefifo: Fix module authors and comments

Quoted 5 lines · Eddie James

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

Clarify the xfr situation in write() and fix module authors
Signed-off-by Edward A. James <[email protected]>

Reviewed-by Andrew Jeffery <[email protected]>

Quoted 45 lines

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

 1 file changed, 7 insertions(+), 5 deletions(-)

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

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

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

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

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

@@ -663,7 +663,7 @@ static ssize_t sbefifo_write_common(struct sbefifo_client *client,
  		goto out;
  	}

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

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

if (!xfr) {
spin_unlock_irq(&sbefifo->lock);
ret = -ENOMEM;

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

@@ -719,8 +719,9 @@ static ssize_t sbefifo_write_common(struct sbefifo_client *client,
  		len -= n;
  		ret += n;

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

-		/* set flag before starting the worker, as it may run through
-		 * and check the flag before we exit this loop!
+		/*
+		 * Set this before starting timer to avoid race condition on
+		 * this flag with the timer function writer.

*/
if (!len)
set_bit(SBEFIFO_XFR_WRITE_DONE, &xfr->flags);

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

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

Eddie James

author

2017-10-05 19:24 UTC #25 lore raw

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

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

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

Andrew Jeffery

Reviewed-by on v3 12/31

2017-10-05 22:16 UTC #26 lore raw

Re: [PATCH linux dev-4.10 v3 12/31] drivers: fsi: sbefifo: Fix include guards in header file

Quoted 5 lines · Eddie James

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

Make them conform to Linux standard headers.
Signed-off-by Edward A. James <[email protected]>

Reviewed-by Andrew Jeffery <[email protected]>

Quoted 25 lines

---
include/linux/fsi-sbefifo.h | 6 +++---

 1 file changed, 3 insertions(+), 3 deletions(-)

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

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

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

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

struct device;
struct sbefifo_client;

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

@@ -27,4 +27,4 @@ extern int sbefifo_drv_write(struct sbefifo_client *client, const char *buf,
  			     size_t len);
  extern void sbefifo_drv_release(struct sbefifo_client *client);

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

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

Eddie James

author

2017-10-05 19:24 UTC #27 lore raw

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

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

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

Andrew Jeffery

Reviewed-by on v3 13/31

2017-10-05 22:48 UTC #28 lore raw

Re: [PATCH linux dev-4.10 v3 13/31] drivers: fsi: SBEFIFO: Fix probe() and remove()

Quoted 7 lines · Eddie James

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

Probe didn't handle a failed misc device registration properly. Remove had the potential for hangs. Also, remove the list of SBEFIFOs and instead just use the device "driver data" pointer.
Signed-off-by Edward A. James <[email protected]>

Reviewed-by Andrew Jeffery <[email protected]>

Quoted 211 lines

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

 1 file changed, 53 insertions(+), 58 deletions(-)

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

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

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

-static struct list_head sbefifo_fifos;
-

static DEFINE_IDA(sbefifo_ida);

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

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

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

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

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

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

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

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

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

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

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

sbefifo_put(sbefifo);

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

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

out_unlock:
spin_unlock(&sbefifo->lock);

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

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

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

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

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

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

-	if (!xfr) {
+	if (IS_ERR(xfr)) {

spin_unlock_irq(&sbefifo->lock);

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

-		ret = -ENOMEM;
+		ret = PTR_ERR(xfr);

goto out;
}

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

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

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

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

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

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

return client;
}

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

@@ -854,69 +851,68 @@ static int sbefifo_probe(struct device *dev)
  		return -EIO;
  	}

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

-	sbefifo->mdev.minor = MISC_DYNAMIC_MINOR;
-	sbefifo->mdev.fops = &sbefifo_fops;
-	sbefifo->mdev.name = sbefifo->name;
-	sbefifo->mdev.parent = dev;

spin_lock_init(&sbefifo->lock);
kref_init(&sbefifo->kref);

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

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

sbefifo->idx = ida_simple_get(&sbefifo_ida, 1, INT_MAX, GFP_KERNEL); snprintf(sbefifo->name, sizeof(sbefifo->name), "sbefifo%d", sbefifo->idx);

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

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

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

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

-	if (dev->of_node) {
-		/* create platform devs for dts child nodes (occ, etc) */
-		for_each_child_of_node(dev->of_node, np) {
-			snprintf(child_name, sizeof(child_name), "%s-dev%d",
-				 sbefifo->name, child_idx++);
-			child = of_platform_device_create(np, child_name, dev);
-			if (!child)
-				dev_warn(dev,
-					 "failed to create child node dev\n");
-		}
+	sbefifo->mdev.minor = MISC_DYNAMIC_MINOR;
+	sbefifo->mdev.fops = &sbefifo_fops;
+	sbefifo->mdev.name = sbefifo->name;
+	sbefifo->mdev.parent = dev;
+	ret = misc_register(&sbefifo->mdev);
+	if (ret) {
+		dev_err(dev, "failed to register miscdevice: %d\n", ret);
+		ida_simple_remove(&sbefifo_ida, sbefifo->idx);
+		sbefifo_put(sbefifo);
+		return ret;

}

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

-	list_add(&sbefifo->link, &sbefifo_fifos);
-	
-	return misc_register(&sbefifo->mdev);
+	/* create platform devs for dts child nodes (occ, etc) */
+	for_each_available_child_of_node(dev->of_node, np) {
+		snprintf(child_name, sizeof(child_name), "%s-dev%d",
+			 sbefifo->name, child_idx++);
+		child = of_platform_device_create(np, child_name, dev);
+		if (!child)
+			dev_warn(dev, "failed to create child %s dev\n",
+				 child_name);
+	}
+
+	dev_set_drvdata(dev, sbefifo);
+
+	return 0;

}

static int sbefifo_remove(struct device *dev)
{

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

-	struct fsi_device *fsi_dev = to_fsi_dev(dev);
-	struct sbefifo *sbefifo, *sbefifo_tmp;
+	struct sbefifo *sbefifo = dev_get_drvdata(dev);

struct sbefifo_xfr *xfr;

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

-	list_for_each_entry_safe(sbefifo, sbefifo_tmp, &sbefifo_fifos, link) {
-		if (sbefifo->fsi_dev != fsi_dev)
-			continue;
+	WRITE_ONCE(sbefifo->rc, -ENODEV);
+	wake_up_all(&sbefifo->wait);

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

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

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

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

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

-		spin_lock(&sbefifo->lock);
-		list_for_each_entry(xfr, &sbefifo->xfrs, xfrs)
-			kfree(xfr);
-		spin_unlock(&sbefifo->lock);
+	if (del_timer_sync(&sbefifo->poll_timer))
+		sbefifo_put(sbefifo);

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

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

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

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

return 0;
}

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

@@ -941,7 +937,6 @@ static int sbefifo_remove(struct device *dev)

static int sbefifo_init(void)
{

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

-	INIT_LIST_HEAD(&sbefifo_fifos);

return fsi_driver_register(&sbefifo_drv);
}

Eddie James

author

2017-10-05 19:24 UTC #29 lore raw

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

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

---
 drivers/fsi/fsi-sbefifo.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

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

Andrew Jeffery

Reviewed-by on v3 14/31

2017-10-05 22:50 UTC #30 lore raw

Re: [PATCH linux dev-4.10 v3 14/31] drivers: fsi: SBEFIFO: check for xfr complete in read wait_event

Quoted 7 lines · 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.
Signed-off-by Edward A. James <[email protected]>

Reviewed-by Andrew Jeffery <[email protected]>

Quoted 25 lines

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

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

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

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

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

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

}

static ssize_t sbefifo_read_common(struct sbefifo_client *client,

Eddie James

author

2017-10-05 19:24 UTC #31 lore raw

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

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

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

Andrew Jeffery

Acked-by on v3 15/31

2017-10-05 22:52 UTC #32 lore raw

Re: [PATCH linux dev-4.10 v3 15/31] drivers: fsi: occ: Fix includes

Quoted 5 lines · Eddie James

From: "Edward A. James" <[email protected]>
Need to include everything we use and not rely on recursive includes.
Signed-off-by Edward A. James <[email protected]>

Acked-by Andrew Jeffery <[email protected]>

Quoted 31 lines

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

Eddie James

author

2017-10-05 19:24 UTC #33 lore raw

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

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

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

Andrew Jeffery

Reviewed-by on v3 16/31

2017-10-05 22:58 UTC #34 lore raw

Re: [PATCH linux dev-4.10 v3 16/31] drivers: fsi: occ: Fix errant kfree calls

Quoted 3 lines · Eddie James

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

Left over from when xfr was an independant structure. Remove these

calls

Quoted 3 lines

as xfr is a part of the client struct.
Signed-off-by Edward A. James <[email protected]>

Reviewed-by Andrew Jeffery <[email protected]>

drivers/fsi/occ.c static ssize_t occ_write_common(struct occ_client

Quoted 9 lines

---
drivers/fsi/occ.c | 2 --

 1 file changed, 2 deletions(-)

drivers/fsi/occ.c +0 -0

index 2d7df8a..cd60c5a 100644
@@ -289,7 +289,6 @@ static ssize_t occ_write_common(struct occ_client

*client,

drivers/fsi/occ.c static ssize_t occ_write_common(struct occ_client

Quoted 8 lines

if (ubuf) {
if (copy_from_user(&xfr->buf[1], ubuf, len)) {
drivers/fsi/occ.c +0 -1
-			kfree(xfr);
rc = -EFAULT;
goto done;
}
drivers/fsi/occ.c +0 -0
@@ -298,7 +297,6 @@ static ssize_t occ_write_common(struct occ_client

*client,

Quoted 7 lines

data_length = (xfr->buf[2] << 8) + xfr->buf[3]; if (data_length > OCC_CMD_DATA_BYTES) {
drivers/fsi/occ.c +0 -1
-		kfree(xfr);
rc = -EINVAL;
goto done;
}

Eddie James

author

2017-10-05 19:24 UTC #35 lore raw

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

In reply to Eddie James

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

Signed-off-by 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

Andrew Jeffery

Reviewed-by on v3 17/31

2017-10-05 22:59 UTC #36 lore raw

Re: [PATCH linux dev-4.10 v3 17/31] drivers: fsi: occ: remove unused occ_command structure

Quoted 3 lines · Eddie James

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

Reviewed-by Andrew Jeffery <[email protected]>

Quoted 23 lines

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

Eddie James

author

2017-10-05 19:24 UTC #37 lore raw

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

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

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

Andrew Jeffery

2017-10-05 23:03 UTC #38 lore raw

Re: [PATCH linux dev-4.10 v3 18/31] drivers: fsi: occ: Use big-endian values

Quoted 76 lines · 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.

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

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

 1 file changed, 8 insertions(+), 6 deletions(-)

drivers/fsi/occ.c +2 -2

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;

drivers/fsi/occ.c +1 -1

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

drivers/fsi/occ.c +2 -2

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

drivers/fsi/occ.c +1 -0

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

drivers/fsi/occ.c +1 -0

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

set_bit(XFR_IN_PROGRESS, &xfr->flags);

spin_unlock_irq(&occ->list_lock);

drivers/fsi/occ.c +0 -0

@@ -591,7 +593,7 @@ static void occ_worker(struct work_struct *work)
  	if (rc)
  		goto done;

drivers/fsi/occ.c +1 -1

-	resp_data_length = (xfr->buf[3] << 8) + xfr->buf[4];
+	resp_data_length = get_unaligned_be16(&resp->data_length);

Quoted 1 line

if (resp_data_length > OCC_RESP_DATA_BYTES) {

You need to do be16_to_cpu() before any comparisons.

Quoted 2 lines

rc = -EDOM;
goto done;

Eddie James

author

2017-10-06 00:41 UTC #39 lore raw

Re: [PATCH linux dev-4.10 v3 18/31] drivers: fsi: occ: Use big-endian values

Quoted 75 lines · Andrew Jeffery

Quoted 74 lines · 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.

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

---
  drivers/fsi/occ.c | 14 ++++++++------
  1 file changed, 8 insertions(+), 6 deletions(-)

drivers/fsi/occ.c +7 -5

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;

Quoted 4 lines

drivers/fsi/occ.c static void occ_worker(struct work_struct *work) · line 597
Quoted 3 lines
drivers/fsi/occ.c +1 -1
-	resp_data_length = (xfr->buf[3] << 8) + xfr->buf[4];
+	resp_data_length = get_unaligned_be16(&resp->data_length);
  	if (resp_data_length > OCC_RESP_DATA_BYTES) {
You need to do be16_to_cpu() before any comparisons.

Hmm, I'm not convinced. I see this in access_ok.h

static __always_inline <http://elixir.free-electrons.com/linux/latest/ident/__always_inline> u16 get_unaligned_be16 <http://elixir.free-electrons.com/linux/latest/ident/get_unaligned_be16>(const void *p) {
return be16_to_cpup
<http://elixir.free-electrons.com/linux/latest/ident/be16_to_cpup>((__be16 <http://elixir.free-electrons.com/linux/latest/ident/__be16> *)p);
} I think it will do the translation as well. Thanks, Eddie

Quoted 3 lines

Quoted 3 lines
drivers/fsi/occ.c +0 -0
  		rc = -EDOM;
  		goto done;

Andrew Jeffery

Reviewed-by

2017-10-06 00:53 UTC #40 lore raw

Re: [PATCH linux dev-4.10 v3 18/31] drivers: fsi: occ: Use big-endian values

Quoted 87 lines · Eddie James

Quoted 74 lines · Andrew Jeffery
Quoted 73 lines · Eddie James
Quoted 1 line
Quoted 1 line
Quoted 1 line
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.
Quoted 1 line
Quoted 1 line
Quoted 1 line
Signed-off-by Edward A. James <[email protected]>
---
drivers/fsi/occ.c | 14 ++++++++------
  1 file changed, 8 insertions(+), 6 deletions(-)
drivers/fsi/occ.c +0 -0
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;
/*
drivers/fsi/occ.c +0 -0
@@ -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;
drivers/fsi/occ.c +0 -0
@@ -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;
Quoted 1 line
Quoted 1 line
Quoted 1 line
struct sbefifo_client *client;
drivers/fsi/occ.c +0 -0
@@ -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;
Quoted 1 line
Quoted 1 line
Quoted 1 line
buf[0] = cpu_to_be32(0x6);
drivers/fsi/occ.c +0 -0
@@ -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);
Quoted 1 line
Quoted 1 line
Quoted 1 line
struct device *sbefifo = occ->sbefifo;
drivers/fsi/occ.c +0 -0
@@ -573,6 +574,7 @@ static void occ_worker(struct work_struct *work)
 > > >   		return;
 > > >   	}
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
drivers/fsi/occ.c +1 -0
+	resp = (struct occ_response *)xfr->buf;
set_bit(XFR_IN_PROGRESS, &xfr->flags);
Quoted 1 line
Quoted 1 line
Quoted 1 line
spin_unlock_irq(&occ->list_lock);
drivers/fsi/occ.c +0 -0
@@ -591,7 +593,7 @@ static void occ_worker(struct work_struct *work)
Quoted 8 lines
Quoted 6 lines
drivers/fsi/occ.c +0 -0
 > > >   	if (rc)
 > > >   		goto done;
Quoted 2 lines
Quoted 2 lines
drivers/fsi/occ.c static void occ_worker(struct work_struct *work) · line 596
Quoted 2 lines
drivers/fsi/occ.c +1 -1
-	resp_data_length = (xfr->buf[3] << 8) + xfr->buf[4];
+	resp_data_length = get_unaligned_be16(&resp->data_length);
if (resp_data_length > OCC_RESP_DATA_BYTES) {
You need to do be16_to_cpu() before any comparisons.
Hmm, I'm not convinced. I see this in access_ok.h

Quoted 8 lines

static  __always_inline
Quoted 1 line
<http://elixir.free-electrons.com/linux/latest/ident/__always_inline>  u16  get_unaligned_be16
<http://elixir.free-electrons.com/linux/latest/ident/get_unaligned_be16>(const  void  *p)
{
return  be16_to_cpup
<http://elixir.free-electrons.com/linux/latest/ident/be16_to_cpup>((__be16
Quoted 1 line
<http://elixir.free-electrons.com/linux/latest/ident/__be16>  *)p);
} I think it will do the translation as well. Thanks, Eddie

Ugh, yeah, brain fart. Sorry about that.

Reviewed-by Andrew Jeffery <[email protected]>

Quoted 6 lines

Quoted 4 lines
Quoted 4 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
rc = -EDOM;
goto done;

Eddie James

author

2017-10-05 19:25 UTC #41 lore raw

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

In reply to Eddie James

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

Safeguard the in-kernel api functions.

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

---
 drivers/fsi/occ.c | 9 +++++++++
 1 file changed, 9 insertions(+)

drivers/fsi/occ.c +9 -0

index 55f293d..a551349 100644
@@ -654,18 +654,27 @@ struct occ_client *occ_drv_open(struct device *dev, unsigned long flags)
 
 int occ_drv_read(struct occ_client *client, char *buf, size_t len)
 {
+	if (!client)
+		return -ENODEV;
+
 	return occ_read_common(client, NULL, buf, len);
 }
 EXPORT_SYMBOL_GPL(occ_drv_read);
 
 int occ_drv_write(struct occ_client *client, const char *buf, size_t len)
 {
+	if (!client)
+		return -ENODEV;
+
 	return occ_write_common(client, NULL, buf, len);
 }
 EXPORT_SYMBOL_GPL(occ_drv_write);
 
 void occ_drv_release(struct occ_client *client)
 {
+	if (!client)
+		return;
+
 	occ_release_common(client);
 }
 EXPORT_SYMBOL_GPL(occ_drv_release);

Signature · 1 line

1.8.3.1

Andrew Jeffery

2017-10-05 23:05 UTC #42 lore raw

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

Quoted 3 lines · Eddie James

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

Safeguard the in-kernel api functions.

Why just the in-kernel API functions? Why not do the check in *{read,write,release}_common()?

drivers/fsi/occ.c struct occ_client *occ_drv_open(struct device

Quoted 11 lines

Signed-off-by Edward A. James <[email protected]>
---
drivers/fsi/occ.c | 9 +++++++++
 1 file changed, 9 insertions(+)
drivers/fsi/occ.c +0 -0
index 55f293d..a551349 100644
@@ -654,18 +654,27 @@ struct occ_client *occ_drv_open(struct device

*dev, unsigned long flags)

Quoted 11 lines

int occ_drv_read(struct occ_client *client, char *buf, size_t len) {
drivers/fsi/occ.c +3 -0
+	if (!client)
+		return -ENODEV;
+
return occ_read_common(client, NULL, buf, len);
}
EXPORT_SYMBOL_GPL(occ_drv_read);

int occ_drv_write(struct occ_client *client, const char *buf, size_t

len)

Quoted 16 lines

{
drivers/fsi/occ.c +3 -0
+	if (!client)
+		return -ENODEV;
+
return occ_write_common(client, NULL, buf, len); }
EXPORT_SYMBOL_GPL(occ_drv_write);

void occ_drv_release(struct occ_client *client) {
drivers/fsi/occ.c +3 -0
+	if (!client)
+		return;
+
occ_release_common(client);
}
EXPORT_SYMBOL_GPL(occ_drv_release);

Eddie James

author

2017-10-05 19:25 UTC #43 lore raw

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

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

---
 drivers/fsi/occ.c | 12 ------------
 1 file changed, 12 deletions(-)

drivers/fsi/occ.c +0 ~~-12~~

index a551349..7ced9b6 100644
@@ -247,12 +247,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);
 }

@@ -316,12 +310,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

Andrew Jeffery

Reviewed-by on v3 20/31

2017-10-05 23:07 UTC #44 lore raw

Re: [PATCH linux dev-4.10 v3 20/31] drivers: fsi: occ: Remove early user buffer checking

Quoted 6 lines · Eddie James

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

We check user buffer access when we copy the data in read()/write() anyway.
Signed-off-by Edward A. James <[email protected]>

Reviewed-by Andrew Jeffery <[email protected]>

Quoted 34 lines

---
drivers/fsi/occ.c | 12 ------------

 1 file changed, 12 deletions(-)

drivers/fsi/occ.c +0 -0

index a551349..7ced9b6 100644
@@ -247,12 +247,6 @@ static ssize_t occ_read(struct file *file, char __user *buf, size_t len,
  {
  	struct occ_client *client = file->private_data;

drivers/fsi/occ.c +0 -6

-	/* check this ahead of time so we don't go changing the xfr state
-	 * needlessly
-	 */
-	if (!access_ok(VERIFY_WRITE, buf, len))
-		return -EFAULT;
-

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

drivers/fsi/occ.c +0 -0

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

drivers/fsi/occ.c +0 -6

-	/* check this ahead of time so we don't go changing the xfr state
-	 * needlessly
-	 */
-	if (!access_ok(VERIFY_READ, buf, len))
-		return -EFAULT;
-

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

Eddie James

author

2017-10-05 19:25 UTC #45 lore raw

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

In reply to Eddie James

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

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

---
 drivers/fsi/occ.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

drivers/fsi/occ.c +6 -6

index 7ced9b6..4e677c6 100644
@@ -386,7 +386,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,

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

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

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

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

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

Andrew Jeffery

Acked-by on v3 21/31

2017-10-05 23:09 UTC #46 lore raw

Re: [PATCH linux dev-4.10 v3 21/31] drivers: fsi: occ: Switch to more logical errnos

Quoted 3 lines · Eddie James

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

Acked-by Andrew Jeffery <[email protected]>

drivers/fsi/occ.c static int occ_write_sbefifo(struct

Quoted 9 lines

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

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

drivers/fsi/occ.c +0 -0

index 7ced9b6..4e677c6 100644
@@ -386,7 +386,7 @@ static int occ_write_sbefifo(struct

sbefifo_client *client, const char *buf,

Quoted 8 lines

drivers/fsi/occ.c +0 -0
  		total += rc;
  	} while (total < len);
drivers/fsi/occ.c +1 -1
-	return (total == len) ? 0 : -EMSGSIZE;
+	return (total == len) ? 0 : -ENOSPC;
}

static int occ_read_sbefifo(struct sbefifo_client *client, char

*buf,

drivers/fsi/occ.c static int occ_read_sbefifo(struct sbefifo_client

Quoted 1 line

drivers/fsi/occ.c +0 -0

@@ -405,7 +405,7 @@ static int occ_read_sbefifo(struct sbefifo_client

*client, char *buf,

Quoted 8 lines

drivers/fsi/occ.c +0 -0
  		total += rc;
  	} while (total < len);
drivers/fsi/occ.c +1 -1
-	return (total == len) ? 0 : -EMSGSIZE;
+	return (total == len) ? 0 : -ENODATA;
}

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

*data,

drivers/fsi/occ.c static int occ_getsram(struct device *sbefifo,

Quoted 1 line

drivers/fsi/occ.c +0 -0

@@ -450,7 +450,7 @@ static int occ_getsram(struct device *sbefifo,

u32 address, u8 *data,

drivers/fsi/occ.c static int occ_putsram(struct device *sbefifo,

Quoted 9 lines

drivers/fsi/occ.c +0 -0

  	    (be32_to_cpu(buf[1]) == 0xC0DEA403))
  		memcpy(data, resp, len);

else

drivers/fsi/occ.c +1 -1

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

free:
kfree(resp);

drivers/fsi/occ.c +0 -0

@@ -498,7 +498,7 @@ static int occ_putsram(struct device *sbefifo,

u32 address, u8 *data,

drivers/fsi/occ.c static int occ_trigger_attn(struct device

Quoted 9 lines

drivers/fsi/occ.c +0 -0
  	/* check for good response */
if ((be32_to_cpu(buf[0]) != data_len) ||
(be32_to_cpu(buf[1]) != 0xC0DEA404))
drivers/fsi/occ.c +1 -1
-		rc = -EFAULT;
+		rc = -EBADMSG;
done:
sbefifo_drv_release(client);
drivers/fsi/occ.c +0 -0
@@ -535,7 +535,7 @@ static int occ_trigger_attn(struct device

*sbefifo)

Quoted 17 lines

drivers/fsi/occ.c +0 -0
  	/* check for good response */
if ((be32_to_cpu(buf[0]) != 0xC0DEA202) || (be32_to_cpu(buf[1]) & 0x0FFFFFFF))
drivers/fsi/occ.c +1 -1
-		rc = -EFAULT;
+		rc = -EBADMSG;
done:
sbefifo_drv_release(client);
drivers/fsi/occ.c +0 -0
@@ -583,7 +583,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) {
drivers/fsi/occ.c +1 -1
-		rc = -EDOM;
+		rc = -EMSGSIZE;
goto done;
}

Eddie James

author

2017-10-05 19:25 UTC #47 lore raw

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

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

---
 drivers/fsi/occ.c | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

drivers/fsi/occ.c +15 ~~-10~~

index 4e677c6..f27345c 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;

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

@@ -197,8 +197,10 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
 		spin_unlock_irq(&client->lock);
 
 		rc = wait_event_interruptible(client->wait,
-			test_bit(XFR_COMPLETE, &xfr->flags) ||
-			test_bit(XFR_CANCELED, &xfr->flags));
+					      test_bit(XFR_COMPLETE,
+						       &xfr->flags) ||
+					      test_bit(XFR_CANCELED,
+						       &xfr->flags));
 
 		spin_lock_irq(&client->lock);

@@ -222,12 +224,14 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
 
 	bytes = min(len, xfr->resp_data_length - client->read_offset);
 	if (ubuf) {
-		if (copy_to_user(ubuf, &xfr->buf[client->read_offset], bytes)) {
+		if (copy_to_user(ubuf, &xfr->buf[client->read_offset],
+				 bytes)) {
 			rc = -EFAULT;
 			goto done;
 		}
-	} else
+	} else {
 		memcpy(kbuf, &xfr->buf[client->read_offset], bytes);
+	}
 
 	client->read_offset += bytes;

@@ -263,6 +267,7 @@ static ssize_t occ_write_common(struct occ_client *client,
 		return -EINVAL;
 
 	spin_lock_irq(&client->lock);
+
 	if (test_and_set_bit(CLIENT_XFR_PENDING, &client->flags)) {
 		rc = -EBUSY;
 		goto done;

@@ -270,7 +275,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) {

@@ -278,8 +282,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) {

@@ -430,7 +435,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

Andrew Jeffery

2017-10-05 23:12 UTC #48 lore raw

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

Quoted 39 lines · Eddie James

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

For better flow and to meet Linux standards.
Signed-off-by Edward A. James <[email protected]>
---
drivers/fsi/occ.c | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)
drivers/fsi/occ.c +0 -0
index 4e677c6..f27345c 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 +2 -1
@@ -180,8 +179,9 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
  		if (client->read_offset) {
  			rc = 0;
  			client->read_offset = 0;
-		} else
+		} else {
  			rc = -ENOMSG;
+		}
goto done;
}
drivers/fsi/occ.c +0 -0
@@ -197,8 +197,10 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
  		spin_unlock_irq(&client->lock);
rc = wait_event_interruptible(client->wait,

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

Quoted 6 lines

drivers/fsi/occ.c +4 -2

-			test_bit(XFR_COMPLETE, &xfr->flags) ||
-			test_bit(XFR_CANCELED, &xfr->flags));
+					      test_bit(XFR_COMPLETE,
+						       &xfr->flags) ||
+					      test_bit(XFR_CANCELED,
+						       &xfr->flags));

This looks worse. Maybe it's worth pulling the condition out into a helper function?

Quoted 56 lines

spin_lock_irq(&client->lock);
drivers/fsi/occ.c +0 -0
@@ -222,12 +224,14 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
bytes = min(len, xfr->resp_data_length - client->read_offset); if (ubuf) {
drivers/fsi/occ.c +2 -1
-		if (copy_to_user(ubuf, &xfr->buf[client->read_offset], bytes)) {
+		if (copy_to_user(ubuf, &xfr->buf[client->read_offset],
+				 bytes)) {
rc = -EFAULT;
goto done;
}
drivers/fsi/occ.c +1 -1
-	} else
+	} else {
memcpy(kbuf, &xfr->buf[client->read_offset], bytes);
drivers/fsi/occ.c +1 -0
+	}
client->read_offset += bytes;
drivers/fsi/occ.c +0 -0
@@ -263,6 +267,7 @@ static ssize_t occ_write_common(struct occ_client *client,
  		return -EINVAL;
spin_lock_irq(&client->lock);
+
if (test_and_set_bit(CLIENT_XFR_PENDING, &client->flags)) { rc = -EBUSY;
goto done;
drivers/fsi/occ.c +0 -0
@@ -270,7 +275,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
@@ -278,8 +282,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
@@ -430,7 +435,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;

Eddie James

author

2017-10-05 19:25 UTC #49 lore raw

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

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

---
 drivers/fsi/occ.c | 2 ++
 1 file changed, 2 insertions(+)

drivers/fsi/occ.c +2 -0

index f27345c..916c342 100644
@@ -788,6 +788,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

Andrew Jeffery

Reviewed-by on v3 23/31

2017-10-05 23:13 UTC #50 lore raw

Re: [PATCH linux dev-4.10 v3 23/31] drivers: fsi: occ: Destroy the ida list on exit

Quoted 5 lines · Eddie James

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

Need to clean up the index list structure.
Signed-off-by Edward A. James <[email protected]>

Reviewed-by Andrew Jeffery <[email protected]>

Quoted 17 lines

---
drivers/fsi/occ.c | 2 ++
 1 file changed, 2 insertions(+)
drivers/fsi/occ.c +0 -0
index f27345c..916c342 100644
@@ -788,6 +788,8 @@ static void occ_exit(void)
  	destroy_workqueue(occ_wq);
platform_driver_unregister(&occ_driver);
drivers/fsi/occ.c +2 -0
+
+	ida_destroy(&occ_ida);
}

module_init(occ_init);

Eddie James

author

2017-10-05 19:25 UTC #51 lore raw

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

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

---
 drivers/fsi/occ.c | 3 ---
 1 file changed, 3 deletions(-)

drivers/fsi/occ.c +0 -3

index 916c342..561b029 100644
@@ -703,9 +703,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

Andrew Jeffery

2017-10-05 23:14 UTC #52 lore raw

Re: [PATCH linux dev-4.10 v3 24/31] drivers: fsi: occ: Remove unnecessary platform_set_drvdata call

Quoted 18 lines · 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.
Signed-off-by Edward A. James <[email protected]>
---
drivers/fsi/occ.c | 3 ---
 1 file changed, 3 deletions(-)
drivers/fsi/occ.c +0 -0
index 916c342..561b029 100644
@@ -703,9 +703,6 @@ static int occ_probe(struct platform_device *pdev)
  	mutex_init(&occ->occ_lock);
  	INIT_WORK(&occ->work, occ_worker);

drivers/fsi/occ.c static int occ_probe(struct platform_device *pdev) · line 708

Quoted 3 lines

drivers/fsi/occ.c +0 -3

-	/* ensure NULL before we probe children, so they don't hang FSI */
-	platform_set_drvdata(pdev, NULL);
-

Bizarre that it was even there in the first place - do you know the history of the comment?

Quoted 3 lines

if (dev->of_node) {
rc = of_property_read_u32(dev->of_node, "reg", &reg); if (!rc) {

Eddie James

author

2017-10-06 00:34 UTC #53 lore raw

Re: [PATCH linux dev-4.10 v3 24/31] drivers: fsi: occ: Remove unnecessary platform_set_drvdata call

Quoted 19 lines · Andrew Jeffery

Quoted 18 lines · 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.
Signed-off-by Edward A. James <[email protected]>
---
  drivers/fsi/occ.c | 3 ---
  1 file changed, 3 deletions(-)
drivers/fsi/occ.c +0 -0
index 916c342..561b029 100644
@@ -703,9 +703,6 @@ static int occ_probe(struct platform_device *pdev)
  	mutex_init(&occ->occ_lock);
  	INIT_WORK(&occ->work, occ_worker);
 

Quoted 5 lines

drivers/fsi/occ.c static int occ_probe(struct platform_device *pdev) · line 708
Quoted 3 lines
drivers/fsi/occ.c +0 -3
-	/* ensure NULL before we probe children, so they don't hang FSI */
-	platform_set_drvdata(pdev, NULL);
-
Bizarre that it was even there in the first place - do you know the history of the comment?

I made the comment. I wasn't sure if there were situations where that wouldn't be NULL before probe() somehow. Regarding the hanging FSI... that was before some FSI core fixes were in that prevent that situation during probe().

Quoted 4 lines

drivers/fsi/occ.c +0 -0

  	if (dev->of_node) {
  		rc = of_property_read_u32(dev->of_node, "reg", &reg);
  		if (!rc) {

Andrew Jeffery

Reviewed-by

2017-10-06 00:50 UTC #54 lore raw

Re: [PATCH linux dev-4.10 v3 24/31] drivers: fsi: occ: Remove unnecessary platform_set_drvdata call

Quoted 25 lines · Eddie James

Quoted 23 lines · Andrew Jeffery
Quoted 21 lines · Eddie James
Quoted 1 line
Quoted 1 line
Quoted 1 line
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.
Quoted 1 line
Quoted 1 line
Quoted 1 line
Signed-off-by Edward A. James <[email protected]>
---
drivers/fsi/occ.c | 3 ---
  1 file changed, 3 deletions(-)
drivers/fsi/occ.c +0 -0
index 916c342..561b029 100644
@@ -703,9 +703,6 @@ static int occ_probe(struct platform_device *pdev)
 > > >   	mutex_init(&occ->occ_lock);
 > > >   	INIT_WORK(&occ->work, occ_worker);
Quoted 2 lines
Quoted 2 lines
drivers/fsi/occ.c static int occ_probe(struct platform_device *pdev) · line 707
Quoted 2 lines
drivers/fsi/occ.c +0 -2
-	/* ensure NULL before we probe children, so they don't hang FSI */
-	platform_set_drvdata(pdev, NULL);
-

Quoted 7 lines

Quoted 2 lines
Bizarre that it was even there in the first place - do you know the history of the comment?
I made the comment. I wasn't sure if there were situations where that wouldn't be NULL before probe() somehow. Regarding the hanging FSI... that was before some FSI core fixes were in that prevent that situation during probe().

Fair enough.

Reviewed-by Andrew Jeffery <[email protected]>

Quoted 7 lines

Quoted 5 lines
Quoted 5 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
   if (dev->of_node) {
   rc = of_property_read_u32(dev->of_node, "reg", &reg);
   if (!rc) {

Eddie James

author

2017-10-05 19:25 UTC #55 lore raw

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

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

---
 drivers/fsi/occ.c | 55 +++++++++++++++++++++++++++++++++++++------------------
 1 file changed, 37 insertions(+), 18 deletions(-)

drivers/fsi/occ.c +37 ~~-18~~

index 561b029..29a17ed 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;

@@ -273,10 +273,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;

@@ -354,7 +359,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;

@@ -419,9 +424,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);

@@ -470,7 +479,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;

@@ -478,6 +487,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);

@@ -518,11 +531,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);

@@ -573,6 +590,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)

@@ -582,6 +600,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

Andrew Jeffery

Reviewed-by on v3 25/31

2017-10-05 23:17 UTC #56 lore raw

Re: [PATCH linux dev-4.10 v3 25/31] drivers: fsi: occ: Add comments for clarity

Quoted 5 lines · Eddie James

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

Fixup some existing comments as well.
Signed-off-by Edward A. James <[email protected]>

Reviewed-by Andrew Jeffery <[email protected]>

Quoted 2 lines

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

------------

drivers/fsi/occ.c struct occ { · line 42

Quoted 13 lines

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

drivers/fsi/occ.c +1 -2

index 561b029..29a17ed 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 */

drivers/fsi/occ.c struct occ { · line 43

Quoted 1 line

drivers/fsi/occ.c +1 -0

+	struct mutex occ_lock;		/* lock access to the

hardware */

Quoted 10 lines

drivers/fsi/occ.c +0 -0
  	struct work_struct work;
  };
drivers/fsi/occ.c +0 -0
@@ -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

Quoted 1 line

- * flags are cleared when the transfer is completed by

closing the file or

Quoted 2 lines

- * reading all of the available response data.
+ * flags are cleared when the transfer is completed by closing the

file or

Quoted 2 lines

+ * reading all of the available response data.
* XFR_IN_PROGRESS is set when a transfer is started from

occ_worker_putsram,

Quoted 1 line

- * and cleared if the transfer fails or occ_worker_getsram

completes.

Quoted 1 line

+ * and cleared if the transfer fails or occ_worker_getsram

completes.

Quoted 1 line

* XFR_COMPLETE is set when a transfer fails or finishes

occ_worker_getsram.

Quoted 3 lines

* 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

Quoted 2 lines

- * or fails.
+ * O_NONBLOCK wasn't specified. Cleared in read() when transfer

completes or

drivers/fsi/occ.c struct occ_xfr { · line 91

Quoted 8 lines

+ * fails.
*/
enum {
XFR_IN_PROGRESS,

drivers/fsi/occ.c +0 -0

@@ -89,9 +89,9 @@ struct occ_xfr {
   * client flags
   *
   * CLIENT_NONBLOCKING is set during open() if the file was opened

with the

drivers/fsi/occ.c struct occ_xfr {

Quoted 3 lines

drivers/fsi/occ.c +1 -1

- * 	O_NONBLOCKING flag.
+ *  O_NONBLOCK flag.
   * CLIENT_XFR_PENDING is set during write() and cleared when all

data has been

drivers/fsi/occ.c enum { · line 104

Quoted 11 lines

drivers/fsi/occ.c +2 -2

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

drivers/fsi/occ.c static ssize_t occ_write_common(struct

Quoted 4 lines

drivers/fsi/occ.c +0 -0

  	wait_queue_head_t wait;
  	size_t read_offset;
  	unsigned long flags;

@@ -273,10 +273,15 @@ static ssize_t occ_write_common(struct

occ_client *client,

drivers/fsi/occ.c static ssize_t occ_write_common(struct occ_client *client,

Quoted 7 lines

drivers/fsi/occ.c +0 -0

  		goto done;
  	}

drivers/fsi/occ.c +1 -3

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

transfer */

drivers/fsi/occ.c static int occ_release_common(struct occ_client

Quoted 12 lines

drivers/fsi/occ.c +1 -0

+	xfr->buf[0] = 1;		/* occ sequence number */

drivers/fsi/occ.c +6 -0

+	/*
+	 * Assume user data follows the occ command format.
+	 * byte 0: command type
+	 * bytes 1-2: data length (msb first)
+	 * bytes 3-n: data
+	 */

if (ubuf) {
if (copy_from_user(&xfr->buf[1], ubuf, len)) { rc = -EFAULT;

drivers/fsi/occ.c +0 -0

@@ -354,7 +359,7 @@ static int occ_release_common(struct occ_client

*client)

drivers/fsi/occ.c static int occ_getsram(struct device *sbefifo,

Quoted 9 lines

drivers/fsi/occ.c +0 -0
  		return 0;
  	}
drivers/fsi/occ.c +1 -1
-	/* operation is in progress; let worker clean up*/
+	/* operation is in progress; let worker clean up */
spin_unlock_irq(&occ->list_lock);
spin_unlock_irq(&client->lock);
return 0;
drivers/fsi/occ.c +0 -0
@@ -419,9 +424,13 @@ static int occ_getsram(struct device *sbefifo,

u32 address, u8 *data,

drivers/fsi/occ.c static int occ_getsram(struct device *sbefifo, u32 address, u8 *data,

Quoted 5 lines

drivers/fsi/occ.c +1 -1

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

drivers/fsi/occ.c

Quoted 4 lines

drivers/fsi/occ.c +0 -0

  	struct sbefifo_client *client;

drivers/fsi/occ.c +2 -0

+	/*
+	 * Magic sequence to do SBE getsram command. SBE will fetch

data from

drivers/fsi/occ.c static int occ_putsram(struct device *sbefifo,

Quoted 6 lines

drivers/fsi/occ.c +2 -0
+	 * specified SRAM address.
+	 */
buf[0] = cpu_to_be32(0x5);
buf[1] = cpu_to_be32(0xa403);
buf[2] = cpu_to_be32(1);
drivers/fsi/occ.c +0 -0
@@ -470,7 +479,7 @@ static int occ_putsram(struct device *sbefifo,

u32 address, u8 *data,

drivers/fsi/occ.c static int occ_putsram(struct device *sbefifo, u32 address, u8 *data,

Quoted 5 lines

drivers/fsi/occ.c +1 -1

  {
  	int rc;
  	__be32 *buf;
-	u32 data_len = ((len + 7) / 8) * 8;
+	u32 data_len = ((len + 7) / 8) * 8;	/* must be

multiples of 8 B */

drivers/fsi/occ.c static int occ_putsram(struct device *sbefifo,

Quoted 4 lines

drivers/fsi/occ.c +0 -0

  	size_t cmd_len = data_len + 20;
  	struct sbefifo_client *client;

drivers/fsi/occ.c +0 -0

@@ -478,6 +487,10 @@ static int occ_putsram(struct device *sbefifo,

u32 address, u8 *data,

drivers/fsi/occ.c

Quoted 5 lines

drivers/fsi/occ.c +0 -0

  	if (!buf)
  		return -ENOMEM;

drivers/fsi/occ.c +2 -0

+	/*
+	 * Magic sequence to do SBE putsram command. SBE will

transfer

drivers/fsi/occ.c static int occ_trigger_attn(struct device

Quoted 6 lines

drivers/fsi/occ.c +2 -0
+	 * data to specified SRAM address.
+	 */
buf[0] = cpu_to_be32(0x5 + (data_len / 4)); buf[1] = cpu_to_be32(0xa404); buf[2] = cpu_to_be32(1);
drivers/fsi/occ.c +0 -0
@@ -518,11 +531,15 @@ static int occ_trigger_attn(struct device

*sbefifo)

drivers/fsi/occ.c

Quoted 5 lines

drivers/fsi/occ.c +0 -0

  	__be32 buf[6];
  	struct sbefifo_client *client;

drivers/fsi/occ.c +2 -0

+	/*
+	 * Magic sequence to do SBE putscom command. SBE will write

8 bytes to

drivers/fsi/occ.c

Quoted 8 lines

drivers/fsi/occ.c +2 -0
+	 * specified SCOM address.
+	 */
buf[0] = cpu_to_be32(0x6);
buf[1] = cpu_to_be32(0xa202);
buf[2] = 0;
buf[3] = cpu_to_be32(0x6D035);
drivers/fsi/occ.c +1 -1
-	buf[4] = cpu_to_be32(0x20010000);
+	buf[4] = cpu_to_be32(0x20010000);	/* trigger occ

attention */

Quoted 19 lines

buf[5] = 0;

client = sbefifo_drv_open(sbefifo, 0);
drivers/fsi/occ.c +0 -0
@@ -573,6 +590,7 @@ static void occ_worker(struct work_struct *work)
  	spin_unlock_irq(&occ->list_lock);
  	mutex_lock(&occ->occ_lock);
drivers/fsi/occ.c +1 -0
+	/* write occ command */
rc = occ_putsram(sbefifo, 0xFFFBE000, xfr->buf, xfr->cmd_data_length); if (rc)
drivers/fsi/occ.c +0 -0
@@ -582,6 +600,7 @@ static void occ_worker(struct work_struct *work)
  	if (rc)
  		goto done;
drivers/fsi/occ.c +1 -0
+	/* read occ response */
rc = occ_getsram(sbefifo, 0xFFFBF000, xfr->buf, 8); if (rc)
goto done;

Eddie James

author

2017-10-05 19:25 UTC #57 lore raw

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

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

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

Andrew Jeffery

Acked-by on v3 26/31

2017-10-05 23:20 UTC #58 lore raw

Re: [PATCH linux dev-4.10 v3 26/31] drivers: fsi: occ: Add OCC response definitions to header

Quoted 5 lines · Eddie James

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

Also fix include guards.
Signed-off-by Edward A. James <[email protected]>

Acked-by Andrew Jeffery <[email protected]>

Quoted 43 lines

---
include/linux/occ.h | 20 +++++++++++++++++---

 1 file changed, 17 insertions(+), 3 deletions(-)

include/linux/occ.h +0 -0

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

include/linux/occ.h +2 -2

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

struct device;
struct occ_client;

include/linux/occ.h +14 -0

+#define OCC_RESP_CMD_IN_PRG		0xFF
+#define OCC_RESP_SUCCESS		0
+#define OCC_RESP_CMD_INVAL		0x11
+#define OCC_RESP_CMD_LEN_INVAL		0x12
+#define OCC_RESP_DATA_INVAL		0x13
+#define OCC_RESP_CHKSUM_ERR		0x14
+#define OCC_RESP_INT_ERR		0x15
+#define OCC_RESP_BAD_STATE		0x16
+#define OCC_RESP_CRIT_EXCEPT		0xE0
+#define OCC_RESP_CRIT_INIT		0xE1
+#define OCC_RESP_CRIT_WATCHDOG		0xE2
+#define OCC_RESP_CRIT_OCB		0xE3
+#define OCC_RESP_CRIT_HW		0xE4
+

extern struct occ_client *occ_drv_open(struct device *dev, unsigned long flags); extern int occ_drv_read(struct occ_client *client, char *buf, size_t len);

include/linux/occ.h +0 -0

@@ -24,4 +38,4 @@ extern int occ_drv_write(struct occ_client *client, const char *buf,
  			 size_t len);
  extern void occ_drv_release(struct occ_client *client);

include/linux/occ.h +1 -1

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

Eddie James

author

2017-10-05 19:25 UTC #59 lore raw

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

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

---
 drivers/fsi/occ.c | 27 ++++++++++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)

drivers/fsi/occ.c +24 -3

index 29a17ed..8555e99 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];

@@ -569,6 +573,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;

@@ -590,6 +597,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);

@@ -601,9 +610,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

Andrew Jeffery

Acked-by on v3 27/31

2017-10-05 23:27 UTC #60 lore raw

Re: [PATCH linux dev-4.10 v3 27/31] drivers: fsi: occ: Poll while receiving "command in progress"

Quoted 28 lines · 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.
Signed-off-by Edward A. James <[email protected]>
---
drivers/fsi/occ.c | 27 ++++++++++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)
drivers/fsi/occ.c +1 -0
index 29a17ed..8555e99 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

drivers/fsi/occ.c line 38

Quoted 2 lines

drivers/fsi/occ.c +2 -0

+#define OCC_TIMEOUT_MS		1000
+#define OCC_CMD_IN_PRG_WAIT_MS	50

Where doe 50ms come from? Is it a bum-plucked number? Or should I dig out my OCC documentation?

The rest looks okay though, so:

Acked-by Andrew Jeffery <[email protected]>

Quoted 48 lines

+
struct occ {
struct device *sbefifo;
char name[32];

drivers/fsi/occ.c +3 -0

@@ -569,6 +573,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;

@@ -590,6 +597,8 @@ static void occ_worker(struct work_struct *work)
  	spin_unlock_irq(&occ->list_lock);
  	mutex_lock(&occ->occ_lock);

drivers/fsi/occ.c +2 -0

+	start = jiffies;
+

/* write occ command */
rc = occ_putsram(sbefifo, 0xFFFBE000, xfr->buf, xfr->cmd_data_length);

drivers/fsi/occ.c +0 -0

@@ -601,9 +610,21 @@ static void occ_worker(struct work_struct *work)
  		goto done;

/* read occ response */

drivers/fsi/occ.c +15 -3

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

Eddie James

author

2017-10-06 01:04 UTC #61 lore raw

Re: [PATCH linux dev-4.10 v3 27/31] drivers: fsi: occ: Poll while receiving "command in progress"

Quoted 29 lines · Andrew Jeffery

Quoted 28 lines · 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.
Signed-off-by Edward A. James <[email protected]>
---
  drivers/fsi/occ.c | 27 ++++++++++++++++++++++++---
  1 file changed, 24 insertions(+), 3 deletions(-)
drivers/fsi/occ.c +1 -0
index 29a17ed..8555e99 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
 

Quoted 6 lines

drivers/fsi/occ.c line 38
Quoted 2 lines
drivers/fsi/occ.c +2 -0
+#define OCC_TIMEOUT_MS		1000
+#define OCC_CMD_IN_PRG_WAIT_MS	50
Where doe 50ms come from? Is it a bum-plucked number? Or should I dig out my OCC documentation?
The rest looks okay though, so:

I just fabricated it. OCC spec doesn't provide any info on what to do here. Just a way to do polling on the "command in progress" response.

Thanks,
Eddie

Quoted 51 lines

Acked-by Andrew Jeffery <[email protected]>

Quoted 48 lines

drivers/fsi/occ.c +21 -3

+
  struct occ {
  	struct device *sbefifo;
  	char name[32];

@@ -569,6 +573,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;

@@ -590,6 +597,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);

@@ -601,9 +610,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) {

Eddie James

author

2017-10-05 19:25 UTC #62 lore raw

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

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

---
 drivers/fsi/occ.c | 86 +++++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 62 insertions(+), 24 deletions(-)

drivers/fsi/occ.c +62 ~~-24~~

index 8555e99..1770823 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,14 +136,20 @@ static void occ_enqueue_xfr(struct occ_xfr *xfr)
 
 	if (empty)
 		queue_work(occ_wq, &occ->work);
+
+	return 0;
 }
 
 static struct occ_client *occ_open_common(struct occ *occ, unsigned long flags)
 {
-	struct occ_client *client = kzalloc(sizeof(*client), GFP_KERNEL);
+	struct occ_client *client;
+
+	if (occ->cancel)
+		return ERR_PTR(-ECANCELED);
 
+	client = kzalloc(sizeof(*client), GFP_KERNEL);
 	if (!client)
-		return NULL;
+		return ERR_PTR(-ENOMEM);
 
 	client->occ = occ;
 	spin_lock_init(&client->lock);

@@ -158,8 +168,8 @@ static int occ_open(struct inode *inode, struct file *file)
 	struct occ *occ = to_occ(mdev);
 
 	client = occ_open_common(occ, file->f_flags);
-	if (!client)
-		return -ENOMEM;
+	if (IS_ERR(client))
+		return PTR_ERR(client);
 
 	file->private_data = client;

@@ -172,6 +182,7 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
 	int rc;
 	size_t bytes;
 	struct occ_xfr *xfr = &client->xfr;
+	struct occ *occ = client->occ;
 
 	if (len > OCC_SRAM_BYTES)
 		return -EINVAL;

@@ -204,7 +215,8 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
 					      test_bit(XFR_COMPLETE,
 						       &xfr->flags) ||
 					      test_bit(XFR_CANCELED,
-						       &xfr->flags));
+						       &xfr->flags) ||
+					      occ->cancel);
 
 		spin_lock_irq(&client->lock);

@@ -272,7 +284,7 @@ static ssize_t occ_write_common(struct occ_client *client,
 
 	spin_lock_irq(&client->lock);
 
-	if (test_and_set_bit(CLIENT_XFR_PENDING, &client->flags)) {
+	if (test_bit(CLIENT_XFR_PENDING, &client->flags)) {
 		rc = -EBUSY;
 		goto done;
 	}

@@ -310,8 +322,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:

@@ -583,6 +598,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);

@@ -676,12 +694,17 @@ static void occ_worker(struct work_struct *work)
 
 struct occ_client *occ_drv_open(struct device *dev, unsigned long flags)
 {
+	struct occ_client *client;
 	struct occ *occ = dev_get_drvdata(dev);
 
 	if (!occ)
 		return NULL;
 
-	return occ_open_common(occ, flags);
+	client = occ_open_common(occ, flags);
+	if (IS_ERR(client))
+		return NULL;
+
+	return client;
 }
 EXPORT_SYMBOL_GPL(occ_drv_open);

@@ -752,23 +775,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;

@@ -778,20 +791,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

Andrew Jeffery

2017-10-05 23:36 UTC #63 lore raw

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

Quoted 51 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.
Signed-off-by Edward A. James <[email protected]>
---
drivers/fsi/occ.c | 86 +++++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 62 insertions(+), 24 deletions(-)
drivers/fsi/occ.c +0 -0
index 8555e99..1770823 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,14 +136,20 @@ 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 static void occ_enqueue_xfr(struct occ_xfr *xfr) · line 148

Quoted 5 lines

drivers/fsi/occ.c +4 -1

-	struct occ_client *client = kzalloc(sizeof(*client), GFP_KERNEL);
+	struct occ_client *client;
+
+	if (occ->cancel)
+		return ERR_PTR(-ECANCELED);

I don't think this test is helpful, in that it's racy. What if remove() gets called immediately after the thread calling into open() has passed the test?

Quoted 163 lines

drivers/fsi/occ.c +1 -0

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

if (!client)

drivers/fsi/occ.c +1 -1

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

client->occ = occ;
spin_lock_init(&client->lock);

drivers/fsi/occ.c +0 -0

@@ -158,8 +168,8 @@ static int occ_open(struct inode *inode, struct file *file)
  	struct occ *occ = to_occ(mdev);

client = occ_open_common(occ, file->f_flags);

drivers/fsi/occ.c +2 -2

-	if (!client)
-		return -ENOMEM;
+	if (IS_ERR(client))
+		return PTR_ERR(client);

file->private_data = client;

drivers/fsi/occ.c +1 -0

@@ -172,6 +182,7 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
  	int rc;
  	size_t bytes;
  	struct occ_xfr *xfr = &client->xfr;
+	struct occ *occ = client->occ;

if (len > OCC_SRAM_BYTES)
return -EINVAL;

drivers/fsi/occ.c +0 -0

@@ -204,7 +215,8 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
  					      test_bit(XFR_COMPLETE,

&xfr->flags) || test_bit(XFR_CANCELED,

drivers/fsi/occ.c +2 -1

-						       &xfr->flags));
+						       &xfr->flags) ||
+					      occ->cancel);

spin_lock_irq(&client->lock);

drivers/fsi/occ.c +0 -0

@@ -272,7 +284,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

@@ -310,8 +322,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

@@ -583,6 +598,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 +0 -0

@@ -676,12 +694,17 @@ static void occ_worker(struct work_struct *work)

struct occ_client *occ_drv_open(struct device *dev, unsigned long flags) {

drivers/fsi/occ.c +1 -0

+	struct occ_client *client;

struct occ *occ = dev_get_drvdata(dev);

if (!occ)
return NULL;

drivers/fsi/occ.c +5 -1

-	return occ_open_common(occ, flags);
+	client = occ_open_common(occ, flags);
+	if (IS_ERR(client))
+		return NULL;
+
+	return client;

}
EXPORT_SYMBOL_GPL(occ_drv_open);

drivers/fsi/occ.c +3 ~~-13~~

@@ -752,23 +775,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

@@ -778,20 +791,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;

Eddie James

author

2017-10-06 01:21 UTC #64 lore raw

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

Quoted 50 lines · Andrew Jeffery

Quoted 49 lines · Eddie James

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

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

---
  drivers/fsi/occ.c | 86 +++++++++++++++++++++++++++++++++++++++----------------
  1 file changed, 62 insertions(+), 24 deletions(-)

drivers/fsi/occ.c +7 -1

index 8555e99..1770823 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,14 +136,20 @@ static void occ_enqueue_xfr(struct occ_xfr *xfr)
 
  	if (empty)
  		queue_work(occ_wq, &occ->work);
+
+	return 0;
  }

Quoted 9 lines

drivers/fsi/occ.c static void occ_enqueue_xfr(struct occ_xfr *xfr) · line 148
Quoted 7 lines
drivers/fsi/occ.c +4 -1
  static struct occ_client *occ_open_common(struct occ *occ, unsigned long flags)
  {
-	struct occ_client *client = kzalloc(sizeof(*client), GFP_KERNEL);
+	struct occ_client *client;
+
+	if (occ->cancel)
+		return ERR_PTR(-ECANCELED);
I don't think this test is helpful, in that it's racy. What if remove() gets called immediately after the thread calling into open() has passed the test?

Yep true. This was helpful before I fixed the memory management of the client, though obviously still racy. No need for it now.

Thnaks,
Eddie

Quoted 164 lines

drivers/fsi/occ.c +51 ~~-22~~

+	client = kzalloc(sizeof(*client), GFP_KERNEL);
  	if (!client)
-		return NULL;
+		return ERR_PTR(-ENOMEM);
 
  	client->occ = occ;
  	spin_lock_init(&client->lock);

@@ -158,8 +168,8 @@ static int occ_open(struct inode *inode, struct file *file)
  	struct occ *occ = to_occ(mdev);
 
  	client = occ_open_common(occ, file->f_flags);
-	if (!client)
-		return -ENOMEM;
+	if (IS_ERR(client))
+		return PTR_ERR(client);
 
  	file->private_data = client;

@@ -172,6 +182,7 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
  	int rc;
  	size_t bytes;
  	struct occ_xfr *xfr = &client->xfr;
+	struct occ *occ = client->occ;
 
  	if (len > OCC_SRAM_BYTES)
  		return -EINVAL;

@@ -204,7 +215,8 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
  					      test_bit(XFR_COMPLETE,
  						       &xfr->flags) ||
  					      test_bit(XFR_CANCELED,
-						       &xfr->flags));
+						       &xfr->flags) ||
+					      occ->cancel);
 
  		spin_lock_irq(&client->lock);

@@ -272,7 +284,7 @@ static ssize_t occ_write_common(struct occ_client *client,
 
  	spin_lock_irq(&client->lock);
 
-	if (test_and_set_bit(CLIENT_XFR_PENDING, &client->flags)) {
+	if (test_bit(CLIENT_XFR_PENDING, &client->flags)) {
  		rc = -EBUSY;
  		goto done;
  	}

@@ -310,8 +322,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:

@@ -583,6 +598,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);

@@ -676,12 +694,17 @@ static void occ_worker(struct work_struct *work)
 
  struct occ_client *occ_drv_open(struct device *dev, unsigned long flags)
  {
+	struct occ_client *client;
  	struct occ *occ = dev_get_drvdata(dev);
 
  	if (!occ)
  		return NULL;
 
-	return occ_open_common(occ, flags);
+	client = occ_open_common(occ, flags);
+	if (IS_ERR(client))
+		return NULL;
+
+	return client;
  }
  EXPORT_SYMBOL_GPL(occ_drv_open);

@@ -752,23 +775,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;

@@ -778,20 +791,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;

Eddie James

author

2017-10-05 19:25 UTC #65 lore raw

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

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

---
 drivers/fsi/occ.c | 92 ++++++++++++++++++++++++++-----------------------------
 1 file changed, 43 insertions(+), 49 deletions(-)

drivers/fsi/occ.c +43 ~~-49~~

index 1770823..f2b5b95 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;

@@ -152,6 +167,7 @@ static struct occ_client *occ_open_common(struct occ *occ, unsigned long flags)
 		return ERR_PTR(-ENOMEM);
 
 	client->occ = occ;
+	kref_init(&client->kref);
 	spin_lock_init(&client->lock);
 	init_waitqueue_head(&client->wait);

@@ -187,6 +203,7 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
 	if (len > OCC_SRAM_BYTES)
 		return -EINVAL;
 
+	occ_get_client(client);
 	spin_lock_irq(&client->lock);
 
 	if (!test_bit(CLIENT_XFR_PENDING, &client->flags)) {

@@ -207,8 +224,6 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
 			goto done;
 		}
 
-		set_bit(XFR_WAITING, &xfr->flags);
-
 		spin_unlock_irq(&client->lock);
 
 		rc = wait_event_interruptible(client->wait,

@@ -220,15 +235,12 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
 
 		spin_lock_irq(&client->lock);
 
-		if (test_bit(XFR_CANCELED, &xfr->flags)) {
-			spin_unlock_irq(&client->lock);
-			kfree(client);
-			return -EBADFD;
-		}
-
-		clear_bit(XFR_WAITING, &xfr->flags);
 		if (!test_bit(XFR_COMPLETE, &xfr->flags)) {
-			rc = -EINTR;
+			if (occ->cancel || test_bit(XFR_CANCELED, &xfr->flags))
+				rc = -ECANCELED;
+			else
+				rc = -EINTR;
+
 			goto done;
 		}
 	}

@@ -259,6 +271,7 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
 
 done:
 	spin_unlock_irq(&client->lock);
+	occ_put_client(client);
 	return rc;
 }

@@ -282,6 +295,7 @@ static ssize_t occ_write_common(struct occ_client *client,
 	if (len > (OCC_CMD_DATA_BYTES + 3) || len < 3)
 		return -EINVAL;
 
+	occ_get_client(client);
 	spin_lock_irq(&client->lock);
 
 	if (test_bit(CLIENT_XFR_PENDING, &client->flags)) {

@@ -331,6 +345,7 @@ static ssize_t occ_write_common(struct occ_client *client,
 
 done:
 	spin_unlock_irq(&client->lock);
+	occ_put_client(client);
 	return rc;
 }

@@ -349,38 +364,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;
 }

@@ -586,7 +589,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);

@@ -609,6 +612,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);

@@ -664,29 +669,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

Eddie James

author

2017-10-05 19:25 UTC #66 lore raw

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

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

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

Andrew Jeffery

Reviewed-by on v3 30/31

2017-10-05 23:38 UTC #67 lore raw

Re: [PATCH linux dev-4.10 v3 30/31] drivers/hwmon/occ: Remove repeated ops for OCC command in progress

Quoted 5 lines · Eddie James

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

This is now handled in the occ driver.
Signed-off-by Edward A. James <[email protected]>

Reviewed-by Andrew Jeffery <[email protected]>

Quoted 40 lines

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

 1 file changed, 1 insertion(+), 13 deletions(-)

drivers/hwmon/occ/p9_sbe.c +0 -1

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

drivers/hwmon/occ/p9_sbe.c +0 -3

-	start = jiffies;
-
-retry:

client = occ_drv_open(p9_sbe_occ->sbe, 0); if (!client) {
rc = -ENODEV;

drivers/hwmon/occ/p9_sbe.c +0 -0

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

drivers/hwmon/occ/p9_sbe.c +1 -9

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

Eddie James

author

2017-10-05 19:25 UTC #68 lore raw

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

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

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