[{"id":1773167,"web_url":"http://patchwork.ozlabs.org/comment/1773167/","msgid":"<48977008-8d05-fb2e-411f-3d4c2fc1a4c5@linux.vnet.ibm.com>","list_archive_url":null,"date":"2017-09-21T22:55:43","subject":"Re: [PATCH linux dev-4.10 2/3] drivers/fsi/occ: refactor to upstream\n\tlist 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> - Fix probe failure scenario\n> - General cleanup\n> - Reorder remove() operations for safety\n> - Add cancel boolean to prevent further xfrs when we're removing\n\nAnd with this one, hasn't been sent upstream yet.\n\nEddie\n\n>\n> Signed-off-by: Edward A. James \n> ---\n> drivers/fsi/occ.c | 227 ++++++++++++++++++++++++++++++++--------------------\n> include/linux/occ.h | 20 ++++-\n> 2 files changed, 155 insertions(+), 92 deletions(-)\n>\n> diff --git a/drivers/fsi/occ.c b/drivers/fsi/occ.c\n> index 621fbf0..4dd5048 100644\n> --- a/drivers/fsi/occ.c\n> +++ b/drivers/fsi/occ.c\n> @@ -10,16 +10,22 @@\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> @@ -28,49 +34,45 @@\n> #define OCC_CMD_DATA_BYTES\t4090\n> #define OCC_RESP_DATA_BYTES\t4089\n>\n> +#define OCC_TIMEOUT_MS\t\t1000\n> +#define OCC_CMD_IN_PRG_WAIT_MS\t50\n> +\n> struct occ {\n> \tstruct device *sbefifo;\n> \tchar name[32];\n> \tint idx;\n> \tstruct miscdevice mdev;\n> \tstruct list_head xfrs;\n> -\tspinlock_t list_lock;\n> -\tstruct mutex occ_lock;\n> +\tspinlock_t list_lock;\t\t/* lock access to the xfrs list */\n> +\tstruct mutex occ_lock;\t\t/* lock access to the hardware */\n> \tstruct work_struct work;\n> +\tbool cancel;\n> };\n>\n> #define to_occ(x)\tcontainer_of((x), struct occ, mdev)\n>\n> -struct occ_command {\n> -\tu8 seq_no;\n> -\tu8 cmd_type;\n> -\tu16 data_length;\n> -\tu8 data[OCC_CMD_DATA_BYTES];\n> -\tu16 checksum;\n> -} __packed;\n> -\n> struct occ_response {\n> \tu8 seq_no;\n> \tu8 cmd_type;\n> \tu8 return_status;\n> -\tu16 data_length;\n> +\t__be16 data_length;\n> \tu8 data[OCC_RESP_DATA_BYTES];\n> -\tu16 checksum;\n> +\t__be16 checksum;\n> } __packed;\n>\n> /*\n> * transfer flags are NOT mutually exclusive\n> - *\n> + *\n> * Initial flags are none; transfer is created and queued from write(). All\n> - * \tflags are cleared when the transfer is completed by closing the file or\n> - * \treading all of the available response data.\n> + * flags are cleared when the transfer is completed by closing the file or\n> + * reading all of the available response data.\n> * XFR_IN_PROGRESS is set when a transfer is started from occ_worker_putsram,\n> - * \tand cleared if the transfer fails or occ_worker_getsram completes.\n> + * and cleared if the transfer fails or occ_worker_getsram completes.\n> * XFR_COMPLETE is set when a transfer fails or finishes occ_worker_getsram.\n> * XFR_CANCELED is set when the transfer's client is released.\n> * XFR_WAITING is set from read() if the transfer isn't complete and\n> - * \tNONBLOCKING wasn't specified. Cleared in read() when transfer completes\n> - * \tor fails.\n> + * O_NONBLOCK wasn't specified. Cleared in read() when transfer completes or\n> + * fails.\n> */\n> enum {\n> \tXFR_IN_PROGRESS,\n> @@ -92,9 +94,9 @@ struct occ_xfr {\n> * client flags\n> *\n> * CLIENT_NONBLOCKING is set during open() if the file was opened with the\n> - * \tO_NONBLOCKING flag.\n> + * O_NONBLOCK flag.\n> * CLIENT_XFR_PENDING is set during write() and cleared when all data has been\n> - * \tread.\n> + * read.\n> */\n> enum {\n> \tCLIENT_NONBLOCKING,\n> @@ -104,7 +106,7 @@ enum {\n> struct occ_client {\n> \tstruct occ *occ;\n> \tstruct occ_xfr xfr;\n> -\tspinlock_t lock;\n> +\tspinlock_t lock;\t\t/* lock access to the client state */\n> \twait_queue_head_t wait;\n> \tsize_t read_offset;\n> \tunsigned long flags;\n> @@ -116,12 +118,15 @@ struct occ_client {\n>\n> static DEFINE_IDA(occ_ida);\n>\n> -static void occ_enqueue_xfr(struct occ_xfr *xfr)\n> +static int occ_enqueue_xfr(struct occ_xfr *xfr)\n> {\n> \tint empty;\n> \tstruct occ_client *client = to_client(xfr);\n> \tstruct occ *occ = client->occ;\n>\n> +\tif (occ->cancel)\n> +\t\treturn -ECANCELED;\n> +\n> \tspin_lock_irq(&occ->list_lock);\n>\n> \tempty = list_empty(&occ->xfrs);\n> @@ -131,14 +136,15 @@ static void occ_enqueue_xfr(struct occ_xfr *xfr)\n>\n> \tif (empty)\n> \t\tqueue_work(occ_wq, &occ->work);\n> +\n> +\treturn 0;\n> }\n>\n> static struct occ_client *occ_open_common(struct occ *occ, unsigned long flags)\n> {\n> -\tstruct occ_client *client;\n> +\tstruct occ_client *client = kzalloc(sizeof(*client), GFP_KERNEL);\n>\n> -\tclient = kzalloc(sizeof(*client), GFP_KERNEL);\n> -\tif (!client)\n> +\tif (!client || occ->cancel)\n> \t\treturn NULL;\n>\n> \tclient->occ = occ;\n> @@ -172,6 +178,7 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,\n> \tint rc;\n> \tsize_t bytes;\n> \tstruct occ_xfr *xfr = &client->xfr;\n> +\tstruct occ *occ = client->occ;\n>\n> \tif (len > OCC_SRAM_BYTES)\n> \t\treturn -EINVAL;\n> @@ -183,8 +190,9 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,\n> \t\tif (client->read_offset) {\n> \t\t\trc = 0;\n> \t\t\tclient->read_offset = 0;\n> -\t\t} else\n> +\t\t} else {\n> \t\t\trc = -ENOMSG;\n> +\t\t}\n>\n> \t\tgoto done;\n> \t}\n> @@ -200,8 +208,11 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,\n> \t\tspin_unlock_irq(&client->lock);\n>\n> \t\trc = wait_event_interruptible(client->wait,\n> -\t\t\ttest_bit(XFR_COMPLETE, &xfr->flags) ||\n> -\t\t\ttest_bit(XFR_CANCELED, &xfr->flags));\n> +\t\t\t\t\t test_bit(XFR_COMPLETE,\n> +\t\t\t\t\t\t &xfr->flags) ||\n> +\t\t\t\t\t test_bit(XFR_CANCELED,\n> +\t\t\t\t\t\t &xfr->flags) ||\n> +\t\t\t\t\t occ->cancel);\n>\n> \t\tspin_lock_irq(&client->lock);\n>\n> @@ -225,12 +236,14 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,\n>\n> \tbytes = min(len, xfr->resp_data_length - client->read_offset);\n> \tif (ubuf) {\n> -\t\tif (copy_to_user(ubuf, &xfr->buf[client->read_offset], bytes)) {\n> +\t\tif (copy_to_user(ubuf, &xfr->buf[client->read_offset],\n> +\t\t\t\t bytes)) {\n> \t\t\trc = -EFAULT;\n> \t\t\tgoto done;\n> \t\t}\n> -\t} else\n> +\t} else {\n> \t\tmemcpy(kbuf, &xfr->buf[client->read_offset], bytes);\n> +\t}\n>\n> \tclient->read_offset += bytes;\n>\n> @@ -250,12 +263,6 @@ static ssize_t occ_read(struct file *file, char __user *buf, size_t len,\n> {\n> \tstruct occ_client *client = file->private_data;\n>\n> -\t/* check this ahead of time so we don't go changing the xfr state\n> -\t * needlessly\n> -\t */\n> -\tif (!access_ok(VERIFY_WRITE, buf, len))\n> -\t\treturn -EFAULT;\n> -\n> \treturn occ_read_common(client, buf, NULL, len);\n> }\n>\n> @@ -272,28 +279,31 @@ static ssize_t occ_write_common(struct occ_client *client,\n> \t\treturn -EINVAL;\n>\n> \tspin_lock_irq(&client->lock);\n> -\tif (test_and_set_bit(CLIENT_XFR_PENDING, &client->flags)) {\n> +\n> +\tif (test_bit(CLIENT_XFR_PENDING, &client->flags)) {\n> \t\trc = -EBUSY;\n> \t\tgoto done;\n> \t}\n>\n> -\t/* clear out the transfer */\n> -\tmemset(xfr, 0, sizeof(*xfr));\n> -\n> -\txfr->buf[0] = 1;\n> +\tmemset(xfr, 0, sizeof(*xfr));\t/* clear out the transfer */\n> +\txfr->buf[0] = 1;\t\t/* occ sequence number */\n>\n> +\t/* Assume user data follows the occ command format.\n> +\t * byte 0: command type\n> +\t * bytes 1-2: data length (msb first)\n> +\t * bytes 3-n: data\n> +\t */\n> \tif (ubuf) {\n> \t\tif (copy_from_user(&xfr->buf[1], ubuf, len)) {\n> -\t\t\tkfree(xfr);\n> \t\t\trc = -EFAULT;\n> \t\t\tgoto done;\n> \t\t}\n> -\t} else\n> +\t} else {\n> \t\tmemcpy(&xfr->buf[1], kbuf, len);\n> +\t}\n>\n> \tdata_length = (xfr->buf[2] << 8) + xfr->buf[3];\n> \tif (data_length > OCC_CMD_DATA_BYTES) {\n> -\t\tkfree(xfr);\n> \t\trc = -EINVAL;\n> \t\tgoto done;\n> \t}\n> @@ -306,9 +316,11 @@ static ssize_t occ_write_common(struct occ_client *client,\n>\n> \txfr->cmd_data_length = data_length + 6;\n> \tclient->read_offset = 0;\n> +\trc = occ_enqueue_xfr(xfr);\n> +\tif (rc)\n> +\t\tgoto done;\n>\n> -\tocc_enqueue_xfr(xfr);\n> -\n> +\tset_bit(CLIENT_XFR_PENDING, &client->flags);\n> \trc = len;\n>\n> done:\n> @@ -321,12 +333,6 @@ static ssize_t occ_write(struct file *file, const char __user *buf,\n> {\n> \tstruct occ_client *client = file->private_data;\n>\n> -\t/* check this ahead of time so we don't go changing the xfr state\n> -\t * needlessly\n> -\t */\n> -\tif (!access_ok(VERIFY_READ, buf, len))\n> -\t\treturn -EFAULT;\n> -\n> \treturn occ_write_common(client, buf, NULL, len);\n> }\n>\n> @@ -366,7 +372,7 @@ static int occ_release_common(struct occ_client *client)\n> \t\treturn 0;\n> \t}\n>\n> -\t/* operation is in progress; let worker clean up*/\n> +\t/* operation is in progress; let worker clean up */\n> \tspin_unlock_irq(&occ->list_lock);\n> \tspin_unlock_irq(&client->lock);\n> \treturn 0;\n> @@ -403,7 +409,7 @@ static int occ_write_sbefifo(struct sbefifo_client *client, const char *buf,\n> \t\ttotal += rc;\n> \t} while (total < len);\n>\n> -\treturn (total == len) ? 0 : -EMSGSIZE;\n> +\treturn (total == len) ? 0 : -ENODATA;\n> }\n>\n> static int occ_read_sbefifo(struct sbefifo_client *client, char *buf,\n> @@ -422,7 +428,7 @@ static int occ_read_sbefifo(struct sbefifo_client *client, char *buf,\n> \t\ttotal += rc;\n> \t} while (total < len);\n>\n> -\treturn (total == len) ? 0 : -EMSGSIZE;\n> +\treturn (total == len) ? 0 : -ENODATA;\n> }\n>\n> static int occ_getsram(struct device *sbefifo, u32 address, u8 *data,\n> @@ -430,10 +436,13 @@ static int occ_getsram(struct device *sbefifo, u32 address, u8 *data,\n> {\n> \tint rc;\n> \tu8 *resp;\n> -\tu32 buf[5];\n> -\tu32 data_len = ((len + 7) / 8) * 8;\n> +\t__be32 buf[5];\n> +\tu32 data_len = ((len + 7) / 8) * 8;\t/* must be multiples of 8 B */\n> \tstruct sbefifo_client *client;\n>\n> +\t/* Magic sequence to do SBE getsram command. SBE will fetch data from\n> +\t * specified SRAM address.\n> +\t */\n> \tbuf[0] = cpu_to_be32(0x5);\n> \tbuf[1] = cpu_to_be32(0xa403);\n> \tbuf[2] = cpu_to_be32(1);\n> @@ -447,7 +456,7 @@ static int occ_getsram(struct device *sbefifo, u32 address, u8 *data,\n> \trc = occ_write_sbefifo(client, (const char *)buf, sizeof(buf));\n> \tif (rc)\n> \t\tgoto done;\n> -\t\n> +\n> \tresp = kzalloc(data_len, GFP_KERNEL);\n> \tif (!resp) {\n> \t\trc = -ENOMEM;\n> @@ -467,7 +476,7 @@ static int occ_getsram(struct device *sbefifo, u32 address, u8 *data,\n> \t (be32_to_cpu(buf[1]) == 0xC0DEA403))\n> \t\tmemcpy(data, resp, len);\n> \telse\n> -\t\trc = -EFAULT;\n> +\t\trc = -EBADMSG;\n>\n> free:\n> \tkfree(resp);\n> @@ -481,8 +490,8 @@ static int occ_putsram(struct device *sbefifo, u32 address, u8 *data,\n> \t\t ssize_t len)\n> {\n> \tint rc;\n> -\tu32 *buf;\n> -\tu32 data_len = ((len + 7) / 8) * 8;\n> +\t__be32 *buf;\n> +\tu32 data_len = ((len + 7) / 8) * 8;\t/* must be multiples of 8 B */\n> \tsize_t cmd_len = data_len + 20;\n> \tstruct sbefifo_client *client;\n>\n> @@ -490,6 +499,9 @@ static int occ_putsram(struct device *sbefifo, u32 address, u8 *data,\n> \tif (!buf)\n> \t\treturn -ENOMEM;\n>\n> +\t/* Magic sequence to do SBE putsram command. SBE will transfer\n> +\t * data to specified SRAM address.\n> +\t */\n> \tbuf[0] = cpu_to_be32(0x5 + (data_len / 4));\n> \tbuf[1] = cpu_to_be32(0xa404);\n> \tbuf[2] = cpu_to_be32(1);\n> @@ -515,7 +527,7 @@ static int occ_putsram(struct device *sbefifo, u32 address, u8 *data,\n> \t/* check for good response */\n> \tif ((be32_to_cpu(buf[0]) != data_len) ||\n> \t (be32_to_cpu(buf[1]) != 0xC0DEA404))\n> -\t\trc = -EFAULT;\n> +\t\trc = -EBADMSG;\n>\n> done:\n> \tsbefifo_drv_release(client);\n> @@ -527,14 +539,17 @@ static int occ_putsram(struct device *sbefifo, u32 address, u8 *data,\n> static int occ_trigger_attn(struct device *sbefifo)\n> {\n> \tint rc;\n> -\tu32 buf[6];\n> +\t__be32 buf[6];\n> \tstruct sbefifo_client *client;\n>\n> +\t/* Magic sequence to do SBE putscom command. SBE will write 8 bytes to\n> +\t * specified SCOM address.\n> +\t */\n> \tbuf[0] = cpu_to_be32(0x6);\n> \tbuf[1] = cpu_to_be32(0xa202);\n> \tbuf[2] = 0;\n> \tbuf[3] = cpu_to_be32(0x6D035);\n> -\tbuf[4] = cpu_to_be32(0x20010000);\n> +\tbuf[4] = cpu_to_be32(0x20010000);\t/* trigger occ attention */\n> \tbuf[5] = 0;\n>\n> \tclient = sbefifo_drv_open(sbefifo, 0);\n> @@ -552,7 +567,7 @@ static int occ_trigger_attn(struct device *sbefifo)\n> \t/* check for good response */\n> \tif ((be32_to_cpu(buf[0]) != 0xC0DEA202) ||\n> \t (be32_to_cpu(buf[1]) & 0x0FFFFFFF))\n> -\t\trc = -EFAULT;\n> +\t\trc = -EBADMSG;\n>\n> done:\n> \tsbefifo_drv_release(client);\n> @@ -564,12 +579,19 @@ static void occ_worker(struct work_struct *work)\n> {\n> \tint rc = 0, empty, waiting, canceled;\n> \tu16 resp_data_length;\n> +\tunsigned long start;\n> +\tconst unsigned long timeout = msecs_to_jiffies(OCC_TIMEOUT_MS);\n> +\tconst long int wait_time = msecs_to_jiffies(OCC_CMD_IN_PRG_WAIT_MS);\n> \tstruct occ_xfr *xfr;\n> +\tstruct occ_response *resp;\n> \tstruct occ_client *client;\n> \tstruct occ *occ = container_of(work, struct occ, work);\n> \tstruct device *sbefifo = occ->sbefifo;\n>\n> again:\n> +\tif (occ->cancel)\n> +\t\treturn;\n> +\n> \tspin_lock_irq(&occ->list_lock);\n>\n> \txfr = list_first_entry_or_null(&occ->xfrs, struct occ_xfr, link);\n> @@ -578,11 +600,14 @@ static void occ_worker(struct work_struct *work)\n> \t\treturn;\n> \t}\n>\n> +\tresp = (struct occ_response *)xfr->buf;\n> \tset_bit(XFR_IN_PROGRESS, &xfr->flags);\n>\n> \tspin_unlock_irq(&occ->list_lock);\n> \tmutex_lock(&occ->occ_lock);\n>\n> +\t/* write occ command */\n> +\tstart = jiffies;\n> \trc = occ_putsram(sbefifo, 0xFFFBE000, xfr->buf,\n> \t\t\t xfr->cmd_data_length);\n> \tif (rc)\n> @@ -592,13 +617,26 @@ static void occ_worker(struct work_struct *work)\n> \tif (rc)\n> \t\tgoto done;\n>\n> -\trc = occ_getsram(sbefifo, 0xFFFBF000, xfr->buf, 8);\n> -\tif (rc)\n> -\t\tgoto done;\n> +\t/* read occ response */\n> +\tdo {\n> +\t\trc = occ_getsram(sbefifo, 0xFFFBF000, xfr->buf, 8);\n> +\t\tif (rc)\n> +\t\t\tgoto done;\n> +\n> +\t\tif (resp->return_status == OCC_RESP_CMD_IN_PRG) {\n> +\t\t\trc = -EALREADY;\n> +\n> +\t\t\tif (time_after(jiffies, start + timeout))\n> +\t\t\t\tbreak;\n>\n> -\tresp_data_length = (xfr->buf[3] << 8) + xfr->buf[4];\n> +\t\t\tset_current_state(TASK_INTERRUPTIBLE);\n> +\t\t\tschedule_timeout(wait_time);\n> +\t\t}\n> +\t} while (rc);\n> +\n> +\tresp_data_length = get_unaligned_be16(&resp->data_length);\n> \tif (resp_data_length > OCC_RESP_DATA_BYTES) {\n> -\t\trc = -EDOM;\n> +\t\trc = -EMSGSIZE;\n> \t\tgoto done;\n> \t}\n>\n> @@ -704,9 +742,6 @@ static int occ_probe(struct platform_device *pdev)\n> \tmutex_init(&occ->occ_lock);\n> \tINIT_WORK(&occ->work, occ_worker);\n>\n> -\t/* ensure NULL before we probe children, so they don't hang FSI */\n> -\tplatform_set_drvdata(pdev, NULL);\n> -\n> \tif (dev->of_node) {\n> \t\trc = of_property_read_u32(dev->of_node, \"reg\", ®);\n> \t\tif (!rc) {\n> @@ -716,23 +751,13 @@ static int occ_probe(struct platform_device *pdev)\n> \t\t\tif (occ->idx < 0)\n> \t\t\t\tocc->idx = ida_simple_get(&occ_ida, 1, INT_MAX,\n> \t\t\t\t\t\t\t GFP_KERNEL);\n> -\t\t} else\n> +\t\t} else {\n> \t\t\tocc->idx = ida_simple_get(&occ_ida, 1, INT_MAX,\n> \t\t\t\t\t\t GFP_KERNEL);\n> -\n> -\t\t/* create platform devs for dts child nodes (hwmon, etc) */\n> -\t\tfor_each_child_of_node(dev->of_node, np) {\n> -\t\t\tsnprintf(child_name, sizeof(child_name), \"occ%d-dev%d\",\n> -\t\t\t\t occ->idx, 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(dev,\n> -\t\t\t\t\t \"failed to create child node dev\\n\");\n> \t\t}\n> -\t} else\n> +\t} else {\n> \t\tocc->idx = ida_simple_get(&occ_ida, 1, INT_MAX, GFP_KERNEL);\n> -\n> -\tplatform_set_drvdata(pdev, occ);\n> +\t}\n>\n> \tsnprintf(occ->name, sizeof(occ->name), \"occ%d\", occ->idx);\n> \tocc->mdev.fops = &occ_fops;\n> @@ -742,20 +767,42 @@ static int occ_probe(struct platform_device *pdev)\n>\n> \trc = misc_register(&occ->mdev);\n> \tif (rc) {\n> -\t\tdev_err(dev, \"failed to register miscdevice\\n\");\n> +\t\tdev_err(dev, \"failed to register miscdevice: %d\\n\", rc);\n> +\t\tida_simple_remove(&occ_ida, occ->idx);\n> \t\treturn rc;\n> \t}\n>\n> +\t/* create platform devs for dts child nodes (hwmon, etc) */\n> +\tfor_each_available_child_of_node(dev->of_node, np) {\n> +\t\tsnprintf(child_name, sizeof(child_name), \"occ%d-dev%d\",\n> +\t\t\t occ->idx, 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 node dev\\n\");\n> +\t}\n> +\n> +\tplatform_set_drvdata(pdev, occ);\n> +\n> \treturn 0;\n> }\n>\n> static int occ_remove(struct platform_device *pdev)\n> {\n> \tstruct occ *occ = platform_get_drvdata(pdev);\n> +\tstruct occ_xfr *xfr;\n> +\tstruct occ_client *client;\n> +\n> +\tocc->cancel = true;\n> +\tlist_for_each_entry(xfr, &occ->xfrs, link) {\n> +\t\tclient = to_client(xfr);\n> +\t\twake_up_interruptible(&client->wait);\n> +\t}\n>\n> -\tflush_work(&occ->work);\n> \tmisc_deregister(&occ->mdev);\n> \tdevice_for_each_child(&pdev->dev, NULL, occ_unregister_child);\n> +\n> +\tflush_work(&occ->work);\n> +\n> \tida_simple_remove(&occ_ida, occ->idx);\n>\n> \treturn 0;\n> @@ -789,6 +836,8 @@ static void occ_exit(void)\n> \tdestroy_workqueue(occ_wq);\n>\n> \tplatform_driver_unregister(&occ_driver);\n> +\n> +\tida_destroy(&occ_ida);\n> }\n>\n> module_init(occ_init);\n> diff --git a/include/linux/occ.h b/include/linux/occ.h\n> index d78332c..0a4a54a 100644\n> --- a/include/linux/occ.h\n> +++ b/include/linux/occ.h\n> @@ -11,12 +11,26 @@\n> * GNU General Public License for more details.\n> */\n>\n> -#ifndef __OCC_H__\n> -#define __OCC_H__\n> +#ifndef LINUX_FSI_OCC_H\n> +#define LINUX_FSI_OCC_H\n>\n> struct device;\n> struct occ_client;\n>\n> +#define OCC_RESP_CMD_IN_PRG\t\t0xFF\n> +#define OCC_RESP_SUCCESS\t\t0\n> +#define OCC_RESP_CMD_INVAL\t\t0x11\n> +#define OCC_RESP_CMD_LEN_INVAL\t\t0x12\n> +#define OCC_RESP_DATA_INVAL\t\t0x13\n> +#define OCC_RESP_CHKSUM_ERR\t\t0x14\n> +#define OCC_RESP_INT_ERR\t\t0x15\n> +#define OCC_RESP_BAD_STATE\t\t0x16\n> +#define OCC_RESP_CRIT_EXCEPT\t\t0xE0\n> +#define OCC_RESP_CRIT_INIT\t\t0xE1\n> +#define OCC_RESP_CRIT_WATCHDOG\t\t0xE2\n> +#define OCC_RESP_CRIT_OCB\t\t0xE3\n> +#define OCC_RESP_CRIT_HW\t\t0xE4\n> +\n> extern struct occ_client *occ_drv_open(struct device *dev,\n> \t\t\t\t unsigned long flags);\n> extern int occ_drv_read(struct occ_client *client, char *buf, size_t len);\n> @@ -24,4 +38,4 @@ extern int occ_drv_write(struct occ_client *client, const char *buf,\n> \t\t\t size_t len);\n> extern void occ_drv_release(struct occ_client *client);\n>\n> -#endif /* __OCC_H__ */\n> +#endif /* LINUX_FSI_OCC_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 3xysgt5Yz6z9t2S\n\tfor ;\n\tFri, 22 Sep 2017 09:07: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 3xysgt4kDpzDsM4\n\tfor ;\n\tFri, 22 Sep 2017 09:07: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 3xysfW5FtmzDqT0\n\tfor ; Fri, 22 Sep 2017 09:05:55 +1000 (AEST)","from pps.filterd (m0098410.ppops.net [127.0.0.1])\n\tby mx0a-001b2d01.pphosted.com (8.16.0.21/8.16.0.21) with SMTP id\n\tv8LN4StS043639\n\tfor ; Thu, 21 Sep 2017 19:05:53 -0400","from e34.co.us.ibm.com (e34.co.us.ibm.com [32.97.110.152])\n\tby mx0a-001b2d01.pphosted.com with ESMTP id 2d4pnfrc3y-1\n\t(version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT)\n\tfor ; Thu, 21 Sep 2017 19:05:53 -0400","from localhost\n\tby e34.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:47 -0600","from b03cxnp08027.gho.boulder.ibm.com (9.17.130.19)\n\tby e34.co.us.ibm.com (192.168.1.134) with IBM ESMTP SMTP Gateway:\n\tAuthorized Use Only! Violators will be prosecuted; \n\tThu, 21 Sep 2017 16:55:44 -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 v8LMthql65208540; Thu, 21 Sep 2017 15:55:43 -0700","from b03ledav005.gho.boulder.ibm.com (unknown [127.0.0.1])\n\tby IMSVA (Postfix) with ESMTP id E1269BE03A;\n\tThu, 21 Sep 2017 16:55:43 -0600 (MDT)","from oc3016140333.ibm.com (unknown [9.41.174.252])\n\tby b03ledav005.gho.boulder.ibm.com (Postfix) with ESMTP id 7BD38BE038;\n\tThu, 21 Sep 2017 16:55:43 -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 2/3] drivers/fsi/occ: refactor to upstream\n\tlist 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-3-git-send-email-eajames@linux.vnet.ibm.com>","Date":"Thu, 21 Sep 2017 17:55:43 -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-3-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-0016-0000-0000-0000078D4A81","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.00462504;\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:45","X-IBM-AV-DETECTION":"SAVI=unused REMOTE=unused XFE=unused","x-cbparentid":"17092122-0017-0000-0000-00003B90CFBC","Message-Id":"<48977008-8d05-fb2e-411f-3d4c2fc1a4c5@linux.vnet.ibm.com>","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=3\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":1774440,"web_url":"http://patchwork.ozlabs.org/comment/1774440/","msgid":"<1506320562.30138.24.camel@aj.id.au>","list_archive_url":null,"date":"2017-09-25T06:22:42","subject":"Re: [PATCH linux dev-4.10 2/3] drivers/fsi/occ: 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>  - Fix probe failure scenario\n>  - General cleanup\n>  - Reorder remove() operations for safety\n>  - Add cancel boolean to prevent further xfrs when we're removing\n\nSee comment on previous patch about these bullet points.\n\n> \n> Signed-off-by: Edward A. James \n> ---\n>  drivers/fsi/occ.c   | 227 ++++++++++++++++++++++++++++++++--------------------\n>  include/linux/occ.h |  20 ++++-\n>  2 files changed, 155 insertions(+), 92 deletions(-)\n> \n> diff --git a/drivers/fsi/occ.c b/drivers/fsi/occ.c\n> index 621fbf0..4dd5048 100644\n> --- a/drivers/fsi/occ.c\n> +++ b/drivers/fsi/occ.c\n> @@ -10,16 +10,22 @@\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> @@ -28,49 +34,45 @@\n>  #define OCC_CMD_DATA_BYTES\t4090\n>  #define OCC_RESP_DATA_BYTES\t4089\n>  \n> +#define OCC_TIMEOUT_MS\t\t1000\n> +#define OCC_CMD_IN_PRG_WAIT_MS\t50\n> +\n>  struct occ {\n>  \tstruct device *sbefifo;\n>  \tchar name[32];\n>  \tint idx;\n>  \tstruct miscdevice mdev;\n>  \tstruct list_head xfrs;\n> -\tspinlock_t list_lock;\n> -\tstruct mutex occ_lock;\n> +\tspinlock_t list_lock;\t\t/* lock access to the xfrs list */\n> +\tstruct mutex occ_lock;\t\t/* lock access to the hardware */\n>  \tstruct work_struct work;\n> +\tbool cancel;\n\nShould this be atomic?\n\n>  };\n>  \n>  #define to_occ(x)\tcontainer_of((x), struct occ, mdev)\n>  \n> -struct occ_command {\n> -\tu8 seq_no;\n> -\tu8 cmd_type;\n> -\tu16 data_length;\n> -\tu8 data[OCC_CMD_DATA_BYTES];\n> -\tu16 checksum;\n> -} __packed;\n> -\n>  struct occ_response {\n>  \tu8 seq_no;\n>  \tu8 cmd_type;\n>  \tu8 return_status;\n> -\tu16 data_length;\n> +\t__be16 data_length;\n>  \tu8 data[OCC_RESP_DATA_BYTES];\n> -\tu16 checksum;\n> +\t__be16 checksum;\n>  } __packed;\n>  \n>  /*\n>   * transfer flags are NOT mutually exclusive\n> - * \n> + *\n>   * Initial flags are none; transfer is created and queued from write(). All\n> - * \tflags are cleared when the transfer is completed by closing the file or\n> - * \treading all of the available response data.\n> + *  flags are cleared when the transfer is completed by closing the file or\n> + *  reading all of the available response data.\n>   * XFR_IN_PROGRESS is set when a transfer is started from occ_worker_putsram,\n> - * \tand cleared if the transfer fails or occ_worker_getsram completes.\n> + *  and cleared if the transfer fails or occ_worker_getsram completes.\n>   * XFR_COMPLETE is set when a transfer fails or finishes occ_worker_getsram.\n>   * XFR_CANCELED is set when the transfer's client is released.\n>   * XFR_WAITING is set from read() if the transfer isn't complete and\n> - * \tNONBLOCKING wasn't specified. Cleared in read() when transfer completes\n> - * \tor fails.\n> + *  O_NONBLOCK wasn't specified. Cleared in read() when transfer completes or\n> + *  fails.\n>   */\n>  enum {\n>  \tXFR_IN_PROGRESS,\n> @@ -92,9 +94,9 @@ struct occ_xfr {\n>   * client flags\n>   *\n>   * CLIENT_NONBLOCKING is set during open() if the file was opened with the\n> - * \tO_NONBLOCKING flag.\n> + *  O_NONBLOCK flag.\n>   * CLIENT_XFR_PENDING is set during write() and cleared when all data has been\n> - * \tread.\n> + *  read.\n>   */\n>  enum {\n>  \tCLIENT_NONBLOCKING,\n> @@ -104,7 +106,7 @@ enum {\n>  struct occ_client {\n>  \tstruct occ *occ;\n>  \tstruct occ_xfr xfr;\n> -\tspinlock_t lock;\n> +\tspinlock_t lock;\t\t/* lock access to the client state */\n>  \twait_queue_head_t wait;\n>  \tsize_t read_offset;\n>  \tunsigned long flags;\n> @@ -116,12 +118,15 @@ struct occ_client {\n>  \n>  static DEFINE_IDA(occ_ida);\n>  \n> -static void occ_enqueue_xfr(struct occ_xfr *xfr)\n> +static int occ_enqueue_xfr(struct occ_xfr *xfr)\n>  {\n>  \tint empty;\n>  \tstruct occ_client *client = to_client(xfr);\n>  \tstruct occ *occ = client->occ;\n>  \n> +\tif (occ->cancel)\n> +\t\treturn -ECANCELED;\n> +\n>  \tspin_lock_irq(&occ->list_lock);\n>  \n>  \tempty = list_empty(&occ->xfrs);\n> @@ -131,14 +136,15 @@ static void occ_enqueue_xfr(struct occ_xfr *xfr)\n>  \n>  \tif (empty)\n>  \t\tqueue_work(occ_wq, &occ->work);\n> +\n> +\treturn 0;\n>  }\n>  \n>  static struct occ_client *occ_open_common(struct occ *occ, unsigned long flags)\n>  {\n> -\tstruct occ_client *client;\n> +\tstruct occ_client *client = kzalloc(sizeof(*client), GFP_KERNEL);\n>  \n> -\tclient = kzalloc(sizeof(*client), GFP_KERNEL);\n> -\tif (!client)\n> +\tif (!client || occ->cancel)\n>  \t\treturn NULL;\n>  \n>  \tclient->occ = occ;\n> @@ -172,6 +178,7 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,\n>  \tint rc;\n>  \tsize_t bytes;\n>  \tstruct occ_xfr *xfr = &client->xfr;\n> +\tstruct occ *occ = client->occ;\n>  \n>  \tif (len > OCC_SRAM_BYTES)\n>  \t\treturn -EINVAL;\n> @@ -183,8 +190,9 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,\n>  \t\tif (client->read_offset) {\n>  \t\t\trc = 0;\n>  \t\t\tclient->read_offset = 0;\n> -\t\t} else\n> +\t\t} else {\n>  \t\t\trc = -ENOMSG;\n> +\t\t}\n>  \n>  \t\tgoto done;\n>  \t}\n> @@ -200,8 +208,11 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,\n>  \t\tspin_unlock_irq(&client->lock);\n>  \n>  \t\trc = wait_event_interruptible(client->wait,\n> -\t\t\ttest_bit(XFR_COMPLETE, &xfr->flags) ||\n> -\t\t\ttest_bit(XFR_CANCELED, &xfr->flags));\n> +\t\t\t\t\t      test_bit(XFR_COMPLETE,\n> +\t\t\t\t\t\t       &xfr->flags) ||\n> +\t\t\t\t\t      test_bit(XFR_CANCELED,\n> +\t\t\t\t\t\t       &xfr->flags) ||\n> +\t\t\t\t\t      occ->cancel);\n>  \n>  \t\tspin_lock_irq(&client->lock);\n>  \n> @@ -225,12 +236,14 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,\n>  \n>  \tbytes = min(len, xfr->resp_data_length - client->read_offset);\n>  \tif (ubuf) {\n> -\t\tif (copy_to_user(ubuf, &xfr->buf[client->read_offset], bytes)) {\n> +\t\tif (copy_to_user(ubuf, &xfr->buf[client->read_offset],\n> +\t\t\t\t bytes)) {\n>  \t\t\trc = -EFAULT;\n>  \t\t\tgoto done;\n>  \t\t}\n> -\t} else\n> +\t} else {\n>  \t\tmemcpy(kbuf, &xfr->buf[client->read_offset], bytes);\n> +\t}\n>  \n>  \tclient->read_offset += bytes;\n>  \n> @@ -250,12 +263,6 @@ static ssize_t occ_read(struct file *file, char __user *buf, size_t len,\n>  {\n>  \tstruct occ_client *client = file->private_data;\n>  \n> -\t/* check this ahead of time so we don't go changing the xfr state\n> -\t * needlessly\n> -\t */\n> -\tif (!access_ok(VERIFY_WRITE, buf, len))\n> -\t\treturn -EFAULT;\n> -\n>  \treturn occ_read_common(client, buf, NULL, len);\n>  }\n>  \n> @@ -272,28 +279,31 @@ static ssize_t occ_write_common(struct occ_client *client,\n>  \t\treturn -EINVAL;\n>  \n>  \tspin_lock_irq(&client->lock);\n> -\tif (test_and_set_bit(CLIENT_XFR_PENDING, &client->flags)) {\n> +\n> +\tif (test_bit(CLIENT_XFR_PENDING, &client->flags)) {\n>  \t\trc = -EBUSY;\n>  \t\tgoto done;\n>  \t}\n>  \n> -\t/* clear out the transfer */\n> -\tmemset(xfr, 0, sizeof(*xfr));\n> -\n> -\txfr->buf[0] = 1;\n> +\tmemset(xfr, 0, sizeof(*xfr));\t/* clear out the transfer */\n\nWhat about the transfer buffer?\n\n> +\txfr->buf[0] = 1;\t\t/* occ sequence number */\n>  \n> +\t/* Assume user data follows the occ command format.\n> +\t * byte 0: command type\n> +\t * bytes 1-2: data length (msb first)\n> +\t * bytes 3-n: data\n> +\t */\n>  \tif (ubuf) {\n>  \t\tif (copy_from_user(&xfr->buf[1], ubuf, len)) {\n> -\t\t\tkfree(xfr);\n>  \t\t\trc = -EFAULT;\n>  \t\t\tgoto done;\n>  \t\t}\n> -\t} else\n> +\t} else {\n>  \t\tmemcpy(&xfr->buf[1], kbuf, len);\n> +\t}\n>  \n>  \tdata_length = (xfr->buf[2] << 8) + xfr->buf[3];\n\nShouldn't data_length be proportional to len? Like, somewhere in the\nneighbourhood of len - 3? Should we check that? What if I just send down 3\nbytes, some kind of command and two others representing the value 4090? Won't\nwe submit a bunch of \"junk\" (overlapping, previously submitted data) to the\nFIFO?\n\nIf the client is shared between higher-level command submitters then you could\npotentially resubmit the previous command.\n\n>  \tif (data_length > OCC_CMD_DATA_BYTES) {\n> -\t\tkfree(xfr);\n>  \t\trc = -EINVAL;\n>  \t\tgoto done;\n>  \t}\n> @@ -306,9 +316,11 @@ static ssize_t occ_write_common(struct occ_client *client,\n>  \n>  \txfr->cmd_data_length = data_length + 6;\n>  \tclient->read_offset = 0;\n> +\trc = occ_enqueue_xfr(xfr);\n> +\tif (rc)\n> +\t\tgoto done;\n>  \n> -\tocc_enqueue_xfr(xfr);\n> -\n> +\tset_bit(CLIENT_XFR_PENDING, &client->flags);\n>  \trc = len;\n>  \n>  done:\n> @@ -321,12 +333,6 @@ static ssize_t occ_write(struct file *file, const char __user *buf,\n>  {\n>  \tstruct occ_client *client = file->private_data;\n>  \n> -\t/* check this ahead of time so we don't go changing the xfr state\n> -\t * needlessly\n> -\t */\n> -\tif (!access_ok(VERIFY_READ, buf, len))\n> -\t\treturn -EFAULT;\n> -\n>  \treturn occ_write_common(client, buf, NULL, len);\n>  }\n>  \n> @@ -366,7 +372,7 @@ static int occ_release_common(struct occ_client *client)\n>  \t\treturn 0;\n>  \t}\n>  \n> -\t/* operation is in progress; let worker clean up*/\n> +\t/* operation is in progress; let worker clean up */\n>  \tspin_unlock_irq(&occ->list_lock);\n>  \tspin_unlock_irq(&client->lock);\n>  \treturn 0;\n> @@ -403,7 +409,7 @@ static int occ_write_sbefifo(struct sbefifo_client *client, const char *buf,\n>  \t\ttotal += rc;\n>  \t} while (total < len);\n>  \n> -\treturn (total == len) ? 0 : -EMSGSIZE;\n> +\treturn (total == len) ? 0 : -ENODATA;\n\nThis doesn't seem right for a write, maybe -ENOSPC?\n>  }\n>  \n>  static int occ_read_sbefifo(struct sbefifo_client *client, char *buf,\n> @@ -422,7 +428,7 @@ static int occ_read_sbefifo(struct sbefifo_client *client, char *buf,\n>  \t\ttotal += rc;\n>  \t} while (total < len);\n>  \n> -\treturn (total == len) ? 0 : -EMSGSIZE;\n> +\treturn (total == len) ? 0 : -ENODATA;\n>  }\n>  \n>  static int occ_getsram(struct device *sbefifo, u32 address, u8 *data,\n> @@ -430,10 +436,13 @@ static int occ_getsram(struct device *sbefifo, u32 address, u8 *data,\n>  {\n>  \tint rc;\n>  \tu8 *resp;\n> -\tu32 buf[5];\n> -\tu32 data_len = ((len + 7) / 8) * 8;\n> +\t__be32 buf[5];\n> +\tu32 data_len = ((len + 7) / 8) * 8;\t/* must be multiples of 8 B */\n>  \tstruct sbefifo_client *client;\n>  \n> +\t/* Magic sequence to do SBE getsram command. SBE will fetch data from\n> +\t * specified SRAM address.\n> +\t */\n\nDo the individual values of the sequence actually mean anything? Or are they\ntruly magic? If possible it would be good to indicate what each does rather\nthan just make some vague claim about them.\n\n>  \tbuf[0] = cpu_to_be32(0x5);\n>  \tbuf[1] = cpu_to_be32(0xa403);\n>  \tbuf[2] = cpu_to_be32(1);\n> @@ -447,7 +456,7 @@ static int occ_getsram(struct device *sbefifo, u32 address, u8 *data,\n>  \trc = occ_write_sbefifo(client, (const char *)buf, sizeof(buf));\n>  \tif (rc)\n>  \t\tgoto done;\n> -\t\n> +\n>  \tresp = kzalloc(data_len, GFP_KERNEL);\n>  \tif (!resp) {\n>  \t\trc = -ENOMEM;\n> @@ -467,7 +476,7 @@ static int occ_getsram(struct device *sbefifo, u32 address, u8 *data,\n>  \t    (be32_to_cpu(buf[1]) == 0xC0DEA403))\n>  \t\tmemcpy(data, resp, len);\n>  \telse\n> -\t\trc = -EFAULT;\n> +\t\trc = -EBADMSG;\n>  \n>  free:\n>  \tkfree(resp);\n> @@ -481,8 +490,8 @@ static int occ_putsram(struct device *sbefifo, u32 address, u8 *data,\n>  \t\t       ssize_t len)\n>  {\n>  \tint rc;\n> -\tu32 *buf;\n> -\tu32 data_len = ((len + 7) / 8) * 8;\n> +\t__be32 *buf;\n> +\tu32 data_len = ((len + 7) / 8) * 8;\t/* must be multiples of 8 B */\n>  \tsize_t cmd_len = data_len + 20;\n>  \tstruct sbefifo_client *client;\n>  \n> @@ -490,6 +499,9 @@ static int occ_putsram(struct device *sbefifo, u32 address, u8 *data,\n>  \tif (!buf)\n>  \t\treturn -ENOMEM;\n>  \n> +\t/* Magic sequence to do SBE putsram command. SBE will transfer\n> +\t * data to specified SRAM address.\n> +\t */\n\nSee above.\n\n>  \tbuf[0] = cpu_to_be32(0x5 + (data_len / 4));\n>  \tbuf[1] = cpu_to_be32(0xa404);\n>  \tbuf[2] = cpu_to_be32(1);\n> @@ -515,7 +527,7 @@ static int occ_putsram(struct device *sbefifo, u32 address, u8 *data,\n>  \t/* check for good response */\n>  \tif ((be32_to_cpu(buf[0]) != data_len) ||\n>  \t    (be32_to_cpu(buf[1]) != 0xC0DEA404))\n> -\t\trc = -EFAULT;\n> +\t\trc = -EBADMSG;\n>  \n>  done:\n>  \tsbefifo_drv_release(client);\n> @@ -527,14 +539,17 @@ static int occ_putsram(struct device *sbefifo, u32 address, u8 *data,\n>  static int occ_trigger_attn(struct device *sbefifo)\n>  {\n>  \tint rc;\n> -\tu32 buf[6];\n> +\t__be32 buf[6];\n>  \tstruct sbefifo_client *client;\n>  \n> +\t/* Magic sequence to do SBE putscom command. SBE will write 8 bytes to\n> +\t * specified SCOM address.\n> +\t */\n\nSee above.\n\n>  \tbuf[0] = cpu_to_be32(0x6);\n>  \tbuf[1] = cpu_to_be32(0xa202);\n>  \tbuf[2] = 0;\n>  \tbuf[3] = cpu_to_be32(0x6D035);\n> -\tbuf[4] = cpu_to_be32(0x20010000);\n> +\tbuf[4] = cpu_to_be32(0x20010000);\t/* trigger occ attention */\n\nThis kind of comment is what I'm interested in.\n\n>  \tbuf[5] = 0;\n>  \n>  \tclient = sbefifo_drv_open(sbefifo, 0);\n> @@ -552,7 +567,7 @@ static int occ_trigger_attn(struct device *sbefifo)\n>  \t/* check for good response */\n>  \tif ((be32_to_cpu(buf[0]) != 0xC0DEA202) ||\n>  \t    (be32_to_cpu(buf[1]) & 0x0FFFFFFF))\n> -\t\trc = -EFAULT;\n> +\t\trc = -EBADMSG;\n>  \n>  done:\n>  \tsbefifo_drv_release(client);\n> @@ -564,12 +579,19 @@ static void occ_worker(struct work_struct *work)\n>  {\n>  \tint rc = 0, empty, waiting, canceled;\n>  \tu16 resp_data_length;\n> +\tunsigned long start;\n> +\tconst unsigned long timeout = msecs_to_jiffies(OCC_TIMEOUT_MS);\n> +\tconst long int wait_time = msecs_to_jiffies(OCC_CMD_IN_PRG_WAIT_MS);\n>  \tstruct occ_xfr *xfr;\n> +\tstruct occ_response *resp;\n>  \tstruct occ_client *client;\n>  \tstruct occ *occ = container_of(work, struct occ, work);\n>  \tstruct device *sbefifo = occ->sbefifo;\n>  \n>  again:\n> +\tif (occ->cancel)\n> +\t\treturn;\n> +\n>  \tspin_lock_irq(&occ->list_lock);\n>  \n>  \txfr = list_first_entry_or_null(&occ->xfrs, struct occ_xfr, link);\n> @@ -578,11 +600,14 @@ static void occ_worker(struct work_struct *work)\n>  \t\treturn;\n>  \t}\n>  \n> +\tresp = (struct occ_response *)xfr->buf;\n\nAre the members all aligned as necessary?\n\n>  \tset_bit(XFR_IN_PROGRESS, &xfr->flags);\n>  \n>  \tspin_unlock_irq(&occ->list_lock);\n>  \tmutex_lock(&occ->occ_lock);\n>  \n> +\t/* write occ command */\n> +\tstart = jiffies;\n>  \trc = occ_putsram(sbefifo, 0xFFFBE000, xfr->buf,\n>  \t\t\t xfr->cmd_data_length);\n>  \tif (rc)\n> @@ -592,13 +617,26 @@ static void occ_worker(struct work_struct *work)\n>  \tif (rc)\n>  \t\tgoto done;\n>  \n> -\trc = occ_getsram(sbefifo, 0xFFFBF000, xfr->buf, 8);\n> -\tif (rc)\n> -\t\tgoto done;\n> +\t/* read occ response */\n> +\tdo {\n> +\t\trc = occ_getsram(sbefifo, 0xFFFBF000, xfr->buf, 8);\n> +\t\tif (rc)\n> +\t\t\tgoto done;\n> +\n> +\t\tif (resp->return_status == OCC_RESP_CMD_IN_PRG) {\n> +\t\t\trc = -EALREADY;\n> +\n> +\t\t\tif (time_after(jiffies, start + timeout))\n> +\t\t\t\tbreak;\n>  \n> -\tresp_data_length = (xfr->buf[3] << 8) + xfr->buf[4];\n> +\t\t\tset_current_state(TASK_INTERRUPTIBLE);\n> +\t\t\tschedule_timeout(wait_time);\n> +\t\t}\n> +\t} while (rc);\n> +\n> +\tresp_data_length = get_unaligned_be16(&resp->data_length);\n>  \tif (resp_data_length > OCC_RESP_DATA_BYTES) {\n> -\t\trc = -EDOM;\n> +\t\trc = -EMSGSIZE;\n>  \t\tgoto done;\n>  \t}\n>  \n> @@ -704,9 +742,6 @@ static int occ_probe(struct platform_device *pdev)\n>  \tmutex_init(&occ->occ_lock);\n>  \tINIT_WORK(&occ->work, occ_worker);\n>  \n> -\t/* ensure NULL before we probe children, so they don't hang FSI */\n> -\tplatform_set_drvdata(pdev, NULL);\n> -\n>  \tif (dev->of_node) {\n>  \t\trc = of_property_read_u32(dev->of_node, \"reg\", ®);\n>  \t\tif (!rc) {\n> @@ -716,23 +751,13 @@ static int occ_probe(struct platform_device *pdev)\n>  \t\t\tif (occ->idx < 0)\n>  \t\t\t\tocc->idx = ida_simple_get(&occ_ida, 1, INT_MAX,\n>  \t\t\t\t\t\t\t  GFP_KERNEL);\n> -\t\t} else\n> +\t\t} else {\n>  \t\t\tocc->idx = ida_simple_get(&occ_ida, 1, INT_MAX,\n>  \t\t\t\t\t\t  GFP_KERNEL);\n> -\n> -\t\t/* create platform devs for dts child nodes (hwmon, etc) */\n> -\t\tfor_each_child_of_node(dev->of_node, np) {\n> -\t\t\tsnprintf(child_name, sizeof(child_name), \"occ%d-dev%d\",\n> -\t\t\t\t occ->idx, 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(dev,\n> -\t\t\t\t\t \"failed to create child node dev\\n\");\n>  \t\t}\n> -\t} else\n> +\t} else {\n>  \t\tocc->idx = ida_simple_get(&occ_ida, 1, INT_MAX, GFP_KERNEL);\n> -\n> -\tplatform_set_drvdata(pdev, occ);\n> +\t}\n>  \n>  \tsnprintf(occ->name, sizeof(occ->name), \"occ%d\", occ->idx);\n>  \tocc->mdev.fops = &occ_fops;\n> @@ -742,20 +767,42 @@ static int occ_probe(struct platform_device *pdev)\n>  \n>  \trc = misc_register(&occ->mdev);\n>  \tif (rc) {\n> -\t\tdev_err(dev, \"failed to register miscdevice\\n\");\n> +\t\tdev_err(dev, \"failed to register miscdevice: %d\\n\", rc);\n> +\t\tida_simple_remove(&occ_ida, occ->idx);\n>  \t\treturn rc;\n>  \t}\n>  \n> +\t/* create platform devs for dts child nodes (hwmon, etc) */\n> +\tfor_each_available_child_of_node(dev->of_node, np) {\n> +\t\tsnprintf(child_name, sizeof(child_name), \"occ%d-dev%d\",\n> +\t\t\t occ->idx, 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 node dev\\n\");\n> +\t}\n> +\n> +\tplatform_set_drvdata(pdev, occ);\n> +\n\nSimilar to the SBEFIFO, it would be nice to separate out the miscdev from the\ncore.\n\n>  \treturn 0;\n>  }\n>  \n>  static int occ_remove(struct platform_device *pdev)\n>  {\n>  \tstruct occ *occ = platform_get_drvdata(pdev);\n> +\tstruct occ_xfr *xfr;\n> +\tstruct occ_client *client;\n> +\n> +\tocc->cancel = true;\n> +\tlist_for_each_entry(xfr, &occ->xfrs, link) {\n> +\t\tclient = to_client(xfr);\n> +\t\twake_up_interruptible(&client->wait);\n> +\t}\n>  \n> -\tflush_work(&occ->work);\n>  \tmisc_deregister(&occ->mdev);\n>  \tdevice_for_each_child(&pdev->dev, NULL, occ_unregister_child);\n> +\n> +\tflush_work(&occ->work);\n\nThis should be okay now that we have the -ENODEV in the SBEFIFO, right (if\nthat's all nice and race-free)? That is, sbefifo_read_ready() and\nsbefifo_write_ready() should trigger and the associated\nsbefifo_{read,write}_common() pulls -ENODEV out of sbefifo->rc?\n\n> +\n>  \tida_simple_remove(&occ_ida, occ->idx);\n>  \n>  \treturn 0;\n> @@ -789,6 +836,8 @@ static void occ_exit(void)\n>  \tdestroy_workqueue(occ_wq);\n>  \n>  \tplatform_driver_unregister(&occ_driver);\n> +\n> +\tida_destroy(&occ_ida);\n>  }\n>  \n>  module_init(occ_init);\n> diff --git a/include/linux/occ.h b/include/linux/occ.h\n> index d78332c..0a4a54a 100644\n> --- a/include/linux/occ.h\n> +++ b/include/linux/occ.h\n> @@ -11,12 +11,26 @@\n>   * GNU General Public License for more details.\n>   */\n>  \n> -#ifndef __OCC_H__\n> -#define __OCC_H__\n> +#ifndef LINUX_FSI_OCC_H\n> +#define LINUX_FSI_OCC_H\n>  \n>  struct device;\n>  struct occ_client;\n>  \n> +#define OCC_RESP_CMD_IN_PRG\t\t0xFF\n> +#define OCC_RESP_SUCCESS\t\t0\n> +#define OCC_RESP_CMD_INVAL\t\t0x11\n> +#define OCC_RESP_CMD_LEN_INVAL\t\t0x12\n> +#define OCC_RESP_DATA_INVAL\t\t0x13\n> +#define OCC_RESP_CHKSUM_ERR\t\t0x14\n> +#define OCC_RESP_INT_ERR\t\t0x15\n> +#define OCC_RESP_BAD_STATE\t\t0x16\n> +#define OCC_RESP_CRIT_EXCEPT\t\t0xE0\n> +#define OCC_RESP_CRIT_INIT\t\t0xE1\n> +#define OCC_RESP_CRIT_WATCHDOG\t\t0xE2\n> +#define OCC_RESP_CRIT_OCB\t\t0xE3\n> +#define OCC_RESP_CRIT_HW\t\t0xE4\n> +\n>  extern struct occ_client *occ_drv_open(struct device *dev,\n>  \t\t\t\t       unsigned long flags);\n>  extern int occ_drv_read(struct occ_client *client, char *buf, size_t len);\n> @@ -24,4 +38,4 @@ extern int occ_drv_write(struct occ_client *client, const char *buf,\n>  \t\t\t size_t len);\n>  extern void occ_drv_release(struct occ_client *client);\n>  \n> -#endif /* __OCC_H__ */\n> +#endif /* LINUX_FSI_OCC_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 3y0vCV3PYpz9tXD\n\tfor ;\n\tMon, 25 Sep 2017 16:23:02 +1000 (AEST)","from lists.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3])\n\tby lists.ozlabs.org (Postfix) with ESMTP id 3y0vCV1bHSzDsNc\n\tfor ;\n\tMon, 25 Sep 2017 16:23:02 +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 3y0vCG625yzDr4N\n\tfor ; Mon, 25 Sep 2017 16:22:50 +1000 (AEST)","from compute4.internal (compute4.nyi.internal [10.202.2.44])\n\tby mailout.nyi.internal (Postfix) with ESMTP id A80A320DE3;\n\tMon, 25 Sep 2017 02:22:48 -0400 (EDT)","from frontend2 ([10.202.2.161])\n\tby compute4.internal (MEProxy); Mon, 25 Sep 2017 02:22:48 -0400","from keelia16 (unknown [203.0.153.9])\n\tby mail.messagingengine.com (Postfix) with ESMTPA id 84CB024335;\n\tMon, 25 Sep 2017 02:22:46 -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=\"NugFZ+td\";\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=messagingengine.com\n\theader.i=@messagingengine.com header.b=\"m3nEc9n/\"; \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=\"NugFZ+td\";\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=messagingengine.com\n\theader.i=@messagingengine.com header.b=\"m3nEc9n/\"; \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=\"NugFZ+td\";\n\tdkim=pass (2048-bit key;\n\tunprotected) header.d=messagingengine.com\n\theader.i=@messagingengine.com\n\theader.b=\"m3nEc9n/\"; 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=OEs5Yl/a8EqE/P9FDOaEZQ449/m0Q5mCYLHSRsdqm\n\thE=; b=NugFZ+tdt+ANEAi1k8Aqm7Hb/Hs+e+EllnD3moXWpc3WUudFgJcrFUwuO\n\t06wWGWGXja4sJjoH8A/4wzxeIl2rvzTpATFsz+Xhe/vAU/UNTXbvp1ykaH4aE0je\n\teBSgSjqIpl/SPiHZx9WD8WT2HZpvqpWkUBGo72ZA8D8DWWCX9dzZ0R/6Y36Cn+gV\n\t5M5MsPVkhbeXLKfeRTjZEC+ilozWc11Gle13VjnclSN7skTIYv6+fthF7fTe5yKy\n\tCqT6h2/kv7L6y0GWvAhifT6CDDKBhj/C/jxOejc/itEwS9Q3GOkLZGYoIHa5CgeQ\n\tKUKBDfn6+vmdD8IC4GFNW45PsckNg==","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=OEs5Yl/a8EqE/P9FDO\n\taEZQ449/m0Q5mCYLHSRsdqmhE=; b=m3nEc9n/MYcymZEq5cpKJWvsm79EAYJ67X\n\tMaKVgknDQ3UYzAFkjRDvr29NuzakMfn83JIXgkW3ns+bKMc6gFs3dXNUN24igVzT\n\tFW3Fy7p1GPDmRFXnCeylqPnuNQ0lHS/+YJnRhNJ1sFMchxupcWPxiTbMBacJR/G0\n\tkJGzy31UzwnybUTAbCxb2eQWfZPRoE2cOjRAkcPZqsmfO9QBDS275jMx3aQ0mi9S\n\thqR3aaoNPLPLznObVqNy/nbL33sMNvzimnHTH5s1NkaqYhPYtXsGl6Db1dWhQFnC\n\tvoV/P27zaClf6uyjrg9+yix5iJFe12DlwJSwwa3FKWj4rotkqhiQ=="],"X-ME-Sender":"","X-Sasl-enc":"SnkLMY5IGP3eJwwtOg/KiAgsGobrxg/+o9s4ZdgCbPh0 1506320567","Message-ID":"<1506320562.30138.24.camel@aj.id.au>","Subject":"Re: [PATCH linux dev-4.10 2/3] drivers/fsi/occ: refactor to\n\tupstream list state","From":"Andrew Jeffery ","To":"Eddie James , openbmc@lists.ozlabs.org","Date":"Mon, 25 Sep 2017 15:52:42 +0930","In-Reply-To":"<1506033838-2078-3-git-send-email-eajames@linux.vnet.ibm.com>","References":"<1506033838-2078-1-git-send-email-eajames@linux.vnet.ibm.com>\n\t<1506033838-2078-3-git-send-email-eajames@linux.vnet.ibm.com>","Content-Type":"multipart/signed; micalg=\"pgp-sha512\";\n\tprotocol=\"application/pgp-signature\";\n\tboundary=\"=-L0nG2BAT09d4i0AM4UdG\"","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":1777827,"web_url":"http://patchwork.ozlabs.org/comment/1777827/","msgid":"","list_archive_url":null,"date":"2017-09-29T22:41:03","subject":"Re: [PATCH linux dev-4.10 2/3] drivers/fsi/occ: refactor to upstream\n\tlist 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:22 AM, Andrew Jeffery wrote:\n> On Thu, 2017-09-21 at 17:43 -0500, Eddie James wrote:\n>> From: \"Edward A. James\" \n>> \n>> Includes various fixes:\n>> - Fix probe failure scenario\n>> - General cleanup\n>> - Reorder remove() operations for safety\n>> - Add cancel boolean to prevent further xfrs when we're removing\n> See comment on previous patch about these bullet points.\n>\n>> \n>> Signed-off-by: Edward A. James \n>> ---\n>> drivers/fsi/occ.c | 227 ++++++++++++++++++++++++++++++++--------------------\n>> include/linux/occ.h | 20 ++++-\n>> 2 files changed, 155 insertions(+), 92 deletions(-)\n>> \n>> diff --git a/drivers/fsi/occ.c b/drivers/fsi/occ.c\n>> index 621fbf0..4dd5048 100644\n>> --- a/drivers/fsi/occ.c\n>> +++ b/drivers/fsi/occ.c\n>> @@ -10,16 +10,22 @@\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>> @@ -28,49 +34,45 @@\n>> #define OCC_CMD_DATA_BYTES\t4090\n>> #define OCC_RESP_DATA_BYTES\t4089\n>> \n>> +#define OCC_TIMEOUT_MS\t\t1000\n>> +#define OCC_CMD_IN_PRG_WAIT_MS\t50\n>> +\n>> struct occ {\n>> \tstruct device *sbefifo;\n>> \tchar name[32];\n>> \tint idx;\n>> \tstruct miscdevice mdev;\n>> \tstruct list_head xfrs;\n>> -\tspinlock_t list_lock;\n>> -\tstruct mutex occ_lock;\n>> +\tspinlock_t list_lock;\t\t/* lock access to the xfrs list */\n>> +\tstruct mutex occ_lock;\t\t/* lock access to the hardware */\n>> \tstruct work_struct work;\n>> +\tbool cancel;\n> Should this be atomic?\n\nShouldn't really matter. Only remove() is writing the value.\n\n>\n>> };\n>> \n>> #define to_occ(x)\tcontainer_of((x), struct occ, mdev)\n>> \n>> -struct occ_command {\n>> -\tu8 seq_no;\n>> -\tu8 cmd_type;\n>> -\tu16 data_length;\n>> -\tu8 data[OCC_CMD_DATA_BYTES];\n>> -\tu16 checksum;\n>> -} __packed;\n>> -\n>> struct occ_response {\n>> \tu8 seq_no;\n>> \tu8 cmd_type;\n>> \tu8 return_status;\n>> -\tu16 data_length;\n>> +\t__be16 data_length;\n>> \tu8 data[OCC_RESP_DATA_BYTES];\n>> -\tu16 checksum;\n>> +\t__be16 checksum;\n>> } __packed;\n>> \n>> /*\n>> * transfer flags are NOT mutually exclusive\n>> - *\n>> + *\n>> * Initial flags are none; transfer is created and queued from write(). All\n>> - * \tflags are cleared when the transfer is completed by closing the file or\n>> - * \treading all of the available response data.\n>> + * flags are cleared when the transfer is completed by closing the file or\n>> + * reading all of the available response data.\n>> * XFR_IN_PROGRESS is set when a transfer is started from occ_worker_putsram,\n>> - * \tand cleared if the transfer fails or occ_worker_getsram completes.\n>> + * and cleared if the transfer fails or occ_worker_getsram completes.\n>> * XFR_COMPLETE is set when a transfer fails or finishes occ_worker_getsram.\n>> * XFR_CANCELED is set when the transfer's client is released.\n>> * XFR_WAITING is set from read() if the transfer isn't complete and\n>> - * \tNONBLOCKING wasn't specified. Cleared in read() when transfer completes\n>> - * \tor fails.\n>> + * O_NONBLOCK wasn't specified. Cleared in read() when transfer completes or\n>> + * fails.\n>> */\n>> enum {\n>> \tXFR_IN_PROGRESS,\n>> @@ -92,9 +94,9 @@ struct occ_xfr {\n>> * client flags\n>> *\n>> * CLIENT_NONBLOCKING is set during open() if the file was opened with the\n>> - * \tO_NONBLOCKING flag.\n>> + * O_NONBLOCK flag.\n>> * CLIENT_XFR_PENDING is set during write() and cleared when all data has been\n>> - * \tread.\n>> + * read.\n>> */\n>> enum {\n>> \tCLIENT_NONBLOCKING,\n>> @@ -104,7 +106,7 @@ enum {\n>> struct occ_client {\n>> \tstruct occ *occ;\n>> \tstruct occ_xfr xfr;\n>> -\tspinlock_t lock;\n>> +\tspinlock_t lock;\t\t/* lock access to the client state */\n>> \twait_queue_head_t wait;\n>> \tsize_t read_offset;\n>> \tunsigned long flags;\n>> @@ -116,12 +118,15 @@ struct occ_client {\n>> \n>> static DEFINE_IDA(occ_ida);\n>> \n>> -static void occ_enqueue_xfr(struct occ_xfr *xfr)\n>> +static int occ_enqueue_xfr(struct occ_xfr *xfr)\n>> {\n>> \tint empty;\n>> \tstruct occ_client *client = to_client(xfr);\n>> \tstruct occ *occ = client->occ;\n>> \n>> +\tif (occ->cancel)\n>> +\t\treturn -ECANCELED;\n>> +\n>> \tspin_lock_irq(&occ->list_lock);\n>> \n>> \tempty = list_empty(&occ->xfrs);\n>> @@ -131,14 +136,15 @@ static void occ_enqueue_xfr(struct occ_xfr *xfr)\n>> \n>> \tif (empty)\n>> \t\tqueue_work(occ_wq, &occ->work);\n>> +\n>> +\treturn 0;\n>> }\n>> \n>> static struct occ_client *occ_open_common(struct occ *occ, unsigned long flags)\n>> {\n>> -\tstruct occ_client *client;\n>> +\tstruct occ_client *client = kzalloc(sizeof(*client), GFP_KERNEL);\n>> \n>> -\tclient = kzalloc(sizeof(*client), GFP_KERNEL);\n>> -\tif (!client)\n>> +\tif (!client || occ->cancel)\n>> \t\treturn NULL;\n>> \n>> \tclient->occ = occ;\n>> @@ -172,6 +178,7 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,\n>> \tint rc;\n>> \tsize_t bytes;\n>> \tstruct occ_xfr *xfr = &client->xfr;\n>> +\tstruct occ *occ = client->occ;\n>> \n>> \tif (len > OCC_SRAM_BYTES)\n>> \t\treturn -EINVAL;\n>> @@ -183,8 +190,9 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,\n>> \t\tif (client->read_offset) {\n>> \t\t\trc = 0;\n>> \t\t\tclient->read_offset = 0;\n>> -\t\t} else\n>> +\t\t} else {\n>> \t\t\trc = -ENOMSG;\n>> +\t\t}\n>> \n>> \t\tgoto done;\n>> \t}\n>> @@ -200,8 +208,11 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,\n>> \t\tspin_unlock_irq(&client->lock);\n>> \n>> \t\trc = wait_event_interruptible(client->wait,\n>> -\t\t\ttest_bit(XFR_COMPLETE, &xfr->flags) ||\n>> -\t\t\ttest_bit(XFR_CANCELED, &xfr->flags));\n>> +\t\t\t\t\t test_bit(XFR_COMPLETE,\n>> +\t\t\t\t\t\t &xfr->flags) ||\n>> +\t\t\t\t\t test_bit(XFR_CANCELED,\n>> +\t\t\t\t\t\t &xfr->flags) ||\n>> +\t\t\t\t\t occ->cancel);\n>> \n>> \t\tspin_lock_irq(&client->lock);\n>> \n>> @@ -225,12 +236,14 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,\n>> \n>> \tbytes = min(len, xfr->resp_data_length - client->read_offset);\n>> \tif (ubuf) {\n>> -\t\tif (copy_to_user(ubuf, &xfr->buf[client->read_offset], bytes)) {\n>> +\t\tif (copy_to_user(ubuf, &xfr->buf[client->read_offset],\n>> +\t\t\t\t bytes)) {\n>> \t\t\trc = -EFAULT;\n>> \t\t\tgoto done;\n>> \t\t}\n>> -\t} else\n>> +\t} else {\n>> \t\tmemcpy(kbuf, &xfr->buf[client->read_offset], bytes);\n>> +\t}\n>> \n>> \tclient->read_offset += bytes;\n>> \n>> @@ -250,12 +263,6 @@ static ssize_t occ_read(struct file *file, char __user *buf, size_t len,\n>> {\n>> \tstruct occ_client *client = file->private_data;\n>> \n>> -\t/* check this ahead of time so we don't go changing the xfr state\n>> -\t * needlessly\n>> -\t */\n>> -\tif (!access_ok(VERIFY_WRITE, buf, len))\n>> -\t\treturn -EFAULT;\n>> -\n>> \treturn occ_read_common(client, buf, NULL, len);\n>> }\n>> \n>> @@ -272,28 +279,31 @@ static ssize_t occ_write_common(struct occ_client *client,\n>> \t\treturn -EINVAL;\n>> \n>> \tspin_lock_irq(&client->lock);\n>> -\tif (test_and_set_bit(CLIENT_XFR_PENDING, &client->flags)) {\n>> +\n>> +\tif (test_bit(CLIENT_XFR_PENDING, &client->flags)) {\n>> \t\trc = -EBUSY;\n>> \t\tgoto done;\n>> \t}\n>> \n>> -\t/* clear out the transfer */\n>> -\tmemset(xfr, 0, sizeof(*xfr));\n>> -\n>> -\txfr->buf[0] = 1;\n>> +\tmemset(xfr, 0, sizeof(*xfr));\t/* clear out the transfer */\n> What about the transfer buffer?\n\nThe buffer is a part of the transfer structure.\n\n>\n>> +\txfr->buf[0] = 1;\t\t/* occ sequence number */\n>> \n>> +\t/* Assume user data follows the occ command format.\n>> +\t * byte 0: command type\n>> +\t * bytes 1-2: data length (msb first)\n>> +\t * bytes 3-n: data\n>> +\t */\n>> \tif (ubuf) {\n>> \t\tif (copy_from_user(&xfr->buf[1], ubuf, len)) {\n>> -\t\t\tkfree(xfr);\n>> \t\t\trc = -EFAULT;\n>> \t\t\tgoto done;\n>> \t\t}\n>> -\t} else\n>> +\t} else {\n>> \t\tmemcpy(&xfr->buf[1], kbuf, len);\n>> +\t}\n>> \n>> \tdata_length = (xfr->buf[2] << 8) + xfr->buf[3];\n> Shouldn't data_length be proportional to len? Like, somewhere in the\n> neighbourhood of len - 3? Should we check that? What if I just send down 3\n> bytes, some kind of command and two others representing the value 4090? Won't\n> we submit a bunch of \"junk\" (overlapping, previously submitted data) to the\n> FIFO?\n\nYes, though it will be 0s, since we cleared it above. But that's the \nuser's problem. Either way you do it, the user has to line up their data \nright. If we send something based on len, if they don't have their data \nlength values correct, the command won't be interpreted correctly \nanyway. And furthermore, if you use len, you don't know if they include \nthe checksum or not, which varies between clients at the moment, though \nyou could ask people to change.\n\n>\n> If the client is shared between higher-level command submitters then you could\n> potentially resubmit the previous command.\n\nThe transfer is cleared out.\n\n>\n>> \tif (data_length > OCC_CMD_DATA_BYTES) {\n>> -\t\tkfree(xfr);\n>> \t\trc = -EINVAL;\n>> \t\tgoto done;\n>> \t}\n>> @@ -306,9 +316,11 @@ static ssize_t occ_write_common(struct occ_client *client,\n>> \n>> \txfr->cmd_data_length = data_length + 6;\n>> \tclient->read_offset = 0;\n>> +\trc = occ_enqueue_xfr(xfr);\n>> +\tif (rc)\n>> +\t\tgoto done;\n>> \n>> -\tocc_enqueue_xfr(xfr);\n>> -\n>> +\tset_bit(CLIENT_XFR_PENDING, &client->flags);\n>> \trc = len;\n>> \n>> done:\n>> @@ -321,12 +333,6 @@ static ssize_t occ_write(struct file *file, const char __user *buf,\n>> {\n>> \tstruct occ_client *client = file->private_data;\n>> \n>> -\t/* check this ahead of time so we don't go changing the xfr state\n>> -\t * needlessly\n>> -\t */\n>> -\tif (!access_ok(VERIFY_READ, buf, len))\n>> -\t\treturn -EFAULT;\n>> -\n>> \treturn occ_write_common(client, buf, NULL, len);\n>> }\n>> \n>> @@ -366,7 +372,7 @@ static int occ_release_common(struct occ_client *client)\n>> \t\treturn 0;\n>> \t}\n>> \n>> -\t/* operation is in progress; let worker clean up*/\n>> +\t/* operation is in progress; let worker clean up */\n>> \tspin_unlock_irq(&occ->list_lock);\n>> \tspin_unlock_irq(&client->lock);\n>> \treturn 0;\n>> @@ -403,7 +409,7 @@ static int occ_write_sbefifo(struct sbefifo_client *client, const char *buf,\n>> \t\ttotal += rc;\n>> \t} while (total < len);\n>> \n>> -\treturn (total == len) ? 0 : -EMSGSIZE;\n>> +\treturn (total == len) ? 0 : -ENODATA;\n> This doesn't seem right for a write, maybe -ENOSPC?\n>> }\n>> \n>> static int occ_read_sbefifo(struct sbefifo_client *client, char *buf,\n>> @@ -422,7 +428,7 @@ static int occ_read_sbefifo(struct sbefifo_client *client, char *buf,\n>> \t\ttotal += rc;\n>> \t} while (total < len);\n>> \n>> -\treturn (total == len) ? 0 : -EMSGSIZE;\n>> +\treturn (total == len) ? 0 : -ENODATA;\n>> }\n>> \n>> static int occ_getsram(struct device *sbefifo, u32 address, u8 *data,\n>> @@ -430,10 +436,13 @@ static int occ_getsram(struct device *sbefifo, u32 address, u8 *data,\n>> {\n>> \tint rc;\n>> \tu8 *resp;\n>> -\tu32 buf[5];\n>> -\tu32 data_len = ((len + 7) / 8) * 8;\n>> +\t__be32 buf[5];\n>> +\tu32 data_len = ((len + 7) / 8) * 8;\t/* must be multiples of 8 B */\n>> \tstruct sbefifo_client *client;\n>> \n>> +\t/* Magic sequence to do SBE getsram command. SBE will fetch data from\n>> +\t * specified SRAM address.\n>> +\t */\n> Do the individual values of the sequence actually mean anything? Or are they\n> truly magic? If possible it would be good to indicate what each does rather\n> than just make some vague claim about them.\n\nThey're mostly just magic SBE values.\n\n>\n>> \tbuf[0] = cpu_to_be32(0x5);\n>> \tbuf[1] = cpu_to_be32(0xa403);\n>> \tbuf[2] = cpu_to_be32(1);\n>> @@ -447,7 +456,7 @@ static int occ_getsram(struct device *sbefifo, u32 address, u8 *data,\n>> \trc = occ_write_sbefifo(client, (const char *)buf, sizeof(buf));\n>> \tif (rc)\n>> \t\tgoto done;\n>> -\t\n>> +\n>> \tresp = kzalloc(data_len, GFP_KERNEL);\n>> \tif (!resp) {\n>> \t\trc = -ENOMEM;\n>> @@ -467,7 +476,7 @@ static int occ_getsram(struct device *sbefifo, u32 address, u8 *data,\n>> \t (be32_to_cpu(buf[1]) == 0xC0DEA403))\n>> \t\tmemcpy(data, resp, len);\n>> \telse\n>> -\t\trc = -EFAULT;\n>> +\t\trc = -EBADMSG;\n>> \n>> free:\n>> \tkfree(resp);\n>> @@ -481,8 +490,8 @@ static int occ_putsram(struct device *sbefifo, u32 address, u8 *data,\n>> \t\t ssize_t len)\n>> {\n>> \tint rc;\n>> -\tu32 *buf;\n>> -\tu32 data_len = ((len + 7) / 8) * 8;\n>> +\t__be32 *buf;\n>> +\tu32 data_len = ((len + 7) / 8) * 8;\t/* must be multiples of 8 B */\n>> \tsize_t cmd_len = data_len + 20;\n>> \tstruct sbefifo_client *client;\n>> \n>> @@ -490,6 +499,9 @@ static int occ_putsram(struct device *sbefifo, u32 address, u8 *data,\n>> \tif (!buf)\n>> \t\treturn -ENOMEM;\n>> \n>> +\t/* Magic sequence to do SBE putsram command. SBE will transfer\n>> +\t * data to specified SRAM address.\n>> +\t */\n> See above.\n>\n>> \tbuf[0] = cpu_to_be32(0x5 + (data_len / 4));\n>> \tbuf[1] = cpu_to_be32(0xa404);\n>> \tbuf[2] = cpu_to_be32(1);\n>> @@ -515,7 +527,7 @@ static int occ_putsram(struct device *sbefifo, u32 address, u8 *data,\n>> \t/* check for good response */\n>> \tif ((be32_to_cpu(buf[0]) != data_len) ||\n>> \t (be32_to_cpu(buf[1]) != 0xC0DEA404))\n>> -\t\trc = -EFAULT;\n>> +\t\trc = -EBADMSG;\n>> \n>> done:\n>> \tsbefifo_drv_release(client);\n>> @@ -527,14 +539,17 @@ static int occ_putsram(struct device *sbefifo, u32 address, u8 *data,\n>> static int occ_trigger_attn(struct device *sbefifo)\n>> {\n>> \tint rc;\n>> -\tu32 buf[6];\n>> +\t__be32 buf[6];\n>> \tstruct sbefifo_client *client;\n>> \n>> +\t/* Magic sequence to do SBE putscom command. SBE will write 8 bytes to\n>> +\t * specified SCOM address.\n>> +\t */\n> See above.\n>\n>> \tbuf[0] = cpu_to_be32(0x6);\n>> \tbuf[1] = cpu_to_be32(0xa202);\n>> \tbuf[2] = 0;\n>> \tbuf[3] = cpu_to_be32(0x6D035);\n>> -\tbuf[4] = cpu_to_be32(0x20010000);\n>> +\tbuf[4] = cpu_to_be32(0x20010000);\t/* trigger occ attention */\n> This kind of comment is what I'm interested in.\n>\n>> \tbuf[5] = 0;\n>> \n>> \tclient = sbefifo_drv_open(sbefifo, 0);\n>> @@ -552,7 +567,7 @@ static int occ_trigger_attn(struct device *sbefifo)\n>> \t/* check for good response */\n>> \tif ((be32_to_cpu(buf[0]) != 0xC0DEA202) ||\n>> \t (be32_to_cpu(buf[1]) & 0x0FFFFFFF))\n>> -\t\trc = -EFAULT;\n>> +\t\trc = -EBADMSG;\n>> \n>> done:\n>> \tsbefifo_drv_release(client);\n>> @@ -564,12 +579,19 @@ static void occ_worker(struct work_struct *work)\n>> {\n>> \tint rc = 0, empty, waiting, canceled;\n>> \tu16 resp_data_length;\n>> +\tunsigned long start;\n>> +\tconst unsigned long timeout = msecs_to_jiffies(OCC_TIMEOUT_MS);\n>> +\tconst long int wait_time = msecs_to_jiffies(OCC_CMD_IN_PRG_WAIT_MS);\n>> \tstruct occ_xfr *xfr;\n>> +\tstruct occ_response *resp;\n>> \tstruct occ_client *client;\n>> \tstruct occ *occ = container_of(work, struct occ, work);\n>> \tstruct device *sbefifo = occ->sbefifo;\n>> \n>> again:\n>> +\tif (occ->cancel)\n>> +\t\treturn;\n>> +\n>> \tspin_lock_irq(&occ->list_lock);\n>> \n>> \txfr = list_first_entry_or_null(&occ->xfrs, struct occ_xfr, link);\n>> @@ -578,11 +600,14 @@ static void occ_worker(struct work_struct *work)\n>> \t\treturn;\n>> \t}\n>> \n>> +\tresp = (struct occ_response *)xfr->buf;\n> Are the members all aligned as necessary?\n\nYes.\n\n>\n>> \tset_bit(XFR_IN_PROGRESS, &xfr->flags);\n>> \n>> \tspin_unlock_irq(&occ->list_lock);\n>> \tmutex_lock(&occ->occ_lock);\n>> \n>> +\t/* write occ command */\n>> +\tstart = jiffies;\n>> \trc = occ_putsram(sbefifo, 0xFFFBE000, xfr->buf,\n>> \t\t\t xfr->cmd_data_length);\n>> \tif (rc)\n>> @@ -592,13 +617,26 @@ static void occ_worker(struct work_struct *work)\n>> \tif (rc)\n>> \t\tgoto done;\n>> \n>> -\trc = occ_getsram(sbefifo, 0xFFFBF000, xfr->buf, 8);\n>> -\tif (rc)\n>> -\t\tgoto done;\n>> +\t/* read occ response */\n>> +\tdo {\n>> +\t\trc = occ_getsram(sbefifo, 0xFFFBF000, xfr->buf, 8);\n>> +\t\tif (rc)\n>> +\t\t\tgoto done;\n>> +\n>> +\t\tif (resp->return_status == OCC_RESP_CMD_IN_PRG) {\n>> +\t\t\trc = -EALREADY;\n>> +\n>> +\t\t\tif (time_after(jiffies, start + timeout))\n>> +\t\t\t\tbreak;\n>> \n>> -\tresp_data_length = (xfr->buf[3] << 8) + xfr->buf[4];\n>> +\t\t\tset_current_state(TASK_INTERRUPTIBLE);\n>> +\t\t\tschedule_timeout(wait_time);\n>> +\t\t}\n>> +\t} while (rc);\n>> +\n>> +\tresp_data_length = get_unaligned_be16(&resp->data_length);\n>> \tif (resp_data_length > OCC_RESP_DATA_BYTES) {\n>> -\t\trc = -EDOM;\n>> +\t\trc = -EMSGSIZE;\n>> \t\tgoto done;\n>> \t}\n>> \n>> @@ -704,9 +742,6 @@ static int occ_probe(struct platform_device *pdev)\n>> \tmutex_init(&occ->occ_lock);\n>> \tINIT_WORK(&occ->work, occ_worker);\n>> \n>> -\t/* ensure NULL before we probe children, so they don't hang FSI */\n>> -\tplatform_set_drvdata(pdev, NULL);\n>> -\n>> \tif (dev->of_node) {\n>> \t\trc = of_property_read_u32(dev->of_node, \"reg\", ®);\n>> \t\tif (!rc) {\n>> @@ -716,23 +751,13 @@ static int occ_probe(struct platform_device *pdev)\n>> \t\t\tif (occ->idx < 0)\n>> \t\t\t\tocc->idx = ida_simple_get(&occ_ida, 1, INT_MAX,\n>> \t\t\t\t\t\t\t GFP_KERNEL);\n>> -\t\t} else\n>> +\t\t} else {\n>> \t\t\tocc->idx = ida_simple_get(&occ_ida, 1, INT_MAX,\n>> \t\t\t\t\t\t GFP_KERNEL);\n>> -\n>> -\t\t/* create platform devs for dts child nodes (hwmon, etc) */\n>> -\t\tfor_each_child_of_node(dev->of_node, np) {\n>> -\t\t\tsnprintf(child_name, sizeof(child_name), \"occ%d-dev%d\",\n>> -\t\t\t\t occ->idx, 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(dev,\n>> -\t\t\t\t\t \"failed to create child node dev\\n\");\n>> \t\t}\n>> -\t} else\n>> +\t} else {\n>> \t\tocc->idx = ida_simple_get(&occ_ida, 1, INT_MAX, GFP_KERNEL);\n>> -\n>> -\tplatform_set_drvdata(pdev, occ);\n>> +\t}\n>> \n>> \tsnprintf(occ->name, sizeof(occ->name), \"occ%d\", occ->idx);\n>> \tocc->mdev.fops = &occ_fops;\n>> @@ -742,20 +767,42 @@ static int occ_probe(struct platform_device *pdev)\n>> \n>> \trc = misc_register(&occ->mdev);\n>> \tif (rc) {\n>> -\t\tdev_err(dev, \"failed to register miscdevice\\n\");\n>> +\t\tdev_err(dev, \"failed to register miscdevice: %d\\n\", rc);\n>> +\t\tida_simple_remove(&occ_ida, occ->idx);\n>> \t\treturn rc;\n>> \t}\n>> \n>> +\t/* create platform devs for dts child nodes (hwmon, etc) */\n>> +\tfor_each_available_child_of_node(dev->of_node, np) {\n>> +\t\tsnprintf(child_name, sizeof(child_name), \"occ%d-dev%d\",\n>> +\t\t\t occ->idx, 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 node dev\\n\");\n>> +\t}\n>> +\n>> +\tplatform_set_drvdata(pdev, occ);\n>> +\n> Similar to the SBEFIFO, it would be nice to separate out the miscdev from the\n> core.\n\nThought about it... it gets complicated to avoid replicating a lot of code.\n\nThanks for the review!\nEddie\n\n>\n>> \treturn 0;\n>> }\n>> \n>> static int occ_remove(struct platform_device *pdev)\n>> {\n>> \tstruct occ *occ = platform_get_drvdata(pdev);\n>> +\tstruct occ_xfr *xfr;\n>> +\tstruct occ_client *client;\n>> +\n>> +\tocc->cancel = true;\n>> +\tlist_for_each_entry(xfr, &occ->xfrs, link) {\n>> +\t\tclient = to_client(xfr);\n>> +\t\twake_up_interruptible(&client->wait);\n>> +\t}\n>> \n>> -\tflush_work(&occ->work);\n>> \tmisc_deregister(&occ->mdev);\n>> \tdevice_for_each_child(&pdev->dev, NULL, occ_unregister_child);\n>> +\n>> +\tflush_work(&occ->work);\n> This should be okay now that we have the -ENODEV in the SBEFIFO, right (if\n> that's all nice and race-free)? That is, sbefifo_read_ready() and\n> sbefifo_write_ready() should trigger and the associated\n> sbefifo_{read,write}_common() pulls -ENODEV out of sbefifo->rc?\n>\n>> +\n>> \tida_simple_remove(&occ_ida, occ->idx);\n>> \n>> \treturn 0;\n>> @@ -789,6 +836,8 @@ static void occ_exit(void)\n>> \tdestroy_workqueue(occ_wq);\n>> \n>> \tplatform_driver_unregister(&occ_driver);\n>> +\n>> +\tida_destroy(&occ_ida);\n>> }\n>> \n>> module_init(occ_init);\n>> diff --git a/include/linux/occ.h b/include/linux/occ.h\n>> index d78332c..0a4a54a 100644\n>> --- a/include/linux/occ.h\n>> +++ b/include/linux/occ.h\n>> @@ -11,12 +11,26 @@\n>> * GNU General Public License for more details.\n>> */\n>> \n>> -#ifndef __OCC_H__\n>> -#define __OCC_H__\n>> +#ifndef LINUX_FSI_OCC_H\n>> +#define LINUX_FSI_OCC_H\n>> \n>> struct device;\n>> struct occ_client;\n>> \n>> +#define OCC_RESP_CMD_IN_PRG\t\t0xFF\n>> +#define OCC_RESP_SUCCESS\t\t0\n>> +#define OCC_RESP_CMD_INVAL\t\t0x11\n>> +#define OCC_RESP_CMD_LEN_INVAL\t\t0x12\n>> +#define OCC_RESP_DATA_INVAL\t\t0x13\n>> +#define OCC_RESP_CHKSUM_ERR\t\t0x14\n>> +#define OCC_RESP_INT_ERR\t\t0x15\n>> +#define OCC_RESP_BAD_STATE\t\t0x16\n>> +#define OCC_RESP_CRIT_EXCEPT\t\t0xE0\n>> +#define OCC_RESP_CRIT_INIT\t\t0xE1\n>> +#define OCC_RESP_CRIT_WATCHDOG\t\t0xE2\n>> +#define OCC_RESP_CRIT_OCB\t\t0xE3\n>> +#define OCC_RESP_CRIT_HW\t\t0xE4\n>> +\n>> extern struct occ_client *occ_drv_open(struct device *dev,\n>> \t\t\t\t unsigned long flags);\n>> extern int occ_drv_read(struct occ_client *client, char *buf, size_t len);\n>> @@ -24,4 +38,4 @@ extern int occ_drv_write(struct occ_client *client, const char *buf,\n>> \t\t\t size_t len);\n>> extern void occ_drv_release(struct occ_client *client);\n>> \n>> -#endif /* __OCC_H__ */\n>> +#endif /* LINUX_FSI_OCC_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 [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 3y3mkR6XMQz9t60\n\tfor ;\n\tSat, 30 Sep 2017 08:41:19 +1000 (AEST)","from lists.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3])\n\tby lists.ozlabs.org (Postfix) with ESMTP id 3y3mkR5hfBzDqfx\n\tfor ;\n\tSat, 30 Sep 2017 08:41:19 +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 3y3mkH37lbzDqG2\n\tfor ; Sat, 30 Sep 2017 08:41:10 +1000 (AEST)","from pps.filterd (m0098394.ppops.net [127.0.0.1])\n\tby mx0a-001b2d01.pphosted.com (8.16.0.21/8.16.0.21) with SMTP id\n\tv8TMdUHZ034620\n\tfor ; Fri, 29 Sep 2017 18:41:08 -0400","from e34.co.us.ibm.com (e34.co.us.ibm.com [32.97.110.152])\n\tby mx0a-001b2d01.pphosted.com with ESMTP id 2d9tj8bv0m-1\n\t(version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT)\n\tfor ; Fri, 29 Sep 2017 18:41:08 -0400","from localhost\n\tby e34.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:41:07 -0600","from b03cxnp07028.gho.boulder.ibm.com (9.17.130.15)\n\tby e34.co.us.ibm.com (192.168.1.134) with IBM ESMTP SMTP Gateway:\n\tAuthorized Use Only! Violators will be prosecuted; \n\tFri, 29 Sep 2017 16:41:05 -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 v8TMf5RX65929426; Fri, 29 Sep 2017 15:41:05 -0700","from b03ledav006.gho.boulder.ibm.com (unknown [127.0.0.1])\n\tby IMSVA (Postfix) with ESMTP id 2F444C6037;\n\tFri, 29 Sep 2017 16:41:05 -0600 (MDT)","from oc3016140333.ibm.com (unknown [9.85.183.77])\n\tby b03ledav006.gho.boulder.ibm.com (Postfix) with ESMTP id 4FD51C603E;\n\tFri, 29 Sep 2017 16:41:04 -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 2/3] drivers/fsi/occ: refactor to upstream\n\tlist state","To":"Andrew Jeffery , openbmc@lists.ozlabs.org","References":"<1506033838-2078-1-git-send-email-eajames@linux.vnet.ibm.com>\n\t<1506033838-2078-3-git-send-email-eajames@linux.vnet.ibm.com>\n\t<1506320562.30138.24.camel@aj.id.au>","From":"Eddie James ","Date":"Fri, 29 Sep 2017 17:41:03 -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":"<1506320562.30138.24.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-0016-0000-0000-00000796D37B","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.00924260; UDB=6.00464728;\n\tIPR=6.00704398; \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:41:06","X-IBM-AV-DETECTION":"SAVI=unused REMOTE=unused XFE=unused","x-cbparentid":"17092922-0017-0000-0000-00003BAB18CF","Message-Id":"","X-Proofpoint-Virus-Version":"vendor=fsecure engine=2.50.10432:, ,\n\tdefinitions=2017-09-29_06:, , signatures=0","X-Proofpoint-Spam-Details":"rule=outbound_notspam policy=outbound score=0\n\tspamscore=0 suspectscore=2\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-1709290322","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"}}]