Re: [PATCH v2] cxl/hdm: allow zero sized committed decoders

[Vishal Aslot]

> ________________________________________
> From: Gregory Price <gourry@xxxxxxxxxx>
> Sent: Thursday, October 2, 2025 11:28 PM
> To: Vishal Aslot
> Cc: Dave Jiang; Davidlohr Bueso; Jonathan Cameron; Alison Schofield; Vishal Verma; Ira Weiny; > Dan Williams; Li Ming; Peter Zijlstra; Dan Carpenter; Zijun Hu; linux-cxl@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v2] cxl/hdm: allow zero sized committed decoders
>
> External email: Use caution opening links or attachments
>
>
> On Fri, Oct 03, 2025 at 12:59:07AM +0000, Vishal Aslot wrote:
>>
>> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
>> index e9e1d555cec6..50164fd1b434 100644
>> --- a/drivers/cxl/core/hdm.c
>> +++ b/drivers/cxl/core/hdm.c
>> @@ -1047,10 +1047,10 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
>>                 }
>>
>> +               port->commit_end = cxld->id;
>> +
>
> Went looking to understand what commit_end actually does here, can you
> help explain?

My understanding is that "commit_end" is the high watermark inside a port with decoders. cxl_num_decoders_committed() returns this high watermark. In init_hdm_decoder(), there is a check to make sure that the next decoder is higher id by 1 (and must also be higher address). If not, it would error out saying "Committed out of order". 

Consider a case of 4 decoders (decoders 0 to 3 in xa_array/cxld->id). Decoder 0 is non-zero sized. The other 3 are zero-sized. All are committed.

In the last revision of the patch, I was doing "put_device()" on the zero-sized decoder, which shifted decoder ids to the left (ids 0, 1, 2, 3 became 0 -> 0, 1 -> deleted, 2 -> 1, 3 -> 2). This is fine and no "Committed out of order" error would be seen.

Now, I'm keeping the zero-sized decoders around, so I must update "commit_end". So I moved it up before the "if (size == 0)" check. This ensures no "Committed out of order" error would be seen because each decoder is one higher than the next.

>
>>                 if (size == 0) {
>> -                       dev_warn(&port->dev,
>> +                       dev_dbg(&port->dev,
>>                                  "decoder%d.%d: Committed with zero size\n",
>>                                  port->id, cxld->id);
>> -                       return -ENXIO;
>> +                       return -ENOSPC;
>>                 }
>> -               port->commit_end = cxld->id;
>>         } else {
>> @@ -1210,6 +1210,9 @@ int devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm,
>>                 rc = init_hdm_decoder(port, cxld, target_map, hdm, i,
>>                                       &dpa_base, info);
>>                 if (rc) {
>> +                       if (rc == -ENOSPC) {
>> +                               continue;
>> +                       }
>
> Don't need brackets here

Okie. Will remove it in v3.

>
>>                         dev_warn(&port->dev,
>>                                  "Failed to initialize decoder%d.%d\n",
>>                                  port->id, i);
>> --
>> 2.34.1