Re: [PATCH v2 2/3] soc: apple: Add driver for Apple PMGR misc controls

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The driver silently masks initialization errors (-ENOMEM) and improperly treats required resources as optional.
- [High] Missing register readback after modifying power states allows the CPU to resume and access unpowered fabric or memory controller domains.
- [High] Asymmetrical suspend and resume sequence for interdependent SoC components breaks dependency chains.
--

commit b1459d0f98e7c6089c43b0f7e6af7581425b2f1e
Author: Hector Martin <marcan@marcan.st>

soc: apple: Add driver for Apple PMGR misc controls
    
Adds a driver for miscellaneous PMGR controls on Apple SoCs to manage
fabric and memory controller power state switching on system standby.

> diff --git a/drivers/soc/apple/apple-pmgr-misc.c b/drivers/soc/apple/apple-pmgr-misc.c
> new file mode 100644
> index 0000000000000..ae8b34f298e8c
> --- /dev/null
> +++ b/drivers/soc/apple/apple-pmgr-misc.c

[ ... ]

> +static void apple_pmgr_sys_dev_set_pstate(struct apple_pmgr_misc *misc,
> +					  enum sys_device dev, bool active)
> +{

[ ... ]

> +	val = readl_relaxed(misc->devices[dev].base + APPLE_CLKGEN_PSTATE);
> +	val &= ~APPLE_CLKGEN_PSTATE_DESIRED;
> +	val |= FIELD_PREP(APPLE_CLKGEN_PSTATE_DESIRED, pstate);
> +	writel_relaxed(val, misc->devices[dev].base + APPLE_CLKGEN_PSTATE);

[Severity: High]
Because writel_relaxed() is weakly ordered on ARM64, could this write remain
in the CPU store buffer?

If we do not perform a readback to flush the write, could dependent devices
attempt to access unpowered fabric or memory controller domains when the PM
core resumes them, potentially leading to a system crash?

> +}
> +
> +static int __maybe_unused apple_pmgr_misc_suspend_noirq(struct device *dev)
> +{
> +	struct apple_pmgr_misc *misc = dev_get_drvdata(dev);
> +	int i;
> +
> +	for (i = 0; i < DEV_MAX; i++)
> +		apple_pmgr_sys_dev_set_pstate(misc, i, false);
> +
> +	return 0;
> +}
> +
> +static int __maybe_unused apple_pmgr_misc_resume_noirq(struct device *dev)
> +{
> +	struct apple_pmgr_misc *misc = dev_get_drvdata(dev);
> +	int i;
> +
> +	for (i = 0; i < DEV_MAX; i++)
> +		apple_pmgr_sys_dev_set_pstate(misc, i, true);

[Severity: High]
Should this loop iterate in reverse order?

The apple_pmgr_misc_suspend_noirq() function suspends components starting
from index 0, and this function resumes them in the exact same forward order.
Does this break strict reverse-order bringup dependencies for these
interdependent components?

> +
> +	return 0;
> +}
> +
> +static bool apple_pmgr_init_device(struct apple_pmgr_misc *misc,
> +				   const struct apple_pmgr_misc_hw *hw,
> +				   enum sys_device dev,
> +				   const char *device_name)
> +{
> +	void __iomem *base;
> +	char name[32];
> +	u32 val;
> +
> +	snprintf(name, sizeof(name), "%s-ps", device_name);
> +
> +	base = devm_platform_ioremap_resource_byname(
> +		to_platform_device(misc->dev), name);
> +	if (IS_ERR(base))
> +		return false;

[Severity: High]
Does this drop the error code returned by
devm_platform_ioremap_resource_byname()? 

If memory allocation fails with -ENOMEM or the property is missing,
converting the result to a boolean false masks the error pointer.

> +
> +	val = readl_relaxed(base + APPLE_CLKGEN_PSTATE);
> +
> +	misc->devices[dev].base = base;
> +	misc->devices[dev].active_state =
> +		FIELD_GET(APPLE_CLKGEN_PSTATE_DESIRED, val);
> +	misc->devices[dev].suspend_state = hw->dev_min_ps[dev];
> +
> +	return true;
> +}
> +
> +static int apple_pmgr_misc_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	const struct apple_pmgr_misc_hw *hw;
> +	struct apple_pmgr_misc *misc;
> +	int ret = -ENODEV;
> +
> +	misc = devm_kzalloc(dev, sizeof(*misc), GFP_KERNEL);
> +	if (!misc)
> +		return -ENOMEM;
> +
> +	misc->dev = dev;
> +	hw = of_device_get_match_data(dev);
> +	if (!hw)
> +		return -EINVAL;
> +
> +	if (apple_pmgr_init_device(misc, hw, DEV_FABRIC, "fabric"))
> +		ret = 0;
> +
> +	if (apple_pmgr_init_device(misc, hw, DEV_DCS, "dcs"))
> +		ret = 0;

[Severity: High]
If one of these initializations fails, does this logic improperly treat
required resources as optional?

The probe function will succeed and return 0 as long as at least one
device maps successfully. Does this silently leave critical SoC components
unmanaged if another initialization fails?

> +
> +	platform_set_drvdata(pdev, misc);
> +
> +	return ret;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260703-pmgr-misc-v2-0-4b26ba10c5a4@chaosmail.tech?part=2