Re: [PATCH v7 8/8] cxl: Allow auto-committed BI hdm decoders

On Tue, 28 Jul 2026, [email protected] wrote:

>Thank you for your contribution! Sashiko AI review found 1 potential issue=
(s) to consider:
>- [High] Auto-committed BI decoders can be improperly assembled into regio=
ns even if BI path setup fails, leading to fatal hardware routing errors or=
 cache incoherence.

False positive, see below.

>--
>
>--- Patch [8]: [PATCH v7 8/8] cxl: Allow auto-committed BI hdm decoders ---
>commit 5bfa0978968677982ba4863deb2a8257dcfab9b1
>Author: Davidlohr Bueso <[email protected]>
>
>cxl: Allow auto-committed BI hdm decoders
>
>This commit allows auto-committed BI HDM decoders on capable platforms.
>It ensures that committed BI decoders are not assembled into regions
>with mismatched coherency models or non-BI windows.
>
>> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
>> --- a/drivers/cxl/core/region.c
>> +++ b/drivers/cxl/core/region.c
>> @@ -1839,6 +1854,16 @@ static int cxl_region_attach_auto(struct cxl_regi=
on *cxlr,
>>  		return -EINVAL;
>>  	}
>>
>> +	/* A committed decoder may only join a region of its own flavor */
>> +	if (cxled_committed_bi(cxled) !=3D
>> +	    (cxlr->type =3D=3D CXL_DECODER_DEVMEM &&
>> +	     cxl_root_decoder_is_bi(cxlr->cxlrd))) {
>> +		dev_err(&cxlr->dev, "%s:%s coherency model mismatch\n",
>> +			dev_name(&cxled_to_memdev(cxled)->dev),
>> +			dev_name(&cxled->cxld.dev));
>> +		return -ENXIO;
>> +	}
>> +
>
>[ ... ]
>
>> @@ -3806,6 +3831,14 @@ static struct cxl_region *construct_region(struct=
 cxl_root_decoder *cxlrd,
>>  	if (part < 0)
>>  		return ERR_PTR(-EBUSY);
>>
>> +	/* avoid UB */
>> +	if (cxled_committed_bi(cxled) && !cxl_root_decoder_is_bi(cxlrd)) {
>> +		dev_err(cxlmd->dev.parent,
>> +			"%s:%s BI decoder in a non-BI window\n",
>> +			dev_name(&cxlmd->dev), dev_name(&cxled->cxld.dev));
>> +		return ERR_PTR(-ENXIO);
>> +	}
>> +
>
>[ ... ]
>
>> diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c
>> --- a/drivers/cxl/port.c
>> +++ b/drivers/cxl/port.c
>> @@ -181,6 +181,10 @@ static int cxl_endpoint_port_probe(struct cxl_port =
*port)
>>  	if (rc)
>>  		return rc;
>>
>> +	/*
>> +	 * Must precede region discovery so that any firmware-committed BI
>> +	 * decoder is adopted before its region is assembled.
>> +	 */
>>  	rc =3D cxl_bi_setup(port);
>>  	if (rc)
>>  		dev_dbg(&port->dev, "BI setup failed rc=3D%d\n", rc);
>
>[Severity: High]
>If cxl_bi_setup() fails in cxl_endpoint_port_probe() (for example, due to
>register read/write failures on intermediate switches), the error is only
>logged and probe continues.
>
>Since cxl_region_attach_auto() and construct_region() authorize the decoder
>to join a BI region based on cxled_committed_bi() checking the hardware st=
ate
>instead of checking the software state (cxlds->bi), could an auto-committed
>BI decoder be improperly assembled into a region even if its path setup fa=
iled?

No, the software state is checked first and it is fatal to the attach.

Auto-assembly runs cxl_add_to_region() -> construct_region() ->
attach_target() -> cxl_region_attach(), and cxl_region_attach() does
before anything else:

	if (cxlr->type =3D=3D CXL_DECODER_DEVMEM &&
	    cxl_root_decoder_is_bi(cxlrd) && !cxlds->bi) {
		dev_err(&cxlr->dev, "%s:%s BI not enabled on device\n", ...);
		return -ENXIO;
	}

So for a device-coherent region under a BI window must have cxlds->bi set.

>
>Does this risk fatal hardware routing errors or silent cache incoherence if
>the OS failed to properly configure or adopt the BI path?
>
>--=20
>Sashiko AI review =B7 https://sashiko.dev/#/patchset/20260728144136.709882=
[email protected]?part=3D8