[PATCH 0/5] staging: comedi: tests: Fix various issues

[PATCH 0/5] staging: comedi: tests: Fix various issues

[Ian Abbott]

The code under "drivers/staging/comedi/drivers/tests/" is built when the
CONFIG_COMEDI_TESTS option is enabled.  That currently needs to be done
on the "make" command line since the option does not appear in the
Kconfig files.

This patch series fixes a compilation error and various other niggles
including checkpatch.pl stuff.

1) staging: comedi: tests: ni_routes_test: Fix compilation error
2) staging: comedi: tests: ni_routes_test: Put complex values in parentheses
3) staging: comedi: tests: ni_routes_test: Avoid CamelCase: <RVi>
4) staging: comedi: tests: ni_routes_test: Lines should not end with a '('
5) staging: comedi: tests: Correct unittest_fptr

 .../staging/comedi/drivers/tests/example_test.c    |  2 +-
 .../staging/comedi/drivers/tests/ni_routes_test.c  | 81 +++++++++++-----------
 drivers/staging/comedi/drivers/tests/unittest.h    |  2 +-
 3 files changed, 42 insertions(+), 43 deletions(-)

[PATCH 1/5] staging: comedi: tests: ni_routes_test: Fix compilation error

[Ian Abbott]

The `ni_routes_test` module is not currently selectable using the
Kconfig files, but can be built by specifying `CONFIG_COMEDI_TESTS=m` on
the "make" command line.  It currently fails to compile due to an extra
parameter added to the `ni_assign_device_routes` function by
commit e3b7ce73c578 ("staging: comedi: ni_routes: Allow alternate board
name for routes").  Fix it by supplying the value `NULL` for the added
`alt_board_name` parameter (which specifies that there is no alternate
board name).

Fixes: e3b7ce73c578 ("staging: comedi: ni_routes: Allow alternate board name for routes")
Cc: Spencer E. Olson <olsonse@umich.edu>
Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
---
 drivers/staging/comedi/drivers/tests/ni_routes_test.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/comedi/drivers/tests/ni_routes_test.c b/drivers/staging/comedi/drivers/tests/ni_routes_test.c
index 4061b3b5f8e9..68defeb53de4 100644
--- a/drivers/staging/comedi/drivers/tests/ni_routes_test.c
+++ b/drivers/staging/comedi/drivers/tests/ni_routes_test.c
@@ -217,7 +217,8 @@ void test_ni_assign_device_routes(void)
 	const u8 *table, *oldtable;
 
 	init_pci_6070e();
-	ni_assign_device_routes(ni_eseries, pci_6070e, &private.routing_tables);
+	ni_assign_device_routes(ni_eseries, pci_6070e, NULL,
+				&private.routing_tables);
 	devroutes = private.routing_tables.valid_routes;
 	table = private.routing_tables.route_values;
 
@@ -253,7 +254,8 @@ void test_ni_assign_device_routes(void)
 	olddevroutes = devroutes;
 	oldtable = table;
 	init_pci_6220();
-	ni_assign_device_routes(ni_mseries, pci_6220, &private.routing_tables);
+	ni_assign_device_routes(ni_mseries, pci_6220, NULL,
+				&private.routing_tables);
 	devroutes = private.routing_tables.valid_routes;
 	table = private.routing_tables.route_values;
 
-- 
2.31.0

[PATCH 2/5] staging: comedi: tests: ni_routes_test: Put complex values in parentheses

[Ian Abbott]

Fix checkpatch.pl errors such as:

ERROR: Macros with complex values should be enclosed in parentheses
+#define I1(x1)	\
+	(int[]){ \
+		x1, 0 \
+	}

Also, wrap that `x1` in parentheses.

Cc: Spencer E. Olson <olsonse@umich.edu>
Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
---
 .../comedi/drivers/tests/ni_routes_test.c      | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/staging/comedi/drivers/tests/ni_routes_test.c b/drivers/staging/comedi/drivers/tests/ni_routes_test.c
index 68defeb53de4..696e610f1822 100644
--- a/drivers/staging/comedi/drivers/tests/ni_routes_test.c
+++ b/drivers/staging/comedi/drivers/tests/ni_routes_test.c
@@ -63,24 +63,24 @@ static const int no_val_dest = O(7), no_val_index = 4;
 
 /* I1 and I2 should not call O(...).  Mostly here to shut checkpatch.pl up */
 #define I1(x1)	\
-	(int[]){ \
-		x1, 0 \
-	}
+	((int[]){ \
+		(x1), 0 \
+	 })
 #define I2(x1, x2)	\
-	(int[]){ \
+	((int[]){ \
 		(x1), (x2), 0 \
-	}
+	 })
 #define I3(x1, x2, x3)	\
-	(int[]){ \
+	((int[]){ \
 		(x1), (x2), (x3), 0 \
-	}
+	 })
 
 /* O9 is build to call O(...) for each arg */
 #define O9(x1, x2, x3, x4, x5, x6, x7, x8, x9)	\
-	(int[]){ \
+	((int[]){ \
 		O(x1), O(x2), O(x3), O(x4), O(x5), O(x6), O(x7), O(x8), O(x9), \
 		0 \
-	}
+	 })
 
 static struct ni_device_routes DR = {
 	.device = "testdev",
-- 
2.31.0

[PATCH 3/5] staging: comedi: tests: ni_routes_test: Avoid CamelCase: <RVi>

[Ian Abbott]

Rename the `RVi` macro to `RVI` to avoid this checkpatch.pl issue:

CHECK: #27: FILE: drivers/staging/comedi/drivers/tests/ni_routes_test.c:27:
+#define RVi(table, src, dest)	((table)[(dest) * NI_NUM_NAMES + (src)])

Cc: Spencer E. Olson <olsonse@umich.edu>
Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
---
Note: checkpatch.pl shows an unrelated CamelCase issue for this patch.
---
 .../comedi/drivers/tests/ni_routes_test.c      | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/staging/comedi/drivers/tests/ni_routes_test.c b/drivers/staging/comedi/drivers/tests/ni_routes_test.c
index 696e610f1822..7dfc70a4cc6f 100644
--- a/drivers/staging/comedi/drivers/tests/ni_routes_test.c
+++ b/drivers/staging/comedi/drivers/tests/ni_routes_test.c
@@ -24,7 +24,7 @@
 #include "../ni_routes.h"
 #include "unittest.h"
 
-#define RVi(table, src, dest)	((table)[(dest) * NI_NUM_NAMES + (src)])
+#define RVI(table, src, dest)	((table)[(dest) * NI_NUM_NAMES + (src)])
 #define O(x)	((x) + NI_NAMES_BASE)
 #define B(x)	((x) - NI_NAMES_BASE)
 #define V(x)	((x) | 0x80)
@@ -244,10 +244,10 @@ void test_ni_assign_device_routes(void)
 		 "all pci-6070e route_set->src's in order of signal source\n");
 
 	unittest(
-	  RVi(table, B(PXI_Star), B(NI_AI_SampleClock)) == V(17) &&
-	  RVi(table, B(NI_10MHzRefClock), B(TRIGGER_LINE(0))) == 0 &&
-	  RVi(table, B(NI_AI_ConvertClock), B(NI_PFI(0))) == 0 &&
-	  RVi(table, B(NI_AI_ConvertClock), B(NI_PFI(2))) ==
+	  RVI(table, B(PXI_Star), B(NI_AI_SampleClock)) == V(17) &&
+	  RVI(table, B(NI_10MHzRefClock), B(TRIGGER_LINE(0))) == 0 &&
+	  RVI(table, B(NI_AI_ConvertClock), B(NI_PFI(0))) == 0 &&
+	  RVI(table, B(NI_AI_ConvertClock), B(NI_PFI(2))) ==
 		V(NI_PFI_OUTPUT_AI_CONVERT),
 	  "pci-6070e finds e-series route_values table\n");
 
@@ -264,10 +264,10 @@ void test_ni_assign_device_routes(void)
 	unittest(oldtable != table, "pci-6220 find other route_values table\n");
 
 	unittest(
-	  RVi(table, B(PXI_Star), B(NI_AI_SampleClock)) == V(20) &&
-	  RVi(table, B(NI_10MHzRefClock), B(TRIGGER_LINE(0))) == V(12) &&
-	  RVi(table, B(NI_AI_ConvertClock), B(NI_PFI(0))) == V(3) &&
-	  RVi(table, B(NI_AI_ConvertClock), B(NI_PFI(2))) == V(3),
+	  RVI(table, B(PXI_Star), B(NI_AI_SampleClock)) == V(20) &&
+	  RVI(table, B(NI_10MHzRefClock), B(TRIGGER_LINE(0))) == V(12) &&
+	  RVI(table, B(NI_AI_ConvertClock), B(NI_PFI(0))) == V(3) &&
+	  RVI(table, B(NI_AI_ConvertClock), B(NI_PFI(2))) == V(3),
 	  "pci-6220 finds m-series route_values table\n");
 }
 
-- 
2.31.0

[PATCH 4/5] staging: comedi: tests: ni_routes_test: Lines should not end with a '('

[Ian Abbott]

Fix checkpatch.pl issues such as the following:

CHECK: Lines should not end with a '('
+	unittest(

Cc: Spencer E. Olson <olsonse@umich.edu>
Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
---
 .../comedi/drivers/tests/ni_routes_test.c     | 23 ++++++++-----------
 1 file changed, 10 insertions(+), 13 deletions(-)

diff --git a/drivers/staging/comedi/drivers/tests/ni_routes_test.c b/drivers/staging/comedi/drivers/tests/ni_routes_test.c
index 7dfc70a4cc6f..96e778031da3 100644
--- a/drivers/staging/comedi/drivers/tests/ni_routes_test.c
+++ b/drivers/staging/comedi/drivers/tests/ni_routes_test.c
@@ -243,13 +243,11 @@ void test_ni_assign_device_routes(void)
 	unittest(route_set_sources_in_order(devroutes),
 		 "all pci-6070e route_set->src's in order of signal source\n");
 
-	unittest(
-	  RVI(table, B(PXI_Star), B(NI_AI_SampleClock)) == V(17) &&
-	  RVI(table, B(NI_10MHzRefClock), B(TRIGGER_LINE(0))) == 0 &&
-	  RVI(table, B(NI_AI_ConvertClock), B(NI_PFI(0))) == 0 &&
-	  RVI(table, B(NI_AI_ConvertClock), B(NI_PFI(2))) ==
-		V(NI_PFI_OUTPUT_AI_CONVERT),
-	  "pci-6070e finds e-series route_values table\n");
+	unittest(RVI(table, B(PXI_Star), B(NI_AI_SampleClock)) == V(17) &&
+		 RVI(table, B(NI_10MHzRefClock), B(TRIGGER_LINE(0))) == 0 &&
+		 RVI(table, B(NI_AI_ConvertClock), B(NI_PFI(0))) == 0 &&
+		 RVI(table, B(NI_AI_ConvertClock), B(NI_PFI(2))) == V(NI_PFI_OUTPUT_AI_CONVERT),
+		 "pci-6070e finds e-series route_values table\n");
 
 	olddevroutes = devroutes;
 	oldtable = table;
@@ -263,12 +261,11 @@ void test_ni_assign_device_routes(void)
 		 "find device pci-6220\n");
 	unittest(oldtable != table, "pci-6220 find other route_values table\n");
 
-	unittest(
-	  RVI(table, B(PXI_Star), B(NI_AI_SampleClock)) == V(20) &&
-	  RVI(table, B(NI_10MHzRefClock), B(TRIGGER_LINE(0))) == V(12) &&
-	  RVI(table, B(NI_AI_ConvertClock), B(NI_PFI(0))) == V(3) &&
-	  RVI(table, B(NI_AI_ConvertClock), B(NI_PFI(2))) == V(3),
-	  "pci-6220 finds m-series route_values table\n");
+	unittest(RVI(table, B(PXI_Star), B(NI_AI_SampleClock)) == V(20) &&
+		 RVI(table, B(NI_10MHzRefClock), B(TRIGGER_LINE(0))) == V(12) &&
+		 RVI(table, B(NI_AI_ConvertClock), B(NI_PFI(0))) == V(3) &&
+		 RVI(table, B(NI_AI_ConvertClock), B(NI_PFI(2))) == V(3),
+		 "pci-6220 finds m-series route_values table\n");
 }
 
 void test_ni_sort_device_routes(void)
-- 
2.31.0

[PATCH 5/5] staging: comedi: tests: Correct unittest_fptr

[Ian Abbott]

The definition of the `unittest_fptr` function pointer type has the
wrong function return type `void *` instead of `void`.  The problem is
hidden by a bunch of unnecessary type-casts.  Fix the type definition
and remove the type-casts.

Cc: Spencer E. Olson <olsonse@umich.edu>
Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
---
 .../comedi/drivers/tests/example_test.c       |  2 +-
 .../comedi/drivers/tests/ni_routes_test.c     | 32 +++++++++----------
 .../staging/comedi/drivers/tests/unittest.h   |  2 +-
 3 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/drivers/staging/comedi/drivers/tests/example_test.c b/drivers/staging/comedi/drivers/tests/example_test.c
index fc65158b8e8e..4d1ab130339d 100644
--- a/drivers/staging/comedi/drivers/tests/example_test.c
+++ b/drivers/staging/comedi/drivers/tests/example_test.c
@@ -53,7 +53,7 @@ void test0(void)
 static int __init unittest_enter(void)
 {
 	const unittest_fptr unit_tests[] = {
-		(unittest_fptr)test0,
+		test0,
 		NULL,
 	};
 
diff --git a/drivers/staging/comedi/drivers/tests/ni_routes_test.c b/drivers/staging/comedi/drivers/tests/ni_routes_test.c
index 96e778031da3..777d9b5d96d4 100644
--- a/drivers/staging/comedi/drivers/tests/ni_routes_test.c
+++ b/drivers/staging/comedi/drivers/tests/ni_routes_test.c
@@ -578,22 +578,22 @@ void test_ni_get_reg_value(void)
 static int __init ni_routes_unittest(void)
 {
 	const unittest_fptr unit_tests[] = {
-		(unittest_fptr)test_ni_assign_device_routes,
-		(unittest_fptr)test_ni_sort_device_routes,
-		(unittest_fptr)test_ni_find_route_set,
-		(unittest_fptr)test_ni_route_set_has_source,
-		(unittest_fptr)test_ni_route_to_register,
-		(unittest_fptr)test_ni_lookup_route_register,
-		(unittest_fptr)test_route_is_valid,
-		(unittest_fptr)test_ni_is_cmd_dest,
-		(unittest_fptr)test_channel_is_pfi,
-		(unittest_fptr)test_channel_is_rtsi,
-		(unittest_fptr)test_ni_count_valid_routes,
-		(unittest_fptr)test_ni_get_valid_routes,
-		(unittest_fptr)test_ni_find_route_source,
-		(unittest_fptr)test_route_register_is_valid,
-		(unittest_fptr)test_ni_check_trigger_arg,
-		(unittest_fptr)test_ni_get_reg_value,
+		test_ni_assign_device_routes,
+		test_ni_sort_device_routes,
+		test_ni_find_route_set,
+		test_ni_route_set_has_source,
+		test_ni_route_to_register,
+		test_ni_lookup_route_register,
+		test_route_is_valid,
+		test_ni_is_cmd_dest,
+		test_channel_is_pfi,
+		test_channel_is_rtsi,
+		test_ni_count_valid_routes,
+		test_ni_get_valid_routes,
+		test_ni_find_route_source,
+		test_route_register_is_valid,
+		test_ni_check_trigger_arg,
+		test_ni_get_reg_value,
 		NULL,
 	};
 
diff --git a/drivers/staging/comedi/drivers/tests/unittest.h b/drivers/staging/comedi/drivers/tests/unittest.h
index b8e622ea1de1..2da3beea2479 100644
--- a/drivers/staging/comedi/drivers/tests/unittest.h
+++ b/drivers/staging/comedi/drivers/tests/unittest.h
@@ -27,7 +27,7 @@ static struct unittest_results {
 	int failed;
 } unittest_results;
 
-typedef void *(*unittest_fptr)(void);
+typedef void (*unittest_fptr)(void);
 
 #define unittest(result, fmt, ...) ({ \
 	bool failed = !(result); \
-- 
2.31.0

Re: [PATCH 0/5] staging: comedi: tests: Fix various issues

[Greg Kroah-Hartman]

On Wed, Apr 07, 2021 at 03:01:37PM +0100, Ian Abbott wrote:
> The code under "drivers/staging/comedi/drivers/tests/" is built when the
> CONFIG_COMEDI_TESTS option is enabled.  That currently needs to be done
> on the "make" command line since the option does not appear in the
> Kconfig files.
> 
> This patch series fixes a compilation error and various other niggles
> including checkpatch.pl stuff.
> 
> 1) staging: comedi: tests: ni_routes_test: Fix compilation error
> 2) staging: comedi: tests: ni_routes_test: Put complex values in parentheses
> 3) staging: comedi: tests: ni_routes_test: Avoid CamelCase: <RVi>
> 4) staging: comedi: tests: ni_routes_test: Lines should not end with a '('
> 5) staging: comedi: tests: Correct unittest_fptr

Should we move these to the normal kernel testing directory instead, so
that they do get some build testing?

thanks,

greg k-h

Re: [PATCH 3/5] staging: comedi: tests: ni_routes_test: Avoid CamelCase: <RVi>

[Spencer Olson]

On Wed, Apr 7, 2021 at 8:11 AM Ian Abbott <abbotti@mev.co.uk> wrote:
>
> Rename the `RVi` macro to `RVI` to avoid this checkpatch.pl issue:
>
> CHECK: #27: FILE: drivers/staging/comedi/drivers/tests/ni_routes_test.c:27:
> +#define RVi(table, src, dest)  ((table)[(dest) * NI_NUM_NAMES + (src)])
>
> Cc: Spencer E. Olson <olsonse@umich.edu>
> Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
> ---
> Note: checkpatch.pl shows an unrelated CamelCase issue for this patch.
> ---
>  .../comedi/drivers/tests/ni_routes_test.c      | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/staging/comedi/drivers/tests/ni_routes_test.c b/drivers/staging/comedi/drivers/tests/ni_routes_test.c
> index 696e610f1822..7dfc70a4cc6f 100644
> --- a/drivers/staging/comedi/drivers/tests/ni_routes_test.c
> +++ b/drivers/staging/comedi/drivers/tests/ni_routes_test.c
> @@ -24,7 +24,7 @@
>  #include "../ni_routes.h"
>  #include "unittest.h"
>
> -#define RVi(table, src, dest)  ((table)[(dest) * NI_NUM_NAMES + (src)])
> +#define RVI(table, src, dest)  ((table)[(dest) * NI_NUM_NAMES + (src)])
>  #define O(x)   ((x) + NI_NAMES_BASE)
>  #define B(x)   ((x) - NI_NAMES_BASE)
>  #define V(x)   ((x) | 0x80)
> @@ -244,10 +244,10 @@ void test_ni_assign_device_routes(void)
>                  "all pci-6070e route_set->src's in order of signal source\n");
>
>         unittest(
> -         RVi(table, B(PXI_Star), B(NI_AI_SampleClock)) == V(17) &&
> -         RVi(table, B(NI_10MHzRefClock), B(TRIGGER_LINE(0))) == 0 &&
> -         RVi(table, B(NI_AI_ConvertClock), B(NI_PFI(0))) == 0 &&
> -         RVi(table, B(NI_AI_ConvertClock), B(NI_PFI(2))) ==
> +         RVI(table, B(PXI_Star), B(NI_AI_SampleClock)) == V(17) &&
> +         RVI(table, B(NI_10MHzRefClock), B(TRIGGER_LINE(0))) == 0 &&
> +         RVI(table, B(NI_AI_ConvertClock), B(NI_PFI(0))) == 0 &&
> +         RVI(table, B(NI_AI_ConvertClock), B(NI_PFI(2))) ==
>                 V(NI_PFI_OUTPUT_AI_CONVERT),
>           "pci-6070e finds e-series route_values table\n");
>
> @@ -264,10 +264,10 @@ void test_ni_assign_device_routes(void)
>         unittest(oldtable != table, "pci-6220 find other route_values table\n");
>
>         unittest(
> -         RVi(table, B(PXI_Star), B(NI_AI_SampleClock)) == V(20) &&
> -         RVi(table, B(NI_10MHzRefClock), B(TRIGGER_LINE(0))) == V(12) &&
> -         RVi(table, B(NI_AI_ConvertClock), B(NI_PFI(0))) == V(3) &&
> -         RVi(table, B(NI_AI_ConvertClock), B(NI_PFI(2))) == V(3),
> +         RVI(table, B(PXI_Star), B(NI_AI_SampleClock)) == V(20) &&
> +         RVI(table, B(NI_10MHzRefClock), B(TRIGGER_LINE(0))) == V(12) &&
> +         RVI(table, B(NI_AI_ConvertClock), B(NI_PFI(0))) == V(3) &&
> +         RVI(table, B(NI_AI_ConvertClock), B(NI_PFI(2))) == V(3),
>           "pci-6220 finds m-series route_values table\n");
>  }
>
> --
> 2.31.0
>

Had not really been meant to be CamelCase (like all the other
exceptions that were meant to mirror documentation from NI).

Spencer

Re: [PATCH 0/5] staging: comedi: tests: Fix various issues

[Ian Abbott]

On 07/04/2021 16:10, Greg Kroah-Hartman wrote:
> On Wed, Apr 07, 2021 at 03:01:37PM +0100, Ian Abbott wrote:
>> The code under "drivers/staging/comedi/drivers/tests/" is built when the
>> CONFIG_COMEDI_TESTS option is enabled.  That currently needs to be done
>> on the "make" command line since the option does not appear in the
>> Kconfig files.
>>
>> This patch series fixes a compilation error and various other niggles
>> including checkpatch.pl stuff.
>>
>> 1) staging: comedi: tests: ni_routes_test: Fix compilation error
>> 2) staging: comedi: tests: ni_routes_test: Put complex values in parentheses
>> 3) staging: comedi: tests: ni_routes_test: Avoid CamelCase: <RVi>
>> 4) staging: comedi: tests: ni_routes_test: Lines should not end with a '('
>> 5) staging: comedi: tests: Correct unittest_fptr
> 
> Should we move these to the normal kernel testing directory instead, so
> that they do get some build testing?

I'm not sure since they are in "drivers/staging".  However, we could add 
something to comedi's Kconfig to allow the tests to be built.

The "ni_routes_test" module in "drivers/staging/comedi/drivers/test/" 
would need a new config option because it will need to either depend on 
or select the COMEDI_NI_ROUTING option.  The "example_test" module (crap 
name) in the same directory doesn't depend on anything in particular.

-- 
-=( Ian Abbott <abbotti@mev.co.uk> || MEV Ltd. is a company  )=-
-=( registered in England & Wales.  Regd. number: 02862268.  )=-
-=( Regd. addr.: S11 & 12 Building 67, Europa Business Park, )=-
-=( Bird Hall Lane, STOCKPORT, SK3 0XA, UK. || www.mev.co.uk )=-

Re: [PATCH 0/5] staging: comedi: tests: Fix various issues

[Spencer Olson]

On Wed, Apr 7, 2021 at 8:11 AM Ian Abbott <abbotti@mev.co.uk> wrote:
>
> The code under "drivers/staging/comedi/drivers/tests/" is built when the
> CONFIG_COMEDI_TESTS option is enabled.  That currently needs to be done
> on the "make" command line since the option does not appear in the
> Kconfig files.
>
> This patch series fixes a compilation error and various other niggles
> including checkpatch.pl stuff.
>
> 1) staging: comedi: tests: ni_routes_test: Fix compilation error
> 2) staging: comedi: tests: ni_routes_test: Put complex values in parentheses
> 3) staging: comedi: tests: ni_routes_test: Avoid CamelCase: <RVi>
> 4) staging: comedi: tests: ni_routes_test: Lines should not end with a '('
> 5) staging: comedi: tests: Correct unittest_fptr
>
>  .../staging/comedi/drivers/tests/example_test.c    |  2 +-
>  .../staging/comedi/drivers/tests/ni_routes_test.c  | 81 +++++++++++-----------
>  drivers/staging/comedi/drivers/tests/unittest.h    |  2 +-
>  3 files changed, 42 insertions(+), 43 deletions(-)
>

Hmm.  That must have been a fat-finger on my part when I was
submitting those patches.  I am pretty sure that at least one revision
in my copy did have the option comedi/Kconfig at one point.

Re: [PATCH 0/5] staging: comedi: tests: Fix various issues

[Spencer Olson]

On Wed, Apr 7, 2021 at 9:39 AM Ian Abbott <abbotti@mev.co.uk> wrote:
>
> On 07/04/2021 16:10, Greg Kroah-Hartman wrote:
> > On Wed, Apr 07, 2021 at 03:01:37PM +0100, Ian Abbott wrote:
> >> The code under "drivers/staging/comedi/drivers/tests/" is built when the
> >> CONFIG_COMEDI_TESTS option is enabled.  That currently needs to be done
> >> on the "make" command line since the option does not appear in the
> >> Kconfig files.
> >>
> >> This patch series fixes a compilation error and various other niggles
> >> including checkpatch.pl stuff.
> >>
> >> 1) staging: comedi: tests: ni_routes_test: Fix compilation error
> >> 2) staging: comedi: tests: ni_routes_test: Put complex values in parentheses
> >> 3) staging: comedi: tests: ni_routes_test: Avoid CamelCase: <RVi>
> >> 4) staging: comedi: tests: ni_routes_test: Lines should not end with a '('
> >> 5) staging: comedi: tests: Correct unittest_fptr
> >
> > Should we move these to the normal kernel testing directory instead, so
> > that they do get some build testing?
>
> I'm not sure since they are in "drivers/staging".  However, we could add
> something to comedi's Kconfig to allow the tests to be built.
>
> The "ni_routes_test" module in "drivers/staging/comedi/drivers/test/"
> would need a new config option because it will need to either depend on
> or select the COMEDI_NI_ROUTING option.  The "example_test" module (crap
> name) in the same directory doesn't depend on anything in particular.
>
> --
> -=( Ian Abbott <abbotti@mev.co.uk> || MEV Ltd. is a company  )=-
> -=( registered in England & Wales.  Regd. number: 02862268.  )=-
> -=( Regd. addr.: S11 & 12 Building 67, Europa Business Park, )=-
> -=( Bird Hall Lane, STOCKPORT, SK3 0XA, UK. || www.mev.co.uk )=-

I don't have any arguments against your complaint of the
"example_test" name :-).  It was written with the hope that more parts
of comedi might follow suit and create some unit tests.  In terms of
placement in the tree, I had been following a pattern set by another
module that also had some unit tests contained local to the module's
directory (can't remember now which one it was).

Spencer

Re: [PATCH 0/5] staging: comedi: tests: Fix various issues

[Ian Abbott]

On 07/04/2021 16:48, Spencer Olson wrote:
> On Wed, Apr 7, 2021 at 9:39 AM Ian Abbott <abbotti@mev.co.uk> wrote:
>> The "ni_routes_test" module in "drivers/staging/comedi/drivers/test/"
>> would need a new config option because it will need to either depend on
>> or select the COMEDI_NI_ROUTING option.  The "example_test" module (crap
>> name) in the same directory doesn't depend on anything in particular.
> 
> I don't have any arguments against your complaint of the
> "example_test" name :-).  It was written with the hope that more parts
> of comedi might follow suit and create some unit tests.  In terms of
> placement in the tree, I had been following a pattern set by another
> module that also had some unit tests contained local to the module's
> directory (can't remember now which one it was).

I might as well rename it to "comedi_example_test" then.

-- 
-=( Ian Abbott <abbotti@mev.co.uk> || MEV Ltd. is a company  )=-
-=( registered in England & Wales.  Regd. number: 02862268.  )=-
-=( Regd. addr.: S11 & 12 Building 67, Europa Business Park, )=-
-=( Bird Hall Lane, STOCKPORT, SK3 0XA, UK. || www.mev.co.uk )=-

Re: [PATCH 0/5] staging: comedi: tests: Fix various issues

[Greg Kroah-Hartman]

On Wed, Apr 07, 2021 at 04:39:25PM +0100, Ian Abbott wrote:
> On 07/04/2021 16:10, Greg Kroah-Hartman wrote:
> > On Wed, Apr 07, 2021 at 03:01:37PM +0100, Ian Abbott wrote:
> > > The code under "drivers/staging/comedi/drivers/tests/" is built when the
> > > CONFIG_COMEDI_TESTS option is enabled.  That currently needs to be done
> > > on the "make" command line since the option does not appear in the
> > > Kconfig files.
> > > 
> > > This patch series fixes a compilation error and various other niggles
> > > including checkpatch.pl stuff.
> > > 
> > > 1) staging: comedi: tests: ni_routes_test: Fix compilation error
> > > 2) staging: comedi: tests: ni_routes_test: Put complex values in parentheses
> > > 3) staging: comedi: tests: ni_routes_test: Avoid CamelCase: <RVi>
> > > 4) staging: comedi: tests: ni_routes_test: Lines should not end with a '('
> > > 5) staging: comedi: tests: Correct unittest_fptr
> > 
> > Should we move these to the normal kernel testing directory instead, so
> > that they do get some build testing?
> 
> I'm not sure since they are in "drivers/staging".  However, we could add
> something to comedi's Kconfig to allow the tests to be built.

We need to get the comedi code out of staging, I really don't think
there is any need to keep it in here anymore, do you?  I'll try to carve
some time next week to look at the code and if I can't find anything
left to do, start moving it out.

thanks,

greg k-h

Re: [PATCH 0/5] staging: comedi: tests: Fix various issues

[Ian Abbott]

On 07/04/2021 18:26, Greg Kroah-Hartman wrote:
> On Wed, Apr 07, 2021 at 04:39:25PM +0100, Ian Abbott wrote:
>> On 07/04/2021 16:10, Greg Kroah-Hartman wrote:
>>> On Wed, Apr 07, 2021 at 03:01:37PM +0100, Ian Abbott wrote:
>>>> The code under "drivers/staging/comedi/drivers/tests/" is built when the
>>>> CONFIG_COMEDI_TESTS option is enabled.  That currently needs to be done
>>>> on the "make" command line since the option does not appear in the
>>>> Kconfig files.
>>>>
>>>> This patch series fixes a compilation error and various other niggles
>>>> including checkpatch.pl stuff.
>>>>
>>>> 1) staging: comedi: tests: ni_routes_test: Fix compilation error
>>>> 2) staging: comedi: tests: ni_routes_test: Put complex values in parentheses
>>>> 3) staging: comedi: tests: ni_routes_test: Avoid CamelCase: <RVi>
>>>> 4) staging: comedi: tests: ni_routes_test: Lines should not end with a '('
>>>> 5) staging: comedi: tests: Correct unittest_fptr
>>>
>>> Should we move these to the normal kernel testing directory instead, so
>>> that they do get some build testing?
>>
>> I'm not sure since they are in "drivers/staging".  However, we could add
>> something to comedi's Kconfig to allow the tests to be built.
> 
> We need to get the comedi code out of staging, I really don't think
> there is any need to keep it in here anymore, do you?  I'll try to carve
> some time next week to look at the code and if I can't find anything
> left to do, start moving it out.

What do you mean - it's only been in staging for 12 and a half years! :-)

-- 
-=( Ian Abbott <abbotti@mev.co.uk> || MEV Ltd. is a company  )=-
-=( registered in England & Wales.  Regd. number: 02862268.  )=-
-=( Regd. addr.: S11 & 12 Building 67, Europa Business Park, )=-
-=( Bird Hall Lane, STOCKPORT, SK3 0XA, UK. || www.mev.co.uk )=-

Re: [PATCH 0/5] staging: comedi: tests: Fix various issues

[Greg Kroah-Hartman]

On Wed, Apr 07, 2021 at 07:20:45PM +0100, Ian Abbott wrote:
> On 07/04/2021 18:26, Greg Kroah-Hartman wrote:
> > On Wed, Apr 07, 2021 at 04:39:25PM +0100, Ian Abbott wrote:
> > > On 07/04/2021 16:10, Greg Kroah-Hartman wrote:
> > > > On Wed, Apr 07, 2021 at 03:01:37PM +0100, Ian Abbott wrote:
> > > > > The code under "drivers/staging/comedi/drivers/tests/" is built when the
> > > > > CONFIG_COMEDI_TESTS option is enabled.  That currently needs to be done
> > > > > on the "make" command line since the option does not appear in the
> > > > > Kconfig files.
> > > > > 
> > > > > This patch series fixes a compilation error and various other niggles
> > > > > including checkpatch.pl stuff.
> > > > > 
> > > > > 1) staging: comedi: tests: ni_routes_test: Fix compilation error
> > > > > 2) staging: comedi: tests: ni_routes_test: Put complex values in parentheses
> > > > > 3) staging: comedi: tests: ni_routes_test: Avoid CamelCase: <RVi>
> > > > > 4) staging: comedi: tests: ni_routes_test: Lines should not end with a '('
> > > > > 5) staging: comedi: tests: Correct unittest_fptr
> > > > 
> > > > Should we move these to the normal kernel testing directory instead, so
> > > > that they do get some build testing?
> > > 
> > > I'm not sure since they are in "drivers/staging".  However, we could add
> > > something to comedi's Kconfig to allow the tests to be built.
> > 
> > We need to get the comedi code out of staging, I really don't think
> > there is any need to keep it in here anymore, do you?  I'll try to carve
> > some time next week to look at the code and if I can't find anything
> > left to do, start moving it out.
> 
> What do you mean - it's only been in staging for 12 and a half years! :-)

Yeah, it's about time :)

Re: [PATCH 0/5] staging: comedi: tests: Fix various issues

[Dan Carpenter]

I've been saying we should move this code out of staging for years now.

Normally I like to do a final static analysis check before drivers move
out of staging.

This driver is doing a bunch of DMA on stack which doesn't work on
all architectures.  You have to use kmalloc() (or vmalloc() I suppose)
memory for DMA.

drivers/staging/comedi/drivers/dt9812.c:249 dt9812_read_info() error: doing dma on the stack (&cmd)
drivers/staging/comedi/drivers/dt9812.c:273 dt9812_read_multiple_registers() error: doing dma on the stack (&cmd)
drivers/staging/comedi/drivers/dt9812.c:299 dt9812_write_multiple_registers() error: doing dma on the stack (&cmd)
drivers/staging/comedi/drivers/dt9812.c:318 dt9812_rmw_multiple_registers() error: doing dma on the stack (&cmd)
drivers/staging/comedi/drivers/dt9812.c:330 dt9812_digital_in() error: doing dma on the stack (value)
drivers/staging/comedi/drivers/dt9812.c:456 dt9812_analog_in() error: doing dma on the stack (val)
drivers/staging/comedi/drivers/dt9812.c:692 dt9812_reset_device() error: doing dma on the stack (&tmp8)
drivers/staging/comedi/drivers/dt9812.c:700 dt9812_reset_device() error: doing dma on the stack (&tmp8)
drivers/staging/comedi/drivers/dt9812.c:711 dt9812_reset_device() error: doing dma on the stack (&tmp16)
drivers/staging/comedi/drivers/dt9812.c:718 dt9812_reset_device() error: doing dma on the stack (&tmp16)
drivers/staging/comedi/drivers/dt9812.c:725 dt9812_reset_device() error: doing dma on the stack (&tmp16)
drivers/staging/comedi/drivers/dt9812.c:732 dt9812_reset_device() error: doing dma on the stack (&tmp32)

The test_ni_get_reg_value() function calls
	unittest(ni_get_reg_value_roffs(-1, O(0), T, 1) == -1,
		 "check bad direct trigger arg for first reg->dest w/offs\n");
The -1 is type promoted to a high positive value so src is greater than
NI_NAMES_BASE.  It's not clear that that was intentional.
drivers/staging/comedi/drivers/tests/../ni_routes.h:269 ni_get_reg_value_roffs() warn: 'src' possible negative type promoted to high

drivers/staging/comedi/drivers/ni_routes.c:61 ni_find_route_values() warn: 'device_family' sometimes too small '8,11' size = 30
    59          for (i = 0; ni_all_route_values[i]; ++i) {
    60                  if (memcmp(ni_all_route_values[i]->family, device_family,
                            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    61                             strnlen(device_family, 30)) == 0) {
                                   ^^^^^^^^^^^^^^^^^^^^^^^^^^
This whole memcmp() is very strange.  Why not just use:

	if (strncmp(ni_all_route_values[i]->family, device_family, 30) == 0)

    62                          rv = &ni_all_route_values[i]->register_values[0][0];
    63                          break;
    64                  }
    65          }

There are a couple places where conditions could probably be deleted.
drivers/staging/comedi/drivers/ni_mio_common.c:2363 ni_ai_cmd() warn: we tested 'dev->irq' before and it was 'true'
drivers/staging/comedi/drivers/usbdux.c:1192 usbduxsub_pwm_irq() warn: we tested 'devpriv->pwm_cmd_running' before and it was 'true'

There is some commented out code that might be worth reviewing.
drivers/staging/comedi/drivers/ni_mio_common.c:6306 ni_E_init() warn: odd binop '0x2 & 0x0'
drivers/staging/comedi/drivers/cb_pcidas64.c:2229 use_hw_sample_counter() warn: ignoring unreachable code.

These places are using char for bitwise operations and there is some
sign extension going on.
drivers/staging/comedi/drivers/icp_multi.c:120 icp_multi_ai_insn_read() warn: using signed char for bitops
drivers/staging/comedi/drivers/usbdux.c:1290 usbdux_pwm_pattern() warn: using signed char for bitops
drivers/staging/comedi/drivers/usbdux.c:1292 usbdux_pwm_pattern() warn: using signed char for bitops

Small resource leaks:
drivers/staging/comedi/drivers/ii_pci20kc.c:503 ii20k_attach() warn: 'dev->mmio' not released on lines: 452,457,465,474,483.
drivers/staging/comedi/drivers/ii_pci20kc.c:503 ii20k_attach() warn: 'membase' not released on lines: 441,452,457,465,474,483.
drivers/staging/comedi/drivers/gsc_hpdi.c:672 gsc_hpdi_auto_attach() warn: 'pcidev->irq' not released on lines: 629,641,646,651,655.
drivers/staging/comedi/drivers/addi_apci_2032.c:289 apci2032_auto_attach() warn: 'pcidev->irq' not released on lines: 279.
drivers/staging/comedi/drivers/cb_pcidas.c:1446 cb_pcidas_auto_attach() warn: 'pcidev->irq' not released on lines: 1295,1301,1305,1340,1358,1378,1409,1427.
drivers/staging/comedi/drivers/cb_pcidas64.c:4046 auto_attach() warn: 'pcidev->irq' not released on lines: 4044.

regards,
dan carpenter

Re: [PATCH 0/5] staging: comedi: tests: Fix various issues

[Ian Abbott]

On 14/04/2021 11:09, Dan Carpenter wrote:
> Small resource leaks:
> drivers/staging/comedi/drivers/ii_pci20kc.c:503 ii20k_attach() warn: 'dev->mmio' not released on lines: 452,457,465,474,483.
> drivers/staging/comedi/drivers/ii_pci20kc.c:503 ii20k_attach() warn: 'membase' not released on lines: 441,452,457,465,474,483.

The comedi core calls the driver's "detach" handler if the "attach" or 
"auto_attach" handler returns an error, so those resources will be freed 
in ii20k_detach().

> drivers/staging/comedi/drivers/gsc_hpdi.c:672 gsc_hpdi_auto_attach() warn: 'pcidev->irq' not released on lines: 629,641,646,651,655.

Similarly, gsc_hpdi_detach() will clean up when gsc_hpdi_auto_attach() 
returns an error.

> drivers/staging/comedi/drivers/addi_apci_2032.c:289 apci2032_auto_attach() warn: 'pcidev->irq' not released on lines: 279.

apci2032_detach() is called to clean up when apci2032_auto_attach() 
returns an error.  The IRQ is released by the call to comedi_pci_detach().

> drivers/staging/comedi/drivers/cb_pcidas.c:1446 cb_pcidas_auto_attach() warn: 'pcidev->irq' not released on lines: 1295,1301,1305,1340,1358,1378,1409,1427.

cb_pcidas_detach() is called to clean up when cb_pcidas_auto_attach() 
returns an error.  Some of the clean-up occurs in the call to 
comedi_pci_detach().

> drivers/staging/comedi/drivers/cb_pcidas64.c:4046 auto_attach() warn: 'pcidev->irq' not released on lines: 4044.

detach() is called to clean up when auto_attach() returns an error.

> regards,
> dan carpenter

Thanks.  I'll take a look at the other stuff you mentioned.

Regards,
Ian Abbott

-- 
-=( Ian Abbott <abbotti@mev.co.uk> || MEV Ltd. is a company  )=-
-=( registered in England & Wales.  Regd. number: 02862268.  )=-
-=( Regd. addr.: S11 & 12 Building 67, Europa Business Park, )=-
-=( Bird Hall Lane, STOCKPORT, SK3 0XA, UK. || www.mev.co.uk )=-

Re: [PATCH 0/5] staging: comedi: tests: Fix various issues

[Ian Abbott]

On 14/04/2021 11:09, Dan Carpenter wrote:
> The test_ni_get_reg_value() function calls
> 	unittest(ni_get_reg_value_roffs(-1, O(0), T, 1) == -1,
> 		 "check bad direct trigger arg for first reg->dest w/offs\n");
> The -1 is type promoted to a high positive value so src is greater than
> NI_NAMES_BASE.  It's not clear that that was intentional.
> drivers/staging/comedi/drivers/tests/../ni_routes.h:269 ni_get_reg_value_roffs() warn: 'src' possible negative type promoted to high

I agree that it appears that ni_get_reg_value_roffs() will be returning 
-1 as expected, but for the wrong reason.  I think the intention was for 
ni_get_reg_value_roffs() to call route_register_is_valid() with src=0 in 
this case, but it actually calls ni_route_to_register() with src=-1 
(because -1 gets converted to 0xffffffff in the test `if (src < 
NI_NAMES_BASE)` where NI_NAMES_BASE is defined as 0x8000u).

> 
> drivers/staging/comedi/drivers/ni_routes.c:61 ni_find_route_values() warn: 'device_family' sometimes too small '8,11' size = 30
>      59          for (i = 0; ni_all_route_values[i]; ++i) {
>      60                  if (memcmp(ni_all_route_values[i]->family, device_family,
>                              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>      61                             strnlen(device_family, 30)) == 0) {
>                                     ^^^^^^^^^^^^^^^^^^^^^^^^^^
> This whole memcmp() is very strange.  Why not just use:
> 
> 	if (strncmp(ni_all_route_values[i]->family, device_family, 30) == 0)

I think even a simple strcmp() would do as well because all the device 
family strings and board name strings are null terminated.  I don't know 
why the magic number 30 is used here!

The above applies similarly to ni_find_valid_routes() too.

> 
>      62                          rv = &ni_all_route_values[i]->register_values[0][0];
>      63                          break;
>      64                  }
>      65          }
> 
> There are a couple places where conditions could probably be deleted.
> drivers/staging/comedi/drivers/ni_mio_common.c:2363 ni_ai_cmd() warn: we tested 'dev->irq' before and it was 'true'

Agreed.  In fact ni_ai_cmd() shouldn't be getting called at all if 
dev->irq is 0.

> drivers/staging/comedi/drivers/usbdux.c:1192 usbduxsub_pwm_irq() warn: we tested 'devpriv->pwm_cmd_running' before and it was 'true'

Agreed.

> There is some commented out code that might be worth reviewing.
> drivers/staging/comedi/drivers/ni_mio_common.c:6306 ni_E_init() warn: odd binop '0x2 & 0x0'

Yes, that can be simplified.

> drivers/staging/comedi/drivers/cb_pcidas64.c:2229 use_hw_sample_counter() warn: ignoring unreachable code.

Yes, that is preceded by:

/* disable for now until I work out a race */
	return 0;

But that change dates back to 2002.  Perhaps we should use `#if 0` 
`#endif` around the unreachable lines of code on the off-chance that it 
gets revisited sometime!

> These places are using char for bitwise operations and there is some
> sign extension going on.
> drivers/staging/comedi/drivers/icp_multi.c:120 icp_multi_ai_insn_read() warn: using signed char for bitops

Agreed, range_code_analog[] ought to be unsigned char instead of plain char.

> drivers/staging/comedi/drivers/usbdux.c:1290 usbdux_pwm_pattern() warn: using signed char for bitops
> drivers/staging/comedi/drivers/usbdux.c:1292 usbdux_pwm_pattern() warn: using signed char for bitops

I think usbdux_pwm_pattern() should be using unsigned char throughout. 
Also, the initialization `char sgn_mask = (16 << chan);` seems strange 
because `chan` can be in the range 0 to 7 here!

-- 
-=( Ian Abbott <abbotti@mev.co.uk> || MEV Ltd. is a company  )=-
-=( registered in England & Wales.  Regd. number: 02862268.  )=-
-=( Regd. addr.: S11 & 12 Building 67, Europa Business Park, )=-
-=( Bird Hall Lane, STOCKPORT, SK3 0XA, UK. || www.mev.co.uk )=-

Re: [PATCH 0/5] staging: comedi: tests: Fix various issues

[Ian Abbott]

On 14/04/2021 11:09, Dan Carpenter wrote:
> This driver is doing a bunch of DMA on stack which doesn't work on
> all architectures.  You have to use kmalloc() (or vmalloc() I suppose)
> memory for DMA.
> 
> drivers/staging/comedi/drivers/dt9812.c:249 dt9812_read_info() error: doing dma on the stack (&cmd)
> drivers/staging/comedi/drivers/dt9812.c:273 dt9812_read_multiple_registers() error: doing dma on the stack (&cmd)
> drivers/staging/comedi/drivers/dt9812.c:299 dt9812_write_multiple_registers() error: doing dma on the stack (&cmd)
> drivers/staging/comedi/drivers/dt9812.c:318 dt9812_rmw_multiple_registers() error: doing dma on the stack (&cmd)
> drivers/staging/comedi/drivers/dt9812.c:330 dt9812_digital_in() error: doing dma on the stack (value)
> drivers/staging/comedi/drivers/dt9812.c:456 dt9812_analog_in() error: doing dma on the stack (val)
> drivers/staging/comedi/drivers/dt9812.c:692 dt9812_reset_device() error: doing dma on the stack (&tmp8)
> drivers/staging/comedi/drivers/dt9812.c:700 dt9812_reset_device() error: doing dma on the stack (&tmp8)
> drivers/staging/comedi/drivers/dt9812.c:711 dt9812_reset_device() error: doing dma on the stack (&tmp16)
> drivers/staging/comedi/drivers/dt9812.c:718 dt9812_reset_device() error: doing dma on the stack (&tmp16)
> drivers/staging/comedi/drivers/dt9812.c:725 dt9812_reset_device() error: doing dma on the stack (&tmp16)
> drivers/staging/comedi/drivers/dt9812.c:732 dt9812_reset_device() error: doing dma on the stack (&tmp32)

Yes, it seems it requires a bit of an overhaul!

-- 
-=( Ian Abbott <abbotti@mev.co.uk> || MEV Ltd. is a company  )=-
-=( registered in England & Wales.  Regd. number: 02862268.  )=-
-=( Regd. addr.: S11 & 12 Building 67, Europa Business Park, )=-
-=( Bird Hall Lane, STOCKPORT, SK3 0XA, UK. || www.mev.co.uk )=-

Re: [PATCH 0/5] staging: comedi: tests: Fix various issues

[Greg Kroah-Hartman]

On Wed, Apr 14, 2021 at 01:40:34PM +0100, Ian Abbott wrote:
> On 14/04/2021 11:09, Dan Carpenter wrote:
> > This driver is doing a bunch of DMA on stack which doesn't work on
> > all architectures.  You have to use kmalloc() (or vmalloc() I suppose)
> > memory for DMA.
> > 
> > drivers/staging/comedi/drivers/dt9812.c:249 dt9812_read_info() error: doing dma on the stack (&cmd)
> > drivers/staging/comedi/drivers/dt9812.c:273 dt9812_read_multiple_registers() error: doing dma on the stack (&cmd)
> > drivers/staging/comedi/drivers/dt9812.c:299 dt9812_write_multiple_registers() error: doing dma on the stack (&cmd)
> > drivers/staging/comedi/drivers/dt9812.c:318 dt9812_rmw_multiple_registers() error: doing dma on the stack (&cmd)
> > drivers/staging/comedi/drivers/dt9812.c:330 dt9812_digital_in() error: doing dma on the stack (value)
> > drivers/staging/comedi/drivers/dt9812.c:456 dt9812_analog_in() error: doing dma on the stack (val)
> > drivers/staging/comedi/drivers/dt9812.c:692 dt9812_reset_device() error: doing dma on the stack (&tmp8)
> > drivers/staging/comedi/drivers/dt9812.c:700 dt9812_reset_device() error: doing dma on the stack (&tmp8)
> > drivers/staging/comedi/drivers/dt9812.c:711 dt9812_reset_device() error: doing dma on the stack (&tmp16)
> > drivers/staging/comedi/drivers/dt9812.c:718 dt9812_reset_device() error: doing dma on the stack (&tmp16)
> > drivers/staging/comedi/drivers/dt9812.c:725 dt9812_reset_device() error: doing dma on the stack (&tmp16)
> > drivers/staging/comedi/drivers/dt9812.c:732 dt9812_reset_device() error: doing dma on the stack (&tmp32)
> 
> Yes, it seems it requires a bit of an overhaul!

As no one has complained about the warning messages they would be
getting if they tried to use this driver in the past year, are there any
real users of it?

This should be an easy thing to fix up for someone interested in getting
some experience in kernel development...

thanks,

greg k-h

Re: [PATCH 0/5] staging: comedi: tests: Fix various issues

[Dan Carpenter]

On Wed, Apr 14, 2021 at 01:34:23PM +0100, Ian Abbott wrote:
> > drivers/staging/comedi/drivers/ni_routes.c:61 ni_find_route_values() warn: 'device_family' sometimes too small '8,11' size = 30
> >      59          for (i = 0; ni_all_route_values[i]; ++i) {
> >      60                  if (memcmp(ni_all_route_values[i]->family, device_family,
> >                              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> >      61                             strnlen(device_family, 30)) == 0) {
> >                                     ^^^^^^^^^^^^^^^^^^^^^^^^^^
> > This whole memcmp() is very strange.  Why not just use:
> > 
> > 	if (strncmp(ni_all_route_values[i]->family, device_family, 30) == 0)
> 
> I think even a simple strcmp() would do as well because all the device
> family strings and board name strings are null terminated.  I don't know why
> the magic number 30 is used here!
> 
> The above applies similarly to ni_find_valid_routes() too.
> 

I was thinking maybe ni_all_route_values[i]->family has an additional
string on the end.  For example, it could end in "_bar" and we want
->family "foo_bar" to match with device_family "foo"?

regards,
dan carpenter

Re: [PATCH 0/5] staging: comedi: tests: Fix various issues

[Dan Carpenter]

On Wed, Apr 14, 2021 at 01:40:34PM +0100, Ian Abbott wrote:
> On 14/04/2021 11:09, Dan Carpenter wrote:
> > This driver is doing a bunch of DMA on stack which doesn't work on
> > all architectures.  You have to use kmalloc() (or vmalloc() I suppose)
> > memory for DMA.
> > 
> > drivers/staging/comedi/drivers/dt9812.c:249 dt9812_read_info() error: doing dma on the stack (&cmd)
> > drivers/staging/comedi/drivers/dt9812.c:273 dt9812_read_multiple_registers() error: doing dma on the stack (&cmd)
> > drivers/staging/comedi/drivers/dt9812.c:299 dt9812_write_multiple_registers() error: doing dma on the stack (&cmd)
> > drivers/staging/comedi/drivers/dt9812.c:318 dt9812_rmw_multiple_registers() error: doing dma on the stack (&cmd)
> > drivers/staging/comedi/drivers/dt9812.c:330 dt9812_digital_in() error: doing dma on the stack (value)
> > drivers/staging/comedi/drivers/dt9812.c:456 dt9812_analog_in() error: doing dma on the stack (val)
> > drivers/staging/comedi/drivers/dt9812.c:692 dt9812_reset_device() error: doing dma on the stack (&tmp8)
> > drivers/staging/comedi/drivers/dt9812.c:700 dt9812_reset_device() error: doing dma on the stack (&tmp8)
> > drivers/staging/comedi/drivers/dt9812.c:711 dt9812_reset_device() error: doing dma on the stack (&tmp16)
> > drivers/staging/comedi/drivers/dt9812.c:718 dt9812_reset_device() error: doing dma on the stack (&tmp16)
> > drivers/staging/comedi/drivers/dt9812.c:725 dt9812_reset_device() error: doing dma on the stack (&tmp16)
> > drivers/staging/comedi/drivers/dt9812.c:732 dt9812_reset_device() error: doing dma on the stack (&tmp32)
> 
> Yes, it seems it requires a bit of an overhaul!

It's just 6 functions in a single file...

regards,
dan carpenter

Re: [PATCH 0/5] staging: comedi: tests: Fix various issues

[Spencer Olson]

On Wed, Apr 14, 2021 at 7:28 AM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> On Wed, Apr 14, 2021 at 01:34:23PM +0100, Ian Abbott wrote:
> > > drivers/staging/comedi/drivers/ni_routes.c:61 ni_find_route_values() warn: 'device_family' sometimes too small '8,11' size = 30
> > >      59          for (i = 0; ni_all_route_values[i]; ++i) {
> > >      60                  if (memcmp(ni_all_route_values[i]->family, device_family,
> > >                              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > >      61                             strnlen(device_family, 30)) == 0) {
> > >                                     ^^^^^^^^^^^^^^^^^^^^^^^^^^
> > > This whole memcmp() is very strange.  Why not just use:
> > >
> > >     if (strncmp(ni_all_route_values[i]->family, device_family, 30) == 0)
> >
> > I think even a simple strcmp() would do as well because all the device
> > family strings and board name strings are null terminated.  I don't know why
> > the magic number 30 is used here!
> >
> > The above applies similarly to ni_find_valid_routes() too.
> >
>
> I was thinking maybe ni_all_route_values[i]->family has an additional
> string on the end.  For example, it could end in "_bar" and we want
> ->family "foo_bar" to match with device_family "foo"?

No that was not my intention.  Looking through the code again, it
looks like my intention was to simply provide some type of a
reasonable limit for the strings that are passed into the function
that come from hard-coded family names that are in ni_660x.c and
ni_mio_common.c where they call ni_assign_device_routes.  Indeed, the
string lengths are (so far) only actually 8 ("ni_660x\0") and 11
("ni_mseries\0" and "ni_eseries\0") characters.  In other words, I
think my intention was to simply create a somewhat generic interface
that other ni device drivers in comedi could use.  There was not a
good reason for using a magic number of 30 except that it was bigger
than either 8 or 11 and was probably large enough to always be
expected to be bigger than any other ni device family name without
being too large.

Spencer

Re: [PATCH 0/5] staging: comedi: tests: Fix various issues

[Spencer Olson]

On Wed, Apr 14, 2021 at 6:34 AM Ian Abbott <abbotti@mev.co.uk> wrote:
>
> On 14/04/2021 11:09, Dan Carpenter wrote:
> > The test_ni_get_reg_value() function calls
> >       unittest(ni_get_reg_value_roffs(-1, O(0), T, 1) == -1,
> >                "check bad direct trigger arg for first reg->dest w/offs\n");
> > The -1 is type promoted to a high positive value so src is greater than
> > NI_NAMES_BASE.  It's not clear that that was intentional.
> > drivers/staging/comedi/drivers/tests/../ni_routes.h:269 ni_get_reg_value_roffs() warn: 'src' possible negative type promoted to high
>
> I agree that it appears that ni_get_reg_value_roffs() will be returning
> -1 as expected, but for the wrong reason.  I think the intention was for
> ni_get_reg_value_roffs() to call route_register_is_valid() with src=0 in
> this case, but it actually calls ni_route_to_register() with src=-1
> (because -1 gets converted to 0xffffffff in the test `if (src <
> NI_NAMES_BASE)` where NI_NAMES_BASE is defined as 0x8000u).
>

Good catch.  Based on the error message that for the unittest failure,
I would agree that my intention had been to test when src is indeed <
NI_NAMES_BASE so that we could test for a bad direct register value.
It does indeed look like sending in 0 for the src would have worked,
since the first row in private table RV in ni_routes_test.c (the row
for destination passed in as "O(0)") does not have a "register value"
equal to 0.  It would be nice to compile and test this code though,
but I can't do it for another couple of weeks.

Actually, looking a little further, I don't think that src=0 will work
here.  There is another argument for ni_get_reg_value_roffs,
direct_reg_offset, that gets added to src before sending to
route_register_is_valid.  I think this was provided to fix an offset
mismatch that was in the original approach to handling signal routing
values (i.e. to provide backwards compatibility).  But, I do think
that value greater or equal to 9 will actually work.

Re: [PATCH 0/5] staging: comedi: tests: Fix various issues

[Ian Abbott]

On 14/04/2021 15:29, Spencer Olson wrote:
> On Wed, Apr 14, 2021 at 6:34 AM Ian Abbott <abbotti@mev.co.uk> wrote:
>>
>> On 14/04/2021 11:09, Dan Carpenter wrote:
>>> The test_ni_get_reg_value() function calls
>>>       unittest(ni_get_reg_value_roffs(-1, O(0), T, 1) == -1,
>>>                "check bad direct trigger arg for first reg->dest w/offs\n");
>>> The -1 is type promoted to a high positive value so src is greater than
>>> NI_NAMES_BASE.  It's not clear that that was intentional.
>>> drivers/staging/comedi/drivers/tests/../ni_routes.h:269 ni_get_reg_value_roffs() warn: 'src' possible negative type promoted to high
>>
>> I agree that it appears that ni_get_reg_value_roffs() will be returning
>> -1 as expected, but for the wrong reason.  I think the intention was for
>> ni_get_reg_value_roffs() to call route_register_is_valid() with src=0 in
>> this case, but it actually calls ni_route_to_register() with src=-1
>> (because -1 gets converted to 0xffffffff in the test `if (src <
>> NI_NAMES_BASE)` where NI_NAMES_BASE is defined as 0x8000u).
>>
> 
> Good catch.  Based on the error message that for the unittest failure,
> I would agree that my intention had been to test when src is indeed <
> NI_NAMES_BASE so that we could test for a bad direct register value.
> It does indeed look like sending in 0 for the src would have worked,
> since the first row in private table RV in ni_routes_test.c (the row
> for destination passed in as "O(0)") does not have a "register value"
> equal to 0.  It would be nice to compile and test this code though,
> but I can't do it for another couple of weeks.
> 
> Actually, looking a little further, I don't think that src=0 will work
> here.  There is another argument for ni_get_reg_value_roffs,
> direct_reg_offset, that gets added to src before sending to
> route_register_is_valid.  I think this was provided to fix an offset
> mismatch that was in the original approach to handling signal routing
> values (i.e. to provide backwards compatibility).  But, I do think
> that value greater or equal to 9 will actually work.

Since that unittest() is calling ni_get_reg_value_roffs() with src=-1
and direct_reg_offset=1, and ni_get_reg_value_roffs() adds
direct_reg_offset to src if src < NI_NAMES_BASE, would setting src=0,
direct_reg_offset=0 work? I.e., like this:

        unittest(ni_get_reg_value_roffs(0, O(0), T, 0) == -1,
                 "check bad direct trigger arg for first reg->dest
w/offs\n");

-- 
-=( Ian Abbott <abbotti@mev.co.uk> || MEV Ltd. is a company  )=-
-=( registered in England & Wales.  Regd. number: 02862268.  )=-
-=( Regd. addr.: S11 & 12 Building 67, Europa Business Park, )=-
-=( Bird Hall Lane, STOCKPORT, SK3 0XA, UK. || www.mev.co.uk )=-

Re: [PATCH 0/5] staging: comedi: tests: Fix various issues

[Ian Abbott]

On 14/04/2021 14:28, Dan Carpenter wrote:
> On Wed, Apr 14, 2021 at 01:34:23PM +0100, Ian Abbott wrote:
>>> drivers/staging/comedi/drivers/ni_routes.c:61 ni_find_route_values() warn: 'device_family' sometimes too small '8,11' size = 30
>>>      59          for (i = 0; ni_all_route_values[i]; ++i) {
>>>      60                  if (memcmp(ni_all_route_values[i]->family, device_family,
>>>                              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>>      61                             strnlen(device_family, 30)) == 0) {
>>>                                     ^^^^^^^^^^^^^^^^^^^^^^^^^^
>>> This whole memcmp() is very strange.  Why not just use:
>>>
>>> 	if (strncmp(ni_all_route_values[i]->family, device_family, 30) == 0)
>>
>> I think even a simple strcmp() would do as well because all the device
>> family strings and board name strings are null terminated.  I don't know why
>> the magic number 30 is used here!
>>
>> The above applies similarly to ni_find_valid_routes() too.
>>
> 
> I was thinking maybe ni_all_route_values[i]->family has an additional
> string on the end.  For example, it could end in "_bar" and we want
> ->family "foo_bar" to match with device_family "foo"?

That doesn't seem to be the case.  The family names are just string
literals pointed to by the 'family' member of 'struct
family_route_values' in
".../comedi/drivers/ni_routing/ni_route_values.h".  Instances of 'struct
family_route_values' are statically defined in
".../comedi/drivers/ni_routing/ni_route_values/*.c".

-- 
-=( Ian Abbott <abbotti@mev.co.uk> || MEV Ltd. is a company  )=-
-=( registered in England & Wales.  Regd. number: 02862268.  )=-
-=( Regd. addr.: S11 & 12 Building 67, Europa Business Park, )=-
-=( Bird Hall Lane, STOCKPORT, SK3 0XA, UK. || www.mev.co.uk )=-

Re: [PATCH 0/5] staging: comedi: tests: Fix various issues

[Ian Abbott]

On 14/04/2021 13:34, Ian Abbott wrote:
> On 14/04/2021 11:09, Dan Carpenter wrote:
>> The test_ni_get_reg_value() function calls
>>     unittest(ni_get_reg_value_roffs(-1, O(0), T, 1) == -1,
>>          "check bad direct trigger arg for first reg->dest w/offs\n");
>> The -1 is type promoted to a high positive value so src is greater than
>> NI_NAMES_BASE.  It's not clear that that was intentional.
>> drivers/staging/comedi/drivers/tests/../ni_routes.h:269 
>> ni_get_reg_value_roffs() warn: 'src' possible negative type promoted 
>> to high
> 
> I agree that it appears that ni_get_reg_value_roffs() will be returning 
> -1 as expected, but for the wrong reason.  I think the intention was for 
> ni_get_reg_value_roffs() to call route_register_is_valid() with src=0 in 
> this case, but it actually calls ni_route_to_register() with src=-1 
> (because -1 gets converted to 0xffffffff in the test `if (src < 
> NI_NAMES_BASE)` where NI_NAMES_BASE is defined as 0x8000u).

Since Spencer agrees that the existing behavior was unintentional, I 
think the least disruptive fix would be change `src < NI_NAMES_BASE` to 
`src < (int)NI_NAMES_BASE` in `ni_get_reg_value_roffs()` in 
".../comedi/drivers/ni_routes.h".

Either that, or change the interface to use unsigned `src` and 
`direct_reg_offset` values.  The negative values only seem to be used by 
the "ni_routes_test" unit testing module.

-- 
-=( Ian Abbott <abbotti@mev.co.uk> || MEV Ltd. is a company  )=-
-=( registered in England & Wales.  Regd. number: 02862268.  )=-
-=( Regd. addr.: S11 & 12 Building 67, Europa Business Park, )=-
-=( Bird Hall Lane, STOCKPORT, SK3 0XA, UK. || www.mev.co.uk )=-