net/islpci_dev: replace

Any comments would be appreciated.

Description: Use msleep() instead of schedule_timeout()
to guarantee the task delays as expected. Also set_current_state() is inserted before schedule_timeout(). If the for-loop were to execute twice, the second time would not set the state before sleeping in the current code; this causes schedule_timeout() to return immediately.

Signed-off-by Nishanth Aravamudan <[email protected]>

drivers/net/wireless/prism54/islpci_dev.c +1 -2

@@ -436,8 +436,7 @@ prism54_bring_down(islpci_private *priv)
 	wmb();
 
 	/* wait a while for the device to reset */
-	set_current_state(TASK_UNINTERRUPTIBLE);
-	schedule_timeout(50*HZ/1000);
+	msleep(50);
 
 	return 0;
 }

Quoted 11 lines

Any comments would be appreciated.

Description: Use msleep() instead of schedule_timeout()
to guarantee the task delays as expected. Also set_current_state() is inserted before schedule_timeout(). If the for-loop were to execute twice, the second time would not set the state before sleeping in the current code; this causes schedule_timeout() to return immediately.

Signed-off-by Nishanth Aravamudan <[email protected]>

---

2.6.9-rc2-vanilla/drivers/net/wireless/prism54/islpci_dev.c 2004-09-13 17:15:41.000000000 -0700

Quoted 1 line

+++

2.6.9-rc2/drivers/net/wireless/prism54/islpci_dev.c 2004-09-23 13:58:42.000000000 -0700

Quoted 17 lines

drivers/net/wireless/prism54/islpci_dev.c +2 -2

@@ -436,8 +436,7 @@ prism54_bring_down(islpci_private *priv)
      wmb();
 
      /* wait a while for the device to reset */
-     set_current_state(TASK_UNINTERRUPTIBLE);
-     schedule_timeout(50*HZ/1000);
+     msleep(50);
 
      return 0;
 }

@@ -489,6 +488,7 @@ islpci_reset_if(islpci_private *priv)
              /* The software reset acknowledge needs about 220 msec here.
              * Be conservative and wait for up to one second. */
 
+             set_current_state(TASK_UNINTERRUPTIBLE);
              remaining = schedule_timeout(HZ);
 

drivers/net/wireless/prism54/islpci_dev.c islpci_reset_if(islpci_private *priv) · line 494

Quoted 1 line

drivers/net/wireless/prism54/islpci_dev.c +0 -0

              if(remaining > 0) {

Looks good to me. IIRC Margit had something to say about this last time this popped around -- CC'ing her to see if there are any outstanding comments.

The patch has wrong line numbers. Doesn't take into account the stacked up netdev changes. (Therefore CC'ing Jeff Garzik)

This breaks 2.4 compatibility.
So either backport to 2.4 or Nish can take over prism54 2.4 maintenance ;-)

Don't say a backport is not possible/reasonable, it happened with netdev_priv().
(In 2.4.27; At least there, we have HAVE_NETDEV_PRIV).

If this is going to be forced, can we at least have a define HAVE_MSLEEP in delay.h ?

I am somewhat confused by the second part of the patch. What has that got to do with msleep ?

Actually, the fix would appear to be correct, but that is a seperate issue and nothing to do with msleep.
(Prims54 developers -> I'll take a look over the weekend)

I am sceptical about the whole msleep patchset as, by their own admission, the janitors have/can not (no hardware) test the majority of the changes. Even more worrying is that incorrect code has directly appeared in mainline kernel BK.

I would rather see an msleep implementation in 2.4.x...

At 00:15 01.10.2004 -0400, Jeff scribeth:

Quoted 1 line

I would rather see an msleep implementation in 2.4.x...

Hear, hear.
And can we PLEASE have a
"#define HAVE_MSLEEP" in delay.h ?!!!
(2.4. AND 2.6)

Quoted 7 lines

At 00:15 01.10.2004 -0400, Jeff scribeth:

Quoted 1 line

I would rather see an msleep implementation in 2.4.x...

Hear, hear.
And can we PLEASE have a
"#define HAVE_MSLEEP" in delay.h ?!!!
(2.4. AND 2.6)

No, this is not how the kernel source tree works, sorry.

At 09:55 01.10.2004 -0700, Greg scribeth:

Quoted 9 lines · Margit Schubert-While

Quoted 7 lines

At 00:15 01.10.2004 -0400, Jeff scribeth:

Quoted 1 line

I would rather see an msleep implementation in 2.4.x...

Hear, hear.
And can we PLEASE have a
"#define HAVE_MSLEEP" in delay.h ?!!!
(2.4. AND 2.6)

Quoted 1 line

No, this is not how the kernel source tree works, sorry.

Well it happened with netdev_priv ;-)
(HAVE_NETDEV_PRIV)

I would rather see an msleep implementation in 2.4.x...

diff -puN include/linux/delay.h~add-msleep-2.4 include/linux/delay.h

include/linux/delay.h renamed from include/linux/delay.h~add-msleep-2.4 +4 -0

@@ -34,4 +34,12 @@ extern unsigned long loops_per_jiffy;
 	({unsigned long msec=(n); while (msec--) udelay(1000);}))
 #endif
 
+void msleep(unsigned int msecs);
+unsigned long msleep_interruptible(unsigned int msecs);
+
+static inline void ssleep(unsigned int seconds)

Quoted 11 lines · maximilian attems

diff -puN include/linux/delay.h~add-msleep-2.4 include/linux/delay.h

include/linux/delay.h 2004-10-30 22:48:46.000000000 +0200 renamed from include/linux/delay.h~add-msleep-2.4 2004-10-30 22:48:46.000000000 +0200 +4 -0

@@ -34,4 +34,12 @@ extern unsigned long loops_per_jiffy;
      ({unsigned long msec=(n); while (msec--) udelay(1000);}))
 #endif
 
+void msleep(unsigned int msecs);
+unsigned long msleep_interruptible(unsigned int msecs);
+
+static inline void ssleep(unsigned int seconds)

[...]

Quoted 1 line

+static inline unsigned int jiffies_to_msecs(const unsigned long j)

include/linux/delay.h 2004-10-30 22:48:46.000000000 +0200

Quoted 3 lines

include/linux/delay.h 2004-10-30 22:48:46.000000000 +0200 renamed from include/linux/delay.h~add-msleep-2.4 2004-10-30 22:48:46.000000000 +0200 +2 -0

+static inline unsigned int jiffies_to_usecs(const unsigned long j)
 
+static inline unsigned long msecs_to_jiffies(const unsigned int m)

I'm pretty sure more than one of these symbols clashes with a symbol defined locally in a driver. I like the patch but we can't apply it until the impact on existing code is evaluated.

Quoted 15 lines · Jeff Garzik

Quoted 11 lines · maximilian attems

diff -puN include/linux/delay.h~add-msleep-2.4 include/linux/delay.h

include/linux/delay.h 2004-10-30 22:48:46.000000000 +0200 renamed from include/linux/delay.h~add-msleep-2.4 2004-10-30 22:48:46.000000000 +0200 +4 -0

@@ -34,4 +34,12 @@ extern unsigned long loops_per_jiffy;
      ({unsigned long msec=(n); while (msec--) udelay(1000);}))
 #endif
 
+void msleep(unsigned int msecs);
+unsigned long msleep_interruptible(unsigned int msecs);
+
+static inline void ssleep(unsigned int seconds)

[...]

Quoted 1 line

+static inline unsigned int jiffies_to_msecs(const unsigned long j)

Quoted 8 lines

include/linux/delay.h 2004-10-30 22:48:46.000000000 +0200

Quoted 3 lines

include/linux/delay.h 2004-10-30 22:48:46.000000000 +0200 renamed from include/linux/delay.h~add-msleep-2.4 2004-10-30 22:48:46.000000000 +0200 +2 -0

+static inline unsigned int jiffies_to_usecs(const unsigned long j)
 
+static inline unsigned long msecs_to_jiffies(const unsigned int m)

I'm pretty sure more than one of these symbols clashes with a symbol defined locally in a driver. I like the patch but we can't apply it until the impact on existing code is evaluated.

More than likely much of the code cleanup that was done before I began my patches, like removing custom msleep()s from drivers will need to be done again, as Jeff points out.

-Nish

Quoted 15 lines · Jeff Garzik

Quoted 11 lines · maximilian attems

diff -puN include/linux/delay.h~add-msleep-2.4 include/linux/delay.h

include/linux/delay.h 2004-10-30 22:48:46.000000000 +0200 renamed from include/linux/delay.h~add-msleep-2.4 2004-10-30 22:48:46.000000000 +0200 +4 -0

@@ -34,4 +34,12 @@ extern unsigned long loops_per_jiffy;
      ({unsigned long msec=(n); while (msec--) udelay(1000);}))
 #endif
 
+void msleep(unsigned int msecs);
+unsigned long msleep_interruptible(unsigned int msecs);
+
+static inline void ssleep(unsigned int seconds)

[...]

Quoted 1 line

+static inline unsigned int jiffies_to_msecs(const unsigned long j)

Quoted 8 lines

include/linux/delay.h 2004-10-30 22:48:46.000000000 +0200

Quoted 3 lines

include/linux/delay.h 2004-10-30 22:48:46.000000000 +0200 renamed from include/linux/delay.h~add-msleep-2.4 2004-10-30 22:48:46.000000000 +0200 +2 -0

+static inline unsigned int jiffies_to_usecs(const unsigned long j)
 
+static inline unsigned long msecs_to_jiffies(const unsigned int m)

I'm pretty sure more than one of these symbols clashes with a symbol defined locally in a driver. I like the patch but we can't apply it until the impact on existing code is evaluated.

More than likely much of the code cleanup that was done before I began my patches, like removing custom msleep()s from drivers will need to be done again, as Jeff points out.

-Nish

Quoted 15 lines · Jeff Garzik

Quoted 11 lines · maximilian attems

diff -puN include/linux/delay.h~add-msleep-2.4 include/linux/delay.h

include/linux/delay.h 2004-10-30 22:48:46.000000000 +0200 renamed from include/linux/delay.h~add-msleep-2.4 2004-10-30 22:48:46.000000000 +0200 +4 -0

@@ -34,4 +34,12 @@ extern unsigned long loops_per_jiffy;
      ({unsigned long msec=(n); while (msec--) udelay(1000);}))
 #endif
 
+void msleep(unsigned int msecs);
+unsigned long msleep_interruptible(unsigned int msecs);
+
+static inline void ssleep(unsigned int seconds)

[...]

Quoted 1 line

+static inline unsigned int jiffies_to_msecs(const unsigned long j)

Quoted 8 lines

include/linux/delay.h 2004-10-30 22:48:46.000000000 +0200

Quoted 3 lines

include/linux/delay.h 2004-10-30 22:48:46.000000000 +0200 renamed from include/linux/delay.h~add-msleep-2.4 2004-10-30 22:48:46.000000000 +0200 +2 -0

+static inline unsigned int jiffies_to_usecs(const unsigned long j)
 
+static inline unsigned long msecs_to_jiffies(const unsigned int m)

I'm pretty sure more than one of these symbols clashes with a symbol defined locally in a driver. I like the patch but we can't apply it until the impact on existing code is evaluated.

More than likely much of the code cleanup that was done before I began my patches, like removing custom msleep()s from drivers will need to be done again, as Jeff points out.

-Nish

Quoted 11 lines · maximilian attems

diff -puN include/linux/delay.h~add-msleep-2.4 include/linux/delay.h

include/linux/delay.h 2004-10-30 22:48:46.000000000 +0200 renamed from include/linux/delay.h~add-msleep-2.4 2004-10-30 22:48:46.000000000 +0200 +4 -0

@@ -34,4 +34,12 @@ extern unsigned long loops_per_jiffy;
      ({unsigned long msec=(n); while (msec--) udelay(1000);}))
 #endif
 
+void msleep(unsigned int msecs);
+unsigned long msleep_interruptible(unsigned int msecs);
+
+static inline void ssleep(unsigned int seconds)

[...]

Quoted 1 line

+static inline unsigned int jiffies_to_msecs(const unsigned long j)

include/linux/delay.h 2004-10-30 22:48:46.000000000 +0200

Quoted 3 lines

include/linux/delay.h 2004-10-30 22:48:46.000000000 +0200 renamed from include/linux/delay.h~add-msleep-2.4 2004-10-30 22:48:46.000000000 +0200 +2 -0

+static inline unsigned int jiffies_to_usecs(const unsigned long j)
 
+static inline unsigned long msecs_to_jiffies(const unsigned int m)

I'm pretty sure more than one of these symbols clashes with a symbol defined locally in a driver. I like the patch but we can't apply it until the impact on existing code is evaluated.

Quoted 11 lines · maximilian attems

diff -puN include/linux/delay.h~add-msleep-2.4 include/linux/delay.h

include/linux/delay.h 2004-10-30 22:48:46.000000000 +0200 renamed from include/linux/delay.h~add-msleep-2.4 2004-10-30 22:48:46.000000000 +0200 +4 -0

@@ -34,4 +34,12 @@ extern unsigned long loops_per_jiffy;
      ({unsigned long msec=(n); while (msec--) udelay(1000);}))
 #endif
 
+void msleep(unsigned int msecs);
+unsigned long msleep_interruptible(unsigned int msecs);
+
+static inline void ssleep(unsigned int seconds)

[...]

Quoted 1 line

+static inline unsigned int jiffies_to_msecs(const unsigned long j)

include/linux/delay.h 2004-10-30 22:48:46.000000000 +0200

Quoted 3 lines

include/linux/delay.h 2004-10-30 22:48:46.000000000 +0200 renamed from include/linux/delay.h~add-msleep-2.4 2004-10-30 22:48:46.000000000 +0200 +2 -0

+static inline unsigned int jiffies_to_usecs(const unsigned long j)
 
+static inline unsigned long msecs_to_jiffies(const unsigned int m)

I'm pretty sure more than one of these symbols clashes with a symbol defined locally in a driver. I like the patch but we can't apply it until the impact on existing code is evaluated.

I'm pretty sure more than one of these symbols clashes with a symbol defined locally in a driver. I like the patch but we can't apply it until the impact on existing code is evaluated.

Jeff

I'm pretty sure more than one of these symbols clashes with a symbol defined locally in a driver. I like the patch but we can't apply it until the impact on existing code is evaluated.

Jeff

I'm pretty sure more than one of these symbols clashes with a symbol defined locally in a driver. I like the patch but we can't apply it until the impact on existing code is evaluated.

Jeff