Re: [PATCH v2 2/8] PCI/ASPM: Fix the behavior of pci_enable_link_state*() APIs

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

[-- Attachment #1: Type: text/plain, Size: 2997 bytes --]

On Sat, 6 Sep 2025, Manivannan Sadhasivam wrote:

> On Tue, Aug 26, 2025 at 03:55:42PM GMT, Ilpo Järvinen wrote:
> > +David
> > 
> > On Mon, 25 Aug 2025, Manivannan Sadhasivam via B4 Relay wrote:
> > 
> > > From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> > > 
> > > pci_enable_link_state() and pci_enable_link_state_locked() APIs are
> > > supposed to be symmectric with pci_disable_link_state() and
> > > pci_disable_link_state_locked() APIs.
> > > 
> > > But unfortunately, they are not symmetric. This behavior was mentioned in
> > > the kernel-doc of these APIs:
> > > 
> > > " Clear and set the default device link state..."
> > > 
> > > and
> > > 
> > > "Also note that this does not enable states disabled by
> > > pci_disable_link_state()"
> > > 
> > > These APIs won't enable all the states specified by the 'state' parameter,
> > > but only enable the ones not previously disabled by the
> > > pci_disable_link_state*() APIs. But this behavior doesn't align with the
> > > naming of these APIs, as they give the impression that these APIs will
> > > enable all the specified states.
> > > 
> > > To resolve this ambiguity, allow these APIs to enable the specified states,
> > > regardeless of whether they were previously disabled or not. This is
> > > accomplished by clearing the previously disabled states from the
> > > 'link::aspm_disable' parameter in __pci_enable_link_state() helper. Also,
> > > reword the kernel-doc to reflect this behavior.
> > > 
> > > The current callers of pci_enable_link_state_locked() APIs (vmd and
> > > pcie-qcom) did not disable the ASPM states before calling this API. So it
> > > is evident that they do not depend on the previous behavior of this API and
> > > intend to enable all the specified states.
> > 
> > While it might be "safe" in the sense that ->aspm_disable is not set by 
> > anything, I'm still not sure if overloading this function for two 
> > different use cases is a good idea.
> > 
> 
> Why? I thought your concern was with the callers of this API. Since that is
> taken care, do you have any other concerns?

I don't think it really matters anymore as it looks the vmd one is going 
to be removed by the David's patch and the qcom one is removed by your patch
so no users remain.

> > I'd like to hear David's opinion on this as he grasps the ->aspm_default 
> > vs ->aspm_disable thing much better than I do.
> > 
> > > And the other API, pci_enable_link_state() doesn't have a caller for now,
> > > but will be used by the 'atheros' WLAN drivers in the subsequent commits.
> > > 
> > > Suggested-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > 
> > This tag sound like I'm endorsing this approach which is not the case. I'd 
> > prefer separate functions for each use case, setting aspm_default and 
> > another for the enable state.
> > 
> 
> Sorry, I misunderstood then. I'll drop this tag.
> 
> - Mani
> 
> 

-- 
 i.