Skip to content
Permalink
Browse files

Enable setpci to target n-th capability of id

Because a capability can exist multiple times with the same id,
there needs to be a way to target a specific one. Instead of
the current behaviour which always targets the first one.
Now you can optionally add `@Number` (e.g `@1`) after the width to
choose which one to target.
  • Loading branch information
JohnAZoidberg authored and gollux committed Oct 14, 2018
1 parent c22c314 commit 6639fd175ae561240639c2c54da409bbc0f94835
Showing with 33 additions and 7 deletions.
  1. +28 −5 setpci.c
  2. +5 −2 setpci.man
@@ -36,6 +36,7 @@ struct op {
unsigned int addr;
unsigned int width; /* Byte width of the access */
unsigned int num_values; /* Number of values to write; 0=read */
unsigned int number; /* The n-th capability of that id */
struct value values[0];
};

@@ -84,11 +85,19 @@ exec_op(struct op *op, struct pci_dev *dev)
if (op->cap_type)
{
struct pci_cap *cap;
cap = pci_find_cap(dev, op->cap_id, op->cap_type);
unsigned int cap_nr = op->number;
cap = pci_find_cap_nr(dev, op->cap_id, op->cap_type, &cap_nr);
if (cap)
addr = cap->addr;
addr = cap->addr;
else if (cap_nr == 0)
die("%s: Instance #%d of %s %04x not found - there are no capabilities with that id.", slot,
op->number, ((op->cap_type == PCI_CAP_NORMAL) ? "Capability" : "Extended capability"),
op->cap_id);
else
die("%s: %s %04x not found", slot, ((op->cap_type == PCI_CAP_NORMAL) ? "Capability" : "Extended capability"), op->cap_id);
die("%s: Instance #%d of %s %04x not found - there %s only %d capability with that id.", slot,
op->number, ((op->cap_type == PCI_CAP_NORMAL) ? "Capability" : "Extended capability"),
op->cap_id, ((cap_nr == 1) ? "is" : "are"), cap_nr);

trace(((op->cap_type == PCI_CAP_NORMAL) ? "(cap %02x @%02x) " : "(ecap %04x @%03x) "), op->cap_id, addr);
}
addr += op->addr;
@@ -341,7 +350,7 @@ GENERIC_HELP
"Setting commands:\n"
"<device>:\t-s [[[<domain>]:][<bus>]:][<slot>][.[<func>]]\n"
"\t\t-d [<vendor>]:[<device>]\n"
"<reg>:\t\t<base>[+<offset>][.(B|W|L)]\n"
"<reg>:\t\t<base>[+<offset>][.(B|W|L)][@<number>]\n"
"<base>:\t\t<address>\n"
"\t\t<named-register>\n"
"\t\t[E]CAP_<capability-name>\n"
@@ -557,7 +566,7 @@ static void parse_register(struct op *op, char *base)

static void parse_op(char *c, struct pci_dev **selected_devices)
{
char *base, *offset, *width, *value;
char *base, *offset, *width, *value, *number;
char *e, *f;
int n, j;
struct op *op;
@@ -566,6 +575,8 @@ static void parse_op(char *c, struct pci_dev **selected_devices)
base = xstrdup(c);
if (value = strchr(base, '='))
*value++ = 0;
if (number = strchr(base, '@'))
*number++ = 0;
if (width = strchr(base, '.'))
*width++ = 0;
if (offset = strchr(base, '+'))
@@ -608,6 +619,18 @@ static void parse_op(char *c, struct pci_dev **selected_devices)
else
op->width = 0;

/* Check which n-th capability of the same id we want */
if (number)
{
unsigned int num;
if (parse_x32(number, NULL, &num) <= 0 || (int) num < 0)
parse_err("Invalid number \"%s\"", number);
op->number = num;

}
else
op->number = 0;

/* Find the register */
parse_register(op, base);
if (!op->width)
@@ -148,9 +148,12 @@ Each of the previous formats can be followed by \fB+offset\fP to add an offset
(a hex number) to the address. This feature can be useful for addressing of registers
living within a capability, or to modify parts of standard registers.
.IP \(bu
Finally, you should append a width specifier \fB.B\fP, \fB.W\fP, or \fB.L\fP to choose
how many bytes (1, 2, or 4) should be transferred. The width can be omitted if you are
To choose how many bytes (1, 2, or 4) should be transferred, you should append a width
specifier \fB.B\fP, \fB.W\fP, or \fB.L\fP. The width can be omitted if you are
referring to a register by its name and the width of the register is well known.
.IP \(bu
Finally, if a capability exists multiple times you can choose which one to target using
\fB@number\fP. Indexing starts at 0.

.PP
All names of registers and width specifiers are case-insensitive.

5 comments on commit 6639fd1

@jic23

This comment has been minimized.

Copy link

jic23 replied Nov 21, 2018

This is a great thing to have, but is actually operating in the reverse of what I would expect. I.e. if there are two and you do @0 you get 1 and @1 you get 0. This due to how pci_add_cap is implemented. If effectively adds them in the reverse order. We could fix by calling the find twice, firstly to discover how many and then to find N - index.

@jic23

This comment has been minimized.

Copy link

jic23 replied Nov 21, 2018

diff` --git a/setpci.c b/setpci.c
  index 73c4d03..fce488f 100644
--- a/setpci.c
+++ b/setpci.c
@@ -85,19 +85,21 @@ exec_op(struct op *op, struct pci_dev *dev)
   if (op->cap_type)
     {
       struct pci_cap *cap;
-      unsigned int cap_nr = op->number;
-      cap = pci_find_cap_nr(dev, op->cap_id, op->cap_type, &cap_nr);
-      if (cap)
-        addr = cap->addr;
-      else if (cap_nr == 0)
+      unsigned int cap_nr;
+      pci_find_cap_nr(dev, op->cap_id, op->cap_type, &cap_nr);
+      if (cap_nr == 0)
         die("%s: Instance #%d of %s %04x not found - there are no capabilities with that id.", slot,
             op->number, ((op->cap_type == PCI_CAP_NORMAL) ? "Capability" : "Extended capability"),
             op->cap_id);
-      else
+      else if (op->number >= cap_nr)
         die("%s: Instance #%d of %s %04x not found - there %s only %d %s with that id.", slot,
             op->number, ((op->cap_type == PCI_CAP_NORMAL) ? "Capability" : "Extended capability"),
             op->cap_id, ((cap_nr == 1) ? "is" : "are"), cap_nr,
             ((cap_nr == 1) ? "capability" : "capabilities"));
+      cap_nr = cap_nr - op->number - 1;
+      cap = pci_find_cap_nr(dev, op->cap_id, op->cap_type, &cap_nr);
+      if (cap)
+        addr = cap->addr;

       trace(((op->cap_type == PCI_CAP_NORMAL) ? "(cap %02x @%02x) " : "(ecap %04x @%03x) "), op->cap_id, addr);
     }
@gollux

This comment has been minimized.

Copy link
Contributor

gollux replied Nov 21, 2018

@jic23

This comment has been minimized.

Copy link

jic23 replied Nov 21, 2018

Agreed. I don't have the overview of whether that might have side effects elsewhere, but that is indeed a nicer fix! Thanks,

@gollux

This comment has been minimized.

Copy link
Contributor

gollux replied Dec 31, 2018

Please sign in to comment.
You can’t perform that action at this time.