[KJ] [PATCH 18/39] net/core: use wait_event_timeout()

[KJ] [PATCH 18/39] net/core: use wait_event_timeout()

[Nishanth Aravamudan]

[-- Attachment #1: Type: text/plain, Size: 1974 bytes --]

Hi,

Please consider applying.

Description: Use wait_event_timeout() instead of custom wait-queue code.
The current code uses TASK_INTERRUPTIBLE but only cares about timing out and the
wq event taking place (does not actively do anything in response to signals), so
wait_event_timeout() should be ok.

Signed-off-by: Nishanth Aravamudan <nacc@us.ibm.com>

--- 2.6.11-rc1-kj-v/net/bluetooth/hidp/core.c	2005-01-15 16:55:44.000000000 -0800
+++ 2.6.11-rc1-kj/net/bluetooth/hidp/core.c	2005-01-18 11:53:24.000000000 -0800
@@ -36,6 +36,7 @@
 #include <linux/ioctl.h>
 #include <linux/file.h>
 #include <linux/init.h>
+#include <linux/wait.h>
 #include <net/sock.h>
 
 #include <linux/input.h>
@@ -325,7 +326,6 @@ static int hidp_session(void *arg)
 	struct sk_buff *skb;
 	int vendor = 0x0000, product = 0x0000;
 	wait_queue_t ctrl_wait, intr_wait;
-	unsigned long timeo = HZ;
 
 	BT_DBG("session %p", session);
 
@@ -370,28 +370,12 @@ static int hidp_session(void *arg)
 
 	hidp_del_timer(session);
 
-	if (intr_sk->sk_state != BT_CONNECTED) {
-		init_waitqueue_entry(&ctrl_wait, current);
-		add_wait_queue(ctrl_sk->sk_sleep, &ctrl_wait);
-		while (timeo && ctrl_sk->sk_state != BT_CLOSED) {
-			set_current_state(TASK_INTERRUPTIBLE);
-			timeo = schedule_timeout(timeo);
-		}
-		set_current_state(TASK_RUNNING);
-		remove_wait_queue(ctrl_sk->sk_sleep, &ctrl_wait);
-		timeo = HZ;
-	}
+	if (intr_sk->sk_state != BT_CONNECTED)
+		wait_event_timeout(ctrl_sk->sk_sleep, (ctrl_sk->sk_state == BT_CLOSED),	HZ);
 
 	fput(session->ctrl_sock->file);
 
-	init_waitqueue_entry(&intr_wait, current);
-	add_wait_queue(intr_sk->sk_sleep, &intr_wait);
-	while (timeo && intr_sk->sk_state != BT_CLOSED) {
-		set_current_state(TASK_INTERRUPTIBLE);
-		timeo = schedule_timeout(timeo);
-	}
-	set_current_state(TASK_RUNNING);
-	remove_wait_queue(intr_sk->sk_sleep, &intr_wait);
+	wait_event_timeout(intr_sk->sk_state, (intr_sk->sk_state == BT_CLOSED), HZ);
 
 	fput(session->intr_sock->file);
 

[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

[KJ] Re: [PATCH 18/39] net/core: use wait_event_timeout()

[Marcel Holtmann]

[-- Attachment #1: Type: text/plain, Size: 873 bytes --]

Hi Nishanth,

> Description: Use wait_event_timeout() instead of custom wait-queue code.
> The current code uses TASK_INTERRUPTIBLE but only cares about timing out and the
> wq event taking place (does not actively do anything in response to signals), so
> wait_event_timeout() should be ok.

this one results in:

  CC [M]  net/bluetooth/hidp/core.o
net/bluetooth/hidp/core.c: In function `hidp_session':
net/bluetooth/hidp/core.c:547: warning: passing arg 1 of `prepare_to_wait' from incompatible pointer type
net/bluetooth/hidp/core.c:547: warning: passing arg 1 of `finish_wait' from incompatible pointer type
net/bluetooth/hidp/core.c:551: warning: passing arg 1 of `prepare_to_wait' from incompatible pointer type
net/bluetooth/hidp/core.c:551: warning: passing arg 1 of `finish_wait' from incompatible pointer type

Please fix this and re-submit.

Regards

Marcel



[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

[Bluez-devel] Re: [PATCH 18/39] net/core: use wait_event_timeout()

[Marcel Holtmann]

Hi Nishanth,

> Description: Use wait_event_timeout() instead of custom wait-queue code.
> The current code uses TASK_INTERRUPTIBLE but only cares about timing out and the
> wq event taking place (does not actively do anything in response to signals), so
> wait_event_timeout() should be ok.

this one results in:

  CC [M]  net/bluetooth/hidp/core.o
net/bluetooth/hidp/core.c: In function `hidp_session':
net/bluetooth/hidp/core.c:547: warning: passing arg 1 of `prepare_to_wait' from incompatible pointer type
net/bluetooth/hidp/core.c:547: warning: passing arg 1 of `finish_wait' from incompatible pointer type
net/bluetooth/hidp/core.c:551: warning: passing arg 1 of `prepare_to_wait' from incompatible pointer type
net/bluetooth/hidp/core.c:551: warning: passing arg 1 of `finish_wait' from incompatible pointer type

Please fix this and re-submit.

Regards

Marcel




-------------------------------------------------------
This SF.Net email is sponsored by: IntelliVIEW -- Interactive Reporting
Tool for open source databases. Create drag-&-drop reports. Save time
by over 75%! Publish reports on the web. Export to DOC, XLS, RTF, etc.
Download a FREE copy at http://www.intelliview.com/go/osdn_nl
_______________________________________________
Bluez-devel mailing list
Bluez-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/bluez-devel

[KJ] [UPDATE PATCH 18/39] net/core: use wait_event_timeout()

[Nishanth Aravamudan]

[-- Attachment #1: Type: text/plain, Size: 2997 bytes --]

On Fri, Jan 21, 2005 at 01:32:14AM +0100, Marcel Holtmann wrote:
> Hi Nishanth,
> 
> > Description: Use wait_event_timeout() instead of custom wait-queue code.
> > The current code uses TASK_INTERRUPTIBLE but only cares about timing out and the
> > wq event taking place (does not actively do anything in response to signals), so
> > wait_event_timeout() should be ok.
> 
> this one results in:
> 
>   CC [M]  net/bluetooth/hidp/core.o
> net/bluetooth/hidp/core.c: In function `hidp_session':
> net/bluetooth/hidp/core.c:547: warning: passing arg 1 of `prepare_to_wait' from incompatible pointer type
> net/bluetooth/hidp/core.c:547: warning: passing arg 1 of `finish_wait' from incompatible pointer type
> net/bluetooth/hidp/core.c:551: warning: passing arg 1 of `prepare_to_wait' from incompatible pointer type
> net/bluetooth/hidp/core.c:551: warning: passing arg 1 of `finish_wait' from incompatible pointer type

Sorry for that, Marcel. The calling case in that function is slightly different
than usual...fixed below.

Thanks,
Nish

Description: Use wait_event_timeout() instead of custom wait-queue code.
The current code uses TASK_INTERRUPTIBLE but only cares about timing out and the
wq event taking place (does not actively do anything in response to signals), so
wait_event_timeout() should be ok.

Signed-off-by: Nishanth Aravamudan <nacc@us.ibm.com>


--- 2.6.11-rc1-kj-v/net/bluetooth/hidp/core.c	2005-01-15 16:55:44.000000000 -0800
+++ 2.6.11-rc1-kj/net/bluetooth/hidp/core.c	2005-01-21 11:07:11.000000000 -0800
@@ -36,6 +36,7 @@
 #include <linux/ioctl.h>
 #include <linux/file.h>
 #include <linux/init.h>
+#include <linux/wait.h>
 #include <net/sock.h>
 
 #include <linux/input.h>
@@ -325,7 +326,6 @@ static int hidp_session(void *arg)
 	struct sk_buff *skb;
 	int vendor = 0x0000, product = 0x0000;
 	wait_queue_t ctrl_wait, intr_wait;
-	unsigned long timeo = HZ;
 
 	BT_DBG("session %p", session);
 
@@ -370,28 +370,14 @@ static int hidp_session(void *arg)
 
 	hidp_del_timer(session);
 
-	if (intr_sk->sk_state != BT_CONNECTED) {
-		init_waitqueue_entry(&ctrl_wait, current);
-		add_wait_queue(ctrl_sk->sk_sleep, &ctrl_wait);
-		while (timeo && ctrl_sk->sk_state != BT_CLOSED) {
-			set_current_state(TASK_INTERRUPTIBLE);
-			timeo = schedule_timeout(timeo);
-		}
-		set_current_state(TASK_RUNNING);
-		remove_wait_queue(ctrl_sk->sk_sleep, &ctrl_wait);
-		timeo = HZ;
-	}
+	if (intr_sk->sk_state != BT_CONNECTED)
+		wait_event_timeout(*(ctrl_sk->sk_sleep),
+				(ctrl_sk->sk_state == BT_CLOSED), HZ);
 
 	fput(session->ctrl_sock->file);
 
-	init_waitqueue_entry(&intr_wait, current);
-	add_wait_queue(intr_sk->sk_sleep, &intr_wait);
-	while (timeo && intr_sk->sk_state != BT_CLOSED) {
-		set_current_state(TASK_INTERRUPTIBLE);
-		timeo = schedule_timeout(timeo);
-	}
-	set_current_state(TASK_RUNNING);
-	remove_wait_queue(intr_sk->sk_sleep, &intr_wait);
+	wait_event_timeout(*(ctrl_sk->sk_sleep),
+			(intr_sk->sk_state == BT_CLOSED), HZ);
 
 	fput(session->intr_sock->file);
 

[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

[UPDATE PATCH 18/39] net/core: use wait_event_timeout()

[Nishanth Aravamudan]

On Fri, Jan 21, 2005 at 01:32:14AM +0100, Marcel Holtmann wrote:
> Hi Nishanth,
> 
> > Description: Use wait_event_timeout() instead of custom wait-queue code.
> > The current code uses TASK_INTERRUPTIBLE but only cares about timing out and the
> > wq event taking place (does not actively do anything in response to signals), so
> > wait_event_timeout() should be ok.
> 
> this one results in:
> 
>   CC [M]  net/bluetooth/hidp/core.o
> net/bluetooth/hidp/core.c: In function `hidp_session':
> net/bluetooth/hidp/core.c:547: warning: passing arg 1 of `prepare_to_wait' from incompatible pointer type
> net/bluetooth/hidp/core.c:547: warning: passing arg 1 of `finish_wait' from incompatible pointer type
> net/bluetooth/hidp/core.c:551: warning: passing arg 1 of `prepare_to_wait' from incompatible pointer type
> net/bluetooth/hidp/core.c:551: warning: passing arg 1 of `finish_wait' from incompatible pointer type

Sorry for that, Marcel. The calling case in that function is slightly different
than usual...fixed below.

Thanks,
Nish

Description: Use wait_event_timeout() instead of custom wait-queue code.
The current code uses TASK_INTERRUPTIBLE but only cares about timing out and the
wq event taking place (does not actively do anything in response to signals), so
wait_event_timeout() should be ok.

Signed-off-by: Nishanth Aravamudan <nacc@us.ibm.com>


--- 2.6.11-rc1-kj-v/net/bluetooth/hidp/core.c	2005-01-15 16:55:44.000000000 -0800
+++ 2.6.11-rc1-kj/net/bluetooth/hidp/core.c	2005-01-21 11:07:11.000000000 -0800
@@ -36,6 +36,7 @@
 #include <linux/ioctl.h>
 #include <linux/file.h>
 #include <linux/init.h>
+#include <linux/wait.h>
 #include <net/sock.h>
 
 #include <linux/input.h>
@@ -325,7 +326,6 @@ static int hidp_session(void *arg)
 	struct sk_buff *skb;
 	int vendor = 0x0000, product = 0x0000;
 	wait_queue_t ctrl_wait, intr_wait;
-	unsigned long timeo = HZ;
 
 	BT_DBG("session %p", session);
 
@@ -370,28 +370,14 @@ static int hidp_session(void *arg)
 
 	hidp_del_timer(session);
 
-	if (intr_sk->sk_state != BT_CONNECTED) {
-		init_waitqueue_entry(&ctrl_wait, current);
-		add_wait_queue(ctrl_sk->sk_sleep, &ctrl_wait);
-		while (timeo && ctrl_sk->sk_state != BT_CLOSED) {
-			set_current_state(TASK_INTERRUPTIBLE);
-			timeo = schedule_timeout(timeo);
-		}
-		set_current_state(TASK_RUNNING);
-		remove_wait_queue(ctrl_sk->sk_sleep, &ctrl_wait);
-		timeo = HZ;
-	}
+	if (intr_sk->sk_state != BT_CONNECTED)
+		wait_event_timeout(*(ctrl_sk->sk_sleep),
+				(ctrl_sk->sk_state == BT_CLOSED), HZ);
 
 	fput(session->ctrl_sock->file);
 
-	init_waitqueue_entry(&intr_wait, current);
-	add_wait_queue(intr_sk->sk_sleep, &intr_wait);
-	while (timeo && intr_sk->sk_state != BT_CLOSED) {
-		set_current_state(TASK_INTERRUPTIBLE);
-		timeo = schedule_timeout(timeo);
-	}
-	set_current_state(TASK_RUNNING);
-	remove_wait_queue(intr_sk->sk_sleep, &intr_wait);
+	wait_event_timeout(*(ctrl_sk->sk_sleep),
+			(intr_sk->sk_state == BT_CLOSED), HZ);
 
 	fput(session->intr_sock->file);
 

[KJ] Re: [UPDATE PATCH 18/39] net/core: use wait_event_timeout()

[Marcel Holtmann]

[-- Attachment #1: Type: text/plain, Size: 1789 bytes --]

Hi Nishanth,

> Description: Use wait_event_timeout() instead of custom wait-queue code.
> The current code uses TASK_INTERRUPTIBLE but only cares about timing out and the
> wq event taking place (does not actively do anything in response to signals), so
> wait_event_timeout() should be ok.
> 
> Signed-off-by: Nishanth Aravamudan <nacc@us.ibm.com>
> 
> 
> --- 2.6.11-rc1-kj-v/net/bluetooth/hidp/core.c	2005-01-15 16:55:44.000000000 -0800
> +++ 2.6.11-rc1-kj/net/bluetooth/hidp/core.c	2005-01-21 11:07:11.000000000 -0800
> @@ -36,6 +36,7 @@
>  #include <linux/ioctl.h>
>  #include <linux/file.h>
>  #include <linux/init.h>
> +#include <linux/wait.h>
>  #include <net/sock.h>
>  
>  #include <linux/input.h>
> @@ -325,7 +326,6 @@ static int hidp_session(void *arg)
>  	struct sk_buff *skb;
>  	int vendor = 0x0000, product = 0x0000;
>  	wait_queue_t ctrl_wait, intr_wait;
> -	unsigned long timeo = HZ;
>  
>  	BT_DBG("session %p", session);
>  
> @@ -370,28 +370,14 @@ static int hidp_session(void *arg)
>  
>  	hidp_del_timer(session);
>  
> -	if (intr_sk->sk_state != BT_CONNECTED) {
> -		init_waitqueue_entry(&ctrl_wait, current);
> -		add_wait_queue(ctrl_sk->sk_sleep, &ctrl_wait);
> -		while (timeo && ctrl_sk->sk_state != BT_CLOSED) {
> -			set_current_state(TASK_INTERRUPTIBLE);
> -			timeo = schedule_timeout(timeo);
> -		}
> -		set_current_state(TASK_RUNNING);
> -		remove_wait_queue(ctrl_sk->sk_sleep, &ctrl_wait);
> -		timeo = HZ;
> -	}
> +	if (intr_sk->sk_state != BT_CONNECTED)
> +		wait_event_timeout(*(ctrl_sk->sk_sleep),
> +				(ctrl_sk->sk_state == BT_CLOSED), HZ);

and this does not make it look better. For me this is too much pointer
stuff here. I think the interface of wait_event_timeout() is wrong. It
should take a pointer to wait_queue_head_t.

Regards

Marcel



[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

[Bluez-devel] Re: [UPDATE PATCH 18/39] net/core: use wait_event_timeout()

[Marcel Holtmann]

Hi Nishanth,

> Description: Use wait_event_timeout() instead of custom wait-queue code.
> The current code uses TASK_INTERRUPTIBLE but only cares about timing out and the
> wq event taking place (does not actively do anything in response to signals), so
> wait_event_timeout() should be ok.
> 
> Signed-off-by: Nishanth Aravamudan <nacc@us.ibm.com>
> 
> 
> --- 2.6.11-rc1-kj-v/net/bluetooth/hidp/core.c	2005-01-15 16:55:44.000000000 -0800
> +++ 2.6.11-rc1-kj/net/bluetooth/hidp/core.c	2005-01-21 11:07:11.000000000 -0800
> @@ -36,6 +36,7 @@
>  #include <linux/ioctl.h>
>  #include <linux/file.h>
>  #include <linux/init.h>
> +#include <linux/wait.h>
>  #include <net/sock.h>
>  
>  #include <linux/input.h>
> @@ -325,7 +326,6 @@ static int hidp_session(void *arg)
>  	struct sk_buff *skb;
>  	int vendor = 0x0000, product = 0x0000;
>  	wait_queue_t ctrl_wait, intr_wait;
> -	unsigned long timeo = HZ;
>  
>  	BT_DBG("session %p", session);
>  
> @@ -370,28 +370,14 @@ static int hidp_session(void *arg)
>  
>  	hidp_del_timer(session);
>  
> -	if (intr_sk->sk_state != BT_CONNECTED) {
> -		init_waitqueue_entry(&ctrl_wait, current);
> -		add_wait_queue(ctrl_sk->sk_sleep, &ctrl_wait);
> -		while (timeo && ctrl_sk->sk_state != BT_CLOSED) {
> -			set_current_state(TASK_INTERRUPTIBLE);
> -			timeo = schedule_timeout(timeo);
> -		}
> -		set_current_state(TASK_RUNNING);
> -		remove_wait_queue(ctrl_sk->sk_sleep, &ctrl_wait);
> -		timeo = HZ;
> -	}
> +	if (intr_sk->sk_state != BT_CONNECTED)
> +		wait_event_timeout(*(ctrl_sk->sk_sleep),
> +				(ctrl_sk->sk_state == BT_CLOSED), HZ);

and this does not make it look better. For me this is too much pointer
stuff here. I think the interface of wait_event_timeout() is wrong. It
should take a pointer to wait_queue_head_t.

Regards

Marcel




-------------------------------------------------------
This SF.Net email is sponsored by: IntelliVIEW -- Interactive Reporting
Tool for open source databases. Create drag-&-drop reports. Save time
by over 75%! Publish reports on the web. Export to DOC, XLS, RTF, etc.
Download a FREE copy at http://www.intelliview.com/go/osdn_nl
_______________________________________________
Bluez-devel mailing list
Bluez-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/bluez-devel

Re: [KJ] Re: [UPDATE PATCH 18/39] net/core: use wait_event_timeout()

[Nish Aravamudan]

[-- Attachment #1: Type: text/plain, Size: 2992 bytes --]

On Sat, 22 Jan 2005 18:57:28 +0100, Marcel Holtmann <marcel@holtmann.org> wrote:
> Hi Nishanth,
> 
> > Description: Use wait_event_timeout() instead of custom wait-queue code.
> > The current code uses TASK_INTERRUPTIBLE but only cares about timing out and the
> > wq event taking place (does not actively do anything in response to signals), so
> > wait_event_timeout() should be ok.
> >
> > Signed-off-by: Nishanth Aravamudan <nacc@us.ibm.com>
> >
> >
> > --- 2.6.11-rc1-kj-v/net/bluetooth/hidp/core.c 2005-01-15 16:55:44.000000000 -0800
> > +++ 2.6.11-rc1-kj/net/bluetooth/hidp/core.c   2005-01-21 11:07:11.000000000 -0800
> > @@ -36,6 +36,7 @@
> >  #include <linux/ioctl.h>
> >  #include <linux/file.h>
> >  #include <linux/init.h>
> > +#include <linux/wait.h>
> >  #include <net/sock.h>
> >
> >  #include <linux/input.h>
> > @@ -325,7 +326,6 @@ static int hidp_session(void *arg)
> >       struct sk_buff *skb;
> >       int vendor = 0x0000, product = 0x0000;
> >       wait_queue_t ctrl_wait, intr_wait;
> > -     unsigned long timeo = HZ;
> >
> >       BT_DBG("session %p", session);
> >
> > @@ -370,28 +370,14 @@ static int hidp_session(void *arg)
> >
> >       hidp_del_timer(session);
> >
> > -     if (intr_sk->sk_state != BT_CONNECTED) {
> > -             init_waitqueue_entry(&ctrl_wait, current);
> > -             add_wait_queue(ctrl_sk->sk_sleep, &ctrl_wait);
> > -             while (timeo && ctrl_sk->sk_state != BT_CLOSED) {
> > -                     set_current_state(TASK_INTERRUPTIBLE);
> > -                     timeo = schedule_timeout(timeo);
> > -             }
> > -             set_current_state(TASK_RUNNING);
> > -             remove_wait_queue(ctrl_sk->sk_sleep, &ctrl_wait);
> > -             timeo = HZ;
> > -     }
> > +     if (intr_sk->sk_state != BT_CONNECTED)
> > +             wait_event_timeout(*(ctrl_sk->sk_sleep),
> > +                             (ctrl_sk->sk_state == BT_CLOSED), HZ);
> 
> and this does not make it look better. For me this is too much pointer
> stuff here. I think the interface of wait_event_timeout() is wrong. It
> should take a pointer to wait_queue_head_t.

Maybe; I'm not the designer of the interface, though, so I was just
trying to use what exists. To be honest, this particular driver is the
*only* one of a good number I've tried to convert that has this issue.
Everywhere else, {add,remove}_wait_queue() is called like so:

{add,remove}_wait_queue(&wq, &wait);

just as it is called via prepare_to_wait() in wait_event(). core.c is
the first driver I've come across that called the functions via:

{add,remove}_wait_queue(wq, &wait);

wait_event() is just a large macro which expands to the first form,
given wq as a parameter, so I think it should be ok, as that form is
the far more common one in the kernel currently.

What do you think? I am open to other ideas/suggestions, but I'm not
certain changing an existing / working interface (which other drivers
are being converted to) is the best idea.

Thanks,
Nish

[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] Re: [UPDATE PATCH 18/39] net/core: use wait_event_timeout()

[Nish Aravamudan]

On Sat, 22 Jan 2005 18:57:28 +0100, Marcel Holtmann <marcel@holtmann.org> wrote:
> Hi Nishanth,
> 
> > Description: Use wait_event_timeout() instead of custom wait-queue code.
> > The current code uses TASK_INTERRUPTIBLE but only cares about timing out and the
> > wq event taking place (does not actively do anything in response to signals), so
> > wait_event_timeout() should be ok.
> >
> > Signed-off-by: Nishanth Aravamudan <nacc@us.ibm.com>
> >
> >
> > --- 2.6.11-rc1-kj-v/net/bluetooth/hidp/core.c 2005-01-15 16:55:44.000000000 -0800
> > +++ 2.6.11-rc1-kj/net/bluetooth/hidp/core.c   2005-01-21 11:07:11.000000000 -0800
> > @@ -36,6 +36,7 @@
> >  #include <linux/ioctl.h>
> >  #include <linux/file.h>
> >  #include <linux/init.h>
> > +#include <linux/wait.h>
> >  #include <net/sock.h>
> >
> >  #include <linux/input.h>
> > @@ -325,7 +326,6 @@ static int hidp_session(void *arg)
> >       struct sk_buff *skb;
> >       int vendor = 0x0000, product = 0x0000;
> >       wait_queue_t ctrl_wait, intr_wait;
> > -     unsigned long timeo = HZ;
> >
> >       BT_DBG("session %p", session);
> >
> > @@ -370,28 +370,14 @@ static int hidp_session(void *arg)
> >
> >       hidp_del_timer(session);
> >
> > -     if (intr_sk->sk_state != BT_CONNECTED) {
> > -             init_waitqueue_entry(&ctrl_wait, current);
> > -             add_wait_queue(ctrl_sk->sk_sleep, &ctrl_wait);
> > -             while (timeo && ctrl_sk->sk_state != BT_CLOSED) {
> > -                     set_current_state(TASK_INTERRUPTIBLE);
> > -                     timeo = schedule_timeout(timeo);
> > -             }
> > -             set_current_state(TASK_RUNNING);
> > -             remove_wait_queue(ctrl_sk->sk_sleep, &ctrl_wait);
> > -             timeo = HZ;
> > -     }
> > +     if (intr_sk->sk_state != BT_CONNECTED)
> > +             wait_event_timeout(*(ctrl_sk->sk_sleep),
> > +                             (ctrl_sk->sk_state == BT_CLOSED), HZ);
> 
> and this does not make it look better. For me this is too much pointer
> stuff here. I think the interface of wait_event_timeout() is wrong. It
> should take a pointer to wait_queue_head_t.

Maybe; I'm not the designer of the interface, though, so I was just
trying to use what exists. To be honest, this particular driver is the
*only* one of a good number I've tried to convert that has this issue.
Everywhere else, {add,remove}_wait_queue() is called like so:

{add,remove}_wait_queue(&wq, &wait);

just as it is called via prepare_to_wait() in wait_event(). core.c is
the first driver I've come across that called the functions via:

{add,remove}_wait_queue(wq, &wait);

wait_event() is just a large macro which expands to the first form,
given wq as a parameter, so I think it should be ok, as that form is
the far more common one in the kernel currently.

What do you think? I am open to other ideas/suggestions, but I'm not
certain changing an existing / working interface (which other drivers
are being converted to) is the best idea.

Thanks,
Nish

[KJ] Re: [UPDATE PATCH 18/39] net/core: use wait_event_timeout()

[Marcel Holtmann]

[-- Attachment #1: Type: text/plain, Size: 3135 bytes --]

Hi Nish,

> > > Description: Use wait_event_timeout() instead of custom wait-queue code.
> > > The current code uses TASK_INTERRUPTIBLE but only cares about timing out and the
> > > wq event taking place (does not actively do anything in response to signals), so
> > > wait_event_timeout() should be ok.
> > >
> > > Signed-off-by: Nishanth Aravamudan <nacc@us.ibm.com>
> > >
> > >
> > > --- 2.6.11-rc1-kj-v/net/bluetooth/hidp/core.c 2005-01-15 16:55:44.000000000 -0800
> > > +++ 2.6.11-rc1-kj/net/bluetooth/hidp/core.c   2005-01-21 11:07:11.000000000 -0800
> > > @@ -36,6 +36,7 @@
> > >  #include <linux/ioctl.h>
> > >  #include <linux/file.h>
> > >  #include <linux/init.h>
> > > +#include <linux/wait.h>
> > >  #include <net/sock.h>
> > >
> > >  #include <linux/input.h>
> > > @@ -325,7 +326,6 @@ static int hidp_session(void *arg)
> > >       struct sk_buff *skb;
> > >       int vendor = 0x0000, product = 0x0000;
> > >       wait_queue_t ctrl_wait, intr_wait;
> > > -     unsigned long timeo = HZ;
> > >
> > >       BT_DBG("session %p", session);
> > >
> > > @@ -370,28 +370,14 @@ static int hidp_session(void *arg)
> > >
> > >       hidp_del_timer(session);
> > >
> > > -     if (intr_sk->sk_state != BT_CONNECTED) {
> > > -             init_waitqueue_entry(&ctrl_wait, current);
> > > -             add_wait_queue(ctrl_sk->sk_sleep, &ctrl_wait);
> > > -             while (timeo && ctrl_sk->sk_state != BT_CLOSED) {
> > > -                     set_current_state(TASK_INTERRUPTIBLE);
> > > -                     timeo = schedule_timeout(timeo);
> > > -             }
> > > -             set_current_state(TASK_RUNNING);
> > > -             remove_wait_queue(ctrl_sk->sk_sleep, &ctrl_wait);
> > > -             timeo = HZ;
> > > -     }
> > > +     if (intr_sk->sk_state != BT_CONNECTED)
> > > +             wait_event_timeout(*(ctrl_sk->sk_sleep),
> > > +                             (ctrl_sk->sk_state == BT_CLOSED), HZ);
> > 
> > and this does not make it look better. For me this is too much pointer
> > stuff here. I think the interface of wait_event_timeout() is wrong. It
> > should take a pointer to wait_queue_head_t.
> 
> Maybe; I'm not the designer of the interface, though, so I was just
> trying to use what exists. To be honest, this particular driver is the
> *only* one of a good number I've tried to convert that has this issue.
> Everywhere else, {add,remove}_wait_queue() is called like so:
> 
> {add,remove}_wait_queue(&wq, &wait);
> 
> just as it is called via prepare_to_wait() in wait_event(). core.c is
> the first driver I've come across that called the functions via:
> 
> {add,remove}_wait_queue(wq, &wait);
> 
> wait_event() is just a large macro which expands to the first form,
> given wq as a parameter, so I think it should be ok, as that form is
> the far more common one in the kernel currently.

we are using the wait queue from struct sock and sk_sleep is already a
pointer. Converting from and to a pointer and later to the struct again
to only match an API looks wrong to me and thus this API is wrong, but
this is only my thought.

Dave, any comments from you about this?

Regards

Marcel



[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

[Bluez-devel] Re: [UPDATE PATCH 18/39] net/core: use wait_event_timeout()

[Marcel Holtmann]

Hi Nish,

> > > Description: Use wait_event_timeout() instead of custom wait-queue code.
> > > The current code uses TASK_INTERRUPTIBLE but only cares about timing out and the
> > > wq event taking place (does not actively do anything in response to signals), so
> > > wait_event_timeout() should be ok.
> > >
> > > Signed-off-by: Nishanth Aravamudan <nacc@us.ibm.com>
> > >
> > >
> > > --- 2.6.11-rc1-kj-v/net/bluetooth/hidp/core.c 2005-01-15 16:55:44.000000000 -0800
> > > +++ 2.6.11-rc1-kj/net/bluetooth/hidp/core.c   2005-01-21 11:07:11.000000000 -0800
> > > @@ -36,6 +36,7 @@
> > >  #include <linux/ioctl.h>
> > >  #include <linux/file.h>
> > >  #include <linux/init.h>
> > > +#include <linux/wait.h>
> > >  #include <net/sock.h>
> > >
> > >  #include <linux/input.h>
> > > @@ -325,7 +326,6 @@ static int hidp_session(void *arg)
> > >       struct sk_buff *skb;
> > >       int vendor = 0x0000, product = 0x0000;
> > >       wait_queue_t ctrl_wait, intr_wait;
> > > -     unsigned long timeo = HZ;
> > >
> > >       BT_DBG("session %p", session);
> > >
> > > @@ -370,28 +370,14 @@ static int hidp_session(void *arg)
> > >
> > >       hidp_del_timer(session);
> > >
> > > -     if (intr_sk->sk_state != BT_CONNECTED) {
> > > -             init_waitqueue_entry(&ctrl_wait, current);
> > > -             add_wait_queue(ctrl_sk->sk_sleep, &ctrl_wait);
> > > -             while (timeo && ctrl_sk->sk_state != BT_CLOSED) {
> > > -                     set_current_state(TASK_INTERRUPTIBLE);
> > > -                     timeo = schedule_timeout(timeo);
> > > -             }
> > > -             set_current_state(TASK_RUNNING);
> > > -             remove_wait_queue(ctrl_sk->sk_sleep, &ctrl_wait);
> > > -             timeo = HZ;
> > > -     }
> > > +     if (intr_sk->sk_state != BT_CONNECTED)
> > > +             wait_event_timeout(*(ctrl_sk->sk_sleep),
> > > +                             (ctrl_sk->sk_state == BT_CLOSED), HZ);
> > 
> > and this does not make it look better. For me this is too much pointer
> > stuff here. I think the interface of wait_event_timeout() is wrong. It
> > should take a pointer to wait_queue_head_t.
> 
> Maybe; I'm not the designer of the interface, though, so I was just
> trying to use what exists. To be honest, this particular driver is the
> *only* one of a good number I've tried to convert that has this issue.
> Everywhere else, {add,remove}_wait_queue() is called like so:
> 
> {add,remove}_wait_queue(&wq, &wait);
> 
> just as it is called via prepare_to_wait() in wait_event(). core.c is
> the first driver I've come across that called the functions via:
> 
> {add,remove}_wait_queue(wq, &wait);
> 
> wait_event() is just a large macro which expands to the first form,
> given wq as a parameter, so I think it should be ok, as that form is
> the far more common one in the kernel currently.

we are using the wait queue from struct sock and sk_sleep is already a
pointer. Converting from and to a pointer and later to the struct again
to only match an API looks wrong to me and thus this API is wrong, but
this is only my thought.

Dave, any comments from you about this?

Regards

Marcel




-------------------------------------------------------
This SF.Net email is sponsored by: IntelliVIEW -- Interactive Reporting
Tool for open source databases. Create drag-&-drop reports. Save time
by over 75%! Publish reports on the web. Export to DOC, XLS, RTF, etc.
Download a FREE copy at http://www.intelliview.com/go/osdn_nl
_______________________________________________
Bluez-devel mailing list
Bluez-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/bluez-devel

[KJ] Re: [UPDATE PATCH 18/39] net/core: use wait_event_timeout()

[David S. Miller]

[-- Attachment #1: Type: text/plain, Size: 631 bytes --]

On Sun, 23 Jan 2005 10:21:08 +0100
Marcel Holtmann <marcel@holtmann.org> wrote:

> we are using the wait queue from struct sock and sk_sleep is already a
> pointer. Converting from and to a pointer and later to the struct again
> to only match an API looks wrong to me and thus this API is wrong, but
> this is only my thought.
> 
> Dave, any comments from you about this?

Not really.  I think the API is ok, it's a tradeoff between one set
of call types putting in a deref compared to the other.  And as the
patch author stated, the bluetooth side is definitely in the
minority.

To be honest, I think the decision is arbitrary.

[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [UPDATE PATCH 18/39] net/core: use wait_event_timeout()

[David S. Miller]

On Sun, 23 Jan 2005 10:21:08 +0100
Marcel Holtmann <marcel@holtmann.org> wrote:

> we are using the wait queue from struct sock and sk_sleep is already a
> pointer. Converting from and to a pointer and later to the struct again
> to only match an API looks wrong to me and thus this API is wrong, but
> this is only my thought.
> 
> Dave, any comments from you about this?

Not really.  I think the API is ok, it's a tradeoff between one set
of call types putting in a deref compared to the other.  And as the
patch author stated, the bluetooth side is definitely in the
minority.

To be honest, I think the decision is arbitrary.