Re: [PATCH net-next v5 02/11] net: ti: prueth: Adds ICSSM Ethernet driver

Parvathi Pudi
2025-04-18T12:23:33Z

Hi,

> On Mon, Apr 14, 2025 at 05:04:49PM +0530, Parvathi Pudi wrote:
>> From: Roger Quadros <rogerq@ti.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@ti.com>
>> Signed-off-by: Andrew F. Davis <afd@ti.com>
>> Signed-off-by: Basharath Hussain Khaja <basharath@couthit.com>
>> Signed-off-by: Parvathi Pudi <parvathi@couthit.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?
>

We will modify the condition as below to avoid leaks
if ((reg == 0) && (eth0_node == NULL))

>> + 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.
>

We will modify this also as below
if ((reg == 1) && (eth1_node == NULL))

>> + 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?
>

We will check and add of_node_put(eth_node) at the end of the for loop.

>> + }
>> + }
>> +
>> + 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.
>

We will remove this.

Thanks and Regards,
Parvathi.