diff mbox series

[net-next,v2,2/9] bnxt_en: add support for retrieving crash dump using ethtool

Message ID 20240816212832.185379-3-michael.chan@broadcom.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series bnxt_en: Update for net-next | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 16 this patch: 16
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers success CCed 4 of 4 maintainers
netdev/build_clang success Errors and warnings before: 16 this patch: 16
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 17 this patch: 17
netdev/checkpatch warning WARNING: line length of 81 exceeds 80 columns WARNING: line length of 89 exceeds 80 columns
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
netdev/contest success net-next-2024-08-17--00-00 (tests: 710)

Commit Message

Michael Chan Aug. 16, 2024, 9:28 p.m. UTC 
From: Vikas Gupta <vikas.gupta@broadcom.com>

Add support for retrieving crash dump using ethtool -w on the
supported interface.

Reviewed-by: Somnath Kotur <somnath.kotur@broadcom.com>
Reviewed-by: Pavan Chebbi <pavan.chebbi@broadcom.com>
Reviewed-by: Andy Gospodarek <andrew.gospodarek@broadcom.com>
Reviewed-by: Simon Horman <horms@kernel.org>
Signed-off-by: Vikas Gupta <vikas.gupta@broadcom.com>
Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
 .../ethernet/broadcom/bnxt/bnxt_coredump.c    | 83 +++++++++++++++++--
 .../net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 13 ++-
 2 files changed, 87 insertions(+), 9 deletions(-)

Comments

Przemek Kitszel Aug. 19, 2024, 10:12 a.m. UTC | #1 
On 8/16/24 23:28, Michael Chan wrote:
> From: Vikas Gupta <vikas.gupta@broadcom.com>
> 
> Add support for retrieving crash dump using ethtool -w on the
> supported interface.
> 
> Reviewed-by: Somnath Kotur <somnath.kotur@broadcom.com>
> Reviewed-by: Pavan Chebbi <pavan.chebbi@broadcom.com>
> Reviewed-by: Andy Gospodarek <andrew.gospodarek@broadcom.com>
> Reviewed-by: Simon Horman <horms@kernel.org>
> Signed-off-by: Vikas Gupta <vikas.gupta@broadcom.com>
> Signed-off-by: Michael Chan <michael.chan@broadcom.com>
> ---
>   .../ethernet/broadcom/bnxt/bnxt_coredump.c    | 83 +++++++++++++++++--
>   .../net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 13 ++-
>   2 files changed, 87 insertions(+), 9 deletions(-)
> 

mostly nitpicks from me here

> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_coredump.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_coredump.c
> index ebbad9ccab6a..9ed915e4c618 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_coredump.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_coredump.c
> @@ -372,14 +372,78 @@ static int __bnxt_get_coredump(struct bnxt *bp, void *buf, u32 *dump_len)
>   	return rc;
>   }
>   
> +static u32 bnxt_copy_crash_data(struct bnxt_ring_mem_info *rmem, void *buf,
> +				u32 dump_len)
> +{
> +	u32 data_copied = 0;
> +	u32 data_len;
> +	int i;
> +
> +	for (i = 0; i < rmem->nr_pages; i++) {
> +		data_len = rmem->page_size;
> +		if (data_copied + data_len > dump_len)
> +			data_len = dump_len - data_copied;
> +		memcpy(buf + data_copied, rmem->pg_arr[i], data_len);
> +		data_copied += data_len;
> +		if (data_copied >= dump_len)

==, but not big deal ;)

> +			break;
> +	}
> +	return data_copied;
> +}
> +
> +static int bnxt_copy_crash_dump(struct bnxt *bp, void *buf, u32 dump_len)
> +{
> +	struct bnxt_ring_mem_info *rmem;
> +	u32 offset = 0;
> +
> +	if (!bp->fw_crash_mem)
> +		return -EEXIST;

I would interpret this as "dump already taken, no slot for a new one",
while you just don't have mem allocated for it, so:
ENOENT 2 No such file or directory
or similar

> +
> +	rmem = &bp->fw_crash_mem->ring_mem;
> +
> +	if (rmem->depth > 1) {
> +		int i;
> +
> +		for (i = 0; i < rmem->nr_pages; i++) {
> +			struct bnxt_ctx_pg_info *pg_tbl;
> +
> +			pg_tbl = bp->fw_crash_mem->ctx_pg_tbl[i];
> +			offset += bnxt_copy_crash_data(&pg_tbl->ring_mem,
> +						       buf + offset,
> +						       dump_len - offset);
> +			if (offset >= dump_len)
> +				break;
> +		}
> +	} else {
> +		bnxt_copy_crash_data(rmem, buf, dump_len);
> +	}
> +
> +	return 0;
> +}
> +
> +static bool bnxt_crash_dump_avail(struct bnxt *bp)
> +{
> +	u32 sig = 0; > +
> +	/* First 4 bytes(signature) of crash dump is always non-zero */
> +	bnxt_copy_crash_dump(bp, &sig, sizeof(u32));

sizeof(sig)

> +	if (!sig)
> +		return false;
> +
> +	return true;

return sig

> +}
> +
>   int bnxt_get_coredump(struct bnxt *bp, u16 dump_type, void *buf, u32 *dump_len)
>   {
>   	if (dump_type == BNXT_DUMP_CRASH) {
> +		if (bp->fw_dbg_cap & DBG_QCAPS_RESP_FLAGS_CRASHDUMP_HOST_DDR)
> +			return bnxt_copy_crash_dump(bp, buf, *dump_len);
>   #ifdef CONFIG_TEE_BNXT_FW
> -		return tee_bnxt_copy_coredump(buf, 0, *dump_len);
> -#else
> -		return -EOPNOTSUPP;
> +		else if (bp->fw_dbg_cap & DBG_QCAPS_RESP_FLAGS_CRASHDUMP_SOC_DDR)
> +			return tee_bnxt_copy_coredump(buf, 0, *dump_len);
>   #endif
> +		else
> +			return -EOPNOTSUPP;
>   	} else {
>   		return __bnxt_get_coredump(bp, buf, dump_len);
>   	}
> @@ -442,10 +506,17 @@ u32 bnxt_get_coredump_length(struct bnxt *bp, u16 dump_type)
>   {
>   	u32 len = 0;
>   
> +	if (dump_type == BNXT_DUMP_CRASH &&
> +	    bp->fw_dbg_cap & DBG_QCAPS_RESP_FLAGS_CRASHDUMP_HOST_DDR &&

I would wrap bitwise-and with parenthesis, even if not mandated by the C
standard (the same in other places with combined expressions).

> +	    bp->fw_crash_mem) {
> +		if (!bnxt_crash_dump_avail(bp))
> +			return 0;
> +
> +		return bp->fw_crash_len;
> +	}
> +
>   	if (bnxt_hwrm_get_dump_len(bp, dump_type, &len)) {
> -		if (dump_type == BNXT_DUMP_CRASH)
> -			len = BNXT_CRASH_DUMP_LEN;
> -		else
> +		if (dump_type != BNXT_DUMP_CRASH)
>   			__bnxt_get_coredump(bp, NULL, &len);
>   	}
>   	return len;
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> index 39eed5831e3a..265956c45ff5 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> @@ -4993,9 +4993,16 @@ static int bnxt_set_dump(struct net_device *dev, struct ethtool_dump *dump)
>   		return -EINVAL;
>   	}
>   
> -	if (!IS_ENABLED(CONFIG_TEE_BNXT_FW) && dump->flag == BNXT_DUMP_CRASH) {
> -		netdev_info(dev, "Cannot collect crash dump as TEE_BNXT_FW config option is not enabled.\n");
> -		return -EOPNOTSUPP;
> +	if (dump->flag == BNXT_DUMP_CRASH) {
> +		if (bp->fw_dbg_cap & DBG_QCAPS_RESP_FLAGS_CRASHDUMP_SOC_DDR &&
> +		    (!IS_ENABLED(CONFIG_TEE_BNXT_FW))) {
> +			netdev_info(dev,
> +				    "Cannot collect crash dump as TEE_BNXT_FW config option is not enabled.\n");
> +			return -EOPNOTSUPP;
> +		} else if (!(bp->fw_dbg_cap & DBG_QCAPS_RESP_FLAGS_CRASHDUMP_HOST_DDR)) {
> +			netdev_info(dev, "Crash dump collection from host memory is not supported on this interface.\n");
> +			return -EOPNOTSUPP;
> +		}
>   	}
>   
>   	bp->dump_flag = dump->flag;
Jakub Kicinski Aug. 20, 2024, 1:41 a.m. UTC | #2 
On Mon, 19 Aug 2024 12:12:39 +0200 Przemek Kitszel wrote:
> > +	if (dump_type == BNXT_DUMP_CRASH &&
> > +	    bp->fw_dbg_cap & DBG_QCAPS_RESP_FLAGS_CRASHDUMP_HOST_DDR &&  
> 
> I would wrap bitwise-and with parenthesis, even if not mandated by the C
> standard (the same in other places with combined expressions).

Personal preference, I guess. I generally lean on the side of limiting
the brackets.
diff mbox series

Patch

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_coredump.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_coredump.c
index ebbad9ccab6a..9ed915e4c618 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_coredump.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_coredump.c
@@ -372,14 +372,78 @@  static int __bnxt_get_coredump(struct bnxt *bp, void *buf, u32 *dump_len)
 	return rc;
 }
 
+static u32 bnxt_copy_crash_data(struct bnxt_ring_mem_info *rmem, void *buf,
+				u32 dump_len)
+{
+	u32 data_copied = 0;
+	u32 data_len;
+	int i;
+
+	for (i = 0; i < rmem->nr_pages; i++) {
+		data_len = rmem->page_size;
+		if (data_copied + data_len > dump_len)
+			data_len = dump_len - data_copied;
+		memcpy(buf + data_copied, rmem->pg_arr[i], data_len);
+		data_copied += data_len;
+		if (data_copied >= dump_len)
+			break;
+	}
+	return data_copied;
+}
+
+static int bnxt_copy_crash_dump(struct bnxt *bp, void *buf, u32 dump_len)
+{
+	struct bnxt_ring_mem_info *rmem;
+	u32 offset = 0;
+
+	if (!bp->fw_crash_mem)
+		return -EEXIST;
+
+	rmem = &bp->fw_crash_mem->ring_mem;
+
+	if (rmem->depth > 1) {
+		int i;
+
+		for (i = 0; i < rmem->nr_pages; i++) {
+			struct bnxt_ctx_pg_info *pg_tbl;
+
+			pg_tbl = bp->fw_crash_mem->ctx_pg_tbl[i];
+			offset += bnxt_copy_crash_data(&pg_tbl->ring_mem,
+						       buf + offset,
+						       dump_len - offset);
+			if (offset >= dump_len)
+				break;
+		}
+	} else {
+		bnxt_copy_crash_data(rmem, buf, dump_len);
+	}
+
+	return 0;
+}
+
+static bool bnxt_crash_dump_avail(struct bnxt *bp)
+{
+	u32 sig = 0;
+
+	/* First 4 bytes(signature) of crash dump is always non-zero */
+	bnxt_copy_crash_dump(bp, &sig, sizeof(u32));
+	if (!sig)
+		return false;
+
+	return true;
+}
+
 int bnxt_get_coredump(struct bnxt *bp, u16 dump_type, void *buf, u32 *dump_len)
 {
 	if (dump_type == BNXT_DUMP_CRASH) {
+		if (bp->fw_dbg_cap & DBG_QCAPS_RESP_FLAGS_CRASHDUMP_HOST_DDR)
+			return bnxt_copy_crash_dump(bp, buf, *dump_len);
 #ifdef CONFIG_TEE_BNXT_FW
-		return tee_bnxt_copy_coredump(buf, 0, *dump_len);
-#else
-		return -EOPNOTSUPP;
+		else if (bp->fw_dbg_cap & DBG_QCAPS_RESP_FLAGS_CRASHDUMP_SOC_DDR)
+			return tee_bnxt_copy_coredump(buf, 0, *dump_len);
 #endif
+		else
+			return -EOPNOTSUPP;
 	} else {
 		return __bnxt_get_coredump(bp, buf, dump_len);
 	}
@@ -442,10 +506,17 @@  u32 bnxt_get_coredump_length(struct bnxt *bp, u16 dump_type)
 {
 	u32 len = 0;
 
+	if (dump_type == BNXT_DUMP_CRASH &&
+	    bp->fw_dbg_cap & DBG_QCAPS_RESP_FLAGS_CRASHDUMP_HOST_DDR &&
+	    bp->fw_crash_mem) {
+		if (!bnxt_crash_dump_avail(bp))
+			return 0;
+
+		return bp->fw_crash_len;
+	}
+
 	if (bnxt_hwrm_get_dump_len(bp, dump_type, &len)) {
-		if (dump_type == BNXT_DUMP_CRASH)
-			len = BNXT_CRASH_DUMP_LEN;
-		else
+		if (dump_type != BNXT_DUMP_CRASH)
 			__bnxt_get_coredump(bp, NULL, &len);
 	}
 	return len;
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
index 39eed5831e3a..265956c45ff5 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
@@ -4993,9 +4993,16 @@  static int bnxt_set_dump(struct net_device *dev, struct ethtool_dump *dump)
 		return -EINVAL;
 	}
 
-	if (!IS_ENABLED(CONFIG_TEE_BNXT_FW) && dump->flag == BNXT_DUMP_CRASH) {
-		netdev_info(dev, "Cannot collect crash dump as TEE_BNXT_FW config option is not enabled.\n");
-		return -EOPNOTSUPP;
+	if (dump->flag == BNXT_DUMP_CRASH) {
+		if (bp->fw_dbg_cap & DBG_QCAPS_RESP_FLAGS_CRASHDUMP_SOC_DDR &&
+		    (!IS_ENABLED(CONFIG_TEE_BNXT_FW))) {
+			netdev_info(dev,
+				    "Cannot collect crash dump as TEE_BNXT_FW config option is not enabled.\n");
+			return -EOPNOTSUPP;
+		} else if (!(bp->fw_dbg_cap & DBG_QCAPS_RESP_FLAGS_CRASHDUMP_HOST_DDR)) {
+			netdev_info(dev, "Crash dump collection from host memory is not supported on this interface.\n");
+			return -EOPNOTSUPP;
+		}
 	}
 
 	bp->dump_flag = dump->flag;