linux-kernel - Re: [PATCH v11 04/11] PCI: brcmstb: Add suspend and resume pm

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]

Date:   Thu, 10 Sep 2020 11:47:03 -0700
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Rob Herring <robh@...nel.org>,
        Jim Quinlan <james.quinlan@...adcom.com>
Cc:     linux-pci@...r.kernel.org,
        Nicolas Saenz Julienne <nsaenzjulienne@...e.de>,
        Christoph Hellwig <hch@....de>,
        Robin Murphy <robin.murphy@....com>,
        bcm-kernel-feedback-list@...adcom.com,
        Lorenzo Pieralisi <lorenzo.pieralisi@....com>,
        Bjorn Helgaas <bhelgaas@...gle.com>,
        "moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE" 
        <linux-rpi-kernel@...ts.infradead.org>,
        "moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE" 
        <linux-arm-kernel@...ts.infradead.org>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v11 04/11] PCI: brcmstb: Add suspend and resume pm_ops



On 9/10/2020 8:56 AM, Rob Herring wrote:
> On Mon, Aug 24, 2020 at 03:30:17PM -0400, Jim Quinlan wrote:
>> From: Jim Quinlan <jquinlan@...adcom.com>
>>
>> Broadcom Set-top (BrcmSTB) boards typically support S2, S3, and S5 suspend
>> and resume.  Now the PCIe driver may do so as well.
>>
>> Signed-off-by: Jim Quinlan <jquinlan@...adcom.com>
>> Acked-by: Florian Fainelli <f.fainelli@...il.com>
>> ---
>>   drivers/pci/controller/pcie-brcmstb.c | 47 +++++++++++++++++++++++++++
>>   1 file changed, 47 insertions(+)
>>
>> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
>> index c2b3d2946a36..3d588ab7a6dd 100644
>> --- a/drivers/pci/controller/pcie-brcmstb.c
>> +++ b/drivers/pci/controller/pcie-brcmstb.c
>> @@ -978,6 +978,47 @@ static void brcm_pcie_turn_off(struct brcm_pcie *pcie)
>>   	brcm_pcie_bridge_sw_init_set(pcie, 1);
>>   }
>>   
>> +static int brcm_pcie_suspend(struct device *dev)
>> +{
>> +	struct brcm_pcie *pcie = dev_get_drvdata(dev);
>> +
>> +	brcm_pcie_turn_off(pcie);
>> +	clk_disable_unprepare(pcie->clk);
>> +
>> +	return 0;
>> +}
>> +
>> +static int brcm_pcie_resume(struct device *dev)
>> +{
>> +	struct brcm_pcie *pcie = dev_get_drvdata(dev);
>> +	void __iomem *base;
>> +	u32 tmp;
>> +	int ret;
>> +
>> +	base = pcie->base;
>> +	clk_prepare_enable(pcie->clk);
>> +
>> +	/* Take bridge out of reset so we can access the SERDES reg */
>> +	brcm_pcie_bridge_sw_init_set(pcie, 0);
>> +
>> +	/* SERDES_IDDQ = 0 */
>> +	tmp = readl(base + PCIE_MISC_HARD_PCIE_HARD_DEBUG);
>> +	u32p_replace_bits(&tmp, 0, PCIE_MISC_HARD_PCIE_HARD_DEBUG_SERDES_IDDQ_MASK);
>> +	writel(tmp, base + PCIE_MISC_HARD_PCIE_HARD_DEBUG);
>> +
>> +	/* wait for serdes to be stable */
>> +	udelay(100);
> 
> Really needs to be a spinloop?
> 
>> +
>> +	ret = brcm_pcie_setup(pcie);
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (pcie->msi)
>> +		brcm_msi_set_regs(pcie->msi);
>> +
>> +	return 0;
>> +}
>> +
>>   static void __brcm_pcie_remove(struct brcm_pcie *pcie)
>>   {
>>   	brcm_msi_remove(pcie);
>> @@ -1087,12 +1128,18 @@ static int brcm_pcie_probe(struct platform_device *pdev)
>>   
>>   MODULE_DEVICE_TABLE(of, brcm_pcie_match);
>>   
>> +static const struct dev_pm_ops brcm_pcie_pm_ops = {
>> +	.suspend_noirq = brcm_pcie_suspend,
>> +	.resume_noirq = brcm_pcie_resume,
> 
> Why do you need interrupts disabled? There's 39 cases of .suspend_noirq
> and 1352 of .suspend in the tree.
> 
> Is doing a clk unprepare even safe in .suspend_noirq? IIRC,
> prepare/unprepare can sleep.

Yes, it is safe, provided that your clock provider (clk-scmi.c in our 
case) supports it, too. In our case the underlying mailbox driver has 
its interrupts flagged with IRQF_NOSUSPEND such that they can still be 
processed at _noirq time.

I think the rationale was to ensure that this would be done much later 
after other subsystem have been made quiescent, but given the Linux 
device driver model, the PCI bridge should be suspended after all 
pci_device child device, so it should be safe not to use _noirq.
-- 
Florian