Re: [PATCH 4/6] PCI/pwrctrl: tc9563: Skip Tx amplitude and DFE tuning for DSP3

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The error path in probe and the remove function unconditionally call `tc9563_pwrctrl_power_off()`, leading to an unbalanced regulator disable and GPIO reset.
- [Medium] The patch adds a skip condition for TC9563_DSP3 to prevent driver failure when tuning properties are present, but fails to handle TC9563_ETHERNET which suffers from the exact same vulnerability if tuning properties are erroneously provided.
--

--- Patch [4]: [PATCH 4/6] PCI/pwrctrl: tc9563: Skip Tx amplitude and DFE tuning for DSP3 ---
commit 1ca1e7c52e5adecc6adfed2aec9cf99588aec9d4
Author: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>

PCI/pwrctrl: tc9563: Skip Tx amplitude and DFE tuning for DSP3

This commit skips Tx amplitude and DFE tuning for DSP3 by returning 0
when these tuning properties are passed. This prevents the driver from
returning an error and failing to probe.

> diff --git a/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c b/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
> index ca19bf60d290d..28e9ec3e172ae 100644
> --- a/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
> +++ b/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
[ ... ]
> @@ -327,6 +328,9 @@ static int tc9563_pwrctrl_set_tx_amplitude(struct tc9563_pwrctrl *tc9563,
>  	case TC9563_DSP2:
>  		port_access = 0x8;
>  		break;
> +	case TC9563_DSP3:
> +		dev_dbg(dev, "Tx amplitude tuning not supported for DSP3\n");
> +		return 0;
>  	default:
>  		return -EINVAL;

[Severity: Medium]
This is a pre-existing issue, but should TC9563_ETHERNET also be handled
to avoid returning -EINVAL here?

If the device tree mistakenly provides tuning properties for the Ethernet MAC
Endpoint node, the driver's power-on sequence fails entirely because
tc9563_pwrctrl_power_on() iterates up to TC9563_MAX.

The patch fixes this exact flaw for TC9563_DSP3, but does TC9563_ETHERNET
suffer from the same vulnerability if tuning properties are erroneously
provided?

>  	}
[ ... ]
> @@ -363,6 +368,9 @@ static int tc9563_pwrctrl_disable_dfe(struct tc9563_pwrctrl *tc9563,
>  		port_access = 0x8;
>  		lane_access = 0x1;
>  		break;
> +	case TC9563_DSP3:
> +		dev_dbg(dev, "DFE tuning not supported for DSP3\n");
> +		return 0;
>  	default:
>  		return -EINVAL;

[Severity: Medium]
This is a pre-existing issue, but does this default case also cause the driver
to fail if tuning properties are provided for TC9563_ETHERNET?

>  	}

[Severity: High]
This is a pre-existing issue, but does the error path in probe and the
remove function unconditionally call tc9563_pwrctrl_power_off(), leading to an
unbalanced regulator disable?

If devm_pci_pwrctrl_device_set_ready() fails during tc9563_pwrctrl_probe(),
the driver jumps to power_off:

drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c:tc9563_pwrctrl_probe() {
    ...
power_off:
    tc9563_pwrctrl_power_off(&tc9563->pwrctrl);
}

Because probe never calls regulator_bulk_enable(), does the error path disable
regulators that were never enabled by this driver?

Additionally, tc9563_pwrctrl_remove() unconditionally calls
tc9563_pwrctrl_power_off(). The PCI power control subsystem automatically
powers down devices when they are unbound, meaning remove() calls
.power_off() on an already powered-off device.

Can this unbalanced disable produce kernel warnings and mess up regulator
use counts?

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