lkml.org 
[lkml]   [2012]   [Jul]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [RFC 2/2] net: Add support for NTB virtual ethernet device
On Sat, Jul 14, 2012 at 01:14:03AM +0200, Jiri Pirko wrote:
> Fri, Jul 13, 2012 at 11:45:00PM CEST, jon.mason@intel.com wrote:
> >A virtual ethernet device that uses the NTB transport API to send/receive data.
> >
> >Signed-off-by: Jon Mason <jon.mason@intel.com>
> >---
> > drivers/net/Kconfig | 4 +
> > drivers/net/Makefile | 1 +
> > drivers/net/ntb_netdev.c | 411 ++++++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 416 insertions(+), 0 deletions(-)
> > create mode 100644 drivers/net/ntb_netdev.c
> >
> >diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
> >index 0c2bd80..9bf8a71 100644
> >--- a/drivers/net/Kconfig
> >+++ b/drivers/net/Kconfig
> >@@ -178,6 +178,10 @@ config NETPOLL_TRAP
> > config NET_POLL_CONTROLLER
> > def_bool NETPOLL
> >
> >+config NTB_NETDEV
> >+ tristate "Virtual Ethernet over NTB"
> >+ depends on NTB
> >+
> > config RIONET
> > tristate "RapidIO Ethernet over messaging driver support"
> > depends on RAPIDIO
> >diff --git a/drivers/net/Makefile b/drivers/net/Makefile
> >index 3d375ca..9890148 100644
> >--- a/drivers/net/Makefile
> >+++ b/drivers/net/Makefile
> >@@ -69,3 +69,4 @@ obj-$(CONFIG_USB_IPHETH) += usb/
> > obj-$(CONFIG_USB_CDC_PHONET) += usb/
> >
> > obj-$(CONFIG_HYPERV_NET) += hyperv/
> >+obj-$(CONFIG_NTB_NETDEV) += ntb_netdev.o
> >diff --git a/drivers/net/ntb_netdev.c b/drivers/net/ntb_netdev.c
> >new file mode 100644
> >index 0000000..bcbd9d4
> >--- /dev/null
> >+++ b/drivers/net/ntb_netdev.c
> >@@ -0,0 +1,411 @@
> >+/*
> >+ * This file is provided under a dual BSD/GPLv2 license. When using or
> >+ * redistributing this file, you may do so under either license.
> >+ *
> >+ * GPL LICENSE SUMMARY
> >+ *
> >+ * Copyright(c) 2012 Intel Corporation. All rights reserved.
> >+ *
> >+ * This program is free software; you can redistribute it and/or modify
> >+ * it under the terms of version 2 of the GNU General Public License as
> >+ * published by the Free Software Foundation.
> >+ *
> >+ * This program is distributed in the hope that it will be useful, but
> >+ * WITHOUT ANY WARRANTY; without even the implied warranty of
> >+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> >+ * General Public License for more details.
> >+ *
> >+ * You should have received a copy of the GNU General Public License
> >+ * along with this program; if not, write to the Free Software
> >+ * Foundation, Inc., 51 Franklin St - Fifth Floor, Boston, MA 02110-1301 USA.
> >+ * The full GNU General Public License is included in this distribution
> >+ * in the file called LICENSE.GPL.
> >+ *
> >+ * BSD LICENSE
> >+ *
> >+ * Copyright(c) 2012 Intel Corporation. All rights reserved.
> >+ *
> >+ * Redistribution and use in source and binary forms, with or without
> >+ * modification, are permitted provided that the following conditions
> >+ * are met:
> >+ *
> >+ * * Redistributions of source code must retain the above copyright
> >+ * notice, this list of conditions and the following disclaimer.
> >+ * * Redistributions in binary form must reproduce the above copy
> >+ * notice, this list of conditions and the following disclaimer in
> >+ * the documentation and/or other materials provided with the
> >+ * distribution.
> >+ * * Neither the name of Intel Corporation nor the names of its
> >+ * contributors may be used to endorse or promote products derived
> >+ * from this software without specific prior written permission.
> >+ *
> >+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> >+ * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> >+ * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> >+ * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
> >+ * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> >+ * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> >+ * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> >+ * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> >+ * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> >+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> >+ * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> >+ *
> >+ * Intel PCIe NTB Network Linux driver
> >+ *
> >+ * Contact Information:
> >+ * Jon Mason <jon.mason@intel.com>
> >+ */
> >+#include <linux/etherdevice.h>
> >+#include <linux/ethtool.h>
> >+#include <linux/module.h>
> >+#include <linux/ntb.h>
> >+
> >+#define NTB_NETDEV_VER "0.4"
>
> Is it really necessary to provide this in-file versioning? Doesn't
> kernel version itself do the trick?

Not necessarily. This may be distributed as a package outside of the kernel and the version is useful for debug.

>
> >+
> >+MODULE_DESCRIPTION(KBUILD_MODNAME);
> >+MODULE_VERSION(NTB_NETDEV_VER);
> >+MODULE_LICENSE("Dual BSD/GPL");
> >+MODULE_AUTHOR("Intel Corporation");
> >+
> >+struct ntb_netdev {
> >+ struct net_device *ndev;
> >+ struct ntb_transport_qp *qp;
> >+};
> >+
> >+#define NTB_TX_TIMEOUT_MS 1000
> >+#define NTB_RXQ_SIZE 100
> >+
> >+static struct net_device *netdev;
> >+
> >+static void ntb_netdev_event_handler(int status)
> >+{
> >+ struct ntb_netdev *dev = netdev_priv(netdev);
> >+
> >+ pr_debug("%s: Event %x, Link %x\n", KBUILD_MODNAME, status,
> >+ ntb_transport_link_query(dev->qp));
> >+
> >+ /* Currently, only link status event is supported */
> >+ if (status)
> >+ netif_carrier_on(netdev);
> >+ else
> >+ netif_carrier_off(netdev);
> >+}
> >+
> >+static void ntb_netdev_rx_handler(struct ntb_transport_qp *qp)
> >+{
> >+ struct net_device *ndev = netdev;
> >+ struct sk_buff *skb;
> >+ int len, rc;
> >+
> >+ while ((skb = ntb_transport_rx_dequeue(qp, &len))) {
> >+ pr_debug("%s: %d byte payload received\n", __func__, len);
> >+
> >+ skb_put(skb, len);
> >+ skb->protocol = eth_type_trans(skb, ndev);
> >+ skb->ip_summed = CHECKSUM_NONE;
> >+
> >+ if (netif_rx(skb) == NET_RX_DROP) {
> >+ ndev->stats.rx_errors++;
> >+ ndev->stats.rx_dropped++;
> >+ } else {
> >+ ndev->stats.rx_packets++;
> >+ ndev->stats.rx_bytes += len;
> >+ }
> >+
> >+ skb = netdev_alloc_skb(ndev, ndev->mtu + ETH_HLEN);
> >+ if (!skb) {
> >+ ndev->stats.rx_errors++;
> >+ ndev->stats.rx_frame_errors++;
> >+ pr_err("%s: No skb\n", __func__);
> >+ break;
> >+ }
> >+
> >+ rc = ntb_transport_rx_enqueue(qp, skb, skb->data,
> >+ ndev->mtu + ETH_HLEN);
> >+ if (rc) {
> >+ ndev->stats.rx_errors++;
> >+ ndev->stats.rx_fifo_errors++;
> >+ pr_err("%s: error re-enqueuing\n", __func__);
> >+ break;
> >+ }
> >+ }
> >+}
> >+
> >+static void ntb_netdev_tx_handler(struct ntb_transport_qp *qp)
> >+{
> >+ struct net_device *ndev = netdev;
> >+ struct sk_buff *skb;
> >+ int len;
> >+
> >+ while ((skb = ntb_transport_tx_dequeue(qp, &len))) {
> >+ ndev->stats.tx_packets++;
> >+ ndev->stats.tx_bytes += skb->len;
> >+ dev_kfree_skb(skb);
> >+ }
> >+
> >+ if (netif_queue_stopped(ndev))
> >+ netif_wake_queue(ndev);
> >+}
> >+
> >+static netdev_tx_t ntb_netdev_start_xmit(struct sk_buff *skb,
> >+ struct net_device *ndev)
> >+{
> >+ struct ntb_netdev *dev = netdev_priv(ndev);
> >+ int rc;
> >+
> >+ pr_debug("%s: ntb_transport_tx_enqueue\n", KBUILD_MODNAME);
> >+
> >+ rc = ntb_transport_tx_enqueue(dev->qp, skb, skb->data, skb->len);
> >+ if (rc)
> >+ goto err;
> >+
> >+ return NETDEV_TX_OK;
> >+
> >+err:
> >+ ndev->stats.tx_dropped++;
> >+ ndev->stats.tx_errors++;
> >+ netif_stop_queue(ndev);
> >+ return NETDEV_TX_BUSY;
> >+}
> >+
> >+static int ntb_netdev_open(struct net_device *ndev)
> >+{
> >+ struct ntb_netdev *dev = netdev_priv(ndev);
> >+ struct sk_buff *skb;
> >+ int rc, i, len;
> >+
> >+ /* Add some empty rx bufs */
> >+ for (i = 0; i < NTB_RXQ_SIZE; i++) {
> >+ skb = netdev_alloc_skb(ndev, ndev->mtu + ETH_HLEN);
> >+ if (!skb) {
> >+ rc = -ENOMEM;
> >+ goto err;
> >+ }
> >+
> >+ rc = ntb_transport_rx_enqueue(dev->qp, skb, skb->data,
> >+ ndev->mtu + ETH_HLEN);
> >+ if (rc == -EINVAL)
> >+ goto err;
> >+ }
> >+
> >+ netif_carrier_off(ndev);
> >+ ntb_transport_link_up(dev->qp);
> >+
> >+ return 0;
> >+
> >+err:
> >+ while ((skb = ntb_transport_rx_remove(dev->qp, &len)))
> >+ kfree(skb);
> >+ return rc;
> >+}
> >+
> >+static int ntb_netdev_close(struct net_device *ndev)
> >+{
> >+ struct ntb_netdev *dev = netdev_priv(ndev);
> >+ struct sk_buff *skb;
> >+ int len;
> >+
> >+ ntb_transport_link_down(dev->qp);
> >+
> >+ while ((skb = ntb_transport_rx_remove(dev->qp, &len)))
> >+ kfree(skb);
> >+
> >+ return 0;
> >+}
> >+
> >+static int ntb_netdev_change_mtu(struct net_device *ndev, int new_mtu)
> >+{
> >+ struct ntb_netdev *dev = netdev_priv(ndev);
> >+ struct sk_buff *skb;
> >+ int len, rc;
> >+
> >+ if (new_mtu > ntb_transport_max_size(dev->qp) - ETH_HLEN)
> >+ return -EINVAL;
> >+
> >+ if (!netif_running(ndev)) {
> >+ ndev->mtu = new_mtu;
> >+ return 0;
> >+ }
> >+
> >+ /* Bring down the link and dispose of posted rx entries */
> >+ ntb_transport_link_down(dev->qp);
> >+
> >+ if (ndev->mtu < new_mtu) {
> >+ int i;
> >+
> >+ for (i = 0; (skb = ntb_transport_rx_remove(dev->qp, &len)); i++)
> >+ kfree(skb);
> >+
> >+ for (; i; i--) {
> >+ skb = netdev_alloc_skb(ndev, new_mtu + ETH_HLEN);
> >+ if (!skb) {
> >+ rc = -ENOMEM;
> >+ goto err;
> >+ }
> >+
> >+ rc = ntb_transport_rx_enqueue(dev->qp, skb, skb->data,
> >+ new_mtu + ETH_HLEN);
> >+ if (rc) {
> >+ kfree(skb);
> >+ goto err;
> >+ }
> >+ }
> >+ }
> >+
> >+ ndev->mtu = new_mtu;
> >+
> >+ ntb_transport_link_up(dev->qp);
> >+
> >+ return 0;
> >+
> >+err:
> >+ ntb_transport_link_down(dev->qp);
> >+
> >+ while ((skb = ntb_transport_rx_remove(dev->qp, &len)))
> >+ kfree(skb);
> >+
> >+ pr_err("Error changing MTU, device inoperable\n");
>
> Would be maybe better to use netdev_err here (and on similar other
> places)

Good point

>
> Also, it might be good to provide rollback in case any of
> netdev_alloc_skb() fails.
>
> >+ return rc;
> >+}
> >+
> >+static void ntb_netdev_tx_timeout(struct net_device *ndev)
> >+{
> >+ if (netif_running(ndev))
> >+ netif_wake_queue(ndev);
> >+}
> >+
> >+static const struct net_device_ops ntb_netdev_ops = {
> >+ .ndo_open = ntb_netdev_open,
> >+ .ndo_stop = ntb_netdev_close,
> >+ .ndo_start_xmit = ntb_netdev_start_xmit,
> >+ .ndo_change_mtu = ntb_netdev_change_mtu,
> >+ .ndo_tx_timeout = ntb_netdev_tx_timeout,
> >+ .ndo_set_mac_address = eth_mac_addr,
>
> Does your device support mac change while it's up and running?

It's virtual ethernet, so there is no hardware limitation, only what is acceptable for the remote side to receive.

>
> >+};
> >+
> >+static void ntb_get_drvinfo(__attribute__((unused)) struct net_device *dev,
> >+ struct ethtool_drvinfo *info)
> >+{
> >+ strlcpy(info->driver, KBUILD_MODNAME, sizeof(info->driver));
> >+ strlcpy(info->version, NTB_NETDEV_VER, sizeof(info->version));
> >+}
> >+
> >+static const char ntb_nic_stats[][ETH_GSTRING_LEN] = {
> >+ "rx_packets", "rx_bytes", "rx_errors", "rx_dropped", "rx_length_errors",
> >+ "rx_frame_errors", "rx_fifo_errors",
> >+ "tx_packets", "tx_bytes", "tx_errors", "tx_dropped",
> >+};
> >+
> >+static int ntb_get_stats_count(__attribute__((unused)) struct net_device *dev)
> >+{
> >+ return ARRAY_SIZE(ntb_nic_stats);
> >+}
> >+
> >+static int ntb_get_sset_count(struct net_device *dev, int sset)
> >+{
> >+ switch (sset) {
> >+ case ETH_SS_STATS:
> >+ return ntb_get_stats_count(dev);
> >+ default:
> >+ return -EOPNOTSUPP;
> >+ }
> >+}
> >+
> >+static void ntb_get_strings(__attribute__((unused)) struct net_device *dev,
> >+ u32 sset, u8 *data)
> >+{
> >+ switch (sset) {
> >+ case ETH_SS_STATS:
> >+ memcpy(data, *ntb_nic_stats, sizeof(ntb_nic_stats));
> >+ }
> >+}
> >+
> >+static void
> >+ntb_get_ethtool_stats(struct net_device *dev,
> >+ __attribute__((unused)) struct ethtool_stats *stats,
> >+ u64 *data)
> >+{
> >+ int i = 0;
> >+
> >+ data[i++] = dev->stats.rx_packets;
> >+ data[i++] = dev->stats.rx_bytes;
> >+ data[i++] = dev->stats.rx_errors;
> >+ data[i++] = dev->stats.rx_dropped;
> >+ data[i++] = dev->stats.rx_length_errors;
> >+ data[i++] = dev->stats.rx_frame_errors;
> >+ data[i++] = dev->stats.rx_fifo_errors;
> >+ data[i++] = dev->stats.tx_packets;
> >+ data[i++] = dev->stats.tx_bytes;
> >+ data[i++] = dev->stats.tx_errors;
> >+ data[i++] = dev->stats.tx_dropped;
> >+}
> >+
> >+static const struct ethtool_ops ntb_ethtool_ops = {
> >+ .get_drvinfo = ntb_get_drvinfo,
> >+ .get_sset_count = ntb_get_sset_count,
> >+ .get_strings = ntb_get_strings,
> >+ .get_ethtool_stats = ntb_get_ethtool_stats,
> >+ .get_link = ethtool_op_get_link,
> >+};
> >+
> >+static int __init ntb_netdev_init_module(void)
> >+{
> >+ struct ntb_netdev *dev;
> >+ int rc;
> >+
> >+ pr_info("%s: Probe\n", KBUILD_MODNAME);
> >+
> >+ netdev = alloc_etherdev(sizeof(struct ntb_netdev));
>
> I might be missing something but this place (module init) does not seems
> like a good place to do alloc_etherdev(). Do you want to support only
> one netdevice instance?
>
> Anyway, I think that using "static netdev" should be avoided in any case.
>

It would fail the probe if there is no underlying ntb hardware, but it would make sense to check for that before allocing the etherdev.

Thanks for the comments!


> >+ if (!netdev)
> >+ return -ENOMEM;
> >+
> >+ dev = netdev_priv(netdev);
> >+ dev->ndev = netdev;
> >+ netdev->features = NETIF_F_HIGHDMA;
> >+
> >+ netdev->hw_features = netdev->features;
> >+ netdev->watchdog_timeo = msecs_to_jiffies(NTB_TX_TIMEOUT_MS);
> >+
> >+ random_ether_addr(netdev->perm_addr);
> >+ memcpy(netdev->dev_addr, netdev->perm_addr, netdev->addr_len);
> >+
> >+ netdev->netdev_ops = &ntb_netdev_ops;
> >+ SET_ETHTOOL_OPS(netdev, &ntb_ethtool_ops);
> >+
> >+ dev->qp = ntb_transport_create_queue(ntb_netdev_rx_handler,
> >+ ntb_netdev_tx_handler,
> >+ ntb_netdev_event_handler);
> >+ if (!dev->qp) {
> >+ rc = -EIO;
> >+ goto err;
> >+ }
> >+
> >+ netdev->mtu = ntb_transport_max_size(dev->qp) - ETH_HLEN;
> >+
> >+ rc = register_netdev(netdev);
> >+ if (rc)
> >+ goto err1;
> >+
> >+ pr_info("%s: %s created\n", KBUILD_MODNAME, netdev->name);
> >+ return 0;
> >+
> >+err1:
> >+ ntb_transport_free_queue(dev->qp);
> >+err:
> >+ free_netdev(netdev);
> >+ return rc;
> >+}
> >+module_init(ntb_netdev_init_module);
> >+
> >+static void __exit ntb_netdev_exit_module(void)
> >+{
> >+ struct ntb_netdev *dev = netdev_priv(netdev);
> >+
> >+ unregister_netdev(netdev);
> >+ ntb_transport_free_queue(dev->qp);
> >+ free_netdev(netdev);
> >+
> >+ pr_info("%s: Driver removed\n", KBUILD_MODNAME);
> >+}
> >+module_exit(ntb_netdev_exit_module);
> >--
> >1.7.5.4
> >
> >--
> >To unsubscribe from this list: send the line "unsubscribe netdev" in
> >the body of a message to majordomo@vger.kernel.org
> >More majordomo info at http://vger.kernel.org/majordomo-info.html


\
 
 \ /
  Last update: 2012-07-14 08:21    [from the cache]
©2003-2011 Jasper Spaans