[i2c-hid] Migrate to new DdkInit/DdkUnbind hooks.
The DdkInit() hook is run after automatically after DdkAdd() is called.
By moving the post-add initialization logic into this hook,
we can guarantee the device will be invisible and not able to
be unbound until InitTxn.Reply() is called.
If InitTxn.Reply() is called with a bad status,
the device manager will begin unbinding the device.
When removing a device, we call DdkAsyncRemove() (rather than
DdkRemoveDeprecated), which will begin unbinding the device.
---------------
DDK lifecycle:
---------------
1) DdkAdd()
2) DdkInit()
3) InitTxn.Reply() // Device becomes visible
4) DdkUnbind() // started by InitTxn.Reply(!ZX_OK) / DdkAsyncRemove
5) UnbindTxn.Reply()
6) DdkRelease()
TEST= fx test i2c-hid-test
BUG=37831
BUG=43261
Change-Id: If8028536483599da18b21cdd4c46ba7b6d4e1073
Reviewed-on: https://fuchsia-review.googlesource.com/c/fuchsia/+/397715
Reviewed-by: David Gilhooley <dgilhooley@google.com>
Testability-Review: David Gilhooley <dgilhooley@google.com>
Commit-Queue: Jocelyn Dang <jocelyndang@google.com>
diff --git a/src/ui/input/drivers/i2c-hid/i2c-hid-test.cc b/src/ui/input/drivers/i2c-hid/i2c-hid-test.cc
index 9c70170..d976509 100644
--- a/src/ui/input/drivers/i2c-hid/i2c-hid-test.cc
+++ b/src/ui/input/drivers/i2c-hid/i2c-hid-test.cc
@@ -224,7 +224,7 @@
}
void TearDown() override {
- device_->DdkUnbindDeprecated();
+ device_->DdkAsyncRemove();
EXPECT_TRUE(ddk_.Ok());
// This should delete the object, which means this test should not leak.
@@ -241,7 +241,12 @@
ddk::I2cChannel channel_;
};
-TEST_F(I2cHidTest, HidTestBind) { ASSERT_OK(device_->Bind(channel_)); }
+TEST_F(I2cHidTest, HidTestBind) {
+ ASSERT_OK(device_->Bind(channel_));
+ ASSERT_OK(ddk_.WaitUntilInitComplete());
+ EXPECT_TRUE(ddk_.init_reply().has_value());
+ EXPECT_OK(ddk_.init_reply().value());
+}
TEST_F(I2cHidTest, HidTestQuery) {
ASSERT_OK(device_->Bind(channel_));
@@ -291,6 +296,8 @@
EXPECT_OK(ddk_.WaitUntilRemove());
EXPECT_TRUE(ddk_.Ok());
+ EXPECT_TRUE(ddk_.init_reply().has_value());
+ EXPECT_NOT_OK(ddk_.init_reply().value());
device_->DdkRelease();
}
diff --git a/src/ui/input/drivers/i2c-hid/i2c-hid.cc b/src/ui/input/drivers/i2c-hid/i2c-hid.cc
index 7f34f6c..57a7e99 100644
--- a/src/ui/input/drivers/i2c-hid/i2c-hid.cc
+++ b/src/ui/input/drivers/i2c-hid/i2c-hid.cc
@@ -411,7 +411,10 @@
if (irq_.is_valid()) {
irq_.destroy();
}
- thrd_join(worker_thread_, NULL);
+ if (worker_thread_started_) {
+ worker_thread_started_ = false;
+ thrd_join(worker_thread_, NULL);
+ }
{
fbl::AutoLock lock(&ifc_lock_);
@@ -419,9 +422,9 @@
}
}
-void I2cHidbus::DdkUnbindDeprecated() {
+void I2cHidbus::DdkUnbindNew(ddk::UnbindTxn txn) {
Shutdown();
- DdkRemoveDeprecated();
+ txn.Reply();
}
void I2cHidbus::DdkRelease() { delete this; }
@@ -481,8 +484,20 @@
i2c_.GetInterrupt(0, &irq_);
}
+ status = DdkAdd("i2c-hid");
+ if (status != ZX_OK) {
+ zxlogf(ERROR, "i2c-hid: could not add device: %d", status);
+ return status;
+ }
+ return ZX_OK;
+}
+
+void I2cHidbus::DdkInit(ddk::InitTxn txn) {
+ init_txn_ = std::move(txn);
+
auto worker_thread = [](void* arg) -> int {
auto dev = reinterpret_cast<I2cHidbus*>(arg);
+ dev->worker_thread_started_ = true;
zx_status_t status = ZX_OK;
// Retry the first transaction a few times; in some cases (e.g. on Slate) the device was powered
// on explicitly during enumeration, and there is a warmup period after powering on the device
@@ -497,11 +512,14 @@
zx::nanosleep(zx::deadline_after(zx::msec(100)));
zxlogf(INFO, "i2c-hid: Retrying reading HID descriptor");
}
+ ZX_ASSERT(dev->init_txn_);
+ // This will make the device visible and able to be unbound.
+ dev->init_txn_->Reply(status);
+ // No need to remove the device, as replying to init_txn_ with an error will schedule
+ // unbinding.
if (status != ZX_OK) {
- dev->DdkRemoveDeprecated();
return thrd_error;
}
- dev->DdkMakeVisible();
if (dev->irq_.is_valid()) {
dev->WorkerThreadIrq();
@@ -509,27 +527,21 @@
dev->WorkerThreadNoIrq();
}
// If |stop_worker_thread_| is not set, than we exited the worker thread because
- // of an error and not a shutdown. Call DdkRemove directly.
+ // of an error and not a shutdown. Call DdkAsyncRemove directly. This is a valid
+ // call even if the device is currently unbinding.
if (!dev->stop_worker_thread_) {
- dev->DdkRemoveDeprecated();
+ dev->DdkAsyncRemove();
return thrd_error;
}
return thrd_success;
};
- status = DdkAdd("i2c-hid", DEVICE_ADD_INVISIBLE);
- if (status != ZX_OK) {
- zxlogf(ERROR, "i2c-hid: could not add device: %d", status);
- return status;
- }
-
int rc = thrd_create_with_name(&worker_thread_, worker_thread, this, "i2c-hid-worker-thread");
if (rc != thrd_success) {
- DdkRemoveDeprecated();
- return ZX_ERR_INTERNAL;
+ return init_txn_->Reply(ZX_ERR_INTERNAL);
}
-
- return ZX_OK;
+ // If the worker thread was created successfully, it is in charge of replying to the init txn,
+ // which will make the device visible.
}
static zx_status_t i2c_hid_bind(void* ctx, zx_device_t* parent) {
diff --git a/src/ui/input/drivers/i2c-hid/i2c-hid.h b/src/ui/input/drivers/i2c-hid/i2c-hid.h
index 8466336..f167769 100644
--- a/src/ui/input/drivers/i2c-hid/i2c-hid.h
+++ b/src/ui/input/drivers/i2c-hid/i2c-hid.h
@@ -10,6 +10,7 @@
#include <threads.h>
#include <memory>
+#include <optional>
#include <ddktl/device.h>
#include <ddktl/protocol/hidbus.h>
@@ -45,7 +46,7 @@
} __PACKED;
class I2cHidbus;
-using DeviceType = ddk::Device<I2cHidbus, ddk::UnbindableDeprecated>;
+using DeviceType = ddk::Device<I2cHidbus, ddk::Initializable, ddk::UnbindableNew>;
class I2cHidbus : public DeviceType, public ddk::HidbusProtocol<I2cHidbus, ddk::base_protocol> {
public:
@@ -68,26 +69,30 @@
zx_status_t HidbusGetProtocol(uint8_t* protocol) { return ZX_ERR_NOT_SUPPORTED; }
zx_status_t HidbusSetProtocol(uint8_t protocol) { return ZX_OK; }
- void DdkUnbindDeprecated();
+ void DdkInit(ddk::InitTxn txn);
+ void DdkUnbindNew(ddk::UnbindTxn txn);
void DdkRelease();
zx_status_t Bind(ddk::I2cChannel i2c);
zx_status_t ReadI2cHidDesc(I2cHidDesc* hiddesc);
- void Shutdown();
-
// Must be called with i2c_lock held.
void WaitForReadyLocked() __TA_REQUIRES(i2c_lock_);
zx_status_t Reset(bool force) __TA_EXCLUDES(i2c_lock_);
private:
+ void Shutdown();
+
I2cHidDesc hiddesc_ = {};
// Signaled when reset received.
fbl::ConditionVariable i2c_reset_cnd_;
- std::atomic<bool> stop_worker_thread_ = false;
+ std::optional<ddk::InitTxn> init_txn_;
+
+ std::atomic_bool worker_thread_started_ = false;
+ std::atomic_bool stop_worker_thread_ = false;
thrd_t worker_thread_;
zx::interrupt irq_;
// The functions to be run in the worker thread. They are responsible for initializing the
