Re: [PATCH v9 02/10] cxl/pci: Add BI topology enable/disable

[Davidlohr Bueso <dave@stgolabs.net>]

On Wed, 23 Sep 2026, sashiko-bot@kernel.org wrote:

>Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
>- [Medium] cxl/pci: TOCTOU race in BI setup against concurrent AER reset

False positive, see below.

>--
>
>commit 1daa4037cbac0ebc16630c3d4f45c358bd7890f9
>Author: Davidlohr Bueso <dave@stgolabs.net>
>
>cxl/pci: Add BI topology enable/disable
>
>This patch implements setup and teardown of the Back-Invalidate (BI) topology
>on CXL endpoints and upstream components. It also adds a reset detection
>mechanism to verify whether a reset cleared the hardware BI Enable state.
>
>> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
>> index b8676a3d6ec90..f3ca9861a7ba1 100644
>> --- a/drivers/cxl/core/pci.c
>> +++ b/drivers/cxl/core/pci.c
>[ ... ]
>> +static int __cxl_bi_ctrl_endpoint(struct cxl_dev_state *cxlds, bool enable)
>> +{
>[ ... ]
>> +	FIELD_MODIFY(CXL_BI_DECODER_CTRL_BI_ENABLE, &ctrl, enable);
>> +	writel(ctrl, bi + CXL_BI_DECODER_CTRL_OFFSET);
>> +	cxlds->bi = enable;
>
>[Severity: Medium]
>Could this lockless update of cxlds->bi race with a concurrent AER reset
>handled by cxl_bi_reset_detected()?
>
>If a user manually binds the cxl_port driver via sysfs concurrently with a
>PCI AER reset (SBR), the manual sysfs bind takes the endpoint->dev lock,
>while the AER recovery (cxl_reset_done()) takes the cxlmd->dev lock, allowing
>them to execute concurrently.

The endpoint port cannot be bound on its own. Unbinding it from cxl_port
tears it down through cxl_detach_ep() and a subsequent sysfs bind fails
with -ENODEV. The only path that reaches __cxl_bi_ctrl_endpoint(enable) is
the endpoint port probe, and that runs synchronously from
devm_cxl_add_endpoint().

The driver core holds device_lock(&cxlmd->dev) across cxl_mem_probe(), and
cxl_reset_done() takes that same lock before calling
cxl_bi_reset_detected(). The reset side therefore cannot observe the
window between the writel() and the cxlds->bi store; it waits for the
probe to return and only then compares the flag against the hardware bit.

>
>There is a window between the writel() that sets the hardware bit and the
>assignment of cxlds->bi. If the AER SBR wipes the hardware bit in this window,
>could cxl_bi_reset_detected() check the still-false cxlds->bi and ignore the
>reset?

I widened the window with a 4s msleep() between the writel() and the store,
rebinded mem0 and issued an SBR through sysfs while the probe was sleeping
in that window:

   [16.846] cxl_pci 0000:0d:00.0: reset via cxl_bus
   [19.290] cxl_pci 0000:0d:00.0: BI requests enabled
   [19.335] cxl_mem mem0: probe: 0
   [19.337] cxl_pci 0000:0d:00.0: BI disabled by reset

The SBR itself lands in the window and clears BI Enable, but the reset
write blocks on the memdev lock until the probe completes. .reset_done then
runs, sees cxlds->bi set with the hardware bit clear, and clears the flag.
A subsequent HDM-DB attach is refused with "BI not enabled on device". The
software state does not desynchronize.

Independently of this, patch 4 re-reads BI Enable after committing each
endpoint decoder, so a stale flag could not commit a decoder with BI on a
device whose BI Enable is clear.

Thanks,
Davidlohr