pcilmr: Ensure that utility can accept either Downstream or Upstream link port

Previously, the utility expected only the Upstream Port to be input and,
in fact, passing the Downstream Port led to strange and buggy error
messages. Improve arguments parsing logic to accept any side of the link.

It seems that the only use case that will not be available now is margining
the internal links of the switch, but this scenario looks as strange as
possible.

Signed-off-by: Nikita Proshkin <n.proshkin@yadro.com>

diff --git a/lmr/lmr.h b/lmr/lmr.h
index 7375c33f6358a7b676c4c00daee000309aa0fc78..6cac3d47b6b21b9b60f36750ed4c5096ae2219e3 100644 (file)
--- a/lmr/lmr.h
+++ b/lmr/lmr.h
@@ -1,7 +1,7 @@
 /*
  *     The PCI Utilities -- Margining utility main header
  *
- *     Copyright (c) 2023 KNS Group LLC (YADRO)
+ *     Copyright (c) 2023-2024 KNS Group LLC (YADRO)
  *
  *     Can be freely distributed and used under the terms of the GNU GPL v2+.
  *
@@ -95,7 +95,7 @@ enum margin_test_status {
 
 /* All lanes Receiver results */
 struct margin_results {
-  u8 recvn; // Receiver Number
+  u8 recvn; // Receiver Number; from 1 to 6
   struct margin_params params;
   bool lane_reversal;
   u8 link_speed;
@@ -104,7 +104,7 @@ struct margin_results {
 
   /* Used to convert steps to physical quantity.
      Calculated from MaxOffset and NumSteps     */
-  double tim_coef;
+  double tim_coef; // from steps to % UI
   double volt_coef;
 
   bool tim_off_reported;
@@ -134,7 +134,7 @@ struct margin_args {
 /* Receiver structure */
 struct margin_recv {
   struct margin_dev *dev;
-  u8 recvn; // Receiver Number
+  u8 recvn; // Receiver Number; from 1 to 6
   bool lane_reversal;
   struct margin_params *params;
 
@@ -161,6 +161,12 @@ struct margin_lanes_data {
 
 /* margin_hw */
 
+bool margin_port_is_down(struct pci_dev *dev);
+
+/* Results through down/up ports */
+bool margin_find_pair(struct pci_access *pacc, struct pci_dev *dev, struct pci_dev **down_port,
+                      struct pci_dev **up_port);
+
 /* Verify that devices form the link with 16 GT/s or 32 GT/s data rate */
 bool margin_verify_link(struct pci_dev *down_port, struct pci_dev *up_port);
 

diff --git a/lmr/margin.c b/lmr/margin.c
index a8c65712a52c26cf8487269d176704deeb75cdfc..e3758dfee7547cdbe6fbff2a5fdf20ac7f04f26f 100644 (file)
--- a/lmr/margin.c
+++ b/lmr/margin.c
@@ -426,13 +426,8 @@ margin_read_params(struct pci_access *pacc, struct pci_dev *dev, u8 recvn,
   struct pci_cap *cap = pci_find_cap(dev, PCI_CAP_ID_EXP, PCI_CAP_NORMAL);
   if (!cap)
     return false;
-  u8 dev_dir = GET_REG_MASK(pci_read_word(dev, cap->addr + PCI_EXP_FLAGS), PCI_EXP_FLAGS_TYPE);
 
-  bool dev_down;
-  if (dev_dir == PCI_EXP_TYPE_ROOT_PORT || dev_dir == PCI_EXP_TYPE_DOWNSTREAM)
-    dev_down = true;
-  else
-    dev_down = false;
+  bool dev_down = margin_port_is_down(dev);
 
   if (recvn == 0)
     {
@@ -453,25 +448,7 @@ margin_read_params(struct pci_access *pacc, struct pci_dev *dev, u8 recvn,
   struct pci_dev *up = NULL;
   struct margin_link link;
 
-  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)
+  if (!margin_find_pair(pacc, dev, &down, &up))
     return false;
 
   if (!margin_fill_link(down, up, &link))

diff --git a/lmr/margin_hw.c b/lmr/margin_hw.c
index fc427c8f50ec58a3ef0b7b28a080c4c456a191aa..43fa1bdcc7069dcd0aa71ce0325dffe31407794a 100644 (file)
--- a/lmr/margin_hw.c
+++ b/lmr/margin_hw.c
@@ -1,7 +1,7 @@
 /*
  *     The PCI Utilities -- Verify and prepare devices before margining
  *
- *     Copyright (c) 2023 KNS Group LLC (YADRO)
+ *     Copyright (c) 2023-2024 KNS Group LLC (YADRO)
  *
  *     Can be freely distributed and used under the terms of the GNU GPL v2+.
  *
@@ -31,6 +31,51 @@ detect_unique_hw(struct pci_dev *dev)
   return MARGIN_HW_DEFAULT;
 }
 
+bool
+margin_port_is_down(struct pci_dev *dev)
+{
+  struct pci_cap *cap = pci_find_cap(dev, PCI_CAP_ID_EXP, PCI_CAP_NORMAL);
+  if (!cap)
+    return false;
+  u8 type = pci_read_byte(dev, PCI_HEADER_TYPE) & 0x7F;
+  u8 dir = GET_REG_MASK(pci_read_word(dev, cap->addr + PCI_EXP_FLAGS), PCI_EXP_FLAGS_TYPE);
+
+  if (type == PCI_HEADER_TYPE_BRIDGE
+      && (dir == PCI_EXP_TYPE_ROOT_PORT || dir == PCI_EXP_TYPE_DOWNSTREAM))
+    return true;
+  else
+    return false;
+}
+
+bool
+margin_find_pair(struct pci_access *pacc, struct pci_dev *dev, struct pci_dev **down_port,
+                 struct pci_dev **up_port)
+{
+  struct pci_cap *cap = pci_find_cap(dev, PCI_CAP_ID_EXP, PCI_CAP_NORMAL);
+  if (!cap)
+    return false;
+  bool given_down = margin_port_is_down(dev);
+
+  for (struct pci_dev *p = pacc->devices; p; p = p->next)
+    {
+      if (given_down && pci_read_byte(dev, PCI_SECONDARY_BUS) == p->bus && dev->domain == p->domain
+          && p->func == 0)
+        {
+          *down_port = dev;
+          *up_port = p;
+          return true;
+        }
+      else if (!given_down && pci_read_byte(p, PCI_SECONDARY_BUS) == dev->bus
+               && dev->domain == p->domain)
+        {
+          *down_port = p;
+          *up_port = dev;
+          return true;
+        }
+    }
+  return false;
+}
+
 bool
 margin_verify_link(struct pci_dev *down_port, struct pci_dev *up_port)
 {
@@ -42,16 +87,11 @@ margin_verify_link(struct pci_dev *down_port, struct pci_dev *up_port)
   if ((pci_read_word(down_port, cap->addr + PCI_EXP_LNKSTA) & PCI_EXP_LNKSTA_SPEED) > 5)
     return false;
 
-  u8 down_type = pci_read_byte(down_port, PCI_HEADER_TYPE) & 0x7F;
   u8 down_sec = pci_read_byte(down_port, PCI_SECONDARY_BUS);
-  u8 down_dir
-    = GET_REG_MASK(pci_read_word(down_port, cap->addr + PCI_EXP_FLAGS), PCI_EXP_FLAGS_TYPE);
 
   // 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 == PCI_HEADER_TYPE_BRIDGE
-        && (down_dir == PCI_EXP_TYPE_ROOT_PORT || down_dir == PCI_EXP_TYPE_DOWNSTREAM)
-        && up_port->func == 0))
+  if (!(down_sec == up_port->bus && margin_port_is_down(down_port) && up_port->func == 0))
     return false;
 
   struct pci_cap *pm = pci_find_cap(up_port, PCI_CAP_ID_PM, PCI_CAP_NORMAL);

diff --git a/pcilmr.c b/pcilmr.c
index cb8bd77a04fded0a3d8c3c811aa040d538778717..345ce7ab26e75befabec76e78c9ed083821e8053 100644 (file)
--- a/pcilmr.c
+++ b/pcilmr.c
@@ -1,7 +1,7 @@
 /*
  *     The PCI Utilities -- Margining utility main function
  *
- *     Copyright (c) 2023 KNS Group LLC (YADRO)
+ *     Copyright (c) 2023-2024 KNS Group LLC (YADRO)
  *
  *     Can be freely distributed and used under the terms of the GNU GPL v2+.
  *
@@ -83,21 +83,6 @@ dev_for_filter(struct pci_access *pacc, char *filter)
   die("No such PCI device: %s or you don't have enough privileges.\n", filter);
 }
 
-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 u8
 parse_csv_arg(char *arg, u8 *vals)
 {
@@ -120,11 +105,13 @@ scan_links(struct pci_access *pacc, bool only_ready)
   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)
+  for (struct pci_dev *p = pacc->devices; p; p = p->next)
     {
-      if (pci_find_cap(up, PCI_EXT_CAP_ID_LMR, PCI_CAP_EXTENDED))
+      if (pci_find_cap(p, PCI_EXT_CAP_ID_LMR, PCI_CAP_EXTENDED) && margin_port_is_down(p))
         {
-          struct pci_dev *down = find_down_port_for_up(pacc, up);
+          struct pci_dev *down = NULL;
+          struct pci_dev *up = NULL;
+          margin_find_pair(pacc, p, &down, &up);
 
           if (down && margin_verify_link(down, up))
             {
@@ -147,11 +134,13 @@ find_ready_links(struct pci_access *pacc, struct pci_dev **down_ports, struct pc
                  bool cnt_only)
 {
   u8 cnt = 0;
-  for (struct pci_dev *up = pacc->devices; up; up = up->next)
+  for (struct pci_dev *p = pacc->devices; p; p = p->next)
     {
-      if (pci_find_cap(up, PCI_EXT_CAP_ID_LMR, PCI_CAP_EXTENDED))
+      if (pci_find_cap(p, PCI_EXT_CAP_ID_LMR, PCI_CAP_EXTENDED) && margin_port_is_down(p))
         {
-          struct pci_dev *down = find_down_port_for_up(pacc, up);
+          struct pci_dev *down = NULL;
+          struct pci_dev *up = NULL;
+          margin_find_pair(pacc, p, &down, &up);
 
           if (down && margin_verify_link(down, up)
               && (margin_check_ready_bit(down) || margin_check_ready_bit(up)))
@@ -328,10 +317,9 @@ main(int argc, char **argv)
       u8 cnt = 0;
       while (optind != argc)
         {
-          up_ports[cnt] = dev_for_filter(pacc, argv[optind]);
-          down_ports[cnt] = find_down_port_for_up(pacc, up_ports[cnt]);
-          if (!down_ports[cnt])
-            die("Cannot find Upstream Component for the specified device: %s\n", argv[optind]);
+          struct pci_dev *dev = dev_for_filter(pacc, argv[optind]);
+          if (!margin_find_pair(pacc, dev, &(down_ports[cnt]), &(up_ports[cnt])))
+            die("Cannot find pair for the specified device: %s\n", argv[optind]);
           cnt++;
           optind++;
         }