Re: [PATCH v13 07/15] cxl: Share HDM decoder register unpacking

[Srirangan Madhavan]

On 9/23/26 8:05 PM, Jonathan Cameron wrote:
> External email: Use caution opening links or attachments
> > -     };
> > +     if (!cxled && cxld->interleave_ways > 8) {
>
> Why does this check make sense now when we didn't have it before
> (that I can find anyway)?  If it isn't tightly coupled to this
> patch and instead is providing some extra checks that are worthwhile
> I'd break it out as a separate patch where that usecase can be
> well described.

I dropped the new > 8 check from v14 patch 7. It was not present in the 
original code, and this patch is intended to only share the existing 
unpacking behavior.

> > +
> > +     if (!committed)
> > +             size = 0;
> > +     if (base == U64_MAX || size == U64_MAX ||
> > +         (size && base > U64_MAX - (size - 1)))
> > +             return -ENXIO;
>
> This second block looks like new defences which is fine but I'm not sure
> it makes sense in here.  Also given we are just checking overflow
> doesn't happen maybe use check_add_overflow(). Saves us thinking too
> much the maths.

Agreed. V14 puts the overflow check in a separate patch, 8/16, and uses 
check_add_overflow().

> > +
> > +     hpa_range = (struct range) {
> > +             .start = base,
> > +             .end = base + size - 1,
> > +     };
> > +     target_type = FIELD_GET(CXL_HDM_DECODER0_CTRL_HOSTONLY, ctrl) ?
> > +                   CXL_DECODER_HOSTONLYMEM : CXL_DECODER_DEVMEM;
>
> Maybe stick to the if / else of the original? I think that is more readable for
> an extra couple of lines.
> Also blank line here to give visual separation before the conditional that
> follows.

Ack. V14 keeps the original if/else.

> It's a functional change to move the target_type selection out of the
> committed check. I'd like to see some discussion of why that is fine to
> do which probably means a separate little patch that has that description
> on its own.

Ack. V14 selects the target type only when the decoder is committed, 
preserving the existing behavior.


--
Regards,
Srirangan