Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 4 Jul 2022 10:24:15 +0530
From:   Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
To:     Krishna chaitanya chundru <quic_krichai@quicinc.com>
Cc:     helgaas@kernel.org, linux-pci@vger.kernel.org,
        linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org,
        quic_vbadigan@quicinc.com, quic_hemantk@quicinc.com,
        quic_nitegupt@quicinc.com, quic_skananth@quicinc.com,
        quic_ramkri@quicinc.com, swboyd@chromium.org,
        dmitry.baryshkov@linaro.org, Jingoo Han <jingoohan1@gmail.com>,
        Gustavo Pimentel <gustavo.pimentel@synopsys.com>,
        Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>,
        Rob Herring <robh@kernel.org>,
        Krzysztof =?utf-8?Q?Wilczy=C5=84ski?= <kw@linux.com>,
        Bjorn Helgaas <bhelgaas@google.com>,
        Andy Gross <agross@kernel.org>,
        Bjorn Andersson <bjorn.andersson@linaro.org>,
        Stanimir Varbanov <svarbanov@mm-sol.com>
Subject: Re: [PATCH v2 2/2] PCI: qcom: Restrict pci transactions after pci
 suspend
Message-ID: <20220704045415.GC6560@thinkpad>
References: <1656055682-18817-1-git-send-email-quic_krichai@quicinc.com>
 <1656495214-4028-1-git-send-email-quic_krichai@quicinc.com>
 <1656495214-4028-3-git-send-email-quic_krichai@quicinc.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <1656495214-4028-3-git-send-email-quic_krichai@quicinc.com>
Precedence: bulk

On Wed, Jun 29, 2022 at 03:03:34PM +0530, Krishna chaitanya chundru wrote:
> If the endpoint device state is D0 and irq's are not freed, then
> kernel try to mask interrupts by writing in to the vector
> table (for MSIX interrupts) and config space (for MSI's).
> 
> These transactions are initiated after clocks are getting disabled
> as part of PM suspend call. Due to it, these transactions are
> resulting in un-clocked access and eventual to crashes.
> 
> So added a logic in qcom driver to restrict the unclocked access.
> And updated the logic to check the link state before masking
> or unmasking the interrupts.
> 

No other PCI driver is doing the DBI access restriction. So this makes me feel
that the fix is somewhere else.

I'll dig into it and come back.

Thanks,
Mani

> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
> ---
>  drivers/pci/controller/dwc/pcie-designware-host.c | 14 +++++++--
>  drivers/pci/controller/dwc/pcie-qcom.c            | 35 +++++++++++++++++++++--
>  2 files changed, 45 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> index 2fa86f3..2a46b40 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> @@ -29,13 +29,23 @@ static void dw_msi_ack_irq(struct irq_data *d)
>  
>  static void dw_msi_mask_irq(struct irq_data *d)
>  {
> -	pci_msi_mask_irq(d);
> +	struct pcie_port *pp = irq_data_get_irq_chip_data(d->parent_data);
> +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> +
> +	if (dw_pcie_link_up(pci))
> +		pci_msi_mask_irq(d);
> +
>  	irq_chip_mask_parent(d);
>  }
>  
>  static void dw_msi_unmask_irq(struct irq_data *d)
>  {
> -	pci_msi_unmask_irq(d);
> +	struct pcie_port *pp = irq_data_get_irq_chip_data(d->parent_data);
> +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> +
> +	if (dw_pcie_link_up(pci))
> +		pci_msi_unmask_irq(d);
> +
>  	irq_chip_unmask_parent(d);
>  }
>  
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 8e9ef37..227bc24a 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -1331,12 +1331,41 @@ static int qcom_pcie_disable_clks_2_7_0(struct qcom_pcie *pcie)
>  	return 0;
>  }
>  
> +static u32 qcom_pcie_read_dbi(struct dw_pcie *pci, void __iomem *base,
> +				u32 reg, size_t size)
> +{
> +	struct qcom_pcie *pcie = to_qcom_pcie(pci);
> +	u32 val;
> +
> +	if (pcie->is_suspended)
> +		return PCIBIOS_BAD_REGISTER_NUMBER;
> +
> +	dw_pcie_read(base + reg, size, &val);
> +	return val;
> +}
> +
> +static void qcom_pcie_write_dbi(struct dw_pcie *pci, void __iomem *base,
> +					u32 reg, size_t size, u32 val)
> +{
> +	struct qcom_pcie *pcie = to_qcom_pcie(pci);
> +
> +	if (pcie->is_suspended)
> +		return;
> +
> +	dw_pcie_write(base + reg, size, val);
> +}
>  
>  static int qcom_pcie_link_up(struct dw_pcie *pci)
>  {
> -	u16 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> -	u16 val = readw(pci->dbi_base + offset + PCI_EXP_LNKSTA);
> +	struct qcom_pcie *pcie = to_qcom_pcie(pci);
> +	u16 offset;
> +	u16 val;
> +
> +	if (pcie->is_suspended)
> +		return false;
>  
> +	offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> +	val = readw(pci->dbi_base + offset + PCI_EXP_LNKSTA);
>  	return !!(val & PCI_EXP_LNKSTA_DLLLA);
>  }
>  
> @@ -1580,6 +1609,8 @@ static const struct qcom_pcie_cfg sc7280_cfg = {
>  static const struct dw_pcie_ops dw_pcie_ops = {
>  	.link_up = qcom_pcie_link_up,
>  	.start_link = qcom_pcie_start_link,
> +	.read_dbi = qcom_pcie_read_dbi,
> +	.write_dbi = qcom_pcie_write_dbi,
>  };
>  
>  static int qcom_pcie_probe(struct platform_device *pdev)
> -- 
> 2.7.4
> 

-- 
மணிவண்ணன் சதாசிவம்