Re: [PATCH v12 06/12] cxl: Add CXL Device Reset helper

[Srirangan Madhavan <smadhavan@nvidia.com>]

On 9/11/26 6:26 PM, Jonathan Cameron wrote:
> External email: Use caution opening links or attachments
> 
> 
> On Thu, 10 Sep 2026 07:08:02 +0000
> Srirangan Madhavan <smadhavan@nvidia.com> wrote:
> 
>> Add an internal CXL Device Reset helper for Type 2 functions that
>> advertise CXL Reset and CXL Reset Memory Clear in the CXL Device DVSEC.
>> The helper disables CXL.cache, performs cache writeback when supported,
>> initiates reset with Memory Clear enabled, waits for completion, and
>> re-enables CXL.cache on exit.
>>
>> Leave the helper unregistered until range validation and reset-scope
>> validation are in place.
>>
>> Signed-off-by: Srirangan Madhavan <smadhavan@nvidia.com>
> A few things inline.
>> ---
>>   drivers/cxl/core/resource.c   | 251 ++++++++++++++++++++++++++++++++++
>>   include/cxl/cxl.h             |   7 +
>>   include/uapi/linux/pci_regs.h |  14 ++
>>   3 files changed, 272 insertions(+)
>>
>> diff --git a/drivers/cxl/core/resource.c b/drivers/cxl/core/resource.c
>> index 6d9f8fe14b16..9c1aa0800521 100644
>> --- a/drivers/cxl/core/resource.c
>> +++ b/drivers/cxl/core/resource.c
>> @@ -8,6 +8,8 @@
>>   #include <linux/export.h>
>>   #include <linux/io.h>
>>   #include <linux/ioport.h>
>> +#include <linux/iommu.h>
>> +#include <linux/jiffies.h>
>>   #include <linux/kernel.h>
>>   #include <linux/pci.h>
>>   #include <linux/slab.h>
>> @@ -492,3 +494,252 @@ void pci_cxl_hdm_init(struct pci_dev *pdev)
>>        if (rc && rc != -ENOTTY && rc != -ENODEV)
>>                pci_dbg(pdev, "CXL HDM cache init failed: %d\n", rc);
>>   }
>> +/*
>> + * CXL r4.0 sec 9.7.2 defines the reset completion timeout encodings.
>> + * Sec 9.7.3 leaves config-space access behavior undefined for 100 ms after
>> + * initiating CXL Reset, then limits software to CXL Status2 access until
>> + * reset completion, timeout, or error.
>> + */
>> +#define CXL_RESET_RRS_WAIT_MS 100
>> +#define CXL_RESET_STATUS_POLL_MS 20
>> +static const u32 cxl_reset_timeout_ms[] = {
>> +     10, 100, 1000, 10000, 100000,
>> +};
>> +
>> +#define CXL_CACHE_WBI_TIMEOUT_US 100000
>> +#define CXL_CACHE_WBI_POLL_US 100
>> +
>> +static int cxl_reset_get_dvsec(struct pci_dev *pdev, u16 *cap_out)
>> +{
>> +     int dvsec, rc;
>> +     u16 cap, ctrl;
>> +
>> +     dvsec = cxl_pci_get_device_dvsec_cap(pdev, 0, &cap);
> 
> That is an odd function given the control read is separate - why
> should we wrap up the cap read?
> 
> I'd just drop this helper and open code the search for the extended
> cap at the few callsites.  A little more code, but less odd and all
> standard PCI stuff not needing an extra helper.
> 
> Probably a bit of code evolution caused this given you are up at v12!
> 
>> +     if (dvsec < 0)
>> +             return dvsec;
>> +
>> +     if (!(cap & PCI_DVSEC_CXL_CACHE_CAPABLE) ||
>> +         !(cap & PCI_DVSEC_CXL_MEM_CAPABLE))
> 
> Why do we need them both? Sure that's type 2, but a non
> class code matching type3 would I think need the same infrastructure
> you are building here. That would have cxl.mem but not cxl.cache
> - I think some of the CXL SSD prototypes fit in this category.
> 
>> +             return -ENOTTY;
>> +
>> +     if (!(cap & PCI_DVSEC_CXL_RST_CAPABLE))
>> +             return -ENOTTY;
>> +     if (!(cap & PCI_DVSEC_CXL_RST_MEM_CLR_CAPABLE))
>> +             return -ENOTTY;
>> +
>> +     rc = pci_read_config_word(pdev, dvsec + PCI_DVSEC_CXL_CTRL, &ctrl);
>> +     if (rc)
>> +             return pcibios_err_to_errno(rc);
>> +
>> +     if (!(ctrl & PCI_DVSEC_CXL_CACHE_ENABLE) ||
>> +         !(ctrl & PCI_DVSEC_CXL_MEM_ENABLE))
>> +             return -ENOTTY;
>> +
>> +     *cap_out = cap;
>> +     return dvsec;
>> +}
>> +
>> +#define CXL_RESET_CTRL2_CMD_MASK \
>> +     (PCI_DVSEC_CXL_INIT_CACHE_WBI | PCI_DVSEC_CXL_INIT_CXL_RST)
>> +
>> +static int cxl_reset_read_ctrl2(struct pci_dev *pdev, int dvsec, u16 *ctrl2)
>> +{
>> +     int rc;
>> +
>> +     rc = pci_read_config_word(pdev, dvsec + PCI_DVSEC_CXL_CTRL2, ctrl2);
>> +     if (rc)
>> +             return pcibios_err_to_errno(rc);
>> +
>> +     *ctrl2 &= ~CXL_RESET_CTRL2_CMD_MASK;
> I'd rename this. It is doing more than reading crl2.  Or push the code
> inline and avoid need for any name bikeshedding.
> 
>> +     return 0;
>> +}
>> +
>> +static int cxl_reset_write_ctrl2(struct pci_dev *pdev, int dvsec, u16 ctrl2)
>> +{
>> +     int rc;
>> +
>> +     rc = pci_write_config_word(pdev, dvsec + PCI_DVSEC_CXL_CTRL2, ctrl2);
>> +     if (rc)
>> +             return pcibios_err_to_errno(rc);
> This helper is also providing little value
>> +
>> +     return 0;
>> +}
>> +
>> +static int cxl_reset_modify_ctrl2(struct pci_dev *pdev, int dvsec, u16 set,
>> +                               u16 clear)
> 
> This needs a rename because it has that subtle mask of previous
> state. What is it actually allowing you to modify?
> 
>> +{
>> +     u16 ctrl2;
>> +     int rc;
>> +
>> +     rc = cxl_reset_read_ctrl2(pdev, dvsec, &ctrl2);
>> +     if (rc)
>> +             return rc;
>> +
>> +     ctrl2 &= ~clear;
>> +     ctrl2 |= set;
>> +     return cxl_reset_write_ctrl2(pdev, dvsec, ctrl2);
> 
> So with them squashed inline this would just be
> {
>          u16 ctrl2;
>          int rc;
> 
>          rc = pci_read_config_word(pdev, dvsec + PCI_DVSEC_CXL_CTRL2, ctrl2);
>          if (rc)
>                  return pcibios_err_to_errno(rc);
> 
>          /* Comment on why this is masked */
>          ctrl2 &= ~(PCI_DVSEC_CXL_INIT_CACHE_WBI | PCI_DVSEC_CXL_INIT_CXL_RST);
>          ctrl2 &= ~clear;
>          ctrl2 |= set;
> 
>          rc = pci_write_config_word(pdev, dvsec + PCI_DVSEC_CXL_CTRL2, ctrl2);
>          if (rc)
>                  return pcibios_err_to_errno(rc);
> 
>          return 0;
> }
> 
> which is if anything easier to read.
> 
>> +}
>> +
>> +static int cxl_reset_enable_cache(struct pci_dev *pdev, int dvsec)
>> +{
>> +     return cxl_reset_modify_ctrl2(pdev, dvsec, 0,
>> +                                   PCI_DVSEC_CXL_DISABLE_CACHING);
>> +}
>> +
>> +static int cxl_reset_initiate(struct pci_dev *pdev, int dvsec)
>> +{
>> +     return cxl_reset_modify_ctrl2(pdev, dvsec,
>> +                                   PCI_DVSEC_CXL_INIT_CXL_RST |
>> +                                   PCI_DVSEC_CXL_RST_MEM_CLR_EN, 0);
>> +}
> 
> 
> 
>> +
>> +static int cxl_reset_execute(struct pci_dev *pdev, int dvsec, u16 cap)
>> +{
>> +     bool target_prepared = false;
>> +     int rc, rc2;
>> +
>> +     rc = cxl_reset_disable_cache(pdev, dvsec, cap);
>> +     if (rc)
>> +             return rc;
>> +
>> +     if (!pci_wait_for_pending_transaction(pdev))
>> +             pci_err(pdev, "timed out waiting for pending transactions\n");
>> +
>> +     rc = pci_dev_reset_iommu_prepare(pdev);
>> +     if (rc)
>> +             pci_err(pdev, "failed to stop IOMMU for CXL reset: %d\n", rc);
> 
> Maybe a comment on whether this is even remotely safe to continue.
> My gut feeling is this sort of thing happens, just give up...
> 
> 
>> +     else
>> +             target_prepared = true;
>> +
>> +     if (!rc)
>> +             rc = cxl_reset_initiate(pdev, dvsec);
>> +     if (!rc)
>> +             rc = cxl_reset_wait_done(pdev, dvsec, cap);
>> +
>> +     rc2 = cxl_reset_enable_cache(pdev, dvsec);
> Can't we delay this until after the iommu is told things are back?
> I'm not keen on the sequence not being a clean tear down then a clean setup
> in the other order. I may well be missing some subtleties. This maybe
> needs some documentation.
> 
>> +     if (rc2 && rc)
>> +             pci_warn(pdev, "failed to re-enable CXL caching: %d\n", rc2);
> 
> What is logic about not printing if we have another failure sat in rc?
> Is it not true anyway?
> 
>> +     else if (rc2)
>> +             rc = rc2;
> 
> This code flow is less than ideal I'd use some gotos rather than that if (!rc)
> dance.
> 
>> +
>> +     if (target_prepared)
>> +             pci_dev_reset_iommu_done(pdev);
>> +     return rc;
> 
>          return rc ? rc : rc2; and some of the complexity above goes away.
> 
>> +}
>> +
>> +int cxl_reset_function(struct pci_dev *pdev, bool probe)
>> +{
>> +     int dvsec;
>> +     u16 cap;
>> +
>> +     dvsec = cxl_reset_get_dvsec(pdev, &cap);
>> +     if (dvsec < 0)
>> +             return dvsec;
>> +
>> +     if (probe)
>> +             return 0;
>> +
>> +     return cxl_reset_execute(pdev, dvsec, cap);
>> +}
> 

The fixes for these now are in v13 patch 10:
   - removed cxl_reset_get_dvsec() and open-coded the standard DVSEC lookup;
   - removed the separate Control2 read and write wrappers;
   - renamed the remaining update helper to 
cxl_reset_update_ctrl2_no_replay() and documented why command bits are 
masked;
   - abort reset when IOMMU preparation fails;
   - moved cache-policy restoration until after IOMMU exclusion is released
   - replaced the if (!rc) flow with explicit unwind paths while 
preserving the primary error.

https://lore.kernel.org/linux-cxl/20260922083924.2451158-11-smadhavan@nvidia.com/

-- 
Regards,
Srirangan