| Message ID | 20250519-mhi_bw_up-v3-4-3acd4a17bbb5@oss.qualcomm.com |
|---|---|
| State | New |
| Headers | show |
| Series | bus: mhi: host: Add support for mhi bus bw | expand |
On Mon, 19 May 2025, Krishna Chaitanya Chundru wrote: > If a driver wants to enable ASPM back after disabling ASPM for some > usecase, it is not being enabled properly because of the aspm_disable > flag is not getting cleared. This flag is being properly when aspm > is controlled by sysfs. This sentence has broken grammar/is missing something? aspm -> ASPM > > Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com> > --- > drivers/pci/pcie/aspm.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c > index 94324fc0d3e650cd3ca2c0bb8c1895ca7e647b9d..0f858ef86111b43328bc7db01e6493ce67178458 100644 > --- a/drivers/pci/pcie/aspm.c > +++ b/drivers/pci/pcie/aspm.c > @@ -1453,6 +1453,7 @@ static int __pci_enable_link_state(struct pci_dev *pdev, int state, bool locked) > down_read(&pci_bus_sem); > mutex_lock(&aspm_lock); > link->aspm_default = pci_calc_aspm_enable_mask(state); > + link->aspm_disable &= ~state; > pcie_config_aspm_link(link, policy_to_aspm_state(link)); > > link->clkpm_default = (state & PCIE_LINK_STATE_CLKPM) ? 1 : 0; I disagree with this change. The problem currently with ASPM driver is that pci_disable_link_state() and pci_enable_link_state() are not symmetric pairs despite their misleading names. pci_enable_link_state() should be renamed to pci_set_default_link_state() and if the symmetric pair is needed for pci_disable_link_state(), it would have to be added separately. I've some (rotting) patches which try to do that, in case you want to try to solve this inconsistency in the ASPM driver (I can send them to you)?
On 5/19/2025 6:51 PM, Ilpo Järvinen wrote: > On Mon, 19 May 2025, Krishna Chaitanya Chundru wrote: > >> If a driver wants to enable ASPM back after disabling ASPM for some >> usecase, it is not being enabled properly because of the aspm_disable >> flag is not getting cleared. This flag is being properly when aspm >> is controlled by sysfs. > > This sentence has broken grammar/is missing something? > > aspm -> ASPM > >> >> Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com> >> --- >> drivers/pci/pcie/aspm.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c >> index 94324fc0d3e650cd3ca2c0bb8c1895ca7e647b9d..0f858ef86111b43328bc7db01e6493ce67178458 100644 >> --- a/drivers/pci/pcie/aspm.c >> +++ b/drivers/pci/pcie/aspm.c >> @@ -1453,6 +1453,7 @@ static int __pci_enable_link_state(struct pci_dev *pdev, int state, bool locked) >> down_read(&pci_bus_sem); >> mutex_lock(&aspm_lock); >> link->aspm_default = pci_calc_aspm_enable_mask(state); >> + link->aspm_disable &= ~state; >> pcie_config_aspm_link(link, policy_to_aspm_state(link)); >> >> link->clkpm_default = (state & PCIE_LINK_STATE_CLKPM) ? 1 : 0; > > I disagree with this change. > > The problem currently with ASPM driver is that pci_disable_link_state() > and pci_enable_link_state() are not symmetric pairs despite their > misleading names. pci_enable_link_state() should be renamed to > pci_set_default_link_state() and if the symmetric pair is needed for > pci_disable_link_state(), it would have to be added separately. > I just want to know what are disadvantages/side effects having this change here, we can use same API to be symmetric with pci_disable_link_state(). The drivers which are using this API has already option to specific the ASPM states which they want to enable and they don't need to call pci_disable_link_state() to specify the states they want to disable. - Krishna Chaitanya. > I've some (rotting) patches which try to do that, in case you want to try > to solve this inconsistency in the ASPM driver (I can send them to you)? >
diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c index 94324fc0d3e650cd3ca2c0bb8c1895ca7e647b9d..0f858ef86111b43328bc7db01e6493ce67178458 100644 --- a/drivers/pci/pcie/aspm.c +++ b/drivers/pci/pcie/aspm.c @@ -1453,6 +1453,7 @@ static int __pci_enable_link_state(struct pci_dev *pdev, int state, bool locked) down_read(&pci_bus_sem); mutex_lock(&aspm_lock); link->aspm_default = pci_calc_aspm_enable_mask(state); + link->aspm_disable &= ~state; pcie_config_aspm_link(link, policy_to_aspm_state(link)); link->clkpm_default = (state & PCIE_LINK_STATE_CLKPM) ? 1 : 0;
If a driver wants to enable ASPM back after disabling ASPM for some usecase, it is not being enabled properly because of the aspm_disable flag is not getting cleared. This flag is being properly when aspm is controlled by sysfs. Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com> --- drivers/pci/pcie/aspm.c | 1 + 1 file changed, 1 insertion(+)