[PATCH linux dev-4.10] fsi: occ: Fix spin_lock_irq errors
Eddie James
eajames at linux.vnet.ibm.com
Sat Jul 8 01:15:44 AEST 2017
On 07/07/2017 01:00 AM, Joel Stanley wrote:
> 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.
It is on the lkml.
This patch tests well, no more warning.
Thanks!
Acked-by: Eddie James <eajames at linux.vnet.ibm.com>
>
> 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);
More information about the openbmc
mailing list