On Tue, May 02, 2023 at 11:02:24AM -0500, Bjorn Helgaas wrote: > On Tue, May 02, 2023 at 06:32:50PM +0530, Ajay Agarwal wrote: > > On Mon, May 01, 2023 at 12:44:39PM -0500, Bjorn Helgaas wrote: > > > On Tue, Apr 11, 2023 at 04:40:33PM +0530, Ajay Agarwal wrote: > > > > Currently the aspm driver does not set ASPM_STATE_L1 bit in > > > > aspm_default when the class driver requests L1SS ASPM state. > > > > This will lead to pcie_config_aspm_link() not enabling the > > > > requested L1SS state. Set ASPM_STATE_L1 when class driver > > > > enables L1ss. > > > > > > Since vmd is currently the only caller of pci_enable_link_state(), and > > > it supplies PCIE_LINK_STATE_ALL: > > > > > > #define PCIE_LINK_STATE_ALL (PCIE_LINK_STATE_L0S | PCIE_LINK_STATE_L1 |\ > > > PCIE_LINK_STATE_CLKPM | PCIE_LINK_STATE_L1_1 |\ > > > PCIE_LINK_STATE_L1_2 | PCIE_LINK_STATE_L1_1_PCIPM |\ > > > PCIE_LINK_STATE_L1_2_PCIPM) > > > > > > I don't think this makes any functional difference at this point, > > > right? > > > > Yes, this does not make any functional difference to the vmd driver. > > ... > > > > > @@ -1170,16 +1170,16 @@ int pci_enable_link_state(struct pci_dev *pdev, int state) > > > > if (state & PCIE_LINK_STATE_L0S) > > > > link->aspm_default |= ASPM_STATE_L0S; > > > > if (state & PCIE_LINK_STATE_L1) > > > > - /* L1 PM substates require L1 */ > > > > - link->aspm_default |= ASPM_STATE_L1 | ASPM_STATE_L1SS; > > > > + link->aspm_default |= ASPM_STATE_L1; > > > > + /* L1 PM substates require L1 */ > > > > if (state & PCIE_LINK_STATE_L1_1) > > > > - link->aspm_default |= ASPM_STATE_L1_1; > > > > + link->aspm_default |= ASPM_STATE_L1_1 | ASPM_STATE_L1; > > > > > > IIUC, this: > > > > > > pci_enable_link_state(PCIE_LINK_STATE_L1_1) > > > > > > currently doesn't actually enable L1.1 because the caller didn't > > > supply "PCIE_LINK_STATE_L1 | PCIE_LINK_STATE_L1_1". > > > > > > I'm not sure that's a problem -- the driver can easily supply both if > > > it wants both. > > > > Consider this: A driver wants to enable L1.1. So it calls: > > pci_enable_link_state(PCIE_LINK_STATE_L1 | PCIE_LINK_STATE_L1_1) > > The current logic will end up enabling L1.2 as well. The driver does > > not want that. > > Hmmm, I think I see what you mean. ASPM_STATE_L1SS includes both > ASPM_STATE_L1_1 and ASPM_STATE_L1_2: > > #define ASPM_STATE_L1_2_MASK (ASPM_STATE_L1_2 | ASPM_STATE_L1_2_PCIPM) > #define ASPM_STATE_L1SS (ASPM_STATE_L1_1 | ASPM_STATE_L1_1_PCIPM |\ > ASPM_STATE_L1_2_MASK) > > so this sets ASPM_STATE_L1_1 and ASPM_STATE_L1_2: > > if (state & PCIE_LINK_STATE_L1) > link->aspm_default |= ASPM_STATE_L1 | ASPM_STATE_L1SS; > > which makes it pointless for a caller to supply PCIE_LINK_STATE_L1_1 > or PCIE_LINK_STATE_L1_2: > > if (state & PCIE_LINK_STATE_L1_1) > link->aspm_default |= ASPM_STATE_L1_1; > if (state & PCIE_LINK_STATE_L1_2) > link->aspm_default |= ASPM_STATE_L1_2; > > > Also, we should be letting the ASPM core driver handle the logic that > > L1.0 needs to be set for L1.1/L1.2 to happen, instead of putting that > > responsibility to the caller driver. > > > > > For devices that support only L1, > > > "pci_enable_link_state(PCIE_LINK_STATE_L1_1)" would implicitly enable > > > L1 even though L1.1 is not supported, which seems a little bit weird. > > > > > If L1.1 is not supported, then ASPM_STATE_L1_1 will not be set in > > `aspm_capable` right? That will not allow L1.1 to be enabled. So, we > > should be fine. > > It seems like there are two questions here: > > 1) We currently enable L1.2 when the caller didn't request it. This > seems clearly wrong and we should fix it. If we can make a patch > that does just this part, that would be good. > Ack. Will do in the next revision. > 2) Should the PCI core enable L1 if the caller requests only L1.1 > (or L1.2)? This one isn't as clear to me, but there's only one > caller, and whatever we do won't make a difference to it, so it can > go either way. If we want to make a semantic change here, that's > OK, but I'd like to make that its own patch if possible. > Ack. Will create a new patch in the next revision. > > > > if (state & PCIE_LINK_STATE_L1_2) > > > > - link->aspm_default |= ASPM_STATE_L1_2; > > > > + link->aspm_default |= ASPM_STATE_L1_2 | ASPM_STATE_L1; > > > > if (state & PCIE_LINK_STATE_L1_1_PCIPM) > > > > - link->aspm_default |= ASPM_STATE_L1_1_PCIPM; > > > > + link->aspm_default |= ASPM_STATE_L1_1_PCIPM | ASPM_STATE_L1; > > > > if (state & PCIE_LINK_STATE_L1_2_PCIPM) > > > > - link->aspm_default |= ASPM_STATE_L1_2_PCIPM; > > > > + link->aspm_default |= ASPM_STATE_L1_2_PCIPM | ASPM_STATE_L1; > > > > pcie_config_aspm_link(link, policy_to_aspm_state(link)); > > > > > > > > link->clkpm_default = (state & PCIE_LINK_STATE_CLKPM) ? 1 : 0; > > > > -- > > > > 2.40.0.577.gac1e443424-goog > > > >
