* [PATCH] drivers/net/wan/wanxl.c: time_before(timeout, jiffies) -> jiffies, timeout @ 2008-03-20 15:43 Roel Kluin 2008-03-20 16:46 ` Joe Perches 2008-03-24 14:42 ` Krzysztof Halasa 0 siblings, 2 replies; 8+ messages in thread From: Roel Kluin @ 2008-03-20 15:43 UTC (permalink / raw) To: khc; +Cc: lkml Not tested, please confirm it's right --- fix reversal of timeout and jiffies Signed-off-by: Roel Kluin <12o3l@tiscali.nl> --- diff --git a/drivers/net/wan/wanxl.c b/drivers/net/wan/wanxl.c index d4aab8a..f61413b 100644 --- a/drivers/net/wan/wanxl.c +++ b/drivers/net/wan/wanxl.c @@ -650,7 +650,7 @@ static int __devinit wanxl_pci_init_one(struct pci_dev *pdev, timeout = jiffies + 20 * HZ; while ((stat = readl(card->plx + PLX_MAILBOX_0)) != 0) { - if (time_before(timeout, jiffies)) { + if (time_before(jiffies, timeout)) { printk(KERN_WARNING "wanXL %s: timeout waiting for" " PUTS to complete\n", pci_name(pdev)); wanxl_pci_remove_one(pdev); ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] drivers/net/wan/wanxl.c: time_before(timeout, jiffies) -> jiffies, timeout 2008-03-20 15:43 [PATCH] drivers/net/wan/wanxl.c: time_before(timeout, jiffies) -> jiffies, timeout Roel Kluin @ 2008-03-20 16:46 ` Joe Perches 2008-03-20 20:23 ` Roel Kluin 2008-03-24 15:01 ` Krzysztof Halasa 2008-03-24 14:42 ` Krzysztof Halasa 1 sibling, 2 replies; 8+ messages in thread From: Joe Perches @ 2008-03-20 16:46 UTC (permalink / raw) To: Roel Kluin; +Cc: khc, lkml On Thu, 2008-03-20 at 16:43 +0100, Roel Kluin wrote: > fix reversal of timeout and jiffies > diff --git a/drivers/net/wan/wanxl.c b/drivers/net/wan/wanxl.c > index d4aab8a..f61413b 100644 > --- a/drivers/net/wan/wanxl.c > +++ b/drivers/net/wan/wanxl.c > @@ -650,7 +650,7 @@ static int __devinit wanxl_pci_init_one(struct pci_dev *pdev, > > timeout = jiffies + 20 * HZ; > while ((stat = readl(card->plx + PLX_MAILBOX_0)) != 0) { > - if (time_before(timeout, jiffies)) { > + if (time_before(jiffies, timeout)) { > printk(KERN_WARNING "wanXL %s: timeout waiting for" > " PUTS to complete\n", pci_name(pdev)); > wanxl_pci_remove_one(pdev); Wouldn't it be better to have a schedule() in those while loops too? Maybe a more generic macro / statement expression would be more readable? perhaps something like (compiled/untested): drivers/net/wan/wanxl.c | 86 ++++++++++++++++------------------------------ 1 files changed, 30 insertions(+), 56 deletions(-) diff --git a/drivers/net/wan/wanxl.c b/drivers/net/wan/wanxl.c index d4aab8a..db1eabe 100644 --- a/drivers/net/wan/wanxl.c +++ b/drivers/net/wan/wanxl.c @@ -51,6 +51,19 @@ static const char* version = "wanXL serial card driver version: 0.48"; #define MBX2_MEMSZ_MASK 0xFFFF0000 /* PUTS Memory Size Register mask */ +#define TEST_UNTIL_TIMEOUT(test, hz) \ +({ \ + typeof test t; \ + unsigned long timeout = jiffies + hz; \ + do { \ + t = test; \ + if (t) \ + break; \ + schedule(); \ + } while (time_before(jiffies, timeout)); \ + t; \ +}) + typedef struct { struct net_device *dev; struct card_t *card; @@ -396,7 +409,6 @@ static int wanxl_open(struct net_device *dev) { port_t *port = dev_to_port(dev); u8 __iomem *dbr = port->card->plx + PLX_DOORBELL_TO_CARD; - unsigned long timeout; int i; if (get_status(port)->open) { @@ -412,18 +424,15 @@ static int wanxl_open(struct net_device *dev) /* signal the card */ writel(1 << (DOORBELL_TO_CARD_OPEN_0 + port->node), dbr); - timeout = jiffies + HZ; - do - if (get_status(port)->open) { - netif_start_queue(dev); - return 0; - } - while (time_after(timeout, jiffies)); + if (!TEST_UNTIL_TIMEOUT((get_status(port)->open), HZ)) { + printk(KERN_ERR "%s: unable to open port\n", dev->name); + /* ask the card to close the port, should it be still alive */ + writel(1 << (DOORBELL_TO_CARD_CLOSE_0 + port->node), dbr); + return -EFAULT; + } - printk(KERN_ERR "%s: unable to open port\n", dev->name); - /* ask the card to close the port, should it be still alive */ - writel(1 << (DOORBELL_TO_CARD_CLOSE_0 + port->node), dbr); - return -EFAULT; + netif_start_queue(dev); + return 0; } @@ -431,7 +440,6 @@ static int wanxl_open(struct net_device *dev) static int wanxl_close(struct net_device *dev) { port_t *port = dev_to_port(dev); - unsigned long timeout; int i; hdlc_close(dev); @@ -439,13 +447,7 @@ static int wanxl_close(struct net_device *dev) writel(1 << (DOORBELL_TO_CARD_CLOSE_0 + port->node), port->card->plx + PLX_DOORBELL_TO_CARD); - timeout = jiffies + HZ; - do - if (!get_status(port)->open) - break; - while (time_after(timeout, jiffies)); - - if (get_status(port)->open) + if (TEST_UNTIL_TIMEOUT((!get_status(port)->open), HZ)) printk(KERN_ERR "%s: unable to close port\n", dev->name); netif_stop_queue(dev); @@ -481,17 +483,11 @@ static struct net_device_stats *wanxl_get_stats(struct net_device *dev) static int wanxl_puts_command(card_t *card, u32 cmd) { - unsigned long timeout = jiffies + 5 * HZ; - writel(cmd, card->plx + PLX_MAILBOX_1); - do { - if (readl(card->plx + PLX_MAILBOX_1) == 0) - return 0; - - schedule(); - }while (time_after(timeout, jiffies)); + if (TEST_UNTIL_TIMEOUT((readl(card->plx + PLX_MAILBOX_1)), 5 * HZ)) + return -1; - return -1; + return 0; } @@ -649,27 +645,12 @@ static int __devinit wanxl_pci_init_one(struct pci_dev *pdev, #endif timeout = jiffies + 20 * HZ; - while ((stat = readl(card->plx + PLX_MAILBOX_0)) != 0) { - if (time_before(timeout, jiffies)) { - printk(KERN_WARNING "wanXL %s: timeout waiting for" - " PUTS to complete\n", pci_name(pdev)); - wanxl_pci_remove_one(pdev); - return -ENODEV; - } - - switch(stat & 0xC0) { - case 0x00: /* hmm - PUTS completed with non-zero code? */ - case 0x80: /* PUTS still testing the hardware */ - break; - - default: - printk(KERN_WARNING "wanXL %s: PUTS test 0x%X" + if (TEST_UNTIL_TIMEOUT(((stat = readl(card->plx + PLX_MAILBOX_0)) != 0), + 20 * HZ)) { + printk(KERN_WARNING "wanXL %s: PUTS test 0x%X" " failed\n", pci_name(pdev), stat & 0x30); wanxl_pci_remove_one(pdev); return -ENODEV; - } - - schedule(); } /* get on-board memory size (PUTS detects no more than 4 MB) */ @@ -734,15 +715,8 @@ static int __devinit wanxl_pci_init_one(struct pci_dev *pdev, return -ENODEV; } - stat = 0; - timeout = jiffies + 5 * HZ; - do { - if ((stat = readl(card->plx + PLX_MAILBOX_5)) != 0) - break; - schedule(); - }while (time_after(timeout, jiffies)); - - if (!stat) { + if (TEST_UNTIL_TIMEOUT(((stat = readl(card->plx + PLX_MAILBOX_5)) != 0), + 5 * HZ)) { printk(KERN_WARNING "wanXL %s: timeout while initializing card " "firmware\n", pci_name(pdev)); wanxl_pci_remove_one(pdev); ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] drivers/net/wan/wanxl.c: time_before(timeout, jiffies) -> jiffies, timeout 2008-03-20 16:46 ` Joe Perches @ 2008-03-20 20:23 ` Roel Kluin 2008-03-20 21:06 ` Joe Perches 2008-03-24 15:01 ` Krzysztof Halasa 1 sibling, 1 reply; 8+ messages in thread From: Roel Kluin @ 2008-03-20 20:23 UTC (permalink / raw) To: Joe Perches; +Cc: khc, lkml Joe Perches wrote: > On Thu, 2008-03-20 at 16:43 +0100, Roel Kluin wrote: >> fix reversal of timeout and jiffies > Wouldn't it be better to have a schedule() in those > while loops too? > > Maybe a more generic macro / statement expression > would be more readable? It appears more readable. > perhaps something like (compiled/untested): A few generic comments (not yet tested) > drivers/net/wan/wanxl.c | 86 ++++++++++++++++------------------------------ > 1 files changed, 30 insertions(+), 56 deletions(-) > > diff --git a/drivers/net/wan/wanxl.c b/drivers/net/wan/wanxl.c > index d4aab8a..db1eabe 100644 > --- a/drivers/net/wan/wanxl.c > +++ b/drivers/net/wan/wanxl.c > @@ -51,6 +51,19 @@ static const char* version = "wanXL serial card driver version: 0.48"; > #define MBX2_MEMSZ_MASK 0xFFFF0000 /* PUTS Memory Size Register mask */ > > > +#define TEST_UNTIL_TIMEOUT(test, hz) \ > +({ \ > + typeof test t; \ typeof(test) t; \ will this work if test is a function? > + unsigned long timeout = jiffies + hz; \ unsigned long timeout = jiffies + (hz); \ > + do { \ > + t = test; \ t = (test); \ > + if (t) \ > + break; \ > + schedule(); \ > + } while (time_before(jiffies, timeout)); \ > + t; \ > +}) > + > typedef struct { > struct net_device *dev; > struct card_t *card; > @@ -396,7 +409,6 @@ static int wanxl_open(struct net_device *dev) > { > port_t *port = dev_to_port(dev); > u8 __iomem *dbr = port->card->plx + PLX_DOORBELL_TO_CARD; > - unsigned long timeout; > int i; > > if (get_status(port)->open) { > @@ -412,18 +424,15 @@ static int wanxl_open(struct net_device *dev) > /* signal the card */ > writel(1 << (DOORBELL_TO_CARD_OPEN_0 + port->node), dbr); > > - timeout = jiffies + HZ; > - do > - if (get_status(port)->open) { > - netif_start_queue(dev); > - return 0; > - } > - while (time_after(timeout, jiffies)); > + if (!TEST_UNTIL_TIMEOUT((get_status(port)->open), HZ)) { > + printk(KERN_ERR "%s: unable to open port\n", dev->name); > + /* ask the card to close the port, should it be still alive */ > + writel(1 << (DOORBELL_TO_CARD_CLOSE_0 + port->node), dbr); > + return -EFAULT; > + } > > - printk(KERN_ERR "%s: unable to open port\n", dev->name); > - /* ask the card to close the port, should it be still alive */ > - writel(1 << (DOORBELL_TO_CARD_CLOSE_0 + port->node), dbr); > - return -EFAULT; > + netif_start_queue(dev); > + return 0; > } > > > @@ -431,7 +440,6 @@ static int wanxl_open(struct net_device *dev) > static int wanxl_close(struct net_device *dev) > { > port_t *port = dev_to_port(dev); > - unsigned long timeout; > int i; > > hdlc_close(dev); > @@ -439,13 +447,7 @@ static int wanxl_close(struct net_device *dev) > writel(1 << (DOORBELL_TO_CARD_CLOSE_0 + port->node), > port->card->plx + PLX_DOORBELL_TO_CARD); > > - timeout = jiffies + HZ; > - do > - if (!get_status(port)->open) > - break; > - while (time_after(timeout, jiffies)); > - > - if (get_status(port)->open) > + if (TEST_UNTIL_TIMEOUT((!get_status(port)->open), HZ)) > printk(KERN_ERR "%s: unable to close port\n", dev->name); > > netif_stop_queue(dev); > @@ -481,17 +483,11 @@ static struct net_device_stats *wanxl_get_stats(struct net_device *dev) > > static int wanxl_puts_command(card_t *card, u32 cmd) > { > - unsigned long timeout = jiffies + 5 * HZ; > - > writel(cmd, card->plx + PLX_MAILBOX_1); > - do { > - if (readl(card->plx + PLX_MAILBOX_1) == 0) > - return 0; > - > - schedule(); > - }while (time_after(timeout, jiffies)); > + if (TEST_UNTIL_TIMEOUT((readl(card->plx + PLX_MAILBOX_1)), 5 * HZ)) > + return -1; > > - return -1; > + return 0; > } > > > @@ -649,27 +645,12 @@ static int __devinit wanxl_pci_init_one(struct pci_dev *pdev, > #endif > > timeout = jiffies + 20 * HZ; you can remove the line above as well > - while ((stat = readl(card->plx + PLX_MAILBOX_0)) != 0) { > - if (time_before(timeout, jiffies)) { > - printk(KERN_WARNING "wanXL %s: timeout waiting for" > - " PUTS to complete\n", pci_name(pdev)); > - wanxl_pci_remove_one(pdev); > - return -ENODEV; > - } > - > - switch(stat & 0xC0) { > - case 0x00: /* hmm - PUTS completed with non-zero code? */ > - case 0x80: /* PUTS still testing the hardware */ > - break; > - > - default: > - printk(KERN_WARNING "wanXL %s: PUTS test 0x%X" > + if (TEST_UNTIL_TIMEOUT(((stat = readl(card->plx + PLX_MAILBOX_0)) != 0), > + 20 * HZ)) { > + printk(KERN_WARNING "wanXL %s: PUTS test 0x%X" > " failed\n", pci_name(pdev), stat & 0x30); > wanxl_pci_remove_one(pdev); > return -ENODEV; Doesn't this change behavior for (stat & 0xC0) != 0x00 or 0x80? > - } > - > - schedule(); > } > > /* get on-board memory size (PUTS detects no more than 4 MB) */ > @@ -734,15 +715,8 @@ static int __devinit wanxl_pci_init_one(struct pci_dev *pdev, > return -ENODEV; > } > > - stat = 0; > - timeout = jiffies + 5 * HZ; > - do { > - if ((stat = readl(card->plx + PLX_MAILBOX_5)) != 0) > - break; > - schedule(); > - }while (time_after(timeout, jiffies)); > - > - if (!stat) { > + if (TEST_UNTIL_TIMEOUT(((stat = readl(card->plx + PLX_MAILBOX_5)) != 0), > + 5 * HZ)) { > printk(KERN_WARNING "wanXL %s: timeout while initializing card " > "firmware\n", pci_name(pdev)); > wanxl_pci_remove_one(pdev); > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drivers/net/wan/wanxl.c: time_before(timeout, jiffies) -> jiffies, timeout 2008-03-20 20:23 ` Roel Kluin @ 2008-03-20 21:06 ` Joe Perches 0 siblings, 0 replies; 8+ messages in thread From: Joe Perches @ 2008-03-20 21:06 UTC (permalink / raw) To: Roel Kluin; +Cc: khc, lkml On Thu, 2008-03-20 at 21:23 +0100, Roel Kluin wrote: > > - while ((stat = readl(card->plx + PLX_MAILBOX_0)) != 0) { > > - if (time_before(timeout, jiffies)) { > > - printk(KERN_WARNING "wanXL %s: timeout waiting for" > > - " PUTS to complete\n", pci_name(pdev)); > > - wanxl_pci_remove_one(pdev); > > - return -ENODEV; > > - } > > - > > - switch(stat & 0xC0) { > > - case 0x00: /* hmm - PUTS completed with non-zero code? */ > > - case 0x80: /* PUTS still testing the hardware */ > > - break; > > - > > - default: > > - printk(KERN_WARNING "wanXL %s: PUTS test 0x%X" > > + if (TEST_UNTIL_TIMEOUT(((stat = readl(card->plx + PLX_MAILBOX_0)) != 0), > > + 20 * HZ)) { > > + printk(KERN_WARNING "wanXL %s: PUTS test 0x%X" > > " failed\n", pci_name(pdev), stat & 0x30); > > wanxl_pci_remove_one(pdev); > > return -ENODEV; > > Doesn't this change behavior for (stat & 0xC0) != 0x00 or 0x80? Probably, I didn't look at it too much. Feel free to fix it. The patch was just to demonstrate the statement expression macro and show the lack of schedules() in some of the loops. Maybe the statement expression macro should optionally schedule too: #define TEST_UNTIL_TIMEOUT(test, hz, schedule) \ ({ \ typeof(test) t; \ unsigned long timeout = jiffies + (hz); \ do { \ t = (test); \ if (t) \ break; \ if ((schedule)) \ schedule(); \ } while (time_before(jiffies, timeout)); \ t; \ }) cheers, Joe ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drivers/net/wan/wanxl.c: time_before(timeout, jiffies) -> jiffies, timeout 2008-03-20 16:46 ` Joe Perches 2008-03-20 20:23 ` Roel Kluin @ 2008-03-24 15:01 ` Krzysztof Halasa 2008-03-24 15:52 ` Joe Perches 1 sibling, 1 reply; 8+ messages in thread From: Krzysztof Halasa @ 2008-03-24 15:01 UTC (permalink / raw) To: Joe Perches; +Cc: Roel Kluin, lkml Joe Perches <joe@perches.com> writes: >> while ((stat = readl(card->plx + PLX_MAILBOX_0)) != 0) { >> - if (time_before(timeout, jiffies)) { >> + if (time_before(jiffies, timeout)) { >> printk(KERN_WARNING "wanXL %s: timeout waiting for" >> " PUTS to complete\n", pci_name(pdev)); >> wanxl_pci_remove_one(pdev); > > Wouldn't it be better to have a schedule() in those > while loops too? There is a schedule() here: timeout = jiffies + 20 * HZ; while ((stat = readl(card->plx + PLX_MAILBOX_0)) != 0) { if (time_before(timeout, jiffies)) { printk(KERN_WARNING "wanXL %s: timeout waiting for" " PUTS to complete\n", pci_name(pdev)); wanxl_pci_remove_one(pdev); return -ENODEV; } switch(stat & 0xC0) { case 0x00: /* hmm - PUTS completed with non-zero code? */ case 0x80: /* PUTS still testing the hardware */ break; default: printk(KERN_WARNING "wanXL %s: PUTS test 0x%X" " failed\n", pci_name(pdev), stat & 0x30); wanxl_pci_remove_one(pdev); return -ENODEV; } schedule(); } The timeout is 20 seconds, busy loop wouldn't make any sense. > Maybe a more generic macro / statement expression > would be more readable? I don't think so. BTW the only "long" loop is the POTS one (after hw reset or rmmod + insmod), IIRC it takes about 1 second for every MB of installed DRAM. Officially you can have 1 or 4 MB, and 16 MB module works (IIRC, on my card) as well - thus 20 * HZ timeout. A couple of reversed time_{after,before}, yes (with reversed arguments, i.e., functionally equivalent but probably harder to parse). Will look at them. -- Krzysztof Halasa ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drivers/net/wan/wanxl.c: time_before(timeout, jiffies) -> jiffies, timeout 2008-03-24 15:01 ` Krzysztof Halasa @ 2008-03-24 15:52 ` Joe Perches 2008-03-24 20:22 ` Krzysztof Halasa 0 siblings, 1 reply; 8+ messages in thread From: Joe Perches @ 2008-03-24 15:52 UTC (permalink / raw) To: Krzysztof Halasa; +Cc: Roel Kluin, lkml On Mon, 2008-03-24 at 16:01 +0100, Krzysztof Halasa wrote: > Joe Perches <joe@perches.com> writes: > > Wouldn't it be better to have a schedule() in those > > while loops too? > There is a schedule() here: What's the value of not having schedule() in the wanxl_open and wanxl_close routines? ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drivers/net/wan/wanxl.c: time_before(timeout, jiffies) -> jiffies, timeout 2008-03-24 15:52 ` Joe Perches @ 2008-03-24 20:22 ` Krzysztof Halasa 0 siblings, 0 replies; 8+ messages in thread From: Krzysztof Halasa @ 2008-03-24 20:22 UTC (permalink / raw) To: Joe Perches; +Cc: Roel Kluin, lkml Joe Perches <joe@perches.com> writes: > What's the value of not having schedule() in the > wanxl_open and wanxl_close routines? There is no need, it's fast. -- Krzysztof Halasa ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drivers/net/wan/wanxl.c: time_before(timeout, jiffies) -> jiffies, timeout 2008-03-20 15:43 [PATCH] drivers/net/wan/wanxl.c: time_before(timeout, jiffies) -> jiffies, timeout Roel Kluin 2008-03-20 16:46 ` Joe Perches @ 2008-03-24 14:42 ` Krzysztof Halasa 1 sibling, 0 replies; 8+ messages in thread From: Krzysztof Halasa @ 2008-03-24 14:42 UTC (permalink / raw) To: Roel Kluin; +Cc: lkml Hi, Roel Kluin <12o3l@tiscali.nl> writes: > --- a/drivers/net/wan/wanxl.c > +++ b/drivers/net/wan/wanxl.c > @@ -650,7 +650,7 @@ static int __devinit wanxl_pci_init_one(struct pci_dev *pdev, > > timeout = jiffies + 20 * HZ; > while ((stat = readl(card->plx + PLX_MAILBOX_0)) != 0) { > - if (time_before(timeout, jiffies)) { > + if (time_before(jiffies, timeout)) { > printk(KERN_WARNING "wanXL %s: timeout waiting for" > " PUTS to complete\n", pci_name(pdev)); > wanxl_pci_remove_one(pdev); > I can't see a bug here - time_before(timeout, jiffies) means "if timeout is before jiffies", IOW if jiffies passed the timeout. Perhaps time_after(jiffies, timeout) is a bit better equivalent, I will use that. -- Krzysztof Halasa ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2008-03-24 20:22 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2008-03-20 15:43 [PATCH] drivers/net/wan/wanxl.c: time_before(timeout, jiffies) -> jiffies, timeout Roel Kluin 2008-03-20 16:46 ` Joe Perches 2008-03-20 20:23 ` Roel Kluin 2008-03-20 21:06 ` Joe Perches 2008-03-24 15:01 ` Krzysztof Halasa 2008-03-24 15:52 ` Joe Perches 2008-03-24 20:22 ` Krzysztof Halasa 2008-03-24 14:42 ` Krzysztof Halasa
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.