* [PATCH linux dev-4.10 0/3] drivers: fsi: Fixup SBEFIFO and OCC locking
@ 2018-02-02 22:11 Eddie James
2018-02-02 22:11 ` [PATCH linux dev-4.10 1/3] drivers: fsi: SBEFIFO: switch to workqueue instead of timer Eddie James
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: Eddie James @ 2018-02-02 22:11 UTC (permalink / raw)
To: openbmc; +Cc: joel, andrew, Eddie James
This series provides a couple of fixes designed to make the FSI client drivers
play nicely with Jeremy's incoming FSI spinlock changes. Firstly, the SBEFIFO
driver must switch to use a workqueue instead of a poll timer so that we can
avoid spinlocking the entire function. Therefore, the second patch switches to
a mutex instead of a spinlock there. Lastly, switch to irqsave and irqrestore
functions in the OCC driver to cleanup the locking there.
Eddie James (3):
drivers: fsi: SBEFIFO: switch to workqueue instead of timer
drivers: fsi: sbefifo: switch to mutex in work function
drivers: fsi: occ: switch to irqsave and irqrestore
drivers/fsi/fsi-sbefifo.c | 94 ++++++++++++++++++++++++++++++-----------------
drivers/fsi/occ.c | 44 ++++++++++++----------
2 files changed, 86 insertions(+), 52 deletions(-)
--
1.8.3.1
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH linux dev-4.10 1/3] drivers: fsi: SBEFIFO: switch to workqueue instead of timer
2018-02-02 22:11 [PATCH linux dev-4.10 0/3] drivers: fsi: Fixup SBEFIFO and OCC locking Eddie James
@ 2018-02-02 22:11 ` Eddie James
2018-02-02 22:11 ` [PATCH linux dev-4.10 2/3] drivers: fsi: sbefifo: switch to mutex in work function Eddie James
2018-02-02 22:11 ` [PATCH linux dev-4.10 3/3] drivers: fsi: occ: switch to irqsave and irqrestore Eddie James
2 siblings, 0 replies; 5+ messages in thread
From: Eddie James @ 2018-02-02 22:11 UTC (permalink / raw)
To: openbmc; +Cc: joel, andrew, Eddie James, Edward A. James
Switch to a workqueue instead of a poll timer so that we can use a mutex
in the work function instead of a spinlock. This allows the use of
Jeremy's FSI scheduling fix.
Signed-off-by: Edward A. James <eajames@us.ibm.com>
---
drivers/fsi/fsi-sbefifo.c | 54 ++++++++++++++++++++++++++++-------------------
1 file changed, 32 insertions(+), 22 deletions(-)
diff --git a/drivers/fsi/fsi-sbefifo.c b/drivers/fsi/fsi-sbefifo.c
index 0697a10..7b6842a 100644
--- a/drivers/fsi/fsi-sbefifo.c
+++ b/drivers/fsi/fsi-sbefifo.c
@@ -29,9 +29,9 @@
#include <linux/sched.h>
#include <linux/slab.h>
#include <linux/spinlock.h>
-#include <linux/timer.h>
#include <linux/uaccess.h>
#include <linux/wait.h>
+#include <linux/workqueue.h>
/*
* The SBEFIFO is a pipe-like FSI device for communicating with
@@ -57,7 +57,7 @@
#define SBEFIFO_MAX_RESCHDULE msecs_to_jiffies(5000)
struct sbefifo {
- struct timer_list poll_timer;
+ struct delayed_work work;
struct fsi_device *fsi_dev;
struct miscdevice mdev;
wait_queue_head_t wait;
@@ -99,6 +99,8 @@ struct sbefifo_client {
unsigned long f_flags;
};
+static struct workqueue_struct *sbefifo_wq;
+
static DEFINE_IDA(sbefifo_ida);
static int sbefifo_inw(struct sbefifo *sbefifo, int reg, u32 *word)
@@ -337,7 +339,7 @@ static void sbefifo_client_release(struct kref *kref)
*/
set_bit(SBEFIFO_XFR_CANCEL, &xfr->flags);
sbefifo_get(sbefifo);
- if (mod_timer(&client->dev->poll_timer, jiffies))
+ if (!queue_work(sbefifo_wq, &sbefifo->work.work))
sbefifo_put(sbefifo);
}
}
@@ -374,10 +376,11 @@ static struct sbefifo_xfr *sbefifo_next_xfr(struct sbefifo *sbefifo)
return NULL;
}
-static void sbefifo_poll_timer(unsigned long data)
+static void sbefifo_worker(struct work_struct *work)
{
static const unsigned long EOT_MASK = 0x000000ff;
- struct sbefifo *sbefifo = (void *)data;
+ struct delayed_work *dwork = to_delayed_work(work);
+ struct sbefifo *sbefifo = container_of(dwork, struct sbefifo, work);
struct sbefifo_buf *rbuf, *wbuf;
struct sbefifo_xfr *xfr, *tmp;
struct sbefifo_buf drain;
@@ -393,6 +396,7 @@ static void sbefifo_poll_timer(unsigned long data)
if (!xfr)
goto out_unlock;
+again:
rbuf = xfr->rbuf;
wbuf = xfr->wbuf;
@@ -414,9 +418,8 @@ static void sbefifo_poll_timer(unsigned long data)
devn = sbefifo_dev_nwwriteable(sts);
if (devn == 0) {
/* No open slot for write. Reschedule. */
- sbefifo->poll_timer.expires = jiffies +
- SBEFIFO_RESCHEDULE;
- add_timer(&sbefifo->poll_timer);
+ queue_delayed_work(sbefifo_wq, &sbefifo->work,
+ SBEFIFO_RESCHEDULE);
goto out_unlock;
}
@@ -468,9 +471,8 @@ static void sbefifo_poll_timer(unsigned long data)
}
/* No data yet. Reschedule. */
- sbefifo->poll_timer.expires = jiffies +
- SBEFIFO_RESCHEDULE;
- add_timer(&sbefifo->poll_timer);
+ queue_delayed_work(sbefifo_wq, &sbefifo->work,
+ SBEFIFO_RESCHEDULE);
goto out_unlock;
} else {
xfr->wait_data_timeout = 0;
@@ -516,10 +518,12 @@ static void sbefifo_poll_timer(unsigned long data)
}
INIT_LIST_HEAD(&sbefifo->xfrs);
- } else if (eot && sbefifo_next_xfr(sbefifo)) {
- sbefifo_get(sbefifo);
- sbefifo->poll_timer.expires = jiffies;
- add_timer(&sbefifo->poll_timer);
+ } else if (eot) {
+ xfr = sbefifo_next_xfr(sbefifo);
+ if (xfr) {
+ wake_up_interruptible(&sbefifo->wait);
+ goto again;
+ }
}
sbefifo_put(sbefifo);
@@ -638,7 +642,7 @@ static ssize_t sbefifo_read_common(struct sbefifo_client *client,
* Fill the read buffer back up.
*/
sbefifo_get(sbefifo);
- if (mod_timer(&client->dev->poll_timer, jiffies))
+ if (!queue_work(sbefifo_wq, &sbefifo->work.work))
sbefifo_put(sbefifo);
} else {
list_del(&xfr->client);
@@ -721,7 +725,7 @@ static ssize_t sbefifo_write_common(struct sbefifo_client *client,
&n))) {
set_bit(SBEFIFO_XFR_CANCEL, &xfr->flags);
sbefifo_get(sbefifo);
- if (mod_timer(&sbefifo->poll_timer, jiffies))
+ if (!queue_work(sbefifo_wq, &sbefifo->work.work))
sbefifo_put(sbefifo);
ret = -ERESTARTSYS;
@@ -741,7 +745,8 @@ static ssize_t sbefifo_write_common(struct sbefifo_client *client,
n)) {
set_bit(SBEFIFO_XFR_CANCEL, &xfr->flags);
sbefifo_get(sbefifo);
- if (mod_timer(&sbefifo->poll_timer, jiffies))
+ if (!queue_work(sbefifo_wq,
+ &sbefifo->work.work))
sbefifo_put(sbefifo);
ret = -EFAULT;
goto out;
@@ -768,7 +773,7 @@ static ssize_t sbefifo_write_common(struct sbefifo_client *client,
* Drain the write buffer.
*/
sbefifo_get(sbefifo);
- if (mod_timer(&client->dev->poll_timer, jiffies))
+ if (!queue_work(sbefifo_wq, &sbefifo->work.work))
sbefifo_put(sbefifo);
}
@@ -928,8 +933,7 @@ static int sbefifo_probe(struct device *dev)
sbefifo->idx);
/* This bit of silicon doesn't offer any interrupts... */
- setup_timer(&sbefifo->poll_timer, sbefifo_poll_timer,
- (unsigned long)sbefifo);
+ INIT_DELAYED_WORK(&sbefifo->work, sbefifo_worker);
sbefifo->mdev.minor = MISC_DYNAMIC_MINOR;
sbefifo->mdev.fops = &sbefifo_fops;
@@ -982,7 +986,7 @@ static int sbefifo_remove(struct device *dev)
ida_simple_remove(&sbefifo_ida, sbefifo->idx);
- if (del_timer_sync(&sbefifo->poll_timer))
+ if (cancel_delayed_work_sync(&sbefifo->work))
sbefifo_put(sbefifo);
sbefifo_put(sbefifo);
@@ -1010,11 +1014,17 @@ static int sbefifo_remove(struct device *dev)
static int sbefifo_init(void)
{
+ sbefifo_wq = create_singlethread_workqueue("sbefifo");
+ if (!sbefifo_wq)
+ return -ENOMEM;
+
return fsi_driver_register(&sbefifo_drv);
}
static void sbefifo_exit(void)
{
+ destroy_workqueue(sbefifo_wq);
+
fsi_driver_unregister(&sbefifo_drv);
ida_destroy(&sbefifo_ida);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH linux dev-4.10 2/3] drivers: fsi: sbefifo: switch to mutex in work function
2018-02-02 22:11 [PATCH linux dev-4.10 0/3] drivers: fsi: Fixup SBEFIFO and OCC locking Eddie James
2018-02-02 22:11 ` [PATCH linux dev-4.10 1/3] drivers: fsi: SBEFIFO: switch to workqueue instead of timer Eddie James
@ 2018-02-02 22:11 ` Eddie James
2018-02-02 22:11 ` [PATCH linux dev-4.10 3/3] drivers: fsi: occ: switch to irqsave and irqrestore Eddie James
2 siblings, 0 replies; 5+ messages in thread
From: Eddie James @ 2018-02-02 22:11 UTC (permalink / raw)
To: openbmc; +Cc: joel, andrew, Eddie James
Use a mutex instead of a spinlock to prevent locking up the bmc while
waiting on FSI operations. This can then be used in conjunction with
Jeremy's FSI spinlock fix.
Signed-off-by: Eddie James <eajames@linux.vnet.ibm.com>
---
drivers/fsi/fsi-sbefifo.c | 42 ++++++++++++++++++++++++++++++------------
1 file changed, 30 insertions(+), 12 deletions(-)
diff --git a/drivers/fsi/fsi-sbefifo.c b/drivers/fsi/fsi-sbefifo.c
index 7b6842a..4d024ed 100644
--- a/drivers/fsi/fsi-sbefifo.c
+++ b/drivers/fsi/fsi-sbefifo.c
@@ -22,6 +22,7 @@
#include <linux/list.h>
#include <linux/miscdevice.h>
#include <linux/module.h>
+#include <linux/mutex.h>
#include <linux/of.h>
#include <linux/of_device.h>
#include <linux/of_platform.h>
@@ -63,7 +64,8 @@ struct sbefifo {
wait_queue_head_t wait;
struct list_head xfrs;
struct kref kref;
- spinlock_t lock;
+ spinlock_t list_lock; /* lock access to the xfrs list */
+ struct mutex sbefifo_lock; /* lock access to the hardware */
char name[32];
int idx;
int rc;
@@ -389,12 +391,16 @@ static void sbefifo_worker(struct work_struct *work)
int ret = 0;
u32 sts;
int i;
+ unsigned long flags;
- spin_lock(&sbefifo->lock);
+ spin_lock_irqsave(&sbefifo->list_lock, flags);
xfr = list_first_entry_or_null(&sbefifo->xfrs, struct sbefifo_xfr,
xfrs);
+ spin_unlock_irqrestore(&sbefifo->list_lock, flags);
if (!xfr)
- goto out_unlock;
+ return;
+
+ mutex_lock(&sbefifo->sbefifo_lock);
again:
rbuf = xfr->rbuf;
@@ -499,7 +505,11 @@ static void sbefifo_worker(struct work_struct *work)
goto out;
set_bit(SBEFIFO_XFR_COMPLETE, &xfr->flags);
+
+ spin_lock_irqsave(&sbefifo->list_lock, flags);
list_del(&xfr->xfrs);
+ spin_unlock_irqrestore(&sbefifo->list_lock, flags);
+
if (unlikely(test_bit(SBEFIFO_XFR_CANCEL,
&xfr->flags)))
kfree(xfr);
@@ -512,14 +522,19 @@ static void sbefifo_worker(struct work_struct *work)
sbefifo->rc = ret;
dev_err(&sbefifo->fsi_dev->dev,
"Fatal bus access failure: %d\n", ret);
+
+ spin_lock_irqsave(&sbefifo->list_lock, flags);
list_for_each_entry_safe(xfr, tmp, &sbefifo->xfrs, xfrs) {
list_del(&xfr->xfrs);
kfree(xfr);
}
INIT_LIST_HEAD(&sbefifo->xfrs);
-
+ spin_unlock_irqrestore(&sbefifo->list_lock, flags);
} else if (eot) {
+ spin_lock_irqsave(&sbefifo->list_lock, flags);
xfr = sbefifo_next_xfr(sbefifo);
+ spin_unlock_irqrestore(&sbefifo->list_lock, flags);
+
if (xfr) {
wake_up_interruptible(&sbefifo->wait);
goto again;
@@ -530,7 +545,7 @@ static void sbefifo_worker(struct work_struct *work)
wake_up_interruptible(&sbefifo->wait);
out_unlock:
- spin_unlock(&sbefifo->lock);
+ mutex_unlock(&sbefifo->sbefifo_lock);
}
static int sbefifo_open(struct inode *inode, struct file *file)
@@ -686,6 +701,7 @@ static ssize_t sbefifo_write_common(struct sbefifo_client *client,
struct sbefifo_xfr *xfr;
ssize_t ret = 0;
size_t n;
+ unsigned long flags;
if ((len >> 2) << 2 != len)
return -EINVAL;
@@ -696,23 +712,23 @@ static ssize_t sbefifo_write_common(struct sbefifo_client *client,
sbefifo_get_client(client);
n = sbefifo_buf_nbwriteable(&client->wbuf);
- spin_lock_irq(&sbefifo->lock);
+ spin_lock_irqsave(&sbefifo->list_lock, flags);
xfr = sbefifo_next_xfr(sbefifo); /* next xfr to be executed */
if ((client->f_flags & O_NONBLOCK) && xfr && n < len) {
- spin_unlock_irq(&sbefifo->lock);
+ spin_unlock_irqrestore(&sbefifo->list_lock, flags);
ret = -EAGAIN;
goto out;
}
xfr = sbefifo_enq_xfr(client); /* this xfr queued up */
if (IS_ERR(xfr)) {
- spin_unlock_irq(&sbefifo->lock);
+ spin_unlock_irqrestore(&sbefifo->list_lock, flags);
ret = PTR_ERR(xfr);
goto out;
}
- spin_unlock_irq(&sbefifo->lock);
+ spin_unlock_irqrestore(&sbefifo->list_lock, flags);
/*
* Partial writes are not really allowed in that EOT is sent exactly
@@ -923,7 +939,8 @@ static int sbefifo_probe(struct device *dev)
}
}
- spin_lock_init(&sbefifo->lock);
+ spin_lock_init(&sbefifo->list_lock);
+ mutex_init(&sbefifo->sbefifo_lock);
kref_init(&sbefifo->kref);
init_waitqueue_head(&sbefifo->wait);
INIT_LIST_HEAD(&sbefifo->xfrs);
@@ -966,8 +983,9 @@ static int sbefifo_remove(struct device *dev)
{
struct sbefifo *sbefifo = dev_get_drvdata(dev);
struct sbefifo_xfr *xfr, *tmp;
+ unsigned long flags;
- spin_lock(&sbefifo->lock);
+ spin_lock_irqsave(&sbefifo->list_lock, flags);
WRITE_ONCE(sbefifo->rc, -ENODEV);
list_for_each_entry_safe(xfr, tmp, &sbefifo->xfrs, xfrs) {
@@ -977,7 +995,7 @@ static int sbefifo_remove(struct device *dev)
INIT_LIST_HEAD(&sbefifo->xfrs);
- spin_unlock(&sbefifo->lock);
+ spin_unlock_irqrestore(&sbefifo->list_lock, flags);
wake_up_all(&sbefifo->wait);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH linux dev-4.10 3/3] drivers: fsi: occ: switch to irqsave and irqrestore
2018-02-02 22:11 [PATCH linux dev-4.10 0/3] drivers: fsi: Fixup SBEFIFO and OCC locking Eddie James
2018-02-02 22:11 ` [PATCH linux dev-4.10 1/3] drivers: fsi: SBEFIFO: switch to workqueue instead of timer Eddie James
2018-02-02 22:11 ` [PATCH linux dev-4.10 2/3] drivers: fsi: sbefifo: switch to mutex in work function Eddie James
@ 2018-02-02 22:11 ` Eddie James
2018-02-05 20:14 ` Christopher Bostic
2 siblings, 1 reply; 5+ messages in thread
From: Eddie James @ 2018-02-02 22:11 UTC (permalink / raw)
To: openbmc; +Cc: joel, andrew, Eddie James
Fix spinlocking by storing the interrupt state and restoring that state
when unlocking, instead of blindly disabling and re-enabling all irqs.
Signed-off-by: Eddie James <eajames@linux.vnet.ibm.com>
---
drivers/fsi/occ.c | 44 +++++++++++++++++++++++++-------------------
1 file changed, 25 insertions(+), 19 deletions(-)
diff --git a/drivers/fsi/occ.c b/drivers/fsi/occ.c
index adc64f3..edb5e977 100644
--- a/drivers/fsi/occ.c
+++ b/drivers/fsi/occ.c
@@ -118,18 +118,19 @@ struct occ_client {
static int occ_enqueue_xfr(struct occ_xfr *xfr)
{
int empty;
+ unsigned long flags;
struct occ_client *client = to_client(xfr);
struct occ *occ = client->occ;
if (occ->cancel)
return -ENODEV;
- spin_lock_irq(&occ->list_lock);
+ spin_lock_irqsave(&occ->list_lock, flags);
empty = list_empty(&occ->xfrs);
list_add_tail(&xfr->link, &occ->xfrs);
- spin_unlock_irq(&occ->list_lock);
+ spin_unlock_irqrestore(&occ->list_lock, flags);
if (empty)
queue_work(occ_wq, &occ->work);
@@ -201,6 +202,7 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
size_t bytes;
struct occ_xfr *xfr;
struct occ *occ;
+ unsigned long flags;
if (!client)
return -ENODEV;
@@ -212,7 +214,7 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
xfr = &client->xfr;
occ = client->occ;
- spin_lock_irq(&client->lock);
+ spin_lock_irqsave(&client->lock, flags);
if (!test_bit(CLIENT_XFR_PENDING, &client->flags)) {
/* we just finished reading all data, return 0 */
@@ -232,12 +234,12 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
goto done;
}
- spin_unlock_irq(&client->lock);
+ spin_unlock_irqrestore(&client->lock, flags);
rc = wait_event_interruptible(client->wait,
occ_read_ready(xfr, occ));
- spin_lock_irq(&client->lock);
+ spin_lock_irqsave(&client->lock, flags);
if (!test_bit(XFR_COMPLETE, &xfr->flags)) {
if (occ->cancel || test_bit(XFR_CANCELED, &xfr->flags))
@@ -274,7 +276,7 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
rc = bytes;
done:
- spin_unlock_irq(&client->lock);
+ spin_unlock_irqrestore(&client->lock, flags);
occ_put_client(client);
return rc;
}
@@ -293,6 +295,7 @@ static ssize_t occ_write_common(struct occ_client *client,
{
int rc;
unsigned int i;
+ unsigned long flags;
u16 data_length, checksum = 0;
struct occ_xfr *xfr;
@@ -305,7 +308,7 @@ static ssize_t occ_write_common(struct occ_client *client,
occ_get_client(client);
xfr = &client->xfr;
- spin_lock_irq(&client->lock);
+ spin_lock_irqsave(&client->lock, flags);
if (test_bit(CLIENT_XFR_PENDING, &client->flags)) {
rc = -EBUSY;
@@ -353,7 +356,7 @@ static ssize_t occ_write_common(struct occ_client *client,
rc = len;
done:
- spin_unlock_irq(&client->lock);
+ spin_unlock_irqrestore(&client->lock, flags);
occ_put_client(client);
return rc;
}
@@ -370,6 +373,7 @@ static int occ_release_common(struct occ_client *client)
{
struct occ *occ;
struct occ_xfr *xfr;
+ unsigned long flags;
if (!client)
return -ENODEV;
@@ -377,13 +381,13 @@ static int occ_release_common(struct occ_client *client)
xfr = &client->xfr;
occ = client->occ;
- spin_lock_irq(&client->lock);
+ spin_lock_irqsave(&client->lock, flags);
set_bit(XFR_CANCELED, &xfr->flags);
if (!test_bit(CLIENT_XFR_PENDING, &client->flags))
goto done;
- spin_lock_irq(&occ->list_lock);
+ spin_lock(&occ->list_lock);
if (!test_bit(XFR_IN_PROGRESS, &xfr->flags)) {
/* already deleted from list if complete */
@@ -391,12 +395,12 @@ static int occ_release_common(struct occ_client *client)
list_del(&xfr->link);
}
- spin_unlock_irq(&occ->list_lock);
+ spin_unlock(&occ->list_lock);
wake_up_all(&client->wait);
done:
- spin_unlock_irq(&client->lock);
+ spin_unlock_irqrestore(&client->lock, flags);
occ_put_client(client);
return 0;
@@ -606,6 +610,7 @@ static void occ_worker(struct work_struct *work)
{
int rc = 0, empty;
u16 resp_data_length;
+ unsigned long flags;
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);
@@ -619,11 +624,11 @@ static void occ_worker(struct work_struct *work)
if (occ->cancel)
return;
- spin_lock_irq(&occ->list_lock);
+ spin_lock_irqsave(&occ->list_lock, flags);
xfr = list_first_entry_or_null(&occ->xfrs, struct occ_xfr, link);
if (!xfr) {
- spin_unlock_irq(&occ->list_lock);
+ spin_unlock_irqrestore(&occ->list_lock, flags);
return;
}
@@ -632,7 +637,7 @@ static void occ_worker(struct work_struct *work)
resp = (struct occ_response *)xfr->buf;
set_bit(XFR_IN_PROGRESS, &xfr->flags);
- spin_unlock_irq(&occ->list_lock);
+ spin_unlock_irqrestore(&occ->list_lock, flags);
mutex_lock(&occ->occ_lock);
start = jiffies;
@@ -686,13 +691,13 @@ static void occ_worker(struct work_struct *work)
xfr->rc = rc;
set_bit(XFR_COMPLETE, &xfr->flags);
- spin_lock_irq(&occ->list_lock);
+ spin_lock_irqsave(&occ->list_lock, flags);
clear_bit(XFR_IN_PROGRESS, &xfr->flags);
list_del(&xfr->link);
empty = list_empty(&occ->xfrs);
- spin_unlock_irq(&occ->list_lock);
+ spin_unlock_irqrestore(&occ->list_lock, flags);
wake_up_interruptible(&client->wait);
occ_put_client(client);
@@ -810,15 +815,16 @@ static int occ_remove(struct platform_device *pdev)
struct occ *occ = platform_get_drvdata(pdev);
struct occ_xfr *xfr;
struct occ_client *client;
+ unsigned long flags;
occ->cancel = true;
- spin_lock_irq(&occ->list_lock);
+ spin_lock_irqsave(&occ->list_lock, flags);
list_for_each_entry(xfr, &occ->xfrs, link) {
client = to_client(xfr);
wake_up_all(&client->wait);
}
- spin_unlock_irq(&occ->list_lock);
+ spin_unlock_irqrestore(&occ->list_lock, flags);
misc_deregister(&occ->mdev);
device_for_each_child(&pdev->dev, NULL, occ_unregister_child);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH linux dev-4.10 3/3] drivers: fsi: occ: switch to irqsave and irqrestore
2018-02-02 22:11 ` [PATCH linux dev-4.10 3/3] drivers: fsi: occ: switch to irqsave and irqrestore Eddie James
@ 2018-02-05 20:14 ` Christopher Bostic
0 siblings, 0 replies; 5+ messages in thread
From: Christopher Bostic @ 2018-02-05 20:14 UTC (permalink / raw)
To: Eddie James, openbmc; +Cc: andrew
Reviewed-by: Christopher Bostic <cbostic@linux.vnet.ibm.com>
On 2/2/18 4:11 PM, Eddie James wrote:
> Fix spinlocking by storing the interrupt state and restoring that state
> when unlocking, instead of blindly disabling and re-enabling all irqs.
>
> Signed-off-by: Eddie James <eajames@linux.vnet.ibm.com>
> ---
> drivers/fsi/occ.c | 44 +++++++++++++++++++++++++-------------------
> 1 file changed, 25 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/fsi/occ.c b/drivers/fsi/occ.c
> index adc64f3..edb5e977 100644
> --- a/drivers/fsi/occ.c
> +++ b/drivers/fsi/occ.c
> @@ -118,18 +118,19 @@ struct occ_client {
> static int occ_enqueue_xfr(struct occ_xfr *xfr)
> {
> int empty;
> + unsigned long flags;
> struct occ_client *client = to_client(xfr);
> struct occ *occ = client->occ;
>
> if (occ->cancel)
> return -ENODEV;
>
> - spin_lock_irq(&occ->list_lock);
> + spin_lock_irqsave(&occ->list_lock, flags);
>
> empty = list_empty(&occ->xfrs);
> list_add_tail(&xfr->link, &occ->xfrs);
>
> - spin_unlock_irq(&occ->list_lock);
> + spin_unlock_irqrestore(&occ->list_lock, flags);
>
> if (empty)
> queue_work(occ_wq, &occ->work);
> @@ -201,6 +202,7 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
> size_t bytes;
> struct occ_xfr *xfr;
> struct occ *occ;
> + unsigned long flags;
>
> if (!client)
> return -ENODEV;
> @@ -212,7 +214,7 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
> xfr = &client->xfr;
> occ = client->occ;
>
> - spin_lock_irq(&client->lock);
> + spin_lock_irqsave(&client->lock, flags);
>
> if (!test_bit(CLIENT_XFR_PENDING, &client->flags)) {
> /* we just finished reading all data, return 0 */
> @@ -232,12 +234,12 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
> goto done;
> }
>
> - spin_unlock_irq(&client->lock);
> + spin_unlock_irqrestore(&client->lock, flags);
>
> rc = wait_event_interruptible(client->wait,
> occ_read_ready(xfr, occ));
>
> - spin_lock_irq(&client->lock);
> + spin_lock_irqsave(&client->lock, flags);
>
> if (!test_bit(XFR_COMPLETE, &xfr->flags)) {
> if (occ->cancel || test_bit(XFR_CANCELED, &xfr->flags))
> @@ -274,7 +276,7 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
> rc = bytes;
>
> done:
> - spin_unlock_irq(&client->lock);
> + spin_unlock_irqrestore(&client->lock, flags);
> occ_put_client(client);
> return rc;
> }
> @@ -293,6 +295,7 @@ static ssize_t occ_write_common(struct occ_client *client,
> {
> int rc;
> unsigned int i;
> + unsigned long flags;
> u16 data_length, checksum = 0;
> struct occ_xfr *xfr;
>
> @@ -305,7 +308,7 @@ static ssize_t occ_write_common(struct occ_client *client,
> occ_get_client(client);
> xfr = &client->xfr;
>
> - spin_lock_irq(&client->lock);
> + spin_lock_irqsave(&client->lock, flags);
>
> if (test_bit(CLIENT_XFR_PENDING, &client->flags)) {
> rc = -EBUSY;
> @@ -353,7 +356,7 @@ static ssize_t occ_write_common(struct occ_client *client,
> rc = len;
>
> done:
> - spin_unlock_irq(&client->lock);
> + spin_unlock_irqrestore(&client->lock, flags);
> occ_put_client(client);
> return rc;
> }
> @@ -370,6 +373,7 @@ static int occ_release_common(struct occ_client *client)
> {
> struct occ *occ;
> struct occ_xfr *xfr;
> + unsigned long flags;
>
> if (!client)
> return -ENODEV;
> @@ -377,13 +381,13 @@ static int occ_release_common(struct occ_client *client)
> xfr = &client->xfr;
> occ = client->occ;
>
> - spin_lock_irq(&client->lock);
> + spin_lock_irqsave(&client->lock, flags);
>
> set_bit(XFR_CANCELED, &xfr->flags);
> if (!test_bit(CLIENT_XFR_PENDING, &client->flags))
> goto done;
>
> - spin_lock_irq(&occ->list_lock);
> + spin_lock(&occ->list_lock);
>
> if (!test_bit(XFR_IN_PROGRESS, &xfr->flags)) {
> /* already deleted from list if complete */
> @@ -391,12 +395,12 @@ static int occ_release_common(struct occ_client *client)
> list_del(&xfr->link);
> }
>
> - spin_unlock_irq(&occ->list_lock);
> + spin_unlock(&occ->list_lock);
>
> wake_up_all(&client->wait);
>
> done:
> - spin_unlock_irq(&client->lock);
> + spin_unlock_irqrestore(&client->lock, flags);
>
> occ_put_client(client);
> return 0;
> @@ -606,6 +610,7 @@ static void occ_worker(struct work_struct *work)
> {
> int rc = 0, empty;
> u16 resp_data_length;
> + unsigned long flags;
> 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);
> @@ -619,11 +624,11 @@ static void occ_worker(struct work_struct *work)
> if (occ->cancel)
> return;
>
> - spin_lock_irq(&occ->list_lock);
> + spin_lock_irqsave(&occ->list_lock, flags);
>
> xfr = list_first_entry_or_null(&occ->xfrs, struct occ_xfr, link);
> if (!xfr) {
> - spin_unlock_irq(&occ->list_lock);
> + spin_unlock_irqrestore(&occ->list_lock, flags);
> return;
> }
>
> @@ -632,7 +637,7 @@ static void occ_worker(struct work_struct *work)
> resp = (struct occ_response *)xfr->buf;
> set_bit(XFR_IN_PROGRESS, &xfr->flags);
>
> - spin_unlock_irq(&occ->list_lock);
> + spin_unlock_irqrestore(&occ->list_lock, flags);
> mutex_lock(&occ->occ_lock);
>
> start = jiffies;
> @@ -686,13 +691,13 @@ static void occ_worker(struct work_struct *work)
> xfr->rc = rc;
> set_bit(XFR_COMPLETE, &xfr->flags);
>
> - spin_lock_irq(&occ->list_lock);
> + spin_lock_irqsave(&occ->list_lock, flags);
>
> clear_bit(XFR_IN_PROGRESS, &xfr->flags);
> list_del(&xfr->link);
> empty = list_empty(&occ->xfrs);
>
> - spin_unlock_irq(&occ->list_lock);
> + spin_unlock_irqrestore(&occ->list_lock, flags);
>
> wake_up_interruptible(&client->wait);
> occ_put_client(client);
> @@ -810,15 +815,16 @@ static int occ_remove(struct platform_device *pdev)
> struct occ *occ = platform_get_drvdata(pdev);
> struct occ_xfr *xfr;
> struct occ_client *client;
> + unsigned long flags;
>
> occ->cancel = true;
>
> - spin_lock_irq(&occ->list_lock);
> + spin_lock_irqsave(&occ->list_lock, flags);
> list_for_each_entry(xfr, &occ->xfrs, link) {
> client = to_client(xfr);
> wake_up_all(&client->wait);
> }
> - spin_unlock_irq(&occ->list_lock);
> + spin_unlock_irqrestore(&occ->list_lock, flags);
>
> misc_deregister(&occ->mdev);
> device_for_each_child(&pdev->dev, NULL, occ_unregister_child);
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-02-05 20:14 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-02-02 22:11 [PATCH linux dev-4.10 0/3] drivers: fsi: Fixup SBEFIFO and OCC locking Eddie James
2018-02-02 22:11 ` [PATCH linux dev-4.10 1/3] drivers: fsi: SBEFIFO: switch to workqueue instead of timer Eddie James
2018-02-02 22:11 ` [PATCH linux dev-4.10 2/3] drivers: fsi: sbefifo: switch to mutex in work function Eddie James
2018-02-02 22:11 ` [PATCH linux dev-4.10 3/3] drivers: fsi: occ: switch to irqsave and irqrestore Eddie James
2018-02-05 20:14 ` Christopher Bostic
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.