[PATCH] qed: allow sleep in qed_mcp_trace_dump()

[PATCH] qed: allow sleep in qed_mcp_trace_dump()

[Caleb Sander]

By default, qed_mcp_cmd_and_union() waits for 10us at a time
in a loop that can run 500K times, so calls to qed_mcp_nvm_rd_cmd()
may block the current thread for over 5s.
We observed thread scheduling delays of over 700ms in production,
with stacktraces pointing to this code as the culprit.

qed_mcp_trace_dump() is called from ethtool, so sleeping is permitted.
It already can sleep in qed_mcp_halt(), which calls qed_mcp_cmd().
Add a "can sleep" parameter to qed_find_nvram_image() and
qed_nvram_read() so they can sleep during qed_mcp_trace_dump().
qed_mcp_trace_get_meta_info() and qed_mcp_trace_read_meta(),
called only by qed_mcp_trace_dump(), allow these functions to sleep.
It's not clear to me that the other caller (qed_grc_dump_mcp_hw_dump())
can sleep, so it keeps b_can_sleep set to false.

Signed-off-by: Caleb Sander <csander@purestorage.com>
---
 drivers/net/ethernet/qlogic/qed/qed_debug.c | 28 +++++++++++++++------
 1 file changed, 20 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/qlogic/qed/qed_debug.c b/drivers/net/ethernet/qlogic/qed/qed_debug.c
index 86ecb080b153..cdcead614e9f 100644
--- a/drivers/net/ethernet/qlogic/qed/qed_debug.c
+++ b/drivers/net/ethernet/qlogic/qed/qed_debug.c
@@ -1830,11 +1830,12 @@ static void qed_grc_clear_all_prty(struct qed_hwfn *p_hwfn,
 /* Finds the meta data image in NVRAM */
 static enum dbg_status qed_find_nvram_image(struct qed_hwfn *p_hwfn,
 					    struct qed_ptt *p_ptt,
 					    u32 image_type,
 					    u32 *nvram_offset_bytes,
-					    u32 *nvram_size_bytes)
+					    u32 *nvram_size_bytes,
+					    bool b_can_sleep)
 {
 	u32 ret_mcp_resp, ret_mcp_param, ret_txn_size;
 	struct mcp_file_att file_att;
 	int nvm_result;
 
@@ -1844,11 +1845,12 @@ static enum dbg_status qed_find_nvram_image(struct qed_hwfn *p_hwfn,
 					DRV_MSG_CODE_NVM_GET_FILE_ATT,
 					image_type,
 					&ret_mcp_resp,
 					&ret_mcp_param,
 					&ret_txn_size,
-					(u32 *)&file_att, false);
+					(u32 *)&file_att,
+					b_can_sleep);
 
 	/* Check response */
 	if (nvm_result || (ret_mcp_resp & FW_MSG_CODE_MASK) !=
 	    FW_MSG_CODE_NVM_OK)
 		return DBG_STATUS_NVRAM_GET_IMAGE_FAILED;
@@ -1871,11 +1873,13 @@ static enum dbg_status qed_find_nvram_image(struct qed_hwfn *p_hwfn,
 
 /* Reads data from NVRAM */
 static enum dbg_status qed_nvram_read(struct qed_hwfn *p_hwfn,
 				      struct qed_ptt *p_ptt,
 				      u32 nvram_offset_bytes,
-				      u32 nvram_size_bytes, u32 *ret_buf)
+				      u32 nvram_size_bytes,
+				      u32 *ret_buf,
+				      bool b_can_sleep)
 {
 	u32 ret_mcp_resp, ret_mcp_param, ret_read_size, bytes_to_copy;
 	s32 bytes_left = nvram_size_bytes;
 	u32 read_offset = 0, param = 0;
 
@@ -1897,11 +1901,11 @@ static enum dbg_status qed_nvram_read(struct qed_hwfn *p_hwfn,
 		if (qed_mcp_nvm_rd_cmd(p_hwfn, p_ptt,
 				       DRV_MSG_CODE_NVM_READ_NVRAM, param,
 				       &ret_mcp_resp,
 				       &ret_mcp_param, &ret_read_size,
 				       (u32 *)((u8 *)ret_buf + read_offset),
-				       false))
+				       b_can_sleep))
 			return DBG_STATUS_NVRAM_READ_FAILED;
 
 		/* Check response */
 		if ((ret_mcp_resp & FW_MSG_CODE_MASK) != FW_MSG_CODE_NVM_OK)
 			return DBG_STATUS_NVRAM_READ_FAILED;
@@ -3378,11 +3382,12 @@ static u32 qed_grc_dump_mcp_hw_dump(struct qed_hwfn *p_hwfn,
 	/* Read HW dump image from NVRAM */
 	status = qed_find_nvram_image(p_hwfn,
 				      p_ptt,
 				      NVM_TYPE_HW_DUMP_OUT,
 				      &hw_dump_offset_bytes,
-				      &hw_dump_size_bytes);
+				      &hw_dump_size_bytes,
+				      false);
 	if (status != DBG_STATUS_OK)
 		return 0;
 
 	hw_dump_size_dwords = BYTES_TO_DWORDS(hw_dump_size_bytes);
 
@@ -3395,11 +3400,13 @@ static u32 qed_grc_dump_mcp_hw_dump(struct qed_hwfn *p_hwfn,
 	/* Read MCP HW dump image into dump buffer */
 	if (dump && hw_dump_size_dwords) {
 		status = qed_nvram_read(p_hwfn,
 					p_ptt,
 					hw_dump_offset_bytes,
-					hw_dump_size_bytes, dump_buf + offset);
+					hw_dump_size_bytes,
+					dump_buf + offset,
+					false);
 		if (status != DBG_STATUS_OK) {
 			DP_NOTICE(p_hwfn,
 				  "Failed to read MCP HW Dump image from NVRAM\n");
 			return 0;
 		}
@@ -4121,11 +4128,13 @@ static enum dbg_status qed_mcp_trace_get_meta_info(struct qed_hwfn *p_hwfn,
 	    (*running_bundle_id ==
 	     DIR_ID_1) ? NVM_TYPE_MFW_TRACE1 : NVM_TYPE_MFW_TRACE2;
 	return qed_find_nvram_image(p_hwfn,
 				    p_ptt,
 				    nvram_image_type,
-				    trace_meta_offset, trace_meta_size);
+				    trace_meta_offset,
+				    trace_meta_size,
+				    true);
 }
 
 /* Reads the MCP Trace meta data from NVRAM into the specified buffer */
 static enum dbg_status qed_mcp_trace_read_meta(struct qed_hwfn *p_hwfn,
 					       struct qed_ptt *p_ptt,
@@ -4137,11 +4146,14 @@ static enum dbg_status qed_mcp_trace_read_meta(struct qed_hwfn *p_hwfn,
 	u32 signature;
 
 	/* Read meta data from NVRAM */
 	status = qed_nvram_read(p_hwfn,
 				p_ptt,
-				nvram_offset_in_bytes, size_in_bytes, buf);
+				nvram_offset_in_bytes,
+				size_in_bytes,
+				buf,
+				true);
 	if (status != DBG_STATUS_OK)
 		return status;
 
 	/* Extract and check first signature */
 	signature = qed_read_unaligned_dword(byte_buf);
-- 
2.25.1

Re: [PATCH] qed: allow sleep in qed_mcp_trace_dump()

[Paolo Abeni]

On Sat, 2022-12-17 at 10:56 -0700, Caleb Sander wrote:
> By default, qed_mcp_cmd_and_union() waits for 10us at a time
> in a loop that can run 500K times, so calls to qed_mcp_nvm_rd_cmd()
> may block the current thread for over 5s.
> We observed thread scheduling delays of over 700ms in production,
> with stacktraces pointing to this code as the culprit.

IMHO this is material eligible for the net tree...

> 
> qed_mcp_trace_dump() is called from ethtool, so sleeping is permitted.
> It already can sleep in qed_mcp_halt(), which calls qed_mcp_cmd().
> Add a "can sleep" parameter to qed_find_nvram_image() and
> qed_nvram_read() so they can sleep during qed_mcp_trace_dump().
> qed_mcp_trace_get_meta_info() and qed_mcp_trace_read_meta(),
> called only by qed_mcp_trace_dump(), allow these functions to sleep.
> It's not clear to me that the other caller (qed_grc_dump_mcp_hw_dump())
> can sleep, so it keeps b_can_sleep set to false.

...but we need a suitable Fixes tag here. Please repost specifying the
target tree into the subject and adding the relevant tag, thanks!

Paolo

Re: [PATCH] qed: allow sleep in qed_mcp_trace_dump()

[Caleb Sander]

On Tue, Dec 20, 2022 at 1:55 AM Paolo Abeni <pabeni@redhat.com> wrote:
>
> On Sat, 2022-12-17 at 10:56 -0700, Caleb Sander wrote:
> > By default, qed_mcp_cmd_and_union() waits for 10us at a time
> > in a loop that can run 500K times, so calls to qed_mcp_nvm_rd_cmd()
> > may block the current thread for over 5s.
> > We observed thread scheduling delays of over 700ms in production,
> > with stacktraces pointing to this code as the culprit.
>
> IMHO this is material eligible for the net tree...
>
> >
> > qed_mcp_trace_dump() is called from ethtool, so sleeping is permitted.
> > It already can sleep in qed_mcp_halt(), which calls qed_mcp_cmd().
> > Add a "can sleep" parameter to qed_find_nvram_image() and
> > qed_nvram_read() so they can sleep during qed_mcp_trace_dump().
> > qed_mcp_trace_get_meta_info() and qed_mcp_trace_read_meta(),
> > called only by qed_mcp_trace_dump(), allow these functions to sleep.
> > It's not clear to me that the other caller (qed_grc_dump_mcp_hw_dump())
> > can sleep, so it keeps b_can_sleep set to false.
>
> ...but we need a suitable Fixes tag here. Please repost specifying the
> target tree into the subject and adding the relevant tag, thanks!

Sure, I can do that, but I would like to get some sign-off from the
driver authors.
The last time we attempted to fix this bug, we were told our change
could cause the driver to sleep in atomic contexts. So it would be great to hear
from QLogic (now Marvell) whether this fix is acceptable.

Thanks,
Caleb

[PATCH net v2] qed: allow sleep in qed_mcp_trace_dump()

[Caleb Sander]

By default, qed_mcp_cmd_and_union() delays 10us at a time in a loop
that can run 500K times, so calls to qed_mcp_nvm_rd_cmd()
may block the current thread for over 5s.
We observed thread scheduling delays over 700ms in production,
with stacktraces pointing to this code as the culprit.

qed_mcp_trace_dump() is called from ethtool, so sleeping is permitted.
It already can sleep in qed_mcp_halt(), which calls qed_mcp_cmd().
Add a "can sleep" parameter to qed_find_nvram_image() and
qed_nvram_read() so they can sleep during qed_mcp_trace_dump().
qed_mcp_trace_get_meta_info() and qed_mcp_trace_read_meta(),
called only by qed_mcp_trace_dump(), allow these functions to sleep.
I can't tell if the other caller (qed_grc_dump_mcp_hw_dump()) can sleep,
so keep b_can_sleep set to false when it calls these functions.

Signed-off-by: Caleb Sander <csander@purestorage.com>
Acked-by: Alok Prasad <palok@marvell.com>
Fixes: c965db4446291 ("qed: Add support for debug data collection")
---
 drivers/net/ethernet/qlogic/qed/qed_debug.c | 28 +++++++++++++++------
 1 file changed, 20 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/qlogic/qed/qed_debug.c b/drivers/net/ethernet/qlogic/qed/qed_debug.c
index 86ecb080b153..cdcead614e9f 100644
--- a/drivers/net/ethernet/qlogic/qed/qed_debug.c
+++ b/drivers/net/ethernet/qlogic/qed/qed_debug.c
@@ -1830,11 +1830,12 @@ static void qed_grc_clear_all_prty(struct qed_hwfn *p_hwfn,
 /* Finds the meta data image in NVRAM */
 static enum dbg_status qed_find_nvram_image(struct qed_hwfn *p_hwfn,
 					    struct qed_ptt *p_ptt,
 					    u32 image_type,
 					    u32 *nvram_offset_bytes,
-					    u32 *nvram_size_bytes)
+					    u32 *nvram_size_bytes,
+					    bool b_can_sleep)
 {
 	u32 ret_mcp_resp, ret_mcp_param, ret_txn_size;
 	struct mcp_file_att file_att;
 	int nvm_result;
 
@@ -1844,11 +1845,12 @@ static enum dbg_status qed_find_nvram_image(struct qed_hwfn *p_hwfn,
 					DRV_MSG_CODE_NVM_GET_FILE_ATT,
 					image_type,
 					&ret_mcp_resp,
 					&ret_mcp_param,
 					&ret_txn_size,
-					(u32 *)&file_att, false);
+					(u32 *)&file_att,
+					b_can_sleep);
 
 	/* Check response */
 	if (nvm_result || (ret_mcp_resp & FW_MSG_CODE_MASK) !=
 	    FW_MSG_CODE_NVM_OK)
 		return DBG_STATUS_NVRAM_GET_IMAGE_FAILED;
@@ -1871,11 +1873,13 @@ static enum dbg_status qed_find_nvram_image(struct qed_hwfn *p_hwfn,
 
 /* Reads data from NVRAM */
 static enum dbg_status qed_nvram_read(struct qed_hwfn *p_hwfn,
 				      struct qed_ptt *p_ptt,
 				      u32 nvram_offset_bytes,
-				      u32 nvram_size_bytes, u32 *ret_buf)
+				      u32 nvram_size_bytes,
+				      u32 *ret_buf,
+				      bool b_can_sleep)
 {
 	u32 ret_mcp_resp, ret_mcp_param, ret_read_size, bytes_to_copy;
 	s32 bytes_left = nvram_size_bytes;
 	u32 read_offset = 0, param = 0;
 
@@ -1897,11 +1901,11 @@ static enum dbg_status qed_nvram_read(struct qed_hwfn *p_hwfn,
 		if (qed_mcp_nvm_rd_cmd(p_hwfn, p_ptt,
 				       DRV_MSG_CODE_NVM_READ_NVRAM, param,
 				       &ret_mcp_resp,
 				       &ret_mcp_param, &ret_read_size,
 				       (u32 *)((u8 *)ret_buf + read_offset),
-				       false))
+				       b_can_sleep))
 			return DBG_STATUS_NVRAM_READ_FAILED;
 
 		/* Check response */
 		if ((ret_mcp_resp & FW_MSG_CODE_MASK) != FW_MSG_CODE_NVM_OK)
 			return DBG_STATUS_NVRAM_READ_FAILED;
@@ -3378,11 +3382,12 @@ static u32 qed_grc_dump_mcp_hw_dump(struct qed_hwfn *p_hwfn,
 	/* Read HW dump image from NVRAM */
 	status = qed_find_nvram_image(p_hwfn,
 				      p_ptt,
 				      NVM_TYPE_HW_DUMP_OUT,
 				      &hw_dump_offset_bytes,
-				      &hw_dump_size_bytes);
+				      &hw_dump_size_bytes,
+				      false);
 	if (status != DBG_STATUS_OK)
 		return 0;
 
 	hw_dump_size_dwords = BYTES_TO_DWORDS(hw_dump_size_bytes);
 
@@ -3395,11 +3400,13 @@ static u32 qed_grc_dump_mcp_hw_dump(struct qed_hwfn *p_hwfn,
 	/* Read MCP HW dump image into dump buffer */
 	if (dump && hw_dump_size_dwords) {
 		status = qed_nvram_read(p_hwfn,
 					p_ptt,
 					hw_dump_offset_bytes,
-					hw_dump_size_bytes, dump_buf + offset);
+					hw_dump_size_bytes,
+					dump_buf + offset,
+					false);
 		if (status != DBG_STATUS_OK) {
 			DP_NOTICE(p_hwfn,
 				  "Failed to read MCP HW Dump image from NVRAM\n");
 			return 0;
 		}
@@ -4121,11 +4128,13 @@ static enum dbg_status qed_mcp_trace_get_meta_info(struct qed_hwfn *p_hwfn,
 	    (*running_bundle_id ==
 	     DIR_ID_1) ? NVM_TYPE_MFW_TRACE1 : NVM_TYPE_MFW_TRACE2;
 	return qed_find_nvram_image(p_hwfn,
 				    p_ptt,
 				    nvram_image_type,
-				    trace_meta_offset, trace_meta_size);
+				    trace_meta_offset,
+				    trace_meta_size,
+				    true);
 }
 
 /* Reads the MCP Trace meta data from NVRAM into the specified buffer */
 static enum dbg_status qed_mcp_trace_read_meta(struct qed_hwfn *p_hwfn,
 					       struct qed_ptt *p_ptt,
@@ -4137,11 +4146,14 @@ static enum dbg_status qed_mcp_trace_read_meta(struct qed_hwfn *p_hwfn,
 	u32 signature;
 
 	/* Read meta data from NVRAM */
 	status = qed_nvram_read(p_hwfn,
 				p_ptt,
-				nvram_offset_in_bytes, size_in_bytes, buf);
+				nvram_offset_in_bytes,
+				size_in_bytes,
+				buf,
+				true);
 	if (status != DBG_STATUS_OK)
 		return status;
 
 	/* Extract and check first signature */
 	signature = qed_read_unaligned_dword(byte_buf);
-- 
2.25.1

Re: [PATCH net v2] qed: allow sleep in qed_mcp_trace_dump()

[Leon Romanovsky]

On Wed, Dec 28, 2022 at 03:00:46PM -0700, Caleb Sander wrote:
> By default, qed_mcp_cmd_and_union() delays 10us at a time in a loop
> that can run 500K times, so calls to qed_mcp_nvm_rd_cmd()
> may block the current thread for over 5s.
> We observed thread scheduling delays over 700ms in production,
> with stacktraces pointing to this code as the culprit.

Please add stacktrace to the commit message.

> 
> qed_mcp_trace_dump() is called from ethtool, so sleeping is permitted.
> It already can sleep in qed_mcp_halt(), which calls qed_mcp_cmd().
> Add a "can sleep" parameter to qed_find_nvram_image() and
> qed_nvram_read() so they can sleep during qed_mcp_trace_dump().
> qed_mcp_trace_get_meta_info() and qed_mcp_trace_read_meta(),
> called only by qed_mcp_trace_dump(), allow these functions to sleep.
> I can't tell if the other caller (qed_grc_dump_mcp_hw_dump()) can sleep,
> so keep b_can_sleep set to false when it calls these functions.
> 
> Signed-off-by: Caleb Sander <csander@purestorage.com>
> Acked-by: Alok Prasad <palok@marvell.com>
> Fixes: c965db4446291 ("qed: Add support for debug data collection")

Fixes line should be before *-by tags.

> ---
>  drivers/net/ethernet/qlogic/qed/qed_debug.c | 28 +++++++++++++++------
>  1 file changed, 20 insertions(+), 8 deletions(-)

Please add changelog here while you are resending patches.

Thanks

> 
> diff --git a/drivers/net/ethernet/qlogic/qed/qed_debug.c b/drivers/net/ethernet/qlogic/qed/qed_debug.c
> index 86ecb080b153..cdcead614e9f 100644
> --- a/drivers/net/ethernet/qlogic/qed/qed_debug.c
> +++ b/drivers/net/ethernet/qlogic/qed/qed_debug.c
> @@ -1830,11 +1830,12 @@ static void qed_grc_clear_all_prty(struct qed_hwfn *p_hwfn,
>  /* Finds the meta data image in NVRAM */
>  static enum dbg_status qed_find_nvram_image(struct qed_hwfn *p_hwfn,
>  					    struct qed_ptt *p_ptt,
>  					    u32 image_type,
>  					    u32 *nvram_offset_bytes,
> -					    u32 *nvram_size_bytes)
> +					    u32 *nvram_size_bytes,
> +					    bool b_can_sleep)
>  {
>  	u32 ret_mcp_resp, ret_mcp_param, ret_txn_size;
>  	struct mcp_file_att file_att;
>  	int nvm_result;
>  
> @@ -1844,11 +1845,12 @@ static enum dbg_status qed_find_nvram_image(struct qed_hwfn *p_hwfn,
>  					DRV_MSG_CODE_NVM_GET_FILE_ATT,
>  					image_type,
>  					&ret_mcp_resp,
>  					&ret_mcp_param,
>  					&ret_txn_size,
> -					(u32 *)&file_att, false);
> +					(u32 *)&file_att,
> +					b_can_sleep);
>  
>  	/* Check response */
>  	if (nvm_result || (ret_mcp_resp & FW_MSG_CODE_MASK) !=
>  	    FW_MSG_CODE_NVM_OK)
>  		return DBG_STATUS_NVRAM_GET_IMAGE_FAILED;
> @@ -1871,11 +1873,13 @@ static enum dbg_status qed_find_nvram_image(struct qed_hwfn *p_hwfn,
>  
>  /* Reads data from NVRAM */
>  static enum dbg_status qed_nvram_read(struct qed_hwfn *p_hwfn,
>  				      struct qed_ptt *p_ptt,
>  				      u32 nvram_offset_bytes,
> -				      u32 nvram_size_bytes, u32 *ret_buf)
> +				      u32 nvram_size_bytes,
> +				      u32 *ret_buf,
> +				      bool b_can_sleep)
>  {
>  	u32 ret_mcp_resp, ret_mcp_param, ret_read_size, bytes_to_copy;
>  	s32 bytes_left = nvram_size_bytes;
>  	u32 read_offset = 0, param = 0;
>  
> @@ -1897,11 +1901,11 @@ static enum dbg_status qed_nvram_read(struct qed_hwfn *p_hwfn,
>  		if (qed_mcp_nvm_rd_cmd(p_hwfn, p_ptt,
>  				       DRV_MSG_CODE_NVM_READ_NVRAM, param,
>  				       &ret_mcp_resp,
>  				       &ret_mcp_param, &ret_read_size,
>  				       (u32 *)((u8 *)ret_buf + read_offset),
> -				       false))
> +				       b_can_sleep))
>  			return DBG_STATUS_NVRAM_READ_FAILED;
>  
>  		/* Check response */
>  		if ((ret_mcp_resp & FW_MSG_CODE_MASK) != FW_MSG_CODE_NVM_OK)
>  			return DBG_STATUS_NVRAM_READ_FAILED;
> @@ -3378,11 +3382,12 @@ static u32 qed_grc_dump_mcp_hw_dump(struct qed_hwfn *p_hwfn,
>  	/* Read HW dump image from NVRAM */
>  	status = qed_find_nvram_image(p_hwfn,
>  				      p_ptt,
>  				      NVM_TYPE_HW_DUMP_OUT,
>  				      &hw_dump_offset_bytes,
> -				      &hw_dump_size_bytes);
> +				      &hw_dump_size_bytes,
> +				      false);
>  	if (status != DBG_STATUS_OK)
>  		return 0;
>  
>  	hw_dump_size_dwords = BYTES_TO_DWORDS(hw_dump_size_bytes);
>  
> @@ -3395,11 +3400,13 @@ static u32 qed_grc_dump_mcp_hw_dump(struct qed_hwfn *p_hwfn,
>  	/* Read MCP HW dump image into dump buffer */
>  	if (dump && hw_dump_size_dwords) {
>  		status = qed_nvram_read(p_hwfn,
>  					p_ptt,
>  					hw_dump_offset_bytes,
> -					hw_dump_size_bytes, dump_buf + offset);
> +					hw_dump_size_bytes,
> +					dump_buf + offset,
> +					false);
>  		if (status != DBG_STATUS_OK) {
>  			DP_NOTICE(p_hwfn,
>  				  "Failed to read MCP HW Dump image from NVRAM\n");
>  			return 0;
>  		}
> @@ -4121,11 +4128,13 @@ static enum dbg_status qed_mcp_trace_get_meta_info(struct qed_hwfn *p_hwfn,
>  	    (*running_bundle_id ==
>  	     DIR_ID_1) ? NVM_TYPE_MFW_TRACE1 : NVM_TYPE_MFW_TRACE2;
>  	return qed_find_nvram_image(p_hwfn,
>  				    p_ptt,
>  				    nvram_image_type,
> -				    trace_meta_offset, trace_meta_size);
> +				    trace_meta_offset,
> +				    trace_meta_size,
> +				    true);
>  }
>  
>  /* Reads the MCP Trace meta data from NVRAM into the specified buffer */
>  static enum dbg_status qed_mcp_trace_read_meta(struct qed_hwfn *p_hwfn,
>  					       struct qed_ptt *p_ptt,
> @@ -4137,11 +4146,14 @@ static enum dbg_status qed_mcp_trace_read_meta(struct qed_hwfn *p_hwfn,
>  	u32 signature;
>  
>  	/* Read meta data from NVRAM */
>  	status = qed_nvram_read(p_hwfn,
>  				p_ptt,
> -				nvram_offset_in_bytes, size_in_bytes, buf);
> +				nvram_offset_in_bytes,
> +				size_in_bytes,
> +				buf,
> +				true);
>  	if (status != DBG_STATUS_OK)
>  		return status;
>  
>  	/* Extract and check first signature */
>  	signature = qed_read_unaligned_dword(byte_buf);
> -- 
> 2.25.1
> 

[PATCH net v3] qed: allow sleep in qed_mcp_trace_dump()

[Caleb Sander]

By default, qed_mcp_cmd_and_union() delays 10us at a time in a loop
that can run 500K times, so calls to qed_mcp_nvm_rd_cmd()
may block the current thread for over 5s.
We observed thread scheduling delays over 700ms in production,
with stacktraces pointing to this code as the culprit.

qed_mcp_trace_dump() is called from ethtool, so sleeping is permitted.
It already can sleep in qed_mcp_halt(), which calls qed_mcp_cmd().
Add a "can sleep" parameter to qed_find_nvram_image() and
qed_nvram_read() so they can sleep during qed_mcp_trace_dump().
qed_mcp_trace_get_meta_info() and qed_mcp_trace_read_meta(),
called only by qed_mcp_trace_dump(), allow these functions to sleep.
I can't tell if the other caller (qed_grc_dump_mcp_hw_dump()) can sleep,
so keep b_can_sleep set to false when it calls these functions.

An example stacktrace from a custom warning we added to the kernel
showing a thread that has not scheduled despite long needing resched:
[ 2745.362925,17] ------------[ cut here ]------------
[ 2745.362941,17] WARNING: CPU: 23 PID: 5640 at arch/x86/kernel/irq.c:233 do_IRQ+0x15e/0x1a0()
[ 2745.362946,17] Thread not rescheduled for 744 ms after irq 99
[ 2745.362956,17] Modules linked in: ...
[ 2745.363339,17] CPU: 23 PID: 5640 Comm: lldpd Tainted: P           O    4.4.182+ #202104120910+6d1da174272d.61x
[ 2745.363343,17] Hardware name: FOXCONN MercuryB/Quicksilver Controller, BIOS H11P1N09 07/08/2020
[ 2745.363346,17]  0000000000000000 ffff885ec07c3ed8 ffffffff8131eb2f ffff885ec07c3f20
[ 2745.363358,17]  ffffffff81d14f64 ffff885ec07c3f10 ffffffff81072ac2 ffff88be98ed0000
[ 2745.363369,17]  0000000000000063 0000000000000174 0000000000000074 0000000000000000
[ 2745.363379,17] Call Trace:
[ 2745.363382,17]  <IRQ>  [<ffffffff8131eb2f>] dump_stack+0x8e/0xcf
[ 2745.363393,17]  [<ffffffff81072ac2>] warn_slowpath_common+0x82/0xc0
[ 2745.363398,17]  [<ffffffff81072b4c>] warn_slowpath_fmt+0x4c/0x50
[ 2745.363404,17]  [<ffffffff810d5a8e>] ? rcu_irq_exit+0xae/0xc0
[ 2745.363408,17]  [<ffffffff817c99fe>] do_IRQ+0x15e/0x1a0
[ 2745.363413,17]  [<ffffffff817c7ac9>] common_interrupt+0x89/0x89
[ 2745.363416,17]  <EOI>  [<ffffffff8132aa74>] ? delay_tsc+0x24/0x50
[ 2745.363425,17]  [<ffffffff8132aa04>] __udelay+0x34/0x40
[ 2745.363457,17]  [<ffffffffa04d45ff>] qed_mcp_cmd_and_union+0x36f/0x7d0 [qed]
[ 2745.363473,17]  [<ffffffffa04d5ced>] qed_mcp_nvm_rd_cmd+0x4d/0x90 [qed]
[ 2745.363490,17]  [<ffffffffa04e1dc7>] qed_mcp_trace_dump+0x4a7/0x630 [qed]
[ 2745.363504,17]  [<ffffffffa04e2556>] ? qed_fw_asserts_dump+0x1d6/0x1f0 [qed]
[ 2745.363520,17]  [<ffffffffa04e4ea7>] qed_dbg_mcp_trace_get_dump_buf_size+0x37/0x80 [qed]
[ 2745.363536,17]  [<ffffffffa04ea881>] qed_dbg_feature_size+0x61/0xa0 [qed]
[ 2745.363551,17]  [<ffffffffa04eb427>] qed_dbg_all_data_size+0x247/0x260 [qed]
[ 2745.363560,17]  [<ffffffffa0482c10>] qede_get_regs_len+0x30/0x40 [qede]
[ 2745.363566,17]  [<ffffffff816c9783>] ethtool_get_drvinfo+0xe3/0x190
[ 2745.363570,17]  [<ffffffff816cc152>] dev_ethtool+0x1362/0x2140
[ 2745.363575,17]  [<ffffffff8109bcc6>] ? finish_task_switch+0x76/0x260
[ 2745.363580,17]  [<ffffffff817c2116>] ? __schedule+0x3c6/0x9d0
[ 2745.363585,17]  [<ffffffff810dbd50>] ? hrtimer_start_range_ns+0x1d0/0x370
[ 2745.363589,17]  [<ffffffff816c1e5b>] ? dev_get_by_name_rcu+0x6b/0x90
[ 2745.363594,17]  [<ffffffff816de6a8>] dev_ioctl+0xe8/0x710
[ 2745.363599,17]  [<ffffffff816a58a8>] sock_do_ioctl+0x48/0x60
[ 2745.363603,17]  [<ffffffff816a5d87>] sock_ioctl+0x1c7/0x280
[ 2745.363608,17]  [<ffffffff8111f393>] ? seccomp_phase1+0x83/0x220
[ 2745.363612,17]  [<ffffffff811e3503>] do_vfs_ioctl+0x2b3/0x4e0
[ 2745.363616,17]  [<ffffffff811e3771>] SyS_ioctl+0x41/0x70
[ 2745.363619,17]  [<ffffffff817c6ffe>] entry_SYSCALL_64_fastpath+0x1e/0x79
[ 2745.363622,17] ---[ end trace f6954aa440266421 ]---

Fixes: c965db4446291 ("qed: Add support for debug data collection")
Signed-off-by: Caleb Sander <csander@purestorage.com>
Acked-by: Alok Prasad <palok@marvell.com>
---
 drivers/net/ethernet/qlogic/qed/qed_debug.c | 28 +++++++++++++++------
 1 file changed, 20 insertions(+), 8 deletions(-)

---
v3: add stacktrace to commit message and put Fixes tag first
v2: add Acked-by and Fixes tags to commit message

diff --git a/drivers/net/ethernet/qlogic/qed/qed_debug.c b/drivers/net/ethernet/qlogic/qed/qed_debug.c
index 86ecb080b153..cdcead614e9f 100644
--- a/drivers/net/ethernet/qlogic/qed/qed_debug.c
+++ b/drivers/net/ethernet/qlogic/qed/qed_debug.c
@@ -1830,11 +1830,12 @@ static void qed_grc_clear_all_prty(struct qed_hwfn *p_hwfn,
 /* Finds the meta data image in NVRAM */
 static enum dbg_status qed_find_nvram_image(struct qed_hwfn *p_hwfn,
 					    struct qed_ptt *p_ptt,
 					    u32 image_type,
 					    u32 *nvram_offset_bytes,
-					    u32 *nvram_size_bytes)
+					    u32 *nvram_size_bytes,
+					    bool b_can_sleep)
 {
 	u32 ret_mcp_resp, ret_mcp_param, ret_txn_size;
 	struct mcp_file_att file_att;
 	int nvm_result;
 
@@ -1844,11 +1845,12 @@ static enum dbg_status qed_find_nvram_image(struct qed_hwfn *p_hwfn,
 					DRV_MSG_CODE_NVM_GET_FILE_ATT,
 					image_type,
 					&ret_mcp_resp,
 					&ret_mcp_param,
 					&ret_txn_size,
-					(u32 *)&file_att, false);
+					(u32 *)&file_att,
+					b_can_sleep);
 
 	/* Check response */
 	if (nvm_result || (ret_mcp_resp & FW_MSG_CODE_MASK) !=
 	    FW_MSG_CODE_NVM_OK)
 		return DBG_STATUS_NVRAM_GET_IMAGE_FAILED;
@@ -1871,11 +1873,13 @@ static enum dbg_status qed_find_nvram_image(struct qed_hwfn *p_hwfn,
 
 /* Reads data from NVRAM */
 static enum dbg_status qed_nvram_read(struct qed_hwfn *p_hwfn,
 				      struct qed_ptt *p_ptt,
 				      u32 nvram_offset_bytes,
-				      u32 nvram_size_bytes, u32 *ret_buf)
+				      u32 nvram_size_bytes,
+				      u32 *ret_buf,
+				      bool b_can_sleep)
 {
 	u32 ret_mcp_resp, ret_mcp_param, ret_read_size, bytes_to_copy;
 	s32 bytes_left = nvram_size_bytes;
 	u32 read_offset = 0, param = 0;
 
@@ -1897,11 +1901,11 @@ static enum dbg_status qed_nvram_read(struct qed_hwfn *p_hwfn,
 		if (qed_mcp_nvm_rd_cmd(p_hwfn, p_ptt,
 				       DRV_MSG_CODE_NVM_READ_NVRAM, param,
 				       &ret_mcp_resp,
 				       &ret_mcp_param, &ret_read_size,
 				       (u32 *)((u8 *)ret_buf + read_offset),
-				       false))
+				       b_can_sleep))
 			return DBG_STATUS_NVRAM_READ_FAILED;
 
 		/* Check response */
 		if ((ret_mcp_resp & FW_MSG_CODE_MASK) != FW_MSG_CODE_NVM_OK)
 			return DBG_STATUS_NVRAM_READ_FAILED;
@@ -3378,11 +3382,12 @@ static u32 qed_grc_dump_mcp_hw_dump(struct qed_hwfn *p_hwfn,
 	/* Read HW dump image from NVRAM */
 	status = qed_find_nvram_image(p_hwfn,
 				      p_ptt,
 				      NVM_TYPE_HW_DUMP_OUT,
 				      &hw_dump_offset_bytes,
-				      &hw_dump_size_bytes);
+				      &hw_dump_size_bytes,
+				      false);
 	if (status != DBG_STATUS_OK)
 		return 0;
 
 	hw_dump_size_dwords = BYTES_TO_DWORDS(hw_dump_size_bytes);
 
@@ -3395,11 +3400,13 @@ static u32 qed_grc_dump_mcp_hw_dump(struct qed_hwfn *p_hwfn,
 	/* Read MCP HW dump image into dump buffer */
 	if (dump && hw_dump_size_dwords) {
 		status = qed_nvram_read(p_hwfn,
 					p_ptt,
 					hw_dump_offset_bytes,
-					hw_dump_size_bytes, dump_buf + offset);
+					hw_dump_size_bytes,
+					dump_buf + offset,
+					false);
 		if (status != DBG_STATUS_OK) {
 			DP_NOTICE(p_hwfn,
 				  "Failed to read MCP HW Dump image from NVRAM\n");
 			return 0;
 		}
@@ -4121,11 +4128,13 @@ static enum dbg_status qed_mcp_trace_get_meta_info(struct qed_hwfn *p_hwfn,
 	    (*running_bundle_id ==
 	     DIR_ID_1) ? NVM_TYPE_MFW_TRACE1 : NVM_TYPE_MFW_TRACE2;
 	return qed_find_nvram_image(p_hwfn,
 				    p_ptt,
 				    nvram_image_type,
-				    trace_meta_offset, trace_meta_size);
+				    trace_meta_offset,
+				    trace_meta_size,
+				    true);
 }
 
 /* Reads the MCP Trace meta data from NVRAM into the specified buffer */
 static enum dbg_status qed_mcp_trace_read_meta(struct qed_hwfn *p_hwfn,
 					       struct qed_ptt *p_ptt,
@@ -4137,11 +4146,14 @@ static enum dbg_status qed_mcp_trace_read_meta(struct qed_hwfn *p_hwfn,
 	u32 signature;
 
 	/* Read meta data from NVRAM */
 	status = qed_nvram_read(p_hwfn,
 				p_ptt,
-				nvram_offset_in_bytes, size_in_bytes, buf);
+				nvram_offset_in_bytes,
+				size_in_bytes,
+				buf,
+				true);
 	if (status != DBG_STATUS_OK)
 		return status;
 
 	/* Extract and check first signature */
 	signature = qed_read_unaligned_dword(byte_buf);
-- 
2.25.1

Re: [PATCH net v3] qed: allow sleep in qed_mcp_trace_dump()

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (master)
by Jakub Kicinski <kuba@kernel.org>:

On Tue,  3 Jan 2023 16:30:21 -0700 you wrote:
> By default, qed_mcp_cmd_and_union() delays 10us at a time in a loop
> that can run 500K times, so calls to qed_mcp_nvm_rd_cmd()
> may block the current thread for over 5s.
> We observed thread scheduling delays over 700ms in production,
> with stacktraces pointing to this code as the culprit.
> 
> qed_mcp_trace_dump() is called from ethtool, so sleeping is permitted.
> It already can sleep in qed_mcp_halt(), which calls qed_mcp_cmd().
> Add a "can sleep" parameter to qed_find_nvram_image() and
> qed_nvram_read() so they can sleep during qed_mcp_trace_dump().
> qed_mcp_trace_get_meta_info() and qed_mcp_trace_read_meta(),
> called only by qed_mcp_trace_dump(), allow these functions to sleep.
> I can't tell if the other caller (qed_grc_dump_mcp_hw_dump()) can sleep,
> so keep b_can_sleep set to false when it calls these functions.
> 
> [...]

Here is the summary with links:
  - [net,v3] qed: allow sleep in qed_mcp_trace_dump()
    https://git.kernel.org/netdev/net/c/5401c3e09928

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

RE: [PATCH] qed: allow sleep in qed_mcp_trace_dump()

[Alok Prasad]

> -----Original Message-----
> From: Caleb Sander <csander@purestorage.com>
> Sent: Saturday, December 17, 2022 7:56 PM
> To: Ariel Elior <aelior@marvell.com>; Manish Chopra 
> <manishc@marvell.com>; netdev@vger.kernel.org
> Cc: Joern Engel <joern@purestorage.com>; Caleb Sander 
> <csander@purestorage.com>
> Subject: [EXT] [PATCH] qed: allow sleep in qed_mcp_trace_dump()
> 
> External Email
> 
> ----------------------------------------------------------------------
> By default, qed_mcp_cmd_and_union() waits for 10us at a time in a loop 
> that can run 500K times, so calls to qed_mcp_nvm_rd_cmd() may block 
> the current thread for over 5s.
> We observed thread scheduling delays of over 700ms in production, with 
> stacktraces pointing to this code as the culprit.
> 
> qed_mcp_trace_dump() is called from ethtool, so sleeping is permitted.
> It already can sleep in qed_mcp_halt(), which calls qed_mcp_cmd().
> Add a "can sleep" parameter to qed_find_nvram_image() and
> qed_nvram_read() so they can sleep during qed_mcp_trace_dump().
> qed_mcp_trace_get_meta_info() and qed_mcp_trace_read_meta(), called 
> only by qed_mcp_trace_dump(), allow these functions to sleep.
> It's not clear to me that the other caller 
> (qed_grc_dump_mcp_hw_dump()) can sleep, so it keeps b_can_sleep set to false.
> 
> Signed-off-by: Caleb Sander <csander@purestorage.com>
> ---
>  drivers/net/ethernet/qlogic/qed/qed_debug.c | 28 +++++++++++++++-----
> -
>  1 file changed, 20 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/ethernet/qlogic/qed/qed_debug.c
> b/drivers/net/ethernet/qlogic/qed/qed_debug.c
> index 86ecb080b153..cdcead614e9f 100644
> --- a/drivers/net/ethernet/qlogic/qed/qed_debug.c
> +++ b/drivers/net/ethernet/qlogic/qed/qed_debug.c
> @@ -1830,11 +1830,12 @@ static void qed_grc_clear_all_prty(struct 
> qed_hwfn *p_hwfn,
>  /* Finds the meta data image in NVRAM */  static enum dbg_status 
> qed_find_nvram_image(struct qed_hwfn *p_hwfn,
>  					    struct qed_ptt *p_ptt,
>  					    u32 image_type,
>  					    u32 *nvram_offset_bytes,
> -					    u32 *nvram_size_bytes)
> +					    u32 *nvram_size_bytes,
> +					    bool b_can_sleep)
>  {
>  	u32 ret_mcp_resp, ret_mcp_param, ret_txn_size;
>  	struct mcp_file_att file_att;
>  	int nvm_result;
> 
> @@ -1844,11 +1845,12 @@ static enum dbg_status 
> qed_find_nvram_image(struct qed_hwfn *p_hwfn,
> 
> 	DRV_MSG_CODE_NVM_GET_FILE_ATT,
>  					image_type,
>  					&ret_mcp_resp,
>  					&ret_mcp_param,
>  					&ret_txn_size,
> -					(u32 *)&file_att, false);
> +					(u32 *)&file_att,
> +					b_can_sleep);
> 
>  	/* Check response */
>  	if (nvm_result || (ret_mcp_resp & FW_MSG_CODE_MASK) !=
>  	    FW_MSG_CODE_NVM_OK)
>  		return DBG_STATUS_NVRAM_GET_IMAGE_FAILED;
> @@ -1871,11 +1873,13 @@ static enum dbg_status 
> qed_find_nvram_image(struct qed_hwfn *p_hwfn,
> 
>  /* Reads data from NVRAM */
>  static enum dbg_status qed_nvram_read(struct qed_hwfn *p_hwfn,
>  				      struct qed_ptt *p_ptt,
>  				      u32 nvram_offset_bytes,
> -				      u32 nvram_size_bytes, u32 *ret_buf)
> +				      u32 nvram_size_bytes,
> +				      u32 *ret_buf,
> +				      bool b_can_sleep)
>  {
>  	u32 ret_mcp_resp, ret_mcp_param, ret_read_size, bytes_to_copy;
>  	s32 bytes_left = nvram_size_bytes;
>  	u32 read_offset = 0, param = 0;
> 
> @@ -1897,11 +1901,11 @@ static enum dbg_status qed_nvram_read(struct 
> qed_hwfn *p_hwfn,
>  		if (qed_mcp_nvm_rd_cmd(p_hwfn, p_ptt,
>  				       DRV_MSG_CODE_NVM_READ_NVRAM, param,
>  				       &ret_mcp_resp,
>  				       &ret_mcp_param, &ret_read_size,
>  				       (u32 *)((u8 *)ret_buf + read_offset),
> -				       false))
> +				       b_can_sleep))
>  			return DBG_STATUS_NVRAM_READ_FAILED;
> 
>  		/* Check response */
>  		if ((ret_mcp_resp & FW_MSG_CODE_MASK) !=
> FW_MSG_CODE_NVM_OK)
>  			return DBG_STATUS_NVRAM_READ_FAILED; @@ -3378,11 +3382,12 @@ 
> static u32 qed_grc_dump_mcp_hw_dump(struct qed_hwfn *p_hwfn,
>  	/* Read HW dump image from NVRAM */
>  	status = qed_find_nvram_image(p_hwfn,
>  				      p_ptt,
>  				      NVM_TYPE_HW_DUMP_OUT,
>  				      &hw_dump_offset_bytes,
> -				      &hw_dump_size_bytes);
> +				      &hw_dump_size_bytes,
> +				      false);
>  	if (status != DBG_STATUS_OK)
>  		return 0;
> 
>  	hw_dump_size_dwords =
> BYTES_TO_DWORDS(hw_dump_size_bytes);
> 
> @@ -3395,11 +3400,13 @@ static u32 qed_grc_dump_mcp_hw_dump(struct 
> qed_hwfn *p_hwfn,
>  	/* Read MCP HW dump image into dump buffer */
>  	if (dump && hw_dump_size_dwords) {
>  		status = qed_nvram_read(p_hwfn,
>  					p_ptt,
>  					hw_dump_offset_bytes,
> -					hw_dump_size_bytes, dump_buf +
> offset);
> +					hw_dump_size_bytes,
> +					dump_buf + offset,
> +					false);
>  		if (status != DBG_STATUS_OK) {
>  			DP_NOTICE(p_hwfn,
>  				  "Failed to read MCP HW Dump image from NVRAM\n");
>  			return 0;
>  		}
> @@ -4121,11 +4128,13 @@ static enum dbg_status 
> qed_mcp_trace_get_meta_info(struct qed_hwfn *p_hwfn,
>  	    (*running_bundle_id ==
>  	     DIR_ID_1) ? NVM_TYPE_MFW_TRACE1 :
> NVM_TYPE_MFW_TRACE2;
>  	return qed_find_nvram_image(p_hwfn,
>  				    p_ptt,
>  				    nvram_image_type,
> -				    trace_meta_offset, trace_meta_size);
> +				    trace_meta_offset,
> +				    trace_meta_size,
> +				    true);
>  }
> 
>  /* Reads the MCP Trace meta data from NVRAM into the specified buffer 
> */  static enum dbg_status qed_mcp_trace_read_meta(struct qed_hwfn 
> *p_hwfn,
>  					       struct qed_ptt *p_ptt,
> @@ -4137,11 +4146,14 @@ static enum dbg_status 
> qed_mcp_trace_read_meta(struct qed_hwfn *p_hwfn,
>  	u32 signature;
> 
>  	/* Read meta data from NVRAM */
>  	status = qed_nvram_read(p_hwfn,
>  				p_ptt,
> -				nvram_offset_in_bytes, size_in_bytes, buf);
> +				nvram_offset_in_bytes,
> +				size_in_bytes,
> +				buf,
> +				true);
>  	if (status != DBG_STATUS_OK)
>  		return status;
> 
>  	/* Extract and check first signature */
>  	signature = qed_read_unaligned_dword(byte_buf);
> --
> 2.25.1

Thanks,

Acked-by: Alok Prasad <palok@marvell.com>