lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Message-ID: <20120320115422.GA7226@electric-eye.fr.zoreil.com> Date: Tue, 20 Mar 2012 12:54:22 +0100 From: Francois Romieu <romieu@...zoreil.com> To: Andreas Fenkart <afenkart@...il.com> Cc: davem@...emloft.net, netdev@...r.kernel.org Subject: Re: [PATCH 1/1] ARC VMAC driver. Andreas Fenkart <afenkart@...il.com> : > This is a driver for the MAC IP block from ARC International. It > is based on an existing driver found in ARC Linux distribution, > but essentially, a full rewrite. > > Signed-off-by: Andreas Fenkart <afenkart@...il.com> > --- > drivers/net/ethernet/Kconfig | 9 + > drivers/net/ethernet/Makefile | 1 + > drivers/net/ethernet/arcvmac.c | 1472 ++++++++++++++++++++++++++++++++++++++++ > drivers/net/ethernet/arcvmac.h | 269 ++++++++ > 4 files changed, 1751 insertions(+), 0 deletions(-) > > diff --git a/drivers/net/ethernet/Kconfig b/drivers/net/ethernet/Kconfig > index 597f4d4..4b6baf8 100644 > --- a/drivers/net/ethernet/Kconfig > +++ b/drivers/net/ethernet/Kconfig > @@ -23,6 +23,15 @@ source "drivers/net/ethernet/aeroflex/Kconfig" > source "drivers/net/ethernet/alteon/Kconfig" > source "drivers/net/ethernet/amd/Kconfig" > source "drivers/net/ethernet/apple/Kconfig" > + > +config ARCVMAC > + tristate "ARC VMAC ethernet driver" > + select MII > + select PHYLIB > + select CRC32 > + help > + MAC present Zoran43xx, IP from ARC international It may not hurt to tell if it targets gigabit or fast ethernet. > + > source "drivers/net/ethernet/atheros/Kconfig" > source "drivers/net/ethernet/cadence/Kconfig" > source "drivers/net/ethernet/adi/Kconfig" > diff --git a/drivers/net/ethernet/Makefile b/drivers/net/ethernet/Makefile > index be5dde0..cb61799 100644 > --- a/drivers/net/ethernet/Makefile > +++ b/drivers/net/ethernet/Makefile > @@ -9,6 +9,7 @@ obj-$(CONFIG_GRETH) += aeroflex/ > obj-$(CONFIG_NET_VENDOR_ALTEON) += alteon/ > obj-$(CONFIG_NET_VENDOR_AMD) += amd/ > obj-$(CONFIG_NET_VENDOR_APPLE) += apple/ > +obj-$(CONFIG_ARCVMAC) += arcvmac.o I am not sure we want it directly under drivers/net/ethernet. [...] > diff --git a/drivers/net/ethernet/arcvmac.c b/drivers/net/ethernet/arcvmac.c > new file mode 100644 > index 0000000..d512806 > --- /dev/null > +++ b/drivers/net/ethernet/arcvmac.c [...] > + unsigned char hwaddr[ETH_ALEN]) > +{ > + struct vmac_priv *ap = netdev_priv(dev); > + u32 mac_lo, mac_hi; > + > + WARN_ON(!hwaddr); Useless. It will issue a nice Oops a few lines below anyway. > + mac_lo = vmac_readl(ap, ADDRL); > + mac_hi = vmac_readl(ap, ADDRH); > + > + hwaddr[0] = (mac_lo >> 0) & 0xff; > + hwaddr[1] = (mac_lo >> 8) & 0xff; > + hwaddr[2] = (mac_lo >> 16) & 0xff; > + hwaddr[3] = (mac_lo >> 24) & 0xff; > + hwaddr[4] = (mac_hi >> 0) & 0xff; > + hwaddr[5] = (mac_hi >> 8) & 0xff; > + return hwaddr; > +} [...] > +static int __devinit vmac_mii_init(struct vmac_priv *ap) > +{ > + unsigned long flags; > + int err, i; > + > + spin_lock_irqsave(&ap->lock, flags); > + > + ap->mii_bus = mdiobus_alloc(); Bug: non GFP_ATOMIC alloc with spinlock held. > + if (!ap->mii_bus) > + return -ENOMEM; > + > + ap->mii_bus->name = "vmac_mii_bus"; > + ap->mii_bus->read = &vmac_mdio_read; > + ap->mii_bus->write = &vmac_mdio_write; > + > + snprintf(ap->mii_bus->id, MII_BUS_ID_SIZE, "%x", 0); > + > + ap->mii_bus->priv = ap; > + > + err = -ENOMEM; > + ap->mii_bus->irq = kmalloc(sizeof(int) * PHY_MAX_ADDR, GFP_KERNEL); Sic. [...] > +static void vmac_mii_exit_unlocked(struct net_device *dev) > +{ > + struct vmac_priv *ap = netdev_priv(dev); > + > + if (ap->phy_dev) It can't be NULL. > + phy_disconnect(ap->phy_dev); > + > + mdiobus_unregister(ap->mii_bus); > + kfree(ap->mii_bus->irq); > + mdiobus_free(ap->mii_bus); > +} > + > +static int vmacether_get_settings(struct net_device *dev, > + struct ethtool_cmd *cmd) > +{ > + struct vmac_priv *ap = netdev_priv(dev); > + struct phy_device *phydev = ap->phy_dev; > + > + if (!phydev) > + return -ENODEV; It can't be NULL here either. Please check you driver. Most of times, it can't be NULL. > + > + return phy_ethtool_gset(phydev, cmd); > +} > + > +static int vmacether_set_settings(struct net_device *dev, > + struct ethtool_cmd *cmd) > +{ > + struct vmac_priv *ap = netdev_priv(dev); > + struct phy_device *phydev = ap->phy_dev; > + > + if (!phydev) > + return -ENODEV; Sic. > + > + return phy_ethtool_sset(phydev, cmd); > +} > + > +static int vmac_ioctl(struct net_device *dev, struct ifreq *rq, int cmd) > +{ > + struct vmac_priv *ap = netdev_priv(dev); > + struct phy_device *phydev = ap->phy_dev; > + > + if (!netif_running(dev)) > + return -EINVAL; > + > + if (!phydev) > + return -ENODEV; Sic. [...] > +static int update_error_counters_unlocked(struct net_device *dev, int status) > +{ > + struct vmac_priv *ap = netdev_priv(dev); > + > + dev_dbg(&ap->pdev->dev, "rx error counter overrun. status = 0x%x\n", > + status); > + > + /* programming error */ > + WARN_ON(status & TXCH_MASK); > + WARN_ON(!(status & (MSER_MASK | RXCR_MASK | RXFR_MASK | RXFL_MASK))); > + > + if (status & MSER_MASK) > + dev->stats.rx_over_errors += 256; /* ran out of BD */ > + if (status & RXCR_MASK) > + dev->stats.rx_crc_errors += 256; > + if (status & RXFR_MASK) > + dev->stats.rx_frame_errors += 256; > + if (status & RXFL_MASK) > + dev->stats.rx_fifo_errors += 256; I would not mind a local &dev->stats variable. [...] > +static void update_tx_errors_unlocked(struct net_device *dev, int status) > +{ > + struct vmac_priv *ap = netdev_priv(dev); > + > + if (status & BD_UFLO) > + dev->stats.tx_fifo_errors++; > + > + if (ap->duplex) > + return; > + > + /* half duplex flags */ > + if (status & BD_LTCL) > + dev->stats.tx_window_errors++; > + if (status & BD_RETRY_CT) > + dev->stats.collisions += (status & BD_RETRY_CT) >> 24; > + if (status & BD_DROP) /* too many retries */ > + dev->stats.tx_aborted_errors++; > + if (status & BD_DEFER) > + dev_vdbg(&ap->pdev->dev, "\"defer to traffic\"\n"); > + if (status & BD_CARLOSS) > + dev->stats.tx_carrier_errors++; Same thing as above. [...] > +static int vmac_poll(struct napi_struct *napi, int budget) > +{ > + struct vmac_priv *ap; > + int rx_work_done; > + > + ap = container_of(napi, struct vmac_priv, napi); You can 'struct vmac_priv *ap = container_of(napi ..." > + > + vmac_tx_reclaim_unlocked(ap->dev, false); > + > + rx_work_done = vmac_rx_receive(ap->dev, budget); > + if (rx_work_done >= budget) { > + /* rx queue is not yet empty/clean */ > + return rx_work_done; > + } Please no comment nor curly brace. > + > + /* no more packet in rx/tx queue, remove device from poll queue */ > + napi_complete(napi); > + > + /* clear status, only 1' affect register state */ > + vmac_writel(ap, RXINT_MASK | TXINT_MASK, STAT); > + > + /* reenable IRQ */ > + vmac_toggle_rxint_unlocked(ap->dev, true); > + vmac_toggle_txint_unlocked(ap->dev, true); > + > + return rx_work_done; > +} > + > +static irqreturn_t vmac_intr(int irq, void *dev_instance) > +{ > + struct net_device *dev = dev_instance; > + struct vmac_priv *ap = netdev_priv(dev); > + u32 status; > + > + spin_lock(&ap->lock); > + > + status = vmac_readl(ap, STAT); > + vmac_writel(ap, status, STAT); I do not know what you are trying to achieve with the lock but it is not held in start_xmit when STAT is written. > + > + if (unlikely(ap->shutdown)) > + dev_err(&ap->pdev->dev, "ISR during close\n"); > + > + if (unlikely(!status & (RXINT_MASK|MDIO_MASK|ERR_MASK))) > + dev_err(&ap->pdev->dev, "Spurious IRQ\n"); > + > + if ((status & RXINT_MASK) && (vmac_readl(ap, ENABLE) & RXINT_MASK) && > + (ap->dma_rx_head != vmac_readl(ap, MAC_RXRING_HEAD))) { > + vmac_toggle_rxint_unlocked(dev, false); > + napi_schedule(&ap->napi); > + } > + > + if ((status & TXINT_MASK) && (vmac_readl(ap, ENABLE) & TXINT_MASK)) { > + vmac_toggle_txint_unlocked(dev, false); > + napi_schedule(&ap->napi); > + } > + > + if (status & MDIO_MASK) > + complete(&ap->mdio_complete); > + > + if (unlikely(status & ERR_MASK)) > + update_error_counters_unlocked(dev, status); Do yourself a favor : move everything to NAPI context. [...] > +static int vmac_start_xmit(struct sk_buff *skb, struct net_device *dev) > +{ > + struct vmac_priv *ap = netdev_priv(dev); > + struct vmac_buffer_desc *desc; > + > + /* running under xmit lock */ > + /* locking: modifies tx_ring head, tx_reclaim only tail */ > + > + /* no scatter/gatter see features below */ > + WARN_ON(skb_shinfo(skb)->nr_frags != 0); > + WARN_ON(skb->len > MAX_TX_BUFFER_LEN); > + > + if (unlikely(fifo_full(&ap->tx_ring))) { > + netif_stop_queue(dev); > + dev_err(&ap->pdev->dev, > + "xmit called while no tx desc available\n"); > + return NETDEV_TX_BUSY; > + } > + > + if (unlikely(skb->len < ETH_ZLEN)) { > + if (skb_padto(skb, ETH_ZLEN)) > + return NETDEV_TX_OK; > + skb_put(skb, ETH_ZLEN - skb->len); > + } > + > + /* fill descriptor */ > + ap->tx_skbuff[ap->tx_ring.head] = skb; > + desc = &ap->txbd[ap->tx_ring.head]; > + WARN_ON(desc->info & cpu_to_le32(BD_DMA_OWN)); > + > + desc->data = dma_map_single(&ap->pdev->dev, skb->data, skb->len, > + DMA_TO_DEVICE); > + > + /* dma might already be polling */ > + wmb(); > + desc->info = cpu_to_le32(BD_DMA_OWN | BD_FRST | BD_LAST | skb->len); > + > + /* kick tx dma, only 1' affect register */ > + vmac_writel(ap, TXPL_MASK, STAT); > + > + dev->stats.tx_packets++; > + dev->stats.tx_bytes += skb->len; No byte queue limit ? > + fifo_inc_head(&ap->tx_ring); > + > + /* stop queue if no more desc available */ > + if (fifo_full(&ap->tx_ring)) > + netif_stop_queue(dev); > + > + return NETDEV_TX_OK; > +} > + > +static int alloc_buffers_unlocked(struct net_device *dev) > +{ > + struct vmac_priv *ap = netdev_priv(dev); > + int size, err = -ENOMEM; > + > + fifo_init(&ap->rx_ring, RX_BDT_LEN); > + fifo_init(&ap->tx_ring, TX_BDT_LEN); > + > + /* initialize skb list */ > + memset(ap->rx_skbuff, 0, sizeof(ap->rx_skbuff)); > + memset(ap->tx_skbuff, 0, sizeof(ap->tx_skbuff)); > + > + /* allocate DMA received descriptors */ > + size = sizeof(*ap->rxbd) * ap->rx_ring.size; > + ap->rxbd = dma_alloc_coherent(&ap->pdev->dev, size, > + &ap->rxbd_dma, > + GFP_KERNEL); > + if (!ap->rxbd) > + goto err_out; > + > + /* allocate DMA transmit descriptors */ > + size = sizeof(*ap->txbd) * ap->tx_ring.size; > + ap->txbd = dma_alloc_coherent(&ap->pdev->dev, size, > + &ap->txbd_dma, > + GFP_KERNEL); > + if (!ap->txbd) > + goto err_free_rxbd; > + > + /* ensure 8-byte aligned */ > + WARN_ON(((uintptr_t)ap->txbd & 0x7) || ((uintptr_t)ap->rxbd & 0x7)); > + > + memset(ap->txbd, 0, sizeof(*ap->txbd) * ap->tx_ring.size); > + memset(ap->rxbd, 0, sizeof(*ap->rxbd) * ap->rx_ring.size); It is already zeroed after dma_alloc_coherent. [...] > +static int vmac_open(struct net_device *dev) > +{ > + struct vmac_priv *ap = netdev_priv(dev); > + struct phy_device *phydev; > + unsigned long flags; > + u32 mask, ctrl; > + int err = 0; > + > + /* locking: no concurrency yet */ > + > + if (!ap) > + return -ENODEV; > + > + spin_lock_irqsave(&ap->lock, flags); > + ap->shutdown = false; > + > + err = get_register_map(ap); > + if (err) > + return err; > + > + vmac_hw_init(dev); > + > + /* mac address changed? */ > + write_mac_reg(dev, dev->dev_addr); > + > + err = alloc_buffers_unlocked(dev); dma_alloc_coherent(... GFP_KERNEL) with spinlock held. What are you trying to lock against anyway ? [...] > +static int vmac_close(struct net_device *dev) > +{ > + struct vmac_priv *ap = netdev_priv(dev); > + unsigned long flags; > + u32 tmp; > + > + /* locking: protect everything, DMA / IRQ / timer */ > + spin_lock_irqsave(&ap->lock, flags); > + > + /* complete running transfer, then stop */ > + tmp = vmac_readl(ap, CONTROL); > + tmp &= ~(TXRN_MASK | RXRN_MASK); > + vmac_writel(ap, tmp, CONTROL); > + > + /* save statistics, before unmapping */ > + update_vmac_stats_unlocked(dev); > + > + /* reenable IRQ, process pending */ > + spin_unlock_irqrestore(&ap->lock, flags); > + > + set_current_state(TASK_INTERRUPTIBLE); > + schedule_timeout(msecs_to_jiffies(20)); > + > + /* shut it down now */ > + spin_lock_irqsave(&ap->lock, flags); > + ap->shutdown = true; > + > + netif_stop_queue(dev); > + napi_disable(&ap->napi); Bug: napi_disable may sleep and the driver is holding a spinlock. > + /* disable phy */ > + phy_stop(ap->phy_dev); > + vmac_mii_exit_unlocked(dev); > + netif_carrier_off(dev); > + > + /* disable interrupts */ > + vmac_writel(ap, 0, ENABLE); > + free_irq(dev->irq, dev); > + > + /* turn off vmac */ > + vmac_writel(ap, 0, CONTROL); > + /* vmac_reset_hw(vmac) */ > + > + /* locking: concurrency off */ > + spin_unlock_irqrestore(&ap->lock, flags); [...] > +static void update_vmac_stats_unlocked(struct net_device *dev) > +{ > + struct net_device_stats *_stats = &dev->stats; What's wrong with plain 'stats' ? [...] > +static void create_multicast_filter(struct net_device *dev, > + int32_t *bitmask) > +{ > + char *addrs; > + u32 crc; > + > + /* locking: done by net_device */ > + > + WARN_ON(netdev_mc_count(dev) == 0); > + WARN_ON(dev->flags & IFF_ALLMULTI); > + > + bitmask[0] = bitmask[1] = 0; > + > + { ? [...] > +static int __devinit vmac_probe(struct platform_device *pdev) > +{ > + struct net_device *dev; > + struct vmac_priv *ap; > + struct resource *mem; > + int err; > + > + /* locking: no concurrency */ > + > + if (dma_get_mask(&pdev->dev) > DMA_BIT_MASK(32) || > + pdev->dev.coherent_dma_mask > DMA_BIT_MASK(32)) { > + dev_err(&pdev->dev, > + "arcvmac supports only 32-bit DMA addresses\n"); > + return -ENODEV; > + } > + > + dev = alloc_etherdev(sizeof(*ap)); > + if (!dev) { > + dev_err(&pdev->dev, "etherdev alloc failed, aborting.\n"); > + return -ENOMEM; > + } > + > + ap = netdev_priv(dev); > + > + err = -ENODEV; > + mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + if (!mem) { > + dev_err(&pdev->dev, "no mmio resource defined\n"); > + goto err_out; > + } > + ap->mem = mem; > + > + err = platform_get_irq(pdev, 0); > + if (err < 0) { > + dev_err(&pdev->dev, "no irq found\n"); > + goto err_out; > + } > + dev->irq = err; net_device.{irq, base_addr} have been obsolete for years. Please don't use it to expose kernel internals to userspace. > + > + spin_lock_init(&ap->lock); > + > + SET_NETDEV_DEV(dev, &pdev->dev); > + ap->dev = dev; > + ap->pdev = pdev; > + > + /* init rx timeout (used for oom) */ > + init_timer(&ap->refill_timer); > + ap->refill_timer.function = vmac_refill_rx_timer; > + ap->refill_timer.data = (unsigned long)dev; > + spin_lock_init(&ap->refill_lock); > + > + netif_napi_add(dev, &ap->napi, vmac_poll, 64); > + dev->netdev_ops = &vmac_netdev_ops; > + dev->ethtool_ops = &vmac_ethtool_ops; > + > + dev->base_addr = (unsigned long)ap->regs; (see above) > + > + /* prevent buffer chaining, favor speed over space */ > + ap->rx_skb_size = ETH_FRAME_LEN + VMAC_BUFFER_PAD; > + > + /* private struct functional */ > + > + /* temporarily map registers to fetch mac addr */ > + err = get_register_map(ap); > + if (err) > + goto err_out; Please use 'goto err_perform_some_unwinding_action' style. You don't need to be literate. 'goto err_napi_del' will be good enough :o) > + > + /* mac address intialize, set vmac_open */ > + read_mac_reg(dev, dev->dev_addr); > + > + if (!is_valid_ether_addr(dev->dev_addr)) > + dev_hw_addr_random(dev, dev->dev_addr); > + > + err = register_netdev(dev); > + if (err) { > + dev_err(&pdev->dev, "Cannot register net device, aborting.\n"); > + goto err_out; Should be something like 'goto err_I_wont_leak_register_memory_region' > + } > + > + /* release the memory region, till open is called */ Why ? It adds a failure opportunity. [...] > diff --git a/drivers/net/ethernet/arcvmac.h b/drivers/net/ethernet/arcvmac.h > new file mode 100644 > index 0000000..f94ab38 > --- /dev/null > +++ b/drivers/net/ethernet/arcvmac.h [...] > +/* stat/enable use same bit mask */ > +#define VMAC_STAT 0x04 > +#define VMAC_ENABLE 0x08 > +# define TXINT_MASK 0x00000001 /* Transmit interrupt */ > +# define RXINT_MASK 0x00000002 /* Receive interrupt */ > +# define ERR_MASK 0x00000004 /* Error interrupt */ > +# define TXCH_MASK 0x00000008 /* Transmit chaining error */ > +# define MSER_MASK 0x00000010 /* Missed packet counter error */ > +# define RXCR_MASK 0x00000100 /* RXCRCERR counter rolled over */ Please keep things aligned and add the extra space after "define" (see tg3.h). [...] > +struct vmac_priv { > + struct net_device *dev; > + struct platform_device *pdev; > + > + struct completion mdio_complete; > + spinlock_t lock; /* protects structure plus hw regs of device */ > + > + /* base address of register set */ > + char *regs; It should be __iomem annotated. [...] > +/* DMA ring management */ > + > +/* for a fifo with size n, > + * - [0..n] fill levels are n + 1 states > + * - there are only n different deltas (head - tail) values > + * => not all fill levels can be represented with head, tail > + * pointers only > + * we give up the n fill level, aka fifo full */ > + > +/* sacrifice one elt as a sentinel */ > +static inline int fifo_used(struct dma_fifo *f); > +static inline int fifo_inc_ct(int ct, int size); > +static inline void fifo_dump(struct dma_fifo *fifo); Please reorder and remove the forward declarations. -- Ueimor -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists