Re: [PATCH] dnet: Dave DNET ethernet controller driver

[Eric Dumazet <dada1@cosmosbay.com>]

Ilya Yanok a écrit :
> Driver for Dave DNET ethernet controller found on Dave/DENX QongEVB-LITE
> FPGA. Heavily based on Dave sources, I've just adopted it to current
> kernel version and done some code cleanup.
> 
> Signed-off-by: Ilya Yanok <yanok@emcraft.com>

> +		skb = dev_alloc_skb(pkt_len + 5);
> +		if (skb != NULL) {


> +			skb->dev = dev;

minor nit : This is not necessary anymore
 
	eth_type_trans(skb, dev) does the skb->dev = dev; initialization
in recent kernels.

commit 4c13eb6657fe9ef7b4dc8f1a405c902e9e5234e0
Author: Arnaldo Carvalho de Melo <acme@redhat.com>
Date:   Wed Apr 25 17:40:23 2007 -0700

    [ETH]: Make eth_type_trans set skb->dev like the other *_type_trans

    One less thing for drivers writers to worry about.

    Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
    Signed-off-by: David S. Miller <davem@davemloft.net>


> +			/* Align IP on 16 byte boundaries */
> +			skb_reserve(skb, 2);
> +			/*
> +			 * 'skb_put()' points to the start of sk_buff
> +			 * data area.
> +			 */
> +			data_ptr = (unsigned int *)skb_put(skb, pkt_len);
> +			for (i = 0; i < (pkt_len + 3) >> 2; i++)
> +				*data_ptr++ = dnet_readl(bp, RX_DATA_FIFO);
> +			skb->protocol = eth_type_trans(skb, dev);
> +			netif_receive_skb(skb);
> +			bp->dev->last_rx = jiffies;


This last_rx init is not anymore needed :

commit babcda74e9d96bb58fd9c6c5112dbdbff169e695
Author: David S. Miller <davem@davemloft.net>
Date:   Mon Nov 3 21:11:17 2008 -0800

    drivers/net: Kill now superfluous ->last_rx stores.

    The generic packet receive code takes care of setting
    netdev->last_rx when necessary, for the sake of the
    bonding ARP monitor.

    Drivers need not do it any more.

    Some cases had to be skipped over because the drivers
    were making use of the ->last_rx value themselves.

    Signed-off-by: David S. Miller <davem@davemloft.net>



> +			npackets++;
> +		} else
> +			printk(KERN_NOTICE
> +			       "%s: No memory to allocate a sk_buff of "
> +			       "size %d.\n", dev->name, pkt_len);

pkt_len is an "unsigned int", so please use %u

> +	}
> +




+static struct ethtool_ops dnet_ethtool_ops = {
+	.get_settings		= dnet_get_settings,
+	.set_settings		= dnet_set_settings,
+	.get_drvinfo		= dnet_get_drvinfo,
+	.get_link		= ethtool_op_get_link,
+};
+

Please add a const qualifier to this.



struct dnet {
+	void __iomem			*regs;
+	spinlock_t			lock;
+	struct platform_device	*pdev;
+	struct net_device		*dev;
+	struct net_device_stats	stats;
+	struct dnet_stats		hw_stats;
+	unsigned int			capabilities; /* read from FPGA */
+	struct napi_struct		napi;


Are you sure you need a "struct net_device_stats stats;" in "struct dnet" ?
One is already included in "struct net_device", you probably can use it.

Thank you