polling loops: change exit condition to

PATCH In review Marcin Slusarz 3 participants 5 messages Dec 4, 2005 Kernel janitors

lore

Share & tools

lore

Marcin Slusarz

author

2005-12-04 00:24 UTC #0 lore raw

PATCH 20/21 polling loops: change exit condition to

who is responsible for those files?

Signed-off-by Marcin Slusarz <[email protected]>

drivers/serial/icom.c +7 -3

diff -upr -X linux-2.6.15-rc4/Documentation/dontdiff linux-2.6.15-rc4-orig/drivers/serial/icom.c linux-2.6.15-rc4/drivers/serial/icom.c
@@ -350,6 +350,7 @@ static void load_code(struct icom_port *
 	unsigned char *new_page = NULL;
 	unsigned char cable_id = NO_CABLE;
 	struct pci_dev *dev = icom_port->adapter->pci_dev;
+	unsigned long end_time;
 
 	/* Clear out any pending interrupts */
 	writew(0x3FFF, icom_port->int_reg);

@@ -461,13 +462,14 @@ static void load_code(struct icom_port *
 	writeb(START_DOWNLOAD, &icom_port->dram->sync);
 
 	/* Wait max 1 Sec for data download and processor to start */
-	for (index = 0; index < 10; index++) {
+	end_time = jiffies + msecs_to_jiffies(1000);
+	while (time_before(jiffies, end_time)) {
 		msleep(100);
 		if (readb(&icom_port->dram->misc_flags) & ICOM_HDW_ACTIVE)
 			break;
 	}
 
-	if (index = 10)
+	if (time_after_eq(jiffies, end_time))
 		status = -1;
 
 	/*

@@ -1015,11 +1017,13 @@ static void icom_send_xchar(struct uart_
 	unsigned char xdata;
 	int index;
 	unsigned long flags;
+	unsigned long end_time;
 
 	trace(ICOM_PORT, "SEND_XCHAR", ch);
 
 	/* wait .1 sec to send char */
-	for (index = 0; index < 10; index++) {
+	end_time = jiffies + msecs_to_jiffies(100);
+	while (time_before(jiffies, end_time)) {
 		spin_lock_irqsave(&port->lock, flags);
 		xdata = readb(&ICOM_PORT->dram->xchar);
 		if (xdata = 0x00) {

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

Jesper Juhl

2005-12-04 04:15 UTC #1 lore raw

Quoted 36 lines · Marcin Slusarz

who is responsible for those files?

Signed-off-by Marcin Slusarz <[email protected]>

diff -upr -X linux-2.6.15-rc4/Documentation/dontdiff linux-2.6.15-rc4-orig/drivers/serial/icom.c linux-2.6.15-rc4/drivers/serial/icom.c

drivers/serial/icom.c 2005-12-03 16:53:10.000000000 +0100 renamed from drivers/serial/icom.c 2005-11-20 16:53:29.000000000 +0100 +5 -2

@@ -350,6 +350,7 @@ static void load_code(struct icom_port *
        unsigned char *new_page = NULL;
        unsigned char cable_id = NO_CABLE;
        struct pci_dev *dev = icom_port->adapter->pci_dev;
+       unsigned long end_time;
 
        /* Clear out any pending interrupts */
        writew(0x3FFF, icom_port->int_reg);

@@ -461,13 +462,14 @@ static void load_code(struct icom_port *
        writeb(START_DOWNLOAD, &icom_port->dram->sync);
 
        /* Wait max 1 Sec for data download and processor to start */
-       for (index = 0; index < 10; index++) {
+       end_time = jiffies + msecs_to_jiffies(1000);
+       while (time_before(jiffies, end_time)) {
                msleep(100);
                if (readb(&icom_port->dram->misc_flags) & ICOM_HDW_ACTIVE)
                        break;
        }
 
-       if (index = 10)
+       if (time_after_eq(jiffies, end_time))
                status = -1;
 
        /*

@@ -1015,11 +1017,13 @@ static void icom_send_xchar(struct uart_
        unsigned char xdata;
        int index;
        unsigned long flags;
+       unsigned long end_time;

drivers/serial/icom.c 2005-12-03 16:53:10.000000000 +0100 static void icom_send_xchar(struct uart_ · line 1029

Quoted 10 lines

drivers/serial/icom.c 2005-12-03 16:53:10.000000000 +0100 renamed from drivers/serial/icom.c 2005-11-20 16:53:29.000000000 +0100 +2 -1

 
        trace(ICOM_PORT, "SEND_XCHAR", ch);
 
        /* wait .1 sec to send char */
-       for (index = 0; index < 10; index++) {
+       end_time = jiffies + msecs_to_jiffies(100);
+       while (time_before(jiffies, end_time)) {
                spin_lock_irqsave(&port->lock, flags);
                xdata = readb(&ICOM_PORT->dram->xchar);
                if (xdata = 0x00) {

Why add a new variable in this second part of the patch? end_time is only used once,so you could just do
while (time_before(jiffies, jiffies + msecs_to_jiffies(100))) {

Adding a new variable end_time in the first part of the patch makes sense since you use it more than once, but in the second part I don't see a need for it - I'd say save the extra variable and also save the assignment to it.

--
Jesper Juhl <[email protected]>
Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please http://www.expita.com/nomime.html

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

Roland Dreier

2005-12-04 05:06 UTC #2 lore raw

In reply to Marcin Slusarz

Quoted 3 lines

Why add a new variable in this second part of the patch? end_time is only used once,so you could just do
while (time_before(jiffies, jiffies + msecs_to_jiffies(100))) {

Umm, think about it... how could that loop ever exit?

The end_time has to be computed before the loop starts and then not change again.

With that said I'm not sure that these patches are a good idea. For example, what if in the code below:

drivers/serial/icom.c static void load_code(struct icom_port * · line 465

Quoted 3 lines

        /* Wait max 1 Sec for data download and processor to start */

drivers/serial/icom.c +1 -1

-       for (index = 0; index < 10; index++) {
+       end_time = jiffies + msecs_to_jiffies(1000);

this gets preempted for a while and never even enters the loop below:

Quoted 2 lines

+       while (time_before(jiffies, end_time)) {
                msleep(100);

then the function will fail without even checking the hardware once.

- R.

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

Jesper Juhl

2005-12-04 05:09 UTC #3 lore raw

In reply to Marcin Slusarz

Quoted 6 lines · Roland Dreier

Quoted 3 lines
Why add a new variable in this second part of the patch? end_time is only used once,so you could just do
while (time_before(jiffies, jiffies + msecs_to_jiffies(100))) {
Umm, think about it... how could that loop ever exit?

Ouch, you are right. I was not thinking straight.

--
Jesper Juhl <[email protected]>
Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please http://www.expita.com/nomime.html

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

Marcin Slusarz

author

2005-12-04 10:55 UTC #4 lore raw

In reply to Marcin Slusarz

Quoted 12 lines · Roland Dreier

The end_time has to be computed before the loop starts and then not change again.
With that said I'm not sure that these patches are a good idea. For example, what if in the code below:
drivers/serial/icom.c static void load_code(struct icom_port * · line 465
Quoted 3 lines
       /* Wait max 1 Sec for data download and processor to start */
drivers/serial/icom.c +1 -1
-       for (index = 0; index < 10; index++) {
+       end_time = jiffies + msecs_to_jiffies(1000);

Quoted 8 lines

this gets preempted for a while and never even enters the loop below:
Quoted 2 lines
+       while (time_before(jiffies, end_time)) {
               msleep(100);
then the function will fail without even checking the hardware once.

ok, so maybe while loops should be changed to do while loops?

ps: does anyone know why i don't get my own emails? (i checked subscription page and everything is fine there)

regards,
Marcin
_______________________________________________
Kernel-janitors mailing list
[email protected]
https://lists.osdl.org/mailman/listinfo/kernel-janitors