* V4L2 drivers: potentially dangerous and inefficient msecs_to_jiffies() calculation @ 2009-09-14 21:07 Andreas Mohr 2009-09-14 21:34 ` Jiri Slaby ` (2 more replies) 0 siblings, 3 replies; 9+ messages in thread From: Andreas Mohr @ 2009-09-14 21:07 UTC (permalink / raw) To: Guennadi Liakhovetski, Mauro Carvalho Chehab Cc: Luca Risolia, linux-media, linux-kernel Hi all, ./drivers/media/video/sn9c102/sn9c102_core.c , ./drivers/media/video/et61x251/et61x251_core.c and ./drivers/media/video/zc0301/zc0301_core.c do cam->module_param.frame_timeout * 1000 * msecs_to_jiffies(1) ); multiple times each. What they should do instead is frame_timeout * msecs_to_jiffies(1000), I'd think. msecs_to_jiffies(1) is quite a bit too boldly assuming that all of the msecs_to_jiffies(x) implementation branches always round up. Not to mention that the current implementation needs one additional multiplication operation as opposed to constant-aggregating it into the msecs_to_jiffies() argument and thus nicely evaporating it into nirvana. HTH, Andreas Mohr ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: V4L2 drivers: potentially dangerous and inefficient msecs_to_jiffies() calculation 2009-09-14 21:07 V4L2 drivers: potentially dangerous and inefficient msecs_to_jiffies() calculation Andreas Mohr @ 2009-09-14 21:34 ` Jiri Slaby 2009-09-14 21:39 ` Andreas Mohr 2009-09-14 21:47 ` Trent Piepho 2009-09-15 19:14 ` Marcin Slusarz 2 siblings, 1 reply; 9+ messages in thread From: Jiri Slaby @ 2009-09-14 21:34 UTC (permalink / raw) To: Andreas Mohr Cc: Guennadi Liakhovetski, Mauro Carvalho Chehab, Luca Risolia, linux-media, linux-kernel On 09/14/2009 11:07 PM, Andreas Mohr wrote: > ./drivers/media/video/zc0301/zc0301_core.c > do > cam->module_param.frame_timeout * > 1000 * msecs_to_jiffies(1) ); > multiple times each. > What they should do instead is > frame_timeout * msecs_to_jiffies(1000), I'd think. In fact, msecs_to_jiffies(frame_timeout * 1000) makes much more sense. > msecs_to_jiffies(1) is quite a bit too boldly assuming > that all of the msecs_to_jiffies(x) implementation branches > always round up. They do, don't they? ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: V4L2 drivers: potentially dangerous and inefficient msecs_to_jiffies() calculation 2009-09-14 21:34 ` Jiri Slaby @ 2009-09-14 21:39 ` Andreas Mohr 2009-09-14 21:50 ` Jiri Slaby 0 siblings, 1 reply; 9+ messages in thread From: Andreas Mohr @ 2009-09-14 21:39 UTC (permalink / raw) To: Jiri Slaby Cc: Andreas Mohr, Guennadi Liakhovetski, Mauro Carvalho Chehab, Luca Risolia, linux-media, linux-kernel Hi, On Mon, Sep 14, 2009 at 11:34:40PM +0200, Jiri Slaby wrote: > On 09/14/2009 11:07 PM, Andreas Mohr wrote: > > ./drivers/media/video/zc0301/zc0301_core.c > > do > > cam->module_param.frame_timeout * > > 1000 * msecs_to_jiffies(1) ); > > multiple times each. > > What they should do instead is > > frame_timeout * msecs_to_jiffies(1000), I'd think. > > In fact, msecs_to_jiffies(frame_timeout * 1000) makes much more sense. Heh, right, even a bit better ;) > > msecs_to_jiffies(1) is quite a bit too boldly assuming > > that all of the msecs_to_jiffies(x) implementation branches > > always round up. > > They do, don't they? I'd hope so, but a slight risk remains, you never know, especially with 4+ or so variants... Andreas Mohr ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: V4L2 drivers: potentially dangerous and inefficient msecs_to_jiffies() calculation 2009-09-14 21:39 ` Andreas Mohr @ 2009-09-14 21:50 ` Jiri Slaby 2009-09-14 22:13 ` Andreas Mohr 0 siblings, 1 reply; 9+ messages in thread From: Jiri Slaby @ 2009-09-14 21:50 UTC (permalink / raw) To: Andreas Mohr Cc: Guennadi Liakhovetski, Mauro Carvalho Chehab, Luca Risolia, linux-media, linux-kernel On 09/14/2009 11:39 PM, Andreas Mohr wrote: > On Mon, Sep 14, 2009 at 11:34:40PM +0200, Jiri Slaby wrote: >> On 09/14/2009 11:07 PM, Andreas Mohr wrote: >>> msecs_to_jiffies(1) is quite a bit too boldly assuming >>> that all of the msecs_to_jiffies(x) implementation branches >>> always round up. >> >> They do, don't they? > > I'd hope so, but a slight risk remains, you never know, > especially with 4+ or so variants... A potential problem here is rather that it may wait longer due to returning 1 jiffie. It's then timeout * 1000 * 1. On 250HZ system it makes a difference of multiple of 4. Don't think it's a real issue in those drivers at all, but it's worth fixing. Care to post a patch? ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: V4L2 drivers: potentially dangerous and inefficient msecs_to_jiffies() calculation 2009-09-14 21:50 ` Jiri Slaby @ 2009-09-14 22:13 ` Andreas Mohr 0 siblings, 0 replies; 9+ messages in thread From: Andreas Mohr @ 2009-09-14 22:13 UTC (permalink / raw) To: Jiri Slaby Cc: Andreas Mohr, Guennadi Liakhovetski, Mauro Carvalho Chehab, Luca Risolia, linux-media, linux-kernel On Mon, Sep 14, 2009 at 11:50:50PM +0200, Jiri Slaby wrote: > A potential problem here is rather that it may wait longer due to > returning 1 jiffie. It's then timeout * 1000 * 1. On 250HZ system it > makes a difference of multiple of 4. Don't think it's a real issue in > those drivers at all, but it's worth fixing. Care to post a patch? *sigh*. :) OK, last thing to be done this day. Generated via precise copy&paste of the change between drivers (which is a flashing warning that they likely contain too much c&p anyway), plus: for file in sn9c102/sn9c102_core.c et61x251/et61x251_core.c zc0301/zc0301_core.c; do diff -upN linux-2.6.31/drivers/media/video/"$file" linux-2.6.31.patched/drivers/media/video/"$file" >> /tmp/v4l2_drivers.diff; done Disclaimer: FULLY UNTESTED, make sure to guard your hen house, use as dog food. ChangeLog: Correct dangerous and inefficient msecs_to_jiffies() calculation in some V4L2 drivers Signed-off-by: Andreas Mohr <andi@lisas.de> --- linux-2.6.31/drivers/media/video/sn9c102/sn9c102_core.c 2009-09-10 00:13:59.000000000 +0200 +++ linux-2.6.31.patched/drivers/media/video/sn9c102/sn9c102_core.c 2009-09-14 23:58:27.000000000 +0200 @@ -1954,8 +1954,10 @@ sn9c102_read(struct file* filp, char __u (!list_empty(&cam->outqueue)) || (cam->state & DEV_DISCONNECTED) || (cam->state & DEV_MISCONFIGURED), - cam->module_param.frame_timeout * - 1000 * msecs_to_jiffies(1) ); + msecs_to_jiffies( + cam->module_param.frame_timeout * 1000 + ) + ); if (timeout < 0) { mutex_unlock(&cam->fileop_mutex); return timeout; --- linux-2.6.31/drivers/media/video/et61x251/et61x251_core.c 2009-09-10 00:13:59.000000000 +0200 +++ linux-2.6.31.patched/drivers/media/video/et61x251/et61x251_core.c 2009-09-14 23:58:54.000000000 +0200 @@ -1379,8 +1379,10 @@ et61x251_read(struct file* filp, char __ (!list_empty(&cam->outqueue)) || (cam->state & DEV_DISCONNECTED) || (cam->state & DEV_MISCONFIGURED), - cam->module_param.frame_timeout * - 1000 * msecs_to_jiffies(1) ); + msecs_to_jiffies( + cam->module_param.frame_timeout * 1000 + ) + ); if (timeout < 0) { mutex_unlock(&cam->fileop_mutex); return timeout; --- linux-2.6.31/drivers/media/video/zc0301/zc0301_core.c 2009-09-10 00:13:59.000000000 +0200 +++ linux-2.6.31.patched/drivers/media/video/zc0301/zc0301_core.c 2009-09-15 00:00:14.000000000 +0200 @@ -819,8 +819,10 @@ zc0301_read(struct file* filp, char __us (!list_empty(&cam->outqueue)) || (cam->state & DEV_DISCONNECTED) || (cam->state & DEV_MISCONFIGURED), - cam->module_param.frame_timeout * - 1000 * msecs_to_jiffies(1) ); + msecs_to_jiffies( + cam->module_param.frame_timeout * 1000 + ) + ); if (timeout < 0) { mutex_unlock(&cam->fileop_mutex); return timeout; ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: V4L2 drivers: potentially dangerous and inefficient msecs_to_jiffies() calculation 2009-09-14 21:07 V4L2 drivers: potentially dangerous and inefficient msecs_to_jiffies() calculation Andreas Mohr 2009-09-14 21:34 ` Jiri Slaby @ 2009-09-14 21:47 ` Trent Piepho 2009-09-15 19:14 ` Marcin Slusarz 2 siblings, 0 replies; 9+ messages in thread From: Trent Piepho @ 2009-09-14 21:47 UTC (permalink / raw) To: Andreas Mohr Cc: Guennadi Liakhovetski, Mauro Carvalho Chehab, Luca Risolia, linux-media, linux-kernel On Mon, 14 Sep 2009, Andreas Mohr wrote: > cam->module_param.frame_timeout * > 1000 * msecs_to_jiffies(1) ); > multiple times each. > What they should do instead is > frame_timeout * msecs_to_jiffies(1000), I'd think. > msecs_to_jiffies(1) is quite a bit too boldly assuming > that all of the msecs_to_jiffies(x) implementation branches > always round up. It might also wait far longer than desired. On a 100 Hz kernel a jiffie is 10 ms. 1000 * msecs_to_jiffies(1) will wait 10 seconds instead of 1. But, I believe there is an issue with msecs_to_jiffies(x) overflowing for very high values of x. I'm not sure where that point is though. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: V4L2 drivers: potentially dangerous and inefficient msecs_to_jiffies() calculation 2009-09-14 21:07 V4L2 drivers: potentially dangerous and inefficient msecs_to_jiffies() calculation Andreas Mohr 2009-09-14 21:34 ` Jiri Slaby 2009-09-14 21:47 ` Trent Piepho @ 2009-09-15 19:14 ` Marcin Slusarz 2009-09-15 19:21 ` Andreas Mohr 2 siblings, 1 reply; 9+ messages in thread From: Marcin Slusarz @ 2009-09-15 19:14 UTC (permalink / raw) To: Andreas Mohr Cc: Guennadi Liakhovetski, Mauro Carvalho Chehab, Luca Risolia, linux-media, LKML, jirislaby Andreas Mohr pisze: > Hi all, > > ./drivers/media/video/sn9c102/sn9c102_core.c > , > ./drivers/media/video/et61x251/et61x251_core.c > and > ./drivers/media/video/zc0301/zc0301_core.c > do > cam->module_param.frame_timeout * > 1000 * msecs_to_jiffies(1) ); > multiple times each. > What they should do instead is > frame_timeout * msecs_to_jiffies(1000), I'd think. Or better: frame_timeout * HZ Marcin ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: V4L2 drivers: potentially dangerous and inefficient msecs_to_jiffies() calculation 2009-09-15 19:14 ` Marcin Slusarz @ 2009-09-15 19:21 ` Andreas Mohr 2009-09-19 10:08 ` Andreas Mohr 0 siblings, 1 reply; 9+ messages in thread From: Andreas Mohr @ 2009-09-15 19:21 UTC (permalink / raw) To: Marcin Slusarz Cc: Andreas Mohr, Guennadi Liakhovetski, Mauro Carvalho Chehab, Luca Risolia, linux-media, LKML, jirislaby Hi, On Tue, Sep 15, 2009 at 09:14:19PM +0200, Marcin Slusarz wrote: > Andreas Mohr pisze: > > ./drivers/media/video/zc0301/zc0301_core.c > > do > > cam->module_param.frame_timeout * > > 1000 * msecs_to_jiffies(1) ); > > multiple times each. > > What they should do instead is > > frame_timeout * msecs_to_jiffies(1000), I'd think. > > Or better: frame_timeout * HZ D'oh! ;-) But then what about the other 3 bazillion places in the kernel doing multiples of seconds? linux-2.6.31]$ find . -name "*.c"|xargs grep msecs_to_jiffies|grep 1000|wc -l 73 If this expression is really better (also/especially from a maintenance POV), then it should get changed. > Marcin Andreas Mohr ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: V4L2 drivers: potentially dangerous and inefficient msecs_to_jiffies() calculation 2009-09-15 19:21 ` Andreas Mohr @ 2009-09-19 10:08 ` Andreas Mohr 0 siblings, 0 replies; 9+ messages in thread From: Andreas Mohr @ 2009-09-19 10:08 UTC (permalink / raw) To: Andreas Mohr Cc: Marcin Slusarz, Guennadi Liakhovetski, Mauro Carvalho Chehab, Luca Risolia, linux-media, LKML, jirislaby Hi, On Tue, Sep 15, 2009 at 09:21:46PM +0200, Andreas Mohr wrote: > Hi, > > On Tue, Sep 15, 2009 at 09:14:19PM +0200, Marcin Slusarz wrote: > > Or better: frame_timeout * HZ > > D'oh! ;-) > > But then what about the other 3 bazillion places in the kernel > doing multiples of seconds? > > linux-2.6.31]$ find . -name "*.c"|xargs grep msecs_to_jiffies|grep 1000|wc -l > 73 > > If this expression is really better (also/especially from a maintenance POV), > then it should get changed. I just saw that my unmodified patch has been committed. I think that that is ok or even preferrable, since "HZ" is a lot less greppable (you'd have to use "\<HZ\>") or uniform than msecs_to_jiffies. In terms of "number of traps avoided" the expressions should be equivalent. Thanks! Andreas Mohr ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2009-09-19 10:08 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2009-09-14 21:07 V4L2 drivers: potentially dangerous and inefficient msecs_to_jiffies() calculation Andreas Mohr 2009-09-14 21:34 ` Jiri Slaby 2009-09-14 21:39 ` Andreas Mohr 2009-09-14 21:50 ` Jiri Slaby 2009-09-14 22:13 ` Andreas Mohr 2009-09-14 21:47 ` Trent Piepho 2009-09-15 19:14 ` Marcin Slusarz 2009-09-15 19:21 ` Andreas Mohr 2009-09-19 10:08 ` Andreas Mohr
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.