Message-ID: <20250116103207.00005461@huawei.com>
Date: Thu, 16 Jan 2025 10:32:07 +0000
From: Jonathan Cameron <Jonathan.Cameron@...wei.com>
To: Dan Williams <dan.j.williams@...el.com>
CC: Ira Weiny <ira.weiny@...el.com>, Dave Jiang <dave.jiang@...el.com>, "Fan
 Ni" <fan.ni@...sung.com>, Jonathan Corbet <corbet@....net>, Andrew Morton
	<akpm@...ux-foundation.org>, Kees Cook <kees@...nel.org>, "Gustavo A. R.
 Silva" <gustavoars@...nel.org>, Davidlohr Bueso <dave@...olabs.net>, "Alison
 Schofield" <alison.schofield@...el.com>, Vishal Verma
	<vishal.l.verma@...el.com>, <linux-cxl@...r.kernel.org>,
	<linux-doc@...r.kernel.org>, <nvdimm@...ts.linux.dev>,
	<linux-kernel@...r.kernel.org>, <linux-hardening@...r.kernel.org>, Li Ming
	<ming.li@...omail.com>
Subject: Re: [PATCH v8 02/21] cxl/mem: Read dynamic capacity configuration
 from the device

On Wed, 15 Jan 2025 14:34:36 -0800
Dan Williams <dan.j.williams@...el.com> wrote:

> Ira Weiny wrote:
> > Dan Williams wrote:  
> > > Ira Weiny wrote:  
> > 
> > [snip]
> >   
> > > > diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> > > > index e8907c403edbd83c8a36b8d013c6bc3391207ee6..05a0718aea73b3b2a02c608bae198eac7c462523 100644
> > > > --- a/drivers/cxl/cxlmem.h
> > > > +++ b/drivers/cxl/cxlmem.h
> > > > @@ -403,6 +403,7 @@ enum cxl_devtype {
> > > >  	CXL_DEVTYPE_CLASSMEM,
> > > >  };
> > > >  
> > > > +#define CXL_MAX_DC_REGION 8  
> > > 
> > > Please no, lets not sign up to have the "which cxl 'region' concept are
> > > you referring to?" debate in perpetuity. "DPA partition", "DPA
> > > resource", "DPA capacity" anything but "region".
> > >   
> > 
> > I'm inclined to agree with Alejandro on this one.  I've walked this
> > tightrope quite a bit with this series.  But there are other places where
> > we have chosen to change the verbiage from the spec and it has made it
> > difficult for new comers to correlate the spec with the code.
> > 
> > So I like Alejandro's idea of adding "HW" to the name to indicate that we
> > are talking about a spec or hardware defined thing.  
> 
> See below, the only people that could potentially be bothered by the
> lack of spec terminology matching are the very same people that are
> sophisticated enough to have read the spec to know its a problem.

It's confusing me.  :)  I know the confusion source exists but
that doesn't mean I remember how all the terms match up.

> 
> > 
> > That said I am open to changing some names where it is clear it is a
> > software structure.  I'll audit the series for that.
> >   
> > > >  	u64 serial;
> > > >  	enum cxl_devtype type;
> > > >  	struct cxl_mailbox cxl_mbox;
> > > >  };
> > > >  
> > > > +#define CXL_DC_REGION_STRLEN 8
> > > > +struct cxl_dc_region_info {
> > > > +	u64 base;
> > > > +	u64 decode_len;
> > > > +	u64 len;  
> > > 
> > > Duplicating partition information in multiple places, like
> > > mds->dc_region[X].base and cxlds->dc_res[X].start, feels like an
> > > RFC-quality decision for expediency that needs to reconciled on the way
> > > to upstream.  
> > 
> > I think this was done to follow a pattern of the mds being passed around
> > rather than creating resources right when partitions are read.
> > 
> > Furthermore this stands to hold this information in CPU endianess rather
> > than holding an array of region info coming from the hardware.  
> 
> Yes, the ask is translate all of this into common information that lives
> at the cxl_dev_state level.
> 
> > 
> > Let see how other changes fall out before I go hacking this though.
> >   
> > >   
> > > > +	u64 blk_size;
> > > > +	u32 dsmad_handle;
> > > > +	u8 flags;
> > > > +	u8 name[CXL_DC_REGION_STRLEN];  
> > > 
> > > No, lets not entertain:
> > > 
> > >     printk("%s\n", mds->dc_region[index].name);
> > > 
> > > ...when:
> > > 
> > >     printk("dc%d\n", index);
> > > 
> > > ...will do.  
> > 
> > Actually these buffers provide a buffer for the (struct
> > resource)dc_res[x].name pointers to point to.  
> 
> I missed that specific detail, but I still challenge whether this
> precision is needed especially since it makes the data structure
> messier. Given these names are for debug only and multi-partition DCD
> devices seem unlikely to ever exist, just use a static shared name for
> adding to ->dpa_res.

Given the read only shared concept relies on multiple hardware dc regions
(I think they map to partitions) then we are very likely to see
multiples.  (maybe I'm lost in terminology as well).


...

> > > 
> > > Linux is not obligated to follow the questionable naming decisions of
> > > specifications.  
> > 
> > We are not.  But as Alejandro says it can be confusing if we don't make
> > some association to the spec.
> > 
> > What do you think about the HW/SW line I propose above?  
> 
> Rename to cxl_dc_partition_info and drop the region_ prefixes, sure.
> 
> Otherwise, for this init-time only concern I would much rather deal with
> the confusion of:
> 
> "why does Linux call this partition when the spec calls it region?":
> which only trips up people that already know the difference because they read the
> spec. In that case the comment will answer their confusion.
> 
> ...versus:
> 
> "why are there multiple region concepts in the CXL subsystem": which
> trips up everyone that greps through the CXL subsystem especially those
> that have no intention of ever reading the spec.

versus one time rename of all internal infrastructure to align to the spec
and only keep the confusion at the boundaries where we have ABI.

Horrible option but how often are those diving in the code that bothered
about the userspace /kernel interaction terminology?

Anyhow, they are all horrible choices.

Jonathan

> 

Powered by blists - more mailing lists