netdev - Re: [PATCH v2 04/15] cxl: add capabilities field to cxl_dev

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <087e4326-4473-9dda-d19b-6a3118f321c4@amd.com>
Date: Thu, 15 Aug 2024 16:43:48 +0100
From: Alejandro Lucero Palau <alucerop@....com>
To: Jonathan Cameron <Jonathan.Cameron@...wei.com>,
 alejandro.lucero-palau@....com
Cc: linux-cxl@...r.kernel.org, netdev@...r.kernel.org,
 dan.j.williams@...el.com, martin.habets@...inx.com, edward.cree@....com,
 davem@...emloft.net, kuba@...nel.org, pabeni@...hat.com,
 edumazet@...gle.com, richard.hughes@....com
Subject: Re: [PATCH v2 04/15] cxl: add capabilities field to cxl_dev_state


On 8/4/24 18:22, Jonathan Cameron wrote:
> On Mon, 15 Jul 2024 18:28:24 +0100
> alejandro.lucero-palau@....com wrote:
>
>> From: Alejandro Lucero <alucerop@....com>
>>
>> Type2 devices have some Type3 functionalities as optional like an mbox
>> or an hdm decoder, and CXL core needs a way to know what a CXL accelerator
>> implements.
>>
>> Add a new field for keeping device capabilities to be initialised by
>> Type2 drivers. Advertise all those capabilities for Type3.
>>
>> Signed-off-by: Alejandro Lucero <alucerop@....com>
> In general seems a reasonable approach, so just minor comments.
>
>> ---
>>   drivers/cxl/core/mbox.c            |  1 +
>>   drivers/cxl/core/memdev.c          |  4 +++-
>>   drivers/cxl/core/port.c            |  2 +-
>>   drivers/cxl/core/regs.c            | 11 ++++++-----
>>   drivers/cxl/cxl.h                  |  2 +-
>>   drivers/cxl/cxlmem.h               |  4 ++++
>>   drivers/cxl/pci.c                  | 15 +++++++++------
>>   drivers/net/ethernet/sfc/efx_cxl.c |  3 ++-
>>   include/linux/cxl_accel_mem.h      |  5 ++++-
>>   9 files changed, 31 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
>> index 2626f3fff201..2ba7d36e3f38 100644
>> --- a/drivers/cxl/core/mbox.c
>> +++ b/drivers/cxl/core/mbox.c
>> @@ -1424,6 +1424,7 @@ struct cxl_memdev_state *cxl_memdev_state_create(struct device *dev)
>>   	mds->cxlds.reg_map.host = dev;
>>   	mds->cxlds.reg_map.resource = CXL_RESOURCE_NONE;
>>   	mds->cxlds.type = CXL_DEVTYPE_CLASSMEM;
>> +	mds->cxlds.capabilities = CXL_DRIVER_CAP_HDM | CXL_DRIVER_CAP_MBOX;
> Add a reference for this perhaps.  Make it clear that a type3 device must
> support mailbox and hdm by pointing at requirement for the various structures
> in a spec reference.
>

I think it would be worth to have documentation, distilling out 
dis-ambiguities from the specs about mandatory/optional registers.


>> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
>> index af8169ccdbc0..8f2a820bd92d 100644
>> --- a/drivers/cxl/cxlmem.h
>> +++ b/drivers/cxl/cxlmem.h
>> @@ -405,6 +405,9 @@ struct cxl_dpa_perf {
>>   	int qos_class;
>>   };
>>   
>> +#define CXL_DRIVER_CAP_HDM	0x1
>> +#define CXL_DRIVER_CAP_MBOX	0x2
>> +
> Enum and BIT() for the defines.  Avoids someone in future
> thinking they can define 0x3 to be something.
>
> Definitely only one definition as well. Seems reasonable for
> this to be CXL wide.
>

OK.

Thanks!


>>   /**
>>    * struct cxl_dev_state - The driver device state
>>    *
>> @@ -438,6 +441,7 @@ struct cxl_dev_state {
>>   	struct resource ram_res;
>>   	u64 serial;
>>   	enum cxl_devtype type;
>> +	uint8_t capabilities;
>>   };