Re: [PATCH 06/19] cxl/hdm: Default CXL_DEVTYPE_DEVMEM decoders to CXL_DECODER_DEVMEM

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

Jonathan Cameron wrote:
> On Sun, 04 Jun 2023 16:32:10 -0700
> Dan Williams <dan.j.williams@intel.com> wrote:
> 
> > In preparation for device-memory region creation, arrange for decoders
> > of CXL_DEVTYPE_DEVMEM memdevs to default to CXL_DECODER_DEVMEM for their
> > target type.
> 
> Why?  CXL_DEVTYPE_DEVMEM might just be a non CLASS code compliant HDM-H
> only device.  I'd want those drivers to always set this explicitly.

As it stands /sys/bus/cxl/devices/decoderX.Y/target_type is read-only.
For non-class-code compliant HDM-H device, or even a device that
supports mixed HDM-H + HDM-DB operation depending on the decoder, there
would need to be some mechanism to communicate that at decoder
instantiation time.

So the default derived from CXL_DEVTYPE_* is indeed an arbitrary
placeholder until a use case for more precision comes along. In that
case I think target_type becomes writable, and "none" becomes a valid
return value.

> > 
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> > ---
> >  drivers/cxl/core/hdm.c |   14 ++++++++++++--
> >  1 file changed, 12 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> > index de8a3fb28331..ca3b99c6eacf 100644
> > --- a/drivers/cxl/core/hdm.c
> > +++ b/drivers/cxl/core/hdm.c
> > @@ -856,12 +856,22 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
> >  		}
> >  		port->commit_end = cxld->id;
> >  	} else {
> > -		/* unless / until type-2 drivers arrive, assume type-3 */
> >  		if (FIELD_GET(CXL_HDM_DECODER0_CTRL_TYPE, ctrl) == 0) {
> >  			ctrl |= CXL_HDM_DECODER0_CTRL_TYPE;
> >  			writel(ctrl, hdm + CXL_HDM_DECODER0_CTRL_OFFSET(which));
> 
> This is setting it to be HOSTMEM if it was previously DEVMEM and that
> makes it inconsistent with the state cached below.

Oh, definite oversight.

> Not sure why it was conditional in the first place - writing to existing value
> should have been safe and would be less code...

Agree, that's busted, will fix.

> 
> >  		}
> > -		cxld->target_type = CXL_DECODER_HOSTMEM;
> > +		if (cxled) {
> > +			struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
> > +			struct cxl_dev_state *cxlds = cxlmd->cxlds;
> > +
> > +			if (cxlds->type == CXL_DEVTYPE_CLASSMEM)
> > +				cxld->target_type = CXL_DECODER_HOSTMEM;
> > +			else
> > +				cxld->target_type = CXL_DECODER_DEVMEM;
> > +		} else {
> > +			/* To be overridden by region type at commit time */
> > +			cxld->target_type = CXL_DECODER_HOSTMEM;
> > +		}
> >  	}
> >  	rc = eiw_to_ways(FIELD_GET(CXL_HDM_DECODER0_CTRL_IW_MASK, ctrl),
> >  			  &cxld->interleave_ways);
> > 
>