Re: [PATCH v4 06/11] PCI/ASPM: Clear aspm_disable as part of __pci_enable_link_state()

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

On Tue, 8 Jul 2025, Manivannan Sadhasivam wrote:

> On Mon, Jun 09, 2025 at 04:21:27PM GMT, Krishna Chaitanya Chundru wrote:
> > ASPM states are not being enabled back with pci_enable_link_state() when
> > they are disabled by pci_disable_link_state(). This is because of the
> > aspm_disable flag is not getting cleared in pci_enable_link_state(), this
> > flag is being properly cleared when ASPM is controlled by sysfs.
> > 
> 
> A comment in pcie_config_aspm_link() says:
> 
>  /* Enable only the states that were not explicitly disabled */
> 
> But the function is called from both aspm_attr_store_common() and
> __pci_enable_link_state(). So I don't know if this is behavior is intentional
> or wrong. 

Hi,

I think it's intentional. Whether the behavior is useful is another good 
question but the current behavior aligns with the explanation in the 
comment.

My understanding of the situation is:

pci_disable_link_state() and pci_enable_link_state() are not symmetric 
despite the names, never have been (this is one of those many quirks ASPM 
driver has which should be eventually cleaned up, IMO).

It might be appropriate to rename pci_enable_link_state() to 
pci_set_default_link_state() to match the name to its functionality (and 
the function comment):

 * pci_enable_link_state - Clear and set the default device link state

Note: "the default ... link state".


I've already raised this concern earlier! As you see, my comment are 
not getting addressed. I'd like to see the author does one of these:

1) Renames pci_enable_link_state() to pci_set_default_link_state()

1b) If pci_enable_link_state() is still needed after that, a new function
is added to symmetrically pair with pci_disable_link_state().

or alternatively,

2) Changelog justifies very clearly why this change is okay with the 
existing callers. (And obviously the function comment should be altered to 
match the functionality in that case too).

If approach 2 is chosen, it should be very carefully reviewed when it 
comes to the callers.


> > Clear the aspm_disable flag with the requested ASPM states requested by
> > pci_enable_link_state().
> > 
> > Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
> 
> Fixes tag?
> 
> - Mani
> 
> > ---
> >  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;
> > 
> > -- 
> > 2.34.1
> > 
> 
> 

-- 
 i.