From fef6108d4556917c45cd9ba397c1c7597f3990e1 Mon Sep 17 00:00:00 2001 From: Andy Fleming Date: Thu, 20 Apr 2006 16:44:29 -0500 Subject: [PATCH] [PATCH] Fix locking in gianfar This patch fixes several bugs in the gianfar driver, including a major one where spinlocks were horribly broken: * Split gianfar locks into two types: TX and RX * Made it so gfar_start() now clears RHALT * Fixed a bug where calling gfar_start_xmit() with interrupts off would corrupt the interrupt state * Fixed a bug where a frame could potentially arrive, and never be handled (if no more frames arrived * Fixed a bug where the rx_work_limit would never be observed by the rx completion code * Fixed a bug where the interrupt handlers were not actually protected by their spinlocks Signed-off-by: Andy Fleming Signed-off-by: Jeff Garzik --- drivers/net/gianfar.c | 56 ++++++++++++++++++------------------ drivers/net/gianfar.h | 67 +++++++++++++++++++++++++++++-------------- drivers/net/gianfar_ethtool.c | 20 +++++++++---- drivers/net/gianfar_sysfs.c | 24 ++++++++-------- 4 files changed, 100 insertions(+), 67 deletions(-) diff --git a/drivers/net/gianfar.c b/drivers/net/gianfar.c index 771e25d..218d317 100644 --- a/drivers/net/gianfar.c +++ b/drivers/net/gianfar.c @@ -210,7 +210,8 @@ static int gfar_probe(struct platform_device *pdev) goto regs_fail; } - spin_lock_init(&priv->lock); + spin_lock_init(&priv->txlock); + spin_lock_init(&priv->rxlock); platform_set_drvdata(pdev, dev); @@ -515,11 +516,13 @@ void stop_gfar(struct net_device *dev) phy_stop(priv->phydev); /* Lock it down */ - spin_lock_irqsave(&priv->lock, flags); + spin_lock_irqsave(&priv->txlock, flags); + spin_lock(&priv->rxlock); gfar_halt(dev); - spin_unlock_irqrestore(&priv->lock, flags); + spin_unlock(&priv->rxlock); + spin_unlock_irqrestore(&priv->txlock, flags); /* Free the IRQs */ if (priv->einfo->device_flags & FSL_GIANFAR_DEV_HAS_MULTI_INTR) { @@ -605,14 +608,15 @@ void gfar_start(struct net_device *dev) tempval |= DMACTRL_INIT_SETTINGS; gfar_write(&priv->regs->dmactrl, tempval); - /* Clear THLT, so that the DMA starts polling now */ - gfar_write(®s->tstat, TSTAT_CLEAR_THALT); - /* Make sure we aren't stopped */ tempval = gfar_read(&priv->regs->dmactrl); tempval &= ~(DMACTRL_GRS | DMACTRL_GTS); gfar_write(&priv->regs->dmactrl, tempval); + /* Clear THLT/RHLT, so that the DMA starts polling now */ + gfar_write(®s->tstat, TSTAT_CLEAR_THALT); + gfar_write(®s->rstat, RSTAT_CLEAR_RHALT); + /* Unmask the interrupts we look for */ gfar_write(®s->imask, IMASK_DEFAULT); } @@ -928,12 +932,13 @@ static int gfar_start_xmit(struct sk_buff *skb, struct net_device *dev) struct txfcb *fcb = NULL; struct txbd8 *txbdp; u16 status; + unsigned long flags; /* Update transmit stats */ priv->stats.tx_bytes += skb->len; /* Lock priv now */ - spin_lock_irq(&priv->lock); + spin_lock_irqsave(&priv->txlock, flags); /* Point at the first free tx descriptor */ txbdp = priv->cur_tx; @@ -1004,7 +1009,7 @@ static int gfar_start_xmit(struct sk_buff *skb, struct net_device *dev) gfar_write(&priv->regs->tstat, TSTAT_CLEAR_THALT); /* Unlock priv */ - spin_unlock_irq(&priv->lock); + spin_unlock_irqrestore(&priv->txlock, flags); return 0; } @@ -1049,7 +1054,7 @@ static void gfar_vlan_rx_register(struct net_device *dev, unsigned long flags; u32 tempval; - spin_lock_irqsave(&priv->lock, flags); + spin_lock_irqsave(&priv->rxlock, flags); priv->vlgrp = grp; @@ -1076,7 +1081,7 @@ static void gfar_vlan_rx_register(struct net_device *dev, gfar_write(&priv->regs->rctrl, tempval); } - spin_unlock_irqrestore(&priv->lock, flags); + spin_unlock_irqrestore(&priv->rxlock, flags); } @@ -1085,12 +1090,12 @@ static void gfar_vlan_rx_kill_vid(struct net_device *dev, uint16_t vid) struct gfar_private *priv = netdev_priv(dev); unsigned long flags; - spin_lock_irqsave(&priv->lock, flags); + spin_lock_irqsave(&priv->rxlock, flags); if (priv->vlgrp) priv->vlgrp->vlan_devices[vid] = NULL; - spin_unlock_irqrestore(&priv->lock, flags); + spin_unlock_irqrestore(&priv->rxlock, flags); } @@ -1179,7 +1184,7 @@ static irqreturn_t gfar_transmit(int irq, void *dev_id, struct pt_regs *regs) gfar_write(&priv->regs->ievent, IEVENT_TX_MASK); /* Lock priv */ - spin_lock(&priv->lock); + spin_lock(&priv->txlock); bdp = priv->dirty_tx; while ((bdp->status & TXBD_READY) == 0) { /* If dirty_tx and cur_tx are the same, then either the */ @@ -1224,7 +1229,7 @@ static irqreturn_t gfar_transmit(int irq, void *dev_id, struct pt_regs *regs) else gfar_write(&priv->regs->txic, 0); - spin_unlock(&priv->lock); + spin_unlock(&priv->txlock); return IRQ_HANDLED; } @@ -1305,9 +1310,10 @@ irqreturn_t gfar_receive(int irq, void *dev_id, struct pt_regs *regs) { struct net_device *dev = (struct net_device *) dev_id; struct gfar_private *priv = netdev_priv(dev); - #ifdef CONFIG_GFAR_NAPI u32 tempval; +#else + unsigned long flags; #endif /* Clear IEVENT, so rx interrupt isn't called again @@ -1330,7 +1336,7 @@ irqreturn_t gfar_receive(int irq, void *dev_id, struct pt_regs *regs) } #else - spin_lock(&priv->lock); + spin_lock_irqsave(&priv->rxlock, flags); gfar_clean_rx_ring(dev, priv->rx_ring_size); /* If we are coalescing interrupts, update the timer */ @@ -1341,7 +1347,7 @@ irqreturn_t gfar_receive(int irq, void *dev_id, struct pt_regs *regs) else gfar_write(&priv->regs->rxic, 0); - spin_unlock(&priv->lock); + spin_unlock_irqrestore(&priv->rxlock, flags); #endif return IRQ_HANDLED; @@ -1490,13 +1496,6 @@ int gfar_clean_rx_ring(struct net_device *dev, int rx_work_limit) /* Update the current rxbd pointer to be the next one */ priv->cur_rx = bdp; - /* If no packets have arrived since the - * last one we processed, clear the IEVENT RX and - * BSY bits so that another interrupt won't be - * generated when we set IMASK */ - if (bdp->status & RXBD_EMPTY) - gfar_write(&priv->regs->ievent, IEVENT_RX_MASK); - return howmany; } @@ -1516,7 +1515,7 @@ static int gfar_poll(struct net_device *dev, int *budget) rx_work_limit -= howmany; *budget -= howmany; - if (rx_work_limit >= 0) { + if (rx_work_limit > 0) { netif_rx_complete(dev); /* Clear the halt bit in RSTAT */ @@ -1533,7 +1532,8 @@ static int gfar_poll(struct net_device *dev, int *budget) gfar_write(&priv->regs->rxic, 0); } - return (rx_work_limit < 0) ? 1 : 0; + /* Return 1 if there's more work to do */ + return (rx_work_limit > 0) ? 0 : 1; } #endif @@ -1629,7 +1629,7 @@ static void adjust_link(struct net_device *dev) struct phy_device *phydev = priv->phydev; int new_state = 0; - spin_lock_irqsave(&priv->lock, flags); + spin_lock_irqsave(&priv->txlock, flags); if (phydev->link) { u32 tempval = gfar_read(®s->maccfg2); u32 ecntrl = gfar_read(®s->ecntrl); @@ -1694,7 +1694,7 @@ static void adjust_link(struct net_device *dev) if (new_state && netif_msg_link(priv)) phy_print_status(phydev); - spin_unlock_irqrestore(&priv->lock, flags); + spin_unlock_irqrestore(&priv->txlock, flags); } /* Update the hash table based on the current list of multicast diff --git a/drivers/net/gianfar.h b/drivers/net/gianfar.h index d37d540..127c98c 100644 --- a/drivers/net/gianfar.h +++ b/drivers/net/gianfar.h @@ -656,43 +656,62 @@ struct gfar { * the buffer descriptor determines the actual condition. */ struct gfar_private { - /* pointers to arrays of skbuffs for tx and rx */ + /* Fields controlled by TX lock */ + spinlock_t txlock; + + /* Pointer to the array of skbuffs */ struct sk_buff ** tx_skbuff; - struct sk_buff ** rx_skbuff; - /* indices pointing to the next free sbk in skb arrays */ + /* next free skb in the array */ u16 skb_curtx; - u16 skb_currx; - /* index of the first skb which hasn't been transmitted - * yet. */ + /* First skb in line to be transmitted */ u16 skb_dirtytx; /* Configuration info for the coalescing features */ unsigned char txcoalescing; unsigned short txcount; unsigned short txtime; + + /* Buffer descriptor pointers */ + struct txbd8 *tx_bd_base; /* First tx buffer descriptor */ + struct txbd8 *cur_tx; /* Next free ring entry */ + struct txbd8 *dirty_tx; /* First buffer in line + to be transmitted */ + unsigned int tx_ring_size; + + /* RX Locked fields */ + spinlock_t rxlock; + + /* skb array and index */ + struct sk_buff ** rx_skbuff; + u16 skb_currx; + + /* RX Coalescing values */ unsigned char rxcoalescing; unsigned short rxcount; unsigned short rxtime; - /* GFAR addresses */ - struct rxbd8 *rx_bd_base; /* Base addresses of Rx and Tx Buffers */ - struct txbd8 *tx_bd_base; + struct rxbd8 *rx_bd_base; /* First Rx buffers */ struct rxbd8 *cur_rx; /* Next free rx ring entry */ - struct txbd8 *cur_tx; /* Next free ring entry */ - struct txbd8 *dirty_tx; /* The Ring entry to be freed. */ - struct gfar __iomem *regs; /* Pointer to the GFAR memory mapped Registers */ - u32 __iomem *hash_regs[16]; - int hash_width; - struct net_device_stats stats; /* linux network statistics */ - struct gfar_extra_stats extra_stats; - spinlock_t lock; + + /* RX parameters */ + unsigned int rx_ring_size; unsigned int rx_buffer_size; unsigned int rx_stash_size; unsigned int rx_stash_index; - unsigned int tx_ring_size; - unsigned int rx_ring_size; + + struct vlan_group *vlgrp; + + /* Unprotected fields */ + /* Pointer to the GFAR memory mapped Registers */ + struct gfar __iomem *regs; + + /* Hash registers and their width */ + u32 __iomem *hash_regs[16]; + int hash_width; + + /* global parameters */ unsigned int fifo_threshold; unsigned int fifo_starve; unsigned int fifo_starve_off; @@ -702,13 +721,15 @@ struct gfar_private { extended_hash:1, bd_stash_en:1; unsigned short padding; - struct vlan_group *vlgrp; - /* Info structure initialized by board setup code */ + unsigned int interruptTransmit; unsigned int interruptReceive; unsigned int interruptError; + + /* info structure initialized by platform code */ struct gianfar_platform_data *einfo; + /* PHY stuff */ struct phy_device *phydev; struct mii_bus *mii_bus; int oldspeed; @@ -716,6 +737,10 @@ struct gfar_private { int oldlink; uint32_t msg_enable; + + /* Network Statistics */ + struct net_device_stats stats; + struct gfar_extra_stats extra_stats; }; static inline u32 gfar_read(volatile unsigned __iomem *addr) diff --git a/drivers/net/gianfar_ethtool.c b/drivers/net/gianfar_ethtool.c index 5de7b2e..d69698c 100644 --- a/drivers/net/gianfar_ethtool.c +++ b/drivers/net/gianfar_ethtool.c @@ -455,10 +455,14 @@ static int gfar_sringparam(struct net_device *dev, struct ethtool_ringparam *rva /* Halt TX and RX, and process the frames which * have already been received */ - spin_lock_irqsave(&priv->lock, flags); + spin_lock_irqsave(&priv->txlock, flags); + spin_lock(&priv->rxlock); + gfar_halt(dev); gfar_clean_rx_ring(dev, priv->rx_ring_size); - spin_unlock_irqrestore(&priv->lock, flags); + + spin_unlock(&priv->rxlock); + spin_unlock_irqrestore(&priv->txlock, flags); /* Now we take down the rings to rebuild them */ stop_gfar(dev); @@ -488,10 +492,14 @@ static int gfar_set_rx_csum(struct net_device *dev, uint32_t data) /* Halt TX and RX, and process the frames which * have already been received */ - spin_lock_irqsave(&priv->lock, flags); + spin_lock_irqsave(&priv->txlock, flags); + spin_lock(&priv->rxlock); + gfar_halt(dev); gfar_clean_rx_ring(dev, priv->rx_ring_size); - spin_unlock_irqrestore(&priv->lock, flags); + + spin_unlock(&priv->rxlock); + spin_unlock_irqrestore(&priv->txlock, flags); /* Now we take down the rings to rebuild them */ stop_gfar(dev); @@ -523,7 +531,7 @@ static int gfar_set_tx_csum(struct net_device *dev, uint32_t data) if (!(priv->einfo->device_flags & FSL_GIANFAR_DEV_HAS_CSUM)) return -EOPNOTSUPP; - spin_lock_irqsave(&priv->lock, flags); + spin_lock_irqsave(&priv->txlock, flags); gfar_halt(dev); if (data) @@ -532,7 +540,7 @@ static int gfar_set_tx_csum(struct net_device *dev, uint32_t data) dev->features &= ~NETIF_F_IP_CSUM; gfar_start(dev); - spin_unlock_irqrestore(&priv->lock, flags); + spin_unlock_irqrestore(&priv->txlock, flags); return 0; } diff --git a/drivers/net/gianfar_sysfs.c b/drivers/net/gianfar_sysfs.c index 51ef181..a6d5c43 100644 --- a/drivers/net/gianfar_sysfs.c +++ b/drivers/net/gianfar_sysfs.c @@ -82,7 +82,7 @@ static ssize_t gfar_set_bd_stash(struct class_device *cdev, else return count; - spin_lock_irqsave(&priv->lock, flags); + spin_lock_irqsave(&priv->rxlock, flags); /* Set the new stashing value */ priv->bd_stash_en = new_setting; @@ -96,7 +96,7 @@ static ssize_t gfar_set_bd_stash(struct class_device *cdev, gfar_write(&priv->regs->attr, temp); - spin_unlock_irqrestore(&priv->lock, flags); + spin_unlock_irqrestore(&priv->rxlock, flags); return count; } @@ -118,7 +118,7 @@ static ssize_t gfar_set_rx_stash_size(struct class_device *cdev, u32 temp; unsigned long flags; - spin_lock_irqsave(&priv->lock, flags); + spin_lock_irqsave(&priv->rxlock, flags); if (length > priv->rx_buffer_size) return count; @@ -142,7 +142,7 @@ static ssize_t gfar_set_rx_stash_size(struct class_device *cdev, gfar_write(&priv->regs->attr, temp); - spin_unlock_irqrestore(&priv->lock, flags); + spin_unlock_irqrestore(&priv->rxlock, flags); return count; } @@ -166,7 +166,7 @@ static ssize_t gfar_set_rx_stash_index(struct class_device *cdev, u32 temp; unsigned long flags; - spin_lock_irqsave(&priv->lock, flags); + spin_lock_irqsave(&priv->rxlock, flags); if (index > priv->rx_stash_size) return count; @@ -180,7 +180,7 @@ static ssize_t gfar_set_rx_stash_index(struct class_device *cdev, temp |= ATTRELI_EI(index); gfar_write(&priv->regs->attreli, flags); - spin_unlock_irqrestore(&priv->lock, flags); + spin_unlock_irqrestore(&priv->rxlock, flags); return count; } @@ -205,7 +205,7 @@ static ssize_t gfar_set_fifo_threshold(struct class_device *cdev, if (length > GFAR_MAX_FIFO_THRESHOLD) return count; - spin_lock_irqsave(&priv->lock, flags); + spin_lock_irqsave(&priv->txlock, flags); priv->fifo_threshold = length; @@ -214,7 +214,7 @@ static ssize_t gfar_set_fifo_threshold(struct class_device *cdev, temp |= length; gfar_write(&priv->regs->fifo_tx_thr, temp); - spin_unlock_irqrestore(&priv->lock, flags); + spin_unlock_irqrestore(&priv->txlock, flags); return count; } @@ -240,7 +240,7 @@ static ssize_t gfar_set_fifo_starve(struct class_device *cdev, if (num > GFAR_MAX_FIFO_STARVE) return count; - spin_lock_irqsave(&priv->lock, flags); + spin_lock_irqsave(&priv->txlock, flags); priv->fifo_starve = num; @@ -249,7 +249,7 @@ static ssize_t gfar_set_fifo_starve(struct class_device *cdev, temp |= num; gfar_write(&priv->regs->fifo_tx_starve, temp); - spin_unlock_irqrestore(&priv->lock, flags); + spin_unlock_irqrestore(&priv->txlock, flags); return count; } @@ -274,7 +274,7 @@ static ssize_t gfar_set_fifo_starve_off(struct class_device *cdev, if (num > GFAR_MAX_FIFO_STARVE_OFF) return count; - spin_lock_irqsave(&priv->lock, flags); + spin_lock_irqsave(&priv->txlock, flags); priv->fifo_starve_off = num; @@ -283,7 +283,7 @@ static ssize_t gfar_set_fifo_starve_off(struct class_device *cdev, temp |= num; gfar_write(&priv->regs->fifo_tx_starve_shutoff, temp); - spin_unlock_irqrestore(&priv->lock, flags); + spin_unlock_irqrestore(&priv->txlock, flags); return count; } -- 1.8.2.3