Re: [PATCH v13 10/15] cxl: Add CXL Device Reset sequencing

[Srirangan Madhavan <smadhavan@nvidia.com>]

On 9/23/26 2:40 PM, Cheatham, Benjamin wrote:
> External email: Use caution opening links or attachments
>> +
>> +     for (;;) {
>> +             rc = pci_read_config_word(pdev, dvsec + PCI_DVSEC_CXL_STATUS2,
>> +                                       &status2);
>> +             if (rc || status2 == U16_MAX)
>> +                     goto not_ready;
>> +             if (status2 & PCI_DVSEC_CXL_RST_ERR)
>> +                     return -EIO;
>> +             if (status2 & PCI_DVSEC_CXL_RST_DONE)
>> +                     return 0;
>> +
>> +not_ready:
>> +             if (time_after_eq(jiffies, deadline))
>> +                     return -ETIMEDOUT;
>> +
>> +             msleep(CXL_RESET_STATUS_POLL_MS);
> 
> The goto here isn't necessary, I think this is functionally equivalent:
> 
>                  if ((rc || status2 == U16_MAX) && time_after_eq(jiffies, deadline))
>                          return -ETIMEDOUT;
>                  else if (status2 & PCI_DVSEC_CXL_RST_ERR)
>                          return -EIO;
>                  else if (status2 & PCI_DVSEC_CXL_RST_DONE)
>                          return 0;
> 
>                  msleep(CXL_RESET_STATUS_POLL_MS);
> 
> Sashiko also brought it up, but the timeout is probably too long. I'd pick something like 20s
> instead. That's probably still too conservative, but I don't know enough to suggest a more practical
> value.

V14 removes the goto, caps the timeout at 20 seconds, and allows one 
poll after the 100 ms quiet period. A config read error now returns 
immediately.

>> +     /*
>> +      * DISABLE_CACHING was the first preparation step. Restore the original
>> +      * cache policy last, after reset exclusion has ended.
>> +      */
>> +     rc2 = cxl_reset_update_ctrl2_no_replay(
>> +             pdev, dvsec, 0, PCI_DVSEC_CXL_DISABLE_CACHING);
>> +     if (rc2)
>> +             pci_err(pdev, "failed to re-enable CXL caching: %d\n", rc2);
>> +     rc = rc ?: rc2;
>> +
>> +     return rc;
>> +}
> 
> This is pretty messy, I think it would be cleaner if you split this into several functions:
> 
> static int __cxl_reset_execute(...)
> {
>          int rc;
> 
>          rc = cxl_reset_disable_cache(...);
>          if (rc)
>                  return rc;
> 
>          rc = cxl_clear_memory(...); // could be open coded, made a function for brevity
>          if (!rc)
>                  rc = cxl_reset_wait_done(...);
> 
>          rc = rc ?: cxl_clear_memory(...); // This one doesn't have the INIT_CXL_RST flag
> 
>          pci_dev_reset_iommu_done(pdev);
> 
>          return rc;
> }
> 
> static int cxl_reset_execute(...)
> {
>          int rc;
> 
>          rc = __cxl_reset_execute(..);
>          if (rc)
>                  // log error
> 
>          return rc ?: cxl_enable_cache(...);
> }
> 
> Hopefully what goes where makes sense just based on the names. I'm also not convinced you
> need to preserve the original error code throughout the function since it's essentially
> the same error conditions for all these functions AFAICT.
> 

Done in v14. I split the reset execution and Memory Clear handling into 
smaller helpers, following your suggested structure.

-- 
Regards,
Srirangan