Re: [PATCH 0/3] cxl: Preparation of type2 accelerators support

[Alejandro Lucero Palau]

Hi,


I'm a bit "surprised" by this patchset. As you rightly say in this cover 
letter, there is a patchset under review, and I have not seen you 
commenting there on the concerns stated in this cover letter.


I would say that is the first thing you should do, at least, to comment 
there, suggest to do other way, pointing to other needed changes, and if 
things do not go well for reaching an agreement, then a patchset like 
this could make sense exposing another way.


Anyway, I think a CXL maintainer should say something about it, but 
after 24hours, I had to say something.


About testing these changes, I wonder how did you proceed. If you have 
used an emulated Type2 device, as I did with the first patchset version, 
you should trigger some of the problems I found, what makes any Type2 
initialization for getting a memdev and finally a cxl region to fail. In 
other words, testing these changes can not be a partly initialised Type2 
but a complete one. This does not mean a patch fixing a known and 
obvious issue for supporting Type2 should not be approved if not tested, 
since the Type2 support is not yet there, but mentioning testing makes 
things confusing, at least for me.


On 7/29/24 09:46, Huang Ying wrote:
> There have been 2 series to add type 2 accelerators support in [1] and [2].
>
> [1] https://lore.kernel.org/linux-cxl/168592149709.1948938.8663425987110396027.stgit@xxxxxxxxxxxxxxxxxxxxxxxxx/
> [2] https://lore.kernel.org/linux-cxl/20240516081202.27023-1-alucerop@xxxxxxx/
>
> Both provide relative complete support, but both are long too.  To
> make it easier to review, some preparation of type2 accelerators
> support is implemented in this series.  More complete support can be
> implemented based on it.
>
> This series has been tested with cxl_test via mocking a type2
> accelerator as in [1] above.  Because more type2 accelerators support
> than that provided by this series is needed to simulate the device,
> the cxl_test patch isn't included in the series.
>
> Huang Ying (3):
>    cxl: Set target type of root decoder based on CFMWS restrictions
>    cxl: Set target type of region with that of root decoder
>    cxl: Avoid to create dax regions for type2 accelerators
>
>   drivers/cxl/acpi.c        |  5 ++++-
>   drivers/cxl/core/region.c | 11 ++++++++++-
>   2 files changed, 14 insertions(+), 2 deletions(-)
>
> Best Regards,
> Huang, Ying