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

[Dan Williams <dan.j.williams@intel.com>]

Ira Weiny wrote:
> 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.

Right, to cache it and simplify other code paths. Otherwise, say more, I
am not picking up the argument.

The alternative is an awkward helper along the lines of:

to_cxl_region_mode(struct cxl_region *cxlr)
{
	guard(rwsem_read)(&cxl_region_rwsem);
	return cxlds->part[cxlr->params->targets[0]->part].mode;
}

...and some other gymnastics when assigning that first decoder to the
targets list.

> > > 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?

We have gotten by without expanding the user ABI. A large swath of DCD
functionality can be had with the kernel doing the mode-to-partition
lookup. Dare a device to require the kernel to expand its ABI.

History is littered with premature spec enabling.