Re: [PATCH v12 09/12] cxl: Restore CXL state after PCI reset
From: Srirangan Madhavan
Date: Tue Sep 22 2026 - 20:10:34 EST
On 9/11/26 6:43 PM, Jonathan Cameron wrote:
External email: Use caution opening links or attachments
On Thu, 10 Sep 2026 07:08:05 +0000
Srirangan Madhavan <smadhavan@xxxxxxxxxx> wrote:
After CXL reset, restore PCI config state enough to reach HDM MMIO,I'm nearly out of time for today and have given quite a bit of
restore cached global and per-decoder HDM state, restore the cached
CXL.cache and CXL.mem enable bits, and then run the normal PCI restore
callbacks.
If reset clears a previously locked decoder, restore and commit its
programming before reapplying the cached lock. Leave a locked committed
decoder unchanged when that state survives reset.
Keep the target IOMMU reset block active until CXL state restore completes
so Bus Master Enable cannot reopen DMA before decoder state is valid.
Signed-off-by: Srirangan Madhavan <smadhavan@xxxxxxxxxx>
feedback on earlier patches. So this is going to be a scan read
at most. Make sure to take another look at this and incorporate
the sort of changes I've asked for elsewhere for v13.
This seems to carry on papering over the cracks after errors that
to me indicate a broken device. I don't see any reason to do that.
If a device needs quirks to say don't run a particular restore do that
but if they are real errors, just give up - your device is not going
to be useable with only some stuff restored and you may be making
an obvious bug a much more subtle one.
Jonathan
---
drivers/cxl/core/resource.c | 411 ++++++++++++++++++++++++++++++++++--
1 file changed, 396 insertions(+), 15 deletions(-)
+static int cxl_restore_hdm_decoder(struct pci_dev *pdev,
+ struct cxl_hdm_decoder_state *state,
+ struct cxl_decoder_settings *settings,
+ void __iomem *hdm)
+{
+ u32 ctrl;
+ int rc;
+
+ rc = cxl_hdm_decoder_uncommit(pdev, hdm, settings->id);
+ if (rc == -EBUSY)
+ return 0;
+ if (rc)
+ return rc;
+
+ cxl_restore_hdm_decoder_state(state, hdm, settings->id);
+
+ if (!(settings->flags & CXL_DECODER_F_ENABLE))
+ return 0;
+
+ scoped_guard(rwsem_read, &cxl_rwsem.dpa)
+ rc = cxl_commit_start(hdm, settings);
+ if (!rc)
Use a helper function for the stuff in here so you can return directly.
+ rc = cxl_commit_wait(hdm, settings);
+ if (rc)
+ pci_err(pdev, "CXL HDM decoder %d restore failed: %d\n",
+ settings->id, rc);
+ else if (settings->flags & CXL_DECODER_F_LOCK) {
+ ctrl = readl(hdm + CXL_HDM_DECODER0_CTRL_OFFSET(settings->id));
+ ctrl |= CXL_HDM_DECODER0_CTRL_LOCK;
+ writel(ctrl, hdm + CXL_HDM_DECODER0_CTRL_OFFSET(settings->id));
+
+ ctrl = readl(hdm + CXL_HDM_DECODER0_CTRL_OFFSET(settings->id));
+ if (PCI_POSSIBLE_ERROR(ctrl) ||
+ !(ctrl & CXL_HDM_DECODER0_CTRL_LOCK)) {
+ pci_err(pdev,
+ "CXL HDM decoder %d failed to restore lock\n",
+ settings->id);
+ return -EIO;
+ }
+ }
+
+ return rc;
+}
+__free(kfree); looks like it will make life simpler in here.
+static void cxl_restore_pci_state_for_hdm_restore(struct pci_dev *pdev,
+ u16 *command)
+{
+ u32 saved_config = pdev->saved_config_space[PCI_COMMAND / 4];
+
+ pdev->saved_config_space[PCI_COMMAND / 4] &= ~PCI_COMMAND_MASTER;
+ pdev->saved_config_space[PCI_COMMAND / 4] |= PCI_COMMAND_INTX_DISABLE;
+ pci_restore_state(pdev);
+ pdev->saved_config_space[PCI_COMMAND / 4] = saved_config;
+ *command = saved_config & 0xffff;
+}
+
+static int cxl_restore_state(struct pci_dev *pdev)
+{
+ struct cxl_hdm_info *snap = cxl_snapshot_hdm(pdev);
+ bool restore_command = false;
+ void __iomem *hdm;
+ int first_rc = 0;
+ u16 command;
+ int rc;
+
+ if (!snap)
+ return 0;
+ if (IS_ERR(snap))
+ return PTR_ERR(snap);
+
+ rc = cxl_hdm_enable_mem(pdev, &command, &restore_command);
+ if (rc) {
+ kfree(snap);
+ return rc;
+ }
+
+ hdm = cxl_pci_hdm_ioremap_current(pdev, snap->hdm_bar,
+ snap->hdm_offset, snap->hdm_size);
+ if (IS_ERR(hdm)) {
+ first_rc = PTR_ERR(hdm);
I'm not really understanding the keep trying and paper over the cracks
going on here. If almost any of these fail it looks to me like
we are in a bad place and it would be cleaner to report and give up.
I restored 'some state' isn't likely to be very useful to anyone
beyond maybe making the breakage subtle rather than major and easy
to find.
+ } else {
+ /*
+ * Restore global HDM control before per-decoder commit. PCI
+ * config memory decoding is enabled for MMIO access, but bus
+ * mastering remains disabled until HDM restore completes.
+ */
+ writel(snap->global_ctrl, hdm + CXL_HDM_DECODER_CTRL_OFFSET);
+
+ for (int i = 0; i < snap->decoder_count; i++) {
+ rc = cxl_restore_hdm_decoder(pdev,
+ &snap->decoder_state[i],
+ &snap->settings[i], hdm);
+ if (rc && !first_rc)
+ first_rc = rc;
+ }
+
+ /* Flush posted HDM writes before PCI_COMMAND can restore BME. */
+ readl(hdm + CXL_HDM_DECODER_CTRL_OFFSET);
+ iounmap(hdm);
+ }
+
+ if (!first_rc && snap->dvsec_ctrl_valid) {
+ rc = cxl_restore_dvsec_ctrl(pdev, snap->dvsec_ctrl);
+ if (rc)
+ first_rc = rc;
+ }
+
+ if (restore_command) {
+ rc = cxl_hdm_restore_command(pdev, command);
+ if (rc && !first_rc)
+ first_rc = rc;
+ }
+
+ kfree(snap);
+ return first_rc;
+}
Hi Jonathan, this patch is now v13's patch 13:
- moved the commit-under-DPA-lock operation into a helper that returns
the result directly;
- stop restoration at the first error instead of continuing with
partially restored state
- leave the PCI command state disabled when restoration fails, preventing the outer PCI path from re-enabling an incompletely restored device.
The snapshot is now taken by the reset caller before reset rather than allocated inside cxl_restore_state(). I kept its cleanup explicit because the caller has staged goto-based cleanup for the snapshot, range reservations, IOMMU state, and cache policy.
https://lore.kernel.org/linux-cxl/20260922083924.2451158-14-smadhavan@xxxxxxxxxx/
--
Regards,
Srirangan