drivers: fsi: client fixes and refactor
Subsystems (2)
v4 cover drivers: fsi: client fixes and refactor
From: "Edward A. James" <[email protected]>
This series refactors the FSI client device drivers. It adds a number of fixes discovered from bugs over the past several months. Most critically, it fixes various race conditions in the unbind/remove pathway of the occ-hwmon, occ, and sbefifo drivers.
Apologies for spamming the list with so many patches...
Changes since v3:
* switch checks for OCC client NULL to the common read/write/close functions.
* add helper function for ugly wait_event_interruptible in OCC read().
* removed useless check for cancel in occ open().
Changes since v2:
* split "General clean-up" patches into many more patches
* Change to wake_up_all() in various places
* Set sbe pointer NULL and add check in occ-hwmon. For race safety.
Changes since v1:
* split into multiple patches.
* add memory management patch.
Edward A. James (31):
drivers: fsi: sbefifo: Fix includes
drivers: fsi: sbefifo: Use a defined reschedule length
drivers: fsi: sbefifo: Use __be32 for big endian values
drivers: fsi: sbefifo: white space fixes
drivers: fsi: sbefifo: replace awkward wait_event expression
drivers: fsi: sbefifo: remove redundant function
drivers: fsi: sbefifo: Use goto to reduce put statements
drivers: fsi: sbefifo: Do an earlier get_client call
drivers: fsi: sbefifo: Remove warning and user data access check
drivers: fsi: sbefifo: destroy the ida list on exit
drivers: fsi: sbefifo: Fix module authors and comments
drivers: fsi: sbefifo: Fix include guards in header file
drivers: fsi: SBEFIFO: Fix probe() and remove()
drivers: fsi: SBEFIFO: check for xfr complete in read wait_event
drivers: fsi: occ: Fix includes
drivers: fsi: occ: Fix errant kfree calls
drivers: fsi: occ: remove unused occ_command structure
drivers: fsi: occ: Use big-endian values
drivers: fsi: occ: Return ENODEV if client is NULL
drivers: fsi: occ: Remove early user buffer checking
drivers: fsi: occ: Switch to more logical errnos
drivers: fsi: occ: fix white space and bracket problems
drivers: fsi: occ: Destroy the ida list on exit
drivers: fsi: occ: Remove unnecessary platform_set_drvdata call
drivers: fsi: occ: Add comments for clarity
drivers: fsi: occ: Add OCC response definitions to header
drivers: fsi: occ: Poll while receiving "command in progress"
drivers: fsi: occ: Add cancel to remove() and fix probe()
drivers: fsi: occ: Fix client memory management drivers/hwmon/occ: Remove repeated ops for OCC command in progress
drivers: hwmon: occ: Cancel occ operations in remove()
drivers/fsi/fsi-sbefifo.c | 311 +++++++++++++++++++++------------------ drivers/fsi/occ.c | 345 ++++++++++++++++++++++++++------------------ drivers/hwmon/occ/p9_sbe.c | 47 +++--- include/linux/fsi-sbefifo.h | 6 +- include/linux/occ.h | 20 ++- 5 files changed, 426 insertions(+), 303 deletions(-)
Signature · 1 line
1.8.3.1
PATCH v4 1/31 drivers: fsi: sbefifo: Fix includes
From: "Edward A. James" <[email protected]>
Need to include everything we use and not rely on recursive includes.
--- drivers/fsi/fsi-sbefifo.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)
drivers/fsi/fsi-sbefifo.c +9 -2
@@ -11,20 +11,27 @@* GNU General Public License for more details. */ -#include <linux/delay.h> +#include <linux/device.h> #include <linux/errno.h> -#include <linux/idr.h> +#include <linux/fs.h> #include <linux/fsi.h> +#include <linux/fsi-sbefifo.h> +#include <linux/idr.h> +#include <linux/kernel.h> +#include <linux/kref.h> #include <linux/list.h> #include <linux/miscdevice.h> #include <linux/module.h> #include <linux/of.h> +#include <linux/of_device.h> #include <linux/of_platform.h> #include <linux/poll.h> #include <linux/sched.h> #include <linux/slab.h> +#include <linux/spinlock.h> #include <linux/timer.h> #include <linux/uaccess.h> +#include <linux/wait.h> /* * The SBEFIFO is a pipe-like FSI device for communicating with
Signature · 1 line
1.8.3.1
PATCH v4 2/31 drivers: fsi: sbefifo: Use a defined reschedule length
In reply to Eddie JamesFrom: "Edward A. James" <[email protected]>
Switch to a #defined reschedule period instead of using 500 ms twice.
--- drivers/fsi/fsi-sbefifo.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
drivers/fsi/fsi-sbefifo.c +4 -2
@@ -51,6 +51,8 @@#define SBEFIFO_EOT_MAGIC 0xffffffff#define SBEFIFO_EOT_ACK 0x14 +#define SBEFIFO_RESCHEDULE msecs_to_jiffies(500) +struct sbefifo {struct timer_list poll_timer;struct fsi_device *fsi_dev;
@@ -404,7 +406,7 @@ static void sbefifo_poll_timer(unsigned long data)if (devn ==0 ) {/* No open slot for write. Reschedule. */ sbefifo->poll_timer.expires = jiffies + -msecs_to_jiffies (500 ); +SBEFIFO_RESCHEDULE ;add_timer (&sbefifo->poll_timer); goto out_unlock; }
@@ -445,7 +447,7 @@ static void sbefifo_poll_timer(unsigned long data)if (devn ==0 ) {/* No data yet. Reschedule. */ sbefifo->poll_timer.expires = jiffies + -msecs_to_jiffies (500 ); +SBEFIFO_RESCHEDULE ;add_timer (&sbefifo->poll_timer); goto out_unlock; }
Signature · 1 line
1.8.3.1
PATCH v4 3/31 drivers: fsi: sbefifo: Use __be32 for big endian values
In reply to Eddie JamesFrom: "Edward A. James" <[email protected]>
Use a real __be32 value instead of u32 for big endian values.
--- drivers/fsi/fsi-sbefifo.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
drivers/fsi/fsi-sbefifo.c +4 -2
@@ -103,7 +103,7 @@ struct sbefifo_client {static int sbefifo_inw (struct sbefifo *sbefifo,int reg,u32 *word) {int rc; -u32 raw_word; +__be32 raw_word; rc =fsi_device_read (sbefifo->fsi_dev, reg, &raw_word,sizeof (raw_word));
@@ -111,17 +111,19 @@ static int sbefifo_inw(struct sbefifo *sbefifo, int reg, u32 *word)return rc; *word =be32_to_cpu (raw_word); +return 0 ; }static int sbefifo_outw (struct sbefifo *sbefifo,int reg,u32 word) { -u32 raw_word =cpu_to_be32 (word); +__be32 raw_word =cpu_to_be32 (word);return fsi_device_write (sbefifo->fsi_dev, reg, &raw_word,sizeof (raw_word)); } +/* Don't flip endianness of data to/from FIFO, just pass through. */ static int sbefifo_readw (struct sbefifo *sbefifo,u32 *word) { return fsi_device_read(sbefifo->fsi_dev, SBEFIFO_DWN, word,
Signature · 1 line
1.8.3.1
PATCH v4 4/31 drivers: fsi: sbefifo: white space fixes
In reply to Eddie JamesFrom: "Edward A. James" <[email protected]>
Fix some spacing issues.
--- drivers/fsi/fsi-sbefifo.c | 66 ++++++++++++++++++++++++----------------------- 1 file changed, 34 insertions(+), 32 deletions(-)
drivers/fsi/fsi-sbefifo.c +34 -32
@@ -147,7 +147,7 @@ static int sbefifo_ack_eot(struct sbefifo *sbefifo)return ret;return sbefifo_outw (sbefifo,SBEFIFO_DWN |SBEFIFO_EOT_ACK , -SBEFIFO_EOT_MAGIC ); +SBEFIFO_EOT_MAGIC ); }static size_t sbefifo_dev_nwreadable (u32 sts)
@@ -221,6 +221,7 @@ static bool sbefifo_buf_readnb(struct sbefifo_buf *buf, size_t n) rpos = buf->buf;WRITE_ONCE (buf->rpos, rpos); +return rpos == wpos; }
@@ -240,14 +241,14 @@ static bool sbefifo_buf_wrotenb(struct sbefifo_buf *buf, size_t n)set_bit (SBEFIFO_BUF_FULL , &buf->flags);WRITE_ONCE (buf->wpos, wpos); +return rpos == wpos; }static void sbefifo_free (struct kref *kref) { -struct sbefifo *sbefifo; + struct sbefifo *sbefifo = container_of(kref, struct sbefifo, kref); - sbefifo = container_of(kref, struct sbefifo, kref);kfree (sbefifo); }
@@ -284,8 +285,7 @@ static struct sbefifo_xfr *sbefifo_client_next_xfr(if (list_empty (&client->xfrs))return NULL ; - return container_of(client->xfrs.next, struct sbefifo_xfr, - client); + return container_of(client->xfrs.next, struct sbefifo_xfr, client); }static bool sbefifo_xfr_rsp_pending (struct sbefifo_client *client)
@@ -360,6 +360,7 @@ static struct sbefifo_xfr *sbefifo_next_xfr(struct sbefifo *sbefifo)kfree (xfr);continue ; } +return xfr; }
@@ -381,7 +382,7 @@ static void sbefifo_poll_timer(unsigned long data)spin_lock (&sbefifo->lock); xfr = list_first_entry_or_null(&sbefifo->xfrs, struct sbefifo_xfr, - xfrs); + xfrs);if (!xfr) goto out_unlock;
@@ -399,8 +400,7 @@ static void sbefifo_poll_timer(unsigned long data)/* Drain the write buffer. */ while ((bufn =sbefifo_buf_nbreadable (wbuf))) { - ret =sbefifo_inw (sbefifo,SBEFIFO_UP |SBEFIFO_STS , - &sts); + ret =sbefifo_inw (sbefifo,SBEFIFO_UP |SBEFIFO_STS , &sts);if (ret) goto out;
@@ -425,9 +425,8 @@ static void sbefifo_poll_timer(unsigned long data)/* Send EOT if the writer is finished. */ if (test_and_clear_bit (SBEFIFO_XFR_WRITE_DONE , &xfr->flags)) { - ret =sbefifo_outw (sbefifo, -SBEFIFO_UP |SBEFIFO_EOT_RAISE , -SBEFIFO_EOT_MAGIC ); + ret =sbefifo_outw (sbefifo,SBEFIFO_UP |SBEFIFO_EOT_RAISE , +SBEFIFO_EOT_MAGIC );if (ret) goto out;
@@ -477,7 +476,7 @@ static void sbefifo_poll_timer(unsigned long data)set_bit (SBEFIFO_XFR_COMPLETE , &xfr->flags);list_del (&xfr->xfrs);if (unlikely (test_bit (SBEFIFO_XFR_CANCEL , - &xfr->flags))) + &xfr->flags)))kfree (xfr);break ; }
@@ -487,7 +486,7 @@ static void sbefifo_poll_timer(unsigned long data)if (unlikely (ret)) { sbefifo->rc = ret;dev_err (&sbefifo->fsi_dev->dev, -"Fatal bus access failure: %d\n" , ret); +"Fatal bus access failure: %d\n" , ret);list_for_each_entry (xfr, &sbefifo->xfrs, xfrs)kfree (xfr);INIT_LIST_HEAD (&sbefifo->xfrs);
@@ -508,7 +507,7 @@ static void sbefifo_poll_timer(unsigned long data)static int sbefifo_open (struct inode *inode,struct file *file) {struct sbefifo *sbefifo =container_of (file->private_data, - struct sbefifo, mdev); + struct sbefifo, mdev);struct sbefifo_client *client;int ret;
@@ -562,12 +561,13 @@ static ssize_t sbefifo_read_common(struct sbefifo_client *client,sbefifo_get_client (client);if (wait_event_interruptible (sbefifo->wait, - (ret =READ_ONCE (sbefifo->rc)) || - (n =sbefifo_buf_nbreadable ( - &client->rbuf)))) { + (ret =READ_ONCE (sbefifo->rc)) || + (n =sbefifo_buf_nbreadable ( + &client->rbuf)))) {sbefifo_put_client (client);return -ERESTARTSYS ; } +if (ret) {INIT_LIST_HEAD (&client->xfrs);sbefifo_put_client (client);
@@ -581,8 +581,9 @@ static ssize_t sbefifo_read_common(struct sbefifo_client *client,sbefifo_put_client (client);return -EFAULT ; } - }else + }else {memcpy (kbuf,READ_ONCE (client->rbuf.rpos), n); + }if (sbefifo_buf_readnb (&client->rbuf, n)) { xfr =sbefifo_client_next_xfr (client);
@@ -605,8 +606,8 @@ static ssize_t sbefifo_read_common(struct sbefifo_client *client,return n; } -static ssize_t sbefifo_read (struct file *file,char __user *buf, -size_t len,loff_t *offset) +static ssize_t sbefifo_read (struct file *file,char __user *buf,size_t len, +loff_t *offset) {struct sbefifo_client *client = file->private_data;
@@ -648,6 +649,7 @@ static ssize_t sbefifo_write_common(struct sbefifo_client *client,spin_unlock_irq (&sbefifo->lock);return -ENOMEM ; } +spin_unlock_irq (&sbefifo->lock);sbefifo_get_client (client);
@@ -659,8 +661,8 @@ static ssize_t sbefifo_write_common(struct sbefifo_client *client,while (len) {if (wait_event_interruptible (sbefifo->wait,READ_ONCE (sbefifo->rc) || - (sbefifo_client_next_xfr (client) == xfr && - (n =sbefifo_buf_nbwriteable ( + (sbefifo_client_next_xfr (client) == xfr && + (n =sbefifo_buf_nbwriteable ( &client->wbuf))))) {set_bit (SBEFIFO_XFR_CANCEL , &xfr->flags);sbefifo_get (sbefifo);
@@ -670,6 +672,7 @@ static ssize_t sbefifo_write_common(struct sbefifo_client *client,sbefifo_put_client (client);return -ERESTARTSYS ; } +if (sbefifo->rc) {INIT_LIST_HEAD (&client->xfrs);sbefifo_put_client (client);
@@ -719,7 +722,7 @@ static ssize_t sbefifo_write_common(struct sbefifo_client *client, }static ssize_t sbefifo_write (struct file *file,const char __user *buf, -size_t len,loff_t *offset) +size_t len,loff_t *offset) {struct sbefifo_client *client = file->private_data;
@@ -820,22 +823,21 @@ static int sbefifo_probe(struct device *dev) sbefifo->fsi_dev = fsi_dev; - ret =sbefifo_inw (sbefifo, -SBEFIFO_UP |SBEFIFO_STS , &sts); + ret =sbefifo_inw (sbefifo,SBEFIFO_UP |SBEFIFO_STS , &sts);if (ret)return ret; +if (!(sts &SBEFIFO_EMPTY )) { -dev_err (&sbefifo->fsi_dev->dev, -"Found data in upstream fifo\n" ); +dev_err (dev,"Found data in upstream fifo\n" );return -EIO ; } ret =sbefifo_inw (sbefifo,SBEFIFO_DWN |SBEFIFO_STS , &sts);if (ret)return ret; +if (!(sts &SBEFIFO_EMPTY )) { -dev_err (&sbefifo->fsi_dev->dev, -"Found data in downstream fifo\n" ); +dev_err (dev,"Found data in downstream fifo\n" );return -EIO ; }
@@ -848,13 +850,13 @@ static int sbefifo_probe(struct device *dev) sbefifo->idx =ida_simple_get (&sbefifo_ida,1 ,INT_MAX ,GFP_KERNEL );snprintf (sbefifo->name,sizeof (sbefifo->name),"sbefifo%d" , - sbefifo->idx); + sbefifo->idx);init_waitqueue_head (&sbefifo->wait);INIT_LIST_HEAD (&sbefifo->xfrs);/* This bit of silicon doesn't offer any interrupts... */ setup_timer (&sbefifo->poll_timer, sbefifo_poll_timer, - (unsigned long )sbefifo); + (unsigned long )sbefifo);if (dev->of_node) {/* create platform devs for dts child nodes (occ, etc) */
@@ -863,7 +865,7 @@ static int sbefifo_probe(struct device *dev) sbefifo->name, child_idx++); child =of_platform_device_create (np, child_name, dev);if (!child) -dev_warn (&sbefifo->fsi_dev->dev, +dev_warn (dev,"failed to create child node dev\n" ); } }
Signature · 1 line
1.8.3.1
PATCH v4 5/31 drivers: fsi: sbefifo: replace awkward wait_event expression
In reply to Eddie JamesFrom: "Edward A. James" <[email protected]>
Use ready() functions to replace awkward boolean expressions in the wait_event_interruptible calls.
--- drivers/fsi/fsi-sbefifo.c | 32 +++++++++++++++++++++++++------- 1 file changed, 25 insertions(+), 7 deletions(-)
drivers/fsi/fsi-sbefifo.c +25 -7
@@ -545,6 +545,16 @@ static unsigned int sbefifo_poll(struct file *file, poll_table *wait)return mask; } +static bool sbefifo_read_ready (struct sbefifo *sbefifo, +struct sbefifo_client *client,size_t *n, +size_t *ret) +{ + *n =sbefifo_buf_nbreadable (&client->rbuf); + *ret =READ_ONCE (sbefifo->rc); + +return *ret || *n; +} +static ssize_t sbefifo_read_common (struct sbefifo_client *client,char __user *ubuf,char *kbuf,size_t len) {
@@ -561,9 +571,8 @@ static ssize_t sbefifo_read_common(struct sbefifo_client *client,sbefifo_get_client (client);if (wait_event_interruptible (sbefifo->wait, - (ret =READ_ONCE (sbefifo->rc)) || - (n =sbefifo_buf_nbreadable ( - &client->rbuf)))) { +sbefifo_read_ready (sbefifo, client, &n, + &ret))) {sbefifo_put_client (client);return -ERESTARTSYS ; }
@@ -619,6 +628,16 @@ static ssize_t sbefifo_read(struct file *file, char __user *buf, size_t len,return sbefifo_read_common (client, buf,NULL , len); } +static bool sbefifo_write_ready (struct sbefifo *sbefifo, +struct sbefifo_xfr *xfr, +struct sbefifo_client *client,size_t *n) +{ +struct sbefifo_xfr *next =sbefifo_client_next_xfr (client); + + *n =sbefifo_buf_nbwriteable (&client->wbuf); +return READ_ONCE (sbefifo->rc) || (next == xfr && *n); +} +static ssize_t sbefifo_write_common (struct sbefifo_client *client,const char __user *ubuf,const char *kbuf,size_t len)
@@ -660,10 +679,9 @@ static ssize_t sbefifo_write_common(struct sbefifo_client *client,*/ while (len) {if (wait_event_interruptible (sbefifo->wait, -READ_ONCE (sbefifo->rc) || - (sbefifo_client_next_xfr (client) == xfr && - (n =sbefifo_buf_nbwriteable ( - &client->wbuf))))) { +sbefifo_write_ready (sbefifo, xfr, + client, + &n))) {set_bit (SBEFIFO_XFR_CANCEL , &xfr->flags);sbefifo_get (sbefifo);if (mod_timer (&sbefifo->poll_timer, jiffies))
Signature · 1 line
1.8.3.1
PATCH v4 6/31 drivers: fsi: sbefifo: remove redundant function
In reply to Eddie JamesFrom: "Edward A. James" <[email protected]>
sbefifo_client_next_xfr function just does what list_first_entry_or_null does.
--- drivers/fsi/fsi-sbefifo.c | 28 +++++++++++++++------------- 1 file changed, 15 insertions(+), 13 deletions(-)
drivers/fsi/fsi-sbefifo.c +15 -13
@@ -279,20 +279,12 @@ static struct sbefifo_xfr *sbefifo_enq_xfr(struct sbefifo_client *client)return xfr; } -static struct sbefifo_xfr *sbefifo_client_next_xfr ( -struct sbefifo_client *client) -{ -if (list_empty (&client->xfrs)) -return NULL ; - - return container_of(client->xfrs.next, struct sbefifo_xfr, client); -} -static bool sbefifo_xfr_rsp_pending (struct sbefifo_client *client) { -struct sbefifo_xfr *xfr; +struct sbefifo_xfr *xfr =list_first_entry_or_null (&client->xfrs, + struct sbefifo_xfr, + client); - xfr =sbefifo_client_next_xfr (client);if (xfr &&test_bit (SBEFIFO_XFR_RESP_PENDING , &xfr->flags))return true;
@@ -595,7 +587,15 @@ static ssize_t sbefifo_read_common(struct sbefifo_client *client, }if (sbefifo_buf_readnb (&client->rbuf, n)) { - xfr =sbefifo_client_next_xfr (client); + xfr =list_first_entry_or_null (&client->xfrs, + struct sbefifo_xfr, client); +if (!xfr) { +/* should be impossible to not have an xfr here */ +WARN_ONCE (1 ,"no xfr in queue" ); +sbefifo_put_client (client); +return -EPROTO ; + } +if (!test_bit (SBEFIFO_XFR_COMPLETE , &xfr->flags)) {/* * Fill the read buffer back up.
@@ -632,7 +632,9 @@ static bool sbefifo_write_ready(struct sbefifo *sbefifo,struct sbefifo_xfr *xfr, struct sbefifo_client *client, size_t *n) { - struct sbefifo_xfr *next = sbefifo_client_next_xfr(client); + struct sbefifo_xfr *next = list_first_entry_or_null(&client->xfrs, + struct sbefifo_xfr, + client); *n =sbefifo_buf_nbwriteable (&client->wbuf);return READ_ONCE (sbefifo->rc) || (next == xfr && *n);
Signature · 1 line
1.8.3.1
Re: [PATCH linux dev-4.10 v4 06/31] drivers: fsi: sbefifo: remove redundant function
Quoted 3 lines · Eddie James
From: "Edward A. James" <[email protected]>
sbefifo_client_next_xfrfunction just does what
list_first_entry_or_null
Quoted 3 lines
does.
Signed-off-by Edward A. James <[email protected]>
drivers/fsi/fsi-sbefifo.c static struct sbefifo_xfr
Quoted 9 lines
---
drivers/fsi/fsi-sbefifo.c | 28 +++++++++++++++-------------1 file changed, 15 insertions(+), 13 deletions(-)drivers/fsi/fsi-sbefifo.c +0
-0@@ -279,20 +279,12 @@ static struct sbefifo_xfr
*sbefifo_enq_xfr(struct sbefifo_client *client)
drivers/fsi/fsi-sbefifo.c static struct sbefifo_xfr *sbefifo_enq_xfr(struct sbefifo_client *client)
Quoted 10 lines
drivers/fsi/fsi-sbefifo.c +0
-0return xfr; }drivers/fsi/fsi-sbefifo.c +0
-7-static struct sbefifo_xfr *sbefifo_client_next_xfr ( -struct sbefifo_client *client) -{ -if (list_empty (&client->xfrs)) -return NULL ; - - return container_of(client->xfrs.next, struct sbefifo_xfr,
client);
drivers/fsi/fsi-sbefifo.c
Quoted 8 lines
drivers/fsi/fsi-sbefifo.c +0
-2-} -static bool
sbefifo_xfr_rsp_pending(struct sbefifo_client *client){drivers/fsi/fsi-sbefifo.c +1
-1-struct sbefifo_xfr *xfr; + struct sbefifo_xfr *xfr = list_first_entry_or_null(&client-xfrs,
drivers/fsi/fsi-sbefifo.c +1
-0+ struct
sbefifo_xfr,
drivers/fsi/fsi-sbefifo.c static ssize_t sbefifo_read_common(struct
Quoted 7 lines
drivers/fsi/fsi-sbefifo.c +1
-0+ client);drivers/fsi/fsi-sbefifo.c +0
-1- xfr =sbefifo_client_next_xfr (client);if (xfr &&
test_bit(SBEFIFO_XFR_RESP_PENDING, &xfr->flags)) return true;drivers/fsi/fsi-sbefifo.c +0
-0@@ -595,7 +587,15 @@ static ssize_t sbefifo_read_common(struct
sbefifo_client *client,
drivers/fsi/fsi-sbefifo.c static ssize_t sbefifo_read_common(struct sbefifo_client *client,
Quoted 6 lines
drivers/fsi/fsi-sbefifo.c +0
-0}
if (sbefifo_buf_readnb(&client->rbuf, n)) {drivers/fsi/fsi-sbefifo.c +2
-1- xfr =sbefifo_client_next_xfr (client); + xfr = list_first_entry_or_null(&client->xfrs, + struct sbefifo_xfr,
client);
drivers/fsi/fsi-sbefifo.c
Quoted 2 lines
drivers/fsi/fsi-sbefifo.c +2
-0+if (!xfr) { +/* should be impossible to not have an xfr
here */
drivers/fsi/fsi-sbefifo.c static bool sbefifo_write_ready(struct sbefifo
Quoted 9 lines
drivers/fsi/fsi-sbefifo.c +5
-0+WARN_ONCE (1 ,"no xfr in queue" ); +sbefifo_put_client (client); +return -EPROTO ; + } +if (!test_bit(SBEFIFO_XFR_COMPLETE,
&xfr->flags)) { /*
* Fill the read buffer back up.drivers/fsi/fsi-sbefifo.c +0
-0@@ -632,7 +632,9 @@ static bool sbefifo_write_ready(struct sbefifo
*sbefifo,
drivers/fsi/fsi-sbefifo.c static bool sbefifo_write_ready(struct sbefifo · line 633
Quoted 2 lines
drivers/fsi/fsi-sbefifo.c +0
-0struct sbefifo_xfr *xfr, struct sbefifo_client *client,
size_t *n)
drivers/fsi/fsi-sbefifo.c static bool sbefifo_write_ready(struct sbefifo · line 637
Quoted 5 lines
drivers/fsi/fsi-sbefifo.c +2
-1{ -struct sbefifo_xfr *next =sbefifo_client_next_xfr (client); + struct sbefifo_xfr *next = list_first_entry_or_null(&client- xfrs, + struct
sbefifo_xfr,
Quoted 4 lines
drivers/fsi/fsi-sbefifo.c +1
-0+ client);
*n =sbefifo_buf_nbwriteable(&client->wbuf); returnREAD_ONCE(sbefifo->rc)|| (next == xfr && *n);
PATCH v4 7/31 drivers: fsi: sbefifo: Use goto to reduce put statements
In reply to Eddie JamesFrom: "Edward A. James" <[email protected]>
Use a goto in the read() and write() functions so that we can just have one get_client and put_client statement.
--- drivers/fsi/fsi-sbefifo.c | 35 ++++++++++++++++++----------------- 1 file changed, 18 insertions(+), 17 deletions(-)
drivers/fsi/fsi-sbefifo.c +18 -17
@@ -565,22 +565,21 @@ static ssize_t sbefifo_read_common(struct sbefifo_client *client,if (wait_event_interruptible (sbefifo->wait,sbefifo_read_ready (sbefifo, client, &n, &ret))) { -sbefifo_put_client (client); -return -ERESTARTSYS ; + ret = -ERESTARTSYS ; + goto out; }if (ret) {INIT_LIST_HEAD (&client->xfrs); -sbefifo_put_client (client); -return ret; + goto out; } n =min_t (size_t, n, len);if (ubuf) {if (copy_to_user (ubuf,READ_ONCE (client->rbuf.rpos), n)) { -sbefifo_put_client (client); -return -EFAULT ; + ret = -EFAULT ; + goto out; } }else {memcpy (kbuf,READ_ONCE (client->rbuf.rpos), n);
@@ -592,8 +591,8 @@ static ssize_t sbefifo_read_common(struct sbefifo_client *client,if (!xfr) {/* should be impossible to not have an xfr here */ WARN_ONCE (1 ,"no xfr in queue" ); -sbefifo_put_client (client); -return -EPROTO ; + ret = -EPROTO ; + goto out; }if (!test_bit (SBEFIFO_XFR_COMPLETE , &xfr->flags)) {
@@ -610,9 +609,11 @@ static ssize_t sbefifo_read_common(struct sbefifo_client *client, } } -sbefifo_put_client (client); + ret = n; -return n; +out: +sbefifo_put_client (client); +return ret; } static ssize_t sbefifo_read(struct file *file, char __user *buf, size_t len,
@@ -689,14 +690,14 @@ static ssize_t sbefifo_write_common(struct sbefifo_client *client,if (mod_timer (&sbefifo->poll_timer, jiffies))sbefifo_put (sbefifo); -sbefifo_put_client (client); -return -ERESTARTSYS ; + ret = -ERESTARTSYS ; + goto out; }if (sbefifo->rc) {INIT_LIST_HEAD (&client->xfrs); -sbefifo_put_client (client); -return sbefifo->rc; + ret = sbefifo->rc; + goto out; } n =min_t (size_t, n, len);
@@ -708,8 +709,8 @@ static ssize_t sbefifo_write_common(struct sbefifo_client *client,sbefifo_get (sbefifo);if (mod_timer (&sbefifo->poll_timer, jiffies))sbefifo_put (sbefifo); -sbefifo_put_client (client); -return -EFAULT ; + ret = -EFAULT ; + goto out; } ubuf += n;
@@ -736,8 +737,8 @@ static ssize_t sbefifo_write_common(struct sbefifo_client *client,sbefifo_put (sbefifo); } +out:sbefifo_put_client (client); -return ret; }
Signature · 1 line
1.8.3.1
PATCH v4 8/31 drivers: fsi: sbefifo: Do an earlier get_client call
In reply to Eddie JamesFrom: "Edward A. James" <[email protected]>
Get client before we start using it.
--- drivers/fsi/fsi-sbefifo.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
drivers/fsi/fsi-sbefifo.c +5 -4
@@ -656,6 +656,7 @@ static ssize_t sbefifo_write_common(struct sbefifo_client *client,if (!len)return 0 ; +sbefifo_get_client (client); n =sbefifo_buf_nbwriteable (&client->wbuf);spin_lock_irq (&sbefifo->lock);
@@ -663,19 +664,19 @@ static ssize_t sbefifo_write_common(struct sbefifo_client *client,if ((client->f_flags &O_NONBLOCK ) && xfr && n < len) {spin_unlock_irq (&sbefifo->lock); -return -EAGAIN ; + ret = -EAGAIN ; + goto out; } xfr =sbefifo_enq_xfr (client);if (!xfr) {spin_unlock_irq (&sbefifo->lock); -return -ENOMEM ; + ret = -ENOMEM ; + goto out; }spin_unlock_irq (&sbefifo->lock); -sbefifo_get_client (client); -/* * Partial writes are not really allowed in that EOT is sent exactly * once per write.
Signature · 1 line
1.8.3.1
PATCH v4 9/31 drivers: fsi: sbefifo: Remove warning and user data access check
In reply to Eddie JamesFrom: "Edward A. James" <[email protected]>
We check for user buffer access when we copy the data. No reason to throw a kernel warning if there is an offset provided; just ignore it.
--- drivers/fsi/fsi-sbefifo.c | 10 ---------- 1 file changed, 10 deletions(-)
drivers/fsi/fsi-sbefifo.c +0 -10
@@ -621,11 +621,6 @@ static ssize_t sbefifo_read(struct file *file, char __user *buf, size_t len, {struct sbefifo_client *client = file->private_data; -WARN_ON (*offset); - -if (!access_ok (VERIFY_WRITE , buf, len)) -return -EFAULT ; -return sbefifo_read_common (client, buf,NULL , len); }
@@ -748,11 +743,6 @@ static ssize_t sbefifo_write(struct file *file, const char __user *buf, {struct sbefifo_client *client = file->private_data; -WARN_ON (*offset); - -if (!access_ok (VERIFY_READ , buf, len)) -return -EFAULT ; -return sbefifo_write_common (client, buf,NULL , len); }
Signature · 1 line
1.8.3.1
PATCH v4 10/31 drivers: fsi: sbefifo: destroy the ida list on exit
In reply to Eddie JamesFrom: "Edward A. James" <[email protected]>
Need to clean this up.
--- drivers/fsi/fsi-sbefifo.c | 2 ++ 1 file changed, 2 insertions(+)
drivers/fsi/fsi-sbefifo.c +2 -0
@@ -947,6 +947,8 @@ static int sbefifo_init(void)static void sbefifo_exit (void ) {fsi_driver_unregister (&sbefifo_drv); + +ida_destroy (&sbefifo_ida); }module_init (sbefifo_init);
Signature · 1 line
1.8.3.1
PATCH v4 11/31 drivers: fsi: sbefifo: Fix module authors and comments
In reply to Eddie JamesFrom: "Edward A. James" <[email protected]>
Clarify the xfr situation in write() and fix module authors
--- drivers/fsi/fsi-sbefifo.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)
drivers/fsi/fsi-sbefifo.c +7 -5
@@ -655,7 +655,7 @@ static ssize_t sbefifo_write_common(struct sbefifo_client *client, n =sbefifo_buf_nbwriteable (&client->wbuf);spin_lock_irq (&sbefifo->lock); - xfr =sbefifo_next_xfr (sbefifo); + xfr =sbefifo_next_xfr (sbefifo);/* next xfr to be executed */ if ((client->f_flags &O_NONBLOCK ) && xfr && n < len) {spin_unlock_irq (&sbefifo->lock);
@@ -663,7 +663,7 @@ static ssize_t sbefifo_write_common(struct sbefifo_client *client, goto out; } - xfr =sbefifo_enq_xfr (client); + xfr =sbefifo_enq_xfr (client);/* this xfr queued up */ if (!xfr) {spin_unlock_irq (&sbefifo->lock); ret = -ENOMEM ;
@@ -719,8 +719,9 @@ static ssize_t sbefifo_write_common(struct sbefifo_client *client, len -= n; ret += n; -/* set flag before starting the worker, as it may run through -* and check the flag before we exit this loop! +/* +* Set this before starting timer to avoid race condition on +* this flag with the timer function writer. */ if (!len)set_bit (SBEFIFO_XFR_WRITE_DONE , &xfr->flags);
@@ -955,4 +956,5 @@ static void sbefifo_exit(void)module_exit (sbefifo_exit);MODULE_LICENSE ("GPL" );MODULE_AUTHOR ("Brad Bishop <[email protected]>" ); -MODULE_DESCRIPTION ("Linux device interface to the POWER self boot engine" ); +MODULE_AUTHOR ("Eddie James <[email protected]>" ); +MODULE_DESCRIPTION ("Linux device interface to the POWER Self Boot Engine" );
Signature · 1 line
1.8.3.1
PATCH v4 12/31 drivers: fsi: sbefifo: Fix include guards in header file
In reply to Eddie JamesFrom: "Edward A. James" <[email protected]>
Make them conform to Linux standard headers.
--- include/linux/fsi-sbefifo.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
include/linux/fsi-sbefifo.h +3 -3
@@ -13,8 +13,8 @@* GNU General Public License for more details. */ -#ifndef __FSI_SBEFIFO_H__ -#define __FSI_SBEFIFO_H__ +#ifndef LINUX_FSI_SBEFIFO_H +#define LINUX_FSI_SBEFIFO_H struct device ;struct sbefifo_client ;
@@ -27,4 +27,4 @@ extern int sbefifo_drv_write(struct sbefifo_client *client, const char *buf, size_t len);extern void sbefifo_drv_release (struct sbefifo_client *client); -#endif /* __FSI_SBEFIFO_H__ */ +#endif /* LINUX_FSI_SBEFIFO_H */
Signature · 1 line
1.8.3.1
PATCH v4 13/31 drivers: fsi: SBEFIFO: Fix probe() and remove()
In reply to Eddie JamesFrom: "Edward A. James" <[email protected]>
Probe didn't handle a failed misc device registration properly. Remove had the potential for hangs. Also, remove the list of SBEFIFOs and instead just use the device "driver data" pointer.
--- drivers/fsi/fsi-sbefifo.c | 111 ++++++++++++++++++++++------------------------ 1 file changed, 53 insertions(+), 58 deletions(-)
drivers/fsi/fsi-sbefifo.c +53 -58
@@ -58,7 +58,6 @@ struct sbefifo {struct fsi_device *fsi_dev;struct miscdevice mdev;wait_queue_head_t wait; -struct list_head link;struct list_head xfrs;struct kref kref;spinlock_t lock;
@@ -96,8 +95,6 @@ struct sbefifo_client {unsigned long f_flags; }; -static struct list_head sbefifo_fifos; -static DEFINE_IDA (sbefifo_ida);static int sbefifo_inw (struct sbefifo *sbefifo,int reg,u32 *word)
@@ -267,9 +264,12 @@ static struct sbefifo_xfr *sbefifo_enq_xfr(struct sbefifo_client *client)struct sbefifo *sbefifo = client->dev;struct sbefifo_xfr *xfr; +if (READ_ONCE (sbefifo->rc)) +return ERR_PTR (sbefifo->rc); + xfr =kzalloc (sizeof (*xfr),GFP_KERNEL );if (!xfr) -return NULL ; +return ERR_PTR (-ENOMEM ); xfr->rbuf = &client->rbuf; xfr->wbuf = &client->wbuf;
@@ -490,7 +490,7 @@ static void sbefifo_poll_timer(unsigned long data) }sbefifo_put (sbefifo); -wake_up (&sbefifo->wait); +wake_up_interruptible (&sbefifo->wait); out_unlock:spin_unlock (&sbefifo->lock);
@@ -605,7 +605,7 @@ static ssize_t sbefifo_read_common(struct sbefifo_client *client, }else {kfree (xfr);list_del (&xfr->client); -wake_up (&sbefifo->wait); +wake_up_interruptible (&sbefifo->wait); } }
@@ -664,9 +664,9 @@ static ssize_t sbefifo_write_common(struct sbefifo_client *client, } xfr =sbefifo_enq_xfr (client);/* this xfr queued up */ -if (!xfr) { +if (IS_ERR (xfr)) {spin_unlock_irq (&sbefifo->lock); - ret = -ENOMEM ; + ret =PTR_ERR (xfr); goto out; }
@@ -769,18 +769,15 @@ static int sbefifo_release(struct inode *inode, struct file *file)struct sbefifo_client *sbefifo_drv_open (struct device *dev,unsigned long flags) { -struct sbefifo_client *client =NULL ; -struct sbefifo *sbefifo; -struct fsi_device *fsi_dev =to_fsi_dev (dev); +struct sbefifo_client *client; +struct sbefifo *sbefifo =dev_get_drvdata (dev); -list_for_each_entry (sbefifo, &sbefifo_fifos, link) { -if (sbefifo->fsi_dev != fsi_dev) -continue ; +if (!sbefifo) +return NULL ; - client =sbefifo_new_client (sbefifo); -if (client) - client->f_flags = flags; - } + client =sbefifo_new_client (sbefifo); +if (client) + client->f_flags = flags;return client; }
@@ -854,69 +851,68 @@ static int sbefifo_probe(struct device *dev)return -EIO ; } - sbefifo->mdev.minor =MISC_DYNAMIC_MINOR ; - sbefifo->mdev.fops = &sbefifo_fops; - sbefifo->mdev.name = sbefifo->name; - sbefifo->mdev.parent = dev;spin_lock_init (&sbefifo->lock);kref_init (&sbefifo->kref); +init_waitqueue_head (&sbefifo->wait); +INIT_LIST_HEAD (&sbefifo->xfrs); sbefifo->idx =ida_simple_get (&sbefifo_ida,1 ,INT_MAX ,GFP_KERNEL );snprintf (sbefifo->name,sizeof (sbefifo->name),"sbefifo%d" , sbefifo->idx); -init_waitqueue_head (&sbefifo->wait); -INIT_LIST_HEAD (&sbefifo->xfrs);/* This bit of silicon doesn't offer any interrupts... */ setup_timer (&sbefifo->poll_timer, sbefifo_poll_timer, (unsigned long )sbefifo); -if (dev->of_node) { -/* create platform devs for dts child nodes (occ, etc) */ -for_each_child_of_node (dev->of_node, np) { -snprintf (child_name,sizeof (child_name),"%s-dev%d" , - sbefifo->name, child_idx++); - child =of_platform_device_create (np, child_name, dev); -if (!child) -dev_warn (dev, -"failed to create child node dev\n" ); - } + sbefifo->mdev.minor =MISC_DYNAMIC_MINOR ; + sbefifo->mdev.fops = &sbefifo_fops; + sbefifo->mdev.name = sbefifo->name; + sbefifo->mdev.parent = dev; + ret =misc_register (&sbefifo->mdev); +if (ret) { +dev_err (dev,"failed to register miscdevice: %d\n" , ret); +ida_simple_remove (&sbefifo_ida, sbefifo->idx); +sbefifo_put (sbefifo); +return ret; } -list_add (&sbefifo->link, &sbefifo_fifos); - -return misc_register (&sbefifo->mdev); +/* create platform devs for dts child nodes (occ, etc) */ +for_each_available_child_of_node (dev->of_node, np) { +snprintf (child_name,sizeof (child_name),"%s-dev%d" , + sbefifo->name, child_idx++); + child =of_platform_device_create (np, child_name, dev); +if (!child) +dev_warn (dev,"failed to create child %s dev\n" , + child_name); + } + +dev_set_drvdata (dev, sbefifo); + +return 0 ; }static int sbefifo_remove (struct device *dev) { -struct fsi_device *fsi_dev =to_fsi_dev (dev); -struct sbefifo *sbefifo, *sbefifo_tmp; +struct sbefifo *sbefifo =dev_get_drvdata (dev);struct sbefifo_xfr *xfr; -list_for_each_entry_safe (sbefifo, sbefifo_tmp, &sbefifo_fifos, link) { -if (sbefifo->fsi_dev != fsi_dev) -continue ; +WRITE_ONCE (sbefifo->rc, -ENODEV ); +wake_up_all (&sbefifo->wait); -device_for_each_child (dev,NULL , sbefifo_unregister_child); +misc_deregister (&sbefifo->mdev); +device_for_each_child (dev,NULL , sbefifo_unregister_child); -misc_deregister (&sbefifo->mdev); -list_del (&sbefifo->link); -ida_simple_remove (&sbefifo_ida, sbefifo->idx); - -if (del_timer_sync (&sbefifo->poll_timer)) -sbefifo_put (sbefifo); +ida_simple_remove (&sbefifo_ida, sbefifo->idx); -spin_lock (&sbefifo->lock); -list_for_each_entry (xfr, &sbefifo->xfrs, xfrs) -kfree (xfr); -spin_unlock (&sbefifo->lock); +if (del_timer_sync (&sbefifo->poll_timer)) +sbefifo_put (sbefifo); -WRITE_ONCE (sbefifo->rc, -ENODEV ); +spin_lock (&sbefifo->lock); +list_for_each_entry (xfr, &sbefifo->xfrs, xfrs) +kfree (xfr); +spin_unlock (&sbefifo->lock); -wake_up (&sbefifo->wait); -sbefifo_put (sbefifo); - } +sbefifo_put (sbefifo);return 0 ; }
@@ -941,7 +937,6 @@ static int sbefifo_remove(struct device *dev)static int sbefifo_init (void ) { -INIT_LIST_HEAD (&sbefifo_fifos);return fsi_driver_register (&sbefifo_drv); }
Signature · 1 line
1.8.3.1
PATCH v4 14/31 drivers: fsi: SBEFIFO: check for xfr complete in read wait_event
In reply to Eddie JamesFrom: "Edward A. James" <[email protected]>
We should check to see if the XFR is complete, not just for a failure or for available data. If we hit EOT without getting more data, we may wait forever.
--- drivers/fsi/fsi-sbefifo.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
drivers/fsi/fsi-sbefifo.c +6 -1
@@ -541,10 +541,15 @@ static bool sbefifo_read_ready(struct sbefifo *sbefifo, struct sbefifo_client *client, size_t *n, size_t *ret) { + struct sbefifo_xfr *xfr = list_first_entry_or_null(&client->xfrs, + struct sbefifo_xfr, + client); + *n =sbefifo_buf_nbreadable (&client->rbuf); *ret =READ_ONCE (sbefifo->rc); -return *ret || *n; +return *ret || *n || + (xfr &&test_bit (SBEFIFO_XFR_COMPLETE , &xfr->flags)); } static ssize_t sbefifo_read_common(struct sbefifo_client *client,
Signature · 1 line
1.8.3.1
PATCH v4 15/31 drivers: fsi: occ: Fix includes
In reply to Eddie JamesFrom: "Edward A. James" <[email protected]>
Need to include everything we use and not rely on recursive includes.
--- drivers/fsi/occ.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
drivers/fsi/occ.c +6 -1
@@ -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>
Signature · 1 line
1.8.3.1
PATCH v4 16/31 drivers: fsi: occ: Fix errant kfree calls
In reply to Eddie JamesFrom: "Edward A. James" <[email protected]>
Left over from when xfr was an independant structure. Remove these calls as xfr is a part of the client struct.
--- drivers/fsi/occ.c | 2 -- 1 file changed, 2 deletions(-)
drivers/fsi/occ.c +0 -2
@@ -289,7 +289,6 @@ static ssize_t occ_write_common(struct occ_client *client,if (ubuf) {if (copy_from_user (&xfr->buf[1 ], ubuf, len)) { -kfree (xfr); rc = -EFAULT ; goto done; }
@@ -298,7 +297,6 @@ static ssize_t occ_write_common(struct occ_client *client, data_length = (xfr->buf[2 ] <<8 ) + xfr->buf[3 ];if (data_length >OCC_CMD_DATA_BYTES ) { -kfree (xfr); rc = -EINVAL ; goto done; }
Signature · 1 line
1.8.3.1
PATCH v4 17/31 drivers: fsi: occ: remove unused occ_command structure
In reply to Eddie JamesFrom: "Edward A. James" <[email protected]>
--- drivers/fsi/occ.c | 8 -------- 1 file changed, 8 deletions(-)
drivers/fsi/occ.c +0 -8
@@ -46,14 +46,6 @@ struct occ {#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;
Signature · 1 line
1.8.3.1
PATCH v4 18/31 drivers: fsi: occ: Use big-endian values
In reply to Eddie JamesFrom: "Edward A. James" <[email protected]>
Switch to __be16 from u16 and __be32 from u32. Also, use kernel access of unaligned be16 instead of manually creating a big-endian value from the occ response buffer.
--- drivers/fsi/occ.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)
drivers/fsi/occ.c +8 -6
@@ -50,9 +50,9 @@ 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;/*
@@ -425,7 +425,7 @@ static int occ_getsram(struct device *sbefifo, u32 address, u8 *data, {int rc;u8 *resp; -u32 buf[5 ]; +__be32 buf[5 ];u32 data_len = ((len +7 ) /8 ) *8 ;struct sbefifo_client *client;
@@ -476,7 +476,7 @@ static int occ_putsram(struct device *sbefifo, u32 address, u8 *data, ssize_t len) {int rc; -u32 *buf; +__be32 *buf;u32 data_len = ((len +7 ) /8 ) *8 ;size_t cmd_len = data_len +20 ;struct sbefifo_client *client;
@@ -522,7 +522,7 @@ 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; buf[0 ] =cpu_to_be32 (0x6 );
@@ -560,6 +560,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;
@@ -573,6 +574,7 @@ 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);
@@ -591,7 +593,7 @@ static void occ_worker(struct work_struct *work)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 ; goto done;
Signature · 1 line
1.8.3.1
PATCH v4 19/31 drivers: fsi: occ: Return ENODEV if client is NULL
In reply to Eddie JamesFrom: "Edward A. James" <[email protected]>
Safeguard the access functions.
--- drivers/fsi/occ.c | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-)
drivers/fsi/occ.c +20 -4
@@ -168,11 +168,16 @@ 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_xfr *xfr; + +if (!client) +return -ENODEV ;if (len >OCC_SRAM_BYTES )return -EINVAL ; + xfr = &client->xfr; +spin_lock_irq (&client->lock);if (!test_bit (CLIENT_XFR_PENDING , &client->flags)) {
@@ -263,11 +268,16 @@ static ssize_t occ_write_common(struct occ_client *client,int rc;unsigned int i;u16 data_length, checksum =0 ; -struct occ_xfr *xfr = &client->xfr; +struct occ_xfr *xfr; + +if (!client) +return -ENODEV ;if (len > (OCC_CMD_DATA_BYTES +3 ) || len <3 )return -EINVAL ; + xfr = &client->xfr; +spin_lock_irq (&client->lock);if (test_and_set_bit (CLIENT_XFR_PENDING , &client->flags)) { rc = -EBUSY ;
@@ -327,8 +337,14 @@ static ssize_t occ_write(struct file *file, const char __user *buf,static int occ_release_common (struct occ_client *client) { -struct occ_xfr *xfr = &client->xfr; -struct occ *occ = client->occ; +struct occ *occ; +struct occ_xfr *xfr; + +if (!client) +return -ENODEV ; + + xfr = &client->xfr; + occ = client->occ;spin_lock_irq (&client->lock);
Signature · 1 line
1.8.3.1
Re: [PATCH linux dev-4.10 v4 19/31] drivers: fsi: occ: Return ENODEV if client is NULL
Quoted 5 lines · Eddie James
From: "Edward A. James" <[email protected]>
Safeguard the access functions.Signed-off-by Edward A. James <[email protected]>
Quoted 61 lines
---
drivers/fsi/occ.c | 24 ++++++++++++++++++++----1 file changed, 20 insertions(+), 4 deletions(-)drivers/fsi/occ.c +4
-1@@ -168,11 +168,16 @@ 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_xfr *xfr; + +if (!client) +return -ENODEV ;
if (len >OCC_SRAM_BYTES)
return-EINVAL;drivers/fsi/occ.c +2
-0+ xfr = &client->xfr; +
spin_lock_irq(&client->lock);
if (!test_bit(CLIENT_XFR_PENDING,&client->flags)) {drivers/fsi/occ.c +4
-1@@ -263,11 +268,16 @@ static ssize_t occ_write_common(struct occ_client *client, int rc; unsigned int i; u16 data_length, checksum = 0; -struct occ_xfr *xfr = &client->xfr; +struct occ_xfr *xfr; + +if (!client) +return -ENODEV ;
if (len > (OCC_CMD_DATA_BYTES+ 3) || len < 3) return-EINVAL;drivers/fsi/occ.c +2
-0+ xfr = &client->xfr; +
spin_lock_irq(&client->lock);
if (test_and_set_bit(CLIENT_XFR_PENDING, &client->flags)) { rc =-EBUSY;drivers/fsi/occ.c +0
-0@@ -327,8 +337,14 @@ static ssize_t occ_write(struct file *file, const char __user *buf,
static intocc_release_common(struct occ_client *client){drivers/fsi/occ.c +8
-2-struct occ_xfr *xfr = &client->xfr; -struct occ *occ = client->occ; +struct occ *occ; +struct occ_xfr *xfr; + +if (!client) +return -ENODEV ; + + xfr = &client->xfr; + occ = client->occ;
spin_lock_irq(&client->lock);
PATCH v4 20/31 drivers: fsi: occ: Remove early user buffer checking
In reply to Eddie JamesFrom: "Edward A. James" <[email protected]>
We check user buffer access when we copy the data in read()/write() anyway.
--- drivers/fsi/occ.c | 12 ------------ 1 file changed, 12 deletions(-)
drivers/fsi/occ.c +0 -12
@@ -252,12 +252,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); }
@@ -326,12 +320,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); }
Signature · 1 line
1.8.3.1
PATCH v4 21/31 drivers: fsi: occ: Switch to more logical errnos
In reply to Eddie JamesFrom: "Edward A. James" <[email protected]>
--- drivers/fsi/occ.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
drivers/fsi/occ.c +6 -6
@@ -402,7 +402,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,
@@ -421,7 +421,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,
@@ -466,7 +466,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);
@@ -514,7 +514,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);
@@ -551,7 +551,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);
@@ -599,7 +599,7 @@ static void occ_worker(struct work_struct *work) resp_data_length =get_unaligned_be16 (&resp->data_length);if (resp_data_length >OCC_RESP_DATA_BYTES ) { - rc = -EDOM ; + rc = -EMSGSIZE ; goto done; }
Signature · 1 line
1.8.3.1
PATCH v4 22/31 drivers: fsi: occ: fix white space and bracket problems
In reply to Eddie JamesFrom: "Edward A. James" <[email protected]>
For better flow and to meet Linux standards.
--- drivers/fsi/occ.c | 28 ++++++++++++++++++---------- 1 file changed, 18 insertions(+), 10 deletions(-)
drivers/fsi/occ.c +18 -10
@@ -132,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 ;
@@ -163,6 +162,12 @@ static int occ_open(struct inode *inode, struct file *file)return 0 ; } +static inline bool occ_read_ready (struct occ_xfr *xfr) +{ +return test_bit (XFR_COMPLETE , &xfr->flags) || +test_bit (XFR_CANCELED , &xfr->flags); +} +static ssize_t occ_read_common (struct occ_client *client,char __user *ubuf,char *kbuf,size_t len) {
@@ -185,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; }
@@ -202,8 +208,7 @@ 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)); +occ_read_ready (xfr));spin_lock_irq (&client->lock);
@@ -227,12 +232,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;
@@ -273,6 +280,7 @@ static ssize_t occ_write_common(struct occ_client *client, xfr = &client->xfr;spin_lock_irq (&client->lock); +if (test_and_set_bit (CLIENT_XFR_PENDING , &client->flags)) { rc = -EBUSY ; goto done;
@@ -280,7 +288,6 @@ static ssize_t occ_write_common(struct occ_client *client,/* clear out the transfer */ memset (xfr,0 ,sizeof (*xfr)); - xfr->buf[0 ] =1 ;if (ubuf) {
@@ -288,8 +295,9 @@ static ssize_t occ_write_common(struct occ_client *client, 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 ) {
@@ -446,7 +454,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 ;
Signature · 1 line
1.8.3.1
Re: [PATCH linux dev-4.10 v4 22/31] drivers: fsi: occ: fix white space and bracket problems
Quoted 5 lines · Eddie James
From: "Edward A. James" <[email protected]>
For better flow and to meet Linux standards.Signed-off-by Edward A. James <[email protected]>
Quoted 106 lines
---
drivers/fsi/occ.c | 28 ++++++++++++++++++----------1 file changed, 18 insertions(+), 10 deletions(-)drivers/fsi/occ.c +0
-0@@ -132,9 +132,8 @@ static void occ_enqueue_xfr(struct occ_xfr *xfr)
static structocc_client*occ_open_common(struct occ *occ, unsigned long flags) {drivers/fsi/occ.c +1
-1-struct occ_client *client; +struct occ_client *client =kzalloc (sizeof (*client),GFP_KERNEL );drivers/fsi/occ.c +0
-1- client =kzalloc (sizeof (*client),GFP_KERNEL );if (!client)
return NULL;drivers/fsi/occ.c +0
-0@@ -163,6 +162,12 @@ static int occ_open(struct inode *inode, struct file *file) return 0; }drivers/fsi/occ.c +6
-0+static inline bool occ_read_ready (struct occ_xfr *xfr) +{ +return test_bit (XFR_COMPLETE , &xfr->flags) || +test_bit (XFR_CANCELED , &xfr->flags); +} +static
ssize_tocc_read_common(struct occ_client *client, char __user *ubuf, char *kbuf, size_t len)
{drivers/fsi/occ.c +2
-1@@ -185,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;
}drivers/fsi/occ.c +0
-0@@ -202,8 +208,7 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf, spin_unlock_irq(&client->lock);
rc = wait_event_interruptible(client->wait,drivers/fsi/occ.c +1
-2-test_bit (XFR_COMPLETE , &xfr->flags) || - test_bit(XFR_CANCELED, &xfr->flags)); + occ_read_ready(xfr));
spin_lock_irq(&client->lock);drivers/fsi/occ.c +0
-0@@ -227,12 +232,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) {drivers/fsi/occ.c +2
-1-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;
}drivers/fsi/occ.c +1
-1- }else + }else {
memcpy(kbuf, &xfr->buf[client->read_offset], bytes);drivers/fsi/occ.c +1
-0+ }
client->read_offset+= bytes;drivers/fsi/occ.c +0
-0@@ -273,6 +280,7 @@ static ssize_t occ_write_common(struct occ_client *client, xfr = &client->xfr;
spin_lock_irq(&client->lock);
+
if (test_and_set_bit(CLIENT_XFR_PENDING, &client->flags)) { rc =-EBUSY;
goto done;drivers/fsi/occ.c +0
-0@@ -280,7 +288,6 @@ static ssize_t occ_write_common(struct occ_client *client,
/* clear out the transfer */
memset(xfr, 0, sizeof(*xfr));
-
xfr->buf[0] = 1;
if (ubuf) {drivers/fsi/occ.c +2
-1@@ -288,8 +295,9 @@ static ssize_t occ_write_common(struct occ_client *client, 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) {drivers/fsi/occ.c +1
-1@@ -446,7 +454,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;
PATCH v4 23/31 drivers: fsi: occ: Destroy the ida list on exit
In reply to Eddie JamesFrom: "Edward A. James" <[email protected]>
Need to clean up the index list structure.
--- drivers/fsi/occ.c | 2 ++ 1 file changed, 2 insertions(+)
drivers/fsi/occ.c +2 -0
@@ -798,6 +798,8 @@ static void occ_exit(void)destroy_workqueue (occ_wq);platform_driver_unregister (&occ_driver); + +ida_destroy (&occ_ida); }module_init (occ_init);
Signature · 1 line
1.8.3.1
PATCH v4 24/31 drivers: fsi: occ: Remove unnecessary platform_set_drvdata call
In reply to Eddie JamesFrom: "Edward A. James" <[email protected]>
The purpose was to set that pointer null before probing. However, it is set null by the kernel by default.
--- drivers/fsi/occ.c | 3 --- 1 file changed, 3 deletions(-)
drivers/fsi/occ.c +0 -3
@@ -713,9 +713,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) {
Signature · 1 line
1.8.3.1
PATCH v4 25/31 drivers: fsi: occ: Add comments for clarity
In reply to Eddie JamesFrom: "Edward A. James" <[email protected]>
Fixup some existing comments as well.
--- drivers/fsi/occ.c | 55 +++++++++++++++++++++++++++++++++++++------------------ 1 file changed, 37 insertions(+), 18 deletions(-)
drivers/fsi/occ.c +37 -18
@@ -39,8 +39,8 @@ 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; };
@@ -57,17 +57,17 @@ struct occ_response {/* * 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 ,
@@ -89,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 ,
@@ -101,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;
@@ -286,10 +286,15 @@ static ssize_t occ_write_common(struct occ_client *client, 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)) { rc = -EFAULT ;
@@ -373,7 +378,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 ;
@@ -438,9 +443,13 @@ static int occ_getsram(struct device *sbefifo, u32 address, u8 *data,int rc;u8 *resp;__be32 buf[5 ]; -u32 data_len = ((len +7 ) /8 ) *8 ; +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 );
@@ -489,7 +498,7 @@ static int occ_putsram(struct device *sbefifo, u32 address, u8 *data, {int rc;__be32 *buf; -u32 data_len = ((len +7 ) /8 ) *8 ; +u32 data_len = ((len +7 ) /8 ) *8 ;/* must be multiples of 8 B */ size_t cmd_len = data_len +20 ;struct sbefifo_client *client;
@@ -497,6 +506,10 @@ 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 );
@@ -537,11 +550,15 @@ static int occ_trigger_attn(struct device *sbefifo)__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 );
@@ -592,6 +609,7 @@ static void occ_worker(struct work_struct *work)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)
@@ -601,6 +619,7 @@ 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;
Signature · 1 line
1.8.3.1
PATCH v4 26/31 drivers: fsi: occ: Add OCC response definitions to header
In reply to Eddie JamesFrom: "Edward A. James" <[email protected]>
Also fix include guards.
--- include/linux/occ.h | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-)
include/linux/occ.h +17 -3
@@ -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 */
Signature · 1 line
1.8.3.1
PATCH v4 27/31 drivers: fsi: occ: Poll while receiving "command in progress"
In reply to Eddie JamesFrom: "Edward A. James" <[email protected]>
Waiting for OCC to return something other than "command in progress" should only be done in the worker function, instead of clients repeating the entire transfer when they receive "command in progress." In this way clients don't have to check for that return status.
--- drivers/fsi/occ.c | 27 ++++++++++++++++++++++++--- 1 file changed, 24 insertions(+), 3 deletions(-)
drivers/fsi/occ.c +24 -3
@@ -23,6 +23,7 @@#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>
@@ -33,6 +34,9 @@#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 ];
@@ -588,6 +592,9 @@ 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;
@@ -609,6 +616,8 @@ static void occ_worker(struct work_struct *work)spin_unlock_irq (&occ->list_lock);mutex_lock (&occ->occ_lock); + start = jiffies; +/* write occ command */ rc =occ_putsram (sbefifo,0xFFFBE000 , xfr->buf, xfr->cmd_data_length);
@@ -620,9 +629,21 @@ static void occ_worker(struct work_struct *work) goto done;/* read occ response */ - rc =occ_getsram (sbefifo,0xFFFBF000 , xfr->buf,8 ); -if (rc) - goto done; +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 ; + +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 ) {
Signature · 1 line
1.8.3.1
PATCH v4 28/31 drivers: fsi: occ: Add cancel to remove() and fix probe()
In reply to Eddie JamesFrom: "Edward A. James" <[email protected]>
Need some data to indicate to clients and the rest of the driver when the device is being removed, so add a cancel boolean. Fix up both the probe and remove functions to properly handle failures and prevent deadlocks.
--- drivers/fsi/occ.c | 71 +++++++++++++++++++++++++++++++++++++++---------------- 1 file changed, 50 insertions(+), 21 deletions(-)
drivers/fsi/occ.c +50 -21
@@ -46,6 +46,7 @@ struct occ {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)
@@ -117,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);
@@ -132,6 +136,8 @@ 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)
@@ -166,10 +172,10 @@ static int occ_open(struct inode *inode, struct file *file)return 0 ; } -static inline bool occ_read_ready (struct occ_xfr *xfr) +static inline bool occ_read_ready (struct occ_xfr *xfr,struct occ *occ) {return test_bit (XFR_COMPLETE , &xfr->flags) || -test_bit (XFR_CANCELED , &xfr->flags); +test_bit (XFR_CANCELED , &xfr->flags) || occ->cancel; } static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
@@ -178,6 +184,7 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,int rc;size_t bytes;struct occ_xfr *xfr; +struct occ *occ;if (!client)return -ENODEV ;
@@ -186,6 +193,7 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,return -EINVAL ; xfr = &client->xfr; + occ = client->occ;spin_lock_irq (&client->lock);
@@ -212,7 +220,7 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,spin_unlock_irq (&client->lock); rc =wait_event_interruptible (client->wait, -occ_read_ready (xfr)); +occ_read_ready (xfr, occ));spin_lock_irq (&client->lock);
@@ -285,7 +293,7 @@ static ssize_t occ_write_common(struct occ_client *client,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; }
@@ -323,8 +331,11 @@ static ssize_t occ_write_common(struct occ_client *client, xfr->cmd_data_length = data_length +6 ; client->read_offset =0 ; -occ_enqueue_xfr (xfr); + rc =occ_enqueue_xfr (xfr); +if (rc) + goto done; +set_bit (CLIENT_XFR_PENDING , &client->flags); rc = len; done:
@@ -602,6 +613,9 @@ static void occ_worker(struct work_struct *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);
@@ -762,23 +776,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;
@@ -788,20 +792,45 @@ 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; + +spin_lock_irq (&occ->list_lock); +list_for_each_entry (xfr, &occ->xfrs, link) { + client =to_client (xfr); +wake_up_all (&client->wait); + } +spin_unlock_irq (&occ->list_lock); -flush_work (&occ->work);misc_deregister (&occ->mdev);device_for_each_child (&pdev->dev,NULL , occ_unregister_child); + +cancel_work_sync (&occ->work); +ida_simple_remove (&occ_ida, occ->idx);return 0 ;
Signature · 1 line
1.8.3.1
Re: [PATCH linux dev-4.10 v4 28/31] drivers: fsi: occ: Add cancel to remove() and fix probe()
Quoted 8 lines · Eddie James
From: "Edward A. James" <[email protected]>
Need some data to indicate to clients and the rest of the driver when the device is being removed, so add a cancel boolean. Fix up both the probe and remove functions to properly handle failures and prevent deadlocks.Signed-off-by Edward A. James <[email protected]>
Quoted 187 lines
---
drivers/fsi/occ.c | 71 +++++++++++++++++++++++++++++++++++++++----------------1 file changed, 50 insertions(+), 21 deletions(-)drivers/fsi/occ.c +0
-0@@ -46,6 +46,7 @@ struct occ {
spinlock_tlist_lock; /* lock access to the xfrs list */
struct mutexocc_lock; /* lock access to the hardware */
structwork_structwork;drivers/fsi/occ.c +1
-0+bool cancel;};
#defineto_occ(x)container_of((x), struct occ, mdev)drivers/fsi/occ.c +0
-0@@ -117,12 +118,15 @@ struct occ_client {
staticDEFINE_IDA(occ_ida);drivers/fsi/occ.c +1
-1-static void occ_enqueue_xfr (struct occ_xfr *xfr) +static int occ_enqueue_xfr (struct occ_xfr *xfr){
int empty;
structocc_client*client =to_client(xfr); struct occ *occ =client->occ;drivers/fsi/occ.c +3
-0+if (occ->cancel) +return -ECANCELED ; +
spin_lock_irq(&occ->list_lock);
empty =list_empty(&occ->xfrs);drivers/fsi/occ.c +0
-0@@ -132,6 +136,8 @@ static void occ_enqueue_xfr(struct occ_xfr *xfr)
if (empty)
queue_work(occ_wq, &occ->work);drivers/fsi/occ.c +2
-0+ +return 0 ;}
static structocc_client*occ_open_common(struct occ *occ, unsigned long flags)drivers/fsi/occ.c +0
-0@@ -166,10 +172,10 @@ static int occ_open(struct inode *inode, struct file *file) return 0; }drivers/fsi/occ.c +1
-1-static inline bool occ_read_ready (struct occ_xfr *xfr) +static inline bool occ_read_ready (struct occ_xfr *xfr,struct occ *occ){
returntest_bit(XFR_COMPLETE, &xfr->flags)||drivers/fsi/occ.c +1
-1-test_bit (XFR_CANCELED , &xfr->flags); +test_bit (XFR_CANCELED , &xfr->flags) || occ->cancel;}
staticssize_tocc_read_common(structocc_client*client, char__user*ubuf,drivers/fsi/occ.c +1
-0@@ -178,6 +184,7 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf, int rc; size_t bytes; struct occ_xfr *xfr; +struct occ *occ;
if (!client)
return-ENODEV;drivers/fsi/occ.c +0
-0@@ -186,6 +193,7 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf, return -EINVAL;
xfr =&client->xfr;drivers/fsi/occ.c +1
-0+ occ = client->occ;
spin_lock_irq(&client->lock);drivers/fsi/occ.c +0
-0@@ -212,7 +220,7 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf, spin_unlock_irq(&client->lock);
rc = wait_event_interruptible(client->wait,drivers/fsi/occ.c +1
-1- occ_read_ready(xfr)); + occ_read_ready(xfr, occ));
spin_lock_irq(&client->lock);drivers/fsi/occ.c +0
-0@@ -285,7 +293,7 @@ static ssize_t occ_write_common(struct occ_client *client,
spin_lock_irq(&client->lock);drivers/fsi/occ.c +1
-1-if (test_and_set_bit (CLIENT_XFR_PENDING , &client->flags)) { +if (test_bit (CLIENT_XFR_PENDING , &client->flags)) {rc =
-EBUSY;
goto done;
}drivers/fsi/occ.c +0
-0@@ -323,8 +331,11 @@ static ssize_t occ_write_common(struct occ_client *client, xfr->cmd_data_length = data_length + 6; client->read_offset = 0;drivers/fsi/occ.c +3
-1-occ_enqueue_xfr (xfr); + rc =occ_enqueue_xfr (xfr); +if (rc) + goto done;drivers/fsi/occ.c +1
-0+set_bit (CLIENT_XFR_PENDING , &client->flags);rc = len;
done:drivers/fsi/occ.c +0
-0@@ -602,6 +613,9 @@ static void occ_worker(struct work_struct *work) struct device *sbefifo = occ->sbefifo;
again:drivers/fsi/occ.c +3
-0+if (occ->cancel) +return ; +
spin_lock_irq(&occ->list_lock);
xfr =list_first_entry_or_null(&occ->xfrs, struct occ_xfr, link);drivers/fsi/occ.c +3
-13@@ -762,23 +776,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;drivers/fsi/occ.c +0
-0@@ -788,20 +792,45 @@ static int occ_probe(struct platform_device *pdev)
rc =misc_register(&occ->mdev);
if (rc) {drivers/fsi/occ.c +2
-1-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;
}drivers/fsi/occ.c +11
-0+/* 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 intocc_remove(struct platform_device *pdev){
struct occ *occ =platform_get_drvdata(pdev);drivers/fsi/occ.c +11
-0+struct occ_xfr *xfr; +struct occ_client *client; + + occ->cancel = true; + +spin_lock_irq (&occ->list_lock); +list_for_each_entry (xfr, &occ->xfrs, link) { + client =to_client (xfr); +wake_up_all (&client->wait); + } +spin_unlock_irq (&occ->list_lock);drivers/fsi/occ.c +0
-1-flush_work (&occ->work);
misc_deregister(&occ->mdev);device_for_each_child(&pdev->dev, NULL, occ_unregister_child);drivers/fsi/occ.c +3
-0+ +cancel_work_sync (&occ->work); +
ida_simple_remove(&occ_ida, occ->idx);
return 0;
Re: [PATCH linux dev-4.10 v4 28/31] drivers: fsi: occ: Add cancel to remove() and fix probe()
In reply to Eddie JamesQuoted 30 lines · Eddie James
From: "Edward A. James" <[email protected]>
Need some data to indicate to clients and the rest of the driver when the device is being removed, so add a cancel boolean. Fix up both the probe and remove functions to properly handle failures and prevent deadlocks.
Signed-off-by Edward A. James <[email protected]>---
drivers/fsi/occ.c| 71 +++++++++++++++++++++++++++++++++++++++----------------1 file changed, 50 insertions(+), 21 deletions(-)drivers/fsi/occ.c +1
-0@@ -46,6 +46,7 @@ struct occ {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)@@ -117,12 +118,15 @@ struct occ_client {static DEFINE_IDA (occ_ida);
drivers/fsi/occ.c struct occ_client { · line 128
Quoted 10 lines
drivers/fsi/occ.c +3
-1-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 ;
Can we have ENODEV here? That seems to be typical for this scenario and ECANCELED seems to be for canceled AIO operations.
Quoted 160 lines
drivers/fsi/occ.c +13
-5+spin_lock_irq (&occ->list_lock); empty =list_empty (&occ->xfrs);@@ -132,6 +136,8 @@ 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)@@ -166,10 +172,10 @@ static int occ_open(struct inode *inode, struct file *file)return 0 ; } -static inline bool occ_read_ready (struct occ_xfr *xfr) +static inline bool occ_read_ready (struct occ_xfr *xfr,struct occ *occ) {return test_bit (XFR_COMPLETE , &xfr->flags) || -test_bit (XFR_CANCELED , &xfr->flags); +test_bit (XFR_CANCELED , &xfr->flags) || occ->cancel; } static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,@@ -178,6 +184,7 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,int rc;size_t bytes;struct occ_xfr *xfr; +struct occ *occ;if (!client)return -ENODEV ;@@ -186,6 +193,7 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,return -EINVAL ; xfr = &client->xfr; + occ = client->occ;spin_lock_irq (&client->lock);@@ -212,7 +220,7 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,spin_unlock_irq (&client->lock); rc =wait_event_interruptible (client->wait, -occ_read_ready (xfr)); +occ_read_ready (xfr, occ));spin_lock_irq (&client->lock);@@ -285,7 +293,7 @@ static ssize_t occ_write_common(struct occ_client *client,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; }@@ -323,8 +331,11 @@ static ssize_t occ_write_common(struct occ_client *client, xfr->cmd_data_length = data_length +6 ; client->read_offset =0 ; -occ_enqueue_xfr (xfr); + rc =occ_enqueue_xfr (xfr); +if (rc) + goto done; +set_bit (CLIENT_XFR_PENDING , &client->flags); rc = len;done:
drivers/fsi/occ.c +0
-0@@ -602,6 +613,9 @@ static void occ_worker(struct work_struct *work)struct device *sbefifo = occ->sbefifo;again:
drivers/fsi/occ.c +3
-0+if (occ->cancel) +return ; +spin_lock_irq(&occ->list_lock); xfr = list_first_entry_or_null(&occ->xfrs, struct occ_xfr, link);drivers/fsi/occ.c +30
-15@@ -762,23 +776,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;@@ -788,20 +792,45 @@ 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; + +spin_lock_irq (&occ->list_lock); +list_for_each_entry (xfr, &occ->xfrs, link) { + client =to_client (xfr); +wake_up_all (&client->wait); + } +spin_unlock_irq (&occ->list_lock); -flush_work (&occ->work);misc_deregister (&occ->mdev);device_for_each_child (&pdev->dev,NULL , occ_unregister_child); + +cancel_work_sync (&occ->work); +ida_simple_remove (&occ_ida, occ->idx);return 0 ;Signature · 1 line
1.8.3.1
Re: [PATCH linux dev-4.10 v4 28/31] drivers: fsi: occ: Add cancel to remove() and fix probe()
Quoted 9 lines · Brad Bishop
Quoted 9 lines · Eddie James
drivers/fsi/occ.c +1
-1-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;
Quoted 5 lines
drivers/fsi/occ.c struct occ_client { · line 128
Quoted 2 lines
drivers/fsi/occ.c +2
-0+if (occ->cancel) +return -ECANCELED ;Can we have ENODEV here? That seems to be typical for this scenario and ECANCELED seems to be for canceled AIO operations.
I agree.
Eddie, I will fold this change in when I merge if you're okay with it?
Cheers,
Joel
Re: [PATCH linux dev-4.10 v4 28/31] drivers: fsi: occ: Add cancel to remove() and fix probe()
Quoted 10 lines · Joel Stanley
Quoted 8 lines · Brad Bishop
Quoted 8 lines · Eddie James
drivers/fsi/occ.c +1
-1-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;
Quoted 7 lines
Quoted 4 lines
drivers/fsi/occ.c struct occ_client { · line 128
Quoted 2 lines
drivers/fsi/occ.c +2
-0+if (occ->cancel) +return -ECANCELED ;Can we have ENODEV here? That seems to be typical for this scenario and ECANCELED seems to be for canceled AIO operations.
I agree.
Eddie, I will fold this change in when I merge if you're okay with it?
Sounds good! Thanks
Eddie
Quoted 5 lines
Cheers,
Joel
PATCH v4 29/31 drivers: fsi: occ: Fix client memory management
In reply to Eddie JamesFrom: "Edward A. James" <[email protected]>
Potential for bad memory access in the worker function. Now fixed by using reference counters.
--- drivers/fsi/occ.c | 92 ++++++++++++++++++++++++++----------------------------- 1 file changed, 43 insertions(+), 49 deletions(-)
drivers/fsi/occ.c +43 -49
@@ -70,15 +70,11 @@ struct occ_response {* 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 -* O_NONBLOCK wasn't specified. Cleared in read() when transfer completes or -* fails. */ enum {XFR_IN_PROGRESS ,XFR_COMPLETE ,XFR_CANCELED , -XFR_WAITING , };struct occ_xfr {
@@ -104,6 +100,7 @@ enum { };struct occ_client { +struct kref kref;struct occ *occ;struct occ_xfr xfr;spinlock_t lock;/* lock access to the client state */
@@ -140,6 +137,24 @@ static int occ_enqueue_xfr(struct occ_xfr *xfr)return 0 ; } +static void occ_get_client (struct occ_client *client) +{ +kref_get (&client->kref); +} + +static void occ_client_release (struct kref *kref) +{ + struct occ_client *client = container_of(kref, struct occ_client, + kref); + +kfree (client); +} + +static void occ_put_client (struct occ_client *client) +{ +kref_put (&client->kref, occ_client_release); +} +static struct occ_client *occ_open_common (struct occ *occ,unsigned long flags) {struct occ_client *client =kzalloc (sizeof (*client),GFP_KERNEL );
@@ -148,6 +163,7 @@ static struct occ_client *occ_open_common(struct occ *occ, unsigned long flags)return NULL ; client->occ = occ; +kref_init (&client->kref);spin_lock_init (&client->lock);init_waitqueue_head (&client->wait);
@@ -192,6 +208,7 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,if (len >OCC_SRAM_BYTES )return -EINVAL ; +occ_get_client (client); xfr = &client->xfr; occ = client->occ;
@@ -215,8 +232,6 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf, goto done; } -set_bit (XFR_WAITING , &xfr->flags); -spin_unlock_irq (&client->lock); rc = wait_event_interruptible(client->wait,
@@ -224,15 +239,12 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,spin_lock_irq (&client->lock); -if (test_bit (XFR_CANCELED , &xfr->flags)) { -spin_unlock_irq (&client->lock); -kfree (client); -return -EBADFD ; - } - -clear_bit (XFR_WAITING , &xfr->flags);if (!test_bit (XFR_COMPLETE , &xfr->flags)) { - rc = -EINTR ; +if (occ->cancel ||test_bit (XFR_CANCELED , &xfr->flags)) + rc = -ECANCELED ; +else + rc = -EINTR ; + goto done; } }
@@ -263,6 +275,7 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf, done:spin_unlock_irq (&client->lock); +occ_put_client (client);return rc; }
@@ -289,6 +302,7 @@ static ssize_t occ_write_common(struct occ_client *client,if (len > (OCC_CMD_DATA_BYTES +3 ) || len <3 )return -EINVAL ; +occ_get_client (client); xfr = &client->xfr;spin_lock_irq (&client->lock);
@@ -340,6 +354,7 @@ static ssize_t occ_write_common(struct occ_client *client, done:spin_unlock_irq (&client->lock); +occ_put_client (client);return rc; }
@@ -364,38 +379,26 @@ static int occ_release_common(struct occ_client *client)spin_lock_irq (&client->lock); -if (!test_bit (CLIENT_XFR_PENDING , &client->flags)) { -spin_unlock_irq (&client->lock); -kfree (client); -return 0 ; - } +set_bit (XFR_CANCELED , &xfr->flags); +if (!test_bit (CLIENT_XFR_PENDING , &client->flags)) + goto done;spin_lock_irq (&occ->list_lock); -set_bit (XFR_CANCELED , &xfr->flags);if (!test_bit (XFR_IN_PROGRESS , &xfr->flags)) {/* already deleted from list if complete */ if (!test_bit (XFR_COMPLETE , &xfr->flags))list_del (&xfr->link); - -spin_unlock_irq (&occ->list_lock); - -if (test_bit (XFR_WAITING , &xfr->flags)) { -/* blocking read; let reader clean up */ -wake_up_interruptible (&client->wait); -spin_unlock_irq (&client->lock); -return 0 ; - } - -spin_unlock_irq (&client->lock); - -kfree (client); -return 0 ; } -/* operation is in progress; let worker clean up */ spin_unlock_irq (&occ->list_lock); + +wake_up_all (&client->wait); + +done:spin_unlock_irq (&client->lock); + +occ_put_client (client);return 0 ; }
@@ -601,7 +604,7 @@ static int occ_trigger_attn(struct device *sbefifo)static void occ_worker (struct work_struct *work) { -int rc =0 , empty, waiting, canceled; +int rc =0 , empty;u16 resp_data_length;unsigned long start;const unsigned long timeout =msecs_to_jiffies (OCC_TIMEOUT_MS );
@@ -624,6 +627,8 @@ static void occ_worker(struct work_struct *work)return ; } + client =to_client (xfr); +occ_get_client (client); resp = (struct occ_response *)xfr->buf;set_bit (XFR_IN_PROGRESS , &xfr->flags);
@@ -679,29 +684,18 @@ static void occ_worker(struct work_struct *work)mutex_unlock (&occ->occ_lock); xfr->rc = rc; - client =to_client (xfr); - -/* lock client to prevent race with read() */ -spin_lock_irq (&client->lock); -set_bit (XFR_COMPLETE , &xfr->flags); - waiting =test_bit (XFR_WAITING , &xfr->flags); - -spin_unlock_irq (&client->lock);spin_lock_irq (&occ->list_lock);clear_bit (XFR_IN_PROGRESS , &xfr->flags);list_del (&xfr->link); empty =list_empty (&occ->xfrs); - canceled =test_bit (XFR_CANCELED , &xfr->flags);spin_unlock_irq (&occ->list_lock); -if (waiting) -wake_up_interruptible (&client->wait); -else if (canceled) -kfree (client); +wake_up_interruptible (&client->wait); +occ_put_client (client);if (!empty) goto again;
Signature · 1 line
1.8.3.1
Re: [PATCH linux dev-4.10 v4 29/31] drivers: fsi: occ: Fix client memory management
Quoted 6 lines · Eddie James
From: "Edward A. James" <[email protected]>
Potential for bad memory access in the worker function. Now fixed by using reference counters.Signed-off-by Edward A. James <[email protected]>
This is hairy, but given the current design I understand the need. I feel that ideas similar to I have for the SBEFIFO might be able to clean it up.
For the moment:
Quoted 225 lines
---
drivers/fsi/occ.c | 92 ++++++++++++++++++++++++++-----------------------------1 file changed, 43 insertions(+), 49 deletions(-)drivers/fsi/occ.c +0
-4@@ -70,15 +70,11 @@ struct occ_response {* 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 -* O_NONBLOCK wasn't specified. Cleared in read() when transfer completes or -* fails. */ enum { XFR_IN_PROGRESS, XFR_COMPLETE, XFR_CANCELED, -XFR_WAITING , };
structocc_xfr{drivers/fsi/occ.c +0
-0@@ -104,6 +100,7 @@ enum { };
structocc_client{drivers/fsi/occ.c +1
-0+struct kref kref;struct occ *occ;
structocc_xfrxfr;
spinlock_tlock; /* lock access to the client state */drivers/fsi/occ.c +0
-0@@ -140,6 +137,24 @@ static int occ_enqueue_xfr(struct occ_xfr *xfr) return 0; }drivers/fsi/occ.c +18
-0+static void occ_get_client (struct occ_client *client) +{ +kref_get (&client->kref); +} + +static void occ_client_release (struct kref *kref) +{ + struct occ_client *client = container_of(kref, struct occ_client, + kref); + +kfree (client); +} + +static void occ_put_client (struct occ_client *client) +{ +kref_put (&client->kref, occ_client_release); +} +static struct
occ_client*occ_open_common(struct occ *occ, unsigned long flags) {
structocc_client*client =kzalloc(sizeof(*client), GFP_KERNEL);drivers/fsi/occ.c +0
-0@@ -148,6 +163,7 @@ static struct occ_client *occ_open_common(struct occ *occ, unsigned long flags) return NULL;
client->occ= occ;drivers/fsi/occ.c +1
-0+kref_init (&client->kref);
spin_lock_init(&client->lock);
init_waitqueue_head(&client->wait);drivers/fsi/occ.c +0
-0@@ -192,6 +208,7 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf, if (len > OCC_SRAM_BYTES) return -EINVAL;drivers/fsi/occ.c +1
-0+occ_get_client (client);xfr =
&client->xfr;
occ =client->occ;drivers/fsi/occ.c +0
-0@@ -215,8 +232,6 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf, goto done; }drivers/fsi/occ.c +0
-2-set_bit (XFR_WAITING , &xfr->flags); -
spin_unlock_irq(&client->lock);
rc = wait_event_interruptible(client->wait,drivers/fsi/occ.c +0
-0@@ -224,15 +239,12 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
spin_lock_irq(&client->lock);drivers/fsi/occ.c +0
-7-if (test_bit (XFR_CANCELED , &xfr->flags)) { -spin_unlock_irq (&client->lock); -kfree (client); -return -EBADFD ; - } - -clear_bit (XFR_WAITING , &xfr->flags);if (!test_bit(XFR_COMPLETE,
&xfr->flags)) {drivers/fsi/occ.c +5
-1- rc = -EINTR ; +if (occ->cancel ||test_bit (XFR_CANCELED , &xfr->flags)) + rc = -ECANCELED ; +else + rc = -EINTR ; +goto done;
}
}drivers/fsi/occ.c +0
-0@@ -263,6 +275,7 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
done:
spin_unlock_irq(&client->lock);drivers/fsi/occ.c +1
-0+occ_put_client (client);return rc;
}drivers/fsi/occ.c +0
-0@@ -289,6 +302,7 @@ static ssize_t occ_write_common(struct occ_client *client, if (len > (OCC_CMD_DATA_BYTES + 3) || len < 3) return -EINVAL;drivers/fsi/occ.c +1
-0+occ_get_client (client);xfr =
&client->xfr;
spin_lock_irq(&client->lock);drivers/fsi/occ.c +0
-0@@ -340,6 +354,7 @@ static ssize_t occ_write_common(struct occ_client *client,
done:
spin_unlock_irq(&client->lock);drivers/fsi/occ.c +1
-0+occ_put_client (client);return rc;
}drivers/fsi/occ.c +0
-0@@ -364,38 +379,26 @@ static int occ_release_common(struct occ_client *client)
spin_lock_irq(&client->lock);drivers/fsi/occ.c +3
-5-if (!test_bit (CLIENT_XFR_PENDING , &client->flags)) { -spin_unlock_irq (&client->lock); -kfree (client); -return 0 ; - } +set_bit (XFR_CANCELED , &xfr->flags); +if (!test_bit (CLIENT_XFR_PENDING , &client->flags)) + goto done;
spin_lock_irq(&occ->list_lock);drivers/fsi/occ.c +0
-1-set_bit (XFR_CANCELED , &xfr->flags);if (!test_bit(XFR_IN_PROGRESS,
&xfr->flags)) { /* already deleted from list if complete */ if (!test_bit(XFR_COMPLETE,&xfr->flags))list_del(&xfr->link);drivers/fsi/occ.c +0
-14- -spin_unlock_irq (&occ->list_lock); - -if (test_bit (XFR_WAITING , &xfr->flags)) { -/* blocking read; let reader clean up */ -wake_up_interruptible (&client->wait); -spin_unlock_irq (&client->lock); -return 0 ; - } - -spin_unlock_irq (&client->lock); - -kfree (client); -return 0 ;}
drivers/fsi/occ.c +0
-1-/* operation is in progress; let worker clean up */
spin_unlock_irq(&occ->list_lock);drivers/fsi/occ.c +4
-0+ +wake_up_all (&client->wait); + +done:
spin_unlock_irq(&client->lock);drivers/fsi/occ.c +2
-0+ +occ_put_client (client);return 0;
}drivers/fsi/occ.c +0
-0@@ -601,7 +604,7 @@ static int occ_trigger_attn(struct device *sbefifo)
static voidocc_worker(struct work_struct *work)
{drivers/fsi/occ.c +1
-1-int rc =0 , empty, waiting, canceled; +int rc =0 , empty;u16
resp_data_length;
unsigned long start;
const unsigned long timeout =msecs_to_jiffies(OCC_TIMEOUT_MS);drivers/fsi/occ.c +0
-0@@ -624,6 +627,8 @@ static void occ_worker(struct work_struct *work) return; }drivers/fsi/occ.c +2
-0+ client =to_client (xfr); +occ_get_client (client);resp = (struct
occ_response*)xfr->buf;
set_bit(XFR_IN_PROGRESS, &xfr->flags);drivers/fsi/occ.c +0
-0@@ -679,29 +684,18 @@ static void occ_worker(struct work_struct *work) mutex_unlock(&occ->occ_lock);
xfr->rc= rc;drivers/fsi/occ.c +0
-5- client =to_client (xfr); - -/* lock client to prevent race with read() */ -spin_lock_irq (&client->lock); -
set_bit(XFR_COMPLETE, &xfr->flags);drivers/fsi/occ.c +0
-3- waiting =test_bit (XFR_WAITING , &xfr->flags); - -spin_unlock_irq (&client->lock);
spin_lock_irq(&occ->list_lock);
clear_bit(XFR_IN_PROGRESS, &xfr->flags);
list_del(&xfr->link);
empty =list_empty(&occ->xfrs);drivers/fsi/occ.c +0
-1- canceled =test_bit (XFR_CANCELED , &xfr->flags);
spin_unlock_irq(&occ->list_lock);drivers/fsi/occ.c +2
-4-if (waiting) -wake_up_interruptible (&client->wait); -else if (canceled) -kfree (client); +wake_up_interruptible (&client->wait); +occ_put_client (client);
if (!empty)
goto again;
Re: [PATCH linux dev-4.10 v4 29/31] drivers: fsi: occ: Fix client memory management
In reply to Eddie JamesQuoted 93 lines · Eddie James
From: "Edward A. James" <[email protected]>
Potential for bad memory access in the worker function. Now fixed by using reference counters.
Signed-off-by Edward A. James <[email protected]>--- drivers/fsi/occ.c | 92 ++++++++++++++++++++++++++----------------------------- 1 file changed, 43 insertions(+), 49 deletions(-)drivers/fsi/occ.c +21
-7@@ -70,15 +70,11 @@ struct occ_response {* 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 -* O_NONBLOCK wasn't specified. Cleared in read() when transfer completes or -* fails. */ enum {XFR_IN_PROGRESS ,XFR_COMPLETE ,XFR_CANCELED , -XFR_WAITING , };struct occ_xfr {@@ -104,6 +100,7 @@ enum { };struct occ_client { +struct kref kref;struct occ *occ;struct occ_xfr xfr;spinlock_t lock;/* lock access to the client state */ @@ -140,6 +137,24 @@ static int occ_enqueue_xfr(struct occ_xfr *xfr)return 0 ; } +static void occ_get_client (struct occ_client *client) +{ +kref_get (&client->kref); +} + +static void occ_client_release (struct kref *kref) +{ + struct occ_client *client = container_of(kref, struct occ_client, + kref); + +kfree (client); +} + +static void occ_put_client (struct occ_client *client) +{ +kref_put (&client->kref, occ_client_release); +} +static struct occ_client *occ_open_common (struct occ *occ,unsigned long flags) {struct occ_client *client =kzalloc (sizeof (*client),GFP_KERNEL );@@ -148,6 +163,7 @@ static struct occ_client *occ_open_common(struct occ *occ, unsigned long flags)return NULL ; client->occ = occ; +kref_init (&client->kref);spin_lock_init (&client->lock);init_waitqueue_head (&client->wait);@@ -192,6 +208,7 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,if (len >OCC_SRAM_BYTES )return -EINVAL ; +occ_get_client (client); xfr = &client->xfr; occ = client->occ;@@ -215,8 +232,6 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf, goto done; } -set_bit (XFR_WAITING , &xfr->flags); -spin_unlock_irq (&client->lock); rc = wait_event_interruptible(client->wait,@@ -224,15 +239,12 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,spin_lock_irq (&client->lock); -if (test_bit (XFR_CANCELED , &xfr->flags)) {
drivers/fsi/occ.c static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf, · line 244
Quoted 10 lines
drivers/fsi/occ.c +2
-7-spin_unlock_irq (&client->lock); -kfree (client); -return -EBADFD ; - } - -clear_bit (XFR_WAITING , &xfr->flags);if (!test_bit (XFR_COMPLETE , &xfr->flags)) { - rc = -EINTR ; +if (occ->cancel ||test_bit (XFR_CANCELED , &xfr->flags)) + rc = -ECANCELED ;
We probably need this ENODEV too. We get here if the client is released or the device is removed while we're waiting for a read to complete.
Quoted 128 lines
drivers/fsi/occ.c +20
-35+else + rc = -EINTR ; + goto done; } }@@ -263,6 +275,7 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf, done:spin_unlock_irq (&client->lock); +occ_put_client (client);return rc; }@@ -289,6 +302,7 @@ static ssize_t occ_write_common(struct occ_client *client,if (len > (OCC_CMD_DATA_BYTES +3 ) || len <3 )return -EINVAL ; +occ_get_client (client); xfr = &client->xfr;spin_lock_irq (&client->lock);@@ -340,6 +354,7 @@ static ssize_t occ_write_common(struct occ_client *client, done:spin_unlock_irq (&client->lock); +occ_put_client (client);return rc; }@@ -364,38 +379,26 @@ static int occ_release_common(struct occ_client *client)spin_lock_irq (&client->lock); -if (!test_bit (CLIENT_XFR_PENDING , &client->flags)) { -spin_unlock_irq (&client->lock); -kfree (client); -return 0 ; - } +set_bit (XFR_CANCELED , &xfr->flags); +if (!test_bit (CLIENT_XFR_PENDING , &client->flags)) + goto done;spin_lock_irq (&occ->list_lock); -set_bit (XFR_CANCELED , &xfr->flags);if (!test_bit (XFR_IN_PROGRESS , &xfr->flags)) {/* already deleted from list if complete */ if (!test_bit (XFR_COMPLETE , &xfr->flags))list_del (&xfr->link); - -spin_unlock_irq (&occ->list_lock); - -if (test_bit (XFR_WAITING , &xfr->flags)) { -/* blocking read; let reader clean up */ -wake_up_interruptible (&client->wait); -spin_unlock_irq (&client->lock); -return 0 ; - } - -spin_unlock_irq (&client->lock); - -kfree (client); -return 0 ; } -/* operation is in progress; let worker clean up */ spin_unlock_irq (&occ->list_lock); + +wake_up_all (&client->wait); + +done:spin_unlock_irq (&client->lock); + +occ_put_client (client);return 0 ; }@@ -601,7 +604,7 @@ static int occ_trigger_attn(struct device *sbefifo)static void occ_worker (struct work_struct *work) { -int rc =0 , empty, waiting, canceled; +int rc =0 , empty;u16 resp_data_length;unsigned long start;const unsigned long timeout =msecs_to_jiffies (OCC_TIMEOUT_MS );@@ -624,6 +627,8 @@ static void occ_worker(struct work_struct *work)return ; } + client =to_client (xfr); +occ_get_client (client); resp = (struct occ_response *)xfr->buf;set_bit (XFR_IN_PROGRESS , &xfr->flags);@@ -679,29 +684,18 @@ static void occ_worker(struct work_struct *work)mutex_unlock (&occ->occ_lock); xfr->rc = rc; - client =to_client (xfr); - -/* lock client to prevent race with read() */ -spin_lock_irq (&client->lock); -set_bit (XFR_COMPLETE , &xfr->flags); - waiting =test_bit (XFR_WAITING , &xfr->flags); - -spin_unlock_irq (&client->lock);spin_lock_irq (&occ->list_lock);clear_bit (XFR_IN_PROGRESS , &xfr->flags);list_del (&xfr->link); empty =list_empty (&occ->xfrs); - canceled =test_bit (XFR_CANCELED , &xfr->flags);spin_unlock_irq (&occ->list_lock); -if (waiting) -wake_up_interruptible (&client->wait); -else if (canceled) -kfree (client); +wake_up_interruptible (&client->wait); +occ_put_client (client);if (!empty) goto again;
PATCH v4 30/31 drivers/hwmon/occ: Remove repeated ops for OCC command in progress
In reply to Eddie JamesFrom: "Edward A. James" <[email protected]>
This is now handled in the occ driver.
--- drivers/hwmon/occ/p9_sbe.c | 14 +------------- 1 file changed, 1 insertion(+), 13 deletions(-)
drivers/hwmon/occ/p9_sbe.c +1 -13
@@ -26,14 +26,10 @@ struct p9_sbe_occ {static int p9_sbe_occ_send_cmd (struct occ *occ,u8 *cmd) {int rc, error; -unsigned long start;struct occ_client *client;struct occ_response *resp = &occ->resp;struct p9_sbe_occ *p9_sbe_occ =to_p9_sbe_occ (occ); - start = jiffies; - -retry: client =occ_drv_open (p9_sbe_occ->sbe,0 );if (!client) { rc = -ENODEV ;
@@ -52,15 +48,7 @@ static int p9_sbe_occ_send_cmd(struct occ *occ, u8 *cmd)switch (resp->return_status) {case RESP_RETURN_CMD_IN_PRG : -if (time_after (jiffies, - start +msecs_to_jiffies (OCC_TIMEOUT_MS ))) - rc = -EALREADY ; -else { -set_current_state (TASK_INTERRUPTIBLE ); -schedule_timeout (msecs_to_jiffies (OCC_CMD_IN_PRG_MS )); - - goto retry; - } + rc = -ETIMEDOUT ;break ;case RESP_RETURN_SUCCESS :occ_reset_error (occ);
Signature · 1 line
1.8.3.1
PATCH v4 31/31 drivers: hwmon: occ: Cancel occ operations in remove()
In reply to Eddie JamesFrom: "Edward A. James" <[email protected]>
Prevent hanging forever waiting for OCC ops to complete.
--- drivers/hwmon/occ/p9_sbe.c | 35 ++++++++++++++++++++++++++++------- 1 file changed, 28 insertions(+), 7 deletions(-)
drivers/hwmon/occ/p9_sbe.c +28 -7
@@ -14,37 +14,54 @@#include <linux/platform_device.h> #include <linux/occ.h> #include <linux/sched.h> +#include <linux/spinlock.h> #include <linux/workqueue.h> struct p9_sbe_occ {struct occ occ;struct device *sbe; +struct occ_client *client; +spinlock_t lock; };#define to_p9_sbe_occ (x) container_of((x), struct p9_sbe_occ, occ) +static void p9_sbe_occ_close_client (struct p9_sbe_occ *occ) +{ +struct occ_client *tmp_client; + +spin_lock_irq (&occ->lock); + tmp_client = occ->client; + occ->client =NULL ; +occ_drv_release (tmp_client); +spin_unlock_irq (&occ->lock); +} +static int p9_sbe_occ_send_cmd (struct occ *occ,u8 *cmd) {int rc, error; -struct occ_client *client;struct occ_response *resp = &occ->resp;struct p9_sbe_occ *p9_sbe_occ =to_p9_sbe_occ (occ); - client =occ_drv_open (p9_sbe_occ->sbe,0 ); -if (!client) { +spin_lock_irq (&p9_sbe_occ->lock); +if (p9_sbe_occ->sbe) + p9_sbe_occ->client =occ_drv_open (p9_sbe_occ->sbe,0 ); +spin_unlock_irq (&p9_sbe_occ->lock); + +if (!p9_sbe_occ->client) { rc = -ENODEV ; goto assign; } - rc =occ_drv_write (client, (const char *)&cmd[1 ],7 ); + rc =occ_drv_write (p9_sbe_occ->client, (const char *)&cmd[1 ],7 );if (rc <0 ) goto err; - rc =occ_drv_read (client, (char *)resp,sizeof (*resp)); + rc =occ_drv_read (p9_sbe_occ->client, (char *)resp,sizeof (*resp));if (rc <0 ) goto err; -occ_drv_release (client); +p9_sbe_occ_close_client (p9_sbe_occ);switch (resp->return_status) {case RESP_RETURN_CMD_IN_PRG :
@@ -72,7 +89,7 @@ static int p9_sbe_occ_send_cmd(struct occ *occ, u8 *cmd) goto done; err: -occ_drv_release (client); +p9_sbe_occ_close_client (p9_sbe_occ);dev_err (occ->bus_dev,"occ bus op failed rc:%d\n" , rc); assign: error = rc;
@@ -132,6 +149,7 @@ static int p9_sbe_occ_probe(struct platform_device *pdev) p9_sbe_occ->sbe = pdev->dev.parent; occ = &p9_sbe_occ->occ; +spin_lock_init (&p9_sbe_occ->lock); occ->bus_dev = &pdev->dev; occ->groups[0 ] = &occ->group; occ->poll_cmd_data =0x20 ;
@@ -152,7 +170,10 @@ static int p9_sbe_occ_probe(struct platform_device *pdev)static int p9_sbe_occ_remove (struct platform_device *pdev) {struct occ *occ =platform_get_drvdata (pdev); +struct p9_sbe_occ *p9_sbe_occ =to_p9_sbe_occ (occ); + p9_sbe_occ->sbe =NULL ; +p9_sbe_occ_close_client (p9_sbe_occ);occ_remove_status_attrs (occ);atomic_dec (&occ_num_occs);
Signature · 1 line
1.8.3.1
Re: [PATCH linux dev-4.10 v4 31/31] drivers: hwmon: occ: Cancel occ operations in remove()
Quoted 23 lines · Eddie James
From: "Edward A. James" <[email protected]>
Prevent hanging forever waiting for OCC ops to complete.
Signed-off-by Edward A. James <[email protected]>---
drivers/hwmon/occ/p9_sbe.c | 35 ++++++++++++++++++++++++++++-------1 file changed, 28 insertions(+), 7 deletions(-)drivers/hwmon/occ/p9_sbe.c +1
-0@@ -14,37 +14,54 @@ #include <linux/platform_device.h> #include <linux/occ.h> #include <linux/sched.h> +#include <linux/spinlock.h> #include <linux/workqueue.h>
structp9_sbe_occ{
struct occ occ;
struct device *sbe;
drivers/hwmon/occ/p9_sbe.c line 24
Quoted 2 lines
drivers/hwmon/occ/p9_sbe.c +2
-0+struct occ_client *client; +spinlock_t lock;
Can you please explain the semantics of this lock in a comment or in the commit message? Who should hold it, when and why, and in what order with respect to other locks if applicable?
We've discussed it in private previously but I admit the details have escaped me, and the way it gets used in this patch looks pretty strange.
Andrew
Quoted 84 lines
};
#defineto_p9_sbe_occ(x)container_of((x), structp9_sbe_occ,
occ)drivers/hwmon/occ/p9_sbe.c +11
-0+static void p9_sbe_occ_close_client (struct p9_sbe_occ *occ) +{ +struct occ_client *tmp_client; + +spin_lock_irq (&occ->lock); + tmp_client = occ->client; + occ->client =NULL ; +occ_drv_release (tmp_client); +spin_unlock_irq (&occ->lock); +} +static int
p9_sbe_occ_send_cmd(struct occ *occ, u8 *cmd){
int rc, error;drivers/hwmon/occ/p9_sbe.c +0
-1-struct occ_client *client;struct
occ_response*resp =&occ->resp; structp9_sbe_occ*p9_sbe_occ=to_p9_sbe_occ(occ);drivers/hwmon/occ/p9_sbe.c +3
-2- client =occ_drv_open (p9_sbe_occ->sbe,0 ); -if (!client) { +spin_lock_irq (&p9_sbe_occ->lock); +if (p9_sbe_occ->sbe) + p9_sbe_occ->client = occ_drv_open(p9_sbe_occ->sbe,0);
drivers/hwmon/occ/p9_sbe.c +3
-0+spin_unlock_irq (&p9_sbe_occ->lock); + +if (!p9_sbe_occ->client) {rc =
-ENODEV;
goto assign;
}drivers/hwmon/occ/p9_sbe.c +1
-1- rc =occ_drv_write (client, (const char *)&cmd[1 ],7 ); + rc = occ_drv_write(p9_sbe_occ->client, (const char*)&cmd[1], 7);
if (rc < 0)
goto err;drivers/hwmon/occ/p9_sbe.c +1
-1- rc =occ_drv_read (client, (char *)resp,sizeof (*resp)); + rc = occ_drv_read(p9_sbe_occ->client, (char *)resp,
sizeof(*resp));
if (rc < 0)
goto err;drivers/hwmon/occ/p9_sbe.c +1
-1-occ_drv_release (client); +p9_sbe_occ_close_client (p9_sbe_occ);
switch (resp->return_status) {
caseRESP_RETURN_CMD_IN_PRG:drivers/hwmon/occ/p9_sbe.c +0
-0@@ -72,7 +89,7 @@ static int p9_sbe_occ_send_cmd(struct occ *occ, u8 *cmd) goto done;
err:drivers/hwmon/occ/p9_sbe.c +1
-1-occ_drv_release (client); +p9_sbe_occ_close_client (p9_sbe_occ);
dev_err(occ->bus_dev, "occ bus op failed rc:%d\n", rc); assign:
error = rc;drivers/hwmon/occ/p9_sbe.c +0
-0@@ -132,6 +149,7 @@ static int p9_sbe_occ_probe(struct platform_device *pdev) p9_sbe_occ->sbe = pdev->dev.parent;
occ =&p9_sbe_occ->occ;drivers/hwmon/occ/p9_sbe.c +1
-0+spin_lock_init (&p9_sbe_occ->lock);
occ->bus_dev=&pdev->dev;
occ->groups[0] =&occ->group;
occ->poll_cmd_data=0x20;drivers/hwmon/occ/p9_sbe.c +1
-0@@ -152,7 +170,10 @@ static int p9_sbe_occ_probe(struct platform_device *pdev) static int p9_sbe_occ_remove(struct platform_device *pdev) { struct occ *occ = platform_get_drvdata(pdev); +struct p9_sbe_occ *p9_sbe_occ =to_p9_sbe_occ (occ);drivers/hwmon/occ/p9_sbe.c +2
-0+ p9_sbe_occ->sbe =NULL ; +p9_sbe_occ_close_client (p9_sbe_occ);
occ_remove_status_attrs(occ);
atomic_dec(&occ_num_occs);