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

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

[Vishal Aslot]

Signed-off-by: Vishal Aslot <vaslot@nvidia.com>
---
 drivers/cxl/core/hdm.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
index e9e1d555cec6..97a5f27f5b72 100644
--- a/drivers/cxl/core/hdm.c
+++ b/drivers/cxl/core/hdm.c
@@ -1,5 +1,5 @@
 // SPDX-License-Identifier: GPL-2.0-only
-/* Copyright(c) 2022 Intel Corporation. All rights reserved. */
+/* Copyright(c) 2022-2025 Intel Corporation. All rights reserved. */
 #include <linux/seq_file.h>
 #include <linux/device.h>
 #include <linux/delay.h>
@@ -1050,7 +1050,7 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
                        dev_warn(&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,11 @@ 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) {
+                               put_device(&cxld->dev);
+                               rc = 0;
+                               continue;
+                       }
                        dev_warn(&port->dev,
                                 "Failed to initialize decoder%d.%d\n",
                                 port->id, i);
--
2.34.1

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

[Dave Jiang]

On 10/1/25 1:37 PM, Vishal Aslot wrote:
> Signed-off-by: Vishal Aslot <vaslot@nvidia.com>

Missing commit log?

> ---
>  drivers/cxl/core/hdm.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index e9e1d555cec6..97a5f27f5b72 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
> @@ -1,5 +1,5 @@
>  // SPDX-License-Identifier: GPL-2.0-only
> -/* Copyright(c) 2022 Intel Corporation. All rights reserved. */
> +/* Copyright(c) 2022-2025 Intel Corporation. All rights reserved. */

Stray change?

>  #include <linux/seq_file.h>
>  #include <linux/device.h>
>  #include <linux/delay.h>
> @@ -1050,7 +1050,7 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
>                         dev_warn(&port->dev,
>                                  "decoder%d.%d: Committed with zero size\n",
>                                  port->id, cxld->id);

I wonder if we should make this dev_dbg() now that it's a valid case.

> -                       return -ENXIO;
> +                       return -ENOSPC;
>                 }
>                 port->commit_end = cxld->id;
>         } else {
> @@ -1210,6 +1210,11 @@ 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) {
> +                               put_device(&cxld->dev);

Why put_device()? Should we enumerate this decoder instead of pretend it doesn't exist? essentially only a dev_set_name() and device_add()?

> +                               rc = 0;

Don't think this is needed since the continue will put it at start of the loop where rc gets written by init_hdm_decoder() return value.

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

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

[Vishal Aslot]

Thanks for the quick review, Dave. Yes, forgot to put the commit message. :)
About the copyright message. Should I not be updating the year to 2025 in it?











________________________________________
From: Dave Jiang <dave.jiang@intel.com>
Sent: Wednesday, October 1, 2025 4:28 PM
To: Vishal Aslot; Davidlohr Bueso; Jonathan Cameron; Alison Schofield; Vishal Verma; Ira Weiny; Dan Williams; Li Ming; Peter Zijlstra; Dan Carpenter; Zijun Hu; linux-cxl@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Subject: Re: [PATCH] cxl/hdm: allow zero sized committed decoders

External email: Use caution opening links or attachments


On 10/1/25 1:37 PM, Vishal Aslot wrote:
> Signed-off-by: Vishal Aslot <vaslot@nvidia.com>

Missing commit log?

> ---
>  drivers/cxl/core/hdm.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index e9e1d555cec6..97a5f27f5b72 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
> @@ -1,5 +1,5 @@
>  // SPDX-License-Identifier: GPL-2.0-only
> -/* Copyright(c) 2022 Intel Corporation. All rights reserved. */
> +/* Copyright(c) 2022-2025 Intel Corporation. All rights reserved. */

Stray change?

>  #include <linux/seq_file.h>
>  #include <linux/device.h>
>  #include <linux/delay.h>
> @@ -1050,7 +1050,7 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
>                         dev_warn(&port->dev,
>                                  "decoder%d.%d: Committed with zero size\n",
>                                  port->id, cxld->id);

I wonder if we should make this dev_dbg() now that it's a valid case.

> -                       return -ENXIO;
> +                       return -ENOSPC;
>                 }
>                 port->commit_end = cxld->id;
>         } else {
> @@ -1210,6 +1210,11 @@ 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) {
> +                               put_device(&cxld->dev);

Why put_device()? Should we enumerate this decoder instead of pretend it doesn't exist? essentially only a dev_set_name() and device_add()?

> +                               rc = 0;

Don't think this is needed since the continue will put it at start of the loop where rc gets written by init_hdm_decoder() return value.

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

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

[Dave Jiang]

On 10/1/25 3:03 PM, Vishal Aslot wrote:
> Thanks for the quick review, Dave. Yes, forgot to put the commit message. :)
> About the copyright message. Should I not be updating the year to 2025 in it?

I wouldn't bother unless you are doing massive changes and in that case it would be adding NVidia copyright anyhow for you. Also, typically linux mailing lists prefer responding inline and not top posting.
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> ________________________________________
> From: Dave Jiang <dave.jiang@intel.com>
> Sent: Wednesday, October 1, 2025 4:28 PM
> To: Vishal Aslot; Davidlohr Bueso; Jonathan Cameron; Alison Schofield; Vishal Verma; Ira Weiny; Dan Williams; Li Ming; Peter Zijlstra; Dan Carpenter; Zijun Hu; linux-cxl@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] cxl/hdm: allow zero sized committed decoders
> 
> External email: Use caution opening links or attachments
> 
> 
> On 10/1/25 1:37 PM, Vishal Aslot wrote:
>> Signed-off-by: Vishal Aslot <vaslot@nvidia.com>
> 
> Missing commit log?
> 
>> ---
>>  drivers/cxl/core/hdm.c | 9 +++++++--
>>  1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
>> index e9e1d555cec6..97a5f27f5b72 100644
>> --- a/drivers/cxl/core/hdm.c
>> +++ b/drivers/cxl/core/hdm.c
>> @@ -1,5 +1,5 @@
>>  // SPDX-License-Identifier: GPL-2.0-only
>> -/* Copyright(c) 2022 Intel Corporation. All rights reserved. */
>> +/* Copyright(c) 2022-2025 Intel Corporation. All rights reserved. */
> 
> Stray change?
> 
>>  #include <linux/seq_file.h>
>>  #include <linux/device.h>
>>  #include <linux/delay.h>
>> @@ -1050,7 +1050,7 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
>>                         dev_warn(&port->dev,
>>                                  "decoder%d.%d: Committed with zero size\n",
>>                                  port->id, cxld->id);
> 
> I wonder if we should make this dev_dbg() now that it's a valid case.
> 
>> -                       return -ENXIO;
>> +                       return -ENOSPC;
>>                 }
>>                 port->commit_end = cxld->id;
>>         } else {
>> @@ -1210,6 +1210,11 @@ 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) {
>> +                               put_device(&cxld->dev);
> 
> Why put_device()? Should we enumerate this decoder instead of pretend it doesn't exist? essentially only a dev_set_name() and device_add()?
> 
>> +                               rc = 0;
> 
> Don't think this is needed since the continue will put it at start of the loop where rc gets written by init_hdm_decoder() return value.
> 
>> +                               continue;
>> +                       }
>>                         dev_warn(&port->dev,
>>                                  "Failed to initialize decoder%d.%d\n",
>>                                  port->id, i);
>> --
>> 2.34.1
> 

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

[Vishal Aslot]

> ________________________________________
> From: Dave Jiang <dave.jiang@intel.com>
> Sent: Wednesday, October 1, 2025 5:46 PM
> To: Vishal Aslot; Davidlohr Bueso; Jonathan Cameron; Alison Schofield; Vishal Verma; Ira Weiny; Dan Williams; Li Ming; Peter Zijlstra; Dan Carpenter; Zijun Hu; linux-cxl@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] cxl/hdm: allow zero sized committed decoders
>
> External email: Use caution opening links or attachments
>
>
> On 10/1/25 3:03 PM, Vishal Aslot wrote:
>> Thanks for the quick review, Dave. Yes, forgot to put the commit message. :)
>> About the copyright message. Should I not be updating the year to 2025 in it?
>
> I wouldn't bother unless you are doing massive changes and in that case it would be adding NVidia copyright anyhow for you. Also, typically linux mailing lists prefer responding inline and > not top posting.

Ah, understood. Thanks for the mentoring.

>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>> ________________________________________
>> From: Dave Jiang <dave.jiang@intel.com>
>> Sent: Wednesday, October 1, 2025 4:28 PM
>> To: Vishal Aslot; Davidlohr Bueso; Jonathan Cameron; Alison Schofield; Vishal Verma; Ira Weiny; Dan Williams; Li Ming; Peter Zijlstra; Dan Carpenter; Zijun Hu; linux-cxl@vger.kernel.org
>> Cc: linux-kernel@vger.kernel.org
>> Subject: Re: [PATCH] cxl/hdm: allow zero sized committed decoders
>>
>> External email: Use caution opening links or attachments
>>
>>
>> On 10/1/25 1:37 PM, Vishal Aslot wrote:
>>> Signed-off-by: Vishal Aslot <vaslot@nvidia.com>
>>
>> Missing commit log?

Yes, will add one in v2.

>>>
>>> ---
>>>  drivers/cxl/core/hdm.c | 9 +++++++--
>>>  1 file changed, 7 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
>>> index e9e1d555cec6..97a5f27f5b72 100644
>>> --- a/drivers/cxl/core/hdm.c
>>> +++ b/drivers/cxl/core/hdm.c
>>> @@ -1,5 +1,5 @@
>>>  // SPDX-License-Identifier: GPL-2.0-only
>>> -/* Copyright(c) 2022 Intel Corporation. All rights reserved. */
>>> +/* Copyright(c) 2022-2025 Intel Corporation. All rights reserved. */
>>
>> Stray change?

No, it was intentional but I understand that for this small update, I don't need to update copyright info. I'll undo this change in v2.

>>
>>>  #include <linux/seq_file.h>
>>>  #include <linux/device.h>
>>>  #include <linux/delay.h>
>>> @@ -1050,7 +1050,7 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
>>>                         dev_warn(&port->dev,
>>>                                  "decoder%d.%d: Committed with zero size\n",
>>>                                  port->id, cxld->id);
>>
>> I wonder if we should make this dev_dbg() now that it's a valid case.

Yes, makes sense. I'll change it to dev_dbg() in v2.

>>
>>> -                       return -ENXIO;
>>> +                       return -ENOSPC;
>>>                 }
>>>                 port->commit_end = cxld->id;
>>>         } else {
>>> @@ -1210,6 +1210,11 @@ 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) {
>>> +                               put_device(&cxld->dev);
>>
>> Why put_device()? Should we enumerate this decoder instead of pretend it doesn't exist? essentially only a dev_set_name() and device_add()?

That is a good point. The decoder has been committed so it should be accounted for in the OS.
I'll remove put_device() in v2.

>>
>>> +                               rc = 0;
>>
>> Don't think this is needed since the continue will put it at start of the loop where rc gets written by init_hdm_decoder() return value.

Agreed. I was being paranoid. Will remove it in v2. 

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

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

[Gregory Price]

On Wed, Oct 01, 2025 at 08:37:26PM +0000, Vishal Aslot wrote:
> @@ -1210,6 +1210,11 @@ 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) {
> +                               put_device(&cxld->dev);
> +                               rc = 0;
> +                               continue;
> +                       }

How do you suggest actually testing this? I briefly poked at this in
QEMU trying to commit decoders, but i found myself incapable of
exercising this path.

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

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

[Dave Jiang]

On 10/1/25 10:48 PM, Gregory Price wrote:
> On Wed, Oct 01, 2025 at 08:37:26PM +0000, Vishal Aslot wrote:
>> @@ -1210,6 +1210,11 @@ 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) {
>> +                               put_device(&cxld->dev);
>> +                               rc = 0;
>> +                               continue;
>> +                       }
> 
> How do you suggest actually testing this? I briefly poked at this in
> QEMU trying to commit decoders, but i found myself incapable of
> exercising this path.

It may be worthwhile adding a cxl_test test case for this.

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

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

[Vishal Aslot]

init_hdm_decoder() fails with -ENXIO if a
committed HDM decoder has zero size.

The CXL spec permits committing zero sized decoders.
See "8.2.4.20.12 Committing Decoder Programming".
It says,"It is legal for software to program Decoder
Size to 0 and commit it. Such a decoder will not
participate in HDM decode."

This patch updates init_hdm_decoder() to return
-ENOSPC if the decoder is commited with zero-size.
The caller leaves the decoder allocated but does not
add it. It simply continues to the next decoder.

Signed-off-by: Vishal Aslot <vaslot@nvidia.com>
---
 drivers/cxl/core/hdm.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

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;
+
                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;
+                       }
                        dev_warn(&port->dev,
                                 "Failed to initialize decoder%d.%d\n",
                                 port->id, i);
--
2.34.1

> ________________________________________
> From: Vishal Aslot <vaslot@nvidia.com>
> Sent: Wednesday, October 1, 2025 8:57 PM
> To: Dave Jiang; Davidlohr Bueso; Jonathan Cameron; Alison Schofield; Vishal Verma; Ira Weiny; Dan Williams; Li Ming; Peter Zijlstra; Dan Carpenter; Zijun Hu; linux-cxl@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] cxl/hdm: allow zero sized committed decoders
>
> External email: Use caution opening links or attachments
>
>
>> ________________________________________
>> From: Dave Jiang <dave.jiang@intel.com>
>> Sent: Wednesday, October 1, 2025 5:46 PM
>> To: Vishal Aslot; Davidlohr Bueso; Jonathan Cameron; Alison Schofield; Vishal Verma; Ira Weiny; Dan Williams; Li Ming; Peter Zijlstra; Dan Carpenter; Zijun Hu; linux-cxl@vger.kernel.org
>> Cc: linux-kernel@vger.kernel.org
>> Subject: Re: [PATCH] cxl/hdm: allow zero sized committed decoders
>>
>> External email: Use caution opening links or attachments
>>
>>
>> On 10/1/25 3:03 PM, Vishal Aslot wrote:
>>> Thanks for the quick review, Dave. Yes, forgot to put the commit message. :)
>>> About the copyright message. Should I not be updating the year to 2025 in it?
>>
>> I wouldn't bother unless you are doing massive changes and in that case it would be adding NVidia copyright anyhow for you. Also, typically linux mailing lists prefer responding inline and >> not top posting.
>
> Ah, understood. Thanks for the mentoring.
>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>> ________________________________________
>>> From: Dave Jiang <dave.jiang@intel.com>
>>> Sent: Wednesday, October 1, 2025 4:28 PM
>>> To: Vishal Aslot; Davidlohr Bueso; Jonathan Cameron; Alison Schofield; Vishal Verma; Ira Weiny; Dan Williams; Li Ming; Peter Zijlstra; Dan Carpenter; Zijun Hu; linux-cxl@vger.kernel.org
>>> Cc: linux-kernel@vger.kernel.org
>>> Subject: Re: [PATCH] cxl/hdm: allow zero sized committed decoders
>>>
>>> External email: Use caution opening links or attachments
>>>
>>>
>>> On 10/1/25 1:37 PM, Vishal Aslot wrote:
>>>> Signed-off-by: Vishal Aslot <vaslot@nvidia.com>
>>>
>>> Missing commit log?
>
> Yes, will add one in v2.
>
>>>>
>>>> ---
>>>>  drivers/cxl/core/hdm.c | 9 +++++++--
>>>>  1 file changed, 7 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
>>>> index e9e1d555cec6..97a5f27f5b72 100644
>>>> --- a/drivers/cxl/core/hdm.c
>>>> +++ b/drivers/cxl/core/hdm.c
>>>> @@ -1,5 +1,5 @@
>>>>  // SPDX-License-Identifier: GPL-2.0-only
>>>> -/* Copyright(c) 2022 Intel Corporation. All rights reserved. */
>>>> +/* Copyright(c) 2022-2025 Intel Corporation. All rights reserved. */
>>>
>>> Stray change?
>
> No, it was intentional but I understand that for this small update, I don't need to update copyright info. I'll undo this change in v2.
>
>>>
>>>>  #include <linux/seq_file.h>
>>>>  #include <linux/device.h>
>>>>  #include <linux/delay.h>
>>>> @@ -1050,7 +1050,7 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
>>>>                         dev_warn(&port->dev,
>>>>                                  "decoder%d.%d: Committed with zero size\n",
>>>>                                  port->id, cxld->id);
>>>
>>> I wonder if we should make this dev_dbg() now that it's a valid case.
>
> Yes, makes sense. I'll change it to dev_dbg() in v2.
>
>>>
>>>> -                       return -ENXIO;
>>>> +                       return -ENOSPC;
>>>>                 }
>>>>                 port->commit_end = cxld->id;
>>>>         } else {
>>>> @@ -1210,6 +1210,11 @@ 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) {
>>>> +                               put_device(&cxld->dev);
>>>
>>> Why put_device()? Should we enumerate this decoder instead of pretend it doesn't exist? essentially only a dev_set_name() and device_add()?
>
> That is a good point. The decoder has been committed so it should be accounted for in the OS.
> I'll remove put_device() in v2.
>
>>>
>>>> +                               rc = 0;
>>>
>>> Don't think this is needed since the continue will put it at start of the loop where rc gets written by init_hdm_decoder() return value.
>
> Agreed. I was being paranoid. Will remove it in v2.
>
>>>
>>>> +                               continue;
>>>> +                       }
>>>>                         dev_warn(&port->dev,
>>>>                                  "Failed to initialize decoder%d.%d\n",
>>>>                                  port->id, i);
>>>> --
>>>> 2.34.1
>>>

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

[Vishal Aslot]

> ________________________________________
> From: Dave Jiang <dave.jiang@intel.com>
> Sent: Thursday, October 2, 2025 10:32 AM
> To: Gregory Price; Vishal Aslot
> Cc: Davidlohr Bueso; Jonathan Cameron; Alison Schofield; Vishal Verma; Ira Weiny; Dan Williams; Li Ming; Peter Zijlstra; Dan Carpenter; Zijun Hu; linux-cxl@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] cxl/hdm: allow zero sized committed decoders
> 
> External email: Use caution opening links or attachments
> 
> 
> On 10/1/25 10:48 PM, Gregory Price wrote:
>> On Wed, Oct 01, 2025 at 08:37:26PM +0000, Vishal Aslot wrote:
>>> @@ -1210,6 +1210,11 @@ 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) {
>>> +                               put_device(&cxld->dev);
>>> +                               rc = 0;
>>> +                               continue;
>>> +                       }
>>
>> How do you suggest actually testing this? I briefly poked at this in
>> QEMU trying to commit decoders, but i found myself incapable of
>> exercising this path.

I tested it locally with our BIOS (UEFI) where we commit and lock all decoders and
all except decoder 0 are zero-sized.

>
> It may be worthwhile adding a cxl_test test case for this.

Yeah, sure. Would cxl mock tests be the right place to explore this?

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

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

[Gregory Price]

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?

>                 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

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

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

[Gregory Price]

On Fri, Oct 03, 2025 at 01:03:16AM +0000, Vishal Aslot wrote:
> > ________________________________________
> > From: Dave Jiang <dave.jiang@intel.com>
> > Sent: Thursday, October 2, 2025 10:32 AM
> > To: Gregory Price; Vishal Aslot
> > Cc: Davidlohr Bueso; Jonathan Cameron; Alison Schofield; Vishal Verma; Ira Weiny; Dan Williams; Li Ming; Peter Zijlstra; Dan Carpenter; Zijun Hu; linux-cxl@vger.kernel.org; linux-kernel@vger.kernel.org
> > Subject: Re: [PATCH] cxl/hdm: allow zero sized committed decoders
> > 
> > External email: Use caution opening links or attachments
> > 
> > 
> > On 10/1/25 10:48 PM, Gregory Price wrote:
> >> On Wed, Oct 01, 2025 at 08:37:26PM +0000, Vishal Aslot wrote:
> >>> @@ -1210,6 +1210,11 @@ 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) {
> >>> +                               put_device(&cxld->dev);
> >>> +                               rc = 0;
> >>> +                               continue;
> >>> +                       }
> >>
> >> How do you suggest actually testing this? I briefly poked at this in
> >> QEMU trying to commit decoders, but i found myself incapable of
> >> exercising this path.
> 
> I tested it locally with our BIOS (UEFI) where we commit and lock all decoders and
> all except decoder 0 are zero-sized.
> 

Ahhh, so are you saying that you will only ever observe the following
(as an example)

endpoint decoders...
decoder2.0   ->  available and can be programmed
decoder2.1   ->  size=0, locked
...
decoder2.N   ->  size=0, locked

or are you suggesting the following is valid:

decoder2.0   ->  size=0, locked
decoder2.1   ->  available and can be programmed
...
decoder2.N   ->  available and can be programmed

~Gregory

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

[Dave Jiang]

On 10/2/25 6:03 PM, Vishal Aslot wrote:
>> ________________________________________
>> From: Dave Jiang <dave.jiang@intel.com>
>> Sent: Thursday, October 2, 2025 10:32 AM
>> To: Gregory Price; Vishal Aslot
>> Cc: Davidlohr Bueso; Jonathan Cameron; Alison Schofield; Vishal Verma; Ira Weiny; Dan Williams; Li Ming; Peter Zijlstra; Dan Carpenter; Zijun Hu; linux-cxl@vger.kernel.org; linux-kernel@vger.kernel.org
>> Subject: Re: [PATCH] cxl/hdm: allow zero sized committed decoders
>>
>> External email: Use caution opening links or attachments
>>
>>
>> On 10/1/25 10:48 PM, Gregory Price wrote:
>>> On Wed, Oct 01, 2025 at 08:37:26PM +0000, Vishal Aslot wrote:
>>>> @@ -1210,6 +1210,11 @@ 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) {
>>>> +                               put_device(&cxld->dev);
>>>> +                               rc = 0;
>>>> +                               continue;
>>>> +                       }
>>>
>>> How do you suggest actually testing this? I briefly poked at this in
>>> QEMU trying to commit decoders, but i found myself incapable of
>>> exercising this path.
> 
> I tested it locally with our BIOS (UEFI) where we commit and lock all decoders and
> all except decoder 0 are zero-sized.
> 
>>
>> It may be worthwhile adding a cxl_test test case for this.
> 
> Yeah, sure. Would cxl mock tests be the right place to explore this?

Yes. Under tools/testing/cxl. Let me know if you need help with that.

DJ

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

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

[Dave Jiang]

On 10/2/25 5:59 PM, Vishal Aslot wrote:
> init_hdm_decoder() fails with -ENXIO if a
> committed HDM decoder has zero size.
> 
> The CXL spec permits committing zero sized decoders.
> See "8.2.4.20.12 Committing Decoder Programming".
> It says,"It is legal for software to program Decoder
> Size to 0 and commit it. Such a decoder will not
> participate in HDM decode."
> 
> This patch updates init_hdm_decoder() to return
> -ENOSPC if the decoder is commited with zero-size.
> The caller leaves the decoder allocated but does not
> add it. It simply continues to the next decoder.
> 
> Signed-off-by: Vishal Aslot <vaslot@nvidia.com>

Hi Vishal,
Next time please post the next version of patches as a new thread and not a response to a previous discussion. It would make lore [1] viewing easier. Thanks!

[1]: https://lore.kernel.org/linux-cxl/aN_cUPzzwUy-s36n@gourry-fedora-PF4VCD3F/T/#t

DJ
 
> ---
>  drivers/cxl/core/hdm.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> 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;
> +
>                 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;
> +                       }
>                         dev_warn(&port->dev,
>                                  "Failed to initialize decoder%d.%d\n",
>                                  port->id, i);
> --
> 2.34.1
> 
>> ________________________________________
>> From: Vishal Aslot <vaslot@nvidia.com>
>> Sent: Wednesday, October 1, 2025 8:57 PM
>> To: Dave Jiang; Davidlohr Bueso; Jonathan Cameron; Alison Schofield; Vishal Verma; Ira Weiny; Dan Williams; Li Ming; Peter Zijlstra; Dan Carpenter; Zijun Hu; linux-cxl@vger.kernel.org
>> Cc: linux-kernel@vger.kernel.org
>> Subject: Re: [PATCH] cxl/hdm: allow zero sized committed decoders
>>
>> External email: Use caution opening links or attachments
>>
>>
>>> ________________________________________
>>> From: Dave Jiang <dave.jiang@intel.com>
>>> Sent: Wednesday, October 1, 2025 5:46 PM
>>> To: Vishal Aslot; Davidlohr Bueso; Jonathan Cameron; Alison Schofield; Vishal Verma; Ira Weiny; Dan Williams; Li Ming; Peter Zijlstra; Dan Carpenter; Zijun Hu; linux-cxl@vger.kernel.org
>>> Cc: linux-kernel@vger.kernel.org
>>> Subject: Re: [PATCH] cxl/hdm: allow zero sized committed decoders
>>>
>>> External email: Use caution opening links or attachments
>>>
>>>
>>> On 10/1/25 3:03 PM, Vishal Aslot wrote:
>>>> Thanks for the quick review, Dave. Yes, forgot to put the commit message. :)
>>>> About the copyright message. Should I not be updating the year to 2025 in it?
>>>
>>> I wouldn't bother unless you are doing massive changes and in that case it would be adding NVidia copyright anyhow for you. Also, typically linux mailing lists prefer responding inline and >> not top posting.
>>
>> Ah, understood. Thanks for the mentoring.
>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>> ________________________________________
>>>> From: Dave Jiang <dave.jiang@intel.com>
>>>> Sent: Wednesday, October 1, 2025 4:28 PM
>>>> To: Vishal Aslot; Davidlohr Bueso; Jonathan Cameron; Alison Schofield; Vishal Verma; Ira Weiny; Dan Williams; Li Ming; Peter Zijlstra; Dan Carpenter; Zijun Hu; linux-cxl@vger.kernel.org
>>>> Cc: linux-kernel@vger.kernel.org
>>>> Subject: Re: [PATCH] cxl/hdm: allow zero sized committed decoders
>>>>
>>>> External email: Use caution opening links or attachments
>>>>
>>>>
>>>> On 10/1/25 1:37 PM, Vishal Aslot wrote:
>>>>> Signed-off-by: Vishal Aslot <vaslot@nvidia.com>
>>>>
>>>> Missing commit log?
>>
>> Yes, will add one in v2.
>>
>>>>>
>>>>> ---
>>>>>  drivers/cxl/core/hdm.c | 9 +++++++--
>>>>>  1 file changed, 7 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
>>>>> index e9e1d555cec6..97a5f27f5b72 100644
>>>>> --- a/drivers/cxl/core/hdm.c
>>>>> +++ b/drivers/cxl/core/hdm.c
>>>>> @@ -1,5 +1,5 @@
>>>>>  // SPDX-License-Identifier: GPL-2.0-only
>>>>> -/* Copyright(c) 2022 Intel Corporation. All rights reserved. */
>>>>> +/* Copyright(c) 2022-2025 Intel Corporation. All rights reserved. */
>>>>
>>>> Stray change?
>>
>> No, it was intentional but I understand that for this small update, I don't need to update copyright info. I'll undo this change in v2.
>>
>>>>
>>>>>  #include <linux/seq_file.h>
>>>>>  #include <linux/device.h>
>>>>>  #include <linux/delay.h>
>>>>> @@ -1050,7 +1050,7 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
>>>>>                         dev_warn(&port->dev,
>>>>>                                  "decoder%d.%d: Committed with zero size\n",
>>>>>                                  port->id, cxld->id);
>>>>
>>>> I wonder if we should make this dev_dbg() now that it's a valid case.
>>
>> Yes, makes sense. I'll change it to dev_dbg() in v2.
>>
>>>>
>>>>> -                       return -ENXIO;
>>>>> +                       return -ENOSPC;
>>>>>                 }
>>>>>                 port->commit_end = cxld->id;
>>>>>         } else {
>>>>> @@ -1210,6 +1210,11 @@ 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) {
>>>>> +                               put_device(&cxld->dev);
>>>>
>>>> Why put_device()? Should we enumerate this decoder instead of pretend it doesn't exist? essentially only a dev_set_name() and device_add()?
>>
>> That is a good point. The decoder has been committed so it should be accounted for in the OS.
>> I'll remove put_device() in v2.
>>
>>>>
>>>>> +                               rc = 0;
>>>>
>>>> Don't think this is needed since the continue will put it at start of the loop where rc gets written by init_hdm_decoder() return value.
>>
>> Agreed. I was being paranoid. Will remove it in v2.
>>
>>>>
>>>>> +                               continue;
>>>>> +                       }
>>>>>                         dev_warn(&port->dev,
>>>>>                                  "Failed to initialize decoder%d.%d\n",
>>>>>                                  port->id, i);
>>>>> --
>>>>> 2.34.1
>>>>

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

[Vishal Aslot]

> ________________________________________
> From: Dave Jiang <dave.jiang@intel.com>
> Sent: Friday, October 3, 2025 11:20 AM
> To: Vishal Aslot; Davidlohr Bueso; Jonathan Cameron; Alison Schofield; Vishal Verma; Ira Weiny; Dan Williams; Li Ming; Peter Zijlstra; Dan Carpenter; Zijun Hu; linux-cxl@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v2] cxl/hdm: allow zero sized committed decoders
> 
> External email: Use caution opening links or attachments
> 
> 
> On 10/2/25 5:59 PM, Vishal Aslot wrote:
>> init_hdm_decoder() fails with -ENXIO if a
>> committed HDM decoder has zero size.
>>
>> The CXL spec permits committing zero sized decoders.
>> See "8.2.4.20.12 Committing Decoder Programming".
>> It says,"It is legal for software to program Decoder
>> Size to 0 and commit it. Such a decoder will not
>> participate in HDM decode."
>>
>> This patch updates init_hdm_decoder() to return
>> -ENOSPC if the decoder is commited with zero-size.
>> The caller leaves the decoder allocated but does not
>> add it. It simply continues to the next decoder.
>>
>> Signed-off-by: Vishal Aslot <vaslot@nvidia.com>
>
> Hi Vishal,
> Next time please post the next version of patches as a new thread and not a response to a > previous discussion. It would make lore [1] viewing easier. Thanks!
>
> [1]: https://lore.kernel.org/linux-cxl/aN_cUPzzwUy-s36n@gourry-fedora-PF4VCD3F/T/#t
>
> DJ

Yep, will do. Sorry I should've checked before hand. 

>
>> ---
>>  drivers/cxl/core/hdm.c | 7 +++++--
>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> 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;
> +
>                 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;
> +                       }
>                         dev_warn(&port->dev,
>                                  "Failed to initialize decoder%d.%d\n",
>                                  port->id, i);
> --
> 2.34.1
>
>> ________________________________________
>> From: Vishal Aslot <vaslot@nvidia.com>
>> Sent: Wednesday, October 1, 2025 8:57 PM
>> To: Dave Jiang; Davidlohr Bueso; Jonathan Cameron; Alison Schofield; Vishal Verma; Ira Weiny; Dan Williams; Li Ming; Peter Zijlstra; Dan Carpenter; Zijun Hu; linux-cxl@vger.kernel.org
>> Cc: linux-kernel@vger.kernel.org
>> Subject: Re: [PATCH] cxl/hdm: allow zero sized committed decoders
>>
>> External email: Use caution opening links or attachments
>>
>>
>>> ________________________________________
>>> From: Dave Jiang <dave.jiang@intel.com>
>>> Sent: Wednesday, October 1, 2025 5:46 PM
>>> To: Vishal Aslot; Davidlohr Bueso; Jonathan Cameron; Alison Schofield; Vishal Verma; Ira Weiny; Dan Williams; Li Ming; Peter Zijlstra; Dan Carpenter; Zijun Hu; linux-cxl@vger.kernel.org
>>> Cc: linux-kernel@vger.kernel.org
>>> Subject: Re: [PATCH] cxl/hdm: allow zero sized committed decoders
>>>
>>> External email: Use caution opening links or attachments
>>>
>>>
>>> On 10/1/25 3:03 PM, Vishal Aslot wrote:
>>>> Thanks for the quick review, Dave. Yes, forgot to put the commit message. :)
>>>> About the copyright message. Should I not be updating the year to 2025 in it?
>>>
>>> I wouldn't bother unless you are doing massive changes and in that case it would be adding NVidia copyright anyhow for you. Also, typically linux mailing lists prefer responding inline and >> not top posting.
>>
>> Ah, understood. Thanks for the mentoring.
>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>> ________________________________________
>>>> From: Dave Jiang <dave.jiang@intel.com>
>>>> Sent: Wednesday, October 1, 2025 4:28 PM
>>>> To: Vishal Aslot; Davidlohr Bueso; Jonathan Cameron; Alison Schofield; Vishal Verma; Ira Weiny; Dan Williams; Li Ming; Peter Zijlstra; Dan Carpenter; Zijun Hu; linux-cxl@vger.kernel.org
>>>> Cc: linux-kernel@vger.kernel.org
>>>> Subject: Re: [PATCH] cxl/hdm: allow zero sized committed decoders
>>>>
>>>> External email: Use caution opening links or attachments
>>>>
>>>>
>>>> On 10/1/25 1:37 PM, Vishal Aslot wrote:
>>>>> Signed-off-by: Vishal Aslot <vaslot@nvidia.com>
>>>>
>>>> Missing commit log?
>>
>> Yes, will add one in v2.
>>
>>>>>
>>>>> ---
>>>>>  drivers/cxl/core/hdm.c | 9 +++++++--
>>>>>  1 file changed, 7 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
>>>>> index e9e1d555cec6..97a5f27f5b72 100644
>>>>> --- a/drivers/cxl/core/hdm.c
>>>>> +++ b/drivers/cxl/core/hdm.c
>>>>> @@ -1,5 +1,5 @@
>>>>>  // SPDX-License-Identifier: GPL-2.0-only
>>>>> -/* Copyright(c) 2022 Intel Corporation. All rights reserved. */
>>>>> +/* Copyright(c) 2022-2025 Intel Corporation. All rights reserved. */
>>>>
>>>> Stray change?
>>>
>>> No, it was intentional but I understand that for this small update, I don't need to update copyright info. I'll undo this change in v2.
>>>
>>>>>
>>>>>>  #include <linux/seq_file.h>
>>>>>>  #include <linux/device.h>
>>>>>>  #include <linux/delay.h>
>>>>>> @@ -1050,7 +1050,7 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
>>>>>>                         dev_warn(&port->dev,
>>>>>>                                  "decoder%d.%d: Committed with zero size\n",
>>>>>>                                  port->id, cxld->id);
>>>>>
>>>>> I wonder if we should make this dev_dbg() now that it's a valid case.
>>>
>>> Yes, makes sense. I'll change it to dev_dbg() in v2.
>>>
>>>>>
>>>>>> -                       return -ENXIO;
>>>>>> +                       return -ENOSPC;
>>>>>>                 }
>>>>>>                 port->commit_end = cxld->id;
>>>>>>         } else {
>>>>>> @@ -1210,6 +1210,11 @@ 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) {
>>>>>> +                               put_device(&cxld->dev);
>>>>>
>>>>> Why put_device()? Should we enumerate this decoder instead of pretend it doesn't exist? essentially only a dev_set_name() and device_add()?
>>>
>>> That is a good point. The decoder has been committed so it should be accounted for in the OS.
>>> I'll remove put_device() in v2.
>>
>>>>>
>>>>>> +                               rc = 0;
>>>>>
>>>>> Don't think this is needed since the continue will put it at start of the loop where rc gets written by init_hdm_decoder() return value.
>>>
>>> Agreed. I was being paranoid. Will remove it in v2.
>>>
>>>>>
>>>>>> +                               continue;
>>>>>> +                       }
>>>>>>                         dev_warn(&port->dev,
>>>>>>                                  "Failed to initialize decoder%d.%d\n",
>>>>>>                                  port->id, i);
>>>>>> --
>>>>>> 2.34.1
>>>>>

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

[Davidlohr Bueso]

On Fri, 03 Oct 2025, Vishal Aslot wrote:

>init_hdm_decoder() fails with -ENXIO if a
>committed HDM decoder has zero size.

This can be removed.

>
>The CXL spec permits committing zero sized decoders.

This is enough info, no need for the quote below.
And then add "Linux currently considers them an error".

>See "8.2.4.20.12 Committing Decoder Programming".
>It says,"It is legal for software to program Decoder
>Size to 0 and commit it. Such a decoder will not
>participate in HDM decode."
>
>This patch updates init_hdm_decoder() to return
>-ENOSPC if the decoder is commited with zero-size.

This is not needed in the changelog. Instead the reader
could be enlightened for potential reasons and use
cases of 0 sized decoders.

>The caller leaves the decoder allocated but does not
>add it. It simply continues to the next decoder.

Thanks,
Davidlohr

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

[Vishal Aslot]

> ________________________________________
> From: Gregory Price <gourry@gourry.net>
> 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@vger.kernel.org; linux-kernel@vger.kernel.org
> 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

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

[Vishal Aslot]

> ________________________________________
> From: Davidlohr Bueso <dave@stgolabs.net>
> Sent: Friday, October 3, 2025 7:06 PM
> To: Vishal Aslot
> Cc: Dave Jiang; Jonathan Cameron; Alison Schofield; Vishal Verma; Ira Weiny; Dan Williams; Li Ming; Peter Zijlstra; Dan Carpenter; Zijun Hu; linux-cxl@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v2] cxl/hdm: allow zero sized committed decoders
> 
> External email: Use caution opening links or attachments
>
>
> On Fri, 03 Oct 2025, Vishal Aslot wrote:
>
>>init_hdm_decoder() fails with -ENXIO if a
>>committed HDM decoder has zero size.
>
> This can be removed.

Thank you, Davidlohr, for the review. Much appreciated!
I will remove it in v3.

>
>>
>>The CXL spec permits committing zero sized decoders.
>
> This is enough info, no need for the quote below.
> And then add "Linux currently considers them an error".

Yep, will fix it in v3.

>
>> See "8.2.4.20.12 Committing Decoder Programming".
>> It says,"It is legal for software to program Decoder
>> Size to 0 and commit it. Such a decoder will not
>> participate in HDM decode."
>>
>> This patch updates init_hdm_decoder() to return
>> -ENOSPC if the decoder is commited with zero-size.
>
> This is not needed in the changelog. Instead the reader
> could be enlightened for potential reasons and use
> cases of 0 sized decoders.

Understood. Will update in v3.

>
>>The caller leaves the decoder allocated but does not
>>add it. It simply continues to the next decoder.
>
> Thanks,
> Davidlohr

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

[Vishal Aslot]

> ________________________________________
> From: Gregory Price <gourry@gourry.net>
> Sent: Friday, October 3, 2025 9:23 AM
> 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@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] cxl/hdm: allow zero sized committed decoders
>
> External email: Use caution opening links or attachments
>
>
> On Fri, Oct 03, 2025 at 01:03:16AM +0000, Vishal Aslot wrote:
>> > ________________________________________
>> > From: Dave Jiang <dave.jiang@intel.com>
>> > Sent: Thursday, October 2, 2025 10:32 AM
>> > To: Gregory Price; Vishal Aslot
>> > Cc: Davidlohr Bueso; Jonathan Cameron; Alison Schofield; Vishal Verma; Ira Weiny; Dan Williams; Li Ming; Peter Zijlstra; Dan Carpenter; Zijun Hu; linux-cxl@vger.kernel.org; linux-kernel@vger.kernel.org
>> > Subject: Re: [PATCH] cxl/hdm: allow zero sized committed decoders
>> >
>> > External email: Use caution opening links or attachments
>> >
>> >
>> > On 10/1/25 10:48 PM, Gregory Price wrote:
>> >> On Wed, Oct 01, 2025 at 08:37:26PM +0000, Vishal Aslot wrote:
>> >>> @@ -1210,6 +1210,11 @@ 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) {
>> >>> +                               put_device(&cxld->dev);
>> >>> +                               rc = 0;
>> >>> +                               continue;
>> >>> +                       }
>> >>
>> >> How do you suggest actually testing this? I briefly poked at this in
>> >> QEMU trying to commit decoders, but i found myself incapable of
>> >> exercising this path.
>>
>> I tested it locally with our BIOS (UEFI) where we commit and lock all decoders and
>> all except decoder 0 are zero-sized.
>>
>
> Ahhh, so are you saying that you will only ever observe the following
> (as an example)
>
> endpoint decoders...
> decoder2.0   ->  available and can be programmed
> decoder2.1   ->  size=0, locked
> ...
> decoder2.N   ->  size=0, locked
>
> or are you suggesting the following is valid:
>
> decoder2.0   ->  size=0, locked
> decoder2.1   ->  available and can be programmed
> ...
> decoder2.N   ->  available and can be programmed
>
> ~Gregory

The first case is what we've got. In our case, the HB has 4 decoders. The end point (a Montage card with single port) has two decoders. For both, we commit & lock all decoders and all decoders except decoder<port>.0 are zero-sized.

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

[Vishal Aslot]

> ________________________________________
> From: Dave Jiang <dave.jiang@intel.com>
> Sent: Friday, October 3, 2025 11:14 AM
> To: Vishal Aslot; Gregory Price
> Cc: Davidlohr Bueso; Jonathan Cameron; Alison Schofield; Vishal Verma; Ira Weiny; Dan Williams; Li Ming; Peter Zijlstra; Dan Carpenter; Zijun Hu; linux-cxl@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] cxl/hdm: allow zero sized committed decoders
>
> External email: Use caution opening links or attachments
>
>
> On 10/2/25 6:03 PM, Vishal Aslot wrote:
>>> ________________________________________
>>> From: Dave Jiang <dave.jiang@intel.com>
>>> Sent: Thursday, October 2, 2025 10:32 AM
>>> To: Gregory Price; Vishal Aslot
>>> Cc: Davidlohr Bueso; Jonathan Cameron; Alison Schofield; Vishal Verma; Ira Weiny; Dan Williams; Li Ming; Peter Zijlstra; Dan Carpenter; Zijun Hu; linux-cxl@vger.kernel.org; linux-kernel@vger.kernel.org
>>> Subject: Re: [PATCH] cxl/hdm: allow zero sized committed decoders
>>>
>>> External email: Use caution opening links or attachments
>>>
>>>
>>> On 10/1/25 10:48 PM, Gregory Price wrote:
>>>> On Wed, Oct 01, 2025 at 08:37:26PM +0000, Vishal Aslot wrote:
>>>>> @@ -1210,6 +1210,11 @@ 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) {
>>>>> +                               put_device(&cxld->dev);
>>>>> +                               rc = 0;
>>>>> +                               continue;
>>>>> +                       }
>>>>
>>>> How do you suggest actually testing this? I briefly poked at this in
>>>> QEMU trying to commit decoders, but i found myself incapable of
>>>> exercising this path.
>>
>> I tested it locally with our BIOS (UEFI) where we commit and lock all decoders and
>> all except decoder 0 are zero-sized.
>>
>>>
>>> It may be worthwhile adding a cxl_test test case for this.
>>
>> Yeah, sure. Would cxl mock tests be the right place to explore this?
>
> Yes. Under tools/testing/cxl. Let me know if you need help with that.
>
> DJ

Hi Dave,
    Would the test be a separate patch or part of this one? Thanks.
-Vishal

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

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

[Dave Jiang]

On 10/4/25 7:05 AM, Vishal Aslot wrote:
>> ________________________________________
>> From: Dave Jiang <dave.jiang@intel.com>
>> Sent: Friday, October 3, 2025 11:14 AM
>> To: Vishal Aslot; Gregory Price
>> Cc: Davidlohr Bueso; Jonathan Cameron; Alison Schofield; Vishal Verma; Ira Weiny; Dan Williams; Li Ming; Peter Zijlstra; Dan Carpenter; Zijun Hu; linux-cxl@vger.kernel.org; linux-kernel@vger.kernel.org
>> Subject: Re: [PATCH] cxl/hdm: allow zero sized committed decoders
>>
>> External email: Use caution opening links or attachments
>>
>>
>> On 10/2/25 6:03 PM, Vishal Aslot wrote:
>>>> ________________________________________
>>>> From: Dave Jiang <dave.jiang@intel.com>
>>>> Sent: Thursday, October 2, 2025 10:32 AM
>>>> To: Gregory Price; Vishal Aslot
>>>> Cc: Davidlohr Bueso; Jonathan Cameron; Alison Schofield; Vishal Verma; Ira Weiny; Dan Williams; Li Ming; Peter Zijlstra; Dan Carpenter; Zijun Hu; linux-cxl@vger.kernel.org; linux-kernel@vger.kernel.org
>>>> Subject: Re: [PATCH] cxl/hdm: allow zero sized committed decoders
>>>>
>>>> External email: Use caution opening links or attachments
>>>>
>>>>
>>>> On 10/1/25 10:48 PM, Gregory Price wrote:
>>>>> On Wed, Oct 01, 2025 at 08:37:26PM +0000, Vishal Aslot wrote:
>>>>>> @@ -1210,6 +1210,11 @@ 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) {
>>>>>> +                               put_device(&cxld->dev);
>>>>>> +                               rc = 0;
>>>>>> +                               continue;
>>>>>> +                       }
>>>>>
>>>>> How do you suggest actually testing this? I briefly poked at this in
>>>>> QEMU trying to commit decoders, but i found myself incapable of
>>>>> exercising this path.
>>>
>>> I tested it locally with our BIOS (UEFI) where we commit and lock all decoders and
>>> all except decoder 0 are zero-sized.
>>>
>>>>
>>>> It may be worthwhile adding a cxl_test test case for this.
>>>
>>> Yeah, sure. Would cxl mock tests be the right place to explore this?
>>
>> Yes. Under tools/testing/cxl. Let me know if you need help with that.
>>
>> DJ
> 
> Hi Dave,
>     Would the test be a separate patch or part of this one? Thanks.

It would be a separate patch and you post a series with a cover letter. You can see examples in the lore archive [1] of submissions more than 1 patches.

[1]: https://lore.kernel.org/linux-cxl/

DJ

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

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

[Gregory Price]

On Sat, Oct 04, 2025 at 01:49:26PM +0000, Vishal Aslot wrote:
> >>
> >
> > Ahhh, so are you saying that you will only ever observe the following
> > (as an example)
> >
> > endpoint decoders...
> > decoder2.0   ->  available and can be programmed
> > decoder2.1   ->  size=0, locked
> > ...
> > decoder2.N   ->  size=0, locked
> >
> > or are you suggesting the following is valid:
> >
> > decoder2.0   ->  size=0, locked
> > decoder2.1   ->  available and can be programmed
> > ...
> > decoder2.N   ->  available and can be programmed
> >
> > ~Gregory
> 
> The first case is what we've got. In our case, the HB has 4 decoders. The end point (a Montage card with single port) has two decoders. For both, we commit & lock all decoders and all decoders except decoder<port>.0 are zero-sized.

If the second case is not supported (I don't think it is), then it's
worth spelling this out.

I don't think either of these cases are relevant for scenarios outside
of the scenario where the host pre-configures the decoders either -
worth spelling that out as well.

Just in the commit log.

~Gregory