Re: [PATCH 07/13] staging: comedi: ni_mio_common: implement global pfi,rtsi routing
On Tue, Sep 25, 2018 at 4:27 AM Ian Abbott wrote:
>
> On 19/09/18 17:38, Spencer E. Olson wrote:
> > Implement device-global config interface for ni_mio devices. In
> > particular, this patch implements:
> > INSN_DEVICE_CONFIG_TEST_ROUTE,
> > INSN_DEVICE_CONFIG_CONNECT_ROUTE,
> > INSN_DEVICE_CONFIG_DISCONNECT_ROUTE,
> > INSN_DEVICE_CONFIG_GET_ROUTES
> > for the ni mio devices. This means that the new abstracted signal/terminal
> > names can be used to define signal routing with regards to the PFI
> > terminals and RTSI trigger bus lines.
> >
> > This also adds ability to identify PFI and RTSI channels on the PFI and
> > RTSI subdevices using the new device-global names. This does not change
> > the values that are set for channel output selections using the subdevice
> > interfaces--these still require direct register values.
> >
> > Annotates and updates tables of register values to reflect this new
> > implementation status.
> >
> > Signed-off-by: Spencer E. Olson
> > ---
> > .../staging/comedi/drivers/ni_mio_common.c| 687 --
> > drivers/staging/comedi/drivers/ni_stc.h | 68 ++
> > 2 files changed, 683 insertions(+), 72 deletions(-)
> >
> > diff --git a/drivers/staging/comedi/drivers/ni_mio_common.c
> > b/drivers/staging/comedi/drivers/ni_mio_common.c
> > index b033f0be9b8a..c42d480df7ff 100644
> > --- a/drivers/staging/comedi/drivers/ni_mio_common.c
> > +++ b/drivers/staging/comedi/drivers/ni_mio_common.c
>
> In trying an experimental application of your patches to "staging-next"
> (to examine the code more carefully), this (PATCH 07/13) failed to apply
> with "git am". When applying the failed patch manually with "patch
> -p1", there was one failed hunk (see below).
>
> > @@ -5040,13 +5077,17 @@ static unsigned int ni_get_rtsi_routing(struct
> > comedi_device *dev,
> > {
> > struct ni_private *devpriv = dev->private;
> >
> > + if (chan >= TRIGGER_LINE(0))
> > + /* allow new and old names of rtsi channels to work. */
> > + chan -= TRIGGER_LINE(0);
> > +
> > if (chan < 4) {
> > return NISTC_RTSI_TRIG_TO_SRC(chan,
> >
> > devpriv->rtsi_trig_a_output_reg);
> > } else if (chan < NISTC_RTSI_TRIG_NUM_CHAN(devpriv->is_m_series)) {
> > return NISTC_RTSI_TRIG_TO_SRC(chan,
> >
> > devpriv->rtsi_trig_b_output_reg);
> > - } else if (chan == NISTC_RTSI_TRIG_OLD_CLK_CHAN)
> > + } else if (chan == NISTC_RTSI_TRIG_OLD_CLK_CHAN) {
> > return NI_RTSI_OUTPUT_RTSI_OSC;
> > }
> >
>
> That is the hunk that failed to apply. The line numbers seem a bit off
> the original source, and the "} else if" part at the end didn't match at
> all.
>
> Perhaps you need to rebase?
After looking at this patch, it looks like this is based on my earlier
patch that you "stamped" reviewed. The patch was titled: "staging:
comedi: ni_mio_common: protect register write overflow". Probably
should have included that one in this patch set or waited for more
time between--oh well. Any suggestion on how to handle this?
>
> --
> -=( Ian Abbott || 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
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 04/13] staging: comedi: ni_routing: Add NI signal routing info
On Tue, Sep 25, 2018 at 6:26 AM Ian Abbott 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 t
Re: [PATCH 05/13] staging: comedi: add interface to ni routing table information
How about making it selected based on COMEDI_NI_TIO? This will impact all the *mio* (except ATMIO16D) and 660x drivers. This seems to be everything that fits into the e-series, m-series, and 660x series devices for which we know the register values. It also doesn't look like anything else depends on COMEDI_NI_TIO. On Tue, Sep 25, 2018 at 4:58 AM Ian Abbott wrote: > > On 19/09/18 17:38, Spencer E. Olson wrote: > > Adds interface and associated unittests for accessing/looking-up/validating > > the new ni routing table information. > > > > Signed-off-by: Spencer E. Olson > > --- > > drivers/staging/comedi/Kconfig| 4 + > > drivers/staging/comedi/drivers/Makefile | 1 + > > drivers/staging/comedi/drivers/ni_routes.c| 523 +++ > > drivers/staging/comedi/drivers/ni_routes.h| 320 + > > drivers/staging/comedi/drivers/ni_stc.h | 4 + > > drivers/staging/comedi/drivers/tests/Makefile | 3 +- > > .../comedi/drivers/tests/ni_routes_test.c | 614 ++ > > 7 files changed, 1468 insertions(+), 1 deletion(-) > > create mode 100644 drivers/staging/comedi/drivers/ni_routes.c > > create mode 100644 drivers/staging/comedi/drivers/ni_routes.h > > create mode 100644 drivers/staging/comedi/drivers/tests/ni_routes_test.c > > > > diff --git a/drivers/staging/comedi/Kconfig b/drivers/staging/comedi/Kconfig > > index 583bce9bb18e..901503c3b279 100644 > > --- a/drivers/staging/comedi/Kconfig > > +++ b/drivers/staging/comedi/Kconfig > > @@ -1097,6 +1097,7 @@ config COMEDI_NI_TIOCMD > > depends on HAS_DMA > > select COMEDI_NI_TIO > > select COMEDI_MITE > > + select COMEDI_NI_ROUTES > > This 'select COMEDI_NI_ROUTES' may be in the wrong place. If you > disable the COMEDI_NI_PCIMIO and COMEDI_NI_660X options, but enable the > COMEDI_NI_ATMIO option (in the COMEDI_ISA_DRIVERS menu) and/or the > COMEDI_NI_MIO_CS option (in the COMEDI_PCMCIA_DRIVERS menu) then the > build breaks for the "ni_atmio" and/or "ni_mio_cs" modules. > > -- > -=( Ian Abbott || 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 de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 04/13] staging: comedi: ni_routing: Add NI signal routing info
On Mon, Oct 1, 2018 at 5:17 AM Ian Abbott wrote:
>
> On 27/09/18 20:27, Spencer E. Olson wrote:
> > See README for a thorough discussion of this content.
> >
> > Adds tables of all register values for routing various signals to various
> > terminals on National Instruments hardware. This information is directly
> > compared to and taken from register-level programming documentation and/or
> > register-level programming examples as provided by National Instruments.
> >
> > Furthermore, this information was mostly compared (favorably) to the
> > register values already used in the comedi drivers for NI hardware.
> >
> > Adds tables of valid routes for many devices. This information is not
> > consistent from device to device, nor entirely consistent within device
> > families. One additional major challenge is that this information does not
> > seem to be obtainable in any programmatic fashion, neither through the
> > proprietary NIDAQmx(-base) c-libraries, nor with register level
> > programming, _nor_ through any documentation. In fact, the only consistent
> > source of this information is through the proprietary NI-MAX software,
> > which currently only runs on Windows platforms. A further challenge is
> > that this information cannot be exported from NI-MAX, except by screenshot.
> >
> > The collection and maintenance of this information is somewhat tedious and
> > requires frequent re-examination and comparison of NI-MAX and/or the
> > NI-MHDDK documentation (register programming information) and NI-MHDDK
> > examples. Tools are added with this patch to facilitate generating CSV
> > files from the data tables. These CSV files can be used with a spreadsheet
> > program to provide better visual comparision with screenshots gathered from
> > NI-MAX. Tools are also added to regenerate the data tables from CSV
> > content--this greatly enhances updating data tables with large changes
> > (such as when adding devices).
> >
> > Signed-off-by: Spencer E. Olson
> > ---
> >
> > Changes since last submission:
> >- [PATCH v2 04/13]: Add routing information for PXIe-6535 and PXIe-6738
> > devices.
> >- [PATCH v2 04/13]: Implements Ian's suggestion to break up components
> > of new
> > ni_routing module into multiple compile units so that .c files are not
> > included from .c files.
> >- [PATCH v2 04/13]: Fixes various function prototypes and "const"
> > variable
> > declarations as per Ian's suggestions.
>
> I'm not bothered by some of the lines being slightly over 80 columns in
> the auto-generated C code (it would be a real pain for the Python
> scripts to avoid that!). And the CamelCase stuff has been explained.
>
> [snip]
> > diff --git
> > a/drivers/staging/comedi/drivers/ni_routing/tools/convert_c_to_py.c
> > b/drivers/staging/comedi/drivers/ni_routing/tools/convert_c_to_py.c
> > new file mode 100644
> > index ..5952aba91953
> > --- /dev/null
> > +++ b/drivers/staging/comedi/drivers/ni_routing/tools/convert_c_to_py.c
> > @@ -0,0 +1,159 @@
> [snip]
> > +void family_write(const struct family_route_values *rv, FILE *fp)
> > +{
> > + fprintf(fp,
> > + " \"%s\" : {\n"
> > + "# dest -> {src0:val0, src1:val1, ...}\n"
> > + , rv->family);
> > + for (unsigned int dest = NI_NAMES_BASE;
> > + dest < (NI_NAMES_BASE + NI_NUM_NAMES);
> > + ++dest) {
> > + unsigned int src = NI_NAMES_BASE;
> > +
> > + for (; src < (NI_NAMES_BASE + NI_NUM_NAMES) &&
> > + RVij(rv, B(src), B(dest)) == 0; ++src)
> > + ;
>
> checkpatch.pl complains here about "suspect code indent for conditional
> statements". That semi-colon needs indenting.
Interesting that checkpatch.pl did not complain about that for me.
Perhaps I just missed it. Fixed and will go on next submission.
>
> --
> -=( Ian Abbott || 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
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 05/13] staging: comedi: add interface to ni routing table information
On Mon, Oct 1, 2018 at 5:17 AM Ian Abbott wrote:
>
> On 27/09/18 20:27, Spencer E. Olson wrote:
> > Adds interface and associated unittests for accessing/looking-up/validating
> > the new ni routing table information.
> >
> > Signed-off-by: Spencer E. Olson
> > ---
> >
> > Changes since last submission:
> >- [PATCH v2 05/13]: Tweak Makefile to build routing info for newly added
> > hardware in updates to [PATCH v2 04/13].
> >- [PATCH v2 05/13]: Fixes placement of "select COMEDI_NI_ROUTING" as per
> > Ian's
> > suggestion.
> >- [PATCH v2 05/13]: Removes a few inline function declarations in unit
> > test.
> >
> > drivers/staging/comedi/Kconfig| 4 +
> > drivers/staging/comedi/drivers/Makefile | 27 +
> > drivers/staging/comedi/drivers/ni_routes.c| 521 +++
> > drivers/staging/comedi/drivers/ni_routes.h| 329 ++
> > drivers/staging/comedi/drivers/ni_stc.h | 4 +
> > drivers/staging/comedi/drivers/tests/Makefile | 3 +-
> > .../comedi/drivers/tests/ni_routes_test.c | 613 ++
> > 7 files changed, 1500 insertions(+), 1 deletion(-)
> > create mode 100644 drivers/staging/comedi/drivers/ni_routes.c
> > create mode 100644 drivers/staging/comedi/drivers/ni_routes.h
> > create mode 100644 drivers/staging/comedi/drivers/tests/ni_routes_test.c
> >
> [snip]
> > diff --git a/drivers/staging/comedi/drivers/ni_routes.c
> > b/drivers/staging/comedi/drivers/ni_routes.c
> > new file mode 100644
> > index ..9e999bc4f3c4
> > --- /dev/null
> > +++ b/drivers/staging/comedi/drivers/ni_routes.c
> [snip]
> > +/* BEGIN Routes search routines */
> > +static int _ni_bsearch_destcmp(const int *key, const struct ni_route_set
> > *elt)
> > +{
> > + if (*key < elt->dest)
> > + return -1;
> > + else if (*key > elt->dest)
> > + return 1;
> > + return 0;
> > +}
> > +
> > +static int _ni_bsearch_srccmp(const int *key, const int *elt)
> > +{
> > + if (*key < *elt)
> > + return -1;
> > + else if (*key > *elt)
> > + return 1;
> > + return 0;
> > +}
>
> Can those be changed to use 'const void *' parameters, like you did for
> for the 'sort' callbacks?
Yes. Done.
>
> > +
> > +/**
> > + * ni_find_route_set() - Finds the proper route set with the specified
> > + *destination.
> > + * @destination: Destination of which to search for the route set.
> > + * @valid_routes: Pointer to device routes within which to search.
> > + *
> > + * Return: NULL if no route_set is found with the specified @destination;
> > + * otherwise, a pointer to the route_set if found.
> > + */
> > +const struct ni_route_set *
> > +ni_find_route_set(const int destination,
> > + const struct ni_device_routes *valid_routes)
> > +{
> > + typedef int (*cmp_ftype)(const void *, const void *);
> > +
> > + return bsearch(&destination, valid_routes->routes,
> > +valid_routes->n_route_sets, sizeof(struct
> > ni_route_set),
> > +(cmp_ftype)_ni_bsearch_destcmp);
> > +}
> > +EXPORT_SYMBOL_GPL(ni_find_route_set);
> > +
> > +/**
> > + * ni_route_set_has_source() - Determines whether the given source is in
> > + * included given route_set.
> > + *
> > + * Return: true if found; false otherwise.
> > + */
> > +bool ni_route_set_has_source(const struct ni_route_set *routes,
> > + const int source)
> > +{
> > + typedef int (*cmp_ftype)(const void *, const void *);
> > +
> > + if (!bsearch(&source, routes->src, routes->n_src, sizeof(int),
> > + (cmp_ftype)_ni_bsearch_srccmp))
> > + return false;
> > + return true;
> > +}
> > +EXPORT_SYMBOL_GPL(ni_route_set_has_source);
>
> Then you wouldn't need those nasty '(cmp_ftype)' type casts.
Good point. Should have noticed this after the last comments.
Implemented and ready for re-submission.
>
> --
> -=( Ian Abbott || 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
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 04/13] staging: comedi: ni_routing: Add NI signal routing info
On Tue, Oct 2, 2018 at 4:17 AM Ian Abbott wrote: > > On 02/10/18 03:24, Spencer E. Olson wrote: > > See README for a thorough discussion of this content. > > > > Adds tables of all register values for routing various signals to various > > terminals on National Instruments hardware. This information is directly > > compared to and taken from register-level programming documentation and/or > > register-level programming examples as provided by National Instruments. > > > > Furthermore, this information was mostly compared (favorably) to the > > register values already used in the comedi drivers for NI hardware. > > > > Adds tables of valid routes for many devices. This information is not > > consistent from device to device, nor entirely consistent within device > > families. One additional major challenge is that this information does not > > seem to be obtainable in any programmatic fashion, neither through the > > proprietary NIDAQmx(-base) c-libraries, nor with register level > > programming, _nor_ through any documentation. In fact, the only consistent > > source of this information is through the proprietary NI-MAX software, > > which currently only runs on Windows platforms. A further challenge is > > that this information cannot be exported from NI-MAX, except by screenshot. > > > > The collection and maintenance of this information is somewhat tedious and > > requires frequent re-examination and comparison of NI-MAX and/or the > > NI-MHDDK documentation (register programming information) and NI-MHDDK > > examples. Tools are added with this patch to facilitate generating CSV > > files from the data tables. These CSV files can be used with a spreadsheet > > program to provide better visual comparision with screenshots gathered from > > NI-MAX. Tools are also added to regenerate the data tables from CSV > > content--this greatly enhances updating data tables with large changes > > (such as when adding devices). > > > > Signed-off-by: Spencer E. Olson > > --- > > > > Patch revisions: > >- [PATCH v3 04/13]: Minor update in indentation for support tool. > > > >- [PATCH v2 04/13]: Add routing information for PXIe-6535 and PXIe-6738 > > devices. > >- [PATCH v2 04/13]: Implements Ian's suggestion to break up components > > of new > > ni_routing module into multiple compile units so that .c files are not > > included from .c files. > >- [PATCH v2 04/13]: Fixes various function prototypes and "const" > > variable > > declarations as per Ian's suggestions. > > I'm not sure if this is a glitch in my email copy of the patch, but I > got a "trailing whitespace error" on one line when applying this patch > with "git am": > > Applying: staging: comedi: ni_routing: Add NI signal routing info > .git/rebase-apply/patch:8112: trailing whitespace. > NI_CtrGate(0), > warning: 1 line adds whitespace errors. > > When looking at line 8212 of > "drivers/staging/comedi/drivers/ni_routing/ni_device_routes/pci-6254.c", > the line was terminated by CRLF instead of LF (which also caused > checkpath.pl to complain about "DOS line endings"). It wasn't there in > the previous patch series, and there is no reason for it to have > appeared in this patch series, which is why I'm suspecting an email > receiving glitch at my end. > This is super strange. I checked the original patch and see no glitches. I checked the patch as posted on the mailing list archive and see no glitches. I downloaded a copy from my email using one email client and see no glitches. I try a different email client (Thunderbird) and there seemed to be a glitch at line 23739 (just some extra linefeed character). Not sure what gives here. I don't see any extra stuff at lines 8112, 8212 or any anything else near there (or any where else in that patch). The funny thing is that when I did download the one from v2 using Thunderbird, I see a random blank line inserted a few lines above 23739 instead. Each of these patch sets was sent out via git send-email. > -- > -=( Ian Abbott || 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 de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: comedi: ni_mio_common: protect register write overflow
On Tue, Oct 2, 2018 at 11:32 AM Greg Kroah-Hartman
wrote:
>
> On Wed, Sep 19, 2018 at 10:17:19AM -0600, Spencer E. Olson wrote:
> > Fixes two problems introduced as early as
> > commit 03aef4b6dc12 ("Staging: comedi: add ni_mio_common code"):
> > (1) Ensures that the last four bits of NISTC_RTSI_TRIGB_OUT_REG register is
> > not unduly overwritten on e-series devices. On e-series devices, the
> > first three of the last four bits are reserved. The last bit defines
> > the output selection of the RGOUT0 pin, otherwise known as
> > RTSI_Sub_Selection. For m-series devices, these last four bits are
> > indeed used as the output selection of the RTSI7 pin (and the
> > RTSI_Sub_Selection bit for the RGOUT0 pin is moved to the
> > RTSI_Trig_Direction register.
> > (2) Allows all 4 RTSI_BRD lines to be treated as valid sources for RTSI
> > lines.
> >
> > This patch also cleans up the ni_get_rtsi_routing command for readability.
> >
> > Fixes: 03aef4b6dc12 ("Staging: comedi: add ni_mio_common code")
> > Signed-off-by: Spencer E. Olson
> > Reviewed-by: Ian Abbott
> > Cc: stable
> > ---
> > I thought I had already submitted this patch a while ago... Whoops.
> > .../staging/comedi/drivers/ni_mio_common.c| 24 +--
> > 1 file changed, 17 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/staging/comedi/drivers/ni_mio_common.c
> > b/drivers/staging/comedi/drivers/ni_mio_common.c
> > index 4dee2fc37aed..4d0d0621780e 100644
> > --- a/drivers/staging/comedi/drivers/ni_mio_common.c
> > +++ b/drivers/staging/comedi/drivers/ni_mio_common.c
> > @@ -4980,7 +4980,10 @@ static int ni_valid_rtsi_output_source(struct
> > comedi_device *dev,
> > case NI_RTSI_OUTPUT_G_SRC0:
> > case NI_RTSI_OUTPUT_G_GATE0:
> > case NI_RTSI_OUTPUT_RGOUT0:
> > - case NI_RTSI_OUTPUT_RTSI_BRD_0:
> > + case NI_RTSI_OUTPUT_RTSI_BRD(0):
> > + case NI_RTSI_OUTPUT_RTSI_BRD(1):
> > + case NI_RTSI_OUTPUT_RTSI_BRD(2):
> > + case NI_RTSI_OUTPUT_RTSI_BRD(3):
> > return 1;
> > case NI_RTSI_OUTPUT_RTSI_OSC:
> > return (devpriv->is_m_series) ? 1 : 0;
> > @@ -5001,11 +5004,18 @@ static int ni_set_rtsi_routing(struct comedi_device
> > *dev,
> > devpriv->rtsi_trig_a_output_reg |= NISTC_RTSI_TRIG(chan, src);
> > ni_stc_writew(dev, devpriv->rtsi_trig_a_output_reg,
> > NISTC_RTSI_TRIGA_OUT_REG);
> > - } else if (chan < 8) {
> > + } else if (chan < NISTC_RTSI_TRIG_NUM_CHAN(devpriv->is_m_series)) {
> > devpriv->rtsi_trig_b_output_reg &=
> > ~NISTC_RTSI_TRIG_MASK(chan);
> > devpriv->rtsi_trig_b_output_reg |= NISTC_RTSI_TRIG(chan, src);
> > ni_stc_writew(dev, devpriv->rtsi_trig_b_output_reg,
> > NISTC_RTSI_TRIGB_OUT_REG);
> > + } else if (chan != NISTC_RTSI_TRIG_OLD_CLK_CHAN) {
> > + /* probably should never reach this, since the
> > + * ni_valid_rtsi_output_source above errors out if chan is too
> > + * high
> > + */
> > + dev_err(dev->class_dev, "%s: unknown rtsi channel\n",
> > __func__);
>
> This patch breaks the build very badly. Always test-build your patches
> at the very least :(
>
> greg k-h
I have been test building this with at least a fairly recent
staging-next rebase (rebase a week or two ago). I'll rebase again and
check this out
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: comedi: ni_mio_common: protect register write overflow
On Tue, Oct 2, 2018 at 6:16 PM Spencer Olson wrote:
>
> On Tue, Oct 2, 2018 at 11:32 AM Greg Kroah-Hartman
> wrote:
> >
> > On Wed, Sep 19, 2018 at 10:17:19AM -0600, Spencer E. Olson wrote:
> > > Fixes two problems introduced as early as
> > > commit 03aef4b6dc12 ("Staging: comedi: add ni_mio_common code"):
> > > (1) Ensures that the last four bits of NISTC_RTSI_TRIGB_OUT_REG register
> > > is
> > > not unduly overwritten on e-series devices. On e-series devices, the
> > > first three of the last four bits are reserved. The last bit defines
> > > the output selection of the RGOUT0 pin, otherwise known as
> > > RTSI_Sub_Selection. For m-series devices, these last four bits are
> > > indeed used as the output selection of the RTSI7 pin (and the
> > > RTSI_Sub_Selection bit for the RGOUT0 pin is moved to the
> > > RTSI_Trig_Direction register.
> > > (2) Allows all 4 RTSI_BRD lines to be treated as valid sources for RTSI
> > > lines.
> > >
> > > This patch also cleans up the ni_get_rtsi_routing command for readability.
> > >
> > > Fixes: 03aef4b6dc12 ("Staging: comedi: add ni_mio_common code")
> > > Signed-off-by: Spencer E. Olson
> > > Reviewed-by: Ian Abbott
> > > Cc: stable
> > > ---
> > > I thought I had already submitted this patch a while ago... Whoops.
> > > .../staging/comedi/drivers/ni_mio_common.c| 24 +--
> > > 1 file changed, 17 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/drivers/staging/comedi/drivers/ni_mio_common.c
> > > b/drivers/staging/comedi/drivers/ni_mio_common.c
> > > index 4dee2fc37aed..4d0d0621780e 100644
> > > --- a/drivers/staging/comedi/drivers/ni_mio_common.c
> > > +++ b/drivers/staging/comedi/drivers/ni_mio_common.c
> > > @@ -4980,7 +4980,10 @@ static int ni_valid_rtsi_output_source(struct
> > > comedi_device *dev,
> > > case NI_RTSI_OUTPUT_G_SRC0:
> > > case NI_RTSI_OUTPUT_G_GATE0:
> > > case NI_RTSI_OUTPUT_RGOUT0:
> > > - case NI_RTSI_OUTPUT_RTSI_BRD_0:
> > > + case NI_RTSI_OUTPUT_RTSI_BRD(0):
> > > + case NI_RTSI_OUTPUT_RTSI_BRD(1):
> > > + case NI_RTSI_OUTPUT_RTSI_BRD(2):
> > > + case NI_RTSI_OUTPUT_RTSI_BRD(3):
> > > return 1;
> > > case NI_RTSI_OUTPUT_RTSI_OSC:
> > > return (devpriv->is_m_series) ? 1 : 0;
> > > @@ -5001,11 +5004,18 @@ static int ni_set_rtsi_routing(struct
> > > comedi_device *dev,
> > > devpriv->rtsi_trig_a_output_reg |= NISTC_RTSI_TRIG(chan,
> > > src);
> > > ni_stc_writew(dev, devpriv->rtsi_trig_a_output_reg,
> > > NISTC_RTSI_TRIGA_OUT_REG);
> > > - } else if (chan < 8) {
> > > + } else if (chan < NISTC_RTSI_TRIG_NUM_CHAN(devpriv->is_m_series)) {
> > > devpriv->rtsi_trig_b_output_reg &=
> > > ~NISTC_RTSI_TRIG_MASK(chan);
> > > devpriv->rtsi_trig_b_output_reg |= NISTC_RTSI_TRIG(chan,
> > > src);
> > > ni_stc_writew(dev, devpriv->rtsi_trig_b_output_reg,
> > > NISTC_RTSI_TRIGB_OUT_REG);
> > > + } else if (chan != NISTC_RTSI_TRIG_OLD_CLK_CHAN) {
> > > + /* probably should never reach this, since the
> > > + * ni_valid_rtsi_output_source above errors out if chan is
> > > too
> > > + * high
> > > + */
> > > + dev_err(dev->class_dev, "%s: unknown rtsi channel\n",
> > > __func__);
> >
> > This patch breaks the build very badly. Always test-build your patches
> > at the very least :(
> >
> > greg k-h
>
> I have been test building this with at least a fairly recent
> staging-next rebase (rebase a week or two ago). I'll rebase again and
> check this out
So the problem had been that I had been compiling the entire time with
my other patch set that I recently have just submitted. When I split
this patch off from that patch set, I had neglected to compile with it
by itsself. The issue was a forgotten "{" at the beginning of the
last if statement.
Should I resubmit this patch and the entire patch set from earlier
today, each separately?
The patch set from today titled "device-global identifiers and routes
introduced" _does_ depend on this patch that was missing the
brace--this is indicated in the patch notes as requested by Ian.
I did just check to make sure that I had not made the same mistake on
the other patch set submitted earlier that was titled: "Add facility
to directly query subdevice timing". That patch set is fine as is and
did not depend on any of the other patches I had been working on.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: comedi: ni_mio_common: scale ao INSN_CONFIG_GET_CMD_TIMING_CONSTRAINTS
On Wed, Oct 24, 2018 at 8:18 AM Dan Carpenter wrote:
>
> On Wed, Oct 24, 2018 at 07:59:45AM -0600, Spencer E. Olson wrote:
> > Changes implementation of INSN_CONFIG_GET_CMD_TIMING_CONSTRAINTS for
> > ni_mio devices to scale the result by the number of channels being used.
>
> I really can't understand this statement at all. What changes the
> implementation?
>
> > The user is already required to indicate which channels (and how many
> > obviously) are intended to be used. There is no point of not using this
> > information--the analog input cards already similarly scale the timing
> > results based on the number of channels.
>
> This sounds like it's just an optimization but I think this patch is a
> behavior change or a bug fix, right? It's not totally clear to me from
> the patch description what the user visible effect of this patch is.
> Could you spell that out a little bit more clearly and resend the
> patch? (It's also possible that I just don't understand Comedi well
> enough to understand the patch description).
>
> >
> > Signed-off-by: Spencer E. Olson
> > ---
> > This patch is made in reference to the last set of patches adding the
> > timing
> > constraint facility in pci_mio_common
> > (51fd3673838396844f15de0e906be5333bfbbc8d).
>
>
> I feel like we should be using the Fixes tag for this. Like so:
>
> Fixes: 51fd36738383 ("staging: comedi: ni_mio_common: implement
> INSN_CONFIG_GET_CMD_TIMING_CONSTRAINTS")
> Signed-off-by: Your
I did consider submitting it as a fix instead, but I couldn't actually
remember what my intentions were when I wrote the original code from
the earlier patch. On the other hand, after reading more on the
interface documentation and user code that I have written (not yet
submitted to the comedi community), I think you are right and that
this should be submitted as a fix. Thanks
>
> regards,
> dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: comedi: clarify/unify macros for NI macro-defined terminals
On Wed, Oct 24, 2018 at 10:39 AM Ian Abbott wrote:
>
> On 24/10/18 15:33, Spencer E. Olson wrote:
> > Uses a single macro to define multiple macros that represent a series of
> > terminals for NI devices. This patch also redefines NI_MAX_COUNTERS as the
> > maximum number of counters possible on NI devices (instead of the maximum
> > index of the counters). This was a little confusing and caused a bug in
> > commit 347e244884c3b ("staging: comedi: tio: implement global tio/ctr
> > routing")
> > when setting/reading registers for counter terminals.
> >
> > Fixes: 347e244884c3b ("staging: comedi: tio: implement global tio/ctr
> > routing")
> > Signed-off-by: Spencer E. Olson
> > ---
> > drivers/staging/comedi/comedi.h | 39 ++---
> > 1 file changed, 21 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/staging/comedi/comedi.h
> > b/drivers/staging/comedi/comedi.h
> > index e90b17775284..09a940066c0e 100644
> > --- a/drivers/staging/comedi/comedi.h
> > +++ b/drivers/staging/comedi/comedi.h
> > @@ -1005,35 +1005,38 @@ enum i8254_mode {
> >* and INSN_DEVICE_CONFIG_GET_ROUTES.
> >*/
> > #define NI_NAMES_BASE 0x8000u
> > +
> > +#define _TERM_N(base, n, x) ((base) + ((x) & ((n) - 1)))
> > +
> > /*
> >* not necessarily all allowed 64 PFIs are valid--certainly not for all
> > devices
> >*/
> > -#define NI_PFI(x)(NI_NAMES_BASE+ ((x) & 0x3f))
> > +#define NI_PFI(x)_TERM_N(NI_NAMES_BASE, 64, x)
> > /* 8 trigger lines by standard, Some devices cannot talk to all eight. */
> > -#define TRIGGER_LINE(x) (NI_PFI(-1) + 1 + ((x) & 0x7))
> > +#define TRIGGER_LINE(x) _TERM_N(NI_PFI(-1) + 1, 8, x)
> > /* 4 RTSI shared MUXes to route signals to/from TRIGGER_LINES on NI
> > hardware */
> > -#define NI_RTSI_BRD(x) (TRIGGER_LINE(-1) + 1 + ((x) & 0x3))
> > +#define NI_RTSI_BRD(x) _TERM_N(TRIGGER_LINE(-1) + 1, 4, x)
> >
> > /* *** Counter/timer names : 8 counters max *** */
> > -#define NI_COUNTER_NAMES_BASE (NI_RTSI_BRD(-1) + 1)
> > -#define NI_MAX_COUNTERS 7
> > -#define NI_CtrSource(x) (NI_COUNTER_NAMES_BASE + ((x) &
> > NI_MAX_COUNTERS))
> > +#define NI_MAX_COUNTERS 8
> > +#define NI_COUNTER_NAMES_BASE(NI_RTSI_BRD(-1) + 1)
> > +#define NI_CtrSource(x)_TERM_N(NI_COUNTER_NAMES_BASE,
> > NI_MAX_COUNTERS, x)
> > /* Gate, Aux, A,B,Z are all treated, at times as gates */
> > -#define NI_GATES_NAMES_BASE(NI_CtrSource(-1) + 1)
> > -#define NI_CtrGate(x) (NI_GATES_NAMES_BASE + ((x) &
> > NI_MAX_COUNTERS))
> > -#define NI_CtrAux(x)(NI_CtrGate(-1) + 1 + ((x) &
> > NI_MAX_COUNTERS))
> > -#define NI_CtrA(x) (NI_CtrAux(-1)+ 1 + ((x) &
> > NI_MAX_COUNTERS))
> > -#define NI_CtrB(x) (NI_CtrA(-1) + 1 + ((x) &
> > NI_MAX_COUNTERS))
> > -#define NI_CtrZ(x) (NI_CtrB(-1) + 1 + ((x) &
> > NI_MAX_COUNTERS))
> > -#define NI_GATES_NAMES_MAX NI_CtrZ(-1)
> > -#define NI_CtrArmStartTrigger(x) (NI_CtrZ(-1)+ 1 + ((x) &
> > NI_MAX_COUNTERS))
> > +#define NI_GATES_NAMES_BASE (NI_CtrSource(-1) + 1)
> > +#define NI_CtrGate(x)_TERM_N(NI_GATES_NAMES_BASE,
> > NI_MAX_COUNTERS, x)
> > +#define NI_CtrAux(x) _TERM_N(NI_CtrGate(-1) + 1, NI_MAX_COUNTERS,
> > x)
> > +#define NI_CtrA(x) _TERM_N(NI_CtrAux(-1) + 1, NI_MAX_COUNTERS,
> > x)
> > +#define NI_CtrB(x) _TERM_N(NI_CtrA(-1) + 1, NI_MAX_COUNTERS,
> > x)
> > +#define NI_CtrZ(x) _TERM_N(NI_CtrB(-1) + 1, NI_MAX_COUNTERS,
> > x)
> > +#define NI_GATES_NAMES_MAX NI_CtrZ(-1)
> > +#define NI_CtrArmStartTrigger(x) _TERM_N(NI_CtrZ(-1)+ 1,
> > NI_MAX_COUNTERS, x)
> > #define NI_CtrInternalOutput(x) \
> > - (NI_CtrArmStartTrigger(-1) + 1 + ((x) &
> > NI_MAX_COUNTERS))
> > + _TERM_N(NI_CtrArmStartTrigger(-1) + 1, NI_MAX_COUNTERS,
> > x)
> > /** external pin(s) labeled conveniently as CtrOut. */
> > -#define NI_CtrOut(x) (NI_CtrInternalOutput(-1) + 1 + ((x) &
> > NI_MAX_COUNTERS))
> > +#define NI_CtrOut(x) _TERM_N(NI_CtrInternalOutput(-1) + 1,
> > NI_MAX_COUNTERS, x)
> > /** For Buffered sampling of ctr -- x series capability. */
> > -#define NI_CtrSampleClock(x) (NI_CtrOut(-1) + 1 + ((x) &
> > NI_MAX_COUNTERS))
> > -#define NI_COUNTER_NAMES_MAX NI_CtrSampleClock(-1)
> > +#define NI_CtrSampleClock(x) _TERM_N(NI_CtrOut(-1) + 1, NI_MAX_COUNTERS,
> > x)
> > +#define NI_COUNTER_NAMES_MAX NI_CtrSampleClock(-1)
> >
> > enum ni_common_signal_names {
> > /* PXI_Star: this is a non-NI-specific signal */
> >
>
> It's no more ugly than the original, although I pity the fool who has to
> make sense of the preprocessor output!
Unfortunately, I do agree. Oh well.
>
> Reviewed-by: Ian Abbott
>
> --
> -=( Ian Abbott || Web: www.mev.co.uk )=-
> -=( MEV Ltd. is a company registered in England & Wales. )=
Re: [PATCH] staging: comedi: change do_insn*_ioctl to allow more samples
On Tue, Oct 30, 2018 at 6:21 AM Ian Abbott wrote:
>
> On 25/10/18 02:36, Spencer E. Olson wrote:
> > Changes do_insn*_ioctl functions to allow for data lengths for each
> > comedi_insn of up to 2^16. This patch also changes these functions to only
> > allocate as much memory as is necessary for each comedi_insn, rather than
> > allocating a fixed-sized scratch space.
> >
> > In testing some user-space code for the new INSN_DEVICE_CONFIG_GET_ROUTES
> > facility with some newer hardware, I discovered that do_insn_ioctl and
> > do_insnlist_ioctl limited the amount of data that can be passed into the
> > kernel for insn's to a length of 256. For some newer hardware, the number
> > of routes can be greater than 1000. Working around the old limits (256)
> > would complicate the user-space/kernel interaction.
> >
> > The new upper limit is reasonable with current memory available and does
> > not otherwise impact the memory footprint for any current or otherwise
> > typical configuration.
> >
> > Signed-off-by: Spencer E. Olson
> > ---
> > drivers/staging/comedi/comedi_fops.c | 37 +---
> > 1 file changed, 22 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/staging/comedi/comedi_fops.c
> > b/drivers/staging/comedi/comedi_fops.c
> > index c1c6b2b4ab91..a163ec2df872 100644
> > --- a/drivers/staging/comedi/comedi_fops.c
> > +++ b/drivers/staging/comedi/comedi_fops.c
> > @@ -1500,7 +1500,7 @@ static int parse_insn(struct comedi_device *dev,
> > struct comedi_insn *insn,
> >* data (for reads) to insns[].data pointers
> >*/
> > /* arbitrary limits */
> > -#define MAX_SAMPLES 256
> > +#define MAX_SAMPLES 65536
> > static int do_insnlist_ioctl(struct comedi_device *dev,
> >struct comedi_insnlist __user *arg, void *file)
> > {
> > @@ -1513,12 +1513,6 @@ static int do_insnlist_ioctl(struct comedi_device
> > *dev,
> > if (copy_from_user(&insnlist, arg, sizeof(insnlist)))
> > return -EFAULT;
> >
> > - data = kmalloc_array(MAX_SAMPLES, sizeof(unsigned int), GFP_KERNEL);
> > - if (!data) {
> > - ret = -ENOMEM;
> > - goto error;
> > - }
> > -
> > insns = kcalloc(insnlist.n_insns, sizeof(*insns), GFP_KERNEL);
> > if (!insns) {
> > ret = -ENOMEM;
> > @@ -1539,6 +1533,14 @@ static int do_insnlist_ioctl(struct comedi_device
> > *dev,
> > ret = -EINVAL;
> > goto error;
> > }
> > +
> > + data = kmalloc_array(insns[i].n, sizeof(unsigned int),
> > + GFP_KERNEL);
> > + if (!data) {
> > + ret = -ENOMEM;
> > + goto error;
> > + }
> > +
> > if (insns[i].insn & INSN_MASK_WRITE) {
> > if (copy_from_user(data, insns[i].data,
> > insns[i].n * sizeof(unsigned
> > int))) {
> > @@ -1560,6 +1562,11 @@ static int do_insnlist_ioctl(struct comedi_device
> > *dev,
> > goto error;
> > }
> > }
> > +
> > + /* free up the single-instruction data memory */
> > + kfree(data);
> > + data = NULL;
> > +
> > if (need_resched())
> > schedule();
> > }
> > @@ -1594,20 +1601,20 @@ static int do_insn_ioctl(struct comedi_device *dev,
> > unsigned int *data = NULL;
> > int ret = 0;
> >
> > - data = kmalloc_array(MAX_SAMPLES, sizeof(unsigned int), GFP_KERNEL);
> > - if (!data) {
> > - ret = -ENOMEM;
> > - goto error;
> > - }
> > -
> > if (copy_from_user(&insn, arg, sizeof(insn))) {
> > - ret = -EFAULT;
> > - goto error;
> > + return -EFAULT;
> > }
> >
> > /* This is where the behavior of insn and insnlist deviate. */
> > if (insn.n > MAX_SAMPLES)
> > insn.n = MAX_SAMPLES;
> > +
> > + data = kmalloc_array(insn.n, sizeof(unsigned int), GFP_KERNEL);
> > + if (!data) {
> > + ret = -ENOMEM;
> > + goto error;
> > + }
> > +
> > if (insn.insn & INSN_MASK_WRITE) {
> > if (copy_from_user(data,
> > insn.data,
> >
>
> There are still some broken drivers that do not check insn->n and assume
> the data has some minimum length, so we will need to make the data array
> some minimum length (say 16 * sizeof(unsigned int)) until those drivers
> have been fixed.
>
I don't think that would be much of a problem, since 16*sizeof(uint)
is pretty negligible. How certain are you that we would not have to
make that higher, as the previous default was a length of
256*sizeof(uint)? It'll take me a day or two to resubmit (have other
things pressing on my time).
> For example (there might be other places) ...
>
> In cb_pcidas64.c:
>
> *
Re: [PATCH] staging: comedi: change do_insn*_ioctl to allow more samples
On Wed, Oct 31, 2018 at 4:49 AM Ian Abbott wrote:
>
> On 25/10/18 02:36, Spencer E. Olson wrote:
> > Changes do_insn*_ioctl functions to allow for data lengths for each
> > comedi_insn of up to 2^16. This patch also changes these functions to only
> > allocate as much memory as is necessary for each comedi_insn, rather than
> > allocating a fixed-sized scratch space.
> >
> > In testing some user-space code for the new INSN_DEVICE_CONFIG_GET_ROUTES
> > facility with some newer hardware, I discovered that do_insn_ioctl and
> > do_insnlist_ioctl limited the amount of data that can be passed into the
> > kernel for insn's to a length of 256. For some newer hardware, the number
> > of routes can be greater than 1000. Working around the old limits (256)
> > would complicate the user-space/kernel interaction.
> >
> > The new upper limit is reasonable with current memory available and does
> > not otherwise impact the memory footprint for any current or otherwise
> > typical configuration.
> >
> > Signed-off-by: Spencer E. Olson
> > ---
> > drivers/staging/comedi/comedi_fops.c | 37 +---
> > 1 file changed, 22 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/staging/comedi/comedi_fops.c
> > b/drivers/staging/comedi/comedi_fops.c
> > index c1c6b2b4ab91..a163ec2df872 100644
> > --- a/drivers/staging/comedi/comedi_fops.c
> > +++ b/drivers/staging/comedi/comedi_fops.c
> > @@ -1500,7 +1500,7 @@ static int parse_insn(struct comedi_device *dev,
> > struct comedi_insn *insn,
> >* data (for reads) to insns[].data pointers
> >*/
> > /* arbitrary limits */
> > -#define MAX_SAMPLES 256
> > +#define MAX_SAMPLES 65536
> > static int do_insnlist_ioctl(struct comedi_device *dev,
> >struct comedi_insnlist __user *arg, void *file)
> > {
> > @@ -1513,12 +1513,6 @@ static int do_insnlist_ioctl(struct comedi_device
> > *dev,
> > if (copy_from_user(&insnlist, arg, sizeof(insnlist)))
> > return -EFAULT;
> >
> > - data = kmalloc_array(MAX_SAMPLES, sizeof(unsigned int), GFP_KERNEL);
> > - if (!data) {
> > - ret = -ENOMEM;
> > - goto error;
> > - }
> > -
> > insns = kcalloc(insnlist.n_insns, sizeof(*insns), GFP_KERNEL);
> > if (!insns) {
> > ret = -ENOMEM;
> > @@ -1539,6 +1533,14 @@ static int do_insnlist_ioctl(struct comedi_device
> > *dev,
> > ret = -EINVAL;
> > goto error;
> > }
> > +
> > + data = kmalloc_array(insns[i].n, sizeof(unsigned int),
> > + GFP_KERNEL);
> > + if (!data) {
> > + ret = -ENOMEM;
> > + goto error;
> > + }
> > +
>
> Something you could do here is work out which insn->n in the instruction
> list is the largest, and allocate the 'data' once outside the
> instruction list handling loop instead of allocating it inside the loop.
I just realized that I left this unfinished. How much headway did you
already make with fixing the various other drivers that had previously
not checked length of insn->n?
(I'll try your last suggestion and get that submitted in the next day or so.)
>
> --
> -=( Ian Abbott || 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
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2] staging: comedi: change do_insn*_ioctl to allow more samples
On Tue, Dec 4, 2018 at 12:08 PM Spencer E. Olson wrote:
>
> Changes do_insn*_ioctl functions to allow for data lengths for each
> comedi_insn of up to 2^16. This patch also changes these functions to only
> allocate as much memory as is necessary for each comedi_insn, rather than
> allocating a fixed-sized scratch space.
>
> In testing some user-space code for the new INSN_DEVICE_CONFIG_GET_ROUTES
> facility with some newer hardware, I discovered that do_insn_ioctl and
> do_insnlist_ioctl limited the amount of data that can be passed into the
> kernel for insn's to a length of 256. For some newer hardware, the number
> of routes can be greater than 1000. Working around the old limits (256)
> would complicate the user-space/kernel interaction.
>
> The new upper limit is reasonable with current memory available and does
> not otherwise impact the memory footprint for any current or otherwise
> typical configuration.
>
> Signed-off-by: Spencer E. Olson
> ---
> Implements Ian's suggestions to:
> 1) Provide a minimum amount of space to allocate (16*sizeof(uint)) to protect
>drivers that do not (yet) check the size of the instruction data (Ian has
>submitted several patches fixing this for other drivers already). In case
>insn.n does not get set, this minimum amount also avoids trying to allocate
>zero-length data.
> 2) Allocate the maximum required space needed for any of the instructions in
> an
>instruction list before executing the list of instructions (this, rather
> than
>allocating and freeing memory separately while iterating through the
>instruction list and executing each instruction).
>
> drivers/staging/comedi/comedi_fops.c | 48 ++--
> 1 file changed, 31 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/staging/comedi/comedi_fops.c
> b/drivers/staging/comedi/comedi_fops.c
> index ceb6ba5dd57c..5d2fcbfe02af 100644
> --- a/drivers/staging/comedi/comedi_fops.c
> +++ b/drivers/staging/comedi/comedi_fops.c
> @@ -1501,25 +1501,21 @@ static int parse_insn(struct comedi_device *dev,
> struct comedi_insn *insn,
> * data (for reads) to insns[].data pointers
> */
> /* arbitrary limits */
> -#define MAX_SAMPLES 256
> +#define MIN_SAMPLES 16
> +#define MAX_SAMPLES 65536
> static int do_insnlist_ioctl(struct comedi_device *dev,
> struct comedi_insnlist __user *arg, void *file)
> {
> struct comedi_insnlist insnlist;
> struct comedi_insn *insns = NULL;
> unsigned int *data = NULL;
> + unsigned int max_n_data_required = MIN_SAMPLES;
> int i = 0;
> int ret = 0;
>
> if (copy_from_user(&insnlist, arg, sizeof(insnlist)))
> return -EFAULT;
>
> - data = kmalloc_array(MAX_SAMPLES, sizeof(unsigned int), GFP_KERNEL);
> - if (!data) {
> - ret = -ENOMEM;
> - goto error;
> - }
> -
> insns = kcalloc(insnlist.n_insns, sizeof(*insns), GFP_KERNEL);
> if (!insns) {
> ret = -ENOMEM;
> @@ -1533,13 +1529,26 @@ static int do_insnlist_ioctl(struct comedi_device
> *dev,
> goto error;
> }
>
> - for (i = 0; i < insnlist.n_insns; i++) {
> + /* Determine maximum memory needed for all instructions. */
> + for (i = 0; i < insnlist.n_insns; ++i) {
> if (insns[i].n > MAX_SAMPLES) {
> dev_dbg(dev->class_dev,
> "number of samples too large\n");
> ret = -EINVAL;
> goto error;
> }
> + max_n_data_required = max(max_n_data_required, insns[i].n);
> + }
I realized just now that this patch does change behavior just bit:
the complaint, error, and exit are done _before_ any instruction is
executed, rather than after the prior instructions in the list are
executed. I argue this is actually a better behavior than partially
executing an instruction list, especially since this pre-inspection
could have already easily been done before.
> +
> + /* Allocate scratch space for all instruction data. */
> + data = kmalloc_array(max_n_data_required, sizeof(unsigned int),
> +GFP_KERNEL);
> + if (!data) {
> + ret = -ENOMEM;
> + goto error;
> + }
> +
> + for (i = 0; i < insnlist.n_insns; ++i) {
> if (insns[i].insn & INSN_MASK_WRITE) {
> if (copy_from_user(data, insns[i].data,
>insns[i].n * sizeof(unsigned
> int))) {
> @@ -1593,22 +1602,27 @@ static int do_insn_ioctl(struct comedi_device *dev,
> {
> struct comedi_insn insn;
> unsigned int *data = NULL;
> + unsigned int n_data = MIN_SAMPLES;
> int ret = 0;
>
> - data = kmalloc_array(MAX_SAMPLES, sizeof(unsigned int), GFP_KERNEL);
> - if (!data) {
> -
Re: [PATCH 1/2] comedi/ni_pcidio: remove unused defines
On Tue, Dec 11, 2018 at 3:50 AM Alexander Schroth wrote: > > Define-statements, which are not used within the file, are being removed > as they add clutter to the code. > Because the file is not being included from anywhere else, this has no > negative side-effects. This is somewhat coincidental, but I just started reading through the meager DMA transfer documentation and the comedi interpretation/reverse-engineering of it. There is a bug (with a hysteresis) in the continuous-generation mode that I added a while ago and I am going to slowly try to track this down. This patch causes me some concern in losing the knowledge that was gained at sometime in the past pertaining to NI's DMA controller(s). The majority of these #defines are for the DMA controller. I would highly suggest we do not remove these #defines as it would make the debugging of this hardware more challenging, because one would have to start from scratch again with the very poor documentation that does exist. > > Signed-off-by: Alexander Schroth > Signed-off-by: Marco Ammon > --- > drivers/staging/comedi/drivers/ni_pcidio.c | 56 -- > 1 file changed, 56 deletions(-) > > diff --git a/drivers/staging/comedi/drivers/ni_pcidio.c > b/drivers/staging/comedi/drivers/ni_pcidio.c > index b9a0dc6eac44..91ffcfb45f55 100644 > --- a/drivers/staging/comedi/drivers/ni_pcidio.c > +++ b/drivers/staging/comedi/drivers/ni_pcidio.c > @@ -49,22 +49,12 @@ > > /* defines for the PCI-DIO-32HS */ > > -#define Window_Address 4 /* W */ > #define Interrupt_And_Window_Status4 /* R */ > -#define IntStatus1 BIT(0) > -#define IntStatus2 BIT(1) > -#define WindowAddressStatus_mask 0x7c > > #define Master_DMA_And_Interrupt_Control 5 /* W */ > -#define InterruptLine(x) ((x) & 3) > -#define OpenIntBIT(2) > -#define Group_Status 5 /* R */ > #define DataLeft BIT(0) > -#define ReqBIT(2) > -#define StopTrig BIT(3) > > #define Group_1_Flags 6 /* R */ > -#define Group_2_Flags 7 /* R */ > #define TransferReady BIT(0) > #define CountExpired BIT(1) > #define Waited BIT(5) > @@ -75,43 +65,22 @@ >/* #define Paused */ > > #define Group_1_First_Clear6 /* W */ > -#define Group_2_First_Clear7 /* W */ > #define ClearWaitedBIT(3) > #define ClearPrimaryTC BIT(4) > #define ClearSecondaryTC BIT(5) > -#define DMAReset BIT(6) > -#define FIFOReset BIT(7) > -#define ClearAll 0xf8 > > #define Group_1_FIFO 8 /* W */ > -#define Group_2_FIFO 12 /* W */ > > #define Transfer_Count 20 > -#define Chip_ID_D 24 > -#define Chip_ID_I 25 > -#define Chip_ID_O 26 > #define Chip_Version 27 > #define Port_IO(x) (28 + (x)) > #define Port_Pin_Directions(x) (32 + (x)) > #define Port_Pin_Mask(x) (36 + (x)) > -#define Port_Pin_Polarities(x) (40 + (x)) > - > -#define Master_Clock_Routing 45 > -#define RTSIClocking(x)(((x) & 3) << 4) > > #define Group_1_Second_Clear 46 /* W */ > -#define Group_2_Second_Clear 47 /* W */ > #define ClearExpired BIT(0) > > -#define Port_Pattern(x)(48 + (x)) > - > #define Data_Path 64 > -#define FIFOEnableABIT(0) > -#define FIFOEnableBBIT(1) > -#define FIFOEnableCBIT(2) > -#define FIFOEnableDBIT(3) > -#define Funneling(x) (((x) & 3) << 4) > -#define GroupDirection BIT(7) > > #define Protocol_Register_165 > #define OpMode Protocol_Register_1 > @@ -120,8 +89,6 @@ > > #define Protocol_Register_266 > #define ClockReg Protocol_Register_2 > -#define ClockLine(x) (((x) & 3) << 5) > -#define InvertStopTrig BIT(7) > #define DataLatching(x) (((x) & 3) << 5) > > #define Protocol_Register_367 > @@ -132,27 +99,15 @@ > > #define Protocol_Register_470 > #define ReqReg Protocol_Register_4 > -#define ReqConditioning(x) (((x) & 7) << 3) > > #define Protocol_Register_571 > #define BlockMode Protocol_Register_5 > > -#define FIFO_Control 72 > -#define ReadyLevel(x) ((x
Re: [PATCH 2/2] comedi/ni_pcidio: make all defines uppercase
On Tue, Dec 11, 2018 at 3:50 AM Alexander Schroth wrote: > > According to the Linux coding guidelines, defines should be written > in uppercase. This patch converts all define-statements in the > ni_pcidio.c file to uppercase, thus matching the coding style of the > kernel. > > Signed-off-by: Alexander Schroth > Signed-off-by: Marco Ammon > --- > drivers/staging/comedi/drivers/ni_pcidio.c | 306 +++-- > 1 file changed, 154 insertions(+), 152 deletions(-) > > diff --git a/drivers/staging/comedi/drivers/ni_pcidio.c > b/drivers/staging/comedi/drivers/ni_pcidio.c > index 91ffcfb45f55..350b32643a50 100644 > --- a/drivers/staging/comedi/drivers/ni_pcidio.c > +++ b/drivers/staging/comedi/drivers/ni_pcidio.c > @@ -49,71 +49,72 @@ > > /* defines for the PCI-DIO-32HS */ > > -#define Interrupt_And_Window_Status4 /* R */ > +#define INTERRUPT_AND_WINDOW_STATUS4 /* R */ > > -#define Master_DMA_And_Interrupt_Control 5 /* W */ > -#define DataLeft BIT(0) > +#define MASTER_DMA_AND_INTERRUPT_CONTROL 5 /* W */ > +#define DATA_LEFT BIT(0) > > -#define Group_1_Flags 6 /* R */ > -#define TransferReady BIT(0) > -#define CountExpired BIT(1) > -#define Waited BIT(5) > -#define PrimaryTC BIT(6) > -#define SecondaryTCBIT(7) > +#define GROUP_1_FLAGS 6 /* R */ > +#define TRANSFER_READY BIT(0) > +#define COUNT_EXPIRED BIT(1) > +#define WAITED BIT(5) > +#define PRIMARY_TC BIT(6) > +#define SECONDARY_TC BIT(7) >/* #define SerialRose */ >/* #define ReqRose */ >/* #define Paused */ > > -#define Group_1_First_Clear6 /* W */ > -#define ClearWaitedBIT(3) > -#define ClearPrimaryTC BIT(4) > -#define ClearSecondaryTC BIT(5) > +#define GROUP_1_FIRST_CLEAR6 /* W */ > +#define CLEAR_WAITED BIT(3) > +#define CLEAR_PRIMARY_TC BIT(4) > +#define CLEAR_SECONDARY_TC BIT(5) > > -#define Group_1_FIFO 8 /* W */ > +#define GROUP_1_FIFO 8 /* W */ > > -#define Transfer_Count 20 > -#define Chip_Version 27 > -#define Port_IO(x) (28 + (x)) > -#define Port_Pin_Directions(x) (32 + (x)) > -#define Port_Pin_Mask(x) (36 + (x)) > +#define TRANSFER_COUNT 20 > +#define CHIP_VERSION 27 > +#define PORT_IO(x) (28 + (x)) > +#define PORT_PIN_DIRECTIONS(x) (32 + (x)) > +#define PORT_PIN_MASK(x) (36 + (x)) > > -#define Group_1_Second_Clear 46 /* W */ > -#define ClearExpired BIT(0) > +#define GROUP_1_SECOND_CLEAR 46 /* W */ > +#define CLEAR_EXPIRED BIT(0) > > -#define Data_Path 64 > +#define DATA_PATH 64 > > -#define Protocol_Register_165 > -#define OpMode Protocol_Register_1 > -#define RunMode(x) ((x) & 7) > -#define Numbered BIT(3) > +#define PROTOCOL_REGISTER_165 > +#define OP_MODEPROTOCOL_REGISTER_1 > +#define RUN_MODE(x)((x) & 7) > +#define NUMBERED BIT(3) > > -#define Protocol_Register_266 > -#define ClockReg Protocol_Register_2 > -#define DataLatching(x) (((x) & 3) << 5) > +#define PROTOCOL_REGISTER_266 > +#define CLOCK_REG PROTOCOL_REGISTER_2 > +#define DATA_LATCHING(x) (((x) & 3) << 5) > > -#define Protocol_Register_367 > -#define Sequence Protocol_Register_3 > +#define PROTOCOL_REGISTER_367 > +#define SEQUENCE PROTOCOL_REGISTER_3 > > -#define Protocol_Register_14 68 /* 16 bit */ > -#define ClockSpeed Protocol_Register_14 > +#define PROTOCOL_REGISTER_14 68 /* 16 bit */ > +#define CLOCK_SPEEDPROTOCOL_REGISTER_14 > > -#define Protocol_Register_470 > -#define ReqReg Protocol_Register_4 > +#define PROTOCOL_REGISTER_470 > +#define REQ_REGPROTOCOL_REGISTER_4 > > -#define Protocol_Register_571 > -#define BlockMode Protocol_Register_5 > +#define PROTOCOL_REGISTER_571 > +#define BLOCK_MODE PROTOCOL_REGISTER_5 > > -#define Protocol_Register_673 > -#define LinePolarities
