Re: [PATCH net-next-2.6 09/13] net-caif: add CAIF netdevice
From: Patrick McHardy <hidden>
Date: 2010-01-21 08:03:10
sjur.brandeland@stericsson.com wrote:
+static void ipcaif_net_init(struct net_device *dev)
+{
+ struct chnl_net *priv;
+ dev->netdev_ops = &netdev_ops;
+ dev->destructor = free_netdev;These (especially ->destructor) should be set in the setup function.
+ dev->flags |= IFF_NOARP; + dev->flags |= IFF_POINTOPOINT; + dev->needed_headroom = CAIF_NEEDED_HEADROOM; + dev->needed_tailroom = CAIF_NEEDED_TAILROOM; + dev->mtu = SIZE_MTU; + dev->tx_queue_len = CAIF_NET_DEFAULT_QUEUE_LEN;
These too I guess, its uncommon to reinitialize mtu and tx_queue_len when setting a device down and up again.
+ + priv = (struct chnl_net *)netdev_priv(dev); + priv->chnl.receive = chnl_recv_cb; + priv->chnl.ctrlcmd = chnl_flowctrl_cb; + priv->netdev = dev; + priv->config.type = CAIF_CHTY_DATAGRAM; + priv->config.phy_pref = CFPHYPREF_HIGH_BW; + priv->config.priority = CAIF_PRIO_LOW; + priv->config.u.dgm.connection_id = -1; /* Insert illegal value */ + priv->flowenabled = false; + + ASSERT_RTNL(); + init_waitqueue_head(&priv->netmgmt_wq); + list_add(&priv->list_field, &chnl_net_list); +}
+static int chnl_net_hard_start_xmit(struct sk_buff *skb, struct net_device *dev)
static netdev_tx_t
+{
+ struct chnl_net *priv;
+ struct cfpkt *pkt = NULL;
+ int len;
+ int result = -1;
+
+ /* Get our private data. */
+ priv = (struct chnl_net *)netdev_priv(dev);
+ if (!priv)
+ return -ENOSPC;This is an impossible condition, netdev_priv() will never return NULL. The cast is also unnecessary.
+
+
+ if (skb->len > priv->netdev->mtu) {
+ pr_warning("CAIF: %s(): Size of skb exceeded MTU\n", __func__);
+ return -ENOSPC;
+ }
+
+ if (!priv->flowenabled) {
+ pr_debug("CAIF: %s(): dropping packets flow off\n", __func__);
+ return NETDEV_TX_BUSY;
+ }
+
+ if (priv->config.type == CAIF_CHTY_DATAGRAM_LOOP) {
+ struct iphdr *hdr;
+ __be32 swap;
+ /* Retrieve IP header. */
+ hdr = ip_hdr(skb);
+ /* Change source and destination address. */
+ swap = hdr->saddr;
+ hdr->saddr = hdr->daddr;
+ hdr->daddr = swap;swap()?
+ }
+ /* Store original SKB length. */
+ len = skb->len;
+
+ pkt = cfpkt_fromnative(CAIF_DIR_OUT, (void *) skb);
+
+ /* Send the packet down the stack. */
+ result = priv->chnl.dn->transmit(priv->chnl.dn, pkt);
+ if (result) {
+ if (result == CFGLU_ERETRY)
+ result = NETDEV_TX_BUSY;
+ return result;
+ }
+
+ /* Update statistics. */
+ dev->stats.tx_packets++;
+ dev->stats.tx_bytes += len;
+
+ return NETDEV_TX_OK;
+}+
+static int ipcaif_newlink(struct net *src_net, struct net_device *dev,
+ struct nlattr *tb[], struct nlattr *data[])
+{
+ int err;
+ struct chnl_net *caifdev;
+ ASSERT_RTNL();
+ caifdev = netdev_priv(dev);
+ caif_netlink_parms(data, &caifdev->config);
+ err = register_netdevice(dev);
+ if (err) {
+ pr_warning("CAIF: %s(): device rtml registration failed\n",
+ __func__);
+ goto out;
+ }
+ dev_hold(dev);What is this reference used for? You don't have a dellink function, so this looks like a leak.
+out:
+ return err;
+}
+
+static int ipcaif_changelink(struct net_device *dev, struct nlattr *tb[],
+ struct nlattr *data[])
+{
+ struct chnl_net *caifdev;
+ ASSERT_RTNL();
+ caifdev = netdev_priv(dev);
+ caif_netlink_parms(data, &caifdev->config);
+ netdev_state_change(dev);
+ return 0;
+}
+
+static size_t ipcaif_get_size(const struct net_device *dev)
+{
+ return
+ /* IFLA_CAIF_IPV4_CONNID */
+ nla_total_size(4) +
+ /* IFLA_CAIF_IPV6_CONNID */
+ nla_total_size(4) +
+ /* IFLA_CAIF_LOOPBACK */
+ nla_total_size(2) +
+ 0;
+}
+
+static const struct nla_policy ipcaif_policy[IFLA_CAIF_MAX + 1] = {
+ [IFLA_CAIF_IPV4_CONNID] = { .type = NLA_U32 },
+ [IFLA_CAIF_IPV6_CONNID] = { .type = NLA_U32 },
+ [IFLA_CAIF_LOOPBACK] = { .type = NLA_U8 }
+};
+
+
+static struct rtnl_link_ops ipcaif_link_ops __read_mostly = {
+ .kind = "caif",
+ .priv_size = (size_t)sizeof(struct chnl_net),Unnecessary cast.
+ .setup = ipcaif_net_init,
+ .maxtype = IFLA_CAIF_MAX,
+ .policy = ipcaif_policy,
+ .newlink = ipcaif_newlink,
+ .changelink = ipcaif_changelink,
+ .get_size = ipcaif_get_size,
+ .fill_info = ipcaif_fill_info,
+
+};
+
+int chnl_net_ioctl(unsigned int cmd, unsigned long arg, bool from_user_land)
+{
+ struct chnl_net *priv;
+ int result = -1;
+ struct chnl_net *dev;
+ struct net_device *netdevptr;
+ int ret;
+ struct ifreq ifreq;
+ struct ifcaif_param param;
+ rtnl_lock();
+ if (from_user_land) {
+ if (copy_from_user(&ifreq, (const void *)arg, sizeof(ifreq)))
+ return -EFAULT;
+ } else
+ memcpy(&ifreq, (void *)arg, sizeof(ifreq));Why do you need both an ioctl and a netlink interface?
+
+ if (cmd == SIOCCAIFNETREMOVE) {
+ pr_debug("CAIF: %s(): %s\n", __func__, ifreq.ifr_name);
+ dev = find_device(ifreq.ifr_name);
+ if (!dev)
+ ret = -ENODEV;
+ else
+ ret = delete_device(dev);
+ rtnl_unlock();
+ return ret;
+ }
+
+ if (cmd != SIOCCAIFNETNEW) {
+ rtnl_unlock();
+ return -ENOIOCTLCMD;
+ }
+ if (ifreq.ifr_ifru.ifru_data != NULL) {
+ if (from_user_land) {
+ ret = copy_from_user(¶m,
+ ifreq.ifr_ifru.ifru_data,
+ sizeof(param));
+ if (ret) {
+ rtnl_unlock();
+ return -EFAULT;
+ }
+ } else
+ memcpy(¶m,
+ ifreq.ifr_ifru.ifru_data,
+ sizeof(param));
+ ifreq.ifr_ifru.ifru_data = ¶m;
+ }
+
+ netdevptr = alloc_netdev(sizeof(struct chnl_net),
+ ifreq.ifr_name, ipcaif_net_init);
+ if (!netdevptr) {
+ rtnl_unlock();
+ return -ENODEV;
+ }
+ dev_hold(netdevptr);
+ priv = (struct chnl_net *)netdev_priv(netdevptr);
+ priv->config.u.dgm.connection_id = param.ipv4_connid;
+
+ if (param.loop)
+ priv->config.type = CAIF_CHTY_DATAGRAM_LOOP;
+ else
+ priv->config.type = CAIF_CHTY_DATAGRAM;
+
+ result = register_netdevice(priv->netdev);
+
+ if (result < 0) {
+ pr_warning("CAIF: %s(): can't register netdev %s %d\n",
+ __func__, ifreq.ifr_name, result);
+ dev_put(netdevptr);
+ rtnl_unlock();
+ return -ENODEV;
+ }
+ pr_debug("CAIF: %s(): netdev channel open:%s\n", __func__, priv->name);
+ rtnl_unlock();
+ return 0;
+};
+
+struct net_device *chnl_net_create(char *name,
+ struct caif_channel_config *config)
+{
+ struct net_device *dev;
+ ASSERT_RTNL();
+ dev = alloc_netdev(sizeof(struct chnl_net), name, ipcaif_net_init);
+ if (!dev)
+ return NULL;
+ ((struct chnl_net *)netdev_priv(dev))->config = *config;
+ dev_hold(dev);
+ return dev;
+}
+EXPORT_SYMBOL(chnl_net_create);
+
+static int __init chnl_init_module(void)
+{
+ int err = -1;
+ caif_register_ioctl(chnl_net_ioctl);
+ err = rtnl_link_register(&ipcaif_link_ops);
+ if (err < 0) {
+ rtnl_link_unregister(&ipcaif_link_ops);You don't need to unregister on error. The ioctl should be unregistered I guess.
+ return err;
+ }
+ return 0;
+}
+
+static void __exit chnl_exit_module(void)
+{
+ struct chnl_net *dev = NULL;
+ struct list_head *list_node;
+ struct list_head *_tmp;
+ rtnl_lock();
+ list_for_each_safe(list_node, _tmp, &chnl_net_list) {
+ dev = list_entry(list_node, struct chnl_net, list_field);
+ delete_device(dev);
+ }
+ rtnl_unlock();
+ rtnl_link_unregister(&ipcaif_link_ops);
+ caif_register_ioctl(NULL);This is racy, rtnl_link_unregister() will clean up all CAIF devices, but the ioctl handler might register new ones after that. I'd suggest to drop the ioctl interface completely.
+} + +module_init(chnl_init_module); +module_exit(chnl_exit_module);