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

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

Commit Message

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

Includes various fixes:
 - Fix probe failure scenario
 - General cleanup
 - Reorder remove() operations for safety
 - Add cancel boolean to prevent further xfrs when we're removing

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

Comments

Eddie James Sept. 21, 2017, 10:55 p.m. | #1
On 09/21/2017 05:43 PM, Eddie James wrote:
> From: "Edward A. James" <eajames@us.ibm.com>
>
> Includes various fixes:
>   - Fix probe failure scenario
>   - General cleanup
>   - Reorder remove() operations for safety
>   - Add cancel boolean to prevent further xfrs when we're removing

And with this one, hasn't been sent upstream yet.

Eddie

>
> Signed-off-by: Edward A. James <eajames@us.ibm.com>
> ---
>   drivers/fsi/occ.c   | 227 ++++++++++++++++++++++++++++++++--------------------
>   include/linux/occ.h |  20 ++++-
>   2 files changed, 155 insertions(+), 92 deletions(-)
>
> diff --git a/drivers/fsi/occ.c b/drivers/fsi/occ.c
> index 621fbf0..4dd5048 100644
> --- a/drivers/fsi/occ.c
> +++ b/drivers/fsi/occ.c
> @@ -10,16 +10,22 @@
>   #include <asm/unaligned.h>
>   #include <linux/device.h>
>   #include <linux/err.h>
> +#include <linux/errno.h>
> +#include <linux/fs.h>
>   #include <linux/fsi-sbefifo.h>
> -#include <linux/init.h>
> +#include <linux/idr.h>
>   #include <linux/kernel.h>
> +#include <linux/list.h>
>   #include <linux/miscdevice.h>
>   #include <linux/module.h>
>   #include <linux/mutex.h>
> +#include <linux/occ.h>
>   #include <linux/of.h>
>   #include <linux/of_platform.h>
>   #include <linux/platform_device.h>
> +#include <linux/sched.h>
>   #include <linux/slab.h>
> +#include <linux/spinlock.h>
>   #include <linux/uaccess.h>
>   #include <linux/wait.h>
>   #include <linux/workqueue.h>
> @@ -28,49 +34,45 @@
>   #define OCC_CMD_DATA_BYTES	4090
>   #define OCC_RESP_DATA_BYTES	4089
>
> +#define OCC_TIMEOUT_MS		1000
> +#define OCC_CMD_IN_PRG_WAIT_MS	50
> +
>   struct occ {
>   	struct device *sbefifo;
>   	char name[32];
>   	int idx;
>   	struct miscdevice mdev;
>   	struct list_head xfrs;
> -	spinlock_t list_lock;
> -	struct mutex occ_lock;
> +	spinlock_t list_lock;		/* lock access to the xfrs list */
> +	struct mutex occ_lock;		/* lock access to the hardware */
>   	struct work_struct work;
> +	bool cancel;
>   };
>
>   #define to_occ(x)	container_of((x), struct occ, mdev)
>
> -struct occ_command {
> -	u8 seq_no;
> -	u8 cmd_type;
> -	u16 data_length;
> -	u8 data[OCC_CMD_DATA_BYTES];
> -	u16 checksum;
> -} __packed;
> -
>   struct occ_response {
>   	u8 seq_no;
>   	u8 cmd_type;
>   	u8 return_status;
> -	u16 data_length;
> +	__be16 data_length;
>   	u8 data[OCC_RESP_DATA_BYTES];
> -	u16 checksum;
> +	__be16 checksum;
>   } __packed;
>
>   /*
>    * transfer flags are NOT mutually exclusive
> - *
> + *
>    * Initial flags are none; transfer is created and queued from write(). All
> - * 	flags are cleared when the transfer is completed by closing the file or
> - * 	reading all of the available response data.
> + *  flags are cleared when the transfer is completed by closing the file or
> + *  reading all of the available response data.
>    * XFR_IN_PROGRESS is set when a transfer is started from occ_worker_putsram,
> - * 	and cleared if the transfer fails or occ_worker_getsram completes.
> + *  and cleared if the transfer fails or occ_worker_getsram completes.
>    * XFR_COMPLETE is set when a transfer fails or finishes occ_worker_getsram.
>    * XFR_CANCELED is set when the transfer's client is released.
>    * XFR_WAITING is set from read() if the transfer isn't complete and
> - * 	NONBLOCKING wasn't specified. Cleared in read() when transfer completes
> - * 	or fails.
> + *  O_NONBLOCK wasn't specified. Cleared in read() when transfer completes or
> + *  fails.
>    */
>   enum {
>   	XFR_IN_PROGRESS,
> @@ -92,9 +94,9 @@ struct occ_xfr {
>    * client flags
>    *
>    * CLIENT_NONBLOCKING is set during open() if the file was opened with the
> - * 	O_NONBLOCKING flag.
> + *  O_NONBLOCK flag.
>    * CLIENT_XFR_PENDING is set during write() and cleared when all data has been
> - * 	read.
> + *  read.
>    */
>   enum {
>   	CLIENT_NONBLOCKING,
> @@ -104,7 +106,7 @@ enum {
>   struct occ_client {
>   	struct occ *occ;
>   	struct occ_xfr xfr;
> -	spinlock_t lock;
> +	spinlock_t lock;		/* lock access to the client state */
>   	wait_queue_head_t wait;
>   	size_t read_offset;
>   	unsigned long flags;
> @@ -116,12 +118,15 @@ struct occ_client {
>
>   static DEFINE_IDA(occ_ida);
>
> -static void occ_enqueue_xfr(struct occ_xfr *xfr)
> +static int occ_enqueue_xfr(struct occ_xfr *xfr)
>   {
>   	int empty;
>   	struct occ_client *client = to_client(xfr);
>   	struct occ *occ = client->occ;
>
> +	if (occ->cancel)
> +		return -ECANCELED;
> +
>   	spin_lock_irq(&occ->list_lock);
>
>   	empty = list_empty(&occ->xfrs);
> @@ -131,14 +136,15 @@ static void occ_enqueue_xfr(struct occ_xfr *xfr)
>
>   	if (empty)
>   		queue_work(occ_wq, &occ->work);
> +
> +	return 0;
>   }
>
>   static struct occ_client *occ_open_common(struct occ *occ, unsigned long flags)
>   {
> -	struct occ_client *client;
> +	struct occ_client *client = kzalloc(sizeof(*client), GFP_KERNEL);
>
> -	client = kzalloc(sizeof(*client), GFP_KERNEL);
> -	if (!client)
> +	if (!client || occ->cancel)
>   		return NULL;
>
>   	client->occ = occ;
> @@ -172,6 +178,7 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
>   	int rc;
>   	size_t bytes;
>   	struct occ_xfr *xfr = &client->xfr;
> +	struct occ *occ = client->occ;
>
>   	if (len > OCC_SRAM_BYTES)
>   		return -EINVAL;
> @@ -183,8 +190,9 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
>   		if (client->read_offset) {
>   			rc = 0;
>   			client->read_offset = 0;
> -		} else
> +		} else {
>   			rc = -ENOMSG;
> +		}
>
>   		goto done;
>   	}
> @@ -200,8 +208,11 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
>   		spin_unlock_irq(&client->lock);
>
>   		rc = wait_event_interruptible(client->wait,
> -			test_bit(XFR_COMPLETE, &xfr->flags) ||
> -			test_bit(XFR_CANCELED, &xfr->flags));
> +					      test_bit(XFR_COMPLETE,
> +						       &xfr->flags) ||
> +					      test_bit(XFR_CANCELED,
> +						       &xfr->flags) ||
> +					      occ->cancel);
>
>   		spin_lock_irq(&client->lock);
>
> @@ -225,12 +236,14 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
>
>   	bytes = min(len, xfr->resp_data_length - client->read_offset);
>   	if (ubuf) {
> -		if (copy_to_user(ubuf, &xfr->buf[client->read_offset], bytes)) {
> +		if (copy_to_user(ubuf, &xfr->buf[client->read_offset],
> +				 bytes)) {
>   			rc = -EFAULT;
>   			goto done;
>   		}
> -	} else
> +	} else {
>   		memcpy(kbuf, &xfr->buf[client->read_offset], bytes);
> +	}
>
>   	client->read_offset += bytes;
>
> @@ -250,12 +263,6 @@ static ssize_t occ_read(struct file *file, char __user *buf, size_t len,
>   {
>   	struct occ_client *client = file->private_data;
>
> -	/* check this ahead of time so we don't go changing the xfr state
> -	 * needlessly
> -	 */
> -	if (!access_ok(VERIFY_WRITE, buf, len))
> -		return -EFAULT;
> -
>   	return occ_read_common(client, buf, NULL, len);
>   }
>
> @@ -272,28 +279,31 @@ static ssize_t occ_write_common(struct occ_client *client,
>   		return -EINVAL;
>
>   	spin_lock_irq(&client->lock);
> -	if (test_and_set_bit(CLIENT_XFR_PENDING, &client->flags)) {
> +
> +	if (test_bit(CLIENT_XFR_PENDING, &client->flags)) {
>   		rc = -EBUSY;
>   		goto done;
>   	}
>
> -	/* clear out the transfer */
> -	memset(xfr, 0, sizeof(*xfr));
> -
> -	xfr->buf[0] = 1;
> +	memset(xfr, 0, sizeof(*xfr));	/* clear out the transfer */
> +	xfr->buf[0] = 1;		/* occ sequence number */
>
> +	/* Assume user data follows the occ command format.
> +	 * byte 0: command type
> +	 * bytes 1-2: data length (msb first)
> +	 * bytes 3-n: data
> +	 */
>   	if (ubuf) {
>   		if (copy_from_user(&xfr->buf[1], ubuf, len)) {
> -			kfree(xfr);
>   			rc = -EFAULT;
>   			goto done;
>   		}
> -	} else
> +	} else {
>   		memcpy(&xfr->buf[1], kbuf, len);
> +	}
>
>   	data_length = (xfr->buf[2] << 8) + xfr->buf[3];
>   	if (data_length > OCC_CMD_DATA_BYTES) {
> -		kfree(xfr);
>   		rc = -EINVAL;
>   		goto done;
>   	}
> @@ -306,9 +316,11 @@ static ssize_t occ_write_common(struct occ_client *client,
>
>   	xfr->cmd_data_length = data_length + 6;
>   	client->read_offset = 0;
> +	rc = occ_enqueue_xfr(xfr);
> +	if (rc)
> +		goto done;
>
> -	occ_enqueue_xfr(xfr);
> -
> +	set_bit(CLIENT_XFR_PENDING, &client->flags);
>   	rc = len;
>
>   done:
> @@ -321,12 +333,6 @@ static ssize_t occ_write(struct file *file, const char __user *buf,
>   {
>   	struct occ_client *client = file->private_data;
>
> -	/* check this ahead of time so we don't go changing the xfr state
> -	 * needlessly
> -	 */
> -	if (!access_ok(VERIFY_READ, buf, len))
> -		return -EFAULT;
> -
>   	return occ_write_common(client, buf, NULL, len);
>   }
>
> @@ -366,7 +372,7 @@ static int occ_release_common(struct occ_client *client)
>   		return 0;
>   	}
>
> -	/* operation is in progress; let worker clean up*/
> +	/* operation is in progress; let worker clean up */
>   	spin_unlock_irq(&occ->list_lock);
>   	spin_unlock_irq(&client->lock);
>   	return 0;
> @@ -403,7 +409,7 @@ static int occ_write_sbefifo(struct sbefifo_client *client, const char *buf,
>   		total += rc;
>   	} while (total < len);
>
> -	return (total == len) ? 0 : -EMSGSIZE;
> +	return (total == len) ? 0 : -ENODATA;
>   }
>
>   static int occ_read_sbefifo(struct sbefifo_client *client, char *buf,
> @@ -422,7 +428,7 @@ static int occ_read_sbefifo(struct sbefifo_client *client, char *buf,
>   		total += rc;
>   	} while (total < len);
>
> -	return (total == len) ? 0 : -EMSGSIZE;
> +	return (total == len) ? 0 : -ENODATA;
>   }
>
>   static int occ_getsram(struct device *sbefifo, u32 address, u8 *data,
> @@ -430,10 +436,13 @@ static int occ_getsram(struct device *sbefifo, u32 address, u8 *data,
>   {
>   	int rc;
>   	u8 *resp;
> -	u32 buf[5];
> -	u32 data_len = ((len + 7) / 8) * 8;
> +	__be32 buf[5];
> +	u32 data_len = ((len + 7) / 8) * 8;	/* must be multiples of 8 B */
>   	struct sbefifo_client *client;
>
> +	/* Magic sequence to do SBE getsram command. SBE will fetch data from
> +	 * specified SRAM address.
> +	 */
>   	buf[0] = cpu_to_be32(0x5);
>   	buf[1] = cpu_to_be32(0xa403);
>   	buf[2] = cpu_to_be32(1);
> @@ -447,7 +456,7 @@ static int occ_getsram(struct device *sbefifo, u32 address, u8 *data,
>   	rc = occ_write_sbefifo(client, (const char *)buf, sizeof(buf));
>   	if (rc)
>   		goto done;
> -	
> +
>   	resp = kzalloc(data_len, GFP_KERNEL);
>   	if (!resp) {
>   		rc = -ENOMEM;
> @@ -467,7 +476,7 @@ static int occ_getsram(struct device *sbefifo, u32 address, u8 *data,
>   	    (be32_to_cpu(buf[1]) == 0xC0DEA403))
>   		memcpy(data, resp, len);
>   	else
> -		rc = -EFAULT;
> +		rc = -EBADMSG;
>
>   free:
>   	kfree(resp);
> @@ -481,8 +490,8 @@ static int occ_putsram(struct device *sbefifo, u32 address, u8 *data,
>   		       ssize_t len)
>   {
>   	int rc;
> -	u32 *buf;
> -	u32 data_len = ((len + 7) / 8) * 8;
> +	__be32 *buf;
> +	u32 data_len = ((len + 7) / 8) * 8;	/* must be multiples of 8 B */
>   	size_t cmd_len = data_len + 20;
>   	struct sbefifo_client *client;
>
> @@ -490,6 +499,9 @@ static int occ_putsram(struct device *sbefifo, u32 address, u8 *data,
>   	if (!buf)
>   		return -ENOMEM;
>
> +	/* Magic sequence to do SBE putsram command. SBE will transfer
> +	 * data to specified SRAM address.
> +	 */
>   	buf[0] = cpu_to_be32(0x5 + (data_len / 4));
>   	buf[1] = cpu_to_be32(0xa404);
>   	buf[2] = cpu_to_be32(1);
> @@ -515,7 +527,7 @@ static int occ_putsram(struct device *sbefifo, u32 address, u8 *data,
>   	/* check for good response */
>   	if ((be32_to_cpu(buf[0]) != data_len) ||
>   	    (be32_to_cpu(buf[1]) != 0xC0DEA404))
> -		rc = -EFAULT;
> +		rc = -EBADMSG;
>
>   done:
>   	sbefifo_drv_release(client);
> @@ -527,14 +539,17 @@ static int occ_putsram(struct device *sbefifo, u32 address, u8 *data,
>   static int occ_trigger_attn(struct device *sbefifo)
>   {
>   	int rc;
> -	u32 buf[6];
> +	__be32 buf[6];
>   	struct sbefifo_client *client;
>
> +	/* Magic sequence to do SBE putscom command. SBE will write 8 bytes to
> +	 * specified SCOM address.
> +	 */
>   	buf[0] = cpu_to_be32(0x6);
>   	buf[1] = cpu_to_be32(0xa202);
>   	buf[2] = 0;
>   	buf[3] = cpu_to_be32(0x6D035);
> -	buf[4] = cpu_to_be32(0x20010000);
> +	buf[4] = cpu_to_be32(0x20010000);	/* trigger occ attention */
>   	buf[5] = 0;
>
>   	client = sbefifo_drv_open(sbefifo, 0);
> @@ -552,7 +567,7 @@ static int occ_trigger_attn(struct device *sbefifo)
>   	/* check for good response */
>   	if ((be32_to_cpu(buf[0]) != 0xC0DEA202) ||
>   	    (be32_to_cpu(buf[1]) & 0x0FFFFFFF))
> -		rc = -EFAULT;
> +		rc = -EBADMSG;
>
>   done:
>   	sbefifo_drv_release(client);
> @@ -564,12 +579,19 @@ static void occ_worker(struct work_struct *work)
>   {
>   	int rc = 0, empty, waiting, canceled;
>   	u16 resp_data_length;
> +	unsigned long start;
> +	const unsigned long timeout = msecs_to_jiffies(OCC_TIMEOUT_MS);
> +	const long int wait_time = msecs_to_jiffies(OCC_CMD_IN_PRG_WAIT_MS);
>   	struct occ_xfr *xfr;
> +	struct occ_response *resp;
>   	struct occ_client *client;
>   	struct occ *occ = container_of(work, struct occ, work);
>   	struct device *sbefifo = occ->sbefifo;
>
>   again:
> +	if (occ->cancel)
> +		return;
> +
>   	spin_lock_irq(&occ->list_lock);
>
>   	xfr = list_first_entry_or_null(&occ->xfrs, struct occ_xfr, link);
> @@ -578,11 +600,14 @@ static void occ_worker(struct work_struct *work)
>   		return;
>   	}
>
> +	resp = (struct occ_response *)xfr->buf;
>   	set_bit(XFR_IN_PROGRESS, &xfr->flags);
>
>   	spin_unlock_irq(&occ->list_lock);
>   	mutex_lock(&occ->occ_lock);
>
> +	/* write occ command */
> +	start = jiffies;
>   	rc = occ_putsram(sbefifo, 0xFFFBE000, xfr->buf,
>   			 xfr->cmd_data_length);
>   	if (rc)
> @@ -592,13 +617,26 @@ static void occ_worker(struct work_struct *work)
>   	if (rc)
>   		goto done;
>
> -	rc = occ_getsram(sbefifo, 0xFFFBF000, xfr->buf, 8);
> -	if (rc)
> -		goto done;
> +	/* read occ response */
> +	do {
> +		rc = occ_getsram(sbefifo, 0xFFFBF000, xfr->buf, 8);
> +		if (rc)
> +			goto done;
> +
> +		if (resp->return_status == OCC_RESP_CMD_IN_PRG) {
> +			rc = -EALREADY;
> +
> +			if (time_after(jiffies, start + timeout))
> +				break;
>
> -	resp_data_length = (xfr->buf[3] << 8) + xfr->buf[4];
> +			set_current_state(TASK_INTERRUPTIBLE);
> +			schedule_timeout(wait_time);
> +		}
> +	} while (rc);
> +
> +	resp_data_length = get_unaligned_be16(&resp->data_length);
>   	if (resp_data_length > OCC_RESP_DATA_BYTES) {
> -		rc = -EDOM;
> +		rc = -EMSGSIZE;
>   		goto done;
>   	}
>
> @@ -704,9 +742,6 @@ static int occ_probe(struct platform_device *pdev)
>   	mutex_init(&occ->occ_lock);
>   	INIT_WORK(&occ->work, occ_worker);
>
> -	/* ensure NULL before we probe children, so they don't hang FSI */
> -	platform_set_drvdata(pdev, NULL);
> -
>   	if (dev->of_node) {
>   		rc = of_property_read_u32(dev->of_node, "reg", &reg);
>   		if (!rc) {
> @@ -716,23 +751,13 @@ static int occ_probe(struct platform_device *pdev)
>   			if (occ->idx < 0)
>   				occ->idx = ida_simple_get(&occ_ida, 1, INT_MAX,
>   							  GFP_KERNEL);
> -		} else
> +		} else {
>   			occ->idx = ida_simple_get(&occ_ida, 1, INT_MAX,
>   						  GFP_KERNEL);
> -
> -		/* create platform devs for dts child nodes (hwmon, etc) */
> -		for_each_child_of_node(dev->of_node, np) {
> -			snprintf(child_name, sizeof(child_name), "occ%d-dev%d",
> -				 occ->idx, child_idx++);
> -			child = of_platform_device_create(np, child_name, dev);
> -			if (!child)
> -				dev_warn(dev,
> -					 "failed to create child node dev\n");
>   		}
> -	} else
> +	} else {
>   		occ->idx = ida_simple_get(&occ_ida, 1, INT_MAX, GFP_KERNEL);
> -
> -	platform_set_drvdata(pdev, occ);
> +	}
>
>   	snprintf(occ->name, sizeof(occ->name), "occ%d", occ->idx);
>   	occ->mdev.fops = &occ_fops;
> @@ -742,20 +767,42 @@ static int occ_probe(struct platform_device *pdev)
>
>   	rc = misc_register(&occ->mdev);
>   	if (rc) {
> -		dev_err(dev, "failed to register miscdevice\n");
> +		dev_err(dev, "failed to register miscdevice: %d\n", rc);
> +		ida_simple_remove(&occ_ida, occ->idx);
>   		return rc;
>   	}
>
> +	/* create platform devs for dts child nodes (hwmon, etc) */
> +	for_each_available_child_of_node(dev->of_node, np) {
> +		snprintf(child_name, sizeof(child_name), "occ%d-dev%d",
> +			 occ->idx, child_idx++);
> +		child = of_platform_device_create(np, child_name, dev);
> +		if (!child)
> +			dev_warn(dev, "failed to create child node dev\n");
> +	}
> +
> +	platform_set_drvdata(pdev, occ);
> +
>   	return 0;
>   }
>
>   static int occ_remove(struct platform_device *pdev)
>   {
>   	struct occ *occ = platform_get_drvdata(pdev);
> +	struct occ_xfr *xfr;
> +	struct occ_client *client;
> +
> +	occ->cancel = true;
> +	list_for_each_entry(xfr, &occ->xfrs, link) {
> +		client = to_client(xfr);
> +		wake_up_interruptible(&client->wait);
> +	}
>
> -	flush_work(&occ->work);
>   	misc_deregister(&occ->mdev);
>   	device_for_each_child(&pdev->dev, NULL, occ_unregister_child);
> +
> +	flush_work(&occ->work);
> +
>   	ida_simple_remove(&occ_ida, occ->idx);
>
>   	return 0;
> @@ -789,6 +836,8 @@ static void occ_exit(void)
>   	destroy_workqueue(occ_wq);
>
>   	platform_driver_unregister(&occ_driver);
> +
> +	ida_destroy(&occ_ida);
>   }
>
>   module_init(occ_init);
> diff --git a/include/linux/occ.h b/include/linux/occ.h
> index d78332c..0a4a54a 100644
> --- a/include/linux/occ.h
> +++ b/include/linux/occ.h
> @@ -11,12 +11,26 @@
>    * GNU General Public License for more details.
>    */
>
> -#ifndef __OCC_H__
> -#define __OCC_H__
> +#ifndef LINUX_FSI_OCC_H
> +#define LINUX_FSI_OCC_H
>
>   struct device;
>   struct occ_client;
>
> +#define OCC_RESP_CMD_IN_PRG		0xFF
> +#define OCC_RESP_SUCCESS		0
> +#define OCC_RESP_CMD_INVAL		0x11
> +#define OCC_RESP_CMD_LEN_INVAL		0x12
> +#define OCC_RESP_DATA_INVAL		0x13
> +#define OCC_RESP_CHKSUM_ERR		0x14
> +#define OCC_RESP_INT_ERR		0x15
> +#define OCC_RESP_BAD_STATE		0x16
> +#define OCC_RESP_CRIT_EXCEPT		0xE0
> +#define OCC_RESP_CRIT_INIT		0xE1
> +#define OCC_RESP_CRIT_WATCHDOG		0xE2
> +#define OCC_RESP_CRIT_OCB		0xE3
> +#define OCC_RESP_CRIT_HW		0xE4
> +
>   extern struct occ_client *occ_drv_open(struct device *dev,
>   				       unsigned long flags);
>   extern int occ_drv_read(struct occ_client *client, char *buf, size_t len);
> @@ -24,4 +38,4 @@ extern int occ_drv_write(struct occ_client *client, const char *buf,
>   			 size_t len);
>   extern void occ_drv_release(struct occ_client *client);
>
> -#endif /* __OCC_H__ */
> +#endif /* LINUX_FSI_OCC_H */
Andrew Jeffery Sept. 25, 2017, 6:22 a.m. | #2
On Thu, 2017-09-21 at 17:43 -0500, Eddie James wrote:
> From: "Edward A. James" <eajames@us.ibm.com>

> Includes various fixes:
>  - Fix probe failure scenario
>  - General cleanup
>  - Reorder remove() operations for safety
>  - Add cancel boolean to prevent further xfrs when we're removing

See comment on previous patch about these bullet points.

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

> diff --git a/drivers/fsi/occ.c b/drivers/fsi/occ.c
> index 621fbf0..4dd5048 100644
> --- a/drivers/fsi/occ.c
> +++ b/drivers/fsi/occ.c
> @@ -10,16 +10,22 @@
>  #include <asm/unaligned.h>
>  #include <linux/device.h>
>  #include <linux/err.h>
> +#include <linux/errno.h>
> +#include <linux/fs.h>
>  #include <linux/fsi-sbefifo.h>
> -#include <linux/init.h>
> +#include <linux/idr.h>
>  #include <linux/kernel.h>
> +#include <linux/list.h>
>  #include <linux/miscdevice.h>
>  #include <linux/module.h>
>  #include <linux/mutex.h>
> +#include <linux/occ.h>
>  #include <linux/of.h>
>  #include <linux/of_platform.h>
>  #include <linux/platform_device.h>
> +#include <linux/sched.h>
>  #include <linux/slab.h>
> +#include <linux/spinlock.h>
>  #include <linux/uaccess.h>
>  #include <linux/wait.h>
>  #include <linux/workqueue.h>
> @@ -28,49 +34,45 @@
>  #define OCC_CMD_DATA_BYTES	4090
>  #define OCC_RESP_DATA_BYTES	4089
>  
> +#define OCC_TIMEOUT_MS		1000
> +#define OCC_CMD_IN_PRG_WAIT_MS	50
> +
>  struct occ {
>  	struct device *sbefifo;
>  	char name[32];
>  	int idx;
>  	struct miscdevice mdev;
>  	struct list_head xfrs;
> -	spinlock_t list_lock;
> -	struct mutex occ_lock;
> +	spinlock_t list_lock;		/* lock access to the xfrs list */
> +	struct mutex occ_lock;		/* lock access to the hardware */
>  	struct work_struct work;
> +	bool cancel;

Should this be atomic?

>  };
>  
>  #define to_occ(x)	container_of((x), struct occ, mdev)
>  
> -struct occ_command {
> -	u8 seq_no;
> -	u8 cmd_type;
> -	u16 data_length;
> -	u8 data[OCC_CMD_DATA_BYTES];
> -	u16 checksum;
> -} __packed;
> -
>  struct occ_response {
>  	u8 seq_no;
>  	u8 cmd_type;
>  	u8 return_status;
> -	u16 data_length;
> +	__be16 data_length;
>  	u8 data[OCC_RESP_DATA_BYTES];
> -	u16 checksum;
> +	__be16 checksum;
>  } __packed;
>  
>  /*
>   * transfer flags are NOT mutually exclusive
> - * 
> + *
>   * Initial flags are none; transfer is created and queued from write(). All
> - * 	flags are cleared when the transfer is completed by closing the file or
> - * 	reading all of the available response data.
> + *  flags are cleared when the transfer is completed by closing the file or
> + *  reading all of the available response data.
>   * XFR_IN_PROGRESS is set when a transfer is started from occ_worker_putsram,
> - * 	and cleared if the transfer fails or occ_worker_getsram completes.
> + *  and cleared if the transfer fails or occ_worker_getsram completes.
>   * XFR_COMPLETE is set when a transfer fails or finishes occ_worker_getsram.
>   * XFR_CANCELED is set when the transfer's client is released.
>   * XFR_WAITING is set from read() if the transfer isn't complete and
> - * 	NONBLOCKING wasn't specified. Cleared in read() when transfer completes
> - * 	or fails.
> + *  O_NONBLOCK wasn't specified. Cleared in read() when transfer completes or
> + *  fails.
>   */
>  enum {
>  	XFR_IN_PROGRESS,
> @@ -92,9 +94,9 @@ struct occ_xfr {
>   * client flags
>   *
>   * CLIENT_NONBLOCKING is set during open() if the file was opened with the
> - * 	O_NONBLOCKING flag.
> + *  O_NONBLOCK flag.
>   * CLIENT_XFR_PENDING is set during write() and cleared when all data has been
> - * 	read.
> + *  read.
>   */
>  enum {
>  	CLIENT_NONBLOCKING,
> @@ -104,7 +106,7 @@ enum {
>  struct occ_client {
>  	struct occ *occ;
>  	struct occ_xfr xfr;
> -	spinlock_t lock;
> +	spinlock_t lock;		/* lock access to the client state */
>  	wait_queue_head_t wait;
>  	size_t read_offset;
>  	unsigned long flags;
> @@ -116,12 +118,15 @@ struct occ_client {
>  
>  static DEFINE_IDA(occ_ida);
>  
> -static void occ_enqueue_xfr(struct occ_xfr *xfr)
> +static int occ_enqueue_xfr(struct occ_xfr *xfr)
>  {
>  	int empty;
>  	struct occ_client *client = to_client(xfr);
>  	struct occ *occ = client->occ;
>  
> +	if (occ->cancel)
> +		return -ECANCELED;
> +
>  	spin_lock_irq(&occ->list_lock);
>  
>  	empty = list_empty(&occ->xfrs);
> @@ -131,14 +136,15 @@ static void occ_enqueue_xfr(struct occ_xfr *xfr)
>  
>  	if (empty)
>  		queue_work(occ_wq, &occ->work);
> +
> +	return 0;
>  }
>  
>  static struct occ_client *occ_open_common(struct occ *occ, unsigned long flags)
>  {
> -	struct occ_client *client;
> +	struct occ_client *client = kzalloc(sizeof(*client), GFP_KERNEL);
>  
> -	client = kzalloc(sizeof(*client), GFP_KERNEL);
> -	if (!client)
> +	if (!client || occ->cancel)
>  		return NULL;
>  
>  	client->occ = occ;
> @@ -172,6 +178,7 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
>  	int rc;
>  	size_t bytes;
>  	struct occ_xfr *xfr = &client->xfr;
> +	struct occ *occ = client->occ;
>  
>  	if (len > OCC_SRAM_BYTES)
>  		return -EINVAL;
> @@ -183,8 +190,9 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
>  		if (client->read_offset) {
>  			rc = 0;
>  			client->read_offset = 0;
> -		} else
> +		} else {
>  			rc = -ENOMSG;
> +		}
>  
>  		goto done;
>  	}
> @@ -200,8 +208,11 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
>  		spin_unlock_irq(&client->lock);
>  
>  		rc = wait_event_interruptible(client->wait,
> -			test_bit(XFR_COMPLETE, &xfr->flags) ||
> -			test_bit(XFR_CANCELED, &xfr->flags));
> +					      test_bit(XFR_COMPLETE,
> +						       &xfr->flags) ||
> +					      test_bit(XFR_CANCELED,
> +						       &xfr->flags) ||
> +					      occ->cancel);
>  
>  		spin_lock_irq(&client->lock);
>  
> @@ -225,12 +236,14 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
>  
>  	bytes = min(len, xfr->resp_data_length - client->read_offset);
>  	if (ubuf) {
> -		if (copy_to_user(ubuf, &xfr->buf[client->read_offset], bytes)) {
> +		if (copy_to_user(ubuf, &xfr->buf[client->read_offset],
> +				 bytes)) {
>  			rc = -EFAULT;
>  			goto done;
>  		}
> -	} else
> +	} else {
>  		memcpy(kbuf, &xfr->buf[client->read_offset], bytes);
> +	}
>  
>  	client->read_offset += bytes;
>  
> @@ -250,12 +263,6 @@ static ssize_t occ_read(struct file *file, char __user *buf, size_t len,
>  {
>  	struct occ_client *client = file->private_data;
>  
> -	/* check this ahead of time so we don't go changing the xfr state
> -	 * needlessly
> -	 */
> -	if (!access_ok(VERIFY_WRITE, buf, len))
> -		return -EFAULT;
> -
>  	return occ_read_common(client, buf, NULL, len);
>  }
>  
> @@ -272,28 +279,31 @@ static ssize_t occ_write_common(struct occ_client *client,
>  		return -EINVAL;
>  
>  	spin_lock_irq(&client->lock);
> -	if (test_and_set_bit(CLIENT_XFR_PENDING, &client->flags)) {
> +
> +	if (test_bit(CLIENT_XFR_PENDING, &client->flags)) {
>  		rc = -EBUSY;
>  		goto done;
>  	}
>  
> -	/* clear out the transfer */
> -	memset(xfr, 0, sizeof(*xfr));
> -
> -	xfr->buf[0] = 1;
> +	memset(xfr, 0, sizeof(*xfr));	/* clear out the transfer */

What about the transfer buffer?

> +	xfr->buf[0] = 1;		/* occ sequence number */
>  
> +	/* Assume user data follows the occ command format.
> +	 * byte 0: command type
> +	 * bytes 1-2: data length (msb first)
> +	 * bytes 3-n: data
> +	 */
>  	if (ubuf) {
>  		if (copy_from_user(&xfr->buf[1], ubuf, len)) {
> -			kfree(xfr);
>  			rc = -EFAULT;
>  			goto done;
>  		}
> -	} else
> +	} else {
>  		memcpy(&xfr->buf[1], kbuf, len);
> +	}
>  
>  	data_length = (xfr->buf[2] << 8) + xfr->buf[3];

Shouldn't data_length be proportional to len? Like, somewhere in the
neighbourhood of len - 3? Should we check that? What if I just send down 3
bytes, some kind of command and two others representing the value 4090? Won't
we submit a bunch of "junk" (overlapping, previously submitted data) to the
FIFO?

If the client is shared between higher-level command submitters then you could
potentially resubmit the previous command.

>  	if (data_length > OCC_CMD_DATA_BYTES) {
> -		kfree(xfr);
>  		rc = -EINVAL;
>  		goto done;
>  	}
> @@ -306,9 +316,11 @@ static ssize_t occ_write_common(struct occ_client *client,
>  
>  	xfr->cmd_data_length = data_length + 6;
>  	client->read_offset = 0;
> +	rc = occ_enqueue_xfr(xfr);
> +	if (rc)
> +		goto done;
>  
> -	occ_enqueue_xfr(xfr);
> -
> +	set_bit(CLIENT_XFR_PENDING, &client->flags);
>  	rc = len;
>  
>  done:
> @@ -321,12 +333,6 @@ static ssize_t occ_write(struct file *file, const char __user *buf,
>  {
>  	struct occ_client *client = file->private_data;
>  
> -	/* check this ahead of time so we don't go changing the xfr state
> -	 * needlessly
> -	 */
> -	if (!access_ok(VERIFY_READ, buf, len))
> -		return -EFAULT;
> -
>  	return occ_write_common(client, buf, NULL, len);
>  }
>  
> @@ -366,7 +372,7 @@ static int occ_release_common(struct occ_client *client)
>  		return 0;
>  	}
>  
> -	/* operation is in progress; let worker clean up*/
> +	/* operation is in progress; let worker clean up */
>  	spin_unlock_irq(&occ->list_lock);
>  	spin_unlock_irq(&client->lock);
>  	return 0;
> @@ -403,7 +409,7 @@ static int occ_write_sbefifo(struct sbefifo_client *client, const char *buf,
>  		total += rc;
>  	} while (total < len);
>  
> -	return (total == len) ? 0 : -EMSGSIZE;
> +	return (total == len) ? 0 : -ENODATA;

This doesn't seem right for a write, maybe -ENOSPC?
>  }
>  
>  static int occ_read_sbefifo(struct sbefifo_client *client, char *buf,
> @@ -422,7 +428,7 @@ static int occ_read_sbefifo(struct sbefifo_client *client, char *buf,
>  		total += rc;
>  	} while (total < len);
>  
> -	return (total == len) ? 0 : -EMSGSIZE;
> +	return (total == len) ? 0 : -ENODATA;
>  }
>  
>  static int occ_getsram(struct device *sbefifo, u32 address, u8 *data,
> @@ -430,10 +436,13 @@ static int occ_getsram(struct device *sbefifo, u32 address, u8 *data,
>  {
>  	int rc;
>  	u8 *resp;
> -	u32 buf[5];
> -	u32 data_len = ((len + 7) / 8) * 8;
> +	__be32 buf[5];
> +	u32 data_len = ((len + 7) / 8) * 8;	/* must be multiples of 8 B */
>  	struct sbefifo_client *client;
>  
> +	/* Magic sequence to do SBE getsram command. SBE will fetch data from
> +	 * specified SRAM address.
> +	 */

Do the individual values of the sequence actually mean anything? Or are they
truly magic? If possible it would be good to indicate what each does rather
than just make some vague claim about them.

>  	buf[0] = cpu_to_be32(0x5);
>  	buf[1] = cpu_to_be32(0xa403);
>  	buf[2] = cpu_to_be32(1);
> @@ -447,7 +456,7 @@ static int occ_getsram(struct device *sbefifo, u32 address, u8 *data,
>  	rc = occ_write_sbefifo(client, (const char *)buf, sizeof(buf));
>  	if (rc)
>  		goto done;
> -	
> +
>  	resp = kzalloc(data_len, GFP_KERNEL);
>  	if (!resp) {
>  		rc = -ENOMEM;
> @@ -467,7 +476,7 @@ static int occ_getsram(struct device *sbefifo, u32 address, u8 *data,
>  	    (be32_to_cpu(buf[1]) == 0xC0DEA403))
>  		memcpy(data, resp, len);
>  	else
> -		rc = -EFAULT;
> +		rc = -EBADMSG;
>  
>  free:
>  	kfree(resp);
> @@ -481,8 +490,8 @@ static int occ_putsram(struct device *sbefifo, u32 address, u8 *data,
>  		       ssize_t len)
>  {
>  	int rc;
> -	u32 *buf;
> -	u32 data_len = ((len + 7) / 8) * 8;
> +	__be32 *buf;
> +	u32 data_len = ((len + 7) / 8) * 8;	/* must be multiples of 8 B */
>  	size_t cmd_len = data_len + 20;
>  	struct sbefifo_client *client;
>  
> @@ -490,6 +499,9 @@ static int occ_putsram(struct device *sbefifo, u32 address, u8 *data,
>  	if (!buf)
>  		return -ENOMEM;
>  
> +	/* Magic sequence to do SBE putsram command. SBE will transfer
> +	 * data to specified SRAM address.
> +	 */

See above.

>  	buf[0] = cpu_to_be32(0x5 + (data_len / 4));
>  	buf[1] = cpu_to_be32(0xa404);
>  	buf[2] = cpu_to_be32(1);
> @@ -515,7 +527,7 @@ static int occ_putsram(struct device *sbefifo, u32 address, u8 *data,
>  	/* check for good response */
>  	if ((be32_to_cpu(buf[0]) != data_len) ||
>  	    (be32_to_cpu(buf[1]) != 0xC0DEA404))
> -		rc = -EFAULT;
> +		rc = -EBADMSG;
>  
>  done:
>  	sbefifo_drv_release(client);
> @@ -527,14 +539,17 @@ static int occ_putsram(struct device *sbefifo, u32 address, u8 *data,
>  static int occ_trigger_attn(struct device *sbefifo)
>  {
>  	int rc;
> -	u32 buf[6];
> +	__be32 buf[6];
>  	struct sbefifo_client *client;
>  
> +	/* Magic sequence to do SBE putscom command. SBE will write 8 bytes to
> +	 * specified SCOM address.
> +	 */

See above.

>  	buf[0] = cpu_to_be32(0x6);
>  	buf[1] = cpu_to_be32(0xa202);
>  	buf[2] = 0;
>  	buf[3] = cpu_to_be32(0x6D035);
> -	buf[4] = cpu_to_be32(0x20010000);
> +	buf[4] = cpu_to_be32(0x20010000);	/* trigger occ attention */

This kind of comment is what I'm interested in.

>  	buf[5] = 0;
>  
>  	client = sbefifo_drv_open(sbefifo, 0);
> @@ -552,7 +567,7 @@ static int occ_trigger_attn(struct device *sbefifo)
>  	/* check for good response */
>  	if ((be32_to_cpu(buf[0]) != 0xC0DEA202) ||
>  	    (be32_to_cpu(buf[1]) & 0x0FFFFFFF))
> -		rc = -EFAULT;
> +		rc = -EBADMSG;
>  
>  done:
>  	sbefifo_drv_release(client);
> @@ -564,12 +579,19 @@ static void occ_worker(struct work_struct *work)
>  {
>  	int rc = 0, empty, waiting, canceled;
>  	u16 resp_data_length;
> +	unsigned long start;
> +	const unsigned long timeout = msecs_to_jiffies(OCC_TIMEOUT_MS);
> +	const long int wait_time = msecs_to_jiffies(OCC_CMD_IN_PRG_WAIT_MS);
>  	struct occ_xfr *xfr;
> +	struct occ_response *resp;
>  	struct occ_client *client;
>  	struct occ *occ = container_of(work, struct occ, work);
>  	struct device *sbefifo = occ->sbefifo;
>  
>  again:
> +	if (occ->cancel)
> +		return;
> +
>  	spin_lock_irq(&occ->list_lock);
>  
>  	xfr = list_first_entry_or_null(&occ->xfrs, struct occ_xfr, link);
> @@ -578,11 +600,14 @@ static void occ_worker(struct work_struct *work)
>  		return;
>  	}
>  
> +	resp = (struct occ_response *)xfr->buf;

Are the members all aligned as necessary?

>  	set_bit(XFR_IN_PROGRESS, &xfr->flags);
>  
>  	spin_unlock_irq(&occ->list_lock);
>  	mutex_lock(&occ->occ_lock);
>  
> +	/* write occ command */
> +	start = jiffies;
>  	rc = occ_putsram(sbefifo, 0xFFFBE000, xfr->buf,
>  			 xfr->cmd_data_length);
>  	if (rc)
> @@ -592,13 +617,26 @@ static void occ_worker(struct work_struct *work)
>  	if (rc)
>  		goto done;
>  
> -	rc = occ_getsram(sbefifo, 0xFFFBF000, xfr->buf, 8);
> -	if (rc)
> -		goto done;
> +	/* read occ response */
> +	do {
> +		rc = occ_getsram(sbefifo, 0xFFFBF000, xfr->buf, 8);
> +		if (rc)
> +			goto done;
> +
> +		if (resp->return_status == OCC_RESP_CMD_IN_PRG) {
> +			rc = -EALREADY;
> +
> +			if (time_after(jiffies, start + timeout))
> +				break;
>  
> -	resp_data_length = (xfr->buf[3] << 8) + xfr->buf[4];
> +			set_current_state(TASK_INTERRUPTIBLE);
> +			schedule_timeout(wait_time);
> +		}
> +	} while (rc);
> +
> +	resp_data_length = get_unaligned_be16(&resp->data_length);
>  	if (resp_data_length > OCC_RESP_DATA_BYTES) {
> -		rc = -EDOM;
> +		rc = -EMSGSIZE;
>  		goto done;
>  	}
>  
> @@ -704,9 +742,6 @@ static int occ_probe(struct platform_device *pdev)
>  	mutex_init(&occ->occ_lock);
>  	INIT_WORK(&occ->work, occ_worker);
>  
> -	/* ensure NULL before we probe children, so they don't hang FSI */
> -	platform_set_drvdata(pdev, NULL);
> -
>  	if (dev->of_node) {
>  		rc = of_property_read_u32(dev->of_node, "reg", &reg);
>  		if (!rc) {
> @@ -716,23 +751,13 @@ static int occ_probe(struct platform_device *pdev)
>  			if (occ->idx < 0)
>  				occ->idx = ida_simple_get(&occ_ida, 1, INT_MAX,
>  							  GFP_KERNEL);
> -		} else
> +		} else {
>  			occ->idx = ida_simple_get(&occ_ida, 1, INT_MAX,
>  						  GFP_KERNEL);
> -
> -		/* create platform devs for dts child nodes (hwmon, etc) */
> -		for_each_child_of_node(dev->of_node, np) {
> -			snprintf(child_name, sizeof(child_name), "occ%d-dev%d",
> -				 occ->idx, child_idx++);
> -			child = of_platform_device_create(np, child_name, dev);
> -			if (!child)
> -				dev_warn(dev,
> -					 "failed to create child node dev\n");
>  		}
> -	} else
> +	} else {
>  		occ->idx = ida_simple_get(&occ_ida, 1, INT_MAX, GFP_KERNEL);
> -
> -	platform_set_drvdata(pdev, occ);
> +	}
>  
>  	snprintf(occ->name, sizeof(occ->name), "occ%d", occ->idx);
>  	occ->mdev.fops = &occ_fops;
> @@ -742,20 +767,42 @@ static int occ_probe(struct platform_device *pdev)
>  
>  	rc = misc_register(&occ->mdev);
>  	if (rc) {
> -		dev_err(dev, "failed to register miscdevice\n");
> +		dev_err(dev, "failed to register miscdevice: %d\n", rc);
> +		ida_simple_remove(&occ_ida, occ->idx);
>  		return rc;
>  	}
>  
> +	/* create platform devs for dts child nodes (hwmon, etc) */
> +	for_each_available_child_of_node(dev->of_node, np) {
> +		snprintf(child_name, sizeof(child_name), "occ%d-dev%d",
> +			 occ->idx, child_idx++);
> +		child = of_platform_device_create(np, child_name, dev);
> +		if (!child)
> +			dev_warn(dev, "failed to create child node dev\n");
> +	}
> +
> +	platform_set_drvdata(pdev, occ);
> +

Similar to the SBEFIFO, it would be nice to separate out the miscdev from the
core.

>  	return 0;
>  }
>  
>  static int occ_remove(struct platform_device *pdev)
>  {
>  	struct occ *occ = platform_get_drvdata(pdev);
> +	struct occ_xfr *xfr;
> +	struct occ_client *client;
> +
> +	occ->cancel = true;
> +	list_for_each_entry(xfr, &occ->xfrs, link) {
> +		client = to_client(xfr);
> +		wake_up_interruptible(&client->wait);
> +	}
>  
> -	flush_work(&occ->work);
>  	misc_deregister(&occ->mdev);
>  	device_for_each_child(&pdev->dev, NULL, occ_unregister_child);
> +
> +	flush_work(&occ->work);

This should be okay now that we have the -ENODEV in the SBEFIFO, right (if
that's all nice and race-free)? That is, sbefifo_read_ready() and
sbefifo_write_ready() should trigger and the associated
sbefifo_{read,write}_common() pulls -ENODEV out of sbefifo->rc?

> +
>  	ida_simple_remove(&occ_ida, occ->idx);
>  
>  	return 0;
> @@ -789,6 +836,8 @@ static void occ_exit(void)
>  	destroy_workqueue(occ_wq);
>  
>  	platform_driver_unregister(&occ_driver);
> +
> +	ida_destroy(&occ_ida);
>  }
>  
>  module_init(occ_init);
> diff --git a/include/linux/occ.h b/include/linux/occ.h
> index d78332c..0a4a54a 100644
> --- a/include/linux/occ.h
> +++ b/include/linux/occ.h
> @@ -11,12 +11,26 @@
>   * GNU General Public License for more details.
>   */
>  
> -#ifndef __OCC_H__
> -#define __OCC_H__
> +#ifndef LINUX_FSI_OCC_H
> +#define LINUX_FSI_OCC_H
>  
>  struct device;
>  struct occ_client;
>  
> +#define OCC_RESP_CMD_IN_PRG		0xFF
> +#define OCC_RESP_SUCCESS		0
> +#define OCC_RESP_CMD_INVAL		0x11
> +#define OCC_RESP_CMD_LEN_INVAL		0x12
> +#define OCC_RESP_DATA_INVAL		0x13
> +#define OCC_RESP_CHKSUM_ERR		0x14
> +#define OCC_RESP_INT_ERR		0x15
> +#define OCC_RESP_BAD_STATE		0x16
> +#define OCC_RESP_CRIT_EXCEPT		0xE0
> +#define OCC_RESP_CRIT_INIT		0xE1
> +#define OCC_RESP_CRIT_WATCHDOG		0xE2
> +#define OCC_RESP_CRIT_OCB		0xE3
> +#define OCC_RESP_CRIT_HW		0xE4
> +
>  extern struct occ_client *occ_drv_open(struct device *dev,
>  				       unsigned long flags);
>  extern int occ_drv_read(struct occ_client *client, char *buf, size_t len);
> @@ -24,4 +38,4 @@ extern int occ_drv_write(struct occ_client *client, const char *buf,
>  			 size_t len);
>  extern void occ_drv_release(struct occ_client *client);
>  
> -#endif /* __OCC_H__ */
> +#endif /* LINUX_FSI_OCC_H */
Eddie James Sept. 29, 2017, 10:41 p.m. | #3
On 09/25/2017 01:22 AM, Andrew Jeffery wrote:
> On Thu, 2017-09-21 at 17:43 -0500, Eddie James wrote:
>> From: "Edward A. James" <eajames@us.ibm.com>
>>   
>> Includes various fixes:
>>   - Fix probe failure scenario
>>   - General cleanup
>>   - Reorder remove() operations for safety
>>   - Add cancel boolean to prevent further xfrs when we're removing
> See comment on previous patch about these bullet points.
>
>>   
>> Signed-off-by: Edward A. James <eajames@us.ibm.com>
>> ---
>>   drivers/fsi/occ.c   | 227 ++++++++++++++++++++++++++++++++--------------------
>>   include/linux/occ.h |  20 ++++-
>>   2 files changed, 155 insertions(+), 92 deletions(-)
>>   
>> diff --git a/drivers/fsi/occ.c b/drivers/fsi/occ.c
>> index 621fbf0..4dd5048 100644
>> --- a/drivers/fsi/occ.c
>> +++ b/drivers/fsi/occ.c
>> @@ -10,16 +10,22 @@
>>   #include <asm/unaligned.h>
>>   #include <linux/device.h>
>>   #include <linux/err.h>
>> +#include <linux/errno.h>
>> +#include <linux/fs.h>
>>   #include <linux/fsi-sbefifo.h>
>> -#include <linux/init.h>
>> +#include <linux/idr.h>
>>   #include <linux/kernel.h>
>> +#include <linux/list.h>
>>   #include <linux/miscdevice.h>
>>   #include <linux/module.h>
>>   #include <linux/mutex.h>
>> +#include <linux/occ.h>
>>   #include <linux/of.h>
>>   #include <linux/of_platform.h>
>>   #include <linux/platform_device.h>
>> +#include <linux/sched.h>
>>   #include <linux/slab.h>
>> +#include <linux/spinlock.h>
>>   #include <linux/uaccess.h>
>>   #include <linux/wait.h>
>>   #include <linux/workqueue.h>
>> @@ -28,49 +34,45 @@
>>   #define OCC_CMD_DATA_BYTES	4090
>>   #define OCC_RESP_DATA_BYTES	4089
>>   
>> +#define OCC_TIMEOUT_MS		1000
>> +#define OCC_CMD_IN_PRG_WAIT_MS	50
>> +
>>   struct occ {
>>   	struct device *sbefifo;
>>   	char name[32];
>>   	int idx;
>>   	struct miscdevice mdev;
>>   	struct list_head xfrs;
>> -	spinlock_t list_lock;
>> -	struct mutex occ_lock;
>> +	spinlock_t list_lock;		/* lock access to the xfrs list */
>> +	struct mutex occ_lock;		/* lock access to the hardware */
>>   	struct work_struct work;
>> +	bool cancel;
> Should this be atomic?

Shouldn't really matter. Only remove() is writing the value.

>
>>   };
>>   
>>   #define to_occ(x)	container_of((x), struct occ, mdev)
>>   
>> -struct occ_command {
>> -	u8 seq_no;
>> -	u8 cmd_type;
>> -	u16 data_length;
>> -	u8 data[OCC_CMD_DATA_BYTES];
>> -	u16 checksum;
>> -} __packed;
>> -
>>   struct occ_response {
>>   	u8 seq_no;
>>   	u8 cmd_type;
>>   	u8 return_status;
>> -	u16 data_length;
>> +	__be16 data_length;
>>   	u8 data[OCC_RESP_DATA_BYTES];
>> -	u16 checksum;
>> +	__be16 checksum;
>>   } __packed;
>>   
>>   /*
>>    * transfer flags are NOT mutually exclusive
>> - *
>> + *
>>    * Initial flags are none; transfer is created and queued from write(). All
>> - * 	flags are cleared when the transfer is completed by closing the file or
>> - * 	reading all of the available response data.
>> + *  flags are cleared when the transfer is completed by closing the file or
>> + *  reading all of the available response data.
>>    * XFR_IN_PROGRESS is set when a transfer is started from occ_worker_putsram,
>> - * 	and cleared if the transfer fails or occ_worker_getsram completes.
>> + *  and cleared if the transfer fails or occ_worker_getsram completes.
>>    * XFR_COMPLETE is set when a transfer fails or finishes occ_worker_getsram.
>>    * XFR_CANCELED is set when the transfer's client is released.
>>    * XFR_WAITING is set from read() if the transfer isn't complete and
>> - * 	NONBLOCKING wasn't specified. Cleared in read() when transfer completes
>> - * 	or fails.
>> + *  O_NONBLOCK wasn't specified. Cleared in read() when transfer completes or
>> + *  fails.
>>    */
>>   enum {
>>   	XFR_IN_PROGRESS,
>> @@ -92,9 +94,9 @@ struct occ_xfr {
>>    * client flags
>>    *
>>    * CLIENT_NONBLOCKING is set during open() if the file was opened with the
>> - * 	O_NONBLOCKING flag.
>> + *  O_NONBLOCK flag.
>>    * CLIENT_XFR_PENDING is set during write() and cleared when all data has been
>> - * 	read.
>> + *  read.
>>    */
>>   enum {
>>   	CLIENT_NONBLOCKING,
>> @@ -104,7 +106,7 @@ enum {
>>   struct occ_client {
>>   	struct occ *occ;
>>   	struct occ_xfr xfr;
>> -	spinlock_t lock;
>> +	spinlock_t lock;		/* lock access to the client state */
>>   	wait_queue_head_t wait;
>>   	size_t read_offset;
>>   	unsigned long flags;
>> @@ -116,12 +118,15 @@ struct occ_client {
>>   
>>   static DEFINE_IDA(occ_ida);
>>   
>> -static void occ_enqueue_xfr(struct occ_xfr *xfr)
>> +static int occ_enqueue_xfr(struct occ_xfr *xfr)
>>   {
>>   	int empty;
>>   	struct occ_client *client = to_client(xfr);
>>   	struct occ *occ = client->occ;
>>   
>> +	if (occ->cancel)
>> +		return -ECANCELED;
>> +
>>   	spin_lock_irq(&occ->list_lock);
>>   
>>   	empty = list_empty(&occ->xfrs);
>> @@ -131,14 +136,15 @@ static void occ_enqueue_xfr(struct occ_xfr *xfr)
>>   
>>   	if (empty)
>>   		queue_work(occ_wq, &occ->work);
>> +
>> +	return 0;
>>   }
>>   
>>   static struct occ_client *occ_open_common(struct occ *occ, unsigned long flags)
>>   {
>> -	struct occ_client *client;
>> +	struct occ_client *client = kzalloc(sizeof(*client), GFP_KERNEL);
>>   
>> -	client = kzalloc(sizeof(*client), GFP_KERNEL);
>> -	if (!client)
>> +	if (!client || occ->cancel)
>>   		return NULL;
>>   
>>   	client->occ = occ;
>> @@ -172,6 +178,7 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
>>   	int rc;
>>   	size_t bytes;
>>   	struct occ_xfr *xfr = &client->xfr;
>> +	struct occ *occ = client->occ;
>>   
>>   	if (len > OCC_SRAM_BYTES)
>>   		return -EINVAL;
>> @@ -183,8 +190,9 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
>>   		if (client->read_offset) {
>>   			rc = 0;
>>   			client->read_offset = 0;
>> -		} else
>> +		} else {
>>   			rc = -ENOMSG;
>> +		}
>>   
>>   		goto done;
>>   	}
>> @@ -200,8 +208,11 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
>>   		spin_unlock_irq(&client->lock);
>>   
>>   		rc = wait_event_interruptible(client->wait,
>> -			test_bit(XFR_COMPLETE, &xfr->flags) ||
>> -			test_bit(XFR_CANCELED, &xfr->flags));
>> +					      test_bit(XFR_COMPLETE,
>> +						       &xfr->flags) ||
>> +					      test_bit(XFR_CANCELED,
>> +						       &xfr->flags) ||
>> +					      occ->cancel);
>>   
>>   		spin_lock_irq(&client->lock);
>>   
>> @@ -225,12 +236,14 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
>>   
>>   	bytes = min(len, xfr->resp_data_length - client->read_offset);
>>   	if (ubuf) {
>> -		if (copy_to_user(ubuf, &xfr->buf[client->read_offset], bytes)) {
>> +		if (copy_to_user(ubuf, &xfr->buf[client->read_offset],
>> +				 bytes)) {
>>   			rc = -EFAULT;
>>   			goto done;
>>   		}
>> -	} else
>> +	} else {
>>   		memcpy(kbuf, &xfr->buf[client->read_offset], bytes);
>> +	}
>>   
>>   	client->read_offset += bytes;
>>   
>> @@ -250,12 +263,6 @@ static ssize_t occ_read(struct file *file, char __user *buf, size_t len,
>>   {
>>   	struct occ_client *client = file->private_data;
>>   
>> -	/* check this ahead of time so we don't go changing the xfr state
>> -	 * needlessly
>> -	 */
>> -	if (!access_ok(VERIFY_WRITE, buf, len))
>> -		return -EFAULT;
>> -
>>   	return occ_read_common(client, buf, NULL, len);
>>   }
>>   
>> @@ -272,28 +279,31 @@ static ssize_t occ_write_common(struct occ_client *client,
>>   		return -EINVAL;
>>   
>>   	spin_lock_irq(&client->lock);
>> -	if (test_and_set_bit(CLIENT_XFR_PENDING, &client->flags)) {
>> +
>> +	if (test_bit(CLIENT_XFR_PENDING, &client->flags)) {
>>   		rc = -EBUSY;
>>   		goto done;
>>   	}
>>   
>> -	/* clear out the transfer */
>> -	memset(xfr, 0, sizeof(*xfr));
>> -
>> -	xfr->buf[0] = 1;
>> +	memset(xfr, 0, sizeof(*xfr));	/* clear out the transfer */
> What about the transfer buffer?

The buffer is a part of the transfer structure.

>
>> +	xfr->buf[0] = 1;		/* occ sequence number */
>>   
>> +	/* Assume user data follows the occ command format.
>> +	 * byte 0: command type
>> +	 * bytes 1-2: data length (msb first)
>> +	 * bytes 3-n: data
>> +	 */
>>   	if (ubuf) {
>>   		if (copy_from_user(&xfr->buf[1], ubuf, len)) {
>> -			kfree(xfr);
>>   			rc = -EFAULT;
>>   			goto done;
>>   		}
>> -	} else
>> +	} else {
>>   		memcpy(&xfr->buf[1], kbuf, len);
>> +	}
>>   
>>   	data_length = (xfr->buf[2] << 8) + xfr->buf[3];
> Shouldn't data_length be proportional to len? Like, somewhere in the
> neighbourhood of len - 3? Should we check that? What if I just send down 3
> bytes, some kind of command and two others representing the value 4090? Won't
> we submit a bunch of "junk" (overlapping, previously submitted data) to the
> FIFO?

Yes, though it will be 0s, since we cleared it above. But that's the 
user's problem. Either way you do it, the user has to line up their data 
right. If we send something based on len, if they don't have their data 
length values correct, the command won't be interpreted correctly 
anyway. And furthermore, if you use len, you don't know if they include 
the checksum or not, which varies between clients at the moment, though 
you could ask people to change.

>
> If the client is shared between higher-level command submitters then you could
> potentially resubmit the previous command.

The transfer is cleared out.

>
>>   	if (data_length > OCC_CMD_DATA_BYTES) {
>> -		kfree(xfr);
>>   		rc = -EINVAL;
>>   		goto done;
>>   	}
>> @@ -306,9 +316,11 @@ static ssize_t occ_write_common(struct occ_client *client,
>>   
>>   	xfr->cmd_data_length = data_length + 6;
>>   	client->read_offset = 0;
>> +	rc = occ_enqueue_xfr(xfr);
>> +	if (rc)
>> +		goto done;
>>   
>> -	occ_enqueue_xfr(xfr);
>> -
>> +	set_bit(CLIENT_XFR_PENDING, &client->flags);
>>   	rc = len;
>>   
>>   done:
>> @@ -321,12 +333,6 @@ static ssize_t occ_write(struct file *file, const char __user *buf,
>>   {
>>   	struct occ_client *client = file->private_data;
>>   
>> -	/* check this ahead of time so we don't go changing the xfr state
>> -	 * needlessly
>> -	 */
>> -	if (!access_ok(VERIFY_READ, buf, len))
>> -		return -EFAULT;
>> -
>>   	return occ_write_common(client, buf, NULL, len);
>>   }
>>   
>> @@ -366,7 +372,7 @@ static int occ_release_common(struct occ_client *client)
>>   		return 0;
>>   	}
>>   
>> -	/* operation is in progress; let worker clean up*/
>> +	/* operation is in progress; let worker clean up */
>>   	spin_unlock_irq(&occ->list_lock);
>>   	spin_unlock_irq(&client->lock);
>>   	return 0;
>> @@ -403,7 +409,7 @@ static int occ_write_sbefifo(struct sbefifo_client *client, const char *buf,
>>   		total += rc;
>>   	} while (total < len);
>>   
>> -	return (total == len) ? 0 : -EMSGSIZE;
>> +	return (total == len) ? 0 : -ENODATA;
> This doesn't seem right for a write, maybe -ENOSPC?
>>   }
>>   
>>   static int occ_read_sbefifo(struct sbefifo_client *client, char *buf,
>> @@ -422,7 +428,7 @@ static int occ_read_sbefifo(struct sbefifo_client *client, char *buf,
>>   		total += rc;
>>   	} while (total < len);
>>   
>> -	return (total == len) ? 0 : -EMSGSIZE;
>> +	return (total == len) ? 0 : -ENODATA;
>>   }
>>   
>>   static int occ_getsram(struct device *sbefifo, u32 address, u8 *data,
>> @@ -430,10 +436,13 @@ static int occ_getsram(struct device *sbefifo, u32 address, u8 *data,
>>   {
>>   	int rc;
>>   	u8 *resp;
>> -	u32 buf[5];
>> -	u32 data_len = ((len + 7) / 8) * 8;
>> +	__be32 buf[5];
>> +	u32 data_len = ((len + 7) / 8) * 8;	/* must be multiples of 8 B */
>>   	struct sbefifo_client *client;
>>   
>> +	/* Magic sequence to do SBE getsram command. SBE will fetch data from
>> +	 * specified SRAM address.
>> +	 */
> Do the individual values of the sequence actually mean anything? Or are they
> truly magic? If possible it would be good to indicate what each does rather
> than just make some vague claim about them.

They're mostly just magic SBE values.

>
>>   	buf[0] = cpu_to_be32(0x5);
>>   	buf[1] = cpu_to_be32(0xa403);
>>   	buf[2] = cpu_to_be32(1);
>> @@ -447,7 +456,7 @@ static int occ_getsram(struct device *sbefifo, u32 address, u8 *data,
>>   	rc = occ_write_sbefifo(client, (const char *)buf, sizeof(buf));
>>   	if (rc)
>>   		goto done;
>> -	
>> +
>>   	resp = kzalloc(data_len, GFP_KERNEL);
>>   	if (!resp) {
>>   		rc = -ENOMEM;
>> @@ -467,7 +476,7 @@ static int occ_getsram(struct device *sbefifo, u32 address, u8 *data,
>>   	    (be32_to_cpu(buf[1]) == 0xC0DEA403))
>>   		memcpy(data, resp, len);
>>   	else
>> -		rc = -EFAULT;
>> +		rc = -EBADMSG;
>>   
>>   free:
>>   	kfree(resp);
>> @@ -481,8 +490,8 @@ static int occ_putsram(struct device *sbefifo, u32 address, u8 *data,
>>   		       ssize_t len)
>>   {
>>   	int rc;
>> -	u32 *buf;
>> -	u32 data_len = ((len + 7) / 8) * 8;
>> +	__be32 *buf;
>> +	u32 data_len = ((len + 7) / 8) * 8;	/* must be multiples of 8 B */
>>   	size_t cmd_len = data_len + 20;
>>   	struct sbefifo_client *client;
>>   
>> @@ -490,6 +499,9 @@ static int occ_putsram(struct device *sbefifo, u32 address, u8 *data,
>>   	if (!buf)
>>   		return -ENOMEM;
>>   
>> +	/* Magic sequence to do SBE putsram command. SBE will transfer
>> +	 * data to specified SRAM address.
>> +	 */
> See above.
>
>>   	buf[0] = cpu_to_be32(0x5 + (data_len / 4));
>>   	buf[1] = cpu_to_be32(0xa404);
>>   	buf[2] = cpu_to_be32(1);
>> @@ -515,7 +527,7 @@ static int occ_putsram(struct device *sbefifo, u32 address, u8 *data,
>>   	/* check for good response */
>>   	if ((be32_to_cpu(buf[0]) != data_len) ||
>>   	    (be32_to_cpu(buf[1]) != 0xC0DEA404))
>> -		rc = -EFAULT;
>> +		rc = -EBADMSG;
>>   
>>   done:
>>   	sbefifo_drv_release(client);
>> @@ -527,14 +539,17 @@ static int occ_putsram(struct device *sbefifo, u32 address, u8 *data,
>>   static int occ_trigger_attn(struct device *sbefifo)
>>   {
>>   	int rc;
>> -	u32 buf[6];
>> +	__be32 buf[6];
>>   	struct sbefifo_client *client;
>>   
>> +	/* Magic sequence to do SBE putscom command. SBE will write 8 bytes to
>> +	 * specified SCOM address.
>> +	 */
> See above.
>
>>   	buf[0] = cpu_to_be32(0x6);
>>   	buf[1] = cpu_to_be32(0xa202);
>>   	buf[2] = 0;
>>   	buf[3] = cpu_to_be32(0x6D035);
>> -	buf[4] = cpu_to_be32(0x20010000);
>> +	buf[4] = cpu_to_be32(0x20010000);	/* trigger occ attention */
> This kind of comment is what I'm interested in.
>
>>   	buf[5] = 0;
>>   
>>   	client = sbefifo_drv_open(sbefifo, 0);
>> @@ -552,7 +567,7 @@ static int occ_trigger_attn(struct device *sbefifo)
>>   	/* check for good response */
>>   	if ((be32_to_cpu(buf[0]) != 0xC0DEA202) ||
>>   	    (be32_to_cpu(buf[1]) & 0x0FFFFFFF))
>> -		rc = -EFAULT;
>> +		rc = -EBADMSG;
>>   
>>   done:
>>   	sbefifo_drv_release(client);
>> @@ -564,12 +579,19 @@ static void occ_worker(struct work_struct *work)
>>   {
>>   	int rc = 0, empty, waiting, canceled;
>>   	u16 resp_data_length;
>> +	unsigned long start;
>> +	const unsigned long timeout = msecs_to_jiffies(OCC_TIMEOUT_MS);
>> +	const long int wait_time = msecs_to_jiffies(OCC_CMD_IN_PRG_WAIT_MS);
>>   	struct occ_xfr *xfr;
>> +	struct occ_response *resp;
>>   	struct occ_client *client;
>>   	struct occ *occ = container_of(work, struct occ, work);
>>   	struct device *sbefifo = occ->sbefifo;
>>   
>>   again:
>> +	if (occ->cancel)
>> +		return;
>> +
>>   	spin_lock_irq(&occ->list_lock);
>>   
>>   	xfr = list_first_entry_or_null(&occ->xfrs, struct occ_xfr, link);
>> @@ -578,11 +600,14 @@ static void occ_worker(struct work_struct *work)
>>   		return;
>>   	}
>>   
>> +	resp = (struct occ_response *)xfr->buf;
> Are the members all aligned as necessary?

Yes.

>
>>   	set_bit(XFR_IN_PROGRESS, &xfr->flags);
>>   
>>   	spin_unlock_irq(&occ->list_lock);
>>   	mutex_lock(&occ->occ_lock);
>>   
>> +	/* write occ command */
>> +	start = jiffies;
>>   	rc = occ_putsram(sbefifo, 0xFFFBE000, xfr->buf,
>>   			 xfr->cmd_data_length);
>>   	if (rc)
>> @@ -592,13 +617,26 @@ static void occ_worker(struct work_struct *work)
>>   	if (rc)
>>   		goto done;
>>   
>> -	rc = occ_getsram(sbefifo, 0xFFFBF000, xfr->buf, 8);
>> -	if (rc)
>> -		goto done;
>> +	/* read occ response */
>> +	do {
>> +		rc = occ_getsram(sbefifo, 0xFFFBF000, xfr->buf, 8);
>> +		if (rc)
>> +			goto done;
>> +
>> +		if (resp->return_status == OCC_RESP_CMD_IN_PRG) {
>> +			rc = -EALREADY;
>> +
>> +			if (time_after(jiffies, start + timeout))
>> +				break;
>>   
>> -	resp_data_length = (xfr->buf[3] << 8) + xfr->buf[4];
>> +			set_current_state(TASK_INTERRUPTIBLE);
>> +			schedule_timeout(wait_time);
>> +		}
>> +	} while (rc);
>> +
>> +	resp_data_length = get_unaligned_be16(&resp->data_length);
>>   	if (resp_data_length > OCC_RESP_DATA_BYTES) {
>> -		rc = -EDOM;
>> +		rc = -EMSGSIZE;
>>   		goto done;
>>   	}
>>   
>> @@ -704,9 +742,6 @@ static int occ_probe(struct platform_device *pdev)
>>   	mutex_init(&occ->occ_lock);
>>   	INIT_WORK(&occ->work, occ_worker);
>>   
>> -	/* ensure NULL before we probe children, so they don't hang FSI */
>> -	platform_set_drvdata(pdev, NULL);
>> -
>>   	if (dev->of_node) {
>>   		rc = of_property_read_u32(dev->of_node, "reg", &reg);
>>   		if (!rc) {
>> @@ -716,23 +751,13 @@ static int occ_probe(struct platform_device *pdev)
>>   			if (occ->idx < 0)
>>   				occ->idx = ida_simple_get(&occ_ida, 1, INT_MAX,
>>   							  GFP_KERNEL);
>> -		} else
>> +		} else {
>>   			occ->idx = ida_simple_get(&occ_ida, 1, INT_MAX,
>>   						  GFP_KERNEL);
>> -
>> -		/* create platform devs for dts child nodes (hwmon, etc) */
>> -		for_each_child_of_node(dev->of_node, np) {
>> -			snprintf(child_name, sizeof(child_name), "occ%d-dev%d",
>> -				 occ->idx, child_idx++);
>> -			child = of_platform_device_create(np, child_name, dev);
>> -			if (!child)
>> -				dev_warn(dev,
>> -					 "failed to create child node dev\n");
>>   		}
>> -	} else
>> +	} else {
>>   		occ->idx = ida_simple_get(&occ_ida, 1, INT_MAX, GFP_KERNEL);
>> -
>> -	platform_set_drvdata(pdev, occ);
>> +	}
>>   
>>   	snprintf(occ->name, sizeof(occ->name), "occ%d", occ->idx);
>>   	occ->mdev.fops = &occ_fops;
>> @@ -742,20 +767,42 @@ static int occ_probe(struct platform_device *pdev)
>>   
>>   	rc = misc_register(&occ->mdev);
>>   	if (rc) {
>> -		dev_err(dev, "failed to register miscdevice\n");
>> +		dev_err(dev, "failed to register miscdevice: %d\n", rc);
>> +		ida_simple_remove(&occ_ida, occ->idx);
>>   		return rc;
>>   	}
>>   
>> +	/* create platform devs for dts child nodes (hwmon, etc) */
>> +	for_each_available_child_of_node(dev->of_node, np) {
>> +		snprintf(child_name, sizeof(child_name), "occ%d-dev%d",
>> +			 occ->idx, child_idx++);
>> +		child = of_platform_device_create(np, child_name, dev);
>> +		if (!child)
>> +			dev_warn(dev, "failed to create child node dev\n");
>> +	}
>> +
>> +	platform_set_drvdata(pdev, occ);
>> +
> Similar to the SBEFIFO, it would be nice to separate out the miscdev from the
> core.

Thought about it... it gets complicated to avoid replicating a lot of code.

Thanks for the review!
Eddie

>
>>   	return 0;
>>   }
>>   
>>   static int occ_remove(struct platform_device *pdev)
>>   {
>>   	struct occ *occ = platform_get_drvdata(pdev);
>> +	struct occ_xfr *xfr;
>> +	struct occ_client *client;
>> +
>> +	occ->cancel = true;
>> +	list_for_each_entry(xfr, &occ->xfrs, link) {
>> +		client = to_client(xfr);
>> +		wake_up_interruptible(&client->wait);
>> +	}
>>   
>> -	flush_work(&occ->work);
>>   	misc_deregister(&occ->mdev);
>>   	device_for_each_child(&pdev->dev, NULL, occ_unregister_child);
>> +
>> +	flush_work(&occ->work);
> This should be okay now that we have the -ENODEV in the SBEFIFO, right (if
> that's all nice and race-free)? That is, sbefifo_read_ready() and
> sbefifo_write_ready() should trigger and the associated
> sbefifo_{read,write}_common() pulls -ENODEV out of sbefifo->rc?
>
>> +
>>   	ida_simple_remove(&occ_ida, occ->idx);
>>   
>>   	return 0;
>> @@ -789,6 +836,8 @@ static void occ_exit(void)
>>   	destroy_workqueue(occ_wq);
>>   
>>   	platform_driver_unregister(&occ_driver);
>> +
>> +	ida_destroy(&occ_ida);
>>   }
>>   
>>   module_init(occ_init);
>> diff --git a/include/linux/occ.h b/include/linux/occ.h
>> index d78332c..0a4a54a 100644
>> --- a/include/linux/occ.h
>> +++ b/include/linux/occ.h
>> @@ -11,12 +11,26 @@
>>    * GNU General Public License for more details.
>>    */
>>   
>> -#ifndef __OCC_H__
>> -#define __OCC_H__
>> +#ifndef LINUX_FSI_OCC_H
>> +#define LINUX_FSI_OCC_H
>>   
>>   struct device;
>>   struct occ_client;
>>   
>> +#define OCC_RESP_CMD_IN_PRG		0xFF
>> +#define OCC_RESP_SUCCESS		0
>> +#define OCC_RESP_CMD_INVAL		0x11
>> +#define OCC_RESP_CMD_LEN_INVAL		0x12
>> +#define OCC_RESP_DATA_INVAL		0x13
>> +#define OCC_RESP_CHKSUM_ERR		0x14
>> +#define OCC_RESP_INT_ERR		0x15
>> +#define OCC_RESP_BAD_STATE		0x16
>> +#define OCC_RESP_CRIT_EXCEPT		0xE0
>> +#define OCC_RESP_CRIT_INIT		0xE1
>> +#define OCC_RESP_CRIT_WATCHDOG		0xE2
>> +#define OCC_RESP_CRIT_OCB		0xE3
>> +#define OCC_RESP_CRIT_HW		0xE4
>> +
>>   extern struct occ_client *occ_drv_open(struct device *dev,
>>   				       unsigned long flags);
>>   extern int occ_drv_read(struct occ_client *client, char *buf, size_t len);
>> @@ -24,4 +38,4 @@ extern int occ_drv_write(struct occ_client *client, const char *buf,
>>   			 size_t len);
>>   extern void occ_drv_release(struct occ_client *client);
>>   
>> -#endif /* __OCC_H__ */
>> +#endif /* LINUX_FSI_OCC_H */

Patch

diff --git a/drivers/fsi/occ.c b/drivers/fsi/occ.c
index 621fbf0..4dd5048 100644
--- a/drivers/fsi/occ.c
+++ b/drivers/fsi/occ.c
@@ -10,16 +10,22 @@ 
 #include <asm/unaligned.h>
 #include <linux/device.h>
 #include <linux/err.h>
+#include <linux/errno.h>
+#include <linux/fs.h>
 #include <linux/fsi-sbefifo.h>
-#include <linux/init.h>
+#include <linux/idr.h>
 #include <linux/kernel.h>
+#include <linux/list.h>
 #include <linux/miscdevice.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
+#include <linux/occ.h>
 #include <linux/of.h>
 #include <linux/of_platform.h>
 #include <linux/platform_device.h>
+#include <linux/sched.h>
 #include <linux/slab.h>
+#include <linux/spinlock.h>
 #include <linux/uaccess.h>
 #include <linux/wait.h>
 #include <linux/workqueue.h>
@@ -28,49 +34,45 @@ 
 #define OCC_CMD_DATA_BYTES	4090
 #define OCC_RESP_DATA_BYTES	4089
 
+#define OCC_TIMEOUT_MS		1000
+#define OCC_CMD_IN_PRG_WAIT_MS	50
+
 struct occ {
 	struct device *sbefifo;
 	char name[32];
 	int idx;
 	struct miscdevice mdev;
 	struct list_head xfrs;
-	spinlock_t list_lock;
-	struct mutex occ_lock;
+	spinlock_t list_lock;		/* lock access to the xfrs list */
+	struct mutex occ_lock;		/* lock access to the hardware */
 	struct work_struct work;
+	bool cancel;
 };
 
 #define to_occ(x)	container_of((x), struct occ, mdev)
 
-struct occ_command {
-	u8 seq_no;
-	u8 cmd_type;
-	u16 data_length;
-	u8 data[OCC_CMD_DATA_BYTES];
-	u16 checksum;
-} __packed;
-
 struct occ_response {
 	u8 seq_no;
 	u8 cmd_type;
 	u8 return_status;
-	u16 data_length;
+	__be16 data_length;
 	u8 data[OCC_RESP_DATA_BYTES];
-	u16 checksum;
+	__be16 checksum;
 } __packed;
 
 /*
  * transfer flags are NOT mutually exclusive
- * 
+ *
  * Initial flags are none; transfer is created and queued from write(). All
- * 	flags are cleared when the transfer is completed by closing the file or
- * 	reading all of the available response data.
+ *  flags are cleared when the transfer is completed by closing the file or
+ *  reading all of the available response data.
  * XFR_IN_PROGRESS is set when a transfer is started from occ_worker_putsram,
- * 	and cleared if the transfer fails or occ_worker_getsram completes.
+ *  and cleared if the transfer fails or occ_worker_getsram completes.
  * XFR_COMPLETE is set when a transfer fails or finishes occ_worker_getsram.
  * XFR_CANCELED is set when the transfer's client is released.
  * XFR_WAITING is set from read() if the transfer isn't complete and
- * 	NONBLOCKING wasn't specified. Cleared in read() when transfer completes
- * 	or fails.
+ *  O_NONBLOCK wasn't specified. Cleared in read() when transfer completes or
+ *  fails.
  */
 enum {
 	XFR_IN_PROGRESS,
@@ -92,9 +94,9 @@  struct occ_xfr {
  * client flags
  *
  * CLIENT_NONBLOCKING is set during open() if the file was opened with the
- * 	O_NONBLOCKING flag.
+ *  O_NONBLOCK flag.
  * CLIENT_XFR_PENDING is set during write() and cleared when all data has been
- * 	read.
+ *  read.
  */
 enum {
 	CLIENT_NONBLOCKING,
@@ -104,7 +106,7 @@  enum {
 struct occ_client {
 	struct occ *occ;
 	struct occ_xfr xfr;
-	spinlock_t lock;
+	spinlock_t lock;		/* lock access to the client state */
 	wait_queue_head_t wait;
 	size_t read_offset;
 	unsigned long flags;
@@ -116,12 +118,15 @@  struct occ_client {
 
 static DEFINE_IDA(occ_ida);
 
-static void occ_enqueue_xfr(struct occ_xfr *xfr)
+static int occ_enqueue_xfr(struct occ_xfr *xfr)
 {
 	int empty;
 	struct occ_client *client = to_client(xfr);
 	struct occ *occ = client->occ;
 
+	if (occ->cancel)
+		return -ECANCELED;
+
 	spin_lock_irq(&occ->list_lock);
 
 	empty = list_empty(&occ->xfrs);
@@ -131,14 +136,15 @@  static void occ_enqueue_xfr(struct occ_xfr *xfr)
 
 	if (empty)
 		queue_work(occ_wq, &occ->work);
+
+	return 0;
 }
 
 static struct occ_client *occ_open_common(struct occ *occ, unsigned long flags)
 {
-	struct occ_client *client;
+	struct occ_client *client = kzalloc(sizeof(*client), GFP_KERNEL);
 
-	client = kzalloc(sizeof(*client), GFP_KERNEL);
-	if (!client)
+	if (!client || occ->cancel)
 		return NULL;
 
 	client->occ = occ;
@@ -172,6 +178,7 @@  static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
 	int rc;
 	size_t bytes;
 	struct occ_xfr *xfr = &client->xfr;
+	struct occ *occ = client->occ;
 
 	if (len > OCC_SRAM_BYTES)
 		return -EINVAL;
@@ -183,8 +190,9 @@  static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
 		if (client->read_offset) {
 			rc = 0;
 			client->read_offset = 0;
-		} else
+		} else {
 			rc = -ENOMSG;
+		}
 
 		goto done;
 	}
@@ -200,8 +208,11 @@  static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
 		spin_unlock_irq(&client->lock);
 
 		rc = wait_event_interruptible(client->wait,
-			test_bit(XFR_COMPLETE, &xfr->flags) ||
-			test_bit(XFR_CANCELED, &xfr->flags));
+					      test_bit(XFR_COMPLETE,
+						       &xfr->flags) ||
+					      test_bit(XFR_CANCELED,
+						       &xfr->flags) ||
+					      occ->cancel);
 
 		spin_lock_irq(&client->lock);
 
@@ -225,12 +236,14 @@  static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
 
 	bytes = min(len, xfr->resp_data_length - client->read_offset);
 	if (ubuf) {
-		if (copy_to_user(ubuf, &xfr->buf[client->read_offset], bytes)) {
+		if (copy_to_user(ubuf, &xfr->buf[client->read_offset],
+				 bytes)) {
 			rc = -EFAULT;
 			goto done;
 		}
-	} else
+	} else {
 		memcpy(kbuf, &xfr->buf[client->read_offset], bytes);
+	}
 
 	client->read_offset += bytes;
 
@@ -250,12 +263,6 @@  static ssize_t occ_read(struct file *file, char __user *buf, size_t len,
 {
 	struct occ_client *client = file->private_data;
 
-	/* check this ahead of time so we don't go changing the xfr state
-	 * needlessly
-	 */
-	if (!access_ok(VERIFY_WRITE, buf, len))
-		return -EFAULT;
-
 	return occ_read_common(client, buf, NULL, len);
 }
 
@@ -272,28 +279,31 @@  static ssize_t occ_write_common(struct occ_client *client,
 		return -EINVAL;
 
 	spin_lock_irq(&client->lock);
-	if (test_and_set_bit(CLIENT_XFR_PENDING, &client->flags)) {
+
+	if (test_bit(CLIENT_XFR_PENDING, &client->flags)) {
 		rc = -EBUSY;
 		goto done;
 	}
 
-	/* clear out the transfer */
-	memset(xfr, 0, sizeof(*xfr));
-
-	xfr->buf[0] = 1;
+	memset(xfr, 0, sizeof(*xfr));	/* clear out the transfer */
+	xfr->buf[0] = 1;		/* occ sequence number */
 
+	/* Assume user data follows the occ command format.
+	 * byte 0: command type
+	 * bytes 1-2: data length (msb first)
+	 * bytes 3-n: data
+	 */
 	if (ubuf) {
 		if (copy_from_user(&xfr->buf[1], ubuf, len)) {
-			kfree(xfr);
 			rc = -EFAULT;
 			goto done;
 		}
-	} else
+	} else {
 		memcpy(&xfr->buf[1], kbuf, len);
+	}
 
 	data_length = (xfr->buf[2] << 8) + xfr->buf[3];
 	if (data_length > OCC_CMD_DATA_BYTES) {
-		kfree(xfr);
 		rc = -EINVAL;
 		goto done;
 	}
@@ -306,9 +316,11 @@  static ssize_t occ_write_common(struct occ_client *client,
 
 	xfr->cmd_data_length = data_length + 6;
 	client->read_offset = 0;
+	rc = occ_enqueue_xfr(xfr);
+	if (rc)
+		goto done;
 
-	occ_enqueue_xfr(xfr);
-
+	set_bit(CLIENT_XFR_PENDING, &client->flags);
 	rc = len;
 
 done:
@@ -321,12 +333,6 @@  static ssize_t occ_write(struct file *file, const char __user *buf,
 {
 	struct occ_client *client = file->private_data;
 
-	/* check this ahead of time so we don't go changing the xfr state
-	 * needlessly
-	 */
-	if (!access_ok(VERIFY_READ, buf, len))
-		return -EFAULT;
-
 	return occ_write_common(client, buf, NULL, len);
 }
 
@@ -366,7 +372,7 @@  static int occ_release_common(struct occ_client *client)
 		return 0;
 	}
 
-	/* operation is in progress; let worker clean up*/
+	/* operation is in progress; let worker clean up */
 	spin_unlock_irq(&occ->list_lock);
 	spin_unlock_irq(&client->lock);
 	return 0;
@@ -403,7 +409,7 @@  static int occ_write_sbefifo(struct sbefifo_client *client, const char *buf,
 		total += rc;
 	} while (total < len);
 
-	return (total == len) ? 0 : -EMSGSIZE;
+	return (total == len) ? 0 : -ENODATA;
 }
 
 static int occ_read_sbefifo(struct sbefifo_client *client, char *buf,
@@ -422,7 +428,7 @@  static int occ_read_sbefifo(struct sbefifo_client *client, char *buf,
 		total += rc;
 	} while (total < len);
 
-	return (total == len) ? 0 : -EMSGSIZE;
+	return (total == len) ? 0 : -ENODATA;
 }
 
 static int occ_getsram(struct device *sbefifo, u32 address, u8 *data,
@@ -430,10 +436,13 @@  static int occ_getsram(struct device *sbefifo, u32 address, u8 *data,
 {
 	int rc;
 	u8 *resp;
-	u32 buf[5];
-	u32 data_len = ((len + 7) / 8) * 8;
+	__be32 buf[5];
+	u32 data_len = ((len + 7) / 8) * 8;	/* must be multiples of 8 B */
 	struct sbefifo_client *client;
 
+	/* Magic sequence to do SBE getsram command. SBE will fetch data from
+	 * specified SRAM address.
+	 */
 	buf[0] = cpu_to_be32(0x5);
 	buf[1] = cpu_to_be32(0xa403);
 	buf[2] = cpu_to_be32(1);
@@ -447,7 +456,7 @@  static int occ_getsram(struct device *sbefifo, u32 address, u8 *data,
 	rc = occ_write_sbefifo(client, (const char *)buf, sizeof(buf));
 	if (rc)
 		goto done;
-	
+
 	resp = kzalloc(data_len, GFP_KERNEL);
 	if (!resp) {
 		rc = -ENOMEM;
@@ -467,7 +476,7 @@  static int occ_getsram(struct device *sbefifo, u32 address, u8 *data,
 	    (be32_to_cpu(buf[1]) == 0xC0DEA403))
 		memcpy(data, resp, len);
 	else
-		rc = -EFAULT;
+		rc = -EBADMSG;
 
 free:
 	kfree(resp);
@@ -481,8 +490,8 @@  static int occ_putsram(struct device *sbefifo, u32 address, u8 *data,
 		       ssize_t len)
 {
 	int rc;
-	u32 *buf;
-	u32 data_len = ((len + 7) / 8) * 8;
+	__be32 *buf;
+	u32 data_len = ((len + 7) / 8) * 8;	/* must be multiples of 8 B */
 	size_t cmd_len = data_len + 20;
 	struct sbefifo_client *client;
 
@@ -490,6 +499,9 @@  static int occ_putsram(struct device *sbefifo, u32 address, u8 *data,
 	if (!buf)
 		return -ENOMEM;
 
+	/* Magic sequence to do SBE putsram command. SBE will transfer
+	 * data to specified SRAM address.
+	 */
 	buf[0] = cpu_to_be32(0x5 + (data_len / 4));
 	buf[1] = cpu_to_be32(0xa404);
 	buf[2] = cpu_to_be32(1);
@@ -515,7 +527,7 @@  static int occ_putsram(struct device *sbefifo, u32 address, u8 *data,
 	/* check for good response */
 	if ((be32_to_cpu(buf[0]) != data_len) ||
 	    (be32_to_cpu(buf[1]) != 0xC0DEA404))
-		rc = -EFAULT;
+		rc = -EBADMSG;
 
 done:
 	sbefifo_drv_release(client);
@@ -527,14 +539,17 @@  static int occ_putsram(struct device *sbefifo, u32 address, u8 *data,
 static int occ_trigger_attn(struct device *sbefifo)
 {
 	int rc;
-	u32 buf[6];
+	__be32 buf[6];
 	struct sbefifo_client *client;
 
+	/* Magic sequence to do SBE putscom command. SBE will write 8 bytes to
+	 * specified SCOM address.
+	 */
 	buf[0] = cpu_to_be32(0x6);
 	buf[1] = cpu_to_be32(0xa202);
 	buf[2] = 0;
 	buf[3] = cpu_to_be32(0x6D035);
-	buf[4] = cpu_to_be32(0x20010000);
+	buf[4] = cpu_to_be32(0x20010000);	/* trigger occ attention */
 	buf[5] = 0;
 
 	client = sbefifo_drv_open(sbefifo, 0);
@@ -552,7 +567,7 @@  static int occ_trigger_attn(struct device *sbefifo)
 	/* check for good response */
 	if ((be32_to_cpu(buf[0]) != 0xC0DEA202) ||
 	    (be32_to_cpu(buf[1]) & 0x0FFFFFFF))
-		rc = -EFAULT;
+		rc = -EBADMSG;
 
 done:
 	sbefifo_drv_release(client);
@@ -564,12 +579,19 @@  static void occ_worker(struct work_struct *work)
 {
 	int rc = 0, empty, waiting, canceled;
 	u16 resp_data_length;
+	unsigned long start;
+	const unsigned long timeout = msecs_to_jiffies(OCC_TIMEOUT_MS);
+	const long int wait_time = msecs_to_jiffies(OCC_CMD_IN_PRG_WAIT_MS);
 	struct occ_xfr *xfr;
+	struct occ_response *resp;
 	struct occ_client *client;
 	struct occ *occ = container_of(work, struct occ, work);
 	struct device *sbefifo = occ->sbefifo;
 
 again:
+	if (occ->cancel)
+		return;
+
 	spin_lock_irq(&occ->list_lock);
 
 	xfr = list_first_entry_or_null(&occ->xfrs, struct occ_xfr, link);
@@ -578,11 +600,14 @@  static void occ_worker(struct work_struct *work)
 		return;
 	}
 
+	resp = (struct occ_response *)xfr->buf;
 	set_bit(XFR_IN_PROGRESS, &xfr->flags);
 
 	spin_unlock_irq(&occ->list_lock);
 	mutex_lock(&occ->occ_lock);
 
+	/* write occ command */
+	start = jiffies;
 	rc = occ_putsram(sbefifo, 0xFFFBE000, xfr->buf,
 			 xfr->cmd_data_length);
 	if (rc)
@@ -592,13 +617,26 @@  static void occ_worker(struct work_struct *work)
 	if (rc)
 		goto done;
 
-	rc = occ_getsram(sbefifo, 0xFFFBF000, xfr->buf, 8);
-	if (rc)
-		goto done;
+	/* read occ response */
+	do {
+		rc = occ_getsram(sbefifo, 0xFFFBF000, xfr->buf, 8);
+		if (rc)
+			goto done;
+
+		if (resp->return_status == OCC_RESP_CMD_IN_PRG) {
+			rc = -EALREADY;
+
+			if (time_after(jiffies, start + timeout))
+				break;
 
-	resp_data_length = (xfr->buf[3] << 8) + xfr->buf[4];
+			set_current_state(TASK_INTERRUPTIBLE);
+			schedule_timeout(wait_time);
+		}
+	} while (rc);
+
+	resp_data_length = get_unaligned_be16(&resp->data_length);
 	if (resp_data_length > OCC_RESP_DATA_BYTES) {
-		rc = -EDOM;
+		rc = -EMSGSIZE;
 		goto done;
 	}
 
@@ -704,9 +742,6 @@  static int occ_probe(struct platform_device *pdev)
 	mutex_init(&occ->occ_lock);
 	INIT_WORK(&occ->work, occ_worker);
 
-	/* ensure NULL before we probe children, so they don't hang FSI */
-	platform_set_drvdata(pdev, NULL);
-
 	if (dev->of_node) {
 		rc = of_property_read_u32(dev->of_node, "reg", &reg);
 		if (!rc) {
@@ -716,23 +751,13 @@  static int occ_probe(struct platform_device *pdev)
 			if (occ->idx < 0)
 				occ->idx = ida_simple_get(&occ_ida, 1, INT_MAX,
 							  GFP_KERNEL);
-		} else
+		} else {
 			occ->idx = ida_simple_get(&occ_ida, 1, INT_MAX,
 						  GFP_KERNEL);
-
-		/* create platform devs for dts child nodes (hwmon, etc) */
-		for_each_child_of_node(dev->of_node, np) {
-			snprintf(child_name, sizeof(child_name), "occ%d-dev%d",
-				 occ->idx, child_idx++);
-			child = of_platform_device_create(np, child_name, dev);
-			if (!child)
-				dev_warn(dev,
-					 "failed to create child node dev\n");
 		}
-	} else
+	} else {
 		occ->idx = ida_simple_get(&occ_ida, 1, INT_MAX, GFP_KERNEL);
-
-	platform_set_drvdata(pdev, occ);
+	}
 
 	snprintf(occ->name, sizeof(occ->name), "occ%d", occ->idx);
 	occ->mdev.fops = &occ_fops;
@@ -742,20 +767,42 @@  static int occ_probe(struct platform_device *pdev)
 
 	rc = misc_register(&occ->mdev);
 	if (rc) {
-		dev_err(dev, "failed to register miscdevice\n");
+		dev_err(dev, "failed to register miscdevice: %d\n", rc);
+		ida_simple_remove(&occ_ida, occ->idx);
 		return rc;
 	}
 
+	/* create platform devs for dts child nodes (hwmon, etc) */
+	for_each_available_child_of_node(dev->of_node, np) {
+		snprintf(child_name, sizeof(child_name), "occ%d-dev%d",
+			 occ->idx, child_idx++);
+		child = of_platform_device_create(np, child_name, dev);
+		if (!child)
+			dev_warn(dev, "failed to create child node dev\n");
+	}
+
+	platform_set_drvdata(pdev, occ);
+
 	return 0;
 }
 
 static int occ_remove(struct platform_device *pdev)
 {
 	struct occ *occ = platform_get_drvdata(pdev);
+	struct occ_xfr *xfr;
+	struct occ_client *client;
+
+	occ->cancel = true;
+	list_for_each_entry(xfr, &occ->xfrs, link) {
+		client = to_client(xfr);
+		wake_up_interruptible(&client->wait);
+	}
 
-	flush_work(&occ->work);
 	misc_deregister(&occ->mdev);
 	device_for_each_child(&pdev->dev, NULL, occ_unregister_child);
+
+	flush_work(&occ->work);
+
 	ida_simple_remove(&occ_ida, occ->idx);
 
 	return 0;
@@ -789,6 +836,8 @@  static void occ_exit(void)
 	destroy_workqueue(occ_wq);
 
 	platform_driver_unregister(&occ_driver);
+
+	ida_destroy(&occ_ida);
 }
 
 module_init(occ_init);
diff --git a/include/linux/occ.h b/include/linux/occ.h
index d78332c..0a4a54a 100644
--- a/include/linux/occ.h
+++ b/include/linux/occ.h
@@ -11,12 +11,26 @@ 
  * GNU General Public License for more details.
  */
 
-#ifndef __OCC_H__
-#define __OCC_H__
+#ifndef LINUX_FSI_OCC_H
+#define LINUX_FSI_OCC_H
 
 struct device;
 struct occ_client;
 
+#define OCC_RESP_CMD_IN_PRG		0xFF
+#define OCC_RESP_SUCCESS		0
+#define OCC_RESP_CMD_INVAL		0x11
+#define OCC_RESP_CMD_LEN_INVAL		0x12
+#define OCC_RESP_DATA_INVAL		0x13
+#define OCC_RESP_CHKSUM_ERR		0x14
+#define OCC_RESP_INT_ERR		0x15
+#define OCC_RESP_BAD_STATE		0x16
+#define OCC_RESP_CRIT_EXCEPT		0xE0
+#define OCC_RESP_CRIT_INIT		0xE1
+#define OCC_RESP_CRIT_WATCHDOG		0xE2
+#define OCC_RESP_CRIT_OCB		0xE3
+#define OCC_RESP_CRIT_HW		0xE4
+
 extern struct occ_client *occ_drv_open(struct device *dev,
 				       unsigned long flags);
 extern int occ_drv_read(struct occ_client *client, char *buf, size_t len);
@@ -24,4 +38,4 @@  extern int occ_drv_write(struct occ_client *client, const char *buf,
 			 size_t len);
 extern void occ_drv_release(struct occ_client *client);
 
-#endif /* __OCC_H__ */
+#endif /* LINUX_FSI_OCC_H */