Received-SPF: pass (google.com: domain of linux-kernel+bounces-75717-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Wed, 21 Feb 2024 16:20:57 -0800
From: Dan Williams <dan.j.williams@intel.com>
To: <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@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@huawei.com>, <prime.zeng@hisilicon.com>,
	<kangkang.shen@futurewei.com>, <wanghuiqiang@huawei.com>,
	<linuxarm@huawei.com>, <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
Message-ID: <65d6936952764_1138c7294e@dwillia2-xfh.jf.intel.com.notmuch>
References: <20240215111455.1462-1-shiju.jose@huawei.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20240215111455.1462-1-shiju.jose@huawei.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?EpvbccEJLAVpZwE7tyx4lzZq3oJJ3VNJaymVepFoCUKvsE3apSUKeA5rVhGc?=
 =?us-ascii?Q?4mw5pKIl6WgqeQT9tv+316GSLM6p8G5ZVj25FcrRIybzoXBXyEOdhSfEGci3?=
 =?us-ascii?Q?Ie/2pm9Y9Ge0w2CKOiC1suyHbxT0UHZkBsLQ4o3Y1m+rfBFfCrAeajtXmX3m?=
 =?us-ascii?Q?CjHjIAaVDZEUSbGHkkQAd0UocJOCSES2qlY3VHtD8f40iSnJEB9RXYWw/mur?=
 =?us-ascii?Q?zBbcl1ohW2Ic+ES1o4llSBioOqNpD9yEaY8ehNhFFihcVA4n7a7C3xEaoH7l?=
 =?us-ascii?Q?5ZolsZkzGrHLAi4nfsFNy/2Jos9vRG1VC+xxQ1WSSVfgEE/qcm0SGo9iCDKr?=
 =?us-ascii?Q?0t+yFQGkolq8NRJ8CzjtfvEnAMONcTXGweqgVlUZfMo4+QRfQIH4j1H4DR3M?=
 =?us-ascii?Q?XImMU+a1DU2AMOGiYnAENWWbqM70OOCkg+fkdAp817nMh/sanVar55nj/wXO?=
 =?us-ascii?Q?6Z3WvQ38rxoc7B3Jo++bW3ID9YrQG3so64dPR1tbm7fJrpLEww2SfUAQGtjW?=
 =?us-ascii?Q?CGprH76OTnwjK12P7kSZlHJ1rQxP5xZlsVvBlZcI6Q8zen+L9PNNVRxejYny?=
 =?us-ascii?Q?ArNpiH3CIp6sYS4r/6PtLM6MTnrc+AocMYjj+kZArJdkV3r45DJ0UVtIi3C4?=
 =?us-ascii?Q?RMvNfM8VzV6kpr62u+4culjYeNDTvx6wJIvuEEEfoMlcxKRQxD+RLEejwpBH?=
 =?us-ascii?Q?Cf7Y5nEO2/O14EdV0LQ12zS2Ct6YjWOa+7P/EnO+ditjolr1FHl+KEe7RytB?=
 =?us-ascii?Q?kTq1GXdAdkvRSmScPVZTVvUtQ1q9HdfBistTtILb26xTVXOjVS5LAen8nzWI?=
 =?us-ascii?Q?wUtz1dWdHRAWUk/UYsJHZHxgpHxvLKtKv3T8a5mlIY4uN+Zf0aFy7tMiVgSv?=
 =?us-ascii?Q?yHBiDdv9bTFH8pCDIj3C2UjmIjaL9pbGO9YHkeaCvylH7nAsUJTWgtTORZHL?=
 =?us-ascii?Q?QBeyH4QoXFAmIHI72MQkTsvgnyEDWo9RxjrZriWkC8gB6bRbaMhvpxGNoMzq?=
 =?us-ascii?Q?e/WeJsvoxhCZZ4jNAsBeqJ2FPYmbYoOeXgvh6IgbTt/mbfWX03vWo6Z/CA5M?=
 =?us-ascii?Q?96PlZTKD/p/+SAFNi/seVnUDZaDFnipZxyfcP4CJNMt12K5Mb32kHeXdrUH6?=
 =?us-ascii?Q?KkdUpW+zlXrJaRF7MdeGn486qrYwefuw14wtZBmreVqh/3liVoAhmwQhR5ex?=
 =?us-ascii?Q?mPgJ3RCKZ5lmYvXVeX9pfdlRnD5tI3InBMvJuThqhNY7lTpun2WeqgO6Ypy5?=
 =?us-ascii?Q?1L7vYNPhN5ydTFlzFRsgEjkpFshNmJrAb6MzruN3eAGZL6susItlJyFYUvk+?=
 =?us-ascii?Q?QUh9e4tIhYhAAmzdMCKDkve4N53RWNKTE5/CXktjacQsMu6ASulQcqMAHINH?=
 =?us-ascii?Q?9SCVtprHPJxx9PFD6/zopeFJeGgTWvmBg0IP7zz5RJ12aGVx+tK7RDB282++?=
 =?us-ascii?Q?oCvNpoBcB2M/DFJJNYB8KDufV90sOPvGp9WFwn3LnVCFVr0pFzKzSDI7nda6?=
 =?us-ascii?Q?LvyxyNVVgP3T4mAPtzlEwsrdjj/AGOh2NQuunhoFSZOgqYeJIFCcj1oOKTFJ?=
 =?us-ascii?Q?mq4fUl1CIJqBgQLgF+WIuSuRFof5vXMOVAehYp1ip2gCdZ48A3vFfneMZcwB?=
 =?us-ascii?Q?5Q=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 866d4238-d346-46e9-d267-08dc333c2678
X-MS-Exchange-CrossTenant-AuthSource: PH8PR11MB8107.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 22 Feb 2024 00:21:01.7924
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: ibRpuLehPJFuS3CdQjAIf2mWmOcFuyrotbN8iZ6Osl15XMb/1kaY8sVbkX9GtiEkYKO5iGtBOL1xBSMORrdpb2eBJe55vgxT4zsiv7zx6dY=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR11MB4705
X-OriginatorOrg: intel.com

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.

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 tradeoff is providing a migration path for legacy on
the way to adding the new thing. If old scrub implementations could be
deprecated / deleted on the way to supporting new scrub use cases that
becomes interesting.

[1]: http://lore.kernel.org/r/20240208220909.GA975234@bhelgaas
[2]: http://lore.kernel.org/r/20240208221305.GA975512@bhelgaas