Re: [PATCH v18 00/19] EDAC: Scrub: introduce generic EDAC RAS control feature driver + CXL/ACPI-RAS2 drivers

[Jonathan Cameron <Jonathan.Cameron@huawei.com>]

> >    
> > 5. CXL features driver supporting ECS control feature.
> > 6. ACPI RAS2 driver adds OS interface for RAS2 communication through
> >    PCC mailbox and extracts ACPI RAS2 feature table (RAS2) and
> >    create platform device for the RAS memory features, which binds
> >    to the memory ACPI RAS2 driver.
> > 7. Memory ACPI RAS2 driver gets the PCC subspace for communicating
> >    with the ACPI compliant platform supports ACPI RAS2. Add callback
> >    functions and registers with EDAC device to support user to
> >    control the HW patrol scrubbers exposed to the kernel via the
> >    ACPI RAS2 table.
> > 8. Support for CXL maintenance mailbox command, which is used by
> >    CXL device memory repair feature.   
> > 9. CXL features driver supporting PPR control feature.
> > 10. CXL features driver supporting memory sparing control feature.
> >     Note: There are other PPR, memory sparing drivers to come.  
> 
> The text above should be inside Documentation, and not on patch 0.
> 
> A big description like that makes hard to review this series. It is
> also easier to review the text after having it parsed by kernel doc
> build specially for summary tables like the "Comparison of scrubbing 
> features", which deserves ReST links processed by Sphinx to the 
> corresponding definitions of the terms that are be compared there.

Whilst I fully agree that having a huge cover letter makes for a burden
for any reviewer coming to the series, this is here at specific request
of reviewers.  We can look at keeping more of it in documentation though
it's a bit white paper like in comparison with what I'd normally expect
to see in kernel documentation.

> 
> > Open Questions based on feedbacks from the community:
> > 1. Leo: Standardize unit for scrub rate, for example ACPI RAS2 does not define
> >    unit for the scrub rate. RAS2 clarification needed.   
> 
> I noticed the same when reviewing a patch series for rasdaemon. Ideally,
> ACPI requires an errata defining what units are expected for scrub rate.

There is a code first ACPI ECN that indeed adds units.  That is accepted
for next ACPI specification release.

Seems the tianocore bugzilla is unhelpfully down for a migration
but it should be id 1013 at bugzilla.tianocore.com

That adds a detailed description of what the scrub rate settings mean but
we may well still have older platforms where the scaling is arbitrary.
The units defined are sufficient to map to whatever presentation we like.

> 
> While ACPI doesn't define it, better to not add support for it - or be
> conservative using a low granularity for it (like using minutes instead 
> of hours).

I don't mind changing this, though for systems we are aware of default scrub
is typically once or twice in 24 hours.

Jonathan