Re: [PATCH v12 09/12] cxl: Restore CXL state after PCI reset

[Srirangan Madhavan <smadhavan@nvidia.com>]

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@nvidia.com> wrote:
> 
>> After CXL reset, restore PCI config state enough to reach HDM MMIO,
>> 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@nvidia.com>
> I'm nearly out of time for today and have given quite a bit of
> 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;
>> +}
> 
>> +
>> +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);
> __free(kfree); looks like it will make life simpler in here.
> 
> 
>> +     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@nvidia.com/

-- 
Regards,
Srirangan