diff mbox

pm_rps: Improve file I/O and restore utilities

Message ID 1389374442-20325-1-git-send-email-jeff.mcgee@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

jeff.mcgee@intel.com Jan. 10, 2014, 5:20 p.m. UTC 
From: Jeff McGee <jeff.mcgee@intel.com>

Use unbuffered file I/O to simplify and ensure proper sysfs access.
Bionic C library may not re-read a read-only file unless unbuffered,
which results in failure to monitor changes in gt_cur_freq_mhz.

Adapt do_writeval to assert that no write error occurs when we expect
none.

Expand use of restore_assert macro to ensure that original min/max
frequencies are always in place after a test failure.

Signed-off-by: Jeff McGee <jeff.mcgee@intel.com>
---
 tests/pm_rps.c | 87 +++++++++++++++++++++++++++++++++++++---------------------
 1 file changed, 55 insertions(+), 32 deletions(-)

Comments

Daniel Vetter Jan. 10, 2014, 5:38 p.m. UTC | #1 
On Fri, Jan 10, 2014 at 11:20:42AM -0600, jeff.mcgee@intel.com wrote:
> From: Jeff McGee <jeff.mcgee@intel.com>
> 
> Use unbuffered file I/O to simplify and ensure proper sysfs access.
> Bionic C library may not re-read a read-only file unless unbuffered,
> which results in failure to monitor changes in gt_cur_freq_mhz.
> 
> Adapt do_writeval to assert that no write error occurs when we expect
> none.
> 
> Expand use of restore_assert macro to ensure that original min/max
> frequencies are always in place after a test failure.

The above three changs should be split into three patches, so that in case
something blows up we don't have to dig out the test functionality changes
from the switch to raw io.

Also I think we should replace restore_assert with an igt cleanup handler.
Those are a bit more robust and will also clean up even when the test
process has caught a signal.

See igt_install_exit_handler. there's already plenty of examples all over
the tree.

Thanks, Daniel
jeff.mcgee@intel.com Jan. 10, 2014, 8:50 p.m. UTC | #2 
On Fri, Jan 10, 2014 at 06:38:59PM +0100, Daniel Vetter wrote:
> On Fri, Jan 10, 2014 at 11:20:42AM -0600, jeff.mcgee@intel.com wrote:
> > From: Jeff McGee <jeff.mcgee@intel.com>
> > 
> > Use unbuffered file I/O to simplify and ensure proper sysfs access.
> > Bionic C library may not re-read a read-only file unless unbuffered,
> > which results in failure to monitor changes in gt_cur_freq_mhz.
> > 
> > Adapt do_writeval to assert that no write error occurs when we expect
> > none.
> > 
> > Expand use of restore_assert macro to ensure that original min/max
> > frequencies are always in place after a test failure.
> 
> The above three changs should be split into three patches, so that in case
> something blows up we don't have to dig out the test functionality changes
> from the switch to raw io.
> 
> Also I think we should replace restore_assert with an igt cleanup handler.
> Those are a bit more robust and will also clean up even when the test
> process has caught a signal.
> 
> See igt_install_exit_handler. there's already plenty of examples all over
> the tree.
> 
> Thanks, Daniel
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

I agree. New patches coming. Thanks

Jeff
diff mbox

Patch

diff --git a/tests/pm_rps.c b/tests/pm_rps.c
index e8affdb..5223760 100644
--- a/tests/pm_rps.c
+++ b/tests/pm_rps.c
@@ -22,6 +22,7 @@ 
  *
  * Authors:
  *    Ben Widawsky <ben@bwidawsk.net>
+ *    Jeff McGee <jeff.mcgee@intel.com>
  *
  */
 
@@ -35,14 +36,6 @@ 
 static bool verbose = false;
 static int origmin, origmax;
 
-#define restore_assert(COND) do { \
-	if (!(COND)) { \
-		writeval(stuff[MIN].filp, origmin); \
-		writeval(stuff[MAX].filp, origmax); \
-		igt_assert(0); \
-	} \
-} while (0);
-
 static const char sysfs_base_path[] = "/sys/class/drm/card%d/gt_%s_freq_mhz";
 enum {
 	CUR,
@@ -66,7 +59,6 @@  static int readval(FILE *filp)
 	int val;
 	int scanned;
 
-	fflush(filp);
 	rewind(filp);
 	scanned = fscanf(filp, "%d", &val);
 	igt_assert(scanned == 1);
@@ -74,32 +66,63 @@  static int readval(FILE *filp)
 	return val;
 }
 
+#define fcur (readval(stuff[CUR].filp))
+#define fmin (readval(stuff[MIN].filp))
+#define fmax (readval(stuff[MAX].filp))
+#define frp0 (readval(stuff[RP0].filp))
+#define frp1 (readval(stuff[RP1].filp))
+#define frpn (readval(stuff[RPn].filp))
+
+static void restore(void)
+{
+	rewind(stuff[MIN].filp);
+	rewind(stuff[MAX].filp);
+
+	if (origmin > fmax) {
+		fprintf(stuff[MAX].filp, "%d", origmax);
+		fprintf(stuff[MIN].filp, "%d", origmin);
+	} else {
+		fprintf(stuff[MIN].filp, "%d", origmin);
+		fprintf(stuff[MAX].filp, "%d", origmax);
+	}
+}
+
+#define restore_assert(COND) do { \
+	if (!(COND)) { \
+		restore(); \
+		igt_assert(0); \
+	} \
+} while (0);
+
 static int do_writeval(FILE *filp, int val, int lerrno)
 {
-	/* Must write twice to sysfs since the first one simply calculates the size and won't return the error */
 	int ret;
+
 	rewind(filp);
 	ret = fprintf(filp, "%d", val);
-	rewind(filp);
-	ret = fprintf(filp, "%d", val);
-	if (ret && lerrno)
-		igt_assert(errno = lerrno);
-	fflush(filp);
+	if (lerrno) {
+		/* Expecting specific error */
+		restore_assert(ret == EOF && errno == lerrno);
+	} else {
+		/* Expecting no error */
+		restore_assert(ret != EOF);
+	}
+
 	return ret;
 }
-#define writeval(filp, val) do_writeval(filp, val, 0)
 
-#define fcur (readval(stuff[CUR].filp))
-#define fmin (readval(stuff[MIN].filp))
-#define fmax (readval(stuff[MAX].filp))
-#define frp0 (readval(stuff[RP0].filp))
-#define frp1 (readval(stuff[RP1].filp))
-#define frpn (readval(stuff[RPn].filp))
+#define writeval(filp, val) do_writeval(filp, val, 0)
+#define writeval_inval(filp, val) do_writeval(filp, val, EINVAL)
 
 static void setfreq(int val)
 {
-	writeval(stuff[MIN].filp, val);
-	writeval(stuff[MAX].filp, val);
+	if (val > fmax) {
+		writeval(stuff[MAX].filp, val);
+		writeval(stuff[MIN].filp, val);
+	} else {
+		writeval(stuff[MIN].filp, val);
+		writeval(stuff[MAX].filp, val);
+	}
 }
 
 static void checkit(void)
@@ -146,6 +169,7 @@  igt_simple_main
 		igt_assert(ret != -1);
 		junk->filp = fopen(path, junk->mode);
 		igt_require(junk->filp);
+		setbuf(junk->filp, NULL);
 
 		val = readval(junk->filp);
 		igt_assert(val >= 0);
@@ -173,17 +197,16 @@  igt_simple_main
 	checkit();
 
 	/* And some errors */
-	writeval(stuff[MIN].filp, frpn - 1);
-	writeval(stuff[MAX].filp, frp0 + 1000);
+	writeval_inval(stuff[MIN].filp, frpn - 1);
+	writeval_inval(stuff[MAX].filp, frp0 + 1000);
 	checkit();
 
-	writeval(stuff[MIN].filp, fmax + 1000);
-	writeval(stuff[MAX].filp, fmin - 1);
+	writeval_inval(stuff[MIN].filp, fmax + 1000);
+	writeval_inval(stuff[MAX].filp, fmin - 1);
 	checkit();
 
-	do_writeval(stuff[MIN].filp, 0x11111110, EINVAL);
-	do_writeval(stuff[MAX].filp, 0, EINVAL);
+	writeval_inval(stuff[MIN].filp, 0x11111110);
+	writeval_inval(stuff[MAX].filp, 0);
 
-	writeval(stuff[MIN].filp, origmin);
-	writeval(stuff[MAX].filp, origmax);
+	restore();
 }