Re: [v2 2/4] cxl/pci: Don't set up decoders for disallowed DVSEC ranges

[Yanfei Xu <yanfei.xu@intel.com>]

On 8/10/2024 3:02 AM, Dan Williams wrote:
> Yanfei Xu wrote:
>> Since it shouldn't create and configure decoders for disallowed
>> ranges, move the check of dvsec_range_allowed() earlier into
>> cxl_dvsec_rr_decode() to filter the disallowed DVSEC ranges out
>> at firtst.
> 
> "Fixes" explain the user visible side effect of the problem. There is no
> problem being fixed with this. It is maybe a conceptual cleanup, but to
> me the risk and thrash is not worth the reward. Unless I am missing
> something this is just code movement for no value.

You are right, this is much like a cleanup as creating decoders for 
disallowed ranges doesn't have side effect for user. Will drop the "Fixes".

I just was aiming to avoid to create decoders for the disallowed ranges, 
since this kind of decoders don't take effect in practice. And we 
already checked and found that, why continued to create that? :)

How about changing the dev_dbg() in below codes to dev_warn() when find 
disallowed ranges to let the user to know what happened.

         cxld_dev = device_find_child(&root->dev, &dvsec_range,
                             dvsec_range_allowed);
         if (!cxld_dev) {
             dev_dbg(dev, "DVSEC Range%d denied by platform\n", i+1);
             continue;
         }
         dev_dbg(dev, "DVSEC Range%d allowed by platform\n", i+1);
         put_device(cxld_dev);

> 
> The "risk" is that I think this breaks cases where BIOS has enabled HDM
> decoders. I.e.
> 
>          /*
>           * If the HDM Decoder Capability is already enabled then assume
>           * that some other agent like platform firmware set it up.
>           */
>          if (global_ctrl & CXL_HDM_DECODER_ENABLE || (!hdm && info->mem_enabled))
>                  return devm_cxl_enable_mem(&port->dev, cxlds);
> 
> ...the DVSEC range resgister values do not matter when HDM decoders are
> enabled.

It won't. Even no allowed range, it only keep 0 in info->ranges first. 
And the check for whether info->ranges is 0 is performed after the check 
you pasted.

     if (global_ctrl & CXL_HDM_DECODER_ENABLE || (!hdm && 
info->mem_enabled))
         return devm_cxl_enable_mem(&port->dev, cxlds);
     else if (!hdm)
         return -ENODEV;
     ......

     if (!info->ranges) {
         dev_err(dev, "No available DVSEC register ranges.\n");
         return -ENXIO;
     }


Thanks,
Yanfei