[PATCH v6 4/6] PCI: Refactor disable_acs_redir and config_acs param handling

Wei Wang
2026-03-12T09:07:07Z

The current implementation mixes disable_acs_redir and config_acs param
processing inside __pci_config_acs(), using acs_mask==0 as an implicit
signal to switch into config_acs_param mode. The intertwined logic is
hard to follow and easy to break:

- The acs_mask==0 special case is non-obvious and obscures which code
paths apply to which parameter.
- The interleaved logic is fragile; changes intended for one parameter
can unintentionally affect the other. For example,
pci_dev_specific_disable_acs_redir() is invoked on the common path
even though it should apply only to disable_acs_redir.

Split the two behaviors into dedicated functions,
pci_param_disable_acs_redir() and pci_param_config_acs(), making the
control flow explicit and easier to maintain.

The new pci_param_config_acs() implementation also improves on the
original logic:

- It searches for the matching device first and parses ACS flags only
for the matched device, avoiding unnecessary work.
- A switch statement replaces multiple if/else blocks, improving
readability.
- Bounds checking (max_shift) prevents overrunning the device’s ACS
capability bits.
- Variable names (enabled_bits, disabled_bits) more clearly express
their roles compared to the previous mask/flags pair.
- Error messages are simplified to reduce dmesg noise.

This refactor separates concerns, improves robustness, and makes future
extensions to ACS parameter handling safer and easier to review.

Signed-off-by: Wei Wang <wei.w.wang@hotmail.com>
---
drivers/pci/pci.c | 168 +++++++++++++++++++++++++++-------------------
1 file changed, 98 insertions(+), 70 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 4d0fb70c4bed..66842df07589 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -910,87 +910,117 @@ struct pci_acs {
u16 fw_ctrl;
};

-static void __pci_config_acs(struct pci_dev *dev, struct pci_acs *caps,
- const char *p, const u16 acs_mask, const u16 acs_flags)
+static bool pci_dev_match_disable_acs_redir(struct pci_dev *dev, const char *p)
{
- u16 flags = acs_flags;
- u16 mask = acs_mask;
- char *delimit;
int ret = 0;

- if (!p)
- return;
-
while (*p) {
- if (!acs_mask) {
- /* Check for ACS flags */
- delimit = strstr(p, "@");
- if (delimit) {
- int end;
- u32 shift = 0;
-
- end = delimit - p - 1;
- mask = 0;
- flags = 0;
-
- while (end > -1) {
- if (*(p + end) == '0') {
- mask |= 1 << shift;
- shift++;
- end--;
- } else if (*(p + end) == '1') {
- mask |= 1 << shift;
- flags |= 1 << shift;
- shift++;
- end--;
- } else if ((*(p + end) == 'x') || (*(p + end) == 'X')) {
- shift++;
- end--;
- } else {
- pci_err(dev, "Invalid ACS flags... Ignoring\n");
- return;
- }
- }
- p = delimit + 1;
- } else {
- pci_err(dev, "ACS Flags missing\n");
- return;
- }
- }
-
ret = pci_dev_str_match(dev, p, &p);
- if (ret < 0) {
+ if (ret == 1)
+ return true;
+ else if (ret < 0)
break;
- } else if (ret == 1) {
- /* Found a match */
- break;
- }
}

- if (ret != 1)
+ return false;
+}
+
+static void pci_param_disable_acs_redir(struct pci_dev *dev,
+ struct pci_acs *caps)
+{
+ u16 acs_redir_mask = PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_EC;
+
+ if (!disable_acs_redir_param ||
+ !pci_dev_match_disable_acs_redir(dev, disable_acs_redir_param) ||
+ !pci_dev_specific_disable_acs_redir(dev))
return;

- if (!pci_dev_specific_disable_acs_redir(dev))
- return;
+ caps->ctrl = caps->fw_ctrl & ~acs_redir_mask;
+}

- if (flags & ~dev->acs_capabilities) {
- pci_err(dev, "Invalid ACS enable flags specified: %#06x\n",
- (u16)(flags & ~dev->acs_capabilities));
- return;
- }
-
- pci_dbg(dev, "ACS mask = %#06x\n", mask);
- pci_dbg(dev, "ACS flags = %#06x\n", flags);
- pci_dbg(dev, "ACS control = %#06x\n", caps->ctrl);
- pci_dbg(dev, "ACS fw_ctrl = %#06x\n", caps->fw_ctrl);
+static const char *pci_dev_match_config_acs(struct pci_dev *dev, const char *p)
+{
+ const char *seg;
+ int ret;

/*
- * For mask bits that are 0, copy them from the firmware setting
- * and apply flags for all the mask bits that are 1.
+ * Example: 11101010x@0000:01:00.0;101x@0000:02:00.0;1@0000:03:00.0
+ * Three segments delimited via ';'. @seg always points to the start
+ * of a segment and p advances to the start of the device id (BDF)
+ * after '@'. Upon match, return the start of the matching segment.
*/
- caps->ctrl = (caps->fw_ctrl & ~mask) | (flags & mask);
+ while (*p) {
+ seg = p;
+ p = strchr(seg, '@');
+ /* If malformed string, stop parsing. */
+ if (!p || p == seg)
+ break;

- pci_info(dev, "Configured ACS to %#06x\n", caps->ctrl);
+ p++;
+ /* pci_dev_str_match() updates p to the next segment. */
+ ret = pci_dev_str_match(dev, p, &p);
+ if (ret == 1)
+ return seg;
+ else if (ret < 0)
+ break;
+ }
+
+ return NULL;
+}
+
+static void pci_param_config_acs(struct pci_dev *dev, struct pci_acs *caps)
+{
+ u16 shift = 0, max_shift = fls(dev->acs_capabilities) - 1;
+ u16 enabled_bits = 0, disabled_bits = 0;
+ const char *p, *seg;
+
+ if (!config_acs_param)
+ return;
+
+ seg = pci_dev_match_config_acs(dev, config_acs_param);
+ if (!seg)
+ return;
+
+ p = strchr(seg, '@');
+ /* Parse bitstring backwards from '@' */
+ while (p > seg) {
+ if (shift > max_shift) {
+ pci_err(dev, "ACS flag bit %d exceed range %d\n",
+ shift, max_shift);
+ return;
+ }
+
+ switch (*--p) {
+ case '1':
+ enabled_bits |= BIT(shift);
+ break;
+ case '0':
+ disabled_bits |= BIT(shift);
+ break;
+ case 'x':
+ case 'X':
+ /* Skip this bit */
+ break;
+ default:
+ pci_err(dev, "Invalid ACS flag character '%c'\n", *p);
+ return;
+ }
+ shift++;
+ }
+
+ if (enabled_bits & ~dev->acs_capabilities) {
+ pci_err(dev, "Invalid ACS enable flags specified: %#06x\n",
+ (u16)(enabled_bits & ~dev->acs_capabilities));
+ return;
+ }
+
+ pci_dbg(dev, "ACS enabled: %#06x, disabled: %#06x\n",
+ enabled_bits, disabled_bits);
+
+ caps->ctrl = (caps->fw_ctrl | enabled_bits) & ~disabled_bits;
+
+ pci_info(dev, "Configured ACS to %#06x (FW default: %#06x)\n",
+ caps->ctrl, caps->fw_ctrl);
}

/**
@@ -1047,10 +1077,8 @@ void pci_enable_acs(struct pci_dev *dev)
* Always apply caps from the command line, even if there is no iommu.
* Trust that the admin has a reason to change the ACS settings.
*/
- __pci_config_acs(dev, &caps, disable_acs_redir_param,
- PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_EC,
- ~(PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_EC));
- __pci_config_acs(dev, &caps, config_acs_param, 0, 0);
+ pci_param_disable_acs_redir(dev, &caps);
+ pci_param_config_acs(dev, &caps);

pci_write_config_word(dev, pos + PCI_ACS_CTRL, caps.ctrl);
}
--
2.51.0