From 94dd16e64207cc43f1a7dd1b13604b85aa83e6fa Mon Sep 17 00:00:00 2001
From: George Kulakowski <kulakowski@google.com>
Date: Thu, 29 Nov 2018 11:22:41 -0800
Subject: [PATCH] [devmgr] Use RAII for the devhost API biglock
Test: clang lock annotation checking
Change-Id: I4f708e1449db34f0694ce1635eecd8d272f460f6
---
system/core/devmgr/devhost/api.cpp | 76 ++++++++------------------
system/core/devmgr/devhost/core.cpp | 32 +++++------
system/core/devmgr/devhost/devhost.cpp | 8 ++-
system/core/devmgr/devhost/lock.h | 20 +++++++
4 files changed, 62 insertions(+), 74 deletions(-)
diff --git a/system/core/devmgr/devhost/api.cpp b/system/core/devmgr/devhost/api.cpp
index eb0c1606655..ef1717f662f 100644
--- a/system/core/devmgr/devhost/api.cpp
+++ b/system/core/devmgr/devhost/api.cpp
@@ -52,10 +52,9 @@ __EXPORT zx_status_t device_add_from_driver(zx_driver_t* drv, zx_device_t* paren
return ZX_ERR_INVALID_ARGS;
}
- DM_LOCK();
+ ApiAutoLock lock;
r = devhost_device_create(drv, parent_ref, args->name, args->ctx, args->ops, &dev);
if (r != ZX_OK) {
- DM_UNLOCK();
return r;
}
if (args->proto_id) {
@@ -96,35 +95,27 @@ __EXPORT zx_status_t device_add_from_driver(zx_driver_t* drv, zx_device_t* paren
// device_remove().
__UNUSED auto ptr = dev.leak_ref();
- DM_UNLOCK();
return r;
}
__EXPORT zx_status_t device_remove(zx_device_t* dev) {
- zx_status_t r;
- DM_LOCK();
+ ApiAutoLock lock;
// This recovers the leaked reference that happened in
// device_add_from_driver() above.
auto dev_ref = fbl::internal::MakeRefPtrNoAdopt(dev);
- r = devhost_device_remove(std::move(dev_ref));
- DM_UNLOCK();
- return r;
+ return devhost_device_remove(std::move(dev_ref));
}
__EXPORT zx_status_t device_rebind(zx_device_t* dev) {
- zx_status_t r;
- DM_LOCK();
+ ApiAutoLock lock;
fbl::RefPtr<zx_device_t> dev_ref(dev);
- r = devhost_device_rebind(dev_ref);
- DM_UNLOCK();
- return r;
+ return devhost_device_rebind(dev_ref);
}
__EXPORT void device_make_visible(zx_device_t* dev) {
- DM_LOCK();
+ ApiAutoLock lock;
fbl::RefPtr<zx_device_t> dev_ref(dev);
devhost_make_visible(dev_ref);
- DM_UNLOCK();
}
@@ -193,76 +184,53 @@ __EXPORT zx_handle_t get_root_resource() {
__EXPORT zx_status_t load_firmware(zx_device_t* dev, const char* path,
zx_handle_t* fw, size_t* size) {
- zx_status_t r;
- DM_LOCK();
+ ApiAutoLock lock;
fbl::RefPtr<zx_device_t> dev_ref(dev);
- r = devhost_load_firmware(dev_ref, path, fw, size);
- DM_UNLOCK();
- return r;
+ return devhost_load_firmware(dev_ref, path, fw, size);
}
// Interface Used by DevHost RPC Layer
zx_status_t device_bind(const fbl::RefPtr<zx_device_t>& dev, const char* drv_libname) {
- zx_status_t r;
- DM_LOCK();
- r = devhost_device_bind(dev, drv_libname);
- DM_UNLOCK();
- return r;
+ ApiAutoLock lock;
+ return devhost_device_bind(dev, drv_libname);
}
zx_status_t device_unbind(const fbl::RefPtr<zx_device_t>& dev) {
- DM_LOCK();
- zx_status_t r = devhost_device_unbind(dev);
- DM_UNLOCK();
- return r;
+ ApiAutoLock lock;
+ return devhost_device_unbind(dev);
}
zx_status_t device_open_at(const fbl::RefPtr<zx_device_t>& dev, fbl::RefPtr<zx_device_t>* out,
const char* path, uint32_t flags) {
- zx_status_t r;
- DM_LOCK();
- r = devhost_device_open_at(dev, out, path, flags);
- DM_UNLOCK();
- return r;
+ ApiAutoLock lock;
+ return devhost_device_open_at(dev, out, path, flags);
}
// This function is intended to consume the reference produced by
// device_open_at()
zx_status_t device_close(fbl::RefPtr<zx_device_t> dev, uint32_t flags) {
- zx_status_t r;
- DM_LOCK();
- r = devhost_device_close(std::move(dev), flags);
- DM_UNLOCK();
- return r;
+ ApiAutoLock lock;
+ return devhost_device_close(std::move(dev), flags);
}
__EXPORT zx_status_t device_get_metadata(zx_device_t* dev, uint32_t type,
void* buf, size_t buflen, size_t* actual) {
- zx_status_t r;
- DM_LOCK();
+ ApiAutoLock lock;
auto dev_ref = fbl::WrapRefPtr(dev);
- r = devhost_get_metadata(dev_ref, type, buf, buflen, actual);
- DM_UNLOCK();
- return r;
+ return devhost_get_metadata(dev_ref, type, buf, buflen, actual);
}
__EXPORT zx_status_t device_add_metadata(zx_device_t* dev, uint32_t type,
const void* data, size_t length) {
- zx_status_t r;
- DM_LOCK();
+ ApiAutoLock lock;
auto dev_ref = fbl::WrapRefPtr(dev);
- r = devhost_add_metadata(dev_ref, type, data, length);
- DM_UNLOCK();
- return r;
+ return devhost_add_metadata(dev_ref, type, data, length);
}
__EXPORT zx_status_t device_publish_metadata(zx_device_t* dev, const char* path,
uint32_t type, const void* data, size_t length) {
- zx_status_t r;
- DM_LOCK();
+ ApiAutoLock lock;
auto dev_ref = fbl::WrapRefPtr(dev);
- r = devhost_publish_metadata(dev_ref, path, type, data, length);
- DM_UNLOCK();
- return r;
+ return devhost_publish_metadata(dev_ref, path, type, data, length);
}
diff --git a/system/core/devmgr/devhost/core.cpp b/system/core/devmgr/devhost/core.cpp
index f1fa82ed58a..36c841caed1 100644
--- a/system/core/devmgr/devhost/core.cpp
+++ b/system/core/devmgr/devhost/core.cpp
@@ -247,9 +247,8 @@ void devhost_finalize() {
while ((dev = list.pop_front()) != nullptr) {
// invoke release op
if (dev->flags & DEV_FLAG_ADDED) {
- DM_UNLOCK();
+ ApiAutoRelock relock;
dev->ReleaseOp();
- DM_LOCK();
}
if (dev->parent) {
@@ -557,9 +556,8 @@ zx_status_t devhost_device_unbind(const fbl::RefPtr<zx_device_t>& dev) REQ_DM_LO
#if TRACE_ADD_REMOVE
printf("call unbind dev: %p(%s)\n", dev.get(), dev->name);
#endif
- DM_UNLOCK();
+ ApiAutoRelock relock;
dev->UnbindOp();
- DM_LOCK();
}
}
return ZX_OK;
@@ -575,14 +573,15 @@ zx_status_t devhost_device_open_at(const fbl::RefPtr<zx_device_t>& dev,
}
fbl::RefPtr<zx_device_t> new_ref(dev);
zx_status_t r;
- DM_UNLOCK();
zx_device_t* opened_dev = nullptr;
- if (path) {
- r = dev->OpenAtOp(&opened_dev, path, flags);
- } else {
- r = dev->OpenOp(&opened_dev, flags);
+ {
+ ApiAutoRelock relock;
+ if (path) {
+ r = dev->OpenAtOp(&opened_dev, path, flags);
+ } else {
+ r = dev->OpenOp(&opened_dev, flags);
+ }
}
- DM_LOCK();
if (r < 0) {
new_ref.reset();
} else if (opened_dev != nullptr) {
@@ -601,10 +600,8 @@ zx_status_t devhost_device_open_at(const fbl::RefPtr<zx_device_t>& dev,
}
zx_status_t devhost_device_close(fbl::RefPtr<zx_device_t> dev, uint32_t flags) REQ_DM_LOCK {
- DM_UNLOCK();
- zx_status_t r = dev->CloseOp(flags);
- DM_LOCK();
- return r;
+ ApiAutoRelock relock;
+ return dev->CloseOp(flags);
}
static zx_status_t devhost_device_suspend_locked(const fbl::RefPtr<zx_device>& dev,
@@ -621,9 +618,10 @@ static zx_status_t devhost_device_suspend_locked(const fbl::RefPtr<zx_device>& d
}
// then invoke our suspend hook
- DM_UNLOCK();
- st = dev->ops->suspend(dev->ctx, flags);
- DM_LOCK();
+ {
+ ApiAutoRelock relock;
+ st = dev->ops->suspend(dev->ctx, flags);
+ }
// default_suspend() returns ZX_ERR_NOT_SUPPORTED
if ((st != ZX_OK) && (st != ZX_ERR_NOT_SUPPORTED)) {
diff --git a/system/core/devmgr/devhost/devhost.cpp b/system/core/devmgr/devhost/devhost.cpp
index 40069821072..e61707f7c72 100644
--- a/system/core/devmgr/devhost/devhost.cpp
+++ b/system/core/devmgr/devhost/devhost.cpp
@@ -533,9 +533,11 @@ static zx_status_t fidl_Suspend(void* raw_ctx, uint32_t flags, fidl_txn_t* txn)
while (device->parent != nullptr) {
device = device->parent;
}
- DM_LOCK();
- zx_status_t r = devhost_device_suspend(device, flags);
- DM_UNLOCK();
+ zx_status_t r;
+ {
+ ApiAutoLock lock;
+ r = devhost_device_suspend(device, flags);
+ }
// TODO(teisenbe): We should probably check this return...
fuchsia_device_manager_ControllerSuspend_reply(txn, r);
return ZX_OK;
diff --git a/system/core/devmgr/devhost/lock.h b/system/core/devmgr/devhost/lock.h
index c021ae89dbf..f81521d401a 100644
--- a/system/core/devmgr/devhost/lock.h
+++ b/system/core/devmgr/devhost/lock.h
@@ -34,4 +34,24 @@ static inline bool DM_LOCK_HELD() {
return thrd_equal(internal::devhost_api_lock_owner.load(), thrd_current());
}
+class ApiAutoLock {
+public:
+ ApiAutoLock() TA_ACQ(&::devmgr::internal::devhost_api_lock) {
+ DM_LOCK();
+ }
+ ~ApiAutoLock() TA_REL(&::devmgr::internal::devhost_api_lock) {
+ DM_UNLOCK();
+ }
+};
+
+class ApiAutoRelock {
+public:
+ ApiAutoRelock() TA_REL(&::devmgr::internal::devhost_api_lock) {
+ DM_UNLOCK();
+ }
+ ~ApiAutoRelock() TA_ACQ(&::devmgr::internal::devhost_api_lock) {
+ DM_LOCK();
+ }
+};
+
} // namespace devmgr
--
GitLab