rpmsg: fix lockdep warnings in virtio rpmsg bus driver
authorAngela Stegmaier <angelabaker@ti.com>
Sat, 4 Mar 2017 22:29:19 +0000 (16:29 -0600)
committerSuman Anna <s-anna@ti.com>
Fri, 8 Mar 2019 02:02:56 +0000 (20:02 -0600)
The virtio rpmsg bus framework uses endpoints as the basis for
sending and receiving messages to/from a remote processor. Each
rpmsg bus device will have a primary endpoint if the corresponding
rpmsg bus driver supports a callback, and secondary child endpoints
associated with the same rpmsg bus device. The life-cycle of these
endpoints are tied to the corresponding rpmsg device. A virtio rpmsg
bus device can also have its own endpoint for supporting name service
announcements from a corresponding remote processor to create and
delete rpmsg devices dynamically.

Each endpoint has a callback lock associated with it to provide
protection/mutual exclusion between threads that process incoming
rpmsg messages and threads that want to delete the endpoint. The
virtio rpmsg name service endpoint callback will run while holding
it's ept->cb_lock to create/delete rpmsg devices for RPMSG_NS_CREATE
and RPMSG_NS_DELETE messages respectively. The latter message
processing will destroy the requested channel, and will ultimately
result in all the secondary rpmsg device endpoints also to be
destroyed. The ept->cb_lock for the channel's endpoint is also
locked during its destruction while setting the callback to NULL.
This results in a seemingly nested locking of the ept->cb_lock even
though the locking is on different mutexes. This will result in a
false warning from the lockdep validator when it is enabled because
the lockdep deals with classes and both are the same class, although
they are different instances.

Similar circular dependency scenarios also exist with remoteproc
error recovery and existing rpmsg drivers - rpmsg_rpc and rpmsg_proto.

These issues are fixed by replacing the existing mutex_lock() calls
with the mutex_lock_nested() API variation and using different
subclasses for the NameService end-point and for the rpmsg channel
device end-points.

Following are example warning signatures that get fixed by this patch:

1. Recursive locking dependency during RPMSG_NS_DESTROY message processing
 =============================================
 WARNING: possible recursive locking detected
 ---------------------------------------------
 kworker/0:1/23 is trying to acquire lock:
 9fded1aa (&ept->cb_lock){+.+.}, at: __rpmsg_destroy_ept+0x40/0x6c [virtio_rpmsg_bus]

 but task is already holding lock:
 b33d53aa (&ept->cb_lock){+.+.}, at: rpmsg_recv_done+0x68/0x344 [virtio_rpmsg_bus]

 other info that might help us debug this:
  Possible unsafe locking scenario:

        CPU0
        ----
   lock(&ept->cb_lock);
   lock(&ept->cb_lock);

  *** DEADLOCK ***

  May be due to missing lock nesting notatio

2. Circular locking dependency during error recovery of rpmsg-rpc driver
 ======================================================
 WARNING: possible circular locking dependency detected
 -------------------------------------------------------
 kworker/0:2/69 is trying to acquire lock:
 2e551f56 (&ept->cb_lock){+.+.}, at: __rpmsg_destroy_ept+0x40/0x6c [virtio_rpmsg_bus]

 but task is already holding lock:
 a034cfcf (&rppcdev->lock){+.+.}, at: rppc_remove+0x8c/0x244 [rpmsg_rpc]

 which lock already depends on the new lock.

 other info that might help us debug this:

 Chain exists of:
  &ept->cb_lock --> rppc_devices_lock --> &rppcdev->lock

  Possible unsafe locking scenario:
        CPU0                    CPU1
        ----                    ----
   lock(&rppcdev->lock);
                                lock(rppc_devices_lock);
                                lock(&rppcdev->lock);
   lock(&ept->cb_lock);

  *** DEADLOCK ***

3. Circular locking dependency during error recovery of rpmsg-proto driver
 ======================================================
 WARNING: possible circular locking dependency detected
 -------------------------------------------------------
 kworker/0:1/23 is trying to acquire lock:
 b012b19d (&ept->cb_lock){+.+.}, at: __rpmsg_destroy_ept+0x40/0x6c [virtio_rpmsg_bus]

 but task is already holding lock:
 d88d2852 (rpmsg_channels_lock){+.+.}, at: rpmsg_proto_remove+0x28/0x170 [rpmsg_proto]

 which lock already depends on the new lock.

 other info that might help us debug this:

  Possible unsafe locking scenario:

        CPU0                    CPU1
        ----                    ----
   lock(rpmsg_channels_lock);
                                lock(&ept->cb_lock);
                                lock(rpmsg_channels_lock);
   lock(&ept->cb_lock);

  *** DEADLOCK ***

Signed-off-by: Angela Stegmaier <angelabaker@ti.com>
[s-anna@ti.com: flip the subclass values, update patch description for 4.19]
Signed-off-by: Suman Anna <s-anna@ti.com>
drivers/rpmsg/virtio_rpmsg_bus.c
include/linux/rpmsg.h

index 9c4ff0e34a379f6baaa4cca539b3716a3a2682ce..d9f401dd253402cccf1035722d78712c6bea7673 100644 (file)
@@ -275,6 +275,9 @@ static struct rpmsg_endpoint *__rpmsg_create_ept(struct virtproc_info *vrp,
                goto free_ept;
        }
        ept->addr = id;
+       ept->cb_lockdep_class = ((ept->addr == RPMSG_NS_ADDR) ?
+                                RPMSG_LOCKDEP_SUBCLASS_NS :
+                                RPMSG_LOCKDEP_SUBCLASS_NORMAL);
 
        mutex_unlock(&vrp->endpoints_lock);
 
@@ -315,7 +318,7 @@ __rpmsg_destroy_ept(struct virtproc_info *vrp, struct rpmsg_endpoint *ept)
        mutex_unlock(&vrp->endpoints_lock);
 
        /* make sure in-flight inbound messages won't invoke cb anymore */
-       mutex_lock(&ept->cb_lock);
+       mutex_lock_nested(&ept->cb_lock, ept->cb_lockdep_class);
        ept->cb = NULL;
        mutex_unlock(&ept->cb_lock);
 
@@ -745,7 +748,7 @@ static int rpmsg_recv_single(struct virtproc_info *vrp, struct device *dev,
 
        if (ept) {
                /* make sure ept->cb doesn't go away while we use it */
-               mutex_lock(&ept->cb_lock);
+               mutex_lock_nested(&ept->cb_lock, ept->cb_lockdep_class);
 
                if (ept->cb)
                        ept->cb(ept->rpdev, msg->data, msg->len, ept->priv,
index 436faf04ba1ca78ce5cadb91857a3c36147e440b..42d62a76c0e2741bd43a383660978f224759c697 100644 (file)
@@ -25,6 +25,10 @@ struct rpmsg_endpoint;
 struct rpmsg_device_ops;
 struct rpmsg_endpoint_ops;
 
+/* lockdep subclasses for use with ept cb_lock mutex nested calls */
+#define RPMSG_LOCKDEP_SUBCLASS_NORMAL   0 /* regular ept cb_lock */
+#define RPMSG_LOCKDEP_SUBCLASS_NS       1 /* name service ept cb_lock */
+
 /**
  * struct rpmsg_channel_info - channel info representation
  * @name: name of service
@@ -71,6 +75,7 @@ typedef int (*rpmsg_rx_cb_t)(struct rpmsg_device *, void *, int, void *, u32);
  * @refcount: when this drops to zero, the ept is deallocated
  * @cb: rx callback handler
  * @cb_lock: must be taken before accessing/changing @cb
+ * @cb_lockdep_class: mutex lockdep class to be used with @cb_lock
  * @addr: local rpmsg address
  * @priv: private data for the driver's use
  *
@@ -93,6 +98,7 @@ struct rpmsg_endpoint {
        struct kref refcount;
        rpmsg_rx_cb_t cb;
        struct mutex cb_lock;
+       int cb_lockdep_class;
        u32 addr;
        void *priv;