[PATCH iwl-next 0/4] ice: Read Tx timestamps in the IRQ top half

[PATCH iwl-next 0/4] ice: Read Tx timestamps in the IRQ top half

[Karol Kolacinski]

On all E82X products (E822, E823 and E825), Tx timestamps are read using
sideband queue.
This is a very robust HW queue and it's used only for accessing PHYs and
CGU, which means it can use spin_locks and delays instead of mutexes and
sleeps like slow admin queue handled by the FW.

This allows reading the sideband queue in the top half of the interrupt
and allows to avoid bottom half scheduling delays, which speeds up Tx
timestamping process significantly.

Introduce new structure ice_sq_ops, which allows to assign lock/unlock
operations based on the queue type on queue init.

Karol Kolacinski (4):
  ice: skip completion for sideband queue writes
  ice: refactor ice_sq_send_cmd and ice_shutdown_sq
  ice: use spin_lock for sideband queue send queue
  ice: read Tx timestamps in the IRQ top half

 drivers/net/ethernet/intel/ice/ice_common.c   |  21 +-
 drivers/net/ethernet/intel/ice/ice_common.h   |   3 +-
 drivers/net/ethernet/intel/ice/ice_controlq.c | 252 +++++++++++-------
 drivers/net/ethernet/intel/ice/ice_controlq.h |  20 +-
 drivers/net/ethernet/intel/ice/ice_ptp.c      |  46 ++--
 drivers/net/ethernet/intel/ice/ice_ptp_hw.c   |  62 +++--
 drivers/net/ethernet/intel/ice/ice_sbq_cmd.h  |   5 +-
 7 files changed, 251 insertions(+), 158 deletions(-)


base-commit: 82bb0098b73f72a026b4bb49206a8c1d90974edc
-- 
2.49.0

[PATCH iwl-next 1/4] ice: skip completion for sideband queue writes

[Karol Kolacinski]

Sideband queue (SBQ) is a HW queue with very short completion time. All
SBQ writes were posted by default, which means that the driver did not
have to wait for completion from the neighbor device, because there was
none. This introduced unnecessary delays, where only those delays were
"ensuring" that the command is "completed" and this was a potential race
condition.

Add the possibility to perform non-posted writes where it's necessary to
wait for completion, instead of relying on fake completion from the FW,
where only the delays are guarding the writes.

Flush the SBQ by reading address 0 from the PHY 0 before issuing SYNC
command to ensure that writes to all PHYs were completed and skip SBQ
message completion if it's posted.

Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Reviewed-by: Milena Olech <milena.olech@intel.com>
Signed-off-by: Karol Kolacinski <karol.kolacinski@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_common.c   | 13 ++--
 drivers/net/ethernet/intel/ice/ice_controlq.c |  4 ++
 drivers/net/ethernet/intel/ice/ice_controlq.h |  1 +
 drivers/net/ethernet/intel/ice/ice_ptp_hw.c   | 62 +++++++++++--------
 drivers/net/ethernet/intel/ice/ice_sbq_cmd.h  |  5 +-
 5 files changed, 52 insertions(+), 33 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_common.c b/drivers/net/ethernet/intel/ice/ice_common.c
index 11a954e8dc62..53b9b5b54187 100644
--- a/drivers/net/ethernet/intel/ice/ice_common.c
+++ b/drivers/net/ethernet/intel/ice/ice_common.c
@@ -1523,6 +1523,7 @@ int ice_sbq_rw_reg(struct ice_hw *hw, struct ice_sbq_msg_input *in, u16 flags)
 {
 	struct ice_sbq_cmd_desc desc = {0};
 	struct ice_sbq_msg_req msg = {0};
+	struct ice_sq_cd cd = {};
 	u16 msg_len;
 	int status;
 
@@ -1535,7 +1536,7 @@ int ice_sbq_rw_reg(struct ice_hw *hw, struct ice_sbq_msg_input *in, u16 flags)
 	msg.msg_addr_low = cpu_to_le16(in->msg_addr_low);
 	msg.msg_addr_high = cpu_to_le32(in->msg_addr_high);
 
-	if (in->opcode)
+	if (in->opcode == ice_sbq_msg_wr_p || in->opcode == ice_sbq_msg_wr_np)
 		msg.data = cpu_to_le32(in->data);
 	else
 		/* data read comes back in completion, so shorten the struct by
@@ -1543,10 +1544,12 @@ int ice_sbq_rw_reg(struct ice_hw *hw, struct ice_sbq_msg_input *in, u16 flags)
 		 */
 		msg_len -= sizeof(msg.data);
 
+	cd.postpone = in->opcode == ice_sbq_msg_wr_p;
+
 	desc.flags = cpu_to_le16(flags);
 	desc.opcode = cpu_to_le16(ice_sbq_opc_neigh_dev_req);
 	desc.param0.cmd_len = cpu_to_le16(msg_len);
-	status = ice_sbq_send_cmd(hw, &desc, &msg, msg_len, NULL);
+	status = ice_sbq_send_cmd(hw, &desc, &msg, msg_len, &cd);
 	if (!status && !in->opcode)
 		in->data = le32_to_cpu
 			(((struct ice_sbq_msg_cmpl *)&msg)->data);
@@ -6260,7 +6263,7 @@ int ice_read_cgu_reg(struct ice_hw *hw, u32 addr, u32 *val)
 	struct ice_sbq_msg_input cgu_msg = {
 		.opcode = ice_sbq_msg_rd,
 		.dest_dev = ice_sbq_dev_cgu,
-		.msg_addr_low = addr
+		.msg_addr_low = addr,
 	};
 	int err;
 
@@ -6290,10 +6293,10 @@ int ice_read_cgu_reg(struct ice_hw *hw, u32 addr, u32 *val)
 int ice_write_cgu_reg(struct ice_hw *hw, u32 addr, u32 val)
 {
 	struct ice_sbq_msg_input cgu_msg = {
-		.opcode = ice_sbq_msg_wr,
+		.opcode = ice_sbq_msg_wr_np,
 		.dest_dev = ice_sbq_dev_cgu,
 		.msg_addr_low = addr,
-		.data = val
+		.data = val,
 	};
 	int err;
 
diff --git a/drivers/net/ethernet/intel/ice/ice_controlq.c b/drivers/net/ethernet/intel/ice/ice_controlq.c
index dcb837cadd18..5fb3a8441beb 100644
--- a/drivers/net/ethernet/intel/ice/ice_controlq.c
+++ b/drivers/net/ethernet/intel/ice/ice_controlq.c
@@ -1086,6 +1086,10 @@ ice_sq_send_cmd(struct ice_hw *hw, struct ice_ctl_q_info *cq,
 	wr32(hw, cq->sq.tail, cq->sq.next_to_use);
 	ice_flush(hw);
 
+	/* If the message is posted, don't wait for completion. */
+	if (cd && cd->postpone)
+		goto sq_send_command_error;
+
 	/* Wait for the command to complete. If it finishes within the
 	 * timeout, copy the descriptor back to temp.
 	 */
diff --git a/drivers/net/ethernet/intel/ice/ice_controlq.h b/drivers/net/ethernet/intel/ice/ice_controlq.h
index 788040dd662e..7c98d3a0314e 100644
--- a/drivers/net/ethernet/intel/ice/ice_controlq.h
+++ b/drivers/net/ethernet/intel/ice/ice_controlq.h
@@ -77,6 +77,7 @@ struct ice_ctl_q_ring {
 /* sq transaction details */
 struct ice_sq_cd {
 	struct libie_aq_desc *wb_desc;
+	u8 postpone : 1;
 };
 
 /* rq event information */
diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
index 523f95271f35..9a4ecf1249ee 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
+++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
@@ -352,6 +352,13 @@ void ice_ptp_src_cmd(struct ice_hw *hw, enum ice_ptp_tmr_cmd cmd)
 static void ice_ptp_exec_tmr_cmd(struct ice_hw *hw)
 {
 	struct ice_pf *pf = container_of(hw, struct ice_pf, hw);
+	struct ice_sbq_msg_input msg = {
+		.dest_dev = ice_sbq_dev_phy_0,
+		.opcode = ice_sbq_msg_rd,
+	};
+
+	/* Flush SBQ by reading address 0 on PHY 0 */
+	ice_sbq_rw_reg(hw, &msg, LIBIE_AQ_FLAG_RD);
 
 	if (!ice_is_primary(hw))
 		hw = ice_get_primary_hw(pf);
@@ -417,10 +424,10 @@ static int ice_write_phy_eth56g(struct ice_hw *hw, u8 port, u32 addr, u32 val)
 {
 	struct ice_sbq_msg_input msg = {
 		.dest_dev = ice_ptp_get_dest_dev_e825(hw, port),
-		.opcode = ice_sbq_msg_wr,
+		.opcode = ice_sbq_msg_wr_p,
 		.msg_addr_low = lower_16_bits(addr),
 		.msg_addr_high = upper_16_bits(addr),
-		.data = val
+		.data = val,
 	};
 	int err;
 
@@ -2342,11 +2349,12 @@ static bool ice_is_40b_phy_reg_e82x(u16 low_addr, u16 *high_addr)
 static int
 ice_read_phy_reg_e82x(struct ice_hw *hw, u8 port, u16 offset, u32 *val)
 {
-	struct ice_sbq_msg_input msg = {0};
+	struct ice_sbq_msg_input msg = {
+		.opcode = ice_sbq_msg_rd,
+	};
 	int err;
 
 	ice_fill_phy_msg_e82x(hw, &msg, port, offset);
-	msg.opcode = ice_sbq_msg_rd;
 
 	err = ice_sbq_rw_reg(hw, &msg, LIBIE_AQ_FLAG_RD);
 	if (err) {
@@ -2419,12 +2427,13 @@ ice_read_64b_phy_reg_e82x(struct ice_hw *hw, u8 port, u16 low_addr, u64 *val)
 static int
 ice_write_phy_reg_e82x(struct ice_hw *hw, u8 port, u16 offset, u32 val)
 {
-	struct ice_sbq_msg_input msg = {0};
+	struct ice_sbq_msg_input msg = {
+		.opcode = ice_sbq_msg_wr_p,
+		.data = val,
+	};
 	int err;
 
 	ice_fill_phy_msg_e82x(hw, &msg, port, offset);
-	msg.opcode = ice_sbq_msg_wr;
-	msg.data = val;
 
 	err = ice_sbq_rw_reg(hw, &msg, LIBIE_AQ_FLAG_RD);
 	if (err) {
@@ -2578,15 +2587,15 @@ static int ice_fill_quad_msg_e82x(struct ice_hw *hw,
 int
 ice_read_quad_reg_e82x(struct ice_hw *hw, u8 quad, u16 offset, u32 *val)
 {
-	struct ice_sbq_msg_input msg = {0};
+	struct ice_sbq_msg_input msg = {
+		.opcode = ice_sbq_msg_rd,
+	};
 	int err;
 
 	err = ice_fill_quad_msg_e82x(hw, &msg, quad, offset);
 	if (err)
 		return err;
 
-	msg.opcode = ice_sbq_msg_rd;
-
 	err = ice_sbq_rw_reg(hw, &msg, LIBIE_AQ_FLAG_RD);
 	if (err) {
 		ice_debug(hw, ICE_DBG_PTP, "Failed to send message to PHY, err %d\n",
@@ -2612,16 +2621,16 @@ ice_read_quad_reg_e82x(struct ice_hw *hw, u8 quad, u16 offset, u32 *val)
 int
 ice_write_quad_reg_e82x(struct ice_hw *hw, u8 quad, u16 offset, u32 val)
 {
-	struct ice_sbq_msg_input msg = {0};
+	struct ice_sbq_msg_input msg = {
+		.opcode = ice_sbq_msg_wr_p,
+		.data = val,
+	};
 	int err;
 
 	err = ice_fill_quad_msg_e82x(hw, &msg, quad, offset);
 	if (err)
 		return err;
 
-	msg.opcode = ice_sbq_msg_wr;
-	msg.data = val;
-
 	err = ice_sbq_rw_reg(hw, &msg, LIBIE_AQ_FLAG_RD);
 	if (err) {
 		ice_debug(hw, ICE_DBG_PTP, "Failed to send message to PHY, err %d\n",
@@ -4259,13 +4268,14 @@ static void ice_ptp_init_phy_e82x(struct ice_ptp_hw *ptp)
  */
 static int ice_read_phy_reg_e810(struct ice_hw *hw, u32 addr, u32 *val)
 {
-	struct ice_sbq_msg_input msg = {0};
+	struct ice_sbq_msg_input msg = {
+		.dest_dev = ice_sbq_dev_phy_0,
+		.opcode = ice_sbq_msg_rd,
+		.msg_addr_low = lower_16_bits(addr),
+		.msg_addr_high = upper_16_bits(addr),
+	};
 	int err;
 
-	msg.msg_addr_low = lower_16_bits(addr);
-	msg.msg_addr_high = upper_16_bits(addr);
-	msg.opcode = ice_sbq_msg_rd;
-	msg.dest_dev = ice_sbq_dev_phy_0;
 
 	err = ice_sbq_rw_reg(hw, &msg, LIBIE_AQ_FLAG_RD);
 	if (err) {
@@ -4289,15 +4299,15 @@ static int ice_read_phy_reg_e810(struct ice_hw *hw, u32 addr, u32 *val)
  */
 static int ice_write_phy_reg_e810(struct ice_hw *hw, u32 addr, u32 val)
 {
-	struct ice_sbq_msg_input msg = {0};
+	struct ice_sbq_msg_input msg = {
+		.dest_dev = ice_sbq_dev_phy_0,
+		.opcode = ice_sbq_msg_wr_p,
+		.msg_addr_low = lower_16_bits(addr),
+		.msg_addr_high = upper_16_bits(addr),
+		.data = val,
+	};
 	int err;
 
-	msg.msg_addr_low = lower_16_bits(addr);
-	msg.msg_addr_high = upper_16_bits(addr);
-	msg.opcode = ice_sbq_msg_wr;
-	msg.dest_dev = ice_sbq_dev_phy_0;
-	msg.data = val;
-
 	err = ice_sbq_rw_reg(hw, &msg, LIBIE_AQ_FLAG_RD);
 	if (err) {
 		ice_debug(hw, ICE_DBG_PTP, "Failed to send message to PHY, err %d\n",
diff --git a/drivers/net/ethernet/intel/ice/ice_sbq_cmd.h b/drivers/net/ethernet/intel/ice/ice_sbq_cmd.h
index 183dd5457d6a..7960f888a655 100644
--- a/drivers/net/ethernet/intel/ice/ice_sbq_cmd.h
+++ b/drivers/net/ethernet/intel/ice/ice_sbq_cmd.h
@@ -53,8 +53,9 @@ enum ice_sbq_dev_id {
 };
 
 enum ice_sbq_msg_opcode {
-	ice_sbq_msg_rd	= 0x00,
-	ice_sbq_msg_wr	= 0x01
+	ice_sbq_msg_rd		= 0x00,
+	ice_sbq_msg_wr_p	= 0x01,
+	ice_sbq_msg_wr_np	= 0x02,
 };
 
 #define ICE_SBQ_MSG_FLAGS	0x40
-- 
2.49.0

Re: [Intel-wired-lan] [PATCH iwl-next 1/4] ice: skip completion for sideband queue writes

[Paul Menzel]

Dear Karol,


Thank you for your patch.

Am 20.05.25 um 13:06 schrieb Karol Kolacinski:
> Sideband queue (SBQ) is a HW queue with very short completion time. All
> SBQ writes were posted by default, which means that the driver did not
> have to wait for completion from the neighbor device, because there was
> none. This introduced unnecessary delays, where only those delays were
> "ensuring" that the command is "completed" and this was a potential race
> condition.
> 
> Add the possibility to perform non-posted writes where it's necessary to
> wait for completion, instead of relying on fake completion from the FW,
> where only the delays are guarding the writes.
> 
> Flush the SBQ by reading address 0 from the PHY 0 before issuing SYNC
> command to ensure that writes to all PHYs were completed and skip SBQ
> message completion if it's posted.

How can these delays be analyzed, and verified that these are gone? It’d 
be great if you added that to the commit mesasge.

> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> Reviewed-by: Milena Olech <milena.olech@intel.com>
> Signed-off-by: Karol Kolacinski <karol.kolacinski@intel.com>
> ---
>   drivers/net/ethernet/intel/ice/ice_common.c   | 13 ++--
>   drivers/net/ethernet/intel/ice/ice_controlq.c |  4 ++
>   drivers/net/ethernet/intel/ice/ice_controlq.h |  1 +
>   drivers/net/ethernet/intel/ice/ice_ptp_hw.c   | 62 +++++++++++--------
>   drivers/net/ethernet/intel/ice/ice_sbq_cmd.h  |  5 +-
>   5 files changed, 52 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_common.c b/drivers/net/ethernet/intel/ice/ice_common.c
> index 11a954e8dc62..53b9b5b54187 100644
> --- a/drivers/net/ethernet/intel/ice/ice_common.c
> +++ b/drivers/net/ethernet/intel/ice/ice_common.c
> @@ -1523,6 +1523,7 @@ int ice_sbq_rw_reg(struct ice_hw *hw, struct ice_sbq_msg_input *in, u16 flags)
>   {
>   	struct ice_sbq_cmd_desc desc = {0};
>   	struct ice_sbq_msg_req msg = {0};
> +	struct ice_sq_cd cd = {};
>   	u16 msg_len;
>   	int status;
>   
> @@ -1535,7 +1536,7 @@ int ice_sbq_rw_reg(struct ice_hw *hw, struct ice_sbq_msg_input *in, u16 flags)
>   	msg.msg_addr_low = cpu_to_le16(in->msg_addr_low);
>   	msg.msg_addr_high = cpu_to_le32(in->msg_addr_high);
>   
> -	if (in->opcode)
> +	if (in->opcode == ice_sbq_msg_wr_p || in->opcode == ice_sbq_msg_wr_np)

For me, it’d be helpful, if the suffixes were `_posted` and `_nonposted`.


Kind regards,

Paul


>   		msg.data = cpu_to_le32(in->data);
>   	else
>   		/* data read comes back in completion, so shorten the struct by
> @@ -1543,10 +1544,12 @@ int ice_sbq_rw_reg(struct ice_hw *hw, struct ice_sbq_msg_input *in, u16 flags)
>   		 */
>   		msg_len -= sizeof(msg.data);
>   
> +	cd.postpone = in->opcode == ice_sbq_msg_wr_p;
> +
>   	desc.flags = cpu_to_le16(flags);
>   	desc.opcode = cpu_to_le16(ice_sbq_opc_neigh_dev_req);
>   	desc.param0.cmd_len = cpu_to_le16(msg_len);
> -	status = ice_sbq_send_cmd(hw, &desc, &msg, msg_len, NULL);
> +	status = ice_sbq_send_cmd(hw, &desc, &msg, msg_len, &cd);
>   	if (!status && !in->opcode)
>   		in->data = le32_to_cpu
>   			(((struct ice_sbq_msg_cmpl *)&msg)->data);
> @@ -6260,7 +6263,7 @@ int ice_read_cgu_reg(struct ice_hw *hw, u32 addr, u32 *val)
>   	struct ice_sbq_msg_input cgu_msg = {
>   		.opcode = ice_sbq_msg_rd,
>   		.dest_dev = ice_sbq_dev_cgu,
> -		.msg_addr_low = addr
> +		.msg_addr_low = addr,
>   	};
>   	int err;
>   
> @@ -6290,10 +6293,10 @@ int ice_read_cgu_reg(struct ice_hw *hw, u32 addr, u32 *val)
>   int ice_write_cgu_reg(struct ice_hw *hw, u32 addr, u32 val)
>   {
>   	struct ice_sbq_msg_input cgu_msg = {
> -		.opcode = ice_sbq_msg_wr,
> +		.opcode = ice_sbq_msg_wr_np,
>   		.dest_dev = ice_sbq_dev_cgu,
>   		.msg_addr_low = addr,
> -		.data = val
> +		.data = val,
>   	};
>   	int err;
>   
> diff --git a/drivers/net/ethernet/intel/ice/ice_controlq.c b/drivers/net/ethernet/intel/ice/ice_controlq.c
> index dcb837cadd18..5fb3a8441beb 100644
> --- a/drivers/net/ethernet/intel/ice/ice_controlq.c
> +++ b/drivers/net/ethernet/intel/ice/ice_controlq.c
> @@ -1086,6 +1086,10 @@ ice_sq_send_cmd(struct ice_hw *hw, struct ice_ctl_q_info *cq,
>   	wr32(hw, cq->sq.tail, cq->sq.next_to_use);
>   	ice_flush(hw);
>   
> +	/* If the message is posted, don't wait for completion. */
> +	if (cd && cd->postpone)
> +		goto sq_send_command_error;
> +
>   	/* Wait for the command to complete. If it finishes within the
>   	 * timeout, copy the descriptor back to temp.
>   	 */
> diff --git a/drivers/net/ethernet/intel/ice/ice_controlq.h b/drivers/net/ethernet/intel/ice/ice_controlq.h
> index 788040dd662e..7c98d3a0314e 100644
> --- a/drivers/net/ethernet/intel/ice/ice_controlq.h
> +++ b/drivers/net/ethernet/intel/ice/ice_controlq.h
> @@ -77,6 +77,7 @@ struct ice_ctl_q_ring {
>   /* sq transaction details */
>   struct ice_sq_cd {
>   	struct libie_aq_desc *wb_desc;
> +	u8 postpone : 1;
>   };
>   
>   /* rq event information */
> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
> index 523f95271f35..9a4ecf1249ee 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
> @@ -352,6 +352,13 @@ void ice_ptp_src_cmd(struct ice_hw *hw, enum ice_ptp_tmr_cmd cmd)
>   static void ice_ptp_exec_tmr_cmd(struct ice_hw *hw)
>   {
>   	struct ice_pf *pf = container_of(hw, struct ice_pf, hw);
> +	struct ice_sbq_msg_input msg = {
> +		.dest_dev = ice_sbq_dev_phy_0,
> +		.opcode = ice_sbq_msg_rd,
> +	};
> +
> +	/* Flush SBQ by reading address 0 on PHY 0 */
> +	ice_sbq_rw_reg(hw, &msg, LIBIE_AQ_FLAG_RD);
>   
>   	if (!ice_is_primary(hw))
>   		hw = ice_get_primary_hw(pf);
> @@ -417,10 +424,10 @@ static int ice_write_phy_eth56g(struct ice_hw *hw, u8 port, u32 addr, u32 val)
>   {
>   	struct ice_sbq_msg_input msg = {
>   		.dest_dev = ice_ptp_get_dest_dev_e825(hw, port),
> -		.opcode = ice_sbq_msg_wr,
> +		.opcode = ice_sbq_msg_wr_p,
>   		.msg_addr_low = lower_16_bits(addr),
>   		.msg_addr_high = upper_16_bits(addr),
> -		.data = val
> +		.data = val,
>   	};
>   	int err;
>   
> @@ -2342,11 +2349,12 @@ static bool ice_is_40b_phy_reg_e82x(u16 low_addr, u16 *high_addr)
>   static int
>   ice_read_phy_reg_e82x(struct ice_hw *hw, u8 port, u16 offset, u32 *val)
>   {
> -	struct ice_sbq_msg_input msg = {0};
> +	struct ice_sbq_msg_input msg = {
> +		.opcode = ice_sbq_msg_rd,
> +	};
>   	int err;
>   
>   	ice_fill_phy_msg_e82x(hw, &msg, port, offset);
> -	msg.opcode = ice_sbq_msg_rd;
>   
>   	err = ice_sbq_rw_reg(hw, &msg, LIBIE_AQ_FLAG_RD);
>   	if (err) {
> @@ -2419,12 +2427,13 @@ ice_read_64b_phy_reg_e82x(struct ice_hw *hw, u8 port, u16 low_addr, u64 *val)
>   static int
>   ice_write_phy_reg_e82x(struct ice_hw *hw, u8 port, u16 offset, u32 val)
>   {
> -	struct ice_sbq_msg_input msg = {0};
> +	struct ice_sbq_msg_input msg = {
> +		.opcode = ice_sbq_msg_wr_p,
> +		.data = val,
> +	};
>   	int err;
>   
>   	ice_fill_phy_msg_e82x(hw, &msg, port, offset);
> -	msg.opcode = ice_sbq_msg_wr;
> -	msg.data = val;
>   
>   	err = ice_sbq_rw_reg(hw, &msg, LIBIE_AQ_FLAG_RD);
>   	if (err) {
> @@ -2578,15 +2587,15 @@ static int ice_fill_quad_msg_e82x(struct ice_hw *hw,
>   int
>   ice_read_quad_reg_e82x(struct ice_hw *hw, u8 quad, u16 offset, u32 *val)
>   {
> -	struct ice_sbq_msg_input msg = {0};
> +	struct ice_sbq_msg_input msg = {
> +		.opcode = ice_sbq_msg_rd,
> +	};
>   	int err;
>   
>   	err = ice_fill_quad_msg_e82x(hw, &msg, quad, offset);
>   	if (err)
>   		return err;
>   
> -	msg.opcode = ice_sbq_msg_rd;
> -
>   	err = ice_sbq_rw_reg(hw, &msg, LIBIE_AQ_FLAG_RD);
>   	if (err) {
>   		ice_debug(hw, ICE_DBG_PTP, "Failed to send message to PHY, err %d\n",
> @@ -2612,16 +2621,16 @@ ice_read_quad_reg_e82x(struct ice_hw *hw, u8 quad, u16 offset, u32 *val)
>   int
>   ice_write_quad_reg_e82x(struct ice_hw *hw, u8 quad, u16 offset, u32 val)
>   {
> -	struct ice_sbq_msg_input msg = {0};
> +	struct ice_sbq_msg_input msg = {
> +		.opcode = ice_sbq_msg_wr_p,
> +		.data = val,
> +	};
>   	int err;
>   
>   	err = ice_fill_quad_msg_e82x(hw, &msg, quad, offset);
>   	if (err)
>   		return err;
>   
> -	msg.opcode = ice_sbq_msg_wr;
> -	msg.data = val;
> -
>   	err = ice_sbq_rw_reg(hw, &msg, LIBIE_AQ_FLAG_RD);
>   	if (err) {
>   		ice_debug(hw, ICE_DBG_PTP, "Failed to send message to PHY, err %d\n",
> @@ -4259,13 +4268,14 @@ static void ice_ptp_init_phy_e82x(struct ice_ptp_hw *ptp)
>    */
>   static int ice_read_phy_reg_e810(struct ice_hw *hw, u32 addr, u32 *val)
>   {
> -	struct ice_sbq_msg_input msg = {0};
> +	struct ice_sbq_msg_input msg = {
> +		.dest_dev = ice_sbq_dev_phy_0,
> +		.opcode = ice_sbq_msg_rd,
> +		.msg_addr_low = lower_16_bits(addr),
> +		.msg_addr_high = upper_16_bits(addr),
> +	};
>   	int err;
>   
> -	msg.msg_addr_low = lower_16_bits(addr);
> -	msg.msg_addr_high = upper_16_bits(addr);
> -	msg.opcode = ice_sbq_msg_rd;
> -	msg.dest_dev = ice_sbq_dev_phy_0;
>   
>   	err = ice_sbq_rw_reg(hw, &msg, LIBIE_AQ_FLAG_RD);
>   	if (err) {
> @@ -4289,15 +4299,15 @@ static int ice_read_phy_reg_e810(struct ice_hw *hw, u32 addr, u32 *val)
>    */
>   static int ice_write_phy_reg_e810(struct ice_hw *hw, u32 addr, u32 val)
>   {
> -	struct ice_sbq_msg_input msg = {0};
> +	struct ice_sbq_msg_input msg = {
> +		.dest_dev = ice_sbq_dev_phy_0,
> +		.opcode = ice_sbq_msg_wr_p,
> +		.msg_addr_low = lower_16_bits(addr),
> +		.msg_addr_high = upper_16_bits(addr),
> +		.data = val,
> +	};
>   	int err;
>   
> -	msg.msg_addr_low = lower_16_bits(addr);
> -	msg.msg_addr_high = upper_16_bits(addr);
> -	msg.opcode = ice_sbq_msg_wr;
> -	msg.dest_dev = ice_sbq_dev_phy_0;
> -	msg.data = val;
> -
>   	err = ice_sbq_rw_reg(hw, &msg, LIBIE_AQ_FLAG_RD);
>   	if (err) {
>   		ice_debug(hw, ICE_DBG_PTP, "Failed to send message to PHY, err %d\n",
> diff --git a/drivers/net/ethernet/intel/ice/ice_sbq_cmd.h b/drivers/net/ethernet/intel/ice/ice_sbq_cmd.h
> index 183dd5457d6a..7960f888a655 100644
> --- a/drivers/net/ethernet/intel/ice/ice_sbq_cmd.h
> +++ b/drivers/net/ethernet/intel/ice/ice_sbq_cmd.h
> @@ -53,8 +53,9 @@ enum ice_sbq_dev_id {
>   };
>   
>   enum ice_sbq_msg_opcode {
> -	ice_sbq_msg_rd	= 0x00,
> -	ice_sbq_msg_wr	= 0x01
> +	ice_sbq_msg_rd		= 0x00,
> +	ice_sbq_msg_wr_p	= 0x01,
> +	ice_sbq_msg_wr_np	= 0x02,
>   };
>   
>   #define ICE_SBQ_MSG_FLAGS	0x40

Re: [PATCH iwl-next 1/4] ice: skip completion for sideband queue writes

[Simon Horman]

On Tue, May 20, 2025 at 01:06:26PM +0200, Karol Kolacinski wrote:
> Sideband queue (SBQ) is a HW queue with very short completion time. All
> SBQ writes were posted by default, which means that the driver did not
> have to wait for completion from the neighbor device, because there was
> none. This introduced unnecessary delays, where only those delays were
> "ensuring" that the command is "completed" and this was a potential race
> condition.
> 
> Add the possibility to perform non-posted writes where it's necessary to
> wait for completion, instead of relying on fake completion from the FW,
> where only the delays are guarding the writes.
> 
> Flush the SBQ by reading address 0 from the PHY 0 before issuing SYNC
> command to ensure that writes to all PHYs were completed and skip SBQ
> message completion if it's posted.
> 
> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> Reviewed-by: Milena Olech <milena.olech@intel.com>
> Signed-off-by: Karol Kolacinski <karol.kolacinski@intel.com>

Reviewed-by: Simon Horman <horms@kernel.org>

[PATCH iwl-next 2/4] ice: refactor ice_sq_send_cmd and ice_shutdown_sq

[Karol Kolacinski]

Refactor ice_sq_send_cmd() and ice_shutdown_sq() to be able to use
a simpler locking, e.g. for new methods, which depend on the control
queue.

Reviewed-by: Milena Olech <milena.olech@intel.com>
Signed-off-by: Karol Kolacinski <karol.kolacinski@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_controlq.c | 155 ++++++++----------
 1 file changed, 72 insertions(+), 83 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_controlq.c b/drivers/net/ethernet/intel/ice/ice_controlq.c
index 5fb3a8441beb..fb7e1218797c 100644
--- a/drivers/net/ethernet/intel/ice/ice_controlq.c
+++ b/drivers/net/ethernet/intel/ice/ice_controlq.c
@@ -456,35 +456,34 @@ static int ice_init_rq(struct ice_hw *hw, struct ice_ctl_q_info *cq)
  * @hw: pointer to the hardware structure
  * @cq: pointer to the specific Control queue
  *
- * The main shutdown routine for the Control Transmit Queue
+ * The main shutdown routine for the Control Transmit Queue.
+ *
+ * Return:
+ * * %0      - success
+ * * %-EBUSY - no send queue descriptors
  */
 static int ice_shutdown_sq(struct ice_hw *hw, struct ice_ctl_q_info *cq)
 {
-	int ret_code = 0;
+	if (!cq->sq.count)
+		return -EBUSY;
 
 	mutex_lock(&cq->sq_lock);
 
-	if (!cq->sq.count) {
-		ret_code = -EBUSY;
-		goto shutdown_sq_out;
-	}
-
 	/* Stop processing of the control queue */
 	wr32(hw, cq->sq.head, 0);
 	wr32(hw, cq->sq.tail, 0);
 	wr32(hw, cq->sq.len, 0);
 	wr32(hw, cq->sq.bal, 0);
 	wr32(hw, cq->sq.bah, 0);
-
 	cq->sq.count = 0;	/* to indicate uninitialized queue */
 
+	mutex_unlock(&cq->sq_lock);
+
 	/* free ring buffers and the ring itself */
 	ICE_FREE_CQ_BUFS(hw, cq, sq);
 	ice_free_cq_ring(hw, &cq->sq);
 
-shutdown_sq_out:
-	mutex_unlock(&cq->sq_lock);
-	return ret_code;
+	return 0;
 }
 
 /**
@@ -990,56 +989,55 @@ static bool ice_sq_done(struct ice_hw *hw, struct ice_ctl_q_info *cq)
  * Main command for the transmit side of a control queue. It puts the command
  * on the queue, bumps the tail, waits for processing of the command, captures
  * command status and results, etc.
+ *
+ * Return:
+ * * %0       - success
+ * * %-EIO    - incorrect control send queue state, timeout or FW error
+ * * %-EINVAL - incorrect arguments
+ * * %-ENOSPC - control send queue is full
  */
-int
-ice_sq_send_cmd(struct ice_hw *hw, struct ice_ctl_q_info *cq,
-		struct libie_aq_desc *desc, void *buf, u16 buf_size,
-		struct ice_sq_cd *cd)
+int ice_sq_send_cmd(struct ice_hw *hw, struct ice_ctl_q_info *cq,
+		    struct libie_aq_desc *desc, void *buf, u16 buf_size,
+		    struct ice_sq_cd *cd)
 {
-	struct ice_dma_mem *dma_buf = NULL;
 	struct libie_aq_desc *desc_on_ring;
-	bool cmd_completed = false;
-	int status = 0;
-	u16 retval = 0;
-	u32 val = 0;
-
-	/* if reset is in progress return a soft error */
+	struct ice_dma_mem *dma_buf;
+	int err = 0;
+	u32 val;
+	/* If reset is in progress return a soft error. */
 	if (hw->reset_ongoing)
 		return -EBUSY;
-	mutex_lock(&cq->sq_lock);
 
+	if (!buf && buf_size)
+		return -EINVAL;
+
+	mutex_lock(&cq->sq_lock);
 	cq->sq_last_status = LIBIE_AQ_RC_OK;
 
 	if (!cq->sq.count) {
 		ice_debug(hw, ICE_DBG_AQ_MSG, "Control Send queue not initialized.\n");
-		status = -EIO;
-		goto sq_send_command_error;
-	}
-
-	if ((buf && !buf_size) || (!buf && buf_size)) {
-		status = -EINVAL;
-		goto sq_send_command_error;
+		err = -EIO;
+		goto err;
 	}
 
 	if (buf) {
-		if (buf_size > cq->sq_buf_size) {
+		if (!buf_size || buf_size > cq->sq_buf_size) {
 			ice_debug(hw, ICE_DBG_AQ_MSG, "Invalid buffer size for Control Send queue: %d.\n",
 				  buf_size);
-			status = -EINVAL;
-			goto sq_send_command_error;
+			err = -EINVAL;
+			goto err;
 		}
 
 		desc->flags |= cpu_to_le16(LIBIE_AQ_FLAG_BUF);
 		if (buf_size > LIBIE_AQ_LG_BUF)
 			desc->flags |= cpu_to_le16(LIBIE_AQ_FLAG_LB);
 	}
-
 	val = rd32(hw, cq->sq.head);
 	if (val >= cq->num_sq_entries) {
 		ice_debug(hw, ICE_DBG_AQ_MSG, "head overrun at %d in the Control Send Queue ring\n",
 			  val);
-		status = -EIO;
-		goto sq_send_command_error;
+		err = -EIO;
+		goto err;
 	}
 
 	/* Call clean and check queue available function to reclaim the
@@ -1049,25 +1047,23 @@ ice_sq_send_cmd(struct ice_hw *hw, struct ice_ctl_q_info *cq,
 	 */
 	if (ice_clean_sq(hw, cq) == 0) {
 		ice_debug(hw, ICE_DBG_AQ_MSG, "Error: Control Send Queue is full.\n");
-		status = -ENOSPC;
-		goto sq_send_command_error;
+		err = -ENOSPC;
+		goto err;
 	}
 
-	/* initialize the temp desc pointer with the right desc */
+	/* Initialize the desc_on_ring with the right descriptor. */
 	desc_on_ring = ICE_CTL_Q_DESC(cq->sq, cq->sq.next_to_use);
-
-	/* if the desc is available copy the temp desc to the right place */
 	memcpy(desc_on_ring, desc, sizeof(*desc_on_ring));
 
-	/* if buf is not NULL assume indirect command */
+	/* If buf is not NULL, assume indirect command. */
 	if (buf) {
 		dma_buf = &cq->sq.r.sq_bi[cq->sq.next_to_use];
-		/* copy the user buf into the respective DMA buf */
+		/* Copy the user buf into the respective DMA buf. */
 		memcpy(dma_buf->va, buf, buf_size);
 		desc_on_ring->datalen = cpu_to_le16(buf_size);
 
-		/* Update the address values in the desc with the pa value
-		 * for respective buffer
+		/* Update the address values in the desc with the pa value for
+		 * respective buffer.
 		 */
 		desc_on_ring->params.generic.addr_high =
 			cpu_to_le32(upper_32_bits(dma_buf->pa));
@@ -1075,9 +1071,7 @@ ice_sq_send_cmd(struct ice_hw *hw, struct ice_ctl_q_info *cq,
 			cpu_to_le32(lower_32_bits(dma_buf->pa));
 	}
 
-	/* Debug desc and buffer */
 	ice_debug(hw, ICE_DBG_AQ_DESC, "ATQ: Control Send queue desc and buffer:\n");
-
 	ice_debug_cq(hw, cq, (void *)desc_on_ring, buf, buf_size, false);
 
 	(cq->sq.next_to_use)++;
@@ -1086,65 +1080,60 @@ ice_sq_send_cmd(struct ice_hw *hw, struct ice_ctl_q_info *cq,
 	wr32(hw, cq->sq.tail, cq->sq.next_to_use);
 	ice_flush(hw);
 
-	/* If the message is posted, don't wait for completion. */
-	if (cd && cd->postpone)
-		goto sq_send_command_error;
-
-	/* Wait for the command to complete. If it finishes within the
-	 * timeout, copy the descriptor back to temp.
-	 */
-	if (ice_sq_done(hw, cq)) {
+	if (cd && cd->postpone) {
+		/* If the message is postponed, don't wait for completion. */
+		ice_debug(hw, ICE_DBG_AQ_MSG, "ATQ: Skipping completion\n");
+	} else if (ice_sq_done(hw, cq)) {
+		/* Wait for the command to complete. If it finishes within
+		 * the timeout, copy the descriptor back to temp.
+		 */
 		memcpy(desc, desc_on_ring, sizeof(*desc));
 		if (buf) {
-			/* get returned length to copy */
+			/* Get returned length to copy. */
 			u16 copy_size = le16_to_cpu(desc->datalen);
 
 			if (copy_size > buf_size) {
 				ice_debug(hw, ICE_DBG_AQ_MSG, "Return len %d > than buf len %d\n",
 					  copy_size, buf_size);
-				status = -EIO;
+				err = -EIO;
 			} else {
 				memcpy(buf, dma_buf->va, copy_size);
 			}
 		}
-		retval = le16_to_cpu(desc->retval);
-		if (retval) {
+
+		/* Strip off FW internal code. */
+		cq->sq_last_status =
+			(enum libie_aq_err)(le16_to_cpu(desc->retval) & 0xFF);
+		if (cq->sq_last_status) {
 			ice_debug(hw, ICE_DBG_AQ_MSG, "Control Send Queue command 0x%04X completed with error 0x%X\n",
 				  le16_to_cpu(desc->opcode),
-				  retval);
-
-			/* strip off FW internal code */
-			retval &= 0xff;
+				  cq->sq_last_status);
+			err = -EIO;
 		}
-		cmd_completed = true;
-		if (!status && retval != LIBIE_AQ_RC_OK)
-			status = -EIO;
-		cq->sq_last_status = (enum libie_aq_err)retval;
-	}
-
-	ice_debug(hw, ICE_DBG_AQ_MSG, "ATQ: desc and buffer writeback:\n");
 
-	ice_debug_cq(hw, cq, (void *)desc, buf, buf_size, true);
+		if (err)
+			goto err;
 
-	/* save writeback AQ if requested */
-	if (cd && cd->wb_desc)
-		memcpy(cd->wb_desc, desc_on_ring, sizeof(*cd->wb_desc));
+		ice_debug(hw, ICE_DBG_AQ_MSG, "ATQ: desc and buffer writeback:\n");
+		ice_debug_cq(hw, cq, (void *)desc, buf, buf_size, true);
 
-	/* update the error if time out occurred */
-	if (!cmd_completed) {
+		/* Save writeback AQ if requested. */
+		if (cd && cd->wb_desc)
+			memcpy(cd->wb_desc, desc_on_ring, sizeof(*cd->wb_desc));
+	} else {
+		/* Update the error if timeout occurred. */
 		if (rd32(hw, cq->rq.len) & cq->rq.len_crit_mask ||
-		    rd32(hw, cq->sq.len) & cq->sq.len_crit_mask) {
+		    rd32(hw, cq->sq.len) & cq->sq.len_crit_mask)
 			ice_debug(hw, ICE_DBG_AQ_MSG, "Critical FW error.\n");
-			status = -EIO;
-		} else {
+		else
 			ice_debug(hw, ICE_DBG_AQ_MSG, "Control Send Queue Writeback timeout.\n");
-			status = -EIO;
-		}
+
+		err = -EIO;
 	}
 
-sq_send_command_error:
+err:
 	mutex_unlock(&cq->sq_lock);
-	return status;
+	return err;
 }
 
 /**
-- 
2.49.0

Re: [PATCH iwl-next 2/4] ice: refactor ice_sq_send_cmd and ice_shutdown_sq

[Simon Horman]

On Tue, May 20, 2025 at 01:06:27PM +0200, Karol Kolacinski wrote:
> Refactor ice_sq_send_cmd() and ice_shutdown_sq() to be able to use
> a simpler locking, e.g. for new methods, which depend on the control
> queue.
> 
> Reviewed-by: Milena Olech <milena.olech@intel.com>
> Signed-off-by: Karol Kolacinski <karol.kolacinski@intel.com>

Reviewed-by: Simon Horman <horms@kernel.org>

[PATCH iwl-next 3/4] ice: use spin_lock for sideband queue send queue

[Karol Kolacinski]

Sideband queue is a HW queue and has much faster completion time than
other queues.

With <5 us for read on average it is possible to use spin_lock to be
able to read/write sideband queue messages in the interrupt top half.

Add send queue lock/unlock operations and assign them based on the queue
type. Use ice_sq_spin_lock/unlock for sideband queue and
ice_sq_mutex_lock/unlock for other queues.

Reviewed-by: Milena Olech <milena.olech@intel.com>
Signed-off-by: Karol Kolacinski <karol.kolacinski@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_common.c   |   8 +-
 drivers/net/ethernet/intel/ice/ice_common.h   |   3 +-
 drivers/net/ethernet/intel/ice/ice_controlq.c | 105 +++++++++++++++---
 drivers/net/ethernet/intel/ice/ice_controlq.h |  19 +++-
 4 files changed, 111 insertions(+), 24 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_common.c b/drivers/net/ethernet/intel/ice/ice_common.c
index 53b9b5b54187..058c93f6429b 100644
--- a/drivers/net/ethernet/intel/ice/ice_common.c
+++ b/drivers/net/ethernet/intel/ice/ice_common.c
@@ -1,5 +1,5 @@
 // SPDX-License-Identifier: GPL-2.0
-/* Copyright (c) 2018-2023, Intel Corporation. */
+/* Copyright (c) 2018-2025, Intel Corporation. */
 
 #include "ice_common.h"
 #include "ice_sched.h"
@@ -1627,8 +1627,10 @@ ice_sq_send_cmd_retry(struct ice_hw *hw, struct ice_ctl_q_info *cq,
 
 		memcpy(desc, &desc_cpy, sizeof(desc_cpy));
 
-		msleep(ICE_SQ_SEND_DELAY_TIME_MS);
-
+		if (cq->qtype == ICE_CTL_Q_SB)
+			udelay(ICE_CTL_Q_SQ_CMD_TIMEOUT_SPIN);
+		else
+			fsleep(ICE_CTL_Q_SQ_CMD_TIMEOUT);
 	} while (++idx < ICE_SQ_SEND_MAX_EXECUTE);
 
 	return status;
diff --git a/drivers/net/ethernet/intel/ice/ice_common.h b/drivers/net/ethernet/intel/ice/ice_common.h
index 3f74570b99bf..b1c766cb4ec5 100644
--- a/drivers/net/ethernet/intel/ice/ice_common.h
+++ b/drivers/net/ethernet/intel/ice/ice_common.h
@@ -1,5 +1,5 @@
 /* SPDX-License-Identifier: GPL-2.0 */
-/* Copyright (c) 2018, Intel Corporation. */
+/* Copyright (c) 2018-2025, Intel Corporation. */
 
 #ifndef _ICE_COMMON_H_
 #define _ICE_COMMON_H_
@@ -15,6 +15,7 @@
 #include "ice_switch.h"
 #include "ice_fdir.h"
 
+#define ICE_SQ_SEND_ATOMIC_DELAY_TIME_US 100
 #define ICE_SQ_SEND_DELAY_TIME_MS	10
 #define ICE_SQ_SEND_MAX_EXECUTE		3
 
diff --git a/drivers/net/ethernet/intel/ice/ice_controlq.c b/drivers/net/ethernet/intel/ice/ice_controlq.c
index fb7e1218797c..b873a9fb3f0b 100644
--- a/drivers/net/ethernet/intel/ice/ice_controlq.c
+++ b/drivers/net/ethernet/intel/ice/ice_controlq.c
@@ -1,5 +1,5 @@
 // SPDX-License-Identifier: GPL-2.0
-/* Copyright (c) 2018, Intel Corporation. */
+/* Copyright (c) 2018-2025, Intel Corporation. */
 
 #include "ice_common.h"
 
@@ -467,7 +467,7 @@ static int ice_shutdown_sq(struct ice_hw *hw, struct ice_ctl_q_info *cq)
 	if (!cq->sq.count)
 		return -EBUSY;
 
-	mutex_lock(&cq->sq_lock);
+	cq->sq_ops.lock(&cq->sq_lock);
 
 	/* Stop processing of the control queue */
 	wr32(hw, cq->sq.head, 0);
@@ -477,7 +477,7 @@ static int ice_shutdown_sq(struct ice_hw *hw, struct ice_ctl_q_info *cq)
 	wr32(hw, cq->sq.bah, 0);
 	cq->sq.count = 0;	/* to indicate uninitialized queue */
 
-	mutex_unlock(&cq->sq_lock);
+	cq->sq_ops.unlock(&cq->sq_lock);
 
 	/* free ring buffers and the ring itself */
 	ICE_FREE_CQ_BUFS(hw, cq, sq);
@@ -776,15 +776,75 @@ int ice_init_all_ctrlq(struct ice_hw *hw)
 	return ice_init_ctrlq(hw, ICE_CTL_Q_MAILBOX);
 }
 
+/**
+ * ice_sq_spin_lock - Call spin_lock_irqsave for union ice_sq_lock
+ * @lock: lock handle
+ */
+static void ice_sq_spin_lock(union ice_sq_lock *lock)
+	__acquires(&lock->sq_spinlock)
+{
+	spin_lock_irqsave(&lock->sq_spinlock, lock->sq_flags);
+}
+
+/**
+ * ice_sq_spin_unlock - Call spin_unlock_irqrestore for union ice_sq_lock
+ * @lock: lock handle
+ */
+static void ice_sq_spin_unlock(union ice_sq_lock *lock)
+	__releases(&lock->sq_spinlock)
+{
+	spin_unlock_irqrestore(&lock->sq_spinlock, lock->sq_flags);
+}
+
+/**
+ * ice_sq_mutex_lock - Call mutex_lock for union ice_sq_lock
+ * @lock: lock handle
+ */
+static void ice_sq_mutex_lock(union ice_sq_lock *lock)
+	__acquires(&lock->sq_mutex)
+{
+	mutex_lock(&lock->sq_mutex);
+}
+
+/**
+ * ice_sq_mutex_unlock - Call mutex_unlock for union ice_sq_lock
+ * @lock: lock handle
+ */
+static void ice_sq_mutex_unlock(union ice_sq_lock *lock)
+	__releases(&lock->sq_mutex)
+{
+	mutex_unlock(&lock->sq_mutex);
+}
+
+static struct ice_sq_ops ice_spin_ops = {
+	.lock = ice_sq_spin_lock,
+	.unlock = ice_sq_spin_unlock,
+};
+
+static struct ice_sq_ops ice_mutex_ops = {
+	.lock = ice_sq_mutex_lock,
+	.unlock = ice_sq_mutex_unlock,
+};
+
 /**
  * ice_init_ctrlq_locks - Initialize locks for a control queue
+ * @hw: pointer to the hardware structure
  * @cq: pointer to the control queue
+ * @q_type: specific control queue type
  *
  * Initializes the send and receive queue locks for a given control queue.
  */
-static void ice_init_ctrlq_locks(struct ice_ctl_q_info *cq)
+static void ice_init_ctrlq_locks(struct ice_hw *hw, struct ice_ctl_q_info *cq,
+				 enum ice_ctl_q q_type)
 {
-	mutex_init(&cq->sq_lock);
+	if (q_type == ICE_CTL_Q_SB) {
+		cq->sq_ops = ice_spin_ops;
+		spin_lock_init(&cq->sq_lock.sq_spinlock);
+	} else {
+		cq->sq_ops = ice_mutex_ops;
+		mutex_init(&cq->sq_lock.sq_mutex);
+	}
+
 	mutex_init(&cq->rq_lock);
 }
 
@@ -806,23 +866,26 @@ static void ice_init_ctrlq_locks(struct ice_ctl_q_info *cq)
  */
 int ice_create_all_ctrlq(struct ice_hw *hw)
 {
-	ice_init_ctrlq_locks(&hw->adminq);
+	ice_init_ctrlq_locks(hw, &hw->adminq, ICE_CTL_Q_ADMIN);
 	if (ice_is_sbq_supported(hw))
-		ice_init_ctrlq_locks(&hw->sbq);
-	ice_init_ctrlq_locks(&hw->mailboxq);
+		ice_init_ctrlq_locks(hw, &hw->sbq, ICE_CTL_Q_SB);
+	ice_init_ctrlq_locks(hw, &hw->mailboxq, ICE_CTL_Q_MAILBOX);
 
 	return ice_init_all_ctrlq(hw);
 }
 
 /**
  * ice_destroy_ctrlq_locks - Destroy locks for a control queue
+ * @hw: pointer to the hardware structure
  * @cq: pointer to the control queue
  *
  * Destroys the send and receive queue locks for a given control queue.
  */
-static void ice_destroy_ctrlq_locks(struct ice_ctl_q_info *cq)
+static void ice_destroy_ctrlq_locks(struct ice_hw *hw,
+				    struct ice_ctl_q_info *cq)
 {
-	mutex_destroy(&cq->sq_lock);
+	if (cq->qtype != ICE_CTL_Q_SB)
+		mutex_destroy(&cq->sq_lock.sq_mutex);
 	mutex_destroy(&cq->rq_lock);
 }
 
@@ -840,10 +903,10 @@ void ice_destroy_all_ctrlq(struct ice_hw *hw)
 	/* shut down all the control queues first */
 	ice_shutdown_all_ctrlq(hw, true);
 
-	ice_destroy_ctrlq_locks(&hw->adminq);
+	ice_destroy_ctrlq_locks(hw, &hw->adminq);
 	if (ice_is_sbq_supported(hw))
-		ice_destroy_ctrlq_locks(&hw->sbq);
-	ice_destroy_ctrlq_locks(&hw->mailboxq);
+		ice_destroy_ctrlq_locks(hw, &hw->sbq);
+	ice_destroy_ctrlq_locks(hw, &hw->mailboxq);
 }
 
 /**
@@ -972,9 +1035,15 @@ static bool ice_sq_done(struct ice_hw *hw, struct ice_ctl_q_info *cq)
 	 */
 	udelay(5);
 
-	return !rd32_poll_timeout(hw, cq->sq.head,
-				  head, head == cq->sq.next_to_use,
-				  20, ICE_CTL_Q_SQ_CMD_TIMEOUT);
+	if (cq->qtype == ICE_CTL_Q_SB)
+		return !read_poll_timeout_atomic(rd32, head,
+						 head == cq->sq.next_to_use, 5,
+						 ICE_CTL_Q_SQ_CMD_TIMEOUT_SPIN,
+						 false, hw, cq->sq.head);
+
+	return !rd32_poll_timeout(hw, cq->sq.head, head,
+				  head == cq->sq.next_to_use, 20,
+				  ICE_CTL_Q_SQ_CMD_TIMEOUT);
 }
 
 /**
@@ -1011,7 +1080,7 @@ int ice_sq_send_cmd(struct ice_hw *hw, struct ice_ctl_q_info *cq,
 	if (!buf && buf_size)
 		return -EINVAL;
 
-	mutex_lock(&cq->sq_lock);
+	cq->sq_ops.lock(&cq->sq_lock);
 	cq->sq_last_status = LIBIE_AQ_RC_OK;
 
 	if (!cq->sq.count) {
@@ -1132,7 +1201,7 @@ int ice_sq_send_cmd(struct ice_hw *hw, struct ice_ctl_q_info *cq,
 	}
 
 err:
-	mutex_unlock(&cq->sq_lock);
+	cq->sq_ops.unlock(&cq->sq_lock);
 	return err;
 }
 
diff --git a/drivers/net/ethernet/intel/ice/ice_controlq.h b/drivers/net/ethernet/intel/ice/ice_controlq.h
index 7c98d3a0314e..776ec57b2061 100644
--- a/drivers/net/ethernet/intel/ice/ice_controlq.h
+++ b/drivers/net/ethernet/intel/ice/ice_controlq.h
@@ -1,5 +1,5 @@
 /* SPDX-License-Identifier: GPL-2.0 */
-/* Copyright (c) 2018, Intel Corporation. */
+/* Copyright (c) 2018-2025, Intel Corporation. */
 
 #ifndef _ICE_CONTROLQ_H_
 #define _ICE_CONTROLQ_H_
@@ -44,6 +44,7 @@ enum ice_ctl_q {
 
 /* Control Queue timeout settings - max delay 1s */
 #define ICE_CTL_Q_SQ_CMD_TIMEOUT	USEC_PER_SEC
+#define ICE_CTL_Q_SQ_CMD_TIMEOUT_SPIN	100
 #define ICE_CTL_Q_ADMIN_INIT_TIMEOUT	10    /* Count 10 times */
 #define ICE_CTL_Q_ADMIN_INIT_MSEC	100   /* Check every 100msec */
 
@@ -88,6 +89,19 @@ struct ice_rq_event_info {
 	u8 *msg_buf;
 };
 
+union ice_sq_lock {
+	struct mutex sq_mutex;	/* Non-atomic SQ lock. */
+	struct {
+		spinlock_t sq_spinlock;	/* Atomic SQ lock. */
+		unsigned long sq_flags;		/* Send queue IRQ flags. */
+	};
+};
+
+struct ice_sq_ops {
+	void (*lock)(union ice_sq_lock *lock);
+	void (*unlock)(union ice_sq_lock *lock);
+};
+
 /* Control Queue information */
 struct ice_ctl_q_info {
 	enum ice_ctl_q qtype;
@@ -98,7 +112,8 @@ struct ice_ctl_q_info {
 	u16 rq_buf_size;		/* receive queue buffer size */
 	u16 sq_buf_size;		/* send queue buffer size */
 	enum libie_aq_err sq_last_status;	/* last status on send queue */
-	struct mutex sq_lock;		/* Send queue lock */
+	union ice_sq_lock sq_lock;	/* Send queue lock. */
+	struct ice_sq_ops sq_ops;	/* Send queue ops. */
 	struct mutex rq_lock;		/* Receive queue lock */
 };
 
-- 
2.49.0

Re: [PATCH iwl-next 3/4] ice: use spin_lock for sideband queue send queue

[Simon Horman]

On Tue, May 20, 2025 at 01:06:28PM +0200, Karol Kolacinski wrote:
> Sideband queue is a HW queue and has much faster completion time than
> other queues.
> 
> With <5 us for read on average it is possible to use spin_lock to be
> able to read/write sideband queue messages in the interrupt top half.
> 
> Add send queue lock/unlock operations and assign them based on the queue
> type. Use ice_sq_spin_lock/unlock for sideband queue and
> ice_sq_mutex_lock/unlock for other queues.
> 
> Reviewed-by: Milena Olech <milena.olech@intel.com>
> Signed-off-by: Karol Kolacinski <karol.kolacinski@intel.com>

...

> +/**
> + * ice_sq_spin_lock - Call spin_lock_irqsave for union ice_sq_lock
> + * @lock: lock handle
> + */
> +static void ice_sq_spin_lock(union ice_sq_lock *lock)
> +	__acquires(&lock->sq_spinlock)
> +{
> +	spin_lock_irqsave(&lock->sq_spinlock, lock->sq_flags);
> +}
> +
> +/**
> + * ice_sq_spin_unlock - Call spin_unlock_irqrestore for union ice_sq_lock
> + * @lock: lock handle
> + */
> +static void ice_sq_spin_unlock(union ice_sq_lock *lock)
> +	__releases(&lock->sq_spinlock)
> +{
> +	spin_unlock_irqrestore(&lock->sq_spinlock, lock->sq_flags);
> +}
> +
> +/**
> + * ice_sq_mutex_lock - Call mutex_lock for union ice_sq_lock
> + * @lock: lock handle
> + */
> +static void ice_sq_mutex_lock(union ice_sq_lock *lock)
> +	__acquires(&lock->sq_mutex)
> +{
> +	mutex_lock(&lock->sq_mutex);
> +}
> +
> +/**
> + * ice_sq_mutex_unlock - Call mutex_unlock for union ice_sq_lock
> + * @lock: lock handle
> + */
> +static void ice_sq_mutex_unlock(union ice_sq_lock *lock)
> +	__releases(&lock->sq_mutex)
> +{
> +	mutex_unlock(&lock->sq_mutex);
> +}

Sparse seems unhappy about the annotations on the mutex functions above,
but curiously happy with those for the corresponding spinlock functions.
I am unsure why.

  .../ice_controlq.c:803:13: warning: context imbalance in 'ice_sq_mutex_lock' - wrong count at exit
  .../ice_controlq.c:813:13: warning: context imbalance in 'ice_sq_mutex_unlock' - wrong count at exit


> +
> +static struct ice_sq_ops ice_spin_ops = {
> +	.lock = ice_sq_spin_lock,
> +	.unlock = ice_sq_spin_unlock,
> +};
> +
> +static struct ice_sq_ops ice_mutex_ops = {
> +	.lock = ice_sq_mutex_lock,
> +	.unlock = ice_sq_mutex_unlock,
> +};
> +

...

[PATCH iwl-next 4/4] ice: read Tx timestamps in the IRQ top half

[Karol Kolacinski]

With sideband queue using delays and spin locks, it is possible to
read timestamps from the PHY in the top half of the interrupt.

This removes bottom half scheduling delays and improves timestamping
read times significantly, from >2 ms to <50 us.

Reviewed-by: Milena Olech <milena.olech@intel.com>
Signed-off-by: Karol Kolacinski <karol.kolacinski@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_ptp.c | 46 ++++++++++++------------
 1 file changed, 22 insertions(+), 24 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
index b8e55931fc52..e1068489fde5 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp.c
+++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
@@ -2694,39 +2694,37 @@ irqreturn_t ice_ptp_ts_irq(struct ice_pf *pf)
 
 	switch (hw->mac_type) {
 	case ICE_MAC_E810:
+	{
+		struct ice_ptp_tx *tx = &pf->ptp.port.tx;
+		u8 idx;
+
+		if (!ice_pf_state_is_nominal(pf))
+			return IRQ_HANDLED;
+
 		/* E810 capable of low latency timestamping with interrupt can
 		 * request a single timestamp in the top half and wait for
 		 * a second LL TS interrupt from the FW when it's ready.
 		 */
-		if (hw->dev_caps.ts_dev_info.ts_ll_int_read) {
-			struct ice_ptp_tx *tx = &pf->ptp.port.tx;
-			u8 idx;
-
-			if (!ice_pf_state_is_nominal(pf))
+		if (!hw->dev_caps.ts_dev_info.ts_ll_int_read) {
+			if (!ice_ptp_pf_handles_tx_interrupt(pf))
 				return IRQ_HANDLED;
 
-			spin_lock(&tx->lock);
-			idx = find_next_bit_wrap(tx->in_use, tx->len,
-						 tx->last_ll_ts_idx_read + 1);
-			if (idx != tx->len)
-				ice_ptp_req_tx_single_tstamp(tx, idx);
-			spin_unlock(&tx->lock);
-
-			return IRQ_HANDLED;
+			set_bit(ICE_MISC_THREAD_TX_TSTAMP, pf->misc_thread);
+			return IRQ_WAKE_THREAD;
 		}
-		fallthrough; /* non-LL_TS E810 */
-	case ICE_MAC_GENERIC:
-	case ICE_MAC_GENERIC_3K_E825:
-		/* All other devices process timestamps in the bottom half due
-		 * to sleeping or polling.
-		 */
-		if (!ice_ptp_pf_handles_tx_interrupt(pf))
-			return IRQ_HANDLED;
 
-		set_bit(ICE_MISC_THREAD_TX_TSTAMP, pf->misc_thread);
-		return IRQ_WAKE_THREAD;
+		spin_lock(&tx->lock);
+		idx = find_next_bit_wrap(tx->in_use, tx->len,
+					 tx->last_ll_ts_idx_read + 1);
+		if (idx != tx->len)
+			ice_ptp_req_tx_single_tstamp(tx, idx);
+		spin_unlock(&tx->lock);
+
+		return IRQ_HANDLED;
+	}
 	case ICE_MAC_E830:
-		/* E830 can read timestamps in the top half using rd32() */
+	case ICE_MAC_GENERIC:
+	case ICE_MAC_GENERIC_3K_E825:
 		if (ice_ptp_process_ts(pf) == ICE_TX_TSTAMP_WORK_PENDING) {
 			/* Process outstanding Tx timestamps. If there
 			 * is more work, re-arm the interrupt to trigger again.
-- 
2.49.0

Re: [PATCH iwl-next 4/4] ice: read Tx timestamps in the IRQ top half

[Simon Horman]

On Tue, May 20, 2025 at 01:06:29PM +0200, Karol Kolacinski wrote:
> With sideband queue using delays and spin locks, it is possible to
> read timestamps from the PHY in the top half of the interrupt.
> 
> This removes bottom half scheduling delays and improves timestamping
> read times significantly, from >2 ms to <50 us.
> 
> Reviewed-by: Milena Olech <milena.olech@intel.com>
> Signed-off-by: Karol Kolacinski <karol.kolacinski@intel.com>

Reviewed-by: Simon Horman <horms@kernel.org>