use time_after macro in iphase.c

Hi Darren,

Haven't you read Matheus and Nishanths sugestions?

Quoted 22 lines · Darren Jenkins\

G'day list.

This is the second part of the split up patch and it applies on top of "[KJ][Patch][updated] use HZ relative values instead of absolute values in iphase.c"

Use the time_after macro's in iphase.c and remove a now redundant variable.

Compile tested with allyesconfig

Signed-off-by Darren Jenkins <[email protected]>

atm/iphase.c 2006-08-30 15:14:43.000000000 +1000 renamed from atm/iphase.c.patch1 2006-08-30 14:46:22.000000000 +1000 +0 -1

@@ -184,12 +184,11 @@ static u16 get_desc (IADEV *dev, struct
   u_short            desc_num, i;
   struct sk_buff        *skb;
   struct ia_vcc         *iavcc_r = NULL;
-  unsigned long delta;
   static unsigned long timer = 0;
   int ltimeout;
 

atm/iphase.c 2006-08-30 15:14:43.000000000 +1000 static u16 get_desc (IADEV *dev, struct · line 192

Quoted 4 lines

atm/iphase.c 2006-08-30 15:14:43.000000000 +1000 renamed from atm/iphase.c.patch1 2006-08-30 14:46:22.000000000 +1000 +1 -1

   ia_hack_tcq (dev);
-  if(((jiffies - timer)>HZ/2)||((dev->ffL.tcq_rd=dev->host_tcq_wr))){
+  if(time_after(jiffies, (timer+HZ/2))||((dev->ffL.tcq_rd=dev->host_tcq_wr))){
 

I guess Nishanth has already said something about this. Isn't supposed the driver to wait 50ms as the original code sugested? If yes, an arbitrary wait value may break its functionality. The kernel's default HZ value is 1000, but it can also change through make config! So I guess we can't relly too much on it's value for these wait statements. IMHO I believe msec_to_jiffies(50) would be the best approach here, since you would have the correct jiffies calculalation that will allow the driver to wait aproximatly 50ms! Please someone correct me if i'm wrong.

atm/iphase.c 2006-08-30 15:14:43.000000000 +1000 static u16 get_desc (IADEV *dev, struct · line 200

Quoted 11 lines

atm/iphase.c 2006-08-30 15:14:43.000000000 +1000 renamed from atm/iphase.c.patch1 2006-08-30 14:46:22.000000000 +1000 +0 -0

      timer = jiffies;
      i=0;

      while (i < dev->num_tx_desc) {

atm/iphase.c 2006-08-30 15:14:43.000000000 +1000 renamed from atm/iphase.c.patch1 2006-08-30 14:46:22.000000000 +1000 +0 -3

@@ -198,9 +197,11 @@ static u16 get_desc (IADEV *dev, struct
            continue;
         }
         ltimeout = dev->desc_tbl[i].iavcc->ltimeout;
-        delta = jiffies - dev->desc_tbl[i].timestamp;
-        if (delta >= ltimeout) {
-           IF_ABR(printk("RECOVER run!! desc_tbl %d = %d  delta = %ld, time = %ld\n", i,dev->desc_tbl[i].timestamp, delta, jiffies);)
 

You could also add a printk message level to this statement! (in a different patch).

atm/iphase.c 2006-08-30 15:14:43.000000000 +1000 static u16 get_desc (IADEV *dev, struct · line 205

Quoted 5 lines

atm/iphase.c 2006-08-30 15:14:43.000000000 +1000 renamed from atm/iphase.c.patch1 2006-08-30 14:46:22.000000000 +1000 +4 -0

+        if (time_after_eq(jiffies,
+             ((unsigned long)dev->desc_tbl[i].timestamp+ltimeout))) {
+           IF_ABR(printk("RECOVER run!! desc_tbl %d = %d  delta = %ld, time = %ld\n",
+             i, dev->desc_tbl[i].timestamp,
 

printk message level

atm/iphase.c 2006-08-30 15:14:43.000000000 +1000 static void rx_intr(struct atm_dev *dev) · line 1227

Quoted 10 lines

atm/iphase.c 2006-08-30 15:14:43.000000000 +1000 renamed from atm/iphase.c.patch1 2006-08-30 14:46:22.000000000 +1000 +1 -0

+             (jiffies-dev->desc_tbl[i].timestamp), jiffies);)
            if (dev->ffL.tcq_rd = dev->ffL.tcq_st)

               dev->ffL.tcq_rd =  dev->ffL.tcq_ed;
            else

atm/iphase.c 2006-08-30 15:14:43.000000000 +1000 renamed from atm/iphase.c.patch1 2006-08-30 14:46:22.000000000 +1000 +0 -1

@@ -1223,7 +1224,7 @@ static void rx_intr(struct atm_dev *dev)
         iadev->rx_tmp_jif = jiffies;
         iadev->rxing = 0;
      }
-     else if (((jiffies - iadev->rx_tmp_jif) > HZ/2) &&
 

The same above.

atm/iphase.c 2006-08-30 15:14:43.000000000 +1000 static void rx_intr(struct atm_dev *dev) · line 1229

Quoted 2 lines

atm/iphase.c 2006-08-30 15:14:43.000000000 +1000 renamed from atm/iphase.c.patch1 2006-08-30 14:46:22.000000000 +1000 +1 -0

+     else if (time_after(jiffies, ((unsigned long)iadev->rx_tmp_jif + HZ/2)) &&
 

The same above and...

Hmmm! If the iadev->rx_tmp_jif represents a temporary jiff timestamp, shouldn't be more correct to change it to and unsigned long type instead of casting it since it serves only that purpose? Despite unsigned int has enough precision to represent an unsigned long (4 bytes), thats a mistake. This approach should remove the necessity of casting and make the code and the variable usage far more clear.
Someone please correct me here if i'm wrong!

Quoted 5 lines

atm/iphase.c 2006-08-30 15:14:43.000000000 +1000 renamed from atm/iphase.c.patch1 2006-08-30 14:46:22.000000000 +1000 +0 -0

                ((iadev->rx_pkt_cnt - iadev->rx_tmp_cnt) = 0)) {

         for (i = 1; i <= iadev->num_rx_desc; i++)
                free_desc(dev, i);

Artur DAssumpcao

G'day Artur

Quoted 3 lines · Artur D'Assumpção

Hi Darren,

Haven't you read Matheus and Nishanths sugestions?

I thought I did.

Quoted 33 lines

Quoted 4 lines · Darren Jenkins\

G'day list.

This is the second part of the split up patch and it applies on top of "[KJ][Patch][updated] use HZ relative values instead of absolute

values

Quoted 9 lines

in iphase.c"

Use the time_after macro's in iphase.c and remove a now redundant variable.

Compile tested with allyesconfig

Signed-off-by Darren Jenkins <[email protected]>

--- drivers/atm/iphase.c.patch1 2006-08-30 14:46:22.000000000

+1000

atm/iphase.c static u16 get_desc (IADEV *dev, struct · line 192

Quoted 11 lines

+++ drivers/atm/iphase.c 2006-08-30 15:14:43.000000000 +1000

atm/iphase.c renamed from atm/iphase.c.patch1 +0 -2

@@ -184,12 +184,11 @@ static u16 get_desc (IADEV *dev, struct
   u_short            desc_num, i;
   struct sk_buff        *skb;
   struct ia_vcc         *iavcc_r = NULL;
-  unsigned long delta;
   static unsigned long timer = 0;
   int ltimeout;
 
   ia_hack_tcq (dev);
-  if(((jiffies - 

timer)>HZ/2)||((dev->ffL.tcq_rd=dev->host_tcq_wr))){

Review static u16 get_desc (IADEV *dev, struct · line 191

Quoted 1 line

(quoted diff) +1 -0

+  if(time_after(jiffies, 

(timer+HZ/2))||((dev->ffL.tcq_rd=dev->host_tcq_wr))){

Quoted 1 line

Quoted 8 lines

I guess Nishanth has already said something about this. Isn't supposed the driver to wait 50ms as the original code sugested? If yes, an arbitrary wait value may break its functionality. The kernel's default HZ value is 1000, but it can also change through make config! So I guess we can't relly too much on it's value for these wait statements. IMHO I believe msec_to_jiffies(50) would be the best approach here, since you would have the correct jiffies calculalation that will allow the driver to wait aproximatly 50ms! Please someone correct me if i'm wrong.

Ok the whole point of using HZ/2 is to make the value of HZ irrelevant to the time calculations.
The HZ value represents how many jiffies in a second, so HZ/2 represents the number of jiffies in half a second, no matter what the value of HZ is. So the code above checks if half a second has passed since timer was assigned it's value.
on line 192: timer = jiffies;

Now you are right about HZ being 1000 (on i386) before it was made configurable.
However it used to be 100 even before that (2.4), and Matthew Wilcox pointed out that the 2.4 driver used the same 50 values.
http://lists.osdl.org/pipermail/kernel-janitors/2006-August/006974.html
I checked and all the 50's in the 2.6 driver were 50's in the 2.4 driver. This means that the delay was supposed to be Half a second, (50 being half of the then HZ value 100) and that's why I used HZ/2 instead of HZ/20.
Now I didn't use msec_to_jiffies() like Nish suggested for a number of reasons.
I guess one is taste :), an other is that in a half second delay, the precision of HZ/2 is fine and I thought the code was just as clear. Also being a bit-pinching Uc guy I really don't like inline functions. ;)

Quoted 39 lines

atm/iphase.c static u16 get_desc (IADEV *dev, struct · line 203

Quoted 12 lines

      timer = jiffies;
      i=0;
      while (i < dev->num_tx_desc) {

atm/iphase.c renamed from atm/iphase.c.patch1 +0 -3

@@ -198,9 +197,11 @@ static u16 get_desc (IADEV *dev, struct
            continue;
         }
         ltimeout = dev->desc_tbl[i].iavcc->ltimeout;
-        delta = jiffies - dev->desc_tbl[i].timestamp;
-        if (delta >= ltimeout) {
-           IF_ABR(printk("RECOVER run!! desc_tbl %d = %d  delta = 

%ld, time = %ld\n", i,dev->desc_tbl[i].timestamp, delta, jiffies);)

Quoted 1 line

You could also add a printk message level to this statement! (in a different patch).

atm/iphase.c static u16 get_desc (IADEV *dev, struct

Quoted 3 lines

atm/iphase.c renamed from atm/iphase.c.patch1 +3 -0

+        if (time_after_eq(jiffies,
+             ((unsigned long)dev->desc_tbl[i].timestamp+ltimeout))) {
+           IF_ABR(printk("RECOVER run!! desc_tbl %d = %d  delta = 

%ld, time = %ld\n",

Quoted 2 lines

+ i, dev->desc_tbl[i].timestamp,

printk message level

atm/iphase.c static void rx_intr(struct atm_dev *dev) · line 1227

Quoted 10 lines

atm/iphase.c renamed from atm/iphase.c.patch1 +1 -1

+             (jiffies-dev->desc_tbl[i].timestamp), jiffies);)
            if (dev->ffL.tcq_rd = dev->ffL.tcq_st)
               dev->ffL.tcq_rd =  dev->ffL.tcq_ed;
            else

@@ -1223,7 +1224,7 @@ static void rx_intr(struct atm_dev *dev)
         iadev->rx_tmp_jif = jiffies;
         iadev->rxing = 0;
      }
-     else if (((jiffies - iadev->rx_tmp_jif) > HZ/2) &&
 

The same above.

Review static void rx_intr(struct atm_dev *dev) · line 1228

Quoted 1 line

(quoted diff) +1 -0

+     else if (time_after(jiffies, ((unsigned 

long)iadev->rx_tmp_jif + HZ/2)) &&

Quoted 1 line

The same above and...

Quoted 7 lines

Hmmm! If the iadev->rx_tmp_jif represents a temporary jiff timestamp, shouldn't be more correct to change it to and unsigned long type instead of casting it since it serves only that purpose? Despite unsigned int has enough precision to represent an unsigned long (4 bytes), thats a mistake. This approach should remove the necessity of casting and make the code and the variable usage far more clear.
Someone please correct me here if i'm wrong!

You are right here.
However this is a minor janitorial patch, and as I can't test my changes (beyond compilation) I wouldn't feel comfortable altering things without good reason
Have a look at the time_after() macro and friends. They check the type of the arguments are unsigned long. I wanted to make as little change as possible in the code so I just cast the arguments to required type so it doesn't generate warnings.

Quoted 7 lines

Quoted 6 lines

                ((iadev->rx_pkt_cnt - iadev->rx_tmp_cnt) = 0)) {
         for (i = 1; i <= iadev->num_rx_desc; i++)
                free_desc(dev, i);

Artur DAssumpcao

Darren J.

Yap, we've talked about that remember? I'm "flea" at #kerneljantors.

I understand very well and I respect your choices.

Anyway I have to disagree here in one point. I don't think this is a janitorial patch tough, but rather a potencial bug fix. When we're stick to i386 arch we know that the internal kernel representation of an unsigned long has 4 bytes (jiffies one, although it has 64bits) and rx_tmp_jif is stated has an u32 type. Until here I agree that a cast does resolve the nasty warnings, although it isn't very pretty to see if it's avoidable.
Anyway there are architectures where the internal unsigned long representation is 8 bytes wide and this timestamp may well loose precision here. It's difficult with this timer resolution for an 8byte precision to overflow (584 million years?!? :P), but an 4byte may well overflow (50 days at 1000 HZ). The time_after() function does handle well with unsigned long (4 byte) counter wrap arounds, but I don't know if because of the potencial different data types width at compare, there could be situations where this could actually fail.

I guess I'd prefer to stick with the unsigned long representation since it's the native jiffies one and leaves out doubts.

G'day again :).

Quoted 1 line · Artur D'Assumpção

Yap, we've talked about that remember? I'm "flea" at #kerneljantors.

G'day. You were right about the book, it is good. :)

Quoted 18 lines

I understand very well and I respect your choices.

Anyway I have to disagree here in one point. I don't think this is a janitorial patch tough, but rather a potencial bug fix. When we're stick to i386 arch we know that the internal kernel representation of an unsigned long has 4 bytes (jiffies one, although it has 64bits) and rx_tmp_jif is stated has an u32 type. Until here I agree that a cast does resolve the nasty warnings, although it isn't very pretty to see if it's avoidable.
Anyway there are architectures where the internal unsigned long representation is 8 bytes wide and this timestamp may well loose precision here. It's difficult with this timer resolution for an 8byte precision to overflow (584 million years?!? :P), but an 4byte may well overflow (50 days at 1000 HZ). The time_after() function does handle well with unsigned long (4 byte) counter wrap arounds, but I don't know if because of the potencial different data types width at compare, there could be situations where this could actually fail.

You are right again, however:
Looking at drivers/atm/Kconfig

292 depends on PCI && ATM && !64BIT

So no 64bit platforms. Also

303 See the file <file:Documentation/networking/iphase.txt> for further details.

Looking at that file

27 - Only x86 platforms are supported.

As we are stuck with i386 I think it is pretty safe, and that's one of the reasons why I didn't think there was a need to change it.

Quoted 3 lines

I guess I'd prefer to stick with the unsigned long representation since it's the native jiffies one and leaves out doubts.

Thats what I would do if I was going to change it.

Darren J.