Re: [PATCH][MIPS][7/7] AR7: ethernet

[Matteo Croce <technoboy85@gmail.com>]

Il Friday 07 September 2007 00:30:25 Andrew Morton ha scritto:
> > On Thu, 6 Sep 2007 17:34:10 +0200 Matteo Croce <technoboy85@gmail.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.

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

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

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

fixed

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

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

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