[PATCH 0/6] PCI/pwrctrl: tc9563: Several fixes and improvements

[PATCH 0/6] PCI/pwrctrl: tc9563: Several fixes and improvements

[Manivannan Sadhasivam via B4 Relay]

Hi,

This series contains several fixes and improvements for the TC9563 Pwrctrl
driver and the switch binding. Most of the fixes are related to ensuring
correctness in parsing the DT properties for the switch ports and the
integrated Endpoint.

More details are in the individual patches.

Testing
=======

This series was validated on the RB3Gen2 board.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
---
Manivannan Sadhasivam (6):
      dt-bindings: PCI: toshiba,tc9563: Restrict Tx Amplitude, DFE and N_FTS to USP, DSP1 and DSP2
      PCI/pwrctrl: tc9563: Fix parsing the integrated Ethernet MAC Endpoint node
      PCI/pwrctrl: tc9563: Power off only the external ports in tc9563_pwrctrl_disable_port()
      PCI/pwrctrl: tc9563: Skip Tx amplitude and DFE tuning for DSP3
      PCI/pwrctrl: tc9563: Rename DSP3 to VDSP
      PCI/pwrctrl: tc9563: Move Integrated MAC Endpoint out of 'tc9563_pwrctrl_ports' enum

 .../devicetree/bindings/pci/toshiba,tc9563.yaml    |  21 +++-
 drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c           | 129 ++++++++++++++-------
 2 files changed, 104 insertions(+), 46 deletions(-)
---
base-commit: dc59e4fea9d83f03bad6bddf3fa2e52491777482
change-id: 20260725-tc9563-fix-64eb7cbb1229

Best regards,
--  
மணிவண்ணன் சதாசிவம்

[PATCH 1/6] dt-bindings: PCI: toshiba,tc9563: Restrict Tx Amplitude, DFE and N_FTS to USP, DSP1 and DSP2

[Manivannan Sadhasivam via B4 Relay]

From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>

TC9563 switch allows configuring Tx Amplitude, DFE and N_FTS settings only
to USP, DSP1 and DSP2 ports. These settings don't apply to DSP3 and also
the integrated Ethernet MAC Endpoint.

So restrict these properties and also move the
'toshiba,tx-amplitude-microvolt' property to DSP2 in the example to conform
to the binding.

Fixes: 26f3ac884835 ("dt-bindings: PCI: Add binding for Toshiba TC9563 PCIe switch")
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
---
 .../devicetree/bindings/pci/toshiba,tc9563.yaml     | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/pci/toshiba,tc9563.yaml b/Documentation/devicetree/bindings/pci/toshiba,tc9563.yaml
index b3ad05d90201..f084830c6d0c 100644
--- a/Documentation/devicetree/bindings/pci/toshiba,tc9563.yaml
+++ b/Documentation/devicetree/bindings/pci/toshiba,tc9563.yaml
@@ -54,7 +54,7 @@ properties:
           - description: I2C slave address
 
 patternProperties:
-  "^pcie@[1-3],0$":
+  "^pcie@[1-2],0$":
     description:
       child nodes describing the internal downstream ports of
       the tc9563 switch.
@@ -64,6 +64,21 @@ patternProperties:
       - $ref: /schemas/pci/pci-pci-bridge.yaml#
     unevaluatedProperties: false
 
+  "^pcie@[3],0$":
+    description:
+      child node describing the internal downstream port connected to the
+      integrated Ethernet MAC endpoint of the tc9563 switch.
+    type: object
+    $ref: /schemas/pci/pci-pci-bridge.yaml#
+    properties:
+      n-fts: false
+    patternProperties:
+      "^ethernet@0,[0-1]$":
+        type: object
+        properties:
+          n-fts: false
+    unevaluatedProperties: false
+
 $defs:
   tc9563-node:
     type: object
@@ -154,6 +169,8 @@ examples:
                     device_type = "pci";
                     ranges;
                     bus-range = <0x04 0xff>;
+
+                    toshiba,tx-amplitude-microvolt = <10>;
                 };
 
                 pcie@3,0 {
@@ -165,8 +182,6 @@ examples:
                     ranges;
                     bus-range = <0x05 0xff>;
 
-                    toshiba,tx-amplitude-microvolt = <10>;
-
                     ethernet@0,0 {
                         reg = <0x50000 0x0 0x0 0x0 0x0>;
                     };

-- 
2.43.0

Re: [PATCH 1/6] dt-bindings: PCI: toshiba,tc9563: Restrict Tx Amplitude, DFE and N_FTS to USP, DSP1 and DSP2

[sashiko-bot]

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

New issues:
- [Medium] Missing schema constraints on Ethernet MAC node silently allows all undocumented properties.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but there is an out-of-bounds array write in `tc9563_pwrctrl_probe` when parsing multiple Ethernet child nodes.
- [High] This problem wasn't introduced by this patch, but the driver assigns PCI switch port configurations based on fragile child node iteration order rather than `reg` addresses.
--

--- Patch [1]: [PATCH 1/6] dt-bindings: PCI: toshiba,tc9563: Restrict Tx Amplitude, DFE and N_FTS to USP, DSP1 and DSP2 ---
commit 54f91cfce5dde398e9e08b16babbdffbd75f5ca1
Author: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>

dt-bindings: PCI: toshiba,tc9563: Restrict Tx Amplitude, DFE and N_FTS to USP, DSP1 and DSP2

This patch restricts Tx Amplitude, DFE, and N_FTS settings to USP, DSP1,
and DSP2 ports in the toshiba,tc9563 PCIe switch binding. It updates the
schema to explicitly disallow these properties on the DSP3 port and the
integrated Ethernet MAC endpoint, and adjusts the example.

> diff --git a/Documentation/devicetree/bindings/pci/toshiba,tc9563.yaml b/Documentation/devicetree/bindings/pci/toshiba,tc9563.yaml
> index b3ad05d90201c..f084830c6d0cb 100644
> --- a/Documentation/devicetree/bindings/pci/toshiba,tc9563.yaml
> +++ b/Documentation/devicetree/bindings/pci/toshiba,tc9563.yaml
[ ... ]
> @@ -64,6 +64,21 @@ patternProperties:
>        - $ref: /schemas/pci/pci-pci-bridge.yaml#
>      unevaluatedProperties: false
>  
> +  "^pcie@[3],0$":
> +    description:
> +      child node describing the internal downstream port connected to the
> +      integrated Ethernet MAC endpoint of the tc9563 switch.
> +    type: object
> +    $ref: /schemas/pci/pci-pci-bridge.yaml#
> +    properties:
> +      n-fts: false
> +    patternProperties:
> +      "^ethernet@0,[0-1]$":
> +        type: object
> +        properties:
> +          n-fts: false

[Severity: Medium]
Does this ethernet node block need an additionalProperties: false constraint?

Without it, the schema defaults to allowing any undeclared properties. 
Could this allow toshiba,tx-amplitude-microvolt to still be placed in the
ethernet node without failing validation?

> +    unevaluatedProperties: false
> +

While reviewing the binding changes, I noticed a couple of pre-existing issues
in the related pci-pwrctrl-tc9563 driver code that might be worth looking into
since they relate to how these device tree nodes are parsed.

[Severity: High]
This isn't a bug introduced by this patch, but there is a potential
out-of-bounds array write in tc9563_pwrctrl_probe() when parsing multiple
Ethernet child nodes:

drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c:tc9563_pwrctrl_probe() {
    ...
    for_each_child_of_node_scoped(node, child) {
        ...
        port++;
        ret = tc9563_pwrctrl_parse_device_dt(tc9563, child1, port);
}

If the device tree contains more than one Ethernet child node under the
pcie@3,0 DSP3 port (which this YAML binding allows via the
^ethernet@0,[0-1]$ pattern), the inner loop can increment the port variable
to 5.

Could this corrupt memory? There are no bounds checks on port before it is
passed to tc9563_pwrctrl_parse_device_dt(), which uses it to index the
cfg[TC9563_MAX] array (size 5). Writing to cfg[5] would overwrite subsequent
struct fields.


[Severity: High]
This is a pre-existing issue, but the driver assigns PCI switch port
configurations based on child node iteration order rather than reg addresses.

drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c:tc9563_pwrctrl_probe() {
    ...
    for_each_child_of_node_scoped(node, child) {
        port++;
}

If a board's Device Tree legitimately omits an unused downstream port (e.g.,
leaving out pcie@1,0), the iteration shifts.

Will this mistakenly map the next port (like pcie@2,0) to TC9563_DSP1? It
seems this would desynchronize the driver's port layout from the physical
hardware, routing commands to the wrong switch ports and missing the DSP3
ethernet handling entirely.

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

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

[Manivannan Sadhasivam via B4 Relay]

From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>

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.

Hence, parse the Endpoint node under DSP3 instead of parsing both the
functions. The existing parsing logic also has one OOB issue as parsing
both functions will result in accessing past the tc9563_pwrctrl->cfg array.

Fixes: 4c9c7be47310 ("PCI: pwrctrl: Add power control driver for TC9563")
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
---
 drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c b/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
index 488e1ec34a7f..482e82e433dd 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;
-		/* 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);
+			if (eth) {
 				ret = tc9563_pwrctrl_parse_device_dt(tc9563,
-								child1, port);
+								eth, TC9563_ETHERNET);
 				if (ret)
 					break;
 			}

-- 
2.43.0

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

[sashiko-bot]

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

[PATCH 3/6] PCI/pwrctrl: tc9563: Power off only the external ports in tc9563_pwrctrl_disable_port()

[Manivannan Sadhasivam via B4 Relay]

From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>

TC9563 supports powering off only the external facing ports like DSP1 and
DSP2. It is not recommended to power off USP and DSP3 as they have fixed
ports/endpoint connected.

Hence, fix tc9563_pwrctrl_disable_port() to power off only DSP1 and DSP2.

Fixes: 4c9c7be47310 ("PCI: pwrctrl: Add power control driver for TC9563")
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
---
 drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c b/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
index 482e82e433dd..ca19bf60d290 100644
--- a/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
+++ b/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
@@ -241,12 +241,18 @@ static int tc9563_pwrctrl_disable_port(struct tc9563_pwrctrl *tc9563,
 	if (!cfg->disable_port)
 		return 0;
 
-	if (port == TC9563_DSP1) {
+	switch (port) {
+	case TC9563_DSP1:
 		seq = dsp1_pwroff_seq;
 		len = ARRAY_SIZE(dsp1_pwroff_seq);
-	} else {
+		break;
+	case TC9563_DSP2:
 		seq = dsp2_pwroff_seq;
 		len = ARRAY_SIZE(dsp2_pwroff_seq);
+		break;
+	default:
+		/* Only external downstream ports DSP1/DSP2 can be powered off */
+		return 0;
 	}
 
 	ret = tc9563_pwrctrl_i2c_bulk_write(tc9563->client, seq, len);

-- 
2.43.0

Re: [PATCH 3/6] PCI/pwrctrl: tc9563: Power off only the external ports in tc9563_pwrctrl_disable_port()

[sashiko-bot]

> From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> 
> TC9563 supports powering off only the external facing ports like DSP1 and
> DSP2. It is not recommended to power off USP and DSP3 as they have fixed
> ports/endpoint connected.
> 
> Hence, fix tc9563_pwrctrl_disable_port() to power off only DSP1 and DSP2.
> 
> Fixes: 4c9c7be47310 ("PCI: pwrctrl: Add power control driver for TC9563")
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>

Sashiko has reviewed this patch and found no issues. It looks great!

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

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

[Manivannan Sadhasivam via B4 Relay]

From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>

DSP3 doesn't support tuning Tx amplitude and DFE settings. So just skip
the setting if DT has passed the tuning properties. There is no need to
error out the whole driver because of it.

Fixes: 4c9c7be47310 ("PCI: pwrctrl: Add power control driver for TC9563")
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
---
 drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c b/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
index ca19bf60d290..28e9ec3e172a 100644
--- a/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
+++ b/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
@@ -308,6 +308,7 @@ static int tc9563_pwrctrl_set_l0s_l1_entry_delay(struct tc9563_pwrctrl *tc9563,
 static int tc9563_pwrctrl_set_tx_amplitude(struct tc9563_pwrctrl *tc9563,
 					   enum tc9563_pwrctrl_ports port)
 {
+	struct device *dev = tc9563->pwrctrl.dev;
 	u32 amp = tc9563->cfg[port].tx_amp;
 	int port_access;
 
@@ -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;
 	}
@@ -345,6 +349,7 @@ static int tc9563_pwrctrl_disable_dfe(struct tc9563_pwrctrl *tc9563,
 				      enum tc9563_pwrctrl_ports port)
 {
 	struct tc9563_pwrctrl_cfg *cfg = &tc9563->cfg[port];
+	struct device *dev = tc9563->pwrctrl.dev;
 	int port_access, lane_access = 0x3;
 	u32 phy_rate = 0x21;
 
@@ -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;
 	}

-- 
2.43.0

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

[sashiko-bot]

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

[PATCH 5/6] PCI/pwrctrl: tc9563: Rename DSP3 to VDSP

[Manivannan Sadhasivam via B4 Relay]

From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>

TC9563 Reference Manual calls DSP3 as VDSP (Virtual Downstream Port). Hence
name it as such to avoid ambiguity.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
---
 drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c b/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
index 28e9ec3e172a..f1179737f221 100644
--- a/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
+++ b/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
@@ -80,7 +80,7 @@ enum tc9563_pwrctrl_ports {
 	TC9563_USP,
 	TC9563_DSP1,
 	TC9563_DSP2,
-	TC9563_DSP3,
+	TC9563_VDSP,
 	TC9563_ETHERNET,
 	TC9563_MAX
 };
@@ -328,8 +328,8 @@ 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");
+	case TC9563_VDSP:
+		dev_dbg(dev, "Tx amplitude tuning not supported for VDSP\n");
 		return 0;
 	default:
 		return -EINVAL;
@@ -368,8 +368,8 @@ 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");
+	case TC9563_VDSP:
+		dev_dbg(dev, "DFE tuning not supported for VDSP\n");
 		return 0;
 	default:
 		return -EINVAL;
@@ -612,10 +612,10 @@ static int tc9563_pwrctrl_probe(struct platform_device *pdev)
 			break;
 
 		/*
-		 * The integrated Ethernet MAC Endpoint under DSP3 is a single
+		 * The integrated Ethernet MAC Endpoint under VDSP is a single
 		 * device whose functions share the same config registers.
 		 */
-		if (port == TC9563_DSP3) {
+		if (port == TC9563_VDSP) {
 			struct device_node *eth __free(device_node) =
 					of_get_next_available_child(child, NULL);
 			if (eth) {

-- 
2.43.0

Re: [PATCH 5/6] PCI/pwrctrl: tc9563: Rename DSP3 to VDSP

[sashiko-bot]

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

Pre-existing issues:
- [High] Unbalanced regulator_bulk_disable in probe error path and remove function.
--

--- Patch [5]: [PATCH 5/6] PCI/pwrctrl: tc9563: Rename DSP3 to VDSP ---
commit c88ec5f207551e44e4f9a4f5673e22f3dde64e6e
Author: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>

PCI/pwrctrl: tc9563: Rename DSP3 to VDSP

TC9563 Reference Manual calls DSP3 as VDSP (Virtual Downstream Port). Hence
name it as such to avoid ambiguity.

> diff --git a/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c b/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
> index 28e9ec3e172ae..f1179737f221f 100644
> --- a/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
> +++ b/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
[ ... ]
> @@ -612,10 +612,10 @@ static int tc9563_pwrctrl_probe(struct platform_device *pdev)
>  			break;
>  
>  		/*
> -		 * The integrated Ethernet MAC Endpoint under DSP3 is a single
> +		 * The integrated Ethernet MAC Endpoint under VDSP is a single
>  		 * device whose functions share the same config registers.
>  		 */
> -		if (port == TC9563_DSP3) {
> +		if (port == TC9563_VDSP) {
>  			struct device_node *eth __free(device_node) =
>  					of_get_next_available_child(child, NULL);
>  			if (eth) {

[Severity: High]
This isn't a bug introduced by this patch, but does the probe error path cause
an unbalanced regulator disable?

In tc9563_pwrctrl_probe(), if devm_pci_pwrctrl_device_set_ready() fails, the
code jumps to power_off:

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

Since tc9563_pwrctrl_power_on() is meant to be called asynchronously by the
PCI subsystem and is never invoked during probe, wouldn't calling
tc9563_pwrctrl_power_off() here invoke regulator_bulk_disable() without a
preceding regulator_bulk_enable()?

This triggers a WARN_ON_ONCE in the regulator core, which crashes systems
with panic_on_warn enabled.

The same unconditional disable appears to occur in tc9563_pwrctrl_remove()
when unbinding the driver.

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

[PATCH 6/6] PCI/pwrctrl: tc9563: Move Integrated MAC Endpoint out of 'tc9563_pwrctrl_ports' enum

[Manivannan Sadhasivam via B4 Relay]

From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>

'tc9563_pwrctrl_ports' is supposed to list only the internal ports of the
switch. But it currently lists the integrated MAC Endpoint as well, which
is wrong.

Hence, move it to a separate 'ep_cfg' struct and also configure/parse the
ports and the MAC Endpoint separately.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
---
 drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c | 96 ++++++++++++++++++++------------
 1 file changed, 60 insertions(+), 36 deletions(-)

diff --git a/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c b/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
index f1179737f221..9d869483b6b3 100644
--- a/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
+++ b/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
@@ -81,7 +81,6 @@ enum tc9563_pwrctrl_ports {
 	TC9563_DSP1,
 	TC9563_DSP2,
 	TC9563_VDSP,
-	TC9563_ETHERNET,
 	TC9563_MAX
 };
 
@@ -109,6 +108,7 @@ struct tc9563_pwrctrl {
 	struct pci_pwrctrl pwrctrl;
 	struct regulator_bulk_data supplies[TC9563_PWRCTL_MAX_SUPPLY];
 	struct tc9563_pwrctrl_cfg cfg[TC9563_MAX];
+	struct tc9563_pwrctrl_cfg ep_cfg;
 	struct gpio_desc *reset_gpio;
 	struct i2c_adapter *adapter;
 	struct i2c_client *client;
@@ -263,11 +263,11 @@ static int tc9563_pwrctrl_disable_port(struct tc9563_pwrctrl *tc9563,
 					     ARRAY_SIZE(common_pwroff_seq));
 }
 
-static int tc9563_pwrctrl_set_l0s_l1_entry_delay(struct tc9563_pwrctrl *tc9563,
-						 enum tc9563_pwrctrl_ports port,
-						 bool is_l1, u32 ns)
+static int tc9563_pwrctrl_set_port_l0s_l1_entry_delay(struct tc9563_pwrctrl *tc9563,
+						      enum tc9563_pwrctrl_ports port,
+						      bool is_l1, u32 ns)
 {
-	u32 rd_val, units;
+	u32 units;
 	int ret;
 
 	if (ns < TC9563_L0S_L1_DELAY_UNIT_NS)
@@ -276,25 +276,6 @@ static int tc9563_pwrctrl_set_l0s_l1_entry_delay(struct tc9563_pwrctrl *tc9563,
 	/* convert to units of 256ns */
 	units = ns / TC9563_L0S_L1_DELAY_UNIT_NS;
 
-	if (port == TC9563_ETHERNET) {
-		ret = tc9563_pwrctrl_i2c_read(tc9563->client,
-					      TC9563_EMBEDDED_ETH_DELAY,
-					      &rd_val);
-		if (ret)
-			return ret;
-
-		if (is_l1)
-			rd_val = u32_replace_bits(rd_val, units,
-						  TC9563_ETH_L1_DELAY_MASK);
-		else
-			rd_val = u32_replace_bits(rd_val, units,
-						  TC9563_ETH_L0S_DELAY_MASK);
-
-		return tc9563_pwrctrl_i2c_write(tc9563->client,
-						TC9563_EMBEDDED_ETH_DELAY,
-						rd_val);
-	}
-
 	ret = tc9563_pwrctrl_i2c_write(tc9563->client, TC9563_PORT_SELECT,
 				       BIT(port));
 	if (ret)
@@ -305,6 +286,34 @@ static int tc9563_pwrctrl_set_l0s_l1_entry_delay(struct tc9563_pwrctrl *tc9563,
 			units);
 }
 
+static int tc9563_pwrctrl_set_eth_l0s_l1_entry_delay(struct tc9563_pwrctrl *tc9563,
+						     bool is_l1, u32 ns)
+{
+	u32 rd_val, units;
+	int ret;
+
+	if (ns < TC9563_L0S_L1_DELAY_UNIT_NS)
+		return 0;
+
+	/* convert to units of 256ns */
+	units = ns / TC9563_L0S_L1_DELAY_UNIT_NS;
+
+	ret = tc9563_pwrctrl_i2c_read(tc9563->client, TC9563_EMBEDDED_ETH_DELAY,
+				      &rd_val);
+	if (ret)
+		return ret;
+
+	if (is_l1)
+		rd_val = u32_replace_bits(rd_val, units,
+					  TC9563_ETH_L1_DELAY_MASK);
+	else
+		rd_val = u32_replace_bits(rd_val, units,
+					  TC9563_ETH_L0S_DELAY_MASK);
+
+	return tc9563_pwrctrl_i2c_write(tc9563->client, TC9563_EMBEDDED_ETH_DELAY,
+					rd_val);
+}
+
 static int tc9563_pwrctrl_set_tx_amplitude(struct tc9563_pwrctrl *tc9563,
 					   enum tc9563_pwrctrl_ports port)
 {
@@ -430,11 +439,9 @@ static int tc9563_pwrctrl_assert_deassert_reset(struct tc9563_pwrctrl *tc9563,
 	return tc9563_pwrctrl_i2c_write(tc9563->client, TC9563_RESET_GPIO, val);
 }
 
-static int tc9563_pwrctrl_parse_device_dt(struct tc9563_pwrctrl *tc9563,
-					  struct device_node *node,
-					  enum tc9563_pwrctrl_ports port)
+static int tc9563_pwrctrl_parse_device_dt(struct device_node *node,
+					  struct tc9563_pwrctrl_cfg *cfg)
 {
-	struct tc9563_pwrctrl_cfg *cfg = &tc9563->cfg[port];
 	int ret;
 
 	/* Disable port if the status of the port is disabled. */
@@ -505,13 +512,13 @@ static int tc9563_pwrctrl_power_on(struct pci_pwrctrl *pwrctrl)
 			goto power_off;
 		}
 
-		ret = tc9563_pwrctrl_set_l0s_l1_entry_delay(tc9563, i, false, cfg->l0s_delay);
+		ret = tc9563_pwrctrl_set_port_l0s_l1_entry_delay(tc9563, i, false, cfg->l0s_delay);
 		if (ret) {
 			dev_err(dev, "Setting L0s entry delay failed\n");
 			goto power_off;
 		}
 
-		ret = tc9563_pwrctrl_set_l0s_l1_entry_delay(tc9563, i, true, cfg->l1_delay);
+		ret = tc9563_pwrctrl_set_port_l0s_l1_entry_delay(tc9563, i, true, cfg->l1_delay);
 		if (ret) {
 			dev_err(dev, "Setting L1 entry delay failed\n");
 			goto power_off;
@@ -536,6 +543,21 @@ static int tc9563_pwrctrl_power_on(struct pci_pwrctrl *pwrctrl)
 		}
 	}
 
+	/* Configure the integrated Ethernet MAC endpoint */
+	ret = tc9563_pwrctrl_set_eth_l0s_l1_entry_delay(tc9563, false,
+							tc9563->ep_cfg.l0s_delay);
+	if (ret) {
+		dev_err(dev, "Setting Ethernet L0s entry delay failed\n");
+		goto power_off;
+	}
+
+	ret = tc9563_pwrctrl_set_eth_l0s_l1_entry_delay(tc9563, true,
+							tc9563->ep_cfg.l1_delay);
+	if (ret) {
+		dev_err(dev, "Setting Ethernet L1 entry delay failed\n");
+		goto power_off;
+	}
+
 	ret = tc9563_pwrctrl_assert_deassert_reset(tc9563, true);
 	if (!ret)
 		return 0;
@@ -593,8 +615,7 @@ static int tc9563_pwrctrl_probe(struct platform_device *pdev)
 
 	pci_pwrctrl_init(&tc9563->pwrctrl, dev);
 
-	port = TC9563_USP;
-	ret = tc9563_pwrctrl_parse_device_dt(tc9563, node, port);
+	ret = tc9563_pwrctrl_parse_device_dt(node, &tc9563->cfg[TC9563_USP]);
 	if (ret) {
 		dev_err(dev, "failed to parse device tree properties: %d\n", ret);
 		goto remove_i2c;
@@ -605,9 +626,12 @@ static int tc9563_pwrctrl_probe(struct platform_device *pdev)
 	 * The first node represents DSP1, the second node represents DSP2,
 	 * and so on.
 	 */
+	port = TC9563_USP;
 	for_each_child_of_node_scoped(node, child) {
-		port++;
-		ret = tc9563_pwrctrl_parse_device_dt(tc9563, child, port);
+		if (++port >= TC9563_MAX)
+			break;
+
+		ret = tc9563_pwrctrl_parse_device_dt(child, &tc9563->cfg[port]);
 		if (ret)
 			break;
 
@@ -619,8 +643,8 @@ static int tc9563_pwrctrl_probe(struct platform_device *pdev)
 			struct device_node *eth __free(device_node) =
 					of_get_next_available_child(child, NULL);
 			if (eth) {
-				ret = tc9563_pwrctrl_parse_device_dt(tc9563,
-								eth, TC9563_ETHERNET);
+				ret = tc9563_pwrctrl_parse_device_dt(eth,
+							&tc9563->ep_cfg);
 				if (ret)
 					break;
 			}

-- 
2.43.0

Re: [PATCH 6/6] PCI/pwrctrl: tc9563: Move Integrated MAC Endpoint out of 'tc9563_pwrctrl_ports' enum

[sashiko-bot]

> From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> 
> 'tc9563_pwrctrl_ports' is supposed to list only the internal ports of the
> switch. But it currently lists the integrated MAC Endpoint as well, which
> is wrong.
> 
> Hence, move it to a separate 'ep_cfg' struct and also configure/parse the
> ports and the MAC Endpoint separately.
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>

Sashiko has reviewed this patch and found no issues. It looks great!

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