[dpdk-dev] ixgbe_vf: Fix getting link state

Message ID 1418822554-1493-1-git-send-email-balazs.nemeth@intel.com (mailing list archive)
State Superseded, archived
Headers

Commit Message

Balazs Nemeth Dec. 17, 2014, 1:22 p.m. UTC
  This patch fixes checking the link state of a virtual function. If the
state has already been checked, it does not need to be checked
again. Previously, get_link_status in the ixgbe_hw struct was
used to track if the information had already been updated, but this
field was always set to false.

Signed-off-by: Balazs Nemeth <balazs.nemeth@intel.com>
---
 lib/librte_pmd_ixgbe/ixgbe/ixgbe_type.h |  2 +-
 lib/librte_pmd_ixgbe/ixgbe/ixgbe_vf.c   | 14 ++++++++------
 lib/librte_pmd_ixgbe/ixgbe_ethdev.c     |  8 +++++++-
 3 files changed, 16 insertions(+), 8 deletions(-)
  

Comments

Thomas Monjalon Dec. 17, 2014, 1:24 p.m. UTC | #1
2014-12-17 13:22, Balazs Nemeth:
> This patch fixes checking the link state of a virtual function. If the
> state has already been checked, it does not need to be checked
> again. Previously, get_link_status in the ixgbe_hw struct was
> used to track if the information had already been updated, but this
> field was always set to false.
> 
> Signed-off-by: Balazs Nemeth <balazs.nemeth@intel.com>

This is the third patch about link status fix in ixgbevf.
Please comment the other ones in the respective mailing threads:
	http://dpdk.org/dev/patchwork/patch/1079
	http://dpdk.org/dev/patchwork/patch/1224
Are they superseded by yours?
  
Choonho Son Dec. 18, 2014, 4:20 a.m. UTC | #2
DPDK pmd code should have the consistency with original network device
driver code.

Linux kernel driver                    ---> DPDK pmd driver
-----------------------------------------------------------------------
ixgbevf_check_mac_link_vf()    ixgbe_check_mac_link_vf()
 at ixgbevf/vf.c                            at
librte_pmd_ixgbe/ixgbe/ixgbe_vf.c

ixgbevf_get_settings()              ixgbe_dev_link_update()
 at ixgbevf/ethtool.c                    at librte_pmd_ixgbe/ixgbe_ethdev.c


In a original device driver, detection link status called by
ixgbevf_get_settings()

        hw->mac.get_link_status = 1;
        hw->mac.ops.check_link(hw, &link_speed, &link_up, false);

Changing ixgbevf_check_mac_link_vf() will break consistency with original
code.



@ {Linux kernel}/drivers/net/ethernet/intel/ixgbevf/vf.c
static s32 ixgbevf_check_mac_link_vf(struct ixgbe_hw *hw,
                                     ixgbe_link_speed *speed,
                                     bool *link_up,
                                     bool autoneg_wait_to_complete)
{
        struct ixgbe_mbx_info *mbx = &hw->mbx;
        struct ixgbe_mac_info *mac = &hw->mac;
        s32 ret_val = 0;
        u32 links_reg;
        u32 in_msg = 0;

        /* If we were hit with a reset drop the link */
        if (!mbx->ops.check_for_rst(hw) || !mbx->timeout)
                mac->get_link_status = true;

        if (!mac->get_link_status)
                goto out;



@ {DPDK}/lib/librte_pmd_ixgbe/ixgbe/ixgbe_vf.c
s32 ixgbe_check_mac_link_vf(struct ixgbe_hw *hw, ixgbe_link_speed *speed,
                bool *link_up, bool autoneg_wait_to_complete)
{
    struct ixgbe_mbx_info *mbx = &hw->mbx;
    struct ixgbe_mac_info *mac = &hw->mac;
    s32 ret_val = IXGBE_SUCCESS;
    u32 links_reg;
    u32 in_msg = 0;
    UNREFERENCED_1PARAMETER(autoneg_wait_to_complete);

    /* If we were hit with a reset drop the link */
    if (!mbx->ops.check_for_rst(hw, 0) || !mbx->timeout)
        mac->get_link_status = true;

    if (!mac->get_link_status)
        goto out;



@ {Linux kernel}/drivers/net/ethernet/intel/ixgbevf/ethtool.c
static int ixgbevf_get_settings(struct net_device *netdev,
                                struct ethtool_cmd *ecmd)
{
        struct ixgbevf_adapter *adapter = netdev_priv(netdev);
        struct ixgbe_hw *hw = &adapter->hw;
        u32 link_speed = 0;
        bool link_up;

        ecmd->supported = SUPPORTED_10000baseT_Full;
        ecmd->autoneg = AUTONEG_DISABLE;
        ecmd->transceiver = XCVR_DUMMY1;
        ecmd->port = -1;

        hw->mac.get_link_status = 1;
        hw->mac.ops.check_link(hw, &link_speed, &link_up, false);

        if (link_up) {
                __u32 speed = SPEED_10000;
                switch (link_speed) {



@ {DPDK}/lib/librte_pmd_ixgbe/ixgbe_ethdev.c
/* return 0 means link status changed, -1 means not changed */
static int
ixgbe_dev_link_update(struct rte_eth_dev *dev, int wait_to_complete)
{
    struct ixgbe_hw *hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
    struct rte_eth_link link, old;
    ixgbe_link_speed link_speed;
    int link_up;
    int diag;

    link.link_status = 0;
    link.link_speed = 0;
    link.link_duplex = 0;
    memset(&old, 0, sizeof(old));
    rte_ixgbe_dev_atomic_read_link_status(dev, &old);

    /* check if it needs to wait to complete, if lsc interrupt is enabled */
    if (wait_to_complete == 0 || dev->data->dev_conf.intr_conf.lsc != 0)
        diag = ixgbe_check_link(hw, &link_speed, &link_up, 0);
    else
        diag = ixgbe_check_link(hw, &link_speed, &link_up, 1);
    if (diag != 0) {
        link.link_speed = ETH_LINK_SPEED_100;
        link.link_duplex = ETH_LINK_HALF_DUPLEX;





2014-12-17 22:24 GMT+09:00 Thomas Monjalon <thomas.monjalon@6wind.com>:
>
> 2014-12-17 13:22, Balazs Nemeth:
> > This patch fixes checking the link state of a virtual function. If the
> > state has already been checked, it does not need to be checked
> > again. Previously, get_link_status in the ixgbe_hw struct was
> > used to track if the information had already been updated, but this
> > field was always set to false.
> >
> > Signed-off-by: Balazs Nemeth <balazs.nemeth@intel.com>
>
> This is the third patch about link status fix in ixgbevf.
> Please comment the other ones in the respective mailing threads:
>         http://dpdk.org/dev/patchwork/patch/1079
>         http://dpdk.org/dev/patchwork/patch/1224
> Are they superseded by yours?
>
> --
> Thomas
>
  

Patch

diff --git a/lib/librte_pmd_ixgbe/ixgbe/ixgbe_type.h b/lib/librte_pmd_ixgbe/ixgbe/ixgbe_type.h
index c67d462..07bba75 100644
--- a/lib/librte_pmd_ixgbe/ixgbe/ixgbe_type.h
+++ b/lib/librte_pmd_ixgbe/ixgbe/ixgbe_type.h
@@ -3573,7 +3573,7 @@  struct ixgbe_mac_info {
 	u32 max_rx_queues;
 	u32 orig_autoc;
 	u8  san_mac_rar_index;
-	bool get_link_status;
+	bool have_link_status;
 	u32 orig_autoc2;
 	u16 max_msix_vectors;
 	bool arc_subsystem_valid;
diff --git a/lib/librte_pmd_ixgbe/ixgbe/ixgbe_vf.c b/lib/librte_pmd_ixgbe/ixgbe/ixgbe_vf.c
index e6b6c51..6cc7d7f 100644
--- a/lib/librte_pmd_ixgbe/ixgbe/ixgbe_vf.c
+++ b/lib/librte_pmd_ixgbe/ixgbe/ixgbe_vf.c
@@ -550,10 +550,13 @@  s32 ixgbe_check_mac_link_vf(struct ixgbe_hw *hw, ixgbe_link_speed *speed,
 	UNREFERENCED_1PARAMETER(autoneg_wait_to_complete);
 
 	/* If we were hit with a reset drop the link */
-	if (!mbx->ops.check_for_rst(hw, 0) || !mbx->timeout)
-		mac->get_link_status = true;
+	if (!mbx->ops.check_for_rst(hw, 0) || !mbx->timeout) {
+		mac->have_link_status = false;
+		ret_val = -1;
+		goto out;
+	}
 
-	if (!mac->get_link_status)
+	if (mac->have_link_status)
 		goto out;
 
 	/* if link status is down no point in checking to see if pf is up */
@@ -610,10 +613,10 @@  s32 ixgbe_check_mac_link_vf(struct ixgbe_hw *hw, ixgbe_link_speed *speed,
 	/* if we passed all the tests above then the link is up and we no
 	 * longer need to check for link
 	 */
-	mac->get_link_status = false;
+	mac->have_link_status = true;
 
 out:
-	*link_up = !mac->get_link_status;
+	*link_up = mac->have_link_status;
 	return ret_val;
 }
 
@@ -722,4 +725,3 @@  int ixgbevf_get_queues(struct ixgbe_hw *hw, unsigned int *num_tcs,
 
 	return err;
 }
-
diff --git a/lib/librte_pmd_ixgbe/ixgbe_ethdev.c b/lib/librte_pmd_ixgbe/ixgbe_ethdev.c
index 9401916..8e622ee 100644
--- a/lib/librte_pmd_ixgbe/ixgbe_ethdev.c
+++ b/lib/librte_pmd_ixgbe/ixgbe_ethdev.c
@@ -2064,7 +2064,7 @@  ixgbe_dev_link_update(struct rte_eth_dev *dev, int wait_to_complete)
 {
 	struct ixgbe_hw *hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
 	struct rte_eth_link link, old;
-	ixgbe_link_speed link_speed;
+	ixgbe_link_speed link_speed = IXGBE_LINK_SPEED_UNKNOWN;
 	int link_up;
 	int diag;
 
@@ -2088,6 +2088,12 @@  ixgbe_dev_link_update(struct rte_eth_dev *dev, int wait_to_complete)
 		return 0;
 	}
 
+	if (link_speed == IXGBE_LINK_SPEED_UNKNOWN &&
+	    hw->mac.have_link_status) {
+		memcpy(&link, &old, sizeof(link));
+		return 0;
+	}
+
 	if (link_up == 0) {
 		rte_ixgbe_dev_atomic_write_link_status(dev, &link);
 		if (link.link_status == old.link_status)