Em Mon, 01 Jun 2009 17:42:24 +0400 Abylai Ospan <aospan@xxxxxxxx> escreveu: > В Пнд, 01/06/2009 в 06:38 -0300, Mauro Carvalho Chehab пишет: > > Please check your patch against kernel codingstyle with checkpatch.pl. There > > are some CodingStyle violations here and on the code bellow. > Please pull new changes - http://udev.netup.ru/hg/v4l-dvb-aospan/rev/0d3e6c0695cc > > 1. checkpatch.pl don't show warnings/errors ( except printk line > length ). > > 2. cnt_storage array dinamically allocated before doing checks. We can't > allocate it in dvb_dmx_init routine because module option > "dvb_demux_tscheck" can be enabled on running system ( not in module > startup only ). > Ok, we're close to a final version. TS continuity check: show error message when discontinuity detected or TEI flag detected in header Signed-off-by: Abylay Ospan <aospan@xxxxxxxx> > --- a/linux/drivers/media/dvb/dvb-core/dvb_demux.c Mon Jun 01 05:22:37 2009 -0300 > +++ b/linux/drivers/media/dvb/dvb-core/dvb_demux.c Mon Jun 01 17:36:33 2009 +0400 > @@ -37,6 +37,15 @@ > ** #define DVB_DEMUX_SECTION_LOSS_LOG to monitor payload loss in the syslog > */ > // #define DVB_DEMUX_SECTION_LOSS_LOG > + > +static int dvb_demux_tscheck; > +module_param(dvb_demux_tscheck, int, 0644); > +MODULE_PARM_DESC(dvb_demux_tscheck, \ > + "enable transport stream continuity and TEI check"); Please remove the backslash on the above line. Just do: MODULE_PARM_DESC(dvb_demux_tscheck, "enable transport stream continuity and TEI check"); > + > +#define dprintk_tscheck(x...) do { \ > + if (dvb_demux_tscheck && printk_ratelimit()) printk(x); } while (0) checkpatch.pl is not perfect. The better is to break the macro as I've shown on my previous email, breaking one statement per line: #define dprintk_tscheck(x...) do { \ if (dvb_demux_tscheck && printk_ratelimit()) \ printk(x); \ } while (0) This allows a faster reading of the line. PS.: The defines are the only places at the source code where the backslashes are needed. > + > > /****************************************************************************** > * static inlined helper functions > @@ -375,6 +384,29 @@ > struct dvb_demux_feed *feed; > u16 pid = ts_pid(buf); > int dvr_done = 0; > + No need for an empty line here. Please remove to keep all vars together. > + int cnt_pid; unsigned cnt_pid; > + > + if (dvb_demux_tscheck) { > + No need for an empty line here. > + if (!demux->cnt_storage) > + demux->cnt_storage = vmalloc(0x1fff); Please add a define for 0x1fff and use the define, instead of using a magic value at vmalloc, like: #define MAX_PID 0x1ffe ... if (!demux->cnt_storage) demux->cnt_storage = vmalloc(MAX_PID + 1); You need to add a check to see if the vmalloc really worked. Also, if you don't have memory for the first packet, it doesn't make sense to keep insisting on allocating memory. Better just to disable the check. So, I would code it like: if (!demux->cnt_storage) { WARN("Couldn't allocate memory for TS/TEI check. Disabling it\n"); dvb_demux_tscheck = 0; goto no_dvb_demux_tscheck; } > + > + /* check pkt counter */ > + cnt_pid = ((buf[1] & 0x1f)<<8) | buf[2]; > + > + if (cnt_pid != 0x1fff) { if (cnt_pid <= MAX_PID) { This will make clear that there's no chance of writing outside the array limit, after changing cnt_pid to unsigned. > + if (buf[1] & 0x80) > + dprintk_tscheck("TEI detected. PID=0x%x data1=0x%x\n", cnt_pid, buf[1]); > + > + if ((buf[3] & 0xf) != demux->cnt_storage[cnt_pid]) > + dprintk_tscheck("TS packet counter mismatch. PID=0x%x expected 0x%x got 0x%x\n",\ > + cnt_pid, demux->cnt_storage[cnt_pid], buf[3] & 0xf); Please, don't add the backslash. Also, in order to have it 80-line compliant, you could break the strings as: dprintk_tscheck("TS packet counter mismatch. " "PID=0x%x expected 0x%x got 0x%x\n", cnt_pid, demux->cnt_storage[cnt_pid], buf[3] & 0xf); > + > + demux->cnt_storage[cnt_pid] = ((buf[3] & 0xf) + 1)&0xf; > + }; > + /* end check */ > + }; > In order to work with the lack of memory, you'll need a label here: no_dvb_demux_tscheck: Another alternative would be to convert the debug logic into a static function. The source code will look cleaner, and the gcc optimizer will tranform it into inline and produce the same binary. Something like: if (demux->cnt_storage) check_ts_continuity(); > list_for_each_entry(feed, &demux->feed_list, list_head) { > if ((feed->pid != pid) && (feed->pid != 0x2000)) > @@ -1160,6 +1192,8 @@ > int i; > struct dmx_demux *dmx = &dvbdemux->dmx; > No need for an empty line here. > + dvbdemux->cnt_storage = NULL; > + > dvbdemux->users = 0; > dvbdemux->filter = vmalloc(dvbdemux->filternum * sizeof(struct dvb_demux_filter)); > > @@ -1226,6 +1260,8 @@ > > void dvb_dmx_release(struct dvb_demux *dvbdemux) > { > + if (dvbdemux->cnt_storage) > + vfree(dvbdemux->cnt_storage); > vfree(dvbdemux->filter); > vfree(dvbdemux->feed); > } > --- a/linux/drivers/media/dvb/dvb-core/dvb_demux.h Mon Jun 01 05:22:37 2009 -0300 > +++ b/linux/drivers/media/dvb/dvb-core/dvb_demux.h Mon Jun 01 17:36:33 2009 +0400 > @@ -128,6 +128,8 @@ > > struct mutex mutex; > spinlock_t lock; > + > + uint8_t *cnt_storage; /* for TS continuity check */ > }; > > int dvb_dmx_init(struct dvb_demux *dvbdemux); Cheers, Mauro -- 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
