Re: [v2] ethernet: ti: eliminate a bit of duplicate code in gbe_probe()

[Markus Elfring]

>>> @@ -3651,22 +3651,18 @@ static int gbe_probe(struct netcp_device *netcp_device, struct device *dev,
>>>      if (ret)
>>>          return ret;
>>>
>>> -    interfaces = of_get_child_by_name(node, "interfaces");
>>> -    if (!interfaces)
>>> -        dev_err(dev, "could not find interfaces\n");
>>> -
>>
>> How do you think about to skip a bit of statements as a reaction for
>> such a null pointer?
>> https://elixir.bootlin.com/linux/v5.1-rc4/source/drivers/net/ethernet/ti/netcp_ethss.c#L3562
â
> Maybe you need to look at the implementation of for_each_child_of_node() and of_node_put().
> NULL check before those functions is not needed.

This information is reasonable in principle.

Was the reference counter incremented even if a null pointer was returned
by such a function call?


>>>      ret = netcp_txpipe_init(&gbe_dev->tx_pipe, netcp_device,
>>>                  gbe_dev->dma_chan_name, gbe_dev->tx_queue_id);
>>> -    if (ret) {
>>> -        of_node_put(interfaces);
>>> +    if (ret)
>>>          return ret;
>>> -    }
>>>
>>>      ret = netcp_txpipe_open(&gbe_dev->tx_pipe);
>>> -    if (ret) {
>>> -        of_node_put(interfaces);
>>> +    if (ret)
>>>          return ret;
>>> -    }

Does the preparation of the NetCP pipe still matter in this special use case?


>>> +
>>> +    interfaces = of_get_child_by_name(node, "interfaces");
>>> +    if (!interfaces)
>>> +        dev_err(dev, "could not find interfaces\n");
>>>
>>>      /* Create network interfaces */
>>>      INIT_LIST_HEAD(&gbe_dev->gbe_intf_head);
>>
>> Can code like the following trigger corresponding software development concerns?
>>
>> for_each_child_of_node(interfaces, interface) {
>> â
>> }
>> of_node_put(interfaces);
>>
â
>> if (!gbe_dev->num_slaves)
>> dev_warn(dev, "No network interface configured\n");

Is this message really required as another response then?

Regards,
Markus