[KJ] [PATCH] class/usblp: cleanup usblp_write()

Nishanth Aravamudan

author

2004-10-22 22:56 UTC #0 lore raw

PATCH 1/1 class/usblp: cleanup usblp_write()

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 <[email protected]>

drivers/usb/class/usblp.c +4 -7

@@ -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);
 		}

_______________________________________________
Kernel-janitors mailing list
[email protected]
http://lists.osdl.org/mailman/listinfo/kernel-janitors

Nishanth Aravamudan

author

2004-11-02 23:04 UTC #1 lore raw

PATCH 1/1 class/usblp: cleanup usblp_write()

Quoted 16 lines · Oliver Neukum

Am Samstag, 23. Oktober 2004 00:56 schrieb Nishanth Aravamudan:
Quoted 6 lines
+                       while (timeout) {
                                if (signal_pending(current)) { remove_wait_queue(&usblp->wait, &wait); return writecount ? writecount : -EINTR; }
-                               set_current_state(TASK_INTERRUPTIBLE);
Quoted 8 lines
-                               if (timeout && !usblp->wcomplete) { -                                       timeout = schedule_timeout(timeout); -                               } else {
-                                       set_current_state(TASK_RUNNING); +                               if (usblp->wcomplete) {
                                        break;
                                }
+                               set_current_state(TASK_INTERRUPTIBLE);

Quoted 6 lines

You can miss a wake up here.
Quoted 2 lines
+ 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 <[email protected]>

drivers/usb/class/usblp.c +3 -6

@@ -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
[email protected]
http://lists.osdl.org/mailman/listinfo/kernel-janitors

Nishanth Aravamudan

author

2004-11-03 17:55 UTC #2 lore raw

PATCH 1/1 class/usblp: cleanup usblp_write()

In reply to Nishanth Aravamudan

Quoted 17 lines · Nishanth Aravamudan

Quoted 16 lines · Oliver Neukum
Am Samstag, 23. Oktober 2004 00:56 schrieb Nishanth Aravamudan:
Quoted 14 lines
+                       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);

Quoted 8 lines

Quoted 6 lines
You can miss a wake up here.
Quoted 2 lines
+ 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 <[email protected]>

drivers/usb/class/usblp.c +3 -5

@@ -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
[email protected]
http://lists.osdl.org/mailman/listinfo/kernel-janitors