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

[Jonathan Cameron <Jonathan.Cameron@huawei.com>]

On Fri, 3 Jan 2025 07:16:51 +0000
Alejandro Lucero Palau <alucerop@amd.com> wrote:

> On 1/2/25 12:49, Jonathan Cameron wrote:
> >>>> diff --git a/include/cxl/cxl.h b/include/cxl/cxl.h
> >>>> index 19e5d883557a..f656fcd4945f 100644
> >>>> --- a/include/cxl/cxl.h
> >>>> +++ b/include/cxl/cxl.h
> >>>> @@ -12,6 +12,25 @@ enum cxl_resource {
> >>>>    	CXL_RES_PMEM,
> >>>>    };
> >>>>    
> >>>> +/* Capabilities as defined for:
> >>>> + *
> >>>> + *	Component Registers (Table 8-22 CXL 3.1 specification)
> >>>> + *	Device Registers (8.2.8.2.1 CXL 3.1 specification)
> >>>> + *
> >>>> + * and currently being used for kernel CXL support.
> >>>> + */
> >>>> +
> >>>> +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  
> >>> Why set it to 64?  All the bitmaps etc will autosize so
> >>> you just need to ensure you use correct set_bit() and test_bit()
> >>> that are happy dealing with bitmaps of multiple longs.
> >>>     
> >> Initially it was set to 32, but DECLARE_BITMAP uses unsigned long, so
> >> for initializing/zeroing the locally allocated bitmap in some functions,
> >> bitmap_clear had to use sizeof for the size, and I was suggested to
> >> define CXL_MAX_CAPS to 64 and use it instead, what seems cleaner.  
> > It should never have been using sizeof() once it was a bitmap.
> > Just clear what is actually used and make sure no code assumes
> > any particular length of bitmap.  Then you will never have
> > to deal with changing it.  
> 
> 
> The problem I had was to zeroing a locally allocated bitmap for avoiding 
> random bits set by the previous use of that memory.
> 
> The macros/functions like bitmap_clear or bitmap_zero require a start 
> and a number of bits, and I did not find any other way than using sizeof.

CXL_MAX_CAPS is fine, but set it to 5 (automatically by making it last
element in enum), not 64 which is made up value and gains you nothing that
I can see.   As you can see in the bitmap_zero implementation:

static __always_inline void bitmap_zero(unsigned long *dst, unsigned int nbits)
{
	unsigned int len = bitmap_size(nbits);

	if (small_const_nbits(nbits))
		*dst = 0;
	else
		memset(dst, 0, len);
}

If it fits in an unsigned long it will just do *dst = 0
which is what we want, but if the bitmap grows in future it will just
do the right thing.

No need for a magic 64 or anything else.

Jonathan

> 
> I was not happy with it, although it was fine for current needs of a 
> bitmap not bigger than unsigned long size. But I was told to use the 
> CXL_MAX_CAPS as currently implemented for using that for the zeroing.
> 
> 
> >
> > Then CXL_MAX_CAP just becomes last entry in this enum.
> >
> > The only time this is becomes tricky with bitmaps is if you need
> > to set a bits in a constant bitmap as then you can't use the
> > set/get functions and have to assume something about the length.
> >
> > Don't think that applies here.
> >
> > Jonathan
> >
> >     
> >>  
> >>>> +};
> >>>> +
> >>>>    struct cxl_dev_state *cxl_accel_state_create(struct device *dev);
> >>>>    
> >>>>    void cxl_set_dvsec(struct cxl_dev_state *cxlds, u16 dvsec);