Re: [PATCH 1/4] cxl: Remove the CXL_DECODER_MIXED mistake

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

Jonathan Cameron wrote:
> On Thu, 16 Jan 2025 22:10:32 -0800
> Dan Williams <dan.j.williams@intel.com> wrote:
> 
> > CXL_DECODER_MIXED is a safety mechanism introduced for the case where
> > platform firmware has programmed an endpoint decoder that straddles a
> > DPA partition boundary. While the kernel is careful to only allocate DPA
> > capacity within a single partition there is no guarantee that platform
> > firmware, or anything that touched the device before the current kernel,
> > gets that right.
> > 
> > However, __cxl_dpa_reserve() will never get to the CXL_DECODER_MIXED
> > designation because of the way it tracks partition boundaries. A
> > request_resource() that spans ->ram_res and ->pmem_res fails with the
> > following signature:
> > 
> >     __cxl_dpa_reserve: cxl_port endpoint15: decoder15.0: failed to reserve allocation
> > 
> > CXL_DECODER_MIXED is dead defensive programming after the driver has
> > already given up on the device. It has never offered any protection in
> > practice, just delete it.
> > 
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> > ---
> >  drivers/cxl/core/hdm.c    |    8 ++++----
> >  drivers/cxl/core/region.c |   12 ------------
> >  drivers/cxl/cxl.h         |    4 +---
> >  3 files changed, 5 insertions(+), 19 deletions(-)
> > 
> > diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> > index 28edd5822486..be8556119d94 100644
> > --- a/drivers/cxl/core/hdm.c
> > +++ b/drivers/cxl/core/hdm.c
> > @@ -329,12 +329,12 @@ static int __cxl_dpa_reserve(struct cxl_endpoint_decoder *cxled,
> >  
> >  	if (resource_contains(&cxlds->pmem_res, res))
> >  		cxled->mode = CXL_DECODER_PMEM;
> > -	else if (resource_contains(&cxlds->ram_res, res))
> > +	if (resource_contains(&cxlds->ram_res, res))
> 
> Logic of removing the else?  I assume there is 0 chance that both conditions
> match, but doesn't this mean if the res is not in ram_res we always hit the next
> else and print the warning?

...bug that I fixed later in the series and did not fold all the way
back to where it came from when splitting the series.

Good catch.

> 
> >  		cxled->mode = CXL_DECODER_RAM;
> >  	else {
> > -		dev_warn(dev, "decoder%d.%d: %pr mixed mode not supported\n",
> > -			 port->id, cxled->cxld.id, cxled->dpa_res);
> > -		cxled->mode = CXL_DECODER_MIXED;
> > +		dev_warn(dev, "decoder%d.%d: %pr does not map any partition\n",
> > +			 port->id, cxled->cxld.id, res);
> > +		cxled->mode = CXL_DECODER_NONE;
> >  	}
> >  
> >  	port->hdm_end++;
> 
> > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> > index f6015f24ad38..0fb8d70fa3e5 100644
> > --- a/drivers/cxl/cxl.h
> > +++ b/drivers/cxl/cxl.h
> > @@ -379,7 +379,6 @@ enum cxl_decoder_mode {
> >  	CXL_DECODER_NONE,
> >  	CXL_DECODER_RAM,
> >  	CXL_DECODER_PMEM,
> > -	CXL_DECODER_MIXED,
> >  	CXL_DECODER_DEAD,
> >  };
> >  
> > @@ -389,10 +388,9 @@ static inline const char *cxl_decoder_mode_name(enum cxl_decoder_mode mode)
> >  		[CXL_DECODER_NONE] = "none",
> >  		[CXL_DECODER_RAM] = "ram",
> >  		[CXL_DECODER_PMEM] = "pmem",
> > -		[CXL_DECODER_MIXED] = "mixed",
> >  	};
> >  
> > -	if (mode >= CXL_DECODER_NONE && mode <= CXL_DECODER_MIXED)
> > +	if (mode >= CXL_DECODER_NONE && mode <= CXL_DECODER_PMEM)
> Maybe just < DEAD is simpler?

I like that.