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

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

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

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

[Markus Elfring]

> v2: Massaged changelog a bit

* How does this wording fit to the adjusted commit subject?

* Would it have been nicer to send this version as a direct reply
  to the previous update suggestion?


> @@ -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


>  	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;
> -	}
> +
> +	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");


Regards,
Markus