Re: [PATCH v15 13/16] PCI/CXL: Restore CXL state after PCI reset

[Srirangan Madhavan]

On 10/26/26 8:56 PM, Alison Schofield wrote:
> Hi Srirangan,
>
> This looks like a copy of cxld_await_commit() w the final test flipped.
> Could cxld_await_commit() take the Committed state to wait for, instead
> of copying it here?
>
> > +
> >   static u32 cxl_hdm_write_range(void __iomem *hdm,
> >                               const struct cxl_decoder_config *config)
> >   {
> > @@ -273,3 +293,163 @@ int cxl_hdm_read_state(struct device *dev, void __iomem *hdm,
> >
> >        return 0;
> >   }
> > +
> > +static int cxl_hdm_decoder_uncommit(struct device *dev, void __iomem *hdm,
> > +                                 int id, bool *retained)
> Same ask as on v14.
> Can the restore helpers return the error and let cxl_restore_state()
> report it? v15 swaps struct pci_dev for struct device, but it still
> runs through cxl_hdm_restore_state(), cxl_hdm_restore_decoder(),
> this function and cxl_hdm_read_decoder_settings(), only for dev_err().
>
> snip
>
> > +cxl_hdm_restore_decoder(struct device *dev, void __iomem *hdm,
> > +                     const struct cxl_endpoint_decoder_settings *settings)
> > +{
> > +     const struct cxl_decoder_config *config = &settings->config;
> > +     bool retained;
> > +     u32 ctrl;
> > +     int rc;
> > +
> > +     rc = cxl_hdm_decoder_uncommit(dev, hdm, config->id, &retained);
> > +     if (rc)
> > +             return rc;
> > +
> > +     if (retained) {
> > +             struct cxl_endpoint_decoder_settings active;
> > +
> > +             rc = cxl_hdm_read_decoder_settings(dev, &active, hdm, config->id);
> > +             if (rc) {
> > +                     dev_err(dev,
> > +                             "CXL HDM decoder %d has invalid configuration: %d\n",
> > +                             config->id, rc);
>
> cxl_hdm_read_decoder_settings() already logs this same message, so it
> gets logged twice. That goes away with the reporting change above.
>
> snip
> > +                     config->id, rc);
> > +             return rc;
> > +     }
> > +
> > +     if (!(config->flags & CXL_DECODER_F_LOCK))
> > +             return 0;
> > +
> > +     ctrl = readl(hdm + CXL_HDM_DECODER0_CTRL_OFFSET(config->id));
> > +     if (ctrl == U32_MAX)
> > +             return -EIO;
> > +     ctrl |= CXL_HDM_DECODER0_CTRL_LOCK;
> > +     writel(ctrl, hdm + CXL_HDM_DECODER0_CTRL_OFFSET(config->id));
> This reads like we commit the decoder first and set Lock On Commit next.
> The spec seems to say the lock only takes effect at the moment the
> decoder commits, making this too late. Looking at spec 8.2.4.20.17.
>
> -- Alison

Hi Alison,

In v16, I changed cxld_await_commit() to handle both commit and 
uncommit, and moved restore error reporting to cxl_restore_state().

I also changed the restore sequence to set Lock On Commit before 
recommitting a cleared locked decoder. Retained locked decoder state is 
still checked and left unchanged.

--
Regards,
Srirangan