Regression since 6.1.46 (commit 8ee39ec): rtsx_pci from drivers/misc/cardreader breaks NVME power state, preventing system boot

My computer doesn't boot with kernels newer than 6.1.45.> Here's what happens:
- system boots in initramfs
- detects my encrypted ZFS pool and asks for password
- mount system, pivots to it, starts real init
- before any daemon had time to start, the system hangs and the kernel writes on the console
"nvme 0000:04:00.0: Unable to change power state from D3cold to D0, device inaccessible"
- if I reboot directly without powering off (using magic sysrq or panic=10), even the UEFI complains about not finding any storage to boot from.
- after a real power off, I can boot using a kernel <= 6.1.45.

The bug has been discussed here:
https://bugzilla.kernel.org/show_bug.cgi?id=217705

My laptop is a Dell XPS 15 9560 (Intel 7700hq).

I bisected between 6.1.45 and 6.1.46 and found this commit

commit 8ee39ec479147e29af704639f8e55fce246ed2d9
Author: Ricky WU <[email protected]>
Date:   Tue Jul 25 09:10:54 2023 +0000

   misc: rtsx: judge ASPM Mode to set PETXCFG Reg

   commit 101bd907b4244a726980ee67f95ed9cafab6ff7a upstream.

   ASPM Mode is ASPM_MODE_CFG need to judge the value of clkreq_0 to set HIGH or LOW, if the ASPM Mode is ASPM_MODE_REG always set to HIGH during the initialization.

Cc [email protected]

Signed-off-by Ricky Wu <[email protected]>

   Link:
https://lore.kernel.org/r/[email protected]

Signed-off-by Greg Kroah-Hartman <[email protected]>

drivers/misc/cardreader/rts5227.c  |  2 +-
drivers/misc/cardreader/rts5228.c  | 18 ------------------
drivers/misc/cardreader/rts5249.c  |  3 +--
drivers/misc/cardreader/rts5260.c  | 18 ------------------
drivers/misc/cardreader/rts5261.c  | 18 ------------------
drivers/misc/cardreader/rtsx_pcr.c |  5 ++++-
6 files changed, 6 insertions(+), 58 deletions(-)

If I build 6.1.51 with this commit reverted, my laptop works again, confirming that this commit is to blame.

Also, blacklisting rtsx_pci_sdmmc and rtsx_pci, while preventing to use the sd card reading, allows to boot the system.

I can't try 6.4 or 6.5 because my system is dependent on ZFS..

Have a nice day,
Paul Grandperrin

(CCing Greg, as he merged the culprit, and Linus, in case he wants to revert this from mainline directly as this apparently affects and annoys quite a few people)

-----Original Message-----
From: Greg KH <[email protected]>
Sent: Wednesday, September 13, 2023 1:03 PM
To: Linux regressions mailing list <[email protected]>
Cc: Paul Grandperrin <[email protected]>; [email protected]; Wei_wang <[email protected]>; Roger Tseng <[email protected]>; Ricky WU <[email protected]>; Linus Torvalds <[email protected]>
Subject: Re: Regression since 6.1.46 (commit 8ee39ec): rtsx_pci from drivers/misc/cardreader breaks NVME power state, preventing system boot

External mail.

Quoted 3 lines · Linux regression tracking (Thorsten Leemhuis)

(CCing Greg, as he merged the culprit, and Linus, in case he wants to revert this from mainline directly as this apparently affects and annoys quite a few people)

The driver authors know about this and have said they are working on a solution. Let's give them a few more days on it before reverting stuff.

thanks,

greg k-h

Hi Greg k-h，

In order to cover the those platform Power saving issue,
our approach on new patch will be different from the previous patch (101bd907b4244a726980ee67f95ed9cafab6ff7a).

So we need used fixed Tag on 101bd907b4244a726980ee67f95ed9cafab6ff7a or a new patch for this problem?

-----Original Message-----
From: Greg KH <[email protected]>
Sent: Tuesday, September 19, 2023 3:06 PM
To: Ricky WU <[email protected]>
Cc: Linux regressions mailing list <[email protected]>; Paul Grandperrin <[email protected]>; [email protected]; Wei_wang <[email protected]>; Roger Tseng <[email protected]>; Linus Torvalds <[email protected]>
Subject: Re: Regression since 6.1.46 (commit 8ee39ec): rtsx_pci from drivers/misc/cardreader breaks NVME power state, preventing system boot

External mail.

Quoted 4 lines · Ricky WU

Hi Greg k-h，

In order to cover the those platform Power saving issue, our approach on new patch will be different from the previous patch

(101bd907b4244a726980ee67f95ed9cafab6ff7a).

Quoted 2 lines

So we need used fixed Tag on

101bd907b4244a726980ee67f95ed9cafab6ff7a

Quoted 1 line

or a new patch for this problem?

I'm sorry, but I do not understand. I think a new change is needed here, right? Or are you wanting a different commit backported to stable trees? What about Linus's tree now?

What exactly should I do here?

confused,

greg k-h

Hi Greg k-h,

This patch is our solution for this issue...
And now how can I push this?

Quoted 4 lines

Hi Greg k-h,

This patch is our solution for this issue...
And now how can I push this?

Submit it properly like any other patch, what is preventing that from happening?

thanks,

greg k-h

Quoted 9 lines · Ricky WU

Quoted 4 lines

Hi Greg k-h,

This patch is our solution for this issue...
And now how can I push this?

Submit it properly like any other patch, what is preventing that from happening?

(commit 8ee39ec) some reader no longer force #CLKREQ to low when system need to enter ASPM. But some platform maybe not implement complete ASPM? I don't know..... it causes problems...

Like in the past Only the platform support L1ss we release the #CLKREQ.
But new patch we move the judgment (L1ss) to probe, because we met some host will clean the config space from S3 or some power saving mode And also we think just to read config space one time when the driver start is enough

Quoted 8 lines · Ricky WU

Quoted 7 lines · Ricky WU

Quoted 3 lines

This patch is our solution for this issue...
And now how can I push this?

Submit it properly like any other patch, what is preventing that from happening?

Quoted 6 lines

(commit 8ee39ec) some reader no longer force #CLKREQ to low when system need to enter ASPM. But some platform maybe not implement complete ASPM? I don't know..... it causes problems...

Like in the past Only the platform support L1ss we release the #CLKREQ.
But new patch we move the judgment (L1ss) to probe, because we met some host will clean the config space from S3 or some power saving mode And also we think just to read config space one time when the driver start is enough

Is there a potential fix which is queued for this or would be the safest option to unbreak the regression to revert the commit in the stable trees temporarily?

I'm asking because in Debian we got the report at
https://bugs.debian.org/1052063

(and ideally to unbreak the situation for the user I would like to include a fix in the next upload we do, but following what you as upstream will do ideally).

Hi Greg k-h,

This patch is our solution for this issue...
And now how can I push this?

Hi Paul，

Can you help to try this patch? Is it work for your platform?

Ricky

---
 drivers/misc/cardreader/rts5227.c  | 55 ++++------------------------
 drivers/misc/cardreader/rts5228.c  | 57 
+++++++++---------------------
 drivers/misc/cardreader/rts5249.c  | 56 ++++-------------------------
 drivers/misc/cardreader/rts5260.c  | 43 +++++++---------------
 drivers/misc/cardreader/rts5261.c  | 52 +++++++--------------------
 drivers/misc/cardreader/rtsx_pcr.c | 51 +++++++++++++++++++++++---
 6 files changed, 102 insertions(+), 212 deletions(-)

drivers/misc/cardreader/rts5227.c +5 -48

index 3dae5e3a1697..cd512284bfb3 100644
@@ -83,63 +83,20 @@ static void rts5227_fetch_vendor_settings(struct 
 rtsx_pcr *pcr)
 
 static void rts5227_init_from_cfg(struct rtsx_pcr *pcr)
 {
-	struct pci_dev *pdev = pcr->pci;
-	int l1ss;
-	u32 lval;
 	struct rtsx_cr_option *option = &pcr->option;
 
-	l1ss = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_L1SS);
-	if (!l1ss)
-		return;
-
-	pci_read_config_dword(pdev, l1ss + PCI_L1SS_CTL1, &lval);
-
 	if (CHK_PCI_PID(pcr, 0x522A)) {
-		if (0 == (lval & 0x0F))
-			rtsx_pci_enable_oobs_polling(pcr);
-		else
+		if (rtsx_check_dev_flag(pcr, ASPM_L1_1_EN | ASPM_L1_2_EN
+				| PM_L1_1_EN | PM_L1_2_EN))
 			rtsx_pci_disable_oobs_polling(pcr);
+		else
+			rtsx_pci_enable_oobs_polling(pcr);
 	}
 
-	if (lval & PCI_L1SS_CTL1_ASPM_L1_1)
-		rtsx_set_dev_flag(pcr, ASPM_L1_1_EN);
-	else
-		rtsx_clear_dev_flag(pcr, ASPM_L1_1_EN);
-
-	if (lval & PCI_L1SS_CTL1_ASPM_L1_2)
-		rtsx_set_dev_flag(pcr, ASPM_L1_2_EN);
-	else
-		rtsx_clear_dev_flag(pcr, ASPM_L1_2_EN);
-
-	if (lval & PCI_L1SS_CTL1_PCIPM_L1_1)
-		rtsx_set_dev_flag(pcr, PM_L1_1_EN);
-	else
-		rtsx_clear_dev_flag(pcr, PM_L1_1_EN);
-
-	if (lval & PCI_L1SS_CTL1_PCIPM_L1_2)
-		rtsx_set_dev_flag(pcr, PM_L1_2_EN);
-	else
-		rtsx_clear_dev_flag(pcr, PM_L1_2_EN);
-
 	if (option->ltr_en) {
-		u16 val;
-
-		pcie_capability_read_word(pcr->pci, PCI_EXP_DEVCTL2, &val);
-		if (val & PCI_EXP_DEVCTL2_LTR_EN) {
-			option->ltr_enabled = true;
-			option->ltr_active = true;
+		if (option->ltr_enabled)
 			rtsx_set_ltr_latency(pcr, option->ltr_active_latency);
-		} else {
-			option->ltr_enabled = false;
-		}
 	}
-
-	if (rtsx_check_dev_flag(pcr, ASPM_L1_1_EN | ASPM_L1_2_EN
-				| PM_L1_1_EN | PM_L1_2_EN))
-		option->force_clkreq_0 = false;
-	else
-		option->force_clkreq_0 = true;
-
 }
 

static int rts5227_extra_init_hw(struct rtsx_pcr *pcr)

drivers/misc/cardreader/rts5227.c +1 -1

@@ -195,7 +152,7 @@ static int rts5227_extra_init_hw(struct rtsx_pcr 
 *pcr)
 		}
 	}
 
-	if (option->force_clkreq_0 && pcr->aspm_mode == ASPM_MODE_CFG)
+	if (option->force_clkreq_0)
 		rtsx_pci_add_cmd(pcr, WRITE_REG_CMD, PETXCFG,
 				FORCE_CLKREQ_DELINK_MASK, FORCE_CLKREQ_LOW);

drivers/misc/cardreader/rts5227.c +0 -0

 	else

drivers/misc/cardreader/rts5228.c +6 -40

index f4ab09439da7..0c7f10bcf6f1 100644
@@ -386,59 +386,25 @@ static void rts5228_process_ocp(struct rtsx_pcr 
 *pcr)
 
 static void rts5228_init_from_cfg(struct rtsx_pcr *pcr)
 {
-	struct pci_dev *pdev = pcr->pci;
-	int l1ss;
-	u32 lval;
 	struct rtsx_cr_option *option = &pcr->option;
 
-	l1ss = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_L1SS);
-	if (!l1ss)
-		return;
-
-	pci_read_config_dword(pdev, l1ss + PCI_L1SS_CTL1, &lval);
-
-	if (0 == (lval & 0x0F))
-		rtsx_pci_enable_oobs_polling(pcr);
-	else
+	if (rtsx_check_dev_flag(pcr, ASPM_L1_1_EN | ASPM_L1_2_EN
+				| PM_L1_1_EN | PM_L1_2_EN))
 		rtsx_pci_disable_oobs_polling(pcr);
-
-	if (lval & PCI_L1SS_CTL1_ASPM_L1_1)
-		rtsx_set_dev_flag(pcr, ASPM_L1_1_EN);
-	else
-		rtsx_clear_dev_flag(pcr, ASPM_L1_1_EN);
-
-	if (lval & PCI_L1SS_CTL1_ASPM_L1_2)
-		rtsx_set_dev_flag(pcr, ASPM_L1_2_EN);
-	else
-		rtsx_clear_dev_flag(pcr, ASPM_L1_2_EN);
-
-	if (lval & PCI_L1SS_CTL1_PCIPM_L1_1)
-		rtsx_set_dev_flag(pcr, PM_L1_1_EN);
 	else
-		rtsx_clear_dev_flag(pcr, PM_L1_1_EN);
-
-	if (lval & PCI_L1SS_CTL1_PCIPM_L1_2)
-		rtsx_set_dev_flag(pcr, PM_L1_2_EN);
-	else
-		rtsx_clear_dev_flag(pcr, PM_L1_2_EN);
+		rtsx_pci_enable_oobs_polling(pcr);
 
 	rtsx_pci_write_register(pcr, ASPM_FORCE_CTL, 0xFF, 0);
-	if (option->ltr_en) {
-		u16 val;
 
-		pcie_capability_read_word(pcr->pci, PCI_EXP_DEVCTL2, &val);
-		if (val & PCI_EXP_DEVCTL2_LTR_EN) {
-			option->ltr_enabled = true;
-			option->ltr_active = true;
+	if (option->ltr_en) {
+		if (option->ltr_enabled)
 			rtsx_set_ltr_latency(pcr, option->ltr_active_latency);
-		} else {
-			option->ltr_enabled = false;
-		}
 	}
 }
 
 static int rts5228_extra_init_hw(struct rtsx_pcr *pcr)
 {
+	struct rtsx_cr_option *option = &pcr->option;
 
 	rtsx_pci_write_register(pcr, RTS5228_AUTOLOAD_CFG1,

drivers/misc/cardreader/rts5228.c +11 -0

 			CD_RESUME_EN_MASK, CD_RESUME_EN_MASK);

@@ -469,6 +435,17 @@ static int rts5228_extra_init_hw(struct rtsx_pcr 
 *pcr)
 	else
 		rtsx_pci_write_register(pcr, PETXCFG, 0x30, 0x00);
 
+	/*
+	 * If u_force_clkreq_0 is enabled, CLKREQ# PIN will be forced
+	 * to drive low, and we forcibly request clock.
+	 */
+	if (option->force_clkreq_0)
+		rtsx_pci_write_register(pcr, PETXCFG,
+				 FORCE_CLKREQ_DELINK_MASK, FORCE_CLKREQ_LOW);
+	else
+		rtsx_pci_write_register(pcr, PETXCFG,
+				 FORCE_CLKREQ_DELINK_MASK, FORCE_CLKREQ_HIGH);
+
 	rtsx_pci_write_register(pcr, PWD_SUSPEND_EN, 0xFF, 0xFB);
 

drivers/misc/cardreader/rts5228.c +0 -0

 	if (pcr->rtd3_en) {

drivers/misc/cardreader/rts5249.c +5 -47

index 47ab72a43256..6c81040e18be 100644
@@ -86,64 +86,22 @@ static void 
 rtsx_base_fetch_vendor_settings(struct rtsx_pcr *pcr)
 
 static void rts5249_init_from_cfg(struct rtsx_pcr *pcr)
 {
-	struct pci_dev *pdev = pcr->pci;
-	int l1ss;
 	struct rtsx_cr_option *option = &(pcr->option);
-	u32 lval;
-
-	l1ss = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_L1SS);
-	if (!l1ss)
-		return;
-
-	pci_read_config_dword(pdev, l1ss + PCI_L1SS_CTL1, &lval);
 
 	if (CHK_PCI_PID(pcr, PID_524A) || CHK_PCI_PID(pcr, PID_525A)) {
-		if (0 == (lval & 0x0F))
-			rtsx_pci_enable_oobs_polling(pcr);
-		else
+		if (rtsx_check_dev_flag(pcr, ASPM_L1_1_EN | ASPM_L1_2_EN
+				| PM_L1_1_EN | PM_L1_2_EN))
 			rtsx_pci_disable_oobs_polling(pcr);
+		else
+			rtsx_pci_enable_oobs_polling(pcr);
 	}
 
-
-	if (lval & PCI_L1SS_CTL1_ASPM_L1_1)
-		rtsx_set_dev_flag(pcr, ASPM_L1_1_EN);
-
-	if (lval & PCI_L1SS_CTL1_ASPM_L1_2)
-		rtsx_set_dev_flag(pcr, ASPM_L1_2_EN);
-
-	if (lval & PCI_L1SS_CTL1_PCIPM_L1_1)
-		rtsx_set_dev_flag(pcr, PM_L1_1_EN);
-
-	if (lval & PCI_L1SS_CTL1_PCIPM_L1_2)
-		rtsx_set_dev_flag(pcr, PM_L1_2_EN);
-
 	if (option->ltr_en) {
-		u16 val;
-
-		pcie_capability_read_word(pdev, PCI_EXP_DEVCTL2, &val);
-		if (val & PCI_EXP_DEVCTL2_LTR_EN) {
-			option->ltr_enabled = true;
-			option->ltr_active = true;
+		if (option->ltr_enabled)
 			rtsx_set_ltr_latency(pcr, option->ltr_active_latency);
-		} else {
-			option->ltr_enabled = false;
-		}
 	}
 }
 
-static int rts5249_init_from_hw(struct rtsx_pcr *pcr)
-{
-	struct rtsx_cr_option *option = &(pcr->option);
-
-	if (rtsx_check_dev_flag(pcr, ASPM_L1_1_EN | ASPM_L1_2_EN
-				| PM_L1_1_EN | PM_L1_2_EN))
-		option->force_clkreq_0 = false;
-	else
-		option->force_clkreq_0 = true;
-
-	return 0;
-}
-
 static void rts52xa_force_power_down(struct rtsx_pcr *pcr, u8 
 pm_state, bool runtime)

{

drivers/misc/cardreader/rts5249.c +2 -2

 	/* Set relink_time to 0 */

@@ -276,7 +234,6 @@ static int rts5249_extra_init_hw(struct rtsx_pcr 
 *pcr)
 	struct rtsx_cr_option *option = &(pcr->option);
 
 	rts5249_init_from_cfg(pcr);
-	rts5249_init_from_hw(pcr);
 
 	rtsx_pci_init_cmd(pcr);

@@ -327,11 +284,12 @@ static int rts5249_extra_init_hw(struct 
 rtsx_pcr *pcr)
 		}
 	}
 
+
 	/*
 	 * If u_force_clkreq_0 is enabled, CLKREQ# PIN will be forced
 	 * to drive low, and we forcibly request clock.
 	 */
-	if (option->force_clkreq_0 && pcr->aspm_mode == ASPM_MODE_CFG)
+	if (option->force_clkreq_0)
 		rtsx_pci_write_register(pcr, PETXCFG,
 			FORCE_CLKREQ_DELINK_MASK, FORCE_CLKREQ_LOW);

drivers/misc/cardreader/rts5249.c +0 -0

 	else

drivers/misc/cardreader/rts5260.c +2 -30

index 79b18f6f73a8..d2d3a6ccb8f7 100644
@@ -480,47 +480,19 @@ static void rts5260_pwr_saving_setting(struct 
 rtsx_pcr *pcr)
 
 static void rts5260_init_from_cfg(struct rtsx_pcr *pcr)
 {
-	struct pci_dev *pdev = pcr->pci;
-	int l1ss;
 	struct rtsx_cr_option *option = &pcr->option;
-	u32 lval;
-
-	l1ss = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_L1SS);
-	if (!l1ss)
-		return;
-
-	pci_read_config_dword(pdev, l1ss + PCI_L1SS_CTL1, &lval);
-
-	if (lval & PCI_L1SS_CTL1_ASPM_L1_1)
-		rtsx_set_dev_flag(pcr, ASPM_L1_1_EN);
-
-	if (lval & PCI_L1SS_CTL1_ASPM_L1_2)
-		rtsx_set_dev_flag(pcr, ASPM_L1_2_EN);
-
-	if (lval & PCI_L1SS_CTL1_PCIPM_L1_1)
-		rtsx_set_dev_flag(pcr, PM_L1_1_EN);
-
-	if (lval & PCI_L1SS_CTL1_PCIPM_L1_2)
-		rtsx_set_dev_flag(pcr, PM_L1_2_EN);
 
 	rts5260_pwr_saving_setting(pcr);
 
 	if (option->ltr_en) {
-		u16 val;
-
-		pcie_capability_read_word(pdev, PCI_EXP_DEVCTL2, &val);
-		if (val & PCI_EXP_DEVCTL2_LTR_EN) {
-			option->ltr_enabled = true;
-			option->ltr_active = true;
+		if (option->ltr_enabled)
 			rtsx_set_ltr_latency(pcr, option->ltr_active_latency);
-		} else {
-			option->ltr_enabled = false;
-		}
 	}
 }
 
 static int rts5260_extra_init_hw(struct rtsx_pcr *pcr)
 {
+	struct rtsx_cr_option *option = &pcr->option;
 
 	/* Set mcu_cnt to 7 to ensure data can be sampled properly */

drivers/misc/cardreader/rts5260.c +11 -0

 	rtsx_pci_write_register(pcr, 0xFC03, 0x7F, 0x07);

@@ -539,6 +511,17 @@ static int rts5260_extra_init_hw(struct rtsx_pcr 
 *pcr)
 
 	rts5260_init_hw(pcr);
 
+	/*
+	 * If u_force_clkreq_0 is enabled, CLKREQ# PIN will be forced
+	 * to drive low, and we forcibly request clock.
+	 */
+	if (option->force_clkreq_0)
+		rtsx_pci_write_register(pcr, PETXCFG,
+				 FORCE_CLKREQ_DELINK_MASK, FORCE_CLKREQ_LOW);
+	else
+		rtsx_pci_write_register(pcr, PETXCFG,
+				 FORCE_CLKREQ_DELINK_MASK, FORCE_CLKREQ_HIGH);
+
 	rtsx_pci_write_register(pcr, pcr->reg_pm_ctrl3, 0x10, 0x00);
 

drivers/misc/cardreader/rts5260.c +0 -0

 	return 0;

drivers/misc/cardreader/rts5261.c +2 -39

index 94af6bf8a25a..67252512a132 100644
@@ -454,54 +454,17 @@ static void rts5261_init_from_hw(struct 
 rtsx_pcr *pcr)
 
 static void rts5261_init_from_cfg(struct rtsx_pcr *pcr)
 {
-	struct pci_dev *pdev = pcr->pci;
-	int l1ss;
-	u32 lval;
 	struct rtsx_cr_option *option = &pcr->option;
 
-	l1ss = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_L1SS);
-	if (!l1ss)
-		return;
-
-	pci_read_config_dword(pdev, l1ss + PCI_L1SS_CTL1, &lval);
-
-	if (lval & PCI_L1SS_CTL1_ASPM_L1_1)
-		rtsx_set_dev_flag(pcr, ASPM_L1_1_EN);
-	else
-		rtsx_clear_dev_flag(pcr, ASPM_L1_1_EN);
-
-	if (lval & PCI_L1SS_CTL1_ASPM_L1_2)
-		rtsx_set_dev_flag(pcr, ASPM_L1_2_EN);
-	else
-		rtsx_clear_dev_flag(pcr, ASPM_L1_2_EN);
-
-	if (lval & PCI_L1SS_CTL1_PCIPM_L1_1)
-		rtsx_set_dev_flag(pcr, PM_L1_1_EN);
-	else
-		rtsx_clear_dev_flag(pcr, PM_L1_1_EN);
-
-	if (lval & PCI_L1SS_CTL1_PCIPM_L1_2)
-		rtsx_set_dev_flag(pcr, PM_L1_2_EN);
-	else
-		rtsx_clear_dev_flag(pcr, PM_L1_2_EN);
-
-	rtsx_pci_write_register(pcr, ASPM_FORCE_CTL, 0xFF, 0);
 	if (option->ltr_en) {
-		u16 val;
-
-		pcie_capability_read_word(pdev, PCI_EXP_DEVCTL2, &val);
-		if (val & PCI_EXP_DEVCTL2_LTR_EN) {
-			option->ltr_enabled = true;
-			option->ltr_active = true;
+		if (option->ltr_enabled)
 			rtsx_set_ltr_latency(pcr, option->ltr_active_latency);
-		} else {
-			option->ltr_enabled = false;
-		}
 	}
 }
 
 static int rts5261_extra_init_hw(struct rtsx_pcr *pcr)
 {
+	struct rtsx_cr_option *option = &pcr->option;
 	u32 val;
 

drivers/misc/cardreader/rts5261.c +11 -0

 	rtsx_pci_write_register(pcr, RTS5261_AUTOLOAD_CFG1,

@@ -547,6 +510,17 @@ static int rts5261_extra_init_hw(struct rtsx_pcr 
 *pcr)
 	else
 		rtsx_pci_write_register(pcr, PETXCFG, 0x30, 0x00);
 
+	/*
+	 * If u_force_clkreq_0 is enabled, CLKREQ# PIN will be forced
+	 * to drive low, and we forcibly request clock.
+	 */
+	if (option->force_clkreq_0)
+		rtsx_pci_write_register(pcr, PETXCFG,
+				 FORCE_CLKREQ_DELINK_MASK, FORCE_CLKREQ_LOW);
+	else
+		rtsx_pci_write_register(pcr, PETXCFG,
+				 FORCE_CLKREQ_DELINK_MASK, FORCE_CLKREQ_HIGH);
+
 	rtsx_pci_write_register(pcr, PWD_SUSPEND_EN, 0xFF, 0xFB);
 

drivers/misc/cardreader/rts5261.c +0 -0

 	if (pcr->rtd3_en) {

drivers/misc/cardreader/rtsx_pcr.c +1 -4

index a3f4b52bb159..a30751ad3733 100644
@@ -1326,11 +1326,8 @@ static int rtsx_pci_init_hw(struct rtsx_pcr 
 *pcr)
 			return err;
 	}
 
-	if (pcr->aspm_mode == ASPM_MODE_REG) {
+	if (pcr->aspm_mode == ASPM_MODE_REG)
 		rtsx_pci_write_register(pcr, ASPM_FORCE_CTL, 0x30, 0x30);
-		rtsx_pci_write_register(pcr, PETXCFG,
-				FORCE_CLKREQ_DELINK_MASK, FORCE_CLKREQ_HIGH);
-	}
 
 	/* No CD interrupt if probing driver with card inserted.

drivers/misc/cardreader/rtsx_pcr.c +45 -2

 	 * So we need to initialize pcr->card_exist here.

@@ -1345,7 +1342,9 @@ static int rtsx_pci_init_hw(struct rtsx_pcr 
 *pcr)
 
 static int rtsx_pci_init_chip(struct rtsx_pcr *pcr)
 {
-	int err;
+	struct rtsx_cr_option *option = &(pcr->option);
+	int err, l1ss;
+	u32 lval;
 	u16 cfg_val;
 	u8 val;

@@ -1430,6 +1429,48 @@ static int rtsx_pci_init_chip(struct rtsx_pcr 
 *pcr)
 			pcr->aspm_enabled = true;
 	}
 
+	l1ss = pci_find_ext_capability(pcr->pci, PCI_EXT_CAP_ID_L1SS);
+	if (l1ss) {
+		pci_read_config_dword(pcr->pci, l1ss + PCI_L1SS_CTL1, &lval);
+
+		if (lval & PCI_L1SS_CTL1_ASPM_L1_1)
+			rtsx_set_dev_flag(pcr, ASPM_L1_1_EN);
+		else
+			rtsx_clear_dev_flag(pcr, ASPM_L1_1_EN);
+
+		if (lval & PCI_L1SS_CTL1_ASPM_L1_2)
+			rtsx_set_dev_flag(pcr, ASPM_L1_2_EN);
+		else
+			rtsx_clear_dev_flag(pcr, ASPM_L1_2_EN);
+
+		if (lval & PCI_L1SS_CTL1_PCIPM_L1_1)
+			rtsx_set_dev_flag(pcr, PM_L1_1_EN);
+		else
+			rtsx_clear_dev_flag(pcr, PM_L1_1_EN);
+
+		if (lval & PCI_L1SS_CTL1_PCIPM_L1_2)
+			rtsx_set_dev_flag(pcr, PM_L1_2_EN);
+		else
+			rtsx_clear_dev_flag(pcr, PM_L1_2_EN);
+
+		pcie_capability_read_word(pcr->pci, PCI_EXP_DEVCTL2, &cfg_val);
+		if (cfg_val & PCI_EXP_DEVCTL2_LTR_EN) {
+			option->ltr_enabled = true;
+			option->ltr_active = true;
+		} else {
+			option->ltr_enabled = false;
+		}
+
+		if (rtsx_check_dev_flag(pcr, ASPM_L1_1_EN | ASPM_L1_2_EN
+				| PM_L1_1_EN | PM_L1_2_EN))
+			option->force_clkreq_0 = false;
+		else
+			option->force_clkreq_0 = true;
+	} else {
+		option->ltr_enabled = false;
+		option->force_clkreq_0 = true;
+	}
+
 	if (pcr->ops->fetch_vendor_settings)
 		pcr->ops->fetch_vendor_settings(pcr);
 
--

2.25.1

Quoted 42 lines

-----Original Message-----
From: Greg KH <[email protected]>
Sent: Tuesday, September 19, 2023 3:06 PM
To: Ricky WU <[email protected]>
Cc: Linux regressions mailing list <[email protected]>;
Paul
Grandperrin <[email protected]>; [email protected]; Wei_wang <[email protected]>; Roger Tseng <[email protected]>; Linus Torvalds
<[email protected]>
Subject: Re: Regression since 6.1.46 (commit 8ee39ec): rtsx_pci from drivers/misc/cardreader breaks NVME power state, preventing system
boot

External mail.

Quoted 3 lines · Ricky WU

Hi Greg k-h，

In order to cover the those platform Power saving issue, our

approach

Quoted 1 line

on new patch will be different from the previous patch

(101bd907b4244a726980ee67f95ed9cafab6ff7a).

Quoted 2 lines

So we need used fixed Tag on

101bd907b4244a726980ee67f95ed9cafab6ff7a

Quoted 1 line

or a new patch for this problem?

I'm sorry, but I do not understand. I think a new change is needed
here, right?
Or are you wanting a different commit backported to stable trees?
What
about Linus's tree now?

What exactly should I do here?

confused,

greg k-h

-----Original Message-----
From: Paul Grandperrin <[email protected]>
Sent: Tuesday, September 12, 2023 8:29 PM
To: [email protected]
Cc: [email protected]; Wei_wang <[email protected]>; Roger Tseng <[email protected]>; Ricky WU <[email protected]>
Subject: Regression since 6.1.46 (commit 8ee39ec): rtsx_pci from drivers/misc/cardreader breaks NVME power state, preventing system boot

External mail.

Hi kernel maintainers!

My computer doesn't boot with kernels newer than 6.1.45.

Here's what happens:
- system boots in initramfs
- detects my encrypted ZFS pool and asks for password
- mount system, pivots to it, starts real init
- before any daemon had time to start, the system hangs and the kernel writes on the console
"nvme 0000:04:00.0: Unable to change power state from D3cold to D0, device inaccessible"
- if I reboot directly without powering off (using magic sysrq or panic=10), even the UEFI complains about not finding any storage to boot from.
- after a real power off, I can boot using a kernel <= 6.1.45.

The bug has been discussed here:
https://bugzilla.kernel.org/show_bug.cgi?id=217705

My laptop is a Dell XPS 15 9560 (Intel 7700hq).

I bisected between 6.1.45 and 6.1.46 and found this commit

commit 8ee39ec479147e29af704639f8e55fce246ed2d9

Author: Ricky WU <[email protected]>
Date:   Tue Jul 25 09:10:54 2023 +0000

misc: rtsx: judge ASPM Mode to set PETXCFG Reg

commit 101bd907b4244a726980ee67f95ed9cafab6ff7a upstream.

ASPM Mode is ASPM_MODE_CFG need to judge the value of clkreq_0 to set HIGH or LOW, if the ASPM Mode is ASPM_MODE_REG always set to HIGH during the initialization.

Cc [email protected]

Signed-off-by Ricky Wu <[email protected]>

Link:
https://lore.kernel.org/r/[email protected]

Signed-off-by Greg Kroah-Hartman <[email protected]>

 drivers/misc/cardreader/rts5227.c  |  2 +-
 drivers/misc/cardreader/rts5228.c  | 18 ------------------
 drivers/misc/cardreader/rts5249.c  |  3 +--
 drivers/misc/cardreader/rts5260.c  | 18 ------------------
 drivers/misc/cardreader/rts5261.c  | 18 ------------------
 drivers/misc/cardreader/rtsx_pcr.c |  5 ++++-
 6 files changed, 6 insertions(+), 58 deletions(-)

If I build 6.1.51 with this commit reverted, my laptop works again, confirming that this commit is to blame.

Also, blacklisting rtsx_pci_sdmmc and rtsx_pci, while preventing to use the sd card reading, allows to boot the system.

I can't try 6.4 or 6.5 because my system is dependent on ZFS..

Have a nice day,
Paul Grandperrin

In the past if the BIOS(config space) not set L1-substate our driver will keep drive low CLKREQ# when HOST want to enter power saving state that make whole system not enter the power saving state. But this patch we release the CLKREQ# to HOST, make whole system can enter power saving state success when the HOST want to enter the power saving state, but I don't know why this system can not wake out success from power saving state"

This is a PCIE CLKREQ# design problem on those platform, the pcie spec allow device release the CLKREQ# to HOST, this patch only do this....

Enable Clock Power Management – Applicable only for Upstream Ports and with form factors that support a “Clock Request” (CLKREQ#) mechanism, this bit operates as follows:
0b Clock power management is disabled and device must hold CLKREQ# signal low.
1b When this bit is Set, the device is permitted to use CLKREQ# signal to power manage Link clock
according to protocol defined in appropriate form factor specification.

Quoted 2 lines

In the past if the BIOS(config space) not set L1-substate our driver will keep

drive low CLKREQ# when HOST want to enter power saving state that make whole system not enter the power saving state.

Quoted 1 line

But this patch we release the CLKREQ# to HOST, make whole system can

enter power saving state success when the HOST want to enter the power saving state, but I don't know why this system can not wake out success from power saving state"

Quoted 3 lines

This is a PCIE CLKREQ# design problem on those platform, the pcie spec

allow device release the CLKREQ# to HOST, this patch only do this....

I spent some time debugging today but I am not a PCIe expert. I think

that the card reader is actually violating the PCIe spec by not forcing

CLKREQ# low on systems that don't support ASPM, as appears to be done

(accidentally?) by the regressing driver change.

The kernel logs on the affected system states the following:

[    0.142326] ACPI FADT declares the system doesn't support PCIe ASPM, so

disable it

The PCIe 3.0 spec states (in the description of the Link Control

Register), regarding enabling clock power management:

Quoted 1 line

Enable Clock Power Management – Applicable only for Upstream Ports and

with form factors that support a “Clock Request” (CLKREQ#) mechanism, this bit operates as follows:

Quoted 1 line

0b Clock power management is disabled and device must hold CLKREQ#

signal low.

Quoted 1 line

1b When this bit is Set, the device is permitted to use CLKREQ# signal to

power manage Link clock

Quoted 1 line

according to protocol defined in appropriate form factor specification.

My reading of this is that on this system which does not support ASPM

and therefore also does not support clock PM, the driver must have the

device hold the line low, but I may be wrong.

It's still unclear to me based on studying the schematic of the laptop

and the PCH datasheet why the one PCIe port is able to break the other

one like it does. The CLKREQ# lines are simply connected directly to the

SRCCLKREQ# lines of the PCH, plus a 10k pull-up to 3v3, which seems

entirely reasonable; any breakage surely would be some software/firmware level

misconfiguration.

Jade