diff mbox series

[linux,dev-4.10,1/3] drivers/fsi/sbefifo: refactor to upstream list state

Message ID 1506033838-2078-2-git-send-email-eajames@linux.vnet.ibm.com
State Superseded, archived
Headers show
Series drivers/fsi: Fixup client remove functions | expand

Commit Message

Eddie James Sept. 21, 2017, 10:43 p.m. UTC 
From: "Edward A. James" <eajames@us.ibm.com>

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

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

Comments

Eddie James Sept. 21, 2017, 10:55 p.m. UTC | #1 
On 09/21/2017 05:43 PM, Eddie James wrote:
> From: "Edward A. James" <eajames@us.ibm.com>
>
> Includes various fixes:
>   - check for complete while waiting for data in read()
>   - fix probe if probe fails
>   - general cleanup
>   - slightly safer (earlier) get_client()
>   - reorder some of the remove() operations for safety

To clarify, this hasn't actually been sent upstream yet. But no changes 
planned to send this upstream.

Thanks,
Eddie

>
> Signed-off-by: Edward A. James <eajames@us.ibm.com>
> ---
>   drivers/fsi/fsi-sbefifo.c   | 329 ++++++++++++++++++++++++--------------------
>   include/linux/fsi-sbefifo.h |   6 +-
>   2 files changed, 180 insertions(+), 155 deletions(-)
>
> diff --git a/drivers/fsi/fsi-sbefifo.c b/drivers/fsi/fsi-sbefifo.c
> index 1c37ff7..5d25ade 100644
> --- a/drivers/fsi/fsi-sbefifo.c
> +++ b/drivers/fsi/fsi-sbefifo.c
> @@ -11,20 +11,27 @@
>    * GNU General Public License for more details.
>    */
>
> -#include <linux/delay.h>
> +#include <linux/device.h>
>   #include <linux/errno.h>
> -#include <linux/idr.h>
> +#include <linux/fs.h>
>   #include <linux/fsi.h>
> +#include <linux/fsi-sbefifo.h>
> +#include <linux/idr.h>
> +#include <linux/kernel.h>
> +#include <linux/kref.h>
>   #include <linux/list.h>
>   #include <linux/miscdevice.h>
>   #include <linux/module.h>
>   #include <linux/of.h>
> +#include <linux/of_device.h>
>   #include <linux/of_platform.h>
>   #include <linux/poll.h>
>   #include <linux/sched.h>
>   #include <linux/slab.h>
> +#include <linux/spinlock.h>
>   #include <linux/timer.h>
>   #include <linux/uaccess.h>
> +#include <linux/wait.h>
>
>   /*
>    * The SBEFIFO is a pipe-like FSI device for communicating with
> @@ -44,14 +51,15 @@
>   #define   SBEFIFO_EOT_MAGIC		0xffffffff
>   #define SBEFIFO_EOT_ACK		0x14
>
> +#define SBEFIFO_RESCHEDULE	msecs_to_jiffies(500)
> +
>   struct sbefifo {
>   	struct timer_list poll_timer;
> -	struct fsi_device *fsi_dev;
> -	struct miscdevice mdev;
> +	struct fsi_device *fsi_dev;	/* parent fsi device */
> +	struct miscdevice mdev;		/* /dev/ entry */
>   	wait_queue_head_t wait;
> -	struct list_head link;
>   	struct list_head xfrs;
> -	struct kref kref;
> +	struct kref kref;		/* reference counter */
>   	spinlock_t lock;
>   	char name[32];
>   	int idx;
> @@ -87,14 +95,12 @@ struct sbefifo_client {
>   	unsigned long f_flags;
>   };
>
> -static struct list_head sbefifo_fifos;
> -
>   static DEFINE_IDA(sbefifo_ida);
>
>   static int sbefifo_inw(struct sbefifo *sbefifo, int reg, u32 *word)
>   {
>   	int rc;
> -	u32 raw_word;
> +	__be32 raw_word;
>
>   	rc = fsi_device_read(sbefifo->fsi_dev, reg, &raw_word,
>   			     sizeof(raw_word));
> @@ -102,17 +108,19 @@ static int sbefifo_inw(struct sbefifo *sbefifo, int reg, u32 *word)
>   		return rc;
>
>   	*word = be32_to_cpu(raw_word);
> +
>   	return 0;
>   }
>
>   static int sbefifo_outw(struct sbefifo *sbefifo, int reg, u32 word)
>   {
> -	u32 raw_word = cpu_to_be32(word);
> +	__be32 raw_word = cpu_to_be32(word);
>
>   	return fsi_device_write(sbefifo->fsi_dev, reg, &raw_word,
>   				sizeof(raw_word));
>   }
>
> +/* Don't flip endianness of data to/from FIFO, just pass through. */
>   static int sbefifo_readw(struct sbefifo *sbefifo, u32 *word)
>   {
>   	return fsi_device_read(sbefifo->fsi_dev, SBEFIFO_DWN, word,
> @@ -136,7 +144,7 @@ static int sbefifo_ack_eot(struct sbefifo *sbefifo)
>   		return ret;
>
>   	return sbefifo_outw(sbefifo, SBEFIFO_DWN | SBEFIFO_EOT_ACK,
> -			SBEFIFO_EOT_MAGIC);
> +			    SBEFIFO_EOT_MAGIC);
>   }
>
>   static size_t sbefifo_dev_nwreadable(u32 sts)
> @@ -210,6 +218,7 @@ static bool sbefifo_buf_readnb(struct sbefifo_buf *buf, size_t n)
>   		rpos = buf->buf;
>
>   	WRITE_ONCE(buf->rpos, rpos);
> +
>   	return rpos == wpos;
>   }
>
> @@ -229,14 +238,14 @@ static bool sbefifo_buf_wrotenb(struct sbefifo_buf *buf, size_t n)
>   		set_bit(SBEFIFO_BUF_FULL, &buf->flags);
>
>   	WRITE_ONCE(buf->wpos, wpos);
> +
>   	return rpos == wpos;
>   }
>
>   static void sbefifo_free(struct kref *kref)
>   {
> -	struct sbefifo *sbefifo;
> +	struct sbefifo *sbefifo = container_of(kref, struct sbefifo, kref);
>
> -	sbefifo = container_of(kref, struct sbefifo, kref);
>   	kfree(sbefifo);
>   }
>
> @@ -255,9 +264,12 @@ static struct sbefifo_xfr *sbefifo_enq_xfr(struct sbefifo_client *client)
>   	struct sbefifo *sbefifo = client->dev;
>   	struct sbefifo_xfr *xfr;
>
> +	if (READ_ONCE(sbefifo->rc))
> +		return ERR_PTR(sbefifo->rc);
> +
>   	xfr = kzalloc(sizeof(*xfr), GFP_KERNEL);
>   	if (!xfr)
> -		return NULL;
> +		return ERR_PTR(-ENOMEM);
>
>   	xfr->rbuf = &client->rbuf;
>   	xfr->wbuf = &client->wbuf;
> @@ -267,21 +279,12 @@ static struct sbefifo_xfr *sbefifo_enq_xfr(struct sbefifo_client *client)
>   	return xfr;
>   }
>
> -static struct sbefifo_xfr *sbefifo_client_next_xfr(
> -		struct sbefifo_client *client)
> -{
> -	if (list_empty(&client->xfrs))
> -		return NULL;
> -
> -	return container_of(client->xfrs.next, struct sbefifo_xfr,
> -			client);
> -}
> -
>   static bool sbefifo_xfr_rsp_pending(struct sbefifo_client *client)
>   {
> -	struct sbefifo_xfr *xfr;
> +	struct sbefifo_xfr *xfr = list_first_entry_or_null(&client->xfrs,
> +							   struct sbefifo_xfr,
> +							   client);
>
> -	xfr = sbefifo_client_next_xfr(client);
>   	if (xfr && test_bit(SBEFIFO_XFR_RESP_PENDING, &xfr->flags))
>   		return true;
>
> @@ -309,10 +312,11 @@ static struct sbefifo_client *sbefifo_new_client(struct sbefifo *sbefifo)
>
>   static void sbefifo_client_release(struct kref *kref)
>   {
> -	struct sbefifo_client *client;
>   	struct sbefifo_xfr *xfr;
> +	struct sbefifo_client *client = container_of(kref,
> +						     struct sbefifo_client,
> +						     kref);
>
> -	client = container_of(kref, struct sbefifo_client, kref);
>   	list_for_each_entry(xfr, &client->xfrs, client) {
>   		/*
>   		 * The client left with pending or running xfrs.
> @@ -349,6 +353,7 @@ static struct sbefifo_xfr *sbefifo_next_xfr(struct sbefifo *sbefifo)
>   			kfree(xfr);
>   			continue;
>   		}
> +
>   		return xfr;
>   	}
>
> @@ -370,7 +375,7 @@ static void sbefifo_poll_timer(unsigned long data)
>
>   	spin_lock(&sbefifo->lock);
>   	xfr = list_first_entry_or_null(&sbefifo->xfrs, struct sbefifo_xfr,
> -			xfrs);
> +				       xfrs);
>   	if (!xfr)
>   		goto out_unlock;
>
> @@ -388,8 +393,7 @@ static void sbefifo_poll_timer(unsigned long data)
>
>   	 /* Drain the write buffer. */
>   	while ((bufn = sbefifo_buf_nbreadable(wbuf))) {
> -		ret = sbefifo_inw(sbefifo, SBEFIFO_UP | SBEFIFO_STS,
> -				&sts);
> +		ret = sbefifo_inw(sbefifo, SBEFIFO_UP | SBEFIFO_STS, &sts);
>   		if (ret)
>   			goto out;
>
> @@ -397,7 +401,7 @@ static void sbefifo_poll_timer(unsigned long data)
>   		if (devn == 0) {
>   			/* No open slot for write.  Reschedule. */
>   			sbefifo->poll_timer.expires = jiffies +
> -				msecs_to_jiffies(500);
> +				SBEFIFO_RESCHEDULE;
>   			add_timer(&sbefifo->poll_timer);
>   			goto out_unlock;
>   		}
> @@ -414,9 +418,8 @@ static void sbefifo_poll_timer(unsigned long data)
>
>   	 /* Send EOT if the writer is finished. */
>   	if (test_and_clear_bit(SBEFIFO_XFR_WRITE_DONE, &xfr->flags)) {
> -		ret = sbefifo_outw(sbefifo,
> -				SBEFIFO_UP | SBEFIFO_EOT_RAISE,
> -				SBEFIFO_EOT_MAGIC);
> +		ret = sbefifo_outw(sbefifo, SBEFIFO_UP | SBEFIFO_EOT_RAISE,
> +				   SBEFIFO_EOT_MAGIC);
>   		if (ret)
>   			goto out;
>
> @@ -438,7 +441,7 @@ static void sbefifo_poll_timer(unsigned long data)
>   		if (devn == 0) {
>   			/* No data yet.  Reschedule. */
>   			sbefifo->poll_timer.expires = jiffies +
> -				msecs_to_jiffies(500);
> +				SBEFIFO_RESCHEDULE;
>   			add_timer(&sbefifo->poll_timer);
>   			goto out_unlock;
>   		}
> @@ -466,7 +469,7 @@ static void sbefifo_poll_timer(unsigned long data)
>   			set_bit(SBEFIFO_XFR_COMPLETE, &xfr->flags);
>   			list_del(&xfr->xfrs);
>   			if (unlikely(test_bit(SBEFIFO_XFR_CANCEL,
> -							&xfr->flags)))
> +					      &xfr->flags)))
>   				kfree(xfr);
>   			break;
>   		}
> @@ -476,7 +479,7 @@ static void sbefifo_poll_timer(unsigned long data)
>   	if (unlikely(ret)) {
>   		sbefifo->rc = ret;
>   		dev_err(&sbefifo->fsi_dev->dev,
> -				"Fatal bus access failure: %d\n", ret);
> +			"Fatal bus access failure: %d\n", ret);
>   		list_for_each_entry(xfr, &sbefifo->xfrs, xfrs)
>   			kfree(xfr);
>   		INIT_LIST_HEAD(&sbefifo->xfrs);
> @@ -497,7 +500,7 @@ static void sbefifo_poll_timer(unsigned long data)
>   static int sbefifo_open(struct inode *inode, struct file *file)
>   {
>   	struct sbefifo *sbefifo = container_of(file->private_data,
> -			struct sbefifo, mdev);
> +					       struct sbefifo, mdev);
>   	struct sbefifo_client *client;
>   	int ret;
>
> @@ -535,6 +538,19 @@ static unsigned int sbefifo_poll(struct file *file, poll_table *wait)
>   	return mask;
>   }
>
> +static bool sbefifo_read_ready(struct sbefifo *sbefifo,
> +			       struct sbefifo_client *client, size_t *n)
> +{
> +	struct sbefifo_xfr *xfr = list_first_entry_or_null(&client->xfrs,
> +							   struct sbefifo_xfr,
> +							   client);
> +
> +	*n = sbefifo_buf_nbreadable(&client->rbuf);
> +
> +	return READ_ONCE(sbefifo->rc) || *n ||
> +		(xfr && test_bit(SBEFIFO_XFR_COMPLETE, &xfr->flags));
> +}
> +
>   static ssize_t sbefifo_read_common(struct sbefifo_client *client,
>   				   char __user *ubuf, char *kbuf, size_t len)
>   {
> @@ -551,30 +567,38 @@ static ssize_t sbefifo_read_common(struct sbefifo_client *client,
>
>   	sbefifo_get_client(client);
>   	if (wait_event_interruptible(sbefifo->wait,
> -				(ret = READ_ONCE(sbefifo->rc)) ||
> -				(n = sbefifo_buf_nbreadable(
> -						&client->rbuf)))) {
> -		sbefifo_put_client(client);
> -		return -ERESTARTSYS;
> +				     sbefifo_read_ready(sbefifo, client,
> +							&n))) {
> +		ret = -ERESTARTSYS;
> +		goto out;
>   	}
> +
> +	ret = READ_ONCE(sbefifo->rc);
>   	if (ret) {
>   		INIT_LIST_HEAD(&client->xfrs);
> -		sbefifo_put_client(client);
> -		return ret;
> +		goto out;
>   	}
>
>   	n = min_t(size_t, n, len);
>
>   	if (ubuf) {
>   		if (copy_to_user(ubuf, READ_ONCE(client->rbuf.rpos), n)) {
> -			sbefifo_put_client(client);
> -			return -EFAULT;
> +			ret = -EFAULT;
> +			goto out;
>   		}
> -	} else
> +	} else {
>   		memcpy(kbuf, READ_ONCE(client->rbuf.rpos), n);
> +	}
>
>   	if (sbefifo_buf_readnb(&client->rbuf, n)) {
> -		xfr = sbefifo_client_next_xfr(client);
> +		xfr = list_first_entry_or_null(&client->xfrs,
> +					       struct sbefifo_xfr, client);
> +		if (!xfr) {
> +			/* should be impossible to not have an xfr here */
> +			wake_up(&sbefifo->wait);
> +			goto out;
> +		}
> +
>   		if (!test_bit(SBEFIFO_XFR_COMPLETE, &xfr->flags)) {
>   			/*
>   			 * Fill the read buffer back up.
> @@ -589,22 +613,31 @@ static ssize_t sbefifo_read_common(struct sbefifo_client *client,
>   		}
>   	}
>
> -	sbefifo_put_client(client);
> +	ret = n;
>
> -	return n;
> +out:
> +	sbefifo_put_client(client);
> +	return ret;
>   }
>
> -static ssize_t sbefifo_read(struct file *file, char __user *buf,
> -		size_t len, loff_t *offset)
> +static ssize_t sbefifo_read(struct file *file, char __user *buf, size_t len,
> +			    loff_t *offset)
>   {
>   	struct sbefifo_client *client = file->private_data;
>
> -	WARN_ON(*offset);
> +	return sbefifo_read_common(client, buf, NULL, len);
> +}
>
> -	if (!access_ok(VERIFY_WRITE, buf, len))
> -		return -EFAULT;
> +static bool sbefifo_write_ready(struct sbefifo *sbefifo,
> +				struct sbefifo_xfr *xfr,
> +				struct sbefifo_client *client, size_t *n)
> +{
> +	struct sbefifo_xfr *next = list_first_entry_or_null(&client->xfrs,
> +							    struct sbefifo_xfr,
> +							    client);
>
> -	return sbefifo_read_common(client, buf, NULL, len);
> +	*n = sbefifo_buf_nbwriteable(&client->wbuf);
> +	return READ_ONCE(sbefifo->rc) || (next == xfr && *n);
>   }
>
>   static ssize_t sbefifo_write_common(struct sbefifo_client *client,
> @@ -612,6 +645,7 @@ static ssize_t sbefifo_write_common(struct sbefifo_client *client,
>   				    size_t len)
>   {
>   	struct sbefifo *sbefifo = client->dev;
> +	struct sbefifo_buf *wbuf = &client->wbuf;
>   	struct sbefifo_xfr *xfr;
>   	ssize_t ret = 0;
>   	size_t n;
> @@ -622,24 +656,26 @@ static ssize_t sbefifo_write_common(struct sbefifo_client *client,
>   	if (!len)
>   		return 0;
>
> -	n = sbefifo_buf_nbwriteable(&client->wbuf);
> +	sbefifo_get_client(client);
> +	n = sbefifo_buf_nbwriteable(wbuf);
>
>   	spin_lock_irq(&sbefifo->lock);
> -	xfr = sbefifo_next_xfr(sbefifo);
>
> +	xfr = sbefifo_next_xfr(sbefifo);	/* next xfr to be executed */
>   	if ((client->f_flags & O_NONBLOCK) && xfr && n < len) {
>   		spin_unlock_irq(&sbefifo->lock);
> -		return -EAGAIN;
> +		ret = -EAGAIN;
> +		goto out;
>   	}
>
> -	xfr = sbefifo_enq_xfr(client);
> -	if (!xfr) {
> +	xfr = sbefifo_enq_xfr(client);		/* this xfr queued up */
> +	if (IS_ERR(xfr)) {
>   		spin_unlock_irq(&sbefifo->lock);
> -		return -ENOMEM;
> +		ret = PTR_ERR(xfr);
> +		goto out;
>   	}
> -	spin_unlock_irq(&sbefifo->lock);
>
> -	sbefifo_get_client(client);
> +	spin_unlock_irq(&sbefifo->lock);
>
>   	/*
>   	 * Partial writes are not really allowed in that EOT is sent exactly
> @@ -647,49 +683,47 @@ static ssize_t sbefifo_write_common(struct sbefifo_client *client,
>   	 */
>   	while (len) {
>   		if (wait_event_interruptible(sbefifo->wait,
> -				READ_ONCE(sbefifo->rc) ||
> -					(sbefifo_client_next_xfr(client) == xfr &&
> -					 (n = sbefifo_buf_nbwriteable(
> -							&client->wbuf))))) {
> +					     sbefifo_write_ready(sbefifo, xfr,
> +								 client,
> +								 &n))) {
>   			set_bit(SBEFIFO_XFR_CANCEL, &xfr->flags);
>   			sbefifo_get(sbefifo);
>   			if (mod_timer(&sbefifo->poll_timer, jiffies))
>   				sbefifo_put(sbefifo);
> -
> -			sbefifo_put_client(client);
> -			return -ERESTARTSYS;
> +			ret = -ERESTARTSYS;
> +			goto out;
>   		}
> -		if (sbefifo->rc) {
> +
> +		ret = READ_ONCE(sbefifo->rc);
> +		if (ret) {
>   			INIT_LIST_HEAD(&client->xfrs);
> -			sbefifo_put_client(client);
> -			return sbefifo->rc;
> +			goto out;
>   		}
>
>   		n = min_t(size_t, n, len);
>
>   		if (ubuf) {
> -			if (copy_from_user(READ_ONCE(client->wbuf.wpos), ubuf,
> -			    n)) {
> +			if (copy_from_user(READ_ONCE(wbuf->wpos), ubuf, n)) {
>   				set_bit(SBEFIFO_XFR_CANCEL, &xfr->flags);
>   				sbefifo_get(sbefifo);
>   				if (mod_timer(&sbefifo->poll_timer, jiffies))
>   					sbefifo_put(sbefifo);
> -				sbefifo_put_client(client);
> -				return -EFAULT;
> +				ret = -EFAULT;
> +				goto out;
>   			}
>
>   			ubuf += n;
>   		} else {
> -			memcpy(READ_ONCE(client->wbuf.wpos), kbuf, n);
> +			memcpy(READ_ONCE(wbuf->wpos), kbuf, n);
>   			kbuf += n;
>   		}
>
> -		sbefifo_buf_wrotenb(&client->wbuf, n);
> +		sbefifo_buf_wrotenb(wbuf, n);
>   		len -= n;
>   		ret += n;
>
> -		/* set flag before starting the worker, as it may run through
> -		 * and check the flag before we exit this loop!
> +		/* Set this before starting timer to avoid race condition on
> +		 * this flag with the timer function writer.
>   		 */
>   		if (!len)
>   			set_bit(SBEFIFO_XFR_WRITE_DONE, &xfr->flags);
> @@ -702,21 +736,16 @@ static ssize_t sbefifo_write_common(struct sbefifo_client *client,
>   			sbefifo_put(sbefifo);
>   	}
>
> +out:
>   	sbefifo_put_client(client);
> -
>   	return ret;
>   }
>
>   static ssize_t sbefifo_write(struct file *file, const char __user *buf,
> -		size_t len, loff_t *offset)
> +			     size_t len, loff_t *offset)
>   {
>   	struct sbefifo_client *client = file->private_data;
>
> -	WARN_ON(*offset);
> -
> -	if (!access_ok(VERIFY_READ, buf, len))
> -		return -EFAULT;
> -
>   	return sbefifo_write_common(client, buf, NULL, len);
>   }
>
> @@ -742,18 +771,15 @@ static int sbefifo_release(struct inode *inode, struct file *file)
>   struct sbefifo_client *sbefifo_drv_open(struct device *dev,
>   					unsigned long flags)
>   {
> -	struct sbefifo_client *client = NULL;
> -	struct sbefifo *sbefifo;
> -	struct fsi_device *fsi_dev = to_fsi_dev(dev);
> +	struct sbefifo_client *client;
> +	struct sbefifo *sbefifo = dev_get_drvdata(dev);
>
> -	list_for_each_entry(sbefifo, &sbefifo_fifos, link) {
> -		if (sbefifo->fsi_dev != fsi_dev)
> -			continue;
> +	if (!sbefifo)
> +		return NULL;
>
> -		client = sbefifo_new_client(sbefifo);
> -		if (client)
> -			client->f_flags = flags;
> -	}
> +	client = sbefifo_new_client(sbefifo);
> +	if (client)
> +		client->f_flags = flags;
>
>   	return client;
>   }
> @@ -802,95 +828,92 @@ static int sbefifo_probe(struct device *dev)
>   	u32 sts;
>   	int ret, child_idx = 0;
>
> -	dev_dbg(dev, "Found sbefifo device\n");
>   	sbefifo = kzalloc(sizeof(*sbefifo), GFP_KERNEL);
>   	if (!sbefifo)
>   		return -ENOMEM;
>
>   	sbefifo->fsi_dev = fsi_dev;
>
> -	ret = sbefifo_inw(sbefifo,
> -			SBEFIFO_UP | SBEFIFO_STS, &sts);
> +	ret = sbefifo_inw(sbefifo, SBEFIFO_UP | SBEFIFO_STS, &sts);
>   	if (ret)
>   		return ret;
> +
>   	if (!(sts & SBEFIFO_EMPTY)) {
> -		dev_err(&sbefifo->fsi_dev->dev,
> -				"Found data in upstream fifo\n");
> +		dev_err(dev, "Found data in upstream fifo\n");
>   		return -EIO;
>   	}
>
>   	ret = sbefifo_inw(sbefifo, SBEFIFO_DWN | SBEFIFO_STS, &sts);
>   	if (ret)
>   		return ret;
> +
>   	if (!(sts & SBEFIFO_EMPTY)) {
> -		dev_err(&sbefifo->fsi_dev->dev,
> -				"Found data in downstream fifo\n");
> +		dev_err(dev, "found data in downstream fifo\n");
>   		return -EIO;
>   	}
>
> -	sbefifo->mdev.minor = MISC_DYNAMIC_MINOR;
> -	sbefifo->mdev.fops = &sbefifo_fops;
> -	sbefifo->mdev.name = sbefifo->name;
> -	sbefifo->mdev.parent = dev;
>   	spin_lock_init(&sbefifo->lock);
>   	kref_init(&sbefifo->kref);
> +	init_waitqueue_head(&sbefifo->wait);
> +	INIT_LIST_HEAD(&sbefifo->xfrs);
>
>   	sbefifo->idx = ida_simple_get(&sbefifo_ida, 1, INT_MAX, GFP_KERNEL);
>   	snprintf(sbefifo->name, sizeof(sbefifo->name), "sbefifo%d",
> -			sbefifo->idx);
> -	init_waitqueue_head(&sbefifo->wait);
> -	INIT_LIST_HEAD(&sbefifo->xfrs);
> +		 sbefifo->idx);
>
>   	/* This bit of silicon doesn't offer any interrupts... */
>   	setup_timer(&sbefifo->poll_timer, sbefifo_poll_timer,
> -			(unsigned long)sbefifo);
> -
> -	if (dev->of_node) {
> -		/* create platform devs for dts child nodes (occ, etc) */
> -		for_each_child_of_node(dev->of_node, np) {
> -			snprintf(child_name, sizeof(child_name), "%s-dev%d",
> -				 sbefifo->name, child_idx++);
> -			child = of_platform_device_create(np, child_name, dev);
> -			if (!child)
> -				dev_warn(&sbefifo->fsi_dev->dev,
> -					 "failed to create child node dev\n");
> -		}
> +		    (unsigned long)sbefifo);
> +
> +	sbefifo->mdev.minor = MISC_DYNAMIC_MINOR;
> +	sbefifo->mdev.fops = &sbefifo_fops;
> +	sbefifo->mdev.name = sbefifo->name;
> +	sbefifo->mdev.parent = dev;
> +	ret = misc_register(&sbefifo->mdev);
> +	if (ret) {
> +		dev_err(dev, "failed to register miscdevice: %d\n", ret);
> +		ida_simple_remove(&sbefifo_ida, sbefifo->idx);
> +		sbefifo_put(sbefifo);
> +		return ret;
> +	}
> +
> +	/* create platform devs for dts child nodes (occ, etc) */
> +	for_each_available_child_of_node(dev->of_node, np) {
> +		snprintf(child_name, sizeof(child_name), "%s-dev%d",
> +			 sbefifo->name, child_idx++);
> +		child = of_platform_device_create(np, child_name, dev);
> +		if (!child)
> +			dev_warn(dev, "failed to create child %s dev\n",
> +				 child_name);
>   	}
>
> -	list_add(&sbefifo->link, &sbefifo_fifos);
> -	
> -	return misc_register(&sbefifo->mdev);
> +	dev_set_drvdata(dev, sbefifo);
> +
> +	return 0;
>   }
>
>   static int sbefifo_remove(struct device *dev)
>   {
> -	struct fsi_device *fsi_dev = to_fsi_dev(dev);
> -	struct sbefifo *sbefifo, *sbefifo_tmp;
> +	struct sbefifo *sbefifo = dev_get_drvdata(dev);
>   	struct sbefifo_xfr *xfr;
>
> -	list_for_each_entry_safe(sbefifo, sbefifo_tmp, &sbefifo_fifos, link) {
> -		if (sbefifo->fsi_dev != fsi_dev)
> -			continue;
> -
> -		device_for_each_child(dev, NULL, sbefifo_unregister_child);
> +	WRITE_ONCE(sbefifo->rc, -ENODEV);
> +	wake_up(&sbefifo->wait);
>
> -		misc_deregister(&sbefifo->mdev);
> -		list_del(&sbefifo->link);
> -		ida_simple_remove(&sbefifo_ida, sbefifo->idx);
> +	misc_deregister(&sbefifo->mdev);
> +	device_for_each_child(dev, NULL, sbefifo_unregister_child);
>
> -		if (del_timer_sync(&sbefifo->poll_timer))
> -			sbefifo_put(sbefifo);
> +	ida_simple_remove(&sbefifo_ida, sbefifo->idx);
>
> -		spin_lock(&sbefifo->lock);
> -		list_for_each_entry(xfr, &sbefifo->xfrs, xfrs)
> -			kfree(xfr);
> -		spin_unlock(&sbefifo->lock);
> +	if (del_timer_sync(&sbefifo->poll_timer))
> +		sbefifo_put(sbefifo);
>
> -		WRITE_ONCE(sbefifo->rc, -ENODEV);
> +	spin_lock(&sbefifo->lock);
> +	list_for_each_entry(xfr, &sbefifo->xfrs, xfrs)
> +		kfree(xfr);
> +	spin_unlock(&sbefifo->lock);
>
> -		wake_up(&sbefifo->wait);
> -		sbefifo_put(sbefifo);
> -	}
> +	sbefifo_put(sbefifo);
>
>   	return 0;
>   }
> @@ -915,17 +938,19 @@ static int sbefifo_remove(struct device *dev)
>
>   static int sbefifo_init(void)
>   {
> -	INIT_LIST_HEAD(&sbefifo_fifos);
>   	return fsi_driver_register(&sbefifo_drv);
>   }
>
>   static void sbefifo_exit(void)
>   {
>   	fsi_driver_unregister(&sbefifo_drv);
> +
> +	ida_destroy(&sbefifo_ida);
>   }
>
>   module_init(sbefifo_init);
>   module_exit(sbefifo_exit);
>   MODULE_LICENSE("GPL");
>   MODULE_AUTHOR("Brad Bishop <bradleyb@fuzziesquirrel.com>");
> -MODULE_DESCRIPTION("Linux device interface to the POWER self boot engine");
> +MODULE_AUTHOR("Eddie James <eajames@linux.vnet.ibm.com>");
> +MODULE_DESCRIPTION("Linux device interface to the POWER Self Boot Engine");
> diff --git a/include/linux/fsi-sbefifo.h b/include/linux/fsi-sbefifo.h
> index 1b46c63..8e55891 100644
> --- a/include/linux/fsi-sbefifo.h
> +++ b/include/linux/fsi-sbefifo.h
> @@ -13,8 +13,8 @@
>    * GNU General Public License for more details.
>    */
>
> -#ifndef __FSI_SBEFIFO_H__
> -#define __FSI_SBEFIFO_H__
> +#ifndef LINUX_FSI_SBEFIFO_H
> +#define LINUX_FSI_SBEFIFO_H
>
>   struct device;
>   struct sbefifo_client;
> @@ -27,4 +27,4 @@ extern int sbefifo_drv_write(struct sbefifo_client *client, const char *buf,
>   			     size_t len);
>   extern void sbefifo_drv_release(struct sbefifo_client *client);
>
> -#endif /* __FSI_SBEFIFO_H__ */
> +#endif /* LINUX_FSI_SBEFIFO_H */
Andrew Jeffery Sept. 25, 2017, 4:57 a.m. UTC | #2 
On Thu, 2017-09-21 at 17:43 -0500, Eddie James wrote:
> From: "Edward A. James" <eajames@us.ibm.com>
> 
> Includes various fixes:
>  - check for complete while waiting for data in read()
>  - fix probe if probe fails
>  - general cleanup
>  - slightly safer (earlier) get_client()
>  - reorder some of the remove() operations for safety

The fact that you can create a commit message that describes the patch in
terms of bullet points indicates to me that these bullet points should all be
separate patches. Squashing conceptually different changes like this makes the
patch much harder to review, as it's hard to tease apart how and why a change
fixes any one of the deficient behaviours.

Please split them out in a follow-up if there is one. My personal preference is
you do a v2 regardless just so you can split the patches out.

> 
> Signed-off-by: Edward A. James <eajames@us.ibm.com>
> ---
>  drivers/fsi/fsi-sbefifo.c   | 329 ++++++++++++++++++++++++--------------------
>  include/linux/fsi-sbefifo.h |   6 +-
>  2 files changed, 180 insertions(+), 155 deletions(-)
> 
> diff --git a/drivers/fsi/fsi-sbefifo.c b/drivers/fsi/fsi-sbefifo.c
> index 1c37ff7..5d25ade 100644
> --- a/drivers/fsi/fsi-sbefifo.c
> +++ b/drivers/fsi/fsi-sbefifo.c
> @@ -11,20 +11,27 @@
>   * GNU General Public License for more details.
>   */
>  
> -#include <linux/delay.h>
> +#include <linux/device.h>
>  #include <linux/errno.h>
> -#include <linux/idr.h>
> +#include <linux/fs.h>
>  #include <linux/fsi.h>
> +#include <linux/fsi-sbefifo.h>
> +#include <linux/idr.h>
> +#include <linux/kernel.h>
> +#include <linux/kref.h>
>  #include <linux/list.h>
>  #include <linux/miscdevice.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
> +#include <linux/of_device.h>
>  #include <linux/of_platform.h>
>  #include <linux/poll.h>
>  #include <linux/sched.h>
>  #include <linux/slab.h>
> +#include <linux/spinlock.h>
>  #include <linux/timer.h>
>  #include <linux/uaccess.h>
> +#include <linux/wait.h>
>  
>  /*
>   * The SBEFIFO is a pipe-like FSI device for communicating with
> @@ -44,14 +51,15 @@
>  #define   SBEFIFO_EOT_MAGIC		0xffffffff
>  #define SBEFIFO_EOT_ACK		0x14
>  
> +#define SBEFIFO_RESCHEDULE	msecs_to_jiffies(500)
> +
>  struct sbefifo {
>  	struct timer_list poll_timer;
> -	struct fsi_device *fsi_dev;
> -	struct miscdevice mdev;
> +	struct fsi_device *fsi_dev;	/* parent fsi device */

Not convinced the comment is helpful.

> +	struct miscdevice mdev;		/* /dev/ entry */

Not necesarily the intent of this patch, but I think it would be a good thing
to split out the SBEFIFO core from the chardev interface.

>  	wait_queue_head_t wait;
> -	struct list_head link;
>  	struct list_head xfrs;
> -	struct kref kref;
> +	struct kref kref;		/* reference counter */

Not convinced the comment is helpful.

>  	spinlock_t lock;
>  	char name[32];
>  	int idx;
> @@ -87,14 +95,12 @@ struct sbefifo_client {
>  	unsigned long f_flags;
>  };
>  
> -static struct list_head sbefifo_fifos;
> -

It's not clear to me why this is being removed. Which one of the bullets in the
commit message accounts for this?

>  static DEFINE_IDA(sbefifo_ida);
>  
>  static int sbefifo_inw(struct sbefifo *sbefifo, int reg, u32 *word)
>  {
>  	int rc;
> -	u32 raw_word;
> +	__be32 raw_word;
>  
>  	rc = fsi_device_read(sbefifo->fsi_dev, reg, &raw_word,
>  			     sizeof(raw_word));
> @@ -102,17 +108,19 @@ static int sbefifo_inw(struct sbefifo *sbefifo, int reg, u32 *word)
>  		return rc;
>  
>  	*word = be32_to_cpu(raw_word);
> +
>  	return 0;
>  }
>  
>  static int sbefifo_outw(struct sbefifo *sbefifo, int reg, u32 word)
>  {
> -	u32 raw_word = cpu_to_be32(word);
> +	__be32 raw_word = cpu_to_be32(word);
>  
>  	return fsi_device_write(sbefifo->fsi_dev, reg, &raw_word,
>  				sizeof(raw_word));
>  }
>  
> +/* Don't flip endianness of data to/from FIFO, just pass through. */
>  static int sbefifo_readw(struct sbefifo *sbefifo, u32 *word)
>  {
>  	return fsi_device_read(sbefifo->fsi_dev, SBEFIFO_DWN, word,
> @@ -136,7 +144,7 @@ static int sbefifo_ack_eot(struct sbefifo *sbefifo)
>  		return ret;
>  
>  	return sbefifo_outw(sbefifo, SBEFIFO_DWN | SBEFIFO_EOT_ACK,
> -			SBEFIFO_EOT_MAGIC);
> +			    SBEFIFO_EOT_MAGIC);
>  }
>  
>  static size_t sbefifo_dev_nwreadable(u32 sts)
> @@ -210,6 +218,7 @@ static bool sbefifo_buf_readnb(struct sbefifo_buf *buf, size_t n)
>  		rpos = buf->buf;
>  
>  	WRITE_ONCE(buf->rpos, rpos);
> +
>  	return rpos == wpos;
>  }
>  
> @@ -229,14 +238,14 @@ static bool sbefifo_buf_wrotenb(struct sbefifo_buf *buf, size_t n)
>  		set_bit(SBEFIFO_BUF_FULL, &buf->flags);
>  
>  	WRITE_ONCE(buf->wpos, wpos);
> +
>  	return rpos == wpos;
>  }
>  
>  static void sbefifo_free(struct kref *kref)
>  {
> -	struct sbefifo *sbefifo;
> +	struct sbefifo *sbefifo = container_of(kref, struct sbefifo, kref);
>  
> -	sbefifo = container_of(kref, struct sbefifo, kref);
>  	kfree(sbefifo);
>  }
>  
> @@ -255,9 +264,12 @@ static struct sbefifo_xfr *sbefifo_enq_xfr(struct sbefifo_client *client)
>  	struct sbefifo *sbefifo = client->dev;
>  	struct sbefifo_xfr *xfr;
>  
> +	if (READ_ONCE(sbefifo->rc))

What semantics are we trying to enforce with READ_ONCE()?

> +		return ERR_PTR(sbefifo->rc);

This will possibly read sbefifo->rc again?

> +
>  	xfr = kzalloc(sizeof(*xfr), GFP_KERNEL);
>  	if (!xfr)
> -		return NULL;
> +		return ERR_PTR(-ENOMEM);
>  
>  	xfr->rbuf = &client->rbuf;
>  	xfr->wbuf = &client->wbuf;
> @@ -267,21 +279,12 @@ static struct sbefifo_xfr *sbefifo_enq_xfr(struct sbefifo_client *client)
>  	return xfr;
>  }
>  
> -static struct sbefifo_xfr *sbefifo_client_next_xfr(
> -		struct sbefifo_client *client)
> -{
> -	if (list_empty(&client->xfrs))
> -		return NULL;
> -
> -	return container_of(client->xfrs.next, struct sbefifo_xfr,
> -			client);
> -}
> -
>  static bool sbefifo_xfr_rsp_pending(struct sbefifo_client *client)
>  {
> -	struct sbefifo_xfr *xfr;
> +	struct sbefifo_xfr *xfr = list_first_entry_or_null(&client->xfrs,
> +							   struct sbefifo_xfr,
> +							   client);
>  
> -	xfr = sbefifo_client_next_xfr(client);
>  	if (xfr && test_bit(SBEFIFO_XFR_RESP_PENDING, &xfr->flags))
>  		return true;
>  
> @@ -309,10 +312,11 @@ static struct sbefifo_client *sbefifo_new_client(struct sbefifo *sbefifo)
>  
>  static void sbefifo_client_release(struct kref *kref)
>  {
> -	struct sbefifo_client *client;
>  	struct sbefifo_xfr *xfr;
> +	struct sbefifo_client *client = container_of(kref,
> +						     struct sbefifo_client,
> +						     kref);
>  
> -	client = container_of(kref, struct sbefifo_client, kref);

Why did we move to assigning this in the declaration? It looks better this way,
and it's not as if the pointer itself is const.

>  	list_for_each_entry(xfr, &client->xfrs, client) {
>  		/*
>  		 * The client left with pending or running xfrs.
> @@ -349,6 +353,7 @@ static struct sbefifo_xfr *sbefifo_next_xfr(struct sbefifo *sbefifo)
>  			kfree(xfr);
>  			continue;
>  		}
> +
>  		return xfr;
>  	}
>  
> @@ -370,7 +375,7 @@ static void sbefifo_poll_timer(unsigned long data)
>  
>  	spin_lock(&sbefifo->lock);
>  	xfr = list_first_entry_or_null(&sbefifo->xfrs, struct sbefifo_xfr,
> -			xfrs);
> +				       xfrs);
>  	if (!xfr)
>  		goto out_unlock;
>  
> @@ -388,8 +393,7 @@ static void sbefifo_poll_timer(unsigned long data)
>  
>  	 /* Drain the write buffer. */
>  	while ((bufn = sbefifo_buf_nbreadable(wbuf))) {
> -		ret = sbefifo_inw(sbefifo, SBEFIFO_UP | SBEFIFO_STS,
> -				&sts);
> +		ret = sbefifo_inw(sbefifo, SBEFIFO_UP | SBEFIFO_STS, &sts);
>  		if (ret)
>  			goto out;
>  
> @@ -397,7 +401,7 @@ static void sbefifo_poll_timer(unsigned long data)
>  		if (devn == 0) {
>  			/* No open slot for write.  Reschedule. */
>  			sbefifo->poll_timer.expires = jiffies +
> -				msecs_to_jiffies(500);
> +				SBEFIFO_RESCHEDULE;
>  			add_timer(&sbefifo->poll_timer);

Not really a comment relevant for the patch and I don't know much about the
operations of the SBEFIFO, but using a timer for slot management feels odd.

However, I think this is covered by the comment in probe about the silicon
lacking interrupts. Ugh.

>  			goto out_unlock;
>  		}
> @@ -414,9 +418,8 @@ static void sbefifo_poll_timer(unsigned long data)
>  
>  	 /* Send EOT if the writer is finished. */
>  	if (test_and_clear_bit(SBEFIFO_XFR_WRITE_DONE, &xfr->flags)) {
> -		ret = sbefifo_outw(sbefifo,
> -				SBEFIFO_UP | SBEFIFO_EOT_RAISE,
> -				SBEFIFO_EOT_MAGIC);
> +		ret = sbefifo_outw(sbefifo, SBEFIFO_UP | SBEFIFO_EOT_RAISE,
> +				   SBEFIFO_EOT_MAGIC);
>  		if (ret)
>  			goto out;
>  
> @@ -438,7 +441,7 @@ static void sbefifo_poll_timer(unsigned long data)
>  		if (devn == 0) {
>  			/* No data yet.  Reschedule. */
>  			sbefifo->poll_timer.expires = jiffies +
> -				msecs_to_jiffies(500);
> +				SBEFIFO_RESCHEDULE;
>  			add_timer(&sbefifo->poll_timer);
>  			goto out_unlock;
>  		}
> @@ -466,7 +469,7 @@ static void sbefifo_poll_timer(unsigned long data)
>  			set_bit(SBEFIFO_XFR_COMPLETE, &xfr->flags);
>  			list_del(&xfr->xfrs);
>  			if (unlikely(test_bit(SBEFIFO_XFR_CANCEL,
> -							&xfr->flags)))
> +					      &xfr->flags)))
>  				kfree(xfr);
>  			break;
>  		}
> @@ -476,7 +479,7 @@ static void sbefifo_poll_timer(unsigned long data)
>  	if (unlikely(ret)) {
>  		sbefifo->rc = ret;
>  		dev_err(&sbefifo->fsi_dev->dev,
> -				"Fatal bus access failure: %d\n", ret);
> +			"Fatal bus access failure: %d\n", ret);
>  		list_for_each_entry(xfr, &sbefifo->xfrs, xfrs)
>  			kfree(xfr);
>  		INIT_LIST_HEAD(&sbefifo->xfrs);
> @@ -497,7 +500,7 @@ static void sbefifo_poll_timer(unsigned long data)
>  static int sbefifo_open(struct inode *inode, struct file *file)
>  {
>  	struct sbefifo *sbefifo = container_of(file->private_data,
> -			struct sbefifo, mdev);
> +					       struct sbefifo, mdev);
>  	struct sbefifo_client *client;
>  	int ret;
>  
> @@ -535,6 +538,19 @@ static unsigned int sbefifo_poll(struct file *file, poll_table *wait)
>  	return mask;
>  }
>  
> +static bool sbefifo_read_ready(struct sbefifo *sbefifo,
> +			       struct sbefifo_client *client, size_t *n)
> +{
> +	struct sbefifo_xfr *xfr = list_first_entry_or_null(&client->xfrs,
> +							   struct sbefifo_xfr,
> +							   client);
> +
> +	*n = sbefifo_buf_nbreadable(&client->rbuf);
> +
> +	return READ_ONCE(sbefifo->rc) || *n ||
> +		(xfr && test_bit(SBEFIFO_XFR_COMPLETE, &xfr->flags));
> +}
> +
>  static ssize_t sbefifo_read_common(struct sbefifo_client *client,
>  				   char __user *ubuf, char *kbuf, size_t len)
>  {
> @@ -551,30 +567,38 @@ static ssize_t sbefifo_read_common(struct sbefifo_client *client,
>  
>  	sbefifo_get_client(client);
>  	if (wait_event_interruptible(sbefifo->wait,
> -				(ret = READ_ONCE(sbefifo->rc)) ||
> -				(n = sbefifo_buf_nbreadable(
> -						&client->rbuf)))) {
> -		sbefifo_put_client(client);
> -		return -ERESTARTSYS;
> +				     sbefifo_read_ready(sbefifo, client,
> +							&n))) {
> +		ret = -ERESTARTSYS;
> +		goto out;
>  	}
> +
> +	ret = READ_ONCE(sbefifo->rc);
>  	if (ret) {
>  		INIT_LIST_HEAD(&client->xfrs);
> -		sbefifo_put_client(client);
> -		return ret;
> +		goto out;
>  	}
>  
>  	n = min_t(size_t, n, len);
>  
>  	if (ubuf) {
>  		if (copy_to_user(ubuf, READ_ONCE(client->rbuf.rpos), n)) {
> -			sbefifo_put_client(client);
> -			return -EFAULT;
> +			ret = -EFAULT;
> +			goto out;
>  		}
> -	} else
> +	} else {
>  		memcpy(kbuf, READ_ONCE(client->rbuf.rpos), n);
> +	}
>  
>  	if (sbefifo_buf_readnb(&client->rbuf, n)) {
> -		xfr = sbefifo_client_next_xfr(client);
> +		xfr = list_first_entry_or_null(&client->xfrs,
> +					       struct sbefifo_xfr, client);
> +		if (!xfr) {
> +			/* should be impossible to not have an xfr here */

Maybe you want a `if (WARN_ON(!xfr)) {` here?

> +			wake_up(&sbefifo->wait);
> +			goto out;
> +		}
> +
>  		if (!test_bit(SBEFIFO_XFR_COMPLETE, &xfr->flags)) {
>  			/*
>  			 * Fill the read buffer back up.
> @@ -589,22 +613,31 @@ static ssize_t sbefifo_read_common(struct sbefifo_client *client,
>  		}
>  	}
>  
> -	sbefifo_put_client(client);
> +	ret = n;
>  
> -	return n;
> +out:
> +	sbefifo_put_client(client);
> +	return ret;
>  }
>  
> -static ssize_t sbefifo_read(struct file *file, char __user *buf,
> -		size_t len, loff_t *offset)
> +static ssize_t sbefifo_read(struct file *file, char __user *buf, size_t len,
> +			    loff_t *offset)
>  {
>  	struct sbefifo_client *client = file->private_data;
>  
> -	WARN_ON(*offset);
> +	return sbefifo_read_common(client, buf, NULL, len);
> +}
>  
> -	if (!access_ok(VERIFY_WRITE, buf, len))
> -		return -EFAULT;
> +static bool sbefifo_write_ready(struct sbefifo *sbefifo,
> +				struct sbefifo_xfr *xfr,
> +				struct sbefifo_client *client, size_t *n)
> +{
> +	struct sbefifo_xfr *next = list_first_entry_or_null(&client->xfrs,
> +							    struct sbefifo_xfr,
> +							    client);
>  
> -	return sbefifo_read_common(client, buf, NULL, len);
> +	*n = sbefifo_buf_nbwriteable(&client->wbuf);
> +	return READ_ONCE(sbefifo->rc) || (next == xfr && *n);
>  }
>  
>  static ssize_t sbefifo_write_common(struct sbefifo_client *client,
> @@ -612,6 +645,7 @@ static ssize_t sbefifo_write_common(struct sbefifo_client *client,
>  				    size_t len)
>  {
>  	struct sbefifo *sbefifo = client->dev;
> +	struct sbefifo_buf *wbuf = &client->wbuf;
>  	struct sbefifo_xfr *xfr;
>  	ssize_t ret = 0;
>  	size_t n;
> @@ -622,24 +656,26 @@ static ssize_t sbefifo_write_common(struct sbefifo_client *client,
>  	if (!len)
>  		return 0;
>  
> -	n = sbefifo_buf_nbwriteable(&client->wbuf);
> +	sbefifo_get_client(client);
> +	n = sbefifo_buf_nbwriteable(wbuf);
>  
>  	spin_lock_irq(&sbefifo->lock);
> -	xfr = sbefifo_next_xfr(sbefifo);
>  
> +	xfr = sbefifo_next_xfr(sbefifo);	/* next xfr to be executed */
>  	if ((client->f_flags & O_NONBLOCK) && xfr && n < len) {
>  		spin_unlock_irq(&sbefifo->lock);
> -		return -EAGAIN;
> +		ret = -EAGAIN;
> +		goto out;
>  	}
>  
> -	xfr = sbefifo_enq_xfr(client);
> -	if (!xfr) {
> +	xfr = sbefifo_enq_xfr(client);		/* this xfr queued up */
> +	if (IS_ERR(xfr)) {
>  		spin_unlock_irq(&sbefifo->lock);
> -		return -ENOMEM;
> +		ret = PTR_ERR(xfr);
> +		goto out;
>  	}
> -	spin_unlock_irq(&sbefifo->lock);
>  
> -	sbefifo_get_client(client);
> +	spin_unlock_irq(&sbefifo->lock);
>  
>  	/*
>  	 * Partial writes are not really allowed in that EOT is sent exactly
> @@ -647,49 +683,47 @@ static ssize_t sbefifo_write_common(struct sbefifo_client *client,
>  	 */
>  	while (len) {
>  		if (wait_event_interruptible(sbefifo->wait,
> -				READ_ONCE(sbefifo->rc) ||
> -					(sbefifo_client_next_xfr(client) == xfr &&
> -					 (n = sbefifo_buf_nbwriteable(
> -							&client->wbuf))))) {
> +					     sbefifo_write_ready(sbefifo, xfr,
> +								 client,
> +								 &n))) {
>  			set_bit(SBEFIFO_XFR_CANCEL, &xfr->flags);
>  			sbefifo_get(sbefifo);
>  			if (mod_timer(&sbefifo->poll_timer, jiffies))
>  				sbefifo_put(sbefifo);
> -
> -			sbefifo_put_client(client);
> -			return -ERESTARTSYS;
> +			ret = -ERESTARTSYS;
> +			goto out;
>  		}
> -		if (sbefifo->rc) {
> +
> +		ret = READ_ONCE(sbefifo->rc);
> +		if (ret) {
>  			INIT_LIST_HEAD(&client->xfrs);
> -			sbefifo_put_client(client);
> -			return sbefifo->rc;
> +			goto out;
>  		}
>  
>  		n = min_t(size_t, n, len);
>  
>  		if (ubuf) {
> -			if (copy_from_user(READ_ONCE(client->wbuf.wpos), ubuf,
> -			    n)) {
> +			if (copy_from_user(READ_ONCE(wbuf->wpos), ubuf, n)) {
>  				set_bit(SBEFIFO_XFR_CANCEL, &xfr->flags);
>  				sbefifo_get(sbefifo);
>  				if (mod_timer(&sbefifo->poll_timer, jiffies))
>  					sbefifo_put(sbefifo);
> -				sbefifo_put_client(client);
> -				return -EFAULT;
> +				ret = -EFAULT;
> +				goto out;
>  			}
>  
>  			ubuf += n;
>  		} else {
> -			memcpy(READ_ONCE(client->wbuf.wpos), kbuf, n);
> +			memcpy(READ_ONCE(wbuf->wpos), kbuf, n);
>  			kbuf += n;
>  		}
>  
> -		sbefifo_buf_wrotenb(&client->wbuf, n);
> +		sbefifo_buf_wrotenb(wbuf, n);
>  		len -= n;
>  		ret += n;
>  
> -		/* set flag before starting the worker, as it may run through
> -		 * and check the flag before we exit this loop!
> +		/* Set this before starting timer to avoid race condition on
> +		 * this flag with the timer function writer.
>  		 */
>  		if (!len)
>  			set_bit(SBEFIFO_XFR_WRITE_DONE, &xfr->flags);
> @@ -702,21 +736,16 @@ static ssize_t sbefifo_write_common(struct sbefifo_client *client,
>  			sbefifo_put(sbefifo);
>  	}
>  
> +out:
>  	sbefifo_put_client(client);
> -
>  	return ret;
>  }
>  
>  static ssize_t sbefifo_write(struct file *file, const char __user *buf,
> -		size_t len, loff_t *offset)
> +			     size_t len, loff_t *offset)
>  {
>  	struct sbefifo_client *client = file->private_data;
>  
> -	WARN_ON(*offset);
> -
> -	if (!access_ok(VERIFY_READ, buf, len))
> -		return -EFAULT;
> -
>  	return sbefifo_write_common(client, buf, NULL, len);
>  }
>  
> @@ -742,18 +771,15 @@ static int sbefifo_release(struct inode *inode, struct file *file)
>  struct sbefifo_client *sbefifo_drv_open(struct device *dev,
>  					unsigned long flags)
>  {
> -	struct sbefifo_client *client = NULL;
> -	struct sbefifo *sbefifo;
> -	struct fsi_device *fsi_dev = to_fsi_dev(dev);
> +	struct sbefifo_client *client;
> +	struct sbefifo *sbefifo = dev_get_drvdata(dev);
>  
> -	list_for_each_entry(sbefifo, &sbefifo_fifos, link) {
> -		if (sbefifo->fsi_dev != fsi_dev)
> -			continue;
> +	if (!sbefifo)
> +		return NULL;
>  
> -		client = sbefifo_new_client(sbefifo);
> -		if (client)
> -			client->f_flags = flags;
> -	}
> +	client = sbefifo_new_client(sbefifo);
> +	if (client)
> +		client->f_flags = flags;
>  
>  	return client;
>  }
> @@ -802,95 +828,92 @@ static int sbefifo_probe(struct device *dev)
>  	u32 sts;
>  	int ret, child_idx = 0;
>  
> -	dev_dbg(dev, "Found sbefifo device\n");
>  	sbefifo = kzalloc(sizeof(*sbefifo), GFP_KERNEL);
>  	if (!sbefifo)
>  		return -ENOMEM;
>  
>  	sbefifo->fsi_dev = fsi_dev;
>  
> -	ret = sbefifo_inw(sbefifo,
> -			SBEFIFO_UP | SBEFIFO_STS, &sts);
> +	ret = sbefifo_inw(sbefifo, SBEFIFO_UP | SBEFIFO_STS, &sts);
>  	if (ret)
>  		return ret;
> +
>  	if (!(sts & SBEFIFO_EMPTY)) {
> -		dev_err(&sbefifo->fsi_dev->dev,
> -				"Found data in upstream fifo\n");
> +		dev_err(dev, "Found data in upstream fifo\n");
>  		return -EIO;
>  	}
>  
>  	ret = sbefifo_inw(sbefifo, SBEFIFO_DWN | SBEFIFO_STS, &sts);
>  	if (ret)
>  		return ret;
> +
>  	if (!(sts & SBEFIFO_EMPTY)) {
> -		dev_err(&sbefifo->fsi_dev->dev,
> -				"Found data in downstream fifo\n");
> +		dev_err(dev, "found data in downstream fifo\n");
>  		return -EIO;
>  	}
>  
> -	sbefifo->mdev.minor = MISC_DYNAMIC_MINOR;
> -	sbefifo->mdev.fops = &sbefifo_fops;
> -	sbefifo->mdev.name = sbefifo->name;
> -	sbefifo->mdev.parent = dev;
>  	spin_lock_init(&sbefifo->lock);
>  	kref_init(&sbefifo->kref);
> +	init_waitqueue_head(&sbefifo->wait);
> +	INIT_LIST_HEAD(&sbefifo->xfrs);
>  
>  	sbefifo->idx = ida_simple_get(&sbefifo_ida, 1, INT_MAX, GFP_KERNEL);
>  	snprintf(sbefifo->name, sizeof(sbefifo->name), "sbefifo%d",
> -			sbefifo->idx);
> -	init_waitqueue_head(&sbefifo->wait);
> -	INIT_LIST_HEAD(&sbefifo->xfrs);
> +		 sbefifo->idx);
>  
>  	/* This bit of silicon doesn't offer any interrupts... */
>  	setup_timer(&sbefifo->poll_timer, sbefifo_poll_timer,
> -			(unsigned long)sbefifo);
> -
> -	if (dev->of_node) {
> -		/* create platform devs for dts child nodes (occ, etc) */
> -		for_each_child_of_node(dev->of_node, np) {
> -			snprintf(child_name, sizeof(child_name), "%s-dev%d",
> -				 sbefifo->name, child_idx++);
> -			child = of_platform_device_create(np, child_name, dev);
> -			if (!child)
> -				dev_warn(&sbefifo->fsi_dev->dev,
> -					 "failed to create child node dev\n");
> -		}
> +		    (unsigned long)sbefifo);
> +
> +	sbefifo->mdev.minor = MISC_DYNAMIC_MINOR;
> +	sbefifo->mdev.fops = &sbefifo_fops;
> +	sbefifo->mdev.name = sbefifo->name;
> +	sbefifo->mdev.parent = dev;
> +	ret = misc_register(&sbefifo->mdev);
> +	if (ret) {
> +		dev_err(dev, "failed to register miscdevice: %d\n", ret);
> +		ida_simple_remove(&sbefifo_ida, sbefifo->idx);
> +		sbefifo_put(sbefifo);
> +		return ret;
> +	}
> +
> +	/* create platform devs for dts child nodes (occ, etc) */
> +	for_each_available_child_of_node(dev->of_node, np) {
> +		snprintf(child_name, sizeof(child_name), "%s-dev%d",
> +			 sbefifo->name, child_idx++);
> +		child = of_platform_device_create(np, child_name, dev);
> +		if (!child)
> +			dev_warn(dev, "failed to create child %s dev\n",
> +				 child_name);
>  	}
>  
> -	list_add(&sbefifo->link, &sbefifo_fifos);
> -	
> -	return misc_register(&sbefifo->mdev);
> +	dev_set_drvdata(dev, sbefifo);
> +
> +	return 0;
>  }
>  
>  static int sbefifo_remove(struct device *dev)
>  {
> -	struct fsi_device *fsi_dev = to_fsi_dev(dev);
> -	struct sbefifo *sbefifo, *sbefifo_tmp;
> +	struct sbefifo *sbefifo = dev_get_drvdata(dev);
>  	struct sbefifo_xfr *xfr;
>  
> -	list_for_each_entry_safe(sbefifo, sbefifo_tmp, &sbefifo_fifos, link) {
> -		if (sbefifo->fsi_dev != fsi_dev)
> -			continue;
> -
> -		device_for_each_child(dev, NULL, sbefifo_unregister_child);
> +	WRITE_ONCE(sbefifo->rc, -ENODEV);
> +	wake_up(&sbefifo->wait);
>  
> -		misc_deregister(&sbefifo->mdev);
> -		list_del(&sbefifo->link);
> -		ida_simple_remove(&sbefifo_ida, sbefifo->idx);
> +	misc_deregister(&sbefifo->mdev);
> +	device_for_each_child(dev, NULL, sbefifo_unregister_child);
>  
> -		if (del_timer_sync(&sbefifo->poll_timer))
> -			sbefifo_put(sbefifo);
> +	ida_simple_remove(&sbefifo_ida, sbefifo->idx);
>  
> -		spin_lock(&sbefifo->lock);
> -		list_for_each_entry(xfr, &sbefifo->xfrs, xfrs)
> -			kfree(xfr);
> -		spin_unlock(&sbefifo->lock);
> +	if (del_timer_sync(&sbefifo->poll_timer))
> +		sbefifo_put(sbefifo);
>  
> -		WRITE_ONCE(sbefifo->rc, -ENODEV);
> +	spin_lock(&sbefifo->lock);
> +	list_for_each_entry(xfr, &sbefifo->xfrs, xfrs)
> +		kfree(xfr);
> +	spin_unlock(&sbefifo->lock);

Do we actually need to lock on this? Shouldn't we have torn down anything that
might be concurrently accessing ->xfrs?

>  
> -		wake_up(&sbefifo->wait);
> -		sbefifo_put(sbefifo);
> -	}
> +	sbefifo_put(sbefifo);
>  
>  	return 0;
>  }
> @@ -915,17 +938,19 @@ static int sbefifo_remove(struct device *dev)
>  
>  static int sbefifo_init(void)
>  {
> -	INIT_LIST_HEAD(&sbefifo_fifos);
>  	return fsi_driver_register(&sbefifo_drv);
>  }
>  
>  static void sbefifo_exit(void)
>  {
>  	fsi_driver_unregister(&sbefifo_drv);
> +
> +	ida_destroy(&sbefifo_ida);
>  }
>  
>  module_init(sbefifo_init);
>  module_exit(sbefifo_exit);
>  MODULE_LICENSE("GPL");
>  MODULE_AUTHOR("Brad Bishop <bradleyb@fuzziesquirrel.com>");
> -MODULE_DESCRIPTION("Linux device interface to the POWER self boot engine");
> +MODULE_AUTHOR("Eddie James <eajames@linux.vnet.ibm.com>");
> +MODULE_DESCRIPTION("Linux device interface to the POWER Self Boot Engine");
> diff --git a/include/linux/fsi-sbefifo.h b/include/linux/fsi-sbefifo.h
> index 1b46c63..8e55891 100644
> --- a/include/linux/fsi-sbefifo.h
> +++ b/include/linux/fsi-sbefifo.h
> @@ -13,8 +13,8 @@
>   * GNU General Public License for more details.
>   */
>  
> -#ifndef __FSI_SBEFIFO_H__
> -#define __FSI_SBEFIFO_H__
> +#ifndef LINUX_FSI_SBEFIFO_H
> +#define LINUX_FSI_SBEFIFO_H
>  
>  struct device;
>  struct sbefifo_client;
> @@ -27,4 +27,4 @@ extern int sbefifo_drv_write(struct sbefifo_client *client, const char *buf,
>  			     size_t len);
>  extern void sbefifo_drv_release(struct sbefifo_client *client);
>  
> -#endif /* __FSI_SBEFIFO_H__ */
> +#endif /* LINUX_FSI_SBEFIFO_H */

Cheers,

Andrew
Andrew Jeffery Sept. 25, 2017, 5:46 a.m. UTC | #3 
On Mon, 2017-09-25 at 14:27 +0930, Andrew Jeffery wrote:
> >  static int sbefifo_remove(struct device *dev)
> >  {
> > -     struct fsi_device *fsi_dev = to_fsi_dev(dev);
> > -     struct sbefifo *sbefifo, *sbefifo_tmp;
> > +     struct sbefifo *sbefifo = dev_get_drvdata(dev);
> >       struct sbefifo_xfr *xfr;
> >  
> > -     list_for_each_entry_safe(sbefifo, sbefifo_tmp, &sbefifo_fifos, link) {
> > -             if (sbefifo->fsi_dev != fsi_dev)
> > -                     continue;
> > -
> > -             device_for_each_child(dev, NULL, sbefifo_unregister_child);
> > +     WRITE_ONCE(sbefifo->rc, -ENODEV);
> > +     wake_up(&sbefifo->wait);

Also, do we want wake_up_all() here? Or does it not matter (exclusive
vs non-exclusive)?
Andrew Jeffery Sept. 25, 2017, 6:16 a.m. UTC | #4 
Hi, it's me again.

On Mon, 2017-09-25 at 14:27 +0930, Andrew Jeffery wrote:
> >  static int sbefifo_remove(struct device *dev)
> >  {
> > -     struct fsi_device *fsi_dev = to_fsi_dev(dev);
> > -     struct sbefifo *sbefifo, *sbefifo_tmp;
> > +     struct sbefifo *sbefifo = dev_get_drvdata(dev);
> >       struct sbefifo_xfr *xfr;
> >  
> > -     list_for_each_entry_safe(sbefifo, sbefifo_tmp, &sbefifo_fifos, link) {
> > -             if (sbefifo->fsi_dev != fsi_dev)
> > -                     continue;
> > -
> > -             device_for_each_child(dev, NULL, sbefifo_unregister_child);
> > +     WRITE_ONCE(sbefifo->rc, -ENODEV);
> > +     wake_up(&sbefifo->wait);
> >  
> > -             misc_deregister(&sbefifo->mdev);
> > -             list_del(&sbefifo->link);
> > -             ida_simple_remove(&sbefifo_ida, sbefifo->idx);
> > +     misc_deregister(&sbefifo->mdev);

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

> > +     device_for_each_child(dev, NULL, sbefifo_unregister_child);
> >  
> > -             if (del_timer_sync(&sbefifo->poll_timer))
> > -                     sbefifo_put(sbefifo);
> > +     ida_simple_remove(&sbefifo_ida, sbefifo->idx);
> >  
> > -             spin_lock(&sbefifo->lock);
> > -             list_for_each_entry(xfr, &sbefifo->xfrs, xfrs)
> > -                     kfree(xfr);
> > -             spin_unlock(&sbefifo->lock);
> > +     if (del_timer_sync(&sbefifo->poll_timer))
> > +             sbefifo_put(sbefifo);

Shouldn't we be doing this before WRITE_ONCE(sbefifo->rc, -ENODEV) to
ensure we avoid a race on the value of ->rc? Otherwise the timer could
fire between the WRITE_ONCE() and here and potentially write ->rc or
cause another job to hang in the FIFO.
Eddie James Sept. 29, 2017, 10:44 p.m. UTC | #5 
On 09/25/2017 01:16 AM, Andrew Jeffery wrote:
> Hi, it's me again.
>
> On Mon, 2017-09-25 at 14:27 +0930, Andrew Jeffery wrote:
>>>   static int sbefifo_remove(struct device *dev)
>>>   {
>>> -     struct fsi_device *fsi_dev = to_fsi_dev(dev);
>>> -     struct sbefifo *sbefifo, *sbefifo_tmp;
>>> +     struct sbefifo *sbefifo = dev_get_drvdata(dev);
>>>        struct sbefifo_xfr *xfr;
>>>   
>>> -     list_for_each_entry_safe(sbefifo, sbefifo_tmp, &sbefifo_fifos, link) {
>>> -             if (sbefifo->fsi_dev != fsi_dev)
>>> -                     continue;
>>> -
>>> -             device_for_each_child(dev, NULL, sbefifo_unregister_child);
>>> +     WRITE_ONCE(sbefifo->rc, -ENODEV);
>>> +     wake_up(&sbefifo->wait);
>>>   
>>> -             misc_deregister(&sbefifo->mdev);
>>> -             list_del(&sbefifo->link);
>>> -             ida_simple_remove(&sbefifo_ida, sbefifo->idx);
>>> +     misc_deregister(&sbefifo->mdev);
> I think we should do this before the wake_up() to ensure that we wake
> all the submitted jobs. Otherwise we might miss some.

Problem is that if the child devices (occ, for example) don't have a 
cancel function, then we would hang waiting for stuff to wake up in 
their remove() functions. So I think we need to set rc and wake_up 
first, then start unregistering clients.

>
>>> +     device_for_each_child(dev, NULL, sbefifo_unregister_child);
>>>   
>>> -             if (del_timer_sync(&sbefifo->poll_timer))
>>> -                     sbefifo_put(sbefifo);
>>> +     ida_simple_remove(&sbefifo_ida, sbefifo->idx);
>>>   
>>> -             spin_lock(&sbefifo->lock);
>>> -             list_for_each_entry(xfr, &sbefifo->xfrs, xfrs)
>>> -                     kfree(xfr);
>>> -             spin_unlock(&sbefifo->lock);
>>> +     if (del_timer_sync(&sbefifo->poll_timer))
>>> +             sbefifo_put(sbefifo);
> Shouldn't we be doing this before WRITE_ONCE(sbefifo->rc, -ENODEV) to
> ensure we avoid a race on the value of ->rc? Otherwise the timer could
> fire between the WRITE_ONCE() and here and potentially write ->rc or
> cause another job to hang in the FIFO.

rc is only ever written with error values, fortunately, so I'm not too 
concerned about it.

Thanks,
Eddie
diff mbox series

Patch

diff --git a/drivers/fsi/fsi-sbefifo.c b/drivers/fsi/fsi-sbefifo.c
index 1c37ff7..5d25ade 100644
--- a/drivers/fsi/fsi-sbefifo.c
+++ b/drivers/fsi/fsi-sbefifo.c
@@ -11,20 +11,27 @@ 
  * GNU General Public License for more details.
  */
 
-#include <linux/delay.h>
+#include <linux/device.h>
 #include <linux/errno.h>
-#include <linux/idr.h>
+#include <linux/fs.h>
 #include <linux/fsi.h>
+#include <linux/fsi-sbefifo.h>
+#include <linux/idr.h>
+#include <linux/kernel.h>
+#include <linux/kref.h>
 #include <linux/list.h>
 #include <linux/miscdevice.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/of_device.h>
 #include <linux/of_platform.h>
 #include <linux/poll.h>
 #include <linux/sched.h>
 #include <linux/slab.h>
+#include <linux/spinlock.h>
 #include <linux/timer.h>
 #include <linux/uaccess.h>
+#include <linux/wait.h>
 
 /*
  * The SBEFIFO is a pipe-like FSI device for communicating with
@@ -44,14 +51,15 @@ 
 #define   SBEFIFO_EOT_MAGIC		0xffffffff
 #define SBEFIFO_EOT_ACK		0x14
 
+#define SBEFIFO_RESCHEDULE	msecs_to_jiffies(500)
+
 struct sbefifo {
 	struct timer_list poll_timer;
-	struct fsi_device *fsi_dev;
-	struct miscdevice mdev;
+	struct fsi_device *fsi_dev;	/* parent fsi device */
+	struct miscdevice mdev;		/* /dev/ entry */
 	wait_queue_head_t wait;
-	struct list_head link;
 	struct list_head xfrs;
-	struct kref kref;
+	struct kref kref;		/* reference counter */
 	spinlock_t lock;
 	char name[32];
 	int idx;
@@ -87,14 +95,12 @@  struct sbefifo_client {
 	unsigned long f_flags;
 };
 
-static struct list_head sbefifo_fifos;
-
 static DEFINE_IDA(sbefifo_ida);
 
 static int sbefifo_inw(struct sbefifo *sbefifo, int reg, u32 *word)
 {
 	int rc;
-	u32 raw_word;
+	__be32 raw_word;
 
 	rc = fsi_device_read(sbefifo->fsi_dev, reg, &raw_word,
 			     sizeof(raw_word));
@@ -102,17 +108,19 @@  static int sbefifo_inw(struct sbefifo *sbefifo, int reg, u32 *word)
 		return rc;
 
 	*word = be32_to_cpu(raw_word);
+
 	return 0;
 }
 
 static int sbefifo_outw(struct sbefifo *sbefifo, int reg, u32 word)
 {
-	u32 raw_word = cpu_to_be32(word);
+	__be32 raw_word = cpu_to_be32(word);
 
 	return fsi_device_write(sbefifo->fsi_dev, reg, &raw_word,
 				sizeof(raw_word));
 }
 
+/* Don't flip endianness of data to/from FIFO, just pass through. */
 static int sbefifo_readw(struct sbefifo *sbefifo, u32 *word)
 {
 	return fsi_device_read(sbefifo->fsi_dev, SBEFIFO_DWN, word,
@@ -136,7 +144,7 @@  static int sbefifo_ack_eot(struct sbefifo *sbefifo)
 		return ret;
 
 	return sbefifo_outw(sbefifo, SBEFIFO_DWN | SBEFIFO_EOT_ACK,
-			SBEFIFO_EOT_MAGIC);
+			    SBEFIFO_EOT_MAGIC);
 }
 
 static size_t sbefifo_dev_nwreadable(u32 sts)
@@ -210,6 +218,7 @@  static bool sbefifo_buf_readnb(struct sbefifo_buf *buf, size_t n)
 		rpos = buf->buf;
 
 	WRITE_ONCE(buf->rpos, rpos);
+
 	return rpos == wpos;
 }
 
@@ -229,14 +238,14 @@  static bool sbefifo_buf_wrotenb(struct sbefifo_buf *buf, size_t n)
 		set_bit(SBEFIFO_BUF_FULL, &buf->flags);
 
 	WRITE_ONCE(buf->wpos, wpos);
+
 	return rpos == wpos;
 }
 
 static void sbefifo_free(struct kref *kref)
 {
-	struct sbefifo *sbefifo;
+	struct sbefifo *sbefifo = container_of(kref, struct sbefifo, kref);
 
-	sbefifo = container_of(kref, struct sbefifo, kref);
 	kfree(sbefifo);
 }
 
@@ -255,9 +264,12 @@  static struct sbefifo_xfr *sbefifo_enq_xfr(struct sbefifo_client *client)
 	struct sbefifo *sbefifo = client->dev;
 	struct sbefifo_xfr *xfr;
 
+	if (READ_ONCE(sbefifo->rc))
+		return ERR_PTR(sbefifo->rc);
+
 	xfr = kzalloc(sizeof(*xfr), GFP_KERNEL);
 	if (!xfr)
-		return NULL;
+		return ERR_PTR(-ENOMEM);
 
 	xfr->rbuf = &client->rbuf;
 	xfr->wbuf = &client->wbuf;
@@ -267,21 +279,12 @@  static struct sbefifo_xfr *sbefifo_enq_xfr(struct sbefifo_client *client)
 	return xfr;
 }
 
-static struct sbefifo_xfr *sbefifo_client_next_xfr(
-		struct sbefifo_client *client)
-{
-	if (list_empty(&client->xfrs))
-		return NULL;
-
-	return container_of(client->xfrs.next, struct sbefifo_xfr,
-			client);
-}
-
 static bool sbefifo_xfr_rsp_pending(struct sbefifo_client *client)
 {
-	struct sbefifo_xfr *xfr;
+	struct sbefifo_xfr *xfr = list_first_entry_or_null(&client->xfrs,
+							   struct sbefifo_xfr,
+							   client);
 
-	xfr = sbefifo_client_next_xfr(client);
 	if (xfr && test_bit(SBEFIFO_XFR_RESP_PENDING, &xfr->flags))
 		return true;
 
@@ -309,10 +312,11 @@  static struct sbefifo_client *sbefifo_new_client(struct sbefifo *sbefifo)
 
 static void sbefifo_client_release(struct kref *kref)
 {
-	struct sbefifo_client *client;
 	struct sbefifo_xfr *xfr;
+	struct sbefifo_client *client = container_of(kref,
+						     struct sbefifo_client,
+						     kref);
 
-	client = container_of(kref, struct sbefifo_client, kref);
 	list_for_each_entry(xfr, &client->xfrs, client) {
 		/*
 		 * The client left with pending or running xfrs.
@@ -349,6 +353,7 @@  static struct sbefifo_xfr *sbefifo_next_xfr(struct sbefifo *sbefifo)
 			kfree(xfr);
 			continue;
 		}
+
 		return xfr;
 	}
 
@@ -370,7 +375,7 @@  static void sbefifo_poll_timer(unsigned long data)
 
 	spin_lock(&sbefifo->lock);
 	xfr = list_first_entry_or_null(&sbefifo->xfrs, struct sbefifo_xfr,
-			xfrs);
+				       xfrs);
 	if (!xfr)
 		goto out_unlock;
 
@@ -388,8 +393,7 @@  static void sbefifo_poll_timer(unsigned long data)
 
 	 /* Drain the write buffer. */
 	while ((bufn = sbefifo_buf_nbreadable(wbuf))) {
-		ret = sbefifo_inw(sbefifo, SBEFIFO_UP | SBEFIFO_STS,
-				&sts);
+		ret = sbefifo_inw(sbefifo, SBEFIFO_UP | SBEFIFO_STS, &sts);
 		if (ret)
 			goto out;
 
@@ -397,7 +401,7 @@  static void sbefifo_poll_timer(unsigned long data)
 		if (devn == 0) {
 			/* No open slot for write.  Reschedule. */
 			sbefifo->poll_timer.expires = jiffies +
-				msecs_to_jiffies(500);
+				SBEFIFO_RESCHEDULE;
 			add_timer(&sbefifo->poll_timer);
 			goto out_unlock;
 		}
@@ -414,9 +418,8 @@  static void sbefifo_poll_timer(unsigned long data)
 
 	 /* Send EOT if the writer is finished. */
 	if (test_and_clear_bit(SBEFIFO_XFR_WRITE_DONE, &xfr->flags)) {
-		ret = sbefifo_outw(sbefifo,
-				SBEFIFO_UP | SBEFIFO_EOT_RAISE,
-				SBEFIFO_EOT_MAGIC);
+		ret = sbefifo_outw(sbefifo, SBEFIFO_UP | SBEFIFO_EOT_RAISE,
+				   SBEFIFO_EOT_MAGIC);
 		if (ret)
 			goto out;
 
@@ -438,7 +441,7 @@  static void sbefifo_poll_timer(unsigned long data)
 		if (devn == 0) {
 			/* No data yet.  Reschedule. */
 			sbefifo->poll_timer.expires = jiffies +
-				msecs_to_jiffies(500);
+				SBEFIFO_RESCHEDULE;
 			add_timer(&sbefifo->poll_timer);
 			goto out_unlock;
 		}
@@ -466,7 +469,7 @@  static void sbefifo_poll_timer(unsigned long data)
 			set_bit(SBEFIFO_XFR_COMPLETE, &xfr->flags);
 			list_del(&xfr->xfrs);
 			if (unlikely(test_bit(SBEFIFO_XFR_CANCEL,
-							&xfr->flags)))
+					      &xfr->flags)))
 				kfree(xfr);
 			break;
 		}
@@ -476,7 +479,7 @@  static void sbefifo_poll_timer(unsigned long data)
 	if (unlikely(ret)) {
 		sbefifo->rc = ret;
 		dev_err(&sbefifo->fsi_dev->dev,
-				"Fatal bus access failure: %d\n", ret);
+			"Fatal bus access failure: %d\n", ret);
 		list_for_each_entry(xfr, &sbefifo->xfrs, xfrs)
 			kfree(xfr);
 		INIT_LIST_HEAD(&sbefifo->xfrs);
@@ -497,7 +500,7 @@  static void sbefifo_poll_timer(unsigned long data)
 static int sbefifo_open(struct inode *inode, struct file *file)
 {
 	struct sbefifo *sbefifo = container_of(file->private_data,
-			struct sbefifo, mdev);
+					       struct sbefifo, mdev);
 	struct sbefifo_client *client;
 	int ret;
 
@@ -535,6 +538,19 @@  static unsigned int sbefifo_poll(struct file *file, poll_table *wait)
 	return mask;
 }
 
+static bool sbefifo_read_ready(struct sbefifo *sbefifo,
+			       struct sbefifo_client *client, size_t *n)
+{
+	struct sbefifo_xfr *xfr = list_first_entry_or_null(&client->xfrs,
+							   struct sbefifo_xfr,
+							   client);
+
+	*n = sbefifo_buf_nbreadable(&client->rbuf);
+
+	return READ_ONCE(sbefifo->rc) || *n ||
+		(xfr && test_bit(SBEFIFO_XFR_COMPLETE, &xfr->flags));
+}
+
 static ssize_t sbefifo_read_common(struct sbefifo_client *client,
 				   char __user *ubuf, char *kbuf, size_t len)
 {
@@ -551,30 +567,38 @@  static ssize_t sbefifo_read_common(struct sbefifo_client *client,
 
 	sbefifo_get_client(client);
 	if (wait_event_interruptible(sbefifo->wait,
-				(ret = READ_ONCE(sbefifo->rc)) ||
-				(n = sbefifo_buf_nbreadable(
-						&client->rbuf)))) {
-		sbefifo_put_client(client);
-		return -ERESTARTSYS;
+				     sbefifo_read_ready(sbefifo, client,
+							&n))) {
+		ret = -ERESTARTSYS;
+		goto out;
 	}
+
+	ret = READ_ONCE(sbefifo->rc);
 	if (ret) {
 		INIT_LIST_HEAD(&client->xfrs);
-		sbefifo_put_client(client);
-		return ret;
+		goto out;
 	}
 
 	n = min_t(size_t, n, len);
 
 	if (ubuf) {
 		if (copy_to_user(ubuf, READ_ONCE(client->rbuf.rpos), n)) {
-			sbefifo_put_client(client);
-			return -EFAULT;
+			ret = -EFAULT;
+			goto out;
 		}
-	} else
+	} else {
 		memcpy(kbuf, READ_ONCE(client->rbuf.rpos), n);
+	}
 
 	if (sbefifo_buf_readnb(&client->rbuf, n)) {
-		xfr = sbefifo_client_next_xfr(client);
+		xfr = list_first_entry_or_null(&client->xfrs,
+					       struct sbefifo_xfr, client);
+		if (!xfr) {
+			/* should be impossible to not have an xfr here */
+			wake_up(&sbefifo->wait);
+			goto out;
+		}
+
 		if (!test_bit(SBEFIFO_XFR_COMPLETE, &xfr->flags)) {
 			/*
 			 * Fill the read buffer back up.
@@ -589,22 +613,31 @@  static ssize_t sbefifo_read_common(struct sbefifo_client *client,
 		}
 	}
 
-	sbefifo_put_client(client);
+	ret = n;
 
-	return n;
+out:
+	sbefifo_put_client(client);
+	return ret;
 }
 
-static ssize_t sbefifo_read(struct file *file, char __user *buf,
-		size_t len, loff_t *offset)
+static ssize_t sbefifo_read(struct file *file, char __user *buf, size_t len,
+			    loff_t *offset)
 {
 	struct sbefifo_client *client = file->private_data;
 
-	WARN_ON(*offset);
+	return sbefifo_read_common(client, buf, NULL, len);
+}
 
-	if (!access_ok(VERIFY_WRITE, buf, len))
-		return -EFAULT;
+static bool sbefifo_write_ready(struct sbefifo *sbefifo,
+				struct sbefifo_xfr *xfr,
+				struct sbefifo_client *client, size_t *n)
+{
+	struct sbefifo_xfr *next = list_first_entry_or_null(&client->xfrs,
+							    struct sbefifo_xfr,
+							    client);
 
-	return sbefifo_read_common(client, buf, NULL, len);
+	*n = sbefifo_buf_nbwriteable(&client->wbuf);
+	return READ_ONCE(sbefifo->rc) || (next == xfr && *n);
 }
 
 static ssize_t sbefifo_write_common(struct sbefifo_client *client,
@@ -612,6 +645,7 @@  static ssize_t sbefifo_write_common(struct sbefifo_client *client,
 				    size_t len)
 {
 	struct sbefifo *sbefifo = client->dev;
+	struct sbefifo_buf *wbuf = &client->wbuf;
 	struct sbefifo_xfr *xfr;
 	ssize_t ret = 0;
 	size_t n;
@@ -622,24 +656,26 @@  static ssize_t sbefifo_write_common(struct sbefifo_client *client,
 	if (!len)
 		return 0;
 
-	n = sbefifo_buf_nbwriteable(&client->wbuf);
+	sbefifo_get_client(client);
+	n = sbefifo_buf_nbwriteable(wbuf);
 
 	spin_lock_irq(&sbefifo->lock);
-	xfr = sbefifo_next_xfr(sbefifo);
 
+	xfr = sbefifo_next_xfr(sbefifo);	/* next xfr to be executed */
 	if ((client->f_flags & O_NONBLOCK) && xfr && n < len) {
 		spin_unlock_irq(&sbefifo->lock);
-		return -EAGAIN;
+		ret = -EAGAIN;
+		goto out;
 	}
 
-	xfr = sbefifo_enq_xfr(client);
-	if (!xfr) {
+	xfr = sbefifo_enq_xfr(client);		/* this xfr queued up */
+	if (IS_ERR(xfr)) {
 		spin_unlock_irq(&sbefifo->lock);
-		return -ENOMEM;
+		ret = PTR_ERR(xfr);
+		goto out;
 	}
-	spin_unlock_irq(&sbefifo->lock);
 
-	sbefifo_get_client(client);
+	spin_unlock_irq(&sbefifo->lock);
 
 	/*
 	 * Partial writes are not really allowed in that EOT is sent exactly
@@ -647,49 +683,47 @@  static ssize_t sbefifo_write_common(struct sbefifo_client *client,
 	 */
 	while (len) {
 		if (wait_event_interruptible(sbefifo->wait,
-				READ_ONCE(sbefifo->rc) ||
-					(sbefifo_client_next_xfr(client) == xfr &&
-					 (n = sbefifo_buf_nbwriteable(
-							&client->wbuf))))) {
+					     sbefifo_write_ready(sbefifo, xfr,
+								 client,
+								 &n))) {
 			set_bit(SBEFIFO_XFR_CANCEL, &xfr->flags);
 			sbefifo_get(sbefifo);
 			if (mod_timer(&sbefifo->poll_timer, jiffies))
 				sbefifo_put(sbefifo);
-
-			sbefifo_put_client(client);
-			return -ERESTARTSYS;
+			ret = -ERESTARTSYS;
+			goto out;
 		}
-		if (sbefifo->rc) {
+
+		ret = READ_ONCE(sbefifo->rc);
+		if (ret) {
 			INIT_LIST_HEAD(&client->xfrs);
-			sbefifo_put_client(client);
-			return sbefifo->rc;
+			goto out;
 		}
 
 		n = min_t(size_t, n, len);
 
 		if (ubuf) {
-			if (copy_from_user(READ_ONCE(client->wbuf.wpos), ubuf,
-			    n)) {
+			if (copy_from_user(READ_ONCE(wbuf->wpos), ubuf, n)) {
 				set_bit(SBEFIFO_XFR_CANCEL, &xfr->flags);
 				sbefifo_get(sbefifo);
 				if (mod_timer(&sbefifo->poll_timer, jiffies))
 					sbefifo_put(sbefifo);
-				sbefifo_put_client(client);
-				return -EFAULT;
+				ret = -EFAULT;
+				goto out;
 			}
 
 			ubuf += n;
 		} else {
-			memcpy(READ_ONCE(client->wbuf.wpos), kbuf, n);
+			memcpy(READ_ONCE(wbuf->wpos), kbuf, n);
 			kbuf += n;
 		}
 
-		sbefifo_buf_wrotenb(&client->wbuf, n);
+		sbefifo_buf_wrotenb(wbuf, n);
 		len -= n;
 		ret += n;
 
-		/* set flag before starting the worker, as it may run through
-		 * and check the flag before we exit this loop!
+		/* Set this before starting timer to avoid race condition on
+		 * this flag with the timer function writer.
 		 */
 		if (!len)
 			set_bit(SBEFIFO_XFR_WRITE_DONE, &xfr->flags);
@@ -702,21 +736,16 @@  static ssize_t sbefifo_write_common(struct sbefifo_client *client,
 			sbefifo_put(sbefifo);
 	}
 
+out:
 	sbefifo_put_client(client);
-
 	return ret;
 }
 
 static ssize_t sbefifo_write(struct file *file, const char __user *buf,
-		size_t len, loff_t *offset)
+			     size_t len, loff_t *offset)
 {
 	struct sbefifo_client *client = file->private_data;
 
-	WARN_ON(*offset);
-
-	if (!access_ok(VERIFY_READ, buf, len))
-		return -EFAULT;
-
 	return sbefifo_write_common(client, buf, NULL, len);
 }
 
@@ -742,18 +771,15 @@  static int sbefifo_release(struct inode *inode, struct file *file)
 struct sbefifo_client *sbefifo_drv_open(struct device *dev,
 					unsigned long flags)
 {
-	struct sbefifo_client *client = NULL;
-	struct sbefifo *sbefifo;
-	struct fsi_device *fsi_dev = to_fsi_dev(dev);
+	struct sbefifo_client *client;
+	struct sbefifo *sbefifo = dev_get_drvdata(dev);
 
-	list_for_each_entry(sbefifo, &sbefifo_fifos, link) {
-		if (sbefifo->fsi_dev != fsi_dev)
-			continue;
+	if (!sbefifo)
+		return NULL;
 
-		client = sbefifo_new_client(sbefifo);
-		if (client)
-			client->f_flags = flags;
-	}
+	client = sbefifo_new_client(sbefifo);
+	if (client)
+		client->f_flags = flags;
 
 	return client;
 }
@@ -802,95 +828,92 @@  static int sbefifo_probe(struct device *dev)
 	u32 sts;
 	int ret, child_idx = 0;
 
-	dev_dbg(dev, "Found sbefifo device\n");
 	sbefifo = kzalloc(sizeof(*sbefifo), GFP_KERNEL);
 	if (!sbefifo)
 		return -ENOMEM;
 
 	sbefifo->fsi_dev = fsi_dev;
 
-	ret = sbefifo_inw(sbefifo,
-			SBEFIFO_UP | SBEFIFO_STS, &sts);
+	ret = sbefifo_inw(sbefifo, SBEFIFO_UP | SBEFIFO_STS, &sts);
 	if (ret)
 		return ret;
+
 	if (!(sts & SBEFIFO_EMPTY)) {
-		dev_err(&sbefifo->fsi_dev->dev,
-				"Found data in upstream fifo\n");
+		dev_err(dev, "Found data in upstream fifo\n");
 		return -EIO;
 	}
 
 	ret = sbefifo_inw(sbefifo, SBEFIFO_DWN | SBEFIFO_STS, &sts);
 	if (ret)
 		return ret;
+
 	if (!(sts & SBEFIFO_EMPTY)) {
-		dev_err(&sbefifo->fsi_dev->dev,
-				"Found data in downstream fifo\n");
+		dev_err(dev, "found data in downstream fifo\n");
 		return -EIO;
 	}
 
-	sbefifo->mdev.minor = MISC_DYNAMIC_MINOR;
-	sbefifo->mdev.fops = &sbefifo_fops;
-	sbefifo->mdev.name = sbefifo->name;
-	sbefifo->mdev.parent = dev;
 	spin_lock_init(&sbefifo->lock);
 	kref_init(&sbefifo->kref);
+	init_waitqueue_head(&sbefifo->wait);
+	INIT_LIST_HEAD(&sbefifo->xfrs);
 
 	sbefifo->idx = ida_simple_get(&sbefifo_ida, 1, INT_MAX, GFP_KERNEL);
 	snprintf(sbefifo->name, sizeof(sbefifo->name), "sbefifo%d",
-			sbefifo->idx);
-	init_waitqueue_head(&sbefifo->wait);
-	INIT_LIST_HEAD(&sbefifo->xfrs);
+		 sbefifo->idx);
 
 	/* This bit of silicon doesn't offer any interrupts... */
 	setup_timer(&sbefifo->poll_timer, sbefifo_poll_timer,
-			(unsigned long)sbefifo);
-
-	if (dev->of_node) {
-		/* create platform devs for dts child nodes (occ, etc) */
-		for_each_child_of_node(dev->of_node, np) {
-			snprintf(child_name, sizeof(child_name), "%s-dev%d",
-				 sbefifo->name, child_idx++);
-			child = of_platform_device_create(np, child_name, dev);
-			if (!child)
-				dev_warn(&sbefifo->fsi_dev->dev,
-					 "failed to create child node dev\n");
-		}
+		    (unsigned long)sbefifo);
+
+	sbefifo->mdev.minor = MISC_DYNAMIC_MINOR;
+	sbefifo->mdev.fops = &sbefifo_fops;
+	sbefifo->mdev.name = sbefifo->name;
+	sbefifo->mdev.parent = dev;
+	ret = misc_register(&sbefifo->mdev);
+	if (ret) {
+		dev_err(dev, "failed to register miscdevice: %d\n", ret);
+		ida_simple_remove(&sbefifo_ida, sbefifo->idx);
+		sbefifo_put(sbefifo);
+		return ret;
+	}
+
+	/* create platform devs for dts child nodes (occ, etc) */
+	for_each_available_child_of_node(dev->of_node, np) {
+		snprintf(child_name, sizeof(child_name), "%s-dev%d",
+			 sbefifo->name, child_idx++);
+		child = of_platform_device_create(np, child_name, dev);
+		if (!child)
+			dev_warn(dev, "failed to create child %s dev\n",
+				 child_name);
 	}
 
-	list_add(&sbefifo->link, &sbefifo_fifos);
-	
-	return misc_register(&sbefifo->mdev);
+	dev_set_drvdata(dev, sbefifo);
+
+	return 0;
 }
 
 static int sbefifo_remove(struct device *dev)
 {
-	struct fsi_device *fsi_dev = to_fsi_dev(dev);
-	struct sbefifo *sbefifo, *sbefifo_tmp;
+	struct sbefifo *sbefifo = dev_get_drvdata(dev);
 	struct sbefifo_xfr *xfr;
 
-	list_for_each_entry_safe(sbefifo, sbefifo_tmp, &sbefifo_fifos, link) {
-		if (sbefifo->fsi_dev != fsi_dev)
-			continue;
-
-		device_for_each_child(dev, NULL, sbefifo_unregister_child);
+	WRITE_ONCE(sbefifo->rc, -ENODEV);
+	wake_up(&sbefifo->wait);
 
-		misc_deregister(&sbefifo->mdev);
-		list_del(&sbefifo->link);
-		ida_simple_remove(&sbefifo_ida, sbefifo->idx);
+	misc_deregister(&sbefifo->mdev);
+	device_for_each_child(dev, NULL, sbefifo_unregister_child);
 
-		if (del_timer_sync(&sbefifo->poll_timer))
-			sbefifo_put(sbefifo);
+	ida_simple_remove(&sbefifo_ida, sbefifo->idx);
 
-		spin_lock(&sbefifo->lock);
-		list_for_each_entry(xfr, &sbefifo->xfrs, xfrs)
-			kfree(xfr);
-		spin_unlock(&sbefifo->lock);
+	if (del_timer_sync(&sbefifo->poll_timer))
+		sbefifo_put(sbefifo);
 
-		WRITE_ONCE(sbefifo->rc, -ENODEV);
+	spin_lock(&sbefifo->lock);
+	list_for_each_entry(xfr, &sbefifo->xfrs, xfrs)
+		kfree(xfr);
+	spin_unlock(&sbefifo->lock);
 
-		wake_up(&sbefifo->wait);
-		sbefifo_put(sbefifo);
-	}
+	sbefifo_put(sbefifo);
 
 	return 0;
 }
@@ -915,17 +938,19 @@  static int sbefifo_remove(struct device *dev)
 
 static int sbefifo_init(void)
 {
-	INIT_LIST_HEAD(&sbefifo_fifos);
 	return fsi_driver_register(&sbefifo_drv);
 }
 
 static void sbefifo_exit(void)
 {
 	fsi_driver_unregister(&sbefifo_drv);
+
+	ida_destroy(&sbefifo_ida);
 }
 
 module_init(sbefifo_init);
 module_exit(sbefifo_exit);
 MODULE_LICENSE("GPL");
 MODULE_AUTHOR("Brad Bishop <bradleyb@fuzziesquirrel.com>");
-MODULE_DESCRIPTION("Linux device interface to the POWER self boot engine");
+MODULE_AUTHOR("Eddie James <eajames@linux.vnet.ibm.com>");
+MODULE_DESCRIPTION("Linux device interface to the POWER Self Boot Engine");
diff --git a/include/linux/fsi-sbefifo.h b/include/linux/fsi-sbefifo.h
index 1b46c63..8e55891 100644
--- a/include/linux/fsi-sbefifo.h
+++ b/include/linux/fsi-sbefifo.h
@@ -13,8 +13,8 @@ 
  * GNU General Public License for more details.
  */
 
-#ifndef __FSI_SBEFIFO_H__
-#define __FSI_SBEFIFO_H__
+#ifndef LINUX_FSI_SBEFIFO_H
+#define LINUX_FSI_SBEFIFO_H
 
 struct device;
 struct sbefifo_client;
@@ -27,4 +27,4 @@  extern int sbefifo_drv_write(struct sbefifo_client *client, const char *buf,
 			     size_t len);
 extern void sbefifo_drv_release(struct sbefifo_client *client);
 
-#endif /* __FSI_SBEFIFO_H__ */
+#endif /* LINUX_FSI_SBEFIFO_H */