[Intel-wired-lan] [PATCH 0/3] igb: VF mailbox timeouts with multiple VF messages

[Intel-wired-lan] [PATCH 0/3] igb: VF mailbox timeouts with multiple VF messages

[Greg Edwards]

We've encountered VF mailbox timeout issues with I350 VFs assigned to VMs, when
NetworkManager or lldpad (LLDP) are assigning multicast addresses in rapid
succession.  A VF message may encounter a mailbox timeout waiting for the ACK
from the PF, and the igbvf_watchdog_task will detect the condition and reset
the VF.  This pattern can continue for some time or indefinitely.

The igb (PF) driver drops the mailbox lock after reading the VF message from
the mailbox and ACK'ing it, and nothing prevents another VF message from being
posted to the mailbox before the PF wants to post its reply.  However, any VF
message posted to the mailbox during this time is silently dropped in
igb_write_mbx_pf() to make way for the PF reply.  This results in a VF mailbox
timeout for the command in the VM if the VF is waiting for an ACK.

Looking at the "Intel Ethernet Controller I350 Datasheet", Revision 2.4,
January 2016, Table 7-66 "PF to VF Messaging Flow", it seems the intent is to
handle this condition.  Before the PF writes a message to the mailbox, it
acquires the lock, then (step 4):

    Read MBVFICR register and verify that
    VFREQ bit of VF[n] is 0, otherwise clear
    PFU bit in PFMailbox[n] and respond to the
    VF message.

While we could implement the above, it seems cleaner with the current code to
not drop the lock in igb_rcv_msg_from_vf() between the read of the VF message
from the mailbox and the write of the PF reply at the end.  This closes the
PF/VF race.

The first patch cleans up some checkpatch.pl warnings in function declarations
touched by subsequent patches.

The second patch adds a new 'unlock' mailbox op, used by the third patch to
unlock the mailbox in some error paths in igb_rcv_msg_from_vf().

The third patch modifies the 'read' mailbox op to take an additional 'unlock'
argument that can be used to leave the mailbox locked after reading the VF
message.  The only read op call site that leaves the mailbox locked is the
mailbox read in igb_rcv_msg_from_vf().

The PF/VF mailbox lock retry logic added with 9ce0e8d72678 ("igb/igbvf:
don't give up") helps us out here as well, as the VF will continue to try to
grab the mailbox lock for up to 1 sec, if it finds it locked.

In my testing, these changes have resolved the VF mailbox timeout issues we
we were encountering.


Greg Edwards (3):
  igb: add argument names to mailbox op function declarations
  igb: expose mailbox unlock method
  igb: do not drop PF mailbox lock after read of VF message

 drivers/net/ethernet/intel/igb/e1000_hw.h  | 17 +++++----
 drivers/net/ethernet/intel/igb/e1000_mbx.c | 57 ++++++++++++++++++++++++++----
 drivers/net/ethernet/intel/igb/e1000_mbx.h | 14 ++++----
 drivers/net/ethernet/intel/igb/igb_main.c  | 14 +++++---
 4 files changed, 79 insertions(+), 23 deletions(-)

-- 
2.9.4

[Intel-wired-lan] [PATCH 1/3] igb: add argument names to mailbox op function declarations

[Greg Edwards]

Signed-off-by: Greg Edwards <gedwards@ddn.com>
---
 drivers/net/ethernet/intel/igb/e1000_hw.h  | 15 ++++++++-------
 drivers/net/ethernet/intel/igb/e1000_mbx.h | 12 ++++++------
 2 files changed, 14 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ethernet/intel/igb/e1000_hw.h b/drivers/net/ethernet/intel/igb/e1000_hw.h
index 2fb2213cd562..fd7865a8d2e3 100644
--- a/drivers/net/ethernet/intel/igb/e1000_hw.h
+++ b/drivers/net/ethernet/intel/igb/e1000_hw.h
@@ -491,13 +491,14 @@ struct e1000_fc_info {
 
 struct e1000_mbx_operations {
 	s32 (*init_params)(struct e1000_hw *hw);
-	s32 (*read)(struct e1000_hw *, u32 *, u16,  u16);
-	s32 (*write)(struct e1000_hw *, u32 *, u16, u16);
-	s32 (*read_posted)(struct e1000_hw *, u32 *, u16,  u16);
-	s32 (*write_posted)(struct e1000_hw *, u32 *, u16, u16);
-	s32 (*check_for_msg)(struct e1000_hw *, u16);
-	s32 (*check_for_ack)(struct e1000_hw *, u16);
-	s32 (*check_for_rst)(struct e1000_hw *, u16);
+	s32 (*read)(struct e1000_hw *hw, u32 *msg, u16 size, u16 mbx_id);
+	s32 (*write)(struct e1000_hw *hw, u32 *msg, u16 size, u16 mbx_id);
+	s32 (*read_posted)(struct e1000_hw *hw, u32 *msg, u16 size, u16 mbx_id);
+	s32 (*write_posted)(struct e1000_hw *hw, u32 *msg, u16 size,
+			    u16 mbx_id);
+	s32 (*check_for_msg)(struct e1000_hw *hw, u16 mbx_id);
+	s32 (*check_for_ack)(struct e1000_hw *hw, u16 mbx_id);
+	s32 (*check_for_rst)(struct e1000_hw *hw, u16 mbx_id);
 };
 
 struct e1000_mbx_stats {
diff --git a/drivers/net/ethernet/intel/igb/e1000_mbx.h b/drivers/net/ethernet/intel/igb/e1000_mbx.h
index 3e7fed73df15..73d90aeb48b2 100644
--- a/drivers/net/ethernet/intel/igb/e1000_mbx.h
+++ b/drivers/net/ethernet/intel/igb/e1000_mbx.h
@@ -67,11 +67,11 @@
 
 #define E1000_PF_CONTROL_MSG	0x0100 /* PF control message */
 
-s32 igb_read_mbx(struct e1000_hw *, u32 *, u16, u16);
-s32 igb_write_mbx(struct e1000_hw *, u32 *, u16, u16);
-s32 igb_check_for_msg(struct e1000_hw *, u16);
-s32 igb_check_for_ack(struct e1000_hw *, u16);
-s32 igb_check_for_rst(struct e1000_hw *, u16);
-s32 igb_init_mbx_params_pf(struct e1000_hw *);
+s32 igb_read_mbx(struct e1000_hw *hw, u32 *msg, u16 size, u16 mbx_id);
+s32 igb_write_mbx(struct e1000_hw *hw, u32 *msg, u16 size, u16 mbx_id);
+s32 igb_check_for_msg(struct e1000_hw *hw, u16 mbx_id);
+s32 igb_check_for_ack(struct e1000_hw *hw, u16 mbx_id);
+s32 igb_check_for_rst(struct e1000_hw *hw, u16 mbx_id);
+s32 igb_init_mbx_params_pf(struct e1000_hw *hw);
 
 #endif /* _E1000_MBX_H_ */
-- 
2.9.4

[Intel-wired-lan] [PATCH 2/3] igb: expose mailbox unlock method

[Greg Edwards]

Add a mailbox unlock method to e1000_mbx_operations, which will be used
to unlock the PF/VF mailbox by the PF.

Signed-off-by: Greg Edwards <gedwards@ddn.com>
---
 drivers/net/ethernet/intel/igb/e1000_hw.h  |  1 +
 drivers/net/ethernet/intel/igb/e1000_mbx.c | 39 ++++++++++++++++++++++++++++++
 drivers/net/ethernet/intel/igb/e1000_mbx.h |  1 +
 3 files changed, 41 insertions(+)

diff --git a/drivers/net/ethernet/intel/igb/e1000_hw.h b/drivers/net/ethernet/intel/igb/e1000_hw.h
index fd7865a8d2e3..6076f258a0a5 100644
--- a/drivers/net/ethernet/intel/igb/e1000_hw.h
+++ b/drivers/net/ethernet/intel/igb/e1000_hw.h
@@ -499,6 +499,7 @@ struct e1000_mbx_operations {
 	s32 (*check_for_msg)(struct e1000_hw *hw, u16 mbx_id);
 	s32 (*check_for_ack)(struct e1000_hw *hw, u16 mbx_id);
 	s32 (*check_for_rst)(struct e1000_hw *hw, u16 mbx_id);
+	s32 (*unlock)(struct e1000_hw *hw, u16 mbx_id);
 };
 
 struct e1000_mbx_stats {
diff --git a/drivers/net/ethernet/intel/igb/e1000_mbx.c b/drivers/net/ethernet/intel/igb/e1000_mbx.c
index 00e263f0c030..6aa44723507b 100644
--- a/drivers/net/ethernet/intel/igb/e1000_mbx.c
+++ b/drivers/net/ethernet/intel/igb/e1000_mbx.c
@@ -125,6 +125,24 @@ s32 igb_check_for_rst(struct e1000_hw *hw, u16 mbx_id)
 }
 
 /**
+ *  igb_unlock_mbx - unlock the mailbox
+ *  @hw: pointer to the HW structure
+ *  @mbx_id: id of mailbox to check
+ *
+ *  returns SUCCESS if the mailbox was unlocked or else ERR_MBX
+ **/
+s32 igb_unlock_mbx(struct e1000_hw *hw, u16 mbx_id)
+{
+	struct e1000_mbx_info *mbx = &hw->mbx;
+	s32 ret_val = -E1000_ERR_MBX;
+
+	if (mbx->ops.unlock)
+		ret_val = mbx->ops.unlock(hw, mbx_id);
+
+	return ret_val;
+}
+
+/**
  *  igb_poll_for_msg - Wait for message notification
  *  @hw: pointer to the HW structure
  *  @mbx_id: id of mailbox to write
@@ -341,6 +359,26 @@ static s32 igb_obtain_mbx_lock_pf(struct e1000_hw *hw, u16 vf_number)
 }
 
 /**
+ *  igb_release_mbx_lock_pf - release mailbox lock
+ *  @hw: pointer to the HW structure
+ *  @vf_number: the VF index
+ *
+ *  return SUCCESS if we released the mailbox lock
+ **/
+static s32 igb_release_mbx_lock_pf(struct e1000_hw *hw, u16 vf_number)
+{
+	u32 p2v_mailbox;
+
+	/* drop PF lock of mailbox, if set */
+	p2v_mailbox = rd32(E1000_P2VMAILBOX(vf_number));
+	if (p2v_mailbox & E1000_P2VMAILBOX_PFU)
+		wr32(E1000_P2VMAILBOX(vf_number),
+		     p2v_mailbox & ~E1000_P2VMAILBOX_PFU);
+
+	return 0;
+}
+
+/**
  *  igb_write_mbx_pf - Places a message in the mailbox
  *  @hw: pointer to the HW structure
  *  @msg: The message buffer
@@ -437,6 +475,7 @@ s32 igb_init_mbx_params_pf(struct e1000_hw *hw)
 	mbx->ops.check_for_msg = igb_check_for_msg_pf;
 	mbx->ops.check_for_ack = igb_check_for_ack_pf;
 	mbx->ops.check_for_rst = igb_check_for_rst_pf;
+	mbx->ops.unlock = igb_release_mbx_lock_pf;
 
 	mbx->stats.msgs_tx = 0;
 	mbx->stats.msgs_rx = 0;
diff --git a/drivers/net/ethernet/intel/igb/e1000_mbx.h b/drivers/net/ethernet/intel/igb/e1000_mbx.h
index 73d90aeb48b2..a98c5dc60afd 100644
--- a/drivers/net/ethernet/intel/igb/e1000_mbx.h
+++ b/drivers/net/ethernet/intel/igb/e1000_mbx.h
@@ -72,6 +72,7 @@ s32 igb_write_mbx(struct e1000_hw *hw, u32 *msg, u16 size, u16 mbx_id);
 s32 igb_check_for_msg(struct e1000_hw *hw, u16 mbx_id);
 s32 igb_check_for_ack(struct e1000_hw *hw, u16 mbx_id);
 s32 igb_check_for_rst(struct e1000_hw *hw, u16 mbx_id);
+s32 igb_unlock_mbx(struct e1000_hw *hw, u16 mbx_id);
 s32 igb_init_mbx_params_pf(struct e1000_hw *hw);
 
 #endif /* _E1000_MBX_H_ */
-- 
2.9.4

[Intel-wired-lan] [PATCH 3/3] igb: do not drop PF mailbox lock after read of VF message

[Greg Edwards]

When the PF receives a mailbox message from the VF, it grabs the mailbox
lock, reads the VF message from the mailbox, ACKs the message and drops
the lock.

While the PF is performing the action for the VF message, nothing
prevents another VF message from being posted to the mailbox.  The
current code handles this condition by just dropping any new VF messages
without processing them.  This results in a mailbox timeout in the VM
for posted messages waiting for an ACK, and the VF is reset by the
igbvf_watchdog_task in the VM.

Given the right sequence of VF messages and mailbox timeouts, this
condition can go on ad infinitum.

Modify the PF mailbox read method to take an 'unlock' argument that
optionally leaves the mailbox locked by the PF after reading the VF
message.  This ensures another VF message is not posted to the mailbox
until after the PF has completed processing the VF message and written
its reply.

Signed-off-by: Greg Edwards <gedwards@ddn.com>
---
 drivers/net/ethernet/intel/igb/e1000_hw.h  |  3 ++-
 drivers/net/ethernet/intel/igb/e1000_mbx.c | 18 ++++++++++++------
 drivers/net/ethernet/intel/igb/e1000_mbx.h |  3 ++-
 drivers/net/ethernet/intel/igb/igb_main.c  | 14 ++++++++++----
 4 files changed, 26 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/intel/igb/e1000_hw.h b/drivers/net/ethernet/intel/igb/e1000_hw.h
index 6076f258a0a5..6ea9f702ba0f 100644
--- a/drivers/net/ethernet/intel/igb/e1000_hw.h
+++ b/drivers/net/ethernet/intel/igb/e1000_hw.h
@@ -491,7 +491,8 @@ struct e1000_fc_info {
 
 struct e1000_mbx_operations {
 	s32 (*init_params)(struct e1000_hw *hw);
-	s32 (*read)(struct e1000_hw *hw, u32 *msg, u16 size, u16 mbx_id);
+	s32 (*read)(struct e1000_hw *hw, u32 *msg, u16 size, u16 mbx_id,
+		    bool unlock);
 	s32 (*write)(struct e1000_hw *hw, u32 *msg, u16 size, u16 mbx_id);
 	s32 (*read_posted)(struct e1000_hw *hw, u32 *msg, u16 size, u16 mbx_id);
 	s32 (*write_posted)(struct e1000_hw *hw, u32 *msg, u16 size,
diff --git a/drivers/net/ethernet/intel/igb/e1000_mbx.c b/drivers/net/ethernet/intel/igb/e1000_mbx.c
index 6aa44723507b..bffd58f7b2a1 100644
--- a/drivers/net/ethernet/intel/igb/e1000_mbx.c
+++ b/drivers/net/ethernet/intel/igb/e1000_mbx.c
@@ -32,7 +32,8 @@
  *
  *  returns SUCCESS if it successfully read message from buffer
  **/
-s32 igb_read_mbx(struct e1000_hw *hw, u32 *msg, u16 size, u16 mbx_id)
+s32 igb_read_mbx(struct e1000_hw *hw, u32 *msg, u16 size, u16 mbx_id,
+		 bool unlock)
 {
 	struct e1000_mbx_info *mbx = &hw->mbx;
 	s32 ret_val = -E1000_ERR_MBX;
@@ -42,7 +43,7 @@ s32 igb_read_mbx(struct e1000_hw *hw, u32 *msg, u16 size, u16 mbx_id)
 		size = mbx->size;
 
 	if (mbx->ops.read)
-		ret_val = mbx->ops.read(hw, msg, size, mbx_id);
+		ret_val = mbx->ops.read(hw, msg, size, mbx_id, unlock);
 
 	return ret_val;
 }
@@ -222,7 +223,7 @@ static s32 igb_read_posted_mbx(struct e1000_hw *hw, u32 *msg, u16 size,
 	ret_val = igb_poll_for_msg(hw, mbx_id);
 
 	if (!ret_val)
-		ret_val = mbx->ops.read(hw, msg, size, mbx_id);
+		ret_val = mbx->ops.read(hw, msg, size, mbx_id, true);
 out:
 	return ret_val;
 }
@@ -423,13 +424,14 @@ static s32 igb_write_mbx_pf(struct e1000_hw *hw, u32 *msg, u16 size,
  *  @msg: The message buffer
  *  @size: Length of buffer
  *  @vf_number: the VF index
+ *  @unlock: unlock the mailbox when done?
  *
  *  This function copies a message from the mailbox buffer to the caller's
  *  memory buffer.  The presumption is that the caller knows that there was
  *  a message due to a VF request so no polling for message is needed.
  **/
 static s32 igb_read_mbx_pf(struct e1000_hw *hw, u32 *msg, u16 size,
-			   u16 vf_number)
+			   u16 vf_number, bool unlock)
 {
 	s32 ret_val;
 	u16 i;
@@ -443,8 +445,12 @@ static s32 igb_read_mbx_pf(struct e1000_hw *hw, u32 *msg, u16 size,
 	for (i = 0; i < size; i++)
 		msg[i] = array_rd32(E1000_VMBMEM(vf_number), i);
 
-	/* Acknowledge the message and release buffer */
-	wr32(E1000_P2VMAILBOX(vf_number), E1000_P2VMAILBOX_ACK);
+	/* Acknowledge the message and release mailbox lock (or not) */
+	if (unlock)
+		wr32(E1000_P2VMAILBOX(vf_number), E1000_P2VMAILBOX_ACK);
+	else
+		wr32(E1000_P2VMAILBOX(vf_number),
+		     E1000_P2VMAILBOX_ACK | E1000_P2VMAILBOX_PFU);
 
 	/* update stats */
 	hw->mbx.stats.msgs_rx++;
diff --git a/drivers/net/ethernet/intel/igb/e1000_mbx.h b/drivers/net/ethernet/intel/igb/e1000_mbx.h
index a98c5dc60afd..a62b08e1572e 100644
--- a/drivers/net/ethernet/intel/igb/e1000_mbx.h
+++ b/drivers/net/ethernet/intel/igb/e1000_mbx.h
@@ -67,7 +67,8 @@
 
 #define E1000_PF_CONTROL_MSG	0x0100 /* PF control message */
 
-s32 igb_read_mbx(struct e1000_hw *hw, u32 *msg, u16 size, u16 mbx_id);
+s32 igb_read_mbx(struct e1000_hw *hw, u32 *msg, u16 size, u16 mbx_id,
+		 bool unlock);
 s32 igb_write_mbx(struct e1000_hw *hw, u32 *msg, u16 size, u16 mbx_id);
 s32 igb_check_for_msg(struct e1000_hw *hw, u16 mbx_id);
 s32 igb_check_for_ack(struct e1000_hw *hw, u16 mbx_id);
diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index 1cf74aa4ebd9..c046b37ad9eb 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -6664,32 +6664,33 @@ static void igb_rcv_msg_from_vf(struct igb_adapter *adapter, u32 vf)
 	struct vf_data_storage *vf_data = &adapter->vf_data[vf];
 	s32 retval;
 
-	retval = igb_read_mbx(hw, msgbuf, E1000_VFMAILBOX_SIZE, vf);
+	retval = igb_read_mbx(hw, msgbuf, E1000_VFMAILBOX_SIZE, vf, false);
 
 	if (retval) {
 		/* if receive failed revoke VF CTS stats and restart init */
 		dev_err(&pdev->dev, "Error receiving message from VF\n");
 		vf_data->flags &= ~IGB_VF_FLAG_CTS;
 		if (!time_after(jiffies, vf_data->last_nack + (2 * HZ)))
-			return;
+			goto unlock;
 		goto out;
 	}
 
 	/* this is a message we already processed, do nothing */
 	if (msgbuf[0] & (E1000_VT_MSGTYPE_ACK | E1000_VT_MSGTYPE_NACK))
-		return;
+		goto unlock;
 
 	/* until the vf completes a reset it should not be
 	 * allowed to start any configuration.
 	 */
 	if (msgbuf[0] == E1000_VF_RESET) {
+		/* unlocks mailbox */
 		igb_vf_reset_msg(adapter, vf);
 		return;
 	}
 
 	if (!(vf_data->flags & IGB_VF_FLAG_CTS)) {
 		if (!time_after(jiffies, vf_data->last_nack + (2 * HZ)))
-			return;
+			goto unlock;
 		retval = -1;
 		goto out;
 	}
@@ -6730,7 +6731,12 @@ static void igb_rcv_msg_from_vf(struct igb_adapter *adapter, u32 vf)
 	else
 		msgbuf[0] |= E1000_VT_MSGTYPE_ACK;
 
+	/* unlocks mailbox */
 	igb_write_mbx(hw, msgbuf, 1, vf);
+	return;
+
+unlock:
+	igb_unlock_mbx(hw, vf);
 }
 
 static void igb_msg_task(struct igb_adapter *adapter)
-- 
2.9.4

[Intel-wired-lan] [PATCH 1/3] igb: add argument names to mailbox op function declarations

[Brown, Aaron F]

> From: Intel-wired-lan [mailto:intel-wired-lan-bounces at osuosl.org] On Behalf
> Of Greg Edwards
> Sent: Wednesday, June 28, 2017 8:22 AM
> To: intel-wired-lan at lists.osuosl.org
> Cc: Greg Edwards <gedwards@ddn.com>
> Subject: [Intel-wired-lan] [PATCH 1/3] igb: add argument names to mailbox
> op function declarations
> 
> Signed-off-by: Greg Edwards <gedwards@ddn.com>
> ---
>  drivers/net/ethernet/intel/igb/e1000_hw.h  | 15 ++++++++-------
>  drivers/net/ethernet/intel/igb/e1000_mbx.h | 12 ++++++------
>  2 files changed, 14 insertions(+), 13 deletions(-)

Tested-by: Aaron Brown <aaron.f.brown@intel.com>

[Intel-wired-lan] [PATCH 2/3] igb: expose mailbox unlock method

[Brown, Aaron F]

> From: Intel-wired-lan [mailto:intel-wired-lan-bounces at osuosl.org] On Behalf
> Of Greg Edwards
> Sent: Wednesday, June 28, 2017 8:22 AM
> To: intel-wired-lan at lists.osuosl.org
> Cc: Greg Edwards <gedwards@ddn.com>
> Subject: [Intel-wired-lan] [PATCH 2/3] igb: expose mailbox unlock method
> 
> Add a mailbox unlock method to e1000_mbx_operations, which will be used
> to unlock the PF/VF mailbox by the PF.
> 
> Signed-off-by: Greg Edwards <gedwards@ddn.com>
> ---
>  drivers/net/ethernet/intel/igb/e1000_hw.h  |  1 +
>  drivers/net/ethernet/intel/igb/e1000_mbx.c | 39
> ++++++++++++++++++++++++++++++
>  drivers/net/ethernet/intel/igb/e1000_mbx.h |  1 +
>  3 files changed, 41 insertions(+)

Tested-by: Aaron Brown <aaron.f.brown@intel.com>

[Intel-wired-lan] [PATCH 3/3] igb: do not drop PF mailbox lock after read of VF message

[Brown, Aaron F]

> From: Intel-wired-lan [mailto:intel-wired-lan-bounces at osuosl.org] On Behalf
> Of Greg Edwards
> Sent: Wednesday, June 28, 2017 8:22 AM
> To: intel-wired-lan at lists.osuosl.org
> Cc: Greg Edwards <gedwards@ddn.com>
> Subject: [Intel-wired-lan] [PATCH 3/3] igb: do not drop PF mailbox lock after
> read of VF message
> 
> When the PF receives a mailbox message from the VF, it grabs the mailbox
> lock, reads the VF message from the mailbox, ACKs the message and drops
> the lock.
> 
> While the PF is performing the action for the VF message, nothing
> prevents another VF message from being posted to the mailbox.  The
> current code handles this condition by just dropping any new VF messages
> without processing them.  This results in a mailbox timeout in the VM
> for posted messages waiting for an ACK, and the VF is reset by the
> igbvf_watchdog_task in the VM.
> 
> Given the right sequence of VF messages and mailbox timeouts, this
> condition can go on ad infinitum.
> 
> Modify the PF mailbox read method to take an 'unlock' argument that
> optionally leaves the mailbox locked by the PF after reading the VF
> message.  This ensures another VF message is not posted to the mailbox
> until after the PF has completed processing the VF message and written
> its reply.
> 
> Signed-off-by: Greg Edwards <gedwards@ddn.com>
> ---
>  drivers/net/ethernet/intel/igb/e1000_hw.h  |  3 ++-
>  drivers/net/ethernet/intel/igb/e1000_mbx.c | 18 ++++++++++++------
>  drivers/net/ethernet/intel/igb/e1000_mbx.h |  3 ++-
>  drivers/net/ethernet/intel/igb/igb_main.c  | 14 ++++++++++----
>  4 files changed, 26 insertions(+), 12 deletions(-)

Tested-by: Aaron Brown <aaron.f.brown@intel.com>

[Intel-wired-lan] [PATCH 3/3] igb: do not drop PF mailbox lock after read of VF message

[Alexander Duyck]

On Wed, Jun 28, 2017 at 8:22 AM, Greg Edwards <gedwards@ddn.com> wrote:
> When the PF receives a mailbox message from the VF, it grabs the mailbox
> lock, reads the VF message from the mailbox, ACKs the message and drops
> the lock.
>
> While the PF is performing the action for the VF message, nothing
> prevents another VF message from being posted to the mailbox.  The
> current code handles this condition by just dropping any new VF messages
> without processing them.  This results in a mailbox timeout in the VM
> for posted messages waiting for an ACK, and the VF is reset by the
> igbvf_watchdog_task in the VM.
>
> Given the right sequence of VF messages and mailbox timeouts, this
> condition can go on ad infinitum.
>
> Modify the PF mailbox read method to take an 'unlock' argument that
> optionally leaves the mailbox locked by the PF after reading the VF
> message.  This ensures another VF message is not posted to the mailbox
> until after the PF has completed processing the VF message and written
> its reply.
>
> Signed-off-by: Greg Edwards <gedwards@ddn.com>

I really don't like the design of this patch. It is fixing the PF for
a VF bug. The VF should not be sending multiple messages to the PF at
the same time. In the case of the Linux VF anyway there should be the
RTNL to prevent multiple messages from being sent. If we are sending
multiple messages at the same time from the VF it points at a bug in
the VF, not the PF driver.

- Alex

[Intel-wired-lan] [PATCH 3/3] igb: do not drop PF mailbox lock after read of VF message

[Greg Edwards]

On Fri, Jul 14, 2017 at 09:03:50PM -0700, Alexander Duyck wrote:
> On Wed, Jun 28, 2017 at 8:22 AM, Greg Edwards <gedwards@ddn.com> wrote:
>> When the PF receives a mailbox message from the VF, it grabs the mailbox
>> lock, reads the VF message from the mailbox, ACKs the message and drops
>> the lock.
>>
>> While the PF is performing the action for the VF message, nothing
>> prevents another VF message from being posted to the mailbox.  The
>> current code handles this condition by just dropping any new VF messages
>> without processing them.  This results in a mailbox timeout in the VM
>> for posted messages waiting for an ACK, and the VF is reset by the
>> igbvf_watchdog_task in the VM.
>>
>> Given the right sequence of VF messages and mailbox timeouts, this
>> condition can go on ad infinitum.
>>
>> Modify the PF mailbox read method to take an 'unlock' argument that
>> optionally leaves the mailbox locked by the PF after reading the VF
>> message.  This ensures another VF message is not posted to the mailbox
>> until after the PF has completed processing the VF message and written
>> its reply.
>
> I really don't like the design of this patch. It is fixing the PF for
> a VF bug. The VF should not be sending multiple messages to the PF at
> the same time. In the case of the Linux VF anyway there should be the
> RTNL to prevent multiple messages from being sent. If we are sending
> multiple messages at the same time from the VF it points at a bug in
> the VF, not the PF driver.

Sure, I can look at fixing it in the VF driver, if that would be the
preferred approach.  The I350 datasheet made it sound like the PF should
be able to handle this condition (Section 7.8.2.9.1 "VF to PF Mailbox"),
but I realize that may be implementation specific.

Greg