RE: [bug report] nvme not re-recognize when changed M.2 SSD in S3 mode

Ricky WU
2024-05-29T07:48:04Z

Hi Lukas,

We use SDEX card replace M.2 nvme card because we don't have enough M.2 nvme card
We tried this patch as below situation:
1.keep the SDEX card enter S3 then resume ->PASS
the video can continue playing, not see the msg "device replace during system sleep"

2. on S3 mode insert the SDEX card then resume -> PASS
Can identify the card and can see the msg "device replace during system sleep"

3.on S3 mode remove the SDEX card then resume -> PASS
No card show on system and can see the msg "device replace during system sleep"

4.replace card on S3 mode (different brand) ->PASS
Can identify the second card and can see the msg "device replace during system sleep"

5.replace card on S3 mode (same brand and same capacity) ->FAIL
Can not see the msg "device replace during system sleep"
Against 5 we found a issue, most card not declare capability "PCI_EXT_CAP_ID_DSN",
even if there is the capability the config space value are 0, cause pci_get_dsn() return 0 normally.
However these cards can show the SN on CrystalDiskInfo.

below is the card pci config space log and lsblk disk info.
Sandisk Corp Device 5007 (prog-if 02 [NVM Express]) this card can see the "PCI_EXT_CAP_ID_DSN" capability
But value is 0
--------------------------------------------------------------------------------------------------------------------
cr@cr-desktop:~$ sudo lspci -s 02:00.0 -vvv
02:00.0 Non-Volatile memory controller: Sandisk Corp Device 5007 (prog-if 02 [NVM Express])
Subsystem: Sandisk Corp Device 5007
Physical Slot: 8
Control: I/O- Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx+
Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
Latency: 0
Interrupt: pin A routed to IRQ 16
Region 0: Memory at a3b00000 (64-bit, non-prefetchable) [size=16K]
Region 4: Memory at a3b04000 (64-bit, non-prefetchable) [size=256]
Capabilities: [80] Power Management version 3
Flags: PMEClk- DSI- D1- D2- AuxCurrent=0mA PME(D0-,D1-,D2-,D3hot-,D3cold-)
Status: D0 NoSoftRst+ PME-Enable- DSel=0 DScale=0 PME-
Capabilities: [90] MSI: Enable- Count=1/32 Maskable- 64bit+
Address: 0000000000000000 Data: 0000
Capabilities: [b0] MSI-X: Enable+ Count=17 Masked-
Vector table: BAR=0 offset=00002000
PBA: BAR=4 offset=00000000
Capabilities: [c0] Express (v2) Endpoint, MSI 00
DevCap: MaxPayload 512 bytes, PhantFunc 0, Latency L0s <1us, L1 unlimited
ExtTag- AttnBtn- AttnInd- PwrInd- RBE+ FLReset+ SlotPowerLimit 0.000W
DevCtl: CorrErr+ NonFatalErr+ FatalErr+ UnsupReq+
RlxdOrd+ ExtTag- PhantFunc- AuxPwr- NoSnoop+ FLReset-
MaxPayload 256 bytes, MaxReadReq 512 bytes
DevSta: CorrErr- NonFatalErr- FatalErr- UnsupReq- AuxPwr- TransPend-
LnkCap: Port #0, Speed 8GT/s, Width x1, ASPM L1, Exit Latency L1 <8us
ClockPM+ Surprise- LLActRep- BwNot- ASPMOptComp+
LnkCtl: ASPM L1 Enabled; RCB 64 bytes Disabled- CommClk+
ExtSynch- ClockPM+ AutWidDis- BWInt- AutBWInt-
LnkSta: Speed 8GT/s (ok), Width x1 (ok)
TrErr- Train- SlotClk+ DLActive- BWMgmt- ABWMgmt-
DevCap2: Completion Timeout: Range B, TimeoutDis+, NROPrPrP-, LTR+
10BitTagComp-, 10BitTagReq-, OBFF Not Supported, ExtFmt+, EETLPPrefix-
EmergencyPowerReduction Not Supported, EmergencyPowerReductionInit-
FRS-, TPHComp-, ExtTPHComp-
AtomicOpsCap: 32bit- 64bit- 128bitCAS-
DevCtl2: Completion Timeout: 50us to 50ms, TimeoutDis-, LTR+, OBFF Disabled
AtomicOpsCtl: ReqEn-
LnkCtl2: Target Link Speed: 8GT/s, EnterCompliance- SpeedDis-
Transmit Margin: Normal Operating Range, EnterModifiedCompliance- ComplianceSOS-
Compliance De-emphasis: -6dB
LnkSta2: Current De-emphasis Level: -6dB, EqualizationComplete+, EqualizationPhase1+
EqualizationPhase2+, EqualizationPhase3+, LinkEqualizationRequest-
Capabilities: [100 v2] Advanced Error Reporting
UESta: DLP- SDES- TLP- FCP- CmpltTO- CmpltAbrt- UnxCmplt- RxOF- MalfTLP- ECRC- UnsupReq- ACSViol-
UEMsk: DLP- SDES- TLP- FCP- CmpltTO- CmpltAbrt- UnxCmplt- RxOF- MalfTLP- ECRC- UnsupReq- ACSViol-
UESvrt: DLP+ SDES+ TLP- FCP+ CmpltTO- CmpltAbrt- UnxCmplt- RxOF+ MalfTLP+ ECRC- UnsupReq- ACSViol-
CESta: RxErr- BadTLP- BadDLLP- Rollover- Timeout- AdvNonFatalErr-
CEMsk: RxErr- BadTLP- BadDLLP- Rollover- Timeout- AdvNonFatalErr+
AERCap: First Error Pointer: 00, ECRCGenCap+ ECRCGenEn- ECRCChkCap+ ECRCChkEn-
MultHdrRecCap- MultHdrRecEn- TLPPfxPres- HdrLogCap-
HeaderLog: 00000000 00000000 00000000 00000000
Capabilities: [150 v1] Device Serial Number 00-00-00-00-00-00-00-00
Capabilities: [1b8 v1] Latency Tolerance Reporting
Max snoop latency: 3145728ns
Max no snoop latency: 3145728ns
Capabilities: [300 v1] Secondary PCI Express
LnkCtl3: LnkEquIntrruptEn-, PerformEqu-
LaneErrStat: 0
Capabilities: [900 v1] L1 PM Substates
L1SubCap: PCI-PM_L1.2+ PCI-PM_L1.1- ASPM_L1.2+ ASPM_L1.1- L1_PM_Substates+
PortCommonModeRestoreTime=32us PortTPowerOnTime=10us
L1SubCtl1: PCI-PM_L1.2+ PCI-PM_L1.1- ASPM_L1.2+ ASPM_L1.1-
T_CommonMode=0us LTR1.2_Threshold=90112ns
L1SubCtl2: T_PwrOn=44us
Kernel driver in use: nvme
Kernel modules: nvme
--------------------------------------------------------------------------------------------------------------------

Here can see the Serial Number is "03969d74164000000000"
--------------------------------------------------------------------------------------------------------------------
cr@cr-desktop:~$ lsblk --nodeps -o name,size,type,mountpoint,serial
NAME SIZE TYPE MOUNTPOINT SERIAL
loop0 4K loop /snap/bare/5
loop1 55.7M loop /snap/core18/2785
loop2 55.7M loop /snap/core18/2790
loop3 63.5M loop /snap/core20/1974
loop4 91.7M loop /snap/gtk-common-themes/1535
loop5 53.3M loop /snap/snapd/19457
loop6 485.5M loop /snap/gnome-42-2204/120
loop7 65.1M loop /snap/gtk-common-themes/1515
loop8 73.9M loop /snap/core22/858
loop9 12.3M loop /snap/snap-store/959
loop10 485.5M loop /snap/gnome-42-2204/126
loop11 219M loop /snap/gnome-3-34-1804/72
loop12 51M loop /snap/snap-store/547
loop13 218.4M loop /snap/gnome-3-34-1804/93
loop14 63.9M loop /snap/sublime-text/122
loop15 321.1M loop /snap/vlc/3721
loop16 65.2M loop /snap/sublime-text/118
sda 232.9G disk 210350450505
nvme0n1 236.1G disk 03969d74164000000000
--------------------------------------------------------------------------------------------------------------------

we tried the 4 SDEX cards and 2 M.2 Nvme,
either it is not declared or the value is 0 all the cards I have.

RIcky

> Below please find a reworked patch which checks the Device Serial Number in
> addition to various other device identity information in config space.
>
> I've moved the check for a replaced device to the ->resume_noirq phase, i.e.
> before the device driver's first access to the device.
> I'm also marking the device removed to prevent the driver from accessing it.
>
> Does this fix the issue for you?
>
> If it does reliably detect a replaced device, could you also double-check the
> opposite case, i.e. if the device is *not* replaced during system sleep?
>
> I think this is about as much as we can do at the PCI layer to detect a replaced
> device. Drivers may have additional ways to identify a device (such as
> reading a WWID from some register) and we could consider providing a library
> function which drivers could call if they detect a replaced device on resume.
>
> If you set CONFIG_DYNAMIC_DEBUG=y and add the following to the command
> line...
>
> pciehp.pciehp_debug=1 dyndbg="file pciehp* +p" log_buf_len=10M
> ignore_loglevel
>
> ...you should see "device replaced during system sleep" messages on resume if
> a replaced device is detected.
>
> -- >8 --
>
> From: Lukas Wunner <lukas@wunner.de>
> Subject: [PATCH] PCI: pciehp: Detect device replacement during system sleep
>
> Ricky reports that replacing a device in a hotplug slot during ACPI sleep state
> S3 does not cause re-enumeration on resume, as one would expect. Instead,
> the new device is treated as if it was the old one.
>
> There is no bulletproof way to detect device replacement, but as a heuristic,
> check whether the device identity in config space matches cached data in
> struct pci_dev (Vendor ID, Device ID, Class Code, Revision ID, Subsystem
> Vendor ID, Subsystem ID). Additionally, cache and compare the Device Serial
> Number (PCIe r6.2 sec 7.9.3). If a mismatch is detected, mark the old device
> disconnected (to prevent its driver from accessing the new device) and
> synthesize a Presence Detect Changed event.
>
> Reported-by: Ricky Wu <ricky_wu@realtek.com>
> Closes:
> https://lore.kernel.org/r/a608b5930d0a48f092f717c0e137454b@realtek.com
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> ---
> drivers/pci/hotplug/pciehp.h | 4 ++++
> drivers/pci/hotplug/pciehp_core.c | 42
> ++++++++++++++++++++++++++++++++++++++-
> drivers/pci/hotplug/pciehp_hpc.c | 5 +++++
> drivers/pci/hotplug/pciehp_pci.c | 4 ++++
> 4 files changed, 54 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pci/hotplug/pciehp.h b/drivers/pci/hotplug/pciehp.h index
> e0a614a..273dd8c 100644
> --- a/drivers/pci/hotplug/pciehp.h
> +++ b/drivers/pci/hotplug/pciehp.h
> @@ -46,6 +46,9 @@
> /**
> * struct controller - PCIe hotplug controller
> * @pcie: pointer to the controller's PCIe port service device
> + * @dsn: cached copy of Device Serial Number of Function 0 in the hotplug
> slot
> + * (PCIe r6.2 sec 7.9.3); used to determine whether a hotplugged device
> + * was replaced with a different one during system sleep
> * @slot_cap: cached copy of the Slot Capabilities register
> * @inband_presence_disabled: In-Band Presence Detect Disable supported
> by
> * controller and disabled per spec recommendation (PCIe r5.0,
> appendix I
> @@ -87,6 +90,7 @@
> */
> struct controller {
> struct pcie_device *pcie;
> + u64 dsn;
>
> u32 slot_cap; /* capabilities and
> quirks */
> unsigned int inband_presence_disabled:1; diff --git
> a/drivers/pci/hotplug/pciehp_core.c b/drivers/pci/hotplug/pciehp_core.c
> index ddd55ad..ff458e6 100644
> --- a/drivers/pci/hotplug/pciehp_core.c
> +++ b/drivers/pci/hotplug/pciehp_core.c
> @@ -284,6 +284,32 @@ static int pciehp_suspend(struct pcie_device *dev)
> return 0;
> }
>
> +static bool pciehp_device_replaced(struct controller *ctrl) {
> + struct pci_dev *pdev __free(pci_dev_put);
> + u32 reg;
> +
> + pdev = pci_get_slot(ctrl->pcie->port->subordinate, PCI_DEVFN(0, 0));
> + if (!pdev)
> + return true;
> +
> + if (pci_read_config_dword(pdev, PCI_VENDOR_ID, &reg) ||
> + reg != (pdev->vendor | (pdev->device << 16)) ||
> + pci_read_config_dword(pdev, PCI_CLASS_REVISION, &reg) ||
> + reg != (pdev->revision | (pdev->class << 8)))
> + return true;
> +
> + if (pdev->hdr_type == PCI_HEADER_TYPE_NORMAL &&
> + (pci_read_config_dword(pdev, PCI_SUBSYSTEM_VENDOR_ID,
> &reg) ||
> + reg != (pdev->subsystem_vendor | (pdev->subsystem_device
> << 16))))
> + return true;
> +
> + if (pci_get_dsn(pdev) != ctrl->dsn)
> + return true;
> +
> + return false;
> +}
> +
> static int pciehp_resume_noirq(struct pcie_device *dev) {
> struct controller *ctrl = get_service_data(dev); @@ -293,9 +319,23
> @@ static int pciehp_resume_noirq(struct pcie_device *dev)
> ctrl->cmd_busy = true;
>
> /* clear spurious events from rediscovery of inserted card */
> - if (ctrl->state == ON_STATE || ctrl->state == BLINKINGOFF_STATE)
> + if (ctrl->state == ON_STATE || ctrl->state == BLINKINGOFF_STATE)
> + {
> pcie_clear_hotplug_events(ctrl);
>
> + /*
> + * If hotplugged device was replaced with a different one
> + * during system sleep, mark the old device disconnected
> + * (to prevent its driver from accessing the new device)
> + * and synthesize a Presence Detect Changed event.
> + */
> + if (pciehp_device_replaced(ctrl)) {
> + ctrl_dbg(ctrl, "device replaced during system
> sleep\n");
> + pci_walk_bus(ctrl->pcie->port->subordinate,
> + pci_dev_set_disconnected,
> NULL);
> + pciehp_request(ctrl, PCI_EXP_SLTSTA_PDC);
> + }
> + }
> +
> return 0;
> }
> #endif
> diff --git a/drivers/pci/hotplug/pciehp_hpc.c
> b/drivers/pci/hotplug/pciehp_hpc.c
> index b1d0a1b3..061f01f 100644
> --- a/drivers/pci/hotplug/pciehp_hpc.c
> +++ b/drivers/pci/hotplug/pciehp_hpc.c
> @@ -1055,6 +1055,11 @@ struct controller *pcie_init(struct pcie_device
> *dev)
> }
> }
>
> + pdev = pci_get_slot(subordinate, PCI_DEVFN(0, 0));
> + if (pdev)
> + ctrl->dsn = pci_get_dsn(pdev);
> + pci_dev_put(pdev);
> +
> return ctrl;
> }
>
> diff --git a/drivers/pci/hotplug/pciehp_pci.c b/drivers/pci/hotplug/pciehp_pci.c
> index ad12515..65e50be 100644
> --- a/drivers/pci/hotplug/pciehp_pci.c
> +++ b/drivers/pci/hotplug/pciehp_pci.c
> @@ -72,6 +72,10 @@ int pciehp_configure_device(struct controller *ctrl)
> pci_bus_add_devices(parent);
> down_read_nested(&ctrl->reset_lock, ctrl->depth);
>
> + dev = pci_get_slot(parent, PCI_DEVFN(0, 0));
> + ctrl->dsn = pci_get_dsn(dev);
> + pci_dev_put(dev);
> +
> out:
> pci_unlock_rescan_remove();
> return ret;
> --
> 2.43.0