On Wed, 5 Aug 2026 01:30:16 -0700 Anisa Su <[email protected]> wrote:
> On Tue, Aug 04, 2026 at 01:07:48AM +0100, Jonathan Cameron wrote: > > On Fri, 31 Jul 2026 09:02:49 +0000 > > [email protected] wrote: > > > > > Thank you for your contribution! Sashiko AI review found 2 potential > > > issue(s) to consider: > > > - [Medium] The commit message claims to extract and store the 'read only' > > > attribute from DSMAS tables, but this logic is completely missing from > > > the code. > > > > Given it will make a lot of difference to a user if they think they have > > writeable > > memory that isn't - I think we probably do want to have readonly here. > > > I did some digging: it seems in v8, the read only flag was gathered and > exposed in sysfs, but dropped during the region -> partition rework. > > v8 of this commit used to do: > dent->read_only = dsmas->flags & ACPI_CDAT_DSMAS_READ_ONLY; > ... > mds->dc_region[i].read_only = dent->read_only; > > > https://lore.kernel.org/all/[email protected]/ > > and a following patch exposed it, with a documented ABI: > > > https://lore.kernel.org/all/[email protected]/ > > /sys/bus/cxl/devices/memX/dcY/size > /sys/bus/cxl/devices/memX/dcY/read_only > /sys/bus/cxl/devices/memX/dcY/shareable > /sys/bus/cxl/devices/memX/dcY/qos_class > > In v9 dynamic capacity moved from its own dc_region[] array and dcY/ > sysfs directory to the generic partition model: > > > https://lore.kernel.org/all/[email protected]/ > > size and qos_class survived that move because ram and pmem already have > them. read_only and shareable did not, because the generic partition > model had nowhere to put attributes that only dynamic capacity has, but > the commit message survived the rework. > > More specifically, read_only was dropped from both the sysfs commit and > this commit. shareable was dropped only from the sysfs commit, but still > read from the DSMAS entry in v9 of this commit. Not sure why though; I > didn't see any consumers of the attribute. Oh well... We use it now > since shared extents are supported to check if an extent marked shared > is actually in a shared partition. > > So TLDR; I've removed the "read only" part from the commit message. But > let me know if you think it should be re-introduced? > > I agree with what you said about it being important for a user to know > whether a partition is read only. Some things to consider: > > - The partition model has no precedent for a DC only attribute. > dynamic_ram_1/ currently exposes size and qos_class, the same two that > ram/ and pmem/ expose. Adding read_only there would be the first > attribute that exists for one partition mode and not the others. > > - shareable is in exactly the same position. It is gathered and used > internally but not exposed anywhere, so if we are adding one of these > back we should probably decide about both. I vaguely recall the discussion on this ending up with we'd call the partition something different to reflect the extra property. dynamic_readonly_1 / dynamic_sharedram_1 maybe? Argument being that these properties are as different from normal RAM as persistent memory is. > > - I don't think the DSMAS Flags bit 6 (read only) is specific to DCD? In theory yes, in practice there isn't a path for non DCD to actually get the data in place and transtion to read only so maybe an expensive source of zeros? So yes allowed but not much use to anyone so we won't see it. > It describes any device scoped memory range, so a read only ram or pmem > partition is expressible in CDAT today and the driver ignores it. > Maybe it should be exposed in sysfs for every partition > rather than only on dynamic_ram_1? Which probably makes it a generic > CDAT/partition > patch rather than something for this series. > Jonathan
