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

[Krishna Chaitanya Chundru]

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@xxxxxxxxxxxxxxxx>
> > ---
> >   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)?