Skip to contentSkip to conversation

dt: unittest: fix breakage and warnings

PATCH In review Frank Rowand 2 participants 8 messages Mar 14, 2015 – Mar 28, 2015 Device tree

lore
Share & tools
lore

Frank Rowand

author
#0 lore raw

cover dt: unittest: fix breakage and warnings

Running checkpatch on early versions of my patchset to fix the devicetree make dependency issues exposed a large number of warnings, including some that are actual bugs. http://lkml.iu.edu/hypermail/linux/kernel/1503.1/03335.html

These patches fix those bugs, and another bug exposed by fixing those bugs. As a result, the number of tests completed increased from 102 to 107.

Since I was already poking around, I fixed a few classes of checkpatch warnings.

There are still plenty of warnings, but the noise is greatly reduced and future checkpatch warnings that are problems should stand out more clearly.

author
#1 lore raw

PATCH 1/5 dt: unittest: early return from test skips tests

From: Frank Rowand <[email protected]>

Fix bugs pointed out by checkpatch.

Mis-coding of two if statements caused early return from function.

Number of tests completed increased from 102 to 107. Number of tests failed increased from 0 to 2.

Signed-off-by Frank Rowand <[email protected]>
---
 drivers/of/unittest.c |   10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)
drivers/of/unittest.c +6 -4
Index: b/drivers/of/unittest.c
===================================================================
@@ -762,12 +762,14 @@ static void __init of_selftest_platform_
 	irq = platform_get_irq(pdev, 0);
 	selftest(irq < 0 && irq != -EPROBE_DEFER, "device parsing error failed - %d\n", irq);
 
-	if (selftest(np = of_find_node_by_path("/testcase-data/platform-tests"),
-		     "No testcase data in device tree\n"));
+	np = of_find_node_by_path("/testcase-data/platform-tests");
+	selftest(np, "No testcase data in device tree\n");
+	if (!np)
 		return;
 
-	if (selftest(!(rc = device_register(&test_bus)),
-		     "testbus registration failed; rc=%i\n", rc));
+	rc = device_register(&test_bus);
+	selftest(!rc, "testbus registration failed; rc=%i\n", rc);
+	if (rc)
 		return;
 
 	for_each_child_of_node(np, child) {
author
#3 lore raw

PATCH 2/5 dt: unittest: typo in error string

In reply to Frank Rowand

From: Frank Rowand <frank.rowand-/[email protected]>

Fix bug pointed out by checkpatch.

Splitting string incorrectly removed a space between two words.

Signed-off-by Frank Rowand <frank.rowand-/[email protected]>
---
 drivers/of/unittest.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
drivers/of/unittest.c +3 -2
Index: b/drivers/of/unittest.c
===================================================================
@@ -212,8 +212,9 @@ static void __init of_selftest_check_tre
 	child_count = of_selftest_check_node_linkage(of_root);
 
 	selftest(child_count > 0, "Device node data structure is corrupted\n");
-	selftest(child_count == allnode_count, "allnodes list size (%i) doesn't match"
-		 "sibling lists size (%i)\n", allnode_count, child_count);
+	selftest(child_count == allnode_count,
+		 "allnodes list size (%i) doesn't match sibling lists size (%i)\n",
+		 allnode_count, child_count);
 	pr_debug("allnodes list size (%i); sibling lists size (%i)\n", allnode_count, child_count);
 }

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

author
#4 lore raw

PATCH 3/5 dt: unittest: add const where needed

In reply to Frank Rowand

From: Frank Rowand <frank.rowand-/[email protected]>

Fix warnings pointed out by checkpatch.

No bugs fixed, but the test code should be a good example of how to use the devicetree API.

Signed-off-by Frank Rowand <frank.rowand-/[email protected]>
---
 drivers/of/unittest.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
drivers/of/unittest.c +4 -4
Index: b/drivers/of/unittest.c
===================================================================
@@ -666,7 +666,7 @@ static void __init of_selftest_parse_int
 	of_node_put(np);
 }
 
-static struct of_device_id match_node_table[] = {
+static const struct of_device_id match_node_table[] = {
 	{ .data = "A", .name = "name0", }, /* Name alone is lowest priority */
 	{ .data = "B", .type = "type1", }, /* followed by type alone */
 

@@ -740,7 +740,7 @@ static void __init of_selftest_platform_
 	int irq, rc;
 	struct device_node *np, *child, *grandchild;
 	struct platform_device *pdev;
-	struct of_device_id match[] = {
+	const struct of_device_id match[] = {
 		{ .compatible = "test-device", },
 		{}
 	};

@@ -941,7 +941,7 @@ static int selftest_remove(struct platfo
 	return 0;
 }
 
-static struct of_device_id selftest_match[] = {
+static const struct of_device_id selftest_match[] = {
 	{ .compatible = "selftest", },
 	{},
 };

@@ -1533,7 +1533,7 @@ static int selftest_i2c_bus_remove(struc
 	return 0;
 }
 
-static struct of_device_id selftest_i2c_bus_match[] = {
+static const struct of_device_id selftest_i2c_bus_match[] = {
 	{ .compatible = "selftest-i2c-bus", },
 	{},
 };

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

author
#5 lore raw

PATCH 4/5 dt: unittest: reduce checkpatch noise - line after declarations

In reply to Frank Rowand

From: Frank Rowand <[email protected]>

Fix warnings pointed out by checkpatch.

No bug fixes, but reduce the number of checkpatch warnings so that future problems will stand out better.

Signed-off-by Frank Rowand <[email protected]>
---
 drivers/of/unittest.c |    5 +++++
 1 file changed, 5 insertions(+)
drivers/of/unittest.c +5 -0
Index: b/drivers/of/unittest.c
===================================================================
@@ -279,6 +279,7 @@ static void __init of_selftest_parse_pha
 
 	for (i = 0; i < 8; i++) {
 		bool passed = true;
+
 		rc = of_parse_phandle_with_args(np, "phandle-list",
 						"#phandle-cells", i, &args);
 

@@ -539,6 +540,7 @@ static void __init of_selftest_parse_int
 
 	for (i = 0; i < 4; i++) {
 		bool passed = true;
+
 		args.args_count = 0;
 		rc = of_irq_parse_one(np, i, &args);
 

@@ -559,6 +561,7 @@ static void __init of_selftest_parse_int
 
 	for (i = 0; i < 4; i++) {
 		bool passed = true;
+
 		args.args_count = 0;
 		rc = of_irq_parse_one(np, i, &args);
 

@@ -611,6 +614,7 @@ static void __init of_selftest_parse_int
 
 	for (i = 0; i < 7; i++) {
 		bool passed = true;
+
 		rc = of_irq_parse_one(np, i, &args);
 
 		/* Test the values from tests-phandle.dtsi */

@@ -905,6 +909,7 @@ static int __init selftest_data_add(void
 	np = selftest_data_node->child;
 	while (np) {
 		struct device_node *next = np->sibling;
+
 		np->parent = of_root;
 		attach_node_and_children(np);
 		np = next;
author
#6 lore raw

PATCH 5/5 dt: unittest: breadcrumbs to reduce pain of future maintainers

In reply to Frank Rowand

From: Frank Rowand <[email protected]>

Fix warnings pointed out by checkpatch.

Checkpatch warns: externs should be avoided in .c files

Reducing pain for future maintainers - adding a comment so that anyone trying to find where the extern data is created will be able to find it. (grep will not find that location)

Signed-off-by Frank Rowand <[email protected]>
---
 drivers/of/unittest.c |    4 ++++
 1 file changed, 4 insertions(+)
drivers/of/unittest.c +4 -0
Index: b/drivers/of/unittest.c
===================================================================
@@ -865,6 +865,10 @@ static int __init selftest_data_add(void
 {
 	void *selftest_data;
 	struct device_node *selftest_data_node, *np;
+	/*
+	 * __dtb_testcases_begin[] and __dtb_testcases_end[] are magically
+	 * created by cmd_dt_S_dtb in scripts/Makefile.lib
+	 */
 	extern uint8_t __dtb_testcases_begin[];
 	extern uint8_t __dtb_testcases_end[];
 	const int size = __dtb_testcases_end - __dtb_testcases_begin;
#7 lore raw
In reply to Frank Rowand
Quoted 11 lines · Frank Rowand

Running checkpatch on early versions of my patchset to fix the devicetree make dependency issues exposed a large number of warnings, including some that are actual bugs. http://lkml.iu.edu/hypermail/linux/kernel/1503.1/03335.html

These patches fix those bugs, and another bug exposed by fixing those bugs. As a result, the number of tests completed increased from 102 to 107.

Since I was already poking around, I fixed a few classes of checkpatch warnings.

There are still plenty of warnings, but the noise is greatly reduced and future checkpatch warnings that are problems should stand out more clearly.

Thanks for rooting these out. I'll apply the bug fix for v4.0, and the rest for v4.1

g.

#2 lore raw

Re: [Patch 1/5] dt: unittest: early return from test skips tests

In reply to Frank Rowand
Quoted 8 lines · Frank Rowand

From: Frank Rowand <[email protected]>

Fix bugs pointed out by checkpatch.

Mis-coding of two if statements caused early return from function.

Number of tests completed increased from 102 to 107. Number of tests failed increased from 0 to 2.

I was going to send this to Linus right away, but on second thought, it isn't actually a regression fix. I'm going to queue it up for v4.1 with the rest of the series instead.

I've also got a bug fix for the new warnings that I'll put in at the same time.

g.

Quoted 34 lines
Signed-off-by Frank Rowand <[email protected]>
---
 drivers/of/unittest.c |   10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

Index: b/drivers/of/unittest.c
===================================================================

drivers/of/unittest.c +6 -5
@@ -762,12 +762,14 @@ static void __init of_selftest_platform_
 	irq = platform_get_irq(pdev, 0);
 	selftest(irq < 0 && irq != -EPROBE_DEFER, "device parsing error failed - %d\n", irq);
 
-	if (selftest(np = of_find_node_by_path("/testcase-data/platform-tests"),
-		     "No testcase data in device tree\n"));
+	np = of_find_node_by_path("/testcase-data/platform-tests");
+	selftest(np, "No testcase data in device tree\n");
+	if (!np)
 		return;
 
-	if (selftest(!(rc = device_register(&test_bus)),
-		     "testbus registration failed; rc=%i\n", rc));
+	rc = device_register(&test_bus);
+	selftest(!rc, "testbus registration failed; rc=%i\n", rc);
+	if (rc)
 		return;
 
 	for_each_child_of_node(np, child) {
--

To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/