Re: [PATCH 2/6] PCI/pwrctrl: tc9563: Fix parsing the integrated Ethernet MAC Endpoint node

[sashiko-bot@kernel.org]

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

New issues:
- [Low] Violation of Cleanup Subsystem rules by mixing `goto`-based error handling and `__free` scope-based cleanup.

Pre-existing issues:
- [High] Out-of-bounds array access due to missing bounds check on `port` variable.
- [High] Unbalanced regulator disable in error and remove paths.
--

--- Patch [2]: [PATCH 2/6] PCI/pwrctrl: tc9563: Fix parsing the integrated Ethernet MAC Endpoint node ---
commit 23e1126e5da4215de8157dc6521fdb07d6e7dcfa
Author: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>

PCI/pwrctrl: tc9563: Fix parsing the integrated Ethernet MAC Endpoint node

DSP3 has an integrated Ethernet MAC Endpoint which has its own set of
config registers for configuring settings such as ASPM. The Endpoint
device has 2 physical functions and those 2 functions share the same
settings.

> diff --git a/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c b/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
> index 488e1ec34a7fb..482e82e433dd5 100644
> --- a/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
> +++ b/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
> @@ -596,12 +596,17 @@ static int tc9563_pwrctrl_probe(struct platform_device *pdev)
>  		ret = tc9563_pwrctrl_parse_device_dt(tc9563, child, port);
>  		if (ret)
>  			break;

[Severity: High]
This isn't a bug introduced by this patch, but does the outer loop in
tc9563_pwrctrl_probe() lack a bounds check on the port variable?

If there are 5 or more children in the device tree, port could reach
TC9563_MAX or greater, leading to an out-of-bounds write on the
tc9563->cfg array.

This could overwrite subsequent fields like reset_gpio or adapter, causing
a crash when those pointers are subsequently dereferenced.

> -		/* Embedded ethernet device are under DSP3 */
> +
> +		/*
> +		 * The integrated Ethernet MAC Endpoint under DSP3 is a single
> +		 * device whose functions share the same config registers.
> +		 */
>  		if (port == TC9563_DSP3) {
> -			for_each_child_of_node_scoped(child, child1) {
> -				port++;
> +			struct device_node *eth __free(device_node) =
> +					of_get_next_available_child(child, NULL);

[Severity: Low]
Does introducing __free(device_node) here conflict with the goto-based error
handling used elsewhere in this function?

The cleanup guidelines state that the usage of goto and cleanup helpers
should never be mixed in the same function to avoid confusing ownership
semantics.

> +			if (eth) {
>  				ret = tc9563_pwrctrl_parse_device_dt(tc9563,
> -								child1, port);
> +								eth, TC9563_ETHERNET);
>  				if (ret)
>  					break;
>  			}

[ ... ]

[Severity: High]
This is a pre-existing issue, but does the error path in
tc9563_pwrctrl_probe() and tc9563_pwrctrl_remove() cause an unbalanced
regulator disable?

If devm_pci_pwrctrl_device_set_ready() fails in tc9563_pwrctrl_probe():

    ret = devm_pci_pwrctrl_device_set_ready(dev, &tc9563->pwrctrl);
    if (ret)
        goto power_off;
...
power_off:
    tc9563_pwrctrl_power_off(&tc9563->pwrctrl);

It jumps to power_off and calls tc9563_pwrctrl_power_off() which disables
the regulators. However, the probe function never enabled them (they are
only enabled asynchronously by the PCI framework later).

Could this mistakenly disable shared regulators and cause system instability?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260725-tc9563-fix-v1-0-ec4286e31331@oss.qualcomm.com?part=2