[{"id":1773165,"web_url":"http://patchwork.ozlabs.org/comment/1773165/","msgid":"","list_archive_url":null,"date":"2017-09-21T22:55:21","subject":"Re: [PATCH linux dev-4.10 1/3] drivers/fsi/sbefifo: refactor to\n\tupstream list state","submitter":{"id":70876,"url":"http://patchwork.ozlabs.org/api/people/70876/","name":"Eddie James","email":"eajames@linux.vnet.ibm.com"},"content":"On 09/21/2017 05:43 PM, Eddie James wrote:\n> From: \"Edward A. James\" \n>\n> Includes various fixes:\n> - check for complete while waiting for data in read()\n> - fix probe if probe fails\n> - general cleanup\n> - slightly safer (earlier) get_client()\n> - reorder some of the remove() operations for safety\n\nTo clarify, this hasn't actually been sent upstream yet. But no changes \nplanned to send this upstream.\n\nThanks,\nEddie\n\n>\n> Signed-off-by: Edward A. James \n> ---\n> drivers/fsi/fsi-sbefifo.c | 329 ++++++++++++++++++++++++--------------------\n> include/linux/fsi-sbefifo.h | 6 +-\n> 2 files changed, 180 insertions(+), 155 deletions(-)\n>\n> diff --git a/drivers/fsi/fsi-sbefifo.c b/drivers/fsi/fsi-sbefifo.c\n> index 1c37ff7..5d25ade 100644\n> --- a/drivers/fsi/fsi-sbefifo.c\n> +++ b/drivers/fsi/fsi-sbefifo.c\n> @@ -11,20 +11,27 @@\n> * GNU General Public License for more details.\n> */\n>\n> -#include \n> +#include \n> #include \n> -#include \n> +#include \n> #include \n> +#include \n> +#include \n> +#include \n> +#include \n> #include \n> #include \n> #include \n> #include \n> +#include \n> #include \n> #include \n> #include \n> #include \n> +#include \n> #include \n> #include \n> +#include \n>\n> /*\n> * The SBEFIFO is a pipe-like FSI device for communicating with\n> @@ -44,14 +51,15 @@\n> #define SBEFIFO_EOT_MAGIC\t\t0xffffffff\n> #define SBEFIFO_EOT_ACK\t\t0x14\n>\n> +#define SBEFIFO_RESCHEDULE\tmsecs_to_jiffies(500)\n> +\n> struct sbefifo {\n> \tstruct timer_list poll_timer;\n> -\tstruct fsi_device *fsi_dev;\n> -\tstruct miscdevice mdev;\n> +\tstruct fsi_device *fsi_dev;\t/* parent fsi device */\n> +\tstruct miscdevice mdev;\t\t/* /dev/ entry */\n> \twait_queue_head_t wait;\n> -\tstruct list_head link;\n> \tstruct list_head xfrs;\n> -\tstruct kref kref;\n> +\tstruct kref kref;\t\t/* reference counter */\n> \tspinlock_t lock;\n> \tchar name[32];\n> \tint idx;\n> @@ -87,14 +95,12 @@ struct sbefifo_client {\n> \tunsigned long f_flags;\n> };\n>\n> -static struct list_head sbefifo_fifos;\n> -\n> static DEFINE_IDA(sbefifo_ida);\n>\n> static int sbefifo_inw(struct sbefifo *sbefifo, int reg, u32 *word)\n> {\n> \tint rc;\n> -\tu32 raw_word;\n> +\t__be32 raw_word;\n>\n> \trc = fsi_device_read(sbefifo->fsi_dev, reg, &raw_word,\n> \t\t\t sizeof(raw_word));\n> @@ -102,17 +108,19 @@ static int sbefifo_inw(struct sbefifo *sbefifo, int reg, u32 *word)\n> \t\treturn rc;\n>\n> \t*word = be32_to_cpu(raw_word);\n> +\n> \treturn 0;\n> }\n>\n> static int sbefifo_outw(struct sbefifo *sbefifo, int reg, u32 word)\n> {\n> -\tu32 raw_word = cpu_to_be32(word);\n> +\t__be32 raw_word = cpu_to_be32(word);\n>\n> \treturn fsi_device_write(sbefifo->fsi_dev, reg, &raw_word,\n> \t\t\t\tsizeof(raw_word));\n> }\n>\n> +/* Don't flip endianness of data to/from FIFO, just pass through. */\n> static int sbefifo_readw(struct sbefifo *sbefifo, u32 *word)\n> {\n> \treturn fsi_device_read(sbefifo->fsi_dev, SBEFIFO_DWN, word,\n> @@ -136,7 +144,7 @@ static int sbefifo_ack_eot(struct sbefifo *sbefifo)\n> \t\treturn ret;\n>\n> \treturn sbefifo_outw(sbefifo, SBEFIFO_DWN | SBEFIFO_EOT_ACK,\n> -\t\t\tSBEFIFO_EOT_MAGIC);\n> +\t\t\t SBEFIFO_EOT_MAGIC);\n> }\n>\n> static size_t sbefifo_dev_nwreadable(u32 sts)\n> @@ -210,6 +218,7 @@ static bool sbefifo_buf_readnb(struct sbefifo_buf *buf, size_t n)\n> \t\trpos = buf->buf;\n>\n> \tWRITE_ONCE(buf->rpos, rpos);\n> +\n> \treturn rpos == wpos;\n> }\n>\n> @@ -229,14 +238,14 @@ static bool sbefifo_buf_wrotenb(struct sbefifo_buf *buf, size_t n)\n> \t\tset_bit(SBEFIFO_BUF_FULL, &buf->flags);\n>\n> \tWRITE_ONCE(buf->wpos, wpos);\n> +\n> \treturn rpos == wpos;\n> }\n>\n> static void sbefifo_free(struct kref *kref)\n> {\n> -\tstruct sbefifo *sbefifo;\n> +\tstruct sbefifo *sbefifo = container_of(kref, struct sbefifo, kref);\n>\n> -\tsbefifo = container_of(kref, struct sbefifo, kref);\n> \tkfree(sbefifo);\n> }\n>\n> @@ -255,9 +264,12 @@ static struct sbefifo_xfr *sbefifo_enq_xfr(struct sbefifo_client *client)\n> \tstruct sbefifo *sbefifo = client->dev;\n> \tstruct sbefifo_xfr *xfr;\n>\n> +\tif (READ_ONCE(sbefifo->rc))\n> +\t\treturn ERR_PTR(sbefifo->rc);\n> +\n> \txfr = kzalloc(sizeof(*xfr), GFP_KERNEL);\n> \tif (!xfr)\n> -\t\treturn NULL;\n> +\t\treturn ERR_PTR(-ENOMEM);\n>\n> \txfr->rbuf = &client->rbuf;\n> \txfr->wbuf = &client->wbuf;\n> @@ -267,21 +279,12 @@ static struct sbefifo_xfr *sbefifo_enq_xfr(struct sbefifo_client *client)\n> \treturn xfr;\n> }\n>\n> -static struct sbefifo_xfr *sbefifo_client_next_xfr(\n> -\t\tstruct sbefifo_client *client)\n> -{\n> -\tif (list_empty(&client->xfrs))\n> -\t\treturn NULL;\n> -\n> -\treturn container_of(client->xfrs.next, struct sbefifo_xfr,\n> -\t\t\tclient);\n> -}\n> -\n> static bool sbefifo_xfr_rsp_pending(struct sbefifo_client *client)\n> {\n> -\tstruct sbefifo_xfr *xfr;\n> +\tstruct sbefifo_xfr *xfr = list_first_entry_or_null(&client->xfrs,\n> +\t\t\t\t\t\t\t struct sbefifo_xfr,\n> +\t\t\t\t\t\t\t client);\n>\n> -\txfr = sbefifo_client_next_xfr(client);\n> \tif (xfr && test_bit(SBEFIFO_XFR_RESP_PENDING, &xfr->flags))\n> \t\treturn true;\n>\n> @@ -309,10 +312,11 @@ static struct sbefifo_client *sbefifo_new_client(struct sbefifo *sbefifo)\n>\n> static void sbefifo_client_release(struct kref *kref)\n> {\n> -\tstruct sbefifo_client *client;\n> \tstruct sbefifo_xfr *xfr;\n> +\tstruct sbefifo_client *client = container_of(kref,\n> +\t\t\t\t\t\t struct sbefifo_client,\n> +\t\t\t\t\t\t kref);\n>\n> -\tclient = container_of(kref, struct sbefifo_client, kref);\n> \tlist_for_each_entry(xfr, &client->xfrs, client) {\n> \t\t/*\n> \t\t * The client left with pending or running xfrs.\n> @@ -349,6 +353,7 @@ static struct sbefifo_xfr *sbefifo_next_xfr(struct sbefifo *sbefifo)\n> \t\t\tkfree(xfr);\n> \t\t\tcontinue;\n> \t\t}\n> +\n> \t\treturn xfr;\n> \t}\n>\n> @@ -370,7 +375,7 @@ static void sbefifo_poll_timer(unsigned long data)\n>\n> \tspin_lock(&sbefifo->lock);\n> \txfr = list_first_entry_or_null(&sbefifo->xfrs, struct sbefifo_xfr,\n> -\t\t\txfrs);\n> +\t\t\t\t xfrs);\n> \tif (!xfr)\n> \t\tgoto out_unlock;\n>\n> @@ -388,8 +393,7 @@ static void sbefifo_poll_timer(unsigned long data)\n>\n> \t /* Drain the write buffer. */\n> \twhile ((bufn = sbefifo_buf_nbreadable(wbuf))) {\n> -\t\tret = sbefifo_inw(sbefifo, SBEFIFO_UP | SBEFIFO_STS,\n> -\t\t\t\t&sts);\n> +\t\tret = sbefifo_inw(sbefifo, SBEFIFO_UP | SBEFIFO_STS, &sts);\n> \t\tif (ret)\n> \t\t\tgoto out;\n>\n> @@ -397,7 +401,7 @@ static void sbefifo_poll_timer(unsigned long data)\n> \t\tif (devn == 0) {\n> \t\t\t/* No open slot for write. Reschedule. */\n> \t\t\tsbefifo->poll_timer.expires = jiffies +\n> -\t\t\t\tmsecs_to_jiffies(500);\n> +\t\t\t\tSBEFIFO_RESCHEDULE;\n> \t\t\tadd_timer(&sbefifo->poll_timer);\n> \t\t\tgoto out_unlock;\n> \t\t}\n> @@ -414,9 +418,8 @@ static void sbefifo_poll_timer(unsigned long data)\n>\n> \t /* Send EOT if the writer is finished. */\n> \tif (test_and_clear_bit(SBEFIFO_XFR_WRITE_DONE, &xfr->flags)) {\n> -\t\tret = sbefifo_outw(sbefifo,\n> -\t\t\t\tSBEFIFO_UP | SBEFIFO_EOT_RAISE,\n> -\t\t\t\tSBEFIFO_EOT_MAGIC);\n> +\t\tret = sbefifo_outw(sbefifo, SBEFIFO_UP | SBEFIFO_EOT_RAISE,\n> +\t\t\t\t SBEFIFO_EOT_MAGIC);\n> \t\tif (ret)\n> \t\t\tgoto out;\n>\n> @@ -438,7 +441,7 @@ static void sbefifo_poll_timer(unsigned long data)\n> \t\tif (devn == 0) {\n> \t\t\t/* No data yet. Reschedule. */\n> \t\t\tsbefifo->poll_timer.expires = jiffies +\n> -\t\t\t\tmsecs_to_jiffies(500);\n> +\t\t\t\tSBEFIFO_RESCHEDULE;\n> \t\t\tadd_timer(&sbefifo->poll_timer);\n> \t\t\tgoto out_unlock;\n> \t\t}\n> @@ -466,7 +469,7 @@ static void sbefifo_poll_timer(unsigned long data)\n> \t\t\tset_bit(SBEFIFO_XFR_COMPLETE, &xfr->flags);\n> \t\t\tlist_del(&xfr->xfrs);\n> \t\t\tif (unlikely(test_bit(SBEFIFO_XFR_CANCEL,\n> -\t\t\t\t\t\t\t&xfr->flags)))\n> +\t\t\t\t\t &xfr->flags)))\n> \t\t\t\tkfree(xfr);\n> \t\t\tbreak;\n> \t\t}\n> @@ -476,7 +479,7 @@ static void sbefifo_poll_timer(unsigned long data)\n> \tif (unlikely(ret)) {\n> \t\tsbefifo->rc = ret;\n> \t\tdev_err(&sbefifo->fsi_dev->dev,\n> -\t\t\t\t\"Fatal bus access failure: %d\\n\", ret);\n> +\t\t\t\"Fatal bus access failure: %d\\n\", ret);\n> \t\tlist_for_each_entry(xfr, &sbefifo->xfrs, xfrs)\n> \t\t\tkfree(xfr);\n> \t\tINIT_LIST_HEAD(&sbefifo->xfrs);\n> @@ -497,7 +500,7 @@ static void sbefifo_poll_timer(unsigned long data)\n> static int sbefifo_open(struct inode *inode, struct file *file)\n> {\n> \tstruct sbefifo *sbefifo = container_of(file->private_data,\n> -\t\t\tstruct sbefifo, mdev);\n> +\t\t\t\t\t struct sbefifo, mdev);\n> \tstruct sbefifo_client *client;\n> \tint ret;\n>\n> @@ -535,6 +538,19 @@ static unsigned int sbefifo_poll(struct file *file, poll_table *wait)\n> \treturn mask;\n> }\n>\n> +static bool sbefifo_read_ready(struct sbefifo *sbefifo,\n> +\t\t\t struct sbefifo_client *client, size_t *n)\n> +{\n> +\tstruct sbefifo_xfr *xfr = list_first_entry_or_null(&client->xfrs,\n> +\t\t\t\t\t\t\t struct sbefifo_xfr,\n> +\t\t\t\t\t\t\t client);\n> +\n> +\t*n = sbefifo_buf_nbreadable(&client->rbuf);\n> +\n> +\treturn READ_ONCE(sbefifo->rc) || *n ||\n> +\t\t(xfr && test_bit(SBEFIFO_XFR_COMPLETE, &xfr->flags));\n> +}\n> +\n> static ssize_t sbefifo_read_common(struct sbefifo_client *client,\n> \t\t\t\t char __user *ubuf, char *kbuf, size_t len)\n> {\n> @@ -551,30 +567,38 @@ static ssize_t sbefifo_read_common(struct sbefifo_client *client,\n>\n> \tsbefifo_get_client(client);\n> \tif (wait_event_interruptible(sbefifo->wait,\n> -\t\t\t\t(ret = READ_ONCE(sbefifo->rc)) ||\n> -\t\t\t\t(n = sbefifo_buf_nbreadable(\n> -\t\t\t\t\t\t&client->rbuf)))) {\n> -\t\tsbefifo_put_client(client);\n> -\t\treturn -ERESTARTSYS;\n> +\t\t\t\t sbefifo_read_ready(sbefifo, client,\n> +\t\t\t\t\t\t\t&n))) {\n> +\t\tret = -ERESTARTSYS;\n> +\t\tgoto out;\n> \t}\n> +\n> +\tret = READ_ONCE(sbefifo->rc);\n> \tif (ret) {\n> \t\tINIT_LIST_HEAD(&client->xfrs);\n> -\t\tsbefifo_put_client(client);\n> -\t\treturn ret;\n> +\t\tgoto out;\n> \t}\n>\n> \tn = min_t(size_t, n, len);\n>\n> \tif (ubuf) {\n> \t\tif (copy_to_user(ubuf, READ_ONCE(client->rbuf.rpos), n)) {\n> -\t\t\tsbefifo_put_client(client);\n> -\t\t\treturn -EFAULT;\n> +\t\t\tret = -EFAULT;\n> +\t\t\tgoto out;\n> \t\t}\n> -\t} else\n> +\t} else {\n> \t\tmemcpy(kbuf, READ_ONCE(client->rbuf.rpos), n);\n> +\t}\n>\n> \tif (sbefifo_buf_readnb(&client->rbuf, n)) {\n> -\t\txfr = sbefifo_client_next_xfr(client);\n> +\t\txfr = list_first_entry_or_null(&client->xfrs,\n> +\t\t\t\t\t struct sbefifo_xfr, client);\n> +\t\tif (!xfr) {\n> +\t\t\t/* should be impossible to not have an xfr here */\n> +\t\t\twake_up(&sbefifo->wait);\n> +\t\t\tgoto out;\n> +\t\t}\n> +\n> \t\tif (!test_bit(SBEFIFO_XFR_COMPLETE, &xfr->flags)) {\n> \t\t\t/*\n> \t\t\t * Fill the read buffer back up.\n> @@ -589,22 +613,31 @@ static ssize_t sbefifo_read_common(struct sbefifo_client *client,\n> \t\t}\n> \t}\n>\n> -\tsbefifo_put_client(client);\n> +\tret = n;\n>\n> -\treturn n;\n> +out:\n> +\tsbefifo_put_client(client);\n> +\treturn ret;\n> }\n>\n> -static ssize_t sbefifo_read(struct file *file, char __user *buf,\n> -\t\tsize_t len, loff_t *offset)\n> +static ssize_t sbefifo_read(struct file *file, char __user *buf, size_t len,\n> +\t\t\t loff_t *offset)\n> {\n> \tstruct sbefifo_client *client = file->private_data;\n>\n> -\tWARN_ON(*offset);\n> +\treturn sbefifo_read_common(client, buf, NULL, len);\n> +}\n>\n> -\tif (!access_ok(VERIFY_WRITE, buf, len))\n> -\t\treturn -EFAULT;\n> +static bool sbefifo_write_ready(struct sbefifo *sbefifo,\n> +\t\t\t\tstruct sbefifo_xfr *xfr,\n> +\t\t\t\tstruct sbefifo_client *client, size_t *n)\n> +{\n> +\tstruct sbefifo_xfr *next = list_first_entry_or_null(&client->xfrs,\n> +\t\t\t\t\t\t\t struct sbefifo_xfr,\n> +\t\t\t\t\t\t\t client);\n>\n> -\treturn sbefifo_read_common(client, buf, NULL, len);\n> +\t*n = sbefifo_buf_nbwriteable(&client->wbuf);\n> +\treturn READ_ONCE(sbefifo->rc) || (next == xfr && *n);\n> }\n>\n> static ssize_t sbefifo_write_common(struct sbefifo_client *client,\n> @@ -612,6 +645,7 @@ static ssize_t sbefifo_write_common(struct sbefifo_client *client,\n> \t\t\t\t size_t len)\n> {\n> \tstruct sbefifo *sbefifo = client->dev;\n> +\tstruct sbefifo_buf *wbuf = &client->wbuf;\n> \tstruct sbefifo_xfr *xfr;\n> \tssize_t ret = 0;\n> \tsize_t n;\n> @@ -622,24 +656,26 @@ static ssize_t sbefifo_write_common(struct sbefifo_client *client,\n> \tif (!len)\n> \t\treturn 0;\n>\n> -\tn = sbefifo_buf_nbwriteable(&client->wbuf);\n> +\tsbefifo_get_client(client);\n> +\tn = sbefifo_buf_nbwriteable(wbuf);\n>\n> \tspin_lock_irq(&sbefifo->lock);\n> -\txfr = sbefifo_next_xfr(sbefifo);\n>\n> +\txfr = sbefifo_next_xfr(sbefifo);\t/* next xfr to be executed */\n> \tif ((client->f_flags & O_NONBLOCK) && xfr && n < len) {\n> \t\tspin_unlock_irq(&sbefifo->lock);\n> -\t\treturn -EAGAIN;\n> +\t\tret = -EAGAIN;\n> +\t\tgoto out;\n> \t}\n>\n> -\txfr = sbefifo_enq_xfr(client);\n> -\tif (!xfr) {\n> +\txfr = sbefifo_enq_xfr(client);\t\t/* this xfr queued up */\n> +\tif (IS_ERR(xfr)) {\n> \t\tspin_unlock_irq(&sbefifo->lock);\n> -\t\treturn -ENOMEM;\n> +\t\tret = PTR_ERR(xfr);\n> +\t\tgoto out;\n> \t}\n> -\tspin_unlock_irq(&sbefifo->lock);\n>\n> -\tsbefifo_get_client(client);\n> +\tspin_unlock_irq(&sbefifo->lock);\n>\n> \t/*\n> \t * Partial writes are not really allowed in that EOT is sent exactly\n> @@ -647,49 +683,47 @@ static ssize_t sbefifo_write_common(struct sbefifo_client *client,\n> \t */\n> \twhile (len) {\n> \t\tif (wait_event_interruptible(sbefifo->wait,\n> -\t\t\t\tREAD_ONCE(sbefifo->rc) ||\n> -\t\t\t\t\t(sbefifo_client_next_xfr(client) == xfr &&\n> -\t\t\t\t\t (n = sbefifo_buf_nbwriteable(\n> -\t\t\t\t\t\t\t&client->wbuf))))) {\n> +\t\t\t\t\t sbefifo_write_ready(sbefifo, xfr,\n> +\t\t\t\t\t\t\t\t client,\n> +\t\t\t\t\t\t\t\t &n))) {\n> \t\t\tset_bit(SBEFIFO_XFR_CANCEL, &xfr->flags);\n> \t\t\tsbefifo_get(sbefifo);\n> \t\t\tif (mod_timer(&sbefifo->poll_timer, jiffies))\n> \t\t\t\tsbefifo_put(sbefifo);\n> -\n> -\t\t\tsbefifo_put_client(client);\n> -\t\t\treturn -ERESTARTSYS;\n> +\t\t\tret = -ERESTARTSYS;\n> +\t\t\tgoto out;\n> \t\t}\n> -\t\tif (sbefifo->rc) {\n> +\n> +\t\tret = READ_ONCE(sbefifo->rc);\n> +\t\tif (ret) {\n> \t\t\tINIT_LIST_HEAD(&client->xfrs);\n> -\t\t\tsbefifo_put_client(client);\n> -\t\t\treturn sbefifo->rc;\n> +\t\t\tgoto out;\n> \t\t}\n>\n> \t\tn = min_t(size_t, n, len);\n>\n> \t\tif (ubuf) {\n> -\t\t\tif (copy_from_user(READ_ONCE(client->wbuf.wpos), ubuf,\n> -\t\t\t n)) {\n> +\t\t\tif (copy_from_user(READ_ONCE(wbuf->wpos), ubuf, n)) {\n> \t\t\t\tset_bit(SBEFIFO_XFR_CANCEL, &xfr->flags);\n> \t\t\t\tsbefifo_get(sbefifo);\n> \t\t\t\tif (mod_timer(&sbefifo->poll_timer, jiffies))\n> \t\t\t\t\tsbefifo_put(sbefifo);\n> -\t\t\t\tsbefifo_put_client(client);\n> -\t\t\t\treturn -EFAULT;\n> +\t\t\t\tret = -EFAULT;\n> +\t\t\t\tgoto out;\n> \t\t\t}\n>\n> \t\t\tubuf += n;\n> \t\t} else {\n> -\t\t\tmemcpy(READ_ONCE(client->wbuf.wpos), kbuf, n);\n> +\t\t\tmemcpy(READ_ONCE(wbuf->wpos), kbuf, n);\n> \t\t\tkbuf += n;\n> \t\t}\n>\n> -\t\tsbefifo_buf_wrotenb(&client->wbuf, n);\n> +\t\tsbefifo_buf_wrotenb(wbuf, n);\n> \t\tlen -= n;\n> \t\tret += n;\n>\n> -\t\t/* set flag before starting the worker, as it may run through\n> -\t\t * and check the flag before we exit this loop!\n> +\t\t/* Set this before starting timer to avoid race condition on\n> +\t\t * this flag with the timer function writer.\n> \t\t */\n> \t\tif (!len)\n> \t\t\tset_bit(SBEFIFO_XFR_WRITE_DONE, &xfr->flags);\n> @@ -702,21 +736,16 @@ static ssize_t sbefifo_write_common(struct sbefifo_client *client,\n> \t\t\tsbefifo_put(sbefifo);\n> \t}\n>\n> +out:\n> \tsbefifo_put_client(client);\n> -\n> \treturn ret;\n> }\n>\n> static ssize_t sbefifo_write(struct file *file, const char __user *buf,\n> -\t\tsize_t len, loff_t *offset)\n> +\t\t\t size_t len, loff_t *offset)\n> {\n> \tstruct sbefifo_client *client = file->private_data;\n>\n> -\tWARN_ON(*offset);\n> -\n> -\tif (!access_ok(VERIFY_READ, buf, len))\n> -\t\treturn -EFAULT;\n> -\n> \treturn sbefifo_write_common(client, buf, NULL, len);\n> }\n>\n> @@ -742,18 +771,15 @@ static int sbefifo_release(struct inode *inode, struct file *file)\n> struct sbefifo_client *sbefifo_drv_open(struct device *dev,\n> \t\t\t\t\tunsigned long flags)\n> {\n> -\tstruct sbefifo_client *client = NULL;\n> -\tstruct sbefifo *sbefifo;\n> -\tstruct fsi_device *fsi_dev = to_fsi_dev(dev);\n> +\tstruct sbefifo_client *client;\n> +\tstruct sbefifo *sbefifo = dev_get_drvdata(dev);\n>\n> -\tlist_for_each_entry(sbefifo, &sbefifo_fifos, link) {\n> -\t\tif (sbefifo->fsi_dev != fsi_dev)\n> -\t\t\tcontinue;\n> +\tif (!sbefifo)\n> +\t\treturn NULL;\n>\n> -\t\tclient = sbefifo_new_client(sbefifo);\n> -\t\tif (client)\n> -\t\t\tclient->f_flags = flags;\n> -\t}\n> +\tclient = sbefifo_new_client(sbefifo);\n> +\tif (client)\n> +\t\tclient->f_flags = flags;\n>\n> \treturn client;\n> }\n> @@ -802,95 +828,92 @@ static int sbefifo_probe(struct device *dev)\n> \tu32 sts;\n> \tint ret, child_idx = 0;\n>\n> -\tdev_dbg(dev, \"Found sbefifo device\\n\");\n> \tsbefifo = kzalloc(sizeof(*sbefifo), GFP_KERNEL);\n> \tif (!sbefifo)\n> \t\treturn -ENOMEM;\n>\n> \tsbefifo->fsi_dev = fsi_dev;\n>\n> -\tret = sbefifo_inw(sbefifo,\n> -\t\t\tSBEFIFO_UP | SBEFIFO_STS, &sts);\n> +\tret = sbefifo_inw(sbefifo, SBEFIFO_UP | SBEFIFO_STS, &sts);\n> \tif (ret)\n> \t\treturn ret;\n> +\n> \tif (!(sts & SBEFIFO_EMPTY)) {\n> -\t\tdev_err(&sbefifo->fsi_dev->dev,\n> -\t\t\t\t\"Found data in upstream fifo\\n\");\n> +\t\tdev_err(dev, \"Found data in upstream fifo\\n\");\n> \t\treturn -EIO;\n> \t}\n>\n> \tret = sbefifo_inw(sbefifo, SBEFIFO_DWN | SBEFIFO_STS, &sts);\n> \tif (ret)\n> \t\treturn ret;\n> +\n> \tif (!(sts & SBEFIFO_EMPTY)) {\n> -\t\tdev_err(&sbefifo->fsi_dev->dev,\n> -\t\t\t\t\"Found data in downstream fifo\\n\");\n> +\t\tdev_err(dev, \"found data in downstream fifo\\n\");\n> \t\treturn -EIO;\n> \t}\n>\n> -\tsbefifo->mdev.minor = MISC_DYNAMIC_MINOR;\n> -\tsbefifo->mdev.fops = &sbefifo_fops;\n> -\tsbefifo->mdev.name = sbefifo->name;\n> -\tsbefifo->mdev.parent = dev;\n> \tspin_lock_init(&sbefifo->lock);\n> \tkref_init(&sbefifo->kref);\n> +\tinit_waitqueue_head(&sbefifo->wait);\n> +\tINIT_LIST_HEAD(&sbefifo->xfrs);\n>\n> \tsbefifo->idx = ida_simple_get(&sbefifo_ida, 1, INT_MAX, GFP_KERNEL);\n> \tsnprintf(sbefifo->name, sizeof(sbefifo->name), \"sbefifo%d\",\n> -\t\t\tsbefifo->idx);\n> -\tinit_waitqueue_head(&sbefifo->wait);\n> -\tINIT_LIST_HEAD(&sbefifo->xfrs);\n> +\t\t sbefifo->idx);\n>\n> \t/* This bit of silicon doesn't offer any interrupts... */\n> \tsetup_timer(&sbefifo->poll_timer, sbefifo_poll_timer,\n> -\t\t\t(unsigned long)sbefifo);\n> -\n> -\tif (dev->of_node) {\n> -\t\t/* create platform devs for dts child nodes (occ, etc) */\n> -\t\tfor_each_child_of_node(dev->of_node, np) {\n> -\t\t\tsnprintf(child_name, sizeof(child_name), \"%s-dev%d\",\n> -\t\t\t\t sbefifo->name, child_idx++);\n> -\t\t\tchild = of_platform_device_create(np, child_name, dev);\n> -\t\t\tif (!child)\n> -\t\t\t\tdev_warn(&sbefifo->fsi_dev->dev,\n> -\t\t\t\t\t \"failed to create child node dev\\n\");\n> -\t\t}\n> +\t\t (unsigned long)sbefifo);\n> +\n> +\tsbefifo->mdev.minor = MISC_DYNAMIC_MINOR;\n> +\tsbefifo->mdev.fops = &sbefifo_fops;\n> +\tsbefifo->mdev.name = sbefifo->name;\n> +\tsbefifo->mdev.parent = dev;\n> +\tret = misc_register(&sbefifo->mdev);\n> +\tif (ret) {\n> +\t\tdev_err(dev, \"failed to register miscdevice: %d\\n\", ret);\n> +\t\tida_simple_remove(&sbefifo_ida, sbefifo->idx);\n> +\t\tsbefifo_put(sbefifo);\n> +\t\treturn ret;\n> +\t}\n> +\n> +\t/* create platform devs for dts child nodes (occ, etc) */\n> +\tfor_each_available_child_of_node(dev->of_node, np) {\n> +\t\tsnprintf(child_name, sizeof(child_name), \"%s-dev%d\",\n> +\t\t\t sbefifo->name, child_idx++);\n> +\t\tchild = of_platform_device_create(np, child_name, dev);\n> +\t\tif (!child)\n> +\t\t\tdev_warn(dev, \"failed to create child %s dev\\n\",\n> +\t\t\t\t child_name);\n> \t}\n>\n> -\tlist_add(&sbefifo->link, &sbefifo_fifos);\n> -\t\n> -\treturn misc_register(&sbefifo->mdev);\n> +\tdev_set_drvdata(dev, sbefifo);\n> +\n> +\treturn 0;\n> }\n>\n> static int sbefifo_remove(struct device *dev)\n> {\n> -\tstruct fsi_device *fsi_dev = to_fsi_dev(dev);\n> -\tstruct sbefifo *sbefifo, *sbefifo_tmp;\n> +\tstruct sbefifo *sbefifo = dev_get_drvdata(dev);\n> \tstruct sbefifo_xfr *xfr;\n>\n> -\tlist_for_each_entry_safe(sbefifo, sbefifo_tmp, &sbefifo_fifos, link) {\n> -\t\tif (sbefifo->fsi_dev != fsi_dev)\n> -\t\t\tcontinue;\n> -\n> -\t\tdevice_for_each_child(dev, NULL, sbefifo_unregister_child);\n> +\tWRITE_ONCE(sbefifo->rc, -ENODEV);\n> +\twake_up(&sbefifo->wait);\n>\n> -\t\tmisc_deregister(&sbefifo->mdev);\n> -\t\tlist_del(&sbefifo->link);\n> -\t\tida_simple_remove(&sbefifo_ida, sbefifo->idx);\n> +\tmisc_deregister(&sbefifo->mdev);\n> +\tdevice_for_each_child(dev, NULL, sbefifo_unregister_child);\n>\n> -\t\tif (del_timer_sync(&sbefifo->poll_timer))\n> -\t\t\tsbefifo_put(sbefifo);\n> +\tida_simple_remove(&sbefifo_ida, sbefifo->idx);\n>\n> -\t\tspin_lock(&sbefifo->lock);\n> -\t\tlist_for_each_entry(xfr, &sbefifo->xfrs, xfrs)\n> -\t\t\tkfree(xfr);\n> -\t\tspin_unlock(&sbefifo->lock);\n> +\tif (del_timer_sync(&sbefifo->poll_timer))\n> +\t\tsbefifo_put(sbefifo);\n>\n> -\t\tWRITE_ONCE(sbefifo->rc, -ENODEV);\n> +\tspin_lock(&sbefifo->lock);\n> +\tlist_for_each_entry(xfr, &sbefifo->xfrs, xfrs)\n> +\t\tkfree(xfr);\n> +\tspin_unlock(&sbefifo->lock);\n>\n> -\t\twake_up(&sbefifo->wait);\n> -\t\tsbefifo_put(sbefifo);\n> -\t}\n> +\tsbefifo_put(sbefifo);\n>\n> \treturn 0;\n> }\n> @@ -915,17 +938,19 @@ static int sbefifo_remove(struct device *dev)\n>\n> static int sbefifo_init(void)\n> {\n> -\tINIT_LIST_HEAD(&sbefifo_fifos);\n> \treturn fsi_driver_register(&sbefifo_drv);\n> }\n>\n> static void sbefifo_exit(void)\n> {\n> \tfsi_driver_unregister(&sbefifo_drv);\n> +\n> +\tida_destroy(&sbefifo_ida);\n> }\n>\n> module_init(sbefifo_init);\n> module_exit(sbefifo_exit);\n> MODULE_LICENSE(\"GPL\");\n> MODULE_AUTHOR(\"Brad Bishop \");\n> -MODULE_DESCRIPTION(\"Linux device interface to the POWER self boot engine\");\n> +MODULE_AUTHOR(\"Eddie James \");\n> +MODULE_DESCRIPTION(\"Linux device interface to the POWER Self Boot Engine\");\n> diff --git a/include/linux/fsi-sbefifo.h b/include/linux/fsi-sbefifo.h\n> index 1b46c63..8e55891 100644\n> --- a/include/linux/fsi-sbefifo.h\n> +++ b/include/linux/fsi-sbefifo.h\n> @@ -13,8 +13,8 @@\n> * GNU General Public License for more details.\n> */\n>\n> -#ifndef __FSI_SBEFIFO_H__\n> -#define __FSI_SBEFIFO_H__\n> +#ifndef LINUX_FSI_SBEFIFO_H\n> +#define LINUX_FSI_SBEFIFO_H\n>\n> struct device;\n> struct sbefifo_client;\n> @@ -27,4 +27,4 @@ extern int sbefifo_drv_write(struct sbefifo_client *client, const char *buf,\n> \t\t\t size_t len);\n> extern void sbefifo_drv_release(struct sbefifo_client *client);\n>\n> -#endif /* __FSI_SBEFIFO_H__ */\n> +#endif /* LINUX_FSI_SBEFIFO_H */","headers":{"Return-Path":"","X-Original-To":["incoming@patchwork.ozlabs.org","openbmc@lists.ozlabs.org"],"Delivered-To":["patchwork-incoming@bilbo.ozlabs.org","openbmc@lists.ozlabs.org"],"Received":["from lists.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3])\n\t(using TLSv1.2 with cipher ADH-AES256-GCM-SHA384 (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xysg92k2Xz9t30\n\tfor ;\n\tFri, 22 Sep 2017 09:06:29 +1000 (AEST)","from lists.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3])\n\tby lists.ozlabs.org (Postfix) with ESMTP id 3xysg91lj5zDsM4\n\tfor ;\n\tFri, 22 Sep 2017 09:06:29 +1000 (AEST)","from mx0a-001b2d01.pphosted.com (mx0b-001b2d01.pphosted.com\n\t[148.163.158.5])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256\n\tbits)) (No client certificate requested)\n\tby lists.ozlabs.org (Postfix) with ESMTPS id 3xysfJ3QRxzDqT0\n\tfor ; Fri, 22 Sep 2017 09:05:44 +1000 (AEST)","from pps.filterd (m0098420.ppops.net [127.0.0.1])\n\tby mx0b-001b2d01.pphosted.com (8.16.0.21/8.16.0.21) with SMTP id\n\tv8LN47r9043683\n\tfor ; Thu, 21 Sep 2017 19:05:41 -0400","from e37.co.us.ibm.com (e37.co.us.ibm.com [32.97.110.158])\n\tby mx0b-001b2d01.pphosted.com with ESMTP id 2d4pnh0b9f-1\n\t(version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT)\n\tfor ; Thu, 21 Sep 2017 19:05:41 -0400","from localhost\n\tby e37.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use\n\tOnly! Violators will be prosecuted\n\tfor from ;\n\tThu, 21 Sep 2017 16:55:26 -0600","from b03cxnp08027.gho.boulder.ibm.com (9.17.130.19)\n\tby e37.co.us.ibm.com (192.168.1.137) with IBM ESMTP SMTP Gateway:\n\tAuthorized Use Only! Violators will be prosecuted; \n\tThu, 21 Sep 2017 16:55:23 -0600","from b03ledav005.gho.boulder.ibm.com\n\t(b03ledav005.gho.boulder.ibm.com [9.17.130.236])\n\tby b03cxnp08027.gho.boulder.ibm.com (8.14.9/8.14.9/NCO v10.0) with\n\tESMTP id v8LMtMVO58327188; Thu, 21 Sep 2017 15:55:22 -0700","from b03ledav005.gho.boulder.ibm.com (unknown [127.0.0.1])\n\tby IMSVA (Postfix) with ESMTP id 714F9BE040;\n\tThu, 21 Sep 2017 16:55:22 -0600 (MDT)","from oc3016140333.ibm.com (unknown [9.41.174.252])\n\tby b03ledav005.gho.boulder.ibm.com (Postfix) with ESMTP id DA732BE038;\n\tThu, 21 Sep 2017 16:55:21 -0600 (MDT)"],"Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=linux.vnet.ibm.com\n\t(client-ip=148.163.158.5; helo=mx0a-001b2d01.pphosted.com;\n\tenvelope-from=eajames@linux.vnet.ibm.com; receiver=)","Subject":"Re: [PATCH linux dev-4.10 1/3] drivers/fsi/sbefifo: refactor to\n\tupstream list state","From":"Eddie James ","To":"openbmc@lists.ozlabs.org","References":"<1506033838-2078-1-git-send-email-eajames@linux.vnet.ibm.com>\n\t<1506033838-2078-2-git-send-email-eajames@linux.vnet.ibm.com>","Date":"Thu, 21 Sep 2017 17:55:21 -0500","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.2.0","MIME-Version":"1.0","In-Reply-To":"<1506033838-2078-2-git-send-email-eajames@linux.vnet.ibm.com>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"7bit","Content-Language":"en-US","X-TM-AS-GCONF":"00","x-cbid":"17092122-0024-0000-0000-0000173AB6F2","X-IBM-SpamModules-Scores":"","X-IBM-SpamModules-Versions":"BY=3.00007775; HX=3.00000241; KW=3.00000007;\n\tPH=3.00000004; SC=3.00000231; SDB=6.00920433; UDB=6.00462505;\n\tIPR=6.00700655; \n\tBA=6.00005601; NDR=6.00000001; ZLA=6.00000005; ZF=6.00000009;\n\tZB=6.00000000; \n\tZP=6.00000000; ZH=6.00000000; ZU=6.00000002; MB=3.00017240;\n\tXFM=3.00000015; UTC=2017-09-21 22:55:24","X-IBM-AV-DETECTION":"SAVI=unused REMOTE=unused XFE=unused","x-cbparentid":"17092122-0025-0000-0000-00004CD066DB","Message-Id":"","X-Proofpoint-Virus-Version":"vendor=fsecure engine=2.50.10432:, ,\n\tdefinitions=2017-09-21_06:, , signatures=0","X-Proofpoint-Spam-Details":"rule=outbound_notspam policy=outbound score=0\n\tspamscore=0 suspectscore=4\n\tmalwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam\n\tadjust=0 reason=mlx scancount=1 engine=8.0.1-1707230000\n\tdefinitions=main-1709210310","X-BeenThere":"openbmc@lists.ozlabs.org","X-Mailman-Version":"2.1.24","Precedence":"list","List-Id":"Development list for OpenBMC ","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"andrew@aj.id.au, \"Edward A. James\" ","Errors-To":"openbmc-bounces+incoming=patchwork.ozlabs.org@lists.ozlabs.org","Sender":"\"openbmc\"\n\t"}},{"id":1774407,"web_url":"http://patchwork.ozlabs.org/comment/1774407/","msgid":"<1506315435.30138.17.camel@aj.id.au>","list_archive_url":null,"date":"2017-09-25T04:57:15","subject":"Re: [PATCH linux dev-4.10 1/3] drivers/fsi/sbefifo: refactor to\n\tupstream list state","submitter":{"id":68332,"url":"http://patchwork.ozlabs.org/api/people/68332/","name":"Andrew Jeffery","email":"andrew@aj.id.au"},"content":"On Thu, 2017-09-21 at 17:43 -0500, Eddie James wrote:\n> From: \"Edward A. James\" \n> \n> Includes various fixes:\n>  - check for complete while waiting for data in read()\n>  - fix probe if probe fails\n>  - general cleanup\n>  - slightly safer (earlier) get_client()\n>  - reorder some of the remove() operations for safety\n\nThe fact that you can create a commit message that describes the patch in\nterms of bullet points indicates to me that these bullet points should all be\nseparate patches. Squashing conceptually different changes like this makes the\npatch much harder to review, as it's hard to tease apart how and why a change\nfixes any one of the deficient behaviours.\n\nPlease split them out in a follow-up if there is one. My personal preference is\nyou do a v2 regardless just so you can split the patches out.\n\n> \n> Signed-off-by: Edward A. James \n> ---\n>  drivers/fsi/fsi-sbefifo.c   | 329 ++++++++++++++++++++++++--------------------\n>  include/linux/fsi-sbefifo.h |   6 +-\n>  2 files changed, 180 insertions(+), 155 deletions(-)\n> \n> diff --git a/drivers/fsi/fsi-sbefifo.c b/drivers/fsi/fsi-sbefifo.c\n> index 1c37ff7..5d25ade 100644\n> --- a/drivers/fsi/fsi-sbefifo.c\n> +++ b/drivers/fsi/fsi-sbefifo.c\n> @@ -11,20 +11,27 @@\n>   * GNU General Public License for more details.\n>   */\n>  \n> -#include \n> +#include \n>  #include \n> -#include \n> +#include \n>  #include \n> +#include \n> +#include \n> +#include \n> +#include \n>  #include \n>  #include \n>  #include \n>  #include \n> +#include \n>  #include \n>  #include \n>  #include \n>  #include \n> +#include \n>  #include \n>  #include \n> +#include \n>  \n>  /*\n>   * The SBEFIFO is a pipe-like FSI device for communicating with\n> @@ -44,14 +51,15 @@\n>  #define   SBEFIFO_EOT_MAGIC\t\t0xffffffff\n>  #define SBEFIFO_EOT_ACK\t\t0x14\n>  \n> +#define SBEFIFO_RESCHEDULE\tmsecs_to_jiffies(500)\n> +\n>  struct sbefifo {\n>  \tstruct timer_list poll_timer;\n> -\tstruct fsi_device *fsi_dev;\n> -\tstruct miscdevice mdev;\n> +\tstruct fsi_device *fsi_dev;\t/* parent fsi device */\n\nNot convinced the comment is helpful.\n\n> +\tstruct miscdevice mdev;\t\t/* /dev/ entry */\n\nNot necesarily the intent of this patch, but I think it would be a good thing\nto split out the SBEFIFO core from the chardev interface.\n\n>  \twait_queue_head_t wait;\n> -\tstruct list_head link;\n>  \tstruct list_head xfrs;\n> -\tstruct kref kref;\n> +\tstruct kref kref;\t\t/* reference counter */\n\nNot convinced the comment is helpful.\n\n>  \tspinlock_t lock;\n>  \tchar name[32];\n>  \tint idx;\n> @@ -87,14 +95,12 @@ struct sbefifo_client {\n>  \tunsigned long f_flags;\n>  };\n>  \n> -static struct list_head sbefifo_fifos;\n> -\n\nIt's not clear to me why this is being removed. Which one of the bullets in the\ncommit message accounts for this?\n\n>  static DEFINE_IDA(sbefifo_ida);\n>  \n>  static int sbefifo_inw(struct sbefifo *sbefifo, int reg, u32 *word)\n>  {\n>  \tint rc;\n> -\tu32 raw_word;\n> +\t__be32 raw_word;\n>  \n>  \trc = fsi_device_read(sbefifo->fsi_dev, reg, &raw_word,\n>  \t\t\t     sizeof(raw_word));\n> @@ -102,17 +108,19 @@ static int sbefifo_inw(struct sbefifo *sbefifo, int reg, u32 *word)\n>  \t\treturn rc;\n>  \n>  \t*word = be32_to_cpu(raw_word);\n> +\n>  \treturn 0;\n>  }\n>  \n>  static int sbefifo_outw(struct sbefifo *sbefifo, int reg, u32 word)\n>  {\n> -\tu32 raw_word = cpu_to_be32(word);\n> +\t__be32 raw_word = cpu_to_be32(word);\n>  \n>  \treturn fsi_device_write(sbefifo->fsi_dev, reg, &raw_word,\n>  \t\t\t\tsizeof(raw_word));\n>  }\n>  \n> +/* Don't flip endianness of data to/from FIFO, just pass through. */\n>  static int sbefifo_readw(struct sbefifo *sbefifo, u32 *word)\n>  {\n>  \treturn fsi_device_read(sbefifo->fsi_dev, SBEFIFO_DWN, word,\n> @@ -136,7 +144,7 @@ static int sbefifo_ack_eot(struct sbefifo *sbefifo)\n>  \t\treturn ret;\n>  \n>  \treturn sbefifo_outw(sbefifo, SBEFIFO_DWN | SBEFIFO_EOT_ACK,\n> -\t\t\tSBEFIFO_EOT_MAGIC);\n> +\t\t\t    SBEFIFO_EOT_MAGIC);\n>  }\n>  \n>  static size_t sbefifo_dev_nwreadable(u32 sts)\n> @@ -210,6 +218,7 @@ static bool sbefifo_buf_readnb(struct sbefifo_buf *buf, size_t n)\n>  \t\trpos = buf->buf;\n>  \n>  \tWRITE_ONCE(buf->rpos, rpos);\n> +\n>  \treturn rpos == wpos;\n>  }\n>  \n> @@ -229,14 +238,14 @@ static bool sbefifo_buf_wrotenb(struct sbefifo_buf *buf, size_t n)\n>  \t\tset_bit(SBEFIFO_BUF_FULL, &buf->flags);\n>  \n>  \tWRITE_ONCE(buf->wpos, wpos);\n> +\n>  \treturn rpos == wpos;\n>  }\n>  \n>  static void sbefifo_free(struct kref *kref)\n>  {\n> -\tstruct sbefifo *sbefifo;\n> +\tstruct sbefifo *sbefifo = container_of(kref, struct sbefifo, kref);\n>  \n> -\tsbefifo = container_of(kref, struct sbefifo, kref);\n>  \tkfree(sbefifo);\n>  }\n>  \n> @@ -255,9 +264,12 @@ static struct sbefifo_xfr *sbefifo_enq_xfr(struct sbefifo_client *client)\n>  \tstruct sbefifo *sbefifo = client->dev;\n>  \tstruct sbefifo_xfr *xfr;\n>  \n> +\tif (READ_ONCE(sbefifo->rc))\n\nWhat semantics are we trying to enforce with READ_ONCE()?\n\n> +\t\treturn ERR_PTR(sbefifo->rc);\n\nThis will possibly read sbefifo->rc again?\n\n> +\n>  \txfr = kzalloc(sizeof(*xfr), GFP_KERNEL);\n>  \tif (!xfr)\n> -\t\treturn NULL;\n> +\t\treturn ERR_PTR(-ENOMEM);\n>  \n>  \txfr->rbuf = &client->rbuf;\n>  \txfr->wbuf = &client->wbuf;\n> @@ -267,21 +279,12 @@ static struct sbefifo_xfr *sbefifo_enq_xfr(struct sbefifo_client *client)\n>  \treturn xfr;\n>  }\n>  \n> -static struct sbefifo_xfr *sbefifo_client_next_xfr(\n> -\t\tstruct sbefifo_client *client)\n> -{\n> -\tif (list_empty(&client->xfrs))\n> -\t\treturn NULL;\n> -\n> -\treturn container_of(client->xfrs.next, struct sbefifo_xfr,\n> -\t\t\tclient);\n> -}\n> -\n>  static bool sbefifo_xfr_rsp_pending(struct sbefifo_client *client)\n>  {\n> -\tstruct sbefifo_xfr *xfr;\n> +\tstruct sbefifo_xfr *xfr = list_first_entry_or_null(&client->xfrs,\n> +\t\t\t\t\t\t\t   struct sbefifo_xfr,\n> +\t\t\t\t\t\t\t   client);\n>  \n> -\txfr = sbefifo_client_next_xfr(client);\n>  \tif (xfr && test_bit(SBEFIFO_XFR_RESP_PENDING, &xfr->flags))\n>  \t\treturn true;\n>  \n> @@ -309,10 +312,11 @@ static struct sbefifo_client *sbefifo_new_client(struct sbefifo *sbefifo)\n>  \n>  static void sbefifo_client_release(struct kref *kref)\n>  {\n> -\tstruct sbefifo_client *client;\n>  \tstruct sbefifo_xfr *xfr;\n> +\tstruct sbefifo_client *client = container_of(kref,\n> +\t\t\t\t\t\t     struct sbefifo_client,\n> +\t\t\t\t\t\t     kref);\n>  \n> -\tclient = container_of(kref, struct sbefifo_client, kref);\n\nWhy did we move to assigning this in the declaration? It looks better this way,\nand it's not as if the pointer itself is const.\n\n>  \tlist_for_each_entry(xfr, &client->xfrs, client) {\n>  \t\t/*\n>  \t\t * The client left with pending or running xfrs.\n> @@ -349,6 +353,7 @@ static struct sbefifo_xfr *sbefifo_next_xfr(struct sbefifo *sbefifo)\n>  \t\t\tkfree(xfr);\n>  \t\t\tcontinue;\n>  \t\t}\n> +\n>  \t\treturn xfr;\n>  \t}\n>  \n> @@ -370,7 +375,7 @@ static void sbefifo_poll_timer(unsigned long data)\n>  \n>  \tspin_lock(&sbefifo->lock);\n>  \txfr = list_first_entry_or_null(&sbefifo->xfrs, struct sbefifo_xfr,\n> -\t\t\txfrs);\n> +\t\t\t\t       xfrs);\n>  \tif (!xfr)\n>  \t\tgoto out_unlock;\n>  \n> @@ -388,8 +393,7 @@ static void sbefifo_poll_timer(unsigned long data)\n>  \n>  \t /* Drain the write buffer. */\n>  \twhile ((bufn = sbefifo_buf_nbreadable(wbuf))) {\n> -\t\tret = sbefifo_inw(sbefifo, SBEFIFO_UP | SBEFIFO_STS,\n> -\t\t\t\t&sts);\n> +\t\tret = sbefifo_inw(sbefifo, SBEFIFO_UP | SBEFIFO_STS, &sts);\n>  \t\tif (ret)\n>  \t\t\tgoto out;\n>  \n> @@ -397,7 +401,7 @@ static void sbefifo_poll_timer(unsigned long data)\n>  \t\tif (devn == 0) {\n>  \t\t\t/* No open slot for write.  Reschedule. */\n>  \t\t\tsbefifo->poll_timer.expires = jiffies +\n> -\t\t\t\tmsecs_to_jiffies(500);\n> +\t\t\t\tSBEFIFO_RESCHEDULE;\n>  \t\t\tadd_timer(&sbefifo->poll_timer);\n\nNot really a comment relevant for the patch and I don't know much about the\noperations of the SBEFIFO, but using a timer for slot management feels odd.\n\nHowever, I think this is covered by the comment in probe about the silicon\nlacking interrupts. Ugh.\n\n>  \t\t\tgoto out_unlock;\n>  \t\t}\n> @@ -414,9 +418,8 @@ static void sbefifo_poll_timer(unsigned long data)\n>  \n>  \t /* Send EOT if the writer is finished. */\n>  \tif (test_and_clear_bit(SBEFIFO_XFR_WRITE_DONE, &xfr->flags)) {\n> -\t\tret = sbefifo_outw(sbefifo,\n> -\t\t\t\tSBEFIFO_UP | SBEFIFO_EOT_RAISE,\n> -\t\t\t\tSBEFIFO_EOT_MAGIC);\n> +\t\tret = sbefifo_outw(sbefifo, SBEFIFO_UP | SBEFIFO_EOT_RAISE,\n> +\t\t\t\t   SBEFIFO_EOT_MAGIC);\n>  \t\tif (ret)\n>  \t\t\tgoto out;\n>  \n> @@ -438,7 +441,7 @@ static void sbefifo_poll_timer(unsigned long data)\n>  \t\tif (devn == 0) {\n>  \t\t\t/* No data yet.  Reschedule. */\n>  \t\t\tsbefifo->poll_timer.expires = jiffies +\n> -\t\t\t\tmsecs_to_jiffies(500);\n> +\t\t\t\tSBEFIFO_RESCHEDULE;\n>  \t\t\tadd_timer(&sbefifo->poll_timer);\n>  \t\t\tgoto out_unlock;\n>  \t\t}\n> @@ -466,7 +469,7 @@ static void sbefifo_poll_timer(unsigned long data)\n>  \t\t\tset_bit(SBEFIFO_XFR_COMPLETE, &xfr->flags);\n>  \t\t\tlist_del(&xfr->xfrs);\n>  \t\t\tif (unlikely(test_bit(SBEFIFO_XFR_CANCEL,\n> -\t\t\t\t\t\t\t&xfr->flags)))\n> +\t\t\t\t\t      &xfr->flags)))\n>  \t\t\t\tkfree(xfr);\n>  \t\t\tbreak;\n>  \t\t}\n> @@ -476,7 +479,7 @@ static void sbefifo_poll_timer(unsigned long data)\n>  \tif (unlikely(ret)) {\n>  \t\tsbefifo->rc = ret;\n>  \t\tdev_err(&sbefifo->fsi_dev->dev,\n> -\t\t\t\t\"Fatal bus access failure: %d\\n\", ret);\n> +\t\t\t\"Fatal bus access failure: %d\\n\", ret);\n>  \t\tlist_for_each_entry(xfr, &sbefifo->xfrs, xfrs)\n>  \t\t\tkfree(xfr);\n>  \t\tINIT_LIST_HEAD(&sbefifo->xfrs);\n> @@ -497,7 +500,7 @@ static void sbefifo_poll_timer(unsigned long data)\n>  static int sbefifo_open(struct inode *inode, struct file *file)\n>  {\n>  \tstruct sbefifo *sbefifo = container_of(file->private_data,\n> -\t\t\tstruct sbefifo, mdev);\n> +\t\t\t\t\t       struct sbefifo, mdev);\n>  \tstruct sbefifo_client *client;\n>  \tint ret;\n>  \n> @@ -535,6 +538,19 @@ static unsigned int sbefifo_poll(struct file *file, poll_table *wait)\n>  \treturn mask;\n>  }\n>  \n> +static bool sbefifo_read_ready(struct sbefifo *sbefifo,\n> +\t\t\t       struct sbefifo_client *client, size_t *n)\n> +{\n> +\tstruct sbefifo_xfr *xfr = list_first_entry_or_null(&client->xfrs,\n> +\t\t\t\t\t\t\t   struct sbefifo_xfr,\n> +\t\t\t\t\t\t\t   client);\n> +\n> +\t*n = sbefifo_buf_nbreadable(&client->rbuf);\n> +\n> +\treturn READ_ONCE(sbefifo->rc) || *n ||\n> +\t\t(xfr && test_bit(SBEFIFO_XFR_COMPLETE, &xfr->flags));\n> +}\n> +\n>  static ssize_t sbefifo_read_common(struct sbefifo_client *client,\n>  \t\t\t\t   char __user *ubuf, char *kbuf, size_t len)\n>  {\n> @@ -551,30 +567,38 @@ static ssize_t sbefifo_read_common(struct sbefifo_client *client,\n>  \n>  \tsbefifo_get_client(client);\n>  \tif (wait_event_interruptible(sbefifo->wait,\n> -\t\t\t\t(ret = READ_ONCE(sbefifo->rc)) ||\n> -\t\t\t\t(n = sbefifo_buf_nbreadable(\n> -\t\t\t\t\t\t&client->rbuf)))) {\n> -\t\tsbefifo_put_client(client);\n> -\t\treturn -ERESTARTSYS;\n> +\t\t\t\t     sbefifo_read_ready(sbefifo, client,\n> +\t\t\t\t\t\t\t&n))) {\n> +\t\tret = -ERESTARTSYS;\n> +\t\tgoto out;\n>  \t}\n> +\n> +\tret = READ_ONCE(sbefifo->rc);\n>  \tif (ret) {\n>  \t\tINIT_LIST_HEAD(&client->xfrs);\n> -\t\tsbefifo_put_client(client);\n> -\t\treturn ret;\n> +\t\tgoto out;\n>  \t}\n>  \n>  \tn = min_t(size_t, n, len);\n>  \n>  \tif (ubuf) {\n>  \t\tif (copy_to_user(ubuf, READ_ONCE(client->rbuf.rpos), n)) {\n> -\t\t\tsbefifo_put_client(client);\n> -\t\t\treturn -EFAULT;\n> +\t\t\tret = -EFAULT;\n> +\t\t\tgoto out;\n>  \t\t}\n> -\t} else\n> +\t} else {\n>  \t\tmemcpy(kbuf, READ_ONCE(client->rbuf.rpos), n);\n> +\t}\n>  \n>  \tif (sbefifo_buf_readnb(&client->rbuf, n)) {\n> -\t\txfr = sbefifo_client_next_xfr(client);\n> +\t\txfr = list_first_entry_or_null(&client->xfrs,\n> +\t\t\t\t\t       struct sbefifo_xfr, client);\n> +\t\tif (!xfr) {\n> +\t\t\t/* should be impossible to not have an xfr here */\n\nMaybe you want a `if (WARN_ON(!xfr)) {` here?\n\n> +\t\t\twake_up(&sbefifo->wait);\n> +\t\t\tgoto out;\n> +\t\t}\n> +\n>  \t\tif (!test_bit(SBEFIFO_XFR_COMPLETE, &xfr->flags)) {\n>  \t\t\t/*\n>  \t\t\t * Fill the read buffer back up.\n> @@ -589,22 +613,31 @@ static ssize_t sbefifo_read_common(struct sbefifo_client *client,\n>  \t\t}\n>  \t}\n>  \n> -\tsbefifo_put_client(client);\n> +\tret = n;\n>  \n> -\treturn n;\n> +out:\n> +\tsbefifo_put_client(client);\n> +\treturn ret;\n>  }\n>  \n> -static ssize_t sbefifo_read(struct file *file, char __user *buf,\n> -\t\tsize_t len, loff_t *offset)\n> +static ssize_t sbefifo_read(struct file *file, char __user *buf, size_t len,\n> +\t\t\t    loff_t *offset)\n>  {\n>  \tstruct sbefifo_client *client = file->private_data;\n>  \n> -\tWARN_ON(*offset);\n> +\treturn sbefifo_read_common(client, buf, NULL, len);\n> +}\n>  \n> -\tif (!access_ok(VERIFY_WRITE, buf, len))\n> -\t\treturn -EFAULT;\n> +static bool sbefifo_write_ready(struct sbefifo *sbefifo,\n> +\t\t\t\tstruct sbefifo_xfr *xfr,\n> +\t\t\t\tstruct sbefifo_client *client, size_t *n)\n> +{\n> +\tstruct sbefifo_xfr *next = list_first_entry_or_null(&client->xfrs,\n> +\t\t\t\t\t\t\t    struct sbefifo_xfr,\n> +\t\t\t\t\t\t\t    client);\n>  \n> -\treturn sbefifo_read_common(client, buf, NULL, len);\n> +\t*n = sbefifo_buf_nbwriteable(&client->wbuf);\n> +\treturn READ_ONCE(sbefifo->rc) || (next == xfr && *n);\n>  }\n>  \n>  static ssize_t sbefifo_write_common(struct sbefifo_client *client,\n> @@ -612,6 +645,7 @@ static ssize_t sbefifo_write_common(struct sbefifo_client *client,\n>  \t\t\t\t    size_t len)\n>  {\n>  \tstruct sbefifo *sbefifo = client->dev;\n> +\tstruct sbefifo_buf *wbuf = &client->wbuf;\n>  \tstruct sbefifo_xfr *xfr;\n>  \tssize_t ret = 0;\n>  \tsize_t n;\n> @@ -622,24 +656,26 @@ static ssize_t sbefifo_write_common(struct sbefifo_client *client,\n>  \tif (!len)\n>  \t\treturn 0;\n>  \n> -\tn = sbefifo_buf_nbwriteable(&client->wbuf);\n> +\tsbefifo_get_client(client);\n> +\tn = sbefifo_buf_nbwriteable(wbuf);\n>  \n>  \tspin_lock_irq(&sbefifo->lock);\n> -\txfr = sbefifo_next_xfr(sbefifo);\n>  \n> +\txfr = sbefifo_next_xfr(sbefifo);\t/* next xfr to be executed */\n>  \tif ((client->f_flags & O_NONBLOCK) && xfr && n < len) {\n>  \t\tspin_unlock_irq(&sbefifo->lock);\n> -\t\treturn -EAGAIN;\n> +\t\tret = -EAGAIN;\n> +\t\tgoto out;\n>  \t}\n>  \n> -\txfr = sbefifo_enq_xfr(client);\n> -\tif (!xfr) {\n> +\txfr = sbefifo_enq_xfr(client);\t\t/* this xfr queued up */\n> +\tif (IS_ERR(xfr)) {\n>  \t\tspin_unlock_irq(&sbefifo->lock);\n> -\t\treturn -ENOMEM;\n> +\t\tret = PTR_ERR(xfr);\n> +\t\tgoto out;\n>  \t}\n> -\tspin_unlock_irq(&sbefifo->lock);\n>  \n> -\tsbefifo_get_client(client);\n> +\tspin_unlock_irq(&sbefifo->lock);\n>  \n>  \t/*\n>  \t * Partial writes are not really allowed in that EOT is sent exactly\n> @@ -647,49 +683,47 @@ static ssize_t sbefifo_write_common(struct sbefifo_client *client,\n>  \t */\n>  \twhile (len) {\n>  \t\tif (wait_event_interruptible(sbefifo->wait,\n> -\t\t\t\tREAD_ONCE(sbefifo->rc) ||\n> -\t\t\t\t\t(sbefifo_client_next_xfr(client) == xfr &&\n> -\t\t\t\t\t (n = sbefifo_buf_nbwriteable(\n> -\t\t\t\t\t\t\t&client->wbuf))))) {\n> +\t\t\t\t\t     sbefifo_write_ready(sbefifo, xfr,\n> +\t\t\t\t\t\t\t\t client,\n> +\t\t\t\t\t\t\t\t &n))) {\n>  \t\t\tset_bit(SBEFIFO_XFR_CANCEL, &xfr->flags);\n>  \t\t\tsbefifo_get(sbefifo);\n>  \t\t\tif (mod_timer(&sbefifo->poll_timer, jiffies))\n>  \t\t\t\tsbefifo_put(sbefifo);\n> -\n> -\t\t\tsbefifo_put_client(client);\n> -\t\t\treturn -ERESTARTSYS;\n> +\t\t\tret = -ERESTARTSYS;\n> +\t\t\tgoto out;\n>  \t\t}\n> -\t\tif (sbefifo->rc) {\n> +\n> +\t\tret = READ_ONCE(sbefifo->rc);\n> +\t\tif (ret) {\n>  \t\t\tINIT_LIST_HEAD(&client->xfrs);\n> -\t\t\tsbefifo_put_client(client);\n> -\t\t\treturn sbefifo->rc;\n> +\t\t\tgoto out;\n>  \t\t}\n>  \n>  \t\tn = min_t(size_t, n, len);\n>  \n>  \t\tif (ubuf) {\n> -\t\t\tif (copy_from_user(READ_ONCE(client->wbuf.wpos), ubuf,\n> -\t\t\t    n)) {\n> +\t\t\tif (copy_from_user(READ_ONCE(wbuf->wpos), ubuf, n)) {\n>  \t\t\t\tset_bit(SBEFIFO_XFR_CANCEL, &xfr->flags);\n>  \t\t\t\tsbefifo_get(sbefifo);\n>  \t\t\t\tif (mod_timer(&sbefifo->poll_timer, jiffies))\n>  \t\t\t\t\tsbefifo_put(sbefifo);\n> -\t\t\t\tsbefifo_put_client(client);\n> -\t\t\t\treturn -EFAULT;\n> +\t\t\t\tret = -EFAULT;\n> +\t\t\t\tgoto out;\n>  \t\t\t}\n>  \n>  \t\t\tubuf += n;\n>  \t\t} else {\n> -\t\t\tmemcpy(READ_ONCE(client->wbuf.wpos), kbuf, n);\n> +\t\t\tmemcpy(READ_ONCE(wbuf->wpos), kbuf, n);\n>  \t\t\tkbuf += n;\n>  \t\t}\n>  \n> -\t\tsbefifo_buf_wrotenb(&client->wbuf, n);\n> +\t\tsbefifo_buf_wrotenb(wbuf, n);\n>  \t\tlen -= n;\n>  \t\tret += n;\n>  \n> -\t\t/* set flag before starting the worker, as it may run through\n> -\t\t * and check the flag before we exit this loop!\n> +\t\t/* Set this before starting timer to avoid race condition on\n> +\t\t * this flag with the timer function writer.\n>  \t\t */\n>  \t\tif (!len)\n>  \t\t\tset_bit(SBEFIFO_XFR_WRITE_DONE, &xfr->flags);\n> @@ -702,21 +736,16 @@ static ssize_t sbefifo_write_common(struct sbefifo_client *client,\n>  \t\t\tsbefifo_put(sbefifo);\n>  \t}\n>  \n> +out:\n>  \tsbefifo_put_client(client);\n> -\n>  \treturn ret;\n>  }\n>  \n>  static ssize_t sbefifo_write(struct file *file, const char __user *buf,\n> -\t\tsize_t len, loff_t *offset)\n> +\t\t\t     size_t len, loff_t *offset)\n>  {\n>  \tstruct sbefifo_client *client = file->private_data;\n>  \n> -\tWARN_ON(*offset);\n> -\n> -\tif (!access_ok(VERIFY_READ, buf, len))\n> -\t\treturn -EFAULT;\n> -\n>  \treturn sbefifo_write_common(client, buf, NULL, len);\n>  }\n>  \n> @@ -742,18 +771,15 @@ static int sbefifo_release(struct inode *inode, struct file *file)\n>  struct sbefifo_client *sbefifo_drv_open(struct device *dev,\n>  \t\t\t\t\tunsigned long flags)\n>  {\n> -\tstruct sbefifo_client *client = NULL;\n> -\tstruct sbefifo *sbefifo;\n> -\tstruct fsi_device *fsi_dev = to_fsi_dev(dev);\n> +\tstruct sbefifo_client *client;\n> +\tstruct sbefifo *sbefifo = dev_get_drvdata(dev);\n>  \n> -\tlist_for_each_entry(sbefifo, &sbefifo_fifos, link) {\n> -\t\tif (sbefifo->fsi_dev != fsi_dev)\n> -\t\t\tcontinue;\n> +\tif (!sbefifo)\n> +\t\treturn NULL;\n>  \n> -\t\tclient = sbefifo_new_client(sbefifo);\n> -\t\tif (client)\n> -\t\t\tclient->f_flags = flags;\n> -\t}\n> +\tclient = sbefifo_new_client(sbefifo);\n> +\tif (client)\n> +\t\tclient->f_flags = flags;\n>  \n>  \treturn client;\n>  }\n> @@ -802,95 +828,92 @@ static int sbefifo_probe(struct device *dev)\n>  \tu32 sts;\n>  \tint ret, child_idx = 0;\n>  \n> -\tdev_dbg(dev, \"Found sbefifo device\\n\");\n>  \tsbefifo = kzalloc(sizeof(*sbefifo), GFP_KERNEL);\n>  \tif (!sbefifo)\n>  \t\treturn -ENOMEM;\n>  \n>  \tsbefifo->fsi_dev = fsi_dev;\n>  \n> -\tret = sbefifo_inw(sbefifo,\n> -\t\t\tSBEFIFO_UP | SBEFIFO_STS, &sts);\n> +\tret = sbefifo_inw(sbefifo, SBEFIFO_UP | SBEFIFO_STS, &sts);\n>  \tif (ret)\n>  \t\treturn ret;\n> +\n>  \tif (!(sts & SBEFIFO_EMPTY)) {\n> -\t\tdev_err(&sbefifo->fsi_dev->dev,\n> -\t\t\t\t\"Found data in upstream fifo\\n\");\n> +\t\tdev_err(dev, \"Found data in upstream fifo\\n\");\n>  \t\treturn -EIO;\n>  \t}\n>  \n>  \tret = sbefifo_inw(sbefifo, SBEFIFO_DWN | SBEFIFO_STS, &sts);\n>  \tif (ret)\n>  \t\treturn ret;\n> +\n>  \tif (!(sts & SBEFIFO_EMPTY)) {\n> -\t\tdev_err(&sbefifo->fsi_dev->dev,\n> -\t\t\t\t\"Found data in downstream fifo\\n\");\n> +\t\tdev_err(dev, \"found data in downstream fifo\\n\");\n>  \t\treturn -EIO;\n>  \t}\n>  \n> -\tsbefifo->mdev.minor = MISC_DYNAMIC_MINOR;\n> -\tsbefifo->mdev.fops = &sbefifo_fops;\n> -\tsbefifo->mdev.name = sbefifo->name;\n> -\tsbefifo->mdev.parent = dev;\n>  \tspin_lock_init(&sbefifo->lock);\n>  \tkref_init(&sbefifo->kref);\n> +\tinit_waitqueue_head(&sbefifo->wait);\n> +\tINIT_LIST_HEAD(&sbefifo->xfrs);\n>  \n>  \tsbefifo->idx = ida_simple_get(&sbefifo_ida, 1, INT_MAX, GFP_KERNEL);\n>  \tsnprintf(sbefifo->name, sizeof(sbefifo->name), \"sbefifo%d\",\n> -\t\t\tsbefifo->idx);\n> -\tinit_waitqueue_head(&sbefifo->wait);\n> -\tINIT_LIST_HEAD(&sbefifo->xfrs);\n> +\t\t sbefifo->idx);\n>  \n>  \t/* This bit of silicon doesn't offer any interrupts... */\n>  \tsetup_timer(&sbefifo->poll_timer, sbefifo_poll_timer,\n> -\t\t\t(unsigned long)sbefifo);\n> -\n> -\tif (dev->of_node) {\n> -\t\t/* create platform devs for dts child nodes (occ, etc) */\n> -\t\tfor_each_child_of_node(dev->of_node, np) {\n> -\t\t\tsnprintf(child_name, sizeof(child_name), \"%s-dev%d\",\n> -\t\t\t\t sbefifo->name, child_idx++);\n> -\t\t\tchild = of_platform_device_create(np, child_name, dev);\n> -\t\t\tif (!child)\n> -\t\t\t\tdev_warn(&sbefifo->fsi_dev->dev,\n> -\t\t\t\t\t \"failed to create child node dev\\n\");\n> -\t\t}\n> +\t\t    (unsigned long)sbefifo);\n> +\n> +\tsbefifo->mdev.minor = MISC_DYNAMIC_MINOR;\n> +\tsbefifo->mdev.fops = &sbefifo_fops;\n> +\tsbefifo->mdev.name = sbefifo->name;\n> +\tsbefifo->mdev.parent = dev;\n> +\tret = misc_register(&sbefifo->mdev);\n> +\tif (ret) {\n> +\t\tdev_err(dev, \"failed to register miscdevice: %d\\n\", ret);\n> +\t\tida_simple_remove(&sbefifo_ida, sbefifo->idx);\n> +\t\tsbefifo_put(sbefifo);\n> +\t\treturn ret;\n> +\t}\n> +\n> +\t/* create platform devs for dts child nodes (occ, etc) */\n> +\tfor_each_available_child_of_node(dev->of_node, np) {\n> +\t\tsnprintf(child_name, sizeof(child_name), \"%s-dev%d\",\n> +\t\t\t sbefifo->name, child_idx++);\n> +\t\tchild = of_platform_device_create(np, child_name, dev);\n> +\t\tif (!child)\n> +\t\t\tdev_warn(dev, \"failed to create child %s dev\\n\",\n> +\t\t\t\t child_name);\n>  \t}\n>  \n> -\tlist_add(&sbefifo->link, &sbefifo_fifos);\n> -\t\n> -\treturn misc_register(&sbefifo->mdev);\n> +\tdev_set_drvdata(dev, sbefifo);\n> +\n> +\treturn 0;\n>  }\n>  \n>  static int sbefifo_remove(struct device *dev)\n>  {\n> -\tstruct fsi_device *fsi_dev = to_fsi_dev(dev);\n> -\tstruct sbefifo *sbefifo, *sbefifo_tmp;\n> +\tstruct sbefifo *sbefifo = dev_get_drvdata(dev);\n>  \tstruct sbefifo_xfr *xfr;\n>  \n> -\tlist_for_each_entry_safe(sbefifo, sbefifo_tmp, &sbefifo_fifos, link) {\n> -\t\tif (sbefifo->fsi_dev != fsi_dev)\n> -\t\t\tcontinue;\n> -\n> -\t\tdevice_for_each_child(dev, NULL, sbefifo_unregister_child);\n> +\tWRITE_ONCE(sbefifo->rc, -ENODEV);\n> +\twake_up(&sbefifo->wait);\n>  \n> -\t\tmisc_deregister(&sbefifo->mdev);\n> -\t\tlist_del(&sbefifo->link);\n> -\t\tida_simple_remove(&sbefifo_ida, sbefifo->idx);\n> +\tmisc_deregister(&sbefifo->mdev);\n> +\tdevice_for_each_child(dev, NULL, sbefifo_unregister_child);\n>  \n> -\t\tif (del_timer_sync(&sbefifo->poll_timer))\n> -\t\t\tsbefifo_put(sbefifo);\n> +\tida_simple_remove(&sbefifo_ida, sbefifo->idx);\n>  \n> -\t\tspin_lock(&sbefifo->lock);\n> -\t\tlist_for_each_entry(xfr, &sbefifo->xfrs, xfrs)\n> -\t\t\tkfree(xfr);\n> -\t\tspin_unlock(&sbefifo->lock);\n> +\tif (del_timer_sync(&sbefifo->poll_timer))\n> +\t\tsbefifo_put(sbefifo);\n>  \n> -\t\tWRITE_ONCE(sbefifo->rc, -ENODEV);\n> +\tspin_lock(&sbefifo->lock);\n> +\tlist_for_each_entry(xfr, &sbefifo->xfrs, xfrs)\n> +\t\tkfree(xfr);\n> +\tspin_unlock(&sbefifo->lock);\n\nDo we actually need to lock on this? Shouldn't we have torn down anything that\nmight be concurrently accessing ->xfrs?\n\n>  \n> -\t\twake_up(&sbefifo->wait);\n> -\t\tsbefifo_put(sbefifo);\n> -\t}\n> +\tsbefifo_put(sbefifo);\n>  \n>  \treturn 0;\n>  }\n> @@ -915,17 +938,19 @@ static int sbefifo_remove(struct device *dev)\n>  \n>  static int sbefifo_init(void)\n>  {\n> -\tINIT_LIST_HEAD(&sbefifo_fifos);\n>  \treturn fsi_driver_register(&sbefifo_drv);\n>  }\n>  \n>  static void sbefifo_exit(void)\n>  {\n>  \tfsi_driver_unregister(&sbefifo_drv);\n> +\n> +\tida_destroy(&sbefifo_ida);\n>  }\n>  \n>  module_init(sbefifo_init);\n>  module_exit(sbefifo_exit);\n>  MODULE_LICENSE(\"GPL\");\n>  MODULE_AUTHOR(\"Brad Bishop \");\n> -MODULE_DESCRIPTION(\"Linux device interface to the POWER self boot engine\");\n> +MODULE_AUTHOR(\"Eddie James \");\n> +MODULE_DESCRIPTION(\"Linux device interface to the POWER Self Boot Engine\");\n> diff --git a/include/linux/fsi-sbefifo.h b/include/linux/fsi-sbefifo.h\n> index 1b46c63..8e55891 100644\n> --- a/include/linux/fsi-sbefifo.h\n> +++ b/include/linux/fsi-sbefifo.h\n> @@ -13,8 +13,8 @@\n>   * GNU General Public License for more details.\n>   */\n>  \n> -#ifndef __FSI_SBEFIFO_H__\n> -#define __FSI_SBEFIFO_H__\n> +#ifndef LINUX_FSI_SBEFIFO_H\n> +#define LINUX_FSI_SBEFIFO_H\n>  \n>  struct device;\n>  struct sbefifo_client;\n> @@ -27,4 +27,4 @@ extern int sbefifo_drv_write(struct sbefifo_client *client, const char *buf,\n>  \t\t\t     size_t len);\n>  extern void sbefifo_drv_release(struct sbefifo_client *client);\n>  \n> -#endif /* __FSI_SBEFIFO_H__ */\n> +#endif /* LINUX_FSI_SBEFIFO_H */\n\nCheers,\n\nAndrew","headers":{"Return-Path":"","X-Original-To":["incoming@patchwork.ozlabs.org","openbmc@lists.ozlabs.org"],"Delivered-To":["patchwork-incoming@bilbo.ozlabs.org","openbmc@lists.ozlabs.org"],"Received":["from lists.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3])\n\t(using TLSv1.2 with cipher ADH-AES256-GCM-SHA384 (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3y0sJz2tLcz9t3t\n\tfor ;\n\tMon, 25 Sep 2017 14:57:39 +1000 (AEST)","from lists.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3])\n\tby lists.ozlabs.org (Postfix) with ESMTP id 3y0sJz07dhzDsNc\n\tfor ;\n\tMon, 25 Sep 2017 14:57:39 +1000 (AEST)","from out1-smtp.messagingengine.com (out1-smtp.messagingengine.com\n\t[66.111.4.25])\n\t(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby lists.ozlabs.org (Postfix) with ESMTPS id 3y0sJh2Br7zDr4N\n\tfor ; Mon, 25 Sep 2017 14:57:23 +1000 (AEST)","from compute4.internal (compute4.nyi.internal [10.202.2.44])\n\tby mailout.nyi.internal (Postfix) with ESMTP id 8EBAD20DBD;\n\tMon, 25 Sep 2017 00:57:20 -0400 (EDT)","from frontend1 ([10.202.2.160])\n\tby compute4.internal (MEProxy); Mon, 25 Sep 2017 00:57:20 -0400","from keelia16 (unknown [203.0.153.9])\n\tby mail.messagingengine.com (Postfix) with ESMTPA id 865847F955;\n\tMon, 25 Sep 2017 00:57:18 -0400 (EDT)"],"Authentication-Results":["ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=aj.id.au header.i=@aj.id.au header.b=\"WZY8G3HO\";\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=messagingengine.com\n\theader.i=@messagingengine.com header.b=\"ODt+K6Zp\"; \n\tdkim-atps=neutral","lists.ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=aj.id.au header.i=@aj.id.au header.b=\"WZY8G3HO\";\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=messagingengine.com\n\theader.i=@messagingengine.com header.b=\"ODt+K6Zp\"; \n\tdkim-atps=neutral","ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=aj.id.au\n\t(client-ip=66.111.4.25; helo=out1-smtp.messagingengine.com;\n\tenvelope-from=andrew@aj.id.au; receiver=)","lists.ozlabs.org; dkim=pass (2048-bit key;\n\tunprotected) header.d=aj.id.au header.i=@aj.id.au header.b=\"WZY8G3HO\";\n\tdkim=pass (2048-bit key;\n\tunprotected) header.d=messagingengine.com\n\theader.i=@messagingengine.com\n\theader.b=\"ODt+K6Zp\"; dkim-atps=neutral"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/relaxed; d=aj.id.au; h=cc\n\t:content-type:date:from:in-reply-to:message-id:mime-version\n\t:references:subject:to:x-me-sender:x-me-sender:x-sasl-enc\n\t:x-sasl-enc; s=fm1; bh=1/q7Mtr/LY8I44oDTyFSp2V19Z8Fx8YvpTGs3gI0X\n\tNo=; b=WZY8G3HOoTRq1IM/tBPWdekpJyZ90ZIEnsEZI9C5LkDKnUjWVca1218C6\n\toXgvmzcwpwi8O2U61omnxBI25GA8kUMMjrzUdR8pE0u1gpDtQAjMUeofSs7nCLsp\n\t/LaoVBcoqo+eKkG22aEebGZM2dHlksIPSdnxjOPQHtmadoRTsecIrbnDAXQzxii0\n\t8prKADKIGEJVj+IUp/PKG6Tsc072WQNvm+efxA4qNqUR4byXRaKJpT3oqSTpQl/S\n\tJyWjUpibl8I/agz1NPWo3DPbRGX9FblkHZ4v0XJYM5HU4UWsaxUdgWh/4kpjfQKO\n\tikifSbURf6wYF936GhFJvCvTOVcMw==","v=1; a=rsa-sha256; c=relaxed/relaxed; d=\n\tmessagingengine.com; h=cc:content-type:date:from:in-reply-to\n\t:message-id:mime-version:references:subject:to:x-me-sender\n\t:x-me-sender:x-sasl-enc:x-sasl-enc; s=fm1; bh=1/q7Mtr/LY8I44oDTy\n\tFSp2V19Z8Fx8YvpTGs3gI0XNo=; b=ODt+K6ZpaN8gdVh5Qhd8646bANgaOXUxv2\n\tlrWqMIjIFHUiHrCCtWSm49Jj4MsbC3l4drHOPdI56OPM8jxwsguY7Fm9YIMnZc4G\n\tHB8jvKiBN3elqBxoReT8DtV5N0/X+5QohJzPQSGljErZfOxp8NwKN+yVJMCGNDks\n\tn6OOkXAZLxEDdbPhGxrywTLncPBq4Dl04YvxybMa8yZXS3LZ4my3GdOEgnvbAh47\n\tbesrUhvFNTQxgop7yqJcneAbV0wBebtlFQaAgNvDQOmRKRxnUQNEE6rFqAlYziV2\n\tXCtZCd4bDMbjYDYctOWaUuHDhMfnwlmaUBzypNJMljdsqvh8roKQ=="],"X-ME-Sender":"","X-Sasl-enc":"zZIu2zmXjB///HXB/X9vMfUVk/m89mgiaPdx67rR/fwJ 1506315439","Message-ID":"<1506315435.30138.17.camel@aj.id.au>","Subject":"Re: [PATCH linux dev-4.10 1/3] drivers/fsi/sbefifo: refactor to\n\tupstream list state","From":"Andrew Jeffery ","To":"Eddie James , openbmc@lists.ozlabs.org","Date":"Mon, 25 Sep 2017 14:27:15 +0930","In-Reply-To":"<1506033838-2078-2-git-send-email-eajames@linux.vnet.ibm.com>","References":"<1506033838-2078-1-git-send-email-eajames@linux.vnet.ibm.com>\n\t<1506033838-2078-2-git-send-email-eajames@linux.vnet.ibm.com>","Content-Type":"multipart/signed; micalg=\"pgp-sha512\";\n\tprotocol=\"application/pgp-signature\";\n\tboundary=\"=-JlP3dEvGs2yh95Rrf4iH\"","X-Mailer":"Evolution 3.22.6-1ubuntu1 ","Mime-Version":"1.0","X-BeenThere":"openbmc@lists.ozlabs.org","X-Mailman-Version":"2.1.24","Precedence":"list","List-Id":"Development list for OpenBMC ","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"\"Edward A. James\" ","Errors-To":"openbmc-bounces+incoming=patchwork.ozlabs.org@lists.ozlabs.org","Sender":"\"openbmc\"\n\t"}},{"id":1774429,"web_url":"http://patchwork.ozlabs.org/comment/1774429/","msgid":"<1506318388.30138.20.camel@aj.id.au>","list_archive_url":null,"date":"2017-09-25T05:46:28","subject":"Re: [PATCH linux dev-4.10 1/3] drivers/fsi/sbefifo: refactor to\n\tupstream list state","submitter":{"id":68332,"url":"http://patchwork.ozlabs.org/api/people/68332/","name":"Andrew Jeffery","email":"andrew@aj.id.au"},"content":"On Mon, 2017-09-25 at 14:27 +0930, Andrew Jeffery wrote:\n> >  static int sbefifo_remove(struct device *dev)\n> >  {\n> > -     struct fsi_device *fsi_dev = to_fsi_dev(dev);\n> > -     struct sbefifo *sbefifo, *sbefifo_tmp;\n> > +     struct sbefifo *sbefifo = dev_get_drvdata(dev);\n> >       struct sbefifo_xfr *xfr;\n> >  \n> > -     list_for_each_entry_safe(sbefifo, sbefifo_tmp, &sbefifo_fifos, link) {\n> > -             if (sbefifo->fsi_dev != fsi_dev)\n> > -                     continue;\n> > -\n> > -             device_for_each_child(dev, NULL, sbefifo_unregister_child);\n> > +     WRITE_ONCE(sbefifo->rc, -ENODEV);\n> > +     wake_up(&sbefifo->wait);\n\nAlso, do we want wake_up_all() here? Or does it not matter (exclusive\nvs non-exclusive)?","headers":{"Return-Path":"","X-Original-To":["incoming@patchwork.ozlabs.org","openbmc@lists.ozlabs.org"],"Delivered-To":["patchwork-incoming@bilbo.ozlabs.org","openbmc@lists.ozlabs.org"],"Received":["from lists.ozlabs.org (lists.ozlabs.org [103.22.144.68])\n\t(using TLSv1.2 with cipher ADH-AES256-GCM-SHA384 (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3y0tPb2hYdz9t4B\n\tfor ;\n\tMon, 25 Sep 2017 15:46:43 +1000 (AEST)","from lists.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3])\n\tby lists.ozlabs.org (Postfix) with ESMTP id 3y0tPb1XBMzDsNc\n\tfor ;\n\tMon, 25 Sep 2017 15:46:43 +1000 (AEST)","from out1-smtp.messagingengine.com (out1-smtp.messagingengine.com\n\t[66.111.4.25])\n\t(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby lists.ozlabs.org (Postfix) with ESMTPS id 3y0tPS4kGdzDr4N\n\tfor ; Mon, 25 Sep 2017 15:46:36 +1000 (AEST)","from compute4.internal (compute4.nyi.internal [10.202.2.44])\n\tby mailout.nyi.internal (Postfix) with ESMTP id E0DAE20C31;\n\tMon, 25 Sep 2017 01:46:33 -0400 (EDT)","from frontend1 ([10.202.2.160])\n\tby compute4.internal (MEProxy); Mon, 25 Sep 2017 01:46:33 -0400","from keelia16 (unknown [203.0.153.9])\n\tby mail.messagingengine.com (Postfix) with ESMTPA id 214FC7E187;\n\tMon, 25 Sep 2017 01:46:31 -0400 (EDT)"],"Authentication-Results":["ozlabs.org; dkim=pass (2048-bit key;\n\tunprotected) header.d=aj.id.au header.i=@aj.id.au header.b=\"QCG/AmGc\";\n\tdkim=pass (2048-bit key;\n\tunprotected) header.d=messagingengine.com\n\theader.i=@messagingengine.com header.b=\"gT5XvIZe\"; \n\tdkim-atps=neutral","lists.ozlabs.org; dkim=pass (2048-bit key;\n\tunprotected) header.d=aj.id.au header.i=@aj.id.au header.b=\"QCG/AmGc\";\n\tdkim=pass (2048-bit key;\n\tunprotected) header.d=messagingengine.com\n\theader.i=@messagingengine.com header.b=\"gT5XvIZe\"; \n\tdkim-atps=neutral","ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=aj.id.au\n\t(client-ip=66.111.4.25; helo=out1-smtp.messagingengine.com;\n\tenvelope-from=andrew@aj.id.au; receiver=)","lists.ozlabs.org; dkim=pass (2048-bit key;\n\tunprotected) header.d=aj.id.au header.i=@aj.id.au header.b=\"QCG/AmGc\";\n\tdkim=pass (2048-bit key;\n\tunprotected) header.d=messagingengine.com\n\theader.i=@messagingengine.com\n\theader.b=\"gT5XvIZe\"; dkim-atps=neutral"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/relaxed; d=aj.id.au; h=cc\n\t:content-type:date:from:in-reply-to:message-id:mime-version\n\t:references:subject:to:x-me-sender:x-me-sender:x-sasl-enc\n\t:x-sasl-enc; s=fm1; bh=TzwAvZNh1CKr20zrvTbkXmZET8pHEHpk3UJ/Byjo7\n\tac=; b=QCG/AmGcJ0ZrFvoc/2YsuV7qG+bgIxQ83XmYZgelIj07R2Eppb6j8RbYr\n\tfLlV8j+B/12wtczJuYCQXQZUQ4HWjJepZQTlOU+PRbyAIHMVzSvx6zwMUtmZApjf\n\tJSsEbS1PawlXHE/QLmD90nJCo432P3Gd/TBkMWUN/3fcr78a9IGNFBAtksQFwhkC\n\tGU+MXvf8Tizcdd4u/DUcaCHU3LEd7DdiZsg37pecMDbVXnR/ZDaCHxuErJwuroKE\n\tMrKGTngDySoOKhrJltileN/OckDekOSEjTfxuRuWgs4hAJJrRldAiENIVlhhp6mA\n\t4pePbHrVWTEZeXKT2DgD8XekZTCsw==","v=1; a=rsa-sha256; c=relaxed/relaxed; d=\n\tmessagingengine.com; h=cc:content-type:date:from:in-reply-to\n\t:message-id:mime-version:references:subject:to:x-me-sender\n\t:x-me-sender:x-sasl-enc:x-sasl-enc; s=fm1; bh=TzwAvZNh1CKr20zrvT\n\tbkXmZET8pHEHpk3UJ/Byjo7ac=; b=gT5XvIZeuZH94PG1xnJqMSgCIdkVeMkwOt\n\tqs9s92X2/qav27moxhW1WqKk++/xn7g40CKGit0RbjQfsr+ZLgd82jiwDE/qQC82\n\tHTyv5aunHy/cdxcZNALt1/Ag7yOCRDraASJShIOH4YlN2c1/KIOwpXDHj7u5z86i\n\tSNGUwdJ2qNTw2gINo8KvnJvOe+lbncrhOFbPwZyiZMo08G3F0NznYGUm1KGo/khj\n\tQfZI2v8AnXE4Hrgx+rKuzb2GT0jqeYCfiuZQSaHTJatF8dKNG6ap/14Vj6MApxr/\n\tvNLkj7NUIT4GDNhxY419OhVao4oiZTvpsjG+pS1QktrdA+DwtIdw=="],"X-ME-Sender":"","X-Sasl-enc":"A96BJ1DaA71sm4BeciSpjvh4uVFcH3QHy61VqqmaEeGe 1506318393","Message-ID":"<1506318388.30138.20.camel@aj.id.au>","Subject":"Re: [PATCH linux dev-4.10 1/3] drivers/fsi/sbefifo: refactor to\n\tupstream list state","From":"Andrew Jeffery ","To":"Eddie James , openbmc@lists.ozlabs.org","Date":"Mon, 25 Sep 2017 15:16:28 +0930","In-Reply-To":"<1506315435.30138.17.camel@aj.id.au>","References":"<1506033838-2078-1-git-send-email-eajames@linux.vnet.ibm.com>\n\t<1506033838-2078-2-git-send-email-eajames@linux.vnet.ibm.com>\n\t<1506315435.30138.17.camel@aj.id.au>","Content-Type":"multipart/signed; micalg=\"pgp-sha512\";\n\tprotocol=\"application/pgp-signature\";\n\tboundary=\"=-HBO/imkyvNnw58T9JgGM\"","X-Mailer":"Evolution 3.22.6-1ubuntu1 ","Mime-Version":"1.0","X-BeenThere":"openbmc@lists.ozlabs.org","X-Mailman-Version":"2.1.24","Precedence":"list","List-Id":"Development list for OpenBMC ","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"\"Edward A. James\" ","Errors-To":"openbmc-bounces+incoming=patchwork.ozlabs.org@lists.ozlabs.org","Sender":"\"openbmc\"\n\t"}},{"id":1774438,"web_url":"http://patchwork.ozlabs.org/comment/1774438/","msgid":"<1506320183.30138.22.camel@aj.id.au>","list_archive_url":null,"date":"2017-09-25T06:16:23","subject":"Re: [PATCH linux dev-4.10 1/3] drivers/fsi/sbefifo: refactor to\n\tupstream list state","submitter":{"id":68332,"url":"http://patchwork.ozlabs.org/api/people/68332/","name":"Andrew Jeffery","email":"andrew@aj.id.au"},"content":"Hi, it's me again.\n\nOn Mon, 2017-09-25 at 14:27 +0930, Andrew Jeffery wrote:\n> >  static int sbefifo_remove(struct device *dev)\n> >  {\n> > -     struct fsi_device *fsi_dev = to_fsi_dev(dev);\n> > -     struct sbefifo *sbefifo, *sbefifo_tmp;\n> > +     struct sbefifo *sbefifo = dev_get_drvdata(dev);\n> >       struct sbefifo_xfr *xfr;\n> >  \n> > -     list_for_each_entry_safe(sbefifo, sbefifo_tmp, &sbefifo_fifos, link) {\n> > -             if (sbefifo->fsi_dev != fsi_dev)\n> > -                     continue;\n> > -\n> > -             device_for_each_child(dev, NULL, sbefifo_unregister_child);\n> > +     WRITE_ONCE(sbefifo->rc, -ENODEV);\n> > +     wake_up(&sbefifo->wait);\n> >  \n> > -             misc_deregister(&sbefifo->mdev);\n> > -             list_del(&sbefifo->link);\n> > -             ida_simple_remove(&sbefifo_ida, sbefifo->idx);\n> > +     misc_deregister(&sbefifo->mdev);\n\nI think we should do this before the wake_up() to ensure that we wake\nall the submitted jobs. Otherwise we might miss some.\n\n> > +     device_for_each_child(dev, NULL, sbefifo_unregister_child);\n> >  \n> > -             if (del_timer_sync(&sbefifo->poll_timer))\n> > -                     sbefifo_put(sbefifo);\n> > +     ida_simple_remove(&sbefifo_ida, sbefifo->idx);\n> >  \n> > -             spin_lock(&sbefifo->lock);\n> > -             list_for_each_entry(xfr, &sbefifo->xfrs, xfrs)\n> > -                     kfree(xfr);\n> > -             spin_unlock(&sbefifo->lock);\n> > +     if (del_timer_sync(&sbefifo->poll_timer))\n> > +             sbefifo_put(sbefifo);\n\nShouldn't we be doing this before WRITE_ONCE(sbefifo->rc, -ENODEV) to\nensure we avoid a race on the value of ->rc? Otherwise the timer could\nfire between the WRITE_ONCE() and here and potentially write ->rc or\ncause another job to hang in the FIFO.","headers":{"Return-Path":"","X-Original-To":["incoming@patchwork.ozlabs.org","openbmc@lists.ozlabs.org"],"Delivered-To":["patchwork-incoming@bilbo.ozlabs.org","openbmc@lists.ozlabs.org"],"Received":["from lists.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3])\n\t(using TLSv1.2 with cipher ADH-AES256-GCM-SHA384 (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3y0v4970fgz9tXD\n\tfor ;\n\tMon, 25 Sep 2017 16:16:41 +1000 (AEST)","from lists.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3])\n\tby lists.ozlabs.org (Postfix) with ESMTP id 3y0v495pt6zDsNc\n\tfor ;\n\tMon, 25 Sep 2017 16:16:41 +1000 (AEST)","from out1-smtp.messagingengine.com (out1-smtp.messagingengine.com\n\t[66.111.4.25])\n\t(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby lists.ozlabs.org (Postfix) with ESMTPS id 3y0v430s7kzDr4N\n\tfor ; Mon, 25 Sep 2017 16:16:33 +1000 (AEST)","from compute4.internal (compute4.nyi.internal [10.202.2.44])\n\tby mailout.nyi.internal (Postfix) with ESMTP id 9F43C20DEF;\n\tMon, 25 Sep 2017 02:16:29 -0400 (EDT)","from frontend2 ([10.202.2.161])\n\tby compute4.internal (MEProxy); Mon, 25 Sep 2017 02:16:29 -0400","from keelia16 (unknown [203.0.153.9])\n\tby mail.messagingengine.com (Postfix) with ESMTPA id 965A12479F;\n\tMon, 25 Sep 2017 02:16:27 -0400 (EDT)"],"Authentication-Results":["ozlabs.org; dkim=pass (2048-bit key;\n\tunprotected) header.d=aj.id.au header.i=@aj.id.au header.b=\"Fp4JDLua\";\n\tdkim=pass (2048-bit key;\n\tunprotected) header.d=messagingengine.com\n\theader.i=@messagingengine.com header.b=\"HQaJhLI7\"; \n\tdkim-atps=neutral","lists.ozlabs.org; dkim=pass (2048-bit key;\n\tunprotected) header.d=aj.id.au header.i=@aj.id.au header.b=\"Fp4JDLua\";\n\tdkim=pass (2048-bit key;\n\tunprotected) header.d=messagingengine.com\n\theader.i=@messagingengine.com header.b=\"HQaJhLI7\"; \n\tdkim-atps=neutral","ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=aj.id.au\n\t(client-ip=66.111.4.25; helo=out1-smtp.messagingengine.com;\n\tenvelope-from=andrew@aj.id.au; receiver=)","lists.ozlabs.org; dkim=pass (2048-bit key;\n\tunprotected) header.d=aj.id.au header.i=@aj.id.au header.b=\"Fp4JDLua\";\n\tdkim=pass (2048-bit key;\n\tunprotected) header.d=messagingengine.com\n\theader.i=@messagingengine.com\n\theader.b=\"HQaJhLI7\"; dkim-atps=neutral"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/relaxed; d=aj.id.au; h=cc\n\t:content-type:date:from:in-reply-to:message-id:mime-version\n\t:references:subject:to:x-me-sender:x-me-sender:x-sasl-enc\n\t:x-sasl-enc; s=fm1; bh=SfORAXe6YZ84gCKpbTiFuiQjAiLc1jXTGNyJL/s7w\n\tcA=; b=Fp4JDLuaMu+ilL3102dfkkfBd+ziF7YIFtX1KY+HIUB272ZttzUxhjw9S\n\tc0du4W9J8pvMo4MZkM/3mDqTGUIdMweZU8r2g4hlVnRgspMQ/cHPjknfdOB+cY9F\n\tYQ9OOfXF9/tAOWO9Tp6RW4wn5tFPAeyM6P5EPGJTWvGM69/bWZiDbUTUFPBSbCFt\n\tRS3EqLb0yk8eTvtw2Oqx5hxiYfIzviUCLjRfuSjpf5yt1NhFMojSTT2e/79se3Xl\n\tuZC9F9OzTG+p2MNrQcXKE4Tp7m3b8rK2ozqhf3Qr8denV1MlHi2VSWkKCh5gTmUP\n\tLlt4ehT0Na0UksqB+Md60EjwBi8ZQ==","v=1; a=rsa-sha256; c=relaxed/relaxed; d=\n\tmessagingengine.com; h=cc:content-type:date:from:in-reply-to\n\t:message-id:mime-version:references:subject:to:x-me-sender\n\t:x-me-sender:x-sasl-enc:x-sasl-enc; s=fm1; bh=SfORAXe6YZ84gCKpbT\n\tiFuiQjAiLc1jXTGNyJL/s7wcA=; b=HQaJhLI7V82mx/bNsCTmQVg+FdGLZfopBy\n\tYL8WGj4vEkjf7bhDhW/N7cKwC+JDD0aSh8LoZXu3ZaS+huIPiFqYUu/ECIZPSNQw\n\tqFQhQrKkUYVXXJw8obdPxc1BfagM/ctO/l90wtyZo/iXaNn+/TsgZ/ZqhO27h2GJ\n\tBpARM8LxJcN835ZsIuzskdb2otEPt6jn+jEHX3rPixgvWX1e35LzdbYq5pQMROcZ\n\t/GMdca1D6RnE98RsyT2yWASOViS2DL0AnT3RlsCrFeuw3MsGLa0acbg83zwfSgQw\n\tzobgglvzX8xoTFWO4GhsARgSvRQtZszuGPzbn3CjXIiqWRquW76Q=="],"X-ME-Sender":"","X-Sasl-enc":"9X6uRh6YUCqHgvGBi75B7KKK0VxOM8WMCgbGucXP/v7Z 1506320188","Message-ID":"<1506320183.30138.22.camel@aj.id.au>","Subject":"Re: [PATCH linux dev-4.10 1/3] drivers/fsi/sbefifo: refactor to\n\tupstream list state","From":"Andrew Jeffery ","To":"Eddie James , openbmc@lists.ozlabs.org","Date":"Mon, 25 Sep 2017 15:46:23 +0930","In-Reply-To":"<1506315435.30138.17.camel@aj.id.au>","References":"<1506033838-2078-1-git-send-email-eajames@linux.vnet.ibm.com>\n\t<1506033838-2078-2-git-send-email-eajames@linux.vnet.ibm.com>\n\t<1506315435.30138.17.camel@aj.id.au>","Content-Type":"multipart/signed; micalg=\"pgp-sha512\";\n\tprotocol=\"application/pgp-signature\";\n\tboundary=\"=-iqXaM3BuT/jm4TPKfhiP\"","X-Mailer":"Evolution 3.22.6-1ubuntu1 ","Mime-Version":"1.0","X-BeenThere":"openbmc@lists.ozlabs.org","X-Mailman-Version":"2.1.24","Precedence":"list","List-Id":"Development list for OpenBMC ","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"\"Edward A. James\" ","Errors-To":"openbmc-bounces+incoming=patchwork.ozlabs.org@lists.ozlabs.org","Sender":"\"openbmc\"\n\t"}},{"id":1777828,"web_url":"http://patchwork.ozlabs.org/comment/1777828/","msgid":"<92ba5ba1-77f2-ede6-6e2c-901777902ccd@linux.vnet.ibm.com>","list_archive_url":null,"date":"2017-09-29T22:44:55","subject":"Re: [PATCH linux dev-4.10 1/3] drivers/fsi/sbefifo: refactor to\n\tupstream list state","submitter":{"id":70876,"url":"http://patchwork.ozlabs.org/api/people/70876/","name":"Eddie James","email":"eajames@linux.vnet.ibm.com"},"content":"On 09/25/2017 01:16 AM, Andrew Jeffery wrote:\n> Hi, it's me again.\n>\n> On Mon, 2017-09-25 at 14:27 +0930, Andrew Jeffery wrote:\n>>> static int sbefifo_remove(struct device *dev)\n>>> {\n>>> - struct fsi_device *fsi_dev = to_fsi_dev(dev);\n>>> - struct sbefifo *sbefifo, *sbefifo_tmp;\n>>> + struct sbefifo *sbefifo = dev_get_drvdata(dev);\n>>> struct sbefifo_xfr *xfr;\n>>> \n>>> - list_for_each_entry_safe(sbefifo, sbefifo_tmp, &sbefifo_fifos, link) {\n>>> - if (sbefifo->fsi_dev != fsi_dev)\n>>> - continue;\n>>> -\n>>> - device_for_each_child(dev, NULL, sbefifo_unregister_child);\n>>> + WRITE_ONCE(sbefifo->rc, -ENODEV);\n>>> + wake_up(&sbefifo->wait);\n>>> \n>>> - misc_deregister(&sbefifo->mdev);\n>>> - list_del(&sbefifo->link);\n>>> - ida_simple_remove(&sbefifo_ida, sbefifo->idx);\n>>> + misc_deregister(&sbefifo->mdev);\n> I think we should do this before the wake_up() to ensure that we wake\n> all the submitted jobs. Otherwise we might miss some.\n\nProblem is that if the child devices (occ, for example) don't have a \ncancel function, then we would hang waiting for stuff to wake up in \ntheir remove() functions. So I think we need to set rc and wake_up \nfirst, then start unregistering clients.\n\n>\n>>> + device_for_each_child(dev, NULL, sbefifo_unregister_child);\n>>> \n>>> - if (del_timer_sync(&sbefifo->poll_timer))\n>>> - sbefifo_put(sbefifo);\n>>> + ida_simple_remove(&sbefifo_ida, sbefifo->idx);\n>>> \n>>> - spin_lock(&sbefifo->lock);\n>>> - list_for_each_entry(xfr, &sbefifo->xfrs, xfrs)\n>>> - kfree(xfr);\n>>> - spin_unlock(&sbefifo->lock);\n>>> + if (del_timer_sync(&sbefifo->poll_timer))\n>>> + sbefifo_put(sbefifo);\n> Shouldn't we be doing this before WRITE_ONCE(sbefifo->rc, -ENODEV) to\n> ensure we avoid a race on the value of ->rc? Otherwise the timer could\n> fire between the WRITE_ONCE() and here and potentially write ->rc or\n> cause another job to hang in the FIFO.\n\nrc is only ever written with error values, fortunately, so I'm not too \nconcerned about it.\n\nThanks,\nEddie","headers":{"Return-Path":"","X-Original-To":["incoming@patchwork.ozlabs.org","openbmc@lists.ozlabs.org"],"Delivered-To":["patchwork-incoming@bilbo.ozlabs.org","openbmc@lists.ozlabs.org"],"Received":["from lists.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3])\n\t(using TLSv1.2 with cipher ADH-AES256-GCM-SHA384 (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3y3mpp3l9Sz9t6B\n\tfor ;\n\tSat, 30 Sep 2017 08:45:06 +1000 (AEST)","from lists.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3])\n\tby lists.ozlabs.org (Postfix) with ESMTP id 3y3mpp2sBvzDqgl\n\tfor ;\n\tSat, 30 Sep 2017 08:45:06 +1000 (AEST)","from mx0a-001b2d01.pphosted.com (mx0a-001b2d01.pphosted.com\n\t[148.163.156.1])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256\n\tbits)) (No client certificate requested)\n\tby lists.ozlabs.org (Postfix) with ESMTPS id 3y3mpk4BrCzDqG2\n\tfor ; Sat, 30 Sep 2017 08:45:02 +1000 (AEST)","from pps.filterd (m0098399.ppops.net [127.0.0.1])\n\tby mx0a-001b2d01.pphosted.com (8.16.0.21/8.16.0.21) with SMTP id\n\tv8TMixNO137038\n\tfor ; Fri, 29 Sep 2017 18:45:00 -0400","from e37.co.us.ibm.com (e37.co.us.ibm.com [32.97.110.158])\n\tby mx0a-001b2d01.pphosted.com with ESMTP id 2d9vchewvg-1\n\t(version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT)\n\tfor ; Fri, 29 Sep 2017 18:45:00 -0400","from localhost\n\tby e37.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use\n\tOnly! Violators will be prosecuted\n\tfor from ;\n\tFri, 29 Sep 2017 16:44:59 -0600","from b03cxnp07028.gho.boulder.ibm.com (9.17.130.15)\n\tby e37.co.us.ibm.com (192.168.1.137) with IBM ESMTP SMTP Gateway:\n\tAuthorized Use Only! Violators will be prosecuted; \n\tFri, 29 Sep 2017 16:44:57 -0600","from b03ledav006.gho.boulder.ibm.com\n\t(b03ledav006.gho.boulder.ibm.com [9.17.130.237])\n\tby b03cxnp07028.gho.boulder.ibm.com (8.14.9/8.14.9/NCO v10.0) with\n\tESMTP id v8TMiukB2032004; Fri, 29 Sep 2017 15:44:56 -0700","from b03ledav006.gho.boulder.ibm.com (unknown [127.0.0.1])\n\tby IMSVA (Postfix) with ESMTP id 8B906C603C;\n\tFri, 29 Sep 2017 16:44:56 -0600 (MDT)","from oc3016140333.ibm.com (unknown [9.85.183.77])\n\tby b03ledav006.gho.boulder.ibm.com (Postfix) with ESMTP id D762DC6037;\n\tFri, 29 Sep 2017 16:44:55 -0600 (MDT)"],"Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=linux.vnet.ibm.com\n\t(client-ip=148.163.156.1; helo=mx0a-001b2d01.pphosted.com;\n\tenvelope-from=eajames@linux.vnet.ibm.com; receiver=)","Subject":"Re: [PATCH linux dev-4.10 1/3] drivers/fsi/sbefifo: refactor to\n\tupstream list state","To":"Andrew Jeffery , openbmc@lists.ozlabs.org","References":"<1506033838-2078-1-git-send-email-eajames@linux.vnet.ibm.com>\n\t<1506033838-2078-2-git-send-email-eajames@linux.vnet.ibm.com>\n\t<1506315435.30138.17.camel@aj.id.au>\n\t<1506320183.30138.22.camel@aj.id.au>","From":"Eddie James ","Date":"Fri, 29 Sep 2017 17:44:55 -0500","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.2.0","MIME-Version":"1.0","In-Reply-To":"<1506320183.30138.22.camel@aj.id.au>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"7bit","Content-Language":"en-US","X-TM-AS-GCONF":"00","x-cbid":"17092922-0024-0000-0000-00001745239A","X-IBM-SpamModules-Scores":"","X-IBM-SpamModules-Versions":"BY=3.00007813; HX=3.00000241; KW=3.00000007;\n\tPH=3.00000004; SC=3.00000233; SDB=6.00924261; UDB=6.00464729;\n\tIPR=6.00704399; \n\tBA=6.00005613; NDR=6.00000001; ZLA=6.00000005; ZF=6.00000009;\n\tZB=6.00000000; \n\tZP=6.00000000; ZH=6.00000000; ZU=6.00000002; MB=3.00017330;\n\tXFM=3.00000015; UTC=2017-09-29 22:44:58","X-IBM-AV-DETECTION":"SAVI=unused REMOTE=unused XFE=unused","x-cbparentid":"17092922-0025-0000-0000-00004CEAACED","Message-Id":"<92ba5ba1-77f2-ede6-6e2c-901777902ccd@linux.vnet.ibm.com>","X-Proofpoint-Virus-Version":"vendor=fsecure engine=2.50.10432:, ,\n\tdefinitions=2017-09-29_07:, , signatures=0","X-Proofpoint-Spam-Details":"rule=outbound_notspam policy=outbound score=0\n\tspamscore=0 suspectscore=0\n\tmalwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam\n\tadjust=0 reason=mlx scancount=1 engine=8.0.1-1707230000\n\tdefinitions=main-1709290323","X-BeenThere":"openbmc@lists.ozlabs.org","X-Mailman-Version":"2.1.24","Precedence":"list","List-Id":"Development list for OpenBMC ","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"\"Edward A. James\" ","Errors-To":"openbmc-bounces+incoming=patchwork.ozlabs.org@lists.ozlabs.org","Sender":"\"openbmc\"\n\t"}}]