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@xxxxxxxxx> 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); -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
