linux-kernel - Re: [EXT] Re: [PATCH v2 1/1] PCI: layerscape: Add power management support

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20230322211929.GA2493702@bhelgaas>
Date:   Wed, 22 Mar 2023 16:19:29 -0500
From:   Bjorn Helgaas <helgaas@...nel.org>
To:     Frank Li <frank.li@....com>
Cc:     "bhelgaas@...gle.com" <bhelgaas@...gle.com>,
        Leo Li <leoyang.li@....com>, dl-linux-imx <linux-imx@....com>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "gustavo.pimentel@...opsys.com" <gustavo.pimentel@...opsys.com>,
        "kw@...ux.com" <kw@...ux.com>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
        "lorenzo.pieralisi@....com" <lorenzo.pieralisi@....com>,
        "M.H. Lian" <minghuan.lian@....com>,
        Mingkai Hu <mingkai.hu@....com>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        Roy Zang <roy.zang@....com>,
        "shawnguo@...nel.org" <shawnguo@...nel.org>,
        "Z.Q. Hou" <zhiqiang.hou@....com>
Subject: Re: [EXT] Re: [PATCH v2 1/1] PCI: layerscape: Add power management
 support

On Tue, Mar 21, 2023 at 09:39:44PM +0000, Frank Li wrote:
> > -----Original Message-----
> > From: Bjorn Helgaas <helgaas@...nel.org>
> > Sent: Tuesday, March 21, 2023 3:59 PM
> > To: Frank Li <frank.li@....com>
> > Cc: bhelgaas@...gle.com; Leo Li <leoyang.li@....com>; dl-linux-imx <linux-
> > imx@....com>; devicetree@...r.kernel.org;
> > gustavo.pimentel@...opsys.com; kw@...ux.com; linux-arm-
> > kernel@...ts.infradead.org; linux-kernel@...r.kernel.org; linux-
> > pci@...r.kernel.org; lorenzo.pieralisi@....com; M.H. Lian
> > <minghuan.lian@....com>; Mingkai Hu <mingkai.hu@....com>;
> > robh+dt@...nel.org; Roy Zang <roy.zang@....com>;
> > shawnguo@...nel.org; Z.Q. Hou <zhiqiang.hou@....com>
> > Subject: [EXT] Re: [PATCH v2 1/1] PCI: layerscape: Add power management
> > support
> > 
> > Caution: EXT Email
> > 
> > On Tue, Mar 21, 2023 at 12:02:20PM -0400, Frank Li wrote:
> > > From: Hou Zhiqiang <Zhiqiang.Hou@....com>
> > >
> > > Add PME_Turn_Off/PME_TO_Ack handshake sequence to PCIe devices,
> > such as
> > > NVME or wifi module, and finally put the PCIe controller into D3 state
> > > after the L2/L3 ready state transition process completion.
> > >
> > > However, it's important to note that not all devices may be able to
> > > tolerate the PME_Turn_Off command. In general, fixed PCIe devices
> > > connected to Layerscape, such as NXP wifi devices, are able to handle
> > > this command.
> > 
> > I know this paragraph is here because I asked whether all PCIe devices
> > could tolerate PME_Turn_Off.  I don't know much about that level of
> > the protocol, but it does look to me like PME_Turn_Off is required,
> > e.g., PCIe r6.0, sec 5.3.3.2.1, 5.3.3.4.
> > 
> > So I'm not sure this paragraph adds anything useful.  If the spec
> > requires it, this paragraph is like saying "it's important to note
> > that some PCIe devices may not follow the spec," which is pointless.
> > 
> > This functionality results in any downstream devices being put in
> > D3cold, right?  I think that *would* be worth mentioning.  There are a
> > few cases where we try to avoid putting devices in D3cold, e.g.,
> > no_d3cold, and I suspect this functionality would put them in D3cold
> > regardless of no_d3cold.  Those are corner cases that you would
> > probably never see on your platform, so a brief mention here is
> > probably enough.
> > 
> > > +static void ls_pcie_set_dstate(struct ls_pcie *pcie, u32 dstate)
> > > +{
> > > +     struct dw_pcie *pci = pcie->pci;
> > > +     u8 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_PM);
> > > +     u32 val;
> > > +
> > > +     val = dw_pcie_readw_dbi(pci, offset + PCI_PM_CTRL);
> > > +     val &= ~PCI_PM_CTRL_STATE_MASK;
> > > +     val |= dstate;
> > > +     dw_pcie_writew_dbi(pci, offset + PCI_PM_CTRL, val);
> > 
> > Is this a power management register for the *Root Port*, i.e., as
> > defined by PCIe r6.0 sec 7.5.2?
> > 
> > Or is it a similar register for the *Root Complex* as a whole that is
> > defined by a Layerscape or DesignWare spec and coincidentally uses the
> > same Capability ID and control register layout as the PCIe one?
> 
> I think it is root port. Does linux framework can do that for it
> automatically?  Or need call pci_set_power_state here instead of
> write register directly?

Well, maybe, the linux framework might already put Root Ports in D3 if
the right conditions are satisfied.  I don't understand all of them,
but you can start at pci_dev_check_d3cold() and look at its users and
instrument things to see what actually happens.

But it might be a problem if that has to be synchronized and done in
the right order with respect to the RC things you do here, because I
don't think there's a hook for the PCI core to call your driver to do
the RC stuff.

Bjorn