[PATCH ethtool] bnxt: Add Broadcom driver support.

[PATCH ethtool] bnxt: Add Broadcom driver support.

[Vasundhara Volam]

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@broadcom.com>
Reviewed-by: Edwin Peer <edwin.peer@broadcom.com>
Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.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" },
+	{ .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" },
+	{ .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"},
+};
+
+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);
+		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);
+		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));
+
+		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

Re: [PATCH ethtool] bnxt: Add Broadcom driver support.

[Michal Kubecek]

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@broadcom.com>
> Reviewed-by: Edwin Peer <edwin.peer@broadcom.com>
> Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.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
> 

Re: [PATCH ethtool] bnxt: Add Broadcom driver support.

[Andrew Lunn]

> +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" },

These look like statistics. Could they be part of ethtool -S

      Andrew

Re: [PATCH ethtool] bnxt: Add Broadcom driver support.

[Vasundhara Volam]

On Mon, Sep 21, 2020 at 2:48 PM Michal Kubecek <mkubecek@suse.cz> wrote:
>
> 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@broadcom.com>
> > Reviewed-by: Edwin Peer <edwin.peer@broadcom.com>
> > Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.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().
These are unsigned only. I will fix the format. Thanks.
>
> > +     { .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".
okay.
>
> > +     { .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?
Okay, I will use tabs "\t". I cannot really use width specifiers as I
have to use 0x in front of the values.
>
> > +
> > +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".)
Okay.
>
> > +             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.
Okay.
>
> > +             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.
Data is already converted to host endian order by ETHTOOL_REGS, so it
will not be an issue.
>
> 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
> >

Re: [PATCH ethtool] bnxt: Add Broadcom driver support.

[Vasundhara Volam]

On Mon, Sep 21, 2020 at 6:11 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> > +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" },
>
> These look like statistics. Could they be part of ethtool -S
Only a couple of PCIe statistics look like counters. But to have all
PCIe statistics (most of them are not plain counters) at one place,
dumping them in the register dump. Thanks.
>
>       Andrew

Re: [PATCH ethtool] bnxt: Add Broadcom driver support.

[Michal Kubecek]

On Tue, Sep 22, 2020 at 11:24:24AM +0530, Vasundhara Volam wrote:
> On Mon, Sep 21, 2020 at 2:48 PM Michal Kubecek <mkubecek@suse.cz> wrote:
> > > +             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.
> 
> Data is already converted to host endian order by ETHTOOL_REGS, so it
> will not be an issue.

It does not work correctly. Assume we are on big endian architecture and
are reading a 16-bit value (stats[i].size = 1) 0x1234 which is laid out
in memory as

    ... 12 34 ...

Copying that by memcpy() to the address of 64-bit pcie_stat, you get

   12 34 00 00 00 00 00 00

which represents 0x1234000000000000, not 0x1234. You will also have the
same problem with 32-bit values (stats[i].size = 2).

Michal

Re: [PATCH ethtool] bnxt: Add Broadcom driver support.

[Vasundhara Volam]

On Tue, Sep 22, 2020 at 12:22 PM Michal Kubecek <mkubecek@suse.cz> wrote:
>
> On Tue, Sep 22, 2020 at 11:24:24AM +0530, Vasundhara Volam wrote:
> > On Mon, Sep 21, 2020 at 2:48 PM Michal Kubecek <mkubecek@suse.cz> wrote:
> > > > +             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.
> >
> > Data is already converted to host endian order by ETHTOOL_REGS, so it
> > will not be an issue.
>
> It does not work correctly. Assume we are on big endian architecture and
> are reading a 16-bit value (stats[i].size = 1) 0x1234 which is laid out
> in memory as
>
>     ... 12 34 ...
>
> Copying that by memcpy() to the address of 64-bit pcie_stat, you get
>
>    12 34 00 00 00 00 00 00
>
> which represents 0x1234000000000000, not 0x1234. You will also have the
> same problem with 32-bit values (stats[i].size = 2).
You are right. I understood the issue now.

I will modify it to use different size variables based on the size and
convert it to a switch statement.

Thanks.
>
> Michal