[Cbe-oss-dev] [RFC 4/9] AXON - Ethernet over PCI-E driver

[Arnd Bergmann]

On Wednesday 20 December 2006 12:13, jdubois at mc.com wrote:
> From: Jean-Christophe DUBOIS <jdubois at mc.com>
> This is an ethernet device emulation between the host and the Cell attached
> to the Axon. It uses the MBX as a messagery and the DMA to move SKBs.

Since this is a network driver, it should probably go to drivers/net/,
and you should take netdev at vger.kernel.org on Cc: when submitting it
for review.

> +
> +#if defined(AXON_DEBUG_NIC)
> +#define dbg_nic_log printk(KERN_DEBUG "AXON_NIC:%s=>", __FUNCTION__);printk
> +#else
> +#define dbg_nic_log if (0) printk
> +#endif
> +
> +#define dbg_nic_err printk(KERN_EMERG "AXON_NIC:%s=>", __FUNCTION__);printk
> +#define dbg_nic_inf printk(KERN_INFO "AXON_NIC:%s=>", __FUNCTION__);printk

dev_dbg/dev_info/dev_err

> +#ifdef __powerpc__
> +#define AXON_NIC_MAC_ADDR "\2AX0N0"
> +#else
> +#define AXON_NIC_MAC_ADDR "\2AX1N0"
> +#endif

You can ask the network layer to generate a random valid mac address
for you, instead of hardcoding these. 

> +
> +#ifndef __powerpc__
> +#define axon_nic_virt_to_bus(x) virt_to_bus( (x) )
> +#else
> +#define axon_nic_virt_to_bus(x) virt_to_phys( (x) )
> +#endif

No, you need to use the pci dma mapping interface, or
the of_device dma mapping on powerpc

> +static struct net_device **axon_nic_devs = NULL;

You shouldn't need to store a global array of these, just attach
it to the struct device you use.

> +static void
> +axon_nic_dma_skb_avail_completion_handler(struct axon_dmax_t
> +					  *p_axon_dmax, struct axon_dma_req_t
> +					  *p_dma_req, void *context)
> +{
> +	struct sk_buff *skb = context;
> +
> +	struct axon_nic_t *axon_nic = netdev_priv(skb->dev);
> +
> +#if defined(AXON_DEBUG_NIC)
> +
> +	dbg_nic_log("Skb 0x%p after DMA completion \n", skb);
> +
> +	axon_nic_skb_print(axon_nic, skb);
> +#endif
> +
> +
> +	skb->protocol = eth_type_trans(skb, skb->dev);
> +
> +
> +	skb->ip_summed = CHECKSUM_UNNECESSARY;
> +
> +	axon_nic->stats.rx_packets++;
> +	axon_nic->stats.rx_bytes += skb_headlen(skb);
> +
> +	skb->dev->last_rx = jiffies;
> +
> +
> +	dbg_nic_log("Skb 0x%p is passed to the network stack\n", skb);
> +	netif_rx(skb);
> +}

I'm not sure I understand what you do here. This looks like you get one
interrupt callback for each incoming packet, which is rather inefficient.
You should probably look into using a NAPI poll() function to avoid
rx and tx interrupts whenever possible.

> +static __init int
> +axon_nic_module_init(void)
> +{
> +	int             ret = 0;
> +
> +	axon_nic_devs_count = axon_board_count();
> +	dbg_nic_inf("Found %d board(s) \n", axon_nic_devs_count);
> +
> +
> +	if (axon_nic_devs_count > 0) {
> +		axon_nic_devs =
> +		    kzalloc(sizeof(struct net_device *) *
> +			    axon_nic_devs_count, GFP_KERNEL);
> +
> +		if (axon_nic_devs != NULL) {
> +			int             i_board;
> +
> +			for (i_board = 0; i_board < axon_nic_devs_count;
> +			     i_board++) {
> +
> +				axon_nic_devs[i_board] =
> +				    alloc_netdev(sizeof(struct axon_nic_t),
> +						 "axon_nic%d",
> +						 axon_nic_init);

Your initialization completely circumvents the Linux driver model.
Normally, the module init function should just register a driver that
is then used for each device that gets found.

I realize that this is not that easy on AXON, since you can use
the same hardware for a number of different tasks and/or kernel
modules. I don't have a good overview of how you try to solve this,
but I think that one side (cell or host) should define how
the AXON interface is used, and the other side should have
a way to detect this. E.g. when you configure the network
interface on one side, the device should pop up on the other side
and the driver loaded automatically. Can you describe which
resources on AXON can be used for different drivers in conflicting
ways? E.g. is everything you need for the network driver always
available, or can all the DMA channels be already in use?

> +module_init(axon_nic_module_init);
> +module_exit(axon_nic_module_cleanup);

these should be right after the respective function.

	Arnd <><