Re: [PATCH v9 03/27] cxl: add capabilities field to cxl_dev_state and cxl_port

On 1/3/25 10:50, Jonathan Cameron wrote:

On Fri, 3 Jan 2025 07:20:48 +0000
Alejandro Lucero Palau [off-list ref] wrote:

quoted

On 1/2/25 14:36, Jonathan Cameron wrote:

quoted

On Mon, 30 Dec 2024 21:44:21 +0000
[off-list ref] wrote:
  

quoted

From: Alejandro Lucero <redacted>

Type2 devices have some Type3 functionalities as optional like an mbox
or an hdm decoder, and CXL core needs a way to know what an CXL accelerator
implements.

Add a new field to cxl_dev_state for keeping device capabilities as
discovered during initialization. Add same field to cxl_port as registers
discovery is also used during port initialization.

Signed-off-by: Alejandro Lucero <redacted>
Reviewed-by: Ben Cheatham <redacted>
Reviewed-by: Fan Ni <redacted>

Comment in thread on v8.  I don't see a reason to have any specific
bitmap length - just use a final entry in the enum without a value set
to let us know how long it actually is.

I could do this but it implies to clear/zeroing the bitmaps with the
final entry value and to mask bitmaps with that when comparing them.

Yes but that is automatic if you use the bitmap functions throughout.

Oh, that is true.

quoted

I tried to avoid the masking, and it led to that use of sizeof and then
CXL_MAX_CAPS=64.

Don't avoid it. You are creating maintenance pain for a bit of unnecessary
micro optimization.  Just make sure to treat this bitmap as a bitmap
in all paths and there will be not reason for a reviewer to ever have
to care what this value is and whether enough bits are zero etc.

I'm afraid I have been in the wrong path regarding the use of the bitmap API.

I even initially implemented my own bitmap_subset because I did not notice there was one already there, and I think that was the starting point of this chain of bad decisions. Because I did implement it poorly it led to keep with the wrong assumptions when I was told to use the existing one.


Happy to have you pointing this out. I'll fix it.


Thank you!

Jonathan

quoted

quoted

Using the bit / bitmap functions should work fine without constraining
that to any particular value - also allowing for greater than 64 entries
with no need to fix up call sites etc.

  

quoted

diff --git a/drivers/cxl/core/regs.c b/drivers/cxl/core/regs.c
index 59cb35b40c7e..144ae9eb6253 100644
--- a/drivers/cxl/core/regs.c
+++ b/drivers/cxl/core/regs.c

@@ -4,6 +4,7 @@
+enum cxl_dev_cap {
+	/* capabilities from Component Registers */
+	CXL_DEV_CAP_RAS,
+	CXL_DEV_CAP_HDM,
+	/* capabilities from Device Registers */
+	CXL_DEV_CAP_DEV_STATUS,
+	CXL_DEV_CAP_MAILBOX_PRIMARY,
+	CXL_DEV_CAP_MEMDEV,
+	CXL_MAX_CAPS = 64

As in v8. I'm not seeing any reason for this.  If you need
a bitmap to be a particular number of unsigned longs, then that
code should be fixed. (only exception being compile time constant
bitmaps where this is tricky to do!)

Obviously I replied with that to v8 after you posted this
so time machines aside no way you could have acted on it yet.


Jonathan
  

quoted

+};
+
   struct cxl_dev_state;
   struct device;