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

[Srirangan Madhavan]

On 8/26/26 11:09 AM, Lucero Palau, Alejandro wrote:
> > +     ctrl2 &= ~clear;
> > +     return cxl_reset_write_ctrl2(pdev, dvsec, ctrl2);
> > +}
>
>
> These two functions are almost the same code. Merging them and using a
> bool for set or clear seems reasonable to me.

I merged the set and clear helpers into cxl_reset_modify_ctrl2(), which 
accepts separate set and clear masks. I used masks instead of a boolean 
because reset initiation needs to set INIT_CXL_RST and clear 
RST_MEM_CLR_EN in the same read-modify-write operation.

> > +static int cxl_reset_enable_cache(struct pci_dev *pdev, int dvsec)
> > +{
> > +     return cxl_reset_clear_ctrl2(pdev, dvsec,
> > +                                  PCI_DVSEC_CXL_DISABLE_CACHING);
> > +}
> > +
> > +static int cxl_reset_initiate(struct pci_dev *pdev, int dvsec)
> > +{
> > +     u16 ctrl2;
> > +     int rc;
> > +
> > +     rc = cxl_reset_read_ctrl2(pdev, dvsec, &ctrl2);
> > +     if (rc)
> > +             return rc;
> > +
> > +     ctrl2 &= ~PCI_DVSEC_CXL_RST_MEM_CLR_EN;
> > +     ctrl2 |= PCI_DVSEC_CXL_INIT_CXL_RST;
> > +     return cxl_reset_write_ctrl2(pdev, dvsec, ctrl2);
> > +}
> > +
> > +static int cxl_reset_wait_cache_wbi(struct pci_dev *pdev, int dvsec)
> > +{
> > +     unsigned long deadline;
> > +     u16 status2;
> > +     int rc;
> > +
> > +     rc = cxl_reset_set_ctrl2(pdev, dvsec, 
> > PCI_DVSEC_CXL_INIT_CACHE_WBI);
> > +     if (rc)
> > +             return rc;
> > +
> > +     deadline = jiffies + usecs_to_jiffies(CXL_CACHE_WBI_TIMEOUT_US);
> > +     do {
> > +             usleep_range(CXL_CACHE_WBI_POLL_US, 
> > CXL_CACHE_WBI_POLL_US + 1);
> > +
> > +             rc = pci_read_config_word(pdev, dvsec + 
> > PCI_DVSEC_CXL_STATUS2,
> > +                                       &status2);
> > +             if (rc)
> > +                     return pcibios_err_to_errno(rc);
> > +             if (status2 != U16_MAX && (status2 & 
> > PCI_DVSEC_CXL_CACHE_INV))
> > +                     return 0;
> > +     } while (time_before(jiffies, deadline));
> > +
> > +     return -ETIMEDOUT;
> > +}
> > +
> > +static int cxl_reset_disable_cache(struct pci_dev *pdev, int dvsec, 
> > u16 cap)
> > +{
> > +     int rc, rc2;
> > +
> > +     rc = cxl_reset_set_ctrl2(pdev, dvsec,
> > +                              PCI_DVSEC_CXL_DISABLE_CACHING);
> > +     if (rc)
> > +             return rc;
> > +
> > +     if (!(cap & PCI_DVSEC_CXL_CACHE_WBI_CAPABLE))
> > +             return 0;
> > +
> > +     rc = cxl_reset_wait_cache_wbi(pdev, dvsec);
> > +     if (!rc)
> > +             return 0;
> > +
> > +     rc2 = cxl_reset_enable_cache(pdev, dvsec);
>
>
> Why do you re-enable the caching? I understand it needs to be disabled
> for safely doing the reset, but why to enable it?
>
> If there is a reason, maybe adding a comment would help to future readers.
>
>
> Thanks,
>
> Alejandro


The reset path sets it temporarily. My concern was if WBI or reset 
preparation fails before reset is issued, leaving the bit set would 
leave the device operating with caching disabled. The error and 
completion path therefore clears the bit to restore the default caching 
policy. I added a comment explaining this cleanup.

--
Regards,
Srirangan