V4L2 drivers: potentially dangerous and inefficient msecs_to_jiffies() calculation

V4L2 drivers: potentially dangerous and inefficient msecs_to_jiffies() calculation

[Andreas Mohr]

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

Re: V4L2 drivers: potentially dangerous and inefficient msecs_to_jiffies() calculation

[Jiri Slaby]

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?

Re: V4L2 drivers: potentially dangerous and inefficient msecs_to_jiffies() calculation

[Andreas Mohr]

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

Re: V4L2 drivers: potentially dangerous and inefficient msecs_to_jiffies() calculation

[Trent Piepho]

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.

Re: V4L2 drivers: potentially dangerous and inefficient msecs_to_jiffies() calculation

[Jiri Slaby]

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?

Re: V4L2 drivers: potentially dangerous and inefficient msecs_to_jiffies() calculation

[Andreas Mohr]

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;

Re: V4L2 drivers: potentially dangerous and inefficient msecs_to_jiffies() calculation

[Marcin Slusarz]

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

Re: V4L2 drivers: potentially dangerous and inefficient msecs_to_jiffies() calculation

[Andreas Mohr]

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

Re: V4L2 drivers: potentially dangerous and inefficient msecs_to_jiffies() calculation

[Andreas Mohr]

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