Linux/66AK2H12: BUG: in RapidIO enumeration

Charles Coldwell

Intellectual 420 points

Part Number: 66AK2H12

Tool/software: Linux

The functions keystone_rio_maint_read and keystone_rio_maint_write
make two calls that could potentially sleep: kzalloc

tbuf = kzalloc(align_len, GFP_NOFS | GFP_DMA);
if (!tbuf)
return -ENOMEM;

and mutex_lock

mutex_lock(&krio_priv->lsu_lock_maint);

(the latter in the inline function keystone_rio_maint_request).

This means keystone_rio_maint_read and keystone_rio_maint_write must
not be called from interrupt context, nor when holding spinlocks.

The function rio_mport_read_config_32 (rio-access.c) locks
rio_config_lock, then calls keystone_rio_maint_read through the pointer
mport->ops->cread. This is not safe and could deadlock the kernel.

The RapidIO enumeration mechanism ends up calling
rio_mport_read_config_32, so we can't enumerate without generating
BUGs.

[ 29.486937] RIO: enumerate master port 0, RIO0 mport
[ 29.493231] BUG: sleeping function called from invalid context at /home/MITLL/CH22073/ojm/keystoneii/processor-sdk-linux/kernel/locking/mutex.c:620
[ 29.506499] in_atomic(): 1, irqs_disabled(): 128, pid: 612, name: modprobe
[ 29.513398] 2 locks held by modprobe/612:
[ 29.517417] #0: (rio_mport_list_lock){+.+.+.}, at: [<c0352b28>] rio_init_mports+0x34/0x2a4
[ 29.525923] #1: (rio_config_lock){......}, at: [<c0354944>] rio_mport_read_config_32+0x40/0xa0
[ 29.534770] irq event stamp: 18684
[ 29.538179] hardirqs last enabled at (18683): [<c06ec7ac>] _raw_spin_unlock_irqrestore+0x6c/0x74
[ 29.547093] hardirqs last disabled at (18684): [<c06ec5cc>] _raw_spin_lock_irqsave+0x1c/0x58
[ 29.555569] softirqs last enabled at (16960): [<c002a924>] __do_softirq+0x234/0x2b0
[ 29.563346] softirqs last disabled at (16937): [<c002ad00>] irq_exit+0xd8/0x114
[ 29.570687] Preemption disabled at:[< (null)>] (null)
[ 29.576019]
[ 29.577513] CPU: 0 PID: 612 Comm: modprobe Tainted: G W 4.4.32-W1CMC-g1c11a44-dirty #53
[ 29.586679] Hardware name: Keystone
[ 29.590183] [<c00180c8>] (unwind_backtrace) from [<c0013d18>] (show_stack+0x10/0x14)
[ 29.597957] [<c0013d18>] (show_stack) from [<c02e9888>] (dump_stack+0x98/0xc4)
[ 29.605208] [<c02e9888>] (dump_stack) from [<c06e7f9c>] (mutex_lock_nested+0x30/0x4b4)
[ 29.613157] [<c06e7f9c>] (mutex_lock_nested) from [<c035b3f8>] (keystone_rio_maint_read+0x12c/0x400)
[ 29.622326] [<c035b3f8>] (keystone_rio_maint_read) from [<c035b700>] (keystone_rio_config_read+0x34/0x3c)
[ 29.631931] [<c035b700>] (keystone_rio_config_read) from [<c0354978>] (rio_mport_read_config_32+0x74/0xa0)
[ 29.641623] [<c0354978>] (rio_mport_read_config_32) from [<c03526f8>] (rio_mport_chk_dev_access+0x44/0x74)
[ 29.651322] [<c03526f8>] (rio_mport_chk_dev_access) from [<bf21ab9c>] (rio_enum_peer+0x34/0x76c [rio_scan])
[ 29.661111] [<bf21ab9c>] (rio_enum_peer [rio_scan]) from [<bf21b524>] (rio_enum_mport+0x134/0x2b8 [rio_scan])
[ 29.671071] [<bf21b524>] (rio_enum_mport [rio_scan]) from [<c0352b90>] (rio_init_mports+0x9c/0x2a4)
[ 29.680157] [<c0352b90>] (rio_init_mports) from [<bf21e034>] (rio_basic_attach+0x34/0x40 [rio_scan])
[ 29.689332] [<bf21e034>] (rio_basic_attach [rio_scan]) from [<c0009840>] (do_one_initcall+0xbc/0x204)
[ 29.698590] [<c0009840>] (do_one_initcall) from [<c00dbe5c>] (do_init_module+0x60/0x3a4)
[ 29.706713] [<c00dbe5c>] (do_init_module) from [<c00affc4>] (load_module+0x1dc8/0x2370)
[ 29.714748] [<c00affc4>] (load_module) from [<c00b078c>] (SyS_finit_module+0x8c/0xc8)
[ 29.722609] [<c00b078c>] (SyS_finit_module) from [<c0010380>] (ret_fast_syscall+0x0/0x1c)

over 8 years ago

0 Charles Coldwell over 8 years ago

Intellectual 420 points


This patch seems to fix the problem: use GFP_ATOMIC and convert the mutex to a spinlock.

diff --git a/drivers/rapidio/devices/keystone_rio.c b/drivers/rapidio/devices/keystone_rio.c
index 9288d2a..e165ff8 100644
--- a/drivers/rapidio/devices/keystone_rio.c
+++ b/drivers/rapidio/devices/keystone_rio.c
@@ -712,7 +712,7 @@ static int keystone_rio_dbell_send(struct rio_mport *mport,
        u32 packet_type = ((info & 0xffff) << 16)
                                | KEYSTONE_RIO_PACKET_TYPE_DBELL;
 
-       spin_lock(&krio_priv->lsu_lock_maint);
+       mutex_lock(&krio_priv->lsu_lock_maint);
        res = keystone_rio_lsu_start_transfer(krio_priv->lsu_maint,
                                              mport->index,
                                              dest_id,
@@ -725,7 +725,7 @@ static int keystone_rio_dbell_send(struct rio_mport *mport,
                                              1,
                                              krio_priv);
        if (res) {
-               spin_unlock(&krio_priv->lsu_lock_maint);
+               mutex_unlock(&krio_priv->lsu_lock_maint);
                dev_err(krio_priv->dev, "DIO: send doorbell error %d\n", res);
                return res;
        }
@@ -746,7 +746,7 @@ static int keystone_rio_dbell_send(struct rio_mport *mport,
                ndelay(KEYSTONE_RIO_TIMEOUT_NSEC);
        }
 
-       spin_unlock(&krio_priv->lsu_lock_maint);
+       mutex_unlock(&krio_priv->lsu_lock_maint);
 
        dev_info(krio_priv->dev, "DIO: send doorbell complete %u\n", count);
        return res;
@@ -788,7 +788,7 @@ static inline int keystone_rio_maint_request(
        u32 count = 0;
        u32 type = ((hopcount & 0xff) << 8) | (packet_type & 0xff);
 
-       spin_lock(&krio_priv->lsu_lock_maint);
+       mutex_lock(&krio_priv->lsu_lock_maint);
 
        res = keystone_rio_lsu_start_transfer(krio_priv->lsu_maint,
                                              port_id,
@@ -802,7 +802,7 @@ static inline int keystone_rio_maint_request(
                                              0,
                                              krio_priv);
        if (res) {
-               spin_unlock(&krio_priv->lsu_lock_maint);
+               mutex_unlock(&krio_priv->lsu_lock_maint);
                dev_err(krio_priv->dev, "maintenance packet transfer error\n");
                return res;
        }
@@ -823,7 +823,7 @@ static inline int keystone_rio_maint_request(
                ndelay(KEYSTONE_RIO_TIMEOUT_NSEC);
        }
 
-       spin_unlock(&krio_priv->lsu_lock_maint);
+       mutex_unlock(&krio_priv->lsu_lock_maint);
 
        return res;
 }
@@ -843,15 +843,11 @@ static int keystone_rio_maint_read(struct keystone_rio_data *krio_priv,
        struct device *dev = krio_priv->dev;
        size_t align_len = L1_CACHE_ALIGN(len);
 
-       tbuf = kzalloc(align_len, GFP_ATOMIC | GFP_DMA);
+       tbuf = kzalloc(align_len, GFP_KERNEL | GFP_DMA);
        if (!tbuf)
                return -ENOMEM;
 
        dma = dma_map_single(dev, tbuf, len, DMA_FROM_DEVICE);
-       if (dma_mapping_error(dev, dma)) {
-               kfree(tbuf);
-               return -ENOMEM;
-       }
 
        res = keystone_rio_maint_request(port_id,
                                         destid,
@@ -902,7 +898,7 @@ static int keystone_rio_maint_write(struct keystone_rio_data *krio_priv,
        struct device *dev = krio_priv->dev;
        size_t align_len = L1_CACHE_ALIGN(len);
 
-       tbuf = kzalloc(align_len, GFP_ATOMIC | GFP_DMA);
+       tbuf = kzalloc(align_len, GFP_KERNEL | GFP_DMA);
        if (!tbuf)
                return -ENOMEM;
 
@@ -920,10 +916,6 @@ static int keystone_rio_maint_write(struct keystone_rio_data *krio_priv,
        }
 
        dma = dma_map_single(dev, tbuf, len, DMA_TO_DEVICE);
-       if (dma_mapping_error(dev, dma)) {
-               kfree(tbuf);
-               return -ENOMEM;
-       }
 
        res = keystone_rio_maint_request(port_id,
                                         destid,
@@ -3305,7 +3297,7 @@ static int keystone_rio_probe(struct platform_device *pdev)
        krio_priv->lsu_free = krio_priv->lsu_start;
        krio_priv->lsu_maint = keystone_rio_lsu_alloc(krio_priv);
 
-       spin_lock_init(&krio_priv->lsu_lock_maint);
+       mutex_init(&krio_priv->lsu_lock_maint);
 
        spin_lock_init(&krio_priv->port_chk_lock);
 
diff --git a/drivers/rapidio/devices/keystone_rio.h b/drivers/rapidio/devices/keystone_rio.h
index 556a5c2..d19efc8 100644
--- a/drivers/rapidio/devices/keystone_rio.h
+++ b/drivers/rapidio/devices/keystone_rio.h
@@ -484,7 +484,7 @@ struct keystone_rio_data {
        u32                     calibrating;
 
        /* To protect against concurrent lsu maint access */
-       spinlock_t              lsu_lock_maint;
+       struct mutex            lsu_lock_maint;
        u8                      lsu_start;
        u8                      lsu_end;
        u8                      lsu_free;