Re: [PATCH v2] cxl/edac: Fail ECS threshold read for reserved encodings

[Yili Zhang]

On 2026-10-20 22:27, Jonathan Cameron wrote:
> This just called out in the CXL upstream sync call as lacking
> review!  Sorry about that - I missed it entirely :(

Hi Jonathan,

Thanks for the review, and no worries at all about the delay.

> 
> So this bit suggests this is hardening against bad device.
> Definitely a good thing to do given it is fairly simple but
> not a fix as such (so no rush to land it!)

Agreed.

> 
> For this one, can we trigger these values by writing a threshold of 0?
> I think that is blocked by cxl_set_ecs_threshold() so I think this
> one is also hardening against a bad device?

Confirmed, the write path cannot produce these encodings.
cxl_set_ecs_threshold() validates the value in a switch that only
accepts 256, 1024 and 4096 and returns -EINVAL for anything else, so
writing 0 (or any other unsupported value) is rejected. Both the
out-of-bounds indices (6-7) and the reserved encodings (0-2) can
therefore only be observed when a device itself reports them, so
this is hardening against a bad device in both cases.

> > Fixes: 85fb6a16ad14 ("cxl/edac: Add CXL memory device ECS control feature")
> 
> My overall suggestion is drop the fixes tag and state that this
> is hardening against devices returning values not currently in the spec.

Will do. I had added the Fixes tag mainly because of the out-of-bounds
read, but as that can only happen with a device reporting values
outside the spec, hardening is indeed the right framing. I'll drop
the Fixes tag and reword the commit message accordingly; the code is
unchanged, so I'll send a v3 with just the commit message updated.

> 
> It might actually evolve to become a real fix with future hardware
> changes using those reserved values but we have no reason to think
> that will happen yet. 
> 
> We had a small discussion on the call about fixes for bad hardware
> and I expressed my view that if they are cheap (like here) in
> maintenance burden by all means take them, but I'll push back on
> anything that needs very complex solutions.

That also fits your maintenance-burden point nicely - this one is a
single validity check on top of the existing lookup, and if a future
spec revision ever assigns meaning to those reserved encodings,
ecs_supp_threshold[] can simply be extended at that point.

> 
> > Suggested-by: Alison Schofield <alison.schofield@xxxxxxxxx>
> > Signed-off-by: Yili Zhang <zhangyili01@xxxxxxxxx>
> 
> Updated Shiju's email address in the +CC.
> 

Thanks also for updating Shiju's address on the CC.

Regards,
Yili