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@xxxxxxxxx> > > >Sent: 22 February 2024 00:21 > > >To: Shiju Jose <shiju.jose@xxxxxxxxxx>; linux-cxl@xxxxxxxxxxxxxxx; linux- > > >acpi@xxxxxxxxxxxxxxx; linux-mm@xxxxxxxxx; dan.j.williams@xxxxxxxxx; > > >dave@xxxxxxxxxxxx; Jonathan Cameron <jonathan.cameron@xxxxxxxxxx>; > > >dave.jiang@xxxxxxxxx; alison.schofield@xxxxxxxxx; vishal.l.verma@xxxxxxxxx; > > >ira.weiny@xxxxxxxxx > > >Cc: linux-edac@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; > > >david@xxxxxxxxxx; Vilas.Sridharan@xxxxxxx; leo.duran@xxxxxxx; > > >Yazen.Ghannam@xxxxxxx; rientjes@xxxxxxxxxx; jiaqiyan@xxxxxxxxxx; > > >tony.luck@xxxxxxxxx; Jon.Grimm@xxxxxxx; dave.hansen@xxxxxxxxxxxxxxx; > > >rafael@xxxxxxxxxx; lenb@xxxxxxxxxx; naoya.horiguchi@xxxxxxx; > > >james.morse@xxxxxxx; jthoughton@xxxxxxxxxx; somasundaram.a@xxxxxxx; > > >erdemaktas@xxxxxxxxxx; pgonda@xxxxxxxxxx; duenwen@xxxxxxxxxx; > > >mike.malvestuto@xxxxxxxxx; gthelen@xxxxxxxxxx; > > >wschwartz@xxxxxxxxxxxxxxxxxxx; dferguson@xxxxxxxxxxxxxxxxxxx; > > >tanxiaofei <tanxiaofei@xxxxxxxxxx>; Zengtao (B) <prime.zeng@xxxxxxxxxxxxx>; > > >kangkang.shen@xxxxxxxxxxxxx; wanghuiqiang <wanghuiqiang@xxxxxxxxxx>; > > >Linuxarm <linuxarm@xxxxxxxxxx>; Shiju Jose <shiju.jose@xxxxxxxxxx> > > >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@xxxxxxxxxx> > > >> > > >> 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@xxxxxxxxxx/#r > > and some old ones, > > https://patchwork.kernel.org/project/linux-arm-kernel/patch/CS1PR84MB0038718F49DBC0FF03919E1184390@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/ > > > > 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@xxxxxxxxxx/ > > 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
