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: <20070906153025.7cb71cb1.akpm@linux-foundation.org> Date: Thu, 6 Sep 2007 15:30:25 -0700 From: Andrew Morton <akpm@...ux-foundation.org> To: Matteo Croce <technoboy85@...il.com> Cc: linux-mips@...ux-mips.org, ejka@...i.kspu.ru, jgarzik@...ox.com, netdev@...r.kernel.org, davem@...emloft.net, kuznet@....inr.ac.ru, pekkas@...core.fi, jmorris@...ei.org, yoshfuji@...ux-ipv6.org, kaber@...eworks.de Subject: Re: [PATCH][MIPS][7/7] AR7: ethernet > On Thu, 6 Sep 2007 17:34:10 +0200 Matteo Croce <technoboy85@...il.com> wrote: > Driver for the cpmac 100M ethernet driver. > It works fine disabling napi support, enabling it gives a kernel panic > when the first IPv6 packet has to be forwarded. > Other than that works fine. > I'm not too sure why I got cc'ed on this (and not on patches 1-6?) but whatever. This patch introduces quite a number of basic coding-style mistakes. Please run it through scripts/checkpatch.pl and review the output. The patch introduces vast number of volatile structure fields. Please see Documentation/volatile-considered-harmful.txt. The patch inroduces a modest number of unneeded (and undesirable) casts of void*, such as + struct cpmac_mdio_regs *regs = (struct cpmac_mdio_regs *)bus->priv; please check for those and fix them up. The driver implements a driver-private skb pool. I don't know if this is something which we like net drivers doing? If it is approved then surely there should be a common implementation for it somewhere? The driver does a lot of open-coded dma_cache_inv() calls (in a way which assumes a 32-bit bus, too). I assume that dma_cache_inv() is some mips thing. I'd have thought that it would be better to use the dma mapping API thoughout the driver, and its associated dma invalidation APIs. The driver has some LINUX_VERSION_CODE ifdefs. We usually prefer that such code not be present in a merged-up driver. > + priv->regs->mac_hash_low = 0xffffffff; > + priv->regs->mac_hash_high = 0xffffffff; > + } else { > + for (i = 0, iter = dev->mc_list; i < dev->mc_count; > + i++, iter = iter->next) { > + hash = 0; > + tmp = iter->dmi_addr[0]; > + hash ^= (tmp >> 2) ^ (tmp << 4); > + tmp = iter->dmi_addr[1]; > + hash ^= (tmp >> 4) ^ (tmp << 2); > + tmp = iter->dmi_addr[2]; > + hash ^= (tmp >> 6) ^ tmp; > + tmp = iter->dmi_addr[4]; > + hash ^= (tmp >> 2) ^ (tmp << 4); > + tmp = iter->dmi_addr[5]; > + hash ^= (tmp >> 4) ^ (tmp << 2); > + tmp = iter->dmi_addr[6]; > + hash ^= (tmp >> 6) ^ tmp; > + hash &= 0x3f; > + if (hash < 32) { > + hashlo |= 1<<hash; > + } else { > + hashhi |= 1<<(hash - 32); > + } > + } > + > + priv->regs->mac_hash_low = hashlo; > + priv->regs->mac_hash_high = hashhi; > + } Do we not have a library function anywhere which will perform this little multicasting hash? > +static inline struct sk_buff *cpmac_rx_one(struct net_device *dev, > + struct cpmac_priv *priv, > + struct cpmac_desc *desc) > +{ > + unsigned long flags; > + char *data; > + struct sk_buff *skb, *result = NULL; > + > + priv->regs->rx_ack[0] = virt_to_phys(desc); > + if (unlikely(!desc->datalen)) { > + if (printk_ratelimit()) > + printk(KERN_WARNING "%s: rx: spurious interrupt\n", > + dev->name); > + priv->stats.rx_errors++; > + return NULL; > + } > + > + spin_lock_irqsave(&priv->lock, flags); > + skb = cpmac_get_skb(dev); > + if (likely(skb)) { > + data = (char *)phys_to_virt(desc->hw_data); > + dma_cache_inv((u32)data, desc->datalen); > + skb_put(desc->skb, desc->datalen); > + desc->skb->protocol = eth_type_trans(desc->skb, dev); > + desc->skb->ip_summed = CHECKSUM_NONE; > + priv->stats.rx_packets++; > + priv->stats.rx_bytes += desc->datalen; > + result = desc->skb; > + desc->skb = skb; > + } else { > +#ifdef CPMAC_DEBUG > + if (printk_ratelimit()) > + printk("%s: low on skbs, dropping packet\n", > + dev->name); > +#endif > + priv->stats.rx_dropped++; > + } > + spin_unlock_irqrestore(&priv->lock, flags); > + > + desc->hw_data = virt_to_phys(desc->skb->data); > + desc->buflen = CPMAC_SKB_SIZE; > + desc->dataflags = CPMAC_OWN; > + dma_cache_wback((u32)desc, 16); > + > + return result; > +} This function is far too large to be inlined. > +static irqreturn_t cpmac_irq(int irq, void *dev_id) > +{ > + struct net_device *dev = (struct net_device *)dev_id; unneeded cast > +static void cpmac_tx_timeout(struct net_device *dev) > +{ > + struct cpmac_priv *priv = netdev_priv(dev); > + struct cpmac_desc *desc; > + > + priv->stats.tx_errors++; > + desc = &priv->desc_ring[priv->tx_head++]; > + priv->tx_head %= 8; Is locking not needed for the above? > +static int __devinit cpmac_probe(struct platform_device *pdev) > +{ > + int i, rc, phy_id; > + struct resource *res; > + struct cpmac_priv *priv; > + struct net_device *dev; > + struct plat_cpmac_data *pdata; > + > + if (strcmp(pdev->name, "cpmac") != 0) > + return -ENODEV; I don't think this can happen? If it can, something is pretty screwed up? > + pdata = pdev->dev.platform_data; > + > + for (phy_id = 0; phy_id < PHY_MAX_ADDR; phy_id++) { > + if (!(pdata->phy_mask & (1 << phy_id))) > + continue; > + if (!cpmac_mii.phy_map[phy_id]) > + continue; > + break; > + } > + > + if (phy_id == PHY_MAX_ADDR) { > + if (external_switch) { > + phy_id = 0; > + } else { > + printk("cpmac: no PHY present\n"); > + return -ENODEV; > + } > + } > + > + dev = alloc_etherdev(sizeof(struct cpmac_priv)); > + > + if (!dev) { > + printk(KERN_ERR "cpmac: Unable to allocate net_device structure!\n"); > + return -ENOMEM; > + } > + > + SET_MODULE_OWNER(dev); > + platform_set_drvdata(pdev, dev); > + priv = netdev_priv(dev); > + > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "regs"); > + if (!res) { > + rc = -ENODEV; > + goto fail; > + } > + > + dev->mem_start = res->start; > + dev->mem_end = res->end; > + dev->irq = platform_get_irq_byname(pdev, "irq"); > + > + dev->mtu = 1500; > + dev->open = cpmac_open; > + dev->stop = cpmac_stop; > + dev->set_config = cpmac_config; > + dev->hard_start_xmit = cpmac_start_xmit; > + dev->do_ioctl = cpmac_ioctl; > + dev->get_stats = cpmac_stats; > + dev->change_mtu = cpmac_change_mtu; > + dev->set_mac_address = cpmac_set_mac_address; > + dev->set_multicast_list = cpmac_set_multicast_list; > + dev->tx_timeout = cpmac_tx_timeout; > + dev->ethtool_ops = &cpmac_ethtool_ops; > + if (!disable_napi) { > + dev->poll = cpmac_poll; > + dev->weight = min(rx_ring_size, 64); > + } > + > + memset(priv, 0, sizeof(struct cpmac_priv)); I think alloc_etherdev() already did that? > + spin_lock_init(&priv->lock); > + priv->msg_enable = netif_msg_init(NETIF_MSG_WOL, 0x3fff); > + priv->config = pdata; > + priv->dev = dev; > + memcpy(dev->dev_addr, priv->config->dev_addr, sizeof(dev->dev_addr)); > + if (phy_id == 31) { > + snprintf(priv->phy_name, BUS_ID_SIZE, PHY_ID_FMT, > + cpmac_mii.id, phy_id); > + } else { > + snprintf(priv->phy_name, BUS_ID_SIZE, "fixed@%d:%d", 100, 1); > + } > + > + if ((rc = register_netdev(dev))) { > + printk("cpmac: error %i registering device %s\n", > + rc, dev->name); > + goto fail; > + } > + > + printk("cpmac: device %s (regs: %p, irq: %d, phy: %s, mac: ", > + dev->name, (u32 *)dev->mem_start, dev->irq, > + priv->phy_name); > + for (i = 0; i < 6; i++) > + printk("%02x%s", dev->dev_addr[i], i < 5 ? ":" : ")\n"); > + > + return 0; > + > +fail: > + free_netdev(dev); > + return rc; > +} > + - 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