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

[Srirangan Madhavan <smadhavan@nvidia.com>]

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