* [PATCH ethtool] bnxt: Add Broadcom driver support. @ 2020-09-21 6:39 Vasundhara Volam 2020-09-21 9:18 ` Michal Kubecek 2020-09-21 12:41 ` Andrew Lunn 0 siblings, 2 replies; 7+ messages in thread From: Vasundhara Volam @ 2020-09-21 6:39 UTC (permalink / raw) To: mkubecek, davem Cc: netdev, kuba, michael.chan, edwin.peer, andrew.gospodarek, 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(®, ®s->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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH ethtool] bnxt: Add Broadcom driver support. 2020-09-21 6:39 [PATCH ethtool] bnxt: Add Broadcom driver support Vasundhara Volam @ 2020-09-21 9:18 ` Michal Kubecek 2020-09-22 5:54 ` Vasundhara Volam 2020-09-21 12:41 ` Andrew Lunn 1 sibling, 1 reply; 7+ messages in thread From: Michal Kubecek @ 2020-09-21 9:18 UTC (permalink / raw) To: Vasundhara Volam Cc: davem, netdev, kuba, michael.chan, edwin.peer, andrew.gospodarek 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(®, ®s->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 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH ethtool] bnxt: Add Broadcom driver support. 2020-09-21 9:18 ` Michal Kubecek @ 2020-09-22 5:54 ` Vasundhara Volam 2020-09-22 6:52 ` Michal Kubecek 0 siblings, 1 reply; 7+ messages in thread From: Vasundhara Volam @ 2020-09-22 5:54 UTC (permalink / raw) To: Michal Kubecek Cc: David Miller, Netdev, Jakub Kicinski, Michael Chan, Edwin Peer, Andrew Gospodarek 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(®, ®s->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 > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH ethtool] bnxt: Add Broadcom driver support. 2020-09-22 5:54 ` Vasundhara Volam @ 2020-09-22 6:52 ` Michal Kubecek 2020-09-22 7:15 ` Vasundhara Volam 0 siblings, 1 reply; 7+ messages in thread From: Michal Kubecek @ 2020-09-22 6:52 UTC (permalink / raw) To: Vasundhara Volam Cc: David Miller, Netdev, Jakub Kicinski, Michael Chan, Edwin Peer, Andrew Gospodarek 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH ethtool] bnxt: Add Broadcom driver support. 2020-09-22 6:52 ` Michal Kubecek @ 2020-09-22 7:15 ` Vasundhara Volam 0 siblings, 0 replies; 7+ messages in thread From: Vasundhara Volam @ 2020-09-22 7:15 UTC (permalink / raw) To: Michal Kubecek Cc: David Miller, Netdev, Jakub Kicinski, Michael Chan, Edwin Peer, Andrew Gospodarek 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH ethtool] bnxt: Add Broadcom driver support. 2020-09-21 6:39 [PATCH ethtool] bnxt: Add Broadcom driver support Vasundhara Volam 2020-09-21 9:18 ` Michal Kubecek @ 2020-09-21 12:41 ` Andrew Lunn 2020-09-22 6:02 ` Vasundhara Volam 1 sibling, 1 reply; 7+ messages in thread From: Andrew Lunn @ 2020-09-21 12:41 UTC (permalink / raw) To: Vasundhara Volam Cc: mkubecek, davem, netdev, kuba, michael.chan, edwin.peer, andrew.gospodarek > +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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH ethtool] bnxt: Add Broadcom driver support. 2020-09-21 12:41 ` Andrew Lunn @ 2020-09-22 6:02 ` Vasundhara Volam 0 siblings, 0 replies; 7+ messages in thread From: Vasundhara Volam @ 2020-09-22 6:02 UTC (permalink / raw) To: Andrew Lunn Cc: Michal Kubecek, David Miller, Netdev, Jakub Kicinski, Michael Chan, Edwin Peer, Andrew Gospodarek 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-09-22 7:15 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-09-21 6:39 [PATCH ethtool] bnxt: Add Broadcom driver support Vasundhara Volam 2020-09-21 9:18 ` Michal Kubecek 2020-09-22 5:54 ` Vasundhara Volam 2020-09-22 6:52 ` Michal Kubecek 2020-09-22 7:15 ` Vasundhara Volam 2020-09-21 12:41 ` Andrew Lunn 2020-09-22 6:02 ` Vasundhara Volam
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.