Re: [PATCH RFC v2 03/18] cxl/mem: Read Dynamic capacity configuration from the device

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

Alison Schofield wrote:
> On Mon, Aug 28, 2023 at 10:20:54PM -0700, Ira Weiny wrote:
> > From: Navneet Singh <navneet.singh@intel.com>
> > 
> > Devices can optionally support Dynamic Capacity (DC).  These devices are
> > known as Dynamic Capacity Devices (DCD).
> 
> snip
> 
> > 
> > +static enum cxl_region_mode
> > +cxl_decoder_to_region_mode(enum cxl_decoder_mode mode)
> > +{
> > +	switch (mode) {
> > +	case CXL_DECODER_NONE:
> > +		return CXL_REGION_NONE;
> > +	case CXL_DECODER_RAM:
> > +		return CXL_REGION_RAM;
> > +	case CXL_DECODER_PMEM:
> > +		return CXL_REGION_PMEM;
> > +	case CXL_DECODER_DEAD:
> > +		return CXL_REGION_DEAD;
> > +	case CXL_DECODER_MIXED:
> > +	default:
> > +		return CXL_REGION_MIXED;
> > +	}
> > +
> > +	return CXL_REGION_MIXED;
> 
> Can the paths to return _MIXED be simplified here?

I suppose:

...
	case CXL_DECODER_MIXED:
	default:
		break;
	}
	
	return CXL_REGION_MIXED;
...

I don't think that makes things any better.

> 
> 
> > +}
> > +
> snip
> 
> > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> > index cd4a9ffdacc7..ed282dcd5cf5 100644
> > --- a/drivers/cxl/cxl.h
> > +++ b/drivers/cxl/cxl.h
> > @@ -374,6 +374,28 @@ static inline const char *cxl_decoder_mode_name(enum cxl_decoder_mode mode)
> >  	return "mixed";
> >  }
> >  
> > +enum cxl_region_mode {
> > +	CXL_REGION_NONE,
> > +	CXL_REGION_RAM,
> > +	CXL_REGION_PMEM,
> > +	CXL_REGION_MIXED,
> > +	CXL_REGION_DEAD,
> > +};
> 
> I'm concerned about _DEAD.
> At first I was going to say name these as CXL_REGION_MODE_*, but it's
> pretty obvious that these are mode words...except for DEAD. Is that 
> an actual mode or is it some type of status? I don't think I see it
> used yet.

My first reaction was to remove this.  But I had to go back and look.  It
took me a minute to trace this.

'Dead' is not used directly.  If a decoder happens to be dead
(CXL_DECODER_DEAD) then it will eventually fail the creation of a region
with CXL_REGION_DEAD as the mode.  CXL_REGION_MIXED fails the same way but
only because mixed mode is not yet supported.  Therefore, decoder mode
DEAD indicates something different and CXL_REGION_DEAD was added to convey
this when converting.

The alternative is to be more explicit and check decoder mode to be !DEAD
prior to trying to convert.  I think I like that but I'm going to sleep on
it.

> 
> > +
> > +static inline const char *cxl_region_mode_name(enum cxl_region_mode mode)
> > +{
> > +	static const char * const names[] = {
> > +		[CXL_REGION_NONE] = "none",
> > +		[CXL_REGION_RAM] = "ram",
> > +		[CXL_REGION_PMEM] = "pmem",
> > +		[CXL_REGION_MIXED] = "mixed",
> > +	};
> > +
> > +	if (mode >= CXL_REGION_NONE && mode <= CXL_REGION_MIXED)
> > +		return names[mode];
> > +	return "mixed";
> > +}
> 
> snip
> 
> > +
> >  /**
> >   * struct cxl_memdev_state - Generic Type-3 Memory Device Class driver data
> >   *
> > @@ -449,6 +464,8 @@ struct cxl_dev_state {
> >   * @enabled_cmds: Hardware commands found enabled in CEL.
> >   * @exclusive_cmds: Commands that are kernel-internal only
> >   * @total_bytes: sum of all possible capacities
> > + * @static_cap: Sum of RAM and PMEM capacities
> > + * @dynamic_cap: Complete DPA range occupied by DC regions
> 
> Wondering about renaming RAM and PMEM caps as 'static'.
> They are changeable via set partition commands.

True but they are static compared to dynamic capacity.  I'm open to other
names but !dynamic is normally referred to as static.  :-/

Thanks for the review!
Ira