On Mon, May 10, 2021 at 7:58 AM Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx> wrote: > > On Fri, 7 May 2021 15:51:47 -0700 > Dan Williams <dan.j.williams@xxxxxxxxx> wrote: > > > While CXL builds upon the PCI software model for dynamic enumeration and > > control, a static platform component is required to bootstrap the CXL > > memory layout. In addition to identifying the host bridges ACPI is > > responsible for enumerating the CXL memory space that can be addressed > > by decoders. This is similar to the requirement for ACPI to publish > > resources reported by _CRS for PCI host bridges. > > > > Introduce the cxl_root object as an abstract "port" into the CXL.mem > > address space described by HDM decoders identified by the ACPI > > CEDT.CHBS. > > > > For now just establish the initial boilerplate and sysfs attributes, to > > be followed by enumeration of the ports within the host bridge. > > > > Note the allocation of CXL core device objects is split into separate > > alloc and add steps in order to separate the alloc error path (kfree()) > > from the device add error path (put_device()). > > > > Signed-off-by: Dan Williams <dan.j.williams@xxxxxxxxx> > > Hi Dan > > Just one bit in here that confused me (assuming I'm reading the code correctly). > You have is_visible for the dev_attr_supports_pmem etc to only show them if > the particular space supports that memory type. That's fine. You also have > the actual sysfs function checking the same flag to decide to return "1" or "0" > which would also be fine, but in combination it's rather odd as the sysfs > read function can never return "0" (sysfs attribute isn't visible in that > condition). Probably deserves at least a comment. Ok. That was deliberate since it's trivial to code and allows the visibility policy to change without needing to go audit the attributes that assumed invisibility. However, yes, it deserves a comment to save brain cycles with that "hmm, that's odd" in the future. > This also needs some documentation for the new sysfs ABI > (Documentation/ABI/...) but that can be in a separate patch. True. > > Otherwise looks good to me. Thanks.
