[PATCH linux dev-4.10] fsi: occ: Fix spin_lock_irq errors
Joel Stanley
joel at jms.id.au
Fri Jul 7 16:00:52 AEST 2017
We are seeing warnings whenever this driver is in use:
WARNING: CPU: 0 PID: 991 at kernel/softirq.c:162 __local_bh_enable_ip+0xa0/0xe0
CPU: 0 PID: 991 Comm: openpower-occ-c Not tainted 4.10.17-00313-g9165bdc8c418 #497
Hardware name: ASpeed SoC
[<80010060>] (unwind_backtrace) from [<8000d924>] (show_stack+0x20/0x24)
[<8000d924>] (show_stack) from [<801daedc>] (dump_stack+0x20/0x28)
[<801daedc>] (dump_stack) from [<80018d04>] (__warn+0xe0/0x108)
[<80018d04>] (__warn) from [<80018df8>] (warn_slowpath_null+0x30/0x38)
[<80018df8>] (warn_slowpath_null) from [<8001c350>] (__local_bh_enable_ip+0xa0/0xe0)
[<8001c350>] (__local_bh_enable_ip) from [<803afdd0>] (unix_release_sock+0x68/0x250)
[<803afdd0>] (unix_release_sock) from [<803affe4>] (unix_release+0x2c/0x3c)
[<803affe4>] (unix_release) from [<803153a8>] (sock_release+0x2c/0xa0)
[<803153a8>] (sock_release) from [<80315438>] (sock_close+0x1c/0x24)
[<80315438>] (sock_close) from [<800e3e60>] (__fput+0x94/0x1c4)
[<800e3e60>] (__fput) from [<800e3ff8>] (____fput+0x18/0x1c)
[<800e3ff8>] (____fput) from [<80031d34>] (task_work_run+0x78/0xa0)
[<80031d34>] (task_work_run) from [<8001b2b4>] (do_exit+0x274/0x8e0)
[<8001b2b4>] (do_exit) from [<8001b9b8>] (do_group_exit+0x4c/0xb4)
[<8001b9b8>] (do_group_exit) from [<800253a4>] (get_signal+0x290/0x5b4)
[<800253a4>] (get_signal) from [<8000ccf0>] (do_signal+0xd4/0x3dc)
[<8000ccf0>] (do_signal) from [<8000d1d4>] (do_work_pending+0xac/0xbc)
[<8000d1d4>] (do_work_pending) from [<8000a58c>] (slow_work_pending+0xc/0x20)
The driver lack spin_unlock_irq calls when releasing locks. Add them.
Also add whitespace around the locking calls so they stand out.
Signed-off-by: Joel Stanley <joel at jms.id.au>
---
Eddie, the locking in this driver is a bit scary. We need to get the driver
properly reviewed as a priority, or else it will need to be removed from the
tree until it is fixed.
This patch is enough to avoid confusing users with WARN_ONs. It's not a full
review or fix for the locking.
drivers/fsi/occ.c | 43 ++++++++++++++++++++++++++++---------------
1 file changed, 28 insertions(+), 15 deletions(-)
diff --git a/drivers/fsi/occ.c b/drivers/fsi/occ.c
index f43ae51cd2e0..d984e519a80e 100644
--- a/drivers/fsi/occ.c
+++ b/drivers/fsi/occ.c
@@ -123,9 +123,11 @@ static void occ_enqueue_xfr(struct occ_xfr *xfr)
struct occ *occ = client->occ;
spin_lock_irq(&occ->list_lock);
+
empty = list_empty(&occ->xfrs);
list_add_tail(&xfr->link, &occ->xfrs);
- spin_unlock(&occ->list_lock);
+
+ spin_unlock_irq(&occ->list_lock);
if (empty)
queue_work(occ_wq, &occ->work);
@@ -175,6 +177,7 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
return -EINVAL;
spin_lock_irq(&client->lock);
+
if (!test_bit(CLIENT_XFR_PENDING, &client->flags)) {
/* we just finished reading all data, return 0 */
if (client->read_offset) {
@@ -193,15 +196,17 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
}
set_bit(XFR_WAITING, &xfr->flags);
- spin_unlock(&client->lock);
+
+ spin_unlock_irq(&client->lock);
rc = wait_event_interruptible(client->wait,
test_bit(XFR_COMPLETE, &xfr->flags) ||
test_bit(XFR_CANCELED, &xfr->flags));
spin_lock_irq(&client->lock);
+
if (test_bit(XFR_CANCELED, &xfr->flags)) {
- spin_unlock(&client->lock);
+ spin_unlock_irq(&client->lock);
kfree(client);
return -EBADFD;
}
@@ -236,7 +241,7 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
rc = bytes;
done:
- spin_unlock(&client->lock);
+ spin_unlock_irq(&client->lock);
return rc;
}
@@ -307,7 +312,7 @@ static ssize_t occ_write_common(struct occ_client *client,
rc = len;
done:
- spin_unlock(&client->lock);
+ spin_unlock_irq(&client->lock);
return rc;
}
@@ -331,36 +336,39 @@ static int occ_release_common(struct occ_client *client)
struct occ *occ = client->occ;
spin_lock_irq(&client->lock);
+
if (!test_bit(CLIENT_XFR_PENDING, &client->flags)) {
- spin_unlock(&client->lock);
+ spin_unlock_irq(&client->lock);
kfree(client);
return 0;
}
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(&occ->list_lock);
+ 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(&client->lock);
+ spin_unlock_irq(&client->lock);
return 0;
}
- spin_unlock(&client->lock);
+ spin_unlock_irq(&client->lock);
+
kfree(client);
return 0;
}
/* operation is in progress; let worker clean up*/
- spin_unlock(&occ->list_lock);
- spin_unlock(&client->lock);
+ spin_unlock_irq(&occ->list_lock);
+ spin_unlock_irq(&client->lock);
return 0;
}
@@ -563,15 +571,16 @@ static void occ_worker(struct work_struct *work)
again:
spin_lock_irq(&occ->list_lock);
+
xfr = list_first_entry(&occ->xfrs, struct occ_xfr, link);
if (!xfr) {
- spin_unlock(&occ->list_lock);
+ spin_unlock_irq(&occ->list_lock);
return;
}
set_bit(XFR_IN_PROGRESS, &xfr->flags);
- spin_unlock(&occ->list_lock);
+ spin_unlock_irq(&occ->list_lock);
mutex_lock(&occ->occ_lock);
rc = occ_putsram(sbefifo, 0xFFFBE000, xfr->buf,
@@ -611,16 +620,20 @@ static void occ_worker(struct work_struct *work)
/* 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(&client->lock);
+
+ 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(&occ->list_lock);
+
+ spin_unlock_irq(&occ->list_lock);
if (waiting)
wake_up_interruptible(&client->wait);
--
2.13.2
More information about the openbmc
mailing list