[PATCH 04/13] staging: comedi: ni_routing: Add NI signal routing info

[Spencer Olson]

On Tue, Sep 25, 2018 at 6:26 AM Ian Abbott <abbotti at mev.co.uk> 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 at mev.co.uk> || 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. )=-