Re: [PATCH v2 02/13] PCI/P2PDMA: Report ACS ports when the paths share no upstream bridge

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Unconditional host bridge whitelist warnings cause severe log spam during silent P2P provider discovery.
--

--- Patch [2]: [PATCH v2 02/13] PCI/P2PDMA: Report ACS ports when the paths share no upstream bridge ---
commit b1b70f40984c0b642c7221458a704c5fa967c62e
Author: Leon Romanovsky <[email protected]>

PCI/P2PDMA: Report ACS ports when the paths share no upstream bridge

A provider and a client below different Root Ports share no upstream
bridge, so the search ends without a match and the walk takes the
host-bridge route directly. Neither path is examined for ACS, so no
redirect diagnostic is emitted and the host-bridge whitelist warning
stays suppressed.

Examine the paths before taking that route, so every redirecting port is
named in the pci=disable_acs_redir= hint.

> diff --git a/drivers/pci/p2pdma.c b/drivers/pci/p2pdma.c
> index 2d3b380cc5781..801c183ae6b7e 100644
> --- a/drivers/pci/p2pdma.c
> +++ b/drivers/pci/p2pdma.c
> @@ -740,8 +741,12 @@ calc_map_type_and_dist(struct pci_dev *provider, struct pci_dev *client,
>  		dist_a++;
>  	}
>  
> -	*dist = dist_a + dist_b;
> -	goto map_through_host_bridge;
> +	/*
> +	 * The paths share no upstream bridge, so the request can only reach
> +	 * the peer through the host bridge. Examine the client path anyway,
> +	 * so the diagnostics below name every ACS port on both paths.
> +	 */
> +	no_common_upstream = true;
>  
>  check_b_path_acs:
>  	bb = b;

[ ... ]

> @@ -761,6 +766,9 @@ calc_map_type_and_dist(struct pci_dev *provider, struct pci_dev *client,
>  	*dist = dist_a + dist_b;
>  
>  	if (!acs_cnt) {
> +		if (no_common_upstream)
> +			goto map_through_host_bridge;
> +
>  		map_type = PCI_P2PDMA_MAP_BUS_ADDR;
>  		goto done;
>  	}

[Severity: Medium]
Will this change cause severe log spam during silent P2P discovery?

Previously, when there was no common upstream bridge, the code jumped directly
to map_through_host_bridge before checking ACS paths, leaving acs_redirects
set to false.

Now that the code falls through and evaluates the ACS paths, acs_redirects can
be set to true even when verbose is false. This true value is then passed into
host_bridge_whitelist():

drivers/pci/p2pdma.c:calc_map_type_and_dist() {
    ...
    if (verbose) {
        /* diagnostics ... */
    }
    acs_redirects = true;

map_through_host_bridge:
    if (!cpu_supports_p2pdma() &&
        !host_bridge_whitelist(provider, client, acs_redirects)) {
    ...
}

Since __host_bridge_whitelist() triggers its warning based solely on that
argument, bypassing the caller's verbose constraint:

drivers/pci/p2pdma.c:__host_bridge_whitelist() {
    ...
    if (warn)
        pci_warn(root, "Host bridge not in P2PDMA whitelist: %04x:%04x\n",
                 vendor, device);
    ...
}

Does calc_map_type_and_dist() need to pass (verbose && acs_redirects) instead
of just acs_redirects to prevent filling dmesg with warnings during silent
provider discovery loops?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2