[PATCH 1/2] powerpc/msi: Fix the msi bitmap alignment tests

Subsystems: linux for powerpc (32-bit and 64-bit), the rest

STALE4389d

3 messages, 2 authors, 2014-10-10 · open the first message on its own page

[PATCH 1/2] powerpc/msi: Fix the msi bitmap alignment tests

From: Michael Ellerman <mpe@ellerman.id.au>
Date: 2014-10-10 08:04:34

When we added the alignment tests recently we failed to check they were
actually passing - oops.

They weren't passing, because the bitmap was full. We should also be a
bit more careful when checking the return code, a negative error return
could by divisible by our alignment value.

Fixes: b0345bbc6d09 ("powerpc/msi: Improve IRQ bitmap allocator")
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
 arch/powerpc/sysdev/msi_bitmap.c | 26 ++++++++++++++++++--------
 1 file changed, 18 insertions(+), 8 deletions(-)

diff --git a/arch/powerpc/sysdev/msi_bitmap.c b/arch/powerpc/sysdev/msi_bitmap.c
index 0c75214b6f92..8155d93dee1d 100644
--- a/arch/powerpc/sysdev/msi_bitmap.c
+++ b/arch/powerpc/sysdev/msi_bitmap.c

@@ -151,7 +151,7 @@ void msi_bitmap_free(struct msi_bitmap *bmp)
 static void __init test_basics(void)
 {
 	struct msi_bitmap bmp;
-	int i, size = 512;
+	int rc, i, size = 512;
 
 	/* Can't allocate a bitmap of 0 irqs */
 	check(msi_bitmap_alloc(&bmp, 0, NULL) != 0);

@@ -185,14 +185,24 @@ static void __init test_basics(void)
 	msi_bitmap_free_hwirqs(&bmp, size / 2, 1);
 	check(msi_bitmap_alloc_hwirqs(&bmp, 1) == size / 2);
 
+	/* Free most of them for the alignment tests */
+	msi_bitmap_free_hwirqs(&bmp, 3, size - 3);
+
 	/* Check we get a naturally aligned offset */
-	check(msi_bitmap_alloc_hwirqs(&bmp, 2) % 2 == 0);
-	check(msi_bitmap_alloc_hwirqs(&bmp, 4) % 4 == 0);
-	check(msi_bitmap_alloc_hwirqs(&bmp, 8) % 8 == 0);
-	check(msi_bitmap_alloc_hwirqs(&bmp, 9) % 16 == 0);
-	check(msi_bitmap_alloc_hwirqs(&bmp, 3) % 4 == 0);
-	check(msi_bitmap_alloc_hwirqs(&bmp, 7) % 8 == 0);
-	check(msi_bitmap_alloc_hwirqs(&bmp, 121) % 128 == 0);
+	rc = msi_bitmap_alloc_hwirqs(&bmp, 2);
+	check(rc >= 0 && rc % 2 == 0);
+	rc = msi_bitmap_alloc_hwirqs(&bmp, 4);
+	check(rc >= 0 && rc % 4 == 0);
+	rc = msi_bitmap_alloc_hwirqs(&bmp, 8);
+	check(rc >= 0 && rc % 8 == 0);
+	rc = msi_bitmap_alloc_hwirqs(&bmp, 9);
+	check(rc >= 0 && rc % 16 == 0);
+	rc = msi_bitmap_alloc_hwirqs(&bmp, 3);
+	check(rc >= 0 && rc % 4 == 0);
+	rc = msi_bitmap_alloc_hwirqs(&bmp, 7);
+	check(rc >= 0 && rc % 8 == 0);
+	rc = msi_bitmap_alloc_hwirqs(&bmp, 121);
+	check(rc >= 0 && rc % 128 == 0);
 
 	msi_bitmap_free(&bmp);

-- 
1.9.1

[PATCH 2/2] powerpc/msi: Use WARN_ON() in msi bitmap selftests

From: Michael Ellerman <mpe@ellerman.id.au>
Date: 2014-10-10 08:04:38

As demonstrated in the previous commit, the failure message from the msi
bitmap selftests is a bit subtle, it's easy to miss a failure in a busy
boot log.

So drop our check() macro and use WARN_ON() instead. This necessitates
inverting all the conditions as well.

Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
 arch/powerpc/sysdev/msi_bitmap.c | 54 ++++++++++++++++++----------------------
 1 file changed, 24 insertions(+), 30 deletions(-)

diff --git a/arch/powerpc/sysdev/msi_bitmap.c b/arch/powerpc/sysdev/msi_bitmap.c
index 8155d93dee1d..73b64c73505b 100644
--- a/arch/powerpc/sysdev/msi_bitmap.c
+++ b/arch/powerpc/sysdev/msi_bitmap.c

@@ -145,69 +145,64 @@ void msi_bitmap_free(struct msi_bitmap *bmp)
 
 #ifdef CONFIG_MSI_BITMAP_SELFTEST
 
-#define check(x)	\
-	if (!(x)) printk("msi_bitmap: test failed at line %d\n", __LINE__);
-
 static void __init test_basics(void)
 {
 	struct msi_bitmap bmp;
 	int rc, i, size = 512;
 
 	/* Can't allocate a bitmap of 0 irqs */
-	check(msi_bitmap_alloc(&bmp, 0, NULL) != 0);
+	WARN_ON(msi_bitmap_alloc(&bmp, 0, NULL) == 0);
 
 	/* of_node may be NULL */
-	check(0 == msi_bitmap_alloc(&bmp, size, NULL));
+	WARN_ON(msi_bitmap_alloc(&bmp, size, NULL));
 
 	/* Should all be free by default */
-	check(0 == bitmap_find_free_region(bmp.bitmap, size,
-					   get_count_order(size)));
+	WARN_ON(bitmap_find_free_region(bmp.bitmap, size, get_count_order(size)));
 	bitmap_release_region(bmp.bitmap, 0, get_count_order(size));
 
 	/* With no node, there's no msi-available-ranges, so expect > 0 */
-	check(msi_bitmap_reserve_dt_hwirqs(&bmp) > 0);
+	WARN_ON(msi_bitmap_reserve_dt_hwirqs(&bmp) <= 0);
 
 	/* Should all still be free */
-	check(0 == bitmap_find_free_region(bmp.bitmap, size,
-					   get_count_order(size)));
+	WARN_ON(bitmap_find_free_region(bmp.bitmap, size, get_count_order(size)));
 	bitmap_release_region(bmp.bitmap, 0, get_count_order(size));
 
 	/* Check we can fill it up and then no more */
 	for (i = 0; i < size; i++)
-		check(msi_bitmap_alloc_hwirqs(&bmp, 1) >= 0);
+		WARN_ON(msi_bitmap_alloc_hwirqs(&bmp, 1) < 0);
 
-	check(msi_bitmap_alloc_hwirqs(&bmp, 1) < 0);
+	WARN_ON(msi_bitmap_alloc_hwirqs(&bmp, 1) >= 0);
 
 	/* Should all be allocated */
-	check(bitmap_find_free_region(bmp.bitmap, size, 0) < 0);
+	WARN_ON(bitmap_find_free_region(bmp.bitmap, size, 0) >= 0);
 
 	/* And if we free one we can then allocate another */
 	msi_bitmap_free_hwirqs(&bmp, size / 2, 1);
-	check(msi_bitmap_alloc_hwirqs(&bmp, 1) == size / 2);
+	WARN_ON(msi_bitmap_alloc_hwirqs(&bmp, 1) != size / 2);
 
 	/* Free most of them for the alignment tests */
 	msi_bitmap_free_hwirqs(&bmp, 3, size - 3);
 
 	/* Check we get a naturally aligned offset */
 	rc = msi_bitmap_alloc_hwirqs(&bmp, 2);
-	check(rc >= 0 && rc % 2 == 0);
+	WARN_ON(rc < 0 && rc % 2 != 0);
 	rc = msi_bitmap_alloc_hwirqs(&bmp, 4);
-	check(rc >= 0 && rc % 4 == 0);
+	WARN_ON(rc < 0 && rc % 4 != 0);
 	rc = msi_bitmap_alloc_hwirqs(&bmp, 8);
-	check(rc >= 0 && rc % 8 == 0);
+	WARN_ON(rc < 0 && rc % 8 != 0);
 	rc = msi_bitmap_alloc_hwirqs(&bmp, 9);
-	check(rc >= 0 && rc % 16 == 0);
+	WARN_ON(rc < 0 && rc % 16 != 0);
 	rc = msi_bitmap_alloc_hwirqs(&bmp, 3);
-	check(rc >= 0 && rc % 4 == 0);
+	WARN_ON(rc < 0 && rc % 4 != 0);
 	rc = msi_bitmap_alloc_hwirqs(&bmp, 7);
-	check(rc >= 0 && rc % 8 == 0);
+	WARN_ON(rc < 0 && rc % 8 != 0);
 	rc = msi_bitmap_alloc_hwirqs(&bmp, 121);
-	check(rc >= 0 && rc % 128 == 0);
+	WARN_ON(rc < 0 && rc % 128 != 0);
 
 	msi_bitmap_free(&bmp);
 
-	/* Clients may check bitmap == NULL for "not-allocated" */
-	check(bmp.bitmap == NULL);
+	/* Clients may WARN_ON bitmap == NULL for "not-allocated" */
+	WARN_ON(bmp.bitmap != NULL);
 
 	kfree(bmp.bitmap);
 }

@@ -229,14 +224,13 @@ static void __init test_of_node(void)
 	of_node_init(&of_node);
 	of_node.full_name = node_name;
 
-	check(0 == msi_bitmap_alloc(&bmp, size, &of_node));
+	WARN_ON(msi_bitmap_alloc(&bmp, size, &of_node));
 
 	/* No msi-available-ranges, so expect > 0 */
-	check(msi_bitmap_reserve_dt_hwirqs(&bmp) > 0);
+	WARN_ON(msi_bitmap_reserve_dt_hwirqs(&bmp) <= 0);
 
 	/* Should all still be free */
-	check(0 == bitmap_find_free_region(bmp.bitmap, size,
-					   get_count_order(size)));
+	WARN_ON(bitmap_find_free_region(bmp.bitmap, size, get_count_order(size)));
 	bitmap_release_region(bmp.bitmap, 0, get_count_order(size));
 
 	/* Now create a fake msi-available-ranges property */

@@ -250,11 +244,11 @@ static void __init test_of_node(void)
 	of_node.properties = &prop;
 
 	/* msi-available-ranges, so expect == 0 */
-	check(msi_bitmap_reserve_dt_hwirqs(&bmp) == 0);
+	WARN_ON(msi_bitmap_reserve_dt_hwirqs(&bmp));
 
 	/* Check we got the expected result */
-	check(0 == bitmap_parselist(expected_str, expected, size));
-	check(bitmap_equal(expected, bmp.bitmap, size));
+	WARN_ON(bitmap_parselist(expected_str, expected, size));
+	WARN_ON(!bitmap_equal(expected, bmp.bitmap, size));
 
 	msi_bitmap_free(&bmp);
 	kfree(bmp.bitmap);

-- 
1.9.1

Re: [PATCH 2/2] powerpc/msi: Use WARN_ON() in msi bitmap selftests

From: Laurentiu Tudor <hidden>
Date: 2014-10-10 08:28:47

Hi Michael,

Comment inline.

On 10/10/2014 11:04 AM, Michael Ellerman wrote:

As demonstrated in the previous commit, the failure message from the msi
bitmap selftests is a bit subtle, it's easy to miss a failure in a busy
boot log.

So drop our check() macro and use WARN_ON() instead. This necessitates
inverting all the conditions as well.

Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
 arch/powerpc/sysdev/msi_bitmap.c | 54 ++++++++++++++++++----------------------

[snip]

quoted hunk

 
 	/* Free most of them for the alignment tests */
 	msi_bitmap_free_hwirqs(&bmp, 3, size - 3);
 
 	/* Check we get a naturally aligned offset */
 	rc = msi_bitmap_alloc_hwirqs(&bmp, 2);
-	check(rc >= 0 && rc % 2 == 0);
+	WARN_ON(rc < 0 && rc % 2 != 0);

Here and below, shouldn't these be:

	WARN_ON(rc < 0 || rc % 2 != 0);

?

quoted hunk

 	rc = msi_bitmap_alloc_hwirqs(&bmp, 4);
-	check(rc >= 0 && rc % 4 == 0);
+	WARN_ON(rc < 0 && rc % 4 != 0);
 	rc = msi_bitmap_alloc_hwirqs(&bmp, 8);
-	check(rc >= 0 && rc % 8 == 0);
+	WARN_ON(rc < 0 && rc % 8 != 0);
 	rc = msi_bitmap_alloc_hwirqs(&bmp, 9);
-	check(rc >= 0 && rc % 16 == 0);
+	WARN_ON(rc < 0 && rc % 16 != 0);
 	rc = msi_bitmap_alloc_hwirqs(&bmp, 3);
-	check(rc >= 0 && rc % 4 == 0);
+	WARN_ON(rc < 0 && rc % 4 != 0);
 	rc = msi_bitmap_alloc_hwirqs(&bmp, 7);
-	check(rc >= 0 && rc % 8 == 0);
+	WARN_ON(rc < 0 && rc % 8 != 0);
 	rc = msi_bitmap_alloc_hwirqs(&bmp, 121);
-	check(rc >= 0 && rc % 128 == 0);
+	WARN_ON(rc < 0 && rc % 128 != 0);
 
 	msi_bitmap_free(&bmp);
 
-	/* Clients may check bitmap == NULL for "not-allocated" */
-	check(bmp.bitmap == NULL);
+	/* Clients may WARN_ON bitmap == NULL for "not-allocated" */
+	WARN_ON(bmp.bitmap != NULL);
 
 	kfree(bmp.bitmap);
 }

@@ -229,14 +224,13 @@ static void __init test_of_node(void)
 	of_node_init(&of_node);
 	of_node.full_name = node_name;
 
-	check(0 == msi_bitmap_alloc(&bmp, size, &of_node));
+	WARN_ON(msi_bitmap_alloc(&bmp, size, &of_node));
 
 	/* No msi-available-ranges, so expect > 0 */
-	check(msi_bitmap_reserve_dt_hwirqs(&bmp) > 0);
+	WARN_ON(msi_bitmap_reserve_dt_hwirqs(&bmp) <= 0);
 
 	/* Should all still be free */
-	check(0 == bitmap_find_free_region(bmp.bitmap, size,
-					   get_count_order(size)));
+	WARN_ON(bitmap_find_free_region(bmp.bitmap, size, get_count_order(size)));
 	bitmap_release_region(bmp.bitmap, 0, get_count_order(size));
 
 	/* Now create a fake msi-available-ranges property */
@@ -250,11 +244,11 @@ static void __init test_of_node(void)
 	of_node.properties = &prop;
 
 	/* msi-available-ranges, so expect == 0 */
-	check(msi_bitmap_reserve_dt_hwirqs(&bmp) == 0);
+	WARN_ON(msi_bitmap_reserve_dt_hwirqs(&bmp));
 
 	/* Check we got the expected result */
-	check(0 == bitmap_parselist(expected_str, expected, size));
-	check(bitmap_equal(expected, bmp.bitmap, size));
+	WARN_ON(bitmap_parselist(expected_str, expected, size));
+	WARN_ON(!bitmap_equal(expected, bmp.bitmap, size));
 
 	msi_bitmap_free(&bmp);
 	kfree(bmp.bitmap);

---
Best Regards, Laurentiu

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help