Re: [PATCH v2 5/5] cxl: Kill enum cxl_decoder_mode

[Ira Weiny <ira.weiny@intel.com>]

Dan Williams wrote:
> Ira Weiny wrote:
> > Dan Williams wrote:
> > > Now that the operational mode of DPA capacity (ram vs pmem... etc) is
> > > tracked in the partition, and no code paths have dependencies on the
> > > mode implying the partition index, the ambiguous 'enum cxl_decoder_mode'
> > > can be cleaned up, specifically this ambiguity on whether the operation
> > > mode implied anything about the partition order.
> > > 
> > > Endpoint decoders simply reference their assigned partition where the
> > > operational mode can be retrieved as partition mode.
> > 
> > You really seem to be defining a region mode not a partition mode.
> 
> To me it comes down to the hierarchy of building up a region.
> 
> The DPA is in a fixed operational mode regardless of whether a region is
> mapped to it. "pmem is always pmem", "ram is always ram" (modulo online
> re-partition which no device has ever built). So calling it a "partition
> mode" reflects that the partition comes first, then the endpoint decoder
> is mapped to a partition, then the region is mapped to an endpoint
> decoder. Region mode is subordinate to partition mode.

Exactly but 

@@ -535,7 +517,7 @@ struct cxl_region_params {
 struct cxl_region {
        struct device dev;
        int id;
-       enum cxl_decoder_mode mode;
+       enum cxl_partition_mode mode;
        enum cxl_decoder_type type;
        struct cxl_nvdimm_bridge *cxl_nvb;
        struct cxl_pmem_region *cxlr_pmem;

... is assigning that partition mode to the region.

> 
> > I did a lot of work to resolve this for DCD interleave in the future.
> > This included the introduction of the DC region mode.  I __think__ that
> > what you have here will work fine.
> > 
> > However, from a user ABI standpoint I'm going to have to play games with
> > having the DCD partitions in a well defined sub-array such that the user
> > can specify which DCD partition they want to use.  So the user concept of
> > decoder mode does not really go away.
> 
> This is the question, do we need to rip that "give userspace explicit
> partition control" ABI band-aid?
> 
> As I mentioned over here [1], I admit that someone might build a "ram,
> dynamic ram, shared ram" device, I remain skeptical that someone will
> build a, for example, "ram, dynamic ram, dynamic ram, shared ram"
> device. We can always make the ABI more complicated in the future, but
> the common case of "userspace need only care about mode and let the
> kernel find the partition", probably carries the implementation for the
> foreseeable future.

I've thought about this all afternoon.  I feel like this is baking in policy in
the kernel.  Wouldn't it be better to export the attributes of the partitions
to the user and have them sort it out?

> 
> [1]: http://lore.kernel.org/67915ce296030_20fa29457@dwillia2-xfh.jf.intel.com.notmuch
> 
> > In the interest of urgency I'm going to give my tag on this.  But I would
> > have preferred this called region mode.  But I can see why partition mode
> > makes sense too.
> 
> It is a fair comment that deserves to be captured in the Glossary of
> Terms entry for "partition".

eh... sure.
Ira