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
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?
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
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.
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?
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 <[email protected]>
--- 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;
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
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
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