[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <b69304e8-d43c-940c-9ee0-d2345ef7b99d@amd.com>
Date: Tue, 30 Jul 2024 07:10:25 +0100
From: Alejandro Lucero Palau <alucerop@....com>
To: Huang Ying <ying.huang@...el.com>, Dan Williams
<dan.j.williams@...el.com>, Dave Jiang <dave.jiang@...el.com>
Cc: linux-cxl@...r.kernel.org, linux-kernel@...r.kernel.org,
Davidlohr Bueso <dave@...olabs.net>,
Jonathan Cameron <jonathan.cameron@...wei.com>,
Alison Schofield <alison.schofield@...el.com>,
Vishal Verma <vishal.l.verma@...el.com>, Ira Weiny <ira.weiny@...el.com>
Subject: Re: [PATCH 0/3] cxl: Preparation of type2 accelerators support
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
Powered by blists - more mailing lists