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 ================ ------------------------------------------------------- SF email is sponsored by - The IT Product Guide Read honest & candid reviews on hundreds of IT Products from real users. Discover which products truly live up to the hype. Start reading now. http://ads.osdn.com/?ad_id=6595&alloc_id=14396&op=click _______________________________________________ ivtv-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/ivtv-devel
