Re: [PATCH] net: ti: icssg-prueth: Fix device_node refcount leak in prueth_probe()

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 1 (1) · Low: 0 (1)

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The SR1.0 sibling driver keeps the same device_node refcount
  leak, and this patch does not touch it.

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but the port-
  node loop in prueth_probe()…
- [Low] This problem wasn't introduced by this patch, but the DT binding
  and the driver disagree on whether "ti,mii-rt" is required.

--

net: ti: icssg-prueth: Fix device_node refcount leak in prueth_probe()

In icssg_prueth.c, prueth_probe() returned directly with -ENODEV when the
ti,mii-g-rt or ti,mii-rt syscon lookup failed. This patch changes both
returns to goto put_cores, so the eth0_node/eth1_node references are
dropped on those error paths.

> diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.c b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
> index 591be5c8056b4..9b971d1dfd8cf 100644
> --- a/drivers/net/ethernet/ti/icssg/icssg_prueth.c
> +++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
> @@ -2258,13 +2258,15 @@ static int prueth_probe(struct platform_device *pdev)
>  	prueth->miig_rt = syscon_regmap_lookup_by_phandle(np, "ti,mii-g-rt");
>  	if (IS_ERR(prueth->miig_rt)) {
>  		dev_err(dev, "couldn't get ti,mii-g-rt syscon regmap\n");
> -		return -ENODEV;
> +		ret = -ENODEV;
> +		goto put_cores;
>  	}

[Severity: Medium]
The subject says this fixes the device_node refcount leak in
prueth_probe(). Does the SR1.0 variant of the driver still have the same
leak?

drivers/net/ethernet/ti/icssg/icssg_prueth_sr1.c has its own
prueth_probe(). Its port loop takes the same of_node_get() references on
eth0_node/eth1_node, and it still returns directly here:

drivers/net/ethernet/ti/icssg/icssg_prueth_sr1.c:prueth_probe() {
    ...
	prueth->miig_rt = syscon_regmap_lookup_by_phandle(np, "ti,mii-g-rt");
	if (IS_ERR(prueth->miig_rt)) {
		dev_err(dev, "couldn't get ti,mii-g-rt syscon regmap\n");
		return -ENODEV;
	}

	prueth->mii_rt = syscon_regmap_lookup_by_phandle(np, "ti,mii-rt");
	if (IS_ERR(prueth->mii_rt)) {
		dev_err(dev, "couldn't get ti,mii-rt syscon regmap\n");
		return -ENODEV;
	}
    ...
}

That file already has a put_cores label that calls of_node_put() on both
nodes. prueth_put_cores() checks each pru/rtu/txpru pointer for NULL, and
at this point those pointers are still zero from devm_kzalloc(). So the
same "ret = -ENODEV; goto put_cores;" change looks like it would work
there too.

CONFIG_TI_ICSSG_PRUETH_SR1 builds the SR1 driver, and in-tree DTs use it,
for example k3-am65-iot2050-common-pg1.dtsi with compatible
"ti,am654-sr1-icssg-prueth".

Should the SR1 driver get the same fix, perhaps as a companion patch with
its own Fixes: tag?

>  
>  	prueth->mii_rt = syscon_regmap_lookup_by_phandle(np, "ti,mii-rt");
>  	if (IS_ERR(prueth->mii_rt)) {
>  		dev_err(dev, "couldn't get ti,mii-rt syscon regmap\n");
> -		return -ENODEV;
> +		ret = -ENODEV;
> +		goto put_cores;
>  	}

[Severity: Low]
This is a pre-existing issue and was not introduced by this patch. The
driver treats ti,mii-rt as mandatory here, but the DT binding does not.

In Documentation/devicetree/bindings/net/ti,icssg-prueth.yaml, ti,mii-rt
is only listed under properties. The top-level required list has
ti,mii-g-rt but not ti,mii-rt:

required:
  - compatible
  - sram
  - dmas
  - dma-names
  - ethernet-ports
  - ti,mii-g-rt
  - interrupts
  - interrupt-names

The binding's example node sets ti,mii-g-rt, ti,pa-stats and ti,iep, but
not ti,mii-rt. A DT written from that example would pass schema
validation and then fail here with "couldn't get ti,mii-rt syscon
regmap".

All in-tree users (k3-am642-*, k3-am654-*, k3-am65-iot2050-common.dtsi)
provide ti,mii-rt, so in-tree boards are not affected.

Should ti,mii-rt be added to the binding's required list and to its
example?

[Severity: Medium]
This is a pre-existing issue and was not introduced by this patch. While
looking at the eth0_node/eth1_node references, the port loop earlier in
prueth_probe() also seems to have a few related problems:

drivers/net/ethernet/ti/icssg/icssg_prueth.c:prueth_probe() {
    ...
	for_each_child_of_node(eth_ports_node, eth_node) {
		u32 reg;

		if (strcmp(eth_node->name, "port"))
			continue;
		ret = of_property_read_u32(eth_node, "reg", &reg);
		if (ret < 0) {
			dev_err(dev, "%pOF error reading port_id %d\n",
				eth_node, ret);
		}

		of_node_get(eth_node);

		if (reg == 0) {
			eth0_node = eth_node;
			...
		} else if (reg == 1) {
			eth1_node = eth_node;
			...
		} else {
			dev_err(dev, "port reg should be 0 or 1\n");
		}
	}
    ...
}

When of_property_read_u32() fails, the code only logs the error and
keeps going. Can the reg == 0 and reg == 1 checks then read an
uninitialized reg?

In the else branch, is the reference taken by of_node_get(eth_node) ever
dropped?

If two available port nodes both have reg = <0> (or both have <1>), the
second assignment to eth0_node (or eth1_node) overwrites the first. Does
the first node's reference leak in that case?

Related to that, can this check ever fire?

	if (eth0_node == eth1_node) {
		dev_err(dev, "port0 and port1 can't have same reg\n");
		of_node_put(eth0_node);
		return -ENODEV;
	}

Each iteration stores a different node into at most one slot. The two
pointers can only be equal when both are NULL, and the earlier
!eth0_node && !eth1_node check already returns in that case. So a DT with
duplicate reg values would probe successfully instead of failing.

The SR1 copy of this loop in icssg_prueth_sr1.c has the same pattern.

>  
>  	prueth->pa_stats = syscon_regmap_lookup_by_phandle(np, "ti,pa-stats");

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261030085651.1944-1-laixiangwu%40cmss.chinamobile.com