Re: [linux-dvb] [PATCH] 2/3: implement DMX_SET_BUFFER_SIZE for dvr

[Oliver Endriss <o.endriss@gmx.de>]

Andrea wrote:
> This patch implements the ioctl DMX_SET_BUFFER_SIZE for the dvr.
> 
> The ioctl used to be supported but not yet implemented.
> 
> The way it works is that it replaces the ringbuffer with a new one.
> Beforehand it flushes the old buffer.
> This means that part of the stream is lost, and reading errors (like overflow) are cleaned.
> The flushing is not a problem since this operation usually occurs at beginning before start reading.
> 
> Since the dvr is *always* up and running this change has to be done while the buffer is written:
> 
> 1) On the userspace side, it is as safe as dvb_dvr_read is now:
> 	- dvb_dvr_set_buffer_size locks the mutex
> 	- dvb_dvr_read does *not* lock the mutex (the code is there commented out)
> 
> So as long as one does not call read simultaneously it works properly.
> Maybe the mutex should be enforced in dvb_dvr_read.
> 
> 2) On the kernel side
> The only thing I am not 100% sure about is whether the spin_lock I've used is enough to guarantee
> synchronization between the new function dvb_dvr_set_buffer_size (which uses spin_lock_irq) and
> dvb_dmxdev_ts_callback and dvb_dmxdev_section_callback (using spin_lock).
> But this looks to me the same as all other functions do.

Please see my other mail, too

>       spin_lock(&dmxdev->lock);
>       mem = buf->data;
>       buf->data = NULL;
>       buf->size = size;
>       dvb_ringbuffer_flush(buf);
>       spin_unlock(&dmxdev->lock);
>       vfree(mem);
>
>       if (buf->size) {
>               mem = vmalloc(dmxdev->dvr_buffer.size);
>               if (!mem)
>                       return -ENOMEM;
>               spin_lock(&dmxdev->lock);
>               buf->data = mem;
>               spin_unlock(&dmxdev->lock);
>       }

I see some problems here:
- Do not release the lock before the ringbuffer is consistent again.
- We should not free the old buffer before we got a new one.
- As the ring buffer can be written from an ISR, we have to use
  spin_lock_irqsave/spin_unlock_irqrestore here.

What about this fragment:
	...
	if (!size)
		return -EINVAL;

	mem = vmalloc(size);
	if (!mem)
		return -ENOMEM;

	mem2 = buf->data;

        spin_lock_irqsave(&dmxdev->lock);
        buf->pread = buf->pwrite = 0;
	buf->data = mem;
	buf->size = size;
	spin_unlock_irqrestore(&dmxdev->lock);

	vfree(mem2);
	return 0;

Any comemnts? I hope that someone else also has a look at this because
I don't have much time atm.

CU
Oliver

-- 
----------------------------------------------------------------
VDR Remote Plugin 0.4.0: http://www.escape-edv.de/endriss/vdr/
----------------------------------------------------------------

_______________________________________________
linux-dvb mailing list
linux-dvb@linuxtv.org
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb