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

[Srirangan Madhavan <smadhavan@nvidia.com>]

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