[PATCH linux dev-4.10 v2 4/9] drivers: fsi: occ: General clean-up
Andrew Jeffery
andrew at aj.id.au
Thu Oct 5 11:37:54 AEDT 2017
On Fri, 2017-09-29 at 17:41 -0500, Eddie James wrote:
> From: "Edward A. James" <eajames at us.ibm.com>
>
> Fix include files. Cleanup formatting and add some comments. Fix an
> errant kfree in an error case. Fix some errnos. Add OCC response
> definitions to the header file.
Unlike the SBEFIFO patch I haven't studied the diff here, but I also don't
intend on ack'ing this as it stands. There's just too much going on to judge
that it's reasonable.
Andrew
>
> Signed-off-by: Edward A. James <eajames at us.ibm.com>
> ---
> drivers/fsi/occ.c | 145 ++++++++++++++++++++++++++++------------------------
> include/linux/occ.h | 20 ++++++--
> 2 files changed, 96 insertions(+), 69 deletions(-)
>
> diff --git a/drivers/fsi/occ.c b/drivers/fsi/occ.c
> index 621fbf0..3313e35 100644
> --- a/drivers/fsi/occ.c
> +++ b/drivers/fsi/occ.c
> @@ -10,16 +10,21 @@
> #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/slab.h>
> +#include <linux/spinlock.h>
> #include <linux/uaccess.h>
> #include <linux/wait.h>
> #include <linux/workqueue.h>
> @@ -34,43 +39,35 @@ struct occ {
> 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;
> };
>
> #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 +89,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 +101,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;
> @@ -135,9 +132,8 @@ static void occ_enqueue_xfr(struct occ_xfr *xfr)
>
> 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)
> return NULL;
>
> @@ -183,8 +179,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 +197,10 @@ 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));
>
> spin_lock_irq(&client->lock);
>
> @@ -225,12 +224,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 +251,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 +267,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)) {
> 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;
> }
> @@ -321,12 +319,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 +358,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 +395,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 : -ENOSPC;
> }
>
> static int occ_read_sbefifo(struct sbefifo_client *client, char *buf,
> @@ -422,7 +414,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 +422,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 +442,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 +462,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 +476,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 +485,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 +513,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 +525,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 +553,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);
> @@ -565,6 +566,7 @@ static void occ_worker(struct work_struct *work)
> int rc = 0, empty, waiting, canceled;
> u16 resp_data_length;
> 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;
> @@ -578,11 +580,13 @@ 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 */
> rc = occ_putsram(sbefifo, 0xFFFBE000, xfr->buf,
> xfr->cmd_data_length);
> if (rc)
> @@ -592,13 +596,14 @@ static void occ_worker(struct work_struct *work)
> if (rc)
> goto done;
>
> + /* read occ response */
> rc = occ_getsram(sbefifo, 0xFFFBF000, xfr->buf, 8);
> if (rc)
> goto done;
>
> - resp_data_length = (xfr->buf[3] << 8) + xfr->buf[4];
> + resp_data_length = get_unaligned_be16(&resp->data_length);
> if (resp_data_length > OCC_RESP_DATA_BYTES) {
> - rc = -EDOM;
> + rc = -EMSGSIZE;
> goto done;
> }
>
> @@ -657,18 +662,27 @@ struct occ_client *occ_drv_open(struct device *dev, unsigned long flags)
>
> int occ_drv_read(struct occ_client *client, char *buf, size_t len)
> {
> + if (!client)
> + return -ENODEV;
> +
> return occ_read_common(client, NULL, buf, len);
> }
> EXPORT_SYMBOL_GPL(occ_drv_read);
>
> int occ_drv_write(struct occ_client *client, const char *buf, size_t len)
> {
> + if (!client)
> + return -ENODEV;
> +
> return occ_write_common(client, NULL, buf, len);
> }
> EXPORT_SYMBOL_GPL(occ_drv_write);
>
> void occ_drv_release(struct occ_client *client)
> {
> + if (!client)
> + return;
> +
> occ_release_common(client);
> }
> EXPORT_SYMBOL_GPL(occ_drv_release);
> @@ -704,9 +718,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", ®);
> if (!rc) {
> @@ -789,6 +800,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 */
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: This is a digitally signed message part
URL: <http://lists.ozlabs.org/pipermail/openbmc/attachments/20171005/004f5121/attachment-0001.sig>
More information about the openbmc
mailing list