On Fri, Dec 13, 2024 at 12:29:42PM -0800, Tushar Dave wrote:
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index dc663c0ca670..fc1c37910d1c 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -4654,11 +4654,10 @@
> Format:
> <ACS flags>@<pci_dev>[; ...]
> Specify one or more PCI devices (in the format
> - specified above) optionally prepended with flags
> - and separated by semicolons. The respective
> - capabilities will be enabled, disabled or
> - unchanged based on what is specified in
> - flags.
> + specified above) prepended with flags and separated
> + by semicolons. The respective capabilities will be
> + enabled, disabled or unchanged based on what is
> + specified in flags.
>
> ACS Flags is defined as follows:
> bit-0 : ACS Source Validation
> @@ -4673,7 +4672,7 @@
> '1' – force enabled
> 'x' – unchanged
> For example,
> - pci=config_acs=10x
> + pci=config_acs=10x@pci:0:0
> would configure all devices that support
> ACS to enable P2P Request Redirect, disable
> Translation Blocking, and leave Source
Is this an unrelated change? The format of the command line shouldn't
be changed to fix the described bug, why is the documentation changed?
> static void __pci_config_acs(struct pci_dev *dev, struct pci_acs *caps,
> - const char *p, u16 mask, u16 flags)
> + const char *p, const u16 acs_mask, const u16 acs_flags)
> {
> + u16 flags = acs_flags;
> + u16 mask = acs_mask;
> char *delimit;
> int ret = 0;
>
> @@ -964,7 +965,7 @@ static void __pci_config_acs(struct pci_dev *dev, struct pci_acs *caps,
> return;
>
> while (*p) {
> - if (!mask) {
> + if (!acs_mask) {
> /* Check for ACS flags */
> delimit = strstr(p, "@");
> if (delimit) {
> @@ -972,6 +973,8 @@ static void __pci_config_acs(struct pci_dev *dev, struct pci_acs *caps,
> u32 shift = 0;
>
> end = delimit - p - 1;
> + mask = 0;
> + flags = 0;
>
> while (end > -1) {
> if (*(p + end) == '0') {
This function the entire fix, right? Because the routine was
clobbering acs_mask as it processed the earlier devices?
> @@ -1028,10 +1031,10 @@ static void __pci_config_acs(struct pci_dev *dev, struct pci_acs *caps,
>
> 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);
>
> - /* If mask is 0 then we copy the bit from the firmware setting. */
> - caps->ctrl = (caps->ctrl & ~mask) | (caps->fw_ctrl & mask);
> - caps->ctrl |= flags;
> + caps->ctrl &= ~mask;
> + caps->ctrl |= (flags & mask);
And why delete fw_ctrl? Doesn't that break the unchanged
functionality?
Jason
