Re: [PATCH v5 12/16] power: sequencing: pcie-m2: support matching on remote "port" node

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] Missing `serdev_controller_put()` in `pwrseq_pcie_m2_create_serdev_one()` returns path.
- [High] Unchecked `pdev->dev.parent` in `pwrseq_pcie_m2_notify()` leads to NULL pointer dereference.
- [High] Mixing `devm` registration with manual resource cleanup causes Use-After-Free of regulator consumers.
--

--- Patch [12]: [PATCH v5 12/16] power: sequencing: pcie-m2: support matching on remote "port" node ---
commit 8eb380a4e579162e00e556321400077f29d6c07e
Author: Chen-Yu Tsai <wenst@chromium.org>

power: sequencing: pcie-m2: support matching on remote "port" node

This commit updates the pcie-m2 power sequencing driver to support matching
against remote "port" nodes for USB hubs. It uses of_graph_get_remote_port()
and falls back to the remote port parent for other connection types. It
also updates the existing matching logic to use device_match_of_node().

> diff --git a/drivers/power/sequencing/pwrseq-pcie-m2.c b/drivers/power/sequencing/pwrseq-pcie-m2.c
> index ddfe6ca824945..01f9f6a717464 100644
> --- a/drivers/power/sequencing/pwrseq-pcie-m2.c
> +++ b/drivers/power/sequencing/pwrseq-pcie-m2.c
> @@ -175,9 +175,16 @@ static int pwrseq_pcie_m2_match(struct pwrseq_device *pwrseq,
>  	 * parent matches the OF node of 'dev'.
>  	 */
>  	for_each_endpoint_of_node(ctx->of_node, endpoint) {
> +		/* USB port devices are tied to the port nodes. */
> +		struct device_node *remote_port __free(device_node) =
> +				of_graph_get_remote_port(endpoint);
> +		if (device_match_of_node(dev, remote_port))
> +			return PWRSEQ_MATCH_OK;
> +
> +		/* Try the remote port parent for other types. */
>  		struct device_node *remote __free(device_node) =
>  				of_graph_get_remote_port_parent(endpoint);
> -		if (remote && (remote == dev_of_node(dev)))
> +		if (device_match_of_node(dev, remote))
>  			return PWRSEQ_MATCH_OK;
>  	}
>  

[Severity: High]
This isn't a bug introduced by this patch, but does 
pwrseq_pcie_m2_create_serdev_one() leak the serdev_ctrl reference on the
early return path?

If the serdev device was already created for the PCI device, the list
iteration returns 0 directly:

drivers/power/sequencing/pwrseq-pcie-m2.c:pwrseq_pcie_m2_create_serdev_one() {
    ...
    scoped_guard(mutex, &ctx->list_lock) {
        list_for_each_entry(pci_dev, &ctx->pci_devices, list) {
            if (pci_dev->pdev == pdev)
                return 0;
        }
    }
    ...
}

This appears to bypass the serdev_controller_put(serdev_ctrl) cleanup. Could
this accumulate leaked references during PCI bus probing or device
notification callbacks?

[Severity: High]
This is a pre-existing issue, but could pwrseq_pcie_m2_notify() trigger a
NULL pointer dereference on systems with a root PCI bus?

Some bus scanning flows use pci_create_root_bus(NULL, ...), where devices
have no parent. The BUS_NOTIFY_ADD_DEVICE notifier is registered globally
and evaluates this without checking if pdev->dev.parent is NULL:

drivers/power/sequencing/pwrseq-pcie-m2.c:pwrseq_pcie_m2_notify() {
    ...
    if (!pci_parent || (pci_parent != pdev->dev.parent->of_node))
        return NOTIFY_DONE;
    ...
}

Could this cause a kernel panic during PCI bus probing on affected systems
when the driver is active?

[Severity: High]
This isn't a bug introduced by this patch, but does mixing devm registration
with manual regulator cleanup cause a use-after-free?

In the probe error path and remove function, the regulators are manually
freed:

drivers/power/sequencing/pwrseq-pcie-m2.c:pwrseq_pcie_m2_probe() {
    ...
err_free_regulators:
    regulator_bulk_free(ctx->num_vregs, ctx->regs);
    ...
}

drivers/power/sequencing/pwrseq-pcie-m2.c:pwrseq_pcie_m2_remove() {
    ...
    regulator_bulk_free(ctx->num_vregs, ctx->regs);
    ...
}

Since the power sequencer device is registered via
devm_pwrseq_device_register(), it remains active until the devres teardown
runs after the function returns. If a consumer calls a power sequencing
operation during this window, or if the driver is unbound via sysfs, will
it access the freed regulator consumer objects?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260715085348.3457359-1-wenst@chromium.org?part=12