ioctl arg passing

ioctl arg passing

[Matt]

Righto, first post to the list, here goes:

I'm writing a char device driver for a dsp card that drives a motion
platform. The basic flow is I basically have to reset the card and upload
an executable file to it, and then poke the card to run it. Once this is
done, I can issue instructions to the card/code to pass and return data
from the card about the platform it's controlling.

To pass the instructions I'm using a generic ioctl which passes the data
between user & kernel-space using a struct which is basically like:

struct instruction_t {
	__s16 code;
	__s16 rxlen;
	__s16 *rxbuf;
	__s16 txlen;
	__s16 *txbuf;
};

(rx|tx)len is the length of the extra data that is provided/requested
in/to be in (rx|tx)buf. Got me so far?

Am I allowed to do this across the ioctl interface? In my ioctl
"handler" I'm attempting to do:

--8<--

struct instruction_t local;
__s16 *temp;

copy_from_user( &local, ( struct instruction_t * ) arg, sizeof( struct instruction_t ) );
temp = kmalloc( sizeof( __s16 ) * local.rxlen, GFP_KERNEL );
copy_from_user( temp, arg, sizeof( __s16 ) * local.rxlen );
local.rxbuf = temp;
temp = kmalloc( sizeof( __s16 ) * local.txlen, GFP_KERNEL );
...

--8<--

Is this going to work as expected? Or am I gonna generate oops-a-plenty?

Cheers

Matt

Re: ioctl arg passing

[Ingo Oeser]

On Mon, Apr 23, 2001 at 05:06:48PM +0100, Matt wrote:
> I'm writing a char device driver for a dsp card that drives a motion
> platform.

Can you elaborate on the dsp card? Is it freely programmable? I'm
working on a project to support this kind of stuff via a
dedicated subsystem for Linux.

The problem is, that it's hard to get access to such cards. So
development is moving very slow :-(

> To pass the instructions I'm using a generic ioctl which passes the data
> between user & kernel-space using a struct which is basically like:
> 
> struct instruction_t {
> 	__s16 code;
> 	__s16 rxlen;
> 	__s16 *rxbuf;
> 	__s16 txlen;
> 	__s16 *txbuf;
> };
 
Such stuff is handled already by my subsystem. You just have to
provide some function to do some checks on memory buffers
(readable, writeable, executable, unreachable, properly aligned
and sized transfer unit and so on) and functions for transfers
(which can be sych/asych), ioctls and and debugging interface for
special purposes.

> (rx|tx)len is the length of the extra data that is provided/requested
> in/to be in (rx|tx)buf. Got me so far?
> 
> Am I allowed to do this across the ioctl interface? In my ioctl
> "handler" I'm attempting to do:
> 
> --8<--
> 
> struct instruction_t local;
> __s16 *temp;
> 
> copy_from_user( &local, ( struct instruction_t * ) arg, sizeof( struct instruction_t ) );
> temp = kmalloc( sizeof( __s16 ) * local.rxlen, GFP_KERNEL );
> copy_from_user( temp, arg, sizeof( __s16 ) * local.rxlen );
> local.rxbuf = temp;
> temp = kmalloc( sizeof( __s16 ) * local.txlen, GFP_KERNEL );
> ....
> 
> --8<--
> 
> Is this going to work as expected? Or am I gonna generate oops-a-plenty?

What do you want to do with the buffers? If you plan to expose
them to user space, this is just plain wrong.

If you use it only inside the kernel, please check that you avoid
using more than PAGE_SIZE as rxlen/txlen. Do scatter-gather
instead and vmalloc(). Either in the driver or by hardware
features.

Regards

Ingo Oeser
-- 
10.+11.03.2001 - 3. Chemnitzer LinuxTag <http://www.tu-chemnitz.de/linux/tag>
         <<<<<<<<<<<<     been there and had much fun   >>>>>>>>>>>>

Re: ioctl arg passing

[Matt]

Ingo Oeser mentioned the following:

| On Mon, Apr 23, 2001 at 05:06:48PM +0100, Matt wrote:
| > I'm writing a char device driver for a dsp card that drives a motion
| > platform.
| 
| Can you elaborate on the dsp card? Is it freely programmable? I'm
| working on a project to support this kind of stuff via a
| dedicated subsystem for Linux.

AFAIK the card could be used for all sorts, but I'm not terribly
knowledgable about it as I've only been told how to program the thing with
respect to it's chosen application, ie. to drive the platform. It's got
analog and digital inputs/outputs, I don't know what else.

I'm writing this driver as part of my final year project at University,
and I'm working from the existing Windows code, so I'm not really exposed
to the cards internals at all.

The card is solely accessed through four consecutive I/O port address,
the first two control the address of ram on the card I want, and I read or
write to the second two. All accesses are 16-bit wide. That's as much as I
know really.

| The problem is, that it's hard to get access to such cards. So
| development is moving very slow :-(

My other problem is that I only have three/four weeks left to do as much
as possible, I've just managed to get my head 'round the Windows code so I
know how the code works, without having to fit it into some other grand
scheme of things.

I did try to write the driver with respect to making it nice and modular,
but without another card I can't work out what might be common to both
etc.

Once I've written the driver, I might be able to help merge it into some
other system, but atm my prority is to get it working as it is, so I can
at least get a good mark, I don't think I'm doing it a bad way, it's just
based heavily in structure on the existing Windows code.

Cheers

Matt

Re: ioctl arg passing

[rui.sousa]

On Mon, 23 Apr 2001, Ingo Oeser wrote:

> On Mon, Apr 23, 2001 at 05:06:48PM +0100, Matt wrote:
> > I'm writing a char device driver for a dsp card that drives a motion
> > platform.
>
> Can you elaborate on the dsp card? Is it freely programmable? I'm
> working on a project to support this kind of stuff via a
> dedicated subsystem for Linux.

Very interesting... The emu10k1 driver (SBLive!) that will appear
shortly in acXX will support loading code to it's DSP. It's a very
simple chip with only 16 instructions but it can generate
hardware interrupts, DMA to host memory, 32 bit math. The maximum
program size is 512 instructions (64 bits each) and can make use of 256
registers (32 bits).

Is there a web page for your project?


> The problem is, that it's hard to get access to such cards. So
> development is moving very slow :-(

If you care, the cheapest emu10k1 is 40$...

> > To pass the instructions I'm using a generic ioctl which passes the data
> > between user & kernel-space using a struct which is basically like:
> >
> > struct instruction_t {
> > 	__s16 code;
> > 	__s16 rxlen;
> > 	__s16 *rxbuf;
> > 	__s16 txlen;
> > 	__s16 *txbuf;
> > };
>
> Such stuff is handled already by my subsystem. You just have to
> provide some function to do some checks on memory buffers
> (readable, writeable, executable, unreachable, properly aligned
> and sized transfer unit and so on) and functions for transfers
> (which can be sych/asych), ioctls and and debugging interface for
> special purposes.
>
> > (rx|tx)len is the length of the extra data that is provided/requested
> > in/to be in (rx|tx)buf. Got me so far?
> >
> > Am I allowed to do this across the ioctl interface? In my ioctl
> > "handler" I'm attempting to do:
> >
> > --8<--
> >
> > struct instruction_t local;
> > __s16 *temp;
> >
> > copy_from_user( &local, ( struct instruction_t * ) arg, sizeof( struct instruction_t ) );
> > temp = kmalloc( sizeof( __s16 ) * local.rxlen, GFP_KERNEL );
> > copy_from_user( temp, arg, sizeof( __s16 ) * local.rxlen );
                          ^^^ local.rxbuf, no ?

> > local.rxbuf = temp;
> > temp = kmalloc( sizeof( __s16 ) * local.txlen, GFP_KERNEL );
> > ....
> >
> > --8<--
> >
> > Is this going to work as expected? Or am I gonna generate oops-a-plenty?

I've used this method in some of my drivers. It works just fine, but
as everybody already told you, you should do some checking on the values
you are passed from user space.

> What do you want to do with the buffers? If you plan to expose
> them to user space, this is just plain wrong.
>
> If you use it only inside the kernel, please check that you avoid
> using more than PAGE_SIZE as rxlen/txlen. Do scatter-gather
> instead and vmalloc(). Either in the driver or by hardware
> features.
>
> Regards
>
> Ingo Oeser
>

Rui Sousa

Re: ioctl arg passing

[Matt]

rui.sousa mentioned the following:

| On Mon, 23 Apr 2001, Ingo Oeser wrote:
| 
| > Can you elaborate on the dsp card? Is it freely programmable? I'm
| > working on a project to support this kind of stuff via a
| > dedicated subsystem for Linux.
| 
| Very interesting... The emu10k1 driver (SBLive!) that will appear
| shortly in acXX will support loading code to it's DSP. It's a very
| simple chip with only 16 instructions but it can generate
| hardware interrupts, DMA to host memory, 32 bit math. The maximum
| program size is 512 instructions (64 bits each) and can make use of 256
| registers (32 bits).
| 
| Is there a web page for your project?

I haven't got one for my Linux port, but the company is Motionbase,
http://www.motionbase.com/, which at least has some piccy's of the
hardware in action. (Unless you meant Ingo's project, oops!)

The nature of driver is such that it's "useless" unless you have one of
these platforms, so it's not really for the average chap... :)

As the card offers analog inputs which are commonly used for
joysticks/steering wheels depending on the application being run, I plan
to create a joystick abstraction device that pipes the analog inputs
through the joystick interface too, so that any software can use joysticks
in a standard way regardless of whether they're using the platform or just
a regular PC. I'm developing for 2.2.x, but once that's working I'll adapt
the code for 2.4.x and use the event interface, devfs etc. where
necessary.

| > > copy_from_user( &local, ( struct instruction_t * ) arg, sizeof( struct instruction_t ) );
| > > temp = kmalloc( sizeof( __s16 ) * local.rxlen, GFP_KERNEL );
| > > copy_from_user( temp, arg, sizeof( __s16 ) * local.rxlen );
|                           ^^^ local.rxbuf, no ?

Yup, that's the one, hopefully everyone except me noticed that one! :)

Thanks for the help so far, appreciated.

Matt

Re: ioctl arg passing

[Mathieu Chouquet-Stringer]

<rui.sousa@mindspeed.com> writes:

> On Mon, 23 Apr 2001, Ingo Oeser wrote:
> 
> > On Mon, Apr 23, 2001 at 05:06:48PM +0100, Matt wrote:
> > > I'm writing a char device driver for a dsp card that drives a motion
> > > platform.
> >
> > Can you elaborate on the dsp card? Is it freely programmable? I'm
> > working on a project to support this kind of stuff via a
> > dedicated subsystem for Linux.
> 
> Very interesting... The emu10k1 driver (SBLive!) that will appear
> shortly in acXX will support loading code to it's DSP. It's a very
> simple chip with only 16 instructions but it can generate
> hardware interrupts, DMA to host memory, 32 bit math. The maximum
> program size is 512 instructions (64 bits each) and can make use of 256
> registers (32 bits).

Do you mean we will be able to have the same kind of stuff they have on
Windows (like the mp3 encoding computed by the SB Live)??

-- 
Mathieu CHOUQUET-STRINGER              E-Mail : mchouque@e-steel.com
     Learning French is trivial: the word for horse is cheval, and
               everything else follows in the same way.
                        -- Alan J. Perlis

[OFFTOPIC] Re: ioctl arg passing

[rui.sousa]

On 23 Apr 2001, Mathieu Chouquet-Stringer wrote:

> <rui.sousa@mindspeed.com> writes:
>
> > On Mon, 23 Apr 2001, Ingo Oeser wrote:
> >
> > > On Mon, Apr 23, 2001 at 05:06:48PM +0100, Matt wrote:
> > > > I'm writing a char device driver for a dsp card that drives a motion
> > > > platform.
> > >
> > > Can you elaborate on the dsp card? Is it freely programmable? I'm
> > > working on a project to support this kind of stuff via a
> > > dedicated subsystem for Linux.
> >
> > Very interesting... The emu10k1 driver (SBLive!) that will appear
> > shortly in acXX will support loading code to it's DSP. It's a very
> > simple chip with only 16 instructions but it can generate
> > hardware interrupts, DMA to host memory, 32 bit math. The maximum
> > program size is 512 instructions (64 bits each) and can make use of 256
> > registers (32 bits).
>
> Do you mean we will be able to have the same kind of stuff they have on
> Windows

If someone writes the dsp code...

> (like the mp3 encoding computed by the SB Live)??

This in particular seems to be a myth...

Rui Sousa

Re: ioctl arg passing

[Matt]

Matt aka Doofus festures mentioned the following:

| struct instruction_t local;
| __s16 *temp;
| 
| copy_from_user( &local, ( struct instruction_t * ) arg, sizeof( struct instruction_t ) );
| temp = kmalloc( sizeof( __s16 ) * local.rxlen, GFP_KERNEL );
| copy_from_user( temp, arg, sizeof( __s16 ) * local.rxlen );

I meant that last line to be:

copy_from_user( temp, local.rxbuf, sizeof( __s16 ) * local.rxlen );
                      ^^^^^^^^^^^

Which'd clear up any confusion as to why I'd want two copies of the same
argument.

That's the main crux of my query, can I retrieve the value of a pointer
in some struct passed via ioctl? In this case, the struct/chunk of memory
referenced by local.rxbuf, (which is rxlen x 2 bytes big).

Apologies, I'm a muppet.

Matt

PS. Thanks for the help so far, I'd meant to add error checking and what
not, I just kept it out to keep the e-mail smaller.

Re: ioctl arg passing

[Ingo Oeser]

On Mon, Apr 23, 2001 at 08:58:54PM +0100, Matt wrote:
> Matt aka Doofus festures mentioned the following:
> 
> | struct instruction_t local;
> | __s16 *temp;
> | 
> | copy_from_user( &local, ( struct instruction_t * ) arg, sizeof( struct instruction_t ) );
> | temp = kmalloc( sizeof( __s16 ) * local.rxlen, GFP_KERNEL );
> | copy_from_user( temp, arg, sizeof( __s16 ) * local.rxlen );
> 
> I meant that last line to be:
> 
> copy_from_user( temp, local.rxbuf, sizeof( __s16 ) * local.rxlen );
>                       ^^^^^^^^^^^
> That's the main crux of my query, can I retrieve the value of a pointer
> in some struct passed via ioctl? In this case, the struct/chunk of memory
> referenced by local.rxbuf, (which is rxlen x 2 bytes big).

Yes, that works (with the obvious note on checking argument sizes
and not kmallocing too much memory).

All "read" functions do the same. As you were clever enough to
copy the pointer itself into kernel space, too (which many driver
writes forget!), you have done the right thing here.

Congratulations! ;-)

Regards

Ingo Oeser
-- 
10.+11.03.2001 - 3. Chemnitzer LinuxTag <http://www.tu-chemnitz.de/linux/tag>
         <<<<<<<<<<<<     been there and had much fun   >>>>>>>>>>>>

Re: ioctl arg passing

[Matt]

Matt mentioned the following:

| struct instruction_t {
| 	__s16 code;
| 	__s16 rxlen;
| 	__s16 *rxbuf;
| 	__s16 txlen;
| 	__s16 *txbuf;
| };

So far, I now know I can grab stuff across the user <-> kernel divide as I
planned. The only problem I'm left with, which was kindly pointed out to
me, is a question of packing with respect to both kernel & user-space.

Can anyone suggest a method of either assuring the above structure is
always packed the same, or alterations so that the problem is
minimised? Either splitting the one ioctl into many, etc.

Thanks

Matt

Re: ioctl arg passing

[Jonathan Lundell]

At 11:09 PM +0100 4/23/01, Matt wrote:
>| struct instruction_t {
>|	__s16 code;
>|	__s16 rxlen;
>|	__s16 *rxbuf;
>|	__s16 txlen;
>|	__s16 *txbuf;
>| };
>
>So far, I now know I can grab stuff across the user <-> kernel divide as I
>planned. The only problem I'm left with, which was kindly pointed out to
>me, is a question of packing with respect to both kernel & user-space.
>
>Can anyone suggest a method of either assuring the above structure is
>always packed the same, or alterations so that the problem is
>minimised? Either splitting the one ioctl into many, etc.

struct instruction_t {
	__s16 code;
	__s16 rxlen;
	__s16 txlen;
	__s16 pad;
	__s16 *rxbuf;
	__s16 *txbuf;
};

This was it's always aligned and packed, regardless of compiler packing settings. This particular layout happens to work with 64-bit pointers as well.... (I'm assuming that __s16 is a signed 16-bit type).

You can move the pointers to the front, but it's not necessary in this case, so I tried to preserve some of your original ordering (code first, rx before tx).
-- 
/Jonathan Lundell.

Re: ioctl arg passing

[Petr Vandrovec]

On 23 Apr 01 at 17:06, Matt wrote:
> struct instruction_t {
>     __s16 code;
>     __s16 rxlen;
>     __s16 *rxbuf;
>     __s16 txlen;
>     __s16 *txbuf;
> };

You should reorder fields, starting with largest fields and going down
to smaller ones. That ways you'll not have troubles with alignment when
someone decides to play with alignment rules...

> struct instruction_t local;
> __s16 *temp;
> 
> copy_from_user( &local, ( struct instruction_t * ) arg, sizeof( struct instruction_t ) );
> temp = kmalloc( sizeof( __s16 ) * local.rxlen, GFP_KERNEL );
> copy_from_user( temp, arg, sizeof( __s16 ) * local.rxlen );
> local.rxbuf = temp;
> temp = kmalloc( sizeof( __s16 ) * local.txlen, GFP_KERNEL );
> ...

As you are using signed value for rxlen/txlen, you should check
for value < 0 ... And there is very low chance that kmalloc() for 
anything bigger than 4KB will succeed. You should either use
vmalloc unconditionally, or at least as fallback. And some error
checking (copy_from_user returns 0 if everything went OK) also
makes driver safer.
                                        Best regards,
                                            Petr Vandrovec
                                            vandrove@vc.cvut.cz

Re: ioctl arg passing

[Alex Bligh - linux-kernel]

> And there is very low chance that kmalloc() for
> anything bigger than 4KB will succeed. You should either use
> vmalloc unconditionally, or at least as fallback.

The phrase 'very low chance' is inaccurate.

How do you think NFS works with -rsize, -wsize > 4096?
kmalloc() uses get_free_pages() to allocate
more than one physically contiguous memory
page, which in turn uses a sort of modified
buddy system to distribute them. And if you
specify the right GFP_ flags, will page out,
if necessary, to do so. (I know the hard way
as this is the one substantial thing I fixed
in the linux kernel just after 1.0 in about 95).

If you need physically (as opposed to virtially)
contiguous memory, unless lots has changed since then,
kmalloc() is the right call. However, you are
correct that it draws on scarce resources.

--
Alex Bligh