On 25/09/18 05:47, Spencer E. Olson wrote:
[N.B. top-posting is frowned upon on the kernel mailing lists.]
These static arrays are
(1) not expressed with as much "const"ness as suggested
(2) included into one compile unit
because
- ni_device_routes.routes and ni_route_set.src are sorted at module
load time using the kernel sort(...) routine.
- ni_device_routes.n_route_sets and ni_route_set.n_src data
members are changed as a result of the counting/sorting.
- ni_device_routes.routes and ni_route_set.src are both searched at
runtime using the kernel bsearch(...) routine.
These choices were made as a compromise between maintenance,
code-execution performance, and memory footprint. Rather than require a
large, mostly sparse table be kept for each ni hardware device, the
signal route information is divided up into one large table of register
values for each device family and smaller hardware-specific sorted lists
that can easily be searched to identify possible signal routes.
It seemed unreasonable to require a developer to maintain the proper
order of the structures to provide for best searching. It also seemed
unreasonable to require the developer to specifically instantiate the
ni_device_routes.n_route_sets and ni_route_set.n_src data members
correctly.
I never noticed that you sort the data on module load. You also have an
exported function 'ni_sort_device_routes()' called internally by
'ni_sort_all_device_routes()' when the "ni_routes" module is loaded. Do
you envision that function ever being called externally?
Your 'ni_sort_all_device_routes()' function iterates through
'device_routes_list[]' and calls 'ni_sort_device_routes()' on each of
the 'struct ni_device_routes *' values in the list. The list of
pointers itself should be 'const', but you have the 'const' in the wrong
place. You have it declared as:
static const struct ni_device_routes *device_routes_list[] = {
...
};
but I think you meant:
static struct ni_device_routes *const device_routes_list[] = {
...
};
i.e. the array of pointers is 'const', not the things they point to.
You work around that with a type cast to remove the 'const' in
'ni_sort_all_device_routes()'. That type cast should be avoided.
You also have type casts in your calls to 'sort()' and 'bsearch()' which
shouldn't be needed if you declare '_ni_sort_destcmp()',
'_ni_sort_srccmp()', '_ni_bsearch_destcmp()' and '_ni_bsearch_srccmp()'
properly.
The 'all_route_values[]' in "[...]/ni_routing/ni_route_values.c" can
still be made 'const':
static const struct family_route_values *const all_route_values[] = {
^^^^^
Because these arrays are sorted (at module load time) by ni_routes, it
seemed best to have the symbols for these tables only have static
linkage, thus ensuring that _only_ the ni_routes module accesses these.
I still think external linkage rather than .c file inclusion should be
doable. It shouldn't be that hard to link the "ni_routes" module from
several object files, and the consequent inability to use 'ARRAY_SIZE()'
on 'device_routes_list[]' and 'all_route_values[]' can be worked around
either by NULL terminating them or by storing their lengths in separate
variables.
If the likes of 'ni_660x_route_values' are declared with external
linkage, only "ni_routes" would be able to link to them if it is built
as a loadable module. If "ni_routes" is built into the kernel image
itself, then other code built into the kernel image would also be able
to link to them, but such code should be subject to review anyway.
--
-=( Ian Abbott <abbotti@xxxxxxxxx> || Web: www.mev.co.uk )=-
-=( MEV Ltd. is a company registered in England & Wales. )=-
-=( Registered number: 02862268. Registered address: )=-
-=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=-
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
