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

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

> >> 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.

> 
> 
> Maybe I need an explicit action here.

J