Re: [PATCH v12 03/12] cxl: Share HDM decoder decode logic

[Srirangan Madhavan <smadhavan@nvidia.com>]

On 9/11/26 5:07 PM, Jonathan Cameron wrote:
> External email: Use caution opening links or attachments
> 
> 
> On Thu, 10 Sep 2026 07:07:59 +0000
> Srirangan Madhavan <smadhavan@nvidia.com> wrote:
> 
>> Move HDM decoder register decoding into a helper shared by normal CXL
>> core enumeration and early PCI HDM cache setup. This keeps validation of
>> base, range overflow, interleave, target type, and enable state in one
>> place before adding another HDM parser.
>>
> Hi Srirangan,
> 
> 
>> Keep caller-owned policy out of the decode helper. A committed zero-size
>> decoder now decodes successfully, while init_hdm_decoder() retains its
> 
> I'd not use decodes for that second bit given it's a decoder.  Choose
> another word - it definitely isn't doing any decoding.
> 
>> existing zero-size rejection.
>>
>> Preserve endpoint DPA state ownership by using the decoded skip value as
>> a local input to devm_cxl_dpa_reserve(). The reservation helper updates
>> cxled->skip under cxl_rwsem.dpa.
>>
>> Reported-by: Fenghua Yu <fenghua.yu@intel.com>
> 
> Add a of Closes tag for the report so we can see exactly what it is
> referring to. I'm guessing the zero length decoders?
> 
>> Signed-off-by: Srirangan Madhavan <smadhavan@nvidia.com>
> 
> Quite a bit of feedback on how this is done.  Maybe I'll get
> convinced in later patches but as it stands this is making the
> code less readable.  If it is useable in the cxl_decoder
> and we can lose the local structure than it becomes more
> convincing.
> 
>> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
>> index 70ffebd3e213..d621d827f59f 100644
>> --- a/drivers/cxl/core/hdm.c
>> +++ b/drivers/cxl/core/hdm.c
>> @@ -907,14 +907,11 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
>>   {
>>        struct cxl_endpoint_decoder *cxled = NULL;
>>        u64 size, base, skip, dpa_size, lo, hi;
>> +     struct cxl_decoder_settings settings;
>>        bool committed;
>>        u32 remainder;
>>        int i, rc;
>> -     u32 ctrl;
>> -     union {
>> -             u64 value;
>> -             unsigned char target_id[8];
>> -     } target_list;
>> +     u32 ctrl, tl_low, tl_high;
>>
>>        if (should_emulate_decoders(info))
>>                return cxl_setup_hdm_decoder_from_dvsec(port, cxld, dpa_base,
>> @@ -927,35 +924,33 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
>>        lo = readl(hdm + CXL_HDM_DECODER0_SIZE_LOW_OFFSET(which));
>>        hi = readl(hdm + CXL_HDM_DECODER0_SIZE_HIGH_OFFSET(which));
>>        size = (hi << 32) + lo;
>> -     committed = !!(ctrl & CXL_HDM_DECODER0_CTRL_COMMITTED);
>> +     tl_low = readl(hdm + CXL_HDM_DECODER0_TL_LOW(which));
>> +     tl_high = readl(hdm + CXL_HDM_DECODER0_TL_HIGH(which));
> 
> Be consistent on either combining these into local variables or not. Right now
> this is the only one handled in the parameters for the next call.
> 
>> +     rc = cxl_hdm_decode_decoder(&settings, which, ctrl, base, size,
>> +                                 ((u64)tl_high << 32) | tl_low, &committed);
>> +     if (rc) {
>> +             dev_warn(&port->dev,
>> +                      "decoder%d.%d: Invalid decoder configuration (ctrl: %#x): %d\n",
>> +                      port->id, cxld->id, ctrl, rc);
>> +             return rc;
>> +     }
>> +
>>        cxld->commit = cxl_decoder_commit;
>>        cxld->reset = cxl_decoder_reset;
>> -
>> -     if (!committed)
>> -             size = 0;
>> -     if (base == U64_MAX || size == U64_MAX) {
>> -             dev_warn(&port->dev, "decoder%d.%d: Invalid resource range\n",
>> -                      port->id, cxld->id);
>> -             return -ENXIO;
>> -     }
>> +     cxld->hpa_range = settings.hpa_range;
>> +     cxld->interleave_ways = settings.interleave_ways;
>> +     cxld->interleave_granularity = settings.interleave_granularity;
>> +     cxld->target_type = settings.target_type;
>> +     cxld->flags = settings.flags;
>> +     size = range_len(&cxld->hpa_range);
> 
> If this settings field matches cxld fields so well, why not embed one in
> there and write to that directly?  Without that I'm seeing little benefit
> in using the settings structure in here.  It is complicating the
> code and the only deduplication is a tiny number of checks.
> 
> 
>>
>>        if (info)
>>                cxled = to_cxl_endpoint_decoder(&cxld->dev);
>> -     cxld->hpa_range = (struct range) {
>> -             .start = base,
>> -             .end = base + size - 1,
>> -     };
>> +     if (!cxled && cxld->interleave_ways > 8)
>> +             return -ENXIO;
>>
>>        /* decoders are enabled if committed */
>>        if (committed) {
>> -             cxld->flags |= CXL_DECODER_F_ENABLE;
>> -             if (ctrl & CXL_HDM_DECODER0_CTRL_LOCK)
>> -                     cxld->flags |= CXL_DECODER_F_LOCK;
>> -             if (FIELD_GET(CXL_HDM_DECODER0_CTRL_HOSTONLY, ctrl))
>> -                     cxld->target_type = CXL_DECODER_HOSTONLYMEM;
>> -             else
>> -                     cxld->target_type = CXL_DECODER_DEVMEM;
>> -
>>                guard(rwsem_write)(&cxl_rwsem.region);
>>                if (cxld->id != cxl_num_decoders_committed(port)) {
>>                        dev_warn(&port->dev,
>> @@ -995,33 +990,15 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
>>                        writel(ctrl, hdm + CXL_HDM_DECODER0_CTRL_OFFSET(which));
>>                }
>>        }
>> -     rc = eiw_to_ways(FIELD_GET(CXL_HDM_DECODER0_CTRL_IW_MASK, ctrl),
>> -                       &cxld->interleave_ways);
>> -     if (rc) {
>> -             dev_warn(&port->dev,
>> -                      "decoder%d.%d: Invalid interleave ways (ctrl: %#x)\n",
>> -                      port->id, cxld->id, ctrl);
>> -             return rc;
>> -     }
>> -     rc = eig_to_granularity(FIELD_GET(CXL_HDM_DECODER0_CTRL_IG_MASK, ctrl),
>> -                              &cxld->interleave_granularity);
>> -     if (rc) {
>> -             dev_warn(&port->dev,
>> -                      "decoder%d.%d: Invalid interleave granularity (ctrl: %#x)\n",
>> -                      port->id, cxld->id, ctrl);
>> -             return rc;
>> -     }
>> -
>>        dev_dbg(&port->dev, "decoder%d.%d: range: %#llx-%#llx iw: %d ig: %d\n",
>>                port->id, cxld->id, cxld->hpa_range.start, cxld->hpa_range.end,
>>                cxld->interleave_ways, cxld->interleave_granularity);
>>
>>        if (!cxled) {
>> -             lo = readl(hdm + CXL_HDM_DECODER0_TL_LOW(which));
>> -             hi = readl(hdm + CXL_HDM_DECODER0_TL_HIGH(which));
>> -             target_list.value = (hi << 32) + lo;
>>                for (i = 0; i < cxld->interleave_ways; i++)
>> -                     cxld->target_map[i] = target_list.target_id[i];
>> +                     cxld->target_map[i] = i < 4 ?
>> +                             (tl_low >> (i * 8)) & 0xff :
>> +                             (tl_high >> ((i - 4) * 8)) & 0xff;
> 
> Can't we keep the type punning and readability it brings?
> Also why is the one thing that is still using the non settings path
> to get to values?  I'm not that convinced it makes sense to do any
> of this with your new settings structure but it needs to be consistent
> at least (like skip is below).
> 
> 
>>
>>                return 0;
>>        }
>> @@ -1036,9 +1013,7 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
>>                        port->id, cxld->id, size, cxld->interleave_ways);
>>                return -ENXIO;
>>        }
>> -     lo = readl(hdm + CXL_HDM_DECODER0_SKIP_LOW(which));
>> -     hi = readl(hdm + CXL_HDM_DECODER0_SKIP_HIGH(which));
>> -     skip = (hi << 32) + lo;
>> +     skip = settings.target_or_skip;
>>        rc = devm_cxl_dpa_reserve(cxled, *dpa_base + skip, dpa_size, skip);
>>        if (rc) {
>>                dev_err(&port->dev,
>> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
>> index 625e4aa427db..5085574521c6 100644
>> --- a/drivers/cxl/core/port.c
>> +++ b/drivers/cxl/core/port.c
>> @@ -34,6 +34,25 @@
>>   static DEFINE_IDA(cxl_port_ida);
>>   static DEFINE_XARRAY(cxl_root_buses);
>>
>> +struct pci_dev *cxl_port_get_uport_pci_dev(struct cxl_port *port)
>> +{
>> +     struct device *uport = port->uport_dev;
>> +     struct device *host;
>> +
>> +     if (is_cxl_memdev(uport)) {
>> +             struct cxl_memdev *cxlmd = to_cxl_memdev(uport);
>> +
>> +             host = cxlmd->dev.parent;
>> +     } else {
>> +             host = uport;
>> +     }
>> +
>> +     if (!host || !dev_is_pci(host))
>> +             return NULL;
>> +
>> +     return pci_dev_get(to_pci_dev(host));
> 
> Very nearly same code in read_cdata_data()
> 
> If you want this helper here, then introduce if first refactoring that code
> to show the helper is useful then use it here as well.  So basically
> put that as a precursor with a note that it will get reuse in this patch.
> 
> 
>> +}
>> +
>>   /*
>>    * The terminal device in PCI is NULL and @platform_bus
>>    * for platform devices (for cxl_test)
>> diff --git a/drivers/cxl/core/resource.c b/drivers/cxl/core/resource.c
>> index 8d3f43640199..e6aa55079c76 100644
>> --- a/drivers/cxl/core/resource.c
>> +++ b/drivers/cxl/core/resource.c
>> @@ -113,3 +113,46 @@ int cxl_commit_wait(void __iomem *hdm, struct cxl_decoder_settings *settings)
>>        return cxld_await_commit(hdm, settings->id);
>>   }
>>   EXPORT_SYMBOL_FOR_MODULES(cxl_commit_wait, "cxl_core");
>> +
>> +int cxl_hdm_decode_decoder(struct cxl_decoder_settings *settings, int id,
> 
> As in the description, this is too many decode given unrelated things
> they are talking about.  cxl_hdm_parse_decoder() maybe or cxl_hdm_unpack_decoder()
> or cxl_hdm_decoder_fill_settings() though then you'd need to put committed in there.
> 
> 
> 
>> +                        u32 ctrl, u64 base, u64 size, u64 target_or_skip,
>> +                        bool *committed)
>> +{
>> +     bool enabled = FIELD_GET(CXL_HDM_DECODER0_CTRL_COMMITTED, ctrl);
>> +     int rc;
>> +
>> +     *settings = (struct cxl_decoder_settings) {
>> +             .id = id,
>> +             .target_or_skip = target_or_skip,
>> +             .target_type = FIELD_GET(CXL_HDM_DECODER0_CTRL_HOSTONLY, ctrl) ?
>> +                            CXL_DECODER_HOSTONLYMEM : CXL_DECODER_DEVMEM,
> 
> Do we have paths where an early exit needs the partly filled in structure?
> I'm assuming not. In which case I'd shunt this down a bit to where you can fill
> in more in one go.
> 
> 
>> +     };
>> +
>> +     if (committed)
>> +             *committed = enabled;
> 
> I'm not sure why committed is special and doesn't go in the settings.
> 
>> +     if (!enabled)
>> +             size = 0;
>> +     if (base == U64_MAX || size == U64_MAX ||
>> +         (size && base > U64_MAX - (size - 1)))
>> +             return -ENXIO;
>> +
>> +     settings->hpa_range = (struct range) {
>> +             .start = base,
>> +             .end = base + size - 1,
>> +     };
> 
> With a bit of reorg, this can be filled in along with the stuff above
> reducing the zeroing then overwriting that is going on currently.
> 
>> +     if (enabled) {
>> +             settings->flags = CXL_DECODER_F_ENABLE;
>> +             if (ctrl & CXL_HDM_DECODER0_CTRL_LOCK)
>> +                     settings->flags |= CXL_DECODER_F_LOCK;
>> +     }
> If you used a local for building flags, this could also be rolled
> in.
>> +
>> +     rc = eiw_to_ways(FIELD_GET(CXL_HDM_DECODER0_CTRL_IW_MASK, ctrl),
>> +                      &settings->interleave_ways);
>> +     if (rc)
>> +             return rc;
>> +
>> +     return eig_to_granularity(FIELD_GET(CXL_HDM_DECODER0_CTRL_IG_MASK,
>> +                                 ctrl),
> Go long for readability.
> 
>> +                               &settings->interleave_granularity);
> Locals for these as well and it becomes
> ...
>          rc = eig_to_granularity(FIELD_GET(CXL_HDM_DECODER0_CTRL_IG_MASK, ctrl), &ig);
>          if (rc)
>                  return rc;
> 
>          *settings = (struct cxl_decoder_settings) {
>                  .id = id,
>                  .hpa_range = {
>                          .start = base,
>                          .end = base + size - 1,
>                  },
>                  .target_or_skip = target_or_skip,
>                  .interleave_ways = iw,
>                  .interleave_granularity = ig,
>                  .target_type = FIELD_GET(CXL_HDM_DECODER0_CTRL_HOSTONLY, ctrl) ?
>                                 CXL_DECODER_HOSTONLYMEM : CXL_DECODER_DEVMEM,
> 
>                  .flags = flags,
>          };
> 
>          return 0;
> }
>> +EXPORT_SYMBOL_FOR_MODULES(cxl_hdm_decode_decoder, "cxl_core");
> 

   I reworked this across v13 patches 2 and 7 based on the comments in 
this review. The shared PCI-device lookup is now a separate precursor, 
and the decoder helper is now cxl_hdm_unpack_decoder().

   The unpack helper validates locals before publishing the settings in 
one assignment, represents committed state in the settings flags, reads 
the target registers consistently, and retains the target-list union.


  
https://lore.kernel.org/linux-cxl/20260922083924.2451158-3-smadhavan@nvidia.com/
  
https://lore.kernel.org/linux-cxl/20260922083924.2451158-8-smadhavan@nvidia.com/

-- 
Regards,
Srirangan