Re: [Xenomai] [PATCH] mpc5200: Add RTDM LPBFIFO driver and demo RTDM FPGA device driver

[Stefan Roese <stefan.roese@gmail.com>]

Hi Gilles,

On 11/16/2012 06:45 PM, Gilles Chanteperdrix wrote:
> On 11/15/2012 03:19 PM, stefan.roese@gmail.com wrote:
>> +/*
>> + * NO_DMA:
>> + *
>> + * Define to enable memcpy() for blockdata transfer instead of DMA
>> + * support. This is available only for test purposes. Should be disabled
>> + * or even removed in the final driver version.
>> + */
>> +#if 0
>> +#define NO_DMA
>> +#endif
>> +
>> +/*
>> + * PRINT_READ_TIME:
>> + *
>> + * Print min and max times consumed by the blockdata transfer, either
>> + * per DMA or per memcpy. Should be disbaled or even removed in the
>> + * final driver version.
>> + */
>> +#if 1
>> +#define PRINT_READ_TIME
>> +#endif
>> +
>> +/*
>> + * DEBUG:
>> + *
>> + * Define to print debug informations. Should be disabled in the
>> + * final driver of course.
>> + */
>> +#if 0
>> +#define DEBUG
>> +#endif
> 
> This is dead code, please use Kconfig or remove the dead code entirely.

Yes. Will move to Kconfig option.

>> +	/*
>> +	 * Block until the blockdata is completely read into
>> +	 * the buffer (done in interrupt)
>> +	 */
>> +	ret = rtdm_event_timedwait(&fpga_ctx->read_event,
>> +				   fpga_ctx->info->timeout, NULL);
>> +
>> +	/*
>> +	 * Mark blockdata as read
>> +	 */
>> +	fpga_ctx->blockdata_ready = 0;
>> +
>> +	if (ret)
>> +		return ret;
> 
> You set blockdata_ready whatever the return value of
> rtdm_event_timedwait so, what if rtdm_event_timedwait fails?

Even when rtdm_event_timedwait() fails (timeout) the blockdata_ready
flag needs to be reset. As it marks the last read access from the
application (task) to be finished. So re-setting it to 0 even in the
timeout case was done intentionally here.

> and you do it without any kind of mutual exclusion, so, what if another
> thread or an irq is doing something between the call to
> rtdm_event_timedwait and the moment where that variable is set to 0?

Only one thread can access the read function at a time (synchronized
with ctx->reading). So there can be no clashes with other read-threads
here. And the interrupt shall set the blockdata_ready flag upon the
cycle data interrupt. To signal new data ready for reading. So I don't
see where some synchronization method is needed here.

Thanks for the review,
Stefan