The memory range of a type2 accelerator should be managed by the type2
accelerator specific driver instead of the common dax region drivers,
as discussed in [1].
[1] https://lore.kernel.org/linux-cxl/66469ff1b8fbc_2c2629427@dwillia2-xfh.jf.intel.com.notmuch/
So, the patch skips dax regions creation for type2 accelerator device
memory regions.
Based on: https://lore.kernel.org/linux-cxl/168592159835.1948938.1647215579839222774.stgit@dwillia2-xfh.jf.intel.com/
Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
Co-developed-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
Reviewed-by: Gregory Price <gourry@gourry.net>
Cc: Davidlohr Bueso <dave@stgolabs.net>
Cc: Jonathan Cameron <jonathan.cameron@huawei.com>
Cc: Dave Jiang <dave.jiang@intel.com>
Cc: Alison Schofield <alison.schofield@intel.com>
Cc: Vishal Verma <vishal.l.verma@intel.com>
Cc: Ira Weiny <ira.weiny@intel.com>
Cc: Alejandro Lucero <alucerop@amd.com>
Cc: Ben Cheatham <benjamin.cheatham@amd.com>
---
drivers/cxl/core/region.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index d709738ada61..708be236c9a2 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -3473,6 +3473,14 @@ static int cxl_region_probe(struct device *dev)
p->res->start, p->res->end, cxlr,
is_system_ram) > 0)
return 0;
+ /*
+ * Accelerator regions have specific usage, skip
+ * device-dax registration.
+ */
+ if (cxlr->type == CXL_DECODER_ACCEL)
+ return 0;
+
+ /* Expander routes to device-dax */
return devm_cxl_add_dax_region(cxlr);
default:
dev_dbg(&cxlr->dev, "unsupported region mode: %d\n",
--
2.39.2Huang Ying wrote:
> The memory range of a type2 accelerator should be managed by the type2
> accelerator specific driver instead of the common dax region drivers,
> as discussed in [1].
>
> [1] https://lore.kernel.org/linux-cxl/66469ff1b8fbc_2c2629427@dwillia2-xfh.jf.intel.com.notmuch/
>
> So, the patch skips dax regions creation for type2 accelerator device
> memory regions.
>
> Based on: https://lore.kernel.org/linux-cxl/168592159835.1948938.1647215579839222774.stgit@dwillia2-xfh.jf.intel.com/
>
> Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
> Co-developed-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> Reviewed-by: Gregory Price <gourry@gourry.net>
> Cc: Davidlohr Bueso <dave@stgolabs.net>
> Cc: Jonathan Cameron <jonathan.cameron@huawei.com>
> Cc: Dave Jiang <dave.jiang@intel.com>
> Cc: Alison Schofield <alison.schofield@intel.com>
> Cc: Vishal Verma <vishal.l.verma@intel.com>
> Cc: Ira Weiny <ira.weiny@intel.com>
> Cc: Alejandro Lucero <alucerop@amd.com>
> Cc: Ben Cheatham <benjamin.cheatham@amd.com>
> ---
> drivers/cxl/core/region.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index d709738ada61..708be236c9a2 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -3473,6 +3473,14 @@ static int cxl_region_probe(struct device *dev)
> p->res->start, p->res->end, cxlr,
> is_system_ram) > 0)
> return 0;
> + /*
> + * Accelerator regions have specific usage, skip
> + * device-dax registration.
> + */
> + if (cxlr->type == CXL_DECODER_ACCEL)
> + return 0;
> +
> + /* Expander routes to device-dax */
So Linux is this weird career choice where you get to argue with
yourself months later. I understand why I took this shortcut of assuming
that the coherence mode determines device-dax routing, but that is not
sufficient.
An HDM-DB region could want the device-dax uAPI, and an HDM-H range
could want to do its own uAPI besides device-dax.
So, in retrospect, I think it is a mistake to assume uAPI from coherence
mode. It really is a property of the region decided by the region
creator independent of the coherence mode or the device type.
I am thinking that 'struct cxl_region' grows something like:
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index 5406e3ab3d4a..4cf1d030404d 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -511,12 +511,25 @@ struct cxl_region_params {
*/
#define CXL_REGION_F_NEEDS_RESET 1
+/*
+ * enum cxl_mem_api - where to route this cxl region
+ * @CXL_MEM_API_DAX: application specific / soft-reserved memory
+ * @CXL_MEM_API_PMEM: direct region to the NVDIMM subsystem
+ * @CXL_MEM_API_NONE: accelerator specific / hard-reserved memory
+ */
+enum cxl_mem_api {
+ CXL_MEM_API_DAX,
+ CXL_MEM_API_PMEM,
+ CXL_MEM_API_NONE,
+};
+
/**
* struct cxl_region - CXL region
* @dev: This region's device
* @id: This region's id. Id is globally unique across all regions
* @mode: Endpoint decoder allocation / access mode
* @type: Endpoint decoder target type
+ * @api: What if any subsystem will present this region to consumers
* @cxl_nvb: nvdimm bridge for coordinating @cxlr_pmem setup / shutdown
* @cxlr_pmem: (for pmem regions) cached copy of the nvdimm bridge
* @flags: Region state flags
@@ -530,6 +543,7 @@ struct cxl_region {
int id;
enum cxl_decoder_mode mode;
enum cxl_decoder_type type;
+ enum cxl_mem_api api;
struct cxl_nvdimm_bridge *cxl_nvb;
struct cxl_pmem_region *cxlr_pmem;
unsigned long flags;
Now, I have not seen how Alejandro's series handles this, but this
type-2 series was shorter so I started here first.
Dan Williams <dan.j.williams@intel.com> writes:
> Huang Ying wrote:
>> The memory range of a type2 accelerator should be managed by the type2
>> accelerator specific driver instead of the common dax region drivers,
>> as discussed in [1].
>>
>> [1] https://lore.kernel.org/linux-cxl/66469ff1b8fbc_2c2629427@dwillia2-xfh.jf.intel.com.notmuch/
>>
>> So, the patch skips dax regions creation for type2 accelerator device
>> memory regions.
>>
>> Based on: https://lore.kernel.org/linux-cxl/168592159835.1948938.1647215579839222774.stgit@dwillia2-xfh.jf.intel.com/
>>
>> Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
>> Co-developed-by: Dan Williams <dan.j.williams@intel.com>
>> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
>> Reviewed-by: Gregory Price <gourry@gourry.net>
>> Cc: Davidlohr Bueso <dave@stgolabs.net>
>> Cc: Jonathan Cameron <jonathan.cameron@huawei.com>
>> Cc: Dave Jiang <dave.jiang@intel.com>
>> Cc: Alison Schofield <alison.schofield@intel.com>
>> Cc: Vishal Verma <vishal.l.verma@intel.com>
>> Cc: Ira Weiny <ira.weiny@intel.com>
>> Cc: Alejandro Lucero <alucerop@amd.com>
>> Cc: Ben Cheatham <benjamin.cheatham@amd.com>
>> ---
>> drivers/cxl/core/region.c | 8 ++++++++
>> 1 file changed, 8 insertions(+)
>>
>> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
>> index d709738ada61..708be236c9a2 100644
>> --- a/drivers/cxl/core/region.c
>> +++ b/drivers/cxl/core/region.c
>> @@ -3473,6 +3473,14 @@ static int cxl_region_probe(struct device *dev)
>> p->res->start, p->res->end, cxlr,
>> is_system_ram) > 0)
>> return 0;
>> + /*
>> + * Accelerator regions have specific usage, skip
>> + * device-dax registration.
>> + */
>> + if (cxlr->type == CXL_DECODER_ACCEL)
>> + return 0;
>> +
>> + /* Expander routes to device-dax */
>
> So Linux is this weird career choice where you get to argue with
> yourself months later. I understand why I took this shortcut of assuming
> that the coherence mode determines device-dax routing, but that is not
> sufficient.
>
> An HDM-DB region could want the device-dax uAPI, and an HDM-H range
> could want to do its own uAPI besides device-dax.
>
> So, in retrospect, I think it is a mistake to assume uAPI from coherence
> mode. It really is a property of the region decided by the region
> creator independent of the coherence mode or the device type.
Yes. It isn't a good idea to determine the region usage based on coherence
mode. In this series, clxr->type is used to designate
expander/accelerator instead of hostonly/dev, it makes more sense to
create device-dax for expander only by default.
> I am thinking that 'struct cxl_region' grows something like:
>
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 5406e3ab3d4a..4cf1d030404d 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -511,12 +511,25 @@ struct cxl_region_params {
> */
> #define CXL_REGION_F_NEEDS_RESET 1
>
> +/*
> + * enum cxl_mem_api - where to route this cxl region
> + * @CXL_MEM_API_DAX: application specific / soft-reserved memory
> + * @CXL_MEM_API_PMEM: direct region to the NVDIMM subsystem
> + * @CXL_MEM_API_NONE: accelerator specific / hard-reserved memory
> + */
> +enum cxl_mem_api {
> + CXL_MEM_API_DAX,
> + CXL_MEM_API_PMEM,
> + CXL_MEM_API_NONE,
> +};
> +
> /**
> * struct cxl_region - CXL region
> * @dev: This region's device
> * @id: This region's id. Id is globally unique across all regions
> * @mode: Endpoint decoder allocation / access mode
> * @type: Endpoint decoder target type
> + * @api: What if any subsystem will present this region to consumers
> * @cxl_nvb: nvdimm bridge for coordinating @cxlr_pmem setup / shutdown
> * @cxlr_pmem: (for pmem regions) cached copy of the nvdimm bridge
> * @flags: Region state flags
> @@ -530,6 +543,7 @@ struct cxl_region {
> int id;
> enum cxl_decoder_mode mode;
> enum cxl_decoder_type type;
> + enum cxl_mem_api api;
> struct cxl_nvdimm_bridge *cxl_nvb;
> struct cxl_pmem_region *cxlr_pmem;
> unsigned long flags;
This looks good to me. The usage is specified by the device driver explicitly.
> Now, I have not seen how Alejandro's series handles this, but this
> type-2 series was shorter so I started here first.
--
Best Regards,
Huang, Ying
I did comment on this some time ago and I'm doing it again. This is originally part of the type2 patchset, and I'm keeping it in V4. I do not understand why you pick code changes (you explicitly said that in the first RFC) from there and use it here, and without previous discussion about this necessity in the list. I do not think this is usual, at least in other kernel subsystems I'm more familiar with, so I will raise this in today's cxl open source collaboration sync. On 10/15/24 07:57, Huang Ying wrote: > The memory range of a type2 accelerator should be managed by the type2 > accelerator specific driver instead of the common dax region drivers, > as discussed in [1]. > > [1] https://lore.kernel.org/linux-cxl/66469ff1b8fbc_2c2629427@dwillia2-xfh.jf.intel.com.notmuch/ > > So, the patch skips dax regions creation for type2 accelerator device > memory regions. > > Based on: https://lore.kernel.org/linux-cxl/168592159835.1948938.1647215579839222774.stgit@dwillia2-xfh.jf.intel.com/ > > Signed-off-by: "Huang, Ying" <ying.huang@intel.com> > Co-developed-by: Dan Williams <dan.j.williams@intel.com> > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > Reviewed-by: Gregory Price <gourry@gourry.net> > Cc: Davidlohr Bueso <dave@stgolabs.net> > Cc: Jonathan Cameron <jonathan.cameron@huawei.com> > Cc: Dave Jiang <dave.jiang@intel.com> > Cc: Alison Schofield <alison.schofield@intel.com> > Cc: Vishal Verma <vishal.l.verma@intel.com> > Cc: Ira Weiny <ira.weiny@intel.com> > Cc: Alejandro Lucero <alucerop@amd.com> > Cc: Ben Cheatham <benjamin.cheatham@amd.com> > --- > drivers/cxl/core/region.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c > index d709738ada61..708be236c9a2 100644 > --- a/drivers/cxl/core/region.c > +++ b/drivers/cxl/core/region.c > @@ -3473,6 +3473,14 @@ static int cxl_region_probe(struct device *dev) > p->res->start, p->res->end, cxlr, > is_system_ram) > 0) > return 0; > + /* > + * Accelerator regions have specific usage, skip > + * device-dax registration. > + */ > + if (cxlr->type == CXL_DECODER_ACCEL) > + return 0; > + > + /* Expander routes to device-dax */ > return devm_cxl_add_dax_region(cxlr); > default: > dev_dbg(&cxlr->dev, "unsupported region mode: %d\n",
Hi, Alejandro, Alejandro Lucero Palau <alucerop@amd.com> writes: > I did comment on this some time ago and I'm doing it again. > > > This is originally part of the type2 patchset, and I'm keeping it in > V4. I do not understand why you pick code changes (you explicitly said > that in the first RFC) from there and use it here, and without > previous discussion about this necessity in the list. I do not think > this is usual, at least in other kernel subsystems I'm more familiar > with, so I will raise this in today's cxl open source collaboration > sync. No. I picked this change from Dan's series as follows, https://eclists.intel.com/sympa//arc/linux-bkc/2024-10/msg00018.html So, I added co-developed-by and signed-off-by of Dan. IIUC, your picked this change from Dan's series too? Feel free to include this change in your series. If your patchset is merged firstly, I will rebase on yours and drop this change. [snip] -- Best Regards, Huang, Ying
On 10/17/24 07:29, Huang, Ying wrote: > Hi, Alejandro, > > Alejandro Lucero Palau <alucerop@amd.com> writes: > >> I did comment on this some time ago and I'm doing it again. >> >> >> This is originally part of the type2 patchset, and I'm keeping it in >> V4. I do not understand why you pick code changes (you explicitly said >> that in the first RFC) from there and use it here, and without >> previous discussion about this necessity in the list. I do not think >> this is usual, at least in other kernel subsystems I'm more familiar >> with, so I will raise this in today's cxl open source collaboration >> sync. > No. I picked this change from Dan's series as follows, > > https://eclists.intel.com/sympa//arc/linux-bkc/2024-10/msg00018.html > > So, I added co-developed-by and signed-off-by of Dan. > > IIUC, your picked this change from Dan's series too? Look, this is not going well. You specifically said in your first patchset you considered the type2 support patchset complete but too large or complex, so you were taking parts of it as a prelude for making it easier to review/accept. Just face that and not twist the argument. FWIW, I'm against you doing so because: 1) You should have commented in the type2 patchset about your concern, and gave advice about doing such a prelude (by me) or offer yourself for doing it. 2) Just following your approach, anyone could do the same for any patchset sent to the list. This is not a good precedent. 3) If this is going to be allowed/approved, I'm not going to be comfortable within this community. If it is just me, I guess it will not be a big loss. None has commented yet except you and me, what I do not know if it is because this is a nasty discussion they do not want to get entangle with, or because they just think your approach is OK. If not further comment and your patchset is accepted, nothing else will be needed to say. > Feel free to include this change in your series. If your patchset is > merged firstly, I will rebase on yours and drop this change. > > [snip] > > -- > Best Regards, > Huang, Ying
Alejandro Lucero Palau <alucerop@amd.com> writes: > On 10/17/24 07:29, Huang, Ying wrote: >> Hi, Alejandro, >> >> Alejandro Lucero Palau <alucerop@amd.com> writes: >> >>> I did comment on this some time ago and I'm doing it again. >>> >>> >>> This is originally part of the type2 patchset, and I'm keeping it in >>> V4. I do not understand why you pick code changes (you explicitly said >>> that in the first RFC) from there and use it here, and without >>> previous discussion about this necessity in the list. I do not think >>> this is usual, at least in other kernel subsystems I'm more familiar >>> with, so I will raise this in today's cxl open source collaboration >>> sync. >> No. I picked this change from Dan's series as follows, >> >> https://eclists.intel.com/sympa//arc/linux-bkc/2024-10/msg00018.html >> >> So, I added co-developed-by and signed-off-by of Dan. >> >> IIUC, your picked this change from Dan's series too? > > > Look, this is not going well. > > > You specifically said in your first patchset you considered the type2 > support patchset complete but too large or complex, so you were taking > parts of it as a prelude for making it easier to review/accept. Just > face that and not twist the argument. Although I listed your patchset in my cover letter. All changes I picked was from Dan's patchset instead of yours. And, I kept Dan's co-developed-by and signed-off-by. If you will pick changes from Dan, please do that too. > FWIW, I'm against you doing so because: > > > 1) You should have commented in the type2 patchset about your concern, > and gave advice about doing such a prelude (by me) or offer yourself > for doing it. > > 2) Just following your approach, anyone could do the same for any > patchset sent to the list. This is not a good precedent. > > 3) If this is going to be allowed/approved, I'm not going to be > comfortable within this community. If it is just me, I guess it will > not be a big loss. > > > None has commented yet except you and me, what I do not know if it is > because this is a nasty discussion they do not want to get entangle > with, or because they just think your approach is OK. If not further > comment and your patchset is accepted, nothing else will be needed to > say. > > >> Feel free to include this change in your series. If your patchset is >> merged firstly, I will rebase on yours and drop this change. >> >> [snip] -- Best Regards, Huang, Ying
On Thu, 17 Oct 2024 15:48:04 +0800 "Huang, Ying" <ying.huang@intel.com> wrote: > Alejandro Lucero Palau <alucerop@amd.com> writes: > > > On 10/17/24 07:29, Huang, Ying wrote: > >> Hi, Alejandro, > >> > >> Alejandro Lucero Palau <alucerop@amd.com> writes: > >> > >>> I did comment on this some time ago and I'm doing it again. > >>> > >>> > >>> This is originally part of the type2 patchset, and I'm keeping it in > >>> V4. I do not understand why you pick code changes (you explicitly said > >>> that in the first RFC) from there and use it here, and without > >>> previous discussion about this necessity in the list. I do not think > >>> this is usual, at least in other kernel subsystems I'm more familiar > >>> with, so I will raise this in today's cxl open source collaboration > >>> sync. > >> No. I picked this change from Dan's series as follows, > >> > >> https://eclists.intel.com/sympa//arc/linux-bkc/2024-10/msg00018.html > >> > >> So, I added co-developed-by and signed-off-by of Dan. > >> > >> IIUC, your picked this change from Dan's series too? > > > > > > Look, this is not going well. Hi Alejandro + Huang, Ying This seems to be an unfortunate case of disconnected work on the same large problem and shows the need for more coordination. Note these are my personal responses to this, other maintainers and community members may well disagree! Alejandro had already clearly adopted the patches from Dan and taken a number of them forwards as part of his patch set. That had happened before Huang, Ying posted the RFC (which referenced Alejandro's work along side Dan's original series). The idea of trying to accelerate the process of upstreaming type 2 support by merging a few low hanging fruit is certainly one I think we can all get behind. However, it needs some coordination to avoid actually slowing down overall progress by both causing spread out reviews and divergence in direction + churn in the base on which the fuller sets are built. So Huang, Ying please work with Alejandro rather than continuing to evolve this set independently. Perhaps an initial step would be to figure out how to reorder Alejandro's series so that any work duplicated by this set is pulled to the front. That should make it easy to identify and discuss differences that have resulted from review of this series. At that point we can focus the review on those patches as the rest of the set continues to evolve. However, I would strongly suggest coordinating that work in order to avoid churning the code when Alejandro may be near to posting a new version of his fuller series. Whilst the precise way we have ended up with two sets changing the same code is unusual it is extremely common for multiple people to be working on the same code and coordination to be needed. Many of the regular sync calls / discord channels / irc etc are used to figure out how this can be done efficiently. Please use those channels here. If it would be useful to have an additional call or similar to ensure a fruitful collaboration then we can set one up. Finally I'll note that I'd have expected to see explicit discussion of how this series relates to Alejandro's set and a suggestion of how to move forwards in the cover letter and that would perhaps have either resolved Alejandro's concerns or at least publicly shown awareness of the issues this would cause for his work. Irrespective of the other reasons for such an intro text, whilst the CXL maintainers were at least somewhat aware, we always appreciate a reminder in a cover letter! Jonathan > > > > > > You specifically said in your first patchset you considered the type2 > > support patchset complete but too large or complex, so you were taking > > parts of it as a prelude for making it easier to review/accept. Just > > face that and not twist the argument. > > Although I listed your patchset in my cover letter. All changes I > picked was from Dan's patchset instead of yours. And, I kept Dan's > co-developed-by and signed-off-by. If you will pick changes from Dan, > please do that too. > > > FWIW, I'm against you doing so because: > > > > > > 1) You should have commented in the type2 patchset about your concern, > > and gave advice about doing such a prelude (by me) or offer yourself > > for doing it. > > > > 2) Just following your approach, anyone could do the same for any > > patchset sent to the list. This is not a good precedent. > > > > 3) If this is going to be allowed/approved, I'm not going to be > > comfortable within this community. If it is just me, I guess it will > > not be a big loss. > > > > > > None has commented yet except you and me, what I do not know if it is > > because this is a nasty discussion they do not want to get entangle > > with, or because they just think your approach is OK. If not further > > comment and your patchset is accepted, nothing else will be needed to > > say. > > > > > >> Feel free to include this change in your series. If your patchset is > >> merged firstly, I will rebase on yours and drop this change. > >> > >> [snip] > > -- > Best Regards, > Huang, Ying >
Hi, Jonathan, Jonathan Cameron <Jonathan.Cameron@Huawei.com> writes: > On Thu, 17 Oct 2024 15:48:04 +0800 > "Huang, Ying" <ying.huang@intel.com> wrote: > >> Alejandro Lucero Palau <alucerop@amd.com> writes: >> >> > On 10/17/24 07:29, Huang, Ying wrote: >> >> Hi, Alejandro, >> >> >> >> Alejandro Lucero Palau <alucerop@amd.com> writes: >> >> >> >>> I did comment on this some time ago and I'm doing it again. >> >>> >> >>> >> >>> This is originally part of the type2 patchset, and I'm keeping it in >> >>> V4. I do not understand why you pick code changes (you explicitly said >> >>> that in the first RFC) from there and use it here, and without >> >>> previous discussion about this necessity in the list. I do not think >> >>> this is usual, at least in other kernel subsystems I'm more familiar >> >>> with, so I will raise this in today's cxl open source collaboration >> >>> sync. >> >> No. I picked this change from Dan's series as follows, >> >> >> >> https://eclists.intel.com/sympa//arc/linux-bkc/2024-10/msg00018.html >> >> >> >> So, I added co-developed-by and signed-off-by of Dan. >> >> >> >> IIUC, your picked this change from Dan's series too? >> > >> > >> > Look, this is not going well. > > Hi Alejandro + Huang, Ying > > This seems to be an unfortunate case of disconnected work on the same > large problem and shows the need for more coordination. > Note these are my personal responses to this, other maintainers and > community members may well disagree! > > Alejandro had already clearly adopted the patches from Dan and taken a > number of them forwards as part of his patch set. That had > happened before Huang, Ying posted the RFC (which referenced > Alejandro's work along side Dan's original series). > > The idea of trying to accelerate the process of upstreaming type 2 > support by merging a few low hanging fruit is certainly one I think > we can all get behind. However, it needs some coordination to avoid > actually slowing down overall progress by both causing spread out > reviews and divergence in direction + churn in the base on which > the fuller sets are built. So Huang, Ying please work with Alejandro > rather than continuing to evolve this set independently. IMHO, it may be better to continue to review this small series and make it ready to be merged (or dropped) ASAP? I can put it as a high priority task. After all, this series begins with code or ideas from Dan. However, this is only my personal opinion, if community thinks that it isn't a good idea, I can drop this series, at least the part that duplicates with Alejandro's series. > Perhaps an initial step would be to figure out how to reorder Alejandro's > series so that any work duplicated by this set is pulled to the front. > That should make it easy to identify and discuss differences that > have resulted from review of this series. At that point we can focus > the review on those patches as the rest of the set continues to evolve. > However, I would strongly suggest coordinating that work in order to > avoid churning the code when Alejandro may be near to posting a new > version of his fuller series. > > Whilst the precise way we have ended up with two sets changing the same > code is unusual it is extremely common for multiple people to be working > on the same code and coordination to be needed. Many of the regular > sync calls / discord channels / irc etc are used to figure out how > this can be done efficiently. Please use those channels here. Yes. More coordination is good. > If it would be useful to have an additional call or similar to ensure > a fruitful collaboration then we can set one up. > > Finally I'll note that I'd have expected to see explicit discussion of > how this series relates to Alejandro's set and a suggestion of how to > move forwards in the cover letter and that would perhaps have either > resolved Alejandro's concerns or at least publicly shown awareness of the > issues this would cause for his work. > > Irrespective of the other reasons for such an intro text, whilst the > CXL maintainers were at least somewhat aware, we always appreciate > a reminder in a cover letter! > > Jonathan > > >> > >> > >> > You specifically said in your first patchset you considered the type2 >> > support patchset complete but too large or complex, so you were taking >> > parts of it as a prelude for making it easier to review/accept. Just >> > face that and not twist the argument. >> >> Although I listed your patchset in my cover letter. All changes I >> picked was from Dan's patchset instead of yours. And, I kept Dan's >> co-developed-by and signed-off-by. If you will pick changes from Dan, >> please do that too. >> >> > FWIW, I'm against you doing so because: >> > >> > >> > 1) You should have commented in the type2 patchset about your concern, >> > and gave advice about doing such a prelude (by me) or offer yourself >> > for doing it. >> > >> > 2) Just following your approach, anyone could do the same for any >> > patchset sent to the list. This is not a good precedent. >> > >> > 3) If this is going to be allowed/approved, I'm not going to be >> > comfortable within this community. If it is just me, I guess it will >> > not be a big loss. >> > >> > >> > None has commented yet except you and me, what I do not know if it is >> > because this is a nasty discussion they do not want to get entangle >> > with, or because they just think your approach is OK. If not further >> > comment and your patchset is accepted, nothing else will be needed to >> > say. >> > >> > >> >> Feel free to include this change in your series. If your patchset is >> >> merged firstly, I will rebase on yours and drop this change. >> >> >> >> [snip] >> -- Best Regards, Huang, Ying
© 2016 - 2024 Red Hat, Inc.