AU: fix bug in GPIO handler that caused failed discovery to be ignored
This solves two issues:
* The GPIO handler will attempt to use GPIO devices even when discovery
via udev has failed. Not any more.
* GPIO discovery would return success even when it failed on a previous
attempt. Now it'll reflect the actual result of the discovery attempt.
* Reporting of file descriptor open errors is obscured due to
intermittent operations that reset errno, now fixed.
* Added test case to ensure that repeat GPIO discovery is not attempted,
and that test mode check will automatically fail if a previous
initialization/discovery had failed.
BUG=None
TEST=Passes unit tests
TEST=All symptoms gone on real update attempt w/ x86-alex
Change-Id: I01a7b1e316dbb5b94487a5aad1560d994feae9ff
Reviewed-on: https://gerrit.chromium.org/gerrit/40946
Commit-Queue: Gilad Arnold <garnold@chromium.org>
Reviewed-by: Gilad Arnold <garnold@chromium.org>
Tested-by: Gilad Arnold <garnold@chromium.org>
diff --git a/gpio_handler.cc b/gpio_handler.cc
index f49a555..8627ba2 100644
--- a/gpio_handler.cc
+++ b/gpio_handler.cc
@@ -46,7 +46,8 @@
: udev_iface_(udev_iface),
fd_(fd),
is_cache_test_mode_(is_cache_test_mode),
- is_discovery_attempted_(false) {
+ is_discovery_attempted_(false),
+ is_discovery_successful_(false) {
CHECK(udev_iface && fd);
// Ensure there's only one instance of this class.
@@ -57,9 +58,8 @@
ResetTestModeSignalingFlags();
// If GPIO discovery not deferred, do it.
- if (!(is_defer_discovery || DiscoverGpios())) {
- LOG(WARNING) << "GPIO discovery failed";
- }
+ if (!is_defer_discovery)
+ DiscoverGpios();
}
StandardGpioHandler::~StandardGpioHandler() {
@@ -67,19 +67,19 @@
}
bool StandardGpioHandler::IsTestModeSignaled() {
- // Attempt GPIO discovery.
- if (!DiscoverGpios()) {
- LOG(WARNING) << "GPIO discovery failed";
- }
+ bool is_returning_cached = false; // for logging purposes
- // Force a check if so requested.
- if (!is_cache_test_mode_)
- ResetTestModeSignalingFlags();
+ // Attempt GPIO discovery first.
+ if (DiscoverGpios()) {
+ // Force a check if so requested.
+ if (!is_cache_test_mode_)
+ ResetTestModeSignalingFlags();
- bool is_returning_cached = !is_first_check_; // for logging purposes
- if (is_first_check_) {
- is_first_check_ = false;
- DoTestModeSignalingProtocol();
+ is_returning_cached = !is_first_check_; // for logging purposes
+ if (is_first_check_) {
+ is_first_check_ = false;
+ DoTestModeSignalingProtocol();
+ }
}
LOG(INFO) << "result: " << (is_test_mode_ ? "test" : "normal") << " mode"
@@ -255,14 +255,14 @@
bool StandardGpioHandler::DiscoverGpios() {
if (is_discovery_attempted_)
- return true;
+ return is_discovery_successful_;
is_discovery_attempted_ = true;
// Obtain libudev instance and attach to a dedicated closer.
struct udev* udev;
if (!(udev = udev_iface_->New())) {
- LOG(ERROR) << "failed to obtain libudev instance";
+ LOG(ERROR) << "failed to obtain libudev instance, aborting GPIO discovery";
return false;
}
scoped_ptr<UdevInterface::UdevCloser>
@@ -284,6 +284,7 @@
}
}
+ is_discovery_successful_ = true;
return true;
}
@@ -302,13 +303,14 @@
string file_name = StringPrintf("%s/%s", gpios_[id].dev_path.c_str(),
dev_name);
if (!fd_->Open(file_name.c_str(), (is_write ? O_WRONLY : O_RDONLY))) {
- const string err_str = StringPrintf("failed to open %s (%s) for %s",
- file_name.c_str(), gpio_defs_[id].name,
- (is_write ? "writing" : "reading"));
if (fd_->IsSettingErrno()) {
- PLOG(ERROR) << err_str;
+ PLOG(ERROR) << "failed to open " << file_name
+ << " (" << gpio_defs_[id].name << ") for "
+ << (is_write ? "writing" : "reading");
} else {
- LOG(ERROR) << err_str;
+ LOG(ERROR) << "failed to open " << file_name
+ << " (" << gpio_defs_[id].name << ") for "
+ << (is_write ? "writing" : "reading");
}
return false;
}
diff --git a/gpio_handler.h b/gpio_handler.h
index c71043a..d1db7c2 100644
--- a/gpio_handler.h
+++ b/gpio_handler.h
@@ -216,7 +216,7 @@
void ResetTestModeSignalingFlags();
// Attempt GPIO discovery, at most once. Returns true if discovery process was
- // successfully completed or already attempted, false otherwise.
+ // successfully completed, false otherwise.
bool DiscoverGpios();
// Assigns a copy of the device name of GPIO |id| to |dev_path_p|. Assumes
@@ -290,8 +290,10 @@
// cached, or reestablished on each query.
const bool is_cache_test_mode_;
- // Indicates whether GPIO discovery was performed.
+ // Indicates whether GPIO discovery was performed, and whether it's been
+ // successful.
bool is_discovery_attempted_;
+ bool is_discovery_successful_;
// Persistent state of the test mode check.
bool is_first_check_;
diff --git a/gpio_handler_unittest.cc b/gpio_handler_unittest.cc
index 53ffb61..3aaae5c 100644
--- a/gpio_handler_unittest.cc
+++ b/gpio_handler_unittest.cc
@@ -23,6 +23,7 @@
false, false);
mock_udev.ExpectAllResourcesDeallocated();
mock_udev.ExpectDiscoverySuccess();
+ mock_file_descriptor.ExpectNumOpenAttempted(0);
}
TEST(StandardGpioHandlerTest, MultiGpioChipInitTest) {
@@ -35,6 +36,24 @@
false, false);
mock_udev.ExpectAllResourcesDeallocated();
mock_udev.ExpectDiscoveryFail();
+ mock_file_descriptor.ExpectNumOpenAttempted(0);
+}
+
+TEST(StandardGpioHandlerTest, FailedFirstGpioInitTest) {
+ // Attempt GPIO discovery with a udev mock that fails the initialization on
+ // the first attempt, then check for test mode. Ensure that (a) discovery is
+ // not attempted a second time, and (b) test mode check returns false (the
+ // default) without attempting to use GPIO signals.
+ FailInitGpioMockUdevInterface mock_udev;
+ TestModeGpioMockFileDescriptor
+ mock_file_descriptor(base::TimeDelta::FromSeconds(1));
+ StandardGpioHandler gpio_handler(&mock_udev, &mock_file_descriptor,
+ false, false);
+ EXPECT_FALSE(gpio_handler.IsTestModeSignaled());
+ mock_udev.ExpectAllResourcesDeallocated();
+ mock_udev.ExpectDiscoveryFail();
+ mock_udev.ExpectNumInitAttempts(1);
+ mock_file_descriptor.ExpectNumOpenAttempted(0);
}
TEST(StandardGpioHandlerTest, TestModeGpioSignalingTest) {
diff --git a/gpio_mock_file_descriptor.cc b/gpio_mock_file_descriptor.cc
index 16f9473..cadec4c 100644
--- a/gpio_mock_file_descriptor.cc
+++ b/gpio_mock_file_descriptor.cc
@@ -49,7 +49,8 @@
GpioMockFileDescriptor::GpioMockFileDescriptor()
: gpio_id_(kMockGpioIdMax),
- gpio_subdev_(kMockGpioSubdevMax) {
+ gpio_subdev_(kMockGpioSubdevMax),
+ num_open_attempted_(0) {
// All GPIOs are initially in the input direction, their read value is "up",
// and they assume an initial write value of "up" with current (init) time.
Time init_time = Time::Now();
@@ -67,6 +68,8 @@
}
bool GpioMockFileDescriptor::Open(const char* path, int flags, mode_t mode) {
+ num_open_attempted_++;
+
EXPECT_EQ(gpio_id_, kMockGpioIdMax);
if (gpio_id_ != kMockGpioIdMax)
return false;
@@ -265,6 +268,11 @@
return is_all_gpios_restored_to_default;
}
+bool GpioMockFileDescriptor::ExpectNumOpenAttempted(unsigned count) {
+ EXPECT_EQ(num_open_attempted_, count);
+ return (num_open_attempted_ == count);
+}
+
size_t GpioMockFileDescriptor::DecodeGpioString(const char* buf,
size_t count,
const char** strs,
diff --git a/gpio_mock_file_descriptor.h b/gpio_mock_file_descriptor.h
index a4fd4e8..c616b41 100644
--- a/gpio_mock_file_descriptor.h
+++ b/gpio_mock_file_descriptor.h
@@ -48,6 +48,10 @@
// otherwise, will fail the current test.
virtual bool ExpectAllGpiosRestoredToDefault();
+ // Returns true iff the number of open attempts equals the argument;
+ // otherwise, will fail the current test.
+ virtual bool ExpectNumOpenAttempted(unsigned count);
+
protected:
// A pair of write value and time at which it was written.
struct MockGpioWriteEvent {
@@ -135,6 +139,9 @@
// The identifier of the currently accessed GPIO sub-device.
MockGpioSubdev gpio_subdev_;
+
+ // Counter for the number of files that were opened with this interface.
+ unsigned num_open_attempted_;
};
diff --git a/gpio_mock_udev_interface.cc b/gpio_mock_udev_interface.cc
index 22c7b7f..d7e7a41 100644
--- a/gpio_mock_udev_interface.cc
+++ b/gpio_mock_udev_interface.cc
@@ -368,4 +368,15 @@
}
+void FailInitGpioMockUdevInterface::ExpectNumInitAttempts(
+ unsigned count) const {
+ EXPECT_EQ(num_init_attempts_, count);
+}
+
+struct udev* FailInitGpioMockUdevInterface::New() {
+ // Increment udev init attempt counter, failing the first attempt.
+ num_init_attempts_++;
+ return num_init_attempts_ == 1 ? NULL : StandardGpioMockUdevInterface::New();
+}
+
} // namespace chromeos_update_engine
diff --git a/gpio_mock_udev_interface.h b/gpio_mock_udev_interface.h
index 724a0f4..04c29e0 100644
--- a/gpio_mock_udev_interface.h
+++ b/gpio_mock_udev_interface.h
@@ -129,6 +129,21 @@
MockDevice gpio_chip2_dev_;
};
+// A udev mock that fails to provide constructs for proper GPIO initialization.
+class FailInitGpioMockUdevInterface : public StandardGpioMockUdevInterface {
+ public:
+ // Default constructor.
+ FailInitGpioMockUdevInterface() : num_init_attempts_(0) {}
+
+ virtual void ExpectNumInitAttempts(unsigned count) const;
+
+ protected:
+ virtual struct udev* New();
+
+ private:
+ unsigned num_init_attempts_;
+};
+
} // namespace chromeos_update_engine
#endif // CHROMEOS_PLATFORM_UPDATE_ENGINE_GPIO_MOCK_UDEV_INTERFACE_H__