On Tue, Jan 07, 2025 at 06:32:42PM -0800, Tushar Dave wrote: > > > On 1/6/25 16:10, Jason Gunthorpe wrote: > > On Mon, Jan 06, 2025 at 12:34:00PM -0800, Tushar Dave wrote: > > > > > > > @@ -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? > > > > > > No, it does not break the unchanged functionality. I removed it because it > > > is not needed after my fix. > > > > I mean, the whole hunk above is not needed, right? Or at least I don't > > see how it relates to your commit message.. > > Without the above hunk, there are cases where ACS flags do not get set > correctly. Here is the example test case without above hunk in my patch (IOW > keeping fw_ctrl changes as it is like original code plus pci_dbg to print > debug info) Isn't that because the bit logic in the code is wrong? > - /* If mask is 0 then we copy the bit from the firmware setting. */ > - caps->ctrl = (caps->ctrl & ~mask) | (caps->fw_ctrl & mask); That comment doesn't match the calculation at all. > > > If it helps, using 'config_acs' the code only allows to configures the lower > > > 7 bits of ACS ctrl for the specified PCI device(s). > > > The bits other than the lower 7 bits of ACS ctrl remain unchanged. > > > The bits specified with 'x' or 'X' that are within the 7 lower bits remain > > > unchanged. Trying to configure bits other than lower 7 bits generates an > > > error message "Invalid ACS flags specified" > > > > But the fw_ctrl was how the x behavior was supposed to be implemented, > > IIRC there were cases where you could not just rely on caps->ctrl as > > something prior had alreaded changed it. > > I read your comment in https://lore.kernel.org/all/20240508131427.GH4650@xxxxxxxxxx/ > > Looking at the current code, the sequence begin with function > 'pci_enable_acs()' that reads PCI_ACS_CTRL into caps->ctrl and invoke the > below three functions that prepares caps->ctrl before writing to ACS CTRL > reg. caps->ctrl is supposed to be the target setting and it is supposed to evolve as it progresses. > i.e. > pci_std_enable_acs() > __pci_config_acs(dev, &caps,disable_acs_redir_param,...) > __pci_config_acs(dev, &caps, config_acs_param, 0, 0) > > Here kernel command line takes precedence over 'pci_std_enable_acs()'. > 'config_acs' takes precedence over 'disable_acs_redir'. IOW, if config_acs > param is used then it takes the final control over what value is getting > written to ACS CTRL reg and that is how it should be, no? Yes, but X in config_acs should copy the FW value not the value modified by disable_acs_redir_param Jason
