From cf254ae8e423f4e7a964b650e5e7c1a64d886fe1 Mon Sep 17 00:00:00 2001
From: Abdulla Kamar <abdulla@google.com>
Date: Fri, 10 May 2019 02:36:10 +0000
Subject: [PATCH] [devhost] Wait for BindDriver in reply to Bind
Calls to fuchsia.device.Controller/Bind should only return once the
operation completes, which is when
fuchsia.device.manager.DeviceController/BindDriver is called.
ZX-3991 #done
Change-Id: I10607640072c8f779ac1d5be79d5a2da6de6288b
---
zircon/system/core/devmgr/devhost/BUILD.gn | 1 +
zircon/system/core/devmgr/devhost/devhost.cpp | 22 ++++++++++++++++
.../system/core/devmgr/devhost/rpc-server.cpp | 14 +++++++---
.../system/core/devmgr/devhost/zx-device.cpp | 15 +++++++++++
zircon/system/core/devmgr/devhost/zx-device.h | 9 +++++++
zircon/system/core/devmgr/dmctl/BUILD.gn | 1 +
zircon/system/dev/test/ddk-test/ddk-test.c | 26 ++++++++++++++++++-
zircon/system/ulib/ddktl/test/ddktl-test.cpp | 23 ++++++++++++++--
zircon/system/ulib/fs/BUILD.gn | 2 +-
zircon/system/utest/driver-test/main.cpp | 9 +++++++
10 files changed, 114 insertions(+), 8 deletions(-)
diff --git a/zircon/system/core/devmgr/devhost/BUILD.gn b/zircon/system/core/devmgr/devhost/BUILD.gn
index 3660c415568..b57ac813f92 100644
--- a/zircon/system/core/devmgr/devhost/BUILD.gn
+++ b/zircon/system/core/devmgr/devhost/BUILD.gn
@@ -42,6 +42,7 @@ library("driver") {
"$zx/system/ulib/fbl",
"$zx/system/ulib/fdio",
"$zx/system/ulib/fidl",
+ "$zx/system/ulib/fit",
"$zx/system/ulib/fs:fs-for-driver",
"$zx/system/ulib/sync",
"$zx/system/ulib/trace:trace-driver",
diff --git a/zircon/system/core/devmgr/devhost/devhost.cpp b/zircon/system/core/devmgr/devhost/devhost.cpp
index 97b14bb99e0..9a69ebddd8d 100644
--- a/zircon/system/core/devmgr/devhost/devhost.cpp
+++ b/zircon/system/core/devmgr/devhost/devhost.cpp
@@ -29,6 +29,7 @@
#include <fbl/auto_lock.h>
#include <fbl/function.h>
#include <fs/handler.h>
+#include <fuchsia/device/c/fidl.h>
#include <fuchsia/device/manager/c/fidl.h>
#include <fuchsia/io/c/fidl.h>
#include <lib/async-loop/cpp/loop.h>
@@ -36,6 +37,7 @@
#include <lib/async/cpp/wait.h>
#include <lib/fdio/fdio.h>
#include <lib/fidl/coding.h>
+#include <lib/fit/defer.h>
#include <lib/zx/debuglog.h>
#include <lib/zx/resource.h>
#include <lib/zx/vmo.h>
@@ -547,6 +549,20 @@ static zx_status_t fidl_CreateCompositeDevice(void* raw_ctx, zx_handle_t raw_rpc
return fuchsia_device_manager_DevhostControllerCreateCompositeDevice_reply(txn, ZX_OK);
}
+// Handles outstanding calls to fuchsia.device.Controller/Bind.
+static void BindReply(const fbl::RefPtr<zx_device_t>& dev) {
+ fs::FidlConnection conn(fidl_txn_t{}, ZX_HANDLE_INVALID, 0);
+ if (dev->PopBindConn(&conn)) {
+ // TODO(DNO-492): additional logging for debugging what looks like a
+ // deadlock. Remove once bug is resolved.
+ printf("DeviceControllerBind finished\n");
+ // TODO(ZX-3431): We ignore the status from device_bind() for
+ // bug-compatibility reasons. Once this bug is resolved, we can
+ // return the actual status.
+ fuchsia_device_ControllerBind_reply(conn.Txn(), ZX_OK);
+ }
+}
+
static zx_status_t fidl_BindDriver(void* raw_ctx, const char* driver_path_data,
size_t driver_path_size, zx_handle_t raw_driver_vmo,
fidl_txn_t* txn) {
@@ -554,6 +570,12 @@ static zx_status_t fidl_BindDriver(void* raw_ctx, const char* driver_path_data,
zx::vmo driver_vmo(raw_driver_vmo);
fbl::StringPiece driver_path(driver_path_data, driver_path_size);
+ // We can now reply to calls to fuchsia.device.Controller/Bind, as we have
+ // completed the request to bind the driver.
+ auto defer = fit::defer([dev = ctx->conn->dev] {
+ BindReply(dev);
+ });
+
// TODO: added to help debug DNO-492, remove when done
if (driver_path == "/boot/driver/zxcrypt.so") {
log(ERROR, "devhost[%s] bind zxcrypt\n", ctx->path);
diff --git a/zircon/system/core/devmgr/devhost/rpc-server.cpp b/zircon/system/core/devmgr/devhost/rpc-server.cpp
index 7951a87d8ee..8fe32008e70 100644
--- a/zircon/system/core/devmgr/devhost/rpc-server.cpp
+++ b/zircon/system/core/devmgr/devhost/rpc-server.cpp
@@ -27,6 +27,7 @@
#include <zircon/syscalls.h>
#include <zircon/types.h>
+#include <fbl/auto_lock.h>
#include <fs/connection.h>
#include <fs/handler.h>
#include <fuchsia/device/c/fidl.h>
@@ -561,6 +562,14 @@ static zx_status_t fidl_DeviceControllerBind(void* ctx, const char* driver_data,
memcpy(drv_libname, driver_data, driver_count);
drv_libname[driver_count] = 0;
+ if (!strcmp(drv_libname, "/boot/driver/zxcrypt.so")) {
+ // TODO(DNO-492): Workaround, due to racing attempts to bind zxcrypt.so
+ // that results in a deadlock.
+ fuchsia_device_ControllerBind_reply(txn, ZX_OK);
+ } else {
+ conn->dev->PushBindConn(fs::FidlConnection::CopyTxn(txn));
+ }
+
// TODO(DNO-492): additional logging for debugging what looks like a
// deadlock. Remove once bug is resolved.
printf("DeviceControllerBind running: %s\n", drv_libname);
@@ -568,10 +577,7 @@ static zx_status_t fidl_DeviceControllerBind(void* ctx, const char* driver_data,
// bug-compatibility reasons. Once this bug is resolved, we can return the
// actual status.
__UNUSED zx_status_t status = device_bind(conn->dev, drv_libname);
- // TODO(DNO-492): additional logging for debugging what looks like a
- // deadlock. Remove once bug is resolved.
- printf("DeviceControllerBind finished: %s %s\n", drv_libname, zx_status_get_string(status));
- return fuchsia_device_ControllerBind_reply(txn, ZX_OK);
+ return ZX_OK;
}
static zx_status_t fidl_DeviceControllerUnbind(void* ctx, fidl_txn_t* txn) {
diff --git a/zircon/system/core/devmgr/devhost/zx-device.cpp b/zircon/system/core/devmgr/devhost/zx-device.cpp
index d00ea30d266..1201f89a95d 100644
--- a/zircon/system/core/devmgr/devhost/zx-device.cpp
+++ b/zircon/system/core/devmgr/devhost/zx-device.cpp
@@ -14,6 +14,21 @@ zx_status_t zx_device::Create(fbl::RefPtr<zx_device>* out_dev) {
return ZX_OK;
}
+void zx_device::PushBindConn(const fs::FidlConnection& conn) {
+ fbl::AutoLock<fbl::Mutex> lock(&bind_conn_lock_);
+ bind_conn_.push_back(conn);
+}
+
+bool zx_device::PopBindConn(fs::FidlConnection* conn) {
+ fbl::AutoLock<fbl::Mutex> lock(&bind_conn_lock_);
+ if (bind_conn_.is_empty()) {
+ return false;
+ }
+ *conn = bind_conn_[0];
+ bind_conn_.erase(0);
+ return true;
+}
+
// We must disable thread-safety analysis due to not being able to statically
// guarantee the lock holding invariant. Instead, we acquire the lock if
// it's not already being held by the current thread.
diff --git a/zircon/system/core/devmgr/devhost/zx-device.h b/zircon/system/core/devmgr/devhost/zx-device.h
index 26152085fc3..90bf1a4bc8f 100644
--- a/zircon/system/core/devmgr/devhost/zx-device.h
+++ b/zircon/system/core/devmgr/devhost/zx-device.h
@@ -12,6 +12,8 @@
#include <fbl/recycler.h>
#include <fbl/ref_counted_upgradeable.h>
#include <fbl/ref_ptr.h>
+#include <fbl/vector.h>
+#include <fs/handler.h>
#include <lib/zx/channel.h>
#include <lib/zx/eventpair.h>
#include <zircon/compiler.h>
@@ -80,6 +82,9 @@ struct zx_device : fbl::RefCountedUpgradeable<zx_device>, fbl::Recyclable<zx_dev
return Dispatch(ops->message, ZX_ERR_NOT_SUPPORTED, msg, txn);
}
+ void PushBindConn(const fs::FidlConnection& conn);
+ bool PopBindConn(fs::FidlConnection* conn);
+
// Check if this devhost has a device with the given ID, and if so returns a
// reference to it.
static fbl::RefPtr<zx_device> GetDeviceFromLocalId(uint64_t local_id);
@@ -180,6 +185,10 @@ private:
// Identifier assigned by devmgr that can be used to assemble composite
// devices.
uint64_t local_id_ = 0;
+
+ // The connection associated with a fuchsia.device.Controller/Bind.
+ fbl::Mutex bind_conn_lock_;
+ fbl::Vector<fs::FidlConnection> bind_conn_ TA_GUARDED(bind_conn_lock_);
};
// zx_device_t objects must be created or initialized by the driver manager's
diff --git a/zircon/system/core/devmgr/dmctl/BUILD.gn b/zircon/system/core/devmgr/dmctl/BUILD.gn
index d702904df07..010be79855a 100644
--- a/zircon/system/core/devmgr/dmctl/BUILD.gn
+++ b/zircon/system/core/devmgr/dmctl/BUILD.gn
@@ -11,6 +11,7 @@ driver("dmctl") {
"$zx/system/ulib/ddk",
"$zx/system/ulib/ddktl",
"$zx/system/ulib/fbl",
+ "$zx/system/ulib/fs:fs-for-driver",
"$zx/system/ulib/zircon",
"$zx/system/ulib/zx",
"$zx/system/ulib/zxcpp",
diff --git a/zircon/system/dev/test/ddk-test/ddk-test.c b/zircon/system/dev/test/ddk-test/ddk-test.c
index b1e0927b3e1..957d794d1bc 100644
--- a/zircon/system/dev/test/ddk-test/ddk-test.c
+++ b/zircon/system/dev/test/ddk-test/ddk-test.c
@@ -6,6 +6,7 @@
#include <ddk/driver.h>
#include <ddk/binding.h>
#include <ddk/protocol/test.h>
+#include <zircon/assert.h>
#include <unittest/unittest.h>
#include <stddef.h>
@@ -57,9 +58,32 @@ static zx_status_t ddk_test_func(void* cookie, test_report_t* report) {
return report->n_failed == 0 ? ZX_OK : ZX_ERR_INTERNAL;
}
+static zx_device_t* child_dev = NULL;
+
+static void child_unbind(void* ctx) {
+ device_remove(child_dev);
+}
+
+static zx_protocol_device_t child_device_ops = {
+ .version = DEVICE_OPS_VERSION,
+ .unbind = child_unbind,
+};
+
zx_status_t ddk_test_bind(void* ctx, zx_device_t* parent) {
+ device_add_args_t args = {
+ .version = DEVICE_ADD_ARGS_VERSION,
+ .name = "child",
+ .ops = &child_device_ops,
+ .flags = DEVICE_ADD_NON_BINDABLE,
+ };
+ ZX_ASSERT(child_dev == NULL);
+ zx_status_t status = device_add(parent, &args, &child_dev);
+ if (status != ZX_OK) {
+ return status;
+ }
+
test_protocol_t proto;
- zx_status_t status = device_get_protocol(parent, ZX_PROTOCOL_TEST, &proto);
+ status = device_get_protocol(parent, ZX_PROTOCOL_TEST, &proto);
if (status != ZX_OK) {
return status;
}
diff --git a/zircon/system/ulib/ddktl/test/ddktl-test.cpp b/zircon/system/ulib/ddktl/test/ddktl-test.cpp
index 15cfa061583..bad3ff2c813 100644
--- a/zircon/system/ulib/ddktl/test/ddktl-test.cpp
+++ b/zircon/system/ulib/ddktl/test/ddktl-test.cpp
@@ -6,9 +6,11 @@
#include <ddk/device.h>
#include <ddk/driver.h>
#include <ddk/protocol/test.h>
+#include <ddktl/device.h>
#include <lib/zx/socket.h>
#include <limits.h>
+#include <memory>
#include <stddef.h>
#include <stdio.h>
#include <stdlib.h>
@@ -62,12 +64,29 @@ zx_status_t ddktl_test_func(void* cookie, test_report_t* report) {
return report->n_failed == 0 ? ZX_OK : ZX_ERR_INTERNAL;
}
+class ChildDevice;
+using ChildDeviceType = ddk::Device<ChildDevice, ddk::Unbindable>;
+
+class ChildDevice : public ChildDeviceType {
+public:
+ ChildDevice(zx_device_t* parent) : ChildDeviceType(parent) {}
+ void DdkUnbind() { DdkRemove(); }
+ void DdkRelease() { delete this; }
+};
+
} // namespace
extern "C" zx_status_t ddktl_test_bind(void* ctx, zx_device_t* parent) {
+ auto device = std::make_unique<ChildDevice>(parent);
+ zx_status_t status = device->DdkAdd("child", DEVICE_ADD_NON_BINDABLE);
+ if (status != ZX_OK) {
+ return status;
+ }
+ // devmgr now owns this
+ __UNUSED auto ptr = device.release();
+
test_protocol_t proto;
- auto status =
- device_get_protocol(parent, ZX_PROTOCOL_TEST, reinterpret_cast<void*>(&proto));
+ status = device_get_protocol(parent, ZX_PROTOCOL_TEST, &proto);
if (status != ZX_OK) {
return status;
}
diff --git a/zircon/system/ulib/fs/BUILD.gn b/zircon/system/ulib/fs/BUILD.gn
index c6535041c8f..b0da9c3a736 100644
--- a/zircon/system/ulib/fs/BUILD.gn
+++ b/zircon/system/ulib/fs/BUILD.gn
@@ -113,7 +113,7 @@ if (is_fuchsia) {
library("fs-for-driver") {
visibility = [
":*",
- "$zx/system/core/devmgr/devhost:*",
+ "$zx/system/core/devmgr/*",
]
sources = [
"block-txn.cpp",
diff --git a/zircon/system/utest/driver-test/main.cpp b/zircon/system/utest/driver-test/main.cpp
index 6e0ad8a51ec..1e95ba22de9 100644
--- a/zircon/system/utest/driver-test/main.cpp
+++ b/zircon/system/utest/driver-test/main.cpp
@@ -105,6 +105,15 @@ void do_one_test(const IsolatedDevmgr& devmgr, const zx::channel& test_root,
return;
}
+ // Check that Bind was synchronous by looking for the child device.
+ char child_devpath[fuchsia_device_test_MAX_DEVICE_PATH_LEN+1];
+ snprintf(child_devpath, sizeof(child_devpath), "%s/child", relative_devpath);
+ fd.reset(openat(devmgr.devfs_root().get(), child_devpath, O_RDWR));
+ if (!fd.is_valid()) {
+ printf("driver-tests: error binding device %s %s\n", devpath, relative_devpath);
+ return;
+ }
+
zx::socket output_copy;
status = output.duplicate(ZX_RIGHT_SAME_RIGHTS, &output_copy);
if (status != ZX_OK) {
--
GitLab