| Message ID | 20240729084611.502889-1-ying.huang@intel.com |
|---|---|
| Headers | show |
| Series | cxl: Preparation of type2 accelerators support | expand |
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@dwillia2-xfh.jf.intel.com/ > [2] https://lore.kernel.org/linux-cxl/20240516081202.27023-1-alucerop@amd.com/ > > 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
Hi, Alejandro, Alejandro Lucero Palau <alucerop@amd.com> 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@dwillia2-xfh.jf.intel.com/ >> [2] https://lore.kernel.org/linux-cxl/20240516081202.27023-1-alucerop@amd.com/ >> >> 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