Received-SPF: pass (google.com: domain of linux-kernel+bounces-85787-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Sender: John Groves <grovesaustin@gmail.com>
From: John Groves <John@Groves.net>
Date: Wed, 28 Feb 2024 16:26:56 -0600
To: Dan Williams <dan.j.williams@intel.com>
Cc: Shiju Jose <shiju.jose@huawei.com>, 
	"linux-cxl@vger.kernel.org" <linux-cxl@vger.kernel.org>, "linux-acpi@vger.kernel.org" <linux-acpi@vger.kernel.org>, 
	"linux-mm@kvack.org" <linux-mm@kvack.org>, "dave@stgolabs.net" <dave@stgolabs.net>, 
	Jonathan Cameron <jonathan.cameron@huawei.com>, "dave.jiang@intel.com" <dave.jiang@intel.com>, 
	"alison.schofield@intel.com" <alison.schofield@intel.com>, "vishal.l.verma@intel.com" <vishal.l.verma@intel.com>, 
	"ira.weiny@intel.com" <ira.weiny@intel.com>, "linux-edac@vger.kernel.org" <linux-edac@vger.kernel.org>, 
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, "david@redhat.com" <david@redhat.com>, 
	"Vilas.Sridharan@amd.com" <Vilas.Sridharan@amd.com>, "leo.duran@amd.com" <leo.duran@amd.com>, 
	"Yazen.Ghannam@amd.com" <Yazen.Ghannam@amd.com>, "rientjes@google.com" <rientjes@google.com>, 
	"jiaqiyan@google.com" <jiaqiyan@google.com>, "tony.luck@intel.com" <tony.luck@intel.com>, 
	"Jon.Grimm@amd.com" <Jon.Grimm@amd.com>, "dave.hansen@linux.intel.com" <dave.hansen@linux.intel.com>, 
	"rafael@kernel.org" <rafael@kernel.org>, "lenb@kernel.org" <lenb@kernel.org>, 
	"naoya.horiguchi@nec.com" <naoya.horiguchi@nec.com>, "james.morse@arm.com" <james.morse@arm.com>, 
	"jthoughton@google.com" <jthoughton@google.com>, "somasundaram.a@hpe.com" <somasundaram.a@hpe.com>, 
	"erdemaktas@google.com" <erdemaktas@google.com>, "pgonda@google.com" <pgonda@google.com>, 
	"duenwen@google.com" <duenwen@google.com>, "mike.malvestuto@intel.com" <mike.malvestuto@intel.com>, 
	"gthelen@google.com" <gthelen@google.com>, "wschwartz@amperecomputing.com" <wschwartz@amperecomputing.com>, 
	"dferguson@amperecomputing.com" <dferguson@amperecomputing.com>, tanxiaofei <tanxiaofei@huawei.com>, 
	"Zengtao (B)" <prime.zeng@hisilicon.com>, "kangkang.shen@futurewei.com" <kangkang.shen@futurewei.com>, 
	wanghuiqiang <wanghuiqiang@huawei.com>, Linuxarm <linuxarm@huawei.com>
Subject: Re: [RFC PATCH v6 00/12] cxl: Add support for CXL feature commands,
 CXL device patrol scrub control and DDR5 ECS control features
Message-ID: <7r4vrkmpma7u7zkzanuame7q4vl4ourygnyww4muzqjfvwvu3y@qkwmmoy6jflr>
References: <20240215111455.1462-1-shiju.jose@huawei.com>
 <65d6936952764_1138c7294e@dwillia2-xfh.jf.intel.com.notmuch>
 <54c55412e9374e4e9cacf8410a5a98cb@huawei.com>
 <65d8f5201f8cc_2509b29467@dwillia2-mobl3.amr.corp.intel.com.notmuch>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <65d8f5201f8cc_2509b29467@dwillia2-mobl3.amr.corp.intel.com.notmuch>

On 24/02/23 11:42AM, Dan Williams wrote:
> Shiju Jose wrote:
> > Hi Dan,
> > 
> > Thanks for the feedback.
> > 
> > Please find reply inline.
> > 
> > >-----Original Message-----
> > >From: Dan Williams <dan.j.williams@intel.com>
> > >Sent: 22 February 2024 00:21
> > >To: Shiju Jose <shiju.jose@huawei.com>; linux-cxl@vger.kernel.org; linux-
> > >acpi@vger.kernel.org; linux-mm@kvack.org; dan.j.williams@intel.com;
> > >dave@stgolabs.net; Jonathan Cameron <jonathan.cameron@huawei.com>;
> > >dave.jiang@intel.com; alison.schofield@intel.com; vishal.l.verma@intel.com;
> > >ira.weiny@intel.com
> > >Cc: linux-edac@vger.kernel.org; linux-kernel@vger.kernel.org;
> > >david@redhat.com; Vilas.Sridharan@amd.com; leo.duran@amd.com;
> > >Yazen.Ghannam@amd.com; rientjes@google.com; jiaqiyan@google.com;
> > >tony.luck@intel.com; Jon.Grimm@amd.com; dave.hansen@linux.intel.com;
> > >rafael@kernel.org; lenb@kernel.org; naoya.horiguchi@nec.com;
> > >james.morse@arm.com; jthoughton@google.com; somasundaram.a@hpe.com;
> > >erdemaktas@google.com; pgonda@google.com; duenwen@google.com;
> > >mike.malvestuto@intel.com; gthelen@google.com;
> > >wschwartz@amperecomputing.com; dferguson@amperecomputing.com;
> > >tanxiaofei <tanxiaofei@huawei.com>; Zengtao (B) <prime.zeng@hisilicon.com>;
> > >kangkang.shen@futurewei.com; wanghuiqiang <wanghuiqiang@huawei.com>;
> > >Linuxarm <linuxarm@huawei.com>; Shiju Jose <shiju.jose@huawei.com>
> > >Subject: RE: [RFC PATCH v6 00/12] cxl: Add support for CXL feature commands,
> > >CXL device patrol scrub control and DDR5 ECS control features
> > >
> > >shiju.jose@ wrote:
> > >> From: Shiju Jose <shiju.jose@huawei.com>
> > >>
> > >> 1. Add support for CXL feature mailbox commands.
> > >> 2. Add CXL device scrub driver supporting patrol scrub control and ECS
> > >> control features.
> > >> 3. Add scrub subsystem driver supports configuring memory scrubs in the
> > >system.
> > >> 4. Register CXL device patrol scrub and ECS with scrub subsystem.
> > >> 5. Add common library for RASF and RAS2 PCC interfaces.
> > >> 6. Add driver for ACPI RAS2 feature table (RAS2).
> > >> 7. Add memory RAS2 driver and register with scrub subsystem.
> > >
> > >I stepped away from this patch set to focus on the changes that landed for v6.8
> > >and the follow-on regression fixups. Now that v6.8 CXL work has quieted down
> > >and I circle back to this set for v6.9 I find the lack of story in this cover letter to
> > >be unsettling. As a reviewer I should not have to put together the story on why
> > >Linux should care about this feature and independently build up the
> > >maintainence-burden vs benefit tradeoff analysis.
> > I will add more details to the cover letter.
> >  
> > >
> > >Maybe it is self evident to others, but for me there is little in these changelogs
> > >besides "mechanism exists, enable it". There are plenty of platform or device
> > >mechanisms that get specified that Linux does not enable for one reason or
> > >another.
> > >
> > >The cover letter needs to answer why it matters, and what are the tradeoffs.
> > >Mind you, in my submissions I do not always get this right in the cover letter [1],
> > >but hopefully at least one of the patches tells the story [2].
> > >
> > >In other words, imagine you are writing the pull request to Linus or someone
> > >else with limited time who needs to make a risk decision on a pull request with a
> > >diffstat of:
> > >
> > >    23 files changed, 3083 insertions(+)
> > >
> > >...where the easiest decision is to just decline. As is, these changelogs are not
> > >close to tipping the scale to "accept".
> > >
> > >[sidebar: how did this manage to implement a new subsystem with 2 consumers
> > >(CXL + ACPI), without modifying a single existing line? Zero deletions? That is
> > >either an indication that Linux perfectly anticipated this future use case
> > >(unlikely), or more work needs to be done to digest an integrate these concepts
> > >into existing code paths]
> > >
> > >One of the first questions for me is why CXL and RAS2 as the first consumers and
> > >not NVDIMM-ARS and/or RASF Patrol Scrub? Part of the maintenance burden
> > We don't personally care about NVDIMMS but would welcome drivers from others.
> 
> Upstream would also welcome consideration of maintenance burden
> reduction before piling on, at least include *some* consideration of the
> implications vs this response that comes off as "that's somebody else's
> problem".
> 
> > Regarding RASF patrol scrub no one cared about it as it's useless and
> > any new implementation should be RAS2.
> 
> The assertion that "RASF patrol scrub no one cared about it as it's
> useless and any new implementation should be RAS2" needs evidence.
> 
> For example, what platforms are going to ship with RAS2 support, what
> are the implications of Linux not having RAS2 scrub support in a month,
> or in year? There are parts of the ACPI spec that have never been
> implemented what is the evidence that RAS2 is not going to suffer the
> same fate as RASF? There are parts of the CXL specification that have
> never been implemented in mass market products.
> 
> > Previous discussions in the community about RASF and scrub could be find here.
> > https://lore.kernel.org/lkml/20230915172818.761-1-shiju.jose@huawei.com/#r
> > and some old ones,
> > https://patchwork.kernel.org/project/linux-arm-kernel/patch/CS1PR84MB0038718F49DBC0FF03919E1184390@CS1PR84MB0038.NAMPRD84.PROD.OUTLOOK.COM/
> > 
> 
> Do not make people hunt for old discussions, if there are useful points
> in that discussion that make the case for the patch set include those in
> the next submission, don't make people hunt for the latest state of the
> story.
> 
> > https://lore.kernel.org/all/20221103155029.2451105-1-jiaqiyan@google.com/
> 
> Yes, now that is a useful changelog, thank you for highlighting it,
> please follow its example.

Just a comment that is not directed at the implementation details: at Micron we
see demand for the scrub control feature, so we do hope to see this support
go in sooner rather than later.

Regards,
John