[BUG] changeset 9029 (http://linuxtv.org/hg/v4l-dvb/rev/aa3e5cc1d833)

[BUG] changeset 9029 (http://linuxtv.org/hg/v4l-dvb/rev/aa3e5cc1d833)

[e9hack]

Hi,

this change set is wrong. The affected functions cannot be called from an interrupt
context, because they may process large buffers. In this case, interrupts are disabled for
a long time. Functions, like dvb_dmx_swfilter_packets(), could be called only from a
tasklet. This change set does hide some strong design bugs in dm1105.c and au0828-dvb.c.

Please revert this change set and do fix the bugs in dm1105.c and au0828-dvb.c (and other
files).

Regards,
Hartmut

Re: [BUG] changeset 9029 (http://linuxtv.org/hg/v4l-dvb/rev/aa3e5cc1d833)

[Andy Walls]

On Mon, 2009-02-02 at 02:46 +0100, e9hack wrote:
> Hi,
> 
> this change set is wrong. The affected functions cannot be called from an interrupt
> context, because they may process large buffers. In this case, interrupts are disabled for
> a long time. Functions, like dvb_dmx_swfilter_packets(), could be called only from a
> tasklet. 

I agree with Hartmut that these functions should not be called from an
interrupt context.

Although for deferrable work, I thought tasklets were deprecated and
that work handlers were the preferred mechanism:
http://lwn.net/Articles/23634/


> This change set does hide some strong design bugs in dm1105.c and au0828-dvb.c.
> 
> Please revert this change set and do fix the bugs in dm1105.c and au0828-dvb.c (and other
> files).

BTW dm1105.c does use a tasklet for IR keypresses.  However, since it is
only imlemented with one "struct infrared", and hence only one
"ir_command", per device and not a queue, it is possible to lose button
presses that happen very close together.  That probably doesn't matter
practically for IR button presses, but the same strategy cannot be used
for TS packets.

Regards,
Andy


> Regards,
> Hartmut

Re: [BUG] changeset 9029 (http://linuxtv.org/hg/v4l-dvb/rev/aa3e5cc1d833)

[Oliver Endriss]

e9hack wrote:
> Hi,
> 
> this change set is wrong. The affected functions cannot be called from an interrupt
> context, because they may process large buffers. In this case, interrupts are disabled for
> a long time. Functions, like dvb_dmx_swfilter_packets(), could be called only from a
> tasklet. This change set does hide some strong design bugs in dm1105.c and au0828-dvb.c.
> 
> Please revert this change set and do fix the bugs in dm1105.c and au0828-dvb.c (and other
> files).

@Mauro:

This changeset _must_ be reverted! It breaks all kernels since 2.6.27
for applications which use DVB and require a low interrupt latency.

It is a very bad idea to call the demuxer to process data buffers with
interrupts disabled!

FYI, a LIRC problem was reported here:
  http://vdrportal.de/board/thread.php?postid=786366#post786366

and it has been verified that changeset
  http://linuxtv.org/hg/v4l-dvb/rev/aa3e5cc1d833
causes the problem:
  http://vdrportal.de/board/thread.php?postid=791813#post791813

Please revert this changeset immediately and submit a fix to the stable
kernels >= 2.6.27.

CU
Oliver

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

Re: [linux-dvb] [BUG] changeset 9029 (http://linuxtv.org/hg/v4l-dvb/rev/aa3e5cc1d833)

[Andy Walls]

On Sun, 2009-02-15 at 13:36 +0100, Oliver Endriss wrote:
> e9hack wrote:
> > Hi,
> > 
> > this change set is wrong. The affected functions cannot be called from an interrupt
> > context, because they may process large buffers. In this case, interrupts are disabled for
> > a long time. Functions, like dvb_dmx_swfilter_packets(), could be called only from a
> > tasklet. This change set does hide some strong design bugs in dm1105.c and au0828-dvb.c.
> > 
> > Please revert this change set and do fix the bugs in dm1105.c and au0828-dvb.c (and other
> > files).

Does anyone have a complete list of the drivers that are the bad actors?
It would be nice to have some sense of the scope of the work to fix
them, and to know who might have hardware for testing.

Hartmut mentioned 2 current problem drivers.  I know cx18 was a problem,
but I fixed it months ago.  I can agree in every driver that does things
wrong, it is a design error.  Fixing design errors will not be a quick
fix for most drivers

Regards,
Andy

> @Mauro:
> 
> This changeset _must_ be reverted! It breaks all kernels since 2.6.27
> for applications which use DVB and require a low interrupt latency.
> 
> It is a very bad idea to call the demuxer to process data buffers with
> interrupts disabled!
> 
> FYI, a LIRC problem was reported here:
>   http://vdrportal.de/board/thread.php?postid=786366#post786366
> 
> and it has been verified that changeset
>   http://linuxtv.org/hg/v4l-dvb/rev/aa3e5cc1d833
> causes the problem:
>   http://vdrportal.de/board/thread.php?postid=791813#post791813
> 
> Please revert this changeset immediately and submit a fix to the stable
> kernels >= 2.6.27.
> 
> CU
> Oliver
> 

Re: [linux-dvb] [BUG] changeset 9029 (http://linuxtv.org/hg/v4l-dvb/rev/aa3e5cc1d833)

[Andy Walls]

On Sun, 2009-02-15 at 13:36 +0100, Oliver Endriss wrote:
> e9hack wrote:
> > Hi,
> > 
> > this change set is wrong. The affected functions cannot be called from an interrupt
> > context, because they may process large buffers. In this case, interrupts are disabled for
> > a long time. Functions, like dvb_dmx_swfilter_packets(), could be called only from a
> > tasklet. This change set does hide some strong design bugs in dm1105.c and au0828-dvb.c.
> > 
> > Please revert this change set and do fix the bugs in dm1105.c and au0828-dvb.c (and other
> > files).
> 
> @Mauro:
> 
> This changeset _must_ be reverted! It breaks all kernels since 2.6.27
> for applications which use DVB and require a low interrupt latency.
> 
> It is a very bad idea to call the demuxer to process data buffers with
> interrupts disabled!
> 
> FYI, a LIRC problem was reported here:
>   http://vdrportal.de/board/thread.php?postid=786366#post786366
> 
> and it has been verified that changeset
>   http://linuxtv.org/hg/v4l-dvb/rev/aa3e5cc1d833
> causes the problem:
>   http://vdrportal.de/board/thread.php?postid=791813#post791813
> 
> Please revert this changeset immediately and submit a fix to the stable
> kernels >= 2.6.27.
> 
> CU
> Oliver


The patch below is an idea for a fix that uses a module parameter to
give back right away the original behavior to those who need it, while
buying time to fix the drivers that are doing things wrong.


I don't know if this patch will be acceptable to anyone, and I suspect
there will be disagreement on the default behavior.

It compiles and comes through checkpatch.pl with only one warning about
an extern declaration I didn't know where to place.  This patch still
needs to be checked for correctness and tested.


Regards,
Andy


Signed-off-by: Andy Walls <awalls@radix.net>


diff -r 3976e528b4a6 linux/drivers/media/dvb/dvb-core/dmxdev.c
--- a/linux/drivers/media/dvb/dvb-core/dmxdev.c	Sat Feb 14 15:08:37 2009 -0500
+++ b/linux/drivers/media/dvb/dvb-core/dmxdev.c	Sun Feb 15 14:55:49 2009 -0500
@@ -35,6 +35,17 @@
 
 module_param(debug, int, 0644);
 MODULE_PARM_DESC(debug, "Turn on/off debugging (default:off).");
+
+/*
+ * FIXME - remove this conservative lock kludge, when the offending drivers
+ * that make some calls improperly from an interrupt context are fixed.
+ */
+int dmxdev_conservative_locks;
+
+module_param_named(conservative_locks, dmxdev_conservative_locks, int, 0644);
+MODULE_PARM_DESC(conservative_locks,
+		 "Work around for drivers that make calls\n"
+		 "\t\twith interrupts disabled (default:0/off).");
 
 #define dprintk	if (debug) printk
 
@@ -364,16 +375,22 @@
 				       enum dmx_success success)
 {
 	struct dmxdev_filter *dmxdevfilter = filter->priv;
-	unsigned long flags;
+	unsigned long flags = 0;
 	int ret;
 
 	if (dmxdevfilter->buffer.error) {
 		wake_up(&dmxdevfilter->buffer.queue);
 		return 0;
 	}
-	spin_lock_irqsave(&dmxdevfilter->dev->lock, flags);
+	if (dmxdev_conservative_locks)
+		spin_lock_irqsave(&dmxdevfilter->dev->lock, flags);
+	else
+		spin_lock(&dmxdevfilter->dev->lock);
 	if (dmxdevfilter->state != DMXDEV_STATE_GO) {
-		spin_unlock_irqrestore(&dmxdevfilter->dev->lock, flags);
+		if (dmxdev_conservative_locks)
+			spin_unlock_irqrestore(&dmxdevfilter->dev->lock, flags);
+		else
+			spin_unlock(&dmxdevfilter->dev->lock);
 		return 0;
 	}
 	del_timer(&dmxdevfilter->timer);
@@ -392,7 +409,10 @@
 	}
 	if (dmxdevfilter->params.sec.flags & DMX_ONESHOT)
 		dmxdevfilter->state = DMXDEV_STATE_DONE;
-	spin_unlock_irqrestore(&dmxdevfilter->dev->lock, flags);
+	if (dmxdev_conservative_locks)
+		spin_unlock_irqrestore(&dmxdevfilter->dev->lock, flags);
+	else
+		spin_unlock(&dmxdevfilter->dev->lock);
 	wake_up(&dmxdevfilter->buffer.queue);
 	return 0;
 }
@@ -404,12 +424,18 @@
 {
 	struct dmxdev_filter *dmxdevfilter = feed->priv;
 	struct dvb_ringbuffer *buffer;
-	unsigned long flags;
+	unsigned long flags = 0;
 	int ret;
 
-	spin_lock_irqsave(&dmxdevfilter->dev->lock, flags);
+	if (dmxdev_conservative_locks)
+		spin_lock_irqsave(&dmxdevfilter->dev->lock, flags);
+	else
+		spin_lock(&dmxdevfilter->dev->lock);
 	if (dmxdevfilter->params.pes.output == DMX_OUT_DECODER) {
-		spin_unlock_irqrestore(&dmxdevfilter->dev->lock, flags);
+		if (dmxdev_conservative_locks)
+			spin_unlock_irqrestore(&dmxdevfilter->dev->lock, flags);
+		else
+			spin_unlock(&dmxdevfilter->dev->lock);
 		return 0;
 	}
 
@@ -419,7 +445,10 @@
 	else
 		buffer = &dmxdevfilter->dev->dvr_buffer;
 	if (buffer->error) {
-		spin_unlock_irqrestore(&dmxdevfilter->dev->lock, flags);
+		if (dmxdev_conservative_locks)
+			spin_unlock_irqrestore(&dmxdevfilter->dev->lock, flags);
+		else
+			spin_unlock(&dmxdevfilter->dev->lock);
 		wake_up(&buffer->queue);
 		return 0;
 	}
@@ -430,7 +459,10 @@
 		dvb_ringbuffer_flush(buffer);
 		buffer->error = ret;
 	}
-	spin_unlock_irqrestore(&dmxdevfilter->dev->lock, flags);
+	if (dmxdev_conservative_locks)
+		spin_unlock_irqrestore(&dmxdevfilter->dev->lock, flags);
+	else
+		spin_unlock(&dmxdevfilter->dev->lock);
 	wake_up(&buffer->queue);
 	return 0;
 }
diff -r 3976e528b4a6 linux/drivers/media/dvb/dvb-core/dvb_demux.c
--- a/linux/drivers/media/dvb/dvb-core/dvb_demux.c	Sat Feb 14 15:08:37 2009 -0500
+++ b/linux/drivers/media/dvb/dvb-core/dvb_demux.c	Sun Feb 15 14:55:49 2009 -0500
@@ -37,6 +37,12 @@
 ** #define DVB_DEMUX_SECTION_LOSS_LOG to monitor payload loss in the syslog
 */
 // #define DVB_DEMUX_SECTION_LOSS_LOG
+
+/*
+ * FIXME - remove this conservative lock kludge, when the offending drivers
+ * that make some calls improperly from an interrupt context are fixed.
+ */
+extern int dmxdev_conservative_locks;
 
 /******************************************************************************
  * static inlined helper functions
@@ -399,9 +405,12 @@
 void dvb_dmx_swfilter_packets(struct dvb_demux *demux, const u8 *buf,
 			      size_t count)
 {
-	unsigned long flags;
+	unsigned long flags = 0;
 
-	spin_lock_irqsave(&demux->lock, flags);
+	if (dmxdev_conservative_locks)
+		spin_lock_irqsave(&demux->lock, flags);
+	else
+		spin_lock(&demux->lock);
 
 	while (count--) {
 		if (buf[0] == 0x47)
@@ -409,17 +418,23 @@
 		buf += 188;
 	}
 
-	spin_unlock_irqrestore(&demux->lock, flags);
+	if (dmxdev_conservative_locks)
+		spin_unlock_irqrestore(&demux->lock, flags);
+	else
+		spin_unlock(&demux->lock);
 }
 
 EXPORT_SYMBOL(dvb_dmx_swfilter_packets);
 
 void dvb_dmx_swfilter(struct dvb_demux *demux, const u8 *buf, size_t count)
 {
-	unsigned long flags;
+	unsigned long flags = 0;
 	int p = 0, i, j;
 
-	spin_lock_irqsave(&demux->lock, flags);
+	if (dmxdev_conservative_locks)
+		spin_lock_irqsave(&demux->lock, flags);
+	else
+		spin_lock(&demux->lock);
 
 	if (demux->tsbufp) {
 		i = demux->tsbufp;
@@ -452,18 +467,24 @@
 	}
 
 bailout:
-	spin_unlock_irqrestore(&demux->lock, flags);
+	if (dmxdev_conservative_locks)
+		spin_unlock_irqrestore(&demux->lock, flags);
+	else
+		spin_unlock(&demux->lock);
 }
 
 EXPORT_SYMBOL(dvb_dmx_swfilter);
 
 void dvb_dmx_swfilter_204(struct dvb_demux *demux, const u8 *buf, size_t count)
 {
-	unsigned long flags;
+	unsigned long flags = 0;
 	int p = 0, i, j;
 	u8 tmppack[188];
 
-	spin_lock_irqsave(&demux->lock, flags);
+	if (dmxdev_conservative_locks)
+		spin_lock_irqsave(&demux->lock, flags);
+	else
+		spin_lock(&demux->lock);
 
 	if (demux->tsbufp) {
 		i = demux->tsbufp;
@@ -504,7 +525,10 @@
 	}
 
 bailout:
-	spin_unlock_irqrestore(&demux->lock, flags);
+	if (dmxdev_conservative_locks)
+		spin_unlock_irqrestore(&demux->lock, flags);
+	else
+		spin_unlock(&demux->lock);
 }
 
 EXPORT_SYMBOL(dvb_dmx_swfilter_204);

Re: [BUG] changeset 9029 (http://linuxtv.org/hg/v4l-dvb/rev/aa3e5cc1d833)

[Trent Piepho]

On Sun, 15 Feb 2009, Oliver Endriss wrote:
> e9hack wrote:
> > this change set is wrong. The affected functions cannot be called from an interrupt
> > context, because they may process large buffers. In this case, interrupts are disabled for
> > a long time. Functions, like dvb_dmx_swfilter_packets(), could be called only from a
> > tasklet. This change set does hide some strong design bugs in dm1105.c and au0828-dvb.c.
> >
> > Please revert this change set and do fix the bugs in dm1105.c and au0828-dvb.c (and other
> > files).
>
> @Mauro:
>
> This changeset _must_ be reverted! It breaks all kernels since 2.6.27
> for applications which use DVB and require a low interrupt latency.
>
> It is a very bad idea to call the demuxer to process data buffers with
> interrupts disabled!

I agree, this is bad.  The demuxer is far too much work to be done with
IRQs off.  IMHO, even doing it under a spin-lock is excessive.  It should
be a mutex.  Drivers should use a work-queue to feed the demuxer.

Re: [linux-dvb] [BUG] changeset 9029 (http://linuxtv.org/hg/v4l-dvb/rev/aa3e5cc1d833)

[VDR User]

Why was this even committed in the first place being that it has such
a horrible result??

Re: [BUG] changeset 9029 (http://linuxtv.org/hg/v4l-dvb/rev/aa3e5cc1d833)

[Mauro Carvalho Chehab]

On Mon, 16 Feb 2009 08:19:06 -0800 (PST)
Trent Piepho <xyzzy@speakeasy.org> wrote:

> On Sun, 15 Feb 2009, Oliver Endriss wrote:
> > e9hack wrote:
> > > this change set is wrong. The affected functions cannot be called from an interrupt
> > > context, because they may process large buffers. In this case, interrupts are disabled for
> > > a long time. Functions, like dvb_dmx_swfilter_packets(), could be called only from a
> > > tasklet. This change set does hide some strong design bugs in dm1105.c and au0828-dvb.c.
> > >
> > > Please revert this change set and do fix the bugs in dm1105.c and au0828-dvb.c (and other
> > > files).
> >
> > @Mauro:
> >
> > This changeset _must_ be reverted! It breaks all kernels since 2.6.27
> > for applications which use DVB and require a low interrupt latency.
> >
> > It is a very bad idea to call the demuxer to process data buffers with
> > interrupts disabled!
> 
> I agree, this is bad.  The demuxer is far too much work to be done with
> IRQs off.  IMHO, even doing it under a spin-lock is excessive.  It should
> be a mutex.  Drivers should use a work-queue to feed the demuxer.

Hartmut, Oliver and Trent: Thanks for helping with this issue. I've just
reverted the changeset. We still need a fix at dm1105, au0828-dvb and maybe
other drivers that call the filtering routines inside IRQ's.


Cheers,
Mauro

Re: [linux-dvb] [BUG] changeset 9029 (http://linuxtv.org/hg/v4l-dvb/rev/aa3e5cc1d833)

[Steven Toth]

> Hartmut, Oliver and Trent: Thanks for helping with this issue. I've just
> reverted the changeset. We still need a fix at dm1105, au0828-dvb and maybe
> other drivers that call the filtering routines inside IRQ's.

Fix the demux, add a worker thread and allow drivers to call it directly.

I'm not a big fan of videobuf_dvb or having each driver do it's own thing as an 
alternative.

Fixing the demux... Would this require and extra buffer copy? probably, but it's 
a trade-off between  the amount of spent during code management on a driver by 
driver basis vs wrestling with videobuf_dvb and all of problems highlighted on 
the ML over the last 2 years.

demux->register_driver()
demux->deliver_payload()
demux->unregister_driver()

Then deprecate sw_filter....N() methods.

That would simplify drivers significantly, at the expense of another buffer copy 
while deliver-payload() clones the buffer into its internal state to be more timely.

- Steve

Re: [linux-dvb] [BUG] changeset 9029 (http://linuxtv.org/hg/v4l-dvb/rev/aa3e5cc1d833)

[Steven Toth]

Steven Toth wrote:
>> Hartmut, Oliver and Trent: Thanks for helping with this issue. I've just
>> reverted the changeset. We still need a fix at dm1105, au0828-dvb and 
>> maybe
>> other drivers that call the filtering routines inside IRQ's.
> 
> Fix the demux, add a worker thread and allow drivers to call it directly.
> 
> I'm not a big fan of videobuf_dvb or having each driver do it's own 
> thing as an alternative.
> 
> Fixing the demux... Would this require and extra buffer copy? probably, 
> but it's a trade-off between  the amount of spent during code management 
> on a driver by driver basis vs wrestling with videobuf_dvb and all of 
> problems highlighted on the ML over the last 2 years.
> 
> demux->register_driver()
> demux->deliver_payload()
> demux->unregister_driver()
> 
> Then deprecate sw_filter....N() methods.
> 
> That would simplify drivers significantly, at the expense of another 
> buffer copy while deliver-payload() clones the buffer into its internal 
> state to be more timely.

I meant to add...

The cx18 and a few other smaller drivers (flexcop?) dvb drivers also call 
directly. cx23885/cx88 does not.

- Steve

Re: [linux-dvb] [BUG] changeset 9029 (http://linuxtv.org/hg/v4l-dvb/rev/aa3e5cc1d833)

[Andy Walls]

On Mon, 2009-02-16 at 14:15 -0500, Steven Toth wrote:
> Steven Toth wrote:
> >> Hartmut, Oliver and Trent: Thanks for helping with this issue. I've just
> >> reverted the changeset. We still need a fix at dm1105, au0828-dvb and 
> >> maybe
> >> other drivers that call the filtering routines inside IRQ's.
> > 
> > Fix the demux, add a worker thread and allow drivers to call it directly.
> > 
> > I'm not a big fan of videobuf_dvb or having each driver do it's own 
> > thing as an alternative.
> > 
> > Fixing the demux... Would this require and extra buffer copy? probably, 
> > but it's a trade-off between  the amount of spent during code management 
> > on a driver by driver basis vs wrestling with videobuf_dvb and all of 
> > problems highlighted on the ML over the last 2 years.
> > 
> > demux->register_driver()
> > demux->deliver_payload()
> > demux->unregister_driver()
> > 
> > Then deprecate sw_filter....N() methods.
> > 
> > That would simplify drivers significantly, at the expense of another 
> > buffer copy while deliver-payload() clones the buffer into its internal 
> > state to be more timely.
> 
> I meant to add...
> 
> The cx18 and a few other smaller drivers (flexcop?) dvb drivers also call 
> directly. cx23885/cx88 does not.

cx18 calls it from it's own worker thread.  To keep up with the rate at
which the CX23418 firmware hands over buffers (and times out the cpu2epu
mailbox!) during a dual analog DTV capture, there's no avoiding having a
worker thread in the cx18 driver.

Regards,
Andy

Re: [linux-dvb] [BUG] changeset 9029 (http://linuxtv.org/hg/v4l-dvb/rev/aa3e5cc1d833)

[Trent Piepho]

On Mon, 16 Feb 2009, Steven Toth wrote:
> > Hartmut, Oliver and Trent: Thanks for helping with this issue. I've just
> > reverted the changeset. We still need a fix at dm1105, au0828-dvb and maybe
> > other drivers that call the filtering routines inside IRQ's.
>
> Fix the demux, add a worker thread and allow drivers to call it directly.
>
> I'm not a big fan of videobuf_dvb or having each driver do it's own thing as an
> alternative.
>
> Fixing the demux... Would this require and extra buffer copy? probably, but it's
> a trade-off between  the amount of spent during code management on a driver by
> driver basis vs wrestling with videobuf_dvb and all of problems highlighted on
> the ML over the last 2 years.

Have the driver copy the data into the demuxer from the interrupt handler
with irqs disabled?  That's still too much.

The right way to do it is to have a queue of DMA buffers.  In the interrupt
handler the driver takes the completed DMA buffer off the "to DMA" queue
and puts it in the "to process" queue.  The driver should not copy and
cetainly not demux the data from the interrupt handler.

Re: [BUG] changeset 9029 (http://linuxtv.org/hg/v4l-dvb/rev/aa3e5cc1d833)

[Oliver Endriss]

Trent Piepho wrote:
> On Sun, 15 Feb 2009, Oliver Endriss wrote:
> > e9hack wrote:
> > > this change set is wrong. The affected functions cannot be called from an interrupt
> > > context, because they may process large buffers. In this case, interrupts are disabled for
> > > a long time. Functions, like dvb_dmx_swfilter_packets(), could be called only from a
> > > tasklet. This change set does hide some strong design bugs in dm1105.c and au0828-dvb.c.
> > >
> > > Please revert this change set and do fix the bugs in dm1105.c and au0828-dvb.c (and other
> > > files).
> >
> > @Mauro:
> >
> > This changeset _must_ be reverted! It breaks all kernels since 2.6.27
> > for applications which use DVB and require a low interrupt latency.
> >
> > It is a very bad idea to call the demuxer to process data buffers with
> > interrupts disabled!
> 
> I agree, this is bad.  The demuxer is far too much work to be done with
> IRQs off.  IMHO, even doing it under a spin-lock is excessive.  It should
> be a mutex.  Drivers should use a work-queue to feed the demuxer.

Agreed, this would be the best solution.

On the other hand, a workqueue handler would be scheduled later, so you
need larger buffers in the driver. Some chipsets have very small
buffers...

Anway, this would be a major change. All drivers must be carefully
modified and tested for an extended period.

Meanwhile I had a look at the changeset, and I do not understand why
spin_lock_irq... should be required everywhere.

Afaics a driver may safely call dvb_dmx_swfilter_packets,
dvb_dmx_swfilter_204 or dvb_dmx_swfilter from process context, tasklet
or interrupt handler 'as is'.

@Andreas:
Could you please explain in more detail what bad things might happen?

CU
Oliver

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

Re: [BUG] changeset 9029 (http://linuxtv.org/hg/v4l-dvb/rev/aa3e5cc1d833)

[Andreas Oberritter]

Oliver Endriss wrote:
> Trent Piepho wrote:
>> On Sun, 15 Feb 2009, Oliver Endriss wrote:
>>> e9hack wrote:
>>>> this change set is wrong. The affected functions cannot be called from an interrupt
>>>> context, because they may process large buffers. In this case, interrupts are disabled for
>>>> a long time. Functions, like dvb_dmx_swfilter_packets(), could be called only from a
>>>> tasklet. This change set does hide some strong design bugs in dm1105.c and au0828-dvb.c.
>>>>
>>>> Please revert this change set and do fix the bugs in dm1105.c and au0828-dvb.c (and other
>>>> files).
>>> @Mauro:
>>>
>>> This changeset _must_ be reverted! It breaks all kernels since 2.6.27
>>> for applications which use DVB and require a low interrupt latency.
>>>
>>> It is a very bad idea to call the demuxer to process data buffers with
>>> interrupts disabled!
>> I agree, this is bad.  The demuxer is far too much work to be done with
>> IRQs off.  IMHO, even doing it under a spin-lock is excessive.  It should
>> be a mutex.  Drivers should use a work-queue to feed the demuxer.
> 
> Agreed, this would be the best solution.
> 
> On the other hand, a workqueue handler would be scheduled later, so you
> need larger buffers in the driver. Some chipsets have very small
> buffers...
> 
> Anway, this would be a major change. All drivers must be carefully
> modified and tested for an extended period.
> 
> Meanwhile I had a look at the changeset, and I do not understand why
> spin_lock_irq... should be required everywhere.
> 
> Afaics a driver may safely call dvb_dmx_swfilter_packets,
> dvb_dmx_swfilter_204 or dvb_dmx_swfilter from process context, tasklet
> or interrupt handler 'as is'.
> 
> @Andreas:
> Could you please explain in more detail what bad things might happen?

To quote myself from the changelog: This fixes a deadlock discovered
by lockdep.

The lock is used in process context (e.g. DMX_START) and might also be
used from interrupt context (e.g. dvb_dmx_swfilter).

>From http://osdir.com/ml/kernel.janitors/2002-08/msg00022.html:

"spin_lock_irq disables local interrupts and then takes the spin_lock.
If you know you're in process context and other users may be in
interrupt context, this is the correct call to make.

spin_lock_irqsave saves local interrupt state into the flags variable,
disables interrupts, then takes the spin_lock.  spin_unlock_irqrestore
restores the local state saved in the flags.  Use this variant if you
don't know whether you're in interrupt or process context."

So, if the assumtions above are correct, then spin_lock_irq must be
used by all functions called from process context and
spin_lock_irqsave must be used by all functions called from an unknown
context.

Regards,
Andreas

Re: [linux-dvb] [BUG] changeset 9029 (http://linuxtv.org/hg/v4l-dvb/rev/aa3e5cc1d833)

[Steven Toth]

Trent Piepho wrote:
> On Mon, 16 Feb 2009, Steven Toth wrote:
>>> Hartmut, Oliver and Trent: Thanks for helping with this issue. I've just
>>> reverted the changeset. We still need a fix at dm1105, au0828-dvb and maybe
>>> other drivers that call the filtering routines inside IRQ's.
>> Fix the demux, add a worker thread and allow drivers to call it directly.
>>
>> I'm not a big fan of videobuf_dvb or having each driver do it's own thing as an
>> alternative.
>>
>> Fixing the demux... Would this require and extra buffer copy? probably, but it's
>> a trade-off between  the amount of spent during code management on a driver by
>> driver basis vs wrestling with videobuf_dvb and all of problems highlighted on
>> the ML over the last 2 years.
> 
> Have the driver copy the data into the demuxer from the interrupt handler
> with irqs disabled?  That's still too much.
> 
> The right way to do it is to have a queue of DMA buffers.  In the interrupt
> handler the driver takes the completed DMA buffer off the "to DMA" queue
> and puts it in the "to process" queue.  The driver should not copy and
> cetainly not demux the data from the interrupt handler.

I know what the right way is Trent (see cx23885) although thank you for 
reminding me. videobuf_dvb hasn't convinced people like me to bury myself in its 
mess or complexity during retro fits cases. Retro fitting videobuf_dvb into cx18 
(at the time) was way too much effort.

Retro fitting it into existing drivers can be painful and I haven't seen any 
volunteers stand up over the last 24 months to get this done.

My suggestion? For the most part we're talking about very low data rates for 
DVB, coupled with fast memory buses for memcpy's. If the group doesn't want 
calls to the sw_filter methods then implement a half-way-house replacement for 
those drivers - as I mentioned above - with the memcpy. Either this approach, or 
make videobuf_dvb mandatory and deprecate sw_filter_...n() to intensionally 
break drivers and force maintainers to comply. This will upset people.

Realistically, this thread will probably go on for a couple of days then trail 
off into nothing. The subject will rear it's head again in a few months, like 
it's done in the past.

A general question to the group: Who wants to volunteer to retro fit 
videobuf_dvb into the current drivers so we can avoid calls to sw_filter_...n() 
directly?

I'm willing to loan any hardware I have to the volunteer assuming we can work 
out something on shipping.

- Steve

Re: [linux-dvb] [BUG] changeset 9029 (http://linuxtv.org/hg/v4l-dvb/rev/aa3e5cc1d833)

[Andreas Oberritter]

Steven Toth wrote:
> A general question to the group: Who wants to volunteer to retro fit 
> videobuf_dvb into the current drivers so we can avoid calls to sw_filter_...n() 
> directly?

Can videobuf_dvb be used by hardware with MPEG decoders, too? My
(probably not up-to-date) impression was that it's been designed
specifically for "budget" cards with no demux or decoder hardware. But
the sw_filter functions are useful for those devices, too, i.e. to
feed a PID to multiple userspace clients or to provide section filters
where some kind of hardware only provides basic PID filtering.

Regards,
Andreas

Re: [linux-dvb] [BUG] changeset 9029 (http://linuxtv.org/hg/v4l-dvb/rev/aa3e5cc1d833)

[Oliver Endriss]

Andreas Oberritter wrote:
> Oliver Endriss wrote:
> > ...
> > @Andreas:
> > Could you please explain in more detail what bad things might happen?
> 
> To quote myself from the changelog: This fixes a deadlock discovered
> by lockdep.

I re-read the commit message, but it still does not ring any bells.
Could you please post the lockdep output?

> The lock is used in process context (e.g. DMX_START) and might also be
> used from interrupt context (e.g. dvb_dmx_swfilter).

Correct:
- dvb_dmx_swfilter uses spin_lock (called from irq, tasklet, whatever)
- DMX_START uses spin_lock_irq (called from process)
-> ok (see below).

> >From http://osdir.com/ml/kernel.janitors/2002-08/msg00022.html:
> 
> "spin_lock_irq disables local interrupts and then takes the spin_lock.
> If you know you're in process context and other users may be in
> interrupt context, this is the correct call to make.
> 
> spin_lock_irqsave saves local interrupt state into the flags variable,
> disables interrupts, then takes the spin_lock.  spin_unlock_irqrestore
> restores the local state saved in the flags.  Use this variant if you
> don't know whether you're in interrupt or process context."
> 
> So, if the assumtions above are correct, then spin_lock_irq must be
> used by all functions called from process context and
> spin_lock_irqsave must be used by all functions called from an unknown
> context.

Correct.

[1] If you want to lock a process against an interrupt handler,
- the process must use spin_lock_irq()
- the interrupt can use spin_lock()

A routine has to use spin_lock_irqsave if (and only if) process and irq
call the routine concurrently. I do not see yet how this might happen.

(Basically, the same happens when locking between tasklet and process
context, except that it is sufficient to use spin_lock_bh instead of
spin_lock_irq.)

Regards
Oliver

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

Re: [linux-dvb] [BUG] changeset 9029 (http://linuxtv.org/hg/v4l-dvb/rev/aa3e5cc1d833)

[Trent Piepho]

On Tue, 17 Feb 2009, Steven Toth wrote:
> Trent Piepho wrote:
> > On Mon, 16 Feb 2009, Steven Toth wrote:
> >>> Hartmut, Oliver and Trent: Thanks for helping with this issue. I've just
> >>> reverted the changeset. We still need a fix at dm1105, au0828-dvb and maybe
> >>> other drivers that call the filtering routines inside IRQ's.
> >> Fix the demux, add a worker thread and allow drivers to call it directly.
> >>
> >> I'm not a big fan of videobuf_dvb or having each driver do it's own thing as an
> >> alternative.
> >>
> >> Fixing the demux... Would this require and extra buffer copy? probably, but it's
> >> a trade-off between  the amount of spent during code management on a driver by
> >> driver basis vs wrestling with videobuf_dvb and all of problems highlighted on
> >> the ML over the last 2 years.
> >
> > Have the driver copy the data into the demuxer from the interrupt handler
> > with irqs disabled?  That's still too much.
> >
> > The right way to do it is to have a queue of DMA buffers.  In the interrupt
> > handler the driver takes the completed DMA buffer off the "to DMA" queue
> > and puts it in the "to process" queue.  The driver should not copy and
> > cetainly not demux the data from the interrupt handler.
>
> I know what the right way is Trent (see cx23885) although thank you for
> reminding me. videobuf_dvb hasn't convinced people like me to bury myself in its
> mess or complexity during retro fits cases. Retro fitting videobuf_dvb into cx18
> (at the time) was way too much effort.
>
> Retro fitting it into existing drivers can be painful and I haven't seen any
> volunteers stand up over the last 24 months to get this done.
>
> My suggestion? For the most part we're talking about very low data rates for
> DVB, coupled with fast memory buses for memcpy's. If the group doesn't want
> calls to the sw_filter methods then implement a half-way-house replacement for
> those drivers - as I mentioned above - with the memcpy. Either this approach, or

The problem is holding a spin lock with irqs disabled for a long amount of
time.  What exactly is your plan that will remove this, yet allow drivers
to process their buffers from an irq handler?

> A general question to the group: Who wants to volunteer to retro fit
> videobuf_dvb into the current drivers so we can avoid calls to sw_filter_...n()
> directly?

I don't see why videobuf_dvb is needed.

Re: [linux-dvb] [BUG] changeset 9029 (http://linuxtv.org/hg/v4l-dvb/rev/aa3e5cc1d833)

[Trent Piepho]

On Wed, 18 Feb 2009, Oliver Endriss wrote:
> [1] If you want to lock a process against an interrupt handler,
> - the process must use spin_lock_irq()
> - the interrupt can use spin_lock()
>
> A routine has to use spin_lock_irqsave if (and only if) process and irq
> call the routine concurrently. I do not see yet how this might happen.

Some code calls the swfilter functions from process context and some
drivers call them from interrupt context.

Re: [BUG] changeset 9029 (http://linuxtv.org/hg/v4l-dvb/rev/aa3e5cc1d833)

[Trent Piepho]

On Tue, 17 Feb 2009, Andreas Oberritter wrote:
> Oliver Endriss wrote:
> > Trent Piepho wrote:
> >> I agree, this is bad.  The demuxer is far too much work to be done with
> >> IRQs off.  IMHO, even doing it under a spin-lock is excessive.  It should
> >> be a mutex.  Drivers should use a work-queue to feed the demuxer.
> >
> > Agreed, this would be the best solution.
> >
> > On the other hand, a workqueue handler would be scheduled later, so you
> > need larger buffers in the driver. Some chipsets have very small
> > buffers...
> >
> > Anway, this would be a major change. All drivers must be carefully
> > modified and tested for an extended period.

Don't most drivers already feed the demuxer from process context and not
the irq handler?  What drivers _do_ have a problem?  I see pluto2 is one.
Anyone have documentation for this chip?

> So, if the assumtions above are correct, then spin_lock_irq must be
> used by all functions called from process context and
> spin_lock_irqsave must be used by all functions called from an unknown
> context.

I agree, to be correct that's what's necessary.  Some drivers call the
demuxer functions from interrupt context, so we have to use the irq
disabling functions everywhere.

But disabling irqs for demuxing causes too much latency.  The proper fix is
to not call the demuxer from interrupt context.

Re: [linux-dvb] [BUG] changeset 9029 (http://linuxtv.org/hg/v4l-dvb/rev/aa3e5cc1d833)

[Andreas Oberritter]

Oliver Endriss wrote:
> I re-read the commit message, but it still does not ring any bells.
> Could you please post the lockdep output?

No. I didn't save it.

Regards,
Andreas

Re: [linux-dvb] [BUG] changeset 9029 (http://linuxtv.org/hg/v4l-dvb/rev/aa3e5cc1d833)

[Steven Toth]

Trent Piepho wrote:
> On Tue, 17 Feb 2009, Steven Toth wrote:
>> Trent Piepho wrote:
>>> On Mon, 16 Feb 2009, Steven Toth wrote:
>>>>> Hartmut, Oliver and Trent: Thanks for helping with this issue. I've just
>>>>> reverted the changeset. We still need a fix at dm1105, au0828-dvb and maybe
>>>>> other drivers that call the filtering routines inside IRQ's.
>>>> Fix the demux, add a worker thread and allow drivers to call it directly.
>>>>
>>>> I'm not a big fan of videobuf_dvb or having each driver do it's own thing as an
>>>> alternative.
>>>>
>>>> Fixing the demux... Would this require and extra buffer copy? probably, but it's
>>>> a trade-off between  the amount of spent during code management on a driver by
>>>> driver basis vs wrestling with videobuf_dvb and all of problems highlighted on
>>>> the ML over the last 2 years.
>>> Have the driver copy the data into the demuxer from the interrupt handler
>>> with irqs disabled?  That's still too much.
>>>
>>> The right way to do it is to have a queue of DMA buffers.  In the interrupt
>>> handler the driver takes the completed DMA buffer off the "to DMA" queue
>>> and puts it in the "to process" queue.  The driver should not copy and
>>> cetainly not demux the data from the interrupt handler.
>> I know what the right way is Trent (see cx23885) although thank you for
>> reminding me. videobuf_dvb hasn't convinced people like me to bury myself in its
>> mess or complexity during retro fits cases. Retro fitting videobuf_dvb into cx18
>> (at the time) was way too much effort.
>>
>> Retro fitting it into existing drivers can be painful and I haven't seen any
>> volunteers stand up over the last 24 months to get this done.
>>
>> My suggestion? For the most part we're talking about very low data rates for
>> DVB, coupled with fast memory buses for memcpy's. If the group doesn't want
>> calls to the sw_filter methods then implement a half-way-house replacement for
>> those drivers - as I mentioned above - with the memcpy. Either this approach, or
> 
> The problem is holding a spin lock with irqs disabled for a long amount of
> time.  What exactly is your plan that will remove this, yet allow drivers
> to process their buffers from an irq handler?

That's not what I was suggesting. I was suggesting adding some ring buffer code 
and a worker thread for each driver context (done in a mythical demux->register 
func). This means that each driver get's it's own worker and ringbuffer. Driver 
mutex on your own ring buffer is localized to your instance of the driver. It 
would be up to your drivers worker thread (instantiated and managed incidentally 
by the demux core, not at irq level), to acquire the long term spinlock via 
sw_filter_n (already in demux core) underdiscussion and NOT block a driver 
calling demux->feedMyPersonalRingBuffer().

> 
>> A general question to the group: Who wants to volunteer to retro fit
>> videobuf_dvb into the current drivers so we can avoid calls to sw_filter_...n()
>> directly?
> 
> I don't see why videobuf_dvb is needed.

That was the point I was trying to make. IE. Push videobuf_dvb like behavior 
into the demux core, having drivers register, give each driver it's own worker 
thread and have that thread, running not in the interrupt context, feed the 
existing sw_filter_n() functions. The price is the cost of doing a memcpy of a 
low bitrate low frequency buffer into your demux's personal ring buffer. That 
has to be more efficient than the current drivers that feed sw_filter_n() 
directly, but not ideal. It's a half-way-house solution that consolidates 
complicated code into code, and keeps the drivers clean and easier to manage.

Trade off an infrequent memcpy on a low volume stream in < 50% of the drivers 
for a simplified approach. You don't have to do this for every driver, 
especially drivers that already implement videobuf_dvb, do it for the current 
problematic drivers...... or, have a volunteer add videobuf_dvb to all of the 
existing drivers.

- Steve

Re: [linux-dvb] [BUG] changeset 9029 (http://linuxtv.org/hg/v4l-dvb/rev/aa3e5cc1d833)

[Oliver Endriss]

Trent Piepho wrote:
> On Wed, 18 Feb 2009, Oliver Endriss wrote:
> > [1] If you want to lock a process against an interrupt handler,
> > - the process must use spin_lock_irq()
> > - the interrupt can use spin_lock()
> >
> > A routine has to use spin_lock_irqsave if (and only if) process and irq
> > call the routine concurrently. I do not see yet how this might happen.
> 
> Some code calls the swfilter functions from process context and some
> drivers call them from interrupt context.

There would be a problem if (and only if) it could happen concurrently
within a given driver. A driver may call the functions either from
process context or from a tasklet/irq.

User space access will occur only if demux_source == DMX_MEMORY_FE.
In this case the driver must not call the routine.

If demux_source == DMX_FRONTEND, the driver may call the routine,
but userspace won't.

Sorry, I need more information to identify the problem.

CU
Oliver

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

Re: [linux-dvb] [BUG] changeset 9029 (http://linuxtv.org/hg/v4l-dvb/rev/aa3e5cc1d833)

[Trent Piepho]

On Wed, 18 Feb 2009, Steven Toth wrote:
> Trent Piepho wrote:
> > On Tue, 17 Feb 2009, Steven Toth wrote:
> >> Trent Piepho wrote:
> >>> On Mon, 16 Feb 2009, Steven Toth wrote:
> >>>> Fixing the demux... Would this require and extra buffer copy? probably, but it's
> >>>> a trade-off between  the amount of spent during code management on a driver by
> >>>> driver basis vs wrestling with videobuf_dvb and all of problems highlighted on
> >>>> the ML over the last 2 years.
> >>> Have the driver copy the data into the demuxer from the interrupt handler
> >>> with irqs disabled?  That's still too much.
> >>>
> >>> The right way to do it is to have a queue of DMA buffers.  In the interrupt
> >>> handler the driver takes the completed DMA buffer off the "to DMA" queue
> >>> and puts it in the "to process" queue.  The driver should not copy and
> >>> cetainly not demux the data from the interrupt handler.
> >> I know what the right way is Trent (see cx23885) although thank you for
> >> reminding me. videobuf_dvb hasn't convinced people like me to bury myself in its
> >> mess or complexity during retro fits cases. Retro fitting videobuf_dvb into cx18
> >> (at the time) was way too much effort.
> >>
> >> Retro fitting it into existing drivers can be painful and I haven't seen any
> >> volunteers stand up over the last 24 months to get this done.
> >>
> >> My suggestion? For the most part we're talking about very low data rates for
> >> DVB, coupled with fast memory buses for memcpy's. If the group doesn't want
> >> calls to the sw_filter methods then implement a half-way-house replacement for
> >> those drivers - as I mentioned above - with the memcpy. Either this approach, or
> >
> > The problem is holding a spin lock with irqs disabled for a long amount of
> > time.  What exactly is your plan that will remove this, yet allow drivers
> > to process their buffers from an irq handler?
>
> That's not what I was suggesting. I was suggesting adding some ring buffer code
> and a worker thread for each driver context (done in a mythical demux->register
> func). This means that each driver get's it's own worker and ringbuffer. Driver

But won't this new ringbuffer be fed from interrupt context?  So instead of
feeding the demuxer from an irq, you are feeding the pre-demuxer ringbuffer
from an irq.  Isn't that going to have the same long term lock holding with
irqs disabled problem?

> >> A general question to the group: Who wants to volunteer to retro fit
> >> videobuf_dvb into the current drivers so we can avoid calls to sw_filter_...n()
> >> directly?
> >
> > I don't see why videobuf_dvb is needed.
>
> That was the point I was trying to make. IE. Push videobuf_dvb like behavior
> into the demux core, having drivers register, give each driver it's own worker
> thread and have that thread, running not in the interrupt context, feed the
> existing sw_filter_n() functions. The price is the cost of doing a memcpy of a

If you look at the pluto2 fix I did, the code to create a work queue is
very simple.  I don't think moving that to the demuxer would make the patch
to the driver much simpler.

The difficulty comes from not using a single buffer so the driver doesn't
need to hold a spin lock while it copies data out.