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
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <c3946cf2-c89b-46ad-85d2-1df5d8c4db4c@intel.com>
Date: Mon, 19 Aug 2024 12:00:05 +0200
From: Przemek Kitszel <przemyslaw.kitszel@...el.com>
To: Michael Chan <michael.chan@...adcom.com>
CC: <netdev@...r.kernel.org>, <edumazet@...gle.com>, <davem@...emloft.net>,
	<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>
Subject: Re: [PATCH net-next v2 1/9] bnxt_en: add support for storing crash
 dump into host memory

On 8/16/24 23:28, Michael Chan wrote:
> From: Vikas Gupta <vikas.gupta@...adcom.com>
> 
> Newer firmware supports automatic DMA of crash dump to host memory
> when it crashes.  If the feature is supported, allocate the required
> memory using the existing context memory infrastructure.  Communicate
> the page table containing the DMA addresses to the firmware.
> 
> 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>
> ---
>   drivers/net/ethernet/broadcom/bnxt/bnxt.c     | 93 +++++++++++++++++++
>   drivers/net/ethernet/broadcom/bnxt/bnxt.h     |  3 +
>   .../ethernet/broadcom/bnxt/bnxt_coredump.c    | 18 +++-
>   .../ethernet/broadcom/bnxt/bnxt_coredump.h    |  8 ++
>   4 files changed, 117 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> index 75e2cfed5769..017eefd78ba4 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> @@ -69,6 +69,7 @@
>   #include "bnxt_tc.h"
>   #include "bnxt_devlink.h"
>   #include "bnxt_debugfs.h"
> +#include "bnxt_coredump.h"
>   #include "bnxt_hwmon.h"
>   
>   #define BNXT_TX_TIMEOUT		(5 * HZ)
> @@ -8946,6 +8947,82 @@ static int bnxt_alloc_ctx_mem(struct bnxt *bp)
>   	return 0;
>   }
>   
> +#define BNXT_SET_CRASHDUMP_PAGE_ATTR(attr)				\
> +do {									\
> +	if (BNXT_PAGE_SIZE == 0x2000)					\
> +		attr = DBG_CRASHDUMP_MEDIUM_CFG_REQ_PG_SIZE_PG_8K;	\
> +	else if (BNXT_PAGE_SIZE == 0x10000)				\
> +		attr = DBG_CRASHDUMP_MEDIUM_CFG_REQ_PG_SIZE_PG_64K;	\
> +	else								\
> +		attr = DBG_CRASHDUMP_MEDIUM_CFG_REQ_PG_SIZE_PG_4K;	\
> +} while (0)
> +
> +static int bnxt_hwrm_crash_dump_mem_cfg(struct bnxt *bp)
> +{
> +	struct hwrm_dbg_crashdump_medium_cfg_input *req;
> +	u16 page_attr = 0;

redundant assignement

> +	int rc;
> +
> +	if (!(bp->fw_dbg_cap & DBG_QCAPS_RESP_FLAGS_CRASHDUMP_HOST_DDR))
> +		return 0;
> +
> +	rc = hwrm_req_init(bp, req, HWRM_DBG_CRASHDUMP_MEDIUM_CFG);
> +	if (rc)
> +		return rc;
> +
> +	BNXT_SET_CRASHDUMP_PAGE_ATTR(page_attr);

I would avoid introducing a macro for just one use here

> +	req->pg_size_lvl = cpu_to_le16(page_attr |
> +				       bp->fw_crash_mem->ring_mem.depth);
> +	req->pbl = cpu_to_le64(bp->fw_crash_mem->ring_mem.pg_tbl_map);
> +	req->size = cpu_to_le32(bp->fw_crash_len);
> +	req->output_dest_flags = cpu_to_le16(BNXT_DBG_CR_DUMP_MDM_CFG_DDR);
> +	return hwrm_req_send(bp, req);
> +}
> +
> +static void bnxt_free_crash_dump_mem(struct bnxt *bp)
> +{
> +	if (bp->fw_crash_mem) {
> +		bnxt_free_ctx_pg_tbls(bp, bp->fw_crash_mem);
> +		kfree(bp->fw_crash_mem);
> +		bp->fw_crash_len = 0;
> +		bp->fw_crash_mem = NULL;

it should be sufficient to have just one variable cleared for the
purpose of "no crash mem allocated yet" detection

> +	}
> +}
> +
> +static int bnxt_alloc_crash_dump_mem(struct bnxt *bp)
> +{
> +	u32 mem_size = 0;
> +	int rc;
> +
> +	if (!(bp->fw_dbg_cap & DBG_QCAPS_RESP_FLAGS_CRASHDUMP_HOST_DDR))
> +		return 0;
> +
> +	rc = bnxt_hwrm_get_dump_len(bp, BNXT_DUMP_CRASH, &mem_size);
> +	if (rc)
> +		return rc;
> +
> +	mem_size = round_up(mem_size, 4);
> +
> +	if (bp->fw_crash_mem && mem_size == bp->fw_crash_len)
> +		return 0;
> +
> +	bnxt_free_crash_dump_mem(bp);

I would say it would be better to have the old buffer still allocated
in case of an allocation failure for the new one (like krealloc()).
Especially in case of shrink request.

> +
> +	bp->fw_crash_mem = kzalloc(sizeof(*bp->fw_crash_mem), GFP_KERNEL);

kmalloc(), assuming that you don't depend on the pages to be cleared

> +	if (!bp->fw_crash_mem)
> +		return -ENOMEM;
> +
> +	rc = bnxt_alloc_ctx_pg_tbls(bp, bp->fw_crash_mem, mem_size, 1, NULL);
> +	if (rc) {
> +		bnxt_free_crash_dump_mem(bp);
> +		return rc;
> +	}
> +
> +	bp->fw_crash_len = mem_size;
> +
> +	return 0;
> +}

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ