I think it looks good, must have missed it, so applied to the next version :-).
Thanks, Chris On Mon, Apr 18, 2005 at 07:13:53PM -0400, D. Hugh Redelmeier wrote: > In the following ancient message, I proposed a patch. I'm pretty sure > that this fixes a bug. > > How come it has been ignored? > > (The line numbers in the patch are slightly off now, but the patch > program should be able to apply it anyway.) > > ---------- Forwarded message ---------- > Date: Tue, 22 Feb 2005 20:31:53 -0500 (EST) > From: D. Hugh Redelmeier <[EMAIL PROTECTED]> > To: [email protected] > Subject: what is the correct type for struct api_cmd? > > On Thu, 27 Jan 2005, D. Hugh Redelmeier wrote: > > | Subject: Re: [ivtv-devel] Re: Patch for x86_64 and tons of output to syslog > > | Remember Axel's post in this thread? > | http://sourceforge.net/mailarchive/message.php?msg_id=10630737 > | I think that he was refering the the field "jiffies" in struct > | api_cmd, not the global variable with the same name. He asked: > | The current "u32 then" only interacts with jiffies which already is > | (unconditionally) a u64. Is this a 64bits issue at all? > | The field jiffies is declared u64. I find that a bit thought > | provoking. Perhaps it too should be "unsigned long". On the other > | hand, the name "api_cmd" suggests that this representation might be > | shared with code outside the driver and therefore that the layout > | cannot be unilaterally changed. Does anyone else know? > > Nobody has responded to this and I think that it matters. So let me > be more explicit in the hope of provoking a response from someone with > insight. > > The field "jiffies" is defined within the definition of struct api_cmd > in ivtv-driver.h. > > The only assignments to the field jiffies is in itv-mailbox.c. The > value assigned, in both cases, is the value of the global jiffies. > The global has type unsigned long. So the type unsigned long might be > a better type for the field -- it is the correct type for all values > assigned to the variable. > > > The only use of the value of field jiffies is in the same file > (reformatted): > if (itv->api[cmd].marked && > ((jiffies - itv->api[cmd].jiffies) < MBOX_TIMEOUT)) > > This test will be done in u64 (unless unsigned long is larger than 64 > bits, but that isn't the case in any C implementation I've seen). It > is done in u64 even if unsigned long is shorter than u64. This may > well create a problem since the global jiffies is expected to overflow > if unsigned long is only 32 bits (as on i386). > > Here's an example. Let's pretend that u64 holds numbers mod 10000 and > unsigned long holds numbers mod 100 just to make the arithmetic more > tractable. Assume that MBOX_TIMEOUT is 10 > > at time 0395: > itv->api[cmd].jiffies = jiffies; /* the value is 95 */ > > > at time 0402: > test ((jiffies - itv->api[cmd].jiffies) < MBOX_TIMEOUT) > == ((02 - 0095) < 10 > == ((0002 - 0095) < 10 > == 9907 < 10 > == FALSE > > the CORRECT version would be: > test ((jiffies - itv->api[cmd].jiffies) < MBOX_TIMEOUT) > == ((02 - 95) < 10 > == 07 < 10 > == TRUE > > The way to get the correct calculation is either to change the type of > the field to unsigned long, or cast it to that type to ensure that > the subtraction is done in unsigned long: > if (itv->api[cmd].marked && > ((jiffies - (unsigned long)itv->api[cmd].jiffies) < MBOX_TIMEOUT)) > > > All this suggests that the type of the jiffies field should be > unsigned long. What leaves me worried is that the struct name > includes the magic acronym "API". Is this struct an interface that > other parts of the kernel or userland know about? Can the type of the > field be changed without impacting code outside ivtv? > > My *guess* is that struct api_cmd is purely internal to ivtv and that > the type of field jiffies can and should be changed to unsigned long. > > I hereby propose the following (untested) patch to ivtv-0.3.2e. A > stylist would rename the field to something more descriptive and > distinct from the global's name. Perhaps "timestamp". Renaming would > require changing all uses of the name too. > > =================================================================== > RCS file: ivtv-0.3.2e/driver/RCS/ivtv-driver.h,v > retrieving revision 1.1 > diff -u -r1.1 ivtv-0.3.2e/driver/ivtv-driver.h > --- ivtv-0.3.2e/driver/ivtv-driver.h 2005/02/23 01:23:01 1.1 > +++ ivtv-0.3.2e/driver/ivtv-driver.h 2005/02/23 01:23:44 > @@ -820,7 +820,7 @@ > > struct api_cmd { > int marked; /* is this used */ > - u64 jiffies; /* last command issued */ > + unsigned long jiffies; /* timestamp of last command issued */ > > u32 s_data[IVTV_MBOX_MAX_DATA]; /* send api data */ > u32 r_data[IVTV_MBOX_MAX_DATA]; /* returned api data */ > ================ end ================ > > > ------------------------------------------------------- > This SF.Net email is sponsored by: New Crystal Reports XI. > Version 11 adds new functionality designed to reduce time involved in > creating, integrating, and deploying reporting solutions. Free runtime info, > new features, or free trial, at: http://www.businessobjects.com/devxi/728 > _______________________________________________ > ivtv-devel mailing list > [email protected] > https://lists.sourceforge.net/lists/listinfo/ivtv-devel -- --- Chris Kennedy / [EMAIL PROTECTED] Engineer KMOS-TV/KTBG-FM Broadcasting Services Department Central Missouri State University ------------------------------------------------------- This SF.Net email is sponsored by: New Crystal Reports XI. Version 11 adds new functionality designed to reduce time involved in creating, integrating, and deploying reporting solutions. Free runtime info, new features, or free trial, at: http://www.businessobjects.com/devxi/728 _______________________________________________ ivtv-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/ivtv-devel
