Re: [PATCH v2] cxl: Check for invalid addresses returned from translation functions on errors

[Alison Schofield]

On Wed, Jan 07, 2026 at 01:03:00PM +0100, Robert Richter wrote:
> On 06.01.26 10:42:36, Alison Schofield wrote:
> > On Tue, Jan 06, 2026 at 06:23:58PM +0100, Robert Richter wrote:
> 
> > > @@ -3207,8 +3220,13 @@ static int region_offset_to_dpa_result(struct cxl_region *cxlr, u64 offset,
> > >  		cxled = p->targets[i];
> > >  		if (cxled->pos != pos)
> > >  			continue;
> > > +
> > > +		dpa = cxl_dpa_resource_start(cxled);
> > 
> > We want to return -ENXIO, not 0 in this case.
> > So jump out here immediately - right?
> > 
> > 		if (dpa == REsOURCE_SIZE_MAX)
> > 			return -ENXIO;
> 
> The users of region_offset_to_dpa_result() handle the
> RESOURCE_SIZE_MAX case. For that reason the dpa is not checked here.
> I did not want to change the function interface with that fix.

region_offset_to_dpa_result() intends to return an rc when it knows
it would return a bad result. This is that case. With this proposed
change, we avoid adding to a RESOURCE_SIZE_MAX, which is defensive,
but isn't quitting immediately correct? These fixups are all about
stopping when any ingredient to the calc looks bad. It looks bad,
let's stop right away.

Seeing that the callsites do this:

	rc = region_offset_to_dpa_result(cxlr, offset, &result);
        if (rc || !result.cxlmd || result.dpa == ULLONG_MAX) {
		...this failed

shows that they do some defensive programming and don't rely only 
on the rc. But the inverse does not seem right - relying on that.

Staring at it more, I guess you could refactor the function to be
a void and get rid of the rc entirely.

Is that the real simplification needed here?

-- Alison

> 
> Thanks for review.
> 
> -Robert
> 
> > 
> > 
> > > +		if (dpa != RESOURCE_SIZE_MAX)
> > > +			dpa += dpa_offset;
> > > +
> > >  		result->cxlmd = cxled_to_memdev(cxled);
> > > -		result->dpa = cxl_dpa_resource_start(cxled) + dpa_offset;
> > > +		result->dpa = dpa;
> > >  
> > >  		return 0;
> > >  	}