[PATCH 00/15] pciutils: Add utility for Lane Margining

[PATCH 00/15] pciutils: Add utility for Lane Margining

[Nikita Proshkin]

PCIe Gen 4 spec introduced new extended capability mandatory for all
ports - Lane Margining at the Receiver. This new feature allows to get an
approximation of the Link eye margin diagram by four points. This
information shows how resistant the Link is to external influences and can
be useful for link debugging and presets tuning.
Previously, this information was only available using a hardware debugger.

Patch series consists of two parts:

* Patch for lspci to add Margining registers reading. There is not much
  information available without issuing any margining commands, but this
  info is useful anyway;
* New pcilmr utility.

Margining capability assumes the exchange of commands with the device, so
its support was implemented as a separate utility pcilmr.
The pcilmr allows you to test Links and supports all the features provided
by the Margining capability. The utility is written using a pcilib, it is
divided into a library part and a main function with arguments parsing in
the pciutils root dir.
Man page is provided as well.

Utility compilation and man page preparation are integrated into the
pciutils Makefile, so run make is enough to start testing the utility
(Gen 4/5 device is required though).
Utility was written with Linux in mind and was tested only on this OS.

Example utility results on different systems you can see in gist:
https://gist.github.com/bombanya/f2b15263712757ffba1a11eea011c419

Patch series is also posted as PR on pciutils github:
https://github.com/pciutils/pciutils/pull/162

Nikita Proshkin (15):
  pciutils-lspci: Fix unsynchronized caches in lspci struct device and
    pci struct pci_dev
  pciutils: Add constants for Lane Margining at the Receiver Extended
    Capability
  pciutils-lspci: Add Lane Margining support to the lspci
  pciutils-pcilmr: Add functions for device checking and preparations
    before main margining processes
  pciutils-pcilmr: Add margining process functions
  pciutils-pcilmr: Add logging functions for margining
  pciutils-pcilmr: Add all-in-one device margining parameters reading
    function
  pciutils-pcilmr: Add function for default margining results log
  pciutils-pcilmr: Add utility main function
  pciutils-pcilmr: Add support for unique hardware quirks
  pciutils-pcilmr: Add the ability to pass multiple links to the utility
  pciutils-pcilmr: Add --scan mode to search for all LMR-capable Links
  pciutils-pcilmr: Add option to save margining results in csv form
  pciutils-pcilmr: Add handling of situations when device reports its
    MaxOffset values equal to 0
  pciutils-pcilmr: Add pcilmr man page

 Makefile                 |  11 +-
 lib/header.h             |   7 +
 lmr_lib/Makefile         |  10 +
 lmr_lib/margin.c         | 528 ++++++++++++++++++++++++++++++++++++++
 lmr_lib/margin.h         | 206 +++++++++++++++
 lmr_lib/margin_hw.c      | 163 ++++++++++++
 lmr_lib/margin_hw.h      |  46 ++++
 lmr_lib/margin_log.c     | 141 +++++++++++
 lmr_lib/margin_log.h     |  23 ++
 lmr_lib/margin_results.c | 255 +++++++++++++++++++
 lmr_lib/margin_results.h |  15 ++
 ls-ecaps.c               |  22 +-
 lspci.c                  |   1 +
 pcilmr.c                 | 532 +++++++++++++++++++++++++++++++++++++++
 pcilmr.man               | 179 +++++++++++++
 15 files changed, 2136 insertions(+), 3 deletions(-)
 create mode 100644 lmr_lib/Makefile
 create mode 100644 lmr_lib/margin.c
 create mode 100644 lmr_lib/margin.h
 create mode 100644 lmr_lib/margin_hw.c
 create mode 100644 lmr_lib/margin_hw.h
 create mode 100644 lmr_lib/margin_log.c
 create mode 100644 lmr_lib/margin_log.h
 create mode 100644 lmr_lib/margin_results.c
 create mode 100644 lmr_lib/margin_results.h
 create mode 100644 pcilmr.c
 create mode 100644 pcilmr.man

-- 
2.34.1

[PATCH 01/15] pciutils-lspci: Fix unsynchronized caches in lspci struct device and pci struct pci_dev

[Nikita Proshkin]

lspci initializes both caches for the device to the same memory block in
its scan_device function. Latter calls to config_fetch function will
realloc cache in struct device, but not in struct pci_dev leading to
the invalid pointer in the latter. pci_dev cache is used by pci_read_*
functions, what will lead to a possible use-after-free situations.

Example:

With patch:

diff --git a/ls-caps.c b/ls-caps.c
index a481b16..b454843 100644
--- a/ls-caps.c
+++ b/ls-caps.c
@@ -1802,6 +1802,7 @@ show_caps(struct device *d, int where)
 	      break;
 	    case PCI_CAP_ID_EXP:
 	      type = cap_express(d, where, cap);
+        struct pci_cap* test = pci_find_cap(d->dev, PCI_CAP_ID_EXP, PCI_CAP_NORMAL);
 	      can_have_ext_caps = 1;
 	      break;
 	    case PCI_CAP_ID_MSIX:

valgrind run:
valgrind ./lspci -vvvs 7:0.0

...
==22835== Invalid read of size 2
==22835==    at 0x11A90A: pci_read_word (in /home/merlin/git/pciutils/lspci)
==22835==    by 0x11EBEC: pci_scan_caps (in /home/merlin/git/pciutils/lspci)
==22835==    by 0x11AC00: pci_fill_info_v38 (in /home/merlin/git/pciutils/lspci)
==22835==    by 0x11ED73: pci_find_cap (in /home/merlin/git/pciutils/lspci)
==22835==    by 0x1126FA: show_caps (in /home/merlin/git/pciutils/lspci)
==22835==    by 0x10E860: show_device (in /home/merlin/git/pciutils/lspci)
==22835==    by 0x10BFA3: main (in /home/merlin/git/pciutils/lspci)
==22835==  Address 0x5249276 is 6 bytes inside a block of size 64 free'd
==22835==    at 0x4E0A13B: realloc (vg_replace_malloc.c:1649)
==22835==    by 0x119BCC: xrealloc (in /home/merlin/git/pciutils/lspci)
==22835==    by 0x10CD2C: config_fetch (in /home/merlin/git/pciutils/lspci)
==22835==    by 0x110DAA: show_caps (in /home/merlin/git/pciutils/lspci)
==22835==    by 0x10E860: show_device (in /home/merlin/git/pciutils/lspci)
==22835==    by 0x10BFA3: main (in /home/merlin/git/pciutils/lspci)
==22835==  Block was alloc'd at
==22835==    at 0x4E050B5: malloc (vg_replace_malloc.c:431)
==22835==    by 0x119B9C: xmalloc (in /home/merlin/git/pciutils/lspci)
==22835==    by 0x10CE80: scan_device (in /home/merlin/git/pciutils/lspci)
==22835==    by 0x10BF0F: main (in /home/merlin/git/pciutils/lspci)
...

Reviewed-by: Sergei Miroshnichenko <s.miroshnichenko@yadro.com>
Signed-off-by: Nikita Proshkin <n.proshkin@yadro.com>
---
 lspci.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lspci.c b/lspci.c
index 9452cd3..071cc11 100644
--- a/lspci.c
+++ b/lspci.c
@@ -107,6 +107,7 @@ config_fetch(struct device *d, unsigned int pos, unsigned int len)
       d->config = xrealloc(d->config, d->config_bufsize);
       d->present = xrealloc(d->present, d->config_bufsize);
       memset(d->present + orig_size, 0, d->config_bufsize - orig_size);
+      pci_setup_cache(d->dev, d->config, d->dev->cache_len);
     }
   result = pci_read_block(d->dev, pos, d->config + pos, len);
   if (result)
-- 
2.34.1

[PATCH 02/15] pciutils: Add constants for Lane Margining at the Receiver Extended Capability

[Nikita Proshkin]

Reviewed-by: Sergei Miroshnichenko <s.miroshnichenko@yadro.com>
Signed-off-by: Nikita Proshkin <n.proshkin@yadro.com>
---
 lib/header.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/lib/header.h b/lib/header.h
index 63fbb92..b0fc2be 100644
--- a/lib/header.h
+++ b/lib/header.h
@@ -1400,6 +1400,13 @@
 #define  PCI_DOE_STS_ERROR		0x4	/* DOE Error */
 #define  PCI_DOE_STS_OBJECT_READY	0x80000000 /* Data Object Ready */
 
+/* Lane Margining at the Receiver Extended Capability */
+#define PCI_LMR_CAPS			0x4 /* Margining Port Capabilities Register */
+#define PCI_LMR_CAPS_DRVR		0x1 /* Margining uses Driver Software */
+#define PCI_LMR_PORT_STS		0x6 /* Margining Port Status Register */
+#define PCI_LMR_PORT_STS_READY		0x1 /* Margining Ready */
+#define PCI_LMR_PORT_STS_SOFT_READY	0x2 /* Margining Software Ready */
+
 /*
  * The PCI interface treats multi-function devices as independent
  * devices.  The slot/function address of each device is encoded
-- 
2.34.1

[PATCH 03/15] pciutils-lspci: Add Lane Margining support to the lspci

[Nikita Proshkin]

Gather all the info available without writing to the config space.
Without any commands margining capability exposes only 3 status bits to
read through Margining Port Capabilities and Margining Port Status registers.
It makes sense to show them anyway. For example, Margining Ready bit
indicates whether the device is actually ready for the margining process.

Reviewed-by: Sergei Miroshnichenko <s.miroshnichenko@yadro.com>
Signed-off-by: Nikita Proshkin <n.proshkin@yadro.com>
---
 ls-ecaps.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/ls-ecaps.c b/ls-ecaps.c
index 267d98e..cd6f5d1 100644
--- a/ls-ecaps.c
+++ b/ls-ecaps.c
@@ -691,6 +691,26 @@ cap_rcec(struct device *d, int where)
     printf("\t\tAssociatedBusNumbers: %02x-%02x\n", nextbusn, lastbusn );
 }
 
+static void
+cap_lmr(struct device *d, int where)
+{
+  printf("Lane Margining at the Receiver\n");
+
+  if (verbose < 2)
+    return;
+
+  if (!config_fetch(d, where, 8))
+    return;
+
+  u16 port_caps = get_conf_word(d, where + PCI_LMR_CAPS);
+  u16 port_status = get_conf_word(d, where + PCI_LMR_PORT_STS);
+
+  printf("\t\tPortCap: Uses Driver%c\n", FLAG(port_caps, PCI_LMR_CAPS_DRVR));
+  printf("\t\tPortSta: MargReady%c MargSoftReady%c\n",
+         FLAG(port_status, PCI_LMR_PORT_STS_READY),
+         FLAG(port_status, PCI_LMR_PORT_STS_SOFT_READY));
+}
+
 static void
 cxl_range(u64 base, u64 size, int n)
 {
@@ -1548,7 +1568,7 @@ show_ext_caps(struct device *d, int type)
 	    printf("Physical Layer 16.0 GT/s <?>\n");
 	    break;
 	  case PCI_EXT_CAP_ID_LMR:
-	    printf("Lane Margining at the Receiver <?>\n");
+            cap_lmr(d, where);
 	    break;
 	  case PCI_EXT_CAP_ID_HIER_ID:
 	    printf("Hierarchy ID <?>\n");
-- 
2.34.1

[PATCH 04/15] pciutils-pcilmr: Add functions for device checking and preparations before main margining processes

[Nikita Proshkin]

Follow the checklist from the section 4.2.13.3 "Receiver Margin Testing
Requirements" of the 5.0 spec:
* Verify the Link is at 16 GT/s or higher data rate, in DO PM state;
* Verify that Margining Ready bit of the device is set;
* Disable the ASPM and Autonomous Speed/Width features for the duration
  of the test.

Also verify that Upstream Port of the Link is Function 0 of a Device,
according to spec,only it must implement margining registers.

Reviewed-by: Sergei Miroshnichenko <s.miroshnichenko@yadro.com>
Signed-off-by: Nikita Proshkin <n.proshkin@yadro.com>
---
 Makefile            |  5 ++-
 lmr_lib/Makefile    | 10 ++++++
 lmr_lib/margin_hw.c | 85 +++++++++++++++++++++++++++++++++++++++++++++
 lmr_lib/margin_hw.h | 39 +++++++++++++++++++++
 4 files changed, 138 insertions(+), 1 deletion(-)
 create mode 100644 lmr_lib/Makefile
 create mode 100644 lmr_lib/margin_hw.c
 create mode 100644 lmr_lib/margin_hw.h

diff --git a/Makefile b/Makefile
index 228cb56..bd636bd 100644
--- a/Makefile
+++ b/Makefile
@@ -67,11 +67,14 @@ PCIINC_INS=lib/config.h lib/header.h lib/pci.h lib/types.h
 
 export
 
-all: lib/$(PCIIMPLIB) lspci$(EXEEXT) setpci$(EXEEXT) example$(EXEEXT) lspci.8 setpci.8 pcilib.7 pci.ids.5 update-pciids update-pciids.8 $(PCI_IDS)
+all: lib/$(PCIIMPLIB) lspci$(EXEEXT) setpci$(EXEEXT) example$(EXEEXT) lspci.8 setpci.8 pcilib.7 pci.ids.5 update-pciids update-pciids.8 $(PCI_IDS) lmr_lib/liblmr.a
 
 lib/$(PCIIMPLIB): $(PCIINC) force
 	$(MAKE) -C lib all
 
+lmr_lib/liblmr.a: $(PCIINC) force
+	$(MAKE) -C lmr_lib all
+
 force:
 
 lib/config.h lib/config.mk:
diff --git a/lmr_lib/Makefile b/lmr_lib/Makefile
new file mode 100644
index 0000000..4f85a17
--- /dev/null
+++ b/lmr_lib/Makefile
@@ -0,0 +1,10 @@
+OBJS=margin_hw
+INCL=$(addsuffix .h,$(OBJS)) $(addprefix ../,$(PCIINC))
+
+$(addsuffix .o, $(OBJS)): %.o: %.c $(INCL)
+
+all: liblmr.a
+
+liblmr.a: $(addsuffix .o, $(OBJS))
+	rm -f $@
+	$(AR) rcs $@ $^
diff --git a/lmr_lib/margin_hw.c b/lmr_lib/margin_hw.c
new file mode 100644
index 0000000..dcc6593
--- /dev/null
+++ b/lmr_lib/margin_hw.c
@@ -0,0 +1,85 @@
+#include <stdio.h>
+#include <string.h>
+#include <stdlib.h>
+
+#include "margin_hw.h"
+
+bool margin_verify_link(struct pci_dev *down_port, struct pci_dev *up_port)
+{
+  struct pci_cap *cap = pci_find_cap(down_port, PCI_CAP_ID_EXP, PCI_CAP_NORMAL);
+  if (!cap)
+    return false;
+  if ((pci_read_word(down_port, cap->addr + PCI_EXP_LNKSTA) & PCI_EXP_LNKSTA_SPEED) < 4)
+    return false;
+  if ((pci_read_word(down_port, cap->addr + PCI_EXP_LNKSTA) & PCI_EXP_LNKSTA_SPEED) > 5)
+    return false;
+
+  uint8_t down_type = pci_read_byte(down_port, PCI_HEADER_TYPE) & 0x7F;
+  uint8_t down_sec = pci_read_byte(down_port, PCI_SECONDARY_BUS);
+  uint8_t down_dir = (pci_read_word(down_port, cap->addr + PCI_EXP_FLAGS) & PCI_EXP_FLAGS_TYPE) >> 4;
+
+  // Verify that devices are linked, down_port is Root Port or Downstream Port of Switch,
+  // up_port is Function 0 of a Device
+  if (!(down_sec == up_port->bus && down_type == 1 
+      && (down_dir == 4 || down_dir == 6) && up_port->func == 0))
+    return false;
+
+  struct pci_cap *pm = pci_find_cap(up_port, PCI_CAP_ID_PM, PCI_CAP_NORMAL);
+  return !(pci_read_word(up_port, pm->addr + PCI_PM_CTRL) & PCI_PM_CTRL_STATE_MASK); // D0
+}
+
+bool margin_check_ready_bit(struct pci_dev *dev)
+{
+  struct pci_cap *lmr = pci_find_cap(dev, PCI_EXT_CAP_ID_LMR, PCI_CAP_EXTENDED);
+  return lmr && (pci_read_word(dev, lmr->addr + PCI_LMR_PORT_STS) & PCI_LMR_PORT_STS_READY);
+}
+
+struct margin_dev margin_fill_wrapper(struct pci_dev *dev)
+{
+  struct pci_cap *cap = pci_find_cap(dev, PCI_CAP_ID_EXP, PCI_CAP_NORMAL);
+  struct margin_dev res = {
+      .dev = dev,
+      .lmr_cap_addr = pci_find_cap(dev, PCI_EXT_CAP_ID_LMR, PCI_CAP_EXTENDED)->addr,
+      .width = (pci_read_word(dev, cap->addr + PCI_EXP_LNKSTA) & PCI_EXP_LNKSTA_WIDTH) >> 4,
+      .retimers_n = ((pci_read_word(dev, cap->addr + PCI_EXP_LNKSTA2) & PCI_EXP_LINKSTA2_RETIMER) > 0) +
+                    ((pci_read_word(dev, cap->addr + PCI_EXP_LNKSTA2) & PCI_EXP_LINKSTA2_2RETIMERS) > 0),
+      .link_speed = (pci_read_word(dev, cap->addr + PCI_EXP_LNKSTA) & PCI_EXP_LNKSTA_SPEED)};
+  return res;
+}
+
+bool margin_prep_dev(struct margin_dev *dev)
+{
+  struct pci_cap *pcie = pci_find_cap(dev->dev, PCI_CAP_ID_EXP, PCI_CAP_NORMAL);
+
+  uint16_t lnk_ctl = pci_read_word(dev->dev, pcie->addr + PCI_EXP_LNKCTL);
+  dev->aspm = lnk_ctl & PCI_EXP_LNKCTL_ASPM;
+  dev->hawd = lnk_ctl & PCI_EXP_LNKCTL_HWAUTWD;
+  lnk_ctl &= ~PCI_EXP_LNKCTL_ASPM;
+  pci_write_word(dev->dev, pcie->addr + PCI_EXP_LNKCTL, lnk_ctl);
+  if (pci_read_word(dev->dev, pcie->addr + PCI_EXP_LNKCTL) & PCI_EXP_LNKCTL_ASPM)
+    return false;
+
+  lnk_ctl |= PCI_EXP_LNKCTL_HWAUTWD;
+  pci_write_word(dev->dev, pcie->addr + PCI_EXP_LNKCTL, lnk_ctl);
+
+  uint16_t lnk_ctl2 = pci_read_word(dev->dev, pcie->addr + PCI_EXP_LNKCTL2);
+  dev->hasd = lnk_ctl2 & PCI_EXP_LNKCTL2_SPEED_DIS;
+  lnk_ctl2 |= PCI_EXP_LNKCTL2_SPEED_DIS;
+  pci_write_word(dev->dev, pcie->addr + PCI_EXP_LNKCTL2, lnk_ctl2);
+
+  return true;
+}
+
+void margin_restore_dev(struct margin_dev *dev)
+{
+  struct pci_cap *pcie = pci_find_cap(dev->dev, PCI_CAP_ID_EXP, PCI_CAP_NORMAL);
+
+  uint16_t lnk_ctl = pci_read_word(dev->dev, pcie->addr + PCI_EXP_LNKCTL);
+  lnk_ctl |= dev->aspm;
+  lnk_ctl &= (~PCI_EXP_LNKCTL_HWAUTWD | dev->hawd);
+  pci_write_word(dev->dev, pcie->addr + PCI_EXP_LNKCTL, lnk_ctl);
+
+  uint16_t lnk_ctl2 = pci_read_word(dev->dev, pcie->addr + PCI_EXP_LNKCTL2);
+  lnk_ctl2 &= (~PCI_EXP_LNKCTL2_SPEED_DIS | dev->hasd);
+  pci_write_word(dev->dev, pcie->addr + PCI_EXP_LNKCTL2, lnk_ctl2);
+}
diff --git a/lmr_lib/margin_hw.h b/lmr_lib/margin_hw.h
new file mode 100644
index 0000000..a436d4b
--- /dev/null
+++ b/lmr_lib/margin_hw.h
@@ -0,0 +1,39 @@
+#ifndef _MARGIN_HW_H
+#define _MARGIN_HW_H
+
+#include <stdbool.h>
+#include <stdint.h>
+
+#include "../lib/pci.h"
+
+/*PCI Device wrapper for margining functions*/
+struct margin_dev
+{
+  struct pci_dev *dev;
+  int lmr_cap_addr;
+  uint8_t width;
+  uint8_t retimers_n;
+  uint8_t link_speed;
+
+  /*Saved Device settings to restore after margining*/
+  uint16_t aspm;
+  uint16_t hasd; /*Hardware Autonomous Speed Disable*/
+  uint16_t hawd; /*Hardware Autonomous Width Disable*/
+};
+
+/*Verify that devices form the link at Gen 4 speed or higher*/
+bool margin_verify_link(struct pci_dev *down_port, struct pci_dev *up_port);
+
+/*Check Margining Ready bit from Margining Port Status Register*/
+bool margin_check_ready_bit(struct pci_dev *dev);
+
+/*Awaits device at Gen 4 or higher*/
+struct margin_dev margin_fill_wrapper(struct pci_dev *dev);
+
+/*Disable ASPM, set Hardware Autonomous Speed/Width Disable bits*/
+bool margin_prep_dev(struct margin_dev *dev);
+
+/*Restore Device ASPM, Hardware Autonomous Speed/Width settings*/
+void margin_restore_dev(struct margin_dev *dev);
+
+#endif
-- 
2.34.1

[PATCH 05/15] pciutils-pcilmr: Add margining process functions

[Nikita Proshkin]

* Implement the margining flow as described in the section "Example
  Software Flow for Lane Margining at Receiver" of the 5.0 spec;
* Implement margining commands formation and response parsing according
  to the table 4-26;
* Use Receiver margining parameters as described in the table 8-11;
* Support lane reversal and simultaneous margining of several link lanes.

Reviewed-by: Sergei Miroshnichenko <s.miroshnichenko@yadro.com>
Signed-off-by: Nikita Proshkin <n.proshkin@yadro.com>
---
 lmr_lib/Makefile |   2 +-
 lmr_lib/margin.c | 404 +++++++++++++++++++++++++++++++++++++++++++++++
 lmr_lib/margin.h | 197 +++++++++++++++++++++++
 3 files changed, 602 insertions(+), 1 deletion(-)
 create mode 100644 lmr_lib/margin.c
 create mode 100644 lmr_lib/margin.h

diff --git a/lmr_lib/Makefile b/lmr_lib/Makefile
index 4f85a17..2003a67 100644
--- a/lmr_lib/Makefile
+++ b/lmr_lib/Makefile
@@ -1,4 +1,4 @@
-OBJS=margin_hw
+OBJS=margin_hw margin
 INCL=$(addsuffix .h,$(OBJS)) $(addprefix ../,$(PCIINC))
 
 $(addsuffix .o, $(OBJS)): %.o: %.c $(INCL)
diff --git a/lmr_lib/margin.c b/lmr_lib/margin.c
new file mode 100644
index 0000000..9d25973
--- /dev/null
+++ b/lmr_lib/margin.c
@@ -0,0 +1,404 @@
+#include <errno.h>
+#include <time.h>
+#include <stdio.h>
+#include <malloc.h>
+
+#include "margin.h"
+
+#define MARG_LANE_CTRL(lmr_cap_addr, lane) ((lmr_cap_addr) + 8 + 4 * (lane))
+#define MARG_LANE_STATUS(lmr_cap_addr, lane) ((lmr_cap_addr) + 10 + 4 * (lane))
+
+#define MARG_TIM(go_left, step, recvn) margin_make_cmd(((go_left) << 6) | (step), 3, recvn)
+#define MARG_VOLT(go_down, step, recvn) margin_make_cmd(((go_down) << 7) | (step), 4, recvn)
+
+// report commands
+#define REPORT_CAPS(recvn) margin_make_cmd(0x88, 1, recvn)
+#define REPORT_VOL_STEPS(recvn) margin_make_cmd(0x89, 1, recvn)
+#define REPORT_TIM_STEPS(recvn) margin_make_cmd(0x8A, 1, recvn)
+#define REPORT_TIM_OFFSET(recvn) margin_make_cmd(0x8B, 1, recvn)
+#define REPORT_VOL_OFFSET(recvn) margin_make_cmd(0x8C, 1, recvn)
+#define REPORT_SAMPL_RATE_V(recvn) margin_make_cmd(0x8D, 1, recvn)
+#define REPORT_SAMPL_RATE_T(recvn) margin_make_cmd(0x8E, 1, recvn)
+#define REPORT_SAMPLE_CNT(recvn) margin_make_cmd(0x8F, 1, recvn)
+#define REPORT_MAX_LANES(recvn) margin_make_cmd(0x90, 1, recvn)
+
+// set commands
+#define NO_COMMAND margin_make_cmd(0x9C, 7, 0)
+#define CLEAR_ERROR_LOG(recvn) margin_make_cmd(0x55, 2, recvn)
+#define GO_TO_NORMAL_SETTINGS(recvn) margin_make_cmd(0xF, 2, recvn)
+#define SET_ERROR_LIMIT(error_limit, recvn) margin_make_cmd(0xC0 | (error_limit), 2, recvn)
+
+static int msleep(long msec)
+{
+  struct timespec ts;
+  int res;
+
+  if (msec < 0)
+  {
+    errno = EINVAL;
+    return -1;
+  }
+
+  ts.tv_sec = msec / 1000;
+  ts.tv_nsec = (msec % 1000) * 1000000;
+
+  do
+  {
+    res = nanosleep(&ts, &ts);
+  } while (res && errno == EINTR);
+
+  return res;
+}
+
+union margin_cmd margin_make_cmd(uint8_t payload, uint8_t type, uint8_t recvn)
+{
+  return (union margin_cmd){.fields = {.payload.payload = payload, .recvn = recvn, 
+                                      .type = type, .reserved = 0}};
+}
+
+bool margin_read_params(struct margin_recv *recv)
+{
+  union margin_payload resp;
+  uint8_t lane = recv->lane_reversal ? recv->dev->width - 1 : 0;
+  margin_set_cmd(recv->dev, lane, NO_COMMAND);
+  bool status = margin_report_cmd(recv->dev, lane, REPORT_CAPS(recv->recvn), &resp);
+  if (status)
+  {
+    recv->volt_support = resp.caps.volt_support;
+    recv->ind_up_down_volt = resp.caps.ind_up_down_volt;
+    recv->ind_left_right_tim = resp.caps.ind_left_right_tim;
+    recv->sample_report_method = resp.caps.sample_report_method;
+    recv->ind_error_sampler = resp.caps.ind_error_sampler;
+    status = margin_report_cmd(recv->dev, lane, REPORT_VOL_STEPS(recv->recvn), &resp);
+  }
+  if (status)
+  {
+    recv->volt_steps = resp.voltage_steps;
+    status = margin_report_cmd(recv->dev, lane, REPORT_TIM_STEPS(recv->recvn), &resp);
+  }
+  if (status)
+  {
+    recv->timing_steps = resp.timing_steps;
+    status = margin_report_cmd(recv->dev, lane, REPORT_TIM_OFFSET(recv->recvn), &resp);
+  }
+  if (status)
+  {
+    recv->timing_offset = resp.offset;
+    status = margin_report_cmd(recv->dev, lane, REPORT_VOL_OFFSET(recv->recvn), &resp);
+  }
+  if (status)
+  {
+    recv->volt_offset = resp.offset;
+    status = margin_report_cmd(recv->dev, lane, REPORT_SAMPL_RATE_V(recv->recvn), &resp);
+  }
+  if (status)
+  {
+    recv->sample_rate_v = resp.sample_rate;
+    status = margin_report_cmd(recv->dev, lane, REPORT_SAMPL_RATE_T(recv->recvn), &resp);
+  }
+  if (status)
+  {
+    recv->sample_rate_t = resp.sample_rate;
+    status = margin_report_cmd(recv->dev, lane, REPORT_MAX_LANES(recv->recvn), &resp);
+  }
+  if (status)
+    recv->max_lanes = resp.max_lanes;
+  return status;
+}
+
+bool margin_report_cmd(struct margin_dev *dev, uint8_t lane, 
+                       union margin_cmd cmd, union margin_payload *result)
+{
+  pci_write_word(dev->dev, MARG_LANE_CTRL(dev->lmr_cap_addr, lane), cmd.cmd);
+  msleep(10);
+  union margin_cmd resp;
+  resp.cmd = pci_read_word(dev->dev, MARG_LANE_STATUS(dev->lmr_cap_addr, lane));
+  *result = resp.fields.payload;
+  return resp.fields.type == cmd.fields.type && resp.fields.recvn == cmd.fields.recvn 
+          && margin_set_cmd(dev, lane, NO_COMMAND);
+}
+
+bool margin_set_cmd(struct margin_dev *dev, uint8_t lane, union margin_cmd cmd)
+{
+  pci_write_word(dev->dev, MARG_LANE_CTRL(dev->lmr_cap_addr, lane), cmd.cmd);
+  msleep(10);
+  return pci_read_word(dev->dev, MARG_LANE_STATUS(dev->lmr_cap_addr, lane)) == cmd.cmd;
+}
+
+void margin_lanes(struct margin_lanes_data arg)
+{
+  uint8_t steps_done = 0;
+  union margin_cmd lane_status;
+  uint8_t marg_type;
+  union margin_cmd step_cmd;
+  bool timing = (arg.dir == TIM_LEFT || arg.dir == TIM_RIGHT);
+
+  if (timing)
+  {
+    marg_type = 3;
+    step_cmd = MARG_TIM(arg.dir == TIM_LEFT, steps_done, arg.recv->recvn);
+  }
+  else
+  {
+    marg_type = 4;
+    step_cmd = MARG_VOLT(arg.dir == VOLT_DOWN, steps_done, arg.recv->recvn);
+  }
+
+  bool failed_lanes[32] = {0};
+  uint8_t alive_lanes = arg.lanes_n;
+
+  for (uint8_t i = 0; i < arg.lanes_n; i++)
+  {
+    margin_set_cmd(arg.recv->dev, arg.results[i].lane, NO_COMMAND);
+    margin_set_cmd(arg.recv->dev, arg.results[i].lane, SET_ERROR_LIMIT(arg.recv->error_limit, arg.recv->recvn));
+    margin_set_cmd(arg.recv->dev, arg.results[i].lane, NO_COMMAND);
+    arg.results[i].steps[arg.dir] = arg.steps_lane_total;
+    arg.results[i].statuses[arg.dir] = MARGIN_THR;
+  }
+
+  while (alive_lanes > 0 && steps_done < arg.steps_lane_total)
+  {
+    alive_lanes = 0;
+    steps_done++;
+    if (timing)
+      step_cmd.fields.payload.step_tim.steps = steps_done;
+    else
+      step_cmd.fields.payload.step_volt.steps = steps_done;
+
+    for (uint8_t i = 0; i < arg.lanes_n; i++)
+    {
+      if (!failed_lanes[i])
+      {
+        alive_lanes++;
+        int ctrl_addr = MARG_LANE_CTRL(arg.recv->dev->lmr_cap_addr, arg.results[i].lane);
+        pci_write_word(arg.recv->dev->dev, ctrl_addr, step_cmd.cmd);
+      }
+    }
+    msleep(MARGIN_STEP_MS);
+
+    for (uint8_t i = 0; i < arg.lanes_n; i++)
+    {
+      if (!failed_lanes[i])
+      {
+        int status_addr = MARG_LANE_STATUS(arg.recv->dev->lmr_cap_addr, arg.results[i].lane);
+        lane_status.cmd = pci_read_word(arg.recv->dev->dev, status_addr);
+        uint8_t step_status = lane_status.fields.payload.step_resp.status;
+        if (!(lane_status.fields.type == marg_type && lane_status.fields.recvn == arg.recv->recvn 
+            && step_status == 2 && lane_status.fields.payload.step_resp.err_count <= arg.recv->error_limit
+            && margin_set_cmd(arg.recv->dev, arg.results[i].lane, NO_COMMAND))) 
+        {
+          alive_lanes--;
+          failed_lanes[i] = true;
+          arg.results[i].steps[arg.dir] = steps_done - 1;
+          arg.results[i].statuses[arg.dir] = (step_status == 3 || step_status == 1 ? MARGIN_NAK : MARGIN_LIM);
+        }
+      }
+    }
+
+    arg.steps_lane_done = steps_done;
+  }
+
+  for (uint8_t i = 0; i < arg.lanes_n; i++)
+  {
+    margin_set_cmd(arg.recv->dev, arg.results[i].lane, NO_COMMAND);
+    margin_set_cmd(arg.recv->dev, arg.results[i].lane, CLEAR_ERROR_LOG(arg.recv->recvn));
+    margin_set_cmd(arg.recv->dev, arg.results[i].lane, NO_COMMAND);
+    margin_set_cmd(arg.recv->dev, arg.results[i].lane, GO_TO_NORMAL_SETTINGS(arg.recv->recvn));
+    margin_set_cmd(arg.recv->dev, arg.results[i].lane, NO_COMMAND);
+  }
+}
+
+bool margin_receiver(struct margin_recv *recv, struct margin_args *args, struct margin_results *results)
+{
+  bool status = true;
+
+  uint8_t *lanes_to_margin = args->lanes;
+  uint8_t lanes_n = args->lanes_n;
+
+  results->recvn = recv->recvn;
+  recv->error_limit = args->error_limit;
+  recv->parallel_lanes = (args->parallel_lanes < 0 ? 1 : args->parallel_lanes);
+  results->lanes_n = lanes_n;
+
+  if (!margin_check_ready_bit(recv->dev->dev))
+  {
+    results->test_status = MARGIN_TEST_READY_BIT;
+    status = false;
+  }
+
+  if (status)
+  {
+    recv->lane_reversal = false;
+    if (!margin_read_params(recv))
+    {
+      recv->lane_reversal = true;
+      if (!margin_read_params(recv))
+      {
+        results->test_status = MARGIN_TEST_CAPS;
+        status = false;
+      }
+    }
+  }
+
+  if (status)
+  {
+    if (recv->parallel_lanes > recv->max_lanes + 1)
+      recv->parallel_lanes = recv->max_lanes + 1;
+
+    results->tim_coef = (double)recv->timing_offset / (double)recv->timing_steps;
+    results->volt_coef = (double)recv->volt_offset / (double)recv->volt_steps * 10.0;
+
+    results->ind_left_right_tim = recv->ind_left_right_tim;
+    results->volt_support = recv->volt_support;
+    results->ind_up_down_volt = recv->ind_up_down_volt;
+    results->lane_reversal = recv->lane_reversal;
+    results->link_speed = recv->dev->link_speed;
+    results->test_status = MARGIN_TEST_OK;
+
+    results->lanes = malloc(sizeof(struct margin_res_lane) * lanes_n);
+    for (uint8_t i = 0; i < lanes_n; i++)
+    {
+      results->lanes[i].lane = recv->lane_reversal ? recv->dev->width - lanes_to_margin[i] - 1 : lanes_to_margin[i];
+    }
+
+    if (args->run_margin)
+    {
+      struct margin_lanes_data margin_lanes_data = {.recv = recv,
+                                                    .verbosity = args->verbosity,
+                                                    .steps_margin_remaining = args->steps_margin_remaining};
+      enum margin_dir dir[] = {TIM_LEFT, TIM_RIGHT, VOLT_UP, VOLT_DOWN};
+
+      uint8_t lanes_done = 0;
+      uint8_t use_lanes = 0;
+      uint8_t steps_t = (args->steps_t < 0 ? recv->timing_steps : args->steps_t);
+      uint8_t steps_v = (args->steps_v < 0 ? recv->volt_steps : args->steps_v);
+      while (lanes_done != lanes_n)
+      {
+        use_lanes = (lanes_done + recv->parallel_lanes > lanes_n) ? lanes_n - lanes_done : recv->parallel_lanes;
+        margin_lanes_data.lanes_numbers = lanes_to_margin + lanes_done;
+        margin_lanes_data.lanes_n = use_lanes;
+        margin_lanes_data.results = results->lanes + lanes_done;
+
+        for (uint8_t i = 0; i < 4; i++)
+        {
+          bool timing = dir[i] == TIM_LEFT || dir[i] == TIM_RIGHT;
+          if (!timing && !recv->volt_support)
+            continue;
+          if (dir[i] == TIM_RIGHT && !recv->ind_left_right_tim)
+            continue;
+          if (dir[i] == VOLT_DOWN && !recv->ind_up_down_volt)
+            continue;
+
+          margin_lanes_data.ind = timing ? recv->ind_left_right_tim : recv->ind_up_down_volt;
+          margin_lanes_data.dir = dir[i];
+          margin_lanes_data.steps_lane_total = timing ? steps_t : steps_v;
+          if (*args->steps_margin_remaining >= margin_lanes_data.steps_lane_total)
+          {
+            *args->steps_margin_remaining -= margin_lanes_data.steps_lane_total;
+          }
+          else
+            *args->steps_margin_remaining = 0;
+          margin_lanes(margin_lanes_data);
+        }
+        lanes_done += use_lanes;
+      }
+      if (recv->lane_reversal)
+      {
+        for (uint8_t i = 0; i < lanes_n; i++)
+          results->lanes[i].lane = lanes_to_margin[i];
+      }
+    }
+  }
+
+  return status;
+}
+
+enum margin_test_status margin_process_args(struct margin_dev *dev, struct margin_args *args)
+{
+  uint8_t receivers_n = 2 + 2 * dev->retimers_n;
+
+  if (args->recvs_n <= 0)
+  {
+    for (uint8_t i = 1; i < receivers_n; i++)
+      args->recvs[i - 1] = i;
+    args->recvs[receivers_n - 1] = 6;
+    args->recvs_n = receivers_n;
+  }
+  else
+  {
+    for (uint8_t i = 0; i < args->recvs_n; i++)
+    {
+      uint8_t recvn = args->recvs[i];
+      if (recvn < 1 || recvn > 6 || (recvn != 6 && recvn > receivers_n - 1))
+      {
+        return MARGIN_TEST_ARGS_RECVS;
+      }
+    }
+  }
+
+  if (args->lanes_n <= 0)
+  {
+    args->lanes_n = dev->width;
+    for (uint8_t i = 0; i < args->lanes_n; i++)
+      args->lanes[i] = i;
+  }
+  else
+  {
+    for (uint8_t i = 0; i < args->lanes_n; i++)
+    {
+      if (args->lanes[i] >= dev->width)
+      {
+        return MARGIN_TEST_ARGS_LANES;
+      }
+    }
+  }
+
+  return MARGIN_TEST_OK;
+}
+
+struct margin_results *margin_link(struct margin_dev *down_port, struct margin_dev *up_port, 
+                                   struct margin_args *args, uint8_t *recvs_n)
+{
+  bool status = true;
+
+  status = margin_prep_dev(down_port);
+  if (status)
+    status = margin_prep_dev(up_port);
+
+  uint8_t receivers_n = status ? args->recvs_n : 1;
+  uint8_t *receivers = args->recvs;
+
+  struct margin_results *results = malloc(sizeof(*results) * receivers_n);
+  struct margin_recv receiver;
+
+  if (!status)
+  {
+    results[0].test_status = MARGIN_TEST_ASPM;
+  }
+
+  if (status)
+  {
+    for (uint8_t i = 0; i < receivers_n; i++)
+    {
+      receiver.dev = (receivers[i] == 6 ? up_port : down_port);
+      receiver.recvn = receivers[i];
+      margin_receiver(&receiver, args, &(results[i]));
+    }
+
+    margin_restore_dev(down_port);
+    margin_restore_dev(up_port);
+  }
+
+  *recvs_n = receivers_n;
+  return results;
+}
+
+void margin_free_results(struct margin_results *results, uint8_t results_n)
+{
+  for (uint8_t i = 0; i < results_n; i++)
+  {
+    if (results[i].test_status == MARGIN_TEST_OK)
+      free(results[i].lanes);
+  }
+  free(results);
+}
diff --git a/lmr_lib/margin.h b/lmr_lib/margin.h
new file mode 100644
index 0000000..bb57a76
--- /dev/null
+++ b/lmr_lib/margin.h
@@ -0,0 +1,197 @@
+#ifndef _MARGIN_H
+#define _MARGIN_H
+
+#include "margin_hw.h"
+
+#define MARGIN_STEP_MS 1000
+
+union margin_payload {
+  unsigned int payload : 8;
+
+  struct caps {
+    bool volt_support : 1;
+    bool ind_up_down_volt : 1;
+    bool ind_left_right_tim : 1;
+    bool sample_report_method : 1;
+    bool ind_error_sampler : 1;
+  } caps;
+
+  unsigned int timing_steps : 6;
+  unsigned int voltage_steps : 7;
+  unsigned int offset : 7;
+  unsigned int max_lanes : 5;
+  unsigned int sample_rate : 6;
+
+  struct step_resp {
+    unsigned int err_count : 6;
+    unsigned int status : 2;
+  } step_resp;
+
+  struct step_tim {
+    unsigned int steps : 6;
+    bool go_left : 1;
+  } step_tim;
+
+  struct step_volt {
+    unsigned int steps : 7;
+    bool go_down : 1;
+  } step_volt;
+
+} __attribute__((packed));
+
+union margin_cmd
+{
+  unsigned int cmd : 16;
+
+  struct fields {
+    unsigned int recvn : 3;
+    unsigned int type : 3;
+    unsigned int reserved : 2;
+    union margin_payload payload;
+  } fields;
+
+} __attribute__((packed));
+
+/*Receiver structure with Lane Margining capabilities fields*/
+struct margin_recv {
+  struct margin_dev *dev;
+  uint8_t recvn; /*Receiver Number*/
+  bool lane_reversal;
+
+  bool ind_error_sampler;
+  bool sample_report_method;
+  bool ind_left_right_tim;
+  bool ind_up_down_volt;
+  bool volt_support;
+
+  uint8_t max_lanes;
+  uint8_t parallel_lanes;
+
+  uint8_t timing_steps;
+  uint8_t timing_offset;
+
+  uint8_t volt_steps;
+  uint8_t volt_offset;
+
+  uint8_t error_limit;
+
+  uint8_t sample_rate_v;
+  uint8_t sample_rate_t;
+};
+
+/*Margining Fail reason*/
+enum margin_fail_status {
+  MARGIN_NAK = 0, /*NAK/Set up for margin*/
+  MARGIN_LIM,     /*Too many errors (device limit)*/
+  MARGIN_THR      /*Test threshold has been reached*/
+};
+
+enum margin_dir {
+  VOLT_UP = 0,
+  VOLT_DOWN,
+  TIM_LEFT,
+  TIM_RIGHT
+};
+
+/*Margining results of one lane of the receiver*/
+struct margin_res_lane {
+  uint8_t lane;
+
+  uint8_t steps[4];
+  enum margin_fail_status statuses[4];
+};
+
+enum margin_test_status {
+  MARGIN_TEST_OK = 0,
+  MARGIN_TEST_READY_BIT,
+  MARGIN_TEST_CAPS,
+
+  // Couldn't run test
+  MARGIN_TEST_PREREQS,
+  MARGIN_TEST_ARGS_LANES,
+  MARGIN_TEST_ARGS_RECVS,
+  MARGIN_TEST_ASPM
+};
+
+/*All lanes Receiver results*/
+struct margin_results {
+  uint8_t recvn; /*Receiver Number*/
+  bool ind_left_right_tim;
+  bool volt_support;
+  bool ind_up_down_volt;
+  bool lane_reversal;
+  uint8_t link_speed;
+
+  enum margin_test_status test_status;
+
+  /*Used to convert steps to physical quantity.
+  Calculated from MaxOffset and NumSteps*/
+  double tim_coef;
+  double volt_coef;
+
+  uint8_t lanes_n;
+  struct margin_res_lane *lanes;
+};
+
+/*pcilmr parameters*/
+struct margin_args {
+  int8_t steps_t;        // -1 == use NumTimingSteps
+  int8_t steps_v;        // -1 == use NumVoltageSteps
+  int8_t parallel_lanes; // [1; MaxLanes + 1]
+  uint8_t error_limit;   // [0; 63]
+  uint8_t recvs[6];      // Receivers Numbers
+  int8_t recvs_n;        // -1 == margin all available receivers
+  uint8_t lanes[32];
+  int8_t lanes_n; // -1 == margin all available lanes
+  bool run_margin;
+  uint8_t verbosity; // 0 - basic; 1 - add info about remaining time and lanes in progress during margining
+
+  uint64_t *steps_margin_remaining;
+};
+
+struct margin_lanes_data {
+  struct margin_recv *recv;
+
+  struct margin_res_lane *results;
+  uint8_t *lanes_numbers;
+  uint8_t lanes_n;
+
+  bool ind;
+  enum margin_dir dir;
+
+  uint8_t steps_lane_done;
+  uint8_t steps_lane_total;
+  uint64_t *steps_margin_remaining;
+
+  uint8_t verbosity;
+};
+
+union margin_cmd margin_make_cmd(uint8_t payload, uint8_t type, uint8_t recvn);
+
+/*Read Receiver Lane Margining capabilities.
+dev, recvn and lane_reversal fields must be initialized*/
+bool margin_read_params(struct margin_recv *recv);
+
+bool margin_report_cmd(struct margin_dev *dev, uint8_t lane, 
+                       union margin_cmd cmd, union margin_payload *result);
+
+bool margin_set_cmd(struct margin_dev *dev, uint8_t lane, union margin_cmd cmd);
+
+/*Uses Lane numbers from margin_res_lane structs.
+Margin all lanes_n lanes simultaneously*/
+void margin_lanes(struct margin_lanes_data arg);
+
+/*Awaits that Receiver is prepared through prep_dev function.
+recv fields dev and recvn must be initialized*/
+bool margin_receiver(struct margin_recv *recv, struct margin_args *args, struct margin_results *results);
+
+enum margin_test_status margin_process_args(struct margin_dev *dev, struct margin_args *args);
+
+/*Awaits that args are prepared through process_args.
+Returns number of margined Receivers through recvs_n*/
+struct margin_results *margin_link(struct margin_dev *down_port, struct margin_dev *up_port, 
+                                   struct margin_args *args, uint8_t *recvs_n);
+
+void margin_free_results(struct margin_results *results, uint8_t results_n);
+
+#endif
-- 
2.34.1

[PATCH 06/15] pciutils-pcilmr: Add logging functions for margining

[Nikita Proshkin]

* Implement option to turn on/off logging for margining;
* Support systems with several PCI domains;
* margin_log_margining function prints margining in progress log using
  one line messages for each Receiver in the form:
  "Margining - <direction> - Lanes [<current simultaneous lanes>] - ETA:
  <current direction-lanes margining remaining time> Steps: <current
  margining steps done> Total ETA: <utility run total remaining time>".

Reviewed-by: Sergei Miroshnichenko <s.miroshnichenko@yadro.com>
Signed-off-by: Nikita Proshkin <n.proshkin@yadro.com>
---
 lmr_lib/Makefile     |   2 +-
 lmr_lib/margin.c     |  12 +++++
 lmr_lib/margin_log.c | 115 +++++++++++++++++++++++++++++++++++++++++++
 lmr_lib/margin_log.h |  21 ++++++++
 4 files changed, 149 insertions(+), 1 deletion(-)
 create mode 100644 lmr_lib/margin_log.c
 create mode 100644 lmr_lib/margin_log.h

diff --git a/lmr_lib/Makefile b/lmr_lib/Makefile
index 2003a67..a0eefd6 100644
--- a/lmr_lib/Makefile
+++ b/lmr_lib/Makefile
@@ -1,4 +1,4 @@
-OBJS=margin_hw margin
+OBJS=margin_hw margin margin_log
 INCL=$(addsuffix .h,$(OBJS)) $(addprefix ../,$(PCIINC))
 
 $(addsuffix .o, $(OBJS)): %.o: %.c $(INCL)
diff --git a/lmr_lib/margin.c b/lmr_lib/margin.c
index 9d25973..38a80bf 100644
--- a/lmr_lib/margin.c
+++ b/lmr_lib/margin.c
@@ -4,6 +4,7 @@
 #include <malloc.h>
 
 #include "margin.h"
+#include "margin_log.h"
 
 #define MARG_LANE_CTRL(lmr_cap_addr, lane) ((lmr_cap_addr) + 8 + 4 * (lane))
 #define MARG_LANE_STATUS(lmr_cap_addr, lane) ((lmr_cap_addr) + 10 + 4 * (lane))
@@ -196,6 +197,7 @@ void margin_lanes(struct margin_lanes_data arg)
     }
 
     arg.steps_lane_done = steps_done;
+    margin_log_margining(arg);
   }
 
   for (uint8_t i = 0; i < arg.lanes_n; i++)
@@ -219,9 +221,11 @@ bool margin_receiver(struct margin_recv *recv, struct margin_args *args, struct
   recv->error_limit = args->error_limit;
   recv->parallel_lanes = (args->parallel_lanes < 0 ? 1 : args->parallel_lanes);
   results->lanes_n = lanes_n;
+  margin_log_receiver(recv);
 
   if (!margin_check_ready_bit(recv->dev->dev))
   {
+    margin_log("\nMargining Ready bit is Clear.\n");
     results->test_status = MARGIN_TEST_READY_BIT;
     status = false;
   }
@@ -234,6 +238,7 @@ bool margin_receiver(struct margin_recv *recv, struct margin_args *args, struct
       recv->lane_reversal = true;
       if (!margin_read_params(recv))
       {
+        margin_log("\nError during caps reading.\n");
         results->test_status = MARGIN_TEST_CAPS;
         status = false;
       }
@@ -255,6 +260,8 @@ bool margin_receiver(struct margin_recv *recv, struct margin_args *args, struct
     results->link_speed = recv->dev->link_speed;
     results->test_status = MARGIN_TEST_OK;
 
+    margin_log_print_caps(recv);
+
     results->lanes = malloc(sizeof(struct margin_res_lane) * lanes_n);
     for (uint8_t i = 0; i < lanes_n; i++)
     {
@@ -263,6 +270,7 @@ bool margin_receiver(struct margin_recv *recv, struct margin_args *args, struct
 
     if (args->run_margin)
     {
+      if (args->verbosity > 0) margin_log("\n");
       struct margin_lanes_data margin_lanes_data = {.recv = recv,
                                                     .verbosity = args->verbosity,
                                                     .steps_margin_remaining = args->steps_margin_remaining};
@@ -302,6 +310,7 @@ bool margin_receiver(struct margin_recv *recv, struct margin_args *args, struct
         }
         lanes_done += use_lanes;
       }
+      if (args->verbosity > 0) margin_log("\n");
       if (recv->lane_reversal)
       {
         for (uint8_t i = 0; i < lanes_n; i++)
@@ -368,12 +377,15 @@ struct margin_results *margin_link(struct margin_dev *down_port, struct margin_d
   uint8_t receivers_n = status ? args->recvs_n : 1;
   uint8_t *receivers = args->recvs;
 
+  margin_log_link(down_port, up_port);
+
   struct margin_results *results = malloc(sizeof(*results) * receivers_n);
   struct margin_recv receiver;
 
   if (!status)
   {
     results[0].test_status = MARGIN_TEST_ASPM;
+    margin_log("\nCouldn't disable ASPM on the given Link.\n");
   }
 
   if (status)
diff --git a/lmr_lib/margin_log.c b/lmr_lib/margin_log.c
new file mode 100644
index 0000000..d32136a
--- /dev/null
+++ b/lmr_lib/margin_log.c
@@ -0,0 +1,115 @@
+#include <stdio.h>
+#include <stdarg.h>
+
+#include "margin_log.h"
+
+bool margin_global_logging = false;
+bool margin_print_domain = true;
+
+void margin_log(char *format, ...)
+{
+  va_list arg;
+  va_start(arg, format);
+  if (margin_global_logging)
+    vprintf(format, arg);
+  va_end(arg);
+}
+
+void margin_log_bdfs(struct pci_dev *down, struct pci_dev *up)
+{
+  if (margin_print_domain)
+  {
+    margin_log("%x:%x:%x.%x -> %x:%x:%x.%x", down->domain, down->bus, down->dev,
+               down->func, up->domain, up->bus, up->dev, up->func);
+  }
+  else
+  {
+    margin_log("%x:%x.%x -> %x:%x.%x", down->bus, down->dev,
+               down->func, up->bus, up->dev, up->func);
+  }
+}
+
+void margin_log_print_caps(struct margin_recv *recv)
+{
+  margin_log("\nError Count Limit = %d\n", recv->error_limit);
+  margin_log("Parallel Lanes: %d\n", recv->parallel_lanes);
+  margin_log("\nIndependent Error Sampler: %d\n", recv->ind_error_sampler);
+  margin_log("Sample Reporting Method: %d\n", recv->sample_report_method);
+  margin_log("Independent Left and Right Timing Margining: %d\n", recv->ind_left_right_tim);
+  margin_log("Voltage Margining Supported: %d\n", recv->volt_support);
+  margin_log("Independent Up and Down Voltage Margining: %d\n", recv->ind_up_down_volt);
+  margin_log("Number of Timing Steps: %d\n", recv->timing_steps);
+  margin_log("Number of Voltage Steps: %d\n", recv->volt_steps);
+  margin_log("Max Timing Offset: %d\n", recv->timing_offset);
+  margin_log("Max Voltage Offset: %d\n", recv->volt_offset);
+  margin_log("Max Lanes: %d\n", recv->max_lanes);
+
+  if (recv->lane_reversal)
+  {
+    margin_log("\nWarning: device uses Lane Reversal.\n");
+    margin_log("However, utility uses logical lane numbers in arguments and for logging.\n");
+  }
+}
+
+void margin_log_link(struct margin_dev *down, struct margin_dev *up)
+{
+  margin_log("Link ");
+  margin_log_bdfs(down->dev, up->dev);
+  margin_log("\nNegotiated Link Width: %d\n", down->width);
+  margin_log("Link Speed: %d.0 GT/s = Gen %d\n", (down->link_speed - 3) * 16, down->link_speed);
+  margin_log("Available receivers: ");
+  uint8_t receivers_n = 2 + 2 * down->retimers_n;
+  for (uint8_t i = 1; i < receivers_n; i++)
+    margin_log("Rx(%X) - %d, ", 10 + i - 1, i);
+  margin_log("Rx(F) - 6\n");
+}
+
+void margin_log_receiver(struct margin_recv *recv)
+{
+  margin_log("\nReceiver = Rx(%X)\n", 10 + recv->recvn - 1);
+}
+
+void margin_log_margining(struct margin_lanes_data arg)
+{
+  char *ind_dirs[] = {"Up", "Down", "Left", "Right"};
+  char *non_ind_dirs[] = {"Voltage", "", "Timing"};
+
+  if (arg.verbosity > 0)
+  {
+    margin_log("\033[2K\rMargining - ");
+    if (arg.ind)
+      margin_log("%s", ind_dirs[arg.dir]);
+    else
+      margin_log("%s", non_ind_dirs[arg.dir]);
+
+    uint8_t lanes_counter = 0;
+    margin_log(" - Lanes ");
+    margin_log("[%d", arg.lanes_numbers[0]);
+    for (uint8_t i = 1; i < arg.lanes_n; i++)
+    {
+      if (arg.lanes_numbers[i] - 1 == arg.lanes_numbers[i - 1])
+      {
+        lanes_counter++;
+        if (lanes_counter == 1)
+          margin_log("-");
+        if (i + 1 == arg.lanes_n)
+          margin_log("%d", arg.lanes_numbers[i]);
+      }
+      else
+      {
+        if (lanes_counter > 0)
+          margin_log("%d", arg.lanes_numbers[i - 1]);
+        margin_log(",%d", arg.lanes_numbers[i]);
+        lanes_counter = 0;
+      }
+    }
+    margin_log("]");
+
+    uint64_t lane_eta_s = (arg.steps_lane_total - arg.steps_lane_done) * MARGIN_STEP_MS / 1000;
+    uint64_t total_eta_s = *arg.steps_margin_remaining * MARGIN_STEP_MS / 1000 + lane_eta_s;
+    margin_log(" - ETA: %3ds Steps: %3d Total ETA: %3dm %2ds",
+               lane_eta_s, arg.steps_lane_done, total_eta_s / 60, total_eta_s % 60);
+
+    fflush(stdout);
+  }
+}
diff --git a/lmr_lib/margin_log.h b/lmr_lib/margin_log.h
new file mode 100644
index 0000000..c1f89e1
--- /dev/null
+++ b/lmr_lib/margin_log.h
@@ -0,0 +1,21 @@
+#ifndef _MARGIN_LOG_H
+#define _MARGIN_LOG_H
+
+#include "margin.h"
+
+extern bool margin_global_logging;
+extern bool margin_print_domain;
+
+void margin_log(char *format, ...);
+
+void margin_log_bdfs(struct pci_dev *down, struct pci_dev *up);
+
+void margin_log_print_caps(struct margin_recv *recv);
+
+void margin_log_link(struct margin_dev *down, struct margin_dev *up);
+
+void margin_log_receiver(struct margin_recv *recv);
+
+void margin_log_margining(struct margin_lanes_data arg);
+
+#endif
-- 
2.34.1

[PATCH 07/15] pciutils-pcilmr: Add all-in-one device margining parameters reading function

[Nikita Proshkin]

Implement a function that performs all necessary actions to obtain device
margining parameters for subsequent processing without running the actual
margining test.

Reviewed-by: Sergei Miroshnichenko <s.miroshnichenko@yadro.com>
Signed-off-by: Nikita Proshkin <n.proshkin@yadro.com>
---
 lmr_lib/margin.c | 85 ++++++++++++++++++++++++++++++++++++++++++++++++
 lmr_lib/margin.h |  4 +++
 2 files changed, 89 insertions(+)

diff --git a/lmr_lib/margin.c b/lmr_lib/margin.c
index 38a80bf..41c8fbf 100644
--- a/lmr_lib/margin.c
+++ b/lmr_lib/margin.c
@@ -414,3 +414,88 @@ void margin_free_results(struct margin_results *results, uint8_t results_n)
   }
   free(results);
 }
+
+bool margin_read_params_standalone(struct pci_access *pacc, struct pci_dev *dev, 
+                                    int8_t recvn, struct margin_recv *caps)
+{
+  struct pci_cap *cap = pci_find_cap(dev, PCI_CAP_ID_EXP, PCI_CAP_NORMAL);
+  uint8_t dev_dir = (pci_read_word(dev, cap->addr + PCI_EXP_FLAGS) & PCI_EXP_FLAGS_TYPE) >> 4;
+
+  bool dev_down;
+  if (dev_dir == 4 || dev_dir == 6)
+    dev_down = true;
+  else
+    dev_down = false;
+
+  if (recvn == -1)
+  {
+    if (dev_down)
+      recvn = 1;
+    else
+      recvn = 6;
+  }
+
+  if (recvn < 1 || recvn > 6)
+    return false;
+  if (dev_down && recvn == 6)
+    return false;
+  if (!dev_down && recvn != 6)
+    return false;
+
+  caps->recvn = recvn;
+
+  struct pci_dev *down = NULL;
+  struct pci_dev *up = NULL;
+  struct margin_dev down_w;
+  struct margin_dev up_w;
+
+  for (struct pci_dev *p = pacc->devices; p; p = p->next)
+  {
+    if (dev_down && pci_read_byte(dev, PCI_SECONDARY_BUS) == p->bus && dev->domain == p->domain && p->func == 0)
+    {
+      down = dev;
+      up = p;
+      break;
+    }
+    else if (!dev_down && pci_read_byte(p, PCI_SECONDARY_BUS) == dev->bus && dev->domain == p->domain)
+    {
+      down = p;
+      up = dev;
+      break;
+    }
+  }
+
+  if (!down)
+    return false;
+  if (!margin_verify_link(down, up))
+    return false;
+
+  down_w = margin_fill_wrapper(down);
+  up_w = margin_fill_wrapper(up);
+
+  caps->dev = (dev_down ? &down_w : &up_w);
+  if (!margin_check_ready_bit(caps->dev->dev))
+    return false;
+
+  if (!margin_prep_dev(&down_w))
+    return false;
+  if (!margin_prep_dev(&up_w))
+  {
+    margin_restore_dev(&down_w);
+    return false;
+  }
+
+  bool status;
+  caps->lane_reversal = false;
+  status = margin_read_params(caps);
+  if (!status)
+  {
+    caps->lane_reversal = true;
+    status = margin_read_params(caps);
+  }
+
+  margin_restore_dev(&down_w);
+  margin_restore_dev(&up_w);
+
+  return status;
+}
diff --git a/lmr_lib/margin.h b/lmr_lib/margin.h
index bb57a76..13cfa73 100644
--- a/lmr_lib/margin.h
+++ b/lmr_lib/margin.h
@@ -172,6 +172,10 @@ union margin_cmd margin_make_cmd(uint8_t payload, uint8_t type, uint8_t recvn);
 dev, recvn and lane_reversal fields must be initialized*/
 bool margin_read_params(struct margin_recv *recv);
 
+/*Fill margin_recv without calling other functions*/
+bool margin_read_params_standalone(struct pci_access *pacc, struct pci_dev* dev, 
+                                   int8_t recvn, struct margin_recv* caps);
+
 bool margin_report_cmd(struct margin_dev *dev, uint8_t lane, 
                        union margin_cmd cmd, union margin_payload *result);
 
-- 
2.34.1

[PATCH 08/15] pciutils-pcilmr: Add function for default margining results log

[Nikita Proshkin]

Lanes are rated according to the minimum/recommended values.
The minimum values are taken from the specification (Lane Margining at
the Receiver - Electrical Requirements).
30% UI recommended value for timing is taken from NVIDIA presentation
"PCIe 4.0 Mass Electrical Margins Data Collection".

Receiver lanes are called 'Weird' if all results of all receiver lanes
are equal to the spec minimum value.

Reviewed-by: Sergei Miroshnichenko <s.miroshnichenko@yadro.com>
Signed-off-by: Nikita Proshkin <n.proshkin@yadro.com>
---
 lmr_lib/Makefile         |   2 +-
 lmr_lib/margin_results.c | 135 +++++++++++++++++++++++++++++++++++++++
 lmr_lib/margin_results.h |  13 ++++
 3 files changed, 149 insertions(+), 1 deletion(-)
 create mode 100644 lmr_lib/margin_results.c
 create mode 100644 lmr_lib/margin_results.h

diff --git a/lmr_lib/Makefile b/lmr_lib/Makefile
index a0eefd6..193e164 100644
--- a/lmr_lib/Makefile
+++ b/lmr_lib/Makefile
@@ -1,4 +1,4 @@
-OBJS=margin_hw margin margin_log
+OBJS=margin_hw margin margin_log margin_results
 INCL=$(addsuffix .h,$(OBJS)) $(addprefix ../,$(PCIINC))
 
 $(addsuffix .o, $(OBJS)): %.o: %.c $(INCL)
diff --git a/lmr_lib/margin_results.c b/lmr_lib/margin_results.c
new file mode 100644
index 0000000..61f483a
--- /dev/null
+++ b/lmr_lib/margin_results.c
@@ -0,0 +1,135 @@
+#include <stdlib.h>
+#include <stdio.h>
+
+#include "margin_results.h"
+
+enum lane_rating {
+  BAD = 0,
+  OKAY,
+  PERFECT,
+  WEIRD,
+  INIT,
+};
+
+static char *const grades[] = {"Bad", "Okay", "Perfect", "Weird"};
+static char *const sts_strings[] = {"NAK", "LIM", "THR"};
+static const double ui[] = {62.5 / 100, 31.25 / 100};
+
+static enum lane_rating rate_lane(double value, double min, double recommended, enum lane_rating cur_rate)
+{
+  enum lane_rating res = PERFECT;
+  if (value < recommended)
+    res = OKAY;
+  if (value < min)
+    res = BAD;
+  if (cur_rate == INIT)
+    return res;
+  if (res < cur_rate)
+    return res;
+  else
+    return cur_rate;
+}
+
+static bool check_recv_weird(struct margin_results *results, double tim_min, double volt_min)
+{
+  bool result = true;
+
+  struct margin_res_lane *lane;
+  for (uint8_t i = 0; i < results->lanes_n && result; i++)
+  {
+    lane = &(results->lanes[i]);
+    if (lane->steps[TIM_LEFT] * results->tim_coef != tim_min)
+      result = false;
+    if (results->ind_left_right_tim && lane->steps[TIM_RIGHT] * results->tim_coef != tim_min)
+      result = false;
+    if (results->volt_support)
+    {
+      if (lane->steps[VOLT_UP] * results->volt_coef != volt_min)
+        result = false;
+      if (results->ind_up_down_volt && lane->steps[VOLT_DOWN] * results->volt_coef != volt_min)
+        result = false;
+    }
+  }
+  return result;
+}
+
+void margin_results_print_brief(struct margin_results *results, uint8_t recvs_n)
+{
+  struct margin_res_lane *lane;
+  struct margin_results *recv;
+
+  enum lane_rating lane_rating;
+
+  uint8_t link_speed;
+
+  char *no_test_msgs[] = {
+      "", "Margining Ready bit is Clear",
+      "Error during caps reading",
+      "Margining prerequisites are not satisfied (Gen 4/5, D0)",
+      "Invalid lanes specified with arguments",
+      "Invalid receivers specified with arguments",
+      "Couldn't disable ASPM"};
+
+  for (uint8_t i = 0; i < recvs_n; i++)
+  {
+    recv = &(results[i]);
+    link_speed = recv->link_speed - 4;
+
+    if (recv->test_status != MARGIN_TEST_OK)
+    {
+      if (recv->test_status < MARGIN_TEST_PREREQS)
+        printf("Rx(%X) -", 10 + recv->recvn - 1);
+      printf(" Couldn't run test (%s)\n\n", no_test_msgs[recv->test_status]);
+      continue;
+    }
+
+    if (recv->lane_reversal)
+      printf("Rx(%X) - Lane Reversal\n", 10 + recv->recvn - 1);
+    if (check_recv_weird(recv, MARGIN_TIM_MIN, MARGIN_VOLT_MIN))
+      lane_rating = WEIRD;
+    else
+      lane_rating = INIT;
+
+    for (uint8_t j = 0; j < recv->lanes_n; j++)
+    {
+      lane = &(recv->lanes[j]);
+      double left_ui = lane->steps[TIM_LEFT] * recv->tim_coef;
+      double right_ui = lane->steps[TIM_RIGHT] * recv->tim_coef;
+      double up_volt = lane->steps[VOLT_UP] * recv->volt_coef;
+      double down_volt = lane->steps[VOLT_DOWN] * recv->volt_coef;
+
+      if (lane_rating != WEIRD)
+      {
+        lane_rating = rate_lane(left_ui, MARGIN_TIM_MIN, MARGIN_TIM_RECOMMEND, INIT);
+        if (recv->ind_left_right_tim)
+          lane_rating = rate_lane(right_ui, MARGIN_TIM_MIN, MARGIN_TIM_RECOMMEND, lane_rating);
+        if (recv->volt_support)
+        {
+          lane_rating = rate_lane(up_volt, MARGIN_VOLT_MIN, MARGIN_VOLT_MIN, lane_rating);
+          if (recv->ind_up_down_volt)
+            lane_rating = rate_lane(down_volt, MARGIN_VOLT_MIN, MARGIN_VOLT_MIN, lane_rating);
+        }
+      }
+
+      printf("Rx(%X) Lane %2d - %s\t", 10 + recv->recvn - 1, lane->lane, grades[lane_rating]);
+      if (recv->ind_left_right_tim)
+        printf("L %4.1f%% UI - %5.2fps - %2dst %s, R %4.1f%% UI - %5.2fps - %2dst %s", left_ui,
+               left_ui * ui[link_speed], lane->steps[TIM_LEFT], sts_strings[lane->statuses[TIM_LEFT]], right_ui,
+               right_ui * ui[link_speed], lane->steps[TIM_RIGHT], sts_strings[lane->statuses[TIM_RIGHT]]);
+      else
+        printf("T %4.1f%% UI - %5.2fps - %2dst %s", left_ui, left_ui * ui[link_speed],
+               lane->steps[TIM_LEFT], sts_strings[lane->statuses[TIM_LEFT]]);
+      if (recv->volt_support)
+      {
+        if (recv->ind_up_down_volt)
+          printf(", U %5.1f mV - %3dst %s, D %5.1f mV - %3dst %s", up_volt,
+                 lane->steps[VOLT_UP], sts_strings[lane->statuses[VOLT_UP]],
+                 down_volt, lane->steps[VOLT_DOWN], sts_strings[lane->statuses[VOLT_DOWN]]);
+        else
+          printf(", V %5.1f mV - %3dst %s", up_volt, lane->steps[VOLT_UP], sts_strings[lane->statuses[VOLT_UP]]);
+      }
+      printf("\n");
+    }
+    printf("\n");
+  }
+}
diff --git a/lmr_lib/margin_results.h b/lmr_lib/margin_results.h
new file mode 100644
index 0000000..ab6ddb0
--- /dev/null
+++ b/lmr_lib/margin_results.h
@@ -0,0 +1,13 @@
+#ifndef _MARGIN_RESULTS_H
+#define _MARGIN_RESULTS_H
+
+#include "margin.h"
+
+#define MARGIN_TIM_MIN 20
+#define MARGIN_TIM_RECOMMEND 30
+
+#define MARGIN_VOLT_MIN 50
+
+void margin_results_print_brief(struct margin_results *results, uint8_t recvs_n);
+
+#endif
-- 
2.34.1

[PATCH 09/15] pciutils-pcilmr: Add utility main function

[Nikita Proshkin]

Reviewed-by: Sergei Miroshnichenko <s.miroshnichenko@yadro.com>
Signed-off-by: Nikita Proshkin <n.proshkin@yadro.com>
---
 Makefile |   8 +-
 pcilmr.c | 322 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 328 insertions(+), 2 deletions(-)
 create mode 100644 pcilmr.c

diff --git a/Makefile b/Makefile
index bd636bd..efec2da 100644
--- a/Makefile
+++ b/Makefile
@@ -67,7 +67,7 @@ PCIINC_INS=lib/config.h lib/header.h lib/pci.h lib/types.h
 
 export
 
-all: lib/$(PCIIMPLIB) lspci$(EXEEXT) setpci$(EXEEXT) example$(EXEEXT) lspci.8 setpci.8 pcilib.7 pci.ids.5 update-pciids update-pciids.8 $(PCI_IDS) lmr_lib/liblmr.a
+all: lib/$(PCIIMPLIB) lspci$(EXEEXT) setpci$(EXEEXT) example$(EXEEXT) lspci.8 setpci.8 pcilib.7 pci.ids.5 update-pciids update-pciids.8 $(PCI_IDS) lmr_lib/liblmr.a pcilmr
 
 lib/$(PCIIMPLIB): $(PCIINC) force
 	$(MAKE) -C lib all
@@ -113,6 +113,10 @@ update-pciids: update-pciids.sh
 example$(EXEEXT): example.o lib/$(PCIIMPLIB)
 example.o: example.c $(PCIINC)
 
+PCILMRINC=margin.h margin_results.h margin_log.h
+pcilmr: pcilmr.o lib/$(PCIIMPLIB) lmr_lib/liblmr.a
+pcilmr.o: pcilmr.c $(addprefix lmr_lib/,$(PCILMRINC)) $(PCIINC)
+
 %$(EXEEXT): %.o
 	$(CC) $(LDFLAGS) $(TARGET_ARCH) $^ $(LDLIBS) -o $@
 
@@ -144,7 +148,7 @@ TAGS:
 
 clean:
 	rm -f `find . -name "*~" -o -name "*.[oa]" -o -name "\#*\#" -o -name TAGS -o -name core -o -name "*.orig"`
-	rm -f update-pciids lspci$(EXEEXT) setpci$(EXEEXT) example$(EXEEXT) lib/config.* *.[578] pci.ids.gz lib/*.pc lib/*.so lib/*.so.* lib/*.dll lib/*.def lib/dllrsrc.rc *-rsrc.rc tags
+	rm -f update-pciids lspci$(EXEEXT) setpci$(EXEEXT) example$(EXEEXT) lib/config.* *.[578] pci.ids.gz lib/*.pc lib/*.so lib/*.so.* lib/*.dll lib/*.def lib/dllrsrc.rc *-rsrc.rc tags pcilmr
 	rm -rf maint/dist
 
 distclean: clean
diff --git a/pcilmr.c b/pcilmr.c
new file mode 100644
index 0000000..bbab276
--- /dev/null
+++ b/pcilmr.c
@@ -0,0 +1,322 @@
+#include <memory.h>
+#include <getopt.h>
+#include <stdlib.h>
+#include <stdio.h>
+
+#include "lmr_lib/margin.h"
+#include "lmr_lib/margin_results.h"
+#include "lmr_lib/margin_log.h"
+
+static void usage(void)
+{
+  printf("Usage:\n"
+         "pcilmr [<margining options>] <downstream component>\n\n"
+         "Device Specifier:\n"
+         "<device/component>:\t[<domain>:]<bus>:<dev>.<func>\n\n"
+         "Margining options:\n\n"
+         "Margining Test settings:\n"
+         "-c\t\t\tPrint Device Lane Margining Capabilities only. Do not run margining.\n"
+         "-l <lane>[,<lane>...]\tSpecify lanes for margining. Default: all link lanes.\n"
+         "\t\t\tRemember that Device may use Lane Reversal for Lane numbering.\n"
+         "\t\t\tHowever, utility uses logical lane numbers in arguments and for logging.\n"
+         "\t\t\tUtility will automatically determine Lane Reversal and tune its calls.\n"
+         "-e <errors>\t\tSpecify Error Count Limit for margining. Default: 4.\n"
+         "-r <recvn>[,<recvn>...]\tSpecify Receivers to select margining targets.\n"
+         "\t\t\tDefault: all available Receivers (including Retimers).\n"
+         "-p <parallel_lanes>\tSpecify number of lanes to margin simultaneously.\n"
+         "\t\t\tDefault: 1.\n"
+         "\t\t\tAccording to spec it's possible for Receiver to margin up\n"
+         "\t\t\tto MaxLanes + 1 lanes simultaneously, but usually this works\n"
+         "\t\t\tbad, so this option is for experiments mostly.\n"
+         "-T\t\t\tTime Margining will continue until the Error Count is no more\n"
+         "\t\t\tthan an Error Count Limit. Use this option to find Link limit.\n"
+         "-V\t\t\tSame as -T option, but for Voltage.\n"
+         "-t <steps>\t\tSpecify maximum number of steps for Time Margining.\n"
+         "-v <steps>\t\tSpecify maximum number of steps for Voltage Margining.\n"
+         "Use only one of -T/-t options at the same time (same for -V/-v).\n"
+         "Without these options utility will use MaxSteps from Device\n"
+         "capabilities as test limit.\n\n");
+}
+
+static struct pci_dev *dev_for_filter(struct pci_access *pacc, char *filter)
+{
+  struct pci_filter pci_filter;
+  char dev[17];
+  strncpy(dev, filter, 16);
+  pci_filter_init(pacc, &pci_filter);
+  if (pci_filter_parse_slot(&pci_filter, dev))
+  {
+    printf("Invalid device ID: %s\n", filter);
+    return NULL;
+  }
+
+  if (pci_filter.bus == -1 || pci_filter.slot == -1 || pci_filter.func == -1)
+  {
+    printf("Invalid device ID: %s\n", filter);
+    return NULL;
+  }
+
+  if (pci_filter.domain == -1)
+    pci_filter.domain = 0;
+
+  for (struct pci_dev *p = pacc->devices; p; p = p->next)
+  {
+    if (pci_filter_match(&pci_filter, p))
+    {
+      return p;
+    }
+  }
+  printf("No such PCI device: %s or you don't have enough privileges.\n", filter);
+  return NULL;
+}
+
+static struct pci_dev *find_down_port_for_up(struct pci_access *pacc, struct pci_dev *up)
+{
+  struct pci_dev *down = NULL;
+  for (struct pci_dev *p = pacc->devices; p; p = p->next)
+  {
+    if (pci_read_byte(p, PCI_SECONDARY_BUS) == up->bus && up->domain == p->domain)
+    {
+      down = p;
+      break;
+    }
+  }
+  return down;
+}
+
+static uint8_t parse_csv_arg(char *arg, uint8_t *lanes)
+{
+  uint8_t cnt = 0;
+  char *token = strtok(arg, ",");
+  while (token)
+  {
+    lanes[cnt] = atoi(token);
+    cnt++;
+    token = strtok(NULL, ",");
+  }
+  return cnt;
+}
+
+int main(int argc, char **argv)
+{
+  struct pci_access *pacc;
+
+  struct pci_dev *up_port;
+  struct pci_dev *down_port;
+
+  struct margin_dev wrapper_up;
+  struct margin_dev wrapper_down;
+
+  bool status = true;
+
+  struct margin_results *results;
+  uint8_t results_n;
+
+  struct margin_args args;
+
+  int8_t steps_t_arg = -1;
+  int8_t steps_v_arg = -1;
+  int8_t parallel_lanes_arg = 1;
+  uint8_t error_limit = 4;
+
+  int8_t lanes_n = -1;
+  int8_t recvs_n = -1;
+
+  bool run_margin = true;
+
+  uint64_t total_steps = 0;
+
+  pacc = pci_alloc();
+  pci_init(pacc);
+  pci_scan_bus(pacc);
+
+  margin_print_domain = false;
+  for (struct pci_dev *dev = pacc->devices; dev; dev = dev->next)
+  {
+    if (dev->domain != 0)
+    {
+      margin_print_domain = true;
+      break;
+    }
+  }
+
+  margin_global_logging = true;
+
+  int c;
+
+  while (status && ((c = getopt(argc, argv, ":r:e:l:cp:t:v:VT")) != -1))
+  {
+    switch (c)
+    {
+    case 't':
+      steps_t_arg = atoi(optarg);
+      break;
+    case 'T':
+      steps_t_arg = 63;
+      break;
+    case 'v':
+      steps_v_arg = atoi(optarg);
+      break;
+    case 'V':
+      steps_v_arg = 127;
+      break;
+    case 'p':
+      parallel_lanes_arg = atoi(optarg);
+      break;
+    case 'c':
+      run_margin = false;
+      break;
+    case 'l':
+      lanes_n = parse_csv_arg(optarg, args.lanes);
+      break;
+    case 'e':
+      error_limit = atoi(optarg);
+      break;
+    case 'r':
+      recvs_n = parse_csv_arg(optarg, args.recvs);
+      break;
+    default:
+      printf("Invalid arguments\n");
+      status = false;
+      usage();
+    }
+  }
+
+  if (status)
+  {
+    if (optind != argc - 1)
+      status = false;
+    if (!status && argc > 1)
+      printf("Invalid arguments\n");
+    if (!status)
+      usage();
+  }
+
+  if (status)
+  {
+    if ((up_port = dev_for_filter(pacc, argv[argc - 1])) == NULL)
+    {
+      status = false;
+    }
+  }
+
+  if (status)
+  {
+    down_port = find_down_port_for_up(pacc, up_port);
+    status = down_port != NULL;
+    if (!status)
+      printf("Cannot find Upstream Component for the specified device\n");
+  }
+
+  if (status)
+  {
+    if (!pci_find_cap(up_port, PCI_CAP_ID_EXP, PCI_CAP_NORMAL))
+    {
+      status = false;
+      printf("Looks like you don't have enough privileges to access "
+             "Device Configuration Space.\nTry to run utility as root.\n");
+    }
+  }
+
+  if (status)
+  {
+    if (!margin_verify_link(down_port, up_port))
+    {
+      printf("Link ");
+      margin_log_bdfs(down_port, up_port);
+      printf(" is not ready for margining.\n"
+             "Link must be at Gen 4/5 speed.\n"
+             "Downstream Component must be at D0 PM state.\n");
+      status = false;
+    }
+    else
+    {
+      wrapper_down = margin_fill_wrapper(down_port);
+      wrapper_up = margin_fill_wrapper(up_port);
+    }
+  }
+
+  if (status)
+  {
+    args.error_limit = error_limit;
+    args.lanes_n = lanes_n;
+    args.recvs_n = recvs_n;
+    args.steps_t = steps_t_arg;
+    args.steps_v = steps_v_arg;
+    args.parallel_lanes = parallel_lanes_arg;
+    args.run_margin = run_margin;
+    args.verbosity = 1;
+    args.steps_margin_remaining = &total_steps;
+
+    enum margin_test_status args_status;
+
+    if ((args_status = margin_process_args(&wrapper_down, &args)) != MARGIN_TEST_OK)
+    {
+      status = false;
+      margin_log_link(&wrapper_down, &wrapper_up);
+      if (args_status == MARGIN_TEST_ARGS_RECVS)
+      {
+        margin_log("\nInvalid RecNums specified.\n");
+      }
+      else if (args_status == MARGIN_TEST_ARGS_LANES)
+      {
+        margin_log("\nInvalid lanes specified.\n");
+      }
+    }
+  }
+
+  if (status)
+  {
+    struct margin_recv caps;
+
+    for (uint8_t i = 0; i < args.recvs_n; i++)
+    {
+      if (margin_read_params_standalone(pacc,
+                                        args.recvs[i] == 6 ? up_port : down_port,
+                                        args.recvs[i], &caps))
+      {
+        uint8_t steps_t = steps_t_arg == -1 ? caps.timing_steps : steps_t_arg;
+        uint8_t steps_v = steps_v_arg == -1 ? caps.volt_steps : steps_v_arg;
+        uint8_t parallel_recv = parallel_lanes_arg > caps.max_lanes + 1 ? caps.max_lanes + 1 : parallel_lanes_arg;
+
+        uint8_t step_multiplier = args.lanes_n / parallel_recv + ((args.lanes_n % parallel_recv) > 0);
+
+        total_steps += steps_t * step_multiplier;
+        if (caps.ind_left_right_tim)
+          total_steps += steps_t * step_multiplier;
+        if (caps.volt_support)
+        {
+          total_steps += steps_v * step_multiplier;
+          if (caps.ind_up_down_volt)
+            total_steps += steps_v * step_multiplier;
+        }
+      }
+    }
+  }
+
+  if (status)
+  {
+    results = margin_link(&wrapper_down, &wrapper_up, &args, &results_n);
+    status = (results != NULL);
+  }
+
+  if (status && run_margin)
+  {
+    printf("\nResults:\n");
+    printf("\nPass/fail criteria:\nTiming:\n");
+    printf("Minimum Offset (spec): %d %% UI\nRecommended Offset: %d %% UI\n", MARGIN_TIM_MIN, MARGIN_TIM_RECOMMEND);
+    printf("\nVoltage:\nMinimum Offset (spec): %d mV\n\n", MARGIN_VOLT_MIN);
+    printf("Margining statuses:\nLIM -\tErrorCount exceeded Error Count Limit (found device limit)\n");
+    printf("NAK -\tDevice didn't execute last command, \n\tso result may be less reliable\n");
+    printf("THR -\tThe set (using the utility options) \n\tstep threshold has been reached\n\n");
+    printf("Notations:\nst - steps\n\n");
+
+    margin_results_print_brief(results, results_n);
+  }
+
+  if (status)
+    margin_free_results(results, results_n);
+
+  pci_cleanup(pacc);
+  return 0;
+}
-- 
2.34.1

[PATCH 10/15] pciutils-pcilmr: Add support for unique hardware quirks

[Nikita Proshkin]

Make it possible to change receiver margining parameters depending on
current hardware specificity.

In our tests Intel Ice Lake CPUs receivers reported
MaxVoltageOffset = 50 (RxA), which led to results several times bigger
than the results of the hardware debugger.
Looks like in Intel Sapphire Rapids this was fixed, these CPUs report
MaxVoltageOffset = 12 (RxA). To solve the problem it was decided to hardcode
Volt Offset to 12 (120 mV) if the test is running on the Ice Lake.

Use /proc/cpuinfo as a convenient and generic way for different CPU
architectures on Unix-like systems. For other operating systems, a
different solution will be needed.

Reviewed-by: Sergei Miroshnichenko <s.miroshnichenko@yadro.com>
Signed-off-by: Nikita Proshkin <n.proshkin@yadro.com>
---
 lmr_lib/margin.c     | 22 ++++++++++++
 lmr_lib/margin.h     |  2 ++
 lmr_lib/margin_hw.c  | 80 +++++++++++++++++++++++++++++++++++++++++++-
 lmr_lib/margin_hw.h  |  7 ++++
 lmr_lib/margin_log.c | 15 +++++++++
 lmr_lib/margin_log.h |  2 ++
 6 files changed, 127 insertions(+), 1 deletion(-)

diff --git a/lmr_lib/margin.c b/lmr_lib/margin.c
index 41c8fbf..ac20e82 100644
--- a/lmr_lib/margin.c
+++ b/lmr_lib/margin.c
@@ -57,6 +57,26 @@ union margin_cmd margin_make_cmd(uint8_t payload, uint8_t type, uint8_t recvn)
                                       .type = type, .reserved = 0}};
 }
 
+void margin_apply_hw_quirks(struct margin_recv *recv)
+{
+  switch (recv->dev->hw)
+  {
+  case MARGIN_ICE_LAKE:
+  {
+    struct pci_cap *cap = pci_find_cap(recv->dev->dev, PCI_CAP_ID_EXP, PCI_CAP_NORMAL);
+    uint8_t dev_type = (pci_read_word(recv->dev->dev, cap->addr + PCI_EXP_FLAGS) & PCI_EXP_FLAGS_TYPE) >> 4;
+    /* Downstream Port - Root Port */
+    if (recv->recvn == 1 && dev_type == 4)
+    {
+      recv->volt_offset = 12;
+    }
+    break;
+  }
+  default:
+    break;
+  }
+}
+
 bool margin_read_params(struct margin_recv *recv)
 {
   union margin_payload resp;
@@ -249,6 +269,7 @@ bool margin_receiver(struct margin_recv *recv, struct margin_args *args, struct
   {
     if (recv->parallel_lanes > recv->max_lanes + 1)
       recv->parallel_lanes = recv->max_lanes + 1;
+    margin_apply_hw_quirks(recv);
 
     results->tim_coef = (double)recv->timing_offset / (double)recv->timing_steps;
     results->volt_coef = (double)recv->volt_offset / (double)recv->volt_steps * 10.0;
@@ -378,6 +399,7 @@ struct margin_results *margin_link(struct margin_dev *down_port, struct margin_d
   uint8_t *receivers = args->recvs;
 
   margin_log_link(down_port, up_port);
+  margin_log_hw_quirks(down_port);
 
   struct margin_results *results = malloc(sizeof(*results) * receivers_n);
   struct margin_recv receiver;
diff --git a/lmr_lib/margin.h b/lmr_lib/margin.h
index 13cfa73..8f3506b 100644
--- a/lmr_lib/margin.h
+++ b/lmr_lib/margin.h
@@ -168,6 +168,8 @@ struct margin_lanes_data {
 
 union margin_cmd margin_make_cmd(uint8_t payload, uint8_t type, uint8_t recvn);
 
+void margin_apply_hw_quirks(struct margin_recv *recv);
+
 /*Read Receiver Lane Margining capabilities.
 dev, recvn and lane_reversal fields must be initialized*/
 bool margin_read_params(struct margin_recv *recv);
diff --git a/lmr_lib/margin_hw.c b/lmr_lib/margin_hw.c
index dcc6593..664f9ed 100644
--- a/lmr_lib/margin_hw.c
+++ b/lmr_lib/margin_hw.c
@@ -4,6 +4,83 @@
 
 #include "margin_hw.h"
 
+union cpuinfo {
+  uint8_t buf[2];
+
+  struct cpu_fields {
+    uint8_t x86_family;
+    uint8_t x86_model;
+  } fields;
+};
+
+enum cpuinfo_field {
+  x86_FAMILY = 0,
+  x86_MODEL = 1
+};
+
+static char *const cpu_field_names[2] = {"cpu family", "model"};
+
+static bool read_cpuinfo(union cpuinfo *cpuinfo)
+{
+  FILE *cpu_file = fopen("/proc/cpuinfo", "r");
+  if (!cpu_file)
+    return false;
+
+  size_t n = 0;
+  char *line = NULL;
+  uint32_t read_fields = 0;
+  uint32_t req_fields = 0;
+  char *line_colon;
+
+  while (getline(&line, &n, cpu_file) > 0 && req_fields == 0)
+  {
+    if (strstr(line, "vendor_id") && strstr(line, "GenuineIntel"))
+    {
+      req_fields = 1 << x86_FAMILY | 1 << x86_MODEL;
+    }
+  }
+
+  rewind(cpu_file);
+  while (getline(&line, &n, cpu_file) > 0 && req_fields != read_fields)
+  {
+    for (uint8_t i = 0; i < sizeof(cpu_field_names) / sizeof(cpu_field_names[0]); i++)
+    {
+      if ((req_fields & (1 << i)) && !(read_fields & (1 << i)) && strstr(line, cpu_field_names[i]))
+      {
+        read_fields |= 1 << i;
+        line_colon = strchr(line, ':');
+        cpuinfo->buf[i] = atoi(line_colon + 1);
+      }
+    }
+  }
+
+  free(line);
+  fclose(cpu_file);
+  if (!req_fields)
+    return false;
+  return req_fields == read_fields;
+}
+
+static enum margin_hw detect_unique_hw(void)
+{
+  union cpuinfo cpuinfo;
+  memset(cpuinfo.buf, 0, sizeof(cpuinfo.buf));
+  if (!read_cpuinfo(&cpuinfo))
+    return MARGIN_HW_DEFAULT;
+
+  uint8_t x86_family = cpuinfo.fields.x86_family;
+  uint8_t x86_model = cpuinfo.fields.x86_model;
+
+  /* Intel Ice Lake */
+  if (x86_family == 6 &&
+      (x86_model == 126 || x86_model == 108 || x86_model == 106))
+  {
+    return MARGIN_ICE_LAKE;
+  }
+
+  return MARGIN_HW_DEFAULT;
+}
+
 bool margin_verify_link(struct pci_dev *down_port, struct pci_dev *up_port)
 {
   struct pci_cap *cap = pci_find_cap(down_port, PCI_CAP_ID_EXP, PCI_CAP_NORMAL);
@@ -43,7 +120,8 @@ struct margin_dev margin_fill_wrapper(struct pci_dev *dev)
       .width = (pci_read_word(dev, cap->addr + PCI_EXP_LNKSTA) & PCI_EXP_LNKSTA_WIDTH) >> 4,
       .retimers_n = ((pci_read_word(dev, cap->addr + PCI_EXP_LNKSTA2) & PCI_EXP_LINKSTA2_RETIMER) > 0) +
                     ((pci_read_word(dev, cap->addr + PCI_EXP_LNKSTA2) & PCI_EXP_LINKSTA2_2RETIMERS) > 0),
-      .link_speed = (pci_read_word(dev, cap->addr + PCI_EXP_LNKSTA) & PCI_EXP_LNKSTA_SPEED)};
+      .link_speed = (pci_read_word(dev, cap->addr + PCI_EXP_LNKSTA) & PCI_EXP_LNKSTA_SPEED),
+      .hw = detect_unique_hw()};
   return res;
 }
 
diff --git a/lmr_lib/margin_hw.h b/lmr_lib/margin_hw.h
index a436d4b..273cf8e 100644
--- a/lmr_lib/margin_hw.h
+++ b/lmr_lib/margin_hw.h
@@ -6,6 +6,11 @@
 
 #include "../lib/pci.h"
 
+enum margin_hw {
+  MARGIN_HW_DEFAULT,
+  MARGIN_ICE_LAKE
+};
+
 /*PCI Device wrapper for margining functions*/
 struct margin_dev
 {
@@ -15,6 +20,8 @@ struct margin_dev
   uint8_t retimers_n;
   uint8_t link_speed;
 
+  enum margin_hw hw;
+
   /*Saved Device settings to restore after margining*/
   uint16_t aspm;
   uint16_t hasd; /*Hardware Autonomous Speed Disable*/
diff --git a/lmr_lib/margin_log.c b/lmr_lib/margin_log.c
index d32136a..e57bd79 100644
--- a/lmr_lib/margin_log.c
+++ b/lmr_lib/margin_log.c
@@ -113,3 +113,18 @@ void margin_log_margining(struct margin_lanes_data arg)
     fflush(stdout);
   }
 }
+
+void margin_log_hw_quirks(struct margin_dev *dev)
+{
+  switch (dev->hw)
+  {
+  case MARGIN_ICE_LAKE:
+    margin_log("\nRunning on Intel Ice Lake CPU.\n"
+               "Applying next quirks for margining process:\n"
+               "  - Set MaxVoltageOffset of Rx(A) to 12 (120 mV).\n");
+    break;
+
+  default:
+    break;
+  }
+}
diff --git a/lmr_lib/margin_log.h b/lmr_lib/margin_log.h
index c1f89e1..5ad0ade 100644
--- a/lmr_lib/margin_log.h
+++ b/lmr_lib/margin_log.h
@@ -18,4 +18,6 @@ void margin_log_receiver(struct margin_recv *recv);
 
 void margin_log_margining(struct margin_lanes_data arg);
 
+void margin_log_hw_quirks(struct margin_dev *dev);
+
 #endif
-- 
2.34.1

[PATCH 11/15] pciutils-pcilmr: Add the ability to pass multiple links to the utility

[Nikita Proshkin]

* Add support for different utility modes;
* Make the default (now --margin) mode capable to accept several
  components and run test for all of them;
* Add --full mode for sequential start of the test on all ready links
  in the system;
* The complication of the main function is due to the need to pre-read the
  parameters of the devices before starting the tests in order to calculate
  Total ETA of the utility.

Reviewed-by: Sergei Miroshnichenko <s.miroshnichenko@yadro.com>
Signed-off-by: Nikita Proshkin <n.proshkin@yadro.com>
---
 pcilmr.c | 316 +++++++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 235 insertions(+), 81 deletions(-)

diff --git a/pcilmr.c b/pcilmr.c
index bbab276..ed37a05 100644
--- a/pcilmr.c
+++ b/pcilmr.c
@@ -7,12 +7,21 @@
 #include "lmr_lib/margin_results.h"
 #include "lmr_lib/margin_log.h"
 
+enum mode {
+  MARGIN,
+  FULL
+};
+
 static void usage(void)
 {
   printf("Usage:\n"
-         "pcilmr [<margining options>] <downstream component>\n\n"
+         "pcilmr [--margin] [<margining options>] <downstream component> ...\n"
+         "pcilmr --full [<margining options>]\n\n"
          "Device Specifier:\n"
          "<device/component>:\t[<domain>:]<bus>:<dev>.<func>\n\n"
+         "Modes:\n"
+         "--margin\t\tMargin selected Links\n"
+         "--full\t\t\tMargin all ready for testing Links in the system (one by one)\n"
          "Margining options:\n\n"
          "Margining Test settings:\n"
          "-c\t\t\tPrint Device Lane Margining Capabilities only. Do not run margining.\n"
@@ -97,27 +106,58 @@ static uint8_t parse_csv_arg(char *arg, uint8_t *lanes)
   return cnt;
 }
 
+static uint8_t find_ready_links(struct pci_access *pacc, struct pci_dev **down_ports, 
+                                struct pci_dev **up_ports, bool cnt_only)
+{
+  uint8_t cnt = 0;
+  for (struct pci_dev *up = pacc->devices; up; up = up->next)
+  {
+    if (pci_find_cap(up, PCI_EXT_CAP_ID_LMR, PCI_CAP_EXTENDED))
+    {
+      struct pci_dev *down = find_down_port_for_up(pacc, up);
+
+      if (down && margin_verify_link(down, up) &&
+          (margin_check_ready_bit(down) || margin_check_ready_bit(up)))
+      {
+        if (!cnt_only)
+        {
+          up_ports[cnt] = up;
+          down_ports[cnt] = down;
+        }
+        cnt++;
+      }
+    }
+  }
+  return cnt;
+}
+
 int main(int argc, char **argv)
 {
   struct pci_access *pacc;
 
-  struct pci_dev *up_port;
-  struct pci_dev *down_port;
+  struct pci_dev **up_ports;
+  struct pci_dev **down_ports;
+  uint8_t ports_n = 0;
 
-  struct margin_dev wrapper_up;
-  struct margin_dev wrapper_down;
+  struct margin_dev *wrappers_up;
+  struct margin_dev *wrappers_down;
+  bool *checks_status_ports;
 
   bool status = true;
+  enum mode mode;
 
-  struct margin_results *results;
-  uint8_t results_n;
+  /*each link has several receivers -> several results*/
+  struct margin_results **results;
+  uint8_t *results_n;
 
-  struct margin_args args;
+  struct margin_args *args;
 
   int8_t steps_t_arg = -1;
   int8_t steps_v_arg = -1;
   int8_t parallel_lanes_arg = 1;
   uint8_t error_limit = 4;
+  uint8_t lanes_arg[32];
+  uint8_t recvs_arg[6];
 
   int8_t lanes_n = -1;
   int8_t recvs_n = -1;
@@ -142,7 +182,29 @@ int main(int argc, char **argv)
 
   margin_global_logging = true;
 
+  struct option long_options[] = {
+      {.name = "margin", .has_arg = no_argument, .flag = NULL, .val = 0},
+      {.name = "full", .has_arg = no_argument, .flag = NULL, .val = 1},
+      {0, 0, 0, 0}};
+
   int c;
+  c = getopt_long(argc, argv, ":", long_options, NULL);
+
+  switch (c)
+  {
+  case -1: /*no options (strings like component are possible)*/
+    /* FALLTHROUGH */
+  case 0:
+    mode = MARGIN;
+    break;
+  case 1:
+    mode = FULL;
+    break;
+  default: /*unknown option symbol*/
+    mode = MARGIN;
+    optind--;
+    break;
+  }
 
   while (status && ((c = getopt(argc, argv, ":r:e:l:cp:t:v:VT")) != -1))
   {
@@ -167,13 +229,13 @@ int main(int argc, char **argv)
       run_margin = false;
       break;
     case 'l':
-      lanes_n = parse_csv_arg(optarg, args.lanes);
+      lanes_n = parse_csv_arg(optarg, lanes_arg);
       break;
     case 'e':
       error_limit = atoi(optarg);
       break;
     case 'r':
-      recvs_n = parse_csv_arg(optarg, args.recvs);
+      recvs_n = parse_csv_arg(optarg, recvs_arg);
       break;
     default:
       printf("Invalid arguments\n");
@@ -184,7 +246,9 @@ int main(int argc, char **argv)
 
   if (status)
   {
-    if (optind != argc - 1)
+    if (mode == FULL && optind != argc)
+      status = false;
+    if (mode == MARGIN && optind == argc)
       status = false;
     if (!status && argc > 1)
       printf("Invalid arguments\n");
@@ -194,25 +258,58 @@ int main(int argc, char **argv)
 
   if (status)
   {
-    if ((up_port = dev_for_filter(pacc, argv[argc - 1])) == NULL)
+    if (mode == FULL)
     {
-      status = false;
+      ports_n = find_ready_links(pacc, NULL, NULL, true);
+      if (ports_n == 0)
+      {
+        printf("Links not found or you don't have enough privileges.\n");
+        status = false;
+      }
+      else
+      {
+        up_ports = malloc(ports_n * sizeof(*up_ports));
+        down_ports = malloc(ports_n * sizeof(*down_ports));
+        find_ready_links(pacc, down_ports, up_ports, false);
+      }
     }
-  }
+    else if (mode == MARGIN)
+    {
+      ports_n = argc - optind;
+      up_ports = malloc(ports_n * sizeof(*up_ports));
+      down_ports = malloc(ports_n * sizeof(*down_ports));
 
-  if (status)
-  {
-    down_port = find_down_port_for_up(pacc, up_port);
-    status = down_port != NULL;
-    if (!status)
-      printf("Cannot find Upstream Component for the specified device\n");
+      uint8_t cnt = 0;
+      while (optind != argc && status)
+      {
+        status = false;
+        if ((up_ports[cnt] = dev_for_filter(pacc, argv[optind])) == NULL)
+          break;
+        down_ports[cnt] = find_down_port_for_up(pacc, up_ports[cnt]);
+        status = down_ports[cnt] != NULL;
+        if (!status)
+          printf("Cannot find Upstream Component for the specified device\n");
+        cnt++;
+        optind++;
+      }
+
+      if (!status)
+      {
+        free(up_ports);
+        free(down_ports);
+      }
+    }
+    else
+      status = false;
   }
 
   if (status)
   {
-    if (!pci_find_cap(up_port, PCI_CAP_ID_EXP, PCI_CAP_NORMAL))
+    if (!pci_find_cap(up_ports[0], PCI_CAP_ID_EXP, PCI_CAP_NORMAL))
     {
       status = false;
+      free(up_ports);
+      free(down_ports);
       printf("Looks like you don't have enough privileges to access "
              "Device Configuration Space.\nTry to run utility as root.\n");
     }
@@ -220,75 +317,86 @@ int main(int argc, char **argv)
 
   if (status)
   {
-    if (!margin_verify_link(down_port, up_port))
-    {
-      printf("Link ");
-      margin_log_bdfs(down_port, up_port);
-      printf(" is not ready for margining.\n"
-             "Link must be at Gen 4/5 speed.\n"
-             "Downstream Component must be at D0 PM state.\n");
-      status = false;
-    }
-    else
-    {
-      wrapper_down = margin_fill_wrapper(down_port);
-      wrapper_up = margin_fill_wrapper(up_port);
-    }
+    results = malloc(ports_n * sizeof(*results));
+    results_n = malloc(ports_n * sizeof(*results_n));
+    wrappers_up = malloc(ports_n * sizeof(*wrappers_up));
+    wrappers_down = malloc(ports_n * sizeof(*wrappers_down));
+    checks_status_ports = malloc(ports_n * sizeof(*checks_status_ports));
+    args = malloc(ports_n * sizeof(*args));
   }
 
   if (status)
   {
-    args.error_limit = error_limit;
-    args.lanes_n = lanes_n;
-    args.recvs_n = recvs_n;
-    args.steps_t = steps_t_arg;
-    args.steps_v = steps_v_arg;
-    args.parallel_lanes = parallel_lanes_arg;
-    args.run_margin = run_margin;
-    args.verbosity = 1;
-    args.steps_margin_remaining = &total_steps;
-
-    enum margin_test_status args_status;
-
-    if ((args_status = margin_process_args(&wrapper_down, &args)) != MARGIN_TEST_OK)
+    for (uint8_t i = 0; i < ports_n; i++)
     {
-      status = false;
-      margin_log_link(&wrapper_down, &wrapper_up);
-      if (args_status == MARGIN_TEST_ARGS_RECVS)
+      args[i].error_limit = error_limit;
+      args[i].parallel_lanes = parallel_lanes_arg;
+      args[i].run_margin = run_margin;
+      args[i].verbosity = 1;
+      args[i].steps_t = steps_t_arg;
+      args[i].steps_v = steps_v_arg;
+      for (uint8_t j = 0; j < recvs_n; j++)
+        args[i].recvs[j] = recvs_arg[j];
+      args[i].recvs_n = recvs_n;
+      for (uint8_t j = 0; j < lanes_n; j++)
+        args[i].lanes[j] = lanes_arg[j];
+      args[i].lanes_n = lanes_n;
+      args[i].steps_margin_remaining = &total_steps;
+
+      bool local_status = true;
+      enum margin_test_status args_status;
+
+      if (!margin_verify_link(down_ports[i], up_ports[i]))
       {
-        margin_log("\nInvalid RecNums specified.\n");
+        local_status = false;
+        checks_status_ports[i] = false;
+        results[i] = malloc(sizeof(*results[i]));
+        results[i]->test_status = MARGIN_TEST_PREREQS;
       }
-      else if (args_status == MARGIN_TEST_ARGS_LANES)
+      else
       {
-        margin_log("\nInvalid lanes specified.\n");
+        wrappers_down[i] = margin_fill_wrapper(down_ports[i]);
+        wrappers_up[i] = margin_fill_wrapper(up_ports[i]);
       }
-    }
-  }
-
-  if (status)
-  {
-    struct margin_recv caps;
 
-    for (uint8_t i = 0; i < args.recvs_n; i++)
-    {
-      if (margin_read_params_standalone(pacc,
-                                        args.recvs[i] == 6 ? up_port : down_port,
-                                        args.recvs[i], &caps))
+      if (local_status)
       {
-        uint8_t steps_t = steps_t_arg == -1 ? caps.timing_steps : steps_t_arg;
-        uint8_t steps_v = steps_v_arg == -1 ? caps.volt_steps : steps_v_arg;
-        uint8_t parallel_recv = parallel_lanes_arg > caps.max_lanes + 1 ? caps.max_lanes + 1 : parallel_lanes_arg;
+        if ((args_status = margin_process_args(wrappers_down + i, args + i)) != MARGIN_TEST_OK)
+        {
+          local_status = false;
+          results[i] = malloc(sizeof(*results[i]));
+          results[i]->test_status = args_status;
+          checks_status_ports[i] = false;
+        }
+      }
 
-        uint8_t step_multiplier = args.lanes_n / parallel_recv + ((args.lanes_n % parallel_recv) > 0);
+      if (local_status)
+      {
+        checks_status_ports[i] = true;
+        struct margin_recv caps;
 
-        total_steps += steps_t * step_multiplier;
-        if (caps.ind_left_right_tim)
-          total_steps += steps_t * step_multiplier;
-        if (caps.volt_support)
+        for (uint8_t j = 0; j < args[i].recvs_n; j++)
         {
-          total_steps += steps_v * step_multiplier;
-          if (caps.ind_up_down_volt)
-            total_steps += steps_v * step_multiplier;
+          if (margin_read_params_standalone(pacc,
+                                            args[i].recvs[j] == 6 ? up_ports[i] : down_ports[i], 
+                                            args[i].recvs[j], &caps))
+          {
+            uint8_t steps_t = steps_t_arg == -1 ? caps.timing_steps : steps_t_arg;
+            uint8_t steps_v = steps_v_arg == -1 ? caps.volt_steps : steps_v_arg;
+            uint8_t parallel_recv = parallel_lanes_arg > caps.max_lanes + 1 ? caps.max_lanes + 1 : parallel_lanes_arg;
+
+            uint8_t step_multiplier = args[i].lanes_n / parallel_recv + ((args[i].lanes_n % parallel_recv) > 0);
+
+            total_steps += steps_t * step_multiplier;
+            if (caps.ind_left_right_tim)
+              total_steps += steps_t * step_multiplier;
+            if (caps.volt_support)
+            {
+              total_steps += steps_v * step_multiplier;
+              if (caps.ind_up_down_volt)
+                total_steps += steps_v * step_multiplier;
+            }
+          }
         }
       }
     }
@@ -296,13 +404,41 @@ int main(int argc, char **argv)
 
   if (status)
   {
-    results = margin_link(&wrapper_down, &wrapper_up, &args, &results_n);
-    status = (results != NULL);
+    for (uint8_t i = 0; i < ports_n; i++)
+    {
+      if (checks_status_ports[i])
+      {
+        results[i] = margin_link(wrappers_down + i, wrappers_up + i, args + i, results_n + i);
+      }
+      else
+      {
+        results_n[i] = 1;
+        if (results[i]->test_status == MARGIN_TEST_PREREQS)
+        {
+          printf("Link ");
+          margin_log_bdfs(down_ports[i], up_ports[i]);
+          printf(" is not ready for margining.\n"
+                 "Link must be at Gen 4/5 speed.\n"
+                 "Downstream Component must be at D0 PM state.\n");
+        }
+        else if (results[i]->test_status == MARGIN_TEST_ARGS_RECVS)
+        {
+          margin_log_link(wrappers_down + i, wrappers_up + i);
+          printf("\nInvalid RecNums specified.\n");
+        }
+        else if (results[i]->test_status == MARGIN_TEST_ARGS_LANES)
+        {
+          margin_log_link(wrappers_down + i, wrappers_up + i);
+          printf("\nInvalid lanes specified.\n");
+        }
+      }
+      printf("\n----\n\n");
+    }
   }
 
   if (status && run_margin)
   {
-    printf("\nResults:\n");
+    printf("Results:\n");
     printf("\nPass/fail criteria:\nTiming:\n");
     printf("Minimum Offset (spec): %d %% UI\nRecommended Offset: %d %% UI\n", MARGIN_TIM_MIN, MARGIN_TIM_RECOMMEND);
     printf("\nVoltage:\nMinimum Offset (spec): %d mV\n\n", MARGIN_VOLT_MIN);
@@ -311,11 +447,29 @@ int main(int argc, char **argv)
     printf("THR -\tThe set (using the utility options) \n\tstep threshold has been reached\n\n");
     printf("Notations:\nst - steps\n\n");
 
-    margin_results_print_brief(results, results_n);
+    for (uint8_t i = 0; i < ports_n; i++)
+    {
+      printf("Link ");
+      margin_log_bdfs(down_ports[i], up_ports[i]);
+      printf(":\n\n");
+      margin_results_print_brief(results[i], results_n[i]);
+      printf("\n");
+    }
   }
 
   if (status)
-    margin_free_results(results, results_n);
+  {
+    for (uint8_t i = 0; i < ports_n; i++)
+      margin_free_results(results[i], results_n[i]);
+    free(results_n);
+    free(results);
+    free(up_ports);
+    free(down_ports);
+    free(wrappers_up);
+    free(wrappers_down);
+    free(checks_status_ports);
+    free(args);
+  }
 
   pci_cleanup(pacc);
   return 0;
-- 
2.34.1

[PATCH 12/15] pciutils-pcilmr: Add --scan mode to search for all LMR-capable Links

[Nikita Proshkin]

Reviewed-by: Sergei Miroshnichenko <s.miroshnichenko@yadro.com>
Signed-off-by: Nikita Proshkin <n.proshkin@yadro.com>
---
 pcilmr.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 46 insertions(+), 5 deletions(-)

diff --git a/pcilmr.c b/pcilmr.c
index ed37a05..46b9f24 100644
--- a/pcilmr.c
+++ b/pcilmr.c
@@ -9,19 +9,22 @@
 
 enum mode {
   MARGIN,
-  FULL
+  FULL,
+  SCAN
 };
 
 static void usage(void)
 {
   printf("Usage:\n"
          "pcilmr [--margin] [<margining options>] <downstream component> ...\n"
-         "pcilmr --full [<margining options>]\n\n"
+         "pcilmr --full [<margining options>]\n"
+         "pcilmr --scan\n\n"
          "Device Specifier:\n"
          "<device/component>:\t[<domain>:]<bus>:<dev>.<func>\n\n"
          "Modes:\n"
          "--margin\t\tMargin selected Links\n"
          "--full\t\t\tMargin all ready for testing Links in the system (one by one)\n"
+         "--scan\t\t\tScan for Links available for margining\n\n"
          "Margining options:\n\n"
          "Margining Test settings:\n"
          "-c\t\t\tPrint Device Lane Margining Capabilities only. Do not run margining.\n"
@@ -106,7 +109,38 @@ static uint8_t parse_csv_arg(char *arg, uint8_t *lanes)
   return cnt;
 }
 
-static uint8_t find_ready_links(struct pci_access *pacc, struct pci_dev **down_ports, 
+static void scan_links(struct pci_access *pacc, bool only_ready)
+{
+  if (only_ready)
+    printf("Links ready for margining:\n");
+  else
+    printf("Links with Lane Margining at the Receiver capabilities:\n");
+  bool flag = true;
+  for (struct pci_dev *up = pacc->devices; up; up = up->next)
+  {
+    if (pci_find_cap(up, PCI_EXT_CAP_ID_LMR, PCI_CAP_EXTENDED))
+    {
+      struct pci_dev *down = find_down_port_for_up(pacc, up);
+
+      if (down && margin_verify_link(down, up))
+      {
+        margin_log_bdfs(down, up);
+        if (!only_ready && (margin_check_ready_bit(down) || margin_check_ready_bit(up)))
+        {
+          printf(" - Ready");
+        }
+        printf("\n");
+        flag = false;
+      }
+    }
+  }
+  if (flag)
+    printf("Links not found or you don't have enough privileges.\n");
+  pci_cleanup(pacc);
+  exit(0);
+}
+
+static uint8_t find_ready_links(struct pci_access *pacc, struct pci_dev **down_ports,
                                 struct pci_dev **up_ports, bool cnt_only)
 {
   uint8_t cnt = 0;
@@ -184,7 +218,8 @@ int main(int argc, char **argv)
 
   struct option long_options[] = {
       {.name = "margin", .has_arg = no_argument, .flag = NULL, .val = 0},
-      {.name = "full", .has_arg = no_argument, .flag = NULL, .val = 1},
+      {.name = "scan", .has_arg = no_argument, .flag = NULL, .val = 1},
+      {.name = "full", .has_arg = no_argument, .flag = NULL, .val = 2},
       {0, 0, 0, 0}};
 
   int c;
@@ -198,6 +233,12 @@ int main(int argc, char **argv)
     mode = MARGIN;
     break;
   case 1:
+    mode = SCAN;
+    if (optind == argc)
+      scan_links(pacc, false);
+    optind--;
+    break;
+  case 2:
     mode = FULL;
     break;
   default: /*unknown option symbol*/
@@ -378,7 +419,7 @@ int main(int argc, char **argv)
         for (uint8_t j = 0; j < args[i].recvs_n; j++)
         {
           if (margin_read_params_standalone(pacc,
-                                            args[i].recvs[j] == 6 ? up_ports[i] : down_ports[i], 
+                                            args[i].recvs[j] == 6 ? up_ports[i] : down_ports[i],
                                             args[i].recvs[j], &caps))
           {
             uint8_t steps_t = steps_t_arg == -1 ? caps.timing_steps : steps_t_arg;
-- 
2.34.1

[PATCH 13/15] pciutils-pcilmr: Add option to save margining results in csv form

[Nikita Proshkin]

Reviewed-by: Sergei Miroshnichenko <s.miroshnichenko@yadro.com>
Signed-off-by: Nikita Proshkin <n.proshkin@yadro.com>
---
 lmr_lib/margin_results.c | 108 +++++++++++++++++++++++++++++++++++++++
 lmr_lib/margin_results.h |   2 +
 pcilmr.c                 |  18 ++++++-
 3 files changed, 126 insertions(+), 2 deletions(-)

diff --git a/lmr_lib/margin_results.c b/lmr_lib/margin_results.c
index 61f483a..cc6132f 100644
--- a/lmr_lib/margin_results.c
+++ b/lmr_lib/margin_results.c
@@ -1,5 +1,7 @@
 #include <stdlib.h>
 #include <stdio.h>
+#include <time.h>
+#include <string.h>
 
 #include "margin_results.h"
 
@@ -133,3 +135,109 @@ void margin_results_print_brief(struct margin_results *results, uint8_t recvs_n)
     printf("\n");
   }
 }
+
+void margin_results_save_csv(struct margin_results *results, uint8_t recvs_n, char *dir, struct pci_dev *up_dev)
+{
+  char timestamp[64];
+  time_t tim = time(NULL);
+  strftime(timestamp, 64, "%FT%H.%M.%S", gmtime(&tim));
+
+  char *path = (char *)malloc((strlen(dir) + 128) * sizeof(*path));
+  FILE *csv;
+
+  struct margin_res_lane *lane;
+  struct margin_results *recv;
+  enum lane_rating lane_rating;
+  uint8_t link_speed;
+
+  for (uint8_t i = 0; i < recvs_n; i++)
+  {
+    recv = &(results[i]);
+    link_speed = recv->link_speed - 4;
+
+    if (recv->test_status != MARGIN_TEST_OK)
+      continue;
+    sprintf(path, "%s/lmr_%0*x.%02x.%02x.%x_Rx%X_%s.csv", dir,
+            up_dev->domain_16 == 0xffff ? 8 : 4,
+            up_dev->domain, up_dev->bus, up_dev->dev,
+            up_dev->func, 10 + recv->recvn - 1, timestamp);
+    csv = fopen(path, "w");
+    if (!csv)
+    {
+      printf("Error while saving %s\n", path);
+      free(path);
+      return;
+    }
+
+    fprintf(csv, "Lane,Lane Status,Left %% UI,Left ps,Left Steps,Left Status,"
+                 "Right %% UI,Right ps,Right Steps,Right Status,"
+                 "Time %% UI,Time ps,Time Steps,Time Status,"
+                 "Up mV,Up Steps,Up Status,Down mV,Down Steps,Down Status,"
+                 "Voltage mV,Voltage Steps,Voltage Status\n");
+
+    if (check_recv_weird(recv, MARGIN_TIM_MIN, MARGIN_VOLT_MIN))
+      lane_rating = WEIRD;
+    else
+      lane_rating = INIT;
+
+    for (uint8_t j = 0; j < recv->lanes_n; j++)
+    {
+      lane = &(recv->lanes[j]);
+      double left_ui = lane->steps[TIM_LEFT] * recv->tim_coef;
+      double right_ui = lane->steps[TIM_RIGHT] * recv->tim_coef;
+      double up_volt = lane->steps[VOLT_UP] * recv->volt_coef;
+      double down_volt = lane->steps[VOLT_DOWN] * recv->volt_coef;
+
+      if (lane_rating != WEIRD)
+      {
+        lane_rating = rate_lane(left_ui, MARGIN_TIM_MIN, MARGIN_TIM_RECOMMEND, INIT);
+        if (recv->ind_left_right_tim)
+          lane_rating = rate_lane(right_ui, MARGIN_TIM_MIN, MARGIN_TIM_RECOMMEND, lane_rating);
+        if (recv->volt_support)
+        {
+          lane_rating = rate_lane(up_volt, MARGIN_VOLT_MIN, MARGIN_VOLT_MIN, lane_rating);
+          if (recv->ind_up_down_volt)
+            lane_rating = rate_lane(down_volt, MARGIN_VOLT_MIN, MARGIN_VOLT_MIN, lane_rating);
+        }
+      }
+
+      fprintf(csv, "%d,%s,", lane->lane, grades[lane_rating]);
+      if (recv->ind_left_right_tim)
+      {
+        fprintf(csv, "%f,%f,%d,%s,%f,%f,%d,%s,NA,NA,NA,NA,",
+                left_ui, left_ui * ui[link_speed], lane->steps[TIM_LEFT], sts_strings[lane->statuses[TIM_LEFT]],
+                right_ui, right_ui * ui[link_speed], lane->steps[TIM_RIGHT], sts_strings[lane->statuses[TIM_RIGHT]]);
+      }
+      else
+      {
+        for (uint8_t k = 0; k < 8; k++)
+          fprintf(csv, "NA,");
+        fprintf(csv, "%f,%f,%d,%s,", left_ui, left_ui * ui[link_speed],
+                lane->steps[TIM_LEFT], sts_strings[lane->statuses[TIM_LEFT]]);
+      }
+      if (recv->volt_support)
+      {
+        if (recv->ind_up_down_volt)
+        {
+          fprintf(csv, "%f,%d,%s,%f,%d,%s,NA,NA,NA\n",
+                  up_volt, lane->steps[VOLT_UP], sts_strings[lane->statuses[VOLT_UP]],
+                  down_volt, lane->steps[VOLT_DOWN], sts_strings[lane->statuses[VOLT_DOWN]]);
+        }
+        else
+        {
+          for (uint8_t k = 0; k < 6; k++)
+            fprintf(csv, "NA,");
+          fprintf(csv, "%f,%d,%s\n", up_volt, lane->steps[VOLT_UP], sts_strings[lane->statuses[VOLT_UP]]);
+        }
+      }
+      else
+      {
+        for (uint8_t k = 0; k < 8; k++)
+          fprintf(csv, "NA,");
+        fprintf(csv, "NA\n");
+      }
+    }
+    fclose(csv);
+  }
+  free(path);
+}
diff --git a/lmr_lib/margin_results.h b/lmr_lib/margin_results.h
index ab6ddb0..69847cb 100644
--- a/lmr_lib/margin_results.h
+++ b/lmr_lib/margin_results.h
@@ -10,4 +10,6 @@
 
 void margin_results_print_brief(struct margin_results *results, uint8_t recvs_n);
 
+void margin_results_save_csv(struct margin_results *results, uint8_t recvs_n, char *dir, struct pci_dev *up_dev);
+
 #endif
diff --git a/pcilmr.c b/pcilmr.c
index 46b9f24..3da28e5 100644
--- a/pcilmr.c
+++ b/pcilmr.c
@@ -47,7 +47,12 @@ static void usage(void)
          "-v <steps>\t\tSpecify maximum number of steps for Voltage Margining.\n"
          "Use only one of -T/-t options at the same time (same for -V/-v).\n"
          "Without these options utility will use MaxSteps from Device\n"
-         "capabilities as test limit.\n\n");
+         "capabilities as test limit.\n\n"
+         "Margining Log settings:\n"
+         "-o <directory>\t\tSave margining results in csv form into the\n"
+         "\t\t\tspecified directory. Utility will generate file with the\n"
+         "\t\t\tname in form of 'lmr_<downstream component>_Rx#_<timestamp>.csv'\n"
+         "\t\t\tfor each successfully tested receiver.\n");
 }
 
 static struct pci_dev *dev_for_filter(struct pci_access *pacc, char *filter)
@@ -198,6 +203,9 @@ int main(int argc, char **argv)
 
   bool run_margin = true;
 
+  char *dir_for_csv = NULL;
+  bool save_csv = false;
+
   uint64_t total_steps = 0;
 
   pacc = pci_alloc();
@@ -247,7 +255,7 @@ int main(int argc, char **argv)
     break;
   }
 
-  while (status && ((c = getopt(argc, argv, ":r:e:l:cp:t:v:VT")) != -1))
+  while (status && ((c = getopt(argc, argv, ":r:e:l:cp:t:v:VTo:")) != -1))
   {
     switch (c)
     {
@@ -278,6 +286,10 @@ int main(int argc, char **argv)
     case 'r':
       recvs_n = parse_csv_arg(optarg, recvs_arg);
       break;
+    case 'o':
+      dir_for_csv = optarg;
+      save_csv = true;
+      break;
     default:
       printf("Invalid arguments\n");
       status = false;
@@ -494,6 +506,8 @@ int main(int argc, char **argv)
       margin_log_bdfs(down_ports[i], up_ports[i]);
       printf(":\n\n");
       margin_results_print_brief(results[i], results_n[i]);
+      if (save_csv)
+        margin_results_save_csv(results[i], results_n[i], dir_for_csv, up_ports[i]);
       printf("\n");
     }
   }
-- 
2.34.1

[PATCH 14/15] pciutils-pcilmr: Add handling of situations when device reports its MaxOffset values equal to 0

[Nikita Proshkin]

According to spec, for the MaxTimingOffset and MaxVoltageOffset parameters
'A 0 value may be reported if the vendor chooses not to report the offset'.

Use max possible Offset value in such situations and report to the user.

Reviewed-by: Sergei Miroshnichenko <s.miroshnichenko@yadro.com>
Signed-off-by: Nikita Proshkin <n.proshkin@yadro.com>
---
 lmr_lib/margin.c         |  9 +++++++--
 lmr_lib/margin.h         |  3 +++
 lmr_lib/margin_log.c     | 11 +++++++++++
 lmr_lib/margin_results.c | 12 ++++++++++++
 4 files changed, 33 insertions(+), 2 deletions(-)

diff --git a/lmr_lib/margin.c b/lmr_lib/margin.c
index ac20e82..cb9a9da 100644
--- a/lmr_lib/margin.c
+++ b/lmr_lib/margin.c
@@ -271,8 +271,13 @@ bool margin_receiver(struct margin_recv *recv, struct margin_args *args, struct
       recv->parallel_lanes = recv->max_lanes + 1;
     margin_apply_hw_quirks(recv);
 
-    results->tim_coef = (double)recv->timing_offset / (double)recv->timing_steps;
-    results->volt_coef = (double)recv->volt_offset / (double)recv->volt_steps * 10.0;
+    results->tim_off_reported = recv->timing_offset != 0;
+    results->volt_off_reported = recv->volt_offset != 0;
+    double tim_offset = results->tim_off_reported ? (double)recv->timing_offset : 50.0;
+    double volt_offset = results->volt_off_reported ? (double)recv->volt_offset : 50.0;
+
+    results->tim_coef = tim_offset / (double)recv->timing_steps;
+    results->volt_coef = volt_offset / (double)recv->volt_steps * 10.0;
 
     results->ind_left_right_tim = recv->ind_left_right_tim;
     results->volt_support = recv->volt_support;
diff --git a/lmr_lib/margin.h b/lmr_lib/margin.h
index 8f3506b..8f82f30 100644
--- a/lmr_lib/margin.h
+++ b/lmr_lib/margin.h
@@ -129,6 +129,9 @@ struct margin_results {
   double tim_coef;
   double volt_coef;
 
+  bool tim_off_reported;
+  bool volt_off_reported;
+
   uint8_t lanes_n;
   struct margin_res_lane *lanes;
 };
diff --git a/lmr_lib/margin_log.c b/lmr_lib/margin_log.c
index e57bd79..83652f1 100644
--- a/lmr_lib/margin_log.c
+++ b/lmr_lib/margin_log.c
@@ -49,6 +49,17 @@ void margin_log_print_caps(struct margin_recv *recv)
     margin_log("\nWarning: device uses Lane Reversal.\n");
     margin_log("However, utility uses logical lane numbers in arguments and for logging.\n");
   }
+
+  if (recv->timing_offset == 0)
+  {
+    margin_log("\nWarning: Vendor chose not to report the Max Timing Offset.\n"
+               "Utility will use its max possible value - 50 (50%% UI).\n");
+  }
+  if (recv->volt_support && recv->volt_offset == 0)
+  {
+    margin_log("\nWarning: Vendor chose not to report the Max Voltage Offset.\n"
+               "Utility will use its max possible value - 50 (500 mV).\n");
+  }
 }
 
 void margin_log_link(struct margin_dev *down, struct margin_dev *up)
diff --git a/lmr_lib/margin_results.c b/lmr_lib/margin_results.c
index cc6132f..656f1fe 100644
--- a/lmr_lib/margin_results.c
+++ b/lmr_lib/margin_results.c
@@ -87,6 +87,18 @@ void margin_results_print_brief(struct margin_results *results, uint8_t recvs_n)
 
     if (recv->lane_reversal)
       printf("Rx(%X) - Lane Reversal\n", 10 + recv->recvn - 1);
+
+    if (!recv->tim_off_reported)
+      printf("Rx(%X) - Attention: Vendor chose not to report the Max Timing Offset.\n"
+             "Utility used its max possible value (50%% UI) for calculations of %% UI and ps.\n"
+             "Keep in mind that for timing results of this receiver only steps values are reliable.\n\n",
+             10 + recv->recvn - 1);
+    if (recv->volt_support && !recv->volt_off_reported)
+      printf("Rx(%X) - Attention: Vendor chose not to report the Max Voltage Offset.\n"
+             "Utility used its max possible value (500 mV) for calculations of mV.\n"
+             "Keep in mind that for voltage results of this receiver only steps values are reliable.\n\n",
+             10 + recv->recvn - 1);
+
     if (check_recv_weird(recv, MARGIN_TIM_MIN, MARGIN_VOLT_MIN))
       lane_rating = WEIRD;
     else
-- 
2.34.1

[PATCH 15/15] pciutils-pcilmr: Add pcilmr man page

[Nikita Proshkin]

Reviewed-by: Sergei Miroshnichenko <s.miroshnichenko@yadro.com>
Signed-off-by: Nikita Proshkin <n.proshkin@yadro.com>
---
 Makefile   |   2 +-
 pcilmr.c   |   7 ++-
 pcilmr.man | 179 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 184 insertions(+), 4 deletions(-)
 create mode 100644 pcilmr.man

diff --git a/Makefile b/Makefile
index efec2da..adc2023 100644
--- a/Makefile
+++ b/Makefile
@@ -67,7 +67,7 @@ PCIINC_INS=lib/config.h lib/header.h lib/pci.h lib/types.h
 
 export
 
-all: lib/$(PCIIMPLIB) lspci$(EXEEXT) setpci$(EXEEXT) example$(EXEEXT) lspci.8 setpci.8 pcilib.7 pci.ids.5 update-pciids update-pciids.8 $(PCI_IDS) lmr_lib/liblmr.a pcilmr
+all: lib/$(PCIIMPLIB) lspci$(EXEEXT) setpci$(EXEEXT) example$(EXEEXT) lspci.8 setpci.8 pcilib.7 pci.ids.5 update-pciids update-pciids.8 $(PCI_IDS) lmr_lib/liblmr.a pcilmr pcilmr.8
 
 lib/$(PCIIMPLIB): $(PCIINC) force
 	$(MAKE) -C lib all
diff --git a/pcilmr.c b/pcilmr.c
index 3da28e5..3e330f8 100644
--- a/pcilmr.c
+++ b/pcilmr.c
@@ -15,7 +15,8 @@ enum mode {
 
 static void usage(void)
 {
-  printf("Usage:\n"
+  printf("! Utility requires preliminary preparation of the system. Refer to the pcilmr man page !\n\n"
+         "Usage:\n"
          "pcilmr [--margin] [<margining options>] <downstream component> ...\n"
          "pcilmr --full [<margining options>]\n"
          "pcilmr --scan\n\n"
@@ -291,7 +292,7 @@ int main(int argc, char **argv)
       save_csv = true;
       break;
     default:
-      printf("Invalid arguments\n");
+      printf("Invalid arguments\n\n");
       status = false;
       usage();
     }
@@ -304,7 +305,7 @@ int main(int argc, char **argv)
     if (mode == MARGIN && optind == argc)
       status = false;
     if (!status && argc > 1)
-      printf("Invalid arguments\n");
+      printf("Invalid arguments\n\n");
     if (!status)
       usage();
   }
diff --git a/pcilmr.man b/pcilmr.man
new file mode 100644
index 0000000..f7108a5
--- /dev/null
+++ b/pcilmr.man
@@ -0,0 +1,179 @@
+.TH PCILMR 8 "@TODAY@" "@VERSION@" "The PCI Utilities"
+.SH NAME
+pcilmr \- margin PCIe Links
+.SH SYNOPSIS
+.B pcilmr
+.RB [ "--margin" ]
+.RI [ "<margining options>" ] " <downstream component> ..."
+.br
+.B pcilmr --full
+.RI [ "<margining options>" ]
+.br
+.B pcilmr --scan
+.SH CONFIGURATION
+List of the requirements for links and system settings
+to run the margining test.
+
+.B BIOS settings
+(depends on the system, relevant for server baseboards
+with Xeon CPUs):
+.IP \[bu] 3
+Turn off PCIE Leaky Bucket Feature, Re-Equalization and Link Degradation;
+.IP \[bu]
+Set Error Thresholds to 0;
+.IP \[bu]
+Intel VMD for NVMe SSDs - in case of strange behavior of the
+.BR pcilmr,
+try to run it with the VMD turned off.
+.PP
+.B Device (link) requirements:
+.IP
+.I "Configured by the user before running the utility, the utility does not change them:"
+.RS
+.IP \[bu] 3
+The current Link data rate must be 16.0 GT/s or higher (right now
+utility supports Gen 4 and 5 Links);
+.IP \[bu]
+Link Downstream Component must be at D0 Power Management State.
+.RE
+.IP
+.I "Configured by the utility during operation, utility set them to their original "
+.I "state after receiving the results:"
+.RS
+.IP \[bu] 3
+The ASPM must be disabled in both the Downstream Port and Upstream Port;
+.IP \[bu]
+The Hardware Autonomous Speed Disable bit of the Link Control 2 register must be Set in both the
+Downstream Port and Upstream Port;
+.IP \[bu]
+The Hardware Autonomous Width Disable bit of the Link Control register must be Set in both the
+Downstream Port and Upstream Port.
+.SH DESCRIPTION
+.B pcilmr
+utility allows you to take advantage of the PCIe Lane Margining at the Receiver
+capability which is mandatory for all Ports supporting a data rate of 16.0 GT/s or
+higher, including Pseudo Ports (Retimers). Lane Margining at Receiver enables system
+software to obtain the margin information of a given Receiver while the Link is in the
+L0 state. The margin information includes both voltage and time, in either direction from
+the current Receiver position. Margining support for timing is required, while support
+for voltage is optional at 16.0 GT/s and required at 32.0 GT/s and higher data rates. Also,
+independent time margining and independent voltage margining is optional.
+
+Utility allows to get an approximation of the eye margin diagram in the form of a rhombus
+(by four points). Lane Margining at the Receiver capability enables users to margin PCIe
+links without a hardware debugger and without the need to stop the target system. Utility
+can be useful to debug link issues due to receiver margins.
+
+However, the utility results may be not particularly accurate and, as it was found out during
+testing, specific devices provide rather dubious capability support and the reliability of
+the information they provide is questionable. The PCIe specification provides reference values
+for the eye diagram, which are also used by the
+.B pcilmr
+to evaluate the results, but it seems that it makes sense to contact the
+manufacturer of a particular device for references.
+
+The Gen 5 Specification sets allowed range for Timing Margin from 20%\~UI to 50%\~UI and
+for Voltage Margin from 50\~mV to 500\~mV. Utility uses 30%\~UI as the recommended
+value for Timing - taken from NVIDIA presentation ("PCIe 4.0 Mass Electrical Margins Data
+Collection").
+
+.B pcilmr
+requires root privileges (to access Extended Configuration Space), but during our testing
+there were no problems with the devices and they successfully returned to their normal initial
+state after the end of testing.
+
+.SH OPTIONS
+.SS Device Specifier
+.B "<device/component>" \t
+.RI [ "<domain>" :] <bus> : <dev> . <func>
+(see
+.BR lspci (8))
+.SS Utility Modes
+.TP
+.BI --margin " <downstream component> ..."
+Margin selected Links.
+.TP
+.B --full
+Margin all ready for testing (in a meaning similar to the
+.B --scan
+option) Links in the system (one by one).
+.TP
+.B --scan
+Scan for Links with negotiated speed 16 GT/s or higher. Mark "Ready" those of them
+in which at least one of the Link sides have Margining Ready bit set meaning that
+these Links are ready for testing and you can run utility on them.
+.SS Margining Test options
+.TP
+.B -c
+Print Device Lane Margining Capabilities only. Do not run margining.
+.TP
+\fB\-l\fI <lane>\fP[\fI,<lane>...\fP]
+Specify lanes for margining.
+.br
+Remember that Device may use Lane Reversal for Lane numbering. However, utility
+uses logical lane numbers in arguments and for logging. Utility will automatically
+determine Lane Reversal and tune its calls.
+.br
+Default: all link lanes.
+.TP
+.BI -e " <errors>"
+Specify Error Count Limit for margining.
+.br
+Default: 4.
+.TP
+\fB-r\fI <recvn>\fP[\fI,<recvn>...\fP]
+Specify Receivers to select margining targets.
+.br
+Default: all available Receivers (including Retimers).
+.TP
+.BI -p " <parallel_lanes>"
+Specify number of lanes to margin simultaneously.
+.br
+According to spec it's possible for Receiver to margin up to MaxLanes + 1
+lanes simultaneously, but usually this works bad, so this option is for
+experiments mostly.
+.br
+Default: 1.
+.PP
+.B "Use only one of -T/-t options at the same time (same for -V/-v)."
+.br
+.B "Without these options utility will use MaxSteps from Device"
+.B "capabilities as test limit."
+.TP
+.B -T
+Time Margining will continue until the Error Count is no more
+than an Error Count Limit. Use this option to find Link limit.
+.TP
+.BI -t " <steps>"
+Specify maximum number of steps for Time Margining.
+.TP
+.B -V
+Same as
+.I -T
+option, but for Voltage.
+.TP
+.BI -v " <steps>"
+Specify maximum number of steps for Voltage Margining.
+.SS Margining Log options
+.TP
+.BI -o " <directory>"
+Save margining results in csv form into the specified directory. Utility
+will generate file with the name in form of
+.RI "\[dq]lmr_" "<downstream component>" "_Rx" # _ <timestamp> ".csv\[dq]"
+for each successfully tested receiver.
+
+.SH EXAMPLES
+Utility syntax example:
+.RS
+.BI "pcilmr -l" " 0,1 " "-r" " 1,6 " "-TV" " ab:0.0 52:0.0"
+.RE
+
+.UR https://gist.github.com/bombanya/f2b15263712757ffba1a11eea011c419
+Examples of collected results on different systems.
+.UE
+
+.SH SEE ALSO
+.nh
+.BR lspci (8),
+.B PCI Express Base Specification (Lane Margining at Receiver)
+.hy
-- 
2.34.1

Re: [PATCH 15/15] pciutils-pcilmr: Add pcilmr man page

[Bjorn Helgaas]

I'm not a hardware person, but this looks like interesting work!

On Fri, Dec 08, 2023 at 12:17:34PM +0300, Nikita Proshkin wrote:
> +Turn off PCIE Leaky Bucket Feature, Re-Equalization and Link Degradation;

s/PCIE/PCIe/ to match other uses here.

> +The current Link data rate must be 16.0 GT/s or higher (right now
> +utility supports Gen 4 and 5 Links);

So far, each major PCIe spec revision has added a single new data
rate, but that may not always be true, and the spec always uses
terminology like "16.0 GT/s or higher" instead of terms like "Gen 4".

So "supports 16.0 GT/s and 32.0 GT/s Links" might be clearer.

> +The Gen 5 Specification sets allowed range for Timing Margin from 20%\~UI to 50%\~UI and

Usage in the spec itself would be more like "PCIe Base Spec Revision 5.0"
since it doesn't use "Gen 5".

> +According to spec it's possible for Receiver to margin up to MaxLanes + 1
> +lanes simultaneously, but usually this works bad, so this option is for

s/works bad/works poorly/

> +experiments mostly.

Re: [PATCH 04/15] pciutils-pcilmr: Add functions for device checking and preparations before main margining processes

[Martin Mareš]

Hello!

> -all: lib/$(PCIIMPLIB) lspci$(EXEEXT) setpci$(EXEEXT) example$(EXEEXT) lspci.8 setpci.8 pcilib.7 pci.ids.5 update-pciids update-pciids.8 $(PCI_IDS)
> +all: lib/$(PCIIMPLIB) lspci$(EXEEXT) setpci$(EXEEXT) example$(EXEEXT) lspci.8 setpci.8 pcilib.7 pci.ids.5 update-pciids update-pciids.8 $(PCI_IDS) lmr_lib/liblmr.a

Is there any advantage with building LMR as a library instead of linking all
the object files with the margining utility?

> --- /dev/null
> +++ b/lmr_lib/margin_hw.c
> @@ -0,0 +1,85 @@
> +#include <stdio.h>
> +#include <string.h>
> +#include <stdlib.h>

Generally: Please add a comment to every source file, which explains the
purpose of the file and contains a copyright notice. See existing files
for the recommeneded format.

> +  uint8_t down_type = pci_read_byte(down_port, PCI_HEADER_TYPE) & 0x7F;
> +  uint8_t down_sec = pci_read_byte(down_port, PCI_SECONDARY_BUS);
> +  uint8_t down_dir = (pci_read_word(down_port, cap->addr + PCI_EXP_FLAGS) & PCI_EXP_FLAGS_TYPE) >> 4;

I would prefer using libpci types (u8, u32 etc.).

> +  if (!(down_sec == up_port->bus && down_type == 1 

Please avoid whitespace at the end of line.

> +bool margin_prep_dev(struct margin_dev *dev)
> +{
> +  struct pci_cap *pcie = pci_find_cap(dev->dev, PCI_CAP_ID_EXP, PCI_CAP_NORMAL);

What if it doesn't exist?

> diff --git a/lmr_lib/margin_hw.h b/lmr_lib/margin_hw.h
> new file mode 100644
> index 0000000..a436d4b
> --- /dev/null
> +++ b/lmr_lib/margin_hw.h
> @@ -0,0 +1,39 @@
> +#ifndef _MARGIN_HW_H
> +#define _MARGIN_HW_H
> +
> +#include <stdbool.h>
> +#include <stdint.h>
> +
> +#include "../lib/pci.h"

Please do not use relative paths to libpci header files.
Instead, supply proper include path to CC in the Makefile.

> +/*PCI Device wrapper for margining functions*/

Please surround "/*" and "*/" by spaces as in existing source files.

				Have a nice fortnight
-- 
Martin `MJ' Mareš                        <mj@ucw.cz>   http://mj.ucw.cz/
United Computer Wizards, Prague, Czech Republic, Europe, Earth, Universe

Re: [PATCH 00/15] pciutils: Add utility for Lane Margining

[Martin Mareš]

Hello!

> PCIe Gen 4 spec introduced new extended capability mandatory for all
> ports - Lane Margining at the Receiver. This new feature allows to get an
> approximation of the Link eye margin diagram by four points. This
> information shows how resistant the Link is to external influences and can
> be useful for link debugging and presets tuning.
> Previously, this information was only available using a hardware debugger.
> 
> Patch series consists of two parts:
> 
> * Patch for lspci to add Margining registers reading. There is not much
>   information available without issuing any margining commands, but this
>   info is useful anyway;
> * New pcilmr utility.
> 
> Margining capability assumes the exchange of commands with the device, so
> its support was implemented as a separate utility pcilmr.
> The pcilmr allows you to test Links and supports all the features provided
> by the Margining capability. The utility is written using a pcilib, it is
> divided into a library part and a main function with arguments parsing in
> the pciutils root dir.
> Man page is provided as well.
> 
> Utility compilation and man page preparation are integrated into the
> pciutils Makefile, so run make is enough to start testing the utility
> (Gen 4/5 device is required though).
> Utility was written with Linux in mind and was tested only on this OS.

Thanks for your contribution. The utility will need some cleanups,
but overall I will be glad to accept it.

				Have a nice fortnight
-- 
Martin `MJ' Mareš                        <mj@ucw.cz>   http://mj.ucw.cz/
United Computer Wizards, Prague, Czech Republic, Europe, Earth, Universe

Re: [PATCH 05/15] pciutils-pcilmr: Add margining process functions

[Martin Mareš]

> +  if (msec < 0)
> +  {
> +    errno = EINVAL;
> +    return -1;
> +  }
[...]

Please follow the indentation style of the rest of pciutils.

> +union margin_payload {
> +  unsigned int payload : 8;
> +
> +  struct caps {
> +    bool volt_support : 1;
> +    bool ind_up_down_volt : 1;
> +    bool ind_left_right_tim : 1;
> +    bool sample_report_method : 1;
> +    bool ind_error_sampler : 1;
> +  } caps;
> +
> +  unsigned int timing_steps : 6;
> +  unsigned int voltage_steps : 7;
> +  unsigned int offset : 7;
> +  unsigned int max_lanes : 5;
> +  unsigned int sample_rate : 6;
> +
> +  struct step_resp {
> +    unsigned int err_count : 6;
> +    unsigned int status : 2;
> +  } step_resp;
> +
> +  struct step_tim {
> +    unsigned int steps : 6;
> +    bool go_left : 1;
> +  } step_tim;
> +
> +  struct step_volt {
> +    unsigned int steps : 7;
> +    bool go_down : 1;
> +  } step_volt;
> +
> +} __attribute__((packed));

Please do not assume that every compiler used to compile the pciutils supports
GCC extensions. See lib/sysdep.h for how we handle such things.

Also, I am not sure that bit fields are good idea: they save a little data
space, but they expand code.

				Have a nice fortnight
-- 
Martin `MJ' Mareš                        <mj@ucw.cz>   http://mj.ucw.cz/
United Computer Wizards, Prague, Czech Republic, Europe, Earth, Universe

Re: [PATCH 06/15] pciutils-pcilmr: Add logging functions for margining

[Martin Mareš]

>  #include "margin.h"
> +#include "margin_log.h"

Is it useful to have multiple include files? It could be more readable to have
a single include file declaring functions from all modules (similar to lib/internal.h
for libpci).

				Have a nice fortnight
-- 
Martin `MJ' Mareš                        <mj@ucw.cz>   http://mj.ucw.cz/
United Computer Wizards, Prague, Czech Republic, Europe, Earth, Universe

Re: [PATCH 10/15] pciutils-pcilmr: Add support for unique hardware quirks

[Martin Mareš]

> +static bool read_cpuinfo(union cpuinfo *cpuinfo)
> +{
> +  FILE *cpu_file = fopen("/proc/cpuinfo", "r");
> +  if (!cpu_file)
> +    return false;

This works only on Linux.

Wouldn't it be possible to identify Ice Lake by PCI ID of the root bridge
instead?

				Have a nice fortnight
-- 
Martin `MJ' Mareš                        <mj@ucw.cz>   http://mj.ucw.cz/
United Computer Wizards, Prague, Czech Republic, Europe, Earth, Universe

Re: [PATCH 13/15] pciutils-pcilmr: Add option to save margining results in csv form

[Martin Mareš]

> +  char timestamp[64];
> +  time_t tim = time(NULL);
> +  strftime(timestamp, 64, "%FT%H.%M.%S", gmtime(&tim));

Please use sizeof(timestamp) to make the code more robust.

> +  char *path = (char *)malloc((strlen(dir) + 128) * sizeof(*path));

Please use xmalloc().

> +    sprintf(path, "%s/lmr_%0*x.%02x.%02x.%x_Rx%X_%s.csv", dir,

Please avoid plain sprintf() everywhere and use snprintf() instead.

> +    if (!csv)
> +    {
> +      printf("Error while saving %s\n", path);
> +      free(path);
> +      return;
> +    }

We have die(...) for that.

> +    for (uint8_t j = 0; j < recv->lanes_n; j++)

It's better to use plain integer types ("int" or "unsigned int") for
loop control variables.

				Have a nice fortnight
-- 
Martin `MJ' Mareš                        <mj@ucw.cz>   http://mj.ucw.cz/
United Computer Wizards, Prague, Czech Republic, Europe, Earth, Universe

Re: [PATCH 04/15] pciutils-pcilmr: Add functions for device checking and preparations before main margining processes

[Nikita Proshkin]

Hello Martin,
Thanks for the review!

On Fri, 8 Dec 2023 18:30:01 +0100
Martin Mareš <mj@ucw.cz> wrote:
 
> > -all: lib/$(PCIIMPLIB) lspci$(EXEEXT) setpci$(EXEEXT) example$(EXEEXT) lspci.8 setpci.8 pcilib.7 pci.ids.5 update-pciids update-pciids.8 $(PCI_IDS)
> > +all: lib/$(PCIIMPLIB) lspci$(EXEEXT) setpci$(EXEEXT) example$(EXEEXT) lspci.8 setpci.8 pcilib.7 pci.ids.5 update-pciids update-pciids.8 $(PCI_IDS) lmr_lib/liblmr.a
> 
> Is there any advantage with building LMR as a library instead of linking all
> the object files with the margining utility?

Actually, there are no advantages, I just thought that the Makefiles would
look more neat with this approach. I will redo the linking to make it 
consistent with the lspci building.

> > +bool margin_prep_dev(struct margin_dev *dev)
> > +{
> > +  struct pci_cap *pcie = pci_find_cap(dev->dev, PCI_CAP_ID_EXP, PCI_CAP_NORMAL);
> 
> What if it doesn't exist?

Nothing good at all. I will add more checks.

> > --- /dev/null
> > +++ b/lmr_lib/margin_hw.c
> > @@ -0,0 +1,85 @@
> > +#include <stdio.h>
> > +#include <string.h>
> > +#include <stdlib.h>
> 
> Generally: Please add a comment to every source file, which explains the
> purpose of the file and contains a copyright notice. See existing files
> for the recommeneded format.
> 
> > +  uint8_t down_type = pci_read_byte(down_port, PCI_HEADER_TYPE) & 0x7F;
> > +  uint8_t down_sec = pci_read_byte(down_port, PCI_SECONDARY_BUS);
> > +  uint8_t down_dir = (pci_read_word(down_port, cap->addr + PCI_EXP_FLAGS) & PCI_EXP_FLAGS_TYPE) >> 4;
> 
> I would prefer using libpci types (u8, u32 etc.).
> 
> > +  if (!(down_sec == up_port->bus && down_type == 1
> 
> Please avoid whitespace at the end of line.
> 
> > diff --git a/lmr_lib/margin_hw.h b/lmr_lib/margin_hw.h
> > new file mode 100644
> > index 0000000..a436d4b
> > --- /dev/null
> > +++ b/lmr_lib/margin_hw.h
> > @@ -0,0 +1,39 @@
> > +#ifndef _MARGIN_HW_H
> > +#define _MARGIN_HW_H
> > +
> > +#include <stdbool.h>
> > +#include <stdint.h>
> > +
> > +#include "../lib/pci.h"
> 
> Please do not use relative paths to libpci header files.
> Instead, supply proper include path to CC in the Makefile.
> 
> > +/*PCI Device wrapper for margining functions*/
> 
> Please surround "/*" and "*/" by spaces as in existing source files.

Got it, I'll rework the code.

Best regards,
Nikita Proshkin

Re: [PATCH 05/15] pciutils-pcilmr: Add margining process functions

[Nikita Proshkin]

On Fri, 8 Dec 2023 18:35:58 +0100
Martin Mareš <mj@ucw.cz> wrote:

> > +union margin_payload {
> > +  unsigned int payload : 8;
> > +
> > +  struct caps {
> > +    bool volt_support : 1;
> > +    bool ind_up_down_volt : 1;
> > +    bool ind_left_right_tim : 1;
> > +    bool sample_report_method : 1;
> > +    bool ind_error_sampler : 1;
> > +  } caps;
> > +
> > +  unsigned int timing_steps : 6;
> > +  unsigned int voltage_steps : 7;
> > +  unsigned int offset : 7;
> > +  unsigned int max_lanes : 5;
> > +  unsigned int sample_rate : 6;
> > +
> > +  struct step_resp {
> > +    unsigned int err_count : 6;
> > +    unsigned int status : 2;
> > +  } step_resp;
> > +
> > +  struct step_tim {
> > +    unsigned int steps : 6;
> > +    bool go_left : 1;
> > +  } step_tim;
> > +
> > +  struct step_volt {
> > +    unsigned int steps : 7;
> > +    bool go_down : 1;
> > +  } step_volt;
> > +
> > +} __attribute__((packed));
> 
> Please do not assume that every compiler used to compile the pciutils supports
> GCC extensions. See lib/sysdep.h for how we handle such things.
> 
> Also, I am not sure that bit fields are good idea: they save a little data
> space, but they expand code.

I had an idea to use anonymous structures here, which would make field
accesses shorter. And actually I would prefer to use bit fields instead of
manipulating bits. But looks like it will be hard to implement this
approach without using GCC packed feature and make it portable.
It seems that I will have to implement the option with a pack of macro 
wrappers around bit masks.

Best regards,
Nikita Proshkin

Re: [PATCH 10/15] pciutils-pcilmr: Add support for unique hardware quirks

[Nikita Proshkin]

On Fri, 8 Dec 2023 18:38:20 +0100
Martin Mareš <mj@ucw.cz> wrote:

> > +static bool read_cpuinfo(union cpuinfo *cpuinfo)
> > +{
> > +  FILE *cpu_file = fopen("/proc/cpuinfo", "r");
> > +  if (!cpu_file)
> > +    return false;
> 
> This works only on Linux.
> 
> Wouldn't it be possible to identify Ice Lake by PCI ID of the root bridge
> instead?

Thanks, great idea! This should work.

Best regards,
Nikita Proshkin

Re: [PATCH 13/15] pciutils-pcilmr: Add option to save margining results in csv form

[Nikita Proshkin]

On Fri, 8 Dec 2023 18:44:23 +0100
Martin Mareš <mj@ucw.cz> wrote:

> > +    if (!csv)
> > +    {
> > +      printf("Error while saving %s\n", path);
> > +      free(path);
> > +      return;
> > +    }
> 
> We have die(...) for that.

I think that die() from common.c is not suitable here, as the utility at that
moment in the code still can and must free up resources malloc'ed in the main
function. 

> > +  char timestamp[64];
> > +  time_t tim = time(NULL);
> > +  strftime(timestamp, 64, "%FT%H.%M.%S", gmtime(&tim));
> 
> Please use sizeof(timestamp) to make the code more robust.
> 
> > +  char *path = (char *)malloc((strlen(dir) + 128) * sizeof(*path));
> 
> Please use xmalloc().
> 
> > +    sprintf(path, "%s/lmr_%0*x.%02x.%02x.%x_Rx%X_%s.csv", dir,
> 
> Please avoid plain sprintf() everywhere and use snprintf() instead.
> 
> > +    for (uint8_t j = 0; j < recv->lanes_n; j++)
> 
> It's better to use plain integer types ("int" or "unsigned int") for
> loop control variables.

Everything else will be fixed. 

Best regards,
Nikita Proshkin

Re: [PATCH 15/15] pciutils-pcilmr: Add pcilmr man page

[Nikita Proshkin]

Hello Bjorn,
Thanks for the review!

On Fri, 8 Dec 2023 11:24:04 -0600
Bjorn Helgaas <helgaas@kernel.org> wrote:

> I'm not a hardware person, but this looks like interesting work!
> 
> On Fri, Dec 08, 2023 at 12:17:34PM +0300, Nikita Proshkin wrote:
> > +Turn off PCIE Leaky Bucket Feature, Re-Equalization and Link Degradation;
> 
> s/PCIE/PCIe/ to match other uses here.
> 
> > +The current Link data rate must be 16.0 GT/s or higher (right now
> > +utility supports Gen 4 and 5 Links);
> 
> So far, each major PCIe spec revision has added a single new data
> rate, but that may not always be true, and the spec always uses
> terminology like "16.0 GT/s or higher" instead of terms like "Gen 4".
> 
> So "supports 16.0 GT/s and 32.0 GT/s Links" might be clearer.
> 
> > +The Gen 5 Specification sets allowed range for Timing Margin from 20%\~UI to 50%\~UI and
> 
> Usage in the spec itself would be more like "PCIe Base Spec Revision 5.0"
> since it doesn't use "Gen 5".
> 
> > +According to spec it's possible for Receiver to margin up to MaxLanes + 1
> > +lanes simultaneously, but usually this works bad, so this option is for
> 
> s/works bad/works poorly/
> 
> > +experiments mostly.

Got it, I will style check the Man page again.

Best regards,
Nikita Proshkin

Re: [PATCH 13/15] pciutils-pcilmr: Add option to save margining results in csv form

[Martin Mareš]

Hi1

> I think that die() from common.c is not suitable here, as the utility at that
> moment in the code still can and must free up resources malloc'ed in the main
> function.

I don't understand why this is needed -- once the process exits, all memory
allocated by it is freed by the kernel.

					Martin

Re: [PATCH 13/15] pciutils-pcilmr: Add option to save margining results in csv form

[Nikita Proshkin]

On Wed, 13 Dec 2023 12:22:45 +0100
Martin Mareš <mj@ucw.cz> wrote:

> > I think that die() from common.c is not suitable here, as the utility at that
> > moment in the code still can and must free up resources malloc'ed in the main
> > function.
> 
> I don't understand why this is needed -- once the process exits, all memory
> allocated by it is freed by the kernel.

Well, yes, I suppose in case of most modern operating systems you are right.

This particular function, about which this thread is about, was conceived as
an optional method to save the results of the utility to a file. According to
the logic of the utility, by this point the testing process has already been
successfully completed and it seemed to me that the utility should no longer
panic.

On the other hand, it makes sense to return a non-zero code to the user if
something went wrong. In fact, there are a few more cases in the main function
of the utility when some checks fail and the utility may exit. If we are
satisfied with the option when the utility does not clean up gracefully,
I can use die() in such cases.

Best regards,
Nikita Proshkin