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@xxxxxxxxx> || 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 )=-
