Re: [PATCH v2 2/2] pmdomain: riscv: Add RPMI device power service

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Strict equality check of power state includes context and reserved bits, breaking domain probe and power off.
- [High] The driver ignores the return value of `pm_genpd_init()`, which leads to list corruption and a kernel crash in the error rollback path.
- [Medium] Missing sync_state callback prevents domains initialized as ON from ever powering off.
- [Low] Kconfig dependency allows compilation without MAILBOX, causing linker errors.
- [Low] Domain name truncation when hardware provides a full 16-character name.
--

commit c4b30c55f0c6f514e587b60c8050b357cd8cdaaa
Author: Joshua Yeong <joshua.yeong@starfivetech.com>

pmdomain: riscv: Add RPMI device power service

This commit adds a generic power domain provider based on the RISC-V RPMI
device power service group. It enumerates the device power domains, queries
their attributes and state, and registers them as a genpd provider.

> diff --git a/drivers/pmdomain/riscv/Kconfig b/drivers/pmdomain/riscv/Kconfig
> new file mode 100644
> index 0000000000000..5c2ef53218a3e
> --- /dev/null
> +++ b/drivers/pmdomain/riscv/Kconfig
> @@ -0,0 +1,13 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +
> +config RISCV_RPMI_DEVICE_POWER
> +	bool "RISC-V RPMI Based Device Power driver"
> +	depends on MAILBOX || COMPILE_TEST

[Severity: Low]
Does this allow the driver to be built when COMPILE_TEST=y and MAILBOX=n?

The RISCV_RPMI_DEVICE_POWER driver uses mailbox APIs like
mbox_request_channel() in drivers/pmdomain/riscv/riscv-rpmi-device-power.c,
but include/linux/mailbox_client.h does not provide dummy stubs when
CONFIG_MAILBOX is not set, which could lead to undefined reference linker
errors.

> +	default RISCV
> +	select PM_GENERIC_DOMAINS if PM

[ ... ]

> diff --git a/drivers/pmdomain/riscv/riscv-rpmi-device-power.c b/drivers/pmdomain/riscv/riscv-rpmi-device-power.c
> new file mode 100644
> index 0000000000000..6590d622c2a44
> --- /dev/null
> +++ b/drivers/pmdomain/riscv/riscv-rpmi-device-power.c

[ ... ]

> +struct rpmi_device_power_domain {
> +	u32 id;
> +	u32 transition_latency;
> +	struct device *dev;
> +	struct rpmi_ctx *mpxy_ctx;
> +	struct generic_pm_domain genpd;
> +	char name[RPMI_PM_DOMAIN_NAME_LEN];

[Severity: Low]
If the hardware provides a full 16-character name in rx.name, does this
truncate the last character?

In rpmi_device_power_get_attrs(), strscpy() copies into this buffer:

	strscpy(mpxy_pm_domain->name, rx.name, sizeof(mpxy_pm_domain->name));

strscpy() reserves one byte for the NUL terminator, which might cause
identical names and conflicts in genpd debugfs if the original hardware
names differ only in the 16th character.

> +};

[ ... ]

> +static int rpmi_pd_power(struct generic_pm_domain *domain, bool power_on)
> +{
> +	struct rpmi_device_power_domain *mpxy_pm_domain;
> +	u32 state, ret_state, domain_id;
> +	int ret;
> +
> +	if (power_on)
> +		state = RPMI_POWER_STATE_GENERIC_ON;
> +	else
> +		state = RPMI_POWER_STATE_GENERIC_OFF;
> +
> +	mpxy_pm_domain = to_rpmi_pd(domain);
> +	domain_id = mpxy_pm_domain->id;
> +
> +	ret = rpmi_power_state_set(mpxy_pm_domain, domain_id, state);
> +	if (!ret)
> +		ret = rpmi_power_state_get(mpxy_pm_domain, domain_id, &ret_state);
> +	if (!ret && state != ret_state)

[Severity: High]
Can this strict equality check fail power state transitions if context bits
or reserved bits differ?

If the state is set to 3 but the hardware returns 0x10003 (indicating it
has lost context, with bit 16 set), this check in rpmi_pd_power() will
result in an -EIO error despite the hardware successfully powering down.

> +		return -EIO;
> +
> +	return ret;
> +}

[ ... ]

> +static int rpmi_pm_domain_probe(struct platform_device *pdev)
> +{

[ ... ]

> +	for (i = 0; i < num_domains; i++, mpxy_pd++) {
> +		u32 state;
> +
> +		mpxy_pd->dev = &pdev->dev;
> +		mpxy_pd->mpxy_ctx = mpxy_ctx;
> +		mpxy_pd->id = i;
> +
> +		ret = rpmi_device_power_get_attrs(i, mpxy_pd);
> +		if (ret) {
> +			dev_warn(mpxy_pd->dev,
> +				 "power domain %d initialization failed\n",
> +				 mpxy_pd->id);
> +			domains[i] = NULL;
> +			continue;
> +		}
> +
> +		ret = rpmi_power_state_get(mpxy_pd, i, &state);
> +		if (ret || (state != RPMI_POWER_STATE_GENERIC_OFF &&
> +			    state != RPMI_POWER_STATE_GENERIC_ON)) {

[Severity: High]
Can this strict equality check reject domains that have lost context?

If a domain is powered off and has lost context (e.g., bit 16 is set,
making the state 0x10003), this strict inequality check in
rpmi_pm_domain_probe() evaluates to true. The domain is discarded (set to
NULL), causing dependent peripheral devices to permanently fail to probe.

> +			dev_warn(mpxy_pd->dev,
> +				 "failed to get state for power domain %d\n",
> +				 mpxy_pd->id);
> +			domains[i] = NULL;
> +			continue;
> +		}
> +
> +		mpxy_pd->genpd.name = mpxy_pd->name;
> +		mpxy_pd->genpd.power_off = rpmi_pd_power_off;
> +		mpxy_pd->genpd.power_on = rpmi_pd_power_on;
> +
> +		pm_genpd_init(&mpxy_pd->genpd, NULL,
> +			      state == RPMI_POWER_STATE_GENERIC_OFF);

[Severity: High]
Does ignoring the return value of pm_genpd_init() here cause a kernel crash
during error rollback?

If pm_genpd_init() fails, the domain's gpd_list_node remains NULL. If a
subsequent operation fails (like of_genpd_add_provider_onecell) and
triggers the error path below, pm_genpd_remove() is called on the
uninitialized domain, which calls list_del() and dereferences the NULL
pointers.

> +
> +		domains[i] = &mpxy_pd->genpd;
> +	}
> +
> +	mpxy_pd_data->domains = domains;
> +	mpxy_pd_data->num_domains = num_domains;
> +
> +	ret = of_genpd_add_provider_onecell(np, mpxy_pd_data);
> +	if (ret) {
> +		while (i--) {
> +			if (domains[i])
> +				pm_genpd_remove(domains[i]);
> +		}
> +
> +		return dev_err_probe(dev, ret, "failed to add genpd provider\n");
> +	}
> +
> +	return 0;
> +}

[ ... ]

> +static struct platform_driver rpmi_pm_domain_platdrv = {
> +	.driver = {
> +		.name = "riscv-rpmi-device-power",
> +		.of_match_table = rpmi_pm_domain_of_match,
> +		.suppress_bind_attrs = true,
> +	},
> +	.probe = rpmi_pm_domain_probe,
> +};

[Severity: Medium]
Are domains initialized as ON at boot able to be powered off?

According to PM domain subsystem guidelines, domains with is_off=false get
the stay_on flag set by pm_genpd_init(). Since this driver neither sets
GENPD_FLAG_NO_STAY_ON on the domains nor provides a .sync_state callback
in its platform_driver struct, the stay_on flag is never cleared. This
leaves the domains powered indefinitely, potentially wasting power and
blocking regulator cleanup.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260830152812.312663-1-joshua.yeong@starfivetech.com?part=2