Re: [dpdk-dev] [PATCH v3 02/15] ethdev: add linkstatus get/set helper functions

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 1/9/2018 4:29 PM, Stephen Hemminger wrote:
> On Tue, 9 Jan 2018 13:30:46 +0300
> Andrew Rybchenko <arybchenko@solarflare.com> wrote:
> 
>> On 01/08/2018 08:45 PM, Stephen Hemminger wrote:
>>> Many drivers are all doing copy/paste of the same code to atomically
>>> update the link status. Reduce duplication, and allow for future
>>> changes by having common function for this.
>>>
>>> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>

<...>

>>> +int
>>> +_rte_eth_linkstatus_set(struct rte_eth_dev *dev,
>>> +		       const struct rte_eth_link *new_link)

I see "_rte" prefix is used to say this is internal only but this is not the
syntax for other internal APIs, only there is one using this syntax, so may be
confusing to have a mix of them.

"@internal" marker in function comment already saying that this is internal.
Also I send a patch to separate public APIs and PMD APIs to different files,
hopefully that will also help clarifying target of the APIs.

And although these two new APIs are internal, they need to be in .map file for
shared library case.

>>> +{
>>> +	volatile uint64_t *dev_link
>>> +		 = (volatile uint64_t *)&(dev->data->dev_link);
>>> +	union {
>>> +		uint64_t val64;
>>> +		struct rte_eth_link link;
>>> +	} orig;
>>> +
>>> +	RTE_BUILD_BUG_ON(sizeof(*new_link) != sizeof(uint64_t));
>>> +
>>> +	orig.val64 = rte_atomic64_exchange(dev_link,
>>> +					   *(const uint64_t *)new_link);
>>> +
>>> +	return (orig.link.link_status == new_link->link_status) ? -1 : 0;  
>>
>> It still contradicts to: -1 if link state has changed, 0 if the same.

+1, function documentation in header file and this conflicts, one needs fixing.

>> BTW, it looks like the return value of the link_update callback is not
>> specified (described) and not used. It explains why different drivers
>> behave differently and nobody notices it.
> 
> It looks like link_status callback could be void.
> The only places that use return value from set are code that wants
> to log something if status changed.

When a set function returns -1/0, it looks like it returns set status. Returning
if link status changes seems odd to me. What about returning original link
struct, so PMD may get this information if they want.