On Tue, Sep 25, 2018 at 6:26 AM Ian Abbott <abbotti@xxxxxxxxx> wrote:
>
> On 25/09/18 05:47, Spencer E. Olson wrote:
>
> [N.B. top-posting is frowned upon on the kernel mailing lists.]
Sorry :-)
>
> > 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?
I struggled with this one a tiny bit. The main reason why this is
exported is so that I could use it inside one of the unit tests in the
tests/ni_routes_test module (these were indeed invaluable for testing
on a system that didn't have real hardware attached). Otherwise, I do
not really imagine this being used externally. Since this function is
fully self-contained (i.e. only touches local variables and no module
variables), I figured that this export was at least safe.
>
> 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[] = {
> ^^^^^
Thanks for the good review and explanation here. I have now
implemented each of these "const" and casting fixes--ready for
re-submission when the other patches in the set are re-finished.
> > 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.
In my working copy, I now have the ni_routing module that is comprised of:
ni_routes.o
ni_routing/ni_route_values.o
ni_routing/ni_route_values/ni_660x.o
ni_routing/ni_route_values/ni_eseries.o
ni_routing/ni_route_values/ni_mseries.o
ni_routing/ni_device_routes.o
(i.e. I've only broken out the components of what was originally
included in ni_routing/ni_route_values.c so far)
I am not very satisfied with the result--with respect to maintenance.
For the ni_route_values*, this is not too much of a deal since there
are not that many hardware families. For ni_device_routes*, this
seems a bit much, since each new hardware device implies
- a new compile unit in the Makefile ni_routing module
- two new lines in ni_routing/ni_device_routes.c (extern declaration
& addition to array)
- and a new file for the device route list in sub-directory ni_routing/
Currently, I have ni_route_values.c declaring each of the arrays from
ni_routing/ni_route_values/ni_*.o, rather than going overboard and
making .h files for these to include in ni_route_values.c. If I also
break out the arrays in ni_routing/ni_device_routes/*.c, I would make
this approach to avoid a fourth implication for maintenance. Breaking
out the ni_routing/ni_device_routes/*.c content into separate compile
units results in 18 more compile units that are linked into
ni_routing.ko and 18 additional lines in ni_device_routes.
I'm willing to be encouraged one way or another, but would like to see
your feedback/thoughts on the maintenance aspect.
>
> --
> -=( 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
