| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <895f8e49-2bbd-4211-b1a5-6708956a580a@intel.com>
Date: Mon, 19 Aug 2024 12:12:39 +0200
From: Przemek Kitszel <przemyslaw.kitszel@...el.com>
To: Michael Chan <michael.chan@...adcom.com>
CC: <netdev@...r.kernel.org>, <edumazet@...gle.com>, <kuba@...nel.org>,
<pabeni@...hat.com>, <pavan.chebbi@...adcom.com>,
<andrew.gospodarek@...adcom.com>, <horms@...nel.org>, <helgaas@...nel.org>,
Vikas Gupta <vikas.gupta@...adcom.com>, Somnath Kotur
<somnath.kotur@...adcom.com>, <davem@...emloft.net>
Subject: Re: [PATCH net-next v2 2/9] bnxt_en: add support for retrieving crash
dump using ethtool
On 8/16/24 23:28, Michael Chan wrote:
> From: Vikas Gupta <vikas.gupta@...adcom.com>
>
> Add support for retrieving crash dump using ethtool -w on the
> supported interface.
>
> Reviewed-by: Somnath Kotur <somnath.kotur@...adcom.com>
> Reviewed-by: Pavan Chebbi <pavan.chebbi@...adcom.com>
> Reviewed-by: Andy Gospodarek <andrew.gospodarek@...adcom.com>
> Reviewed-by: Simon Horman <horms@...nel.org>
> Signed-off-by: Vikas Gupta <vikas.gupta@...adcom.com>
> Signed-off-by: Michael Chan <michael.chan@...adcom.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;
Powered by blists - more mailing lists