Il Friday 07 September 2007 00:30:25 Andrew Morton ha scritto:

quoted

On Thu, 6 Sep 2007 17:34:10 +0200 Matteo Croce [off-list ref] 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.

I mailed every maintainer in the respective section in the file MAINTAINERS
and you were in the "NETWORK DEVICE DRIVERS" section

This patch introduces quite a number of basic coding-style mistakes. 
Please run it through scripts/checkpatch.pl and review the output.

Already done. I'm collecting other suggestions before committing

The patch introduces vast number of volatile structure fields.  Please see
Documentation/volatile-considered-harmful.txt.

Removing them and the kernel hangs at module load

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.

Done

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?

Are you referring at cpmac_poll?

The driver has some LINUX_VERSION_CODE ifdefs.  We usually prefer that such
code not be present in a merged-up driver.

I will remove in the final release, now I need for testing: my running kernel
is older than current git

quoted

+			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?

Can you tell me the function so i'll implement it?

quoted

+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.

quoted

+static irqreturn_t cpmac_irq(int irq, void *dev_id)
+{
+	struct net_device *dev = (struct net_device *)dev_id;

unneeded cast

fixed

quoted

+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?

Hehe, so screwed that you won't care about your ethernet ;)

quoted

+	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?

What? zeroing the memory or other stuff?

quoted

+	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;
+}
+

What about this?

Thanks for Your attention,
Matteo Croce