Re: [PATCH v27 04/10] firmware: psci: Add support for PSCI auxiliary devices

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] firmware: psci: premature fw_devlink consumer pickup in arm_psci_probe()
--

commit 693a73228dca9776538a9dc808fb4cd1f0b1d4d0
Author: Shivendra Pratap <shivendra.pratap@oss.qualcomm.com>

firmware: psci: Add support for PSCI auxiliary devices

This commit introduces a PSCI platform driver that binds to the "arm,psci-1.0"
device node and creates child devices on the auxiliary bus. It also converts
cpuidle-psci-domain from a platform driver into an auxiliary driver that binds
to the child device created by the PSCI provider.

> diff --git a/drivers/firmware/psci/psci-devices.c b/drivers/firmware/psci/psci-devices.c
> new file mode 100644
> index 0000000000000..d23befc490ed5
> --- /dev/null
> +++ b/drivers/firmware/psci/psci-devices.c

[ ... ]

> +static int arm_psci_probe(struct platform_device *pdev)
> +{
> +	const struct arm_psci_device_info *info;
> +	int auxdev_count = 0;
> +	int i, ret;
> +
> +	for (i = 0; i < ARRAY_SIZE(arm_psci_devices); i++) {
> +		info = &arm_psci_devices[i];
> +		ret = arm_psci_auxiliary_device_create(&pdev->dev, info);
> +		if (ret)
> +			dev_warn(&pdev->dev, "failed to create %s aux device: %d\n",
> +				 info->name, ret);
> +		else
> +			auxdev_count++;
> +	}
> +
> +	return auxdev_count ? 0 : -ENODEV;
> +}

[Severity: Medium]
Can returning 0 from arm_psci_probe() when psci_cpuidle_domain_probe() defers
(or probes asynchronously) cause fw_devlink to prematurely mark the child
power-domain device tree nodes as FWNODE_FLAG_NOT_DEVICE?

Previously, psci_cpuidle_domain_driver was the platform_driver for the
"arm,psci-1.0" platform device (where psci_fwnode->dev == &pdev->dev). When
psci_cpuidle_domain_probe() returned -EPROBE_DEFER, really_probe() did not
call device_links_driver_bound(&pdev->dev).

On platforms where a PSCI child power domain references an external parent
power domain (for example arch/arm64/boot/dts/qcom/agatti.dtsi and
arch/arm64/boot/dts/qcom/sm6375.dtsi, where cluster_pd has
power-domains = <&mpm> pointing to "qcom,mpm"), arm_psci_probe() and
psci_cpuidle_domain_probe() run during of_platform_default_populate_init() at
arch_initcall_sync, while qcom_mpm_driver (drivers/irqchip/irq-qcom-mpm.c) is
registered at device_initcall (or as a module).

When arm_psci_probe() -> arm_psci_auxiliary_device_create() ->
__auxiliary_device_add() -> device_add() runs, psci_cpuidle_domain_probe()
calls dt_idle_pd_init_topology() -> of_genpd_add_subdomain(), which returns
-EPROBE_DEFER because the mpm genpd provider is not registered yet.
psci_cpuidle_domain_probe() then unwinds:

drivers/cpuidle/cpuidle-psci-domain.c:psci_cpuidle_domain_probe() {
	...
	/* Link genpd masters/subdomains to model the CPU topology. */
	ret = dt_idle_pd_init_topology(np);
	if (ret)
		goto remove_pd;
	...
remove_pd:
	dt_idle_pd_remove_topology(np);
	psci_pd_remove();
exit:
	pr_err("failed to create CPU PM domains ret=%d\n", ret);
	return ret;
}

Here psci_pd_remove() -> of_genpd_del_provider() -> device_del(&gpd->dev)
resets child_fwnode->dev to NULL for all child power-domain nodes under the
PSCI node.

Because bus_probe_device(&auxdev->dev) does not propagate probe deferral to
device_add(&auxdev->dev), arm_psci_auxiliary_device_create() and
arm_psci_probe() still return 0. really_probe(&pdev->dev) then calls
driver_bound(&pdev->dev) -> device_links_driver_bound(&pdev->dev):

drivers/base/core.c:device_links_driver_bound() {
	...
	if (dev->fwnode && dev->fwnode->dev == dev) {
		fwnode_links_purge_suppliers(dev->fwnode);
		fw_devlink_pickup_dangling_consumers(dev);
	}
	...
}

Because child_fwnode->dev is NULL on the child power-domain nodes (which also
happens if psci-cpuidle-domain probes asynchronously),
__fw_devlink_pickup_dangling_consumers() sets FWNODE_FLAG_NOT_DEVICE on each
child power-domain fwnode and moves their consumer fwnode_links to the parent
PSCI platform device (&pdev->dev).

When the supplier device (mpm_pdev) is later added,
__fw_devlink_link_to_consumers(&mpm_pdev->dev) sees
get_dev_from_fwnode(cluster_pd_fwnode) == NULL, falls back to
fwnode_get_next_parent_dev(cluster_pd_fwnode) (&pdev->dev), and attempts to
create a DL_FLAG_SYNC_STATE_ONLY device link to &pdev->dev. Because
pdev->dev.links.status is already DL_DEV_DRIVER_BOUND, device_link_add() fails
and logs "Failed to create device link", leaving mpm_pdev without a
SYNC_STATE_ONLY link.

Furthermore, because FWNODE_FLAG_NOT_DEVICE is never cleared, even after
psci_cpuidle_domain_probe() succeeds on deferred probe retry and registers
genpd->dev via of_genpd_add_provider_simple(), fw_devlink_create_devlink()
keeps redirecting consumers of the child power-domain nodes to &pdev->dev:

drivers/base/core.c:fw_devlink_create_devlink() {
	...
	if (fwnode_test_flag(sup_handle, FWNODE_FLAG_NOT_DEVICE))
		sup_dev = fwnode_get_next_parent_dev(sup_handle);
	else
		sup_dev = get_dev_from_fwnode(sup_handle);
	...
}

Doesn't this leave the child genpd->dev devices without consumer device links
to hold off genpd_provider_sync_state()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261029-arm-psci-system_reset2-vendor-reboots-v27-0-fa4b3ff1359b@oss.qualcomm.com?part=4