Re-examine module arg handling in hm2_soc_ol #20

Closed
ArcEye opened this Issue Jan 29, 2017 · 2 comments

Projects

None yet

1 participant

@ArcEye
Collaborator
ArcEye commented Jan 29, 2017 edited

Peculiarities in the module argument setting led to problems before even getting the hm2_soc_ol driver loaded in testing.

A descriptor field which should have been a NULL string, in fact returned a string which contained
"(null)"
#10 (comment)

The debug int value was uninitialised and resulted in it containing some random value, but never 0.
#10 (comment)

There should be scope for using the argc/argv system within the halcmd newinst handling of arguments after the -- delimiter, for any strings required to be passed to instantiable components.

This would completely sidestep this problem, which may well relate to the fact that kernel module args are persistent, because it was never envisaged that the same module would be loaded twice.

This was got around within components generated by instcomp, by saving the values locally and then zeroing any values, making values 0 or -1 before returning.
This prevents a value passed to one instance, being passed by default to all other instances, when no value is intended for that arg.

@ArcEye ArcEye added a commit to ArcEye/machinekit-multicore that referenced this issue Feb 1, 2017
@ArcEye ArcEye Modify parameter handling in hm2_soc_ol driver for DE0-NANO-Soc
It now complies with the guidelines established when writing
instcomp code generation, to prevent errors from the legacy
kernel module parameters being used and re-used
by instantiated components.

Fixes Issue #20

Signed-off-by: Mick <arceye@mgware.co.uk>
f80a4a4
@ArcEye ArcEye added a commit to ArcEye/machinekit-multicore that referenced this issue Feb 1, 2017
@ArcEye ArcEye Modify parameter handling in hm2_soc_ol driver for DE0-NANO-Soc
It now complies with the guidelines established when writing
instcomp code generation, to prevent errors from the legacy
kernel module parameters being used and re-used
by instantiated components.

Fixes Issue #20

Signed-off-by: Mick <arceye@mgware.co.uk>
f9b37dd
@ArcEye
Collaborator
ArcEye commented Feb 1, 2017 edited

hm2_soc_ol driver re-written in respect of parameter passing.

It now complies with the guidelines established when writing instcomp code generation, to prevent errors from the legacy kernel module parameters being used and re-used by instantiated components.

Basically:
int parameters
Instance parameters should consist of int values only. eg.

static int debug = 0;
RTAPI_IP_INT(debug, "turn on extra debug output");

These are safe to re-use the same allocated memory space when creating a new instance.

However, because new instances will re-use the same parameter, it should be zeroed or voided by the previous instance, once the value has been saved.
A value which is essentially bool, can just be set to 0, but a value which is within a range, is safest to be set to -1, then the new instance can set the default value if this -1 is passed unchanged.

Where the value should not be reused (eg. hm2_soc_ol board numbering), the value should also be set to -1 after being saved locally.
Then the next instance upon finding a -1 value, will know that there has been a previous instance created and that it is unsafe to continue without a valid value.

String parameters are particularly unsafe in re-use of the kernel module parameter, because the allocated memory can only be guaranteed sufficient for the first string passed.
If a second longer one is used, it could overrun.

They also appear prone to peculiar errors, such as found with the hm2_soc_ol where a parameter
which was set to NULL, was in fact returning a string of "(null)".

Strings can be passed easily after any int instance params and the -- delimiter, via the argc/argv mechanism and are therefore certain to be per instance args
eg

newinst hm2_soc_ol hm2-socfpga0 debug=0 num=1 -- config=firmware=socfpga/dtbo/DE0_Nano_SoC_DB25.7I76_7I76_7I76_7I76.dtbo num_encoders=2 num_stepgens=4 descriptor=/some/descriptor/file/address

The argc/argv passed to instantiate(), contain the module name at argv[0] and the instance name at argv[1]
They do not contain any of the instance params, which are dealt with separately.
Therefore, in the example above, config=xxx would be at argv[2] and descriptor=xxx at argv[3]
The component simply needs to test and parse these args and act accordingly.

In all cases the params / args are saved locally and those local copies used in all operations thereafter.
Referring to a global parameter after another instance has been created, will result in the value set by the latest instance being passed, not the original one.

@ArcEye
Collaborator
ArcEye commented Feb 1, 2017

Log of a fully debug printed load with new params
https://gist.github.com/ArcEye/0e1c56996ba8d6a404ff7bb60d239386

Log of an error load when the config=xxxx string is not passed
https://gist.github.com/ArcEye/111b9358bb313e225b5f2cda999c9209

@ArcEye ArcEye closed this Feb 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment