[Xenomai-core] Analogy a4l_ioctl_bufinfo() bug?

[Xenomai-core] Analogy a4l_ioctl_bufinfo() bug?

[Daniele Nicolodi]

Hello. As reported in the xenomai-help mailing list I'm investigating
why a4l_mmap() is not supported for the ni pcimio driver. Hacking on the
driver I discovered that, apparently, the only problem in supporting
mmap of the device buffer is in the a4l_ioctl_bufinfo() function.

In my interpretation, this function does more than what it promises.
>From the documentation it looks like it just has to return the current
buffer size. However, if my understanding is correct, it also copies
data to/from the hardware FIFO to the device buffer. It is unclear to me
why this is necessary.

In doing so it uses the __munge() function, around line 630 in buffer.h.
This function in turns uses the driver specific munge function. In the
case of the ni pcimio driver this function is ni_ai_munge16() in
mio_common.c. This function expects to be called when a valid command
has been sent to the subdevice, because it unconditionally deferences
cmd->nb_chan, around line 1385.

However, when the a4l_ioctl_bufinfo() function is called, there is no
check that a valid command has been sent to the device, and generally it
 hasn't. This causes a kernel OOPS for trying to deference a NULL pointer.

My hacky solution has been to introduce a check, with an early return,
for a NULL cmd. I'm sure this is not the right solution.

However, to solve the problem properly, I need to understand why
a4l_ioctl_bufinfo() has to mess with the buffer data, when it is only
interested into the buffer dimensions.

Thanks. Cheers,
-- 
Daniele

Re: [Xenomai-core] Analogy a4l_ioctl_bufinfo() bug?

[Daniele Nicolodi]

[-- Attachment #1: Type: text/plain, Size: 571 bytes --]

Daniele Nicolodi wrote:
> Hello. As reported in the xenomai-help mailing list I'm investigating
> why a4l_mmap() is not supported for the ni pcimio driver. Hacking on the
> driver I discovered that, apparently, the only problem in supporting
> mmap of the device buffer is in the a4l_ioctl_bufinfo() function.

The atached patch solves the problem. My solution is to avoid to mess
with the subdevice buffer if a transmission is not occouring. This makes
ni pcimio driver work as expected. The second patch enables mmapping for
the ni pcimio driver.

Cheers,
-- 
Daniele


[-- Attachment #2: analogy03.patch --]
[-- Type: text/x-diff, Size: 1078 bytes --]

commit d0bbc69a106012a2152ff8ff8fe4897ae0c34bcd
Author: Daniele Nicolodi <nicolodi@domain.hid>
Date:   Wed Mar 17 15:24:22 2010 +0100

    Fix a4i_ioctl_bufinfo to do not try to copy data from the subdevice buffer if a transfer is not occuring.

diff --git a/ksrc/drivers/analogy/buffer.c b/ksrc/drivers/analogy/buffer.c
index 0c66b4a..1a8acf7 100644
--- a/ksrc/drivers/analogy/buffer.c
+++ b/ksrc/drivers/analogy/buffer.c
@@ -560,6 +560,14 @@ int a4l_ioctl_bufinfo(a4l_cxt_t * cxt, void *arg)
 
 	buf = dev->transfer.bufs[info.idx_subd];
 
+	/* If a transfer is not occuring, simply return buffer
+	   informations, otherwise make the transfer progress */
+	if (! test_bit(A4L_TSF_BUSY,
+		       &(dev->transfer.status[info.idx_subd]))) {
+		info.rw_count = 0;
+		goto a4l_ioctl_bufinfo_out;
+	}
+
 	ret = __handle_event(buf);
 
 	if (info.idx_subd == dev->transfer.idx_read_subd) {
@@ -621,6 +629,8 @@ int a4l_ioctl_bufinfo(a4l_cxt_t * cxt, void *arg)
 		buf->mng_count += tmp_cnt;
 	}
 
+a4l_ioctl_bufinfo_out:	
+
 	/* Sets the buffer size */
 	info.buf_size = buf->size;
 

[-- Attachment #3: analogy02.patch --]
[-- Type: text/x-diff, Size: 1112 bytes --]

commit 230e1c7da63e2324e25ae9bf959f5c10d3d8c3ec
Author: Daniele Nicolodi <nicolodi@domain.hid>
Date:   Wed Mar 17 15:26:26 2010 +0100

    Enable subdevice buffer mmapping for ni pcimio driver.

diff --git a/ksrc/drivers/analogy/national_instruments/mio_common.c b/ksrc/drivers/analogy/national_instruments/mio_common.c
index 1a39206..a51a3cd 100644
--- a/ksrc/drivers/analogy/national_instruments/mio_common.c
+++ b/ksrc/drivers/analogy/national_instruments/mio_common.c
@@ -4913,7 +4913,7 @@ int ni_E_init(a4l_dev_t *dev)
 		a4l_dbg(1, drv_dbg, dev, 
 			"mio_common: AI: %d channels\n", boardtype.n_adchan);
 
-		subd->flags = A4L_SUBD_AI | A4L_SUBD_CMD;
+		subd->flags = A4L_SUBD_AI | A4L_SUBD_CMD | A4L_SUBD_MMAP;
 		subd->rng_desc = ni_range_lkup[boardtype.gainlkup];
 
 		subd->chan_desc = kmalloc(sizeof(a4l_chdesc_t) + 
@@ -4981,7 +4981,7 @@ int ni_E_init(a4l_dev_t *dev)
 		
 
 		if (boardtype.ao_fifo_depth) {
-			subd->flags |= A4L_SUBD_CMD;
+			subd->flags |= A4L_SUBD_CMD | A4L_SUBD_MMAP;
 			subd->do_cmd = &ni_ao_cmd;
 			subd->cmd_mask = &mio_ao_cmd_mask;
 			subd->do_cmdtest = &ni_ao_cmdtest;

Re: [Xenomai-core] Analogy a4l_ioctl_bufinfo() bug?

[Alexis Berlemont]

Daniele Nicolodi wrote:
> Daniele Nicolodi wrote:
>> Hello. As reported in the xenomai-help mailing list I'm investigating
>> why a4l_mmap() is not supported for the ni pcimio driver. Hacking on the
>> driver I discovered that, apparently, the only problem in supporting
>> mmap of the device buffer is in the a4l_ioctl_bufinfo() function.
> 
> The atached patch solves the problem. My solution is to avoid to mess
> with the subdevice buffer if a transmission is not occouring. This makes
> ni pcimio driver work as expected. The second patch enables mmapping for
> the ni pcimio driver.
Merged. Thank you.
> 
> Cheers,
> 
> 
> ------------------------------------------------------------------------
> 
> _______________________________________________
> Xenomai-core mailing list
> Xenomai-core@domain.hid
> https://mail.gna.org/listinfo/xenomai-core

Alexis.