| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20231011050058.GC3508@thinkpad>
Date: Wed, 11 Oct 2023 10:30:58 +0530
From: Manivannan Sadhasivam <mani@...nel.org>
To: Bjorn Helgaas <helgaas@...nel.org>
Cc: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>,
lpieralisi@...nel.org, kw@...ux.com, bhelgaas@...gle.com,
robh@...nel.org, gustavo.pimentel@...opsys.com,
jingoohan1@...il.com, andersson@...nel.org,
konrad.dybcio@...aro.org, linux-pci@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org
Subject: Re: [PATCH v2 2/2] PCI: qcom: Enable ASPM for platforms supporting
1.9.0 ops
On Tue, Oct 10, 2023 at 11:29:45AM -0500, Bjorn Helgaas wrote:
> On Tue, Oct 10, 2023 at 09:29:14PM +0530, Manivannan Sadhasivam wrote:
> > ASPM is supported by Qcom host controllers/bridges on most of the recent
> > platforms and so the devices tested so far. But for enabling ASPM by
> > default (without Kconfig/cmdline/sysfs), BIOS has to enable ASPM on both
> > host bridge and downstream devices during boot. Unfortunately, none of the
> > BIOS available on Qcom platforms enables ASPM.
>
> I think this covers over a PCI core defect. If the devices advertise
> ASPM support, which both the qcom host controller and the endpoint
> devices do, the PCI core should be able to enable it without being
> prodded as this patch does.
>
Right.
> We had a long conversation about this at [1], but never came to a good
> resolution. Since we don't know how to fix the PCI core issue, I
> guess we have no choice but to do things like this patch, at least for
> now.
>
> If/when we ever *do* fix the PCI core issue, it would likely result in
> enabling ASPM (if advertised by both ends of the link) for *all* qcom
> controllers, not just the 1.9.0 ones.
>
That would be a welcome move IMO :)
> And I think that even today, users can enable ASPM on non-1.9.0
> controllers via sysfs. So if you are concerned about ASPM not being
> tested on those controllers, you may want to make them not advertise
> ASPM support.
>
That will completely take away the power saving if users want it at some time.
I'm planning to enable ASPM for other configs as well in the coming days once I
get hold of the test platforms. For platforms where ASPM is not really desired,
like IPQ SoCs used in routers, I will just disable it.
> Even with this patch, I guess hot-added devices don't get ASPM
> enabled? That's basically what [1] is about.
>
Yeah, that's a limitation like the BIOS enablement. Fortunately, most of the
platforms based on Qcom SoCs do not have hot pluggable PCIe slots like in PCs.
But we should fix it too.
- Mani
> Bjorn
>
> [1] https://lore.kernel.org/linux-pci/20230615070421.1704133-1-kai.heng.feng@canonical.com/
>
> > Due to this, the platforms
> > making use of Qcom SoCs draw high power during runtime.
> >
> > To fix this power issue, users/distros have to enable ASPM using configs
> > such as (Kconfig/cmdline/sysfs) or the BIOS has to start enabling ASPM.
> > The latter may happen in the future, but that won't address the issue on
> > current platforms. Also, asking users/distros to enable a feature to get
> > the power management right would provide an unpleasant out-of-the-box
> > experience.
> >
> > So the apt solution is to enable ASPM in the controller driver itself. And
> > this is being accomplished by calling pci_enable_link_state() in the newly
> > introduced host_post_init() callback for all the devices connected to the
> > bus. This function enables all supported link low power states for both
> > host bridge and the downstream devices.
> >
> > Due to limited testing, ASPM is only enabled for platforms making use of
> > ops_1_9_0 callbacks.
> >
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
> > ---
> > drivers/pci/controller/dwc/pcie-qcom.c | 28 ++++++++++++++++++++++++++
> > 1 file changed, 28 insertions(+)
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> > index 367acb419a2b..c324c3daaa5a 100644
> > --- a/drivers/pci/controller/dwc/pcie-qcom.c
> > +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> > @@ -222,6 +222,7 @@ struct qcom_pcie_ops {
> > int (*get_resources)(struct qcom_pcie *pcie);
> > int (*init)(struct qcom_pcie *pcie);
> > int (*post_init)(struct qcom_pcie *pcie);
> > + void (*host_post_init)(struct qcom_pcie *pcie);
> > void (*deinit)(struct qcom_pcie *pcie);
> > void (*ltssm_enable)(struct qcom_pcie *pcie);
> > int (*config_sid)(struct qcom_pcie *pcie);
> > @@ -967,6 +968,22 @@ static int qcom_pcie_post_init_2_7_0(struct qcom_pcie *pcie)
> > return 0;
> }
> >
> > +static int qcom_pcie_enable_aspm(struct pci_dev *pdev, void *userdata)
> > +{
> > + /* Downstream devices need to be in D0 state before enabling PCI PM substates */
> > + pci_set_power_state(pdev, PCI_D0);
> > + pci_enable_link_state(pdev, PCIE_LINK_STATE_ALL);
> > +
> > + return 0;
> > +}
> > +
> > +static void qcom_pcie_host_post_init_2_7_0(struct qcom_pcie *pcie)
> > +{
> > + struct dw_pcie_rp *pp = &pcie->pci->pp;
> > +
> > + pci_walk_bus(pp->bridge->bus, qcom_pcie_enable_aspm, NULL);
> > +}
> > +
> > static void qcom_pcie_deinit_2_7_0(struct qcom_pcie *pcie)
> > {
> > struct qcom_pcie_resources_2_7_0 *res = &pcie->res.v2_7_0;
> > @@ -1219,9 +1236,19 @@ static void qcom_pcie_host_deinit(struct dw_pcie_rp *pp)
> > pcie->cfg->ops->deinit(pcie);
> > }
> >
> > +static void qcom_pcie_host_post_init(struct dw_pcie_rp *pp)
> > +{
> > + struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > + struct qcom_pcie *pcie = to_qcom_pcie(pci);
> > +
> > + if (pcie->cfg->ops->host_post_init)
> > + pcie->cfg->ops->host_post_init(pcie);
> > +}
> > +
> > static const struct dw_pcie_host_ops qcom_pcie_dw_ops = {
> > .host_init = qcom_pcie_host_init,
> > .host_deinit = qcom_pcie_host_deinit,
> > + .host_post_init = qcom_pcie_host_post_init,
> > };
> >
> > /* Qcom IP rev.: 2.1.0 Synopsys IP rev.: 4.01a */
> > @@ -1283,6 +1310,7 @@ static const struct qcom_pcie_ops ops_1_9_0 = {
> > .get_resources = qcom_pcie_get_resources_2_7_0,
> > .init = qcom_pcie_init_2_7_0,
> > .post_init = qcom_pcie_post_init_2_7_0,
> > + .host_post_init = qcom_pcie_host_post_init_2_7_0,
> > .deinit = qcom_pcie_deinit_2_7_0,
> > .ltssm_enable = qcom_pcie_2_3_2_ltssm_enable,
> > .config_sid = qcom_pcie_config_sid_1_9_0,
> > --
> > 2.25.1
> >
--
மணிவண்ணன் சதாசிவம்
Powered by blists - more mailing lists