Message-ID: <20250109142433.00004ea7@huawei.com>
Date: Thu, 9 Jan 2025 14:24:33 +0000
From: Jonathan Cameron <Jonathan.Cameron@...wei.com>
To: Borislav Petkov <bp@...en8.de>
CC: Shiju Jose <shiju.jose@...wei.com>, "linux-edac@...r.kernel.org"
	<linux-edac@...r.kernel.org>, "linux-cxl@...r.kernel.org"
	<linux-cxl@...r.kernel.org>, "linux-acpi@...r.kernel.org"
	<linux-acpi@...r.kernel.org>, "linux-mm@...ck.org" <linux-mm@...ck.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"tony.luck@...el.com" <tony.luck@...el.com>, "rafael@...nel.org"
	<rafael@...nel.org>, "lenb@...nel.org" <lenb@...nel.org>,
	"mchehab@...nel.org" <mchehab@...nel.org>, "dan.j.williams@...el.com"
	<dan.j.williams@...el.com>, "dave@...olabs.net" <dave@...olabs.net>,
	"dave.jiang@...el.com" <dave.jiang@...el.com>, "alison.schofield@...el.com"
	<alison.schofield@...el.com>, "vishal.l.verma@...el.com"
	<vishal.l.verma@...el.com>, "ira.weiny@...el.com" <ira.weiny@...el.com>,
	"david@...hat.com" <david@...hat.com>, "Vilas.Sridharan@....com"
	<Vilas.Sridharan@....com>, "leo.duran@....com" <leo.duran@....com>,
	"Yazen.Ghannam@....com" <Yazen.Ghannam@....com>, "rientjes@...gle.com"
	<rientjes@...gle.com>, "jiaqiyan@...gle.com" <jiaqiyan@...gle.com>,
	"Jon.Grimm@....com" <Jon.Grimm@....com>, "dave.hansen@...ux.intel.com"
	<dave.hansen@...ux.intel.com>, "naoya.horiguchi@....com"
	<naoya.horiguchi@....com>, "james.morse@....com" <james.morse@....com>,
	"jthoughton@...gle.com" <jthoughton@...gle.com>, "somasundaram.a@....com"
	<somasundaram.a@....com>, "erdemaktas@...gle.com" <erdemaktas@...gle.com>,
	"pgonda@...gle.com" <pgonda@...gle.com>, "duenwen@...gle.com"
	<duenwen@...gle.com>, "gthelen@...gle.com" <gthelen@...gle.com>,
	"wschwartz@...erecomputing.com" <wschwartz@...erecomputing.com>,
	"dferguson@...erecomputing.com" <dferguson@...erecomputing.com>,
	"wbs@...amperecomputing.com" <wbs@...amperecomputing.com>,
	"nifan.cxl@...il.com" <nifan.cxl@...il.com>, tanxiaofei
	<tanxiaofei@...wei.com>, "Zengtao (B)" <prime.zeng@...ilicon.com>, "Roberto
 Sassu" <roberto.sassu@...wei.com>, "kangkang.shen@...urewei.com"
	<kangkang.shen@...urewei.com>, wanghuiqiang <wanghuiqiang@...wei.com>,
	Linuxarm <linuxarm@...wei.com>
Subject: Re: [PATCH v18 04/19] EDAC: Add memory repair control feature

On Thu, 9 Jan 2025 13:32:22 +0100
Borislav Petkov <bp@...en8.de> wrote:

Hi Boris,

> On Thu, Jan 09, 2025 at 11:00:43AM +0000, Shiju Jose wrote:
> > The min_ and max_ attributes of the control attributes are added  for your
> > feedback on V15 to expose supported ranges of these control attributes to the user, 
> > in the following links.    
> 
> Sure, but you can make that differently:
> 
> cat /sys/bus/edac/devices/<dev-name>/mem_repairX/bank
> [x:y]
> 
> which is the allowed range.

To my thinking that would fail the test of being an intuitive interface.
To issue a repair command requires that multiple attributes be configured
before triggering the actual repair.

Think of it as setting the coordinates of the repair in a high dimensional
space.

In the extreme case of fine grained repair (Cacheline), to identify the
relevant subunit of memory (obtained from the error record that we are
basing the decision to repair on) we need to specify all of:

Channel, sub-channel, rank,  bank group, row, column and nibble mask.
For coarser granularity repair only specify a subset of these applies and
only the relevant controls are exposed to userspace.

They are broken out as specific attributes to enable each to be set before
triggering the action with a write to the repair attribute.

There are several possible alternatives:

Option 1

"A:B:C:D:E:F:G:H:I:J" opaque single write to trigger the repair where
each number is providing one of those coordinates and where a readback
let's us known what each number is.

That single attribute interface is very hard to extend in an intuitive way.

History tell us more levels will be introduced in the middle, not just
at the finest granularity, making such an interface hard to extend in
a backwards compatible way.

Another alternative of a key value list would make for a nasty sysfs
interface.

Option 2 
There are sysfs interfaces that use a selection type presentation.

Write: "C", Read: "A, B, [C], D" but that only works well for discrete sets
of options and is a pain to parse if read back is necessary.

So in conclusion, I think the proposed multiple sysfs attribute style
with them reading back the most recent value written is the least bad
solution to a complex control interface.

> 
> echo ... 
> 
> then writes in the bank.
> 
> > ... so we would propose we do not add max_ and min_ for now and see how the
> > use cases evolve.  
> 
> Yes, you should apply that same methodology to the rest of the new features
> you're adding: only add functionality for the stuff that is actually being
> used now. You can always extend it later.
> 
> Changing an already user-visible API is a whole different story and a lot lot
> harder, even impossible.
> 
> So I'd suggest you prune the EDAC patches from all the hypothetical usage and
> then send only what remains so that I can try to queue them.

Sure. In this case the addition of min/max was perhaps a wrong response to
your request for a way to those ranges rather than just rejecting a write
of something out of range as earlier version did.

We can revisit in future if range discovery becomes necessary.  Personally
I don't think it is given we are only taking these actions in response error
records that give us precisely what to write and hence are always in range.

Jonathan

> 
> Thx.
> 

Powered by blists - more mailing lists