[devhost] Wait for BindDriver in reply to Bind
This reverts commit cccafba349eee52bd9900c0017383107d067b970. We
workaround the FVM issues by replying straight away when binding fvm.so.
Calls to fuchsia.device.Controller/Bind should only reply once the
operation completes, which is when
fuchsia.device.manager.DeviceController/BindDriver is called.
ZX-3991 #done
ZX-4169
Change-Id: I54adfe2817f132bc1385b53fdbb45e897fdecf29
diff --git a/zircon/system/core/devmgr/devhost/BUILD.gn b/zircon/system/core/devmgr/devhost/BUILD.gn
index 746a603..1341f60 100644
--- a/zircon/system/core/devmgr/devhost/BUILD.gn
+++ b/zircon/system/core/devmgr/devhost/BUILD.gn
@@ -42,6 +42,7 @@
"$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/device-controller-connection.cpp b/zircon/system/core/devmgr/devhost/device-controller-connection.cpp
index 31e7749d..b7edce7 100644
--- a/zircon/system/core/devmgr/devhost/device-controller-connection.cpp
+++ b/zircon/system/core/devmgr/devhost/device-controller-connection.cpp
@@ -5,6 +5,8 @@
#include "device-controller-connection.h"
#include <fbl/auto_lock.h>
+#include <fuchsia/device/c/fidl.h>
+#include <lib/fit/defer.h>
#include <lib/zx/vmo.h>
#include <zircon/status.h>
#include "../shared/fidl_txn.h"
@@ -23,6 +25,17 @@
DeviceControllerConnection* conn;
};
+// Handles outstanding calls to fuchsia.device.Controller/Bind.
+void BindReply(const fbl::RefPtr<zx_device_t>& dev) {
+ fs::FidlConnection conn(fidl_txn_t{}, ZX_HANDLE_INVALID, 0);
+ if (dev->PopBindConn(&conn)) {
+ // 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);
+ }
+}
+
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) {
@@ -30,6 +43,12 @@
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: api lock integration
log(RPC_IN, "devhost[%s] bind driver '%.*s'\n", ctx->path, static_cast<int>(driver_path_size),
driver_path_data);
diff --git a/zircon/system/core/devmgr/devhost/rpc-server.cpp b/zircon/system/core/devmgr/devhost/rpc-server.cpp
index 7951a87d..3b7ad20 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>
@@ -557,21 +558,26 @@
static zx_status_t fidl_DeviceControllerBind(void* ctx, const char* driver_data,
size_t driver_count, fidl_txn_t* txn) {
auto conn = static_cast<DevfsConnection*>(ctx);
+
char drv_libname[fuchsia_device_MAX_DRIVER_PATH_LEN + 1];
memcpy(drv_libname, driver_data, driver_count);
drv_libname[driver_count] = 0;
- // TODO(DNO-492): additional logging for debugging what looks like a
- // deadlock. Remove once bug is resolved.
- printf("DeviceControllerBind running: %s\n", drv_libname);
+ if (!strcmp(drv_libname, "/boot/driver/fvm.so")) {
+ // TODO(ZX-4198): Workaround for flaky tests involving FVM.
+ zx_status_t status = fuchsia_device_ControllerBind_reply(txn, ZX_OK);
+ if (status != ZX_OK) {
+ return status;
+ }
+ } else {
+ conn->dev->PushBindConn(fs::FidlConnection::CopyTxn(txn));
+ }
+
// 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.
__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 44493af..e4391207 100644
--- a/zircon/system/core/devmgr/devhost/zx-device.cpp
+++ b/zircon/system/core/devmgr/devhost/zx-device.cpp
@@ -15,6 +15,21 @@
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 fd10cdf..0a7e4c39 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>
@@ -81,6 +83,9 @@
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);
@@ -188,6 +193,10 @@
// 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 7d5eed18..baabbb1 100644
--- a/zircon/system/core/devmgr/dmctl/BUILD.gn
+++ b/zircon/system/core/devmgr/dmctl/BUILD.gn
@@ -11,6 +11,7 @@
"$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",
]
diff --git a/zircon/system/dev/test/ddk-test/ddk-test.c b/zircon/system/dev/test/ddk-test/ddk-test.c
index e7bb565..139aa35 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>
@@ -59,9 +60,32 @@
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 436aced..f1e8007 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>
@@ -60,11 +62,29 @@
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;
- 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/fs/BUILD.gn b/zircon/system/ulib/fs/BUILD.gn
index 1f02b4a..22da651 100644
--- a/zircon/system/ulib/fs/BUILD.gn
+++ b/zircon/system/ulib/fs/BUILD.gn
@@ -112,7 +112,7 @@
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 33bc697..4442d85 100644
--- a/zircon/system/utest/driver-test/main.cpp
+++ b/zircon/system/utest/driver-test/main.cpp
@@ -105,6 +105,15 @@
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) {
