Re: [PATCH v2 2/3] cxl/region: Calculate a target position in a region interleave

[Jim Harris <jim.harris@samsung.com>]

On Mon, Oct 23, 2023 at 11:10:29AM -0700, Alison Schofield wrote:
> On Tue, Oct 17, 2023 at 05:33:54PM +0000, Jim Harris wrote:
> > 
> > > On Oct 15, 2023, at 11:02 PM, alison.schofield@intel.com wrote:
> > > 
> > > + if (p->nr_targets != p->interleave_ways)
> > > + return 0;
> > 
> > There is code just before this that compares nr_targets and interleave_ways
> > before calling cxl_region_setup_targets().
> > 
> > Decoder fields are set between that comparison and here, but I see no reason
> > these fields shouldn’t be set before that earlier comparison. It actually
> > makes the code more consistent, otherwise in the case where
> > cxl_region_setup_targets() fails, the first n-1 decoders have their fields
> > set, but the last one would not.
> > 
> > Then this return 0 can just be an else clause on the earlier comparison.
> > 
> 
> I see what you mean and my first thought was 'sure this could be a little
> cleanup before this patch', but then I struggle to justify it. Those first
> n-1 decoders have iw,ig,hpa fields set because we had hope that this region
> would create. On the last decoder, is there any value in setting up the
> decoder when we know it's a failed setup?
> 
> 

I have a patch outstanding in this same area, which may provide some
additional context.

https://lore.kernel.org/linux-cxl/169703589120.1202031.14696100866518083806.stgit@bgt-140510-bm03.eng.stellus.in/

I think the value is that the all of the decoders get treated the same.
Note that before the nr_targets == interleave_ways check, it sets
cxled->pos, as well as put the decoder in the targets array and
increments the nr_targets.

But ultimately my suggested changes probably belong more in my patch
(or a follow-up to my patch), so I think it's fine to skip my
feedback on this part.

Jim