Use hwmonio for attribute IO
Moves disparate error handling scenarios from the method doing the
IO to the call point.
Resolves openbmc/openbmc#2166
Change-Id: I3b6d2e175433dd8b2946ae60381901f2d7ca1798
Signed-off-by: Brad Bishop <bradleyb@fuzziesquirrel.com>
diff --git a/fan_speed.cpp b/fan_speed.cpp
index 459b9d0..5e4c9fc 100644
--- a/fan_speed.cpp
+++ b/fan_speed.cpp
@@ -2,6 +2,10 @@
#include "hwmon.hpp"
#include "sysfs.hpp"
#include <experimental/filesystem>
+#include <phosphor-logging/elog-errors.hpp>
+#include <xyz/openbmc_project/Control/Device/error.hpp>
+
+using namespace phosphor::logging;
namespace hwmon
{
@@ -13,12 +17,25 @@
if (curValue != value)
{
//Write target out to sysfs
- curValue = sysfs::writeSysfsWithCallout(value,
- sysfsRoot,
- instance,
- type,
- id,
- entry::target);
+ try
+ {
+ ioAccess.write(
+ value,
+ type,
+ id,
+ entry::target);
+ }
+ catch (const std::system_error& e)
+ {
+ using namespace sdbusplus::xyz::openbmc_project::Control::
+ Device::Error;
+ report<WriteFailure>(
+ xyz::openbmc_project::Control::Device::
+ WriteFailure::CALLOUT_ERRNO(e.code().value()),
+ xyz::openbmc_project::Control::Device::
+ WriteFailure::CALLOUT_DEVICE_PATH(devPath.c_str()));
+ exit(EXIT_FAILURE);
+ }
}
return FanSpeedObject::target(value);
@@ -29,8 +46,7 @@
{
namespace fs = std::experimental::filesystem;
- auto path = sysfsRoot + "/" + instance;
- auto fullPath = sysfs::make_sysfs_path(path,
+ auto fullPath = sysfs::make_sysfs_path(ioAccess.path(),
type::pwm,
id,
entry::enable);
@@ -38,12 +54,25 @@
if (fs::exists(fullPath))
{
//This class always uses RPM mode
- sysfs::writeSysfsWithCallout(enable::rpmMode,
- sysfsRoot,
- instance,
- type::pwm,
- id,
- entry::enable);
+ try
+ {
+ ioAccess.write(
+ enable::rpmMode,
+ type::pwm,
+ id,
+ entry::enable);
+ }
+ catch (const std::system_error& e)
+ {
+ using namespace sdbusplus::xyz::openbmc_project::Control::
+ Device::Error;
+ phosphor::logging::report<WriteFailure>(
+ xyz::openbmc_project::Control::Device::
+ WriteFailure::CALLOUT_ERRNO(e.code().value()),
+ xyz::openbmc_project::Control::Device::
+ WriteFailure::CALLOUT_DEVICE_PATH(devPath.c_str()));
+ exit(EXIT_FAILURE);
+ }
}
}
diff --git a/fan_speed.hpp b/fan_speed.hpp
index eb9882c..b47d718 100644
--- a/fan_speed.hpp
+++ b/fan_speed.hpp
@@ -1,6 +1,7 @@
#pragma once
#include "interface.hpp"
+#include "sysfs.hpp"
namespace hwmon
{
@@ -18,22 +19,23 @@
/**
* @brief Constructs FanSpeed Object
*
- * @param[in] sysfsRoot - The hwmon class root
- * @param[in] instance - The hwmon instance (ex. hwmon1)
+ * @param[in] instancePath - The hwmon instance path
+ * (ex. /sys/class/hwmon/hwmon1)
+ * @param[in] devPath - The /sys/devices sysfs path
* @param[in] id - The hwmon id
* @param[in] bus - Dbus bus object
* @param[in] objPath - Dbus object path
* @param[in] defer - Dbus object registration defer
*/
- FanSpeed(const std::string& sysfsRoot,
- const std::string& instance,
+ FanSpeed(const std::string& instancePath,
+ const std::string& devPath,
const std::string& id,
sdbusplus::bus::bus& bus,
const char* objPath,
bool defer) : FanSpeedObject(bus, objPath, defer),
- sysfsRoot(sysfsRoot),
- instance(instance),
- id(id)
+ id(id),
+ ioAccess(instancePath),
+ devPath(devPath)
{
// Nothing to do here
}
@@ -51,14 +53,15 @@
void enable();
private:
- /** @brief hwmon class root */
- std::string sysfsRoot;
- /** @brief hwmon instance */
- std::string instance;
/** @brief hwmon type */
static constexpr auto type = "fan";
/** @brief hwmon id */
std::string id;
+ /** @brief Hwmon sysfs access. */
+ sysfs::hwmonio::HwmonIO ioAccess;
+ /** @brief Physical device path. */
+ std::string devPath;
+
};
} // namespace hwmon
diff --git a/mainloop.cpp b/mainloop.cpp
index 11f03a5..7399df4 100644
--- a/mainloop.cpp
+++ b/mainloop.cpp
@@ -146,8 +146,8 @@
}
auto addValue(const SensorSet::key_type& sensor,
- const std::string& hwmonRoot,
- const std::string& instance,
+ const std::string& devPath,
+ sysfs::hwmonio::HwmonIO& ioAccess,
ObjectInfo& info)
{
static constexpr bool deferSignals = true;
@@ -157,38 +157,59 @@
auto& obj = std::get<Object>(info);
auto& objPath = std::get<std::string>(info);
- int val = 0;
- bool retry = true;
- size_t count = 10;
-
-
- //Retry for up to a second if device is busy
-
- while (retry)
+ auto readWithRetry = [&ioAccess](
+ const auto& type,
+ const auto& id,
+ const auto& sensor,
+ auto retries,
+ const auto& delay)
{
- try
- {
- val = sysfs::readSysfsWithCallout(hwmonRoot,
- instance,
- sensor.first,
- sensor.second,
- hwmon::entry::input,
- count > 0); //throw DeviceBusy until last attempt
- }
- catch (sysfs::DeviceBusyException& e)
- {
- count--;
- std::this_thread::sleep_for(std::chrono::milliseconds{100});
- continue;
- }
- catch(const std::exception& ioe)
- {
- using namespace sdbusplus::xyz::openbmc_project::Sensor::Device::Error;
- commit<ReadFailure>();
+ auto value = 0;
- return static_cast<std::shared_ptr<ValueObject>>(nullptr);
+ while(true)
+ {
+ try
+ {
+ value = ioAccess.read(type, id, sensor);
+ }
+ catch (const std::system_error& e)
+ {
+ if (e.code().value() != EAGAIN || !retries)
+ {
+ throw;
+ }
+
+ --retries;
+ std::this_thread::sleep_for(delay);
+ continue;
+ }
+ break;
}
- retry = false;
+ return value;
+ };
+
+ static constexpr auto retries = 10;
+ auto val = 0;
+ try
+ {
+ // Retry for up to a second if device is busy
+ val = readWithRetry(
+ sensor.first,
+ sensor.second,
+ hwmon::entry::input,
+ retries,
+ std::chrono::milliseconds{100});
+ }
+ catch (const std::system_error& e)
+ {
+ using namespace sdbusplus::xyz::openbmc_project::Sensor::Device::Error;
+ report<ReadFailure>(
+ xyz::openbmc_project::Sensor::Device::
+ ReadFailure::CALLOUT_ERRNO(e.code().value()),
+ xyz::openbmc_project::Sensor::Device::
+ ReadFailure::CALLOUT_DEVICE_PATH(devPath.c_str()));
+
+ return static_cast<std::shared_ptr<ValueObject>>(nullptr);
}
auto iface = std::make_shared<ValueObject>(bus, objPath.c_str(), deferSignals);
@@ -219,7 +240,8 @@
_devPath(devPath),
_prefix(prefix),
_root(root),
- state()
+ state(),
+ ioAccess(path)
{
std::string p = path;
while (!p.empty() && p.back() == '/')
@@ -293,7 +315,7 @@
objectPath.append(label);
ObjectInfo info(&_bus, std::move(objectPath), Object());
- auto valueInterface = addValue(i.first, _hwmonRoot, _instance, info);
+ auto valueInterface = addValue(i.first, _devPath, ioAccess, info);
if (!valueInterface)
{
#ifdef REMOVE_ON_FAIL
@@ -308,7 +330,7 @@
//TODO openbmc/openbmc#1347
// Handle application restarts to set/refresh fan speed values
auto target = addTarget<hwmon::FanSpeed>(
- i.first, _hwmonRoot, _instance, info);
+ i.first, ioAccess.path(), _devPath, info);
if (target)
{
@@ -367,21 +389,10 @@
int value;
try
{
- try
- {
- value = sysfs::readSysfsWithCallout(_hwmonRoot,
- _instance,
- i.first.first,
- i.first.second,
- hwmon::entry::input);
- }
- catch (sysfs::DeviceBusyException& e)
- {
- //Just go with the current values and try again later.
- //TODO: openbmc/openbmc#2048 could keep an eye on
- //how long the device is actually busy.
- continue;
- }
+ value = ioAccess.read(
+ i.first.first,
+ i.first.second,
+ hwmon::entry::input);
auto& objInfo = std::get<ObjectInfo>(i.second);
auto& obj = std::get<Object>(objInfo);
@@ -410,10 +421,24 @@
}
}
}
- catch (const std::exception& e)
+ catch (const std::system_error& e)
{
- using namespace sdbusplus::xyz::openbmc_project::Sensor::Device::Error;
- commit<ReadFailure>();
+ if (e.code().value() == EAGAIN)
+ {
+ //Just go with the current values and try again later.
+ //TODO: openbmc/openbmc#2048 could keep an eye on
+ //how long the device is actually busy.
+ continue;
+ }
+
+ using namespace sdbusplus::xyz::openbmc_project::
+ Sensor::Device::Error;
+ report<ReadFailure>(
+ xyz::openbmc_project::Sensor::Device::
+ ReadFailure::CALLOUT_ERRNO(e.code().value()),
+ xyz::openbmc_project::Sensor::Device::
+ ReadFailure::CALLOUT_DEVICE_PATH(
+ _devPath.c_str()));
#ifdef REMOVE_ON_FAIL
destroy.push_back(i.first);
diff --git a/mainloop.hpp b/mainloop.hpp
index 90f1efa..b62481d 100644
--- a/mainloop.hpp
+++ b/mainloop.hpp
@@ -5,6 +5,7 @@
#include <experimental/any>
#include <sdbusplus/server.hpp>
#include "sensorset.hpp"
+#include "sysfs.hpp"
#include "interface.hpp"
static constexpr auto default_interval = 1000000;
@@ -79,4 +80,6 @@
SensorState state;
/** @brief Sleep interval in microseconds. */
uint64_t _interval = default_interval;
+ /** @brief Hwmon sysfs access. */
+ sysfs::hwmonio::HwmonIO ioAccess;
};
diff --git a/targets.hpp b/targets.hpp
index 86058ca..863eae5 100644
--- a/targets.hpp
+++ b/targets.hpp
@@ -31,8 +31,7 @@
* @tparam T - The target type
*
* @param[in] sensor - A sensor type and name
- * @param[in] hwmonRoot - The root hwmon path
- * @param[in] instance - The target instance name
+ * @param[in] instance - The target instance path
* @param[in] info - The sdbusplus server connection and interfaces
*
* @return A shared pointer to the target interface object
@@ -40,8 +39,8 @@
*/
template <typename T>
std::shared_ptr<T> addTarget(const SensorSet::key_type& sensor,
- const std::string& hwmonRoot,
- const std::string& instance,
+ const std::string& instancePath,
+ const std::string& devPath,
ObjectInfo& info)
{
std::shared_ptr<T> target;
@@ -53,15 +52,14 @@
auto& objPath = std::get<std::string>(info);
// Check if target sysfs file exists
- auto targetPath = hwmonRoot + '/' + instance;
- auto sysfsFullPath = sysfs::make_sysfs_path(targetPath,
+ auto sysfsFullPath = sysfs::make_sysfs_path(instancePath,
sensor.first,
sensor.second,
hwmon::entry::target);
if (fs::exists(sysfsFullPath))
{
- target = std::make_shared<T>(hwmonRoot,
- instance,
+ target = std::make_shared<T>(instancePath,
+ devPath,
sensor.second,
bus,
objPath.c_str(),