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

[Srirangan Madhavan]

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