* [KJ] [PATCH] class/usblp: cleanup usblp_write()
@ 2004-10-22 22:56 Nishanth Aravamudan
2004-11-02 23:04 ` Nishanth Aravamudan
2004-11-03 17:55 ` Nishanth Aravamudan
0 siblings, 2 replies; 3+ messages in thread
From: Nishanth Aravamudan @ 2004-10-22 22:56 UTC (permalink / raw)
To: kernel-janitors
[-- Attachment #1: Type: text/PLAIN, Size: 1532 bytes --]
Hello,
In the process of cleaning up the remaining schedule_timeout()s in the
current kernel, I came across drivers/usb/class/usblp.c::usblp_write().
in this function, the second (counting by nesting) while-loop seemed
very confusing to me. I believe the following patch would have the same
result but is significantly cleaner. Would someone be able to test this
and / or verify my belief?
Thanks,
Nish
Description: The while-loop seemed excessively blocked with
conditionals. By reorganizing the code so timeout is the condition for
the loop and changing the checks within the loop, several lines of code
were removed.
Signed-off-by: Nishanth Aravamudan <nacc@us.ibm.com>
--- 2.6.9-bk7-vanilla/drivers/usb/class/usblp.c 2004-10-22 10:41:05.000000000 -0700
+++ 2.6.9-bk7/drivers/usb/class/usblp.c 2004-10-22 15:48:38.000000000 -0700
@@ -636,19 +636,16 @@ static ssize_t usblp_write(struct file *
timeout = USBLP_WRITE_TIMEOUT;
add_wait_queue(&usblp->wait, &wait);
- while ( 1==1 ) {
-
+ while (timeout) {
if (signal_pending(current)) {
remove_wait_queue(&usblp->wait, &wait);
return writecount ? writecount : -EINTR;
}
- set_current_state(TASK_INTERRUPTIBLE);
- if (timeout && !usblp->wcomplete) {
- timeout = schedule_timeout(timeout);
- } else {
- set_current_state(TASK_RUNNING);
+ if (usblp->wcomplete) {
break;
}
+ set_current_state(TASK_INTERRUPTIBLE);
+ timeout = schedule_timeout(timeout);
}
remove_wait_queue(&usblp->wait, &wait);
}
[-- 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
^ permalink raw reply [flat|nested] 3+ messages in thread* [KJ] [PATCH] class/usblp: cleanup usblp_write()
2004-10-22 22:56 [KJ] [PATCH] class/usblp: cleanup usblp_write() Nishanth Aravamudan
@ 2004-11-02 23:04 ` Nishanth Aravamudan
2004-11-03 17:55 ` Nishanth Aravamudan
1 sibling, 0 replies; 3+ messages in thread
From: Nishanth Aravamudan @ 2004-11-02 23:04 UTC (permalink / raw)
To: kernel-janitors
On Sat, Oct 23, 2004 at 02:20:00AM +0200, Oliver Neukum wrote:
> Am Samstag, 23. Oktober 2004 00:56 schrieb Nishanth Aravamudan:
> > + while (timeout) {
> > if (signal_pending(current)) {
> > remove_wait_queue(&usblp->wait, &wait);
> > return writecount ? writecount : -EINTR;
> > }
> > - set_current_state(TASK_INTERRUPTIBLE);
> > - if (timeout && !usblp->wcomplete) {
> > - timeout = schedule_timeout(timeout);
> > - } else {
> > - set_current_state(TASK_RUNNING);
> > + if (usblp->wcomplete) {
> > break;
> > }
> > + set_current_state(TASK_INTERRUPTIBLE);
>
> You can miss a wake up here.
>
> > + timeout = schedule_timeout(timeout);
> > }
>
> The order of checking wcomplete and setting the state must be reversed.
OK, thanks for the pointer. Please find the corrected patch below.
-Nish
Description: The while-loop seemed excessively blocked with
conditionals. By reorganizing the code so timeout is the condition for
the loop and changing the checks within the loop, several lines of code
were removed.
Signed-off-by: Nishanth Aravamudan <nacc@us.ibm.com>
--- 2.6.10-rc1-vanilla/drivers/usb/class/usblp.c 2004-10-30 15:33:37.000000000 -0700
+++ 2.6.10-rc1/drivers/usb/class/usblp.c 2004-11-02 15:01:28.000000000 -0800
@@ -636,19 +636,16 @@ static ssize_t usblp_write(struct file *
timeout = USBLP_WRITE_TIMEOUT;
add_wait_queue(&usblp->wait, &wait);
- while ( 1=1 ) {
-
+ while (timeout) {
if (signal_pending(current)) {
remove_wait_queue(&usblp->wait, &wait);
return writecount ? writecount : -EINTR;
}
set_current_state(TASK_INTERRUPTIBLE);
- if (timeout && !usblp->wcomplete) {
- timeout = schedule_timeout(timeout);
- } else {
- set_current_state(TASK_RUNNING);
+ if (usblp->wcomplete) {
break;
}
+ timeout = schedule_timeout(timeout);
}
remove_wait_queue(&usblp->wait, &wait);
}
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors
^ permalink raw reply [flat|nested] 3+ messages in thread* [KJ] [PATCH] class/usblp: cleanup usblp_write()
2004-10-22 22:56 [KJ] [PATCH] class/usblp: cleanup usblp_write() Nishanth Aravamudan
2004-11-02 23:04 ` Nishanth Aravamudan
@ 2004-11-03 17:55 ` Nishanth Aravamudan
1 sibling, 0 replies; 3+ messages in thread
From: Nishanth Aravamudan @ 2004-11-03 17:55 UTC (permalink / raw)
To: kernel-janitors
On Tue, Nov 02, 2004 at 03:04:15PM -0800, Nishanth Aravamudan wrote:
> On Sat, Oct 23, 2004 at 02:20:00AM +0200, Oliver Neukum wrote:
> > Am Samstag, 23. Oktober 2004 00:56 schrieb Nishanth Aravamudan:
> > > + while (timeout) {
> > > if (signal_pending(current)) {
> > > remove_wait_queue(&usblp->wait, &wait);
> > > return writecount ? writecount : -EINTR;
> > > }
> > > - set_current_state(TASK_INTERRUPTIBLE);
> > > - if (timeout && !usblp->wcomplete) {
> > > - timeout = schedule_timeout(timeout);
> > > - } else {
> > > - set_current_state(TASK_RUNNING);
> > > + if (usblp->wcomplete) {
> > > break;
> > > }
> > > + set_current_state(TASK_INTERRUPTIBLE);
> >
> > You can miss a wake up here.
> >
> > > + timeout = schedule_timeout(timeout);
> > > }
> >
> > The order of checking wcomplete and setting the state must be reversed.
>
> OK, thanks for the pointer. Please find the corrected patch below.
Sorry, I missed a reassignment of the state to TASK_RUNNING in this
patch. Thanks, Domen, for cathing this. Please find the (further)
corrected patch below.
Description: The while-loop seemed excessively blocked with
conditionals. By reorganizing the code so timeout is the condition for
the loop and changing the checks within the loop, several lines of code
were removed.
Signed-off-by: Nishanth Aravamudan <nacc@us.ibm.com>
--- 2.6.10-rc1-vanilla/drivers/usb/class/usblp.c 2004-10-30 15:33:37.000000000 -0700
+++ 2.6.10-rc1/drivers/usb/class/usblp.c 2004-11-03 09:54:41.000000000 -0800
@@ -636,19 +636,17 @@ static ssize_t usblp_write(struct file *
timeout = USBLP_WRITE_TIMEOUT;
add_wait_queue(&usblp->wait, &wait);
- while ( 1=1 ) {
-
+ while (timeout) {
if (signal_pending(current)) {
remove_wait_queue(&usblp->wait, &wait);
return writecount ? writecount : -EINTR;
}
set_current_state(TASK_INTERRUPTIBLE);
- if (timeout && !usblp->wcomplete) {
- timeout = schedule_timeout(timeout);
- } else {
+ if (usblp->wcomplete) {
set_current_state(TASK_RUNNING);
break;
}
+ timeout = schedule_timeout(timeout);
}
remove_wait_queue(&usblp->wait, &wait);
}
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2004-11-03 17:55 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2004-10-22 22:56 [KJ] [PATCH] class/usblp: cleanup usblp_write() Nishanth Aravamudan
2004-11-02 23:04 ` Nishanth Aravamudan
2004-11-03 17:55 ` Nishanth Aravamudan
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.