From 75b35ceab71c50f43b11a9ee9bd4e034a9fc7447 Mon Sep 17 00:00:00 2001
From: Todd Eisenberger <teisenbe@google.com>
Date: Thu, 25 Apr 2019 00:42:02 +0000
Subject: [PATCH] [devcoordinator] Fix buggy composite device behavior
If a component was a device with MUST_ISOLATE set on it, we would try to
bind to the non-proxied side of it, which doesn't speak the device
protocols.
Change-Id: I64eaccec5d51997f4374d0277b040a952f681667
---
.../devcoordinator/composite-device.cpp | 8 +-
.../devmgr/devcoordinator/coordinator.cpp | 6 +-
zircon/system/dev/board/test/BUILD.gn | 1 +
zircon/system/dev/board/test/test-board.cpp | 18 ++++
zircon/system/dev/board/test/test/child-1.c | 2 +-
zircon/system/dev/board/test/test/child-2.c | 22 ++++-
.../dev/board/test/test/child-2.proxy.c | 86 +++++++++++++++++++
zircon/system/dev/board/test/test/composite.c | 7 ++
.../ulib/ddk/include/ddk/platform-defs.h | 1 +
zircon/system/utest/platform-bus/main.cpp | 34 ++++----
10 files changed, 164 insertions(+), 21 deletions(-)
create mode 100644 zircon/system/dev/board/test/test/child-2.proxy.c
diff --git a/zircon/system/core/devmgr/devcoordinator/composite-device.cpp b/zircon/system/core/devmgr/devcoordinator/composite-device.cpp
index a9aad304b2f..ba586e88d73 100644
--- a/zircon/system/core/devmgr/devcoordinator/composite-device.cpp
+++ b/zircon/system/core/devmgr/devcoordinator/composite-device.cpp
@@ -127,9 +127,15 @@ zx_status_t CompositeDevice::TryAssemble() {
// Create all of the proxies for the component devices, in the same process
for (auto& component : bound_) {
const auto& component_dev = component.component_device();
- const auto& bound_dev = component.bound_device();
+ auto bound_dev = component.bound_device();
coordinator = component_dev->coordinator;
+ // If the device we're bound to is proxied, we care about its proxy
+ // rather than it, since that's the side that we communicate with.
+ if (bound_dev->proxy()) {
+ bound_dev = bound_dev->proxy();
+ }
+
// Check if we need to use the proxy. If not, share a reference straight
// to the target device rather than the instance of the component device
// that bound to it.
diff --git a/zircon/system/core/devmgr/devcoordinator/coordinator.cpp b/zircon/system/core/devmgr/devcoordinator/coordinator.cpp
index ef4b167b659..dd31246b0e1 100644
--- a/zircon/system/core/devmgr/devcoordinator/coordinator.cpp
+++ b/zircon/system/core/devmgr/devcoordinator/coordinator.cpp
@@ -589,10 +589,14 @@ zx_status_t Coordinator::AddDevice(const fbl::RefPtr<Device>& parent, zx::channe
}
devices_.push_back(dev);
+ // Note that |dev->parent()| may not match |parent| here, so we should always
+ // use |dev->parent()|. This case can happen if |parent| refers to a device
+ // proxy.
+
// If we're creating a device that's using the component driver, inform the
// component.
if (component_driver_ != nullptr && dev->libname() == component_driver_->libname) {
- CompositeDeviceComponent* component = parent->component();
+ CompositeDeviceComponent* component = dev->parent()->component();
component->set_component_device(dev);
status = component->composite()->TryAssemble();
if (status != ZX_OK && status != ZX_ERR_SHOULD_WAIT) {
diff --git a/zircon/system/dev/board/test/BUILD.gn b/zircon/system/dev/board/test/BUILD.gn
index 3fa7c5b70cd..eba0ce32aa0 100644
--- a/zircon/system/dev/board/test/BUILD.gn
+++ b/zircon/system/dev/board/test/BUILD.gn
@@ -7,6 +7,7 @@ testonly = true
simple_drivers = [
"child-1",
"child-2",
+ "child-2.proxy",
"child-3",
"parent",
"composite",
diff --git a/zircon/system/dev/board/test/test-board.cpp b/zircon/system/dev/board/test/test-board.cpp
index b628f04f668..19fe9d63911 100644
--- a/zircon/system/dev/board/test/test-board.cpp
+++ b/zircon/system/dev/board/test/test-board.cpp
@@ -114,6 +114,17 @@ zx_status_t TestBoard::Create(zx_device_t* parent) {
BI_ABORT_IF(NE, BIND_PROTOCOL, ZX_PROTOCOL_POWER),
BI_MATCH_IF(EQ, BIND_POWER_DOMAIN, 3),
};
+ const zx_bind_inst_t child2_match[] = {
+ BI_ABORT_IF(NE, BIND_PROTOCOL, ZX_PROTOCOL_PDEV),
+ BI_ABORT_IF(NE, BIND_PLATFORM_DEV_VID, PDEV_VID_TEST),
+ BI_ABORT_IF(NE, BIND_PLATFORM_DEV_PID, PDEV_PID_PBUS_TEST),
+ BI_MATCH_IF(EQ, BIND_PLATFORM_DEV_DID, PDEV_DID_TEST_CHILD_2),
+ };
+ const zx_bind_inst_t child4_match[] = {
+ BI_ABORT_IF(NE, BIND_PLATFORM_DEV_VID, PDEV_VID_TEST),
+ BI_ABORT_IF(NE, BIND_PLATFORM_DEV_PID, PDEV_PID_PBUS_TEST),
+ BI_MATCH_IF(EQ, BIND_PLATFORM_DEV_DID, PDEV_DID_TEST_CHILD_4),
+ };
device_component_part_t gpio_component[] = {
{ fbl::count_of(root_match), root_match },
{ fbl::count_of(gpio_match), gpio_match },
@@ -130,11 +141,18 @@ zx_status_t TestBoard::Create(zx_device_t* parent) {
{ fbl::count_of(root_match), root_match },
{ fbl::count_of(power_match), power_match },
};
+ device_component_part_t child4_component[] = {
+ { fbl::count_of(root_match), root_match },
+ { fbl::count_of(child2_match), child2_match },
+ { fbl::count_of(child4_match), child4_match },
+ };
+
device_component_t composite[] = {
{ fbl::count_of(gpio_component), gpio_component },
{ fbl::count_of(clock_component), clock_component },
{ fbl::count_of(i2c_component), i2c_component },
{ fbl::count_of(power_component), power_component },
+ { fbl::count_of(child4_component), child4_component },
};
const uint32_t test_metadata_value = 12345;
diff --git a/zircon/system/dev/board/test/test/child-1.c b/zircon/system/dev/board/test/test/child-1.c
index 81d8ed910c2..0be1d1fd68f 100644
--- a/zircon/system/dev/board/test/test/child-1.c
+++ b/zircon/system/dev/board/test/test/child-1.c
@@ -72,7 +72,7 @@ static zx_status_t test_bind(void* ctx, zx_device_t* parent) {
device_add_args_t child_2_args = {
.version = DEVICE_ADD_ARGS_VERSION,
- .name = "child-2-top",
+ .name = "child-2",
.ctx = child_2,
.ops = &test_device_protocol,
.props = child_2_props,
diff --git a/zircon/system/dev/board/test/test/child-2.c b/zircon/system/dev/board/test/test/child-2.c
index e12d0bf5345..41b9eb2e894 100644
--- a/zircon/system/dev/board/test/test/child-2.c
+++ b/zircon/system/dev/board/test/test/child-2.c
@@ -22,8 +22,18 @@ static void test_release(void* ctx) {
free(ctx);
}
+static zx_status_t test_rxrpc(void* ctx, zx_handle_t channel) {
+ if (channel == ZX_HANDLE_INVALID) {
+ return ZX_OK;
+ }
+ // This won't actually get called, since the other half doesn't send
+ // messages at the moment
+ __builtin_trap();
+};
+
static zx_protocol_device_t test_device_protocol = {
.version = DEVICE_OPS_VERSION,
+ .rxrpc = test_rxrpc,
.release = test_release,
};
@@ -62,11 +72,21 @@ static zx_status_t test_bind(void* ctx, zx_device_t* parent) {
return ZX_ERR_NO_MEMORY;
}
+ zx_device_prop_t child_props[] = {
+ { BIND_PLATFORM_DEV_VID, 0, PDEV_VID_TEST},
+ { BIND_PLATFORM_DEV_PID, 0, PDEV_PID_PBUS_TEST},
+ { BIND_PLATFORM_DEV_DID, 0, PDEV_DID_TEST_CHILD_4 },
+ };
+
device_add_args_t args = {
.version = DEVICE_ADD_ARGS_VERSION,
- .name = "child-2",
+ .name = "child-4",
.ctx = test,
.ops = &test_device_protocol,
+ .props = child_props,
+ .prop_count = countof(child_props),
+ .flags = DEVICE_ADD_MUST_ISOLATE,
+ .proxy_args = ",",
};
status = device_add(parent, &args, &test->zxdev);
diff --git a/zircon/system/dev/board/test/test/child-2.proxy.c b/zircon/system/dev/board/test/test/child-2.proxy.c
new file mode 100644
index 00000000000..dd7b4b3a730
--- /dev/null
+++ b/zircon/system/dev/board/test/test/child-2.proxy.c
@@ -0,0 +1,86 @@
+// Copyright 2018 The Fuchsia Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include <stdlib.h>
+#include <string.h>
+
+#include <ddk/binding.h>
+#include <ddk/debug.h>
+#include <ddk/device.h>
+#include <ddk/driver.h>
+#include <ddk/platform-defs.h>
+#include <ddk/protocol/gpio.h>
+#include <ddk/protocol/platform/device.h>
+
+#define DRIVER_NAME "test-child-4"
+
+typedef struct {
+ zx_device_t* zxdev;
+ zx_handle_t rpc_channel;
+} test_t;
+
+static void test_release(void* ctx) {
+ test_t* dev = (test_t*)ctx;
+ zx_handle_close(dev->rpc_channel);
+ free(dev);
+}
+
+static zx_status_t test_get_protocol(void* ctx, uint32_t protocol_id, void* proto) {
+ // Lie about supporting the CLOCK protocol. The composite device will just
+ // check that we claimed to support it. Note the non-proxied device does
+ // not claim to support this protocol, so if we see it, we must be talking
+ // to the proxy.
+ if (protocol_id == ZX_PROTOCOL_CLOCK) {
+ // Zero out the proto ops in case something tries using it
+ memset(proto, 0, sizeof(uintptr_t));
+ return ZX_OK;
+ }
+ return ZX_ERR_NOT_SUPPORTED;
+}
+
+static zx_protocol_device_t test_device_protocol = {
+ .version = DEVICE_OPS_VERSION,
+ .get_protocol = test_get_protocol,
+ .release = test_release,
+};
+static zx_status_t test_create(void* ctx, zx_device_t* parent, const char* name, const char* args,
+ zx_handle_t rpc_channel) {
+ zx_status_t status;
+
+ zxlogf(INFO, "test_create: %s \n", DRIVER_NAME);
+
+ test_t* test = calloc(1, sizeof(test_t));
+ if (!test) {
+ return ZX_ERR_NO_MEMORY;
+ }
+ test->rpc_channel = rpc_channel;
+
+ device_add_args_t dev_args = {
+ .version = DEVICE_ADD_ARGS_VERSION,
+ .name = "child-4",
+ .ctx = test,
+ .ops = &test_device_protocol,
+ };
+
+ status = device_add(parent, &dev_args, &test->zxdev);
+ if (status != ZX_OK) {
+ zxlogf(ERROR, "%s: device_add failed: %d\n", DRIVER_NAME, status);
+ free(test);
+ return status;
+ }
+
+ return ZX_OK;
+}
+
+static zx_driver_ops_t test_driver_ops = {
+ .version = DRIVER_OPS_VERSION,
+ .create = test_create,
+};
+
+ZIRCON_DRIVER_BEGIN(test_bus, test_driver_ops, "zircon", "0.1", 4)
+ BI_ABORT_IF(NE, BIND_PROTOCOL, ZX_PROTOCOL_PDEV),
+ BI_ABORT_IF(NE, BIND_PLATFORM_DEV_VID, PDEV_VID_TEST),
+ BI_ABORT_IF(NE, BIND_PLATFORM_DEV_PID, PDEV_PID_PBUS_TEST),
+ BI_MATCH_IF(EQ, BIND_PLATFORM_DEV_DID, PDEV_DID_TEST_CHILD_4),
+ZIRCON_DRIVER_END(test_bus)
diff --git a/zircon/system/dev/board/test/test/composite.c b/zircon/system/dev/board/test/test/composite.c
index e29b5ef50b9..b5629f7059a 100644
--- a/zircon/system/dev/board/test/test/composite.c
+++ b/zircon/system/dev/board/test/test/composite.c
@@ -28,6 +28,7 @@ enum {
COMPONENT_CLOCK,
COMPONENT_I2C,
COMPONENT_POWER,
+ COMPONENT_CHILD4,
COMPONENT_COUNT,
};
@@ -151,6 +152,7 @@ static zx_status_t test_bind(void* ctx, zx_device_t* parent) {
clock_protocol_t clock;
i2c_protocol_t i2c;
power_protocol_t power;
+ clock_protocol_t child4;
status = device_get_protocol(components[COMPONENT_PDEV], ZX_PROTOCOL_PDEV, &pdev);
if (status != ZX_OK) {
@@ -177,6 +179,11 @@ static zx_status_t test_bind(void* ctx, zx_device_t* parent) {
zxlogf(ERROR, "%s: could not get protocol ZX_PROTOCOL_POWER\n", DRIVER_NAME);
return status;
}
+ status = device_get_protocol(components[COMPONENT_CHILD4], ZX_PROTOCOL_CLOCK, &child4);
+ if (status != ZX_OK) {
+ zxlogf(ERROR, "%s: could not get protocol from child4\n", DRIVER_NAME);
+ return status;
+ }
if ((status = test_gpio(&gpio)) != ZX_OK) {
zxlogf(ERROR, "%s: test_gpio failed: %d\n", DRIVER_NAME, status);
diff --git a/zircon/system/ulib/ddk/include/ddk/platform-defs.h b/zircon/system/ulib/ddk/include/ddk/platform-defs.h
index 928636ef605..689163a1df4 100644
--- a/zircon/system/ulib/ddk/include/ddk/platform-defs.h
+++ b/zircon/system/ulib/ddk/include/ddk/platform-defs.h
@@ -195,6 +195,7 @@ __BEGIN_CDECLS
#define PDEV_DID_TEST_CLOCK 7
#define PDEV_DID_TEST_I2C 8
#define PDEV_DID_TEST_POWER 9
+#define PDEV_DID_TEST_CHILD_4 10
// ARM
#define PDEV_VID_ARM 18
diff --git a/zircon/system/utest/platform-bus/main.cpp b/zircon/system/utest/platform-bus/main.cpp
index 9e5e4d91bae..f7b30dbcb29 100644
--- a/zircon/system/utest/platform-bus/main.cpp
+++ b/zircon/system/utest/platform-bus/main.cpp
@@ -64,55 +64,55 @@ bool enumeration_test() {
fbl::unique_fd fd;
ASSERT_EQ(RecursiveWaitForFile(devmgr.devfs_root(), "sys/platform",
- zx::deadline_after(zx::sec(5)), &fd),
+ zx::time::infinite(), &fd),
ZX_OK);
EXPECT_EQ(RecursiveWaitForFile(devmgr.devfs_root(), "sys/platform/test-board",
- zx::deadline_after(zx::sec(5)), &fd),
+ zx::time::infinite(), &fd),
ZX_OK);
EXPECT_EQ(RecursiveWaitForFile(devmgr.devfs_root(), "sys/platform/11:01:1",
- zx::deadline_after(zx::sec(5)), &fd),
+ zx::time::infinite(), &fd),
ZX_OK);
EXPECT_EQ(RecursiveWaitForFile(devmgr.devfs_root(), "sys/platform/11:01:1/child-1",
- zx::deadline_after(zx::sec(5)), &fd),
+ zx::time::infinite(), &fd),
ZX_OK);
- EXPECT_EQ(RecursiveWaitForFile(devmgr.devfs_root(), "sys/platform/11:01:1/child-1/child-2-top",
- zx::deadline_after(zx::sec(5)), &fd),
+ EXPECT_EQ(RecursiveWaitForFile(devmgr.devfs_root(), "sys/platform/11:01:1/child-1/child-2",
+ zx::time::infinite(), &fd),
ZX_OK);
EXPECT_EQ(RecursiveWaitForFile(devmgr.devfs_root(),
- "sys/platform/11:01:1/child-1/child-2-top/child-2",
- zx::deadline_after(zx::sec(5)), &fd),
+ "sys/platform/11:01:1/child-1/child-2/child-4",
+ zx::time::infinite(), &fd),
ZX_OK);
EXPECT_EQ(RecursiveWaitForFile(devmgr.devfs_root(), "sys/platform/11:01:1/child-1/child-3-top",
- zx::deadline_after(zx::sec(5)), &fd),
+ zx::time::infinite(), &fd),
ZX_OK);
EXPECT_EQ(RecursiveWaitForFile(devmgr.devfs_root(),
"sys/platform/11:01:1/child-1/child-3-top/child-3",
- zx::deadline_after(zx::sec(5)), &fd),
+ zx::time::infinite(), &fd),
ZX_OK);
EXPECT_EQ(RecursiveWaitForFile(devmgr.devfs_root(),
"sys/platform/11:01:5/test-gpio/gpio-3/component",
- zx::deadline_after(zx::sec(5)), &fd),
+ zx::time::infinite(), &fd),
ZX_OK);
EXPECT_EQ(RecursiveWaitForFile(devmgr.devfs_root(),
"sys/platform/11:01:7/test-clock/clock-1/component",
- zx::deadline_after(zx::sec(5)), &fd),
+ zx::time::infinite(), &fd),
ZX_OK);
EXPECT_EQ(RecursiveWaitForFile(devmgr.devfs_root(),
"sys/platform/11:01:8/test-i2c/i2c/i2c-1-5/component",
- zx::deadline_after(zx::sec(5)), &fd),
+ zx::time::infinite(), &fd),
ZX_OK);
EXPECT_EQ(RecursiveWaitForFile(devmgr.devfs_root(),
"sys/platform/11:01:6/component",
- zx::deadline_after(zx::sec(5)), &fd),
+ zx::time::infinite(), &fd),
ZX_OK);
EXPECT_EQ(RecursiveWaitForFile(devmgr.devfs_root(),
"composite-dev/composite",
- zx::deadline_after(zx::sec(5)), &fd),
+ zx::time::infinite(), &fd),
ZX_OK);
const int dirfd = devmgr.devfs_root().get();
@@ -120,9 +120,9 @@ bool enumeration_test() {
EXPECT_EQ(fstatat(dirfd, "sys/platform/test-board", &st, 0), 0);
EXPECT_EQ(fstatat(dirfd, "sys/platform/11:01:1", &st, 0), 0);
EXPECT_EQ(fstatat(dirfd, "sys/platform/11:01:1/child-1", &st, 0), 0);
- EXPECT_EQ(fstatat(dirfd, "sys/platform/11:01:1/child-1/child-2-top", &st, 0), 0);
+ EXPECT_EQ(fstatat(dirfd, "sys/platform/11:01:1/child-1/child-2", &st, 0), 0);
EXPECT_EQ(fstatat(dirfd, "sys/platform/11:01:1/child-1/child-3-top", &st, 0), 0);
- EXPECT_EQ(fstatat(dirfd, "sys/platform/11:01:1/child-1/child-2-top/child-2", &st, 0), 0);
+ EXPECT_EQ(fstatat(dirfd, "sys/platform/11:01:1/child-1/child-2/child-4", &st, 0), 0);
EXPECT_EQ(fstatat(dirfd, "sys/platform/11:01:1/child-1/child-3-top/child-3", &st, 0), 0);
EXPECT_EQ(fstatat(dirfd, "sys/platform/11:01:5/test-gpio/gpio-3/component", &st, 0), 0);
EXPECT_EQ(fstatat(dirfd, "sys/platform/11:01:7/test-clock/clock-1/component", &st, 0), 0);
--
GitLab