[dpdk-dev] [PATCH v5 00/15] common ethdev linkstatus functions

Ferruh Yigit ferruh.yigit at intel.com
Wed Jan 17 15:32:17 CET 2018


On 1/17/2018 7:56 AM, Andrew Rybchenko wrote:
> On 01/16/2018 09:37 PM, Stephen Hemminger wrote:
>> While reviewing drivers, noticed a lot of unnecessary
>> duplication of code in drivers for handling the eth_dev link status
>> information. While consolidating this, it also became obvious that
>> some drivers behave differently for no good reason.
>>
>> It also was a good chance to introduce atomic exchange primitives
>> in EAL because there are other places using cmpset where not
>> necessary (such as bonding).
>>
>> Mostly only compile tested only, don't have all of the hardware
>> available (except ixgbe and virtio) to test.
>>
>> Note: the eth_dev_link_update function return value is inconsistent
>> across drivers. Should be changed to be void.
> 
> I would say "link_update" callback return value is inconsistent across
> drivers. I'm not sure which direction is right here: make it consistent
> or make it void. Also any changes in link information could be
> important. As I understand it should not happen without up/down,
> but bugs with loss of intermediate transitions are definitely possible.
> So, notifying about any changes in link information is definitely safer.
> May be not now.

Again, why not return previous link status, it is simple enough to prevent
inconsistent usage.

rte_eth_link_get() already discards the return value, so won't be a problem there.

For the cases PMD would like know about link changes, they will need to
implement almost same link_update function with a return value, so why not use
existing link_update function?

Like been in virtio, link_update() used in interrupt handler, and calls a
callback process if status changes. When link_update() return status changed to
void, I guess they will need to implement another version of the link_update
with return and use it.

> 
>>
>> v5
>>   - checkpatch whitespace cleanup
>>
>> v4
>>   - incorporate review feedback
>>   - rename _rte_linkstatus to rte_linkstatus
>>   - change return value of _rte_linkstatus
>>   - optimize get on 64bit platforms
>>   - change return value of rte_linkstatus_set
>>
>> v3
>>   - align rte_linkstatus_get with rte_atomic64_read
>>   - virtio use ETH_SPEED_NUM_10G
>>   - add net/
>>
>> v2
>>   - function names changed
>>   - rebased to current master
>>
>> Stephen Hemminger (15):
>>    eal: introduce atomic exchange operation
>>    ethdev: add linkstatus get/set helper functions
>>    net/virtio: use eth_linkstatus_set
>>    net/vmxnet3: use rte_eth_linkstatus_set
>>    net/dpaa2: use rte_eth_linkstatus_set
>>    net/nfp: use rte_eth_linkstatus functions
>>    net/e1000: use rte_eth_linkstatus helpers
>>    net/ixgbe: use rte_eth_linkstatus functions
>>    net/sfc: use new rte_eth_linkstatus functions
>>    net/i40e: use rte_eth_linkstatus functions
>>    net/liquidio: use rte_eth_linkstatus_set
>>    net/thunderx: use rte_eth_linkstatus_set
>>    net/szedata: use _rte_eth_linkstatus_set
>>    net/octeontx: use rte_eth_linkstatus_set
>>    net/enic: use rte_eth_linkstatus_set
>>
>>   drivers/net/dpaa2/dpaa2_ethdev.c                   | 75 ++---------------
>>   drivers/net/e1000/em_ethdev.c                      | 69 ++--------------
>>   drivers/net/e1000/igb_ethdev.c                     | 70 ++--------------
>>   drivers/net/enic/enic_ethdev.c                     |  5 +-
>>   drivers/net/enic/enic_main.c                       | 17 ++--
>>   drivers/net/i40e/i40e_ethdev.c                     | 43 ++--------
>>   drivers/net/i40e/i40e_ethdev_vf.c                  | 18 +---
>>   drivers/net/ixgbe/ixgbe_ethdev.c                   | 96 ++++------------------
>>   drivers/net/liquidio/lio_ethdev.c                  | 53 ++----------
>>   drivers/net/nfp/nfp_net.c                          | 77 ++---------------
>>   drivers/net/octeontx/octeontx_ethdev.c             | 17 +---
>>   drivers/net/sfc/sfc_ethdev.c                       | 21 +----
>>   drivers/net/sfc/sfc_ev.c                           | 20 +----
>>   drivers/net/szedata2/rte_eth_szedata2.c            | 19 ++---
>>   drivers/net/thunderx/nicvf_ethdev.c                | 18 +---
>>   drivers/net/virtio/virtio_ethdev.c                 | 65 +++------------
>>   drivers/net/vmxnet3/vmxnet3_ethdev.c               | 86 ++++---------------
>>   .../common/include/arch/x86/rte_atomic.h           | 24 ++++++
>>   .../common/include/arch/x86/rte_atomic_32.h        | 12 +++
>>   .../common/include/arch/x86/rte_atomic_64.h        | 12 +++
>>   lib/librte_eal/common/include/generic/rte_atomic.h | 78 ++++++++++++++++++
>>   lib/librte_ether/rte_ethdev.c                      | 22 +----
>>   lib/librte_ether/rte_ethdev.h                      | 62 ++++++++++++++
>>   23 files changed, 302 insertions(+), 677 deletions(-)
>>
> 



More information about the dev mailing list