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

Commit Message

Andy Walls Feb. 15, 2009, 8:25 p.m. UTC

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 <[e-mail]>





--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

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);