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?
>
> The situation you assume is an issue that the of_get_child_by_name() function needs to consider
>  and has been irrelevant to our patch.

I suggest to reconsider the software situation a bit more.


> 1, when returning NULL, the of_get_child_by_name () function needs to ensure that the resources
>  it has allocated are released;
> 2, when returning NULL, if of_get_child_by_name() can't release its resources, then the
>  outer function has no way to release these resources.
>
> If you are interested, you can check the of_get_child_by_name() function further

It seems that the corresponding software documentation can be improved also here.
https://elixir.bootlin.com/linux/v5.1-rc4/source/drivers/of/base.c#L863


> and send it to me if you find any problems.

I find the exception handling suspicious in the discussed function implementation.


>>>>>      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?
â
> Please refer to my reply above.

I propose to take additional software design possibilities into account.


> We have checked the netcp_txpipe_init() and the netcp_txpipe_open() function.

I wonder if such function calls are still relevant if a questionable system
configuration would be detected before.


> However, your questions may not actually be related to our patch.

Your update suggestion triggered related adjustment ideas.



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

Is the exception handling still questionable in this function?

Regards,
Markus