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

[Huang, Ying]

Hi, Alejandro,

Alejandro Lucero Palau <alucerop@xxxxxxx> writes:

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

Sorry for confusing.  The patchset is just some preparation work for
type2 accelerator support.  Your patchset is more complete.  I just want
to relief your overhead a bit.

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

Yes.  To test this patchset, I need more complete type2 support.  I used
some patches from your and Dan's patchset on top of this patchset to do
the test.

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