[Intel-wired-lan] [PATCH v1 1/2] fm10k: don't clear the RXQCTL register when enabling or disabling queues

[Intel-wired-lan] [PATCH v1 1/2] fm10k: don't clear the RXQCTL register when enabling or disabling queues

[Jacob Keller]

Ensure that other bits in the RXQCTL register do not get cleared. This
ensures that bits related to queue ownership are maintained.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
 drivers/net/ethernet/intel/fm10k/fm10k_pci.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/intel/fm10k/fm10k_pci.c b/drivers/net/ethernet/intel/fm10k/fm10k_pci.c
index 59dc13f3f321..eea411472f1d 100644
--- a/drivers/net/ethernet/intel/fm10k/fm10k_pci.c
+++ b/drivers/net/ethernet/intel/fm10k/fm10k_pci.c
@@ -734,15 +734,15 @@ static void fm10k_configure_rx_ring(struct fm10k_intfc *interface,
 	u64 rdba = ring->dma;
 	struct fm10k_hw *hw = &interface->hw;
 	u32 size = ring->count * sizeof(union fm10k_rx_desc);
-	u32 rxqctl = FM10K_RXQCTL_ENABLE | FM10K_RXQCTL_PF;
-	u32 rxdctl = FM10K_RXDCTL_WRITE_BACK_MIN_DELAY;
+	u32 rxqctl, rxdctl = FM10K_RXDCTL_WRITE_BACK_MIN_DELAY;
 	u32 srrctl = FM10K_SRRCTL_BUFFER_CHAINING_EN;
 	u32 rxint = FM10K_INT_MAP_DISABLE;
 	u8 rx_pause = interface->rx_pause;
 	u8 reg_idx = ring->reg_idx;
 
 	/* disable queue to avoid issues while updating state */
-	fm10k_write_reg(hw, FM10K_RXQCTL(reg_idx), 0);
+	rxqctl = fm10k_read_reg(hw, FM10K_RXQCTL(reg_idx));
+	rxqctl &= ~FM10K_RXQCTL_ENABLE;
 	fm10k_write_flush(hw);
 
 	/* possible poll here to verify ring resources have been cleaned */
@@ -797,6 +797,8 @@ static void fm10k_configure_rx_ring(struct fm10k_intfc *interface,
 	fm10k_write_reg(hw, FM10K_RXINT(reg_idx), rxint);
 
 	/* enable queue */
+	rxqctl = fm10k_read_reg(hw, FM10K_RXQCTL(reg_idx));
+	rxqctl |= FM10K_RXQCTL_ENABLE;
 	fm10k_write_reg(hw, FM10K_RXQCTL(reg_idx), rxqctl);
 
 	/* place buffers on ring for receive data */
-- 
2.9.2.701.gf965a18

[Intel-wired-lan] [PATCH v1 2/2] fm10k: don't re-map queues when a mailbox message suffices

[Jacob Keller]

When the PF assigns a new MAC Address to a VF, it uses a little register
trick to allow a later load of VF driver access to the MAC Address at
start without having to wait for a mailbox message. Unfortunately, to do
this the PF must assign ownership of the Queues and take it away from
the VF, otherwise writing these registers would be unsafe when the VF
driver is active.

This causes a potential race where a VF could try to access its queues
while they are owned by the PF. The fault detection code will prevent
this and issue a FUM fault error when the VF does this.

We can do better, by simply avoiding the register trick when it's not
necessary. We already have a mailbox message which indicates the new
MAC/VLAN pair. The PF currently attempts to send this message and
ignores failures which happen in the case where VF driver is not
loaded.

Fix this logic so that we always send the mailbox message first, then if
the mailbox errors due to no connected VF driver, we will fall back to
the register routine. This prevents using the pre-load method of writing
registers when we have an active VF driver, and thus prevents the need
to take queue ownership back. The whole logic is cleaner and avoids the
messy register interactions that could occur.

We still keep the register flow so that the PF can ensure a new driver
load will have the MAC Address at start without having to wait for the
PF to send a new mailbox message.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
 drivers/net/ethernet/intel/fm10k/fm10k_pf.c | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/intel/fm10k/fm10k_pf.c b/drivers/net/ethernet/intel/fm10k/fm10k_pf.c
index 682299dd0ce4..accfce8a6783 100644
--- a/drivers/net/ethernet/intel/fm10k/fm10k_pf.c
+++ b/drivers/net/ethernet/intel/fm10k/fm10k_pf.c
@@ -867,10 +867,6 @@ static s32 fm10k_iov_assign_default_mac_vlan_pf(struct fm10k_hw *hw,
 	vf_q_idx = fm10k_vf_queue_index(hw, vf_idx);
 	qmap_idx = qmap_stride * vf_idx;
 
-	/* MAP Tx queue back to 0 temporarily, and disable it */
-	fm10k_write_reg(hw, FM10K_TQMAP(qmap_idx), 0);
-	fm10k_write_reg(hw, FM10K_TXDCTL(vf_q_idx), 0);
-
 	/* Determine correct default VLAN ID. The FM10K_VLAN_OVERRIDE bit is
 	 * used here to indicate to the VF that it will not have privilege to
 	 * write VLAN_TABLE. All policy is enforced on the PF but this allows
@@ -886,9 +882,22 @@ static s32 fm10k_iov_assign_default_mac_vlan_pf(struct fm10k_hw *hw,
 	fm10k_tlv_attr_put_mac_vlan(msg, FM10K_MAC_VLAN_MSG_DEFAULT_MAC,
 				    vf_info->mac, vf_vid);
 
-	/* load onto outgoing mailbox, ignore any errors on enqueue */
-	if (vf_info->mbx.ops.enqueue_tx)
-		vf_info->mbx.ops.enqueue_tx(hw, &vf_info->mbx, msg);
+	/* try loading a message onto outgoing mailbox first */
+	if (vf_info->mbx.ops.enqueue_tx) {
+		err = vf_info->mbx.ops.enqueue_tx(hw, &vf_info->mbx, msg);
+		if (err != FM10K_MBX_ERR_NO_MBX)
+			return err;
+	}
+
+	/* If we aren't connected to a mailbox, this is most likely because
+	 * the VF driver is not running. It should thus be safe to re-map
+	 * queues and use the registers to pass the address and vlan so that
+	 * the VF driver gets correct information during its initialization.
+	 */
+
+	/* MAP Tx queue back to 0 temporarily, and disable it */
+	fm10k_write_reg(hw, FM10K_TQMAP(qmap_idx), 0);
+	fm10k_write_reg(hw, FM10K_TXDCTL(vf_q_idx), 0);
 
 	/* verify ring has disabled before modifying base address registers */
 	txdctl = fm10k_read_reg(hw, FM10K_TXDCTL(vf_q_idx));
-- 
2.9.2.701.gf965a18

[Intel-wired-lan] [PATCH v1 2/2] fm10k: don't re-map queues when a mailbox message suffices

[Keller, Jacob E]

On Wed, 2016-08-03 at 15:05 -0700, Jacob Keller wrote:
> When the PF assigns a new MAC Address to a VF, it uses a little
> register
> trick to allow a later load of VF driver access to the MAC Address at
> start without having to wait for a mailbox message. Unfortunately, to
> do
> this the PF must assign ownership of the Queues and take it away from
> the VF, otherwise writing these registers would be unsafe when the VF
> driver is active.
> 
> This causes a potential race where a VF could try to access its
> queues
> while they are owned by the PF. The fault detection code will prevent
> this and issue a FUM fault error when the VF does this.
> 
> We can do better, by simply avoiding the register trick when it's not
> necessary. We already have a mailbox message which indicates the new
> MAC/VLAN pair. The PF currently attempts to send this message and
> ignores failures which happen in the case where VF driver is not
> loaded.
> 
> Fix this logic so that we always send the mailbox message first, then
> if
> the mailbox errors due to no connected VF driver, we will fall back
> to
> the register routine. This prevents using the pre-load method of
> writing
> registers when we have an active VF driver, and thus prevents the
> need
> to take queue ownership back. The whole logic is cleaner and avoids
> the
> messy register interactions that could occur.
> 
> We still keep the register flow so that the PF can ensure a new
> driver
> load will have the MAC Address at start without having to wait for
> the
> PF to send a new mailbox message.
> 
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>

This patch should not be sent, it has a bug we discovered in testing. I
have a re-worked patch that I will submit shortly.

Thanks,
Jake

[Intel-wired-lan] [PATCH v1 1/2] fm10k: don't clear the RXQCTL register when enabling or disabling queues

[Singh, Krishneil K]

-----Original Message-----
From: Intel-wired-lan [mailto:intel-wired-lan-bounces at lists.osuosl.org] On Behalf Of Jacob Keller
Sent: Wednesday, August 3, 2016 3:05 PM
To: Intel Wired LAN <intel-wired-lan@lists.osuosl.org>
Subject: [Intel-wired-lan] [PATCH v1 1/2] fm10k: don't clear the RXQCTL register when enabling or disabling queues

Ensure that other bits in the RXQCTL register do not get cleared. This ensures that bits related to queue ownership are maintained.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---

Tested-by: Krishneil Singh <Krishneil.k.singh@intel.com>