Message-ID: <20250416091632.GM395307@horms.kernel.org>
Date: Wed, 16 Apr 2025 10:16:32 +0100
From: Simon Horman <horms@...nel.org>
To: Parvathi Pudi <parvathi@...thit.com>
Cc: danishanwar@...com, rogerq@...nel.org, andrew+netdev@...n.ch,
	davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
	pabeni@...hat.com, robh@...nel.org, krzk+dt@...nel.org,
	conor+dt@...nel.org, nm@...com, ssantosh@...nel.org,
	tony@...mide.com, richardcochran@...il.com, glaroque@...libre.com,
	schnelle@...ux.ibm.com, m-karicheri2@...com, s.hauer@...gutronix.de,
	rdunlap@...radead.org, diogo.ivo@...mens.com, basharath@...thit.com,
	jacob.e.keller@...el.com, m-malladi@...com,
	javier.carrasco.cruz@...il.com, afd@...com, s-anna@...com,
	linux-arm-kernel@...ts.infradead.org, netdev@...r.kernel.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
	pratheesh@...com, prajith@...com, vigneshr@...com, praneeth@...com,
	srk@...com, rogerq@...com, krishna@...thit.com, pmohan@...thit.com,
	mohan@...thit.com
Subject: Re: [PATCH net-next v5 02/11] net: ti: prueth: Adds ICSSM Ethernet
 driver

On Mon, Apr 14, 2025 at 05:04:49PM +0530, Parvathi Pudi wrote:
> From: Roger Quadros <rogerq@...com>
> 
> Updates Kernel configuration to enable PRUETH driver and its dependencies
> along with makefile changes to add the new PRUETH driver.
> 
> Changes includes init and deinit of ICSSM PRU Ethernet driver including
> net dev registration and firmware loading for DUAL-MAC mode running on
> PRU-ICSS2 instance.
> 
> Changes also includes link handling, PRU booting, default firmware loading
> and PRU stopping using existing remoteproc driver APIs.
> 
> Signed-off-by: Roger Quadros <rogerq@...com>
> Signed-off-by: Andrew F. Davis <afd@...com>
> Signed-off-by: Basharath Hussain Khaja <basharath@...thit.com>
> Signed-off-by: Parvathi Pudi <parvathi@...thit.com>

...

> diff --git a/drivers/net/ethernet/ti/icssm/icssm_prueth.c b/drivers/net/ethernet/ti/icssm/icssm_prueth.c

...

> +static int icssm_prueth_probe(struct platform_device *pdev)
> +{
> +	struct device_node *eth0_node = NULL, *eth1_node = NULL;
> +	struct device_node *eth_node, *eth_ports_node;
> +	enum pruss_pru_id pruss_id0, pruss_id1;
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np;
> +	struct prueth *prueth;
> +	int i, ret;
> +
> +	np = dev->of_node;
> +	if (!np)
> +		return -ENODEV; /* we don't support non DT */
> +
> +	prueth = devm_kzalloc(dev, sizeof(*prueth), GFP_KERNEL);
> +	if (!prueth)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, prueth);
> +	prueth->dev = dev;
> +	prueth->fw_data = device_get_match_data(dev);
> +
> +	eth_ports_node = of_get_child_by_name(np, "ethernet-ports");
> +	if (!eth_ports_node)
> +		return -ENOENT;
> +
> +	for_each_child_of_node(eth_ports_node, eth_node) {
> +		u32 reg;
> +
> +		if (strcmp(eth_node->name, "ethernet-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);

Hi Roger, Parvathi, all,

I feel that I'm missing something obvious here.
But I have some questions about the reference to eth_node
taken on the line above.

> +
> +		if (reg == 0) {
> +			eth0_node = eth_node;

If, while iterating through the for loop above, we reach this point more
than once, then will the reference to the previously node assigned to
eth0_node be leaked?

> +			if (!of_device_is_available(eth0_node)) {
> +				of_node_put(eth0_node);
> +				eth0_node = NULL;
> +			}
> +		} else if (reg == 1) {
> +			eth1_node = eth_node;

Likewise here for eth1_node.

> +			if (!of_device_is_available(eth1_node)) {
> +				of_node_put(eth1_node);
> +				eth1_node = NULL;
> +			}
> +		} else {
> +			dev_err(dev, "port reg should be 0 or 1\n");

And, perhaps more to the point, is the reference to eth_node leaked if
we reach this line?

> +		}
> +	}
> +
> +	of_node_put(eth_ports_node);
> +
> +	/* At least one node must be present and available else we fail */
> +	if (!eth0_node && !eth1_node) {
> +		dev_err(dev, "neither port0 nor port1 node available\n");
> +		return -ENODEV;
> +	}
> +
> +	if (eth0_node == eth1_node) {

Given the if / else if condition in the for loop above,
I'm not sure this can ever occur.

> +		dev_err(dev, "port0 and port1 can't have same reg\n");
> +		of_node_put(eth0_node);
> +		return -ENODEV;
> +	}
> +
> +	prueth->eth_node[PRUETH_MAC0] = eth0_node;
> +	prueth->eth_node[PRUETH_MAC1] = eth1_node;
> +
> +	if (eth0_node) {
> +		prueth->pru0 = pru_rproc_get(np, 0, &pruss_id0);
> +		if (IS_ERR(prueth->pru0)) {
> +			ret = PTR_ERR(prueth->pru0);
> +			if (ret != -EPROBE_DEFER)
> +				dev_err(dev, "unable to get PRU0: %d\n", ret);
> +			goto put_pru;
> +		}
> +	}
> +
> +	if (eth1_node) {
> +		prueth->pru1 = pru_rproc_get(np, 1, &pruss_id1);
> +		if (IS_ERR(prueth->pru1)) {
> +			ret = PTR_ERR(prueth->pru1);
> +			if (ret != -EPROBE_DEFER)
> +				dev_err(dev, "unable to get PRU1: %d\n", ret);
> +			goto put_pru;
> +		}
> +	}
> +
> +	/* setup netdev interfaces */
> +	if (eth0_node) {
> +		ret = icssm_prueth_netdev_init(prueth, eth0_node);
> +		if (ret) {
> +			if (ret != -EPROBE_DEFER) {
> +				dev_err(dev, "netdev init %s failed: %d\n",
> +					eth0_node->name, ret);
> +			}
> +			goto put_pru;
> +		}
> +	}
> +
> +	if (eth1_node) {
> +		ret = icssm_prueth_netdev_init(prueth, eth1_node);
> +		if (ret) {
> +			if (ret != -EPROBE_DEFER) {
> +				dev_err(dev, "netdev init %s failed: %d\n",
> +					eth1_node->name, ret);
> +			}
> +			goto netdev_exit;
> +		}
> +	}
> +
> +	/* register the network devices */
> +	if (eth0_node) {
> +		ret = register_netdev(prueth->emac[PRUETH_MAC0]->ndev);
> +		if (ret) {
> +			dev_err(dev, "can't register netdev for port MII0");
> +			goto netdev_exit;
> +		}
> +
> +		prueth->registered_netdevs[PRUETH_MAC0] =
> +			prueth->emac[PRUETH_MAC0]->ndev;
> +	}
> +
> +	if (eth1_node) {
> +		ret = register_netdev(prueth->emac[PRUETH_MAC1]->ndev);
> +		if (ret) {
> +			dev_err(dev, "can't register netdev for port MII1");
> +			goto netdev_unregister;
> +		}
> +
> +		prueth->registered_netdevs[PRUETH_MAC1] =
> +			prueth->emac[PRUETH_MAC1]->ndev;
> +	}
> +
> +	if (eth1_node)
> +		of_node_put(eth1_node);
> +	if (eth0_node)
> +		of_node_put(eth0_node);
> +	return 0;
> +
> +netdev_unregister:
> +	for (i = 0; i < PRUETH_NUM_MACS; i++) {
> +		if (!prueth->registered_netdevs[i])
> +			continue;
> +		unregister_netdev(prueth->registered_netdevs[i]);
> +	}
> +
> +netdev_exit:
> +	for (i = 0; i < PRUETH_NUM_MACS; i++) {
> +		eth_node = prueth->eth_node[i];
> +		if (!eth_node)
> +			continue;
> +
> +		icssm_prueth_netdev_exit(prueth, eth_node);
> +	}
> +
> +put_pru:
> +	if (eth1_node) {
> +		if (prueth->pru1)
> +			pru_rproc_put(prueth->pru1);
> +		of_node_put(eth1_node);
> +	}
> +
> +	if (eth0_node) {
> +		if (prueth->pru0)
> +			pru_rproc_put(prueth->pru0);
> +		of_node_put(eth0_node);
> +	}
> +
> +	return ret;
> +}
> +
> +static void icssm_prueth_remove(struct platform_device *pdev)
> +{
> +	struct prueth *prueth = platform_get_drvdata(pdev);
> +	struct device_node *eth_node;
> +	int i;
> +
> +	for (i = 0; i < PRUETH_NUM_MACS; i++) {
> +		if (!prueth->registered_netdevs[i])
> +			continue;
> +		unregister_netdev(prueth->registered_netdevs[i]);
> +	}
> +
> +	for (i = 0; i < PRUETH_NUM_MACS; i++) {
> +		eth_node = prueth->eth_node[i];
> +		if (!eth_node)
> +			continue;
> +
> +		icssm_prueth_netdev_exit(prueth, eth_node);
> +		of_node_put(eth_node);
> +	}
> +
> +	pruss_put(prueth->pruss);
> +
> +	if (prueth->eth_node[PRUETH_MAC0])
> +		pru_rproc_put(prueth->pru0);
> +	if (prueth->eth_node[PRUETH_MAC1])
> +		pru_rproc_put(prueth->pru1);
> +}

...

Powered by blists - more mailing lists