On Tue, Sep 22, 2026 at 12:34:02AM +0100, Jonathan Cameron wrote: > 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. > Yeah I think we discussed making it its own partition mode but I don't think there was a strong consensus, just that we agreed it should be exposed somewhere, somehow...
I am leaning towards just putting it under dynamic_ram_1 even though these attributes would only exist for dynamic_ram mode: /sys/bus/cxl/devices/memX/dynamic_ram_1/shareable /sys/bus/cxl/devices/memX/dynamic_ram_1/read_only Mainly because if we go up to 8 partitions, we would have (dynamic_ram, dynamic_shared, dynamic_readonly, dynamic_shared_readonly) * 8. And NDCTL would have to know how to handle creating each type. I also don't see anything in the code that makes it terrible: - each partition mode has its own attribute array and visibility callback function, so it doesn't affect ram/pmem modes I brought it up because I wasn't sure if there was a specific reason why shareable and read_only weren't carried over after the partition rework. From a code standpoint, it seems fine to me? I assumed it was so all partitions would share common sub-attributes but I guess I shouldn't have assumed... > > > > - 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. > Mmm very true. You might just clarifying this point here but just to make sure we're on the same page: I dropped the R_O part of the series and deferred it for the future because it includes implementing write rejection for r_o partitions which IMO can be done after the core functionality is in. Thanks, Anisa > > 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 >
