netdev - Re: [PATCH v2 01/15] cxl: add type2 device basic support

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: <37bb4ee4-9d5f-17f4-7311-5be97ca83c34@amd.com>
Date: Fri, 9 Aug 2024 08:26:38 +0100
From: Alejandro Lucero Palau <alucerop@....com>
To: Jonathan Cameron <Jonathan.Cameron@...wei.com>
Cc: Dave Jiang <dave.jiang@...el.com>, alejandro.lucero-palau@....com,
 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 01/15] cxl: add type2 device basic support


On 8/4/24 17:44, Jonathan Cameron wrote:
>>>> diff --git a/include/linux/cxl_accel_mem.h b/include/linux/cxl_accel_mem.h
>>>> new file mode 100644
>>>> index 000000000000..daf46d41f59c
>>>> --- /dev/null
>>>> +++ b/include/linux/cxl_accel_mem.h
>>>> @@ -0,0 +1,22 @@
>>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>>> +/* Copyright(c) 2024 Advanced Micro Devices, Inc. */
>>>> +
>>>> +#include <linux/cdev.h>
>>> Don't think this header is needed?
>>>   
>>>> +
>>>> +#ifndef __CXL_ACCEL_MEM_H
>>>> +#define __CXL_ACCEL_MEM_H
>>>> +
>>>> +enum accel_resource{
>>>> +	CXL_ACCEL_RES_DPA,
>>>> +	CXL_ACCEL_RES_RAM,
>>>> +	CXL_ACCEL_RES_PMEM,
>>>> +};
>>>> +
>>>> +typedef struct cxl_dev_state cxl_accel_state;
>>> Please use 'struct cxl_dev_state' directly. There's no good reason to hide the type.
>>
>> That is what I think I was told to do although not explicitly. There
>> were concerns in the RFC about accel drivers too loose for doing things
>> regarding CXL and somehow CXL core should keep control as much as
>> possible.  I was even thought I was being asked to implement auxbus with
>> the CXL part of an accel as an auxiliar device which should be bound to
>> a CXL core driver. Then Jonathan Cameron the only one explicitly giving
>> the possibility of the opaque approach and disadvising the auxbus idea.
> I wasn't thinking a typedef to hide it.
> More making all state accesses that are needed through accessor functions so
> that from the 'internals' become opaque to the accelerator code and
> we can radically change how things are structured internally with
> no impact to the (hopefully large number of) CXL accelerator drivers.
>
> So here, I'd just expect a
> struct cxl_device_state; forwards declaration.
>
> Or potentially one to a a different structure after refactors etc.


OK. It makes sense. I thought the concern was about external driver 
modules using the internal cxl structs.

This is the main point in this second patchset version, so if none else 
says the opposite during the next days, I will take it as the right move 
forward and send a new version 3 soon.

Thank you


>>
>> Maybe I need an explicit action here.
> J