| Message ID | 20230411111034.1473044-3-ajayagarwal@google.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | ASPM: aspm_disable/default/support state handling fixes | expand |
[+cc Nirmal, Jonathan, since vmd is the only caller of pci_enable_link_state()] 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? > Signed-off-by: Ajay Agarwal <ajayagarwal@google.com> > --- > drivers/pci/pcie/aspm.c | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c > index 5765b226102a..7c9935f331f1 100644 > --- a/drivers/pci/pcie/aspm.c > +++ b/drivers/pci/pcie/aspm.c > @@ -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. 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 (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 >
On Mon, May 01, 2023 at 12:44:39PM -0500, Bjorn Helgaas wrote: > [+cc Nirmal, Jonathan, since vmd is the only caller of > pci_enable_link_state()] > > 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. > > Signed-off-by: Ajay Agarwal <ajayagarwal@google.com> > > --- > > drivers/pci/pcie/aspm.c | 12 ++++++------ > > 1 file changed, 6 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c > > index 5765b226102a..7c9935f331f1 100644 > > --- a/drivers/pci/pcie/aspm.c > > +++ b/drivers/pci/pcie/aspm.c > > @@ -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. 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. > > 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 > >
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 > > >
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 > > > >
diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c index 5765b226102a..7c9935f331f1 100644 --- a/drivers/pci/pcie/aspm.c +++ b/drivers/pci/pcie/aspm.c @@ -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; 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;
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. Signed-off-by: Ajay Agarwal <ajayagarwal@google.com> --- drivers/pci/pcie/aspm.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)