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

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

Jonathan Cameron wrote:
> On Wed, 22 Jan 2025 00:59:38 -0800
> Dan Williams <dan.j.williams@intel.com> 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.
> > 
> > With this in place PMEM can now be partition0 which happens today when
> > the RAM capacity size is zero. Dynamic RAM can appear above PMEM when
> > DCD arrives, etc. Code sequences that hard coded the "PMEM after RAM"
> > assumption can now just iterate partitions and consult the partition
> > mode after the fact.
> > 
> > Cc: Dave Jiang <dave.jiang@intel.com>
> > Cc: Alejandro Lucero <alucerop@amd.com>
> > Cc: Ira Weiny <ira.weiny@intel.com>
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> A few things inline.
> 
> Jonathan
> 
> > diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> > index 591aeb26c9e1..bb478e7b12f6 100644
> > --- a/drivers/cxl/core/hdm.c
> > +++ b/drivers/cxl/core/hdm.c
[..]
> > -	/*
> > -	 * Only allow modes that are supported by the current partition
> > -	 * configuration
> > -	 */
> > -	if (mode == CXL_DECODER_PMEM && !cxl_pmem_size(cxlds)) {
> > -		dev_dbg(dev, "no available pmem capacity\n");
> > -		return -ENXIO;
> > +	part = -1;
> > +	for (int i = 0; i < cxlds->nr_partitions; i++)
> 
> Similar to previous comment can use early loop exit and
> part as the loop iteration variable short code and no magic i
> appears.

Yeah, might as well.

[..]
> > @@ -2728,11 +2733,17 @@ static int poison_by_decoder(struct device *dev, void *arg)
> >  		return rc;
> >  
> >  	cxlmd = cxled_to_memdev(cxled);
> > +	cxlds = cxlmd->cxlds;
> > +	if (cxled->part < 0)
> > +		mode = CXL_PARTMODE_NONE;
> Ah.  Here is our mysterious none.  Maybe add a comment on what
> this means in practice.  Race condition, actual hole, crazy decoder
> someone (e.g bios) setup?

I just fixed it up like this:

@@ -2724,15 +2729,18 @@ static int poison_by_decoder(struct device *dev, void *arg)
                return rc;
 
        cxled = to_cxl_endpoint_decoder(dev);
-       if (!cxled->dpa_res || !resource_size(cxled->dpa_res))
+       if (!cxled->dpa_res)
                return rc;
 
        cxlmd = cxled_to_memdev(cxled);
+       cxlds = cxlmd->cxlds;
+       mode = cxlds->part[cxled->part].mode;
+

...because there is no such thing as a decoder with allocated capacity
but no partition set, and an endpoint decoder will never be zero sized.

I also thought about adding a lockdep_assert_held(&cxl_dpa_rwsem)
because this function is a few calls away from that context, but will
just leave that alone for now.