Re: [PATCH 1/1] PCI: Fix Extend ACS configurability

[Jason Gunthorpe <jgg@nvidia.com>]

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..

> 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.

What about your fix to the mask changes this reasoning? If nothing
then it should be its own patch with its own explanation..

Jason