[PATCH linux dev-4.10 v2 1/9] drivers: fsi: SBEFIFO: General clean-up
Eddie James
eajames at linux.vnet.ibm.com
Fri Oct 6 02:11:13 AEDT 2017
On 10/04/2017 06:59 PM, Andrew Jeffery wrote:
> On Fri, 2017-09-29 at 17:41 -0500, Eddie James wrote:
>> From: "Edward A. James" <eajames at us.ibm.com>
>>
>> Correct formatting and improve the flow a bit. Remove warnings for
>> offset in read/write functions, and remove the user pointer verification
>> since the copy_from/to_user will do that anyway. Correct the include
>> guards in the header file.
> This is similar to what I complained about last time, just without bullet
> points in the description.
>
> I tried to read the diff but really struggled to make sense of it. Keeping
> track of the added/removed/changed functions - amongst formatting and control
> flow changes - to decide whether they were appropriate is just *really*
> difficult, and I can't say with confidence that as a whole this patch is okay.
>
> So, apologies, but I don't plan on acking this as it stands, and I don't think
> it should be submitted unless several people are ready to weld Tested-by tags
> to it.
OK, we can just drop this patch, as it has no real functional changes.
We'll just be out of sync with what I send upstream.
Thanks,
Eddie
>
> Andrew
>
>>
>> Signed-off-by: Edward A. James <eajames at us.ibm.com>
>> ---
>> drivers/fsi/fsi-sbefifo.c | 207 +++++++++++++++++++++++++-------------------
>> include/linux/fsi-sbefifo.h | 6 +-
>> 2 files changed, 119 insertions(+), 94 deletions(-)
>>
>> diff --git a/drivers/fsi/fsi-sbefifo.c b/drivers/fsi/fsi-sbefifo.c
>> index 1c37ff7..a4cd353 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,6 +51,8 @@
>> #define SBEFIFO_EOT_MAGIC 0xffffffff
>> #define SBEFIFO_EOT_ACK 0x14
>>
>> +#define SBEFIFO_RESCHEDULE msecs_to_jiffies(500)
>> +
>> struct sbefifo {
>> struct timer_list poll_timer;
>> struct fsi_device *fsi_dev;
>> @@ -94,7 +103,7 @@ struct sbefifo_client {
>> static int sbefifo_inw(struct sbefifo *sbefifo, int reg, u32 *word)
>> {
>> int rc;
>> - u32 raw_word;
>> + __be32 raw_word;
>>
>> rc = fsi_device_read(sbefifo->fsi_dev, reg, &raw_word,
>> sizeof(raw_word));
>> @@ -102,17 +111,19 @@ static int sbefifo_inw(struct sbefifo *sbefifo, int reg, u32 *word)
>> return rc;
>>
>> *word = be32_to_cpu(raw_word);
>> +
>> return 0;
>> }
>>
>> static int sbefifo_outw(struct sbefifo *sbefifo, int reg, u32 word)
>> {
>> - u32 raw_word = cpu_to_be32(word);
>> + __be32 raw_word = cpu_to_be32(word);
>>
>> return fsi_device_write(sbefifo->fsi_dev, reg, &raw_word,
>> sizeof(raw_word));
>> }
>>
>> +/* Don't flip endianness of data to/from FIFO, just pass through. */
>> static int sbefifo_readw(struct sbefifo *sbefifo, u32 *word)
>> {
>> return fsi_device_read(sbefifo->fsi_dev, SBEFIFO_DWN, word,
>> @@ -136,7 +147,7 @@ static int sbefifo_ack_eot(struct sbefifo *sbefifo)
>> return ret;
>>
>> return sbefifo_outw(sbefifo, SBEFIFO_DWN | SBEFIFO_EOT_ACK,
>> - SBEFIFO_EOT_MAGIC);
>> + SBEFIFO_EOT_MAGIC);
>> }
>>
>> static size_t sbefifo_dev_nwreadable(u32 sts)
>> @@ -210,6 +221,7 @@ static bool sbefifo_buf_readnb(struct sbefifo_buf *buf, size_t n)
>> rpos = buf->buf;
>>
>> WRITE_ONCE(buf->rpos, rpos);
>> +
>> return rpos == wpos;
>> }
>>
>> @@ -229,14 +241,14 @@ static bool sbefifo_buf_wrotenb(struct sbefifo_buf *buf, size_t n)
>> set_bit(SBEFIFO_BUF_FULL, &buf->flags);
>>
>> WRITE_ONCE(buf->wpos, wpos);
>> +
>> return rpos == wpos;
>> }
>>
>> static void sbefifo_free(struct kref *kref)
>> {
>> - struct sbefifo *sbefifo;
>> + struct sbefifo *sbefifo = container_of(kref, struct sbefifo, kref);
>>
>> - sbefifo = container_of(kref, struct sbefifo, kref);
>> kfree(sbefifo);
>> }
>>
>> @@ -267,21 +279,12 @@ static struct sbefifo_xfr *sbefifo_enq_xfr(struct sbefifo_client *client)
>> return xfr;
>> }
>>
>> -static struct sbefifo_xfr *sbefifo_client_next_xfr(
>> - struct sbefifo_client *client)
>> -{
>> - if (list_empty(&client->xfrs))
>> - return NULL;
>> -
>> - return container_of(client->xfrs.next, struct sbefifo_xfr,
>> - client);
>> -}
>> -
>> static bool sbefifo_xfr_rsp_pending(struct sbefifo_client *client)
>> {
>> - struct sbefifo_xfr *xfr;
>> + struct sbefifo_xfr *xfr = list_first_entry_or_null(&client->xfrs,
>> + struct sbefifo_xfr,
>> + client);
>>
>> - xfr = sbefifo_client_next_xfr(client);
>> if (xfr && test_bit(SBEFIFO_XFR_RESP_PENDING, &xfr->flags))
>> return true;
>>
>> @@ -349,6 +352,7 @@ static struct sbefifo_xfr *sbefifo_next_xfr(struct sbefifo *sbefifo)
>> kfree(xfr);
>> continue;
>> }
>> +
>> return xfr;
>> }
>>
>> @@ -370,7 +374,7 @@ static void sbefifo_poll_timer(unsigned long data)
>>
>> spin_lock(&sbefifo->lock);
>> xfr = list_first_entry_or_null(&sbefifo->xfrs, struct sbefifo_xfr,
>> - xfrs);
>> + xfrs);
>> if (!xfr)
>> goto out_unlock;
>>
>> @@ -388,8 +392,7 @@ static void sbefifo_poll_timer(unsigned long data)
>>
>> /* Drain the write buffer. */
>> while ((bufn = sbefifo_buf_nbreadable(wbuf))) {
>> - ret = sbefifo_inw(sbefifo, SBEFIFO_UP | SBEFIFO_STS,
>> - &sts);
>> + ret = sbefifo_inw(sbefifo, SBEFIFO_UP | SBEFIFO_STS, &sts);
>> if (ret)
>> goto out;
>>
>> @@ -397,7 +400,7 @@ static void sbefifo_poll_timer(unsigned long data)
>> if (devn == 0) {
>> /* No open slot for write. Reschedule. */
>> sbefifo->poll_timer.expires = jiffies +
>> - msecs_to_jiffies(500);
>> + SBEFIFO_RESCHEDULE;
>> add_timer(&sbefifo->poll_timer);
>> goto out_unlock;
>> }
>> @@ -414,9 +417,8 @@ static void sbefifo_poll_timer(unsigned long data)
>>
>> /* Send EOT if the writer is finished. */
>> if (test_and_clear_bit(SBEFIFO_XFR_WRITE_DONE, &xfr->flags)) {
>> - ret = sbefifo_outw(sbefifo,
>> - SBEFIFO_UP | SBEFIFO_EOT_RAISE,
>> - SBEFIFO_EOT_MAGIC);
>> + ret = sbefifo_outw(sbefifo, SBEFIFO_UP | SBEFIFO_EOT_RAISE,
>> + SBEFIFO_EOT_MAGIC);
>> if (ret)
>> goto out;
>>
>> @@ -438,7 +440,7 @@ static void sbefifo_poll_timer(unsigned long data)
>> if (devn == 0) {
>> /* No data yet. Reschedule. */
>> sbefifo->poll_timer.expires = jiffies +
>> - msecs_to_jiffies(500);
>> + SBEFIFO_RESCHEDULE;
>> add_timer(&sbefifo->poll_timer);
>> goto out_unlock;
>> }
>> @@ -466,7 +468,7 @@ static void sbefifo_poll_timer(unsigned long data)
>> set_bit(SBEFIFO_XFR_COMPLETE, &xfr->flags);
>> list_del(&xfr->xfrs);
>> if (unlikely(test_bit(SBEFIFO_XFR_CANCEL,
>> - &xfr->flags)))
>> + &xfr->flags)))
>> kfree(xfr);
>> break;
>> }
>> @@ -476,7 +478,7 @@ static void sbefifo_poll_timer(unsigned long data)
>> if (unlikely(ret)) {
>> sbefifo->rc = ret;
>> dev_err(&sbefifo->fsi_dev->dev,
>> - "Fatal bus access failure: %d\n", ret);
>> + "Fatal bus access failure: %d\n", ret);
>> list_for_each_entry(xfr, &sbefifo->xfrs, xfrs)
>> kfree(xfr);
>> INIT_LIST_HEAD(&sbefifo->xfrs);
>> @@ -497,7 +499,7 @@ static void sbefifo_poll_timer(unsigned long data)
>> static int sbefifo_open(struct inode *inode, struct file *file)
>> {
>> struct sbefifo *sbefifo = container_of(file->private_data,
>> - struct sbefifo, mdev);
>> + struct sbefifo, mdev);
>> struct sbefifo_client *client;
>> int ret;
>>
>> @@ -535,6 +537,16 @@ static unsigned int sbefifo_poll(struct file *file, poll_table *wait)
>> return mask;
>> }
>>
>> +static bool sbefifo_read_ready(struct sbefifo *sbefifo,
>> + struct sbefifo_client *client, size_t *n,
>> + size_t *ret)
>> +{
>> + *n = sbefifo_buf_nbreadable(&client->rbuf);
>> + *ret = READ_ONCE(sbefifo->rc);
>> +
>> + return *ret || *n;
>> +}
>> +
>> static ssize_t sbefifo_read_common(struct sbefifo_client *client,
>> char __user *ubuf, char *kbuf, size_t len)
>> {
>> @@ -551,30 +563,37 @@ static ssize_t sbefifo_read_common(struct sbefifo_client *client,
>>
>> sbefifo_get_client(client);
>> if (wait_event_interruptible(sbefifo->wait,
>> - (ret = READ_ONCE(sbefifo->rc)) ||
>> - (n = sbefifo_buf_nbreadable(
>> - &client->rbuf)))) {
>> - sbefifo_put_client(client);
>> - return -ERESTARTSYS;
>> + sbefifo_read_ready(sbefifo, client,
>> + &n, &ret))) {
>> + ret = -ERESTARTSYS;
>> + goto out;
>> }
>> +
>> if (ret) {
>> INIT_LIST_HEAD(&client->xfrs);
>> - sbefifo_put_client(client);
>> - return ret;
>> + goto out;
>> }
>>
>> n = min_t(size_t, n, len);
>>
>> if (ubuf) {
>> if (copy_to_user(ubuf, READ_ONCE(client->rbuf.rpos), n)) {
>> - sbefifo_put_client(client);
>> - return -EFAULT;
>> + ret = -EFAULT;
>> + goto out;
>> }
>> - } else
>> + } else {
>> memcpy(kbuf, READ_ONCE(client->rbuf.rpos), n);
>> + }
>>
>> if (sbefifo_buf_readnb(&client->rbuf, n)) {
>> - xfr = sbefifo_client_next_xfr(client);
>> + xfr = list_first_entry_or_null(&client->xfrs,
>> + struct sbefifo_xfr, client);
>> + if (!xfr) {
>> + /* should be impossible to not have an xfr here */
>> + WARN_ONCE(1, "no xfr in queue");
>> + goto out;
>> + }
>> +
>> if (!test_bit(SBEFIFO_XFR_COMPLETE, &xfr->flags)) {
>> /*
>> * Fill the read buffer back up.
>> @@ -589,22 +608,31 @@ static ssize_t sbefifo_read_common(struct sbefifo_client *client,
>> }
>> }
>>
>> - sbefifo_put_client(client);
>> + ret = n;
>>
>> - return n;
>> +out:
>> + sbefifo_put_client(client);
>> + return ret;
>> }
>>
>> -static ssize_t sbefifo_read(struct file *file, char __user *buf,
>> - size_t len, loff_t *offset)
>> +static ssize_t sbefifo_read(struct file *file, char __user *buf, size_t len,
>> + loff_t *offset)
>> {
>> struct sbefifo_client *client = file->private_data;
>>
>> - WARN_ON(*offset);
>> + return sbefifo_read_common(client, buf, NULL, len);
>> +}
>>
>> - if (!access_ok(VERIFY_WRITE, buf, len))
>> - return -EFAULT;
>> +static bool sbefifo_write_ready(struct sbefifo *sbefifo,
>> + struct sbefifo_xfr *xfr,
>> + struct sbefifo_client *client, size_t *n)
>> +{
>> + struct sbefifo_xfr *next = list_first_entry_or_null(&client->xfrs,
>> + struct sbefifo_xfr,
>> + client);
>>
>> - return sbefifo_read_common(client, buf, NULL, len);
>> + *n = sbefifo_buf_nbwriteable(&client->wbuf);
>> + return READ_ONCE(sbefifo->rc) || (next == xfr && *n);
>> }
>>
>> static ssize_t sbefifo_write_common(struct sbefifo_client *client,
>> @@ -612,6 +640,7 @@ static ssize_t sbefifo_write_common(struct sbefifo_client *client,
>> size_t len)
>> {
>> struct sbefifo *sbefifo = client->dev;
>> + struct sbefifo_buf *wbuf = &client->wbuf;
>> struct sbefifo_xfr *xfr;
>> ssize_t ret = 0;
>> size_t n;
>> @@ -622,24 +651,26 @@ static ssize_t sbefifo_write_common(struct sbefifo_client *client,
>> if (!len)
>> return 0;
>>
>> - n = sbefifo_buf_nbwriteable(&client->wbuf);
>> + sbefifo_get_client(client);
>> + n = sbefifo_buf_nbwriteable(wbuf);
>>
>> spin_lock_irq(&sbefifo->lock);
>> - xfr = sbefifo_next_xfr(sbefifo);
>>
>> + xfr = sbefifo_next_xfr(sbefifo); /* next xfr to be executed */
>> if ((client->f_flags & O_NONBLOCK) && xfr && n < len) {
>> spin_unlock_irq(&sbefifo->lock);
>> - return -EAGAIN;
>> + ret = -EAGAIN;
>> + goto out;
>> }
>>
>> - xfr = sbefifo_enq_xfr(client);
>> + xfr = sbefifo_enq_xfr(client); /* this xfr queued up */
>> if (!xfr) {
>> spin_unlock_irq(&sbefifo->lock);
>> - return -ENOMEM;
>> + ret = PTR_ERR(xfr);
>> + goto out;
>> }
>> - spin_unlock_irq(&sbefifo->lock);
>>
>> - sbefifo_get_client(client);
>> + spin_unlock_irq(&sbefifo->lock);
>>
>> /*
>> * Partial writes are not really allowed in that EOT is sent exactly
>> @@ -647,49 +678,47 @@ static ssize_t sbefifo_write_common(struct sbefifo_client *client,
>> */
>> while (len) {
>> if (wait_event_interruptible(sbefifo->wait,
>> - READ_ONCE(sbefifo->rc) ||
>> - (sbefifo_client_next_xfr(client) == xfr &&
>> - (n = sbefifo_buf_nbwriteable(
>> - &client->wbuf))))) {
>> + sbefifo_write_ready(sbefifo, xfr,
>> + client,
>> + &n))) {
>> set_bit(SBEFIFO_XFR_CANCEL, &xfr->flags);
>> sbefifo_get(sbefifo);
>> if (mod_timer(&sbefifo->poll_timer, jiffies))
>> sbefifo_put(sbefifo);
>> -
>> - sbefifo_put_client(client);
>> - return -ERESTARTSYS;
>> + ret = -ERESTARTSYS;
>> + goto out;
>> }
>> +
>> if (sbefifo->rc) {
>> INIT_LIST_HEAD(&client->xfrs);
>> - sbefifo_put_client(client);
>> - return sbefifo->rc;
>> + ret = sbefifo->rc;
>> + goto out;
>> }
>>
>> n = min_t(size_t, n, len);
>>
>> if (ubuf) {
>> - if (copy_from_user(READ_ONCE(client->wbuf.wpos), ubuf,
>> - n)) {
>> + if (copy_from_user(READ_ONCE(wbuf->wpos), ubuf, n)) {
>> set_bit(SBEFIFO_XFR_CANCEL, &xfr->flags);
>> sbefifo_get(sbefifo);
>> if (mod_timer(&sbefifo->poll_timer, jiffies))
>> sbefifo_put(sbefifo);
>> - sbefifo_put_client(client);
>> - return -EFAULT;
>> + ret = -EFAULT;
>> + goto out;
>> }
>>
>> ubuf += n;
>> } else {
>> - memcpy(READ_ONCE(client->wbuf.wpos), kbuf, n);
>> + memcpy(READ_ONCE(wbuf->wpos), kbuf, n);
>> kbuf += n;
>> }
>>
>> - sbefifo_buf_wrotenb(&client->wbuf, n);
>> + sbefifo_buf_wrotenb(wbuf, n);
>> len -= n;
>> ret += n;
>>
>> - /* set flag before starting the worker, as it may run through
>> - * and check the flag before we exit this loop!
>> + /* Set this before starting timer to avoid race condition on
>> + * this flag with the timer function writer.
>> */
>> if (!len)
>> set_bit(SBEFIFO_XFR_WRITE_DONE, &xfr->flags);
>> @@ -702,21 +731,16 @@ static ssize_t sbefifo_write_common(struct sbefifo_client *client,
>> sbefifo_put(sbefifo);
>> }
>>
>> +out:
>> sbefifo_put_client(client);
>> -
>> return ret;
>> }
>>
>> static ssize_t sbefifo_write(struct file *file, const char __user *buf,
>> - size_t len, loff_t *offset)
>> + size_t len, loff_t *offset)
>> {
>> struct sbefifo_client *client = file->private_data;
>>
>> - WARN_ON(*offset);
>> -
>> - if (!access_ok(VERIFY_READ, buf, len))
>> - return -EFAULT;
>> -
>> return sbefifo_write_common(client, buf, NULL, len);
>> }
>>
>> @@ -802,29 +826,27 @@ static int sbefifo_probe(struct device *dev)
>> u32 sts;
>> int ret, child_idx = 0;
>>
>> - dev_dbg(dev, "Found sbefifo device\n");
>> sbefifo = kzalloc(sizeof(*sbefifo), GFP_KERNEL);
>> if (!sbefifo)
>> return -ENOMEM;
>>
>> sbefifo->fsi_dev = fsi_dev;
>>
>> - ret = sbefifo_inw(sbefifo,
>> - SBEFIFO_UP | SBEFIFO_STS, &sts);
>> + ret = sbefifo_inw(sbefifo, SBEFIFO_UP | SBEFIFO_STS, &sts);
>> if (ret)
>> return ret;
>> +
>> if (!(sts & SBEFIFO_EMPTY)) {
>> - dev_err(&sbefifo->fsi_dev->dev,
>> - "Found data in upstream fifo\n");
>> + dev_err(dev, "Found data in upstream fifo\n");
>> return -EIO;
>> }
>>
>> ret = sbefifo_inw(sbefifo, SBEFIFO_DWN | SBEFIFO_STS, &sts);
>> if (ret)
>> return ret;
>> +
>> if (!(sts & SBEFIFO_EMPTY)) {
>> - dev_err(&sbefifo->fsi_dev->dev,
>> - "Found data in downstream fifo\n");
>> + dev_err(dev, "found data in downstream fifo\n");
>> return -EIO;
>> }
>>
>> @@ -837,13 +859,13 @@ static int sbefifo_probe(struct device *dev)
>>
>> sbefifo->idx = ida_simple_get(&sbefifo_ida, 1, INT_MAX, GFP_KERNEL);
>> snprintf(sbefifo->name, sizeof(sbefifo->name), "sbefifo%d",
>> - sbefifo->idx);
>> + sbefifo->idx);
>> init_waitqueue_head(&sbefifo->wait);
>> INIT_LIST_HEAD(&sbefifo->xfrs);
>>
>> /* This bit of silicon doesn't offer any interrupts... */
>> setup_timer(&sbefifo->poll_timer, sbefifo_poll_timer,
>> - (unsigned long)sbefifo);
>> + (unsigned long)sbefifo);
>>
>> if (dev->of_node) {
>> /* create platform devs for dts child nodes (occ, etc) */
>> @@ -852,7 +874,7 @@ static int sbefifo_probe(struct device *dev)
>> sbefifo->name, child_idx++);
>> child = of_platform_device_create(np, child_name, dev);
>> if (!child)
>> - dev_warn(&sbefifo->fsi_dev->dev,
>> + dev_warn(dev,
>> "failed to create child node dev\n");
>> }
>> }
>> @@ -922,10 +944,13 @@ static int sbefifo_init(void)
>> static void sbefifo_exit(void)
>> {
>> fsi_driver_unregister(&sbefifo_drv);
>> +
>> + ida_destroy(&sbefifo_ida);
>> }
>>
>> module_init(sbefifo_init);
>> module_exit(sbefifo_exit);
>> MODULE_LICENSE("GPL");
>> MODULE_AUTHOR("Brad Bishop <bradleyb at fuzziesquirrel.com>");
>> -MODULE_DESCRIPTION("Linux device interface to the POWER self boot engine");
>> +MODULE_AUTHOR("Eddie James <eajames at 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 */
More information about the openbmc
mailing list