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