Dear All,
This email is mainly to give people an idea of current progress towards
a new
subsystem as discussed in the thread starting with
http://lkml.org/lkml/2008/5/20/135
Sorry for the mass list bombardment, but clearly some elements of this
discussion
will end up in various different territories.
Some elements of a prototype subsystem are in place. It draws very heavily
on parts of the input and hwmon subsystems diverging only where necessary.
The only test driver currently integrated is for an ST LIS3L02DQ
accelerometer
which has more than a few quirks to make it tricky to handle (and some what
sketchy documentation.) More chips will follow over next week or so but
hopefully the driver for this chip gives enough of an idea of how I envision
the system working to encourage discussion / advice.
Note that I haven't dealt with anywhere near all the possible locking issues
etc and am well aware that this needs to be done. Other cleanups that will
need to be done include working out the layout in sysfs to make it more
intuitive. Also sorry for the somewhat rough and ready nature of the
attached
patch (against 2.6.26-rc4)
Ring buffer design is a large part of the attached patch. I'm not sure if
I am going about this the right way. Basically, we need ring buffers with
almost no write latency but can waste plenty of time reading from them
(in general case - we do however want reading the last available value to be
fast). What is there works, but probably has at least a few nasty corner
cases that I haven't prevented.
Interfaces (these are per device) - at somepoint a procfs interface similar
to that used in the input subsystem would make device querying
simpler.
Sysfs - Parameter Control - gain / offsets etc
State control, turn interrupts on and off etc.
Interrupt control parameters (threshold etc)
Ring buffer parameters as relevant (currently fixed)
Individual input reading (acceleration values here)
Minor numbers for various chrdevs associated with this device.
chrdev- 3 types of chrdev at the moment
Ring buffer events
Ring buffer access (currently ripping data off the buffer only)
Interrupt events - for lis3l02dq these are only threshold breaks
Functionality yet to be implemented.
Polled based capture (use a peroidic timer if available)
Hardware ring buffering for devices that support it (two level ring
buffer -
hard and soft may be appropriate)
A chrdev for polling of whole device (with timestamps etc).
Composite interrupt handling (some devices allow logical combinations
of different interrupt signals to be used as the trigger condition).
Documenation ;)
Cleaner solution to data alignment in the ring buffer (currently I'm
cheating
and manually doing it)
Lots lots more....
Anyhow, all comments welcome. Can anyone think of a better name?
(I'm not keen on industrialio. It's too long if nothing else!
It will do as a working title for now)
Thanks,
--
Jonathan Cameron
Sorry, forgot to include the headers in the original patch. Obviously these
are very rough and ready at the moment and if nothing else their overall
layout isn't particularly logical.
> Dear All,
>
> This email is mainly to give people an idea of current progress towards
> a new
> subsystem as discussed in the thread starting with
> http://lkml.org/lkml/2008/5/20/135
>
> Sorry for the mass list bombardment, but clearly some elements of this
> discussion
> will end up in various different territories.
>
> Some elements of a prototype subsystem are in place. It draws very heavily
> on parts of the input and hwmon subsystems diverging only where necessary.
>
> The only test driver currently integrated is for an ST LIS3L02DQ
> accelerometer
> which has more than a few quirks to make it tricky to handle (and some what
> sketchy documentation.) More chips will follow over next week or so but
> hopefully the driver for this chip gives enough of an idea of how I envision
> the system working to encourage discussion / advice.
>
> Note that I haven't dealt with anywhere near all the possible locking issues
> etc and am well aware that this needs to be done. Other cleanups that will
> need to be done include working out the layout in sysfs to make it more
> intuitive. Also sorry for the somewhat rough and ready nature of the
> attached
> patch (against 2.6.26-rc4)
>
> Ring buffer design is a large part of the attached patch. I'm not sure if
> I am going about this the right way. Basically, we need ring buffers with
> almost no write latency but can waste plenty of time reading from them
> (in general case - we do however want reading the last available value to be
> fast). What is there works, but probably has at least a few nasty corner
> cases that I haven't prevented.
>
> Interfaces (these are per device) - at somepoint a procfs interface similar
> to that used in the input subsystem would make device querying
> simpler.
>
> Sysfs - Parameter Control - gain / offsets etc
> State control, turn interrupts on and off etc.
> Interrupt control parameters (threshold etc)
> Ring buffer parameters as relevant (currently fixed)
> Individual input reading (acceleration values here)
> Minor numbers for various chrdevs associated with this device.
>
> chrdev- 3 types of chrdev at the moment
> Ring buffer events
> Ring buffer access (currently ripping data off the buffer only)
> Interrupt events - for lis3l02dq these are only threshold breaks
>
> Functionality yet to be implemented.
> Polled based capture (use a peroidic timer if available)
>
> Hardware ring buffering for devices that support it (two level ring
> buffer -
> hard and soft may be appropriate)
>
> A chrdev for polling of whole device (with timestamps etc).
>
> Composite interrupt handling (some devices allow logical combinations
> of different interrupt signals to be used as the trigger condition).
>
> Documenation ;)
>
> Cleaner solution to data alignment in the ring buffer (currently I'm
> cheating
> and manually doing it)
>
> Lots lots more....
>
> Anyhow, all comments welcome. Can anyone think of a better name?
> (I'm not keen on industrialio. It's too long if nothing else!
> It will do as a working title for now)
>
> Thanks,
>
> --
>
> Jonathan Cameron
>
On Thu, 26 Jun 2008 19:01:30 +0100 Jonathan Cameron wrote:
> Documenation ;)
and Documentation ;)
Ugh. Attachment. Please send patches inline so that they can be reviewed
without having to do tons of copy-and-paste. Possibly Documentation/email-clients.txt
can help you with thunderbird.
There are several typos in the Kconfig text parts...
and in source code comments.
>From Documentation/CodingStyle:
The preferred style for long (multi-line) comments is:
/*
* This is the preferred style for multi-line
* comments in the Linux kernel source code.
* Please use it consistently.
*
* Description: A column of asterisks on the left side,
* with beginning and ending almost-blank lines.
*/
Please review/use Documentation/CodingStyle.
This Makefile (drivers/industrialio/misc/Makefile)
doesn't seem to do anything.
---
~Randy
Linux Plumbers Conference, 17-19 September 2008, Portland, Oregon USA
http://linuxplumbersconf.org/
On Thu, 2008-06-26 at 19:01 +0100, Jonathan Cameron wrote:
> Sysfs - Parameter Control - gain / offsets etc
> State control, turn interrupts on and off etc.
As in turn userspace [interrupt] event notification on and off? I would
have thought it'd be the kernel driver's responsibility to turn the
device's interrupt generation on and off according to needs for
data/events etc.
I know this is in a rough state but a few comments anyway. First, there
are heaps of casts-of-void-pointers (eg casts of kmalloc returns) which
are superfluous and hard readability 'coz you have to split the line to
fit in 80cols.
And hardcoded type-size assumptions everywhere. Don't be scared of
sizeof ;-)
> Anyhow, all comments welcome. Can anyone think of a better name?
> (I'm not keen on industrialio. It's too long if nothing else!
> It will do as a working title for now)
I don't mind industrial io but as a function prefix, iio works better.
I can't see it being used elsewhere.
also:
+/* As the ring buffer contents are device dependent this functionality
+ * must remain part of the driver and not the ring buffer subsystem */
+static ssize_t
+lis3l02dq_read_accel_from_ring(struct industrialio_ring_buffer *ring,
+ int element, char *buf)
+{
+ int val, ret, len;
+ uint16_t temp;
+ char *data, *datalock;
+
+ data = kmalloc(8, GFP_KERNEL);
+ if (data == NULL) {
+ ret = -ENOMEM;
+ goto error_ret;
+ }
+ ret = industrialio_read_last_from_ring(ring, data);
+ datalock = data + 2*element;
I haven't looked deeply at the ringbuffer code but can you guarantee
that later elements are at higher addresses than the lower ones? As in,
can one datum in the the ring buffer wrap to the beginning again?
+ kfree(data);
You free the data before you use it? Though you are using it through a
different pointer below. I wouldn't be scared of allocating 8 bytes on
the stack rather than kmalloc'ing (unless you expect this to be called
in a deep callchain)
+ temp = (((uint16_t)((datalock[1]))) << 8)
+ | (uint16_t)(datalock[0]);
+ val = *((int16_t *)(&temp));
All this data/datalock/bitshuffle nonsense would be nicer if you just
used structs and unions, yeah?
union channel {
char data[2];
int16_t val;
}
struct datum {
union channel elements[3];
}
or something.
+ len = sprintf(buf, "ring %d\n", val);
+
+ return len;
+error_ret:
+ return ret;
+}
Incidentally, is there much that your ringbuffer can do which kfifo
can't? Apart from having a bunch of extra nice accessor-helpers sitting
on the top.
Overall looking good and useful, can't wait 'till it's done :-)
--Ben.
Ben Nizette wrote:
> On Thu, 2008-06-26 at 19:01 +0100, Jonathan Cameron wrote:
>
>> Sysfs - Parameter Control - gain / offsets etc
>> State control, turn interrupts on and off etc.
>
> As in turn userspace [interrupt] event notification on and off? I would
> have thought it'd be the kernel driver's responsibility to turn the
> device's interrupt generation on and off according to needs for
> data/events etc.
Ok, there is a division here between interrupt handling on the host side
which indeed should be turned on and off transparently by the driver
and actually telling the sensor which interrupts to generate. In a sense
this is simply a case of terminology and what is actually being requested
is event notifications (which then match with those sent up to userspace).
>
> I know this is in a rough state but a few comments anyway. First, there
> are heaps of casts-of-void-pointers (eg casts of kmalloc returns) which
> are superfluous and hard readability 'coz you have to split the line to
> fit in 80cols.
Oops, silly habit that one - I'll clean those out.
> And hardcoded type-size assumptions everywhere. Don't be scared of
> sizeof ;-)
Yup, definitely need to clean them up.
>> Anyhow, all comments welcome. Can anyone think of a better name?
>> (I'm not keen on industrialio. It's too long if nothing else!
>> It will do as a working title for now)
>
> I don't mind industrial io but as a function prefix, iio works better.
> I can't see it being used elsewhere.
Good point - I'd droped to indio in some cases but iio will make those 80
character limits even easier ;)
> also:
>
> +/* As the ring buffer contents are device dependent this functionality
> + * must remain part of the driver and not the ring buffer subsystem */
> +static ssize_t
> +lis3l02dq_read_accel_from_ring(struct industrialio_ring_buffer *ring,
> + int element, char *buf)
> +{
> + int val, ret, len;
> + uint16_t temp;
> + char *data, *datalock;
> +
> + data = kmalloc(8, GFP_KERNEL);
> + if (data == NULL) {
> + ret = -ENOMEM;
> + goto error_ret;
> + }
> + ret = industrialio_read_last_from_ring(ring, data);
> + datalock = data + 2*element;
>
> I haven't looked deeply at the ringbuffer code but can you guarantee
> that later elements are at higher addresses than the lower ones? As in,
> can one datum in the the ring buffer wrap to the beginning again?
At the moment the ring buffer has to be a whole number of datums big.
I'm inclined to keep it way and move more towards dynamic allocation
such that it is true than to try handling split data reading sets.
> + kfree(data);
>
> You free the data before you use it? Though you are using it through a
> different pointer below. I wouldn't be scared of allocating 8 bytes on
> the stack rather than kmalloc'ing (unless you expect this to be called
> in a deep callchain)
Ouch. That's an out and out bug!
>
> + temp = (((uint16_t)((datalock[1]))) << 8)
> + | (uint16_t)(datalock[0]);
> + val = *((int16_t *)(&temp));
>
> All this data/datalock/bitshuffle nonsense would be nicer if you just
> used structs and unions, yeah?
>
> union channel {
> char data[2];
> int16_t val;
> }
Good point, I'd forgotten you could do that with unions.
>
> struct datum {
> union channel elements[3];
> }
>
> or something.
>
> + len = sprintf(buf, "ring %d\n", val);
> +
> + return len;
> +error_ret:
> + return ret;
> +}
Good approach, I'll switch to that.
>
> Incidentally, is there much that your ringbuffer can do which kfifo
> can't? Apart from having a bunch of extra nice accessor-helpers sitting
> on the top.
Not sure, I'll look into it.
>
> Overall looking good and useful, can't wait 'till it's done :-)
Thanks for the comments.
Jonathan
On Fri, 2008-06-27 at 10:45 +0100, Jonathan Cameron wrote:
> Ben Nizette wrote:
> > On Thu, 2008-06-26 at 19:01 +0100, Jonathan Cameron wrote:
> >
> >> Sysfs - Parameter Control - gain / offsets etc
> >> State control, turn interrupts on and off etc.
> >
> > As in turn userspace [interrupt] event notification on and off? I would
> > have thought it'd be the kernel driver's responsibility to turn the
> > device's interrupt generation on and off according to needs for
> > data/events etc.
> Ok, there is a division here between interrupt handling on the host side
> which indeed should be turned on and off transparently by the driver
> and actually telling the sensor which interrupts to generate. In a sense
> this is simply a case of terminology and what is actually being requested
> is event notifications (which then match with those sent up to userspace).
Righteo, I suspected as much :-)
< snip good replies :-) >
>
> > also:
> >
> > +/* As the ring buffer contents are device dependent this functionality
> > + * must remain part of the driver and not the ring buffer subsystem */
> > +static ssize_t
> > +lis3l02dq_read_accel_from_ring(struct industrialio_ring_buffer *ring,
> > + int element, char *buf)
> > +{
> > + int val, ret, len;
> > + uint16_t temp;
> > + char *data, *datalock;
> > +
> > + data = kmalloc(8, GFP_KERNEL);
> > + if (data == NULL) {
> > + ret = -ENOMEM;
> > + goto error_ret;
> > + }
> > + ret = industrialio_read_last_from_ring(ring, data);
> > + datalock = data + 2*element;
> >
> > I haven't looked deeply at the ringbuffer code but can you guarantee
> > that later elements are at higher addresses than the lower ones? As in,
> > can one datum in the the ring buffer wrap to the beginning again?
> At the moment the ring buffer has to be a whole number of datums big.
> I'm inclined to keep it way and move more towards dynamic allocation
> such that it is true than to try handling split data reading sets.
Yup, agreed. Just so long as your code thinks about :-)
>
> > + kfree(data);
> >
> > You free the data before you use it? Though you are using it through a
> > different pointer below. I wouldn't be scared of allocating 8 bytes on
> > the stack rather than kmalloc'ing (unless you expect this to be called
> > in a deep callchain)
> Ouch. That's an out and out bug!
> >
> > + temp = (((uint16_t)((datalock[1]))) << 8)
> > + | (uint16_t)(datalock[0]);
> > + val = *((int16_t *)(&temp));
> >
> > All this data/datalock/bitshuffle nonsense would be nicer if you just
> > used structs and unions, yeah?
> >
> > union channel {
> > char data[2];
> > int16_t val;
> > }
> Good point, I'd forgotten you could do that with unions.
Cool, just watch endianness of course :-)
> >
> > struct datum {
> > union channel elements[3];
> > }
> >
> > or something.
> >
> > + len = sprintf(buf, "ring %d\n", val);
> > +
> > + return len;
> > +error_ret:
> > + return ret;
> > +}
> Good approach, I'll switch to that.
>
> >
> > Incidentally, is there much that your ringbuffer can do which kfifo
> > can't? Apart from having a bunch of extra nice accessor-helpers sitting
> > on the top.
> Not sure, I'll look into it.
kfifo won't be a drop in replacement, it's just a very simple ring fifo.
I suspect your higher level ring buffer accessors and allocators could
live on top of it though.
> >
> > Overall looking good and useful, can't wait 'till it's done :-)
> Thanks for the comments.
>
> Jonathan
np,
--Ben.
Hi Ben,
>>> union channel {
>>> char data[2];
>>> int16_t val;
>>> }
>> Good point, I'd forgotten you could do that with unions.
>
> Cool, just watch endianness of course :-)
>
That and the annoyance of alignment issues making that approach taking way more
space that you'd think.
>>> Incidentally, is there much that your ringbuffer can do which kfifo
>>> can't? Apart from having a bunch of extra nice accessor-helpers sitting
>>> on the top.
>> Not sure, I'll look into it.
>
> kfifo won't be a drop in replacement, it's just a very simple ring fifo.
> I suspect your higher level ring buffer accessors and allocators could
> live on top of it though.
Sure, but from my understanding of kfifo it takes a much more symmetric
approach to reading and writing with locking used to prevent them occuring
concurrently. You obviously can use it without locking but I don't believe
that it provide any facility for coping with the nasty case, (buffer full
and hence during read attempts a certain amount of what is copied out may
have become invalid). It maybe the case that, as you say suitable high level
functions on top of a kfifo would be a good way to proceed (afterall, kfifo
is well tested etc), but I fear, given how little of kfifo's code would actually
be used it would be more likely to cause problems than not.
It might be best to leave this decision until the exact requirements of the ring
buffer are actually known.
Jonathan