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  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]
Date:   Mon, 21 Sep 2020 11:18:20 +0200
From:   Michal Kubecek <mkubecek@...e.cz>
To:     Vasundhara Volam <vasundhara-v.volam@...adcom.com>
Cc:     davem@...emloft.net, netdev@...r.kernel.org, kuba@...nel.org,
        michael.chan@...adcom.com, edwin.peer@...adcom.com,
        andrew.gospodarek@...adcom.com
Subject: Re: [PATCH ethtool] bnxt: Add Broadcom driver support.

On Mon, Sep 21, 2020 at 12:09:51PM +0530, Vasundhara Volam wrote:
> This patch adds the initial support for parsing registers dumped
> by the Broadcom driver. Currently, PXP and PCIe registers are
> parsed.
> 
> Reviewed-by: Andy Gospodarek <gospo@...adcom.com>
> Reviewed-by: Edwin Peer <edwin.peer@...adcom.com>
> Signed-off-by: Vasundhara Volam <vasundhara-v.volam@...adcom.com>
> ---
>  Makefile.am |  2 +-
>  bnxt.c      | 86 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  ethtool.c   |  1 +
>  internal.h  |  3 +++
>  4 files changed, 91 insertions(+), 1 deletion(-)
>  create mode 100644 bnxt.c
> 
> diff --git a/Makefile.am b/Makefile.am
> index 0e237d0..e3e311d 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -17,7 +17,7 @@ ethtool_SOURCES += \
>  		  smsc911x.c at76c50x-usb.c sfc.c stmmac.c	\
>  		  sff-common.c sff-common.h sfpid.c sfpdiag.c	\
>  		  ixgbevf.c tse.c vmxnet3.c qsfp.c qsfp.h fjes.c lan78xx.c \
> -		  igc.c qsfp-dd.c qsfp-dd.h
> +		  igc.c qsfp-dd.c qsfp-dd.h bnxt.c
>  endif
>  
>  if ENABLE_BASH_COMPLETION
> diff --git a/bnxt.c b/bnxt.c
> new file mode 100644
> index 0000000..91ed819
> --- /dev/null
> +++ b/bnxt.c
> @@ -0,0 +1,86 @@
> +/* Code to dump registers for NetXtreme-E/NetXtreme-C Broadcom devices.
> + *
> + * Copyright (c) 2020 Broadcom Inc.
> + */
> +#include <stdio.h>
> +#include "internal.h"
> +
> +#define BNXT_PXP_REG_LEN	0x3110
> +#define BNXT_PCIE_STATS_LEN	(12 * sizeof(u64))
> +
> +struct bnxt_pcie_stat {
> +	const char *name;
> +	u16 offset;
> +	u8 size;
> +	const char *format;
> +};
> +
> +static const struct bnxt_pcie_stat bnxt_pcie_stats[] = {
> +	{ .name = "PL Signal integrity errors     ", .offset = 0, .size = 4, .format = "%lld" },
> +	{ .name = "DL Signal integrity errors     ", .offset = 4, .size = 4, .format = "%lld" },
> +	{ .name = "TLP Signal integrity errors    ", .offset = 8, .size = 4, .format = "%lld" },
> +	{ .name = "Link integrity                 ", .offset = 12, .size = 4, .format = "%lld" },
> +	{ .name = "TX TLP traffic rate            ", .offset = 16, .size = 4, .format = "%lld" },
> +	{ .name = "RX TLP traffic rate            ", .offset = 20, .size = 4, .format = "%lld" },
> +	{ .name = "TX DLLP traffic rate           ", .offset = 24, .size = 4, .format = "%lld" },
> +	{ .name = "RX DLLP traffic rate           ", .offset = 28, .size = 4, .format = "%lld" },

Are all of these really interpreted as signed? Moreover, you are always
passing a u64 varable to printf().

> +	{ .name = "Equalization Phase 0 time(ms)  ", .offset = 33, .size = 1, .format = "0x%lx" },
> +	{ .name = "Equalization Phase 1 time(ms)  ", .offset = 32, .size = 1, .format = "0x%lx" },
> +	{ .name = "Equalization Phase 2 time(ms)  ", .offset = 35, .size = 1, .format = "0x%lx" },
> +	{ .name = "Equalization Phase 3 time(ms)  ", .offset = 34, .size = 1, .format = "0x%lx" },

Again, you are always passing a u64 variable so the format should rather
be "0x%llx".

> +	{ .name = "PHY LTSSM Histogram 0          ", .offset = 36, .size = 2, .format = "0x%llx"},
> +	{ .name = "PHY LTSSM Histogram 1          ", .offset = 38, .size = 2, .format = "0x%llx"},
> +	{ .name = "PHY LTSSM Histogram 2          ", .offset = 40, .size = 2, .format = "0x%llx"},
> +	{ .name = "PHY LTSSM Histogram 3          ", .offset = 42, .size = 2, .format = "0x%llx"},
> +	{ .name = "Recovery Histogram 0           ", .offset = 44, .size = 2, .format = "0x%llx"},
> +	{ .name = "Recovery Histogram 1           ", .offset = 46, .size = 2, .format = "0x%llx"},
> +};

I don't really like the trailing spaces in register names; why don't you
use printf() format for column alignment?

> +
> +int bnxt_dump_regs(struct ethtool_drvinfo *info __maybe_unused, struct ethtool_regs *regs)
> +{
> +	const struct bnxt_pcie_stat *stats = bnxt_pcie_stats;
> +	u16 *pcie_stats;
> +	u64 pcie_stat;
> +	u32 reg, i;
> +
> +	if (regs->len < BNXT_PXP_REG_LEN) {
> +		fprintf(stdout, "Length too short, expected atleast %x\n",
> +			BNXT_PXP_REG_LEN);

This will show "...atleast 3110" which is rather confusing without the
"0x" prefix. (Also, a space is missing in "atleast".)

> +		return -1;
> +	}
> +
> +	fprintf(stdout, "PXP Registers\n");
> +	fprintf(stdout, "Offset\tValue\n");
> +	fprintf(stdout, "------\t-------\n");
> +	for (i = 0; i < BNXT_PXP_REG_LEN; i += sizeof(reg)) {
> +		memcpy(&reg, &regs->data[i], sizeof(reg));
> +		if (reg)
> +			fprintf(stdout, "0x%04x\t0x%08x\n", i, reg);
> +	}
> +	fprintf(stdout, "\n");
> +
> +	if (!regs->version)
> +		return 0;
> +
> +	if (regs->len < (BNXT_PXP_REG_LEN + BNXT_PCIE_STATS_LEN)) {
> +		fprintf(stdout, "Length is too short, expected %lx\n",
> +			BNXT_PXP_REG_LEN + BNXT_PCIE_STATS_LEN);

The same problem here, "3170" actually meaning 0x3170 or 12656.

> +		return -1;
> +	}
> +
> +	pcie_stats = (u16 *)(regs->data + BNXT_PXP_REG_LEN);
> +	fprintf(stdout, "PCIe statistics:\n");
> +	fprintf(stdout, "----------------\n");
> +	for (i = 0; i < ARRAY_SIZE(bnxt_pcie_stats); i++) {
> +		pcie_stat = 0;
> +		memcpy(&pcie_stat, &pcie_stats[stats[i].offset],
> +		       stats[i].size * sizeof(u16));

This will only work on little endian architectures.

Michal

> +
> +		fprintf(stdout, "%s", stats[i].name);
> +		fprintf(stdout, stats[i].format, pcie_stat);
> +		fprintf(stdout, "\n");
> +	}
> +
> +	fprintf(stdout, "\n");
> +	return 0;
> +}
> diff --git a/ethtool.c b/ethtool.c
> index ab9b457..89bd15c 100644
> --- a/ethtool.c
> +++ b/ethtool.c
> @@ -1072,6 +1072,7 @@ static const struct {
>  	{ "dsa", dsa_dump_regs },
>  	{ "fec", fec_dump_regs },
>  	{ "igc", igc_dump_regs },
> +	{ "bnxt_en", bnxt_dump_regs },
>  #endif
>  };
>  
> diff --git a/internal.h b/internal.h
> index d096a28..935ebac 100644
> --- a/internal.h
> +++ b/internal.h
> @@ -396,4 +396,7 @@ int fec_dump_regs(struct ethtool_drvinfo *info, struct ethtool_regs *regs);
>  /* Intel(R) Ethernet Controller I225-LM/I225-V adapter family */
>  int igc_dump_regs(struct ethtool_drvinfo *info, struct ethtool_regs *regs);
>  
> +/* Broadcom Ethernet Controller */
> +int bnxt_dump_regs(struct ethtool_drvinfo *info, struct ethtool_regs *regs);
> +
>  #endif /* ETHTOOL_INTERNAL_H__ */
> -- 
> 1.8.3.1
> 

Powered by blists - more mailing lists