Re: [PATCH v2] net: thunderx: Correctly distinguish between VF and LMAC count

[David Miller]

From: Pavel Fedin 
Date: Tue, 08 Dec 2015 10:37:44 +0300

> Commit bc69fdfc6c13
> ("net: thunderx: Enable BGX LMAC's RX/TX only after VF is up")
> introduces lmac_cnt member and starts verifying VF number against it.
> This is plain wrong, and works only because currently we have hardcoded
> 1:1 mapping between VFs and LMACs, and in this case num_vf_en and
> lmac_cnt are always equal. However in future this may change, and the
> code will badly misbehave. The worst consequence of this is failure to
> deliver link status messages, causing VFs to go defunct because since
> commit 0b72a9a1060e ("net: thunderx: Switchon carrier only upon
> interface link up") VF will not fully bring itself up without it.
> 
> This patch fixes the potential problem by doing VF number checks against
> the num_vf_en. Since lmac_cnt is not used anywhere else, it is removed.
> 
> Additionally some duplicated code is factored out into nic_enable_vf()
> 
> Signed-off-by: Pavel Fedin 

Applied.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2] net: thunderx: Correctly distinguish between VF and LMAC count

[Pavel Fedin]

Commit bc69fdfc6c13
("net: thunderx: Enable BGX LMAC's RX/TX only after VF is up")
introduces lmac_cnt member and starts verifying VF number against it.
This is plain wrong, and works only because currently we have hardcoded
1:1 mapping between VFs and LMACs, and in this case num_vf_en and
lmac_cnt are always equal. However in future this may change, and the
code will badly misbehave. The worst consequence of this is failure to
deliver link status messages, causing VFs to go defunct because since
commit 0b72a9a1060e ("net: thunderx: Switchon carrier only upon
interface link up") VF will not fully bring itself up without it.

This patch fixes the potential problem by doing VF number checks against
the num_vf_en. Since lmac_cnt is not used anywhere else, it is removed.

Additionally some duplicated code is factored out into nic_enable_vf()

Signed-off-by: Pavel Fedin 
---
v1 => v2:
- Updated subject and commit message for better understanding
---
 drivers/net/ethernet/cavium/thunder/nic_main.c | 39 --
 1 file changed, 18 insertions(+), 21 deletions(-)

diff --git a/drivers/net/ethernet/cavium/thunder/nic_main.c 
b/drivers/net/ethernet/cavium/thunder/nic_main.c
index 4b7fd63..5f24d11 100644
--- a/drivers/net/ethernet/cavium/thunder/nic_main.c
+++ b/drivers/net/ethernet/cavium/thunder/nic_main.c
@@ -37,7 +37,6 @@ struct nicpf {
 #defineNIC_GET_BGX_FROM_VF_LMAC_MAP(map)   ((map >> 4) & 0xF)
 #defineNIC_GET_LMAC_FROM_VF_LMAC_MAP(map)  (map & 0xF)
u8  vf_lmac_map[MAX_LMAC];
-   u8  lmac_cnt;
struct delayed_work dwork;
struct workqueue_struct *check_link;
u8  link[MAX_LMAC];
@@ -280,7 +279,6 @@ static void nic_set_lmac_vf_mapping(struct nicpf *nic)
u64 lmac_credit;
 
nic->num_vf_en = 0;
-   nic->lmac_cnt = 0;
 
for (bgx = 0; bgx < NIC_MAX_BGX; bgx++) {
if (!(bgx_map & (1 << bgx)))
@@ -290,7 +288,6 @@ static void nic_set_lmac_vf_mapping(struct nicpf *nic)
nic->vf_lmac_map[next_bgx_lmac++] =
NIC_SET_VF_LMAC_MAP(bgx, lmac);
nic->num_vf_en += lmac_cnt;
-   nic->lmac_cnt += lmac_cnt;
 
/* Program LMAC credits */
lmac_credit = (1ull << 1); /* channel credit enable */
@@ -618,6 +615,21 @@ static int nic_config_loopback(struct nicpf *nic, struct 
set_loopback *lbk)
return 0;
 }
 
+static void nic_enable_vf(struct nicpf *nic, int vf, bool enable)
+{
+   int bgx, lmac;
+
+   nic->vf_enabled[vf] = enable;
+
+   if (vf >= nic->num_vf_en)
+   return;
+
+   bgx = NIC_GET_BGX_FROM_VF_LMAC_MAP(nic->vf_lmac_map[vf]);
+   lmac = NIC_GET_LMAC_FROM_VF_LMAC_MAP(nic->vf_lmac_map[vf]);
+
+   bgx_lmac_rx_tx_enable(nic->node, bgx, lmac, enable);
+}
+
 /* Interrupt handler to handle mailbox messages from VFs */
 static void nic_handle_mbx_intr(struct nicpf *nic, int vf)
 {
@@ -717,29 +729,14 @@ static void nic_handle_mbx_intr(struct nicpf *nic, int vf)
break;
case NIC_MBOX_MSG_CFG_DONE:
/* Last message of VF config msg sequence */
-   nic->vf_enabled[vf] = true;
-   if (vf >= nic->lmac_cnt)
-   goto unlock;
-
-   bgx = NIC_GET_BGX_FROM_VF_LMAC_MAP(nic->vf_lmac_map[vf]);
-   lmac = NIC_GET_LMAC_FROM_VF_LMAC_MAP(nic->vf_lmac_map[vf]);
-
-   bgx_lmac_rx_tx_enable(nic->node, bgx, lmac, true);
+   nic_enable_vf(nic, vf, true);
goto unlock;
case NIC_MBOX_MSG_SHUTDOWN:
/* First msg in VF teardown sequence */
-   nic->vf_enabled[vf] = false;
if (vf >= nic->num_vf_en)
nic->sqs_used[vf - nic->num_vf_en] = false;
nic->pqs_vf[vf] = 0;
-
-   if (vf >= nic->lmac_cnt)
-   break;
-
-   bgx = NIC_GET_BGX_FROM_VF_LMAC_MAP(nic->vf_lmac_map[vf]);
-   lmac = NIC_GET_LMAC_FROM_VF_LMAC_MAP(nic->vf_lmac_map[vf]);
-
-   bgx_lmac_rx_tx_enable(nic->node, bgx, lmac, false);
+   nic_enable_vf(nic, vf, false);
break;
case NIC_MBOX_MSG_ALLOC_SQS:
nic_alloc_sqs(nic, _alloc);
@@ -958,7 +955,7 @@ static void nic_poll_for_link(struct work_struct *work)
 
mbx.link_status.msg = NIC_MBOX_MSG_BGX_LINK_CHANGE;
 
-   for (vf = 0; vf < nic->lmac_cnt; vf++) {
+   for (vf = 0; vf < nic->num_vf_en; vf++) {
/* Poll only if VF is UP */
if (!nic->vf_enabled[vf])
continue;
-- 
2.4.4

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To