lkml.org 
[lkml]   [2008]   [Mar]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] drivers/net/wan/wanxl.c: time_before(timeout, jiffies) -> jiffies, timeout
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);
>
>


\
 
 \ /
  Last update: 2008-03-20 21:35    [W:0.081 / U:1.376 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site