Skip to contentSkip to conversation

lib and tuning cleanup series

PATCH In review Joe Konno 2 participants 9 messages Sep 8, 2017 – Sep 11, 2017

lore
Share & tools
lore

Joe Konno

author
#0 lore raw

cover lib and tuning cleanup series

From: Joe Konno <joe.konno(a)intel.com>

This series could be split in two, one for the libpci stuff in "lib", and another for "tuning" cleanups. The cleanups in "lib" are not dependent on the cleanups in "tuning" and vice-versa. There should be no changes to PowerTOP operations or usage as a result of these patches-- code cleanups only.

I was re-learning libpci a while back and, during my walks along various PCI buses, spotted a trivial refactor for the libpci functionality in "lib". This is to make it (slightly) more tempting to embark on future changes to walk PCI buses with libpci instead of sysfs-- especially in "tuning" cases.

While looking into additional runtime PM tunables, I stumbled upon #defines used for tunable states. Implemented those states in an enum instead. Proved easy as those defines were not used as factors in math operations, so they were acting as enum entries to begin with. After the enum was implemented, gcc warned about unhandled states in tunable::result_string()'s switch statement, so I fixed it.

Joe Konno (4):
lib: refactor pci_access checks
lib: have check_pci_access() scan the entire bus
tuning: use tunable_state enum instead of defines
tuning: complete tunable result switch

 src/lib.cpp                | 14 ++++++++++----
 src/tuning/bluetooth.cpp   |  6 +++---
 src/tuning/bluetooth.h     |  2 +-
 src/tuning/ethernet.cpp    |  4 ++--
 src/tuning/ethernet.h      |  2 +-
 src/tuning/runtime.cpp     |  2 +-
 src/tuning/runtime.h       |  2 +-
 src/tuning/tunable.h       | 26 +++++++++++++++++---------
 src/tuning/tuningi2c.cpp   |  2 +-
 src/tuning/tuningi2c.h     |  2 +-
 src/tuning/tuningsysfs.cpp |  2 +-
 src/tuning/tuningsysfs.h   |  2 +-
 src/tuning/tuningusb.cpp   |  2 +-
 src/tuning/tuningusb.h     |  2 +-
 src/tuning/wifi.cpp        |  2 +-
 src/tuning/wifi.h          |  2 +-
 16 files changed, 44 insertions(+), 30 deletions(-)
Signature · 1 line
2.13.5
author
#1 lore raw

PATCH 1/4 lib: refactor pci_access checks

From: Joe Konno <joe.konno(a)intel.com>

Pull the libpci initialization bits out and into its own dedicated function.

Signed-off-by Joe Konno <joe.konno(a)intel.com>
---
 src/lib.cpp | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)
src/lib.cpp +9 -4
index 5e48f375d087..2b95e64f427b 100644
@@ -296,16 +296,21 @@ void format_watts(double W, char *buffer, unsigned int len)
 #ifndef HAVE_NO_PCI
 static struct pci_access *pci_access;
 
+static void check_pci_access(void)
+{
+	if (!pci_access) {
+		pci_access = pci_alloc();
+		pci_init(pci_access);
+	}
+}
+
 char *pci_id_to_name(uint16_t vendor, uint16_t device, char *buffer, int len)
 {
 	char *ret;
 
 	buffer[0] = 0;
 
-	if (!pci_access) {
-		pci_access = pci_alloc();
-		pci_init(pci_access);
-	}
+	check_pci_access();
 
 	ret = pci_lookup_name(pci_access, buffer, len, PCI_LOOKUP_VENDOR | PCI_LOOKUP_DEVICE, vendor, device);
 
Signature · 1 line
2.13.5
#2 lore raw

Re: [Powertop] [PATCH 1/4 v1] lib: refactor pci_access checks

Quoted 3 lines · Joe Konno

From: Joe Konno <joe.konno(a)intel.com>

Pull the libpci initialization bits out and into its own dedicated function.

I think the name you choose for the function is odd since what it does is that it creates the pci_access object if it doesn't exist. It certainly ain't checking anything.

Maybe something along the lines of create_pci_access or initialize_pci_access would be a better name?

Quoted 42 lines
Signed-off-by Joe Konno <joe.konno(a)intel.com>
---
 src/lib.cpp | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)
src/lib.cpp +9 -4
index 5e48f375d087..2b95e64f427b 100644
@@ -296,16 +296,21 @@ void format_watts(double W, char *buffer, unsigned int len)
 #ifndef HAVE_NO_PCI
 static struct pci_access *pci_access;
 
+static void check_pci_access(void)
+{
+	if (!pci_access) {
+		pci_access = pci_alloc();
+		pci_init(pci_access);
+	}
+}
+
 char *pci_id_to_name(uint16_t vendor, uint16_t device, char *buffer, int len)
 {
 	char *ret;
 
 	buffer[0] = 0;
 
-	if (!pci_access) {
-		pci_access = pci_alloc();
-		pci_init(pci_access);
-	}
+	check_pci_access();
 
 	ret = pci_lookup_name(pci_access, buffer, len, PCI_LOOKUP_VENDOR | PCI_LOOKUP_DEVICE, vendor, device);
 
Signature · 6 lines
2.13.5

_______________________________________________
PowerTop mailing list
PowerTop(a)lists.01.org
https://lists.01.org/mailman/listinfo/powertop
author
#3 lore raw

PATCH 2/4 lib: have check_pci_access() scan the entire bus

In reply to Joe Konno

From: Joe Konno <joe.konno(a)intel.com>

Signed-off-by Joe Konno <joe.konno(a)intel.com>
---
 src/lib.cpp | 1 +
 1 file changed, 1 insertion(+)
src/lib.cpp +1 -0
index 2b95e64f427b..d2a9de71e7aa 100644
@@ -301,6 +301,7 @@ static void check_pci_access(void)
 	if (!pci_access) {
 		pci_access = pci_alloc();
 		pci_init(pci_access);
+		pci_scan_bus(pci_access);
 	}
 }
 
Signature · 1 line
2.13.5
author
#4 lore raw

PATCH 3/4 tuning: use tunable_state enum instead of defines

In reply to Joe Konno

From: Joe Konno <joe.konno(a)intel.com>

Signed-off-by Joe Konno <joe.konno(a)intel.com>
---
 src/tuning/bluetooth.cpp   |  6 +++---
 src/tuning/bluetooth.h     |  2 +-
 src/tuning/ethernet.cpp    |  4 ++--
 src/tuning/ethernet.h      |  2 +-
 src/tuning/runtime.cpp     |  2 +-
 src/tuning/runtime.h       |  2 +-
 src/tuning/tunable.h       | 14 ++++++++------
 src/tuning/tuningi2c.cpp   |  2 +-
 src/tuning/tuningi2c.h     |  2 +-
 src/tuning/tuningsysfs.cpp |  2 +-
 src/tuning/tuningsysfs.h   |  2 +-
 src/tuning/tuningusb.cpp   |  2 +-
 src/tuning/tuningusb.h     |  2 +-
 src/tuning/wifi.cpp        |  2 +-
 src/tuning/wifi.h          |  2 +-
 15 files changed, 25 insertions(+), 23 deletions(-)
src/tuning/bluetooth.cpp +3 -3
index 295884952986..2e3d1ff6a013 100644
@@ -103,16 +103,16 @@ struct hci_dev_info {
 
 static int previous_bytes = -1;
 static time_t last_check_time = 0;
-static int last_check_result;
+static tunable_state last_check_result;
 
-int bt_tunable::good_bad(void)
+tunable_state bt_tunable::good_bad(void)
 {
 	struct hci_dev_info devinfo;
 	FILE *file = 0;
 	int fd;
 	int thisbytes = 0;
 	int ret;
-	int result = TUNE_GOOD;
+	tunable_state result = TUNE_GOOD;
 
 	fd = socket(AF_BLUETOOTH, SOCK_RAW, BTPROTO_HCI);
 	if (fd < 0)
src/tuning/bluetooth.h +1 -1
index ecb667d5da13..dd9a627c584e 100644
@@ -35,7 +35,7 @@ class bt_tunable : public tunable {
 public:
 	bt_tunable(void);
 
-	virtual int good_bad(void);
+	virtual tunable_state good_bad(void);
 
 	virtual void toggle(void);
 
src/tuning/ethernet.cpp +2 -2
index 5b128d1d98d6..2613848ed413 100644
@@ -57,13 +57,13 @@ ethernet_tunable::ethernet_tunable(const char *iface) : tunable("", 0.3, _("Good
 }
 
 
-int ethernet_tunable::good_bad(void)
+tunable_state ethernet_tunable::good_bad(void)
 {
 	int sock;
 	struct ifreq ifr;
 	struct ethtool_wolinfo wol;
 	int ret;
-	int result = TUNE_GOOD;
+	tunable_state result = TUNE_GOOD;
 
 	memset(&ifr, 0, sizeof(struct ifreq));
 
src/tuning/ethernet.h +1 -1
index 85810fbaae91..f72987bab4ae 100644
@@ -36,7 +36,7 @@ public:
 	char interf[4096];
 	ethernet_tunable(const char *iface);
 
-	virtual int good_bad(void);
+	virtual tunable_state good_bad(void);
 
 	virtual void toggle(void);
 
src/tuning/runtime.cpp +1 -1
index 9b6abeda0970..78a246197ddd 100644
@@ -82,7 +82,7 @@ runtime_tunable::runtime_tunable(const char *path, const char *bus, const char *
 	snprintf(toggle_bad, sizeof(toggle_bad), "echo 'on' > '%s';", runtime_path);
 }
 
-int runtime_tunable::good_bad(void)
+tunable_state runtime_tunable::good_bad(void)
 {
 	string content;
 
src/tuning/runtime.h +1 -1
index b292a0fabff3..5a29defe31f7 100644
@@ -36,7 +36,7 @@ class runtime_tunable : public tunable {
 public:
 	runtime_tunable(const char *runtime_path, const char *bus, const char *dev);
 
-	virtual int good_bad(void);
+	virtual tunable_state good_bad(void);
 
 	virtual void toggle(void);
 
src/tuning/tunable.h +8 -6
index 3372378ea8ba..5ea7cdfb5389 100644
@@ -31,11 +31,13 @@
 
 using namespace std;
 
-#define TUNE_GOOD    1
-#define TUNE_BAD     -1
-#define TUNE_UNFIXABLE -2
-#define TUNE_UNKNOWN 0
-#define TUNE_NEUTRAL 0
+enum tunable_state {
+	TUNE_GOOD,
+	TUNE_BAD,
+	TUNE_UNFIXABLE,
+	TUNE_NEUTRAL,
+	TUNE_UNKNOWN
+};
 
 class tunable {
 
@@ -53,7 +55,7 @@ public:
 	tunable(const char *str, double _score, const char *good = "", const char *bad = "", const char *neutral ="");
 	virtual ~tunable() {};
 
-	virtual int good_bad(void) { return TUNE_NEUTRAL; }
+	virtual tunable_state good_bad(void) { return TUNE_NEUTRAL; }
 
 	virtual char *result_string(void)
 	{
src/tuning/tuningi2c.cpp +1 -1
index b75e8119137d..1a2859897831 100644
@@ -62,7 +62,7 @@ i2c_tunable::i2c_tunable(const char *path, const char *name, bool is_adapter) :
 	snprintf(toggle_bad, sizeof(toggle_bad), "echo 'on' > '%s';", i2c_path);
 }
 
-int i2c_tunable::good_bad(void)
+tunable_state i2c_tunable::good_bad(void)
 {
 	string content;
 
src/tuning/tuningi2c.h +1 -1
index 8fd87845f325..86827215ff76 100644
@@ -32,7 +32,7 @@ class i2c_tunable : public tunable {
 public:
 	i2c_tunable(const char *path, const char *name, bool is_adapter);
 
-	virtual int good_bad(void);
+	virtual tunable_state good_bad(void);
 
 	virtual void toggle(void);
 
src/tuning/tuningsysfs.cpp +1 -1
index 954f52e4d81b..3aaa948d6553 100644
@@ -51,7 +51,7 @@ sysfs_tunable::sysfs_tunable(const char *str, const char *_sysfs_path, const cha
 	snprintf(toggle_bad, sizeof(toggle_bad), "echo '%s' > '%s';", bad_value, sysfs_path);
 }
 
-int sysfs_tunable::good_bad(void)
+tunable_state sysfs_tunable::good_bad(void)
 {
 	char current_value[4096], *c;
 	ifstream file;
src/tuning/tuningsysfs.h +1 -1
index 57b9de725c02..f32f8ec8e1c4 100644
@@ -39,7 +39,7 @@ class sysfs_tunable : public tunable {
 public:
 	sysfs_tunable(const char *str, const char *sysfs_path, const char *target_content);
 
-	virtual int good_bad(void);
+	virtual tunable_state good_bad(void);
 
 	virtual void toggle(void);
 
src/tuning/tuningusb.cpp +1 -1
index 3b7b2b165596..8a65eb67c8ea 100644
@@ -78,7 +78,7 @@ usb_tunable::usb_tunable(const char *path, const char *name) : tunable("", 0.9,
 	snprintf(toggle_bad, sizeof(toggle_bad), "echo 'on' > '%s';", usb_path);
 }
 
-int usb_tunable::good_bad(void)
+tunable_state usb_tunable::good_bad(void)
 {
 	string content;
 
src/tuning/tuningusb.h +1 -1
index 4e27e3abd5d7..5023f9b4cf1a 100644
@@ -37,7 +37,7 @@ class usb_tunable : public tunable {
 public:
 	usb_tunable(const char *usb_path, const char *path);
 
-	virtual int good_bad(void);
+	virtual tunable_state good_bad(void);
 
 	virtual void toggle(void);
 
src/tuning/wifi.cpp +1 -1
index f7a91ec36834..f957d376c940 100644
@@ -51,7 +51,7 @@ wifi_tunable::wifi_tunable(const char *_iface) : tunable("", 1.5, _("Good"), _("
 	snprintf(toggle_bad, sizeof(toggle_bad), "iw dev %s set power_save off", iface);
 }
 
-int wifi_tunable::good_bad(void)
+tunable_state wifi_tunable::good_bad(void)
 {
 	if (get_wifi_power_saving(iface))
 		return TUNE_GOOD;
src/tuning/wifi.h +1 -1
index 50ca68c9e3ae..424fdf610f94 100644
@@ -36,7 +36,7 @@ class wifi_tunable : public tunable {
 public:
 	wifi_tunable(const char *_iface);
 
-	virtual int good_bad(void);
+	virtual tunable_state good_bad(void);
 
 	virtual void toggle(void);
 
Signature · 1 line
2.13.5
Acked-by on v1 3/4
#5 lore raw

Re: [Powertop] [PATCH 3/4 v1] tuning: use tunable_state enum instead of defines

Quoted 1 line · Joe Konno

From: Joe Konno <joe.konno(a)intel.com>

I like this one!

Acked-by Magnus Fromreide <magfr(a)lysator.liu.se>
Quoted 1 line
Signed-off-by Joe Konno <joe.konno(a)intel.com>
author
#6 lore raw

PATCH 4/4 tuning: complete tunable result switch

In reply to Joe Konno

From: Joe Konno <joe.konno(a)intel.com>

After implementing the tunable_state enum, compiler warning showed switch statement within tunable result code was incomplete. Put another way, not all possible enum values were checked for. Fix the code so the compiler warning goes away.

Signed-off-by Joe Konno <joe.konno(a)intel.com>
---
 src/tuning/tunable.h | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)
src/tuning/tunable.h +9 -3
index 5ea7cdfb5389..afb789d6f4a7 100644
@@ -59,14 +59,20 @@ public:
 
 	virtual char *result_string(void)
 	{
+		char *result = NULL;
+
 		switch (good_bad()) {
 		case TUNE_GOOD:
-			return good_string;
+			result = good_string;
 		case TUNE_BAD:
 		case TUNE_UNFIXABLE:
-			return bad_string;
+			result = bad_string;
+		case TUNE_NEUTRAL:
+		case TUNE_UNKNOWN:
+			result = neutral_string;
 		}
-		return neutral_string;
+
+		return result;
 	}
 
 
Signature · 1 line
2.13.5
#7 lore raw

Re: [Powertop] [PATCH 4/4 v1] tuning: complete tunable result switch

Quoted 6 lines · Joe Konno

From: Joe Konno <joe.konno(a)intel.com>

After implementing the tunable_state enum, compiler warning showed switch statement within tunable result code was incomplete. Put another way, not all possible enum values were checked for. Fix the code so the compiler warning goes away.

Why do you change it from multiple return statements? Wouldn't

              switch (good_bad()) {
              case TUNE_GOOD:
                      return good_string;
              case TUNE_BAD:
              case TUNE_UNFIXABLE:
                      return bad_string;
+             case TUNE_NEUTRAL:
+             case TUNE_UNKNOWN:
+                     break;
              }
              return neutral_string;

make for an eqiuivalent but smaller patch?

Quoted 40 lines
Signed-off-by Joe Konno <joe.konno(a)intel.com>
---
 src/tuning/tunable.h | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)
src/tuning/tunable.h +9 -3
index 5ea7cdfb5389..afb789d6f4a7 100644
@@ -59,14 +59,20 @@ public:
 
 	virtual char *result_string(void)
 	{
+		char *result = NULL;
+
 		switch (good_bad()) {
 		case TUNE_GOOD:
-			return good_string;
+			result = good_string;
 		case TUNE_BAD:
 		case TUNE_UNFIXABLE:
-			return bad_string;
+			result = bad_string;
+		case TUNE_NEUTRAL:
+		case TUNE_UNKNOWN:
+			result = neutral_string;
 		}
-		return neutral_string;
+
+		return result;
 	}
 
 
Signature · 6 lines
2.13.5

_______________________________________________
PowerTop mailing list
PowerTop(a)lists.01.org
https://lists.01.org/mailman/listinfo/powertop
author
#8 lore raw
In reply to Joe Konno

Feedback appreciated, Magnus. I'll integrate that feedback into a v2 submission.

Quoted 46 lines · On Fri, 8 Sep 2017 15:36:47 -0700 Joe Konno <joe.konno(a)linux.intel.com>…

From: Joe Konno <joe.konno(a)intel.com>

This series could be split in two, one for the libpci stuff in "lib", and another for "tuning" cleanups. The cleanups in "lib" are not dependent on the cleanups in "tuning" and vice-versa. There should be no changes to PowerTOP operations or usage as a result of these patches-- code cleanups only.

I was re-learning libpci a while back and, during my walks along various PCI buses, spotted a trivial refactor for the libpci functionality in "lib". This is to make it (slightly) more tempting to embark on future changes to walk PCI buses with libpci instead of sysfs-- especially in "tuning" cases.

While looking into additional runtime PM tunables, I stumbled upon #defines used for tunable states. Implemented those states in an enum instead. Proved easy as those defines were not used as factors in math operations, so they were acting as enum entries to begin with. After the enum was implemented, gcc warned about unhandled states in tunable::result_string()'s switch statement, so I fixed it.

Joe Konno (4):
lib: refactor pci_access checks
lib: have check_pci_access() scan the entire bus
tuning: use tunable_state enum instead of defines
tuning: complete tunable result switch

 src/lib.cpp                | 14 ++++++++++----
 src/tuning/bluetooth.cpp   |  6 +++---
 src/tuning/bluetooth.h     |  2 +-
 src/tuning/ethernet.cpp    |  4 ++--
 src/tuning/ethernet.h      |  2 +-
 src/tuning/runtime.cpp     |  2 +-
 src/tuning/runtime.h       |  2 +-
 src/tuning/tunable.h       | 26 +++++++++++++++++---------
 src/tuning/tuningi2c.cpp   |  2 +-
 src/tuning/tuningi2c.h     |  2 +-
 src/tuning/tuningsysfs.cpp |  2 +-
 src/tuning/tuningsysfs.h   |  2 +-
 src/tuning/tuningusb.cpp   |  2 +-
 src/tuning/tuningusb.h     |  2 +-
 src/tuning/wifi.cpp        |  2 +-
 src/tuning/wifi.h          |  2 +-
 16 files changed, 44 insertions(+), 30 deletions(-)