regulators: Stop boot if cfg file not found/valid
Stop the boot during a BMC chassison/poweron if the regulators JSON
configuration file cannot be found or is invalid.
During the boot, the regulators application configures the voltage
regulators in the system. One of the most important voltage regulator
settings that can be changed is the output voltage.
The rules on how to configure the voltage regulators are defined in the
JSON config file. If this file cannot be found or is invalid, the
voltage regulators cannot be configured.
That would mean that the voltage regulators would be turned on during
the boot using their hardware default settings. It is common for some
of the hardware defaults to be incorrect, occasionally by a significant
amount.
If the hardware defaults are significantly off, it is possible that
hardware damage could occur. For example, if the output voltage is too
high, downstream hardware components could be damaged.
For this reason, the boot needs to be stopped. A critical error is
logged, and the executable run by the systemd service file exits with a
non-zero return code indicating failure.
Note that this behavior will only occur if the phosphor-regulators
application is being used. The application must be explicitly enabled
using a meson build option.
Tested:
* Verified that boot is stopped if JSON config file not found/valid
* Verified critical error logged
* Verified regsctl exits with non-zero value causing config service to
fail
* Verified that boot continues if JSON config file valid
* Tested with both 'obmcutil chassison' and 'obmcutil poweron'
* For complete test plan see
https://gist.github.com/smccarney/8801cad1fe1c4ae8913e57d9474bfaac
Signed-off-by: Shawn McCarney <shawnmm@us.ibm.com>
Change-Id: I211f19cd9b98daea86645611633f8943c44b0f75
diff --git a/phosphor-regulators/src/error_logging.cpp b/phosphor-regulators/src/error_logging.cpp
index bf3467c..bc3d388 100644
--- a/phosphor-regulators/src/error_logging.cpp
+++ b/phosphor-regulators/src/error_logging.cpp
@@ -36,9 +36,20 @@
void DBusErrorLogging::logConfigFileError(Entry::Level severity,
Journal& journal)
{
+ std::string message{
+ "xyz.openbmc_project.Power.Regulators.Error.ConfigFile"};
+ if (severity == Entry::Level::Critical)
+ {
+ // Specify a different message property for critical config file errors.
+ // These are logged when a critical operation cannot be performed due to
+ // the lack of a valid config file. These errors may require special
+ // handling, like stopping a power on attempt.
+ message =
+ "xyz.openbmc_project.Power.Regulators.Error.ConfigFile.Critical";
+ }
+
std::map<std::string, std::string> additionalData{};
- logError("xyz.openbmc_project.Power.Regulators.Error.ConfigFile", severity,
- additionalData, journal);
+ logError(message, severity, additionalData, journal);
}
void DBusErrorLogging::logDBusError(Entry::Level severity, Journal& journal)
diff --git a/phosphor-regulators/src/interfaces/manager_interface.cpp b/phosphor-regulators/src/interfaces/manager_interface.cpp
index f4df124..1b35d25 100644
--- a/phosphor-regulators/src/interfaces/manager_interface.cpp
+++ b/phosphor-regulators/src/interfaces/manager_interface.cpp
@@ -54,7 +54,7 @@
reply.method_return();
}
- catch (sdbusplus::internal_exception_t& e)
+ catch (sdbusplus::exception_t& e)
{
return sd_bus_error_set(error, e.name(), e.description());
}
@@ -89,7 +89,7 @@
reply.method_return();
}
- catch (sdbusplus::internal_exception_t& e)
+ catch (sdbusplus::exception_t& e)
{
return sd_bus_error_set(error, e.name(), e.description());
}
diff --git a/phosphor-regulators/src/manager.cpp b/phosphor-regulators/src/manager.cpp
index 778e4b4..94f1431 100644
--- a/phosphor-regulators/src/manager.cpp
+++ b/phosphor-regulators/src/manager.cpp
@@ -22,6 +22,8 @@
#include "rule.hpp"
#include "utility.hpp"
+#include <xyz/openbmc_project/Common/error.hpp>
+
#include <algorithm>
#include <chrono>
#include <exception>
@@ -101,14 +103,18 @@
}
else
{
+ // Write error message to journal
services.getJournal().logError("Unable to configure regulator devices: "
"Configuration file not loaded");
- // TODO: Log error
- }
- // TODO Configuration errors that should halt poweron,
- // throw InternalFailure exception (or similar) to
- // fail the call(busctl) to this method
+ // Log critical error since regulators could not be configured. Could
+ // cause hardware damage if default regulator settings are very wrong.
+ services.getErrorLogging().logConfigFileError(Entry::Level::Critical,
+ services.getJournal());
+
+ // Throw InternalFailure to propogate error status to D-Bus client
+ throw sdbusplus::xyz::openbmc_project::Common::Error::InternalFailure{};
+ }
}
void Manager::interfacesAddedHandler(sdbusplus::message::message& msg)
@@ -331,7 +337,9 @@
services.getJournal().logError(exception_utils::getMessages(e));
services.getJournal().logError("Unable to load configuration file");
- // TODO: Create error log entry
+ // Log error
+ services.getErrorLogging().logConfigFileError(Entry::Level::Error,
+ services.getJournal());
}
}
diff --git a/phosphor-regulators/src/regsctl/main.cpp b/phosphor-regulators/src/regsctl/main.cpp
index e00a7c1..9f65c8b 100644
--- a/phosphor-regulators/src/regsctl/main.cpp
+++ b/phosphor-regulators/src/regsctl/main.cpp
@@ -19,6 +19,7 @@
#include <CLI/CLI.hpp>
#include <algorithm>
+#include <exception>
#include <iostream>
#include <string>
@@ -28,66 +29,47 @@
{
auto rc = 0;
- bool monitorEnable = false;
- bool monitorDisable = false;
-
- CLI::App app{"Regulators control app for OpenBMC phosphor-regulators"};
-
- // Add dbus methods group
- auto methods = app.add_option_group("Methods");
- // Configure method
- CLI::App* config =
- methods->add_subcommand("config", "Configure regulators");
- config->set_help_flag("-h,--help", "Configure regulators method help");
- // Monitor method
- CLI::App* monitor =
- methods->add_subcommand("monitor", "Monitor regulators");
- monitor->set_help_flag("-h,--help", "Monitor regulators method help");
- monitor->add_flag("-e,--enable", monitorEnable,
- "Enable regulator monitoring");
- monitor->add_flag("-d,--disable", monitorDisable,
- "Disable regulator monitoring");
- // Monitor subcommand requires only 1 option be provided
- monitor->require_option(1);
- // Methods group requires only 1 subcommand to be given
- methods->require_subcommand(1);
-
- CLI11_PARSE(app, argc, argv);
-
- if (app.got_subcommand("config"))
+ try
{
- auto resp = callMethod("Configure");
- if (!resp)
+ bool monitorEnable = false;
+ bool monitorDisable = false;
+
+ CLI::App app{"Regulators control app for OpenBMC phosphor-regulators"};
+
+ // Add dbus methods group
+ auto methods = app.add_option_group("Methods");
+ // Configure method
+ CLI::App* config =
+ methods->add_subcommand("config", "Configure regulators");
+ config->set_help_flag("-h,--help", "Configure regulators method help");
+ // Monitor method
+ CLI::App* monitor =
+ methods->add_subcommand("monitor", "Monitor regulators");
+ monitor->set_help_flag("-h,--help", "Monitor regulators method help");
+ monitor->add_flag("-e,--enable", monitorEnable,
+ "Enable regulator monitoring");
+ monitor->add_flag("-d,--disable", monitorDisable,
+ "Disable regulator monitoring");
+ // Monitor subcommand requires only 1 option be provided
+ monitor->require_option(1);
+ // Methods group requires only 1 subcommand to be given
+ methods->require_subcommand(1);
+
+ CLI11_PARSE(app, argc, argv);
+
+ if (app.got_subcommand("config"))
{
- rc = -1;
- std::cerr << "regsctl: Failed to begin configuring regulators"
- << std::endl;
+ callMethod("Configure");
+ }
+ else if (app.got_subcommand("monitor"))
+ {
+ callMethod("Monitor", monitorEnable);
}
}
- if (app.got_subcommand("monitor"))
+ catch (const std::exception& e)
{
- if (monitorEnable)
- {
- auto resp = callMethod("Monitor", true);
- if (!resp)
- {
- rc = -1;
- std::cerr << "regsctl: Failed to enable "
- "monitoring of regulators"
- << std::endl;
- }
- }
- if (monitorDisable)
- {
- auto resp = callMethod("Monitor", false);
- if (!resp)
- {
- rc = -1;
- std::cerr << "regsctl: Failed to disable "
- "monitoring of regulators"
- << std::endl;
- }
- }
+ rc = 1;
+ std::cerr << e.what() << std::endl;
}
return rc;