On 22-02-23 14:24:00, Dan Williams wrote: > On Wed, Feb 23, 2022 at 1:50 PM Ben Widawsky <ben.widawsky@xxxxxxxxx> wrote: > > > > On 22-02-17 11:57:59, Dan Williams wrote: > > > On Thu, Feb 17, 2022 at 10:36 AM Ben Widawsky <ben.widawsky@xxxxxxxxx> wrote: > > > > > > > > Consolidating earlier discussions... > > > > > > > > On 22-01-28 16:25:34, Dan Williams wrote: > > > > > On Thu, Jan 27, 2022 at 4:27 PM Ben Widawsky <ben.widawsky@xxxxxxxxx> wrote: > > > > > > > > > > > > The region creation APIs create a vacant region. Configuring the region > > > > > > works in the same way as similar subsystems such as devdax. Sysfs attrs > > > > > > will be provided to allow userspace to configure the region. Finally > > > > > > once all configuration is complete, userspace may activate the region. > > > > > > > > > > > > Introduced here are the most basic attributes needed to configure a > > > > > > region. Details of these attribute are described in the ABI > > > > > > > > > > s/attribute/attributes/ > > > > > > > > > > > Documentation. Sanity checking of configuration parameters are done at > > > > > > region binding time. This consolidates all such logic in one place, > > > > > > rather than being strewn across multiple places. > > > > > > > > > > I think that's too late for some of the validation. The complex > > > > > validation that the region driver does throughout the topology is > > > > > different from the basic input validation that can be done at the > > > > > sysfs write time. For example ,this patch allows negative > > > > > interleave_granularity values to specified, just return -EINVAL. I > > > > > agree that sysfs should not validate everything, I disagree with > > > > > pushing all validation to cxl_region_probe(). > > > > > > > > > > > > > Okay. It might save us some back and forth if you could outline everything you'd > > > > expect to be validated, but I can also make an attempt to figure out the > > > > reasonable set of things. > > > > > > Input validation. Every value that gets written to a sysfs attribute > > > should be checked for validity, more below: > > > > > > > > > > > > > > > > > > > A example is provided below: > > > > > > > > > > > > /sys/bus/cxl/devices/region0.0:0 > > > > > > ├── interleave_granularity > > > > > > ...validate granularity is within spec and can be supported by the root decoder. > > > > > > > > > ├── interleave_ways > > > > > > ...validate ways is within spec and can be supported by the root decoder. > > > > I'm not sure how to do this one. Validation requires device positions and we > > can't set the targets until ways is set. Can you please provide some more > > insight on what you'd like me to check in addition to the value being within > > spec? > > For example you could check that interleave_ways is >= to the root > level interleave. I.e. it would be invalid to attempt a x1 interleave > on a decoder that is x2 interleaved at the host-bridge level. I tried to convince myself that that assertion always holds and didn't feel super comfortable. If you do, I can add those kinds of checks. Thanks. Ben
