Message-ID: <c7d4288d-4d0e-a3c2-83d2-c3f1b282d4ac@linux.intel.com>
Date: Fri, 11 Jul 2025 12:21:36 +0300 (EEST)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: Manivannan Sadhasivam <mani@...nel.org>
cc: Krishna Chaitanya Chundru <krishna.chundru@....qualcomm.com>, 
    Bjorn Helgaas <bhelgaas@...gle.com>, Jingoo Han <jingoohan1@...il.com>, 
    Lorenzo Pieralisi <lpieralisi@...nel.org>, Rob Herring <robh@...nel.org>, 
    Jeff Johnson <jjohnson@...nel.org>, Bartosz Golaszewski <brgl@...ev.pl>, 
    Krzysztof Wilczyński <kwilczynski@...nel.org>, 
    linux-pci@...r.kernel.org, LKML <linux-kernel@...r.kernel.org>, 
    linux-arm-msm@...r.kernel.org, mhi@...ts.linux.dev, 
    linux-wireless@...r.kernel.org, ath11k@...ts.infradead.org, 
    qiang.yu@....qualcomm.com, quic_vbadigan@...cinc.com, 
    quic_vpernami@...cinc.com, quic_mrana@...cinc.com, 
    Jeff Johnson <jeff.johnson@....qualcomm.com>
Subject: Re: [PATCH v4 06/11] PCI/ASPM: Clear aspm_disable as part of
 __pci_enable_link_state()

On Fri, 11 Jul 2025, Manivannan Sadhasivam wrote:

> On Wed, Jul 09, 2025 at 06:01:22PM GMT, Krishna Chaitanya Chundru wrote:
> > 
> > 
> > On 7/9/2025 2:40 PM, Ilpo Järvinen wrote:
> > > 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:
> >
> > I replied to your comment on v3 patch[1], and I feel instead of having
> > new function() we can use same API to our purpose.

It's not about what "feels" something. One should clearly write down why
such conversion is correct/acceptable when it comes to existing callers 
if changing an existing API. The note should be such that it remains a 
permanent record for future (in the changelog).

I don't have answer to what are the expectations or intent of the existing 
callers. Convincing a patch is fine is responsibility of the one who is 
submitting the patch, not reviewer's.

Unfortunately, it is usually quite hard to figure out for existing drivers 
we're not familiar with. I'm not saying your "feel" is necessarily wrong, 
but the existing callers need to be properly investigated if you choose 
that path, not just handwaved over. It likely boils down if the 
->aspm_default and controlling it are useful features to have in the ASPM 
driver as your patch would take away that ability.

> You replied to Ilpo, but never got an agreement. Please try to close the
> discussions before posting next rev. If reviewers forgot to reply to your query,
> feel free to ping them in the same thread itself.
>
> > > 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.
> > > 
> > I am in favor of approach 2 which you suggested, but lets wait for other
> > reviewers feedback on this. Based up on the response i will make
> > necessary changes in v5.
> > 
> 
> I would go for (1). It is always going to be a problem to change a legacy API
> like this. We might end up causing regressions. So it is safe to rename to
> reflect the purpose and try to come up with a new API that does what you want.
> If callers want to migrate to the new API, they can also do it in the future.

That's my recommendation as well.

-- 
 i.

Powered by blists - more mailing lists