[Intel-wired-lan] [PATCH net-next v1 0/4] Eliminate race condition occurrence during sending ASQ commands

[Intel-wired-lan] [PATCH net-next v1 0/4] Eliminate race condition occurrence during sending ASQ commands

[Jedrzej Jagielski]

Provide modifications in ASQ commands in order to
prevent for race condition occurrences.
Add new versions of ASQ commands based on sending commands
in atomic context. New versions of commands are marked as _v2.
Also change already existing ASQ commands to be able to work
in atomic context. Make them to invoking previously added
i40e_asq_send_command_atomic() function to perform aq calls.

Eliminate problem with race conditions connected to accessing
hw->aq.asq_last_status during callig ASQ commands.

1. Add new versions of send ASQ command functions including
new version of i40e_aq_remove_macvlan().

2. Add new version of i40e_aq_add_macvlan() function.

3. Change i40e_aqc_add_filters() and i40e_aqc_del_filters() functions
located in i40e_main.c to use new versions of i40e_aq_add_macvlan()
and i40e_aq_remove_macvlan().

4. Change several i40e_aq_ functions to send commands in atomic
context by using i40e_asq_send_command_atomic(..., true).

Jedrzej Jagielski (4):
  i40e: Add new versions of send ASQ command functions
  i40e: Add new version of i40e_aq_add_macvlan function
  i40e: Fix race condition while adding/deleting MAC/VLAN filters
  i40e: Add sending commands in atomic context

 drivers/net/ethernet/intel/i40e/i40e_adminq.c |  92 +++++++++-
 drivers/net/ethernet/intel/i40e/i40e_common.c | 162 +++++++++++++++---
 drivers/net/ethernet/intel/i40e/i40e_main.c   |  24 +--
 .../net/ethernet/intel/i40e/i40e_prototype.h  |  25 +++
 4 files changed, 258 insertions(+), 45 deletions(-)

-- 
2.27.0

[Intel-wired-lan] [PATCH net-next v1 1/4] i40e: Add new versions of send ASQ command functions

[Jedrzej Jagielski]

ASQ send command functions are returning only i40e status codes
yet some calling functions also need Admin Queue status
that is stored in hw->aq.asq_last_status. Since hw object
is stored on a heap it introduces a possibility for
a race condition in access to hw if calling function is not
fast enough to read hw->aq.asq_last_status before next
send ASQ command is executed.

Add new versions of send ASQ command functions that return
Admin Queue status on the stack to avoid race conditions
in access to hw->aq.asq_last_status.
Add new _v2 version of i40e_aq_remove_macvlan that is using
new _v2 versions of ASQ send command functions and returns
the Admin Queue status on the stack.

Signed-off-by: Sylwester Dziedziuch <sylwesterx.dziedziuch@intel.com>
Signed-off-by: Jedrzej Jagielski <jedrzej.jagielski@intel.com>
---
 drivers/net/ethernet/intel/i40e/i40e_adminq.c | 92 +++++++++++++++++--
 drivers/net/ethernet/intel/i40e/i40e_common.c | 49 ++++++++++
 .../net/ethernet/intel/i40e/i40e_prototype.h  | 20 ++++
 3 files changed, 153 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_adminq.c b/drivers/net/ethernet/intel/i40e/i40e_adminq.c
index 7abef88801fb..10db5180d518 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_adminq.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_adminq.c
@@ -769,7 +769,7 @@ static bool i40e_asq_done(struct i40e_hw *hw)
 }
 
 /**
- *  i40e_asq_send_command_atomic - send command to Admin Queue
+ *  i40e_asq_send_command_atomic_exec - send command to Admin Queue
  *  @hw: pointer to the hw struct
  *  @desc: prefilled descriptor describing the command (non DMA mem)
  *  @buff: buffer to use for indirect commands
@@ -780,11 +780,13 @@ static bool i40e_asq_done(struct i40e_hw *hw)
  *  This is the main send command driver routine for the Admin Queue send
  *  queue.  It runs the queue, cleans the queue, etc
  **/
-i40e_status
-i40e_asq_send_command_atomic(struct i40e_hw *hw, struct i40e_aq_desc *desc,
-			     void *buff, /* can be NULL */ u16  buff_size,
-			     struct i40e_asq_cmd_details *cmd_details,
-			     bool is_atomic_context)
+static i40e_status
+i40e_asq_send_command_atomic_exec(struct i40e_hw *hw,
+				  struct i40e_aq_desc *desc,
+				  void *buff, /* can be NULL */
+				  u16  buff_size,
+				  struct i40e_asq_cmd_details *cmd_details,
+				  bool is_atomic_context)
 {
 	i40e_status status = 0;
 	struct i40e_dma_mem *dma_buff = NULL;
@@ -794,8 +796,6 @@ i40e_asq_send_command_atomic(struct i40e_hw *hw, struct i40e_aq_desc *desc,
 	u16  retval = 0;
 	u32  val = 0;
 
-	mutex_lock(&hw->aq.asq_mutex);
-
 	if (hw->aq.asq.count == 0) {
 		i40e_debug(hw, I40E_DEBUG_AQ_MESSAGE,
 			   "AQTX: Admin queue not initialized.\n");
@@ -969,6 +969,36 @@ i40e_asq_send_command_atomic(struct i40e_hw *hw, struct i40e_aq_desc *desc,
 	}
 
 asq_send_command_error:
+	return status;
+}
+
+/**
+ *  i40e_asq_send_command_atomic - send command to Admin Queue
+ *  @hw: pointer to the hw struct
+ *  @desc: prefilled descriptor describing the command (non DMA mem)
+ *  @buff: buffer to use for indirect commands
+ *  @buff_size: size of buffer for indirect commands
+ *  @cmd_details: pointer to command details structure
+ *  @is_atomic_context: is the function called in an atomic context?
+ *
+ *  Acquires the lock and calls the main send command execution
+ *  routine.
+ **/
+i40e_status
+i40e_asq_send_command_atomic(struct i40e_hw *hw,
+			     struct i40e_aq_desc *desc,
+			     void *buff, /* can be NULL */
+			     u16  buff_size,
+			     struct i40e_asq_cmd_details *cmd_details,
+			     bool is_atomic_context)
+{
+	i40e_status status = I40E_SUCCESS;
+
+	mutex_lock(&hw->aq.asq_mutex);
+	status = i40e_asq_send_command_atomic_exec(hw, desc, buff, buff_size,
+						   cmd_details,
+						   is_atomic_context);
+
 	mutex_unlock(&hw->aq.asq_mutex);
 	return status;
 }
@@ -982,6 +1012,52 @@ i40e_asq_send_command(struct i40e_hw *hw, struct i40e_aq_desc *desc,
 					    cmd_details, false);
 }
 
+/**
+ *  i40e_asq_send_command_atomic_v2 - send command to Admin Queue
+ *  @hw: pointer to the hw struct
+ *  @desc: prefilled descriptor describing the command (non DMA mem)
+ *  @buff: buffer to use for indirect commands
+ *  @buff_size: size of buffer for indirect commands
+ *  @cmd_details: pointer to command details structure
+ *  @is_atomic_context: is the function called in an atomic context?
+ *  @aq_status: pointer to Admin Queue status return value
+ *
+ *  Acquires the lock and calls the main send command execution
+ *  routine. Returns the last Admin Queue status in aq_status
+ *  to avoid race conditions in access to hw->aq.asq_last_status.
+ **/
+i40e_status
+i40e_asq_send_command_atomic_v2(struct i40e_hw *hw,
+				struct i40e_aq_desc *desc,
+				void *buff, /* can be NULL */
+				u16  buff_size,
+				struct i40e_asq_cmd_details *cmd_details,
+				bool is_atomic_context,
+				enum i40e_admin_queue_err *aq_status)
+{
+	i40e_status status = I40E_SUCCESS;
+
+	mutex_lock(&hw->aq.asq_mutex);
+	status = i40e_asq_send_command_atomic_exec(hw, desc, buff,
+						   buff_size,
+						   cmd_details,
+						   is_atomic_context);
+	if (aq_status)
+		*aq_status = hw->aq.asq_last_status;
+	mutex_unlock(&hw->aq.asq_mutex);
+	return status;
+}
+
+inline i40e_status
+i40e_asq_send_command_v2(struct i40e_hw *hw, struct i40e_aq_desc *desc,
+			 void *buff, /* can be NULL */ u16  buff_size,
+			 struct i40e_asq_cmd_details *cmd_details,
+			 enum i40e_admin_queue_err *aq_status)
+{
+	return i40e_asq_send_command_atomic_v2(hw, desc, buff, buff_size,
+					       cmd_details, true, aq_status);
+}
+
 /**
  *  i40e_fill_default_direct_cmd_desc - AQ descriptor helper function
  *  @desc:     pointer to the temp descriptor (non DMA mem)
diff --git a/drivers/net/ethernet/intel/i40e/i40e_common.c b/drivers/net/ethernet/intel/i40e/i40e_common.c
index 9ddeb015eb7e..cdd6e5bdbe4f 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_common.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_common.c
@@ -2721,6 +2721,55 @@ i40e_status i40e_aq_remove_macvlan(struct i40e_hw *hw, u16 seid,
 	return status;
 }
 
+/**
+ * i40e_aq_remove_macvlan_v2
+ * @hw: pointer to the hw struct
+ * @seid: VSI for the mac address
+ * @mv_list: list of macvlans to be removed
+ * @count: length of the list
+ * @cmd_details: pointer to command details structure or NULL
+ * @aq_status: pointer to Admin Queue status return value
+ *
+ * Remove MAC/VLAN addresses from the HW filtering.
+ * The _v2 version returns the last Admin Queue status in aq_status
+ * to avoid race conditions in access to hw->aq.asq_last_status.
+ * It also calls _v2 versions of asq_send_command functions to
+ * get the aq_status on the stack.
+ **/
+i40e_status
+i40e_aq_remove_macvlan_v2(struct i40e_hw *hw, u16 seid,
+			  struct i40e_aqc_remove_macvlan_element_data *mv_list,
+			  u16 count, struct i40e_asq_cmd_details *cmd_details,
+			  enum i40e_admin_queue_err *aq_status)
+{
+	struct i40e_aq_desc desc;
+	struct i40e_aqc_macvlan *cmd;
+	i40e_status status;
+	u16 buf_size;
+
+	if (count == 0 || !mv_list || !hw)
+		return I40E_ERR_PARAM;
+
+	buf_size = count * sizeof(*mv_list);
+
+	/* prep the rest of the request */
+	i40e_fill_default_direct_cmd_desc(&desc, i40e_aqc_opc_remove_macvlan);
+	cmd = (struct i40e_aqc_macvlan *)&desc.params.raw;
+	cmd->num_addresses = cpu_to_le16(count);
+	cmd->seid[0] = cpu_to_le16(I40E_AQC_MACVLAN_CMD_SEID_VALID | seid);
+	cmd->seid[1] = 0;
+	cmd->seid[2] = 0;
+
+	desc.flags |= cpu_to_le16((u16)(I40E_AQ_FLAG_BUF | I40E_AQ_FLAG_RD));
+	if (buf_size > I40E_AQ_LARGE_BUF)
+		desc.flags |= cpu_to_le16((u16)I40E_AQ_FLAG_LB);
+
+	status = i40e_asq_send_command_atomic_v2(hw, &desc, mv_list, buf_size,
+						 cmd_details, true, aq_status);
+
+	return status;
+}
+
 /**
  * i40e_mirrorrule_op - Internal helper function to add/delete mirror rule
  * @hw: pointer to the hw struct
diff --git a/drivers/net/ethernet/intel/i40e/i40e_prototype.h b/drivers/net/ethernet/intel/i40e/i40e_prototype.h
index 9241b6005ad3..4ace56a911d9 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_prototype.h
+++ b/drivers/net/ethernet/intel/i40e/i40e_prototype.h
@@ -27,10 +27,25 @@ i40e_asq_send_command(struct i40e_hw *hw, struct i40e_aq_desc *desc,
 		      void *buff, /* can be NULL */ u16  buff_size,
 		      struct i40e_asq_cmd_details *cmd_details);
 i40e_status
+i40e_asq_send_command_v2(struct i40e_hw *hw,
+			 struct i40e_aq_desc *desc,
+			 void *buff, /* can be NULL */
+			 u16  buff_size,
+			 struct i40e_asq_cmd_details *cmd_details,
+			 enum i40e_admin_queue_err *aq_status);
+i40e_status
 i40e_asq_send_command_atomic(struct i40e_hw *hw, struct i40e_aq_desc *desc,
 			     void *buff, /* can be NULL */ u16  buff_size,
 			     struct i40e_asq_cmd_details *cmd_details,
 			     bool is_atomic_context);
+i40e_status
+i40e_asq_send_command_atomic_v2(struct i40e_hw *hw,
+				struct i40e_aq_desc *desc,
+				void *buff, /* can be NULL */
+				u16  buff_size,
+				struct i40e_asq_cmd_details *cmd_details,
+				bool is_atomic_context,
+				enum i40e_admin_queue_err *aq_status);
 
 /* debug function for adminq */
 void i40e_debug_aq(struct i40e_hw *hw, enum i40e_debug_mask mask,
@@ -153,6 +168,11 @@ i40e_status i40e_aq_add_macvlan(struct i40e_hw *hw, u16 vsi_id,
 i40e_status i40e_aq_remove_macvlan(struct i40e_hw *hw, u16 vsi_id,
 			struct i40e_aqc_remove_macvlan_element_data *mv_list,
 			u16 count, struct i40e_asq_cmd_details *cmd_details);
+i40e_status
+i40e_aq_remove_macvlan_v2(struct i40e_hw *hw, u16 seid,
+			  struct i40e_aqc_remove_macvlan_element_data *mv_list,
+			  u16 count, struct i40e_asq_cmd_details *cmd_details,
+			  enum i40e_admin_queue_err *aq_status);
 i40e_status i40e_aq_add_mirrorrule(struct i40e_hw *hw, u16 sw_seid,
 			u16 rule_type, u16 dest_vsi, u16 count, __le16 *mr_list,
 			struct i40e_asq_cmd_details *cmd_details,
-- 
2.27.0

[Intel-wired-lan] [PATCH net-next v1 2/4] i40e: Add new version of i40e_aq_add_macvlan function

[Jedrzej Jagielski]

ASQ send command functions are returning only i40e status codes
yet some calling functions also need Admin Queue status
that is stored in hw->aq.asq_last_status. Since hw object
is stored on a heap it introduces a possibility for
a race condition in access to hw if calling function is not
fast enough to read hw->aq.asq_last_status before next
send ASQ command is executed.

Add new _v2 version of i40e_aq_add_macvlan that is using
new _v2 versions of ASQ send command functions and returns
the Admin Queue status on the stack.

Signed-off-by: Sylwester Dziedziuch <sylwesterx.dziedziuch@intel.com>
Signed-off-by: Jedrzej Jagielski <jedrzej.jagielski@intel.com>
---
 drivers/net/ethernet/intel/i40e/i40e_common.c | 92 +++++++++++++++----
 .../net/ethernet/intel/i40e/i40e_prototype.h  |  5 +
 2 files changed, 80 insertions(+), 17 deletions(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_common.c b/drivers/net/ethernet/intel/i40e/i40e_common.c
index cdd6e5bdbe4f..a15b09b39979 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_common.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_common.c
@@ -2632,33 +2632,28 @@ i40e_status i40e_aq_get_veb_parameters(struct i40e_hw *hw,
 }
 
 /**
- * i40e_aq_add_macvlan
- * @hw: pointer to the hw struct
- * @seid: VSI for the mac address
+ * i40e_prepare_add_macvlan
  * @mv_list: list of macvlans to be added
+ * @desc: pointer to AQ descriptor structure
  * @count: length of the list
- * @cmd_details: pointer to command details structure or NULL
+ * @seid: VSI for the mac address
  *
- * Add MAC/VLAN addresses to the HW filtering
+ * Internal helper function that prepares the add macvlan request
+ * and returns the buffer size.
  **/
-i40e_status i40e_aq_add_macvlan(struct i40e_hw *hw, u16 seid,
-			struct i40e_aqc_add_macvlan_element_data *mv_list,
-			u16 count, struct i40e_asq_cmd_details *cmd_details)
+static u16
+i40e_prepare_add_macvlan(struct i40e_aqc_add_macvlan_element_data *mv_list,
+			 struct i40e_aq_desc *desc, u16 count, u16 seid)
 {
-	struct i40e_aq_desc desc;
 	struct i40e_aqc_macvlan *cmd =
-		(struct i40e_aqc_macvlan *)&desc.params.raw;
-	i40e_status status;
+		(struct i40e_aqc_macvlan *)&desc->params.raw;
 	u16 buf_size;
 	int i;
 
-	if (count == 0 || !mv_list || !hw)
-		return I40E_ERR_PARAM;
-
 	buf_size = count * sizeof(*mv_list);
 
 	/* prep the rest of the request */
-	i40e_fill_default_direct_cmd_desc(&desc, i40e_aqc_opc_add_macvlan);
+	i40e_fill_default_direct_cmd_desc(desc, i40e_aqc_opc_add_macvlan);
 	cmd->num_addresses = cpu_to_le16(count);
 	cmd->seid[0] = cpu_to_le16(I40E_AQC_MACVLAN_CMD_SEID_VALID | seid);
 	cmd->seid[1] = 0;
@@ -2669,9 +2664,36 @@ i40e_status i40e_aq_add_macvlan(struct i40e_hw *hw, u16 seid,
 			mv_list[i].flags |=
 			       cpu_to_le16(I40E_AQC_MACVLAN_ADD_USE_SHARED_MAC);
 
-	desc.flags |= cpu_to_le16((u16)(I40E_AQ_FLAG_BUF | I40E_AQ_FLAG_RD));
+	desc->flags |= cpu_to_le16((u16)(I40E_AQ_FLAG_BUF | I40E_AQ_FLAG_RD));
 	if (buf_size > I40E_AQ_LARGE_BUF)
-		desc.flags |= cpu_to_le16((u16)I40E_AQ_FLAG_LB);
+		desc->flags |= cpu_to_le16((u16)I40E_AQ_FLAG_LB);
+
+	return buf_size;
+}
+
+/**
+ * i40e_aq_add_macvlan
+ * @hw: pointer to the hw struct
+ * @seid: VSI for the mac address
+ * @mv_list: list of macvlans to be added
+ * @count: length of the list
+ * @cmd_details: pointer to command details structure or NULL
+ *
+ * Add MAC/VLAN addresses to the HW filtering
+ **/
+i40e_status
+i40e_aq_add_macvlan(struct i40e_hw *hw, u16 seid,
+		    struct i40e_aqc_add_macvlan_element_data *mv_list,
+		    u16 count, struct i40e_asq_cmd_details *cmd_details)
+{
+	struct i40e_aq_desc desc;
+	i40e_status status;
+	u16 buf_size;
+
+	if (count == 0 || !mv_list || !hw)
+		return I40E_ERR_PARAM;
+
+	buf_size = i40e_prepare_add_macvlan(mv_list, &desc, count, seid);
 
 	status = i40e_asq_send_command(hw, &desc, mv_list, buf_size,
 				       cmd_details);
@@ -2679,6 +2701,42 @@ i40e_status i40e_aq_add_macvlan(struct i40e_hw *hw, u16 seid,
 	return status;
 }
 
+/**
+ * i40e_aq_add_macvlan_v2
+ * @hw: pointer to the hw struct
+ * @seid: VSI for the mac address
+ * @mv_list: list of macvlans to be added
+ * @count: length of the list
+ * @cmd_details: pointer to command details structure or NULL
+ * @aq_status: pointer to Admin Queue status return value
+ *
+ * Add MAC/VLAN addresses to the HW filtering.
+ * The _v2 version returns the last Admin Queue status in aq_status
+ * to avoid race conditions in access to hw->aq.asq_last_status.
+ * It also calls _v2 versions of asq_send_command functions to
+ * get the aq_status on the stack.
+ **/
+i40e_status
+i40e_aq_add_macvlan_v2(struct i40e_hw *hw, u16 seid,
+		       struct i40e_aqc_add_macvlan_element_data *mv_list,
+		       u16 count, struct i40e_asq_cmd_details *cmd_details,
+		       enum i40e_admin_queue_err *aq_status)
+{
+	struct i40e_aq_desc desc;
+	i40e_status status;
+	u16 buf_size;
+
+	if (count == 0 || !mv_list || !hw)
+		return I40E_ERR_PARAM;
+
+	buf_size = i40e_prepare_add_macvlan(mv_list, &desc, count, seid);
+
+	status = i40e_asq_send_command_atomic_v2(hw, &desc, mv_list, buf_size,
+						 cmd_details, true, aq_status);
+
+	return status;
+}
+
 /**
  * i40e_aq_remove_macvlan
  * @hw: pointer to the hw struct
diff --git a/drivers/net/ethernet/intel/i40e/i40e_prototype.h b/drivers/net/ethernet/intel/i40e/i40e_prototype.h
index 4ace56a911d9..ebdcde6f1aeb 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_prototype.h
+++ b/drivers/net/ethernet/intel/i40e/i40e_prototype.h
@@ -165,6 +165,11 @@ i40e_status i40e_aq_get_veb_parameters(struct i40e_hw *hw,
 i40e_status i40e_aq_add_macvlan(struct i40e_hw *hw, u16 vsi_id,
 			struct i40e_aqc_add_macvlan_element_data *mv_list,
 			u16 count, struct i40e_asq_cmd_details *cmd_details);
+i40e_status
+i40e_aq_add_macvlan_v2(struct i40e_hw *hw, u16 seid,
+		       struct i40e_aqc_add_macvlan_element_data *mv_list,
+		       u16 count, struct i40e_asq_cmd_details *cmd_details,
+		       enum i40e_admin_queue_err *aq_status);
 i40e_status i40e_aq_remove_macvlan(struct i40e_hw *hw, u16 vsi_id,
 			struct i40e_aqc_remove_macvlan_element_data *mv_list,
 			u16 count, struct i40e_asq_cmd_details *cmd_details);
-- 
2.27.0

[Intel-wired-lan] [PATCH net-next v1 3/4] i40e: Fix race condition while adding/deleting MAC/VLAN filters

[Jedrzej Jagielski]

There was a race condition in access to hw->aq.asq_last_status
while adding and deleting  MAC/VLAN filters causing
incorrect error status to be printed as ERROR OK instead of
the correct error.

Change calls to i40e_aq_add_macvlan in i40e_aqc_add_filters
and i40e_aq_remove_macvlan in i40e_aqc_del_filters
to  _v2 versions that return Admin Queue status on the stack
to avoid race conditions in access to hw->aq.asq_last_status.

Fixes: 0093631966aa ("i40e: remove duplicate add/delete adminq command code for filters")
Signed-off-by: Sylwester Dziedziuch <sylwesterx.dziedziuch@intel.com>
Signed-off-by: Jedrzej Jagielski <jedrzej.jagielski@intel.com>
---
 drivers/net/ethernet/intel/i40e/i40e_main.c | 24 +++++++++++----------
 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
index d80b758a1423..304d7f1296ed 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_main.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
@@ -2187,19 +2187,19 @@ void i40e_aqc_del_filters(struct i40e_vsi *vsi, const char *vsi_name,
 			  int num_del, int *retval)
 {
 	struct i40e_hw *hw = &vsi->back->hw;
+	enum i40e_admin_queue_err aq_status;
 	i40e_status aq_ret;
-	int aq_err;
 
-	aq_ret = i40e_aq_remove_macvlan(hw, vsi->seid, list, num_del, NULL);
-	aq_err = hw->aq.asq_last_status;
+	aq_ret = i40e_aq_remove_macvlan_v2(hw, vsi->seid, list, num_del, NULL,
+					   &aq_status);
 
 	/* Explicitly ignore and do not report when firmware returns ENOENT */
-	if (aq_ret && !(aq_err == I40E_AQ_RC_ENOENT)) {
+	if (aq_ret && !(aq_status == I40E_AQ_RC_ENOENT)) {
 		*retval = -EIO;
 		dev_info(&vsi->back->pdev->dev,
 			 "ignoring delete macvlan error on %s, err %s, aq_err %s\n",
 			 vsi_name, i40e_stat_str(hw, aq_ret),
-			 i40e_aq_str(hw, aq_err));
+			 i40e_aq_str(hw, aq_status));
 	}
 }
 
@@ -2222,10 +2222,10 @@ void i40e_aqc_add_filters(struct i40e_vsi *vsi, const char *vsi_name,
 			  int num_add)
 {
 	struct i40e_hw *hw = &vsi->back->hw;
-	int aq_err, fcnt;
+	enum i40e_admin_queue_err aq_status;
+	int fcnt;
 
-	i40e_aq_add_macvlan(hw, vsi->seid, list, num_add, NULL);
-	aq_err = hw->aq.asq_last_status;
+	i40e_aq_add_macvlan_v2(hw, vsi->seid, list, num_add, NULL, &aq_status);
 	fcnt = i40e_update_filter_state(num_add, list, add_head);
 
 	if (fcnt != num_add) {
@@ -2233,17 +2233,19 @@ void i40e_aqc_add_filters(struct i40e_vsi *vsi, const char *vsi_name,
 			set_bit(__I40E_VSI_OVERFLOW_PROMISC, vsi->state);
 			dev_warn(&vsi->back->pdev->dev,
 				 "Error %s adding RX filters on %s, promiscuous mode forced on\n",
-				 i40e_aq_str(hw, aq_err), vsi_name);
+				 i40e_aq_str(hw, aq_status), vsi_name);
 		} else if (vsi->type == I40E_VSI_SRIOV ||
 			   vsi->type == I40E_VSI_VMDQ1 ||
 			   vsi->type == I40E_VSI_VMDQ2) {
 			dev_warn(&vsi->back->pdev->dev,
 				 "Error %s adding RX filters on %s, please set promiscuous on manually for %s\n",
-				 i40e_aq_str(hw, aq_err), vsi_name, vsi_name);
+				 i40e_aq_str(hw, aq_status), vsi_name,
+					     vsi_name);
 		} else {
 			dev_warn(&vsi->back->pdev->dev,
 				 "Error %s adding RX filters on %s, incorrect VSI type: %i.\n",
-				 i40e_aq_str(hw, aq_err), vsi_name, vsi->type);
+				 i40e_aq_str(hw, aq_status), vsi_name,
+					     vsi->type);
 		}
 	}
 }
-- 
2.27.0

[Intel-wired-lan] [PATCH net-next v1 4/4] i40e: Add sending commands in atomic context

[Jedrzej Jagielski]

Change functions:
- i40e_aq_add_macvlan
- i40e_aq_remove_macvlan
- i40e_aq_delete_element
- i40e_aq_add_vsi
- i40e_aq_update_vsi_params
to explicitly use i40e_asq_send_command_atomic(..., true)
instead of i40e_asq_send_command, as they use mutexes and do some
work in an atomic context.
Without this change setting vlan via netdev will fail with
call trace cased by bug "BUG: scheduling while atomic".

Signed-off-by: Witold Fijalkowski <witoldx.fijalkowski@intel.com>
Signed-off-by: Jedrzej Jagielski <jedrzej.jagielski@intel.com>
---
 drivers/net/ethernet/intel/i40e/i40e_common.c | 21 +++++++++++--------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_common.c b/drivers/net/ethernet/intel/i40e/i40e_common.c
index a15b09b39979..214400623dd9 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_common.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_common.c
@@ -1899,8 +1899,9 @@ i40e_status i40e_aq_add_vsi(struct i40e_hw *hw,
 
 	desc.flags |= cpu_to_le16((u16)(I40E_AQ_FLAG_BUF | I40E_AQ_FLAG_RD));
 
-	status = i40e_asq_send_command(hw, &desc, &vsi_ctx->info,
-				    sizeof(vsi_ctx->info), cmd_details);
+	status = i40e_asq_send_command_atomic(hw, &desc, &vsi_ctx->info,
+					      sizeof(vsi_ctx->info),
+					      cmd_details, true);
 
 	if (status)
 		goto aq_add_vsi_exit;
@@ -2287,8 +2288,9 @@ i40e_status i40e_aq_update_vsi_params(struct i40e_hw *hw,
 
 	desc.flags |= cpu_to_le16((u16)(I40E_AQ_FLAG_BUF | I40E_AQ_FLAG_RD));
 
-	status = i40e_asq_send_command(hw, &desc, &vsi_ctx->info,
-				    sizeof(vsi_ctx->info), cmd_details);
+	status = i40e_asq_send_command_atomic(hw, &desc, &vsi_ctx->info,
+					      sizeof(vsi_ctx->info),
+					      cmd_details, true);
 
 	vsi_ctx->vsis_allocated = le16_to_cpu(resp->vsi_used);
 	vsi_ctx->vsis_unallocated = le16_to_cpu(resp->vsi_free);
@@ -2695,8 +2697,8 @@ i40e_aq_add_macvlan(struct i40e_hw *hw, u16 seid,
 
 	buf_size = i40e_prepare_add_macvlan(mv_list, &desc, count, seid);
 
-	status = i40e_asq_send_command(hw, &desc, mv_list, buf_size,
-				       cmd_details);
+	status = i40e_asq_send_command_atomic(hw, &desc, mv_list, buf_size,
+					      cmd_details, true);
 
 	return status;
 }
@@ -2773,8 +2775,8 @@ i40e_status i40e_aq_remove_macvlan(struct i40e_hw *hw, u16 seid,
 	if (buf_size > I40E_AQ_LARGE_BUF)
 		desc.flags |= cpu_to_le16((u16)I40E_AQ_FLAG_LB);
 
-	status = i40e_asq_send_command(hw, &desc, mv_list, buf_size,
-				       cmd_details);
+	status = i40e_asq_send_command_atomic(hw, &desc, mv_list, buf_size,
+					      cmd_details, true);
 
 	return status;
 }
@@ -3975,7 +3977,8 @@ i40e_status i40e_aq_delete_element(struct i40e_hw *hw, u16 seid,
 
 	cmd->seid = cpu_to_le16(seid);
 
-	status = i40e_asq_send_command(hw, &desc, NULL, 0, cmd_details);
+	status = i40e_asq_send_command_atomic(hw, &desc, NULL, 0,
+					      cmd_details, true);
 
 	return status;
 }
-- 
2.27.0

[Intel-wired-lan] [PATCH net-next v1 1/4] i40e: Add new versions of send ASQ command functions

[Nguyen, Anthony L]

On Mon, 2022-01-10 at 13:11 +0000, Jedrzej Jagielski wrote:
> ASQ send command functions are returning only i40e status codes
> yet some calling functions also need Admin Queue status
> that is stored in hw->aq.asq_last_status. Since hw object
> is stored on a heap it introduces a possibility for
> a race condition in access to hw if calling function is not
> fast enough to read hw->aq.asq_last_status before next
> send ASQ command is executed.
> 
> Add new versions of send ASQ command functions that return
> Admin Queue status on the stack to avoid race conditions
> in access to hw->aq.asq_last_status.
> Add new _v2 version of i40e_aq_remove_macvlan that is using
> new _v2 versions of ASQ send command functions and returns
> the Admin Queue status on the stack.
> 
> Signed-off-by: Sylwester Dziedziuch <sylwesterx.dziedziuch@intel.com>
> Signed-off-by: Jedrzej Jagielski <jedrzej.jagielski@intel.com>
> ---
> ?drivers/net/ethernet/intel/i40e/i40e_adminq.c | 92
> +++++++++++++++++--
> ?drivers/net/ethernet/intel/i40e/i40e_common.c | 49 ++++++++++
> ?.../net/ethernet/intel/i40e/i40e_prototype.h? | 20 ++++
> ?3 files changed, 153 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_adminq.c
> b/drivers/net/ethernet/intel/i40e/i40e_adminq.c
> index 7abef88801fb..10db5180d518 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_adminq.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_adminq.c
> @@ -769,7 +769,7 @@ static bool i40e_asq_done(struct i40e_hw *hw)
> ?}
> ?
> ?/**
> - *? i40e_asq_send_command_atomic - send command to Admin Queue
> + *? i40e_asq_send_command_atomic_exec - send command to Admin Queue
> ? *? @hw: pointer to the hw struct
> ? *? @desc: prefilled descriptor describing the command (non DMA mem)
> ? *? @buff: buffer to use for indirect commands
> @@ -780,11 +780,13 @@ static bool i40e_asq_done(struct i40e_hw *hw)
> ? *? This is the main send command driver routine for the Admin Queue
> send
> ? *? queue.? It runs the queue, cleans the queue, etc
> ? **/
> -i40e_status
> -i40e_asq_send_command_atomic(struct i40e_hw *hw, struct i40e_aq_desc
> *desc,
> -??????????????????????????? void *buff, /* can be NULL */ u16?
> buff_size,
> -??????????????????????????? struct i40e_asq_cmd_details
> *cmd_details,
> -??????????????????????????? bool is_atomic_context)
> +static i40e_status
> +i40e_asq_send_command_atomic_exec(struct i40e_hw *hw,
> +???????????????????????????????? struct i40e_aq_desc *desc,
> +???????????????????????????????? void *buff, /* can be NULL */
> +???????????????????????????????? u16? buff_size,
> +???????????????????????????????? struct i40e_asq_cmd_details
> *cmd_details,
> +???????????????????????????????? bool is_atomic_context)
> ?{
> ????????i40e_status status = 0;
> ????????struct i40e_dma_mem *dma_buff = NULL;
> @@ -794,8 +796,6 @@ i40e_asq_send_command_atomic(struct i40e_hw *hw,
> struct i40e_aq_desc *desc,
> ????????u16? retval = 0;
> ????????u32? val = 0;
> ?
> -???????mutex_lock(&hw->aq.asq_mutex);
> -
> ????????if (hw->aq.asq.count == 0) {
> ????????????????i40e_debug(hw, I40E_DEBUG_AQ_MESSAGE,
> ?????????????????????????? "AQTX: Admin queue not initialized.\n");
> @@ -969,6 +969,36 @@ i40e_asq_send_command_atomic(struct i40e_hw *hw,
> struct i40e_aq_desc *desc,
> ????????}
> ?
> ?asq_send_command_error:
> +???????return status;
> +}
> +
> +/**
> + *? i40e_asq_send_command_atomic - send command to Admin Queue
> + *? @hw: pointer to the hw struct
> + *? @desc: prefilled descriptor describing the command (non DMA mem)
> + *? @buff: buffer to use for indirect commands
> + *? @buff_size: size of buffer for indirect commands
> + *? @cmd_details: pointer to command details structure
> + *? @is_atomic_context: is the function called in an atomic context?
> + *
> + *? Acquires the lock and calls the main send command execution
> + *? routine.
> + **/
> +i40e_status
> +i40e_asq_send_command_atomic(struct i40e_hw *hw,
> +??????????????????????????? struct i40e_aq_desc *desc,
> +??????????????????????????? void *buff, /* can be NULL */
> +??????????????????????????? u16? buff_size,
> +??????????????????????????? struct i40e_asq_cmd_details
> *cmd_details,
> +??????????????????????????? bool is_atomic_context)
> +{
> +???????i40e_status status = I40E_SUCCESS;

This initialization isn't needed.

> +???????mutex_lock(&hw->aq.asq_mutex);
> +???????status = i40e_asq_send_command_atomic_exec(hw, desc, buff,
> buff_size,
> +????????????????????????????????????????????????? cmd_details,
> +?????????????????????????????????????????????????
> is_atomic_context);
> +
> ????????mutex_unlock(&hw->aq.asq_mutex);
> ????????return status;
> ?}
> @@ -982,6 +1012,52 @@ i40e_asq_send_command(struct i40e_hw *hw,
> struct i40e_aq_desc *desc,
> ??????????????????????????????????????????? cmd_details, false);
> ?}
> ?
> +/**
> + *? i40e_asq_send_command_atomic_v2 - send command to Admin Queue
> + *? @hw: pointer to the hw struct
> + *? @desc: prefilled descriptor describing the command (non DMA mem)
> + *? @buff: buffer to use for indirect commands
> + *? @buff_size: size of buffer for indirect commands
> + *? @cmd_details: pointer to command details structure
> + *? @is_atomic_context: is the function called in an atomic context?
> + *? @aq_status: pointer to Admin Queue status return value
> + *
> + *? Acquires the lock and calls the main send command execution
> + *? routine. Returns the last Admin Queue status in aq_status
> + *? to avoid race conditions in access to hw->aq.asq_last_status.
> + **/
> +i40e_status
> +i40e_asq_send_command_atomic_v2(struct i40e_hw *hw,
> +???????????????????????????????struct i40e_aq_desc *desc,
> +???????????????????????????????void *buff, /* can be NULL */
> +???????????????????????????????u16? buff_size,
> +???????????????????????????????struct i40e_asq_cmd_details
> *cmd_details,
> +???????????????????????????????bool is_atomic_context,
> +???????????????????????????????enum i40e_admin_queue_err *aq_status)
> +{
> +???????i40e_status status = I40E_SUCCESS;

This as well.

> +???????mutex_lock(&hw->aq.asq_mutex);
> +???????status = i40e_asq_send_command_atomic_exec(hw, desc, buff,
> +????????????????????????????????????????????????? buff_size,
> +????????????????????????????????????????????????? cmd_details,
> +?????????????????????????????????????????????????
> is_atomic_context);
> +???????if (aq_status)
> +???????????????*aq_status = hw->aq.asq_last_status;
> +???????mutex_unlock(&hw->aq.asq_mutex);
> +???????return status;
> +}
> +
> +inline i40e_status
> +i40e_asq_send_command_v2(struct i40e_hw *hw, struct i40e_aq_desc
> *desc,
> +??????????????????????? void *buff, /* can be NULL */ u16?
> buff_size,
> +??????????????????????? struct i40e_asq_cmd_details *cmd_details,
> +??????????????????????? enum i40e_admin_queue_err *aq_status)

Don't use inline in c files.

> +{
> +???????return i40e_asq_send_command_atomic_v2(hw, desc, buff,
> buff_size,
> +????????????????????????????????????????????? cmd_details, true,
> aq_status);
> +}
> +
> ?/**
> ? *? i40e_fill_default_direct_cmd_desc - AQ descriptor helper
> function
> ? *? @desc:???? pointer to the temp descriptor (non DMA mem)
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_common.c
> b/drivers/net/ethernet/intel/i40e/i40e_common.c
> index 9ddeb015eb7e..cdd6e5bdbe4f 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_common.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_common.c
> @@ -2721,6 +2721,55 @@ i40e_status i40e_aq_remove_macvlan(struct
> i40e_hw *hw, u16 seid,
> ????????return status;
> ?}
> ?
> +/**
> + * i40e_aq_remove_macvlan_v2
> + * @hw: pointer to the hw struct
> + * @seid: VSI for the mac address
> + * @mv_list: list of macvlans to be removed
> + * @count: length of the list
> + * @cmd_details: pointer to command details structure or NULL
> + * @aq_status: pointer to Admin Queue status return value
> + *
> + * Remove MAC/VLAN addresses from the HW filtering.
> + * The _v2 version returns the last Admin Queue status in aq_status
> + * to avoid race conditions in access to hw->aq.asq_last_status.
> + * It also calls _v2 versions of asq_send_command functions to
> + * get the aq_status on the stack.
> + **/
> +i40e_status
> +i40e_aq_remove_macvlan_v2(struct i40e_hw *hw, u16 seid,
> +???????????????????????? struct i40e_aqc_remove_macvlan_element_data
> *mv_list,
> +???????????????????????? u16 count, struct i40e_asq_cmd_details
> *cmd_details,
> +???????????????????????? enum i40e_admin_queue_err *aq_status)
> +{
> +???????struct i40e_aq_desc desc;
> +???????struct i40e_aqc_macvlan *cmd;

Not RCT.

> +???????i40e_status status;
> +???????u16 buf_size;
> +
> +???????if (count == 0 || !mv_list || !hw)
> +???????????????return I40E_ERR_PARAM;
> +
> +???????buf_size = count * sizeof(*mv_list);
> +
> +???????/* prep the rest of the request */
> +???????i40e_fill_default_direct_cmd_desc(&desc,
> i40e_aqc_opc_remove_macvlan);
> +???????cmd = (struct i40e_aqc_macvlan *)&desc.params.raw;
> +???????cmd->num_addresses = cpu_to_le16(count);
> +???????cmd->seid[0] = cpu_to_le16(I40E_AQC_MACVLAN_CMD_SEID_VALID |
> seid);
> +???????cmd->seid[1] = 0;
> +???????cmd->seid[2] = 0;
> +
> +???????desc.flags |= cpu_to_le16((u16)(I40E_AQ_FLAG_BUF |
> I40E_AQ_FLAG_RD));
> +???????if (buf_size > I40E_AQ_LARGE_BUF)
> +???????????????desc.flags |= cpu_to_le16((u16)I40E_AQ_FLAG_LB);
> +
> +???????status = i40e_asq_send_command_atomic_v2(hw, &desc, mv_list,
> buf_size,
> +??????????????????????????????????????????????? cmd_details, true,
> aq_status);

This can be returned directly, there's no need to assign it to status.
Which removes the need for the status variable.

> +
> +???????return status;
> +}
> +

[Intel-wired-lan] [PATCH net-next v1 2/4] i40e: Add new version of i40e_aq_add_macvlan function

[Nguyen, Anthony L]

On Mon, 2022-01-10 at 13:11 +0000, Jedrzej Jagielski wrote:

<snip>

> +/**
> + * i40e_aq_add_macvlan
> + * @hw: pointer to the hw struct
> + * @seid: VSI for the mac address
> + * @mv_list: list of macvlans to be added
> + * @count: length of the list
> + * @cmd_details: pointer to command details structure or NULL
> + *
> + * Add MAC/VLAN addresses to the HW filtering
> + **/
> +i40e_status
> +i40e_aq_add_macvlan(struct i40e_hw *hw, u16 seid,
> +?????????????????? struct i40e_aqc_add_macvlan_element_data
> *mv_list,
> +?????????????????? u16 count, struct i40e_asq_cmd_details
> *cmd_details)
> +{
> +???????struct i40e_aq_desc desc;
> +???????i40e_status status;
> +???????u16 buf_size;
> +
> +???????if (count == 0 || !mv_list || !hw)
> +???????????????return I40E_ERR_PARAM;
> +
> +???????buf_size = i40e_prepare_add_macvlan(mv_list, &desc, count,
> seid);
> ?
> ????????status = i40e_asq_send_command(hw, &desc, mv_list, buf_size,
> ?????????????????????????????????????? cmd_details);

Not directly part of this patch, but if you are touching this function.
This function could also be returned directly since the line directly
following this is a 'return status'.

> @@ -2679,6 +2701,42 @@ i40e_status i40e_aq_add_macvlan(struct i40e_hw
> *hw, u16 seid,
> ????????return status;
> ?}
> ?
> +/**
> + * i40e_aq_add_macvlan_v2
> + * @hw: pointer to the hw struct
> + * @seid: VSI for the mac address
> + * @mv_list: list of macvlans to be added
> + * @count: length of the list
> + * @cmd_details: pointer to command details structure or NULL
> + * @aq_status: pointer to Admin Queue status return value
> + *
> + * Add MAC/VLAN addresses to the HW filtering.
> + * The _v2 version returns the last Admin Queue status in aq_status
> + * to avoid race conditions in access to hw->aq.asq_last_status.
> + * It also calls _v2 versions of asq_send_command functions to
> + * get the aq_status on the stack.
> + **/
> +i40e_status
> +i40e_aq_add_macvlan_v2(struct i40e_hw *hw, u16 seid,
> +????????????????????? struct i40e_aqc_add_macvlan_element_data
> *mv_list,
> +????????????????????? u16 count, struct i40e_asq_cmd_details
> *cmd_details,
> +????????????????????? enum i40e_admin_queue_err *aq_status)
> +{
> +???????struct i40e_aq_desc desc;
> +???????i40e_status status;
> +???????u16 buf_size;
> +
> +???????if (count == 0 || !mv_list || !hw)
> +???????????????return I40E_ERR_PARAM;
> +
> +???????buf_size = i40e_prepare_add_macvlan(mv_list, &desc, count,
> seid);
> +
> +???????status = i40e_asq_send_command_atomic_v2(hw, &desc, mv_list,
> buf_size,
> +??????????????????????????????????????????????? cmd_details, true,
> aq_status);
> +
> +???????return status;

Same here. This function can be returned direclty and status removed.

> +}
> +