linux-kernel - 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20230317215650.GA1973940@bhelgaas>
Date:   Fri, 17 Mar 2023 16:56:50 -0500
From:   Bjorn Helgaas <helgaas@...nel.org>
To:     Frank Li <Frank.Li@....com>
Cc:     lorenzo.pieralisi@....com, kw@...ux.com, Zhiqiang.Hou@....com,
        bhelgaas@...gle.com, devicetree@...r.kernel.org,
        gustavo.pimentel@...opsys.com, leoyang.li@....com,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        linux-pci@...r.kernel.org, minghuan.Lian@....com,
        mingkai.hu@....com, robh+dt@...nel.org, roy.zang@....com,
        shawnguo@...nel.org
Subject: Re: [PATCH 1/1] PCI: layerscape: Add power management support

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?

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.

> +	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;
> +}