Re: [PATCH][5/5] RapidIO support: net driver over messaging

[Stephen Hemminger <shemminger@osdl.org>]

How much is this like ethernet? does it still do ARP?
Can it do promiscious receive?

> +LIST_HEAD(rionet_peers);

Does this have to be global?

Not sure about the locking of this stuff, are you
relying on the RTNL?

> +
> +static int rionet_change_mtu(struct net_device *ndev, int new_mtu)
> +{
> +	struct rionet_private *rnet = ndev->priv;
> +
> +	if (netif_msg_drv(rnet))
> +		printk(KERN_WARNING
> +		       "%s: rionet_change_mtu(): not implemented\n", DRV_NAME);
> +
> +	return 0;
> +}

If you can allow any mtu then don't need this at all.
Or if you are limited then better return an error for bad values.

> +static void rionet_set_multicast_list(struct net_device *ndev)
> +{
> +	struct rionet_private *rnet = ndev->priv;
> +
> +	if (netif_msg_drv(rnet))
> +		printk(KERN_WARNING
> +		       "%s: rionet_set_multicast_list(): not implemented\n",
> +		       DRV_NAME);
> +}

If you can't handle it then just leave dev->set_multicast_list
as NULL and all attempts to add or delete will get -EINVAL

> +
> +static int rionet_open(struct net_device *ndev)
> +{


> +	/* Initialize inbound message ring */
> +	for (i = 0; i < RIONET_RX_RING_SIZE; i++)
> +		rnet->rx_skb[i] = NULL;
> +	rnet->rx_slot = 0;
> +	rionet_rx_fill(ndev, 0);
> +
> +	rnet->tx_slot = 0;
> +	rnet->tx_cnt = 0;
> +	rnet->ack_slot = 0;
> +
> +	spin_lock_init(&rnet->lock);
> +
> +	rnet->msg_enable = RIONET_DEFAULT_MSGLEVEL;

Better to do all initialization of the per device data
in the place it is allocated (rio_setup_netdev)
> +
> +static int rionet_ioctl(struct net_device *ndev, struct ifreq *rq, int cmd)
> +{
> +	return -EOPNOTSUPP;
> +}

Unneeded, if dev->do_ioctl is NULL, then all private ioctl's will
return -EINVAL that is what you want.

> +
> +static u32 rionet_get_link(struct net_device *ndev)
> +{
> +	return netif_carrier_ok(ndev);
> +}

Use ethtool_op_get_link
> +
> +static int rionet_setup_netdev(struct rio_mport *mport)
> +{
> +	int rc = 0;
> +	struct net_device *ndev = NULL;
> +	struct rionet_private *rnet;
> +	u16 device_id;
> +
> +	/* Allocate our net_device structure */
> +	ndev = alloc_etherdev(sizeof(struct rionet_private));
> +	if (ndev == NULL) {
> +		printk(KERN_INFO "%s: could not allocate ethernet device.\n",
> +		       DRV_NAME);
> +		rc = -ENOMEM;
> +		goto out;
> +	}
> +
> +	/*
> +	 * XXX hack, store point a static at ndev so we can get it...
> +	 * Perhaps need an array of these that the handler can
> +	 * index via the mbox number.
> +	 */
> +	sndev = ndev;
> +
> +	/* Set up private area */
> +	rnet = (struct rionet_private *)ndev->priv;
> +	rnet->mport = mport;
> +
> +	/* Set the default MAC address */
> +	device_id = rio_local_get_device_id(mport);
> +	ndev->dev_addr[0] = 0x00;
> +	ndev->dev_addr[1] = 0x01;
> +	ndev->dev_addr[2] = 0x00;
> +	ndev->dev_addr[3] = 0x01;
> +	ndev->dev_addr[4] = device_id >> 8;
> +	ndev->dev_addr[5] = device_id & 0xff;
> +
> +	/* Fill in the driver function table */
> +	ndev->open = &rionet_open;
> +	ndev->hard_start_xmit = &rionet_start_xmit;
> +	ndev->stop = &rionet_close;
> +	ndev->get_stats = &rionet_stats;
> +	ndev->change_mtu = &rionet_change_mtu;
> +	ndev->set_mac_address = &rionet_set_mac_address;
> +	ndev->set_multicast_list = &rionet_set_multicast_list;
> +	ndev->do_ioctl = &rionet_ioctl;
> +	SET_ETHTOOL_OPS(ndev, &rionet_ethtool_ops);
> +
> +	ndev->mtu = RIO_MAX_MSG_SIZE - 14;
> +
> +	SET_MODULE_OWNER(ndev);

Can you set any ndev->features to get better performance. 
	Can you take >32bit data addresses? then set HIGHDMA
	You are doing your on locking, can you use LLTX?
	Does the hardware support scatter gather?
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/