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@xxxxxxxxxxxxxxxx> > > > > 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'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@xxxxxxxxxxxxxxx> > > 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 -- மணிவண்ணன் சதாசிவம்
