linux-kernel - RE: [EXT] Re: [PATCH 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: <AM6PR04MB4838980A1243C5ECDDDF454F88829@AM6PR04MB4838.eurprd04.prod.outlook.com>
Date:   Sat, 18 Mar 2023 01:15:18 +0000
From:   Frank Li <frank.li@....com>
To:     Bjorn Helgaas <helgaas@...nel.org>
CC:     "lorenzo.pieralisi@....com" <lorenzo.pieralisi@....com>,
        "kw@...ux.com" <kw@...ux.com>, "Z.Q. Hou" <zhiqiang.hou@....com>,
        "bhelgaas@...gle.com" <bhelgaas@...gle.com>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "gustavo.pimentel@...opsys.com" <gustavo.pimentel@...opsys.com>,
        Leo Li <leoyang.li@....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>,
        "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>
Subject: RE: [EXT] Re: [PATCH 1/1] PCI: layerscape: Add power management
 support

> 
> Caution: EXT Email
> 
> On Fri, Mar 17, 2023 at 04:05:28PM -0400, Frank Li wrote:
> > From: Hou Zhiqiang <Zhiqiang.Hou@....com>
> >
> > Add PME_Turn_Off/PME_TO_Ack handshake sequence, and finally
> > put the PCIe controller into D3 state after the L2/L3 ready
> > state transition process completion.
> 
> Can you please include a sentence or two about what this means for
> devices below the PCIe controller?  Is this guaranteed to be safe for
> them, i.e., can all PCIe devices tolerate PME_Turn_Off, etc., and
> resume correctly afterwards?

We can't guarantee all PCIe devices tolerate PME_Turn_off etc. We just
follow PCI Spec and test some available devices to make sure our implement
 is correct.

Anyways, it was still better than nothing.  

> 
> I suspect other drivers will copy this sort of pattern if it is safe
> and useful.
> 
> >  struct ls_pcie {
> >       struct dw_pcie *pci;
> > +     const struct ls_pcie_drvdata *drvdata;
> > +     void __iomem *pf_base;
> > +     void __iomem *lut_base;
> > +     bool big_endian;
> > +     bool ep_presence;
> 
> This means "any downstream device present", right?  Could be an
> Endpoint or could be a Switch Upstream Port?  I guess it's basically a
> cache of dw_pcie_link_up() at ls_pcie_host_init()-time.

Should be an Endpoint.  Most of our user case is that connect pcie
wifi module.   

> 
> > +     bool pm_support;
> > +     struct regmap *scfg;
> > +     int index;
> >  };
> 
> > +static void ls1021a_pcie_send_turnoff_msg(struct ls_pcie *pcie)
> > +{
> > +     u32 val;
> > +
> > +     if (!pcie->scfg) {
> > +             dev_dbg(pcie->pci->dev, "SYSCFG is NULL\n");
> > +             return;
> > +     }
> > +
> > +     /* Send Turn_off message */
> > +     regmap_read(pcie->scfg, SCFG_PEXPMWRCR(pcie->index), &val);
> > +     val |= PMXMTTURNOFF;
> > +     regmap_write(pcie->scfg, SCFG_PEXPMWRCR(pcie->index), val);
> > +
> > +     /*
> > +      * Components with an upstream port must respond to
> > +      * PME_Turn_Off with PME_TO_Ack but we can't check.
> > +      *
> > +      * The standard recommends a 1-10ms timeout after which to
> > +      * proceed anyway as if acks were received.
> 
> Spec citation please.
> 
> > +      */
> > +     mdelay(10);
> > +
> > +     /* Clear Turn_off message */
> > +     regmap_read(pcie->scfg, SCFG_PEXPMWRCR(pcie->index), &val);
> > +     val &= ~PMXMTTURNOFF;
> > +     regmap_write(pcie->scfg, SCFG_PEXPMWRCR(pcie->index), val);
> > +}
> 
> > +static bool ls_pcie_pm_check(struct ls_pcie *pcie)
> 
> This is used as a boolean ("if (!ls_pcie_pm_check())") so it needs a
> better name.  "Check" doesn't give any hint about what a true or false
> return value means.  Something like "pm_supported" *would* give a
> hint because "if (!ls_pcie_pm_supported())" is a sensible question to
> ask.
> 
> > +{
> > +     if (!pcie->ep_presence) {
> > +             dev_dbg(pcie->pci->dev, "Endpoint isn't present\n");
> > +             return false;
> > +     }
> > +
> > +     if (!pcie->pm_support)
> > +             return false;
> 
> Why test the negative ("!pcie->pm_support") and then return false?
> How about:
> 
>   if (pcie->pm_support)
>     return true;
> 
>   return false;
> 
> or even better, just:
> 
>   return pcie->pm_support;
> 
> > +     return true;
> > +}