RE: [PATCH net-next 1/9] net: hns3: Add support of HNS3 Ethernet Driver for hip08 SoC
Hi Yuval,
> -Original Message-
> From: Mintz, Yuval [mailto:yuval.mi...@cavium.com]
> Sent: Wednesday, June 14, 2017 9:04 AM
> To: Salil Mehta; da...@davemloft.net
> Cc: Zhuangyuzeng (Yisen); huangdaode; lipeng (Y);
> mehta.salil@gmail.com; netdev@vger.kernel.org; linux-
> ker...@vger.kernel.org; Linuxarm
> Subject: RE: [PATCH net-next 1/9] net: hns3: Add support of HNS3
> Ethernet Driver for hip08 SoC
>
> > > > +static void hns3_nic_net_down(struct net_device *ndev) {
> > > > + struct hns3_nic_priv *priv = netdev_priv(ndev);
> > > > + struct hnae3_ae_ops *ops;
> > > > + int i;
> > > > +
> > > > + netif_tx_stop_all_queues(ndev);
> > > > + netif_carrier_off(ndev);
> > > > + netif_tx_disable(ndev);
> > > > +
> > > > + ops = priv->ae_handle->ae_algo->ops;
> > > > +
> > > > + if (ops->stop)
> > > > + ops->stop(priv->ae_handle);
> > > > +
> > > > + netif_tx_stop_all_queues(ndev);
> > >
> > > Looks a bit excessive. Why do you need all these
> > > netif_tx_stop_all_queues()?
> > If we are disabling the netdev. We need to stop scheduling the queues
> > associated with that netdev for TX, so we need this code. Why do you
> think
> > it is excessive?
>
> Why do you need both netif_tx_disable() and netif_tx_stop_all_queues()?
> And why would you need to re-do netif_tx_stop_all_queues() after
> calling ops->stop()?
Oh yes! I missed this totally, In fact, netif_tx_diable is doing almost
the similar job what netif_tx_stop_all_queues() is doing with some
lock protection.
Thanks for identifying this. I will fix this in V3 patch.
Thanks
Salil
>
> > > Isn't mqprio going to override your priority2tc mapping with the
> one
> > > provided by user?
> > I guess you are referring to below code in the mqprio_init() - right?
> >
> > static int mqprio_init(struct Qdisc *sch, struct nlattr *opt)
> > {
> > [...]
> > /* Always use supplied priority mappings */
> > for (i = 0; i < TC_BITMASK + 1; i++)
> > netdev_set_prio_tc_map(dev, i, qopt->prio_tc_map[i]);
> > [...]
> > }
> >
> > In this case yes, you are right below code seems to be redundant:
> >
> > + /* Assign UP2TC map for the VSI */
> > + for (i = 0; i < HNAE3_MAX_TC; i++) {
> > + netdev_set_prio_tc_map(ndev,
> > + kinfo->tc_info[i].up,
> > + kinfo->tc_info[i].tc);
> >
> > Hope I am not missing anything here?
> You're not; That's what I meant.
Will remove this code in V3 patch.
Thanks
Salil
RE: [PATCH net-next 1/9] net: hns3: Add support of HNS3 Ethernet Driver for hip08 SoC
> > > +static void hns3_nic_net_down(struct net_device *ndev) {
> > > + struct hns3_nic_priv *priv = netdev_priv(ndev);
> > > + struct hnae3_ae_ops *ops;
> > > + int i;
> > > +
> > > + netif_tx_stop_all_queues(ndev);
> > > + netif_carrier_off(ndev);
> > > + netif_tx_disable(ndev);
> > > +
> > > + ops = priv->ae_handle->ae_algo->ops;
> > > +
> > > + if (ops->stop)
> > > + ops->stop(priv->ae_handle);
> > > +
> > > + netif_tx_stop_all_queues(ndev);
> >
> > Looks a bit excessive. Why do you need all these
> > netif_tx_stop_all_queues()?
> If we are disabling the netdev. We need to stop scheduling the queues
> associated with that netdev for TX, so we need this code. Why do you think
> it is excessive?
Why do you need both netif_tx_disable() and netif_tx_stop_all_queues()?
And why would you need to re-do netif_tx_stop_all_queues() after
calling ops->stop()?
> > Isn't mqprio going to override your priority2tc mapping with the one
> > provided by user?
> I guess you are referring to below code in the mqprio_init() - right?
>
> static int mqprio_init(struct Qdisc *sch, struct nlattr *opt)
> {
> [...]
> /* Always use supplied priority mappings */
> for (i = 0; i < TC_BITMASK + 1; i++)
> netdev_set_prio_tc_map(dev, i, qopt->prio_tc_map[i]);
> [...]
> }
>
> In this case yes, you are right below code seems to be redundant:
>
> +/* Assign UP2TC map for the VSI */
> +for (i = 0; i < HNAE3_MAX_TC; i++) {
> +netdev_set_prio_tc_map(ndev,
> + kinfo->tc_info[i].up,
> + kinfo->tc_info[i].tc);
>
> Hope I am not missing anything here?
You're not; That's what I meant.
RE: [PATCH net-next 1/9] net: hns3: Add support of HNS3 Ethernet Driver for hip08 SoC
Hi Yuval
> -Original Message-
> From: Mintz, Yuval [mailto:yuval.mi...@cavium.com]
> Sent: Saturday, June 10, 2017 1:43 PM
> To: Salil Mehta; da...@davemloft.net
> Cc: Zhuangyuzeng (Yisen); huangdaode; lipeng (Y);
> mehta.salil@gmail.com; netdev@vger.kernel.org; linux-
> ker...@vger.kernel.org; Linuxarm
> Subject: RE: [PATCH net-next 1/9] net: hns3: Add support of HNS3
> Ethernet Driver for hip08 SoC
>
> > +static void hns3_nic_net_down(struct net_device *ndev) {
> > + struct hns3_nic_priv *priv = netdev_priv(ndev);
> > + struct hnae3_ae_ops *ops;
> > + int i;
> > +
> > + netif_tx_stop_all_queues(ndev);
> > + netif_carrier_off(ndev);
> > + netif_tx_disable(ndev);
> > +
> > + ops = priv->ae_handle->ae_algo->ops;
> > +
> > + if (ops->stop)
> > + ops->stop(priv->ae_handle);
> > +
> > + netif_tx_stop_all_queues(ndev);
>
> Looks a bit excessive. Why do you need all these
> netif_tx_stop_all_queues()?
If we are disabling the netdev. We need to stop scheduling
the queues associated with that netdev for TX, so we need
this code. Why do you think it is excessive?
Thanks
Salil
>
> > +int hns3_nic_net_xmit_hw(struct net_device *ndev,
> ...
> > +out_map_frag_fail:
> > +
> > + while (ring->next_to_use != next_to_use) {
> > + if (ring->next_to_use != next_to_use)
> > + dma_unmap_page(dev,
> > + ring->desc_cb[ring->next_to_use].dma,
> > + ring->desc_cb[ring->next_to_use].length,
> > + DMA_TO_DEVICE);
> > + else
> > + dma_unmap_single(dev,
> > +ring->desc_cb[next_to_use].dma,
> > +ring->desc_cb[next_to_use].length,
> > +DMA_TO_DEVICE);
> > + }
>
> Something looks completely broken in this error-handling 'loop'.
This looks bad indeed. I will clean this logic.
Thanks
Salil
>
> > +static int hns3_setup_tc(struct net_device *ndev, u8 tc) {
> ...
> > + /* Assign UP2TC map for the VSI */
> > + for (i = 0; i < HNAE3_MAX_TC; i++) {
> > + netdev_set_prio_tc_map(ndev,
> > + kinfo->tc_info[i].up,
> > + kinfo->tc_info[i].tc);
> > + }
> ...
> > +static int hns3_nic_setup_tc(struct net_device *dev, u32 handle,
> > +u32 chain_index, __be16 protocol,
> > +struct tc_to_netdev *tc)
> > +{
> > + if (handle != TC_H_ROOT || tc->type != TC_SETUP_MQPRIO)
> > + return -EINVAL;
> > +
> > + return hns3_setup_tc(dev, tc->mqprio->num_tc); }
>
> Isn't mqprio going to override your priority2tc mapping with the one
> provided
> by user?
I guess you are referring to below code in the mqprio_init() - right?
static int mqprio_init(struct Qdisc *sch, struct nlattr *opt)
{
[...]
/* Always use supplied priority mappings */
for (i = 0; i < TC_BITMASK + 1; i++)
netdev_set_prio_tc_map(dev, i, qopt->prio_tc_map[i]);
[...]
}
In this case yes, you are right below code seems to be redundant:
+ /* Assign UP2TC map for the VSI */
+ for (i = 0; i < HNAE3_MAX_TC; i++) {
+ netdev_set_prio_tc_map(ndev,
+ kinfo->tc_info[i].up,
+ kinfo->tc_info[i].tc);
Hope I am not missing anything here?
Thanks
Salil
>
> > +
> > +static int hns3_handle_rx_bd(struct hns3_enet_ring *ring,
> > +struct sk_buff **out_skb, int *out_bnum) {
> ...
> > + /* Prefetch first cache line of first page */
> > + prefetch(va);
> > +#if L1_CACHE_BYTES < 128
> > + prefetch(va + L1_CACHE_BYTES);
> > +#endif
>
> Might be better to comment what you're actually fetching
Idea is to cache few bytes of the header of the packet. Our L1
Cache line size is 64B so need to prefetch twice to make it
128B. But in actual we can have greater size of caches with
128B Level 1 cache lines. In such a case, single fetch would
suffice to cache in the relevant part of the header.
Will provide a comment over it - no problem.
Thanks
Salil
>
>
RE: [PATCH net-next 1/9] net: hns3: Add support of HNS3 Ethernet Driver for hip08 SoC
> +static void hns3_nic_net_down(struct net_device *ndev) {
> + struct hns3_nic_priv *priv = netdev_priv(ndev);
> + struct hnae3_ae_ops *ops;
> + int i;
> +
> + netif_tx_stop_all_queues(ndev);
> + netif_carrier_off(ndev);
> + netif_tx_disable(ndev);
> +
> + ops = priv->ae_handle->ae_algo->ops;
> +
> + if (ops->stop)
> + ops->stop(priv->ae_handle);
> +
> + netif_tx_stop_all_queues(ndev);
Looks a bit excessive. Why do you need all these netif_tx_stop_all_queues()?
> +int hns3_nic_net_xmit_hw(struct net_device *ndev,
...
> +out_map_frag_fail:
> +
> + while (ring->next_to_use != next_to_use) {
> + if (ring->next_to_use != next_to_use)
> + dma_unmap_page(dev,
> +ring->desc_cb[ring->next_to_use].dma,
> +ring->desc_cb[ring->next_to_use].length,
> +DMA_TO_DEVICE);
> + else
> + dma_unmap_single(dev,
> + ring->desc_cb[next_to_use].dma,
> + ring->desc_cb[next_to_use].length,
> + DMA_TO_DEVICE);
> + }
Something looks completely broken in this error-handling 'loop'.
> +static int hns3_setup_tc(struct net_device *ndev, u8 tc) {
...
> + /* Assign UP2TC map for the VSI */
> + for (i = 0; i < HNAE3_MAX_TC; i++) {
> + netdev_set_prio_tc_map(ndev,
> +kinfo->tc_info[i].up,
> +kinfo->tc_info[i].tc);
> + }
...
> +static int hns3_nic_setup_tc(struct net_device *dev, u32 handle,
> + u32 chain_index, __be16 protocol,
> + struct tc_to_netdev *tc)
> +{
> + if (handle != TC_H_ROOT || tc->type != TC_SETUP_MQPRIO)
> + return -EINVAL;
> +
> + return hns3_setup_tc(dev, tc->mqprio->num_tc); }
Isn't mqprio going to override your priority2tc mapping with the one provided
by user?
> +
> +static int hns3_handle_rx_bd(struct hns3_enet_ring *ring,
> + struct sk_buff **out_skb, int *out_bnum) {
...
> + /* Prefetch first cache line of first page */
> + prefetch(va);
> +#if L1_CACHE_BYTES < 128
> + prefetch(va + L1_CACHE_BYTES);
> +#endif
Might be better to comment what you're actually fetching
[PATCH net-next 1/9] net: hns3: Add support of HNS3 Ethernet Driver for hip08 SoC
This patch adds the support of Hisilicon Network Subsystem 3
Ethernet driver to hip08 family of SoCs.
This driver includes basic Rx/Tx functionality. It also includes
the client registration code with the HNAE3(Hisilicon Network
Acceleration Engine 3) framework.
This work provides the initial support to the hip08 SoC and
would incrementally add features or enhancements.
Signed-off-by: Daode Huang
Signed-off-by: lipeng
Signed-off-by: Salil Mehta
Signed-off-by: Yisen Zhuang
---
.../net/ethernet/hisilicon/hns3/hns3pf/hns3_enet.c | 2851
.../net/ethernet/hisilicon/hns3/hns3pf/hns3_enet.h | 585
2 files changed, 3436 insertions(+)
create mode 100644 drivers/net/ethernet/hisilicon/hns3/hns3pf/hns3_enet.c
create mode 100644 drivers/net/ethernet/hisilicon/hns3/hns3pf/hns3_enet.h
diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hns3_enet.c
b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hns3_enet.c
new file mode 100644
index 000..d0e4f22
--- /dev/null
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hns3_enet.c
@@ -0,0 +1,2851 @@
+/*
+ * Copyright (c) 2016~2017 Hisilicon Limited.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include "hnae3.h"
+#include "hns3_enet.h"
+
+const char hns3_driver_name[] = "hns3";
+static const char hns3_driver_string[] =
+ "Hisilicon Ethernet Network Driver for Hi162x Family";
+static const char hns3_copyright[] = "Copyright (c) 2017 Huawei Corporation.";
+
+/* hns3_pci_tbl - PCI Device ID Table
+ *
+ * Last entry must be all 0s
+ *
+ * { Vendor ID, Device ID, SubVendor ID, SubDevice ID,
+ * Class, Class Mask, private data (not used) }
+ */
+static const struct pci_device_id hns3_pci_tbl[] = {
+ {PCI_VDEVICE(HUAWEI, HNAE3_DEV_ID_GE), 0},
+ {PCI_VDEVICE(HUAWEI, HNAE3_DEV_ID_25GE), 0},
+ {PCI_VDEVICE(HUAWEI, HNAE3_DEV_ID_25GE_RDMA), 0},
+ {PCI_VDEVICE(HUAWEI, HNAE3_DEV_ID_25GE_RDMA_MACSEC), 0},
+ {PCI_VDEVICE(HUAWEI, HNAE3_DEV_ID_50GE_RDMA), 0},
+ {PCI_VDEVICE(HUAWEI, HNAE3_DEV_ID_50GE_RDMA_MACSEC), 0},
+ {PCI_VDEVICE(HUAWEI, HNAE3_DEV_ID_100G_RDMA_MACSEC), 0},
+ /* required last entry */
+ {0, }
+};
+MODULE_DEVICE_TABLE(pci, hns3_pci_tbl);
+
+/* use only for netconsole to poll with the device without interrupt */
+#ifdef CONFIG_NET_POLL_CONTROLLER
+void hns3_nic_poll_controller(struct net_device *ndev)
+{
+ struct hns3_nic_priv *priv = netdev_priv(ndev);
+ struct hnae3_handle *h = priv->ae_handle;
+ unsigned long flag;
+ int i;
+
+ local_irq_save(flag);
+ for (i = 0; i < h->kinfo.num_tqp_vectors; i++)
+ napi_schedule(&h->kinfo.tqp_vectors[i].napi);
+ local_irq_restore(flag);
+}
+#endif
+
+static irqreturn_t hns3_irq_handle(int irq, void *dev)
+{
+ struct hns3_enet_tqp_vector *tqp_vector = dev;
+
+ napi_schedule(&tqp_vector->napi);
+
+ return IRQ_HANDLED;
+}
+
+static int hns3_nic_init_irq(struct hns3_nic_priv *priv)
+{
+ struct pci_dev *pdev = priv->ae_handle->pdev;
+ struct hns3_enet_tqp_vector *tqp_vectors;
+ int txrx_int_idx = 0;
+ int rx_int_idx = 0;
+ int tx_int_idx = 0;
+ int ret;
+ int i;
+
+ for (i = 0; i < priv->vector_num; i++) {
+ tqp_vectors = &priv->tqp_vector[i];
+
+ if (tqp_vectors->irq_init_flag == HNS3_VEVTOR_INITED)
+ continue;
+
+ if (tqp_vectors->tx_group.ring && tqp_vectors->rx_group.ring) {
+ snprintf(tqp_vectors->name, HNAE3_INT_NAME_LEN - 1,
+"%s-%s-%d", priv->netdev->name, "TxRx",
+txrx_int_idx++);
+ txrx_int_idx++;
+ } else if (tqp_vectors->rx_group.ring) {
+ snprintf(tqp_vectors->name, HNAE3_INT_NAME_LEN - 1,
+"%s-%s-%d", priv->netdev->name, "Rx",
+rx_int_idx++);
+ } else if (tqp_vectors->tx_group.ring) {
+ snprintf(tqp_vectors->name, HNAE3_INT_NAME_LEN - 1,
+"%s-%s-%d", priv->netdev->name, "Tx",
+tx_int_idx++);
+ } else {
+ /* Skip this unused q_vector */
+ continue;
+ }
+
+ tqp_vectors->name[HNAE3_INT_NAME_LEN - 1] = '\0';
+
+ ret = devm_request_irq(&pdev->dev, tqp_vectors->vector_irq,
+ hns3_irq_handle, 0, tqp_vectors->name,
+ tqp
