common ethdev linkstatus functions
Subsystems (2)
v3 cover common ethdev linkstatus functions
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.
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 | 65 ++------------ drivers/net/e1000/em_ethdev.c | 70 ++------------- drivers/net/e1000/igb_ethdev.c | 71 ++-------------- drivers/net/enic/enic_ethdev.c | 5 +- drivers/net/enic/enic_main.c | 16 ++-- drivers/net/i40e/i40e_ethdev.c | 44 ++-------- drivers/net/i40e/i40e_ethdev_vf.c | 19 +---- drivers/net/ixgbe/ixgbe_ethdev.c | 99 ++++------------------ drivers/net/liquidio/lio_ethdev.c | 54 ++---------- drivers/net/nfp/nfp_net.c | 75 ++-------------- drivers/net/octeontx/octeontx_ethdev.c | 16 +--- drivers/net/sfc/sfc_ethdev.c | 27 ++---- drivers/net/sfc/sfc_ev.c | 23 +---- drivers/net/szedata2/rte_eth_szedata2.c | 20 ++--- drivers/net/thunderx/nicvf_ethdev.c | 17 +--- drivers/net/virtio/virtio_ethdev.c | 64 +++----------- drivers/net/vmxnet3/vmxnet3_ethdev.c | 85 ++++--------------- .../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 | 36 ++++++++ lib/librte_ether/rte_ethdev.h | 28 ++++++ 23 files changed, 303 insertions(+), 657 deletions(-)
Signature · 1 line
2.15.1
PATCH v3 1/15 eal: introduce atomic exchange operation
To handle atomic update of link status (64 bit), every driver was doing its own version using cmpset.
Atomic exchange is a useful primitive in its own right; therefore make it a EAL routine.
Signed-off-by Stephen Hemminger <[email protected]>
--- .../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 ++++++++++++++++++++++ 4 files changed, 126 insertions(+)
lib/librte_eal/common/include/arch/x86/rte_atomic.h +24 -0
@@ -59,6 +59,18 @@ rte_atomic16_cmpset(volatile uint16_t *dst, uint16_t exp, uint16_t src)return res; } +static inline uint16_t +rte_atomic16_exchange (volatile uint16_t *dst,uint16_t val) +{ + asmvolatile ( +MPLOCKED +"xchgw %0, %1;" + :"=r" (val),"=m" (*dst) + :"0" (val),"m" (*dst) + :"memory" );/* no-clobber list */ +return val; +} +static inline int rte_atomic16_test_and_set (rte_atomic16_t *v) {return rte_atomic16_cmpset ((volatile uint16_t *)&v->cnt,0 ,1 );
@@ -133,6 +145,18 @@ rte_atomic32_cmpset(volatile uint32_t *dst, uint32_t exp, uint32_t src)return res; } +static inline uint32_t +rte_atomic32_exchange (volatile uint32_t *dst,uint32_t val) +{ + asmvolatile ( +MPLOCKED +"xchgl %0, %1;" + :"=r" (val),"=m" (*dst) + :"0" (val),"m" (*dst) + :"memory" );/* no-clobber list */ +return val; +} +static inline int rte_atomic32_test_and_set (rte_atomic32_t *v) {return rte_atomic32_cmpset ((volatile uint32_t *)&v->cnt,0 ,1 );
lib/librte_eal/common/include/arch/x86/rte_atomic_32.h +12 -0
@@ -98,6 +98,18 @@ rte_atomic64_cmpset(volatile uint64_t *dst, uint64_t exp, uint64_t src)return res; } +static inline uint64_t +rte_atomic64_exchange (volatile uint64_t *dest,uint64_t val) +{ +uint64_t old; + +do { + old = *dest; + }while (rte_atomic64_t_cmpset (dest, old, val)); + +return old; +} +static inline void rte_atomic64_init (rte_atomic64_t *v) {
lib/librte_eal/common/include/arch/x86/rte_atomic_64.h +12 -0
@@ -71,6 +71,18 @@ rte_atomic64_cmpset(volatile uint64_t *dst, uint64_t exp, uint64_t src)return res; } +static inline uint64_t +rte_atomic64_exchange (volatile uint64_t *dst,uint64_t val) +{ + asmvolatile ( +MPLOCKED +"xchgq %0, %1;" + :"=r" (val),"=m" (*dst) + :"0" (val),"m" (*dst) + :"memory" );/* no-clobber list */ +return val; +} +static inline void rte_atomic64_init (rte_atomic64_t *v) {
lib/librte_eal/common/include/generic/rte_atomic.h +78 -0
96 lines not on this page · raw message
Signature · 1 line
2.15.1
Re: [PATCH v3 01/15] eal: introduce atomic exchange operation
Quoted 6 lines · Stephen Hemminger
To handle atomic update of link status (64 bit), every driver was doing its own version using cmpset.
Atomic exchange is a useful primitive in its own right; therefore make it a EAL routine.Signed-off-by Stephen Hemminger <[email protected]>
<...>
lib/librte_eal/common/include/arch/x86/rte_atomic_32.h rte_atomic64_cmpset(volatile uint64_t *dst, uint64_t exp, uint64_t src) · line 108
Quoted 12 lines
lib/librte_eal/common/include/arch/x86/rte_atomic_32.h +8
-0@@ -98,6 +98,18 @@ rte_atomic64_cmpset(volatile uint64_t *dst, uint64_t exp, uint64_t src)return res; } +static inline uint64_t +rte_atomic64_exchange (volatile uint64_t *dest,uint64_t val) +{ +uint64_t old; + +do { + old = *dest; + }while (rte_atomic64_t_cmpset (dest, old, val));
rte_atomic64_cmpset ? (without _t)
Quoted 3 lines
lib/librte_eal/common/include/arch/x86/rte_atomic_32.h +3
-0+ +return old; +}
<...>
Re: [PATCH v3 01/15] eal: introduce atomic exchange operation
Quoted 25 lines · On Thu, 11 Jan 2018 17:01:10 +0000 Ferruh Yigit <[email protected]> wrote:
Quoted 6 lines · Stephen Hemminger
To handle atomic update of link status (64 bit), every driver was doing its own version using cmpset.
Atomic exchange is a useful primitive in its own right; therefore make it a EAL routine.Signed-off-by Stephen Hemminger <[email protected]><...>
lib/librte_eal/common/include/arch/x86/rte_atomic_32.h rte_atomic64_cmpset(volatile uint64_t *dst, uint64_t exp, uint64_t src) · line 108
Quoted 12 lines
lib/librte_eal/common/include/arch/x86/rte_atomic_32.h +8
-0@@ -98,6 +98,18 @@ rte_atomic64_cmpset(volatile uint64_t *dst, uint64_t exp, uint64_t src)return res; } +static inline uint64_t +rte_atomic64_exchange (volatile uint64_t *dest,uint64_t val) +{ +uint64_t old; + +do { + old = *dest; + }while (rte_atomic64_t_cmpset (dest, old, val));
rte_atomic64_cmpset? (without _t)
Quoted 6 lines
Quoted 3 lines
lib/librte_eal/common/include/arch/x86/rte_atomic_32.h +3
-0+ +return old; +}<...>
Addressed in followup patch, needed to build on 32bit.
Will fold into next version.
Re: [PATCH v3 01/15] eal: introduce atomic exchange operation
In reply to Ferruh YigitQuoted 14 lines · On Thu, 11 Jan 2018 17:01:10 +0000 Ferruh Yigit <[email protected]> wrote:
Quoted 6 lines · Stephen Hemminger
To handle atomic update of link status (64 bit), every driver was doing its own version using cmpset.
Atomic exchange is a useful primitive in its own right; therefore make it a EAL routine.Signed-off-by Stephen Hemminger <[email protected]><...>
Quoted 4 lines
lib/librte_eal/common/include/arch/x86/rte_atomic.h +0
-0@@ -98,6 +98,18 @@ rte_atomic64_cmpset(volatile uint64_t *dst, uint64_t exp, uint64_t src)return res; }
Quoted 10 lines
lib/librte_eal/common/include/arch/x86/rte_atomic_32.h rte_atomic64_cmpset(volatile uint64_t *dst, uint64_t exp, uint64_t src) · line 108
Quoted 8 lines
lib/librte_eal/common/include/arch/x86/rte_atomic_32.h +8
-0+static inline uint64_t +rte_atomic64_exchange (volatile uint64_t *dest,uint64_t val) +{ +uint64_t old; + +do { + old = *dest; + }while (rte_atomic64_t_cmpset (dest, old, val));
rte_atomic64_cmpset? (without _t)
Wasn't that fixed in later version?
PATCH v3 2/15 ethdev: add linkstatus get/set helper functions
In reply to Stephen HemmingerMany 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 <[email protected]>
--- lib/librte_ether/rte_ethdev.c | 36 ++++++++++++++++++++++++++++++++++++ lib/librte_ether/rte_ethdev.h | 28 ++++++++++++++++++++++++++++ 2 files changed, 64 insertions(+)
lib/librte_ether/rte_ethdev.c +36 -0
@@ -1495,6 +1495,42 @@ rte_eth_link_get_nowait(uint16_t port_id, struct rte_eth_link *eth_link) } } +int +_rte_eth_linkstatus_set (struct rte_eth_dev *dev, +const struct rte_eth_link *new_link) +{ +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 ; +} + +void +_rte_eth_linkstatus_get (const struct rte_eth_dev *dev, +struct rte_eth_link *link) +{ +volatile uint64_t *src = (uint64_t *)&(dev->data->dev_link); +uint64_t tmp; + +RTE_BUILD_BUG_ON (sizeof (*link) !=sizeof (uint64_t )); + +/* can't use rte_atomic64_read because it returns signed int */ +do { + tmp = *src; + }while (!rte_atomic64_cmpset (src, tmp, tmp)); + +memcpy (link, &tmp,sizeof (tmp)); +} +int rte_eth_stats_get (uint16_t port_id,struct rte_eth_stats *stats) {
lib/librte_ether/rte_ethdev.h +28 -0
@@ -2218,6 +2218,34 @@ int rte_eth_dev_set_link_down(uint16_t port_id);*/ void rte_eth_dev_close (uint16_t port_id); + +/** +* @internal +* Atomically set the link status for the specific device. +* It is for use by DPDK device driver use only. +* User applications should not call it +* +* @param dev +* Pointer to struct rte_eth_dev. +* @param link +* New link status value. +* @return +* -1 if link state has changed, 0 if the same. +*/ +int _rte_eth_linkstatus_set (struct rte_eth_dev *dev, +const struct rte_eth_link *link); + +/** +* @internal +* Atomically get the link speed and status. +* @param dev +* Pointer to struct rte_eth_dev. +* @param link +* link status value. +*/ +void _rte_eth_linkstatus_get (const struct rte_eth_dev *dev, +struct rte_eth_link *link); +/** * Reset a Ethernet device and keep its port id. *
Signature · 1 line
2.15.1
Re: [PATCH v3 02/15] ethdev: add linkstatus get/set helper functions
Quoted 25 lines · Stephen Hemminger
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 <[email protected]>--- lib/librte_ether/rte_ethdev.c | 36 ++++++++++++++++++++++++++++++++++++ lib/librte_ether/rte_ethdev.h | 28 ++++++++++++++++++++++++++++ 2 files changed, 64 insertions(+)lib/librte_ether/rte_ethdev.c +7
-0@@ -1495,6 +1495,42 @@ rte_eth_link_get_nowait(uint16_t port_id, struct rte_eth_link *eth_link) } } +int +_rte_eth_linkstatus_set (struct rte_eth_dev *dev, +const struct rte_eth_link *new_link) +{ +volatile uint64_t *dev_link + = (volatile uint64_t *)&(dev->data->dev_link); +union {
lib/librte_ether/rte_ethdev.c rte_eth_link_get_nowait(uint16_t port_id, struct rte_eth_link *eth_link) · line 1514
Quoted 10 lines
lib/librte_ether/rte_ethdev.c +10
-0+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. 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.
Quoted 61 lines
lib/librte_ether/rte_ethdev.c +19
-0+} + +void +_rte_eth_linkstatus_get (const struct rte_eth_dev *dev, +struct rte_eth_link *link) +{ +volatile uint64_t *src = (uint64_t *)&(dev->data->dev_link); +uint64_t tmp; + +RTE_BUILD_BUG_ON (sizeof (*link) !=sizeof (uint64_t )); + +/* can't use rte_atomic64_read because it returns signed int */ +do { + tmp = *src; + }while (!rte_atomic64_cmpset (src, tmp, tmp)); + +memcpy (link, &tmp,sizeof (tmp)); +} +int rte_eth_stats_get (uint16_t port_id,struct rte_eth_stats *stats) {lib/librte_ether/rte_ethdev.h +28
-0@@ -2218,6 +2218,34 @@ int rte_eth_dev_set_link_down(uint16_t port_id);*/ void rte_eth_dev_close (uint16_t port_id); + +/** +* @internal +* Atomically set the link status for the specific device. +* It is for use by DPDK device driver use only. +* User applications should not call it +* +* @param dev +* Pointer to struct rte_eth_dev. +* @param link +* New link status value. +* @return +* -1 if link state has changed, 0 if the same. +*/ +int _rte_eth_linkstatus_set (struct rte_eth_dev *dev, +const struct rte_eth_link *link); + +/** +* @internal +* Atomically get the link speed and status. +* @param dev +* Pointer to struct rte_eth_dev. +* @param link +* link status value. +*/ +void _rte_eth_linkstatus_get (const struct rte_eth_dev *dev, +struct rte_eth_link *link); +/** * Reset a Ethernet device and keep its port id. *
Re: [PATCH v3 02/15] ethdev: add linkstatus get/set helper functions
Quoted 33 lines · On Tue, 9 Jan 2018 13:30:46 +0300 Andrew Rybchenko <[email protected]>…
Quoted 32 lines · Stephen Hemminger
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 <[email protected]>--- lib/librte_ether/rte_ethdev.c | 36 ++++++++++++++++++++++++++++++++++++ lib/librte_ether/rte_ethdev.h | 28 ++++++++++++++++++++++++++++ 2 files changed, 64 insertions(+)lib/librte_ether/rte_ethdev.c +14
-0@@ -1495,6 +1495,42 @@ rte_eth_link_get_nowait(uint16_t port_id, struct rte_eth_link *eth_link) } } +int +_rte_eth_linkstatus_set (struct rte_eth_dev *dev, +const struct rte_eth_link *new_link) +{ +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,
Quoted 8 lines
lib/librte_ether/rte_ethdev.c rte_eth_link_get_nowait(uint16_t port_id, struct rte_eth_link *eth_link) · line 1514
Quoted 3 lines
lib/librte_ether/rte_ethdev.c +3
-0+ *(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. BTW, it looks like the return value of the
link_updatecallback 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.
Re: [PATCH v3 02/15] ethdev: add linkstatus get/set helper functions
Quoted 9 lines · Stephen Hemminger
Quoted 6 lines · On Tue, 9 Jan 2018 13:30:46 +0300 Andrew Rybchenko <[email protected]>…
Quoted 5 lines · Stephen Hemminger
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 <[email protected]>
<...>
Quoted 3 lines
Quoted 3 lines
lib/librte_ether/rte_ethdev.c rte_eth_link_get_nowait(uint16_t port_id, struct rte_eth_link *eth_link) · line 1500
Quoted 3 lines
lib/librte_ether/rte_ethdev.c +3
-0+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.
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
lib/librte_ether/rte_ethdev.c +8
-0+{ +volatile uint64_t *dev_link + = (volatile uint64_t *)&(dev->data->dev_link); +union { +uint64_t val64; +struct rte_eth_link link; + } orig; +
Quoted 8 lines
Quoted 8 lines
lib/librte_ether/rte_ethdev.c rte_eth_link_get_nowait(uint16_t port_id, struct rte_eth_link *eth_link) · line 1514
Quoted 6 lines
lib/librte_ether/rte_ethdev.c +6
-0+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.
Quoted 7 lines
Quoted 3 lines
BTW, it looks like the return value of the
link_updatecallback is not specified (described) and not used. It explains why different drivers behave differently and nobody notices it.It looks like
link_statuscallback 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.
PATCH v3 3/15 net/virtio: use eth_linkstatus_set
In reply to Stephen HemmingerUse the new comon code in ethdev to handle link status update.
Signed-off-by Stephen Hemminger <[email protected]>
--- drivers/net/virtio/virtio_ethdev.c | 64 +++++++------------------------------- 1 file changed, 11 insertions(+), 53 deletions(-)
drivers/net/virtio/virtio_ethdev.c +11 -53
@@ -14,7 +14,6 @@#include <rte_string_fns.h> #include <rte_memzone.h> #include <rte_malloc.h> -#include <rte_atomic.h> #include <rte_branch_prediction.h> #include <rte_pci.h> #include <rte_bus_pci.h>
@@ -771,46 +770,6 @@ static const struct eth_dev_ops virtio_eth_dev_ops = { .mac_addr_set = virtio_mac_addr_set, }; -static inline int -virtio_dev_atomic_read_link_status (struct rte_eth_dev *dev, -struct rte_eth_link *link) -{ -struct rte_eth_link *dst = link; -struct rte_eth_link *src = &(dev->data->dev_link); - -if (rte_atomic64_cmpset ((uint64_t *)dst, *(uint64_t *)dst, - *(uint64_t *)src) ==0 ) -return -1 ; - -return 0 ; -} - -/** -* Atomically writes the link status information into global -* structure rte_eth_dev. -* -* @param dev -* - Pointer to the structure rte_eth_dev to read from. -* - Pointer to the buffer to be saved with the link status. -* -* @return -* - On success, zero. -* - On failure, negative value. -*/ -static inline int -virtio_dev_atomic_write_link_status (struct rte_eth_dev *dev, -struct rte_eth_link *link) -{ -struct rte_eth_link *dst = &(dev->data->dev_link); -struct rte_eth_link *src = link; - -if (rte_atomic64_cmpset ((uint64_t *)dst, *(uint64_t *)dst, - *(uint64_t *)src) ==0 ) -return -1 ; - -return 0 ; -} -static void virtio_update_stats (struct rte_eth_dev *dev,struct rte_eth_stats *stats) {
@@ -1913,8 +1872,10 @@ static void virtio_dev_stop(struct rte_eth_dev *dev) {struct virtio_hw *hw = dev->data->dev_private; -struct rte_eth_link link;struct rte_intr_conf *intr_conf = &dev->data->dev_conf.intr_conf; +struct rte_eth_link link = { + .link_status =ETH_LINK_DOWN , + };PMD_INIT_LOG (DEBUG ,"stop" );
@@ -1922,21 +1883,19 @@ virtio_dev_stop(struct rte_eth_dev *dev)virtio_intr_disable (dev); hw->started =0 ; -memset (&link,0 ,sizeof (link)); -virtio_dev_atomic_write_link_status (dev, &link); +_rte_eth_linkstatus_set (dev, &link); }static int virtio_dev_link_update(struct rte_eth_dev *dev, __rte_unused int wait_to_complete) { -struct rte_eth_link link, old; -uint16_t status;struct virtio_hw *hw = dev->data->dev_private; -memset (&link,0 ,sizeof (link)); -virtio_dev_atomic_read_link_status (dev, &link); - old = link; - link.link_duplex =ETH_LINK_FULL_DUPLEX ; - link.link_speed =SPEED_10G ; +uint16_t status; +struct rte_eth_link link = { + .link_speed =ETH_SPEED_NUM_10G , + .link_duplex =ETH_LINK_FULL_DUPLEX , + .link_autoneg =ETH_LINK_SPEED_FIXED , + };if (hw->started ==0 ) { link.link_status =ETH_LINK_DOWN ;
@@ -1957,9 +1916,8 @@ virtio_dev_link_update(struct rte_eth_dev *dev, __rte_unused int wait_to_complet }else { link.link_status =ETH_LINK_UP ; } -virtio_dev_atomic_write_link_status (dev, &link); -return (old.link_status == link.link_status) ?-1 :0 ; +return _rte_eth_linkstatus_set (dev, &link); } static int
Signature · 1 line
2.15.1
Re: [PATCH v3 03/15] net/virtio: use eth_linkstatus_set
Quoted 3 lines · Stephen Hemminger
Use the new comon code in ethdev to handle link status update.
Signed-off-by Stephen Hemminger <[email protected]>
<...>
Quoted 6 lines
static int
virtio_dev_link_update(struct rte_eth_dev *dev, __rte_unused int wait_to_complete){drivers/net/virtio/virtio_ethdev.c +0
-2-struct rte_eth_link link, old; -uint16_t status;struct virtio_hw *hw = dev->data->dev_private;
drivers/net/virtio/virtio_ethdev.c virtio_dev_stop(struct rte_eth_dev *dev) · line 1897
Quoted 10 lines
drivers/net/virtio/virtio_ethdev.c +5
-5-memset (&link,0 ,sizeof (link)); -virtio_dev_atomic_read_link_status (dev, &link); - old = link; - link.link_duplex =ETH_LINK_FULL_DUPLEX ; - link.link_speed =SPEED_10G ; +uint16_t status; +struct rte_eth_link link = { + .link_speed =ETH_SPEED_NUM_10G , + .link_duplex =ETH_LINK_FULL_DUPLEX , + .link_autoneg =ETH_LINK_SPEED_FIXED ,
With a new cleanup commit [1] ETH_LINK_FIXED used for autoneg value (instead of ETH_LINK_SPEED_FIXED). And since is ETH_LINK_FIXED is 0, setting autoneg to it can be omitted.
[1]93dd7b328a9b ("ethdev: fix link autonegotiation value")
This commit is in next-net.
Unlike Linux "net-next" sub-tree which is for next release development tree, dpdk next-net is for current release network drivers which this patch seems fits into.
I already rebased this patchset to next-net, I will share it to continue discussion from that point.
Quoted 16 lines
drivers/net/virtio/virtio_ethdev.c +2
-2+ };if (hw->started ==0 ) { link.link_status =ETH_LINK_DOWN ;@@ -1957,9 +1916,8 @@ virtio_dev_link_update(struct rte_eth_dev *dev, __rte_unused int wait_to_complet }else { link.link_status =ETH_LINK_UP ; } -virtio_dev_atomic_write_link_status (dev, &link); -return (old.link_status == link.link_status) ?-1 :0 ; +return _rte_eth_linkstatus_set (dev, &link); } static int
PATCH v3 4/15 net/vmxnet3: use rte_eth_linkstatus_set
In reply to Stephen HemmingerUse new _rte_eth_link_update helper.
Also remove no longer necessary include of rte_atomic.h
Signed-off-by Stephen Hemminger <[email protected]>
--- drivers/net/vmxnet3/vmxnet3_ethdev.c | 85 +++++++----------------------------- 1 file changed, 15 insertions(+), 70 deletions(-)
drivers/net/vmxnet3/vmxnet3_ethdev.c +15 -70
@@ -20,7 +20,6 @@#include <rte_debug.h> #include <rte_pci.h> #include <rte_bus_pci.h> -#include <rte_atomic.h> #include <rte_branch_prediction.h> #include <rte_memory.h> #include <rte_memzone.h>
@@ -157,59 +156,6 @@ gpa_zone_reserve(struct rte_eth_dev *dev, uint32_t size,return rte_memzone_reserve_aligned (z_name, size, socket_id,0 , align); } -/** -* Atomically reads the link status information from global -* structure rte_eth_dev. -* -* @param dev -* - Pointer to the structure rte_eth_dev to read from. -* - Pointer to the buffer to be saved with the link status. -* -* @return -* - On success, zero. -* - On failure, negative value. -*/ - -static int -vmxnet3_dev_atomic_read_link_status (struct rte_eth_dev *dev, -struct rte_eth_link *link) -{ -struct rte_eth_link *dst = link; -struct rte_eth_link *src = &(dev->data->dev_link); - -if (rte_atomic64_cmpset ((uint64_t *)dst, *(uint64_t *)dst, - *(uint64_t *)src) ==0 ) -return -1 ; - -return 0 ; -} - -/** -* Atomically writes the link status information into global -* structure rte_eth_dev. -* -* @param dev -* - Pointer to the structure rte_eth_dev to write to. -* - Pointer to the buffer to be saved with the link status. -* -* @return -* - On success, zero. -* - On failure, negative value. -*/ -static int -vmxnet3_dev_atomic_write_link_status (struct rte_eth_dev *dev, -struct rte_eth_link *link) -{ -struct rte_eth_link *dst = &(dev->data->dev_link); -struct rte_eth_link *src = link; - -if (rte_atomic64_cmpset ((uint64_t *)dst, *(uint64_t *)dst, - *(uint64_t *)src) ==0 ) -return -1 ; - -return 0 ; -} -/* * This function is based on vmxnet3_disable_intr() */
@@ -814,9 +760,13 @@ vmxnet3_dev_start(struct rte_eth_dev *dev)static void vmxnet3_dev_stop (struct rte_eth_dev *dev) { -struct rte_eth_link link;struct vmxnet3_hw *hw = dev->data->dev_private; - +struct rte_eth_link link = { + .link_speed =ETH_SPEED_NUM_10G , + .link_duplex =ETH_LINK_FULL_DUPLEX , + .link_autoneg =ETH_LINK_SPEED_FIXED , + .link_status =ETH_LINK_DOWN , + };PMD_INIT_FUNC_TRACE ();if (hw->adapter_stopped ==1 ) {
@@ -849,8 +799,7 @@ vmxnet3_dev_stop(struct rte_eth_dev *dev)vmxnet3_dev_clear_queues (dev);/* Clear recorded link status */ -memset (&link,0 ,sizeof (link)); -vmxnet3_dev_atomic_write_link_status (dev, &link); +_rte_eth_linkstatus_set (dev, &link); }/*
@@ -1130,25 +1079,21 @@ __vmxnet3_dev_link_update(struct rte_eth_dev *dev, __rte_unused int wait_to_complete) {struct vmxnet3_hw *hw = dev->data->dev_private; -struct rte_eth_link old = {0 }, link; +struct rte_eth_link link = { + .link_speed =ETH_SPEED_NUM_10G , + .link_duplex =ETH_LINK_FULL_DUPLEX , + .link_autoneg =ETH_LINK_SPEED_FIXED , + .link_status =ETH_LINK_DOWN , + };uint32_t ret; -memset (&link,0 ,sizeof (link)); -vmxnet3_dev_atomic_read_link_status (dev, &old); -VMXNET3_WRITE_BAR1_REG (hw,VMXNET3_REG_CMD ,VMXNET3_CMD_GET_LINK ); ret =VMXNET3_READ_BAR1_REG (hw,VMXNET3_REG_CMD ); -if (ret &0x1 ) { +if (ret &0x1 ) link.link_status =ETH_LINK_UP ; - link.link_duplex =ETH_LINK_FULL_DUPLEX ; - link.link_speed =ETH_SPEED_NUM_10G ; - link.link_autoneg =ETH_LINK_SPEED_FIXED ; - } - -vmxnet3_dev_atomic_write_link_status (dev, &link); -return (old.link_status == link.link_status) ?-1 :0 ; +return _rte_eth_linkstatus_set (dev, &link); } static int
Signature · 1 line
2.15.1
Re: [PATCH v3 04/15] net/vmxnet3: use rte_eth_linkstatus_set
Quoted 4 lines · Stephen Hemminger
Use new
_rte_eth_link_updatehelper.
Also remove no longer necessary include ofrte_atomic.hSigned-off-by Stephen Hemminger <[email protected]>
<...>
drivers/net/vmxnet3/vmxnet3_ethdev.c __vmxnet3_dev_link_update(struct rte_eth_dev *dev, · line 1086
Quoted 10 lines
drivers/net/vmxnet3/vmxnet3_ethdev.c +5
-1@@ -1130,25 +1079,21 @@ __vmxnet3_dev_link_update(struct rte_eth_dev *dev, __rte_unused int wait_to_complete) {struct vmxnet3_hw *hw = dev->data->dev_private; -struct rte_eth_link old = {0 }, link; +struct rte_eth_link link = { + .link_speed =ETH_SPEED_NUM_10G , + .link_duplex =ETH_LINK_FULL_DUPLEX , + .link_autoneg =ETH_LINK_SPEED_FIXED , + .link_status =ETH_LINK_DOWN ,
Link status before this patch was "ETH_LINK_UP", this is changing it to "ETH_LINK_DOWN", intentional?
Quoted 25 lines
drivers/net/vmxnet3/vmxnet3_ethdev.c +3
-11+ };uint32_t ret; -memset (&link,0 ,sizeof (link)); -vmxnet3_dev_atomic_read_link_status (dev, &old); -VMXNET3_WRITE_BAR1_REG (hw,VMXNET3_REG_CMD ,VMXNET3_CMD_GET_LINK ); ret =VMXNET3_READ_BAR1_REG (hw,VMXNET3_REG_CMD ); -if (ret &0x1 ) { +if (ret &0x1 ) link.link_status =ETH_LINK_UP ; - link.link_duplex =ETH_LINK_FULL_DUPLEX ; - link.link_speed =ETH_SPEED_NUM_10G ; - link.link_autoneg =ETH_LINK_SPEED_FIXED ; - } - -vmxnet3_dev_atomic_write_link_status (dev, &link); -return (old.link_status == link.link_status) ?-1 :0 ; +return _rte_eth_linkstatus_set (dev, &link); } static int
PATCH v3 5/15 net/dpaa2: use rte_eth_linkstatus_set
In reply to Stephen HemmingerUse new helper function to update the link status.
As a good side effect this fixes a but because this driver was not returning correct status (should be -1 in link_status changed).
Signed-off-by Stephen Hemminger <[email protected]>
--- drivers/net/dpaa2/dpaa2_ethdev.c | 65 +++++----------------------------------- 1 file changed, 7 insertions(+), 58 deletions(-)
drivers/net/dpaa2/dpaa2_ethdev.c +7 -58
@@ -56,58 +56,6 @@ static int dpaa2_dev_set_link_up(struct rte_eth_dev *dev);static int dpaa2_dev_set_link_down (struct rte_eth_dev *dev);static int dpaa2_dev_mtu_set (struct rte_eth_dev *dev,uint16_t mtu); -/** -* Atomically reads the link status information from global -* structure rte_eth_dev. -* -* @param dev -* - Pointer to the structure rte_eth_dev to read from. -* - Pointer to the buffer to be saved with the link status. -* -* @return -* - On success, zero. -* - On failure, negative value. -*/ -static inline int -dpaa2_dev_atomic_read_link_status (struct rte_eth_dev *dev, -struct rte_eth_link *link) -{ -struct rte_eth_link *dst = link; -struct rte_eth_link *src = &dev->data->dev_link; - -if (rte_atomic64_cmpset ((uint64_t *)dst, *(uint64_t *)dst, - *(uint64_t *)src) ==0 ) -return -1 ; - -return 0 ; -} - -/** -* Atomically writes the link status information into global -* structure rte_eth_dev. -* -* @param dev -* - Pointer to the structure rte_eth_dev to read from. -* - Pointer to the buffer to be saved with the link status. -* -* @return -* - On success, zero. -* - On failure, negative value. -*/ -static inline int -dpaa2_dev_atomic_write_link_status (struct rte_eth_dev *dev, -struct rte_eth_link *link) -{ -struct rte_eth_link *dst = &dev->data->dev_link; -struct rte_eth_link *src = link; - -if (rte_atomic64_cmpset ((uint64_t *)dst, *(uint64_t *)dst, - *(uint64_t *)src) ==0 ) -return -1 ; - -return 0 ; -} -static int dpaa2_vlan_filter_set (struct rte_eth_dev *dev,uint16_t vlan_id,int on) {
@@ -804,7 +752,7 @@ dpaa2_dev_stop(struct rte_eth_dev *dev)/* clear the recorded link status */ memset (&link,0 ,sizeof (link)); -dpaa2_dev_atomic_write_link_status (dev, &link); +_rte_eth_link_write (dev, &link); } static void
@@ -836,7 +784,7 @@ dpaa2_dev_close(struct rte_eth_dev *dev) }memset (&link,0 ,sizeof (link)); -dpaa2_dev_atomic_write_link_status (dev, &link); +_rte_eth_link_write (dev, &link); } static void
@@ -1291,8 +1239,8 @@ dpaa2_dev_link_update(struct rte_eth_dev *dev,RTE_LOG (ERR ,PMD ,"dpni is NULL\n" );return 0 ; } -memset (&old,0 ,sizeof (old)); -dpaa2_dev_atomic_read_link_status (dev, &old); + +_rte_eth_link_read (dev, &old); ret =dpni_get_link_state (dpni,CMD_PRI_LOW , priv->token, &state);if (ret <0 ) {
@@ -1314,13 +1262,14 @@ dpaa2_dev_link_update(struct rte_eth_dev *dev,else link.link_duplex =ETH_LINK_FULL_DUPLEX ; -dpaa2_dev_atomic_write_link_status (dev, &link); + ret =_rte_eth_linkstatus_set (dev, &link);if (link.link_status)PMD_DRV_LOG (INFO ,"Port %d Link is Up\n" , dev->data->port_id);else PMD_DRV_LOG (INFO ,"Port %d Link is Down" , dev->data->port_id); -return 0 ; + +return ret; }/**
Signature · 1 line
2.15.1
PATCH v3 6/15 net/nfp: use rte_eth_linkstatus functions
In reply to Stephen HemmingerUse new _rte_eth_linkstatus_get/set helper function.
Signed-off-by Stephen Hemminger <[email protected]>
--- drivers/net/nfp/nfp_net.c | 75 +++++------------------------------------------ 1 file changed, 8 insertions(+), 67 deletions(-)
drivers/net/nfp/nfp_net.c +8 -67
@@ -204,57 +204,6 @@ nn_cfg_writeq(struct nfp_net_hw *hw, int off, uint64_t val)nn_writeq (rte_cpu_to_le_64 (val), hw->ctrl_bar + off); } -/* -* Atomically reads link status information from global structure rte_eth_dev. -* -* @param dev -* - Pointer to the structure rte_eth_dev to read from. -* - Pointer to the buffer to be saved with the link status. -* -* @return -* - On success, zero. -* - On failure, negative value. -*/ -static inline int -nfp_net_dev_atomic_read_link_status (struct rte_eth_dev *dev, -struct rte_eth_link *link) -{ -struct rte_eth_link *dst = link; -struct rte_eth_link *src = &dev->data->dev_link; - -if (rte_atomic64_cmpset ((uint64_t *)dst, *(uint64_t *)dst, - *(uint64_t *)src) ==0 ) -return -1 ; - -return 0 ; -} - -/* -* Atomically writes the link status information into global -* structure rte_eth_dev. -* -* @param dev -* - Pointer to the structure rte_eth_dev to read from. -* - Pointer to the buffer to be saved with the link status. -* -* @return -* - On success, zero. -* - On failure, negative value. -*/ -static inline int -nfp_net_dev_atomic_write_link_status (struct rte_eth_dev *dev, -struct rte_eth_link *link) -{ -struct rte_eth_link *dst = &dev->data->dev_link; -struct rte_eth_link *src = link; - -if (rte_atomic64_cmpset ((uint64_t *)dst, *(uint64_t *)dst, - *(uint64_t *)src) ==0 ) -return -1 ; - -return 0 ; -} -static void nfp_net_rx_queue_release_mbufs (struct nfp_net_rxq *rxq) {
@@ -977,7 +926,7 @@ static int nfp_net_link_update(struct rte_eth_dev *dev, __rte_unused int wait_to_complete) { struct nfp_net_hw *hw; -struct rte_eth_link link, old; +struct rte_eth_link link;uint32_t nn_link_status; static const uint32_t ls_to_ethtool[] = {
@@ -995,9 +944,6 @@ nfp_net_link_update(struct rte_eth_dev *dev, __rte_unused int wait_to_complete) hw =NFP_NET_DEV_PRIVATE_TO_HW (dev->data->dev_private); -memset (&old,0 ,sizeof (old)); -nfp_net_dev_atomic_read_link_status (dev, &old); - nn_link_status =nn_cfg_readl (hw,NFP_NET_CFG_STS );memset (&link,0 ,sizeof (struct rte_eth_link ));
@@ -1015,15 +961,13 @@ nfp_net_link_update(struct rte_eth_dev *dev, __rte_unused int wait_to_complete)else link.link_speed = ls_to_ethtool[nn_link_status]; -if (old.link_status != link.link_status) { -nfp_net_dev_atomic_write_link_status (dev, &link); -if (link.link_status) -PMD_DRV_LOG (INFO ,"NIC Link is Up\n" ); -else -PMD_DRV_LOG (INFO ,"NIC Link is Down\n" ); +if (_rte_eth_linkstatus_set (dev, &link) ==0 )return 0 ; - } +if (link.link_status) +PMD_DRV_LOG (INFO ,"NIC Link is Up\n" ); +else +PMD_DRV_LOG (INFO ,"NIC Link is Down\n" );return -1 ; }
@@ -1354,8 +1298,7 @@ nfp_net_dev_link_status_print(struct rte_eth_dev *dev)struct rte_pci_device *pci_dev =RTE_ETH_DEV_TO_PCI (dev);struct rte_eth_link link; -memset (&link,0 ,sizeof (link)); -nfp_net_dev_atomic_read_link_status (dev, &link); +_rte_eth_linkstatus_get (dev, &link);if (link.link_status) RTE_LOG(INFO, PMD, "Port %d: Link Up - speed %u Mbps - %s\n", dev->data->port_id, link.link_speed,
@@ -1408,9 +1351,7 @@ nfp_net_dev_interrupt_handler(void *param)PMD_DRV_LOG (DEBUG ,"We got a LSC interrupt!!!\n" ); -/* get the link status */ -memset (&link,0 ,sizeof (link)); -nfp_net_dev_atomic_read_link_status (dev, &link); +_rte_eth_linkstatus_get (dev, &link);nfp_net_link_update (dev,0 );
Signature · 1 line
2.15.1
PATCH v3 7/15 net/e1000: use rte_eth_linkstatus helpers
In reply to Stephen HemmingerUse new rte_eth_linkstatus_get/set API.
Signed-off-by Stephen Hemminger <[email protected]>
--- drivers/net/e1000/em_ethdev.c | 70 +++-------------------------------------- drivers/net/e1000/igb_ethdev.c | 71 +++--------------------------------------- 2 files changed, 10 insertions(+), 131 deletions(-)
drivers/net/e1000/em_ethdev.c +5 -65
@@ -20,7 +20,6 @@#include <rte_ethdev_pci.h> #include <rte_memory.h> #include <rte_eal.h> -#include <rte_atomic.h> #include <rte_malloc.h> #include <rte_dev.h>
@@ -193,57 +192,6 @@ static const struct eth_dev_ops eth_em_ops = { .txq_info_get = em_txq_info_get, }; -/** -* Atomically reads the link status information from global -* structure rte_eth_dev. -* -* @param dev -* - Pointer to the structure rte_eth_dev to read from. -* - Pointer to the buffer to be saved with the link status. -* -* @return -* - On success, zero. -* - On failure, negative value. -*/ -static inline int -rte_em_dev_atomic_read_link_status (struct rte_eth_dev *dev, -struct rte_eth_link *link) -{ -struct rte_eth_link *dst = link; -struct rte_eth_link *src = &(dev->data->dev_link); - -if (rte_atomic64_cmpset ((uint64_t *)dst, *(uint64_t *)dst, - *(uint64_t *)src) ==0 ) -return -1 ; - -return 0 ; -} - -/** -* Atomically writes the link status information into global -* structure rte_eth_dev. -* -* @param dev -* - Pointer to the structure rte_eth_dev to read from. -* - Pointer to the buffer to be saved with the link status. -* -* @return -* - On success, zero. -* - On failure, negative value. -*/ -static inline int -rte_em_dev_atomic_write_link_status (struct rte_eth_dev *dev, -struct rte_eth_link *link) -{ -struct rte_eth_link *dst = &(dev->data->dev_link); -struct rte_eth_link *src = link; - -if (rte_atomic64_cmpset ((uint64_t *)dst, *(uint64_t *)dst, - *(uint64_t *)src) ==0 ) -return -1 ; - -return 0 ; -}/** * eth_em_dev_is_ich8 - Check for ICH8 device
@@ -769,7 +717,7 @@ eth_em_stop(struct rte_eth_dev *dev)/* clear the recorded link status */ memset (&link,0 ,sizeof (link)); -rte_em_dev_atomic_write_link_status (dev, &link); +_rte_eth_linkstatus_set (dev, &link);if (!rte_intr_allow_others (intr_handle))/* resume to the default handler */
@@ -1127,7 +1075,7 @@ eth_em_link_update(struct rte_eth_dev *dev, int wait_to_complete) {struct e1000_hw *hw =E1000_DEV_PRIVATE_TO_HW (dev->data->dev_private); -struct rte_eth_link link, old; +struct rte_eth_link link;int link_check, count; link_check =0 ;
@@ -1162,8 +1110,6 @@ eth_em_link_update(struct rte_eth_dev *dev, int wait_to_complete)rte_delay_ms (EM_LINK_UPDATE_CHECK_INTERVAL ); }memset (&link,0 ,sizeof (link)); -rte_em_dev_atomic_read_link_status (dev, &link); - old = link;/* Now we check if a transition has happened */ if (link_check && (link.link_status ==ETH_LINK_DOWN )) {
@@ -1182,14 +1128,8 @@ eth_em_link_update(struct rte_eth_dev *dev, int wait_to_complete) link.link_status =ETH_LINK_DOWN ; link.link_autoneg =ETH_LINK_SPEED_FIXED ; } -rte_em_dev_atomic_write_link_status (dev, &link); - -/* not changed */ -if (old.link_status == link.link_status) -return -1 ; -/* changed */ -return 0 ; +return _rte_eth_linkstatus_set (dev, &link); }/*
@@ -1599,8 +1539,8 @@ eth_em_interrupt_action(struct rte_eth_dev *dev,if (ret <0 )return 0 ; -memset (&link,0 ,sizeof (link)); -rte_em_dev_atomic_read_link_status (dev, &link); +_rte_eth_linkstatus_get (dev, &link); +if (link.link_status) { PMD_INIT_LOG(INFO, " Port %d: Link Up - speed %u Mbps - %s", dev->data->port_id, link.link_speed,
drivers/net/e1000/igb_ethdev.c +5 -66
120 lines not on this page · raw message
Signature · 1 line
2.15.1
PATCH v3 8/15 net/ixgbe: use rte_eth_linkstatus functions
In reply to Stephen HemmingerUse the new helper functions from eth_dev for
handling atomic link_info update.
Signed-off-by Stephen Hemminger <[email protected]>
--- drivers/net/ixgbe/ixgbe_ethdev.c | 99 +++++++--------------------------------- 1 file changed, 17 insertions(+), 82 deletions(-)
drivers/net/ixgbe/ixgbe_ethdev.c +17 -82
@@ -20,7 +20,6 @@#include <rte_debug.h> #include <rte_pci.h> #include <rte_bus_pci.h> -#include <rte_atomic.h> #include <rte_branch_prediction.h> #include <rte_memory.h> #include <rte_eal.h>
@@ -781,58 +780,6 @@ static const struct rte_ixgbe_xstats_name_off rte_ixgbevf_stats_strings[] = {#define IXGBEVF_NB_XSTATS (sizeof(rte_ixgbevf_stats_strings) / \ sizeof(rte_ixgbevf_stats_strings[0])) -/** -* Atomically reads the link status information from global -* structure rte_eth_dev. -* -* @param dev -* - Pointer to the structure rte_eth_dev to read from. -* - Pointer to the buffer to be saved with the link status. -* -* @return -* - On success, zero. -* - On failure, negative value. -*/ -static inline int -rte_ixgbe_dev_atomic_read_link_status (struct rte_eth_dev *dev, -struct rte_eth_link *link) -{ -struct rte_eth_link *dst = link; -struct rte_eth_link *src = &(dev->data->dev_link); - -if (rte_atomic64_cmpset ((uint64_t *)dst, *(uint64_t *)dst, - *(uint64_t *)src) ==0 ) -return -1 ; - -return 0 ; -} - -/** -* Atomically writes the link status information into global -* structure rte_eth_dev. -* -* @param dev -* - Pointer to the structure rte_eth_dev to read from. -* - Pointer to the buffer to be saved with the link status. -* -* @return -* - On success, zero. -* - On failure, negative value. -*/ -static inline int -rte_ixgbe_dev_atomic_write_link_status (struct rte_eth_dev *dev, -struct rte_eth_link *link) -{ -struct rte_eth_link *dst = &(dev->data->dev_link); -struct rte_eth_link *src = link; - -if (rte_atomic64_cmpset ((uint64_t *)dst, *(uint64_t *)dst, - *(uint64_t *)src) ==0 ) -return -1 ; - -return 0 ; -} -/* * This function is the same as ixgbe_is_sfp() in base/ixgbe.h. */
@@ -2753,7 +2700,7 @@ ixgbe_dev_stop(struct rte_eth_dev *dev)/* Clear recorded link status */ memset (&link,0 ,sizeof (link)); -rte_ixgbe_dev_atomic_write_link_status (dev, &link); +_rte_eth_linkstatus_set (dev, &link);if (!rte_intr_allow_others (intr_handle))/* resume to the default handler */
@@ -3938,7 +3885,6 @@ ixgbe_dev_link_update_share(struct rte_eth_dev *dev, int wait_to_complete, int vf) {struct ixgbe_hw *hw =IXGBE_DEV_PRIVATE_TO_HW (dev->data->dev_private); -struct rte_eth_link link, old;ixgbe_link_speed link_speed =IXGBE_LINK_SPEED_UNKNOWN ;struct ixgbe_interrupt *intr =IXGBE_DEV_PRIVATE_TO_INTR (dev->data->dev_private);
@@ -3947,13 +3893,11 @@ ixgbe_dev_link_update_share(struct rte_eth_dev *dev,u32 speed =0 ;int wait =1 ;bool autoneg = false; - - link.link_status =ETH_LINK_DOWN ; - link.link_speed =0 ; - link.link_duplex =ETH_LINK_HALF_DUPLEX ; - link.link_autoneg =ETH_LINK_AUTONEG ; -memset (&old,0 ,sizeof (old)); -rte_ixgbe_dev_atomic_read_link_status (dev, &old); +struct rte_eth_link link = { + .link_status =ETH_LINK_DOWN , + .link_duplex =ETH_LINK_HALF_DUPLEX , + .link_autoneg =ETH_LINK_AUTONEG , + }; hw->mac.get_link_status = true;
@@ -3977,19 +3921,15 @@ ixgbe_dev_link_update_share(struct rte_eth_dev *dev,if (diag !=0 ) { link.link_speed =ETH_SPEED_NUM_100M ; link.link_duplex =ETH_LINK_FULL_DUPLEX ; -rte_ixgbe_dev_atomic_write_link_status (dev, &link); -if (link.link_status == old.link_status) -return -1 ; -return 0 ; + +return _rte_eth_linkstatus_set (dev, &link); }if (link_up ==0 ) { -rte_ixgbe_dev_atomic_write_link_status (dev, &link); intr->flags |=IXGBE_FLAG_NEED_LINK_CONFIG ; -if (link.link_status == old.link_status) -return -1 ; -return 0 ; +return _rte_eth_linkstatus_set (dev, &link); } + intr->flags &= ~IXGBE_FLAG_NEED_LINK_CONFIG ; link.link_status =ETH_LINK_UP ; link.link_duplex =ETH_LINK_FULL_DUPLEX ;
@@ -4021,12 +3961,8 @@ ixgbe_dev_link_update_share(struct rte_eth_dev *dev, link.link_speed =ETH_SPEED_NUM_10G ;break ; } -rte_ixgbe_dev_atomic_write_link_status (dev, &link); -if (link.link_status == old.link_status) -return -1 ; - -return 0 ; +return _rte_eth_linkstatus_set (dev, &link); } static int
@@ -4225,8 +4161,8 @@ ixgbe_dev_link_status_print(struct rte_eth_dev *dev)struct rte_pci_device *pci_dev =RTE_ETH_DEV_TO_PCI (dev);struct rte_eth_link link; -memset (&link,0 ,sizeof (link)); -rte_ixgbe_dev_atomic_read_link_status (dev, &link); +_rte_eth_linkstatus_get (dev, &link); +if (link.link_status) { PMD_INIT_LOG(INFO, "Port %d: Link Up - speed %u Mbps - %s", (int)(dev->data->port_id),
@@ -4261,7 +4197,6 @@ ixgbe_dev_interrupt_action(struct rte_eth_dev *dev,struct ixgbe_interrupt *intr =IXGBE_DEV_PRIVATE_TO_INTR (dev->data->dev_private);int64_t timeout; -struct rte_eth_link link;struct ixgbe_hw *hw =IXGBE_DEV_PRIVATE_TO_HW (dev->data->dev_private);
@@ -4278,9 +4213,10 @@ ixgbe_dev_interrupt_action(struct rte_eth_dev *dev, }if (intr->flags &IXGBE_FLAG_NEED_LINK_UPDATE ) { +struct rte_eth_link link; +/* get the link status before link update, for predicting later */ -memset (&link,0 ,sizeof (link)); -rte_ixgbe_dev_atomic_read_link_status (dev, &link); +_rte_eth_linkstatus_get (dev, &link);ixgbe_dev_link_update (dev,0 );
@@ -6799,9 +6735,8 @@ ixgbe_start_timecounters(struct rte_eth_dev *dev)uint32_t shift =0 ;/* Get current link speed. */ -memset (&link,0 ,sizeof (link));ixgbe_dev_link_update (dev,1 ); -rte_ixgbe_dev_atomic_read_link_status (dev, &link); +_rte_eth_linkstatus_get (dev, &link);switch (link.link_speed) {case ETH_SPEED_NUM_100M :
Signature · 1 line
2.15.1
PATCH v3 9/15 net/sfc: use new rte_eth_linkstatus functions
In reply to Stephen HemmingerUse the new API (_rte_eth_linkstatus_set) to handle link status update.
Signed-off-by Stephen Hemminger <[email protected]>
--- drivers/net/sfc/sfc_ethdev.c | 27 +++++++-------------------- drivers/net/sfc/sfc_ev.c | 23 ++++------------------- 2 files changed, 11 insertions(+), 39 deletions(-)
drivers/net/sfc/sfc_ethdev.c +7 -20
@@ -238,22 +238,12 @@ static int sfc_dev_link_update(struct rte_eth_dev *dev, int wait_to_complete) {struct sfc_adapter *sa = dev->data->dev_private; -struct rte_eth_link *dev_link = &dev->data->dev_link; -struct rte_eth_link old_link;struct rte_eth_link current_link;sfc_log_init (sa,"entry" ); -retry: -EFX_STATIC_ASSERT (sizeof (*dev_link) ==sizeof (rte_atomic64_t)); - *(int64_t *)&old_link =rte_atomic64_read ((rte_atomic64_t *)dev_link); -if (sa->state !=SFC_ADAPTER_STARTED ) {sfc_port_link_mode_to_info (EFX_LINK_UNKNOWN , ¤t_link); -if (!rte_atomic64_cmpset ((volatile uint64_t *)dev_link, - *(uint64_t *)&old_link, - *(uint64_t *)¤t_link)) - goto retry; }else if (wait_to_complete) {efx_link_mode_t link_mode;
@@ -261,21 +251,18 @@ sfc_dev_link_update(struct rte_eth_dev *dev, int wait_to_complete) link_mode =EFX_LINK_UNKNOWN ;sfc_port_link_mode_to_info (link_mode, ¤t_link); -if (!rte_atomic64_cmpset ((volatile uint64_t *)dev_link, - *(uint64_t *)&old_link, - *(uint64_t *)¤t_link)) - goto retry; }else {sfc_ev_mgmt_qpoll (sa); - *(int64_t *)¤t_link = -rte_atomic64_read ((rte_atomic64_t *)dev_link); +_rte_eth_linkstatus_get (dev, ¤t_link); } -if (old_link.link_status != current_link.link_status) -sfc_info (sa,"Link status is %s" , - current_link.link_status ?"UP" :"DOWN" ); +if (_rte_eth_linkstatus_set (dev, ¤t_link) ==0 ) +return 0 ; + +sfc_info (sa,"Link status is %s" , + current_link.link_status ?"UP" :"DOWN" ); -return old_link.link_status == current_link.link_status ?0 :-1 ; +return -1 ; } static void
drivers/net/sfc/sfc_ev.c +4 -19
@@ -404,29 +404,14 @@ sfc_ev_link_change(void *arg, efx_link_mode_t link_mode) {struct sfc_evq *evq = arg;struct sfc_adapter *sa = evq->sa; -struct rte_eth_link *dev_link = &sa->eth_dev->data->dev_link;struct rte_eth_link new_link; -uint64_t new_link_u64; -uint64_t old_link_u64; - -EFX_STATIC_ASSERT (sizeof (*dev_link) ==sizeof (rte_atomic64_t));sfc_port_link_mode_to_info (link_mode, &new_link); +if (_rte_eth_linkstatus_set (sa->eth_dev, &new_link) ==0 ) +return B_FALSE ; - new_link_u64 = *(uint64_t *)&new_link; -do { - old_link_u64 =rte_atomic64_read ((rte_atomic64_t *)dev_link); -if (old_link_u64 == new_link_u64) -break ; - -if (rte_atomic64_cmpset ((volatile uint64_t *)dev_link, - old_link_u64, new_link_u64)) { - evq->sa->port.lsc_seq++; -break ; - } - }while (B_TRUE ); - -return B_FALSE ; + evq->sa->port.lsc_seq++; +return B_TRUE ; }static const efx_ev_callbacks_t sfc_ev_callbacks = {
Signature · 1 line
2.15.1
Re: [PATCH v3 09/15] net/sfc: use new rte_eth_linkstatus functions
Quoted 89 lines · Stephen Hemminger
Use the new API (
_rte_eth_linkstatus_set) to handle link status update.Signed-off-by Stephen Hemminger <[email protected]>--- drivers/net/sfc/sfc_ethdev.c | 27 +++++++-------------------- drivers/net/sfc/sfc_ev.c | 23 ++++------------------- 2 files changed, 11 insertions(+), 39 deletions(-)drivers/net/sfc/sfc_ethdev.c +7
-20@@ -238,22 +238,12 @@ static int sfc_dev_link_update(struct rte_eth_dev *dev, int wait_to_complete) {struct sfc_adapter *sa = dev->data->dev_private; -struct rte_eth_link *dev_link = &dev->data->dev_link; -struct rte_eth_link old_link;struct rte_eth_link current_link;sfc_log_init (sa,"entry" ); -retry: -EFX_STATIC_ASSERT (sizeof (*dev_link) ==sizeof (rte_atomic64_t)); - *(int64_t *)&old_link =rte_atomic64_read ((rte_atomic64_t *)dev_link); -if (sa->state !=SFC_ADAPTER_STARTED ) {sfc_port_link_mode_to_info (EFX_LINK_UNKNOWN , ¤t_link); -if (!rte_atomic64_cmpset ((volatile uint64_t *)dev_link, - *(uint64_t *)&old_link, - *(uint64_t *)¤t_link)) - goto retry; }else if (wait_to_complete) {efx_link_mode_t link_mode;@@ -261,21 +251,18 @@ sfc_dev_link_update(struct rte_eth_dev *dev, int wait_to_complete) link_mode =EFX_LINK_UNKNOWN ;sfc_port_link_mode_to_info (link_mode, ¤t_link); -if (!rte_atomic64_cmpset ((volatile uint64_t *)dev_link, - *(uint64_t *)&old_link, - *(uint64_t *)¤t_link)) - goto retry; }else {sfc_ev_mgmt_qpoll (sa); - *(int64_t *)¤t_link = -rte_atomic64_read ((rte_atomic64_t *)dev_link); +_rte_eth_linkstatus_get (dev, ¤t_link); } -if (old_link.link_status != current_link.link_status) -sfc_info (sa,"Link status is %s" , - current_link.link_status ?"UP" :"DOWN" ); +if (_rte_eth_linkstatus_set (dev, ¤t_link) ==0 ) +return 0 ; + +sfc_info (sa,"Link status is %s" , + current_link.link_status ?"UP" :"DOWN" ); -return old_link.link_status == current_link.link_status ?0 :-1 ; +return -1 ; } static voiddrivers/net/sfc/sfc_ev.c +2
-11@@ -404,29 +404,14 @@ sfc_ev_link_change(void *arg, efx_link_mode_t link_mode) {struct sfc_evq *evq = arg;struct sfc_adapter *sa = evq->sa; -struct rte_eth_link *dev_link = &sa->eth_dev->data->dev_link;struct rte_eth_link new_link; -uint64_t new_link_u64; -uint64_t old_link_u64; - -EFX_STATIC_ASSERT (sizeof (*dev_link) ==sizeof (rte_atomic64_t));sfc_port_link_mode_to_info (link_mode, &new_link); +if (_rte_eth_linkstatus_set (sa->eth_dev, &new_link) ==0 ) +return B_FALSE ; - new_link_u64 = *(uint64_t *)&new_link; - do { - old_link_u64 = rte_atomic64_read((rte_atomic64_t *)dev_link); - if (old_link_u64 == new_link_u64) - break; -
drivers/net/sfc/sfc_ev.c sfc_ev_link_change(void *arg, efx_link_mode_t link_mode) · line 414
Quoted 10 lines
drivers/net/sfc/sfc_ev.c +2
-8-if (rte_atomic64_cmpset ((volatile uint64_t *)dev_link, - old_link_u64, new_link_u64)) { - evq->sa->port.lsc_seq++; -break ; - } - }while (B_TRUE ); - -return B_FALSE ; + evq->sa->port.lsc_seq++; +return B_TRUE ;
It still returns B_TRUE, but should return B_FALSE as before. Also before the patch lsc_seq is incremented in the case of any changes in link status, but now in the case of up/down change only.
Quoted 3 lines
drivers/net/sfc/sfc_ev.c +0
-0}static const efx_ev_callbacks_t sfc_ev_callbacks = {
Re: [PATCH v3 09/15] net/sfc: use new rte_eth_linkstatus functions
Quoted 96 lines · On Tue, 9 Jan 2018 13:35:58 +0300 Andrew Rybchenko <[email protected]>…
Quoted 95 lines · Stephen Hemminger
Use the new API (
_rte_eth_linkstatus_set) to handle link status update.Signed-off-by Stephen Hemminger <[email protected]>--- drivers/net/sfc/sfc_ethdev.c | 27 +++++++-------------------- drivers/net/sfc/sfc_ev.c | 23 ++++------------------- 2 files changed, 11 insertions(+), 39 deletions(-)drivers/net/sfc/sfc_ethdev.c +7
-20@@ -238,22 +238,12 @@ static int sfc_dev_link_update(struct rte_eth_dev *dev, int wait_to_complete) {struct sfc_adapter *sa = dev->data->dev_private; -struct rte_eth_link *dev_link = &dev->data->dev_link; -struct rte_eth_link old_link;struct rte_eth_link current_link;sfc_log_init (sa,"entry" ); -retry: -EFX_STATIC_ASSERT (sizeof (*dev_link) ==sizeof (rte_atomic64_t)); - *(int64_t *)&old_link =rte_atomic64_read ((rte_atomic64_t *)dev_link); -if (sa->state !=SFC_ADAPTER_STARTED ) {sfc_port_link_mode_to_info (EFX_LINK_UNKNOWN , ¤t_link); -if (!rte_atomic64_cmpset ((volatile uint64_t *)dev_link, - *(uint64_t *)&old_link, - *(uint64_t *)¤t_link)) - goto retry; }else if (wait_to_complete) {efx_link_mode_t link_mode;@@ -261,21 +251,18 @@ sfc_dev_link_update(struct rte_eth_dev *dev, int wait_to_complete) link_mode =EFX_LINK_UNKNOWN ;sfc_port_link_mode_to_info (link_mode, ¤t_link); -if (!rte_atomic64_cmpset ((volatile uint64_t *)dev_link, - *(uint64_t *)&old_link, - *(uint64_t *)¤t_link)) - goto retry; }else {sfc_ev_mgmt_qpoll (sa); - *(int64_t *)¤t_link = -rte_atomic64_read ((rte_atomic64_t *)dev_link); +_rte_eth_linkstatus_get (dev, ¤t_link); } -if (old_link.link_status != current_link.link_status) -sfc_info (sa,"Link status is %s" , - current_link.link_status ?"UP" :"DOWN" ); +if (_rte_eth_linkstatus_set (dev, ¤t_link) ==0 ) +return 0 ; + +sfc_info (sa,"Link status is %s" , + current_link.link_status ?"UP" :"DOWN" ); -return old_link.link_status == current_link.link_status ?0 :-1 ; +return -1 ; } static voiddrivers/net/sfc/sfc_ev.c +2
-17@@ -404,29 +404,14 @@ sfc_ev_link_change(void *arg, efx_link_mode_t link_mode) {struct sfc_evq *evq = arg;struct sfc_adapter *sa = evq->sa; -struct rte_eth_link *dev_link = &sa->eth_dev->data->dev_link;struct rte_eth_link new_link; -uint64_t new_link_u64; -uint64_t old_link_u64; - -EFX_STATIC_ASSERT (sizeof (*dev_link) ==sizeof (rte_atomic64_t));sfc_port_link_mode_to_info (link_mode, &new_link); +if (_rte_eth_linkstatus_set (sa->eth_dev, &new_link) ==0 ) +return B_FALSE ; - new_link_u64 = *(uint64_t *)&new_link; -do { - old_link_u64 =rte_atomic64_read ((rte_atomic64_t *)dev_link); -if (old_link_u64 == new_link_u64) -break ; - -if (rte_atomic64_cmpset ((volatile uint64_t *)dev_link, - old_link_u64, new_link_u64)) { - evq->sa->port.lsc_seq++; -break ; - } - }while (B_TRUE );
Quoted 8 lines
drivers/net/sfc/sfc_ev.c sfc_ev_link_change(void *arg, efx_link_mode_t link_mode) · line 414
Quoted 4 lines
drivers/net/sfc/sfc_ev.c +2
-2- -return B_FALSE ; + evq->sa->port.lsc_seq++; +return B_TRUE ;It still returns
B_TRUE, but should returnB_FALSEas before. Also before the patchlsc_seqis incremented in the case of any changes in link status, but now in the case of up/down change only.
The old code looked broken and did not match the comments. It always returned B_FALSE independent of whether link status changed or not.
Re: [PATCH v3 09/15] net/sfc: use new rte_eth_linkstatus functions
Quoted 106 lines · Stephen Hemminger
Quoted 95 lines · On Tue, 9 Jan 2018 13:35:58 +0300 Andrew Rybchenko <[email protected]>…
Quoted 94 lines · Stephen Hemminger
Use the new API (
_rte_eth_linkstatus_set) to handle link status update.Signed-off-by Stephen Hemminger <[email protected]>--- drivers/net/sfc/sfc_ethdev.c | 27 +++++++-------------------- drivers/net/sfc/sfc_ev.c | 23 ++++------------------- 2 files changed, 11 insertions(+), 39 deletions(-)drivers/net/sfc/sfc_ethdev.c +7
-20@@ -238,22 +238,12 @@ static int sfc_dev_link_update(struct rte_eth_dev *dev, int wait_to_complete) {struct sfc_adapter *sa = dev->data->dev_private; -struct rte_eth_link *dev_link = &dev->data->dev_link; -struct rte_eth_link old_link;struct rte_eth_link current_link;sfc_log_init (sa,"entry" ); -retry: -EFX_STATIC_ASSERT (sizeof (*dev_link) ==sizeof (rte_atomic64_t)); - *(int64_t *)&old_link =rte_atomic64_read ((rte_atomic64_t *)dev_link); -if (sa->state !=SFC_ADAPTER_STARTED ) {sfc_port_link_mode_to_info (EFX_LINK_UNKNOWN , ¤t_link); -if (!rte_atomic64_cmpset ((volatile uint64_t *)dev_link, - *(uint64_t *)&old_link, - *(uint64_t *)¤t_link)) - goto retry; }else if (wait_to_complete) {efx_link_mode_t link_mode;@@ -261,21 +251,18 @@ sfc_dev_link_update(struct rte_eth_dev *dev, int wait_to_complete) link_mode =EFX_LINK_UNKNOWN ;sfc_port_link_mode_to_info (link_mode, ¤t_link); -if (!rte_atomic64_cmpset ((volatile uint64_t *)dev_link, - *(uint64_t *)&old_link, - *(uint64_t *)¤t_link)) - goto retry; }else {sfc_ev_mgmt_qpoll (sa); - *(int64_t *)¤t_link = -rte_atomic64_read ((rte_atomic64_t *)dev_link); +_rte_eth_linkstatus_get (dev, ¤t_link); } -if (old_link.link_status != current_link.link_status) -sfc_info (sa,"Link status is %s" , - current_link.link_status ?"UP" :"DOWN" ); +if (_rte_eth_linkstatus_set (dev, ¤t_link) ==0 ) +return 0 ; + +sfc_info (sa,"Link status is %s" , + current_link.link_status ?"UP" :"DOWN" ); -return old_link.link_status == current_link.link_status ?0 :-1 ; +return -1 ; } static voiddrivers/net/sfc/sfc_ev.c +2
-16@@ -404,29 +404,14 @@ sfc_ev_link_change(void *arg, efx_link_mode_t link_mode) {struct sfc_evq *evq = arg;struct sfc_adapter *sa = evq->sa; -struct rte_eth_link *dev_link = &sa->eth_dev->data->dev_link;struct rte_eth_link new_link; -uint64_t new_link_u64; -uint64_t old_link_u64; - -EFX_STATIC_ASSERT (sizeof (*dev_link) ==sizeof (rte_atomic64_t));sfc_port_link_mode_to_info (link_mode, &new_link); +if (_rte_eth_linkstatus_set (sa->eth_dev, &new_link) ==0 ) +return B_FALSE ; - new_link_u64 = *(uint64_t *)&new_link; - do { - old_link_u64 = rte_atomic64_read((rte_atomic64_t *)dev_link); - if (old_link_u64 == new_link_u64) - break; - - if (rte_atomic64_cmpset((volatile uint64_t *)dev_link, - old_link_u64, new_link_u64)) { - evq->sa->port.lsc_seq++; - break; - }Quoted 8 lines
drivers/net/sfc/sfc_ev.c sfc_ev_link_change(void *arg, efx_link_mode_t link_mode) · line 414
Quoted 5 lines
drivers/net/sfc/sfc_ev.c +2
-3- }while (B_TRUE ); - -return B_FALSE ; + evq->sa->port.lsc_seq++; +return B_TRUE ;It still returns
B_TRUE, but should returnB_FALSEas before. Also before the patchlsc_seqis incremented in the case of any changes in link status, but now in the case of up/down change only.
Quoted 3 lines
The old code looked broken and did not match the comments. It always returned
B_FALSEindependent of whether link status changed or not.
Which comments does it not match?
Yes, because it is internal callback and return value is unrelated to link status changes. Return value affects further events processing. If B_TRUE is returned, it means something bad has happened and events processing should be aborted.
PATCH v3 10/15 net/i40e: use rte_eth_linkstatus functions
In reply to Stephen HemmingerUse new rte_linkstatus update API
Signed-off-by Stephen Hemminger <[email protected]>
--- drivers/net/i40e/i40e_ethdev.c | 44 ++++++--------------------------------- drivers/net/i40e/i40e_ethdev_vf.c | 19 ++--------------- 2 files changed, 8 insertions(+), 55 deletions(-)
drivers/net/i40e/i40e_ethdev.c +6 -38
@@ -627,34 +627,6 @@ static struct rte_pci_driver rte_i40e_pmd = { .remove = eth_i40e_pci_remove, }; -static inline int -rte_i40e_dev_atomic_read_link_status (struct rte_eth_dev *dev, -struct rte_eth_link *link) -{ -struct rte_eth_link *dst = link; -struct rte_eth_link *src = &(dev->data->dev_link); - -if (rte_atomic64_cmpset ((uint64_t *)dst, *(uint64_t *)dst, - *(uint64_t *)src) ==0 ) -return -1 ; - -return 0 ; -} - -static inline int -rte_i40e_dev_atomic_write_link_status (struct rte_eth_dev *dev, -struct rte_eth_link *link) -{ -struct rte_eth_link *dst = &(dev->data->dev_link); -struct rte_eth_link *src = link; - -if (rte_atomic64_cmpset ((uint64_t *)dst, *(uint64_t *)dst, - *(uint64_t *)src) ==0 ) -return -1 ; - -return 0 ; -} -RTE_PMD_REGISTER_PCI (net_i40e, rte_i40e_pmd);RTE_PMD_REGISTER_PCI_TABLE (net_i40e, pci_id_i40e_map);RTE_PMD_REGISTER_KMOD_DEP (net_i40e,"* igb_uio | uio_pci_generic | vfio-pci" );
@@ -2327,17 +2299,16 @@ i40e_dev_link_update(struct rte_eth_dev *dev,#define MAX_REPEAT_TIME 10/* 1s (10 * 100ms) in total */ struct i40e_hw *hw =I40E_DEV_PRIVATE_TO_HW (dev->data->dev_private);struct i40e_link_status link_status; -struct rte_eth_link link, old; +struct rte_eth_link link;int status;unsigned rep_cnt =MAX_REPEAT_TIME ;bool enable_lse = dev->data->dev_conf.intr_conf.lsc ? true : false;memset (&link,0 ,sizeof (link)); -memset (&old,0 ,sizeof (old)); -memset (&link_status,0 ,sizeof (link_status)); -rte_i40e_dev_atomic_read_link_status (dev, &old); do { + memset(&link_status, 0, sizeof(link_status)); + /* Get link status information from hardware */ status = i40e_aq_get_link_info(hw, enable_lse, &link_status, NULL);
@@ -2390,13 +2361,10 @@ i40e_dev_link_update(struct rte_eth_dev *dev, ETH_LINK_SPEED_FIXED); out: -rte_i40e_dev_atomic_write_link_status (dev, &link); -if (link.link_status == old.link_status) -return -1 ; - + status =_rte_eth_linkstatus_set (dev, &link);i40e_notify_all_vfs_link_status (dev); -return 0 ; +return status; }/* Get all the statistics of a VSI */
@@ -9854,7 +9822,7 @@ i40e_start_timecounters(struct rte_eth_dev *dev)/* Get current link speed. */ memset (&link,0 ,sizeof (link));i40e_dev_link_update (dev,1 ); -rte_i40e_dev_atomic_read_link_status (dev, &link); +_rte_eth_linkstatus_get (dev, &link);switch (link.link_speed) {case ETH_SPEED_NUM_40G :
drivers/net/i40e/i40e_ethdev_vf.c +2 -17
@@ -1034,20 +1034,6 @@ static const struct rte_pci_id pci_id_i40evf_map[] = { { .vendor_id = 0, /* sentinel */ }, }; -static inline int -i40evf_dev_atomic_write_link_status (struct rte_eth_dev *dev, -struct rte_eth_link *link) -{ -struct rte_eth_link *dst = &(dev->data->dev_link); -struct rte_eth_link *src = link; - -if (rte_atomic64_cmpset ((uint64_t *)dst, *(uint64_t *)dst, - *(uint64_t *)src) ==0 ) -return -1 ; - -return 0 ; -} -/* Disable IRQ0 */ static inline void i40evf_disable_irq0 (struct i40e_hw *hw)
@@ -2069,6 +2055,7 @@ i40evf_dev_link_update(struct rte_eth_dev *dev,* while Linux driver does not */ +memset (&new_link,0 ,sizeof (new_link));/* Linux driver PF host */ switch (vf->link_speed) {case I40E_LINK_SPEED_100MB :
@@ -2100,9 +2087,7 @@ i40evf_dev_link_update(struct rte_eth_dev *dev, new_link.link_autoneg = dev->data->dev_conf.link_speeds Ð_LINK_SPEED_FIXED ; -i40evf_dev_atomic_write_link_status (dev, &new_link); - -return 0 ; +return _rte_eth_linkstatus_set (dev, &new_link); } static void
Signature · 1 line
2.15.1
PATCH v3 11/15 net/liquidio: use _rte_eth_linkstatus_set
In reply to Stephen HemmingerUse the new link update API, and cleanup the logic in the the link update routine.
Tested-by Shijith Thotton <[email protected]>
Signed-off-by Stephen Hemminger <[email protected]>
--- drivers/net/liquidio/lio_ethdev.c | 54 ++++++--------------------------------- 1 file changed, 8 insertions(+), 46 deletions(-)
drivers/net/liquidio/lio_ethdev.c +8 -46
@@ -930,32 +930,6 @@ lio_dev_vlan_filter_set(struct rte_eth_dev *eth_dev, uint16_t vlan_id, int on)return 0 ; } -/** -* Atomically writes the link status information into global -* structure rte_eth_dev. -* -* @param eth_dev -* - Pointer to the structure rte_eth_dev to read from. -* - Pointer to the buffer to be saved with the link status. -* -* @return -* - On success, zero. -* - On failure, negative value. -*/ -static inline int -lio_dev_atomic_write_link_status (struct rte_eth_dev *eth_dev, -struct rte_eth_link *link) -{ -struct rte_eth_link *dst = ð_dev->data->dev_link; -struct rte_eth_link *src = link; - -if (rte_atomic64_cmpset ((uint64_t *)dst, *(uint64_t *)dst, - *(uint64_t *)src) ==0 ) -return -1 ; - -return 0 ; -} -static uint64_t lio_hweight64 (uint64_t w) {
@@ -975,23 +949,17 @@ lio_dev_link_update(struct rte_eth_dev *eth_dev, int wait_to_complete __rte_unused) {struct lio_device *lio_dev =LIO_DEV (eth_dev); -struct rte_eth_link link, old; - -/* Initialize */ - link.link_status =ETH_LINK_DOWN ; - link.link_speed =ETH_SPEED_NUM_NONE ; - link.link_duplex =ETH_LINK_HALF_DUPLEX ; - link.link_autoneg =ETH_LINK_AUTONEG ; -memset (&old,0 ,sizeof (old)); +struct rte_eth_link link = { + .link_status =ETH_LINK_DOWN , + .link_speed =ETH_SPEED_NUM_NONE , + .link_duplex =ETH_LINK_HALF_DUPLEX , + .link_autoneg =ETH_LINK_AUTONEG , + };/* Return what we found */ if (lio_dev->linfo.link.s.link_up ==0 ) {/* Interface is down */ -if (lio_dev_atomic_write_link_status (eth_dev, &link)) -return -1 ; -if (link.link_status == old.link_status) -return -1 ; -return 0 ; +return _rte_eth_linkstatus_set (eth_dev, &link); } link.link_status =ETH_LINK_UP ;/* Interface is up */
@@ -1008,13 +976,7 @@ lio_dev_link_update(struct rte_eth_dev *eth_dev, link.link_duplex =ETH_LINK_HALF_DUPLEX ; } -if (lio_dev_atomic_write_link_status (eth_dev, &link)) -return -1 ; - -if (link.link_status == old.link_status) -return -1 ; - -return 0 ; +return _rte_eth_linkstatus_set (eth_dev, &link); }/**
Signature · 1 line
2.15.1
PATCH v3 12/15 net/thunderx: use _rte_eth_linkstatus_set
In reply to Stephen HemmingerUse new helper function.
Signed-off-by Stephen Hemminger <[email protected]>
--- drivers/net/thunderx/nicvf_ethdev.c | 17 ++--------------- 1 file changed, 2 insertions(+), 15 deletions(-)
drivers/net/thunderx/nicvf_ethdev.c +2 -15
@@ -43,7 +43,6 @@#include <sys/queue.h> #include <rte_alarm.h> -#include <rte_atomic.h> #include <rte_branch_prediction.h> #include <rte_byteorder.h> #include <rte_common.h>
@@ -76,19 +75,6 @@ static void nicvf_dev_stop_cleanup(struct rte_eth_dev *dev, bool cleanup);static void nicvf_vf_stop (struct rte_eth_dev *dev,struct nicvf *nic,bool cleanup); -static inline int -nicvf_atomic_write_link_status (struct rte_eth_dev *dev, -struct rte_eth_link *link) -{ -struct rte_eth_link *dst = &dev->data->dev_link; -struct rte_eth_link *src = link; - -if (rte_atomic64_cmpset ((uint64_t *)dst, *(uint64_t *)dst, - *(uint64_t *)src) ==0 ) -return -1 ; - -return 0 ; -}static inline void nicvf_set_eth_link_status (struct nicvf *nic,struct rte_eth_link *link)
@@ -170,7 +156,8 @@ nicvf_dev_link_update(struct rte_eth_dev *dev, int wait_to_complete)memset (&link,0 ,sizeof (link));nicvf_set_eth_link_status (nic, &link); } -return nicvf_atomic_write_link_status (dev, &link); + +return _rte_eth_linkstatus_set (dev, &link); } static int
Signature · 1 line
2.15.1
PATCH v3 13/15 net/szedata: use _rte_eth_linkstatus_set
In reply to Stephen HemmingerYet another driver which was not returing correct value on link change.
Since this driver can't be built on x86 could not even do a compile test.
Signed-off-by Stephen Hemminger <[email protected]>
--- drivers/net/szedata2/rte_eth_szedata2.c | 20 ++++++-------------- 1 file changed, 6 insertions(+), 14 deletions(-)
drivers/net/szedata2/rte_eth_szedata2.c +6 -14
@@ -50,7 +50,6 @@#include <rte_memcpy.h> #include <rte_kvargs.h> #include <rte_dev.h> -#include <rte_atomic.h> #include "rte_eth_szedata2.h" #include "szedata2_iobuf.h"
@@ -1173,14 +1172,15 @@ static int eth_link_update(struct rte_eth_dev *dev, int wait_to_complete __rte_unused) { - struct rte_eth_link link; -struct rte_eth_link *link_ptr = &link; -struct rte_eth_link *dev_link = &dev->data->dev_link; struct pmd_internals *internals = (struct pmd_internals *) dev->data->dev_private;const volatile struct szedata2_ibuf *ibuf;uint32_t i;bool link_is_up = false; +struct rte_eth_link link = { + .link_duplex =ETH_LINK_FULL_DUPLEX , + .link_autoneg =ETH_LINK_SPEED_FIXED , + };switch (get_link_speed (internals)) {case SZEDATA2_LINK_SPEED_10G :
@@ -1197,9 +1197,6 @@ eth_link_update(struct rte_eth_dev *dev,break ; } -/* szedata2 uses only full duplex */ - link.link_duplex =ETH_LINK_FULL_DUPLEX ; -for (i =0 ; i < szedata2_ibuf_count; i++) { ibuf =ibuf_ptr_by_index (internals->pci_rsc, i);/*
@@ -1212,14 +1209,9 @@ eth_link_update(struct rte_eth_dev *dev, } } - link.link_status = (link_is_up) ?ETH_LINK_UP :ETH_LINK_DOWN ; - - link.link_autoneg =ETH_LINK_SPEED_FIXED ; + link.link_status = link_is_up ?ETH_LINK_UP :ETH_LINK_DOWN ; -rte_atomic64_cmpset ((uint64_t *)dev_link, *(uint64_t *)dev_link, - *(uint64_t *)link_ptr); - -return 0 ; +return _rte_eth_linkstatus_set (dev, &link); } static int
Signature · 1 line
2.15.1
PATCH v3 14/15 net/octeontx: use rte_eth_linkstatus_set
In reply to Stephen HemmingerCommon function matches this drivers usage.
Signed-off-by Stephen Hemminger <[email protected]>
--- drivers/net/octeontx/octeontx_ethdev.c | 16 +--------------- 1 file changed, 1 insertion(+), 15 deletions(-)
drivers/net/octeontx/octeontx_ethdev.c +1 -15
@@ -490,20 +490,6 @@ octeontx_dev_promisc_disable(struct rte_eth_dev *dev)octeontx_port_promisc_set (nic,0 ); } -static inline int -octeontx_atomic_write_link_status (struct rte_eth_dev *dev, -struct rte_eth_link *link) -{ -struct rte_eth_link *dst = &dev->data->dev_link; -struct rte_eth_link *src = link; - -if (rte_atomic64_cmpset ((uint64_t *)dst, *(uint64_t *)dst, - *(uint64_t *)src) ==0 ) -return -1 ; - -return 0 ; -} -static int octeontx_port_link_status (struct octeontx_nic *nic) {
@@ -575,7 +561,7 @@ octeontx_dev_link_update(struct rte_eth_dev *dev, link.link_duplex =ETH_LINK_AUTONEG ; link.link_autoneg =ETH_LINK_SPEED_AUTONEG ; -return octeontx_atomic_write_link_status (dev, &link); +return _rte_eth_linkstatus_set (dev, &link); } static int
Signature · 1 line
2.15.1
PATCH v3 15/15 net/enic: use _rte_eth_linkstatus_set
In reply to Stephen HemmingerThis driver was not doing atomic update of link status information. And the return value was different than others.
The hardware also does not do autonegotiation (at least on Linux).
Signed-off-by Stephen Hemminger <[email protected]>
--- drivers/net/enic/enic_ethdev.c | 5 ++--- drivers/net/enic/enic_main.c | 16 +++++++--------- 2 files changed, 9 insertions(+), 12 deletions(-)
drivers/net/enic/enic_ethdev.c +2 -3
@@ -446,10 +446,9 @@ static void enicpmd_dev_stop(struct rte_eth_dev *eth_dev)ENICPMD_FUNC_TRACE ();enic_disable (enic); +memset (&link,0 ,sizeof (link)); -rte_atomic64_cmpset ((uint64_t *)ð_dev->data->dev_link, - *(uint64_t *)ð_dev->data->dev_link, - *(uint64_t *)&link); +_rte_eth_linkstatus_set (eth_dev, &link); }/*
drivers/net/enic/enic_main.c +7 -9
@@ -413,16 +413,14 @@ enic_free_consistent(void *priv,int enic_link_update (struct enic *enic) {struct rte_eth_dev *eth_dev = enic->rte_dev; -int ret; -int link_status =0 ; +struct rte_eth_link link = { + .link_status =enic_get_link_status (enic), + .link_duplex =ETH_LINK_FULL_DUPLEX , + .link_speed =vnic_dev_port_speed (enic->vdev), + .link_autoneg =ETH_LINK_FIXED , + }; - link_status =enic_get_link_status (enic); - ret = (link_status == enic->link_status); - enic->link_status = link_status; - eth_dev->data->dev_link.link_status = link_status; - eth_dev->data->dev_link.link_duplex =ETH_LINK_FULL_DUPLEX ; - eth_dev->data->dev_link.link_speed =vnic_dev_port_speed (enic->vdev); -return ret; +return _rte_eth_linkstatus_set (eth_dev, &link); } static void
Signature · 1 line
2.15.1
PATCH v4 1/15 eal: introduce atomic exchange operation
In reply to Stephen HemmingerFrom: Stephen Hemminger <[email protected]>
To handle atomic update of link status (64 bit), every driver was doing its own version using cmpset.
Atomic exchange is a useful primitive in its own right; therefore make it a EAL routine.
Signed-off-by Stephen Hemminger <[email protected]>
--- .../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 ++++++++++++++++++++++ 4 files changed, 126 insertions(+)
lib/librte_eal/common/include/arch/x86/rte_atomic.h +24 -0
@@ -59,6 +59,18 @@ rte_atomic16_cmpset(volatile uint16_t *dst, uint16_t exp, uint16_t src)return res; } +static inline uint16_t +rte_atomic16_exchange (volatile uint16_t *dst,uint16_t val) +{ + asmvolatile ( +MPLOCKED +"xchgw %0, %1;" + :"=r" (val),"=m" (*dst) + :"0" (val),"m" (*dst) + :"memory" );/* no-clobber list */ +return val; +} +static inline int rte_atomic16_test_and_set (rte_atomic16_t *v) {return rte_atomic16_cmpset ((volatile uint16_t *)&v->cnt,0 ,1 );
@@ -133,6 +145,18 @@ rte_atomic32_cmpset(volatile uint32_t *dst, uint32_t exp, uint32_t src)return res; } +static inline uint32_t +rte_atomic32_exchange (volatile uint32_t *dst,uint32_t val) +{ + asmvolatile ( +MPLOCKED +"xchgl %0, %1;" + :"=r" (val),"=m" (*dst) + :"0" (val),"m" (*dst) + :"memory" );/* no-clobber list */ +return val; +} +static inline int rte_atomic32_test_and_set (rte_atomic32_t *v) {return rte_atomic32_cmpset ((volatile uint32_t *)&v->cnt,0 ,1 );
lib/librte_eal/common/include/arch/x86/rte_atomic_32.h +12 -0
@@ -98,6 +98,18 @@ rte_atomic64_cmpset(volatile uint64_t *dst, uint64_t exp, uint64_t src)return res; } +static inline uint64_t +rte_atomic64_exchange (volatile uint64_t *dest,uint64_t val) +{ +uint64_t old; + +do { + old = *dest; + }while (rte_atomic64_cmpset (dest, old, val)); + +return old; +} +static inline void rte_atomic64_init (rte_atomic64_t *v) {
lib/librte_eal/common/include/arch/x86/rte_atomic_64.h +12 -0
@@ -71,6 +71,18 @@ rte_atomic64_cmpset(volatile uint64_t *dst, uint64_t exp, uint64_t src)return res; } +static inline uint64_t +rte_atomic64_exchange (volatile uint64_t *dst,uint64_t val) +{ + asmvolatile ( +MPLOCKED +"xchgq %0, %1;" + :"=r" (val),"=m" (*dst) + :"0" (val),"m" (*dst) + :"memory" );/* no-clobber list */ +return val; +} +static inline void rte_atomic64_init (rte_atomic64_t *v) {
lib/librte_eal/common/include/generic/rte_atomic.h +78 -0
96 lines not on this page · raw message
Signature · 1 line
2.14.3
PATCH v4 2/15 ethdev: add linkstatus get/set helper functions
From: Stephen Hemminger <[email protected]>
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 <[email protected]>
--- lib/librte_ether/rte_ethdev.c | 36 +++++++++++++++++++++++++++++++++ lib/librte_ether/rte_ethdev.h | 28 +++++++++++++++++++++++++ lib/librte_ether/rte_ethdev_version.map | 8 ++++++++ 3 files changed, 72 insertions(+)
lib/librte_ether/rte_ethdev.c +36 -0
@@ -1500,6 +1500,42 @@ rte_eth_link_get_nowait(uint16_t port_id, struct rte_eth_link *eth_link) } } +int +_rte_eth_linkstatus_set (struct rte_eth_dev *dev, +const struct rte_eth_link *new_link) +{ +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 ; +} + +void +_rte_eth_linkstatus_get (const struct rte_eth_dev *dev, +struct rte_eth_link *link) +{ +volatile uint64_t *src = (uint64_t *)&(dev->data->dev_link); +uint64_t tmp; + +RTE_BUILD_BUG_ON (sizeof (*link) !=sizeof (uint64_t )); + +/* can't use rte_atomic64_read because it returns signed int */ +do { + tmp = *src; + }while (!rte_atomic64_cmpset (src, tmp, tmp)); + +memcpy (link, &tmp,sizeof (tmp)); +} +int rte_eth_stats_get (uint16_t port_id,struct rte_eth_stats *stats) {
lib/librte_ether/rte_ethdev.h +28 -0
@@ -2220,6 +2220,34 @@ int rte_eth_dev_set_link_down(uint16_t port_id);*/ void rte_eth_dev_close (uint16_t port_id); + +/** +* @internal +* Atomically set the link status for the specific device. +* It is for use by DPDK device driver use only. +* User applications should not call it +* +* @param dev +* Pointer to struct rte_eth_dev. +* @param link +* New link status value. +* @return +* 0 if link state has changed, -1 if the same. +*/ +int _rte_eth_linkstatus_set (struct rte_eth_dev *dev, +const struct rte_eth_link *link); + +/** +* @internal +* Atomically get the link speed and status. +* @param dev +* Pointer to struct rte_eth_dev. +* @param link +* link status value. +*/ +void _rte_eth_linkstatus_get (const struct rte_eth_dev *dev, +struct rte_eth_link *link); +/** * Reset a Ethernet device and keep its port id. *
lib/librte_ether/rte_ethdev_version.map +8 -0
@@ -198,6 +198,14 @@ DPDK_17.11 { } DPDK_17.08; +DPDK_18.02 { + global: + + _rte_eth_linkstatus_get; + _rte_eth_linkstatus_set; + +} DPDK_17.11; + EXPERIMENTAL { global:
Signature · 1 line
2.14.3
PATCH v4 3/15 net/virtio: use eth_linkstatus_set
In reply to Ferruh YigitFrom: Stephen Hemminger <[email protected]>
Use the new comon code in ethdev to handle link status update.
Signed-off-by Stephen Hemminger <[email protected]>
--- drivers/net/virtio/virtio_ethdev.c | 63 ++++++-------------------------------- 1 file changed, 10 insertions(+), 53 deletions(-)
drivers/net/virtio/virtio_ethdev.c +10 -53
@@ -14,7 +14,6 @@#include <rte_string_fns.h> #include <rte_memzone.h> #include <rte_malloc.h> -#include <rte_atomic.h> #include <rte_branch_prediction.h> #include <rte_pci.h> #include <rte_bus_pci.h>
@@ -774,46 +773,6 @@ static const struct eth_dev_ops virtio_eth_dev_ops = { .mac_addr_set = virtio_mac_addr_set, }; -static inline int -virtio_dev_atomic_read_link_status (struct rte_eth_dev *dev, -struct rte_eth_link *link) -{ -struct rte_eth_link *dst = link; -struct rte_eth_link *src = &(dev->data->dev_link); - -if (rte_atomic64_cmpset ((uint64_t *)dst, *(uint64_t *)dst, - *(uint64_t *)src) ==0 ) -return -1 ; - -return 0 ; -} - -/** -* Atomically writes the link status information into global -* structure rte_eth_dev. -* -* @param dev -* - Pointer to the structure rte_eth_dev to read from. -* - Pointer to the buffer to be saved with the link status. -* -* @return -* - On success, zero. -* - On failure, negative value. -*/ -static inline int -virtio_dev_atomic_write_link_status (struct rte_eth_dev *dev, -struct rte_eth_link *link) -{ -struct rte_eth_link *dst = &(dev->data->dev_link); -struct rte_eth_link *src = link; - -if (rte_atomic64_cmpset ((uint64_t *)dst, *(uint64_t *)dst, - *(uint64_t *)src) ==0 ) -return -1 ; - -return 0 ; -} -static void virtio_update_stats (struct rte_eth_dev *dev,struct rte_eth_stats *stats) {
@@ -1916,8 +1875,10 @@ static void virtio_dev_stop(struct rte_eth_dev *dev) {struct virtio_hw *hw = dev->data->dev_private; -struct rte_eth_link link;struct rte_intr_conf *intr_conf = &dev->data->dev_conf.intr_conf; +struct rte_eth_link link = { + .link_status =ETH_LINK_DOWN , + };PMD_INIT_LOG (DEBUG ,"stop" );
@@ -1925,21 +1886,18 @@ virtio_dev_stop(struct rte_eth_dev *dev)virtio_intr_disable (dev); hw->started =0 ; -memset (&link,0 ,sizeof (link)); -virtio_dev_atomic_write_link_status (dev, &link); +_rte_eth_linkstatus_set (dev, &link); }static int virtio_dev_link_update(struct rte_eth_dev *dev, __rte_unused int wait_to_complete) { -struct rte_eth_link link, old; -uint16_t status;struct virtio_hw *hw = dev->data->dev_private; -memset (&link,0 ,sizeof (link)); -virtio_dev_atomic_read_link_status (dev, &link); - old = link; - link.link_duplex =ETH_LINK_FULL_DUPLEX ; - link.link_speed =ETH_SPEED_NUM_10G ; +uint16_t status; +struct rte_eth_link link = { + .link_speed =ETH_SPEED_NUM_10G , + .link_duplex =ETH_LINK_FULL_DUPLEX , + };if (hw->started ==0 ) { link.link_status =ETH_LINK_DOWN ;
@@ -1960,9 +1918,8 @@ virtio_dev_link_update(struct rte_eth_dev *dev, __rte_unused int wait_to_complet }else { link.link_status =ETH_LINK_UP ; } -virtio_dev_atomic_write_link_status (dev, &link); -return (old.link_status == link.link_status) ?-1 :0 ; +return _rte_eth_linkstatus_set (dev, &link); } static int
Signature · 1 line
2.14.3
PATCH v4 4/15 net/vmxnet3: use rte_eth_linkstatus_set
In reply to Ferruh YigitFrom: Stephen Hemminger <[email protected]>
Use new _rte_eth_link_update helper.
Also remove no longer necessary include of rte_atomic.h
Signed-off-by Stephen Hemminger <[email protected]>
--- drivers/net/vmxnet3/vmxnet3_ethdev.c | 85 +++++++----------------------------- 1 file changed, 15 insertions(+), 70 deletions(-)
drivers/net/vmxnet3/vmxnet3_ethdev.c +15 -70
@@ -20,7 +20,6 @@#include <rte_debug.h> #include <rte_pci.h> #include <rte_bus_pci.h> -#include <rte_atomic.h> #include <rte_branch_prediction.h> #include <rte_memory.h> #include <rte_memzone.h>
@@ -160,59 +159,6 @@ gpa_zone_reserve(struct rte_eth_dev *dev, uint32_t size,return rte_memzone_reserve_aligned (z_name, size, socket_id,0 , align); } -/** -* Atomically reads the link status information from global -* structure rte_eth_dev. -* -* @param dev -* - Pointer to the structure rte_eth_dev to read from. -* - Pointer to the buffer to be saved with the link status. -* -* @return -* - On success, zero. -* - On failure, negative value. -*/ - -static int -vmxnet3_dev_atomic_read_link_status (struct rte_eth_dev *dev, -struct rte_eth_link *link) -{ -struct rte_eth_link *dst = link; -struct rte_eth_link *src = &(dev->data->dev_link); - -if (rte_atomic64_cmpset ((uint64_t *)dst, *(uint64_t *)dst, - *(uint64_t *)src) ==0 ) -return -1 ; - -return 0 ; -} - -/** -* Atomically writes the link status information into global -* structure rte_eth_dev. -* -* @param dev -* - Pointer to the structure rte_eth_dev to write to. -* - Pointer to the buffer to be saved with the link status. -* -* @return -* - On success, zero. -* - On failure, negative value. -*/ -static int -vmxnet3_dev_atomic_write_link_status (struct rte_eth_dev *dev, -struct rte_eth_link *link) -{ -struct rte_eth_link *dst = &(dev->data->dev_link); -struct rte_eth_link *src = link; - -if (rte_atomic64_cmpset ((uint64_t *)dst, *(uint64_t *)dst, - *(uint64_t *)src) ==0 ) -return -1 ; - -return 0 ; -} -/* * This function is based on vmxnet3_disable_intr() */
@@ -817,9 +763,13 @@ vmxnet3_dev_start(struct rte_eth_dev *dev)static void vmxnet3_dev_stop (struct rte_eth_dev *dev) { -struct rte_eth_link link;struct vmxnet3_hw *hw = dev->data->dev_private; - +struct rte_eth_link link = { + .link_speed =ETH_SPEED_NUM_10G , + .link_duplex =ETH_LINK_FULL_DUPLEX , + .link_autoneg =ETH_LINK_FIXED , + .link_status =ETH_LINK_DOWN , + };PMD_INIT_FUNC_TRACE ();if (hw->adapter_stopped ==1 ) {
@@ -852,8 +802,7 @@ vmxnet3_dev_stop(struct rte_eth_dev *dev)vmxnet3_dev_clear_queues (dev);/* Clear recorded link status */ -memset (&link,0 ,sizeof (link)); -vmxnet3_dev_atomic_write_link_status (dev, &link); +_rte_eth_linkstatus_set (dev, &link); }/*
@@ -1133,25 +1082,21 @@ __vmxnet3_dev_link_update(struct rte_eth_dev *dev, __rte_unused int wait_to_complete) {struct vmxnet3_hw *hw = dev->data->dev_private; -struct rte_eth_link old = {0 }, link; +struct rte_eth_link link = { + .link_speed =ETH_SPEED_NUM_10G , + .link_duplex =ETH_LINK_FULL_DUPLEX , + .link_autoneg =ETH_LINK_AUTONEG , + .link_status =ETH_LINK_UP , + };uint32_t ret; -memset (&link,0 ,sizeof (link)); -vmxnet3_dev_atomic_read_link_status (dev, &old); -VMXNET3_WRITE_BAR1_REG (hw,VMXNET3_REG_CMD ,VMXNET3_CMD_GET_LINK ); ret =VMXNET3_READ_BAR1_REG (hw,VMXNET3_REG_CMD ); -if (ret &0x1 ) { +if (ret &0x1 ) link.link_status =ETH_LINK_UP ; - link.link_duplex =ETH_LINK_FULL_DUPLEX ; - link.link_speed =ETH_SPEED_NUM_10G ; - link.link_autoneg =ETH_LINK_AUTONEG ; - } - -vmxnet3_dev_atomic_write_link_status (dev, &link); -return (old.link_status == link.link_status) ?-1 :0 ; +return _rte_eth_linkstatus_set (dev, &link); } static int
Signature · 1 line
2.14.3
PATCH v4 5/15 net/dpaa2: use rte_eth_linkstatus_set
In reply to Ferruh YigitFrom: Stephen Hemminger <[email protected]>
Use new helper function to update the link status.
As a good side effect this fixes a but because this driver was not returning correct status (should be -1 in link_status changed).
Signed-off-by Stephen Hemminger <[email protected]>
--- drivers/net/dpaa2/dpaa2_ethdev.c | 65 +++++----------------------------------- 1 file changed, 7 insertions(+), 58 deletions(-)
drivers/net/dpaa2/dpaa2_ethdev.c +7 -58
@@ -57,58 +57,6 @@ static int dpaa2_dev_set_link_up(struct rte_eth_dev *dev);static int dpaa2_dev_set_link_down (struct rte_eth_dev *dev);static int dpaa2_dev_mtu_set (struct rte_eth_dev *dev,uint16_t mtu); -/** -* Atomically reads the link status information from global -* structure rte_eth_dev. -* -* @param dev -* - Pointer to the structure rte_eth_dev to read from. -* - Pointer to the buffer to be saved with the link status. -* -* @return -* - On success, zero. -* - On failure, negative value. -*/ -static inline int -dpaa2_dev_atomic_read_link_status (struct rte_eth_dev *dev, -struct rte_eth_link *link) -{ -struct rte_eth_link *dst = link; -struct rte_eth_link *src = &dev->data->dev_link; - -if (rte_atomic64_cmpset ((uint64_t *)dst, *(uint64_t *)dst, - *(uint64_t *)src) ==0 ) -return -1 ; - -return 0 ; -} - -/** -* Atomically writes the link status information into global -* structure rte_eth_dev. -* -* @param dev -* - Pointer to the structure rte_eth_dev to read from. -* - Pointer to the buffer to be saved with the link status. -* -* @return -* - On success, zero. -* - On failure, negative value. -*/ -static inline int -dpaa2_dev_atomic_write_link_status (struct rte_eth_dev *dev, -struct rte_eth_link *link) -{ -struct rte_eth_link *dst = &dev->data->dev_link; -struct rte_eth_link *src = link; - -if (rte_atomic64_cmpset ((uint64_t *)dst, *(uint64_t *)dst, - *(uint64_t *)src) ==0 ) -return -1 ; - -return 0 ; -} -static int dpaa2_vlan_filter_set (struct rte_eth_dev *dev,uint16_t vlan_id,int on) {
@@ -837,7 +785,7 @@ dpaa2_dev_stop(struct rte_eth_dev *dev)/* clear the recorded link status */ memset (&link,0 ,sizeof (link)); -dpaa2_dev_atomic_write_link_status (dev, &link); +_rte_eth_linkstatus_set (dev, &link); } static void
@@ -869,7 +817,7 @@ dpaa2_dev_close(struct rte_eth_dev *dev) }memset (&link,0 ,sizeof (link)); -dpaa2_dev_atomic_write_link_status (dev, &link); +_rte_eth_linkstatus_set (dev, &link); } static void
@@ -1327,8 +1275,8 @@ dpaa2_dev_link_update(struct rte_eth_dev *dev,RTE_LOG (ERR ,PMD ,"dpni is NULL\n" );return 0 ; } -memset (&old,0 ,sizeof (old)); -dpaa2_dev_atomic_read_link_status (dev, &old); + +_rte_eth_linkstatus_get (dev, &old); ret =dpni_get_link_state (dpni,CMD_PRI_LOW , priv->token, &state);if (ret <0 ) {
@@ -1350,13 +1298,14 @@ dpaa2_dev_link_update(struct rte_eth_dev *dev,else link.link_duplex =ETH_LINK_FULL_DUPLEX ; -dpaa2_dev_atomic_write_link_status (dev, &link); + ret =_rte_eth_linkstatus_set (dev, &link);if (link.link_status)PMD_DRV_LOG (INFO ,"Port %d Link is Up\n" , dev->data->port_id);else PMD_DRV_LOG (INFO ,"Port %d Link is Down" , dev->data->port_id); -return 0 ; + +return ret; }/**
Signature · 1 line
2.14.3
Re: [PATCH v4 05/15] net/dpaa2: use rte_eth_linkstatus_set
Quoted 4 lines · Ferruh Yigit
From: Stephen Hemminger <[email protected]>
Use new helper function to update the link status.
As a good side effect this fixes a but because this driver was not
^^^^^^
needs rephrasingQuoted 6 lines
returning correct status (should be -1 in
link_statuschanged).Signed-off-by Stephen Hemminger <[email protected]>--- drivers/net/dpaa2/dpaa2_ethdev.c | 65 +++++----------------------------------- 1 file changed, 7 insertions(+), 58 deletions(-)
Other than the change in commit message highlighted above:
Acked-by Shreyansh Jain <[email protected]>
Thanks.
Re: [PATCH v4 05/15] net/dpaa2: use rte_eth_linkstatus_set
Quoted 16 lines · Shreyansh Jain
Quoted 4 lines · Ferruh Yigit
From: Stephen Hemminger <[email protected]>
Use new helper function to update the link status.
As a good side effect this fixes a but because this driver was not^^^^^^
needs rephrasingQuoted 7 lines
returning correct status (should be -1 in
link_statuschanged).Signed-off-by Stephen Hemminger <[email protected]>---
drivers/net/dpaa2/dpaa2_ethdev.c| 65
+++++-----------------------------------1 file changed, 7 insertions(+), 58 deletions(-)
Quoted 6 lines
Other than the change in commit message highlighted above:
Acked-by Shreyansh Jain <[email protected]>Thanks.
Also, I forgot to add in previous email, I am assuming _rte_eth_linkstatus_get would be rte_eth_linkstatus_get eventually.
I concur with your comments in [1] - it certainly would be better to have either @internal or _rte - and not both.
PATCH v4 6/15 net/nfp: use rte_eth_linkstatus functions
In reply to Ferruh YigitFrom: Stephen Hemminger <[email protected]>
Use new _rte_eth_linkstatus_get/set helper function.
Signed-off-by Stephen Hemminger <[email protected]>
--- drivers/net/nfp/nfp_net.c | 75 +++++------------------------------------------ 1 file changed, 8 insertions(+), 67 deletions(-)
drivers/net/nfp/nfp_net.c +8 -67
@@ -213,57 +213,6 @@ nn_cfg_writeq(struct nfp_net_hw *hw, int off, uint64_t val)nn_writeq (rte_cpu_to_le_64 (val), hw->ctrl_bar + off); } -/* -* Atomically reads link status information from global structure rte_eth_dev. -* -* @param dev -* - Pointer to the structure rte_eth_dev to read from. -* - Pointer to the buffer to be saved with the link status. -* -* @return -* - On success, zero. -* - On failure, negative value. -*/ -static inline int -nfp_net_dev_atomic_read_link_status (struct rte_eth_dev *dev, -struct rte_eth_link *link) -{ -struct rte_eth_link *dst = link; -struct rte_eth_link *src = &dev->data->dev_link; - -if (rte_atomic64_cmpset ((uint64_t *)dst, *(uint64_t *)dst, - *(uint64_t *)src) ==0 ) -return -1 ; - -return 0 ; -} - -/* -* Atomically writes the link status information into global -* structure rte_eth_dev. -* -* @param dev -* - Pointer to the structure rte_eth_dev to read from. -* - Pointer to the buffer to be saved with the link status. -* -* @return -* - On success, zero. -* - On failure, negative value. -*/ -static inline int -nfp_net_dev_atomic_write_link_status (struct rte_eth_dev *dev, -struct rte_eth_link *link) -{ -struct rte_eth_link *dst = &dev->data->dev_link; -struct rte_eth_link *src = link; - -if (rte_atomic64_cmpset ((uint64_t *)dst, *(uint64_t *)dst, - *(uint64_t *)src) ==0 ) -return -1 ; - -return 0 ; -} -static void nfp_net_rx_queue_release_mbufs (struct nfp_net_rxq *rxq) {
@@ -999,7 +948,7 @@ static int nfp_net_link_update(struct rte_eth_dev *dev, __rte_unused int wait_to_complete) { struct nfp_net_hw *hw; -struct rte_eth_link link, old; +struct rte_eth_link link;uint32_t nn_link_status; static const uint32_t ls_to_ethtool[] = {
@@ -1017,9 +966,6 @@ nfp_net_link_update(struct rte_eth_dev *dev, __rte_unused int wait_to_complete) hw =NFP_NET_DEV_PRIVATE_TO_HW (dev->data->dev_private); -memset (&old,0 ,sizeof (old)); -nfp_net_dev_atomic_read_link_status (dev, &old); - nn_link_status =nn_cfg_readl (hw,NFP_NET_CFG_STS );memset (&link,0 ,sizeof (struct rte_eth_link ));
@@ -1037,15 +983,13 @@ nfp_net_link_update(struct rte_eth_dev *dev, __rte_unused int wait_to_complete)else link.link_speed = ls_to_ethtool[nn_link_status]; -if (old.link_status != link.link_status) { -nfp_net_dev_atomic_write_link_status (dev, &link); -if (link.link_status) -PMD_DRV_LOG (INFO ,"NIC Link is Up\n" ); -else -PMD_DRV_LOG (INFO ,"NIC Link is Down\n" ); +if (_rte_eth_linkstatus_set (dev, &link) ==0 )return 0 ; - } +if (link.link_status) +PMD_DRV_LOG (INFO ,"NIC Link is Up\n" ); +else +PMD_DRV_LOG (INFO ,"NIC Link is Down\n" );return -1 ; }
@@ -1376,8 +1320,7 @@ nfp_net_dev_link_status_print(struct rte_eth_dev *dev)struct rte_pci_device *pci_dev =RTE_ETH_DEV_TO_PCI (dev);struct rte_eth_link link; -memset (&link,0 ,sizeof (link)); -nfp_net_dev_atomic_read_link_status (dev, &link); +_rte_eth_linkstatus_get (dev, &link);if (link.link_status) RTE_LOG(INFO, PMD, "Port %d: Link Up - speed %u Mbps - %s\n", dev->data->port_id, link.link_speed,
@@ -1430,9 +1373,7 @@ nfp_net_dev_interrupt_handler(void *param)PMD_DRV_LOG (DEBUG ,"We got a LSC interrupt!!!\n" ); -/* get the link status */ -memset (&link,0 ,sizeof (link)); -nfp_net_dev_atomic_read_link_status (dev, &link); +_rte_eth_linkstatus_get (dev, &link);nfp_net_link_update (dev,0 );
Signature · 1 line
2.14.3
PATCH v4 7/15 net/e1000: use rte_eth_linkstatus helpers
In reply to Ferruh YigitFrom: Stephen Hemminger <[email protected]>
Use new rte_eth_linkstatus_get/set API.
Signed-off-by Stephen Hemminger <[email protected]>
--- drivers/net/e1000/em_ethdev.c | 70 +++-------------------------------------- drivers/net/e1000/igb_ethdev.c | 71 +++--------------------------------------- 2 files changed, 10 insertions(+), 131 deletions(-)
drivers/net/e1000/em_ethdev.c +5 -65
@@ -20,7 +20,6 @@#include <rte_ethdev_pci.h> #include <rte_memory.h> #include <rte_eal.h> -#include <rte_atomic.h> #include <rte_malloc.h> #include <rte_dev.h>
@@ -196,57 +195,6 @@ static const struct eth_dev_ops eth_em_ops = { .txq_info_get = em_txq_info_get, }; -/** -* Atomically reads the link status information from global -* structure rte_eth_dev. -* -* @param dev -* - Pointer to the structure rte_eth_dev to read from. -* - Pointer to the buffer to be saved with the link status. -* -* @return -* - On success, zero. -* - On failure, negative value. -*/ -static inline int -rte_em_dev_atomic_read_link_status (struct rte_eth_dev *dev, -struct rte_eth_link *link) -{ -struct rte_eth_link *dst = link; -struct rte_eth_link *src = &(dev->data->dev_link); - -if (rte_atomic64_cmpset ((uint64_t *)dst, *(uint64_t *)dst, - *(uint64_t *)src) ==0 ) -return -1 ; - -return 0 ; -} - -/** -* Atomically writes the link status information into global -* structure rte_eth_dev. -* -* @param dev -* - Pointer to the structure rte_eth_dev to read from. -* - Pointer to the buffer to be saved with the link status. -* -* @return -* - On success, zero. -* - On failure, negative value. -*/ -static inline int -rte_em_dev_atomic_write_link_status (struct rte_eth_dev *dev, -struct rte_eth_link *link) -{ -struct rte_eth_link *dst = &(dev->data->dev_link); -struct rte_eth_link *src = link; - -if (rte_atomic64_cmpset ((uint64_t *)dst, *(uint64_t *)dst, - *(uint64_t *)src) ==0 ) -return -1 ; - -return 0 ; -}/** * eth_em_dev_is_ich8 - Check for ICH8 device
@@ -772,7 +720,7 @@ eth_em_stop(struct rte_eth_dev *dev)/* clear the recorded link status */ memset (&link,0 ,sizeof (link)); -rte_em_dev_atomic_write_link_status (dev, &link); +_rte_eth_linkstatus_set (dev, &link);if (!rte_intr_allow_others (intr_handle))/* resume to the default handler */
@@ -1130,7 +1078,7 @@ eth_em_link_update(struct rte_eth_dev *dev, int wait_to_complete) {struct e1000_hw *hw =E1000_DEV_PRIVATE_TO_HW (dev->data->dev_private); -struct rte_eth_link link, old; +struct rte_eth_link link;int link_check, count; link_check =0 ;
@@ -1165,8 +1113,6 @@ eth_em_link_update(struct rte_eth_dev *dev, int wait_to_complete)rte_delay_ms (EM_LINK_UPDATE_CHECK_INTERVAL ); }memset (&link,0 ,sizeof (link)); -rte_em_dev_atomic_read_link_status (dev, &link); - old = link;/* Now we check if a transition has happened */ if (link_check && (link.link_status ==ETH_LINK_DOWN )) {
@@ -1185,14 +1131,8 @@ eth_em_link_update(struct rte_eth_dev *dev, int wait_to_complete) link.link_status =ETH_LINK_DOWN ; link.link_autoneg =ETH_LINK_FIXED ; } -rte_em_dev_atomic_write_link_status (dev, &link); - -/* not changed */ -if (old.link_status == link.link_status) -return -1 ; -/* changed */ -return 0 ; +return _rte_eth_linkstatus_set (dev, &link); }/*
@@ -1602,8 +1542,8 @@ eth_em_interrupt_action(struct rte_eth_dev *dev,if (ret <0 )return 0 ; -memset (&link,0 ,sizeof (link)); -rte_em_dev_atomic_read_link_status (dev, &link); +_rte_eth_linkstatus_get (dev, &link); +if (link.link_status) { PMD_INIT_LOG(INFO, " Port %d: Link Up - speed %u Mbps - %s", dev->data->port_id, link.link_speed,
drivers/net/e1000/igb_ethdev.c +5 -66
120 lines not on this page · raw message
Signature · 1 line
2.14.3
PATCH v4 8/15 net/ixgbe: use rte_eth_linkstatus functions
In reply to Ferruh YigitFrom: Stephen Hemminger <[email protected]>
Use the new helper functions from eth_dev for
handling atomic link_info update.
Signed-off-by Stephen Hemminger <[email protected]>
--- drivers/net/ixgbe/ixgbe_ethdev.c | 99 +++++++--------------------------------- 1 file changed, 17 insertions(+), 82 deletions(-)
drivers/net/ixgbe/ixgbe_ethdev.c +17 -82
@@ -20,7 +20,6 @@#include <rte_debug.h> #include <rte_pci.h> #include <rte_bus_pci.h> -#include <rte_atomic.h> #include <rte_branch_prediction.h> #include <rte_memory.h> #include <rte_eal.h>
@@ -787,58 +786,6 @@ static const struct rte_ixgbe_xstats_name_off rte_ixgbevf_stats_strings[] = {#define IXGBEVF_NB_XSTATS (sizeof(rte_ixgbevf_stats_strings) / \ sizeof(rte_ixgbevf_stats_strings[0])) -/** -* Atomically reads the link status information from global -* structure rte_eth_dev. -* -* @param dev -* - Pointer to the structure rte_eth_dev to read from. -* - Pointer to the buffer to be saved with the link status. -* -* @return -* - On success, zero. -* - On failure, negative value. -*/ -static inline int -rte_ixgbe_dev_atomic_read_link_status (struct rte_eth_dev *dev, -struct rte_eth_link *link) -{ -struct rte_eth_link *dst = link; -struct rte_eth_link *src = &(dev->data->dev_link); - -if (rte_atomic64_cmpset ((uint64_t *)dst, *(uint64_t *)dst, - *(uint64_t *)src) ==0 ) -return -1 ; - -return 0 ; -} - -/** -* Atomically writes the link status information into global -* structure rte_eth_dev. -* -* @param dev -* - Pointer to the structure rte_eth_dev to read from. -* - Pointer to the buffer to be saved with the link status. -* -* @return -* - On success, zero. -* - On failure, negative value. -*/ -static inline int -rte_ixgbe_dev_atomic_write_link_status (struct rte_eth_dev *dev, -struct rte_eth_link *link) -{ -struct rte_eth_link *dst = &(dev->data->dev_link); -struct rte_eth_link *src = link; - -if (rte_atomic64_cmpset ((uint64_t *)dst, *(uint64_t *)dst, - *(uint64_t *)src) ==0 ) -return -1 ; - -return 0 ; -} -/* * This function is the same as ixgbe_is_sfp() in base/ixgbe.h. */
@@ -2760,7 +2707,7 @@ ixgbe_dev_stop(struct rte_eth_dev *dev)/* Clear recorded link status */ memset (&link,0 ,sizeof (link)); -rte_ixgbe_dev_atomic_write_link_status (dev, &link); +_rte_eth_linkstatus_set (dev, &link);if (!rte_intr_allow_others (intr_handle))/* resume to the default handler */
@@ -3945,7 +3892,6 @@ ixgbe_dev_link_update_share(struct rte_eth_dev *dev, int wait_to_complete, int vf) {struct ixgbe_hw *hw =IXGBE_DEV_PRIVATE_TO_HW (dev->data->dev_private); -struct rte_eth_link link, old;ixgbe_link_speed link_speed =IXGBE_LINK_SPEED_UNKNOWN ;struct ixgbe_interrupt *intr =IXGBE_DEV_PRIVATE_TO_INTR (dev->data->dev_private);
@@ -3954,13 +3900,11 @@ ixgbe_dev_link_update_share(struct rte_eth_dev *dev,u32 speed =0 ;int wait =1 ;bool autoneg = false; - - link.link_status =ETH_LINK_DOWN ; - link.link_speed =0 ; - link.link_duplex =ETH_LINK_HALF_DUPLEX ; - link.link_autoneg =ETH_LINK_AUTONEG ; -memset (&old,0 ,sizeof (old)); -rte_ixgbe_dev_atomic_read_link_status (dev, &old); +struct rte_eth_link link = { + .link_status =ETH_LINK_DOWN , + .link_duplex =ETH_LINK_HALF_DUPLEX , + .link_autoneg =ETH_LINK_AUTONEG , + }; hw->mac.get_link_status = true;
@@ -3984,19 +3928,15 @@ ixgbe_dev_link_update_share(struct rte_eth_dev *dev,if (diag !=0 ) { link.link_speed =ETH_SPEED_NUM_100M ; link.link_duplex =ETH_LINK_FULL_DUPLEX ; -rte_ixgbe_dev_atomic_write_link_status (dev, &link); -if (link.link_status == old.link_status) -return -1 ; -return 0 ; + +return _rte_eth_linkstatus_set (dev, &link); }if (link_up ==0 ) { -rte_ixgbe_dev_atomic_write_link_status (dev, &link); intr->flags |=IXGBE_FLAG_NEED_LINK_CONFIG ; -if (link.link_status == old.link_status) -return -1 ; -return 0 ; +return _rte_eth_linkstatus_set (dev, &link); } + intr->flags &= ~IXGBE_FLAG_NEED_LINK_CONFIG ; link.link_status =ETH_LINK_UP ; link.link_duplex =ETH_LINK_FULL_DUPLEX ;
@@ -4028,12 +3968,8 @@ ixgbe_dev_link_update_share(struct rte_eth_dev *dev, link.link_speed =ETH_SPEED_NUM_10G ;break ; } -rte_ixgbe_dev_atomic_write_link_status (dev, &link); -if (link.link_status == old.link_status) -return -1 ; - -return 0 ; +return _rte_eth_linkstatus_set (dev, &link); } static int
@@ -4232,8 +4168,8 @@ ixgbe_dev_link_status_print(struct rte_eth_dev *dev)struct rte_pci_device *pci_dev =RTE_ETH_DEV_TO_PCI (dev);struct rte_eth_link link; -memset (&link,0 ,sizeof (link)); -rte_ixgbe_dev_atomic_read_link_status (dev, &link); +_rte_eth_linkstatus_get (dev, &link); +if (link.link_status) { PMD_INIT_LOG(INFO, "Port %d: Link Up - speed %u Mbps - %s", (int)(dev->data->port_id),
@@ -4268,7 +4204,6 @@ ixgbe_dev_interrupt_action(struct rte_eth_dev *dev,struct ixgbe_interrupt *intr =IXGBE_DEV_PRIVATE_TO_INTR (dev->data->dev_private);int64_t timeout; -struct rte_eth_link link;struct ixgbe_hw *hw =IXGBE_DEV_PRIVATE_TO_HW (dev->data->dev_private);
@@ -4285,9 +4220,10 @@ ixgbe_dev_interrupt_action(struct rte_eth_dev *dev, }if (intr->flags &IXGBE_FLAG_NEED_LINK_UPDATE ) { +struct rte_eth_link link; +/* get the link status before link update, for predicting later */ -memset (&link,0 ,sizeof (link)); -rte_ixgbe_dev_atomic_read_link_status (dev, &link); +_rte_eth_linkstatus_get (dev, &link);ixgbe_dev_link_update (dev,0 );
@@ -6806,9 +6742,8 @@ ixgbe_start_timecounters(struct rte_eth_dev *dev)uint32_t shift =0 ;/* Get current link speed. */ -memset (&link,0 ,sizeof (link));ixgbe_dev_link_update (dev,1 ); -rte_ixgbe_dev_atomic_read_link_status (dev, &link); +_rte_eth_linkstatus_get (dev, &link);switch (link.link_speed) {case ETH_SPEED_NUM_100M :
Signature · 1 line
2.14.3
PATCH v4 9/15 net/sfc: use new rte_eth_linkstatus functions
In reply to Ferruh YigitFrom: Stephen Hemminger <[email protected]>
Use the new API (_rte_eth_linkstatus_set) to handle link status update.
Signed-off-by Stephen Hemminger <[email protected]>
--- drivers/net/sfc/sfc_ethdev.c | 27 +++++++-------------------- drivers/net/sfc/sfc_ev.c | 23 ++++------------------- 2 files changed, 11 insertions(+), 39 deletions(-)
drivers/net/sfc/sfc_ethdev.c +7 -20
@@ -232,22 +232,12 @@ static int sfc_dev_link_update(struct rte_eth_dev *dev, int wait_to_complete) {struct sfc_adapter *sa = dev->data->dev_private; -struct rte_eth_link *dev_link = &dev->data->dev_link; -struct rte_eth_link old_link;struct rte_eth_link current_link;sfc_log_init (sa,"entry" ); -retry: -EFX_STATIC_ASSERT (sizeof (*dev_link) ==sizeof (rte_atomic64_t)); - *(int64_t *)&old_link =rte_atomic64_read ((rte_atomic64_t *)dev_link); -if (sa->state !=SFC_ADAPTER_STARTED ) {sfc_port_link_mode_to_info (EFX_LINK_UNKNOWN , ¤t_link); -if (!rte_atomic64_cmpset ((volatile uint64_t *)dev_link, - *(uint64_t *)&old_link, - *(uint64_t *)¤t_link)) - goto retry; }else if (wait_to_complete) {efx_link_mode_t link_mode;
@@ -255,21 +245,18 @@ sfc_dev_link_update(struct rte_eth_dev *dev, int wait_to_complete) link_mode =EFX_LINK_UNKNOWN ;sfc_port_link_mode_to_info (link_mode, ¤t_link); -if (!rte_atomic64_cmpset ((volatile uint64_t *)dev_link, - *(uint64_t *)&old_link, - *(uint64_t *)¤t_link)) - goto retry; }else {sfc_ev_mgmt_qpoll (sa); - *(int64_t *)¤t_link = -rte_atomic64_read ((rte_atomic64_t *)dev_link); +_rte_eth_linkstatus_get (dev, ¤t_link); } -if (old_link.link_status != current_link.link_status) -sfc_info (sa,"Link status is %s" , - current_link.link_status ?"UP" :"DOWN" ); +if (_rte_eth_linkstatus_set (dev, ¤t_link) ==0 ) +return 0 ; + +sfc_info (sa,"Link status is %s" , + current_link.link_status ?"UP" :"DOWN" ); -return old_link.link_status == current_link.link_status ?0 :-1 ; +return -1 ; } static void
drivers/net/sfc/sfc_ev.c +4 -19
@@ -382,29 +382,14 @@ sfc_ev_link_change(void *arg, efx_link_mode_t link_mode) {struct sfc_evq *evq = arg;struct sfc_adapter *sa = evq->sa; -struct rte_eth_link *dev_link = &sa->eth_dev->data->dev_link;struct rte_eth_link new_link; -uint64_t new_link_u64; -uint64_t old_link_u64; - -EFX_STATIC_ASSERT (sizeof (*dev_link) ==sizeof (rte_atomic64_t));sfc_port_link_mode_to_info (link_mode, &new_link); +if (_rte_eth_linkstatus_set (sa->eth_dev, &new_link) ==0 ) +return B_FALSE ; - new_link_u64 = *(uint64_t *)&new_link; -do { - old_link_u64 =rte_atomic64_read ((rte_atomic64_t *)dev_link); -if (old_link_u64 == new_link_u64) -break ; - -if (rte_atomic64_cmpset ((volatile uint64_t *)dev_link, - old_link_u64, new_link_u64)) { - evq->sa->port.lsc_seq++; -break ; - } - }while (B_TRUE ); - -return B_FALSE ; + evq->sa->port.lsc_seq++; +return B_TRUE ; }static const efx_ev_callbacks_t sfc_ev_callbacks = {
Signature · 1 line
2.14.3
Re: [PATCH v4 09/15] net/sfc: use new rte_eth_linkstatus functions
Quoted 9 lines · Ferruh Yigit
From: Stephen Hemminger <[email protected]>
Use the new API (
_rte_eth_linkstatus_set) to handle link status update.Signed-off-by Stephen Hemminger <[email protected]>--- drivers/net/sfc/sfc_ethdev.c | 27 +++++++-------------------- drivers/net/sfc/sfc_ev.c | 23 ++++------------------- 2 files changed, 11 insertions(+), 39 deletions(-)
<...>
Quoted 28 lines
drivers/net/sfc/sfc_ev.c +2
-14@@ -382,29 +382,14 @@ sfc_ev_link_change(void *arg, efx_link_mode_t link_mode) {struct sfc_evq *evq = arg;struct sfc_adapter *sa = evq->sa; -struct rte_eth_link *dev_link = &sa->eth_dev->data->dev_link;struct rte_eth_link new_link; -uint64_t new_link_u64; -uint64_t old_link_u64; - -EFX_STATIC_ASSERT (sizeof (*dev_link) ==sizeof (rte_atomic64_t));sfc_port_link_mode_to_info (link_mode, &new_link); +if (_rte_eth_linkstatus_set (sa->eth_dev, &new_link) ==0 ) +return B_FALSE ; - new_link_u64 = *(uint64_t *)&new_link; - do { - old_link_u64 = rte_atomic64_read((rte_atomic64_t *)dev_link); - if (old_link_u64 == new_link_u64) - break; - - if (rte_atomic64_cmpset((volatile uint64_t *)dev_link, - old_link_u64, new_link_u64)) { - evq->sa->port.lsc_seq++;
drivers/net/sfc/sfc_ev.c sfc_ev_link_change(void *arg, efx_link_mode_t link_mode) · line 395
Quoted 10 lines
drivers/net/sfc/sfc_ev.c +2
-5-break ; - } - }while (B_TRUE ); - -return B_FALSE ; + evq->sa->port.lsc_seq++; +return B_TRUE ; }static const efx_ev_callbacks_t sfc_ev_callbacks = {
I've provided review notes for the callback a couple of times already, but it is ignored.
The callback returns B_FALSE before the change and it must be preserved. The return value must not depend on link status changes. lsc_seq should be incremented in the case of any changes in link, not up/down changes only and it should be preserved.
PATCH v4 10/15 net/i40e: use rte_eth_linkstatus functions
In reply to Ferruh YigitFrom: Stephen Hemminger <[email protected]>
Use new rte_linkstatus update API
Signed-off-by Stephen Hemminger <[email protected]>
--- drivers/net/i40e/i40e_ethdev.c | 44 ++++++--------------------------------- drivers/net/i40e/i40e_ethdev_vf.c | 19 ++--------------- 2 files changed, 8 insertions(+), 55 deletions(-)
drivers/net/i40e/i40e_ethdev.c +6 -38
@@ -628,34 +628,6 @@ static struct rte_pci_driver rte_i40e_pmd = { .remove = eth_i40e_pci_remove, }; -static inline int -rte_i40e_dev_atomic_read_link_status (struct rte_eth_dev *dev, -struct rte_eth_link *link) -{ -struct rte_eth_link *dst = link; -struct rte_eth_link *src = &(dev->data->dev_link); - -if (rte_atomic64_cmpset ((uint64_t *)dst, *(uint64_t *)dst, - *(uint64_t *)src) ==0 ) -return -1 ; - -return 0 ; -} - -static inline int -rte_i40e_dev_atomic_write_link_status (struct rte_eth_dev *dev, -struct rte_eth_link *link) -{ -struct rte_eth_link *dst = &(dev->data->dev_link); -struct rte_eth_link *src = link; - -if (rte_atomic64_cmpset ((uint64_t *)dst, *(uint64_t *)dst, - *(uint64_t *)src) ==0 ) -return -1 ; - -return 0 ; -} -RTE_PMD_REGISTER_PCI (net_i40e, rte_i40e_pmd);RTE_PMD_REGISTER_PCI_TABLE (net_i40e, pci_id_i40e_map);RTE_PMD_REGISTER_KMOD_DEP (net_i40e,"* igb_uio | uio_pci_generic | vfio-pci" );
@@ -2345,17 +2317,16 @@ i40e_dev_link_update(struct rte_eth_dev *dev,#define MAX_REPEAT_TIME 10/* 1s (10 * 100ms) in total */ struct i40e_hw *hw =I40E_DEV_PRIVATE_TO_HW (dev->data->dev_private);struct i40e_link_status link_status; -struct rte_eth_link link, old; +struct rte_eth_link link;int status;unsigned rep_cnt =MAX_REPEAT_TIME ;bool enable_lse = dev->data->dev_conf.intr_conf.lsc ? true : false;memset (&link,0 ,sizeof (link)); -memset (&old,0 ,sizeof (old)); -memset (&link_status,0 ,sizeof (link_status)); -rte_i40e_dev_atomic_read_link_status (dev, &old); do { + memset(&link_status, 0, sizeof(link_status)); + /* Get link status information from hardware */ status = i40e_aq_get_link_info(hw, enable_lse, &link_status, NULL);
@@ -2408,13 +2379,10 @@ i40e_dev_link_update(struct rte_eth_dev *dev, ETH_LINK_SPEED_FIXED); out: -rte_i40e_dev_atomic_write_link_status (dev, &link); -if (link.link_status == old.link_status) -return -1 ; - + status =_rte_eth_linkstatus_set (dev, &link);i40e_notify_all_vfs_link_status (dev); -return 0 ; +return status; }/* Get all the statistics of a VSI */
@@ -10032,7 +10000,7 @@ i40e_start_timecounters(struct rte_eth_dev *dev)/* Get current link speed. */ memset (&link,0 ,sizeof (link));i40e_dev_link_update (dev,1 ); -rte_i40e_dev_atomic_read_link_status (dev, &link); +_rte_eth_linkstatus_get (dev, &link);switch (link.link_speed) {case ETH_SPEED_NUM_40G :
drivers/net/i40e/i40e_ethdev_vf.c +2 -17
@@ -1034,20 +1034,6 @@ static const struct rte_pci_id pci_id_i40evf_map[] = { { .vendor_id = 0, /* sentinel */ }, }; -static inline int -i40evf_dev_atomic_write_link_status (struct rte_eth_dev *dev, -struct rte_eth_link *link) -{ -struct rte_eth_link *dst = &(dev->data->dev_link); -struct rte_eth_link *src = link; - -if (rte_atomic64_cmpset ((uint64_t *)dst, *(uint64_t *)dst, - *(uint64_t *)src) ==0 ) -return -1 ; - -return 0 ; -} -/* Disable IRQ0 */ static inline void i40evf_disable_irq0 (struct i40e_hw *hw)
@@ -2075,6 +2061,7 @@ i40evf_dev_link_update(struct rte_eth_dev *dev,* while Linux driver does not */ +memset (&new_link,0 ,sizeof (new_link));/* Linux driver PF host */ switch (vf->link_speed) {case I40E_LINK_SPEED_100MB :
@@ -2106,9 +2093,7 @@ i40evf_dev_link_update(struct rte_eth_dev *dev, new_link.link_autoneg = dev->data->dev_conf.link_speeds Ð_LINK_SPEED_FIXED ; -i40evf_dev_atomic_write_link_status (dev, &new_link); - -return 0 ; +return _rte_eth_linkstatus_set (dev, &new_link); } static void
Signature · 1 line
2.14.3
PATCH v4 11/15 net/liquidio: use _rte_eth_linkstatus_set
In reply to Ferruh YigitFrom: Stephen Hemminger <[email protected]>
Use the new link update API, and cleanup the logic in the the link update routine.
Tested-by Shijith Thotton <[email protected]>
Signed-off-by Stephen Hemminger <[email protected]>
--- drivers/net/liquidio/lio_ethdev.c | 54 ++++++--------------------------------- 1 file changed, 8 insertions(+), 46 deletions(-)
drivers/net/liquidio/lio_ethdev.c +8 -46
@@ -904,32 +904,6 @@ lio_dev_vlan_filter_set(struct rte_eth_dev *eth_dev, uint16_t vlan_id, int on)return 0 ; } -/** -* Atomically writes the link status information into global -* structure rte_eth_dev. -* -* @param eth_dev -* - Pointer to the structure rte_eth_dev to read from. -* - Pointer to the buffer to be saved with the link status. -* -* @return -* - On success, zero. -* - On failure, negative value. -*/ -static inline int -lio_dev_atomic_write_link_status (struct rte_eth_dev *eth_dev, -struct rte_eth_link *link) -{ -struct rte_eth_link *dst = ð_dev->data->dev_link; -struct rte_eth_link *src = link; - -if (rte_atomic64_cmpset ((uint64_t *)dst, *(uint64_t *)dst, - *(uint64_t *)src) ==0 ) -return -1 ; - -return 0 ; -} -static uint64_t lio_hweight64 (uint64_t w) {
@@ -949,23 +923,17 @@ lio_dev_link_update(struct rte_eth_dev *eth_dev, int wait_to_complete __rte_unused) {struct lio_device *lio_dev =LIO_DEV (eth_dev); -struct rte_eth_link link, old; - -/* Initialize */ - link.link_status =ETH_LINK_DOWN ; - link.link_speed =ETH_SPEED_NUM_NONE ; - link.link_duplex =ETH_LINK_HALF_DUPLEX ; - link.link_autoneg =ETH_LINK_AUTONEG ; -memset (&old,0 ,sizeof (old)); +struct rte_eth_link link = { + .link_status =ETH_LINK_DOWN , + .link_speed =ETH_SPEED_NUM_NONE , + .link_duplex =ETH_LINK_HALF_DUPLEX , + .link_autoneg =ETH_LINK_AUTONEG , + };/* Return what we found */ if (lio_dev->linfo.link.s.link_up ==0 ) {/* Interface is down */ -if (lio_dev_atomic_write_link_status (eth_dev, &link)) -return -1 ; -if (link.link_status == old.link_status) -return -1 ; -return 0 ; +return _rte_eth_linkstatus_set (eth_dev, &link); } link.link_status =ETH_LINK_UP ;/* Interface is up */
@@ -982,13 +950,7 @@ lio_dev_link_update(struct rte_eth_dev *eth_dev, link.link_duplex =ETH_LINK_HALF_DUPLEX ; } -if (lio_dev_atomic_write_link_status (eth_dev, &link)) -return -1 ; - -if (link.link_status == old.link_status) -return -1 ; - -return 0 ; +return _rte_eth_linkstatus_set (eth_dev, &link); }/**
Signature · 1 line
2.14.3
PATCH v4 12/15 net/thunderx: use _rte_eth_linkstatus_set
In reply to Ferruh YigitFrom: Stephen Hemminger <[email protected]>
Use new helper function.
Signed-off-by Stephen Hemminger <[email protected]>
--- drivers/net/thunderx/nicvf_ethdev.c | 18 ++---------------- 1 file changed, 2 insertions(+), 16 deletions(-)
drivers/net/thunderx/nicvf_ethdev.c +2 -16
@@ -15,7 +15,6 @@#include <sys/queue.h> #include <rte_alarm.h> -#include <rte_atomic.h> #include <rte_branch_prediction.h> #include <rte_byteorder.h> #include <rte_common.h>
@@ -69,20 +68,6 @@ nicvf_init_log(void)rte_log_set_level (nicvf_logtype_driver,RTE_LOG_NOTICE ); } -static inline int -nicvf_atomic_write_link_status (struct rte_eth_dev *dev, -struct rte_eth_link *link) -{ -struct rte_eth_link *dst = &dev->data->dev_link; -struct rte_eth_link *src = link; - -if (rte_atomic64_cmpset ((uint64_t *)dst, *(uint64_t *)dst, - *(uint64_t *)src) ==0 ) -return -1 ; - -return 0 ; -} -static inline void nicvf_set_eth_link_status (struct nicvf *nic,struct rte_eth_link *link) {
@@ -163,7 +148,8 @@ nicvf_dev_link_update(struct rte_eth_dev *dev, int wait_to_complete)memset (&link,0 ,sizeof (link));nicvf_set_eth_link_status (nic, &link); } -return nicvf_atomic_write_link_status (dev, &link); + +return _rte_eth_linkstatus_set (dev, &link); } static int
Signature · 1 line
2.14.3
PATCH v4 13/15 net/szedata: use _rte_eth_linkstatus_set
In reply to Ferruh YigitFrom: Stephen Hemminger <[email protected]>
Yet another driver which was not returing correct value on link change.
Since this driver can't be built on x86 could not even do a compile test.
Signed-off-by Stephen Hemminger <[email protected]>
--- drivers/net/szedata2/rte_eth_szedata2.c | 20 ++++++-------------- 1 file changed, 6 insertions(+), 14 deletions(-)
drivers/net/szedata2/rte_eth_szedata2.c +6 -14
@@ -50,7 +50,6 @@#include <rte_memcpy.h> #include <rte_kvargs.h> #include <rte_dev.h> -#include <rte_atomic.h> #include "rte_eth_szedata2.h" #include "szedata2_iobuf.h"
@@ -1173,14 +1172,15 @@ static int eth_link_update(struct rte_eth_dev *dev, int wait_to_complete __rte_unused) { - struct rte_eth_link link; -struct rte_eth_link *link_ptr = &link; -struct rte_eth_link *dev_link = &dev->data->dev_link; struct pmd_internals *internals = (struct pmd_internals *) dev->data->dev_private;const volatile struct szedata2_ibuf *ibuf;uint32_t i;bool link_is_up = false; +struct rte_eth_link link = { + .link_duplex =ETH_LINK_FULL_DUPLEX , + .link_autoneg =ETH_LINK_FIXED , + };switch (get_link_speed (internals)) {case SZEDATA2_LINK_SPEED_10G :
@@ -1197,9 +1197,6 @@ eth_link_update(struct rte_eth_dev *dev,break ; } -/* szedata2 uses only full duplex */ - link.link_duplex =ETH_LINK_FULL_DUPLEX ; -for (i =0 ; i < szedata2_ibuf_count; i++) { ibuf =ibuf_ptr_by_index (internals->pci_rsc, i);/*
@@ -1212,14 +1209,9 @@ eth_link_update(struct rte_eth_dev *dev, } } - link.link_status = (link_is_up) ?ETH_LINK_UP :ETH_LINK_DOWN ; - - link.link_autoneg =ETH_LINK_FIXED ; + link.link_status = link_is_up ?ETH_LINK_UP :ETH_LINK_DOWN ; -rte_atomic64_cmpset ((uint64_t *)dev_link, *(uint64_t *)dev_link, - *(uint64_t *)link_ptr); - -return 0 ; +return _rte_eth_linkstatus_set (dev, &link); } static int
Signature · 1 line
2.14.3
PATCH v4 14/15 net/octeontx: use rte_eth_linkstatus_set
In reply to Ferruh YigitFrom: Stephen Hemminger <[email protected]>
Common function matches this drivers usage.
Signed-off-by Stephen Hemminger <[email protected]>
--- drivers/net/octeontx/octeontx_ethdev.c | 16 +--------------- 1 file changed, 1 insertion(+), 15 deletions(-)
drivers/net/octeontx/octeontx_ethdev.c +1 -15
@@ -487,20 +487,6 @@ octeontx_dev_promisc_disable(struct rte_eth_dev *dev)octeontx_port_promisc_set (nic,0 ); } -static inline int -octeontx_atomic_write_link_status (struct rte_eth_dev *dev, -struct rte_eth_link *link) -{ -struct rte_eth_link *dst = &dev->data->dev_link; -struct rte_eth_link *src = link; - -if (rte_atomic64_cmpset ((uint64_t *)dst, *(uint64_t *)dst, - *(uint64_t *)src) ==0 ) -return -1 ; - -return 0 ; -} -static int octeontx_port_link_status (struct octeontx_nic *nic) {
@@ -572,7 +558,7 @@ octeontx_dev_link_update(struct rte_eth_dev *dev, link.link_duplex =ETH_LINK_FULL_DUPLEX ; link.link_autoneg =ETH_LINK_AUTONEG ; -return octeontx_atomic_write_link_status (dev, &link); +return _rte_eth_linkstatus_set (dev, &link); } static int
Signature · 1 line
2.14.3
PATCH v4 15/15 net/enic: use _rte_eth_linkstatus_set
In reply to Ferruh YigitFrom: Stephen Hemminger <[email protected]>
This driver was not doing atomic update of link status information. And the return value was different than others.
The hardware also does not do autonegotiation (at least on Linux).
Signed-off-by Stephen Hemminger <[email protected]>
--- drivers/net/enic/enic_ethdev.c | 5 ++--- drivers/net/enic/enic_main.c | 16 +++++++--------- 2 files changed, 9 insertions(+), 12 deletions(-)
drivers/net/enic/enic_ethdev.c +2 -3
@@ -426,10 +426,9 @@ static void enicpmd_dev_stop(struct rte_eth_dev *eth_dev)ENICPMD_FUNC_TRACE ();enic_disable (enic); +memset (&link,0 ,sizeof (link)); -rte_atomic64_cmpset ((uint64_t *)ð_dev->data->dev_link, - *(uint64_t *)ð_dev->data->dev_link, - *(uint64_t *)&link); +_rte_eth_linkstatus_set (eth_dev, &link); }/*
drivers/net/enic/enic_main.c +7 -9
@@ -379,16 +379,14 @@ enic_free_consistent(void *priv,int enic_link_update (struct enic *enic) {struct rte_eth_dev *eth_dev = enic->rte_dev; -int ret; -int link_status =0 ; +struct rte_eth_link link = { + .link_status =enic_get_link_status (enic), + .link_duplex =ETH_LINK_FULL_DUPLEX , + .link_speed =vnic_dev_port_speed (enic->vdev), + .link_autoneg =ETH_LINK_FIXED , + }; - link_status =enic_get_link_status (enic); - ret = (link_status == enic->link_status); - enic->link_status = link_status; - eth_dev->data->dev_link.link_status = link_status; - eth_dev->data->dev_link.link_duplex =ETH_LINK_FULL_DUPLEX ; - eth_dev->data->dev_link.link_speed =vnic_dev_port_speed (enic->vdev); -return ret; +return _rte_eth_linkstatus_set (eth_dev, &link); } static void
Signature · 1 line
2.14.3
Re: [PATCH v4 15/15] net/enic: use _rte_eth_linkstatus_set
Quoted 8 lines · Ferruh Yigit
From: Stephen Hemminger <[email protected]>
This driver was not doing atomic update of link status information. And the return value was different than others.
The hardware also does not do autonegotiation (at least on Linux).Signed-off-by Stephen Hemminger <[email protected]>---
Here is an ack, in case you need it. Please feel free to add it to v4 if you make it (i.e. dropping the leading _).
Acked-by Hyong Youb Kim <[email protected]>
FWIW, the NIC hardware (Cisco VIC) does autonegotiate. But, the link settings (autoneg/fixed/10/25/40) are controlled by a management entity (e.g. UCS manager). The drivers including the netdev enic driver cannot change them and currently do not know if the current speed has been autonegotiated or fixed by admin. So, they simply report the current link speed as fixed. I know, it is a little unconventional.
-Hyong