[PATCH linux dev-4.13 14/16] fsi: sbefifo: Switch list_lock from spinlock to mutex
Eddie James
eajames at linux.vnet.ibm.com
Fri Feb 23 07:35:39 AEDT 2018
On 02/19/2018 10:18 PM, Andrew Jeffery wrote:
> We allocate GFP_KERNEL memory for a list element in sbefifo_enq_xfr() (elided
> from the stack trace presumably by inlining) but do so under list_lock, which
> was previously a spin lock. Enabling lock debugging gives the following
> warning:
Acked-by: Eddie James <eajames at linux.vnet.ibm.com>
>
> [ 188.830000] BUG: sleeping function called from invalid context at mm/slab.h:408
> [ 188.830000] in_atomic(): 1, irqs_disabled(): 128, pid: 110, name: kworker/u2:9
> [ 188.830000] INFO: lockdep is turned off.
> [ 188.830000] irq event stamp: 9002
> [ 188.830000] hardirqs last enabled at (9001): [<801105bc>] kmem_cache_alloc_trace+0x8c/0x284
> [ 188.830000] hardirqs last disabled at (9002): [<8049458c>] _raw_spin_lock_irqsave+0x2c/0xa0
> [ 188.830000] softirqs last enabled at (5370): [<8000987c>] __do_softirq+0x38c/0x510
> [ 188.830000] softirqs last disabled at (5363): [<8001f154>] irq_exit+0x120/0x16c
> [ 188.830000] CPU: 0 PID: 110 Comm: kworker/u2:9 Tainted: G W 4.10.17-00534-gb846201e7a2d #2277
> [ 188.830000] Hardware name: ASpeed SoC
> [ 188.830000] Workqueue: occ occ_worker
> [ 188.830000] [<800108f8>] (unwind_backtrace) from [<8000e02c>] (show_stack+0x20/0x24)
> [ 188.830000] [<8000e02c>] (show_stack) from [<8022d1c0>] (dump_stack+0x20/0x28)
> [ 188.830000] [<8022d1c0>] (dump_stack) from [<80048bf8>] (___might_sleep+0x174/0x2ac)
> [ 188.830000] [<80048bf8>] (___might_sleep) from [<80048da0>] (__might_sleep+0x70/0xb0)
> [ 188.830000] [<80048da0>] (__might_sleep) from [<801106f8>] (kmem_cache_alloc_trace+0x1c8/0x284)
> [ 188.830000] [<801106f8>] (kmem_cache_alloc_trace) from [<803330e8>] (sbefifo_write_common+0x228/0x4b4)
> [ 188.830000] [<803330e8>] (sbefifo_write_common) from [<80333740>] (sbefifo_drv_write+0x24/0x28)
> [ 188.830000] [<80333740>] (sbefifo_drv_write) from [<80333b58>] (occ_write_sbefifo+0x44/0x60)
> [ 188.830000] [<80333b58>] (occ_write_sbefifo) from [<80334530>] (occ_worker+0x38/0x4a0)
> [ 188.830000] [<80334530>] (occ_worker) from [<80034a2c>] (process_one_work+0x23c/0x6d4)
> [ 188.830000] [<80034a2c>] (process_one_work) from [<80034f08>] (worker_thread+0x44/0x528)
> [ 188.830000] [<80034f08>] (worker_thread) from [<8003c8c0>] (kthread+0x160/0x17c)
> [ 188.830000] [<8003c8c0>] (kthread) from [<8000a888>] (ret_from_fork+0x14/0x2c)
>
> Avoid the warning (or using GFP_ATOMIC memory) by switching to a mutex, as we
> only require mutual exclusion, not pre-emption be disabled (occ_worker() is
> executed on a workqueue). This should also reduce the latency impact of calling
> into the OCC.
>
> Signed-off-by: Andrew Jeffery <andrew at aj.id.au>
> ---
> drivers/fsi/fsi-sbefifo.c | 35 ++++++++++++++++-------------------
> 1 file changed, 16 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/fsi/fsi-sbefifo.c b/drivers/fsi/fsi-sbefifo.c
> index 5b12eec2602b..cc9b9e36ac72 100644
> --- a/drivers/fsi/fsi-sbefifo.c
> +++ b/drivers/fsi/fsi-sbefifo.c
> @@ -67,7 +67,7 @@ struct sbefifo {
> wait_queue_head_t wait;
> struct list_head xfrs;
> struct kref kref;
> - spinlock_t list_lock; /* lock access to the xfrs list */
> + struct mutex list_lock; /* lock access to the xfrs list */
> struct mutex sbefifo_lock; /* lock access to the hardware */
> char name[32];
> int idx;
> @@ -399,12 +399,11 @@ static void sbefifo_worker(struct work_struct *work)
> int ret = 0;
> u32 sts;
> int i;
> - unsigned long flags;
>
> - spin_lock_irqsave(&sbefifo->list_lock, flags);
> + mutex_lock(&sbefifo->list_lock);
> xfr = list_first_entry_or_null(&sbefifo->xfrs, struct sbefifo_xfr,
> xfrs);
> - spin_unlock_irqrestore(&sbefifo->list_lock, flags);
> + mutex_unlock(&sbefifo->list_lock);
> if (!xfr)
> return;
>
> @@ -516,9 +515,9 @@ static void sbefifo_worker(struct work_struct *work)
>
> set_bit(SBEFIFO_XFR_COMPLETE, &xfr->flags);
>
> - spin_lock_irqsave(&sbefifo->list_lock, flags);
> + mutex_lock(&sbefifo->list_lock);
> list_del(&xfr->xfrs);
> - spin_unlock_irqrestore(&sbefifo->list_lock, flags);
> + mutex_unlock(&sbefifo->list_lock);
>
> if (unlikely(test_bit(SBEFIFO_XFR_CANCEL,
> &xfr->flags)))
> @@ -535,17 +534,17 @@ static void sbefifo_worker(struct work_struct *work)
> dev_err(&sbefifo->fsi_dev->dev,
> "Fatal bus access failure: %d\n", ret);
>
> - spin_lock_irqsave(&sbefifo->list_lock, flags);
> + mutex_lock(&sbefifo->list_lock);
> 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);
> + mutex_unlock(&sbefifo->list_lock);
> } else if (eot) {
> - spin_lock_irqsave(&sbefifo->list_lock, flags);
> + mutex_lock(&sbefifo->list_lock);
> xfr = sbefifo_next_xfr(sbefifo);
> - spin_unlock_irqrestore(&sbefifo->list_lock, flags);
> + mutex_unlock(&sbefifo->list_lock);
>
> if (xfr) {
> wake_up_interruptible(&sbefifo->wait);
> @@ -715,7 +714,6 @@ 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;
> @@ -726,26 +724,26 @@ static ssize_t sbefifo_write_common(struct sbefifo_client *client,
> sbefifo_get_client(client);
> n = sbefifo_buf_nbwriteable(&client->wbuf);
>
> - spin_lock_irqsave(&sbefifo->list_lock, flags);
> + mutex_lock(&sbefifo->list_lock);
>
> /* next xfr to be executed */
> xfr = list_first_entry_or_null(&sbefifo->xfrs, struct sbefifo_xfr,
> xfrs);
>
> if ((client->f_flags & O_NONBLOCK) && xfr && n < len) {
> - spin_unlock_irqrestore(&sbefifo->list_lock, flags);
> + mutex_unlock(&sbefifo->list_lock);
> ret = -EAGAIN;
> goto out;
> }
>
> xfr = sbefifo_enq_xfr(client); /* this xfr queued up */
> if (IS_ERR(xfr)) {
> - spin_unlock_irqrestore(&sbefifo->list_lock, flags);
> + mutex_unlock(&sbefifo->list_lock);
> ret = PTR_ERR(xfr);
> goto out;
> }
>
> - spin_unlock_irqrestore(&sbefifo->list_lock, flags);
> + mutex_unlock(&sbefifo->list_lock);
>
> /*
> * Partial writes are not really allowed in that EOT is sent exactly
> @@ -954,7 +952,7 @@ static int sbefifo_probe(struct device *dev)
> }
> }
>
> - spin_lock_init(&sbefifo->list_lock);
> + mutex_init(&sbefifo->list_lock);
> mutex_init(&sbefifo->sbefifo_lock);
> kref_init(&sbefifo->kref);
> init_waitqueue_head(&sbefifo->wait);
> @@ -998,11 +996,10 @@ static int sbefifo_remove(struct device *dev)
> {
> struct sbefifo *sbefifo = dev_get_drvdata(dev);
> struct sbefifo_xfr *xfr, *tmp;
> - unsigned long flags;
>
> /* lock the sbefifo so to prevent deleting an ongoing xfr */
> mutex_lock(&sbefifo->sbefifo_lock);
> - spin_lock_irqsave(&sbefifo->list_lock, flags);
> + mutex_lock(&sbefifo->list_lock);
>
> WRITE_ONCE(sbefifo->rc, -ENODEV);
> list_for_each_entry_safe(xfr, tmp, &sbefifo->xfrs, xfrs) {
> @@ -1012,7 +1009,7 @@ static int sbefifo_remove(struct device *dev)
>
> INIT_LIST_HEAD(&sbefifo->xfrs);
>
> - spin_unlock_irqrestore(&sbefifo->list_lock, flags);
> + mutex_unlock(&sbefifo->list_lock);
> mutex_unlock(&sbefifo->sbefifo_lock);
>
> wake_up_all(&sbefifo->wait);
More information about the openbmc
mailing list