[PATCH v5 0/3] PCI: Restrict pci transactions after pci suspend

[PATCH v5 0/3] PCI: Restrict pci transactions after pci suspend

[Krishna chaitanya chundru]

If the endpoint device state is D0 and irq's are not freed, then
kernel try to mask interrupts in system suspend path by writing in to
the vector table (for MSIX interrupts) and config space (for MSI's).

These transactions are initiated in the pm suspend after pcie clocks got
disabled as part of platform driver pm  suspend call. Due to it, these
transactions are resulting in un-clocked access and eventually to crashes.

So added a logic in qcom driver to restrict these unclocked access.
And updated the logic to check the link state before masking
or unmasking the interrupts.

And some devices are taking time to settle the link in L1ss, so added a
retry logic in the suspend ops.

Krishna chaitanya chundru (3):
  PCI: qcom: Add system PM support
  PCI: qcom: Restrict pci transactions after pci suspend
  PCI: qcom: Add retry logic for link to be stable in L1ss

 drivers/pci/controller/dwc/pcie-designware-host.c |  14 ++-
 drivers/pci/controller/dwc/pcie-qcom.c            | 117 +++++++++++++++++++++-
 2 files changed, 127 insertions(+), 4 deletions(-)

-- 
2.7.4

[PATCH v5 1/3] PCI: qcom: Add system PM support

[Krishna chaitanya chundru]

Add suspend and resume pm callbacks.

When system suspends, and if the link is in L1ss, disable the clocks
and power down the phy so that system enters into low power state to
save the maximum power. And when the system resumes, enable the clocks
back and power on phy if they are disabled in the suspend path.

we are doing this only when link is in l1ss but not in L2/L3 as
no where we are forcing link to L2/L3 by sending PME turn off.

is_suspended flag indicates if the clocks are disabled in the suspend
path or not. And this flag is being used to restrict the access to
config space, dbi etc when clock are turned-off.

Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
---
Changes since v4:
	- Rebasing the code and removed the supports_system_suspend flag
	- in the resume path as is_suspended will serve its purpose.
Changes since v3:
	- Powering down the phy in suspend and powering it on resume to
	  acheive maximum power savings.
Changes since v2:
	- Replaced the enable, disable clks ops with suspend and resume
	- Renamed support_pm_opsi flag  with supports_system_suspend.
Changes since v1:
	- Fixed compilation errors.
---
---
 drivers/pci/controller/dwc/pcie-qcom.c | 76 ++++++++++++++++++++++++++++++++++
 1 file changed, 76 insertions(+)

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index ccf6953..9dd50fc0 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -44,6 +44,9 @@
 #define PCIE20_PARF_PM_CTRL			0x20
 #define REQ_NOT_ENTR_L1				BIT(5)
 
+#define PCIE20_PARF_PM_STTS			0x24
+#define PCIE20_PARF_PM_STTS_LINKST_IN_L1SUB	BIT(8)
+
 #define PCIE20_PARF_PHY_CTRL			0x40
 #define PHY_CTRL_PHY_TX0_TERM_OFFSET_MASK	GENMASK(20, 16)
 #define PHY_CTRL_PHY_TX0_TERM_OFFSET(x)		((x) << 16)
@@ -211,6 +214,8 @@ struct qcom_pcie_ops {
 	void (*post_deinit)(struct qcom_pcie *pcie);
 	void (*ltssm_enable)(struct qcom_pcie *pcie);
 	int (*config_sid)(struct qcom_pcie *pcie);
+	int (*suspend)(struct qcom_pcie *pcie);
+	int (*resume)(struct qcom_pcie *pcie);
 };
 
 struct qcom_pcie_cfg {
@@ -219,6 +224,7 @@ struct qcom_pcie_cfg {
 	unsigned int has_ddrss_sf_tbu_clk:1;
 	unsigned int has_aggre0_clk:1;
 	unsigned int has_aggre1_clk:1;
+	unsigned int supports_system_suspend:1;
 };
 
 struct qcom_pcie {
@@ -229,6 +235,7 @@ struct qcom_pcie {
 	struct phy *phy;
 	struct gpio_desc *reset;
 	const struct qcom_pcie_cfg *cfg;
+	unsigned int is_suspended:1;
 };
 
 #define to_qcom_pcie(x)		dev_get_drvdata((x)->dev)
@@ -1297,6 +1304,29 @@ static void qcom_pcie_deinit_2_7_0(struct qcom_pcie *pcie)
 	regulator_bulk_disable(ARRAY_SIZE(res->supplies), res->supplies);
 }
 
+static int qcom_pcie_resume_2_7_0(struct qcom_pcie *pcie)
+{
+	struct qcom_pcie_resources_2_7_0 *res = &pcie->res.v2_7_0;
+	int ret;
+
+	ret = clk_bulk_prepare_enable(res->num_clks, res->clks);
+
+	phy_power_on(pcie->phy);
+
+	return ret;
+}
+
+static int qcom_pcie_suspend_2_7_0(struct qcom_pcie *pcie)
+{
+	struct qcom_pcie_resources_2_7_0 *res = &pcie->res.v2_7_0;
+
+	phy_power_off(pcie->phy);
+
+	clk_bulk_disable_unprepare(res->num_clks, res->clks);
+
+	return 0;
+}
+
 static int qcom_pcie_get_resources_2_9_0(struct qcom_pcie *pcie)
 {
 	struct qcom_pcie_resources_2_9_0 *res = &pcie->res.v2_9_0;
@@ -1590,6 +1620,8 @@ static const struct qcom_pcie_ops ops_1_9_0 = {
 	.deinit = qcom_pcie_deinit_2_7_0,
 	.ltssm_enable = qcom_pcie_2_3_2_ltssm_enable,
 	.config_sid = qcom_pcie_config_sid_sm8250,
+	.suspend = qcom_pcie_suspend_2_7_0,
+	.resume = qcom_pcie_resume_2_7_0,
 };
 
 /* Qcom IP rev.: 2.9.0  Synopsys IP rev.: 5.00a */
@@ -1655,6 +1687,7 @@ static const struct qcom_pcie_cfg sm8450_pcie1_cfg = {
 static const struct qcom_pcie_cfg sc7280_cfg = {
 	.ops = &ops_1_9_0,
 	.has_tbu_clk = true,
+	.supports_system_suspend = true,
 };
 
 static const struct qcom_pcie_cfg sc8180x_cfg = {
@@ -1760,6 +1793,48 @@ static int qcom_pcie_probe(struct platform_device *pdev)
 	return ret;
 }
 
+static int __maybe_unused qcom_pcie_pm_suspend(struct device *dev)
+{
+	struct qcom_pcie *pcie = dev_get_drvdata(dev);
+	u32 val;
+
+	if (!pcie->cfg->supports_system_suspend)
+		return 0;
+
+	/* if the link is not in l1ss don't turn off clocks */
+	val = readl(pcie->parf + PCIE20_PARF_PM_STTS);
+	if (!(val & PCIE20_PARF_PM_STTS_LINKST_IN_L1SUB)) {
+		dev_warn(dev, "Link is not in L1ss\n");
+		return 0;
+	}
+
+	if (pcie->cfg->ops->suspend)
+		pcie->cfg->ops->suspend(pcie);
+
+	pcie->is_suspended = true;
+
+	return 0;
+}
+
+static int __maybe_unused qcom_pcie_pm_resume(struct device *dev)
+{
+	struct qcom_pcie *pcie = dev_get_drvdata(dev);
+
+	if (!pcie->is_suspended)
+		return 0;
+
+	if (pcie->cfg->ops->resume)
+		pcie->cfg->ops->resume(pcie);
+
+	pcie->is_suspended = false;
+
+	return 0;
+}
+
+static const struct dev_pm_ops qcom_pcie_pm_ops = {
+	NOIRQ_SYSTEM_SLEEP_PM_OPS(qcom_pcie_pm_suspend, qcom_pcie_pm_resume)
+};
+
 static const struct of_device_id qcom_pcie_match[] = {
 	{ .compatible = "qcom,pcie-apq8084", .data = &apq8084_cfg },
 	{ .compatible = "qcom,pcie-ipq8064", .data = &ipq8064_cfg },
@@ -1796,6 +1871,7 @@ static struct platform_driver qcom_pcie_driver = {
 	.probe = qcom_pcie_probe,
 	.driver = {
 		.name = "qcom-pcie",
+		.pm = pm_sleep_ptr(&qcom_pcie_pm_ops),
 		.suppress_bind_attrs = true,
 		.of_match_table = qcom_pcie_match,
 	},
-- 
2.7.4

Re: [PATCH v5 1/3] PCI: qcom: Add system PM support

[Rob Herring]

On Wed, Aug 03, 2022 at 04:58:52PM +0530, Krishna chaitanya chundru wrote:
> Add suspend and resume pm callbacks.
> 
> When system suspends, and if the link is in L1ss, disable the clocks
> and power down the phy so that system enters into low power state to
> save the maximum power. And when the system resumes, enable the clocks
> back and power on phy if they are disabled in the suspend path.
> 
> we are doing this only when link is in l1ss but not in L2/L3 as
> no where we are forcing link to L2/L3 by sending PME turn off.
> 
> is_suspended flag indicates if the clocks are disabled in the suspend
> path or not. And this flag is being used to restrict the access to
> config space, dbi etc when clock are turned-off.
> 
> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
> ---
> Changes since v4:
> 	- Rebasing the code and removed the supports_system_suspend flag
> 	- in the resume path as is_suspended will serve its purpose.
> Changes since v3:
> 	- Powering down the phy in suspend and powering it on resume to
> 	  acheive maximum power savings.
> Changes since v2:
> 	- Replaced the enable, disable clks ops with suspend and resume
> 	- Renamed support_pm_opsi flag  with supports_system_suspend.
> Changes since v1:
> 	- Fixed compilation errors.
> ---
> ---
>  drivers/pci/controller/dwc/pcie-qcom.c | 76 ++++++++++++++++++++++++++++++++++
>  1 file changed, 76 insertions(+)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index ccf6953..9dd50fc0 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -44,6 +44,9 @@
>  #define PCIE20_PARF_PM_CTRL			0x20
>  #define REQ_NOT_ENTR_L1				BIT(5)
>  
> +#define PCIE20_PARF_PM_STTS			0x24
> +#define PCIE20_PARF_PM_STTS_LINKST_IN_L1SUB	BIT(8)
> +
>  #define PCIE20_PARF_PHY_CTRL			0x40
>  #define PHY_CTRL_PHY_TX0_TERM_OFFSET_MASK	GENMASK(20, 16)
>  #define PHY_CTRL_PHY_TX0_TERM_OFFSET(x)		((x) << 16)
> @@ -211,6 +214,8 @@ struct qcom_pcie_ops {
>  	void (*post_deinit)(struct qcom_pcie *pcie);
>  	void (*ltssm_enable)(struct qcom_pcie *pcie);
>  	int (*config_sid)(struct qcom_pcie *pcie);
> +	int (*suspend)(struct qcom_pcie *pcie);
> +	int (*resume)(struct qcom_pcie *pcie);

This is really the wrong direction. We don't need the DWC driver 
defining per platform ops and then a platform defining sub-platform ops. 
IOW, qcom_pcie_ops should go away.

But for now, you don't even need the ops as only 1 version is 
implemented. Do you really expect other versions to do something 
different than turn off clocks and phys?

>  };
>  
>  struct qcom_pcie_cfg {
> @@ -219,6 +224,7 @@ struct qcom_pcie_cfg {
>  	unsigned int has_ddrss_sf_tbu_clk:1;
>  	unsigned int has_aggre0_clk:1;
>  	unsigned int has_aggre1_clk:1;
> +	unsigned int supports_system_suspend:1;
>  };
>  
>  struct qcom_pcie {
> @@ -229,6 +235,7 @@ struct qcom_pcie {
>  	struct phy *phy;
>  	struct gpio_desc *reset;
>  	const struct qcom_pcie_cfg *cfg;
> +	unsigned int is_suspended:1;
>  };
>  
>  #define to_qcom_pcie(x)		dev_get_drvdata((x)->dev)
> @@ -1297,6 +1304,29 @@ static void qcom_pcie_deinit_2_7_0(struct qcom_pcie *pcie)
>  	regulator_bulk_disable(ARRAY_SIZE(res->supplies), res->supplies);
>  }
>  
> +static int qcom_pcie_resume_2_7_0(struct qcom_pcie *pcie)
> +{
> +	struct qcom_pcie_resources_2_7_0 *res = &pcie->res.v2_7_0;
> +	int ret;
> +
> +	ret = clk_bulk_prepare_enable(res->num_clks, res->clks);
> +
> +	phy_power_on(pcie->phy);
> +
> +	return ret;
> +}
> +
> +static int qcom_pcie_suspend_2_7_0(struct qcom_pcie *pcie)
> +{
> +	struct qcom_pcie_resources_2_7_0 *res = &pcie->res.v2_7_0;
> +
> +	phy_power_off(pcie->phy);
> +
> +	clk_bulk_disable_unprepare(res->num_clks, res->clks);
> +
> +	return 0;
> +}
> +
>  static int qcom_pcie_get_resources_2_9_0(struct qcom_pcie *pcie)
>  {
>  	struct qcom_pcie_resources_2_9_0 *res = &pcie->res.v2_9_0;
> @@ -1590,6 +1620,8 @@ static const struct qcom_pcie_ops ops_1_9_0 = {
>  	.deinit = qcom_pcie_deinit_2_7_0,
>  	.ltssm_enable = qcom_pcie_2_3_2_ltssm_enable,
>  	.config_sid = qcom_pcie_config_sid_sm8250,
> +	.suspend = qcom_pcie_suspend_2_7_0,
> +	.resume = qcom_pcie_resume_2_7_0,
>  };
>  
>  /* Qcom IP rev.: 2.9.0  Synopsys IP rev.: 5.00a */
> @@ -1655,6 +1687,7 @@ static const struct qcom_pcie_cfg sm8450_pcie1_cfg = {
>  static const struct qcom_pcie_cfg sc7280_cfg = {
>  	.ops = &ops_1_9_0,
>  	.has_tbu_clk = true,
> +	.supports_system_suspend = true,
>  };
>  
>  static const struct qcom_pcie_cfg sc8180x_cfg = {
> @@ -1760,6 +1793,48 @@ static int qcom_pcie_probe(struct platform_device *pdev)
>  	return ret;
>  }
>  
> +static int __maybe_unused qcom_pcie_pm_suspend(struct device *dev)
> +{
> +	struct qcom_pcie *pcie = dev_get_drvdata(dev);
> +	u32 val;
> +
> +	if (!pcie->cfg->supports_system_suspend)
> +		return 0;
> +
> +	/* if the link is not in l1ss don't turn off clocks */
> +	val = readl(pcie->parf + PCIE20_PARF_PM_STTS);
> +	if (!(val & PCIE20_PARF_PM_STTS_LINKST_IN_L1SUB)) {
> +		dev_warn(dev, "Link is not in L1ss\n");
> +		return 0;
> +	}

If anything, this looks like the version specific code.

> +
> +	if (pcie->cfg->ops->suspend)
> +		pcie->cfg->ops->suspend(pcie);
> +
> +	pcie->is_suspended = true;
> +
> +	return 0;
> +}
> +
> +static int __maybe_unused qcom_pcie_pm_resume(struct device *dev)
> +{
> +	struct qcom_pcie *pcie = dev_get_drvdata(dev);
> +
> +	if (!pcie->is_suspended)
> +		return 0;

Pretty sure the driver core should take care of not calling resume if 
not suspended.

> +
> +	if (pcie->cfg->ops->resume)
> +		pcie->cfg->ops->resume(pcie);
> +
> +	pcie->is_suspended = false;
> +
> +	return 0;
> +}
> +
> +static const struct dev_pm_ops qcom_pcie_pm_ops = {
> +	NOIRQ_SYSTEM_SLEEP_PM_OPS(qcom_pcie_pm_suspend, qcom_pcie_pm_resume)
> +};
> +
>  static const struct of_device_id qcom_pcie_match[] = {
>  	{ .compatible = "qcom,pcie-apq8084", .data = &apq8084_cfg },
>  	{ .compatible = "qcom,pcie-ipq8064", .data = &ipq8064_cfg },
> @@ -1796,6 +1871,7 @@ static struct platform_driver qcom_pcie_driver = {
>  	.probe = qcom_pcie_probe,
>  	.driver = {
>  		.name = "qcom-pcie",
> +		.pm = pm_sleep_ptr(&qcom_pcie_pm_ops),
>  		.suppress_bind_attrs = true,
>  		.of_match_table = qcom_pcie_match,
>  	},
> -- 
> 2.7.4
> 

Re: [PATCH v5 1/3] PCI: qcom: Add system PM support

[Krishna Chaitanya Chundru]

On 8/11/2022 3:20 AM, Rob Herring wrote:
> On Wed, Aug 03, 2022 at 04:58:52PM +0530, Krishna chaitanya chundru wrote:
>> Add suspend and resume pm callbacks.
>>
>> When system suspends, and if the link is in L1ss, disable the clocks
>> and power down the phy so that system enters into low power state to
>> save the maximum power. And when the system resumes, enable the clocks
>> back and power on phy if they are disabled in the suspend path.
>>
>> we are doing this only when link is in l1ss but not in L2/L3 as
>> no where we are forcing link to L2/L3 by sending PME turn off.
>>
>> is_suspended flag indicates if the clocks are disabled in the suspend
>> path or not. And this flag is being used to restrict the access to
>> config space, dbi etc when clock are turned-off.
>>
>> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
>> ---
>> Changes since v4:
>> 	- Rebasing the code and removed the supports_system_suspend flag
>> 	- in the resume path as is_suspended will serve its purpose.
>> Changes since v3:
>> 	- Powering down the phy in suspend and powering it on resume to
>> 	  acheive maximum power savings.
>> Changes since v2:
>> 	- Replaced the enable, disable clks ops with suspend and resume
>> 	- Renamed support_pm_opsi flag  with supports_system_suspend.
>> Changes since v1:
>> 	- Fixed compilation errors.
>> ---
>> ---
>>   drivers/pci/controller/dwc/pcie-qcom.c | 76 ++++++++++++++++++++++++++++++++++
>>   1 file changed, 76 insertions(+)
>>
>> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
>> index ccf6953..9dd50fc0 100644
>> --- a/drivers/pci/controller/dwc/pcie-qcom.c
>> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
>> @@ -44,6 +44,9 @@
>>   #define PCIE20_PARF_PM_CTRL			0x20
>>   #define REQ_NOT_ENTR_L1				BIT(5)
>>   
>> +#define PCIE20_PARF_PM_STTS			0x24
>> +#define PCIE20_PARF_PM_STTS_LINKST_IN_L1SUB	BIT(8)
>> +
>>   #define PCIE20_PARF_PHY_CTRL			0x40
>>   #define PHY_CTRL_PHY_TX0_TERM_OFFSET_MASK	GENMASK(20, 16)
>>   #define PHY_CTRL_PHY_TX0_TERM_OFFSET(x)		((x) << 16)
>> @@ -211,6 +214,8 @@ struct qcom_pcie_ops {
>>   	void (*post_deinit)(struct qcom_pcie *pcie);
>>   	void (*ltssm_enable)(struct qcom_pcie *pcie);
>>   	int (*config_sid)(struct qcom_pcie *pcie);
>> +	int (*suspend)(struct qcom_pcie *pcie);
>> +	int (*resume)(struct qcom_pcie *pcie);
> This is really the wrong direction. We don't need the DWC driver
> defining per platform ops and then a platform defining sub-platform ops.
> IOW, qcom_pcie_ops should go away.
>
> But for now, you don't even need the ops as only 1 version is
> implemented. Do you really expect other versions to do something
> different than turn off clocks and phys?
We will remove this in next patch.
>
>>   };
>>   
>>   struct qcom_pcie_cfg {
>> @@ -219,6 +224,7 @@ struct qcom_pcie_cfg {
>>   	unsigned int has_ddrss_sf_tbu_clk:1;
>>   	unsigned int has_aggre0_clk:1;
>>   	unsigned int has_aggre1_clk:1;
>> +	unsigned int supports_system_suspend:1;
>>   };
>>   
>>   struct qcom_pcie {
>> @@ -229,6 +235,7 @@ struct qcom_pcie {
>>   	struct phy *phy;
>>   	struct gpio_desc *reset;
>>   	const struct qcom_pcie_cfg *cfg;
>> +	unsigned int is_suspended:1;
>>   };
>>   
>>   #define to_qcom_pcie(x)		dev_get_drvdata((x)->dev)
>> @@ -1297,6 +1304,29 @@ static void qcom_pcie_deinit_2_7_0(struct qcom_pcie *pcie)
>>   	regulator_bulk_disable(ARRAY_SIZE(res->supplies), res->supplies);
>>   }
>>   
>> +static int qcom_pcie_resume_2_7_0(struct qcom_pcie *pcie)
>> +{
>> +	struct qcom_pcie_resources_2_7_0 *res = &pcie->res.v2_7_0;
>> +	int ret;
>> +
>> +	ret = clk_bulk_prepare_enable(res->num_clks, res->clks);
>> +
>> +	phy_power_on(pcie->phy);
>> +
>> +	return ret;
>> +}
>> +
>> +static int qcom_pcie_suspend_2_7_0(struct qcom_pcie *pcie)
>> +{
>> +	struct qcom_pcie_resources_2_7_0 *res = &pcie->res.v2_7_0;
>> +
>> +	phy_power_off(pcie->phy);
>> +
>> +	clk_bulk_disable_unprepare(res->num_clks, res->clks);
>> +
>> +	return 0;
>> +}
>> +
>>   static int qcom_pcie_get_resources_2_9_0(struct qcom_pcie *pcie)
>>   {
>>   	struct qcom_pcie_resources_2_9_0 *res = &pcie->res.v2_9_0;
>> @@ -1590,6 +1620,8 @@ static const struct qcom_pcie_ops ops_1_9_0 = {
>>   	.deinit = qcom_pcie_deinit_2_7_0,
>>   	.ltssm_enable = qcom_pcie_2_3_2_ltssm_enable,
>>   	.config_sid = qcom_pcie_config_sid_sm8250,
>> +	.suspend = qcom_pcie_suspend_2_7_0,
>> +	.resume = qcom_pcie_resume_2_7_0,
>>   };
>>   
>>   /* Qcom IP rev.: 2.9.0  Synopsys IP rev.: 5.00a */
>> @@ -1655,6 +1687,7 @@ static const struct qcom_pcie_cfg sm8450_pcie1_cfg = {
>>   static const struct qcom_pcie_cfg sc7280_cfg = {
>>   	.ops = &ops_1_9_0,
>>   	.has_tbu_clk = true,
>> +	.supports_system_suspend = true,
>>   };
>>   
>>   static const struct qcom_pcie_cfg sc8180x_cfg = {
>> @@ -1760,6 +1793,48 @@ static int qcom_pcie_probe(struct platform_device *pdev)
>>   	return ret;
>>   }
>>   
>> +static int __maybe_unused qcom_pcie_pm_suspend(struct device *dev)
>> +{
>> +	struct qcom_pcie *pcie = dev_get_drvdata(dev);
>> +	u32 val;
>> +
>> +	if (!pcie->cfg->supports_system_suspend)
>> +		return 0;
>> +
>> +	/* if the link is not in l1ss don't turn off clocks */
>> +	val = readl(pcie->parf + PCIE20_PARF_PM_STTS);
>> +	if (!(val & PCIE20_PARF_PM_STTS_LINKST_IN_L1SUB)) {
>> +		dev_warn(dev, "Link is not in L1ss\n");
>> +		return 0;
>> +	}
> If anything, this looks like the version specific code.

This is common for all qcom specific code. Here we are checking for link 
is in l1ss or not

If the link is present in l1ss only we go ahead turn off clocks and phy.

>
>> +
>> +	if (pcie->cfg->ops->suspend)
>> +		pcie->cfg->ops->suspend(pcie);
>> +
>> +	pcie->is_suspended = true;
>> +
>> +	return 0;
>> +}
>> +
>> +static int __maybe_unused qcom_pcie_pm_resume(struct device *dev)
>> +{
>> +	struct qcom_pcie *pcie = dev_get_drvdata(dev);
>> +
>> +	if (!pcie->is_suspended)
>> +		return 0;
> Pretty sure the driver core should take care of not calling resume if
> not suspended.

We are returning success in suspend call even if we didn't turn off 
clocks/phy. so I am using

this flag to know the clocks are off/not.

>
>> +
>> +	if (pcie->cfg->ops->resume)
>> +		pcie->cfg->ops->resume(pcie);
>> +
>> +	pcie->is_suspended = false;
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct dev_pm_ops qcom_pcie_pm_ops = {
>> +	NOIRQ_SYSTEM_SLEEP_PM_OPS(qcom_pcie_pm_suspend, qcom_pcie_pm_resume)
>> +};
>> +
>>   static const struct of_device_id qcom_pcie_match[] = {
>>   	{ .compatible = "qcom,pcie-apq8084", .data = &apq8084_cfg },
>>   	{ .compatible = "qcom,pcie-ipq8064", .data = &ipq8064_cfg },
>> @@ -1796,6 +1871,7 @@ static struct platform_driver qcom_pcie_driver = {
>>   	.probe = qcom_pcie_probe,
>>   	.driver = {
>>   		.name = "qcom-pcie",
>> +		.pm = pm_sleep_ptr(&qcom_pcie_pm_ops),
>>   		.suppress_bind_attrs = true,
>>   		.of_match_table = qcom_pcie_match,
>>   	},
>> -- 
>> 2.7.4
>>

Re: [PATCH v5 1/3] PCI: qcom: Add system PM support

[Krishna Chaitanya Chundru]

On 8/24/2022 8:58 AM, Krishna Chaitanya Chundru wrote:
>
> On 8/11/2022 3:20 AM, Rob Herring wrote:
>> On Wed, Aug 03, 2022 at 04:58:52PM +0530, Krishna chaitanya chundru 
>> wrote:
>>> Add suspend and resume pm callbacks.
>>>
>>> When system suspends, and if the link is in L1ss, disable the clocks
>>> and power down the phy so that system enters into low power state to
>>> save the maximum power. And when the system resumes, enable the clocks
>>> back and power on phy if they are disabled in the suspend path.
>>>
>>> we are doing this only when link is in l1ss but not in L2/L3 as
>>> no where we are forcing link to L2/L3 by sending PME turn off.
>>>
>>> is_suspended flag indicates if the clocks are disabled in the suspend
>>> path or not. And this flag is being used to restrict the access to
>>> config space, dbi etc when clock are turned-off.
>>>
>>> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
>>> ---
>>> Changes since v4:
>>>     - Rebasing the code and removed the supports_system_suspend flag
>>>     - in the resume path as is_suspended will serve its purpose.
>>> Changes since v3:
>>>     - Powering down the phy in suspend and powering it on resume to
>>>       acheive maximum power savings.
>>> Changes since v2:
>>>     - Replaced the enable, disable clks ops with suspend and resume
>>>     - Renamed support_pm_opsi flag  with supports_system_suspend.
>>> Changes since v1:
>>>     - Fixed compilation errors.
>>> ---
>>> ---
>>>   drivers/pci/controller/dwc/pcie-qcom.c | 76 
>>> ++++++++++++++++++++++++++++++++++
>>>   1 file changed, 76 insertions(+)
>>>
>>> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c 
>>> b/drivers/pci/controller/dwc/pcie-qcom.c
>>> index ccf6953..9dd50fc0 100644
>>> --- a/drivers/pci/controller/dwc/pcie-qcom.c
>>> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
>>> @@ -44,6 +44,9 @@
>>>   #define PCIE20_PARF_PM_CTRL            0x20
>>>   #define REQ_NOT_ENTR_L1                BIT(5)
>>>   +#define PCIE20_PARF_PM_STTS            0x24
>>> +#define PCIE20_PARF_PM_STTS_LINKST_IN_L1SUB    BIT(8)
>>> +
>>>   #define PCIE20_PARF_PHY_CTRL            0x40
>>>   #define PHY_CTRL_PHY_TX0_TERM_OFFSET_MASK    GENMASK(20, 16)
>>>   #define PHY_CTRL_PHY_TX0_TERM_OFFSET(x)        ((x) << 16)
>>> @@ -211,6 +214,8 @@ struct qcom_pcie_ops {
>>>       void (*post_deinit)(struct qcom_pcie *pcie);
>>>       void (*ltssm_enable)(struct qcom_pcie *pcie);
>>>       int (*config_sid)(struct qcom_pcie *pcie);
>>> +    int (*suspend)(struct qcom_pcie *pcie);
>>> +    int (*resume)(struct qcom_pcie *pcie);
>> This is really the wrong direction. We don't need the DWC driver
>> defining per platform ops and then a platform defining sub-platform ops.
>> IOW, qcom_pcie_ops should go away.
>>
>> But for now, you don't even need the ops as only 1 version is
>> implemented. Do you really expect other versions to do something
>> different than turn off clocks and phys?
> We will remove this in next patch.

Rob, each version is having a clk structure for example we are using 
"struct qcom_pcie_resources_2_7_0 *res = &pcie->res.v2_7_0;"

This we can't keep these in the common function. so for that reason I 
added these suspend and resume ops.

>>
>>>   };
>>>     struct qcom_pcie_cfg {
>>> @@ -219,6 +224,7 @@ struct qcom_pcie_cfg {
>>>       unsigned int has_ddrss_sf_tbu_clk:1;
>>>       unsigned int has_aggre0_clk:1;
>>>       unsigned int has_aggre1_clk:1;
>>> +    unsigned int supports_system_suspend:1;
>>>   };
>>>     struct qcom_pcie {
>>> @@ -229,6 +235,7 @@ struct qcom_pcie {
>>>       struct phy *phy;
>>>       struct gpio_desc *reset;
>>>       const struct qcom_pcie_cfg *cfg;
>>> +    unsigned int is_suspended:1;
>>>   };
>>>     #define to_qcom_pcie(x) dev_get_drvdata((x)->dev)
>>> @@ -1297,6 +1304,29 @@ static void qcom_pcie_deinit_2_7_0(struct 
>>> qcom_pcie *pcie)
>>>       regulator_bulk_disable(ARRAY_SIZE(res->supplies), res->supplies);
>>>   }
>>>   +static int qcom_pcie_resume_2_7_0(struct qcom_pcie *pcie)
>>> +{
>>> +    struct qcom_pcie_resources_2_7_0 *res = &pcie->res.v2_7_0;
>>> +    int ret;
>>> +
>>> +    ret = clk_bulk_prepare_enable(res->num_clks, res->clks);
>>> +
>>> +    phy_power_on(pcie->phy);
>>> +
>>> +    return ret;
>>> +}
>>> +
>>> +static int qcom_pcie_suspend_2_7_0(struct qcom_pcie *pcie)
>>> +{
>>> +    struct qcom_pcie_resources_2_7_0 *res = &pcie->res.v2_7_0;
>>> +
>>> +    phy_power_off(pcie->phy);
>>> +
>>> +    clk_bulk_disable_unprepare(res->num_clks, res->clks);
>>> +
>>> +    return 0;
>>> +}
>>> +
>>>   static int qcom_pcie_get_resources_2_9_0(struct qcom_pcie *pcie)
>>>   {
>>>       struct qcom_pcie_resources_2_9_0 *res = &pcie->res.v2_9_0;
>>> @@ -1590,6 +1620,8 @@ static const struct qcom_pcie_ops ops_1_9_0 = {
>>>       .deinit = qcom_pcie_deinit_2_7_0,
>>>       .ltssm_enable = qcom_pcie_2_3_2_ltssm_enable,
>>>       .config_sid = qcom_pcie_config_sid_sm8250,
>>> +    .suspend = qcom_pcie_suspend_2_7_0,
>>> +    .resume = qcom_pcie_resume_2_7_0,
>>>   };
>>>     /* Qcom IP rev.: 2.9.0  Synopsys IP rev.: 5.00a */
>>> @@ -1655,6 +1687,7 @@ static const struct qcom_pcie_cfg 
>>> sm8450_pcie1_cfg = {
>>>   static const struct qcom_pcie_cfg sc7280_cfg = {
>>>       .ops = &ops_1_9_0,
>>>       .has_tbu_clk = true,
>>> +    .supports_system_suspend = true,
>>>   };
>>>     static const struct qcom_pcie_cfg sc8180x_cfg = {
>>> @@ -1760,6 +1793,48 @@ static int qcom_pcie_probe(struct 
>>> platform_device *pdev)
>>>       return ret;
>>>   }
>>>   +static int __maybe_unused qcom_pcie_pm_suspend(struct device *dev)
>>> +{
>>> +    struct qcom_pcie *pcie = dev_get_drvdata(dev);
>>> +    u32 val;
>>> +
>>> +    if (!pcie->cfg->supports_system_suspend)
>>> +        return 0;
>>> +
>>> +    /* if the link is not in l1ss don't turn off clocks */
>>> +    val = readl(pcie->parf + PCIE20_PARF_PM_STTS);
>>> +    if (!(val & PCIE20_PARF_PM_STTS_LINKST_IN_L1SUB)) {
>>> +        dev_warn(dev, "Link is not in L1ss\n");
>>> +        return 0;
>>> +    }
>> If anything, this looks like the version specific code.
>
> This is common for all qcom specific code. Here we are checking for 
> link is in l1ss or not
>
> If the link is present in l1ss only we go ahead turn off clocks and phy.
>
>>
>>> +
>>> +    if (pcie->cfg->ops->suspend)
>>> +        pcie->cfg->ops->suspend(pcie);
>>> +
>>> +    pcie->is_suspended = true;
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static int __maybe_unused qcom_pcie_pm_resume(struct device *dev)
>>> +{
>>> +    struct qcom_pcie *pcie = dev_get_drvdata(dev);
>>> +
>>> +    if (!pcie->is_suspended)
>>> +        return 0;
>> Pretty sure the driver core should take care of not calling resume if
>> not suspended.
>
> We are returning success in suspend call even if we didn't turn off 
> clocks/phy. so I am using
>
> this flag to know the clocks are off/not.
>
>>
>>> +
>>> +    if (pcie->cfg->ops->resume)
>>> +        pcie->cfg->ops->resume(pcie);
>>> +
>>> +    pcie->is_suspended = false;
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static const struct dev_pm_ops qcom_pcie_pm_ops = {
>>> +    NOIRQ_SYSTEM_SLEEP_PM_OPS(qcom_pcie_pm_suspend, 
>>> qcom_pcie_pm_resume)
>>> +};
>>> +
>>>   static const struct of_device_id qcom_pcie_match[] = {
>>>       { .compatible = "qcom,pcie-apq8084", .data = &apq8084_cfg },
>>>       { .compatible = "qcom,pcie-ipq8064", .data = &ipq8064_cfg },
>>> @@ -1796,6 +1871,7 @@ static struct platform_driver qcom_pcie_driver 
>>> = {
>>>       .probe = qcom_pcie_probe,
>>>       .driver = {
>>>           .name = "qcom-pcie",
>>> +        .pm = pm_sleep_ptr(&qcom_pcie_pm_ops),
>>>           .suppress_bind_attrs = true,
>>>           .of_match_table = qcom_pcie_match,
>>>       },
>>> -- 
>>> 2.7.4
>>>

[PATCH v5 2/3] PCI: qcom: Restrict pci transactions after pci suspend

[Krishna chaitanya chundru]

If the endpoint device state is D0 and irq's are not freed, then
kernel try to mask interrupts in system suspend path by writing
in to the vector table (for MSIX interrupts) and config space (for MSI's).

These transactions are initiated in the pm suspend after pcie clocks got
disabled as part of platform driver pm  suspend call. Due to it, these
transactions are resulting in un-clocked access and eventually to crashes.

So added a logic in qcom driver to restrict these unclocked access.
And updated the logic to check the link state before masking
or unmasking the interrupts.

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            | 36 +++++++++++++++++++++--
 2 files changed, 46 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
index 9979302..2b1e395 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 9dd50fc0..f7dd5dc 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -1433,11 +1433,41 @@ static int qcom_pcie_post_init_2_9_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);
 }
 
@@ -1702,6 +1732,8 @@ static const struct qcom_pcie_cfg ipq6018_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

Re: [PATCH v5 2/3] PCI: qcom: Restrict pci transactions after pci suspend

[kernel test robot]

Hi Krishna,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on helgaas-pci/next]
[also build test ERROR on next-20220803]
[cannot apply to linus/master v5.19]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Krishna-chaitanya-chundru/PCI-Restrict-pci-transactions-after-pci-suspend/20220803-193033
base:   https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next
config: s390-randconfig-r044-20220804 (https://download.01.org/0day-ci/archive/20220804/202208041800.BLHfNzzW-lkp@intel.com/config)
compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project 26dd42705c2af0b8f6e5d6cdb32c9bd5ed9524eb)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install s390 cross compiling tool for clang build
        # apt-get install binutils-s390x-linux-gnu
        # https://github.com/intel-lab-lkp/linux/commit/9e799ab52db3245911b15a8903977d99554d900d
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Krishna-chaitanya-chundru/PCI-Restrict-pci-transactions-after-pci-suspend/20220803-193033
        git checkout 9e799ab52db3245911b15a8903977d99554d900d
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=s390 SHELL=/bin/bash drivers/pci/controller/dwc/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from drivers/pci/controller/dwc/pcie-designware-host.c:11:
   In file included from include/linux/irqchip/chained_irq.h:10:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/s390/include/asm/io.h:75:
   include/asm-generic/io.h:464:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __raw_readb(PCI_IOBASE + addr);
                             ~~~~~~~~~~ ^
   include/asm-generic/io.h:477:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
                                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/big_endian.h:37:59: note: expanded from macro '__le16_to_cpu'
   #define __le16_to_cpu(x) __swab16((__force __u16)(__le16)(x))
                                                             ^
   include/uapi/linux/swab.h:102:54: note: expanded from macro '__swab16'
   #define __swab16(x) (__u16)__builtin_bswap16((__u16)(x))
                                                        ^
   In file included from drivers/pci/controller/dwc/pcie-designware-host.c:11:
   In file included from include/linux/irqchip/chained_irq.h:10:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/s390/include/asm/io.h:75:
   include/asm-generic/io.h:490:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
                                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/big_endian.h:35:59: note: expanded from macro '__le32_to_cpu'
   #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
                                                             ^
   include/uapi/linux/swab.h:115:54: note: expanded from macro '__swab32'
   #define __swab32(x) (__u32)__builtin_bswap32((__u32)(x))
                                                        ^
   In file included from drivers/pci/controller/dwc/pcie-designware-host.c:11:
   In file included from include/linux/irqchip/chained_irq.h:10:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/s390/include/asm/io.h:75:
   include/asm-generic/io.h:501:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writeb(value, PCI_IOBASE + addr);
                               ~~~~~~~~~~ ^
   include/asm-generic/io.h:511:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
                                                         ~~~~~~~~~~ ^
   include/asm-generic/io.h:521:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
                                                         ~~~~~~~~~~ ^
   include/asm-generic/io.h:609:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           readsb(PCI_IOBASE + addr, buffer, count);
                  ~~~~~~~~~~ ^
   include/asm-generic/io.h:617:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           readsw(PCI_IOBASE + addr, buffer, count);
                  ~~~~~~~~~~ ^
   include/asm-generic/io.h:625:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           readsl(PCI_IOBASE + addr, buffer, count);
                  ~~~~~~~~~~ ^
   include/asm-generic/io.h:634:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           writesb(PCI_IOBASE + addr, buffer, count);
                   ~~~~~~~~~~ ^
   include/asm-generic/io.h:643:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           writesw(PCI_IOBASE + addr, buffer, count);
                   ~~~~~~~~~~ ^
   include/asm-generic/io.h:652:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           writesl(PCI_IOBASE + addr, buffer, count);
                   ~~~~~~~~~~ ^
>> drivers/pci/controller/dwc/pcie-designware-host.c:33:24: error: static assertion failed due to requirement '__builtin_types_compatible_p(struct pcie_port, struct dw_pcie_rp) || __builtin_types_compatible_p(struct pcie_port, void)': pointer type mismatch in container_of()
           struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
                                 ^~~~~~~~~~~~~~~~~~~~~~
   drivers/pci/controller/dwc/pcie-designware.h:327:34: note: expanded from macro 'to_dw_pcie_from_pp'
   #define to_dw_pcie_from_pp(port) container_of((port), struct dw_pcie, pp)
                                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/container_of.h:19:2: note: expanded from macro 'container_of'
           static_assert(__same_type(*(ptr), ((type *)0)->member) ||       \
           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/build_bug.h:77:34: note: expanded from macro 'static_assert'
   #define static_assert(expr, ...) __static_assert(expr, ##__VA_ARGS__, #expr)
                                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/build_bug.h:78:41: note: expanded from macro '__static_assert'
   #define __static_assert(expr, msg, ...) _Static_assert(expr, msg)
                                           ^              ~~~~
   drivers/pci/controller/dwc/pcie-designware-host.c:44:24: error: static assertion failed due to requirement '__builtin_types_compatible_p(struct pcie_port, struct dw_pcie_rp) || __builtin_types_compatible_p(struct pcie_port, void)': pointer type mismatch in container_of()
           struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
                                 ^~~~~~~~~~~~~~~~~~~~~~
   drivers/pci/controller/dwc/pcie-designware.h:327:34: note: expanded from macro 'to_dw_pcie_from_pp'
   #define to_dw_pcie_from_pp(port) container_of((port), struct dw_pcie, pp)
                                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/container_of.h:19:2: note: expanded from macro 'container_of'
           static_assert(__same_type(*(ptr), ((type *)0)->member) ||       \
           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/build_bug.h:77:34: note: expanded from macro 'static_assert'
   #define static_assert(expr, ...) __static_assert(expr, ##__VA_ARGS__, #expr)
                                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/build_bug.h:78:41: note: expanded from macro '__static_assert'
   #define __static_assert(expr, msg, ...) _Static_assert(expr, msg)
                                           ^              ~~~~
   12 warnings and 2 errors generated.


vim +33 drivers/pci/controller/dwc/pcie-designware-host.c

    29	
    30	static void dw_msi_mask_irq(struct irq_data *d)
    31	{
    32		struct pcie_port *pp = irq_data_get_irq_chip_data(d->parent_data);
  > 33		struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
    34	
    35		if (dw_pcie_link_up(pci))
    36			pci_msi_mask_irq(d);
    37	
    38		irq_chip_mask_parent(d);
    39	}
    40	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH v5 2/3] PCI: qcom: Restrict pci transactions after pci suspend

[Stephen Boyd]

Quoting Krishna chaitanya chundru (2022-08-03 04:28:53)
> If the endpoint device state is D0 and irq's are not freed, then
> kernel try to mask interrupts in system suspend path by writing
> in to the vector table (for MSIX interrupts) and config space (for MSI's).
>
> These transactions are initiated in the pm suspend after pcie clocks got
> disabled as part of platform driver pm  suspend call. Due to it, these
> transactions are resulting in un-clocked access and eventually to crashes.

Why are the platform driver pm suspend calls disabling clks that early?
Can they disable clks in noirq phase, or even later, so that we don't
have to check if the device is clocking in the irq poking functions?
It's best to keep irq operations fast, so that irq control is fast given
that these functions are called from irq flow handlers.

Re: [PATCH v5 2/3] PCI: qcom: Restrict pci transactions after pci suspend

[Krishna Chaitanya Chundru]

On 8/9/2022 12:42 AM, Stephen Boyd wrote:
> Quoting Krishna chaitanya chundru (2022-08-03 04:28:53)
>> If the endpoint device state is D0 and irq's are not freed, then
>> kernel try to mask interrupts in system suspend path by writing
>> in to the vector table (for MSIX interrupts) and config space (for MSI's).
>>
>> These transactions are initiated in the pm suspend after pcie clocks got
>> disabled as part of platform driver pm  suspend call. Due to it, these
>> transactions are resulting in un-clocked access and eventually to crashes.
> Why are the platform driver pm suspend calls disabling clks that early?
> Can they disable clks in noirq phase, or even later, so that we don't
> have to check if the device is clocking in the irq poking functions?
> It's best to keep irq operations fast, so that irq control is fast given
> that these functions are called from irq flow handlers.

We are registering the pcie pm suspend ops as noirq ops only. And this 
msix and config

access is coming at the later point of time that is reason we added that 
check.

Re: [PATCH v5 2/3] PCI: qcom: Restrict pci transactions after pci suspend

[Stephen Boyd]

Quoting Krishna Chaitanya Chundru (2022-08-23 20:37:59)
>
> On 8/9/2022 12:42 AM, Stephen Boyd wrote:
> > Quoting Krishna chaitanya chundru (2022-08-03 04:28:53)
> >> If the endpoint device state is D0 and irq's are not freed, then
> >> kernel try to mask interrupts in system suspend path by writing
> >> in to the vector table (for MSIX interrupts) and config space (for MSI's).
> >>
> >> These transactions are initiated in the pm suspend after pcie clocks got
> >> disabled as part of platform driver pm  suspend call. Due to it, these
> >> transactions are resulting in un-clocked access and eventually to crashes.
> > Why are the platform driver pm suspend calls disabling clks that early?
> > Can they disable clks in noirq phase, or even later, so that we don't
> > have to check if the device is clocking in the irq poking functions?
> > It's best to keep irq operations fast, so that irq control is fast given
> > that these functions are called from irq flow handlers.
>
> We are registering the pcie pm suspend ops as noirq ops only. And this
> msix and config
>
> access is coming at the later point of time that is reason we added that
> check.
>

What is accessing msix and config? Can you dump_stack() after noirq ops
are called and figure out what is trying to access the bus when it is
powered down?

Re: [PATCH v5 2/3] PCI: qcom: Restrict pci transactions after pci suspend

[Krishna Chaitanya Chundru]

On 8/24/2022 10:50 PM, Stephen Boyd wrote:
> Quoting Krishna Chaitanya Chundru (2022-08-23 20:37:59)
>> On 8/9/2022 12:42 AM, Stephen Boyd wrote:
>>> Quoting Krishna chaitanya chundru (2022-08-03 04:28:53)
>>>> If the endpoint device state is D0 and irq's are not freed, then
>>>> kernel try to mask interrupts in system suspend path by writing
>>>> in to the vector table (for MSIX interrupts) and config space (for MSI's).
>>>>
>>>> These transactions are initiated in the pm suspend after pcie clocks got
>>>> disabled as part of platform driver pm  suspend call. Due to it, these
>>>> transactions are resulting in un-clocked access and eventually to crashes.
>>> Why are the platform driver pm suspend calls disabling clks that early?
>>> Can they disable clks in noirq phase, or even later, so that we don't
>>> have to check if the device is clocking in the irq poking functions?
>>> It's best to keep irq operations fast, so that irq control is fast given
>>> that these functions are called from irq flow handlers.
>> We are registering the pcie pm suspend ops as noirq ops only. And this
>> msix and config
>>
>> access is coming at the later point of time that is reason we added that
>> check.
>>
> What is accessing msix and config? Can you dump_stack() after noirq ops
> are called and figure out what is trying to access the bus when it is
> powered down?

The msix and config space is being accessed to mask interrupts. The 
access is coming at the end of the suspend

and near CPU disable. We tried to dump the stack there but the call 
stack is not coming as it is near cpu disable.

But we got dump at resume please have look at it

[   54.946268] Enabling non-boot CPUs ...
[   54.951182] CPU: 1 PID: 21 Comm: cpuhp/1 Not tainted 5.15.41 #105 
43491e4414b1db8a6f59d56b617b520d92a9498e
[   54.961122] Hardware name: Qualcomm Technologies, Inc. sc7280 IDP 
SKU2 platform (DT)
[   54.969088] Call trace:
[   54.971612]  dump_backtrace+0x0/0x200
[   54.975399]  show_stack+0x20/0x2c
[   54.978826]  dump_stack_lvl+0x6c/0x90
[   54.982614]  dump_stack+0x18/0x38
[   54.986043]  dw_msi_unmask_irq+0x2c/0x58
[   54.990096]  irq_enable+0x58/0x90
[   54.993522]  __irq_startup+0x68/0x94
[   54.997216]  irq_startup+0xf4/0x140
[   55.000820]  irq_affinity_online_cpu+0xc8/0x154
[   55.005491]  cpuhp_invoke_callback+0x19c/0x6e4
[   55.010077]  cpuhp_thread_fun+0x11c/0x188
[   55.014216]  smpboot_thread_fn+0x1ac/0x30c
[   55.018445]  kthread+0x140/0x30c
[   55.021788]  ret_from_fork+0x10/0x20
[   55.028243] CPU1 is up

So the same stack should be called at the suspend path while disabling CPU.

If there is any other way to remove these calls can you please help us 
point that way.

Thanks & Regards,
Krishna Chaitanya

Re: [PATCH v5 2/3] PCI: qcom: Restrict pci transactions after pci suspend

[Stephen Boyd]

Quoting Krishna Chaitanya Chundru (2022-08-25 06:52:43)
>
> On 8/24/2022 10:50 PM, Stephen Boyd wrote:
> > Quoting Krishna Chaitanya Chundru (2022-08-23 20:37:59)
> >> On 8/9/2022 12:42 AM, Stephen Boyd wrote:
> >>> Quoting Krishna chaitanya chundru (2022-08-03 04:28:53)
> >>>> If the endpoint device state is D0 and irq's are not freed, then
> >>>> kernel try to mask interrupts in system suspend path by writing
> >>>> in to the vector table (for MSIX interrupts) and config space (for MSI's).
> >>>>
> >>>> These transactions are initiated in the pm suspend after pcie clocks got
> >>>> disabled as part of platform driver pm  suspend call. Due to it, these
> >>>> transactions are resulting in un-clocked access and eventually to crashes.
> >>> Why are the platform driver pm suspend calls disabling clks that early?
> >>> Can they disable clks in noirq phase, or even later, so that we don't
> >>> have to check if the device is clocking in the irq poking functions?
> >>> It's best to keep irq operations fast, so that irq control is fast given
> >>> that these functions are called from irq flow handlers.
> >> We are registering the pcie pm suspend ops as noirq ops only. And this
> >> msix and config
> >>
> >> access is coming at the later point of time that is reason we added that
> >> check.
> >>
> > What is accessing msix and config? Can you dump_stack() after noirq ops
> > are called and figure out what is trying to access the bus when it is
> > powered down?
>
> The msix and config space is being accessed to mask interrupts. The
> access is coming at the end of the suspend
>
> and near CPU disable. We tried to dump the stack there but the call
> stack is not coming as it is near cpu disable.

That is odd that you can't get a stacktrace.

>
> But we got dump at resume please have look at it
>
> [   54.946268] Enabling non-boot CPUs ...
> [   54.951182] CPU: 1 PID: 21 Comm: cpuhp/1 Not tainted 5.15.41 #105
> 43491e4414b1db8a6f59d56b617b520d92a9498e
> [   54.961122] Hardware name: Qualcomm Technologies, Inc. sc7280 IDP
> SKU2 platform (DT)
> [   54.969088] Call trace:
> [   54.971612]  dump_backtrace+0x0/0x200
> [   54.975399]  show_stack+0x20/0x2c
> [   54.978826]  dump_stack_lvl+0x6c/0x90
> [   54.982614]  dump_stack+0x18/0x38
> [   54.986043]  dw_msi_unmask_irq+0x2c/0x58
> [   54.990096]  irq_enable+0x58/0x90
> [   54.993522]  __irq_startup+0x68/0x94
> [   54.997216]  irq_startup+0xf4/0x140
> [   55.000820]  irq_affinity_online_cpu+0xc8/0x154
> [   55.005491]  cpuhp_invoke_callback+0x19c/0x6e4
> [   55.010077]  cpuhp_thread_fun+0x11c/0x188
> [   55.014216]  smpboot_thread_fn+0x1ac/0x30c
> [   55.018445]  kthread+0x140/0x30c
> [   55.021788]  ret_from_fork+0x10/0x20
> [   55.028243] CPU1 is up
>
> So the same stack should be called at the suspend path while disabling CPU.

Sounds like you're getting hit by affinity changes while offlining CPUs
during suspend (see irq_migrate_all_off_this_cpu()). That will happen
after devices are suspended (all phases of suspend ops).

>
> If there is any other way to remove these calls can you please help us
> point that way.

I'm not sure. I believe genirq assumes the irqchips are always
accessible. There is some support to suspend irqchips. See how the
struct irq_chip::irq_suspend() function is called by syscore ops in the
generic irqchip 'irq_gc_syscore_ops' hooks. Maybe you could add a
syscore suspend/resume hook to disable/enable the clks and power to the
PCI controller. syscore ops run after secondary CPUs are hotplugged out
during suspend.

Or maybe setting the IRQCHIP_MASK_ON_SUSPEND flag can be used so that on
irq migration nothing writes the irq hardware because it is already
masked in the hardware earlier. I think the problem is that on resume
we'll restart the irq from the first CPU online event, when you don't
want to do that because it is too early.

I have another question though, which is do MSIs support wakeup? I don't
see how it works if the whole bus is effectively off during suspend. If
wakeup needs to be supported then I suspect the bus can't be powered
down during suspend.

Re: [PATCH v5 2/3] PCI: qcom: Restrict pci transactions after pci suspend

[Manivannan Sadhasivam]

On Fri, Aug 26, 2022 at 03:23:00PM -0500, Stephen Boyd wrote:
> Quoting Krishna Chaitanya Chundru (2022-08-25 06:52:43)
> >
> > On 8/24/2022 10:50 PM, Stephen Boyd wrote:
> > > Quoting Krishna Chaitanya Chundru (2022-08-23 20:37:59)
> > >> On 8/9/2022 12:42 AM, Stephen Boyd wrote:
> > >>> Quoting Krishna chaitanya chundru (2022-08-03 04:28:53)
> > >>>> If the endpoint device state is D0 and irq's are not freed, then
> > >>>> kernel try to mask interrupts in system suspend path by writing
> > >>>> in to the vector table (for MSIX interrupts) and config space (for MSI's).
> > >>>>
> > >>>> These transactions are initiated in the pm suspend after pcie clocks got
> > >>>> disabled as part of platform driver pm  suspend call. Due to it, these
> > >>>> transactions are resulting in un-clocked access and eventually to crashes.
> > >>> Why are the platform driver pm suspend calls disabling clks that early?
> > >>> Can they disable clks in noirq phase, or even later, so that we don't
> > >>> have to check if the device is clocking in the irq poking functions?
> > >>> It's best to keep irq operations fast, so that irq control is fast given
> > >>> that these functions are called from irq flow handlers.
> > >> We are registering the pcie pm suspend ops as noirq ops only. And this
> > >> msix and config
> > >>
> > >> access is coming at the later point of time that is reason we added that
> > >> check.
> > >>
> > > What is accessing msix and config? Can you dump_stack() after noirq ops
> > > are called and figure out what is trying to access the bus when it is
> > > powered down?
> >
> > The msix and config space is being accessed to mask interrupts. The
> > access is coming at the end of the suspend
> >
> > and near CPU disable. We tried to dump the stack there but the call
> > stack is not coming as it is near cpu disable.
> 
> That is odd that you can't get a stacktrace.
> 
> >
> > But we got dump at resume please have look at it
> >
> > [   54.946268] Enabling non-boot CPUs ...
> > [   54.951182] CPU: 1 PID: 21 Comm: cpuhp/1 Not tainted 5.15.41 #105
> > 43491e4414b1db8a6f59d56b617b520d92a9498e
> > [   54.961122] Hardware name: Qualcomm Technologies, Inc. sc7280 IDP
> > SKU2 platform (DT)
> > [   54.969088] Call trace:
> > [   54.971612]  dump_backtrace+0x0/0x200
> > [   54.975399]  show_stack+0x20/0x2c
> > [   54.978826]  dump_stack_lvl+0x6c/0x90
> > [   54.982614]  dump_stack+0x18/0x38
> > [   54.986043]  dw_msi_unmask_irq+0x2c/0x58
> > [   54.990096]  irq_enable+0x58/0x90
> > [   54.993522]  __irq_startup+0x68/0x94
> > [   54.997216]  irq_startup+0xf4/0x140
> > [   55.000820]  irq_affinity_online_cpu+0xc8/0x154
> > [   55.005491]  cpuhp_invoke_callback+0x19c/0x6e4
> > [   55.010077]  cpuhp_thread_fun+0x11c/0x188
> > [   55.014216]  smpboot_thread_fn+0x1ac/0x30c
> > [   55.018445]  kthread+0x140/0x30c
> > [   55.021788]  ret_from_fork+0x10/0x20
> > [   55.028243] CPU1 is up
> >
> > So the same stack should be called at the suspend path while disabling CPU.
> 
> Sounds like you're getting hit by affinity changes while offlining CPUs
> during suspend (see irq_migrate_all_off_this_cpu()). That will happen
> after devices are suspended (all phases of suspend ops).

The affinity setting should not happen since DWC MSI controller doesn't support
setting IRQ affinity (hierarchial IRQ domain). In the migrate_one_irq()
function, there is a check for the existence of the irq_set_affinity()
callback, but the DWC MSI controller return -EINVAL in the callback. So this
is the reason the migration was still atempted?

A quick check would be to test this suspend/resume with GIC ITS for MSI since
it supports settings IRQ affinity and resides in a separate domain.
Chaitanya, can you try that?

> 
> >
> > If there is any other way to remove these calls can you please help us
> > point that way.
> 
> I'm not sure. I believe genirq assumes the irqchips are always
> accessible. There is some support to suspend irqchips. See how the
> struct irq_chip::irq_suspend() function is called by syscore ops in the
> generic irqchip 'irq_gc_syscore_ops' hooks. Maybe you could add a
> syscore suspend/resume hook to disable/enable the clks and power to the
> PCI controller. syscore ops run after secondary CPUs are hotplugged out
> during suspend.
> 
> Or maybe setting the IRQCHIP_MASK_ON_SUSPEND flag can be used so that on
> irq migration nothing writes the irq hardware because it is already
> masked in the hardware earlier. I think the problem is that on resume
> we'll restart the irq from the first CPU online event, when you don't
> want to do that because it is too early.
> 
> I have another question though, which is do MSIs support wakeup? I don't
> see how it works if the whole bus is effectively off during suspend. If
> wakeup needs to be supported then I suspect the bus can't be powered
> down during suspend.

Wake up should be handled by a dedicated side-band GPIO or in-band PME message.

But I still wonder how the link stays in L1/L1ss when the clocks are disabled
and PHY is powered down. Maybe the link or phy is powered by a separate power
domain like MX that keeps the link active?

Thanks,
Mani

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

Re: [PATCH v5 2/3] PCI: qcom: Restrict pci transactions after pci suspend

[Krishna Chaitanya Chundru]

On 8/27/2022 10:56 PM, Manivannan Sadhasivam wrote:
> On Fri, Aug 26, 2022 at 03:23:00PM -0500, Stephen Boyd wrote:
>> Quoting Krishna Chaitanya Chundru (2022-08-25 06:52:43)
>>> On 8/24/2022 10:50 PM, Stephen Boyd wrote:
>>>> Quoting Krishna Chaitanya Chundru (2022-08-23 20:37:59)
>>>>> On 8/9/2022 12:42 AM, Stephen Boyd wrote:
>>>>>> Quoting Krishna chaitanya chundru (2022-08-03 04:28:53)
>>>>>>> If the endpoint device state is D0 and irq's are not freed, then
>>>>>>> kernel try to mask interrupts in system suspend path by writing
>>>>>>> in to the vector table (for MSIX interrupts) and config space (for MSI's).
>>>>>>>
>>>>>>> These transactions are initiated in the pm suspend after pcie clocks got
>>>>>>> disabled as part of platform driver pm  suspend call. Due to it, these
>>>>>>> transactions are resulting in un-clocked access and eventually to crashes.
>>>>>> Why are the platform driver pm suspend calls disabling clks that early?
>>>>>> Can they disable clks in noirq phase, or even later, so that we don't
>>>>>> have to check if the device is clocking in the irq poking functions?
>>>>>> It's best to keep irq operations fast, so that irq control is fast given
>>>>>> that these functions are called from irq flow handlers.
>>>>> We are registering the pcie pm suspend ops as noirq ops only. And this
>>>>> msix and config
>>>>>
>>>>> access is coming at the later point of time that is reason we added that
>>>>> check.
>>>>>
>>>> What is accessing msix and config? Can you dump_stack() after noirq ops
>>>> are called and figure out what is trying to access the bus when it is
>>>> powered down?
>>> The msix and config space is being accessed to mask interrupts. The
>>> access is coming at the end of the suspend
>>>
>>> and near CPU disable. We tried to dump the stack there but the call
>>> stack is not coming as it is near cpu disable.
>> That is odd that you can't get a stacktrace.
>>
>>> But we got dump at resume please have look at it
>>>
>>> [   54.946268] Enabling non-boot CPUs ...
>>> [   54.951182] CPU: 1 PID: 21 Comm: cpuhp/1 Not tainted 5.15.41 #105
>>> 43491e4414b1db8a6f59d56b617b520d92a9498e
>>> [   54.961122] Hardware name: Qualcomm Technologies, Inc. sc7280 IDP
>>> SKU2 platform (DT)
>>> [   54.969088] Call trace:
>>> [   54.971612]  dump_backtrace+0x0/0x200
>>> [   54.975399]  show_stack+0x20/0x2c
>>> [   54.978826]  dump_stack_lvl+0x6c/0x90
>>> [   54.982614]  dump_stack+0x18/0x38
>>> [   54.986043]  dw_msi_unmask_irq+0x2c/0x58
>>> [   54.990096]  irq_enable+0x58/0x90
>>> [   54.993522]  __irq_startup+0x68/0x94
>>> [   54.997216]  irq_startup+0xf4/0x140
>>> [   55.000820]  irq_affinity_online_cpu+0xc8/0x154
>>> [   55.005491]  cpuhp_invoke_callback+0x19c/0x6e4
>>> [   55.010077]  cpuhp_thread_fun+0x11c/0x188
>>> [   55.014216]  smpboot_thread_fn+0x1ac/0x30c
>>> [   55.018445]  kthread+0x140/0x30c
>>> [   55.021788]  ret_from_fork+0x10/0x20
>>> [   55.028243] CPU1 is up
>>>
>>> So the same stack should be called at the suspend path while disabling CPU.
>> Sounds like you're getting hit by affinity changes while offlining CPUs
>> during suspend (see irq_migrate_all_off_this_cpu()). That will happen
>> after devices are suspended (all phases of suspend ops).
> The affinity setting should not happen since DWC MSI controller doesn't support
> setting IRQ affinity (hierarchial IRQ domain). In the migrate_one_irq()
> function, there is a check for the existence of the irq_set_affinity()
> callback, but the DWC MSI controller return -EINVAL in the callback. So this
> is the reason the migration was still atempted?
>
> A quick check would be to test this suspend/resume with GIC ITS for MSI since
> it supports settings IRQ affinity and resides in a separate domain.
> Chaitanya, can you try that?

Hi mani,

I tried with gic its there also I see same behavior.

The only which helps to comment out affinity in the following function.

diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c
index 21b3ac2a29d2..042afec1cf9d 100644
--- a/kernel/irq/irqdesc.c
+++ b/kernel/irq/irqdesc.c
@@ -487,8 +487,9 @@ static int alloc_descs(unsigned int start, unsigned 
int cnt, int node,



                if (affinity) {
                         if (affinity->is_managed) {
-                               flags = IRQD_AFFINITY_MANAGED |
-                                       IRQD_MANAGED_SHUTDOWN;
+//                             flags = IRQD_AFFINITY_MANAGED |
+//                                     IRQD_MANAGED_SHUTDOWN;
+                               flags = 0;//IRQD_AFFINITY_MANAGED |
                         }
                         mask = &affinity->mask;
                         node = cpu_to_node(cpumask_first(mask));

>>> If there is any other way to remove these calls can you please help us
>>> point that way.
>> I'm not sure. I believe genirq assumes the irqchips are always
>> accessible. There is some support to suspend irqchips. See how the
>> struct irq_chip::irq_suspend() function is called by syscore ops in the
>> generic irqchip 'irq_gc_syscore_ops' hooks. Maybe you could add a
>> syscore suspend/resume hook to disable/enable the clks and power to the
>> PCI controller. syscore ops run after secondary CPUs are hotplugged out
>> during suspend.
>>
>> Or maybe setting the IRQCHIP_MASK_ON_SUSPEND flag can be used so that on
>> irq migration nothing writes the irq hardware because it is already
>> masked in the hardware earlier. I think the problem is that on resume
>> we'll restart the irq from the first CPU online event, when you don't
>> want to do that because it is too early.
>>
>> I have another question though, which is do MSIs support wakeup? I don't
>> see how it works if the whole bus is effectively off during suspend. If
>> wakeup needs to be supported then I suspect the bus can't be powered
>> down during suspend.
> Wake up should be handled by a dedicated side-band GPIO or in-band PME message.
>
> But I still wonder how the link stays in L1/L1ss when the clocks are disabled
> and PHY is powered down. Maybe the link or phy is powered by a separate power
> domain like MX that keeps the link active?
We will come back to you on this.
>
> Thanks,
> Mani
>

Re: [PATCH v5 2/3] PCI: qcom: Restrict pci transactions after pci suspend

[Manivannan Sadhasivam]

On Mon, Aug 29, 2022 at 11:01:58PM +0530, Krishna Chaitanya Chundru wrote:
> 
> On 8/27/2022 10:56 PM, Manivannan Sadhasivam wrote:
> > On Fri, Aug 26, 2022 at 03:23:00PM -0500, Stephen Boyd wrote:
> > > Quoting Krishna Chaitanya Chundru (2022-08-25 06:52:43)
> > > > On 8/24/2022 10:50 PM, Stephen Boyd wrote:
> > > > > Quoting Krishna Chaitanya Chundru (2022-08-23 20:37:59)
> > > > > > On 8/9/2022 12:42 AM, Stephen Boyd wrote:
> > > > > > > Quoting Krishna chaitanya chundru (2022-08-03 04:28:53)
> > > > > > > > If the endpoint device state is D0 and irq's are not freed, then
> > > > > > > > kernel try to mask interrupts in system suspend path by writing
> > > > > > > > in to the vector table (for MSIX interrupts) and config space (for MSI's).
> > > > > > > > 
> > > > > > > > These transactions are initiated in the pm suspend after pcie clocks got
> > > > > > > > disabled as part of platform driver pm  suspend call. Due to it, these
> > > > > > > > transactions are resulting in un-clocked access and eventually to crashes.
> > > > > > > Why are the platform driver pm suspend calls disabling clks that early?
> > > > > > > Can they disable clks in noirq phase, or even later, so that we don't
> > > > > > > have to check if the device is clocking in the irq poking functions?
> > > > > > > It's best to keep irq operations fast, so that irq control is fast given
> > > > > > > that these functions are called from irq flow handlers.
> > > > > > We are registering the pcie pm suspend ops as noirq ops only. And this
> > > > > > msix and config
> > > > > > 
> > > > > > access is coming at the later point of time that is reason we added that
> > > > > > check.
> > > > > > 
> > > > > What is accessing msix and config? Can you dump_stack() after noirq ops
> > > > > are called and figure out what is trying to access the bus when it is
> > > > > powered down?
> > > > The msix and config space is being accessed to mask interrupts. The
> > > > access is coming at the end of the suspend
> > > > 
> > > > and near CPU disable. We tried to dump the stack there but the call
> > > > stack is not coming as it is near cpu disable.
> > > That is odd that you can't get a stacktrace.
> > > 
> > > > But we got dump at resume please have look at it
> > > > 
> > > > [   54.946268] Enabling non-boot CPUs ...
> > > > [   54.951182] CPU: 1 PID: 21 Comm: cpuhp/1 Not tainted 5.15.41 #105
> > > > 43491e4414b1db8a6f59d56b617b520d92a9498e
> > > > [   54.961122] Hardware name: Qualcomm Technologies, Inc. sc7280 IDP
> > > > SKU2 platform (DT)
> > > > [   54.969088] Call trace:
> > > > [   54.971612]  dump_backtrace+0x0/0x200
> > > > [   54.975399]  show_stack+0x20/0x2c
> > > > [   54.978826]  dump_stack_lvl+0x6c/0x90
> > > > [   54.982614]  dump_stack+0x18/0x38
> > > > [   54.986043]  dw_msi_unmask_irq+0x2c/0x58
> > > > [   54.990096]  irq_enable+0x58/0x90
> > > > [   54.993522]  __irq_startup+0x68/0x94
> > > > [   54.997216]  irq_startup+0xf4/0x140
> > > > [   55.000820]  irq_affinity_online_cpu+0xc8/0x154
> > > > [   55.005491]  cpuhp_invoke_callback+0x19c/0x6e4
> > > > [   55.010077]  cpuhp_thread_fun+0x11c/0x188
> > > > [   55.014216]  smpboot_thread_fn+0x1ac/0x30c
> > > > [   55.018445]  kthread+0x140/0x30c
> > > > [   55.021788]  ret_from_fork+0x10/0x20
> > > > [   55.028243] CPU1 is up
> > > > 
> > > > So the same stack should be called at the suspend path while disabling CPU.
> > > Sounds like you're getting hit by affinity changes while offlining CPUs
> > > during suspend (see irq_migrate_all_off_this_cpu()). That will happen
> > > after devices are suspended (all phases of suspend ops).
> > The affinity setting should not happen since DWC MSI controller doesn't support
> > setting IRQ affinity (hierarchial IRQ domain). In the migrate_one_irq()
> > function, there is a check for the existence of the irq_set_affinity()
> > callback, but the DWC MSI controller return -EINVAL in the callback. So this
> > is the reason the migration was still atempted?
> > 
> > A quick check would be to test this suspend/resume with GIC ITS for MSI since
> > it supports settings IRQ affinity and resides in a separate domain.
> > Chaitanya, can you try that?
> 
> Hi mani,
> 
> I tried with gic its there also I see same behavior.
> 

Okay

> The only which helps to comment out affinity in the following function.
> 
> diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c
> index 21b3ac2a29d2..042afec1cf9d 100644
> --- a/kernel/irq/irqdesc.c
> +++ b/kernel/irq/irqdesc.c
> @@ -487,8 +487,9 @@ static int alloc_descs(unsigned int start, unsigned int
> cnt, int node,
> 
> 
> 
>                if (affinity) {
>                         if (affinity->is_managed) {
> -                               flags = IRQD_AFFINITY_MANAGED |
> -                                       IRQD_MANAGED_SHUTDOWN;
> +//                             flags = IRQD_AFFINITY_MANAGED |
> +//                                     IRQD_MANAGED_SHUTDOWN;
> +                               flags = 0;//IRQD_AFFINITY_MANAGED |
>                         }
>                         mask = &affinity->mask;
>                         node = cpu_to_node(cpumask_first(mask));
> 

The only solution I can think of is keeping the clocks related to DBI access
active or switch to another clock source that consumes less power if available
during suspend.

But limiting the DBI access using hacks doesn't look good.

Thanks,
Mani

> > > > If there is any other way to remove these calls can you please help us
> > > > point that way.
> > > I'm not sure. I believe genirq assumes the irqchips are always
> > > accessible. There is some support to suspend irqchips. See how the
> > > struct irq_chip::irq_suspend() function is called by syscore ops in the
> > > generic irqchip 'irq_gc_syscore_ops' hooks. Maybe you could add a
> > > syscore suspend/resume hook to disable/enable the clks and power to the
> > > PCI controller. syscore ops run after secondary CPUs are hotplugged out
> > > during suspend.
> > > 
> > > Or maybe setting the IRQCHIP_MASK_ON_SUSPEND flag can be used so that on
> > > irq migration nothing writes the irq hardware because it is already
> > > masked in the hardware earlier. I think the problem is that on resume
> > > we'll restart the irq from the first CPU online event, when you don't
> > > want to do that because it is too early.
> > > 
> > > I have another question though, which is do MSIs support wakeup? I don't
> > > see how it works if the whole bus is effectively off during suspend. If
> > > wakeup needs to be supported then I suspect the bus can't be powered
> > > down during suspend.
> > Wake up should be handled by a dedicated side-band GPIO or in-band PME message.
> > 
> > But I still wonder how the link stays in L1/L1ss when the clocks are disabled
> > and PHY is powered down. Maybe the link or phy is powered by a separate power
> > domain like MX that keeps the link active?
> We will come back to you on this.

Okay.

Thanks,
Mani

> > 
> > Thanks,
> > Mani
> > 
> 

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

Re: [PATCH v5 2/3] PCI: qcom: Restrict pci transactions after pci suspend

[Sai Prakash Ranjan]

On 8/30/2022 5:25 PM, Manivannan Sadhasivam wrote:

<SNIP>...

>> diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c
>> index 21b3ac2a29d2..042afec1cf9d 100644
>> --- a/kernel/irq/irqdesc.c
>> +++ b/kernel/irq/irqdesc.c
>> @@ -487,8 +487,9 @@ static int alloc_descs(unsigned int start, unsigned int
>> cnt, int node,
>>
>>
>>
>>                 if (affinity) {
>>                          if (affinity->is_managed) {
>> -                               flags = IRQD_AFFINITY_MANAGED |
>> -                                       IRQD_MANAGED_SHUTDOWN;
>> +//                             flags = IRQD_AFFINITY_MANAGED |
>> +//                                     IRQD_MANAGED_SHUTDOWN;
>> +                               flags = 0;//IRQD_AFFINITY_MANAGED |
>>                          }
>>                          mask = &affinity->mask;
>>                          node = cpu_to_node(cpumask_first(mask));
>>
> The only solution I can think of is keeping the clocks related to DBI access
> active or switch to another clock source that consumes less power if available
> during suspend.
>
> But limiting the DBI access using hacks doesn't look good.

Why not just define "irq_startup and irq_shutdown" callbacks for dw_pcie_msi_irq_chip?
So when the cpu is offlined and irq_shutdown is called for that irqchip in migrate_one_irq(),
you would mask the irq and then disable the clocks. Similarly, on CPU onlining, you would
enable the clocks and unmask the irq. This way XO is still achieved as you are turning off
the clocks before suspend and back on after resume.

Thanks,
Sai

[PATCH v5 3/3] PCI: qcom: Add retry logic for link to be stable in L1ss

[Krishna chaitanya chundru]

Some specific devices are taking time to settle the link in L1ss.
So added a retry logic before returning from the suspend op.

Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
---
 drivers/pci/controller/dwc/pcie-qcom.c | 25 ++++++++++++++++++++-----
 1 file changed, 20 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index f7dd5dc..f3201bd 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -1829,15 +1829,30 @@ static int __maybe_unused qcom_pcie_pm_suspend(struct device *dev)
 {
 	struct qcom_pcie *pcie = dev_get_drvdata(dev);
 	u32 val;
+	ktime_t timeout, start;
 
 	if (!pcie->cfg->supports_system_suspend)
 		return 0;
 
-	/* if the link is not in l1ss don't turn off clocks */
-	val = readl(pcie->parf + PCIE20_PARF_PM_STTS);
-	if (!(val & PCIE20_PARF_PM_STTS_LINKST_IN_L1SUB)) {
-		dev_warn(dev, "Link is not in L1ss\n");
-		return 0;
+	start = ktime_get();
+	/* Wait max 100 ms */
+	timeout = ktime_add_ms(start, 100);
+	while (1) {
+		bool timedout = ktime_after(ktime_get(), timeout);
+
+		/* if the link is not in l1ss don't turn off clocks */
+		val = readl(pcie->parf + PCIE20_PARF_PM_STTS);
+		if ((val & PCIE20_PARF_PM_STTS_LINKST_IN_L1SUB)) {
+			dev_info(dev, "Link enters L1ss after %d ms\n",
+					ktime_to_ms(ktime_get() - start));
+			break;
+		}
+
+		if (timedout) {
+			dev_warn(dev, "Link is not in L1ss\n");
+			return 0;
+		}
+		usleep_range(1000, 1200);
 	}
 
 	if (pcie->cfg->ops->suspend)
-- 
2.7.4

Re: [PATCH v5 3/3] PCI: qcom: Add retry logic for link to be stable in L1ss

[kernel test robot]

Hi Krishna,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on helgaas-pci/next]
[also build test WARNING on next-20220803]
[cannot apply to linus/master v5.19]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Krishna-chaitanya-chundru/PCI-Restrict-pci-transactions-after-pci-suspend/20220803-193033
base:   https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next
config: mips-allyesconfig (https://download.01.org/0day-ci/archive/20220804/202208041821.cik847re-lkp@intel.com/config)
compiler: mips-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/64b4ad561ad4a28aa8840303f29669bf7af77757
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Krishna-chaitanya-chundru/PCI-Restrict-pci-transactions-after-pci-suspend/20220803-193033
        git checkout 64b4ad561ad4a28aa8840303f29669bf7af77757
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=mips SHELL=/bin/bash drivers/pci/controller/dwc/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   In file included from include/linux/device.h:15,
                    from include/linux/node.h:18,
                    from include/linux/cpu.h:17,
                    from include/linux/of_device.h:5,
                    from drivers/pci/controller/dwc/pcie-qcom.c:20:
   drivers/pci/controller/dwc/pcie-qcom.c: In function 'qcom_pcie_pm_suspend':
>> drivers/pci/controller/dwc/pcie-qcom.c:1846:39: warning: format '%d' expects argument of type 'int', but argument 3 has type 's64' {aka 'long long int'} [-Wformat=]
    1846 |                         dev_info(dev, "Link enters L1ss after %d ms\n",
         |                                       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/dev_printk.h:110:30: note: in definition of macro 'dev_printk_index_wrap'
     110 |                 _p_func(dev, fmt, ##__VA_ARGS__);                       \
         |                              ^~~
   include/linux/dev_printk.h:150:58: note: in expansion of macro 'dev_fmt'
     150 |         dev_printk_index_wrap(_dev_info, KERN_INFO, dev, dev_fmt(fmt), ##__VA_ARGS__)
         |                                                          ^~~~~~~
   drivers/pci/controller/dwc/pcie-qcom.c:1846:25: note: in expansion of macro 'dev_info'
    1846 |                         dev_info(dev, "Link enters L1ss after %d ms\n",
         |                         ^~~~~~~~
   drivers/pci/controller/dwc/pcie-qcom.c:1846:64: note: format string is defined here
    1846 |                         dev_info(dev, "Link enters L1ss after %d ms\n",
         |                                                               ~^
         |                                                                |
         |                                                                int
         |                                                               %lld


vim +1846 drivers/pci/controller/dwc/pcie-qcom.c

  1827	
  1828	static int __maybe_unused qcom_pcie_pm_suspend(struct device *dev)
  1829	{
  1830		struct qcom_pcie *pcie = dev_get_drvdata(dev);
  1831		u32 val;
  1832		ktime_t timeout, start;
  1833	
  1834		if (!pcie->cfg->supports_system_suspend)
  1835			return 0;
  1836	
  1837		start = ktime_get();
  1838		/* Wait max 100 ms */
  1839		timeout = ktime_add_ms(start, 100);
  1840		while (1) {
  1841			bool timedout = ktime_after(ktime_get(), timeout);
  1842	
  1843			/* if the link is not in l1ss don't turn off clocks */
  1844			val = readl(pcie->parf + PCIE20_PARF_PM_STTS);
  1845			if ((val & PCIE20_PARF_PM_STTS_LINKST_IN_L1SUB)) {
> 1846				dev_info(dev, "Link enters L1ss after %d ms\n",
  1847						ktime_to_ms(ktime_get() - start));
  1848				break;
  1849			}
  1850	
  1851			if (timedout) {
  1852				dev_warn(dev, "Link is not in L1ss\n");
  1853				return 0;
  1854			}
  1855			usleep_range(1000, 1200);
  1856		}
  1857	
  1858		if (pcie->cfg->ops->suspend)
  1859			pcie->cfg->ops->suspend(pcie);
  1860	
  1861		pcie->is_suspended = true;
  1862	
  1863		return 0;
  1864	}
  1865	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH v5 3/3] PCI: qcom: Add retry logic for link to be stable in L1ss

[Matthias Kaehlcke]

On Wed, Aug 03, 2022 at 04:58:54PM +0530, Krishna chaitanya chundru wrote:
> Some specific devices are taking time to settle the link in L1ss.
> So added a retry logic before returning from the suspend op.
> 
> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
> ---
>  drivers/pci/controller/dwc/pcie-qcom.c | 25 ++++++++++++++++++++-----
>  1 file changed, 20 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index f7dd5dc..f3201bd 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -1829,15 +1829,30 @@ static int __maybe_unused qcom_pcie_pm_suspend(struct device *dev)
>  {
>  	struct qcom_pcie *pcie = dev_get_drvdata(dev);
>  	u32 val;
> +	ktime_t timeout, start;
>  
>  	if (!pcie->cfg->supports_system_suspend)
>  		return 0;
>  
> -	/* if the link is not in l1ss don't turn off clocks */
> -	val = readl(pcie->parf + PCIE20_PARF_PM_STTS);
> -	if (!(val & PCIE20_PARF_PM_STTS_LINKST_IN_L1SUB)) {
> -		dev_warn(dev, "Link is not in L1ss\n");
> -		return 0;
> +	start = ktime_get();
> +	/* Wait max 100 ms */
> +	timeout = ktime_add_ms(start, 100);

In my tests 100 ms is ample margin for most NVMe models (it's often 0 and
generally < 10), however with one model I saw delays of up to 150 ms, so
this should probably be 200 ms or so (it's a long time, but most of the
time the actual delay is significantly lower

> +	while (1) {
> +		bool timedout = ktime_after(ktime_get(), timeout);

'timedout' looks very similar to the other local variable 'timeout'
in this function. Actually why not just do without the new variable and
put this after reading the register.

   		if (ktime_after(ktime_get(), timeout)) {
			dev_warn(dev, "Link is not in L1ss\n");
 			return 0;
		}

> +
> +		/* if the link is not in l1ss don't turn off clocks */
> +		val = readl(pcie->parf + PCIE20_PARF_PM_STTS);
> +		if ((val & PCIE20_PARF_PM_STTS_LINKST_IN_L1SUB)) {
> +			dev_info(dev, "Link enters L1ss after %d ms\n",
> +					ktime_to_ms(ktime_get() - start));


Probably this should be dev_dbg() to avoid cluttering the kernel log that
isn't relevant most of the time.

> +			break;
> +		}
> +
> +		if (timedout) {
> +			dev_warn(dev, "Link is not in L1ss\n");
> +			return 0;
> +		}
> +		usleep_range(1000, 1200);

You could use fsleep() instead of specifying a range.

Based on my testing I think a slightly higher delay like 5ms wouldn't hurt.
That would result in less 'busy looping' for slower NVMes and would still
be reasonable fast for those that need 10 ms or so.

Actually you could replace the entire loop with something like this:

	if (readl_poll_timeout(pcie->parf + PCIE20_PARF_PM_STTS, val,
	    val & PCIE20_PARF_PM_STTS_LINKST_IN_L1SUB, 5000, 200000) {
	    dev_warn(dev, "Link is not in L1ss\n");
	    return 0;
	}

Re: [PATCH v5 3/3] PCI: qcom: Add retry logic for link to be stable in L1ss

[Krishna Chaitanya Chundru]

On 8/5/2022 3:03 AM, Matthias Kaehlcke wrote:
> On Wed, Aug 03, 2022 at 04:58:54PM +0530, Krishna chaitanya chundru wrote:
>> Some specific devices are taking time to settle the link in L1ss.
>> So added a retry logic before returning from the suspend op.
>>
>> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
>> ---
>>   drivers/pci/controller/dwc/pcie-qcom.c | 25 ++++++++++++++++++++-----
>>   1 file changed, 20 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
>> index f7dd5dc..f3201bd 100644
>> --- a/drivers/pci/controller/dwc/pcie-qcom.c
>> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
>> @@ -1829,15 +1829,30 @@ static int __maybe_unused qcom_pcie_pm_suspend(struct device *dev)
>>   {
>>   	struct qcom_pcie *pcie = dev_get_drvdata(dev);
>>   	u32 val;
>> +	ktime_t timeout, start;
>>   
>>   	if (!pcie->cfg->supports_system_suspend)
>>   		return 0;
>>   
>> -	/* if the link is not in l1ss don't turn off clocks */
>> -	val = readl(pcie->parf + PCIE20_PARF_PM_STTS);
>> -	if (!(val & PCIE20_PARF_PM_STTS_LINKST_IN_L1SUB)) {
>> -		dev_warn(dev, "Link is not in L1ss\n");
>> -		return 0;
>> +	start = ktime_get();
>> +	/* Wait max 100 ms */
>> +	timeout = ktime_add_ms(start, 100);
> In my tests 100 ms is ample margin for most NVMe models (it's often 0 and
> generally < 10), however with one model I saw delays of up to 150 ms, so
> this should probably be 200 ms or so (it's a long time, but most of the
> time the actual delay is significantly lower
ok I will increase the time to 200.
>
>> +	while (1) {
>> +		bool timedout = ktime_after(ktime_get(), timeout);
> 'timedout' looks very similar to the other local variable 'timeout'
> in this function. Actually why not just do without the new variable and
> put this after reading the register.
>
>     		if (ktime_after(ktime_get(), timeout)) {
> 			dev_warn(dev, "Link is not in L1ss\n");
>   			return 0;
> 		}
ok sure will update in the next patch.
>> +
>> +		/* if the link is not in l1ss don't turn off clocks */
>> +		val = readl(pcie->parf + PCIE20_PARF_PM_STTS);
>> +		if ((val & PCIE20_PARF_PM_STTS_LINKST_IN_L1SUB)) {
>> +			dev_info(dev, "Link enters L1ss after %d ms\n",
>> +					ktime_to_ms(ktime_get() - start));
>
> Probably this should be dev_dbg() to avoid cluttering the kernel log that
> isn't relevant most of the time.
ok sure will update in next patch.
>
>> +			break;
>> +		}
>> +
>> +		if (timedout) {
>> +			dev_warn(dev, "Link is not in L1ss\n");
>> +			return 0;
>> +		}
>> +		usleep_range(1000, 1200);
> You could use fsleep() instead of specifying a range.
>
> Based on my testing I think a slightly higher delay like 5ms wouldn't hurt.
> That would result in less 'busy looping' for slower NVMes and would still
> be reasonable fast for those that need 10 ms or so.
>
> Actually you could replace the entire loop with something like this:
>
> 	if (readl_poll_timeout(pcie->parf + PCIE20_PARF_PM_STTS, val,
> 	    val & PCIE20_PARF_PM_STTS_LINKST_IN_L1SUB, 5000, 200000) {
> 	    dev_warn(dev, "Link is not in L1ss\n");
> 	    return 0;
> 	}

Ok we will look in to this option and will update the patch if needed.

Re: [PATCH v5 3/3] PCI: qcom: Add retry logic for link to be stable in L1ss

[Krishna Chaitanya Chundru]

On 8/5/2022 3:03 AM, Matthias Kaehlcke wrote:
> On Wed, Aug 03, 2022 at 04:58:54PM +0530, Krishna chaitanya chundru wrote:
>> Some specific devices are taking time to settle the link in L1ss.
>> So added a retry logic before returning from the suspend op.
>>
>> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
>> ---
>>   drivers/pci/controller/dwc/pcie-qcom.c | 25 ++++++++++++++++++++-----
>>   1 file changed, 20 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
>> index f7dd5dc..f3201bd 100644
>> --- a/drivers/pci/controller/dwc/pcie-qcom.c
>> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
>> @@ -1829,15 +1829,30 @@ static int __maybe_unused qcom_pcie_pm_suspend(struct device *dev)
>>   {
>>   	struct qcom_pcie *pcie = dev_get_drvdata(dev);
>>   	u32 val;
>> +	ktime_t timeout, start;
>>   
>>   	if (!pcie->cfg->supports_system_suspend)
>>   		return 0;
>>   
>> -	/* if the link is not in l1ss don't turn off clocks */
>> -	val = readl(pcie->parf + PCIE20_PARF_PM_STTS);
>> -	if (!(val & PCIE20_PARF_PM_STTS_LINKST_IN_L1SUB)) {
>> -		dev_warn(dev, "Link is not in L1ss\n");
>> -		return 0;
>> +	start = ktime_get();
>> +	/* Wait max 100 ms */
>> +	timeout = ktime_add_ms(start, 100);
> In my tests 100 ms is ample margin for most NVMe models (it's often 0 and
> generally < 10), however with one model I saw delays of up to 150 ms, so
> this should probably be 200 ms or so (it's a long time, but most of the
> time the actual delay is significantly lower
>
>> +	while (1) {
>> +		bool timedout = ktime_after(ktime_get(), timeout);
> 'timedout' looks very similar to the other local variable 'timeout'
> in this function. Actually why not just do without the new variable and
> put this after reading the register.
>
>     		if (ktime_after(ktime_get(), timeout)) {
> 			dev_warn(dev, "Link is not in L1ss\n");
>   			return 0;
> 		}
>
>> +
>> +		/* if the link is not in l1ss don't turn off clocks */
>> +		val = readl(pcie->parf + PCIE20_PARF_PM_STTS);
>> +		if ((val & PCIE20_PARF_PM_STTS_LINKST_IN_L1SUB)) {
>> +			dev_info(dev, "Link enters L1ss after %d ms\n",
>> +					ktime_to_ms(ktime_get() - start));
>
> Probably this should be dev_dbg() to avoid cluttering the kernel log that
> isn't relevant most of the time.
>
>> +			break;
>> +		}
>> +
>> +		if (timedout) {
>> +			dev_warn(dev, "Link is not in L1ss\n");
>> +			return 0;
>> +		}
>> +		usleep_range(1000, 1200);
> You could use fsleep() instead of specifying a range.
>
> Based on my testing I think a slightly higher delay like 5ms wouldn't hurt.
> That would result in less 'busy looping' for slower NVMes and would still
> be reasonable fast for those that need 10 ms or so.
>
> Actually you could replace the entire loop with something like this:
>
> 	if (readl_poll_timeout(pcie->parf + PCIE20_PARF_PM_STTS, val,
> 	    val & PCIE20_PARF_PM_STTS_LINKST_IN_L1SUB, 5000, 200000) {
> 	    dev_warn(dev, "Link is not in L1ss\n");
> 	    return 0;
> 	}

Hi Matthias,

In the v6 patch series we tried to include this logic, but as we are 
going with syscore ops

all the interrupts will be disabled by that time. and this timeout logic 
is enabling interrupts

and this causes the suspend to fail. So going with the previous logic.

Thanks & Regards,

Krishna chaitanya.

Re: [PATCH v5 3/3] PCI: qcom: Add retry logic for link to be stable in L1ss

[kernel test robot]

Hi Krishna,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on helgaas-pci/next]
[also build test WARNING on next-20220804]
[cannot apply to linus/master v5.19]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Krishna-chaitanya-chundru/PCI-Restrict-pci-transactions-after-pci-suspend/20220803-193033
base:   https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next
config: arm64-randconfig-r024-20220804 (https://download.01.org/0day-ci/archive/20220805/202208051112.qnLsiuff-lkp@intel.com/config)
compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project 26dd42705c2af0b8f6e5d6cdb32c9bd5ed9524eb)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install arm64 cross compiling tool for clang build
        # apt-get install binutils-aarch64-linux-gnu
        # https://github.com/intel-lab-lkp/linux/commit/64b4ad561ad4a28aa8840303f29669bf7af77757
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Krishna-chaitanya-chundru/PCI-Restrict-pci-transactions-after-pci-suspend/20220803-193033
        git checkout 64b4ad561ad4a28aa8840303f29669bf7af77757
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm64 SHELL=/bin/bash drivers/pci/controller/dwc/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/pci/controller/dwc/pcie-qcom.c:1847:6: warning: format specifies type 'int' but the argument has type 's64' (aka 'long long') [-Wformat]
                                           ktime_to_ms(ktime_get() - start));
                                           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/dev_printk.h:150:67: note: expanded from macro 'dev_info'
           dev_printk_index_wrap(_dev_info, KERN_INFO, dev, dev_fmt(fmt), ##__VA_ARGS__)
                                                                    ~~~     ^~~~~~~~~~~
   include/linux/dev_printk.h:110:23: note: expanded from macro 'dev_printk_index_wrap'
                   _p_func(dev, fmt, ##__VA_ARGS__);                       \
                                ~~~    ^~~~~~~~~~~
   1 warning generated.


vim +1847 drivers/pci/controller/dwc/pcie-qcom.c

  1827	
  1828	static int __maybe_unused qcom_pcie_pm_suspend(struct device *dev)
  1829	{
  1830		struct qcom_pcie *pcie = dev_get_drvdata(dev);
  1831		u32 val;
  1832		ktime_t timeout, start;
  1833	
  1834		if (!pcie->cfg->supports_system_suspend)
  1835			return 0;
  1836	
  1837		start = ktime_get();
  1838		/* Wait max 100 ms */
  1839		timeout = ktime_add_ms(start, 100);
  1840		while (1) {
  1841			bool timedout = ktime_after(ktime_get(), timeout);
  1842	
  1843			/* if the link is not in l1ss don't turn off clocks */
  1844			val = readl(pcie->parf + PCIE20_PARF_PM_STTS);
  1845			if ((val & PCIE20_PARF_PM_STTS_LINKST_IN_L1SUB)) {
  1846				dev_info(dev, "Link enters L1ss after %d ms\n",
> 1847						ktime_to_ms(ktime_get() - start));
  1848				break;
  1849			}
  1850	
  1851			if (timedout) {
  1852				dev_warn(dev, "Link is not in L1ss\n");
  1853				return 0;
  1854			}
  1855			usleep_range(1000, 1200);
  1856		}
  1857	
  1858		if (pcie->cfg->ops->suspend)
  1859			pcie->cfg->ops->suspend(pcie);
  1860	
  1861		pcie->is_suspended = true;
  1862	
  1863		return 0;
  1864	}
  1865	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH v5 0/3] PCI: Restrict pci transactions after pci suspend

[Bjorn Helgaas]

On Wed, Aug 03, 2022 at 04:58:51PM +0530, Krishna chaitanya chundru wrote:
> If the endpoint device state is D0 and irq's are not freed, then
> kernel try to mask interrupts in system suspend path by writing in to
> the vector table (for MSIX interrupts) and config space (for MSI's).

If clocks are being turned off while the PCI core is still accessing
the device, I think that means qcom suspend is not implemented
correctly.

> These transactions are initiated in the pm suspend after pcie clocks got
> disabled as part of platform driver pm  suspend call. Due to it, these
> transactions are resulting in un-clocked access and eventually to crashes.
> 
> So added a logic in qcom driver to restrict these unclocked access.
> And updated the logic to check the link state before masking
> or unmasking the interrupts.
> 
> And some devices are taking time to settle the link in L1ss, so added a
> retry logic in the suspend ops.
> 
> Krishna chaitanya chundru (3):
>   PCI: qcom: Add system PM support
>   PCI: qcom: Restrict pci transactions after pci suspend
>   PCI: qcom: Add retry logic for link to be stable in L1ss
> 
>  drivers/pci/controller/dwc/pcie-designware-host.c |  14 ++-
>  drivers/pci/controller/dwc/pcie-qcom.c            | 117 +++++++++++++++++++++-
>  2 files changed, 127 insertions(+), 4 deletions(-)
> 
> -- 
> 2.7.4
> 

Re: [PATCH v5 0/3] PCI: Restrict pci transactions after pci suspend

[Krishna Chaitanya Chundru]

On 8/25/2022 1:59 AM, Bjorn Helgaas wrote:
> On Wed, Aug 03, 2022 at 04:58:51PM +0530, Krishna chaitanya chundru wrote:
>> If the endpoint device state is D0 and irq's are not freed, then
>> kernel try to mask interrupts in system suspend path by writing in to
>> the vector table (for MSIX interrupts) and config space (for MSI's).
> If clocks are being turned off while the PCI core is still accessing
> the device, I think that means qcom suspend is not implemented
> correctly.

we are registering the suspend and resume ops as NO_IRQ pm ops and in 
those suspend ops we are disbaling clks there.

NO_IRQ ops is the last the pm ops that are getting called. But we are 
getting pcie access nearly at end of the suspend and near cpu disable.

The pcie access is nothing but the interrupts masks to endpoint to 
disable the interrupts.

>> These transactions are initiated in the pm suspend after pcie clocks got
>> disabled as part of platform driver pm  suspend call. Due to it, these
>> transactions are resulting in un-clocked access and eventually to crashes.
>>
>> So added a logic in qcom driver to restrict these unclocked access.
>> And updated the logic to check the link state before masking
>> or unmasking the interrupts.
>>
>> And some devices are taking time to settle the link in L1ss, so added a
>> retry logic in the suspend ops.
>>
>> Krishna chaitanya chundru (3):
>>    PCI: qcom: Add system PM support
>>    PCI: qcom: Restrict pci transactions after pci suspend
>>    PCI: qcom: Add retry logic for link to be stable in L1ss
>>
>>   drivers/pci/controller/dwc/pcie-designware-host.c |  14 ++-
>>   drivers/pci/controller/dwc/pcie-qcom.c            | 117 +++++++++++++++++++++-
>>   2 files changed, 127 insertions(+), 4 deletions(-)
>>
>> -- 
>> 2.7.4
>>